[U-Boot] [PATCH V6 1/3] Initial support for Marvell Orion5x SoC

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

Hi Prafulla,

Prafulla Wadaskar a ?crit :
>  
> 
>> -----Original Message-----
>> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr] 
>> Sent: Monday, April 12, 2010 8:33 PM
>> To: Prafulla Wadaskar
>> Cc: Wolfgang Denk; U-Boot at lists.denx.de
>> Subject: Re: [U-Boot] [PATCH V6 1/3] Initial support for 
>> Marvell Orion5x SoC
>>
>> (apparently one of Prafulla's answers did not reach me; 
>> copy-pasting it 
>> and replying even though it's slightly out of place within the thread)
>>
>> 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 
>> Prafulla Wadaskar
>>>> Sent: Friday, April 09, 2010 3:08 PM
>>>> To: Albert Aribaud; U-Boot at lists.denx.de
>>>> Subject: Re: [U-Boot] [PATCH V6 3/3] Add support for the 
>>>> LaCie ED Mini V2 board
>>>>
>>>>  
>>>>
>>>>> -----Original Message-----
>>>>> From: u-boot-bounces at lists.denx.de 
>>>>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of 
>> Albert Aribaud
>>>>> Sent: Friday, April 09, 2010 1:59 PM
>>>>> To: U-Boot at lists.denx.de
>>>>> Subject: [U-Boot] [PATCH V6 3/3] Add support for the LaCie ED 
>>>>> Mini V2 board
>>>>>
>>>>> This patch adds support for the LaCie ED Mini V2 product
>>>>> which is based on the Marvell Orion5x SoC.
>>>>> ---
>>>>>  MAINTAINERS                                |    4 +
>>>>>  MAKEALL                                    |    1 +
>>>>>  Makefile                                   |    3 +
>>>>>  board/LaCie/edminiv2/Makefile              |   58 +++++++++++
>>>>>  board/LaCie/edminiv2/board_lowlevel_init.S |   59 +++++++++++
>>>>>  board/LaCie/edminiv2/config.mk             |   27 +++++
>>>>>  board/LaCie/edminiv2/edminiv2.c            |   93 
>>>> ++++++++++++++++++
>>>>>  board/LaCie/edminiv2/edminiv2.h            |   54 ++++++++++
>>>>>  include/configs/edminiv2.h                 |  147 
>>>>> ++++++++++++++++++++++++++++
>>>>>  9 files changed, 446 insertions(+), 0 deletions(-)
>>>>>  create mode 100644 board/LaCie/edminiv2/Makefile
>>>>>  create mode 100644 board/LaCie/edminiv2/board_lowlevel_init.S
>>>>>  create mode 100644 board/LaCie/edminiv2/config.mk
>>>>>  create mode 100644 board/LaCie/edminiv2/edminiv2.c
>>>>>  create mode 100644 board/LaCie/edminiv2/edminiv2.h
>>>>>  create mode 100644 include/configs/edminiv2.h
>>>>>
>>>> ..snip..
>>>>> diff --git a/board/LaCie/edminiv2/board_lowlevel_init.S 
>>>>> b/board/LaCie/edminiv2/board_lowlevel_init.S
>>>>> new file mode 100644
>>>>> index 0000000..00e68e9
>>>>> --- /dev/null
>>>>> +++ b/board/LaCie/edminiv2/board_lowlevel_init.S
>>>>> @@ -0,0 +1,59 @@
>>>>> +/*
>>>>> + * Copyright (C) 2010 Albert ARIBAUD <albert.aribaud@free.fr>
>>>>> + *
>>>>> + * (C) Copyright 2009
>>>>> + * Marvell Semiconductor <www.marvell.com>
>>>>> + * Written-by: Prafulla Wadaskar <prafulla@marvell.com>
>>>>> + *
>>>>> + * See file CREDITS for list of people who contributed to this
>>>>> + * project.
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU General Public 
>> License as
>>>>> + * published by the Free Software Foundation; either 
>> version 2 of
>>>>> + * the License, or (at your option) any later version.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will 
>> be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied 
>> warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>> + * GNU General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General 
>>>> Public License
>>>>> + * along with this program; if not, write to the Free Software
>>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>>>>> + * MA 02110-1301 USA
>>>>> + */
>>>>> +
>>>>> +#include "edminiv2.h"
>>>>> +
>>>>> +/*
>>>>> + * Low-level init happens right after start.S has switched 
>>>> to SVC32,
>>>>> + * flushed and disabled caches and disabled MMU. We're 
>>>> still running
>>>>> + * from the boot chip select, so the first thing we should 
>>>> do is set
>>>>> + * up RAM for us to relocate into.
>>>>> + *
>>>>> + * board_low_level_init is called by the Orion5x 
>>>> lowlevel_init code,
>>>>> + * and sets up board-specifics such as MPPs and GPIOs.
>>>>> + */
>>>>> +
>>>>> +.globl board_lowlevel_init
>>>>> +
>>>>> +board_lowlevel_init:
>>>>> +
>>>>> +	/* Use R3 as the base for Device Bus registers */
>>>>> +	add     r3, r4, #0x10000
>>>>> +
>>>>> +	/* init MPPs */
>>>>> +	ldr	r6, =EDMINIV2_MPP0_7
>>>>> +	str	r6, [r3, #0x000]
>>>>> +	ldr	r6, =EDMINIV2_MPP8_15
>>>>> +	str	r6, [r3, #0x004]
>>>>> +	ldr	r6, =EDMINIV2_MPP16_23
>>>>> +	str	r6, [r3, #0x050]
>>>>> +
>>>>> +	/* init GPIOs */
>>>>> +	ldr	r6, =EDMINIV2_GPIO_OUT_ENABLE
>>>>> +	str	r6, [r3, #0x104]
>>>>> +
>>>>> +	/* Return to lowlevel_init via saved link register */
>>>>> +	mov pc, lr
>>>> Dear Albert
>>>>
>>>> You are just doing mpp and gpio settings here, those are IO 
>>>> specific only
>>>> you can have mpp and gpio configs as in case of Kirkwood (c 
>>>> functions) and call them from your bard_init.
>>>> Please remove this file.
>>>> and dependency of this code with lowlevel_init.S in patch 1/2
>>>>
>>>> That's it.
>>> Dear Albert
>>>
>>> Can you pls do the needfull for above and post V7 for this patch?
>> I can, but IMO that would doing C code for the sake of C code.
>>
>> The ED Mini board is a product, not a dev board, and has a completely 
>> static and predefined MPP and GPIO configuration; there will 
>> not even be 
>> an API to modify them from within U-boot (this was discussed 
>> in earlier 
>> iterations of the patch) and thus their whole handling throughout the 
>> whole patch takes no more than these eight ASM statements.
>>
>> Unless there is a pressing reason to switch to C, I would 
>> prefer to keep 
>> MPP+GPIO inits as they are. C code here would not provide any benefit.
> This is how C evolved :-)

Hmm... It only did because/when assembly became impractical. :)

> 1. Using C itself is benefit over assembly. Stragically prefer C wherever possible.
> 2. Converting mpp and gpio init in c function and putting them in board_init completely removes this file. patch becomes simple and smaller.
> 3. cpu/arm926ejs/orion5x/lowlevel_init.S in patch 1/3 will become independent and simple.

All right. However:

>  Actually you can pull it to board/LaCie/edminiv2/ since it has DRAM configuration and that is board specific.
>  I have put this as review comment earlier

And I'd answered it I believe. :) The DRAM-setting code is SoC specific, 
not board specific. Even the guidelines are variant-specific, not 
board-specific.

> It is not necessary to use Assembly here.

Well there it is necessary, because you can't use a C stack yet for 
instance, since you don't have RAM initialized yet.

> Our common objective is to add functional, clean, readable and smaller code to the repository.
> 
> I hope you will agree with me.

Partly: the DRAM init part has to stay in asm and in the SoC code I'm 
afraid.

> Regards..
> Prafulla . .

Amicalement,
-- 
Albert.