diff mbox series

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

Message ID 20250113094654.12998-1-eichest@gmail.com (mailing list archive)
State New
Headers show
Series [v1] clk: imx: imx8-acm: fix flags for acm clocks | expand

Commit Message

Stefan Eichenberger Jan. 13, 2025, 9:46 a.m. UTC
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(-)

Comments

Shengjiu Wang Jan. 14, 2025, 7:49 a.m. UTC | #1
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
>
>
Stefan Eichenberger Jan. 14, 2025, 11:58 a.m. UTC | #2
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
diff mbox series

Patch

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);