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

Simon Glass
2022-09-01T14:35:13Z

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