[PATCH 0/3] i2c: i801: enable irq

[PATCH 0/3] i2c: i801: enable irq

[Daniel Kurtz]

This set of patches enables the Intel PCH SMBus controller interrupt.

The Interrupt can signify:
 INTR - the end of a complete transaction
 DEV_ERR - that a device did not ACK a transaction
 BYTE_DONE - the completion of a single byte during a byte-by-byte transaction

This patchset arrives with the following caveats:

 1)  It has only been tested with a Cougar Point (Intel 6 Series PCH) SMBus
controller, so the irq is only enabled for that chip type.

 2) It has not been tested with any devices that do transactions that use the
    PEC.  In fact, I believe that an additional small patch would be required
    to the driver working correctly in interrupt mode with PEC.

 3) It has not been tested in SMBus Slave mode.

 4) It has not been tested with SMI#-type interrupts.

 5) The BIOS has to configure the PCH SMBus IRQ properly.

 6) It has not been tested with a device that does byte-by-byte smbus (non-i2c)
    reads.

 7) It has not been tested with smbus 'process call' transactions.

If would be very helpful if somebody could help test on other chipsets, with
a PEC device, or on additional BIOS that woudl be very helpful.

In the meantime, the interrupt behavior is only enabled on the Cougar Point,
and even here, it can be completely disabled with the "Interrupt" feature like
other advanced features of the driver.

Daniel Kurtz (3):
  i2c: i801: refactor i801_block_transaction_byte_by_byte
  i2c: i801: enable irq for i801 smbus transactions
  i2c: i801: enable irq for byte_by_byte transactions

 drivers/i2c/busses/i2c-i801.c |  193 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 170 insertions(+), 23 deletions(-)

-- 
1.7.3.1

[PATCH 1/3] i2c: i801: refactor i801_block_transaction_byte_by_byte

[Daniel Kurtz]

1) As a slight optimization, pull some logic out of the byte loop during
byte-by-byte transactions by just setting the I801_LAST_BYTE bit, as
defined in the i801 (PCH) datasheet, when reading the last byte of a
byte-by-byte I2C_SMBUS_READ.

2) Clear INTR after clearing BYTE_DONE per ICH10 datasheet [1] pg. 711:

  When the last byte of a block message is received, the host controller
sets DS. However, it does not set the INTR bit (and generate another
interrupt) until DS is cleared. Thus, for a block message of n bytes,
the ICH10 will generate n+1 interrupts.

3) If DEV_ERR is set in polling loop, abort the transaction and return
-ENXIO to the caller. DEV_ERR is set if the device does not respond with
an acknowledge, and the SMBus controller times out (minimum 25ms).

[1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf

Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c |   45 ++++++++++++++++++++++++----------------
 1 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ab26840d..f3418cf 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -116,8 +116,7 @@
 #define I801_PROC_CALL		0x10	/* unimplemented */
 #define I801_BLOCK_DATA		0x14
 #define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
-#define I801_BLOCK_LAST		0x34
-#define I801_I2C_BLOCK_LAST	0x38	/* ICH5 and later */
+#define I801_LAST_BYTE		0x20
 #define I801_START		0x40
 #define I801_PEC_EN		0x80	/* ICH3 and later */
 
@@ -320,7 +319,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	}
 
 	status = i801_transaction(priv, I801_BLOCK_DATA | ENABLE_INT9 |
-				  I801_PEC_EN * hwpec);
+				  (hwpec ? I801_PEC_EN : 0));
 	if (status)
 		return status;
 
@@ -336,6 +335,10 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	return 0;
 }
 
+/*
+ * i2c write uses cmd=I801_BLOCK_DATA, I2C_EN=1
+ * i2c read uses cmd=I801_I2C_BLOCK_DATA
+ */
 static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 					       union i2c_smbus_data *data,
 					       char read_write, int command,
@@ -358,19 +361,15 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		outb_p(data->block[1], SMBBLKDAT(priv));
 	}
 
