[PATCH] ASoC: wm8985: rework and fix the clock calculation

[PATCH] ASoC: wm8985: rework and fix the clock calculation

[Petr Kulhavy]

The clock calculation has several issues:
1) if PLL is used in master mode the BCLK output runs at double the speed
2) de-facto only 44.1kHz and 48kHz sampling rates are supported, other
   rates like 8kHz, 12kHz, 24kHz fail to find the proper BCLK divider
3) the wm8985->sysclk variable has a misleading name and is used wrongly
   in the clock calculation in wm8985_hw_params() which is the root cause
   for (1)
4) wm8985->bclk is used only in wm8985_hw_params() and therefore no
   need to store it in the wm8985_priv structure

Therefore the clock calculation is rewritten in more clean and proper way:
- move wm8985_priv->bclk as a local variable into mw8985_hw_params()
- new variable wm8985_priv->pllout holds the actual frequency that is input
  to the MCLKDIV post-divider
- move wm8985_priv->sysclk as a local variable into mw8985_hw_params()
- sysclk is now always calculated as 256 * fs
- the MCLKDIV is looked up as pllout/sysclk
- fs_ratios[] is replaced by simpler mclk_divs[] lookup table

With this patch all rates: 8, 11.025, 12, 16, 22.05, 24, 32, 44.1 and 48kHz
work properly and generate the correct BCLK.

Signed-off-by: Petr Kulhavy <petr@barix.com>
---
 sound/soc/codecs/wm8985.c | 59 ++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/sound/soc/codecs/wm8985.c b/sound/soc/codecs/wm8985.c
