[PATCH] i2c: i2c-au1550: properly terminate zero-byte transfers

[PATCH] i2c: i2c-au1550: properly terminate zero-byte transfers

[Manuel Lauss]

Hi Jean,

Here is my third try to fix the corrupted RTC minute register
on my board in conjunction with the i2c-au1550 driver.

Here's a trace of the controller accessing the RTC (0xA2):
http://mlau.at/files/i2c-au1550-original_behav.png

Notice the missing stop condition after the quick probe.

Here's a trace with the patch applied:
http://mlau.at/files/i2c-au1550-misbehavior-patch-behav.png

Please apply!

---
>From 443520b88bda030ba304dcbbbf14f977abad5ff5 Mon Sep 17 00:00:00 2001
From: Manuel Lauss <mano-nEyxjcs6f3Vin2gBucwGBecsttgLyre6@public.gmane.org>
Date: Mon, 14 Jan 2008 09:28:11 +0100
Subject: [PATCH] i2c: i2c-au1550: properly terminate zero-byte transfers

Zero-bytes transfers would leave the bus transaction unfinished
(no i2c stop is sent), with the following transfer actually
sending the slave address to the previously addressed device,
resulting in weird device failures (e.g. reset minute register
values in my RTC).
This patch instructs the controller to send an I2C STOP right after
the slave address in case of a zero-byte transfer.

Signed-off-by: Manuel Lauss <mano-nEyxjcs6f3Vin2gBucwGBecsttgLyre6@public.gmane.org>
---
 drivers/i2c/busses/i2c-au1550.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-au1550.c b/drivers/i2c/busses/i2c-au1550.c
index 2f68416..7d51a43 100644
--- a/drivers/i2c/busses/i2c-au1550.c
+++ b/drivers/i2c/busses/i2c-au1550.c
@@ -105,7 +105,7 @@ wait_master_done(struct i2c_au1550_data *adap)
 }
 
 static int
-do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd)
+do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd, int q)
 {
 	volatile psc_smb_t	*sp;
 	u32			stat;
@@ -134,6 +134,10 @@ do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd)
 	if (rd)
 		addr |= 1;
 
+	/* zero-byte xfers stop immediately */
+	if (q)
+		addr |= PSC_SMBTXRX_STP;
+
 	/* Put byte into fifo, start up master.
 	*/
 	sp->psc_smbtxrx = addr;
@@ -142,7 +146,7 @@ do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd)
 	au_sync();
 	if (wait_ack(adap))
 		return -EIO;
-	return 0;
+	return (q) ? wait_master_done(adap) : 0;
 }
 
 static u32
