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

[Moffett, Kyle D]

Wolfgang,

Thanks for your detailed reviews!

Once I get these last few style issues resolved, what more do I need to do to get this merged?  I don't really want to spam the list with more nearly identical copies of these patches unless I'm sure that all the necessary review items have been taken care of.


On Mar 15, 2011, at 15:36, Wolfgang Denk wrote:
> In message <1300208664-18339-5-git-send-email-Kyle.D.Moffett at 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!

I did run it on this patch, although I forgot to run it on the resurrected reset patch (which is also now fixed).  It's a common gripe on the LKML that the tool is overzealous about certain warnings in cases where the "fix" makes the code less readable.

Specifically:

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

I could "fix" this code to read:

        mpc85xx_gpio_set(gpio_high|gpio_low|gpio_in,
                                gpio_high|gpio_low, gpio_high);

And it would be much harder to visually compare the three bitmask arguments against each other.


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

Fixed, thanks!


>> +/*
>> + * 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[])

This one is only a warning, and it's much more readable to have 1 84-character line than split it across 2 different lines.  Even still, this warning is only issued if you pass "--subjective" to checkpatch, which is documented to "enable more subjective tests".  This thread discusses it further:
  http://lkml.org/lkml/2009/12/15/490

>> +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?

Just a mental placeholder for the fact that the U_BOOT_CMD macro inserts the name of the command in that spot.  Will remove.


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

Fixed, thanks!


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

At this point if the board does not reset due to hardware failure it's better off hanging than silently falling through.


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

I'm pretty sure it's been tested, but not very recently; the memory on all the shipped units is ECC anyways.

Cheers,
Kyle Moffett