index 18f2babe1090..628127aa3c96 100644
--- a/sound/soc/codecs/wm8985.c
+++ b/sound/soc/codecs/wm8985.c
@@ -181,22 +181,11 @@ static const int volume_update_regs[] = {
 struct wm8985_priv {
 	struct regmap *regmap;
 	struct regulator_bulk_data supplies[WM8985_NUM_SUPPLIES];
-	unsigned int sysclk;
-	unsigned int bclk;
+	unsigned int pllout;	/* input rate to the MCLKDIV divider */
 };
 
-static const struct {
-	int div;
-	int ratio;
-} fs_ratios[] = {
-	{ 10, 128 },
-	{ 15, 192 },
-	{ 20, 256 },
-	{ 30, 384 },
-	{ 40, 512 },
-	{ 60, 768 },
-	{ 80, 1024 },
-	{ 120, 1536 }
+static const int mclk_divs[] = {
+	10, 15, 20, 30, 40, 60, 80, 120
 };
 
 static const int srates[] = { 48000, 32000, 24000, 16000, 12000, 8000 };
@@ -693,15 +682,18 @@ static int wm8985_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec;
 	struct wm8985_priv *wm8985;
 	u16 blen, srate_idx;
-	unsigned int tmp;
 	int srate_best;
+	unsigned int bclk;	/* bit clock matching the current params */
+	unsigned int sysclk;	/* the actual sysclk after post division */
 
 	codec = dai->codec;
 	wm8985 = snd_soc_codec_get_drvdata(codec);
 
-	wm8985->bclk = snd_soc_params_to_bclk(params);
-	if ((int)wm8985->bclk < 0)
-		return wm8985->bclk;
+	/* always use 256 * fs in order to get best filter quality */
+	sysclk = 256 * params_rate(params);
+	bclk = snd_soc_params_to_bclk(params);
+	if ((int)bclk < 0)
+		return bclk;
 
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
@@ -742,29 +734,28 @@ static int wm8985_hw_params(struct snd_pcm_substream *substream,
 	snd_soc_update_bits(codec, WM8985_ADDITIONAL_CONTROL,
 			    WM8985_SR_MASK, srate_idx << WM8985_SR_SHIFT);
 
-	dev_dbg(dai->dev, "Target BCLK = %uHz\n", wm8985->bclk);
-	dev_dbg(dai->dev, "SYSCLK = %uHz\n", wm8985->sysclk);
+	dev_dbg(dai->dev, "Target BCLK = %uHz\n", bclk);
+	dev_dbg(dai->dev, "SYSCLK = %uHz\n", sysclk);
 
-	for (i = 0; i < ARRAY_SIZE(fs_ratios); ++i) {
-		if (wm8985->sysclk / params_rate(params)
-				== fs_ratios[i].ratio)
+	/* select the appropriate mclk divider */
+	for (i = 0; i < ARRAY_SIZE(mclk_divs); ++i) {
+		if (wm8985->pllout / mclk_divs[i] * 10
+				== sysclk)
 			break;
 	}
 
-	if (i == ARRAY_SIZE(fs_ratios)) {
+	if (i == ARRAY_SIZE(mclk_divs)) {
 		dev_err(dai->dev, "Unable to configure MCLK ratio %u/%u\n",
-			wm8985->sysclk, params_rate(params));
+			wm8985->pllout, sysclk);
 		return -EINVAL;
 	}
 
-	dev_dbg(dai->dev, "MCLK ratio = %dfs\n", fs_ratios[i].ratio);
 	snd_soc_update_bits(codec, WM8985_CLOCK_GEN_CONTROL,
 			    WM8985_MCLKDIV_MASK, i << WM8985_MCLKDIV_SHIFT);
 
 	/* select the appropriate bclk divider */
-	tmp = (wm8985->sysclk / fs_ratios[i].div) * 10;
 	for (i = 0; i < ARRAY_SIZE(bclk_divs); ++i) {
-		if (wm8985->bclk == tmp / bclk_divs[i])
+		if (bclk == sysclk / bclk_divs[i])
 			break;
 	}
 
@@ -786,6 +777,10 @@ struct pll_div {
 };
 
 #define FIXED_PLL_SIZE ((1ULL << 24) * 10)
+/*
+ * source = MCLK input of the chip
+ * target = the f2 coming out of the PLL before /4 divider
+ */
 static int pll_factors(struct pll_div *pll_div, unsigned int target,
 		       unsigned int source)
 {
@@ -872,17 +867,23 @@ static int wm8985_set_sysclk(struct snd_soc_dai *dai,
 				    WM8985_CLKSEL_MASK, 0);
 		snd_soc_update_bits(codec, WM8985_POWER_MANAGEMENT_1,
 				    WM8985_PLLEN_MASK, 0);
+		wm8985->pllout = freq;
 		break;
 	case WM8985_CLKSRC_PLL:
 		snd_soc_update_bits(codec, WM8985_CLOCK_GEN_CONTROL,
 				    WM8985_CLKSEL_MASK, WM8985_CLKSEL);
+		/*
+		 * in order to run the PLL within the recommended 90MHz
+		 * operating range the wm8985_set_pll() configures the PLL
+		 * to output double the required frequency
+		 */
+		wm8985->pllout = 2 * freq;
 		break;
 	default:
 		dev_err(dai->dev, "Unknown clock source %d\n", clk_id);
 		return -EINVAL;
 	}
 
-	wm8985->sysclk = freq;
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH] ASoC: wm8985: rework and fix the clock calculation

[Charles Keepax]

On Thu, May 12, 2016 at 08:48:54AM +0200, Petr Kulhavy wrote:
> The clock calculation has several issues:
> 1) if PLL is used in master mode the BCLK output runs at double the speed
> 2) de-facto only 44.1kHz and 48kHz sampling rates are supported, other
>    rates like 8kHz, 12kHz, 24kHz fail to find the proper BCLK divider

These certainly seem like things that need fixed :-)

