Re: [U-Boot] [PATCH 09/10] Adding interactive ddr

14 Dec 2010

Dear York Sun,
In message 1291863340-4354-9-git-send-email-yorksun@freescale.com you wrote:
...
Use environment variable to active the interactive debugging.
...
s/active/activate/
...

diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/Makefile b/arch/powerpc/cpu/mpc8xxx/ddr/Makefile
index cb7f856..4bd416a 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/Makefile
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/Makefile
@@ -11,15 +11,15 @@ include $(TOPDIR)/config.mk
 LIB	= $(obj)libddr.a
COBJS-$(CONFIG_FSL_DDR1)	+= main.o util.o ctrl_regs.o options.o \

		   lc_common_dimm_params.o




		   lc_common_dimm_params.o interactive.o



COBJS-$(CONFIG_FSL_DDR1)	+= ddr1_dimm_params.o
COBJS-$(CONFIG_FSL_DDR2)	+= main.o util.o ctrl_regs.o options.o \

		   lc_common_dimm_params.o




		   lc_common_dimm_params.o interactive.o



COBJS-$(CONFIG_FSL_DDR2)	+= ddr2_dimm_params.o
COBJS-$(CONFIG_FSL_DDR3)	+= main.o util.o ctrl_regs.o options.o \

		   lc_common_dimm_params.o




		   lc_common_dimm_params.o interactive.o



COBJS-$(CONFIG_FSL_DDR3)	+= ddr3_dimm_params.o
Building interactive.o should depend on CONFIG_FSL_DDR_INTERACTIVE
being set.
...
SRCS	:= $(START:.o=.S) $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
new file mode 100644
index 0000000..7d492a9
--- /dev/null
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
@@ -0,0 +1,1882 @@
+/*


Copyright 2010 Freescale Semiconductor, Inc.







This program is free software; you can redistribute it and/or



modify it under the terms of the GNU General Public License



Version 2 as published by the Free Software Foundation.


*/

NAK.   V2 or later is mandatory.
...
+static void fsl_ddr_generic_edit(void *pdata,

	   void *pend,


	   unsigned int element_size,


	   unsigned int element_num,


	   unsigned int value)



