[PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Ulrik Bech Hald]

Under certain rare conditions, I2C_STAT[13].RDR bit may be set,
and the corresponding interrupt fire, even when there is no
data in the receive FIFO, or the I2C data transfer is still
ongoing. These spurious RDR events must be ignored by the
software.

A check for OMAP_I2C_STAT_BB is introduced in the ISR to
prevent further processing of RDR interrupt, if the bus is busy.

Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
Signed-off-by: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ece0125..88feea1 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -668,6 +668,15 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			omap_i2c_complete_cmd(dev, err);
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
+			/* 3430 I2C Errata 1.15
+			 * RDR could be set when the bus is busy, then
+			 * ignore the interrupt, and clear the bit.
+			 */
+			u8 stat2 = 0;
+			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+			if (stat2 & OMAP_I2C_STAT_BB)
+				return IRQ_HANDLED;
+
 			if (dev->fifo_size) {
 				if (stat & OMAP_I2C_STAT_RRDY)
 					num_bytes = dev->fifo_size;
-- 
1.5.4.3

Re: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Kevin Hilman]

Ulrik Bech Hald <ubh@ti.com> writes:

> Under certain rare conditions, I2C_STAT[13].RDR bit may be set,
> and the corresponding interrupt fire, even when there is no
> data in the receive FIFO, or the I2C data transfer is still
> ongoing. These spurious RDR events must be ignored by the
> software.
>
> A check for OMAP_I2C_STAT_BB is introduced in the ISR to
> prevent further processing of RDR interrupt, if the bus is busy.
>
> Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
> Signed-off-by: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>

Minor CodingStyle comments below...

> ---
>  drivers/i2c/busses/i2c-omap.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ece0125..88feea1 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -668,6 +668,15 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  			omap_i2c_complete_cmd(dev, err);
>  		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
>  			u8 num_bytes = 1;

But the local variable define here, followed by a blank line, and
the assignment to zero is unnecessary.

> +			/* 3430 I2C Errata 1.15
> +			 * RDR could be set when the bus is busy, then
> +			 * ignore the interrupt, and clear the bit.
> +			 */

Move the '*/' to the end of the previous line. 

> +			u8 stat2 = 0;
> +			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +			if (stat2 & OMAP_I2C_STAT_BB)
> +				return IRQ_HANDLED;
> +
>  			if (dev->fifo_size) {
>  				if (stat & OMAP_I2C_STAT_RRDY)
>  					num_bytes = dev->fifo_size;
> -- 
> 1.5.4.3
>
> --
> 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 v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Paul Walmsley]

Hello Ulrik, Jagadeesh, 

On Mon, 22 Jun 2009, Ulrik Bech Hald wrote:

> Under certain rare conditions, I2C_STAT[13].RDR bit may be set,
> and the corresponding interrupt fire, even when there is no
> data in the receive FIFO, or the I2C data transfer is still
> ongoing. These spurious RDR events must be ignored by the
> software.
> 
> A check for OMAP_I2C_STAT_BB is introduced in the ISR to
> prevent further processing of RDR interrupt, if the bus is busy.
> 
> Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
> Signed-off-by: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ece0125..88feea1 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -668,6 +668,15 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  			omap_i2c_complete_cmd(dev, err);
>  		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
>  			u8 num_bytes = 1;
> +			/* 3430 I2C Errata 1.15
> +			 * RDR could be set when the bus is busy, then
> +			 * ignore the interrupt, and clear the bit.
> +			 */
> +			u8 stat2 = 0;
> +			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +			if (stat2 & OMAP_I2C_STAT_BB)
> +				return IRQ_HANDLED;

Why use stat2? Why not just test stat again?


> +
>  			if (dev->fifo_size) {
>  				if (stat & OMAP_I2C_STAT_RRDY)
>  					num_bytes = dev->fifo_size;
> -- 
> 1.5.4.3
> 
> --
> 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
> 


- Paul

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Pakaravoor, Jagadeesh]

Hi Paul,

> > +			u8 stat2 = 0;
> > +			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > +			if (stat2 & OMAP_I2C_STAT_BB)
> > +				return IRQ_HANDLED;
> 
> Why use stat2? Why not just test stat again?

Stat is read at the beginning of the ISR, what if BB bit gets cleared/set a while later, not along with RDR, as a corner case?

If we keep it this way, re-reading the register; what could be the potential problem?

Thanks!
-Jagadeesh

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Paul Walmsley]

Hello Jagadeesh,

On Tue, 23 Jun 2009, Pakaravoor, Jagadeesh wrote:

> > > +			u8 stat2 = 0;
> > > +			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > > +			if (stat2 & OMAP_I2C_STAT_BB)
> > > +				return IRQ_HANDLED;
> > 
> > Why use stat2? Why not just test stat again?
> 
> Stat is read at the beginning of the ISR, what if BB bit gets 
> cleared/set a while later, not along with RDR, as a corner case?

If that is possible, then the comment in this patch needs to be changed:

> +                     /* 3430 I2C Errata 1.15
> +                      * RDR could be set when the bus is busy, then
> +                      * ignore the interrupt, and clear the bit.
> +                      */

This implies that the state of the BB bit is important when the RDR bit is 
set.  The closest sample we have for that is the contents of the 'stat' 
variable.

> If we keep it this way, re-reading the register; what could be the 
> potential problem?

It doesn't match the definition of the erratum as expressed in the 
comment.  Is it possible for the RDR bit to be erroneously set when BB = 
0?


- Paul

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Hald, Ulrik Bech]

Hi Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Monday, June 22, 2009 10:20 PM
> To: Pakaravoor, Jagadeesh
> Cc: Hald, Ulrik Bech; linux-omap@vger.kernel.org
> Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> event
> 
> Hello Jagadeesh,
> 
> On Tue, 23 Jun 2009, Pakaravoor, Jagadeesh wrote:
> 
> > > > +			u8 stat2 = 0;
> > > > +			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > > > +			if (stat2 & OMAP_I2C_STAT_BB)
> > > > +				return IRQ_HANDLED;
> > >
> > > Why use stat2? Why not just test stat again?
> >
> > Stat is read at the beginning of the ISR, what if BB bit gets
> > cleared/set a while later, not along with RDR, as a corner case?
> 
> If that is possible, then the comment in this patch needs to be changed:
> 
> > +                     /* 3430 I2C Errata 1.15
> > +                      * RDR could be set when the bus is busy, then
> > +                      * ignore the interrupt, and clear the bit.
> > +                      */
> 
> This implies that the state of the BB bit is important when the RDR bit is
> set.  The closest sample we have for that is the contents of the 'stat'
> variable.
> 
If I understand it correctly, you're concerned that the wording of the comment lets one think, that the state of the bus is critical at the moment the RDR interrupt is set? I guess you're right about that, since the wording could be a little misleading.

The point of the errata workaround is only to prevent handling of the RDR interrupt, if the bus is busy at the time when the RDR is to be handled. It doesn't matter if BB has been set/cleared before that.

So maybe, the wording of the comment should be:

/* 3430 I2C Errata 1.15, 2430 I2C Errata 1.67:
 * RDR should not be handled when bus is busy */

?

> > If we keep it this way, re-reading the register; what could be the
> > potential problem?
> 
> It doesn't match the definition of the erratum as expressed in the
> comment.  Is it possible for the RDR bit to be erroneously set when BB =
> 0?

Yes, the nature of the errata is that the RDR interrupt could be spurious. Although, if the bus is not busy and the RDR is set erroneously (there are no bytes in the FIFO to be drained), then the normal isr code would just leave and do nothing, which is what we also need in the errata work around.

> 
> 
> - Paul

/Ulrik

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Paul Walmsley]

Hello Ulrik,

On Tue, 23 Jun 2009, Hald, Ulrik Bech wrote:

> > -----Original Message-----
> > From: Paul Walmsley [mailto:paul@pwsan.com]
> > Sent: Monday, June 22, 2009 10:20 PM
> > To: Pakaravoor, Jagadeesh
> > Cc: Hald, Ulrik Bech; linux-omap@vger.kernel.org
> > Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> > event
> > 
> > Hello Jagadeesh,
> > 
> > On Tue, 23 Jun 2009, Pakaravoor, Jagadeesh wrote:
> > 
> > > > > +			u8 stat2 = 0;
> > > > > +			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > > > > +			if (stat2 & OMAP_I2C_STAT_BB)
> > > > > +				return IRQ_HANDLED;
> > > >
> > > > Why use stat2? Why not just test stat again?
> > >
> > > Stat is read at the beginning of the ISR, what if BB bit gets
> > > cleared/set a while later, not along with RDR, as a corner case?
> > 
> > If that is possible, then the comment in this patch needs to be changed:
> > 
> > > +                     /* 3430 I2C Errata 1.15
> > > +                      * RDR could be set when the bus is busy, then
> > > +                      * ignore the interrupt, and clear the bit.
> > > +                      */
> > 
> > This implies that the state of the BB bit is important when the RDR bit is
> > set.  The closest sample we have for that is the contents of the 'stat'
> > variable.
> > 
> If I understand it correctly, you're concerned that the wording of the 
> comment lets one think, that the state of the bus is critical at the 
> moment the RDR interrupt is set? I guess you're right about that, since 
> the wording could be a little misleading.

My concern is that the comment and the code seem to conflict.

> The point of the errata workaround is only to prevent handling of the 
> RDR interrupt, if the bus is busy at the time when the RDR is to be 
> handled. It doesn't matter if BB has been set/cleared before that.
> 
> So maybe, the wording of the comment should be:
> 
> /* 3430 I2C Errata 1.15, 2430 I2C Errata 1.67:
>  * RDR should not be handled when bus is busy */
> 
> ?

Yes, that is better.

> > > If we keep it this way, re-reading the register; what could be the
> > > potential problem?
> > 
> > It doesn't match the definition of the erratum as expressed in the
> > comment.  Is it possible for the RDR bit to be erroneously set when BB =
> > 0?
> 
> Yes, the nature of the errata is that the RDR interrupt could be 
> spurious. Although, if the bus is not busy and the RDR is set 
> erroneously (there are no bytes in the FIFO to be drained), then the 
> normal isr code would just leave and do nothing, which is what we also 
> need in the errata work around.

Okay.  So it looks like there is a unfixable race here.  Is it possible 
for BB to be 0 during the stat2 read, then for BB to go to 1 immediately 
afterwards?  Then the rest of the RDR handler would be entered.  If this 
is possible, what will the ISR do -- will it hang?

If this assessment of the problem is accurate, the stat2 read shrinks 
the race window, and then indeed seems useful.


- Paul

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Menon, Nishanth]

Paul, Ulrik,

Just adding my view to this discussion:
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Paul Walmsley
> Sent: Tuesday, June 23, 2009 9:05 PM
> To: Hald, Ulrik Bech
> Cc: Pakaravoor, Jagadeesh; linux-omap@vger.kernel.org
> Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> event
> > >
> > > > > > +			u8 stat2 = 0;
> > > > > > +			stat2 = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > > > > > +			if (stat2 & OMAP_I2C_STAT_BB)
> > > > > > +				return IRQ_HANDLED;
> > > > >
> > > > > Why use stat2? Why not just test stat again?
> > > >
> > > > Stat is read at the beginning of the ISR, what if BB bit gets
> > > > cleared/set a while later, not along with RDR, as a corner case?
> > >
> > > If that is possible, then the comment in this patch needs to be
> changed:
> > >
> > > > +                     /* 3430 I2C Errata 1.15
> > > > +                      * RDR could be set when the bus is busy, then
> > > > +                      * ignore the interrupt, and clear the bit.
> > > > +                      */
> > >
> > > This implies that the state of the BB bit is important when the RDR
> bit is
> > > set.  The closest sample we have for that is the contents of the
> 'stat'
> > > variable.
> > >
> > If I understand it correctly, you're concerned that the wording of the
> > comment lets one think, that the state of the bus is critical at the
> > moment the RDR interrupt is set? I guess you're right about that, since
> > the wording could be a little misleading.
> 
> My concern is that the comment and the code seem to conflict.
> 
> > The point of the errata workaround is only to prevent handling of the
> > RDR interrupt, if the bus is busy at the time when the RDR is to be
> > handled. It doesn't matter if BB has been set/cleared before that.
> >
> > So maybe, the wording of the comment should be:
> >
> > /* 3430 I2C Errata 1.15, 2430 I2C Errata 1.67:
> >  * RDR should not be handled when bus is busy */
> >
> > ?
> 
> Yes, that is better.
> 
> > > > If we keep it this way, re-reading the register; what could be the
> > > > potential problem?
> > >
> > > It doesn't match the definition of the erratum as expressed in the
> > > comment.  Is it possible for the RDR bit to be erroneously set when BB
> =
> > > 0?
> >
> > Yes, the nature of the errata is that the RDR interrupt could be
> > spurious. Although, if the bus is not busy and the RDR is set
> > erroneously (there are no bytes in the FIFO to be drained), then the
> > normal isr code would just leave and do nothing, which is what we also
> > need in the errata work around.
> 
> Okay.  So it looks like there is a unfixable race here.  Is it possible
> for BB to be 0 during the stat2 read, then for BB to go to 1 immediately
> afterwards?  Then the rest of the RDR handler would be entered.  If this
> is possible, what will the ISR do -- will it hang?
> 
> If this assessment of the problem is accurate, the stat2 read shrinks
> the race window, and then indeed seems useful.
> 
> 

