[PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Bryan O'Donoghue]

We have both shared_ops for the Titan Top GDSC and a hard-coded always on
whack the register and forget about it in probe().

@static struct clk_branch camcc_gdsc_clk = {}

Only one representation of the Top GDSC is required. Use the CCF
representation not the hard-coded register write.

Fixes: ff93872a9c61 ("clk: qcom: camcc-sc8280xp: Add sc8280xp CAMCC")
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # Lenovo X13s
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/clk/qcom/camcc-sc8280xp.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/camcc-sc8280xp.c b/drivers/clk/qcom/camcc-sc8280xp.c
index 479964f91608..f99cd968459c 100644
--- a/drivers/clk/qcom/camcc-sc8280xp.c
+++ b/drivers/clk/qcom/camcc-sc8280xp.c
@@ -3031,19 +3031,14 @@ static int camcc_sc8280xp_probe(struct platform_device *pdev)
 	clk_lucid_pll_configure(&camcc_pll6, regmap, &camcc_pll6_config);
 	clk_lucid_pll_configure(&camcc_pll7, regmap, &camcc_pll7_config);
 
-	/* Keep some clocks always-on */
-	qcom_branch_set_clk_en(regmap, 0xc1e4); /* CAMCC_GDSC_CLK */
-
 	ret = qcom_cc_really_probe(&pdev->dev, &camcc_sc8280xp_desc, regmap);
 	if (ret)
-		goto err_disable;
+		goto err_put_rpm;
 
 	pm_runtime_put(&pdev->dev);
 
 	return 0;
 
-err_disable:
-	regmap_update_bits(regmap, 0xc1e4, BIT(0), 0);
 err_put_rpm:
 	pm_runtime_put_sync(&pdev->dev);
 

---
base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
change-id: 20240715-linux-next-24-07-13-sc8280xp-camcc-fixes-274f11b396ac

Best regards,
-- 
Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Satya Priya Kakitapalli (Temp)]

On 7/15/2024 8:29 PM, Bryan O'Donoghue wrote:
> We have both shared_ops for the Titan Top GDSC and a hard-coded always on
> whack the register and forget about it in probe().
>
> @static struct clk_branch camcc_gdsc_clk = {}
>
> Only one representation of the Top GDSC is required. Use the CCF
> representation not the hard-coded register write.
>
> Fixes: ff93872a9c61 ("clk: qcom: camcc-sc8280xp: Add sc8280xp CAMCC")
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # Lenovo X13s
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   drivers/clk/qcom/camcc-sc8280xp.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/clk/qcom/camcc-sc8280xp.c b/drivers/clk/qcom/camcc-sc8280xp.c
> index 479964f91608..f99cd968459c 100644
> --- a/drivers/clk/qcom/camcc-sc8280xp.c
> +++ b/drivers/clk/qcom/camcc-sc8280xp.c
> @@ -3031,19 +3031,14 @@ static int camcc_sc8280xp_probe(struct platform_device *pdev)
>   	clk_lucid_pll_configure(&camcc_pll6, regmap, &camcc_pll6_config);
>   	clk_lucid_pll_configure(&camcc_pll7, regmap, &camcc_pll7_config);
>   
> -	/* Keep some clocks always-on */
> -	qcom_branch_set_clk_en(regmap, 0xc1e4); /* CAMCC_GDSC_CLK */


As I mentioned on [1], this change might break the GDSC functionality. 
Hence this shouldn't be removed.


[1] 
https://lore.kernel.org/linux-clk/0b84b689-8ab8-bcdf-f058-da2ead73786c@quicinc.com/


> -
>   	ret = qcom_cc_really_probe(&pdev->dev, &camcc_sc8280xp_desc, regmap);
>   	if (ret)
> -		goto err_disable;
> +		goto err_put_rpm;
>   
>   	pm_runtime_put(&pdev->dev);
>   
>   	return 0;
>   
> -err_disable:
> -	regmap_update_bits(regmap, 0xc1e4, BIT(0), 0);


This change is required, hence can go as a separate patch.


>   err_put_rpm:
>   	pm_runtime_put_sync(&pdev->dev);
>   
>
> ---
> base-commit: 3fe121b622825ff8cc995a1e6b026181c48188db
> change-id: 20240715-linux-next-24-07-13-sc8280xp-camcc-fixes-274f11b396ac
>
> Best regards,

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Bryan O'Donoghue]

On 17/07/2024 07:32, Satya Priya Kakitapalli (Temp) wrote:
> 
> On 7/15/2024 8:29 PM, Bryan O'Donoghue wrote:
>> We have both shared_ops for the Titan Top GDSC and a hard-coded always on
>> whack the register and forget about it in probe().
>>
>> @static struct clk_branch camcc_gdsc_clk = {}
>>
>> Only one representation of the Top GDSC is required. Use the CCF
>> representation not the hard-coded register write.
>>
>> Fixes: ff93872a9c61 ("clk: qcom: camcc-sc8280xp: Add sc8280xp CAMCC")
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # Lenovo X13s
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   drivers/clk/qcom/camcc-sc8280xp.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/camcc-sc8280xp.c 
>> b/drivers/clk/qcom/camcc-sc8280xp.c
>> index 479964f91608..f99cd968459c 100644
>> --- a/drivers/clk/qcom/camcc-sc8280xp.c
>> +++ b/drivers/clk/qcom/camcc-sc8280xp.c
>> @@ -3031,19 +3031,14 @@ static int camcc_sc8280xp_probe(struct 
>> platform_device *pdev)
>>       clk_lucid_pll_configure(&camcc_pll6, regmap, &camcc_pll6_config);
>>       clk_lucid_pll_configure(&camcc_pll7, regmap, &camcc_pll7_config);
>> -    /* Keep some clocks always-on */
>> -    qcom_branch_set_clk_en(regmap, 0xc1e4); /* CAMCC_GDSC_CLK */
> 
> 
> As I mentioned on [1], this change might break the GDSC functionality. 
> Hence this shouldn't be removed.

How would it break ?

We park the clock to XO it never gets turned off this way.

---
bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Satya Priya Kakitapalli (Temp)]

On 7/17/2024 2:19 PM, Bryan O'Donoghue wrote:
> On 17/07/2024 07:32, Satya Priya Kakitapalli (Temp) wrote:
>>
>> On 7/15/2024 8:29 PM, Bryan O'Donoghue wrote:
>>> We have both shared_ops for the Titan Top GDSC and a hard-coded 
>>> always on
>>> whack the register and forget about it in probe().
>>>
>>> @static struct clk_branch camcc_gdsc_clk = {}
>>>
>>> Only one representation of the Top GDSC is required. Use the CCF
>>> representation not the hard-coded register write.
>>>
>>> Fixes: ff93872a9c61 ("clk: qcom: camcc-sc8280xp: Add sc8280xp CAMCC")
>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # Lenovo X13s
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> ---
>>>   drivers/clk/qcom/camcc-sc8280xp.c | 7 +------
>>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/camcc-sc8280xp.c 
>>> b/drivers/clk/qcom/camcc-sc8280xp.c
>>> index 479964f91608..f99cd968459c 100644
>>> --- a/drivers/clk/qcom/camcc-sc8280xp.c
>>> +++ b/drivers/clk/qcom/camcc-sc8280xp.c
>>> @@ -3031,19 +3031,14 @@ static int camcc_sc8280xp_probe(struct 
>>> platform_device *pdev)
>>>       clk_lucid_pll_configure(&camcc_pll6, regmap, &camcc_pll6_config);
>>>       clk_lucid_pll_configure(&camcc_pll7, regmap, &camcc_pll7_config);
>>> -    /* Keep some clocks always-on */
>>> -    qcom_branch_set_clk_en(regmap, 0xc1e4); /* CAMCC_GDSC_CLK */
>>
>>
>> As I mentioned on [1], this change might break the GDSC 
>> functionality. Hence this shouldn't be removed.
>
> How would it break ?
>
> We park the clock to XO it never gets turned off this way.
>

Parking the parent at XO doesn't ensure the branch clock is always on, 
it can be disabled by consumers or CCF if modelled.

If the CCF disables this clock in late init, then the clock stays in 
disabled state until it is enabled again explicitly. Hence it is 
recommended to not model such always-on clocks.


> ---
> bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Bryan O'Donoghue]

On 17/07/2024 12:08, Satya Priya Kakitapalli (Temp) wrote:
>> How would it break ?
>>
>> We park the clock to XO it never gets turned off this way.
>>
> 
> Parking the parent at XO doesn't ensure the branch clock is always on, 
> it can be disabled by consumers or CCF if modelled.
> 
> If the CCF disables this clock in late init, then the clock stays in 
> disabled state until it is enabled again explicitly. Hence it is 
> recommended to not model such always-on clocks.

What is the use-case to keep that clock always-on unless/util someone 
wants camss ?

I've tested this patch on sc8280xp and it works just fine.

---
bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Satya Priya Kakitapalli (Temp)]

On 7/17/2024 4:41 PM, Bryan O'Donoghue wrote:
> On 17/07/2024 12:08, Satya Priya Kakitapalli (Temp) wrote:
>>> How would it break ?
>>>
>>> We park the clock to XO it never gets turned off this way.
>>>
>>
>> Parking the parent at XO doesn't ensure the branch clock is always 
>> on, it can be disabled by consumers or CCF if modelled.
>>
>> If the CCF disables this clock in late init, then the clock stays in 
>> disabled state until it is enabled again explicitly. Hence it is 
>> recommended to not model such always-on clocks.
>
> What is the use-case to keep that clock always-on unless/util someone 
> wants camss ?
>

