[U-Boot] [RFC] Review of U-Boot timer API
J. William Campbell
jwilliamcampbell at comcast.net
Sun May 22 08:57:49 CEST 2011
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
>
>
More information about the U-Boot
mailing list