[PATCH RESEND 1/3] i2c: mxs: distinguish i.MX23 and i.MX28 based I2C controller

[PATCH RESEND 1/3] i2c: mxs: distinguish i.MX23 and i.MX28 based I2C controller

[Marek Vasut]

From: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

It seems the PIO mode does not work, or at least not like it works
on a i.MX28. Each short transfer needs about one second (without an
error message) but does not send anything on the I2C lines.

From: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Christoph Baumann <cb-/RsSufbtIHM@public.gmane.org>
Cc: Fabio Estevam <r49496-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Torsten Fleischer <to-fleischer-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index df8ff5a..ca54ac4 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -96,10 +96,17 @@
 #define MXS_CMD_I2C_READ	(MXS_I2C_CTRL0_SEND_NAK_ON_LAST | \
 				 MXS_I2C_CTRL0_MASTER_MODE)
 
+enum mxs_i2c_devtype {
+	MXS_I2C_UNKNOWN = 0,
+	MXS_I2C_V1,
+	MXS_I2C_V2,
+};
+
 /**
  * struct mxs_i2c_dev - per device, private MXS-I2C data
  *
  * @dev: driver model device node
+ * @dev_type: distinguish i.MX23/i.MX28 features
  * @regs: IO registers pointer
  * @cmd_complete: completion object for transaction wait
  * @cmd_err: error code for last transaction
@@ -107,6 +114,7 @@
  */
 struct mxs_i2c_dev {
 	struct device *dev;
+	enum mxs_i2c_devtype dev_type;
 	void __iomem *regs;
 	struct completion cmd_complete;
 	int cmd_err;
@@ -491,9 +499,10 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	 * is set to 8 bytes, transfers shorter than 8 bytes are transfered
 	 * using PIO mode while longer transfers use DMA. The 8 byte border is
 	 * based on this empirical measurement and a lot of previous frobbing.
+	 * Note: this special feature only works on i.MX28 SoC
 	 */
 	i2c->cmd_err = 0;
-	if (msg->len < 8) {
+	if (i2c->dev_type == MXS_I2C_V2 && msg->len < 8) {
 		ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
 		if (ret)
 			mxs_i2c_reset(i2c);
@@ -632,8 +641,28 @@ static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c)
 	return 0;
 }
 
+static struct platform_device_id mxs_i2c_devtype[] = {
+	{
+		.name = "imx23-i2c",
+		.driver_data = MXS_I2C_V1,
+	}, {
+		.name = "imx28-i2c",
+		.driver_data = MXS_I2C_V2,
+	}, { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, mxs_i2c_devtype);
+
+static const struct of_device_id mxs_i2c_dt_ids[] = {
+	{ .compatible = "fsl,imx23-i2c", .data = &mxs_i2c_devtype[0], },
+	{ .compatible = "fsl,imx28-i2c", .data = &mxs_i2c_devtype[1], },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_i2c_dt_ids);
+
 static int mxs_i2c_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id =
+				of_match_device(mxs_i2c_dt_ids, &pdev->dev);
 	struct device *dev = &pdev->dev;
 	struct mxs_i2c_dev *i2c;
 	struct i2c_adapter *adap;
@@ -645,6 +674,11 @@ static int mxs_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
+	if (of_id) {
+		const struct platform_device_id *device_id = of_id->data;
+		i2c->dev_type = device_id->driver_data;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(pdev, 0);
 
@@ -720,12 +754,6 @@ static int mxs_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id mxs_i2c_dt_ids[] = {
-	{ .compatible = "fsl,imx28-i2c", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, mxs_i2c_dt_ids);
-
 static struct platform_driver mxs_i2c_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
-- 
1.7.10.4

[PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Marek Vasut]

Analyze and rework the PIO mode operation. The PIO mode operation
was unreliable on MX28, by analyzing the bus with LA, the checks
for when data were available or were to be sent were wrong.

The PIO WRITE has to be completely reworked as it multiple problems.
The MX23 datasheet helped here, see comments in the code for details.
The problems boil down to:
- RUN bit in CTRL0 must be set after DATA register was written
- The PIO transfer must be 4 bytes long tops, otherwise use
  clock stretching.
Both of these fixes are implemented.

The PIO READ operation can only be done for up to four bytes as
we are unable to read out the data from the DATA register fast
enough.

This patch also tries to document the investigation within the
code.

Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Christoph Baumann <cb-/RsSufbtIHM@public.gmane.org>
Cc: Fabio Estevam <r49496-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Torsten Fleischer <to-fleischer-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |  263 ++++++++++++++++++++++++++----------------
 1 file changed, 166 insertions(+), 97 deletions(-)

V2: - Fix the data register shift computation algorithm and document that
      some more. The original algo failed for short 1-byte writes as seen
      in http://www.mail-archive.com/linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg12812.html
    - Add me a copyright/authorship line, since according to git blame, I
      have quite a lot of authored lines in the driver. Wolfram, I brought
      this up some time ago already, but finally got to it. You're OK with
      this, right?

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index ca54ac4..741e5dc 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -1,6 +1,7 @@
 /*
  * Freescale MXS I2C bus driver
  *
+ * Copyright (C) 2012-2013 Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
  * Copyright (C) 2011-2012 Wolfram Sang, Pengutronix e.K.
  *
  * based on a (non-working) driver which was:
@@ -295,48 +296,11 @@ write_init_pio_fail:
 	return -EINVAL;
 }
 
-static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c)
+static int mxs_i2c_pio_wait_xfer_end(struct mxs_i2c_dev *i2c)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 
-	while (!(readl(i2c->regs + MXS_I2C_DEBUG0) &
-		MXS_I2C_DEBUG0_DMAREQ)) {
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
-		cond_resched();
-	}
-
-	return 0;
-}
-
-static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last)
-{
-	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
-
-	/*
-	 * We do not use interrupts in the PIO mode. Due to the
-	 * maximum transfer length being 8 bytes in PIO mode, the
-	 * overhead of interrupt would be too large and this would
-	 * neglect the gain from using the PIO mode.
-	 */
-
-	while (!(readl(i2c->regs + MXS_I2C_CTRL1) &
-		MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ)) {
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
-		cond_resched();
-	}
-
-	writel(MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ,
-		i2c->regs + MXS_I2C_CTRL1_CLR);
-
-	/*
-	 * When ending a transfer with a stop, we have to wait for the bus to
-	 * go idle before we report the transfer as completed. Otherwise the
-	 * start of the next transfer may race with the end of the current one.
-	 */
-	while (last && (readl(i2c->regs + MXS_I2C_STAT) &
-			(MXS_I2C_STAT_BUS_BUSY | MXS_I2C_STAT_CLK_GEN_BUSY))) {
+	while (readl(i2c->regs + MXS_I2C_CTRL0) & MXS_I2C_CTRL0_RUN) {
 		if (time_after(jiffies, timeout))
 			return -ETIMEDOUT;
 		cond_resched();
@@ -374,106 +338,201 @@ static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd)
 	writel(reg, i2c->regs + MXS_I2C_CTRL0);
 }
 
+/*
+ * Start WRITE transaction on the I2C bus. By studying i.MX23 datasheet,
+ * CTRL0::PIO_MODE bit description clarifies the order in which the registers
+ * must be written during PIO mode operation. First, the CTRL0 register has
+ * to be programmed with all the necessary bits but the RUN bit. Then the
+ * payload has to be written into the DATA register. Finally, the transmission
+ * is executed by setting the RUN bit in CTRL0.
+ */
+static void mxs_i2c_pio_trigger_write_cmd(struct mxs_i2c_dev *i2c, u32 cmd,
+					  u32 data)
+{
+	writel(cmd, i2c->regs + MXS_I2C_CTRL0);
+	writel(data, i2c->regs + MXS_I2C_DATA);
+	writel(MXS_I2C_CTRL0_RUN, i2c->regs + MXS_I2C_CTRL0_SET);
+}
+
 static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 			struct i2c_msg *msg, uint32_t flags)
 {
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
 	uint32_t addr_data = msg->addr << 1;
 	uint32_t data = 0;
-	int i, shifts_left, ret;
+	int i, ret, xlen = 0, xmit = 0;
+	uint32_t start;
 
 	/* Mute IRQs coming from this block. */
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR);
 
+	/*
+	 * MX23 idea:
+	 * - Enable CTRL0::PIO_MODE (1 << 24)
+	 * - Enable CTRL1::ACK_MODE (1 << 27)
+	 *
+	 * WARNING! The MX23 is broken in some way, even if it claims
+	 * to support PIO, when we try to transfer any amount of data
+	 * that is not aligned to 4 bytes, the DMA engine will have
+	 * bits in DEBUG1::DMA_BYTES_ENABLES still set even after the
+	 * transfer. This in turn will mess up the next transfer as
+	 * the block it emit one byte write onto the bus terminated
+	 * with a NAK+STOP. A possible workaround is to reset the IP
+	 * block after every PIO transmission, which might just work.
+	 *
+	 * NOTE: The CTRL0::PIO_MODE description is important, since
+	 * it outlines how the PIO mode is really supposed to work.
+	 */
+
 	if (msg->flags & I2C_M_RD) {
+		/*
+		 * PIO READ transfer:
+		 *
+		 * This transfer MUST be limited to 4 bytes maximum. It is not
+		 * possible to transfer more than four bytes via PIO, since we
+		 * can not in any way make sure we can read the data from the
+		 * DATA register fast enough. Besides, the RX FIFO is only four
+		 * bytes deep, thus we can only really read up to four bytes at
+		 * time. Finally, there is no bit indicating us that new data
+		 * arrived at the FIFO and can thus be fetched from the DATA
+		 * register.
+		 */
+		BUG_ON(msg->len > 4);
+
 		addr_data |= I2C_SMBUS_READ;
 
 		/* SELECT command. */
-		mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_SELECT);
+		mxs_i2c_pio_trigger_write_cmd(i2c, MXS_CMD_I2C_SELECT, addr_data);
 
-		ret = mxs_i2c_pio_wait_dmareq(i2c);
-		if (ret)
-			return ret;
-
-		writel(addr_data, i2c->regs + MXS_I2C_DATA);
-		writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR);
-
-		ret = mxs_i2c_pio_wait_cplt(i2c, 0);
-		if (ret)
-			return ret;
-
-		if (mxs_i2c_pio_check_error_state(i2c))
+		ret = mxs_i2c_pio_wait_xfer_end(i2c);
+		if (ret) {
+			dev_err(i2c->dev,
+				"PIO: Failed to send SELECT command!\n");
 			goto cleanup;
+		}
 
 		/* READ command. */
 		mxs_i2c_pio_trigger_cmd(i2c,
 					MXS_CMD_I2C_READ | flags |
 					MXS_I2C_CTRL0_XFER_COUNT(msg->len));
 
+		ret = mxs_i2c_pio_wait_xfer_end(i2c);
+		if (ret) {
+			dev_err(i2c->dev,
+				"PIO: Failed to send SELECT command!\n");
+			goto cleanup;
+		}
+
+		data = readl(i2c->regs + MXS_I2C_DATA);
 		for (i = 0; i < msg->len; i++) {
-			if ((i & 3) == 0) {
-				ret = mxs_i2c_pio_wait_dmareq(i2c);
-				if (ret)
-					return ret;
-				data = readl(i2c->regs + MXS_I2C_DATA);
-				writel(MXS_I2C_DEBUG0_DMAREQ,
-				       i2c->regs + MXS_I2C_DEBUG0_CLR);
-			}
 			msg->buf[i] = data & 0xff;
 			data >>= 8;
 		}
 	} else {
+		/*
+		 * PIO WRITE transfer:
+		 *
+		 * The code below implements clock stretching to circumvent
+		 * the possibility of kernel not being able to supply data
+		 * fast enough. It is possible to transfer arbitrary amount
+		 * of data using PIO write.
+		 */
 		addr_data |= I2C_SMBUS_WRITE;
 
-		/* WRITE command. */
-		mxs_i2c_pio_trigger_cmd(i2c,
-					MXS_CMD_I2C_WRITE | flags |
-					MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1));
-
 		/*
 		 * The LSB of data buffer is the first byte blasted across
 		 * the bus. Higher order bytes follow. Thus the following
 		 * filling schematic.
 		 */
+
 		data = addr_data << 24;
+
+		/* Start the transfer with START condition. */
+		start = MXS_I2C_CTRL0_PRE_SEND_START;
+
+		/* If the transfer is long, use clock stretching. */
+		if (msg->len > 3)
+			start |= MXS_I2C_CTRL0_RETAIN_CLOCK;
+
 		for (i = 0; i < msg->len; i++) {
 			data >>= 8;
 			data |= (msg->buf[i] << 24);
-			if ((i & 3) == 2) {
-				ret = mxs_i2c_pio_wait_dmareq(i2c);
-				if (ret)
-					return ret;
-				writel(data, i2c->regs + MXS_I2C_DATA);
-				writel(MXS_I2C_DEBUG0_DMAREQ,
-				       i2c->regs + MXS_I2C_DEBUG0_CLR);
+
+			xmit = 0;
+
+			/* This is the last transfer of the message. */
+			if (i + 1 == msg->len) {
+				/* Add optional STOP flag. */
+				start |= flags;
+				/* Remove RETAIN_CLOCK bit. */
+				start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK;
+				xmit = 1;
 			}
-		}
 
-		shifts_left = 24 - (i & 3) * 8;
-		if (shifts_left) {
-			data >>= shifts_left;
-			ret = mxs_i2c_pio_wait_dmareq(i2c);
-			if (ret)
-				return ret;
-			writel(data, i2c->regs + MXS_I2C_DATA);
+			/* Four bytes are ready in the "data" variable. */
+			if ((i & 3) == 2)
+				xmit = 1;
+
+			/* Nothing interesting happened, continue stuffing. */
+			if (!xmit)
+				continue;
+
+			/*
+			 * Compute the size of the transfer and shift the
+			 * data accordingly.
+			 *
+			 * i = (4k + 0) .... xlen = 2
+			 * i = (4k + 1) .... xlen = 3
+			 * i = (4k + 2) .... xlen = 4
+			 * i = (4k + 3) .... xlen = 1
+			 */
+
+			if ((i % 4) == 3)
+				xlen = 1;
+			else
+				xlen = (i % 4) + 2;
+
+			data >>= (4 - xlen) * 8;
+
+			dev_dbg(i2c->dev,
+				"PIO: len=%i pos=%i total=%i [W%s%s%s]\n",
+				xlen, i, msg->len,
+				start & MXS_I2C_CTRL0_PRE_SEND_START ? "S": "",
+				start & MXS_I2C_CTRL0_POST_SEND_STOP ? "E": "",
+				start & MXS_I2C_CTRL0_RETAIN_CLOCK ? "C": "");
+
 			writel(MXS_I2C_DEBUG0_DMAREQ,
 			       i2c->regs + MXS_I2C_DEBUG0_CLR);
+
+			mxs_i2c_pio_trigger_write_cmd(i2c,
+				start | MXS_I2C_CTRL0_MASTER_MODE |
+				MXS_I2C_CTRL0_DIRECTION |
+				MXS_I2C_CTRL0_XFER_COUNT(xlen), data);
+
+			/* The START condition is sent only once. */
+			start &= ~MXS_I2C_CTRL0_PRE_SEND_START;
+
+			/* Wait for the end of the transfer. */
+			ret = mxs_i2c_pio_wait_xfer_end(i2c);
+			if (ret) {
+				dev_err(i2c->dev,
+					"PIO: Failed to finish WRITE cmd!\n");
+				break;
+			}
+
+			/* Check NAK here ? */
 		}
 	}
 
-	ret = mxs_i2c_pio_wait_cplt(i2c, flags & MXS_I2C_CTRL0_POST_SEND_STOP);
-	if (ret)
-		return ret;
-
 	/* make sure we capture any occurred error into cmd_err */
-	mxs_i2c_pio_check_error_state(i2c);
+	ret = mxs_i2c_pio_check_error_state(i2c);
 
 cleanup:
 	/* Clear any dangling IRQs and re-enable interrupts. */
 	writel(MXS_I2C_IRQ_MASK, i2c->regs + MXS_I2C_CTRL1_CLR);
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -485,6 +544,7 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
 	int ret;
 	int flags;
+	int use_pio = 0;
 
 	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
 
@@ -495,16 +555,24 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 		return -EINVAL;
 
 	/*
-	 * The current boundary to select between PIO/DMA transfer method
-	 * is set to 8 bytes, transfers shorter than 8 bytes are transfered
-	 * using PIO mode while longer transfers use DMA. The 8 byte border is
-	 * based on this empirical measurement and a lot of previous frobbing.
-	 * Note: this special feature only works on i.MX28 SoC
+	 * The MX28 I2C IP block can only do PIO READ for transfer of to up
+	 * 4 bytes of length. The write transfer is not limited as it can use
+	 * clock stretching to avoid FIFO underruns.
 	 */
+	if ((msg->flags & I2C_M_RD) && (msg->len <= 4))
+		use_pio = 1;
+	if (!(msg->flags & I2C_M_RD) && (msg->len < 7))
+		use_pio = 1;
+
+	/* Disable PIO on MX23. */
+	if (i2c->dev_type == MXS_I2C_V1)
+		use_pio = 0;
+
 	i2c->cmd_err = 0;
-	if (i2c->dev_type == MXS_I2C_V2 && msg->len < 8) {
+	if (use_pio) {
 		ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
-		if (ret)
+		/* No need to reset the block if NAK was received. */
+		if (ret && (ret != -ENXIO))
 			mxs_i2c_reset(i2c);
 	} else {
 		INIT_COMPLETION(i2c->cmd_complete);
@@ -516,9 +584,11 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 						msecs_to_jiffies(1000));
 		if (ret == 0)
 			goto timeout;
+
+		ret = i2c->cmd_err;
 	}
 
-	if (i2c->cmd_err == -ENXIO) {
+	if (ret == -ENXIO) {
 		/*
 		 * If the transfer fails with a NAK from the slave the
 		 * controller halts until it gets told to return to idle state.
@@ -527,8 +597,6 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 		       i2c->regs + MXS_I2C_CTRL1_SET);
 	}
 
-	ret = i2c->cmd_err;
-
 	dev_dbg(i2c->dev, "Done with err=%d\n", ret);
 
 	return ret;
@@ -775,6 +843,7 @@ static void __exit mxs_i2c_exit(void)
 }
 module_exit(mxs_i2c_exit);
 
+MODULE_AUTHOR("Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>");
 MODULE_AUTHOR("Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
 MODULE_DESCRIPTION("MXS I2C Bus Driver");
 MODULE_LICENSE("GPL");
-- 
1.7.10.4

[PATCH RESEND 3/3] i2c: mxs: Fix PIO mode on i.MX23

[Marek Vasut]

The i.MX23 I2C controller is also capable of PIO, but needs a little harder
push to behave. The controller needs to be reset after every PIO/DMA operation
for some reason, otherwise in rare cases, the controller can hang or emit
bytes onto the bus.

Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Christoph Baumann <cb-/RsSufbtIHM@public.gmane.org>
Cc: Fabio Estevam <r49496-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Torsten Fleischer <to-fleischer-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |   40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 741e5dc..a1d2ca2 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -36,10 +36,12 @@
 
 #define MXS_I2C_CTRL0		(0x00)
 #define MXS_I2C_CTRL0_SET	(0x04)
+#define MXS_I2C_CTRL0_CLR	(0x08)
 
 #define MXS_I2C_CTRL0_SFTRST			0x80000000
 #define MXS_I2C_CTRL0_RUN			0x20000000
 #define MXS_I2C_CTRL0_SEND_NAK_ON_LAST		0x02000000
+#define MXS_I2C_CTRL0_PIO_MODE			0x01000000
 #define MXS_I2C_CTRL0_RETAIN_CLOCK		0x00200000
 #define MXS_I2C_CTRL0_POST_SEND_STOP		0x00100000
 #define MXS_I2C_CTRL0_PRE_SEND_START		0x00080000
@@ -69,10 +71,9 @@
 #define MXS_I2C_STAT_BUS_BUSY			0x00000800
 #define MXS_I2C_STAT_CLK_GEN_BUSY		0x00000400
 
-#define MXS_I2C_DATA		(0xa0)
+#define MXS_I2C_DATA(i2c)	((i2c->dev_type == MXS_I2C_V1) ? 0x60 : 0xa0)
 
-#define MXS_I2C_DEBUG0		(0xb0)
-#define MXS_I2C_DEBUG0_CLR	(0xb8)
+#define MXS_I2C_DEBUG0_CLR(i2c)	((i2c->dev_type == MXS_I2C_V1) ? 0x78 : 0xb8)
 
 #define MXS_I2C_DEBUG0_DMAREQ	0x80000000
 
@@ -350,7 +351,11 @@ static void mxs_i2c_pio_trigger_write_cmd(struct mxs_i2c_dev *i2c, u32 cmd,
 					  u32 data)
 {
 	writel(cmd, i2c->regs + MXS_I2C_CTRL0);
-	writel(data, i2c->regs + MXS_I2C_DATA);
+
+	if (i2c->dev_type == MXS_I2C_V1)
+		writel(MXS_I2C_CTRL0_PIO_MODE, i2c->regs + MXS_I2C_CTRL0_SET);
+
+	writel(data, i2c->regs + MXS_I2C_DATA(i2c));
 	writel(MXS_I2C_CTRL0_RUN, i2c->regs + MXS_I2C_CTRL0_SET);
 }
 
@@ -383,7 +388,6 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 	 * NOTE: The CTRL0::PIO_MODE description is important, since
 	 * it outlines how the PIO mode is really supposed to work.
 	 */
-
 	if (msg->flags & I2C_M_RD) {
 		/*
 		 * PIO READ transfer:
@@ -423,7 +427,7 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 			goto cleanup;
 		}
 
-		data = readl(i2c->regs + MXS_I2C_DATA);
+		data = readl(i2c->regs + MXS_I2C_DATA(i2c));
 		for (i = 0; i < msg->len; i++) {
 			msg->buf[i] = data & 0xff;
 			data >>= 8;
@@ -502,7 +506,7 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 				start & MXS_I2C_CTRL0_RETAIN_CLOCK ? "C": "");
 
 			writel(MXS_I2C_DEBUG0_DMAREQ,
-			       i2c->regs + MXS_I2C_DEBUG0_CLR);
+			       i2c->regs + MXS_I2C_DEBUG0_CLR(i2c));
 
 			mxs_i2c_pio_trigger_write_cmd(i2c,
 				start | MXS_I2C_CTRL0_MASTER_MODE |
@@ -532,6 +536,10 @@ cleanup:
 	writel(MXS_I2C_IRQ_MASK, i2c->regs + MXS_I2C_CTRL1_CLR);
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
 
+	/* Clear the PIO_MODE on i.MX23 */
+	if (i2c->dev_type == MXS_I2C_V1)
+		writel(MXS_I2C_CTRL0_PIO_MODE, i2c->regs + MXS_I2C_CTRL0_CLR);
+
 	return ret;
 }
 
@@ -564,10 +572,6 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	if (!(msg->flags & I2C_M_RD) && (msg->len < 7))
 		use_pio = 1;
 
-	/* Disable PIO on MX23. */
-	if (i2c->dev_type == MXS_I2C_V1)
-		use_pio = 0;
-
 	i2c->cmd_err = 0;
 	if (use_pio) {
 		ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
@@ -597,6 +601,20 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 		       i2c->regs + MXS_I2C_CTRL1_SET);
 	}
 
+	/*
+	 * WARNING!
+	 * The i.MX23 is strange. After each and every operation, it's I2C IP
+	 * block must be reset, otherwise the IP block will misbehave. This can
+	 * be observed on the bus by the block sending out one single byte onto
+	 * the bus. In case such an error happens, bit 27 will be set in the
+	 * DEBUG0 register. This bit is not documented in the i.MX23 datasheet
+	 * and is marked as "TBD" instead. To reset this bit to a correct state,
+	 * reset the whole block. Since the block reset does not take long, do
+	 * reset the block after every transfer to play safe.
+	 */
+	if (i2c->dev_type == MXS_I2C_V1)
+		mxs_i2c_reset(i2c);
+
 	dev_dbg(i2c->dev, "Done with err=%d\n", ret);
 
 	return ret;
-- 
1.7.10.4

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]

On Tue, Jul 30, 2013 at 11:20:36PM +0200, Marek Vasut wrote:
> Analyze and rework the PIO mode operation. The PIO mode operation
> was unreliable on MX28, by analyzing the bus with LA, the checks
> for when data were available or were to be sent were wrong.
> 
> The PIO WRITE has to be completely reworked as it multiple problems.
> The MX23 datasheet helped here, see comments in the code for details.
> The problems boil down to:
> - RUN bit in CTRL0 must be set after DATA register was written
> - The PIO transfer must be 4 bytes long tops, otherwise use
>   clock stretching.
> Both of these fixes are implemented.
> 
> The PIO READ operation can only be done for up to four bytes as
> we are unable to read out the data from the DATA register fast
> enough.
> 
> This patch also tries to document the investigation within the
> code.
> 
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl
have used these patches. Don't be shy :)

> Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Christoph Baumann <cb-/RsSufbtIHM@public.gmane.org>
> Cc: Fabio Estevam <r49496-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Torsten Fleischer <to-fleischer-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-mxs.c |  263 ++++++++++++++++++++++++++----------------
>  1 file changed, 166 insertions(+), 97 deletions(-)
> 
> V2: - Fix the data register shift computation algorithm and document that
>       some more. The original algo failed for short 1-byte writes as seen
>       in http://www.mail-archive.com/linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg12812.html
>     - Add me a copyright/authorship line, since according to git blame, I
>       have quite a lot of authored lines in the driver. Wolfram, I brought
>       this up some time ago already, but finally got to it. You're OK with
>       this, right?

Sure.

One question: Wouldn't it be more logical to have this patch first (fix
pio) and then squash patches 1 and 3 as one on the top (add mx23 to now
working pio)? I am not pushing too hard if this means a lot of work, but
it sounds a bit more logical to me.

