| Message ID | 20240311-apss-ipq-pll-ipq5018-hang-v1-1-8ed42b7a904d@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure | expand |
On 3/11/2024 8:36 PM, Gabor Juhos wrote: > Booting v6.8 results in a hang on various IPQ5018 based boards. > Investigating the problem showed that the hang happens when the > clk_alpha_pll_stromer_plus_set_rate() function tries to write > into the PLL_MODE register of the APSS PLL. > > Checking the downstream code revealed that it uses [1] stromer > specific operations for IPQ5018, whereas in the current code > the stromer plus specific operations are used. > > The ops in the 'ipq_pll_stromer_plus' clock definition can't be > changed since that is needed for IPQ5332, so add a new alpha pll > clock declaration which uses the correct stromer ops and use this > new clock for IPQ5018 to avoid the boot failure. > > 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4/drivers/clk/qcom/apss-ipq5018.c#L67 Thanks for catching this! Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> > > Cc: stable@vger.kernel.org > Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018") > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > --- > Based on v6.8. > --- > drivers/clk/qcom/apss-ipq-pll.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-)
On 3/11/24 16:06, Gabor Juhos wrote: > Booting v6.8 results in a hang on various IPQ5018 based boards. > Investigating the problem showed that the hang happens when the > clk_alpha_pll_stromer_plus_set_rate() function tries to write > into the PLL_MODE register of the APSS PLL. > > Checking the downstream code revealed that it uses [1] stromer > specific operations for IPQ5018, whereas in the current code > the stromer plus specific operations are used. > > The ops in the 'ipq_pll_stromer_plus' clock definition can't be > changed since that is needed for IPQ5332, so add a new alpha pll > clock declaration which uses the correct stromer ops and use this > new clock for IPQ5018 to avoid the boot failure. > > 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4/drivers/clk/qcom/apss-ipq5018.c#L67 > > Cc: stable@vger.kernel.org > Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018") > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > --- > Based on v6.8. > --- > drivers/clk/qcom/apss-ipq-pll.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c > index 678b805f13d45..11f1ae59438f7 100644 > --- a/drivers/clk/qcom/apss-ipq-pll.c > +++ b/drivers/clk/qcom/apss-ipq-pll.c > @@ -55,6 +55,24 @@ static struct clk_alpha_pll ipq_pll_huayra = { > }, > }; > > +static struct clk_alpha_pll ipq_pll_stromer = { > + .offset = 0x0, > + .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], CLK_ALPHA_PLL_TYPE_STROMER? [...] > static const struct apss_pll_data ipq5018_pll_data = { > .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS, and here? The L register differs, so the rattesetting done from Linux must have never worked anyway? Konrad
Quoting Gabor Juhos (2024-03-11 08:06:36) > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c > index 678b805f13d45..11f1ae59438f7 100644 > --- a/drivers/clk/qcom/apss-ipq-pll.c > +++ b/drivers/clk/qcom/apss-ipq-pll.c > @@ -55,6 +55,24 @@ static struct clk_alpha_pll ipq_pll_huayra = { > }, > }; > > +static struct clk_alpha_pll ipq_pll_stromer = { > + .offset = 0x0, > + .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], > + .flags = SUPPORTS_DYNAMIC_UPDATE, > + .clkr = { > + .enable_reg = 0x0, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ const?
2024. 03. 13. 19:36 keltezéssel, Konrad Dybcio írta: > > > On 3/11/24 16:06, Gabor Juhos wrote: >> Booting v6.8 results in a hang on various IPQ5018 based boards. >> Investigating the problem showed that the hang happens when the >> clk_alpha_pll_stromer_plus_set_rate() function tries to write >> into the PLL_MODE register of the APSS PLL. >> >> Checking the downstream code revealed that it uses [1] stromer >> specific operations for IPQ5018, whereas in the current code >> the stromer plus specific operations are used. >> >> The ops in the 'ipq_pll_stromer_plus' clock definition can't be >> changed since that is needed for IPQ5332, so add a new alpha pll >> clock declaration which uses the correct stromer ops and use this >> new clock for IPQ5018 to avoid the boot failure. >> >> 1. >> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4/drivers/clk/qcom/apss-ipq5018.c#L67 >> >> Cc: stable@vger.kernel.org >> Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018") >> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> >> --- >> Based on v6.8. >> --- >> drivers/clk/qcom/apss-ipq-pll.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c >> index 678b805f13d45..11f1ae59438f7 100644 >> --- a/drivers/clk/qcom/apss-ipq-pll.c >> +++ b/drivers/clk/qcom/apss-ipq-pll.c >> @@ -55,6 +55,24 @@ static struct clk_alpha_pll ipq_pll_huayra = { >> }, >> }; >> +static struct clk_alpha_pll ipq_pll_stromer = { >> + .offset = 0x0, >> + .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], > > CLK_ALPHA_PLL_TYPE_STROMER? I admit that using CLK_ALPHA_PLL_TYPE_STROMER would be less confusing. However 'ipq_pll_offsets' array has no entry for that enum, and given the fact that the CLK_ALPHA_PLL_TYPE_STROMER_PLUS entry uses the correct register offsets it makes little sense to add another entry with the same offsets. Although the 'clk_alpha_pll_regs' in clk-alpha-pll.c has an entry for CLK_ALPHA_PLL_TYPE_STROMER, but the offsets defined there are not 'exactly' the same as the ones defined locally in 'ipq_pll_offsets'. They will be identical if [1] gets accepted but we are not there yet. > > [...] > >> static const struct apss_pll_data ipq5018_pll_data = { >> .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS, > > and here? The value of the 'pll_type' field is used only in the probe function to decide which configuration function should be called for the actual PLL. Changing this means that the probe function must be changed as well. Originally, I did not want to change this as it is not required for fixing the bug, but it might worth to do it to avoid further confusion. I will modify that in the next version. I also have a small patch-set on top of the current one which reduces the mess in the driver, but that still needs some testing. Will send it later. > The L register differs, so the rattesetting done from Linux must > have never worked anyway? Probably not, although I did not test the original commit which added IPQ5018 support to the driver. I have noticed the bug in next-20240130 first, but I had no time to find the root cause that time. Later it turned out that cpufreq scaling was broken in v6.8-rc1 on some platforms [2] which has been fixed with 98323e9d7017 ("topology: Set capacity_freq_ref in all cases"). This was the change which made the bug visible on IPQ5018. 1. https://lore.kernel.org/lkml/20240311-alpha-pll-stromer-cleanup-v1-0-f7c0c5607cca@gmail.com/ 2. https://lore.kernel.org/lkml/75f0bfc7-fb95-409a-a5d9-b00732e892f0@bp.renesas.com/ Regards, Gabor
Hi Stephen, 2024. 03. 13. 23:57 keltezéssel, Stephen Boyd írta: > Quoting Gabor Juhos (2024-03-11 08:06:36) >> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c >> index 678b805f13d45..11f1ae59438f7 100644 >> --- a/drivers/clk/qcom/apss-ipq-pll.c >> +++ b/drivers/clk/qcom/apss-ipq-pll.c >> @@ -55,6 +55,24 @@ static struct clk_alpha_pll ipq_pll_huayra = { >> }, >> }; >> >> +static struct clk_alpha_pll ipq_pll_stromer = { >> + .offset = 0x0, >> + .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], >> + .flags = SUPPORTS_DYNAMIC_UPDATE, >> + .clkr = { >> + .enable_reg = 0x0, >> + .enable_mask = BIT(0), >> + .hw.init = &(struct clk_init_data){ > > const? You are right, I will add it in the next version. Thanks, Gabor
On 3/14/24 14:50, Gabor Juhos wrote: > 2024. 03. 13. 19:36 keltezéssel, Konrad Dybcio írta: >> >> >> On 3/11/24 16:06, Gabor Juhos wrote: >>> Booting v6.8 results in a hang on various IPQ5018 based boards. >>> Investigating the problem showed that the hang happens when the >>> clk_alpha_pll_stromer_plus_set_rate() function tries to write >>> into the PLL_MODE register of the APSS PLL. >>> >>> Checking the downstream code revealed that it uses [1] stromer >>> specific operations for IPQ5018, whereas in the current code >>> the stromer plus specific operations are used. >>> >>> The ops in the 'ipq_pll_stromer_plus' clock definition can't be >>> changed since that is needed for IPQ5332, so add a new alpha pll >>> clock declaration which uses the correct stromer ops and use this >>> new clock for IPQ5018 to avoid the boot failure. >>> >>> 1. >>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4/drivers/clk/qcom/apss-ipq5018.c#L67 >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018") >>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> >>> --- >>> Based on v6.8. >>> --- >>> drivers/clk/qcom/apss-ipq-pll.c | 20 +++++++++++++++++++- >>> 1 file changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c >>> index 678b805f13d45..11f1ae59438f7 100644 >>> --- a/drivers/clk/qcom/apss-ipq-pll.c >>> +++ b/drivers/clk/qcom/apss-ipq-pll.c >>> @@ -55,6 +55,24 @@ static struct clk_alpha_pll ipq_pll_huayra = { >>> }, >>> }; >>> +static struct clk_alpha_pll ipq_pll_stromer = { >>> + .offset = 0x0, >>> + .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], >> >> CLK_ALPHA_PLL_TYPE_STROMER? > > I admit that using CLK_ALPHA_PLL_TYPE_STROMER would be less confusing. However > 'ipq_pll_offsets' array has no entry for that enum, and given the fact that the > CLK_ALPHA_PLL_TYPE_STROMER_PLUS entry uses the correct register offsets it makes > little sense to add another entry with the same offsets. > > Although the 'clk_alpha_pll_regs' in clk-alpha-pll.c has an entry for > CLK_ALPHA_PLL_TYPE_STROMER, but the offsets defined there are not 'exactly' the > same as the ones defined locally in 'ipq_pll_offsets'. They will be identical if > [1] gets accepted but we are not there yet. Oh, I completely overlooked that this driver has its own array.. Hm.. I suppose it would make sense to rename these indices to IPQ_PLL_x to help avoid such confusion.. Konrad
2024. 03. 14. 15:00 keltezéssel, Konrad Dybcio írta: ... >>>> @@ -55,6 +55,24 @@ static struct clk_alpha_pll ipq_pll_huayra = { >>>> }, >>>> }; >>>> +static struct clk_alpha_pll ipq_pll_stromer = { >>>> + .offset = 0x0, >>>> + .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], >>> >>> CLK_ALPHA_PLL_TYPE_STROMER? >> >> I admit that using CLK_ALPHA_PLL_TYPE_STROMER would be less confusing. However >> 'ipq_pll_offsets' array has no entry for that enum, and given the fact that the >> CLK_ALPHA_PLL_TYPE_STROMER_PLUS entry uses the correct register offsets it makes >> little sense to add another entry with the same offsets. >> >> Although the 'clk_alpha_pll_regs' in clk-alpha-pll.c has an entry for >> CLK_ALPHA_PLL_TYPE_STROMER, but the offsets defined there are not 'exactly' the >> same as the ones defined locally in 'ipq_pll_offsets'. They will be identical if >> [1] gets accepted but we are not there yet. > > Oh, I completely overlooked that this driver has its own array.. Hm.. > > I suppose it would make sense to rename these indices to IPQ_PLL_x to > help avoid such confusion.. Yes, that would work. To be honest, I have tried that already a few days ago, but then I had a better idea. It will be possible to use the entry from 'clk_alpha_pll_regs' for 'ipq_pll_stromer' and for 'ipq_pll_stromer_plus'. To be precise, it would be usable already but for correctness it needs the series mentioned in my previous mail. Then the local entry can be removed from 'ipq_pll_regs' entirely. Once it is done, the 'ipq_pll_regs' can be converted to be an one-dimensional array containing the IPQ Huayra specific offsets only. Alternatively the remaining sole element can be moved into 'clk_alpha_pll_regs' array. Additionally, the 'pll_type' field in the match data structure is redundant so that can be removed as well. This eliminates the need of a separate enum for IPQ specific indices. Regards, Gabor
diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c index 678b805f13d45..11f1ae59438f7 100644 --- a/drivers/clk/qcom/apss-ipq-pll.c +++ b/drivers/clk/qcom/apss-ipq-pll.c @@ -55,6 +55,24 @@ static struct clk_alpha_pll ipq_pll_huayra = { }, }; +static struct clk_alpha_pll ipq_pll_stromer = { + .offset = 0x0, + .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], + .flags = SUPPORTS_DYNAMIC_UPDATE, + .clkr = { + .enable_reg = 0x0, + .enable_mask = BIT(0), + .hw.init = &(struct clk_init_data){ + .name = "a53pll", + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xo", + }, + .num_parents = 1, + .ops = &clk_alpha_pll_stromer_ops, + }, + }, +}; + static struct clk_alpha_pll ipq_pll_stromer_plus = { .offset = 0x0, .regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], @@ -145,7 +163,7 @@ struct apss_pll_data { static const struct apss_pll_data ipq5018_pll_data = { .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS, - .pll = &ipq_pll_stromer_plus, + .pll = &ipq_pll_stromer, .pll_config = &ipq5018_pll_config, };
Booting v6.8 results in a hang on various IPQ5018 based boards. Investigating the problem showed that the hang happens when the clk_alpha_pll_stromer_plus_set_rate() function tries to write into the PLL_MODE register of the APSS PLL. Checking the downstream code revealed that it uses [1] stromer specific operations for IPQ5018, whereas in the current code the stromer plus specific operations are used. The ops in the 'ipq_pll_stromer_plus' clock definition can't be changed since that is needed for IPQ5332, so add a new alpha pll clock declaration which uses the correct stromer ops and use this new clock for IPQ5018 to avoid the boot failure. 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4/drivers/clk/qcom/apss-ipq5018.c#L67 Cc: stable@vger.kernel.org Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018") Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> --- Based on v6.8. --- drivers/clk/qcom/apss-ipq-pll.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240311-apss-ipq-pll-ipq5018-hang-74d9a8f47136 Best regards,