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

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1298311199-18775-8-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.
...

There are a number of formal issues - please run checkpatch:
total: 12 errors, 12 warnings, 1443 lines checked

ERROR: do not initialise statics to 0 or NULL
ERROR: space prohibited after that open parenthesis '('
ERROR: space prohibited before that close parenthesis ')'
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
WARNING: externs should be avoided in .c files
WARNING: please, no spaces at the start of a line

Please fix.

...
> +		puts("    Waiting 1 sec for reset...");
> +		for (i = 0; i < 10; i++) {
> +			udelay(100000);
> +			puts(".");
> +		}

Do you really need a full second here?

...
> +U_BOOT_CMD(
> +	hww1u1a_is_cpu_a, 1, 0, do_hww1u1a_is_cpu_a,
> +	"Test if this is CPU A (versus B) on the eXMeritus HWW-1U-1A board",
> +	/*  */" && echo This is CPU A\n"
> +	"if hww1u1a_is_cpu_a; then\n"
> +	"    ## Do stuff\n"
> +	"else\n"
> +	"    ## Do other stuff\n"
> +	"fi"

Please don't misuse the help messages like that.  Use external
documentation instead.

> +
> +/* Create a prompt-like string: "uboot at HOSTNAME% " */
> +#define PROMPT_PREFIX "uboot at exm"
> +#define PROMPT_SUFFIX "% "
> +
> +/* This function returns a PS1 prompt based on the serial number */
> +static char *hww1u1a_prompt = NULL;
> +const char *hww1u1a_get_ps1(void)
> +{
> +	unsigned long len, i, j;
> +	const char *serialnr;
> +
> +	/* If our prompt was already set, just use that */
> +	if (hww1u1a_prompt)
> +		return hww1u1a_prompt;
> +
> +	/* Use our serial number if present, otherwise "UNPROGRAMMED-[AB]" */
> +	serialnr = getenv("serial#");
> +	if (!serialnr || !serialnr[0]) {
> +		if (hww1u1a_is_cpu_a())
> +			serialnr = "999999-XA";
> +		else
> +			serialnr = "999999-XB";
> +	}
> +
> +	/*
> +	 * We will turn the serial number into a hostname by:
> +	 *   (A) Delete all non-alphanumerics.
> +	 *   (B) Lowercase all letters
> +	 *   (C) Prefix "exm"
> +	 */
> +	for (i = 0, len = 0; serialnr[i]; i++)
> +		if (isalnum(serialnr[i]))
> +			len++;
> +
> +	len += sizeof(PROMPT_PREFIX PROMPT_SUFFIX); /* Includes NUL */
> +	hww1u1a_prompt = malloc(len);
> +	if (!hww1u1a_prompt)
> +		return PROMPT_PREFIX "UNKNOWN(ENOMEM)" PROMPT_SUFFIX;
> +
> +	/* Now actually fill it in */
> +	i = 0;
> +
> +	/* Handle the prefix */
> +	for (j = 0; j < sizeof(PROMPT_PREFIX) - 1; j++)
> +		hww1u1a_prompt[i++] = PROMPT_PREFIX[j];
> +
> +	/* Now the serial# part of the hostname */
> +	for (j = 0; serialnr[j]; j++)
> +		if (isalnum(serialnr[j]))
> +			hww1u1a_prompt[i++] = tolower(serialnr[j]);
> +
> +	/* Finally the suffix */
> +	for (j = 0; j < sizeof(PROMPT_SUFFIX); j++)
> +		hww1u1a_prompt[i++] = PROMPT_SUFFIX[j];
> +
> +	/* This should all have added up, but just in case */
> +	hww1u1a_prompt[len - 1] = '\0';
> +
> +	/* Now we're done */
> +	return hww1u1a_prompt;
> +}
> +
> +#ifndef CONFIG_DDR_SPD
> +phys_size_t fixed_sdram(void)
> +{
> +	/* This is manual (non-SPD) DDR2 SDRAM configuration (2GB ECC) */
> +	volatile ccsr_ddr_t *ddr = (ccsr_ddr_t *)CONFIG_SYS_MPC85xx_DDR_ADDR;
> +	phys_size_t dram_size = (2048ULL) << 20;
> +
> +	/*
> +	 * First program the DDR registers, then allow time for the SDRAM
> +	 * configuration and clocks to settle before enabling everything.
> +	 */
> +	out_be32(&ddr->cs0_bnds,	0x0000003F);
> +	out_be32(&ddr->cs1_bnds,	0x0040007F);
> +	out_be32(&ddr->cs0_config,	0x80014202);
> +	out_be32(&ddr->cs1_config,	0x80014202);
> +	out_be32(&ddr->timing_cfg_0,	0x00330804);
> +	out_be32(&ddr->timing_cfg_1,	0x6f69f643);
> +	out_be32(&ddr->timing_cfg_2,	0x022068cd);
> +	out_be32(&ddr->timing_cfg_3,	0x00030000);
> +	out_be32(&ddr->sdram_cfg_2,	0x04400010);
> +	out_be32(&ddr->sdram_mode,	0x00440452);
> +	out_be32(&ddr->sdram_mode_2,	0x00000000);
> +	out_be32(&ddr->sdram_md_cntl,	0x00000000);
> +	out_be32(&ddr->sdram_interval,	0x0a280110);
> +	out_be32(&ddr->sdram_data_init,	0xDEADBEEF);
> +	out_be32(&ddr->sdram_clk_cntl,	0x02000000);
> +	out_be32(&ddr->timing_cfg_4,	0x00000000);
> +	out_be32(&ddr->timing_cfg_5,	0x00000000);
> +	out_be32(&ddr->ddr_zq_cntl,	0x00000000);
> +	out_be32(&ddr->ddr_wrlvl_cntl,	0x00000000);
> +	out_be32(&ddr->ip_rev1,		0x00020403);
> +	out_be32(&ddr->ip_rev2,		0x00000100);
> +	out_be32(&ddr->err_int_en,	0x0000000D);
> +	out_be32(&ddr->err_disable,	0x00000000);
> +	out_be32(&ddr->err_sbe,		0x00010000);
> +	out_be32(&ddr->ddr_cdr1,	0x00040000);
> +	out_be32(&ddr->ddr_cdr2,	0x00000000);
> +	out_be32(&ddr->sdram_cfg,	0x73000000);
> +	udelay(500);
> +	out_be32(&ddr->sdram_cfg,	0xF3000000);
> +
> +	/* Now wait until memory is initialized */
> +	while (in_be32(&ddr->sdram_cfg_2) & 0x00000010)
> +		udelay(1000);

Please avoid all these magic constants and use symbolic names instead
like other boards are doing.

...
> diff --git a/include/configs/HWW1U1A.h b/include/configs/HWW1U1A.h
> new file mode 100644
> index 0000000..cb8664e
> --- /dev/null
> +++ b/include/configs/HWW1U1A.h
...
> +/************************************************************************
> + * High-level system configuration options				*
> + ************************************************************************/

Incorrect multiline comment style. Please fix globally.

> +/*#define DEBUG 1*/

Please remove dead code.

> +#define CONFIG_BOOKE 1		/* Power/PowerPC Book-E			*/
> +#define CONFIG_E500 1		/* e500 (Power ISA v2.03 with SPE)	*/
> +#define CONFIG_MPC85xx 1	/* MPC8540/60/55/41/48 family		*/
> +#define CONFIG_FSL_ELBC 1	/* FreeScale Enhanced LocalBus Cntlr	*/
> +#define CONFIG_FSL_LAW 1	/* FreeScale Local Access Window	*/
> +#define CONFIG_P2020 1		/* FreeScale P2020			*/
> +#define CONFIG_HWW1U1A 1	/* eXMeritus HardwareWall HWW-1U-1A	*/
> +#define CONFIG_MP 1		/* Multiprocessing support		*/
> +#define CONFIG_HWCONFIG 1	/* Use hwconfig from environment	*/
> +
> +#define CONFIG_L2_CACHE 1		/* L2 cache enabled		*/
> +#define CONFIG_BTB 1			/* Branch predition enabled	*/
> +
> +#define CONFIG_PANIC_HANG 1		/* No board reset on panic	*/
> +#define CONFIG_BOARD_EARLY_INIT_R 1	/* Call board_early_init_r()	*/
> +#define CONFIG_BOARD_RESET_R 1		/* Call board_reset_r()		*/
> +#define CONFIG_CMD_REGINFO 1		/* Dump various CPU regs	*/

Please remove the "1" for all macros that are actually used with
"#ifdef"s.  Please fix globally.

> +#undef CONFIG_CLOCKS_IN_MHZ

Please don't undef what is not defined anyway.

> +#define CONFIG_ENV_SECT_SIZE		   0x20000 /* 128kB (1 flash sector) */
...
> +#define CONFIG_SYS_PCIE3_MEM_SIZE	0x20000000 /* 512M */
> +#define CONFIG_SYS_PCIE2_MEM_SIZE	0x20000000 /* 512M */
> +#define CONFIG_SYS_PCIE1_MEM_SIZE	0x20000000 /* 512M */
> +#define CONFIG_SYS_PCIE3_IO_SIZE	0x00010000 /* 64k */
> +#define CONFIG_SYS_PCIE2_IO_SIZE	0x00010000 /* 64k */
> +#define CONFIG_SYS_PCIE1_IO_SIZE	0x00010000 /* 64k */

If you write such constants as "(128 << 10)", "(512 << 20)" resp. 
"(64 << 10)" they may become better readable.

> + ************************************************************************/
> +#define CONFIG_ENV_SIZE		0x02000 /* 8kB */
> +#define CONFIG_ENV_SIZE_REDUND	0x02000 /* 8kB */

Ditto.

> +/* Don't bother checksumming the flash on boot */
> +#undef	CONFIG_SYS_FLASH_CHECKSUM

Don't undef what is not defined anyway.

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 is such a fine line between genius and stupidity."
- David St. Hubbins, "Spinal Tap"