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@xxxxxxxxxx>
> ---
>   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