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

[Wolfgang Denk <wd@denx.de>]

Dear Mikhail Kshevetskiy,

In message <20100329162420.40f5433b@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@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.

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

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

> +	/* 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.

> +	/* ---------------------------------------------------------------- */
> +	/* 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.

...
> +	/* 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.


> +	/* 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.


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

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

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

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

> +/*
> + * 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.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There's a way out of any cage.
	-- Captain Christopher Pike, "The Menagerie" ("The Cage"),
	   stardate unknown.