[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 at denx.de>
> Cc: Stefano Babic <sbabic at 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
> +