[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