Re: [PATCH v2 2/5] clk: qcom: common: introduce qcom_cc_sync_state()

[Brian Masney]

Hi Konrad,

On Thu, Jun 25, 2026 at 11:44:12AM +0200, Konrad Dybcio wrote:
> On 6/16/26 11:09 PM, Brian Masney wrote:
> > Several qcom clk providers currently have a sync_state helper set to
> > icc_sync_state(). With an upcoming change to the clk framework, if
> > sync_state is not defined for the device, then the clk framework sets it
> > to clk_sync_state().
> > 
> > Clk providers that require their own sync_state will need to call the
> > framework level clk_sync_state(). Let's introduce a new common helper
> > qcom_cc_sync_state() that calls icc_sync_state() and clk_sync_state().
> > 
> > Tested-by: Jens Glathe <jens.glathe@xxxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Brian Masney <bmasney@xxxxxxxxxx>
> > ---
> >  drivers/clk/qcom/common.c | 9 +++++++++
> >  drivers/clk/qcom/common.h | 1 +
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > index eec369d2173b..31382c49c948 100644
> > --- a/drivers/clk/qcom/common.c
> > +++ b/drivers/clk/qcom/common.c
> > @@ -3,12 +3,14 @@
> >   * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
> >   */
> >  
> > +#include <linux/clk.h>
> >  #include <linux/export.h>
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/clk-provider.h>
> >  #include <linux/interconnect-clk.h>
> > +#include <linux/interconnect-provider.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/reset-controller.h>
> >  #include <linux/of.h>
> > @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
> >  
> > +void qcom_cc_sync_state(struct device *dev)
> > +{
> > +	icc_sync_state(dev);
> 
> As mentioned before, we really need to test for (qcom_cc_desc)->icc_hws
> here
> 
> If icc_sync_state() is called without an interconnect provider
> being registered, the framework state gets messed up:
> 
> --- drivers/interconnect/core.c
> void icc_sync_state(struct device *dev)
> {
> 	struct icc_provider *p;
> 	struct icc_node *n;
> 	static int count; // function-static variable
> 
> 	count++; // called for every clock controller in this revision
> 
> 	if (count < providers_count) // kaboom
> 		return;
> 
> 	// actual sync_state never happens anymore
> 
> Presumably one would pass this through drvdata, but be careful as
> some drivers use it for other purposes today

My next version of this series that I haven't posted yet allows chaining
the sync_state callbacks at the driver core level. It doesn't require
any of the QC clk driver changes, and will allow us to play nicely with
the pmdomain subsystem, and any others the move to sync_state in the
future.

I think I see the confusion between us the last few rounds of review. In
this series, I added qcom_cc_sync_state() and converted 6 drivers over to
use it. (I excluded clk-cbf-8996.c since it is separate.) Only the 6
drivers today that called icc_sync_state() now call qcom_cc_sync_state() ->
icc_sync_state(). So from my vantage point it is the same overall
functionality.

I didn't look at this from the perspective of qcom_cc_sync_state() would
be common infrastructure, and a newly added driver in the future that may
not interact with the ICC framework may use this. Is this correct?

I asked earlier if this was an existing bug, and the response was no.

Once I fix my x13s today, and able to verify my v3 sync_state still
works as expected, I'll post the new version.

Brian