[U-Boot] [PATCH 2/2] EfikaSB: Add preliminary EfikaSB support

Marek Vasut
2011-09-22T12:29:32Z

On Thursday, September 22, 2011 11:44:01 AM Stefano Babic wrote:
> On 09/19/2011 12:42 PM, Marek Vasut wrote:
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
>
> Hi Marek,
>
> sorry for late review. I missed this patch...
>
> Only a couple of minor issues:
> > +#if !defined(CONFIG_MACH_EFIKAMX) && !defined(CONFIG_MACH_EFIKASB)
> > +#error "Missing CONFIG_MACH_EFIKAMX or CONFIG_MACH_EFIKASB in config.h!"
> > +#endif
>
> This seems not necessary because CONFIG_MACH_EFIKA* is set at the build
> time with the option in boards.cfg. With a correct boards.cfg, we cannot
> get this error.

Well once someone adds another efika, he can forget about it. And there's mx53
efika in the works.

>
> > @@ -284,9 +304,9 @@ int board_mmc_init(bd_t *bis)
> >
> > int ret;
> >
> > /* SDHC1 is used on all revisions, setup control pins first */
> >
> > - mxc_request_iomux(MX51_PIN_GPIO1_0,
> > + mxc_request_iomux(EFIKA_SD1_CD,
> >
> > IOMUX_CONFIG_ALT0 | IOMUX_CONFIG_SION);
> >
> > - mxc_iomux_set_pad(MX51_PIN_GPIO1_0,
> > + mxc_iomux_set_pad(EFIKA_SD1_CD,
> >
> > PAD_CTL_DRV_HIGH | PAD_CTL_HYS_ENABLE |
> > PAD_CTL_PUE_KEEPER | PAD_CTL_100K_PU |
> > PAD_CTL_ODE_OPENDRAIN_NONE |
> >
> > @@ -298,11 +318,13 @@ int board_mmc_init(bd_t *bis)
> >
> > PAD_CTL_100K_PU | PAD_CTL_ODE_OPENDRAIN_NONE |
> > PAD_CTL_SRE_FAST);
> >
> > - gpio_direction_input(IOMUX_TO_GPIO(MX51_PIN_GPIO1_0));
> > + gpio_direction_input(IOMUX_TO_GPIO(EFIKA_SD1_CD));
> >
> > gpio_direction_input(IOMUX_TO_GPIO(MX51_PIN_GPIO1_1));
> >
> > +#ifndef CONFIG_MACH_EFIKASB
>
> It is better to have the check consistent in the file. You mix #ifdef
> CONFIG_MACH_EFIKAMX with #ifndef CONFIG_MACH_EFIKASB, that is the same.

It expresses the intention much better IMO. And see above -- mx53 efika in the
works.

>
> > /* Internal SDHC1 IOMUX + SDHC2 IOMUX on old boards */
> > if (get_efika_rev() < EFIKAMX_BOARD_REV_12) {
> >
> > +#endif
>
> At the moment, the #ifdef seems redundant. You hard-code the efikasb
> revision to zero, and then get_efika_rev() is always smaller as
> EFIKAMX_BOARD_REV_12. What about to introduce a macro such as board_is()
> to increase readability ?

Yes it would, but it'd also increase code size.

>
> This if statement really means:
>
> if (board_is(EFIKASB) || (board_is(EFIKAMX) &&
> get_efika_rev() < EFIKAMX_BOARD_REV_12))
>
> > /*
> >
> > * Board initialization
> >
> > @@ -616,7 +661,11 @@ int board_early_init_f(void)
> >
> > int board_init(void)
> > {
> >
> > +#ifdef CONFIG_MACH_EFIKAMX
> >
> > gd->bd->bi_arch_number = MACH_TYPE_MX51_EFIKAMX;
> >
> > +#else
> > + gd->bd->bi_arch_number = MACH_TYPE_MX51_EFIKASB;
> > +#endif
> >
> > gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
>
> Can we use the new rule to set up the MACH-ID ? You can move the #ifdef
> inside config.h and let common code to set it.

Can we do that in a subsequent patch ?
>
> Best regards,
> Stefano Babic