[PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

This patchset enhaces the recently added Orion Timer driver to support
all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
timer support is then enabled per default for those platforms, so that
the board config files don't need to be changed. Also necessary is
some dts hacking, so that the timer DT node is available in early
U-Boot stages.

I've successfully tested this patchset on an Armada XP board. Additional
test on other boards and platforms are very welcome and necessary.

Thanks,
Stefan

Stefan Roese (6):
  timer: orion-timer: Add support for other Armada SoC's
  timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support
  arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms
  arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step
  arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1
  arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot,dm-pre-reloc" to timer
    DT node

 arch/arm/Kconfig                          |  4 ++
 arch/arm/dts/Makefile                     |  6 ++-
 arch/arm/dts/armada-375.dtsi              |  4 +-
 arch/arm/dts/mvebu-u-boot.dtsi            | 11 ++++
 arch/arm/mach-mvebu/include/mach/config.h |  5 --
 drivers/timer/Kconfig                     |  5 +-
 drivers/timer/orion-timer.c               | 66 +++++++++++++++++++++--
 7 files changed, 89 insertions(+), 12 deletions(-)

-- 
2.37.2

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

Hi Stefan,

On 01.09.22 13:52, Stefan Herbrechtsmeier wrote:
> Hi,
> 
> Am 01.09.2022 um 11:27 schrieb Stefan Roese:
>> Hi Tony,
>>
>> On 01.09.22 09:39, Tony Dinh wrote:
>>
>> <snip>
>>
>>>>> Some ideas.
>>>>>
>>>>> The get_timer() function looks wrong assigning an uint64_t to ulong.
>>>>>
>>>>> lib/time.c
>>>>>
>>>>>       static uint64_t notrace tick_to_time(uint64_t tick)
>>>>>       uint64_t notrace get_ticks(void)
>>>>>       uint64_t __weak notrace get_ticks(void)
>>>>>
>>>>>       ulong __weak get_timer(ulong base)
>>>>>       {
>>>>>               return tick_to_time(get_ticks()) - base;
>>>>>       }
>>>>>
>>>>> Most of the timer infrastructure is using uint64_t. I'm seeing this
>>>>> __weak function get_timer was invoked in Kirkwood boards. Both in
>>>>> sleep and timer commands.
>>>>
>>>> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
>>>> is why we don't need a u64 for the timer.
>>>
>>> Thanks for your explanation! However, would you agree that code is
>>> problematic and needed some improvement ? IOW, depending what the
>>> compiler does, it might return the 1st 32 bit of the 64-bit integer
>>> result?
>>
>> It will return the lower 32 bits if the system is 32bit, yes.
>>
>> To check if we have a problem here, please add this (totally untested)
>> code and extend it if it makes sense:
>>
>> diff --git a/lib/time.c b/lib/time.c
>> index bbf191f67323..ef5252419f3b 100644
>> --- a/lib/time.c
>> +++ b/lib/time.c
>> @@ -146,7 +146,15 @@ int __weak timer_init(void)
>>   /* Returns time in milliseconds */
>>   ulong __weak get_timer(ulong base)
>>   {
>> -       return tick_to_time(get_ticks()) - base;
>> +       u64 ticks = get_ticks();
>> +       u64 time_ms = tick_to_time(ticks);
>> +
>> +       if (time_ms & 0xffffffff00000000ULL)
>> +               printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);
>> +       if ((time_ms - base) & 0xffffffff80000000ULL)
>> +               printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n", 
>> ticks, time_ms, base, time_ms - base);
>> +
>> +       return time_ms - base;
>>   }
>>
>> At least here, you seem to have a wrap around with the 32bits AFAICT:
>>
>>> GoFlexHome> sleep 20.5
>>> do_sleep got a timer start = 15031
>>> do_sleep delay = 20000
>>> do_sleep delay = 20500
>>> do_sleep sleeping...
>>> do_sleep start 15031 current 100
>>> <snip>
>>> do_sleep start 15031 current 6400
>>> do_sleep end of sleep ... current = 4294952265
>>>
>>> *** Something strange happened here. current should be 6500, but it
>>> seems to have garbage. So the loop exits prematurely.
>>
>> 4294952265 = 0xFFFFC549!
>>
> 
> Does the driver use a 32 bit counter without the timer_conv_64 function 
> inside the get_count function?

Yes, it missed this conversion function call. Thanks for noticing.

Thanks,
Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan,

On Thu, Sep 1, 2022 at 7:51 PM Tony Dinh <mibodhi@gmail.com> wrote:
>
> Hello,
>
> On Thu, Sep 1, 2022 at 4:46 PM Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > Hi Stefan R,
> >
> > On Thu, Sep 1, 2022 at 7:35 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, 1 Sept 2022 at 03:27, Stefan Roese <sr@denx.de> wrote:
> > > >
> > > > Hi Tony,
> > > >
> > > > On 01.09.22 09:39, Tony Dinh wrote:
> > > >
> > > > <snip>
> > > >
> > > > >>> Some ideas.
> > > > >>>
> > > > >>> The get_timer() function looks wrong assigning an uint64_t to ulong.
> > > > >>>
> > > > >>> lib/time.c
> > > > >>>
> > > > >>>       static uint64_t notrace tick_to_time(uint64_t tick)
> > > > >>>       uint64_t notrace get_ticks(void)
> > > > >>>       uint64_t __weak notrace get_ticks(void)
> > > > >>>
> > > > >>>       ulong __weak get_timer(ulong base)
> > > > >>>       {
> > > > >>>               return tick_to_time(get_ticks()) - base;
> > > > >>>       }
> > > > >>>
> > > > >>> Most of the timer infrastructure is using uint64_t. I'm seeing this
> > > > >>> __weak function get_timer was invoked in Kirkwood boards. Both in
> > > > >>> sleep and timer commands.
> > > > >>
> > > > >> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
> > > > >> is why we don't need a u64 for the timer.
> > >
> > > This is wrong, I meant that get_tbclk() can run at 1MHZ (for example).
> > > The tick is 1KHz.
> > >
> > > > >
> > > > > Thanks for your explanation! However, would you agree that code is
> > > > > problematic and needed some improvement ? IOW, depending what the
> > > > > compiler does, it might return the 1st 32 bit of the 64-bit integer
> > > > > result?
> > >
> > > Yes, we should really use ulong for the tick count as well. The use of
> > > 64-bits seems wrong (on 32-bit machines).
> > >
> > > >
> > > > It will return the lower 32 bits if the system is 32bit, yes.
> > > >
> > > > To check if we have a problem here, please add this (totally untested)
> > > > code and extend it if it makes sense:
> > > >
> > > > diff --git a/lib/time.c b/lib/time.c
> > > > index bbf191f67323..ef5252419f3b 100644
> > > > --- a/lib/time.c
> > > > +++ b/lib/time.c
> > > > @@ -146,7 +146,15 @@ int __weak timer_init(void)
> > > >   /* Returns time in milliseconds */
> > > >   ulong __weak get_timer(ulong base)
> > > >   {
> > > > -       return tick_to_time(get_ticks()) - base;
> > > > +       u64 ticks = get_ticks();
> > > > +       u64 time_ms = tick_to_time(ticks);
> > > > +
> > > > +       if (time_ms & 0xffffffff00000000ULL)
> > > > +               printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);
> > > > +       if ((time_ms - base) & 0xffffffff80000000ULL)
> > > > +               printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",
> > > > ticks, time_ms, base, time_ms - base);
> > > > +
> > > > +       return time_ms - base;
> > > >   }
> >
> > With this patch, indeed it showed a wrap around. And the system was
> > frozen during the next command.
> >
> > Below is the log (my annotated comment starts with ***).
> >
> > <BEGIN LOG>
> > U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Sep 01 2022 - 15:44:22 -0700)
> > Pogoplug V4
> >
> > SoC:   Kirkwood 88F6281_A1
> > Model: Cloud Engines PogoPlug Series 4
> > DRAM:  128 MiB
> > orion_timer_probe Clock Rate 166000000
> > orion_timer_probe successful
> > Core:  19 devices, 15 uclasses, devicetree: separate
> > NAND:  128 MiB
> > MMC:   mvsdio@90000: 0
> > Loading Environment from NAND... OK
> > In:    serial
> > Out:   serial
> > Err:   serial
> > pcie0.0: Link up
> > Net:   eth0: ethernet-controller@72000
> > Hit any key to stop autoboot:  0
> > Pogo_V4> sleep 5
> > do_sleep got a timer start = 14344
> > do_sleep delay = 5000
> > do_sleep delay = 5000
> > do_sleep sleeping...
> > do_sleep start 14344 curent 100
> > do_sleep start 14344 curent 200
> > <snip>
> > do_sleep start 14344 curent 4800
> > do_sleep start 14344 curent 4900
> > do_sleep end of sleep ... current = 5000
> >
> > Pogo_V4> sleep 10
> > do_sleep got a timer start = 22370
> > do_sleep delay = 10000
> > do_sleep delay = 10000
> > do_sleep sleeping...
> > do_sleep start 22370 curent 100
> > do_sleep start 22370 curent 200
> > do_sleep start 22370 curent 300
> > do_sleep start 22370 curent 400
> > <snip>
> > do_sleep start 22370 curent 3300
> > do_sleep start 22370 curent 3400
> > do_sleep start 22370 curent 3500
> > ticks=188 time_ms=0 base=22370 ret=-22370
> > do_sleep end of sleep ... current = 4294944926
> >
> > *** we are seeing wrap around here
> >
> > Pogo_V4> sleep 15
> > do_sleep got a timer start = 15733
> > do_sleep delay = 15000
> > do_sleep delay = 15000
> > do_sleep sleeping...
> > do_sleep start 15733 curent 100
> > do_sleep start 15733 curent 200
> > do_sleep start 15733 curent 300
> > do_sleep start 15733 curent 400
> > <snip>
> >
> > do_sleep start 15733 curent 9900
> > do_sleep start 15733 curent 10000
> > do_sleep start 15733 curent 10100
> >
> > *** And the system was frozen here
> >
> > <END LOG>
> >
> > Thanks,
> > Tony
> >
> >
> >
> >
> >
> >
> > > >
> > > > At least here, you seem to have a wrap around with the 32bits AFAICT:
> > > >
> > > > > GoFlexHome> sleep 20.5
> > > > > do_sleep got a timer start = 15031
> > > > > do_sleep delay = 20000
> > > > > do_sleep delay = 20500
> > > > > do_sleep sleeping...
> > > > > do_sleep start 15031 current 100
> > > > > <snip>
> > > > > do_sleep start 15031 current 6400
> > > > > do_sleep end of sleep ... current = 4294952265
> > > > >
> > > > > *** Something strange happened here. current should be 6500, but it
> > > > > seems to have garbage. So the loop exits prematurely.
> > > >
> > > > 4294952265 = 0xFFFFC549!
> > >
> > > Yes this all needs a look, I think.
> > >
> > > Regards,
> > > Simon
>
> I think Stefan H's response above is right.
>
> drivers/timer/orion-timer.c
>
> struct orion_timer_priv {
>         void *base;
> };
>
> static uint64_t orion_timer_get_count(struct udevice *dev)
> {
>         struct orion_timer_priv *priv = dev_get_priv(dev);
>         return ~readl(priv->base + TIMER0_VAL);
> }
>
> To handle the wrap-around in a 32-bit system, it should invoke "u64
> timer_conv_64(u32 count)". This function in timer-uclass increments
> the tbh when the tbl wraps around.
>
>         return timer_conv_64(~readl(priv->base + TIMER0_VAL));
>
> I'll patch that and do some tests.

That was it. With the timer_conv_64, ticks go up and never go down.
And I don't see the system frozen anymore.

return timer_conv_64(~readl(priv->base + TIMER0_VAL));

Please squash this (or make this change in your V2 patch).

Thanks,
Tony

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hello,

On Thu, Sep 1, 2022 at 4:46 PM Tony Dinh <mibodhi@gmail.com> wrote:
>
> Hi Stefan R,
>
> On Thu, Sep 1, 2022 at 7:35 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, 1 Sept 2022 at 03:27, Stefan Roese <sr@denx.de> wrote:
> > >
> > > Hi Tony,
> > >
> > > On 01.09.22 09:39, Tony Dinh wrote:
> > >
> > > <snip>
> > >
> > > >>> Some ideas.
> > > >>>
> > > >>> The get_timer() function looks wrong assigning an uint64_t to ulong.
> > > >>>
> > > >>> lib/time.c
> > > >>>
> > > >>>       static uint64_t notrace tick_to_time(uint64_t tick)
> > > >>>       uint64_t notrace get_ticks(void)
> > > >>>       uint64_t __weak notrace get_ticks(void)
> > > >>>
> > > >>>       ulong __weak get_timer(ulong base)
> > > >>>       {
> > > >>>               return tick_to_time(get_ticks()) - base;
> > > >>>       }
> > > >>>
> > > >>> Most of the timer infrastructure is using uint64_t. I'm seeing this
> > > >>> __weak function get_timer was invoked in Kirkwood boards. Both in
> > > >>> sleep and timer commands.
> > > >>
> > > >> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
> > > >> is why we don't need a u64 for the timer.
> >
> > This is wrong, I meant that get_tbclk() can run at 1MHZ (for example).
> > The tick is 1KHz.
> >
> > > >
> > > > Thanks for your explanation! However, would you agree that code is
> > > > problematic and needed some improvement ? IOW, depending what the
> > > > compiler does, it might return the 1st 32 bit of the 64-bit integer
> > > > result?
> >
> > Yes, we should really use ulong for the tick count as well. The use of
> > 64-bits seems wrong (on 32-bit machines).
> >
> > >
> > > It will return the lower 32 bits if the system is 32bit, yes.
> > >
> > > To check if we have a problem here, please add this (totally untested)
> > > code and extend it if it makes sense:
> > >
> > > diff --git a/lib/time.c b/lib/time.c
> > > index bbf191f67323..ef5252419f3b 100644
> > > --- a/lib/time.c
> > > +++ b/lib/time.c
> > > @@ -146,7 +146,15 @@ int __weak timer_init(void)
> > >   /* Returns time in milliseconds */
> > >   ulong __weak get_timer(ulong base)
> > >   {
> > > -       return tick_to_time(get_ticks()) - base;
> > > +       u64 ticks = get_ticks();
> > > +       u64 time_ms = tick_to_time(ticks);
> > > +
> > > +       if (time_ms & 0xffffffff00000000ULL)
> > > +               printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);
> > > +       if ((time_ms - base) & 0xffffffff80000000ULL)
> > > +               printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",
> > > ticks, time_ms, base, time_ms - base);
> > > +
> > > +       return time_ms - base;
> > >   }
>
> With this patch, indeed it showed a wrap around. And the system was
> frozen during the next command.
>
> Below is the log (my annotated comment starts with ***).
>
> <BEGIN LOG>
> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Sep 01 2022 - 15:44:22 -0700)
> Pogoplug V4
>
> SoC:   Kirkwood 88F6281_A1
> Model: Cloud Engines PogoPlug Series 4
> DRAM:  128 MiB
> orion_timer_probe Clock Rate 166000000
> orion_timer_probe successful
> Core:  19 devices, 15 uclasses, devicetree: separate
> NAND:  128 MiB
> MMC:   mvsdio@90000: 0
> Loading Environment from NAND... OK
> In:    serial
> Out:   serial
> Err:   serial
> pcie0.0: Link up
> Net:   eth0: ethernet-controller@72000
> Hit any key to stop autoboot:  0
> Pogo_V4> sleep 5
> do_sleep got a timer start = 14344
> do_sleep delay = 5000
> do_sleep delay = 5000
> do_sleep sleeping...
> do_sleep start 14344 curent 100
> do_sleep start 14344 curent 200
> <snip>
> do_sleep start 14344 curent 4800
> do_sleep start 14344 curent 4900
> do_sleep end of sleep ... current = 5000
>
> Pogo_V4> sleep 10
> do_sleep got a timer start = 22370
> do_sleep delay = 10000
> do_sleep delay = 10000
> do_sleep sleeping...
> do_sleep start 22370 curent 100
> do_sleep start 22370 curent 200
> do_sleep start 22370 curent 300
> do_sleep start 22370 curent 400
> <snip>
> do_sleep start 22370 curent 3300
> do_sleep start 22370 curent 3400
> do_sleep start 22370 curent 3500
> ticks=188 time_ms=0 base=22370 ret=-22370
> do_sleep end of sleep ... current = 4294944926
>
> *** we are seeing wrap around here
>
> Pogo_V4> sleep 15
> do_sleep got a timer start = 15733
> do_sleep delay = 15000
> do_sleep delay = 15000
> do_sleep sleeping...
> do_sleep start 15733 curent 100
> do_sleep start 15733 curent 200
> do_sleep start 15733 curent 300
> do_sleep start 15733 curent 400
> <snip>
>
> do_sleep start 15733 curent 9900
> do_sleep start 15733 curent 10000
> do_sleep start 15733 curent 10100
>
> *** And the system was frozen here
>
> <END LOG>
>
> Thanks,
> Tony
>
>
>
>
>
>
> > >
> > > At least here, you seem to have a wrap around with the 32bits AFAICT:
> > >
> > > > GoFlexHome> sleep 20.5
> > > > do_sleep got a timer start = 15031
> > > > do_sleep delay = 20000
> > > > do_sleep delay = 20500
> > > > do_sleep sleeping...
> > > > do_sleep start 15031 current 100
> > > > <snip>
> > > > do_sleep start 15031 current 6400
> > > > do_sleep end of sleep ... current = 4294952265
> > > >
> > > > *** Something strange happened here. current should be 6500, but it
> > > > seems to have garbage. So the loop exits prematurely.
> > >
> > > 4294952265 = 0xFFFFC549!
> >
> > Yes this all needs a look, I think.
> >
> > Regards,
> > Simon

I think Stefan H's response above is right.

drivers/timer/orion-timer.c

struct orion_timer_priv {
        void *base;
};

static uint64_t orion_timer_get_count(struct udevice *dev)
{
        struct orion_timer_priv *priv = dev_get_priv(dev);
        return ~readl(priv->base + TIMER0_VAL);
}

To handle the wrap-around in a 32-bit system, it should invoke "u64
timer_conv_64(u32 count)". This function in timer-uclass increments
the tbh when the tbl wraps around.

        return timer_conv_64(~readl(priv->base + TIMER0_VAL));

I'll patch that and do some tests.

Thanks,
Tony

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan R,

On Thu, Sep 1, 2022 at 7:35 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Thu, 1 Sept 2022 at 03:27, Stefan Roese <sr@denx.de> wrote:
> >
> > Hi Tony,
> >
> > On 01.09.22 09:39, Tony Dinh wrote:
> >
> > <snip>
> >
> > >>> Some ideas.
> > >>>
> > >>> The get_timer() function looks wrong assigning an uint64_t to ulong.
> > >>>
> > >>> lib/time.c
> > >>>
> > >>>       static uint64_t notrace tick_to_time(uint64_t tick)
> > >>>       uint64_t notrace get_ticks(void)
> > >>>       uint64_t __weak notrace get_ticks(void)
> > >>>
> > >>>       ulong __weak get_timer(ulong base)
> > >>>       {
> > >>>               return tick_to_time(get_ticks()) - base;
> > >>>       }
> > >>>
> > >>> Most of the timer infrastructure is using uint64_t. I'm seeing this
> > >>> __weak function get_timer was invoked in Kirkwood boards. Both in
> > >>> sleep and timer commands.
> > >>
> > >> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
> > >> is why we don't need a u64 for the timer.
>
> This is wrong, I meant that get_tbclk() can run at 1MHZ (for example).
> The tick is 1KHz.
>
> > >
> > > Thanks for your explanation! However, would you agree that code is
> > > problematic and needed some improvement ? IOW, depending what the
> > > compiler does, it might return the 1st 32 bit of the 64-bit integer
> > > result?
>
> Yes, we should really use ulong for the tick count as well. The use of
> 64-bits seems wrong (on 32-bit machines).
>
> >
> > It will return the lower 32 bits if the system is 32bit, yes.
> >
> > To check if we have a problem here, please add this (totally untested)
> > code and extend it if it makes sense:
> >
> > diff --git a/lib/time.c b/lib/time.c
> > index bbf191f67323..ef5252419f3b 100644
> > --- a/lib/time.c
> > +++ b/lib/time.c
> > @@ -146,7 +146,15 @@ int __weak timer_init(void)
> >   /* Returns time in milliseconds */
> >   ulong __weak get_timer(ulong base)
> >   {
> > -       return tick_to_time(get_ticks()) - base;
> > +       u64 ticks = get_ticks();
> > +       u64 time_ms = tick_to_time(ticks);
> > +
> > +       if (time_ms & 0xffffffff00000000ULL)
> > +               printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);
> > +       if ((time_ms - base) & 0xffffffff80000000ULL)
> > +               printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",
> > ticks, time_ms, base, time_ms - base);
> > +
> > +       return time_ms - base;
> >   }

With this patch, indeed it showed a wrap around. And the system was
frozen during the next command.

Below is the log (my annotated comment starts with ***).

<BEGIN LOG>
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Sep 01 2022 - 15:44:22 -0700)
Pogoplug V4

SoC:   Kirkwood 88F6281_A1
Model: Cloud Engines PogoPlug Series 4
DRAM:  128 MiB
orion_timer_probe Clock Rate 166000000
orion_timer_probe successful
Core:  19 devices, 15 uclasses, devicetree: separate
NAND:  128 MiB
MMC:   mvsdio@90000: 0
Loading Environment from NAND... OK
In:    serial
Out:   serial
Err:   serial
pcie0.0: Link up
Net:   eth0: ethernet-controller@72000
Hit any key to stop autoboot:  0
Pogo_V4> sleep 5
do_sleep got a timer start = 14344
do_sleep delay = 5000
do_sleep delay = 5000
do_sleep sleeping...
do_sleep start 14344 curent 100
do_sleep start 14344 curent 200
<snip>
do_sleep start 14344 curent 4800
do_sleep start 14344 curent 4900
do_sleep end of sleep ... current = 5000

Pogo_V4> sleep 10
do_sleep got a timer start = 22370
do_sleep delay = 10000
do_sleep delay = 10000
do_sleep sleeping...
do_sleep start 22370 curent 100
do_sleep start 22370 curent 200
do_sleep start 22370 curent 300
do_sleep start 22370 curent 400
<snip>
do_sleep start 22370 curent 3300
do_sleep start 22370 curent 3400
do_sleep start 22370 curent 3500
ticks=188 time_ms=0 base=22370 ret=-22370
do_sleep end of sleep ... current = 4294944926

*** we are seeing wrap around here

Pogo_V4> sleep 15
do_sleep got a timer start = 15733
do_sleep delay = 15000
do_sleep delay = 15000
do_sleep sleeping...
do_sleep start 15733 curent 100
do_sleep start 15733 curent 200
do_sleep start 15733 curent 300
do_sleep start 15733 curent 400
<snip>

do_sleep start 15733 curent 9900
do_sleep start 15733 curent 10000
do_sleep start 15733 curent 10100

*** And the system was frozen here

<END LOG>

Thanks,
Tony






> >
> > At least here, you seem to have a wrap around with the 32bits AFAICT:
> >
> > > GoFlexHome> sleep 20.5
> > > do_sleep got a timer start = 15031
> > > do_sleep delay = 20000
> > > do_sleep delay = 20500
> > > do_sleep sleeping...
> > > do_sleep start 15031 current 100
> > > <snip>
> > > do_sleep start 15031 current 6400
> > > do_sleep end of sleep ... current = 4294952265
> > >
> > > *** Something strange happened here. current should be 6500, but it
> > > seems to have garbage. So the loop exits prematurely.
> >
> > 4294952265 = 0xFFFFC549!
>
> Yes this all needs a look, I think.
>
> Regards,
> Simon

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Simon Glass]

Hi,

On Thu, 1 Sept 2022 at 03:27, Stefan Roese <sr@denx.de> wrote:
>
> Hi Tony,
>
> On 01.09.22 09:39, Tony Dinh wrote:
>
> <snip>
>
> >>> Some ideas.
> >>>
> >>> The get_timer() function looks wrong assigning an uint64_t to ulong.
> >>>
> >>> lib/time.c
> >>>
> >>>       static uint64_t notrace tick_to_time(uint64_t tick)
> >>>       uint64_t notrace get_ticks(void)
> >>>       uint64_t __weak notrace get_ticks(void)
> >>>
> >>>       ulong __weak get_timer(ulong base)
> >>>       {
> >>>               return tick_to_time(get_ticks()) - base;
> >>>       }
> >>>
> >>> Most of the timer infrastructure is using uint64_t. I'm seeing this
> >>> __weak function get_timer was invoked in Kirkwood boards. Both in
> >>> sleep and timer commands.
> >>
> >> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
> >> is why we don't need a u64 for the timer.

This is wrong, I meant that get_tbclk() can run at 1MHZ (for example).
The tick is 1KHz.

> >
> > Thanks for your explanation! However, would you agree that code is
> > problematic and needed some improvement ? IOW, depending what the
> > compiler does, it might return the 1st 32 bit of the 64-bit integer
> > result?

Yes, we should really use ulong for the tick count as well. The use of
64-bits seems wrong (on 32-bit machines).

>
> It will return the lower 32 bits if the system is 32bit, yes.
>
> To check if we have a problem here, please add this (totally untested)
> code and extend it if it makes sense:
>
> diff --git a/lib/time.c b/lib/time.c
> index bbf191f67323..ef5252419f3b 100644
> --- a/lib/time.c
> +++ b/lib/time.c
> @@ -146,7 +146,15 @@ int __weak timer_init(void)
>   /* Returns time in milliseconds */
>   ulong __weak get_timer(ulong base)
>   {
> -       return tick_to_time(get_ticks()) - base;
> +       u64 ticks = get_ticks();
> +       u64 time_ms = tick_to_time(ticks);
> +
> +       if (time_ms & 0xffffffff00000000ULL)
> +               printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);
> +       if ((time_ms - base) & 0xffffffff80000000ULL)
> +               printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",
> ticks, time_ms, base, time_ms - base);
> +
> +       return time_ms - base;
>   }
>
> At least here, you seem to have a wrap around with the 32bits AFAICT:
>
> > GoFlexHome> sleep 20.5
> > do_sleep got a timer start = 15031
> > do_sleep delay = 20000
> > do_sleep delay = 20500
> > do_sleep sleeping...
> > do_sleep start 15031 current 100
> > <snip>
> > do_sleep start 15031 current 6400
> > do_sleep end of sleep ... current = 4294952265
> >
> > *** Something strange happened here. current should be 6500, but it
> > seems to have garbage. So the loop exits prematurely.
>
> 4294952265 = 0xFFFFC549!

Yes this all needs a look, I think.

Regards,
Simon

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Herbrechtsmeier]

Hi,

Am 01.09.2022 um 11:27 schrieb Stefan Roese:
> Hi Tony,
> 
> On 01.09.22 09:39, Tony Dinh wrote:
> 
> <snip>
> 
>>>> Some ideas.
>>>>
>>>> The get_timer() function looks wrong assigning an uint64_t to ulong.
>>>>
>>>> lib/time.c
>>>>
>>>>       static uint64_t notrace tick_to_time(uint64_t tick)
>>>>       uint64_t notrace get_ticks(void)
>>>>       uint64_t __weak notrace get_ticks(void)
>>>>
>>>>       ulong __weak get_timer(ulong base)
>>>>       {
>>>>               return tick_to_time(get_ticks()) - base;
>>>>       }
>>>>
>>>> Most of the timer infrastructure is using uint64_t. I'm seeing this
>>>> __weak function get_timer was invoked in Kirkwood boards. Both in
>>>> sleep and timer commands.
>>>
>>> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
>>> is why we don't need a u64 for the timer.
>>
>> Thanks for your explanation! However, would you agree that code is
>> problematic and needed some improvement ? IOW, depending what the
>> compiler does, it might return the 1st 32 bit of the 64-bit integer
>> result?
> 
> It will return the lower 32 bits if the system is 32bit, yes.
> 
> To check if we have a problem here, please add this (totally untested)
> code and extend it if it makes sense:
> 
> diff --git a/lib/time.c b/lib/time.c
> index bbf191f67323..ef5252419f3b 100644
> --- a/lib/time.c
> +++ b/lib/time.c
> @@ -146,7 +146,15 @@ int __weak timer_init(void)
>   /* Returns time in milliseconds */
>   ulong __weak get_timer(ulong base)
>   {
> -       return tick_to_time(get_ticks()) - base;
> +       u64 ticks = get_ticks();
> +       u64 time_ms = tick_to_time(ticks);
> +
> +       if (time_ms & 0xffffffff00000000ULL)
> +               printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);
> +       if ((time_ms - base) & 0xffffffff80000000ULL)
> +               printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n", 
> ticks, time_ms, base, time_ms - base);
> +
> +       return time_ms - base;
>   }
> 
> At least here, you seem to have a wrap around with the 32bits AFAICT:
> 
>> GoFlexHome> sleep 20.5
>> do_sleep got a timer start = 15031
>> do_sleep delay = 20000
>> do_sleep delay = 20500
>> do_sleep sleeping...
>> do_sleep start 15031 current 100
>> <snip>
>> do_sleep start 15031 current 6400
>> do_sleep end of sleep ... current = 4294952265
>>
>> *** Something strange happened here. current should be 6500, but it
>> seems to have garbage. So the loop exits prematurely.
> 
> 4294952265 = 0xFFFFC549!
> 

Does the driver use a 32 bit counter without the timer_conv_64 function 
inside the get_count function?

Regards
   Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

Hi Tony,

On 01.09.22 09:39, Tony Dinh wrote:

<snip>

>>> Some ideas.
>>>
>>> The get_timer() function looks wrong assigning an uint64_t to ulong.
>>>
>>> lib/time.c
>>>
>>>       static uint64_t notrace tick_to_time(uint64_t tick)
>>>       uint64_t notrace get_ticks(void)
>>>       uint64_t __weak notrace get_ticks(void)
>>>
>>>       ulong __weak get_timer(ulong base)
>>>       {
>>>               return tick_to_time(get_ticks()) - base;
>>>       }
>>>
>>> Most of the timer infrastructure is using uint64_t. I'm seeing this
>>> __weak function get_timer was invoked in Kirkwood boards. Both in
>>> sleep and timer commands.
>>
>> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
>> is why we don't need a u64 for the timer.
> 
> Thanks for your explanation! However, would you agree that code is
> problematic and needed some improvement ? IOW, depending what the
> compiler does, it might return the 1st 32 bit of the 64-bit integer
> result?

It will return the lower 32 bits if the system is 32bit, yes.

To check if we have a problem here, please add this (totally untested)
code and extend it if it makes sense:

diff --git a/lib/time.c b/lib/time.c
index bbf191f67323..ef5252419f3b 100644
--- a/lib/time.c
+++ b/lib/time.c
@@ -146,7 +146,15 @@ int __weak timer_init(void)
  /* Returns time in milliseconds */
  ulong __weak get_timer(ulong base)
  {
-       return tick_to_time(get_ticks()) - base;
+       u64 ticks = get_ticks();
+       u64 time_ms = tick_to_time(ticks);
+
+       if (time_ms & 0xffffffff00000000ULL)
+               printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);
+       if ((time_ms - base) & 0xffffffff80000000ULL)
+               printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n", 
ticks, time_ms, base, time_ms - base);
+
+       return time_ms - base;
  }

At least here, you seem to have a wrap around with the 32bits AFAICT:

> GoFlexHome> sleep 20.5
> do_sleep got a timer start = 15031
> do_sleep delay = 20000
> do_sleep delay = 20500
> do_sleep sleeping...
> do_sleep start 15031 current 100
> <snip>
> do_sleep start 15031 current 6400
> do_sleep end of sleep ... current = 4294952265
> 
> *** Something strange happened here. current should be 6500, but it
> seems to have garbage. So the loop exits prematurely.

4294952265 = 0xFFFFC549!

Thanks,
Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Simon,

On Wed, Aug 31, 2022 at 7:27 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tony,
>
> On Wed, 31 Aug 2022 at 19:39, Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > Hi Stefan,
> >
> > On Wed, Aug 31, 2022 at 2:53 PM Tony Dinh <mibodhi@gmail.com> wrote:
> > >
> > > Hi Stefan,
> > >
> > > On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh <mibodhi@gmail.com> wrote:
> > > >
> > > > Hi Stefan,
> > > >
> > > > On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese <sr@denx.de> wrote:
> > > > >
> > > > > Hi Tony,
> > > > >
> > > > > On 31.08.22 08:30, Tony Dinh wrote:
> > > > > > Hi Stefan,
> > > > > >
> > > > > > On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese <sr@denx.de> wrote:
> > > > > >>
> > > > > >> Hi Tony,
> > > > > >>
> > > > > >> On 31.08.22 07:02, Stefan Roese wrote:
> > > > > >>> Hi Tony,
> > > > > >>>
> > > > > >>> On 31.08.22 00:15, Tony Dinh wrote:
> > > > > >>>> Hi Stefan,
> > > > > >>>>
> > > > > >>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
> > > > > >>>>>
> > > > > >>>>> This patchset enhaces the recently added Orion Timer driver to support
> > > > > >>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> > > > > >>>>> timer support is then enabled per default for those platforms, so that
> > > > > >>>>> the board config files don't need to be changed. Also necessary is
> > > > > >>>>> some dts hacking, so that the timer DT node is available in early
> > > > > >>>>> U-Boot stages.
> > > > > >>>>>
> > > > > >>>>> I've successfully tested this patchset on an Armada XP board. Additional
> > > > > >>>>> test on other boards and platforms are very welcome and necessary.
> > > > > >>>>
> > > > > >>>> I've run some tests with the following 2 Kirkwood boards: Cloud
> > > > > >>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> > > > > >>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> > > > > >>>>
> > > > > >>>> It seems that it was either frozen or the timer did not expire at some
> > > > > >>>> subsequent sleep commands. Sometime it happened at 2nd command, some
> > > > > >>>> time at a later sleep command. For example,
> > > > > >>>>
> > > > > >>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> > > > > >>>> stopwatch)
> > > > > >>>>
> > > > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
> > > > > >>>> Pogoplug V4
> > > > > >>>>
> > > > > >>>> Hit any key to stop autoboot:  0
> > > > > >>>> Pogo_V4> sleep 10
> > > > > >>>> Pogo_V4> sleep 31.5
> > > > > >>>> <frozen here>
> > > > > >>>
> > > > > >>> Does the cmd interface support fractial numbers? Please test again with
> > > > > >>> 31 or other integral numbers.
> > > > > >>>
> > > > > >>>> === Sheevaplug (RTC battery is old, so the date was not updated, but
> > > > > >>>> the clock seems OK)
> > > > > >>>>
> > > > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
> > > > > >>>> Marvell-Sheevaplug
> > > > > >>>> Hit any key to stop autoboot:  0
> > > > > >>>> => date
> > > > > >>>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> > > > > >>>> => sleep 10
> > > > > >>>> => date
> > > > > >>>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> > > > > >>>> => sleep 10
> > > > > >>>> => sleep 20.1
> > > > > >>>> <frozen here>
> > > > > >>>>
> > > > > >>>> Please let me know what I can do (i.e. perhaps running a debug patch).
> > > > > >>>
> > > > > >>> Please see above. I assume that the fractional numbers result in very
> > > > > >>> long numbers internally, which result in a frozen / hanging system.
> > > > > >>
> > > > > >> I just tested fractional numbers on another board and hey, it just
> > > > > >> works. Learned something new. So we seem to have a problem here. Let
> > > > > >> me see, if I can find something.
> > > > > >
> > > > > > I've added debug printfs and possibly tracked down this issue. Seems
> > > > > > like in the 2nd call to sleep, get_timer(0) did not reset the start
> > > > > > number.
> > > > > >
> > > > > > cmd/sleep.c
> > > > > > static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > >                      char *const argv[])
> > > > > > {
> > > > > >         ulong start = get_timer(0);
> > > > > >
> > > > > > "do_sleep got a timer start = 2015" is the 1st call to sleep 5.
> > > > > > "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
> > > > > >
> > > > > > <BEGIN log>
> > > > > > Pogo_V4> sleep 5
> > > > > > do_sleep got a timer start = 2015
> > > > > > do_sleep delay = 5000
> > > > > > do_sleep delay = 5000
> > > > > > do_sleep sleeping...
> > > > > > do_sleep start 2015 curent 100
> > > > > > do_sleep start 2015 curent 200
> > > > > > do_sleep start 2015 curent 300
> > > > > > <snip>
> > > > > > do_sleep start 2015 curent 4900
> > > > > > do_sleep end of sleep ... current = 5000
> > > > > > Pogo_V4>
> > > > > >
> > > > > > Pogo_V4> sleep 10
> > > > > > do_sleep got a timer start = 16304
> > > > > > do_sleep delay = 10000
> > > > > > do_sleep delay = 10000
> > > > > > do_sleep sleeping...
> > > > > > <snip>
> > > > > >
> > > > > > <END log>
> > > > > >
> > > > > > So somewhere in the DM timer, "start" got accumulated. I think each
> > > > > > get_timer(0) should be a different timer instance. It looks like the
> > > > > > same timer instance is used again and again, causing the "start "to
> > > > > > grow bigger, and at one point it might just overflow.
> > > > >
> > > > > Frankly I don't really understand the problem you describe above. What
> > > > > do you mean with "timer instance"? get_timer(0) will return different
> > > > > values, depending on when you call this function. So where exactly is
> > > > > the problem with the 2nd "sleep 10" above?
> > > >
> > > > Please ignore what I said above! I misunderstood what get_timer()
> > > > does. I'll try again on another KIrkwood  board to see if the behavior
> > > > will be the same.
> > >
> > > I've  run the tests again with the Seagate GoFlex Home board (Kirkwood
> > > 88F6281). Below is the log, with my annotated comments starting with
> > > ****.
> > > In these tests,  current = get_timer(start) is inside the while loop.
> > > And I printed out the start and current values when (current % 100 ==
> > > 0).
> > >
> > > <BEGIN LOG>
> > > U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700)
> > > Seagate GoFlex Home
> > >
> > > SoC:   Kirkwood 88F6281_A1
> > > DRAM:  128 MiB
> > > Core:  12 devices, 10 uclasses, devicetree: separate
> > > NAND:  orion_timer_probe successful
> > > 256 MiB
> > > Loading Environment from NAND... OK
> > > In:    serial
> > > Out:   serial
> > > Err:   serial
> > > Net:   eth0: ethernet-controller@72000
> > > Hit any key to stop autoboot:  0
> > >
> > > GoFlexHome> date
> > > Date: 2022-08-31 (Wednesday)    Time: 20:43:03
> > >
> > > GoFlexHome> sleep 5
> > > do_sleep got a timer start = 1244
> > > do_sleep delay = 5000
> > > do_sleep delay = 5000
> > > do_sleep sleeping...
> > > do_sleep start 1244 current 100
> > > <snip>
> > > do_sleep start 1244 current 2300
> > > <snip>
> > > do_sleep start 1244 current 3700
> > > <snip>
> > > do_sleep start 1244 current 4800
> > > do_sleep start 1244 current 4900
> > > do_sleep end of sleep ... current = 5000
> > >
> > > **** working as intended
> > >
> > > GoFlexHome> sleep 10
> > > do_sleep got a timer start = 11428
> > > do_sleep delay = 10000
> > > do_sleep delay = 10000
> > > do_sleep sleeping...
> > > do_sleep start 11428 current 100
> > > <snip>
> > > do_sleep start 11428 current 2300
> > > <snip>
> > > do_sleep start 11428 current 9900
> > > do_sleep end of sleep ... current = 10000
> > >
> > > **** working as intended
> > >
> > > GoFlexHome> sleep 20.5
> > > do_sleep got a timer start = 15031
> > > do_sleep delay = 20000
> > > do_sleep delay = 20500
> > > do_sleep sleeping...
> > > do_sleep start 15031 current 100
> > > <snip>
> > > do_sleep start 15031 current 6400
> > > do_sleep end of sleep ... current = 4294952265
> > >
> > > *** Something strange happened here. current should be 6500, but it
> > > seems to have garbage. So the loop exits prematurely.
> > >
> > > GoFlexHome> sleep 20.5
> > > do_sleep got a timer start = 8339
> > > do_sleep delay = 20000
> > > do_sleep delay = 20500
> > > do_sleep sleeping...
> > > do_sleep start 8339 current 100
> > > do_sleep start 8339 current 200
> > > <snip>
> > > do_sleep start 8339 current 12300
> > > do_sleep start 8339 current 12400
> > > do_sleep start 8339 current 12500
> > > do_sleep start 8339 current 12600
> > > do_sleep start 8339 current 12700
> > > do_sleep start 8339 current 12800
> > > do_sleep start 8339 current 12900
> > > do_sleep start 8339 current 13000
> > > do_sleep start 8339 current 13100
> > >
> > > *** It was frozen right here. Control-C could not interrupt the loop,
> > > so my guess
> > > *** either it was stuck during the call to get_timer(). Or the current value
> > > *** overflowed and crashed the system.
> > >
> > > <END LOG>
> > >
> > > Finally, I removed the patch set, and rerun the sleep tests on this
> > > board. They all worked fine, I did not see anything strange.
> > >
> >
> > Some ideas.
> >
> > The get_timer() function looks wrong assigning an uint64_t to ulong.
> >
> > lib/time.c
> >
> >      static uint64_t notrace tick_to_time(uint64_t tick)
> >      uint64_t notrace get_ticks(void)
> >      uint64_t __weak notrace get_ticks(void)
> >
> >      ulong __weak get_timer(ulong base)
> >      {
> >              return tick_to_time(get_ticks()) - base;
> >      }
> >
> > Most of the timer infrastructure is using uint64_t. I'm seeing this
> > __weak function get_timer was invoked in Kirkwood boards. Both in
> > sleep and timer commands.
>
> The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
> is why we don't need a u64 for the timer.

Thanks for your explanation! However, would you agree that code is
problematic and needed some improvement ? IOW, depending what the
compiler does, it might return the 1st 32 bit of the 64-bit integer
result?

Thanks,
Tony

> Regards,
> Simon

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Simon Glass]

Hi Tony,

On Wed, 31 Aug 2022 at 19:39, Tony Dinh <mibodhi@gmail.com> wrote:
>
> Hi Stefan,
>
> On Wed, Aug 31, 2022 at 2:53 PM Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > Hi Stefan,
> >
> > On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh <mibodhi@gmail.com> wrote:
> > >
> > > Hi Stefan,
> > >
> > > On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese <sr@denx.de> wrote:
> > > >
> > > > Hi Tony,
> > > >
> > > > On 31.08.22 08:30, Tony Dinh wrote:
> > > > > Hi Stefan,
> > > > >
> > > > > On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese <sr@denx.de> wrote:
> > > > >>
> > > > >> Hi Tony,
> > > > >>
> > > > >> On 31.08.22 07:02, Stefan Roese wrote:
> > > > >>> Hi Tony,
> > > > >>>
> > > > >>> On 31.08.22 00:15, Tony Dinh wrote:
> > > > >>>> Hi Stefan,
> > > > >>>>
> > > > >>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
> > > > >>>>>
> > > > >>>>> This patchset enhaces the recently added Orion Timer driver to support
> > > > >>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> > > > >>>>> timer support is then enabled per default for those platforms, so that
> > > > >>>>> the board config files don't need to be changed. Also necessary is
> > > > >>>>> some dts hacking, so that the timer DT node is available in early
> > > > >>>>> U-Boot stages.
> > > > >>>>>
> > > > >>>>> I've successfully tested this patchset on an Armada XP board. Additional
> > > > >>>>> test on other boards and platforms are very welcome and necessary.
> > > > >>>>
> > > > >>>> I've run some tests with the following 2 Kirkwood boards: Cloud
> > > > >>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> > > > >>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> > > > >>>>
> > > > >>>> It seems that it was either frozen or the timer did not expire at some
> > > > >>>> subsequent sleep commands. Sometime it happened at 2nd command, some
> > > > >>>> time at a later sleep command. For example,
> > > > >>>>
> > > > >>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> > > > >>>> stopwatch)
> > > > >>>>
> > > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
> > > > >>>> Pogoplug V4
> > > > >>>>
> > > > >>>> Hit any key to stop autoboot:  0
> > > > >>>> Pogo_V4> sleep 10
> > > > >>>> Pogo_V4> sleep 31.5
> > > > >>>> <frozen here>
> > > > >>>
> > > > >>> Does the cmd interface support fractial numbers? Please test again with
> > > > >>> 31 or other integral numbers.
> > > > >>>
> > > > >>>> === Sheevaplug (RTC battery is old, so the date was not updated, but
> > > > >>>> the clock seems OK)
> > > > >>>>
> > > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
> > > > >>>> Marvell-Sheevaplug
> > > > >>>> Hit any key to stop autoboot:  0
> > > > >>>> => date
> > > > >>>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> > > > >>>> => sleep 10
> > > > >>>> => date
> > > > >>>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> > > > >>>> => sleep 10
> > > > >>>> => sleep 20.1
> > > > >>>> <frozen here>
> > > > >>>>
> > > > >>>> Please let me know what I can do (i.e. perhaps running a debug patch).
> > > > >>>
> > > > >>> Please see above. I assume that the fractional numbers result in very
> > > > >>> long numbers internally, which result in a frozen / hanging system.
> > > > >>
> > > > >> I just tested fractional numbers on another board and hey, it just
> > > > >> works. Learned something new. So we seem to have a problem here. Let
> > > > >> me see, if I can find something.
> > > > >
> > > > > I've added debug printfs and possibly tracked down this issue. Seems
> > > > > like in the 2nd call to sleep, get_timer(0) did not reset the start
> > > > > number.
> > > > >
> > > > > cmd/sleep.c
> > > > > static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > >                      char *const argv[])
> > > > > {
> > > > >         ulong start = get_timer(0);
> > > > >
> > > > > "do_sleep got a timer start = 2015" is the 1st call to sleep 5.
> > > > > "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
> > > > >
> > > > > <BEGIN log>
> > > > > Pogo_V4> sleep 5
> > > > > do_sleep got a timer start = 2015
> > > > > do_sleep delay = 5000
> > > > > do_sleep delay = 5000
> > > > > do_sleep sleeping...
> > > > > do_sleep start 2015 curent 100
> > > > > do_sleep start 2015 curent 200
> > > > > do_sleep start 2015 curent 300
> > > > > <snip>
> > > > > do_sleep start 2015 curent 4900
> > > > > do_sleep end of sleep ... current = 5000
> > > > > Pogo_V4>
> > > > >
> > > > > Pogo_V4> sleep 10
> > > > > do_sleep got a timer start = 16304
> > > > > do_sleep delay = 10000
> > > > > do_sleep delay = 10000
> > > > > do_sleep sleeping...
> > > > > <snip>
> > > > >
> > > > > <END log>
> > > > >
> > > > > So somewhere in the DM timer, "start" got accumulated. I think each
> > > > > get_timer(0) should be a different timer instance. It looks like the
> > > > > same timer instance is used again and again, causing the "start "to
> > > > > grow bigger, and at one point it might just overflow.
> > > >
> > > > Frankly I don't really understand the problem you describe above. What
> > > > do you mean with "timer instance"? get_timer(0) will return different
> > > > values, depending on when you call this function. So where exactly is
> > > > the problem with the 2nd "sleep 10" above?
> > >
> > > Please ignore what I said above! I misunderstood what get_timer()
> > > does. I'll try again on another KIrkwood  board to see if the behavior
> > > will be the same.
> >
> > I've  run the tests again with the Seagate GoFlex Home board (Kirkwood
> > 88F6281). Below is the log, with my annotated comments starting with
> > ****.
> > In these tests,  current = get_timer(start) is inside the while loop.
> > And I printed out the start and current values when (current % 100 ==
> > 0).
> >
> > <BEGIN LOG>
> > U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700)
> > Seagate GoFlex Home
> >
> > SoC:   Kirkwood 88F6281_A1
> > DRAM:  128 MiB
> > Core:  12 devices, 10 uclasses, devicetree: separate
> > NAND:  orion_timer_probe successful
> > 256 MiB
> > Loading Environment from NAND... OK
> > In:    serial
> > Out:   serial
> > Err:   serial
> > Net:   eth0: ethernet-controller@72000
> > Hit any key to stop autoboot:  0
> >
> > GoFlexHome> date
> > Date: 2022-08-31 (Wednesday)    Time: 20:43:03
> >
> > GoFlexHome> sleep 5
> > do_sleep got a timer start = 1244
> > do_sleep delay = 5000
> > do_sleep delay = 5000
> > do_sleep sleeping...
> > do_sleep start 1244 current 100
> > <snip>
> > do_sleep start 1244 current 2300
> > <snip>
> > do_sleep start 1244 current 3700
> > <snip>
> > do_sleep start 1244 current 4800
> > do_sleep start 1244 current 4900
> > do_sleep end of sleep ... current = 5000
> >
> > **** working as intended
> >
> > GoFlexHome> sleep 10
> > do_sleep got a timer start = 11428
> > do_sleep delay = 10000
> > do_sleep delay = 10000
> > do_sleep sleeping...
> > do_sleep start 11428 current 100
> > <snip>
> > do_sleep start 11428 current 2300
> > <snip>
> > do_sleep start 11428 current 9900
> > do_sleep end of sleep ... current = 10000
> >
> > **** working as intended
> >
> > GoFlexHome> sleep 20.5
> > do_sleep got a timer start = 15031
> > do_sleep delay = 20000
> > do_sleep delay = 20500
> > do_sleep sleeping...
> > do_sleep start 15031 current 100
> > <snip>
> > do_sleep start 15031 current 6400
> > do_sleep end of sleep ... current = 4294952265
> >
> > *** Something strange happened here. current should be 6500, but it
> > seems to have garbage. So the loop exits prematurely.
> >
> > GoFlexHome> sleep 20.5
> > do_sleep got a timer start = 8339
> > do_sleep delay = 20000
> > do_sleep delay = 20500
> > do_sleep sleeping...
> > do_sleep start 8339 current 100
> > do_sleep start 8339 current 200
> > <snip>
> > do_sleep start 8339 current 12300
> > do_sleep start 8339 current 12400
> > do_sleep start 8339 current 12500
> > do_sleep start 8339 current 12600
> > do_sleep start 8339 current 12700
> > do_sleep start 8339 current 12800
> > do_sleep start 8339 current 12900
> > do_sleep start 8339 current 13000
> > do_sleep start 8339 current 13100
> >
> > *** It was frozen right here. Control-C could not interrupt the loop,
> > so my guess
> > *** either it was stuck during the call to get_timer(). Or the current value
> > *** overflowed and crashed the system.
> >
> > <END LOG>
> >
> > Finally, I removed the patch set, and rerun the sleep tests on this
> > board. They all worked fine, I did not see anything strange.
> >
>
> Some ideas.
>
> The get_timer() function looks wrong assigning an uint64_t to ulong.
>
> lib/time.c
>
>      static uint64_t notrace tick_to_time(uint64_t tick)
>      uint64_t notrace get_ticks(void)
>      uint64_t __weak notrace get_ticks(void)
>
>      ulong __weak get_timer(ulong base)
>      {
>              return tick_to_time(get_ticks()) - base;
>      }
>
> Most of the timer infrastructure is using uint64_t. I'm seeing this
> __weak function get_timer was invoked in Kirkwood boards. Both in
> sleep and timer commands.

The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that
is why we don't need a u64 for the timer.

Regards,
Simon

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan,

On Wed, Aug 31, 2022 at 2:53 PM Tony Dinh <mibodhi@gmail.com> wrote:
>
> Hi Stefan,
>
> On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > Hi Stefan,
> >
> > On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese <sr@denx.de> wrote:
> > >
> > > Hi Tony,
> > >
> > > On 31.08.22 08:30, Tony Dinh wrote:
> > > > Hi Stefan,
> > > >
> > > > On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese <sr@denx.de> wrote:
> > > >>
> > > >> Hi Tony,
> > > >>
> > > >> On 31.08.22 07:02, Stefan Roese wrote:
> > > >>> Hi Tony,
> > > >>>
> > > >>> On 31.08.22 00:15, Tony Dinh wrote:
> > > >>>> Hi Stefan,
> > > >>>>
> > > >>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
> > > >>>>>
> > > >>>>> This patchset enhaces the recently added Orion Timer driver to support
> > > >>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> > > >>>>> timer support is then enabled per default for those platforms, so that
> > > >>>>> the board config files don't need to be changed. Also necessary is
> > > >>>>> some dts hacking, so that the timer DT node is available in early
> > > >>>>> U-Boot stages.
> > > >>>>>
> > > >>>>> I've successfully tested this patchset on an Armada XP board. Additional
> > > >>>>> test on other boards and platforms are very welcome and necessary.
> > > >>>>
> > > >>>> I've run some tests with the following 2 Kirkwood boards: Cloud
> > > >>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> > > >>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> > > >>>>
> > > >>>> It seems that it was either frozen or the timer did not expire at some
> > > >>>> subsequent sleep commands. Sometime it happened at 2nd command, some
> > > >>>> time at a later sleep command. For example,
> > > >>>>
> > > >>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> > > >>>> stopwatch)
> > > >>>>
> > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
> > > >>>> Pogoplug V4
> > > >>>>
> > > >>>> Hit any key to stop autoboot:  0
> > > >>>> Pogo_V4> sleep 10
> > > >>>> Pogo_V4> sleep 31.5
> > > >>>> <frozen here>
> > > >>>
> > > >>> Does the cmd interface support fractial numbers? Please test again with
> > > >>> 31 or other integral numbers.
> > > >>>
> > > >>>> === Sheevaplug (RTC battery is old, so the date was not updated, but
> > > >>>> the clock seems OK)
> > > >>>>
> > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
> > > >>>> Marvell-Sheevaplug
> > > >>>> Hit any key to stop autoboot:  0
> > > >>>> => date
> > > >>>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> > > >>>> => sleep 10
> > > >>>> => date
> > > >>>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> > > >>>> => sleep 10
> > > >>>> => sleep 20.1
> > > >>>> <frozen here>
> > > >>>>
> > > >>>> Please let me know what I can do (i.e. perhaps running a debug patch).
> > > >>>
> > > >>> Please see above. I assume that the fractional numbers result in very
> > > >>> long numbers internally, which result in a frozen / hanging system.
> > > >>
> > > >> I just tested fractional numbers on another board and hey, it just
> > > >> works. Learned something new. So we seem to have a problem here. Let
> > > >> me see, if I can find something.
> > > >
> > > > I've added debug printfs and possibly tracked down this issue. Seems
> > > > like in the 2nd call to sleep, get_timer(0) did not reset the start
> > > > number.
> > > >
> > > > cmd/sleep.c
> > > > static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                      char *const argv[])
> > > > {
> > > >         ulong start = get_timer(0);
> > > >
> > > > "do_sleep got a timer start = 2015" is the 1st call to sleep 5.
> > > > "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
> > > >
> > > > <BEGIN log>
> > > > Pogo_V4> sleep 5
> > > > do_sleep got a timer start = 2015
> > > > do_sleep delay = 5000
> > > > do_sleep delay = 5000
> > > > do_sleep sleeping...
> > > > do_sleep start 2015 curent 100
> > > > do_sleep start 2015 curent 200
> > > > do_sleep start 2015 curent 300
> > > > <snip>
> > > > do_sleep start 2015 curent 4900
> > > > do_sleep end of sleep ... current = 5000
> > > > Pogo_V4>
> > > >
> > > > Pogo_V4> sleep 10
> > > > do_sleep got a timer start = 16304
> > > > do_sleep delay = 10000
> > > > do_sleep delay = 10000
> > > > do_sleep sleeping...
> > > > <snip>
> > > >
> > > > <END log>
> > > >
> > > > So somewhere in the DM timer, "start" got accumulated. I think each
> > > > get_timer(0) should be a different timer instance. It looks like the
> > > > same timer instance is used again and again, causing the "start "to
> > > > grow bigger, and at one point it might just overflow.
> > >
> > > Frankly I don't really understand the problem you describe above. What
> > > do you mean with "timer instance"? get_timer(0) will return different
> > > values, depending on when you call this function. So where exactly is
> > > the problem with the 2nd "sleep 10" above?
> >
> > Please ignore what I said above! I misunderstood what get_timer()
> > does. I'll try again on another KIrkwood  board to see if the behavior
> > will be the same.
>
> I've  run the tests again with the Seagate GoFlex Home board (Kirkwood
> 88F6281). Below is the log, with my annotated comments starting with
> ****.
> In these tests,  current = get_timer(start) is inside the while loop.
> And I printed out the start and current values when (current % 100 ==
> 0).
>
> <BEGIN LOG>
> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700)
> Seagate GoFlex Home
>
> SoC:   Kirkwood 88F6281_A1
> DRAM:  128 MiB
> Core:  12 devices, 10 uclasses, devicetree: separate
> NAND:  orion_timer_probe successful
> 256 MiB
> Loading Environment from NAND... OK
> In:    serial
> Out:   serial
> Err:   serial
> Net:   eth0: ethernet-controller@72000
> Hit any key to stop autoboot:  0
>
> GoFlexHome> date
> Date: 2022-08-31 (Wednesday)    Time: 20:43:03
>
> GoFlexHome> sleep 5
> do_sleep got a timer start = 1244
> do_sleep delay = 5000
> do_sleep delay = 5000
> do_sleep sleeping...
> do_sleep start 1244 current 100
> <snip>
> do_sleep start 1244 current 2300
> <snip>
> do_sleep start 1244 current 3700
> <snip>
> do_sleep start 1244 current 4800
> do_sleep start 1244 current 4900
> do_sleep end of sleep ... current = 5000
>
> **** working as intended
>
> GoFlexHome> sleep 10
> do_sleep got a timer start = 11428
> do_sleep delay = 10000
> do_sleep delay = 10000
> do_sleep sleeping...
> do_sleep start 11428 current 100
> <snip>
> do_sleep start 11428 current 2300
> <snip>
> do_sleep start 11428 current 9900
> do_sleep end of sleep ... current = 10000
>
> **** working as intended
>
> GoFlexHome> sleep 20.5
> do_sleep got a timer start = 15031
> do_sleep delay = 20000
> do_sleep delay = 20500
> do_sleep sleeping...
> do_sleep start 15031 current 100
> <snip>
> do_sleep start 15031 current 6400
> do_sleep end of sleep ... current = 4294952265
>
> *** Something strange happened here. current should be 6500, but it
> seems to have garbage. So the loop exits prematurely.
>
> GoFlexHome> sleep 20.5
> do_sleep got a timer start = 8339
> do_sleep delay = 20000
> do_sleep delay = 20500
> do_sleep sleeping...
> do_sleep start 8339 current 100
> do_sleep start 8339 current 200
> <snip>
> do_sleep start 8339 current 12300
> do_sleep start 8339 current 12400
> do_sleep start 8339 current 12500
> do_sleep start 8339 current 12600
> do_sleep start 8339 current 12700
> do_sleep start 8339 current 12800
> do_sleep start 8339 current 12900
> do_sleep start 8339 current 13000
> do_sleep start 8339 current 13100
>
> *** It was frozen right here. Control-C could not interrupt the loop,
> so my guess
> *** either it was stuck during the call to get_timer(). Or the current value
> *** overflowed and crashed the system.
>
> <END LOG>
>
> Finally, I removed the patch set, and rerun the sleep tests on this
> board. They all worked fine, I did not see anything strange.
>

Some ideas.

The get_timer() function looks wrong assigning an uint64_t to ulong.

lib/time.c

     static uint64_t notrace tick_to_time(uint64_t tick)
     uint64_t notrace get_ticks(void)
     uint64_t __weak notrace get_ticks(void)

     ulong __weak get_timer(ulong base)
     {
             return tick_to_time(get_ticks()) - base;
     }

Most of the timer infrastructure is using uint64_t. I'm seeing this
__weak function get_timer was invoked in Kirkwood boards. Both in
sleep and timer commands.

Thanks,
Tony

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan,

On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh <mibodhi@gmail.com> wrote:
>
> Hi Stefan,
>
> On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese <sr@denx.de> wrote:
> >
> > Hi Tony,
> >
> > On 31.08.22 08:30, Tony Dinh wrote:
> > > Hi Stefan,
> > >
> > > On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese <sr@denx.de> wrote:
> > >>
> > >> Hi Tony,
> > >>
> > >> On 31.08.22 07:02, Stefan Roese wrote:
> > >>> Hi Tony,
> > >>>
> > >>> On 31.08.22 00:15, Tony Dinh wrote:
> > >>>> Hi Stefan,
> > >>>>
> > >>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
> > >>>>>
> > >>>>> This patchset enhaces the recently added Orion Timer driver to support
> > >>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> > >>>>> timer support is then enabled per default for those platforms, so that
> > >>>>> the board config files don't need to be changed. Also necessary is
> > >>>>> some dts hacking, so that the timer DT node is available in early
> > >>>>> U-Boot stages.
> > >>>>>
> > >>>>> I've successfully tested this patchset on an Armada XP board. Additional
> > >>>>> test on other boards and platforms are very welcome and necessary.
> > >>>>
> > >>>> I've run some tests with the following 2 Kirkwood boards: Cloud
> > >>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> > >>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> > >>>>
> > >>>> It seems that it was either frozen or the timer did not expire at some
> > >>>> subsequent sleep commands. Sometime it happened at 2nd command, some
> > >>>> time at a later sleep command. For example,
> > >>>>
> > >>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> > >>>> stopwatch)
> > >>>>
> > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
> > >>>> Pogoplug V4
> > >>>>
> > >>>> Hit any key to stop autoboot:  0
> > >>>> Pogo_V4> sleep 10
> > >>>> Pogo_V4> sleep 31.5
> > >>>> <frozen here>
> > >>>
> > >>> Does the cmd interface support fractial numbers? Please test again with
> > >>> 31 or other integral numbers.
> > >>>
> > >>>> === Sheevaplug (RTC battery is old, so the date was not updated, but
> > >>>> the clock seems OK)
> > >>>>
> > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
> > >>>> Marvell-Sheevaplug
> > >>>> Hit any key to stop autoboot:  0
> > >>>> => date
> > >>>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> > >>>> => sleep 10
> > >>>> => date
> > >>>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> > >>>> => sleep 10
> > >>>> => sleep 20.1
> > >>>> <frozen here>
> > >>>>
> > >>>> Please let me know what I can do (i.e. perhaps running a debug patch).
> > >>>
> > >>> Please see above. I assume that the fractional numbers result in very
> > >>> long numbers internally, which result in a frozen / hanging system.
> > >>
> > >> I just tested fractional numbers on another board and hey, it just
> > >> works. Learned something new. So we seem to have a problem here. Let
> > >> me see, if I can find something.
> > >
> > > I've added debug printfs and possibly tracked down this issue. Seems
> > > like in the 2nd call to sleep, get_timer(0) did not reset the start
> > > number.
> > >
> > > cmd/sleep.c
> > > static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc,
> > >                      char *const argv[])
> > > {
> > >         ulong start = get_timer(0);
> > >
> > > "do_sleep got a timer start = 2015" is the 1st call to sleep 5.
> > > "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
> > >
> > > <BEGIN log>
> > > Pogo_V4> sleep 5
> > > do_sleep got a timer start = 2015
> > > do_sleep delay = 5000
> > > do_sleep delay = 5000
> > > do_sleep sleeping...
> > > do_sleep start 2015 curent 100
> > > do_sleep start 2015 curent 200
> > > do_sleep start 2015 curent 300
> > > <snip>
> > > do_sleep start 2015 curent 4900
> > > do_sleep end of sleep ... current = 5000
> > > Pogo_V4>
> > >
> > > Pogo_V4> sleep 10
> > > do_sleep got a timer start = 16304
> > > do_sleep delay = 10000
> > > do_sleep delay = 10000
> > > do_sleep sleeping...
> > > <snip>
> > >
> > > <END log>
> > >
> > > So somewhere in the DM timer, "start" got accumulated. I think each
> > > get_timer(0) should be a different timer instance. It looks like the
> > > same timer instance is used again and again, causing the "start "to
> > > grow bigger, and at one point it might just overflow.
> >
> > Frankly I don't really understand the problem you describe above. What
> > do you mean with "timer instance"? get_timer(0) will return different
> > values, depending on when you call this function. So where exactly is
> > the problem with the 2nd "sleep 10" above?
>
> Please ignore what I said above! I misunderstood what get_timer()
> does. I'll try again on another KIrkwood  board to see if the behavior
> will be the same.

I've  run the tests again with the Seagate GoFlex Home board (Kirkwood
88F6281). Below is the log, with my annotated comments starting with
****.
In these tests,  current = get_timer(start) is inside the while loop.
And I printed out the start and current values when (current % 100 ==
0).

<BEGIN LOG>
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700)
Seagate GoFlex Home

SoC:   Kirkwood 88F6281_A1
DRAM:  128 MiB
Core:  12 devices, 10 uclasses, devicetree: separate
NAND:  orion_timer_probe successful
256 MiB
Loading Environment from NAND... OK
In:    serial
Out:   serial
Err:   serial
Net:   eth0: ethernet-controller@72000
Hit any key to stop autoboot:  0

GoFlexHome> date
Date: 2022-08-31 (Wednesday)    Time: 20:43:03

GoFlexHome> sleep 5
do_sleep got a timer start = 1244
do_sleep delay = 5000
do_sleep delay = 5000
do_sleep sleeping...
do_sleep start 1244 current 100
<snip>
do_sleep start 1244 current 2300
<snip>
do_sleep start 1244 current 3700
<snip>
do_sleep start 1244 current 4800
do_sleep start 1244 current 4900
do_sleep end of sleep ... current = 5000

**** working as intended

GoFlexHome> sleep 10
do_sleep got a timer start = 11428
do_sleep delay = 10000
do_sleep delay = 10000
do_sleep sleeping...
do_sleep start 11428 current 100
<snip>
do_sleep start 11428 current 2300
<snip>
do_sleep start 11428 current 9900
do_sleep end of sleep ... current = 10000

**** working as intended

GoFlexHome> sleep 20.5
do_sleep got a timer start = 15031
do_sleep delay = 20000
do_sleep delay = 20500
do_sleep sleeping...
do_sleep start 15031 current 100
<snip>
do_sleep start 15031 current 6400
do_sleep end of sleep ... current = 4294952265

*** Something strange happened here. current should be 6500, but it
seems to have garbage. So the loop exits prematurely.

GoFlexHome> sleep 20.5
do_sleep got a timer start = 8339
do_sleep delay = 20000
do_sleep delay = 20500
do_sleep sleeping...
do_sleep start 8339 current 100
do_sleep start 8339 current 200
<snip>
do_sleep start 8339 current 12300
do_sleep start 8339 current 12400
do_sleep start 8339 current 12500
do_sleep start 8339 current 12600
do_sleep start 8339 current 12700
do_sleep start 8339 current 12800
do_sleep start 8339 current 12900
do_sleep start 8339 current 13000
do_sleep start 8339 current 13100

*** It was frozen right here. Control-C could not interrupt the loop,
so my guess
*** either it was stuck during the call to get_timer(). Or the current value
*** overflowed and crashed the system.

<END LOG>

Finally, I removed the patch set, and rerun the sleep tests on this
board. They all worked fine, I did not see anything strange.

Thanks,
Tony

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan,

On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese <sr@denx.de> wrote:
>
> Hi Tony,
>
> On 31.08.22 08:30, Tony Dinh wrote:
> > Hi Stefan,
> >
> > On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese <sr@denx.de> wrote:
> >>
> >> Hi Tony,
> >>
> >> On 31.08.22 07:02, Stefan Roese wrote:
> >>> Hi Tony,
> >>>
> >>> On 31.08.22 00:15, Tony Dinh wrote:
> >>>> Hi Stefan,
> >>>>
> >>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
> >>>>>
> >>>>> This patchset enhaces the recently added Orion Timer driver to support
> >>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> >>>>> timer support is then enabled per default for those platforms, so that
> >>>>> the board config files don't need to be changed. Also necessary is
> >>>>> some dts hacking, so that the timer DT node is available in early
> >>>>> U-Boot stages.
> >>>>>
> >>>>> I've successfully tested this patchset on an Armada XP board. Additional
> >>>>> test on other boards and platforms are very welcome and necessary.
> >>>>
> >>>> I've run some tests with the following 2 Kirkwood boards: Cloud
> >>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> >>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> >>>>
> >>>> It seems that it was either frozen or the timer did not expire at some
> >>>> subsequent sleep commands. Sometime it happened at 2nd command, some
> >>>> time at a later sleep command. For example,
> >>>>
> >>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> >>>> stopwatch)
> >>>>
> >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
> >>>> Pogoplug V4
> >>>>
> >>>> Hit any key to stop autoboot:  0
> >>>> Pogo_V4> sleep 10
> >>>> Pogo_V4> sleep 31.5
> >>>> <frozen here>
> >>>
> >>> Does the cmd interface support fractial numbers? Please test again with
> >>> 31 or other integral numbers.
> >>>
> >>>> === Sheevaplug (RTC battery is old, so the date was not updated, but
> >>>> the clock seems OK)
> >>>>
> >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
> >>>> Marvell-Sheevaplug
> >>>> Hit any key to stop autoboot:  0
> >>>> => date
> >>>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> >>>> => sleep 10
> >>>> => date
> >>>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> >>>> => sleep 10
> >>>> => sleep 20.1
> >>>> <frozen here>
> >>>>
> >>>> Please let me know what I can do (i.e. perhaps running a debug patch).
> >>>
> >>> Please see above. I assume that the fractional numbers result in very
> >>> long numbers internally, which result in a frozen / hanging system.
> >>
> >> I just tested fractional numbers on another board and hey, it just
> >> works. Learned something new. So we seem to have a problem here. Let
> >> me see, if I can find something.
> >
> > I've added debug printfs and possibly tracked down this issue. Seems
> > like in the 2nd call to sleep, get_timer(0) did not reset the start
> > number.
> >
> > cmd/sleep.c
> > static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc,
> >                      char *const argv[])
> > {
> >         ulong start = get_timer(0);
> >
> > "do_sleep got a timer start = 2015" is the 1st call to sleep 5.
> > "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
> >
> > <BEGIN log>
> > Pogo_V4> sleep 5
> > do_sleep got a timer start = 2015
> > do_sleep delay = 5000
> > do_sleep delay = 5000
> > do_sleep sleeping...
> > do_sleep start 2015 curent 100
> > do_sleep start 2015 curent 200
> > do_sleep start 2015 curent 300
> > <snip>
> > do_sleep start 2015 curent 4900
> > do_sleep end of sleep ... current = 5000
> > Pogo_V4>
> >
> > Pogo_V4> sleep 10
> > do_sleep got a timer start = 16304
> > do_sleep delay = 10000
> > do_sleep delay = 10000
> > do_sleep sleeping...
> > <snip>
> >
> > <END log>
> >
> > So somewhere in the DM timer, "start" got accumulated. I think each
> > get_timer(0) should be a different timer instance. It looks like the
> > same timer instance is used again and again, causing the "start "to
> > grow bigger, and at one point it might just overflow.
>
> Frankly I don't really understand the problem you describe above. What
> do you mean with "timer instance"? get_timer(0) will return different
> values, depending on when you call this function. So where exactly is
> the problem with the 2nd "sleep 10" above?

Please ignore what I said above! I misunderstood what get_timer()
does. I'll try again on another KIrkwood  board to see if the behavior
will be the same.

Thanks,
Tony


> Thanks,
> Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

Hi Tony,

On 31.08.22 08:30, Tony Dinh wrote:
> Hi Stefan,
> 
> On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Tony,
>>
>> On 31.08.22 07:02, Stefan Roese wrote:
>>> Hi Tony,
>>>
>>> On 31.08.22 00:15, Tony Dinh wrote:
>>>> Hi Stefan,
>>>>
>>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> This patchset enhaces the recently added Orion Timer driver to support
>>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
>>>>> timer support is then enabled per default for those platforms, so that
>>>>> the board config files don't need to be changed. Also necessary is
>>>>> some dts hacking, so that the timer DT node is available in early
>>>>> U-Boot stages.
>>>>>
>>>>> I've successfully tested this patchset on an Armada XP board. Additional
>>>>> test on other boards and platforms are very welcome and necessary.
>>>>
>>>> I've run some tests with the following 2 Kirkwood boards: Cloud
>>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
>>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
>>>>
>>>> It seems that it was either frozen or the timer did not expire at some
>>>> subsequent sleep commands. Sometime it happened at 2nd command, some
>>>> time at a later sleep command. For example,
>>>>
>>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
>>>> stopwatch)
>>>>
>>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
>>>> Pogoplug V4
>>>>
>>>> Hit any key to stop autoboot:  0
>>>> Pogo_V4> sleep 10
>>>> Pogo_V4> sleep 31.5
>>>> <frozen here>
>>>
>>> Does the cmd interface support fractial numbers? Please test again with
>>> 31 or other integral numbers.
>>>
>>>> === Sheevaplug (RTC battery is old, so the date was not updated, but
>>>> the clock seems OK)
>>>>
>>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
>>>> Marvell-Sheevaplug
>>>> Hit any key to stop autoboot:  0
>>>> => date
>>>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
>>>> => sleep 10
>>>> => date
>>>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
>>>> => sleep 10
>>>> => sleep 20.1
>>>> <frozen here>
>>>>
>>>> Please let me know what I can do (i.e. perhaps running a debug patch).
>>>
>>> Please see above. I assume that the fractional numbers result in very
>>> long numbers internally, which result in a frozen / hanging system.
>>
>> I just tested fractional numbers on another board and hey, it just
>> works. Learned something new. So we seem to have a problem here. Let
>> me see, if I can find something.
> 
> I've added debug printfs and possibly tracked down this issue. Seems
> like in the 2nd call to sleep, get_timer(0) did not reset the start
> number.
> 
> cmd/sleep.c
> static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc,
>                      char *const argv[])
> {
>         ulong start = get_timer(0);
> 
> "do_sleep got a timer start = 2015" is the 1st call to sleep 5.
> "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
> 
> <BEGIN log>
> Pogo_V4> sleep 5
> do_sleep got a timer start = 2015
> do_sleep delay = 5000
> do_sleep delay = 5000
> do_sleep sleeping...
> do_sleep start 2015 curent 100
> do_sleep start 2015 curent 200
> do_sleep start 2015 curent 300
> <snip>
> do_sleep start 2015 curent 4900
> do_sleep end of sleep ... current = 5000
> Pogo_V4>
> 
> Pogo_V4> sleep 10
> do_sleep got a timer start = 16304
> do_sleep delay = 10000
> do_sleep delay = 10000
> do_sleep sleeping...
> <snip>
> 
> <END log>
> 
> So somewhere in the DM timer, "start" got accumulated. I think each
> get_timer(0) should be a different timer instance. It looks like the
> same timer instance is used again and again, causing the "start "to
> grow bigger, and at one point it might just overflow.

Frankly I don't really understand the problem you describe above. What
do you mean with "timer instance"? get_timer(0) will return different
values, depending on when you call this function. So where exactly is
the problem with the 2nd "sleep 10" above?

Thanks,
Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan,

On Tue, Aug 30, 2022 at 11:19 PM Stefan Roese <sr@denx.de> wrote:
>
> Hi Tony,
>
> On 31.08.22 07:08, Stefan Roese wrote:
> > Hi Tony,
> >
> > On 31.08.22 07:02, Stefan Roese wrote:
> >> Hi Tony,
> >>
> >> On 31.08.22 00:15, Tony Dinh wrote:
> >>> Hi Stefan,
> >>>
> >>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
> >>>>
> >>>> This patchset enhaces the recently added Orion Timer driver to support
> >>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> >>>> timer support is then enabled per default for those platforms, so that
> >>>> the board config files don't need to be changed. Also necessary is
> >>>> some dts hacking, so that the timer DT node is available in early
> >>>> U-Boot stages.
> >>>>
> >>>> I've successfully tested this patchset on an Armada XP board.
> >>>> Additional
> >>>> test on other boards and platforms are very welcome and necessary.
> >>>
> >>> I've run some tests with the following 2 Kirkwood boards: Cloud
> >>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> >>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> >>>
> >>> It seems that it was either frozen or the timer did not expire at some
> >>> subsequent sleep commands. Sometime it happened at 2nd command, some
> >>> time at a later sleep command. For example,
> >>>
> >>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> >>> stopwatch)
> >>>
> >>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24
> >>> -0700)
> >>> Pogoplug V4
> >>>
> >>> Hit any key to stop autoboot:  0
> >>> Pogo_V4> sleep 10
> >>> Pogo_V4> sleep 31.5
> >>> <frozen here>
> >>
> >> Does the cmd interface support fractial numbers? Please test again with
> >> 31 or other integral numbers.
> >>
> >>> === Sheevaplug (RTC battery is old, so the date was not updated, but
> >>> the clock seems OK)
> >>>
> >>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24
> >>> -0700)
> >>> Marvell-Sheevaplug
> >>> Hit any key to stop autoboot:  0
> >>> => date
> >>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> >>> => sleep 10
> >>> => date
> >>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> >>> => sleep 10
> >>> => sleep 20.1
> >>> <frozen here>
> >>>
> >>> Please let me know what I can do (i.e. perhaps running a debug patch).
> >>
> >> Please see above. I assume that the fractional numbers result in very
> >> long numbers internally, which result in a frozen / hanging system.
> >
> > I just tested fractional numbers on another board and hey, it just
> > works. Learned something new. So we seem to have a problem here. Let
> > me see, if I can find something.
>
> I can't reproduce this problem on my Armada XP platform. When your
> system is frozen, can you interrupt the sleep cmd via Ctrl-C? Can you
> please also test without this patchset applied, if a series of sleep
> commands works fine there?

Indeed, it works without the patchset. I ran an older version (U-Boot
2021.10) and entered some random sleep periods such as

Pogo_V4> sleep 5
Pogo_V4> sleep 20.5
Pogo_V4> sleep 35
Pogo_V4> sleep 15.3
Pogo_V4> sleep 33.33
Pogo_V4> sleep 77.23

And it is also true that I could not do Ctrl-C when it is frozen
running with the new timer.

Thanks,
Tony

> Thanks,
> Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan,

On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese <sr@denx.de> wrote:
>
> Hi Tony,
>
> On 31.08.22 07:02, Stefan Roese wrote:
> > Hi Tony,
> >
> > On 31.08.22 00:15, Tony Dinh wrote:
> >> Hi Stefan,
> >>
> >> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
> >>>
> >>> This patchset enhaces the recently added Orion Timer driver to support
> >>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> >>> timer support is then enabled per default for those platforms, so that
> >>> the board config files don't need to be changed. Also necessary is
> >>> some dts hacking, so that the timer DT node is available in early
> >>> U-Boot stages.
> >>>
> >>> I've successfully tested this patchset on an Armada XP board. Additional
> >>> test on other boards and platforms are very welcome and necessary.
> >>
> >> I've run some tests with the following 2 Kirkwood boards: Cloud
> >> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> >> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> >>
> >> It seems that it was either frozen or the timer did not expire at some
> >> subsequent sleep commands. Sometime it happened at 2nd command, some
> >> time at a later sleep command. For example,
> >>
> >> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> >> stopwatch)
> >>
> >> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
> >> Pogoplug V4
> >>
> >> Hit any key to stop autoboot:  0
> >> Pogo_V4> sleep 10
> >> Pogo_V4> sleep 31.5
> >> <frozen here>
> >
> > Does the cmd interface support fractial numbers? Please test again with
> > 31 or other integral numbers.
> >
> >> === Sheevaplug (RTC battery is old, so the date was not updated, but
> >> the clock seems OK)
> >>
> >> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
> >> Marvell-Sheevaplug
> >> Hit any key to stop autoboot:  0
> >> => date
> >> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> >> => sleep 10
> >> => date
> >> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> >> => sleep 10
> >> => sleep 20.1
> >> <frozen here>
> >>
> >> Please let me know what I can do (i.e. perhaps running a debug patch).
> >
> > Please see above. I assume that the fractional numbers result in very
> > long numbers internally, which result in a frozen / hanging system.
>
> I just tested fractional numbers on another board and hey, it just
> works. Learned something new. So we seem to have a problem here. Let
> me see, if I can find something.