>  	if (msg->flags & I2C_M_RD) {
> +		/*
> +		 * PIO READ transfer:
> +		 *
> +		 * This transfer MUST be limited to 4 bytes maximum. It is not
> +		 * possible to transfer more than four bytes via PIO, since we
> +		 * can not in any way make sure we can read the data from the
> +		 * DATA register fast enough. Besides, the RX FIFO is only four
> +		 * bytes deep, thus we can only really read up to four bytes at
> +		 * time. Finally, there is no bit indicating us that new data
> +		 * arrived at the FIFO and can thus be fetched from the DATA
> +		 * register.
> +		 */
> +		BUG_ON(msg->len > 4);

How could this happen? There is a check in mxs_i2c_xfer_msg.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Alexandre Belloni]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 05/08/2013 12:36, Wolfram Sang wrote:
> On Tue, Jul 30, 2013 at 11:20:36PM +0200, Marek Vasut wrote:
>> Analyze and rework the PIO mode operation. The PIO mode operation
>> was unreliable on MX28, by analyzing the bus with LA, the checks
>> for when data were available or were to be sent were wrong.
>>
>> The PIO WRITE has to be completely reworked as it multiple problems.
>> The MX23 datasheet helped here, see comments in the code for details.
>> The problems boil down to:
>> - RUN bit in CTRL0 must be set after DATA register was written
>> - The PIO transfer must be 4 bytes long tops, otherwise use
>>   clock stretching.
>> Both of these fixes are implemented.
>>
>> The PIO READ operation can only be done for up to four bytes as
>> we are unable to read out the data from the DATA register fast
>> enough.
>>
>> This patch also tries to document the investigation within the
>> code.
>>
>> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>
> Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl
> have used these patches. Don't be shy :)
>
>

