Dear Kumar Gala,
In message <[EMAIL PROTECTED]> you wrote:
>
> Signed-off-by: James Yang <[EMAIL PROTECTED]>
> Signed-off-by: Jon Loeliger <[EMAIL PROTECTED]>
> Signed-off-by: Becky Bruce <[EMAIL PROTECTED]>
> Signed-off-by: Ed Swarthout <[EMAIL PROTECTED]>
> Signed-off-by: Kumar Gala <[EMAIL PROTECTED]>
> ---
> 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: [EMAIL PROTECTED]
I perceive a possibility of an immediate chronological sequence of
events which includes a violence.
- Terry Pratchett, _The Dark Side of the Sun_
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
