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