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

[Graeme Russ]

On 22/05/11 17:21, J. William Campbell wrote:
> On 5/21/2011 11:23 PM, Graeme Russ wrote:
>> On 22/05/11 14:26, Reinhard Meyer wrote:

>>> - 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

We would have to change the prototype for udelay()

> 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.

Agree

> 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.

Who is ever going to check if udelay() returned an error? It is a primitive
that one assumes 'just works' and should work for all possible parameter values

>>> 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?

What if the udelay parameter is calculated? You would then need an explicit
check for >65ms - Seems a bit onerous if you're expecting to occasionally
(if ever) to exceed this, but it not being out of the realm of possibility

>>> 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.

True, but useful

>>     - 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

*cough* minutes *cough* ;)

> is certainly long enough for anything this precise. This certainly should
> NOT be mandatory. 64 bits is not for free on many processors.

True - I agree there is no need for it

Regards,

Graeme