a)       why would bus be busy? - answer bus is busy before a transaction starts - each transaction is an atomic operation. If someone goofs around with signal while a master is sending/receiving data, there is AL(Arbitration Lost).. not Bus busy.

b)       Look at the flow in TRM figure 18-13 I2C Master Reciever Mode, interrupt mode -> where does BB get checked by the h/w? Not in the middle of the transaction..

Apologies if I am wrong, so am not entirely following the discussion here..

Regards,
Nishanth Menon

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Hald, Ulrik Bech]

> -----Original Message-----
> From: Menon, Nishanth
> Sent: Tuesday, June 23, 2009 3:27 PM
> To: Paul Walmsley; Hald, Ulrik Bech
> Cc: Pakaravoor, Jagadeesh; linux-omap@vger.kernel.org
> Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> event
> 
> Paul, Ulrik,
> 
> Just adding my view to this discussion:
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Paul Walmsley
> > Sent: Tuesday, June 23, 2009 9:05 PM
> > To: Hald, Ulrik Bech
> > Cc: Pakaravoor, Jagadeesh; linux-omap@vger.kernel.org
> > Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> > event
> > > >
> > > > > > > +			u8 stat2 = 0;
> > > > > > > +			stat2 = omap_i2c_read_reg(dev,
> OMAP_I2C_STAT_REG);
> > > > > > > +			if (stat2 & OMAP_I2C_STAT_BB)
> > > > > > > +				return IRQ_HANDLED;
> > > > > >
> > > > > > Why use stat2? Why not just test stat again?
> > > > >
> > > > > Stat is read at the beginning of the ISR, what if BB bit gets
> > > > > cleared/set a while later, not along with RDR, as a corner case?
> > > >
> > > > If that is possible, then the comment in this patch needs to be
> > changed:
> > > >
> > > > > +                     /* 3430 I2C Errata 1.15
> > > > > +                      * RDR could be set when the bus is busy,
> then
> > > > > +                      * ignore the interrupt, and clear the bit.
> > > > > +                      */
> > > >
> > > > This implies that the state of the BB bit is important when the RDR
> > bit is
> > > > set.  The closest sample we have for that is the contents of the
> > 'stat'
> > > > variable.
> > > >
> > > If I understand it correctly, you're concerned that the wording of the
> > > comment lets one think, that the state of the bus is critical at the
> > > moment the RDR interrupt is set? I guess you're right about that,
> since
> > > the wording could be a little misleading.
> >
> > My concern is that the comment and the code seem to conflict.
> >
> > > The point of the errata workaround is only to prevent handling of the
> > > RDR interrupt, if the bus is busy at the time when the RDR is to be
> > > handled. It doesn't matter if BB has been set/cleared before that.
> > >
> > > So maybe, the wording of the comment should be:
> > >
> > > /* 3430 I2C Errata 1.15, 2430 I2C Errata 1.67:
> > >  * RDR should not be handled when bus is busy */
> > >
> > > ?
> >
> > Yes, that is better.
> >
> > > > > If we keep it this way, re-reading the register; what could be the
> > > > > potential problem?
> > > >
> > > > It doesn't match the definition of the erratum as expressed in the
> > > > comment.  Is it possible for the RDR bit to be erroneously set when
> BB
> > =
> > > > 0?
> > >
> > > Yes, the nature of the errata is that the RDR interrupt could be
> > > spurious. Although, if the bus is not busy and the RDR is set
> > > erroneously (there are no bytes in the FIFO to be drained), then the
> > > normal isr code would just leave and do nothing, which is what we also
> > > need in the errata work around.
> >
> > Okay.  So it looks like there is a unfixable race here.  Is it possible
> > for BB to be 0 during the stat2 read, then for BB to go to 1 immediately
> > afterwards?  Then the rest of the RDR handler would be entered.  If this
> > is possible, what will the ISR do -- will it hang?
> >
> > If this assessment of the problem is accurate, the stat2 read shrinks
> > the race window, and then indeed seems useful.
> >
> >
> 
> a)       why would bus be busy? - answer bus is busy before a transaction
> starts - each transaction is an atomic operation. If someone goofs around
> with signal while a master is sending/receiving data, there is
> AL(Arbitration Lost).. not Bus busy.
> 
But couldn't the BB bit be set by the I2C controller to signal a new receive transaction has started, while the ISR is still in the process of handling the RDR interrupt?

