Re: [PATCH 3/5] clk: qcom: common: Attach clock power domains conditionally

[Jagadeesh Kona <quic_jkona@quicinc.com>]

On 2/20/2025 4:18 PM, Dmitry Baryshkov wrote:
> On Thu, Feb 20, 2025 at 12:43:42PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 2/19/2025 5:27 PM, Dmitry Baryshkov wrote:
>>> On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote:
>>>>
>>>>
>>>> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote:
>>>>>> Attach clock power domains in qcom_cc_really_probe() only
>>>>>> if the clock controller has not already attached to them.
>>>>>
>>>>> Squash this to the previous patch and call the new function. No need to
>>>>> duplicate the code.
>>>>>
>>>>
>>>> I tried calling the new function here instead of duplicating code, but that
>>>> is leading to below warning since the desc passed to qcom_cc_really_probe()
>>>> has a const qualifier and hence we cannot update desc->pd_list inside
>>>> qcom_cc_really_probe().
>>>>
>>>> drivers/clk/qcom/common.c:305:33:   WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>>>
>>> It sounds like this can be fixed with a one-line patch.
>>>
>>
>> Removing const qualifier to qcom_cc_really_probe() will fix this, but that requires changes in
>> many other drivers which are currently passing const descriptor to it.
> 
> And this points out that the pd_list should not be a part of the
> struct qcom_cc_desc. You are not using it in the code, so allocate that
> memory on the fly, pass it to devm_pm_domain_attach_list() and then
> forget about it.
> 

Above suggestion looks good, but we need to store the pd_list to pass it to GDSC driver to attach
the power domains as GDSC parent domains. Instead, we can add a new API wrapper for attaching PDs
and to map the regmap(qcom_cc_attach_pds_map) and all clock drivers that need multiple power domains
support can update to use below new API and all new clock drivers can just use the new API.

The implementation would be something like below

--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
+struct regmap *qcom_cc_attach_pds_map(struct platform_device *pdev, struct qcom_cc_desc *desc)
+{
+       int ret;
+
+       ret = devm_pm_domain_attach_list(&pdev->dev, NULL, &desc->pd_list);
+       if (ret < 0 && ret != -EEXIST)
+               return ERR_PTR(ret);
+
+       return qcom_cc_map(pdev, desc);
+}
+EXPORT_SYMBOL_GPL(qcom_cc_attach_pds_map);
+


--- a/drivers/clk/qcom/videocc-sm8550.c
+++ b/drivers/clk/qcom/videocc-sm8550.c
@@ -542,6 +542,12 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
        int ret;
        u32 sleep_clk_offset = 0x8140;

+       regmap = qcom_cc_attach_pds_map(pdev, &video_cc_sm8550_desc);
+       if (IS_ERR(regmap)) {
+               pm_runtime_put(&pdev->dev);
+               return PTR_ERR(regmap);
+       }
+
        ret = devm_pm_runtime_enable(&pdev->dev);
        if (ret)
                return ret;
@@ -550,12 +556,6 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
        if (ret)
                return ret;

-       regmap = qcom_cc_map(pdev, &video_cc_sm8550_desc);
-       if (IS_ERR(regmap)) {
-               pm_runtime_put(&pdev->dev);
-               return PTR_ERR(regmap);
-       }
-

This way also, we are aligning more towards common code and the code will be uniform across all
clock drivers and this doesn't require separate callback in each individual clock driver.

Thanks,
Jagadeesh 

>>
>> But I can squash this to my previous patch by updating my qcom_cc_attach_pds() function
>> prototype as below and then calling that new function here
>>
>> -int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc)
>> +int qcom_cc_attach_pds(struct device *dev, struct dev_pm_domain_list *pd_list)
>>
>> -               ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>> -               if (ret < 0 && ret != -EEXIST)
>> +               ret = qcom_cc_attach_pds(dev, cc->pd_list);
>> +               if (ret)
>>
>> Thanks,
>> Jagadeesh
>>
>>>>
>>>> Thanks,
>>>> Jagadeesh
>>>>
>>>>>>
>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>>>>>> ---
>>>>>>  drivers/clk/qcom/common.c | 9 ++++++---
>>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>>>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644
>>>>>> --- a/drivers/clk/qcom/common.c
>>>>>> +++ b/drivers/clk/qcom/common.c
>>>>>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev,
>>>>>>  	if (!cc)
>>>>>>  		return -ENOMEM;
>>>>>>  
>>>>>> -	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>>>>>> -	if (ret < 0 && ret != -EEXIST)
>>>>>> -		return ret;
>>>>>> +	cc->pd_list = desc->pd_list;
>>>>>> +	if (!cc->pd_list) {
>>>>>> +		ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>>>>>> +		if (ret < 0 && ret != -EEXIST)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>>  
>>>>>>  	reset = &cc->reset;
>>>>>>  	reset->rcdev.of_node = dev->of_node;
>>>>>>
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>>>
>>>
>