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

[Peter Tyser <ptyser@xes-inc.com>]

Hello,

[snip]

> +
> +/* DDR SDRAM control configuration 2 (DDR_SDRAM_CFG_2) */
> +static void set_ddr_sdram_cfg_2(fsl_memctl_config_regs_t *ddr,
> +			       const memctl_options_t *popts)
> +{
> +	unsigned int frc_sr = 0;	/* Force self refresh */
> +	unsigned int sr_ie = 0;		/* Self-refresh interrupt enable */
> +	unsigned int dll_rst_dis;	/* DLL reset disable */
> +	unsigned int dqs_cfg;		/* DQS configuration */
> +	unsigned int odt_cfg;		/* ODT configuration */
> +	unsigned int num_pr;		/* Number of posted refreshes */
> +	unsigned int obc_cfg;		/* On-The-Fly Burst Chop Cfg */
> +	unsigned int ap_en;		/* Address Parity Enable */
> +	unsigned int d_init;		/* DRAM data initialization */
> +	unsigned int rcw_en = 0;	/* Register Control Word Enable */
> +	unsigned int md_en = 0;		/* Mirrored DIMM Enable */
> +
> +	dll_rst_dis = 1;	/* Make this configurable */
> +	dqs_cfg = popts->DQS_config;
> +	if (popts->cs_local_opts[0].odt_rd_cfg
> +	    || popts->cs_local_opts[0].odt_wr_cfg) {
> +		/* FIXME */
> +		odt_cfg = 2;
> +	} else {
> +		odt_cfg = 0;
> +	}
> +
> +	num_pr = 1;	/* Make this configurable */
> +
> +	/*
> +	 * 8572 manual says
> +	 *     {TIMING_CFG_1[PRETOACT]
> +	 *      + [DDR_SDRAM_CFG_2[NUM_PR]
> +	 *        * ({EXT_REFREC || REFREC} + 8 + 2)]}
> +	 *      << DDR_SDRAM_INTERVAL[REFINT]
> +	 */
> +
> +	obc_cfg = 0;	/* Make this configurable? */
> +	ap_en = 0;	/* Make this configurable? */
> +
> +#if defined(CONFIG_ECC_INIT_VIA_DDRCONTROLLER)
> +	/* Use the DDR controller to auto initialize memory. */
> +	d_init = 1;
> +	ddr->ddr_data_init = CONFIG_MEM_INIT_VALUE;
> +	debug("DDR: ddr_data_init = 0x%08x\n", ddr->ddr_data_init);
> +#else
> +	/* Memory will be initialized via DMA, or not at all. */
> +	d_init = 0;
> +#endif
> +

I'm using the current head (33aa4eac66b71c797bbc13b3afe432a2132947d4) on
a mpc8572-based board.  It uses DDR2 and has support for ECC.  While
enabling ECC support, I noticed that the "old"
CONFIG_ECC_INIT_VIA_DDRCONTROLLER define is still being used to enable
memory initialization on bootup  while a new ECC_init_using_memctl field
in the memctl_options_s structure is also present.

Currently it looks like ECC_init_using_memctl doesn't do anything and is
only referenced in one location below:

> +	/* Pick ECC modes */
> +#ifdef CONFIG_DDR_ECC
> +	popts->ECC_mode = 1;		  /* 0 = disabled, 1 = enabled */
> +#else
> +	popts->ECC_mode = 0;		  /* 0 = disabled, 1 = enabled */
> +#endif
> +	popts->ECC_init_using_memctl = 1; /* 0 = use DMA, 1 = use memctl */

The intended functionality of CONFIG_ECC_INIT_VIA_DDRCONTROLLER and
ECC_init_using_memctl seem to be the same, but only one of them is being
used, and they currently are unrelated which is a bit confusing.

Should the ECC_init_using_memctl field be removed, or is the intention
to replace CONFIG_ECC_INIT_VIA_DDRCONTROLLER functionality with
ECC_init_using_memctl as some point?

Thanks for any insight,
Peter