Message-ID: <a3903d37-03ab-4f1c-acf0-4683d1297906@quicinc.com>
Date: Thu, 20 Feb 2025 12:43:00 +0530
From: Jagadeesh Kona <quic_jkona@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Bjorn Andersson
	<andersson@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        "Stephen
 Boyd" <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Konrad Dybcio
	<konradybcio@...nel.org>,
        Ajit Pandey <quic_ajipan@...cinc.com>,
        Imran Shaik
	<quic_imrashai@...cinc.com>,
        Taniya Das <quic_tdas@...cinc.com>,
        "Satya Priya
 Kakitapalli" <quic_skakitap@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple
 power domains



On 2/19/2025 5:32 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 19, 2025 at 05:08:52PM +0530, Jagadeesh Kona wrote:
>>
>>
>> On 2/18/2025 10:49 PM, Dmitry Baryshkov wrote:
>>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>>> During boot-up, the PLL configuration might be missed even after
>>>>> calling pll_configure() from the clock controller probe. This can
>>>>> happen because the PLL is connected to one or more rails that are
>>>>> turned off, and the current clock controller code cannot enable
>>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>>> with suboptimal settings, causing functional issues.
>>>>>
>>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>>> Therefore, add support to attach multiple power domains to videocc on
>>>>> these platforms.
>>>>>
>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@...cinc.com>
>>>>> ---
>>>>>   drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>>>   drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>>>   2 files changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>>>   	struct regmap *regmap;
>>>>>   	int ret;
>>>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>>   	ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>   	if (ret)
>>>>>   		return ret;
>>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>>>   	int ret;
>>>>>   	u32 sleep_clk_offset = 0x8140;
>>>>> +	ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>>   	ret = devm_pm_runtime_enable(&pdev->dev);
>>>>>   	if (ret)
>>>>>   		return ret;
>>>>>
>>>>
>>>> What's the difference between doing the attach here or doing it in
>>>> really_probe() ?
>>>
>>> I'd second this. If the domains are to be attached before calling any
>>> other functions, move the call to the qcom_cc_map(), so that all drivers
>>> get all domains attached before configuring PLLs instead of manually
>>> calling the function.
>>>
>>
>> I earlier tried moving the attach PDs call to qcom_cc_map(), but I faced the below issues
>> 1. desc passed to qcom_cc_map() has const qualifier, so updating desc->pd_list
>>    inside qcom_cc_map() is leading to a warning.
> 
> And? Can you fix the warning?
> 

I can remove the const qualifier in qcom_cc_map() prototype to fix this, but that requires changes
in many other clock drivers also since they are currently passing const descriptor to qcom_cc_map().
So would like to keep the qcom_cc_map() unchanged.

>> 2. If we attach the PDs after calling get_sync() on device, I observed
>>    that PDS are not getting enabled during probe. Currently qcom_cc_map()
>>    is called after get_sync() is already called on device.
> 
> Move PM handling into qcom_cc_map(). Then together with the Bryan's
> proposal most of the probe() functions can just call qcom_cc_probe()
> 

I agree with this approach to move entire PM handling to qcom_cc_map() but one concern is const
qualifier mentioned above and it also enables runtime PM for clock controllers that doesn't need
any runtime PM(e.g:- GCC/GPUCC). That may not cause any issue but we also need to see from where
we need to call pm_runtime_put().

We may have to add pm_runtime_put() at the end of both qcom_cc_probe() and qcom_cc_really_probe()
to move the device back to suspend after probe. But ideally runtime PM is not required for most
clock controllers except MMCC's that have MMCX dependency. Please let me know your thoughts on this.

Thanks,
Jagadeesh

>>
>> Probably, we can add a new function qcom_cc_attach_pds_map() where we can
>> attach PDs and call qcom_cc_map() inside it. We can then invoke this new
>> function at the start of probe before get_sync(). I will post this change
>> in next version if this aligns with your thoughts.
>>
>> Thanks,
>> Jagadeesh
> 

Powered by blists - more mailing lists