[U-Boot] [PATCH v2] add new board nas62x0

[Luka Perkov]

Hi Marek,

On Sun, Mar 18, 2012 at 04:15:53PM +0100, Marek Vasut wrote:
> > + * Copyright (C) 2011 G??rald Kerma <dreagle at doukki.net>
> 
> Can you please fix your name here?

I think your mail agent is not displaying UTF8 characters correctly. If
this is a problem we could change it if Gerald is ok with that. Nobody
else had problems with this...

> > +#define IB62x0_OE_LOW		(~(0))
> > +#define IB62x0_OE_HIGH		(~(0))
> 
> Fix this constant please (0xffffffff) and remove those parenthesis ... btw 
> OE_HIGH and OE_LOW have both the same value?

Are you sure? It's done this way in:

board/Marvell/dreamplug/dreamplug.h
board/Marvell/sheevaplug/sheevaplug.h
board/Seagate/dockstar/dockstar.h

> > +# Configure RGMII-0 interface pad voltage to 1.8V
> > +DATA 0xFFD100e0 0x1b1b1b9b
> 
> Make usage of upper/lower case consistent across files in your patch please 
> (lowercase prefered).

Ok. I will send this in v4 once I get your feedback on other items.

> > +#define CONFIG_SKIP_LOWLEVEL_INIT	/* disable board lowlevel_init */
> 
> Are you sure you want to skip lowlevel init? It'll break cache setup etc. I 
> believe.

I will retest and send v4 once I get your feedback on other items.

> > +# define CONFIG_SYS_PROMPT	"IB-NAS6210 # "
> > +#elif CONFIG_BOARD_IS_IB_NAS6220
> > +# define CONFIG_SYS_PROMPT	"IB-NAS6220 # "
> > +#else
> > +# error Unknown RaidSonic ICY BOX board specified
> > +#endif
> 
> Please make the prompt like "=> " so we can run tests on this :)

Ok.

> > +#define CONFIG_MVSATA_IDE_USE_PORT0
> > +# ifdef CONFIG_BOARD_IS_IB_NAS6210
> > +#  undef CONFIG_SYS_IDE_MAXBUS
> > +#  define CONFIG_SYS_IDE_MAXBUS		1
> > +#  undef CONFIG_SYS_IDE_MAXDEVICE
> > +#  define CONFIG_SYS_IDE_MAXDEVICE	1
> > +# elif CONFIG_BOARD_IS_IB_NAS6220
> > +#  define CONFIG_MVSATA_IDE_USE_PORT1
> > +# endif
> > +#define CONFIG_SYS_ATA_IDE0_OFFSET	KW_SATA_PORT0_OFFSET
> > +# ifdef CONFIG_BOARD_IS_IB_NAS6220
> > +#  define CONFIG_SYS_ATA_IDE1_OFFSET	KW_SATA_PORT1_OFFSET
> > +# endif
> > +#endif /* CONFIG_CMD_IDE */
> 
> please don't use this "#[space][space]define" convention.

I must disagree here. This is more readable and there are many examples
in u-boot that use that syntax:

$ egrep '#[ ]+define' * -r | wc -l
12557

> The patch looks good otherwise )

Cool. Thanks for reviewing.

Bye,
Luka