RE: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
Hello Paul!
I am working on this issue and will get back soon.
Regards
Moiz Sonasath
Linux Baseport Team, Dallas
214-567-5514
-Original Message-
From: Paul Walmsley [mailto:p...@pwsan.com]
Sent: Wednesday, July 08, 2009 11:51 AM
To: Sonasath, Moiz
Cc: linux-omap@vger.kernel.org; Pakaravoor, Jagadeesh; Kamat, Nishant; Pandita,
Vikram
Subject: Re: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata
1.153
Hello Moiz, Nishant, Jagadeesh,
Any plans to respond to these comments?
- Paul
On Mon, 22 Jun 2009, Paul Walmsley wrote:
> Moiz, Nishant, Jagadeesh,
>
> Looks like this patch has some serious problems.
>
> On Mon, 22 Jun 2009, Sonasath, Moiz wrote:
>
> > This patch includes the workarround for I2C Errata 1.153: When an
> > XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG
> >
> > Signed-off-by: Jagadeesh Bhaskar Pakaravoor
> > Signed-off by: Nishant Kamat
> > Signed-off-by: Moiz Sonasath
> > ---
> > drivers/i2c/busses/i2c-omap.c | 54
> > +---
> > 1 files changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index ece0125..e84836b 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
> > #define omap_i2c_rev1_isr NULL
> > #endif
> >
> > +/* I2C Errata 1.153:
> > + * When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> > + * Otherwise some data bytes can be lost while transferring them from the
> > + * memory to the I2C interface.
> > + */
> > +
> > +static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
> > +{
> > + u16 xudf;
> > + int counter = 500;
>
> Shouldn't the timeout threshold depend on the I2C bus speed and transmit
> FIFO threshold?
>
> > +
> > + /* We are in interrupt context. Wait for XUDF for max 7 msec */
>
> There's no way an interrupt service routine should loop for up to 7
> milliseconds. Why does it need to wait this long? Shouldn't this patch
> zero the transmit FIFO threshold to minimize the amount of time between
> XRDY/XDR and XUDF?
>
> > + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > + while (!(xudf & OMAP_I2C_STAT_XUDF) && counter--) {
>
> Why is a separate I2C_STAT loop used here? Shouldn't this patch just use
> the existing I2C_STAT loop in the ISR and use a state variable to indicate
> the current state of the ISR?
>
> > + if (xudf & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK |
> > + OMAP_I2C_STAT_AL))
> > + return -EINVAL;
> > + udelay(10);
>
> udelay() in an ISR is a big red flag. Why is this needed?
>
> > + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > + }
> > +
> > + if (!counter) {
> > + /* Clear Tx FIFO */
> > + omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
> > + OMAP_I2C_BUF_TXFIF_CLR);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static irqreturn_t
> > omap_i2c_isr(int this_irq, void *dev_id)
> > {
> > @@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > u16 bits;
> > u16 stat, w;
> > int err, count = 0;
> > + int error;
> >
> > if (dev->idle)
> > return IRQ_NONE;
> > @@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
> > dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> > if (count++ == 100) {
> > - dev_warn(dev->dev, "Too much work in one IRQ\n");
> > + dev_dbg(dev->dev, "Too much work in one IRQ\n");
> > break;
> > }
> >
> > @@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > OMAP_I2C_BUFSTAT_REG);
> > }
> > while (num_bytes) {
> > - num_bytes--;
> > w = 0;
> > if (dev->buf_len) {
> > + if (cpu_is_omap34xx()) {
> > + /* OMAP3430 Errata 1.153 */
>
> W
Re: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
Hello Moiz, Nishant, Jagadeesh,
Any plans to respond to these comments?
- Paul
On Mon, 22 Jun 2009, Paul Walmsley wrote:
> Moiz, Nishant, Jagadeesh,
>
> Looks like this patch has some serious problems.
>
> On Mon, 22 Jun 2009, Sonasath, Moiz wrote:
>
> > This patch includes the workarround for I2C Errata 1.153: When an
> > XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG
> >
> > Signed-off-by: Jagadeesh Bhaskar Pakaravoor
> > Signed-off by: Nishant Kamat
> > Signed-off-by: Moiz Sonasath
> > ---
> > drivers/i2c/busses/i2c-omap.c | 54
> > +---
> > 1 files changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index ece0125..e84836b 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
> > #define omap_i2c_rev1_isr NULL
> > #endif
> >
> > +/* I2C Errata 1.153:
> > + * When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> > + * Otherwise some data bytes can be lost while transferring them from the
> > + * memory to the I2C interface.
> > + */
> > +
> > +static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
> > +{
> > + u16 xudf;
> > + int counter = 500;
>
> Shouldn't the timeout threshold depend on the I2C bus speed and transmit
> FIFO threshold?
>
> > +
> > + /* We are in interrupt context. Wait for XUDF for max 7 msec */
>
> There's no way an interrupt service routine should loop for up to 7
> milliseconds. Why does it need to wait this long? Shouldn't this patch
> zero the transmit FIFO threshold to minimize the amount of time between
> XRDY/XDR and XUDF?
>
> > + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > + while (!(xudf & OMAP_I2C_STAT_XUDF) && counter--) {
>
> Why is a separate I2C_STAT loop used here? Shouldn't this patch just use
> the existing I2C_STAT loop in the ISR and use a state variable to indicate
> the current state of the ISR?
>
> > + if (xudf & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK |
> > + OMAP_I2C_STAT_AL))
> > + return -EINVAL;
> > + udelay(10);
>
> udelay() in an ISR is a big red flag. Why is this needed?
>
> > + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > + }
> > +
> > + if (!counter) {
> > + /* Clear Tx FIFO */
> > + omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
> > + OMAP_I2C_BUF_TXFIF_CLR);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static irqreturn_t
> > omap_i2c_isr(int this_irq, void *dev_id)
> > {
> > @@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > u16 bits;
> > u16 stat, w;
> > int err, count = 0;
> > + int error;
> >
> > if (dev->idle)
> > return IRQ_NONE;
> > @@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
> > dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> > if (count++ == 100) {
> > - dev_warn(dev->dev, "Too much work in one IRQ\n");
> > + dev_dbg(dev->dev, "Too much work in one IRQ\n");
> > break;
> > }
> >
> > @@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > OMAP_I2C_BUFSTAT_REG);
> > }
> > while (num_bytes) {
> > - num_bytes--;
> > w = 0;
> > if (dev->buf_len) {
> > + if (cpu_is_omap34xx()) {
> > + /* OMAP3430 Errata 1.153 */
>
> What about this I2C IP block on previous chip versions? Is this problem
> also present on 2430, which also has FIFO transmit capability?
>
> > + error =
> > omap_i2c_wait_for_xudf(dev);
> > + if (error) {
> > + omap_i2c_ack_stat(dev,
> > stat &
> > +
> > (OMAP_I2C_STAT_XRDY |
> > +
> > OMAP_I2C_STAT_XDR));
> > + dev_err(dev->dev,
> > "Transmit error\n");
> > +
> > omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_XUDF);
> > +
> > + return IRQ_HANDLED;
> > + }
> > + }
> > w = *dev->buf++;
> > - dev->buf_len--;
> >
Re: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
Menon, Nishanth wrote:
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Igor Mazanov
Sent: Tuesday, June 23, 2009 4:43 PM
To: linux-omap@vger.kernel.org
Subject: Re: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon
errata 1.153
/*
* Is there a bug in the OMAP5910 I2C controller? It
* generates a bunch of fake XRDY interrupts under high load.
* As result, there is a very high chance to have a situation
* when dev->buf_len = 0 already, but I2C_CNT != 0. So, there
* is no ARDY irq then, no complete_cmd, controller timed out
* issues...
*
* Workaround:
*
* When we get XRDY interrupt without transmit underflow flag
* (XUDF bit in the I2C_STAT register), delay for 100 microsecs
* and ignore it.
*
* We write data into I2C_DATA register in case of transmit
* underflow condition ONLY.
*/
if (stat & OMAP_I2C_STAT_XRDY) {
if (!(stat & OMAP_I2C_STAT_XUDF)) {
udelay(100);
continue;
} else {
w = 0;
if (dev->buf_len) {
w = *dev->buf++;
dev->buf_len--;
if (dev->buf_len) {
w |= *dev->buf++ << 8;
dev->buf_len--;
}
omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
} else {
dev_err(dev->dev, "XRDY IRQ while no "
"data to send\n");
break;
}
continue;
}
}
Could you submit a patch please?
OMAP1 I2C controller generates a huge amount of fake XRDY interrupts when large
continuous blocks of data are transmitted via I2C. As result data have no time
to be transmitted physically over I2C data line if we just look on XRDY bit
before writing to I2C_DATA register. Taking into account a transmit underrun
condition (XUDF bit in the I2C_STAT register) help us to transmit in more
predictable way.
So,
Signed-off-by: Igor Mazanov
---
drivers/i2c/busses/i2c-omap.c | 140
1 files changed, 98 insertions(+), 42 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ece0125..7464848 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -436,10 +436,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
- /* 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);
+ /* Clear the FIFO Buffers
+* Useless for OMAP1 family - according TRMs (spru681b, spru760c),
+* FIFO are not controllable in this way
+*/
+ if (!cpu_class_is_omap1()) {
+ 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);
+ }
init_completion(&dev->cmd_complete);
dev->cmd_err = 0;
@@ -578,55 +583,106 @@ static irqreturn_t
omap_i2c_rev1_isr(int this_irq, void *dev_id)
{
struct omap_i2c_dev *dev = dev_id;
- u16 iv, w;
+ u16 bits;
+ u16 stat, w;
+ int err, count = 0;
if (dev->idle)
return IRQ_NONE;
- iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
- switch (iv) {
- case 0x00: /* None */
- break;
- case 0x01: /* Arbitration lost */
- dev_err(dev->dev, "Arbitration lost\n");
- omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_AL);
- break;
- case 0x02: /* No acknowledgement */
- omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_NACK);
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_STP);
- break;
- case 0x03: /* Register access ready */
- omap_i2c_complete_cmd(dev, 0);
- break;
- case 0x04: /* Receive data ready */
- if (dev->buf_len) {
+ bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
+ while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
+ dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
+ if (count++ == 200) {
+ dev_warn(dev->dev, "Too much work in one IRQ\n");
+ break;
+ }
+
+ omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
+
+ err = 0;
+ if (stat & OMAP_I2C_STAT
RE: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Igor Mazanov
> Sent: Tuesday, June 23, 2009 4:43 PM
> To: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon
> errata 1.153
>
> /*
> * Is there a bug in the OMAP5910 I2C controller? It
> * generates a bunch of fake XRDY interrupts under high load.
> * As result, there is a very high chance to have a situation
> * when dev->buf_len = 0 already, but I2C_CNT != 0. So, there
> * is no ARDY irq then, no complete_cmd, controller timed out
> * issues...
> *
> * Workaround:
> *
> * When we get XRDY interrupt without transmit underflow flag
> * (XUDF bit in the I2C_STAT register), delay for 100 microsecs
> * and ignore it.
> *
> * We write data into I2C_DATA register in case of transmit
> * underflow condition ONLY.
> */
> if (stat & OMAP_I2C_STAT_XRDY) {
> if (!(stat & OMAP_I2C_STAT_XUDF)) {
> udelay(100);
> continue;
> } else {
> w = 0;
> if (dev->buf_len) {
> w = *dev->buf++;
> dev->buf_len--;
> if (dev->buf_len) {
> w |= *dev->buf++ << 8;
> dev->buf_len--;
> }
> omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> } else {
> dev_err(dev->dev, "XRDY IRQ while no "
> "data to send\n");
> break;
> }
> continue;
> }
> }
>
Could you submit a patch please?
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] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
Hello,
Menon, Nishanth wrote:
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Sonasath, Moiz
Sent: Monday, June 22, 2009 7:50 PM
To: linux-omap@vger.kernel.org
Cc: Pandita, Vikram
Subject: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon
errata 1.153
This patch includes the workarround for I2C Errata 1.153: When an XRDY/XDR
is hit, wait for XUDF before writing data to DATA_REG
Is this workaround valid for omap2430 also?
Some kind of such workaround needs to be applied and for OMAP1 ISR too.
I had
the same problem on our OMAP5910 based custom made board. While writing
a large contiguous amount of data constantly occurs a situation when
dev->buf_len = 0, but I2C_CNT register != 0, and, as result, I2C
controller doesn't generate ARDY IRQ, no complete_cmd occurs in ISR, and
we get "controller timed out" issues then...
So, here is a part of modified OMAP1 ISR. It works for me at least.
/*
* Is there a bug in the OMAP5910 I2C controller? It
* generates a bunch of fake XRDY interrupts under high load.
* As result, there is a very high chance to have a situation
* when dev->buf_len = 0 already, but I2C_CNT != 0. So, there
* is no ARDY irq then, no complete_cmd, controller timed out
* issues...
*
* Workaround:
*
* When we get XRDY interrupt without transmit underflow flag
* (XUDF bit in the I2C_STAT register), delay for 100 microsecs
* and ignore it.
*
* We write data into I2C_DATA register in case of transmit
* underflow condition ONLY.
*/
if (stat & OMAP_I2C_STAT_XRDY) {
if (!(stat & OMAP_I2C_STAT_XUDF)) {
udelay(100);
continue;
} else {
w = 0;
if (dev->buf_len) {
w = *dev->buf++;
dev->buf_len--;
if (dev->buf_len) {
w |= *dev->buf++ << 8;
dev->buf_len--;
}
omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
} else {
dev_err(dev->dev, "XRDY IRQ while no "
"data to send\n");
break;
}
continue;
}
}
Regards,
Igor.
--
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] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Kamat, Nishant
> Sent: Tuesday, June 23, 2009 12:55 PM
>
> > > @@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > > while ((stat = (omap_i2c_read_reg(dev,
> > OMAP_I2C_STAT_REG))) & bits) {
> > > dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> > > if (count++ == 100) {
> > > - dev_warn(dev->dev, "Too much work in
> > one IRQ\n");
> > > + dev_dbg(dev->dev, "Too much work in one IRQ\n");
> >
> > Should stay as dev_warn I think.
> >
>
> When I2C is used to transfer a large number of bytes continuously (e.g.
> during some camera sensor firmware update), we hit the max count more
> often now (because of the delay introduced by the workaround
> implementation). In this case, its undesirable to see the dev_warn
> messages fill up the console. Changing this to dev_dbg means that this
> message is not printed in the expected case.
Just wondering on this -> cant I do a multibyte write upto 255 bytes? Is
count==100 not enough given that we used buffered writes? The real question is
this:
Are devices expected to trigger retry logic to the extent where the error
condition is triggered?
I think this might be an indication of something else being at fault with the
sensor /configuration of sensor - hiding the error messages by moving warn->dbg
is not correct IMHO.
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] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
Kevin,
> -Original Message-
> From: Kevin Hilman
>
> "Sonasath, Moiz" writes:
>
> > This patch includes the workarround for I2C Errata 1.153:
[...]
> > +static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
> > +{
> > + u16 xudf;
> > + int counter = 500;
> > +
> > + /* We are in interrupt context. Wait for XUDF for max 7 msec */
>
> What does being in interrupt context have to do with how long you
> wait? Threaded interrupts are now in mainline and will become the
> default, so this ISR may run in thread context.
The interrupt context comment was meant to say that we can't sleep. Perhaps,
with threaded interrupts that might not be true (I am not sure). We can remove
the interrupt context comment.
> > @@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > while ((stat = (omap_i2c_read_reg(dev,
> OMAP_I2C_STAT_REG))) & bits) {
> > dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> > if (count++ == 100) {
> > - dev_warn(dev->dev, "Too much work in
> one IRQ\n");
> > + dev_dbg(dev->dev, "Too much work in one IRQ\n");
>
> Should stay as dev_warn I think.
>
When I2C is used to transfer a large number of bytes continuously (e.g. during
some camera sensor firmware update), we hit the max count more often now
(because of the delay introduced by the workaround implementation). In this
case, its undesirable to see the dev_warn messages fill up the console.
Changing this to dev_dbg means that this message is not printed in the expected
case.
Regards,
Nishant
--
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] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
> -Original Message- > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > ow...@vger.kernel.org] On Behalf Of Sonasath, Moiz > Sent: Monday, June 22, 2009 7:50 PM > To: linux-omap@vger.kernel.org > Cc: Pandita, Vikram > Subject: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon > errata 1.153 > > This patch includes the workarround for I2C Errata 1.153: When an XRDY/XDR > is hit, wait for XUDF before writing data to DATA_REG Is this workaround valid for omap2430 also? 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] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
Moiz, Nishant, Jagadeesh,
Looks like this patch has some serious problems.
On Mon, 22 Jun 2009, Sonasath, Moiz wrote:
> This patch includes the workarround for I2C Errata 1.153: When an
> XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG
>
> Signed-off-by: Jagadeesh Bhaskar Pakaravoor
> Signed-off by: Nishant Kamat
> Signed-off-by: Moiz Sonasath
> ---
> drivers/i2c/busses/i2c-omap.c | 54 +---
> 1 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ece0125..e84836b 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
> #define omap_i2c_rev1_isrNULL
> #endif
>
> +/* I2C Errata 1.153:
> + * When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> + * Otherwise some data bytes can be lost while transferring them from the
> + * memory to the I2C interface.
> + */
> +
> +static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
> +{
> + u16 xudf;
> + int counter = 500;
Shouldn't the timeout threshold depend on the I2C bus speed and transmit
FIFO threshold?
> +
> + /* We are in interrupt context. Wait for XUDF for max 7 msec */
There's no way an interrupt service routine should loop for up to 7
milliseconds. Why does it need to wait this long? Shouldn't this patch
zero the transmit FIFO threshold to minimize the amount of time between
XRDY/XDR and XUDF?
> + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> + while (!(xudf & OMAP_I2C_STAT_XUDF) && counter--) {
Why is a separate I2C_STAT loop used here? Shouldn't this patch just use
the existing I2C_STAT loop in the ISR and use a state variable to indicate
the current state of the ISR?
> + if (xudf & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK |
> + OMAP_I2C_STAT_AL))
> + return -EINVAL;
> + udelay(10);
udelay() in an ISR is a big red flag. Why is this needed?
> + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> + }
> +
> + if (!counter) {
> + /* Clear Tx FIFO */
> + omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
> + OMAP_I2C_BUF_TXFIF_CLR);
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> static irqreturn_t
> omap_i2c_isr(int this_irq, void *dev_id)
> {
> @@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> u16 bits;
> u16 stat, w;
> int err, count = 0;
> + int error;
>
> if (dev->idle)
> return IRQ_NONE;
> @@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
> dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> if (count++ == 100) {
> - dev_warn(dev->dev, "Too much work in one IRQ\n");
> + dev_dbg(dev->dev, "Too much work in one IRQ\n");
> break;
> }
>
> @@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id)
> OMAP_I2C_BUFSTAT_REG);
> }
> while (num_bytes) {
> - num_bytes--;
> w = 0;
> if (dev->buf_len) {
> + if (cpu_is_omap34xx()) {
> + /* OMAP3430 Errata 1.153 */
What about this I2C IP block on previous chip versions? Is this problem
also present on 2430, which also has FIFO transmit capability?
> + error =
> omap_i2c_wait_for_xudf(dev);
> + if (error) {
> + omap_i2c_ack_stat(dev,
> stat &
> +
> (OMAP_I2C_STAT_XRDY |
> +
> OMAP_I2C_STAT_XDR));
> + dev_err(dev->dev,
> "Transmit error\n");
> +
> omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_XUDF);
> +
> + return IRQ_HANDLED;
> + }
> + }
> w = *dev->buf++;
> - dev->buf_len--;
> /* Data reg from 2430 is 8 bit wide */
> if (!cpu_is_omap2430() &&
> !cpu_is_omap34xx()) {
> @@ -728,6 +771,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
>
Re: [PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
"Sonasath, Moiz" writes:
> This patch includes the workarround for I2C Errata 1.153: When an XRDY/XDR is
> hit, wait for XUDF before writing data to DATA_REG
How was this tested/validated, on what platforms etc.
> Signed-off-by: Jagadeesh Bhaskar Pakaravoor
> Signed-off by: Nishant Kamat
> Signed-off-by: Moiz Sonasath
> ---
> drivers/i2c/busses/i2c-omap.c | 54 +---
> 1 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ece0125..e84836b 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
> #define omap_i2c_rev1_isrNULL
> #endif
>
> +/* I2C Errata 1.153:
> + * When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> + * Otherwise some data bytes can be lost while transferring them from the
> + * memory to the I2C interface.
> + */
> +
> +static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
> +{
> + u16 xudf;
> + int counter = 500;
> +
> + /* We are in interrupt context. Wait for XUDF for max 7 msec */
What does being in interrupt context have to do with how long you
wait? Threaded interrupts are now in mainline and will become the
default, so this ISR may run in thread context.
> + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> + while (!(xudf & OMAP_I2C_STAT_XUDF) && counter--) {
> + if (xudf & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK |
> + OMAP_I2C_STAT_AL))
> + return -EINVAL;
> + udelay(10);
> + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> + }
> +
> + if (!counter) {
> + /* Clear Tx FIFO */
> + omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
> + OMAP_I2C_BUF_TXFIF_CLR);
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> static irqreturn_t
> omap_i2c_isr(int this_irq, void *dev_id)
> {
> @@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> u16 bits;
> u16 stat, w;
> int err, count = 0;
> + int error;
>
> if (dev->idle)
> return IRQ_NONE;
> @@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
> dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> if (count++ == 100) {
> - dev_warn(dev->dev, "Too much work in one IRQ\n");
> + dev_dbg(dev->dev, "Too much work in one IRQ\n");
Should stay as dev_warn I think.
> break;
> }
>
> @@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id)
> OMAP_I2C_BUFSTAT_REG);
> }
> while (num_bytes) {
> - num_bytes--;
> w = 0;
> if (dev->buf_len) {
> + if (cpu_is_omap34xx()) {
> + /* OMAP3430 Errata 1.153 */
> + error =
> omap_i2c_wait_for_xudf(dev);
> + if (error) {
> + omap_i2c_ack_stat(dev,
> stat &
> +
> (OMAP_I2C_STAT_XRDY |
> +
> OMAP_I2C_STAT_XDR));
> + dev_err(dev->dev,
> "Transmit error\n");
> +
> omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_XUDF);
> +
> + return IRQ_HANDLED;
> + }
> + }
Yuck, too much wrapping here. Running through checkpatch would've warned
you about this. Looks like you need to break this out into a subroutine.
> w = *dev->buf++;
> - dev->buf_len--;
> /* Data reg from 2430 is 8 bit wide */
> if (!cpu_is_omap2430() &&
> !cpu_is_omap34xx()) {
> @@ -728,6 +771,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
> dev->buf_len--;
> }
> }
> + omap_i2c_write_reg(dev,
> + OMAP_I2C_DATA_REG, w);
> + num_bytes--;
> +
[PATCH] [RFC][OMAP3:I2C]Workaround for OMAP3430 I2C silicon errata 1.153
This patch includes the workarround for I2C Errata 1.153: When an XRDY/XDR is
hit, wait for XUDF before writing data to DATA_REG
Signed-off-by: Jagadeesh Bhaskar Pakaravoor
Signed-off by: Nishant Kamat
Signed-off-by: Moiz Sonasath
---
drivers/i2c/busses/i2c-omap.c | 54 +---
1 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ece0125..e84836b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
#define omap_i2c_rev1_isr NULL
#endif
+/* I2C Errata 1.153:
+ * When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
+ * Otherwise some data bytes can be lost while transferring them from the
+ * memory to the I2C interface.
+ */
+
+static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev)
+{
+ u16 xudf;
+ int counter = 500;
+
+ /* We are in interrupt context. Wait for XUDF for max 7 msec */
+ xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ while (!(xudf & OMAP_I2C_STAT_XUDF) && counter--) {
+ if (xudf & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK |
+ OMAP_I2C_STAT_AL))
+ return -EINVAL;
+ udelay(10);
+ xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ }
+
+ if (!counter) {
+ /* Clear Tx FIFO */
+ omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
+ OMAP_I2C_BUF_TXFIF_CLR);
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
static irqreturn_t
omap_i2c_isr(int this_irq, void *dev_id)
{
@@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
u16 bits;
u16 stat, w;
int err, count = 0;
+ int error;
if (dev->idle)
return IRQ_NONE;
@@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
if (count++ == 100) {
- dev_warn(dev->dev, "Too much work in one IRQ\n");
+ dev_dbg(dev->dev, "Too much work in one IRQ\n");
break;
}
@@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id)
OMAP_I2C_BUFSTAT_REG);
}
while (num_bytes) {
- num_bytes--;
w = 0;
if (dev->buf_len) {
+ if (cpu_is_omap34xx()) {
+ /* OMAP3430 Errata 1.153 */
+ error =
omap_i2c_wait_for_xudf(dev);
+ if (error) {
+ omap_i2c_ack_stat(dev,
stat &
+
(OMAP_I2C_STAT_XRDY |
+
OMAP_I2C_STAT_XDR));
+ dev_err(dev->dev,
"Transmit error\n");
+
omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_XUDF);
+
+ return IRQ_HANDLED;
+ }
+ }
w = *dev->buf++;
- dev->buf_len--;
/* Data reg from 2430 is 8 bit wide */
if (!cpu_is_omap2430() &&
!cpu_is_omap34xx()) {
@@ -728,6 +771,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
dev->buf_len--;
}
}
+ omap_i2c_write_reg(dev,
+ OMAP_I2C_DATA_REG, w);
+ num_bytes--;
+ dev->buf_len--;
} else {
if (stat & OMAP_I2C_STAT_XRDY)
dev_err(dev->dev,
@@ -739,7 +786,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
"data to send\n");
break;
}
- omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
}
