[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Simon Glass]

Hi Graeme,

On Sat, Jul 9, 2011 at 10:24 PM, Graeme Russ <graeme.russ at gmail.com> wrote:

> Hi Simon,
>
> On 06/07/11 02:49, Simon Glass wrote:
> > These functions provide access to the high resolution microsecond timer
> > and tidy up a global variable in the code.
>
> Excellent - Good to see microsecond timers making their way in already
>
> >
> >       /* reset time, capture current incrementer value time */
> > -     gd->lastinc = readl(&timer_base->cntr_1us) /
> (TIMER_CLK/CONFIG_SYS_HZ);
> > +     gd->lastinc = timer_get_us() / (TIMER_CLK/CONFIG_SYS_HZ);
>
> CONFIG_SYS_HZ is always supposed to be 1000 and in future I think it should
> be removed entirely - Can you clean this up to not us CONFIG_SYS_HZ?
>
> I will take a look.


> >       gd->tbl = 0;            /* start "advancing" time stamp from 0 */
> >  }
> >
> > @@ -91,7 +90,7 @@ ulong get_timer_masked(void)
> >       ulong now;
> >
> >       /* current tick value */
> > -     now = readl(&timer_base->cntr_1us) / (TIMER_CLK / CONFIG_SYS_HZ);
> > +     now = timer_get_us() / (TIMER_CLK / CONFIG_SYS_HZ);
>
> And here
>
> >
> >       if (now >= gd->lastinc) /* normal mode (non roll) */
> >               /* move stamp forward with absolute diff ticks */
> > @@ -120,3 +119,17 @@ ulong get_tbclk(void)
> >  {
> >       return CONFIG_SYS_HZ;
> >  }
>
> Hmmm, maybe all of the above should use get_tbclk()?
>
> > +
> > +
> > +unsigned long timer_get_us(void)
> > +{
> > +     struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE;
> > +
> > +     return readl(&timer_base->cntr_1us);
> > +}
>
> timer_get_us needs to be u64 (unsigned long long). Also, the new timer API
> will define this as time_now_us, would be great if you could use this
> naming convention now to save me doing a mass of replaces later
>

Yes will do.

>
> > +
> > +unsigned long timer_get_future_us(u32 delay)
> > +{
> > +     return timer_get_us() + delay;
> > +}
>
> C'mon - We've been here before - This is timer API stuff. Where are you
> going to use this? Why can't the proposed API be used instead?
>
> I know you don't like the 'time since' implementation, but this has been
> discussed to death and it appears to me that the majority decision was to
> go that route rather than the 'future time' route. It is a completely
> pointless exercise and a complete waste of my time to re-write the timer
> API just to have someone that doesn't like a particular aspect go off and
> write a one-off function.
>

Well this code pre-dates this and I haven't revised it. I will take another
look and sort this out. In fact from memory the return value isn't even
used!

>
> > +
> > diff --git a/arch/arm/include/asm/arch-tegra2/timer.h
> b/arch/arm/include/asm/arch-tegra2/timer.h
> > new file mode 100644
> > index 0000000..5d5445e
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-tegra2/timer.h
> > @@ -0,0 +1,34 @@
> > +/*
> > + * Copyright (c) 2011 The Chromium OS Authors.
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +/* Tegra2 timer functions */
> > +
> > +#ifndef _TEGRA2_TIMER_H
> > +#define _TEGRA2_TIMER_H
> > +
> > +/* returns the current monotonic timer value in microseconds */
> > +unsigned long timer_get_us(void);
> > +
> > +/* returns what the time will likely be some microseconds into the
> future */
> > +unsigned long timer_get_future_us(u32 delay);
>
> 'likely' meaning it may or may not - no guarantee though. The new timer API
> is designed specifically designed to resolve this - 'At least x ms/us have
> passed' or 'at most x ms/us have passed'. No more 'x ms/us _might_ have
> passed'
>

Yes, watch this space.

BTW I have come across another problem where initialization must be done
which has long delays in it (LCD display power-up sequence). It's starting
to feel like we should have a central place for registering a timer which
calls back when a time has expired. That way we can continue with other
tings while we wait for the time to elapse. Something like:


/* Move to the next state */
static int next_state(void *my_data)
{
/* do some things, and then if you want to be called again... */
timer_register(timer_now_ms() + 40, next_state, my_data)
}

void start_lcd_init()
{
// do first thing
...
// set a timer to do the next thing later
timer_register(timer_now_ms() + 200, next_state, my_data)
}

...

Every now and then code (or a timer wait function) can call

timer_check()

which will call any expired timers on the list and remove them. This allows
LCD init to continue while downloading the kernel, for example.


I haven't thought too hard about this, but apart from LCD I know that USB
has some big delays. Obviously we can't make U-Boot multi-threaded but we
can perhaps do something simple like the above. What do you think?

Also given that your timer API stuff seems to have a good reception and
people are very happy, is there any impediment to getting this in sooner
rather than later?

Regards,
Simon



> > +
> > +#endif
> > +
>
> Regards,
>
> Graeme
>