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

[Bui Duc Phuc]

Hi Morimoto-san, Mark,

Thank you for your review.

> If it is needed for register access,

Yes, enabling this clock is essential as it functions as a bus bridge clock.
Currently, the SPU clock is still enabled by the bootloader. In legacy
kernels (v4.2 and earlier) using the Armadillo board-file/defconfig, this
clock remained active after boot, allowing the FSI to function correctly.
However, after migrating to a full Device Tree (DTS) implementation,
the kernel's unused clock cleanup mechanism disables the SPU clock
because it isn't explicitly claimed. This leads to a system hang every
time aplay is executed, as the FSI registers become inaccessible
without this clock.

> you need to call it on
> fsi_hw_startup/shutdown() which cares suspend/resume too.

I previously attempted to manage the clock within fsi_hw_startup/
shutdown, but the system would hang when stopping aplay
(e.g., via Ctrl+C). This happens because certain cleanup operations,
such as fsi_irq_disable(), are performed after fsi_hw_shutdown()
finishes. These operations require register access, which triggers a
system hang if the SPU clock has already been disabled. Therefore,
I moved the clock management to fsi_dai_startup/shutdown to ensure
the clock remains active throughout the entire lifecycle of the stream.

Furthermore, my testing shows that using dai_startup/shutdown
eliminates the need for explicit Suspend/Resume handling for this clock.
Since the ALSA framework typically invokes the hw_ callbacks during
power management transitions rather than the dai_ ones, the SPU clock
state remains stable, preventing any illegal register access during
these transitions.

> As Mark mentioned, it should be optional.
> Otherwise it breaks compatibility.

You are right. I will implement it this way in v2.

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

You are right. Currently, our FSIA is configured as a slave,
so it never executes the clk_init() function.

> 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 ?

Yes. I can handle the implementation/coding side of this.

> Should we also be managing the clock during system suspend, or if the
> power consumption doesn't really matter should we just keep it enabled
> all the time and not worry about starting and stopping it?

Regarding the SPU clock management, I haven't measured the exact
power consumption of this block yet. However, to keep the code simple
and ensure maximum stability for register access (avoiding system
hangs during cleanup), I am open to enabling it once in fsi_probe()
if you find the dynamic management in dai_startup/shutdown
unnecessary.

> This is going to unconditionally require a clock called "spu" on all
> devices using this driver, not just the one SoC you mentioned as
> requiring it.  Presumably this worked at least somewhere (possibly the
> clock is always on, or they're just lucky that something else enables
> it) and this will cause regressions for those platforms?
> This should either (ideally) be conditional, or use _optional.

Thank you for your suggestion. I will switch to using
devm_clk_get_optional() in the v2

Best regards,
Phuc

On Mon, Apr 6, 2026 at 6:52 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
>
>
> 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