[PATCH 0/2] I2C: OMAP: spurious IRQ fixes

[PATCH 0/2] I2C: OMAP: spurious IRQ fixes

[Paul Walmsley]

Hi,

This is a set of I2C fixes.  Its primary purpose is to deal with the
I2C spurious IRQ problem:

Spurious irq 95: 0xffffffdf, please flush posted write for irq 56

It seems to work: under constant I2C load, no spurious IRQ messages
showed up after several hours of testing.  (Without these patches,
spurious IRQs usually show up in a few minutes.)  Some of the code has
also been cleaned up.

Any feedback on how this series works for others is appreciated.

Tested on N800, Beagle and 3430SDP.


- Paul

---

  text    data     bss     dec     hex filename
3636832  210560  104320 3951712  3c4c60 vmlinux.3430sdp.orig
3636896  210560  104320 3951776  3c4ca0 vmlinux.3430sdp


Paul Walmsley (2):
      I2C: OMAP: overhaul the rev2+ interrupt service routine and omap_i2c_xfer_msg()
      I2C: OMAP: use consolidated flags field rather than bitfields


 drivers/i2c/busses/i2c-omap.c |  150 ++++++++++++++++++++++++++---------------
 1 files changed, 95 insertions(+), 55 deletions(-)

[PATCH 1/2] I2C: OMAP: use consolidated flags field rather than bitfields

[Paul Walmsley]

Convert existing flag usage in the i2c-omap.c driver to use a 'flags' u8
rather than bitfields.  Define static inline functions to set and clear
these flags.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 drivers/i2c/busses/i2c-omap.c |   51 +++++++++++++++++++++++++++--------------
 1 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 9919c08..387a2d3 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -26,6 +26,9 @@
  * You should have received a copy of the GNU General Public License
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * To do:
+ * - Add DMA support for large reads/writes
  */
 
 #include <linux/module.h>
@@ -157,6 +160,10 @@
 #define SYSC_CLOCKACTIVITY_FCLK		0x2
 
 
+/* omap_i2c_dev.flags */
+#define OMAP_I2C_BAD_HW			(1 << 0)  /* bad hardware */
+#define OMAP_I2C_IDLE			(1 << 1)  /* device clocks off */
+
 struct omap_i2c_dev {
 	struct device		*dev;
 	void __iomem		*base;		/* virtual */
@@ -166,18 +173,17 @@ struct omap_i2c_dev {
 	struct completion	cmd_complete;
 	struct resource		*ioarea;
 	u32			speed;		/* Speed of bus in Khz */
-	u16			cmd_err;
 	u8			*buf;
 	size_t			buf_len;
 	struct i2c_adapter	adapter;
+	u16			cmd_err;
+	u16			iestate;	/* Saved interrupt register */
 	u8			fifo_size;	/* use as flag and value
 						 * fifo_size==0 implies no fifo
 						 * if set, should be trsh+1
 						 */
 	u8			rev;
-	unsigned		b_hw:1;		/* bad h/w fixes */
-	unsigned		idle:1;
-	u16			iestate;	/* Saved interrupt register */
+	u8			flags;
 };
 
 static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
@@ -191,6 +197,17 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
 	return __raw_readw(i2c_dev->base + reg);
 }
 
+static inline void omap_i2c_set_flag(struct omap_i2c_dev *i2c_dev, u8 flag_mask)
+{
+	i2c_dev->flags |= flag_mask;
+}
+
+static inline void omap_i2c_clear_flag(struct omap_i2c_dev *i2c_dev,
+				       u8 flag_mask)
+{
+	i2c_dev->flags &= ~flag_mask;
+}
+
 static int __init omap_i2c_get_clocks(struct omap_i2c_dev *dev)
 {
 	int ret;
@@ -226,32 +243,30 @@ static void omap_i2c_put_clocks(struct omap_i2c_dev *dev)
 
 static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 {
-	WARN_ON(!dev->idle);
+	WARN_ON(!(dev->flags & OMAP_I2C_IDLE));
 
 	clk_enable(dev->iclk);
 	clk_enable(dev->fclk);
-	dev->idle = 0;
+	omap_i2c_clear_flag(dev, OMAP_I2C_IDLE);
 	if (dev->iestate)
 		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
 }
 
 static void omap_i2c_idle(struct omap_i2c_dev *dev)
 {
-	u16 iv;
-
-	WARN_ON(dev->idle);
+	WARN_ON(dev->flags & OMAP_I2C_IDLE);
 
 	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
 	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
 	if (dev->rev < OMAP_I2C_REV_2) {
-		iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG); /* Read clears */
+		omap_i2c_read_reg(dev, OMAP_I2C_IV_REG); /* Read clears */
 	} else {
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
 
-		/* Flush posted write before the dev->idle store occurs */
+		/* Flush posted write before setting the idle flag */
 		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 	}
-	dev->idle = 1;
+	omap_i2c_set_flag(dev, OMAP_I2C_IDLE);
 	clk_disable(dev->fclk);
 	clk_disable(dev->iclk);
 }
