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

[Luka Perkov]

Hi Marek,

On Mon, Mar 19, 2012 at 04:50:52PM +0100, Marek Vasut wrote:
> > > > +#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
> 
> Does that mean it's inherently correct and not just a duplicated bug?

Well I really dont know. Judging by your comments it looks like kirkwood
target could use some optimizations in "core" and not only in the board
code.

> > > > +#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.
> 
> Ok, what's the result? From IRC I take it you must define this ... why?

It generates error when building without it:

/home/luka/uboot/arch/arm/cpu/arm926ejs/start.S:393: undefined reference to `lowlevel_init'
arm-openwrt-linux-ld: BFD (GNU Binutils) 2.22 assertion fail elf32-arm.c:13830

All other kirkwood targets I looked at define CONFIG_SKIP_LOWLEVEL_INIT,
including the ones mentioned above; here are their configs for
comparison:

include/configs/dreamplug.h
include/configs/sheevaplug.h
include/configs/dockstar.h 

> > > > +#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
> 
> Again, the fact that it's used doesn't mean it's correct. It's not more readable 
> actually, it's readable in the same way given you have good editor. Also, 
> doesn't checkpatch.pl caugh on this stuff ?

checkpatch.pl is ok with this. It's readable only if your editor uses
different colors for this, and I would not like to go into editor fight
here. I use vim probably as you but that is not important. I'll resend
v4 with this indentation unless Wolfgang has some objections.
If indentation is wrong in all other places in u-boot we can easily fix
that with some sed magic.

This is my proposal - I'll resend v4 and it should be ok to commit
without fixes for:

 1) IB62x0_OE_LOW and IB62x0_OE_HIGH
 2) CONFIG_SKIP_LOWLEVEL_INIT
 3) ifdef indentation

Because fixing the 1) and 2) is more than adding support for this new
board, and if it was in the same patch I would need to separate it. That
is a different issue.

I'll put on my TODO list, and work on this after commit:

 * replace tabs with spaces in boards.config
 * look at IB62x0_OE_LOW and IB62x0_OE_HIGH issue
 * look at CONFIG_SKIP_LOWLEVEL_INIT issue

If nobody has objections I'll resend v4...

Bye,
Luka