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

[J. William Campbell]

On 5/21/2011 11:23 PM, Graeme Russ wrote:
> 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)
Hi All,
       I agree that a mandated HAL would be a real good idea in reducing 
the uncertainty in what is required. This is a very good idea, IMHO.
> 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
Agreed, although this glitch probably would never be observed.
>> 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.
I wonder about this requirement. I think it might be better to say that 
udelay can only be used up to a delay of a short unsigned (65 ms), If a 
longer delay is required, a get_timer loop should be used.  Since udelay 
uses a different resolution, it implies a more precise delay is 
required. If you are delaying more than 65 ms, I suspect precision is 
not what you are after, and udelay should return an error (not be void). 
This makes implementing udelay a lot easier, in that one can always 
convert the desired time interval into an internal clock resolution 
without difficulty.
>> 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.
What if it provides worse accuracy? It is better if udelay can use the 
hardware timer resolution directly. This is easy if the delay is a short 
unsigned, so the desired delay can be internally expressed as a long 
unsigned in hardware timer ticks. Otherwise, it gets harder, and udelay 
is not oriented towards longer delays. It CAN be done, but why should we?
>> 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
Only the get_raw_ms should be mandatory. I propose adding udelay to the 
HAL, as it is best mechanized at the hardware timer tick level.

> 	- get_raw_us() - 32-bit microsecond counter
Used for performance monitor. Not mandatory.
> 	- get_raw_us64() - 64-bit microsecond counter
I can see no possible use for this in u-boot. What operation that we 
care about could take this long  and require microsecond resolution? 71 
seconds is certainly long enough for anything this precise. This 
certainly should NOT be mandatory. 64 bits is not for free on many 
processors.

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