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

[Charles Keepax <ckeepax@opensource.wolfsonmicro.com>]

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