[U-Boot] [PATCH v6 4/4] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices

[Wolfgang Denk <wd@denx.de>]

Dear Kyle Moffett,

In message <1300208664-18339-5-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> The eXMeritus HWW-1U-1A unit is a DO-160-certified 13lb 1U chassis
> with 3 independent TEMPEST zones.  Two independent P2020 computers may
> be found inside each zone.  Complete hardware support is included.

Please run checkpatch on your submissions!

...
> +	/* Ok, now go ahead and program all of those in one go */
> +	mpc85xx_gpio_set(	gpio_high|gpio_low|gpio_in,
> +				gpio_high|gpio_low,
> +				gpio_high);

ERROR: space prohibited after that open parenthesis '('
#427: FILE: board/exmeritus/hww1u1a/hww1u1a.c:100:
+       mpc85xx_gpio_set(       gpio_high|gpio_low|gpio_in,

> +	/*
> +	 * If things have been taken out of reset early (for example, by one
> +	 * of the BDI3000 debuggers), then we need to put them back in reset
> +	 * and delay a while before we continue.
> +	 */
> +#define GPIO_RESETS (GPIO_DIMM_RESET|GPIO_USB_RESET|GPIO_GETH0_RESET)
> +	if (mpc85xx_gpio_get(GPIO_RESETS)) {

Please don;t add #defines right in the middle of the code.

> +/*
> + * This little shell function just returns whether or not it's CPU A.
> + * It can be used to select the right device-tree when booting, etc.
> + */
> +int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

WARNING: line over 80 characters
#463: FILE: board/exmeritus/hww1u1a/hww1u1a.c:136:
+int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

> +U_BOOT_CMD(
> +	hww1u1a_test_cpu_a, 1, 0, do_hww1u1a_test_cpu_a,
> +	"Test if this is CPU A (versus B) on the eXMeritus HWW-1U-1A board",
> +	/*  */" && <command-if-true>\n"
> +	"hww1u1a_test_cpu_a || <command-if-false>\n"

What is this empty comment needed for?

> +	/* Now the serial# part of the hostname */
> +	for (j = 0; serialnr[j]; j++)
> +		if (isalnum(serialnr[j]))
> +			hww1u1a_prompt[i++] = tolower(serialnr[j]);

Braces needed for multiline statements.

...
> +		/* Turn on the "HRESET_REQ" pin (hard-reset request) */
> +		printf("\nRESET: Hardware reset triggered, waiting...\n");
> +		out_be32(&gur->rstcr, 0x2);
> +		while (1)
> +			udelay(10000);
> +	}

Should that not be an infinite wait here?

...
> +/* Enable the U-Boot "memory test" */
> +#define CONFIG_SYS_MEMTEST_START 0x00000000
> +#define CONFIG_SYS_MEMTEST_END   0x7fffffff

I think this has not been tested, right?


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
News is what a chap who doesn't care much  about  anything  wants  to
read. And it's only news until he's read it. After that it's dead.
                           - Evelyn Waugh _Scoop_ (1938) bk. 1, ch. 5