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
.
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 <[email protected]>
---
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
Rename the SMBHSTCNT register bit access constants to match the style of
other register bits.
Signed-off-by: Daniel Kurtz <[email protected]>
---
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
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 <[email protected]>
---
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
Later patches enable interrupts. This preliminary patch removes the older
unsupported ENABLE_INT9 flag.
Signed-off-by: Daniel Kurtz <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> drivers/i2c/busses/i2c-i801.c | 28 ++++++++++++++--------------
> 1 files changed, 14 insertions(+), 14 deletions(-)
> (...)
Applied, thanks.
--
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
On Thu, Jun 28, 2012 at 12:51 AM, Jean Delvare <[email protected]> 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 <[email protected]>
>> ---
>> ?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
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 <[email protected]>
> ---
> drivers/i2c/busses/i2c-i801.c | 9 ++++-----
> 1 files changed, 4 insertions(+), 5 deletions(-)
> (...)
Applied, thanks.
--
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 <[email protected]> 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
On Thu, Jun 28, 2012 at 12:07 AM, Jean Delvare <[email protected]> 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 <[email protected]>
>> ---
>> ?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
On Thu, 28 Jun 2012 15:51:34 +0800, Daniel Kurtz wrote:
> On Thu, Jun 28, 2012 at 12:07 AM, Jean Delvare <[email protected]> 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