diff mbox series

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

Message ID 20240715-linux-next-24-07-13-sc8280xp-camcc-fixes-v1-1-fadb5d9445c1@linaro.org
State New
Headers show
Series clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding | expand

Commit Message

Bryan O'Donoghue July 15, 2024, 2:59 p.m. UTC 
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(-)


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

Best regards,

Comments

Satya Priya Kakitapalli July 19, 2024, 7:25 a.m. UTC | #1 
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
Bryan O'Donoghue July 20, 2024, 8:33 a.m. UTC | #2 
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
Satya Priya Kakitapalli July 22, 2024, 8:57 a.m. UTC | #3 
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
Bryan O'Donoghue July 23, 2024, 9:29 a.m. UTC | #4 
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
Satya Priya Kakitapalli July 23, 2024, 11:37 a.m. UTC | #5 
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
Dmitry Baryshkov July 23, 2024, 11:49 a.m. UTC | #6 
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
Satya Priya Kakitapalli July 26, 2024, 7:01 a.m. UTC | #7 
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
Bryan O'Donoghue July 26, 2024, 8:31 p.m. UTC | #8 
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
diff mbox series

Patch

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);