[U-Boot] [PATCH V2 1/2] ARMV7: Versatile Express Coretile CortexA9x4 support

[Wolfgang Denk]

Dear matt.waddel at linaro.org,

In message <1280373167-20890-2-git-send-email-matt.waddel at linaro.org> you wrote:
> From: Matt Waddel <matt.waddel at linaro.org>
> 
> Adds support for the ARM quad-core Cortex-A9 processor.  This system
> includes a motherboard(Versatile Express), daughterboard(Coretile),
> and SOC(Cortex-A9 quad core).  The serial port, ethernet, and flash
> systems work with these additions.  The naming convention is:
>   SOC -> CortexA9 quad core = ca9x4
>   daughterboard -> Coretile = ct
>   motherboard -> Versatile Express = vxp
> This gives ca9x4_ct_vxp.c as the board support file.
> 
> Signed-off-by: Matt Waddel <matt.waddel at linaro.org>
...
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -663,6 +663,7 @@ LIST_ARMV7="		\
>  	omap4_sdp4430		\
>  	s5p_goni		\
>  	smdkc100		\
> +	ca9x4_ct_vxp		\

Please keep lists sorted.

> +++ b/arch/arm/include/asm/arch-armv7/systimer.h
> @@ -0,0 +1,50 @@
...
> +struct systimer {
> +	u32 Timer0Load;		/* 0x00 */
> +	u32 Timer0Value;
> +	u32 Timer0Control;
> +	u32 Timer0IntClr;
> +	u32 Timer0RIS;
> +	u32 Timer0MIS;
> +	u32 Timer0BGLoad;
> +	u32 Timer1Load;		/* 0x20 */
> +	u32 Timer1Value;
> +	u32 Timer1Control;
> +	u32 Timer1IntClr;
> +	u32 Timer1RIS;
> +	u32 Timer1MIS;
> +	u32 Timer1BGLoad;

We don't allow CamelCase identifiers in U-Boot. Variable names must be
all lower case.  Please fix globally.


> +++ b/board/armltd/vexpress/ca9x4_ct_vxp.c
> @@ -0,0 +1,231 @@
....
> +#if defined(CONFIG_SHOW_BOOT_PROGRESS)
> +void show_boot_progress(int progress)
> +{
> +    printf("Boot reached stage %d\n", progress);

Indentation by TABs only. Please fix globally.

...
> +int misc_init_r(void)
> +{
> +	setenv("verify", "n");
> +	return 0;
> +}

NAK. We don't allow that such decisions are enforced on the end user.
Feel free to add such a default setting to your environment, but leave
the user the freedom to decide otherwise.


> +static void flash__init(void)
> +{
> +	/* Setup the sytem control register to allow writing to flash */
> +	uint tmp = *(uint *)(VEXPRESS_FLASHCTRL);
> +	tmp |= VEXPRESS_FLASHPROG_FLVPPEN;
> +	*(uint *)(VEXPRESS_FLASHCTRL) = tmp;

NAK.  Please always use I/O accessors to access device registers and
the like. Please fix globally.

> +/* Use the ARM Watchdog System to cause reset */
> +void reset_cpu(ulong addr)
> +{
> +	writeb(WDT_EN, &wdt_base->WdogControl);
> +	writel(WDT_RESET_LOAD, &wdt_base->WdogLoad);

Does a 

	while (1)
		;

make sense here?

...
> +TEXT_BASE = 0x06000000
> +
> +LDSCRIPT := $(SRCTREE)/board/armltd/vexpress/u-boot.lds
> +

Please do not add trailing empty lines.

> +/* Sample environment settings */
> +#define CONFIG_BOOTCOMMAND		"bootm"
> +#define CONFIG_BOOTARGS			"root=/dev/nfs ip=dhcp mem=1024M " \
> +					"console=ttyAMA0 video=vc:1-2clcdfb: "\
> +					"nfsroot=10.1.77.36:/work/exports/share"
> +#define	CONFIG_EXTRA_ENV_SETTINGS	"ethaddr=00:02:F7:00:19:17\0"	\
> +					"ipaddr=10.1.77.77\0"		\
> +					"gatewayip=10.1.77.1\0"		\
> +					"serverip=10.1.77.36\0"
> +#define CONFIG_BOOTFILE			"/work/exports/vexpress"

NAK. We do not allow network settings in the default configuration.
They may make sense to you, but they do not make sense to most others.


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
They're usually so busy thinking about what  happens  next  that  the
only  time they ever find out what is happening now is when they come
to look back on it.                 - Terry Pratchett, _Wyrd Sisters_