Message-ID: <CAE-0n53pjPjSw0m93biWOXRkV=V5u-HVqBx8=MLgQ=Z2ydfEKA@mail.gmail.com>
Date: Mon, 12 Aug 2024 15:56:40 -0700
From: Stephen Boyd <swboyd@...omium.org>
To: Amit Pundir <amit.pundir@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, 
	patches@...ts.linux.dev, linux-arm-msm@...r.kernel.org, 
	Laura Nao <laura.nao@...labora.com>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	Douglas Anderson <dianders@...omium.org>, Taniya Das <quic_tdas@...cinc.com>
Subject: Re: [PATCH] clk: qcom: Park shared RCGs upon registration

Quoting Amit Pundir (2024-08-12 02:39:47)
> On Tue, 6 Aug 2024 at 03:07, Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > Quoting Amit Pundir (2024-08-05 03:43:14)
> > > On Sat, 3 Aug 2024 at 06:29, Stephen Boyd <swboyd@...omium.org> wrote:
> > > >
> > > > Also please send back the dmesg so we can see what clks are configured
> > > > for at boot time. If they're using TCXO source at boot then they're not
> > > > going to be broken. In which case those clks can keep using the old clk
> > > > ops and we can focus on the ones that aren't sourcing from TCXO.
> > >
> > > Thank your for this debug patch. I thought I narrowed down the
> > > breakage to the clks in drivers/clk/qcom/gcc-sm8550.c, until I ran
> > > into the following kernel panic in ucsi_glink driver in later test
> > > runs.
> >
> > Thanks for the info. These are the clks that aren't sourcing from XO
> > at registration time:
> >
> >   gcc_qupv3_wrap1_s7_clk_src with cfg 0x102601 -> parent is gpll0_out_even
> >   gcc_ufs_phy_axi_clk_src with cfg 0x103 -> parent is gpll0_out_main
> >   gcc_ufs_phy_ice_core_clk_src with cfg 0x503 -> parent is gpll4_out_main
> >   gcc_ufs_phy_unipro_core_clk_src with cfg 0x103 -> parent is gpll0_out_main
> >   gcc_usb30_prim_master_clk_src with cfg 0x105 -> parent is gpll0_out_main
> >
> > The original patch is going to inform the clk framework that the parent
> > of these clks aren't XO but something like gpll0_out_even, whatever the
> > hardware is configured for. That may cause these PLLs to be turned off
> > earlier than before if, for example, gcc_ufs_phy_axi_clk_src is turned
> > off by a consumer and gcc_usb30_prim_master_clk_src is left enabled at
> > boot. That's why we force park clks at registration time, so that they
> > can't have their parent clk get turned off by some other clk consumer
> > enabling and then disabling a clk that's also parented to the same
> > parent.
> >
> > This same problem exists for RCGs that aren't shared too, but it's
> > particularly bad for shared RCGs because the parent PLLs aren't turned
> > on automatically by the hardware when things like the GDSC do their
> > housekeeping. At least when software is in control we can enable the
> > parent PLL and unstick the RCG that was previously cut off.
>
> Thank you Stephen for the detailed explanation. Really appreciate this
> knowledge dump.
>
> >
> > Can you narrow down the list above to the clk that matters? I guess if
> > USB isn't working then gcc_usb30_prim_master_clk_src is the one that
> > should be changed and nothing else. Although, I noticed that in the
> > first dmesg log you sent the serial console had garbage, and that's
> > likely because the rate changed while the clk was registered. I don't
> > know why the gcc_qupv3_wrap1_s7_clk_src is marked with the shared clk
> > ops. That's confusing to me as I don't expect that to need to be parked
> > for any reasons. Maybe qcom folks can comment there but I'd expect plain
> > rcg2_ops to be used for those clks. Anyway, if you can narrow down to
> > which clk needs to be left untouched it would be helpful.
>
> gcc_qupv3_wrap1_s7_clk_src and gcc_usb30_prim_master_clk_src need to
> be left untouched to fix the Audio codec and USB-C host mode breakages
> respectively. It seem to have fixed the serial console garbage dump
> issue as well.

Alright. Can you try with this patch for the gcc_qupv3* clks on top? And
keep gcc_usb30_prim_master_clk_src on the new clk_ops? I think we need
two patches. One for the usb clk and one for these QUP clks that don't
need to be parked.

----8<----
diff --git a/drivers/clk/qcom/gcc-sm8550.c b/drivers/clk/qcom/gcc-sm8550.c
index 33c5d422da9b..1ce41fc7e77e 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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_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_no_init_park_ops,
+	.ops = &clk_rcg2_ops,
 };

 static struct clk_rcg2 gcc_qupv3_wrap2_s7_clk_src = {

>
> UFS on SM8550-HDK has it's own issues when booting AOSP from it, so I
> couldn't test/verify if the reported gcc_ufs_phy_*_clk_src clk changes
> make any difference. But I'll bookmark this change for future
> reference if I run into any relevant UFS probe deferrals once we fix
> the existing/on-going issues.
>

Ominous but OK.

>
> Thank you for diagnosing this race in ucsi_glink. I needed to run an
> overnight reboot test to reproduce this crash, and could reproduce it
> on ~380th reboot. I'll check if it has already been reported or fixed
> on linux-next.

Amazing! Can you add the msleep() so that it is highly likely?

----8<----
diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index 9ebc0ba35947..12169b0d2adb 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -97,6 +97,8 @@ struct pmic_glink_client
*devm_pmic_glink_register_client(struct device *dev,

 	devres_add(dev, client);

+	msleep(10000);
+
 	return client;
 }
 EXPORT_SYMBOL_GPL(devm_pmic_glink_register_client);

Powered by blists - more mailing lists