arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
--- arch/arm/cpu/armv7/mx6/ddr.c | 559 ++++++++++++++++++++++++++++++++ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 5 + 2 files changed, 564 insertions(+)
arch/arm/cpu/armv7/mx6/ddr.c +559 -0
@@ -13,6 +13,565 @@#include <asm/io.h> #include <asm/types.h> +#if defined(CONFIG_MX6QDL ) || defined(CONFIG_MX6Q ) || defined(CONFIG_MX6D ) + +static int wait_for_bit (void *reg,const uint32_t mask,bool set) +{ +unsigned int timeout =1000 ; +u32 val; + +while (--timeout) { + val =readl (reg); +if (!set) + val = ~val; + +if ((val & mask) == mask) +return 0 ; + +udelay (1 ); + } + +printf ("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n" , + __func__, reg, mask, set); +hang ();/* DRAM couldn't be calibrated, game over :-( */ +} + +static void reset_read_data_fifos (void ) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; + +/* Reset data FIFOs twice. */ +setbits_le32 (&mmdc0->mpdgctrl0,1 <<31 ); +wait_for_bit (&mmdc0->mpdgctrl0,1 <<31 ,0 ); + +setbits_le32 (&mmdc0->mpdgctrl0,1 <<31 ); +wait_for_bit (&mmdc0->mpdgctrl0,1 <<31 ,0 ); +} + +static void precharge_all (const bool cs0_enable,const bool cs1_enable) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; + +/* +* Issue the Precharge-All command to the DDR device for both +* chip selects. Note, CON_REQ bit should also remain set. If +* only using one chip select, then precharge only the desired +* chip select. +*/ +if (cs0_enable) {/* CS0 */ +writel (0x04008050 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } + +if (cs1_enable) {/* CS1 */ +writel (0x04008058 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } +} + +static void force_delay_measurement (int bus_size) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; + +writel (0x800 , &mmdc0->mpmur0); +if (bus_size ==0x2 ) +writel (0x800 , &mmdc1->mpmur0); +} + +static void modify_dg_result (u32 *reg_st0,u32 *reg_st1,u32 *reg_ctrl) +{ +u32 dg_tmp_val, dg_dl_abs_offset, dg_hc_del, val_ctrl; + +/* +* DQS gating absolute offset should be modified from reflecting +* (HW_DG_LOWx + HW_DG_UPx)/2 to reflecting (HW_DG_UPx - 0x80) +*/ + + val_ctrl =readl (reg_ctrl); + val_ctrl &=0xf0000000 ; + + dg_tmp_val = ((readl (reg_st0) &0x07ff0000 ) >>16 ) -0xc0 ; + dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; + + val_ctrl |= dg_dl_abs_offset + dg_hc_del; + + dg_tmp_val = ((readl (reg_st1) &0x07ff0000 ) >>16 ) -0xc0 ; + dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; + + val_ctrl |= (dg_dl_abs_offset + dg_hc_del) <<16 ; + +writel (val_ctrl, reg_ctrl); +} + +int mmdc_do_write_level_calibration (void ) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; +u32 esdmisc_val, zq_val; +u32 errors =0 ; +u32 ldectrl[4 ]; +u32 ddr_mr1 =0x4 ; + +/* +* Stash old values in case calibration fails, +* we need to restore them +*/ + ldectrl[0 ] =readl (&mmdc0->mpwldectrl0); + ldectrl[1 ] =readl (&mmdc0->mpwldectrl1); + ldectrl[2 ] =readl (&mmdc1->mpwldectrl0); + ldectrl[3 ] =readl (&mmdc1->mpwldectrl1); + +/* disable DDR logic power down timer */ +clrbits_le32 (&mmdc0->mdpdc,0xff00 ); + +/* disable Adopt power down timer */ +setbits_le32 (&mmdc0->mapsr,0x1 ); + +debug ("Starting write leveling calibration.\n" ); + +/* +* 2. disable auto refresh and ZQ calibration +* before proceeding with Write Leveling calibration +*/ + esdmisc_val =readl (&mmdc0->mdref); +writel (0x0000C000 , &mmdc0->mdref); + zq_val =readl (&mmdc0->mpzqhwctrl); +writel (zq_val & ~0x3 , &mmdc0->mpzqhwctrl); + +/* 3. increase walat and ralat to maximum */ +setbits_le32 (&mmdc0->mdmisc, + (1 <<6 ) | (1 <<7 ) | (1 <<8 ) | (1 <<16 ) | (1 <<17 )); +setbits_le32 (&mmdc1->mdmisc, + (1 <<6 ) | (1 <<7 ) | (1 <<8 ) | (1 <<16 ) | (1 <<17 )); +/* +* 4 & 5. Configure the external DDR device to enter write-leveling +* mode through Load Mode Register command. +* Register setting: +* Bits[31:16] MR1 value (0x0080 write leveling enable) +* Bit[9] set WL_EN to enable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/ +writel (0x00808231 , &mmdc0->mdscr); + +/* 6. Activate automatic calibration by setting MPWLGCR[HW_WL_EN] */ +writel (0x00000001 , &mmdc0->mpwlgcr); + +/* +* 7. Upon completion of this process the MMDC de-asserts +* the MPWLGCR[HW_WL_EN] +*/ +wait_for_bit (&mmdc0->mpwlgcr,1 <<0 ,0 ); + +/* +* 8. check for any errors: check both PHYs for x64 configuration, +* if x32, check only PHY0 +*/ +if (readl (&mmdc0->mpwlgcr) &0x00000F00 ) + errors |=1 ; +if (readl (&mmdc1->mpwlgcr) &0x00000F00 ) + errors |=2 ; + +debug ("Ending write leveling calibration. Error mask: 0x%x\n" , errors); + +/* check to see if cal failed */ +if ((readl (&mmdc0->mpwldectrl0) ==0x001F001F ) && + (readl (&mmdc0->mpwldectrl1) ==0x001F001F ) && + (readl (&mmdc1->mpwldectrl0) ==0x001F001F ) && + (readl (&mmdc1->mpwldectrl1) ==0x001F001F )) { +debug ("Cal seems to have soft-failed due to memory not supporting write leveling on all channels. Restoring original write leveling values.\n" ); +writel (ldectrl[0 ], &mmdc0->mpwldectrl0); +writel (ldectrl[1 ], &mmdc0->mpwldectrl1); +writel (ldectrl[2 ], &mmdc1->mpwldectrl0); +writel (ldectrl[3 ], &mmdc1->mpwldectrl1); + errors |=4 ; + } + +/* +* User should issue MRS command to exit write leveling mode +* through Load Mode Register command +* Register setting: +* Bits[31:16] MR1 value "ddr_mr1" value from initialization +* Bit[9] clear WL_EN to disable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/ +writel ((ddr_mr1 <<16 ) +0x8031 , &mmdc0->mdscr); + +/* re-enable auto refresh and zq cal */ +writel (esdmisc_val, &mmdc0->mdref); +writel (zq_val, &mmdc0->mpzqhwctrl); + +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl1)); +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl1)); + +/* We must force a readback of these values, to get them to stick */ +readl (&mmdc0->mpwldectrl0); +readl (&mmdc0->mpwldectrl1); +readl (&mmdc1->mpwldectrl0); +readl (&mmdc1->mpwldectrl1); + +/* enable DDR logic power down timer: */ +setbits_le32 (&mmdc0->mdpdc,0x00005500 ); + +/* Enable Adopt power down timer: */ +clrbits_le32 (&mmdc0->mapsr,0x1 ); + +/* Clear CON_REQ */ +writel (0 , &mmdc0->mdscr); + +return errors; +} + +int mmdc_do_dqs_calibration (void ) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; +struct mx6dq_iomux_ddr_regs *mx6_ddr_iomux = + (struct mx6dq_iomux_ddr_regs *)MX6DQ_IOM_DDR_BASE ; +bool cs0_enable; +bool cs1_enable; +bool cs0_enable_initial; +bool cs1_enable_initial; +u32 esdmisc_val; +u32 bus_size; +u32 temp_ref; +u32 pddword =0x00ffff00 ;/* best so far, place into MPPDCMPR1 */ +u32 errors =0 ; +u32 initdelay =0x40404040 ; + +/* check to see which chip selects are enabled */ + cs0_enable_initial =readl (&mmdc0->mdctl) &0x80000000 ; + cs1_enable_initial =readl (&mmdc0->mdctl) &0x40000000 ; + +/* disable DDR logic power down timer: */ +clrbits_le32 (&mmdc0->mdpdc,0xff00 ); + +/* disable Adopt power down timer: */ +setbits_le32 (&mmdc0->mapsr,0x1 ); + +/* set DQS pull ups */ +setbits_le32 (&mx6_ddr_iomux->dram_sdqs0,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs1,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs2,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs3,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs4,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs5,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs6,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs7,0x7000 ); + +/* Save old RALAT and WALAT values */ + esdmisc_val =readl (&mmdc0->mdmisc); + +setbits_le32 (&mmdc0->mdmisc, + (1 <<6 ) | (1 <<7 ) | (1 <<8 ) | (1 <<16 ) | (1 <<17 )); + +/* Disable auto refresh before proceeding with calibration */ + temp_ref =readl (&mmdc0->mdref); +writel (0x0000c000 , &mmdc0->mdref); + +/* +* Per the ref manual, issue one refresh cycle MDSCR[CMD]= 0x2, +* this also sets the CON_REQ bit. +*/ +if (cs0_enable_initial) +writel (0x00008020 , &mmdc0->mdscr); +if (cs1_enable_initial) +writel (0x00008028 , &mmdc0->mdscr); + +/* poll to make sure the con_ack bit was asserted */ +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + +/* +* Check MDMISC register CALIB_PER_CS to see which CS calibration +* is targeted to (under normal cases, it should be cleared +* as this is the default value, indicating calibration is directed +* to CS0). +* Disable the other chip select not being target for calibration +* to avoid any potential issues. This will get re-enabled@end +* of calibration. +*/ +if ((readl (&mmdc0->mdmisc) &0x00100000 ) ==0 ) +clrbits_le32 (&mmdc0->mdctl,1 <<30 );/* clear SDE_1 */ +else +clrbits_le32 (&mmdc0->mdctl,1 <<31 );/* clear SDE_0 */ + +/* +* Check to see which chip selects are now enabled for +* the remainder of the calibration. +*/ + cs0_enable =readl (&mmdc0->mdctl) &0x80000000 ; + cs1_enable =readl (&mmdc0->mdctl) &0x40000000 ; + +/* Check to see what the data bus size is */ + bus_size = (readl (&mmdc0->mdctl) &0x30000 ) >>16 ; +debug ("Data bus size: %d (%d bits)\n" , bus_size,1 << (bus_size +4 )); + +precharge_all (cs0_enable, cs1_enable); + +/* Write the pre-defined value into MPPDCMPR1 */ +writel (pddword, &mmdc0->mppdcmpr1); + +/* +* Issue a write access to the external DDR device by setting +* the bit SW_DUMMY_WR (bit 0) in the MPSWDAR0 and then poll +* this bit until it clears to indicate completion of the write access. +*/ +setbits_le32 (&mmdc0->mpswdar0,1 ); +wait_for_bit (&mmdc0->mpswdar0,1 <<0 ,0 ); + +/* Set the RD_DL_ABS# bits to their default values +* (will be calibrated later in the read delay-line calibration). +* Both PHYs for x64 configuration, if x32, do only PHY0. +*/ +writel (initdelay, &mmdc0->mprddlctl); +if (bus_size ==0x2 ) +writel (initdelay, &mmdc1->mprddlctl); + +/* Force a measurment, for previous delay setup to take effect. */ +force_delay_measurement (bus_size); + +/* +* *************************** +* Read DQS Gating calibration +* *************************** +*/ +debug ("Starting Read DQS Gating calibration.\n" ); + +/* +* Reset the read data FIFOs (two resets); only need to issue reset +* to PHY0 since in x64 mode, the reset will also go to PHY1. +*/ +reset_read_data_fifos (); + +/* +* Start the automatic read DQS gating calibration process by +* asserting MPDGCTRL0[HW_DG_EN] and MPDGCTRL0[DG_CMP_CYC] +* and then poll MPDGCTRL0[HW_DG_EN]] until this bit clears +* to indicate completion. +* Also, ensure that MPDGCTRL0[HW_DG_ERR] is clear to indicate +* no errors were seen during calibration. +*/ + +/* +* Set bit 30: chooses option to wait 32 cycles instead of +* 16 before comparing read data. +*/ +setbits_le32 (&mmdc0->mpdgctrl0,1 <<30 ); + +/* Set bit 28 to start automatic read DQS gating calibration */ +setbits_le32 (&mmdc0->mpdgctrl0,5 <<28 ); + +/* Poll for completion. MPDGCTRL0[HW_DG_EN] should be 0 */ +wait_for_bit (&mmdc0->mpdgctrl0,1 <<28 ,0 ); + +/* +* Check to see if any errors were encountered during calibration +* (check MPDGCTRL0[HW_DG_ERR]). +* Check both PHYs for x64 configuration, if x32, check only PHY0. +*/ +if (readl (&mmdc0->mpdgctrl0) &0x00001000 ) + errors |=1 ; + +if ((bus_size ==0x2 ) && (readl (&mmdc1->mpdgctrl0) &0x00001000 )) + errors |=2 ; + +/* +* DQS gating absolute offset should be modified from +* reflecting (HW_DG_LOWx + HW_DG_UPx)/2 to +* reflecting (HW_DG_UPx - 0x80) +*/ +modify_dg_result (&mmdc0->mpdghwst0, &mmdc0->mpdghwst1, + &mmdc0->mpdgctrl0); +modify_dg_result (&mmdc0->mpdghwst2, &mmdc0->mpdghwst3, + &mmdc0->mpdgctrl1); +if (bus_size ==0x2 ) { +modify_dg_result (&mmdc1->mpdghwst0, &mmdc1->mpdghwst1, + &mmdc1->mpdgctrl0); +modify_dg_result (&mmdc1->mpdghwst2, &mmdc1->mpdghwst3, + &mmdc1->mpdgctrl1); + } +debug ("Ending Read DQS Gating calibration. Error mask: 0x%x\n" , errors); + +/* +* ********************** +* Read Delay calibration +* ********************** +*/ +debug ("Starting Read Delay calibration.\n" ); + +reset_read_data_fifos (); + +/* +* 4. Issue the Precharge-All command to the DDR device for both +* chip selects. If only using one chip select, then precharge +* only the desired chip select. +*/ +precharge_all (cs0_enable, cs1_enable); + +/* +* 9. Read delay-line calibration +* Start the automatic read calibration process by asserting +* MPRDDLHWCTL[HW_RD_DL_EN]. +*/ +writel (0x00000030 , &mmdc0->mprddlhwctl); + +/* +* 10. poll for completion +* MMDC indicates that the write data calibration had finished by +* setting MPRDDLHWCTL[HW_RD_DL_EN] = 0. Also, ensure that +* no error bits were set. +*/ +wait_for_bit (&mmdc0->mprddlhwctl,1 <<4 ,0 ); + +/* check both PHYs for x64 configuration, if x32, check only PHY0 */ +if (readl (&mmdc0->mprddlhwctl) &0x0000000f ) + errors |=4 ; + +if ((bus_size ==0x2 ) && (readl (&mmdc1->mprddlhwctl) &0x0000000f )) + errors |=8 ; + +debug ("Ending Read Delay calibration. Error mask: 0x%x\n" , errors); + +/* +* *********************** +* Write Delay Calibration +* *********************** +*/ +debug ("Starting Write Delay calibration.\n" ); + +reset_read_data_fifos (); + +/* +* 4. Issue the Precharge-All command to the DDR device for both +* chip selects. If only using one chip select, then precharge +* only the desired chip select. +*/ +precharge_all (cs0_enable, cs1_enable); + +/* +* 8. Set the WR_DL_ABS# bits to their default values. +* Both PHYs for x64 configuration, if x32, do only PHY0. +*/ +writel (initdelay, &mmdc0->mpwrdlctl); +if (bus_size ==0x2 ) +writel (initdelay, &mmdc1->mpwrdlctl); + +/* +* XXX This isn't in the manual. Force a measurement, +* for previous delay setup to effect. +*/ +force_delay_measurement (bus_size); + +/* +* 9. 10. Start the automatic write calibration process +* by asserting MPWRDLHWCTL0[HW_WR_DL_EN]. +*/ +writel (0x00000030 , &mmdc0->mpwrdlhwctl); + +/* +* Poll for completion. +* MMDC indicates that the write data calibration had finished +* by setting MPWRDLHWCTL[HW_WR_DL_EN] = 0. +* Also, ensure that no error bits were set. +*/ +wait_for_bit (&mmdc0->mpwrdlhwctl,1 <<4 ,0 ); + +/* Check both PHYs for x64 configuration, if x32, check only PHY0 */ +if (readl (&mmdc0->mpwrdlhwctl) &0x0000000f ) + errors |=16 ; + +if ((bus_size ==0x2 ) && (readl (&mmdc1->mpwrdlhwctl) &0x0000000f )) + errors |=32 ; + +debug ("Ending Write Delay calibration. Error mask: 0x%x\n" , errors); + +reset_read_data_fifos (); + +/* Enable DDR logic power down timer */ +setbits_le32 (&mmdc0->mdpdc,0x00005500 ); + +/* Enable Adopt power down timer */ +clrbits_le32 (&mmdc0->mapsr,0x1 ); + +/* Restore MDMISC value (RALAT, WALAT) to MMDCP1 */ +writel (esdmisc_val, &mmdc0->mdmisc); + +/* Clear DQS pull ups */ +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs0,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs1,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs2,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs3,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs4,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs5,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs6,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs7,0x7000 ); + +/* Re-enable SDE (chip selects) if they were set initially */ +if (cs1_enable_initial) +/* Set SDE_1 */ +setbits_le32 (&mmdc0->mdctl,1 <<30 ); + +if (cs0_enable_initial) +/* Set SDE_0 */ +setbits_le32 (&mmdc0->mdctl,1 <<31 ); + +/* Re-enable to auto refresh */ +writel (temp_ref, &mmdc0->mdref); + +/* Clear the MDSCR (including the con_req bit) */ +writel (0x0 , &mmdc0->mdscr);/* CS0 */ + +/* Poll to make sure the con_ack bit is clear */ +wait_for_bit (&mmdc0->mdscr,1 <<14 ,0 ); + +/* +* Print out the registers that were updated as a result +* of the calibration process. +*/ +debug ("MMDC registers updated from calibration\n" ); +debug ("Read DQS gating calibration:\n" ); +debug ("\tMPDGCTRL0 PHY0 = 0x%08X\n" ,readl (&mmdc0->mpdgctrl0)); +debug ("\tMPDGCTRL1 PHY0 = 0x%08X\n" ,readl (&mmdc0->mpdgctrl1)); +debug ("\tMPDGCTRL0 PHY1 = 0x%08X\n" ,readl (&mmdc1->mpdgctrl0)); +debug ("\tMPDGCTRL1 PHY1 = 0x%08X\n" ,readl (&mmdc1->mpdgctrl1)); +debug ("Read calibration:\n" ); +debug ("\tMPRDDLCTL PHY0 = 0x%08X\n" ,readl (&mmdc0->mprddlctl)); +debug ("\tMPRDDLCTL PHY1 = 0x%08X\n" ,readl (&mmdc1->mprddlctl)); +debug ("Write calibration:\n" ); +debug ("\tMPWRDLCTL PHY0 = 0x%08X\n" ,readl (&mmdc0->mpwrdlctl)); +debug ("\tMPWRDLCTL PHY1 = 0x%08X\n" ,readl (&mmdc1->mpwrdlctl)); + +/* +* Registers below are for debugging purposes. These print out +* the upper and lower boundaries captured during +* read DQS gating calibration. +*/ +debug ("Status registers bounds for read DQS gating:\n" ); +debug ("\tMPDGHWST0 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst0)); +debug ("\tMPDGHWST1 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst1)); +debug ("\tMPDGHWST2 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst2)); +debug ("\tMPDGHWST3 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst3)); +debug ("\tMPDGHWST0 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst0)); +debug ("\tMPDGHWST1 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst1)); +debug ("\tMPDGHWST2 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst2)); +debug ("\tMPDGHWST3 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst3)); + +debug ("Final do_dqs_calibration error mask: 0x%x\n" , errors); + +return errors; +} +#endif +#if defined(CONFIG_MX6SX )/* Configure MX6SX mmdc iomux */ void mx6sx_dram_iocfg(unsigned width,
arch/arm/include/asm/arch-mx6/mx6-ddr.h +5 -0
@@ -456,6 +456,11 @@ void mx6sl_dram_iocfg(unsigned width, const struct mx6sl_iomux_ddr_regs *, const struct mx6sl_iomux_grp_regs *); +#if defined(CONFIG_MX6QDL ) || defined(CONFIG_MX6Q ) || defined(CONFIG_MX6D ) +int mmdc_do_write_level_calibration (void ); +int mmdc_do_dqs_calibration (void ); +#endif +/* configure mx6 mmdc registers */ void mx6_dram_cfg(const struct mx6_ddr_sysinfo *, const struct mx6_mmdc_calibration *,
Signature · 1 line
2.1.4
PATCH 2/2 arm: imx6: Enable DDR calibration on Novena
Enable the DDR calibration functionality on Novena to deal with the memory SoDIMM on this board. Moreover, tweak the initial DDR DRAM parameters so the calibration works properly.
--- board/kosagi/novena/novena_spl.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
board/kosagi/novena/novena_spl.c +14 -9
@@ -434,8 +434,8 @@ static struct mx6dq_iomux_ddr_regs novena_ddr_ioregs = { .dram_ras = 0x00000038, .dram_reset = 0x00000038,/* SDCKE[0:1]: 100k pull-up */ - .dram_sdcke0 = 0x00003000, - .dram_sdcke1 = 0x00003000, + .dram_sdcke0 = 0x00000038, + .dram_sdcke1 = 0x00000038,/* SDBA2: pull-up disabled */ .dram_sdba2 = 0x00000000,/* SDODT[0:1]: 100k pull-up, 40 ohm */
@@ -512,10 +512,10 @@ static struct mx6_ddr_sysinfo novena_ddr_info = {/* Single chip select */ .ncs = 1, .cs1_mirror = 0, - .rtt_wr = 1, /* RTT_Wr = RZQ/4 */ - .rtt_nom = 2, /* RTT_Nom = RZQ/2 */ - .walat = 3, /* Write additional latency */ - .ralat = 7, /* Read additional latency */ + .rtt_wr = 0, /* RTT_Wr = RZQ/4 */ + .rtt_nom = 1, /* RTT_Nom = RZQ/2 */ + .walat = 0, /* Write additional latency */ + .ralat = 5, /* Read additional latency */ .mif3_mode = 3, /* Command prediction working mode */ .bi_on = 1, /* Bank interleaving enabled */ .sde_to_rst = 0x10, /* 14 cycles, 200us (JEDEC default) */
@@ -530,9 +530,9 @@ static struct mx6_ddr3_cfg elpida_4gib_1600 = { .rowaddr = 16, .coladdr = 10, .pagesz = 2, - .trcd = 1300, - .trcmin = 4900, - .trasmin = 3590, + .trcd = 1375, + .trcmin = 4875, + .trasmin = 3500, };static void ccgr_init (void )
@@ -601,6 +601,11 @@ void board_init_f(ulong dummy)mx6dq_dram_iocfg (64 , &novena_ddr_ioregs, &novena_grp_ioregs);mx6_dram_cfg (&novena_ddr_info, &novena_mmdc_calib, &elpida_4gib_1600); +/* Perform DDR DRAM calibration */ +udelay (100 ); +mmdc_do_write_level_calibration (); +mmdc_do_dqs_calibration (); +/* Clear the BSS. */ memset (__bss_start,0 , __bss_end - __bss_start);
Signature · 1 line
2.1.4
PATCH 2/2 arm: imx6: Enable DDR calibration on Novena
Hi Marek,
Quoted 2 lines · Marek Vasut
Enable the DDR calibration functionality on Novena to deal with the memory SoDIMM on this board.
Shouldn't this be in two patches?
Review
Quoted 9 lines
Moreover, tweak the initial DDR DRAM parameters so the calibration works properly.
Signed-off-by Marek Vasut <[email protected]>--- board/kosagi/novena/novena_spl.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)(quoted diff) +0
-0
b/board/kosagi/novena/novena_spl.c
board/kosagi/novena/novena_spl.c static struct mx6dq_iomux_ddr_regs
Quoted 4 lines
index eb46265..f779bb4 100644
board/kosagi/novena/novena_spl.c +0
-0@@ -434,8 +434,8 @@ static struct mx6dq_iomux_ddr_regs
novena_ddr_ioregs = {
Quoted 39 lines
board/kosagi/novena/novena_spl.c +9
-9.dram_ras = 0x00000038, .dram_reset = 0x00000038,/* SDCKE[0:1]: 100k pull-up */ - .dram_sdcke0 = 0x00003000, - .dram_sdcke1 = 0x00003000, + .dram_sdcke0 = 0x00000038, + .dram_sdcke1 = 0x00000038,/* SDBA2: pull-up disabled */ .dram_sdba2 = 0x00000000,/* SDODT[0:1]: 100k pull-up, 40 ohm */ @@ -512,10 +512,10 @@ static struct mx6_ddr_sysinfo novena_ddr_info = {/* Single chip select */ .ncs = 1, .cs1_mirror = 0, - .rtt_wr = 1, /* RTT_Wr = RZQ/4 */ - .rtt_nom = 2, /* RTT_Nom = RZQ/2 */ - .walat = 3, /* Write additional latency */ - .ralat = 7, /* Read additional latency */ + .rtt_wr = 0, /* RTT_Wr = RZQ/4 */ + .rtt_nom = 1, /* RTT_Nom = RZQ/2 */ + .walat = 0, /* Write additional latency */ + .ralat = 5, /* Read additional latency */ .mif3_mode = 3, /* Command prediction working mode */ .bi_on = 1, /* Bank interleaving enabled */ .sde_to_rst = 0x10, /* 14 cycles, 200us (JEDEC default) */@@ -530,9 +530,9 @@ static struct mx6_ddr3_cfg elpida_4gib_1600 = { .rowaddr = 16, .coladdr = 10, .pagesz = 2, - .trcd = 1300, - .trcmin = 4900, - .trasmin = 3590, + .trcd = 1375, + .trcmin = 4875, + .trasmin = 3500, };static void ccgr_init (void )
board/kosagi/novena/novena_spl.c void board_init_f(ulong dummy) · line 605
Quoted 5 lines
board/kosagi/novena/novena_spl.c +2
-0@@ -601,6 +601,11 @@ void board_init_f(ulong dummy)mx6dq_dram_iocfg (64 , &novena_ddr_ioregs, &novena_grp_ioregs);mx6_dram_cfg (&novena_ddr_info, &novena_mmdc_calib, &elpida_4gib_1600); +/* Perform DDR DRAM calibration */ +udelay (100 );
Shouldn't the return values be tested?
Quoted 7 lines
board/kosagi/novena/novena_spl.c +3
-0+mmdc_do_write_level_calibration (); +mmdc_do_dqs_calibration (); +/* Clear the BSS. */ memset (__bss_start,0 , __bss_end - __bss_start);
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 2/2 arm: imx6: Enable DDR calibration on Novena
Quoted 7 lines · Eric Nelson
Hi Marek,
Quoted 2 lines · Marek Vasut
Enable the DDR calibration functionality on Novena to deal with the memory SoDIMM on this board.
Shouldn't this be in two patches?
Not really, the old values work without the enabled calibration. This change needs to be done atomically.
[...]
Quoted 11 lines
board/kosagi/novena/novena_spl.c void board_init_f(ulong dummy) · line 606
Quoted 9 lines
static void
ccgr_init(void)board/kosagi/novena/novena_spl.c +2
-0@@ -601,6 +601,11 @@ void board_init_f(ulong dummy)mx6dq_dram_iocfg (64 , &novena_ddr_ioregs, &novena_grp_ioregs);mx6_dram_cfg (&novena_ddr_info, &novena_mmdc_calib, &elpida_4gib_1600); +/* Perform DDR DRAM calibration */ +udelay (100 );Shouldn't the return values be tested?
I guess yes, but if the calibration fails, that what ? It's game over ;-)
I will just change those functions to void type and do hang() if we get errors. I believe that's the most sane way to handle it and it prevents development of boilerplate code.
Quoted 6 lines
Quoted 6 lines
board/kosagi/novena/novena_spl.c +3
-0+mmdc_do_write_level_calibration (); +mmdc_do_dqs_calibration (); +/* Clear the BSS. */
memset(__bss_start, 0, __bss_end - __bss_start);
Best regards,
Marek Vasut
PATCH 2/2 arm: imx6: Enable DDR calibration on Novena
Hi Marek,
Quoted 21 lines · Marek Vasut
Quoted 7 lines · Eric Nelson
Hi Marek,
Quoted 2 lines · Marek Vasut
Enable the DDR calibration functionality on Novena to deal with the memory SoDIMM on this board.
Shouldn't this be in two patches?
Not really, the old values work without the enabled calibration. This change needs to be done atomically.
[...]
Quoted 7 lines
Quoted 7 lines
static void
ccgr_init(void)board/kosagi/novena/novena_spl.c +0
-0@@ -601,6 +601,11 @@ void board_init_f(ulong dummy)mx6dq_dram_iocfg (64 , &novena_ddr_ioregs, &novena_grp_ioregs);mx6_dram_cfg (&novena_ddr_info, &novena_mmdc_calib, &elpida_4gib_1600);
Quoted 6 lines
Quoted 4 lines
board/kosagi/novena/novena_spl.c void board_init_f(ulong dummy) · line 605
Quoted 2 lines
board/kosagi/novena/novena_spl.c +2
-0+/* Perform DDR DRAM calibration */ +udelay (100 );Shouldn't the return values be tested?
I guess yes, but if the calibration fails, that what ? It's game over ;-)
Do you think it's possible/practical to reboot the system in this case?
Quoted 9 lines
I will just change those functions to void type and do
hang()if we get errors. I believe that's the most sane way to handle it and it prevents development of boilerplate code.Quoted 4 lines
Quoted 4 lines
board/kosagi/novena/novena_spl.c +3
-0+mmdc_do_write_level_calibration (); +mmdc_do_dqs_calibration (); +
Quoted 11 lines
Quoted 2 lines
Quoted 2 lines
board/kosagi/novena/novena_spl.c +0
-0/* Clear the BSS. */ memset(__bss_start, 0, __bss_end - __bss_start);Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Regards,
Nikolay
PATCH 2/2 arm: imx6: Enable DDR calibration on Novena
Quoted 25 lines · Nikolay Dimitrov
Hi Marek,
Quoted 22 lines · Marek Vasut
Quoted 7 lines · Eric Nelson
Hi Marek,
Quoted 2 lines · Marek Vasut
Enable the DDR calibration functionality on Novena to deal with the memory SoDIMM on this board.
Shouldn't this be in two patches?
Not really, the old values work without the enabled calibration. This change needs to be done atomically.
[...]
Quoted 8 lines
Quoted 8 lines
static void
ccgr_init(void)(quoted diff) +0
-0@@ -601,6 +601,11 @@ void board_init_f(ulong dummy) mx6dq_dram_iocfg(64, &novena_ddr_ioregs, &novena_grp_ioregs); mx6_dram_cfg(&novena_ddr_info, &novena_mmdc_calib, &elpida_4gib_1600);
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
board/kosagi/novena/novena_spl.c void board_init_f(ulong dummy) · line 605
Quoted 2 lines
board/kosagi/novena/novena_spl.c +2
-0+/* Perform DDR DRAM calibration */ +udelay (100 );Shouldn't the return values be tested?
I guess yes, but if the calibration fails, that what ? It's game over ;-)
Do you think it's possible/practical to reboot the system in this case?
Well, you can call hang() , that's how we've been handling critical failures.
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
In reply to Marek VasutHi Marek,
Quoted 4 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
I'm glad to see that others are interested in this.
I've been working on something similar, but struggling to have time to finish and clean it up:
https://github.com/ericnelsonaz/u-boot/tree/memcal-pass1
My aim is/was a bit different though, and aims to be a replacement for the DDR stress tool, which is cumbersome to use.
To do that, I put together a pseudo-board with Kconfig options for the serial console, memory bus width, and such.
Quoted 12 lines
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>--- arch/arm/cpu/armv7/mx6/ddr.c | 559 ++++++++++++++++++++++++++++++++ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 5 + 2 files changed, 564 insertions(+)arch/arm/cpu/armv7/mx6/ddr.c +0
-0
arch/arm/cpu/armv7/mx6/ddr.c line 15
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +0
-0@@ -13,6 +13,565 @@#include <asm/io.h> #include <asm/types.h>
I'll review this in detail later, but off-hand, I think this could use a new CONFIG_ variable to exclude it from boards that don't use it.
It also shouldn't be difficult to support i.MX6SL and LPDDR here.
arch/arm/cpu/armv7/mx6/ddr.c line 22
Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c +7
-0+#if defined(CONFIG_MX6QDL ) || defined(CONFIG_MX6Q ) || defined(CONFIG_MX6D ) + +static int wait_for_bit (void *reg,const uint32_t mask,bool set) +{ +unsigned int timeout =1000 ; +u32 val; +
Regards,
Eric
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 7 lines · Eric Nelson
Hi Marek,
Quoted 3 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Quoted 8 lines
I'm glad to see that others are interested in this.
I've been working on something similar, but struggling to have time to finish and clean it up:
https://github.com/ericnelsonaz/u-boot/tree/memcal-pass1My aim is/was a bit different though, and aims to be a replacement for the DDR stress tool, which is cumbersome to use.
Excellent, we can add this as an optional feature or something which can be triggered by command, since the "full" calibration takes some time.
Quoted 2 lines
To do that, I put together a pseudo-board with
Kconfigoptions for the serial console, memory bus width, and such.
Or maybe this can be built as a mutated SPL ?
Quoted 15 lines
Quoted 15 lines
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>---
arch/arm/cpu/armv7/mx6/ddr.c | 559 ++++++++++++++++++++++++++++++++ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 5 + 2 files changed, 564 insertions(+)arch/arm/cpu/armv7/mx6/ddr.c +0
-0@@ -13,6 +13,565 @@
Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c line 14
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +0
-0#include <asm/io.h> #include <asm/types.h> I'll review this in detail later, but off-hand, I think this could use a new
CONFIG_variable to exclude it from boards that don't use it.It also shouldn't be difficult to support i.MX6SL and LPDDR here.
This is OK, the code will be compiled for these MX6 variants, but it will be dropped from the final binary if you don't call it explicitly (see patch 2/2).
I don't have SX/SL, so I couldn't test it there.
Thanks!
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 22
Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c +1
-0+#if defined(CONFIG_MX6QDL) || defined(CONFIG_MX6Q) ||
defined(CONFIG_MX6D)+arch/arm/cpu/armv7/mx6/ddr.c +5
-0+static int wait_for_bit (void *reg,const uint32_t mask,bool set) +{ +unsigned int timeout =1000 ; +u32 val; +Regards,
Quoted 1 line
Eric
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Marek,
Quoted 17 lines · Marek Vasut
Quoted 7 lines · Eric Nelson
Hi Marek,
Quoted 3 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Quoted 8 lines
I'm glad to see that others are interested in this.
I've been working on something similar, but struggling to have time to finish and clean it up:
https://github.com/ericnelsonaz/u-boot/tree/memcal-pass1My aim is/was a bit different though, and aims to be a replacement for the DDR stress tool, which is cumbersome to use.
Quoted 8 lines
Excellent, we can add this as an optional feature or something which can be triggered by command, since the "full" calibration takes some time.
Quoted 2 lines
To do that, I put together a pseudo-board with
Kconfigoptions for the serial console, memory bus width, and such.Or maybe this can be built as a mutated SPL ?
That's how I have things hacked together. The board "mx6memcal" essentially **only** produces SPL.
It runs calibration, then spits out the results in a form usable by either a .cfg file or SPL data structure.
I'd like to add a trailing memory test with frequency walking as done with the DDR stress tool before submitting it though.
Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>---
arch/arm/cpu/armv7/mx6/ddr.c | 559 ++++++++++++++++++++++++++++++++ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 5 + 2 files changed, 564 insertions(+)arch/arm/cpu/armv7/mx6/ddr.c +0
-0@@ -13,6 +13,565 @@#include <asm/io.h> #include <asm/types.h>
Quoted 9 lines
Quoted 4 lines
I'll review this in detail later, but off-hand, I think this could use a new
CONFIG_variable to exclude it from boards that don't use it.It also shouldn't be difficult to support i.MX6SL and LPDDR here.
This is OK, the code will be compiled for these MX6 variants, but it will be dropped from the final binary if you don't call it explicitly (see patch 2/2).
Gotcha.
Quoted 2 lines
I don't have SX/SL, so I couldn't test it there.
I have a couple of SL boards, so I can do that (mx6slevk and a custom board using DDR3). Both of them support SPL, so it shouldn't take long to validate.
Regards,
Eric
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 1 line · Eric Nelson
Hi Marek,
Hi!
Quoted 27 lines · Marek Vasut
Quoted 18 lines · Eric Nelson
Quoted 9 lines
Hi Marek,
Quoted 3 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
I'm glad to see that others are interested in this.
Quoted 7 lines
I've been working on something similar, but struggling to have time
to finish and clean it up:
https://github.com/ericnelsonaz/u-boot/tree/memcal-pass1My aim is/was a bit different though, and aims to be a replacement for the DDR stress tool, which is cumbersome to use.
Quoted 7 lines
Excellent, we can add this as an optional feature or something which can be triggered by command, since the "full" calibration takes some time.
Quoted 2 lines
To do that, I put together a pseudo-board with
Kconfigoptions for the serial console, memory bus width, and such.Or maybe this can be built as a mutated SPL ?
Quoted 8 lines
That's how I have things hacked together. The board "mx6memcal" essentially **only** produces SPL.
It runs calibration, then spits out the results in a form usable by either a .cfg file or SPL data structure.
I'd like to add a trailing memory test with frequency walking as done with the DDR stress tool before submitting it though.
Gotcha! That'd be great thing to have %^)
Quoted 27 lines
Quoted 21 lines
Quoted 21 lines
Quoted 14 lines
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>---
arch/arm/cpu/armv7/mx6/ddr.c | 559 ++++++++++++++++++++++++++++++++ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 5 + 2 files changed, 564 insertions(+)(quoted diff) +0
-0
b/arch/arm/cpu/armv7/mx6/ddr.cindex 6b039e4..194411f 100644arch/arm/cpu/armv7/mx6/ddr.c line 15
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +0
-0@@ -13,6 +13,565 @@#include <asm/io.h> #include <asm/types.h> I'll review this in detail later, but off-hand, I think this could use a new
CONFIG_variable to exclude it from boards that don't use it.Quoted 5 lines
Quoted 1 line
It also shouldn't be difficult to support i.MX6SL and LPDDR here.
This is OK, the code will be compiled for these MX6 variants, but it will be dropped from the final binary if you don't call it explicitly (see patch 2/2).
Quoted 7 lines
Gotcha.
Quoted 1 line
I don't have SX/SL, so I couldn't test it there.
I have a couple of SL boards, so I can do that (mx6slevk and a custom board using DDR3). Both of them support SPL, so it shouldn't take long to validate.
Excellent! Let's do this as a subsequent patch though, since this code is actually tested extensively and I'd like to keep it as a reference for possible bisecting ;-)
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Marek,
Quoted 3 lines · Marek Vasut
Quoted 2 lines · Eric Nelson
Quoted 1 line · Marek Vasut
On Wednesday, December 16, 2015 at 04:00:38 PM, Eric Nelson wrote:
...
Quoted 10 lines
Quoted 5 lines
Quoted 1 line
I don't have SX/SL, so I couldn't test it there.
I have a couple of SL boards, so I can do that (mx6slevk and a custom board using DDR3). Both of them support SPL, so it shouldn't take long to validate.
Excellent! Let's do this as a subsequent patch though, since this code is actually tested extensively and I'd like to keep it as a reference for possible bisecting ;-)
Will do.
I'll review and test your patch first, then follow up with SoloLite and LPDDR2 support.
Regards,
Eric
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 1 line · Eric Nelson
Hi Marek,
Hi!
Quoted 12 lines · Marek Vasut
Quoted 3 lines · Eric Nelson
Quoted 2 lines · Marek Vasut
Quoted 1 line · Eric Nelson
...
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
I don't have SX/SL, so I couldn't test it there.
I have a couple of SL boards, so I can do that (mx6slevk and a custom board using DDR3). Both of them support SPL, so it shouldn't take long to validate.
Quoted 8 lines
Quoted 3 lines
Excellent! Let's do this as a subsequent patch though, since this code is actually tested extensively and I'd like to keep it as a reference for possible bisecting ;-)
Will do.
I'll review and test your patch first, then follow up with SoloLite and LPDDR2 support.
Excellent, thanks!
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
In reply to Eric NelsonQuoted 8 lines · Eric Nelson
Hi Marek,
Quoted 4 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Quoted 8 lines
I'm glad to see that others are interested in this.
I've been working on something similar, but struggling to have time to finish and clean it up:
https://github.com/ericnelsonaz/u-boot/tree/memcal-pass1My aim is/was a bit different though, and aims to be a replacement for the DDR stress tool, which is cumbersome to use.
Eric,
I would love to see a series posted that adds IMX6 MMDC calibration and stress test to U-Boot. I agree the Freescale code is very difficult to use and I don't trust what its doing. I'm currently seeing the Freescale DDR3 stress test fail on 8Gb density memory and I am not convinced its not a problem with their code (yet I haven't had time to pick through it with a fine toothed comb and compare with how I setup the MMDC in U-Boot).
Regards,
Tim
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Tim,
Quoted 15 lines · Tim Harvey
Quoted 14 lines · Eric Nelson
Hi Marek,
Quoted 4 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
I'm glad to see that others are interested in this.
I've been working on something similar, but struggling to have time to finish and clean it up:
https://github.com/ericnelsonaz/u-boot/tree/memcal-pass1
Quoted 7 lines
Quoted 2 lines
My aim is/was a bit different though, and aims to be a replacement for the DDR stress tool, which is cumbersome to use.
Eric,
I would love to see a series posted that adds IMX6 MMDC calibration and stress test to U-Boot.
Will do. Some other demands on my time will push this into next week (and Christmas) though.
I'll try to get an RFC version out before the holiday though, (after reviewing Marek's patch).
Quoted 2 lines
I agree the Freescale code is very difficult to use and I don't trust what its doing.
It's always tough to trust code that you can't see and discuss.
Because it's a pain to run, I also suspect that many (most) boards are running with calibration gathered from a small set of boards, and I've seen lots of board->board variation.
Something that can be run using imx_usb can make the process of gathering data much easier.
The board->board variations hint that Marek's on the right track and that calibration really should be done at run-time.
Quoted 5 lines
I'm currently seeing the Freescale DDR3 stress test
fail on 8Gb density memory and I am not convinced its not a problem with their code (yet I haven't had time to pick through it with a fine toothed comb and compare with how I setup the MMDC in U-Boot).
I haven't seen any issues with 8GiB densities, but have only tested on a small set of board designs (primarily Nitrogen6_max).
Regards,
Eric
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
In reply to Marek VasutQuoted 6 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>
Marek,
This is great - this would be a great addition to U-Boot IMX6 SPL.
You must have forgotten to post a dependent patch that adds some of the registers to mmdc_p_regs. If you can post that I can run this through some testing. Also, in a follow-on post we should add some more verbiage about how long this takes to perform (I believe you told me ~10ms) and where to refer in the IMX6 RM's for the steps followed.
Regards,
Tim
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 8 lines · Tim Harvey
Quoted 6 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>
Quoted 7 lines
Marek,
This is great - this would be a great addition to U-Boot IMX6 SPL.
You must have forgotten to post a dependent patch that adds some of the registers to
mmdc_p_regs. If you can post that I can run this through some testing.
What exactly is missing please ? I am using this on Novena for a while without issues.
Quoted 3 lines
Also, in a follow-on post we should add some
more verbiage about how long this takes to perform (I believe you told me ~10ms) and where to refer in the IMX6 RM's for the steps followed.
Freescale AN4467 is the right thing ... I mean NXP AN4467 of course.
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 11 lines · Marek Vasut
Quoted 10 lines · Tim Harvey
Quoted 6 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>Marek,
Quoted 8 lines
Quoted 5 lines
This is great - this would be a great addition to U-Boot IMX6 SPL.
You must have forgotten to post a dependent patch that adds some of the registers to
mmdc_p_regs. If you can post that I can run this through some testing.What exactly is missing please ? I am using this on Novena for a while without issues.
sorry my bad... forgot I was on an old branch. All is good.
I will run some boards with this through some stress testing and review.
Tim
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 18 lines · Tim Harvey
Quoted 17 lines · Marek Vasut
Quoted 15 lines · Tim Harvey
Quoted 6 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>Marek,
This is great - this would be a great addition to U-Boot IMX6 SPL.
You must have forgotten to post a dependent patch that adds some of the registers to
mmdc_p_regs. If you can post that I can run this through some testing.
Quoted 6 lines
Quoted 2 lines
What exactly is missing please ? I am using this on Novena for a while without issues.
sorry my bad... forgot I was on an old branch. All is good.
I will run some boards with this through some stress testing and review.
Whew! OKi, thanks!
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
In reply to Marek VasutHi Marek,
Quoted 3 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Great work!
Regards,
Nikolay
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 8 lines · Nikolay Dimitrov
Hi Marek,
Quoted 3 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
Great work!
Thanks, I am flattered :)
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
In reply to Marek VasutHi Marek,
Quoted 3 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
It would be good to have a reference to AN4467 in the commit message, since that document is the best reference to understanding this code:
http://cache.freescale.com/files/32bit/doc/app_note/AN4467.pdf
Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>--- arch/arm/cpu/armv7/mx6/ddr.c | 559 ++++++++++++++++++++++++++++++++ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 5 + 2 files changed, 564 insertions(+)
Note that there's another implementation of this in the barebox project:
arch/arm/cpu/armv7/mx6/ddr.c line 17
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0@@ -13,6 +13,565 @@#include <asm/io.h> #include <asm/types.h> +#if defined(CONFIG_MX6QDL ) || defined(CONFIG_MX6Q ) || defined(CONFIG_MX6D ) +
Should this use the common wait_for_bit?
http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#237952
I'm not certain that the <ctrl-c> handling is appropriate when used here.
Quoted 23 lines
arch/arm/cpu/armv7/mx6/ddr.c +23
-0+static int wait_for_bit (void *reg,const uint32_t mask,bool set) +{ +unsigned int timeout =1000 ; +u32 val; + +while (--timeout) { + val =readl (reg); +if (!set) + val = ~val; + +if ((val & mask) == mask) +return 0 ; + +udelay (1 ); + } + +printf ("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n" , + __func__, reg, mask, set); +hang ();/* DRAM couldn't be calibrated, game over :-( */ +} + +static void reset_read_data_fifos (void ) +{
arch/arm/cpu/armv7/mx6/ddr.c line 50
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; + +/* Reset data FIFOs twice. */ +setbits_le32 (&mmdc0->mpdgctrl0,1 <<31 ); +wait_for_bit (&mmdc0->mpdgctrl0,1 <<31 ,0 ); + +setbits_le32 (&mmdc0->mpdgctrl0,1 <<31 ); +wait_for_bit (&mmdc0->mpdgctrl0,1 <<31 ,0 ); +} +
See comments below about chip-selects during calibration.
arch/arm/cpu/armv7/mx6/ddr.c line 60
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+static void precharge_all (const bool cs0_enable,const bool cs1_enable) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; + +/* +* Issue the Precharge-All command to the DDR device for both +* chip selects. Note, CON_REQ bit should also remain set. If +* only using one chip select, then precharge only the desired +* chip select. +*/
AN4467 says that a wait for tRP is needed here, though none of the other implementations explicitly do that.
The wait for CON_ACK seems like a good idea, though I wonder if the waits shouldn't be performed **before** the commands are issued.
I'd also like to see a named constant for MDSCR_CON_ACK, and since waiting for it is done in multiple places, perhaps wait_for_con_ack().
Quoted 23 lines
arch/arm/cpu/armv7/mx6/ddr.c +23
-0+if (cs0_enable) {/* CS0 */ +writel (0x04008050 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } + +if (cs1_enable) {/* CS1 */ +writel (0x04008058 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } +} + +static void force_delay_measurement (int bus_size) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; + +writel (0x800 , &mmdc0->mpmur0); +if (bus_size ==0x2 ) +writel (0x800 , &mmdc1->mpmur0); +} + +static void modify_dg_result (u32 *reg_st0,u32 *reg_st1,u32 *reg_ctrl) +{
arch/arm/cpu/armv7/mx6/ddr.c line 93
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+u32 dg_tmp_val, dg_dl_abs_offset, dg_hc_del, val_ctrl; + +/* +* DQS gating absolute offset should be modified from reflecting +* (HW_DG_LOWx + HW_DG_UPx)/2 to reflecting (HW_DG_UPx - 0x80) +*/ + + val_ctrl =readl (reg_ctrl); + val_ctrl &=0xf0000000 ; +
Though this matches the app note and other implementations, I think this would be easier to understand by shifting first and anding with 0x7ff.
arch/arm/cpu/armv7/mx6/ddr.c line 94
Quoted 1 line
arch/arm/cpu/armv7/mx6/ddr.c +1
-0+ dg_tmp_val = ((readl (reg_st0) &0x07ff0000 ) >>16 ) -0xc0 ;
Especially since these calculations operate on the shifted values:
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+ dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; + + val_ctrl |= dg_dl_abs_offset + dg_hc_del;
arch/arm/cpu/armv7/mx6/ddr.c line 108
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+ + dg_tmp_val = ((readl (reg_st1) &0x07ff0000 ) >>16 ) -0xc0 ; + dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; + + val_ctrl |= (dg_dl_abs_offset + dg_hc_del) <<16 ; + +writel (val_ctrl, reg_ctrl); +} +
I'd recommend passing parameters of mx6_ddr_sysinfo (input) and mx6_mmdc_calibration (output) to this routine.
arch/arm/cpu/armv7/mx6/ddr.c line 115
Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c +7
-0+int mmdc_do_write_level_calibration (void ) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; +u32 esdmisc_val, zq_val; +u32 errors =0 ; +u32 ldectrl[4 ];
I believe the 4 here comes from this calculation:
ddr_mr1 = ((sysinfo->rtt_nom & 1) ? 1 : 0) << 2 | ((sysinfo->rtt_nom & 2) ? 1 : 0) << 6;
arch/arm/cpu/armv7/mx6/ddr.c line 117
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+u32 ddr_mr1 =0x4 ; +
I'm not sure how this is useful (trying to proceed if things fail).
Have you seen this triggered?
arch/arm/cpu/armv7/mx6/ddr.c line 129
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c +12
-0+/* +* Stash old values in case calibration fails, +* we need to restore them +*/ + ldectrl[0 ] =readl (&mmdc0->mpwldectrl0); + ldectrl[1 ] =readl (&mmdc0->mpwldectrl1); + ldectrl[2 ] =readl (&mmdc1->mpwldectrl0); + ldectrl[3 ] =readl (&mmdc1->mpwldectrl1); + +/* disable DDR logic power down timer */ +clrbits_le32 (&mmdc0->mdpdc,0xff00 ); +
Either s/Adopt/Automatic/ or just get rid of Adopt...
This is a typo in the app note.
arch/arm/cpu/armv7/mx6/ddr.c line 138
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* disable Adopt power down timer */ +setbits_le32 (&mmdc0->mapsr,0x1 ); + +debug ("Starting write leveling calibration.\n" ); + +/* +* 2. disable auto refresh and ZQ calibration +* before proceeding with Write Leveling calibration +*/
Trusting the esdmisc value read from here seems like a bad idea. It would be clearer if the code below that restores things just (re)configures mdref directly.
arch/arm/cpu/armv7/mx6/ddr.c line 141
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+ esdmisc_val =readl (&mmdc0->mdref); +writel (0x0000C000 , &mmdc0->mdref); + zq_val =readl (&mmdc0->mpzqhwctrl);
clrbits_le32?
arch/arm/cpu/armv7/mx6/ddr.c line 144
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+writel (zq_val & ~0x3 , &mmdc0->mpzqhwctrl); + +/* 3. increase walat and ralat to maximum */
(3 << 16) | (7 << 6) would be clearer, since these are numeric values in tbe bitfields.
arch/arm/cpu/armv7/mx6/ddr.c line 148
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+setbits_le32 (&mmdc0->mdmisc, + (1 <<6 ) | (1 <<7 ) | (1 <<8 ) | (1 <<16 ) | (1 <<17 )); +setbits_le32 (&mmdc1->mdmisc, + (1 <<6 ) | (1 <<7 ) | (1 <<8 ) | (1 <<16 ) | (1 <<17 ));
I had a concern about this bit of code (setting RALAT and WALAT to maximum values). It seems odd that we calibrate with one set of delays and then choose a different set later.
Experimentally, I've found using min or max values seems to have no effect on the result.
arch/arm/cpu/armv7/mx6/ddr.c line 157
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* +* 4 & 5. Configure the external DDR device to enter write-leveling +* mode through Load Mode Register command. +* Register setting: +* Bits[31:16] MR1 value (0x0080 write leveling enable) +* Bit[9] set WL_EN to enable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/
wait_for_con_ack() seems like a good idea here:
arch/arm/cpu/armv7/mx6/ddr.c line 166
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+writel (0x00808231 , &mmdc0->mdscr); + +/* 6. Activate automatic calibration by setting MPWLGCR[HW_WL_EN] */ +writel (0x00000001 , &mmdc0->mpwlgcr); + +/* +* 7. Upon completion of this process the MMDC de-asserts +* the MPWLGCR[HW_WL_EN] +*/
Why 1 << 0?
arch/arm/cpu/armv7/mx6/ddr.c line 174
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c +8
-0+wait_for_bit (&mmdc0->mpwlgcr,1 <<0 ,0 ); + +/* +* 8. check for any errors: check both PHYs for x64 configuration, +* if x32, check only PHY0 +*/ +if (readl (&mmdc0->mpwlgcr) &0x00000F00 ) + errors |=1 ;
This should be conditional on (sysinfo->dsize == 2) to allow use with a 32-bit bus:
arch/arm/cpu/armv7/mx6/ddr.c line 179
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+if (readl (&mmdc1->mpwlgcr) &0x00000F00 ) + errors |=2 ; + +debug ("Ending write leveling calibration. Error mask: 0x%x\n" , errors); +
Where are you getting this test from?
I have code that tests each byte individually against a max value of 0x2f, but I'm having trouble finding the source of the test at the moment.
arch/arm/cpu/armv7/mx6/ddr.c line 182
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+/* check to see if cal failed */ + if ((readl(&mmdc0->mpwldectrl0) == 0x001F001F) && + (readl(&mmdc0->mpwldectrl1) == 0x001F001F) &&
Ditto (sysinfo->dsize == 2):
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c +11
-0+ (readl(&mmdc1->mpwldectrl0) == 0x001F001F) && + (readl(&mmdc1->mpwldectrl1) == 0x001F001F)) { +debug ("Cal seems to have soft-failed due to memory not supporting write leveling on all channels. Restoring original write leveling values.\n" ); +writel (ldectrl[0 ], &mmdc0->mpwldectrl0); +writel (ldectrl[1 ], &mmdc0->mpwldectrl1); +writel (ldectrl[2 ], &mmdc1->mpwldectrl0); +writel (ldectrl[3 ], &mmdc1->mpwldectrl1); + errors |=4 ; + } + +/*
arch/arm/cpu/armv7/mx6/ddr.c line 203
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+* User should issue MRS command to exit write leveling mode +* through Load Mode Register command +* Register setting: +* Bits[31:16] MR1 value "ddr_mr1" value from initialization +* Bit[9] clear WL_EN to disable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/ +writel ((ddr_mr1 <<16 ) +0x8031 , &mmdc0->mdscr); +
As mentioned earlier, you can just set mdref here, instead of using a saved value. The only assignment is currently this:
mmdc0->mdref = (0 << 14) | (3 << 11);
arch/arm/cpu/armv7/mx6/ddr.c line 205
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+/* re-enable auto refresh and zq cal */ +writel (esdmisc_val, &mmdc0->mdref);
I think you can just use clrbits32_le here and get rid of zq_val.
arch/arm/cpu/armv7/mx6/ddr.c line 216
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c +11
-0+writel (zq_val, &mmdc0->mpzqhwctrl); + +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl1)); +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl1)); +
Why the dummy reads below?
I'd like to see the values fed back in an output parameter:
calib->p0_mpwldectrl0 = readl(&mmdc0->mpwldectrl0); calib->p0_mpwldectrl1 = readl(&mmdc0->mpwldectrl1);
Absent that, I don't think a dummy read is needed.
arch/arm/cpu/armv7/mx6/ddr.c line 225
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* We must force a readback of these values, to get them to stick */ +readl (&mmdc0->mpwldectrl0); +readl (&mmdc0->mpwldectrl1); +readl (&mmdc1->mpwldectrl0); +readl (&mmdc1->mpwldectrl1); + +/* enable DDR logic power down timer: */ +setbits_le32 (&mmdc0->mdpdc,0x00005500 ); +
Again, s/Adopt/Automatic/
arch/arm/cpu/armv7/mx6/ddr.c line 234
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* Enable Adopt power down timer: */ +clrbits_le32 (&mmdc0->mapsr,0x1 ); + +/* Clear CON_REQ */ +writel (0 , &mmdc0->mdscr); + +return errors; +} +
This should also have parameters of mx6_ddr_sysinfo (input) and mx6_mmdc_calibration (output), at least for sysinfo->dsize
arch/arm/cpu/armv7/mx6/ddr.c line 238
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+int mmdc_do_dqs_calibration (void ) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ;
Note that this is specific to MX6DQ:
arch/arm/cpu/armv7/mx6/ddr.c line 251
Quoted 13 lines
arch/arm/cpu/armv7/mx6/ddr.c +13
-0+struct mx6dq_iomux_ddr_regs *mx6_ddr_iomux = + (struct mx6dq_iomux_ddr_regs *)MX6DQ_IOM_DDR_BASE ; +bool cs0_enable; +bool cs1_enable; +bool cs0_enable_initial; +bool cs1_enable_initial; +u32 esdmisc_val; +u32 bus_size; +u32 temp_ref; +u32 pddword =0x00ffff00 ;/* best so far, place into MPPDCMPR1 */ +u32 errors =0 ; +u32 initdelay =0x40404040 ; +
sysinfo can be used here instead of reading mdctl, and I think cs0_enable_initial has to be true because of the way we're using sysinfo->ncs.
i.e.
cs1_enable_initial = (sysinfo->ncs == 2);
arch/arm/cpu/armv7/mx6/ddr.c line 262
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c +11
-0+/* check to see which chip selects are enabled */ + cs0_enable_initial =readl (&mmdc0->mdctl) &0x80000000 ; + cs1_enable_initial =readl (&mmdc0->mdctl) &0x40000000 ; + +/* disable DDR logic power down timer: */ +clrbits_le32 (&mmdc0->mdpdc,0xff00 ); + +/* disable Adopt power down timer: */ +setbits_le32 (&mmdc0->mapsr,0x1 ); + +/* set DQS pull ups */
These should use clrsetbits_le32 to clear bit 15:
arch/arm/cpu/armv7/mx6/ddr.c line 272
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+setbits_le32 (&mx6_ddr_iomux->dram_sdqs0,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs1,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs2,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs3,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs4,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs5,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs6,0x7000 ); +setbits_le32 (&mx6_ddr_iomux->dram_sdqs7,0x7000 ); + +/* Save old RALAT and WALAT values */
RALAT and WALAT are in sysinfo, and esdmisc_val isn't needed.
arch/arm/cpu/armv7/mx6/ddr.c line 275
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+ esdmisc_val =readl (&mmdc0->mdmisc); + + setbits_le32(&mmdc0->mdmisc,
(3 << 16) | (7 << 6)
arch/arm/cpu/armv7/mx6/ddr.c line 277
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+ (1 << 6) | (1 << 7) | (1 << 8) | (1 << 16) | (1 << 17)); +
See previous notes about mdref.
It also seems that we'll always call both of the calibration routines in this order:
mmdc_do_write_level_calibration() mmdc_do_dqs_calibration()
So restoring refresh values in mmdc_do_write_level_calibration() only to disable them again seems pointless and perhaps the two routines should be combined.
arch/arm/cpu/armv7/mx6/ddr.c line 285
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c +8
-0+/* Disable auto refresh before proceeding with calibration */ + temp_ref =readl (&mmdc0->mdref); +writel (0x0000c000 , &mmdc0->mdref); + +/* +* Per the ref manual, issue one refresh cycle MDSCR[CMD]= 0x2, +* this also sets the CON_REQ bit. +*/
wait_for_con_ack() before the command?
Also, the cs0_enable_initial is superfluous (we have to have one chip select and don't support only CS1):
Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c +7
-0+if (cs0_enable_initial) +writel (0x00008020 , &mmdc0->mdscr); +if (cs1_enable_initial) +writel (0x00008028 , &mmdc0->mdscr); + +/* poll to make sure the con_ack bit was asserted */ +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 );
arch/arm/cpu/armv7/mx6/ddr.c line 302
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+ +/* +* Check MDMISC register CALIB_PER_CS to see which CS calibration +* is targeted to (under normal cases, it should be cleared +* as this is the default value, indicating calibration is directed +* to CS0). +* Disable the other chip select not being target for calibration +* to avoid any potential issues. This will get re-enabled at end +* of calibration. +*/
As far as I can tell, we have no way of setting bit 20 (CALIB_PER_CS), and if we did, I think we'd need additional logic, at least to clear the other bit in mdctl.
Tim, handling of these bits may be the source of the issue you're having with multiple chip selects.
arch/arm/cpu/armv7/mx6/ddr.c line 307
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+if ((readl (&mmdc0->mdmisc) &0x00100000 ) ==0 ) +clrbits_le32 (&mmdc0->mdctl,1 <<30 );/* clear SDE_1 */ +else +clrbits_le32 (&mmdc0->mdctl,1 <<31 );/* clear SDE_0 */ +
Here's what Section 5 of the app note says:
The user has the option of populating DDR memory devices on chip select 0 (CSD0), chip select 1 (CSD1), or both chip selects. The control mechanism to decide which chip select the associated calibration is targeted to is found in MDMISC[CALIB_PER_CS] bit. If the DDR memory devices are populated on CSD0, then this bit would be cleared. If these devices are populated only on CSD1, then this bit would be set. Should both chip selects be populated, it is an absolute must that the memories populated on CSD1 are ?mirrored? with the memories populated on CSD0, as both CSD share the same delay line register set during normal DDR operation. Given the similarities in memory device layout, either setting can be used for MDMISC[CALIB_PER_CS] bit. However, the examples in this document clear this bit, hence targeting calibration toward CSD0.
Given this, it seems as if the test above is useless and we should always target CS0.
arch/arm/cpu/armv7/mx6/ddr.c line 311
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+/* +* Check to see which chip selects are now enabled for +* the remainder of the calibration. +*/
And this should go away:
arch/arm/cpu/armv7/mx6/ddr.c line 314
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+ cs0_enable =readl (&mmdc0->mdctl) &0x80000000 ; + cs1_enable =readl (&mmdc0->mdctl) &0x40000000 ; +
bus_size is in sysinfo.
arch/arm/cpu/armv7/mx6/ddr.c line 318
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+/* Check to see what the data bus size is */ + bus_size = (readl (&mmdc0->mdctl) &0x30000 ) >>16 ; +debug ("Data bus size: %d (%d bits)\n" , bus_size,1 << (bus_size +4 )); +
And the parameters here can go away:
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c +8
-0+precharge_all (cs0_enable, cs1_enable); + +/* Write the pre-defined value into MPPDCMPR1 */ +writel (pddword, &mmdc0->mppdcmpr1); + +/* +* Issue a write access to the external DDR device by setting +* the bit SW_DUMMY_WR (bit 0) in the MPSWDAR0 and then poll
arch/arm/cpu/armv7/mx6/ddr.c line 336
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+* this bit until it clears to indicate completion of the write access. +*/ +setbits_le32 (&mmdc0->mpswdar0,1 ); +wait_for_bit (&mmdc0->mpswdar0,1 <<0 ,0 ); + +/* Set the RD_DL_ABS# bits to their default values +* (will be calibrated later in the read delay-line calibration). +* Both PHYs for x64 configuration, if x32, do only PHY0. +*/ +writel (initdelay, &mmdc0->mprddlctl);
if (sysinfo->dsize == 0x2)
Quoted 24 lines
arch/arm/cpu/armv7/mx6/ddr.c +24
-0+if (bus_size ==0x2 ) +writel (initdelay, &mmdc1->mprddlctl); + +/* Force a measurment, for previous delay setup to take effect. */ +force_delay_measurement (bus_size); + +/* +* *************************** +* Read DQS Gating calibration +* *************************** +*/ +debug ("Starting Read DQS Gating calibration.\n" ); + +/* +* Reset the read data FIFOs (two resets); only need to issue reset +* to PHY0 since in x64 mode, the reset will also go to PHY1. +*/ +reset_read_data_fifos (); + +/* +* Start the automatic read DQS gating calibration process by +* asserting MPDGCTRL0[HW_DG_EN] and MPDGCTRL0[DG_CMP_CYC] +* and then poll MPDGCTRL0[HW_DG_EN]] until this bit clears +* to indicate completion.
arch/arm/cpu/armv7/mx6/ddr.c line 370
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+* Also, ensure that MPDGCTRL0[HW_DG_ERR] is clear to indicate +* no errors were seen during calibration. +*/ + +/* +* Set bit 30: chooses option to wait 32 cycles instead of +* 16 before comparing read data. +*/ +setbits_le32 (&mmdc0->mpdgctrl0,1 <<30 ); +
I think you need to poll for bit 30 clear here.
arch/arm/cpu/armv7/mx6/ddr.c line 371
Quoted 1 line
arch/arm/cpu/armv7/mx6/ddr.c +1
-0+/* Set bit 28 to start automatic read DQS gating calibration */
1 << 28 ?
arch/arm/cpu/armv7/mx6/ddr.c line 384
Quoted 13 lines
arch/arm/cpu/armv7/mx6/ddr.c +13
-0+setbits_le32 (&mmdc0->mpdgctrl0,5 <<28 ); + +/* Poll for completion. MPDGCTRL0[HW_DG_EN] should be 0 */ +wait_for_bit (&mmdc0->mpdgctrl0,1 <<28 ,0 ); + +/* +* Check to see if any errors were encountered during calibration +* (check MPDGCTRL0[HW_DG_ERR]). +* Check both PHYs for x64 configuration, if x32, check only PHY0. +*/ +if (readl (&mmdc0->mpdgctrl0) &0x00001000 ) + errors |=1 ; +
sysinfo->dsize
Quoted 108 lines
arch/arm/cpu/armv7/mx6/ddr.c +108
-0+if ((bus_size ==0x2 ) && (readl (&mmdc1->mpdgctrl0) &0x00001000 )) + errors |=2 ; + +/* +* DQS gating absolute offset should be modified from +* reflecting (HW_DG_LOWx + HW_DG_UPx)/2 to +* reflecting (HW_DG_UPx - 0x80) +*/ +modify_dg_result (&mmdc0->mpdghwst0, &mmdc0->mpdghwst1, + &mmdc0->mpdgctrl0); +modify_dg_result (&mmdc0->mpdghwst2, &mmdc0->mpdghwst3, + &mmdc0->mpdgctrl1); +if (bus_size ==0x2 ) { +modify_dg_result (&mmdc1->mpdghwst0, &mmdc1->mpdghwst1, + &mmdc1->mpdgctrl0); +modify_dg_result (&mmdc1->mpdghwst2, &mmdc1->mpdghwst3, + &mmdc1->mpdgctrl1); + } +debug ("Ending Read DQS Gating calibration. Error mask: 0x%x\n" , errors); + +/* +* ********************** +* Read Delay calibration +* ********************** +*/ +debug ("Starting Read Delay calibration.\n" ); + +reset_read_data_fifos (); + +/* +* 4. Issue the Precharge-All command to the DDR device for both +* chip selects. If only using one chip select, then precharge +* only the desired chip select. +*/ +precharge_all (cs0_enable, cs1_enable); + +/* +* 9. Read delay-line calibration +* Start the automatic read calibration process by asserting +* MPRDDLHWCTL[HW_RD_DL_EN]. +*/ +writel (0x00000030 , &mmdc0->mprddlhwctl); + +/* +* 10. poll for completion +* MMDC indicates that the write data calibration had finished by +* setting MPRDDLHWCTL[HW_RD_DL_EN] = 0. Also, ensure that +* no error bits were set. +*/ +wait_for_bit (&mmdc0->mprddlhwctl,1 <<4 ,0 ); + +/* check both PHYs for x64 configuration, if x32, check only PHY0 */ +if (readl (&mmdc0->mprddlhwctl) &0x0000000f ) + errors |=4 ; + +if ((bus_size ==0x2 ) && (readl (&mmdc1->mprddlhwctl) &0x0000000f )) + errors |=8 ; + +debug ("Ending Read Delay calibration. Error mask: 0x%x\n" , errors); + +/* +* *********************** +* Write Delay Calibration +* *********************** +*/ +debug ("Starting Write Delay calibration.\n" ); + +reset_read_data_fifos (); + +/* +* 4. Issue the Precharge-All command to the DDR device for both +* chip selects. If only using one chip select, then precharge +* only the desired chip select. +*/ +precharge_all (cs0_enable, cs1_enable); + +/* +* 8. Set the WR_DL_ABS# bits to their default values. +* Both PHYs for x64 configuration, if x32, do only PHY0. +*/ +writel (initdelay, &mmdc0->mpwrdlctl); +if (bus_size ==0x2 ) +writel (initdelay, &mmdc1->mpwrdlctl); + +/* +* XXX This isn't in the manual. Force a measurement, +* for previous delay setup to effect. +*/ +force_delay_measurement (bus_size); + +/* +* 9. 10. Start the automatic write calibration process +* by asserting MPWRDLHWCTL0[HW_WR_DL_EN]. +*/ +writel (0x00000030 , &mmdc0->mpwrdlhwctl); + +/* +* Poll for completion. +* MMDC indicates that the write data calibration had finished +* by setting MPWRDLHWCTL[HW_WR_DL_EN] = 0. +* Also, ensure that no error bits were set. +*/ +wait_for_bit (&mmdc0->mpwrdlhwctl,1 <<4 ,0 ); + +/* Check both PHYs for x64 configuration, if x32, check only PHY0 */ +if (readl (&mmdc0->mpwrdlhwctl) &0x0000000f ) + errors |=16 ; +
arch/arm/cpu/armv7/mx6/ddr.c line 502
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+if ((bus_size ==0x2 ) && (readl (&mmdc1->mpwrdlhwctl) &0x0000000f )) + errors |=32 ; + +debug ("Ending Write Delay calibration. Error mask: 0x%x\n" , errors); + +reset_read_data_fifos (); + +/* Enable DDR logic power down timer */ +setbits_le32 (&mmdc0->mdpdc,0x00005500 ); +
s/Adopt/Automatic/
arch/arm/cpu/armv7/mx6/ddr.c line 505
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+/* Enable Adopt power down timer */ +clrbits_le32 (&mmdc0->mapsr,0x1 ); +
sysinfo->ralat, sysinfo->walat
arch/arm/cpu/armv7/mx6/ddr.c line 508
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+/* Restore MDMISC value (RALAT, WALAT) to MMDCP1 */ +writel (esdmisc_val, &mmdc0->mdmisc); +
i.e.
clrsetbits_le32(&mmdc0->mdmisc, (3 << 16) | (7 << 6), (sysinfo->walat << 16) | (sysinfo->ralat << 6));
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+/* Clear DQS pull ups */ +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs0,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs1,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs2,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs3,0x7000 );
arch/arm/cpu/armv7/mx6/ddr.c line 523
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+clrbits_le32 (&mx6_ddr_iomux->dram_sdqs4,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs5,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs6,0x7000 ); +clrbits_le32 (&mx6_ddr_iomux->dram_sdqs7,0x7000 ); + +/* Re-enable SDE (chip selects) if they were set initially */ +if (cs1_enable_initial) +/* Set SDE_1 */ +setbits_le32 (&mmdc0->mdctl,1 <<30 ); +
Again, I think this test is superfluous since we don't support a routing of CS1 by itself.
arch/arm/cpu/armv7/mx6/ddr.c line 527
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+if (cs0_enable_initial) +/* Set SDE_0 */ +setbits_le32 (&mmdc0->mdctl,1 <<31 ); +
writel((0 << 14) | /* REF_SEL: Periodic refresh cycle: 64kHz */ (3 << 11) /* REFR: Refresh Rate - 4 refreshes */, &mmdc0->mdref);
Quoted 16 lines
arch/arm/cpu/armv7/mx6/ddr.c +16
-0+/* Re-enable to auto refresh */ +writel (temp_ref, &mmdc0->mdref); + +/* Clear the MDSCR (including the con_req bit) */ +writel (0x0 , &mmdc0->mdscr);/* CS0 */ + +/* Poll to make sure the con_ack bit is clear */ +wait_for_bit (&mmdc0->mdscr,1 <<14 ,0 ); + +/* +* Print out the registers that were updated as a result +* of the calibration process. +*/ +debug ("MMDC registers updated from calibration\n" ); +debug ("Read DQS gating calibration:\n" ); +debug ("\tMPDGCTRL0 PHY0 = 0x%08X\n" ,readl (&mmdc0->mpdgctrl0));
arch/arm/cpu/armv7/mx6/ddr.c line 553
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+debug ("\tMPDGCTRL1 PHY0 = 0x%08X\n" ,readl (&mmdc0->mpdgctrl1)); +debug ("\tMPDGCTRL0 PHY1 = 0x%08X\n" ,readl (&mmdc1->mpdgctrl0)); +debug ("\tMPDGCTRL1 PHY1 = 0x%08X\n" ,readl (&mmdc1->mpdgctrl1)); +debug ("Read calibration:\n" ); +debug ("\tMPRDDLCTL PHY0 = 0x%08X\n" ,readl (&mmdc0->mprddlctl)); +debug ("\tMPRDDLCTL PHY1 = 0x%08X\n" ,readl (&mmdc1->mprddlctl)); +debug ("Write calibration:\n" ); +debug ("\tMPWRDLCTL PHY0 = 0x%08X\n" ,readl (&mmdc0->mpwrdlctl)); +debug ("\tMPWRDLCTL PHY1 = 0x%08X\n" ,readl (&mmdc1->mpwrdlctl)); +
Here's where it would be useful to update a calibration output parameter:
calib->p0_mpdgctrl0 = readl(&mmdc0->mpdgctrl0);
calib->p0_mpdgctrl1 = readl(&mmdc0->mpdgctrl1);
calib->p0_mprddlctl = readl(&mmdc0->mprddlctl);
calib->p0_mpwrdlctl = readl(&mmdc0->mpwrdlctl);
if (sysinfo->dsize == 2) {
calib->p1_mpwldectrl0 = readl(&mmdc1->mpwldectrl0);
calib->p1_mpwldectrl1 = readl(&mmdc1->mpwldectrl1);
calib->p1_mpdgctrl0 = readl(&mmdc1->mpdgctrl0);
calib->p1_mpdgctrl1 = readl(&mmdc1->mpdgctrl1);
calib->p1_mprddlctl = readl(&mmdc1->mprddlctl);
calib->p1_mpwrdlctl = readl(&mmdc1->mpwrdlctl);
}Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c +7
-0+/* +* Registers below are for debugging purposes. These print out +* the upper and lower boundaries captured during +* read DQS gating calibration. +*/ +debug ("Status registers bounds for read DQS gating:\n" ); +debug ("\tMPDGHWST0 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst0));
arch/arm/cpu/armv7/mx6/ddr.c line 570
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+debug ("\tMPDGHWST1 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst1)); +debug ("\tMPDGHWST2 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst2)); +debug ("\tMPDGHWST3 PHY0 = 0x%08x\n" ,readl (&mmdc0->mpdghwst3)); +debug ("\tMPDGHWST0 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst0)); +debug ("\tMPDGHWST1 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst1)); +debug ("\tMPDGHWST2 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst2)); +debug ("\tMPDGHWST3 PHY1 = 0x%08x\n" ,readl (&mmdc1->mpdghwst3)); + +debug ("Final do_dqs_calibration error mask: 0x%x\n" , errors); +
Finally, I'm not sure what the current caller(s) will do if errors are returned from either of these two routines...
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+ return errors; +} +#endif +
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 11 lines · Eric Nelson
Hi Marek,
Quoted 3 lines · Marek Vasut
Add DDR3 calibration code for i.MX6Q, i.MX6D and i.MX6DL. This code fine-tunes the behavior of the MMDC controller in order to improve the signal integrity and memory stability.
It would be good to have a reference to AN4467 in the commit message, since that document is the best reference to understanding this code:
http://cache.freescale.com/files/32bit/doc/app_note/AN4467.pdf
OK
Quoted 9 lines
Quoted 8 lines
Signed-off-by Marek Vasut <[email protected]>Cc Stefano Babic <[email protected]>---
arch/arm/cpu/armv7/mx6/ddr.c | 559 ++++++++++++++++++++++++++++++++ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 5 + 2 files changed, 564 insertions(+)
Quoted 5 lines
Note that there's another implementation of this in the barebox project:
http://git.pengutronix.de/?p=barebox.git;a=blob;f=arch/arm/mach-imx/imx6-mm
dc.c;h=146df573e4eeed7132e93899b4b539e842bb6ba2;hb=HEAD
Yeah, I sent them a patch for it too ;-)
Quoted 9 lines
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +0
-0@@ -13,6 +13,565 @@#include <asm/io.h> #include <asm/types.h>
Quoted 8 lines
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +1
-0+#if defined(CONFIG_MX6QDL) || defined(CONFIG_MX6Q) ||
defined(CONFIG_MX6D)+Should this use the common
wait_for_bit?http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#237952I'm not certain that the <ctrl-c> handling is appropriate when used here.
Yes, that's why I called it exactly the same, so once the patches above hit mainline, I could just nuke this function and replace it with the common one.
Quoted 50 lines
Quoted 23 lines
arch/arm/cpu/armv7/mx6/ddr.c +23
-0+static int wait_for_bit (void *reg,const uint32_t mask,bool set) +{ +unsigned int timeout =1000 ; +u32 val; + +while (--timeout) { + val =readl (reg); +if (!set) + val = ~val; + +if ((val & mask) == mask) +return 0 ; + +udelay (1 ); + } + +printf ("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n" , + __func__, reg, mask, set); +hang ();/* DRAM couldn't be calibrated, game over :-( */ +} + +static void reset_read_data_fifos (void ) +{arch/arm/cpu/armv7/mx6/ddr.c line 50
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; + +/* Reset data FIFOs twice. */ +setbits_le32 (&mmdc0->mpdgctrl0,1 <<31 ); +wait_for_bit (&mmdc0->mpdgctrl0,1 <<31 ,0 ); + +setbits_le32 (&mmdc0->mpdgctrl0,1 <<31 ); +wait_for_bit (&mmdc0->mpdgctrl0,1 <<31 ,0 ); +} +See comments below about chip-selects during calibration.
arch/arm/cpu/armv7/mx6/ddr.c line 60
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+static void precharge_all (const bool cs0_enable,const bool cs1_enable) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; + +/* +* Issue the Precharge-All command to the DDR device for both +* chip selects. Note, CON_REQ bit should also remain set. If +* only using one chip select, then precharge only the desired +* chip select. +*/ AN4467 says that a wait for tRP is needed here, though none of the other implementations explicitly do that.
Quoted 4 lines
The wait for
CON_ACKseems like a good idea, though I wonder if the waits shouldn't be performed **before** the commands are issued.I'd also like to see a named constant for
MDSCR_CON_ACK
That'd be great. Do you know of some header where the MMDC register bits are defined ? I am definitelly not gonna rewrite the the i.MX6 MMDC bits from the datasheet by hand.
Quoted 2 lines
, and since
waiting for it is done in multiple places, perhapswait_for_con_ack().
Implementing a function for the sake of just wrapping another function seems wasteful to me.
Quoted 29 lines
Quoted 29 lines
arch/arm/cpu/armv7/mx6/ddr.c +29
-0+if (cs0_enable) {/* CS0 */ +writel (0x04008050 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } + +if (cs1_enable) {/* CS1 */ +writel (0x04008058 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } +} + +static void force_delay_measurement (int bus_size) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; + +writel (0x800 , &mmdc0->mpmur0); +if (bus_size ==0x2 ) +writel (0x800 , &mmdc1->mpmur0); +} + +static void modify_dg_result (u32 *reg_st0,u32 *reg_st1,u32 *reg_ctrl) +{ +u32 dg_tmp_val, dg_dl_abs_offset, dg_hc_del, val_ctrl; + +/* +* DQS gating absolute offset should be modified from reflecting +* (HW_DG_LOWx + HW_DG_UPx)/2 to reflecting (HW_DG_UPx - 0x80) +*/
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c line 93
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+ + val_ctrl =readl (reg_ctrl); + val_ctrl &=0xf0000000 ; +Though this matches the app note and other implementations, I think this would be easier to understand by shifting first and anding with
0x7ff.
I think it is better to stick with the appnote, so it's easy to map the code and the appnote between one another.
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c line 94
Quoted 1 line
arch/arm/cpu/armv7/mx6/ddr.c +1
-0+ dg_tmp_val = ((readl (reg_st0) &0x07ff0000 ) >>16 ) -0xc0 ;Especially since these calculations operate on the shifted values:
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+ dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; + + val_ctrl |= dg_dl_abs_offset + dg_hc_del; + + dg_tmp_val = ((readl (reg_st1) &0x07ff0000 ) >>16 ) -0xc0 ; + dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; +
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c line 108
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+ val_ctrl |= (dg_dl_abs_offset + dg_hc_del) <<16 ; + +writel (val_ctrl, reg_ctrl); +} +I'd recommend passing parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output) to this routine.
Why exactly ?
Quoted 13 lines
arch/arm/cpu/armv7/mx6/ddr.c line 115
Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c +7
-0+int mmdc_do_write_level_calibration (void ) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; +u32 esdmisc_val, zq_val; +u32 errors =0 ; +u32 ldectrl[4 ];I believe the 4 here comes from this calculation:
ddr_mr1 = ((sysinfo->rtt_nom & 1) ? 1 : 0) << 2 | ((sysinfo->rtt_nom & 2) ? 1 : 0) << 6;
Quoted 6 lines
arch/arm/cpu/armv7/mx6/ddr.c
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+u32 ddr_mr1 =0x4 ; +I'm not sure how this is useful (trying to proceed if things fail).
Have you seen this triggered?
What ? I see an assignment , what would trigger here ?
Quoted 15 lines
arch/arm/cpu/armv7/mx6/ddr.c line 129
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c +12
-0+/* +* Stash old values in case calibration fails, +* we need to restore them +*/ + ldectrl[0 ] =readl (&mmdc0->mpwldectrl0); + ldectrl[1 ] =readl (&mmdc0->mpwldectrl1); + ldectrl[2 ] =readl (&mmdc1->mpwldectrl0); + ldectrl[3 ] =readl (&mmdc1->mpwldectrl1); + +/* disable DDR logic power down timer */ +clrbits_le32 (&mmdc0->mdpdc,0xff00 ); +Either s/Adopt/Automatic/ or just get rid of Adopt...
Quoted 1 line
This is a typo in the app note.
OK
Quoted 5 lines
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+/* disable Adopt power down timer */ +setbits_le32 (&mmdc0->mapsr,0x1 ); + +debug ("Starting write leveling calibration.\n" ); +
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c line 138
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+/* +* 2. disable auto refresh and ZQ calibration +* before proceeding with Write Leveling calibration +*/ Trusting the esdmisc value read from here seems like a bad idea. It would be clearer if the code below that restores things just (re)configures mdref directly.
Can you elaborate some more on why is it a bad idea?
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c line 141
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+ esdmisc_val =readl (&mmdc0->mdref); +writel (0x0000C000 , &mmdc0->mdref); + zq_val =readl (&mmdc0->mpzqhwctrl);
clrbits_le32?
The zq_val is used further in the code, so no.
Quoted 16 lines
arch/arm/cpu/armv7/mx6/ddr.c line 144
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+writel (zq_val & ~0x3 , &mmdc0->mpzqhwctrl); + +/* 3. increase walat and ralat to maximum */ (3 << 16) | (7 << 6) would be clearer, since these are numeric values in tbe bitfields.
arch/arm/cpu/armv7/mx6/ddr.c line 148
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+setbits_le32 (&mmdc0->mdmisc, + (1 <<6 ) | (1 <<7 ) | (1 <<8 ) | (1 <<16 ) | (1 <<17 )); +setbits_le32 (&mmdc1->mdmisc, + (1 <<6 ) | (1 <<7 ) | (1 <<8 ) | (1 <<16 ) | (1 <<17 ));I had a concern about this bit of code (setting RALAT and WALAT to maximum values). It seems odd that we calibrate with one set of delays and then choose a different set later.
Quoted 2 lines
Experimentally, I've found using min or max values seems to have no effect on the result.
OK
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 157
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* +* 4 & 5. Configure the external DDR device to enter write-leveling +* mode through Load Mode Register command. +* Register setting: +* Bits[31:16] MR1 value (0x0080 write leveling enable) +* Bit[9] set WL_EN to enable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/
wait_for_con_ack()seems like a good idea here:
Why ?
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 166
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+writel (0x00808231 , &mmdc0->mdscr); + +/* 6. Activate automatic calibration by setting MPWLGCR[HW_WL_EN] */ +writel (0x00000001 , &mmdc0->mpwlgcr); + +/* +* 7. Upon completion of this process the MMDC de-asserts +* the MPWLGCR[HW_WL_EN] +*/ Why 1 << 0?
It waits for the autocalibration to finish, see 6. above.
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 174
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c +8
-0+wait_for_bit (&mmdc0->mpwlgcr,1 <<0 ,0 ); + +/* +* 8. check for any errors: check both PHYs for x64 configuration, +* if x32, check only PHY0 +*/ +if (readl (&mmdc0->mpwlgcr) &0x00000F00 ) + errors |=1 ;This should be conditional on (
sysinfo->dsize== 2) to
Quoted 1 line
allow use with a 32-bit bus:
I believe mmdc1 will contain 0x0 << 8 if you use 32bit bus, right ?
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 179
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+if (readl (&mmdc1->mpwlgcr) &0x00000F00 ) + errors |=2 ; + +debug ("Ending write leveling calibration. Error mask: 0x%x\n" , errors); +Where are you getting this test from?
I have code that tests each byte individually against a max value of
0x2f, but I'm having trouble finding the source of the test at the moment.
This one came I think from the freescale code from novena repo.
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c line 182
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+/* check to see if cal failed */ + if ((readl(&mmdc0->mpwldectrl0) == 0x001F001F) && + (readl(&mmdc0->mpwldectrl1) == 0x001F001F) &&Ditto (
sysinfo->dsize== 2):arch/arm/cpu/armv7/mx6/ddr.c
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+ (readl(&mmdc1->mpwldectrl0) == 0x001F001F) && + (readl(&mmdc1->mpwldectrl1) == 0x001F001F)) { + debug("Cal seems to have soft-failed due to memory not
supporting
Quoted 45 lines
Quoted 9 lines
write leveling on all channels. Restoring original write leveling
values.\n"); +writel(ldectrl[0], &mmdc0->mpwldectrl0);arch/arm/cpu/armv7/mx6/ddr.c +7
-0+writel (ldectrl[1 ], &mmdc0->mpwldectrl1); +writel (ldectrl[2 ], &mmdc1->mpwldectrl0); +writel (ldectrl[3 ], &mmdc1->mpwldectrl1); + errors |=4 ; + } + +/* arch/arm/cpu/armv7/mx6/ddr.c line 203
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+* User should issue MRS command to exit write leveling mode +* through Load Mode Register command +* Register setting: +* Bits[31:16] MR1 value "ddr_mr1" value from initialization +* Bit[9] clear WL_EN to disable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/ +writel ((ddr_mr1 <<16 ) +0x8031 , &mmdc0->mdscr); +As mentioned earlier, you can just set mdref here, instead of using a saved value. The only assignment is currently this:
mmdc0->mdref = (0 << 14) | (3 << 11);arch/arm/cpu/armv7/mx6/ddr.c line 205
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+/* re-enable auto refresh and zq cal */ +writel (esdmisc_val, &mmdc0->mdref);I think you can just use
clrbits32_lehere and get rid ofzq_val.arch/arm/cpu/armv7/mx6/ddr.c line 216
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c +11
-0+writel (zq_val, &mmdc0->mpzqhwctrl); + +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl1)); +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl1)); +Why the dummy reads below?
Quoted 5 lines
I'd like to see the values fed back in an output parameter:
calib->p0_mpwldectrl0 = readl(&mmdc0->mpwldectrl0); calib->p0_mpwldectrl1 = readl(&mmdc0->mpwldectrl1);Absent that, I don't think a dummy read is needed.
You do not need those values anymore, since they are already loaded into the hardware ... or why would you want them to be adjusted ?
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c line 225
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* We must force a readback of these values, to get them to stick */ +readl (&mmdc0->mpwldectrl0); +readl (&mmdc0->mpwldectrl1); +readl (&mmdc1->mpwldectrl0); +readl (&mmdc1->mpwldectrl1); + +/* enable DDR logic power down timer: */ +setbits_le32 (&mmdc0->mdpdc,0x00005500 ); +Again, s/Adopt/Automatic/
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c line 234
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* Enable Adopt power down timer: */ +clrbits_le32 (&mmdc0->mapsr,0x1 ); + +/* Clear CON_REQ */ +writel (0 , &mmdc0->mdscr); + +return errors; +} +This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsize
Would it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
[...]
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Marek,
Quoted 2 lines · Marek Vasut
Quoted 1 line · Eric Nelson
On 12/16/2015 07:40 AM, Marek Vasut wrote:
...
Quoted 15 lines
Quoted 15 lines
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c +1
-0@@ -13,6 +13,565 @@#include <asm/io.h> #include <asm/types.h> +#if defined(CONFIG_MX6QDL) || defined(CONFIG_MX6Q) ||
defined(CONFIG_MX6D)+Should this use the common
wait_for_bit?http://lists.denx.de/pipermail/u-boot/2015-December/thread.html#237952
Quoted 6 lines
Quoted 2 lines
I'm not certain that the <ctrl-c> handling is appropriate when used here.
Yes, that's why I called it exactly the same, so once the patches above hit mainline, I could just nuke this function and replace it with the common one.
Sounds good.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+static int wait_for_bit (void *reg,const uint32_t mask,bool set) +{
...
Quoted 17 lines
Quoted 17 lines
See comments below about chip-selects during calibration.
arch/arm/cpu/armv7/mx6/ddr.c line 60
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+static void precharge_all (const bool cs0_enable,const bool cs1_enable) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; + +/* +* Issue the Precharge-All command to the DDR device for both +* chip selects. Note, CON_REQ bit should also remain set. If +* only using one chip select, then precharge only the desired +* chip select. +*/ AN4467 says that a wait for tRP is needed here, though none of the other implementations explicitly do that.
Quoted 7 lines
Quoted 4 lines
The wait for
CON_ACKseems like a good idea, though I wonder if the waits shouldn't be performed **before** the commands are issued.I'd also like to see a named constant for
MDSCR_CON_ACKThat'd be great. Do you know of some header where the MMDC register bits are defined ?
I don't.
I think this is stuff that's supposed to be done by the boot loader ;).
Quoted 3 lines
I am definitelly not gonna rewrite the the i.MX6 MMDC bits from the datasheet by hand.
How about just one constant?
Quoted 6 lines
Quoted 2 lines
, and since
waiting for it is done in multiple places, perhapswait_for_con_ack().Implementing a function for the sake of just wrapping another function seems wasteful to me.
I was thinking macro, but okay.
Quoted 38 lines
Quoted 29 lines
Quoted 29 lines
arch/arm/cpu/armv7/mx6/ddr.c +29
-0+if (cs0_enable) {/* CS0 */ +writel (0x04008050 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } + +if (cs1_enable) {/* CS1 */ +writel (0x04008058 , &mmdc0->mdscr); +wait_for_bit (&mmdc0->mdscr,1 <<14 ,1 ); + } +} + +static void force_delay_measurement (int bus_size) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; + +writel (0x800 , &mmdc0->mpmur0); +if (bus_size ==0x2 ) +writel (0x800 , &mmdc1->mpmur0); +} + +static void modify_dg_result (u32 *reg_st0,u32 *reg_st1,u32 *reg_ctrl) +{ +u32 dg_tmp_val, dg_dl_abs_offset, dg_hc_del, val_ctrl; + +/* +* DQS gating absolute offset should be modified from reflecting +* (HW_DG_LOWx + HW_DG_UPx)/2 to reflecting (HW_DG_UPx - 0x80) +*/ Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c line 93
Quoted 4 lines
arch/arm/cpu/armv7/mx6/ddr.c +4
-0+ + val_ctrl =readl (reg_ctrl); + val_ctrl &=0xf0000000 ; +Though this matches the app note and other implementations, I think this would be easier to understand by shifting first and anding with
0x7ff.
Quoted 3 lines
I think it is better to stick with the appnote, so it's easy to map the code and the appnote between one another.
Okay. My hope is that a future app note will refer to the new code...
Quoted 21 lines
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c line 94
Quoted 1 line
arch/arm/cpu/armv7/mx6/ddr.c +1
-0+ dg_tmp_val = ((readl (reg_st0) &0x07ff0000 ) >>16 ) -0xc0 ;Especially since these calculations operate on the shifted values:
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+ dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; + + val_ctrl |= dg_dl_abs_offset + dg_hc_del; + + dg_tmp_val = ((readl (reg_st1) &0x07ff0000 ) >>16 ) -0xc0 ; + dg_dl_abs_offset = dg_tmp_val &0x7f ; + dg_hc_del = (dg_tmp_val &0x780 ) <<1 ; +Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c line 108
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+ val_ctrl |= (dg_dl_abs_offset + dg_hc_del) <<16 ; + +writel (val_ctrl, reg_ctrl); +} +I'd recommend passing parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output) to this routine.
Quoted 2 lines
Why exactly ?
I sent a note about this in response to Tim's comment.
Quoted 13 lines
Quoted 13 lines
arch/arm/cpu/armv7/mx6/ddr.c line 115
Quoted 7 lines
arch/arm/cpu/armv7/mx6/ddr.c +7
-0+int mmdc_do_write_level_calibration (void ) +{ +struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR ; +struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR ; +u32 esdmisc_val, zq_val; +u32 errors =0 ; +u32 ldectrl[4 ];I believe the 4 here comes from this calculation:
ddr_mr1 = ((sysinfo->rtt_nom & 1) ? 1 : 0) << 2 | ((sysinfo->rtt_nom & 2) ? 1 : 0) << 6;
Quoted 9 lines
Quoted 6 lines
arch/arm/cpu/armv7/mx6/ddr.c
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+u32 ddr_mr1 =0x4 ; +I'm not sure how this is useful (trying to proceed if things fail).
Have you seen this triggered?
What ? I see an assignment , what would trigger here ?
This code appears to be saving calibration data so that it can be restored later in the case of failure.
I don't understand why.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c +12
-0+/* +* Stash old values in case calibration fails, +* we need to restore them +*/ + ldectrl[0 ] =readl (&mmdc0->mpwldectrl0); + ldectrl[1 ] =readl (&mmdc0->mpwldectrl1); + ldectrl[2 ] =readl (&mmdc1->mpwldectrl0); + ldectrl[3 ] =readl (&mmdc1->mpwldectrl1); + +/* disable DDR logic power down timer */ +clrbits_le32 (&mmdc0->mdpdc,0xff00 ); +
...
Quoted 6 lines
Quoted 3 lines
Trusting the esdmisc value read from here seems like a bad idea. It would be clearer if the code below that restores things just (re)configures mdref directly.
Can you elaborate some more on why is it a bad idea?
This is another case where you're assuming prior configuration of registers, when the value written should be based on the board's configuration.
Using an input sysinfo structure makes that clearer.
Quoted 8 lines
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c line 141
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+ esdmisc_val =readl (&mmdc0->mdref); +writel (0x0000C000 , &mmdc0->mdref); + zq_val =readl (&mmdc0->mpzqhwctrl);
clrbits_le32?The
zq_valis used further in the code, so no.
zq_val can go away.
Just clrbits_le32 here and set the bits later.
ZQ_MODE should always be 3 on exit from the routine.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+writel (zq_val & ~0x3 , &mmdc0->mpzqhwctrl); + +/* 3. increase walat and ralat to maximum */
...
Quoted 12 lines
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 157
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* +* 4 & 5. Configure the external DDR device to enter write-leveling +* mode through Load Mode Register command. +* Register setting: +* Bits[31:16] MR1 value (0x0080 write leveling enable) +* Bit[9] set WL_EN to enable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/
wait_for_con_ack()seems like a good idea here:
Quoted 2 lines
Why ?
The RM says that the CON_ACK bit should be polled after issuing commands with the CON_REQ bit set.
This isn't always done in the code from the app note, but it's not clear that this is by design.
Quoted 12 lines
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 166
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+writel (0x00808231 , &mmdc0->mdscr); + +/* 6. Activate automatic calibration by setting MPWLGCR[HW_WL_EN] */ +writel (0x00000001 , &mmdc0->mpwlgcr); + +/* +* 7. Upon completion of this process the MMDC de-asserts +* the MPWLGCR[HW_WL_EN] +*/ Why 1 << 0?
Quoted 2 lines
It waits for the autocalibration to finish, see 6. above.
This was a nit-pick: why not simply 1?
Quoted 11 lines
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c line 174
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c +8
-0+wait_for_bit (&mmdc0->mpwlgcr,1 <<0 ,0 ); + +/* +* 8. check for any errors: check both PHYs for x64 configuration, +* if x32, check only PHY0 +*/ +if (readl (&mmdc0->mpwlgcr) &0x00000F00 ) + errors |=1 ;This should be conditional on (
sysinfo->dsize== 2) to
Quoted 4 lines
Quoted 1 line
allow use with a 32-bit bus:
I believe mmdc1 will contain
0x0<< 8 if you use 32bit bus, right ?
This will become important when we support devices that only have one MMDC. (See MMDC1 macro)
Quoted 8 lines
Quoted 8 lines
arch/arm/cpu/armv7/mx6/ddr.c line 179
Quoted 5 lines
arch/arm/cpu/armv7/mx6/ddr.c +5
-0+if (readl (&mmdc1->mpwlgcr) &0x00000F00 ) + errors |=2 ; + +debug ("Ending write leveling calibration. Error mask: 0x%x\n" , errors); +Where are you getting this test from?
Quoted 6 lines
Quoted 3 lines
I have code that tests each byte individually against a max value of
0x2f, but I'm having trouble finding the source of the test at the moment.This one came I think from the freescale code from novena repo.
I think this test is wrong, and we should really find the proper test for failure.
Quoted 32 lines
Quoted 32 lines
arch/arm/cpu/armv7/mx6/ddr.c line 182
Quoted 3 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+/* check to see if cal failed */ + if ((readl(&mmdc0->mpwldectrl0) == 0x001F001F) && + (readl(&mmdc0->mpwldectrl1) == 0x001F001F) &&Ditto (
sysinfo->dsize== 2):Quoted 13 lines
arch/arm/cpu/armv7/mx6/ddr.c +3
-0+ (readl(&mmdc1->mpwldectrl0) == 0x001F001F) && + (readl(&mmdc1->mpwldectrl1) == 0x001F001F)) { + debug("Cal seems to have soft-failed due to memory notsupporting
write leveling on all channels. Restoring original write leveling
values.\n"); +writel(ldectrl[0], &mmdc0->mpwldectrl0);arch/arm/cpu/armv7/mx6/ddr.c +7
-0+writel (ldectrl[1 ], &mmdc0->mpwldectrl1); +writel (ldectrl[2 ], &mmdc1->mpwldectrl0); +writel (ldectrl[3 ], &mmdc1->mpwldectrl1); + errors |=4 ; + } + +/* arch/arm/cpu/armv7/mx6/ddr.c line 203
Quoted 10 lines
arch/arm/cpu/armv7/mx6/ddr.c +10
-0+* User should issue MRS command to exit write leveling mode +* through Load Mode Register command +* Register setting: +* Bits[31:16] MR1 value "ddr_mr1" value from initialization +* Bit[9] clear WL_EN to disable MMDC DQS output +* Bits[6:4] set CMD bits for Load Mode Register programming +* Bits[2:0] set CMD_BA to 0x1 for DDR MR1 programming +*/ +writel ((ddr_mr1 <<16 ) +0x8031 , &mmdc0->mdscr); +As mentioned earlier, you can just set mdref here, instead of using a saved value. The only assignment is currently this:
Quoted 8 lines
Quoted 8 lines
mmdc0->mdref = (0 << 14) | (3 << 11);arch/arm/cpu/armv7/mx6/ddr.c line 205
Quoted 2 lines
arch/arm/cpu/armv7/mx6/ddr.c +2
-0+/* re-enable auto refresh and zq cal */ +writel (esdmisc_val, &mmdc0->mdref);I think you can just use
clrbits32_lehere and get rid ofzq_val.
Sorry: I mean setbits_le32:
setbits_le32(&mmdc0->mpzqhwctrl, 3);
Quoted 14 lines
Quoted 14 lines
arch/arm/cpu/armv7/mx6/ddr.c line 216
Quoted 11 lines
arch/arm/cpu/armv7/mx6/ddr.c +11
-0+writel (zq_val, &mmdc0->mpzqhwctrl); + +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc0->mpwldectrl1)); +debug ("\tMMDC_MPWLDECTRL0 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl0)); +debug ("\tMMDC_MPWLDECTRL1 after write level cal: 0x%08X\n" , +readl (&mmdc1->mpwldectrl1)); +Why the dummy reads below?
Quoted 9 lines
Quoted 5 lines
I'd like to see the values fed back in an output parameter:
calib->p0_mpwldectrl0 = readl(&mmdc0->mpwldectrl0); calib->p0_mpwldectrl1 = readl(&mmdc0->mpwldectrl1);Absent that, I don't think a dummy read is needed.
You do not need those values anymore, since they are already loaded into the hardware ... or why would you want them to be adjusted ?
Not adjusted, just returned so they can be printf'd by a caller.
Still, why the dummy reads?
Quoted 25 lines
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c line 225
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* We must force a readback of these values, to get them to stick */ +readl (&mmdc0->mpwldectrl0); +readl (&mmdc0->mpwldectrl1); +readl (&mmdc1->mpwldectrl0); +readl (&mmdc1->mpwldectrl1); + +/* enable DDR logic power down timer: */ +setbits_le32 (&mmdc0->mdpdc,0x00005500 ); +Again, s/Adopt/Automatic/
Quoted 12 lines
arch/arm/cpu/armv7/mx6/ddr.c line 234
Quoted 9 lines
arch/arm/cpu/armv7/mx6/ddr.c +9
-0+/* Enable Adopt power down timer: */ +clrbits_le32 (&mmdc0->mapsr,0x1 ); + +/* Clear CON_REQ */ +writel (0 , &mmdc0->mdscr); + +return errors; +} +This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsize
Quoted 5 lines
Would it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
I think that's a bit of overstatement, but I'm okay sending patches in principle.
I do think that at least the test for calibration failure should be fixed before your patch is applied.
This also has the benefit of allowing discussion about each of my points individually instead of in one e-mail thread.
Regards,
Eric
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 1 line · Eric Nelson
Hi Marek,
Hi Eric,
[..]
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsizeWould it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
Quoted 8 lines
I think that's a bit of overstatement, but I'm okay sending patches in principle.
I do think that at least the test for calibration failure should be fixed before your patch is applied.
This also has the benefit of allowing discussion about each of my points individually instead of in one e-mail thread.
I have to admit I'm a bit lost in this. What do you say we ask Stefano to apply this so people can start fiddling with it. I'd also like to see the patches for MX6SX and your fixes (if you feel like it).
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Marek,
Quoted 22 lines · Marek Vasut
Quoted 1 line · Eric Nelson
Hi Marek,
Hi Eric,
[..]
Quoted 15 lines
Quoted 8 lines
Quoted 2 lines
This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsizeWould it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
I think that's a bit of overstatement, but I'm okay sending patches in principle.
I do think that at least the test for calibration failure should be fixed before your patch is applied.
Quoted 7 lines
Quoted 2 lines
This also has the benefit of allowing discussion about each of my points individually instead of in one e-mail thread.
I have to admit I'm a bit lost in this. What do you say we ask Stefano to apply this so people can start fiddling with it. I'd also like to see the patches for MX6SX and your fixes (if you feel like it).
I'm okay with that.
I was hoping to check out the error handling code but unfortunately, the boards I have that are supporting SPL are all either i.MX6DL, i.MX6S, or i.MX6SL.
You should be able to force a failure by setting WALAT and/or RALAT to extreme values and see how the DDR controller responds.
Regards,
Eric
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 1 line · Eric Nelson
Hi Marek,
Hi!
Quoted 30 lines · Marek Vasut
Quoted 22 lines · Eric Nelson
Quoted 1 line
Hi Marek,
Hi Eric,
[..]
Quoted 15 lines
Quoted 8 lines
Quoted 2 lines
This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsizeWould it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
I think that's a bit of overstatement, but I'm okay sending patches in principle.
I do think that at least the test for calibration failure should be fixed before your patch is applied.
Quoted 6 lines
Quoted 2 lines
This also has the benefit of allowing discussion about each of my points individually instead of in one e-mail thread.
I have to admit I'm a bit lost in this. What do you say we ask Stefano to apply this so people can start fiddling with it. I'd also like to see the patches for MX6SX and your fixes (if you feel like it).
Quoted 8 lines
I'm okay with that.
I was hoping to check out the error handling code but unfortunately, the boards I have that are supporting SPL are all either i.MX6DL, i.MX6S, or i.MX6SL.
You should be able to force a failure by setting WALAT and/or RALAT to extreme values and see how the DDR controller responds.
I only have Q ;-) The DDR calibration code _should_ work on DL though.
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 37 lines · Marek Vasut
Quoted 1 line · Eric Nelson
Hi Marek,
Hi!
Quoted 32 lines · Marek Vasut
Quoted 22 lines · Eric Nelson
Quoted 1 line
Hi Marek,
Hi Eric,
[..]
Quoted 15 lines
Quoted 8 lines
Quoted 2 lines
This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsizeWould it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
I think that's a bit of overstatement, but I'm okay sending patches in principle.
I do think that at least the test for calibration failure should be fixed before your patch is applied.
Quoted 6 lines
Quoted 2 lines
This also has the benefit of allowing discussion about each of my points individually instead of in one e-mail thread.
I have to admit I'm a bit lost in this. What do you say we ask Stefano to apply this so people can start fiddling with it. I'd also like to see the patches for MX6SX and your fixes (if you feel like it).
I'm okay with that.
Quoted 9 lines
Quoted 6 lines
I was hoping to check out the error handling code but unfortunately, the boards I have that are supporting SPL are all either i.MX6DL, i.MX6S, or i.MX6SL.
You should be able to force a failure by setting WALAT and/or RALAT to extreme values and see how the DDR controller responds.
I only have Q ;-) The DDR calibration code _should_ work on DL though.
It at least needs a change to some #ifdefs:
+#if defined(CONFIG_MX6QDL) || defined(CONFIG_MX6Q) || defined(CONFIG_MX6D)
Regards,
Eric
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 42 lines · Eric Nelson
Quoted 41 lines · Marek Vasut
Quoted 1 line · Eric Nelson
Hi Marek,
Hi!
Quoted 36 lines · Marek Vasut
Quoted 22 lines · Eric Nelson
Quoted 1 line
Hi Marek,
Hi Eric,
[..]
Quoted 15 lines
Quoted 8 lines
Quoted 2 lines
This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsizeWould it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
I think that's a bit of overstatement, but I'm okay sending patches in principle.
I do think that at least the test for calibration failure should be fixed before your patch is applied.
Quoted 6 lines
Quoted 2 lines
This also has the benefit of allowing discussion about each of my points individually instead of in one e-mail thread.
I have to admit I'm a bit lost in this. What do you say we ask Stefano to apply this so people can start fiddling with it. I'd also like to see the patches for MX6SX and your fixes (if you feel like it).
I'm okay with that.
I was hoping to check out the error handling code but unfortunately, the boards I have that are supporting SPL are all either i.MX6DL, i.MX6S, or i.MX6SL.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
You should be able to force a failure by setting WALAT and/or RALAT to extreme values and see how the DDR controller responds.
I only have Q ;-) The DDR calibration code _should_ work on DL though.
It at least needs a change to some #ifdefs:
+#if
defined(CONFIG_MX6QDL)||defined(CONFIG_MX6Q)||defined(CONFIG_MX6D)
And then you can test it ;-) Have fun, I'm looking forward to improvements :)
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 46 lines · Marek Vasut
Quoted 45 lines · Eric Nelson
Quoted 44 lines · Marek Vasut
Quoted 1 line · Eric Nelson
Hi Marek,
Hi!
Quoted 38 lines · Marek Vasut
Quoted 22 lines · Eric Nelson
Quoted 1 line
Hi Marek,
Hi Eric,
[..]
Quoted 15 lines
Quoted 8 lines
Quoted 2 lines
This should also have parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output), at least forsysinfo->dsizeWould it be possible for you to send a subsequent patch(set)? I would like to have this code as a working base , since I tested this thoroughly. If I apply all of your changes, it would basically mean almost complete rewrite of the code and that would disallow me bisect possible bugs introduced by these changes.
I think that's a bit of overstatement, but I'm okay sending patches in principle.
I do think that at least the test for calibration failure should be fixed before your patch is applied.
Quoted 6 lines
Quoted 2 lines
This also has the benefit of allowing discussion about each of my points individually instead of in one e-mail thread.
I have to admit I'm a bit lost in this. What do you say we ask Stefano to apply this so people can start fiddling with it. I'd also like to see the patches for MX6SX and your fixes (if you feel like it).
I'm okay with that.
I was hoping to check out the error handling code but unfortunately, the boards I have that are supporting SPL are all either i.MX6DL, i.MX6S, or i.MX6SL.
You should be able to force a failure by setting WALAT and/or RALAT to extreme values and see how the DDR controller responds.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
I only have Q ;-) The DDR calibration code _should_ work on DL though.
It at least needs a change to some #ifdefs:
+#if
defined(CONFIG_MX6QDL)||defined(CONFIG_MX6Q)||defined(CONFIG_MX6D)And then you can test it ;-) Have fun, I'm looking forward to improvements :)
I was able to test the auto calibration a couple of weeks ago on a set of boards. I have a mix of boards with IMX6Q/IMX6DL 16/32/64bit 2/4/8Gb density - a pretty broad range. I did find the that a couple of my boards hung during mx6_dram_cfg if I skip writing anything to the calib registers (I made mx6_dram_cfg able to take a null struct mx6_mmc_calibration and call mmdc_do_write_level_calibration() and mmdc_do_dqs_calibration() automatically if null after config). I haven't had time to troubleshoot yet. Its possible I need some initial value for the calib registers or its possible there is a step in the init that should differ if we have not yet calibrated.
I am all for committing what we have (as its opt-in) and we can continue to improve/test/troubleshoot.
Tim
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Tim, Marek, Fabio,
Quoted 11 lines · Tim Harvey
I was able to test the auto calibration a couple of weeks ago on a set of boards. I have a mix of boards with IMX6Q/IMX6DL 16/32/64bit 2/4/8Gb density - a pretty broad range. I did find the that a couple of my boards hung during
mx6_dram_cfgif I skip writing anything to the calib registers (I mademx6_dram_cfgable to take a null structmx6_mmc_calibrationand callmmdc_do_write_level_calibration()andmmdc_do_dqs_calibration()automatically if null after config). I haven't had time to troubleshoot yet. Its possible I need some initial value for the calib registers or its possible there is a step in the init that should differ if we have not yet calibrated.
Quoted 2 lines
I am all for committing what we have (as its opt-in) and we can continue to improve/test/troubleshoot.
I was thinking about it and I agree that it is better to get it in and goes on with tests, as we can achieve much more testers.
However, I have seen that there is a general attempt for a wait_for_bit() function, posted here:
http://patchwork.ozlabs.org/patch/572085/
This can help to drop the i.MX6 implementation and reuse a general utility.
Best regards,
Stefano Babic
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 20 lines · Stefano Babic
Hi Tim, Marek, Fabio,
Quoted 11 lines · Tim Harvey
I was able to test the auto calibration a couple of weeks ago on a set of boards. I have a mix of boards with IMX6Q/IMX6DL 16/32/64bit 2/4/8Gb density - a pretty broad range. I did find the that a couple of my boards hung during
mx6_dram_cfgif I skip writing anything to the calib registers (I mademx6_dram_cfgable to take a null structmx6_mmc_calibrationand callmmdc_do_write_level_calibration()andmmdc_do_dqs_calibration()automatically if null after config). I haven't had time to troubleshoot yet. Its possible I need some initial value for the calib registers or its possible there is a step in the init that should differ if we have not yet calibrated.Quoted 2 lines
I am all for committing what we have (as its opt-in) and we can continue to improve/test/troubleshoot.
I was thinking about it and I agree that it is better to get it in and goes on with tests, as we can achieve much more testers.
Quoted 6 lines
However, I have seen that there is a general attempt for a
wait_for_bit()function, posted here:http://patchwork.ozlabs.org/patch/572085/This can help to drop the i.MX6 implementation and reuse a general utility.
This patch was not applied yet, but the conversion will be straightforward once it is applied.
So, please apply these patches.
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Marek,
Quoted 24 lines · Marek Vasut
Quoted 23 lines · Stefano Babic
Hi Tim, Marek, Fabio,
Quoted 11 lines · Tim Harvey
I was able to test the auto calibration a couple of weeks ago on a set of boards. I have a mix of boards with IMX6Q/IMX6DL 16/32/64bit 2/4/8Gb density - a pretty broad range. I did find the that a couple of my boards hung during
mx6_dram_cfgif I skip writing anything to the calib registers (I mademx6_dram_cfgable to take a null structmx6_mmc_calibrationand callmmdc_do_write_level_calibration()andmmdc_do_dqs_calibration()automatically if null after config). I haven't had time to troubleshoot yet. Its possible I need some initial value for the calib registers or its possible there is a step in the init that should differ if we have not yet calibrated.Quoted 2 lines
I am all for committing what we have (as its opt-in) and we can continue to improve/test/troubleshoot.
I was thinking about it and I agree that it is better to get it in and goes on with tests, as we can achieve much more testers.
However, I have seen that there is a general attempt for a
wait_for_bit()function, posted here:
Quoted 6 lines
Quoted 3 lines
http://patchwork.ozlabs.org/patch/572085/This can help to drop the i.MX6 implementation and reuse a general utility.
This patch was not applied yet, but the conversion will be straightforward once it is applied.
Yes, it is - then it is better to this before applying.
Quoted 3 lines
So, please apply these patches.
We are not hurry for the release, and I think we have time to make things right at once.
Best regards,
Stefano Babic
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 29 lines · Stefano Babic
Hi Marek,
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Stefano Babic
Hi Tim, Marek, Fabio,
Quoted 11 lines · Tim Harvey
I was able to test the auto calibration a couple of weeks ago on a set of boards. I have a mix of boards with IMX6Q/IMX6DL 16/32/64bit 2/4/8Gb density - a pretty broad range. I did find the that a couple of my boards hung during
mx6_dram_cfgif I skip writing anything to the calib registers (I mademx6_dram_cfgable to take a null structmx6_mmc_calibrationand callmmdc_do_write_level_calibration()andmmdc_do_dqs_calibration()automatically if null after config). I haven't had time to troubleshoot yet. Its possible I need some initial value for the calib registers or its possible there is a step in the init that should differ if we have not yet calibrated.Quoted 2 lines
I am all for committing what we have (as its opt-in) and we can continue to improve/test/troubleshoot.
I was thinking about it and I agree that it is better to get it in and goes on with tests, as we can achieve much more testers.
However, I have seen that there is a general attempt for a
wait_for_bit()function, posted here:http://patchwork.ozlabs.org/patch/572085/
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
This can help to drop the i.MX6 implementation and reuse a general utility.
This patch was not applied yet, but the conversion will be straightforward once it is applied.
Yes, it is - then it is better to this before applying.
It is not clear when the wait_for_bit() will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.
Quoted 4 lines
Quoted 1 line
So, please apply these patches.
We are not hurry for the release, and I think we have time to make things right at once.
The shorter they will be in the tree, the less testing they will have.
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 5 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.
Just wait for a while - if it takes too much, I reconsider to apply this first and factorize wait_for_bit() in a follow-up patch.
Best regards,
Stefano Babic
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 8 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The wait_for_bit() can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 7 lines · Marek Vasut
Quoted 6 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.
Quoted 7 lines
Quoted 2 lines
Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.
Did I miss something or isn't v4 of wait_for_bit good to go?
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160124/a4bf824a/attachment.sig>
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 11 lines · Tom Rini
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.
Quoted 7 lines
Quoted 5 lines
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.Did I miss something or isn't v4 of
wait_for_bitgood to go?
I don't really know if it's good to go, but this patch does not depend on it in any way. A subsequent patch can drop the wait_for_bit() from here, it's the same as the wait_for_bit() in dwc2 and other wait_for_bit() anywhere else, but I don't see a reason why this patch should not be applied now.
If I follow the logic in this thread, it would also be possible to say that this patch should wait until Eric submits the MX6S DDR support for example. We could indefinitelly wait for new and new stuff which might possibly block this.
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 20 lines · Marek Vasut
Quoted 11 lines · Tom Rini
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.Quoted 7 lines
Quoted 5 lines
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.Did I miss something or isn't v4 of
wait_for_bitgood to go?
Quoted 8 lines
I don't really know if it's good to go, but this patch does not depend on it in any way. A subsequent patch can drop the
wait_for_bit()from here, it's the same as thewait_for_bit()in dwc2 and otherwait_for_bit()anywhere else, but I don't see a reason why this patch should not be applied now.If I follow the logic in this thread, it would also be possible to say that this patch should wait until Eric submits the MX6S DDR support for example. We could indefinitelly wait for new and new stuff which might possibly block this.
Why don't you ack/test/review the wait_for_bit series and post a follow-up to this one that uses the common function, which can be squashed into the original and then this gets picked up?
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160124/54df2c0a/attachment.sig>
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 27 lines · Tom Rini
Quoted 26 lines · Marek Vasut
Quoted 11 lines · Tom Rini
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.Quoted 7 lines
Quoted 5 lines
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.Did I miss something or isn't v4 of
wait_for_bitgood to go?I don't really know if it's good to go, but this patch does not depend on it in any way. A subsequent patch can drop the
wait_for_bit()from here, it's the same as thewait_for_bit()in dwc2 and otherwait_for_bit()anywhere else, but I don't see a reason why this patch should not be applied now.
Quoted 8 lines
Quoted 4 lines
If I follow the logic in this thread, it would also be possible to say that this patch should wait until Eric submits the MX6S DDR support for example. We could indefinitelly wait for new and new stuff which might possibly block this.
Why don't you ack/test/review the
wait_for_bitseries and post a follow-up to this one that uses the common function, which can be squashed into the original and then this gets picked up?
Why ? I can send subsequent patch which removes the duplicate wait_for_bit() from this code once the wait_for_bit series is applied. I don't see a problem with that and the wait_for_bit() being so explicitly pulled out from the code is done with that in mind. But that does not block this patch from being applied now. Does it?
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 37 lines · Marek Vasut
Quoted 27 lines · Tom Rini
Quoted 26 lines · Marek Vasut
Quoted 11 lines · Tom Rini
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.Quoted 7 lines
Quoted 5 lines
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.Did I miss something or isn't v4 of
wait_for_bitgood to go?I don't really know if it's good to go, but this patch does not depend on it in any way. A subsequent patch can drop the
wait_for_bit()from here, it's the same as thewait_for_bit()in dwc2 and otherwait_for_bit()anywhere else, but I don't see a reason why this patch should not be applied now.Quoted 8 lines
Quoted 4 lines
If I follow the logic in this thread, it would also be possible to say that this patch should wait until Eric submits the MX6S DDR support for example. We could indefinitelly wait for new and new stuff which might possibly block this.
Why don't you ack/test/review the
wait_for_bitseries and post a follow-up to this one that uses the common function, which can be squashed into the original and then this gets picked up?
Quoted 5 lines
Why ? I can send subsequent patch which removes the duplicate
wait_for_bit()from this code once thewait_for_bitseries is applied. I don't see a problem with that and thewait_for_bit()being so explicitly pulled out from the code is done with that in mind. But that does not block this patch from being applied now. Does it?
If I tell you I'm probably going to have the wait_for_bit stuff applied before the next imx PR is ready... ?
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160124/d729d4cb/attachment.sig>
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 39 lines · Tom Rini
Quoted 38 lines · Marek Vasut
Quoted 36 lines · Tom Rini
Quoted 27 lines · Marek Vasut
Quoted 12 lines · Tom Rini
Quoted 11 lines · Marek Vasut
Quoted 9 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.Quoted 7 lines
Quoted 5 lines
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.Did I miss something or isn't v4 of
wait_for_bitgood to go?I don't really know if it's good to go, but this patch does not depend on it in any way. A subsequent patch can drop the
wait_for_bit()from here, it's the same as thewait_for_bit()in dwc2 and otherwait_for_bit()anywhere else, but I don't see a reason why this patch should not be applied now.Quoted 4 lines
If I follow the logic in this thread, it would also be possible to say that this patch should wait until Eric submits the MX6S DDR support for example. We could indefinitelly wait for new and new stuff which might possibly block this.
Why don't you ack/test/review the
wait_for_bitseries and post a follow-up to this one that uses the common function, which can be squashed into the original and then this gets picked up?
Quoted 8 lines
Quoted 5 lines
Why ? I can send subsequent patch which removes the duplicate
wait_for_bit()from this code once thewait_for_bitseries is applied. I don't see a problem with that and thewait_for_bit()being so explicitly pulled out from the code is done with that in mind. But that does not block this patch from being applied now. Does it?If I tell you I'm probably going to have the
wait_for_bitstuff applied before the next imx PR is ready... ?
Well so what ? I still do not see why these patches should be blocked by the wait_for_bit() stuff. The conversion to wait_for_bit() can well be done after that, so what is the problem ?
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
In reply to Tom RiniQuoted 39 lines · Tom Rini
Quoted 38 lines · Marek Vasut
Quoted 36 lines · Tom Rini
Quoted 27 lines · Marek Vasut
Quoted 12 lines · Tom Rini
Quoted 11 lines · Marek Vasut
Quoted 9 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.Quoted 7 lines
Quoted 5 lines
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.Did I miss something or isn't v4 of
wait_for_bitgood to go?I don't really know if it's good to go, but this patch does not depend on it in any way. A subsequent patch can drop the
wait_for_bit()from here, it's the same as thewait_for_bit()in dwc2 and otherwait_for_bit()anywhere else, but I don't see a reason why this patch should not be applied now.Quoted 4 lines
If I follow the logic in this thread, it would also be possible to say that this patch should wait until Eric submits the MX6S DDR support for example. We could indefinitelly wait for new and new stuff which might possibly block this.
Why don't you ack/test/review the
wait_for_bitseries and post a follow-up to this one that uses the common function, which can be squashed into the original and then this gets picked up?
Quoted 8 lines
Quoted 5 lines
Why ? I can send subsequent patch which removes the duplicate
wait_for_bit()from this code once thewait_for_bitseries is applied. I don't see a problem with that and thewait_for_bit()being so explicitly pulled out from the code is done with that in mind. But that does not block this patch from being applied now. Does it?If I tell you I'm probably going to have the
wait_for_bitstuff applied before the next imx PR is ready... ?
So, why is this patchset not applied yet ... ?
Best regards,
Marek Vasut
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 46 lines · Marek Vasut
Quoted 45 lines · Tom Rini
Quoted 43 lines · Marek Vasut
Quoted 28 lines · Tom Rini
Quoted 27 lines · Marek Vasut
Quoted 12 lines · Tom Rini
Quoted 11 lines · Marek Vasut
Quoted 9 lines · Stefano Babic
Quoted 4 lines · Marek Vasut
It is not clear when the
wait_for_bit()will be applied, I am certain there will be another round for so. I do not want to wait for it and I don't see a reason why those patches should block this if the conversion can be done afterward.Just wait for a while - if it takes too much, I reconsider to apply this first and factorize
wait_for_bit()in a follow-up patch.Quoted 7 lines
Quoted 5 lines
I have waited for over a month and I fail to see a reason why patches which will be applied at uncertain point in the future shall block this patchset. The
wait_for_bit()can be removed by a subsequent patch, it is already pulled out explicitly in the code, so I don't see a problem with applying this.Did I miss something or isn't v4 of
wait_for_bitgood to go?I don't really know if it's good to go, but this patch does not depend on it in any way. A subsequent patch can drop the
wait_for_bit()from here, it's the same as thewait_for_bit()in dwc2 and otherwait_for_bit()anywhere else, but I don't see a reason why this patch should not be applied now.Quoted 8 lines
Quoted 4 lines
If I follow the logic in this thread, it would also be possible to say that this patch should wait until Eric submits the MX6S DDR support for example. We could indefinitelly wait for new and new stuff which might possibly block this.
Why don't you ack/test/review the
wait_for_bitseries and post a follow-up to this one that uses the common function, which can be squashed into the original and then this gets picked up?Why ? I can send subsequent patch which removes the duplicate
wait_for_bit()from this code once thewait_for_bitseries is applied. I don't see a problem with that and thewait_for_bit()being so explicitly pulled out from the code is done with that in mind. But that does not block this patch from being applied now. Does it?
Quoted 5 lines
Quoted 2 lines
If I tell you I'm probably going to have the
wait_for_bitstuff applied before the next imx PR is ready... ?So, why is this patchset not applied yet ... ?
Stefano,
I would also like to see this applied. I have some time in the coming weeks to do some extensive testing of it over a variety of memory chips and configurations.
Regards,
Tim
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Tim,
Quoted 6 lines · Tim Harvey
Quoted 2 lines
So, why is this patchset not applied yet ... ?
Stefano,
I would also like to see this applied.
Did I not ? Sorry if it is not, but I was convinced I applied Marek's.
In log I see:
commit d339f16911c790196f5aaea3682819b9c03633bb
Author: Marek Vasut <[email protected]> Date: Wed Dec 16 15:40:06 2015 +0100
arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
What have I missed ?
Quoted 3 lines
I have some time in the coming
weeks to do some extensive testing of it over a variety of memory chips and configurations.
Nice - let us know your progress.
Best regards,
Stefano
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Quoted 15 lines · Stefano Babic
Hi Tim,
Quoted 6 lines · Tim Harvey
Quoted 2 lines
So, why is this patchset not applied yet ... ?
Stefano,
I would also like to see this applied.
Did I not ? Sorry if it is not, but I was convinced I applied Marek's.
In log I see:
Quoted 7 lines
commit
d339f16911c790196f5aaea3682819b9c03633bbAuthor: Marek Vasut <[email protected]> Date: Wed Dec 16 15:40:06 2015 +0100arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
What have I missed ?
Stefano,
Sorry - my mistake. Yes it's in there.
Thanks!
Tim
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
In reply to Eric NelsonQuoted 2 lines · Eric Nelson
Hi Marek,
<snip>
Quoted 3 lines
I'd recommend passing parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output) to this routine.
I don't know that this would make sense. The mx6_ddr_sysinfo shouldn't be affected by calibration and the mx6_mmdc_calibration structure would not be used if you are using auto-calibration.
In fact, I recommend an additional patch that will call the auto-calibration functions in mx6_ddr3_cfg if it is passed a NULL for mx6_ddr_calibration.
Tim
PATCH 1/2 arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL
Hi Tim,
Quoted 8 lines · Tim Harvey
Quoted 2 lines · Eric Nelson
Hi Marek,
<snip>
Quoted 3 lines
I'd recommend passing parameters of
mx6_ddr_sysinfo(input) andmx6_mmdc_calibration(output) to this routine.
Quoted 8 lines
I don't know that this would make sense. The
mx6_ddr_sysinfoshouldn't be affected by calibration and themx6_mmdc_calibrationstructure would not be used if you are using auto-calibration.In fact, I recommend an additional patch that will call the auto-calibration functions in
mx6_ddr3_cfgif it is passed a NULL formx6_ddr_calibration.
Using sysinfo allows simpler determination of things like bus size and rtt_nom.
This should be based on a board's configuration instead of inferring the parameters by reading and interpreting registers that must have been set before the calls to do_write_level_calibration().
The DDR stress tool does this (read from registers) because it's distributed as a binary and JTAG is used to configure things before running.
The calibration parameter would be used in a subsequent patch to add a "calibration" pseudo-board.
Regards,
Eric