It was on my todo list until now.

Tested-by: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

- -- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBCAAGBQJR/4hNAAoJEKbNnwlvZCyzv0gP/1/ED0u/HwbJFU6mxi3ALpLB
NJ/RYMlmU+u+4FlNzlAEYBPSIp/2IFhgmTLnf+t1E3VsvjZ0mchYZwp/3IH/Z4X5
HAx5X0/mJ19Nqlq7ARuQvKaF+i7IQxOY61GybxX8W7gvHOL2FXufZFdVh+KjRFDY
rOnxgugCF473iZ7YUA5p+qJo9H/uJKaUcHQO56yUJkA3BOM4MVWztMUI0wuICg9H
Khq4zKuL6/qXsL1fz3sT5L1pUUYUZFUt75pz1zIYvovEOg+Bk3kSHz2xRQPYHXJ2
UzTDANNnaIEkh9ZzuNoVywOb3Qme9QLufD/ju29QYmOZvVBuTo9oSgdxuZGZ8Wxl
u6y02wp3y1zWXhHc2nvOAyWOWYiYvIJrn7dkvMhLmiJbc941gyJdnOgutsKZlBnu
atPOVt1muj0sz0wTHe9UvpSqQf18T8kKdtjh+VyI8px5OWfjc7FmJVOPHTUwZ/UV
obzNskq/MAFoib/hd6hU3raDpRDQYBmgVet1HHyhWHObn2SC8hRLTwIoZ5YHo+pH
SZ62nGJmeVm1Bfk10jUrmWQ7NEMiDEJR0wnGiCmZm3x/fwXe4Gu8eiDKZxYeLCJO
zytXpNq3yW+2KuxMvwfsiVJ3kHx6GGHWVLtSLuCV1Nqmw4bpXdOuGGPdLLjgu7Nx
w8pZ+BZpzDDmXtZGPXNk
=3+JD
-----END PGP SIGNATURE-----

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Marek Vasut]

