[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[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 <marex@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
---
 arch/arm/cpu/armv7/mx6/ddr.c            | 559 ++++++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-mx6/mx6-ddr.h |   5 +
 2 files changed, 564 insertions(+)

diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
index 6b039e4..194411f 100644
--- a/arch/arm/cpu/armv7/mx6/ddr.c
+++ b/arch/arm/cpu/armv7/mx6/ddr.c
@@ -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,
diff --git a/arch/arm/include/asm/arch-mx6/mx6-ddr.h b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
index 68d9bda..12c30d2 100644
--- a/arch/arm/include/asm/arch-mx6/mx6-ddr.h
+++ b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
@@ -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 *,
-- 
2.1.4

[U-Boot] [PATCH 2/2] arm: imx6: Enable DDR calibration on Novena

[Marek Vasut]

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.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 board/kosagi/novena/novena_spl.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/board/kosagi/novena/novena_spl.c b/board/kosagi/novena/novena_spl.c
index eb46265..f779bb4 100644
--- a/board/kosagi/novena/novena_spl.c
+++ b/board/kosagi/novena/novena_spl.c
@@ -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);
 
-- 
2.1.4

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Marek,

On 12/16/2015 07:40 AM, Marek Vasut wrote:
> 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.

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  arch/arm/cpu/armv7/mx6/ddr.c            | 559 ++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/arch-mx6/mx6-ddr.h |   5 +
>  2 files changed, 564 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
> index 6b039e4..194411f 100644
> --- a/arch/arm/cpu/armv7/mx6/ddr.c
> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
> @@ -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.

> +#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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Wednesday, December 16, 2015 at 04:00:38 PM, Eric Nelson wrote:
> Hi Marek,
> 
> On 12/16/2015 07:40 AM, Marek Vasut wrote:
> > 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.

Excellent, we can add this as an optional feature or something which can
be triggered by command, since the "full" calibration takes some time.

> To do that, I put together a pseudo-board with Kconfig options for
> the serial console, memory bus width, and such.

Or maybe this can be built as a mutated SPL ?

> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> > 
> >  arch/arm/cpu/armv7/mx6/ddr.c            | 559
> >  ++++++++++++++++++++++++++++++++
> >  arch/arm/include/asm/arch-mx6/mx6-ddr.h |   5 +
> >  2 files changed, 564 insertions(+)
> > 
> > diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
> > index 6b039e4..194411f 100644
> > --- a/arch/arm/cpu/armv7/mx6/ddr.c
> > +++ b/arch/arm/cpu/armv7/mx6/ddr.c
> > @@ -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.

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!

> > +#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

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Marek,

On 12/16/2015 08:33 AM, Marek Vasut wrote:
> On Wednesday, December 16, 2015 at 04:00:38 PM, Eric Nelson wrote:
>> Hi Marek,
>>
>> On 12/16/2015 07:40 AM, Marek Vasut wrote:
>>> 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.
> 
> Excellent, we can add this as an optional feature or something which can
> be triggered by command, since the "full" calibration takes some time.
> 
>> To do that, I put together a pseudo-board with Kconfig options 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.

>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Stefano Babic <sbabic@denx.de>
>>> ---
>>>
>>>  arch/arm/cpu/armv7/mx6/ddr.c            | 559
>>>  ++++++++++++++++++++++++++++++++
>>>  arch/arm/include/asm/arch-mx6/mx6-ddr.h |   5 +
>>>  2 files changed, 564 insertions(+)
>>>
>>> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
>>> index 6b039e4..194411f 100644
>>> --- a/arch/arm/cpu/armv7/mx6/ddr.c
>>> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
>>> @@ -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.
> 
> 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.

> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Wednesday, December 16, 2015 at 05:28:48 PM, Eric Nelson wrote:
> Hi Marek,

Hi!

> On 12/16/2015 08:33 AM, Marek Vasut wrote:
> > On Wednesday, December 16, 2015 at 04:00:38 PM, Eric Nelson wrote:
> >> Hi Marek,
> >> 
> >> On 12/16/2015 07:40 AM, Marek Vasut wrote:
> >>> 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.
> > 
> > Excellent, we can add this as an optional feature or something which can
> > be triggered by command, since the "full" calibration takes some time.
> > 
> >> To do that, I put together a pseudo-board with Kconfig options 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.

Gotcha! That'd be great thing to have %^)

> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Cc: Stefano Babic <sbabic@denx.de>
> >>> ---
> >>> 
> >>>  arch/arm/cpu/armv7/mx6/ddr.c            | 559
> >>>  ++++++++++++++++++++++++++++++++
> >>>  arch/arm/include/asm/arch-mx6/mx6-ddr.h |   5 +
> >>>  2 files changed, 564 insertions(+)
> >>> 
> >>> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c
> >>> b/arch/arm/cpu/armv7/mx6/ddr.c index 6b039e4..194411f 100644
> >>> --- a/arch/arm/cpu/armv7/mx6/ddr.c
> >>> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
> >>> @@ -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.
> > 
> > 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.
> 
> > 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 ;-)

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Marek,

