[U-Boot] [PATCH 1/1 v5] arm: add support for the suen3 board from keymile

[Prafulla Wadaskar]

> -----Original Message-----
> From: Heiko Schocher [mailto:hs at denx.de] 
> Sent: Thursday, February 18, 2010 1:54 PM
> To: U-Boot user list
> Cc: Prafulla Wadaskar; Wolfgang Denk; Scott Wood; Stefan Roese
> Subject: [PATCH 1/1 v5] arm: add support for the suen3 board 
> from keymile
> 
> Add support for the ARM part of the mgcoge2, named suen3.
> This board is based on the Marvell Kirkwood (88F6281) SoC.
> As there come more board variants, common code is stored in
> board/keymile/km_arm/km_arm.c
> 
> Signed-off-by: Holger Brunck <holger.brunck at keymile.com>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Signed-off-by: Heiko Schocher <hs at denx.de>
> ---
> - changes since v1:
>   added comments from Wolfgang Denk:
>   get rid of flash_info_t define in board config
>   (to get this working patch 1/2 is introduced/needed)
> 
> - changes since v2:
>   added comments from Wolfgang Denk
>   - rearranged if/else in do_spi_toggle()
>   - added I/O accessor functions for bootcounter
> 
> - changes since v3:
>   added comment Scott Wood
>   - removed nand_init in do_spi_toggle()
> 
>   added comments from Prafulla Wadaskar
>   - km-arm.h renamed to km_arm.h
>   - reworked eeprom_write_enable() (deleted it)
>     (when reviewing this function, it cropped up, that
>      this pin is connected through a gpio pin, not as
>      in previous version, through the boco (a FPGA))
>   - moved set_sda(), set_scl(), get_sda(), get_scl()
>     to km_arm.c
>   - split patch in 4 patches (for each board an extra patch)
>   - renamed sdramregs.txt in kwbimage.cfg, also license
>     info added.
> 
> - changes since v4
>   added comments from Prafulla Wadaskar
>   - add comments in board/keymile/km_arm/kwbimage.cfg
>   - cleanup define CONFIG_KIRKWOOD_GPIO
>   - corect wrong name at end of config file
>   - add only support for the suen3 target, others
>     follow later
> 
> ---
>  MAINTAINERS                       |    2 +-
>  MAKEALL                           |    1 +
>  Makefile                          |    6 +
>  board/keymile/common/common.c     |    6 +-
>  board/keymile/km_arm/Makefile     |   51 ++++++
>  board/keymile/km_arm/config.mk    |   28 ++++
>  board/keymile/km_arm/km_arm.c     |  323 
> +++++++++++++++++++++++++++++++++++++
>  board/keymile/km_arm/kwbimage.cfg |  176 ++++++++++++++++++++
>  include/configs/km_arm.h          |  191 ++++++++++++++++++++++
>  include/configs/suen3.h           |  103 ++++++++++++
>  10 files changed, 884 insertions(+), 3 deletions(-)
>  create mode 100644 board/keymile/km_arm/Makefile
>  create mode 100644 board/keymile/km_arm/config.mk
>  create mode 100644 board/keymile/km_arm/km_arm.c
>  create mode 100644 board/keymile/km_arm/kwbimage.cfg

>  create mode 100644 include/configs/km_arm.h
>  create mode 100644 include/configs/suen3.h

It would be good if you can merge these two files since single board board support
For additional board that you are planning, you can even add separate file or think of splitting it.
For better readability and to avoid complexity, I recommend to have one single file per board in include/config

...snip...
> diff --git a/board/keymile/km_arm/km_arm.c 
> b/board/keymile/km_arm/km_arm.c
> new file mode 100644
> index 0000000..31610be
> --- /dev/null
> +++ b/board/keymile/km_arm/km_arm.c
> @@ -0,0 +1,323 @@
> +/*
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Prafulla Wadaskar <prafulla at marvell.com>
> + *
> + * (C) Copyright 2009
> + * Stefan Roese, DENX Software Engineering, sr at denx.de.
> + *
> + * (C) Copyright 2010
> + * Heiko Schocher, DENX Software Engineering, hs at denx.de.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
> +
> +#include <common.h>
> +#include <i2c.h>
> +#include <nand.h>
> +#include <netdev.h>
> +#include <miiphy.h>
> +#include <asm/io.h>
> +#include <asm/arch/kirkwood.h>
> +#include <asm/arch/mpp.h>
> +
> +#include "../common/common.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static int	io_dev;
> +extern I2C_MUX_DEVICE *i2c_mux_ident_muxstring (uchar *buf);
> +
> +/* Multi-Purpose Pins Functionality configuration */
> +u32 kwmpp_config[] = {
> +	MPP0_NF_IO2,
> +	MPP1_NF_IO3,
> +	MPP2_NF_IO4,
> +	MPP3_NF_IO5,
> +	MPP4_NF_IO6,
> +	MPP5_NF_IO7,
> +	MPP6_SYSRST_OUTn,
> +	MPP7_PEX_RST_OUTn,
> +#if defined(CONFIG_SOFT_I2C)
> +	MPP8_GPIO,		/* SDA */
> +	MPP9_GPIO,		/* SCL */
> +#else

you can remove else part and ifdef with CONFIG_HARD_I2C

> +	MPP8_TW_SDA,
> +	MPP9_TW_SCK,
> +#endif

...snip...
> +int misc_init_r(void)
> +{
> +	I2C_MUX_DEVICE	*i2cdev;
> +	char *str;
> +	int mach_type;
> +
> +	/* add I2C Bus for I/O Expander */
> +	i2cdev = i2c_mux_ident_muxstring((uchar *)"pca9554a:70:a");
> +	io_dev = i2cdev->busid;
> +	puts("Piggy:");
> +	if (ethernet_present() == 0)
> +		puts (" not");
> +	puts(" present\n");
> +

Please remove below stuff, this is risky and not needed

> +	str = getenv("mach_type");
> +	if (str != NULL) {
> +		mach_type = simple_strtoul(str, NULL, 10);
> +		printf("Overwriting MACH_TYPE with %d!!!\n", mach_type);
> +		gd->bd->bi_arch_number = mach_type;
> +	}
> +	return 0;
> +}

Rest code looks good to me

Regards..
Prafulla . .