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

[Prafulla Wadaskar]

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr] 
> Sent: Tuesday, April 13, 2010 4:39 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
> 
...snip...
> >>>>> +.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,

You are right, the code is SoC specific, but the values to be programmed are board specific.

if you are interested,
Look at cpu/arm926ejs/at91/lowlevel_init.s how it programs SMRDATA.
Follow similar arch here too.
-Define similar data segment for holding soc_reg_addr and data array
-Define macros for SOC register addresses in soc specific header file
-Define macros for SOC register values in board specific header file
This way we can retain lowlevel_init.S in soc specific folder, whereas you can pass data through board specific macros.

> not board specific. Even the guidelines are variant-specific, not 
> board-specific.

To me:
Type of DRAM used, how it is interfaced, clock speed- all are board specific.

If removed mpp and gpio inits from current file,
Lowlevel_init.S mostly represents DRAM configuration here.

For a particular board we know what static configuration is required. Those should be configured through lowlevel_init.S
I am mapping this with Kirkwood kwbimage.cfg implementation.

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

That's why lowlevel_init should do basic static initialization in asm (i.e. mostly DRAM configuration)
And other initialization in either arch_cpu_init() or arch_misc_init() or board_init().

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

Sure,
DRAM init should go in lowlevel_init i.e. asm code, that's need of Orion5X
Either its location or its architecture need to change to address board specific issues.

Regards..
Prafulla . .