| Message ID | 20240715-linux-next-24-07-13-sc8280xp-camcc-fixes-v1-1-fadb5d9445c1@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: qcom: camcc-sc8280xp: Remove always-on GDSC hard-coding | expand |
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,
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
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
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
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
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
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
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
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
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
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
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
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 >
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);
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,