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

[Bui Duc Phuc]

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