Skip to contentSkip to conversation

clk: qcom: gcc: Update disp gpll0 branch for 7180/845

PATCH In review Taniya Das 3 participants 4 messages Jul 15, 2020 – Jul 23, 2020

Subsystems (3)
Arm Qualcomm mailing list Common clk Qualcomm clock drivers
lore
Share & tools
lore

Taniya Das

author
#0 lore raw

PATCH 1/1 clk: qcom: gcc: Update disp gpll0 branch for 7180/845

The display gpll0 branch clock needs to be always left enabled, thus move the clock ops to _aon branch ops.

Signed-off-by Taniya Das <[email protected]>
---
 drivers/clk/qcom/gcc-sc7180.c | 2 +-
 drivers/clk/qcom/gcc-sdm845.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/clk/qcom/gcc-sc7180.c +1 -1
index ca4383e..538677b 100644
@@ -1061,7 +1061,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
 				.hw = &gpll0.clkr.hw,
 			},
 			.num_parents = 1,
-			.ops = &clk_branch2_ops,
+			.ops = &clk_branch2_aon_ops,
 		},
 	},
 };
drivers/clk/qcom/gcc-sdm845.c +2 -2
index f6ce888..90f7feb 100644
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, 2020, The Linux Foundation. All rights reserved.
  */
 
 #include <linux/kernel.h>

@@ -1344,7 +1344,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
 				"gpll0",
 			},
 			.num_parents = 1,
-			.ops = &clk_branch2_ops,
+			.ops = &clk_branch2_aon_ops,
 		},
 	},
 };

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

maintainer
#1 lore raw

Quoting Taniya Das (2020-07-14 23:54:10)

Quoted 4 lines

The display gpll0 branch clock needs to be always left enabled, thus move the clock ops to _aon branch ops.

Signed-off-by Taniya Das <[email protected]>

I wanted to apply this but it needs more commit text. Here's the commit text I wrote.

clk: qcom: gcc: Make disp gpll0 branch aon for sc7180/sdm845

The display gpll0 branch clock inside GCC needs to always be enabled. Otherwise the AHB clk (disp_cc_mdss_ahb_clk_src) for the display clk controller (dispcc) will stop clocking while sourcing from gpll0 when this branch inside GCC is turned off during unused clk disabling. We can never turn this branch off because the AHB clk for the display subsystem is needed to read/write any registers inside the display subsystem including clk related ones. This makes this branch a really easy way to turn off AHB access to the display subsystem and cause all sorts of mayhem. Let's just make the clk ops keep the clk enabled forever and ignore any attempts to disable this clk so that dispcc access keep working.

Signed-off-by Taniya Das <[email protected]>
Reported-by Evan Green <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Fixes 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
Fixes 06391eddb60a ("clk: qcom: Add Global Clock controller (GCC) driver for SDM845")
Quoted 10 lines
---
 drivers/clk/qcom/gcc-sc7180.c | 2 +-
 drivers/clk/qcom/gcc-sdm845.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/clk/qcom/gcc-sc7180.c +0 -0
index ca4383e..538677b 100644

drivers/clk/qcom/gcc-sc7180.c static struct clk_branch gcc_disp_gpll0_clk_src = { · line 1064

Quoted 5 lines
drivers/clk/qcom/gcc-sc7180.c +1 -1
@@ -1061,7 +1061,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
                                .hw = &gpll0.clkr.hw,
                        },
                        .num_parents = 1,
-                       .ops = &clk_branch2_ops,
+                       .ops = &clk_branch2_aon_ops,

I'm worried that dispcc may probe before GCC and then this branch is disabled out of boot. Then we'll try to read dispcc registers to determine clk rates and blow up. I hope that doesn't happen and fixing it would need this patch plus the usage of PM clks in the dispcc driver. I suppose if we run into that problem we can use PM clks to make this branch turn on before accessing any dispcc clks.

#2 lore raw
In reply to Taniya Das
Quoted 2 lines · Taniya Das

The display gpll0 branch clock needs to be always left enabled, thus move the clock ops to _aon branch ops.

Does this also apply to sm8250/sm8150?

Quoted 44 lines
Signed-off-by Taniya Das <[email protected]>
---
  drivers/clk/qcom/gcc-sc7180.c | 2 +-
  drivers/clk/qcom/gcc-sdm845.c | 4 ++--
  2 files changed, 3 insertions(+), 3 deletions(-)
drivers/clk/qcom/gcc-sc7180.c +1 -1
index ca4383e..538677b 100644
@@ -1061,7 +1061,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
  				.hw = &gpll0.clkr.hw,
  			},
  			.num_parents = 1,
-			.ops = &clk_branch2_ops,
+			.ops = &clk_branch2_aon_ops,
  		},
  	},
  };
drivers/clk/qcom/gcc-sdm845.c +2 -3
index f6ce888..90f7feb 100644
@@ -1,6 +1,6 @@
  // SPDX-License-Identifier: GPL-2.0
  /*
- * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, 2020, The Linux Foundation. All rights reserved.
   */
 
  #include <linux/kernel.h>

@@ -1344,7 +1344,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
  				"gpll0",
  			},
  			.num_parents = 1,
-			.ops = &clk_branch2_ops,
+			.ops = &clk_branch2_aon_ops,
  		},
  	},
  };
--

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Signature · 2 lines
With best wishes
Dmitry
maintainer
#3 lore raw

Quoting Dmitry Baryshkov (2020-07-21 07:31:56)

Quoted 6 lines · Taniya Das
Quoted 2 lines

The display gpll0 branch clock needs to be always left enabled, thus move the clock ops to _aon branch ops.

Does this also apply to sm8250/sm8150?

Probably.