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