On 12/16/2015 09:50 AM, Marek Vasut wrote:
> On Wednesday, December 16, 2015 at 05:28:48 PM, Eric Nelson wrote:
>> On 12/16/2015 08:33 AM, Marek Vasut wrote:
>>> On Wednesday, December 16, 2015 at 04:00:38 PM, Eric Nelson wrote:
...

>>> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Wednesday, December 16, 2015 at 06:07:02 PM, Eric Nelson wrote:
> Hi Marek,

Hi!

> On 12/16/2015 09:50 AM, Marek Vasut wrote:
> > On Wednesday, December 16, 2015 at 05:28:48 PM, Eric Nelson wrote:
> >> On 12/16/2015 08:33 AM, Marek Vasut wrote:
> >>> On Wednesday, December 16, 2015 at 04:00:38 PM, Eric Nelson wrote:
> ...
> 
> >>> 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.

Excellent, thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tim Harvey]

On Wed, Dec 16, 2015 at 6:40 AM, Marek Vasut <marex@denx.de> wrote:
> 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 <marex@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>

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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tim Harvey]

On Wed, Dec 16, 2015 at 7:00 AM, Eric Nelson <eric@nelint.com> wrote:
> Hi Marek,
>
> On 12/16/2015 07:40 AM, Marek Vasut wrote:
>> 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.

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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Thursday, December 17, 2015 at 04:36:20 PM, Tim Harvey wrote:
> On Wed, Dec 16, 2015 at 6:40 AM, Marek Vasut <marex@denx.de> wrote:
> > 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 <marex@denx.de>
> > Cc: Stefano Babic <sbabic@denx.de>
> 
> 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.

> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Tim,

On 12/17/2015 08:39 AM, Tim Harvey wrote:
> On Wed, Dec 16, 2015 at 7:00 AM, Eric Nelson <eric@nelint.com> wrote:
>> Hi Marek,
>>
>> On 12/16/2015 07:40 AM, Marek Vasut wrote:
>>> 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.
> 
> 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).

> 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.

> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tim Harvey]

On Thu, Dec 17, 2015 at 7:40 AM, Marek Vasut <marex@denx.de> wrote:
> On Thursday, December 17, 2015 at 04:36:20 PM, Tim Harvey wrote:
>> On Wed, Dec 16, 2015 at 6:40 AM, Marek Vasut <marex@denx.de> wrote:
>> > 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 <marex@denx.de>
>> > Cc: Stefano Babic <sbabic@denx.de>
>>
>> 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.

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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Thursday, December 17, 2015 at 05:15:46 PM, Tim Harvey wrote:
> On Thu, Dec 17, 2015 at 7:40 AM, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, December 17, 2015 at 04:36:20 PM, Tim Harvey wrote:
> >> On Wed, Dec 16, 2015 at 6:40 AM, Marek Vasut <marex@denx.de> wrote:
> >> > 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 <marex@denx.de>
> >> > Cc: Stefano Babic <sbabic@denx.de>
> >> 
> >> 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.
> 
> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Nikolay Dimitrov]

Hi Marek,

On 12/16/2015 04:40 PM, Marek Vasut wrote:
> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Thursday, December 17, 2015 at 10:32:03 PM, Nikolay Dimitrov wrote:
> Hi Marek,
> 
> On 12/16/2015 04:40 PM, Marek Vasut wrote:
> > 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Marek,

On 12/16/2015 07:40 AM, Marek Vasut wrote:
> 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

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  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:

http://git.pengutronix.de/?p=barebox.git;a=blob;f=arch/arm/mach-imx/imx6-mmdc.c;h=146df573e4eeed7132e93899b4b539e842bb6ba2;hb=HEAD

> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
> index 6b039e4..194411f 100644
> --- a/arch/arm/cpu/armv7/mx6/ddr.c
> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
> @@ -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.

> +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);
> +}
> +

See comments below about chip-selects during calibration.

> +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().

> +	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;
> +

Though this matches the app note and other implementations, I think
this would be easier to understand by shifting first and anding with
0x7ff.

> +	dg_tmp_val = ((readl(reg_st0) & 0x07ff0000) >> 16) - 0xc0;

Especially since these calculations operate on the shifted values:

> +	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);
> +}
> +

I'd recommend passing parameters of mx6_ddr_sysinfo (input) and
mx6_mmdc_calibration (output) to this routine.

> +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;

> +	u32 ddr_mr1 = 0x4;
> +

I'm not sure how this is useful (trying to proceed if things fail).

Have you seen this triggered?

> +	/*
> +	 * 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.

> +	/* 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.

> +	esdmisc_val = readl(&mmdc0->mdref);
> +	writel(0x0000C000, &mmdc0->mdref);
> +	zq_val = readl(&mmdc0->mpzqhwctrl);

clrbits_le32?

> +	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.

> +	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.

> +	/*
> +	 * 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:

> +	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?

> +	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:

> +	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.

> +	/* check to see if cal failed */
> +	if ((readl(&mmdc0->mpwldectrl0) == 0x001F001F) &&
> +	    (readl(&mmdc0->mpwldectrl1) == 0x001F001F) &&

Ditto (sysinfo->dsize == 2):

> +	    (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);
> +

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);

> +	/* 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.

> +	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.

> +	/* 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/

> +	/* 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

> +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:

> +	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);

> +	/* 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:

> +	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.

> +	esdmisc_val = readl(&mmdc0->mdmisc);
> +
> +	setbits_le32(&mmdc0->mdmisc,

(3 << 16) | (7 << 6)

> +		     (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.

> +	/* 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):

> +	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 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.

> +	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.

> +	/*
> +	 * Check to see which chip selects are now enabled for
> +	 * the remainder of the calibration.
> +	 */

And this should go away:

> +	cs0_enable = readl(&mmdc0->mdctl) & 0x80000000;
> +	cs1_enable = readl(&mmdc0->mdctl) & 0x40000000;
> +

bus_size is in sysinfo.

> +	/* 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:

> +	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 (sysinfo->dsize == 0x2)

> +	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);
> +
I think you need to poll for bit 30 clear here.

> +	/* Set bit 28 to start automatic read DQS gating calibration */

1 << 28 ?

> +	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

> +	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);
> +

s/Adopt/Automatic/

> +	/* Enable Adopt power down timer */
> +	clrbits_le32(&mmdc0->mapsr, 0x1);
> +

sysinfo->ralat, sysinfo->walat

