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