Anyway, when I look at the code below where the patch is introduced:
..
			if (dev->fifo_size) {
				if (stat & OMAP_I2C_STAT_RRDY)
					num_bytes = dev->fifo_size;
				else
					num_bytes = omap_i2c_read_reg(dev,
							OMAP_I2C_BUFSTAT_REG);
			}
			while (num_bytes) {
				num_bytes--;
				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
				if (dev->buf_len) {
					*dev->buf++ = w;
					dev->buf_len--;
					/* Data reg from 2430 is 8 bit wide */
					if (!cpu_is_omap2430() &&
							!cpu_is_omap34xx()) {
						if (dev->buf_len) {
							*dev->buf++ = w >> 8;
							dev->buf_len--;
						}
					}
				} else {
					if (stat & OMAP_I2C_STAT_RRDY)
						dev_err(dev->dev,
							"RRDY IRQ while no data"
								" requested\n");
					if (stat & OMAP_I2C_STAT_RDR)
						dev_err(dev->dev,
							"RDR IRQ while no data"
								" requested\n");
					break;
				}
			}
..

... is there really a need for the patch in the first place? The data extraction from the FIFO is handled the same way for both RDR and RRDY interrupts. The only difference is what num_bytes is set to. Sure, RDR could be set spuriously, but if we're handling it the same way as RRDY what's the problem?
If data is ready in the FIFO, we extract only what is there. If no data is ready, we simply leave. 

Please correct me if I am wrong..

> b)       Look at the flow in TRM figure 18-13 I2C Master Reciever Mode,
> interrupt mode -> where does BB get checked by the h/w? Not in the middle
> of the transaction..
> 
> Apologies if I am wrong, so am not entirely following the discussion
> here..
> 
> Regards,
> Nishanth Menon

/Ulrik

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Paul Walmsley]

Hello Ulrik,

On Thu, 25 Jun 2009, Hald, Ulrik Bech wrote:

