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