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

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

[Daniel Kurtz]

This is a second version of a set of patches enables the Intel PCH SMBus
controller interrupt.  It refactors the second two patches a little bit by
relying on DEV_ERR interrupt for timeouts, instead of using an explicit
wait_event_timeout.

The first attempt received absolutely no response. Maybe this time someone
will be interested.

The SMBus Host Controller 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 |  199 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 175 insertions(+), 24 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@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

[PATCH 2/3 v2] 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.

An i2c/smbus transaction is always terminated by one of the following
interrupt sources: FAILED, BUS_ERR, DEV_ERR, or on success: INTR

When the isr fires for one of these cases, it sets the ->status variable
and wakes up the waitq.  The waitq then saves off the status code, and
clears ->status (in preperation for some future transaction).
The SMBus controller generates an INTR irq at the end of each
transaction where INTREN was set in the HST_CNT register.

No locking is needed around accesses to priv->status since all writes to
it are serialized: it is only ever set once in the isr at the end of a
transaction, and cleared while no irqs can occur.  In addition, the i2c
adapter lock guarantees that entire i2c transactions for a single
adapter are always serialized.

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@chromium.org>
---
 drivers/i2c/busses/i2c-i801.c |  107 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index f3418cf..b123133 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,6 @@
 
 /* Other settings */
 #define MAX_TIMEOUT		100
-#define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
 
 /* I801 command constants */
 #define I801_QUICK		0x00
@@ -116,7 +121,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 */
 
@@ -134,6 +143,9 @@
 				 SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \
 				 SMBHSTSTS_INTR)
 
+#define STATUS_RESULT_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
+				 SMBHSTSTS_DEV_ERR | SMBHSTSTS_INTR)
+
 /* Older devices have their ID defined in <linux/pci_ids.h> */
 #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
 #define PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS	0x1d22
@@ -151,6 +163,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 +176,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 +185,7 @@ static const char *i801_feature_names[] = {
 	"Block buffer",
 	"Block process call",
 	"I2C block read",
+	"Interrupt"
 };
 
 static unsigned int disable_features;
@@ -207,7 +226,12 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
 {
 	int result = 0;
 
-	/* If the SMBus is still busy, we give up */
+	/*
+	 * If the SMBus is still busy, we give up
+	 * Note: This timeout condition only happens when using polling
+	 * transactions.  For interrupt operation, NAK/timeout is indicated by
+	 * DEV_ERR.
+	 */
 	if (timeout) {
 		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
 		/* try to stop the current command */
@@ -265,8 +289,15 @@ 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));
+		wait_event(priv->waitq, (status = priv->status));
+		priv->status = 0;
+		return i801_check_post(priv, status, 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 +311,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 		return result;
 
 	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
+
 	return 0;
 }
 
@@ -318,7 +350,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;
@@ -336,6 +368,44 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 }
 
 /*
+ * i801 signals transaction completion with one of these interrupts:
+ *   INTR - Success
+ *   DEV_ERR - Invalid command, NAK or communication timeout
+ *   BUS_ERR - SMI# transaction collision
+ *   FAILED - transaction was canceled due to a KILL request
+ * When any of these occur, update ->status and wake up the waitq.
+ * ->status must be cleared before kicking off the next transaction.
+ */
+static irqreturn_t i801_isr(int irq, void *dev_id)
+{
+	struct i801_priv *priv = dev_id;
+	u8 pcists, hststs;
+
+	/* 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);
+
+	/*
+	 * Clear irq sources and report transaction result.
+	 * ->status must be cleared before the next transaction is started.
+	 */
+	hststs &= STATUS_RESULT_FLAGS;
+	if (hststs) {
+		outb(hststs, SMBHSTSTS(priv));
+		priv->status |= hststs;
+		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 +440,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 +641,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 +875,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 +953,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 +982,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 v2] 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 |   51 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b123133..f957eca 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -168,6 +168,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;
@@ -368,18 +375,24 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 }
 
 /*
- * i801 signals transaction completion with one of these interrupts:
+ * There are two kinds of interrupts:
+ *
+ * (1) i801 signals transaction completion with one of these interrupts:
  *   INTR - Success
  *   DEV_ERR - Invalid command, NAK or communication timeout
  *   BUS_ERR - SMI# transaction collision
  *   FAILED - transaction was canceled due to a KILL request
  * When any of these occur, update ->status and wake up the waitq.
  * ->status must be cleared before kicking off the next transaction.
+ *
+* (2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
+*     occurs for each byte of a byte-by-byte to prepare the next byte.
  */
 static irqreturn_t i801_isr(int irq, void *dev_id)
 {
 	struct i801_priv *priv = dev_id;
 	u8 pcists, hststs;
+	u8 cmd;
 
 	/* Confirm this is our interrupt */
 	pci_read_config_byte(priv->pci_dev, SMBPCISTS, &pcists);
@@ -391,6 +404,27 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	hststs = inb(SMBHSTSTS(priv));
 	dev_dbg(&priv->pci_dev->dev, "irq: hststs = %02x\n", hststs);
 
+	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 irq sources and report transaction result.
 	 * ->status must be cleared before the next transaction is started.
@@ -437,6 +471,21 @@ 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));
+		wait_event(priv->waitq, (status = priv->status));
+		priv->status = 0;
+		return i801_check_post(priv, status, 0);
+	}
+
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= I801_LAST_BYTE;
-- 
1.7.3.1

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

