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@xxxxxxxxxxxxxxx >> Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018") >> Signed-off-by: Gabor Juhos <j4g8y7@xxxxxxxxx> >> --- >> 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@xxxxxxxxx/ 2. https://lore.kernel.org/lkml/75f0bfc7-fb95-409a-a5d9-b00732e892f0@xxxxxxxxxxxxxx/ Regards, Gabor
