Skip to contentSkip to conversation

ALI1563 SMBus driver fix - take 2

PATCH In review [email protected] 3 participants 7 messages May 19, 2005 – Jun 2, 2005

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ALI1563 SMBus driver fix - take 2

Hello,

This patch fixes "grave" bugs in i2c-ali1563 driver. It seems on recent chipset revisions the HSTS_DONE is set only for block transfers, so we must detect the end of ordinary transaction other way. Also due to missing and mask, setting other transfer modes was not possible. Moreover the continous byte mode transfer uses DAT0 for command rather than CMD command. All those changes were tested with help of Chunhao Huang from Winbond.

I'm willing to maintain the driver. Second patch adds me as maintainer if this is neccessary.

Regards

Rudolf

drivers/i2c/busses/i2c-ali1563.c +31 -15
diff -Naur a/drivers/i2c/busses/i2c-ali1563.c b/drivers/i2c/busses/i2c-ali1563.c
@@ -2,6 +2,7 @@
  *	i2c-ali1563.c - i2c driver for the ALi 1563 Southbridge
  *
  *	Copyright (C) 2004 Patrick Mochel
+ *		      2005 Rudolf Marek <[email protected]>
  *
  *	The 1563 southbridge is deceptively similar to the 1533, with a
  *	few notable exceptions. One of those happens to be the fact they
@@ -57,10 +58,11 @@
 #define HST_CNTL2_BLOCK		0x05
 
 
+#define HST_CNTL2_SIZEMASK	0x38
 
 static unsigned short ali1563_smba;
 
-static int ali1563_transaction(struct i2c_adapter * a)
+static int ali1563_transaction(struct i2c_adapter * a, int size)
 {
 	u32 data;
 	int timeout;
@@ -73,7 +75,7 @@
 
 	data = inb_p(SMB_HST_STS);
 	if (data & HST_STS_BAD) {
-		dev_warn(&a->dev,"ali1563: Trying to reset busy device\n");
+		dev_err(&a->dev, "ali1563: Trying to reset busy device\n");
 		outb_p(data | HST_STS_BAD,SMB_HST_STS);
 		data = inb_p(SMB_HST_STS);
 		if (data & HST_STS_BAD)
@@ -94,19 +96,31 @@
 
 	if (timeout && !(data & HST_STS_BAD))
 		return 0;
-	dev_warn(&a->dev, "SMBus Error: %s%s%s%s%s\n",
-		timeout ? "Timeout " : "",
-		data & HST_STS_FAIL ? "Transaction Failed " : "",
-		data & HST_STS_BUSERR ? "No response or Bus Collision " : "",
-		data & HST_STS_DEVERR ? "Device Error " : "",
-		!(data & HST_STS_DONE) ? "Transaction Never Finished " : "");
 
-	if (!(data & HST_STS_DONE))
+	if (!timeout) {
+		dev_err(&a->dev, "Timeout - Trying to KILL transaction!\n");
 		/* Issue 'kill' to host controller */
 		outb_p(HST_CNTL2_KILL,SMB_HST_CNTL2);
-	else
-		/* Issue timeout to reset all devices on bus */
+		data = inb_p(SMB_HST_STS);
+ 	}
+	
+	/* device error - no response, ignore the autodetection case */	
+	if ((data & HST_STS_DEVERR) && (size != HST_CNTL2_QUICK)) {
+		dev_err(&a->dev, "Device error!\n");
+	}
+
+	/* bus collision */
+	if (data & HST_STS_BUSERR) {
+		dev_err(&a->dev, "Bus collision!\n");
+		/* Issue timeout, hoping it helps */
 		outb_p(HST_CNTL1_TIMEOUT,SMB_HST_CNTL1);
+	}
+
+	if (data & HST_STS_FAIL) {
+		dev_err(&a->dev, "Cleaning fail after KILL!\n");
+		outb_p(0x0,SMB_HST_CNTL2);
+	}
+
 	return -1;
 }
 
@@ -149,7 +163,7 @@
 
 	if (timeout && !(data & HST_STS_BAD))
 		return 0;
-	dev_warn(&a->dev, "SMBus Error: %s%s%s%s%s\n",
+	dev_err(&a->dev, "SMBus Error: %s%s%s%s%s\n",
 		timeout ? "Timeout " : "",
 		data & HST_STS_FAIL ? "Transaction Failed " : "",
 		data & HST_STS_BUSERR ? "No response or Bus Collision " : "",
@@ -242,13 +256,15 @@
 	}
 
 	outb_p(((addr & 0x7f) << 1) | (rw & 0x01), SMB_HST_ADD);
-	outb_p(inb_p(SMB_HST_CNTL2) | (size << 3), SMB_HST_CNTL2);
+	outb_p((inb_p(SMB_HST_CNTL2) & ~HST_CNTL2_SIZEMASK) | (size << 3), SMB_HST_CNTL2);
 
 	/* Write the command register */
+
 	switch(size) {
 	case HST_CNTL2_BYTE:
 		if (rw= I2C_SMBUS_WRITE)
-			outb_p(cmd, SMB_HST_CMD);
+			/* Beware it uses DAT0 register and not CMD! */
+			outb_p(cmd, SMB_HST_DAT0);
 		break;
 	case HST_CNTL2_BYTE_DATA:
 		outb_p(cmd, SMB_HST_CMD);
@@ -268,7 +284,7 @@
 		goto Done;
 	}
 
-	if ((error = ali1563_transaction(a)))
+	if ((error = ali1563_transaction(a, size)))
 		goto Done;
 
 	if ((rw = I2C_SMBUS_WRITE) || (size = HST_CNTL2_QUICK))
MAINTAINERS +6 -0
diff -Naur a/MAINTAINERS b/MAINTAINERS
@@ -247,6 +247,12 @@
 W:	http://www.linux-usb.org/SpeedTouch/
 S:	Maintained
 
+ALI1563 I2C DRIVER
+P:	Rudolf Marek
+M:	[email protected]
+L:	[email protected]
+S:	Maintained
+
 ALPHA PORT
 P:	Richard Henderson
 M:	[email protected]
Applied
#1 lore raw

PATCH 1/1 ALI1563 SMBus driver fix - take 2

Quoted 11 lines · On Thu, Apr 21, 2005 at 09:07:56AM -0000, [email protected] wrote:

Hello,

This patch fixes "grave" bugs in i2c-ali1563 driver. It seems on recent chipset revisions the HSTS_DONE is set only for block transfers, so we must detect the end of ordinary transaction other way. Also due to missing and mask, setting other transfer modes was not possible. Moreover the continous byte mode transfer uses DAT0 for command rather than CMD command. All those changes were tested with help of Chunhao Huang from Winbond.

I'm willing to maintain the driver. Second patch adds me as maintainer if this is neccessary.

Applied, thanks.

greg k-h

#2 lore raw

PATCH 1/1 ALI1563 SMBus driver fix - take 2

In reply to [email protected]

Hi Greg, Rudolf,

Quoted 9 lines

Please can my patch go to 2.6.12? It is quite critical bug fix. Without the fixes driver is simply unusable.

1) It wont work with newer revisions
2) It would not work even with older because i2c transaction size mask is missing. That means if you use eeprom driver and sensor driver you wont get any data. I mean data that would make sense.The missing mask would trigger some WORD modes...

