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

[Prafulla Wadaskar]

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr] 
> Sent: Saturday, November 14, 2009 2:53 PM
> To: Prafulla Wadaskar
> Cc: U-Boot at lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik
> Subject: Re: [U-Boot] [PATCH] Initial support for Orion5x SoC 
> and EDMini board
> 
> 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.

It should be patch series with properly addressed dependencies
i.e. 
(1/4)basic orion soc support patch
(2/4)orion gpio driver
(3/4)orion UART support
(4/4) Board support patch

All patches when applied in series should build u-boot for respective board.
The spilt has to be done carefully because individual patch may be applied to different repositories,
like nandf to u-boot-nand.git, basic orion support to u-boot-marvell.git, egiga driver patch will go in u-boot-net.git etc

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

I will suggest to use lower part of available ram for this

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

Thanks , I will provide separate patch for this

> 
> >> +     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 

Okay I will take this back
Whereas I found 
 +             sprintf(rev_str,"0x%02x", rev);
At so many places, can you reduce it?
 
> > 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".

That's good approach for this board, if you are planning to put support for more Lacie boards, then its better to have boards/lacie

Regards..
Prafulla . .