The clock also has dependency on MMCX rail, this rail anyway will be OFF 
until there is a use-case. So the clock will also be OFF.


> I've tested this patch on sc8280xp and it works just fine.
>

Is the cam_cc_gdsc_clk clock ON after the boot up?


> ---
> bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Bryan O'Donoghue]

On 19/07/2024 08:25, Satya Priya Kakitapalli (Temp) wrote:
>>
>> What is the use-case to keep that clock always-on unless/util someone 
>> wants camss ?
>>
> 
> The clock also has dependency on MMCX rail, this rail anyway will be OFF 
> until there is a use-case. So the clock will also be OFF.

arch/arm64/boot/dts/qcom/sc8280xp.dtsi

camcc: clock-controller@ad00000 {
     power-domains = <&rpmhpd SC8280XP_MMCX>;
};

> 
> 
>> I've tested this patch on sc8280xp and it works just fine.
>>
> 
> Is the cam_cc_gdsc_clk clock ON after the boot up?

I have no idea. Why does it matter ?

---
bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Satya Priya Kakitapalli (Temp)]

On 7/20/2024 2:03 PM, Bryan O'Donoghue wrote:
> On 19/07/2024 08:25, Satya Priya Kakitapalli (Temp) wrote:
>>>
>>> What is the use-case to keep that clock always-on unless/util 
>>> someone wants camss ?
>>>
>>
>> The clock also has dependency on MMCX rail, this rail anyway will be 
>> OFF until there is a use-case. So the clock will also be OFF.
>
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>
> camcc: clock-controller@ad00000 {
>     power-domains = <&rpmhpd SC8280XP_MMCX>;
> };
>
>>
>>
>>> I've tested this patch on sc8280xp and it works just fine.
>>>
>>
>> Is the cam_cc_gdsc_clk clock ON after the boot up?
>
> I have no idea. Why does it matter ?
>

This clock expected to be kept always ON, as per design, or else the 
GDSC transition form ON to OFF (vice versa) wont work.

Want to know the clock status after bootup, to understand if the clock 
got turned off during the late init. May I know exactly what you have 
tested? Did you test the camera usecases as well?


> ---
> bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Bryan O'Donoghue]

On 22/07/2024 09:57, Satya Priya Kakitapalli (Temp) wrote:
>> I have no idea. Why does it matter ?
>>
> 
> This clock expected to be kept always ON, as per design, or else the 
> GDSC transition form ON to OFF (vice versa) wont work.

Yes, parking to XO per this patch works for me. So I guess its already 
on and is left in that state by the park.

> Want to know the clock status after bootup, to understand if the clock 
> got turned off during the late init. May I know exactly what you have 
> tested? Did you test the camera usecases as well?

Of course.

The camera works on x13s with this patch. That's what I mean by tested.

---
bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Satya Priya Kakitapalli (Temp)]

On 7/23/2024 2:59 PM, Bryan O'Donoghue wrote:
> On 22/07/2024 09:57, Satya Priya Kakitapalli (Temp) wrote:
>>> I have no idea. Why does it matter ?
>>>
>>
>> This clock expected to be kept always ON, as per design, or else the 
>> GDSC transition form ON to OFF (vice versa) wont work.
>
> Yes, parking to XO per this patch works for me. So I guess its already 
> on and is left in that state by the park.
>

Parking RCG to XO doesn't keep the branch clock always-on. It just keeps 
the parent RCG at 19.2MHz, branch can still be disabled by clearing 
bit(0). So during late init, the CCF will disable this clock(in 
clk_disable_unused API) if modelled. Hence this clock shouldn't be modelled.


>> Want to know the clock status after bootup, to understand if the 
>> clock got turned off during the late init. May I know exactly what 
>> you have tested? Did you test the camera usecases as well?
>
> Of course.
>
> The camera works on x13s with this patch. That's what I mean by tested.
>
> ---
> bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Dmitry Baryshkov]

On Tue, 23 Jul 2024 at 14:37, Satya Priya Kakitapalli (Temp)
<quic_skakitap@quicinc.com> wrote:
>
>
> On 7/23/2024 2:59 PM, Bryan O'Donoghue wrote:
> > On 22/07/2024 09:57, Satya Priya Kakitapalli (Temp) wrote:
> >>> I have no idea. Why does it matter ?
> >>>
> >>
> >> This clock expected to be kept always ON, as per design, or else the
> >> GDSC transition form ON to OFF (vice versa) wont work.
> >
> > Yes, parking to XO per this patch works for me. So I guess its already
> > on and is left in that state by the park.
> >
>
> Parking RCG to XO doesn't keep the branch clock always-on. It just keeps
> the parent RCG at 19.2MHz, branch can still be disabled by clearing
> bit(0). So during late init, the CCF will disable this clock(in
> clk_disable_unused API) if modelled. Hence this clock shouldn't be modelled.