> 3) the wm8985->sysclk variable has a misleading name and is used wrongly
>    in the clock calculation in wm8985_hw_params() which is the root cause
>    for (1)
> 4) wm8985->bclk is used only in wm8985_hw_params() and therefore no
>    need to store it in the wm8985_priv structure
> 
> Therefore the clock calculation is rewritten in more clean and proper way:
> - move wm8985_priv->bclk as a local variable into mw8985_hw_params()
> - new variable wm8985_priv->pllout holds the actual frequency that is input
>   to the MCLKDIV post-divider
> - move wm8985_priv->sysclk as a local variable into mw8985_hw_params()
> - sysclk is now always calculated as 256 * fs
> - the MCLKDIV is looked up as pllout/sysclk
> - fs_ratios[] is replaced by simpler mclk_divs[] lookup table
> 
> With this patch all rates: 8, 11.025, 12, 16, 22.05, 24, 32, 44.1 and 48kHz
> work properly and generate the correct BCLK.
> 
> Signed-off-by: Petr Kulhavy <petr@barix.com>
> ---
>  sound/soc/codecs/wm8985.c | 59 ++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8985.c b/sound/soc/codecs/wm8985.c
> index 18f2babe1090..628127aa3c96 100644
> --- a/sound/soc/codecs/wm8985.c
> +++ b/sound/soc/codecs/wm8985.c
> @@ -181,22 +181,11 @@ static const int volume_update_regs[] = {
>  struct wm8985_priv {
>  	struct regmap *regmap;
>  	struct regulator_bulk_data supplies[WM8985_NUM_SUPPLIES];
> -	unsigned int sysclk;
> -	unsigned int bclk;
> +	unsigned int pllout;	/* input rate to the MCLKDIV divider */
>  };
>  
> -static const struct {
> -	int div;
> -	int ratio;
> -} fs_ratios[] = {
> -	{ 10, 128 },
> -	{ 15, 192 },
> -	{ 20, 256 },
> -	{ 30, 384 },
> -	{ 40, 512 },
> -	{ 60, 768 },
> -	{ 80, 1024 },
> -	{ 120, 1536 }
> +static const int mclk_divs[] = {
> +	10, 15, 20, 30, 40, 60, 80, 120
>  };
>  
>  static const int srates[] = { 48000, 32000, 24000, 16000, 12000, 8000 };
> @@ -693,15 +682,18 @@ static int wm8985_hw_params(struct snd_pcm_substream *substream,
>  	struct snd_soc_codec *codec;
>  	struct wm8985_priv *wm8985;
>  	u16 blen, srate_idx;
> -	unsigned int tmp;
>  	int srate_best;
> +	unsigned int bclk;	/* bit clock matching the current params */
> +	unsigned int sysclk;	/* the actual sysclk after post division */
>  
>  	codec = dai->codec;
>  	wm8985 = snd_soc_codec_get_drvdata(codec);
>  
> -	wm8985->bclk = snd_soc_params_to_bclk(params);
> -	if ((int)wm8985->bclk < 0)
> -		return wm8985->bclk;
> +	/* always use 256 * fs in order to get best filter quality */
> +	sysclk = 256 * params_rate(params);

Seems quite bold to assume we always want to use 256*fs, I am not
super familiar with the part itself but are there good reasons to
do that? What if someone wanted to use say a direct MCLK and a
lower multiple?

The intent presumably here is that set_sysclk is where we set the
target SYSCLK and set_pll should set the PLL output then here we
probably should work out the correct MCLKDIV to get those.

> +	bclk = snd_soc_params_to_bclk(params);
> +	if ((int)bclk < 0)
> +		return bclk;
>  
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:
> @@ -742,29 +734,28 @@ static int wm8985_hw_params(struct snd_pcm_substream *substream,
>  	snd_soc_update_bits(codec, WM8985_ADDITIONAL_CONTROL,
>  			    WM8985_SR_MASK, srate_idx << WM8985_SR_SHIFT);
>  
> -	dev_dbg(dai->dev, "Target BCLK = %uHz\n", wm8985->bclk);
> -	dev_dbg(dai->dev, "SYSCLK = %uHz\n", wm8985->sysclk);
> +	dev_dbg(dai->dev, "Target BCLK = %uHz\n", bclk);
> +	dev_dbg(dai->dev, "SYSCLK = %uHz\n", sysclk);
>  
> -	for (i = 0; i < ARRAY_SIZE(fs_ratios); ++i) {
> -		if (wm8985->sysclk / params_rate(params)
> -				== fs_ratios[i].ratio)
> +	/* select the appropriate mclk divider */
> +	for (i = 0; i < ARRAY_SIZE(mclk_divs); ++i) {
> +		if (wm8985->pllout / mclk_divs[i] * 10
> +				== sysclk)
>  			break;
>  	}
>  
> -	if (i == ARRAY_SIZE(fs_ratios)) {
> +	if (i == ARRAY_SIZE(mclk_divs)) {
>  		dev_err(dai->dev, "Unable to configure MCLK ratio %u/%u\n",
> -			wm8985->sysclk, params_rate(params));
> +			wm8985->pllout, sysclk);
>  		return -EINVAL;
>  	}
>  
> -	dev_dbg(dai->dev, "MCLK ratio = %dfs\n", fs_ratios[i].ratio);
>  	snd_soc_update_bits(codec, WM8985_CLOCK_GEN_CONTROL,
>  			    WM8985_MCLKDIV_MASK, i << WM8985_MCLKDIV_SHIFT);
>  
>  	/* select the appropriate bclk divider */
> -	tmp = (wm8985->sysclk / fs_ratios[i].div) * 10;
>  	for (i = 0; i < ARRAY_SIZE(bclk_divs); ++i) {
> -		if (wm8985->bclk == tmp / bclk_divs[i])
> +		if (bclk == sysclk / bclk_divs[i])
>  			break;
>  	}
>  
> @@ -786,6 +777,10 @@ struct pll_div {
>  };
>  
>  #define FIXED_PLL_SIZE ((1ULL << 24) * 10)
> +/*
> + * source = MCLK input of the chip
> + * target = the f2 coming out of the PLL before /4 divider
> + */
>  static int pll_factors(struct pll_div *pll_div, unsigned int target,
>  		       unsigned int source)
>  {
> @@ -872,17 +867,23 @@ static int wm8985_set_sysclk(struct snd_soc_dai *dai,
>  				    WM8985_CLKSEL_MASK, 0);
>  		snd_soc_update_bits(codec, WM8985_POWER_MANAGEMENT_1,
>  				    WM8985_PLLEN_MASK, 0);
> +		wm8985->pllout = freq;
>  		break;
>  	case WM8985_CLKSRC_PLL:
>  		snd_soc_update_bits(codec, WM8985_CLOCK_GEN_CONTROL,
>  				    WM8985_CLKSEL_MASK, WM8985_CLKSEL);
> +		/*
> +		 * in order to run the PLL within the recommended 90MHz
> +		 * operating range the wm8985_set_pll() configures the PLL
> +		 * to output double the required frequency
> +		 */
> +		wm8985->pllout = 2 * freq;

Were does this *2 come from? Is this the PLLPRESCALE?

>  		break;
>  	default:
>  		dev_err(dai->dev, "Unknown clock source %d\n", clk_id);
>  		return -EINVAL;
>  	}
>  
> -	wm8985->sysclk = freq;
>  	return 0;
>  }
>  
> -- 
> 1.9.1

Thanks,
Charles

Re: [PATCH] ASoC: wm8985: rework and fix the clock calculation

[Petr Kulhavy]

On 12.05.2016 14:15, Charles Keepax wrote:
> On Thu, May 12, 2016 at 08:48:54AM +0200, Petr Kulhavy wrote:
> @@ -693,15 +682,18 @@ static int wm8985_hw_params(struct snd_pcm_substream *substream,
>   	struct snd_soc_codec *codec;
>   	struct wm8985_priv *wm8985;
>   	u16 blen, srate_idx;
> -	unsigned int tmp;
>   	int srate_best;
> +	unsigned int bclk;	/* bit clock matching the current params */
> +	unsigned int sysclk;	/* the actual sysclk after post division */
>   
>   	codec = dai->codec;
>   	wm8985 = snd_soc_codec_get_drvdata(codec);
>   
> -	wm8985->bclk = snd_soc_params_to_bclk(params);
> -	if ((int)wm8985->bclk < 0)
> -		return wm8985->bclk;
> +	/* always use 256 * fs in order to get best filter quality */
> +	sysclk = 256 * params_rate(params);
> Seems quite bold to assume we always want to use 256*fs, I am not
> super familiar with the part itself but are there good reasons to
> do that? What if someone wanted to use say a direct MCLK and a
> lower multiple?
>
> The intent presumably here is that set_sysclk is where we set the
> target SYSCLK and set_pll should set the PLL output then here we
> probably should work out the correct MCLKDIV to get those.
As I understand the datasheet the codec is basically designed for 256*fs 
clock:
DAC and ADC expect it, so do the cutoffs for the digital filters, ALC 
attack and delay times.
Also all the timing diagrams are specified at 256fs clock.
>>   static int pll_factors(struct pll_div *pll_div, unsigned int target,
>>   		       unsigned int source)
>>   {
>> @@ -872,17 +867,23 @@ static int wm8985_set_sysclk(struct snd_soc_dai *dai,
>>   				    WM8985_CLKSEL_MASK, 0);
>>   		snd_soc_update_bits(codec, WM8985_POWER_MANAGEMENT_1,
>>   				    WM8985_PLLEN_MASK, 0);
>> +		wm8985->pllout = freq;
>>   		break;
>>   	case WM8985_CLKSRC_PLL:
>>   		snd_soc_update_bits(codec, WM8985_CLOCK_GEN_CONTROL,
>>   				    WM8985_CLKSEL_MASK, WM8985_CLKSEL);
>> +		/*
>> +		 * in order to run the PLL within the recommended 90MHz
>> +		 * operating range the wm8985_set_pll() configures the PLL
>> +		 * to output double the required frequency
>> +		 */
>> +		wm8985->pllout = 2 * freq;
> Were does this *2 come from? Is this the PLLPRESCALE?
It comes from the following line in wm8985_set_pll() :
                  ret = pll_factors(&pll_div, freq_out * 4 * 2, freq_in);

This formula is taken over from the datasheet (the section Master Clock 
and PLL, calculation of the f2) and is correct.
It attempts to get the PLL to run at approx. 90MHz where it performs the 
best.
But since there is only f/4 fixed post-divider after the PLL (see the 
Figure 40 in the wm8758 or Figure 38 in the wm8985 datasheet) the 
F_pllout is effectively freq*2.

Petr

Re: [PATCH] ASoC: wm8985: rework and fix the clock calculation

[Charles Keepax]

On Thu, May 12, 2016 at 03:06:12PM +0200, Petr Kulhavy wrote:
> 
> 
> On 12.05.2016 14:15, Charles Keepax wrote:
> >On Thu, May 12, 2016 at 08:48:54AM +0200, Petr Kulhavy wrote:
> >@@ -693,15 +682,18 @@ static int wm8985_hw_params(struct snd_pcm_substream *substream,
> >  	struct snd_soc_codec *codec;
> >  	struct wm8985_priv *wm8985;
> >  	u16 blen, srate_idx;
> >-	unsigned int tmp;
> >  	int srate_best;
> >+	unsigned int bclk;	/* bit clock matching the current params */
> >+	unsigned int sysclk;	/* the actual sysclk after post division */
> >  	codec = dai->codec;
> >  	wm8985 = snd_soc_codec_get_drvdata(codec);
> >-	wm8985->bclk = snd_soc_params_to_bclk(params);
> >-	if ((int)wm8985->bclk < 0)
> >-		return wm8985->bclk;
> >+	/* always use 256 * fs in order to get best filter quality */
> >+	sysclk = 256 * params_rate(params);
> >Seems quite bold to assume we always want to use 256*fs, I am not
> >super familiar with the part itself but are there good reasons to
> >do that? What if someone wanted to use say a direct MCLK and a
> >lower multiple?
> >
> >The intent presumably here is that set_sysclk is where we set the
> >target SYSCLK and set_pll should set the PLL output then here we
> >probably should work out the correct MCLKDIV to get those.
> As I understand the datasheet the codec is basically designed for 256*fs
> clock:
> DAC and ADC expect it, so do the cutoffs for the digital filters, ALC attack
> and delay times.
> Also all the timing diagrams are specified at 256fs clock.

Ok that seems pretty conclusive that we should always have this
at 256fs.

> >>  static int pll_factors(struct pll_div *pll_div, unsigned int target,
> >>  		       unsigned int source)
> >>  {
> >>@@ -872,17 +867,23 @@ static int wm8985_set_sysclk(struct snd_soc_dai *dai,
> >>  				    WM8985_CLKSEL_MASK, 0);
> >>  		snd_soc_update_bits(codec, WM8985_POWER_MANAGEMENT_1,
> >>  				    WM8985_PLLEN_MASK, 0);
> >>+		wm8985->pllout = freq;
> >>  		break;
> >>  	case WM8985_CLKSRC_PLL:
> >>  		snd_soc_update_bits(codec, WM8985_CLOCK_GEN_CONTROL,
> >>  				    WM8985_CLKSEL_MASK, WM8985_CLKSEL);
> >>+		/*
> >>+		 * in order to run the PLL within the recommended 90MHz
> >>+		 * operating range the wm8985_set_pll() configures the PLL
> >>+		 * to output double the required frequency
> >>+		 */
> >>+		wm8985->pllout = 2 * freq;
> >Were does this *2 come from? Is this the PLLPRESCALE?
> It comes from the following line in wm8985_set_pll() :
>                  ret = pll_factors(&pll_div, freq_out * 4 * 2, freq_in);
> 
> This formula is taken over from the datasheet (the section Master Clock and
> PLL, calculation of the f2) and is correct.
> It attempts to get the PLL to run at approx. 90MHz where it performs the
> best.
> But since there is only f/4 fixed post-divider after the PLL (see the Figure
> 40 in the wm8758 or Figure 38 in the wm8985 datasheet) the F_pllout is
> effectively freq*2.

Ok that also makes sense.

I will have another quick look over the patch.

Thanks,
Charles

Re: [PATCH] ASoC: wm8985: rework and fix the clock calculation

[Charles Keepax]

On Thu, May 12, 2016 at 08:48:54AM +0200, Petr Kulhavy wrote:
> The clock calculation has several issues:
> 1) if PLL is used in master mode the BCLK output runs at double the speed
> 2) de-facto only 44.1kHz and 48kHz sampling rates are supported, other
>    rates like 8kHz, 12kHz, 24kHz fail to find the proper BCLK divider
> 3) the wm8985->sysclk variable has a misleading name and is used wrongly
>    in the clock calculation in wm8985_hw_params() which is the root cause
>    for (1)
> 4) wm8985->bclk is used only in wm8985_hw_params() and therefore no
>    need to store it in the wm8985_priv structure
> 
> Therefore the clock calculation is rewritten in more clean and proper way:
> - move wm8985_priv->bclk as a local variable into mw8985_hw_params()
> - new variable wm8985_priv->pllout holds the actual frequency that is input
>   to the MCLKDIV post-divider
> - move wm8985_priv->sysclk as a local variable into mw8985_hw_params()
> - sysclk is now always calculated as 256 * fs
> - the MCLKDIV is looked up as pllout/sysclk
> - fs_ratios[] is replaced by simpler mclk_divs[] lookup table
> 
> With this patch all rates: 8, 11.025, 12, 16, 22.05, 24, 32, 44.1 and 48kHz
> work properly and generate the correct BCLK.
> 
> Signed-off-by: Petr Kulhavy <petr@barix.com>
> ---

Reviewed-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles