[PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Stefan Eichenberger]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

Currently, the flags for the ACM clocks are set to 0. This configuration
causes the fsl-sai audio driver to fail when attempting to set the
sysclk, returning an EINVAL error. The following error messages
highlight the issue:
fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
imx-hdmi sound-hdmi: failed to set cpu sysclk: -22

By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
driver does not support reparenting and instead relies on the clock tree
as defined in the device tree. This change resolves the issue with the
fsl-sai audio driver.

CC: stable@vger.kernel.org
Fixes: d3a0946d7ac9 ("clk: imx: imx8: add audio clock mux driver")
Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
---
 drivers/clk/imx/clk-imx8-acm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
index c169fe53a35f..f20832a17ea3 100644
--- a/drivers/clk/imx/clk-imx8-acm.c
+++ b/drivers/clk/imx/clk-imx8-acm.c
@@ -371,7 +371,8 @@ static int imx8_acm_clk_probe(struct platform_device *pdev)
 	for (i = 0; i < priv->soc_data->num_sels; i++) {
 		hws[sels[i].clkid] = devm_clk_hw_register_mux_parent_data_table(dev,
 										sels[i].name, sels[i].parents,
-										sels[i].num_parents, 0,
+										sels[i].num_parents,
+										CLK_SET_RATE_NO_REPARENT,
 										base + sels[i].reg,
 										sels[i].shift, sels[i].width,
 										0, NULL, NULL);
-- 
2.45.2

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Shengjiu Wang]

On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
>
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> Currently, the flags for the ACM clocks are set to 0. This configuration
> causes the fsl-sai audio driver to fail when attempting to set the
> sysclk, returning an EINVAL error. The following error messages
> highlight the issue:
> fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> imx-hdmi sound-hdmi: failed to set cpu sysclk: -22

The reason for this error is that the current clock parent can't
support the rate
you require (I think you want 11289600).

We can configure the dts to provide such source, for example:

 &sai5 {
+       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
+                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
+                       <&sai5_lpcg 0>;
+       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
+       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
+                                <722534400>, <45158400>, <11289600>,
+                               <49152000>;
        status = "okay";
 };

Then your case should work.

>
> By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM

I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
the driver don't get an error from clk_set_rate().

Best regards
Shengjiu Wang

> driver does not support reparenting and instead relies on the clock tree
> as defined in the device tree. This change resolves the issue with the
> fsl-sai audio driver.
>
> CC: stable@vger.kernel.org
> Fixes: d3a0946d7ac9 ("clk: imx: imx8: add audio clock mux driver")
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/clk/imx/clk-imx8-acm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
> index c169fe53a35f..f20832a17ea3 100644
> --- a/drivers/clk/imx/clk-imx8-acm.c
> +++ b/drivers/clk/imx/clk-imx8-acm.c
> @@ -371,7 +371,8 @@ static int imx8_acm_clk_probe(struct platform_device *pdev)
>         for (i = 0; i < priv->soc_data->num_sels; i++) {
>                 hws[sels[i].clkid] = devm_clk_hw_register_mux_parent_data_table(dev,
>                                                                                 sels[i].name, sels[i].parents,
> -                                                                               sels[i].num_parents, 0,
> +                                                                               sels[i].num_parents,
> +                                                                               CLK_SET_RATE_NO_REPARENT,
>                                                                                 base + sels[i].reg,
>                                                                                 sels[i].shift, sels[i].width,
>                                                                                 0, NULL, NULL);
> --
> 2.45.2
>
>

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Stefan Eichenberger]

Hi Shengjiu Wang,

On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> >
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >
> > Currently, the flags for the ACM clocks are set to 0. This configuration
> > causes the fsl-sai audio driver to fail when attempting to set the
> > sysclk, returning an EINVAL error. The following error messages
> > highlight the issue:
> > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> 
> The reason for this error is that the current clock parent can't
> support the rate
> you require (I think you want 11289600).
> 
> We can configure the dts to provide such source, for example:
> 
>  &sai5 {
> +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> +                       <&sai5_lpcg 0>;
> +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> +                                <722534400>, <45158400>, <11289600>,
> +                               <49152000>;
>         status = "okay";
>  };
> 
> Then your case should work.
> 
> >
> > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> 
> I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> the driver don't get an error from clk_set_rate().

Thanks for the proposal, I will try it out tomorrow. Isn't this a
problem if other SAIs use the same clock source but with different
rates? 

If we have to define fixed rates in the DTS or else the clock driver
will return an error, isn't that a problem? Maybe I should change the
sai driver so that it ignores the failure and just takes the rate
configured? In the end audio works, even if it can't set the requested
rate.

Regards,
Stefan

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Stefan Eichenberger]

Hi Shengjiu Wang,

On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> Hi Shengjiu Wang,
> 
> On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > >
> > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > >
> > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > causes the fsl-sai audio driver to fail when attempting to set the
> > > sysclk, returning an EINVAL error. The following error messages
> > > highlight the issue:
> > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > 
> > The reason for this error is that the current clock parent can't
> > support the rate
> > you require (I think you want 11289600).
> > 
> > We can configure the dts to provide such source, for example:
> > 
> >  &sai5 {
> > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > +                       <&sai5_lpcg 0>;
> > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > +                                <722534400>, <45158400>, <11289600>,
> > +                               <49152000>;
> >         status = "okay";
> >  };
> > 
> > Then your case should work.
> > 
> > >
> > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > 
> > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > the driver don't get an error from clk_set_rate().
> 
> Thanks for the proposal, I will try it out tomorrow. Isn't this a
> problem if other SAIs use the same clock source but with different
> rates? 
> 
> If we have to define fixed rates in the DTS or else the clock driver
> will return an error, isn't that a problem? Maybe I should change the
> sai driver so that it ignores the failure and just takes the rate
> configured? In the end audio works, even if it can't set the requested
> rate.

The following clock tree change would allow the driver to work
in our scenario:
&sai5 {
	assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
			  <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
	assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
	assigned-clock-rates = <0>, <11289600>;
};

However, this means we have to switch the parent clock to the audio pll
1. For the simple setup with two SAIs, one for analog audio and one for
HDMI this wouldn't be a problem. But I'm not sure if this is a good
solution if a customer would add a third SAI which requires again a
different parent clock rate.

