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

[J. William Campbell]

On 5/22/2011 11:29 PM, Albert ARIBAUD wrote:
> Hi all,
>
> Sorry, could not follow the discussion although I find it very
> interesting, so I will handle the task of coming in late and asking the
> silly questions.
I am glad you are looking at our discussion. I am sure we are going to 
need all the help/oversight/questions that we can get, as this is a 
change that will affect all architectures.
> Le 23/05/2011 07:25, Graeme Russ a écrit :
>
>> On Mon, May 23, 2011 at 3:02 PM, J. William Campbell
>> <jwilliamcampbell at comcast.net>   wrote:
>>> On 5/22/2011 6:42 PM, Graeme Russ wrote:
>>>> OK, so in summary, we can (in theory) have:
>>>>    - A timer API in /lib/ with a single u32 get_timer(u32 base) function
>>>>    - A HAL with two functions
>>>>      - u32 get_raw_ticks()
>>>>      - u32 get_raw_tick_rate() which returns the tick rate in kHz (which
>>>>        max's out at just after 4GHz)
>>>>    - A helper function in /lib/ u32 get_raw_ms() which uses get_raw_ticks()
>>>>      and get_tick_rate() to correctly maintain the ms counter used by
>>>>      get_timer() - This function can be weak (so next point)
>>>>    - If the hardware supports a native 32-bit ms counter, get_raw_ms()
>>>>      can be overridden to simply return the hardware counter. In this case,
>>>>      get_raw_ticks() would return 1
> Are you sure you did not mean 'get_raw_ticks_rate' here? Besides, I'd
> like the name to specify the units used: 'get_raw_ticks_rate_in_khz' (or
> conversively 'get_raw_ticks_per_ms', depending on which is simpler to
> implement and use).
I think you are correct, it was the rate function desired here. I think 
the best way to go is use a "get_raw_tick_rate_in_mhz" function, because 
it is probably the easiest one to implement, and in many cases something 
like it already exists.
>>>>    - Calling of get_raw_ticks() regularly in the main loop (how ofter will
>>>>      depend on the raw tick rate, but I image it will never be necessary
>>>>      to call more often than once every few minutes)
> That's to keep track of get_raw_ticks() rollovers, right? And then the
> get_timer function (which, again, I would prefer to have '... in ms'
> expressed in its name) would call get_raw_ticks() in confidence that at
> most one rollover may have occurred since the last time the helper
> function was called, so a simple difference of the current vs last tick
> value will always be correct.
Exactly so. Note that this same function probably needs to be called in 
udelay for the same reason. More precisely, the get_timer function will 
call get_raw_ms, which will call get_raw_ticks. I think it may be better 
to move get_timer "down" a level in the hierarchy,
so we don't need a get_raw_ms. get_timer would then be part of the HAL. 
One would use a get_timer(0) in order to do what get_raw_ms alone would 
have done. If the user had a good reason, he would then override 
get_timer with his own version. What do you think Graeme? It reduces the 
nesting depth by one level. As for the name change to get_timer_in_ms, I 
would support it. Naturally, such a change would be up to Mr. Denk. 
Since by definition that is what the function does, it seems to be a 
good change from my point of view.
>>>>    - If the hardware implements a native 32-bit 1ms counter, no call in
>>>>      the main loop is required
>>>>    - An optional HAL function u32 get_raw_us() which can be used for
>>>>      performance profiling (must wrap correctly)
>>> Hi All,
>>>        Graeme, I think you have stated exactly what is the "best" approach to
>>> this problem.  I will provide a version of "get_raw_ms" that is  initialized
>>> using get_raw_tick_rate that will work for all "reasonable" values of
>>> raw_tick_rate. This will be the "generic" solution. Both the initialization
>>> function and the get_raw_ms function can be overridden if there is reason to
>>> do so, like "exact" clock rates. I will do the same with get_raw_us. This
>>> will be posted sometime on Monday for people to review, and to make sure I
>>> didn't get too far off base. Thank you to both Graeme and Reinhard for
>>> looking at/working on this.. Hopefully, this solution will put this timing
>>> issue to rest for all future ports as well as the presently existing ones.
> In Greame's description, I did not see a get_raw_ms, only a get_raw_us.
> Was this last one a typo or is that a third HAL function?
get_raw_ms was referenced as a library function a few lines above.  
Right now, I think the functionality we require from the HAL is

   1. get_raw_tick_rate_in_mhz
   2. get_raw_ms
   3. get_raw_ticks
   4. (optional)get_raw_us

There is also  APIs for these functions called get_timer.

I think we need to add a call to another function called 
"initialize_timer_system" or similar that will initialize the data 
structures in gd by calling get_raw_tick_rate_in_mhz. Additionally, I 
think we need to provide a udelay function, simply because it can 
interact with calling get_raw_ms often enough. We are somewhat caught 
between two fires here, in that on the one hand we want to provide a 
very "generic" approach to the timing system that will work on "any" CPU 
while on the other hand we want to allow easy 
customization/simplification of the timer system where appropriate. 
There is more than one way to approach this. Another approach is to 
define the HAL as:

   1. get_timer (_in_ms if approved)
   2. udelay
   3. (optional)get_raw_us
   4. get_raw_tick_rate_in_mhz
   5.   initialize_timer_system

We would provide a "reference" implementation of get_timer and udelay 
with comments regarding the implementation of the get_raw_ticks 
functionality (static inline comes to mind). This would provide a 
functional but possibly not optimal timer system. Later, once everything 
is working, the user could "hack up" the get_timer routines and the 
initialize_timer_system routines to remove any functionality that was 
not relevant to his system, such as using a fixed clock rate etc. The 
benefit of this approach is that in simple systems where space is real 
tight one can still utilize a minimal, optimal timer system without the 
extra code required to make it generic. There are no "extra levels". The 
disadvantage of this approach is that it allows the user to wander off 
into an incompatible non-standard implementation. Thoughts on this 
subject are welcome. The design we have will work, but it will require 
somewhat more text than the "present" system. How much more, and does it 
matter, I don't know.
>> Great - I am in the middle of cleaning up the current usages of the timer
>> API, reducing it all down to get_timer() - I will then 'libify'
>> get_timer() and setup the hooks into the HAL get_raw_ticks() and
>> get_raw_tick_rate() API
>>
>> I think there will need to be a lot of cleanup, especially in arch/arm to
>> get this to all fit
I share your concern. We may have to iterate a bit.

Best Regards,
Bill Campbell

>> Regards,
>>
>> Graeme
> Amicalement,