[U-Boot] [PATCH 02/14] FSL DDR: Rewrite the FSL mpc8xxx DDR controller setup code.

[Wolfgang Denk <wd@denx.de>]

Dear Kumar Gala,

In message <1218557182-1328-2-git-send-email-galak@kernel.crashing.org> you wrote:
> 
> Signed-off-by: James Yang <James.Yang@freescale.com>
> Signed-off-by: Jon Loeliger <jdl@freescale.com>
> Signed-off-by: Becky Bruce <becky.bruce@freescale.com>
> Signed-off-by: Ed Swarthout <Ed.Swarthout@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
>  cpu/mpc85xx/Makefile               |    9 +
>  cpu/mpc86xx/Makefile               |    4 +
>  cpu/mpc8xxx/Makefile               |   30 +
>  cpu/mpc8xxx/common_timing_params.h |   54 +
>  cpu/mpc8xxx/ddr2_dimm_params.h     |   99 ++
>  cpu/mpc8xxx/fsl_ddr1.c             |  383 ++++++
>  cpu/mpc8xxx/fsl_ddr2.c             |  384 ++++++
>  cpu/mpc8xxx/fsl_ddr_sdram.c        | 2537 ++++++++++++++++++++++++++++++++++++

My comments will focus on this file - this is a monster of a file.

...
> +		/*
> +		 * Find maximum tCKmin_X_ps to find slowest DIMM.
> +		 */
> +		tCKmin_X_ps = max(tCKmin_X_ps, dimm_params[i].tCKmin_X_ps);
> +
> +		/*
> +		 * Find minimum tCKmax_ps to find fastest slow speed,
> +		 * i.e., this is the slowest the whole system can go.
> +		 */
> +		tCKmax_ps = min(tCKmax_ps, dimm_params[i].tCKmax_ps);
> +
> +		/*
> +		 * Find maximum tCKmax_ps to find slowest slow speed,
> +		 * i.e., this is the slowest any dimm can go.
> +		 */
> +		tCKmax_max_ps = max(tCKmax_max_ps, dimm_params[i].tCKmax_ps);
> +
> +		/*
> +		 * Find maximum tRCD_ps to find slowest ras-to-cas delay.
> +		 */
> +		tRCD_ps = max(tRCD_ps, dimm_params[i].tRCD_ps);
> +
> +		/*
> +		 * Find maximum tRP_ps to find slowest row precharge time.
> +		 */
> +		tRP_ps = max(tRP_ps, dimm_params[i].tRP_ps);

This goes on for hundrets of lines.

Please change ALL such single-line comments into one-liners.

Also:

> +
> +		/*
> +		 * Find maximum tRAS_ps to find slowest active to
> +		 * precharge time.
> +		 */
> +		tRAS_ps = max(tRAS_ps, dimm_params[i].tRAS_ps);
> +
> +		/*
> +		 * Find maximum tWR_ps to find slowest write recovery time.
> +		 */
> +		tWR_ps = max(tWR_ps, dimm_params[i].tWR_ps);
> +
> +		/*
> +		 * Find maximum tWTR_ps to find slowest write-to-read
> +		 * command delay.
> +		 */
> +		tWTR_ps = max(tWTR_ps, dimm_params[i].tWTR_ps);
> +
> +		/*
> +		 * Find maximum tRFC_ps to find slowest auto-refresh to
> +		 * active/auto refresh command period.
> +		 */
> +		tRFC_ps = max(tRFC_ps, dimm_params[i].tRFC_ps);
> +
> +		/*
> +		 * Find maximum tRRD_ps to find slowest row active to
> +		 * row active delay.
> +		 */
> +		tRRD_ps = max(tRRD_ps, dimm_params[i].tRRD_ps);
> +
> +		/*
> +		 * Find maximum tRC_ps to find slowest
> +		 * active/auto-refresh time.
> +		 */
> +		tRC_ps = max(tRC_ps, dimm_params[i].tRC_ps);
> +
> +		/*
> +		 * Find maximum refresh_rate_ps to find slowest refresh time.
> +		 */
> +		refresh_rate_ps = max(refresh_rate_ps,
> +				      dimm_params[i].refresh_rate_ps);
> +
> +		/*
> +		 * Find maximum tIS_ps to find slowest.
> +		 */
> +		tIS_ps = max(tIS_ps, dimm_params[i].tIS_ps);
> +
> +		/*
> +		 * Find maximum tIH_ps to find slowest.
> +		 */
> +		tIH_ps = max(tIH_ps, dimm_params[i].tIH_ps);
> +
> +		/*
> +		 * Find maximum tDS_ps to find slowest.
> +		 */
> +		tDS_ps = max(tDS_ps, dimm_params[i].tDS_ps);
> +
> +		/*
> +		 * Find maximum tDH_ps to find slowest.
> +		 */
> +		tDH_ps = max(tDH_ps, dimm_params[i].tDH_ps);
> +
> +		/*
> +		 * Find maximum tRTP_ps to find slowest.
> +		 */
> +		tRTP_ps = max(tRTP_ps, dimm_params[i].tRTP_ps);
> +
> +		 *
> +		 * FIXME: is finding the slowest value the correct
> +		 * strategy for this parameter?
> +		 */
> +		tDQSQ_max_ps = max(tDQSQ_max_ps, dimm_params[i].tDQSQ_max_ps);
> +
> +		/*
> +		 * Find maximum tQHS_ps to find slowest.
> +		 */
> +		tQHS_ps = max(tQHS_ps, dimm_params[i].tQHS_ps);

Do we really need a  "find  maximum  FOO  to  find  slowest"  (what?)
comment  for  each and every of these similar lines? A single comment
at the beginning of the block should do.

> +	/* CHECKME: */
> +	if (!outpdimm->all_DIMMs_registered
> +	    && !outpdimm->all_DIMMs_unbuffered) {
> +		printf("ERROR:  Mix of registered buffered and unbuffered DIMMs detected!\n");
> +	}
...
> +			if (dimm_params[i].caslat_X == temp2) {
> +				if (mclk_ps >= dimm_params[i].tCKmin_X_ps) {
> +					debug("CL = %u ok on DIMM %u at tCK=%u ps with its tCKmin_X_ps of %u\n",
> +					       temp2, i, mclk_ps,
> +					       dimm_params[i].tCKmin_X_ps);
> +					continue;
> +				} else {
> +					not_ok++;
> +				}
> +			}
> +
> +			if (dimm_params[i].caslat_X_minus_1 == temp2) {
> +				if (mclk_ps >= dimm_params[i].tCKmin_X_minus_1_ps) {
> +					debug("CL = %u ok on DIMM %u at tCK=%u ps with its tCKmin_X_minus_1_ps of %u\n",
> +					       temp2, i, mclk_ps, dimm_params[i].tCKmin_X_minus_1_ps);

etc. Mind the maximum line length. (LCS: "The limit on the length of
lines is 80 columns and this is a strongly preferred limit.")

There are functions in this file that span for nearly 500 lines of
code - LCS says: "Functions should be short and sweet, and do just one
thing.  They should fit on one or two screenfuls of text (the ISO/ANSI
screen size is 80x24, as we all know), and do one thing and do that
well."

I think this file needs a rework. 

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
I perceive a possibility of an immediate  chronological  sequence  of
events which includes a violence.
                        - Terry Pratchett, _The Dark Side of the Sun_