[Jean Delvare]

Hi Daniel,

On Fri,  6 Jan 2012 18:58:19 +0800, Daniel Kurtz wrote:
> This is a second version of a set of patches enables the Intel PCH SMBus
> controller interrupt.  It refactors the second two patches a little bit by
> relying on DEV_ERR interrupt for timeouts, instead of using an explicit
> wait_event_timeout.
> 
> The first attempt received absolutely no response. Maybe this time someone
> will be interested.

I was on vacation. But I am very interested and will review and test
your patches. There have been several attempts to add IRQ support to
i2c-i801 in the past but each time there was a blocker issue which
prevented it from making it into mainline. Hopefully this time we'll
get it there!

> 
> The SMBus Host Controller 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.

I can test on ICH10 easily, and also on ICH7 and ICH3-M if needed.

>  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.

I think I can test this.

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

But not this.

> 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.

I will do what I can with the hardware I have here.

> 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 |  199 ++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 175 insertions(+), 24 deletions(-)

-- 
Jean Delvare

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

[Daniel Kurtz]

On Fri, Jan 6, 2012 at 7:35 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Daniel,
>
> On Fri,  6 Jan 2012 18:58:19 +0800, Daniel Kurtz wrote:
>> This is a second version of a set of patches enables the Intel PCH SMBus
>> controller interrupt.  It refactors the second two patches a little bit by
>> relying on DEV_ERR interrupt for timeouts, instead of using an explicit
>> wait_event_timeout.
>>
>> The first attempt received absolutely no response. Maybe this time someone
>> will be interested.
>
> I was on vacation. But I am very interested and will review and test
> your patches. There have been several attempts to add IRQ support to
> i2c-i801 in the past but each time there was a blocker issue which
> prevented it from making it into mainline. Hopefully this time we'll
> get it there!
>
>>
>> The SMBus Host Controller 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.
>
> I can test on ICH10 easily, and also on ICH7 and ICH3-M if needed.
>
>>  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.
>
> I think I can test this.
>
>>  7) It has not been tested with smbus 'process call' transactions.
>
> But not this.
>
>> 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.
>
> I will do what I can with the hardware I have here.

Jean,
Ping?


>
>> 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 |  199 ++++++++++++++++++++++++++++++++++++-----
>>  1 files changed, 175 insertions(+), 24 deletions(-)
>
> --
> Jean Delvare

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

[Daniel Kurtz]

Jean? Anyone? ping?

On Mon, Feb 20, 2012 at 7:23 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Fri, Jan 6, 2012 at 7:35 PM, Jean Delvare <khali@linux-fr.org> wrote:
>> Hi Daniel,
>>
>> On Fri,  6 Jan 2012 18:58:19 +0800, Daniel Kurtz wrote:
>>> This is a second version of a set of patches enables the Intel PCH SMBus
>>> controller interrupt.  It refactors the second two patches a little bit by
>>> relying on DEV_ERR interrupt for timeouts, instead of using an explicit
>>> wait_event_timeout.
>>>
>>> The first attempt received absolutely no response. Maybe this time someone
>>> will be interested.
>>
>> I was on vacation. But I am very interested and will review and test
>> your patches. There have been several attempts to add IRQ support to
>> i2c-i801 in the past but each time there was a blocker issue which
>> prevented it from making it into mainline. Hopefully this time we'll
>> get it there!
>>
>>>
>>> The SMBus Host Controller 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.
>>
>> I can test on ICH10 easily, and also on ICH7 and ICH3-M if needed.
>>
>>>  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.
>>
>> I think I can test this.
>>
>>>  7) It has not been tested with smbus 'process call' transactions.
>>
>> But not this.
>>
>>> 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.
>>
>> I will do what I can with the hardware I have here.
>
> Jean,
> Ping?
>
>
>>
>>> 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 |  199 ++++++++++++++++++++++++++++++++++++-----
>>>  1 files changed, 175 insertions(+), 24 deletions(-)
>>
>> --
>> Jean Delvare

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@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) &&

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

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

[Jean Delvare]

Hi Daniel,