Quoted 4 lines

3) eeprom reading wont work because command type goes to DAT0 and not to CMD.

So whole patch is needed to address all "grave" issues.

I'd second Rudolf's request. This patch would be really welcome into 2.6.12, without it the i2c-ali1563 driver is plain broken and will most probably generate lots of support requests.

Thanks,

Signature · 1 line
Jean Delvare
#3 lore raw

PATCH 1/1 ALI1563 SMBus driver fix - take 2

In reply to [email protected]
Quoted 11 lines · Jean Delvare

Hi Greg, Rudolf,

Quoted 9 lines

Please can my patch go to 2.6.12? It is quite critical bug fix. Without the fixes driver is simply unusable.

1) It wont work with newer revisions
2) It would not work even with older because i2c transaction size mask is missing. That means if you use eeprom driver and sensor driver you wont get any data. I mean data that would make sense.The missing mask would trigger some WORD modes...

Quoted 8 lines
Quoted 4 lines

3) eeprom reading wont work because command type goes to DAT0 and not to CMD.

So whole patch is needed to address all "grave" issues.

I'd second Rudolf's request. This patch would be really welcome into 2.6.12, without it the i2c-ali1563 driver is plain broken and will most probably generate lots of support requests.

Ok, I've moved it up to my "before 2.6.12 is out" set of patches to be sent out.

thanks,

greg k-h

#4 lore raw

PATCH 1/1 ALI1563 SMBus driver fix - take 2

In reply to [email protected]
Quoted 4 lines

Applied, thanks.

greg k-h

Hi,

Please can my patch go to 2.6.12? It is quite critical bug fix. Without the fixes driver is simply unusable.

1) It wont work with newer revisions
2) It would not work even with older because i2c transaction size mask is missing. That means if you use eeprom driver and sensor driver you wont get any data. I mean data that would make sense.The missing mask would trigger some WORD modes...

3) eeprom reading wont work because command type goes to DAT0 and not to CMD.

So whole patch is needed to address all "grave" issues.

Thanks

Regards
Rudolf

#5 lore raw
In reply to [email protected]

Hello,

Quoted 2 lines

Ok, I've moved it up to my "before 2.6.12 is out" set of patches to be sent out.

I would like to ask what are your plans whith my patch? I'm asking because I cant see it in rc5.

Thanks
Regards

Rudolf

#6 lore raw
In reply to [email protected]
Quoted 6 lines · Rudolf Marek

Hello,

Quoted 2 lines

Ok, I've moved it up to my "before 2.6.12 is out" set of patches to be sent out.

I would like to ask what are your plans whith my patch? I'm asking because I cant see it in rc5.

Sent to Linus yesterday.

thanks,

greg k-h