[PATCH 00/15] Unregister critical branch clocks + some RPM

[PATCH 00/15] Unregister critical branch clocks + some RPM

[Konrad Dybcio]

On Qualcomm SoCs, certain branch clocks either need to be always-on, or
should be if you're interested in touching some part of the hardware.

Using CLK_IS_CRITICAL for this purpose sounds like a genius idea,
however that messes with the runtime pm handling - if a clock is
marked as such, the clock controller device will never enter the
"suspended" state, leaving the associated resources online, which in
turn breaks SoC-wide suspend.

This series aims to solve that on a couple SoCs that I could test the
changes on and it sprinkles some runtime pm enablement atop these drivers.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (15):
      clk: qcom: branch: Add a helper for setting the enable bit
      clk: qcom: Use qcom_branch_set_clk_en()
      clk: qcom: gcc-sm6375: Unregister critical clocks
      clk: qcom: gcc-sm6375: Add runtime PM
      clk: qcom: gpucc-sm6375: Unregister critical clocks
      clk: qcom: gpucc-sm6115: Unregister critical clocks
      clk: qcom: gpucc-sm6115: Add runtime PM
      clk: qcom: gcc-sm6115: Unregister critical clocks
      clk: qcom: gcc-sm6115: Add runtime PM
      clk: qcom: gcc-qcm2290: Unregister critical clocks
      clk: qcom: gcc-qcm2290: Add runtime PM
      arm64: dts: qcom: sm6375: Add VDD_CX to GCC
      arm64: dts: qcom: qcm2290: Add VDD_CX to GCC
      arm64: dts: qcom: sm6115: Add VDD_CX to GCC
      arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

 arch/arm64/boot/dts/qcom/qcm2290.dtsi |   1 +
 arch/arm64/boot/dts/qcom/sm6115.dtsi  |   3 +
 arch/arm64/boot/dts/qcom/sm6375.dtsi  |   1 +
 drivers/clk/qcom/clk-branch.h         |   7 ++
 drivers/clk/qcom/dispcc-qcm2290.c     |   2 +-
 drivers/clk/qcom/dispcc-sc7280.c      |   2 +-
 drivers/clk/qcom/dispcc-sc8280xp.c    |   2 +-
 drivers/clk/qcom/dispcc-sm6115.c      |   2 +-
 drivers/clk/qcom/dispcc-sm8250.c      |   2 +-
 drivers/clk/qcom/dispcc-sm8450.c      |   2 +-
 drivers/clk/qcom/dispcc-sm8550.c      |   2 +-
 drivers/clk/qcom/gcc-qcm2290.c        | 136 ++++++++---------------------
 drivers/clk/qcom/gcc-sa8775p.c        |  18 ++--
 drivers/clk/qcom/gcc-sc7180.c         |  16 ++--
 drivers/clk/qcom/gcc-sc7280.c         |  14 +--
 drivers/clk/qcom/gcc-sc8180x.c        |  20 ++---
 drivers/clk/qcom/gcc-sc8280xp.c       |  18 ++--
 drivers/clk/qcom/gcc-sdx55.c          |   2 +-
 drivers/clk/qcom/gcc-sdx65.c          |   2 +-
 drivers/clk/qcom/gcc-sdx75.c          |   4 +-
 drivers/clk/qcom/gcc-sm6115.c         | 155 ++++++++--------------------------
 drivers/clk/qcom/gcc-sm6375.c         |  34 +++++---
 drivers/clk/qcom/gcc-sm7150.c         |  16 ++--
 drivers/clk/qcom/gcc-sm8250.c         |  12 +--
 drivers/clk/qcom/gcc-sm8350.c         |  14 +--
 drivers/clk/qcom/gcc-sm8450.c         |  14 +--
 drivers/clk/qcom/gcc-sm8550.c         |  14 +--
 drivers/clk/qcom/gpucc-sc7280.c       |   4 +-
 drivers/clk/qcom/gpucc-sc8280xp.c     |   4 +-
 drivers/clk/qcom/gpucc-sm6115.c       |  57 ++++++-------
 drivers/clk/qcom/gpucc-sm6375.c       |  38 ++-------
 drivers/clk/qcom/gpucc-sm8550.c       |   4 +-
 drivers/clk/qcom/lpasscorecc-sc7180.c |   2 +-
 drivers/clk/qcom/videocc-sm8250.c     |   4 +-
 drivers/clk/qcom/videocc-sm8350.c     |   4 +-
 drivers/clk/qcom/videocc-sm8450.c     |   6 +-
 drivers/clk/qcom/videocc-sm8550.c     |   6 +-
 37 files changed, 245 insertions(+), 399 deletions(-)
---
base-commit: 2205be537aeb1ca2ace998e2fefaa2be04e393e4
change-id: 20230717-topic-branch_aon_cleanup-6976c13fe71c

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

[PATCH 01/15] clk: qcom: branch: Add a helper for setting the enable bit

[Konrad Dybcio]

We harcode some clocks to be always-on, as they're essential to the
functioning of the SoC / some peripherals. Add a helper to do so
to make the writes less magic.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/clk-branch.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h
index 0cf800b9d08d..155818cc8d49 100644
--- a/drivers/clk/qcom/clk-branch.h
+++ b/drivers/clk/qcom/clk-branch.h
@@ -47,6 +47,7 @@ struct clk_branch {
 #define CBCR_FORCE_MEM_PERIPH_OFF	BIT(12)
 #define CBCR_WAKEUP			GENMASK(11, 8)
 #define CBCR_SLEEP			GENMASK(7, 4)
+#define CBCR_CLOCK_ENABLE		BIT(0)
 
 static inline void qcom_branch_set_force_mem_core(struct regmap *regmap,
 						  struct clk_branch clk, bool on)
@@ -81,6 +82,12 @@ static inline void qcom_branch_set_sleep(struct regmap *regmap, struct clk_branc
 			   FIELD_PREP(CBCR_SLEEP, val));
 }
 
+static inline void qcom_branch_set_clk_en(struct regmap *regmap, u32 cbcr)
+{
+	regmap_update_bits(regmap, cbcr, CBCR_CLOCK_ENABLE,
+			   CBCR_CLOCK_ENABLE);
+}
+
 extern const struct clk_ops clk_branch_ops;
 extern const struct clk_ops clk_branch2_ops;
 extern const struct clk_ops clk_branch_simple_ops;

-- 
2.41.0

Re: [PATCH 01/15] clk: qcom: branch: Add a helper for setting the enable bit

[Johan Hovold]

On Mon, Jul 17, 2023 at 05:19:08PM +0200, Konrad Dybcio wrote:
> We harcode some clocks to be always-on, as they're essential to the

typo: hardcode

> functioning of the SoC / some peripherals. Add a helper to do so
> to make the writes less magic.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Looks good otherwise:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

[PATCH 02/15] clk: qcom: Use qcom_branch_set_clk_en()

[Konrad Dybcio]

Instead of magically poking at the bit0 of branch clocks' CBCR, use
the newly introduced helper.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/dispcc-qcm2290.c     |  2 +-
 drivers/clk/qcom/dispcc-sc7280.c      |  2 +-
 drivers/clk/qcom/dispcc-sc8280xp.c    |  2 +-
 drivers/clk/qcom/dispcc-sm6115.c      |  2 +-
 drivers/clk/qcom/dispcc-sm8250.c      |  2 +-
 drivers/clk/qcom/dispcc-sm8450.c      |  2 +-
 drivers/clk/qcom/dispcc-sm8550.c      |  2 +-
 drivers/clk/qcom/gcc-sa8775p.c        | 18 +++++++++---------
 drivers/clk/qcom/gcc-sc7180.c         | 16 ++++++++--------
 drivers/clk/qcom/gcc-sc7280.c         | 14 +++++++-------
 drivers/clk/qcom/gcc-sc8180x.c        | 20 ++++++++++----------
 drivers/clk/qcom/gcc-sc8280xp.c       | 18 +++++++++---------
 drivers/clk/qcom/gcc-sdx55.c          |  2 +-
 drivers/clk/qcom/gcc-sdx65.c          |  2 +-
 drivers/clk/qcom/gcc-sdx75.c          |  4 ++--
 drivers/clk/qcom/gcc-sm6375.c         |  6 +++---
 drivers/clk/qcom/gcc-sm7150.c         | 16 ++++++++--------
 drivers/clk/qcom/gcc-sm8250.c         | 12 ++++++------
 drivers/clk/qcom/gcc-sm8350.c         | 14 +++++++-------
 drivers/clk/qcom/gcc-sm8450.c         | 14 +++++++-------
 drivers/clk/qcom/gcc-sm8550.c         | 14 +++++++-------
 drivers/clk/qcom/gpucc-sc7280.c       |  4 ++--
 drivers/clk/qcom/gpucc-sc8280xp.c     |  4 ++--
 drivers/clk/qcom/gpucc-sm8550.c       |  4 ++--
 drivers/clk/qcom/lpasscorecc-sc7180.c |  2 +-
 drivers/clk/qcom/videocc-sm8250.c     |  4 ++--
 drivers/clk/qcom/videocc-sm8350.c     |  4 ++--
 drivers/clk/qcom/videocc-sm8450.c     |  6 +++---
 drivers/clk/qcom/videocc-sm8550.c     |  6 +++---
 29 files changed, 109 insertions(+), 109 deletions(-)

diff --git a/drivers/clk/qcom/dispcc-qcm2290.c b/drivers/clk/qcom/dispcc-qcm2290.c
index 44dd5cfcc150..5f90e8c15c01 100644
--- a/drivers/clk/qcom/dispcc-qcm2290.c
+++ b/drivers/clk/qcom/dispcc-qcm2290.c
@@ -520,7 +520,7 @@ static int disp_cc_qcm2290_probe(struct platform_device *pdev)
 	clk_alpha_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
 
 	/* Keep DISP_CC_XO_CLK always-ON */
-	regmap_update_bits(regmap, 0x604c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x604c);
 
 	ret = qcom_cc_really_probe(pdev, &disp_cc_qcm2290_desc, regmap);
 	if (ret) {
diff --git a/drivers/clk/qcom/dispcc-sc7280.c b/drivers/clk/qcom/dispcc-sc7280.c
index ad596d567f6a..975f31b51539 100644
--- a/drivers/clk/qcom/dispcc-sc7280.c
+++ b/drivers/clk/qcom/dispcc-sc7280.c
@@ -882,7 +882,7 @@ static int disp_cc_sc7280_probe(struct platform_device *pdev)
 	 * Keep the clocks always-ON
 	 * DISP_CC_XO_CLK
 	 */
-	regmap_update_bits(regmap, 0x5008, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x5008);
 
 	return qcom_cc_really_probe(pdev, &disp_cc_sc7280_desc, regmap);
 }
diff --git a/drivers/clk/qcom/dispcc-sc8280xp.c b/drivers/clk/qcom/dispcc-sc8280xp.c
index 167470beb369..37b8bbbe9282 100644
--- a/drivers/clk/qcom/dispcc-sc8280xp.c
+++ b/drivers/clk/qcom/dispcc-sc8280xp.c
@@ -3179,7 +3179,7 @@ static int disp_cc_sc8280xp_probe(struct platform_device *pdev)
 	}
 
 	/* DISP_CC_XO_CLK always-on */
-	regmap_update_bits(regmap, 0x605c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x605c);
 
 out_pm_runtime_put:
 	pm_runtime_put_sync(&pdev->dev);
diff --git a/drivers/clk/qcom/dispcc-sm6115.c b/drivers/clk/qcom/dispcc-sm6115.c
index 1937edf23f21..cf88ca143bd7 100644
--- a/drivers/clk/qcom/dispcc-sm6115.c
+++ b/drivers/clk/qcom/dispcc-sm6115.c
@@ -584,7 +584,7 @@ static int disp_cc_sm6115_probe(struct platform_device *pdev)
 	clk_alpha_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
 
 	/* Keep DISP_CC_XO_CLK always-ON */
-	regmap_update_bits(regmap, 0x604c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x604c);
 
 	ret = qcom_cc_really_probe(pdev, &disp_cc_sm6115_desc, regmap);
 	if (ret) {
diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
index e17bb8b543b5..4f2297043820 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -1366,7 +1366,7 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev)
 	regmap_update_bits(regmap, 0x8000, 0x10, 0x10);
 
 	/* DISP_CC_XO_CLK always-on */
-	regmap_update_bits(regmap, 0x605c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x605c);
 
 	ret = qcom_cc_really_probe(pdev, &disp_cc_sm8250_desc, regmap);
 
diff --git a/drivers/clk/qcom/dispcc-sm8450.c b/drivers/clk/qcom/dispcc-sm8450.c
index adbfd30bfc96..e258cd9ba87e 100644
--- a/drivers/clk/qcom/dispcc-sm8450.c
+++ b/drivers/clk/qcom/dispcc-sm8450.c
@@ -1789,7 +1789,7 @@ static int disp_cc_sm8450_probe(struct platform_device *pdev)
 	 * Keep clocks always enabled:
 	 *	disp_cc_xo_clk
 	 */
-	regmap_update_bits(regmap, 0xe05c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0xe05c);
 
 	ret = qcom_cc_really_probe(pdev, &disp_cc_sm8450_desc, regmap);
 
diff --git a/drivers/clk/qcom/dispcc-sm8550.c b/drivers/clk/qcom/dispcc-sm8550.c
index 1e5a11081860..2bd6ca2c952f 100644
--- a/drivers/clk/qcom/dispcc-sm8550.c
+++ b/drivers/clk/qcom/dispcc-sm8550.c
@@ -1774,7 +1774,7 @@ static int disp_cc_sm8550_probe(struct platform_device *pdev)
 	 * Keep clocks always enabled:
 	 *	disp_cc_xo_clk
 	 */
-	regmap_update_bits(regmap, 0xe054, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0xe054);
 
 	ret = qcom_cc_really_probe(pdev, &disp_cc_sm8550_desc, regmap);
 
diff --git a/drivers/clk/qcom/gcc-sa8775p.c b/drivers/clk/qcom/gcc-sa8775p.c
index bb94ff367abd..9db144b7f05f 100644
--- a/drivers/clk/qcom/gcc-sa8775p.c
+++ b/drivers/clk/qcom/gcc-sa8775p.c
@@ -4748,15 +4748,15 @@ static int gcc_sa8775p_probe(struct platform_device *pdev)
 	 * GCC_DISP1_XO_CLK, GCC_DISP_AHB_CLK, GCC_DISP_XO_CLK,
 	 * GCC_GPU_CFG_AHB_CLK, GCC_VIDEO_AHB_CLK, GCC_VIDEO_XO_CLK.
 	 */
-	regmap_update_bits(regmap, 0x32004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x32020, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xc7004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xc7018, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x33004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x33018, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x7d004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x34004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x34024, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x32004);
+	qcom_branch_set_clk_en(regmap, 0x32020);
+	qcom_branch_set_clk_en(regmap, 0xc7004);
+	qcom_branch_set_clk_en(regmap, 0xc7018);
+	qcom_branch_set_clk_en(regmap, 0x33004);
+	qcom_branch_set_clk_en(regmap, 0x33018);
+	qcom_branch_set_clk_en(regmap, 0x7d004);
+	qcom_branch_set_clk_en(regmap, 0x34004);
+	qcom_branch_set_clk_en(regmap, 0x34024);
 
 	return qcom_cc_really_probe(pdev, &gcc_sa8775p_desc, regmap);
 }
diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
index cef3c77564cf..4f79ecb8300d 100644
--- a/drivers/clk/qcom/gcc-sc7180.c
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -2447,14 +2447,14 @@ static int gcc_sc7180_probe(struct platform_device *pdev)
 	 * GCC_CPUSS_GNOC_CLK, GCC_VIDEO_AHB_CLK, GCC_CAMERA_AHB_CLK,
 	 * GCC_DISP_AHB_CLK, GCC_GPU_CFG_AHB_CLK
 	 */
