[U-Boot] [PATCH 2/3] FSL DDR: Rewrite the FSL mpc8xxx DDR controller setup code.

[Wolfgang Denk <wd@denx.de>]

Dear Kumar Gala,

In message <1218603638-21473-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 |   55 ++
>  cpu/mpc8xxx/ddr2_dimm_params.h     |   88 +++
>  cpu/mpc8xxx/fsl_ddr1.c             |  362 ++++++++++
>  cpu/mpc8xxx/fsl_ddr2.c             |  363 ++++++++++
>  cpu/mpc8xxx/fsl_ddr_ctrl.c         | 1155 ++++++++++++++++++++++++++++++++
>  cpu/mpc8xxx/fsl_ddr_sdram.c        | 1280 ++++++++++++++++++++++++++++++++++++
>  cpu/mpc8xxx/fsl_ddr_sdram.h        |   91 +++
>  cpu/mpc8xxx/fsl_memctrl.h          |   45 ++
>  cpu/mpc8xxx/memctl_options.h       |   85 +++
>  12 files changed, 3567 insertions(+), 0 deletions(-)
>  create mode 100644 cpu/mpc8xxx/Makefile
>  create mode 100644 cpu/mpc8xxx/common_timing_params.h
>  create mode 100644 cpu/mpc8xxx/ddr2_dimm_params.h
>  create mode 100644 cpu/mpc8xxx/fsl_ddr1.c
>  create mode 100644 cpu/mpc8xxx/fsl_ddr2.c
>  create mode 100644 cpu/mpc8xxx/fsl_ddr_ctrl.c
>  create mode 100644 cpu/mpc8xxx/fsl_ddr_sdram.c
>  create mode 100644 cpu/mpc8xxx/fsl_ddr_sdram.h
>  create mode 100644 cpu/mpc8xxx/fsl_memctrl.h
>  create mode 100644 cpu/mpc8xxx/memctl_options.h

Coding Style: maximum line length exceeded; some of your lines are
more than 125 characters long!

-> sed -n 's/^+//p' /tmp/patch | expand | awk 'length > 80'
                                                "for memctl=%u dimm=%u\n", i, j);
                                for (j = 0; j < CONFIG_DIMM_SLOTS_PER_CTLR; j++) {
                                        dw = pinfo->dimm_params[i][j].data_width;
                                for (j = 0; j < CONFIG_DIMM_SLOTS_PER_CTLR; j++) {
                                        pinfo->dimm_params[i][j].base_address = addr;
                                        addr += (phys_addr_t)(pinfo->dimm_params[i][j].dimm_capacity >> dbw_capacity_adjust[i]);
                                pinfo->common_timing_params[i].base_address = (phys_addr_t)cur_memsize;
                                for (j = 0; j < CONFIG_DIMM_SLOTS_PER_CTLR; j++) {
                                        pinfo->dimm_params[i][j].base_address = (phys_addr_t)cur_memsize;
                                        cur_memsize+= pinfo->dimm_params[i][j].dimm_capacity >> dbw_capacity_adjust[i];
                                        total_mem_per_memctl += pinfo->dimm_params[i][j].dimm_capacity >> dbw_capacity_adjust[i];
                                pinfo->common_timing_params[i].total_mem = total_mem_per_memctl;
                debug("FSL Memory controller configuration register computation\n");
                        if (pinfo->common_timing_params[i].ndimms_present == 0) {
                        total_memory += pinfo->common_timing_params[i].total_mem;
                        printf("This U-Boot only supports less than 4G of DDR.\n");
                        printf("You could rebuild it using CONFIG_PHYS_64BIT.\n");


> +unsigned int
> +ddr_compute_dimm_parameters(const ddr2_spd_eeprom_t *spd,
> +			     dimm_params_t *pdimm,
> +			     unsigned int dimm_number)
> +{
> +	unsigned int retval;
> +
> +	/*
> +	 * FIXME debate: shouldn't have to dynamically determine memory type
> +	 * This ought to be determined through a config #define
> +	 */
...
> +	{
> +		unsigned char dimm_type = spd->dimm_type;
> +
> +		/* FIXME: what about registered SO-DIMM? */

Coding style - please do not declare variables in the middle of a
function.

Get rid of this extra block.

> +static int fsl_ddr_sdram_get_rtt(void)
> +{
> +	int rtt;
> +
> +#if defined(CONFIG_FSL_DDR1)
> +	rtt = 0;
> +#elif defined(CONFIG_FSL_DDR2)
> +	rtt = 3;
> +#else
> +#error "Need Rtt value for DDR3"
> +#endif
> +
> +	return rtt;
> +}

Make inline?

> +/* Chip Select Configuration (CSn_CONFIG) */
> +static void set_csn_config(int i, fsl_memctl_config_regs_t *ddr,
> +			       const memctl_options_t *popts,
> +			       const dimm_params_t *dimm_parameters)
> +{
> +	/* CS_n_EN: Chip Select enable */
> +	unsigned int cs_n_en;
> +	/* INTLV_EN: Memory controller interleave enable */
> +	unsigned int intlv_en;
> +	/* INTLV_CTL: Interleaving control */
> +	unsigned int intlv_ctl;
> +	/* AP_n_EN: Chip select n auto-precharge enable */
> +	unsigned int ap_n_en;
> +	/* ODT_RD_CFG: ODT for reads configuration */
> +	unsigned int odt_rd_cfg;
> +	/* ODT_WR_CFG: ODT for writes configuration */
> +	unsigned int odt_wr_cfg;
> +	/* BA_BITS_CS_n: Number of bank bits for SDRAM on chip select n */
> +	unsigned int ba_bits_cs_n;
> +	/* ROW_BITS_CS_n: Number of row bits for SDRAM on chip select n */
> +	unsigned int row_bits_cs_n;
> +	/* COL_BITS_CS_n: Number of ocl bits for SDRAM on chip select n */
> +	unsigned int col_bits_cs_n;
> +
> +	cs_n_en = 0;
> +	intlv_en = 0;
> +	intlv_ctl = 0;
> +	ap_n_en = 0;
> +	odt_rd_cfg = 0;
> +	odt_wr_cfg = 0;
> +	ba_bits_cs_n = 0;
> +	row_bits_cs_n = 0;
> +	col_bits_cs_n = 0;

Why don't you initialize the variables right in the declaration?

> +	/* Compute CS_CONFIG only for existing ranks of each DIMM.  */
> +	if ((((i&1) == 0)
> +	     && (dimm_parameters[i/2].n_ranks == 1))
> +	    || (dimm_parameters[i/2].n_ranks == 2)) {
> +		cs_n_en = 1;
> +		unsigned int n_banks_per_sdram_device;

Coding style: Do not declare variables in the middle of the code.


> +	ddr->cs[i].config = (0
> +		| ((cs_n_en & 0x1) << 31)
> +		| ((intlv_en & 0x3) << 29)
> +		| ((intlv_en & 0xf) << 24)
> +		| ((ap_n_en & 0x1) << 23)
> +
> +		/*
> +		 * XXX: some implementation only have 1 bit
> +		 * starting at left.
> +		 */
> +		| ((odt_rd_cfg & 0x7) << 20)
> +
> +		/*
> +		 * FIXME: Some implementation
> +		 * only have 1 bit starting at left.
> +		 */
> +		| ((odt_wr_cfg & 0x7) << 16)
> +
> +		| ((ba_bits_cs_n & 0x3) << 14)
> +		| ((row_bits_cs_n & 0x7) << 8)
> +		| ((col_bits_cs_n & 0x7) << 0)
> +		);
> +}

Please do not interrupt the code with multiline comments!


> +	/*
> +	 * FIXME: Are these configurable?
> +	 */
> +	trwt_mclk = 0;		/* RWT: Read-to-write turnaround */
> +	twrt_mclk = 0;		/* WRT: Write-to-read turnaround */
> +		/* 7.5 ns on -3E; 0 means WL - CL + BL/2 + 1 */

Fix indentation.

> +static void set_timing_cfg_3(fsl_memctl_config_regs_t *ddr,
> +			       const common_timing_params_t *common_dimm)
> +{
...
> +	ddr->timing_cfg_3 = (0
> +		| ((ext_refrec & 0x7) << 16)	/* EXT_ACTTOPRE */
> +		);

Write as a single line withouut the "0 | "

> +#if defined(CONFIG_FSL_DDR1)
> +		wr = 0;       /* Historical */
> +#elif defined(CONFIG_FSL_DDR2)
> +		{
> +			const unsigned int mclk_ps

Do not declare variables in the middle of a function.

> +unsigned int
> +compute_fsl_memctl_config_regs(const memctl_options_t *popts,
> +			       fsl_memctl_config_regs_t *ddr,
> +			       const common_timing_params_t *common_dimm,
> +			       const dimm_params_t *dimm_parameters,
> +			       unsigned int dbw_capacity_adjust)
> +{
...
> +		if (popts->memctl_interleaving && popts->ba_intlv_ctl) {
> +			/*
> +			 * This works superbank 2CS
> +			 * There are 2 memory controllers configured
> +			 * identically, memory is interleaved between them,
> +			 * and each controller uses rank interleaving within
> +			 * itself. Therefore the starting and ending address
> +			 * on each controller is twice the amount present on
> +			 * each controller.
> +			 */
> +			ea = (2 *
> +				common_dimm->total_mem >>
> +					 dbw_capacity_adjust) - 1;
> +		}
> +		else if (!popts->memctl_interleaving && popts->ba_intlv_ctl) {
> +			/*
> +			 * If memory interleaving between controllers is NOT
> +			 * enabled, the starting address for each memory
> +			 * controller is distinct.  However, because rank
> +			 * interleaving is enabled, the starting and ending
> +			 * addresses of the total memory on that memory
> +			 * controller needs to be programmed into its
> +			 * respective CS0_BNDS.
> +			 */
> +			sa = common_dimm->base_address;
> +			ea = sa +
> +				(common_dimm->total_mem >> dbw_capacity_adjust)
> +				- 1;
> +		}
> +		else if (popts->memctl_interleaving && !popts->ba_intlv_ctl) {

etc. 

Coding style: make this "} else if (...) {" (on one line).

> +			if (i == 0) {
> +				ea = (2 *
> +					(dimm_parameters[0].rank_density >>
> +						 dbw_capacity_adjust)) - 1;

This is very difficult to read.

> +unsigned int
> +populate_memctl_options(const common_timing_params_t *pcommon_params,
> +			memctl_options_t *popts,
> +			unsigned int ctrl_num)
> +{
...
> +	/*
> +	 * CAS latency plus additive latency must be at least 3 cycles
> +	 * for ODT_RD_CFG to be enabled.
> +	 */
> +#if 0
> +	/* This check should be in fsl ddr regs. */
> +	for (i = 0; i < CONFIG_CHIP_SELECTS_PER_CTRL; i++) {
> +		if (popts->cs_local_opts[i].odt_rd_cfg) {
> +			if ((popts->cas_latency_override_value +
> +				popts->additive_latency_override_value) < 3) {
> +				printf("CHECKME:  CAS latency and Additive "
> +				" latency must be at least 3 cycles for "
> +				"ODT_RD_CFG to be enabled "
> +				"(chip select = %u)\n", i);
> +			}
> +		}
> +	}
> +#endif
> +
> +#if 0
...

Please remove dead code.

> +unsigned int
> +check_fsl_memctl_config_regs(const fsl_memctl_config_regs_t *ddr)
> +{
...
> +			for (i = 0; i < CONFIG_NUM_DDR_CONTROLLERS; i++) {
> +				addr = 0;
> +/* pinfo->common_timing_params[i].base_address = addr; */

Indendation not correct.



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 at denx.de
You humans have that emotional need  to  express  gratitude.  "You're
welcome," I believe, is the correct response.
	-- Spock, "Bread and Circuses", stardate 4041.2