Skip to contentSkip to conversation

IXP425: Fixing timer code Part 1/1

PATCH New Stefan Althoefer 3 participants 5 messages Dec 4, 2008 – Jan 4, 2009

lore
Share & tools
lore

Stefan Althoefer

author
#0 lore raw

PATCH 1/1 IXP425: Fixing timer code Part 1/1

[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 <[email protected]>

----

cpu/ixp/timer.c renamed from /cpu/ixp/timer.c +9 -1
diff -uprN u-boot-orig//cpu/ixp/timer.c u-boot/cpu/ixp/timer.c
@@ -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;
#1 lore raw

PATCH 1/1 IXP425: Fixing timer code Part 1/1

Dear Stefan Althoefer,

Quoted 10 lines · In message <49384728.LMggwd1oTzMugOAp%[email protected]> 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

Signature · 5 lines
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.
author
#2 lore raw

PATCH 1/1 IXP425: Fixing timer code Part 1/1

Quoted 13 lines · Wolfgang Denk

Dear Stefan Althoefer,

Quoted 9 lines · In message <49384728.LMggwd1oTzMugOAp%[email protected]> 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

Quoted 7 lines

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

author
#3 lore raw

PATCH 1/1 IXP425: Fixing timer code Part 1/1

In reply to Wolfgang Denk

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 <[email protected]>
---
 cpu/ixp/timer.c |   64 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 15 deletions(-)
cpu/ixp/timer.c +49 -15
index 09d8ad5..d30adbc 100644
@@ -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 */
Signature · 1 line
1.5.6
#4 lore raw

PATCH 1/1 IXP425: Fixing timer code Part 1/1

Quoted 10 lines · 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).

Quoted 8 lines

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

Quoted 25 lines
Signed-off-by Stefan Althoefer <[email protected]>
---
 cpu/ixp/timer.c |   64 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 49 insertions(+), 15 deletions(-)
cpu/ixp/timer.c +6 -2
index 09d8ad5..d30adbc 100644
@@ -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)

cpu/ixp/timer.c line 54

Quoted 10 lines
cpu/ixp/timer.c +4 -1
+/* 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 '/'

cpu/ixp/timer.c void ixp425_udelay(unsigned long usec) · line 68

Quoted 13 lines
cpu/ixp/timer.c +5 -1
+		& ~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 '>'

cpu/ixp/timer.c void ixp425_udelay(unsigned long usec) · line 75

Quoted 7 lines
cpu/ixp/timer.c +5 -0
+		ixp425_udelay (1000);
+		usec -= 1000;
+	}
+	/* and now the rest */
+	ixp425_udelay (usec);
 }

Best Regards,
J.