[PATCH v2 0/2] omap i2c interrupt handler fixes

[PATCH v2 0/2] omap i2c interrupt handler fixes

[Alexander Shishkin]

Hi,

This is the second version of the patchset, now addressing comments
from Nishanth and changing the retry counter into a real timeout.

Regards,
--
Alex

[PATCH v2 1/2] omap i2c: make errata 1.153 workaround a separate function

[Alexander Shishkin]

This is to avoid insanely long lines and levels of indentation.

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: linux-i2c@vger.kernel.org
CC: linux-omap@vger.kernel.org
CC: nm@ti.com
---
 drivers/i2c/busses/i2c-omap.c |   43 ++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 75bf3ad..2d146ac 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -671,6 +671,27 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
 #define omap_i2c_rev1_isr		NULL
 #endif
 
+/*
+ * OMAP3430 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 errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
+{
+	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
+		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
+							OMAP_I2C_STAT_XDR));
+			*err |= OMAP_I2C_STAT_XUDF;
+			return -ETIMEDOUT;
+		}
+		cpu_relax();
+		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+	}
+
+	return 0;
+}
+
 static irqreturn_t
 omap_i2c_isr(int this_irq, void *dev_id)
 {
@@ -794,25 +815,9 @@ complete:
 					break;
 				}
 
-				/*
-				 * OMAP3430 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.
-				 */
-
-				if (dev->rev <= OMAP_I2C_REV_ON_3430) {
-						while (!(stat & OMAP_I2C_STAT_XUDF)) {
-							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
-								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-								err |= OMAP_I2C_STAT_XUDF;
-								goto complete;
-							}
-							cpu_relax();
-							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
-						}
-				}
+				if ((dev->rev <= OMAP_I2C_REV_ON_3430) &&
+				    errata_omap3_1p153(dev, &stat, &err))
+					goto complete;
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-- 
1.6.3.3

[PATCH v2 2/2] omap i2c: add a timeout to the busy waiting

[Alexander Shishkin]

The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
context, which may lead to kernel hangs. The problem can be reproduced
by running the bus with wrong (too high) speed.

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: linux-i2c@vger.kernel.org
CC: linux-omap@vger.kernel.org
CC: nm@ti.com
---
 drivers/i2c/busses/i2c-omap.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 2d146ac..7d56a25 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -678,6 +678,8 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
  */
 static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
 {
+	unsigned long timeout = jiffies + msecs_to_jiffies(1);
+
 	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
 			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
@@ -685,6 +687,12 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
 			*err |= OMAP_I2C_STAT_XUDF;
 			return -ETIMEDOUT;
 		}
+
+		if (time_after(jiffies, timeout)) {
+			dev_err(dev->dev, "timeout waiting on XUDF bit\n");
+			return 0;
+		}
+
 		cpu_relax();
 		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 	}
-- 
1.6.3.3

Re: [PATCH v2 2/2] omap i2c: add a timeout to the busy waiting

[Tony Lindgren]

* Alexander Shishkin <virtuoso@slind.org> [091221 03:30]:
> The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
> context, which may lead to kernel hangs. The problem can be reproduced
> by running the bus with wrong (too high) speed.
> 
> Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
> CC: linux-i2c@vger.kernel.org
> CC: linux-omap@vger.kernel.org
> CC: nm@ti.com

Acked-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/i2c/busses/i2c-omap.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 2d146ac..7d56a25 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -678,6 +678,8 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
>   */
>  static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
>  {
> +	unsigned long timeout = jiffies + msecs_to_jiffies(1);
> +
>  	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
>  		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
>  			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
> @@ -685,6 +687,12 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
>  			*err |= OMAP_I2C_STAT_XUDF;
>  			return -ETIMEDOUT;
>  		}
> +
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(dev->dev, "timeout waiting on XUDF bit\n");
> +			return 0;
> +		}
> +
>  		cpu_relax();
>  		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>  	}
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] omap i2c: make errata 1.153 workaround a separate function

[Tony Lindgren]

* Alexander Shishkin <virtuoso-0lOfPCoBze7YtjvyW6yDsg@public.gmane.org> [091221 03:29]:
> This is to avoid insanely long lines and levels of indentation.
> 
> Signed-off-by: Alexander Shishkin <virtuoso-0lOfPCoBze7YtjvyW6yDsg@public.gmane.org>
> CC: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> CC: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> CC: nm-l0cyMroinI0@public.gmane.org

Nice, this driver sure could use some clean-up.

Acked-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

> ---
>  drivers/i2c/busses/i2c-omap.c |   43 ++++++++++++++++++++++------------------
>  1 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 75bf3ad..2d146ac 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -671,6 +671,27 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
>  #define omap_i2c_rev1_isr		NULL
>  #endif
>  
> +/*
> + * OMAP3430 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 errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
> +{
> +	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
> +		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> +			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
> +							OMAP_I2C_STAT_XDR));
> +			*err |= OMAP_I2C_STAT_XUDF;
> +			return -ETIMEDOUT;
> +		}
> +		cpu_relax();
> +		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +	}
> +
> +	return 0;
> +}
> +
>  static irqreturn_t
>  omap_i2c_isr(int this_irq, void *dev_id)
>  {
> @@ -794,25 +815,9 @@ complete:
>  					break;
>  				}
>  
> -				/*
> -				 * OMAP3430 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.
> -				 */
> -
> -				if (dev->rev <= OMAP_I2C_REV_ON_3430) {
> -						while (!(stat & OMAP_I2C_STAT_XUDF)) {
> -							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> -								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> -								err |= OMAP_I2C_STAT_XUDF;
> -								goto complete;
> -							}
> -							cpu_relax();
> -							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> -						}
> -				}
> +				if ((dev->rev <= OMAP_I2C_REV_ON_3430) &&
> +				    errata_omap3_1p153(dev, &stat, &err))
> +					goto complete;
>  
>  				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
>  			}
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] omap i2c: make errata 1.153 workaround a separate function

[Alexander Shishkin]

On Mon, Dec 21, 2009 at 01:29:58 +0200, Alexander Shishkin wrote:
> This is to avoid insanely long lines and levels of indentation.

Ben, do you want me to resend these with Tony's acks or can you consider
applying them?

Regards,
--
Alex