Dear Wolfram Sang,

> On Tue, Jul 30, 2013 at 11:20:36PM +0200, Marek Vasut wrote:
> > Analyze and rework the PIO mode operation. The PIO mode operation
> > was unreliable on MX28, by analyzing the bus with LA, the checks
> > for when data were available or were to be sent were wrong.
> > 
> > The PIO WRITE has to be completely reworked as it multiple problems.
> > The MX23 datasheet helped here, see comments in the code for details.
> > The problems boil down to:
> > - RUN bit in CTRL0 must be set after DATA register was written
> > - The PIO transfer must be 4 bytes long tops, otherwise use
> > 
> >   clock stretching.
> > 
> > Both of these fixes are implemented.
> > 
> > The PIO READ operation can only be done for up to four bytes as
> > we are unable to read out the data from the DATA register fast
> > enough.
> > 
> > This patch also tries to document the investigation within the
> > code.
> > 
> > Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> 
> Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl
> have used these patches. Don't be shy :)
> 
> > Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > Cc: Christoph Baumann <cb-/RsSufbtIHM@public.gmane.org>
> > Cc: Fabio Estevam <r49496-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Torsten Fleischer <to-fleischer-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org>
> > Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> > ---
> > 
> >  drivers/i2c/busses/i2c-mxs.c |  263
> >  ++++++++++++++++++++++++++---------------- 1 file changed, 166
> >  insertions(+), 97 deletions(-)
> > 
> > V2: - Fix the data register shift computation algorithm and document that
> > 
> >       some more. The original algo failed for short 1-byte writes as seen
> >       in
> >       http://www.mail-archive.com/linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg12812.htm
> >       l
> >     
> >     - Add me a copyright/authorship line, since according to git blame, I
> >     
> >       have quite a lot of authored lines in the driver. Wolfram, I
> >       brought this up some time ago already, but finally got to it.
> >       You're OK with this, right?
> 
> Sure.
> 
> One question: Wouldn't it be more logical to have this patch first (fix
> pio) and then squash patches 1 and 3 as one on the top (add mx23 to now
> working pio)? I am not pushing too hard if this means a lot of work, but
> it sounds a bit more logical to me.

I would prefer to keep Juergens' patch separate from mine if you don't mind to 
be clear on the authorship.

> >  	if (msg->flags & I2C_M_RD) {
> > 
> > +		/*
> > +		 * PIO READ transfer:
> > +		 *
> > +		 * This transfer MUST be limited to 4 bytes maximum. It is not
> > +		 * possible to transfer more than four bytes via PIO, since we
> > +		 * can not in any way make sure we can read the data from the
> > +		 * DATA register fast enough. Besides, the RX FIFO is only four
> > +		 * bytes deep, thus we can only really read up to four bytes at
> > +		 * time. Finally, there is no bit indicating us that new data
> > +		 * arrived at the FIFO and can thus be fetched from the DATA
> > +		 * register.
> > +		 */
> > +		BUG_ON(msg->len > 4);
> 
> How could this happen? There is a check in mxs_i2c_xfer_msg.

It cannot happen, I added the check here to make sure when someone becomes 
adventurous enough to start messing with these constants, it will stop him early 
enough.

Best regards,
Marek Vasut

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Shawn Guo]

On Mon, Aug 05, 2013 at 12:36:47PM +0200, Wolfram Sang wrote:
> Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl
> have used these patches. Don't be shy :)

Tested-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]


> > One question: Wouldn't it be more logical to have this patch first (fix
> > pio) and then squash patches 1 and 3 as one on the top (add mx23 to now
> > working pio)? I am not pushing too hard if this means a lot of work, but
> > it sounds a bit more logical to me.
> 
> I would prefer to keep Juergens' patch separate from mine if you don't mind to 
> be clear on the authorship.

