Quoting Amit Pundir (2024-08-12 02:39:47)
> On Tue, 6 Aug 2024 at 03:07, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> >
> > Quoting Amit Pundir (2024-08-05 03:43:14)
> > > On Sat, 3 Aug 2024 at 06:29, Stephen Boyd <swboyd@xxxxxxxxxxxx> 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);