One potential solution could be that the SAI driver tries to first
derive the clock from the current parent and only if this fails it tries
to modify its parent clock. What do you think about this solution?

Regards,
Stefan

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Shengjiu Wang]

On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
>
> Hi Shengjiu Wang,
>
> On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > Hi Shengjiu Wang,
> >
> > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > >
> > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > >
> > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > sysclk, returning an EINVAL error. The following error messages
> > > > highlight the issue:
> > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > >
> > > The reason for this error is that the current clock parent can't
> > > support the rate
> > > you require (I think you want 11289600).
> > >
> > > We can configure the dts to provide such source, for example:
> > >
> > >  &sai5 {
> > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > +                       <&sai5_lpcg 0>;
> > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > +                                <722534400>, <45158400>, <11289600>,
> > > +                               <49152000>;
> > >         status = "okay";
> > >  };
> > >
> > > Then your case should work.
> > >
> > > >
> > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > >
> > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > the driver don't get an error from clk_set_rate().
> >
> > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > problem if other SAIs use the same clock source but with different
> > rates?
> >
> > If we have to define fixed rates in the DTS or else the clock driver
> > will return an error, isn't that a problem? Maybe I should change the
> > sai driver so that it ignores the failure and just takes the rate
> > configured? In the end audio works, even if it can't set the requested
> > rate.
>
> The following clock tree change would allow the driver to work
> in our scenario:
> &sai5 {
>         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
>                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
>         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
>         assigned-clock-rates = <0>, <11289600>;
> };

In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
which should fit for most audio requirements.

>
> However, this means we have to switch the parent clock to the audio pll
> 1. For the simple setup with two SAIs, one for analog audio and one for
> HDMI this wouldn't be a problem. But I'm not sure if this is a good
> solution if a customer would add a third SAI which requires again a
> different parent clock rate.

We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
PLL_1 for 44kHz,  even with a third SAI or more,  they should work.

>
> One potential solution could be that the SAI driver tries to first
> derive the clock from the current parent and only if this fails it tries
> to modify its parent clock. What do you think about this solution?
>

It's better to avoid modifying parent rate, as drivers don't know if the parent
is used by another instance.

Best regards
Shengjiu Wang

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Stefan Eichenberger]

On Wed, Jan 15, 2025 at 05:14:09PM +0800, Shengjiu Wang wrote:
> On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> >
> > Hi Shengjiu Wang,
> >
> > On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > > Hi Shengjiu Wang,
> > >
> > > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > >
> > > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > > >
> > > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > > sysclk, returning an EINVAL error. The following error messages
> > > > > highlight the issue:
> > > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > > >
> > > > The reason for this error is that the current clock parent can't
> > > > support the rate
> > > > you require (I think you want 11289600).
> > > >
> > > > We can configure the dts to provide such source, for example:
> > > >
> > > >  &sai5 {
> > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > +                       <&sai5_lpcg 0>;
> > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > +                                <722534400>, <45158400>, <11289600>,
> > > > +                               <49152000>;
> > > >         status = "okay";
> > > >  };
> > > >
> > > > Then your case should work.
> > > >
> > > > >
> > > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > > >
> > > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > > the driver don't get an error from clk_set_rate().
> > >
> > > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > > problem if other SAIs use the same clock source but with different
> > > rates?
> > >
> > > If we have to define fixed rates in the DTS or else the clock driver
> > > will return an error, isn't that a problem? Maybe I should change the
> > > sai driver so that it ignores the failure and just takes the rate
> > > configured? In the end audio works, even if it can't set the requested
> > > rate.
> >
> > The following clock tree change would allow the driver to work
> > in our scenario:
> > &sai5 {
> >         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> >                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
> >         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
> >         assigned-clock-rates = <0>, <11289600>;
> > };
> 
> In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
> PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
> which should fit for most audio requirements.
> 
> >
> > However, this means we have to switch the parent clock to the audio pll
> > 1. For the simple setup with two SAIs, one for analog audio and one for
> > HDMI this wouldn't be a problem. But I'm not sure if this is a good
> > solution if a customer would add a third SAI which requires again a
> > different parent clock rate.
> 
> We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
> PLL_1 for 44kHz,  even with a third SAI or more,  they should work.
> 
> >
> > One potential solution could be that the SAI driver tries to first
> > derive the clock from the current parent and only if this fails it tries
> > to modify its parent clock. What do you think about this solution?
> >

I did some more testing and I'm still not happy with the current
solution. The problem is that if we change the SAI5 mclk clock parent it
can either support the 44kHz series or the 48kHz series. However, in the
case of HDMI we do not know in advance what the user wants.

This means when testing either this works:
speaker-test -D hw:2,0 -r 48000 -c 2
or this works:
speaker-test -D hw:2,0 -r 44100 -c 2
With:
card 2: imxaudiohdmitx [imx-audio-hdmi-tx], device 0: i.MX HDMI i2s-hifi-0 [i.MX HDMI i2s-hifi-0]

If I on the other hand avoid that fsl_sai_set_mclk_rate is called by
patching imx-hdmi.c. I can make both rates work with the standard clock
settings. So I wonder if calling snd_soc_dai_set_sysclk is really
necessary for the i.MX8?
diff --git a/sound/soc/fsl/imx-hdmi.c b/sound/soc/fsl/imx-hdmi.c
index f9cec6f0aecd..7d8aa58645b7 100644
--- a/sound/soc/fsl/imx-hdmi.c
+++ b/sound/soc/fsl/imx-hdmi.c
@@ -92,15 +92,6 @@ static int imx_hdmi_hw_params(struct snd_pcm_substream *substream,
        u32 slot_width = data->cpu_priv.slot_width;
        int ret;

-       /* MCLK always is (256 or 192) * rate. */
-       ret = snd_soc_dai_set_sysclk(cpu_dai, data->cpu_priv.sysclk_id[tx],
-                                    8 * slot_width * params_rate(params),
-                                    tx ? SND_SOC_CLOCK_OUT : SND_SOC_CLOCK_IN);
-       if (ret && ret != -ENOTSUPP) {
-               dev_err(dev, "failed to set cpu sysclk: %d\n", ret);
-               return ret;
-       }
-
        ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0, 2, slot_width);
        if (ret && ret != -ENOTSUPP) {
                dev_err(dev, "failed to set cpu dai tdm slot: %d\n", ret);

Regards,
Stefan

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Shengjiu Wang]

