[U-Boot] [PATCH] IXP425: Fixing timer code Part 1/1

[U-Boot] [PATCH] IXP425: Fixing timer code Part 1/1

[Stefan Althoefer]

[PATCH] IXP425: Fixing timer code

The non-interrupted timer code is inaccurate.

I found that the timing error of udelay() is caused by
to much looping overhead. The actual timing routine
was called repeatedly for each microsecond. When
I used bigger slices it became more accurate.

Some IXP425 ports have this line in their config file:

#define CONFIG_SYS_HZ   3333333    /* spec says 66.666 MHz,
                                but it appears to be 33 */

With this patch, this is nonsense. Instead you should use:

#ifdef CONFIG_USE_IRQ
/* Interrupt driven timer wants system tick here */
#define CONFIG_SYS_HZ                  1000
#else
/* The code in cpu/ixp/timer.c needs timer clock tick in HZ */
#define CONFIG_SYS_HZ                  66666666
#endif




The patch is against "latest" u-boot git-repository

Please (still) be patient if style of submission or patches are
offending.

Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
----

diff -uprN u-boot-orig//cpu/ixp/timer.c u-boot/cpu/ixp/timer.c
--- u-boot-orig//cpu/ixp/timer.c	2008-12-02 17:25:31.000000000 +0100
+++ u-boot/cpu/ixp/timer.c	2008-12-02 22:27:22.000000000 +0100
@@ -46,6 +46,8 @@ void ixp425_udelay(unsigned long usec)
 	 */
 	unsigned long usecs = CONFIG_SYS_HZ/1000000L & ~IXP425_OST_RELOAD_MASK;
 
+	usecs *= usec;
+
 	*IXP425_OSST = IXP425_OSST_TIMER_1_PEND;
 	usecs |= IXP425_OST_ONE_SHOT | IXP425_OST_ENABLE;
 	*IXP425_OSRT1 = usecs;
@@ -54,7 +56,13 @@ void ixp425_udelay(unsigned long usec)
 
 void udelay (unsigned long usec)
 {
-	while (usec--) ixp425_udelay(1);
+	/* ipx425_udelay has big overhead, so call it in bigger slices */
+	while (usec>1000){
+		ixp425_udelay(1000);
+		usec -= 1000;
+	}
+	/* and now the rest */
+	ixp425_udelay(usec);
 }
 
 static ulong reload_constant = 0xfffffff0;

[U-Boot] [PATCH] IXP425: Fixing timer code Part 1/1

[Wolfgang Denk]

Dear Stefan Althoefer,

In message <49384728.LMggwd1oTzMugOAp%stefan.althoefer@web.de> you wrote:
> 
> With this patch, this is nonsense. Instead you should use:
> 
> #ifdef CONFIG_USE_IRQ
> /* Interrupt driven timer wants system tick here */
> #define CONFIG_SYS_HZ                  1000
> #else
> /* The code in cpu/ixp/timer.c needs timer clock tick in HZ */
> #define CONFIG_SYS_HZ                  66666666
> #endif

No, this is wrong. CONFIG_SYS_HZ should always be 1000, without
exceptions.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Accident: A condition in which presence of mind is good, but  absence
of body is better.

[U-Boot] [PATCH] IXP425: Fixing timer code Part 1/1

[Stefan Althoefer]

Wolfgang Denk schrieb:
> Dear Stefan Althoefer,
> 
> In message <49384728.LMggwd1oTzMugOAp%stefan.althoefer@web.de> you wrote:
>> With this patch, this is nonsense. Instead you should use:
>>
>> #ifdef CONFIG_USE_IRQ
>> /* Interrupt driven timer wants system tick here */
>> #define CONFIG_SYS_HZ                  1000
>> #else
>> /* The code in cpu/ixp/timer.c needs timer clock tick in HZ */
>> #define CONFIG_SYS_HZ                  66666666
>> #endif
> 
> No, this is wrong. CONFIG_SYS_HZ should always be 1000, without
> exceptions.
> 
> Best regards,
> 
> Wolfgang Denk
> 

