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

[Jagadeesh Kona <quic_jkona@quicinc.com>]

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@quicinc.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