Hello Prafulla,
Prafulla Wadaskar wrote:
>> -----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>
>> ---
[...]
>> 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
Hmm.. I think we tend to collect common config options in
one file, so board config files are not so big. Ok, actual
it is only one arm board from keymile here, but there
come more, and so it is sensible to collect common
options for this group of boards immediately ...
> ...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
But if this is in code, the user can fast switch to HARD_I2C,
if he wants it ... Ok, I delete it.
>> + 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
see comment for this from Stefan:
http://lists.denx.de/pipermail/u-boot/2010-February/067410.html
I got no comment from you, so I lthought it is Ok!
What should I do?
>> + 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
puhh... ;-)
Thanks for your comments!
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany