[PATCH] clk: zynq: return -ETIMEDOUT if the PLL never locks

[PATCH] clk: zynq: return -ETIMEDOUT if the PLL never locks

[Linkai Gong]

zynq_pll_enable() waits for lock under a spinlock with no
timeout. Poll with a 1ms bound and return the error.

Fixes: 3682af46d55f ("clk: zynq: Factor out PLL driver")
Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
---
 drivers/clk/zynq/pll.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/zynq/pll.c b/drivers/clk/zynq/pll.c
index 44c609378364..96cf45088ba0 100644
--- a/drivers/clk/zynq/pll.c
+++ b/drivers/clk/zynq/pll.c
@@ -10,6 +10,7 @@
 #include <linux/clk-provider.h>
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 
 /**
  * struct zynq_pll - pll clock
@@ -119,6 +120,7 @@ static int zynq_pll_enable(struct clk_hw *hw)
 	unsigned long flags = 0;
 	u32 reg;
 	struct zynq_pll *clk = to_zynq_pll(hw);
+	int ret;
 
 	if (zynq_pll_is_enabled(hw))
 		return 0;
@@ -131,12 +133,12 @@ static int zynq_pll_enable(struct clk_hw *hw)
 	reg = readl(clk->pll_ctrl);
 	reg &= ~(PLLCTRL_RESET_MASK | PLLCTRL_PWRDWN_MASK);
 	writel(reg, clk->pll_ctrl);
-	while (!(readl(clk->pll_status) & (1 << clk->lockbit)))
-		;
+	ret = readl_poll_timeout_atomic(clk->pll_status, reg,
+					reg & (1 << clk->lockbit), 10, 1000);
 
 	spin_unlock_irqrestore(clk->lock, flags);
 
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.25.1

Re: [PATCH] clk: zynq: return -ETIMEDOUT if the PLL never locks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing hardware state rollback on PLL lock timeout.
--

commit 91f1719daf95abfb29082ef831a0b53acd17a349
Author: Linkai Gong <gonglinkai@kylinos.cn>

clk: zynq: return -ETIMEDOUT if the PLL never locks

zynq_pll_enable() waits for lock under a spinlock with no timeout. Poll
with a 1ms bound and return the error.

> diff --git a/drivers/clk/zynq/pll.c b/drivers/clk/zynq/pll.c
> index fe90b50e1545d..4e3a1cb40be74 100644
> --- a/drivers/clk/zynq/pll.c
> +++ b/drivers/clk/zynq/pll.c
[ ... ]
> @@ -131,12 +133,12 @@ static int zynq_pll_enable(struct clk_hw *hw)
>  	reg = readl(clk->pll_ctrl);
>  	reg &= ~(PLLCTRL_RESET_MASK | PLLCTRL_PWRDWN_MASK);
>  	writel(reg, clk->pll_ctrl);
> -	while (!(readl(clk->pll_status) & (1 << clk->lockbit)))
> -		;
> +	ret = readl_poll_timeout_atomic(clk->pll_status, reg,
> +					reg & (1 << clk->lockbit), 10, 1000);
>  
>  	spin_unlock_irqrestore(clk->lock, flags);
>  
> -	return 0;
> +	return ret;

[Severity: High]
Does this leave the hardware state desynchronized if the PLL fails to lock?

If readl_poll_timeout_atomic() returns -ETIMEDOUT, the function returns the
error directly without restoring the PLLCTRL_RESET_MASK and
PLLCTRL_PWRDWN_MASK bits in the control register.

Because zynq_pll_is_enabled() checks those same bits to determine if the
PLL is enabled:

drivers/clk/zynq/pll.c:zynq_pll_is_enabled() {
    ...
	reg = readl(clk->pll_ctrl);
    ...
	return !(reg & (PLLCTRL_RESET_MASK | PLLCTRL_PWRDWN_MASK));
}

A subsequent call to zynq_pll_enable() will falsely succeed since
zynq_pll_is_enabled() will return true, hiding the lock failure. Should we
re-assert the reset and power-down bits in the error path before returning?

>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901125323.1556551-1-gonglinkai@kylinos.cn?part=1

Re: [PATCH] clk: zynq: return -ETIMEDOUT if the PLL never locks

[Michal Simek]

On 9/1/26 14:53, Linkai Gong wrote:
> zynq_pll_enable() waits for lock under a spinlock with no
> timeout. Poll with a 1ms bound and return the error.
> 
> Fixes: 3682af46d55f ("clk: zynq: Factor out PLL driver")
> Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
> ---
>   drivers/clk/zynq/pll.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/zynq/pll.c b/drivers/clk/zynq/pll.c
> index 44c609378364..96cf45088ba0 100644
> --- a/drivers/clk/zynq/pll.c
> +++ b/drivers/clk/zynq/pll.c
> @@ -10,6 +10,7 @@
>   #include <linux/clk-provider.h>
>   #include <linux/slab.h>
>   #include <linux/io.h>
> +#include <linux/iopoll.h>
>   
>   /**
>    * struct zynq_pll - pll clock
> @@ -119,6 +120,7 @@ static int zynq_pll_enable(struct clk_hw *hw)
>   	unsigned long flags = 0;
>   	u32 reg;
>   	struct zynq_pll *clk = to_zynq_pll(hw);
> +	int ret;
>   
>   	if (zynq_pll_is_enabled(hw))
>   		return 0;
> @@ -131,12 +133,12 @@ static int zynq_pll_enable(struct clk_hw *hw)
>   	reg = readl(clk->pll_ctrl);
>   	reg &= ~(PLLCTRL_RESET_MASK | PLLCTRL_PWRDWN_MASK);
>   	writel(reg, clk->pll_ctrl);
> -	while (!(readl(clk->pll_status) & (1 << clk->lockbit)))
> -		;
> +	ret = readl_poll_timeout_atomic(clk->pll_status, reg,
> +					reg & (1 << clk->lockbit), 10, 1000);

BIT(clk->lockbit)

And 10 and 1000 are magic values.

Fix itself is fine but I would prefer to explain more why 1ms upper limit was 
used. I don't think it is going to be a problem and 10-1000us is fine. I just 
want to make sure that it will be clear that this value is not coming from any 
TRM but still at least range is aligned with expectation in HW.

Thanks,
Michal

Re: [PATCH] clk: zynq: return -ETIMEDOUT if the PLL never locks

[Linkai Gong]

On Mon, Sep 07, 2026 at 02:58:18PM +0200, Michal Simek wrote:
> BIT(clk->lockbit)
>
> And 10 and 1000 are magic values.
>
> Fix itself is fine but I would prefer to explain more why 1ms upper limit was
> used. I don't think it is going to be a problem and 10-1000us is fine. I just
> want to make sure that it will be clear that this value is not coming from any
> TRM but still at least range is aligned with expectation in HW.

Thanks for the review.

Agreed on both points. v2 will use BIT(), name the poll delay/timeout,
and clarify in the commit message that 1 ms is a software upper bound
for a stuck PLL under spinlock, not a TRM-derived value; lock is still
expected well within that window on Zynq.

Will send v2 shortly.

Thanks,
Linkai

[PATCH v2] clk: zynq: return -ETIMEDOUT if the PLL never locks

[Linkai Gong]

zynq_pll_enable() waits for lock under a spinlock with no timeout.
A stuck PLL would wedge the enable path with IRQs off.

Poll with readl_poll_timeout_atomic() and return -ETIMEDOUT on
failure. The 1 ms upper bound is a software limit for a stuck PLL,
not a TRM-derived value; Zynq PLL lock is still expected well
within that window.

Changes in v2:
- Use BIT(clk->lockbit)
- Name the poll delay/timeout constants
- Clarify the 1 ms bound in the commit message

Fixes: 3682af46d55f ("clk: zynq: Factor out PLL driver")
Signed-off-by: Linkai Gong <gonglinkai@kylinos.cn>
---
 drivers/clk/zynq/pll.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/zynq/pll.c b/drivers/clk/zynq/pll.c
index fe90b50e1545..6a98bc60fb91 100644
--- a/drivers/clk/zynq/pll.c
+++ b/drivers/clk/zynq/pll.c
@@ -9,7 +9,9 @@
 #include <linux/clk/zynq.h>
 #include <linux/clk-provider.h>
 #include <linux/slab.h>
+#include <linux/bits.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 
 /**
  * struct zynq_pll - pll clock
@@ -41,6 +43,10 @@ struct zynq_pll {
 #define PLL_FBDIV_MIN	13
 #define PLL_FBDIV_MAX	66
 
+/* Software bound for a stuck PLL under spinlock; not from the TRM. */
+#define PLL_LOCK_POLL_DELAY_US	10
+#define PLL_LOCK_TIMEOUT_US	1000
+
 /**
  * zynq_pll_determine_rate() - Round a clock frequency
  * @hw:		Handle between common and hardware-specific interfaces
@@ -119,6 +125,7 @@ static int zynq_pll_enable(struct clk_hw *hw)
 	unsigned long flags = 0;
 	u32 reg;
 	struct zynq_pll *clk = to_zynq_pll(hw);
+	int ret;
 
 	if (zynq_pll_is_enabled(hw))
 		return 0;
@@ -131,12 +138,14 @@ static int zynq_pll_enable(struct clk_hw *hw)
 	reg = readl(clk->pll_ctrl);
 	reg &= ~(PLLCTRL_RESET_MASK | PLLCTRL_PWRDWN_MASK);
 	writel(reg, clk->pll_ctrl);
-	while (!(readl(clk->pll_status) & (1 << clk->lockbit)))
-		;
+	ret = readl_poll_timeout_atomic(clk->pll_status, reg,
+					reg & BIT(clk->lockbit),
+					PLL_LOCK_POLL_DELAY_US,
+					PLL_LOCK_TIMEOUT_US);
 
 	spin_unlock_irqrestore(clk->lock, flags);
 
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.25.1