| Message ID | CD8CC2B65FEE304DA95744A5472698F2028A41187E@dlee06.ent.ti.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
"Sonasath, Moiz" <m-sonasath@ti.com> 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 <j-pakaravoor@ti.com> > Signed-off by: Nishant Kamat <nskamat@ti.com> > Signed-off-by: Moiz Sonasath<m-sonasath@ti.com> > --- > 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 */ 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--; > + 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); > } > omap_i2c_ack_stat(dev, > stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); > -- > 1.5.6.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 -- 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
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 <j-pakaravoor@ti.com> > Signed-off by: Nishant Kamat <nskamat@ti.com> > Signed-off-by: Moiz Sonasath<m-sonasath@ti.com> > --- > 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--; > /* 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); > } > omap_i2c_ack_stat(dev, > stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); - 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
> -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kevin, > -----Original Message----- > From: Kevin Hilman > > "Sonasath, Moiz" <m-sonasath@ti.com> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Menon, Nishanth wrote: >> -----Original Message----- >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- >> owner@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <j-pakaravoor@ti.com> > > Signed-off by: Nishant Kamat <nskamat@ti.com> > > Signed-off-by: Moiz Sonasath<m-sonasath@ti.com> > > --- > > 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--; > > /* 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); > > } > > omap_i2c_ack_stat(dev, > > stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); > > > - 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 > -- 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
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:paul@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 <j-pakaravoor@ti.com> > > Signed-off by: Nishant Kamat <nskamat@ti.com> > > Signed-off-by: Moiz Sonasath<m-sonasath@ti.com> > > --- > > 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--; > > /* 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); > > } > > omap_i2c_ack_stat(dev, > > stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); > > > - 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 > -- 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
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); } omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));