From mboxrd@z Thu Jan 1 00:00:00 1970 From: Graeme Russ Date: Sat, 21 May 2011 22:38:29 +1000 Subject: [U-Boot] [RFC] Review of U-Boot timer API Message-ID: <4DD7B245.5000008@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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!)