+	if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
+	    read_write == I2C_SMBUS_READ)
+		smbcmd = I801_I2C_BLOCK_DATA;
+	else
+		smbcmd = I801_BLOCK_DATA;
+
 	for (i = 1; i <= len; i++) {
-		if (i == len && read_write == I2C_SMBUS_READ) {
-			if (command == I2C_SMBUS_I2C_BLOCK_DATA)
-				smbcmd = I801_I2C_BLOCK_LAST;
-			else
-				smbcmd = I801_BLOCK_LAST;
-		} else {
-			if (command == I2C_SMBUS_I2C_BLOCK_DATA
-			 && read_write == I2C_SMBUS_READ)
-				smbcmd = I801_I2C_BLOCK_DATA;
-			else
-				smbcmd = I801_BLOCK_DATA;
-		}
+		if (i == len && read_write == I2C_SMBUS_READ)
+			smbcmd |= I801_LAST_BYTE;
 		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
 
 		if (i == 1)
@@ -382,8 +381,9 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		do {
 			msleep(1);
 			status = inb_p(SMBHSTSTS(priv));
-		} while ((!(status & SMBHSTSTS_BYTE_DONE))
-			 && (timeout++ < MAX_TIMEOUT));
+		} while (!(status & SMBHSTSTS_BYTE_DONE) &&
+			 !(status & SMBHSTSTS_DEV_ERR) &&
+			 timeout++ < MAX_TIMEOUT);
 
 		result = i801_check_post(priv, status, timeout > MAX_TIMEOUT);
 		if (result < 0)
@@ -414,8 +414,17 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 			outb_p(data->block[i+1], SMBBLKDAT(priv));
 
 		/* signals SMBBLKDAT ready */
-		outb_p(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR, SMBHSTSTS(priv));
+		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
+	}
+
+	/* Wait for command-completion interrupt so we can clear it */
+	timeout = 0;
+	status = inb_p(SMBHSTSTS(priv));
+	while (!(status & SMBHSTSTS_INTR) && timeout++ < MAX_TIMEOUT) {
+		msleep(1);
+		status = inb_p(SMBHSTSTS(priv));
 	}
+	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
 
 	return 0;
 }
-- 
1.7.3.1

[PATCH 2/3] i2c: i801: enable irq for i801 smbus transactions

[Daniel Kurtz]

Add a new 'feature' to i2c-i801 to enable using i801 interrupts.
When the feature is enabled, then an isr is installed for the device's
pci irq.  The SMBus controller generates an INTR irq at the end of each
transaction where INTREN was set in the HST_CNT register.

For this patch, the INTREN bit is set only for smbus block, byte and word
transactions, but not for emulated i2c reads or writes.

The use of the DS (BYTE_DONE) interrupt with byte-by-byte i2c
transactions is implemented in a subsequent patch.

Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c |  107 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index f3418cf..3abc624 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -59,10 +59,12 @@
   Block process call transaction   no
   I2C block read transaction       yes  (doesn't use the block buffer)
   Slave mode                       no
+  Interrupt processing             yes
 
   See the file Documentation/i2c/busses/i2c-i801 for details.
 */
 
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/kernel.h>
@@ -90,6 +92,7 @@
 
 /* PCI Address Constants */
 #define SMBBAR		4
+#define SMBPCISTS	0x006
 #define SMBHSTCFG	0x040
 
 /* Host configuration bits for SMBHSTCFG */
@@ -97,6 +100,9 @@
 #define SMBHSTCFG_SMB_SMI_EN	2
 #define SMBHSTCFG_I2C_EN	4
 
+/* Host status bits for SMBHSTSTS */
+#define SMBPCISTS_INTS		8
+
 /* Auxiliary control register bits, ICH4+ only */
 #define SMBAUXCTL_CRC		1
 #define SMBAUXCTL_E32B		2
@@ -106,7 +112,7 @@
 
 /* Other settings */
 #define MAX_TIMEOUT		100
-#define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
+#define IRQ_TIMEOUT		(HZ/2)
 
 /* I801 command constants */
 #define I801_QUICK		0x00
@@ -116,7 +122,11 @@
 #define I801_PROC_CALL		0x10	/* unimplemented */
 #define I801_BLOCK_DATA		0x14
 #define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
-#define I801_LAST_BYTE		0x20
+
+/* I801 Hosts Control register bits */
+#define I801_INTREN		0x01
+#define I801_KILL		0x02
+#define I801_LAST_BYTE		0x20	/* ICH5 and later */
 #define I801_START		0x40
 #define I801_PEC_EN		0x80	/* ICH3 and later */
 
@@ -151,6 +161,11 @@ struct i801_priv {
 	unsigned char original_hstcfg;
 	struct pci_dev *pci_dev;
 	unsigned int features;
+
+	/* isr processing */
+	wait_queue_head_t waitq;
+	spinlock_t lock;
+	u8 status;
 };
 
 static struct pci_driver i801_driver;
@@ -159,6 +174,7 @@ static struct pci_driver i801_driver;
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
 #define FEATURE_I2C_BLOCK_READ	(1 << 3)
+#define FEATURE_IRQ		(1 << 4)
 /* Not really a feature, but it's convenient to handle it as such */
 #define FEATURE_IDF		(1 << 15)
 
@@ -167,6 +183,7 @@ static const char *i801_feature_names[] = {
 	"Block buffer",
 	"Block process call",
 	"I2C block read",
+	"Interrupt"
 };
 
 static unsigned int disable_features;
@@ -255,6 +272,20 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
 	return result;
 }
 
+/* interrupt handling: fetch & consume host status out of algo_data */
+static inline u8 i801_get_status(struct i801_priv *priv)
+{
+	unsigned long flags;
+	u8 status;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	status = priv->status;
+	priv->status = 0;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return status;
+}
+
 static int i801_transaction(struct i801_priv *priv, int xact)
 {
 	int status;
@@ -265,8 +296,16 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 	if (result < 0)
 		return result;
 
+	if (priv->features & FEATURE_IRQ) {
+		outb(xact | I801_INTREN | I801_START, SMBHSTCNT(priv));
+		timeout = wait_event_timeout(priv->waitq,
+					     (status = i801_get_status(priv)),
+					     IRQ_TIMEOUT);
+		return i801_check_post(priv, status, timeout == 0);
+	}
+
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
-	 * INTREN, SMBSCMD are passed in xact */
+	 * SMBSCMD are passed in xact */
 	outb_p(xact | I801_START, SMBHSTCNT(priv));
 
 	/* We will always wait for a fraction of a second! */
@@ -280,6 +319,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 		return result;
 
 	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
+
 	return 0;
 }
 
@@ -318,7 +358,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 			outb_p(data->block[i+1], SMBBLKDAT(priv));
 	}
 
-	status = i801_transaction(priv, I801_BLOCK_DATA | ENABLE_INT9 |
+	status = i801_transaction(priv, I801_BLOCK_DATA |
 				  (hwpec ? I801_PEC_EN : 0));
 	if (status)
 		return status;
@@ -335,6 +375,38 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	return 0;
 }
 
+static irqreturn_t i801_isr(int irq, void *dev_id)
+{
+	struct i801_priv *priv = dev_id;
+	u8 pcists, hststs;
+	unsigned long flags;
+
+	/* Confirm this is our interrupt */
+	pci_read_config_byte(priv->pci_dev, SMBPCISTS, &pcists);
+	if (!(pcists & SMBPCISTS_INTS)) {
+		dev_dbg(&priv->pci_dev->dev, "irq: pcists.ints not set\n");
+		return IRQ_NONE;
+	}
+
+	hststs = inb(SMBHSTSTS(priv));
+	dev_dbg(&priv->pci_dev->dev, "irq: hststs = %02x\n", hststs);
+
+	/* Only process irq sources */
+	hststs &= STATUS_FLAGS;
+
+	/* Clear and report irq sources */
+	if (hststs) {
+		outb(hststs, SMBHSTSTS(priv));
+
+		spin_lock_irqsave(&priv->lock, flags);
+		priv->status |= hststs;
+		spin_unlock_irqrestore(&priv->lock, flags);
+		wake_up(&priv->waitq);
+	}
+
+	return IRQ_HANDLED;
+}
+
 /*
  * i2c write uses cmd=I801_BLOCK_DATA, I2C_EN=1
  * i2c read uses cmd=I801_I2C_BLOCK_DATA
@@ -370,7 +442,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= I801_LAST_BYTE;
-		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
+		outb_p(smbcmd, SMBHSTCNT(priv));
 
 		if (i == 1)
 			outb_p(inb(SMBHSTCNT(priv)) | I801_START,
@@ -571,7 +643,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		ret = i801_block_transaction(priv, data, read_write, size,
 					     hwpec);
 	else
-		ret = i801_transaction(priv, xact | ENABLE_INT9);
+		ret = i801_transaction(priv, xact);
 
 	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
 	   time, so we forcibly disable it after every transaction. Turn off
@@ -805,6 +877,10 @@ static int __devinit i801_probe(struct pci_dev *dev,
 		break;
 	}
 
+	/* IRQ processing only tested on CougarPoint PCH */
+	if (dev->device == PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS)
+		priv->features |= FEATURE_IRQ;
+
 	/* Disable features on user request */
 	for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) {
 		if (priv->features & disable_features & (1 << i))
@@ -879,8 +955,24 @@ static int __devinit i801_probe(struct pci_dev *dev,
 	i801_probe_optional_slaves(priv);
 
 	pci_set_drvdata(dev, priv);
+
+	if (priv->features & FEATURE_IRQ) {
+		init_waitqueue_head(&priv->waitq);
+		spin_lock_init(&priv->lock);
+
+		err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
+				  i801_driver.name, priv);
+		if (err) {
+			dev_err(&dev->dev, "Failed to allocate irq %d: %d",
+				dev->irq, err);
+			goto exit_del_adapter;
+		}
+	}
 	return 0;
 
+exit_del_adapter:
+	pci_set_drvdata(dev, NULL);
+	i2c_del_adapter(&priv->adapter);
 exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
@@ -892,6 +984,9 @@ static void __devexit i801_remove(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
+	if (priv->features & FEATURE_IRQ)
+		free_irq(dev->irq, priv);
+
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 	pci_release_region(dev, SMBBAR);
-- 
1.7.3.1

[PATCH 3/3] i2c: i801: enable irq for byte_by_byte transactions

[Daniel Kurtz]

Byte-by-byte transactions are used primarily for accessing i2c devices
with an smbus controller.  For these transactions, for each byte that is
read or written, the SMBus controller generates a BYTE_DONE irq.  The isr
reads/writes the next byte, and clears the irq flag to start the next byte.
On the penultimate irq, the isr also sets the LAST_BYTE flag.

There is no locking around the cmd/len/count/data variables, since the
i2c adapter lock ensures there is never multiple simultaneous transactions
for the same device, and the driver thread never accesses these variables
while interrupts might be occurring.

The end result is a dramatic speed up in emulated i2c-over smbus block
read and write transactions.

Note: This patch has only been tested and verified by doing i2c read and
write block transfers on Cougar Point 6 Series PCH.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/i2c/busses/i2c-i801.c |   49 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 3abc624..1b6673b 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -166,6 +166,13 @@ struct i801_priv {
 	wait_queue_head_t waitq;
 	spinlock_t lock;
 	u8 status;
+
+	/* Command state used during by isr */
+	u8 cmd;
+	bool is_read;
+	int count;
+	int len;
+	u8 *data;
 };
 
 static struct pci_driver i801_driver;
@@ -379,6 +386,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 {
 	struct i801_priv *priv = dev_id;
 	u8 pcists, hststs;
+	u8 cmd;
 	unsigned long flags;
 
 	/* Confirm this is our interrupt */
@@ -394,16 +402,35 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	/* Only process irq sources */
 	hststs &= STATUS_FLAGS;
 
-	/* Clear and report irq sources */
+	if (hststs & SMBHSTSTS_BYTE_DONE) {
+		if (priv->is_read) {
+			priv->data[priv->count++] = inb(SMBBLKDAT(priv));
+
+			/* Set LAST_BYTE for last byte of read transaction */
+			cmd = priv->cmd;
+			if (priv->count == priv->len - 1)
+				cmd |= I801_LAST_BYTE;
+			outb_p(cmd | I801_START, SMBHSTCNT(priv));
+		} else if (priv->count < priv->len - 1) {
+			outb(priv->data[++priv->count], SMBBLKDAT(priv));
+			outb_p(priv->cmd | I801_START, SMBHSTCNT(priv));
+		}
+
+		/* Clear BYTE_DONE to start next transaction. */
+		outb(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
+
+		/* Clear BYTE_DONE so it does not wake_up waitq */
+		hststs &= ~SMBHSTSTS_BYTE_DONE;
+	}
+
+	/* Clear and report other irq sources */
 	if (hststs) {
 		outb(hststs, SMBHSTSTS(priv));
-
 		spin_lock_irqsave(&priv->lock, flags);
 		priv->status |= hststs;
 		spin_unlock_irqrestore(&priv->lock, flags);
 		wake_up(&priv->waitq);
 	}
-
 	return IRQ_HANDLED;
 }
 
@@ -439,6 +466,22 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	else
 		smbcmd = I801_BLOCK_DATA;
 
+	if (priv->features & FEATURE_IRQ) {
+		priv->is_read = (read_write == I2C_SMBUS_READ);
+		if (len == 1 && priv->is_read)
+			smbcmd |= I801_LAST_BYTE;
+		priv->cmd = smbcmd | I801_INTREN;
+		priv->len = len;
+		priv->count = 0;
+		priv->data = &data->block[1];
+
+		outb(priv->cmd | I801_START, SMBHSTCNT(priv));
+		timeout = wait_event_timeout(priv->waitq,
+					     (status = i801_get_status(priv)),
+					     IRQ_TIMEOUT);
+		return i801_check_post(priv, status, timeout == 0);
+	}
+
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= I801_LAST_BYTE;
-- 
1.7.3.1

[PATCH 1/3] i2c: i801: refactor i801_block_transaction_byte_by_byte

[Daniel Kurtz]

1) As a slight optimization, pull some logic out of the byte loop during
byte-by-byte transactions by just setting the I801_LAST_BYTE bit, as
defined in the i801 (PCH) datasheet, when reading the last byte of a
byte-by-byte I2C_SMBUS_READ.

2) Clear INTR after clearing BYTE_DONE per ICH10 datasheet [1] pg. 711:

  When the last byte of a block message is received, the host controller
sets DS. However, it does not set the INTR bit (and generate another
interrupt) until DS is cleared. Thus, for a block message of n bytes,
the ICH10 will generate n+1 interrupts.

3) If DEV_ERR is set in polling loop, abort the transaction and return
-ENXIO to the caller. DEV_ERR is set if the device does not respond with
an acknowledge, and the SMBus controller times out (minimum 25ms).

