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

[Reinhard Meyer]

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.

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

> At the lowest level, the U-Boot timer API consists of a unsigned 32-bit
> free running timestamp which increments every millisecond (wraps around
> every 4294967 seconds, or about every 49.7 days). The U-Boot timer API
> allows:
>   - Time deltas to be measured between arbitrary code execution points
> 	ulong start_time;
> 	ulong elapsed_time;
>
> 	start_time = get_timer(0);
> 	...
> 	elapsed_time = get_timer(start_time);
>
>   - Repetition of code for a specified duration
> 	ulong start_time;
>
> 	start_time = get_timer(0);
>
> 	while (get_timer(start_time)<  REPEAT_TIME) {
> 		...
> 	}
>
>   - Device timeout detection
> 	ulong start_time;
>
> 	send_command_to_device();
> 	start = get_timer (0);
> 	while (device_is_busy()) {
> 		if (get_timer(start)>  TIMEOUT)
> 			return ERR_TIMOUT;
> 		udelay(1);
> 	}
> 	return ERR_OK;
correct.

>
> 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.

>
> 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.

> 	void set_timer(ulong preset)
> 	void reset_timer(void)
>
> get_timer() returns the number of milliseconds since 'start_time'. If
> 'start_time' is zero, therefore, it returns the current value of the
> free running counter which can be used as a reference for subsequent
> timing measurements.
>
> set_timer() sets the free running counter to the value of 'preset'
> reset_timer() sets the free running counter to the value of zero[1]. In
> theory, the following examples are all identical
>
> 	----------------------------------------------------
> 	ulong start_time;
> 	ulong elapsed_time;
>
> 	start_time = get_timer(0);
> 	...
> 	elapsed_time = get_timer(start_time);
> 	----------------------------------------------------
> 	ulong elapsed_time;
>
> 	reset_timer();
> 	...
> 	elapsed_time = get_timer(0);
> 	----------------------------------------------------
> 	ulong elapsed_time;
>
> 	set_timer(0);
> 	...
> 	elapsed_time = get_timer(0);
> 	----------------------------------------------------
>
> [1] arch/arm/cpu/arm926ejs/at91/ and arch/arm/cpu/arm926ejs/davinci/ are
> exceptions, they set the free running counter to get_ticks() instead
Not anymore on at91.
>
> Architecture Specific Peculiarities
> ===================================
> ARM
>   - Generally define get_timer_masked() and reset_timer_masked()
>   - [get,reset]_timer_masked() are exposed outside arch\arm which is a bad
>     idea as no other arches define these functions - build breakages are
>     possible although the external files are most likely ARM specific (for
>     now!)
>   - Most CPUs define their own versions of the API get/set functions which
>     are wrappers to the _masked equivalents. These all tend to be the same.
>     The API functions could be moved into arch/arm/lib and made weak for
>     the rare occasions where they need to be different
>   - Implementations generally look sane[2] except for the following:
>     - arm_intcm - No timer code (unused CPU arch?)
>     - arm1136/mx35 - set_timer() is a NOP
>     - arm926ejs/at91 - reset_timer() sets counter to get_ticks()
>                        no implelemtation of set_timer()
See current master for actual implementation!
>     - arm926ejs/davinci - reset_timer() sets counter to get_ticks()
>                           no implelemtation of set_timer()
>     - arm926ejs/mb86r0x - No implementation of set_timer()
>     - arm926ejs/nomadik - No implementation of set_timer()
>     - arm946es - No timer code (unused CPU arch?)
>     - ixp - No implementation of set_timer()
>           - If CONFIG_TIMER_IRQ is defined, timer is incremented by an
>             interrupt routine - reset_timer() writes directly to the
>             counter without interrupts disables - Could cause corruption
>             if reset_timer() is not atomic
>           - If CONFIG_TIMER_IRQ is not defined, reset_timer() appears to
>             not be implemented
>     - pxa - set_timer() is a NOP
>     - sa1100 - get_timer() does not subtract the argument
>
> nios, powerpc, sparc, x86
>   - All look sane[2]
>
> avr32
>   - Not obvious, but looks sane[2]
>
> blackfin
>   - Does not implement set_timer()
>   - Provides a 64-bit get_ticks() which simply returns 32-bit get_timer(0)
>   - reset_timer() calls timer_init()
>   - Looks sane[2]
>
> m68k
>   - Looks sane[2] if CONFIG_MCFTMR et al are defined. If CONFIG_MCFPIT is
>     defined instead, reset_timer() is unimplemented and build breakage will
>     result if cfi driver is included in the build - No configurations use
>     this define, so that code is dead anyway
>
> microblaze
>   - Only sane[2] if CONFIG_SYS_INTC_0 and CONFIG_SYS_TIMER_0 are both defined.
>     Doing so enables a timer interrupt which increments the internal
>     counter. Otherwise, it is incremented when get_timer() is called which
>     will lead to horrible (i.e. none at all) accuracy
>
> mips
>   - Not obvious, but looks sane[2]
>
> sh
>   - Generally looks sane[2]
>   - Writes 0xffff to the CMCOR_0 control register when resetting to zero,
>     but writes the actual 'set' value when not zero
>   - Uses a 32-bit microsecond based timebase:
> 	static unsigned long get_usec (void)
> 	{
> 	...
> 	}
>
> 	get_timer(ulong base)
> 	{
> 		return (get_usec() / 1000) - base;
> 	}
>
>   - Timer will wrap after ~4295 seconds (71 minutes)
>
> [2] Sane means something close to:
> 	void reset_timer (void)
> 	{
> 		timestamp = 0;
> 	}
>
> 	ulong get_timer(ulong base)
> 	{
> 		return timestamp - base;
> 	}
>
> 	void set_timer(ulong t)
> 	{
> 		timestamp = t;
> 	}
>
> 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 !!!
>
> 0. Fix arch/arm/cpu/sa1100/timer.c:get_timer()
> ----------------------------------------------
> This appears to be the only get_timer() which does not subtract the
> argument from timestamp
>
> 1. Remove set_timer()
> ---------------------
> regex "[^e]set_timer\s*\([^)]*\);" reveals 14 call sites for set_timer()
> and all except arch/sh/lib/time_sh2:[timer_init(),reset_timer()] are
> set_timer(0). The code in arch/sh is trivial to resolve in order to
> remove set_timer()
>
> 2. Remove reset_timer()
> -----------------------------------------------
> regex "[\t ]*reset_timer\s*\([^)]*\);" reveals 22 callers across the
> following groups:
>   - timer_init() - Make reset_timer() static or fold into timer_init()
>   - board_init_r() - Unnecessary
>   - arch/m68k/lib/time.c:wait_ticks() - convert[3]
>   - Board specific flash drivers - convert[3]
>   - drivers/block/mg_disk.c:mg_wait() - Unnecessary
>   - drivers/mtd/cfi_flash.c:flash_status_check() - Unnecessary
>   - drivers/mtd/cfi_flash.c:flash_status_poll() - Unnecessary
>
> [3] These instance can be trivially converted to use one of the three
> examples at the beginning of this document
>
> 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.
>
> Most of these instance can be converted to use one of the three examples
> at the beginning of this document, but __udelay() will need closer
> examination
>
> This is preventing nesting of timed code - Any code which uses udelay()
> has the potential to foul up outer-loop timers. The problem is somewhat
> unique to ARM though
>
> 4. Implement microsecond API - get_usec_timer()
> -----------------------------------------------
>   - Useful for profiling
>   - A 32-bit microsecond counter wraps in 71 minutes - Probably OK for most
>     U-Boot usage scenarios
>   - By default could return get_timer() * 1000 if hardware does not support
>     microsecond accuracy - Beware of results>  32 bits!
>   - 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!

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

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.

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.

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...

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
- must not delay less than the given time, may delay longer
(this might be true especially for very small delay values)
- shall not be used for delays in the seconds range and longer
(or any other limit we see practical)
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!

2. u32 get_timer(u32 base) with the following properties:
- must return the elapsed milliseconds since base
- must work without wrapping problems for at least several hours

Best Regards,
Reinhard