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

[Albert ARIBAUD]

Le 22/01/2011 11:42, Reinhard Meyer a écrit :
> Dear Albert ARIBAUD,
>> Hi All,
>>
>> I am starting this thread to revive and, hopefully, come to a general
>> agreement on how timers should be implemented and used in the ARM
>> architecture, and get rid of current quick fixes. Let us start with
>> Reinhard's proposal:
>>
>>> There were several suggestions about that in the past (including from
>>> me) that involve rework everywhere HZ related timeouts are used. I
>>> still prefer a method as follows (because it does not need repeated
>>> mul/div calculations nor necessarily 64 bit arithmetic):
>>
>> Agreed for unnecessary mult-div, but 64-bit we would not avoid, and
>> should not IMO, when the HW has it.
>>
>>> u32 timeout = timeout_init(100); /* 100ms timeout */
>>>
>>> do {...} while (!timed_out(timeout));
>>>
>>> Internally it would be like:
>>>
>>> timeout_init(x):
>>> return fast_tick + (x * fast_tick_rate) / CONFIG_SYS_HZ;
>>> /* this might need 64 bit precision in some implementations */
>>>
>>> time_out(x):
>>> return ((i32)(x - fast_tick))< 0;
>>>
>>> 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'.
>
> 1. you always need signed compares there, unless you never anticipate a
> rollover of your timer value to zero.

I don't think rollover to zero is an issue if the tick counter is 
free-running on 32 (or 64 bits); and if it is free-running on some other 
number of bits, the time() macro just needs anding to work.

> 2. whats the problem with initializing the timeout value at the beginning?

There isn't a problem as such; simply, as the timeout is not 
conceptually needed at the beginning of the timing loop, I would favor 
implementations which do not need it at the beginning either.

>> I'd rather have a single 'time(x)' (or 'ticks_elapsed(x)', names are
>> negotiable) macro which subtract its argument from the current ticks,
>> 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)') :
>>
>> #define time(x) (ticks - x)
>> #define ms_to_ticks(m) ( (m * fast_tick_rate) / CONFIG_SYS_HZ)
>
> We have exactly an equivalent of this in use at AT91.
> It works only as long as the ticks value does not roll back to zero -
> which in the current 64 bit implementation I did is after the end of
> the universe...

Let us take a case where the tick value rolls over 0. Say, ticks is 
equal to 2^32 - 100, and we should delay for 200 ticks. At the beginning 
of the loop, 'then' becomes equal to 2^32 - 100. While the loop runs, 
time(then) equals 'ticks - then', or, '2^32 - 100 + n - (2^32 - 100)', 
or '2^32 - 100 - 2^32 + 100 +n', or (since we're computing modulo 2^32) 
'-100 + 100 +n', or 'n', where n is the number of ticks elapsed since 
'now' was caculated. When n is 200 or more, the loop end condition is 
met. Crossing the 0 value did not affect the computation -- or did it?

> Note also that "fast_tick_rate" would not be a constant in AT91, it is
> dynamically calculated from main xtal frequency measured against the 32kHz
> xtal and PLL settings.

That's another issue entirely, with two consequences, but they don't 
really differ with respect to which proposal, yours or mine, is considered.

1. This makes the compiler unable to optimize ms to tick conversions at 
compile time. Not a really big deal, and unavoidable as soon as the 
decision is taken to calibrate the fast tick at run time.

2. If calibration is done continuously, then ticks *may* not be 
monotonous. If it is done once at startup, a la bogomips calculation, 
then we should be safe.

>> Note that time(x) assumes unsigned arguments and amounts to an unsigned
>> compare, because we're always computing an difference time, i.e. even
>> with x = 2 and ticks = 1, the result is correct -- that's assuming ticks
>> is monotonous 32-bits (or 64-bits for the platforms that can support it
>> as an atomic value)
>
> Assume: Monotonous AND never wrapping back to zero!

I think zero wraparound is not an issue if the ticks are on N (ideally, 
32 or 64) bits, which is most probably the case for a free-running 
counter. We need to assume monotonous at least during delay loops; since 
we have little or no parallel execution, we should be able to avoid 
recalibration while a delay loop is running, right?

>> Your example would then become
>>
>> then = time(0);
>> do {...} while ( time(then)< ms_to_ticks(100) );
>
> That looks ugly to me. We don't want to see the high speed(64 bit) values
> in the drivers - I think.

We don't see them, actually; but why would HW tick values would be 
unwelcome in drivers that already handle, and actually are designed to 
handle, lots of other HW related values? And besides, how does this 
differ from your proposal in this respect?

>> ... moving the actual timeout value impact from the time sample before
>> the 'while' to the 'while' condition at then end.
>
> Which does a multiply and a divide in 64 bit each loop iteration...
> (fast_tick_rate being a variable)

Not if the compiler is half decent; it will optimize out the constant 
expression. And if you don't trust the compiler, you can pre-compute the 
value of the delay.

>> For expressiveness, added macros such as:
>>
>> #define now() time(0)
>> #define ms_elapsed(then,ms) ( time(then)< ms_to_ticks(ms) )
>>
>> ... would allow writing the same example as:
>>
>> then = now();
>> do {...} while ( !ms_elapsed(then,100) );
>
> Why make everything so complicated???

I am surprised by this question. The last point I make is to simplify 
things, not complicate them, and besides, it is only for expressiveness 
and clarity of the code.

> The how-many-th thread about timers is this ? :)

The last one, trust me. :)

> Best Regards,
> Reinhard

Amicalement,
-- 
Albert.