[PATCH 1/3] i2c: mxs: always end a transfer with a proper STOP

[PATCH 1/3] i2c: mxs: always end a transfer with a proper STOP

[Lucas Stach]

Our transfers always start with the device address, so there is never a
situation where we just do a restart transfer. Full blown transfers should
always end with a STOP as per i2c spec.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |   32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 120f246..f9704b2 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -87,10 +87,12 @@
 				 MXS_I2C_CTRL0_XFER_COUNT(1))
 
 #define MXS_CMD_I2C_WRITE	(MXS_I2C_CTRL0_PRE_SEND_START |	\
+				 MXS_I2C_CTRL0_POST_SEND_STOP | \
 				 MXS_I2C_CTRL0_MASTER_MODE |	\
 				 MXS_I2C_CTRL0_DIRECTION)
 
 #define MXS_CMD_I2C_READ	(MXS_I2C_CTRL0_SEND_NAK_ON_LAST | \
+				 MXS_I2C_CTRL0_POST_SEND_STOP | \
 				 MXS_I2C_CTRL0_MASTER_MODE)
 
 /**
@@ -158,8 +160,7 @@ static void mxs_i2c_dma_irq_callback(void *param)
 	mxs_i2c_dma_finish(i2c);
 }
 
-static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
-			struct i2c_msg *msg, uint32_t flags)
+static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 {
 	struct dma_async_tx_descriptor *desc;
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
@@ -200,7 +201,7 @@ static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
 		 */
 
 		/* Queue the PIO register write transfer. */
-		i2c->pio_data[1] = flags | MXS_CMD_I2C_READ |
+		i2c->pio_data[1] = MXS_CMD_I2C_READ |
 				MXS_I2C_CTRL0_XFER_COUNT(msg->len);
 		desc = dmaengine_prep_slave_sg(i2c->dmach,
 					(struct scatterlist *)&i2c->pio_data[1],
@@ -231,7 +232,7 @@ static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
 		 */
 
 		/* Queue the PIO register write transfer. */
-		i2c->pio_data[0] = flags | MXS_CMD_I2C_WRITE |
+		i2c->pio_data[0] = MXS_CMD_I2C_WRITE |
 				MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1);
 		desc = dmaengine_prep_slave_sg(i2c->dmach,
 					(struct scatterlist *)&i2c->pio_data[0],
@@ -326,8 +327,7 @@ static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
 	return 0;
 }
 
-static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
-			struct i2c_msg *msg, uint32_t flags)
+static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 {
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
 	uint32_t addr_data = msg->addr << 1;
@@ -355,7 +355,7 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 			return ret;
 
 		/* READ command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ | flags |
+		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ |
 			MXS_I2C_CTRL0_XFER_COUNT(msg->len),
 			i2c->regs + MXS_I2C_CTRL0);
 
@@ -373,7 +373,7 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 		addr_data |= I2C_SMBUS_WRITE;
 
 		/* WRITE command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_WRITE | flags |
+		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_WRITE |
 			MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1),
 			i2c->regs + MXS_I2C_CTRL0);
 
@@ -418,17 +418,13 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 /*
  * Low level master read/write transaction.
  */
-static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
-				int stop)
+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;
 
-	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
-
-	dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
-		msg->addr, msg->len, msg->flags, stop);
+	dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x\n",
+		msg->addr, msg->len, msg->flags);
 
 	if (msg->len == 0)
 		return -EINVAL;
