On Tue, Sep 10, 2013 at 5:25 AM, Benoît Thébaudeau <benoit.thebaud...@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.thebaud...@advansee.com> wrote: >> > Dear Rob Herring, >> > >> > On Sunday, September 8, 2013 10:12:50 PM, Rob Herring wrote: >> >> From: Rob Herring <rob.herr...@calxeda.com> >> >> >> >> Convert mx25 to use the commmon timer code. >> >> >> >> Signed-off-by: Rob Herring <rob.herr...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
