diff mbox

[v2] i2c: tegra: Check for overflow errors with BUG_ON.

Message ID 1313458336-31456-1-git-send-email-dianders@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Doug Anderson Aug. 16, 2011, 1:32 a.m. UTC 
This change doesn't fix any known problems but turns
on the overflow detection feature of the i2c controller
in the hopes of flushing out any current (or future)
bugs in the i2c driver.

Inspired by a change on nvidia's git server:
  http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commit;h=266d1b7397284505e55d06254b497cb32be07b69

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
This takes Felipe Balbi's suggestion of making the overflow
error non-fatal.

 drivers/i2c/busses/i2c-tegra.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

Comments

Stephen Warren Aug. 16, 2011, 2:26 a.m. UTC | #1 
Doug Anderson wrote at Monday, August 15, 2011 7:32 PM:
> This change doesn't fix any known problems but turns
> on the overflow detection feature of the i2c controller
> in the hopes of flushing out any current (or future)
> bugs in the i2c driver.
> 
> Inspired by a change on nvidia's git server:
>   http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commit;h=266d1b7397284505e55d06254b497cb32be07b69
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> This takes Felipe Balbi's suggestion of making the overflow
> error non-fatal.

This seems like a reasonable debug aid/validation check to me;
Acked-by: Stephen Warren <swarren@nvidia.com>
Felipe Balbi Aug. 16, 2011, 6:34 a.m. UTC | #2 
On Mon, Aug 15, 2011 at 06:32:16PM -0700, Doug Anderson wrote:
> This change doesn't fix any known problems but turns
> on the overflow detection feature of the i2c controller
> in the hopes of flushing out any current (or future)
> bugs in the i2c driver.
> 
> Inspired by a change on nvidia's git server:
>   http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commit;h=266d1b7397284505e55d06254b497cb32be07b69
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
> This takes Felipe Balbi's suggestion of making the overflow
> error non-fatal.

the WARN() was very nice idea, I was thinking of just returning an error
to the client, but the adition of WARN() will make it really bold ;-)
Rhyland Klein Aug. 16, 2011, 6 p.m. UTC | #3 
On Mon, 2011-08-15 at 18:32 -0700, Doug Anderson wrote:
> This change doesn't fix any known problems but turns
> on the overflow detection feature of the i2c controller
> in the hopes of flushing out any current (or future)
> bugs in the i2c driver.
> 
> Inspired by a change on nvidia's git server:
>   http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commit;h=266d1b7397284505e55d06254b497cb32be07b69
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> This takes Felipe Balbi's suggestion of making the overflow
> error non-fatal.
> 
>  drivers/i2c/busses/i2c-tegra.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 2440b74..34634b7 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -83,6 +83,7 @@
>  #define I2C_ERR_NO_ACK				0x01
>  #define I2C_ERR_ARBITRATION_LOST		0x02
>  #define I2C_ERR_UNKNOWN_INTERRUPT		0x04
> +#define I2C_ERR_FIFO_OVERFLOW			0x08
>  
>  #define PACKET_HEADER0_HEADER_SIZE_SHIFT	28
>  #define PACKET_HEADER0_PACKET_ID_SHIFT		16
> @@ -367,7 +368,8 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  {
>  	u32 status;
> -	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> +	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
> +		I2C_INT_TX_FIFO_OVERFLOW;
>  	struct tegra_i2c_dev *i2c_dev = dev_id;
>  
>  	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
> @@ -389,6 +391,14 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	}
>  
>  	if (unlikely(status & status_err)) {
> +		/*
> +		 * Overflow should be impossible unless we've got a
> +		 * bug in the driver, but check anyway.
> +		 */
> +		if (status & I2C_INT_TX_FIFO_OVERFLOW) {
> +			WARN_ON(1);
> +			i2c_dev->msg_err |= I2C_ERR_FIFO_OVERFLOW;
> +		}
>  		if (status & I2C_INT_NO_ACK)
>  			i2c_dev->msg_err |= I2C_ERR_NO_ACK;
>  		if (status & I2C_INT_ARBITRATION_LOST)
> @@ -423,7 +433,7 @@ err:
>  	/* An error occurred, mask all interrupts */
>  	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
>  		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
> -		I2C_INT_RX_FIFO_DATA_REQ);
> +		I2C_INT_RX_FIFO_DATA_REQ | I2C_INT_TX_FIFO_OVERFLOW);
>  	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
>  	if (i2c_dev->is_dvc)
>  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
> @@ -473,7 +483,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	if (!(msg->flags & I2C_M_RD))
>  		tegra_i2c_fill_tx_fifo(i2c_dev);
>  
> -	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> +	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
> +		I2C_INT_TX_FIFO_OVERFLOW;
>  	if (msg->flags & I2C_M_RD)
>  		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
>  	else if (i2c_dev->msg_buf_remaining)

Seems good to me.

Acked-by: Rhyland Klein <rklein@nvidia.com>
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2440b74..34634b7 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -83,6 +83,7 @@ 
 #define I2C_ERR_NO_ACK				0x01
 #define I2C_ERR_ARBITRATION_LOST		0x02
 #define I2C_ERR_UNKNOWN_INTERRUPT		0x04
+#define I2C_ERR_FIFO_OVERFLOW			0x08
 
 #define PACKET_HEADER0_HEADER_SIZE_SHIFT	28
 #define PACKET_HEADER0_PACKET_ID_SHIFT		16
@@ -367,7 +368,8 @@  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 {
 	u32 status;
-	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
+		I2C_INT_TX_FIFO_OVERFLOW;
 	struct tegra_i2c_dev *i2c_dev = dev_id;
 
 	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
@@ -389,6 +391,14 @@  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	}
 
 	if (unlikely(status & status_err)) {
+		/*
+		 * Overflow should be impossible unless we've got a
+		 * bug in the driver, but check anyway.
+		 */
+		if (status & I2C_INT_TX_FIFO_OVERFLOW) {
+			WARN_ON(1);
+			i2c_dev->msg_err |= I2C_ERR_FIFO_OVERFLOW;
+		}
 		if (status & I2C_INT_NO_ACK)
 			i2c_dev->msg_err |= I2C_ERR_NO_ACK;
 		if (status & I2C_INT_ARBITRATION_LOST)
@@ -423,7 +433,7 @@  err:
 	/* An error occurred, mask all interrupts */
 	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
 		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
-		I2C_INT_RX_FIFO_DATA_REQ);
+		I2C_INT_RX_FIFO_DATA_REQ | I2C_INT_TX_FIFO_OVERFLOW);
 	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
@@ -473,7 +483,8 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (!(msg->flags & I2C_M_RD))
 		tegra_i2c_fill_tx_fifo(i2c_dev);
 
-	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
+		I2C_INT_TX_FIFO_OVERFLOW;
 	if (msg->flags & I2C_M_RD)
 		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
 	else if (i2c_dev->msg_buf_remaining)