@@ -440,13 +436,13 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	 * based on this empirical measurement and a lot of previous frobbing.
 	 */
 	if (msg->len < 8) {
-		ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
+		ret = mxs_i2c_pio_setup_xfer(adap, msg);
 		if (ret)
 			mxs_i2c_reset(i2c);
 	} else {
 		i2c->cmd_err = 0;
 		INIT_COMPLETION(i2c->cmd_complete);
-		ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
+		ret = mxs_i2c_dma_setup_xfer(adap, msg);
 		if (ret)
 			return ret;
 
@@ -479,7 +475,7 @@ static int mxs_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 	int err;
 
 	for (i = 0; i < num; i++) {
-		err = mxs_i2c_xfer_msg(adap, &msgs[i], i == (num - 1));
+		err = mxs_i2c_xfer_msg(adap, &msgs[i]);
 		if (err)
 			return err;
 	}
-- 
1.7.10.4

[PATCH 2/3] i2c: mxs: remove races in PIO code

[Lucas Stach]

This commit fixes the three following races in PIO code:

- The CTRL0 register is racy in itself, when programming transfer state and
  run bit in the same cycle the hardware sometimes ends up using the state
  from the last transfer. Fix this by programming state in one cycle, make
  sure the write is flushed down APBX bus by reading back the reg and only
  then trigger the run bit.

- Only clear the DMAREQ bit in DEBUG0 after the read/write to the data reg
  happened. Otherwise we are racing with the hardware about who touches
  the data reg first.

- When checking for completion of a transfer it's not sufficient to check
  if the data engine finished, but also a check for i2c bus idle is needed.
  In PIO mode we are really fast to program the next transfer after a finished
  one, so the controller possibly tries to start a new transfer while the
  clkgen engine is still busy writing the NAK/STOP from the last transfer to
  the bus.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |   56 ++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index f9704b2..e8f07dc 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -65,6 +65,10 @@
 #define MXS_I2C_CTRL1_SLAVE_STOP_IRQ		0x02
 #define MXS_I2C_CTRL1_SLAVE_IRQ			0x01
 
+#define MXS_I2C_STAT		(0x50)
+#define MXS_I2C_STAT_BUS_BUSY			0x00000800
+#define MXS_I2C_STAT_CLK_GEN_BUSY		0x00000400
+
 #define MXS_I2C_DATA		(0xa0)
 
 #define MXS_I2C_DEBUG0		(0xb0)
@@ -298,12 +302,10 @@ static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c)
 		cond_resched();
 	}
 
-	writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR);
-
 	return 0;
 }
 
-static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
+static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 
@@ -324,9 +326,33 @@ static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
 	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))) {
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
+		cond_resched();
+	}
+
 	return 0;
 }
 
