Re: [RFC PATCH] clk: qcom: hfpll: return lock timeout from enable paths

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

On 7/4/26 11:02 AM, Antony Kurniawan Soemardi wrote:

[...]

>>>>>> Seems that way - cpu, L2, and GPU, maybe others
>>>>>
>>>>> nope, tested on Sony Xperia SP (MSM8960T), the phone hangs
>>>>
>>>> [...]
>>>>
>>>>> [    2.679716] L2 @ Undefined rate. Forcing new rate
>>>>
>>>> This seems odd. What's the reported rate there?
>>>
>>> if you're asking clk_get_rate(clks[l2_mux]), it's 0 Hz.
>>
>> Hm, are the parents registered?
> 
> pardon me for stupid question, but how do I verify the parent
> registration?

Parents are just other locks that happen to be specified in the
parent_data/parent_names field of this L2 clock.

For checking if they're registered at all, you can check if they're
present in the debugfs summary I mentioned (they would also have their
own directory in /sys/kernel/debug/clk). For checking if they're
registered at some specific point in time, you could hack in a
clk_get() and null-check it

[...]

> Do you have other idea how to solve this? I added some pr_err if the
> read poll is timeout:
> 
> [   11.164436] [pmOS-rd]:   ❬❬ PMOS STAGE 2 ❭❭
> [   20.400232] krait_add_pri_mux: krait0_pri_mux, hfpll_name: hfpll0
> [   20.400793] Enabling HFPLL hfpll1
> [   20.455491] krait_add_pri_mux: krait1_pri_mux, hfpll_name: hfpll1
> [   20.456081] Enabling HFPLL hfpll_l2
> [   20.560623] HFPLL hfpll_l2 failed to lock (val=0x6, ret=-110, hd->lock_bit=0)
> [   20.758961] krait_add_pri_mux: krait_l2_pri_mux, hfpll_name: hfpll_l2

I noticed a bug (patch attached) in the enable function. I then realized
the patch from the OP also fixes it.

I then tried to validate that the code we have in Linux even checks the
right register.. Unfortunately because this SoC is so old, it's difficult
for me to find docs for it (and the register layout I have at hand isn't
itself enough to deduce some things), so that's inconclusive.

I then took to msm-3.x and lk sources, but they simply never check the
value of the status register, they just call udelay(60) with presumably
very high hopes and a lot of prayers..

Can you check the state of the gcc_base+0x3420 register before and after
the /* De-assert active-low PLL reset. */ line?

Konrad

[-- Attachment #2: 0001-clk-qcom-clk-hfpll-Fix-inverted-break-condition-in-..patch --]
[-- Type: text/x-patch, Size: 1299 bytes --]

From 5c65b279afcc53a7afbd63d743e056ca00b299a0 Mon Sep 17 00:00:00 2001
From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Date: Mon, 6 Jul 2026 12:46:24 +0200
Subject: [PATCH] clk: qcom: clk-hfpll: Fix inverted break condition in .enable

The commit referenced in the Fixes tag did away with an open-coded
while-loop, replacing it with a nicer wrapper.

The syntax of that wrapper however, includes a 'break condition',
contrary to the while-loop which expects a 'keep going condition'.

As a result, regmap_read_poll_timeout() is looking for the opposite of
the desired condition. Fix that.

Fixes: fcfbfe373d41 ("clk: qcom: clk-hfpll: use poll_timeout macro")
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/clk/qcom/clk-hfpll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
index 705352aff067..62fbf511003e 100644
--- a/drivers/clk/qcom/clk-hfpll.c
+++ b/drivers/clk/qcom/clk-hfpll.c
@@ -82,7 +82,7 @@ static void __clk_hfpll_enable(struct clk_hw *hw)
 		 * prevent any sort of stall.
 		 */
 		regmap_read_poll_timeout(regmap, hd->status_reg, val,
-					 !(val & BIT(hd->lock_bit)), 0,
+					 val & BIT(hd->lock_bit), 0,
 					 100 * USEC_PER_MSEC);
 	else
 		udelay(60);
-- 
2.54.0