Should I then hide the 66666666 in the code, or define something like
"#define CONFIG_SYS_CLK_FREQ 66666666" ?

In /cpu/ixp/interrupts.c "#define FREQ 66666666" is used privately,
but I don't like this. However, this frequency is the same for all
IXP clock speeds so it does not really need to be in the config file.

-- Stefan

[U-Boot] [PATCH] IXP425: Fixing timer code Part 1/1

[Stefan Althoefer]

This fixes bugs in non-interupt timer code for IXP425

Fixed udelay() implementation to be more accurate.
The timing error of udelay() was caused by
too much looping overhead. The actual timing routine
was called repeatedly for each microsecond. When
bigger slices are used, it becomes more accurate.
CONFIG_SYS_TIMER_CLK_FREQ can be set by config file
if clock speed differs from IXP425 default (66,666MHz).

Fixed get_timer() to return correct time in units
of CONFIG_SYS_HZ. The code now works for times
in execess of 64 seconds (the timer overflow
limit), assumed get_timer() is called with a period
< 64 seconds.

do_sleep() now works correctly.

Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
---
 cpu/ixp/timer.c |   64 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/cpu/ixp/timer.c b/cpu/ixp/timer.c
index 09d8ad5..d30adbc 100644
--- a/cpu/ixp/timer.c
+++ b/cpu/ixp/timer.c
@@ -33,18 +33,28 @@
 #include <asm/arch/ixp425.h>

 #ifndef CONFIG_USE_IRQ
+
+#ifndef CONFIG_SYS_TIMER_CLK_FREQ
+#define CONFIG_SYS_TIMER_CLK_FREQ 66666666
+#endif
+
 ulong get_timer (ulong base)
 {
-       return get_timer_masked () - base;
+	return get_timer_masked () - base;
 }