+static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd)
+{
+	u32 reg;
+
+	writel(cmd, i2c->regs + MXS_I2C_CTRL0);
+
+	/* readback makes sure the write is latched into hardware */
+	reg = readl(i2c->regs + MXS_I2C_CTRL0);
+	reg |= MXS_I2C_CTRL0_RUN;
+	writel(reg, i2c->regs + MXS_I2C_CTRL0);
+}
+
 static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 {
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
@@ -341,23 +367,22 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 		addr_data |= I2C_SMBUS_READ;
 
 		/* SELECT command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_SELECT,
-			i2c->regs + MXS_I2C_CTRL0);
+		mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_SELECT);
 
 		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);
+		ret = mxs_i2c_pio_wait_cplt(i2c, 0);
 		if (ret)
 			return ret;
 
 		/* READ command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ |
-			MXS_I2C_CTRL0_XFER_COUNT(msg->len),
-			i2c->regs + MXS_I2C_CTRL0);
+		mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_READ |
+					MXS_I2C_CTRL0_XFER_COUNT(msg->len));
 
 		for (i = 0; i < msg->len; i++) {
 			if ((i & 3) == 0) {
@@ -365,6 +390,8 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 				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;
@@ -373,9 +400,8 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 		addr_data |= I2C_SMBUS_WRITE;
 
 		/* WRITE command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_WRITE |
-			MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1),
-			i2c->regs + MXS_I2C_CTRL0);
+		mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_WRITE |
+					MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1));
 
 		/*
 		 * The LSB of data buffer is the first byte blasted across
@@ -391,6 +417,8 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 				if (ret)
 					return ret;
 				writel(data, i2c->regs + MXS_I2C_DATA);
+				writel(MXS_I2C_DEBUG0_DMAREQ,
+				       i2c->regs + MXS_I2C_DEBUG0_CLR);
 			}
 		}
 
@@ -401,10 +429,12 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 			if (ret)
 				return ret;
 			writel(data, i2c->regs + MXS_I2C_DATA);
+			writel(MXS_I2C_DEBUG0_DMAREQ,
+			       i2c->regs + MXS_I2C_DEBUG0_CLR);
 		}
 	}
 
-	ret = mxs_i2c_pio_wait_cplt(i2c);
+	ret = mxs_i2c_pio_wait_cplt(i2c, 1);
 	if (ret)
 		return ret;
 
-- 
1.7.10.4

[PATCH 3/3] i2c: mxs: do error checking and handling in PIO mode

[Lucas Stach]

In PIO mode we can end up with the same errors as in DMA mode, but as IRQs
are disabled there we have to check for them manually after each command.

Also don't use the big controller reset hammer when receiving a NAK from a
slave. It's sufficient to tell the controller to continue at a clean state.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c |   41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index e8f07dc..c982670 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -56,6 +56,7 @@
 #define MXS_I2C_CTRL1_SET	(0x44)
 #define MXS_I2C_CTRL1_CLR	(0x48)
 
+#define MXS_I2C_CTRL1_CLR_GOT_A_NAK		0x10000000
 #define MXS_I2C_CTRL1_BUS_FREE_IRQ		0x80
 #define MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ	0x40
 #define MXS_I2C_CTRL1_NO_SLAVE_ACK_IRQ		0x20
@@ -341,6 +342,23 @@ static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last)
 	return 0;
 }
 
+static int mxs_i2c_pio_check_error_state(struct mxs_i2c_dev *i2c)
+{
+	u32 state;
+
+	state = readl(i2c->regs + MXS_I2C_CTRL1_CLR) & MXS_I2C_IRQ_MASK;
+
+	if (state & MXS_I2C_CTRL1_NO_SLAVE_ACK_IRQ)
+		i2c->cmd_err = -ENXIO;
+	else if (state & (MXS_I2C_CTRL1_EARLY_TERM_IRQ |
+			  MXS_I2C_CTRL1_MASTER_LOSS_IRQ |
+			  MXS_I2C_CTRL1_SLAVE_STOP_IRQ |
+			  MXS_I2C_CTRL1_SLAVE_IRQ))
+		i2c->cmd_err = -EIO;
+
+	return i2c->cmd_err;
+}
+
 static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd)
 {
 	u32 reg;
@@ -380,6 +398,9 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 		if (ret)
 			return ret;
 
+		if (mxs_i2c_pio_check_error_state(i2c))
+			goto cleanup;
+
 		/* READ command. */
 		mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_READ |
 					MXS_I2C_CTRL0_XFER_COUNT(msg->len));
@@ -438,6 +459,10 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 	if (ret)
 		return ret;
 
+	/* make sure we capture any occurred error into cmd_err */
+	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);
@@ -465,12 +490,12 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg)
 	 * using PIO mode while longer transfers use DMA. The 8 byte border is
 	 * based on this empirical measurement and a lot of previous frobbing.
 	 */
+	i2c->cmd_err = 0;
 	if (msg->len < 8) {
 		ret = mxs_i2c_pio_setup_xfer(adap, msg);
 		if (ret)
 			mxs_i2c_reset(i2c);
 	} else {
-		i2c->cmd_err = 0;
 		INIT_COMPLETION(i2c->cmd_complete);
 		ret = mxs_i2c_dma_setup_xfer(adap, msg);
 		if (ret)
@@ -480,13 +505,19 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg)
 						msecs_to_jiffies(1000));
 		if (ret == 0)
 			goto timeout;
+	}
 
-		if (i2c->cmd_err == -ENXIO)
-			mxs_i2c_reset(i2c);
-
-		ret = i2c->cmd_err;
+	if (i2c->cmd_err == -ENXIO) {
+		/*
+		 * If the transfer fails with a NAK from the slave the
+		 * controller halts until it gets told to return to idle state.
+		 */
+		writel(MXS_I2C_CTRL1_CLR_GOT_A_NAK,
+		       i2c->regs + MXS_I2C_CTRL1_SET);
 	}
 