If you add both SoB, everything should be fine. We often work on someone
else's patches, no?

> > >  	if (msg->flags & I2C_M_RD) {
> > > 
> > > +		/*
> > > +		 * PIO READ transfer:
> > > +		 *
> > > +		 * This transfer MUST be limited to 4 bytes maximum. It is not
> > > +		 * possible to transfer more than four bytes via PIO, since we
> > > +		 * can not in any way make sure we can read the data from the
> > > +		 * DATA register fast enough. Besides, the RX FIFO is only four
> > > +		 * bytes deep, thus we can only really read up to four bytes at
> > > +		 * time. Finally, there is no bit indicating us that new data
> > > +		 * arrived at the FIFO and can thus be fetched from the DATA
> > > +		 * register.
> > > +		 */
> > > +		BUG_ON(msg->len > 4);
> > 
> > How could this happen? There is a check in mxs_i2c_xfer_msg.
> 
> It cannot happen, I added the check here to make sure when someone becomes 
> adventurous enough to start messing with these constants, it will stop him early 
> enough.

Then, add the comment to the check so ppl will notice there? I like to
keep BUG_ON sparse, since it is hard to tell if the occasion is really
worth stopping the machine.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Marek Vasut]

Dear Wolfram Sang,

> > > One question: Wouldn't it be more logical to have this patch first (fix
> > > pio) and then squash patches 1 and 3 as one on the top (add mx23 to now
> > > working pio)? I am not pushing too hard if this means a lot of work,
> > > but it sounds a bit more logical to me.
> > 
> > I would prefer to keep Juergens' patch separate from mine if you don't
> > mind to be clear on the authorship.
> 
> If you add both SoB, everything should be fine. We often work on someone
> else's patches, no?

