Re: [PATCH 4/4] clk: implement sync_state support

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

Hi Brian, Alexandre,

thanks for looking into this! Alexandre asked me to confirm that
it will work for RTC devices, but my current understanding is that
it will not and here is why:

On Fri, 2026-06-26 at 12:32 -0400, Brian Masney 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.366737] clk: Disabling unused clocks not associated with a device
>     [    0.367232] PM: genpd: Disabling unused power domains
>     [    7.791413] qcom-qmp-pcie-phy 1c24000.phy: clk: Disabling unused clocks
>     [    7.799702] qcom_aoss_qmp c300000.power-management: clk: Disabling unused clocks
>     [    8.548820] qcom-qmp-pcie-phy 1c14000.phy: clk: Disabling unused clocks
>     [    9.121849] qcom-qmp-usb-phy 88f1000.phy: clk: Disabling unused clocks
>     [    9.121985] qcom-qmp-usb-phy 88ef000.phy: clk: Disabling unused clocks
>     [    9.122691] qcom-edp-phy aec5a00.phy: clk: Disabling unused clocks
>     [    9.122760] disp_cc-sc8280xp af00000.clock-controller: clk: Disabling unused clocks
>     [    9.142121] qcom-qmp-combo-phy 88eb000.phy: clk: Disabling unused clocks
>     [    9.169149] qcom-qmp-combo-phy 8903000.phy: clk: Disabling unused clocks
>     [   16.057997] qcom-cpufreq-hw 18591000.cpufreq: clk: Disabling unused clocks
>     [   16.058149] clk-rpmh 18200000.rsc:clock-controller: clk: Disabling unused clocks
>     [   16.334879] qcom-qmp-pcie-phy 1c06000.phy: clk: Disabling unused clocks
>     [   16.706113] camcc-sc8280xp ad00000.clock-controller: clk: Disabling unused clocks
>     [   21.565731] q6prm-lpass-clock 3000000.remoteproc:glink-edge:gpr:service@2:clock-controller: clk: Disabling unused clocks
>     [   21.597069] va_macro 3370000.codec: clk: Disabling unused clocks
>     [   21.605039] rx_macro 3200000.rxmacro: clk: Disabling unused clocks
>     [   21.630313] wsa_macro 3240000.codec: clk: Disabling unused clocks
>     [   21.635069] tx_macro 3220000.txmacro: clk: Disabling unused clocks
> 
> Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on AML-S905X-CC, AML-S805X-CC, BPI-M2S, BPI-M5
> Tested-by: Xuyang Dong <dongxuyang@eswincomputing.com> # hfp550
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> Link: https://www.youtube.com/watch?v=tXYzM8yLIQA
> Link: https://lore.kernel.org/all/20210407034456.516204-1-saravanak@google.com/
> Link: https://lore.kernel.org/all/20221227204528.1899863-1-abel.vesa@linaro.org/
> ---
>  drivers/clk/clk.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
>  include/linux/clk.h | 14 ++++++++++
>  2 files changed, 79 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 048adfa86a5d..2b6fae54702a 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1442,14 +1442,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
>  	clk_core_unprepare_lock(core);
>  }
>  
> -static void __init clk_unprepare_unused_subtree(struct clk_core *core)
> +/*
> + * Returns true if @core should be skipped during an unused-clock sweep for
> + * @dev.  When @dev is NULL the sweep is the global late_initcall pass; when
> + * @dev is non-NULL the sweep is a per-device sync_state pass.
> + */
> +static bool clk_core_skip_unused(struct clk_core *core, struct device *dev)
> +{
> +	/*
> +	 * At late_initcall, skip clocks that belong to a device — they will be
> +	 * handled at sync_state time.
> +	 */
> +	if (!dev && core->dev)
> +		return true;

This will skip all core->dev != NULL, but the deferred sync-based disabling
will only work for those driver-bound devices, not driverless class devices?
Probably a regression in power consumption as-is, see also below...

> +
> +	/* When called from sync_state, only process clocks for this device. */
> +	if (dev && core->dev != dev)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void clk_unprepare_unused_subtree(struct clk_core *core,
> +					 struct device *dev)
>  {
>  	struct clk_core *child;
>  
>  	lockdep_assert_held(&prepare_lock);
>  
>  	hlist_for_each_entry(child, &core->children, child_node)
> -		clk_unprepare_unused_subtree(child);
> +		clk_unprepare_unused_subtree(child, dev);
> +
> +	if (clk_core_skip_unused(core, dev))
> +		return;
>  
>  	if (core->prepare_count)
>  		return;
> @@ -1467,7 +1492,8 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>  	}
>  }
>  
> -static void __init clk_disable_unused_subtree(struct clk_core *core)
> +static void clk_disable_unused_subtree(struct clk_core *core,
> +				       struct device *dev)
>  {
>  	struct clk_core *child;
>  	unsigned long flags;
> @@ -1475,7 +1501,10 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>  	lockdep_assert_held(&prepare_lock);
>  
>  	hlist_for_each_entry(child, &core->children, child_node)
> -		clk_disable_unused_subtree(child);
> +		clk_disable_unused_subtree(child, dev);
> +
> +	if (clk_core_skip_unused(core, dev))
> +		return;
>  
>  	if (core->flags & CLK_OPS_PARENT_ENABLE)
>  		clk_core_prepare_enable(core->parent);
> @@ -1508,7 +1537,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>  		clk_core_disable_unprepare(core->parent);
>  }
>  
> -static bool clk_ignore_unused __initdata;
> +static bool clk_ignore_unused;
>  static int __init clk_ignore_unused_setup(char *__unused)
>  {
>  	clk_ignore_unused = true;
> @@ -1516,17 +1545,27 @@ static int __init clk_ignore_unused_setup(char *__unused)
>  }
>  __setup("clk_ignore_unused", clk_ignore_unused_setup);
>  
> -static int __init clk_disable_unused(void)
> +static int __clk_disable_unused(struct device *dev)
>  {
>  	struct clk_core *core;
>  	int ret;
>  
>  	if (clk_ignore_unused) {
> -		pr_warn("clk: Not disabling unused clocks\n");
> +		/*
> +		 * Only show this message once for the providers that are not
> +		 * associated with a device, which is the first pass of
> +		 * disabling unused clocks. Otherwise it'll show up for each
> +		 * clk provider as well.
> +		 */
> +		if (!dev)
> +			pr_warn("clk: Not disabling unused clocks\n");
>  		return 0;
>  	}
>  
> -	pr_info("clk: Disabling unused clocks\n");
> +	if (dev)
> +		dev_info(dev, "clk: Disabling unused clocks\n");

It becomes quite chatty on large systems?

> +	else
> +		pr_info("clk: Disabling unused clocks not associated with a device\n");
>  
>  	ret = clk_pm_runtime_get_all();
>  	if (ret)
> @@ -1538,16 +1577,16 @@ static int __init clk_disable_unused(void)
>  	clk_prepare_lock();
>  
>  	hlist_for_each_entry(core, &clk_root_list, child_node)
> -		clk_disable_unused_subtree(core);
> +		clk_disable_unused_subtree(core, dev);
>  
>  	hlist_for_each_entry(core, &clk_orphan_list, child_node)
> -		clk_disable_unused_subtree(core);
> +		clk_disable_unused_subtree(core, dev);
>  
>  	hlist_for_each_entry(core, &clk_root_list, child_node)
> -		clk_unprepare_unused_subtree(core);
> +		clk_unprepare_unused_subtree(core, dev);
>  
>  	hlist_for_each_entry(core, &clk_orphan_list, child_node)
> -		clk_unprepare_unused_subtree(core);
> +		clk_unprepare_unused_subtree(core, dev);
>  
>  	clk_prepare_unlock();
>  
> @@ -1555,8 +1594,19 @@ static int __init clk_disable_unused(void)
>  
>  	return 0;
>  }
> +
> +static int __init clk_disable_unused(void)
> +{
> +	return __clk_disable_unused(NULL);
> +}
>  late_initcall_sync(clk_disable_unused);
>  
> +void clk_sync_state(struct device *dev)
> +{
> +	__clk_disable_unused(dev);
> +}
> +EXPORT_SYMBOL_GPL(clk_sync_state);
> +
>  static int clk_core_determine_round_nolock(struct clk_core *core,
>  					   struct clk_rate_request *req)
>  {
> @@ -4339,8 +4389,10 @@ __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) {

So this will only work for driver-bound devices, but not for RTC-class
devices, right?

In the use-case we've discussed with Alexandre:

clk = devm_clk_register(&pcf85063->rtc->dev, &pcf85063->clkout_hw);

&pcf85063->rtc->dev is an rtc_class device -- it never has dev->driver set.

>  		core->owner = dev->driver->owner;
> +		dev_add_sync_state(dev, clk_sync_state);
> +	}
>  	core->hw = hw;
>  	core->flags = init->flags;
>  	core->num_parents = init->num_parents;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 998ba3f261da..31a0c9224c46 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -846,6 +846,20 @@ void devm_clk_put(struct device *dev, struct clk *clk);
>   */
>  
>  
> +/*
> + * clk_sync_state - sync_state callback to disable unused clocks
> + * @dev: the clock provider device whose unused clocks should be disabled
> + *
> + * It is called by the driver core once all consumers of @dev have probed,
> + * and disables any clocks belonging to @dev that are unused at that point.
> + *
> + * If a clock provider doesn't have a sync_state callback, then the framework
> + * will set up clk_sync_state() on your drivers behalf. If your driver needs

That is only true if dev->driver is set at __clk_register() time?

> + * a sync_state callback, then that callback also needs to call
> + * clk_sync_state().
> + */
> +void clk_sync_state(struct device *dev);
> +
>  /**
>   * clk_round_rate - adjust a rate to the exact rate a clock can provide
>   * @clk: clock source

I've also tested the series with regards to PCF85053 RTC clkout issue, but
the clock is not being disabled in practice.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com