I've added debug printfs and possibly tracked down this issue. Seems
like in the 2nd call to sleep, get_timer(0) did not reset the start
number.

cmd/sleep.c
static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc,
                    char *const argv[])
{
       ulong start = get_timer(0);

"do_sleep got a timer start = 2015" is the 1st call to sleep 5.
"do_sleep got a timer start = 16304" is the 2nd call to sleep 10.

<BEGIN log>
Pogo_V4> sleep 5
do_sleep got a timer start = 2015
do_sleep delay = 5000
do_sleep delay = 5000
do_sleep sleeping...
do_sleep start 2015 curent 100
do_sleep start 2015 curent 200
do_sleep start 2015 curent 300
<snip>
do_sleep start 2015 curent 4900
do_sleep end of sleep ... current = 5000
Pogo_V4>

Pogo_V4> sleep 10
do_sleep got a timer start = 16304
do_sleep delay = 10000
do_sleep delay = 10000
do_sleep sleeping...
<snip>

<END log>

So somewhere in the DM timer, "start" got accumulated. I think each
get_timer(0) should be a different timer instance. It looks like the
same timer instance is used again and again, causing the "start "to
grow bigger, and at one point it might just overflow.

Thanks,
Tony

> Thanks,
> Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

Hi Tony,

On 31.08.22 07:08, Stefan Roese wrote:
> Hi Tony,
> 
> On 31.08.22 07:02, Stefan Roese wrote:
>> Hi Tony,
>>
>> On 31.08.22 00:15, Tony Dinh wrote:
>>> Hi Stefan,
>>>
>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> This patchset enhaces the recently added Orion Timer driver to support
>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
>>>> timer support is then enabled per default for those platforms, so that
>>>> the board config files don't need to be changed. Also necessary is
>>>> some dts hacking, so that the timer DT node is available in early
>>>> U-Boot stages.
>>>>
>>>> I've successfully tested this patchset on an Armada XP board. 
>>>> Additional
>>>> test on other boards and platforms are very welcome and necessary.
>>>
>>> I've run some tests with the following 2 Kirkwood boards: Cloud
>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
>>>
>>> It seems that it was either frozen or the timer did not expire at some
>>> subsequent sleep commands. Sometime it happened at 2nd command, some
>>> time at a later sleep command. For example,
>>>
>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
>>> stopwatch)
>>>
>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 
>>> -0700)
>>> Pogoplug V4
>>>
>>> Hit any key to stop autoboot:  0
>>> Pogo_V4> sleep 10
>>> Pogo_V4> sleep 31.5
>>> <frozen here>
>>
>> Does the cmd interface support fractial numbers? Please test again with
>> 31 or other integral numbers.
>>
>>> === Sheevaplug (RTC battery is old, so the date was not updated, but
>>> the clock seems OK)
>>>
>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 
>>> -0700)
>>> Marvell-Sheevaplug
>>> Hit any key to stop autoboot:  0
>>> => date
>>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
>>> => sleep 10
>>> => date
>>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
>>> => sleep 10
>>> => sleep 20.1
>>> <frozen here>
>>>
>>> Please let me know what I can do (i.e. perhaps running a debug patch).
>>
>> Please see above. I assume that the fractional numbers result in very
>> long numbers internally, which result in a frozen / hanging system.
> 
> I just tested fractional numbers on another board and hey, it just
> works. Learned something new. So we seem to have a problem here. Let
> me see, if I can find something.

I can't reproduce this problem on my Armada XP platform. When your
system is frozen, can you interrupt the sleep cmd via Ctrl-C? Can you
please also test without this patchset applied, if a series of sleep
commands works fine there?

Thanks,
Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

Hi Tony,

On 31.08.22 07:02, Stefan Roese wrote:
> Hi Tony,
> 
> On 31.08.22 00:15, Tony Dinh wrote:
>> Hi Stefan,
>>
>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
>>>
>>> This patchset enhaces the recently added Orion Timer driver to support
>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
>>> timer support is then enabled per default for those platforms, so that
>>> the board config files don't need to be changed. Also necessary is
>>> some dts hacking, so that the timer DT node is available in early
>>> U-Boot stages.
>>>
>>> I've successfully tested this patchset on an Armada XP board. Additional
>>> test on other boards and platforms are very welcome and necessary.
>>
>> I've run some tests with the following 2 Kirkwood boards: Cloud
>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
>>
>> It seems that it was either frozen or the timer did not expire at some
>> subsequent sleep commands. Sometime it happened at 2nd command, some
>> time at a later sleep command. For example,
>>
>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
>> stopwatch)
>>
>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
>> Pogoplug V4
>>
>> Hit any key to stop autoboot:  0
>> Pogo_V4> sleep 10
>> Pogo_V4> sleep 31.5
>> <frozen here>
> 
> Does the cmd interface support fractial numbers? Please test again with
> 31 or other integral numbers.
> 
>> === Sheevaplug (RTC battery is old, so the date was not updated, but
>> the clock seems OK)
>>
>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
>> Marvell-Sheevaplug
>> Hit any key to stop autoboot:  0
>> => date
>> Date: 2000-01-01 (Saturday)    Time:  0:02:55
>> => sleep 10
>> => date
>> Date: 2000-01-01 (Saturday)    Time:  0:03:18
>> => sleep 10
>> => sleep 20.1
>> <frozen here>
>>
>> Please let me know what I can do (i.e. perhaps running a debug patch).
> 
> Please see above. I assume that the fractional numbers result in very
> long numbers internally, which result in a frozen / hanging system.

I just tested fractional numbers on another board and hey, it just
works. Learned something new. So we seem to have a problem here. Let
me see, if I can find something.

Thanks,
Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

Hi Tony,

On 31.08.22 00:15, Tony Dinh wrote:
> Hi Stefan,
> 
> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
>>
>> This patchset enhaces the recently added Orion Timer driver to support
>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
>> timer support is then enabled per default for those platforms, so that
>> the board config files don't need to be changed. Also necessary is
>> some dts hacking, so that the timer DT node is available in early
>> U-Boot stages.
>>
>> I've successfully tested this patchset on an Armada XP board. Additional
>> test on other boards and platforms are very welcome and necessary.
> 
> I've run some tests with the following 2 Kirkwood boards: Cloud
> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
> 
> It seems that it was either frozen or the timer did not expire at some
> subsequent sleep commands. Sometime it happened at 2nd command, some
> time at a later sleep command. For example,
> 
> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
> stopwatch)
> 
> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
> Pogoplug V4
> 
> Hit any key to stop autoboot:  0
> Pogo_V4> sleep 10
> Pogo_V4> sleep 31.5
> <frozen here>

Does the cmd interface support fractial numbers? Please test again with
31 or other integral numbers.

> === Sheevaplug (RTC battery is old, so the date was not updated, but
> the clock seems OK)
> 
> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
> Marvell-Sheevaplug
> Hit any key to stop autoboot:  0
> => date
> Date: 2000-01-01 (Saturday)    Time:  0:02:55
> => sleep 10
> => date
> Date: 2000-01-01 (Saturday)    Time:  0:03:18
> => sleep 10
> => sleep 20.1
> <frozen here>
> 
> Please let me know what I can do (i.e. perhaps running a debug patch).

Please see above. I assume that the fractional numbers result in very
long numbers internally, which result in a frozen / hanging system.

Thanks,
Stefan

> Thanks,
> Tony
> 
>> Thanks,
>> Stefan
>>
>> Stefan Roese (6):
>>    timer: orion-timer: Add support for other Armada SoC's
>>    timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support
>>    arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms
>>    arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step
>>    arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1
>>    arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot,dm-pre-reloc" to timer
>>      DT node
>>
>>   arch/arm/Kconfig                          |  4 ++
>>   arch/arm/dts/Makefile                     |  6 ++-
>>   arch/arm/dts/armada-375.dtsi              |  4 +-
>>   arch/arm/dts/mvebu-u-boot.dtsi            | 11 ++++
>>   arch/arm/mach-mvebu/include/mach/config.h |  5 --
>>   drivers/timer/Kconfig                     |  5 +-
>>   drivers/timer/orion-timer.c               | 66 +++++++++++++++++++++--
>>   7 files changed, 89 insertions(+), 12 deletions(-)
>>
>> --
>> 2.37.2
>>

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

Hi Stefan,

On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese <sr@denx.de> wrote:
>
> This patchset enhaces the recently added Orion Timer driver to support
> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
> timer support is then enabled per default for those platforms, so that
> the board config files don't need to be changed. Also necessary is
> some dts hacking, so that the timer DT node is available in early
> U-Boot stages.
>
> I've successfully tested this patchset on an Armada XP board. Additional
> test on other boards and platforms are very welcome and necessary.

I've run some tests with the following 2 Kirkwood boards: Cloud
Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION),
and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).

It seems that it was either frozen or the timer did not expire at some
subsequent sleep commands. Sometime it happened at 2nd command, some
time at a later sleep command. For example,

=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my
stopwatch)

U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700)
Pogoplug V4

Hit any key to stop autoboot:  0
Pogo_V4> sleep 10
Pogo_V4> sleep 31.5
<frozen here>

=== Sheevaplug (RTC battery is old, so the date was not updated, but
the clock seems OK)

U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700)
Marvell-Sheevaplug
Hit any key to stop autoboot:  0
=> date
Date: 2000-01-01 (Saturday)    Time:  0:02:55
=> sleep 10
=> date
Date: 2000-01-01 (Saturday)    Time:  0:03:18
=> sleep 10
=> sleep 20.1
<frozen here>

Please let me know what I can do (i.e. perhaps running a debug patch).
Thanks,
Tony

> Thanks,
> Stefan
>
> Stefan Roese (6):
>   timer: orion-timer: Add support for other Armada SoC's
>   timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support
>   arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms
>   arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step
>   arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1
>   arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot,dm-pre-reloc" to timer
>     DT node
>
>  arch/arm/Kconfig                          |  4 ++
>  arch/arm/dts/Makefile                     |  6 ++-
>  arch/arm/dts/armada-375.dtsi              |  4 +-
>  arch/arm/dts/mvebu-u-boot.dtsi            | 11 ++++
>  arch/arm/mach-mvebu/include/mach/config.h |  5 --
>  drivers/timer/Kconfig                     |  5 +-
>  drivers/timer/orion-timer.c               | 66 +++++++++++++++++++++--
>  7 files changed, 89 insertions(+), 12 deletions(-)
>
> --
> 2.37.2
>

[PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

This patchset enhaces the recently added Orion Timer driver to support
all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this
timer support is then enabled per default for those platforms, so that
the board config files don't need to be changed. Also necessary is
some dts hacking, so that the timer DT node is available in early
U-Boot stages.

I've successfully tested this patchset on an Armada XP board. Additional
test on other boards and platforms are very welcome and necessary.

Thanks,
Stefan

Stefan Roese (6):
  timer: orion-timer: Add support for other Armada SoC's
  timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support
  arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms
  arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step
  arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1
  arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot,dm-pre-reloc" to timer
    DT node

 arch/arm/Kconfig                          |  4 ++
 arch/arm/dts/Makefile                     |  6 ++-
 arch/arm/dts/armada-375.dtsi              |  4 +-
 arch/arm/dts/mvebu-u-boot.dtsi            | 11 ++++
 arch/arm/mach-mvebu/include/mach/config.h |  5 --
 drivers/timer/Kconfig                     |  5 +-
 drivers/timer/orion-timer.c               | 66 +++++++++++++++++++++--
 7 files changed, 89 insertions(+), 12 deletions(-)

-- 
2.37.2