On Wed, Jan 15, 2025 at 6:39 PM Stefan Eichenberger <eichest@gmail.com> wrote:
>
> On Wed, Jan 15, 2025 at 05:14:09PM +0800, Shengjiu Wang wrote:
> > On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > >
> > > Hi Shengjiu Wang,
> > >
> > > On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > > > Hi Shengjiu Wang,
> > > >
> > > > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > >
> > > > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > > > >
> > > > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > > > sysclk, returning an EINVAL error. The following error messages
> > > > > > highlight the issue:
> > > > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > > > >
> > > > > The reason for this error is that the current clock parent can't
> > > > > support the rate
> > > > > you require (I think you want 11289600).
> > > > >
> > > > > We can configure the dts to provide such source, for example:
> > > > >
> > > > >  &sai5 {
> > > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > > +                       <&sai5_lpcg 0>;
> > > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > > +                                <722534400>, <45158400>, <11289600>,
> > > > > +                               <49152000>;
> > > > >         status = "okay";
> > > > >  };
> > > > >
> > > > > Then your case should work.
> > > > >
> > > > > >
> > > > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > > > >
> > > > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > > > the driver don't get an error from clk_set_rate().
> > > >
> > > > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > > > problem if other SAIs use the same clock source but with different
> > > > rates?
> > > >
> > > > If we have to define fixed rates in the DTS or else the clock driver
> > > > will return an error, isn't that a problem? Maybe I should change the
> > > > sai driver so that it ignores the failure and just takes the rate
> > > > configured? In the end audio works, even if it can't set the requested
> > > > rate.
> > >
> > > The following clock tree change would allow the driver to work
> > > in our scenario:
> > > &sai5 {
> > >         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > >                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
> > >         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
> > >         assigned-clock-rates = <0>, <11289600>;
> > > };
> >
> > In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
> > PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
> > which should fit for most audio requirements.
> >
> > >
> > > However, this means we have to switch the parent clock to the audio pll
> > > 1. For the simple setup with two SAIs, one for analog audio and one for
> > > HDMI this wouldn't be a problem. But I'm not sure if this is a good
> > > solution if a customer would add a third SAI which requires again a
> > > different parent clock rate.
> >
> > We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
> > PLL_1 for 44kHz,  even with a third SAI or more,  they should work.
> >
> > >
> > > One potential solution could be that the SAI driver tries to first
> > > derive the clock from the current parent and only if this fails it tries
> > > to modify its parent clock. What do you think about this solution?
> > >
>
> I did some more testing and I'm still not happy with the current
> solution. The problem is that if we change the SAI5 mclk clock parent it
> can either support the 44kHz series or the 48kHz series. However, in the
> case of HDMI we do not know in advance what the user wants.
>
> This means when testing either this works:
> speaker-test -D hw:2,0 -r 48000 -c 2
> or this works:
> speaker-test -D hw:2,0 -r 44100 -c 2
> With:
> card 2: imxaudiohdmitx [imx-audio-hdmi-tx], device 0: i.MX HDMI i2s-hifi-0 [i.MX HDMI i2s-hifi-0]

Are you using the setting below?  then should not either,  should both works
 &sai5 {
+       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
+                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
+                       <&sai5_lpcg 0>;
+       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
+       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
+                                <722534400>, <45158400>, <11289600>,
+                               <49152000>;
        status = "okay";
 };

best regards
Shengjiu Wang
>
> If I on the other hand avoid that fsl_sai_set_mclk_rate is called by
> patching imx-hdmi.c. I can make both rates work with the standard clock
> settings. So I wonder if calling snd_soc_dai_set_sysclk is really
> necessary for the i.MX8?
> diff --git a/sound/soc/fsl/imx-hdmi.c b/sound/soc/fsl/imx-hdmi.c
> index f9cec6f0aecd..7d8aa58645b7 100644
> --- a/sound/soc/fsl/imx-hdmi.c
> +++ b/sound/soc/fsl/imx-hdmi.c
> @@ -92,15 +92,6 @@ static int imx_hdmi_hw_params(struct snd_pcm_substream *substream,
>         u32 slot_width = data->cpu_priv.slot_width;
>         int ret;
>
> -       /* MCLK always is (256 or 192) * rate. */
> -       ret = snd_soc_dai_set_sysclk(cpu_dai, data->cpu_priv.sysclk_id[tx],
> -                                    8 * slot_width * params_rate(params),
> -                                    tx ? SND_SOC_CLOCK_OUT : SND_SOC_CLOCK_IN);
> -       if (ret && ret != -ENOTSUPP) {
> -               dev_err(dev, "failed to set cpu sysclk: %d\n", ret);
> -               return ret;
> -       }
> -
>         ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0, 2, slot_width);
>         if (ret && ret != -ENOTSUPP) {
>                 dev_err(dev, "failed to set cpu dai tdm slot: %d\n", ret);
>
> Regards,
> Stefan
>
>

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Stefan Eichenberger]

Hi Shengjiu Wang,