-	regmap_update_bits(regmap, 0x48004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b008, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b02c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b028, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b030, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x48004);
+	qcom_branch_set_clk_en(regmap, 0x0b004);
+	qcom_branch_set_clk_en(regmap, 0x0b008);
+	qcom_branch_set_clk_en(regmap, 0x0b00c);
+	qcom_branch_set_clk_en(regmap, 0x0b02c);
+	qcom_branch_set_clk_en(regmap, 0x0b028);
+	qcom_branch_set_clk_en(regmap, 0x0b030);
+	qcom_branch_set_clk_en(regmap, 0x71004);
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
 					ARRAY_SIZE(gcc_dfs_clocks));
diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 1dc804154031..b23f7103d08d 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -3458,13 +3458,13 @@ static int gcc_sc7280_probe(struct platform_device *pdev)
 	 * GCC_CAMERA_AHB_CLK/XO_CLK, GCC_DISP_AHB_CLK/XO_CLK
 	 * GCC_VIDEO_AHB_CLK/XO_CLK, GCC_GPU_CFG_AHB_CLK
 	 */
-	regmap_update_bits(regmap, 0x26004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x26028, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x27004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x2701C, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x28004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x28014, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x26004);
+	qcom_branch_set_clk_en(regmap, 0x26028);
+	qcom_branch_set_clk_en(regmap, 0x27004);
+	qcom_branch_set_clk_en(regmap, 0x2701C);
+	qcom_branch_set_clk_en(regmap, 0x28004);
+	qcom_branch_set_clk_en(regmap, 0x28014);
+	qcom_branch_set_clk_en(regmap, 0x71004);
 	regmap_update_bits(regmap, 0x7100C, BIT(13), BIT(13));
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
diff --git a/drivers/clk/qcom/gcc-sc8180x.c b/drivers/clk/qcom/gcc-sc8180x.c
index c41b9f010585..14f09c407198 100644
--- a/drivers/clk/qcom/gcc-sc8180x.c
+++ b/drivers/clk/qcom/gcc-sc8180x.c
@@ -4587,16 +4587,16 @@ static int gcc_sc8180x_probe(struct platform_device *pdev)
 	 * GCC_CPUSS_GNOC_CLK, GCC_CPUSS_DVM_BUS_CLK, GCC_NPU_CFG_AHB_CLK and
 	 * GCC_GPU_CFG_AHB_CLK
 	 */
-	regmap_update_bits(regmap, 0xb004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xb008, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xb00c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xb040, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xb044, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xb048, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x48004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x48190, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x4d004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0xb004);
+	qcom_branch_set_clk_en(regmap, 0xb008);
+	qcom_branch_set_clk_en(regmap, 0xb00c);
+	qcom_branch_set_clk_en(regmap, 0xb040);
+	qcom_branch_set_clk_en(regmap, 0xb044);
+	qcom_branch_set_clk_en(regmap, 0xb048);
+	qcom_branch_set_clk_en(regmap, 0x48004);
+	qcom_branch_set_clk_en(regmap, 0x48190);
+	qcom_branch_set_clk_en(regmap, 0x4d004);
+	qcom_branch_set_clk_en(regmap, 0x71004);
 
 	/* Disable the GPLL0 active input to NPU and GPU via MISC registers */
 	regmap_update_bits(regmap, 0x4d110, 0x3, 0x3);
diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
index 1fb6ffac730c..a9b3735baa4b 100644
--- a/drivers/clk/qcom/gcc-sc8280xp.c
+++ b/drivers/clk/qcom/gcc-sc8280xp.c
@@ -7549,15 +7549,15 @@ static int gcc_sc8280xp_probe(struct platform_device *pdev)
 	 * 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
 	 */
-	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));
+	qcom_branch_set_clk_en(regmap, 0x26004);
+	qcom_branch_set_clk_en(regmap, 0x26020);
+	qcom_branch_set_clk_en(regmap, 0x27004);
+	qcom_branch_set_clk_en(regmap, 0x27028);
+	qcom_branch_set_clk_en(regmap, 0x71004);
+	qcom_branch_set_clk_en(regmap, 0x28004);
+	qcom_branch_set_clk_en(regmap, 0x28028);
+	qcom_branch_set_clk_en(regmap, 0xbb004);
+	qcom_branch_set_clk_en(regmap, 0xbb028);
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
 	if (ret)
diff --git a/drivers/clk/qcom/gcc-sdx55.c b/drivers/clk/qcom/gcc-sdx55.c
index d5e17122698c..b1ef6223b5ee 100644
--- a/drivers/clk/qcom/gcc-sdx55.c
+++ b/drivers/clk/qcom/gcc-sdx55.c
@@ -1616,7 +1616,7 @@ static int gcc_sdx55_probe(struct platform_device *pdev)
 	 * of the system:
 	 * GCC_SYS_NOC_CPUSS_AHB_CLK, GCC_CPUSS_AHB_CLK, GCC_CPUSS_GNOC_CLK
 	 */
-	regmap_update_bits(regmap, 0x6d008, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x6d008);
 	regmap_update_bits(regmap, 0x6d008, BIT(21), BIT(21));
 	regmap_update_bits(regmap, 0x6d008, BIT(22), BIT(22));
 
diff --git a/drivers/clk/qcom/gcc-sdx65.c b/drivers/clk/qcom/gcc-sdx65.c
index b0c17043551d..62ea059d528d 100644
--- a/drivers/clk/qcom/gcc-sdx65.c
+++ b/drivers/clk/qcom/gcc-sdx65.c
@@ -1579,7 +1579,7 @@ static int gcc_sdx65_probe(struct platform_device *pdev)
 	 * of the system:
 	 * GCC_SYS_NOC_CPUSS_AHB_CLK, GCC_CPUSS_AHB_CLK, GCC_CPUSS_GNOC_CLK
 	 */
-	regmap_update_bits(regmap, 0x6d008, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x6d008);
 	regmap_update_bits(regmap, 0x6d008, BIT(21), BIT(21));
 	regmap_update_bits(regmap, 0x6d008, BIT(22), BIT(22));
 
diff --git a/drivers/clk/qcom/gcc-sdx75.c b/drivers/clk/qcom/gcc-sdx75.c
index b6772abdcec5..3c838fb43ce8 100644
--- a/drivers/clk/qcom/gcc-sdx75.c
+++ b/drivers/clk/qcom/gcc-sdx75.c
@@ -2940,8 +2940,8 @@ static int gcc_sdx75_probe(struct platform_device *pdev)
 	 * gcc_ahb_pcie_link_clk
 	 * gcc_xo_pcie_link_clk
 	 */
-	regmap_update_bits(regmap, 0x3e004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x3e008, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x3e004);
+	qcom_branch_set_clk_en(regmap, 0x3e008);
 
 	return qcom_cc_really_probe(pdev, &gcc_sdx75_desc, regmap);
 }
diff --git a/drivers/clk/qcom/gcc-sm6375.c b/drivers/clk/qcom/gcc-sm6375.c
index 417a0fd242ec..e94e88bdfb91 100644
--- a/drivers/clk/qcom/gcc-sm6375.c
+++ b/drivers/clk/qcom/gcc-sm6375.c
@@ -3885,9 +3885,9 @@ static int gcc_sm6375_probe(struct platform_device *pdev)
 	 * Keep the following clocks always on:
 	 * GCC_CAMERA_XO_CLK, GCC_CPUSS_GNOC_CLK, GCC_DISP_XO_CLK
 	 */
-	regmap_update_bits(regmap, 0x17028, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x2b004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x1702c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x17028);
+	qcom_branch_set_clk_en(regmap, 0x2b004);
+	qcom_branch_set_clk_en(regmap, 0x1702c);
 
 	clk_lucid_pll_configure(&gpll10, regmap, &gpll10_config);
 	clk_lucid_pll_configure(&gpll11, regmap, &gpll11_config);
diff --git a/drivers/clk/qcom/gcc-sm7150.c b/drivers/clk/qcom/gcc-sm7150.c
index 6da87f0436d0..696cca37b48b 100644
--- a/drivers/clk/qcom/gcc-sm7150.c
+++ b/drivers/clk/qcom/gcc-sm7150.c
@@ -3008,14 +3008,14 @@ static int gcc_sm7150_probe(struct platform_device *pdev)
 	 * GCC_DISP_AHB_CLK, GCC_CAMERA_XO_CLK, GCC_VIDEO_XO_CLK,
 	 * GCC_DISP_XO_CLK, GCC_GPU_CFG_AHB_CLK
 	 */
-	regmap_update_bits(regmap, 0x48004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b008, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b02c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b028, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b030, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x48004);
+	qcom_branch_set_clk_en(regmap, 0x0b004);
+	qcom_branch_set_clk_en(regmap, 0x0b008);
+	qcom_branch_set_clk_en(regmap, 0x0b00c);
+	qcom_branch_set_clk_en(regmap, 0x0b02c);
+	qcom_branch_set_clk_en(regmap, 0x0b028);
+	qcom_branch_set_clk_en(regmap, 0x0b030);
+	qcom_branch_set_clk_en(regmap, 0x71004);
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_sm7150_dfs_desc,
 					ARRAY_SIZE(gcc_sm7150_dfs_desc));
diff --git a/drivers/clk/qcom/gcc-sm8250.c b/drivers/clk/qcom/gcc-sm8250.c
index b6cf4bc88d4d..ad3dd69dd198 100644
--- a/drivers/clk/qcom/gcc-sm8250.c
+++ b/drivers/clk/qcom/gcc-sm8250.c
@@ -3648,12 +3648,12 @@ static int gcc_sm8250_probe(struct platform_device *pdev)
 	 * GCC_CPUSS_DVM_BUS_CLK, GCC_GPU_CFG_AHB_CLK,
 	 * GCC_SYS_NOC_CPUSS_AHB_CLK
 	 */
-	regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b008, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x4818c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x52000, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x0b004);
+	qcom_branch_set_clk_en(regmap, 0x0b008);
+	qcom_branch_set_clk_en(regmap, 0x0b00c);
+	qcom_branch_set_clk_en(regmap, 0x4818c);
+	qcom_branch_set_clk_en(regmap, 0x71004);
+	qcom_branch_set_clk_en(regmap, 0x52000);
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
 				       ARRAY_SIZE(gcc_dfs_clocks));
diff --git a/drivers/clk/qcom/gcc-sm8350.c b/drivers/clk/qcom/gcc-sm8350.c
index 1385a98eb3bb..b56a7669b770 100644
--- a/drivers/clk/qcom/gcc-sm8350.c
+++ b/drivers/clk/qcom/gcc-sm8350.c
@@ -3811,13 +3811,13 @@ static int gcc_sm8350_probe(struct platform_device *pdev)
 	 * GCC_CAMERA_AHB_CLK, GCC_CAMERA_XO_CLK, GCC_DISP_AHB_CLK, GCC_DISP_XO_CLK,
 	 * GCC_GPU_CFG_AHB_CLK, GCC_VIDEO_AHB_CLK, GCC_VIDEO_XO_CLK
 	 */
-	regmap_update_bits(regmap, 0x26004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x26018, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x27004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x2701c, 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, 0x28020, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x26004);
+	qcom_branch_set_clk_en(regmap, 0x26018);
+	qcom_branch_set_clk_en(regmap, 0x27004);
+	qcom_branch_set_clk_en(regmap, 0x2701c);
+	qcom_branch_set_clk_en(regmap, 0x71004);
+	qcom_branch_set_clk_en(regmap, 0x28004);
+	qcom_branch_set_clk_en(regmap, 0x28020);
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
 	if (ret)
diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
index 75635d40a12d..5a7a723ff5ef 100644
--- a/drivers/clk/qcom/gcc-sm8450.c
+++ b/drivers/clk/qcom/gcc-sm8450.c
@@ -3285,13 +3285,13 @@ static int gcc_sm8450_probe(struct platform_device *pdev)
 	 * gcc_disp_xo_clk, gcc_gpu_cfg_ahb_clk, gcc_video_ahb_clk,
 	 * gcc_video_xo_clk
 	 */
-	regmap_update_bits(regmap, 0x36004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x36020, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x37004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x3701c, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x81004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x42004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x42028, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x36004);
+	qcom_branch_set_clk_en(regmap, 0x36020);
+	qcom_branch_set_clk_en(regmap, 0x37004);
+	qcom_branch_set_clk_en(regmap, 0x3701c);
+	qcom_branch_set_clk_en(regmap, 0x81004);
+	qcom_branch_set_clk_en(regmap, 0x42004);
+	qcom_branch_set_clk_en(regmap, 0x42028);
 
 	return qcom_cc_really_probe(pdev, &gcc_sm8450_desc, regmap);
 }
diff --git a/drivers/clk/qcom/gcc-sm8550.c b/drivers/clk/qcom/gcc-sm8550.c
index 277cd4f020ff..a6404421021f 100644
--- a/drivers/clk/qcom/gcc-sm8550.c
+++ b/drivers/clk/qcom/gcc-sm8550.c
@@ -3349,13 +3349,13 @@ static int gcc_sm8550_probe(struct platform_device *pdev)
 	 * gcc_disp_xo_clk, gcc_gpu_cfg_ahb_clk, gcc_video_ahb_clk,
 	 * gcc_video_xo_clk
 	 */
-	regmap_update_bits(regmap, 0x26004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x26028, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x27004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x27018, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x32004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x32030, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x26004);
+	qcom_branch_set_clk_en(regmap, 0x26028);
+	qcom_branch_set_clk_en(regmap, 0x27004);
+	qcom_branch_set_clk_en(regmap, 0x27018);
+	qcom_branch_set_clk_en(regmap, 0x71004);
+	qcom_branch_set_clk_en(regmap, 0x32004);
+	qcom_branch_set_clk_en(regmap, 0x32030);
 
 	/* Clear GDSC_SLEEP_ENA_VOTE to stop votes being auto-removed in sleep. */
 	regmap_write(regmap, 0x52024, 0x0);
diff --git a/drivers/clk/qcom/gpucc-sc7280.c b/drivers/clk/qcom/gpucc-sc7280.c
index 1490cd45a654..a678c51cd75e 100644
--- a/drivers/clk/qcom/gpucc-sc7280.c
+++ b/drivers/clk/qcom/gpucc-sc7280.c
@@ -461,8 +461,8 @@ static int gpu_cc_sc7280_probe(struct platform_device *pdev)
 	 * Keep the clocks always-ON
 	 * GPU_CC_CB_CLK, GPUCC_CX_GMU_CLK
 	 */
-	regmap_update_bits(regmap, 0x1170, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x1098, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x1170);
+	qcom_branch_set_clk_en(regmap, 0x1098);
 	regmap_update_bits(regmap, 0x1098, BIT(13), BIT(13));
 
 	return qcom_cc_really_probe(pdev, &gpu_cc_sc7280_desc, regmap);
diff --git a/drivers/clk/qcom/gpucc-sc8280xp.c b/drivers/clk/qcom/gpucc-sc8280xp.c
index 8e147ee294ee..c709365a3c57 100644
--- a/drivers/clk/qcom/gpucc-sc8280xp.c
+++ b/drivers/clk/qcom/gpucc-sc8280xp.c
@@ -448,8 +448,8 @@ static int gpu_cc_sc8280xp_probe(struct platform_device *pdev)
 	 * Keep the clocks always-ON
 	 * GPU_CC_CB_CLK, GPU_CC_CXO_CLK
 	 */
-	regmap_update_bits(regmap, 0x1170, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x109c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x1170);
+	qcom_branch_set_clk_en(regmap, 0x109c);
 
 	ret = qcom_cc_really_probe(pdev, &gpu_cc_sc8280xp_desc, regmap);
 	pm_runtime_put(&pdev->dev);
diff --git a/drivers/clk/qcom/gpucc-sm8550.c b/drivers/clk/qcom/gpucc-sm8550.c
index 8a2e3522af51..225807979435 100644
--- a/drivers/clk/qcom/gpucc-sm8550.c
+++ b/drivers/clk/qcom/gpucc-sm8550.c
@@ -581,8 +581,8 @@ static int gpu_cc_sm8550_probe(struct platform_device *pdev)
 	 *	gpu_cc_cxo_aon_clk
 	 *	gpu_cc_demet_clk
 	 */
