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
