[U-Boot] [PATCH 2/4] arm, omap3: Add support for TechNexion modules

[Stefano Babic]

On 21/11/2011 09:44, Tapani Utriainen wrote:
> 
> Add support for TechNexion TAM3517 SoM
> 
> Signed-off-by: Tapani Utriainen <tapani at technexion.com>
> CC: Sandeep Paulraj <s-paulraj at ti.com>

Hi Tapani,

> ---
>  arch/arm/include/asm/mach-types.h  |    1 
>  board/technexion/tam3517/Makefile  |   49 ++++
>  board/technexion/tam3517/tam3517.c |  150 ++++++++++++
>  board/technexion/tam3517/tam3517.h |  388 +++++++++++++++++++++++++++++++++
>  boards.cfg                         |    1 
>  include/configs/tam3517.h          |  427 +++++++++++++++++++++++++++++++++++++
>  6 files changed, 1016 insertions(+)
> 

I am also porting a TAM3517 based custom board to U-Boot mainline. Maybe
we can share some experiences ;-)

> diff --git a/arch/arm/include/asm/mach-types.h b/arch/arm/include/asm/mach-types.h
> index 2d5c3bc..685b46f 100644
> --- a/arch/arm/include/asm/mach-types.h
> +++ b/arch/arm/include/asm/mach-types.h
> @@ -467,6 +467,7 @@ extern unsigned int __machine_arch_type;
>  #define MACH_TYPE_OMAP4_PANDA          2791
>  #define MACH_TYPE_TI8168EVM            2800
>  #define MACH_TYPE_TETON_BGA            2816
> +#define MACH_TYPE_TAM3517              2818

This is wrong. The mach-types.h is taken from Linux and the rule is to
always be in sync with tthe kernel. If the MACH-TYPE is not set, it must
be set in the board configuration file.


> diff --git a/board/technexion/tam3517/tam3517.c b/board/technexion/tam3517/tam3517.c
> new file mode 100644

As I said, I am also porting a similar board and I am facing the problem
to support multiple board using the same module (TAM3517). My idea was
to have a common include/configs/tam3517.h file, that is included by all
boards using the module, such as the twister and a custom boards. This
allows to factorize most of common setup.

Then we could have a board/technexion/twister board in parallel with
other boards using only the module. What do you think about ?

> +#include <asm/mach-types.h>

We do not need mach-types.h

> +#include "tam3517.h"
> +
> +#define AM3517_IP_SW_RESET	0x48002598
> +#define CPGMACSS_SW_RST		(1 << 1)

These are obsolete, too. It is (or it will be soon) common code. See
previous patches for AM 3517 (mcx board).