@@ -262,7 +266,8 @@ au1550_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs, int num)
 
 	for (i = 0; !err && i < num; i++) {
 		p = &msgs[i];
-		err = do_address(adap, p->addr, p->flags & I2C_M_RD);
+		err = do_address(adap, p->addr, p->flags & I2C_M_RD,
+				 (p->len == 0));
 		if (err || !p->len)
 			continue;
 		if (p->flags & I2C_M_RD)
-- 
1.5.3.7


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c: i2c-au1550: properly terminate zero-byte transfers

[Jean Delvare]

Hi Manuel,

On Mon, 14 Jan 2008 09:46:52 +0100, Manuel Lauss wrote:
> Here is my third try to fix the corrupted RTC minute register
> on my board in conjunction with the i2c-au1550 driver.
> 
> Here's a trace of the controller accessing the RTC (0xA2):
> http://mlau.at/files/i2c-au1550-original_behav.png
> 
> Notice the missing stop condition after the quick probe.
> 
> Here's a trace with the patch applied:
> http://mlau.at/files/i2c-au1550-misbehavior-patch-behav.png
> 
> Please apply!

Nice pictures, it's really helpful to be able to visualize what's
happening on the I2C bus.

> 
> ---
> From 443520b88bda030ba304dcbbbf14f977abad5ff5 Mon Sep 17 00:00:00 2001
> From: Manuel Lauss <mano-nEyxjcs6f3Vin2gBucwGBecsttgLyre6@public.gmane.org>
> Date: Mon, 14 Jan 2008 09:28:11 +0100
> Subject: [PATCH] i2c: i2c-au1550: properly terminate zero-byte transfers
> 
> Zero-bytes transfers would leave the bus transaction unfinished
> (no i2c stop is sent), with the following transfer actually
> sending the slave address to the previously addressed device,
> resulting in weird device failures (e.g. reset minute register
> values in my RTC).
> This patch instructs the controller to send an I2C STOP right after
> the slave address in case of a zero-byte transfer.
> 
> Signed-off-by: Manuel Lauss <mano-nEyxjcs6f3Vin2gBucwGBecsttgLyre6@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-au1550.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-au1550.c b/drivers/i2c/busses/i2c-au1550.c
> index 2f68416..7d51a43 100644
> --- a/drivers/i2c/busses/i2c-au1550.c
> +++ b/drivers/i2c/busses/i2c-au1550.c
> @@ -105,7 +105,7 @@ wait_master_done(struct i2c_au1550_data *adap)
>  }
>  
>  static int
> -do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd)
> +do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd, int q)
>  {
>  	volatile psc_smb_t	*sp;
>  	u32			stat;
> @@ -134,6 +134,10 @@ do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd)
>  	if (rd)
>  		addr |= 1;
>  
> +	/* zero-byte xfers stop immediately */
> +	if (q)
> +		addr |= PSC_SMBTXRX_STP;
> +
>  	/* Put byte into fifo, start up master.
>  	*/
>  	sp->psc_smbtxrx = addr;
> @@ -142,7 +146,7 @@ do_address(struct i2c_au1550_data *adap, unsigned int addr, int rd)
>  	au_sync();
>  	if (wait_ack(adap))
>  		return -EIO;
> -	return 0;
> +	return (q) ? wait_master_done(adap) : 0;
>  }
>  
>  static u32
> @@ -262,7 +266,8 @@ au1550_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs, int num)
>  
>  	for (i = 0; !err && i < num; i++) {
>  		p = &msgs[i];
> -		err = do_address(adap, p->addr, p->flags & I2C_M_RD);
> +		err = do_address(adap, p->addr, p->flags & I2C_M_RD,
> +				 (p->len == 0));
>  		if (err || !p->len)
>  			continue;
>  		if (p->flags & I2C_M_RD)

This approach looks much nicer than your previous proposal. I like it.
Applied, thanks!

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c: i2c-au1550: properly terminate zero-byte transfers

[Manuel Lauss]

On Thu, Jan 17, 2008 at 04:52:30PM +0100, Jean Delvare wrote:
> Hi Manuel,
> 
> This approach looks much nicer than your previous proposal. I like it.
> Applied, thanks!

Thank you, however there's still the problem with unterminated bus xfers
after receiving a NAK. I've only seen them with a PCA9539 if one's trying
to access a non-existant register.  However it can't be solved to everyones
satisfaction I'm afraid...
(see http://mlau.at/files/i2c-au1550-stopfix-1-behav_2.png )

-- 
 Manuel Lauss

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c: i2c-au1550: properly terminate zero-byte transfers

[Jean Delvare]

Hi Manuel,

On Thu, 17 Jan 2008 17:37:33 +0100, Manuel Lauss wrote:
> On Thu, Jan 17, 2008 at 04:52:30PM +0100, Jean Delvare wrote:
> > Hi Manuel,
> > 
> > This approach looks much nicer than your previous proposal. I like it.
> > Applied, thanks!
> 
> Thank you, however there's still the problem with unterminated bus xfers
> after receiving a NAK. I've only seen them with a PCA9539 if one's trying
> to access a non-existant register.  However it can't be solved to everyones
> satisfaction I'm afraid...
> (see http://mlau.at/files/i2c-au1550-stopfix-1-behav_2.png )

Indeed. As I understand it, the hardware simply offers no clean
solution to this problem, as you have to ask for the stop condition
before you send an address or data byte. As you can't foresee
transaction failures, you just can't ask for the stop condition before
the failure (or simply nack) happens. Which means that you either don't
send a stop condition at all (what the driver does now) or you send an
extra byte before the stop condition (slightly better but still not
perfect.) Or maybe you have to read the datasheet in details to find
out how to do the right thing, some chips can be very tricky in this
respect.

Anyway, I will take whatever patch you think will improve the situation
on this platform.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c