RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event
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-ow...@vger.kernel.org [mailto:linux-omap-
ow...@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
RE: [PATCH v2 1/1] i2c:OMAP3:Errata workaround for spurious RDR event
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html - Paul -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@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
Hi Paul, -Original Message- From: Paul Walmsley [mailto:p...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@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
Hello Ulrik, On Tue, 23 Jun 2009, Hald, Ulrik Bech wrote: -Original Message- From: Paul Walmsley [mailto:p...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@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, Ulrik, Just adding my view to this discussion: -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@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
Ulrik Bech Hald u...@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 u...@ti.com
Signed-off-by: Jagadeesh Bhaskar Pakaravoor j-pakarav...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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
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 u...@ti.com
Signed-off-by: Jagadeesh Bhaskar Pakaravoor j-pakarav...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
