From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Denk Date: Wed, 13 Aug 2008 11:26:57 +0200 Subject: [U-Boot] [PATCH 2/3] FSL DDR: Rewrite the FSL mpc8xxx DDR controller setup code. In-Reply-To: <1218603638-21473-2-git-send-email-galak@kernel.crashing.org> References: <1218603638-21473-1-git-send-email-galak@kernel.crashing.org> <1218603638-21473-2-git-send-email-galak@kernel.crashing.org> Message-ID: <20080813092657.D6C2F248AC@gemini.denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Kumar Gala, In message <1218603638-21473-2-git-send-email-galak@kernel.crashing.org> you wrote: > Signed-off-by: James Yang > Signed-off-by: Jon Loeliger > Signed-off-by: Becky Bruce > Signed-off-by: Ed Swarthout > Signed-off-by: Kumar Gala > --- > 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