Re: [PATCH] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
From: Tomasz Figa
Date: Thu Aug 06 2020 - 12:59:04 EST
Hi Sylwester,
2020年8月6日(木) 18:06 Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>:
>
> 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@xxxxxxxxxxx>
> ---
> 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
>