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

[Kumar Gala]

On Aug 29, 2008, at 5:16 PM, Peter Tyser wrote:

> 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?

Now that the code is unified its will be easier to clean this up.  I  
need to look at the ECC bits a little more to provide a better answer.

- k