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

[Graeme Russ]

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

> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>  arch/arm/cpu/armv7/tegra2/timer.c        |   27 +++++++++++++++++------
>  arch/arm/include/asm/arch-tegra2/timer.h |   34 ++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 7 deletions(-)
>  create mode 100644 arch/arm/include/asm/arch-tegra2/timer.h
> 
> diff --git a/arch/arm/cpu/armv7/tegra2/timer.c b/arch/arm/cpu/armv7/tegra2/timer.c
> index fb061d0..b69c172 100644
> --- a/arch/arm/cpu/armv7/tegra2/timer.c
> +++ b/arch/arm/cpu/armv7/tegra2/timer.c
> @@ -38,13 +38,12 @@
>  #include <common.h>
>  #include <asm/io.h>
>  #include <asm/arch/tegra2.h>
> +#include <asm/arch/timer.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE;
> -
>  /* counter runs at 1MHz */
> -#define TIMER_CLK	(1000000)
> +#define TIMER_CLK	1000000
>  #define TIMER_LOAD_VAL	0xffffffff
>  
>  /* timer without interrupts */
> @@ -67,10 +66,10 @@ void set_timer(ulong t)
>  void __udelay(unsigned long usec)
>  {
>  	long tmo = usec * (TIMER_CLK / 1000) / 1000;
> -	unsigned long now, last = readl(&timer_base->cntr_1us);
> +	unsigned long now, last = timer_get_us();
>  
>  	while (tmo > 0) {
> -		now = readl(&timer_base->cntr_1us);
> +		now = timer_get_us();
>  		if (last > now) /* count up timer overflow */
>  			tmo -= TIMER_LOAD_VAL - last + now;
>  		else
> @@ -82,7 +81,7 @@ void __udelay(unsigned long usec)
>  void reset_timer_masked(void)
>  {
>  	/* 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?

>  	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

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

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

> +
> +#endif
> +

Regards,

Graeme