On Fri,  6 Jan 2012 18:58:21 +0800, Daniel Kurtz wrote:
> 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.
> 
> An i2c/smbus transaction is always terminated by one of the following
> interrupt sources: FAILED, BUS_ERR, DEV_ERR, or on success: INTR

Missing trailing dot.

> When the isr fires for one of these cases, it sets the ->status variable
> and wakes up the waitq.  The waitq then saves off the status code, and
> clears ->status (in preperation for some future transaction).

Typo: preparation.

> The SMBus controller generates an INTR irq at the end of each
> transaction where INTREN was set in the HST_CNT register.
> 
> No locking is needed around accesses to priv->status since all writes to
> it are serialized: it is only ever set once in the isr at the end of a
> transaction, and cleared while no irqs can occur.  In addition, the i2c
> adapter lock guarantees that entire i2c transactions for a single
> adapter are always serialized.

If no locking is needed, then why do you introduce and initialize a
spinlock?

> 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

I don't understand the "emulated" in this sentence.

> DS (BYTE_DONE) interrupt with byte-by-byte i2c transactions is
> implemented in a subsequent patch.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |  107 ++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 100 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f3418cf..b123133 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 */

I guess you meant "PCI status bits for SMBPCISTS"?

> +#define SMBPCISTS_INTS		8

I'd prefer this to be expressed as an hexadecimal mask.

> +
>  /* Auxiliary control register bits, ICH4+ only */
>  #define SMBAUXCTL_CRC		1
>  #define SMBAUXCTL_E32B		2
> @@ -106,7 +112,6 @@
>  
>  /* Other settings */
>  #define MAX_TIMEOUT		100
> -#define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
>  
>  /* I801 command constants */
>  #define I801_QUICK		0x00
> @@ -116,7 +121,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

This is redundant with SMBHSTCNT_KILL, right?

> +#define I801_LAST_BYTE		0x20	/* ICH5 and later */

Not true, this bit exists prior to ICH5.

>  #define I801_START		0x40
>  #define I801_PEC_EN		0x80	/* ICH3 and later */
>  
> @@ -134,6 +143,9 @@
>  				 SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \
>  				 SMBHSTSTS_INTR)
>  
> +#define STATUS_RESULT_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
> +				 SMBHSTSTS_DEV_ERR | SMBHSTSTS_INTR)
> +

You could swap the definitions so you can define STATUS_FLAGS in terms
of STATUS_RESULT_FLAGS.

>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>  #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
>  #define PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS	0x1d22
> @@ -151,6 +163,11 @@ struct i801_priv {
>  	unsigned char original_hstcfg;
>  	struct pci_dev *pci_dev;
>  	unsigned int features;
> +
> +	/* isr processing */
> +	wait_queue_head_t waitq;

This needs <linux/wait.h>.

> +	spinlock_t lock;

This needs <linux/spinlock.h> (if you need this at all...)

> +	u8 status;
>  };
>  
>  static struct pci_driver i801_driver;
> @@ -159,6 +176,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 +185,7 @@ static const char *i801_feature_names[] = {
>  	"Block buffer",
>  	"Block process call",
>  	"I2C block read",
> +	"Interrupt"

Trailing comma please.

>  };
>  
>  static unsigned int disable_features;
> @@ -207,7 +226,12 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>  {
>  	int result = 0;
>  
> -	/* If the SMBus is still busy, we give up */
> +	/*
> +	 * If the SMBus is still busy, we give up
> +	 * Note: This timeout condition only happens when using polling
> +	 * transactions.  For interrupt operation, NAK/timeout is indicated by
> +	 * DEV_ERR.
> +	 */
>  	if (timeout) {
>  		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
>  		/* try to stop the current command */
> @@ -265,8 +289,15 @@ 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));
> +		wait_event(priv->waitq, (status = priv->status));
> +		priv->status = 0;
> +		return i801_check_post(priv, status, 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 +311,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  		return result;
>  
>  	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
> +
>  	return 0;

Please avoid mixing whitespace changes with real changes.

>  }
>  
> @@ -318,7 +350,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 |

Dropping ENABLE_INT9 would have made a nice preliminary cleanup patch...

