Hi Javier,
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.
>
> 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.
> +#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
> +/* 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.
> +
> + 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?
> +
> +/*
> + * 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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
