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

[Simon Glass]

Hi Graeme,

On Wed, Jun 15, 2011 at 4:09 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
[snip]
>>
>> BTW should the deltas return a signed value?
>
> No - times are unsigned utilising the entire range of the u32 so (to -
> from) will always returned the unsigned delta, even if there is a wrap
> between them. I cannot imagine we would ever need to measure time backward
> anyway.

Can you please explain this again in different words as I don't
follow. The difference between two unsigned times may be +ve or -ve,
surely.  For example, say:

now = 100
event_time = 200

then the delta is currently -100. Some people might loop until the
delta is >= 0. This is like setting a future time and waiting for it
to happen. If the delta is unsigned then this won't work.

>> No I mean _ms at the end, so it is always last. time_min_ms_since()
>> sounds like a martian trying to learn English. See my comment above:
>
> I read it like: Time API - Minimum Milliseconds Since (Epoch). Last time I
> checked I wasn't a martian ;)

I see. I wonder how you checked? Skin color and height I suppose.

>
> time_min_since_ms reads like: Time API - Minimum Since Millisecond (Epoch)
>
> Anyway, time_since_ms() is fine by me
>
>> time_since_ms() - we shouldn't even define time_since_raw_ms() and
>> time_since_max_ms() in my view.
>
> I agree that the raw versions should be dropped entirely. The max versions
> should be kept (and will be used for boot profiling as you are after the
> maximum time an operation could possibly have run)

OK I wasn't suggesting dropping raw, just hiding it. My main point was
just that the common one should have the obvious name. Anyway, I
suppose raw is not that useful.

>
>>>
>>>> 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.
>>>
>>> I tend to agree - I have been thinking about 'since' functions for a while now
>>
>> Cool.
>>
>>>
>>> [snip]
>>>
>>>>>
>>>>> 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.
>>>
>>> An example from drivers/net/ns7520_eth.c:
>>>
>>>        ulStartJiffies = time_ms_now();
>>>        while (time_ms_delta_min(ulStartJiffies, time_ms_now())
>>>                        < NS7520_MII_NEG_DELAY) {
>>>
>>> Could be reduced to:
>>>
>>>        ulStartJiffies = time_ms_now();
>>>        while (time_min_ms_since(ulStartJiffies) < NS7520_MII_NEG_DELAY) {
>>>
>>> And with a rename:
>>>
>>>        start_ms = time_ms_now();
>>>        while (time_min_ms_since(start_ms) < NS7520_MII_NEG_DELAY) {
>>>
>>
>> or:
>>
>>         start_ms = time_now_ms();
>>         while (time_since_ms(start_ms) < NS7520_MII_NEG_DELAY) {
>>
>
> OK, So the API could now look like:
>
> /* Functions to retrieve the current value of the timers */
> u32 time_now_ms(void);
> u32 time_now_us(void);
> u64 time_now_ticks(void);
>
> /* Functions to determine 'minimum' time elapsed (the usual case) */
> u32 time_since_ms(u32 start);
> u32 time_since_us(u32 start);
> u64 time_since_us(u64 start);
time_since_ticks()
>
> /* Functions to determine 'maximum' time elapsed (performance profiling) */
> u32 time_max_since_ms(u32 start);
> u32 time_max_since_us(u32 start);
> u64 time_max_since_ticks(u64 start);
>
> /* Default udelay() */
> void udelay(u32 delay)
> {
>        u32 start = time_now_us();
>
>        while(time_since_us(start) < delay)
>                ;
> }

Well it has my vote - does it solve your 80-column problems? Some
might complain about the proliferation of 'since' functions. I don't
have a strong view on this - but it is perhaps preferable to having
everyone do things manually. The strongly orthogonal nature of your
functions helps with understanding anyway I think.

One interesting aspect to me is whether it might be possible for the
'since' functions to have a debug mode which prints out a message when
the boot appears to be hung. Sometimes U-Boot hangs on USB or similar
and there is no message displayed. I imagine it might be possible to
detect a hung or repeating timeout and print a stack trace. Another
thing is how much time was spent hanging around during a boot (as a
summary at the end).  Just a thought, and NOT suggesting doing
anything about it now.

Regards,
Simon

>
> Regards,
>
> Graeme
>