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@xxxxxxxxx] Sent: Wednesday, July 08, 2009 11:51 AM To: Sonasath, Moiz Cc: linux-omap@xxxxxxxxxxxxxxx; 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@xxxxxx> > > Signed-off by: Nishant Kamat <nskamat@xxxxxx> > > Signed-off-by: Moiz Sonasath<m-sonasath@xxxxxx> > > --- > > 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
