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

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

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

Regards,

Graeme

--------

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;

The U-Boot timer API is not a 'callback' API and cannot 'trigger' a
function call after a pre-determined time.

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;
	}

U-Boot Timer API Details
========================
There are currently three functions in the U-Boot timer API:
	ulong get_timer(ulong start_time)
	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

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()
   - 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()

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()

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
 - get_usec_timer_64() could offer a longer period (584942 years!)