Hi Jean, On Jul 18 2016 or thereabouts, Jean Delvare wrote: > Hi Benjamin, > > Once again, sorry for the late review. > > On Fri, 24 Jun 2016 16:39:49 +0200, Benjamin Tissoires wrote: > > The i801 chip can handle the Host Notify feature since ICH 3 as mentioned > > in http://www.intel.com/content/dam/doc/datasheet/82801ca-io-controller-hub-3-datasheet.pdf > > > > Enable the functionality unconditionally and propagate the alert > > on each notification. > > > > With a T440s and a Synaptics touchpad that implements Host Notify, the > > payload data is always 0x0000, so I am not sure if the device actually > > sends the payload or if there is a problem regarding the implementation. > > Out of curiosity, does it work on at least one machine? I have tested the T440, t450 and t460 (3 different generation of Intel processor), and none seems to be working. The Synaptics touchpad used is mostly the same, so maybe that's a device issue. > > > Tested-by: Andrew Duggan <aduggan@xxxxxxxxxxxxx> > > Acked-by: Wolfram Sang <wsa@xxxxxxxxxxxxx> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> > > --- > > changes in v2: > > - removed the description of the Slave functionality support in the chip table > > (the table shows what is supported, not what the hardware is capable of) > > - use i2c-smbus to forward the notification > > - remove the fifo, and directly retrieve the address and payload in the worker > > - do not check for Host Notification is the feature is not enabled > > - use inw_p() to read the payload instead of 2 inb_p() calls > > - add /* fall-through */ comment > > - unconditionally enable Host Notify if the hardware supports it (can be > > disabled by the user) > > > > no changes in v3 > > > > changes in v4: > > - make use of the new API -> no more worker spawning here > > - solved a race between the access of the Host Notify registers and the actual > > I2C transfers. > > > > changes in v5: > > - added SKL Host Notify support > > > > changes in v6: > > - select I2C_SMBUS in Kconfig to prevent an undefined reference when I2C_I801 > > is set to 'Y' while I2C_SMBUS is set to 'M' > > > > no changes in v7 > > > > changes in v8: > > - reapplied after http://patchwork.ozlabs.org/patch/632768/ and merged the > > conflict (minor conflict in the struct i801_priv). > > - removed the .resume hook as upstream changed suspend/resume hooks and there > > is no need in the end to re-enable host notify on resume (tested on Lenovo > > t440 and t450). > > - kept Wolfram's Acked-by as the changes were minor > > > > changes in v9: > > - re-add the .resume hook. Looks like the Lenovo T440 sometimes forget to > > re-enable Host Notify on resume, so better have it reset for everybody > > > > drivers/i2c/busses/Kconfig | 1 + > > drivers/i2c/busses/i2c-i801.c | 88 +++++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 86 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index efa3d9b..b4b6cb0 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -91,6 +91,7 @@ config I2C_I801 > > tristate "Intel 82801 (ICH/PCH)" > > depends on PCI > > select CHECK_SIGNATURE if X86 && DMI > > + select I2C_SMBUS > > help > > If you say yes to this option, support will be included for the Intel > > 801 family of mainboard I2C interfaces. Specifically, the following > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > > index b436963..312caed 100644 > > --- a/drivers/i2c/busses/i2c-i801.c > > +++ b/drivers/i2c/busses/i2c-i801.c > > @@ -72,6 +72,7 @@ > > * Block process call transaction no > > * I2C block read transaction yes (doesn't use the block buffer) > > * Slave mode no > > + * SMBus Host Notify yes > > * Interrupt processing yes > > * > > * See the file Documentation/i2c/busses/i2c-i801 for details. > > @@ -86,6 +87,7 @@ > > #include <linux/ioport.h> > > #include <linux/init.h> > > #include <linux/i2c.h> > > +#include <linux/i2c-smbus.h> > > #include <linux/acpi.h> > > #include <linux/io.h> > > #include <linux/dmi.h> > > @@ -113,6 +115,10 @@ > > #define SMBPEC(p) (8 + (p)->smba) /* ICH3 and later */ > > #define SMBAUXSTS(p) (12 + (p)->smba) /* ICH4 and later */ > > #define SMBAUXCTL(p) (13 + (p)->smba) /* ICH4 and later */ > > +#define SMBSLVSTS(p) (16 + (p)->smba) /* ICH3 and later */ > > +#define SMBSLVCMD(p) (17 + (p)->smba) /* ICH3 and later */ > > +#define SMBNTFDADD(p) (20 + (p)->smba) /* ICH3 and later */ > > +#define SMBNTFDDAT(p) (22 + (p)->smba) /* ICH3 and later */ > > > > /* PCI Address Constants */ > > #define SMBBAR 4 > > @@ -177,6 +183,12 @@ > > #define SMBHSTSTS_INTR 0x02 > > #define SMBHSTSTS_HOST_BUSY 0x01 > > > > +/* Host Notify Status registers bits */ > > +#define SMBSLVSTS_HST_NTFY_STS 1 > > + > > +/* Host Notify Command registers bits */ > > +#define SMBSLVCMD_HST_NTFY_INTREN 0x01 > > "register" needs no "s" in these 2 comments. Also it would be nice to > stick to hexadecimal for consistency (I know it's not the case > elsewhere in the driver, but let's not add to it.) > > > + > > #define STATUS_ERROR_FLAGS (SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \ > > SMBHSTSTS_DEV_ERR) > > > > @@ -252,13 +264,17 @@ struct i801_priv { > > */ > > bool acpi_reserved; > > struct mutex acpi_lock; > > + struct smbus_host_notify *host_notify; > > }; > > > > +#define SMBHSTNTFY_SIZE 8 > > This constant isn't used anywhere? > > > + > > #define FEATURE_SMBUS_PEC (1 << 0) > > #define FEATURE_BLOCK_BUFFER (1 << 1) > > #define FEATURE_BLOCK_PROC (1 << 2) > > #define FEATURE_I2C_BLOCK_READ (1 << 3) > > #define FEATURE_IRQ (1 << 4) > > +#define FEATURE_HOST_NOTIFY (1 << 5) > > /* Not really a feature, but it's convenient to handle it as such */ > > #define FEATURE_IDF (1 << 15) > > #define FEATURE_TCO (1 << 16) > > @@ -269,6 +285,7 @@ static const char *i801_feature_names[] = { > > "Block process call", > > "I2C block read", > > "Interrupt", > > + "SMBus Host Notify", > > }; > > > > static unsigned int disable_features; > > @@ -277,7 +294,8 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n" > > "\t\t 0x01 disable SMBus PEC\n" > > "\t\t 0x02 disable the block buffer\n" > > "\t\t 0x08 disable the I2C block read functionality\n" > > - "\t\t 0x10 don't use interrupts "); > > + "\t\t 0x10 don't use interrupts\n" > > + "\t\t 0x20 disable SMBus Host Notify "); > > > > /* Make sure the SMBus host is ready to start transmitting. > > Return 0 if it is, -EBUSY if it is not. */ > > @@ -511,8 +529,23 @@ static void i801_isr_byte_done(struct i801_priv *priv) > > outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv)); > > } > > > > +static irqreturn_t i801_host_notify_isr(struct i801_priv *priv) > > +{ > > + unsigned short addr; > > + unsigned int data; > > + > > + addr = inb_p(SMBNTFDADD(priv)) >> 1; > > + data = inw_p(SMBNTFDDAT(priv)); > > The ICH9 datasheet I'm looking at says the data is in 2 8-bit > registers. I wonder if a 16-bit read is OK an all chipsets. Well, if it > isn't we'll learn about it someday, I suppose. Well, yes, I suppose too :) I am about to send a series to address all of your remarks. Thanks for the review! Cheers, Benjamin > > > + > > + i2c_handle_smbus_host_notify(priv->host_notify, addr, data); > > This can fail, in theory. Maybe this should be handled? > > > + > > + /* clear Host Notify bit and return */ > > + outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv)); > > + return IRQ_HANDLED; > > +} > > + > > /* > > - * There are two kinds of interrupts: > > + * There are three kinds of interrupts: > > * > > * 1) i801 signals transaction completion with one of these interrupts: > > * INTR - Success > > @@ -524,6 +557,8 @@ static void i801_isr_byte_done(struct i801_priv *priv) > > * > > * 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. > > + * > > + * 3) Host Notify interrupts > > */ > > static irqreturn_t i801_isr(int irq, void *dev_id) > > { > > @@ -536,6 +571,12 @@ static irqreturn_t i801_isr(int irq, void *dev_id) > > if (!(pcists & SMBPCISTS_INTS)) > > return IRQ_NONE; > > > > + if (priv->features & FEATURE_HOST_NOTIFY) { > > + status = inb_p(SMBSLVSTS(priv)); > > + if (status & SMBSLVSTS_HST_NTFY_STS) > > + return i801_host_notify_isr(priv); > > + } > > + > > status = inb_p(SMBHSTSTS(priv)); > > if (status & SMBHSTSTS_BYTE_DONE) > > i801_isr_byte_done(priv); > > @@ -847,7 +888,28 @@ static u32 i801_func(struct i2c_adapter *adapter) > > I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK | > > ((priv->features & FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : 0) | > > ((priv->features & FEATURE_I2C_BLOCK_READ) ? > > - I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0); > > + I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0) | > > + ((priv->features & FEATURE_HOST_NOTIFY) ? > > + I2C_FUNC_SMBUS_HOST_NOTIFY : 0); > > +} > > + > > +static int i801_enable_host_notify(struct i2c_adapter *adapter) > > +{ > > + struct i801_priv *priv = i2c_get_adapdata(adapter); > > + > > + if (!(priv->features & FEATURE_HOST_NOTIFY)) > > + return -ENOTSUPP; > > Why do you return an error here? This forces you to special-case it on > further error handling. You could simply return 0 instead, and save > some tests. > > > + > > + if (!priv->host_notify) > > + priv->host_notify = i2c_setup_smbus_host_notify(adapter); > > + if (!priv->host_notify) > > + return -ENOMEM; > > Minor optimization: if priv->host_notify was already set, the second > test is not needed. > > > + > > + outb_p(SMBSLVCMD_HST_NTFY_INTREN, SMBSLVCMD(priv)); > > + /* clear Host Notify bit to allow a new notification */ > > + outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv)); > > + > > + return 0; > > } > > > > static const struct i2c_algorithm smbus_algorithm = { > > @@ -1379,6 +1441,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > > priv->features |= FEATURE_SMBUS_PEC; > > priv->features |= FEATURE_BLOCK_BUFFER; > > priv->features |= FEATURE_TCO; > > + priv->features |= FEATURE_HOST_NOTIFY; > > break; > > > > case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0: > > @@ -1398,6 +1461,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > > priv->features |= FEATURE_BLOCK_BUFFER; > > /* fall through */ > > case PCI_DEVICE_ID_INTEL_82801CA_3: > > + priv->features |= FEATURE_HOST_NOTIFY; > > + /* fall through */ > > case PCI_DEVICE_ID_INTEL_82801BA_2: > > case PCI_DEVICE_ID_INTEL_82801AB_3: > > case PCI_DEVICE_ID_INTEL_82801AA_3: > > @@ -1507,6 +1572,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > > return err; > > } > > > > + /* > > + * Enable Host Notify for chips that supports it. > > + * It is done after i2c_add_adapter() so that we are sure the work queue > > + * is not used if i2c_add_adapter() fails. > > + */ > > + err = i801_enable_host_notify(&priv->adapter); > > + if (err && err != -ENOTSUPP) > > + dev_warn(&dev->dev, "Unable to enable SMBus Host Notify\n"); > > + > > i801_probe_optional_slaves(priv); > > /* We ignore errors - multiplexing is optional */ > > i801_add_mux(priv); > > @@ -1553,6 +1627,14 @@ static int i801_suspend(struct device *dev) > > > > static int i801_resume(struct device *dev) > > { > > + struct pci_dev *pci_dev = to_pci_dev(dev); > > + struct i801_priv *priv = pci_get_drvdata(pci_dev); > > + int err; > > + > > + err = i801_enable_host_notify(&priv->adapter); > > + if (err && err != -ENOTSUPP) > > + dev_warn(dev, "Unable to enable SMBus Host Notify\n"); > > + > > return 0; > > } > > #endif > > > -- > Jean Delvare > SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
