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 theoriginally 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 amjust way too busy at present.
Same here. Nevertheless some xmax holidays are coming up, and maybe some idle hours in them ;)
Best Regards, Reinhard