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

[Tomasz Figa <tomasz.figa@gmail.com>]

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
>