Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[David Shah]

Hi,

There has been a somewhat longstanding issue on the Pyra, where any kind of soft reboot causes
it to hang (only a true power off and on again works). The background is at
https://projects.goldelico.com/p/gta04-kernel/issues/876/.

The failure is typically of the following form: https://dev.pyra-handheld.com/snippets/765
(the exact failure sequence has changed a bit in different kernel versions).

With the pertinent line being:
[    0.000000] clock: dpll_abe_ck failed transition to 'locked'

This only happens on the Pyra, not the OMAP5 uEVM. This seems to be because
the Pyra uses TIMER8 for the backlight PWM.

Looking around at some other OMAP5 clocking code, I found
https://gitlab.com/linux-omap4-dev/omapboot/-/blob/kexec_support/arch/omap5/clock.c#L335
This to me suggests that both CM_CLKSEL_ABE_PLL_REF and CM_CLKSEL_WKUPAON
should be set to 1. I found that only CM_CLKSEL_ABE_PLL_REF was 1 and 
CM_CLKSEL_WKUPAON was 0 at the point of checking DPLL lock.

I wrote a very hacky patch just to force CM_CLKSEL_WKUPAON to 1 at startup, to test
this theory: https://dev.pyra-handheld.com/snippets/770 (breaking every rule in the
book, I know :)

And indeed with this reboots now seem to work fine.

The question is, what is the correct way/place to deal with this? Is this even a Linux
issue at all, or should U-Boot be doing something here? A quick glance suggests that
nothing in the kernel deals with CM_CLKSEL_WKUPAON at all but I may have missed
something.

Best

David

Re: Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[Tony Lindgren]

Hi,

* David Shah <dave@ds0.me> [200723 14:08]:
> Hi,
> 
> There has been a somewhat longstanding issue on the Pyra, where any kind of soft reboot causes
> it to hang (only a true power off and on again works). The background is at
> https://projects.goldelico.com/p/gta04-kernel/issues/876/.
> 
> The failure is typically of the following form: https://dev.pyra-handheld.com/snippets/765
> (the exact failure sequence has changed a bit in different kernel versions).
> 
> With the pertinent line being:
> [    0.000000] clock: dpll_abe_ck failed transition to 'locked'
> 
> This only happens on the Pyra, not the OMAP5 uEVM. This seems to be because
> the Pyra uses TIMER8 for the backlight PWM.

OK good to hear this one is tracked down now.

> Looking around at some other OMAP5 clocking code, I found
> https://gitlab.com/linux-omap4-dev/omapboot/-/blob/kexec_support/arch/omap5/clock.c#L335
> This to me suggests that both CM_CLKSEL_ABE_PLL_REF and CM_CLKSEL_WKUPAON
> should be set to 1. I found that only CM_CLKSEL_ABE_PLL_REF was 1 and 
> CM_CLKSEL_WKUPAON was 0 at the point of checking DPLL lock.
> 
> I wrote a very hacky patch just to force CM_CLKSEL_WKUPAON to 1 at startup, to test
> this theory: https://dev.pyra-handheld.com/snippets/770 (breaking every rule in the
> book, I know :)
> 
> And indeed with this reboots now seem to work fine.
> 
> The question is, what is the correct way/place to deal with this? Is this even a Linux
> issue at all, or should U-Boot be doing something here? A quick glance suggests that
> nothing in the kernel deals with CM_CLKSEL_WKUPAON at all but I may have missed
> something.

Not sure if CM_CLKSEL_WKUPAON can be always configured to 1 for ABE
usage. So probably configuring it with assigned-clocks does not sound
like a right solution.

If it only needs to be configured to 1 for reboot, sounds like it should
be set in omap44xx_restart(). And we should also set it to 1 for omap4
too.

Regards,

Tony

Re: Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[David Shah]

Hi Tony,

On Mon, 2020-07-27 at 01:28 -0700, Tony Lindgren wrote:
> Hi,
> 
> * David Shah <dave@ds0.me> [200723 14:08]:
> > Hi,
> > 
> > There has been a somewhat longstanding issue on the Pyra, where any
> > kind of soft reboot causes
> > it to hang (only a true power off and on again works). The
> > background is at
> > https://projects.goldelico.com/p/gta04-kernel/issues/876/.
> > 
> > The failure is typically of the following form: 
> > https://dev.pyra-handheld.com/snippets/765
> > (the exact failure sequence has changed a bit in different kernel
> > versions).
> > 
> > With the pertinent line being:
> > [    0.000000] clock: dpll_abe_ck failed transition to 'locked'
> > 
> > This only happens on the Pyra, not the OMAP5 uEVM. This seems to be
> > because
> > the Pyra uses TIMER8 for the backlight PWM.
> 
> OK good to hear this one is tracked down now.
> 
> > Looking around at some other OMAP5 clocking code, I found
> > https://gitlab.com/linux-omap4-dev/omapboot/-/blob/kexec_support/arch/omap5/clock.c#L335
> > This to me suggests that both CM_CLKSEL_ABE_PLL_REF and
> > CM_CLKSEL_WKUPAON
> > should be set to 1. I found that only CM_CLKSEL_ABE_PLL_REF was 1
> > and 
> > CM_CLKSEL_WKUPAON was 0 at the point of checking DPLL lock.
> > 
> > I wrote a very hacky patch just to force CM_CLKSEL_WKUPAON to 1 at
> > startup, to test
> > this theory: https://dev.pyra-handheld.com/snippets/770 (breaking
> > every rule in the
> > book, I know :)
> > 
> > And indeed with this reboots now seem to work fine.
> > 
> > The question is, what is the correct way/place to deal with this?
> > Is this even a Linux
> > issue at all, or should U-Boot be doing something here? A quick
> > glance suggests that
> > nothing in the kernel deals with CM_CLKSEL_WKUPAON at all but I may
> > have missed
> > something.
> 
> Not sure if CM_CLKSEL_WKUPAON can be always configured to 1 for ABE
> usage. So probably configuring it with assigned-clocks does not sound
> like a right solution.
> 
> If it only needs to be configured to 1 for reboot, sounds like it
> should
> be set in omap44xx_restart(). And we should also set it to 1 for
> omap4
> too.