[1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/i2c/busses/i2c-i801.c |   45 ++++++++++++++++++++++++----------------
 1 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ab26840d..f3418cf 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -116,8 +116,7 @@
 #define I801_PROC_CALL		0x10	/* unimplemented */
 #define I801_BLOCK_DATA		0x14
 #define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
-#define I801_BLOCK_LAST		0x34
-#define I801_I2C_BLOCK_LAST	0x38	/* ICH5 and later */
+#define I801_LAST_BYTE		0x20
 #define I801_START		0x40
 #define I801_PEC_EN		0x80	/* ICH3 and later */
 
@@ -320,7 +319,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	}
 
 	status = i801_transaction(priv, I801_BLOCK_DATA | ENABLE_INT9 |
-				  I801_PEC_EN * hwpec);
+				  (hwpec ? I801_PEC_EN : 0));
 	if (status)
 		return status;
 
@@ -336,6 +335,10 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	return 0;
 }
 
+/*
+ * i2c write uses cmd=I801_BLOCK_DATA, I2C_EN=1
+ * i2c read uses cmd=I801_I2C_BLOCK_DATA
+ */
 static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 					       union i2c_smbus_data *data,
 					       char read_write, int command,
@@ -358,19 +361,15 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		outb_p(data->block[1], SMBBLKDAT(priv));
 	}
 