@@ -455,7 +470,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	if (!(msg->flags & I2C_M_RD))
 		w |= OMAP_I2C_CON_TRX;
 
-	if (!dev->b_hw && stop)
+	if (!(dev->flags & OMAP_I2C_BAD_HW) && stop)
 		w |= OMAP_I2C_CON_STP;
 
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
@@ -463,7 +478,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	/*
 	 * Don't write stt and stp together on some hardware.
 	 */
-	if (dev->b_hw && stop) {
+	if ((dev->flags & OMAP_I2C_BAD_HW) && stop) {
 		unsigned long delay = jiffies + OMAP_I2C_TIMEOUT;
 		u16 con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
 		while (con & OMAP_I2C_CON_STT) {
@@ -580,7 +595,7 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 iv, w;
 
-	if (dev->idle)
+	if (dev->flags & OMAP_I2C_IDLE)
 		return IRQ_NONE;
 
 	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
@@ -640,7 +655,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	u16 stat, w;
 	int err, count = 0;
 
-	if (dev->idle)
+	if (dev->flags & OMAP_I2C_IDLE)
 		return IRQ_NONE;
 
 	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -806,7 +821,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		speed = 100;	/* Defualt speed */
 
 	dev->speed = speed;
-	dev->idle = 1;
+	omap_i2c_set_flag(dev, OMAP_I2C_IDLE);
 	dev->dev = &pdev->dev;
 	dev->irq = irq->start;
 	dev->base = ioremap(mem->start, mem->end - mem->start + 1);
@@ -837,7 +852,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		 * call back latencies.
 		 */
 		dev->fifo_size = (dev->fifo_size / 2);
-		dev->b_hw = 1; /* Enable hardware fixes */
+		omap_i2c_set_flag(dev, OMAP_I2C_BAD_HW);  /* Enable hw fixes */
 	}
 
 	/* reset ASAP, clearing any IRQs */

[PATCH 2/2] I2C: OMAP: overhaul the rev2+ interrupt service routine and omap_i2c_xfer_msg()

[Paul Walmsley]

High I2C transmit activity can cause spurious IRQs.  This patch
restructures the i2c-omap.c interrupt service routine to reduce these
and to clean up the driver.

Correctness:

- Move the test for receive FIFO overflows and transmit FIFO underflows
  up to the top of the ISR.  This ensures that they will be caught
  even if RRDY, RDR, XRDY, XDR interrupts occur.

- Ignore the transmit FIFO underflow flag if it occurs before we've
  written any data into the FIFO, since we expect it to occur then.
  (OMAP_I2C_XUDF_EXPECTED flag)

- Remove double-clears of the interrupt status register via
  omap_i2c_ack_stat().

- When starting I2C activity, flush posted writes to clear the I2C
  FIFOs and to start/stop the I2C bus

- Clear the interrupt status register _after_ all of the interrupt
  events have been processed, not before.

Clean up:

- Remove an unnecessary device register read in the ISR by using the
  dev->iestate cached register.

With this patch applied, I was unable to trigger any I2C spurious IRQs
after a couple of hours of continuous transmission on I2C1.  It's hard to
say with complete certainty that all the spurious interrupts are gone.
Without RTL access, it's hard to be sure of the correct state machine
sequence.  The IP block may be so bugged that we may have to add a
readback after each __raw_writew().

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Ari Kauppi <ext-ari.kauppi@nokia.com>
Cc: Eero Nurkkala <ext-eero.nurkkala@nokia.com>
---
 drivers/i2c/busses/i2c-omap.c |  101 ++++++++++++++++++++++++++---------------
 1 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 387a2d3..9f14040 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -2,7 +2,7 @@
  * TI OMAP I2C master mode driver
  *
  * Copyright (C) 2003 MontaVista Software, Inc.
- * Copyright (C) 2005 Nokia Corporation
+ * Copyright (C) 2005, 2009 Nokia Corporation
  * Copyright (C) 2004 - 2007 Texas Instruments.
  *
  * Originally written by MontaVista Software, Inc.
@@ -12,6 +12,7 @@
  *	Juha Yrjölä <juha.yrjola@solidboot.com>
  *	Syed Khasim <x0khasim@ti.com>
  *	Nishant Menon <nm@ti.com>
+ *	Paul Walmsley <paul@pwsan.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -159,10 +160,10 @@
 #define SYSC_IDLEMODE_SMART		0x2
 #define SYSC_CLOCKACTIVITY_FCLK		0x2
 
-
 /* omap_i2c_dev.flags */
 #define OMAP_I2C_BAD_HW			(1 << 0)  /* bad hardware */
 #define OMAP_I2C_IDLE			(1 << 1)  /* device clocks off */
+#define OMAP_I2C_XUDF_EXPECTED		(1 << 2)  /* no data xmitted yet */
 
 struct omap_i2c_dev {
 	struct device		*dev;
@@ -197,6 +198,18 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
 	return __raw_readw(i2c_dev->base + reg);
 }
 
+static inline void omap_i2c_posted_write_barrier(struct omap_i2c_dev *i2c_dev)
+{
+	omap_i2c_read_reg(i2c_dev, OMAP_I2C_SYSC_REG);
+}
+
+static inline void omap_i2c_write_nonposted_reg(struct omap_i2c_dev *i2c_dev,
+						int reg, u16 val)
+{
+	omap_i2c_write_reg(i2c_dev, reg, val);
+	omap_i2c_posted_write_barrier(i2c_dev);
+}
+
 static inline void omap_i2c_set_flag(struct omap_i2c_dev *i2c_dev, u8 flag_mask)
 {
 	i2c_dev->flags |= flag_mask;
@@ -261,10 +274,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	if (dev->rev < OMAP_I2C_REV_2) {
 		omap_i2c_read_reg(dev, OMAP_I2C_IV_REG); /* Read clears */
 	} else {
-		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
-
 		/* Flush posted write before setting the idle flag */
-		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+		omap_i2c_write_nonposted_reg(dev, OMAP_I2C_STAT_REG,
+					     dev->iestate);
 	}
 	omap_i2c_set_flag(dev, OMAP_I2C_IDLE);
 	clk_disable(dev->fclk);
@@ -400,11 +412,12 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
 	/* Enable interrupts */
-	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG,
-			(OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
+	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
 			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
 			OMAP_I2C_IE_AL)  | ((dev->fifo_size) ?
-				(OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0));
+					    (OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0);
+	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+
 	return 0;
 }
 
@@ -454,11 +467,13 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	/* Clear the FIFO Buffers */
 	w = omap_i2c_read_reg(dev, OMAP_I2C_BUF_REG);
 	w |= OMAP_I2C_BUF_RXFIF_CLR | OMAP_I2C_BUF_TXFIF_CLR;
-	omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, w);
+	omap_i2c_write_nonposted_reg(dev, OMAP_I2C_BUF_REG, w);
 
 	init_completion(&dev->cmd_complete);
 	dev->cmd_err = 0;
 
+	omap_i2c_set_flag(dev, OMAP_I2C_XUDF_EXPECTED);
+
 	w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
 
 	/* High speed configuration */
@@ -480,6 +495,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	 */
 	if ((dev->flags & OMAP_I2C_BAD_HW) && stop) {
 		unsigned long delay = jiffies + OMAP_I2C_TIMEOUT;
+		/* The following also acts as a posted write barrier */
 		u16 con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
 		while (con & OMAP_I2C_CON_STT) {
 			con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
@@ -498,6 +514,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
 	}
 
+	omap_i2c_posted_write_barrier(dev);
+
 	/*
 	 * REVISIT: We should abort the transfer on signals, but the bus goes
 	 * into arbitration and we're currently unable to recover from it.
@@ -529,7 +547,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		if (stop) {
 			w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
 			w |= OMAP_I2C_CON_STP;
-			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
+			omap_i2c_write_nonposted_reg(dev, OMAP_I2C_CON_REG, w);
 		}
 		return -EREMOTEIO;
 	}
@@ -580,12 +598,6 @@ omap_i2c_complete_cmd(struct omap_i2c_dev *dev, u16 err)
 	complete(&dev->cmd_complete);
 }
 
-static inline void
-omap_i2c_ack_stat(struct omap_i2c_dev *dev, u16 stat)
-{
-	omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
-}
-
 /* rev1 devices are apparently only on some 15xx */
 #ifdef CONFIG_ARCH_OMAP15XX
 
@@ -651,26 +663,36 @@ static irqreturn_t
 omap_i2c_isr(int this_irq, void *dev_id)
 {
 	struct omap_i2c_dev *dev = dev_id;
-	u16 bits;
 	u16 stat, w;
 	int err, count = 0;
 
 	if (dev->flags & OMAP_I2C_IDLE)
 		return IRQ_NONE;
 
-	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
-	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
+	/* This also acts as a posted write barrier as the ISR loops */
+	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & dev->iestate) {
+
 		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
 		if (count++ == 100) {
 			dev_warn(dev->dev, "Too much work in one IRQ\n");
 			break;
 		}
 
-		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
-
 		err = 0;
+
+		if (stat & OMAP_I2C_STAT_ROVR) {
+			dev_err(dev->dev, "Receive overrun\n");
+			dev->cmd_err |= OMAP_I2C_STAT_ROVR;
+		}
+		if (stat & OMAP_I2C_STAT_XUDF &&
+		    !(dev->flags & OMAP_I2C_XUDF_EXPECTED)) {
+			dev_err(dev->dev, "Transmit underflow\n");
+			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
+		}
+
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
+			/* XXX should not be necessary per TRM */
 			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
 					   OMAP_I2C_CON_STP);
 		}
@@ -678,9 +700,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
 		}
