Patchwork [U-Boot,5/9] ARM: mx25: convert to common timer code

login
register
mail settings
Submitter Rob Herring
Date Sept. 8, 2013, 8:12 p.m.
Message ID <1378671174-18535-6-git-send-email-robherring2@gmail.com>
Download mbox | patch
Permalink /patch/273456/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Rob Herring - Sept. 8, 2013, 8:12 p.m.
From: Rob Herring <rob.herring@calxeda.com>

Convert mx25 to use the commmon timer code.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/cpu/arm926ejs/mx25/timer.c | 117 ------------------------------------
 include/configs/mx25pdk.h           |   3 +
 include/configs/tx25.h              |   2 +
 include/configs/zmx25.h             |   3 +
 4 files changed, 8 insertions(+), 117 deletions(-)
Benoît Thébaudeau - Sept. 8, 2013, 11:56 p.m.
Dear Rob Herring,

On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Convert mx25 to use the commmon timer code.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
[...]
> diff --git a/include/configs/mx25pdk.h b/include/configs/mx25pdk.h
> index ccd3b6c..568ed6c 100644
> --- a/include/configs/mx25pdk.h
> +++ b/include/configs/mx25pdk.h
> @@ -15,6 +15,9 @@
>  #define CONFIG_SYS_TEXT_BASE		0x81200000
>  #define CONFIG_MXC_GPIO
>  
> +#define CONFIG_SYS_TIMER_RATE		32768
                                                ^
MXC_CLK32 could be used here.

> +#define CONFIG_SYS_TIMER_COUNTER	(IMX_GPT1_BASE + 0x24)

This Linux-style (base + offset) register access is against U-Boot rules. You
could write:
(&((struct gpt_regs *)IMX_GPT1_BASE)->counter)

> +
>  #define CONFIG_DISPLAY_CPUINFO
>  #define CONFIG_DISPLAY_BOARDINFO
>  
> diff --git a/include/configs/tx25.h b/include/configs/tx25.h
> index 2d7479b..f879441 100644
> --- a/include/configs/tx25.h
> +++ b/include/configs/tx25.h
> @@ -15,6 +15,8 @@
>   */
>  #define CONFIG_MX25
>  #define CONFIG_MX25_CLK32		32000	/* OSC32K frequency */
> +#define CONFIG_SYS_TIMER_RATE		CONFIG_MX25_CLK32

Ditto 1.

> +#define CONFIG_SYS_TIMER_COUNTER	(IMX_GPT1_BASE + 0x24)

Ditto 2.

>  
>  #define	CONFIG_SYS_MONITOR_LEN		(256 << 10)	/* 256 kB for U-Boot */
>  
> diff --git a/include/configs/zmx25.h b/include/configs/zmx25.h
> index 2e7f145..deaadfa 100644
> --- a/include/configs/zmx25.h
> +++ b/include/configs/zmx25.h
> @@ -14,6 +14,9 @@
>  #define CONFIG_MX25
>  #define CONFIG_SYS_TEXT_BASE		0xA0000000
>  
> +#define CONFIG_SYS_TIMER_RATE		32768

Ditto 1.

> +#define CONFIG_SYS_TIMER_COUNTER	(IMX_GPT1_BASE + 0x24)

Ditto 2.