-	regmap_update_bits(regmap, 0x9004, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x900c, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x9004);
+	qcom_branch_set_clk_en(regmap, 0x900c);
 
 	return qcom_cc_really_probe(pdev, &gpu_cc_sm8550_desc, regmap);
 }
diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
index 010867dcc2ef..7fa2e28489fd 100644
--- a/drivers/clk/qcom/lpasscorecc-sc7180.c
+++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
@@ -405,7 +405,7 @@ static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
 	 * Keep the CLK always-ON
 	 * LPASS_AUDIO_CORE_SYSNOC_SWAY_CORE_CLK
 	 */
-	regmap_update_bits(regmap, 0x24000, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x24000);
 
 	/* PLL settings */
 	regmap_write(regmap, 0x1008, 0x20);
diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
index ad46c4014a40..1f269025f3f8 100644
--- a/drivers/clk/qcom/videocc-sm8250.c
+++ b/drivers/clk/qcom/videocc-sm8250.c
@@ -384,8 +384,8 @@ static int video_cc_sm8250_probe(struct platform_device *pdev)
 	clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
 
 	/* Keep VIDEO_CC_AHB_CLK and VIDEO_CC_XO_CLK ALWAYS-ON */
-	regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0xe58);
+	qcom_branch_set_clk_en(regmap, 0xeec);
 
 	ret = qcom_cc_really_probe(pdev, &video_cc_sm8250_desc, regmap);
 
diff --git a/drivers/clk/qcom/videocc-sm8350.c b/drivers/clk/qcom/videocc-sm8350.c
index b148877fc73d..4e6b2c7fe61b 100644
--- a/drivers/clk/qcom/videocc-sm8350.c
+++ b/drivers/clk/qcom/videocc-sm8350.c
@@ -524,8 +524,8 @@ static int video_cc_sm8350_probe(struct platform_device *pdev)
 	 *      video_cc_ahb_clk
 	 *      video_cc_xo_clk
 	 */
-	regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0xe58);
+	qcom_branch_set_clk_en(regmap, 0xeec);
 
 	ret = qcom_cc_really_probe(pdev, &video_cc_sm8350_desc, regmap);
 	pm_runtime_put(&pdev->dev);
diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
index 7d0029b8b799..deaf58c95749 100644
--- a/drivers/clk/qcom/videocc-sm8450.c
+++ b/drivers/clk/qcom/videocc-sm8450.c
@@ -428,9 +428,9 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
 	 *	video_cc_sleep_clk
 	 *	video_cc_xo_clk
 	 */
-	regmap_update_bits(regmap, 0x80e4, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x8130, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x8114, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x80e4);
+	qcom_branch_set_clk_en(regmap, 0x8130);
+	qcom_branch_set_clk_en(regmap, 0x8114);
 
 	ret = qcom_cc_really_probe(pdev, &video_cc_sm8450_desc, regmap);
 
diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
index e2400fe23e60..802bbd616b2f 100644
--- a/drivers/clk/qcom/videocc-sm8550.c
+++ b/drivers/clk/qcom/videocc-sm8550.c
@@ -435,9 +435,9 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
 	 *	video_cc_sleep_clk
 	 *	video_cc_xo_clk
 	 */
-	regmap_update_bits(regmap, 0x80f4, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x8140, BIT(0), BIT(0));
-	regmap_update_bits(regmap, 0x8124, BIT(0), BIT(0));
+	qcom_branch_set_clk_en(regmap, 0x80f4);
+	qcom_branch_set_clk_en(regmap, 0x8140);
+	qcom_branch_set_clk_en(regmap, 0x8124);
 
 	ret = qcom_cc_really_probe(pdev, &video_cc_sm8550_desc, regmap);
 

-- 
2.41.0

Re: [PATCH 02/15] clk: qcom: Use qcom_branch_set_clk_en()

[Johan Hovold]

On Mon, Jul 17, 2023 at 05:19:09PM +0200, Konrad Dybcio wrote:
> Instead of magically poking at the bit0 of branch clocks' CBCR, use
> the newly introduced helper.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Nice cleanup.

Acked-by: Johan Hovold <johan+linaro@kernel.org>

[PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Konrad Dybcio]

Some clocks need to be always-on, but we don't really do anything
with them, other than calling enable() once and telling Linux they're
enabled.

Unregister them to save a couple of bytes and, perhaps more
importantly, allow for runtime suspend of the clock controller device,
as CLK_IS_CRITICAL prevents the latter.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm6375.c | 103 ++++++------------------------------------
 1 file changed, 13 insertions(+), 90 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm6375.c b/drivers/clk/qcom/gcc-sm6375.c
index e94e88bdfb91..14dafea45ac9 100644
--- a/drivers/clk/qcom/gcc-sm6375.c
+++ b/drivers/clk/qcom/gcc-sm6375.c
@@ -1742,22 +1742,6 @@ static struct clk_branch gcc_cam_throttle_rt_clk = {
 	},
 };
 
-static struct clk_branch gcc_camera_ahb_clk = {
-	.halt_reg = 0x17008,
-	.halt_check = BRANCH_HALT_DELAY,
-	.hwcg_reg = 0x17008,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x17008,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_camera_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_camss_axi_clk = {
 	.halt_reg = 0x58044,
 	.halt_check = BRANCH_HALT,
@@ -2308,22 +2292,6 @@ static struct clk_branch gcc_cfg_noc_usb3_prim_axi_clk = {
 	},
 };
 
-static struct clk_branch gcc_disp_ahb_clk = {
-	.halt_reg = 0x1700c,
-	.halt_check = BRANCH_HALT_VOTED,
-	.hwcg_reg = 0x1700c,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x1700c,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_disp_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_regmap_div gcc_disp_gpll0_clk_src = {
 	.reg = 0x17058,
 	.shift = 0,
@@ -2454,22 +2422,6 @@ static struct clk_branch gcc_gp3_clk = {
 	},
 };
 
-static struct clk_branch gcc_gpu_cfg_ahb_clk = {
-	.halt_reg = 0x36004,
-	.halt_check = BRANCH_HALT_VOTED,
-	.hwcg_reg = 0x36004,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x36004,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_gpu_cfg_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_gpu_gpll0_clk_src = {
 	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
@@ -3093,26 +3045,6 @@ static struct clk_branch gcc_sdcc2_apps_clk = {
 	},
 };
 
-static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = {
-	.halt_reg = 0x2b06c,
-	.halt_check = BRANCH_HALT_VOTED,
-	.hwcg_reg = 0x2b06c,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x79004,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_sys_noc_cpuss_ahb_clk",
-			.parent_hws = (const struct clk_hw*[]) {
-				&gcc_cpuss_ahb_postdiv_clk_src.clkr.hw,
-			},
-			.num_parents = 1,
-			.flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_sys_noc_ufs_phy_axi_clk = {
 	.halt_reg = 0x45098,
 	.halt_check = BRANCH_HALT,
@@ -3432,22 +3364,6 @@ static struct clk_branch gcc_venus_ctl_axi_clk = {
 	},
 };
 
-static struct clk_branch gcc_video_ahb_clk = {
-	.halt_reg = 0x17004,
-	.halt_check = BRANCH_HALT_DELAY,
-	.hwcg_reg = 0x17004,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x17004,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_video_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_video_axi0_clk = {
 	.halt_reg = 0x1701c,
 	.halt_check = BRANCH_HALT_VOTED,
@@ -3614,7 +3530,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr,
 	[GCC_CAM_THROTTLE_NRT_CLK] = &gcc_cam_throttle_nrt_clk.clkr,
 	[GCC_CAM_THROTTLE_RT_CLK] = &gcc_cam_throttle_rt_clk.clkr,
-	[GCC_CAMERA_AHB_CLK] = &gcc_camera_ahb_clk.clkr,
 	[GCC_CAMSS_AXI_CLK] = &gcc_camss_axi_clk.clkr,
 	[GCC_CAMSS_AXI_CLK_SRC] = &gcc_camss_axi_clk_src.clkr,
 	[GCC_CAMSS_CCI_0_CLK] = &gcc_camss_cci_0_clk.clkr,
@@ -3670,7 +3585,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_CFG_NOC_USB3_PRIM_AXI_CLK] = &gcc_cfg_noc_usb3_prim_axi_clk.clkr,
 	[GCC_CPUSS_AHB_CLK_SRC] = &gcc_cpuss_ahb_clk_src.clkr,
 	[GCC_CPUSS_AHB_POSTDIV_CLK_SRC] = &gcc_cpuss_ahb_postdiv_clk_src.clkr,
-	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,
 	[GCC_DISP_GPLL0_CLK_SRC] = &gcc_disp_gpll0_clk_src.clkr,
 	[GCC_DISP_GPLL0_DIV_CLK_SRC] = &gcc_disp_gpll0_div_clk_src.clkr,
 	[GCC_DISP_HF_AXI_CLK] = &gcc_disp_hf_axi_clk.clkr,
@@ -3682,7 +3596,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_GP2_CLK_SRC] = &gcc_gp2_clk_src.clkr,
 	[GCC_GP3_CLK] = &gcc_gp3_clk.clkr,
 	[GCC_GP3_CLK_SRC] = &gcc_gp3_clk_src.clkr,
-	[GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
 	[GCC_GPU_GPLL0_CLK_SRC] = &gcc_gpu_gpll0_clk_src.clkr,
 	[GCC_GPU_GPLL0_DIV_CLK_SRC] = &gcc_gpu_gpll0_div_clk_src.clkr,
 	[GCC_GPU_MEMNOC_GFX_CLK] = &gcc_gpu_memnoc_gfx_clk.clkr,
@@ -3738,7 +3651,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_SDCC2_AHB_CLK] = &gcc_sdcc2_ahb_clk.clkr,
 	[GCC_SDCC2_APPS_CLK] = &gcc_sdcc2_apps_clk.clkr,
 	[GCC_SDCC2_APPS_CLK_SRC] = &gcc_sdcc2_apps_clk_src.clkr,
-	[GCC_SYS_NOC_CPUSS_AHB_CLK] = &gcc_sys_noc_cpuss_ahb_clk.clkr,
 	[GCC_SYS_NOC_UFS_PHY_AXI_CLK] = &gcc_sys_noc_ufs_phy_axi_clk.clkr,
 	[GCC_SYS_NOC_USB3_PRIM_AXI_CLK] = &gcc_sys_noc_usb3_prim_axi_clk.clkr,
 	[GCC_UFS_PHY_AHB_CLK] = &gcc_ufs_phy_ahb_clk.clkr,
@@ -3765,7 +3677,6 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_VCODEC0_AXI_CLK] = &gcc_vcodec0_axi_clk.clkr,
 	[GCC_VENUS_AHB_CLK] = &gcc_venus_ahb_clk.clkr,
 	[GCC_VENUS_CTL_AXI_CLK] = &gcc_venus_ctl_axi_clk.clkr,
-	[GCC_VIDEO_AHB_CLK] = &gcc_video_ahb_clk.clkr,
 	[GCC_VIDEO_AXI0_CLK] = &gcc_video_axi0_clk.clkr,
 	[GCC_VIDEO_THROTTLE_CORE_CLK] = &gcc_video_throttle_core_clk.clkr,
 	[GCC_VIDEO_VCODEC0_SYS_CLK] = &gcc_video_vcodec0_sys_clk.clkr,
@@ -3883,11 +3794,23 @@ static int gcc_sm6375_probe(struct platform_device *pdev)
 
 	/*
 	 * Keep the following clocks always on:
-	 * GCC_CAMERA_XO_CLK, GCC_CPUSS_GNOC_CLK, GCC_DISP_XO_CLK
+	 * GCC_CAMERA_XO_CLK
+	 * GCC_CPUSS_GNOC_CLK
+	 * GCC_DISP_XO_CLK
+	 * GCC_CAMERA_AHB_CLK
+	 * GCC_DISP_AHB_CLK
+	 * GCC_GPU_CFG_AHB_CLK
+	 * GCC_SYS_NOC_CPUSS_AHB_CLK
+	 * GCC_VIDEO_AHB_CLK
 	 */
 	qcom_branch_set_clk_en(regmap, 0x17028);
 	qcom_branch_set_clk_en(regmap, 0x2b004);
 	qcom_branch_set_clk_en(regmap, 0x1702c);
+	qcom_branch_set_clk_en(regmap, 0x17008);
+	qcom_branch_set_clk_en(regmap, 0x1700c);
+	qcom_branch_set_clk_en(regmap, 0x36004);
+	qcom_branch_set_clk_en(regmap, 0x2b06c);
+	qcom_branch_set_clk_en(regmap, 0x17004);
 
 	clk_lucid_pll_configure(&gpll10, regmap, &gpll10_config);
 	clk_lucid_pll_configure(&gpll11, regmap, &gpll11_config);

-- 
2.41.0

Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Johan Hovold]

On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
> Some clocks need to be always-on, but we don't really do anything
> with them, other than calling enable() once and telling Linux they're
> enabled.
> 
> Unregister them to save a couple of bytes and, perhaps more
> importantly, allow for runtime suspend of the clock controller device,
> as CLK_IS_CRITICAL prevents the latter.

But this doesn't sound right. How can you disable a controller which
still has clocks enabled?

Shouldn't instead these clocks be modelled properly so that they are
only enabled when actually needed?

Johan

Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Konrad Dybcio]

On 18.07.2023 15:20, Johan Hovold wrote:
> On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
>> Some clocks need to be always-on, but we don't really do anything
>> with them, other than calling enable() once and telling Linux they're
>> enabled.
>>
>> Unregister them to save a couple of bytes and, perhaps more
>> importantly, allow for runtime suspend of the clock controller device,
>> as CLK_IS_CRITICAL prevents the latter.
> 
> But this doesn't sound right. How can you disable a controller which
> still has clocks enabled?
> 
> Shouldn't instead these clocks be modelled properly so that they are
> only enabled when actually needed?
Hm.. We do have clk_branch2_aon_ops, but something still needs to
toggle these clocks.

I *think* we could leave a permanent vote in probe() without breaking
runtime pm! I'll give it a spin bit later..

Konrad

Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Bjorn Andersson]

On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote:
> On 18.07.2023 15:20, Johan Hovold wrote:
> > On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
> >> Some clocks need to be always-on, but we don't really do anything
> >> with them, other than calling enable() once and telling Linux they're
> >> enabled.
> >>
> >> Unregister them to save a couple of bytes and, perhaps more
> >> importantly, allow for runtime suspend of the clock controller device,
> >> as CLK_IS_CRITICAL prevents the latter.
> > 
> > But this doesn't sound right. How can you disable a controller which
> > still has clocks enabled?
> > 
> > Shouldn't instead these clocks be modelled properly so that they are
> > only enabled when actually needed?
> Hm.. We do have clk_branch2_aon_ops, but something still needs to
> toggle these clocks.
> 

Before we started replacing these clocks with static votes, I handled
exactly this problem in the turingcc-qcs404 driver by registering the
ahb clock with a pm_clk_add(). The clock framework will then
automagically keep the clock enabled around operations, but it will also
keep the runtime state active as long as the clock is prepared.

As mentioned in an earlier reply today, there's no similarity to this in
the reset or gdsc code, so we'd need to add that in order to rely on
such mechanism.

> I *think* we could leave a permanent vote in probe() without breaking
> runtime pm! I'll give it a spin bit later..
> 

Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would
properly connect the two, and thereby handle probe order between the two
clock controllers.

But it would prevent the power-domain of the parent provider to ever
suspending. Using pm_clk_add() this would at least depend on client
votes.

Regards,
Bjorn

Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Johan Hovold]

On Tue, Jul 18, 2023 at 09:23:52AM -0700, Bjorn Andersson wrote:
> On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote:
> > On 18.07.2023 15:20, Johan Hovold wrote:
> > > On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
> > >> Some clocks need to be always-on, but we don't really do anything
> > >> with them, other than calling enable() once and telling Linux they're
> > >> enabled.
> > >>
> > >> Unregister them to save a couple of bytes and, perhaps more
> > >> importantly, allow for runtime suspend of the clock controller device,
> > >> as CLK_IS_CRITICAL prevents the latter.
> > > 
> > > But this doesn't sound right. How can you disable a controller which
> > > still has clocks enabled?
> > > 
> > > Shouldn't instead these clocks be modelled properly so that they are
> > > only enabled when actually needed?
> > Hm.. We do have clk_branch2_aon_ops, but something still needs to
> > toggle these clocks.
> > 
> 
> Before we started replacing these clocks with static votes, I handled
> exactly this problem in the turingcc-qcs404 driver by registering the
> ahb clock with a pm_clk_add(). The clock framework will then
> automagically keep the clock enabled around operations, but it will also
> keep the runtime state active as long as the clock is prepared.
> 
> As mentioned in an earlier reply today, there's no similarity to this in
> the reset or gdsc code, so we'd need to add that in order to rely on
> such mechanism.

This reminds me of:

	4cc47e8add63 ("clk: qcom: gdsc: Remove direct runtime PM calls")

which recently removed a broken attempt to implement this for gdscs.

Just stumbled over GENPD_FLAG_PM_CLK which may provide a way forward at
least for genpd (but see below).

> > I *think* we could leave a permanent vote in probe() without breaking
> > runtime pm! I'll give it a spin bit later..
> > 
> 
> Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would
> properly connect the two, and thereby handle probe order between the two
> clock controllers.

Yeah, this dependency really should be described eventually.

> But it would prevent the power-domain of the parent provider to ever
> suspending. Using pm_clk_add() this would at least depend on client
> votes.

IIUC using pm_clk_add() would also prevent the parent from suspending
due to that resume call in clk_prepare().

And this mechanism is also used for GENPD_FLAG_PM_CLK...

Johan

Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Konrad Dybcio]

On 19.07.2023 10:43, Johan Hovold wrote:
> On Tue, Jul 18, 2023 at 09:23:52AM -0700, Bjorn Andersson wrote:
>> On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote:
>>> On 18.07.2023 15:20, Johan Hovold wrote:
>>>> On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
>>>>> Some clocks need to be always-on, but we don't really do anything
>>>>> with them, other than calling enable() once and telling Linux they're
>>>>> enabled.
>>>>>
>>>>> Unregister them to save a couple of bytes and, perhaps more
>>>>> importantly, allow for runtime suspend of the clock controller device,
>>>>> as CLK_IS_CRITICAL prevents the latter.
>>>>
>>>> But this doesn't sound right. How can you disable a controller which
>>>> still has clocks enabled?
>>>>
>>>> Shouldn't instead these clocks be modelled properly so that they are
>>>> only enabled when actually needed?
>>> Hm.. We do have clk_branch2_aon_ops, but something still needs to
>>> toggle these clocks.
>>>
>>
>> Before we started replacing these clocks with static votes, I handled
>> exactly this problem in the turingcc-qcs404 driver by registering the
>> ahb clock with a pm_clk_add(). The clock framework will then
>> automagically keep the clock enabled around operations, but it will also
>> keep the runtime state active as long as the clock is prepared.
>>
>> As mentioned in an earlier reply today, there's no similarity to this in
>> the reset or gdsc code, so we'd need to add that in order to rely on
>> such mechanism.
> 
> This reminds me of:
> 
> 	4cc47e8add63 ("clk: qcom: gdsc: Remove direct runtime PM calls")
> 
> which recently removed a broken attempt to implement this for gdscs.
> 
> Just stumbled over GENPD_FLAG_PM_CLK which may provide a way forward at
> least for genpd (but see below).
> 
>>> I *think* we could leave a permanent vote in probe() without breaking
>>> runtime pm! I'll give it a spin bit later..
>>>
>>
>> Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would
>> properly connect the two, and thereby handle probe order between the two
>> clock controllers.
> 
> Yeah, this dependency really should be described eventually.
> 
>> But it would prevent the power-domain of the parent provider to ever
>> suspending. Using pm_clk_add() this would at least depend on client
>> votes.
> 
> IIUC using pm_clk_add() would also prevent the parent from suspending
> due to that resume call in clk_prepare().
> 
> And this mechanism is also used for GENPD_FLAG_PM_CLK...
So.. how do we go about solving the issue that this patch tried to
address?

Konrad

Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Konrad Dybcio]

On 9.08.2023 18:52, Konrad Dybcio wrote:
> On 19.07.2023 10:43, Johan Hovold wrote:
>> On Tue, Jul 18, 2023 at 09:23:52AM -0700, Bjorn Andersson wrote:
>>> On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote:
>>>> On 18.07.2023 15:20, Johan Hovold wrote:
>>>>> On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
>>>>>> Some clocks need to be always-on, but we don't really do anything
>>>>>> with them, other than calling enable() once and telling Linux they're
>>>>>> enabled.
>>>>>>
>>>>>> Unregister them to save a couple of bytes and, perhaps more
>>>>>> importantly, allow for runtime suspend of the clock controller device,
>>>>>> as CLK_IS_CRITICAL prevents the latter.
>>>>>
>>>>> But this doesn't sound right. How can you disable a controller which
>>>>> still has clocks enabled?
>>>>>
>>>>> Shouldn't instead these clocks be modelled properly so that they are
>>>>> only enabled when actually needed?
>>>> Hm.. We do have clk_branch2_aon_ops, but something still needs to
>>>> toggle these clocks.
>>>>
>>>
>>> Before we started replacing these clocks with static votes, I handled
>>> exactly this problem in the turingcc-qcs404 driver by registering the
>>> ahb clock with a pm_clk_add(). The clock framework will then
>>> automagically keep the clock enabled around operations, but it will also
>>> keep the runtime state active as long as the clock is prepared.
>>>
>>> As mentioned in an earlier reply today, there's no similarity to this in
>>> the reset or gdsc code, so we'd need to add that in order to rely on
>>> such mechanism.
>>
>> This reminds me of:
>>
>> 	4cc47e8add63 ("clk: qcom: gdsc: Remove direct runtime PM calls")
>>
>> which recently removed a broken attempt to implement this for gdscs.
>>
>> Just stumbled over GENPD_FLAG_PM_CLK which may provide a way forward at
>> least for genpd (but see below).
>>
>>>> I *think* we could leave a permanent vote in probe() without breaking
>>>> runtime pm! I'll give it a spin bit later..
>>>>
>>>
>>> Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would
>>> properly connect the two, and thereby handle probe order between the two
>>> clock controllers.
>>
>> Yeah, this dependency really should be described eventually.
>>
>>> But it would prevent the power-domain of the parent provider to ever
>>> suspending. Using pm_clk_add() this would at least depend on client
>>> votes.
>>
>> IIUC using pm_clk_add() would also prevent the parent from suspending
>> due to that resume call in clk_prepare().
>>
>> And this mechanism is also used for GENPD_FLAG_PM_CLK...
> So.. how do we go about solving the issue that this patch tried to
> address?
I see things this way:

- clock controllers (non-gcc) that use magic writes today,
  they should stay as they are to avoid dramatic spaghetti wrt
  dt backwards compatibility

- clock controllers (non-gcc) that use CLK_IS_CRITICAL are
  transitioned to magic writes to skip the PM code fluff which
  prevents shutting down the PDs if any clock is critical and
  for uniformity with point 1 (as the device trees still don't
  contain any references to the necessary clocks)

- new clock controllers are modeled with use of pm_clk and with
  proper device tree references

