[PATCH] i2c-omap: fix I2C timeouts due to recursive omap_i2c_{un,}idle()

[PATCH] i2c-omap: fix I2C timeouts due to recursive omap_i2c_{un,}idle()

[Paul Walmsley]

omap_i2c_unidle() and omap_i2c_idle() are called recursively during
omap_i2c_probe().  This is evidently unexpected and will wipe
out the I2C interrupt enable register the second time that
omap_i2c_idle() is called consecutively.  Any I2C transactions
following a probe of a bus with at least one device on it will then
time out.

Fix by moving omap_i2c_idle() further up in omap_i2c_probe().  Ensure
the I2C controller is marked as idle before the probe starts.  Also
attempt to catch future reappearances of this bug early in development
by warning in omap_i2c_{un,}idle() when they are called recursively.

Problem reported by David Brownell <david-b@pacbell.net>.

Tested on 3430SDP and 2430SDP.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: David Brownell <david-b@pacbell.net>
Cc: Richard Woodruff <r-woodruff2@ti.com>

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 1e6c3fd..a999606 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -202,6 +202,8 @@ 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);
+
 	if (dev->iclk != NULL)
 		clk_enable(dev->iclk);
 	clk_enable(dev->fclk);
@@ -214,6 +216,8 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 {
 	u16 iv;
 
+	WARN_ON(dev->idle);
+
 	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
 	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
 	if (dev->rev1)
@@ -748,6 +752,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		*speed = 100; /* Defualt speed */
 
 	dev->speed = *speed;
+	dev->idle = 1;
 	dev->dev = &pdev->dev;
 	dev->irq = irq->start;
 	dev->base = ioremap(mem->start, mem->end - mem->start + 1);
@@ -797,6 +802,8 @@ omap_i2c_probe(struct platform_device *pdev)
 	dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n",
 		 pdev->id, r >> 4, r & 0xf, dev->speed);
 
+	omap_i2c_idle(dev);
+
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
@@ -813,8 +820,6 @@ omap_i2c_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 
-	omap_i2c_idle(dev);
-
 	return 0;
 
 err_free_irq:

Re: [PATCH] i2c-omap: fix I2C timeouts due to recursive omap_i2c_{un,}idle()

[Steve Sakoman]

On Fri, Oct 10, 2008 at 9:58 AM, Paul Walmsley <paul@pwsan.com> wrote:
>
> omap_i2c_unidle() and omap_i2c_idle() are called recursively during
> omap_i2c_probe().  This is evidently unexpected and will wipe
> out the I2C interrupt enable register the second time that
> omap_i2c_idle() is called consecutively.  Any I2C transactions
> following a probe of a bus with at least one device on it will then
> time out.
>
> Fix by moving omap_i2c_idle() further up in omap_i2c_probe().  Ensure
> the I2C controller is marked as idle before the probe starts.  Also
> attempt to catch future reappearances of this bug early in development
> by warning in omap_i2c_{un,}idle() when they are called recursively.
>
> Problem reported by David Brownell <david-b@pacbell.net>.
>
> Tested on 3430SDP and 2430SDP.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: Richard Woodruff <r-woodruff2@ti.com>

I can confirm that this patch eliminates the i2c timeout issues on Overo.

Acked-by; Steve Sakoman <steve@sakoman.com>

Re: [PATCH] i2c-omap: fix I2C timeouts due to recursive omap_i2c_{un,}idle()

[David Brownell]

On Friday 10 October 2008, Paul Walmsley wrote:
> Problem reported by David Brownell <david-b@pacbell.net>.

Hmm, seems to solve the problem for me too... though for
the life of me I can't quite see how that explains all
the symptoms I've observed.  I'll keep an eye out to see
what comes up over time; I could believe this is only one
of the problems lurking here.

- Dave

Re: [PATCH] i2c-omap: fix I2C timeouts due to recursive omap_i2c_{un,}idle()

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [081011 00:32]:
> On Friday 10 October 2008, Paul Walmsley wrote:
> > Problem reported by David Brownell <david-b@pacbell.net>.
> 
> Hmm, seems to solve the problem for me too... though for
> the life of me I can't quite see how that explains all
> the symptoms I've observed.  I'll keep an eye out to see
> what comes up over time; I could believe this is only one
> of the problems lurking here.

Great to hear Paul got it figured out! Pushed.

Tony