+	ret = i2c->cmd_err;
+
 	dev_dbg(i2c->dev, "Done with err=%d\n", ret);
 
 	return ret;
-- 
1.7.10.4

Re: [PATCH 2/3] i2c: mxs: remove races in PIO code

[Marek Vasut]

Dear Lucas Stach,

sorry for the delay, I'm totalled ... well, trying to make it through the queue 
now.

CC Fabio on MXS patches please.

[...]

> -static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
> +static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last)

You can use bool instead of int here and then use "true"/"false" below.

Best regards,
Marek Vasut

Re: [PATCH 3/3] i2c: mxs: do error checking and handling in PIO mode

[Marek Vasut]

Dear Lucas Stach,

> In PIO mode we can end up with the same errors as in DMA mode, but as IRQs
> are disabled there we have to check for them manually after each command.
> 
> Also don't use the big controller reset hammer when receiving a NAK from a
> slave. It's sufficient to tell the controller to continue at a clean state.
> 
> Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Stamp it with:

Tested-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

Good job, thanks!

Best regards,
Marek Vasut

Re: [PATCH 3/3] i2c: mxs: do error checking and handling in PIO mode

[Wolfram Sang]

On Tue, Apr 02, 2013 at 12:59:22AM +0200, Marek Vasut wrote:
> Dear Lucas Stach,
> 
> > In PIO mode we can end up with the same errors as in DMA mode, but as IRQs
> > are disabled there we have to check for them manually after each command.
> > 
> > Also don't use the big controller reset hammer when receiving a NAK from a
> > slave. It's sufficient to tell the controller to continue at a clean state.
> > 
> > Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> Stamp it with:
> 
> Tested-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

Did you test only this patch or all of them?

Re: [PATCH 1/3] i2c: mxs: always end a transfer with a proper STOP

[Wolfram Sang]

On Thu, Mar 14, 2013 at 12:49:08PM +0100, Lucas Stach wrote:
> Our transfers always start with the device address, so there is never a
> situation where we just do a restart transfer. Full blown transfers should
> always end with a STOP as per i2c spec.

? I don't get the description. Does "restart transfer" mean repeated
start? What has the device address to do with it?

Re: [PATCH 3/3] i2c: mxs: do error checking and handling in PIO mode

[Marek Vasut]

Dear Wolfram Sang,

> On Tue, Apr 02, 2013 at 12:59:22AM +0200, Marek Vasut wrote:
> > Dear Lucas Stach,
> > 
> > > In PIO mode we can end up with the same errors as in DMA mode, but as
> > > IRQs are disabled there we have to check for them manually after each
> > > command.
> > > 
> > > Also don't use the big controller reset hammer when receiving a NAK
> > > from a slave. It's sufficient to tell the controller to continue at a
> > > clean state.
> > > 
> > > Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > 
> > Stamp it with:
> > 
> > Tested-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> 
> Did you test only this patch or all of them?

Whole set.

Best regards,
Marek Vasut

Re: [PATCH 1/3] i2c: mxs: always end a transfer with a proper STOP

[Lucas Stach]

Am Montag, den 08.04.2013, 19:21 +0200 schrieb Wolfram Sang:
> On Thu, Mar 14, 2013 at 12:49:08PM +0100, Lucas Stach wrote:
> > Our transfers always start with the device address, so there is never a
> > situation where we just do a restart transfer. Full blown transfers should
> > always end with a STOP as per i2c spec.
> 
> ? I don't get the description. Does "restart transfer" mean repeated
> start? What has the device address to do with it?
> 

A restart transfer is when you just repeat the START condition, without
putting the device address on the bus again.

