Re: [PATCH 2/7] clk: thead: th1520-ap: Poll for PLL lock and wait for stability

From: Drew Fustini <fustini@kernel.org>
To: Yao Zi <ziyao@disroot.org>
Cc: Guo Ren <guoren@kernel.org>, Fu Wei <wefu@redhat.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Paul Walmsley <pjw@kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Alexandre Ghiti <alex@ghiti.fr>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Icenowy Zheng <uwu@icenowy.me>,
	linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
	Han Gao <rabenda.cn@gmail.com>, Han Gao <gaohan@iscas.ac.cn>
Subject: Re: [PATCH 2/7] clk: thead: th1520-ap: Poll for PLL lock and wait for stability
Date: Wed, 26 Nov 2025 08:39:11 -0600	[thread overview]
Message-ID: <aScRD64dGZGxndNL@gen8> (raw)
In-Reply-To: <aSUgQpd16Ud8xTx6@pie>

On Tue, Nov 25, 2025 at 03:19:30AM +0000, Yao Zi wrote:
> On Mon, Nov 24, 2025 at 02:08:00PM -0800, Drew Fustini wrote:
> > On Thu, Nov 20, 2025 at 01:14:11PM +0000, Yao Zi wrote:
> > > All PLLs found on TH1520 SoC take 21250ns at maximum to lock, and their
> > > lock status is indicated by register PLL_STS (offset 0x80 inside AP
> > > clock controller). We should poll the register to ensure the PLL
> > > actually locks after enabling it.
> > > 
> > > Furthermore, a 30us delay is added after enabling the PLL, after which
> > > the PLL could be considered stable as stated by vendor clock code.
> > > 
> > > Fixes: 56a48c1833aa ("clk: thead: add support for enabling/disabling PLLs")
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > ---
> > >  drivers/clk/thead/clk-th1520-ap.c | 34 +++++++++++++++++++++++++++++--
> > >  1 file changed, 32 insertions(+), 2 deletions(-)
> > 
> > Thanks for working on this patch series.
> > 
> > [...]
> > > @@ -299,9 +310,21 @@ static void ccu_pll_disable(struct clk_hw *hw)
> > >  static int ccu_pll_enable(struct clk_hw *hw)
> > >  {
> > >  	struct ccu_pll *pll = hw_to_ccu_pll(hw);
> > > +	u32 reg;
> > > +	int ret;
> > >  
> > > -	return regmap_clear_bits(pll->common.map, pll->common.cfg1,
> > > -				 TH1520_PLL_VCO_RST);
> > > +	regmap_clear_bits(pll->common.map, pll->common.cfg1,
> > > +			  TH1520_PLL_VCO_RST);
> > > +
> > > +	ret = regmap_read_poll_timeout_atomic(pll->common.map, TH1520_PLL_STS,
> > > +					      reg, reg & pll->lock_sts_mask,
> > > +					      5, TH1520_PLL_LOCK_TIMEOUT_US);
> > 
> > Is there a reason for the specific value of 5 uS polling delay?
> 
> No, it was picked randomly. A smaller value would reduce latency of
> PLL enabling, and I could tune it more carefully by some testing. But
> it's hard to predict how much improvement it will bring.

Okay, I was just curious. I think it is okay to stick with that current
value if it is working correctly.

> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	udelay(TH1520_PLL_STABLE_DELAY_US);
> > 
> > Is it the case that the 30 uS delay after the lock bit is set is just so
> > that it has the same behavior as the vendor's code? Or did you notice
> > stability problems without this?
> 
> This aligns with the vendor code, and I haven't yet observed stability
> issues without the delay. But I think it's more safe to keep the
> behavior similar since it's hard to test all working conditions.

Okay, that seems reasonable to play it safe.

Thanks,
Drew