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

[PATCH] 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 (duplicated)
code for polling with a timeout. This patch refactors that code a bit
and moves it to a common helper function which is then used
in .set_rate callbacks for all the PLLs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/clk/samsung/clk-pll.c | 94 +++++++++++++----------------------
 1 file changed, 35 insertions(+), 59 deletions(-)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index ac70ad785d8e..0929644be99a 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -63,6 +63,27 @@ 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)
+{
+	ktime_t timeout;
+
+	/* Wait until the PLL is in steady locked state */
+	timeout = ktime_add_ms(ktime_get(), PLL_TIMEOUT_MS);
+
+	while (!(readl_relaxed(pll->con_reg) & reg_mask)) {
+		if (ktime_after(ktime_get(), timeout)) {
+			pr_err("%s: Could not lock PLL %s\n",
+				__func__, clk_hw_get_name(&pll->hw));
+			return -EFAULT;
+		}
+
+		cpu_relax();
+	}
+
+	return 0;
+}
+
 static int samsung_pll3xxx_enable(struct clk_hw *hw)
 {
 	struct samsung_clk_pll *pll = to_clk_pll(hw);
@@ -241,12 +262,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 +336,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 +374,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 +450,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 +501,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 +587,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 +646,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 +1020,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 +1112,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.17.1

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

[Tomasz Figa]

Hi Sylwester,

2020年8月6日(木) 18:06 Sylwester Nawrocki <s.nawrocki@samsung.com>:
>
> 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 (duplicated)
> code for polling with a timeout. This patch refactors that code a bit
> and moves it to a common helper function which is then used
> in .set_rate callbacks for all the PLLs.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  drivers/clk/samsung/clk-pll.c | 94 +++++++++++++----------------------
>  1 file changed, 35 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index ac70ad785d8e..0929644be99a 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -63,6 +63,27 @@ 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)
> +{
> +       ktime_t timeout;
> +
> +       /* Wait until the PLL is in steady locked state */
> +       timeout = ktime_add_ms(ktime_get(), PLL_TIMEOUT_MS);
> +
> +       while (!(readl_relaxed(pll->con_reg) & reg_mask)) {
> +               if (ktime_after(ktime_get(), timeout)) {
> +                       pr_err("%s: Could not lock PLL %s\n",
> +                               __func__, clk_hw_get_name(&pll->hw));
> +                       return -EFAULT;
> +               }
> +
> +               cpu_relax();
> +       }

Thanks for the patch! Good to have this consolidated. How about going
one step further and using the generic
readl_relaxed_poll_timeout_atomic() helper?

Best regards,
Tomasz

> +
> +       return 0;
> +}
> +
>  static int samsung_pll3xxx_enable(struct clk_hw *hw)
>  {
>         struct samsung_clk_pll *pll = to_clk_pll(hw);
> @@ -241,12 +262,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 +336,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 +374,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 +450,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 +501,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 +587,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 +646,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 +1020,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 +1112,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.17.1
>

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

[Chanwoo Choi]

Hi Sylwester,

On 8/7/20 1:06 AM, 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 (duplicated)
> code for polling with a timeout. This patch refactors that code a bit
> and moves it to a common helper function which is then used
> in .set_rate callbacks for all the PLLs.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  drivers/clk/samsung/clk-pll.c | 94 +++++++++++++----------------------
>  1 file changed, 35 insertions(+), 59 deletions(-)
> 

(snip)

It fix the infinite loop and unify the duplicate code.
It looks good to me. Thanks.

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Sylwester Nawrocki]

Hi Tomasz,

On 8/6/20 18:11, Tomasz Figa wrote:
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -63,6 +63,27 @@ 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)
>> +{
>> +       ktime_t timeout;
>> +
>> +       /* Wait until the PLL is in steady locked state */
>> +       timeout = ktime_add_ms(ktime_get(), PLL_TIMEOUT_MS);
>> +
>> +       while (!(readl_relaxed(pll->con_reg) & reg_mask)) {
>> +               if (ktime_after(ktime_get(), timeout)) {
>> +                       pr_err("%s: Could not lock PLL %s\n",
>> +                               __func__, clk_hw_get_name(&pll->hw));
>> +                       return -EFAULT;
>> +               }
>> +
>> +               cpu_relax();
>> +       }

> Thanks for the patch! Good to have this consolidated. How about going
> one step further and using the generic
> readl_relaxed_poll_timeout_atomic() helper?

Might be a good suggestion, I was considering those helpers but ended
up not using them in the patch. The cpu_relax() call might also not be
really needed now, when there is the ktime code within the loop.
Having multiple occurrences of readl_relaxed_poll_timeout_atomic() could
increase the code size due to inlining. How about keeping the 
samsung_pll_lock_wait() function and just changing its implementation?

-- 
Thanks,
Sylwester

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

[Tomasz Figa]

2020年8月7日(金) 19:06 Sylwester Nawrocki <s.nawrocki@samsung.com>:
>
> Hi Tomasz,
>
> On 8/6/20 18:11, Tomasz Figa wrote:
> >> --- a/drivers/clk/samsung/clk-pll.c
> >> +++ b/drivers/clk/samsung/clk-pll.c
> >> @@ -63,6 +63,27 @@ 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)
> >> +{
> >> +       ktime_t timeout;
> >> +
> >> +       /* Wait until the PLL is in steady locked state */
> >> +       timeout = ktime_add_ms(ktime_get(), PLL_TIMEOUT_MS);
> >> +
> >> +       while (!(readl_relaxed(pll->con_reg) & reg_mask)) {
> >> +               if (ktime_after(ktime_get(), timeout)) {
> >> +                       pr_err("%s: Could not lock PLL %s\n",
> >> +                               __func__, clk_hw_get_name(&pll->hw));
> >> +                       return -EFAULT;
> >> +               }
> >> +
> >> +               cpu_relax();
> >> +       }
>
> > Thanks for the patch! Good to have this consolidated. How about going
> > one step further and using the generic
> > readl_relaxed_poll_timeout_atomic() helper?
>
> Might be a good suggestion, I was considering those helpers but ended
> up not using them in the patch. The cpu_relax() call might also not be
> really needed now, when there is the ktime code within the loop.
> Having multiple occurrences of readl_relaxed_poll_timeout_atomic() could
> increase the code size due to inlining. How about keeping the
> samsung_pll_lock_wait() function and just changing its implementation?

Sounds good to me, thanks!

Best regards,
Tomasz

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

[Sylwester Nawrocki]

On 07.08.2020 22:20, Stephen Boyd wrote:
> Quoting Sylwester Nawrocki (2020-08-07 10:06:08)

>> On 8/6/20 18:11, Tomasz Figa wrote:
>>>> --- a/drivers/clk/samsung/clk-pll.c
>>>> +++ b/drivers/clk/samsung/clk-pll.c
>>>> @@ -63,6 +63,27 @@ 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)
>>>> +{
>>>> +       ktime_t timeout;
>>>> +
>>>> +       /* Wait until the PLL is in steady locked state */
>>>> +       timeout = ktime_add_ms(ktime_get(), PLL_TIMEOUT_MS);
>>>> +
>>>> +       while (!(readl_relaxed(pll->con_reg) & reg_mask)) {
>>>> +               if (ktime_after(ktime_get(), timeout)) {
>>>> +                       pr_err("%s: Could not lock PLL %s\n",
>>>> +                               __func__, clk_hw_get_name(&pll->hw));
>>>> +                       return -EFAULT;
>>>> +               }
>>>> +
>>>> +               cpu_relax();
>>>> +       }
>>
>>> Thanks for the patch! Good to have this consolidated. How about going
>>> one step further and using the generic
>>> readl_relaxed_poll_timeout_atomic() helper?
>>
>> Might be a good suggestion, I was considering those helpers but ended
>> up not using them in the patch. The cpu_relax() call might also not be
>> really needed now, when there is the ktime code within the loop.
>> Having multiple occurrences of readl_relaxed_poll_timeout_atomic() could
>> increase the code size due to inlining. How about keeping the 
>> samsung_pll_lock_wait() function and just changing its implementation?
> 
> None of these concerns are mentioned in the commit text. And they seem
> like problems that should be addressed if they're actually problems vs.
> avoiding a common macro and not mentioning them.

Sure, I will improve the commit text, I just didn't investigate in detail
how the common macro could or could not be used before Tomasz's review.