Received-SPF: pass (google.com: domain of linux-kernel+bounces-103321-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <2b95a593-225e-47b1-8bda-03240eb0f81e@gmail.com>
Date: Thu, 14 Mar 2024 14:50:18 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to
 fix boot failure
Content-Language: hu
To: Konrad Dybcio <konrad.dybcio@linaro.org>,
 Bjorn Andersson <andersson@kernel.org>,
 Michael Turquette <mturquette@baylibre.com>, Stephen Boyd
 <sboyd@kernel.org>, Sricharan Ramabadhran <quic_srichara@quicinc.com>,
 Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
 Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
Cc: linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
 linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <20240311-apss-ipq-pll-ipq5018-hang-v1-1-8ed42b7a904d@gmail.com>
 <58f07908-127a-438d-84e2-e059f269859b@linaro.org>
From: Gabor Juhos <j4g8y7@gmail.com>
In-Reply-To: <58f07908-127a-438d-84e2-e059f269859b@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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