FWIW Qualcomm nowadays just keep these clocks always on (answering
Johan's question - they're either off-from-Linux-POV-not-really-in-
hardware or these clocks retain the enable bit, I don't know) in
their shipping downstream kernels, the power leak is probably
minimal, but if we can avoid it, every nanowatt is to our advantage

Konrad

[PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM

[Konrad Dybcio]

The GCC block on SM6375 is powered by the VDD_CX rail. We need to ensure
that it's enabled to prevent unwanted power collapse.

Enable runtime PM to keep the power flowing only when necessary.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm6375.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm6375.c b/drivers/clk/qcom/gcc-sm6375.c
index 14dafea45ac9..4b2de545d3f8 100644
--- a/drivers/clk/qcom/gcc-sm6375.c
+++ b/drivers/clk/qcom/gcc-sm6375.c
@@ -7,6 +7,7 @@
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,sm6375-gcc.h>
@@ -3784,9 +3785,19 @@ static int gcc_sm6375_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	int ret;
 
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
+
 	regmap = qcom_cc_map(pdev, &gcc_sm6375_desc);
-	if (IS_ERR(regmap))
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
+	}
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
 	if (ret)
@@ -3817,7 +3828,10 @@ static int gcc_sm6375_probe(struct platform_device *pdev)
 	clk_lucid_pll_configure(&gpll8, regmap, &gpll8_config);
 	clk_zonda_pll_configure(&gpll9, regmap, &gpll9_config);
 
-	return qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
 }
 
 static struct platform_driver gcc_sm6375_driver = {

-- 
2.41.0

Re: [PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 05:19:11PM +0200, Konrad Dybcio wrote:
> The GCC block on SM6375 is powered by the VDD_CX rail. We need to ensure
> that it's enabled to prevent unwanted power collapse.
> 
> Enable runtime PM to keep the power flowing only when necessary.
> 

Are you sure this is necessary? If VDD_CX was really possible to fully
"power collapse" then I would expect that you lose all register
settings. This is not something we want or can even handle for GCC.
You would need to restore all frequency settings, branch bits etc etc.

Otherwise it's a retention state, but these are covered by the
corners/levels, not the enable/disable state.

I think most of these power domains are effectively always-on. The
important part is voting for minimal corners/levels so they can go to
minimal retention state with vlow/vmin.

Thanks,
Stephan

Re: [PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM

[Bjorn Andersson]

On Mon, Jul 17, 2023 at 06:26:35PM +0200, Stephan Gerhold wrote:
> On Mon, Jul 17, 2023 at 05:19:11PM +0200, Konrad Dybcio wrote:
> > The GCC block on SM6375 is powered by the VDD_CX rail. We need to ensure
> > that it's enabled to prevent unwanted power collapse.
> > 
> > Enable runtime PM to keep the power flowing only when necessary.
> > 
> 
> Are you sure this is necessary? If VDD_CX was really possible to fully
> "power collapse" then I would expect that you lose all register
> settings. This is not something we want or can even handle for GCC.
> You would need to restore all frequency settings, branch bits etc etc.
> 

This differ between platforms, some allow us to completely power down CX
while keeping registers state using MX, others require that CX stays in
retention at least.

So, CX isn't the only rail powering GCC. For the most part though, we
have a relationship between frequencies votes for by clients and the
corner of CX, and hence I think the current description is ok...

> Otherwise it's a retention state, but these are covered by the
> corners/levels, not the enable/disable state.
> 

I _think_ we still want to suspend each individual device (and the vote
from Linux), and let the system keep us at retention, instead of off...

> I think most of these power domains are effectively always-on. The
> important part is voting for minimal corners/levels so they can go to
> minimal retention state with vlow/vmin.
> 

When you hit s2idle, you should expect to have the majority of the
rpm(h) PDs be voted off.

Regards,
Bjorn

> Thanks,
> Stephan

Re: [PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 09:02:29PM -0700, Bjorn Andersson wrote:
> On Mon, Jul 17, 2023 at 06:26:35PM +0200, Stephan Gerhold wrote:
> > On Mon, Jul 17, 2023 at 05:19:11PM +0200, Konrad Dybcio wrote:
> > > The GCC block on SM6375 is powered by the VDD_CX rail. We need to ensure
> > > that it's enabled to prevent unwanted power collapse.
> > > 
> > > Enable runtime PM to keep the power flowing only when necessary.
> > > 
> > 
> > Are you sure this is necessary? If VDD_CX was really possible to fully
> > "power collapse" then I would expect that you lose all register
> > settings. This is not something we want or can even handle for GCC.
> > You would need to restore all frequency settings, branch bits etc etc.
> > 
> 
> This differ between platforms, some allow us to completely power down CX
> while keeping registers state using MX, others require that CX stays in
> retention at least.
> 
> So, CX isn't the only rail powering GCC. For the most part though, we
> have a relationship between frequencies votes for by clients and the
> corner of CX, and hence I think the current description is ok...
> 

This patch is just about sending enable/disable votes for the power
domains though, based on runtime PM which triggers when all the clocks
are disabled.

It's unrelated to voting for CX corners required by certain clock
frequencies (we handle those in the OPP tables of the consumers).
And it's also unrelated to ensuring rentention of register contents
since we actually release all votes when the clocks are idle.

So while adding runtime PM to all the clock drivers sounds nice, I'm
a bit confused what problem we're actually solving with this patch. :)

Thanks,
Stephan

Re: [PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM

[Konrad Dybcio]

On 18.07.2023 14:07, Stephan Gerhold wrote:
> On Mon, Jul 17, 2023 at 09:02:29PM -0700, Bjorn Andersson wrote:
>> On Mon, Jul 17, 2023 at 06:26:35PM +0200, Stephan Gerhold wrote:
>>> On Mon, Jul 17, 2023 at 05:19:11PM +0200, Konrad Dybcio wrote:
>>>> The GCC block on SM6375 is powered by the VDD_CX rail. We need to ensure
>>>> that it's enabled to prevent unwanted power collapse.
>>>>
>>>> Enable runtime PM to keep the power flowing only when necessary.
>>>>
>>>
>>> Are you sure this is necessary? If VDD_CX was really possible to fully
>>> "power collapse" then I would expect that you lose all register
>>> settings. This is not something we want or can even handle for GCC.
>>> You would need to restore all frequency settings, branch bits etc etc.
>>>
>>
>> This differ between platforms, some allow us to completely power down CX
>> while keeping registers state using MX, others require that CX stays in
>> retention at least.
>>
>> So, CX isn't the only rail powering GCC. For the most part though, we
>> have a relationship between frequencies votes for by clients and the
>> corner of CX, and hence I think the current description is ok...
>>
> 
> This patch is just about sending enable/disable votes for the power
> domains though, based on runtime PM which triggers when all the clocks
> are disabled.
> 
> It's unrelated to voting for CX corners required by certain clock
> frequencies (we handle those in the OPP tables of the consumers).
> And it's also unrelated to ensuring rentention of register contents
> since we actually release all votes when the clocks are idle.
> 
> So while adding runtime PM to all the clock drivers sounds nice, I'm
> a bit confused what problem we're actually solving with this patch. :)
In a very specific and unfortunate situation, there could be no other
CX votes, and trying to access (perhaps at least parts of) GCC would
result in a failure.

Konrad

Re: [PATCH 04/15] clk: qcom: gcc-sm6375: Add runtime PM

[Johan Hovold]

On Mon, Jul 17, 2023 at 05:19:11PM +0200, Konrad Dybcio wrote:
> The GCC block on SM6375 is powered by the VDD_CX rail. We need to ensure
> that it's enabled to prevent unwanted power collapse.

This bit is not correct either (and similar throughout the series).

> Enable runtime PM to keep the power flowing only when necessary.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---

> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
>  	regmap = qcom_cc_map(pdev, &gcc_sm6375_desc);
> -	if (IS_ERR(regmap))
> +	if (IS_ERR(regmap)) {
> +		pm_runtime_put(&pdev->dev);
>  		return PTR_ERR(regmap);
> +	}
>  
>  	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks));
>  	if (ret)

Looks like you forgot to update this error path.

> @@ -3817,7 +3828,10 @@ static int gcc_sm6375_probe(struct platform_device *pdev)
>  	clk_lucid_pll_configure(&gpll8, regmap, &gpll8_config);
>  	clk_zonda_pll_configure(&gpll9, regmap, &gpll9_config);
>  
> -	return qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
> +	ret = qcom_cc_really_probe(pdev, &gcc_sm6375_desc, regmap);
> +	pm_runtime_put(&pdev->dev);
> +
> +	return ret;

Johan

[PATCH 05/15] clk: qcom: gpucc-sm6375: Unregister critical clocks

[Konrad Dybcio]

Some clocks need to be always-on, but we don't really do anything
with them, other than calling enable() once and telling Linux they're
enabled.

Unregister them to save a couple of bytes and, perhaps more
importantly, allow for runtime suspend of the clock controller device,
as CLK_IS_CRITICAL prevents the latter.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm6375.c   | 105 ++++++++++++++++++++++++++++++++++------
 drivers/clk/qcom/gpucc-sm6375.c |  38 +++------------
 2 files changed, 97 insertions(+), 46 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm6375.c b/drivers/clk/qcom/gcc-sm6375.c
index 4b2de545d3f8..a8eb7a47e284 100644
--- a/drivers/clk/qcom/gcc-sm6375.c
+++ b/drivers/clk/qcom/gcc-sm6375.c
@@ -1743,6 +1743,21 @@ static struct clk_branch gcc_cam_throttle_rt_clk = {
 	},
 };
 
+static struct clk_branch gcc_camera_ahb_clk = {
+	.halt_reg = 0x17008,
+	.halt_check = BRANCH_HALT_DELAY,
+	.hwcg_reg = 0x17008,
+	.hwcg_bit = 1,
+	.clkr = {
+		.enable_reg = 0x17008,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_camera_ahb_clk",
+			.ops = &clk_branch2_aon_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_camss_axi_clk = {
 	.halt_reg = 0x58044,
 	.halt_check = BRANCH_HALT,
@@ -2293,6 +2308,21 @@ static struct clk_branch gcc_cfg_noc_usb3_prim_axi_clk = {
 	},
 };
 
+static struct clk_branch gcc_disp_ahb_clk = {
+	.halt_reg = 0x1700c,
+	.halt_check = BRANCH_HALT_VOTED,
+	.hwcg_reg = 0x1700c,
+	.hwcg_bit = 1,
+	.clkr = {
+		.enable_reg = 0x1700c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_disp_ahb_clk",
+			.ops = &clk_branch2_aon_ops,
+		},
+	},
+};
+
 static struct clk_regmap_div gcc_disp_gpll0_clk_src = {
 	.reg = 0x17058,
 	.shift = 0,
@@ -2423,6 +2453,21 @@ static struct clk_branch gcc_gp3_clk = {
 	},
 };
 
+static struct clk_branch gcc_gpu_cfg_ahb_clk = {
+	.halt_reg = 0x36004,
+	.halt_check = BRANCH_HALT_VOTED,
+	.hwcg_reg = 0x36004,
+	.hwcg_bit = 1,
+	.clkr = {
+		.enable_reg = 0x36004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_gpu_cfg_ahb_clk",
+			.ops = &clk_branch2_aon_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_gpu_gpll0_clk_src = {
 	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
@@ -3046,6 +3091,26 @@ static struct clk_branch gcc_sdcc2_apps_clk = {
 	},
 };
 
+static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = {
+	.halt_reg = 0x2b06c,
+	.halt_check = BRANCH_HALT_VOTED,
+	.hwcg_reg = 0x2b06c,
+	.hwcg_bit = 1,
+	.clkr = {
+		.enable_reg = 0x79004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_sys_noc_cpuss_ahb_clk",
+			.parent_hws = (const struct clk_hw*[]) {
+				&gcc_cpuss_ahb_postdiv_clk_src.clkr.hw,
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_aon_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_sys_noc_ufs_phy_axi_clk = {
 	.halt_reg = 0x45098,
 	.halt_check = BRANCH_HALT,
@@ -3365,6 +3430,21 @@ static struct clk_branch gcc_venus_ctl_axi_clk = {
 	},
 };
 
+static struct clk_branch gcc_video_ahb_clk = {
+	.halt_reg = 0x17004,
+	.halt_check = BRANCH_HALT_DELAY,
+	.hwcg_reg = 0x17004,
+	.hwcg_bit = 1,
+	.clkr = {
+		.enable_reg = 0x17004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_video_ahb_clk",
+			.ops = &clk_branch2_aon_ops,
+		},
+	},
+};
+
 static struct clk_branch gcc_video_axi0_clk = {
 	.halt_reg = 0x1701c,
 	.halt_check = BRANCH_HALT_VOTED,
@@ -3531,6 +3611,7 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr,
 	[GCC_CAM_THROTTLE_NRT_CLK] = &gcc_cam_throttle_nrt_clk.clkr,
 	[GCC_CAM_THROTTLE_RT_CLK] = &gcc_cam_throttle_rt_clk.clkr,
+	[GCC_CAMERA_AHB_CLK] = &gcc_camera_ahb_clk.clkr,
 	[GCC_CAMSS_AXI_CLK] = &gcc_camss_axi_clk.clkr,
 	[GCC_CAMSS_AXI_CLK_SRC] = &gcc_camss_axi_clk_src.clkr,
 	[GCC_CAMSS_CCI_0_CLK] = &gcc_camss_cci_0_clk.clkr,
@@ -3586,6 +3667,7 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_CFG_NOC_USB3_PRIM_AXI_CLK] = &gcc_cfg_noc_usb3_prim_axi_clk.clkr,
 	[GCC_CPUSS_AHB_CLK_SRC] = &gcc_cpuss_ahb_clk_src.clkr,
 	[GCC_CPUSS_AHB_POSTDIV_CLK_SRC] = &gcc_cpuss_ahb_postdiv_clk_src.clkr,
+	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,
 	[GCC_DISP_GPLL0_CLK_SRC] = &gcc_disp_gpll0_clk_src.clkr,
 	[GCC_DISP_GPLL0_DIV_CLK_SRC] = &gcc_disp_gpll0_div_clk_src.clkr,
 	[GCC_DISP_HF_AXI_CLK] = &gcc_disp_hf_axi_clk.clkr,
@@ -3597,6 +3679,7 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_GP2_CLK_SRC] = &gcc_gp2_clk_src.clkr,
 	[GCC_GP3_CLK] = &gcc_gp3_clk.clkr,
 	[GCC_GP3_CLK_SRC] = &gcc_gp3_clk_src.clkr,
+	[GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
 	[GCC_GPU_GPLL0_CLK_SRC] = &gcc_gpu_gpll0_clk_src.clkr,
 	[GCC_GPU_GPLL0_DIV_CLK_SRC] = &gcc_gpu_gpll0_div_clk_src.clkr,
 	[GCC_GPU_MEMNOC_GFX_CLK] = &gcc_gpu_memnoc_gfx_clk.clkr,
@@ -3652,6 +3735,7 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_SDCC2_AHB_CLK] = &gcc_sdcc2_ahb_clk.clkr,
 	[GCC_SDCC2_APPS_CLK] = &gcc_sdcc2_apps_clk.clkr,
 	[GCC_SDCC2_APPS_CLK_SRC] = &gcc_sdcc2_apps_clk_src.clkr,
+	[GCC_SYS_NOC_CPUSS_AHB_CLK] = &gcc_sys_noc_cpuss_ahb_clk.clkr,
 	[GCC_SYS_NOC_UFS_PHY_AXI_CLK] = &gcc_sys_noc_ufs_phy_axi_clk.clkr,
 	[GCC_SYS_NOC_USB3_PRIM_AXI_CLK] = &gcc_sys_noc_usb3_prim_axi_clk.clkr,
 	[GCC_UFS_PHY_AHB_CLK] = &gcc_ufs_phy_ahb_clk.clkr,
@@ -3678,6 +3762,7 @@ static struct clk_regmap *gcc_sm6375_clocks[] = {
 	[GCC_VCODEC0_AXI_CLK] = &gcc_vcodec0_axi_clk.clkr,
 	[GCC_VENUS_AHB_CLK] = &gcc_venus_ahb_clk.clkr,
 	[GCC_VENUS_CTL_AXI_CLK] = &gcc_venus_ctl_axi_clk.clkr,
+	[GCC_VIDEO_AHB_CLK] = &gcc_video_ahb_clk.clkr,
 	[GCC_VIDEO_AXI0_CLK] = &gcc_video_axi0_clk.clkr,
 	[GCC_VIDEO_THROTTLE_CORE_CLK] = &gcc_video_throttle_core_clk.clkr,
 	[GCC_VIDEO_VCODEC0_SYS_CLK] = &gcc_video_vcodec0_sys_clk.clkr,
@@ -3805,23 +3890,11 @@ static int gcc_sm6375_probe(struct platform_device *pdev)
 
 	/*
 	 * Keep the following clocks always on:
-	 * GCC_CAMERA_XO_CLK
-	 * GCC_CPUSS_GNOC_CLK
-	 * GCC_DISP_XO_CLK
-	 * GCC_CAMERA_AHB_CLK
-	 * GCC_DISP_AHB_CLK
-	 * GCC_GPU_CFG_AHB_CLK
-	 * GCC_SYS_NOC_CPUSS_AHB_CLK
-	 * GCC_VIDEO_AHB_CLK
+	 * GCC_CAMERA_XO_CLK, GCC_CPUSS_GNOC_CLK, GCC_DISP_XO_CLK
 	 */
-	qcom_branch_set_clk_en(regmap, 0x17028);
-	qcom_branch_set_clk_en(regmap, 0x2b004);
-	qcom_branch_set_clk_en(regmap, 0x1702c);
-	qcom_branch_set_clk_en(regmap, 0x17008);
-	qcom_branch_set_clk_en(regmap, 0x1700c);
-	qcom_branch_set_clk_en(regmap, 0x36004);
-	qcom_branch_set_clk_en(regmap, 0x2b06c);
-	qcom_branch_set_clk_en(regmap, 0x17004);
+	regmap_update_bits(regmap, 0x17028, BIT(0), BIT(0));
+	regmap_update_bits(regmap, 0x2b004, BIT(0), BIT(0));
+	regmap_update_bits(regmap, 0x1702c, BIT(0), BIT(0));
 
 	clk_lucid_pll_configure(&gpll10, regmap, &gpll10_config);
 	clk_lucid_pll_configure(&gpll11, regmap, &gpll11_config);
diff --git a/drivers/clk/qcom/gpucc-sm6375.c b/drivers/clk/qcom/gpucc-sm6375.c
index 2d863dc3d83b..d70c6ed0440b 100644
--- a/drivers/clk/qcom/gpucc-sm6375.c
+++ b/drivers/clk/qcom/gpucc-sm6375.c
@@ -182,20 +182,6 @@ static struct clk_rcg2 gpucc_gx_gfx3d_clk_src = {
 	},
 };
 
-static struct clk_branch gpucc_ahb_clk = {
-	.halt_reg = 0x1078,
-	.halt_check = BRANCH_HALT_DELAY,
-	.clkr = {
-		.enable_reg = 0x1078,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gpucc_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gpucc_cx_gfx3d_clk = {
 	.halt_reg = 0x10a4,
 	.halt_check = BRANCH_HALT_DELAY,
@@ -293,20 +279,6 @@ static struct clk_branch gpucc_cxo_clk = {
 	},
 };
 
-static struct clk_branch gpucc_gx_cxo_clk = {
-	.halt_reg = 0x1060,
-	.halt_check = BRANCH_HALT_DELAY,
-	.clkr = {
-		.enable_reg = 0x1060,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gpucc_gx_cxo_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gpucc_gx_gfx3d_clk = {
 	.halt_reg = 0x1054,
 	.halt_check = BRANCH_HALT_DELAY,
@@ -380,7 +352,6 @@ static struct gdsc gpu_gx_gdsc = {
 };
 
 static struct clk_regmap *gpucc_sm6375_clocks[] = {
-	[GPU_CC_AHB_CLK] = &gpucc_ahb_clk.clkr,
 	[GPU_CC_CX_GFX3D_CLK] = &gpucc_cx_gfx3d_clk.clkr,
 	[GPU_CC_CX_GFX3D_SLV_CLK] = &gpucc_cx_gfx3d_slv_clk.clkr,
 	[GPU_CC_CX_GMU_CLK] = &gpucc_cx_gmu_clk.clkr,
@@ -388,7 +359,6 @@ static struct clk_regmap *gpucc_sm6375_clocks[] = {
 	[GPU_CC_CXO_AON_CLK] = &gpucc_cxo_aon_clk.clkr,
 	[GPU_CC_CXO_CLK] = &gpucc_cxo_clk.clkr,
 	[GPU_CC_GMU_CLK_SRC] = &gpucc_gmu_clk_src.clkr,
-	[GPU_CC_GX_CXO_CLK] = &gpucc_gx_cxo_clk.clkr,
 	[GPU_CC_GX_GFX3D_CLK] = &gpucc_gx_gfx3d_clk.clkr,
 	[GPU_CC_GX_GFX3D_CLK_SRC] = &gpucc_gx_gfx3d_clk_src.clkr,
 	[GPU_CC_GX_GMU_CLK] = &gpucc_gx_gmu_clk.clkr,
@@ -454,6 +424,14 @@ static int gpucc_sm6375_probe(struct platform_device *pdev)
 	clk_lucid_pll_configure(&gpucc_pll0, regmap, &gpucc_pll0_config);
 	clk_lucid_pll_configure(&gpucc_pll1, regmap, &gpucc_pll1_config);
 
+	/*
+	 * Keep clocks always enabled:
+	 *	gpucc_ahb_clk
+	 *	gpucc_gx_cxo_clk
+	 */
+	qcom_branch_set_clk_en(regmap, 0x1078);
+	qcom_branch_set_clk_en(regmap, 0x1060);
+
 	ret = qcom_cc_really_probe(pdev, &gpucc_sm6375_desc, regmap);
 	pm_runtime_put(&pdev->dev);
 

-- 
2.41.0

Re: [PATCH 05/15] clk: qcom: gpucc-sm6375: Unregister critical clocks

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 05:19:12PM +0200, Konrad Dybcio wrote:
> Some clocks need to be always-on, but we don't really do anything
> with them, other than calling enable() once and telling Linux they're
> enabled.
> 
> Unregister them to save a couple of bytes and, perhaps more
> importantly, allow for runtime suspend of the clock controller device,
> as CLK_IS_CRITICAL prevents the latter.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/clk/qcom/gcc-sm6375.c   | 105 ++++++++++++++++++++++++++++++++++------
>  drivers/clk/qcom/gpucc-sm6375.c |  38 +++------------
>  2 files changed, 97 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sm6375.c b/drivers/clk/qcom/gcc-sm6375.c
> index 4b2de545d3f8..a8eb7a47e284 100644
> --- a/drivers/clk/qcom/gcc-sm6375.c
> +++ b/drivers/clk/qcom/gcc-sm6375.c
> @@ -1743,6 +1743,21 @@ static struct clk_branch gcc_cam_throttle_rt_clk = {
>  	},
>  };
>  
> +static struct clk_branch gcc_camera_ahb_clk = {
> +	.halt_reg = 0x17008,
> +	.halt_check = BRANCH_HALT_DELAY,
> +	.hwcg_reg = 0x17008,
> +	.hwcg_bit = 1,
> +	.clkr = {
> +		.enable_reg = 0x17008,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gcc_camera_ahb_clk",
> +			.ops = &clk_branch2_aon_ops,
> +		},
> +	},
> +};
> +

You seem to revert PATCH 03/15 here.

Stephan

Re: [PATCH 05/15] clk: qcom: gpucc-sm6375: Unregister critical clocks

[Konrad Dybcio]

On 17.07.2023 18:15, Stephan Gerhold wrote:
> On Mon, Jul 17, 2023 at 05:19:12PM +0200, Konrad Dybcio wrote:
>> Some clocks need to be always-on, but we don't really do anything
>> with them, other than calling enable() once and telling Linux they're
>> enabled.
>>
>> Unregister them to save a couple of bytes and, perhaps more
>> importantly, allow for runtime suspend of the clock controller device,
>> as CLK_IS_CRITICAL prevents the latter.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---

> You seem to revert PATCH 03/15 here.
> 
Argh, I had an iffy conflict resolution that I thought I had gotten
rid of.. but looks like in the end my brain farted anyway..

Thanks for spotting that!

Konrad

[PATCH 06/15] clk: qcom: gpucc-sm6115: Unregister critical clocks

[Konrad Dybcio]

Some clocks need to be always-on, but we don't really do anything
with them, other than calling enable() once and telling Linux they're
enabled.

Unregister them to save a couple of bytes and, perhaps more
importantly, allow for runtime suspend of the clock controller device,
as CLK_IS_CRITICAL prevents the latter.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gpucc-sm6115.c | 38 ++++++++------------------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/drivers/clk/qcom/gpucc-sm6115.c b/drivers/clk/qcom/gpucc-sm6115.c
index c84727e8352d..ac048f7973d0 100644
--- a/drivers/clk/qcom/gpucc-sm6115.c
+++ b/drivers/clk/qcom/gpucc-sm6115.c
@@ -233,20 +233,6 @@ static struct clk_rcg2 gpu_cc_gx_gfx3d_clk_src = {
 	},
 };
 
-static struct clk_branch gpu_cc_ahb_clk = {
-	.halt_reg = 0x1078,
-	.halt_check = BRANCH_HALT_DELAY,
-	.clkr = {
-		.enable_reg = 0x1078,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gpu_cc_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gpu_cc_crc_ahb_clk = {
 	.halt_reg = 0x107c,
 	.halt_check = BRANCH_HALT_DELAY,
@@ -335,20 +321,6 @@ static struct clk_branch gpu_cc_cxo_clk = {
 	},
 };
 
-static struct clk_branch gpu_cc_gx_cxo_clk = {
-	.halt_reg = 0x1060,
-	.halt_check = BRANCH_HALT_DELAY,
-	.clkr = {
-		.enable_reg = 0x1060,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gpu_cc_gx_cxo_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gpu_cc_gx_gfx3d_clk = {
 	.halt_reg = 0x1054,
 	.halt_check = BRANCH_HALT_SKIP,
@@ -417,7 +389,6 @@ static struct gdsc gpu_gx_gdsc = {
 };
 
 static struct clk_regmap *gpu_cc_sm6115_clocks[] = {
-	[GPU_CC_AHB_CLK] = &gpu_cc_ahb_clk.clkr,
 	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,
 	[GPU_CC_CX_GFX3D_CLK] = &gpu_cc_cx_gfx3d_clk.clkr,
 	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
@@ -425,7 +396,6 @@ static struct clk_regmap *gpu_cc_sm6115_clocks[] = {
 	[GPU_CC_CXO_AON_CLK] = &gpu_cc_cxo_aon_clk.clkr,
 	[GPU_CC_CXO_CLK] = &gpu_cc_cxo_clk.clkr,
 	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
-	[GPU_CC_GX_CXO_CLK] = &gpu_cc_gx_cxo_clk.clkr,
 	[GPU_CC_GX_GFX3D_CLK] = &gpu_cc_gx_gfx3d_clk.clkr,
 	[GPU_CC_GX_GFX3D_CLK_SRC] = &gpu_cc_gx_gfx3d_clk_src.clkr,
 	[GPU_CC_PLL0] = &gpu_cc_pll0.clkr,
@@ -487,6 +457,14 @@ static int gpu_cc_sm6115_probe(struct platform_device *pdev)
 	qcom_branch_set_force_mem_core(regmap, gpu_cc_gx_gfx3d_clk, true);
 	qcom_branch_set_force_periph_on(regmap, gpu_cc_gx_gfx3d_clk, true);
 
+	/*
+	 * Keep clocks always enabled:
+	 *      gpu_cc_ahb_clk
+	 *      gpu_cc_gx_cxo_clk
+	 */
+	qcom_branch_set_clk_en(regmap, 0x1078);
+	qcom_branch_set_clk_en(regmap, 0x1060);
+
 	return qcom_cc_really_probe(pdev, &gpu_cc_sm6115_desc, regmap);
 }
 

-- 
2.41.0

[PATCH 07/15] clk: qcom: gpucc-sm6115: Add runtime PM

[Konrad Dybcio]

The GPU_CC block on SM6115 is powered by the VDD_CX rail. We need to
ensure that it's enabled to prevent unwanted power collapse.

Enable runtime PM to keep the power flowing only when necessary.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gpucc-sm6115.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gpucc-sm6115.c b/drivers/clk/qcom/gpucc-sm6115.c
index ac048f7973d0..6fb84492d292 100644
--- a/drivers/clk/qcom/gpucc-sm6115.c
+++ b/drivers/clk/qcom/gpucc-sm6115.c
@@ -7,6 +7,7 @@
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,sm6115-gpucc.h>
@@ -442,10 +443,21 @@ MODULE_DEVICE_TABLE(of, gpu_cc_sm6115_match_table);
 static int gpu_cc_sm6115_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
+	int ret;
+
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
 
 	regmap = qcom_cc_map(pdev, &gpu_cc_sm6115_desc);
-	if (IS_ERR(regmap))
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
+	}
 
 	clk_alpha_pll_configure(&gpu_cc_pll0, regmap, &gpu_cc_pll0_config);
 	clk_alpha_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll1_config);
@@ -465,7 +477,10 @@ static int gpu_cc_sm6115_probe(struct platform_device *pdev)
 	qcom_branch_set_clk_en(regmap, 0x1078);
 	qcom_branch_set_clk_en(regmap, 0x1060);
 
-	return qcom_cc_really_probe(pdev, &gpu_cc_sm6115_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &gpu_cc_sm6115_desc, regmap);
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
 }
 
 static struct platform_driver gpu_cc_sm6115_driver = {

-- 
2.41.0

Re: [PATCH 07/15] clk: qcom: gpucc-sm6115: Add runtime PM

[Johan Hovold]

On Mon, Jul 17, 2023 at 05:19:14PM +0200, Konrad Dybcio wrote:
> The GPU_CC block on SM6115 is powered by the VDD_CX rail. We need to
> ensure that it's enabled to prevent unwanted power collapse.

This bit is not correct, the power domain would not have been disabled
until you enable runtime PM as part of this very patch.

I noticed similar claims have incorrectly been made in the past, for
example, in the recent:

	2a541abd9837 clk: qcom: gcc-sc8280xp: Add runtime PM

and older:

	a91c483b42fa ("clk: qcom: videocc-sm8250: use runtime PM for the clock controller")

Johan

Re: [PATCH 07/15] clk: qcom: gpucc-sm6115: Add runtime PM

[Konrad Dybcio]

On 18.07.2023 15:24, Johan Hovold wrote:
> On Mon, Jul 17, 2023 at 05:19:14PM +0200, Konrad Dybcio wrote:
>> The GPU_CC block on SM6115 is powered by the VDD_CX rail. We need to
>> ensure that it's enabled to prevent unwanted power collapse.
> 
> This bit is not correct, the power domain would not have been disabled
> until you enable runtime PM as part of this very patch.
Right, this was a bit of a thought-jump. The part that ensures there's
any vote at all is actually the DT commit adding a reference to the
genpd.

Konrad

Re: [PATCH 07/15] clk: qcom: gpucc-sm6115: Add runtime PM

[Konrad Dybcio]

On 18.07.2023 15:28, Konrad Dybcio wrote:
> On 18.07.2023 15:24, Johan Hovold wrote:
>> On Mon, Jul 17, 2023 at 05:19:14PM +0200, Konrad Dybcio wrote:
>>> The GPU_CC block on SM6115 is powered by the VDD_CX rail. We need to
>>> ensure that it's enabled to prevent unwanted power collapse.
>>
>> This bit is not correct, the power domain would not have been disabled
>> until you enable runtime PM as part of this very patch.
> Right, this was a bit of a thought-jump. The part that ensures there's
> any vote at all is actually the DT commit adding a reference to the
> genpd.
Well I read it again and I think my original intention was "it = the power
domain" and not "it = runtime PM", it makes sense that way..

Anyway, I'll rephrase that to make it less ambiguous.

Konrad

[PATCH 08/15] clk: qcom: gcc-sm6115: Unregister critical clocks

[Konrad Dybcio]

Some clocks need to be always-on, but we don't really do anything
with them, other than calling enable() once and telling Linux they're
enabled.

Unregister them to save a couple of bytes and, perhaps more
importantly, allow for runtime suspend of the clock controller device,
as CLK_IS_CRITICAL prevents the latter.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm6115.c | 133 ++++++------------------------------------
 1 file changed, 18 insertions(+), 115 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm6115.c b/drivers/clk/qcom/gcc-sm6115.c
index 033e308ff865..1b6016e7ddeb 100644
--- a/drivers/clk/qcom/gcc-sm6115.c
+++ b/drivers/clk/qcom/gcc-sm6115.c
@@ -1585,36 +1585,6 @@ static struct clk_branch gcc_cam_throttle_rt_clk = {
 	},
 };
 
-static struct clk_branch gcc_camera_ahb_clk = {
-	.halt_reg = 0x17008,
-	.halt_check = BRANCH_HALT_DELAY,
-	.hwcg_reg = 0x17008,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x17008,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_camera_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
-static struct clk_branch gcc_camera_xo_clk = {
-	.halt_reg = 0x17028,
-	.halt_check = BRANCH_HALT,
-	.clkr = {
-		.enable_reg = 0x17028,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_camera_xo_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_camss_axi_clk = {
 	.halt_reg = 0x58044,
 	.halt_check = BRANCH_HALT,
@@ -2123,38 +2093,6 @@ static struct clk_branch gcc_cfg_noc_usb3_prim_axi_clk = {
 	},
 };
 
-static struct clk_branch gcc_cpuss_gnoc_clk = {
-	.halt_reg = 0x2b004,
-	.halt_check = BRANCH_HALT_VOTED,
-	.hwcg_reg = 0x2b004,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x79004,
-		.enable_mask = BIT(22),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_cpuss_gnoc_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
-static struct clk_branch gcc_disp_ahb_clk = {
-	.halt_reg = 0x1700c,
-	.halt_check = BRANCH_HALT,
-	.hwcg_reg = 0x1700c,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x1700c,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_disp_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_regmap_div gcc_disp_gpll0_clk_src = {
 	.reg = 0x17058,
 	.shift = 0,
@@ -2214,20 +2152,6 @@ static struct clk_branch gcc_disp_throttle_core_clk = {
 	},
 };
 
-static struct clk_branch gcc_disp_xo_clk = {
-	.halt_reg = 0x1702c,
-	.halt_check = BRANCH_HALT,
-	.clkr = {
-		.enable_reg = 0x1702c,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_disp_xo_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_gp1_clk = {
 	.halt_reg = 0x4d000,
 	.halt_check = BRANCH_HALT,
@@ -2282,22 +2206,6 @@ static struct clk_branch gcc_gp3_clk = {
 	},
 };
 
-static struct clk_branch gcc_gpu_cfg_ahb_clk = {
-	.halt_reg = 0x36004,
-	.halt_check = BRANCH_HALT,
-	.hwcg_reg = 0x36004,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x36004,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_gpu_cfg_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_gpu_gpll0_clk_src = {
 	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
@@ -2770,22 +2678,6 @@ static struct clk_branch gcc_sdcc2_apps_clk = {
 	},
 };
 
-static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = {
-	.halt_reg = 0x2b06c,
-	.halt_check = BRANCH_HALT_VOTED,
-	.hwcg_reg = 0x2b06c,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x79004,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_sys_noc_cpuss_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_sys_noc_ufs_phy_axi_clk = {
 	.halt_reg = 0x45098,
 	.halt_check = BRANCH_HALT,
@@ -3271,8 +3163,6 @@ static struct clk_regmap *gcc_sm6115_clocks[] = {
 	[GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr,
 	[GCC_CAM_THROTTLE_NRT_CLK] = &gcc_cam_throttle_nrt_clk.clkr,
 	[GCC_CAM_THROTTLE_RT_CLK] = &gcc_cam_throttle_rt_clk.clkr,
-	[GCC_CAMERA_AHB_CLK] = &gcc_camera_ahb_clk.clkr,
-	[GCC_CAMERA_XO_CLK] = &gcc_camera_xo_clk.clkr,
 	[GCC_CAMSS_AXI_CLK] = &gcc_camss_axi_clk.clkr,
 	[GCC_CAMSS_AXI_CLK_SRC] = &gcc_camss_axi_clk_src.clkr,
 	[GCC_CAMSS_CAMNOC_ATB_CLK] = &gcc_camss_camnoc_atb_clk.clkr,
@@ -3321,20 +3211,16 @@ static struct clk_regmap *gcc_sm6115_clocks[] = {
 	[GCC_CAMSS_TOP_AHB_CLK] = &gcc_camss_top_ahb_clk.clkr,
 	[GCC_CAMSS_TOP_AHB_CLK_SRC] = &gcc_camss_top_ahb_clk_src.clkr,
 	[GCC_CFG_NOC_USB3_PRIM_AXI_CLK] = &gcc_cfg_noc_usb3_prim_axi_clk.clkr,
-	[GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr,
-	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,
 	[GCC_DISP_GPLL0_CLK_SRC] = &gcc_disp_gpll0_clk_src.clkr,
 	[GCC_DISP_GPLL0_DIV_CLK_SRC] = &gcc_disp_gpll0_div_clk_src.clkr,
 	[GCC_DISP_HF_AXI_CLK] = &gcc_disp_hf_axi_clk.clkr,
 	[GCC_DISP_THROTTLE_CORE_CLK] = &gcc_disp_throttle_core_clk.clkr,
-	[GCC_DISP_XO_CLK] = &gcc_disp_xo_clk.clkr,
 	[GCC_GP1_CLK] = &gcc_gp1_clk.clkr,
 	[GCC_GP1_CLK_SRC] = &gcc_gp1_clk_src.clkr,
 	[GCC_GP2_CLK] = &gcc_gp2_clk.clkr,
 	[GCC_GP2_CLK_SRC] = &gcc_gp2_clk_src.clkr,
 	[GCC_GP3_CLK] = &gcc_gp3_clk.clkr,
 	[GCC_GP3_CLK_SRC] = &gcc_gp3_clk_src.clkr,
-	[GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
 	[GCC_GPU_GPLL0_CLK_SRC] = &gcc_gpu_gpll0_clk_src.clkr,
 	[GCC_GPU_GPLL0_DIV_CLK_SRC] = &gcc_gpu_gpll0_div_clk_src.clkr,
 	[GCC_GPU_IREF_CLK] = &gcc_gpu_iref_clk.clkr,
@@ -3375,7 +3261,6 @@ static struct clk_regmap *gcc_sm6115_clocks[] = {
 	[GCC_SDCC2_AHB_CLK] = &gcc_sdcc2_ahb_clk.clkr,
 	[GCC_SDCC2_APPS_CLK] = &gcc_sdcc2_apps_clk.clkr,
 	[GCC_SDCC2_APPS_CLK_SRC] = &gcc_sdcc2_apps_clk_src.clkr,
-	[GCC_SYS_NOC_CPUSS_AHB_CLK] = &gcc_sys_noc_cpuss_ahb_clk.clkr,
 	[GCC_SYS_NOC_UFS_PHY_AXI_CLK] = &gcc_sys_noc_ufs_phy_axi_clk.clkr,
 	[GCC_SYS_NOC_USB3_PRIM_AXI_CLK] = &gcc_sys_noc_usb3_prim_axi_clk.clkr,
 	[GCC_UFS_CLKREF_CLK] = &gcc_ufs_clkref_clk.clkr,
@@ -3512,6 +3397,24 @@ static int gcc_sm6115_probe(struct platform_device *pdev)
 	clk_alpha_pll_configure(&gpll10, regmap, &gpll10_config);
 	clk_alpha_pll_configure(&gpll11, regmap, &gpll11_config);
 
+	/*
+	 * Keep the following clocks always on:
+	 * GCC_CAMERA_AHB_CLK
+	 * GCC_CAMERA_XO_CLK
+	 * GCC_CPUSS_GNOC_CLK
+	 * GCC_DISP_AHB_CLK
+	 * GCC_DISP_XO_CLK
+	 * GCC_GPU_CFG_AHB_CLK
+	 * GCC_SYS_NOC_CPUSS_AHB_CLK
+	 */
+	qcom_branch_set_clk_en(regmap, 0x17008);
+	qcom_branch_set_clk_en(regmap, 0x17028);
+	qcom_branch_set_clk_en(regmap, 0x2b004);
+	qcom_branch_set_clk_en(regmap, 0x1700c);
+	qcom_branch_set_clk_en(regmap, 0x1702c);
+	qcom_branch_set_clk_en(regmap, 0x36004);
+	qcom_branch_set_clk_en(regmap, 0x2b06c);
+
 	return qcom_cc_really_probe(pdev, &gcc_sm6115_desc, regmap);
 }
 

-- 
2.41.0

[PATCH 09/15] clk: qcom: gcc-sm6115: Add runtime PM

[Konrad Dybcio]

The GCC block on SM6115 is mostly powered by the VDD_CX rail. We need
to ensure that it's enabled to prevent unwanted power collapse.

Enable runtime PM to keep the power flowing only when necessary.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm6115.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm6115.c b/drivers/clk/qcom/gcc-sm6115.c
index 1b6016e7ddeb..7f1e278c63c0 100644
--- a/drivers/clk/qcom/gcc-sm6115.c
+++ b/drivers/clk/qcom/gcc-sm6115.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/clk-provider.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reset-controller.h>
 
@@ -3383,14 +3384,26 @@ static int gcc_sm6115_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	int ret;
 
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
+
 	regmap = qcom_cc_map(pdev, &gcc_sm6115_desc);
-	if (IS_ERR(regmap))
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
+	}
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
 			ARRAY_SIZE(gcc_dfs_clocks));
-	if (ret)
+	if (ret) {
+		pm_runtime_put(&pdev->dev);
 		return ret;
+	}
 
 	clk_alpha_pll_configure(&gpll8, regmap, &gpll8_config);
 	clk_alpha_pll_configure(&gpll9, regmap, &gpll9_config);
@@ -3415,7 +3428,10 @@ static int gcc_sm6115_probe(struct platform_device *pdev)
 	qcom_branch_set_clk_en(regmap, 0x36004);
 	qcom_branch_set_clk_en(regmap, 0x2b06c);
 
-	return qcom_cc_really_probe(pdev, &gcc_sm6115_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &gcc_sm6115_desc, regmap);
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
 }
 
 static struct platform_driver gcc_sm6115_driver = {

-- 
2.41.0

[PATCH 10/15] clk: qcom: gcc-qcm2290: Unregister critical clocks

[Konrad Dybcio]

Some clocks need to be always-on, but we don't really do anything
with them, other than calling enable() once and telling Linux they're
enabled.

Unregister them to save a couple of bytes and, perhaps more
importantly, allow for runtime suspend of the clock controller device,
as CLK_IS_CRITICAL prevents the latter.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-qcm2290.c | 114 ++++++-----------------------------------
 1 file changed, 16 insertions(+), 98 deletions(-)

diff --git a/drivers/clk/qcom/gcc-qcm2290.c b/drivers/clk/qcom/gcc-qcm2290.c
index 48995e50c6bd..1a8acc2de921 100644
--- a/drivers/clk/qcom/gcc-qcm2290.c
+++ b/drivers/clk/qcom/gcc-qcm2290.c
@@ -1397,36 +1397,6 @@ static struct clk_branch gcc_cam_throttle_rt_clk = {
 	},
 };
 
-static struct clk_branch gcc_camera_ahb_clk = {
-	.halt_reg = 0x17008,
-	.halt_check = BRANCH_HALT_DELAY,
-	.hwcg_reg = 0x17008,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x17008,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_camera_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
-static struct clk_branch gcc_camera_xo_clk = {
-	.halt_reg = 0x17028,
-	.halt_check = BRANCH_HALT,
-	.clkr = {
-		.enable_reg = 0x17028,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_camera_xo_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_camss_axi_clk = {
 	.halt_reg = 0x58044,
 	.halt_check = BRANCH_HALT,
@@ -1825,22 +1795,6 @@ static struct clk_branch gcc_cfg_noc_usb3_prim_axi_clk = {
 	},
 };
 
-static struct clk_branch gcc_disp_ahb_clk = {
-	.halt_reg = 0x1700c,
-	.halt_check = BRANCH_HALT,
-	.hwcg_reg = 0x1700c,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x1700c,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_disp_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_regmap_div gcc_disp_gpll0_clk_src = {
 	.reg = 0x17058,
 	.shift = 0,
@@ -1899,20 +1853,6 @@ static struct clk_branch gcc_disp_throttle_core_clk = {
 	},
 };
 
-static struct clk_branch gcc_disp_xo_clk = {
-	.halt_reg = 0x1702c,
-	.halt_check = BRANCH_HALT,
-	.clkr = {
-		.enable_reg = 0x1702c,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_disp_xo_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_gp1_clk = {
 	.halt_reg = 0x4d000,
 	.halt_check = BRANCH_HALT,
@@ -1964,22 +1904,6 @@ static struct clk_branch gcc_gp3_clk = {
 	},
 };
 
-static struct clk_branch gcc_gpu_cfg_ahb_clk = {
-	.halt_reg = 0x36004,
-	.halt_check = BRANCH_HALT,
-	.hwcg_reg = 0x36004,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x36004,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_gpu_cfg_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_gpu_gpll0_clk_src = {
 	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
@@ -2439,22 +2363,6 @@ static struct clk_branch gcc_sdcc2_apps_clk = {
 	},
 };
 
-static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = {
-	.halt_reg = 0x2b06c,
-	.halt_check = BRANCH_HALT_VOTED,
-	.hwcg_reg = 0x2b06c,
-	.hwcg_bit = 1,
-	.clkr = {
-		.enable_reg = 0x79004,
-		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
-			.name = "gcc_sys_noc_cpuss_ahb_clk",
-			.flags = CLK_IS_CRITICAL,
-			.ops = &clk_branch2_ops,
-		},
-	},
-};
-
 static struct clk_branch gcc_sys_noc_usb3_prim_axi_clk = {
 	.halt_reg = 0x1a080,
 	.halt_check = BRANCH_HALT,
@@ -2774,8 +2682,6 @@ static struct clk_regmap *gcc_qcm2290_clocks[] = {
 	[GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr,
 	[GCC_CAM_THROTTLE_NRT_CLK] = &gcc_cam_throttle_nrt_clk.clkr,
 	[GCC_CAM_THROTTLE_RT_CLK] = &gcc_cam_throttle_rt_clk.clkr,
-	[GCC_CAMERA_AHB_CLK] = &gcc_camera_ahb_clk.clkr,
-	[GCC_CAMERA_XO_CLK] = &gcc_camera_xo_clk.clkr,
 	[GCC_CAMSS_AXI_CLK] = &gcc_camss_axi_clk.clkr,
 	[GCC_CAMSS_AXI_CLK_SRC] = &gcc_camss_axi_clk_src.clkr,
 	[GCC_CAMSS_CAMNOC_ATB_CLK] = &gcc_camss_camnoc_atb_clk.clkr,
@@ -2816,19 +2722,16 @@ static struct clk_regmap *gcc_qcm2290_clocks[] = {
 	[GCC_CAMSS_TOP_AHB_CLK] = &gcc_camss_top_ahb_clk.clkr,
 	[GCC_CAMSS_TOP_AHB_CLK_SRC] = &gcc_camss_top_ahb_clk_src.clkr,
 	[GCC_CFG_NOC_USB3_PRIM_AXI_CLK] = &gcc_cfg_noc_usb3_prim_axi_clk.clkr,
-	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,
 	[GCC_DISP_GPLL0_CLK_SRC] = &gcc_disp_gpll0_clk_src.clkr,
 	[GCC_DISP_GPLL0_DIV_CLK_SRC] = &gcc_disp_gpll0_div_clk_src.clkr,
 	[GCC_DISP_HF_AXI_CLK] = &gcc_disp_hf_axi_clk.clkr,
 	[GCC_DISP_THROTTLE_CORE_CLK] = &gcc_disp_throttle_core_clk.clkr,
-	[GCC_DISP_XO_CLK] = &gcc_disp_xo_clk.clkr,
 	[GCC_GP1_CLK] = &gcc_gp1_clk.clkr,
 	[GCC_GP1_CLK_SRC] = &gcc_gp1_clk_src.clkr,
 	[GCC_GP2_CLK] = &gcc_gp2_clk.clkr,
 	[GCC_GP2_CLK_SRC] = &gcc_gp2_clk_src.clkr,
 	[GCC_GP3_CLK] = &gcc_gp3_clk.clkr,
 	[GCC_GP3_CLK_SRC] = &gcc_gp3_clk_src.clkr,
-	[GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
 	[GCC_GPU_GPLL0_CLK_SRC] = &gcc_gpu_gpll0_clk_src.clkr,
 	[GCC_GPU_GPLL0_DIV_CLK_SRC] = &gcc_gpu_gpll0_div_clk_src.clkr,
 	[GCC_GPU_IREF_CLK] = &gcc_gpu_iref_clk.clkr,
@@ -2869,7 +2772,6 @@ static struct clk_regmap *gcc_qcm2290_clocks[] = {
 	[GCC_SDCC2_AHB_CLK] = &gcc_sdcc2_ahb_clk.clkr,
 	[GCC_SDCC2_APPS_CLK] = &gcc_sdcc2_apps_clk.clkr,
 	[GCC_SDCC2_APPS_CLK_SRC] = &gcc_sdcc2_apps_clk_src.clkr,
-	[GCC_SYS_NOC_CPUSS_AHB_CLK] = &gcc_sys_noc_cpuss_ahb_clk.clkr,
 	[GCC_SYS_NOC_USB3_PRIM_AXI_CLK] = &gcc_sys_noc_usb3_prim_axi_clk.clkr,
 	[GCC_USB30_PRIM_MASTER_CLK] = &gcc_usb30_prim_master_clk.clkr,
 	[GCC_USB30_PRIM_MASTER_CLK_SRC] = &gcc_usb30_prim_master_clk_src.clkr,
@@ -2994,6 +2896,22 @@ static int gcc_qcm2290_probe(struct platform_device *pdev)
 	clk_alpha_pll_configure(&gpll8, regmap, &gpll8_config);
 	clk_alpha_pll_configure(&gpll9, regmap, &gpll9_config);
 
+	/*
+	 * Keep clocks always enabled:
+	 * GCC_CAMERA_AHB_CLK
+	 * GCC_CAMERA_XO_CLK
+	 * GCC_DISP_AHB_CLK
+	 * GCC_DISP_XO_CLK
+	 * GCC_GPU_CFG_AHB_CLK
+	 * GCC_SYS_NOC_CPUSS_AHB_CLK
+	 */
+	qcom_branch_set_clk_en(regmap, 0x17008);
+	qcom_branch_set_clk_en(regmap, 0x17028);
+	qcom_branch_set_clk_en(regmap, 0x1700c);
+	qcom_branch_set_clk_en(regmap, 0x1702c);
+	qcom_branch_set_clk_en(regmap, 0x36004);
+	qcom_branch_set_clk_en(regmap, 0x2b06c);
+
 	return qcom_cc_really_probe(pdev, &gcc_qcm2290_desc, regmap);
 }
 

-- 
2.41.0

[PATCH 11/15] clk: qcom: gcc-qcm2290: Add runtime PM

[Konrad Dybcio]

The GCC block on QCM2290 is mostly powered by the VDD_CX rail. We need
to ensure that it's enabled to prevent unwanted power collapse.

Enable runtime PM to keep the power flowing only when necessary.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-qcm2290.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-qcm2290.c b/drivers/clk/qcom/gcc-qcm2290.c
index 1a8acc2de921..573cb550d678 100644
--- a/drivers/clk/qcom/gcc-qcm2290.c
+++ b/drivers/clk/qcom/gcc-qcm2290.c
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,gcc-qcm2290.h>
@@ -2882,14 +2883,26 @@ static int gcc_qcm2290_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	int ret;
 
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
+
 	regmap = qcom_cc_map(pdev, &gcc_qcm2290_desc);
-	if (IS_ERR(regmap))
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
+	}
 
 	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
 				       ARRAY_SIZE(gcc_dfs_clocks));
-	if (ret)
+	if (ret) {
+		pm_runtime_put(&pdev->dev);
 		return ret;
+	}
 
 	clk_alpha_pll_configure(&gpll10, regmap, &gpll10_config);
 	clk_alpha_pll_configure(&gpll11, regmap, &gpll11_config);
@@ -2912,7 +2925,10 @@ static int gcc_qcm2290_probe(struct platform_device *pdev)
 	qcom_branch_set_clk_en(regmap, 0x36004);
 	qcom_branch_set_clk_en(regmap, 0x2b06c);
 
-	return qcom_cc_really_probe(pdev, &gcc_qcm2290_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &gcc_qcm2290_desc, regmap);
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
 }
 
 static struct platform_driver gcc_qcm2290_driver = {

-- 
2.41.0

[PATCH 12/15] arm64: dts: qcom: sm6375: Add VDD_CX to GCC

[Konrad Dybcio]

The GCC block is mainly powered by VDD_CX. Describe that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm6375.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6375.dtsi b/arch/arm64/boot/dts/qcom/sm6375.dtsi
index e7ff55443da7..6fec45b54c98 100644
--- a/arch/arm64/boot/dts/qcom/sm6375.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6375.dtsi
@@ -904,6 +904,7 @@ gcc: clock-controller@1400000 {
 			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
 				 <&rpmcc RPM_SMD_XO_A_CLK_SRC>,
 				 <&sleep_clk>;
+			power-domains = <&rpmpd SM6375_VDDCX>;
 			#power-domain-cells = <1>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;

-- 
2.41.0

[PATCH 13/15] arm64: dts: qcom: qcm2290: Add VDD_CX to GCC

[Konrad Dybcio]

The GCC block is mainly powered by VDD_CX. Describe that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/qcm2290.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/qcm2290.dtsi b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
index d46e591e72b5..a3191e3548c1 100644
--- a/arch/arm64/boot/dts/qcom/qcm2290.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
@@ -622,6 +622,7 @@ gcc: clock-controller@1400000 {
 			reg = <0x0 0x01400000 0x0 0x1f0000>;
 			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, <&sleep_clk>;
 			clock-names = "bi_tcxo", "sleep_clk";
+			power-domains = <&rpmpd QCM2290_VDDCX>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;

-- 
2.41.0

[PATCH 14/15] arm64: dts: qcom: sm6115: Add VDD_CX to GCC

[Konrad Dybcio]

The GCC block is mainly powered by VDD_CX. Describe that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm6115.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
index 839c60351240..29b5b388cd94 100644
--- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
@@ -784,6 +784,7 @@ gcc: clock-controller@1400000 {
 			reg = <0x0 0x01400000 0x0 0x1f0000>;
 			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, <&sleep_clk>;
 			clock-names = "bi_tcxo", "sleep_clk";
+			power-domains = <&rpmpd SM6115_VDDCX>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;

-- 
2.41.0

[PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Konrad Dybcio]

The GPU_CC block is powered by VDD_CX. Describe that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
index 29b5b388cd94..bfaaa1801a4d 100644
--- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
@@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
 			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
 				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
 				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
+			power-domains = <&rpmpd SM6115_VDDCX>;
+			required-opps = <&rpmpd_opp_low_svs>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;

-- 
2.41.0

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
> The GPU_CC block is powered by VDD_CX. Describe that.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> index 29b5b388cd94..bfaaa1801a4d 100644
> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> +			power-domains = <&rpmpd SM6115_VDDCX>;
> +			required-opps = <&rpmpd_opp_low_svs>;

Where is this required-opp coming from? The clocks in gpucc seem to have
different voltage requirements depending on the rates, but we usually
handle that in the OPP tables of the consumer.

Thanks,
Stephan

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Konrad Dybcio]

On 17.07.2023 18:28, Stephan Gerhold wrote:
> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
>> The GPU_CC block is powered by VDD_CX. Describe that.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>> index 29b5b388cd94..bfaaa1801a4d 100644
>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
>> +			power-domains = <&rpmpd SM6115_VDDCX>;
>> +			required-opps = <&rpmpd_opp_low_svs>;
> 
> Where is this required-opp coming from? The clocks in gpucc seem to have
> different voltage requirements depending on the rates, but we usually
> handle that in the OPP tables of the consumer.
The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
but quite obviously the GPU won't work then

Konrad
> 
> Thanks,
> Stephan

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
> On 17.07.2023 18:28, Stephan Gerhold wrote:
> > On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
> >> The GPU_CC block is powered by VDD_CX. Describe that.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
> >>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >> index 29b5b388cd94..bfaaa1801a4d 100644
> >> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
> >>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
> >>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
> >>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> >> +			power-domains = <&rpmpd SM6115_VDDCX>;
> >> +			required-opps = <&rpmpd_opp_low_svs>;
> > 
> > Where is this required-opp coming from? The clocks in gpucc seem to have
> > different voltage requirements depending on the rates, but we usually
> > handle that in the OPP tables of the consumer.
> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
> but quite obviously the GPU won't work then
> 

The levels needed for the GPU clocks to run should be in the GPU OPP
table though, just like e.g. sdhc2_opp_table for the SDCC clocks.

I still don't really understand why this is specified here. :)

Stephan

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Konrad Dybcio]

On 17.07.2023 18:56, Stephan Gerhold wrote:
> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
>> On 17.07.2023 18:28, Stephan Gerhold wrote:
>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
>>>> The GPU_CC block is powered by VDD_CX. Describe that.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>> index 29b5b388cd94..bfaaa1801a4d 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
>>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
>>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
>>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
>>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
>>>> +			required-opps = <&rpmpd_opp_low_svs>;
>>>
>>> Where is this required-opp coming from? The clocks in gpucc seem to have
>>> different voltage requirements depending on the rates, but we usually
>>> handle that in the OPP tables of the consumer.
>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
>> but quite obviously the GPU won't work then
>>
> 
> The levels needed for the GPU clocks to run should be in the GPU OPP
> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
> 
> I still don't really understand why this is specified here. :)
The GPU_CC block needs this rail to be at a certain power level for
register access. This describes that requirement.

Konrad

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
> On 17.07.2023 18:56, Stephan Gerhold wrote:
> > On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
> >> On 17.07.2023 18:28, Stephan Gerhold wrote:
> >>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
> >>>> The GPU_CC block is powered by VDD_CX. Describe that.
> >>>>
> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>> ---
> >>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>> index 29b5b388cd94..bfaaa1801a4d 100644
> >>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
> >>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
> >>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
> >>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> >>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
> >>>> +			required-opps = <&rpmpd_opp_low_svs>;
> >>>
> >>> Where is this required-opp coming from? The clocks in gpucc seem to have
> >>> different voltage requirements depending on the rates, but we usually
> >>> handle that in the OPP tables of the consumer.
> >> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
> >> but quite obviously the GPU won't work then
> >>
> > 
> > The levels needed for the GPU clocks to run should be in the GPU OPP
> > table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
> > 
> > I still don't really understand why this is specified here. :)
> The GPU_CC block needs this rail to be at a certain power level for
> register access. This describes that requirement.
> 

Can you show where this is defined downstream? On a quick look I didn't
see something like that anywhere. Or is this from some secret
documentation?

Thanks,
Stephan

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Konrad Dybcio]

On 17.07.2023 19:23, Stephan Gerhold wrote:
> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
>> On 17.07.2023 18:56, Stephan Gerhold wrote:
>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
>>>> On 17.07.2023 18:28, Stephan Gerhold wrote:
>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
>>>>>> The GPU_CC block is powered by VDD_CX. Describe that.
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> ---
>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
>>>>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
>>>>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
>>>>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
>>>>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
>>>>>> +			required-opps = <&rpmpd_opp_low_svs>;
>>>>>
>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have
>>>>> different voltage requirements depending on the rates, but we usually
>>>>> handle that in the OPP tables of the consumer.
>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
>>>> but quite obviously the GPU won't work then
>>>>
>>>
>>> The levels needed for the GPU clocks to run should be in the GPU OPP
>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
>>>
>>> I still don't really understand why this is specified here. :)
>> The GPU_CC block needs this rail to be at a certain power level for
>> register access. This describes that requirement.
>>
> 
> Can you show where this is defined downstream? On a quick look I didn't
> see something like that anywhere. Or is this from some secret
> documentation?
As far as downstream goes, you can notice that no branch's or RCG's
vdd tables ever define a level lower than the one I mentioned.

Konrad

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote:
> On 17.07.2023 19:23, Stephan Gerhold wrote:
> > On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
> >> On 17.07.2023 18:56, Stephan Gerhold wrote:
> >>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
> >>>> On 17.07.2023 18:28, Stephan Gerhold wrote:
> >>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
> >>>>>> The GPU_CC block is powered by VDD_CX. Describe that.
> >>>>>>
> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>>> ---
> >>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
> >>>>>>  1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>> index 29b5b388cd94..bfaaa1801a4d 100644
> >>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
> >>>>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
> >>>>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
> >>>>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> >>>>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
> >>>>>> +			required-opps = <&rpmpd_opp_low_svs>;
> >>>>>
> >>>>> Where is this required-opp coming from? The clocks in gpucc seem to have
> >>>>> different voltage requirements depending on the rates, but we usually
> >>>>> handle that in the OPP tables of the consumer.
> >>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
> >>>> but quite obviously the GPU won't work then
> >>>>
> >>>
> >>> The levels needed for the GPU clocks to run should be in the GPU OPP
> >>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
> >>>
> >>> I still don't really understand why this is specified here. :)
> >> The GPU_CC block needs this rail to be at a certain power level for
> >> register access. This describes that requirement.
> >>
> > 
> > Can you show where this is defined downstream? On a quick look I didn't
> > see something like that anywhere. Or is this from some secret
> > documentation?
> As far as downstream goes, you can notice that no branch's or RCG's
> vdd tables ever define a level lower than the one I mentioned.
> 

As far as I can tell the vdd tables are only used when the clock is
actually enabled though, not for writing to registers while they are
disabled.

Stephan

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Konrad Dybcio]

On 18.07.2023 13:56, Stephan Gerhold wrote:
> On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote:
>> On 17.07.2023 19:23, Stephan Gerhold wrote:
>>> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
>>>> On 17.07.2023 18:56, Stephan Gerhold wrote:
>>>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
>>>>>> On 17.07.2023 18:28, Stephan Gerhold wrote:
>>>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
>>>>>>>> The GPU_CC block is powered by VDD_CX. Describe that.
>>>>>>>>
>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>> ---
>>>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644
>>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
>>>>>>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
>>>>>>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
>>>>>>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
>>>>>>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
>>>>>>>> +			required-opps = <&rpmpd_opp_low_svs>;
>>>>>>>
>>>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have
>>>>>>> different voltage requirements depending on the rates, but we usually
>>>>>>> handle that in the OPP tables of the consumer.
>>>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
>>>>>> but quite obviously the GPU won't work then
>>>>>>
>>>>>
>>>>> The levels needed for the GPU clocks to run should be in the GPU OPP
>>>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
>>>>>
>>>>> I still don't really understand why this is specified here. :)
>>>> The GPU_CC block needs this rail to be at a certain power level for
>>>> register access. This describes that requirement.
>>>>
>>>
>>> Can you show where this is defined downstream? On a quick look I didn't
>>> see something like that anywhere. Or is this from some secret
>>> documentation?
>> As far as downstream goes, you can notice that no branch's or RCG's
>> vdd tables ever define a level lower than the one I mentioned.
>>
> 
> As far as I can tell the vdd tables are only used when the clock is
> actually enabled though, not for writing to registers while they are
> disabled.
Maybe, but you can also notice that even XO rates require at least
SVS_LOW to function.

Konrad

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Dmitry Baryshkov]

On Tue, 18 Jul 2023 at 15:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 18.07.2023 13:56, Stephan Gerhold wrote:
> > On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote:
> >> On 17.07.2023 19:23, Stephan Gerhold wrote:
> >>> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
> >>>> On 17.07.2023 18:56, Stephan Gerhold wrote:
> >>>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
> >>>>>> On 17.07.2023 18:28, Stephan Gerhold wrote:
> >>>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
> >>>>>>>> The GPU_CC block is powered by VDD_CX. Describe that.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>>>>> ---
> >>>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
> >>>>>>>>  1 file changed, 2 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644
> >>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
> >>>>>>>>                        clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
> >>>>>>>>                                 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
> >>>>>>>>                                 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> >>>>>>>> +                      power-domains = <&rpmpd SM6115_VDDCX>;
> >>>>>>>> +                      required-opps = <&rpmpd_opp_low_svs>;
> >>>>>>>
> >>>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have
> >>>>>>> different voltage requirements depending on the rates, but we usually
> >>>>>>> handle that in the OPP tables of the consumer.
> >>>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
> >>>>>> but quite obviously the GPU won't work then
> >>>>>>
> >>>>>
> >>>>> The levels needed for the GPU clocks to run should be in the GPU OPP
> >>>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
> >>>>>
> >>>>> I still don't really understand why this is specified here. :)
> >>>> The GPU_CC block needs this rail to be at a certain power level for
> >>>> register access. This describes that requirement.
> >>>>
> >>>
> >>> Can you show where this is defined downstream? On a quick look I didn't
> >>> see something like that anywhere. Or is this from some secret
> >>> documentation?
> >> As far as downstream goes, you can notice that no branch's or RCG's
> >> vdd tables ever define a level lower than the one I mentioned.
> >>
> >
> > As far as I can tell the vdd tables are only used when the clock is
> > actually enabled though, not for writing to registers while they are
> > disabled.
> Maybe, but you can also notice that even XO rates require at least
> SVS_LOW to function.

But the vdd tables are related to clock rates, which, in the upstream
design, should be voted by the consumers, not by the clock driver.


-- 
With best wishes
Dmitry

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Konrad Dybcio]

On 18.07.2023 15:08, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 15:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 18.07.2023 13:56, Stephan Gerhold wrote:
>>> On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote:
>>>> On 17.07.2023 19:23, Stephan Gerhold wrote:
>>>>> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
>>>>>> On 17.07.2023 18:56, Stephan Gerhold wrote:
>>>>>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
>>>>>>>> On 17.07.2023 18:28, Stephan Gerhold wrote:
>>>>>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
>>>>>>>>>> The GPU_CC block is powered by VDD_CX. Describe that.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
>>>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644
>>>>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
>>>>>>>>>>                        clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
>>>>>>>>>>                                 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
>>>>>>>>>>                                 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
>>>>>>>>>> +                      power-domains = <&rpmpd SM6115_VDDCX>;
>>>>>>>>>> +                      required-opps = <&rpmpd_opp_low_svs>;
>>>>>>>>>
>>>>>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have
>>>>>>>>> different voltage requirements depending on the rates, but we usually
>>>>>>>>> handle that in the OPP tables of the consumer.
>>>>>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
>>>>>>>> but quite obviously the GPU won't work then
>>>>>>>>
>>>>>>>
>>>>>>> The levels needed for the GPU clocks to run should be in the GPU OPP
>>>>>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
>>>>>>>
>>>>>>> I still don't really understand why this is specified here. :)
>>>>>> The GPU_CC block needs this rail to be at a certain power level for
>>>>>> register access. This describes that requirement.
>>>>>>
>>>>>
>>>>> Can you show where this is defined downstream? On a quick look I didn't
>>>>> see something like that anywhere. Or is this from some secret
>>>>> documentation?
>>>> As far as downstream goes, you can notice that no branch's or RCG's
>>>> vdd tables ever define a level lower than the one I mentioned.
>>>>
>>>
>>> As far as I can tell the vdd tables are only used when the clock is
>>> actually enabled though, not for writing to registers while they are
>>> disabled.
>> Maybe, but you can also notice that even XO rates require at least
>> SVS_LOW to function.
> 
> But the vdd tables are related to clock rates, which, in the upstream
> design, should be voted by the consumers, not by the clock driver.
Not all of the clocks are associated with OPP tables upstream, and it
would be nice if the clock controller block had power flowing to it
in case one wanted to access a different clock.

Konrad

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Bjorn Andersson]

On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
> On 17.07.2023 18:56, Stephan Gerhold wrote:
> > On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
> >> On 17.07.2023 18:28, Stephan Gerhold wrote:
> >>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
> >>>> The GPU_CC block is powered by VDD_CX. Describe that.
> >>>>
> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>> ---
> >>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>> index 29b5b388cd94..bfaaa1801a4d 100644
> >>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
> >>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
> >>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
> >>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> >>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
> >>>> +			required-opps = <&rpmpd_opp_low_svs>;
> >>>
> >>> Where is this required-opp coming from? The clocks in gpucc seem to have
> >>> different voltage requirements depending on the rates, but we usually
> >>> handle that in the OPP tables of the consumer.
> >> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
> >> but quite obviously the GPU won't work then
> >>
> > 
> > The levels needed for the GPU clocks to run should be in the GPU OPP
> > table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
> > 
> > I still don't really understand why this is specified here. :)
> The GPU_CC block needs this rail to be at a certain power level for
> register access. This describes that requirement.
> 

And that is not the lowest level reported by command db?
Please describe this part in the commit message as well.

Thanks,
Bjorn

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Konrad Dybcio]

On 18.07.2023 06:25, Bjorn Andersson wrote:
> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
>> On 17.07.2023 18:56, Stephan Gerhold wrote:
>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
>>>> On 17.07.2023 18:28, Stephan Gerhold wrote:
>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
>>>>>> The GPU_CC block is powered by VDD_CX. Describe that.
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> ---
>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
>>>>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
>>>>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
>>>>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
>>>>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
>>>>>> +			required-opps = <&rpmpd_opp_low_svs>;
>>>>>
>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have
>>>>> different voltage requirements depending on the rates, but we usually
>>>>> handle that in the OPP tables of the consumer.
>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
>>>> but quite obviously the GPU won't work then
>>>>
>>>
>>> The levels needed for the GPU clocks to run should be in the GPU OPP
>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
>>>
>>> I still don't really understand why this is specified here. :)
>> The GPU_CC block needs this rail to be at a certain power level for
>> register access. This describes that requirement.
>>
> 
> And that is not the lowest level reported by command db?
> Please describe this part in the commit message as well.
command-what? ;)

RPM exports VDD_NONE (off), VDD_MIN (the lowest state before collapse)
and then low_svs is usually the lowest "actually on" state for all
consumers.

Konrad

Re: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

[Bjorn Andersson]

On Tue, Jul 18, 2023 at 02:21:31PM +0200, Konrad Dybcio wrote:
> On 18.07.2023 06:25, Bjorn Andersson wrote:
> > On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote:
> >> On 17.07.2023 18:56, Stephan Gerhold wrote:
> >>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote:
> >>>> On 17.07.2023 18:28, Stephan Gerhold wrote:
> >>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote:
> >>>>>> The GPU_CC block is powered by VDD_CX. Describe that.
> >>>>>>
> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>>> ---
> >>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++
> >>>>>>  1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>> index 29b5b388cd94..bfaaa1801a4d 100644
> >>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 {
> >>>>>>  			clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>,
> >>>>>>  				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
> >>>>>>  				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> >>>>>> +			power-domains = <&rpmpd SM6115_VDDCX>;
> >>>>>> +			required-opps = <&rpmpd_opp_low_svs>;
> >>>>>
> >>>>> Where is this required-opp coming from? The clocks in gpucc seem to have
> >>>>> different voltage requirements depending on the rates, but we usually
> >>>>> handle that in the OPP tables of the consumer.
> >>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET,
> >>>> but quite obviously the GPU won't work then
> >>>>
> >>>
> >>> The levels needed for the GPU clocks to run should be in the GPU OPP
> >>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks.
> >>>
> >>> I still don't really understand why this is specified here. :)
> >> The GPU_CC block needs this rail to be at a certain power level for
> >> register access. This describes that requirement.
> >>
> > 
> > And that is not the lowest level reported by command db?
> > Please describe this part in the commit message as well.
> command-what? ;)
> 

Apparently doesn't matter that I read that line multiple times, my brain
really wanted a 'h' in there.

> RPM exports VDD_NONE (off), VDD_MIN (the lowest state before collapse)
> and then low_svs is usually the lowest "actually on" state for all
> consumers.
> 

In rpmhpd I changed it such that the minimal enabled state would be
!disabled (so that the automatic enablement during probe would be
sufficient to access registers), but talking to Ulf this is
provider-specific.

So unless you can figure out a acceptable lowest non-disabled state this
is what has to be done...


PS. My ask for mentioning this in the commit message still stands.

Regards,
Bjorn