On Thu, Jan 16, 2025 at 12:01:13PM +0800, Shengjiu Wang wrote:
> On Wed, Jan 15, 2025 at 6:39 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> >
> > On Wed, Jan 15, 2025 at 05:14:09PM +0800, Shengjiu Wang wrote:
> > > On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > >
> > > > Hi Shengjiu Wang,
> > > >
> > > > On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > > > > Hi Shengjiu Wang,
> > > > >
> > > > > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > > > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > > > > >
> > > > > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > > > > sysclk, returning an EINVAL error. The following error messages
> > > > > > > highlight the issue:
> > > > > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > > > > >
> > > > > > The reason for this error is that the current clock parent can't
> > > > > > support the rate
> > > > > > you require (I think you want 11289600).
> > > > > >
> > > > > > We can configure the dts to provide such source, for example:
> > > > > >
> > > > > >  &sai5 {
> > > > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > +                       <&sai5_lpcg 0>;
> > > > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > > > +                                <722534400>, <45158400>, <11289600>,
> > > > > > +                               <49152000>;
> > > > > >         status = "okay";
> > > > > >  };
> > > > > >
> > > > > > Then your case should work.
> > > > > >
> > > > > > >
> > > > > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > > > > >
> > > > > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > > > > the driver don't get an error from clk_set_rate().
> > > > >
> > > > > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > > > > problem if other SAIs use the same clock source but with different
> > > > > rates?
> > > > >
> > > > > If we have to define fixed rates in the DTS or else the clock driver
> > > > > will return an error, isn't that a problem? Maybe I should change the
> > > > > sai driver so that it ignores the failure and just takes the rate
> > > > > configured? In the end audio works, even if it can't set the requested
> > > > > rate.
> > > >
> > > > The following clock tree change would allow the driver to work
> > > > in our scenario:
> > > > &sai5 {
> > > >         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > >                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
> > > >         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
> > > >         assigned-clock-rates = <0>, <11289600>;
> > > > };
> > >
> > > In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
> > > PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
> > > which should fit for most audio requirements.
> > >
> > > >
> > > > However, this means we have to switch the parent clock to the audio pll
> > > > 1. For the simple setup with two SAIs, one for analog audio and one for
> > > > HDMI this wouldn't be a problem. But I'm not sure if this is a good
> > > > solution if a customer would add a third SAI which requires again a
> > > > different parent clock rate.
> > >
> > > We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
> > > PLL_1 for 44kHz,  even with a third SAI or more,  they should work.
> > >
> > > >
> > > > One potential solution could be that the SAI driver tries to first
> > > > derive the clock from the current parent and only if this fails it tries
> > > > to modify its parent clock. What do you think about this solution?
> > > >
> >
> > I did some more testing and I'm still not happy with the current
> > solution. The problem is that if we change the SAI5 mclk clock parent it
> > can either support the 44kHz series or the 48kHz series. However, in the
> > case of HDMI we do not know in advance what the user wants.
> >
> > This means when testing either this works:
> > speaker-test -D hw:2,0 -r 48000 -c 2
> > or this works:
> > speaker-test -D hw:2,0 -r 44100 -c 2
> > With:
> > card 2: imxaudiohdmitx [imx-audio-hdmi-tx], device 0: i.MX HDMI i2s-hifi-0 [i.MX HDMI i2s-hifi-0]
> 
> Are you using the setting below?  then should not either,  should both works
>  &sai5 {
> +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> +                       <&sai5_lpcg 0>;
> +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> +                                <722534400>, <45158400>, <11289600>,
> +                               <49152000>;
>         status = "okay";
>  };

Sorry I didn't communicate that properly. Yes I was trying with these
settings but they do not work. The problem does not seem to be that the
driver can not adjust the rate for the audio (so e.g. 44kHz or 48kHz)
but that snd_soc_dai_set_sysclk in imx-hdmi.c fails with EINVAL. This
results in a call to fsl_sai_set_mclk_rate in fsl_sai.c with clk_id 1
(mclk_clk[1]) and a freq of 11289600 which causes the fail.
Interestingly, in fsl_sai_set_bclk we then only use clk_get_rate on
mclk_clk[0] which is set to audio_ipg_clk (rate 175000000) and we do not
use mclk_clk[1] anymore at all. This is why I'm not sure if this call to
snd_soc_dai_set_syclk is really necessary?

Regards,
Stefan

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Shengjiu Wang]

On Thu, Jan 16, 2025 at 3:24 PM Stefan Eichenberger <eichest@gmail.com> wrote:
>
> Hi Shengjiu Wang,
>
> On Thu, Jan 16, 2025 at 12:01:13PM +0800, Shengjiu Wang wrote:
> > On Wed, Jan 15, 2025 at 6:39 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > >
> > > On Wed, Jan 15, 2025 at 05:14:09PM +0800, Shengjiu Wang wrote:
> > > > On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > >
> > > > > Hi Shengjiu Wang,
> > > > >
> > > > > On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > > > > > Hi Shengjiu Wang,
> > > > > >
> > > > > > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > > > > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > > > >
> > > > > > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > > > > > >
> > > > > > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > > > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > > > > > sysclk, returning an EINVAL error. The following error messages
> > > > > > > > highlight the issue:
> > > > > > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > > > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > > > > > >
> > > > > > > The reason for this error is that the current clock parent can't
> > > > > > > support the rate
> > > > > > > you require (I think you want 11289600).
> > > > > > >
> > > > > > > We can configure the dts to provide such source, for example:
> > > > > > >
> > > > > > >  &sai5 {
> > > > > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > +                       <&sai5_lpcg 0>;
> > > > > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > > > > +                                <722534400>, <45158400>, <11289600>,
> > > > > > > +                               <49152000>;
> > > > > > >         status = "okay";
> > > > > > >  };
> > > > > > >
> > > > > > > Then your case should work.
> > > > > > >
> > > > > > > >
> > > > > > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > > > > > >
> > > > > > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > > > > > the driver don't get an error from clk_set_rate().
> > > > > >
> > > > > > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > > > > > problem if other SAIs use the same clock source but with different
> > > > > > rates?
> > > > > >
> > > > > > If we have to define fixed rates in the DTS or else the clock driver
> > > > > > will return an error, isn't that a problem? Maybe I should change the
> > > > > > sai driver so that it ignores the failure and just takes the rate
> > > > > > configured? In the end audio works, even if it can't set the requested
> > > > > > rate.
> > > > >
> > > > > The following clock tree change would allow the driver to work
> > > > > in our scenario:
> > > > > &sai5 {
> > > > >         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > >                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
> > > > >         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
> > > > >         assigned-clock-rates = <0>, <11289600>;
> > > > > };
> > > >
> > > > In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
> > > > PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
> > > > which should fit for most audio requirements.
> > > >
> > > > >
> > > > > However, this means we have to switch the parent clock to the audio pll
> > > > > 1. For the simple setup with two SAIs, one for analog audio and one for
> > > > > HDMI this wouldn't be a problem. But I'm not sure if this is a good
> > > > > solution if a customer would add a third SAI which requires again a
> > > > > different parent clock rate.
> > > >
> > > > We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
> > > > PLL_1 for 44kHz,  even with a third SAI or more,  they should work.
> > > >
> > > > >
> > > > > One potential solution could be that the SAI driver tries to first
> > > > > derive the clock from the current parent and only if this fails it tries
> > > > > to modify its parent clock. What do you think about this solution?
> > > > >
> > >
> > > I did some more testing and I'm still not happy with the current
> > > solution. The problem is that if we change the SAI5 mclk clock parent it
> > > can either support the 44kHz series or the 48kHz series. However, in the
> > > case of HDMI we do not know in advance what the user wants.
> > >
> > > This means when testing either this works:
> > > speaker-test -D hw:2,0 -r 48000 -c 2
> > > or this works:
> > > speaker-test -D hw:2,0 -r 44100 -c 2
> > > With:
> > > card 2: imxaudiohdmitx [imx-audio-hdmi-tx], device 0: i.MX HDMI i2s-hifi-0 [i.MX HDMI i2s-hifi-0]
> >
> > Are you using the setting below?  then should not either,  should both works
> >  &sai5 {
> > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > +                       <&sai5_lpcg 0>;
> > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > +                                <722534400>, <45158400>, <11289600>,
> > +                               <49152000>;
> >         status = "okay";
> >  };
>
> Sorry I didn't communicate that properly. Yes I was trying with these
> settings but they do not work. The problem does not seem to be that the
> driver can not adjust the rate for the audio (so e.g. 44kHz or 48kHz)
> but that snd_soc_dai_set_sysclk in imx-hdmi.c fails with EINVAL. This
> results in a call to fsl_sai_set_mclk_rate in fsl_sai.c with clk_id 1
> (mclk_clk[1]) and a freq of 11289600 which causes the fail.
> Interestingly, in fsl_sai_set_bclk we then only use clk_get_rate on
> mclk_clk[0] which is set to audio_ipg_clk (rate 175000000) and we do not
> use mclk_clk[1] anymore at all. This is why I'm not sure if this call to
> snd_soc_dai_set_syclk is really necessary?
>

