Hi Prafulla and all, Prafulla Wadaskar a écrit : > >> -----Original Message----- >> From: u-boot-boun...@lists.denx.de >> [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Albert Aribaud >> Sent: Saturday, November 14, 2009 6:32 AM >> To: U-Boot@lists.denx.de >> Subject: [U-Boot] [PATCH] Initial support for Orion5x SoC and >> EDMini board >> >> This patch adds initial u-boot support for the Marvell orion5x >> SoC and the LAcie ED Mini V2 board--support is limited to serial, >> flash and environment. Further support will be added as devices >> common with kirkwood are made useable across SoCs. >> >> Signed-off-by: Albert Aribaud <albert.arib...@free.fr> >> --- > > Hi Albert > > Please break this patch in to small pieces > [...] > Pls edit MAKEALL and Maintainers files too for new board support > [...] > Arm: add Edminv2 board support (orion5x based)
Ok. > Break this in to basic orion5x Soc support patch > And individual drivers patches like gpio, uart, etc.. Will do. What exact criteria should I use for splitting patches? Obviously some patches will never be compiled alone, e.g. basic Orion5x support won't be compiled until board support patches come in; also, having SoC and board support without uart support, if it even compiles, will be pretty useless as the resulting u-boot won't even have a console. >> +# (C) Copyright 2009 >> +# Marvell Semiconductor <www.marvell.com> >> +# Written-by: Prafulla Wadaskar <prafu...@marvell.com> > > This is copy paste mistake, pls correct in all files > You can keep my reference, i.e. based on (ref: > boards/Marvell/openrd_base/openrd_base.c) Ok. >> +TEXT_BASE = 0x00600000 > > Is this valid for your board? BTW: how much DRAM you have on this board? Yes it works for the board--I'm using this value for testing. However, in very old and non-reuseable u-boot code, it was 0x00F00000. I'll switch to this in order to minimize the difference between these older u-boots and the current mainline one. The board has 64M RAM. >> + /* >> + * default gpio configuration >> + * There are maximum 64 gpios controlled through 2 sets >> of registers > > I don't think there are 64 GPIOs in orion5x Right, I'll fix the comment. That's 24 (with only 20 actually referenced by the board). >> + orion5x_config_gpio(EDMINIV2_OE_VAL_LOW, EDMINIV2_OE_LOW); > > You can eliminate LOW here, since mpps less than 3 > Pls remove word LOW here Ok. >> diff --git a/cpu/arm926ejs/orion5x/Makefile >> [...] >> +COBJS-y = dram.o >> +COBJS-y += cpu.o > > Pls put these lines in order Ok. Note this comes directly from kirkwood's Makefile, so the lines are in the same order (dram, cpu, mpp, timer) there too. >> + * There are maximum 64 Multi-Pourpose Pins on Orion5x > Correct this -64 Will do. >> +#define PCIE_DEV_ID_OFF (ORION5X_REG_PCIE_BASE + 0x0000) >> +#define PCIE_DEV_REV_OFF (ORION5X_REG_PCIE_BASE + 0x0008) > > Pls move the defination to header file Will do. >> + if (dev == MV88F5181_DEV_ID) { >> + dev_name = "MV88F5181"; >> + if (rev == MV88F5181_REV_B1) { >> + rev_name = "B1"; >> + } else if (rev == MV88F5181L_REV_A1) { >> + dev_name = "MV88F5181L"; >> + rev_name = "A1"; >> + } else if (rev == MV88F5181L_REV_A0) { >> + dev_name = "MV88F5181L"; >> + rev_name = "A0"; >> + } else { >> + sprintf(rev_str,"0x%02x", rev); >> + } >> + } else if (dev == MV88F5182_DEV_ID) { >> + dev_name = "MV88F5182"; >> + if (rev == MV88F5182_REV_A2) { >> + rev_name = "A2"; >> + } else { >> + sprintf(rev_str,"0x%02x", rev); >> + } >> + } else if (dev == MV88F5281_DEV_ID) { >> + dev_name = "MV88F5281"; >> + if (rev == MV88F5281_REV_D2) { >> + rev_name = "D2"; >> + } else if (rev == MV88F5281_REV_D1) { >> + rev_name = "D1"; >> + } else if (rev == MV88F5281_REV_D0) { >> + rev_name = "D0"; >> + } else { >> + sprintf(rev_str,"0x%02x", rev); >> + } >> + } else if (dev == MV88F6183_DEV_ID) { >> + dev_name = "MV88F6183"; >> + if (rev == MV88F6183_REV_B0) { >> + rev_name = "B0"; >> + } else { >> + sprintf(rev_str,"0x%02x", rev); >> + } >> + } else { >> + sprintf(dev_str,"0x%04x", rev); >> + sprintf(rev_str,"0x%02x", rev); > > This is common line, pls take out of if-else Can you be more specific? The outer 'if' sequence deals with device id and I see no no common line in its then/.../else branches, and the inner ifs are for revision and again, I cannot pinpoint the common line. Unless you would want the sprintf's to be done inconditionally before entering the if's? Note however that there is a typo: + } else { + sprintf(dev_str,"0x%04x", rev); + sprintf(rev_str,"0x%02x", rev); Should actually be + } else { + sprintf(dev_str,"0x%04x", dev); <- this is dev, not rev + sprintf(rev_str,"0x%02x", rev); >> +#ifdef CONFIG_ORION5X_EGIGA >> +int cpu_eth_init(bd_t *bis) >> +{ >> + orion5x_egiga_initialize(bis); >> + return 0; >> +} >> +#endif > > Pls do not add this by default, make it a part of egiga driver patch Will do. >> + * read feroceon/sheeva core extra feature register > > Pls remove name sheeva > >> + * write feroceon/sheeva core extra feature register > > Same Right. >> + * MBus-L to Mbus Bridge Registers >> + * Ref: Datasheet sec:A.3 > > Are those references correct? No, they're not, they're kirkwood's. I'll do a pass on these and fix them wrt orion. >> +#define GPIO_MAX 50 > > Is this correct defination for Orion? No. Should be 26. Will fix and add a reference to the specs. >> + * Kirorion5xood-specific GPIO API > > Copy paste error Worse, actually: it is a ':%s//' search/replace error. Will fix. >> +/* Documented registers */ >> +#define ORION5X_TWSI_BASE >> (ORION5X_REGISTER(0x11000)) >> +#define ORION5X_UART0_BASE >> (ORION5X_REGISTER(0x12000)) >> +#define ORION5X_UART1_BASE >> (ORION5X_REGISTER(0x12100)) >> +#define ORION5X_MPP_BASE >> (ORION5X_REGISTER(0x10000)) >> +#define ORION5X_GPIO_BASE >> (ORION5X_REGISTER(0x10100)) >> +#define ORION5X_CPU_WIN_BASE >> (ORION5X_REGISTER(0x20000)) >> +#define ORION5X_CPU_REG_BASE >> (ORION5X_REGISTER(0x20100)) >> +#define ORION5X_TIMER_BASE >> (ORION5X_REGISTER(0x20300)) >> +#define ORION5X_REG_PCI_BASE >> (ORION5X_REGISTER(0x30000)) >> +#define ORION5X_REG_PCIE_BASE >> (ORION5X_REGISTER(0x40000)) >> +#define ORION5X_USB20_PORT0_BASE >> (ORION5X_REGISTER(0x50000)) >> +#define ORION5X_USB20_PORT1_BASE >> (ORION5X_REGISTER(0xA0000)) >> +#define ORION5X_EGIGA_BASE >> (ORION5X_REGISTER(0x72000)) > > I hope these are inlined with orion5x and not the result of copy-paste, pls > check this carefully Yes they are in line--while I started off a copy of the kirkwood list, I took extra care to match each one with Orion5x's spec. >> +#define CONFIG_IDENT_STRING " Lacie ED Mini V2" > > Short board name will be prefered Understood, will put " EDMiniV2". >> +#define CONFIG_SYS_PROMPT "Marvell>> " /* > > Is this Marvell custom board ? > If not, even you can choose to keep in in boards instead of boards/Marvell/ No, it's not a Marvell custom board, it's a LaCie product board. I'll move the board to boards/ directly (or maybe "boards/lacie", to provide a home for other lacie boards? Any best pratice here to follow?) and change the prompt to "EDMiniV2". >> +/* >> + * Disabling many default commands for staggered bring-up >> + */ >> +#if 0 > > This is not allowed, pls remove it, or use some defination here > I suggest to use config_cmd_default.h folloed by def/undefs for required > commands to inline with other arm boards configuration Will do. Thanks a lot for the review. I'll fix the issues you pointed out and resubmit a patchset. Amicalement, -- Albert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot