[PATCH v3 0/2] clk: qcom: gcc-sm8550: Fix shared clk parking breakage

[PATCH v3 0/2] clk: qcom: gcc-sm8550: Fix shared clk parking breakage

[Stephen Boyd]

Amit Pundir reported[1] that a recent commit 01a0a6cc8cfd ("clk: qcom:
Park shared RCGs upon registration") broke USB and audio on sm8550-hdk.
These two patches fix the issue by skipping the parking bit of the
shared RCGs for all clks except for the ones that were causing trouble,
i.e. the display RCGs on sc7180.

The first patch is all that's required, while the second patch can be
applied anytime to simplify the QUP clks that don't need to be parked.

Changes from v2: https://lore.kernel.org/r/20240827231237.1014813-1-swboyd@chromium.org
 * Reword first patch commit text to clarify further

Changes from v1: https://lore.kernel.org/r/20240819233628.2074654-1-swboyd@chromium.org
 * Revert most of the bad patch, except for the sc7180 display clks that still
   want to park at init
 * Re-order series

[1] https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@mail.gmail.com/

Stephen Boyd (2):
  clk: qcom: dispcc-sc7180: Only park display clks at init
  clk: qcom: gcc-sm8550: Don't use shared clk_ops for QUPs

 drivers/clk/qcom/clk-rcg.h       |  1 +
 drivers/clk/qcom/clk-rcg2.c      | 36 ++++++++++++++++++++--
 drivers/clk/qcom/dispcc-sc7180.c |  8 ++---
 drivers/clk/qcom/gcc-sm8550.c    | 52 ++++++++++++++++----------------
 4 files changed, 64 insertions(+), 33 deletions(-)


base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
-- 
https://chromeos.dev

[PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Stephen Boyd]

Amit Pundir reports that audio and USB-C host mode stops working on
sm8550 after commit 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon
registration"). That's because the gcc_usb30_prim_master_clk_src clk is
registered and clk_rcg2_shared_init() parks it on XO. Running USB at a
slower speed than the link supports is effectively under-clocking the
USB link and probably confusing the downstream USB devices.

We didn't need to change all the shared RCGs to park on XO at
registration time in commit commit 01a0a6cc8cfd ("clk: qcom: Park shared
RCGs upon registration"). Instead, we only needed to park the few
display related clks on sc7180 to fix the issue.

Fix sm8550 (and likely other qcom SoCs) by skipping the parking part of
clk_rcg2_shared_init(). Make that the default init clk_op for shared
RCGs, but keep the part where we cache the config register as that's
still necessary to figure out the true parent of the clk is. Introduce
another set of clk_ops 'clk_rcg2_shared_init_park' that does what
clk_rcg2_shared_init() was doing and use that for the display clks on
sc7180. This fixes the sm8550 problem and limits the "park upon
registration" logic to the display clks that need it.

Fixes: 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
Cc: Konrad Dybcio <konradybcio@kernel.org>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Reported-by: Amit Pundir <amit.pundir@linaro.org>
Closes: https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@mail.gmail.com
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/clk/qcom/clk-rcg.h       |  1 +
 drivers/clk/qcom/clk-rcg2.c      | 36 +++++++++++++++++++++++++++++---
 drivers/clk/qcom/dispcc-sc7180.c |  8 +++----
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index d7414361e432..5f479a29d969 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -198,6 +198,7 @@ extern const struct clk_ops clk_byte2_ops;
 extern const struct clk_ops clk_pixel_ops;
 extern const struct clk_ops clk_gfx3d_ops;
 extern const struct clk_ops clk_rcg2_shared_ops;
+extern const struct clk_ops clk_rcg2_shared_init_park_ops;
 extern const struct clk_ops clk_dp_ops;
 
 struct clk_rcg_dfs_data {
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 30b19bd39d08..5f0b729d7115 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -1305,6 +1305,31 @@ clk_rcg2_shared_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 }
 
 static int clk_rcg2_shared_init(struct clk_hw *hw)
+{
+	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+
+	/*
+	 * Cache the cfg so that the parent is properly mapped at registration.
+	 */
+	regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &rcg->parked_cfg);
+
+	return 0;
+}
+
+const struct clk_ops clk_rcg2_shared_ops = {
+	.init = clk_rcg2_shared_init,
+	.enable = clk_rcg2_shared_enable,
+	.disable = clk_rcg2_shared_disable,
+	.get_parent = clk_rcg2_shared_get_parent,
+	.set_parent = clk_rcg2_shared_set_parent,
+	.recalc_rate = clk_rcg2_shared_recalc_rate,
+	.determine_rate = clk_rcg2_determine_rate,
+	.set_rate = clk_rcg2_shared_set_rate,
+	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
+};
+EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
+
+static int clk_rcg2_shared_init_park(struct clk_hw *hw)
 {
 	/*
 	 * This does a few things:
@@ -1335,8 +1360,13 @@ static int clk_rcg2_shared_init(struct clk_hw *hw)
 	return 0;
 }
 
-const struct clk_ops clk_rcg2_shared_ops = {
-	.init = clk_rcg2_shared_init,
+/*
+ * Like clk_rcg2_shared_ops but also park the clk at init to avoid the parent
+ * from being shutdown, getting the clk stuck when it is turned on
+ * automatically by the GDSC.
+ */
+const struct clk_ops clk_rcg2_shared_init_park_ops = {
+	.init = clk_rcg2_shared_init_park,
 	.enable = clk_rcg2_shared_enable,
 	.disable = clk_rcg2_shared_disable,
 	.get_parent = clk_rcg2_shared_get_parent,
@@ -1346,7 +1376,7 @@ const struct clk_ops clk_rcg2_shared_ops = {
 	.set_rate = clk_rcg2_shared_set_rate,
 	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
 };
-EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
+EXPORT_SYMBOL_GPL(clk_rcg2_shared_init_park_ops);
 
 /* Common APIs to be used for DFS based RCGR */
 static void clk_rcg2_dfs_populate_freq(struct clk_hw *hw, unsigned int l,
diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
index 4710247be530..068af819f23e 100644
--- a/drivers/clk/qcom/dispcc-sc7180.c
+++ b/drivers/clk/qcom/dispcc-sc7180.c
@@ -154,7 +154,7 @@ static struct clk_rcg2 disp_cc_mdss_ahb_clk_src = {
 		.parent_data = disp_cc_parent_data_4,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_4),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_shared_init_park_ops,
 	},
 };
 
@@ -263,7 +263,7 @@ static struct clk_rcg2 disp_cc_mdss_mdp_clk_src = {
 		.name = "disp_cc_mdss_mdp_clk_src",
 		.parent_data = disp_cc_parent_data_3,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_shared_init_park_ops,
 	},
 };
 
@@ -291,7 +291,7 @@ static struct clk_rcg2 disp_cc_mdss_rot_clk_src = {
 		.name = "disp_cc_mdss_rot_clk_src",
 		.parent_data = disp_cc_parent_data_3,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_shared_init_park_ops,
 	},
 };
 
