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

[Stefano Babic <sbabic@denx.de>]

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

Hi Tapani,

>  arch/arm/include/asm/mach-types.h  |    1 
>  board/technexion/tao3530/Makefile  |   43 ++++
>  board/technexion/tao3530/tao3530.c |  207 ++++++++++++++++++++
>  board/technexion/tao3530/tao3530.h |  382 +++++++++++++++++++++++++++++++++++++
>  boards.cfg                         |    1 
>  include/configs/tao3530.h          |  362 +++++++++++++++++++++++++++++++++++
>  6 files changed, 996 insertions(+)
> 

Most my comments from TAM3517 are still valid for this module. I post
here only new comments ;-)

I think you have already got the comment, but you should better explain
in the commit message what the patch does. This patch is different from
the TAM3517 patch, but both share the same commit message.

> diff --git a/board/technexion/tao3530/Makefile b/board/technexion/tao3530/Makefile
> new file mode 100644
> index 0000000..59c7d4f
> --- /dev/null
> +++ b/board/technexion/tao3530/Makefile
> @@ -0,0 +1,43 @@
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.

Author, Copyright ?


> diff --git a/board/technexion/tao3530/tao3530.c b/board/technexion/tao3530/tao3530.c
> new file mode 100644
> index 0000000..b8a4ac8
> --- /dev/null
> +++ b/board/technexion/tao3530/tao3530.c
> @@ -0,0 +1,207 @@
> +/*
> + * Maintainer :
> + *      Tapani Utriainen <linuxfae@technexion.com>
> + *

The correct and used way is to set author and copyright in the header
file, and put the maintainer name in the MAINTAINER file.

> +/*
> + * Routine: misc_init_r
> + * Description: Configure board specific parts
> + */
> +int misc_init_r(void)
> +{
> +	struct gpio *gpio5_base = (struct gpio *)OMAP34XX_GPIO5_BASE;
> +	struct gpio *gpio6_base = (struct gpio *)OMAP34XX_GPIO6_BASE;
> +
> +	twl4030_power_init();
> +	twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
> +
> +
> +	/* Configure GPIOs to output */
> +	/* GPIO23 */
> +	writel(~(GPIO10 | GPIO8 | GPIO2 | GPIO1), &gpio6_base->oe);
> +	writel(~(GPIO31 | GPIO30 | GPIO22 | GPIO21 |
> +		 GPIO15 | GPIO14 | GPIO13 | GPIO12), &gpio5_base->oe);

You have already used the GPIO framework - why do you write directly the
registers ?

This should be fixed globally.

> +
> +	/* Set GPIOs */
> +	writel(GPIO10 | GPIO8 | GPIO2 | GPIO1,
> +		&gpio6_base->setdataout);
> +	writel(GPIO31 | GPIO30 | GPIO29 | GPIO28 | GPIO22 | GPIO21 |
> +		GPIO15 | GPIO14 | GPIO13 | GPIO12, &gpio5_base->setdataout);
> +
> +#if defined(CONFIG_CMD_NET)
> +	setup_net_chip();
> +#endif
> +	dieid_num_r();
> +
> +	/* Set memory size environment variable, depending on revision */
> +	switch (tao3530_revision()) {
> +	case 0x2:  /* Rev C1 -- 256MB */
> +		 setenv("mem_size", "mem=256M");

mmmhh...memory is computed with get_ram_size(), not on the revision number.

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
=====================================================================