Re: [PATCH v2 5/5] clk: implement sync_state support

[Ulf Hansson]

On Wed, Jun 17, 2026 at 5:02 PM Brian Masney <bmasney@xxxxxxxxxx> wrote:
>
> Hi Ulf,
>
> On Wed, Jun 17, 2026 at 04:24:05PM +0200, Ulf Hansson wrote:
> > On Tue, Jun 16, 2026 at 11:09 PM Brian Masney <bmasney@xxxxxxxxxx> wrote:
> > >
> > > The existing support for disabling unused clks runs in the late initcall
> > > stage, and it has been known for a long time that this is broken since
> > > it runs too early in the boot up process. It doesn't work for kernel
> > > modules, and it also doesn't work if all of the consumers haven't fully
> > > probed yet. Folks have long recommended to boot certain platforms with
> > > clk_ignore_unused to work around issues with disabling unused clks.
> > >
> > > Let's go ahead and add a framework-level sync_state callback for the clk
> > > subsystem. If a driver doesn't have a sync_state callback configured,
> > > which is the 99+% use case today, then let's set it up to use the
> > > clk_sync_state() introduced in this commit so that no driver changes
> > > are needed.
> > >
> > > At the time of this writing, there are currently only 7 clk drivers that
> > > implement sync_state, and all are Qualcomm SoCs where they interact with
> > > the interconnect framework via icc_sync_state(). A shared helper has
> > > been created for this platform that calls clk_sync_state(). It is
> > > expected that any new clk drivers that want to implement their own
> > > sync_state will also need to call clk_sync_state() at the end of their
> > > custom sync_state callback.
> > >
> > > There will be several stages of disabling unused clks:
> > >
> > > - The first phase will be executed at late_initcall and it will only
> > >   disable unused clks that do not have a struct dev.
> > > - The sync_state callback will be invoked for each clk driver once all
> > >   consumers have probed.
> > >
> > > This is based on previous attempts by Saravana Kannan and Abel Vesa
> > > that are linked below.
> > >
> > > This change was tested on a Thinkpad x13s laptop.
> > >
> > >     [    0.308051] clk: Disabling unused clocks not associated with a device
> > >     [    6.541069] qcom_aoss_qmp c300000.power-management: clk: Disabling unused clocks
> > >     [    6.843310] qcom-qmp-pcie-phy 1c24000.phy: clk: Disabling unused clocks
> > >     [    7.604556] qcom-qmp-pcie-phy 1c14000.phy: clk: Disabling unused clocks
> > >     [    8.446161] qcom-qmp-usb-phy 88f1000.phy: clk: Disabling unused clocks
> > >     [    8.446293] qcom-qmp-usb-phy 88ef000.phy: clk: Disabling unused clocks
> > >     [    8.546067] qcom-qmp-combo-phy 88eb000.phy: clk: Disabling unused clocks
> > >     [    8.546203] qcom-qmp-combo-phy 8903000.phy: clk: Disabling unused clocks
> > >     [    8.546254] qcom-edp-phy aec5a00.phy: clk: Disabling unused clocks
> > >     [   15.436834] qcom-cpufreq-hw 18591000.cpufreq: clk: Disabling unused clocks
> > >     [   15.436953] clk-rpmh 18200000.rsc:clock-controller: clk: Disabling unused clocks
> > >     [   15.723348] qcom-qmp-pcie-phy 1c06000.phy: clk: Disabling unused clocks
> > >     [   21.063241] q6prm-lpass-clock 3000000.remoteproc:glink-edge:gpr:service@2:clock-controller: clk: Disabling unused clocks
> > >     [   21.081996] va_macro 3370000.codec: clk: Disabling unused clocks
> > >     [   21.092740] rx_macro 3200000.rxmacro: clk: Disabling unused clocks
> > >     [   21.118261] wsa_macro 3240000.codec: clk: Disabling unused clocks
> > >     [   21.128758] tx_macro 3220000.txmacro: clk: Disabling unused clocks
> > >
> > > Tested-by: Jens Glathe <jens.glathe@xxxxxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Brian Masney <bmasney@xxxxxxxxxx>
> > > Link: https://www.youtube.com/watch?v=tXYzM8yLIQA
> > > Link: https://lore.kernel.org/all/20210407034456.516204-1-saravanak@xxxxxxxxxx/
> > > Link: https://lore.kernel.org/all/20221227204528.1899863-1-abel.vesa@xxxxxxxxxx/
> >
> > For future revisions, please add ulfh@xxxxxxxxxx on to/cc.
> >
> > > ---
> > >  drivers/clk/clk.c | 72 +++++++++++++++++++++++++++++++++++++++++++++----------
> > >  1 file changed, 59 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 9cb2b42d1be4..7a15cceec620 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> >
> > [...]
> >
> > >  void clk_sync_state(struct device *dev)
> > >  {
> > > -       /* Will fill in */
> > > +       __clk_disable_unused(dev);
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_sync_state);
> > >
> > > @@ -4345,8 +4382,17 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
> > >         core->dev = dev;
> > >         clk_pm_runtime_init(core);
> > >         core->of_node = np;
> > > -       if (dev && dev->driver)
> > > +       if (dev && dev->driver) {
> > >                 core->owner = dev->driver->owner;
> > > +
> > > +               /*
> > > +                * If a clk provider sets their own sync_state, then it needs to
> > > +                * also call clk_sync_state(). dev_set_drv_sync_state() won't
> > > +                * overwrite the sync_state callback, and this call will fail
> > > +                * with -EBUSY.
> > > +                */
> > > +               dev_set_drv_sync_state(dev, clk_sync_state);
> >
> > We have cases where a device node represents a provider for multiple
> > types of resources, like clocks, power-domains (genpds), resets, etc,
> > as in the qcom case, for example.
> >
> > For power-domain provider drivers (genpd) we also try to assign the
> > ->sync_state() callback, see of_genpd_add_provider_simple() and
> > of_genpd_add_provider_simple(). This means the above doesn't play well
> > with how genpd behaves, so we need to figure out a way to manage these
> > cases.
> >
> > In this regard, we also have of_genpd_sync_state(), which allows a
> > genpd provider driver to explicitly call genpd's sync state function,
> > if/when needed.
> >
> > Unfortunately I am not able to suggest a detailed solution for how to
> > move this forward at this point, as it requires some more thinking and
> > I am heading for some vacation very soon.
>
> One approach I initially considered was to make it so that we can have a
> list of sync_state callbacks that can be added to. I already did some
> work on this, but I didn't think it was worth it for just the QC clk
> drivers in isolation, but it would address the concern here.

Right, maybe that would work.

One more thing to somewhat consider, is the problem I have been trying
to address for power-domains providers [1]. At least for genpd
providers, we need a more fine grained sync state solution, which also
must be able to co-exists with other subsystems sync state support. I
am not sure if something like this is needed for clocks too?

Kind regards
Uffe

[1]
https://lore.kernel.org/all/20260508123910.114273-1-ulf.hansson@xxxxxxxxxx/