> +
>  #define CONFIG_MACH_TYPE	MACH_TYPE_ZMX25
>  /*
>   * Environment settings

Best regards,
Benoît
Rob Herring - Sept. 9, 2013, 9 p.m.
On Sun, Sep 8, 2013 at 6:56 PM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
> Dear Rob Herring,
>
> On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> Convert mx25 to use the commmon timer code.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> ---
> [...]
>> diff --git a/include/configs/mx25pdk.h b/include/configs/mx25pdk.h
>> index ccd3b6c..568ed6c 100644
>> --- a/include/configs/mx25pdk.h
>> +++ b/include/configs/mx25pdk.h
>> @@ -15,6 +15,9 @@
>>  #define CONFIG_SYS_TEXT_BASE         0x81200000
>>  #define CONFIG_MXC_GPIO
>>
>> +#define CONFIG_SYS_TIMER_RATE                32768
>                                                 ^
> MXC_CLK32 could be used here.

The problem the circular dependency that creates. MXC_CLK32 depends on
CONFIG_MX25_CLK32. Ordering could fix this, but


>> +#define CONFIG_SYS_TIMER_COUNTER     (IMX_GPT1_BASE + 0x24)
>
> This Linux-style (base + offset) register access is against U-Boot rules. You
> could write:
> (&((struct gpt_regs *)IMX_GPT1_BASE)->counter)

This may also have ordering issues. Including imx-regs.h just for the
base address doesn't work on mx27 for example.

Also, it seems like if u-boot is moving towards using kconfig, then
creating more include dependencies in the config headers is the wrong
direction.

Rob
Benoît Thébaudeau - Sept. 10, 2013, 10:25 a.m.
On Monday, September 9, 2013 11:00:51 PM, Rob Herring wrote:
> On Sun, Sep 8, 2013 at 6:56 PM, Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> > Dear Rob Herring,
> >
> > On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> Convert mx25 to use the commmon timer code.
> >>
> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> >> ---
> > [...]
> >> diff --git a/include/configs/mx25pdk.h b/include/configs/mx25pdk.h
> >> index ccd3b6c..568ed6c 100644
> >> --- a/include/configs/mx25pdk.h
> >> +++ b/include/configs/mx25pdk.h
> >> @@ -15,6 +15,9 @@
> >>  #define CONFIG_SYS_TEXT_BASE         0x81200000
> >>  #define CONFIG_MXC_GPIO
> >>
> >> +#define CONFIG_SYS_TIMER_RATE                32768
> >                                                 ^
> > MXC_CLK32 could be used here.
> 
> The problem the circular dependency that creates. MXC_CLK32 depends on
> CONFIG_MX25_CLK32. Ordering could fix this, but

"but" what?

Yes:
#define CONFIG_MX25_CLK32		32000	/* OSC32K frequency */
#include <asm/arch/clock.h>
#define CONFIG_SYS_TIMER_RATE		MXC_CLK32

> >> +#define CONFIG_SYS_TIMER_COUNTER     (IMX_GPT1_BASE + 0x24)
> >
> > This Linux-style (base + offset) register access is against U-Boot rules.
> > You
> > could write:
> > (&((struct gpt_regs *)IMX_GPT1_BASE)->counter)
> 
> This may also have ordering issues. Including imx-regs.h just for the
> base address doesn't work on mx27 for example.

There has to be a way to make the inclusion of imx-regs.h work on mx27 like on
mx25. Also, imx27lite-common.h uses UART1_BASE from imx-regs.h, and it is very
dirty to use literal constants for CONFIG_SYS_TIMER_COUNTER instead of
definitions from imx-regs.h. The fix here should really be to make the inclusion
of imx-regs.h work on mx27.

> Also, it seems like if u-boot is moving towards using kconfig, then
> creating more include dependencies in the config headers is the wrong
> direction.

Right. However, the only thing that asm/arch/clock.h does here is to define a
SoC-specific value with a default value. Converted to kconfig, that would just
give:

Kconfig file for i.MX25 SoC:
---
config MXC_CLK32
	int "32-kHz oscillator frequency"
	default 32768
	help
	  Exact frequency of the 32-kHz oscillator, expressed in Hz
---

Kconfig file for your generic timer base driver:
---
config SYS_TIMER_RATE
	int "System timer rate (Hz)"
	default MXC_CLK32 if MXC
---

Best regards,
Benoît
Rob Herring - Sept. 10, 2013, 4:20 p.m.
On Tue, Sep 10, 2013 at 5:25 AM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
> On Monday, September 9, 2013 11:00:51 PM, Rob Herring wrote:
>> On Sun, Sep 8, 2013 at 6:56 PM, Benoît Thébaudeau
>> <benoit.thebaudeau@advansee.com> wrote:
>> > Dear Rob Herring,
>> >
>> > On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
>> >> From: Rob Herring <rob.herring@calxeda.com>
>> >>
>> >> Convert mx25 to use the commmon timer code.
>> >>
>> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> >> ---
>> > [...]
>> >> diff --git a/include/configs/mx25pdk.h b/include/configs/mx25pdk.h
>> >> index ccd3b6c..568ed6c 100644
>> >> --- a/include/configs/mx25pdk.h
>> >> +++ b/include/configs/mx25pdk.h
>> >> @@ -15,6 +15,9 @@
>> >>  #define CONFIG_SYS_TEXT_BASE         0x81200000
>> >>  #define CONFIG_MXC_GPIO
>> >>
>> >> +#define CONFIG_SYS_TIMER_RATE                32768
>> >                                                 ^
>> > MXC_CLK32 could be used here.
>>
>> The problem the circular dependency that creates. MXC_CLK32 depends on
>> CONFIG_MX25_CLK32. Ordering could fix this, but
>
> "but" what?

