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

[J. William Campbell <jwilliamcampbell@comcast.net>]

On 5/24/2011 9:51 AM, Graeme Russ wrote:
> On 25/05/11 00:19, Wolfgang Denk wrote:
> =============
<snip>
Hi all,
         I have a few of questions.
First, it seems that the get_timer interface is expected to work 
properly only after relocation and only when bss is available. I say 
this because the PPC version uses an (initialized) variable, timestamp, 
to hold the time. If that is the case, there is no need to hold the 
timer static data in gd, as I have been doing up to  now. Am I correct ?
      Second, udelay is expected to be available before  bss is 
available, very early in the startup process. There a comments to that 
effect in several places in the code. Therefore, udelay cannot use 
global or static variables. Is this true?
     And third, udelay is only expected/required to work correctly for 
"short", like say 10 seconds. I say this based on comments contained in 
powerpr/lib/time.c. Please ack or nak this as well.

I have a couple of "small" changes to the previously submitted code, 
based partly on the above
> Exposing ticks and tick_frequency to everyone via a 'tick' HAL
>
> In /lib/timer.c

ulong timestamp = 0;
/*
      This routine is called to initialize the timer after BSS is available
*/
void  __weak prescaler_init(void)
{
     u32 tick_frequency = get_tick_frequency();
      /* initialize prescaler variables */
}

> void __weak prescaler(void)
> {
> 	u32 ticks = get_ticks();
                     /* Bill's algorithm */
                    /* result stored in timestamp; */
> }
>
> u32 get_timer(u32 base)
> {
#if defined(CONFIG_SYS_NEED_PRESCALER)
> 	prescaler();
#endif
> 	return timestamp - base;
> }
>
> In /arch/cpu/soc/timer.c or /arch/cpu/timer.c or /board/<board>/timer.c
>
> u32 get_ticks(void)
> {
> 	u32 ticks;
>
> 	/* Get ticks from hardware counter */
>
> 	return ticks;
> }
>
> u32 get_tick_frequency(void)
> {
> 	u32 tick_frequency;
>
> 	/* Determine tick frequency - likely very trivial */
>
> 	return tick_frequency;
> }
or instead the user may override prescaler_init and prescaler if, for 
some reason, a highly optimized version is desired. Note also that if 
the configuration variable CONFIG_SYS_NEED_PRESCALER is not defined, the 
additional prescaler routines will not be called anywhere so the 
routines should not be loaded. Yes, it it a #define to manage, but it 
should allow the existing u-boots to be the same size as before, with no 
unused code. This size matters to some people a lot!
> =======================
> Not exposing ticks and tick_frequency to everyone
>
> In /lib/timer.c
>
> void prescaler(u32 ticks, u32 tick_frequency)
> {
> 	u32 current_ms;
>
> 	/* Bill's algorithm */
>
> 	/* result stored in gd->timer_in_ms; */
> }
>
> In /arch/cpu/soc/timer.c or /arch/cpu/timer.c or /board/<board>/timer.c
>
> static u32 get_ticks(void)
> {
> 	u32 ticks;
>
> 	/* Get ticks from hardware counter */
>
> 	return ticks;
> }
>
> static u32 get_tick_frequency(void)
> {
> 	u32 tick_frequency;
>
> 	/* Determine tick frequency */
>
> 	return tick_frequency;
> }
>
> u32 get_timer(u32 base)
> {
> 	u32 ticks = get_ticks();
> 	u32 tick_frequency = get_tick_frequency();
>
> 	prescaler(ticks, tick_frequency);
>
> 	return gd->timer_in_ms - base;
> }
>
> ===============
> I personally prefer the first - There is only one implementation of
> get_timer() in the entire code and the platform implementer never has to
> concern themselves with what the tick counter is used for. If the API gets
> extended to include get_timer_in_seconds() there is ZERO impact on
> platforms. Using the second method, any new feature would have to be
> implemented on all platforms - and we all know how well that works ;)
>
> And what about those few platforms that are actually capable of generating
> a 1ms timebase (either via interrupts or natively in a hardware counter)
> without the prescaler? Well, with prescaler() declared weak, all you need
> to do in /arch/cpu/soc/timer.c or /arch/cpu/timer.c or
> /board/<board>/timer.c is:
>
> For platforms with a 1ms hardware counter:
> void prescaler(void /* or u32 ticks, u32 tick_frequency*/)
> {
> 	gd->timer_in_ms = get_milliseconds();
> }
>
> For platforms with a 1ms interrupt source:
> void timer_isr(void *unused)
> {
> 	gd->timer_in_ms++;
> }
>
> void prescaler(void /* or u32 ticks, u32 tick_frequency*/)
> {
> }
>
>
> And finally, if the platform supports interrupts but either the hardware
> counter has better accuracy than the interrupt generator or the interrupt
> generator cannot generate 1ms interrupts, configure the interrupt generator
> to fire at any rate better than the tick counter rollover listed in
> previous post and:
>
> void timer_isr(void *unused)
> {
> 	/*
> 	 * We are here to stop the tick counter rolling over. All we
> 	 * need to do is kick the prescaler - get_timer() does that :)
> 	 */
> 	get_timer(0);
> }
>
> In summary, platform specific code reduces to:
>   - For a platform that cannot generate 1ms interrupts AND the hardware
>     counter is not in ms
>     - Implement get_ticks() and get_tick_frequency()
>     - Optionally implement as ISR to kick the prescaler
>   - For a platform that can generate 1ms interrupts, or the hardware
>     counter is in ms
>     - Override the prescaler() function to do nothing
>
> If none of the above not look 'simple', I invite you to have a look in
> arch/arm ;)
>
> Regards,
>
> Graeme
>
>