Re: [PATCH] ASoC: rockchip: rockchip_sai: Hand over hclk control exclusively to Runtime PM

[Bui Duc Phuc]

Hi all,

Regarding the case where PM configuration is not enabled, with the old
source code, I suspect there is an unbalanced clk_disable_unprepare()
call on hclk when the driver is unbound after a successful probe under
CONFIG_PM=n.

The actual enable_count / prepare_count sequence for hclk (with values
clamped at 0) would be:

probe:

devm_clk_get_enabled           0 -> 1
runtime_resume (manual)       1 -> 2
clk_disable_unprepare            2 -> 1 (at the end of probe)

unbind:

remove -> runtime_suspend   1 -> 0 (ops->disable/unprepare executed here)
devm cleanup                          0 -> WARN "already disabled" /
"already unprepared"

This conclusion is based solely on code inspection; I do not have
hardware available to verify it.
I noticed that Nicolas tested and ACKed the use of devm_clk_get_enabled(),
so I'm not sure whether that testing included the CONFIG_PM=n configuration.

https://lore.kernel.org/all/2818018.CQOukoFCf9@workhorse/

If it did, then I may have overlooked something.

@Nicolas (or anyone familiar with this), could you please help
double-check if my understanding is correct?

Best regards,
Phuc



On Tue, Jun 23, 2026 at 5:53 PM Bui Duc Phuc <phucduc.bui@xxxxxxxxx> wrote:
>
> Hi Mark,
>
> Thank you for your review.
>
> > >    1 Reverting back to devm_clk_get() to remove the implicit devres
> > >      enable/disable behavior.
> > >    2 Manually enabling and disabling hclk explicitly only around the
> > >      early register access before Runtime PM takes over.
> > >    3 Dropping the stray clk_disable_unprepare() at the end of probe()
> > >      so Runtime PM solely owns hclk afterward.
> >
> > Note that runtime PM can be disabled at build time so we might not have
> > runtime PM at all...
> >
>
> Thanks for pointing this out. You're right that with !CONFIG_PM, the
> driver only relies on the
> two manual calls to rokchip_sai_runtime_resume() / suspend(), so hclk
> stays enabled the
> whole time. I understand this is unvavoidable in that configuration,
> throgh, since there's no
> Runtime PM to re-enable the clock when it's needed.
>
> I'll update the commit message to reflect that the driver uses a
> combination of Runtime PM
> and explicit manual enable/disable, rather than relying on Runtime PM alone.
>
> > > Links:
> > > 1 This change is based on the discussion around manual hclk handing during probe(),
> > >   as raised by Krysztof:
> > >   https://lore.kernel.org/all/20e4754b-ea9a-404d-b529-ec44a7263cbf@xxxxxxxxxx/#t
> > > 2 Background for the earlier devm_clk_get_enbabled() conversion:
> > >   https://lore.kernel.org/all/2818018.CQOukoFCf9@workhorse/
> >
> > > An alternative approach would be use devm_regmap_init_mmio_clk() and let regmap
> > > manage clock enablement around register accesses. If preferred, I can rework the
> > > driver accordingly.
> >
> > > -     sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
> > > +     sai->hclk = devm_clk_get(&pdev->dev, "hclk");
> > >       if (IS_ERR(sai->hclk))
> > >               return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
> > >                                    "Failed to get hclk\n");
> > >
> > > +     ret = clk_prepare_enable(sai->hclk);
> > > +     if (ret)
> > > +             return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
> > > +
> >
> > > @@ -1482,8 +1492,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
> > >       pm_runtime_use_autosuspend(&pdev->dev);
> > >       pm_runtime_put(&pdev->dev);
> > >
> > > -     clk_disable_unprepare(sai->hclk);
> > > -
> > >       return 0;
> >
> > Are you sure that the runtime PM state there is such that it knows a
> > reference is held?  The driver used pm_runtime_get_noresume() so the
> > device didn't have RPM_ACTIVE set I think?
>
>
> You are right, pm_runtime_get_noresume() doesn't set RPM_ACTIVE. I
> think we need to add
> pm_runtime_set_active() before pm_runtime_enable(). Otherwise, with CONFIG_PM,
> the pm_runtime_put() at the end of probe() might skip the suspend,
> since the core still considers
> the device suspended .
>
> >
> > The runtime PM API really is a miserable collection of landmines :(
>
> Yeah, plenty of landmines indeed :(
> I checked, and rockchip_spdif.c does use devm_regmap_init_mmio_clk() for hclk,
> rather than wrapping every register access in pm_runtime_get_sync() /
> pm_runtime_put()
> the way rockchip_sai does.
>
> Best regards,
> Phuc