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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
