Re: DRA7 clock question

[Grygorii Strashko <grygorii.strashko@xxxxxx>]

On 28/10/2021 18:16, Geert Uytterhoeven wrote:
> Hi Tero, Tony,
>
> I accidentally stumbled across the following code in drivers/clk/ti/apll.c:
>
>      static int dra7_apll_enable(struct clk_hw *hw)
>      {
>              struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>              int r = 0, i = 0;
>              struct dpll_data *ad;
>              const char *clk_name;
>              u8 state = 1;
>              u32 v;
>
>              ad = clk->dpll_data;
>              if (!ad)
>                      return -EINVAL;
>
>              clk_name = clk_hw_get_name(&clk->hw);
>
>              state <<= __ffs(ad->idlest_mask);
>
> state is shifted to its bit position...
>
>              /* Check is already locked */
>              v = ti_clk_ll_ops->clk_readl(&ad->idlest_reg);
>
>              if ((v & ad->idlest_mask) == state)
>
> ... and checked.
>
>                      return r;
>
>              v = ti_clk_ll_ops->clk_readl(&ad->control_reg);
>              v &= ~ad->enable_mask;
>              v |= APLL_FORCE_LOCK << __ffs(ad->enable_mask);
>              ti_clk_ll_ops->clk_writel(v, &ad->control_reg);
>
>              state <<= __ffs(ad->idlest_mask);
>
> state is shifted again? ...

this is probably copy-paste err

>              while (1) {
>                      v = ti_clk_ll_ops->clk_readl(&ad->idlest_reg);
>                      if ((v & ad->idlest_mask) == state)
>
> ... and checked again?

this is correct wait for locking

>                              break;
>                      if (i > MAX_APLL_WAIT_TRIES)
>                              break;
>                      i++;
>                      udelay(1);
>              }
>
>              if (i == MAX_APLL_WAIT_TRIES) {
>                      pr_warn("clock: %s failed transition to '%s'\n",
>                              clk_name, (state) ? "locked" : "bypassed");
>                      r = -EBUSY;
>              } else
>                      pr_debug("clock: %s transition to '%s' in %d loops\n",
>                               clk_name, (state) ? "locked" : "bypassed", i);
>
>              return r;
>      }
>
>      static void dra7_apll_disable(struct clk_hw *hw)
>      {
>              struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>              struct dpll_data *ad;
>              u8 state = 1;
>              u32 v;
>
>              ad = clk->dpll_data;
>
>              state <<= __ffs(ad->idlest_mask);
>
> state is shifted to its bit position, but it is never used below?
> Perhaps one of the check blocks above should be moved here?

this is probably copy-paste issue also

> I checked git history and the original patch submissions, and even
> the oldest submission I could find had the same logic.

I think, old logic (basic) can be found at

arch/arm/mach-omap2/dpll3xxx.c

>              v = ti_clk_ll_ops->clk_readl(&ad->control_reg);
>              v &= ~ad->enable_mask;
>              v |= APLL_AUTO_IDLE << __ffs(ad->enable_mask);
>              ti_clk_ll_ops->clk_writel(v, &ad->control_reg);
>      }
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                          Geert


--
Best regards,
grygorii