I agree, but I still don't like squashing the two patches together. I forgot to 
mention it last time, but please, look at the patches one more time. Jurgens' 
does the DT change and mine fixes the PIO on MX23, I'd like to keep these 
changes separate.

> > > >  	if (msg->flags & I2C_M_RD) {
> > > > 
> > > > +		/*
> > > > +		 * PIO READ transfer:
> > > > +		 *
> > > > +		 * This transfer MUST be limited to 4 bytes maximum. It 
is not
> > > > +		 * possible to transfer more than four bytes via PIO, 
since we
> > > > +		 * can not in any way make sure we can read the data 
from the
> > > > +		 * DATA register fast enough. Besides, the RX FIFO is 
only four
> > > > +		 * bytes deep, thus we can only really read up to four 
bytes at
> > > > +		 * time. Finally, there is no bit indicating us that new 
data
> > > > +		 * arrived at the FIFO and can thus be fetched from the 
DATA
> > > > +		 * register.
> > > > +		 */
> > > > +		BUG_ON(msg->len > 4);
> > > 
> > > How could this happen? There is a check in mxs_i2c_xfer_msg.
> > 
> > It cannot happen, I added the check here to make sure when someone
> > becomes adventurous enough to start messing with these constants, it
> > will stop him early enough.
> 
> Then, add the comment to the check so ppl will notice there?

If I could add a big red flashing sign into the code stating "If you go over 4 
bytes here, a kitten will die", then by all means, I would add it. 
Unfortunatelly, there is no such thing possible.

> I like to
> keep BUG_ON sparse, since it is hard to tell if the occasion is really
> worth stopping the machine.

While I agree with you on this, I would also like for the kernel to check if 
someone accidentally screwed up and thoroughly warn him. We cannot continue 
anyway, because if more than 4 bytes are requested, the controller would simply 
not give us more than the last 4 bytes and this would result in an undefined 
behavior and data corruption during reception. We simply do not want that.

Maybe WARN_ONCE and return with error might just work?

Best regards,
Marek Vasut

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]


> I agree, but I still don't like squashing the two patches together. I forgot to 
> mention it last time, but please, look at the patches one more time. Jurgens' 
> does the DT change and mine fixes the PIO on MX23, I'd like to keep these 
> changes separate.

Ok, as said, I am not pushing hard on this.

> > Then, add the comment to the check so ppl will notice there?
> 
> If I could add a big red flashing sign into the code stating "If you go over 4 
> bytes here, a kitten will die", then by all means, I would add it. 
> Unfortunatelly, there is no such thing possible.

I'd say if somebody changes the code with that comment on top of it, it
is really deserved...


> Maybe WARN_ONCE and return with error might just work?

... but if you insist, I can accept this.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Marek Vasut]

Dear Wolfram Sang,

> > I agree, but I still don't like squashing the two patches together. I
> > forgot to mention it last time, but please, look at the patches one more
> > time. Jurgens' does the DT change and mine fixes the PIO on MX23, I'd
> > like to keep these changes separate.
> 
> Ok, as said, I am not pushing hard on this.

Thanks!

> > > Then, add the comment to the check so ppl will notice there?
> > 
> > If I could add a big red flashing sign into the code stating "If you go
> > over 4 bytes here, a kitten will die", then by all means, I would add
> > it. Unfortunatelly, there is no such thing possible.
> 
> I'd say if somebody changes the code with that comment on top of it, it
> is really deserved...

You mean the kitten ... ? ;-)

> > Maybe WARN_ONCE and return with error might just work?
> 
> ... but if you insist, I can accept this.

Thinking about it harder, WARN might be more appropriate. Maybe I'll just remove 
it altogether, I'll think about it some more.

Best regards,
Marek Vasut

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]


> Thinking about it harder, WARN might be more appropriate. Maybe I'll just remove 
> it altogether, I'll think about it some more.

And when resending, please rebase on top of "i2c: i2c-mxs: Use DMA
mode even for small transfers" which I pushed out a few minutes ago.

Thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Marek Vasut]

Dear Wolfram Sang,

> > Thinking about it harder, WARN might be more appropriate. Maybe I'll just
> > remove it altogether, I'll think about it some more.
> 
> And when resending, please rebase on top of "i2c: i2c-mxs: Use DMA
> mode even for small transfers" which I pushed out a few minutes ago.

OK, thanks for the heads up.

Best regards,
Marek Vasut

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Torsten Fleischer]

Hi Marek,
> +
> +			mxs_i2c_pio_trigger_write_cmd(i2c,
> +				start | MXS_I2C_CTRL0_MASTER_MODE |
> +				MXS_I2C_CTRL0_DIRECTION |
> +				MXS_I2C_CTRL0_XFER_COUNT(xlen), data);
> +
> +			/* The START condition is sent only once. */
> +			start &= ~MXS_I2C_CTRL0_PRE_SEND_START;
> +
> +			/* Wait for the end of the transfer. */
> +			ret = mxs_i2c_pio_wait_xfer_end(i2c);
> +			if (ret) {
> +				dev_err(i2c->dev,
> +					"PIO: Failed to finish WRITE cmd!\n");
> +				break;
> +			}
> +
> +			/* Check NAK here ? */

checking for the NAK at this point is really necessary.

