On 28/10/2021 19:13, Grygorii Strashko wrote:

On 28/10/2021 18:16, Geert Uytterhoeven wrote:

quoted

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.

quoted

             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

quoted

                             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

quoted

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

quoted

             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