>  				  (hwpec ? I801_PEC_EN : 0));
>  	if (status)
>  		return status;
> @@ -336,6 +368,44 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  }
>  
>  /*
> + * i801 signals transaction completion with one of these interrupts:
> + *   INTR - Success
> + *   DEV_ERR - Invalid command, NAK or communication timeout
> + *   BUS_ERR - SMI# transaction collision
> + *   FAILED - transaction was canceled due to a KILL request
> + * When any of these occur, update ->status and wake up the waitq.
> + * ->status must be cleared before kicking off the next transaction.
> + */
> +static irqreturn_t i801_isr(int irq, void *dev_id)
> +{
> +	struct i801_priv *priv = dev_id;
> +	u8 pcists, hststs;
> +
> +	/* Confirm this is our interrupt */
> +	pci_read_config_byte(priv->pci_dev, SMBPCISTS, &pcists);

According to the datasheet this is a 16-bit register, you should read
it with pci_read_config_word().

> +	if (!(pcists & SMBPCISTS_INTS)) {
> +		dev_dbg(&priv->pci_dev->dev, "irq: pcists.ints not set\n");

This is expected in case of shared interrupts, right?

> +		return IRQ_NONE;
> +	}
> +
> +	hststs = inb(SMBHSTSTS(priv));

BTW, the rest of the driver is using inb_p/outb_p instead of inb/outb.
Do you believe it would be safe to use inb/outb everywhere else?

> +	dev_dbg(&priv->pci_dev->dev, "irq: hststs = %02x\n", hststs);
> +
> +	/*
> +	 * Clear irq sources and report transaction result.
> +	 * ->status must be cleared before the next transaction is started.
> +	 */
> +	hststs &= STATUS_RESULT_FLAGS;
> +	if (hststs) {

If not, something's seriously wrong, isn't it?

> +		outb(hststs, SMBHSTSTS(priv));
> +		priv->status |= hststs;
> +		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 +440,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 +641,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 +875,10 @@ static int __devinit i801_probe(struct pci_dev *dev,
>  		break;
>  	}
>  
> +	/* IRQ processing only tested on CougarPoint PCH */

I'll test on other chips, I have ICH3-M, ICH5, ICH7 and ICH10 here. The
INTS bit in PCI Status Register is new in ICH5 so your code will not
work on older chips. I think interrupts were supported before that, but
probably not shared interrupts?

OTOH I'm not completely sure why you check this bit in the first
place... hststs == 0 should be enough to detect the not-for-us shared
interrupt case?

> +	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 +953,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",

Missing "\n".

> +				dev->irq, err);
> +			goto exit_del_adapter;
> +		}

I believe order is wrong, and interrupt handler should be installed
_before_ registering the adapter. Otherwise you have a race condition
where the handler could be called before the waitqueue and spinlock are
initialized.

> +	}
>  	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 +982,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);

Here again I think order is wrong and you should delete the adapter
first, otherwise there is a small chance that you free the irq while an
SMBus transaction is being processed.

>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  	pci_release_region(dev, SMBBAR);


-- 
Jean Delvare

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

[Jean Delvare]

One more note on this...

On Fri,  6 Jan 2012 18:58:21 +0800, Daniel Kurtz wrote:
> 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.

What if bit 1 of host configuration register (SMB_SMI_EN) is set? My
understanding is that our ISR will never be called in that case, so we
should stick to polled mode? We do check it already but it's only
informative at the moment:

	if (temp & SMBHSTCFG_SMB_SMI_EN)
		dev_dbg(&dev->dev, "SMBus using interrupt SMI#\n");
	else
		dev_dbg(&dev->dev, "SMBus using PCI Interrupt\n");

-- 
Jean Delvare

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

[Jean Delvare]

On Tue, 19 Jun 2012 20:47:04 +0200, Jean Delvare wrote:
> On Fri,  6 Jan 2012 18:58:21 +0800, Daniel Kurtz wrote:
> > @@ -879,8 +953,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",
> 
> Missing "\n".
> 
> > +				dev->irq, err);
> > +			goto exit_del_adapter;
> > +		}
> 
> I believe order is wrong, and interrupt handler should be installed
> _before_ registering the adapter. Otherwise you have a race condition
> where the handler could be called before the waitqueue and spinlock are
> initialized.

Oh, and it's not a theoretical thing. I can reproducibly crash the
kernel by removing and loading the i2c-i801 driver again. Swapping
request_irq() and i2c_add_adapter() solves it.

-- 
Jean Delvare

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

[Daniel Kurtz]

On Wed, Jun 20, 2012 at 4:58 PM, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue, 19 Jun 2012 20:47:04 +0200, Jean Delvare wrote:
>> On Fri,  6 Jan 2012 18:58:21 +0800, Daniel Kurtz wrote:
>> > @@ -879,8 +953,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",
>>
>> Missing "\n".
>>
>> > +                           dev->irq, err);
>> > +                   goto exit_del_adapter;
>> > +           }
>>
>> I believe order is wrong, and interrupt handler should be installed
>> _before_ registering the adapter. Otherwise you have a race condition
>> where the handler could be called before the waitqueue and spinlock are
>> initialized.
>
> Oh, and it's not a theoretical thing. I can reproducibly crash the
> kernel by removing and loading the i2c-i801 driver again. Swapping
> request_irq() and i2c_add_adapter() solves it.

Jean,

