[PATCH 0/2] 8280 display ahb clk fixes

[PATCH 0/2] 8280 display ahb clk fixes

[Konrad Dybcio]

Little fixes to shut down disp ahb clk when dispss is not in use.

If possible, the dt patch should theoretically go in first, but x13s
worked fine regardless.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (2):
      arm64: dts: qcom: sc8280xp: Use the correct AHB clock for DISPSS1
      clk: qcom: gcc-sc8280xp: Don't keep display AHB clocks always-on

 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 4 ++--
 drivers/clk/qcom/gcc-sc8280xp.c        | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)
---
base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
change-id: 20230929-topic-8280_ahbdisp-e170e0ca2442

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/2] arm64: dts: qcom: sc8280xp: Use the correct AHB clock for DISPSS1

[Konrad Dybcio]

SC8280XP has two instances of the display subsystem. Make sure it uses
the correct clocks.

Fixes: 57d6ef683a15 ("arm64: dts: qcom: sc8280xp: Define some of the display blocks")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index cad59af7ccef..92bcbb1770ea 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -4562,7 +4562,7 @@ mdss1: display-subsystem@22000000 {
 			reg = <0 0x22000000 0 0x1000>;
 			reg-names = "mdss";
 
-			clocks = <&gcc GCC_DISP_AHB_CLK>,
+			clocks = <&gcc GCC_DISP1_AHB_CLK>,
 				 <&dispcc1 DISP_CC_MDSS_AHB_CLK>,
 				 <&dispcc1 DISP_CC_MDSS_MDP_CLK>;
 			clock-names = "iface",
@@ -5002,7 +5002,7 @@ dispcc1: clock-controller@22100000 {
 			compatible = "qcom,sc8280xp-dispcc1";
 			reg = <0 0x22100000 0 0x20000>;
 
-			clocks = <&gcc GCC_DISP_AHB_CLK>,
+			clocks = <&gcc GCC_DISP1_AHB_CLK>,
 				 <&rpmhcc RPMH_CXO_CLK>,
 				 <0>,
 				 <&mdss1_dp0_phy 0>,

-- 
2.39.2

[PATCH 2/2] clk: qcom: gcc-sc8280xp: Don't keep display AHB clocks always-on

[Konrad Dybcio]

These clocks are consumed by the dispcc[01] clock controllers, so there's
no reason to keep them on from gcc probe. Remove that hack.

Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sc8280xp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
index bfb77931e868..bf95f82a3818 100644
--- a/drivers/clk/qcom/gcc-sc8280xp.c
+++ b/drivers/clk/qcom/gcc-sc8280xp.c
@@ -7545,18 +7545,16 @@ static int gcc_sc8280xp_probe(struct platform_device *pdev)
 
 	/*
 	 * Keep the clocks always-ON
-	 * GCC_CAMERA_AHB_CLK, GCC_CAMERA_XO_CLK, GCC_DISP_AHB_CLK,
+	 * GCC_CAMERA_AHB_CLK, GCC_CAMERA_XO_CLK,
 	 * GCC_DISP_XO_CLK, GCC_GPU_CFG_AHB_CLK, GCC_VIDEO_AHB_CLK,
-	 * GCC_VIDEO_XO_CLK, GCC_DISP1_AHB_CLK, GCC_DISP1_XO_CLK
+	 * GCC_VIDEO_XO_CLK, GCC_DISP1_XO_CLK
 	 */
 	regmap_update_bits(regmap, 0x26004, BIT(0), BIT(0));
 	regmap_update_bits(regmap, 0x26020, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x27004, BIT(0), BIT(0));
 	regmap_update_bits(regmap, 0x27028, BIT(0), BIT(0));
 	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
 	regmap_update_bits(regmap, 0x28004, BIT(0), BIT(0));
 	regmap_update_bits(regmap, 0x28028, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xbb004, BIT(0), BIT(0));
 	regmap_update_bits(regmap, 0xbb028, BIT(0), BIT(0));
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));

-- 
2.39.2

Re: [PATCH 2/2] clk: qcom: gcc-sc8280xp: Don't keep display AHB clocks always-on

[Johan Hovold]

On Fri, Sep 29, 2023 at 03:38:53PM +0200, Konrad Dybcio wrote:
> These clocks are consumed by the dispcc[01] clock controllers, so there's
> no reason to keep them on from gcc probe. Remove that hack.

Eh, how did you test this patch?

The GCC_DISP_AHB_CLK clocks are not modelled by the clock driver
currently so nothing is guaranteeing them to be enabled if we were to
apply this patch. They just happen to be left on by the bootloader on
some machines currently (well at least one of them is on one machine).

So this series is broken and should not be applied.

NAK.

Also, please CC me on sc8280xp and X13s related patches.
 
