[U-Boot] TIMER cleanup RFC, was: [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Reinhard Meyer]

Dear J. William Campbell,
> On 11/30/2010 1:14 AM, Reinhard Meyer wrote:
>> Dear Wolfgang Denk,
>>
>> what we really need is only a 32 bit monotonous free running tick that
>> increments
>> at a rate of at least 1 MHz. As someone pointed out a while ago, even
>> at 1GHz that would
>> last for four seconds before it rolls over. But a 1HGz counter could
>> be 64 bit internally
>> and always be returned as 32 bits when it is shifted right to bring it
>> into the "few" MHz
>> range.
>>
>> Any architecture should be able to provide such a 32 bit value. On
>> powerpc that would
>> simply be tbu|tbl shifted right a few bits.
>>
>> An architecture and SoC specific timer should export 3 functions:
>>
>> int timer_init(void);
>> u32 get_tick(void); /* return the current value of the internal free
>> running counter */
>> u32 get_tbclk(void); /* return the rate at which that counter
>> increments (per second) */
>>
>> A generic timer function common to *most* architectures and SoCs would
>> use those two
>> functions to provice udelay() and reset_timer() and get_timer().
>> Any other timer functions should not be required in u-boot anymore.
>>
>> However get_timer() and reset_timer() are a bit of a functional problem:
>>
>> currently reset_timer() does either actually RESET the free running
>> timer (BAD!) or
>> remember its current value in another (gd-)static variable which later
>> is subtracted
>> when get_timer() is called. That precludes the use of several timers
>> concurrently.
>>
>> Also, since the 1000Hz base for that timer is usually derived from
>> get_tick() by
>> dividing it by some value, the resulting 1000Hz base is not exhausting
>> the 32 bits
>> before it wraps to zero.
(###)
>>
>> Therefore I propose two new functions that are to replace
>> reset_timer() and get_timer():
>>
>> u32 init_timeout(u32 timeout_in_ms); /* return the 32 bit tick value
>> when the timeout will be */
>> bool is_timeout(u32 reference); /* return true if reference is in the
>> past */
>>
>> A timeout loop would therefore be like:
>>
>> u32 t_ref = timeout_init(3000);    /* init a 3 second timeout */
>>
>> do ... loop ... while (!is_timeout(t_ref));
>>
>> coarse sketches of those functions:
>>
>> u32 init_timeout(u32 ms)
>> {
>>     return get_ticks() + ((u64)get_tbclk() * (u64)ms) / (u64)1000;
>> }
>>
>> bool is_timeout(u32 reference)
>> {
>>     return ((int)get_ticks() - (int)reference)>  0;
>> }
>>
>> Unfortunately this requires to "fix" all uses of get_timer() and
>> friends, but I see no other
>> long term solution to the current incoherencies.
>>
>> Comments welcome (and I would provide patches)...
> Hi All,
>       The idea of changing the get_timer interface to the
> init_timeout/is_timeout pair has the advantage that it is only necessary
> to change the delay time in ms to an internal timebase once, and after
> that, only a 32-bit subtraction is required.
Exactly.
> I do not however like the
> idea of using 64 bit math to do so, as on many systems this is quite
> expensive. However, this is a feature that can be optimized for
> particular CPUs.
64 Bit math is only necessary when get_tbclk() times the maximun anticipated
timeout (in ms) is larger than 32 bits. This happens easyly when tbclk is a
few MHz. Besides the current working(!) implementations use 64 bit math
already. You can optimize into 32 bits by factoring the timeout_in_ms into
whole seconds, and the remainder.
> I also REALLY don't like the idea of having a get_ticks
> function, because for sure people will use this instead of the desired
> interface to the timer because it is "better". Then we get back into a
> mess. Since in most cases get_ticks is one or two instructions, please,
> let us hide them in init_timeout/is_timeout.
Agreed. However I intended to split the timer into two sources (read above):
one hardware dependant part exporting exactly those functions, and one
generic part using them.
>        An alternate approach, which has the merit of being more like the
> originally intended interface, simply disallows reset_timer since it is 
> totally unnecessary. The only dis-advantage of the original approach
> using just get_timer is that the conversion to ms must be considered at
> each call to get_timer, and will require at a minimum one 32 bit integer
> to remember the hardware timer value the last time get_timer was called
"remembering" that inside the timer function is exactly what I want to get
rid of. And that would not work in case of the usage method (2.) below.
> (unless the hardware time can be trivially converted to a 32 bit value
> in ms, which is quite uncommon).
Indeed.
> This is not a high price to pay, and
> matches the current usage. This is probably for Mr. Denk to decide. If
The "current usages" are mainly those two:
1. reset_timer(); ... if (get_timer(0) > TIMEOUT) ...
--> this is an ab-use, of course, but can be found in u-boot.
2. epoch = get_timer(0); ... if (get_timer(epoch) > TIMEOUT) ...
--> this is the correct way to use get_timer(), however it has the problem
of incorrect rollover after 32 bits. See (###) above!
> we were just starting now, the init_timeout/is_timeout is simpler, but
> since we are not, perhaps keeping the current approach has value.
Finding the current uses of get_timer() is a simple grep, fixing the code
should not require more than a few minutes per location. Testing, however
is a problem. But ANY change would require testing.
>        I would really like to help by providing some patches, but I am
> just way too busy at present.
Same here. Nevertheless some xmax holidays are coming up, and maybe some
idle hours in them ;)

Best Regards,
Reinhard