> > -----Original Message-----
> > From: Menon, Nishanth
> > Sent: Tuesday, June 23, 2009 3:27 PM
> > To: Paul Walmsley; Hald, Ulrik Bech
> > Cc: Pakaravoor, Jagadeesh; linux-omap@vger.kernel.org
> > Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> > event
> > 
> > Paul, Ulrik,
> > 
> > Just adding my view to this discussion:
> > > -----Original Message-----
> > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > > owner@vger.kernel.org] On Behalf Of Paul Walmsley
> > > Sent: Tuesday, June 23, 2009 9:05 PM
> > > To: Hald, Ulrik Bech
> > > Cc: Pakaravoor, Jagadeesh; linux-omap@vger.kernel.org
> > > Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> > > event
> > > > >
> > > > > > > > +			u8 stat2 = 0;
> > > > > > > > +			stat2 = omap_i2c_read_reg(dev,
> > OMAP_I2C_STAT_REG);
> > > > > > > > +			if (stat2 & OMAP_I2C_STAT_BB)
> > > > > > > > +				return IRQ_HANDLED;
> > > > > > >
> > > > > > > Why use stat2? Why not just test stat again?
> > > > > >
> > > > > > Stat is read at the beginning of the ISR, what if BB bit gets
> > > > > > cleared/set a while later, not along with RDR, as a corner case?
> > > > >
> > > > > If that is possible, then the comment in this patch needs to be
> > > changed:
> > > > >
> > > > > > +                     /* 3430 I2C Errata 1.15
> > > > > > +                      * RDR could be set when the bus is busy,
> > then
> > > > > > +                      * ignore the interrupt, and clear the bit.
> > > > > > +                      */
> > > > >
> > > > > This implies that the state of the BB bit is important when the RDR
> > > bit is
> > > > > set.  The closest sample we have for that is the contents of the
> > > 'stat'
> > > > > variable.
> > > > >
> > > > If I understand it correctly, you're concerned that the wording of the
> > > > comment lets one think, that the state of the bus is critical at the
> > > > moment the RDR interrupt is set? I guess you're right about that,
> > since
> > > > the wording could be a little misleading.
> > >
> > > My concern is that the comment and the code seem to conflict.
> > >
> > > > The point of the errata workaround is only to prevent handling of the
> > > > RDR interrupt, if the bus is busy at the time when the RDR is to be
> > > > handled. It doesn't matter if BB has been set/cleared before that.
> > > >
> > > > So maybe, the wording of the comment should be:
> > > >
> > > > /* 3430 I2C Errata 1.15, 2430 I2C Errata 1.67:
> > > >  * RDR should not be handled when bus is busy */
> > > >
> > > > ?
> > >
> > > Yes, that is better.
> > >
> > > > > > If we keep it this way, re-reading the register; what could be the
> > > > > > potential problem?
> > > > >
> > > > > It doesn't match the definition of the erratum as expressed in the
> > > > > comment.  Is it possible for the RDR bit to be erroneously set when
> > BB
> > > =
> > > > > 0?
> > > >
> > > > Yes, the nature of the errata is that the RDR interrupt could be
> > > > spurious. Although, if the bus is not busy and the RDR is set
> > > > erroneously (there are no bytes in the FIFO to be drained), then the
> > > > normal isr code would just leave and do nothing, which is what we also
> > > > need in the errata work around.
> > >
> > > Okay.  So it looks like there is a unfixable race here.  Is it possible
> > > for BB to be 0 during the stat2 read, then for BB to go to 1 immediately
> > > afterwards?  Then the rest of the RDR handler would be entered.  If this
> > > is possible, what will the ISR do -- will it hang?
> > >
> > > If this assessment of the problem is accurate, the stat2 read shrinks
> > > the race window, and then indeed seems useful.
> > >
> > >
> > 
> > a)       why would bus be busy? - answer bus is busy before a transaction
> > starts - each transaction is an atomic operation. If someone goofs around
> > with signal while a master is sending/receiving data, there is
> > AL(Arbitration Lost).. not Bus busy.
> > 
> But couldn't the BB bit be set by the I2C controller to signal a new receive transaction has started, while the ISR is still in the process of handling the RDR interrupt?
> 
> Anyway, when I look at the code below where the patch is introduced:
> ..
> 			if (dev->fifo_size) {
> 				if (stat & OMAP_I2C_STAT_RRDY)
> 					num_bytes = dev->fifo_size;
> 				else
> 					num_bytes = omap_i2c_read_reg(dev,
> 							OMAP_I2C_BUFSTAT_REG);
> 			}
> 			while (num_bytes) {
> 				num_bytes--;
> 				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> 				if (dev->buf_len) {
> 					*dev->buf++ = w;
> 					dev->buf_len--;
> 					/* Data reg from 2430 is 8 bit wide */
> 					if (!cpu_is_omap2430() &&
> 							!cpu_is_omap34xx()) {
> 						if (dev->buf_len) {
> 							*dev->buf++ = w >> 8;
> 							dev->buf_len--;
> 						}
> 					}
> 				} else {
> 					if (stat & OMAP_I2C_STAT_RRDY)
> 						dev_err(dev->dev,
> 							"RRDY IRQ while no data"
> 								" requested\n");
> 					if (stat & OMAP_I2C_STAT_RDR)
> 						dev_err(dev->dev,
> 							"RDR IRQ while no data"
> 								" requested\n");
> 					break;
> 				}
> 			}
> ..
> 
> ... is there really a need for the patch in the first place? The data 
> extraction from the FIFO is handled the same way for both RDR and RRDY 
> interrupts. The only difference is what num_bytes is set to. Sure, RDR 
> could be set spuriously, but if we're handling it the same way as RRDY 
> what's the problem? If data is ready in the FIFO, we extract only what 
> is there. If no data is ready, we simply leave.