omap44xx_restart doesn't seem like the right place to me, as the bug
also affects hard resets (i.e. NRESWARM assertion) and it would be nice
to have these working, too.

Would a better solution be to set it early during startup (the first
part of clock init), and then clear it when the DPLLs are set up and
locked?

> Regards,
> 
> Tony

Thanks for your help!

David

Re: Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[Tony Lindgren]

* David Shah <dave@ds0.me> [200727 08:58]:
> On Mon, 2020-07-27 at 01:28 -0700, Tony Lindgren wrote:
> > If it only needs to be configured to 1 for reboot, sounds like it
> > should
> > be set in omap44xx_restart(). And we should also set it to 1 for
> > omap4
> > too.
> 
> omap44xx_restart doesn't seem like the right place to me, as the bug
> also affects hard resets (i.e. NRESWARM assertion) and it would be nice
> to have these working, too.

Ah right, the device reboots fine, but the rebooted kernel
won't initialize properly.

> Would a better solution be to set it early during startup (the first
> part of clock init), and then clear it when the DPLLs are set up and
> locked?

Yes sounds like then the place to configure this is in the
drivers/clk/ti/clk-54xx.c omap5xxx_dt_clk_init(). Maybe add
a comment to the patch description that a similar patch may
be also needed for omap4 on some devices.

Regards,

Tony

Re: Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[H. Nikolaus Schaller]

Hi David and Tony,

> Am 28.07.2020 um 10:42 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * David Shah <dave@ds0.me> [200727 08:58]:
>> On Mon, 2020-07-27 at 01:28 -0700, Tony Lindgren wrote:
>>> If it only needs to be configured to 1 for reboot, sounds like it
>>> should
>>> be set in omap44xx_restart(). And we should also set it to 1 for
>>> omap4
>>> too.
>> 
>> omap44xx_restart doesn't seem like the right place to me, as the bug
>> also affects hard resets (i.e. NRESWARM assertion) and it would be nice
>> to have these working, too.
> 
> Ah right, the device reboots fine, but the rebooted kernel
> won't initialize properly.
> 
>> Would a better solution be to set it early during startup (the first
>> part of clock init), and then clear it when the DPLLs are set up and
>> locked?
> 
> Yes sounds like then the place to configure this is in the
> drivers/clk/ti/clk-54xx.c omap5xxx_dt_clk_init(). Maybe add
> a comment to the patch description that a similar patch may
> be also needed for omap4 on some devices.

It seems as if the conclusion was that the patch is at the right
location omap5xxx_dt_clk_init(). Has it been merged somewhere?

It seems as if we still need it in our 5.10-rc series:

https://git.goldelico.com/?p=letux-kernel.git;a=commit;h=aa8eda144e8faa185e154710ca0eef12adb2d29d

BR and thanks,
Nikolaus

Re: Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [201123 20:11]:
> Hi David and Tony,
> 
> > Am 28.07.2020 um 10:42 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > * David Shah <dave@ds0.me> [200727 08:58]:
> >> On Mon, 2020-07-27 at 01:28 -0700, Tony Lindgren wrote:
> >>> If it only needs to be configured to 1 for reboot, sounds like it
> >>> should
> >>> be set in omap44xx_restart(). And we should also set it to 1 for
> >>> omap4
> >>> too.
> >> 
> >> omap44xx_restart doesn't seem like the right place to me, as the bug
> >> also affects hard resets (i.e. NRESWARM assertion) and it would be nice
> >> to have these working, too.
> > 
> > Ah right, the device reboots fine, but the rebooted kernel
> > won't initialize properly.
> > 
> >> Would a better solution be to set it early during startup (the first
> >> part of clock init), and then clear it when the DPLLs are set up and
> >> locked?
> > 
> > Yes sounds like then the place to configure this is in the
> > drivers/clk/ti/clk-54xx.c omap5xxx_dt_clk_init(). Maybe add
> > a comment to the patch description that a similar patch may
> > be also needed for omap4 on some devices.
> 
> It seems as if the conclusion was that the patch is at the right
> location omap5xxx_dt_clk_init(). Has it been merged somewhere?
> 
> It seems as if we still need it in our 5.10-rc series:
> 
> https://git.goldelico.com/?p=letux-kernel.git;a=commit;h=aa8eda144e8faa185e154710ca0eef12adb2d29d