> +
> +/*
> + * Routine: board_init
> + * Description: Early hardware init.
> + */
> +int board_init(void)
> +{
> +	DECLARE_GLOBAL_DATA_PTR;
> +
> +	gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
> +	/* board id for Linux */
> +	gd->bd->bi_arch_number = MACH_TYPE_TAM3517;

This should be removed, it is part of generic arm library.

> +#if defined(CONFIG_XR16L2751)

If I have understood the schematics, the twister board (because this
code corresponds to this board) has always a XR16L2751 UART. Should we
not drop the #ifdef ?

> +/*
> + * Routine: misc_init_r
> + */
> +int misc_init_r(void)
> +{
> +	int ctr = 0;
> +
> +	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> +#ifdef CONFIG_BOOT_FROM_MMC
> +	omap_set_gpio_direction(TAM3517_SW3_PIN8, 1);

This is obsolete - use the standard GPIO framework.

> +#if defined(CONFIG_DRIVER_TI_EMAC)
> +	/* allow the PHY to stabilize and settle down */
> +	do {
> +		udelay(1000);
> +		ctr++;
> +	} while (ctr < 300);
> +
> +	/*ensure that the module is out of reset*/
> +	reset = readl(AM3517_IP_SW_RESET);
> +	reset &= (~CPGMACSS_SW_RST);
> +	writel(reset, AM3517_IP_SW_RESET);
> +#endif

Drop this part - it is in EMAC initialization, not needed in board code.

> +	if (smc911x_initialize(0, CONFIG_SMC911X_BASE) <= 0)
> +		n_eth--;

Why do we need to count the interfaces ? This is already done by the
network subsystem.

> diff --git a/board/technexion/tam3517/tam3517.h b/board/technexion/tam3517/tam3517.h
> new file mode 10064

> +	/* ETK (ES2 onwards) */\
> +	MUX_VAL(CP(ETK_CLK_ES2),	(IDIS | PTU | EN  | M0)) \
> +	MUX_VAL(CP(ETK_CTL_ES2),	(IDIS | PTD | DIS | M0)) \
> +	MUX_VAL(CP(ETK_D0_ES2),		(IEN  | PTD | DIS | M0)) \
> +	MUX_VAL(CP(ETK_D1_ES2),		(IEN  | PTD | DIS | M0)) \
> +	MUX_VAL(CP(ETK_D2_ES2),		(IEN  | PTD | EN  | M0)) \

..but the pins for ETK are used as USB for EHCI-OMAP, are'nt they ? This
setup seems to me wrong.


> diff --git a/include/configs/tam3517.h b/include/configs/tam3517.h

See my previous comment regarding a common configuration file for the
module and a specific file for the boards.

> +/*
> + * High Level Configuration Options
> + */
> +#define CONFIG_ARMV7		1	/* This is an ARM V7 CPU core */

"1" is not needed, this must fixed globally.

> +
> +#define CONFIG_CMDLINE_TAG		1	/* enable passing of ATAGs */
> +#define CONFIG_SETUP_MEMORY_TAGS	1
> +#define CONFIG_INITRD_TAG		1
> +#define CONFIG_REVISION_TAG		1

Ditto.

> +#if CONFIG_DRAM_BUS_WIDTH == 16
> +#define _CONFIG_DRAM_BUS_WIDTH	"16"
> +#else
> +#ifndef CONFIG_DRAM_BUS_WIDTH
> +#define CONFIG_DRAM_BUS_WIDTH	32
> +#endif
> +#define _CONFIG_DRAM_BUS_WIDTH	"32"
> +#endif

I have not used any of these CONFIG_DRAM_BUS_WIDTH. Why do we need it ?
And I do not see code in u-boot using it. IMHO it is dead code.

> +/* USB
> + * Enable CONFIG_MUSB_HCD for Host functionalities MSC, keyboard
> + * Enable CONFIG_MUSB_UDD for Device functionalities.
> + */
> +#define CONFIG_USB_AM35X		1
> +#define CONFIG_MUSB_HCD			1

Is it working ? I do not understand. I have USB working on the twister
board, but using EHCI-OMAP and with another configuration for the pin
multiplexer.


> +/*-----------------------------------------------------------------------
> + * CFI FLASH driver setup
> + */
> +/* timeout values are in ticks */
> +#define CONFIG_SYS_FLASH_ERASE_TOUT	(100 * CONFIG_SYS_HZ)
> +#define CONFIG_SYS_FLASH_WRITE_TOUT	(100 * CONFIG_SYS_HZ)

Which CFI has the board ? I suposed none.

> +/*-----------------------------------------------------
> + * ethernet support for TAM3517
> + *-----------------------------------------------------
> + */
> +#define CONFIG_MII
> +
> +#if defined(CONFIG_CMD_NET)
> +/* Disable u-boot support for EMAC LAN, due compile problems
> +   Linux kernel support is enough to enable the interface
> +*/
> +/*
> +#define CONFIG_DRIVER_TI_EMAC
> +#define CONFIG_DRIVER_TI_EMAC_USE_RMII
> +#define CONFIG_EMAC_MDIO_PHY_NUM   0
> +#define CONFIG_ARM926EJS	1
> +*/
> +#define CONFIG_NET_RETRY_COUNT 10
> +#define CONFIG_NET_MULTI
> +
> +#define CONFIG_SMC911X          1
> +#define CONFIG_SMC911X_16_BIT	1
> +#define CONFIG_SMC911X_BASE     0x2C000000
> +#define CONFIG_SMC911X_NO_EEPROM	1
> +

The whole setup for SPL is missing. I have already implemented and
tested, and I am asking you how we can proceed at the best. Should I
post my patches (only for TAM3517), too, and then check together what is
still missing ? I can add you in the Signed-off and put you as board
maintainer, as you suggest (by the way, I have not seen a change in the
MAINTAINER file...)

Let me know what you think

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================