[PATCH 0/8 v3] i2c: i801: enable irq

[PATCH 0/8 v3] i2c: i801: enable irq

[Daniel Kurtz]

v3 incoporates much review feedback from Jean Delvare.
I think I got most of the feedback in this version, but please double check!

The patchset is based on linus/master, and tested by me only on a Cougar Point
(Intel 6 Series PCH) SMBus controller, although Jean has tested earlier,
modified versions of these patches on ICH5, ICH7-M and ICH10.
This version should also work with no regressions on ICH3-M, even for interrupt
enabled SMBus byte-by-byte reads.

Note: The interrupt byte-by-byte patches have not yet been tested for SMBus
(not I2C) write transactions.  Testing help would be appreciated.

Daniel Kurtz (8):
  i2c: i801: refactor use of LAST_BYTE
    i801_block_transaction_byte_by_byte
  i2c: i801: optimize waiting for HWPEC to finish
  i2c: i801: check INTR after every transaction
  i2c: i801: check and return errors during byte-by-byte transfers
  i2c: i801: rename some SMBHSTCNT bit constants
  i2c: i801: drop ENABLE_INT9
  i2c: i801: enable irq for i801 smbus transactions
  i2c: i801: enable irq for byte_by_byte transactions

 drivers/i2c/busses/i2c-i801.c |  263 ++++++++++++++++++++++++++++++++---------
 1 files changed, 205 insertions(+), 58 deletions(-)

-- 
1.7.7.3
.

[PATCH 1/8 v3] i2c: i801: refactor use of LAST_BYTE i801_block_transaction_byte_by_byte

[Daniel Kurtz]

As a slight optimization, pull some logic out of the polling 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.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ae2945a..51e11eb 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -117,8 +117,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 */
 
@@ -338,6 +337,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	return 0;
 }
 
+/*
+ * For "byte-by-byte" block transactions:
+ *   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,
@@ -360,19 +364,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)
-- 
1.7.7.3

[PATCH 2/8 v3] i2c: i801: optimize waiting for HWPEC to finish

[Daniel Kurtz]

When a transaction has finished (including the PEC), the SMBus controller
sets the INTR bit.
Slightly optimize the polling loop by reading status before the first
sleep.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 51e11eb..8b74e1e 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -291,11 +291,11 @@ static void i801_wait_hwpec(struct i801_priv *priv)
 	int timeout = 0;
 	int status;
 
-	do {
+	status = inb_p(SMBHSTSTS(priv));
+	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
 		usleep_range(250, 500);
 		status = inb_p(SMBHSTSTS(priv));
-	} while ((!(status & SMBHSTSTS_INTR))
-		 && (timeout++ < MAX_RETRIES));
+	}
 
 	if (timeout > MAX_RETRIES)
 		dev_dbg(&priv->pci_dev->dev, "PEC Timeout!\n");
-- 
1.7.7.3

[PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Daniel Kurtz]

Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
INTR should be checked & cleared separately, only after BYTE_DONE is
first cleared:

  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.

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

Currently, the INTR bit was only checked & cleared separately if the PEC
was used.
This patch checks and clears INTR at the very end of every successful
transaction, regardless of whether the PEC is used.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8b74e1e..6a53338 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
 	return result;
 }
 
+/* wait for INTR bit as advised by Intel */
+static void i801_wait_intr(struct i801_priv *priv)
+{
+	int timeout = 0;
+	int status;
+
+	status = inb_p(SMBHSTSTS(priv));
+	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
+		usleep_range(250, 500);
+		status = inb_p(SMBHSTSTS(priv));
+	}
+
+	if (timeout > MAX_RETRIES)
+		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
+
+	outb_p(status, SMBHSTSTS(priv));
+}
+
 static int i801_transaction(struct i801_priv *priv, int xact)
 {
 	int status;
@@ -281,26 +299,9 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 	if (result < 0)
 		return result;
 
-	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
-	return 0;
-}
+	i801_wait_intr(priv);
 
-/* wait for INTR bit as advised by Intel */
-static void i801_wait_hwpec(struct i801_priv *priv)
-{
-	int timeout = 0;
-	int status;
-
-	status = inb_p(SMBHSTSTS(priv));
-	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
-		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
-	}
-
-	if (timeout > MAX_RETRIES)
-		dev_dbg(&priv->pci_dev->dev, "PEC Timeout!\n");
-
-	outb_p(status, SMBHSTSTS(priv));
+	return 0;
 }
 
 static int i801_block_transaction_by_block(struct i801_priv *priv,
@@ -416,9 +417,11 @@ 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));
 	}
 
+	i801_wait_intr(priv);
+
 	return 0;
 }
 
@@ -474,9 +477,6 @@ static int i801_block_transaction(struct i801_priv *priv,
 							     read_write,
 							     command, hwpec);
 
-	if (result == 0 && hwpec)
-		i801_wait_hwpec(priv);
-
 	if (command == I2C_SMBUS_I2C_BLOCK_DATA
 	 && read_write == I2C_SMBUS_WRITE) {
 		/* restore saved configuration register value */
-- 
1.7.7.3

[PATCH 4/8 v3] i2c: i801: check and return errors during byte-by-byte transfers

[Daniel Kurtz]

If an error is detected in the polling loop, abort the transaction and
return an error code.

 * DEV_ERR is set if the device does not respond with an acknowledge, and
the SMBus controller times out (minimum 25ms).
 * BUS_ERR is set if a bus arbitration collision is detected.  In other
words, when the SMBus controller tries to generate a START condition, but
detects that the SMBDATA is being held low, usually by another SMBus/I2C
master.
 * FAILED is only set if a the transaction is set by software (using the
SMBHSTCNT KILL bit).

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 6a53338..ba56b74 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -131,9 +131,11 @@
 #define SMBHSTSTS_INTR		0x02
 #define SMBHSTSTS_HOST_BUSY	0x01
 
-#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_FAILED | \
-				 SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \
-				 SMBHSTSTS_INTR)
+#define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
+				 SMBHSTSTS_DEV_ERR)
+
+#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
+				 STATUS_ERROR_FLAGS)
 
 /* Older devices have their ID defined in <linux/pci_ids.h> */
 #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
@@ -385,7 +387,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		do {
 			usleep_range(250, 500);
 			status = inb_p(SMBHSTSTS(priv));
-		} while ((!(status & SMBHSTSTS_BYTE_DONE))
+		} while (!(status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS))
 			 && (timeout++ < MAX_RETRIES));
 
 		result = i801_check_post(priv, status, timeout > MAX_RETRIES);
-- 
1.7.7.3

[PATCH 5/8 v3] i2c: i801: rename some SMBHSTCNT bit constants

[Daniel Kurtz]

Rename the SMBHSTCNT register bit access constants to match the style of
other register bits.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ba56b74..c57bb0c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -102,9 +102,6 @@
 #define SMBAUXCTL_CRC		1
 #define SMBAUXCTL_E32B		2
 
-/* kill bit for SMBHSTCNT */
-#define SMBHSTCNT_KILL		2
-
 /* Other settings */
 #define MAX_RETRIES		400
 #define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
@@ -117,9 +114,13 @@
 #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
-#define I801_START		0x40
-#define I801_PEC_EN		0x80	/* ICH3 and later */
+
+/* I801 Host Control register bits */
+#define SMBHSTCNT_INTREN	0x01
+#define SMBHSTCNT_KILL		0x02
+#define SMBHSTCNT_LAST_BYTE	0x20
+#define SMBHSTCNT_START		0x40
+#define SMBHSTCNT_PEC_EN	0x80	/* ICH3 and later */
 
 /* I801 Hosts Status register bits */
 #define SMBHSTSTS_BYTE_DONE	0x80
@@ -289,7 +290,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * INTREN, SMBSCMD are passed in xact */
-	outb_p(xact | I801_START, SMBHSTCNT(priv));
+	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
 	/* We will always wait for a fraction of a second! */
 	do {
@@ -324,7 +325,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);
+				  SMBHSTCNT_PEC_EN * hwpec);
 	if (status)
 		return status;
 
@@ -375,11 +376,11 @@ 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;
+			smbcmd |= SMBHSTCNT_LAST_BYTE;
 		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
 
 		if (i == 1)
-			outb_p(inb(SMBHSTCNT(priv)) | I801_START,
+			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
 			       SMBHSTCNT(priv));
 
 		/* We will always wait for a fraction of a second! */
-- 
1.7.7.3

[PATCH 5/8 v3] i2c: i801: rename some SMBHSTCNT bit constants

[Daniel Kurtz]

Rename the SMBHSTCNT register bit access constants to match the style of
other register bits.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ba56b74..c57bb0c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -102,9 +102,6 @@
 #define SMBAUXCTL_CRC		1
 #define SMBAUXCTL_E32B		2
 
-/* kill bit for SMBHSTCNT */
-#define SMBHSTCNT_KILL		2
-
 /* Other settings */
 #define MAX_RETRIES		400
 #define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
@@ -117,9 +114,13 @@
 #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
-#define I801_START		0x40
-#define I801_PEC_EN		0x80	/* ICH3 and later */
+
+/* I801 Host Control register bits */
+#define SMBHSTCNT_INTREN	0x01
+#define SMBHSTCNT_KILL		0x02
+#define SMBHSTCNT_LAST_BYTE	0x20
+#define SMBHSTCNT_START		0x40
+#define SMBHSTCNT_PEC_EN	0x80	/* ICH3 and later */
 
 /* I801 Hosts Status register bits */
 #define SMBHSTSTS_BYTE_DONE	0x80
@@ -289,7 +290,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * INTREN, SMBSCMD are passed in xact */
-	outb_p(xact | I801_START, SMBHSTCNT(priv));
+	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
 	/* We will always wait for a fraction of a second! */
 	do {
@@ -324,7 +325,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);
+				  SMBHSTCNT_PEC_EN * hwpec);
 	if (status)
 		return status;
 
@@ -375,11 +376,11 @@ 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;
+			smbcmd |= SMBHSTCNT_LAST_BYTE;
 		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
 
 		if (i == 1)
-			outb_p(inb(SMBHSTCNT(priv)) | I801_START,
+			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
 			       SMBHSTCNT(priv));
 
 		/* We will always wait for a fraction of a second! */
-- 
1.7.7.3

[PATCH 6/8 v3] i2c: i801: drop ENABLE_INT9

[Daniel Kurtz]

Later patches enable interrupts.  This preliminary patch removes the older
unsupported ENABLE_INT9 flag.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c57bb0c..6fa2a0b 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -104,7 +104,6 @@
 
 /* Other settings */
 #define MAX_RETRIES		400
-#define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
 
 /* I801 command constants */
 #define I801_QUICK		0x00
@@ -289,7 +288,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 		return result;
 
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
-	 * INTREN, SMBSCMD are passed in xact */
+	 * SMBSCMD are passed in xact */
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
 	/* We will always wait for a fraction of a second! */
@@ -324,7 +323,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 |
 				  SMBHSTCNT_PEC_EN * hwpec);
 	if (status)
 		return status;
@@ -377,7 +376,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 |= SMBHSTCNT_LAST_BYTE;
-		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
+		outb_p(smbcmd, SMBHSTCNT(priv));
 
 		if (i == 1)
 			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
@@ -567,7 +566,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
-- 
1.7.7.3

[PATCH 7/8 v3] 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 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 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 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@chromium.org>
---
 drivers/i2c/busses/i2c-i801.c |   93 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 6fa2a0b..6bfedc0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -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)
@@ -134,8 +137,9 @@
 #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
 				 SMBHSTSTS_DEV_ERR)
 
-#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
-				 STATUS_ERROR_FLAGS)
+#define STATUS_RESULT_FLAGS	(SMBHSTSTS_INTR | STATUS_ERROR_FLAGS)
+
+#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | STATUS_RESULT_FLAGS)
 
 /* Older devices have their ID defined in <linux/pci_ids.h> */
 #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
@@ -155,6 +159,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 +171,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 +180,7 @@ static const char *i801_feature_names[] = {
 	"Block buffer",
 	"Block process call",
 	"I2C block read",
+	"Interrupt",
 };
 
 static unsigned int disable_features;
@@ -211,7 +221,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 */
@@ -287,6 +302,14 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 	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, 0);
+	}
+
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * SMBSCMD are passed in xact */
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
@@ -341,6 +364,37 @@ 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 hststs;
+
+	hststs = inb_p(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_p(hststs, SMBHSTSTS(priv));
+		priv->status |= hststs;
+		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
@@ -801,6 +855,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))
@@ -848,16 +906,31 @@ static int __devinit i801_probe(struct pci_dev *dev,
 	}
 	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;
 
@@ -869,14 +942,18 @@ static int __devinit i801_probe(struct pci_dev *dev,
 	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:
@@ -891,6 +968,10 @@ static void __devexit i801_remove(struct pci_dev *dev)
 	i2c_del_adapter(&priv->adapter);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 	pci_release_region(dev, SMBBAR);
+
+	if (priv->features & FEATURE_IRQ)
+		free_irq(dev->irq, priv);
+
 	pci_set_drvdata(dev, NULL);
 	kfree(priv);
 	/*
-- 
1.7.7.3

[PATCH 8/8 v3] 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 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.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 6bfedc0..bbd3508 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -163,6 +163,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;
@@ -363,14 +370,53 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	return 0;
 }
 
+static void i801_isr_byte_done(struct i801_priv *priv)
+{
+	/* For SMBus block reads, length is first byte read */
+	if (priv->is_read && ((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;
+	} else if (priv->is_read) {
+		priv->data[priv->count++] = inb(SMBBLKDAT(priv));
+		/* Set LAST_BYTE for last byte of read transaction */
+		if (priv->count == priv->len - 1)
+			priv->cmd |= SMBHSTCNT_LAST_BYTE;
+		outb_p(priv->cmd, 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));
+		outb_p(priv->cmd, SMBHSTCNT(priv));
+	}
+
+	/* Clear BYTE_DONE to start next transaction. */
+	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)
 {
@@ -380,6 +426,9 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	hststs = inb_p(SMBHSTSTS(priv));
 	dev_dbg(&priv->pci_dev->dev, "irq: hststs = %02x\n", hststs);
 
+	if (hststs & 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.
@@ -427,6 +476,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 |= 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, 0);
+	}
+
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
-- 
1.7.7.3

Re: [PATCH 1/8 v3] i2c: i801: refactor use of LAST_BYTE i801_block_transaction_byte_by_byte

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:08 +0800, Daniel Kurtz wrote:
> As a slight optimization, pull some logic out of the polling 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.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   28 ++++++++++++++--------------
>  1 files changed, 14 insertions(+), 14 deletions(-)
> (...)

Applied, thanks.

-- 
Jean Delvare

Re: [PATCH 2/8 v3] i2c: i801: optimize waiting for HWPEC to finish

[Jean Delvare]

Hi Daniel,

On Wed, 27 Jun 2012 21:54:09 +0800, Daniel Kurtz wrote:
> When a transaction has finished (including the PEC), the SMBus controller
> sets the INTR bit.
> Slightly optimize the polling loop by reading status before the first
> sleep.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 51e11eb..8b74e1e 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -291,11 +291,11 @@ static void i801_wait_hwpec(struct i801_priv *priv)
>  	int timeout = 0;
>  	int status;
>  
> -	do {
> +	status = inb_p(SMBHSTSTS(priv));
> +	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
>  		usleep_range(250, 500);
>  		status = inb_p(SMBHSTSTS(priv));
> -	} while ((!(status & SMBHSTSTS_INTR))
> -		 && (timeout++ < MAX_RETRIES));
> +	}
>  
>  	if (timeout > MAX_RETRIES)
>  		dev_dbg(&priv->pci_dev->dev, "PEC Timeout!\n");

Hmm, I would be very cautious here. Deep memories whisper to me
that we have sometimes done it that way in the past due to hardware
bugs. I think it was for the PIIX4 and not the 82801, but I wouldn't be
surprised if early 82801 chips had some hardware bugs as well.

Also note that optimizing the polling code isn't a priority when you're
about to enable interrupts for most chips supported by the driver. If
anything, leaving the polling code unchanged for now could even be a
goal, to make sure that users can disable interrupts if that doesn't
work for them, and things keep working as before.

Plus, PEC isn't used that much (neither you nor me can test it). And
lastly, are you really certain that there is a benefit? Are you sure
that the SMBus controller is always faster to receive and process the
PEC byte than the CPU is to reach the INTR check?

Unless you have actual numbers showing a significant improvement, I'd
rather postpone this change.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
> Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
> INTR should be checked & cleared separately, only after BYTE_DONE is
> first cleared:
> 
>   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.
> 
> [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
> 
> Currently, the INTR bit was only checked & cleared separately if the PEC
> was used.
> This patch checks and clears INTR at the very end of every successful
> transaction, regardless of whether the PEC is used.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
>  1 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 8b74e1e..6a53338 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>  	return result;
>  }
>  
> +/* wait for INTR bit as advised by Intel */
> +static void i801_wait_intr(struct i801_priv *priv)
> +{
> +	int timeout = 0;
> +	int status;
> +
> +	status = inb_p(SMBHSTSTS(priv));
> +	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
> +		usleep_range(250, 500);
> +		status = inb_p(SMBHSTSTS(priv));
> +	}

Per my comment on previous patch, I've swapped the logic here to be in
line with what we had before. I have no objection to trying this change
again, but later, and only if you have actual numbers to back it up.

> +
> +	if (timeout > MAX_RETRIES)
> +		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> +
> +	outb_p(status, SMBHSTSTS(priv));

Wouldn't it be more correct to only write the INTR bit? Writing back
the whole 8 bits frightens me a little especially because of bit
INUSE_STS. If we ever want to support this feature, I think we have to
first ban writing back the whole status to register HST_STS. And this
is the only place where we still do AFAICS.

(This isn't a regression from your patch, the old code was already
doing that, but it might be the opportunity to fix it.)

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
> Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
> INTR should be checked & cleared separately, only after BYTE_DONE is
> first cleared:
> 
>   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.
> 
> [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
> 
> Currently, the INTR bit was only checked & cleared separately if the PEC
> was used.
> This patch checks and clears INTR at the very end of every successful
> transaction, regardless of whether the PEC is used.
> 
> Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
>  1 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 8b74e1e..6a53338 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>  	return result;
>  }
>  
> +/* wait for INTR bit as advised by Intel */
> +static void i801_wait_intr(struct i801_priv *priv)
> +{
> +	int timeout = 0;
> +	int status;
> +
> +	status = inb_p(SMBHSTSTS(priv));
> +	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
> +		usleep_range(250, 500);
> +		status = inb_p(SMBHSTSTS(priv));
> +	}

Per my comment on previous patch, I've swapped the logic here to be in
line with what we had before. I have no objection to trying this change
again, but later, and only if you have actual numbers to back it up.

> +
> +	if (timeout > MAX_RETRIES)
> +		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> +
> +	outb_p(status, SMBHSTSTS(priv));

Wouldn't it be more correct to only write the INTR bit? Writing back
the whole 8 bits frightens me a little especially because of bit
INUSE_STS. If we ever want to support this feature, I think we have to
first ban writing back the whole status to register HST_STS. And this
is the only place where we still do AFAICS.

(This isn't a regression from your patch, the old code was already
doing that, but it might be the opportunity to fix it.)

-- 
Jean Delvare

Re: [PATCH 4/8 v3] i2c: i801: check and return errors during byte-by-byte transfers

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:11 +0800, Daniel Kurtz wrote:
> If an error is detected in the polling loop, abort the transaction and
> return an error code.
> 
>  * DEV_ERR is set if the device does not respond with an acknowledge, and
> the SMBus controller times out (minimum 25ms).
>  * BUS_ERR is set if a bus arbitration collision is detected.  In other
> words, when the SMBus controller tries to generate a START condition, but
> detects that the SMBDATA is being held low, usually by another SMBus/I2C
> master.
>  * FAILED is only set if a the transaction is set by software (using the
> SMBHSTCNT KILL bit).

Not quite sure what you mean with "set by software". Other than this,
patch looks good, applied, thanks.

> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 6a53338..ba56b74 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -131,9 +131,11 @@
>  #define SMBHSTSTS_INTR		0x02
>  #define SMBHSTSTS_HOST_BUSY	0x01
>  
> -#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_FAILED | \
> -				 SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \
> -				 SMBHSTSTS_INTR)
> +#define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
> +				 SMBHSTSTS_DEV_ERR)
> +
> +#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
> +				 STATUS_ERROR_FLAGS)
>  
>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>  #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
> @@ -385,7 +387,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		do {
>  			usleep_range(250, 500);
>  			status = inb_p(SMBHSTSTS(priv));
> -		} while ((!(status & SMBHSTSTS_BYTE_DONE))
> +		} while (!(status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS))
>  			 && (timeout++ < MAX_RETRIES));
>  
>  		result = i801_check_post(priv, status, timeout > MAX_RETRIES);


-- 
Jean Delvare

Re: [PATCH 4/8 v3] i2c: i801: check and return errors during byte-by-byte transfers

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:11 +0800, Daniel Kurtz wrote:
> If an error is detected in the polling loop, abort the transaction and
> return an error code.
> 
>  * DEV_ERR is set if the device does not respond with an acknowledge, and
> the SMBus controller times out (minimum 25ms).
>  * BUS_ERR is set if a bus arbitration collision is detected.  In other
> words, when the SMBus controller tries to generate a START condition, but
> detects that the SMBDATA is being held low, usually by another SMBus/I2C
> master.
>  * FAILED is only set if a the transaction is set by software (using the
> SMBHSTCNT KILL bit).

Not quite sure what you mean with "set by software". Other than this,
patch looks good, applied, thanks.

