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

2013-09-10 Thread Rob Herring 
On Tue, Sep 10, 2013 at 5:25 AM, Benoît Thébaudeau
 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
>>  wrote:
>> > Dear Rob Herring,
>> >
>> > On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
>> >> From: Rob Herring 
>> >>
>> >> Convert mx25 to use the commmon timer code.
>> >>
>> >> Signed-off-by: Rob Herring 
>> >> ---
>> > [...]
>> >> 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 0x8120
>> >>  #define CONFIG_MXC_GPIO
>> >>
>> >> +#define CONFIG_SYS_TIMER_RATE32768
>> > ^
>> > 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 
> #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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-09-10 Thread Benoît Thébaudeau 
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
>  wrote:
> > Dear Rob Herring,
> >
> > On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
> >> From: Rob Herring 
> >>
> >> Convert mx25 to use the commmon timer code.
> >>
> >> Signed-off-by: Rob Herring 
> >> ---
> > [...]
> >> 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 0x8120
> >>  #define CONFIG_MXC_GPIO
> >>
> >> +#define CONFIG_SYS_TIMER_RATE32768
> > ^
> > 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 
#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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-09-09 Thread Rob Herring 
On Sun, Sep 8, 2013 at 6:56 PM, Benoît Thébaudeau
 wrote:
> Dear Rob Herring,
>
> On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
>> From: Rob Herring 
>>
>> Convert mx25 to use the commmon timer code.
>>
>> Signed-off-by: Rob Herring 
>> ---
> [...]
>> 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 0x8120
>>  #define CONFIG_MXC_GPIO
>>
>> +#define CONFIG_SYS_TIMER_RATE32768
> ^
> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-09-08 Thread Benoît Thébaudeau 
Dear Rob Herring,

On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
> From: Rob Herring 
> 
> Convert mx25 to use the commmon timer code.
> 
> Signed-off-by: Rob Herring 
> ---
[...]
> 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 0x8120
>  #define CONFIG_MXC_GPIO
>  
> +#define CONFIG_SYS_TIMER_RATE32768
^
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_CLK3232000   /* OSC32K frequency */
> +#define CONFIG_SYS_TIMER_RATECONFIG_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 0xA000
>  
> +#define CONFIG_SYS_TIMER_RATE32768

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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-09-08 Thread Rob Herring 
From: Rob Herring 

Convert mx25 to use the commmon timer code.

Signed-off-by: Rob Herring 
---
 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(-)

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 
-#include 
 #include 
 #include 
-#include 
-
-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 + 99;
-   do_div(us, 100);
-   return us;
-}
-#else
-/* ~2% error */
-#define TICK_PER_TIME  ((MXC_CLK32 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ)
-#define US_PER_TICK(100 / 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 += (0x - 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   0x8120
 #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)
 
 #defineCONFIG_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   0xA000
 
+#define CONFI