[U-Boot] [RFC] ARM timing code refactoring

[Albert ARIBAUD]

Hi Wolfgang,

Le 22/01/2011 20:19, Wolfgang Denk a écrit :

> Dear Albert ARIBAUD,

>> Agreed for unnecessary mult-div, but 64-bit we would not avoid, and
>> should not IMO, when the HW has it.
>
> When attempting to come up with true generic code, we should probably
> _always_ use a (virtual) unsigned 64 bit counter.

That's fine with me. We'll just have to keep in mind that with a 32-bit 
HW counter, the upper 32 bits of the 64-bit virtual counter must be 
managed by SW -- which should not prove to be too much of a problem, but 
will force us to handle HW-to-SW rollovers in get_timer() on each 
virtual timer reads.

>>> If the tick were really high speed (and then 64 bits), fast_tick
>>> could be derived by shifting the tick some bits to the right.
>>
>> The only thing I slightly dislike about the overall idea is the signed
>> rather than unsigned comparison in the timeout function (I prefer using
>> the full 32-bit range, even if only as an academic point) and the fact
>> that the value of the timeout is encoded in advance in the loop control
>> variable 'timeout'.
>
> Please don't.  Use an unsigned counter and allow it to roll over.
> Anything else causes additional (and unnecessary) restrictions and
> makes the code more complicated.

Hmm... don't we actually agree here? I was also expressing my preference 
for unsigned.

>> I'd rather have a single 'time(x)' (or 'ticks_elapsed(x)', names are
>> negotiable) macro which subtract its argument from the current ticks,
>
> I'm not sure what you have in mind with "substract".  I strongly
> recommend to avoid problems with wrap-around etc. right from the
> beginning, and let unsigned arithmentcs handle this for us.

That's what I meant with my comment about subtracting: the difference of 
two unsigned integers is always correct.

>> e.g. 'then = time(0)' would set 'then' to the number of ticks elapsed
>> from boot, while 'now = time(then)' would set 'now' the ticks elapsed
>> from 'then'; and a 'ms_to_ticks(x)' (again, or 'milliseconds(x)') :
>
> Do we really need such a function?  As far as I can tell we don't
> really have any time (in the sense of wallclock time) related
> requirements, we only need delta times, and nobody really cares about
> the starting point.

Hmm... My idea with providing time() with an argument was that precisely 
since we are interested only in elapsed time, not absolute time, our 
basic time function should be able to tell us relative times.

> We should _always_ be able to use the standard approach of
>
> 	start = get_timer()
>
> 	while ((get_timer() - start)<  timeout) {
> suall		... wait ...
> 	}

Functionally this is the same as what I suggested, minus the absolute 
get_timer() vs relative time(x) call. Either way works, I'm not going to 
try and make a point that we use "my" time(x).

We still need ms-to-tick and us-to-tick conversions, though, because 
usually timing constraints will be expressed in us or ms, not in ticks.

Just one thing, however:

>> Your example would then become
>>
>> 	then = time(0);
>> 	do {...} while ( time(then)<  ms_to_ticks(100) );
>
> We should NOT do this.  It is bound to break as soon as the code
> in the loop (the "..." part) needs to implement a timeout, too.

I'm not sure to understand why. Can you develop how you think this would 
break with an inner timeout?

>> 	#define now() time(0)
>>    	#define ms_elapsed(then,ms) ( time(then)<  ms_to_ticks(ms) )
>
> This is not a good isea for the same reason.

Those two were just syntactic sugaring anyway; if you don't like them, 
no worries.

> Best regards,
>
> Wolfgang Denk

Amicalement,
-- 
Albert.