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

Tue Apr 13 12:33:17 CEST 2010

> -----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 at free.fr>
> >> > + *
> >> > + * (C) Copyright 2009
> >> > + * Marvell Semiconductor <www.marvell.com>
> >> > + * Written-by: Prafulla Wadaskar <prafulla at 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 :-)

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

It is not necessary to use Assembly here.
Our common objective is to add functional, clean, readable and smaller code to the repository.

I hope you will agree with me.

Regards..
Prafulla . .

> 
> Amicalement,
> -- 
> Albert.
> 