[U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code
ep93xx: Refactoring of the timer code, including the following changes
* use a free running timer instead of a periodical one * use unsigned long long for total number of ticks * hold the timer state in a structure instead of separate variables * increment the timer counter instead of decrementing it * remove unused function udelay_masked() * remove unused function set_timer()
Signed-off-by: Matthias Kaehlcke matthias@kaehlcke.net --- Changes with regard to rev1:
* use TIMER_FREQ directly instead of recalculating it with rounding errors
cpu/arm920t/ep93xx/timer.c | 70 ++++++++++++++++--------------------------- 1 files changed, 26 insertions(+), 44 deletions(-)
diff --git a/cpu/arm920t/ep93xx/timer.c b/cpu/arm920t/ep93xx/timer.c index 98c759c..b1a01a0 100644 --- a/cpu/arm920t/ep93xx/timer.c +++ b/cpu/arm920t/ep93xx/timer.c @@ -34,16 +34,18 @@ #include <div64.h>
#define TIMER_CLKSEL (1 << 3) -#define TIMER_MODE (1 << 6) #define TIMER_ENABLE (1 << 7)
-#define TIMER_FREQ 508469 -#define TIMER_LOAD_VAL (TIMER_FREQ / CONFIG_SYS_HZ) +#define TIMER_FREQ 508469 +#define TIMER_MAX_VAL 0xFFFFFFFF
-static ulong timestamp; -static ulong lastdec; +static struct ep93xx_timer +{ + unsigned long long ticks; + unsigned long last_update; +} timer;
-static inline unsigned long clk_to_systicks(unsigned long clk_ticks) +static inline unsigned long clk_to_systicks(unsigned long long clk_ticks) { unsigned long long sys_ticks = (clk_ticks * CONFIG_SYS_HZ); do_div(sys_ticks, TIMER_FREQ); @@ -57,10 +59,10 @@ static inline unsigned long usecs_to_ticks(unsigned long usecs)
if (usecs >= 1000) { ticks = usecs / 1000; - ticks *= (TIMER_LOAD_VAL * CONFIG_SYS_HZ); + ticks *= TIMER_FREQ; ticks /= 1000; } else { - ticks = usecs * TIMER_LOAD_VAL * CONFIG_SYS_HZ; + ticks = usecs * TIMER_FREQ; ticks /= (1000 * 1000); }
@@ -71,7 +73,7 @@ static inline unsigned long read_timer(void) { struct timer_regs *timer = (struct timer_regs *)TIMER_BASE;
- return readl(&timer->timer3.value); + return TIMER_MAX_VAL - readl(&timer->timer3.value); }
/* @@ -81,17 +83,15 @@ unsigned long long get_ticks(void) { const unsigned long now = read_timer();
- if (lastdec >= now) { - /* normal mode */ - timestamp += lastdec - now; - } else { - /* we have an overflow ... */ - timestamp += lastdec + TIMER_LOAD_VAL - now; - } + if (now >= timer.last_update) + timer.ticks += now - timer.last_update; + else + /* an overflow occurred */ + timer.ticks += TIMER_MAX_VAL - timer.last_update + now;
- lastdec = now; + timer.last_update = now;
- return timestamp; + return timer.ticks; }
unsigned long get_timer_masked(void) @@ -106,8 +106,8 @@ unsigned long get_timer(unsigned long base)
void reset_timer_masked(void) { - lastdec = read_timer(); - timestamp = 0; + timer.last_update = read_timer(); + timer.ticks = 0; }
void reset_timer(void) @@ -115,28 +115,11 @@ void reset_timer(void) reset_timer_masked(); }
-void set_timer(unsigned long t) -{ - timestamp = t; -} - void __udelay(unsigned long usec) { - const unsigned long ticks = usecs_to_ticks(usec); - const unsigned long target = clk_to_systicks(ticks) + get_timer(0); - - while (get_timer_masked() < target) - /* noop */; -} - -void udelay_masked(unsigned long usec) -{ - const unsigned long ticks = usecs_to_ticks(usec); - const unsigned long target = clk_to_systicks(ticks) + get_timer(0); - - reset_timer_masked(); + const unsigned long target = get_ticks() + usecs_to_ticks(usec);
- while (get_timer_masked() < target) + while (get_ticks() < target) /* noop */; }
@@ -147,12 +130,11 @@ int timer_init(void) /* use timer 3 with 508KHz and free running */ writel(TIMER_CLKSEL, &timer->timer3.control);
- /* auto load, manual update of Timer 3 */ - lastdec = TIMER_LOAD_VAL; - writel(TIMER_LOAD_VAL, &timer->timer3.load); + /* set initial timer value 3 */ + writel(TIMER_MAX_VAL, &timer->timer3.load);
- /* Enable the timer and periodic mode */ - writel(TIMER_ENABLE | TIMER_MODE | TIMER_CLKSEL, + /* Enable the timer */ + writel(TIMER_ENABLE | TIMER_CLKSEL, &timer->timer3.control);
reset_timer_masked();
Matthias Kaehlcke wrote:
ep93xx: Refactoring of the timer code, including the following changes
- use a free running timer instead of a periodical one
- use unsigned long long for total number of ticks
- hold the timer state in a structure instead of separate variables
- increment the timer counter instead of decrementing it
- remove unused function udelay_masked()
- remove unused function set_timer()
Signed-off-by: Matthias Kaehlcke matthias@kaehlcke.net
This set has been applied to ARM. Thanks Tom
Hi Tom,
El Thu, Feb 25, 2010 at 09:54:59AM -0600 Tom ha dit:
Matthias Kaehlcke wrote:
ep93xx: Refactoring of the timer code, including the following changes
- use a free running timer instead of a periodical one
- use unsigned long long for total number of ticks
- hold the timer state in a structure instead of separate variables
- increment the timer counter instead of decrementing it
- remove unused function udelay_masked()
- remove unused function set_timer()
Signed-off-by: Matthias Kaehlcke matthias@kaehlcke.net
This set has been applied to ARM.
i was/am working on a new version of the patch, taking into account your remarks about the unit of TIMER_FREQ and fixing some issues discussed with Alessandro Rubini off-list, who worked on a similar patch.
what do you prefer, a patch based on the ones you just applied, or revert them and get one with all fixes (only one patch, cause the function clk_to_systicks() fixed in the first patch of the set is removed)
i apologize for not having taken the time to notify about this and causing you overhead :(
i was/am working on a new version of the patch, taking into account your remarks about the unit of TIMER_FREQ and fixing some issues discussed with Alessandro Rubini off-list, who worked on a similar patch.
Actually, I checked the point we disagreed about, which is the unit of get_ticks() and get_tbclk(). You currently return hw-ticks in get_ticks, and CONFIG_SYS_HZ (i.e. 1000) in get_tbclk. However, these two functions are expected to be used together, so they must be consistent in their return value.
It's true that the functions are little used (they are mostly used in ppc code, within cpu/*/interrupts), and that's why I didn't even provide them in cpu/arm926ejs/nomadik/timer.c. All few users assume they are consistent, but there is no documentation:
tornado% grep -qr get_tbclk README* doc || echo not found not found tornado% grep -qr get_ticks README* doc/* || echo not found not found
I've made a quick tour of all definitions in cpu/ and here is the result. As you see, at91 (which you used as reference, I understand) is wrong, while all the others use either hwticks or SYS_HZ consistently.
source-file get_ticks() get_tbclk
arm920t/at91/timer.c get_timer() CONFIG_SYS_HZ arm920t/a320/timer.c get_timer() CONFIG_SYS_HZ arm920t/s3c24x0/timer.c hw-ticks (I didn't understand) arm920t/imx/timer.c get_timer() CONFIG_SYS_HZ arm920t/at91rm9200/timer.c get_timer() CONFIG_SYS_HZ pxa/timer.c hw-ticks-32bits TIMER_FREQ_HZ arm_cortexa8/omap3/timer.c get_timer() CONFIG_SYS_HZ arm_cortexa8/s5pc1xx/timer.c get_timer() CONFIG_SYS_HZ arm925t/timer.c get_timer() CONFIG_SYS_HZ ppc4xx/cpu.c lib_ppc/ticks.S freqProcessor blackfin/interrupts.c get_timer() CONFIG_SYS_HZ sa1100/timer.c get_timer() CONFIG_SYS_HZ mpc824x/cpu.c lib_ppc/ticks.S bus_freq mpc8xx/cpu.c lib_ppc/ticks.S timebase mpc512x/cpu.c lib_ppc/ticks.S timebase mpc5xxx/cpu.c lib_ppc/ticks.S timebase mpc8220/cpu.c lib_ppc/ticks.S timebase at32ap/interrupts.c hw-ticks cpu_hz mpc85xx/cpu.c lib_ppc/ticks.S timebase 74xx_7xx/cpu.c lib_ppc/ticks.S timebase arm1136/omap24xx/timer.c get_timer() CONFIG_SYS_HZ mpc86xx/cpu.c lib_ppc/ticks.S timebase mpc8260/cpu.c lib_ppc/ticks.S timebase arm926ejs/omap/timer.c get_timer() CONFIG_SYS_HZ arm926ejs/davinci/timer.c get_timer() CONFIG_SYS_HZ arm926ejs/versatile/timer.c get_timer() CONFIG_SYS_HZ arm926ejs/at91/timer.c hwticks CONFIG_SYS_HZ arm926ejs/spear/timer.c hwticks CONFIG_SYS_HZ arm1176/s3c64xx/timer.c hwrticks (I didn't understand) lh7a40x/timer.c get_timer (I didn't understand) mpc5xx/cpu.c lib_ppc/ticks.S timebase
/alessandro
Hi,
El Thu, Feb 25, 2010 at 08:22:59PM +0100 Alessandro Rubini ha dit:
i was/am working on a new version of the patch, taking into account your remarks about the unit of TIMER_FREQ and fixing some issues discussed with Alessandro Rubini off-list, who worked on a similar patch.
Actually, I checked the point we disagreed about, which is the unit of get_ticks() and get_tbclk(). You currently return hw-ticks in get_ticks, and CONFIG_SYS_HZ (i.e. 1000) in get_tbclk. However, these two functions are expected to be used together, so they must be consistent in their return value.
actually there is no disagreement between us, i totally agree with you that the return value of get_ticks() should be in CONFIG_SYS_HZ resolution and consistent with get_tbclk(). the patch i sent you yesterday off-list fixes exactly this.
It's true that the functions are little used (they are mostly used in ppc code, within cpu/*/interrupts), and that's why I didn't even provide them in cpu/arm926ejs/nomadik/timer.c. All few users assume they are consistent, but there is no documentation:
tornado% grep -qr get_tbclk README* doc || echo not found not found tornado% grep -qr get_ticks README* doc/* || echo not found not foundI've made a quick tour of all definitions in cpu/ and here is the result. As you see, at91 (which you used as reference, I understand) is wrong, while all the others use either hwticks or SYS_HZ consistently.
yes, i used precisely at91 as reference, i liked it's code structure and didn't notice that it is wrong in this point.
thanks for your research!
Matthias Kaehlcke wrote:
Hi Tom,
El Thu, Feb 25, 2010 at 09:54:59AM -0600 Tom ha dit:
Matthias Kaehlcke wrote:
ep93xx: Refactoring of the timer code, including the following changes
- use a free running timer instead of a periodical one
- use unsigned long long for total number of ticks
- hold the timer state in a structure instead of separate variables
- increment the timer counter instead of decrementing it
- remove unused function udelay_masked()
- remove unused function set_timer()
Signed-off-by: Matthias Kaehlcke matthias@kaehlcke.net
This set has been applied to ARM.
i was/am working on a new version of the patch, taking into account your remarks about the unit of TIMER_FREQ and fixing some issues discussed with Alessandro Rubini off-list, who worked on a similar patch.
what do you prefer, a patch based on the ones you just applied, or revert them and get one with all fixes (only one patch, cause the function clk_to_systicks() fixed in the first patch of the set is removed)
i apologize for not having taken the time to notify about this and causing you overhead :(
Its ok with me if you just rebase and submit a smaller patch. Tom
participants (3)
-
Alessandro Rubini -
Matthias Kaehlcke -
Tom