Re: [PATCH v1] ASoC: simple-card-utils: set CPU sysclk before codec for mclk-fs

[Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>]

Hi Stefano

> When simple-audio-card uses simple-audio-card,mclk-fs,
> simple_util_hw_params() programs sysclk for codec DAIs and CPU DAIs.
> 
> Some CPU DAIs may apply the final MCLK rate as part of their .set_sysclk()
> callback (e.g. by changing the underlying clock rate via clk_set_rate()).
> If codec sysclk is configured before the CPU DAI applies the final
> MCLK rate, the codec may configure its internal clocking based on a
> non-final MCLK setting, leading to an incorrect clocking state on the
> first playback after boot.
> 
> This behaviour was observed on i.MX95 systems using fsl_sai with
> downstream kernels, which made the issue visible.
> However, the problem is generic and can affect any setup where the
> CPU DAI applies the final MCLK rate in set_sysclk() while
> simple-audio-card uses mclk-fs.
> 
> A reproducible symptom is that the first playback runs at the wrong speed
> ("slow" audio), while subsequent playbacks work correctly.
> 
> Example setup:
>   - CPU DAI: fsl_sai (i.MX95)
>   - Codec: wm8904
>   - simple-audio-card,mclk-fs enabled
> 
> Reproducer:
>   aplay -D hw:wm8904audio /home/Front_Center_8k.wav
> 
> Debug evidence:
> ftrace showed snd_soc_dai_set_sysclk() called from simple_util_hw_params
> for both codec and CPU DAIs, and confirmed wm8904_set_sysclk() ran before
> the CPU DAI sysclk call, and the CPU DAI then applied the final
> MCLK settings.
> 
> Fix this by setting the CPU DAI sysclk before the codec sysclk
> when mclk-fs is used, so that the codec always configures its internal
> clocking based on the final MCLK rate.
> 
> Signed-off-by: Stefano Radaelli <stefano.r@variscite.com>
> ---
>  sound/soc/generic/simple-card-utils.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index bdc02e85b089..50f60e4996a5 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -501,15 +501,21 @@ int simple_util_hw_params(struct snd_pcm_substream *substream,
>  				goto end;
>  		}
>  
> -		for_each_rtd_codec_dais(rtd, i, sdai) {
> -			pdai = simple_props_to_dai_codec(props, i);
> +		/*
> +		 * When mclk-fs is used, some CPU DAIs may change the
> +		 * actual MCLK rate in set_sysclk() (e.g. via clk_set_rate()).
> +		 * Configure CPU DAI first so codec clocking/FLL sees
> +		 * the final MCLK rate.
> +		 */
> +		for_each_rtd_cpu_dais(rtd, i, sdai) {
> +			pdai = simple_props_to_dai_cpu(props, i);
>  			ret = snd_soc_dai_set_sysclk(sdai, 0, mclk, pdai->clk_direction);
>  			if (ret && ret != -ENOTSUPP)
>  				goto end;
>  		}
>  
> -		for_each_rtd_cpu_dais(rtd, i, sdai) {
> -			pdai = simple_props_to_dai_cpu(props, i);
> +		for_each_rtd_codec_dais(rtd, i, sdai) {
> +			pdai = simple_props_to_dai_codec(props, i);
>  			ret = snd_soc_dai_set_sysclk(sdai, 0, mclk, pdai->clk_direction);
>  			if (ret && ret != -ENOTSUPP)
>  				goto end;

Actually, this is one of my concern (= which should be worked first
(CPU or Codec)). I don't remember why current code calls Codec first,
but maybe original code was.
I guess, some other Card needs to call as-is (= Codec first), and
you want to call opposite (= CPU first). It depends on clock
provider/consumer settings, instead of mclk-fs, but based on each Card.

Not same but similar issue we had before about how we want to call
.trigger.

	356caf663deee8dc46ff3168ec0b24bcbeb00b28
	("ASoC: add new trigger ordering method")

It was fixed order, but is now controlled by flag.
Maybe we want to have similar method ?

// unreliable name
#define XX_CLK_START_CODEC_FIRST	0
#define XX_CLK_START_CPU_FIRST		1

	if (xx_clk_start_first == XX_CLK_START_CODEC_FIRST) {
		// same as current code
		start_codec_clk()
		start_cpu_clk()
	} else {
		// opposite
		start_cpu_clk()
		start_codec_clk()
	}

Thank you for your help !!

Best regards
---
Kuninori Morimoto