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

[Konrad Dybcio]

On 6/25/26 1:38 PM, Brian Masney wrote:
> 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?

Aaah right, my mistake. I assumed that adding that binding
qcom_cc_sync_state to all qcom clock drivers was the next step.

Konrad