* [PATCH] i2c: omap: fix draining irq handling
@ 2013-01-20 18:37 Aaro Koskinen
2013-01-21 8:44 ` Felipe Balbi
2013-01-23 9:57 ` Wolfram Sang
0 siblings, 2 replies; 5+ messages in thread
From: Aaro Koskinen @ 2013-01-20 18:37 UTC (permalink / raw)
To: w.sang-bIcnvbaLZ9MEGnE8C9+IrQ, ben-linux-elnMNo+KYs3YtjvyW6yDsg,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, balbi-l0cyMroinI0
Cc: Aaro Koskinen
Commit 0bdfe0cb803dce699ff337c35d8e97ac355fa417 (i2c: omap: sanitize
exit path) changed the interrupt handler to exit early and complete
the transfer after the draining IRQ is handled. As a result, the ARDY
may not be cleared properly, and it may cause all future I2C transfers
to timeout with "timeout waiting for bus ready". This is reproducible
at least with N900 when twl4030_gpio makes a long write (> FIFO size)
during the probe (http://marc.info/?l=linux-omap&m=135818882610432&w=2).
The fix is to continue until we get ARDY interrupt that completes the
transfer. Tested with 3.8-rc4 + N900: 20 boots in a row without errors;
without the patch the problem triggers after few reboots.
Signed-off-by: Aaro Koskinen <aaro.koskinen-X3B1VOXEql0@public.gmane.org>
---
drivers/i2c/busses/i2c-omap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 832f16e..4cc2f05 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -963,7 +963,7 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
i2c_omap_errata_i207(dev, stat);
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
- break;
+ continue;
}
if (stat & OMAP_I2C_STAT_RRDY) {
@@ -989,7 +989,7 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
break;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XDR);
- break;
+ continue;
}
if (stat & OMAP_I2C_STAT_XRDY) {
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] i2c: omap: fix draining irq handling
2013-01-20 18:37 [PATCH] i2c: omap: fix draining irq handling Aaro Koskinen
@ 2013-01-21 8:44 ` Felipe Balbi
2013-01-21 18:01 ` Aaro Koskinen
2013-01-23 9:57 ` Wolfram Sang
1 sibling, 1 reply; 5+ messages in thread
From: Felipe Balbi @ 2013-01-21 8:44 UTC (permalink / raw)
To: Aaro Koskinen; +Cc: w.sang, ben-linux, linux-omap, linux-i2c, balbi
[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]
HI,
On Sun, Jan 20, 2013 at 08:37:02PM +0200, Aaro Koskinen wrote:
> Commit 0bdfe0cb803dce699ff337c35d8e97ac355fa417 (i2c: omap: sanitize
> exit path) changed the interrupt handler to exit early and complete
> the transfer after the draining IRQ is handled. As a result, the ARDY
> may not be cleared properly, and it may cause all future I2C transfers
> to timeout with "timeout waiting for bus ready". This is reproducible
> at least with N900 when twl4030_gpio makes a long write (> FIFO size)
> during the probe (http://marc.info/?l=linux-omap&m=135818882610432&w=2).
>
> The fix is to continue until we get ARDY interrupt that completes the
> transfer. Tested with 3.8-rc4 + N900: 20 boots in a row without errors;
> without the patch the problem triggers after few reboots.
fair enough, but ...
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> ---
> drivers/i2c/busses/i2c-omap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 832f16e..4cc2f05 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -963,7 +963,7 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
> i2c_omap_errata_i207(dev, stat);
>
> omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
> - break;
> + continue;
... when we get draining Interrupt, it means we're receiving the last
few bytes, meaning that by the time we handle it there's nothing pending
to be transferred.
I would rather wait for ARDY before exiting omap_i2c_xfer_msg() rather
than here. Specially since that was the programming model suggested by
our IP engineers. Such handling is part of my other patchset and can be
easily ported.
OTOH, we might decide to go with this during this -rc and fix it all up
for v3.9 on my other patchset.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: omap: fix draining irq handling
2013-01-21 8:44 ` Felipe Balbi
@ 2013-01-21 18:01 ` Aaro Koskinen
2013-01-21 18:27 ` Felipe Balbi
0 siblings, 1 reply; 5+ messages in thread
From: Aaro Koskinen @ 2013-01-21 18:01 UTC (permalink / raw)
To: Felipe Balbi, Wolfram Sang; +Cc: ben-linux, linux-omap, linux-i2c
Hi,
On Mon, Jan 21, 2013 at 10:44:31AM +0200, Felipe Balbi wrote:
> On Sun, Jan 20, 2013 at 08:37:02PM +0200, Aaro Koskinen wrote:
> > Commit 0bdfe0cb803dce699ff337c35d8e97ac355fa417 (i2c: omap: sanitize
> > exit path) changed the interrupt handler to exit early and complete
> > the transfer after the draining IRQ is handled. As a result, the ARDY
> > may not be cleared properly, and it may cause all future I2C transfers
> > to timeout with "timeout waiting for bus ready". This is reproducible
> > at least with N900 when twl4030_gpio makes a long write (> FIFO size)
> > during the probe (http://marc.info/?l=linux-omap&m=135818882610432&w=2).
> >
> > The fix is to continue until we get ARDY interrupt that completes the
> > transfer. Tested with 3.8-rc4 + N900: 20 boots in a row without errors;
> > without the patch the problem triggers after few reboots.
>
> fair enough, but ...
>
> > Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > ---
> > drivers/i2c/busses/i2c-omap.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index 832f16e..4cc2f05 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -963,7 +963,7 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
> > i2c_omap_errata_i207(dev, stat);
> >
> > omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
> > - break;
> > + continue;
>
> ... when we get draining Interrupt, it means we're receiving the last
> few bytes, meaning that by the time we handle it there's nothing pending
> to be transferred.
>
> I would rather wait for ARDY before exiting omap_i2c_xfer_msg() rather
> than here. Specially since that was the programming model suggested by
> our IP engineers. Such handling is part of my other patchset and can be
> easily ported.
>
> OTOH, we might decide to go with this during this -rc and fix it all up
> for v3.9 on my other patchset.
Completing through ARDY interrupt has been known to work reliably for
years. So for 3.8-rc I would prefer to just restore the old behaviour.
A.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: omap: fix draining irq handling
2013-01-21 18:01 ` Aaro Koskinen
@ 2013-01-21 18:27 ` Felipe Balbi
0 siblings, 0 replies; 5+ messages in thread
From: Felipe Balbi @ 2013-01-21 18:27 UTC (permalink / raw)
To: Aaro Koskinen
Cc: Felipe Balbi, Wolfram Sang, ben-linux, linux-omap, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]
Hi,
On Mon, Jan 21, 2013 at 08:01:59PM +0200, Aaro Koskinen wrote:
> Hi,
>
> On Mon, Jan 21, 2013 at 10:44:31AM +0200, Felipe Balbi wrote:
> > On Sun, Jan 20, 2013 at 08:37:02PM +0200, Aaro Koskinen wrote:
> > > Commit 0bdfe0cb803dce699ff337c35d8e97ac355fa417 (i2c: omap: sanitize
> > > exit path) changed the interrupt handler to exit early and complete
> > > the transfer after the draining IRQ is handled. As a result, the ARDY
> > > may not be cleared properly, and it may cause all future I2C transfers
> > > to timeout with "timeout waiting for bus ready". This is reproducible
> > > at least with N900 when twl4030_gpio makes a long write (> FIFO size)
> > > during the probe (http://marc.info/?l=linux-omap&m=135818882610432&w=2).
> > >
> > > The fix is to continue until we get ARDY interrupt that completes the
> > > transfer. Tested with 3.8-rc4 + N900: 20 boots in a row without errors;
> > > without the patch the problem triggers after few reboots.
> >
> > fair enough, but ...
> >
> > > Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > > ---
> > > drivers/i2c/busses/i2c-omap.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > > index 832f16e..4cc2f05 100644
> > > --- a/drivers/i2c/busses/i2c-omap.c
> > > +++ b/drivers/i2c/busses/i2c-omap.c
> > > @@ -963,7 +963,7 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
> > > i2c_omap_errata_i207(dev, stat);
> > >
> > > omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
> > > - break;
> > > + continue;
> >
> > ... when we get draining Interrupt, it means we're receiving the last
> > few bytes, meaning that by the time we handle it there's nothing pending
> > to be transferred.
> >
> > I would rather wait for ARDY before exiting omap_i2c_xfer_msg() rather
> > than here. Specially since that was the programming model suggested by
> > our IP engineers. Such handling is part of my other patchset and can be
> > easily ported.
> >
> > OTOH, we might decide to go with this during this -rc and fix it all up
> > for v3.9 on my other patchset.
>
> Completing through ARDY interrupt has been known to work reliably for
> years. So for 3.8-rc I would prefer to just restore the old behaviour.
fair enough, I'll rebase my patchset on top of $SUBJECT
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: omap: fix draining irq handling
2013-01-20 18:37 [PATCH] i2c: omap: fix draining irq handling Aaro Koskinen
2013-01-21 8:44 ` Felipe Balbi
@ 2013-01-23 9:57 ` Wolfram Sang
1 sibling, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2013-01-23 9:57 UTC (permalink / raw)
To: Aaro Koskinen; +Cc: ben-linux, linux-omap, linux-i2c, balbi
On Sun, Jan 20, 2013 at 08:37:02PM +0200, Aaro Koskinen wrote:
> Commit 0bdfe0cb803dce699ff337c35d8e97ac355fa417 (i2c: omap: sanitize
> exit path) changed the interrupt handler to exit early and complete
> the transfer after the draining IRQ is handled. As a result, the ARDY
> may not be cleared properly, and it may cause all future I2C transfers
> to timeout with "timeout waiting for bus ready". This is reproducible
> at least with N900 when twl4030_gpio makes a long write (> FIFO size)
> during the probe (http://marc.info/?l=linux-omap&m=135818882610432&w=2).
>
> The fix is to continue until we get ARDY interrupt that completes the
> transfer. Tested with 3.8-rc4 + N900: 20 boots in a row without errors;
> without the patch the problem triggers after few reboots.
>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Applied to current and interpreted Felipe's comments as ack. Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-01-23 9:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-20 18:37 [PATCH] i2c: omap: fix draining irq handling Aaro Koskinen
2013-01-21 8:44 ` Felipe Balbi
2013-01-21 18:01 ` Aaro Koskinen
2013-01-21 18:27 ` Felipe Balbi
2013-01-23 9:57 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).