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

[Wolfgang Denk]

Dear "Moffett, Kyle D",

In message <475AB4E4-F287-44E3-8A16-213506955843 at boeing.com> you wrote:
> 
...
> >> +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.
>
> Ok, is just " && echo This is CPU A\n" OK?  I'm trying to show possible 
> usage of the command and it has no actual arguments or output on its own.

No, usage documentation is not supposed to be found here. We just
provide the synopsis.

> >> +	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.
...
> I only use this code for debugging so if the numeric constants are
> completely unacceptable I will just delete it rather than try to rewrite
> it to make everybody happy.

If you prefer so...

> >> > +/************************************************************************>
> >> + * High-level system configuration options				> *
> >> + > ************************************************************************/
> > 
> > Incorrect multiline comment style. Please fix globally.
>
> Hmm, the intent was to group config options into "sections" of a sort
> (such as I2C, PCI, networking, etc).
>
> Is there a better syntax for me to use or am I not allowed to do that?

Use two blank lines as block separator?

> > Please remove the "1" for all macros that are actually used with
> > "#ifdef"s.  Please fix globally.
>
> I added all of the "1"s after I experienced random build breakage from a
> couple config variables being "#ifdef" in one place and "#if" in
> another.  Is there a convenient list somewhere of which should be which?

Such code should be fixed.  Do you remember where this was?

Sorry, I don;t have such a list, but in generall all "features" are
only "ifdef"; only where a numeric value is really needed (like UART
index or similar) a "#define name number" makes sense.

> > 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.
>
> This was also copied from P2020DS.
> These are address-space sizes next to the actual base addresses, so I
> would rather leave them in this form to make them easier to correlate
> against the base addresses.

I cannot really follow this argument, but I don't have to maintain
that code so I don't insist.

> Also, the environment sector size (with comment about flash sector)
> makes it much more obvious that 0xefff6000 specified later on is exactly
> one sector prior to the 0xefff8000 base address of the U-Boot image.

As it is not really obvious that the difference of a sector sice is
not just a coincidence you could improve readability by using

#define CONFIG_ENV_ADDR (CONFIG_SYS_TEXT_BASE - CONFIG_ENV_SECT_SIZE)
#define CONFIG_ENV_ADDR_REDUND (CONFIG_ENV_ADDR - CONFIG_ENV_SECT_SIZE)

Also, instead of adding a comment "Must be == MONITOR_BASE" (which is
actually wrong, as TEXT_BASE is the reference) you could easily
guarantee this by writing

#define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_TEXT_BASE



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
Our business is run on trust.  We trust you will pay in advance.