[U-Boot] [PATCH] Initial support for Orion5x SoC and EDMini board

[Albert ARIBAUD <albert.aribaud@free.fr>]

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@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 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.