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

[Rob Herring]

On Tue, Sep 10, 2013 at 5:25 AM, Benoît Thébaudeau
<benoit.thebaudeau at 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 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?

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