| Message ID | CAJtrzLNz3TBBRHoNtHkuGci4YJZ9PLrJfvGaR_XdVbrdcprxbQ@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
> Dear Wolfgang Denk, > > Thank you for your response. > > >> Since the .rel.text section is required by the relocation code, I > >> assume that .bss global variables cannot be used until after > >> relocation? > > > > Why do you have to make such assumptions? That's documented > > behaviour. Didn't you RTFM? > > I thought I had done a thorough search previously but after receiving > your response I managed to find some of the details outlined in the > README file. > > I have attached an updated patch below which hopefully addresses the > other issues you highlighted. Hehe, you should RTFM ;-) http://www.denx.de/wiki/U-Boot/Patches Generally, use git send-email to send the patch. > > Kind Regards, > > Mark Norman > > > > The s3c24x0 timer has been updated to use the global_data struct. > Restructured code based on other timer.c files. > Updated comments and several parameters. > > Signed-off-by: Mark Norman <mpnorman@gmail.com> > --- > arch/arm/cpu/arm920t/s3c24x0/timer.c | 155 > +++++++++++++-------------------- arch/arm/include/asm/global_data.h | > 4 + > 2 files changed, 65 insertions(+), 94 deletions(-) > > diff --git a/arch/arm/cpu/arm920t/s3c24x0/timer.c > b/arch/arm/cpu/arm920t/s3c24x0/timer.c > index 9571870..5695c62 100644 > --- a/arch/arm/cpu/arm920t/s3c24x0/timer.c > +++ b/arch/arm/cpu/arm920t/s3c24x0/timer.c > @@ -35,142 +35,109 @@ > #include <asm/io.h> > #include <asm/arch/s3c24x0_cpu.h> > > -int timer_load_val = 0; > -static ulong timer_clk; > +DECLARE_GLOBAL_DATA_PTR; > > -/* macro to read the 16 bit timer */ > -static inline ulong READ_TIMER(void) > +/* Read the 16 bit timer */ > +static inline ulong read_timer(void) > { > struct s3c24x0_timers *timers = s3c24x0_get_base_timers(); > > return readl(&timers->tcnto4) & 0xffff; > } > > -static ulong timestamp; > -static ulong lastdec; > - > int timer_init(void) > { > + /* > + * PWM Timer 4 is used because it has no output. > + * Prescaler is hard fixed at 250, divider at 2. > + * This generates a Timer clock frequency of 100kHz (@PCLK=50MHz) and > + * therefore 10us timer ticks. > + */ > + const ulong prescaler = 250; > struct s3c24x0_timers *timers = s3c24x0_get_base_timers(); > ulong tmr; > > - /* use PWM Timer 4 because it has no output */ > - /* prescaler for Timer 4 is 16 */ > - writel(0x0f00, &timers->tcfg0); > - if (timer_load_val == 0) { > - /* > - * for 10 ms clock period @ PCLK with 4 bit divider = 1/2 > - * (default) and prescaler = 16. Should be 10390 > - * @33.25MHz and 15625 @ 50 MHz > - */ > - timer_load_val = get_PCLK() / (2 * 16 * 100); > - timer_clk = get_PCLK() / (2 * 16); > - } > - /* load value for 10 ms timeout */ > - lastdec = timer_load_val; > - writel(timer_load_val, &timers->tcntb4); > - /* auto load, manual update of timer 4 */ > + /* Set prescaler for Timer 4 */ > + writel((prescaler-1) << 8, &timers->tcfg0); Can we get rid of the magic numbers please? > + > + /* Calculate timer freq, approx 100kHz @ PCLK=50MHz. */ > + gd->timer_rate_hz = get_PCLK() / (2 * prescaler); Can get_PCLK() be renamed to be lower-case only? > + > + /* Set timer for 0.5s timeout (50000 ticks @ 10us ticks). */ > + gd->timer_reset_value = 50000; > + writel(gd->timer_reset_value, &timers->tcntb4); > + gd->lastdec = gd->timer_reset_value; > + > + /* Load the initial timer 4 count value using the manual update bit. */ > tmr = (readl(&timers->tcon) & ~0x0700000) | 0x0600000; Magic values :-( > writel(tmr, &timers->tcon); > - /* auto load, start timer 4 */ > + > + /* Configure timer 4 for auto reload and start it. */ > tmr = (tmr & ~0x0700000) | 0x0500000; > writel(tmr, &timers->tcon); > - timestamp = 0; > + > + gd->timestamp = 0; > > return (0); return 0; if you're cleaning this crap up, do it all at once ;-) > } > > /* > - * timer without interrupts > + * Get the number of ticks (in CONFIG_SYS_HZ resolution) > */ > -ulong get_timer(ulong base) > +unsigned long long get_ticks(void) > { > - return get_timer_masked() - base; > + return get_timer(0); > } > > -void __udelay (unsigned long usec) > +unsigned long get_timer_raw(void) > { > - ulong tmo; > - ulong start = get_ticks(); > + ulong now = read_timer(); > > - tmo = usec / 1000; > - tmo *= (timer_load_val * 100); > - tmo /= 1000; > + if (gd->lastdec >= now) { > + /* normal mode */ > + gd->timestamp += gd->lastdec - now; > + } else { > + /* we have an overflow ... */ > + gd->timestamp += gd->lastdec + gd->timer_reset_value - now; > + } > + gd->lastdec = now; > > - while ((ulong) (get_ticks() - start) < tmo) > - /*NOP*/; > + return gd->timestamp; > } > > -ulong get_timer_masked(void) > +/* > + * This function is derived from PowerPC code (timebase clock frequency). > + * On ARM it returns the number of timer ticks per second. > + */ > +ulong get_tbclk(void) > { > - ulong tmr = get_ticks(); > - > - return tmr / (timer_clk / CONFIG_SYS_HZ); > + return CONFIG_SYS_HZ; > } > > -void udelay_masked(unsigned long usec) > +ulong get_timer_masked(void) > { > - ulong tmo; > - ulong endtime; > - signed long diff; > - > - if (usec >= 1000) { > - tmo = usec / 1000; > - tmo *= (timer_load_val * 100); > - tmo /= 1000; > - } else { > - tmo = usec * (timer_load_val * 100); > - tmo /= (1000 * 1000); > - } > + unsigned long tmr = get_timer_raw(); > > - endtime = get_ticks() + tmo; > + return (tmr * CONFIG_SYS_HZ) / gd->timer_rate_hz; > +} > > - do { > - ulong now = get_ticks(); > - diff = endtime - now; > - } while (diff >= 0); > +ulong get_timer(ulong base) > +{ > + return get_timer_masked() - base; > } > > -/* > - * This function is derived from PowerPC code (read timebase as long > long). - * On ARM it just returns the timer value. > - */ > -unsigned long long get_ticks(void) > +void __udelay(unsigned long usec) > { > - ulong now = READ_TIMER(); > + unsigned long tmp; > + unsigned long tmo; > > - if (lastdec >= now) { > - /* normal mode */ > - timestamp += lastdec - now; > - } else { > - /* we have an overflow ... */ > - timestamp += lastdec + timer_load_val - now; > - } > - lastdec = now; > + /* convert usec to ticks. */ > + tmo = ((gd->timer_rate_hz / 1000) * usec) / 1000; > > - return timestamp; > -} > + tmp = get_timer_raw() + tmo; /* get current timestamp */ > > -/* > - * This function is derived from PowerPC code (timebase clock frequency). > - * On ARM it returns the number of timer ticks per second. > - */ > -ulong get_tbclk(void) > -{ > - ulong tbclk; > - > -#if defined(CONFIG_SMDK2400) > - tbclk = timer_load_val * 100; > -#elif defined(CONFIG_SBC2410X) || \ > - defined(CONFIG_SMDK2410) || \ > - defined(CONFIG_S3C2440) || \ > - defined(CONFIG_VCMA9) > - tbclk = CONFIG_SYS_HZ; > -#else > -# error "tbclk not configured" > -#endif > - > - return tbclk; > + while (get_timer_raw() < tmp) /* loop till event */ > + /*NOP*/; > } > > /* > diff --git a/arch/arm/include/asm/global_data.h > b/arch/arm/include/asm/global_data.h > index fac98d5..b836915 100644 > --- a/arch/arm/include/asm/global_data.h > +++ b/arch/arm/include/asm/global_data.h > @@ -67,6 +67,10 @@ typedef struct global_data { > #ifdef CONFIG_IXP425 > unsigned long timestamp; > #endif > +#ifdef CONFIG_S3C24X0 > + unsigned long lastdec; > + unsigned long timestamp; > +#endif I wonder about this change ... > unsigned long relocaddr; /* Start address of U-Boot in RAM */ > phys_size_t ram_size; /* RAM size */ > unsigned long mon_len; /* monitor len */ Cheers
Dear Mark Norman, In message <CAJtrzLNz3TBBRHoNtHkuGci4YJZ9PLrJfvGaR_XdVbrdcprxbQ@mail.gmail.com> you wrote: > Dear Wolfgang Denk, > > Thank you for your response. > > >> =A0Since the .rel.text section is required by the relocation code, I > >> assume that .bss global variables cannot be used until after > >> relocation? > > > > Why do you have to make such assumptions? =A0That's documented > > behaviour. =A0Didn't you RTFM? > > > > I thought I had done a thorough search previously but after receiving > your response I managed to find some of the details outlined in the > README file. > > I have attached an updated patch below which hopefully addresses the > other issues you highlighted. It seems you read my message only halfway through. > Kind Regards, > > Mark Norman > > > > The s3c24x0 timer has been updated to use the global_data struct. > Restructured code based on other timer.c files. > Updated comments and several parameters. > > Signed-off-by: Mark Norman <mpnorman@gmail.com> > --- I wrote before: Then please submit a proper patch - these introductory comments don't belong into the commit message and should be moved into the comment section (below the "---" line). Best regards, Wolfgang Denk
Hi Marek Vasut, >> >> I have attached an updated patch below which hopefully addresses the >> other issues you highlighted. > > Hehe, you should RTFM ;-) http://www.denx.de/wiki/U-Boot/Patches > > Generally, use git send-email to send the patch. > Thanks for your feedback. I have submitted an updated patch using git send-email. I think I got the formatting right this time. :) >> >> The s3c24x0 timer has been updated to use the global_data struct. >> Restructured code based on other timer.c files. >> Updated comments and several parameters. >> >> Signed-off-by: Mark Norman <mpnorman@gmail.com> >> --- >> arch/arm/cpu/arm920t/s3c24x0/timer.c | 155 >> +++++++++++++-------------------- arch/arm/include/asm/global_data.h | >> 4 + >> 2 files changed, 65 insertions(+), 94 deletions(-) >> >> diff --git a/arch/arm/cpu/arm920t/s3c24x0/timer.c >> b/arch/arm/cpu/arm920t/s3c24x0/timer.c >> index 9571870..5695c62 100644 >> --- a/arch/arm/cpu/arm920t/s3c24x0/timer.c >> +++ b/arch/arm/cpu/arm920t/s3c24x0/timer.c >> @@ -35,142 +35,109 @@ >> #include <asm/io.h> >> #include <asm/arch/s3c24x0_cpu.h> >> >> -int timer_load_val = 0; >> -static ulong timer_clk; >> +DECLARE_GLOBAL_DATA_PTR; >> >> -/* macro to read the 16 bit timer */ >> -static inline ulong READ_TIMER(void) >> +/* Read the 16 bit timer */ >> +static inline ulong read_timer(void) >> { >> struct s3c24x0_timers *timers = s3c24x0_get_base_timers(); >> >> return readl(&timers->tcnto4) & 0xffff; >> } >> >> -static ulong timestamp; >> -static ulong lastdec; >> - >> int timer_init(void) >> { >> + /* >> + * PWM Timer 4 is used because it has no output. >> + * Prescaler is hard fixed at 250, divider at 2. >> + * This generates a Timer clock frequency of 100kHz (@PCLK=50MHz) and >> + * therefore 10us timer ticks. >> + */ >> + const ulong prescaler = 250; >> struct s3c24x0_timers *timers = s3c24x0_get_base_timers(); >> ulong tmr; >> >> - /* use PWM Timer 4 because it has no output */ >> - /* prescaler for Timer 4 is 16 */ >> - writel(0x0f00, &timers->tcfg0); >> - if (timer_load_val == 0) { >> - /* >> - * for 10 ms clock period @ PCLK with 4 bit divider = 1/2 >> - * (default) and prescaler = 16. Should be 10390 >> - * @33.25MHz and 15625 @ 50 MHz >> - */ >> - timer_load_val = get_PCLK() / (2 * 16 * 100); >> - timer_clk = get_PCLK() / (2 * 16); >> - } >> - /* load value for 10 ms timeout */ >> - lastdec = timer_load_val; >> - writel(timer_load_val, &timers->tcntb4); >> - /* auto load, manual update of timer 4 */ >> + /* Set prescaler for Timer 4 */ >> + writel((prescaler-1) << 8, &timers->tcfg0); > > Can we get rid of the magic numbers please? I have declared some bitfields to remove the magic numbers. > >> + >> + /* Calculate timer freq, approx 100kHz @ PCLK=50MHz. */ >> + gd->timer_rate_hz = get_PCLK() / (2 * prescaler); > > Can get_PCLK() be renamed to be lower-case only? I haven't addressed this in my latest patch as it would be quite a large change that I though might be worthy of its own patch (there are several related functions get_FCLK, get_HCLK, etc. which could be updated at the same time). >> diff --git a/arch/arm/include/asm/global_data.h >> b/arch/arm/include/asm/global_data.h >> index fac98d5..b836915 100644 >> --- a/arch/arm/include/asm/global_data.h >> +++ b/arch/arm/include/asm/global_data.h >> @@ -67,6 +67,10 @@ typedef struct global_data { >> #ifdef CONFIG_IXP425 >> unsigned long timestamp; >> #endif >> +#ifdef CONFIG_S3C24X0 >> + unsigned long lastdec; >> + unsigned long timestamp; >> +#endif > > I wonder about this change ... > The latest patch no longer creates new variables in the global_data struct and instead reuses some of the existing ones. Any more feedback would be welcomed. Cheers Mark Norman
diff --git a/arch/arm/cpu/arm920t/s3c24x0/timer.c b/arch/arm/cpu/arm920t/s3c24x0/timer.c index 9571870..5695c62 100644 --- a/arch/arm/cpu/arm920t/s3c24x0/timer.c +++ b/arch/arm/cpu/arm920t/s3c24x0/timer.c @@ -35,142 +35,109 @@ #include <asm/io.h> #include <asm/arch/s3c24x0_cpu.h> -int timer_load_val = 0; -static ulong timer_clk; +DECLARE_GLOBAL_DATA_PTR; -/* macro to read the 16 bit timer */ -static inline ulong READ_TIMER(void) +/* Read the 16 bit timer */ +static inline ulong read_timer(void) { struct s3c24x0_timers *timers = s3c24x0_get_base_timers(); return readl(&timers->tcnto4) & 0xffff; } -static ulong timestamp; -static ulong lastdec; - int timer_init(void) { + /* + * PWM Timer 4 is used because it has no output. + * Prescaler is hard fixed at 250, divider at 2. + * This generates a Timer clock frequency of 100kHz (@PCLK=50MHz) and + * therefore 10us timer ticks. + */ + const ulong prescaler = 250; struct s3c24x0_timers *timers = s3c24x0_get_base_timers(); ulong tmr; - /* use PWM Timer 4 because it has no output */ - /* prescaler for Timer 4 is 16 */ - writel(0x0f00, &timers->tcfg0); - if (timer_load_val == 0) { - /* - * for 10 ms clock period @ PCLK with 4 bit divider = 1/2 - * (default) and prescaler = 16. Should be 10390 - * @33.25MHz and 15625 @ 50 MHz - */ - timer_load_val = get_PCLK() / (2 * 16 * 100); - timer_clk = get_PCLK() / (2 * 16); - } - /* load value for 10 ms timeout */ - lastdec = timer_load_val; - writel(timer_load_val, &timers->tcntb4); - /* auto load, manual update of timer 4 */ + /* Set prescaler for Timer 4 */ + writel((prescaler-1) << 8, &timers->tcfg0); + + /* Calculate timer freq, approx 100kHz @ PCLK=50MHz. */ + gd->timer_rate_hz = get_PCLK() / (2 * prescaler); + + /* Set timer for 0.5s timeout (50000 ticks @ 10us ticks). */ + gd->timer_reset_value = 50000; + writel(gd->timer_reset_value, &timers->tcntb4); + gd->lastdec = gd->timer_reset_value; + + /* Load the initial timer 4 count value using the manual update bit. */ tmr = (readl(&timers->tcon) & ~0x0700000) | 0x0600000; writel(tmr, &timers->tcon); - /* auto load, start timer 4 */ + + /* Configure timer 4 for auto reload and start it. */ tmr = (tmr & ~0x0700000) | 0x0500000; writel(tmr, &timers->tcon); - timestamp = 0; + + gd->timestamp = 0; return (0); } /* - * timer without interrupts + * Get the number of ticks (in CONFIG_SYS_HZ resolution) */ -ulong get_timer(ulong base) +unsigned long long get_ticks(void) { - return get_timer_masked() - base; + return get_timer(0); } -void __udelay (unsigned long usec) +unsigned long get_timer_raw(void) { - ulong tmo; - ulong start = get_ticks(); + ulong now = read_timer(); - tmo = usec / 1000; - tmo *= (timer_load_val * 100); - tmo /= 1000; + if (gd->lastdec >= now) { + /* normal mode */ + gd->timestamp += gd->lastdec - now; + } else { + /* we have an overflow ... */ + gd->timestamp += gd->lastdec + gd->timer_reset_value - now; + } + gd->lastdec = now; - while ((ulong) (get_ticks() - start) < tmo) - /*NOP*/; + return gd->timestamp; } -ulong get_timer_masked(void) +/* + * This function is derived from PowerPC code (timebase clock frequency). + * On ARM it returns the number of timer ticks per second. + */ +ulong get_tbclk(void) { - ulong tmr = get_ticks(); - - return tmr / (timer_clk / CONFIG_SYS_HZ); + return CONFIG_SYS_HZ; } -void udelay_masked(unsigned long usec) +ulong get_timer_masked(void) { - ulong tmo; - ulong endtime; - signed long diff; - - if (usec >= 1000) { - tmo = usec / 1000; - tmo *= (timer_load_val * 100); - tmo /= 1000; - } else { - tmo = usec * (timer_load_val * 100); - tmo /= (1000 * 1000); - } + unsigned long tmr = get_timer_raw(); - endtime = get_ticks() + tmo; + return (tmr * CONFIG_SYS_HZ) / gd->timer_rate_hz; +} - do { - ulong now = get_ticks(); - diff = endtime - now; - } while (diff >= 0); +ulong get_timer(ulong base) +{ + return get_timer_masked() - base; } -/* - * This function is derived from PowerPC code (read timebase as long long). - * On ARM it just returns the timer value. - */ -unsigned long long get_ticks(void) +void __udelay(unsigned long usec) { - ulong now = READ_TIMER(); + unsigned long tmp; + unsigned long tmo; - if (lastdec >= now) { - /* normal mode */ - timestamp += lastdec - now; - } else { - /* we have an overflow ... */ - timestamp += lastdec + timer_load_val - now; - } - lastdec = now; + /* convert usec to ticks. */ + tmo = ((gd->timer_rate_hz / 1000) * usec) / 1000; - return timestamp; -} + tmp = get_timer_raw() + tmo; /* get current timestamp */ -/* - * This function is derived from PowerPC code (timebase clock frequency). - * On ARM it returns the number of timer ticks per second. - */ -ulong get_tbclk(void) -{ - ulong tbclk; - -#if defined(CONFIG_SMDK2400) - tbclk = timer_load_val * 100; -#elif defined(CONFIG_SBC2410X) || \ - defined(CONFIG_SMDK2410) || \ - defined(CONFIG_S3C2440) || \ - defined(CONFIG_VCMA9) - tbclk = CONFIG_SYS_HZ; -#else -# error "tbclk not configured" -#endif - - return tbclk; + while (get_timer_raw() < tmp) /* loop till event */ + /*NOP*/; } /* diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h index fac98d5..b836915 100644 --- a/arch/arm/include/asm/global_data.h +++ b/arch/arm/include/asm/global_data.h @@ -67,6 +67,10 @@ typedef struct global_data { #ifdef CONFIG_IXP425 unsigned long timestamp; #endif +#ifdef CONFIG_S3C24X0 + unsigned long lastdec; + unsigned long timestamp; +#endif unsigned long relocaddr; /* Start address of U-Boot in RAM */ phys_size_t ram_size; /* RAM size */ unsigned long mon_len; /* monitor len */