> 
> Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 6a53338..ba56b74 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -131,9 +131,11 @@
>  #define SMBHSTSTS_INTR		0x02
>  #define SMBHSTSTS_HOST_BUSY	0x01
>  
> -#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_FAILED | \
> -				 SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \
> -				 SMBHSTSTS_INTR)
> +#define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
> +				 SMBHSTSTS_DEV_ERR)
> +
> +#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
> +				 STATUS_ERROR_FLAGS)
>  
>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>  #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
> @@ -385,7 +387,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		do {
>  			usleep_range(250, 500);
>  			status = inb_p(SMBHSTSTS(priv));
> -		} while ((!(status & SMBHSTSTS_BYTE_DONE))
> +		} while (!(status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS))
>  			 && (timeout++ < MAX_RETRIES));
>  
>  		result = i801_check_post(priv, status, timeout > MAX_RETRIES);


-- 
Jean Delvare

Re: [PATCH 5/8 v3] i2c: i801: rename some SMBHSTCNT bit constants

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:12 +0800, Daniel Kurtz wrote:
> Rename the SMBHSTCNT register bit access constants to match the style of
> other register bits.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ba56b74..c57bb0c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -102,9 +102,6 @@
>  #define SMBAUXCTL_CRC		1
>  #define SMBAUXCTL_E32B		2
>  
> -/* kill bit for SMBHSTCNT */
> -#define SMBHSTCNT_KILL		2
> -
>  /* Other settings */
>  #define MAX_RETRIES		400
>  #define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
> @@ -117,9 +114,13 @@
>  #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
> -#define I801_START		0x40
> -#define I801_PEC_EN		0x80	/* ICH3 and later */
> +
> +/* I801 Host Control register bits */
> +#define SMBHSTCNT_INTREN	0x01
> +#define SMBHSTCNT_KILL		0x02
> +#define SMBHSTCNT_LAST_BYTE	0x20
> +#define SMBHSTCNT_START		0x40
> +#define SMBHSTCNT_PEC_EN	0x80	/* ICH3 and later */
>  
>  /* I801 Hosts Status register bits */
>  #define SMBHSTSTS_BYTE_DONE	0x80
> @@ -289,7 +290,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>  	 * INTREN, SMBSCMD are passed in xact */
> -	outb_p(xact | I801_START, SMBHSTCNT(priv));
> +	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>  
>  	/* We will always wait for a fraction of a second! */
>  	do {
> @@ -324,7 +325,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);
> +				  SMBHSTCNT_PEC_EN * hwpec);

Would be the right time to change this to a saner construct as your
original patch set was doing.

>  	if (status)
>  		return status;
>  
> @@ -375,11 +376,11 @@ 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;
> +			smbcmd |= SMBHSTCNT_LAST_BYTE;
>  		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
>  
>  		if (i == 1)
> -			outb_p(inb(SMBHSTCNT(priv)) | I801_START,
> +			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
>  			       SMBHSTCNT(priv));
>  
>  		/* We will always wait for a fraction of a second! */

Other than this, looks good, applied.

-- 
Jean Delvare

Re: [PATCH 5/8 v3] i2c: i801: rename some SMBHSTCNT bit constants

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:12 +0800, Daniel Kurtz wrote:
> Rename the SMBHSTCNT register bit access constants to match the style of
> other register bits.
> 
> Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ba56b74..c57bb0c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -102,9 +102,6 @@
>  #define SMBAUXCTL_CRC		1
>  #define SMBAUXCTL_E32B		2
>  
> -/* kill bit for SMBHSTCNT */
> -#define SMBHSTCNT_KILL		2
> -
>  /* Other settings */
>  #define MAX_RETRIES		400
>  #define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
> @@ -117,9 +114,13 @@
>  #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
> -#define I801_START		0x40
> -#define I801_PEC_EN		0x80	/* ICH3 and later */
> +
> +/* I801 Host Control register bits */
> +#define SMBHSTCNT_INTREN	0x01
> +#define SMBHSTCNT_KILL		0x02
> +#define SMBHSTCNT_LAST_BYTE	0x20
> +#define SMBHSTCNT_START		0x40
> +#define SMBHSTCNT_PEC_EN	0x80	/* ICH3 and later */
>  
>  /* I801 Hosts Status register bits */
>  #define SMBHSTSTS_BYTE_DONE	0x80
> @@ -289,7 +290,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>  	 * INTREN, SMBSCMD are passed in xact */
> -	outb_p(xact | I801_START, SMBHSTCNT(priv));
> +	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>  
>  	/* We will always wait for a fraction of a second! */
>  	do {
> @@ -324,7 +325,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);
> +				  SMBHSTCNT_PEC_EN * hwpec);

Would be the right time to change this to a saner construct as your
original patch set was doing.

>  	if (status)
>  		return status;
>  
> @@ -375,11 +376,11 @@ 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;
> +			smbcmd |= SMBHSTCNT_LAST_BYTE;
>  		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
>  
>  		if (i == 1)
> -			outb_p(inb(SMBHSTCNT(priv)) | I801_START,
> +			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
>  			       SMBHSTCNT(priv));
>  
>  		/* We will always wait for a fraction of a second! */

Other than this, looks good, applied.

-- 
Jean Delvare

Re: [PATCH 4/8 v3] i2c: i801: check and return errors during byte-by-byte transfers

[Daniel Kurtz]

On Thu, Jun 28, 2012 at 12:51 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Wed, 27 Jun 2012 21:54:11 +0800, Daniel Kurtz wrote:
>> If an error is detected in the polling loop, abort the transaction and
>> return an error code.
>>
>>  * DEV_ERR is set if the device does not respond with an acknowledge, and
>> the SMBus controller times out (minimum 25ms).
>>  * BUS_ERR is set if a bus arbitration collision is detected.  In other
>> words, when the SMBus controller tries to generate a START condition, but
>> detects that the SMBDATA is being held low, usually by another SMBus/I2C
>> master.
>>  * FAILED is only set if a the transaction is set by software (using the
>> SMBHSTCNT KILL bit).

That was supposed to say:

 * FAILED is only set if a transaction is stopped by software (using
the SMBHSTCNT KILL bit).

> Not quite sure what you mean with "set by software". Other than this,
> patch looks good, applied, thanks.

Applied where?
I'd like to send a rebased patchset onto whatever you have already staged.

>
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/i2c/busses/i2c-i801.c |   10 ++++++----
>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 6a53338..ba56b74 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -131,9 +131,11 @@
>>  #define SMBHSTSTS_INTR               0x02
>>  #define SMBHSTSTS_HOST_BUSY  0x01
>>
>> -#define STATUS_FLAGS         (SMBHSTSTS_BYTE_DONE | SMBHSTSTS_FAILED | \
>> -                              SMBHSTSTS_BUS_ERR | SMBHSTSTS_DEV_ERR | \
>> -                              SMBHSTSTS_INTR)
>> +#define STATUS_ERROR_FLAGS   (SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
>> +                              SMBHSTSTS_DEV_ERR)
>> +
>> +#define STATUS_FLAGS         (SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
>> +                              STATUS_ERROR_FLAGS)
>>
>>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>>  #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS        0x1c22
>> @@ -385,7 +387,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>               do {
>>                       usleep_range(250, 500);
>>                       status = inb_p(SMBHSTSTS(priv));
>> -             } while ((!(status & SMBHSTSTS_BYTE_DONE))
>> +             } while (!(status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS))
>>                        && (timeout++ < MAX_RETRIES));
>>
>>               result = i801_check_post(priv, status, timeout > MAX_RETRIES);
>
>
> --
> Jean Delvare

Re: [PATCH 6/8 v3] i2c: i801: drop ENABLE_INT9

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:13 +0800, Daniel Kurtz wrote:
> Later patches enable interrupts.  This preliminary patch removes the older
> unsupported ENABLE_INT9 flag.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> (...)

Applied, thanks.

-- 
Jean Delvare

Re: [PATCH 6/8 v3] i2c: i801: drop ENABLE_INT9

[Jean Delvare]

On Wed, 27 Jun 2012 21:54:13 +0800, Daniel Kurtz wrote:
> Later patches enable interrupts.  This preliminary patch removes the older
> unsupported ENABLE_INT9 flag.
> 
> Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> (...)

Applied, thanks.

-- 
Jean Delvare

Re: [PATCH 4/8 v3] i2c: i801: check and return errors during byte-by-byte transfers

[Jean Delvare]

On Thu, 28 Jun 2012 11:46:28 +0800, Daniel Kurtz wrote:
> On Thu, Jun 28, 2012 at 12:51 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Wed, 27 Jun 2012 21:54:11 +0800, Daniel Kurtz wrote:
> >> If an error is detected in the polling loop, abort the transaction and
> >> return an error code.
> >>
> >>  * DEV_ERR is set if the device does not respond with an acknowledge, and
> >> the SMBus controller times out (minimum 25ms).
> >>  * BUS_ERR is set if a bus arbitration collision is detected.  In other
> >> words, when the SMBus controller tries to generate a START condition, but
> >> detects that the SMBDATA is being held low, usually by another SMBus/I2C
> >> master.
> >>  * FAILED is only set if a the transaction is set by software (using the
> >> SMBHSTCNT KILL bit).
> 
> That was supposed to say:
> 
>  * FAILED is only set if a transaction is stopped by software (using
> the SMBHSTCNT KILL bit).

Changed, thanks!

> > Not quite sure what you mean with "set by software". Other than this,
> > patch looks good, applied, thanks.
> 
> Applied where?
> I'd like to send a rebased patchset onto whatever you have already staged.

http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/

-- 
Jean Delvare

Re: [PATCH 4/8 v3] i2c: i801: check and return errors during byte-by-byte transfers

[Jean Delvare]

On Thu, 28 Jun 2012 11:46:28 +0800, Daniel Kurtz wrote:
> On Thu, Jun 28, 2012 at 12:51 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Wed, 27 Jun 2012 21:54:11 +0800, Daniel Kurtz wrote:
> >> If an error is detected in the polling loop, abort the transaction and
> >> return an error code.
> >>
> >>  * DEV_ERR is set if the device does not respond with an acknowledge, and
> >> the SMBus controller times out (minimum 25ms).
> >>  * BUS_ERR is set if a bus arbitration collision is detected.  In other
> >> words, when the SMBus controller tries to generate a START condition, but
> >> detects that the SMBDATA is being held low, usually by another SMBus/I2C
> >> master.
> >>  * FAILED is only set if a the transaction is set by software (using the
> >> SMBHSTCNT KILL bit).
> 
> That was supposed to say:
> 
>  * FAILED is only set if a transaction is stopped by software (using
> the SMBHSTCNT KILL bit).

Changed, thanks!

> > Not quite sure what you mean with "set by software". Other than this,
> > patch looks good, applied, thanks.
> 
> Applied where?
> I'd like to send a rebased patchset onto whatever you have already staged.

http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Daniel Kurtz]

On Thu, Jun 28, 2012 at 12:07 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
>> Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
>> INTR should be checked & cleared separately, only after BYTE_DONE is
>> first cleared:
>>
>>   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.
>>
>> [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
>>
>> Currently, the INTR bit was only checked & cleared separately if the PEC
>> was used.
>> This patch checks and clears INTR at the very end of every successful
>> transaction, regardless of whether the PEC is used.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
>>  1 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 8b74e1e..6a53338 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>>       return result;
>>  }
>>
>> +/* wait for INTR bit as advised by Intel */
>> +static void i801_wait_intr(struct i801_priv *priv)
>> +{
>> +     int timeout = 0;
>> +     int status;
>> +
>> +     status = inb_p(SMBHSTSTS(priv));
>> +     while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
>> +             usleep_range(250, 500);
>> +             status = inb_p(SMBHSTSTS(priv));
>> +     }
>
> Per my comment on previous patch, I've swapped the logic here to be in
> line with what we had before. I have no objection to trying this change
> again, but later, and only if you have actual numbers to back it up.
>
>> +
>> +     if (timeout > MAX_RETRIES)
>> +             dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
>> +
>> +     outb_p(status, SMBHSTSTS(priv));
>
> Wouldn't it be more correct to only write the INTR bit? Writing back
> the whole 8 bits frightens me a little especially because of bit
> INUSE_STS. If we ever want to support this feature, I think we have to
> first ban writing back the whole status to register HST_STS. And this
> is the only place where we still do AFAICS.

It looks like the current code does it this way to clear any error
bits that may be set in the timeout case (errors set, but no INTR).
For example, if there was a bus / arbitration error while waiting for
PEC.

Perhaps we can split the difference (I tested this and it has no
obvious regression):

+     /* Clear INTR, or in case of timeout, any other status bits. */
+     outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));

But in a separate patch...

>
> (This isn't a regression from your patch, the old code was already
> doing that, but it might be the opportunity to fix it.)
>
> --
> Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Thu, 28 Jun 2012 15:51:34 +0800, Daniel Kurtz wrote:
> On Thu, Jun 28, 2012 at 12:07 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
> >> +
> >> +     if (timeout > MAX_RETRIES)
> >> +             dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> >> +
> >> +     outb_p(status, SMBHSTSTS(priv));
> >
> > Wouldn't it be more correct to only write the INTR bit? Writing back
> > the whole 8 bits frightens me a little especially because of bit
> > INUSE_STS. If we ever want to support this feature, I think we have to
> > first ban writing back the whole status to register HST_STS. And this
> > is the only place where we still do AFAICS.
> 
> It looks like the current code does it this way to clear any error
> bits that may be set in the timeout case (errors set, but no INTR).
> For example, if there was a bus / arbitration error while waiting for
> PEC.
> 
> Perhaps we can split the difference (I tested this and it has no
> obvious regression):
> 
> +     /* Clear INTR, or in case of timeout, any other status bits. */
> +     outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> 
> But in a separate patch...
> 

I agree, this would be a reasonable compromise.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Thu, 28 Jun 2012 15:51:34 +0800, Daniel Kurtz wrote:
> On Thu, Jun 28, 2012 at 12:07 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
> >> +
> >> +     if (timeout > MAX_RETRIES)
> >> +             dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> >> +
> >> +     outb_p(status, SMBHSTSTS(priv));
> >
> > Wouldn't it be more correct to only write the INTR bit? Writing back
> > the whole 8 bits frightens me a little especially because of bit
> > INUSE_STS. If we ever want to support this feature, I think we have to
> > first ban writing back the whole status to register HST_STS. And this
> > is the only place where we still do AFAICS.
> 
> It looks like the current code does it this way to clear any error
> bits that may be set in the timeout case (errors set, but no INTR).
> For example, if there was a bus / arbitration error while waiting for
> PEC.
> 
> Perhaps we can split the difference (I tested this and it has no
> obvious regression):
> 
> +     /* Clear INTR, or in case of timeout, any other status bits. */
> +     outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> 
> But in a separate patch...
> 

I agree, this would be a reasonable compromise.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

Hi again Daniel,

On Wed, 27 Jun 2012 18:07:24 +0200, Jean Delvare wrote:
> On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
> > Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
> > INTR should be checked & cleared separately, only after BYTE_DONE is
> > first cleared:
> > 
> >   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.
> > 
> > [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
> > 
> > Currently, the INTR bit was only checked & cleared separately if the PEC
> > was used.
> > This patch checks and clears INTR at the very end of every successful
> > transaction, regardless of whether the PEC is used.
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > ---
> >  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
> >  1 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index 8b74e1e..6a53338 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
> >  	return result;
> >  }
> >  
> > +/* wait for INTR bit as advised by Intel */
> > +static void i801_wait_intr(struct i801_priv *priv)
> > +{
> > +	int timeout = 0;
> > +	int status;
> > +
> > +	status = inb_p(SMBHSTSTS(priv));
> > +	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
> > +		usleep_range(250, 500);
> > +		status = inb_p(SMBHSTSTS(priv));
> > +	}
> 
> Per my comment on previous patch, I've swapped the logic here to be in
> line with what we had before. I have no objection to trying this change
> again, but later, and only if you have actual numbers to back it up.

I've done some performance measurements, and it turns out that, with the
version of this patch modified by me, all short transactions are twice
as slow as before. This is because i801_transaction waits twice now:
once for BUSY to be clear, and then again once for INTR to be set.

I don't think this makes much sense, as both events normally happen at
the same time. As a matter of fact, the original code did clear INTR at
the end of i801_transaction without checking that it was set. Also, we
call i801_check_post() _before_ i801_wait_intr(), which makes no sense
IMHO. If INTR was really not set yet, then checking for errors was
wrong, it was too early.

I'm not even sure why we rely on the BUSY bit in the first place. It
would seem easier to just wait for INTR.

Thinking about it some more, the idea of function i801_wait_intr() is a
little strange. Normally, you'd wait for INTR, then do what you have
to, and lastly clear the INTR bit. Having a function which waits for
INTR and clears it immediately seems wrong.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

Hi again Daniel,

On Wed, 27 Jun 2012 18:07:24 +0200, Jean Delvare wrote:
> On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
> > Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
> > INTR should be checked & cleared separately, only after BYTE_DONE is
> > first cleared:
> > 
> >   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.
> > 
> > [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
> > 
> > Currently, the INTR bit was only checked & cleared separately if the PEC
> > was used.
> > This patch checks and clears INTR at the very end of every successful
> > transaction, regardless of whether the PEC is used.
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
> >  1 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index 8b74e1e..6a53338 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
> >  	return result;
> >  }
> >  
> > +/* wait for INTR bit as advised by Intel */
> > +static void i801_wait_intr(struct i801_priv *priv)
> > +{
> > +	int timeout = 0;
> > +	int status;
> > +
> > +	status = inb_p(SMBHSTSTS(priv));
> > +	while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
> > +		usleep_range(250, 500);
> > +		status = inb_p(SMBHSTSTS(priv));
> > +	}
> 
> Per my comment on previous patch, I've swapped the logic here to be in
> line with what we had before. I have no objection to trying this change
> again, but later, and only if you have actual numbers to back it up.

I've done some performance measurements, and it turns out that, with the
version of this patch modified by me, all short transactions are twice
as slow as before. This is because i801_transaction waits twice now:
once for BUSY to be clear, and then again once for INTR to be set.

I don't think this makes much sense, as both events normally happen at
the same time. As a matter of fact, the original code did clear INTR at
the end of i801_transaction without checking that it was set. Also, we
call i801_check_post() _before_ i801_wait_intr(), which makes no sense
IMHO. If INTR was really not set yet, then checking for errors was
wrong, it was too early.

I'm not even sure why we rely on the BUSY bit in the first place. It
would seem easier to just wait for INTR.

Thinking about it some more, the idea of function i801_wait_intr() is a
little strange. Normally, you'd wait for INTR, then do what you have
to, and lastly clear the INTR bit. Having a function which waits for
INTR and clears it immediately seems wrong.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Daniel Kurtz]

On Mon, Jul 2, 2012 at 5:20 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi again Daniel,
>
> On Wed, 27 Jun 2012 18:07:24 +0200, Jean Delvare wrote:
>> On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
>> > Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
>> > INTR should be checked & cleared separately, only after BYTE_DONE is
>> > first cleared:
>> >
>> >   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.
>> >
>> > [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
>> >
>> > Currently, the INTR bit was only checked & cleared separately if the PEC
>> > was used.
>> > This patch checks and clears INTR at the very end of every successful
>> > transaction, regardless of whether the PEC is used.
>> >
>> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> > ---
>> >  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
>> >  1 files changed, 23 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> > index 8b74e1e..6a53338 100644
>> > --- a/drivers/i2c/busses/i2c-i801.c
>> > +++ b/drivers/i2c/busses/i2c-i801.c
>> > @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>> >     return result;
>> >  }
>> >
>> > +/* wait for INTR bit as advised by Intel */
>> > +static void i801_wait_intr(struct i801_priv *priv)
>> > +{
>> > +   int timeout = 0;
>> > +   int status;
>> > +
>> > +   status = inb_p(SMBHSTSTS(priv));
>> > +   while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
>> > +           usleep_range(250, 500);
>> > +           status = inb_p(SMBHSTSTS(priv));
>> > +   }
>>
>> Per my comment on previous patch, I've swapped the logic here to be in
>> line with what we had before. I have no objection to trying this change
>> again, but later, and only if you have actual numbers to back it up.
>
> I've done some performance measurements, and it turns out that, with the
> version of this patch modified by me, all short transactions are twice
> as slow as before. This is because i801_transaction waits twice now:
> once for BUSY to be clear, and then again once for INTR to be set.

Does a fast sequence of such transactions actually take any longer? Or
just a single short transaction?

My understanding is that the INTR wait is really waiting for the
entire transaction to complete (ie., including i2c STOP condition),
not just the byte transfer phase.

By waiting here at the end of a transaction, we make sure the status
bits are actually clear before starting the next transaction.

-Dan

> I don't think this makes much sense, as both events normally happen at
> the same time. As a matter of fact, the original code did clear INTR at
> the end of i801_transaction without checking that it was set. Also, we
> call i801_check_post() _before_ i801_wait_intr(), which makes no sense
> IMHO. If INTR was really not set yet, then checking for errors was
> wrong, it was too early.
>
> I'm not even sure why we rely on the BUSY bit in the first place. It
> would seem easier to just wait for INTR.
>
> Thinking about it some more, the idea of function i801_wait_intr() is a
> little strange. Normally, you'd wait for INTR, then do what you have
> to, and lastly clear the INTR bit. Having a function which waits for
> INTR and clears it immediately seems wrong.
>
> --
> Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Daniel Kurtz]

On Mon, Jul 2, 2012 at 5:20 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi again Daniel,
>
> On Wed, 27 Jun 2012 18:07:24 +0200, Jean Delvare wrote:
>> On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
>> > Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
>> > INTR should be checked & cleared separately, only after BYTE_DONE is
>> > first cleared:
>> >
>> >   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.
>> >
>> > [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
>> >
>> > Currently, the INTR bit was only checked & cleared separately if the PEC
>> > was used.
>> > This patch checks and clears INTR at the very end of every successful
>> > transaction, regardless of whether the PEC is used.
>> >
>> > Signed-off-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> > ---
>> >  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
>> >  1 files changed, 23 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> > index 8b74e1e..6a53338 100644
>> > --- a/drivers/i2c/busses/i2c-i801.c
>> > +++ b/drivers/i2c/busses/i2c-i801.c
>> > @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>> >     return result;
>> >  }
>> >
>> > +/* wait for INTR bit as advised by Intel */
>> > +static void i801_wait_intr(struct i801_priv *priv)
>> > +{
>> > +   int timeout = 0;
>> > +   int status;
>> > +
>> > +   status = inb_p(SMBHSTSTS(priv));
>> > +   while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
>> > +           usleep_range(250, 500);
>> > +           status = inb_p(SMBHSTSTS(priv));
>> > +   }
>>
>> Per my comment on previous patch, I've swapped the logic here to be in
>> line with what we had before. I have no objection to trying this change
>> again, but later, and only if you have actual numbers to back it up.
>
> I've done some performance measurements, and it turns out that, with the
> version of this patch modified by me, all short transactions are twice
> as slow as before. This is because i801_transaction waits twice now:
> once for BUSY to be clear, and then again once for INTR to be set.

Does a fast sequence of such transactions actually take any longer? Or
just a single short transaction?

My understanding is that the INTR wait is really waiting for the
entire transaction to complete (ie., including i2c STOP condition),
not just the byte transfer phase.

By waiting here at the end of a transaction, we make sure the status
bits are actually clear before starting the next transaction.

-Dan

> I don't think this makes much sense, as both events normally happen at
> the same time. As a matter of fact, the original code did clear INTR at
> the end of i801_transaction without checking that it was set. Also, we
> call i801_check_post() _before_ i801_wait_intr(), which makes no sense
> IMHO. If INTR was really not set yet, then checking for errors was
> wrong, it was too early.
>
> I'm not even sure why we rely on the BUSY bit in the first place. It
> would seem easier to just wait for INTR.
>
> Thinking about it some more, the idea of function i801_wait_intr() is a
> little strange. Normally, you'd wait for INTR, then do what you have
> to, and lastly clear the INTR bit. Having a function which waits for
> INTR and clears it immediately seems wrong.
>
> --
> Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Mon, 2 Jul 2012 09:19:24 +0800, Daniel Kurtz wrote:
> On Mon, Jul 2, 2012 at 5:20 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > I've done some performance measurements, and it turns out that, with the
> > version of this patch modified by me, all short transactions are twice
> > as slow as before. This is because i801_transaction waits twice now:
> > once for BUSY to be clear, and then again once for INTR to be set.
> 
> Does a fast sequence of such transactions actually take any longer? Or
> just a single short transaction?

Both are affected, this is the problem. See:

Original driver:

# time i2cdump -y 8 0x2f b >/dev/null

real	0m0.157s
user	0m0.000s
sys	0m0.010s

After this patch:

# time i2cdump -y 8 0x2f b >/dev/null

real	0m0.279s
user	0m0.003s
sys	0m0.011s

This is on a fast machine with recent kernel. On my ICH3-M laptop with
kernel 2.6.32, the slowdown for a full register dump is from 2 seconds
to 4 seconds. Big performance regression.

> My understanding is that the INTR wait is really waiting for the
> entire transaction to complete (ie., including i2c STOP condition),
> not just the byte transfer phase.

This is my understanding as well, but I'm fairly certain that this is
the case of the BUSY flag as well. I think BUSY gets cleared at the
same time INTR (or any of the error status bits) gets set. Which is why
I think checking BUSY is redundant. As a matter of fact, we ignore BUSY
completely in i801_block_transaction_byte_by_byte(), so I see no reason
why we couldn't do the same in i2c_transaction().

> By waiting here at the end of a transaction, we make sure the status
> bits are actually clear before starting the next transaction.

I have no objection to clearing the status bits, simply I think the
sequence is wrong. I'll write and post a RFC patch later today
illustrating what I think should be done.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Mon, 2 Jul 2012 09:19:24 +0800, Daniel Kurtz wrote:
> On Mon, Jul 2, 2012 at 5:20 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > I've done some performance measurements, and it turns out that, with the
> > version of this patch modified by me, all short transactions are twice
> > as slow as before. This is because i801_transaction waits twice now:
> > once for BUSY to be clear, and then again once for INTR to be set.
> 
> Does a fast sequence of such transactions actually take any longer? Or
> just a single short transaction?

Both are affected, this is the problem. See:

Original driver:

# time i2cdump -y 8 0x2f b >/dev/null

real	0m0.157s
user	0m0.000s
sys	0m0.010s

After this patch:

# time i2cdump -y 8 0x2f b >/dev/null

real	0m0.279s
user	0m0.003s
sys	0m0.011s

This is on a fast machine with recent kernel. On my ICH3-M laptop with
kernel 2.6.32, the slowdown for a full register dump is from 2 seconds
to 4 seconds. Big performance regression.

> My understanding is that the INTR wait is really waiting for the
> entire transaction to complete (ie., including i2c STOP condition),
> not just the byte transfer phase.

This is my understanding as well, but I'm fairly certain that this is
the case of the BUSY flag as well. I think BUSY gets cleared at the
same time INTR (or any of the error status bits) gets set. Which is why
I think checking BUSY is redundant. As a matter of fact, we ignore BUSY
completely in i801_block_transaction_byte_by_byte(), so I see no reason
why we couldn't do the same in i2c_transaction().

> By waiting here at the end of a transaction, we make sure the status
> bits are actually clear before starting the next transaction.

I have no objection to clearing the status bits, simply I think the
sequence is wrong. I'll write and post a RFC patch later today
illustrating what I think should be done.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Mon, 2 Jul 2012 12:08:14 +0200, Jean Delvare wrote:
> On Mon, 2 Jul 2012 09:19:24 +0800, Daniel Kurtz wrote:
> > My understanding is that the INTR wait is really waiting for the
> > entire transaction to complete (ie., including i2c STOP condition),
> > not just the byte transfer phase.
> 
> This is my understanding as well, but I'm fairly certain that this is
> the case of the BUSY flag as well. I think BUSY gets cleared at the
> same time INTR (or any of the error status bits) gets set. Which is why
> I think checking BUSY is redundant. As a matter of fact, we ignore BUSY
> completely in i801_block_transaction_byte_by_byte(), so I see no reason
> why we couldn't do the same in i2c_transaction().

To be complete, I made some testing and error bits can be set before
BUSY is cleared. I spotted several transitions 0x41 -> 0x45 -> 0x44
when accessing non-existent devices. On success, I never witnessed INTR
and BUSY being set at the same time, transition is always 0x41 -> 0x42.

-- 
Jean Delvare

Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

[Jean Delvare]

On Mon, 2 Jul 2012 12:08:14 +0200, Jean Delvare wrote:
> On Mon, 2 Jul 2012 09:19:24 +0800, Daniel Kurtz wrote:
> > My understanding is that the INTR wait is really waiting for the
> > entire transaction to complete (ie., including i2c STOP condition),
> > not just the byte transfer phase.
> 
> This is my understanding as well, but I'm fairly certain that this is
> the case of the BUSY flag as well. I think BUSY gets cleared at the
> same time INTR (or any of the error status bits) gets set. Which is why
> I think checking BUSY is redundant. As a matter of fact, we ignore BUSY
> completely in i801_block_transaction_byte_by_byte(), so I see no reason
> why we couldn't do the same in i2c_transaction().

To be complete, I made some testing and error bits can be set before
BUSY is cleared. I spotted several transitions 0x41 -> 0x45 -> 0x44
when accessing non-existent devices. On success, I never witnessed INTR
and BUSY being set at the same time, transition is always 0x41 -> 0x42.

-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

Hi Daniel,

On Wed, 27 Jun 2012 21:54:14 +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.
> 
> 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 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 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@chromium.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |   93 ++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 6fa2a0b..6bfedc0 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -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)
> @@ -134,8 +137,9 @@
>  #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
>  				 SMBHSTSTS_DEV_ERR)
>  
> -#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
> -				 STATUS_ERROR_FLAGS)
> +#define STATUS_RESULT_FLAGS	(SMBHSTSTS_INTR | STATUS_ERROR_FLAGS)

I see no good reason to introduce this, you use it only once and
STATUS_FLAGS would work as well there.

> +
> +#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | STATUS_RESULT_FLAGS)
>  
>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>  #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
> @@ -155,6 +159,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 +171,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 +180,7 @@ static const char *i801_feature_names[] = {
>  	"Block buffer",
>  	"Block process call",
>  	"I2C block read",
> +	"Interrupt",
>  };
>  
>  static unsigned int disable_features;
> @@ -211,7 +221,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 */
> @@ -287,6 +302,14 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  	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, 0);
> +	}
> +
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>  	 * SMBSCMD are passed in xact */
>  	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
> @@ -341,6 +364,37 @@ 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 hststs;

It's named "status" everywhere else, now that the confusion with the
PCI status register is no longer possible, I would advise that we stick
with "status".

> +
> +	hststs = inb_p(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_p(hststs, SMBHSTSTS(priv));
> +		priv->status |= hststs;
> +		wake_up(&priv->waitq);
> +	}

Your original patch did handle shared interrupts, I don't think this
version does. We are supposed to return IRQ_NONE if the interrupt
wasn't ours, aren't we? Plus, logging the register value when the
interrupt isn't ours fills the kernel log very fast for heavily shared
interrupts (as is the case on my old ICH3-M laptop.)

> +
> +	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
> @@ -801,6 +855,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))
> @@ -848,16 +906,31 @@ static int __devinit i801_probe(struct pci_dev *dev,
>  	}
>  	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;
>  
> @@ -869,14 +942,18 @@ static int __devinit i801_probe(struct pci_dev *dev,
>  	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:
> @@ -891,6 +968,10 @@ static void __devexit i801_remove(struct pci_dev *dev)
>  	i2c_del_adapter(&priv->adapter);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  	pci_release_region(dev, SMBBAR);
> +
> +	if (priv->features & FEATURE_IRQ)
> +		free_irq(dev->irq, priv);
> +

It should make no difference in practice, but I would prefer that we
swamp these, to have the exact same order as in the error path of the
probe function.

>  	pci_set_drvdata(dev, NULL);
>  	kfree(priv);
>  	/*


-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

Hi Daniel,

On Wed, 27 Jun 2012 21:54:14 +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.
> 
> 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 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 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>
> ---
>  drivers/i2c/busses/i2c-i801.c |   93 ++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 6fa2a0b..6bfedc0 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -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)
> @@ -134,8 +137,9 @@
>  #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
>  				 SMBHSTSTS_DEV_ERR)
>  
> -#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
> -				 STATUS_ERROR_FLAGS)
> +#define STATUS_RESULT_FLAGS	(SMBHSTSTS_INTR | STATUS_ERROR_FLAGS)

I see no good reason to introduce this, you use it only once and
STATUS_FLAGS would work as well there.

> +
> +#define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | STATUS_RESULT_FLAGS)
>  
>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>  #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS	0x1c22
> @@ -155,6 +159,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 +171,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 +180,7 @@ static const char *i801_feature_names[] = {
>  	"Block buffer",
>  	"Block process call",
>  	"I2C block read",
> +	"Interrupt",
>  };
>  
>  static unsigned int disable_features;
> @@ -211,7 +221,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 */
> @@ -287,6 +302,14 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  	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, 0);
> +	}
> +
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
>  	 * SMBSCMD are passed in xact */
>  	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
> @@ -341,6 +364,37 @@ 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 hststs;

It's named "status" everywhere else, now that the confusion with the
PCI status register is no longer possible, I would advise that we stick
with "status".

> +
> +	hststs = inb_p(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_p(hststs, SMBHSTSTS(priv));
> +		priv->status |= hststs;
> +		wake_up(&priv->waitq);
> +	}

Your original patch did handle shared interrupts, I don't think this
version does. We are supposed to return IRQ_NONE if the interrupt
wasn't ours, aren't we? Plus, logging the register value when the
interrupt isn't ours fills the kernel log very fast for heavily shared
interrupts (as is the case on my old ICH3-M laptop.)

> +
> +	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
> @@ -801,6 +855,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))
> @@ -848,16 +906,31 @@ static int __devinit i801_probe(struct pci_dev *dev,
>  	}
>  	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;
>  
> @@ -869,14 +942,18 @@ static int __devinit i801_probe(struct pci_dev *dev,
>  	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:
> @@ -891,6 +968,10 @@ static void __devexit i801_remove(struct pci_dev *dev)
>  	i2c_del_adapter(&priv->adapter);
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  	pci_release_region(dev, SMBBAR);
> +
> +	if (priv->features & FEATURE_IRQ)
> +		free_irq(dev->irq, priv);
> +

It should make no difference in practice, but I would prefer that we
swamp these, to have the exact same order as in the error path of the
probe function.

>  	pci_set_drvdata(dev, NULL);
>  	kfree(priv);
>  	/*


-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

Hi again Daniel,

On Wed, 27 Jun 2012 21:54:14 +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.
> 
> 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 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 I2C reads or writes.  The use of the DS
> (BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in
> a subsequent patch.

Hmm, I have hit a bug with this patch. I was testing it on my ICH3-M
laptop, and while SMBus byte and word reads worked fine, SMBus block
reads did fail once in a while.

The ICH3-M lacks the block buffer, so SMBus block reads use the
i801_block_transaction_byte_by_byte function, which at this point does
not use interrupts. However, the IRQ is heavily shared on this laptop:
additionally to SMBus, it is used for USB, sound, network and even the
graphics chip. So function i801_isr() is called all the time.

If i801_isr() is being called while
i801_block_transaction_byte_by_byte() is running, there is a chance
that the status register will have some flags set, in particular INTR.
If so, the code in i801_isr() will clear the flags and wake up the
wait queue while nobody was actually waiting for. And
i801_block_transaction_byte_by_byte() will wait for an event which was
already processed, leading to a timeout.

You should be able to reproduce this bug by loading i2c-i801 with
option disable_features=0x0002, assuming your SMBus IRQ is shared.

This leads me to several thoughts (feel free to correct me if I'm
wrong, I am getting very tired):

1* This must be the reason why a bit was added to the PCI status
   register starting with the ICH5, telling you if an interrupt has been
   delivered to you. Checking for it as you originally did was a good idea
   after all.

2* Is there any reason why you decided to clear the status bits in
   i801_isr(), rather than after wait_event()? I am no expert in
   interrupt handling, I admit, but letting the "caller" clear the status
   bits would ensure we don't clear status bits when nobody is actually
   waiting to catch them.

3* Applying this patch (7/8) without the one adding interrupt support
   to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing
   interrupts and polling isn't safe. So unless we implement 1* or 2*,
   both patches would have to be merged before being pushed upstream.

4* Even then, we have to keep in mind that i801_isr() may be called
   before the transaction is finished (if IRQ is shared.) My testing
   has shown that error flags may be raised before the BUSY flag is
   cleared, so we should use in i801_isr() the same strict conditions
   we are now using in the polling code. In other words, if we get an
   interrupt but the BUSY flag is still set, then it's not our
   interrupt and we should ignore it. This applies even if 2* or 3*
   above are implemented, but no longer if 1* is implemented.

5* Your claim that no locking is needed might have to be revisited when
   interrupts are shared.

Implementing 1* has the drawback of limiting interrupt support to ICH5
and later chips, but I suspect it is the easiest and safest way, so I
have no objection if you want to do that.

-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

Hi again Daniel,

On Wed, 27 Jun 2012 21:54:14 +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.
> 
> 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 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 I2C reads or writes.  The use of the DS
> (BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in
> a subsequent patch.

Hmm, I have hit a bug with this patch. I was testing it on my ICH3-M
laptop, and while SMBus byte and word reads worked fine, SMBus block
reads did fail once in a while.

The ICH3-M lacks the block buffer, so SMBus block reads use the
i801_block_transaction_byte_by_byte function, which at this point does
not use interrupts. However, the IRQ is heavily shared on this laptop:
additionally to SMBus, it is used for USB, sound, network and even the
graphics chip. So function i801_isr() is called all the time.

If i801_isr() is being called while
i801_block_transaction_byte_by_byte() is running, there is a chance
that the status register will have some flags set, in particular INTR.
If so, the code in i801_isr() will clear the flags and wake up the
wait queue while nobody was actually waiting for. And
i801_block_transaction_byte_by_byte() will wait for an event which was
already processed, leading to a timeout.

You should be able to reproduce this bug by loading i2c-i801 with
option disable_features=0x0002, assuming your SMBus IRQ is shared.

This leads me to several thoughts (feel free to correct me if I'm
wrong, I am getting very tired):

1* This must be the reason why a bit was added to the PCI status
   register starting with the ICH5, telling you if an interrupt has been
   delivered to you. Checking for it as you originally did was a good idea
   after all.

2* Is there any reason why you decided to clear the status bits in
   i801_isr(), rather than after wait_event()? I am no expert in
   interrupt handling, I admit, but letting the "caller" clear the status
   bits would ensure we don't clear status bits when nobody is actually
   waiting to catch them.

3* Applying this patch (7/8) without the one adding interrupt support
   to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing
   interrupts and polling isn't safe. So unless we implement 1* or 2*,
   both patches would have to be merged before being pushed upstream.

4* Even then, we have to keep in mind that i801_isr() may be called
   before the transaction is finished (if IRQ is shared.) My testing
   has shown that error flags may be raised before the BUSY flag is
   cleared, so we should use in i801_isr() the same strict conditions
   we are now using in the polling code. In other words, if we get an
   interrupt but the BUSY flag is still set, then it's not our
   interrupt and we should ignore it. This applies even if 2* or 3*
   above are implemented, but no longer if 1* is implemented.

5* Your claim that no locking is needed might have to be revisited when
   interrupts are shared.

Implementing 1* has the drawback of limiting interrupt support to ICH5
and later chips, but I suspect it is the easiest and safest way, so I
have no objection if you want to do that.

-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Daniel Kurtz]

Hi Jean,

On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi again Daniel,
>
> On Wed, 27 Jun 2012 21:54:14 +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.
>>
>> 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 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 I2C reads or writes.  The use of the DS
>> (BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in
>> a subsequent patch.
>
> Hmm, I have hit a bug with this patch. I was testing it on my ICH3-M
> laptop, and while SMBus byte and word reads worked fine, SMBus block
> reads did fail once in a while.
>
> The ICH3-M lacks the block buffer, so SMBus block reads use the
> i801_block_transaction_byte_by_byte function, which at this point does
> not use interrupts. However, the IRQ is heavily shared on this laptop:
> additionally to SMBus, it is used for USB, sound, network and even the
> graphics chip. So function i801_isr() is called all the time.
>
> If i801_isr() is being called while
> i801_block_transaction_byte_by_byte() is running, there is a chance
> that the status register will have some flags set, in particular INTR.
> If so, the code in i801_isr() will clear the flags and wake up the
> wait queue while nobody was actually waiting for. And
> i801_block_transaction_byte_by_byte() will wait for an event which was
> already processed, leading to a timeout.
>
> You should be able to reproduce this bug by loading i2c-i801 with
> option disable_features=0x0002, assuming your SMBus IRQ is shared.
>
> This leads me to several thoughts (feel free to correct me if I'm
> wrong, I am getting very tired):
>
> 1* This must be the reason why a bit was added to the PCI status
>    register starting with the ICH5, telling you if an interrupt has been
>    delivered to you. Checking for it as you originally did was a good idea
>    after all.

Reducing scope to get accepted patches in sooner sounds like a good plan to me.

> 2* Is there any reason why you decided to clear the status bits in
>    i801_isr(), rather than after wait_event()? I am no expert in
>    interrupt handling, I admit, but letting the "caller" clear the status
>    bits would ensure we don't clear status bits when nobody is actually
>    waiting to catch them.

BYTE_DONE is cleared to kick off the next byte, and it doesn't
generate a wait_event, thus it is cleared right in the irq.

The logic for clearing the STATUS_FLAGS was simply that they all
indicate the end of a transaction, so there won't be any further
interrupts.  Thus, it is safe to clear it in the irq, since the irq
will be followed by exactly one wait_event, which can read and process
saved status.

> 3* Applying this patch (7/8) without the one adding interrupt support
>    to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing
>    interrupts and polling isn't safe. So unless we implement 1* or 2*,
>    both patches would have to be merged before being pushed upstream.
>
> 4* Even then, we have to keep in mind that i801_isr() may be called
>    before the transaction is finished (if IRQ is shared.) My testing
>    has shown that error flags may be raised before the BUSY flag is
>    cleared, so we should use in i801_isr() the same strict conditions
>    we are now using in the polling code. In other words, if we get an
>    interrupt but the BUSY flag is still set, then it's not our
>    interrupt and we should ignore it. This applies even if 2* or 3*
>    above are implemented, but no longer if 1* is implemented.
>
> 5* Your claim that no locking is needed might have to be revisited when
>    interrupts are shared.
>
> Implementing 1* has the drawback of limiting interrupt support to ICH5
> and later chips, but I suspect it is the easiest and safest way, so I
> have no objection if you want to do that.

Let's do this first, and then refactor later to add support for
pre-ICH5 parts, if needed.
It sounds like supporting the pre-ICH5 parts w/ shared IRQ is not very
pretty, since we are no longer guaranteed that we get a single
transaction terminating interrupt.

>
> --
> Jean Delvare

Do you still maintain a public git repository?
Have you updated it with the accepted version of the previous cleanup patches?
I see this one, but it doesn't look updated:
  http://git.kernel.org/?p=linux/kernel/git/jdelvare/staging.git;a=summary

I'd like to fix up the interrupt patches per your feedback, but I'm
not quite sure what the current status is of all the cleanup patches.

In other words, if you push up the complete set of cleanup patches, I
can then rebase the new consolidated irq patch onto it, and send them
here for review.

Thanks!
-Dan

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Daniel Kurtz]

Hi Jean,

On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi again Daniel,
>
> On Wed, 27 Jun 2012 21:54:14 +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.
>>
>> 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 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 I2C reads or writes.  The use of the DS
>> (BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in
>> a subsequent patch.
>
> Hmm, I have hit a bug with this patch. I was testing it on my ICH3-M
> laptop, and while SMBus byte and word reads worked fine, SMBus block
> reads did fail once in a while.
>
> The ICH3-M lacks the block buffer, so SMBus block reads use the
> i801_block_transaction_byte_by_byte function, which at this point does
> not use interrupts. However, the IRQ is heavily shared on this laptop:
> additionally to SMBus, it is used for USB, sound, network and even the
> graphics chip. So function i801_isr() is called all the time.
>
> If i801_isr() is being called while
> i801_block_transaction_byte_by_byte() is running, there is a chance
> that the status register will have some flags set, in particular INTR.
> If so, the code in i801_isr() will clear the flags and wake up the
> wait queue while nobody was actually waiting for. And
> i801_block_transaction_byte_by_byte() will wait for an event which was
> already processed, leading to a timeout.
>
> You should be able to reproduce this bug by loading i2c-i801 with
> option disable_features=0x0002, assuming your SMBus IRQ is shared.
>
> This leads me to several thoughts (feel free to correct me if I'm
> wrong, I am getting very tired):
>
> 1* This must be the reason why a bit was added to the PCI status
>    register starting with the ICH5, telling you if an interrupt has been
>    delivered to you. Checking for it as you originally did was a good idea
>    after all.

Reducing scope to get accepted patches in sooner sounds like a good plan to me.

> 2* Is there any reason why you decided to clear the status bits in
>    i801_isr(), rather than after wait_event()? I am no expert in
>    interrupt handling, I admit, but letting the "caller" clear the status
>    bits would ensure we don't clear status bits when nobody is actually
>    waiting to catch them.

BYTE_DONE is cleared to kick off the next byte, and it doesn't
generate a wait_event, thus it is cleared right in the irq.

The logic for clearing the STATUS_FLAGS was simply that they all
indicate the end of a transaction, so there won't be any further
interrupts.  Thus, it is safe to clear it in the irq, since the irq
will be followed by exactly one wait_event, which can read and process
saved status.

> 3* Applying this patch (7/8) without the one adding interrupt support
>    to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing
>    interrupts and polling isn't safe. So unless we implement 1* or 2*,
>    both patches would have to be merged before being pushed upstream.
>
> 4* Even then, we have to keep in mind that i801_isr() may be called
>    before the transaction is finished (if IRQ is shared.) My testing
>    has shown that error flags may be raised before the BUSY flag is
>    cleared, so we should use in i801_isr() the same strict conditions
>    we are now using in the polling code. In other words, if we get an
>    interrupt but the BUSY flag is still set, then it's not our
>    interrupt and we should ignore it. This applies even if 2* or 3*
>    above are implemented, but no longer if 1* is implemented.
>
> 5* Your claim that no locking is needed might have to be revisited when
>    interrupts are shared.
>
> Implementing 1* has the drawback of limiting interrupt support to ICH5
> and later chips, but I suspect it is the easiest and safest way, so I
> have no objection if you want to do that.

Let's do this first, and then refactor later to add support for
pre-ICH5 parts, if needed.
It sounds like supporting the pre-ICH5 parts w/ shared IRQ is not very
pretty, since we are no longer guaranteed that we get a single
transaction terminating interrupt.

>
> --
> Jean Delvare

Do you still maintain a public git repository?
Have you updated it with the accepted version of the previous cleanup patches?
I see this one, but it doesn't look updated:
  http://git.kernel.org/?p=linux/kernel/git/jdelvare/staging.git;a=summary

I'd like to fix up the interrupt patches per your feedback, but I'm
not quite sure what the current status is of all the cleanup patches.

In other words, if you push up the complete set of cleanup patches, I
can then rebase the new consolidated irq patch onto it, and send them
here for review.

Thanks!
-Dan

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

Hi Daniel,

On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote:
> On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > You should be able to reproduce this bug by loading i2c-i801 with
> > option disable_features=0x0002, assuming your SMBus IRQ is shared.
> >
> > This leads me to several thoughts (feel free to correct me if I'm
> > wrong, I am getting very tired):
> >
> > 1* This must be the reason why a bit was added to the PCI status
> >    register starting with the ICH5, telling you if an interrupt has been
> >    delivered to you. Checking for it as you originally did was a good idea
> >    after all.
> 
> Reducing scope to get accepted patches in sooner sounds like a good plan to me.

Agreed.

> > 2* Is there any reason why you decided to clear the status bits in
> >    i801_isr(), rather than after wait_event()? I am no expert in
> >    interrupt handling, I admit, but letting the "caller" clear the status
> >    bits would ensure we don't clear status bits when nobody is actually
> >    waiting to catch them.
> 
> BYTE_DONE is cleared to kick off the next byte, and it doesn't
> generate a wait_event, thus it is cleared right in the irq.

No problem with BYTE_DONE.

> The logic for clearing the STATUS_FLAGS was simply that they all
> indicate the end of a transaction, so there won't be any further
> interrupts.  Thus, it is safe to clear it in the irq, since the irq
> will be followed by exactly one wait_event, which can read and process
> saved status.

It is safe if and only if someone is actually listening to the event.
This was not the case for me yesterday. Even if we don't mix interrupts
and polling, i801_isr() might still get called when we aren't
listening. Not only because the IRQ may be shared, but also for events
we don't yet handle, such as an SMBus Alert. Not sure about slave
mode.

> > 3* Applying this patch (7/8) without the one adding interrupt support
> >    to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing
> >    interrupts and polling isn't safe. So unless we implement 1* or 2*,
> >    both patches would have to be merged before being pushed upstream.
> >
> > 4* Even then, we have to keep in mind that i801_isr() may be called
> >    before the transaction is finished (if IRQ is shared.) My testing
> >    has shown that error flags may be raised before the BUSY flag is
> >    cleared, so we should use in i801_isr() the same strict conditions
> >    we are now using in the polling code. In other words, if we get an
> >    interrupt but the BUSY flag is still set, then it's not our
> >    interrupt and we should ignore it. This applies even if 2* or 3*
> >    above are implemented, but no longer if 1* is implemented.
> >
> > 5* Your claim that no locking is needed might have to be revisited when
> >    interrupts are shared.
> >
> > Implementing 1* has the drawback of limiting interrupt support to ICH5
> > and later chips, but I suspect it is the easiest and safest way, so I
> > have no objection if you want to do that.
> 
> Let's do this first, and then refactor later to add support for
> pre-ICH5 parts, if needed.

OK, fine with me. The only downside is that it excludes my
heavily-shared IRQ test machine, so testing that the shared IRQ case is
properly covered will be a little harder.

> It sounds like supporting the pre-ICH5 parts w/ shared IRQ is not very
> pretty, since we are no longer guaranteed that we get a single
> transaction terminating interrupt.

Indeed. In that case, using interrupts will be much like using polling,
except that status check happens whenever we receive an interrupt,
rather than at fixed time interval.

> Do you still maintain a public git repository?

I never did.

> Have you updated it with the accepted version of the previous cleanup patches?
> I see this one, but it doesn't look updated:
>   http://git.kernel.org/?p=linux/kernel/git/jdelvare/staging.git;a=summary

This tree is only for sending patches to Linus. It doesn't reflect the
current status of my working tree.

I am maintaining quilt series for that, which you can find at:
http://khali.linux-fr.org/devel/linux-3/

> I'd like to fix up the interrupt patches per your feedback, but I'm
> not quite sure what the current status is of all the cleanup patches.
> 
> In other words, if you push up the complete set of cleanup patches, I
> can then rebase the new consolidated irq patch onto it, and send them
> here for review.

I updated the i2c series this morning, so it's up-to-date.

-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

On Thu, 5 Jul 2012 10:10:15 +0200, Jean Delvare wrote:
> On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote:
> > On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > > Implementing 1* has the drawback of limiting interrupt support to ICH5
> > > and later chips, but I suspect it is the easiest and safest way, so I
> > > have no objection if you want to do that.
> > 
> > Let's do this first, and then refactor later to add support for
> > pre-ICH5 parts, if needed.
> 
> OK, fine with me. The only downside is that it excludes my
> heavily-shared IRQ test machine, so testing that the shared IRQ case is
> properly covered will be a little harder.

Actually, no problem there: I can reproduce the issue just fine on my
ICH5 system, which shares an IRQ between the sound chip and the SMBus
controller. So I can use that system to test the updated code too.

-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

On Thu, 5 Jul 2012 10:10:15 +0200, Jean Delvare wrote:
> On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote:
> > On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > > Implementing 1* has the drawback of limiting interrupt support to ICH5
> > > and later chips, but I suspect it is the easiest and safest way, so I
> > > have no objection if you want to do that.
> > 
> > Let's do this first, and then refactor later to add support for
> > pre-ICH5 parts, if needed.
> 
> OK, fine with me. The only downside is that it excludes my
> heavily-shared IRQ test machine, so testing that the shared IRQ case is
> properly covered will be a little harder.

Actually, no problem there: I can reproduce the issue just fine on my
ICH5 system, which shares an IRQ between the sound chip and the SMBus
controller. So I can use that system to test the updated code too.

-- 
Jean Delvare

Re: [PATCH 8/8 v3] i2c: i801: enable irq for byte_by_byte transactions

[Jean Delvare]

Hi Daniel,

On Wed, 27 Jun 2012 21:54:15 +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
> 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.

Two issues remaining:

> +static void i801_isr_byte_done(struct i801_priv *priv)
> +{
> +	/* For SMBus block reads, length is first byte read */
> +	if (priv->is_read && ((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;
> +	} else if (priv->is_read) {

The "else" is wrong. When count == 0, you receive two bytes from the
controller: the block length in SMBHSTDAT0 and the first data byte in
SMBBLKDAT. So the code flow must fall through.

> +		priv->data[priv->count++] = inb(SMBBLKDAT(priv));

This is lacking a boundary check. As I reported in an earlier review,
you shouldn't assume that the hardware will only emit the number of
interrupts you are expecting. If for any reason (crazy hardware or
driver bug) you get more interrupts than expected, you don't want to
overflow priv->data[].

> +		/* Set LAST_BYTE for last byte of read transaction */
> +		if (priv->count == priv->len - 1)
> +			priv->cmd |= SMBHSTCNT_LAST_BYTE;
> +		outb_p(priv->cmd, 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));
> +		outb_p(priv->cmd, SMBHSTCNT(priv));
> +	}
> +
> +	/* Clear BYTE_DONE to start next transaction. */
> +	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> +}

The rest looks OK.

-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Daniel Kurtz]

On Thu, Jul 5, 2012 at 4:10 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Daniel,
>
> On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote:
>> On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> > You should be able to reproduce this bug by loading i2c-i801 with
>> > option disable_features=0x0002, assuming your SMBus IRQ is shared.
>> >
>> > This leads me to several thoughts (feel free to correct me if I'm
>> > wrong, I am getting very tired):
>> >
>> > 1* This must be the reason why a bit was added to the PCI status
>> >    register starting with the ICH5, telling you if an interrupt has been
>> >    delivered to you. Checking for it as you originally did was a good idea
>> >    after all.
>>
>> Reducing scope to get accepted patches in sooner sounds like a good plan to me.
>
> Agreed.
>
>> > 2* Is there any reason why you decided to clear the status bits in
>> >    i801_isr(), rather than after wait_event()? I am no expert in
>> >    interrupt handling, I admit, but letting the "caller" clear the status
>> >    bits would ensure we don't clear status bits when nobody is actually
>> >    waiting to catch them.
>>
>> BYTE_DONE is cleared to kick off the next byte, and it doesn't
>> generate a wait_event, thus it is cleared right in the irq.
>
> No problem with BYTE_DONE.
>
>> The logic for clearing the STATUS_FLAGS was simply that they all
>> indicate the end of a transaction, so there won't be any further
>> interrupts.  Thus, it is safe to clear it in the irq, since the irq
>> will be followed by exactly one wait_event, which can read and process
>> saved status.
>
> It is safe if and only if someone is actually listening to the event.
> This was not the case for me yesterday. Even if we don't mix interrupts
> and polling, i801_isr() might still get called when we aren't
> listening. Not only because the IRQ may be shared, but also for events
> we don't yet handle, such as an SMBus Alert. Not sure about slave
> mode.