That looks correct on 2430, 3430 where the I2C controller has a FIFO.  
But for 2420, dev->fifo_size == 0, which causes num_bytes = 1, which will 
attempt a single read from the I2C controller.  (This assumes that the bug 
is present on 2420 - do you know if the erratum applies there also?)


- Paul

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Paul Walmsley]

On Thu, 2 Jul 2009, Paul Walmsley wrote:

> That looks correct on 2430, 3430 where the I2C controller has a FIFO.  
> But for 2420, dev->fifo_size == 0, which causes num_bytes = 1, which will 
> attempt a single read from the I2C controller.  (This assumes that the bug 
> is present on 2420 - do you know if the erratum applies there also?)

Thinking about this further, this shouldn't apply to 2420 since it is 
using the FIFO-less rev 2 I2C, and therefore won't have the RDR bit.
So, I agree with Ulrik: no patch seems necessary for this erratum.

> 
> 
> - Paul
> --
> 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
> 


- Paul

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Paul Walmsley]

On Tue, 23 Jun 2009, Paul Walmsley wrote:

> Okay.  So it looks like there is a unfixable race here.  Is it possible 
> for BB to be 0 during the stat2 read, then for BB to go to 1 immediately 
> afterwards?  Then the rest of the RDR handler would be entered.  If this 
> is possible, what will the ISR do -- will it hang?

By the way, it would also be helpful if you could check what the "certain 
rare conditions" are that erratum 1.15 refers to that cause this 
problem...


- Paul

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Hald, Ulrik Bech]

Hi Paul,
> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Tuesday, June 23, 2009 4:18 PM
> To: Hald, Ulrik Bech
> Cc: Pakaravoor, Jagadeesh; linux-omap@vger.kernel.org
> Subject: RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR
> event
> 
> On Tue, 23 Jun 2009, Paul Walmsley wrote:
> 
> > Okay.  So it looks like there is a unfixable race here.  Is it possible
> > for BB to be 0 during the stat2 read, then for BB to go to 1 immediately
> > afterwards?  Then the rest of the RDR handler would be entered.  If this
> > is possible, what will the ISR do -- will it hang?
> 
> By the way, it would also be helpful if you could check what the "certain
> rare conditions" are that erratum 1.15 refers to that cause this
> problem...
> 
Yeah, I would like to know that too, but unfortunately the errata specification (which is all I have to lean on) doesn't say.
> 
> - Paul

Ulrik

RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event

[Paul Walmsley]

Hello Ulrik,

one other thing to check,

On Tue, 23 Jun 2009, Hald, Ulrik Bech wrote:

> > -----Original Message-----
> > From: Paul Walmsley [mailto:paul@pwsan.com]
>
> > Is it possible for the RDR bit to be erroneously set when BB =
> > 0?
> 
> Yes, the nature of the errata is that the RDR interrupt could be 
> spurious. Although, if the bus is not busy and the RDR is set 
> erroneously (there are no bytes in the FIFO to be drained), then the 
> normal isr code would just leave and do nothing, which is what we also 
> need in the errata work around.

Is the BB bit indirectly controlled by the driver, or by the I2C IP block?

If the former, then perhaps there is no need to read the BB bit at all. 
The ISR can just ignore the RDR interrupts until the rest of the driver 
code indicates that the RDRs are worth paying attention to.


- Paul