I recall Dave needs to resend it to the clock maintainers as
listed with scripts/get_maintainer.pl -f option.

Regards,

Tony

Re: Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[H. Nikolaus Schaller]

Hi David,

> Am 01.12.2020 um 08:37 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [201123 20:11]:
>> Hi David and Tony,
>> 
>>> Am 28.07.2020 um 10:42 schrieb Tony Lindgren <tony@atomide.com>:
>>> 
>>> * David Shah <dave@ds0.me> [200727 08:58]:
>>>> On Mon, 2020-07-27 at 01:28 -0700, Tony Lindgren wrote:
>>>>> If it only needs to be configured to 1 for reboot, sounds like it
>>>>> should
>>>>> be set in omap44xx_restart(). And we should also set it to 1 for
>>>>> omap4
>>>>> too.
>>>> 
>>>> omap44xx_restart doesn't seem like the right place to me, as the bug
>>>> also affects hard resets (i.e. NRESWARM assertion) and it would be nice
>>>> to have these working, too.
>>> 
>>> Ah right, the device reboots fine, but the rebooted kernel
>>> won't initialize properly.
>>> 
>>>> Would a better solution be to set it early during startup (the first
>>>> part of clock init), and then clear it when the DPLLs are set up and
>>>> locked?
>>> 
>>> Yes sounds like then the place to configure this is in the
>>> drivers/clk/ti/clk-54xx.c omap5xxx_dt_clk_init(). Maybe add
>>> a comment to the patch description that a similar patch may
>>> be also needed for omap4 on some devices.
>> 
>> It seems as if the conclusion was that the patch is at the right
>> location omap5xxx_dt_clk_init(). Has it been merged somewhere?
>> 
>> It seems as if we still need it in our 5.10-rc series:
>> 
>> https://git.goldelico.com/?p=letux-kernel.git;a=commit;h=aa8eda144e8faa185e154710ca0eef12adb2d29d
> 
> I recall Dave needs to resend it to the clock maintainers as
> listed with scripts/get_maintainer.pl -f option.

Do you think you will find to push this upstream?

BR and thanks,
Nikolaus

Re: Understanding OMAP5 DPLL_ABE and CM_CLKSEL_WKUPAON

[H. Nikolaus Schaller]

Hi David,

> Am 02.12.2020 um 16:10 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi David,
> 
>> Am 01.12.2020 um 08:37 schrieb Tony Lindgren <tony@atomide.com>:
>> 
>> * H. Nikolaus Schaller <hns@goldelico.com> [201123 20:11]:
>>> Hi David and Tony,
>>> 
>>>> Am 28.07.2020 um 10:42 schrieb Tony Lindgren <tony@atomide.com>:
>>>> 
>>>> * David Shah <dave@ds0.me> [200727 08:58]:
>>>>> On Mon, 2020-07-27 at 01:28 -0700, Tony Lindgren wrote:
>>>>>> If it only needs to be configured to 1 for reboot, sounds like it
>>>>>> should
>>>>>> be set in omap44xx_restart(). And we should also set it to 1 for
>>>>>> omap4
>>>>>> too.
>>>>> 
>>>>> omap44xx_restart doesn't seem like the right place to me, as the bug
>>>>> also affects hard resets (i.e. NRESWARM assertion) and it would be nice
>>>>> to have these working, too.
>>>> 
>>>> Ah right, the device reboots fine, but the rebooted kernel
>>>> won't initialize properly.
>>>> 
>>>>> Would a better solution be to set it early during startup (the first
>>>>> part of clock init), and then clear it when the DPLLs are set up and
>>>>> locked?
>>>> 
>>>> Yes sounds like then the place to configure this is in the
>>>> drivers/clk/ti/clk-54xx.c omap5xxx_dt_clk_init(). Maybe add
>>>> a comment to the patch description that a similar patch may
>>>> be also needed for omap4 on some devices.
>>> 
>>> It seems as if the conclusion was that the patch is at the right
>>> location omap5xxx_dt_clk_init(). Has it been merged somewhere?
>>> 
>>> It seems as if we still need it in our 5.10-rc series:
>>> 
>>> https://git.goldelico.com/?p=letux-kernel.git;a=commit;h=aa8eda144e8faa185e154710ca0eef12adb2d29d
>> 
>> I recall Dave needs to resend it to the clock maintainers as
>> listed with scripts/get_maintainer.pl -f option.
> 
> Do you think you will find to push this upstream?

I hope your are well.

If you don't disagree I can add some of your original description
to the commit message and your and my signed-off and post it for
review.

BR and thanks,
Nikolaus