[U-Boot] [PATCH 1/1 v2] ppc4xx: Autocalibration can set RDCC to over aggressive value.

[Wolfgang Denk]

Dear Adam Graham,

In message <1231887261-30490-1-git-send-email-agraham at amcc.com> you wrote:
>
> --- a/cpu/ppc4xx/4xx_ibm_ddr2_autocalib.c
> +++ b/cpu/ppc4xx/4xx_ibm_ddr2_autocalib.c
> @@ -61,6 +61,12 @@
>  #define NUMLOOPS		1	/* configure as you deem approporiate */
>  #define NUMMEMWORDS		16
>  
> +#define DDR_VERBOSE_PRT(dbg, lvl, fmt, args...) \
> +	do { \
> +	if (lvl > dbg) \
> +		printf(fmt, ##args); \
> +	} while (0)
> +

We have a creaping featurism here. I  seriously  doubt  if  all  this
debug  code  is really needed in a production release. I recommend to
get rid of all this dynamic run-time verbosity  switching  code,  and
simply use debug() resp. debugX() instead. Keep in mind that this is a
boot loader, and memory footprint counts.

[Also, indentation was wrong, too.]

...
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best result: 0x%04x\n",
> +					wdtr, clkp, best_result);
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best WRDTR: 0x%04x\n",
> +					wdtr, clkp, tcal.clocks.wrdtr);
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best CLKTR: 0x%04x\n",
> +					wdtr, clkp, tcal.clocks.clktr);
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best RQDC: 0x%04x\n",
> +					wdtr, clkp, tcal.autocal.rqfd);
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best RFDC: 0x%04x\n",
> +					wdtr, clkp, tcal.autocal.rffd);
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best RDCC: 0x%08x\n",
> +					wdtr, clkp, tcal.clocks.rdcc);
> +				mfsdram(SDRAM_RTSR, val);
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best loop RTSR: 0x%08x\n",
> +					wdtr, clkp, val);
> +				mfsdram(SDRAM_FCSR, val);
> +				DDR_VERBOSE_PRT(2, verbose_lvl,
> +					"** (%d)(%d)  best loop FCSR: 0x%08x\n",
> +					wdtr, clkp, val);

In your implementation above - does it really make sense to test  the
verbosity  level  against  the very same constant again and again and
again? I don't think so. This code is just difficult to read.

Please simplyfy. All this complexity of debug code has no place in the
production version.

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
"It's when they say 2 + 2 = 5 that I begin to argue."    - Eric Pepke