-void ixp425_udelay(unsigned long usec)
+/* udelay uses timer 1 */
+
+void ixp425_udelay (unsigned long usec)
 {
 	/*
 	 * This function has a max usec, but since it is called from udelay
 	 * we should not have to worry... be happy
 	 */
-	unsigned long usecs = CONFIG_SYS_HZ/1000000L & ~IXP425_OST_RELOAD_MASK;
+	unsigned long usecs = CONFIG_SYS_TIMER_CLK_FREQ/1000000L
+		& ~IXP425_OST_RELOAD_MASK;
+
+	usecs *= usec;

 	*IXP425_OSST = IXP425_OSST_TIMER_1_PEND;
 	usecs |= IXP425_OST_ONE_SHOT | IXP425_OST_ENABLE;
@@ -54,30 +64,54 @@ void ixp425_udelay(unsigned long usec)

 void udelay (unsigned long usec)
 {
-	while (usec--) ixp425_udelay(1);
+	/* ipx425_udelay has big overhead, so call it in bigger slices */
+	while (usec>1000) {
+		ixp425_udelay (1000);
+		usec -= 1000;
+	}
+	/* and now the rest */
+	ixp425_udelay (usec);
 }

-static ulong reload_constant = 0xfffffff0;
+/* get_timer uses timer 2 */
+
+static ulong	reload_constant = 0xfffffff0;
+static int	timer_initialized = 0;
+static int	timer_accum;

 void reset_timer_masked (void)
 {
-	ulong reload = reload_constant | IXP425_OST_ONE_SHOT | IXP425_OST_ENABLE;
+	ulong reload = reload_constant | IXP425_OST_ENABLE;

-	*IXP425_OSST = IXP425_OSST_TIMER_1_PEND;
-	*IXP425_OSRT1 = reload;
+	*IXP425_OSST = IXP425_OSST_TIMER_2_PEND;
+	*IXP425_OSRT2 = reload;
 }

 ulong get_timer_masked (void)
 {
+	ulong current = *IXP425_OST2;
+	ulong this;
+
+	if (!timer_initialized) {
+		reset_timer_masked ();
+		timer_accum = 0;
+		timer_initialized = 1;
+	}
+
 	/*
-	 * Note that it is possible for this to wrap!
-	 * In this case we return max.
+	 * Timer will wrap after 64 seconds. This will
+	 * be detected if get_timer is called within
+	 * the next 64 seconds.
 	 */
-	ulong current = *IXP425_OST1;
-	if (*IXP425_OSST & IXP425_OSST_TIMER_1_PEND)
-	{
-		return reload_constant;
+	current = *IXP425_OST2;
+	if (*IXP425_OSST & IXP425_OSST_TIMER_2_PEND) {
+		*IXP425_OSST = IXP425_OSST_TIMER_2_PEND;
+		current = *IXP425_OST2;
+		timer_accum += reload_constant /
+			(CONFIG_SYS_TIMER_CLK_FREQ / CONFIG_SYS_HZ);
 	}
-	return (reload_constant - current);
+	this = (reload_constant - current) /
+		(CONFIG_SYS_TIMER_CLK_FREQ / CONFIG_SYS_HZ);
+	return (this + timer_accum);
 }
 #endif /* #ifndef CONFIG_USE_IRQ */
-- 
1.5.6

[U-Boot] [PATCH] IXP425: Fixing timer code Part 1/1

[Jean-Christophe PLAGNIOL-VILLARD]

On 20:08 Sun 21 Dec     , Stefan Althoefer wrote:
> This fixes bugs in non-interupt timer code for IXP425
> 
> Fixed udelay() implementation to be more accurate.
> The timing error of udelay() was caused by
> too much looping overhead. The actual timing routine
> was called repeatedly for each microsecond. When
> bigger slices are used, it becomes more accurate.
> CONFIG_SYS_TIMER_CLK_FREQ can be set by config file
> if clock speed differs from IXP425 default (66,666MHz).
> 
> Fixed get_timer() to return correct time in units
> of CONFIG_SYS_HZ. The code now works for times
> in execess of 64 seconds (the timer overflow
> limit), assumed get_timer() is called with a period
> < 64 seconds.
> 
> do_sleep() now works correctly.
> 

some general comments
please check the coding style
and please use the readx/writex accessors

> Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
> ---
>  cpu/ixp/timer.c |   64 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/cpu/ixp/timer.c b/cpu/ixp/timer.c
> index 09d8ad5..d30adbc 100644
> --- a/cpu/ixp/timer.c
> +++ b/cpu/ixp/timer.c
> @@ -33,18 +33,28 @@
>  #include <asm/arch/ixp425.h>
> 
>  #ifndef CONFIG_USE_IRQ
> +
> +#ifndef CONFIG_SYS_TIMER_CLK_FREQ
> +#define CONFIG_SYS_TIMER_CLK_FREQ 66666666
> +#endif
> +
>  ulong get_timer (ulong base)
>  {
> -       return get_timer_masked () - base;
> +	return get_timer_masked () - base;
>  }
> 
> -void ixp425_udelay(unsigned long usec)
> +/* udelay uses timer 1 */
> +
> +void ixp425_udelay (unsigned long usec)
>  {
>  	/*
>  	 * This function has a max usec, but since it is called from udelay
>  	 * we should not have to worry... be happy
>  	 */
> -	unsigned long usecs = CONFIG_SYS_HZ/1000000L & ~IXP425_OST_RELOAD_MASK;
> +	unsigned long usecs = CONFIG_SYS_TIMER_CLK_FREQ/1000000L
please add a space before and after '/'
> +		& ~IXP425_OST_RELOAD_MASK;
> +
> +	usecs *= usec;
> 
>  	*IXP425_OSST = IXP425_OSST_TIMER_1_PEND;
>  	usecs |= IXP425_OST_ONE_SHOT | IXP425_OST_ENABLE;
> @@ -54,30 +64,54 @@ void ixp425_udelay(unsigned long usec)
> 
>  void udelay (unsigned long usec)
>  {
> -	while (usec--) ixp425_udelay(1);
> +	/* ipx425_udelay has big overhead, so call it in bigger slices */
> +	while (usec>1000) {
please add a space before and after '>'
> +		ixp425_udelay (1000);
> +		usec -= 1000;
> +	}
> +	/* and now the rest */
> +	ixp425_udelay (usec);
>  }
> 
Best Regards,
J.