Hi Prafulla and all,
Prafulla Wadaskar a écrit :
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@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.aribaud@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 prafulla@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 setsof 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,