+{

char *pcdata = (char *)pdata;		/* BIG ENDIAN ONLY */

pcdata += element_num * element_size;
if ((pcdata + element_size) > (char *) pend) {
debug("trying to write past end of data\n");


return;



Should that not be an error message that is always enabled?
...

default:
debug("unexpected element size %u\n", element_size);


break;



Ditto?
...

static const options_strings_t options[] = {
{"cs0_odt_rd_cfg", offsetof(memctl_options_t, cs_local_opts[0].odt_rd_cfg)},


{"cs0_odt_wr_cfg", offsetof(memctl_options_t, cs_local_opts[0].odt_wr_cfg)},


{"cs0_odt_rtt_norm", offsetof(memctl_options_t, cs_local_opts[0].odt_rtt_norm)},


{"cs0_odt_rtt_wr", offsetof(memctl_options_t, cs_local_opts[0].odt_rtt_wr)},


{"cs1_odt_rd_cfg", offsetof(memctl_options_t, cs_local_opts[1].odt_rd_cfg)},


{"cs1_odt_wr_cfg", offsetof(memctl_options_t, cs_local_opts[1].odt_wr_cfg)},


{"cs1_odt_rtt_norm", offsetof(memctl_options_t, cs_local_opts[1].odt_rtt_norm)},


{"cs1_odt_rtt_wr", offsetof(memctl_options_t, cs_local_opts[1].odt_rtt_wr)},


{"cs2_odt_rd_cfg", offsetof(memctl_options_t, cs_local_opts[2].odt_rd_cfg)},


{"cs2_odt_wr_cfg", offsetof(memctl_options_t, cs_local_opts[2].odt_wr_cfg)},


{"cs2_odt_rtt_norm", offsetof(memctl_options_t, cs_local_opts[2].odt_rtt_norm)},


{"cs2_odt_rtt_wr", offsetof(memctl_options_t, cs_local_opts[2].odt_rtt_wr)},


{"cs3_odt_rd_cfg", offsetof(memctl_options_t, cs_local_opts[3].odt_rd_cfg)},


{"cs3_odt_wr_cfg", offsetof(memctl_options_t, cs_local_opts[3].odt_wr_cfg)},


{"cs3_odt_rtt_norm", offsetof(memctl_options_t, cs_local_opts[3].odt_rtt_norm)},


{"cs3_odt_rtt_wr", offsetof(memctl_options_t, cs_local_opts[3].odt_rtt_wr)},



Lines too long.
...

if (handle_uint_option_table(options, nopts, (u32) p,
			optname_str, value_str))


return;



Please use braces for multiline statements.
...

printf("couldn't find option string %s\n", optname_str);

+}



+static void print_fsl_memctl_config_regs(const fsl_ddr_cfg_regs_t *ddr)
+{

unsigned int i;

for (i = 0; i < CONFIG_CHIP_SELECTS_PER_CTRL; i++) {
printf("cs%u_bnds           = %08X\n", i, ddr->cs[i].bnds);


printf("cs%u_config         = %08X\n", i, ddr->cs[i].config);


printf("cs%u_config_2       = %08X\n", i, ddr->cs[i].config_2);


}

printf("timing_cfg_3       = %08X\n", ddr->timing_cfg_3);
printf("timing_cfg_0       = %08X\n", ddr->timing_cfg_0);
printf("timing_cfg_1       = %08X\n", ddr->timing_cfg_1);
printf("timing_cfg_2       = %08X\n", ddr->timing_cfg_2);
printf("ddr_sdram_cfg      = %08X\n", ddr->ddr_sdram_cfg);
printf("ddr_sdram_cfg_2    = %08X\n", ddr->ddr_sdram_cfg_2);
printf("ddr_sdram_mode     = %08X\n", ddr->ddr_sdram_mode);
printf("ddr_sdram_mode_2   = %08X\n", ddr->ddr_sdram_mode_2);
printf("ddr_sdram_mode_3   = %08X\n", ddr->ddr_sdram_mode_3);
printf("ddr_sdram_mode_4   = %08X\n", ddr->ddr_sdram_mode_4);
printf("ddr_sdram_mode_5   = %08X\n", ddr->ddr_sdram_mode_5);
printf("ddr_sdram_mode_6   = %08X\n", ddr->ddr_sdram_mode_6);
printf("ddr_sdram_mode_7   = %08X\n", ddr->ddr_sdram_mode_7);
printf("ddr_sdram_mode_8   = %08X\n", ddr->ddr_sdram_mode_8);
printf("ddr_sdram_interval = %08X\n", ddr->ddr_sdram_interval);
printf("ddr_data_init      = %08X\n", ddr->ddr_data_init);
printf("ddr_sdram_clk_cntl = %08X\n", ddr->ddr_sdram_clk_cntl);
printf("ddr_init_addr      = %08X\n", ddr->ddr_init_addr);
printf("ddr_init_ext_addr  = %08X\n", ddr->ddr_init_ext_addr);
printf("timing_cfg_4       = %08X\n", ddr->timing_cfg_4);
printf("timing_cfg_5       = %08X\n", ddr->timing_cfg_5);
printf("ddr_zq_cntl        = %08X\n", ddr->ddr_zq_cntl);
printf("ddr_wrlvl_cntl     = %08X\n", ddr->ddr_wrlvl_cntl);
printf("ddr_sr_cntr        = %08X\n", ddr->ddr_sr_cntr);
printf("ddr_sdram_rcw_1    = %08X\n", ddr->ddr_sdram_rcw_1);
printf("ddr_sdram_rcw_2    = %08X\n", ddr->ddr_sdram_rcw_2);
printf("ddr_cdr1           = %08X\n", ddr->ddr_cdr1);
printf("ddr_cdr2           = %08X\n", ddr->ddr_cdr2);
printf("err_disable        = %08X\n", ddr->err_disable);
printf("err_int_en         = %08X\n", ddr->err_int_en);
for (i = 0; i < 18; i++)
printf("debug_%02d	= %08X\n", i+1, ddr->debug[i]);



+}



+static void fsl_ddr_regs_edit(fsl_ddr_info_t *pinfo,

	unsigned int ctrl_num,


	const char *regname,


	unsigned int value)



+{

unsigned int i;
fsl_ddr_cfg_regs_t *ddr;
char buf[20];

debug("fsl_ddr_regs_edit: ctrl_num = %u, "
"regname = %s, value = 0x%08X\n",


ctrl_num, regname, value);


if (ctrl_num > CONFIG_NUM_DDR_CONTROLLERS)
return;



/* FIXME: Change this into struct like the other editing functions */
ddr = &(pinfo->fsl_ddr_config_reg[ctrl_num]);

for (i = 0; i < CONFIG_CHIP_SELECTS_PER_CTRL; i++) {
sprintf(buf, "cs%u_bnds", i);


if (strcmp(buf, regname) == 0) {


	ddr->cs[i].bnds = value;


	return;


}



sprintf(buf, "cs%u_config", i);


if (strcmp(buf, regname) == 0) {


	ddr->cs[i].config = value;


	return;


}



sprintf(buf, "cs%u_config_2", i);


if (strcmp(buf, regname) == 0) {


	ddr->cs[i].config_2 = value;


	return;


}


}

Use format string / pointer table and a loop.
...

if (strcmp("timing_cfg_3", regname) == 0) {
ddr->timing_cfg_3 = value;


return;


}

if (strcmp("timing_cfg_0", regname) == 0) {
ddr->timing_cfg_0 = value;


return;


}

if (strcmp("timing_cfg_1", regname) == 0) {
ddr->timing_cfg_1 = value;


return;


}

if (strcmp("timing_cfg_2", regname) == 0) {
ddr->timing_cfg_2 = value;


return;


}

if (strcmp("ddr_sdram_cfg", regname) == 0) {
ddr->ddr_sdram_cfg = value;


return;


}

if (strcmp("ddr_sdram_cfg_2", regname) == 0) {
ddr->ddr_sdram_cfg_2 = value;


return;


}

if (strcmp("ddr_sdram_mode", regname) == 0) {
ddr->ddr_sdram_mode = value;


return;


}

if (strcmp("ddr_sdram_mode_2", regname) == 0) {
ddr->ddr_sdram_mode_2 = value;


return;


}

if (strcmp("ddr_sdram_mode_3", regname) == 0) {
ddr->ddr_sdram_mode_3 = value;


return;


}

if (strcmp("ddr_sdram_mode_4", regname) == 0) {
ddr->ddr_sdram_mode_4 = value;


return;


}

if (strcmp("ddr_sdram_mode_5", regname) == 0) {
ddr->ddr_sdram_mode_5 = value;


return;


}

if (strcmp("ddr_sdram_mode_6", regname) == 0) {
ddr->ddr_sdram_mode_6 = value;


return;


}

if (strcmp("ddr_sdram_mode_7", regname) == 0) {
ddr->ddr_sdram_mode_7 = value;


return;


}

if (strcmp("ddr_sdram_mode_8", regname) == 0) {
ddr->ddr_sdram_mode_8 = value;


return;


}

if (strcmp("ddr_sdram_interval", regname) == 0) {
ddr->ddr_sdram_interval = value;


return;


}

if (strcmp("ddr_data_init", regname) == 0) {
ddr->ddr_data_init = value;


return;


}

if (strcmp("ddr_sdram_clk_cntl", regname) == 0) {
ddr->ddr_sdram_clk_cntl = value;


return;


}

if (strcmp("ddr_init_addr", regname) == 0) {
ddr->ddr_init_addr = value;


return;


}

if (strcmp("ddr_init_ext_addr", regname) == 0) {
ddr->ddr_init_ext_addr = value;


return;


}

if (strcmp("timing_cfg_4", regname) == 0) {
ddr->timing_cfg_4 = value;


return;


}

if (strcmp("timing_cfg_5", regname) == 0) {
ddr->timing_cfg_5 = value;


return;


}

if (strcmp("ddr_zq_cntl", regname) == 0) {
ddr->ddr_zq_cntl = value;


return;


}

if (strcmp("ddr_wrlvl_cntl", regname) == 0) {
ddr->ddr_wrlvl_cntl = value;


return;


}

if (strcmp("ddr_sr_cntr", regname) == 0) {
ddr->ddr_sr_cntr = value;


return;


}

if (strcmp("ddr_sdram_rcw_1", regname) == 0) {
ddr->ddr_sdram_rcw_1 = value;


return;


}

if (strcmp("ddr_sdram_rcw_2", regname) == 0) {
ddr->ddr_sdram_rcw_2 = value;


return;


}

if (strcmp("ddr_cdr1", regname) == 0) {
ddr->ddr_cdr1 = value;


return;


}

if (strcmp("ddr_cdr2", regname) == 0) {
ddr->ddr_cdr2 = value;


return;


}

if (strcmp("err_disable", regname) == 0) {
ddr->err_disable = value;


return;


}

if (strcmp("err_int_en", regname) == 0) {
ddr->err_int_en = value;


return;


}

Use string / pointer table and a loop.
...
...
+#define PRINT_NXS(x, y, z) printf("%-3d    : %02x %s\n", x, y, z);
+#define PRINT_NNXXS(n0, n1, x0, x1, s) printf("%-3d-%3d: %02x %02x %s\n", n0, n1, x0, x1, s);
+#define PRINT_SNNlots(x, y, z, arr) do {printf(x); printf("%-3d-%3d: ", y, z); for (i = y; i <= z; i++) printf("%02x", arr[i - y]); } while (0)
Lines way too long; please fix globally.
...
...

		if (strcmp(argv[i], "dimmparms") == 0) {


			step_mask |= STEP_COMPUTE_DIMM_PARMS;


			continue;


		}



		if (strcmp(argv[i], "commonparms") == 0) {


			step_mask |= STEP_COMPUTE_COMMON_PARMS;


			continue;


		}



		if (strcmp(argv[i], "opts") == 0) {


			step_mask |= STEP_GATHER_OPTS;


			continue;


		}



		if (strcmp(argv[i], "addresses") == 0) {


			step_mask |= STEP_ASSIGN_ADDRESSES;


			continue;


		}



		if (strcmp(argv[i], "regs") == 0) {


			step_mask |= STEP_COMPUTE_REGS;


			continue;


		}



Here again the code could be made much smaller and easier to read by
using a data (table) driven approach and a loop through the table.
Please consider using this technique globally.
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@denx.de
But it's real. And if it's real it can be affected ... we may not  be
able  to break it, but, I'll bet you credits to Navy Beans we can put
a dent in it.
    -- deSalle, "Catspaw", stardate 3018.2

    