@@ -305,7 +305,7 @@ static struct clk_rcg2 disp_cc_mdss_vsync_clk_src = {
 		.name = "disp_cc_mdss_vsync_clk_src",
 		.parent_data = disp_cc_parent_data_0,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_shared_init_park_ops,
 	},
 };
 
-- 
https://chromeos.dev

[PATCH v3 2/2] clk: qcom: gcc-sm8550: Don't use shared clk_ops for QUPs

[Stephen Boyd]

The QUPs aren't shared in a way that requires parking the RCG at an
always on parent in case some other entity turns on the clk. The
hardware is capable of setting a new frequency itself with the DFS mode,
so parking is unnecessary. Furthermore, there aren't any GDSCs for these
devices, so there isn't a possibility of the GDSC turning on the clks
for housekeeping purposes.

This wasn't a problem to mark these clks shared until we started parking
shared RCGs at clk registration time in commit 01a0a6cc8cfd ("clk: qcom:
Park shared RCGs upon registration"). Parking at init is actually
harmful to the UART when earlycon is used. If the device is pumping out
data while the frequency changes you'll see garbage on the serial
console until the driver can probe and actually set a proper frequency.

Revert the QUP part of commit 929c75d57566 ("clk: qcom: gcc-sm8550: Mark
RCGs shared where applicable") so that the QUPs don't get parked during
clk registration and break UART operations.

Fixes: 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
Fixes: 929c75d57566 ("clk: qcom: gcc-sm8550: Mark RCGs shared where applicable")
Cc: Konrad Dybcio <konradybcio@kernel.org>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Reported-by: Amit Pundir <amit.pundir@linaro.org>
Closes: https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@mail.gmail.com
Tested-by: Amit Pundir <amit.pundir@linaro.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/clk/qcom/gcc-sm8550.c | 52 +++++++++++++++++------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm8550.c b/drivers/clk/qcom/gcc-sm8550.c
index 7944ddb4b47d..0244a05866b8 100644
--- a/drivers/clk/qcom/gcc-sm8550.c
+++ b/drivers/clk/qcom/gcc-sm8550.c
@@ -536,7 +536,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s0_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -551,7 +551,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s1_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -566,7 +566,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s2_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -581,7 +581,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s3_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -596,7 +596,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s4_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -611,7 +611,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s5_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -626,7 +626,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s6_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -641,7 +641,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s7_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -656,7 +656,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s8_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -671,7 +671,7 @@ static struct clk_rcg2 gcc_qupv3_i2c_s9_clk_src = {
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_ops,
 	},
 };
 
@@ -700,7 +700,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s0_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s0_clk_src = {
@@ -717,7 +717,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s1_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s1_clk_src = {
@@ -750,7 +750,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s2_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s2_clk_src = {
@@ -767,7 +767,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s3_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s3_clk_src = {
@@ -784,7 +784,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s4_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s4_clk_src = {
@@ -801,7 +801,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s5_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s5_clk_src = {
@@ -818,7 +818,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s6_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s6_clk_src = {
@@ -835,7 +835,7 @@ static struct clk_init_data gcc_qupv3_wrap1_s7_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap1_s7_clk_src = {
@@ -852,7 +852,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s0_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s0_clk_src = {
@@ -869,7 +869,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s1_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s1_clk_src = {
@@ -886,7 +886,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s2_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s2_clk_src = {
@@ -903,7 +903,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s3_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s3_clk_src = {
@@ -920,7 +920,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s4_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s4_clk_src = {
@@ -937,7 +937,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s5_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s5_clk_src = {
@@ -975,7 +975,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s6_clk_src_init = {
 	.parent_data = gcc_parent_data_8,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_8),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s6_clk_src = {
@@ -992,7 +992,7 @@ static struct clk_init_data gcc_qupv3_wrap2_s7_clk_src_init = {
 	.parent_data = gcc_parent_data_0,
 	.num_parents = ARRAY_SIZE(gcc_parent_data_0),
 	.flags = CLK_SET_RATE_PARENT,
-	.ops = &clk_rcg2_shared_ops,
+	.ops = &clk_rcg2_ops,
 };
 
 static struct clk_rcg2 gcc_qupv3_wrap2_s7_clk_src = {
-- 
https://chromeos.dev

Re: [PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Johan Hovold]

On Wed, Aug 28, 2024 at 10:17:07AM -0700, Stephen Boyd wrote:
> Amit Pundir reports that audio and USB-C host mode stops working on
> sm8550 after commit 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon
> registration"). That's because the gcc_usb30_prim_master_clk_src clk is
> registered and clk_rcg2_shared_init() parks it on XO. Running USB at a
> slower speed than the link supports is effectively under-clocking the
> USB link and probably confusing the downstream USB devices.
> 
> We didn't need to change all the shared RCGs to park on XO at
> registration time in commit commit 01a0a6cc8cfd ("clk: qcom: Park shared
> RCGs upon registration"). Instead, we only needed to park the few
> display related clks on sc7180 to fix the issue.
> 
> Fix sm8550 (and likely other qcom SoCs) by skipping the parking part of
> clk_rcg2_shared_init(). Make that the default init clk_op for shared
> RCGs, but keep the part where we cache the config register as that's
> still necessary to figure out the true parent of the clk is. Introduce
> another set of clk_ops 'clk_rcg2_shared_init_park' that does what
> clk_rcg2_shared_init() was doing and use that for the display clks on
> sc7180. This fixes the sm8550 problem and limits the "park upon
> registration" logic to the display clks that need it.
> 
> Fixes: 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Reported-by: Amit Pundir <amit.pundir@linaro.org>
> Closes: https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@mail.gmail.com
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

I can confirm that this fixes the earlycon issue on x1e80100 that Bryan
reported here:

	https://lore.kernel.org/lkml/20240823-x1e80100-clk-fix-v1-1-0b1b4f5a96e8@linaro.org/

Perhaps you can drop the "dispcc-sc7180:" prefix from Subject when
applying (or resending) since this fixes a regression on multiple
Qualcomm SoCs. For example, use something like:

	clk: qcom: Only park sc7180 display clks at init

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

Johan

Re: [PATCH v3 2/2] clk: qcom: gcc-sm8550: Don't use shared clk_ops for QUPs

[Johan Hovold]

On Wed, Aug 28, 2024 at 10:17:08AM -0700, Stephen Boyd wrote:
> The QUPs aren't shared in a way that requires parking the RCG at an
> always on parent in case some other entity turns on the clk. The
> hardware is capable of setting a new frequency itself with the DFS mode,
> so parking is unnecessary. Furthermore, there aren't any GDSCs for these
> devices, so there isn't a possibility of the GDSC turning on the clks
> for housekeeping purposes.
> 
> This wasn't a problem to mark these clks shared until we started parking
> shared RCGs at clk registration time in commit 01a0a6cc8cfd ("clk: qcom:
> Park shared RCGs upon registration"). Parking at init is actually
> harmful to the UART when earlycon is used. If the device is pumping out
> data while the frequency changes you'll see garbage on the serial
> console until the driver can probe and actually set a proper frequency.
> 
> Revert the QUP part of commit 929c75d57566 ("clk: qcom: gcc-sm8550: Mark
> RCGs shared where applicable") so that the QUPs don't get parked during
> clk registration and break UART operations.

So this one doesn't really fix anything after patch 1/2 in v3, so
perhaps you can rephrase and drop "break UART operations" here.
 
> Fixes: 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")

And drop this.

> Fixes: 929c75d57566 ("clk: qcom: gcc-sm8550: Mark RCGs shared where applicable")
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>

> Reported-by: Amit Pundir <amit.pundir@linaro.org>
> Closes: https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@mail.gmail.com

And possibly these two as well.

> Tested-by: Amit Pundir <amit.pundir@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Johan

Re: [PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Neil Armstrong]

Hi,

On 28/08/2024 19:17, Stephen Boyd wrote:
> Amit Pundir reports that audio and USB-C host mode stops working on
> sm8550 after commit 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon
> registration"). That's because the gcc_usb30_prim_master_clk_src clk is
> registered and clk_rcg2_shared_init() parks it on XO. Running USB at a
> slower speed than the link supports is effectively under-clocking the
> USB link and probably confusing the downstream USB devices.
> 
> We didn't need to change all the shared RCGs to park on XO at
> registration time in commit commit 01a0a6cc8cfd ("clk: qcom: Park shared
> RCGs upon registration"). Instead, we only needed to park the few
> display related clks on sc7180 to fix the issue.
> 
> Fix sm8550 (and likely other qcom SoCs) by skipping the parking part of
> clk_rcg2_shared_init(). Make that the default init clk_op for shared
> RCGs, but keep the part where we cache the config register as that's
> still necessary to figure out the true parent of the clk is. Introduce
> another set of clk_ops 'clk_rcg2_shared_init_park' that does what
> clk_rcg2_shared_init() was doing and use that for the display clks on
> sc7180. This fixes the sm8550 problem and limits the "park upon
> registration" logic to the display clks that need it.
> 
> Fixes: 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
> Cc: Konrad Dybcio <konradybcio@kernel.org>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Reported-by: Amit Pundir <amit.pundir@linaro.org>
> Closes: https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@mail.gmail.com
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>   drivers/clk/qcom/clk-rcg.h       |  1 +
>   drivers/clk/qcom/clk-rcg2.c      | 36 +++++++++++++++++++++++++++++---
>   drivers/clk/qcom/dispcc-sc7180.c |  8 +++----
>   3 files changed, 38 insertions(+), 7 deletions(-)
> 
<snip>

I've been testing this serie on SM8650, and with 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
it fixes an issue we have that triggers:
[   18.740736] ------------[ cut here ]------------
[   18.745837] ufs_phy_gdsc status stuck at 'off'
[   18.745929] WARNING: CPU: 1 PID: 197 at drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x15c/0x164
...
after waking up UFS from runtime suspend.

So I suspect we'll need to figure out which SM8650 GCC shared clocks would need
to use clk_rcg2_shared_init_park_ops ?

We also had random boot crash when initializing the display very late on multiple platforms,
and so far "clk: qcom: Park shared RCGs upon registration" fixed that, but I also suppose Ill
need to reflect the changes done to dispcc-sc7180.c to dispcc-sm8650.c and others.

Neil

Re: [PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Stephen Boyd]

Quoting Neil Armstrong (2024-08-29 08:32:48)
>
>
> I've been testing this serie on SM8650, and with 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
> it fixes an issue we have that triggers:
> [   18.740736] ------------[ cut here ]------------
> [   18.745837] ufs_phy_gdsc status stuck at 'off'
> [   18.745929] WARNING: CPU: 1 PID: 197 at drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x15c/0x164
> ...
> after waking up UFS from runtime suspend.

Oof that's not good.

>
> So I suspect we'll need to figure out which SM8650 GCC shared clocks would need
> to use clk_rcg2_shared_init_park_ops ?
>
> We also had random boot crash when initializing the display very late on multiple platforms,
> and so far "clk: qcom: Park shared RCGs upon registration" fixed that, but I also suppose Ill
> need to reflect the changes done to dispcc-sc7180.c to dispcc-sm8650.c and others.

It sounds like it's better to make the default always park at
registration time and special case the one or two places where that
isn't possible, i.e. USB because it has special rate requirements. So I
should just go back to v1 then and pile on the QUP patches.

In the long run we probably need to put the clk rate and enable control
into the pm_domains for the gdscs that require them and implement the
"parking" and "unparking" there instead of in the clk_ops layer. That
way we know that when the gdsc is turned on it will have a clk that
works, and when it is turned off it is always parked. We can also use
the gdsc on/off state at init time to increment the enable count on the
shared PLLs by using clk_hw_get_clk() and clk consumer APIs. This will
let us move the boot state into the kernel in a way that doesn't break
when a child clk sharing a PLL is turned off.

Re: [PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Stephen Boyd]

Quoting Stephen Boyd (2024-08-29 09:34:05)
> Quoting Neil Armstrong (2024-08-29 08:32:48)
> >
> >
> > I've been testing this serie on SM8650, and with 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
> > it fixes an issue we have that triggers:
> > [   18.740736] ------------[ cut here ]------------
> > [   18.745837] ufs_phy_gdsc status stuck at 'off'
> > [   18.745929] WARNING: CPU: 1 PID: 197 at drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x15c/0x164
> > ...
> > after waking up UFS from runtime suspend.
>
> Oof that's not good.
>
> >
> > So I suspect we'll need to figure out which SM8650 GCC shared clocks would need
> > to use clk_rcg2_shared_init_park_ops ?
> >
> > We also had random boot crash when initializing the display very late on multiple platforms,
> > and so far "clk: qcom: Park shared RCGs upon registration" fixed that, but I also suppose Ill
> > need to reflect the changes done to dispcc-sc7180.c to dispcc-sm8650.c and others.
>
> It sounds like it's better to make the default always park at
> registration time and special case the one or two places where that
> isn't possible, i.e. USB because it has special rate requirements. So I
> should just go back to v1 then and pile on the QUP patches.

I've done this now and I'll push out clk-fixes with the QUP patches.

Re: [PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Johan Hovold]

On Fri, Aug 30, 2024 at 03:29:22PM -0700, Stephen Boyd wrote:
> Quoting Stephen Boyd (2024-08-29 09:34:05)

> > It sounds like it's better to make the default always park at
> > registration time and special case the one or two places where that
> > isn't possible, i.e. USB because it has special rate requirements. So I
> > should just go back to v1 then and pile on the QUP patches.
> 
> I've done this now and I'll push out clk-fixes with the QUP patches.

I assumed you'd fix up all the other SoCs affected by this, but I only
saw fixes for sm8550, sm8650 and x1e80100 in your fixes branch.

Just sent a corresponding fix for sc8280xp, which I've confirmed also
needs this for QUP:

	https://lore.kernel.org/lkml/20240902070830.8535-1-johan+linaro@kernel.org/

But what about the sm8550 USB issue? Don't the other platforms also need
a corresponding fix (e.g. for when booting from USB)?

Johan

Re: [PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Stephen Boyd]

Quoting Johan Hovold (2024-09-02 00:15:53)
> On Fri, Aug 30, 2024 at 03:29:22PM -0700, Stephen Boyd wrote:
> > Quoting Stephen Boyd (2024-08-29 09:34:05)
> 
> > > It sounds like it's better to make the default always park at
> > > registration time and special case the one or two places where that
> > > isn't possible, i.e. USB because it has special rate requirements. So I
> > > should just go back to v1 then and pile on the QUP patches.
> > 
> > I've done this now and I'll push out clk-fixes with the QUP patches.
> 
> I assumed you'd fix up all the other SoCs affected by this, but I only
> saw fixes for sm8550, sm8650 and x1e80100 in your fixes branch.
> 
> Just sent a corresponding fix for sc8280xp, which I've confirmed also
> needs this for QUP:

Thanks!

> 
>         https://lore.kernel.org/lkml/20240902070830.8535-1-johan+linaro@kernel.org/
> 
> But what about the sm8550 USB issue? Don't the other platforms also need
> a corresponding fix (e.g. for when booting from USB)?

I don't know. Are you seeing USB host issues on other platforms with
shared RCG clk_ops for the USB clk? It looks inconsistent that sometimes
there's a USB GDSC but the shared clk ops aren't used. If nothing is
broken then let's work on the proper fix, which is parking RCGs when the
GDSC is turned off so that turning on the GDSC always works. If USB is
broken for you then send another patch.

Re: [PATCH v3 1/2] clk: qcom: dispcc-sc7180: Only park display clks at init

[Johan Hovold]

On Tue, Sep 03, 2024 at 12:55:07PM -0700, Stephen Boyd wrote:
> Quoting Johan Hovold (2024-09-02 00:15:53)
> > On Fri, Aug 30, 2024 at 03:29:22PM -0700, Stephen Boyd wrote:
> > > Quoting Stephen Boyd (2024-08-29 09:34:05)
> > 
> > > > It sounds like it's better to make the default always park at
> > > > registration time and special case the one or two places where that
> > > > isn't possible, i.e. USB because it has special rate requirements. So I
> > > > should just go back to v1 then and pile on the QUP patches.
> > > 
> > > I've done this now and I'll push out clk-fixes with the QUP patches.
> > 
> > I assumed you'd fix up all the other SoCs affected by this, but I only
> > saw fixes for sm8550, sm8650 and x1e80100 in your fixes branch.
> > 
> > Just sent a corresponding fix for sc8280xp, which I've confirmed also
> > needs this for QUP:
> 
> Thanks!
> 
> >         https://lore.kernel.org/lkml/20240902070830.8535-1-johan+linaro@kernel.org/
> > 
> > But what about the sm8550 USB issue? Don't the other platforms also need
> > a corresponding fix (e.g. for when booting from USB)?
> 
> I don't know. Are you seeing USB host issues on other platforms with
> shared RCG clk_ops for the USB clk? It looks inconsistent that sometimes
> there's a USB GDSC but the shared clk ops aren't used. If nothing is
> broken then let's work on the proper fix, which is parking RCGs when the
> GDSC is turned off so that turning on the GDSC always works. If USB is
> broken for you then send another patch.

I haven't noticed any USB issues on the two platforms I work on and
which both have USB GDSCs (sc8280xp and x1e80100), but then again I
don't test USB booting regularly (if that's when the issue show up).

I was hoping you would have a theory as to why things broke on sm8550 so
that you can reason about which platforms need this, rather than have
multiple users debug these issues and play whack-a-mole with incomplete
fixes.

I don't see anything SoC specific about the QUP fixes and assume early
console handover is now broken on all Qualcomm SoCs for example.

Johan