Re: [PATCH v3] clk: samsung: Prevent potential endless loop in the PLL set_rate ops

[Chanwoo Choi]

Hi Sylwester,

On 8/13/20 6:55 PM, Sylwester Nawrocki wrote:
> In the .set_rate callback for some PLLs there is a loop polling state
> of the PLL lock bit and it may become an endless loop when something
> goes wrong with the PLL. For some PLLs there is already code for polling
> with a timeout but it uses the ktime API, which doesn't work in some
> conditions when the set_rate op is called, in particular during
> initialization of the clk provider before the clocksource initialization
> has completed. Hence the ktime API cannot be used to reliably detect
> the PLL locking timeout.
> 
> This patch adds a common helper function for busy waiting on the PLL lock
> bit with timeout detection.
> 
> Actual PLL lock time depends on the P divider value, the VCO frequency
> and a constant PLL type specific LOCK_FACTOR and can be calculated as
> 
>  lock_time = Pdiv * LOCK_FACTOR / VCO_freq
> 
> Common timeout value of 10 ms is used for all the PLLs with an assumption
> that maximum possible value of Pdiv is 64, maximum possible LOCK_FACTOR
> value is 3000 and minimum VCO frequency is 24 MHz.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> ---
> Changes for v3:
>  - use busy-loop with udelay() instead of ktime API
> Changes for v2:
>  - use common readl_relaxed_poll_timeout_atomic() macro
> ---
>  drivers/clk/samsung/clk-pll.c | 94 ++++++++++++++++---------------------------
>  1 file changed, 34 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index ac70ad7..c83d261 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -15,7 +15,7 @@
>  #include "clk.h"
>  #include "clk-pll.h"
> 
> -#define PLL_TIMEOUT_MS		10
> +#define PLL_TIMEOUT_US		10000U
> 
>  struct samsung_clk_pll {
>  	struct clk_hw		hw;
> @@ -63,6 +63,25 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
>  	return rate_table[i - 1].rate;
>  }
> 
> +static int samsung_pll_lock_wait(struct samsung_clk_pll *pll,
> +				 unsigned int reg_mask)
> +{
> +	int i;
> +
> +	/* Wait until the PLL is in steady locked state */
> +	for (i = 0; i < PLL_TIMEOUT_US / 2; i++) {
> +		if (readl_relaxed(pll->con_reg) & reg_mask)
> +			return 0;
> +
> +		udelay(2);
> +		cpu_relax();
> +	}
> +
> +	pr_err("Could not lock PLL %s\n", clk_hw_get_name(&pll->hw));
> +
> +	return -ETIMEDOUT;
> +}
> +
>  static int samsung_pll3xxx_enable(struct clk_hw *hw)
>  {
>  	struct samsung_clk_pll *pll = to_clk_pll(hw);
> @@ -241,12 +260,9 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  	writel_relaxed(tmp, pll->con_reg);
> 
>  	/* Wait until the PLL is locked if it is enabled. */
> -	if (tmp & BIT(pll->enable_offs)) {
> -		do {
> -			cpu_relax();
> -			tmp = readl_relaxed(pll->con_reg);
> -		} while (!(tmp & BIT(pll->lock_offs)));
> -	}
> +	if (tmp & BIT(pll->enable_offs))
> +		return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
> +
>  	return 0;
>  }
> 
> @@ -318,7 +334,7 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  					unsigned long parent_rate)
>  {
>  	struct samsung_clk_pll *pll = to_clk_pll(hw);
> -	u32 tmp, pll_con0, pll_con1;
> +	u32 pll_con0, pll_con1;
>  	const struct samsung_pll_rate_table *rate;
> 
>  	rate = samsung_get_pll_settings(pll, drate);
> @@ -356,13 +372,8 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  	pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
>  	writel_relaxed(pll_con1, pll->con_reg + 4);
> 
> -	/* wait_lock_time */
> -	if (pll_con0 & BIT(pll->enable_offs)) {
> -		do {
> -			cpu_relax();
> -			tmp = readl_relaxed(pll->con_reg);
> -		} while (!(tmp & BIT(pll->lock_offs)));
> -	}
> +	if (pll_con0 & BIT(pll->enable_offs))
> +		return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
> 
>  	return 0;
>  }
> @@ -437,7 +448,6 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  	struct samsung_clk_pll *pll = to_clk_pll(hw);
>  	const struct samsung_pll_rate_table *rate;
>  	u32 con0, con1;
> -	ktime_t start;
> 
>  	/* Get required rate settings from table */
>  	rate = samsung_get_pll_settings(pll, drate);
> @@ -489,20 +499,7 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  	writel_relaxed(con0, pll->con_reg);
> 
>  	/* Wait for locking. */
> -	start = ktime_get();
> -	while (!(readl_relaxed(pll->con_reg) & PLL45XX_LOCKED)) {
> -		ktime_t delta = ktime_sub(ktime_get(), start);
> -
> -		if (ktime_to_ms(delta) > PLL_TIMEOUT_MS) {
> -			pr_err("%s: could not lock PLL %s\n",
> -					__func__, clk_hw_get_name(hw));
> -			return -EFAULT;
> -		}
> -
> -		cpu_relax();
> -	}
> -
> -	return 0;
> +	return samsung_pll_lock_wait(pll, PLL45XX_LOCKED);
>  }
> 
>  static const struct clk_ops samsung_pll45xx_clk_ops = {
> @@ -588,7 +585,6 @@ static int samsung_pll46xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  	struct samsung_clk_pll *pll = to_clk_pll(hw);
>  	const struct samsung_pll_rate_table *rate;
>  	u32 con0, con1, lock;
> -	ktime_t start;
> 
>  	/* Get required rate settings from table */
>  	rate = samsung_get_pll_settings(pll, drate);
> @@ -648,20 +644,7 @@ static int samsung_pll46xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  	writel_relaxed(con1, pll->con_reg + 0x4);
> 
>  	/* Wait for locking. */
> -	start = ktime_get();
> -	while (!(readl_relaxed(pll->con_reg) & PLL46XX_LOCKED)) {
> -		ktime_t delta = ktime_sub(ktime_get(), start);
> -
> -		if (ktime_to_ms(delta) > PLL_TIMEOUT_MS) {
> -			pr_err("%s: could not lock PLL %s\n",
> -					__func__, clk_hw_get_name(hw));
> -			return -EFAULT;
> -		}
> -
> -		cpu_relax();
> -	}
> -
> -	return 0;
> +	return samsung_pll_lock_wait(pll, PLL46XX_LOCKED);
>  }
> 
>  static const struct clk_ops samsung_pll46xx_clk_ops = {
> @@ -1035,14 +1018,9 @@ static int samsung_pll2550xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  			(rate->sdiv << PLL2550XX_S_SHIFT);
>  	writel_relaxed(tmp, pll->con_reg);
> 
> -	/* wait_lock_time */
> -	do {
> -		cpu_relax();
> -		tmp = readl_relaxed(pll->con_reg);
> -	} while (!(tmp & (PLL2550XX_LOCK_STAT_MASK
> -			<< PLL2550XX_LOCK_STAT_SHIFT)));
> -
> -	return 0;
> +	/* Wait for locking. */
> +	return samsung_pll_lock_wait(pll,
> +			PLL2550XX_LOCK_STAT_MASK << PLL2550XX_LOCK_STAT_SHIFT);
>  }
> 
>  static const struct clk_ops samsung_pll2550xx_clk_ops = {
> @@ -1132,13 +1110,9 @@ static int samsung_pll2650x_set_rate(struct clk_hw *hw, unsigned long drate,
>  	con1 |= ((rate->kdiv & PLL2650X_K_MASK) << PLL2650X_K_SHIFT);
>  	writel_relaxed(con1, pll->con_reg + 4);
> 
> -	do {
> -		cpu_relax();
> -		con0 = readl_relaxed(pll->con_reg);
> -	} while (!(con0 & (PLL2650X_LOCK_STAT_MASK
> -			<< PLL2650X_LOCK_STAT_SHIFT)));
> -
> -	return 0;
> +	/* Wait for locking. */
> +	return samsung_pll_lock_wait(pll,
> +			PLL2650X_LOCK_STAT_MASK << PLL2650X_LOCK_STAT_SHIFT);
>  }
> 
>  static const struct clk_ops samsung_pll2650x_clk_ops = {
> --
> 2.7.4
> 
> 
> 

Thanks.

Acked-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics