diff mbox

ASoC: nau8825: extend FLL function

Message ID 1456683827-6597-1-git-send-email-KCHSU0@nuvoton.com (mailing list archive)
State New, archived
Headers show

Commit Message

AS50 KCHSU0 Feb. 28, 2016, 6:23 p.m. UTC 
Extend FLL clock source from MCLK/BCLK/FS.
Modify FLL calculation and parameter setting.

Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
---
 sound/soc/codecs/nau8825.c | 128 ++++++++++++++++++++++++++++++++-------------
 sound/soc/codecs/nau8825.h |  21 ++++++--
 2 files changed, 108 insertions(+), 41 deletions(-)

Comments

Mark Brown March 1, 2016, 3:23 a.m. UTC | #1 
On Mon, Feb 29, 2016 at 02:23:47AM +0800, John Hsu wrote:
> Extend FLL clock source from MCLK/BCLK/FS.
> Modify FLL calculation and parameter setting.

I'm sorry but this really doesn't tell me very much about what this is
supposed to do so it's very hard to tell if the change does it or not.
It also sounds like there's multiple different changes going on here, if
nothing else it sounds like there's the addition of some new sources for
the FLL and also some unspecified changes to the calculations.  Separate
changes should be in separate patches.

> +	/* We selected MCLK source but the clock itself managed externally */
> +	if (!nau8825->mclk)
> +		return 0;
> +

That comment sounds *very* suspicous, if we are using MCLK we should
manage it via the clock API.  If the platform doesn't have good clock
support we should fix the platform.

