Re: DRA7 clock question

[Tero Kristo <kristo@kernel.org>]

On 28/10/2021 19:13, Grygorii Strashko wrote:
> 
> 
> 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

Yeah, this looks like something for someone to fix. The bug has been 
masked by the fact that the autoidle_mask for dra7 is always 0x1, 
meaning the shift value becomes zero.

> 
>>
>>              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

Yes, this can be dropped. When a clock is going to autoidle, we don't 
really care when it does so and thus are not polling the status.

-Tero

> 
>>
>> 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
>>
> 
>