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

["Xuyang Dong" <dongxuyang@eswincomputing.com>]

> >>
> >> 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@oldschoolsolutions.biz>
> >> 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 | 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
> >> @@ -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;
> >> +
> >> +	/* 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,7 +1545,7 @@ 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;
> >> @@ -1526,7 +1555,10 @@ static int __init clk_disable_unused(void)
> >>  		return 0;
> >>  	}
> >>  
> >> -	pr_info("clk: Disabling unused clocks\n");
> >> +	if (dev)
> >> +		dev_info(dev, "clk: Disabling unused clocks\n");
> >> +	else
> >> +		pr_info("clk: Disabling unused clocks not associated with a device\n");
> >>  
> >>  	ret = clk_pm_runtime_get_all();
> >>  	if (ret)
> >> @@ -1538,16 +1570,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,11 +1587,16 @@ 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)
> >>  {
> >> -	/* 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);
> >> +	}
> >>  	core->hw = hw;
> >>  	core->flags = init->flags;
> >>  	core->num_parents = init->num_parents;
> > 
> > Hi Brian,
> > 
> > Thank you for the patch set. I tested this in my tree on Sifive HiFive 
> > Premier P550, and it behaved correctly.
> > 
> > Tested-by: Xuyang Dong <dongxuyang@eswincomputing.com> # hfp550
> 
> If I understand correctly your platform, you tested nothing. You
> executed no code from common part and no code from the drivers listed
> here. This is not a real test in such case.
> 
> When you test code, please actually be sure you execute the tested code.
> 
> I am afraid the tag is not applicable.
> 

Hi Krzysztof and Brian,

I applied this series via b4 shazam 
https://lore.kernel.org/all/20260616-clk-sync-state-v2-0-15f82c64d95c@redhat.com/.

I dropped 'clk_ignore_unused' at compile time. After booting the system, 
I ran dmesg | grep "unused clock" and observed the following output:

[root@eswin-os:~]$ dmesg | grep "unused clock"
[    6.388986] clk: Disabling unused clocks not associated with a device
[    6.395585] eic7700-clock 51828000.clock-controller: clk: Disabling unused clocks
[    6.758786] eic7700-hsp-clock 50440000.clock-controller: clk: Disabling unused clocks
[root@eswin-os:~]$ 

Does this testing approach look correct to you?

Best regards,
Xuyang Dong