> +	if (nau8825->mclk_freq != freq) {
> +		nau8825->mclk_freq = freq;
> +
> +		freq = clk_round_rate(nau8825->mclk, freq);
> +		ret = clk_set_rate(nau8825->mclk, freq);
> +		if (ret) {
> +			dev_err(nau8825->dev, "Unable to set mclk rate\n");
> +			return ret;
> +		}

We store the frequency even if we failed to set it.  This means that if
we retry we'll skip over setting which is buggy.
AS50 KCHSU0 March 8, 2016, 11:58 a.m. UTC | #2 
Hi,

On 3/1/2016 11:23 AM, Mark Brown wrote:
> On Mon, Feb 29, 2016 at 02:23:47AM +0800, John Hsu wrote:
>   
>> Extend FLL clock source from MCLK/BCLK/FS.
>> Modify FLL calculation and parameter setting.
>>     
>
> I'm sorry but this really doesn't tell me very much about what this is
> supposed to do so it's very hard to tell if the change does it or not.
> It also sounds like there's multiple different changes going on here, if
> nothing else it sounds like there's the addition of some new sources for
> the FLL and also some unspecified changes to the calculations.  Separate
> changes should be in separate patches.
>
>   
In the patch, we add FLL clock source selection. The source can be from 
MCLK, BCLK or FS.
Besides, driver extend higher frequency for better performance in FLL 
calculation,
and has different register apply if fraction or not. Just separate it. 
Right?

>> +	/* We selected MCLK source but the clock itself managed externally */
>> +	if (!nau8825->mclk)
>> +		return 0;
>> +
>>     
>
> That comment sounds *very* suspicous, if we are using MCLK we should
> manage it via the clock API.  If the platform doesn't have good clock
> support we should fix the platform.
>
>   
In initiation, we get mclk object from platform as the following code.
If the mclk is not found, we don't need to prepare it in the driver.

    nau8825->mclk = devm_clk_get(dev, "mclk");
    ...
    } else if (PTR_ERR(nau8825->mclk) == -ENOENT) {
        /* The MCLK is managed externally or not used at all */
        nau8825->mclk = NULL;
        dev_info(dev, "No 'mclk' clock found, assume MCLK is managed 
externally");


>> +	if (nau8825->mclk_freq != freq) {
>> +		nau8825->mclk_freq = freq;
>> +
>> +		freq = clk_round_rate(nau8825->mclk, freq);
>> +		ret = clk_set_rate(nau8825->mclk, freq);
>> +		if (ret) {
>> +			dev_err(nau8825->dev, "Unable to set mclk rate\n");
>> +			return ret;
>> +		}
>>     
>
> We store the frequency even if we failed to set it.  This means that if
> we retry we'll skip over setting which is buggy.
>   
Yes, it's better to store it when everything get done correctly.
Mark Brown March 9, 2016, 2:40 a.m. UTC | #3 
On Tue, Mar 08, 2016 at 07:58:53PM +0800, John Hsu wrote:

> In the patch, we add FLL clock source selection. The source can be from
> MCLK, BCLK or FS.
> Besides, driver extend higher frequency for better performance in FLL
> calculation,
> and has different register apply if fraction or not. Just separate it.
> Right?

I think that's what I was asking for, yes.

> >That comment sounds *very* suspicous, if we are using MCLK we should
> >manage it via the clock API.  If the platform doesn't have good clock
> >support we should fix the platform.

> In initiation, we get mclk object from platform as the following code.
> If the mclk is not found, we don't need to prepare it in the driver.

>    nau8825->mclk = devm_clk_get(dev, "mclk");
>    ...
>    } else if (PTR_ERR(nau8825->mclk) == -ENOENT) {
>        /* The MCLK is managed externally or not used at all */
>        nau8825->mclk = NULL;
>        dev_info(dev, "No 'mclk' clock found, assume MCLK is managed
> externally");

This really isn't a good way to be handling things, you should be
ensuring that platforms that have an MCLK provide one via the clock API.
If the clock is missing that should indicate that it's the second case
where it's not used at all.
AS50 KCHSU0 March 10, 2016, 4:31 a.m. UTC | #4 
On 3/9/2016 10:40 AM, Mark Brown wrote:
> On Tue, Mar 08, 2016 at 07:58:53PM +0800, John Hsu wrote:
>
>   
>> In the patch, we add FLL clock source selection. The source can be from
>> MCLK, BCLK or FS.
>> Besides, driver extend higher frequency for better performance in FLL
>> calculation,
>> and has different register apply if fraction or not. Just separate it.
>> Right?
>>     
>
> I think that's what I was asking for, yes.
>   

I'll split these solution into two patches later and resubmit.

>   
>>> That comment sounds *very* suspicous, if we are using MCLK we should
>>> manage it via the clock API.  If the platform doesn't have good clock
>>> support we should fix the platform.
>>>       
>
>   
>> In initiation, we get mclk object from platform as the following code.
>> If the mclk is not found, we don't need to prepare it in the driver.
>>     
>
>   
>>    nau8825->mclk = devm_clk_get(dev, "mclk");
>>    ...
>>    } else if (PTR_ERR(nau8825->mclk) == -ENOENT) {
>>        /* The MCLK is managed externally or not used at all */
>>        nau8825->mclk = NULL;
>>        dev_info(dev, "No 'mclk' clock found, assume MCLK is managed
>> externally");
>>     
>
> This really isn't a good way to be handling things, you should be
> ensuring that platforms that have an MCLK provide one via the clock API.
> If the clock is missing that should indicate that it's the second case
> where it's not used at all.
>   

Do you mean I should use devm_clk_get to check mclk exist instead of
nau8825->mclk in nau8825_mclk_prepare function? And return a warn-
ing message if mclk is missing?
Mark Brown March 10, 2016, 5:24 a.m. UTC | #5 
On Thu, Mar 10, 2016 at 12:31:54PM +0800, John Hsu wrote:
> On 3/9/2016 10:40 AM, Mark Brown wrote:

> >This really isn't a good way to be handling things, you should be
> >ensuring that platforms that have an MCLK provide one via the clock API.
> >If the clock is missing that should indicate that it's the second case
> >where it's not used at all.

> Do you mean I should use devm_clk_get to check mclk exist instead of
> nau8825->mclk in nau8825_mclk_prepare function? And return a warn-
> ing message if mclk is missing?

That might make sense, yes.  Let's see the code.
diff mbox

Patch

diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index c1b87c5..504c969 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -31,7 +31,7 @@ 
 #include "nau8825.h"
 
 #define NAU_FREF_MAX 13500000
-#define NAU_FVCO_MAX 100000000
+#define NAU_FVCO_MAX 124000000
 #define NAU_FVCO_MIN 90000000
 
 struct nau8825_fll {
@@ -874,8 +874,8 @@  static int nau8825_codec_probe(struct snd_soc_codec *codec)
 static int nau8825_calc_fll_param(unsigned int fll_in, unsigned int fs,
 		struct nau8825_fll *fll_param)
 {
-	u64 fvco;
-	unsigned int fref, i;
+	u64 fvco, fvco_max;
+	unsigned int fref, i, fvco_sel;
 
 	/* Ensure the reference clock frequency (FREF) is <= 13.5MHz by dividing
 	 * freq_in by 1, 2, 4, or 8 using FLL pre-scalar.
@@ -900,18 +900,23 @@  static int nau8825_calc_fll_param(unsigned int fll_in, unsigned int fs,
 	fll_param->ratio = fll_ratio[i].val;
 
 	/* Calculate the frequency of DCO (FDCO) given freq_out = 256 * Fs.
-	 * FDCO must be within the 90MHz - 100MHz or the FFL cannot be
+	 * FDCO must be within the 90MHz - 124MHz or the FFL cannot be
 	 * guaranteed across the full range of operation.
 	 * FDCO = freq_out * 2 * mclk_src_scaling
 	 */
+	fvco_max = 0;
+	fvco_sel = ARRAY_SIZE(mclk_src_scaling);
 	for (i = 0; i < ARRAY_SIZE(mclk_src_scaling); i++) {
 		fvco = 256 * fs * 2 * mclk_src_scaling[i].param;
-		if (NAU_FVCO_MIN < fvco && fvco < NAU_FVCO_MAX)
-			break;
+		if (fvco > NAU_FVCO_MIN && fvco < NAU_FVCO_MAX &&
+			fvco_max < fvco) {
+			fvco_max = fvco;
+			fvco_sel = i;
+		}
 	}
-	if (i == ARRAY_SIZE(mclk_src_scaling))
+	if (ARRAY_SIZE(mclk_src_scaling) == fvco_sel)
 		return -EINVAL;
-	fll_param->mclk_src = mclk_src_scaling[i].val;
+	fll_param->mclk_src = mclk_src_scaling[fvco_sel].val;
 
 	/* Calculate the FLL 10-bit integer input and the FLL 16-bit fractional
 	 * input based on FDCO, FREF and FLL ratio.
@@ -926,7 +931,8 @@  static void nau8825_fll_apply(struct nau8825 *nau8825,
 		struct nau8825_fll *fll_param)
 {
 	regmap_update_bits(nau8825->regmap, NAU8825_REG_CLK_DIVIDER,
-		NAU8825_CLK_MCLK_SRC_MASK, fll_param->mclk_src);
+		NAU8825_CLK_SRC_MASK | NAU8825_CLK_MCLK_SRC_MASK,
+		NAU8825_CLK_SRC_MCLK | fll_param->mclk_src);
 	regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL1,
 			NAU8825_FLL_RATIO_MASK, fll_param->ratio);
 	/* FLL 16-bit fractional input */
@@ -939,10 +945,25 @@  static void nau8825_fll_apply(struct nau8825 *nau8825,
 			NAU8825_FLL_REF_DIV_MASK, fll_param->clk_ref_div);
 	/* select divided VCO input */
 	regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL5,
-			NAU8825_FLL_FILTER_SW_MASK, 0x0000);
-	/* FLL sigma delta modulator enable */
-	regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
-			NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
+		NAU8825_FLL_CLK_SW_MASK, NAU8825_FLL_CLK_SW_REF);
+	/* Disable free-running mode */
+	regmap_update_bits(nau8825->regmap,
+		NAU8825_REG_FLL6, NAU8825_DCO_EN, 0);
+	if (fll_param->fll_frac) {
+		regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL5,
+			NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
+			NAU8825_FLL_FTR_SW_MASK,
+			NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
+			NAU8825_FLL_FTR_SW_FILTER);
+		regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
+			NAU8825_SDM_EN, NAU8825_SDM_EN);
+	} else {
+		regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL5,
+			NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
+			NAU8825_FLL_FTR_SW_MASK, NAU8825_FLL_FTR_SW_ACCU);
+		regmap_update_bits(nau8825->regmap,
+			NAU8825_REG_FLL6, NAU8825_SDM_EN, 0);
+	}
 }
 
 /* freq_out must be 256*Fs in order to achieve the best performance */
@@ -970,6 +991,36 @@  static int nau8825_set_pll(struct snd_soc_codec *codec, int pll_id, int source,
 	return 0;
 }
 
+static int nau8825_mclk_prepare(struct nau8825 *nau8825, unsigned int freq)
+{
+	int ret = 0;
+
+	/* We selected MCLK source but the clock itself managed externally */
+	if (!nau8825->mclk)
+		return 0;
+
+	if (!nau8825->mclk_freq) {
+		ret = clk_prepare_enable(nau8825->mclk);
+		if (ret) {
+			dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
+			return ret;
+		}
+	}
+
+	if (nau8825->mclk_freq != freq) {
+		nau8825->mclk_freq = freq;
+
+		freq = clk_round_rate(nau8825->mclk, freq);
+		ret = clk_set_rate(nau8825->mclk, freq);
+		if (ret) {
+			dev_err(nau8825->dev, "Unable to set mclk rate\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 	unsigned int freq)
 {
@@ -981,29 +1032,9 @@  static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 		regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
 			NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_MCLK);
 		regmap_update_bits(regmap, NAU8825_REG_FLL6, NAU8825_DCO_EN, 0);
-
-		/* We selected MCLK source but the clock itself managed externally */
-		if (!nau8825->mclk)
-			break;
-
-		if (!nau8825->mclk_freq) {
-			ret = clk_prepare_enable(nau8825->mclk);
-			if (ret) {
-				dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
-				return ret;
-			}
-		}
-
-		if (nau8825->mclk_freq != freq) {
-			nau8825->mclk_freq = freq;
-
-			freq = clk_round_rate(nau8825->mclk, freq);
-			ret = clk_set_rate(nau8825->mclk, freq);
-			if (ret) {
-				dev_err(nau8825->dev, "Unable to set mclk rate\n");
-				return ret;
-			}
-		}
+		ret = nau8825_mclk_prepare(nau8825, freq);
+		if (ret)
+			return ret;
 
 		break;
 	case NAU8825_CLK_INTERNAL:
@@ -1011,7 +1042,32 @@  static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 			NAU8825_DCO_EN);
 		regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
 			NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_VCO);
+		if (nau8825->mclk_freq) {
+			clk_disable_unprepare(nau8825->mclk);
+			nau8825->mclk_freq = 0;
+		}
+
+		break;
+	case NAU8825_CLK_FLL_MCLK:
+		regmap_update_bits(regmap, NAU8825_REG_FLL3,
+			NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_MCLK);
+		ret = nau8825_mclk_prepare(nau8825, freq);
+		if (ret)
+			return ret;
+
+		break;
+	case NAU8825_CLK_FLL_BLK:
+		regmap_update_bits(regmap, NAU8825_REG_FLL3,
+			NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_BLK);
+		if (nau8825->mclk_freq) {
+			clk_disable_unprepare(nau8825->mclk);
+			nau8825->mclk_freq = 0;
+		}
 
+		break;
+	case NAU8825_CLK_FLL_FS:
+		regmap_update_bits(regmap, NAU8825_REG_FLL3,
+			NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_FS);
 		if (nau8825->mclk_freq) {
 			clk_disable_unprepare(nau8825->mclk);
 			nau8825->mclk_freq = 0;
diff --git a/sound/soc/codecs/nau8825.h b/sound/soc/codecs/nau8825.h
index dff8edb..8237693 100644
--- a/sound/soc/codecs/nau8825.h
+++ b/sound/soc/codecs/nau8825.h
@@ -112,20 +112,28 @@ 
 
 /* FLL3 (0x06) */
 #define NAU8825_FLL_INTEGER_MASK		(0x3ff << 0)
+#define NAU8825_FLL_CLK_SRC_SFT		10
+#define NAU8825_FLL_CLK_SRC_MASK		(0x3 << NAU8825_FLL_CLK_SRC_SFT)
+#define NAU8825_FLL_CLK_SRC_MCLK		(0 << NAU8825_FLL_CLK_SRC_SFT)
+#define NAU8825_FLL_CLK_SRC_BLK		(0x2 << NAU8825_FLL_CLK_SRC_SFT)
+#define NAU8825_FLL_CLK_SRC_FS			(0x3 << NAU8825_FLL_CLK_SRC_SFT)
 
 /* FLL4 (0x07) */
 #define NAU8825_FLL_REF_DIV_MASK		(0x3 << 10)
 
 /* FLL5 (0x08) */
-#define NAU8825_FLL_FILTER_SW_MASK		(0x1 << 14)
+#define NAU8825_FLL_PDB_DAC_EN		(0x1 << 15)
+#define NAU8825_FLL_LOOP_FTR_EN		(0x1 << 14)
+#define NAU8825_FLL_CLK_SW_MASK		(0x1 << 13)
+#define NAU8825_FLL_CLK_SW_N2			(0x1 << 13)
+#define NAU8825_FLL_CLK_SW_REF		(0x0 << 13)
+#define NAU8825_FLL_FTR_SW_MASK		(0x1 << 12)
+#define NAU8825_FLL_FTR_SW_ACCU		(0x1 << 12)
+#define NAU8825_FLL_FTR_SW_FILTER		(0x0 << 12)
 
 /* FLL6 (0x9) */
-#define NAU8825_DCO_EN_MASK			(0x1 << 15)
 #define NAU8825_DCO_EN				(0x1 << 15)
-#define NAU8825_DCO_DIS				(0x0 << 15)
-#define NAU8825_SDM_EN_MASK			(0x1 << 14)
 #define NAU8825_SDM_EN				(0x1 << 14)
-#define NAU8825_SDM_DIS				(0x0 << 14)
 
 /* HSD_CTRL (0xc) */
 #define NAU8825_HSD_AUTO_MODE	(1 << 6)
@@ -306,6 +314,9 @@ 
 enum {
 	NAU8825_CLK_MCLK = 0,
 	NAU8825_CLK_INTERNAL,
+	NAU8825_CLK_FLL_MCLK,
+	NAU8825_CLK_FLL_BLK,
+	NAU8825_CLK_FLL_FS,
 };
 
 struct nau8825 {