But it is already modelled:

static struct clk_branch camcc_gdsc_clk = {
        .halt_reg = 0xc1e4,
        .halt_check = BRANCH_HALT,
....
};

>
>
> >> Want to know the clock status after bootup, to understand if the
> >> clock got turned off during the late init. May I know exactly what
> >> you have tested? Did you test the camera usecases as well?
> >
> > Of course.
> >
> > The camera works on x13s with this patch. That's what I mean by tested.
> >
> > ---
> > bod



-- 
With best wishes
Dmitry

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Satya Priya Kakitapalli (Temp)]

On 7/23/2024 2:59 PM, Bryan O'Donoghue wrote:
> On 22/07/2024 09:57, Satya Priya Kakitapalli (Temp) wrote:
>>> I have no idea. Why does it matter ?
>>>
>>
>> This clock expected to be kept always ON, as per design, or else the 
>> GDSC transition form ON to OFF (vice versa) wont work.
>
> Yes, parking to XO per this patch works for me. So I guess its already 
> on and is left in that state by the park.
>
>> Want to know the clock status after bootup, to understand if the 
>> clock got turned off during the late init. May I know exactly what 
>> you have tested? Did you test the camera usecases as well?
>
> Of course.
>
> The camera works on x13s with this patch. That's what I mean by tested.
>

It might be working in your case, but it is not the HW design 
recommended way to do. The same should not be propagated to other 
target's camcc drivers, as I already observed it is not working on SM8150.


> ---
> bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Bryan O'Donoghue]

On 26/07/2024 08:01, Satya Priya Kakitapalli (Temp) wrote:
> 
> On 7/23/2024 2:59 PM, Bryan O'Donoghue wrote:
>> On 22/07/2024 09:57, Satya Priya Kakitapalli (Temp) wrote:
>>>> I have no idea. Why does it matter ?
>>>>
>>>
>>> This clock expected to be kept always ON, as per design, or else the 
>>> GDSC transition form ON to OFF (vice versa) wont work.
>>
>> Yes, parking to XO per this patch works for me. So I guess its already 
>> on and is left in that state by the park.
>>
>>> Want to know the clock status after bootup, to understand if the 
>>> clock got turned off during the late init. May I know exactly what 
>>> you have tested? Did you test the camera usecases as well?
>>
>> Of course.
>>
>> The camera works on x13s with this patch. That's what I mean by tested.
>>
> 
> It might be working in your case, but it is not the HW design 
> recommended way to do. The same should not be propagated to other 
> target's camcc drivers, as I already observed it is not working on SM8150.

I don't think the argument here really stands up.

We've established that the GDSC clock and PDs will remain on when the 
clock gets parked right ?

Am I missing something obvious here ?

---
bod

Re: [PATCH] clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding

[Satya Priya Kakitapalli (Temp)]

On 7/27/2024 2:01 AM, Bryan O'Donoghue wrote:
> On 26/07/2024 08:01, Satya Priya Kakitapalli (Temp) wrote:
>>
>> On 7/23/2024 2:59 PM, Bryan O'Donoghue wrote:
>>> On 22/07/2024 09:57, Satya Priya Kakitapalli (Temp) wrote:
>>>>> I have no idea. Why does it matter ?
>>>>>
>>>>
>>>> This clock expected to be kept always ON, as per design, or else 
>>>> the GDSC transition form ON to OFF (vice versa) wont work.
>>>
>>> Yes, parking to XO per this patch works for me. So I guess its 
>>> already on and is left in that state by the park.
>>>
>>>> Want to know the clock status after bootup, to understand if the 
>>>> clock got turned off during the late init. May I know exactly what 
>>>> you have tested? Did you test the camera usecases as well?
>>>
>>> Of course.
>>>
>>> The camera works on x13s with this patch. That's what I mean by tested.
>>>
>>
>> It might be working in your case, but it is not the HW design 
>> recommended way to do. The same should not be propagated to other 
>> target's camcc drivers, as I already observed it is not working on 
>> SM8150.
>
> I don't think the argument here really stands up.
>
> We've established that the GDSC clock and PDs will remain on when the 
> clock gets parked right ?
>
> Am I missing something obvious here ?
>

Yes, just parking the RCG at XO, does not ensure the branch clock, i.e, 
the 'cam_cc_gdsc_clk' as always ON. When I compiled the camcc-sm8150 
driver statically, I see that the clock is getting disabled in late_init 
if it is modelled.


Can you confirm if the camcc-sc8280xp driver is compiled statically or 
as a module at your end?


> ---
> bod
>