[PATCH] I2C-MPC: Fix up error handling

[PATCH] I2C-MPC: Fix up error handling

[Kumar Gala]

* If we have an Unfinished (MCF) or Arbitration Lost (MAL) error and
  the bus is still busy reset the controller.  This prevents the
  controller from getting in a hung state for transactions for other
  devices.

* Fixed up propogating the errors from i2c_wait.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

---
commit 17ce1b687bda4abb8ff0989b63a140225c33de09
tree 0463714babd656b80a032ed5b9ec6f42c42ef2e5
parent 735344d2f587938da9012070f881b725269c4dc9
author Kumar Gala <galak@kernel.crashing.org> Tue, 18 Apr 2006 11:24:28 -0500
committer Kumar Gala <galak@kernel.crashing.org> Tue, 18 Apr 2006 11:24:28 -0500

 drivers/i2c/busses/i2c-mpc.c |   43 ++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 2721e4c..8d2c866 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -115,11 +115,20 @@ static int i2c_wait(struct mpc_i2c *i2c,
 
 	if (!(x & CSR_MCF)) {
 		pr_debug("I2C: unfinished\n");
+
+		/* reset the controller if the bus is still busy */
+		if (x & CSR_MBB)
+			writeccr(i2c, 0);
+
 		return -EIO;
 	}
 
 	if (x & CSR_MAL) {
 		pr_debug("I2C: MAL\n");
+
+		/* reset the controller if the bus is still busy */
+		if (x & CSR_MBB)
+			writeccr(i2c, 0);
 		return -EIO;
 	}
 
@@ -160,7 +169,7 @@ static void mpc_i2c_stop(struct mpc_i2c 
 static int mpc_write(struct mpc_i2c *i2c, int target,
 		     const u8 * data, int length, int restart)
 {
-	int i;
+	int i, ret;
 	unsigned timeout = i2c->adap.timeout;
 	u32 flags = restart ? CCR_RSTA : 0;
 
@@ -172,15 +181,17 @@ static int mpc_write(struct mpc_i2c *i2c
 	/* Write target byte */
 	writeb((target << 1), i2c->base + MPC_I2C_DR);
 
-	if (i2c_wait(i2c, timeout, 1) < 0)
-		return -1;
+	ret = i2c_wait(i2c, timeout, 1);
+	if (ret < 0)
+		return ret;
 
 	for (i = 0; i < length; i++) {
 		/* Write data byte */
 		writeb(data[i], i2c->base + MPC_I2C_DR);
 
-		if (i2c_wait(i2c, timeout, 1) < 0)
-			return -1;
+		ret = i2c_wait(i2c, timeout, 1);
+		if (ret < 0)
+			return ret;
 	}
 
 	return 0;
@@ -190,7 +201,7 @@ static int mpc_read(struct mpc_i2c *i2c,
 		    u8 * data, int length, int restart)
 {
 	unsigned timeout = i2c->adap.timeout;
-	int i;
+	int i, ret;
 	u32 flags = restart ? CCR_RSTA : 0;
 
 	/* Start with MEN */
@@ -201,8 +212,9 @@ static int mpc_read(struct mpc_i2c *i2c,
 	/* Write target address byte - this time with the read flag set */
 	writeb((target << 1) | 1, i2c->base + MPC_I2C_DR);
 
-	if (i2c_wait(i2c, timeout, 1) < 0)
-		return -1;
+	ret = i2c_wait(i2c, timeout, 1);
+	if (ret < 0)
+		return ret;
 
 	if (length) {
 		if (length == 1)
@@ -214,8 +226,9 @@ static int mpc_read(struct mpc_i2c *i2c,
 	}
 
 	for (i = 0; i < length; i++) {
-		if (i2c_wait(i2c, timeout, 0) < 0)
-			return -1;
+		ret = i2c_wait(i2c, timeout, 0);
+		if (ret < 0)
+			return ret;
 
 		/* Generate txack on next to last byte */
 		if (i == length - 2)
@@ -246,8 +259,13 @@ static int mpc_xfer(struct i2c_adapter *
 			return -EINTR;
 		}
 		if (time_after(jiffies, orig_jiffies + HZ)) {
-			pr_debug("I2C: timeout\n");
-			return -EIO;
+			writeccr(i2c, 0);
+
+			/* try one more time before we error */
+			if (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
+				pr_debug("I2C: timeout\n");
+				return -EIO;
+			}
 		}
 		schedule();
 	}
@@ -325,6 +343,7 @@ static int fsl_i2c_probe(struct platform
 			goto fail_irq;
 		}
 
+	writeccr(i2c, 0);
 	mpc_i2c_setclock(i2c);
 	platform_set_drvdata(pdev, i2c);
 

Re: [PATCH] I2C-MPC: Fix up error handling

[Jean Delvare]

Hi Kumar,

Is there a datasheet available for this chip?

> * If we have an Unfinished (MCF) or Arbitration Lost (MAL) error and
>   the bus is still busy reset the controller.  This prevents the
>   controller from getting in a hung state for transactions for other
>   devices.

What "other devices" are you talking about? If the _bus_ is busy, it
might be caused by any chip on the bus. Resetting the controller may or
may not help. But it's hard for me to say more without technical
documentation. Can you explain what the CSR_MBB bit means exactly?
Please also explain the scenario you are trying to address here.

> * Fixed up propogating the errors from i2c_wait.

Yes, I like this.

> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -115,11 +115,20 @@ static int i2c_wait(struct mpc_i2c *i2c,
>  
>  	if (!(x & CSR_MCF)) {
>  		pr_debug("I2C: unfinished\n");
> +
> +		/* reset the controller if the bus is still busy */
> +		if (x & CSR_MBB)
> +			writeccr(i2c, 0);
> +
>  		return -EIO;
>  	}
>  
>  	if (x & CSR_MAL) {
>  		pr_debug("I2C: MAL\n");
> +
> +		/* reset the controller if the bus is still busy */
> +		if (x & CSR_MBB)
> +			writeccr(i2c, 0);
>  		return -EIO;
>  	}
>  

Please try being consistent with your blank lines.

> @@ -246,8 +259,13 @@ static int mpc_xfer(struct i2c_adapter *
>  			return -EINTR;
>  		}
>  		if (time_after(jiffies, orig_jiffies + HZ)) {
> -			pr_debug("I2C: timeout\n");
> -			return -EIO;
> +			writeccr(i2c, 0);
> +
> +			/* try one more time before we error */
> +			if (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
> +				pr_debug("I2C: timeout\n");
> +				return -EIO;
> +			}
>  		}
>  		schedule();
>  	}
> @@ -325,6 +343,7 @@ static int fsl_i2c_probe(struct platform
>  			goto fail_irq;
>  		}
>  
> +	writeccr(i2c, 0);
>  	mpc_i2c_setclock(i2c);
>  	platform_set_drvdata(pdev, i2c);

These last two changes are not mentioned in your header comment. What
are they? Why are they needed? They look like hacks to me.

Thanks,
-- 
Jean Delvare

Re: [PATCH] I2C-MPC: Fix up error handling

[Kumar Gala]

On Apr 25, 2006, at 8:22 AM, Jean Delvare wrote:

> Hi Kumar,
>
> Is there a datasheet available for this chip?
>
>> * If we have an Unfinished (MCF) or Arbitration Lost (MAL) error and
>>   the bus is still busy reset the controller.  This prevents the
>>   controller from getting in a hung state for transactions for other
>>   devices.
>
> What "other devices" are you talking about? If the _bus_ is busy, it
> might be caused by any chip on the bus. Resetting the controller  
> may or
> may not help. But it's hard for me to say more without technical
> documentation. Can you explain what the CSR_MBB bit means exactly?
> Please also explain the scenario you are trying to address here.

Here's the definintion of CSR_MBB:

Bus busy. Indicates the status of the bus. When a START condition is  
detected, MBB is set. If a STOP condition
is detected, it is cleared.

What I meant is that I have a I2C slave device which is hanging up  
the bus on some transactions.  In those cases we will end up in one  
of the error conditions CSR_MCF, or CSR_MAL.  If I don't reset the  
controller all future transactions regardless of which device they  
are to fail.

>> * Fixed up propogating the errors from i2c_wait.
>
> Yes, I like this.
>
>> --- a/drivers/i2c/busses/i2c-mpc.c
>> +++ b/drivers/i2c/busses/i2c-mpc.c
>> @@ -115,11 +115,20 @@ static int i2c_wait(struct mpc_i2c *i2c,
>>
>>  	if (!(x & CSR_MCF)) {
>>  		pr_debug("I2C: unfinished\n");
>> +
>> +		/* reset the controller if the bus is still busy */
>> +		if (x & CSR_MBB)
>> +			writeccr(i2c, 0);
>> +
>>  		return -EIO;
>>  	}
>>
>>  	if (x & CSR_MAL) {
>>  		pr_debug("I2C: MAL\n");
>> +
>> +		/* reset the controller if the bus is still busy */
>> +		if (x & CSR_MBB)
>> +			writeccr(i2c, 0);
>>  		return -EIO;
>>  	}
>>
>
> Please try being consistent with your blank lines.
>
>> @@ -246,8 +259,13 @@ static int mpc_xfer(struct i2c_adapter *
>>  			return -EINTR;
>>  		}
>>  		if (time_after(jiffies, orig_jiffies + HZ)) {
>> -			pr_debug("I2C: timeout\n");
>> -			return -EIO;
>> +			writeccr(i2c, 0);
>> +
>> +			/* try one more time before we error */
>> +			if (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
>> +				pr_debug("I2C: timeout\n");
>> +				return -EIO;
>> +			}
>>  		}
>>  		schedule();
>>  	}
>> @@ -325,6 +343,7 @@ static int fsl_i2c_probe(struct platform
>>  			goto fail_irq;
>>  		}
>>
>> +	writeccr(i2c, 0);
>>  	mpc_i2c_setclock(i2c);
>>  	platform_set_drvdata(pdev, i2c);
>
> These last two changes are not mentioned in your header comment. What
> are they? Why are they needed? They look like hacks to me.

Sorry about that, figured they fell in a catch all.

The first is an attempt to reduce the errors related to the buggy  
slave device.

The second (writeccr(i2c, 0)) is just ensure the controller is in a  
known state when we startup.

The