Re: [PATCH 3/3] ASoC: renesas: fsi: Fix hang by enabling SPU clock

[Kuninori Morimoto]

Hi

Thank you for the patch

> From: bui duc phuc <phucduc.bui@xxxxxxxxx>
> 
> The FSI on r8a7740 requires the SPU clock to be enabled
> before accessing its registers.
> Without this clock, register access may lead to a system
> hang.
> Retrieve the "spu" clock in probe and enable it during
> DAI startup. Disable the clock on shutdown to match the
> audio stream lifecycle.
> This ensures safe register access and prevents system
> hangs during audio playback.
> This is required even if the FSI functional clock is
> enabled, as internal units depend on the SPU clock.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@xxxxxxxxx>
> ---
(snip)
> @@ -1554,6 +1555,11 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
>                            struct snd_soc_dai *dai)
>  {
>         struct fsi_priv *fsi = fsi_get_priv(substream);
> +       int ret;
> +
> +       ret = clk_prepare_enable(fsi->master->clk_spu);
> +       if (ret)
> +               return ret;
> 
>         fsi_clk_invalid(fsi);

If it is needed for register access, you need to call it on
fsi_hw_startup/shutdown() which cares suspend/resume too.

And I guess it need to count user, because we have FSI-A / FSI-B ?

> @@ -1963,6 +1970,13 @@ static int fsi_probe(struct platform_device *pdev)
>         master->core            = core;
>         spin_lock_init(&master->lock);
> 
> +       /* SPU clock is required for FSI register access */
> +       master->clk_spu = devm_clk_get(&pdev->dev, "spu");
> +       if (IS_ERR(master->clk_spu)) {
> +               dev_err(&pdev->dev, "Failed to get spu clock\n");
> +               return PTR_ERR(master->clk_spu);
> +       }

As Mark mentioned, it should be optional. Otherwise it breaks compatibility.
And we already have fsi_clk_init() for clock initialize.
spu should be handled in it.

Now, it is called if clock master (A.

(A)	if (fsi_is_clk_master(fsi)) {
		if (fsi->clk_cpg)
			fsi_clk_init(dev, fsi, 0, 1, 1,
				     fsi_clk_set_rate_cpg);
		else
			fsi_clk_init(dev, fsi, 1, 1, 0,
				     fsi_clk_set_rate_external);
	}

I think it (A) can be checked inside fsi_clk_init().
fsi_clk_init() is now called when .set_fmt, but it can be called
at _probe() timing ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto