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