[U-Boot] [PATCH 2/2] add support of arm/pxa270 board made by voipac

[Mikhail Kshevetskiy]

Dear Wolfgang Denk,

Here is my comments to your notices,
the updated patch is in the following letter.

On Tue, 30 Mar 2010 23:15:45 +0200
Wolfgang Denk <wd at denx.de> wrote:

> Dear Mikhail Kshevetskiy,
> 
> In message <20100329162420.40f5433b at laska.campus-ws.pu.ru> you wrote:
> > This patch is based on custom u-boot-1.1.2 version produced by voipac
> > (http://www.voipac.com) and board/trizepsiv files from current u-boot.
> > 
> > Up to now only PXA270 DIMM module with NOR flash is tested.
> > 
> > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at gmail.com>
> > ---
> >  Makefile                      |    3 +
> >  board/vpac270/Makefile        |   51 +++
> >  board/vpac270/config.mk       |    3 +
> >  board/vpac270/lowlevel_init.S |  740 +++++++++++++++++++++++++++++++++++++++++
> >  board/vpac270/vpac270.c       |  101 ++++++
> >  include/configs/vpac270.h     |  329 ++++++++++++++++++
> >  6 files changed, 1227 insertions(+), 0 deletions(-)
> >  create mode 100644 board/vpac270/Makefile
> >  create mode 100644 board/vpac270/config.mk
> >  create mode 100644 board/vpac270/lowlevel_init.S
> >  create mode 100644 board/vpac270/vpac270.c
> >  create mode 100644 include/configs/vpac270.h
> 
> Entries to MAKEALL and MAINTAINERS missing.

fixed

> > diff --git a/board/vpac270/config.mk b/board/vpac270/config.mk
> > new file mode 100644
> > index 0000000..4486f6b
> > --- /dev/null
> > +++ b/board/vpac270/config.mk
> > @@ -0,0 +1,3 @@
> > +TEXT_BASE =0xa1f00000
> > +# 0xa1700000
> > +#TEXT_BASE = 0
> 
> Please do not add dead code. Remove it.

fixed

> > diff --git a/board/vpac270/lowlevel_init.S b/board/vpac270/lowlevel_init.S
> > new file mode 100644
> > index 0000000..1df381c
> ...
> > +/* wait for coprocessor write complete */
> > +   .macro CPWAIT reg
> > +   mrc	p15,0,\reg,c2,c0,0
> > +   mov	\reg,\reg
> > +   sub	pc,pc,#4
> > +   .endm
> 
> Indentation and vertial alignment by TAB only. Please fix globally.

fixed,
I try to keep lowlevel_init.S as much close to the same file from board/trizeptiv
as it was possible, so initially i keep formatting and comments style from the
upstream version of board/trizeptiv/lowlevel_init.S.

> 
> > +	/* Set up GPIO pins first ----------------------------------------- */
> > +
> > +	ldr		r0,	=GPSR0
> > +	ldr		r1,	=CONFIG_SYS_GPSR0_VAL
> > +	str		r1,   [r0]
> 
> One TAB between instruction and arguments should be sufficient.

same as above 

> > +	/* ---------------------------------------------------------------- */
> > +	/* Enable memory interface					    */
> > +	/*								    */
> > +	/* The sequence below is based on the recommended init steps	    */
> > +	/* detailed in the Intel PXA250 Operating Systems Developers Guide, */
> > +	/* Chapter 10.							    */
> > +	/* ---------------------------------------------------------------- */
> 
> Incorrect multiline comment style. Please fix globally.

same as above
 
> ...
> > +	/* MECR: Memory Expansion Card Register				    */
> > +	ldr	r2,  =CONFIG_SYS_MECR_VAL
> > +	str	r2,  [r1, #MECR_OFFSET]
> > +	ldr	r2,	[r1, #MECR_OFFSET]
> 
> Inconsistent indentation - please fix globally.

same as above
 
> > +	/* MCMEM0: Card Interface slot 0 timing				    */
> > +	ldr	r2,  =CONFIG_SYS_MCMEM0_VAL
> > +	str	r2,  [r1, #MCMEM0_OFFSET]
> > +	ldr	r2,	[r1, #MCMEM0_OFFSET]
> > +
> > +	/* MCMEM1: Card Interface slot 1 timing				    */
> > +	ldr	r2,  =CONFIG_SYS_MCMEM1_VAL
> > +	str	r2,  [r1, #MCMEM1_OFFSET]
> > +	ldr	r2,	[r1, #MCMEM1_OFFSET]
> > +
> > +	/* MCATT0: Card Interface Attribute Space Timing, slot 0	    */
> > +	ldr	r2,  =CONFIG_SYS_MCATT0_VAL
> > +	str	r2,  [r1, #MCATT0_OFFSET]
> > +	ldr	r2,	[r1, #MCATT0_OFFSET]
> > +
> > +	/* MCATT1: Card Interface Attribute Space Timing, slot 1	    */
> > +	ldr	r2,  =CONFIG_SYS_MCATT1_VAL
> > +	str	r2,  [r1, #MCATT1_OFFSET]
> > +	ldr	r2,	[r1, #MCATT1_OFFSET]
> > +
> > +	/* MCIO0: Card Interface I/O Space Timing, slot 0		    */
> > +	ldr	r2,  =CONFIG_SYS_MCIO0_VAL
> > +	str	r2,  [r1, #MCIO0_OFFSET]
> > +	ldr	r2,	[r1, #MCIO0_OFFSET]
> > +
> > +	/* MCIO1: Card Interface I/O Space Timing, slot 1		    */
> > +	ldr	r2,  =CONFIG_SYS_MCIO1_VAL
> > +	str	r2,  [r1, #MCIO1_OFFSET]
> > +	ldr	r2,	[r1, #MCIO1_OFFSET]
> > +
> > +	/* ---------------------------------------------------------------- */
> > +	/* Step 2c: Write FLYCNFG  FIXME: what's that???		    */
> > +	/* ---------------------------------------------------------------- */
> > +@	ldr	r2,  =CONFIG_SYS_FLYCNFG_VAL
> > +@	str	r2,  [r1, #FLYCNFG_OFFSET]
> > +@	str	r2,	[r1, #FLYCNFG_OFFSET]
> > +
> > +	/* ---------------------------------------------------------------- */
> > +	/* Step 2d: Initialize Timing for Sync Memory (SDCLK0)		    */
> > +	/* ---------------------------------------------------------------- */
> > +
> > +	/* Before accessing MDREFR we need a valid DRI field, so we set	    */
> > +	/* this to power on defaults + DRI field.			    */
> > +
> > +	ldr	r4,	[r1, #MDREFR_OFFSET]
> > +	ldr	r2,	=0xFFF
> > +	bic	r4,	r4, r2
> > +
> > +	ldr	r3,	=CONFIG_SYS_MDREFR_VAL
> > +	and	r3,	r3,  r2
> > +
> > +	orr	r4,	r4, r3
> > +	str	r4,	[r1, #MDREFR_OFFSET]	/* write back MDREFR	    */
> 
> 
> Hm... I think most of this does not belong into lowlevel_init.S which
> really should be kept minimal. Most of this should be rewritten in C.

the similar or exactly the same peace of code can be found in lowlevel
initialization of almost all arm/pxa boards, so i think it is reasonable to keep
this code in place.

> > diff --git a/board/vpac270/vpac270.c b/board/vpac270/vpac270.c
> > new file mode 100644
> > index 0000000..723e9d8
> > --- /dev/null
> > +++ b/board/vpac270/vpac270.c
> ...
> > +int board_late_init(void)
> > +{
> > +#if defined(CONFIG_SERIAL_MULTI)
> > +	char *console=getenv("boot_console");
> > +
> > +	if (console == NULL) console = BOOT_CONSOLE;
> 
> Incorrect indentation. Please check and fix globally.

fixed,
The same as early, i try to keep this file similar to 
board/trizeptiv/conxs.c file from the upstream.

> 
> > diff --git a/include/configs/vpac270.h b/include/configs/vpac270.h
> > new file mode 100644
> > index 0000000..969d6d3
> > --- /dev/null
> > +++ b/include/configs/vpac270.h
> ...
> > +//#define DEBUG 15
> 
> No C++ comments please. And don't add dead code.

fixed
 
> ...
> > +#define CONFIG_BOOTDELAY		3
> > +#define CONFIG_BOOTCOMMAND		"FIXME"
> > +#define CONFIG_BOOTARGS			"root=/dev/mtdblock2 rootfstype=jffs2 console=ttyS0,115200"
> 
> Line too long. Please fix globally.

fixed


> > +#define CONFIG_STACKSIZE		(64*1024)	/* regular stack */
> > +#ifdef CONFIG_USE_IRQ
> > +  #define CONFIG_STACKSIZE_IRQ		(4*1024)	/* IRQ stack */
> > +  #define CONFIG_STACKSIZE_FIQ		(4*1024)	/* FIQ stack */
> > +#endif
> 
> Indentation by TAB only!

fixed
 
> > +/*
> > + * GPIO settings
> > + */
> > +#if 1       /* minimal vpac270 settings, includes FFUART and Ethernet */
> ...
> > +#else       /* maximal vpac270 settings */
> ...
> 
> Do not add dead code. Remove the "#if 1" and the whole "else" block.

fixed