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

[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
> <benoit.thebaudeau at advansee.com> wrote:
> > Dear Rob Herring,
> >
> > On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote:
> >> From: Rob Herring <rob.herring at calxeda.com>
> >>
> >> Convert mx25 to use the commmon timer code.
> >>
> >> Signed-off-by: Rob Herring <rob.herring at 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