> +	/* 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));

> +	/* 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);
> +

Again, I think this test is superfluous since we don't
support a routing of CS1 by itself.

> +	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);

> +	/* 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));
> +

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);
	}

> +	/*
> +	 * 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);
> +

Finally, I'm not sure what the current caller(s) will do if errors
are returned from either of these two routines...

> +	return errors;
> +}
> +#endif
> +

[U-Boot] [PATCH 2/2] arm: imx6: Enable DDR calibration on Novena

[Eric Nelson]

Hi Marek,

On 12/16/2015 07:40 AM, Marek Vasut wrote:
> Enable the DDR calibration functionality on Novena to deal with the
> memory SoDIMM on this board.

Shouldn't this be in two patches?

> Moreover, tweak the initial DDR DRAM parameters so the
> calibration works properly.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  board/kosagi/novena/novena_spl.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/board/kosagi/novena/novena_spl.c
b/board/kosagi/novena/novena_spl.c
> index eb46265..f779bb4 100644
> --- a/board/kosagi/novena/novena_spl.c
> +++ b/board/kosagi/novena/novena_spl.c
> @@ -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);

Shouldn't the return values be tested?

> +	mmdc_do_write_level_calibration();
> +	mmdc_do_dqs_calibration();
> +
>  	/* Clear the BSS. */
>  	memset(__bss_start, 0, __bss_end - __bss_start);
>
>

[U-Boot] [PATCH 2/2] arm: imx6: Enable DDR calibration on Novena

[Marek Vasut]

On Sunday, December 20, 2015 at 08:33:58 PM, Eric Nelson wrote:
> Hi Marek,
> 
> On 12/16/2015 07:40 AM, Marek Vasut wrote:
> > 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.

[...]

> >  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);
> 
> 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.

> > +	mmdc_do_write_level_calibration();
> > +	mmdc_do_dqs_calibration();
> > +
> > 
> >  	/* Clear the BSS. */
> >  	memset(__bss_start, 0, __bss_end - __bss_start);

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Sunday, December 20, 2015 at 08:31:46 PM, Eric Nelson wrote:
> Hi Marek,
> 
> On 12/16/2015 07:40 AM, Marek Vasut wrote:
> > 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

> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> > 
> >  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:
> 
> 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 ;-)

> > diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
> > index 6b039e4..194411f 100644
> > --- a/arch/arm/cpu/armv7/mx6/ddr.c
> > +++ b/arch/arm/cpu/armv7/mx6/ddr.c
> > @@ -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.

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.

> > +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);
> > +}
> > +
> 
> See comments below about chip-selects during calibration.
> 
> > +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

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.

> , and since
> waiting for it is done in multiple places, perhaps wait_for_con_ack().

Implementing a function for the sake of just wrapping another function
seems wasteful to me.

> > +	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;
> > +
> 
> 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.

> > +	dg_tmp_val = ((readl(reg_st0) & 0x07ff0000) >> 16) - 0xc0;
> 
> Especially since these calculations operate on the shifted values:
> > +	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);
> > +}
> > +
> 
> I'd recommend passing parameters of mx6_ddr_sysinfo (input) and
> mx6_mmdc_calibration (output) to this routine.

Why exactly ?

> > +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;
> 
> > +	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 ?

> > +	/*
> > +	 * 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.

OK

> > +	/* 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.

Can you elaborate some more on why is it a bad idea?

> > +	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.

> > +	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.
> 
> > +	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.

OK

> > +	/*
> > +	 * 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 ?

> > +	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.

> > +	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:

I believe mmdc1 will contain 0x0 << 8 if you use 32bit bus, right ?

> > +	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.

> > +	/* check to see if cal failed */
> > +	if ((readl(&mmdc0->mpwldectrl0) == 0x001F001F) &&
> > +	    (readl(&mmdc0->mpwldectrl1) == 0x001F001F) &&
> 
> Ditto (sysinfo->dsize == 2):
> > +	    (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);
> > +
> 
> 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);
> 
> > +	/* 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.
> 
> > +	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.

You do not need those values anymore, since they are already loaded into the 
hardware ... or why would you want them to be adjusted ?

> > +	/* 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/
> 
> > +	/* 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

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.

[...]

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tim Harvey]

On Sun, Dec 20, 2015 at 11:31 AM, Eric Nelson <eric@nelint.com> wrote:
> Hi Marek,
>
<snip>
> I'd recommend passing parameters of mx6_ddr_sysinfo (input) and
> mx6_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

[U-Boot] [PATCH 2/2] arm: imx6: Enable DDR calibration on Novena

[Nikolay Dimitrov]

Hi Marek,

On 12/22/2015 03:26 AM, Marek Vasut wrote:
> On Sunday, December 20, 2015 at 08:33:58 PM, Eric Nelson wrote:
>> Hi Marek,
>>
>> On 12/16/2015 07:40 AM, Marek Vasut wrote:
>>> 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.
>
> [...]
>
>>>   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);
>>
>> 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?

>
> 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.
>
>>> +	mmdc_do_write_level_calibration();
>>> +	mmdc_do_dqs_calibration();
>>> +
>>>
>>>   	/* 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Tim,

On 12/21/2015 09:13 PM, Tim Harvey wrote:
> On Sun, Dec 20, 2015 at 11:31 AM, Eric Nelson <eric@nelint.com> wrote:
>> Hi Marek,
>>
> <snip>
>> I'd recommend passing parameters of mx6_ddr_sysinfo (input) and
>> mx6_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.
> 

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

[U-Boot] [PATCH 2/2] arm: imx6: Enable DDR calibration on Novena

[Marek Vasut]

On Tuesday, December 22, 2015 at 09:30:00 AM, Nikolay Dimitrov wrote:
> Hi Marek,
> 
> On 12/22/2015 03:26 AM, Marek Vasut wrote:
> > On Sunday, December 20, 2015 at 08:33:58 PM, Eric Nelson wrote:
> >> Hi Marek,
> >> 
> >> On 12/16/2015 07:40 AM, Marek Vasut wrote:
> >>> 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.
> > 
> > [...]
> > 
> >>>   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);
> >> 
> >> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Marek,

On 12/21/2015 06:52 PM, Marek Vasut wrote:
> On Sunday, December 20, 2015 at 08:31:46 PM, Eric Nelson wrote:
>> On 12/16/2015 07:40 AM, Marek Vasut wrote:
...
>>> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
>>> index 6b039e4..194411f 100644
>>> --- a/arch/arm/cpu/armv7/mx6/ddr.c
>>> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
>>> @@ -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.
> 
> 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.

>>> +static int wait_for_bit(void *reg, const uint32_t mask, bool set)
>>> +{

...

>>
>> See comments below about chip-selects during calibration.
>>
>>> +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
> 
> That'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 ;).

> I am definitelly not gonna rewrite the the i.MX6 MMDC bits
> from the datasheet by hand.
> 

How about just one constant?

>> , and since
>> waiting for it is done in multiple places, perhaps wait_for_con_ack().
> 
> Implementing a function for the sake of just wrapping another function
> seems wasteful to me.
> 

I was thinking macro, but okay.

>>> +	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;
>>> +
>>
>> 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.
> 

Okay. My hope is that a future app note will refer to the new code...

>>> +	dg_tmp_val = ((readl(reg_st0) & 0x07ff0000) >> 16) - 0xc0;
>>
>> Especially since these calculations operate on the shifted values:
>>> +	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);
>>> +}
>>> +
>>
>> I'd recommend passing parameters of mx6_ddr_sysinfo (input) and
>> mx6_mmdc_calibration (output) to this routine.
> 
> Why exactly ?
> 

I sent a note about this in response to Tim's comment.

>>> +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;
>>
>>> +	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.

>>> +	/*
>>> +	 * 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);
>>> +
...

>> 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.

>>> +	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.
> 

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.

>>> +	writel(zq_val & ~0x3, &mmdc0->mpzqhwctrl);
>>> +
>>> +	/* 3. increase walat and ralat to maximum */
>>

...

