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

[Albert ARIBAUD]

Hi again Prafulla,

Prafulla Wadaskar a écrit :

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

Duly noted. Currently, I only add support for SoC, UART and NOT FLASH 
(not NAND) so only u-boot-marvell.git needs to be considered; I'll keep 
other repos in mind for upcoming patches like egiga and others.

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

Understood. Note the 0x00600000 value comes from kirkwood too, in case 
this matters.

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

Hmm yes, I think I can start by setting dev_name/rev_name tu NULL, go 
through the ifs and set dev_name/rev_name only if appropriate, and then 
do the sprintf()s only if dev_name/rev_name are still NULL after the ifs.

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

I guess there's at least a second one to come, so I'll go for the 
boards/lacie/* approach.

> Regards..
> Prafulla . .

Thanks again!

Amicalement,
-- 
Albert.