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

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

[Sylwester Nawrocki]

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@samsung.com>
---
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

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@samsung.com>
> ---
> 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@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Sylwester Nawrocki]

On 14.08.2020 02:46, Chanwoo Choi wrote:
> 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.

>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> 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(-)

> Thanks.
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
 
Patch applied, thank you for your comments.

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

[Sylwester Nawrocki]

On 15.09.2020 13:34, Sylwester Nawrocki wrote:
> On 14.08.2020 02:46, Chanwoo Choi wrote:
>> 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.
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> ---
>>> 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(-)
>> Thanks.
>>
>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>  
> Patch applied, thank you for your comments.

And dropped now as it causes issues on arm64. As reported by Marek
it seems udelay() doesn't work before the system timer is initialized.