[PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Gabor Juhos]

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],
+	.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,
 };
 

---
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
change-id: 20240311-apss-ipq-pll-ipq5018-hang-74d9a8f47136

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>

Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Kathiravan Thirumoorthy]

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(-)

Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Konrad Dybcio]

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

Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Gabor Juhos]

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

Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Konrad Dybcio]

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

Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Gabor Juhos]

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

Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Stephen Boyd]

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?

Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

[Gabor Juhos]

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