From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Mon, 25 May 2009 09:08:12 +0200
Subject: [U-Boot] [PATCH v10] Marvell MV88F6281GTW_GE Board support
In-Reply-To: <73173D32E9439E4ABB5151606C3E19E201CF9E6A04@SC-VEXCH1.marvell.com>
References: <73173D32E9439E4ABB5151606C3E19E201CF9E6A04@SC-VEXCH1.marvell.com>
Message-ID: <200905250908.13242.sr@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Monday 25 May 2009 06:48:40 Prafulla Wadaskar wrote:
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <netdev.h>
> > > +#include <asm/arch/kirkwood.h>
> > > +#include <asm/arch/mpp.h>
> > > +#include "mv88f6281gtw_ge.h"
> >
> > please move the mv88f6281gtw_ge.h
> > to include/asm-arm/arch-kirkwood/
>
> Sure... I will do it :-)

Sorry for the late reply, but for my taste such board specific headers don't 
belong into the "common" include/asm-arch/arch-kirkwood directory. The 
original location in the board directory is the best place (IMHO).

> > > +int board_init(void)
> > > +{
> > > +	/*
> > > +	 * default gpio configuration
> > > +	 * There are maximum 64 gpios controlled through 2 sets
> >
> > of registers
> >
> > > +	 * the  below configuration configures mainly initial LED status
> > > +	 */
> > > +	kw_config_gpio(MV88F6281GTW_GE_OE_VAL_LOW,
> > > +			MV88F6281GTW_GE_OE_VAL_HIGH,
> > > +			MV88F6281GTW_GE_OE_LOW,
> >
> > MV88F6281GTW_GE_OE_HIGH);
> >
> > > +
> > > +	/* Multi-Purpose Pins Functionality configuration */
> > > +	u32 kwmpp_config[] = {
> >
> > do you really need to configure all of this IO?
>
> Yes
> I device needs all IO initializations done to make sure unused MPPs must be
> configured to GPIOs. This is required to reduce overall boards noise that
> we have identified through h/w test in the lab
>
> > > +		MPP0_SPI_SCn,
> > > +		MPP1_SPI_MOSI,
> > > +		MPP2_SPI_SCK,
> > > +		MPP3_SPI_MISO,
> >
> > please add a device init api as done for davinci or at91
> > (could be done in an other patch)
>
> I could not get any reference for davinci or at91 for "device_init"
> Can you please provide me a specific pointer ?
>
> OR do you mean to create new api "device_init" to do soc/device specific
> init ? I think this is really not required here since it is specific to
> kirkwood implementation and may not apply for other architectures.
>
> In my opinion we do not need any specific init here in this code... Pls
> clarify...

I think the current code version is fine. We don't need to create an over-
complex API here.

Thanks.

Best regards,
Stefan

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