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

[Marek Vasut <marex@denx.de>]

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.

[...]