>>> +	/*
>>> +	 * 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 ?
> 

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.

>>> +	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.
> 

This was a nit-pick: why not simply 1?

>>> +	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:
> 
> 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)

>>> +	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.
> 

I think this test is wrong, and we should really find the
proper test for failure.

>>> +	/* check to see if cal failed */
>>> +	if ((readl(&mmdc0->mpwldectrl0) == 0x001F001F) &&
>>> +	    (readl(&mmdc0->mpwldectrl1) == 0x001F001F) &&
>>
>> Ditto (sysinfo->dsize == 2):
>>> +	    (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);
>>> +
>>
>> 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);
>>
>>> +	/* 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.
>>

Sorry: I mean setbits_le32:

	setbits_le32(&mmdc0->mpzqhwctrl, 3);

>>> +	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.
> 
> 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?

>>> +	/* 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/
>>
>>> +	/* 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
> 
> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Tuesday, December 22, 2015 at 04:37:12 PM, Eric Nelson wrote:
> Hi Marek,

Hi Eric,

[..]

> >> This should also have parameters of mx6_ddr_sysinfo (input) and
> >> mx6_mmdc_calibration (output), at least for sysinfo->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.
> 
> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

Hi Marek,

On 01/13/2016 07:10 PM, Marek Vasut wrote:
> On Tuesday, December 22, 2015 at 04:37:12 PM, Eric Nelson wrote:
>> Hi Marek,
> 
> Hi Eric,
> 
> [..]
> 
>>>> This should also have parameters of mx6_ddr_sysinfo (input) and
>>>> mx6_mmdc_calibration (output), at least for sysinfo->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.
>>
>> 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).
> 

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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Thursday, January 14, 2016 at 03:37:09 AM, Eric Nelson wrote:
> Hi Marek,

Hi!

> On 01/13/2016 07:10 PM, Marek Vasut wrote:
> > On Tuesday, December 22, 2015 at 04:37:12 PM, Eric Nelson wrote:
> >> Hi Marek,
> > 
> > Hi Eric,
> > 
> > [..]
> > 
> >>>> This should also have parameters of mx6_ddr_sysinfo (input) and
> >>>> mx6_mmdc_calibration (output), at least for sysinfo->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.
> >> 
> >> 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).
> 
> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Eric Nelson]

On 01/13/2016 07:50 PM, Marek Vasut wrote:
> On Thursday, January 14, 2016 at 03:37:09 AM, Eric Nelson wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 01/13/2016 07:10 PM, Marek Vasut wrote:
>>> On Tuesday, December 22, 2015 at 04:37:12 PM, Eric Nelson wrote:
>>>> Hi Marek,
>>>
>>> Hi Eric,
>>>
>>> [..]
>>>
>>>>>> This should also have parameters of mx6_ddr_sysinfo (input) and
>>>>>> mx6_mmdc_calibration (output), at least for sysinfo->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.
>>>>
>>>> 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).
>>
>> 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.
> 

It at least needs a change to some #ifdefs:

+#if defined(CONFIG_MX6QDL) || defined(CONFIG_MX6Q) || defined(CONFIG_MX6D)

Regards,


Eric

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Thursday, January 14, 2016 at 03:52:27 AM, Eric Nelson wrote:
> On 01/13/2016 07:50 PM, Marek Vasut wrote:
> > On Thursday, January 14, 2016 at 03:37:09 AM, Eric Nelson wrote:
> >> Hi Marek,
> > 
> > Hi!
> > 
> >> On 01/13/2016 07:10 PM, Marek Vasut wrote:
> >>> On Tuesday, December 22, 2015 at 04:37:12 PM, Eric Nelson wrote:
> >>>> Hi Marek,
> >>> 
> >>> Hi Eric,
> >>> 
> >>> [..]
> >>> 
> >>>>>> This should also have parameters of mx6_ddr_sysinfo (input) and
> >>>>>> mx6_mmdc_calibration (output), at least for sysinfo->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.
> >>>> 
> >>>> 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).
> >> 
> >> 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.
> 
> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tim Harvey]

On Wed, Jan 13, 2016 at 7:06 PM, Marek Vasut <marex@denx.de> wrote:
> On Thursday, January 14, 2016 at 03:52:27 AM, Eric Nelson wrote:
>> On 01/13/2016 07:50 PM, Marek Vasut wrote:
>> > On Thursday, January 14, 2016 at 03:37:09 AM, Eric Nelson wrote:
>> >> Hi Marek,
>> >
>> > Hi!
>> >
>> >> On 01/13/2016 07:10 PM, Marek Vasut wrote:
>> >>> On Tuesday, December 22, 2015 at 04:37:12 PM, Eric Nelson wrote:
>> >>>> Hi Marek,
>> >>>
>> >>> Hi Eric,
>> >>>
>> >>> [..]
>> >>>
>> >>>>>> This should also have parameters of mx6_ddr_sysinfo (input) and
>> >>>>>> mx6_mmdc_calibration (output), at least for sysinfo->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.
>> >>>>
>> >>>> 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).
>> >>
>> >> 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.
>>
>> 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Stefano Babic]

Hi Tim, Marek, Fabio,

On 14/01/2016 15:25, Tim Harvey wrote:

> 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.

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

-- 
=====================================================================
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
=====================================================================

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Sunday, January 24, 2016 at 11:47:56 AM, Stefano Babic wrote:
> Hi Tim, Marek, Fabio,
> 
> On 14/01/2016 15:25, Tim Harvey wrote:
> > 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.
> 
> 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.

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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Stefano Babic]

Hi Marek,

On 24/01/2016 17:01, Marek Vasut wrote:
> On Sunday, January 24, 2016 at 11:47:56 AM, Stefano Babic wrote:
>> Hi Tim, Marek, Fabio,
>>
>> On 14/01/2016 15:25, Tim Harvey wrote:
>>> 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.
>>
>> 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.
> 
> 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.

> 
> 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

-- 
=====================================================================
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
=====================================================================

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Sunday, January 24, 2016 at 06:03:52 PM, Stefano Babic wrote:
> Hi Marek,
> 
> On 24/01/2016 17:01, Marek Vasut wrote:
> > On Sunday, January 24, 2016 at 11:47:56 AM, Stefano Babic wrote:
> >> Hi Tim, Marek, Fabio,
> >> 
> >> On 14/01/2016 15:25, Tim Harvey wrote:
> >>> 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.
> >> 
> >> 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.
> > 
> > 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.

> > 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Stefano Babic]

On 24/01/2016 18:11, Marek Vasut wrote:

> 
> 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


-- 
=====================================================================
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
=====================================================================

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> On 24/01/2016 18:11, Marek Vasut wrote:
> > 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tom Rini]

On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
> On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> > On 24/01/2016 18:11, Marek Vasut wrote:
> > > 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.

Did I miss something or isn't v4 of wait_for_bit good to go?

-- 
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>

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Sunday, January 24, 2016 at 08:33:57 PM, Tom Rini wrote:
> On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
> > On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> > > On 24/01/2016 18:11, Marek Vasut wrote:
> > > > 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.
> 
> Did I miss something or isn't v4 of wait_for_bit good 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tom Rini]

On Sun, Jan 24, 2016 at 11:07:30PM +0100, Marek Vasut wrote:
> On Sunday, January 24, 2016 at 08:33:57 PM, Tom Rini wrote:
> > On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
> > > On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> > > > On 24/01/2016 18:11, Marek Vasut wrote:
> > > > > 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.
> > 
> > Did I miss something or isn't v4 of wait_for_bit good 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.

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?

-- 
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>

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Sunday, January 24, 2016 at 11:21:51 PM, Tom Rini wrote:
> On Sun, Jan 24, 2016 at 11:07:30PM +0100, Marek Vasut wrote:
> > On Sunday, January 24, 2016 at 08:33:57 PM, Tom Rini wrote:
> > > On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
> > > > On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> > > > > On 24/01/2016 18:11, Marek Vasut wrote:
> > > > > > 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.
> > > 
> > > Did I miss something or isn't v4 of wait_for_bit good 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.
> 
> 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?

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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tom Rini]

On Mon, Jan 25, 2016 at 12:00:05AM +0100, Marek Vasut wrote:
> On Sunday, January 24, 2016 at 11:21:51 PM, Tom Rini wrote:
> > On Sun, Jan 24, 2016 at 11:07:30PM +0100, Marek Vasut wrote:
> > > On Sunday, January 24, 2016 at 08:33:57 PM, Tom Rini wrote:
> > > > On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
> > > > > On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> > > > > > On 24/01/2016 18:11, Marek Vasut wrote:
> > > > > > > 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.
> > > > 
> > > > Did I miss something or isn't v4 of wait_for_bit good 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.
> > 
> > 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?
> 
> 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?

If I tell you I'm probably going to have the wait_for_bit stuff applied
before the next imx PR is ready... ?

-- 
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>

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Monday, January 25, 2016 at 12:30:00 AM, Tom Rini wrote:
> On Mon, Jan 25, 2016 at 12:00:05AM +0100, Marek Vasut wrote:
> > On Sunday, January 24, 2016 at 11:21:51 PM, Tom Rini wrote:
> > > On Sun, Jan 24, 2016 at 11:07:30PM +0100, Marek Vasut wrote:
> > > > On Sunday, January 24, 2016 at 08:33:57 PM, Tom Rini wrote:
> > > > > On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
> > > > > > On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> > > > > > > On 24/01/2016 18:11, Marek Vasut wrote:
> > > > > > > > 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.
> > > > > 
> > > > > Did I miss something or isn't v4 of wait_for_bit good 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.
> > > 
> > > 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?
> > 
> > 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?
> 
> If I tell you I'm probably going to have the wait_for_bit stuff 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Marek Vasut]

On Monday, January 25, 2016 at 12:30:00 AM, Tom Rini wrote:
> On Mon, Jan 25, 2016 at 12:00:05AM +0100, Marek Vasut wrote:
> > On Sunday, January 24, 2016 at 11:21:51 PM, Tom Rini wrote:
> > > On Sun, Jan 24, 2016 at 11:07:30PM +0100, Marek Vasut wrote:
> > > > On Sunday, January 24, 2016 at 08:33:57 PM, Tom Rini wrote:
> > > > > On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
> > > > > > On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
> > > > > > > On 24/01/2016 18:11, Marek Vasut wrote:
> > > > > > > > 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.
> > > > > 
> > > > > Did I miss something or isn't v4 of wait_for_bit good 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.
> > > 
> > > 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?
> > 
> > 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?
> 
> If I tell you I'm probably going to have the wait_for_bit stuff applied
> before the next imx PR is ready... ?

So, why is this patchset not applied yet ... ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tim Harvey]

On Sun, Jan 31, 2016 at 9:25 AM, Marek Vasut <marex@denx.de> wrote:
> On Monday, January 25, 2016 at 12:30:00 AM, Tom Rini wrote:
>> On Mon, Jan 25, 2016 at 12:00:05AM +0100, Marek Vasut wrote:
>> > On Sunday, January 24, 2016 at 11:21:51 PM, Tom Rini wrote:
>> > > On Sun, Jan 24, 2016 at 11:07:30PM +0100, Marek Vasut wrote:
>> > > > On Sunday, January 24, 2016 at 08:33:57 PM, Tom Rini wrote:
>> > > > > On Sun, Jan 24, 2016 at 06:22:10PM +0100, Marek Vasut wrote:
>> > > > > > On Sunday, January 24, 2016 at 06:18:23 PM, Stefano Babic wrote:
>> > > > > > > On 24/01/2016 18:11, Marek Vasut wrote:
>> > > > > > > > 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.
>> > > > >
>> > > > > Did I miss something or isn't v4 of wait_for_bit good 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.
>> > >
>> > > 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?
>> >
>> > 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?
>>
>> If I tell you I'm probably going to have the wait_for_bit stuff 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

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Stefano Babic]

Hi Tim,

On 02/06/2016 15:20, Tim Harvey wrote:

>> 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 <marex@denx.de>
Date:   Wed Dec 16 15:40:06 2015 +0100

    arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

What have I missed ?

> 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

-- 
=====================================================================
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
=====================================================================

[U-Boot] [PATCH 1/2] arm: imx6: Add DDR3 calibration code for MX6 Q/D/DL

[Tim Harvey]

On Thu, Jun 2, 2016 at 7:23 AM, Stefano Babic <sbabic@denx.de> wrote:
> Hi Tim,
>
> On 02/06/2016 15:20, Tim Harvey wrote:
>
>>> 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 <marex@denx.de>
> Date:   Wed Dec 16 15:40:06 2015 +0100
>
>     arm: 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