Could you please check if you have the below commit in your test tree?
35121e9def07 clk: imx: imx8: Use clk_hw pointer for self registered
clock in clk_parent_data

if not, please cherry-pick it.

The audio_ipg_clk can be selected if there is no other choice.
but the rate 175000000 is not accurate for 44kHz. what we got
is 44102Hz. This is the reason I don't like to use this source.

best regards
Shengjiu Wang

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Stefan Eichenberger]

Hi Shengjiu Wang,

On Thu, Jan 16, 2025 at 03:30:55PM +0800, Shengjiu Wang wrote:
> On Thu, Jan 16, 2025 at 3:24 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> >
> > Hi Shengjiu Wang,
> >
> > On Thu, Jan 16, 2025 at 12:01:13PM +0800, Shengjiu Wang wrote:
> > > On Wed, Jan 15, 2025 at 6:39 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 15, 2025 at 05:14:09PM +0800, Shengjiu Wang wrote:
> > > > > On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > >
> > > > > > Hi Shengjiu Wang,
> > > > > >
> > > > > > On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > > > > > > Hi Shengjiu Wang,
> > > > > > >
> > > > > > > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > > > > > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > > > > > > >
> > > > > > > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > > > > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > > > > > > sysclk, returning an EINVAL error. The following error messages
> > > > > > > > > highlight the issue:
> > > > > > > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > > > > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > > > > > > >
> > > > > > > > The reason for this error is that the current clock parent can't
> > > > > > > > support the rate
> > > > > > > > you require (I think you want 11289600).
> > > > > > > >
> > > > > > > > We can configure the dts to provide such source, for example:
> > > > > > > >
> > > > > > > >  &sai5 {
> > > > > > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > > +                       <&sai5_lpcg 0>;
> > > > > > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > > > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > > > > > +                                <722534400>, <45158400>, <11289600>,
> > > > > > > > +                               <49152000>;
> > > > > > > >         status = "okay";
> > > > > > > >  };
> > > > > > > >
> > > > > > > > Then your case should work.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > > > > > > >
> > > > > > > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > > > > > > the driver don't get an error from clk_set_rate().
> > > > > > >
> > > > > > > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > > > > > > problem if other SAIs use the same clock source but with different
> > > > > > > rates?
> > > > > > >
> > > > > > > If we have to define fixed rates in the DTS or else the clock driver
> > > > > > > will return an error, isn't that a problem? Maybe I should change the
> > > > > > > sai driver so that it ignores the failure and just takes the rate
> > > > > > > configured? In the end audio works, even if it can't set the requested
> > > > > > > rate.
> > > > > >
> > > > > > The following clock tree change would allow the driver to work
> > > > > > in our scenario:
> > > > > > &sai5 {
> > > > > >         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > >                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
> > > > > >         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
> > > > > >         assigned-clock-rates = <0>, <11289600>;
> > > > > > };
> > > > >
> > > > > In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
> > > > > PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
> > > > > which should fit for most audio requirements.
> > > > >
> > > > > >
> > > > > > However, this means we have to switch the parent clock to the audio pll
> > > > > > 1. For the simple setup with two SAIs, one for analog audio and one for
> > > > > > HDMI this wouldn't be a problem. But I'm not sure if this is a good
> > > > > > solution if a customer would add a third SAI which requires again a
> > > > > > different parent clock rate.
> > > > >
> > > > > We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
> > > > > PLL_1 for 44kHz,  even with a third SAI or more,  they should work.
> > > > >
> > > > > >
> > > > > > One potential solution could be that the SAI driver tries to first
> > > > > > derive the clock from the current parent and only if this fails it tries
> > > > > > to modify its parent clock. What do you think about this solution?
> > > > > >
> > > >
> > > > I did some more testing and I'm still not happy with the current
> > > > solution. The problem is that if we change the SAI5 mclk clock parent it
> > > > can either support the 44kHz series or the 48kHz series. However, in the
> > > > case of HDMI we do not know in advance what the user wants.
> > > >
> > > > This means when testing either this works:
> > > > speaker-test -D hw:2,0 -r 48000 -c 2
> > > > or this works:
> > > > speaker-test -D hw:2,0 -r 44100 -c 2
> > > > With:
> > > > card 2: imxaudiohdmitx [imx-audio-hdmi-tx], device 0: i.MX HDMI i2s-hifi-0 [i.MX HDMI i2s-hifi-0]
> > >
> > > Are you using the setting below?  then should not either,  should both works
> > >  &sai5 {
> > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > +                       <&sai5_lpcg 0>;
> > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > +                                <722534400>, <45158400>, <11289600>,
> > > +                               <49152000>;
> > >         status = "okay";
> > >  };
> >
> > Sorry I didn't communicate that properly. Yes I was trying with these
> > settings but they do not work. The problem does not seem to be that the
> > driver can not adjust the rate for the audio (so e.g. 44kHz or 48kHz)
> > but that snd_soc_dai_set_sysclk in imx-hdmi.c fails with EINVAL. This
> > results in a call to fsl_sai_set_mclk_rate in fsl_sai.c with clk_id 1
> > (mclk_clk[1]) and a freq of 11289600 which causes the fail.
> > Interestingly, in fsl_sai_set_bclk we then only use clk_get_rate on
> > mclk_clk[0] which is set to audio_ipg_clk (rate 175000000) and we do not
> > use mclk_clk[1] anymore at all. This is why I'm not sure if this call to
> > snd_soc_dai_set_syclk is really necessary?
> >
> 
> Could you please check if you have the below commit in your test tree?
> 35121e9def07 clk: imx: imx8: Use clk_hw pointer for self registered
> clock in clk_parent_data
> 
> if not, please cherry-pick it.
> 
> The audio_ipg_clk can be selected if there is no other choice.
> but the rate 175000000 is not accurate for 44kHz. what we got
> is 44102Hz. This is the reason I don't like to use this source.

