[U-Boot] [PATCH] Update s3c24x0 timer implementation

[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Mark Norman]

I have been doing some work to get U-Boot running on a samsung S3C2440
based SBC (QQ2440).  I experienced several issues getting the board
running including "raise: Signal # 8 caught" errors being printed to
the console.  After a bit of debugging with a JTAG debugger I found
the problems were due to arch/arm/cpu/arm920t/s3c24x0/timer.c using
several global variables prior to relocation to RAM.  I noticed that
the global variable .bss section is shared with the .rel.text section.
 Since the .rel.text section is required by the relocation code, I
assume that .bss global variables cannot be used until after
relocation?

After studying several other timer.c files I developed the following
patch which uses the global data struct to store the global variables.
 I also restructured some of the code based on structure of the other
timer.c files.  I have confirmed it works correctly on the SBC I have.

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 |  158 +++++++++++++--------------------
 arch/arm/include/asm/global_data.h   |   12 +--
 2 files changed, 66 insertions(+), 104 deletions(-)

diff --git a/arch/arm/cpu/arm920t/s3c24x0/timer.c
b/arch/arm/cpu/arm920t/s3c24x0/timer.c
index 9571870..1552345 100644
--- a/arch/arm/cpu/arm920t/s3c24x0/timer.c
+++ b/arch/arm/cpu/arm920t/s3c24x0/timer.c
@@ -35,146 +35,112 @@
 #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)
 {
 	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 */
+	/* Use PWM Timer 4 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.
+	 */
+
+	/* Prescaler for Timer 4 is 250 */
+	const ulong prescaler = 250;
+	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*/;
 }

 /*
- * reset the cpu by setting up the watchdog timer and let him time out
+ * Reset the cpu by setting up the watchdog timer and let him time out
  */
 void reset_cpu(ulong ignored)
 {
diff --git a/arch/arm/include/asm/global_data.h
b/arch/arm/include/asm/global_data.h
index fac98d5..a20a9f7 100644
--- a/arch/arm/include/asm/global_data.h
+++ b/arch/arm/include/asm/global_data.h
@@ -38,9 +38,6 @@ typedef	struct	global_data {
 	unsigned long	flags;
 	unsigned long	baudrate;
 	unsigned long	have_console;	/* serial_init() was called */
-#ifdef CONFIG_PRE_CONSOLE_BUFFER
-	unsigned long	precon_buf_idx;	/* Pre-Console buffer index */
-#endif
 	unsigned long	env_addr;	/* Address  of Environment struct */
 	unsigned long	env_valid;	/* Checksum of Environment valid? */
 	unsigned long	fb_base;	/* base address of frame buffer */
@@ -67,6 +64,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 */
@@ -78,11 +79,6 @@ typedef	struct	global_data {
 #endif
 	void		**jt;		/* jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
-#if defined(CONFIG_POST) || defined(CONFIG_LOGBUFFER)
-	unsigned long	post_log_word; /* Record POST activities */
-	unsigned long	post_log_res; /* success of POST test */
-	unsigned long	post_init_f_time; /* When post_init_f started */
-#endif
 } gd_t;

 /*
-- 
1.7.1

[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Wolfgang Denk]

Dear Mark Norman,

In message <CAJtrzLOvjswmuww+xNLXxeSH4QxOBZww3JrZGKCgLq5w6B=y3w@mail.gmail.com> you wrote:
>
>  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?

> After studying several other timer.c files I developed the following
> patch which uses the global data struct to store the global variables.
>  I also restructured some of the code based on structure of the other
> timer.c files.  I have confirmed it works correctly on the SBC I have.

Then please submit a proper patch - these introductury comments don;lt
belong intot he commit message and shouldbemoved into the comment
section (below the "---" line).


...
> +	/* Use PWM Timer 4 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.
> +	 */

Incorrect multiline comment format; please fix globally.

> +	/* Prescaler for Timer 4 is 250 */
> +	const ulong prescaler = 250;
> +	writel((prescaler-1) << 8, &timers->tcfg0);

Please move declarations up.  Don't split declarations by comment
lines.  Add a blank line between declarations and code.

...
> --- a/arch/arm/include/asm/global_data.h
> +++ b/arch/arm/include/asm/global_data.h
> @@ -38,9 +38,6 @@ typedef	struct	global_data {
>  	unsigned long	flags;
>  	unsigned long	baudrate;
>  	unsigned long	have_console;	/* serial_init() was called */
> -#ifdef CONFIG_PRE_CONSOLE_BUFFER
> -	unsigned long	precon_buf_idx;	/* Pre-Console buffer index */
> -#endif

Make sure not to add such unrelated, incorrect changes!

> -#if defined(CONFIG_POST) || defined(CONFIG_LOGBUFFER)
> -	unsigned long	post_log_word; /* Record POST activities */
> -	unsigned long	post_log_res; /* success of POST test */
> -	unsigned long	post_init_f_time; /* When post_init_f started */
> -#endif

Ditto.

Best regards,

Wolfgang Denk

-- 
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
I think there's a world market for about five computers.
         -- attr. Thomas J. Watson (Chairman of the Board, IBM), 1943

[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Mark Norman]

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.

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);
+
+	/* 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 */
-- 
1.7.1

[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Marek Vasut]

> 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

[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Mark Norman]

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

[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Wolfgang Denk]

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

-- 
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
Sometimes a feeling is all we humans have to go on.
	-- Kirk, "A Taste of Armageddon", stardate 3193.9

[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Mark Norman]

The s3c24x0 timer has been updated to avoid using static variables prior
to BSS being made available.
Restructured code based on other timer.c files.
Updated comments and several parameters.

Signed-off-by: Mark Norman <mpnorman@gmail.com>
---
Changes for v2:
   - Fixed multi-line comment format
   - Formatting updates to separate declarations from code
   - Removed unrelated changes accidentally included in original patch
Changes for v3:
   - Added bitfield declarations to avoid using magic numbers.
   - Change to use existing global_data variables instead of creating new
     ones.

 arch/arm/cpu/arm920t/s3c24x0/timer.c |  180 ++++++++++++++++------------------
 1 files changed, 85 insertions(+), 95 deletions(-)

diff --git a/arch/arm/cpu/arm920t/s3c24x0/timer.c b/arch/arm/cpu/arm920t/s3c24x0/timer.c
index 9571870..8e1b935 100644
--- a/arch/arm/cpu/arm920t/s3c24x0/timer.c
+++ b/arch/arm/cpu/arm920t/s3c24x0/timer.c
@@ -35,116 +35,93 @@
 #include <asm/io.h>
 #include <asm/arch/s3c24x0_cpu.h>
 
-int timer_load_val = 0;
-static ulong timer_clk;
+/* Timer Control Register (TCON) bitfields */
+#define TCON_TMR4_AUTO_RELOAD (1<<22)
+#define TCON_TMR4_MAN_UPDATE  (1<<21)
+#define TCON_TMR4_START       (1<<20)
+#define TCON_TMR4_MASK        (TCON_TMR4_AUTO_RELOAD | TCON_TMR4_MAN_UPDATE | \
+				TCON_TMR4_START)
 
-/* macro to read the 16 bit timer */
-static inline ulong READ_TIMER(void)
+/* Timer Configuration Register 0 (TCFG0) bitfields */
+#define TCFG0_PRESCALER1(x)   (((x) & 0xff) << 8)
+
+/* Watchdog Timer Control Register (WTCON) bitfields */
+#define WTCON_TIMER_EN        (1<<5)
+#define WTCON_RESET_EN        (1<<0)
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define timestamp          (gd->tbl)
+#define lastdec            (gd->lastinc)
+#define timer_rate_hz      (gd->timer_rate_hz)
+#define timer_reset_value  (gd->timer_reset_value)
+
+/* 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;
+	const ulong divider = 2;
 	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 */
-	tmr = (readl(&timers->tcon) & ~0x0700000) | 0x0600000;
-	writel(tmr, &timers->tcon);
-	/* auto load, start timer 4 */
-	tmr = (tmr & ~0x0700000) | 0x0500000;
-	writel(tmr, &timers->tcon);
-	timestamp = 0;
-
-	return (0);
-}
-
-/*
- * timer without interrupts
- */
-ulong get_timer(ulong base)
-{
-	return get_timer_masked() - base;
-}
-
-void __udelay (unsigned long usec)
-{
-	ulong tmo;
-	ulong start = get_ticks();
+	/* Set prescaler for Timer 4 */
+	writel(TCFG0_PRESCALER1(prescaler-1), &timers->tcfg0);
 
-	tmo = usec / 1000;
-	tmo *= (timer_load_val * 100);
-	tmo /= 1000;
+	/* Calculate timer freq, approx 100kHz @ PCLK=50MHz. */
+	timer_rate_hz = get_PCLK() / (divider * prescaler);
 
-	while ((ulong) (get_ticks() - start) < tmo)
-		/*NOP*/;
-}
-
-ulong get_timer_masked(void)
-{
-	ulong tmr = get_ticks();
+	/* Set timer for 0.5s timeout (50000 ticks @ 10us ticks). */
+	timer_reset_value = 50000;
+	writel(timer_reset_value, &timers->tcntb4);
+	lastdec = timer_reset_value;
 
-	return tmr / (timer_clk / CONFIG_SYS_HZ);
-}
+	/* Load the initial timer 4 count value using the manual update bit. */
+	tmr = readl(&timers->tcon);
+	tmr &= ~TCON_TMR4_MASK;
+	tmr |= (TCON_TMR4_AUTO_RELOAD | TCON_TMR4_MAN_UPDATE);
+	writel(tmr, &timers->tcon);
 
-void udelay_masked(unsigned long usec)
-{
-	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);
-	}
+	/* Configure timer 4 for auto reload and start it. */
+	tmr &= ~TCON_TMR4_MASK;
+	tmr |= (TCON_TMR4_AUTO_RELOAD | TCON_TMR4_START);
+	writel(tmr, &timers->tcon);
 
-	endtime = get_ticks() + tmo;
+	timestamp = 0;
 
-	do {
-		ulong now = get_ticks();
-		diff = endtime - now;
-	} while (diff >= 0);
+	return 0;
 }
 
 /*
- * This function is derived from PowerPC code (read timebase as long long).
- * On ARM it just returns the timer value.
+ * Get the number of ticks (in CONFIG_SYS_HZ resolution)
  */
 unsigned long long get_ticks(void)
 {
-	ulong now = READ_TIMER();
+	return get_timer(0);
+}
+
+unsigned long get_timer_raw(void)
+{
+	ulong now = read_timer();
 
 	if (lastdec >= now) {
 		/* normal mode */
 		timestamp += lastdec - now;
 	} else {
 		/* we have an overflow ... */
-		timestamp += lastdec + timer_load_val - now;
+		timestamp += lastdec + timer_reset_value - now;
 	}
 	lastdec = now;
 
@@ -157,20 +134,33 @@ unsigned long long get_ticks(void)
  */
 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;
+	return CONFIG_SYS_HZ;
+}
+
+ulong get_timer_masked(void)
+{
+	unsigned long tmr = get_timer_raw();
+
+	return (tmr * CONFIG_SYS_HZ) / timer_rate_hz;
+}
+
+ulong get_timer(ulong base)
+{
+	return get_timer_masked() - base;
+}
+
+void __udelay(unsigned long usec)
+{
+	unsigned long tmp;
+	unsigned long tmo;
+
+	/* convert usec to ticks. */
+	tmo = ((timer_rate_hz / 1000) * usec) / 1000;
+
+	tmp = get_timer_raw() + tmo;	/* get current timestamp */
+
+	while (get_timer_raw() < tmp)   /* loop till event */
+		/*NOP*/;
 }
 
 /*
@@ -189,7 +179,7 @@ void reset_cpu(ulong ignored)
 	writel(0x0001, &watchdog->wtcnt);
 
 	/* Enable watchdog timer; assert reset at timer timeout */
-	writel(0x0021, &watchdog->wtcon);
+	writel(WTCON_TIMER_EN | WTCON_RESET_EN, &watchdog->wtcon);
 
 	while (1)
 		/* loop forever and wait for reset to happen */;
-- 
1.7.1