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