I've tested the patches by writing to the EEPROM using the at24 driver.
If the data to write cross the EEPROM's page boundary then the at24 driver 
splits the I2C write commands.

After the first I2C write command is done the at24 driver immediately tries to 
send the next I2C write command. Normally the EEPROM signals a NAK, because 
the previous write operation is internally still in progress. The NAK will be 
handled by the at24 driver. It later retries the I2C write command.

But if the the EEPROM signals a NAK and the I2C write command has more than 3 
and less than 7 bytes then the error "PIO: Failed to finish WRITE cmd!" 
appears, because the loop is not exited due to the NAK.

Best regards,
Torsten Fleischer

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Marek Vasut]

Dear Torsten Fleischer,

> Hi Marek,
> 
> > +
> > +			mxs_i2c_pio_trigger_write_cmd(i2c,
> > +				start | MXS_I2C_CTRL0_MASTER_MODE |
> > +				MXS_I2C_CTRL0_DIRECTION |
> > +				MXS_I2C_CTRL0_XFER_COUNT(xlen), data);
> > +
> > +			/* The START condition is sent only once. */
> > +			start &= ~MXS_I2C_CTRL0_PRE_SEND_START;
> > +
> > +			/* Wait for the end of the transfer. */
> > +			ret = mxs_i2c_pio_wait_xfer_end(i2c);
> > +			if (ret) {
> > +				dev_err(i2c->dev,
> > +					"PIO: Failed to finish WRITE cmd!\n");
> > +				break;
> > +			}
> > +
> > +			/* Check NAK here ? */
> 
> checking for the NAK at this point is really necessary.
> 
> I've tested the patches by writing to the EEPROM using the at24 driver.
> If the data to write cross the EEPROM's page boundary then the at24 driver
> splits the I2C write commands.
> 
> After the first I2C write command is done the at24 driver immediately tries
> to send the next I2C write command. Normally the EEPROM signals a NAK,
> because the previous write operation is internally still in progress. The
> NAK will be handled by the at24 driver. It later retries the I2C write
> command.
> 
> But if the the EEPROM signals a NAK and the I2C write command has more than
> 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE cmd!"
> appears, because the loop is not exited due to the NAK.

Ok, so we check

HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we "goto 
cleanup" with ret = -ENXIO . How does it sound to you? Wolfram, is -ENXIO the 
right one here?

Best regards,
Marek Vasut

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Torsten Fleischer]

Hi Marek,

> > > +
> > > +			/* Check NAK here ? */
> > 
> > checking for the NAK at this point is really necessary.
> > 
> > I've tested the patches by writing to the EEPROM using the at24 driver.
> > If the data to write cross the EEPROM's page boundary then the at24 driver
> > splits the I2C write commands.
> > 
> > After the first I2C write command is done the at24 driver immediately
> > tries
> > to send the next I2C write command. Normally the EEPROM signals a NAK,
> > because the previous write operation is internally still in progress. The
> > NAK will be handled by the at24 driver. It later retries the I2C write
> > command.
> > 
> > But if the the EEPROM signals a NAK and the I2C write command has more
> > than
> > 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE cmd!"
> > appears, because the loop is not exited due to the NAK.
> 
> Ok, so we check
> 
> HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we
> "goto cleanup" with ret = -ENXIO . How does it sound to you?

this sounds good to me.

Best regards
Torsten Fleischer

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Marek Vasut]

Dear Torsten Fleischer,

> Hi Marek,
> 
> > > > +
> > > > +			/* Check NAK here ? */
> > > 
> > > checking for the NAK at this point is really necessary.
> > > 
> > > I've tested the patches by writing to the EEPROM using the at24 driver.
> > > If the data to write cross the EEPROM's page boundary then the at24
> > > driver splits the I2C write commands.
> > > 
> > > After the first I2C write command is done the at24 driver immediately
> > > tries
> > > to send the next I2C write command. Normally the EEPROM signals a NAK,
> > > because the previous write operation is internally still in progress.
> > > The NAK will be handled by the at24 driver. It later retries the I2C
> > > write command.
> > > 
> > > But if the the EEPROM signals a NAK and the I2C write command has more
> > > than
> > > 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE
> > > cmd!" appears, because the loop is not exited due to the NAK.
> > 
> > Ok, so we check
> > 
> > HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we
> > "goto cleanup" with ret = -ENXIO . How does it sound to you?
> 
> this sounds good to me.

Does it fix your EEPROM issue then ?

Best regards,
Marek Vasut

Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Torsten Fleischer]

Hi Marek,
> > 
> > > > > +
> > > > > +			/* Check NAK here ? */
> > > > 
> > > > checking for the NAK at this point is really necessary.
> > > > 
> > > > I've tested the patches by writing to the EEPROM using the at24
> > > > driver.
> > > > If the data to write cross the EEPROM's page boundary then the at24
> > > > driver splits the I2C write commands.
> > > > 
> > > > After the first I2C write command is done the at24 driver immediately
> > > > tries
> > > > to send the next I2C write command. Normally the EEPROM signals a NAK,
> > > > because the previous write operation is internally still in progress.
> > > > The NAK will be handled by the at24 driver. It later retries the I2C
> > > > write command.
> > > > 
> > > > But if the the EEPROM signals a NAK and the I2C write command has more
> > > > than
> > > > 3 and less than 7 bytes then the error "PIO: Failed to finish WRITE
> > > > cmd!" appears, because the loop is not exited due to the NAK.
> > > 
> > > Ok, so we check
> > > 
> > > HW_I2C_STAT :: GOT_A_NAK bit at the end of the loop and if it's set, we
> > > "goto cleanup" with ret = -ENXIO . How does it sound to you?
> > 
> > this sounds good to me.
> 
> Does it fix your EEPROM issue then ?

yes, this fix solves the EEPROM issue. I have tested it.

Best regards
Torsten Fleischer