On Thu, Dec 27, 2012 at 9:07 AM, Igor Grinberg <grinb...@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.marti...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot