[PATCH 0/3 v4] i2c-i801: Enable irq

[PATCH 0/3 v4] i2c-i801: Enable irq

[Jean Delvare]

This is a respin of Daniel's patch set adding support for interrupts to
the i2c-i801 driver, incorporating the feedback from review and testing
results. I think it is ready to go upstream. Further cleanups,
optimizations and enablements can happen later.

Daniel Kurtz (2):
  i2c-i801: Enable irq for SMBus transactions
  i2c-i801: Enable irq for byte_by_byte transactions

Jean Delvare (1):
  i2c-i801: Enable interrupts on ICH5/7/8/9/10

-- 
Jean Delvare

[PATCH 1/3 v4] i2c-i801: Enable IRQ for SMBus transactions

[Jean Delvare]

From: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Add a new 'feature' to i2c-i801 to enable using PCI 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 preparation 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 interrupts 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 I2C reads or writes.  The use of the DS
(BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in
a subsequent patch.

The interrupt feature has only been enabled for COUGARPOINT hardware.
In addition, it is disabled if SMBus is using the SMI# interrupt.

Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
I updated the documentation too.

 Documentation/i2c/busses/i2c-i801 |   13 +++-
 drivers/i2c/busses/i2c-i801.c     |   98 +++++++++++++++++++++++++++++++++++--
 2 files changed, 104 insertions(+), 7 deletions(-)

--- linux-3.5-rc6.orig/drivers/i2c/busses/i2c-i801.c	2012-07-10 11:58:01.766298677 +0200
+++ linux-3.5-rc6/drivers/i2c/busses/i2c-i801.c	2012-07-10 13:50:38.083239548 +0200
@@ -60,10 +60,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>
@@ -76,6 +78,7 @@
 #include <linux/io.h>
 #include <linux/dmi.h>
 #include <linux/slab.h>
+#include <linux/wait.h>
 
 /* I801 SMBus address offsets */
 #define SMBHSTSTS(p)	(0 + (p)->smba)
@@ -91,8 +94,12 @@
 
 /* PCI Address Constants */
 #define SMBBAR		4
+#define SMBPCISTS	0x006
 #define SMBHSTCFG	0x040
 
+/* Host status bits for SMBPCISTS */
+#define SMBPCISTS_INTS		0x08
+
 /* Host configuration bits for SMBHSTCFG */
 #define SMBHSTCFG_HST_EN	1
 #define SMBHSTCFG_SMB_SMI_EN	2
@@ -155,6 +162,10 @@ struct i801_priv {
 	unsigned char original_hstcfg;
 	struct pci_dev *pci_dev;
 	unsigned int features;
+
+	/* isr processing */
+	wait_queue_head_t waitq;
+	u8 status;
 };
 
 static struct pci_driver i801_driver;
@@ -163,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)
 
@@ -171,6 +183,7 @@ static const char *i801_feature_names[]
 	"Block buffer",
 	"Block process call",
 	"I2C block read",
+	"Interrupt",
 };
 
 static unsigned int disable_features;
@@ -215,7 +228,12 @@ static int i801_check_post(struct i801_p
 {
 	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 (unlikely(status < 0)) {
 		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
 		/* try to stop the current command */
@@ -305,6 +323,14 @@ static int i801_transaction(struct i801_
 	if (result < 0)
 		return result;
 
+	if (priv->features & FEATURE_IRQ) {
+		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
+		       SMBHSTCNT(priv));
+		wait_event(priv->waitq, (status = priv->status));
+		priv->status = 0;
+		return i801_check_post(priv, status);
+	}
+
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * SMBSCMD are passed in xact */
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
@@ -348,6 +374,43 @@ static int i801_block_transaction_by_blo
 }
 
 /*
+ * 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;
+	u16 pcists;
+	u8 status;
+
+	/* Confirm this is our interrupt */
+	pci_read_config_word(priv->pci_dev, SMBPCISTS, &pcists);
+	if (!(pcists & SMBPCISTS_INTS))
+		return IRQ_NONE;
+
+	status = inb_p(SMBHSTSTS(priv));
+	dev_dbg(&priv->pci_dev->dev, "irq: status = %02x\n", status);
+
+	/*
+	 * Clear irq sources and report transaction result.
+	 * ->status must be cleared before the next transaction is started.
+	 */
+	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
+	if (status) {
+		outb_p(status, SMBHSTSTS(priv));
+		priv->status |= status;
+		wake_up(&priv->waitq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
  * For "byte-by-byte" block transactions:
  *   I2C write uses cmd=I801_BLOCK_DATA, I2C_EN=1
  *   I2C read uses cmd=I801_I2C_BLOCK_DATA
@@ -799,6 +862,10 @@ static int __devinit i801_probe(struct p
 		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))
@@ -846,16 +913,31 @@ static int __devinit i801_probe(struct p
 	}
 	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, temp);
 
-	if (temp & SMBHSTCFG_SMB_SMI_EN)
+	if (temp & SMBHSTCFG_SMB_SMI_EN) {
 		dev_dbg(&dev->dev, "SMBus using interrupt SMI#\n");
-	else
+		/* Disable SMBus interrupt feature if SMBus using SMI# */
+		priv->features &= ~FEATURE_IRQ;
+	} else {
 		dev_dbg(&dev->dev, "SMBus using PCI Interrupt\n");
+	}
 
 	/* Clear special mode bits */
 	if (priv->features & (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER))
 		outb_p(inb_p(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
+	if (priv->features & FEATURE_IRQ) {
+		init_waitqueue_head(&priv->waitq);
+
+		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\n",
+				dev->irq, err);
+			goto exit_release;
+		}
+	}
+
 	/* set up the sysfs linkage to our parent device */
 	priv->adapter.dev.parent = &dev->dev;
 
@@ -867,14 +949,18 @@ static int __devinit i801_probe(struct p
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
-		goto exit_release;
+		goto exit_free_irq;
 	}
 
 	i801_probe_optional_slaves(priv);
 
 	pci_set_drvdata(dev, priv);
+
 	return 0;
 
+exit_free_irq:
+	if (priv->features & FEATURE_IRQ)
+		free_irq(dev->irq, priv);
 exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
@@ -888,7 +974,11 @@ static void __devexit i801_remove(struct
 
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+
+	if (priv->features & FEATURE_IRQ)
+		free_irq(dev->irq, priv);
 	pci_release_region(dev, SMBBAR);
+
 	pci_set_drvdata(dev, NULL);
 	kfree(priv);
 	/*
--- linux-3.5-rc6.orig/Documentation/i2c/busses/i2c-i801	2012-05-27 22:55:43.000000000 +0200
+++ linux-3.5-rc6/Documentation/i2c/busses/i2c-i801	2012-07-10 13:57:25.414254138 +0200
@@ -38,9 +38,10 @@ Module Parameters
 Disable selected features normally supported by the device. This makes it
 possible to work around possible driver or hardware bugs if the feature in
 question doesn't work as intended for whatever reason. Bit values:
-  1  disable SMBus PEC
-  2  disable the block buffer
-  8  disable the I2C block read functionality
+ 0x01  disable SMBus PEC
+ 0x02  disable the block buffer
+ 0x08  disable the I2C block read functionality
+ 0x10  don't use interrupts
 
 
 Description
@@ -86,6 +87,12 @@ SMBus 2.0 Support
 The 82801DB (ICH4) and later chips support several SMBus 2.0 features.
 
 
+Interrupt Support
+-----------------
+
+PCI interrupt support is supported on the 82801EB (ICH5) and later chips.
+
+
 Hidden ICH SMBus
 ----------------
 

-- 
Jean Delvare

[PATCH 2/3 v4] i2c-i801: Enable interrupts on ICH5/7/8/9/10

[Jean Delvare]

Enable interrupts on more devices. ICH5, ICH7(-M) and ICH10 have been
tested to work OK. ICH8 and ICH9 are expected to work just fine as
they are very close to ICH7 and ICH10.

Ultimately we want to enable this feature on at least every device
since the ICH5, but for now we limit the exposure. We'll enable it for
other devices if we don't get negative feedback.

As a bonus, let the user know when interrupts are used.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

--- linux-3.5-rc6.orig/drivers/i2c/busses/i2c-i801.c	2012-07-10 14:03:24.942225018 +0200
+++ linux-3.5-rc6/drivers/i2c/busses/i2c-i801.c	2012-07-10 14:05:39.764553569 +0200
@@ -862,8 +862,14 @@ static int __devinit i801_probe(struct p
 		break;
 	}
 
-	/* IRQ processing only tested on CougarPoint PCH */
-	if (dev->device == PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS)
+	/* IRQ processing tested on CougarPoint PCH, ICH5, ICH7-M and ICH10 */
+	if (dev->device == PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS ||
+	    dev->device == PCI_DEVICE_ID_INTEL_82801EB_3 ||
+	    dev->device == PCI_DEVICE_ID_INTEL_ICH7_17 ||
+	    dev->device == PCI_DEVICE_ID_INTEL_ICH8_5 ||
+	    dev->device == PCI_DEVICE_ID_INTEL_ICH9_6 ||
+	    dev->device == PCI_DEVICE_ID_INTEL_ICH10_4 ||
+	    dev->device == PCI_DEVICE_ID_INTEL_ICH10_5)
 		priv->features |= FEATURE_IRQ;
 
 	/* Disable features on user request */
@@ -917,8 +923,6 @@ static int __devinit i801_probe(struct p
 		dev_dbg(&dev->dev, "SMBus using interrupt SMI#\n");
 		/* Disable SMBus interrupt feature if SMBus using SMI# */
 		priv->features &= ~FEATURE_IRQ;
-	} else {
-		dev_dbg(&dev->dev, "SMBus using PCI Interrupt\n");
 	}
 
 	/* Clear special mode bits */
@@ -936,6 +940,7 @@ static int __devinit i801_probe(struct p
 				dev->irq, err);
 			goto exit_release;
 		}
+		dev_info(&dev->dev, "SMBus using PCI Interrupt\n");
 	}
 
 	/* set up the sysfs linkage to our parent device */

-- 
Jean Delvare

[PATCH 3/3 v4] i2c-i801: Enable IRQ for byte_by_byte transactions

[Jean Delvare]

From: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

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 faster I2C 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, as well as I2C read
block transfers on ICH5.

Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c |   85 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 78 insertions(+), 7 deletions(-)

--- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c	2012-07-07 21:40:03.000000000 +0200
+++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c	2012-07-08 15:18:53.278760438 +0200
@@ -166,6 +166,13 @@ struct i801_priv {
 	/* isr processing */
 	wait_queue_head_t waitq;
 	u8 status;
+
+	/* Command state used by isr for byte-by-byte block transactions */
+	u8 cmd;
+	bool is_read;
+	int count;
+	int len;
+	u8 *data;
 };
 
 static struct pci_driver i801_driver;
@@ -373,14 +380,60 @@ static int i801_block_transaction_by_blo
 	return 0;
 }
 
+static void i801_isr_byte_done(struct i801_priv *priv)
+{
+	if (priv->is_read) {
+		/* For SMBus block reads, length is received with first byte */
+		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
+		    (priv->count == 0)) {
+			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;
+		}
+
+		/* Read next byte */
+		if (priv->count < priv->len)
+			priv->data[priv->count++] = inb(SMBBLKDAT(priv));
+		else
+			dev_dbg(&priv->pci_dev->dev,
+				"Discarding extra byte on block read\n");
+
+		/* Set LAST_BYTE for last byte of read transaction */
+		if (priv->count == priv->len - 1)
+			outb_p(priv->cmd | SMBHSTCNT_LAST_BYTE,
+			       SMBHSTCNT(priv));
+	} else if (priv->count < priv->len - 1) {
+		/* Write next byte, except for IRQ after last byte */
+		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
+	}
+
+	/* Clear BYTE_DONE to continue with next byte */
+	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(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.
+ * 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)
 {
@@ -396,6 +449,9 @@ static irqreturn_t i801_isr(int irq, voi
 	status = inb_p(SMBHSTSTS(priv));
 	dev_dbg(&priv->pci_dev->dev, "irq: status = %02x\n", status);
 
+	if (status & SMBHSTSTS_BYTE_DONE)
+		i801_isr_byte_done(priv);
+
 	/*
 	 * Clear irq sources and report transaction result.
 	 * ->status must be cleared before the next transaction is started.
@@ -442,6 +498,21 @@ static int i801_block_transaction_byte_b
 	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 |= SMBHSTCNT_LAST_BYTE;
+		priv->cmd = smbcmd | SMBHSTCNT_INTREN;
+		priv->len = len;
+		priv->count = 0;
+		priv->data = &data->block[1];
+
+		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
+		wait_event(priv->waitq, (status = priv->status));
+		priv->status = 0;
+		return i801_check_post(priv, status);
+	}
+
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;

-- 
Jean Delvare