* DRA7 clock question
@ 2021-10-28 15:16 Geert Uytterhoeven
2021-10-28 16:13 ` Grygorii Strashko
0 siblings, 1 reply; 4+ messages in thread
From: Geert Uytterhoeven @ 2021-10-28 15:16 UTC (permalink / raw)
To: Tero Kristo, Tony Lindgren
Cc: linux-clk, open list:TI ETHERNET SWITCH DRIVER (CPSW)
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? ...
while (1) {
v = ti_clk_ll_ops->clk_readl(&ad->idlest_reg);
if ((v & ad->idlest_mask) == state)
... and checked again?
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?
I checked git history and the original patch submissions, and even
the oldest submission I could find had the same logic.
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: DRA7 clock question
2021-10-28 15:16 DRA7 clock question Geert Uytterhoeven
@ 2021-10-28 16:13 ` Grygorii Strashko
2021-10-29 5:34 ` Tero Kristo
0 siblings, 1 reply; 4+ messages in thread
From: Grygorii Strashko @ 2021-10-28 16:13 UTC (permalink / raw)
To: Geert Uytterhoeven, Tero Kristo, Tony Lindgren
Cc: linux-clk, open list:TI ETHERNET SWITCH DRIVER (CPSW)
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: DRA7 clock question
2021-10-28 16:13 ` Grygorii Strashko
@ 2021-10-29 5:34 ` Tero Kristo
2021-10-29 6:45 ` Tony Lindgren
0 siblings, 1 reply; 4+ messages in thread
From: Tero Kristo @ 2021-10-29 5:34 UTC (permalink / raw)
To: Grygorii Strashko, Geert Uytterhoeven, Tony Lindgren
Cc: linux-clk, open list:TI ETHERNET SWITCH DRIVER (CPSW)
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
>>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: DRA7 clock question
2021-10-29 5:34 ` Tero Kristo
@ 2021-10-29 6:45 ` Tony Lindgren
0 siblings, 0 replies; 4+ messages in thread
From: Tony Lindgren @ 2021-10-29 6:45 UTC (permalink / raw)
To: Tero Kristo
Cc: Grygorii Strashko, Geert Uytterhoeven, linux-clk,
open list:TI ETHERNET SWITCH DRIVER (CPSW)
* Tero Kristo <kristo@kernel.org> [211029 05:35]:
> On 28/10/2021 19:13, Grygorii Strashko wrote:
> > On 28/10/2021 18:16, Geert Uytterhoeven wrote:
> > >
> > > 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.
Heh nice lucky bug there :)
Regards,
Tony
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-29 6:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-28 15:16 DRA7 clock question Geert Uytterhoeven
2021-10-28 16:13 ` Grygorii Strashko
2021-10-29 5:34 ` Tero Kristo
2021-10-29 6:45 ` Tony Lindgren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox