On Thu, Dec 27, 2012 at 9:07 AM, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Javier,
Hi Igor,
Thanks a lot for your feedback.
On 12/26/12 05:24, Javier Martinez Canillas wrote:
Even when the IGEPv2 board and the IGEP Computer-on-Module are different from a form factor point of view, they are very similar in the fact that share many components and how they are wired.
So, it is possible (and better) to have a single board file for both devices and just use the CONFIG_MACH_TYPE to make a differentiation between both boards when needed.
I'm wondering... isn't there a way to distinguish between the boards in runtime? Because if there is, you could even use the same binary. This is what we do on cm-t3530/cm-t3730 and is very maintainable, avoids code duplication, and avoids multiple ifdeffery.
Sadly there isn't.
It would have been great to have a way to distinguish between the two boards but it seems the IGEP hardware designers decided this was not necessary.
This change avoids duplication by removing 298 lines of code and makes future maintenance easier.
Signed-off-by: Javier Martinez Canillas javier.martinez@collabora.co.uk
board/isee/igep0020/Makefile | 43 --------- board/isee/igep0020/igep0020.c | 177 -------------------------------------- board/isee/igep0020/igep0020.h | 151 -------------------------------- board/isee/igep0030/Makefile | 43 --------- board/isee/igep0030/igep0030.c | 118 ------------------------- board/isee/igep0030/igep0030.h | 151 -------------------------------- board/isee/igep00x0/Makefile | 43 +++++++++ board/isee/igep00x0/igep00x0.c | 186 ++++++++++++++++++++++++++++++++++++++++ board/isee/igep00x0/igep00x0.h | 165 +++++++++++++++++++++++++++++++++++ boards.cfg | 8 +- 10 files changed, 398 insertions(+), 687 deletions(-) delete mode 100644 board/isee/igep0020/Makefile delete mode 100644 board/isee/igep0020/igep0020.c delete mode 100644 board/isee/igep0020/igep0020.h delete mode 100644 board/isee/igep0030/Makefile delete mode 100644 board/isee/igep0030/igep0030.c delete mode 100644 board/isee/igep0030/igep0030.h create mode 100644 board/isee/igep00x0/Makefile create mode 100644 board/isee/igep00x0/igep00x0.c create mode 100644 board/isee/igep00x0/igep00x0.h
[...]
diff --git a/board/isee/igep00x0/igep00x0.c b/board/isee/igep00x0/igep00x0.c new file mode 100644 index 0000000..c8b2fbf --- /dev/null +++ b/board/isee/igep00x0/igep00x0.c
[...]
+#include <common.h> +#include <twl4030.h> +#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0020) +#include <netdev.h> +#include <asm/gpio.h> +#include <asm/arch/omap_gpmc.h> +#endif
Is there a problem including three above files for igep0030? If not, then it will be better to remove the if.
Ok, will do.
+#include <asm/io.h> +#include <asm/arch/mem.h> +#include <asm/arch/mmc_host_def.h> +#include <asm/arch/mux.h> +#include <asm/arch/sys_proto.h> +#include <asm/mach-types.h> +#include "igep00x0.h"
+DECLARE_GLOBAL_DATA_PTR;
+#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0020) && defined(CONFIG_CMD_NET)
Does the igep0030 uses CONFIG_CMD_NET? If it does not, then you can just disable CONFIG_CMD_NET for igep0030 and here (and other net related places) just use #ifdef CONFIG_CMD_NET
Ok, will simplify this too.
+/* GPMC definitions for LAN9221 chips */ +static const u32 gpmc_lan_config[] = {
NET_LAN9221_GPMC_CONFIG1,NET_LAN9221_GPMC_CONFIG2,NET_LAN9221_GPMC_CONFIG3,NET_LAN9221_GPMC_CONFIG4,NET_LAN9221_GPMC_CONFIG5,NET_LAN9221_GPMC_CONFIG6,+}; +#endif
[...]
+#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0020) && defined(CONFIG_CMD_NET) +/*
- Routine: setup_net_chip
- Description: Setting up the configuration GPMC registers specific to the
Ethernet hardware.- */
+static void setup_net_chip(void) +{
struct ctrl *ctrl_base = (struct ctrl *)OMAP34XX_CTRL_BASE;enable_gpmc_cs_config(gpmc_lan_config, &gpmc_cfg->cs[5], 0x2C000000,GPMC_SIZE_16M);/* Enable off mode for NWE in PADCONF_GPMC_NWE register */writew(readw(&ctrl_base->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);/* Enable off mode for NOE in PADCONF_GPMC_NADV_ALE register */writew(readw(&ctrl_base->gpmc_noe) | 0x0E00, &ctrl_base->gpmc_noe);/* Enable off mode for ALE in PADCONF_GPMC_NADV_ALE register */writew(readw(&ctrl_base->gpmc_nadv_ale) | 0x0E00,&ctrl_base->gpmc_nadv_ale);/* Make GPIO 64 as output pin and send a magic pulse through it */if (!gpio_request(64, "")) {gpio_direction_output(64, 0);gpio_set_value(64, 1);udelay(1);gpio_set_value(64, 0);udelay(1);gpio_set_value(64, 1);}+}
having here:
#else static inline void setup_net_chip(void) {}
or equivalent will remove the need to place the call inside the ifdef below.
+#endif
[...]
+/*
- Routine: misc_init_r
- Description: Configure board specific parts
- */
+int misc_init_r(void) +{
twl4030_power_init();+#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0020) && defined(CONFIG_CMD_NET)
MUX_IGEP0020();setup_net_chip();+#endif
+#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0030)
MUX_IGEP0030();+#endif
Why do you need to setup the board specific muxes in misc_init_r()? Why not do this in set_muxconf_regs() along with the common muxes.
Perfect, will change that.
dieid_num_r();return 0;+}
+/*
- Routine: set_muxconf_regs
- Description: Setting up the configuration Mux registers specific to the
hardware. Many pins need to be moved from protect to primary
mode.- */
+void set_muxconf_regs(void) +{
MUX_DEFAULT();+}
[...]
diff --git a/board/isee/igep00x0/igep00x0.h b/board/isee/igep00x0/igep00x0.h new file mode 100644 index 0000000..83822d2 --- /dev/null +++ b/board/isee/igep00x0/igep00x0.h
[...]
+const omap3_sysinfo sysinfo = {
DDR_STACKED,+#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0020)
"OMAP3 IGEP v2 board",+#endif +#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0030)
"OMAP3 IGEP COM Module",+#endif +#if defined(CONFIG_ENV_IS_IN_ONENAND)
"ONENAND",+#else
"NAND",+#endif +};
+#if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0020) && defined(CONFIG_CMD_NET) +static void setup_net_chip(void); +#endif
Why do you need this declaration?
It was necessary because I didn't include an:
#else static inline void setup_net_chip(void) {}
as you suggested, but with that change it won't be necessary to use ifdeffery anymore.
+/*
- IEN - Input Enable
- IDIS - Input Disable
- PTD - Pull type Down
- PTU - Pull type Up
- DIS - Pull type selection is inactive
- EN - Pull type selection is active
- M0 - Mode 0
- The commented string gives the final mux configuration for that pin
- */
+#define MUX_DEFAULT()\
MUX_VAL(CP(SDRC_D0), (IEN | PTD | DIS | M0)) /* SDRC_D0 */\[...]
MUX_VAL(CP(SDRC_CKE1), (IDIS | PTU | EN | M0)) /* SDRC_CKE1 */+#endif
+#define MUX_IGEP0020() \
MUX_VAL(CP(GPMC_WAIT2), (IEN | PTU | DIS | M4)) /* GPIO_64-ETH_NRST */\+#define MUX_IGEP0030() \
MUX_VAL(CP(UART1_TX), (IDIS | PTD | DIS | M0)) /* UART1_TX */\MUX_VAL(CP(UART1_RX), (IEN | PTD | DIS | M0)) /* UART1_RX */\diff --git a/boards.cfg b/boards.cfg index 91504c0..35fbc85 100644 --- a/boards.cfg +++ b/boards.cfg @@ -253,10 +253,10 @@ cm_t35 arm armv7 cm_t35 - omap3_overo arm armv7 overo - omap3 omap3_pandora arm armv7 pandora - omap3 dig297 arm armv7 dig297 comelit omap3 -igep0020 arm armv7 igep0020 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0020,BOOT_ONENAND -igep0020_nand arm armv7 igep0020 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0020,BOOT_NAND -igep0030 arm armv7 igep0030 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0030,BOOT_ONENAND -igep0030_nand arm armv7 igep0030 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0030,BOOT_NAND +igep0020 arm armv7 igep00x0 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0020,BOOT_ONENAND +igep0020_nand arm armv7 igep00x0 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0020,BOOT_NAND +igep0030 arm armv7 igep00x0 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0030,BOOT_ONENAND +igep0030_nand arm armv7 igep00x0 isee omap3 igep00x0:MACH_TYPE=MACH_TYPE_IGEP0030,BOOT_NAND am3517_evm arm armv7 am3517evm logicpd omap3 mt_ventoux arm armv7 mt_ventoux teejet omap3 omap3_zoom1 arm armv7 zoom1 logicpd omap3
-- Regards, Igor.
Thanks a lot for your suggestions and best regards, Javier