On 22/05/11 14:26, 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.
Excellent > As it showed up recently, common cfi code still calls reset_timer() - which > certainly > can be fixed with little effort... Yes, this is one of the easy fixes as all call sites already use the start = get_timer(0), elapsed = get_timer(start) convention anyway - The reset_timer() calls are 100% redundant (provided get_timer() behaves correctly at the 32-bit rollover for all arches) >> 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. I am in no way suggesting this - I just want to clarify the API for anyone who needs to use it >> 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. So this needs to be clearly spelt out in formal documentation >> 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 !!! I don't think there will be much resistance to this >> 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. Excellent. I have not pulled master for a little while, guess I should >> - 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! > Yes, this has already been pointer out - 42 bits are needed as a bare minimum. However, we can get away with 32-bits provided get_timer() is called at least every 71 minutes P.S. Can we use the main loop to kick the timer? >> - get_usec_timer_64() could offer a longer period (584942 years!) > Correct. And a "must be" when one uses such division. Unless we can rely on get_timer() to be called at least every 71 minutes in which case we can handle the msb's without error in software > 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. This is where things get interesting and we need to start pushing a mandated low-level HAL. For example, I believe get_timer() should be implemented in /lib as: ulong get_timer(ulong base) { return get_raw_msec() - base; } get_raw_ms() MUST: - Return an unsigned 32-but value which increments every 1ms - Wraps from 0xffffffff to 0x00000000 - Be atomic (no possibility of corruption by an interrupt) The counter behind get_raw_ms() can be maintained by either: 1. A hardware timer programmed with a 1ms increment 2. A hardware timer programmed with a non-1ms increment scaled in software 3. A software counter ticked by a 1ms interrupt 4. A software counter ticked by a non-1ms interrupt scaled in software get_raw_ms() does not need a fixed epoch - It could be 1st Jan 1970, the date the CPU/SOC was manufactured, when the device was turned on, your eldest child's birthday - whatever. It will not matter provided the counter wraps correctly > 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. So these are all examples of #2 x86 is an example of #3 > 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... Strictly speaking, I don't think we should allow this - There should never be timer glitches > 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 Absolutely > - must not delay less than the given time, may delay longer > (this might be true especially for very small delay values) Hadn't though about that, but OK > - shall not be used for delays in the seconds range and longer > (or any other limit we see practical) Any udelay up to the full range of a ulong should be handled without error by udelay() - If the arch dependant implementation of udelay() uses get_timer() to implement long delays due to hardware limitations then that should be fine. > 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! ACK - as mentioned, udelay() can use get_timer() of the delay is >> 1ms if such an implementation provides better accuracy. If the HAL implementation of get_raw_ms() uses a hardware level microsecond time base, there will be no accuracy advantage anyway. > 2. u32 get_timer(u32 base) with the following properties: > - must return the elapsed milliseconds since base ACK > - must work without wrapping problems for at least several hours Provided that the architecture implementation of get_raw_ms() is implemented as described, the only limitation will be the maximum measurable delay. The timer API should work correctly no matter how long the device has been running I think the HAL should implement: - get_raw_ms() - 32-bit millisecond counter - get_raw_us() - 32-bit microsecond counter - get_raw_us64() - 64-bit microsecond counter Regards, Graeme _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
