[U-Boot] [PATCH] 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 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;

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

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

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 */

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.
participants (3)
-
Jean-Christophe PLAGNIOL-VILLARD
-
Stefan Althoefer
-
Wolfgang Denk