Oops. But it is fragile is what I meant to say.

> Yes:
> #define CONFIG_MX25_CLK32               32000   /* OSC32K frequency */
> #include <asm/arch/clock.h>
> #define CONFIG_SYS_TIMER_RATE           MXC_CLK32

This example highlights the fragility as you have to know all the
CONFIG_* defines clock.h and anything clock.h includes.


>> >> +#define CONFIG_SYS_TIMER_COUNTER     (IMX_GPT1_BASE + 0x24)
>> >
>> > This Linux-style (base + offset) register access is against U-Boot rules.
>> > You
>> > could write:
>> > (&((struct gpt_regs *)IMX_GPT1_BASE)->counter)
>>
>> This may also have ordering issues. Including imx-regs.h just for the
>> base address doesn't work on mx27 for example.
>
> There has to be a way to make the inclusion of imx-regs.h work on mx27 like on
> mx25. Also, imx27lite-common.h uses UART1_BASE from imx-regs.h, and it is very
> dirty to use literal constants for CONFIG_SYS_TIMER_COUNTER instead of
> definitions from imx-regs.h. The fix here should really be to make the inclusion
> of imx-regs.h work on mx27.

Well, to start with mx27 imx-regs.h has this:

#ifdef CONFIG_MXC_UART
extern void mx27_uart1_init_pins(void);
#endif /* CONFIG_MXC_UART */

#ifdef CONFIG_FEC_MXC
extern void mx27_fec_init_pins(void);
#endif /* CONFIG_FEC_MXC */

#ifdef CONFIG_MXC_MMC
extern void mx27_sd1_init_pins(void);
extern void mx27_sd2_init_pins(void);
#endif /* CONFIG_MXC_MMC */

I will drop mx27 from the series and leave this to someone else to fix.

>> Also, it seems like if u-boot is moving towards using kconfig, then
>> creating more include dependencies in the config headers is the wrong
>> direction.
>
> Right. However, the only thing that asm/arch/clock.h does here is to define a
> SoC-specific value with a default value. Converted to kconfig, that would just
> give:
>
> Kconfig file for i.MX25 SoC:
> ---
> config MXC_CLK32
>         int "32-kHz oscillator frequency"
>         default 32768
>         help
>           Exact frequency of the 32-kHz oscillator, expressed in Hz
> ---
>
> Kconfig file for your generic timer base driver:
> ---
> config SYS_TIMER_RATE
>         int "System timer rate (Hz)"
>         default MXC_CLK32 if MXC

This would not scale well to hundreds of platforms, so it probably
needs to be in the platform kconfig. But this is another discussion...

Rob

Patch

diff --git a/arch/arm/cpu/arm926ejs/mx25/timer.c b/arch/arm/cpu/arm926ejs/mx25/timer.c
index 42b6076..7f19791 100644
--- a/arch/arm/cpu/arm926ejs/mx25/timer.c
+++ b/arch/arm/cpu/arm926ejs/mx25/timer.c
@@ -21,65 +21,8 @@ 
  */
 
 #include <common.h>
-#include <div64.h>
 #include <asm/io.h>
 #include <asm/arch/imx-regs.h>
-#include <asm/arch/clock.h>
-
-DECLARE_GLOBAL_DATA_PTR;
-
-#define timestamp	(gd->arch.tbl)
-#define lastinc		(gd->arch.lastinc)
-
-/*
- * "time" is measured in 1 / CONFIG_SYS_HZ seconds,
- * "tick" is internal timer period
- */
-#ifdef CONFIG_MX25_TIMER_HIGH_PRECISION
-/* ~0.4% error - measured with stop-watch on 100s boot-delay */
-static inline unsigned long long tick_to_time(unsigned long long tick)
-{
-	tick *= CONFIG_SYS_HZ;
-	do_div(tick, MXC_CLK32);
-	return tick;
-}
-
-static inline unsigned long long time_to_tick(unsigned long long time)
-{
-	time *= MXC_CLK32;
-	do_div(time, CONFIG_SYS_HZ);
-	return time;
-}
-
-static inline unsigned long long us_to_tick(unsigned long long us)
-{
-	us = us * MXC_CLK32 + 999999;
-	do_div(us, 1000000);
-	return us;
-}
-#else
-/* ~2% error */
-#define TICK_PER_TIME	((MXC_CLK32 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ)
-#define US_PER_TICK	(1000000 / MXC_CLK32)
-
-static inline unsigned long long tick_to_time(unsigned long long tick)
-{
-	do_div(tick, TICK_PER_TIME);
-	return tick;
-}
-
-static inline unsigned long long time_to_tick(unsigned long long time)
-{
-	return time * TICK_PER_TIME;
-}
-
-static inline unsigned long long us_to_tick(unsigned long long us)
-{
-	us += US_PER_TICK - 1;
-	do_div(us, US_PER_TICK);
-	return us;
-}
-#endif
 
 /* nothing really to do with interrupts, just starts up a counter. */
 /* The 32KHz 32-bit timer overruns in 134217 seconds */
@@ -104,63 +47,3 @@  int timer_init(void)
 
 	return 0;
 }
-
-unsigned long long get_ticks(void)
-{
-	struct gpt_regs *gpt = (struct gpt_regs *)IMX_GPT1_BASE;
-	ulong now = readl(&gpt->counter); /* current tick value */
-
-	if (now >= lastinc) {
-		/*
-		 * normal mode (non roll)
-		 * move stamp forward with absolut diff ticks
-		 */
-		timestamp += (now - lastinc);
-	} else {
-		/* we have rollover of incrementer */
-		timestamp += (0xFFFFFFFF - lastinc) + now;
-	}
-	lastinc = now;
-	return timestamp;
-}
-
-ulong get_timer_masked(void)
-{
-	/*
-	 * get_ticks() returns a long long (64 bit), it wraps in
-	 * 2^64 / MXC_CLK32 = 2^64 / 2^15 = 2^49 ~ 5 * 10^14 (s) ~
-	 * 5 * 10^9 days... and get_ticks() * CONFIG_SYS_HZ wraps in
-	 * 5 * 10^6 days - long enough.
-	 */
-	return tick_to_time(get_ticks());
-}
-
-ulong get_timer(ulong base)
-{
-	return get_timer_masked() - base;
-}
-
-/* delay x useconds AND preserve advance timstamp value */
-void __udelay(unsigned long usec)
-{
-	unsigned long long tmp;
-	ulong tmo;
-
-	tmo = us_to_tick(usec);
-	tmp = get_ticks() + tmo;	/* get current timestamp */
-
-	while (get_ticks() < tmp)	/* loop till event */
-		 /*NOP*/;
-}
-
-/*
- * 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;
-
-	tbclk = MXC_CLK32;
-	return tbclk;
-}
diff --git a/include/configs/mx25pdk.h b/include/configs/mx25pdk.h
index ccd3b6c..568ed6c 100644
--- a/include/configs/mx25pdk.h
+++ b/include/configs/mx25pdk.h
@@ -15,6 +15,9 @@ 
 #define CONFIG_SYS_TEXT_BASE		0x81200000
 #define CONFIG_MXC_GPIO
 
+#define CONFIG_SYS_TIMER_RATE		32768
+#define CONFIG_SYS_TIMER_COUNTER	(IMX_GPT1_BASE + 0x24)
+
 #define CONFIG_DISPLAY_CPUINFO
 #define CONFIG_DISPLAY_BOARDINFO
 
diff --git a/include/configs/tx25.h b/include/configs/tx25.h
index 2d7479b..f879441 100644
--- a/include/configs/tx25.h
+++ b/include/configs/tx25.h
@@ -15,6 +15,8 @@ 
  */
 #define CONFIG_MX25
 #define CONFIG_MX25_CLK32		32000	/* OSC32K frequency */
+#define CONFIG_SYS_TIMER_RATE		CONFIG_MX25_CLK32
+#define CONFIG_SYS_TIMER_COUNTER	(IMX_GPT1_BASE + 0x24)
 
 #define	CONFIG_SYS_MONITOR_LEN		(256 << 10)	/* 256 kB for U-Boot */
 
diff --git a/include/configs/zmx25.h b/include/configs/zmx25.h
index 2e7f145..deaadfa 100644
--- a/include/configs/zmx25.h
+++ b/include/configs/zmx25.h
@@ -14,6 +14,9 @@ 
 #define CONFIG_MX25
 #define CONFIG_SYS_TEXT_BASE		0xA0000000
 
+#define CONFIG_SYS_TIMER_RATE		32768
+#define CONFIG_SYS_TIMER_COUNTER	(IMX_GPT1_BASE + 0x24)
+
 #define CONFIG_MACH_TYPE	MACH_TYPE_ZMX25
 /*
  * Environment settings