In the MXS driver we put the device address on the bus for every
transaction we get handed in from the i2c core, so there is never a
situation where we just repeat the start condition without sending out
the device address. Before this patch we would not match every
transaction, but only the last in the list of pending ones, with a STOP
condition, which is a violation of the spec.

If there are no other comments, I'll send out a V2 today, to take in
Marek's remarks.
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] i2c: mxs: always end a transfer with a proper STOP

[Wolfram Sang]

Hi,

> A restart transfer is when you just repeat the START condition, without
> putting the device address on the bus again.

Well, never heard this term before. Where did you get it from?

> In the MXS driver we put the device address on the bus for every
> transaction we get handed in from the i2c core, so there is never a
> situation where we just repeat the start condition without sending out
> the device address. Before this patch we would not match every
> transaction, but only the last in the list of pending ones, with a STOP
> condition, which is a violation of the spec.

I still don't get it. You can drop a STOP if you replace it with
a repeated start. In fact, this is crucial in multi-master setups,
otherwise another master could break into your transfer containing
multilple messages. So, if MXS does the right thing on sending START
(doing a correct start sequence), we should not send STOP. If it needs
the STOP to create a correct START, then be it. But then, I'd wonder why
it worked so far...

Regards,

   Wolfram

Re: [PATCH 1/3] i2c: mxs: always end a transfer with a proper STOP

[Lucas Stach]

Hi Wolfram,

Am Dienstag, den 09.04.2013, 10:32 +0200 schrieb Wolfram Sang:
> Hi,
> 
> > A restart transfer is when you just repeat the START condition, without
> > putting the device address on the bus again.
> 
> Well, never heard this term before. Where did you get it from?
> 
> > In the MXS driver we put the device address on the bus for every
> > transaction we get handed in from the i2c core, so there is never a
> > situation where we just repeat the start condition without sending out
> > the device address. Before this patch we would not match every
> > transaction, but only the last in the list of pending ones, with a STOP
> > condition, which is a violation of the spec.
> 
> I still don't get it. You can drop a STOP if you replace it with
> a repeated start. In fact, this is crucial in multi-master setups,
> otherwise another master could break into your transfer containing
> multilple messages. So, if MXS does the right thing on sending START
> (doing a correct start sequence), we should not send STOP. If it needs
> the STOP to create a correct START, then be it. But then, I'd wonder why
> it worked so far...
> 

Ok, I looked this up again and got a nice explanation by Uwe and it
seems I based this patch on a wrong interpretation of the spec on my
side. I'll resend without this one.

Regards,
Lucas

-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 1/2] i2c: mxs: remove races in PIO code

[Lucas Stach]

This commit fixes the three following races in PIO code:

- The CTRL0 register is racy in itself, when programming transfer state and
  run bit in the same cycle the hardware sometimes ends up using the state
  from the last transfer. Fix this by programming state in one cycle, make
  sure the write is flushed down APBX bus by reading back the reg and only
  then trigger the run bit.

- Only clear the DMAREQ bit in DEBUG0 after the read/write to the data reg
  happened. Otherwise we are racing with the hardware about who touches
  the data reg first.

- When checking for completion of a transfer it's not sufficient to check
  if the data engine finished, but also a check for i2c bus idle is needed.
  In PIO mode we are really fast to program the next transfer after a finished
  one, so the controller possibly tries to start a new transfer while the
  clkgen engine is still busy writing the NAK/STOP from the last transfer to
  the bus.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c | 58 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 120f246..f8558550 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -65,6 +65,10 @@
 #define MXS_I2C_CTRL1_SLAVE_STOP_IRQ		0x02
 #define MXS_I2C_CTRL1_SLAVE_IRQ			0x01
 
+#define MXS_I2C_STAT		(0x50)
+#define MXS_I2C_STAT_BUS_BUSY			0x00000800
+#define MXS_I2C_STAT_CLK_GEN_BUSY		0x00000400
+
 #define MXS_I2C_DATA		(0xa0)
 
 #define MXS_I2C_DEBUG0		(0xb0)
@@ -297,12 +301,10 @@ static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c)
 		cond_resched();
 	}
 
