* [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code [not found] <7ee75976e78e2f82b4163fe1ff4233a850d4393c.1266966938.git.matthias@kaehlcke.net> @ 2010-02-23 23:22 ` Matthias Kaehlcke 2010-02-25 15:54 ` Tom 0 siblings, 1 reply; 6+ messages in thread From: Matthias Kaehlcke @ 2010-02-23 23:22 UTC (permalink / raw) To: u-boot 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(); -- 1.6.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code 2010-02-23 23:22 ` [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code Matthias Kaehlcke @ 2010-02-25 15:54 ` Tom 2010-02-25 18:15 ` Matthias Kaehlcke 0 siblings, 1 reply; 6+ messages in thread From: Tom @ 2010-02-25 15:54 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code 2010-02-25 15:54 ` Tom @ 2010-02-25 18:15 ` Matthias Kaehlcke 2010-02-25 19:22 ` Alessandro Rubini 2010-02-26 0:13 ` Tom 0 siblings, 2 replies; 6+ messages in thread From: Matthias Kaehlcke @ 2010-02-25 18:15 UTC (permalink / raw) To: u-boot 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 :( -- Matthias Kaehlcke Embedded Linux Developer Barcelona Someone has said that it requires less mental effort to condemn than to think (Emma Goldman) .''`. using free software / Debian GNU/Linux | http://debian.org : :' : `. `'` gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `- ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code 2010-02-25 18:15 ` Matthias Kaehlcke @ 2010-02-25 19:22 ` Alessandro Rubini 2010-02-25 19:47 ` Matthias Kaehlcke 2010-02-26 0:13 ` Tom 1 sibling, 1 reply; 6+ messages in thread From: Alessandro Rubini @ 2010-02-25 19:22 UTC (permalink / raw) To: u-boot > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code 2010-02-25 19:22 ` Alessandro Rubini @ 2010-02-25 19:47 ` Matthias Kaehlcke 0 siblings, 0 replies; 6+ messages in thread From: Matthias Kaehlcke @ 2010-02-25 19:47 UTC (permalink / raw) To: u-boot 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 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. 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 Embedded Linux Developer Barcelona We build too many walls and not enough bridges (Isaac Newton) .''`. using free software / Debian GNU/Linux | http://debian.org : :' : `. `'` gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `- ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code 2010-02-25 18:15 ` Matthias Kaehlcke 2010-02-25 19:22 ` Alessandro Rubini @ 2010-02-26 0:13 ` Tom 1 sibling, 0 replies; 6+ messages in thread From: Tom @ 2010-02-26 0:13 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-26 0:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <7ee75976e78e2f82b4163fe1ff4233a850d4393c.1266966938.git.matthias@kaehlcke.net>
2010-02-23 23:22 ` [U-Boot] [PATCH 2/2 rev2] ep93xx: Refactoring of timer code Matthias Kaehlcke
2010-02-25 15:54 ` Tom
2010-02-25 18:15 ` Matthias Kaehlcke
2010-02-25 19:22 ` Alessandro Rubini
2010-02-25 19:47 ` Matthias Kaehlcke
2010-02-26 0:13 ` Tom
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox