diff mbox

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

Message ID 1378671174-18535-6-git-send-email-robherring2@gmail.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Rob Herring Sept. 8, 2013, 8:12 p.m. UTC 
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(-)

Comments

Benoît Thébaudeau Sept. 8, 2013, 11:56 p.m. UTC | #1 
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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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
diff mbox

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