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

[Wolfgang Denk]

Dear "Moffett, Kyle D",

In message <5B9D9C87-C278-4AF3-B20C-26ECFF6C09B7 at boeing.com> you wrote:
> 
> > 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 * c=
> onst argv[])
> 
> This one is only a warning, and it's much more readable to have 1 84-charac=

In U-Boot, it is considered to be an ERROR.

> ter line than split it across 2 different lines.  Even still, this warning =
> is only issued if you pass "--subjective" to checkpatch, which is documente=
> d to "enable more subjective tests".  This thread discusses it further:

No, I get this warning without such flags.  The command I run is just
"checkpatch.pl --no-tree"

BTW - could you please restrict your line length to some 70 characters
or so?  Thanks.

> >> +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"
> >=20
> > What is this empty comment needed for?
> 
> Just a mental placeholder for the fact that the U_BOOT_CMD macro inserts th=
> e name of the command in that spot.  Will remove.

We don't provide usage examples in the help text.  This should be
fixed in the first place.

> >> +		/* 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);
> >> +	}
> >=20
> > Should that not be an infinite wait here?
> 
> At this point if the board does not reset due to hardware failure it's bett=
> er off hanging than silently falling through.

Why don't you simply call "hang()" then?

> >> +/* Enable the U-Boot "memory test" */
> >> +#define CONFIG_SYS_MEMTEST_START 0x00000000
> >> +#define CONFIG_SYS_MEMTEST_END   0x7fffffff
> >=20
> > 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.

I guess testing it would reveal that it crashes your system because you
are overwriting the exception vectors.

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
Madness has no purpose.  Or reason.  But it may have a goal.
	-- Spock, "The Alternative Factor", stardate 3088.7