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

[Stefan Roese <sr@denx.de>]

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