[U-Boot] [RFC][Timer API] Revised Specification - Implementation details

Simon Glass
2011-06-15T16:03:23Z

Hi Graeme,

On Wed, Jun 15, 2011 at 6:17 AM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Wolfgang,
...
>>
>> ? ? ? /*
>> ? ? ? ?* round - used to control rounding:
>> ? ? ? ?* <0 : round down, return time that has passed AT LEAST
>> ? ? ? ?* =0 : don't round, provide raw time difference
>> ? ? ? ?* >0 : round up, return time that has passed AT MOST
>> ? ? ? ?*/
>> ? ? ? ?u32 delta_timer(u32 from, u32 to, int round)
>> ? ? ? ?{
>
> [snip]
>
>> ? ? ? }
>
> I decided to implement three separate functions:
>
> u32 time_ms_delta_min(u32 from, u32 to)
> u32 time_ms_delta_max(u32 from, u32 to)
> u32 time_ms_delta_raw(u32 from, u32 to)
>
> So if you only use one, the rest get stripped out of the binary

Here is m 2p worth:

- the common case is min I think, so let's get rid of the min prefix
so everyone will use it without question or needing to read screeds of
doc

- would prefer the ms and us at the end as I think it reads better.
Getting the time is the important bit - the units are generally at the
end

This code from your excellent wiki page bothers me. Can we find a way
to shrink it?

now = timer_ms_now();
if (time_ms_delta_min(start, now)> timeout)

How about:

if (time_since_ms(start) > timeout)

The idea of the time 'since' an event is more natural than the delta
between then and now which seems more abstract.

>
> The ms_ part allows for:
>
> u32 time_us_delta_min(u32 from, u32 to)
> u32 time_us_delta_max(u32 from, u32 to)
> u32 time_us_delta_raw(u32 from, u32 to)
>
> I intend to let the time_us_delta* functions drop to ms resolution of the
> underlying tick counter is not sub-millisecond. Where the tick counter is
> microsecond (or better), then arch-specific udelay becomes a trivial
> implementation of a loop using time_us_now() and time_us_delta_min() -
> Actually, we can make this a weak default and have arches with a non
> microsecond tick counter override it.
>
>> So our timeout from case 1) above would now be written like this:
>>
>> ? ? ? u32 start,now;
>> ? ? ? u32 timeout = 5 * CONFIG_SYS_HZ; /* timeout after 5 seconds */
>>
>> ? ? ? start = get_timer(0);
>
> I now have:
> ? ? ? ?start = time_ms_now();
>
>>
>> ? ? ? while (test_for_event() == 0) {
>> ? ? ? ? ? ? ? now = get_timer(0);
>>
>> ? ? ? ? ? ? ? if (delta_timer(start, now) > timeout)
>> ? ? ? ? ? ? ? ? ? ? ? handle_timeout();
>>
>> ? ? ? ? ? ? ? udelay(100);
>> ? ? ? }
>>
>> and would be guaranteed never to terminate early.
>>
>>
>>
>> Comments?
>
> With the 'time_ms_' prefix, it's starting to get rather long, and I'm
> pushing a lot of timeout checks beyond 80 columns - especially when
> existing code has variables like 'start_time_tx' - I'm starting to consider
> dropping the time_ prefix (all time functions will still have a ms_ or us_
> prefix anyway) and rename a lot of variables

Now I see why you want units at the start. Seems a bit ugly to me -
which lines are getting long - do you mean the time delta check line?
If so see above, or perhaps it is shorter without _min.

Regards,
Simon

>
> Thoughts?
>
> Regards,
>
> Graeme
>
>