Thanks for taking a look at these patches... and for testing them!
They were from more than 7 months ago, so it will take me a little
time to context switch back to the point where I can really digest
your feedback and update and test them again myself.  But, I wanted to
just let you know that I am reading your review comments, and will get
to it as soon as I can.
Speaking of which, I have only seen comments on patches 1 & 2 so far.
Any comments on patch 3?

-Daniel

>
> --
> Jean Delvare

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

[Jean Delvare]

On Wed, 20 Jun 2012 18:21:47 +0800, Daniel Kurtz wrote:
> Thanks for taking a look at these patches... and for testing them!

Thanks a lot for your patience!

> They were from more than 7 months ago, so it will take me a little
> time to context switch back to the point where I can really digest
> your feedback and update and test them again myself.  But, I wanted to
> just let you know that I am reading your review comments, and will get
> to it as soon as I can.

Great, thanks. On my end, I'll do some more testing on the hardware I
have, and post comments if I have more.

> Speaking of which, I have only seen comments on patches 1 & 2 so far.
> Any comments on patch 3?

Didn't have the time yesterday, I'll comment on it today. Actually it's
the very first item on my to-do list for the afternoon.

-- 
Jean Delvare

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

[Jean Delvare]

Hi Daniel,

On Fri,  6 Jan 2012 18:58:22 +0800, Daniel Kurtz wrote:
> Byte-by-byte transactions are used primarily for accessing i2c devices
> with an smbus controller.  For these transactions, for each byte that is

On recent chips, yes. On older chips (ICH3 and older), the block buffer
didn't exist, so even SMBus block transactions went for byte-by-byte.
Not that it matters much at the moment, as your interrupt support
doesn't yet cover these old chips anyway.

BTW, please use proper capitalization in comments: I2C and SMBus.

(As a side note, it might be worth checking if block buffer can now be
used with I2C block transactions too. The comment that says it doesn't
is getting old, maybe there was a bug somewhere in the code back then,
or that may have been a hardware issue possibly fixed in recent chips.)

> 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.

Here again the term "emulated" makes little sense IMHO. I2C block reads
and writes are just one of the transaction types commonly supported by
SMBus controllers.

I agree that the performance gain is great. Maybe no longer dramatic
compared to the usleep_delay() version, but certainly compared to the
msleep() version, especially if HZ < 1000. In my case I see a 2.5x
improvement which is still very good.

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

I'll get at least the I2C block read tested on ICH5.

> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   51 ++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 50 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index b123133..f957eca 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -168,6 +168,13 @@ struct i801_priv {
>  	wait_queue_head_t waitq;
>  	spinlock_t lock;
>  	u8 status;
> +
> +	/* Command state used during by isr */

Might be worth mentioning that it's only needed for byte-by-byte block
transactions.

> +	u8 cmd;
> +	bool is_read;
> +	int count;
> +	int len;
> +	u8 *data;
>  };
>  
>  static struct pci_driver i801_driver;
> @@ -368,18 +375,24 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  }
>  
>  /*
> - * i801 signals transaction completion with one of these interrupts:
> + * There are two kinds of interrupts:
> + *
> + * (1) i801 signals transaction completion with one of these interrupts:
>   *   INTR - Success
>   *   DEV_ERR - Invalid command, NAK or communication timeout
>   *   BUS_ERR - SMI# transaction collision
>   *   FAILED - transaction was canceled due to a KILL request
>   * When any of these occur, update ->status and wake up the waitq.
>   * ->status must be cleared before kicking off the next transaction.
> + *
> +* (2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
> +*     occurs for each byte of a byte-by-byte to prepare the next byte.

These stars aren't properly aligned.

>   */
>  static irqreturn_t i801_isr(int irq, void *dev_id)
>  {
>  	struct i801_priv *priv = dev_id;
>  	u8 pcists, hststs;
> +	u8 cmd;
>  
>  	/* Confirm this is our interrupt */
>  	pci_read_config_byte(priv->pci_dev, SMBPCISTS, &pcists);
> @@ -391,6 +404,27 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>  	hststs = inb(SMBHSTSTS(priv));
>  	dev_dbg(&priv->pci_dev->dev, "irq: hststs = %02x\n", hststs);
>  
> +	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));

I don't think you need the local variable "cmd". The only change is
I801_LAST_BYTE and it's only ever added, so I believe you could do:

			/* Set LAST_BYTE for last byte of read transaction */
			if (priv->count == priv->len - 1)
				priv->cmd |= I801_LAST_BYTE;
			outb_p(priv->cmd, SMBHSTCNT(priv));

as is done in the polling code. If you were worried about writing to
priv, I don't think it is an issue. If you felt safe reading from it
without a lock, writing to it is just as safe.

