| Message ID | 1316428941-8957-3-git-send-email-marek.vasut@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
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. > @@ -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. > /* 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 ? 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. Best regards, Stefano Babic
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
On 09/22/2011 02:29 PM, Marek Vasut wrote: >> 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. Then there will be a review for the new code. At the moment, this part behaves as dead code. Do you mean the same board files will be used ? I am not aware about a board having two different SOCs. Probably (I say probably, we will see whan the patches for a new board will be sent...) we will have a different structure, as the MX53 have different setup as the MX51. In the same way we have now a mx51evk and mx53evk. >>> >>> +#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. Personally I find confusing if sometimes an #ifdef is used and the next time #ifndef with the opposite CONFIG is taken, and both part of code are compiled at the same time. >> 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. I let you decide. >>> +#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 ? Surely, you can add a patch to this patchset. Best regards, Stefano Babic
On Thursday, September 22, 2011 04:00:38 PM Stefano Babic wrote: > On 09/22/2011 02:29 PM, Marek Vasut wrote: > >> 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. > > Then there will be a review for the new code. At the moment, this part > behaves as dead code. Dead code? it's all used, I don't see your point. To me, it's more readable. Hmhm ... > > Do you mean the same board files will be used ? I am not aware about a > board having two different SOCs. Probably (I say probably, we will see > whan the patches for a new board will be sent...) we will have a > different structure, as the MX53 have different setup as the MX51. In > the same way we have now a mx51evk and mx53evk. We'll see ... I don't have the board just yet. > > >>> +#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. > > Personally I find confusing if sometimes an #ifdef is used and the next > time #ifndef with the opposite CONFIG is taken, and both part of code > are compiled at the same time. > > >> 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. > > I let you decide. > > >>> +#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 ? > > Surely, you can add a patch to this patchset. Eventually, yes ... not today though. > > Best regards, > Stefano Babic
diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c index 0f84ae0..33fbc86 100644 --- a/board/efikamx/efikamx.c +++ b/board/efikamx/efikamx.c @@ -46,6 +46,19 @@ DECLARE_GLOBAL_DATA_PTR; #error "CONFIG_MXC_SPI not set, this is essential for board's operation!" #endif +#if !defined(CONFIG_MACH_EFIKAMX) && !defined(CONFIG_MACH_EFIKASB) +#error "Missing CONFIG_MACH_EFIKAMX or CONFIG_MACH_EFIKASB in config.h!" +#endif + +/* + * Pin definition + */ +#ifdef CONFIG_MACH_EFIKAMX +#define EFIKA_SD1_CD MX51_PIN_GPIO1_0 +#else +#define EFIKA_SD1_CD MX51_PIN_EIM_CS2 +#endif + /* * Shared variables / local defines */ @@ -65,6 +78,7 @@ void efikamx_toggle_led(uint32_t mask); /* * Board identification */ +#ifdef CONFIG_MACH_EFIKAMX u32 get_efika_rev(void) { u32 rev = 0; @@ -96,6 +110,12 @@ u32 get_efika_rev(void) return (~rev & 0x7) + 1; } +#else +inline u32 get_efika_rev(void) +{ + return 0; +} +#endif u32 get_board_rev(void) { @@ -273,7 +293,7 @@ int board_mmc_getcd(u8 *absent, struct mmc *mmc) struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) - *absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_0)); + *absent = gpio_get_value(IOMUX_TO_GPIO(EFIKA_SD1_CD)); else *absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8)); @@ -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 /* Internal SDHC1 IOMUX + SDHC2 IOMUX on old boards */ if (get_efika_rev() < EFIKAMX_BOARD_REV_12) { +#endif /* SDHC1 IOMUX */ mxc_request_iomux(MX51_PIN_SD1_CMD, IOMUX_CONFIG_ALT0 | IOMUX_CONFIG_SION); @@ -384,6 +406,7 @@ int board_mmc_init(bd_t *bis) ret = fsl_esdhc_initialize(bis, &esdhc_cfg[0]); if (!ret) ret = fsl_esdhc_initialize(bis, &esdhc_cfg[1]); +#ifndef CONFIG_MACH_EFIKASB } else { /* New boards use only SDHC1 */ /* SDHC1 IOMUX */ mxc_request_iomux(MX51_PIN_SD1_CMD, @@ -414,6 +437,7 @@ int board_mmc_init(bd_t *bis) ret = fsl_esdhc_initialize(bis, &esdhc_cfg[0]); } +#endif return ret; } #endif @@ -500,6 +524,7 @@ static inline void setup_iomux_usb(void) { } /* * LED configuration */ +#if defined(CONFIG_MACH_EFIKAMX) void setup_iomux_led(void) { /* Blue LED */ @@ -524,6 +549,26 @@ void efikamx_toggle_led(uint32_t mask) gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_CSI1_HSYNC), mask & EFIKAMX_LED_RED); } +#else +void setup_iomux_led(void) +{ + /* CAPS-LOCK LED */ + mxc_request_iomux(MX51_PIN_EIM_CS0, IOMUX_CONFIG_GPIO); + gpio_direction_output(IOMUX_TO_GPIO(MX51_PIN_EIM_CS0), 0); + + /* ALARM-LED LED */ + mxc_request_iomux(MX51_PIN_GPIO1_3, IOMUX_CONFIG_GPIO); + gpio_direction_output(IOMUX_TO_GPIO(MX51_PIN_GPIO1_3), 0); +} + +void efikamx_toggle_led(uint32_t mask) +{ + gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_EIM_CS0), + mask & EFIKAMX_LED_BLUE); + gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_3), + !(mask & EFIKAMX_LED_GREEN)); +} +#endif /* * 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; return 0;
Signed-off-by: Marek Vasut <marek.vasut@gmail.com> Cc: Stefano Babic <sbabic@denx.de> --- board/efikamx/efikamx.c | 57 +++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 53 insertions(+), 4 deletions(-)