+	if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
+	    read_write == I2C_SMBUS_READ)
+		smbcmd = I801_I2C_BLOCK_DATA;
+	else
+		smbcmd = I801_BLOCK_DATA;
+
 	for (i = 1; i <= len; i++) {
-		if (i == len && read_write == I2C_SMBUS_READ) {
-			if (command == I2C_SMBUS_I2C_BLOCK_DATA)
-				smbcmd = I801_I2C_BLOCK_LAST;
-			else
-				smbcmd = I801_BLOCK_LAST;
-		} else {
-			if (command == I2C_SMBUS_I2C_BLOCK_DATA
-			 && read_write == I2C_SMBUS_READ)
-				smbcmd = I801_I2C_BLOCK_DATA;
-			else
-				smbcmd = I801_BLOCK_DATA;
-		}
+		if (i == len && read_write == I2C_SMBUS_READ)
+			smbcmd |= I801_LAST_BYTE;
 		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
 
 		if (i == 1)
@@ -382,8 +381,9 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		do {
 			msleep(1);
 			status = inb_p(SMBHSTSTS(priv));
-		} while ((!(status & SMBHSTSTS_BYTE_DONE))
-			 && (timeout++ < MAX_TIMEOUT));
+		} while (!(status & SMBHSTSTS_BYTE_DONE) &&
+			 !(status & SMBHSTSTS_DEV_ERR) &&
+			 timeout++ < MAX_TIMEOUT);
 
 		result = i801_check_post(priv, status, timeout > MAX_TIMEOUT);
 		if (result < 0)
@@ -414,8 +414,17 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 			outb_p(data->block[i+1], SMBBLKDAT(priv));
 
 		/* signals SMBBLKDAT ready */
-		outb_p(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR, SMBHSTSTS(priv));
+		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
+	}
+
+	/* Wait for command-completion interrupt so we can clear it */
+	timeout = 0;
+	status = inb_p(SMBHSTSTS(priv));
+	while (!(status & SMBHSTSTS_INTR) && timeout++ < MAX_TIMEOUT) {
+		msleep(1);
+		status = inb_p(SMBHSTSTS(priv));
 	}
+	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
 
 	return 0;
 }
-- 
1.7.3.1

Re: [PATCH 1/3] i2c: i801: refactor i801_block_transaction_byte_by_byte

[Jean Delvare]

Hi Daniel,

Sorry for the very late reply, I'm only reviewing this patch series now.

On Fri,  6 Jan 2012 18:58:20 +0800, Daniel Kurtz wrote:
> 1) As a slight optimization, pull some logic out of the byte loop during
> byte-by-byte transactions by just setting the I801_LAST_BYTE bit, as
> defined in the i801 (PCH) datasheet, when reading the last byte of a
> byte-by-byte I2C_SMBUS_READ.

I agree it's much cleaner, and certainly more efficient too.

> 2) Clear INTR after clearing BYTE_DONE per ICH10 datasheet [1] pg. 711:
> 
>   When the last byte of a block message is received, the host controller
> sets DS. However, it does not set the INTR bit (and generate another
> interrupt) until DS is cleared. Thus, for a block message of n bytes,
> the ICH10 will generate n+1 interrupts.

Interesting. I wonder if this could explain the lockups we were seeing
when we first tried to convert the driver to use interrupts many years
ago.

How does this interact with hwpec? The driver already waits for INTR in
that case. Should this be dropped now, or do we really need to do it
twice?

> 3) If DEV_ERR is set in polling loop, abort the transaction and return
> -ENXIO to the caller. DEV_ERR is set if the device does not respond with
> an acknowledge, and the SMBus controller times out (minimum 25ms).

I agree, modulo comments below.

Overall I like these changes much. But I would recommend not mixing
code cleanups (point 1 above) with behavioral changes (points 2 and 3).
It's a lot easier to review, test, bisect and backport patches when
each patch does juts one thing.