-	writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR);
-
 	return 0;
 }
 
-static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
+static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 
@@ -323,9 +325,33 @@ static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
 	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))) {
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
+		cond_resched();
+	}
+
 	return 0;
 }
 
+static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd)
+{
+	u32 reg;
+
+	writel(cmd, i2c->regs + MXS_I2C_CTRL0);
+
+	/* readback makes sure the write is latched into hardware */
+	reg = readl(i2c->regs + MXS_I2C_CTRL0);
+	reg |= MXS_I2C_CTRL0_RUN;
+	writel(reg, i2c->regs + MXS_I2C_CTRL0);
+}
+
 static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 			struct i2c_msg *msg, uint32_t flags)
 {
@@ -341,23 +367,23 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 		addr_data |= I2C_SMBUS_READ;
 
 		/* SELECT command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_SELECT,
-			i2c->regs + MXS_I2C_CTRL0);
+		mxs_i2c_pio_trigger_cmd(i2c, MXS_CMD_I2C_SELECT);
 
 		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);
+		ret = mxs_i2c_pio_wait_cplt(i2c, 0);
 		if (ret)
 			return ret;
 
 		/* READ command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ | flags |
-			MXS_I2C_CTRL0_XFER_COUNT(msg->len),
-			i2c->regs + MXS_I2C_CTRL0);
+		mxs_i2c_pio_trigger_cmd(i2c,
+					MXS_CMD_I2C_READ | flags |
+					MXS_I2C_CTRL0_XFER_COUNT(msg->len));
 
 		for (i = 0; i < msg->len; i++) {
 			if ((i & 3) == 0) {
@@ -365,6 +391,8 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 				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;
@@ -373,9 +401,9 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 		addr_data |= I2C_SMBUS_WRITE;
 
 		/* WRITE command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_WRITE | flags |
-			MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1),
-			i2c->regs + MXS_I2C_CTRL0);
+		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
@@ -391,6 +419,8 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 				if (ret)
 					return ret;
 				writel(data, i2c->regs + MXS_I2C_DATA);
+				writel(MXS_I2C_DEBUG0_DMAREQ,
+				       i2c->regs + MXS_I2C_DEBUG0_CLR);
 			}
 		}
 
@@ -401,10 +431,12 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 			if (ret)
 				return ret;
 			writel(data, i2c->regs + MXS_I2C_DATA);
+			writel(MXS_I2C_DEBUG0_DMAREQ,
+			       i2c->regs + MXS_I2C_DEBUG0_CLR);
 		}
 	}
 
-	ret = mxs_i2c_pio_wait_cplt(i2c);
+	ret = mxs_i2c_pio_wait_cplt(i2c, flags & MXS_I2C_CTRL0_POST_SEND_STOP);
 	if (ret)
 		return ret;
 
-- 
1.8.2.rc2

[PATCH v2 2/2] i2c: mxs: do error checking and handling in PIO mode

[Lucas Stach]

In PIO mode we can end up with the same errors as in DMA mode, but as IRQs
are disabled there we have to check for them manually after each command.

Also don't use the big controller reset hammer when receiving a NAK from a
slave. It's sufficient to tell the controller to continue at a clean state.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Tested-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
---
 drivers/i2c/busses/i2c-mxs.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index f8558550..bf8b04e 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -56,6 +56,7 @@
 #define MXS_I2C_CTRL1_SET	(0x44)
 #define MXS_I2C_CTRL1_CLR	(0x48)
 
+#define MXS_I2C_CTRL1_CLR_GOT_A_NAK		0x10000000
 #define MXS_I2C_CTRL1_BUS_FREE_IRQ		0x80
 #define MXS_I2C_CTRL1_DATA_ENGINE_CMPLT_IRQ	0x40
 #define MXS_I2C_CTRL1_NO_SLAVE_ACK_IRQ		0x20
@@ -340,6 +341,23 @@ static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c, int last)
 	return 0;
 }
 
+static int mxs_i2c_pio_check_error_state(struct mxs_i2c_dev *i2c)
+{
+	u32 state;
+
+	state = readl(i2c->regs + MXS_I2C_CTRL1_CLR) & MXS_I2C_IRQ_MASK;
+
+	if (state & MXS_I2C_CTRL1_NO_SLAVE_ACK_IRQ)
+		i2c->cmd_err = -ENXIO;
+	else if (state & (MXS_I2C_CTRL1_EARLY_TERM_IRQ |
+			  MXS_I2C_CTRL1_MASTER_LOSS_IRQ |
+			  MXS_I2C_CTRL1_SLAVE_STOP_IRQ |
+			  MXS_I2C_CTRL1_SLAVE_IRQ))
+		i2c->cmd_err = -EIO;
+
+	return i2c->cmd_err;
+}
+
 static void mxs_i2c_pio_trigger_cmd(struct mxs_i2c_dev *i2c, u32 cmd)
 {
 	u32 reg;
@@ -380,6 +398,9 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 		if (ret)
 			return ret;
 
+		if (mxs_i2c_pio_check_error_state(i2c))
+			goto cleanup;
+
 		/* READ command. */
 		mxs_i2c_pio_trigger_cmd(i2c,
 					MXS_CMD_I2C_READ | flags |
@@ -440,6 +461,10 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 	if (ret)
 		return ret;
 
+	/* make sure we capture any occurred error into cmd_err */
+	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);
@@ -471,12 +496,12 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	 * using PIO mode while longer transfers use DMA. The 8 byte border is
 	 * based on this empirical measurement and a lot of previous frobbing.
 	 */
+	i2c->cmd_err = 0;
 	if (msg->len < 8) {
 		ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
 		if (ret)
 			mxs_i2c_reset(i2c);
 	} else {
-		i2c->cmd_err = 0;
 		INIT_COMPLETION(i2c->cmd_complete);
 		ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
 		if (ret)
@@ -486,13 +511,19 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 						msecs_to_jiffies(1000));
 		if (ret == 0)
 			goto timeout;
+	}
 
