[U-Boot] [RFC] Review of U-Boot timer API

[Graeme Russ <graeme.russ@gmail.com>]

On 22/05/11 14:26, Reinhard Meyer wrote:
> Dear Graeme Russ,
>> Hi All,
>>
>> I've just had a good look through the timer API code with the view of
>> rationalising it and fixing up some of the inconsistencies. What I found
>> was a little scarier than I expected! Anyway, here is a write-up of what I
>> found - Please comment
> 
> We have been at this discussion a multiple of times :) but never reached a
> consent.
> 
> However, at current master, I have reduced at91 architecture to only use
> get_timer(base), set_timer() never existed and reset_timer() has been removed.

Excellent

> As it showed up recently, common cfi code still calls reset_timer() - which
> certainly
> can be fixed with little effort...

Yes, this is one of the easy fixes as all call sites already use the start
= get_timer(0), elapsed = get_timer(start) convention anyway - The
reset_timer() calls are 100% redundant (provided get_timer() behaves
correctly at the 32-bit rollover for all arches)

>> The U-Boot timer API is not a 'callback' API and cannot 'trigger' a
>> function call after a pre-determined time.
> that would be too complex to implement and of little use in a single task
> system. u-boot can do fine with polling.

I am in no way suggesting this - I just want to clarify the API for anyone
who needs to use it

>> NOTE: http://lists.denx.de/pipermail/u-boot/2010-June/073024.html appears
>> to imply the following implementation of get_timer() is wrong:
>>
>>     ulong get_timer(ulong base)
>>     {
>>         return get_timer_masked() - base;
>>     }
> Is is not wrong as long as get_timer_masked() returns the full 32 bit space
> of numbers and 0xffffffff is followed by 0x00000000. Most implementations
> probably do NOT have this property.
>>
>> U-Boot Timer API Details
>> ========================
>> There are currently three functions in the U-Boot timer API:
>>     ulong get_timer(ulong start_time)
> As you point out in the following, this is the only function required.
> However it REQUIRES that the internal timer value must exploit the full
> 32 bit range of 0x00000000 to 0xffffffff before it wraps back to 0x00000000.

So this needs to be clearly spelt out in formal documentation

>> Rationalising the API
>> =====================
>> Realistically, the value of the free running timer at the start of a timing
>> operation is irrelevant (even if the counter wraps during the timed period).
>> Moreover, using reset_timer() and set_timer() makes nested usage of the
>> timer API impossible. So in theory, the entire API could be reduced to
>> simply get_timer()
> Full ACK here !!!

I don't think there will be much resistance to this

>> 3. Remove reset_timer_masked()
>> ------------------------------
>> This is only implemented in arm but has propagated outside arch/arm and
>> into board/ and drivers/ (bad!)
>>
>> regex "[\t ]*reset_timer_masked\s*\([^)]*\);" reveals 135 callers!
>>
>> A lot are in timer_init() and reset_timer(), but the list includes:
>>   - arch/arm/cpu/arm920t/at91rm9200/spi.c:AT91F_SpiWrite()
>>   - arch/arm/cpu/arm926ejs/omap/timer.c:__udelay()
>>   - arch/arm/cpu/arm926ejs/versatile/timer.c:__udelay()
>>   - arch/arm/armv7/s5p-common/timer.c:__udelay()
>>   - arch/arm/lh7a40x/timer.c:__udelay()
>>   - A whole bunch of board specific flash drivers
>>   - board/mx1ads/syncflash.c:flash_erase()
>>   - board/trab/cmd_trab.c:do_burn_in()
>>   - board/trab/cmd_trab.c:led_blink()
>>   - board/trab/cmd_trab.c:do_temp_log()
>>   - drivers/mtd/spi/eeprom_m95xxx.c:spi_write()
>>   - drivers/net/netarm_eth.c:na_mii_poll_busy()
>>   - drivers/net/netarm_eth.c:reset_eth()
>>   - drivers/spi/atmel_dataflash_spi.c/AT91F_SpiWrite()
> Fixed in current master.

Excellent. I have not pulled master for a little while, guess I should

