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