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

[Nicolas Frattaroli]

Hello, finally got around to responding to this.

On Wednesday, 24 June 2026 04:37:33 Central European Summer Time Bui Duc Phuc wrote:
> 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.

If "old source code" refers to what's currently in the tree with the
devm_clk_get_enabled, then yes. There's the manual resume function
call which isn't balanced.

I think your follow-up patch does make sense in light of that.

> 
> 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'll e-mail you off-list to see if we can rectify that.

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

Nope, I basically never build with PM/RPM disabled.

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

Hm, I'm worried only ungating the clock around register accesses isn't
going to go well if the clock is needed by the internal logic of the
device as well.

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

Agreed, this needs fixing. That is an odd behaviour of the PM API.

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