-		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
-					OMAP_I2C_STAT_AL))
-			omap_i2c_complete_cmd(dev, err);
+
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {
@@ -717,9 +737,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 					break;
 				}
 			}
-			omap_i2c_ack_stat(dev,
-				stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
-			continue;
 		}
 		if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
 			u8 num_bytes = 1;
@@ -758,18 +775,26 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				}
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-			omap_i2c_ack_stat(dev,
-				stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-			continue;
-		}
-		if (stat & OMAP_I2C_STAT_ROVR) {
-			dev_err(dev->dev, "Receive overrun\n");
-			dev->cmd_err |= OMAP_I2C_STAT_ROVR;
-		}
-		if (stat & OMAP_I2C_STAT_XUDF) {
-			dev_err(dev->dev, "Transmit underflow\n");
-			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
+			omap_i2c_clear_flag(dev, OMAP_I2C_XUDF_EXPECTED);
+
+			/*
+			 * It seems that there must be some delay between
+			 * writing to the FIFO and clearing the interrupt
+			 * status register
+			 */
+			omap_i2c_posted_write_barrier(dev);
 		}
+
+		/*
+		 * Ensure the device interrupt clears before the
+		 * MPUINTC unmasks the IRQ line - avoiding spurious
+		 * IRQs
+		 */
+		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
+
+		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
+			    OMAP_I2C_STAT_AL))
+			omap_i2c_complete_cmd(dev, err);
 	}
 
 	return count ? IRQ_HANDLED : IRQ_NONE;


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] I2C: OMAP: spurious IRQ fixes

