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

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

[Stefano Radaelli]

From: Stefano Radaelli <stefano.r@variscite.com>

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;

base-commit: b7ff7151e653aa296ab6c5495b2c1ab7c21eb250
-- 
2.47.3

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

[Kuninori Morimoto]

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

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

[Stefano Radaelli]

Hi Kuninori
> 
> 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 suggestion. I agree that the default ordering should
remain unchanged to maintain backward compatibility, and we should
implement a configurable feature to change the order when needed.
Following your recommendation, I could implement something similar to the
trigger ordering approach:

#define SND_SOC_SYSCLK_ORDER_CODEC_FIRST    0
#define SND_SOC_SYSCLK_ORDER_CPU_FIRST      1

static int asoc_simple_init_dai(struct snd_soc_dai *dai,
                                struct asoc_simple_dai *simple_dai)
{
    int sysclk_order = SND_SOC_SYSCLK_ORDER_CODEC_FIRST;  // <- Default

    if (simple_dai->sysclk) {
        if (sysclk_order == SND_SOC_SYSCLK_ORDER_CODEC_FIRST) {
            // Codec first
            ret = snd_soc_dai_set_sysclk(codec_dai, 0, sysclk, ...);
            ret = snd_soc_dai_set_sysclk(cpu_dai, 0, sysclk, ...);
        } else {
            // CPU first
            ret = snd_soc_dai_set_sysclk(cpu_dai, 0, sysclk, ...);
            ret = snd_soc_dai_set_sysclk(codec_dai, 0, sysclk, ...);
        }
    }
}

This approach would work well for our case and is acceptable as-is for now.
The ordering would be controlled via the define in the driver code.
As a future enhancement, would you be open to adding a device tree property
to make this configurable per-board?

I'll send you v2 with this modification.

Thanks a lot for your guidance,
Best Regards,
Stefano