The real reason for clearing the flag in the hard irq is that
otherwise, we end up with an infinite irq loop.  The interrupt is
apparently level triggered, and must be cleared before exiting the
ISR.

Unfortunately, I only had time today to discover this, but not time to fix it.
Next week I will be away, so I won't have a chance to provide more
patches for the next 10 days.

I think the 6 patches you have already make a complete set, however,
and shouldn't be waiting for the interrupt patch(es) which can follow
at some future date.

-Daniel

>
>> > 3* Applying this patch (7/8) without the one adding interrupt support
>> >    to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing
>> >    interrupts and polling isn't safe. So unless we implement 1* or 2*,
>> >    both patches would have to be merged before being pushed upstream.
>> >
>> > 4* Even then, we have to keep in mind that i801_isr() may be called
>> >    before the transaction is finished (if IRQ is shared.) My testing
>> >    has shown that error flags may be raised before the BUSY flag is
>> >    cleared, so we should use in i801_isr() the same strict conditions
>> >    we are now using in the polling code. In other words, if we get an
>> >    interrupt but the BUSY flag is still set, then it's not our
>> >    interrupt and we should ignore it. This applies even if 2* or 3*
>> >    above are implemented, but no longer if 1* is implemented.
>> >
>> > 5* Your claim that no locking is needed might have to be revisited when
>> >    interrupts are shared.
>> >
>> > Implementing 1* has the drawback of limiting interrupt support to ICH5
>> > and later chips, but I suspect it is the easiest and safest way, so I
>> > have no objection if you want to do that.
>>
>> Let's do this first, and then refactor later to add support for
>> pre-ICH5 parts, if needed.
>
> OK, fine with me. The only downside is that it excludes my
> heavily-shared IRQ test machine, so testing that the shared IRQ case is
> properly covered will be a little harder.
>
>> It sounds like supporting the pre-ICH5 parts w/ shared IRQ is not very
>> pretty, since we are no longer guaranteed that we get a single
>> transaction terminating interrupt.
>
> Indeed. In that case, using interrupts will be much like using polling,
> except that status check happens whenever we receive an interrupt,
> rather than at fixed time interval.
>
>> Do you still maintain a public git repository?
>
> I never did.
>
>> Have you updated it with the accepted version of the previous cleanup patches?
>> I see this one, but it doesn't look updated:
>>   http://git.kernel.org/?p=linux/kernel/git/jdelvare/staging.git;a=summary
>
> This tree is only for sending patches to Linus. It doesn't reflect the
> current status of my working tree.
>
> I am maintaining quilt series for that, which you can find at:
> http://khali.linux-fr.org/devel/linux-3/
>
>> I'd like to fix up the interrupt patches per your feedback, but I'm
>> not quite sure what the current status is of all the cleanup patches.
>>
>> In other words, if you push up the complete set of cleanup patches, I
>> can then rebase the new consolidated irq patch onto it, and send them
>> here for review.
>
> I updated the i2c series this morning, so it's up-to-date.
>
> --
> Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

Hi Daniel,

On Fri, 6 Jul 2012 18:28:28 +0800, Daniel Kurtz wrote:
> On Thu, Jul 5, 2012 at 4:10 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote:
> >> The logic for clearing the STATUS_FLAGS was simply that they all
> >> indicate the end of a transaction, so there won't be any further
> >> interrupts.  Thus, it is safe to clear it in the irq, since the irq
> >> will be followed by exactly one wait_event, which can read and process
> >> saved status.
> >
> > It is safe if and only if someone is actually listening to the event.
> > This was not the case for me yesterday. Even if we don't mix interrupts
> > and polling, i801_isr() might still get called when we aren't
> > listening. Not only because the IRQ may be shared, but also for events
> > we don't yet handle, such as an SMBus Alert. Not sure about slave
> > mode.
> 
> The real reason for clearing the flag in the hard irq is that
> otherwise, we end up with an infinite irq loop.  The interrupt is
> apparently level triggered, and must be cleared before exiting the
> ISR.

Oh, OK. If we have to do it that way, then we do.

> Unfortunately, I only had time today to discover this, but not time to fix it.
> Next week I will be away, so I won't have a chance to provide more
> patches for the next 10 days.
> 
> I think the 6 patches you have already make a complete set, however,
> and shouldn't be waiting for the interrupt patch(es) which can follow
> at some future date.

Agreed, that's why they are already in linux-next waiting for the next
merge window to open.

That being said, I would hate to miss that merge window for interrupt
support patches, as this is really a great performance improvement. So,
if you have no objection, I propose to fix your patches myself today of
tomorrow, test them, and if I find no problem, post them and push them
to linux-next. You can review and test them when you return.

-- 
Jean Delvare

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

[Jean Delvare]

Hi Daniel,

On Fri, 6 Jul 2012 18:28:28 +0800, Daniel Kurtz wrote:
> On Thu, Jul 5, 2012 at 4:10 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote:
> >> The logic for clearing the STATUS_FLAGS was simply that they all
> >> indicate the end of a transaction, so there won't be any further
> >> interrupts.  Thus, it is safe to clear it in the irq, since the irq
> >> will be followed by exactly one wait_event, which can read and process
> >> saved status.
> >
> > It is safe if and only if someone is actually listening to the event.
> > This was not the case for me yesterday. Even if we don't mix interrupts
> > and polling, i801_isr() might still get called when we aren't
> > listening. Not only because the IRQ may be shared, but also for events
> > we don't yet handle, such as an SMBus Alert. Not sure about slave
> > mode.
> 
> The real reason for clearing the flag in the hard irq is that
> otherwise, we end up with an infinite irq loop.  The interrupt is
> apparently level triggered, and must be cleared before exiting the
> ISR.

Oh, OK. If we have to do it that way, then we do.

> Unfortunately, I only had time today to discover this, but not time to fix it.
> Next week I will be away, so I won't have a chance to provide more
> patches for the next 10 days.
> 
> I think the 6 patches you have already make a complete set, however,
> and shouldn't be waiting for the interrupt patch(es) which can follow
> at some future date.

Agreed, that's why they are already in linux-next waiting for the next
merge window to open.

That being said, I would hate to miss that merge window for interrupt
support patches, as this is really a great performance improvement. So,
if you have no objection, I propose to fix your patches myself today of
tomorrow, test them, and if I find no problem, post them and push them
to linux-next. You can review and test them when you return.

-- 
Jean Delvare

Re: [PATCH 8/8 v3] i2c: i801: enable irq for byte_by_byte transactions

[Jean Delvare]

One more note...

On Wed, 27 Jun 2012 21:54:15 +0800, Daniel Kurtz wrote:
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> (...)
> +static void i801_isr_byte_done(struct i801_priv *priv)
> +{
> +	/* For SMBus block reads, length is first byte read */
> +	if (priv->is_read && ((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;
> +	} else if (priv->is_read) {
> +		priv->data[priv->count++] = inb(SMBBLKDAT(priv));
> +		/* Set LAST_BYTE for last byte of read transaction */
> +		if (priv->count == priv->len - 1)
> +			priv->cmd |= SMBHSTCNT_LAST_BYTE;
> +		outb_p(priv->cmd, SMBHSTCNT(priv));

This register write doesn't seem to be needed, except when
SMBHSTCNT_LAST_BYTE must be added.

> +	} else if (priv->count < priv->len - 1) {
> +		/* Write next byte, except for IRQ after last byte */
> +		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
> +		outb_p(priv->cmd, SMBHSTCNT(priv));

Same here. I know we do write to SMBHSTCNT after for every byte in
i801_block_transaction_byte_by_byte(), that must be the reason why you
did it in the interrupt path too, but I don't know why we did that and
I don't think we actually had to.

-- 
Jean Delvare

Re: [PATCH 8/8 v3] i2c: i801: enable irq for byte_by_byte transactions

[Jean Delvare]

One more note...

On Wed, 27 Jun 2012 21:54:15 +0800, Daniel Kurtz wrote:
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> (...)
> +static void i801_isr_byte_done(struct i801_priv *priv)
> +{
> +	/* For SMBus block reads, length is first byte read */
> +	if (priv->is_read && ((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;
> +	} else if (priv->is_read) {
> +		priv->data[priv->count++] = inb(SMBBLKDAT(priv));
> +		/* Set LAST_BYTE for last byte of read transaction */
> +		if (priv->count == priv->len - 1)
> +			priv->cmd |= SMBHSTCNT_LAST_BYTE;
> +		outb_p(priv->cmd, SMBHSTCNT(priv));

This register write doesn't seem to be needed, except when
SMBHSTCNT_LAST_BYTE must be added.

> +	} else if (priv->count < priv->len - 1) {
> +		/* Write next byte, except for IRQ after last byte */
> +		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
> +		outb_p(priv->cmd, SMBHSTCNT(priv));

Same here. I know we do write to SMBHSTCNT after for every byte in
i801_block_transaction_byte_by_byte(), that must be the reason why you
did it in the interrupt path too, but I don't know why we did that and
I don't think we actually had to.

-- 
Jean Delvare