Thanks a lot for the hint, in my test setup I indeed have not applied
this commit. I tested it now with cherry-picking the commit and with
applying the change to the dts. This made it work. I will do some more
testing tomrrow and if I can't find any addtional issue I will use this
as a solution. 

Should I add the change to our Apalis board dtsi file or directly to
imx8qm-ss-audio.dtsi? I think it fixes kind of a general issue or not?

Thanks for your support
Stefan

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Shengjiu Wang]

On Fri, Jan 17, 2025 at 1:35 AM Stefan Eichenberger <eichest@gmail.com> wrote:
>
> Hi Shengjiu Wang,
>
> On Thu, Jan 16, 2025 at 03:30:55PM +0800, Shengjiu Wang wrote:
> > On Thu, Jan 16, 2025 at 3:24 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > >
> > > Hi Shengjiu Wang,
> > >
> > > On Thu, Jan 16, 2025 at 12:01:13PM +0800, Shengjiu Wang wrote:
> > > > On Wed, Jan 15, 2025 at 6:39 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > >
> > > > > On Wed, Jan 15, 2025 at 05:14:09PM +0800, Shengjiu Wang wrote:
> > > > > > On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Shengjiu Wang,
> > > > > > >
> > > > > > > On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > > > > > > > Hi Shengjiu Wang,
> > > > > > > >
> > > > > > > > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > > > > > > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > > > > > > > >
> > > > > > > > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > > > > > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > > > > > > > sysclk, returning an EINVAL error. The following error messages
> > > > > > > > > > highlight the issue:
> > > > > > > > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > > > > > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > > > > > > > >
> > > > > > > > > The reason for this error is that the current clock parent can't
> > > > > > > > > support the rate
> > > > > > > > > you require (I think you want 11289600).
> > > > > > > > >
> > > > > > > > > We can configure the dts to provide such source, for example:
> > > > > > > > >
> > > > > > > > >  &sai5 {
> > > > > > > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > > > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > > > +                       <&sai5_lpcg 0>;
> > > > > > > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > > > > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > > > > > > +                                <722534400>, <45158400>, <11289600>,
> > > > > > > > > +                               <49152000>;
> > > > > > > > >         status = "okay";
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > > Then your case should work.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > > > > > > > >
> > > > > > > > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > > > > > > > the driver don't get an error from clk_set_rate().
> > > > > > > >
> > > > > > > > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > > > > > > > problem if other SAIs use the same clock source but with different
> > > > > > > > rates?
> > > > > > > >
> > > > > > > > If we have to define fixed rates in the DTS or else the clock driver
> > > > > > > > will return an error, isn't that a problem? Maybe I should change the
> > > > > > > > sai driver so that it ignores the failure and just takes the rate
> > > > > > > > configured? In the end audio works, even if it can't set the requested
> > > > > > > > rate.
> > > > > > >
> > > > > > > The following clock tree change would allow the driver to work
> > > > > > > in our scenario:
> > > > > > > &sai5 {
> > > > > > >         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > > >                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
> > > > > > >         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
> > > > > > >         assigned-clock-rates = <0>, <11289600>;
> > > > > > > };
> > > > > >
> > > > > > In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
> > > > > > PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
> > > > > > which should fit for most audio requirements.
> > > > > >
> > > > > > >
> > > > > > > However, this means we have to switch the parent clock to the audio pll
> > > > > > > 1. For the simple setup with two SAIs, one for analog audio and one for
> > > > > > > HDMI this wouldn't be a problem. But I'm not sure if this is a good
> > > > > > > solution if a customer would add a third SAI which requires again a
> > > > > > > different parent clock rate.
> > > > > >
> > > > > > We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
> > > > > > PLL_1 for 44kHz,  even with a third SAI or more,  they should work.
> > > > > >
> > > > > > >
> > > > > > > One potential solution could be that the SAI driver tries to first
> > > > > > > derive the clock from the current parent and only if this fails it tries
> > > > > > > to modify its parent clock. What do you think about this solution?
> > > > > > >
> > > > >
> > > > > I did some more testing and I'm still not happy with the current
> > > > > solution. The problem is that if we change the SAI5 mclk clock parent it
> > > > > can either support the 44kHz series or the 48kHz series. However, in the
> > > > > case of HDMI we do not know in advance what the user wants.
> > > > >
> > > > > This means when testing either this works:
> > > > > speaker-test -D hw:2,0 -r 48000 -c 2
> > > > > or this works:
> > > > > speaker-test -D hw:2,0 -r 44100 -c 2
> > > > > With:
> > > > > card 2: imxaudiohdmitx [imx-audio-hdmi-tx], device 0: i.MX HDMI i2s-hifi-0 [i.MX HDMI i2s-hifi-0]
> > > >
> > > > Are you using the setting below?  then should not either,  should both works
> > > >  &sai5 {
> > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > +                       <&sai5_lpcg 0>;
> > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > +                                <722534400>, <45158400>, <11289600>,
> > > > +                               <49152000>;
> > > >         status = "okay";
> > > >  };
> > >
> > > Sorry I didn't communicate that properly. Yes I was trying with these
> > > settings but they do not work. The problem does not seem to be that the
> > > driver can not adjust the rate for the audio (so e.g. 44kHz or 48kHz)
> > > but that snd_soc_dai_set_sysclk in imx-hdmi.c fails with EINVAL. This
> > > results in a call to fsl_sai_set_mclk_rate in fsl_sai.c with clk_id 1
> > > (mclk_clk[1]) and a freq of 11289600 which causes the fail.
> > > Interestingly, in fsl_sai_set_bclk we then only use clk_get_rate on
> > > mclk_clk[0] which is set to audio_ipg_clk (rate 175000000) and we do not
> > > use mclk_clk[1] anymore at all. This is why I'm not sure if this call to
> > > snd_soc_dai_set_syclk is really necessary?
> > >
> >
> > Could you please check if you have the below commit in your test tree?
> > 35121e9def07 clk: imx: imx8: Use clk_hw pointer for self registered
> > clock in clk_parent_data
> >
> > if not, please cherry-pick it.
> >
> > The audio_ipg_clk can be selected if there is no other choice.
> > but the rate 175000000 is not accurate for 44kHz. what we got
> > is 44102Hz. This is the reason I don't like to use this source.
>
> Thanks a lot for the hint, in my test setup I indeed have not applied
> this commit. I tested it now with cherry-picking the commit and with
> applying the change to the dts. This made it work. I will do some more
> testing tomrrow and if I can't find any addtional issue I will use this
> as a solution.
>
I am thinking that your change may still be needed.

Your change plus the below change. then we can support more rate,
not only 48k/44kHz.  SAI driver will select the parent by itself before
the clk_set_rate().

+       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
+                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
+                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
+                       <&sai5_lpcg 0>;
+       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
+       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
+                                <722534400>, <45158400>, <11289600>,
+                               <49152000>;
+       clocks = <&sai5_lpcg 1>,
+               <&clk_dummy>,
+               <&sai5_lpcg 0>,
+               <&clk_dummy>,
+               <&clk_dummy>,
+               <&aud_pll_div0_lpcg 0>,
+               <&aud_pll_div1_lpcg 0>;
+       clock-names = "bus", "mclk0", "mclk1", "mclk2", "mclk3",
"pll8k", "pll11k";

> Should I add the change to our Apalis board dtsi file or directly to
> imx8qm-ss-audio.dtsi? I think it fixes kind of a general issue or not?

It is still board related, so I think it is better to add to the board dts file.

Best regards
Shengjiu Wang

>
> Thanks for your support
> Stefan

Re: [PATCH v1] clk: imx: imx8-acm: fix flags for acm clocks

[Stefan Eichenberger]

On Fri, Jan 17, 2025 at 02:56:15PM +0800, Shengjiu Wang wrote:
> On Fri, Jan 17, 2025 at 1:35 AM Stefan Eichenberger <eichest@gmail.com> wrote:
> >
> > Hi Shengjiu Wang,
> >
> > On Thu, Jan 16, 2025 at 03:30:55PM +0800, Shengjiu Wang wrote:
> > > On Thu, Jan 16, 2025 at 3:24 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > >
> > > > Hi Shengjiu Wang,
> > > >
> > > > On Thu, Jan 16, 2025 at 12:01:13PM +0800, Shengjiu Wang wrote:
> > > > > On Wed, Jan 15, 2025 at 6:39 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 15, 2025 at 05:14:09PM +0800, Shengjiu Wang wrote:
> > > > > > > On Wed, Jan 15, 2025 at 4:33 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Shengjiu Wang,
> > > > > > > >
> > > > > > > > On Tue, Jan 14, 2025 at 12:58:24PM +0100, Stefan Eichenberger wrote:
> > > > > > > > > Hi Shengjiu Wang,
> > > > > > > > >
> > > > > > > > > On Tue, Jan 14, 2025 at 03:49:10PM +0800, Shengjiu Wang wrote:
> > > > > > > > > > On Mon, Jan 13, 2025 at 5:54 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > > > > > > > > >
> > > > > > > > > > > Currently, the flags for the ACM clocks are set to 0. This configuration
> > > > > > > > > > > causes the fsl-sai audio driver to fail when attempting to set the
> > > > > > > > > > > sysclk, returning an EINVAL error. The following error messages
> > > > > > > > > > > highlight the issue:
> > > > > > > > > > > fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
> > > > > > > > > > > imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
> > > > > > > > > >
> > > > > > > > > > The reason for this error is that the current clock parent can't
> > > > > > > > > > support the rate
> > > > > > > > > > you require (I think you want 11289600).
> > > > > > > > > >
> > > > > > > > > > We can configure the dts to provide such source, for example:
> > > > > > > > > >
> > > > > > > > > >  &sai5 {
> > > > > > > > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > > > > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > > > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > > > > > > > +                       <&sai5_lpcg 0>;
> > > > > > > > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > > > > > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > > > > > > > +                                <722534400>, <45158400>, <11289600>,
> > > > > > > > > > +                               <49152000>;
> > > > > > > > > >         status = "okay";
> > > > > > > > > >  };
> > > > > > > > > >
> > > > > > > > > > Then your case should work.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > By setting the flag CLK_SET_RATE_NO_REPARENT, we signal that the ACM
> > > > > > > > > >
> > > > > > > > > > I don't think CLK_SET_RATE_NO_REPARENT is a good choice. which will cause
> > > > > > > > > > the driver don't get an error from clk_set_rate().
> > > > > > > > >
> > > > > > > > > Thanks for the proposal, I will try it out tomorrow. Isn't this a
> > > > > > > > > problem if other SAIs use the same clock source but with different
> > > > > > > > > rates?
> > > > > > > > >
> > > > > > > > > If we have to define fixed rates in the DTS or else the clock driver
> > > > > > > > > will return an error, isn't that a problem? Maybe I should change the
> > > > > > > > > sai driver so that it ignores the failure and just takes the rate
> > > > > > > > > configured? In the end audio works, even if it can't set the requested
> > > > > > > > > rate.
> > > > > > > >
> > > > > > > > The following clock tree change would allow the driver to work
> > > > > > > > in our scenario:
> > > > > > > > &sai5 {
> > > > > > > >         assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > > > >                           <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>;
> > > > > > > >         assigned-clock-parents = <&aud_pll_div1_lpcg 0>;
> > > > > > > >         assigned-clock-rates = <0>, <11289600>;
> > > > > > > > };
> > > > > > >
> > > > > > > In which we configure PLL_0 for 48KHz series (8kHz/16kHz/32kHz/48kHz),
> > > > > > > PLL_1 for 44kHz series (11kHz/22kHz/44kHz),
> > > > > > > which should fit for most audio requirements.
> > > > > > >
> > > > > > > >
> > > > > > > > However, this means we have to switch the parent clock to the audio pll
> > > > > > > > 1. For the simple setup with two SAIs, one for analog audio and one for
> > > > > > > > HDMI this wouldn't be a problem. But I'm not sure if this is a good
> > > > > > > > solution if a customer would add a third SAI which requires again a
> > > > > > > > different parent clock rate.
> > > > > > >
> > > > > > > We won't change the PLL's rate in the driver,  so as PLL_0 for 48kHz,
> > > > > > > PLL_1 for 44kHz,  even with a third SAI or more,  they should work.
> > > > > > >
> > > > > > > >
> > > > > > > > One potential solution could be that the SAI driver tries to first
> > > > > > > > derive the clock from the current parent and only if this fails it tries
> > > > > > > > to modify its parent clock. What do you think about this solution?
> > > > > > > >
> > > > > >
> > > > > > I did some more testing and I'm still not happy with the current
> > > > > > solution. The problem is that if we change the SAI5 mclk clock parent it
> > > > > > can either support the 44kHz series or the 48kHz series. However, in the
> > > > > > case of HDMI we do not know in advance what the user wants.
> > > > > >
> > > > > > This means when testing either this works:
> > > > > > speaker-test -D hw:2,0 -r 48000 -c 2
> > > > > > or this works:
> > > > > > speaker-test -D hw:2,0 -r 44100 -c 2
> > > > > > With:
> > > > > > card 2: imxaudiohdmitx [imx-audio-hdmi-tx], device 0: i.MX HDMI i2s-hifi-0 [i.MX HDMI i2s-hifi-0]
> > > > >
> > > > > Are you using the setting below?  then should not either,  should both works
> > > > >  &sai5 {
> > > > > +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> > > > > +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> > > > > +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> > > > > +                       <&sai5_lpcg 0>;
> > > > > +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> > > > > +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> > > > > +                                <722534400>, <45158400>, <11289600>,
> > > > > +                               <49152000>;
> > > > >         status = "okay";
> > > > >  };
> > > >
> > > > Sorry I didn't communicate that properly. Yes I was trying with these
> > > > settings but they do not work. The problem does not seem to be that the
> > > > driver can not adjust the rate for the audio (so e.g. 44kHz or 48kHz)
> > > > but that snd_soc_dai_set_sysclk in imx-hdmi.c fails with EINVAL. This
> > > > results in a call to fsl_sai_set_mclk_rate in fsl_sai.c with clk_id 1
> > > > (mclk_clk[1]) and a freq of 11289600 which causes the fail.
> > > > Interestingly, in fsl_sai_set_bclk we then only use clk_get_rate on
> > > > mclk_clk[0] which is set to audio_ipg_clk (rate 175000000) and we do not
> > > > use mclk_clk[1] anymore at all. This is why I'm not sure if this call to
> > > > snd_soc_dai_set_syclk is really necessary?
> > > >
> > >
> > > Could you please check if you have the below commit in your test tree?
> > > 35121e9def07 clk: imx: imx8: Use clk_hw pointer for self registered
> > > clock in clk_parent_data
> > >
> > > if not, please cherry-pick it.
> > >
> > > The audio_ipg_clk can be selected if there is no other choice.
> > > but the rate 175000000 is not accurate for 44kHz. what we got
> > > is 44102Hz. This is the reason I don't like to use this source.
> >
> > Thanks a lot for the hint, in my test setup I indeed have not applied
> > this commit. I tested it now with cherry-picking the commit and with
> > applying the change to the dts. This made it work. I will do some more
> > testing tomrrow and if I can't find any addtional issue I will use this
> > as a solution.
> >
> I am thinking that your change may still be needed.
> 
> Your change plus the below change. then we can support more rate,
> not only 48k/44kHz.  SAI driver will select the parent by itself before
> the clk_set_rate().

When testing the only two frequency that are not working and seem to be
supported by HDMI is first 88.2 kHz.
fsl-sai 59090000.sai: failed to derive required Tx rate: 5644800
fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_hw_params on 59090000.sai: -22

and second 32kHz:
fsl-sai 59090000.sai: failed to set clock rate (8192000): -22
fsl-sai 59090000.sai: ASoC: error at snd_soc_dai_set_sysclk on 59090000.sai: -22
imx-hdmi sound-hdmi: failed to set cpu sysclk: -22
 i.MX HDMI: ASoC: error at snd_soc_link_hw_params on i.MX HDMI: -22

For our use case this doesn't seem to be relevant at the moment. I will
clarify this internally.
> 
> +       assigned-clocks = <&acm IMX_ADMA_ACM_SAI5_MCLK_SEL>,
> +                       <&acm IMX_ADMA_ACM_AUD_CLK1_SEL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_PLL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_SLV_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_0 IMX_SC_PM_CLK_MST_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_PLL>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_SLV_BUS>,
> +                       <&clk IMX_SC_R_AUDIO_PLL_1 IMX_SC_PM_CLK_MST_BUS>,
> +                       <&sai5_lpcg 0>;
> +       assigned-clock-parents = <&aud_pll_div0_lpcg 0>, <&aud_rec1_lpcg 0>;
> +       assigned-clock-rates = <0>, <0>, <786432000>, <49152000>, <12288000>,
> +                                <722534400>, <45158400>, <11289600>,
> +                               <49152000>;
> +       clocks = <&sai5_lpcg 1>,
> +               <&clk_dummy>,
> +               <&sai5_lpcg 0>,
> +               <&clk_dummy>,
> +               <&clk_dummy>,
> +               <&aud_pll_div0_lpcg 0>,
> +               <&aud_pll_div1_lpcg 0>;
> +       clock-names = "bus", "mclk0", "mclk1", "mclk2", "mclk3",
> "pll8k", "pll11k";
> 
> > Should I add the change to our Apalis board dtsi file or directly to
> > imx8qm-ss-audio.dtsi? I think it fixes kind of a general issue or not?
> 
> It is still board related, so I think it is better to add to the board dts file.

Okay, then I will add this change to the board dts and prepare a v2.

Regards,
Stefan