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

[J. William Campbell <jwilliamcampbell@comcast.net>]

On 5/21/2011 9:26 PM, 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.
>
> 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!
Hi All,
       True, as previously discussed.
>>    - 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.
Also True.
> 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.
       This statement, as written, is false. While it is true that the 
power PC (and others) have a 64 bit counter that runs at a many 
megahertz rate, it is NOT true that all 64 bits must be used to obtain a 
millisecond timer. A millisecond timer can be produced by using the 33 
bits of the 64 bit counter whose lsb is <= 1 ms and > 0.5 ms. This value 
can be extracted by a right shift and masking of the 64 bit counter. Two 
32 bit multiplies and adds will produce a correct monotonic 1 ms timer. 
This is certainly the best way to go on systems that do not have a 
hardware 64 bit by 32 bit divide. See the thread "ARM timing code 
refactoring" for an example of how this can be done. This timer will 
NEVER result in a wrong value, although it does loose  1 ms every 1193 
hours.
> 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).
Or as above on the PPC
> Of course this results in a wrong wrap "gigaseconds" after the timer has been started,
> but certainly this can be ignored...
FWIW, this does not happen with the shift and multiply approach.
> 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
I think this is a correct appreciation of what is needed.

Best Regards.
Bill Campbell
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>