>>   - If hardware supports microsecond resolution counters, get_timer() could
>>     simply use get_usec_timer() / 1000
> 
> That is wrong. Dividing 32 bits by any number will result in a result that
> does not
> exploit the full 32 bit range, i.e. wrap from (0xffffffff/1000) to 0x00000000,
> which makes time differences go wrong when they span across such a wrap!
> 

Yes, this has already been pointer out - 42 bits are needed as a bare
minimum. However, we can get away with 32-bits provided get_timer() is
called at least every 71 minutes

P.S. Can we use the main loop to kick the timer?

>>   - get_usec_timer_64() could offer a longer period (584942 years!)
> Correct. And a "must be" when one uses such division.

Unless we can rely on get_timer() to be called at least every 71 minutes in
which case we can handle the msb's without error in software

> But you have to realize that most hardware does not provide a simple means to
> implement a timer that runs in either exact microseconds or exact
> milliseconds.

This is where things get interesting and we need to start pushing a
mandated low-level HAL. For example, I believe get_timer() should be
implemented in /lib as:

	ulong get_timer(ulong base)
	{
		return get_raw_msec() - base;
	}

get_raw_ms() MUST:
 - Return an unsigned 32-but value which increments every 1ms
 - Wraps from 0xffffffff to 0x00000000
 - Be atomic (no possibility of corruption by an interrupt)

The counter behind get_raw_ms() can be maintained by either:

 1. A hardware timer programmed with a 1ms increment
 2. A hardware timer programmed with a non-1ms increment scaled in software
 3. A software counter ticked by a 1ms interrupt
 4. A software counter ticked by a non-1ms interrupt scaled in software

get_raw_ms() does not need a fixed epoch - It could be 1st Jan 1970, the
date the CPU/SOC was manufactured, when the device was turned on, your
eldest child's birthday - whatever. It will not matter provided the counter
wraps correctly

> Powerpc for example has a 64 bit free running hardware counter at CPU clock,
> which can be in the GHz range, making the lower 32 bits overflow within
> seconds,
> so the full 64 bits MUST BE used to obtain a millisecond timer by division.
> arm/at91 has a timer that can be made to appear like a 32 bit free running
> counter
> at some fraction of cpu clock (can be brought into a few MHz value by a
> prescaler)
> and the current implementation extends this to 64 bits by software, so it is
> similar to powerpc.

So these are all examples of #2

x86 is an example of #3

> A get timer() simply uses this 64 bit value by dividing it by (tickrate/1000).
> 
> Of course this results in a wrong wrap "gigaseconds" after the timer has
> been started,
> but certainly this can be ignored...

Strictly speaking, I don't think we should allow this - There should never
be timer glitches

> On any account, I see only the following two functions to be implemented
> for use by
> other u-boot code parts:
> 
> 1. void udelay(u32 microseconds) with the following properties:
> - must not affect get_timer() results

Absolutely

> - must not delay less than the given time, may delay longer
> (this might be true especially for very small delay values)

Hadn't though about that, but OK

> - shall not be used for delays in the seconds range and longer
> (or any other limit we see practical)

Any udelay up to the full range of a ulong should be handled without error
by udelay() - If the arch dependant implementation of udelay() uses
get_timer() to implement long delays due to hardware limitations then that
should be fine.

> Note: a loop doing "n" udelays of "m" microseconds might take _much_ longer
> than
> "n * m" microseconds and therefore is the wrong approach to implement a
> timeout.
> get_timer() must be used for any such timeouts instead!

ACK - as mentioned, udelay() can use get_timer() of the delay is >> 1ms if
such an implementation provides better accuracy. If the HAL implementation
of get_raw_ms() uses a hardware level microsecond time base, there will be
no accuracy advantage anyway.

> 2. u32 get_timer(u32 base) with the following properties:
> - must return the elapsed milliseconds since base

ACK

> - must work without wrapping problems for at least several hours

Provided that the architecture implementation of get_raw_ms() is
implemented as described, the only limitation will be the maximum
measurable delay. The timer API should work correctly no matter how long
the device has been running

I think the HAL should implement:
	- get_raw_ms() - 32-bit millisecond counter
	- get_raw_us() - 32-bit microsecond counter
	- get_raw_us64() - 64-bit microsecond counter

Regards,

Graeme