> +		} else if (priv->count < priv->len - 1) {

Is this just paranoia or do you believe it could actually happen? I
admit I don't see how. If it is paranoia then the same should be done
for the read part.

> +			outb(priv->data[++priv->count], SMBBLKDAT(priv));
> +			outb_p(priv->cmd | I801_START, SMBHSTCNT(priv));

I don't get the I801_START here and above. We are in the middle of the block
transaction. The polling-based code only sets I801_START at the
beginning, not for every byte, so why would it be different here?

> +		}
> +
> +		/* 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;

SMBHSTSTS_BYTE_DONE isn't part of STATUS_RESULT_FLAGS anyway, so it
will be removed from hststs right after.

> +	}
> +
>  	/*
>  	 * Clear irq sources and report transaction result.
>  	 * ->status must be cleared before the next transaction is started.
> @@ -437,6 +471,21 @@ 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));
> +		wait_event(priv->waitq, (status = priv->status));
> +		priv->status = 0;
> +		return i801_check_post(priv, status, 0);
> +	}
> +
>  	for (i = 1; i <= len; i++) {
>  		if (i == len && read_write == I2C_SMBUS_READ)
>  			smbcmd |= I801_LAST_BYTE;


-- 
Jean Delvare

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

[Jean Delvare]

Hi Daniel,

On Wed, 20 Jun 2012 15:34:49 +0200, Jean Delvare wrote:
> I'll get at least the I2C block read tested on ICH5.

Some more notes about this patch, now that have done some testing...

> > (...)
> > +		} else if (priv->count < priv->len - 1) {
> 
> Is this just paranoia or do you believe it could actually happen? I
> admit I don't see how. If it is paranoia then the same should be done
> for the read part.

This is good paranoia and I recommend doing the same for block reads,
where it is even more important. An array overrun on block write means
you'll be sending random memory bytes to your I2C device, which is
already bad. But an array overrun on block read means you'll _trash_
random memory bytes, with tragic consequences. The bug right below did
exactly this to me: the block read would never stop, and after a few
seconds only my machine would die with a different error each time,
depending on what memory got trashed.

I ended up testing for both count < len and len <= I2C_SMBUS_BLOCK_MAX,
for both reads and writes. Probably that's overkill and either should
be sufficient, but I wanted to play it really safe at first.

> 
> > +			outb(priv->data[++priv->count], SMBBLKDAT(priv));
> > +			outb_p(priv->cmd | I801_START, SMBHSTCNT(priv));
> 
> I don't get the I801_START here and above. We are in the middle of the block
> transaction. The polling-based code only sets I801_START at the
> beginning, not for every byte, so why would it be different here?

I confirm this is a serious bug. The patch broke I2C block read on my
ICH5 machine completely, until I removed these two I801_START.

After fixing this, testing is conclusive on my ICH5 machine (where the
SMBus interrupt is shared.) BTW, the debug message complaining when
pcists.ints isn't set should be dropped, in my case the interrupt is
shared with the sound chip and DVB-T card, and this caused a massive
flood of this message while I was debugging. The message isn't terribly
useful anyway IMHO, there's nothing wrong with that case.

Next step for me is testing on my ICH7-M laptop.

-- 
Jean Delvare

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

[Jean Delvare]

Hi again Daniel,

On Fri,  6 Jan 2012 18:58:19 +0800, Daniel Kurtz wrote:
> This is a second version of a set of patches enables the Intel PCH SMBus
> controller interrupt.  It refactors the second two patches a little bit by
> relying on DEV_ERR interrupt for timeouts, instead of using an explicit
> wait_event_timeout.
> 
> The first attempt received absolutely no response. Maybe this time someone
> will be interested.
> 
> The SMBus Host Controller 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.

I just finished testing (my modified version of the driver [1] which
includes all the fixes discussed in this thread so far) on my ICH7-M
machine which is running kernel 3.0. I could only test SMBus quick
write and SMBus byte read, but that worked just fine. Speed boost is
impressive, with a factor 7.0x for the former and 2.1x for the latter!
Very nice, thanks Daniel!

[1] http://khali.linux-fr.org/devel/misc/i2c-i801/
    Everyone is invited to test this on ICH5+, just add your device ID
    to the list of interrupt-enabled devices if it's not there yet,
    test and report.

>  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.

Couldn't test that either.

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

Well the driver doesn't support it yet anyway.

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

I don't think it can work at all. As I understand it, you have to fall
back to polled mode if interrupts are set to SMI#. Probably not a big
deal in practice, my 4 test systems have interrupt set to PCI type. I
think it's easier for the BIOS to do busy polling than interrupt
handling, so unless the BIOS needs the SMBus slave mode, it probably
will never set interrupt mode to SMI# for the SMBus.

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

Sounds like a reasonable assumption.

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

I planned on testing this on my ICH3-M system, but it turns out your
interrupt-based implementation only works for ICH5 and later chips. As
ICH5 and later chips all implement the block buffer, there's no reason
for the byte-by-byte-code to ever be used for SMBus block transactions.
However, the block buffer feature can be disabled for testing purpose
by passing module parameter disable_features=0x0002.

I just did, and actually it doesn't work. i2cdump shows 32 bytes no
matter what the device said. Debug log shows that the driver reads
fewer bytes from the device though, as it is supposed to. So I think
the problem is simply that the interrupt path is missing this compared
to the polled path:

		if (i == 1 && read_write == I2C_SMBUS_READ
		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
			len = inb_p(SMBHSTDAT0(priv));
			(...)
			data->block[0] = len;
		}

I.e. we don't let the caller know how many bytes we actually read from
the device. I fixed it with:

--- i2c-i801.orig/i2c-i801.c	2012-06-27 09:51:25.000000000 +0200
+++ i2c-i801/i2c-i801.c	2012-06-27 11:10:25.362853361 +0200
@@ -408,6 +408,24 @@ static irqreturn_t i801_isr(int irq, voi
 
 	if (hststs & SMBHSTSTS_BYTE_DONE) {
 		if (priv->is_read) {
+			if (priv->count == 0
+			 && (priv->cmd & 0x1c) == I801_BLOCK_DATA) {
+				priv->len = inb_p(SMBHSTDAT0(priv));
+				if (priv->len < 1
+				 || priv->len > I2C_SMBUS_BLOCK_MAX) {
+					dev_err(&priv->pci_dev->dev,
+						"Illegal SMBus block read size %d\n",
+						priv->len);
+					/* FIXME: Recover */
+					priv->len = I2C_SMBUS_BLOCK_MAX;
+				} else {
+					dev_dbg(&priv->pci_dev->dev,
+						"SMBus block read size is %d\n",
+						priv->len);
+				}
+				priv->data[-1] = priv->len;
+			}
+
 			if (priv->count < priv->len) {
 				priv->data[priv->count++] = inb(SMBBLKDAT(priv));
 

Context in your version of the driver will be somewhat different, but
you get the idea.

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

Can't test this either. Devices implementing these are quite rare.

> 
> 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.

Tested so far, successfully: ICH5, ICH7-M and ICH10. Tested and didn't
work: ICH3-M (but at least I tested there was no regression introduced
by your patches.)

I think it's time to respin this patch series with all the fixes I
suggested, unless you object to some of the non-critical changes. If
you don't have the time, just tell me and I can take care, if you don't
mind. I really would like to see this patch set go in kernel 3.6, which
means it should go into linux-next ASAP.

Thanks again,
-- 
Jean Delvare

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

[Daniel Kurtz]

Hi Jean

On Wed, Jun 27, 2012 at 5:24 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi again Daniel,
>
> On Fri,  6 Jan 2012 18:58:19 +0800, Daniel Kurtz wrote:
>> This is a second version of a set of patches enables the Intel PCH SMBus
>> controller interrupt.  It refactors the second two patches a little bit by
>> relying on DEV_ERR interrupt for timeouts, instead of using an explicit
>> wait_event_timeout.
>>
>> The first attempt received absolutely no response. Maybe this time someone
>> will be interested.
>>
>> The SMBus Host Controller 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.
>
> I just finished testing (my modified version of the driver [1] which
> includes all the fixes discussed in this thread so far) on my ICH7-M
> machine which is running kernel 3.0. I could only test SMBus quick
> write and SMBus byte read, but that worked just fine. Speed boost is
> impressive, with a factor 7.0x for the former and 2.1x for the latter!
> Very nice, thanks Daniel!

Thanks for your testing and reviews!

> [1] http://khali.linux-fr.org/devel/misc/i2c-i801/
>    Everyone is invited to test this on ICH5+, just add your device ID
>    to the list of interrupt-enabled devices if it's not there yet,
>    test and report.
>
>>  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.
>
> Couldn't test that either.
>
>>
>>  3) It has not been tested in SMBus Slave mode.
>
> Well the driver doesn't support it yet anyway.
>
>>
>>  4) It has not been tested with SMI#-type interrupts.
>
> I don't think it can work at all. As I understand it, you have to fall
> back to polled mode if interrupts are set to SMI#. Probably not a big
> deal in practice, my 4 test systems have interrupt set to PCI type. I
> think it's easier for the BIOS to do busy polling than interrupt
> handling, so unless the BIOS needs the SMBus slave mode, it probably
> will never set interrupt mode to SMI# for the SMBus.
>
>>
>>  5) The BIOS has to configure the PCH SMBus IRQ properly.
>
> Sounds like a reasonable assumption.
>
>>
>>  6) It has not been tested with a device that does byte-by-byte smbus (non-i2c)
>>     reads.
>
> I planned on testing this on my ICH3-M system, but it turns out your
> interrupt-based implementation only works for ICH5 and later chips. As
> ICH5 and later chips all implement the block buffer, there's no reason
> for the byte-by-byte-code to ever be used for SMBus block transactions.
> However, the block buffer feature can be disabled for testing purpose
> by passing module parameter disable_features=0x0002.
>
> I just did, and actually it doesn't work. i2cdump shows 32 bytes no
> matter what the device said. Debug log shows that the driver reads
> fewer bytes from the device though, as it is supposed to. So I think
> the problem is simply that the interrupt path is missing this compared
> to the polled path:
>
>                if (i == 1 && read_write == I2C_SMBUS_READ
>                 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
>                        len = inb_p(SMBHSTDAT0(priv));
>                        (...)
>                        data->block[0] = len;
>                }
>
> I.e. we don't let the caller know how many bytes we actually read from
> the device. I fixed it with:
>

