[U-Boot] [PATCH] Initial support for Orion5x SoC and EDMini board
Albert ARIBAUD
albert.aribaud at free.fr
Sat Nov 14 10:22:32 CET 2009
Hi Prafulla and all,
Prafulla Wadaskar a écrit :
>
>> -----Original Message-----
>> From: u-boot-bounces at lists.denx.de
>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
>> Sent: Saturday, November 14, 2009 6:32 AM
>> To: U-Boot at 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 at 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 at 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.
More information about the U-Boot
mailing list