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

[Marek Vasut]

Dear 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.

Not optimizations, bugfixes ;-) Nice job pointing this out, Luka :-) Prafulla, 
can you please comment?
> 
> > > > > +#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

Define it empty in your arch/arm/cpu/..../kirkwood.c and be done with it ;-)
> 
> 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

Why do you need to skip it? Does it hang or something?

> 
> > > > > +#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.

Let's flame!!!

Hm ... on second thought, I use VIM too ... how are we supposed to flame about 
editors if we both happily use the same one? ;-)

> I'll resend
> v4 with this indentation unless Wolfgang has some objections.

Good idea.

> If indentation is wrong in all other places in u-boot we can easily fix
> that with some sed magic.

... or elisp script :trollface: :-)

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

You can wait for Prafulla with #1 and #2, also for #2 check my comment. But we 
have two bugs going on for granted here at least and they're not your boards 
fault. On the other hand, it'd be cool if you could fix them prior to adding 
your board ;-)

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

Wait just a few hours until the other people express their opinion so you don't 
waste too much of your time.

> 
> Bye,
> Luka

Best regards,
Marek Vasut