I was just in the middle of finalizing a new patchset when I saw your
last email.
I incorporated (and tested for no-regressions) the snippet below.
Unfortunately, I'm not set up to test this type of transaction, so
hopefully you can double check my version of this patch with your
setup.

> --- i2c-i801.orig/i2c-i801.c    2012-06-27 09:51:25.000000000 +0200
> +++ i2c-i801/i2c-i801.c 2012-06-27 11:10:25.362853361 +0200
> @@ -408,6 +408,24 @@ static irqreturn_t i801_isr(int irq, voi
>
>        if (hststs & SMBHSTSTS_BYTE_DONE) {
>                if (priv->is_read) {
> +                       if (priv->count == 0
> +                        && (priv->cmd & 0x1c) == I801_BLOCK_DATA) {
> +                               priv->len = inb_p(SMBHSTDAT0(priv));
> +                               if (priv->len < 1
> +                                || priv->len > I2C_SMBUS_BLOCK_MAX) {
> +                                       dev_err(&priv->pci_dev->dev,
> +                                               "Illegal SMBus block read size %d\n",
> +                                               priv->len);
> +                                       /* FIXME: Recover */
> +                                       priv->len = I2C_SMBUS_BLOCK_MAX;
> +                               } else {
> +                                       dev_dbg(&priv->pci_dev->dev,
> +                                               "SMBus block read size is %d\n",
> +                                               priv->len);
> +                               }
> +                               priv->data[-1] = priv->len;
> +                       }
> +
>                        if (priv->count < priv->len) {
>                                priv->data[priv->count++] = inb(SMBBLKDAT(priv));
>
>
> Context in your version of the driver will be somewhat different, but
> you get the idea.
>
>>  7) It has not been tested with smbus 'process call' transactions.
>
> Can't test this either. Devices implementing these are quite rare.
>
>>
>> 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.
>
> Tested so far, successfully: ICH5, ICH7-M and ICH10. Tested and didn't
> work: ICH3-M (but at least I tested there was no regression introduced
> by your patches.)
>
> I think it's time to respin this patch series with all the fixes I
> suggested, unless you object to some of the non-critical changes. If
> you don't have the time, just tell me and I can take care, if you don't
> mind. I really would like to see this patch set go in kernel 3.6, which
> means it should go into linux-next ASAP.
>
> Thanks again,
> --
> Jean Delvare

Thanks again!
-Daniel

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

[Jean Delvare]

On Wed, 27 Jun 2012 21:56:34 +0800, Daniel Kurtz wrote:
> On Wed, Jun 27, 2012 at 5:24 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > I planned on testing this on my ICH3-M system, but it turns out your
> > interrupt-based implementation only works for ICH5 and later chips. As
> > ICH5 and later chips all implement the block buffer, there's no reason
> > for the byte-by-byte-code to ever be used for SMBus block transactions.
> > However, the block buffer feature can be disabled for testing purpose
> > by passing module parameter disable_features=0x0002.
> >
> > I just did, and actually it doesn't work. i2cdump shows 32 bytes no
> > matter what the device said. Debug log shows that the driver reads
> > fewer bytes from the device though, as it is supposed to. So I think
> > the problem is simply that the interrupt path is missing this compared
> > to the polled path:
> >
> >                if (i == 1 && read_write == I2C_SMBUS_READ
> >                 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> >                        len = inb_p(SMBHSTDAT0(priv));
> >                        (...)
> >                        data->block[0] = len;
> >                }
> >
> > I.e. we don't let the caller know how many bytes we actually read from
> > the device. I fixed it with:
> >
> 
> I was just in the middle of finalizing a new patchset when I saw your
> last email.
> I incorporated (and tested for no-regressions) the snippet below.
> Unfortunately, I'm not set up to test this type of transaction, so
> hopefully you can double check my version of this patch with your
> setup.

Sure, no problem, I will do another full round of testing on the new
patchset.

-- 
Jean Delvare