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