linux-kernel - Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple power domains

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a06f1e5b-78fb-4f95-96bb-985a585d4706@quicinc.com>
Date: Mon, 24 Feb 2025 12:07:21 +0530
From: Jagadeesh Kona <quic_jkona@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        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>
CC: 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/22/2025 12:49 AM, Konrad Dybcio wrote:
> On 18.02.2025 4:46 PM, 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() ?
>>
>> There doesn't seem to be any difference except that we will have an additional delay introduced.
>>
>> Are you describing a race condition ?
>>
>> I don't see _logic_ here to moving the call into the controller's higher level probe.
>>
>> Can you describe some more ?
> 
> I think this is a sane course of action:
> 
> 1. associate pll config with the pll (treewide change)
> 2. add a generic pll_configure call that parses the type
> 3. store PLLs to be configured in an array of dt-bindings indices
> 4. move PLL configuration to really_probe
> 5. move RPM enablement to really_probe, make it conditional to limit resource
>    waste on e.g. gcc
> 6. move attaching GDSCs to really_probe
> 
> Konrad

Thanks Konrad for your feedback and suggestion.

I will check and work on this approach of moving PLL configure and RPM enablement
to a common code.

Thanks,
Jagadeesh