-		if (i2c->cmd_err == -ENXIO)
-			mxs_i2c_reset(i2c);
-
-		ret = i2c->cmd_err;
+	if (i2c->cmd_err == -ENXIO) {
+		/*
+		 * If the transfer fails with a NAK from the slave the
+		 * controller halts until it gets told to return to idle state.
+		 */
+		writel(MXS_I2C_CTRL1_CLR_GOT_A_NAK,
+		       i2c->regs + MXS_I2C_CTRL1_SET);
 	}
 
+	ret = i2c->cmd_err;
+
 	dev_dbg(i2c->dev, "Done with err=%d\n", ret);
 
 	return ret;
-- 
1.8.2.rc2

Re: [PATCH v2 1/2] i2c: mxs: remove races in PIO code

[Wolfram Sang]

On Mon, Apr 15, 2013 at 12:16:54PM +0200, Lucas Stach wrote:
> This commit fixes the three following races in PIO code:
> 
> - The CTRL0 register is racy in itself, when programming transfer state and
>   run bit in the same cycle the hardware sometimes ends up using the state
>   from the last transfer. Fix this by programming state in one cycle, make
>   sure the write is flushed down APBX bus by reading back the reg and only
>   then trigger the run bit.
> 
> - Only clear the DMAREQ bit in DEBUG0 after the read/write to the data reg
>   happened. Otherwise we are racing with the hardware about who touches
>   the data reg first.
> 
> - When checking for completion of a transfer it's not sufficient to check
>   if the data engine finished, but also a check for i2c bus idle is needed.
>   In PIO mode we are really fast to program the next transfer after a finished
>   one, so the controller possibly tries to start a new transfer while the
>   clkgen engine is still busy writing the NAK/STOP from the last transfer to
>   the bus.
> 
> Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Applied to for-next, thanks! Both.

I'd prefer to not use reply-to <old_series> when sending <new_series>.