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

[Jean Delvare]

Hi Daniel,

On Fri,  6 Jan 2012 18:58:21 +0800, Daniel Kurtz wrote:
> Add a new 'feature' to i2c-i801 to enable using i801 interrupts.
> When the feature is enabled, then an isr is installed for the device's
> pci irq.
> 
> An i2c/smbus transaction is always terminated by one of the following
> interrupt sources: FAILED, BUS_ERR, DEV_ERR, or on success: INTR

Missing trailing dot.

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

Typo: preparation.

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

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

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

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

> DS (BYTE_DONE) interrupt with byte-by-byte i2c transactions is
> implemented in a subsequent patch.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@xxxxxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-i801.c |  107 ++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 100 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f3418cf..b123133 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -59,10 +59,12 @@
>    Block process call transaction   no
>    I2C block read transaction       yes  (doesn't use the block buffer)
>    Slave mode                       no
> +  Interrupt processing             yes
>  
>    See the file Documentation/i2c/busses/i2c-i801 for details.
>  */
>  
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/kernel.h>
> @@ -90,6 +92,7 @@
>  
>  /* PCI Address Constants */
>  #define SMBBAR		4
> +#define SMBPCISTS	0x006
>  #define SMBHSTCFG	0x040
>  
>  /* Host configuration bits for SMBHSTCFG */
> @@ -97,6 +100,9 @@
>  #define SMBHSTCFG_SMB_SMI_EN	2
>  #define SMBHSTCFG_I2C_EN	4
>  
> +/* Host status bits for SMBHSTSTS */

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

> +#define SMBPCISTS_INTS		8

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

> +
>  /* Auxiliary control register bits, ICH4+ only */
>  #define SMBAUXCTL_CRC		1
>  #define SMBAUXCTL_E32B		2
> @@ -106,7 +112,6 @@
>  
>  /* Other settings */
>  #define MAX_TIMEOUT		100
> -#define ENABLE_INT9		0	/* set to 0x01 to enable - untested */
>  
>  /* I801 command constants */
>  #define I801_QUICK		0x00
> @@ -116,7 +121,11 @@
>  #define I801_PROC_CALL		0x10	/* unimplemented */
>  #define I801_BLOCK_DATA		0x14
>  #define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
> -#define I801_LAST_BYTE		0x20
> +
> +/* I801 Hosts Control register bits */
> +#define I801_INTREN		0x01
> +#define I801_KILL		0x02

This is redundant with SMBHSTCNT_KILL, right?

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

Not true, this bit exists prior to ICH5.

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

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

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

This needs <linux/wait.h>.

> +	spinlock_t lock;

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

> +	u8 status;
>  };
>  
>  static struct pci_driver i801_driver;
> @@ -159,6 +176,7 @@ static struct pci_driver i801_driver;
>  #define FEATURE_BLOCK_BUFFER	(1 << 1)
>  #define FEATURE_BLOCK_PROC	(1 << 2)
>  #define FEATURE_I2C_BLOCK_READ	(1 << 3)
> +#define FEATURE_IRQ		(1 << 4)
>  /* Not really a feature, but it's convenient to handle it as such */
>  #define FEATURE_IDF		(1 << 15)
>  
> @@ -167,6 +185,7 @@ static const char *i801_feature_names[] = {
>  	"Block buffer",
>  	"Block process call",
>  	"I2C block read",
> +	"Interrupt"

Trailing comma please.

>  };
>  
>  static unsigned int disable_features;
> @@ -207,7 +226,12 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>  {
>  	int result = 0;
>  
> -	/* If the SMBus is still busy, we give up */
> +	/*
> +	 * If the SMBus is still busy, we give up
> +	 * Note: This timeout condition only happens when using polling
> +	 * transactions.  For interrupt operation, NAK/timeout is indicated by
> +	 * DEV_ERR.
> +	 */
>  	if (timeout) {
>  		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
>  		/* try to stop the current command */
> @@ -265,8 +289,15 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  	if (result < 0)
>  		return result;
>  
> +	if (priv->features & FEATURE_IRQ) {
> +		outb(xact | I801_INTREN | I801_START, SMBHSTCNT(priv));
> +		wait_event(priv->waitq, (status = priv->status));
> +		priv->status = 0;
> +		return i801_check_post(priv, status, 0);
> +	}
> +
>  	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
> -	 * INTREN, SMBSCMD are passed in xact */
> +	 * SMBSCMD are passed in xact */
>  	outb_p(xact | I801_START, SMBHSTCNT(priv));
>  
>  	/* We will always wait for a fraction of a second! */
> @@ -280,6 +311,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  		return result;
>  
>  	outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
> +
>  	return 0;

Please avoid mixing whitespace changes with real changes.

>  }
>  
> @@ -318,7 +350,7 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  			outb_p(data->block[i+1], SMBBLKDAT(priv));
>  	}
>  
> -	status = i801_transaction(priv, I801_BLOCK_DATA | ENABLE_INT9 |
> +	status = i801_transaction(priv, I801_BLOCK_DATA |

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

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

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

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

This is expected in case of shared interrupts, right?

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

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

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

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

> +		outb(hststs, SMBHSTSTS(priv));
> +		priv->status |= hststs;
> +		wake_up(&priv->waitq);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
>   * i2c write uses cmd=I801_BLOCK_DATA, I2C_EN=1
>   * i2c read uses cmd=I801_I2C_BLOCK_DATA
>   */
> @@ -370,7 +440,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	for (i = 1; i <= len; i++) {
>  		if (i == len && read_write == I2C_SMBUS_READ)
>  			smbcmd |= I801_LAST_BYTE;
> -		outb_p(smbcmd | ENABLE_INT9, SMBHSTCNT(priv));
> +		outb_p(smbcmd, SMBHSTCNT(priv));
>  
>  		if (i == 1)
>  			outb_p(inb(SMBHSTCNT(priv)) | I801_START,
> @@ -571,7 +641,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		ret = i801_block_transaction(priv, data, read_write, size,
>  					     hwpec);
>  	else
> -		ret = i801_transaction(priv, xact | ENABLE_INT9);
> +		ret = i801_transaction(priv, xact);
>  
>  	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
>  	   time, so we forcibly disable it after every transaction. Turn off
> @@ -805,6 +875,10 @@ static int __devinit i801_probe(struct pci_dev *dev,
>  		break;
>  	}
>  
> +	/* IRQ processing only tested on CougarPoint PCH */

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

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

> +	if (dev->device == PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS)
> +		priv->features |= FEATURE_IRQ;
> +
>  	/* Disable features on user request */
>  	for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) {
>  		if (priv->features & disable_features & (1 << i))
> @@ -879,8 +953,24 @@ static int __devinit i801_probe(struct pci_dev *dev,
>  	i801_probe_optional_slaves(priv);
>  
>  	pci_set_drvdata(dev, priv);
> +
> +	if (priv->features & FEATURE_IRQ) {
> +		init_waitqueue_head(&priv->waitq);
> +		spin_lock_init(&priv->lock);
> +
> +		err = request_irq(dev->irq, i801_isr, IRQF_SHARED,
> +				  i801_driver.name, priv);
> +		if (err) {
> +			dev_err(&dev->dev, "Failed to allocate irq %d: %d",

Missing "\n".

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

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

> +	}
>  	return 0;
>  
> +exit_del_adapter:
> +	pci_set_drvdata(dev, NULL);
> +	i2c_del_adapter(&priv->adapter);
>  exit_release:
>  	pci_release_region(dev, SMBBAR);
>  exit:
> @@ -892,6 +982,9 @@ static void __devexit i801_remove(struct pci_dev *dev)
>  {
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
> +	if (priv->features & FEATURE_IRQ)
> +		free_irq(dev->irq, priv);
> +
>  	i2c_del_adapter(&priv->adapter);

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

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


-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/