> 
> [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
> 
> Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   45 ++++++++++++++++++++++++----------------
>  1 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ab26840d..f3418cf 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -116,8 +116,7 @@
>  #define I801_PROC_CALL		0x10	/* unimplemented */
>  #define I801_BLOCK_DATA		0x14
>  #define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
> -#define I801_BLOCK_LAST		0x34
> -#define I801_I2C_BLOCK_LAST	0x38	/* ICH5 and later */
> +#define I801_LAST_BYTE		0x20
>  #define I801_START		0x40
>  #define I801_PEC_EN		0x80	/* ICH3 and later */
>  
> @@ -320,7 +319,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  	}
>  
>  	status = i801_transaction(priv, I801_BLOCK_DATA | ENABLE_INT9 |
> -				  I801_PEC_EN * hwpec);
> +				  (hwpec ? I801_PEC_EN : 0));
>  	if (status)
>  		return status;
>  
> @@ -336,6 +335,10 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  	return 0;
>  }
>  
> +/*
> + * i2c write uses cmd=I801_BLOCK_DATA, I2C_EN=1
> + * i2c read uses cmd=I801_I2C_BLOCK_DATA
> + */
>  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  					       union i2c_smbus_data *data,
>  					       char read_write, int command,
> @@ -358,19 +361,15 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		outb_p(data->block[1], SMBBLKDAT(priv));
>  	}
>  
> +	if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
> +	    read_write == I2C_SMBUS_READ)
> +		smbcmd = I801_I2C_BLOCK_DATA;
> +	else
> +		smbcmd = I801_BLOCK_DATA;
> +
>  	for (i = 1; i <= len; i++) {
> -		if (i == len && read_write == I2C_SMBUS_READ) {
> -			if (command == I2C_SMBUS_I2C_BLOCK_DATA)
> -				smbcmd = I801_I2C_BLOCK_LAST;
> -			else
> -				smbcmd = I801_BLOCK_LAST;
> -		} else {
> -			if (command == I2C_SMBUS_I2C_BLOCK_DATA
> -			 && read_write == I2C_SMBUS_READ)
> -				smbcmd = I801_I2C_BLOCK_DATA;
> -			else
> -				smbcmd = I801_BLOCK_DATA;
> -		}
> +		if (i == len && read_write == I2C_SMBUS_READ)
> +			smbcmd |= I801_LAST_BYTE;
>  		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
>  
>  		if (i == 1)
> @@ -382,8 +381,9 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		do {
>  			msleep(1);
>  			status = inb_p(SMBHSTSTS(priv));
> -		} while ((!(status & SMBHSTSTS_BYTE_DONE))
> -			 && (timeout++ < MAX_TIMEOUT));
> +		} while (!(status & SMBHSTSTS_BYTE_DONE) &&
> +			 !(status & SMBHSTSTS_DEV_ERR) &&

I think the following would be more efficient?

		} while (!(status & (SMBHSTSTS_BYTE_DONE | SMBHSTSTS_DEV_ERR)) &&

I am also curious why we wouldn't check for other error flags, in
particular SMBHSTSTS_FAILED and SMBHSTSTS_BUS_ERR? It might be a good
use case for STATUS_FLAGS.

> +			 timeout++ < MAX_TIMEOUT);
>  
>  		result = i801_check_post(priv, status, timeout > MAX_TIMEOUT);
>  		if (result < 0)
> @@ -414,8 +414,17 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  			outb_p(data->block[i+1], SMBBLKDAT(priv));
>  
>  		/* signals SMBBLKDAT ready */
> -		outb_p(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR, SMBHSTSTS(priv));
> +		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> +	}
> +
> +	/* Wait for command-completion interrupt so we can clear it */
> +	timeout = 0;
> +	status = inb_p(SMBHSTSTS(priv));
> +	while (!(status & SMBHSTSTS_INTR) && timeout++ < MAX_TIMEOUT) {
> +		msleep(1);

This should be usleep_range(250, 500) these days.

> +		status = inb_p(SMBHSTSTS(priv));
>  	}

I think it would make sense to have (at least) a debug message here if
we hit the timeout, as we do in i801_wait_hwpec() for example.

> +	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
>  
>  	return 0;
>  }


-- 
Jean Delvare