[U-Boot] [PATCH 2/3] FSL DDR: Rewrite the FSL mpc8xxx DDR controller setup code.
Wolfgang Denk
wd at denx.de
Wed Aug 13 11:26:57 CEST 2008
Dear Kumar Gala,
In message <1218603638-21473-2-git-send-email-galak at kernel.crashing.org> you wrote:
> Signed-off-by: James Yang <James.Yang at freescale.com>
> Signed-off-by: Jon Loeliger <jdl at freescale.com>
> Signed-off-by: Becky Bruce <becky.bruce at freescale.com>
> Signed-off-by: Ed Swarthout <Ed.Swarthout at freescale.com>
> Signed-off-by: Kumar Gala <galak at 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
More information about the U-Boot
mailing list