[Aaro Koskinen]

Hello,

Paul Walmsley wrote:
> It seems to work: under constant I2C load, no spurious IRQ messages
> showed up after several hours of testing.  (Without these patches,
> spurious IRQs usually show up in a few minutes.)  Some of the code has
> also been cleaned up.
> 
> Any feedback on how this series works for others is appreciated.
> 
> Tested on N800, Beagle and 3430SDP.

With these patches I'm getting receive overruns quite easily on rx51 when doing large reads from TWL4030 RTC.

A.

Re: [PATCH 0/2] I2C: OMAP: spurious IRQ fixes

[Paul Walmsley]

On Mon, 27 Apr 2009, Aaro Koskinen wrote:

> Paul Walmsley wrote:
> > It seems to work: under constant I2C load, no spurious IRQ messages
> > showed up after several hours of testing.  (Without these patches,
> > spurious IRQs usually show up in a few minutes.)  Some of the code has
> > also been cleaned up.
> > 
> > Any feedback on how this series works for others is appreciated.
> > 
> > Tested on N800, Beagle and 3430SDP.
> 
> With these patches I'm getting receive overruns quite easily on rx51 when
> doing large reads from TWL4030 RTC.

A brief update for anyone following along on the list: 

Aaro sent me a test-case.  It seems that the receive overruns only affect 
PM kernels.  They are caused by the MPU going to sleep while it waits for 
an I2C interrupt.  It turns out that the current driver causes receive 
overruns also; it just hides them.  The receive overruns are fairly 
straightforward to deal with by setting an MPU wakeup latency constraint 
during I2C operations.