> Fixes: d65d005f9a6c ("clk: qcom: add sc8280xp GCC driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/clk/qcom/gcc-sc8280xp.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> index bfb77931e868..bf95f82a3818 100644
> --- a/drivers/clk/qcom/gcc-sc8280xp.c
> +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> @@ -7545,18 +7545,16 @@ static int gcc_sc8280xp_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * Keep the clocks always-ON
> -	 * GCC_CAMERA_AHB_CLK, GCC_CAMERA_XO_CLK, GCC_DISP_AHB_CLK,
> +	 * GCC_CAMERA_AHB_CLK, GCC_CAMERA_XO_CLK,
>  	 * GCC_DISP_XO_CLK, GCC_GPU_CFG_AHB_CLK, GCC_VIDEO_AHB_CLK,
> -	 * GCC_VIDEO_XO_CLK, GCC_DISP1_AHB_CLK, GCC_DISP1_XO_CLK
> +	 * GCC_VIDEO_XO_CLK, GCC_DISP1_XO_CLK
>  	 */
>  	regmap_update_bits(regmap, 0x26004, BIT(0), BIT(0));
>  	regmap_update_bits(regmap, 0x26020, BIT(0), BIT(0));
> -	regmap_update_bits(regmap, 0x27004, BIT(0), BIT(0));
>  	regmap_update_bits(regmap, 0x27028, BIT(0), BIT(0));
>  	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
>  	regmap_update_bits(regmap, 0x28004, BIT(0), BIT(0));
>  	regmap_update_bits(regmap, 0x28028, BIT(0), BIT(0));
> -	regmap_update_bits(regmap, 0xbb004, BIT(0), BIT(0));
>  	regmap_update_bits(regmap, 0xbb028, BIT(0), BIT(0));

Johan

Re: [PATCH 2/2] clk: qcom: gcc-sc8280xp: Don't keep display AHB clocks always-on

[Konrad Dybcio]

On 9/30/23 11:39, Johan Hovold wrote:
> On Fri, Sep 29, 2023 at 03:38:53PM +0200, Konrad Dybcio wrote:
>> These clocks are consumed by the dispcc[01] clock controllers, so there's
>> no reason to keep them on from gcc probe. Remove that hack.
> 
> Eh, how did you test this patch?
Oehh you're right, I didn't notice that I still had clk_ignore_unused :/

> 
> The GCC_DISP_AHB_CLK clocks are not modelled by the clock driver
> currently so nothing is guaranteeing them to be enabled if we were to
> apply this patch. They just happen to be left on by the bootloader on
> some machines currently (well at least one of them is on one machine).
What fooled me is that despite not being modeled by the clock driver, it 
is defined in bindings and referenced in the device tree.

Another thing I'll fix up!

> So this series is broken and should not be applied.
Agreed, I'll revisit this tonight or next week.

> 
> NAK.
> 
> Also, please CC me on sc8280xp and X13s related patches.
Omitting you was not intentional, sorry!

Konrad

Re: [PATCH 2/2] clk: qcom: gcc-sc8280xp: Don't keep display AHB clocks always-on

[Johan Hovold]

On Sat, Sep 30, 2023 at 06:44:47PM +0200, Konrad Dybcio wrote:
> On 9/30/23 11:39, Johan Hovold wrote:
> > On Fri, Sep 29, 2023 at 03:38:53PM +0200, Konrad Dybcio wrote:
> >> These clocks are consumed by the dispcc[01] clock controllers, so there's
> >> no reason to keep them on from gcc probe. Remove that hack.
> > 
> > Eh, how did you test this patch?

> Oehh you're right, I didn't notice that I still had clk_ignore_unused :/

That doesn't matter since these clocks are never even registered with
the clock framework.

But you'd notice that if you try to verify the clock state by looking at
/sys/kernel/debug/clk/clk_summary for example.

> > The GCC_DISP_AHB_CLK clocks are not modelled by the clock driver
> > currently so nothing is guaranteeing them to be enabled if we were to
> > apply this patch. They just happen to be left on by the bootloader on
> > some machines currently (well at least one of them is on one machine).

> What fooled me is that despite not being modeled by the clock driver, it 
> is defined in bindings and referenced in the device tree.
> 
> Another thing I'll fix up!

Right, a number of Qualcomm SoCs apparently fail to register these
clocks. You should start by determining why that is as I assume (hope)
it was done for a reason.

Then the Qualcomm drivers use sloppy bulk clock look-up and enable so
that an integrator would never even notice when clocks are missing. Once
the clocks are registered, that could be tightened up as well.

Johan

Re: [PATCH 2/2] clk: qcom: gcc-sc8280xp: Don't keep display AHB clocks always-on

[Konrad Dybcio]

On 10/2/23 10:34, Johan Hovold wrote:
> On Sat, Sep 30, 2023 at 06:44:47PM +0200, Konrad Dybcio wrote:
>> On 9/30/23 11:39, Johan Hovold wrote:
>>> On Fri, Sep 29, 2023 at 03:38:53PM +0200, Konrad Dybcio wrote:
>>>> These clocks are consumed by the dispcc[01] clock controllers, so there's
>>>> no reason to keep them on from gcc probe. Remove that hack.
>>>
>>> Eh, how did you test this patch?
> 
>> Oehh you're right, I didn't notice that I still had clk_ignore_unused :/
> 
> That doesn't matter since these clocks are never even registered with
> the clock framework.
That's the point, if it was missing and was not enabled I would have 
noticed display not working (unless the bootloader left it on, which it 
did for at least the mdss instance with the eDP panel)

> 
> But you'd notice that if you try to verify the clock state by looking at
> /sys/kernel/debug/clk/clk_summary for example.
> 
>>> The GCC_DISP_AHB_CLK clocks are not modelled by the clock driver
>>> currently so nothing is guaranteeing them to be enabled if we were to
>>> apply this patch. They just happen to be left on by the bootloader on
>>> some machines currently (well at least one of them is on one machine).
> 
>> What fooled me is that despite not being modeled by the clock driver, it
>> is defined in bindings and referenced in the device tree.
>>
>> Another thing I'll fix up!
> 
> Right, a number of Qualcomm SoCs apparently fail to register these
> clocks. You should start by determining why that is as I assume (hope)
> it was done for a reason.
The reason is, downstream lazily enables clocks because people decided 
they don't leak much power and are still enabled after a clock 
controller reset (or something) and we started blindly copying that 
around 2017 :/

> 
> Then the Qualcomm drivers use sloppy bulk clock look-up and enable so
> that an integrator would never even notice when clocks are missing. Once
> the clocks are registered, that could be tightened up as well.
Yeah the current state of things is not great.

Konrad