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

[Graeme Russ]

Hi Simon,

On Thu, Jun 16, 2011 at 7:58 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Graeme,
>
> On Wed, Jun 15, 2011 at 1:38 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
>> On 16/06/11 02:03, Simon Glass wrote:
>
>>> - 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
>>
>> I don't like this idea - Extrapolate this to dropping the 'ms' common case
>> and you end up with:
>>
>> u32 time_delta(u32 from, u32 to)
>> u32 time_delta_max(u32 from, u32 to)
>> u32 time_delta_raw(u32 from, u32 to)
>>
>> u32 time_us_delta(u32 from, u32 to)
>> u32 time_us_delta_max(u32 from, u32 to)
>> u32 time_us_delta_raw(u32 from, u32 to)
>>
>> Not as grep'able IMHO
>
> The reason I suggested this is not because it is common, in that ms is
> more common than us. It is that users will find time_ms_delta_min
> confusing. There is a min, max and raw - which should they use? While
> everyone on this list is a genius there will be other people who will
> choose one at random and run with it.
>
> To my mind 'min' is the safe one and the others will be rarely used.
>
> I agree that removing ms is a bad idea. But this is pretty grep'able
> for sensible values of grep:
>
> u32 time_delta_ms(u32 from, u32 to)
> u32 time_delta_us(u32 from, u32 to)
>
> [hide these away in the closet:
> u32 time_delta_max_ms(u32 from, u32 to)
> u32 time_delta_raw_ms(u32 from, u32 to)
> u32 time_delta_max_us(u32 from, u32 to)
> u32 time_delta_raw_us(u32 from, u32 to)
> ]
>
> int time_since_ms(u32 from)
>
> Also bear in mind that _us() will be seldom used. I will cheerfully
> use it for boot timing though.
>
> 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.

>>> - 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
>>
>> Hmm, time_since_ms or time_ms_since - I suppose either reads OK - But if I
>> keep min/max/raw, I find time_min_ms_since (barely) easier in the eye than
>> time_min_since_ms - I'm not bothered either way and will go with the
>> general consensus
>
> 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 ;)

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)

>>
>>> 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);

/* 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)
		;
}

Regards,

Graeme