So the plan now is to send two sets of patches.  One set will resolve the 
spurious IRQ problems.  The other will resolve the receive overrun 
issues and will be mostly against the PM branch.


- Paul

RE: [PATCH 0/2] I2C: OMAP: spurious IRQ fixes

[Woodruff, Richard]

> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Paul Walmsley
> A brief update for anyone following along on the list:
>
> Aaro sent me a test-case.  It seems that the receive overruns only affect
> PM kernels.  They are caused by the MPU going to sleep while it waits for
> an I2C interrupt.  It turns out that the current driver causes receive
> overruns also; it just hides them.  The receive overruns are fairly
> straightforward to deal with by setting an MPU wakeup latency constraint
> during I2C operations.

What kind of latency is being seen by i2c in kernel snapshot you are using?

Do you have an idea of what is tolerable given an I2C frequency? Is this error showing up in 100k/400k/HS mode?

Sometimes a constraint is the only way but it does have cost.

Regards,
Richard W.

RE: [PATCH 0/2] I2C: OMAP: spurious IRQ fixes

[Paul Walmsley]

On Tue, 12 May 2009, Woodruff, Richard wrote:

> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Paul Walmsley
> > A brief update for anyone following along on the list:
> >
> > Aaro sent me a test-case.  It seems that the receive overruns only affect
> > PM kernels.  They are caused by the MPU going to sleep while it waits for
> > an I2C interrupt.  It turns out that the current driver causes receive
> > overruns also; it just hides them.  The receive overruns are fairly
> > straightforward to deal with by setting an MPU wakeup latency constraint
> > during I2C operations.
> 
> What kind of latency is being seen by i2c in kernel snapshot you are using?
> Do you have an idea of what is tolerable given an I2C frequency?
> Is this error showing up in 100k/400k/HS mode?

The case that we observed was with I2C1 at 2.6MHz with the FIFO threshold 
at 4 bytes of an 8 byte FIFO, the driver default.

The maximum MPU wakeup latency to avoid overruns seems primarily a 
function of the I2C FIFO size, FIFO threshold, and I2C bus clock.  The 
OpenOffice spreadsheet used to compute this is here.  Comments and review
welcomed:

    http://www.pwsan.com/omap/i2c-wakeup-latency.ods

Crappy HTML version is here:

    http://www.pwsan.com/omap/i2c-wakeup-latency.html

It computed a 12 microsecond constraint for the above parameters.

> Sometimes a constraint is the only way but it does have cost.

Determining the actual cost of this constraint is an interesting problem.  
At first one would assume there would be a power consumption cost.  But it 
may be that allowing the MPU to go off while waiting for the interrupt 
consumes more power in the long run, since restarting takes time.

One thing that would help is if the Linux I2C interface had a way for 
applications to indicate that a transfer was not latency-sensitive. The 
cost of the constraint would probably depend on the I2C slave device's 
characteristics and the characteristics of the I2C application.

At least receive overruns are handled gracefully by the controller. 
Transmit FIFO underruns may be the more pressing problem.


- Paul

RE: [PATCH 0/2] I2C: OMAP: spurious IRQ fixes

[Paul Walmsley]

One correction:

On Tue, 12 May 2009, Paul Walmsley wrote:

> At least receive overruns are handled gracefully by the controller. 
> Transmit FIFO underruns may be the more pressing problem.

Rather, transmit underruns are probably nonfatal in master mode also.  
But both receive overruns and transmit underruns in I2C slave mode are 
problematic, since the OMAP doesn't control the clock.


- Paul