Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
[PATCH 5/5] i2c-i801: fixup regarding watchdog timer
137 views
Skip to first unread message
Matt Fleming
Jul 27, 2015, 9:40:06 AM7/27/15
to
From: Andy Shevchenko <andriy.s...@linux.intel.com>
Change &array[0] to array since it's the same.
It fixes 80 character limit warning as well.
Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Signed-off-by: Matt Fleming <matt.f...@intel.com>
---
drivers/i2c/busses/i2c-i801.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c79dbe116ccc..e41005927746 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1173,7 +1173,8 @@ static void i801_add_tco(struct i801_priv *priv)
if (!(tco_ctl & TCOCTL_EN))
return;
- memset(&tco_res[0], 0, sizeof(tco_res));
+ memset(tco_res, 0, sizeof(tco_res));
+
res = &tco_res[ICH_RES_IO_TCO];
res->start = tco_base & ~1;
res->end = res->start + 32 - 1;
@@ -1226,7 +1227,7 @@ static void i801_add_tco(struct i801_priv *priv)
res->flags = IORESOURCE_MEM;
pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
- &tco_res[0], 3, &tco_platform_data,
+ tco_res, 3, &tco_platform_data,
sizeof(tco_platform_data));
if (IS_ERR(pdev)) {
dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Change &array[0] to array since it's the same.
It fixes 80 character limit warning as well.
Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Signed-off-by: Matt Fleming <matt.f...@intel.com>
---
drivers/i2c/busses/i2c-i801.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c79dbe116ccc..e41005927746 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1173,7 +1173,8 @@ static void i801_add_tco(struct i801_priv *priv)
if (!(tco_ctl & TCOCTL_EN))
return;
- memset(&tco_res[0], 0, sizeof(tco_res));
+ memset(tco_res, 0, sizeof(tco_res));
+
res = &tco_res[ICH_RES_IO_TCO];
res->start = tco_base & ~1;
res->end = res->start + 32 - 1;
@@ -1226,7 +1227,7 @@ static void i801_add_tco(struct i801_priv *priv)
res->flags = IORESOURCE_MEM;
pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
- &tco_res[0], 3, &tco_platform_data,
+ tco_res, 3, &tco_platform_data,
sizeof(tco_platform_data));
if (IS_ERR(pdev)) {
dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Matt Fleming
Jul 27, 2015, 9:40:06 AM7/27/15
to
From: Andy Shevchenko <andriy.s...@linux.intel.com>
Prevent nested inclusion of the header.
Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
Cc: Wim Van Sebroeck <w...@iguana.be>
include/linux/platform_data/iTCO_wdt.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/iTCO_wdt.h b/include/linux/platform_data/iTCO_wdt.h
index ce53c2b01f6d..0d4f0e87594f 100644
--- a/include/linux/platform_data/iTCO_wdt.h
+++ b/include/linux/platform_data/iTCO_wdt.h
@@ -3,6 +3,7 @@
*/
#ifndef _ITCO_WDT_H_
+#define _ITCO_WDT_H_
/* Watchdog resources */
#define ICH_RES_IO_TCO 0
Prevent nested inclusion of the header.
Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
Cc: Wim Van Sebroeck <w...@iguana.be>
include/linux/platform_data/iTCO_wdt.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/iTCO_wdt.h b/include/linux/platform_data/iTCO_wdt.h
index ce53c2b01f6d..0d4f0e87594f 100644
--- a/include/linux/platform_data/iTCO_wdt.h
+++ b/include/linux/platform_data/iTCO_wdt.h
@@ -3,6 +3,7 @@
*/
#ifndef _ITCO_WDT_H_
+#define _ITCO_WDT_H_
/* Watchdog resources */
#define ICH_RES_IO_TCO 0
Matt Fleming
Jul 27, 2015, 9:40:06 AM7/27/15
to
From: Mika Westerberg <mika.we...@linux.intel.com>
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
drivers/i2c/busses/i2c-i801.c | 127 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 126 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3fdc27e..c79dbe116ccc 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -88,12 +88,13 @@
#include <linux/slab.h>
#include <linux/wait.h>
#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/iTCO_wdt.h>
#if (defined CONFIG_I2C_MUX_GPIO || defined CONFIG_I2C_MUX_GPIO_MODULE) && \
defined CONFIG_DMI
#include <linux/gpio.h>
#include <linux/i2c-mux-gpio.h>
-#include <linux/platform_device.h>
#endif
/* I801 SMBus address offsets */
@@ -113,6 +114,16 @@
#define SMBPCICTL 0x004
#define SMBPCISTS 0x006
#define SMBHSTCFG 0x040
+#define TCOBASE 0x050
+#define TCOCTL 0x054
+
+#define ACPIBASE 0x040
+#define ACPIBASE_SMI_OFF 0x030
+#define ACPICTRL 0x044
+#define ACPICTRL_EN BIT(7)
+
+#define SBREG_BAR 0x10
+#define SBREG_SMBCTRL 0xc6000c
/* Host status bits for SMBPCISTS */
#define SMBPCISTS_INTS 0x08
@@ -125,6 +136,9 @@
#define SMBHSTCFG_SMB_SMI_EN 2
#define SMBHSTCFG_I2C_EN 4
+/* TCO configuration bits for TCOCTL */
+#define TCOCTL_EN BIT(8)
+
/* Auxiliary control register bits, ICH4+ only */
#define SMBAUXCTL_CRC 1
#define SMBAUXCTL_E32B 2
@@ -221,6 +235,7 @@ struct i801_priv {
const struct i801_mux_config *mux_drvdata;
struct platform_device *mux_pdev;
#endif
+ struct platform_device *tco_pdev;
};
#define FEATURE_SMBUS_PEC (1 << 0)
@@ -230,6 +245,7 @@ struct i801_priv {
#define FEATURE_IRQ (1 << 4)
/* Not really a feature, but it's convenient to handle it as such */
#define FEATURE_IDF (1 << 15)
+#define FEATURE_TCO (1 << 16)
static const char *i801_feature_names[] = {
"SMBus PEC",
@@ -1132,6 +1148,102 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
}
#endif
+static const struct iTCO_wdt_platform_data tco_platform_data = {
+ .name = "Intel PCH",
+ .iTCO_version = 4,
+};
+
+static DEFINE_SPINLOCK(p2sb_spinlock);
+
+static void i801_add_tco(struct i801_priv *priv)
+{
+ struct pci_dev *pci_dev = priv->pci_dev;
+ struct resource tco_res[3], *res;
+ struct platform_device *pdev;
+ unsigned int devfn;
+ u32 tco_base, tco_ctl;
+ u32 base_addr, ctrl_val;
+ u64 base64_addr;
+
+ if (!(priv->features & FEATURE_TCO))
+ return;
+
+ pci_read_config_dword(pci_dev, TCOBASE, &tco_base);
+ pci_read_config_dword(pci_dev, TCOCTL, &tco_ctl);
+ if (!(tco_ctl & TCOCTL_EN))
+ return;
+
+ memset(&tco_res[0], 0, sizeof(tco_res));
+ res = &tco_res[ICH_RES_IO_TCO];
+ res->start = tco_base & ~1;
+ res->end = res->start + 32 - 1;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Power Management registers.
+ */
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 2);
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPIBASE, &base_addr);
+
+ res = &tco_res[ICH_RES_IO_SMI];
+ res->start = (base_addr & ~1) + ACPIBASE_SMI_OFF;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Enable the ACPI I/O space.
+ */
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPICTRL, &ctrl_val);
+ ctrl_val |= ACPICTRL_EN;
+ pci_bus_write_config_dword(pci_dev->bus, devfn, ACPICTRL, ctrl_val);
+
+ /*
+ * We must access the NO_REBOOT bit over the Primary to Sideband
+ * bridge (P2SB). The BIOS prevents the P2SB device from being
+ * enumerated by the PCI subsystem, so we need to unhide/hide it
+ * to lookup the P2SB BAR.
+ */
+ spin_lock(&p2sb_spinlock);
+
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
+
+ /* Unhide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
+ base64_addr = base_addr & 0xfffffff0;
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
+ base64_addr |= (u64)base_addr << 32;
+
+ /* Hide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x1);
+ spin_unlock(&p2sb_spinlock);
+
+ res = &tco_res[ICH_RES_MEM_OFF];
+ res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_MEM;
+
+ pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+ &tco_res[0], 3, &tco_platform_data,
+ sizeof(tco_platform_data));
+ if (IS_ERR(pdev)) {
+ dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
+ return;
+ }
+
+ priv->tco_pdev = pdev;
+}
+
+static void i801_del_tco(struct i801_priv *priv)
+{
+ if (priv->tco_pdev) {
+ platform_device_unregister(priv->tco_pdev);
+ priv->tco_pdev = NULL;
+ }
+}
+
static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
unsigned char temp;
@@ -1149,6 +1261,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
priv->pci_dev = dev;
switch (dev->device) {
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
+ priv->features |= FEATURE_I2C_BLOCK_READ;
+ priv->features |= FEATURE_IRQ;
+ priv->features |= FEATURE_SMBUS_PEC;
+ priv->features |= FEATURE_BLOCK_BUFFER;
+ priv->features |= FEATURE_TCO;
+ break;
+
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2:
@@ -1265,6 +1386,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
dev_info(&dev->dev, "SMBus using %s\n",
priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
+ i801_add_tco(priv);
+
/* set up the sysfs linkage to our parent device */
priv->adapter.dev.parent = &dev->dev;
@@ -1296,6 +1419,8 @@ static void i801_remove(struct pci_dev *dev)
i2c_del_adapter(&priv->adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+ i801_del_tco(priv);
+
/*
* do not call pci_disable_device(dev) since it can cause hard hangs on
* some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
drivers/i2c/busses/i2c-i801.c | 127 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 126 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3fdc27e..c79dbe116ccc 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -88,12 +88,13 @@
#include <linux/slab.h>
#include <linux/wait.h>
#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/iTCO_wdt.h>
#if (defined CONFIG_I2C_MUX_GPIO || defined CONFIG_I2C_MUX_GPIO_MODULE) && \
defined CONFIG_DMI
#include <linux/gpio.h>
#include <linux/i2c-mux-gpio.h>
-#include <linux/platform_device.h>
#endif
/* I801 SMBus address offsets */
@@ -113,6 +114,16 @@
#define SMBPCICTL 0x004
#define SMBPCISTS 0x006
#define SMBHSTCFG 0x040
+#define TCOBASE 0x050
+#define TCOCTL 0x054
+
+#define ACPIBASE 0x040
+#define ACPIBASE_SMI_OFF 0x030
+#define ACPICTRL 0x044
+#define ACPICTRL_EN BIT(7)
+
+#define SBREG_BAR 0x10
+#define SBREG_SMBCTRL 0xc6000c
/* Host status bits for SMBPCISTS */
#define SMBPCISTS_INTS 0x08
@@ -125,6 +136,9 @@
#define SMBHSTCFG_SMB_SMI_EN 2
#define SMBHSTCFG_I2C_EN 4
+/* TCO configuration bits for TCOCTL */
+#define TCOCTL_EN BIT(8)
+
/* Auxiliary control register bits, ICH4+ only */
#define SMBAUXCTL_CRC 1
#define SMBAUXCTL_E32B 2
@@ -221,6 +235,7 @@ struct i801_priv {
const struct i801_mux_config *mux_drvdata;
struct platform_device *mux_pdev;
#endif
+ struct platform_device *tco_pdev;
};
#define FEATURE_SMBUS_PEC (1 << 0)
@@ -230,6 +245,7 @@ struct i801_priv {
#define FEATURE_IRQ (1 << 4)
/* Not really a feature, but it's convenient to handle it as such */
#define FEATURE_IDF (1 << 15)
+#define FEATURE_TCO (1 << 16)
static const char *i801_feature_names[] = {
"SMBus PEC",
@@ -1132,6 +1148,102 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
}
#endif
+static const struct iTCO_wdt_platform_data tco_platform_data = {
+ .name = "Intel PCH",
+ .iTCO_version = 4,
+};
+
+static DEFINE_SPINLOCK(p2sb_spinlock);
+
+static void i801_add_tco(struct i801_priv *priv)
+{
+ struct pci_dev *pci_dev = priv->pci_dev;
+ struct resource tco_res[3], *res;
+ struct platform_device *pdev;
+ unsigned int devfn;
+ u32 tco_base, tco_ctl;
+ u32 base_addr, ctrl_val;
+ u64 base64_addr;
+
+ if (!(priv->features & FEATURE_TCO))
+ return;
+
+ pci_read_config_dword(pci_dev, TCOBASE, &tco_base);
+ pci_read_config_dword(pci_dev, TCOCTL, &tco_ctl);
+ if (!(tco_ctl & TCOCTL_EN))
+ return;
+
+ memset(&tco_res[0], 0, sizeof(tco_res));
+ res = &tco_res[ICH_RES_IO_TCO];
+ res->start = tco_base & ~1;
+ res->end = res->start + 32 - 1;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Power Management registers.
+ */
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 2);
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPIBASE, &base_addr);
+
+ res = &tco_res[ICH_RES_IO_SMI];
+ res->start = (base_addr & ~1) + ACPIBASE_SMI_OFF;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Enable the ACPI I/O space.
+ */
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPICTRL, &ctrl_val);
+ ctrl_val |= ACPICTRL_EN;
+ pci_bus_write_config_dword(pci_dev->bus, devfn, ACPICTRL, ctrl_val);
+
+ /*
+ * We must access the NO_REBOOT bit over the Primary to Sideband
+ * bridge (P2SB). The BIOS prevents the P2SB device from being
+ * enumerated by the PCI subsystem, so we need to unhide/hide it
+ * to lookup the P2SB BAR.
+ */
+ spin_lock(&p2sb_spinlock);
+
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
+
+ /* Unhide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
+ base64_addr = base_addr & 0xfffffff0;
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
+ base64_addr |= (u64)base_addr << 32;
+
+ /* Hide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x1);
+ spin_unlock(&p2sb_spinlock);
+
+ res = &tco_res[ICH_RES_MEM_OFF];
+ res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_MEM;
+
+ pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+ &tco_res[0], 3, &tco_platform_data,
+ sizeof(tco_platform_data));
+ if (IS_ERR(pdev)) {
+ dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
+ return;
+ }
+
+ priv->tco_pdev = pdev;
+}
+
+static void i801_del_tco(struct i801_priv *priv)
+{
+ if (priv->tco_pdev) {
+ platform_device_unregister(priv->tco_pdev);
+ priv->tco_pdev = NULL;
+ }
+}
+
static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
unsigned char temp;
@@ -1149,6 +1261,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
priv->pci_dev = dev;
switch (dev->device) {
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
+ priv->features |= FEATURE_I2C_BLOCK_READ;
+ priv->features |= FEATURE_IRQ;
+ priv->features |= FEATURE_SMBUS_PEC;
+ priv->features |= FEATURE_BLOCK_BUFFER;
+ priv->features |= FEATURE_TCO;
+ break;
+
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2:
@@ -1265,6 +1386,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
dev_info(&dev->dev, "SMBus using %s\n",
priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
+ i801_add_tco(priv);
+
/* set up the sysfs linkage to our parent device */
priv->adapter.dev.parent = &dev->dev;
@@ -1296,6 +1419,8 @@ static void i801_remove(struct pci_dev *dev)
i2c_del_adapter(&priv->adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+ i801_del_tco(priv);
+
/*
* do not call pci_disable_device(dev) since it can cause hard hangs on
* some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
Matt Fleming
Jul 27, 2015, 9:40:08 AM7/27/15
to
From: Matt Fleming <matt.f...@intel.com>
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
This change introduces a new 'struct iTCO_wdt_platform_data' for use
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.
Cc: Peter Tyser <pty...@xes-inc.com>
Cc: Samuel Ortiz <sa...@linux.intel.com>
Cc: Lee Jones <lee....@linaro.org>
drivers/watchdog/Kconfig | 2 +-
drivers/watchdog/iTCO_wdt.c | 11 +++++------
include/linux/mfd/lpc_ich.h | 6 ------
include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
5 files changed, 53 insertions(+), 16 deletions(-)
create mode 100644 include/linux/platform_data/iTCO_wdt.h
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 8de34398abc0..d190b74a6321 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -66,6 +66,7 @@
#include <linux/pci.h>
#include <linux/mfd/core.h>
#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/iTCO_wdt.h>
#define ACPIBASE 0x40
#define ACPIBASE_GPE_OFF 0x28
@@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
priv->actrl_pbase_save = reg_save;
}
-static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
+static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
{
+ struct iTCO_wdt_platform_data *pdata;
struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+ struct lpc_ich_info *info;
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
+
+ pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ info = &lpc_chipset_info[priv->chipset];
+
+ pdata->iTCO_version = info->iTCO_version;
+ strcpy(pdata->name, info->name);
+
+ cell->platform_data = pdata;
+ cell->pdata_size = sizeof(*pdata);
+ return 0;
+}
+
+static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
+{
+ struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
cell->platform_data = &lpc_chipset_info[priv->chipset];
cell->pdata_size = sizeof(struct lpc_ich_info);
@@ -933,7 +956,7 @@ gpe0_done:
lpc_chipset_info[priv->chipset].use_gpio = ret;
lpc_ich_enable_gpio_space(dev);
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+ lpc_ich_finalize_gpio_cell(dev);
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
@@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
res->end = base_addr + ACPIBASE_PMC_END;
}
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+ ret = lpc_ich_finalize_wdt_cell(dev);
+ if (ret)
+ goto wdt_done;
+
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..5336fe2ff689 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -797,7 +797,7 @@ config ITCO_WDT
tristate "Intel TCO Timer/Watchdog"
depends on (X86 || IA64) && PCI
select WATCHDOG_CORE
- select LPC_ICH
+ depends on LPC_ICH || I2C_I801
---help---
Hardware driver for the intel TCO timer based watchdog devices.
These drivers are included in the Intel 82801 I/O Controller
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3c3fd417ddeb..9a6e70976f64 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -66,8 +66,7 @@
#include <linux/spinlock.h> /* For spin_lock/spin_unlock/... */
#include <linux/uaccess.h> /* For copy_to_user/put_user/... */
#include <linux/io.h> /* For inb/outb/... */
-#include <linux/mfd/core.h>
-#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/iTCO_wdt.h>
#include "iTCO_vendor.h"
@@ -418,9 +417,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
{
int ret = -ENODEV;
unsigned long val32;
- struct lpc_ich_info *ich_info = dev_get_platdata(&dev->dev);
+ struct iTCO_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
- if (!ich_info)
+ if (!pdata)
goto out;
spin_lock_init(&iTCO_wdt_private.io_lock);
@@ -435,7 +434,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
if (!iTCO_wdt_private.smi_res)
goto out;
- iTCO_wdt_private.iTCO_version = ich_info->iTCO_version;
+ iTCO_wdt_private.iTCO_version = pdata->iTCO_version;
iTCO_wdt_private.dev = dev;
iTCO_wdt_private.pdev = to_pci_dev(dev->dev.parent);
@@ -501,7 +500,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
}
pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
- ich_info->name, ich_info->iTCO_version, (u64)TCOBASE);
+ pdata->name, pdata->iTCO_version, (u64)TCOBASE);
/* Clear out the (probably old) status */
if (iTCO_wdt_private.iTCO_version == 3) {
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 8feac782fa83..2b300b44f994 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -20,12 +20,6 @@
#ifndef LPC_ICH_H
#define LPC_ICH_H
-/* Watchdog resources */
-#define ICH_RES_IO_TCO 0
-#define ICH_RES_IO_SMI 1
-#define ICH_RES_MEM_OFF 2
-#define ICH_RES_MEM_GCS_PMC 0
-
/* GPIO resources */
#define ICH_RES_GPIO 0
#define ICH_RES_GPE0 1
diff --git a/include/linux/platform_data/iTCO_wdt.h b/include/linux/platform_data/iTCO_wdt.h
new file mode 100644
index 000000000000..ce53c2b01f6d
--- /dev/null
+++ b/include/linux/platform_data/iTCO_wdt.h
@@ -0,0 +1,18 @@
+/*
+ * Platform data for the Intel TCO Watchdog
+ */
+
+#ifndef _ITCO_WDT_H_
+
+/* Watchdog resources */
+#define ICH_RES_IO_TCO 0
+#define ICH_RES_IO_SMI 1
+#define ICH_RES_MEM_OFF 2
+#define ICH_RES_MEM_GCS_PMC 0
+
+struct iTCO_wdt_platform_data {
+ char name[32];
+ unsigned int iTCO_version;
+};
+
+#endif /* _ITCO_WDT_H_ */
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
This change introduces a new 'struct iTCO_wdt_platform_data' for use
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.
Cc: Peter Tyser <pty...@xes-inc.com>
Cc: Samuel Ortiz <sa...@linux.intel.com>
Cc: Lee Jones <lee....@linaro.org>
Cc: Wim Van Sebroeck <w...@iguana.be>
drivers/mfd/lpc_ich.c | 32 +++++++++++++++++++++++++++++---
drivers/watchdog/Kconfig | 2 +-
drivers/watchdog/iTCO_wdt.c | 11 +++++------
include/linux/mfd/lpc_ich.h | 6 ------
include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
5 files changed, 53 insertions(+), 16 deletions(-)
create mode 100644 include/linux/platform_data/iTCO_wdt.h
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 8de34398abc0..d190b74a6321 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -66,6 +66,7 @@
#include <linux/pci.h>
#include <linux/mfd/core.h>
#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/iTCO_wdt.h>
#define ACPIBASE 0x40
#define ACPIBASE_GPE_OFF 0x28
@@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
priv->actrl_pbase_save = reg_save;
}
-static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
+static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
{
+ struct iTCO_wdt_platform_data *pdata;
struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+ struct lpc_ich_info *info;
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
+
+ pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ info = &lpc_chipset_info[priv->chipset];
+
+ pdata->iTCO_version = info->iTCO_version;
+ strcpy(pdata->name, info->name);
+
+ cell->platform_data = pdata;
+ cell->pdata_size = sizeof(*pdata);
+ return 0;
+}
+
+static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
+{
+ struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
cell->platform_data = &lpc_chipset_info[priv->chipset];
cell->pdata_size = sizeof(struct lpc_ich_info);
@@ -933,7 +956,7 @@ gpe0_done:
lpc_chipset_info[priv->chipset].use_gpio = ret;
lpc_ich_enable_gpio_space(dev);
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+ lpc_ich_finalize_gpio_cell(dev);
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
@@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
res->end = base_addr + ACPIBASE_PMC_END;
}
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+ ret = lpc_ich_finalize_wdt_cell(dev);
+ if (ret)
+ goto wdt_done;
+
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..5336fe2ff689 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -797,7 +797,7 @@ config ITCO_WDT
tristate "Intel TCO Timer/Watchdog"
depends on (X86 || IA64) && PCI
select WATCHDOG_CORE
- select LPC_ICH
+ depends on LPC_ICH || I2C_I801
---help---
Hardware driver for the intel TCO timer based watchdog devices.
These drivers are included in the Intel 82801 I/O Controller
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3c3fd417ddeb..9a6e70976f64 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -66,8 +66,7 @@
#include <linux/spinlock.h> /* For spin_lock/spin_unlock/... */
#include <linux/uaccess.h> /* For copy_to_user/put_user/... */
#include <linux/io.h> /* For inb/outb/... */
-#include <linux/mfd/core.h>
-#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/iTCO_wdt.h>
#include "iTCO_vendor.h"
@@ -418,9 +417,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
{
int ret = -ENODEV;
unsigned long val32;
- struct lpc_ich_info *ich_info = dev_get_platdata(&dev->dev);
+ struct iTCO_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
- if (!ich_info)
+ if (!pdata)
goto out;
spin_lock_init(&iTCO_wdt_private.io_lock);
@@ -435,7 +434,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
if (!iTCO_wdt_private.smi_res)
goto out;
- iTCO_wdt_private.iTCO_version = ich_info->iTCO_version;
+ iTCO_wdt_private.iTCO_version = pdata->iTCO_version;
iTCO_wdt_private.dev = dev;
iTCO_wdt_private.pdev = to_pci_dev(dev->dev.parent);
@@ -501,7 +500,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
}
pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
- ich_info->name, ich_info->iTCO_version, (u64)TCOBASE);
+ pdata->name, pdata->iTCO_version, (u64)TCOBASE);
/* Clear out the (probably old) status */
if (iTCO_wdt_private.iTCO_version == 3) {
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 8feac782fa83..2b300b44f994 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -20,12 +20,6 @@
#ifndef LPC_ICH_H
#define LPC_ICH_H
-/* Watchdog resources */
-#define ICH_RES_IO_TCO 0
-#define ICH_RES_IO_SMI 1
-#define ICH_RES_MEM_OFF 2
-#define ICH_RES_MEM_GCS_PMC 0
-
/* GPIO resources */
#define ICH_RES_GPIO 0
#define ICH_RES_GPE0 1
diff --git a/include/linux/platform_data/iTCO_wdt.h b/include/linux/platform_data/iTCO_wdt.h
new file mode 100644
index 000000000000..ce53c2b01f6d
--- /dev/null
+++ b/include/linux/platform_data/iTCO_wdt.h
@@ -0,0 +1,18 @@
+/*
+ * Platform data for the Intel TCO Watchdog
+ */
+
+#ifndef _ITCO_WDT_H_
+
+/* Watchdog resources */
+#define ICH_RES_IO_TCO 0
+#define ICH_RES_IO_SMI 1
+#define ICH_RES_MEM_OFF 2
+#define ICH_RES_MEM_GCS_PMC 0
+
+struct iTCO_wdt_platform_data {
+ char name[32];
+ unsigned int iTCO_version;
+};
+
+#endif /* _ITCO_WDT_H_ */
Matt Fleming
Jul 27, 2015, 9:40:09 AM7/27/15
to
From: Matt Fleming <matt.f...@intel.com>
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
1 file changed, 34 insertions(+), 24 deletions(-)
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 9a6e70976f64..17dfbc51b85a 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -145,58 +145,65 @@ static inline unsigned int ticks_to_seconds(int ticks)
return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 6) / 10;
}
+static inline u32 no_reboot_bit(void)
+{
+ u32 enable_bit;
+
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 3:
+ enable_bit = 0x00000010;
+ break;
+ case 2:
+ enable_bit = 0x00000020;
+ break;
+ default:
+ enable_bit = 0x00000002;
+ break;
+ }
+
+ return enable_bit;
+}
+
static void iTCO_wdt_set_NO_REBOOT_bit(void)
{
u32 val32;
/* Set the NO_REBOOT bit: this disables reboots */
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000010;
- writel(val32, iTCO_wdt_private.gcs_pmc);
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000020;
+ val32 |= no_reboot_bit();
writel(val32, iTCO_wdt_private.gcs_pmc);
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 |= 0x00000002;
+ val32 |= no_reboot_bit();
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
}
}
static int iTCO_wdt_unset_NO_REBOOT_bit(void)
{
+ u32 enable_bit = no_reboot_bit();
int ret = 0;
- u32 val32;
+ u32 val32 = 0;
/* Unset the NO_REBOOT bit: this enables reboots */
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffef;
- writel(val32, iTCO_wdt_private.gcs_pmc);
-
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000010)
- ret = -EIO;
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffdf;
+ val32 &= ~enable_bit;
writel(val32, iTCO_wdt_private.gcs_pmc);
val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000020)
- ret = -EIO;
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 &= 0xfffffffd;
+ val32 &= ~enable_bit;
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- if (val32 & 0x00000002)
- ret = -EIO;
}
+ if (val32 & enable_bit)
+ ret = -EIO;
+
return ret; /* returns: 0 = OK, -EIO = Error */
}
@@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
+ if (iTCO_wdt_private.iTCO_version == 4) {
+ outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ } else if (iTCO_wdt_private.iTCO_version == 3) {
outl(0x20008, TCO1_STS);
} else {
outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
1 file changed, 34 insertions(+), 24 deletions(-)
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 9a6e70976f64..17dfbc51b85a 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -145,58 +145,65 @@ static inline unsigned int ticks_to_seconds(int ticks)
return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 6) / 10;
}
+static inline u32 no_reboot_bit(void)
+{
+ u32 enable_bit;
+
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 3:
+ enable_bit = 0x00000010;
+ break;
+ case 2:
+ enable_bit = 0x00000020;
+ break;
+ default:
+ enable_bit = 0x00000002;
+ break;
+ }
+
+ return enable_bit;
+}
+
static void iTCO_wdt_set_NO_REBOOT_bit(void)
{
u32 val32;
/* Set the NO_REBOOT bit: this disables reboots */
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000010;
- writel(val32, iTCO_wdt_private.gcs_pmc);
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000020;
+ val32 |= no_reboot_bit();
writel(val32, iTCO_wdt_private.gcs_pmc);
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 |= 0x00000002;
+ val32 |= no_reboot_bit();
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
}
}
static int iTCO_wdt_unset_NO_REBOOT_bit(void)
{
+ u32 enable_bit = no_reboot_bit();
int ret = 0;
- u32 val32;
+ u32 val32 = 0;
/* Unset the NO_REBOOT bit: this enables reboots */
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffef;
- writel(val32, iTCO_wdt_private.gcs_pmc);
-
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000010)
- ret = -EIO;
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffdf;
+ val32 &= ~enable_bit;
writel(val32, iTCO_wdt_private.gcs_pmc);
val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000020)
- ret = -EIO;
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 &= 0xfffffffd;
+ val32 &= ~enable_bit;
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- if (val32 & 0x00000002)
- ret = -EIO;
}
+ if (val32 & enable_bit)
+ ret = -EIO;
+
return ret; /* returns: 0 = OK, -EIO = Error */
}
@@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
pdata->name, pdata->iTCO_version, (u64)TCOBASE);
/* Clear out the (probably old) status */
- if (iTCO_wdt_private.iTCO_version == 3) {
/* Clear out the (probably old) status */
+ if (iTCO_wdt_private.iTCO_version == 4) {
+ outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ } else if (iTCO_wdt_private.iTCO_version == 3) {
outl(0x20008, TCO1_STS);
} else {
outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
Matt Fleming
Jul 27, 2015, 9:40:10 AM7/27/15
to
From: Matt Fleming <matt.f...@intel.com>
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
Andy Shevchenko (2):
iTCO_wdt: fixup for the header
i2c-i801: fixup regarding watchdog timer
Matt Fleming (2):
iTCO_wdt: Expose watchdog properties using platform data
iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 128 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/watchdog/Kconfig | 2 +-
drivers/watchdog/iTCO_wdt.c | 69 ++++++++++--------
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/iTCO_wdt.h | 19 +++++
6 files changed, 215 insertions(+), 41 deletions(-)
create mode 100644 include/linux/platform_data/iTCO_wdt.h
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
Andy Shevchenko (2):
iTCO_wdt: fixup for the header
i2c-i801: fixup regarding watchdog timer
Matt Fleming (2):
iTCO_wdt: Expose watchdog properties using platform data
iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 128 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/watchdog/Kconfig | 2 +-
drivers/watchdog/iTCO_wdt.c | 69 ++++++++++--------
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/iTCO_wdt.h | 19 +++++
6 files changed, 215 insertions(+), 41 deletions(-)
create mode 100644 include/linux/platform_data/iTCO_wdt.h
Guenter Roeck
Jul 27, 2015, 9:50:08 AM7/27/15
to
in the cleanup path of this driver. Can you use devm_kzalloc() ?
Otherwise I think you'll need a cleanup path.
Guenter
Guenter Roeck
Jul 27, 2015, 10:10:07 AM7/27/15
to
On 07/27/2015 06:38 AM, Matt Fleming wrote:
> From: Mika Westerberg <mika.we...@linux.intel.com>
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
That really asks for an mfd driver, but that might be a bit overkill.
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Copying the i2c mailing list for additional feedback.
Not sure if that makes too much sense here, though, without converting
the rest of the driver to use BIT as well.
is strictly speaking unnecessary.
> + platform_device_unregister(priv->tco_pdev);
> + priv->tco_pdev = NULL;
Matt Fleming
Jul 27, 2015, 10:20:06 AM7/27/15
to
On Mon, 27 Jul, at 06:49:08AM, Guenter Roeck wrote:
>
> I don't see the platform data freed anywhere, neither in the error path nor
> in the cleanup path of this driver. Can you use devm_kzalloc() ?
> Otherwise I think you'll need a cleanup path.
Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!
>
> I don't see the platform data freed anywhere, neither in the error path nor
> in the cleanup path of this driver. Can you use devm_kzalloc() ?
> Otherwise I think you'll need a cleanup path.
--
Matt Fleming, Intel Open Source Technology Center
Guenter Roeck
Jul 27, 2015, 10:20:06 AM7/27/15
to
On 07/27/2015 06:38 AM, Matt Fleming wrote:
> From: Andy Shevchenko <andriy.s...@linux.intel.com>
>
> Change &array[0] to array since it's the same.
>
> It fixes 80 character limit warning as well.
>
> Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Should be merged into patch 2.
>
> Change &array[0] to array since it's the same.
>
> It fixes 80 character limit warning as well.
>
> Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Guenter
Guenter Roeck
Jul 27, 2015, 10:20:06 AM7/27/15
to
On 07/27/2015 06:38 AM, Matt Fleming wrote:
> From: Andy Shevchenko <andriy.s...@linux.intel.com>
>
> Prevent nested inclusion of the header.
>
> Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Should be merged into patch 1.
>
> Prevent nested inclusion of the header.
>
> Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Guenter
> ---
> include/linux/platform_data/iTCO_wdt.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/platform_data/iTCO_wdt.h b/include/linux/platform_data/iTCO_wdt.h
> index ce53c2b01f6d..0d4f0e87594f 100644
> --- a/include/linux/platform_data/iTCO_wdt.h
> +++ b/include/linux/platform_data/iTCO_wdt.h
> @@ -3,6 +3,7 @@
> */
>
> #ifndef _ITCO_WDT_H_
> +#define _ITCO_WDT_H_
>
> /* Watchdog resources */
> #define ICH_RES_IO_TCO 0
>
--
Guenter Roeck
Jul 27, 2015, 10:30:08 AM7/27/15
to
On 07/27/2015 07:19 AM, Matt Fleming wrote:
> On Mon, 27 Jul, at 06:49:08AM, Guenter Roeck wrote:
>>
>> I don't see the platform data freed anywhere, neither in the error path nor
>> in the cleanup path of this driver. Can you use devm_kzalloc() ?
>> Otherwise I think you'll need a cleanup path.
>
> Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!
>
Or maybe just use a static data structure, like in the i2c driver.
>>
>> I don't see the platform data freed anywhere, neither in the error path nor
>> in the cleanup path of this driver. Can you use devm_kzalloc() ?
>> Otherwise I think you'll need a cleanup path.
>
> Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!
>
Guenter
Guenter Roeck
Jul 27, 2015, 10:30:08 AM7/27/15
to
On 07/27/2015 06:38 AM, Matt Fleming wrote:
for clarification. We don't know what Intel will come up with for version 5,
and by then no one will remember that bit 2 applies for version 1 and 4 only.
Thanks,
Guenter
Lee Jones
Jul 27, 2015, 11:40:07 AM7/27/15
to
On Mon, 27 Jul 2015, Matt Fleming wrote:
> From: Matt Fleming <matt.f...@intel.com>
>
> Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> the SMBus, unlike previous generations of PCH/ICH where it was on the
> LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> watchdog driver is kind of backwards anyway.
>
> This change introduces a new 'struct iTCO_wdt_platform_data' for use
> inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> code, which neatly avoids having to include lpc_ich headers in the i801
> i2c driver.
>
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.
Is this patch related to Andy's patch-set?
> From: Matt Fleming <matt.f...@intel.com>
>
> Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> the SMBus, unlike previous generations of PCH/ICH where it was on the
> LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> watchdog driver is kind of backwards anyway.
>
> This change introduces a new 'struct iTCO_wdt_platform_data' for use
> inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> code, which neatly avoids having to include lpc_ich headers in the i801
> i2c driver.
>
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.
https://lkml.org/lkml/2015/7/27/599
> Cc: Peter Tyser <pty...@xes-inc.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> Cc: Lee Jones <lee....@linaro.org>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
> ---
> drivers/mfd/lpc_ich.c | 32 +++++++++++++++++++++++++++++---
> drivers/watchdog/Kconfig | 2 +-
> drivers/watchdog/iTCO_wdt.c | 11 +++++------
> include/linux/mfd/lpc_ich.h | 6 ------
> include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> 5 files changed, 53 insertions(+), 16 deletions(-)
> create mode 100644 include/linux/platform_data/iTCO_wdt.h
Why do they all have to be in a single patch?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Andy Shevchenko
Jul 27, 2015, 11:50:07 AM7/27/15
to
On Mon, 2015-07-27 at 16:33 +0100, Lee Jones wrote:
> On Mon, 27 Jul 2015, Matt Fleming wrote:
>
> > From: Matt Fleming <matt.f...@intel.com>
> >
> > Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible
> > across
> > the SMBus, unlike previous generations of PCH/ICH where it was on
> > the
> > LPC bus. Because it's on the SMBus, it doesn't make sense to pass
> > around
> > a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> > watchdog driver is kind of backwards anyway.
> >
> > This change introduces a new 'struct iTCO_wdt_platform_data' for
> > use
> > inside the iTCO watchdog driver and by the upcoming Intel
> > Sunrisepoint
> > code, which neatly avoids having to include lpc_ich headers in the
> > i801
> > i2c driver.
> >
> > A simple translation layer is provided for converting from the
> > existing
> > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Is this patch related to Andy's patch-set?
No, it's a separate stuff. They are pretty independent as I can see.
> On Mon, 27 Jul 2015, Matt Fleming wrote:
>
> > From: Matt Fleming <matt.f...@intel.com>
> >
> > Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible
> > across
> > the SMBus, unlike previous generations of PCH/ICH where it was on
> > the
> > LPC bus. Because it's on the SMBus, it doesn't make sense to pass
> > around
> > a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> > watchdog driver is kind of backwards anyway.
> >
> > This change introduces a new 'struct iTCO_wdt_platform_data' for
> > use
> > inside the iTCO watchdog driver and by the upcoming Intel
> > Sunrisepoint
> > code, which neatly avoids having to include lpc_ich headers in the
> > i801
> > i2c driver.
> >
> > A simple translation layer is provided for converting from the
> > existing
> > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Is this patch related to Andy's patch-set?
>
> https://lkml.org/lkml/2015/7/27/599
>
> > Cc: Peter Tyser <pty...@xes-inc.com>
> > Cc: Samuel Ortiz <sa...@linux.intel.com>
> > Cc: Lee Jones <lee....@linaro.org>
> > Cc: Wim Van Sebroeck <w...@iguana.be>
> > Signed-off-by: Matt Fleming <matt.f...@intel.com>
> > ---
> > drivers/mfd/lpc_ich.c | 32
> > +++++++++++++++++++++++++++++---
> > drivers/watchdog/Kconfig | 2 +-
> > drivers/watchdog/iTCO_wdt.c | 11 +++++------
> > include/linux/mfd/lpc_ich.h | 6 ------
> > include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> > 5 files changed, 53 insertions(+), 16 deletions(-)
> > create mode 100644 include/linux/platform_data/iTCO_wdt.h
>
> How are all of these changes related?
>
> Why do they all have to be in a single patch?
>
--
Intel Finland Oy
Matt Fleming
Jul 27, 2015, 4:40:07 PM7/27/15
to
On Mon, 27 Jul, at 04:33:44PM, Lee Jones wrote:
> On Mon, 27 Jul 2015, Matt Fleming wrote:
>
> > From: Matt Fleming <matt.f...@intel.com>
> >
> > Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> > the SMBus, unlike previous generations of PCH/ICH where it was on the
> > LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> > a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> > watchdog driver is kind of backwards anyway.
> >
> > This change introduces a new 'struct iTCO_wdt_platform_data' for use
> > inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> > code, which neatly avoids having to include lpc_ich headers in the i801
> > i2c driver.
> >
> > A simple translation layer is provided for converting from the existing
> > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Is this patch related to Andy's patch-set?
>
> https://lkml.org/lkml/2015/7/27/599
Nope, the two are independent.
> On Mon, 27 Jul 2015, Matt Fleming wrote:
>
> > From: Matt Fleming <matt.f...@intel.com>
> >
> > Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> > the SMBus, unlike previous generations of PCH/ICH where it was on the
> > LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> > a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> > watchdog driver is kind of backwards anyway.
> >
> > This change introduces a new 'struct iTCO_wdt_platform_data' for use
> > inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> > code, which neatly avoids having to include lpc_ich headers in the i801
> > i2c driver.
> >
> > A simple translation layer is provided for converting from the existing
> > 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Is this patch related to Andy's patch-set?
>
> https://lkml.org/lkml/2015/7/27/599
> > Cc: Peter Tyser <pty...@xes-inc.com>
> > Cc: Samuel Ortiz <sa...@linux.intel.com>
> > Cc: Lee Jones <lee....@linaro.org>
> > Cc: Wim Van Sebroeck <w...@iguana.be>
> > Signed-off-by: Matt Fleming <matt.f...@intel.com>
> > ---
> > drivers/mfd/lpc_ich.c | 32 +++++++++++++++++++++++++++++---
> > drivers/watchdog/Kconfig | 2 +-
> > drivers/watchdog/iTCO_wdt.c | 11 +++++------
> > include/linux/mfd/lpc_ich.h | 6 ------
> > include/linux/platform_data/iTCO_wdt.h | 18 ++++++++++++++++++
> > 5 files changed, 53 insertions(+), 16 deletions(-)
> > create mode 100644 include/linux/platform_data/iTCO_wdt.h
>
> How are all of these changes related?
The Kconfig change shouldn't have snuck into this patch though, that's
wrong, sorry.
> Why do they all have to be in a single patch?
that introduces the platform data structure and then a separate one that
uses it in iTCO_wdt and lpc_ich?
Technically we could also split the change between iTCO_wdt and lpc_ich,
but then we'd be passing in an 'lpc_ich_info' and pulling out a
'iTCO_wdt_platform_data' and that just seems crazy.
--
Matt Fleming, Intel Open Source Technology Center
Lee Jones
Jul 27, 2015, 5:40:05 PM7/27/15
to
Cross-subsystem patches are a pain in a backside.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Matt Fleming
Jul 28, 2015, 5:20:06 AM7/28/15
to
On Mon, 27 Jul, at 07:14:08AM, Guenter Roeck wrote:
> On 07/27/2015 06:38 AM, Matt Fleming wrote:
> >From: Andy Shevchenko <andriy.s...@linux.intel.com>
> >
> >Change &array[0] to array since it's the same.
> >
> >It fixes 80 character limit warning as well.
> >
> >Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> >Cc: Jean Delvare <jdel...@suse.com>
> >Cc: Wolfram Sang <w...@the-dreams.de>
> >Signed-off-by: Matt Fleming <matt.f...@intel.com>
>
> Should be merged into patch 2.
OK, will do.
> On 07/27/2015 06:38 AM, Matt Fleming wrote:
> >From: Andy Shevchenko <andriy.s...@linux.intel.com>
> >
> >Change &array[0] to array since it's the same.
> >
> >It fixes 80 character limit warning as well.
> >
> >Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> >Cc: Jean Delvare <jdel...@suse.com>
> >Cc: Wolfram Sang <w...@the-dreams.de>
> >Signed-off-by: Matt Fleming <matt.f...@intel.com>
>
> Should be merged into patch 2.
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Jul 28, 2015, 5:20:06 AM7/28/15
to
On Mon, 27 Jul, at 07:13:31AM, Guenter Roeck wrote:
> On 07/27/2015 06:38 AM, Matt Fleming wrote:
> >From: Andy Shevchenko <andriy.s...@linux.intel.com>
> >
> >Prevent nested inclusion of the header.
> >
> >Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> >Cc: Wim Van Sebroeck <w...@iguana.be>
> >Signed-off-by: Matt Fleming <matt.f...@intel.com>
>
> Should be merged into patch 1.
> On 07/27/2015 06:38 AM, Matt Fleming wrote:
> >From: Andy Shevchenko <andriy.s...@linux.intel.com>
> >
> >Prevent nested inclusion of the header.
> >
> >Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> >Cc: Wim Van Sebroeck <w...@iguana.be>
> >Signed-off-by: Matt Fleming <matt.f...@intel.com>
>
> Should be merged into patch 1.
OK, will do.
--
Matt Fleming, Intel Open Source Technology Center
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Jul 28, 2015, 5:20:06 AM7/28/15
to
On Mon, 27 Jul, at 10:32:36PM, Lee Jones wrote:
>
> If it's possible to split them up per subsystem, that would be ideal.
> Cross-subsystem patches are a pain in a backside.
Unfortunately that's not entirely possible because you need at least one
>
> If it's possible to split them up per subsystem, that would be ideal.
> Cross-subsystem patches are a pain in a backside.
patch that touches both lpc_ich and iTCO_wdt when you make the
transition from 'lpc_ich_info' to 'iTCO_wdt_platform_data'.
And it's not like this patch series can be split up and taken through
separate maintainer trees anyway. It all really needs to go through one
tree (whichever one that may be).
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Jul 28, 2015, 5:40:06 AM7/28/15
to
On Mon, 27 Jul, at 07:08:08AM, Guenter Roeck wrote:
> >@@ -113,6 +114,16 @@
> > #define SMBPCICTL 0x004
> > #define SMBPCISTS 0x006
> > #define SMBHSTCFG 0x040
> >+#define TCOBASE 0x050
> >+#define TCOCTL 0x054
> >+
> >+#define ACPIBASE 0x040
> >+#define ACPIBASE_SMI_OFF 0x030
> >+#define ACPICTRL 0x044
> >+#define ACPICTRL_EN BIT(7)
>
> If you use BIT, you should include bitops.h.
> Not sure if that makes too much sense here, though, without converting
> the rest of the driver to use BIT as well.
OK, I'll just switch to the existing notation used throughout the
> >@@ -113,6 +114,16 @@
> > #define SMBPCICTL 0x004
> > #define SMBPCISTS 0x006
> > #define SMBHSTCFG 0x040
> >+#define TCOBASE 0x050
> >+#define TCOCTL 0x054
> >+
> >+#define ACPIBASE 0x040
> >+#define ACPIBASE_SMI_OFF 0x030
> >+#define ACPICTRL 0x044
> >+#define ACPICTRL_EN BIT(7)
>
> If you use BIT, you should include bitops.h.
> Not sure if that makes too much sense here, though, without converting
> the rest of the driver to use BIT as well.
driver rather than using bitops.
> >+static void i801_del_tco(struct i801_priv *priv)
> >+{
> >+ if (priv->tco_pdev) {
>
> platform_device_unregister() handles NULL pointers, so this if statement
> is strictly speaking unnecessary.
> >+ platform_device_unregister(priv->tco_pdev);
> >+ priv->tco_pdev = NULL;
>
> Unnecessary; priv is going to be freed right afterwards.
--
Matt Fleming, Intel Open Source Technology Center
Lee Jones
Jul 28, 2015, 5:50:08 AM7/28/15
to
On Mon, 27 Jul 2015, Matt Fleming wrote:
> #define ACPIBASE 0x40
> #define ACPIBASE_GPE_OFF 0x28
> @@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
> priv->actrl_pbase_save = reg_save;
> }
>
> -static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
> +static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
> {
> + struct iTCO_wdt_platform_data *pdata;
> struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> + struct lpc_ich_info *info;
> + struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> +
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
Better to use devm_*
> + info = &lpc_chipset_info[priv->chipset];
> +
> + pdata->iTCO_version = info->iTCO_version;
> + strcpy(pdata->name, info->name);
> + cell->platform_data = pdata;
> + cell->pdata_size = sizeof(*pdata);
> + return 0;
> +}
> +
> +static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
> +{
> + struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> + struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
>
> cell->platform_data = &lpc_chipset_info[priv->chipset];
> cell->pdata_size = sizeof(struct lpc_ich_info);
are the actual similarities and differences between the two finalise
functions? They looks like they share enough lines for it to make
sense to have one function call and do different things in say a
switch statement, no?
> @@ -933,7 +956,7 @@ gpe0_done:
> lpc_chipset_info[priv->chipset].use_gpio = ret;
> lpc_ich_enable_gpio_space(dev);
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> + lpc_ich_finalize_gpio_cell(dev);
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
>
> @@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> res->end = base_addr + ACPIBASE_PMC_END;
> }
>
> - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> + ret = lpc_ich_finalize_wdt_cell(dev);
> + if (ret)
> + goto wdt_done;
> +
> ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Matt Fleming
Jul 28, 2015, 6:00:09 AM7/28/15
to
On Mon, 27 Jul, at 07:24:09AM, Guenter Roeck wrote:
> On 07/27/2015 07:19 AM, Matt Fleming wrote:
> >On Mon, 27 Jul, at 06:49:08AM, Guenter Roeck wrote:
> >>
> >>I don't see the platform data freed anywhere, neither in the error path nor
> >>in the cleanup path of this driver. Can you use devm_kzalloc() ?
> >>Otherwise I think you'll need a cleanup path.
> >
> >Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!
> >
> Or maybe just use a static data structure, like in the i2c driver.
The point of dynamically allocating it is that we can use the data from
> On 07/27/2015 07:19 AM, Matt Fleming wrote:
> >On Mon, 27 Jul, at 06:49:08AM, Guenter Roeck wrote:
> >>
> >>I don't see the platform data freed anywhere, neither in the error path nor
> >>in the cleanup path of this driver. Can you use devm_kzalloc() ?
> >>Otherwise I think you'll need a cleanup path.
> >
> >Oops, good catch. Yes, devm_kzalloc() can be used here, thanks!
> >
> Or maybe just use a static data structure, like in the i2c driver.
the static lpc_ich_chipset_info array and munge into the correct
platform data.
The alternative would be to go and mass-modify that array to include
iTCO_wdt_platform_data objects that we could directly pass to the
iTCO_wdt driver. I wanted to avoid the code churn, but I'm not super
bothered either way if people have strong opinions about it.
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Jul 28, 2015, 6:20:08 AM7/28/15
to
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Jul 28, 2015, 7:10:06 AM7/28/15
to
confusing to start mixing cases rather than sticking with the ugly upper
case. Especially since when you look in the iTCO_wdt driver all the
function and type names are written that way.
> > #define ACPIBASE 0x40
> > #define ACPIBASE_GPE_OFF 0x28
> > @@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
> > priv->actrl_pbase_save = reg_save;
> > }
> >
> > -static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
> > +static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
> > {
> > + struct iTCO_wdt_platform_data *pdata;
>
> Lowercase please.
> > struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > + struct lpc_ich_info *info;
> > + struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> > +
> > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
>
> Where is this freed?
>
> Better to use devm_*
> > + info = &lpc_chipset_info[priv->chipset];
> > +
> > + pdata->iTCO_version = info->iTCO_version;
>
> Lowercase please.
pdata->itco_version = info->iTCO_version;
I'm not sure that's an improvement.
>
> > + strcpy(pdata->name, info->name);
>
> strncpy() is safer.
declarations are of identical size in these two objects (but I guess
that could change in the future).
> > + cell->platform_data = pdata;
> > + cell->pdata_size = sizeof(*pdata);
> > + return 0;
> > +}
> > +
> > +static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
> > +{
> > + struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > + struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
> >
> > cell->platform_data = &lpc_chipset_info[priv->chipset];
> > cell->pdata_size = sizeof(struct lpc_ich_info);
>
> It's pretty hard to tell from the patch without applying it, but what
> are the actual similarities and differences between the two finalise
> functions? They looks like they share enough lines for it to make
> sense to have one function call and do different things in say a
> switch statement, no?
we use the static lpc_chipsec_info array.
I'm just personally not a fan of performing memory allocations from
within switch statement bodies, which is why I implemented this as two
separate finalize functions.
> > @@ -933,7 +956,7 @@ gpe0_done:
> > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > lpc_ich_enable_gpio_space(dev);
> >
> > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > + lpc_ich_finalize_gpio_cell(dev);
> > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> >
> > @@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > res->end = base_addr + ACPIBASE_PMC_END;
> > }
> >
> > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > + ret = lpc_ich_finalize_wdt_cell(dev);
> > + if (ret)
> > + goto wdt_done;
> > +
> > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
>
> Why do you have an mfd_add_devices() call for each device?
was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
iTCO_wdt driver to mfd model").
There's no good reason that I can see. Aaron?
--
Matt Fleming, Intel Open Source Technology Center
Lee Jones
Jul 28, 2015, 7:40:06 AM7/28/15
to
You are the only one. What makes iTCO 'special'?
$ ls drivers/watchdog/ | grep [A-Z]
iTCO_vendor.h
iTCO_vendor_support.c
iTCO_wdt.c
Kconfig
Makefile
Mixed case names (filenames, variables, etc) are frowned upon and
shouldn't be allowed anywhere. Please read Chapter 4 of
Documentation/CodingStyle.
> > > #define ACPIBASE 0x40
> > > #define ACPIBASE_GPE_OFF 0x28
> > > @@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
> > > priv->actrl_pbase_save = reg_save;
> > > }
> > >
> > > -static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
> > > +static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
> > > {
> > > + struct iTCO_wdt_platform_data *pdata;
> >
> > Lowercase please.
>
> See above.
> > > struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > > + struct lpc_ich_info *info;
> > > + struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> > > +
> > > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > + if (!pdata)
> > > + return -ENOMEM;
> >
> > Where is this freed?
> >
> > Better to use devm_*
>
> Yeah, Guenter caught this too. devm_* would definitely be better.
> > > + info = &lpc_chipset_info[priv->chipset];
> > > +
> > > + pdata->iTCO_version = info->iTCO_version;
> >
> > Lowercase please.
>
> Hmm... but then this line will read,
>
> pdata->itco_version = info->iTCO_version;
>
> I'm not sure that's an improvement.
coding standards we normally abide by. You can submit it either as
patch 1 of this set, or independently.
> > > + strcpy(pdata->name, info->name);
> >
> > strncpy() is safer.
>
> OK, I'll update this. Though it's worth pointing out that the name[]
> declarations are of identical size in these two objects (but I guess
> that could change in the future).
> > > + cell->platform_data = pdata;
> > > + cell->pdata_size = sizeof(*pdata);
> > > + return 0;
> > > +}
> > > +
> > > +static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
> > > +{
> > > + struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> > > + struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
> > >
> > > cell->platform_data = &lpc_chipset_info[priv->chipset];
> > > cell->pdata_size = sizeof(struct lpc_ich_info);
> >
> > It's pretty hard to tell from the patch without applying it, but what
> > are the actual similarities and differences between the two finalise
> > functions? They looks like they share enough lines for it to make
> > sense to have one function call and do different things in say a
> > switch statement, no?
>
> For LPC_WDT we dynamically allocate the platform data, and for LPC_GPIO
> we use the static lpc_chipsec_info array.
>
> I'm just personally not a fan of performing memory allocations from
> within switch statement bodies, which is why I implemented this as two
> separate finalize functions.
once it's applied.
> > > @@ -933,7 +956,7 @@ gpe0_done:
> > > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > > lpc_ich_enable_gpio_space(dev);
> > >
> > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > > + lpc_ich_finalize_gpio_cell(dev);
> > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> > >
> > > @@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > > res->end = base_addr + ACPIBASE_PMC_END;
> > > }
> > >
> > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > > + ret = lpc_ich_finalize_wdt_cell(dev);
> > > + if (ret)
> > > + goto wdt_done;
> > > +
> > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> >
> > Why do you have an mfd_add_devices() call for each device?
>
> Good question. This call has been present since March 2012 when support
> was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> iTCO_wdt driver to mfd model").
>
> There's no good reason that I can see. Aaron?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Matt Fleming
Jul 28, 2015, 8:50:07 AM7/28/15
to
On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
>
> The driver shouldn't be called that either.
>
> You are the only one. What makes iTCO 'special'?
I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
>
> The driver shouldn't be called that either.
>
> You are the only one. What makes iTCO 'special'?
it must have made sense to him at the time.
> > > > + info = &lpc_chipset_info[priv->chipset];
> > > > +
> > > > + pdata->iTCO_version = info->iTCO_version;
> > >
> > > Lowercase please.
> >
> > Hmm... but then this line will read,
> >
> > pdata->itco_version = info->iTCO_version;
> >
> > I'm not sure that's an improvement.
>
> Please consider making all of the variable names conform to the
> coding standards we normally abide by. You can submit it either as
> patch 1 of this set, or independently.
--
Matt Fleming, Intel Open Source Technology Center
Lee Jones
Jul 28, 2015, 11:10:05 AM7/28/15
to
On Tue, 28 Jul 2015, Matt Fleming wrote:
> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >
> > The driver shouldn't be called that either.
> >
> > You are the only one. What makes iTCO 'special'?
>
> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> it must have made sense to him at the time.
>
> > > > > + info = &lpc_chipset_info[priv->chipset];
> > > > > +
> > > > > + pdata->iTCO_version = info->iTCO_version;
> > > >
> > > > Lowercase please.
> > >
> > > Hmm... but then this line will read,
> > >
> > > pdata->itco_version = info->iTCO_version;
> > >
> > > I'm not sure that's an improvement.
> >
> > Please consider making all of the variable names conform to the
> > coding standards we normally abide by. You can submit it either as
> > patch 1 of this set, or independently.
>
> Right, I figured we were fast approaching this rabit hole.
No rabbit hole, just some fixups. If it takes you any more than 10
> >
> > The driver shouldn't be called that either.
> >
> > You are the only one. What makes iTCO 'special'?
>
> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> it must have made sense to him at the time.
>
> > > > > + info = &lpc_chipset_info[priv->chipset];
> > > > > +
> > > > > + pdata->iTCO_version = info->iTCO_version;
> > > >
> > > > Lowercase please.
> > >
> > > Hmm... but then this line will read,
> > >
> > > pdata->itco_version = info->iTCO_version;
> > >
> > > I'm not sure that's an improvement.
> >
> > Please consider making all of the variable names conform to the
> > coding standards we normally abide by. You can submit it either as
> > patch 1 of this set, or independently.
>
> Right, I figured we were fast approaching this rabit hole.
mins, I'd be surprised.
Let me know if you think it'll be too much trouble and I'll do the
fixups myself.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Guenter Roeck
Jul 28, 2015, 11:20:05 AM7/28/15
to
On 07/28/2015 08:00 AM, Lee Jones wrote:
> On Tue, 28 Jul 2015, Matt Fleming wrote:
>> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
>>>
>>> The driver shouldn't be called that either.
>>>
>>> You are the only one. What makes iTCO 'special'?
>>
>> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
>> it must have made sense to him at the time.
>>
Coding style wasn't as strict then as it is today. iTCO has just been kept
> On Tue, 28 Jul 2015, Matt Fleming wrote:
>> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
>>>
>>> The driver shouldn't be called that either.
>>>
>>> You are the only one. What makes iTCO 'special'?
>>
>> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
>> it must have made sense to him at the time.
>>
for historic reasons.
Sure, we could have changed it to lowercase, but so far no one bothered.
Plus, of course, there is always the element that some maintainers hate
that kind of cleanup, and patch submitters like Matt are stuck between
a rock and a hard place.
Guenter
Lee Jones
Jul 28, 2015, 11:30:07 AM7/28/15
to
On Tue, 28 Jul 2015, Guenter Roeck wrote:
> On 07/28/2015 08:00 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>
> >>>The driver shouldn't be called that either.
> >>>
> >>>You are the only one. What makes iTCO 'special'?
> >>
> >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>it must have made sense to him at the time.
> >>
>
> Coding style wasn't as strict then as it is today. iTCO has just been kept
> for historic reasons.
For sure, I get that, but it doesn't mean we can't do-the-right-thing
> On 07/28/2015 08:00 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>
> >>>The driver shouldn't be called that either.
> >>>
> >>>You are the only one. What makes iTCO 'special'?
> >>
> >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>it must have made sense to him at the time.
> >>
>
> Coding style wasn't as strict then as it is today. iTCO has just been kept
> for historic reasons.
(tm) now does it?
> Sure, we could have changed it to lowercase, but so far no one bothered.
> Plus, of course, there is always the element that some maintainers hate
> that kind of cleanup,
Greg KH et. al, have been doing public presentations telling everyone
that there is always kernel work for anyone who has the time; spelling
corrections and all.
> and patch submitters like Matt are stuck between
> a rock and a hard place.
Either do the 5min clean-up or don't, no biggy. If you don't then I
can do it. At the very least please don't add any _new_ camel case
variables or filenames.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Matt Fleming
Jul 28, 2015, 11:50:06 AM7/28/15
to
On Tue, 28 Jul, at 04:28:32PM, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
>
> > On 07/28/2015 08:00 AM, Lee Jones wrote:
> > >On Tue, 28 Jul 2015, Matt Fleming wrote:
> > >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > >>>
> > >>>The driver shouldn't be called that either.
> > >>>
> > >>>You are the only one. What makes iTCO 'special'?
> > >>
> > >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > >>it must have made sense to him at the time.
> > >>
> >
> > Coding style wasn't as strict then as it is today. iTCO has just been kept
> > for historic reasons.
>
> For sure, I get that, but it doesn't mean we can't do-the-right-thing
> (tm) now does it?
>
> > Sure, we could have changed it to lowercase, but so far no one bothered.
> > Plus, of course, there is always the element that some maintainers hate
> > that kind of cleanup,
>
> Really?
Yes, really.
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
>
> > On 07/28/2015 08:00 AM, Lee Jones wrote:
> > >On Tue, 28 Jul 2015, Matt Fleming wrote:
> > >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > >>>
> > >>>The driver shouldn't be called that either.
> > >>>
> > >>>You are the only one. What makes iTCO 'special'?
> > >>
> > >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > >>it must have made sense to him at the time.
> > >>
> >
> > Coding style wasn't as strict then as it is today. iTCO has just been kept
> > for historic reasons.
>
> For sure, I get that, but it doesn't mean we can't do-the-right-thing
> (tm) now does it?
>
> > Sure, we could have changed it to lowercase, but so far no one bothered.
> > Plus, of course, there is always the element that some maintainers hate
> > that kind of cleanup,
>
> Really?
> Surely any kind of clean-up is good clean-up. Especially as Greg KH
> et. al, have been doing public presentations telling everyone that
> there is always kernel work for anyone who has the time; spelling
> corrections and all.
those patches, other maintainers are less so.
> Matt,
>
> Either do the 5min clean-up or don't, no biggy. If you don't then I
> can do it.
> At the very least please don't add any _new_ camel case variables or
> filenames.
--
Matt Fleming, Intel Open Source Technology Center
Lee Jones
Jul 28, 2015, 12:00:07 PM7/28/15
to
On Tue, 28 Jul 2015, Matt Fleming wrote:
> On Tue, 28 Jul, at 04:28:32PM, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> >
> > > On 07/28/2015 08:00 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Matt Fleming wrote:
> > > >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > > >>>
> > > >>>The driver shouldn't be called that either.
> > > >>>
> > > >>>You are the only one. What makes iTCO 'special'?
> > > >>
> > > >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > > >>it must have made sense to him at the time.
> > > >>
> > >
> > > Coding style wasn't as strict then as it is today. iTCO has just been kept
> > > for historic reasons.
> >
> > For sure, I get that, but it doesn't mean we can't do-the-right-thing
> > (tm) now does it?
> >
> > > Sure, we could have changed it to lowercase, but so far no one bothered.
> > > Plus, of course, there is always the element that some maintainers hate
> > > that kind of cleanup,
> >
> > Really?
>
> Yes, really.
Well then they are silly (replace silly with something less ML
> On Tue, 28 Jul, at 04:28:32PM, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> >
> > > On 07/28/2015 08:00 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Matt Fleming wrote:
> > > >>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> > > >>>
> > > >>>The driver shouldn't be called that either.
> > > >>>
> > > >>>You are the only one. What makes iTCO 'special'?
> > > >>
> > > >>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> > > >>it must have made sense to him at the time.
> > > >>
> > >
> > > Coding style wasn't as strict then as it is today. iTCO has just been kept
> > > for historic reasons.
> >
> > For sure, I get that, but it doesn't mean we can't do-the-right-thing
> > (tm) now does it?
> >
> > > Sure, we could have changed it to lowercase, but so far no one bothered.
> > > Plus, of course, there is always the element that some maintainers hate
> > > that kind of cleanup,
> >
> > Really?
>
> Yes, really.
friendly if you like).
> > Surely any kind of clean-up is good clean-up. Especially as Greg KH
> > et. al, have been doing public presentations telling everyone that
> > there is always kernel work for anyone who has the time; spelling
> > corrections and all.
>
> That's what staging/ was invented for ;-) Greg is quite happy to take
> those patches, other maintainers are less so.
controversial or still in a state of flux. Not to over-rule
maintainers which didn't think a clean-up was important enough for
acceptance.
> > Either do the 5min clean-up or don't, no biggy. If you don't then I
> > can do it.
>
> Go for it.
>
> > At the very least please don't add any _new_ camel case variables or
> > filenames.
>
> OK, sure.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Guenter Roeck
Jul 28, 2015, 1:10:06 PM7/28/15
to
On 07/28/2015 08:28 AM, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
>
>> On 07/28/2015 08:00 AM, Lee Jones wrote:
>>> On Tue, 28 Jul 2015, Matt Fleming wrote:
>>>> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
>>>>>
>>>>> The driver shouldn't be called that either.
>>>>>
>>>>> You are the only one. What makes iTCO 'special'?
>>>>
>>>> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
>>>> it must have made sense to him at the time.
>>>>
>>
>> Coding style wasn't as strict then as it is today. iTCO has just been kept
>> for historic reasons.
>
> For sure, I get that, but it doesn't mean we can't do-the-right-thing
> (tm) now does it?
>
>> Sure, we could have changed it to lowercase, but so far no one bothered.
>> Plus, of course, there is always the element that some maintainers hate
>> that kind of cleanup,
>
> Really? Surely any kind of clean-up is good clean-up. Especially as
> Greg KH et. al, have been doing public presentations telling everyone
> that there is always kernel work for anyone who has the time; spelling
> corrections and all.
>
Yes, really. Just try to submit cleanup patches to maintainers other than
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
>
>> On 07/28/2015 08:00 AM, Lee Jones wrote:
>>> On Tue, 28 Jul 2015, Matt Fleming wrote:
>>>> On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
>>>>>
>>>>> The driver shouldn't be called that either.
>>>>>
>>>>> You are the only one. What makes iTCO 'special'?
>>>>
>>>> I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
>>>> it must have made sense to him at the time.
>>>>
>>
>> Coding style wasn't as strict then as it is today. iTCO has just been kept
>> for historic reasons.
>
> For sure, I get that, but it doesn't mean we can't do-the-right-thing
> (tm) now does it?
>
>> Sure, we could have changed it to lowercase, but so far no one bothered.
>> Plus, of course, there is always the element that some maintainers hate
>> that kind of cleanup,
>
> Really? Surely any kind of clean-up is good clean-up. Especially as
> Greg KH et. al, have been doing public presentations telling everyone
> that there is always kernel work for anyone who has the time; spelling
> corrections and all.
>
Greg and myself, and you'll see. It is a minefield.
Guenter
Lee Jones
Jul 28, 2015, 1:40:06 PM7/28/15
to
On Tue, 28 Jul 2015, Guenter Roeck wrote:
> On 07/28/2015 08:28 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> >
> >>On 07/28/2015 08:00 AM, Lee Jones wrote:
> >>>On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>>>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>>>
> >>>>>The driver shouldn't be called that either.
> >>>>>
> >>>>>You are the only one. What makes iTCO 'special'?
> >>>>
> >>>>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>>>it must have made sense to him at the time.
> >>>>
> >>
> >>Coding style wasn't as strict then as it is today. iTCO has just been kept
> >>for historic reasons.
> >
> >For sure, I get that, but it doesn't mean we can't do-the-right-thing
> >(tm) now does it?
> >
> >>Sure, we could have changed it to lowercase, but so far no one bothered.
> >>Plus, of course, there is always the element that some maintainers hate
> >>that kind of cleanup,
> >
> >Really? Surely any kind of clean-up is good clean-up. Especially as
> >Greg KH et. al, have been doing public presentations telling everyone
> >that there is always kernel work for anyone who has the time; spelling
> >corrections and all.
> >
>
> Yes, really. Just try to submit cleanup patches to maintainers other than
> Greg and myself, and you'll see. It is a minefield.
Admittedly some of us have our quirks, but I'm happy to challenge
> On 07/28/2015 08:28 AM, Lee Jones wrote:
> >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> >
> >>On 07/28/2015 08:00 AM, Lee Jones wrote:
> >>>On Tue, 28 Jul 2015, Matt Fleming wrote:
> >>>>On Tue, 28 Jul, at 12:37:21PM, Lee Jones wrote:
> >>>>>
> >>>>>The driver shouldn't be called that either.
> >>>>>
> >>>>>You are the only one. What makes iTCO 'special'?
> >>>>
> >>>>I don't know, I didn't write it. It looks like Wim did ~9 years ago, so
> >>>>it must have made sense to him at the time.
> >>>>
> >>
> >>Coding style wasn't as strict then as it is today. iTCO has just been kept
> >>for historic reasons.
> >
> >For sure, I get that, but it doesn't mean we can't do-the-right-thing
> >(tm) now does it?
> >
> >>Sure, we could have changed it to lowercase, but so far no one bothered.
> >>Plus, of course, there is always the element that some maintainers hate
> >>that kind of cleanup,
> >
> >Really? Surely any kind of clean-up is good clean-up. Especially as
> >Greg KH et. al, have been doing public presentations telling everyone
> >that there is always kernel work for anyone who has the time; spelling
> >corrections and all.
> >
>
> Yes, really. Just try to submit cleanup patches to maintainers other than
> Greg and myself, and you'll see. It is a minefield.
anyone that won't accept clean-up patches that make things better.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Guenter Roeck
Jul 28, 2015, 3:00:06 PM7/28/15
to
in the context of encouraging (or discouraging) new kernel developers.
If you are invited to the KS, it might make sense to attend that discussion.
Guenter
Aaron Sierra
Jul 28, 2015, 3:20:05 PM7/28/15
to
> > > > @@ -933,7 +956,7 @@ gpe0_done:
> > > > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > > > lpc_ich_enable_gpio_space(dev);
> > > >
> > > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > > > + lpc_ich_finalize_gpio_cell(dev);
> > > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > > &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> > > >
> > > > @@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > > > res->end = base_addr + ACPIBASE_PMC_END;
> > > > }
> > > >
> > > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > > > + ret = lpc_ich_finalize_wdt_cell(dev);
> > > > + if (ret)
> > > > + goto wdt_done;
> > > > +
> > > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > > &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> > >
> > > Why do you have an mfd_add_devices() call for each device?
> >
> > Good question. This call has been present since March 2012 when support
> > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> > iTCO_wdt driver to mfd model").
> >
> > There's no good reason that I can see. Aaron?
I chose to call mfd_add_devices() in each device init function
> > > > lpc_chipset_info[priv->chipset].use_gpio = ret;
> > > > lpc_ich_enable_gpio_space(dev);
> > > >
> > > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > > > + lpc_ich_finalize_gpio_cell(dev);
> > > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > > &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> > > >
> > > > @@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> > > > res->end = base_addr + ACPIBASE_PMC_END;
> > > > }
> > > >
> > > > - lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > > > + ret = lpc_ich_finalize_wdt_cell(dev);
> > > > + if (ret)
> > > > + goto wdt_done;
> > > > +
> > > > ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > > &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> > >
> > > Why do you have an mfd_add_devices() call for each device?
> >
> > Good question. This call has been present since March 2012 when support
> > was first added for iTCO_wdt in commit 887c8ec7219f ("watchdog: Convert
> > iTCO_wdt driver to mfd model").
> >
> > There's no good reason that I can see. Aaron?
because I thought it was the easiest way to avoid registering an
incomplete/invalid MFD cell should an error occur during init.
That way device registration wouldn't be an all-or-nothing affair.
Doesn't mfd_add_devices() bail out after the first unsuccessful
mfd to platform device translation?
-Aaron S.
Jean Delvare
Jul 29, 2015, 2:50:05 AM7/29/15
to
Hi Matt,
Le Monday 27 July 2015 à 14:38 +0100, Matt Fleming a écrit :
> From: Matt Fleming <matt.f...@intel.com>
>
> The revision of the watchdog hardware in Sunrisepoint necessitates a new
> "version" inside the TCO watchdog driver because some of the register
> layouts have changed.
>
> @@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
merge both with a conditional. If you prefer not to, then it probably
makes sense to change the whole block to a switch/case construct.
--
Jean Delvare
SUSE L3 Support
Le Monday 27 July 2015 à 14:38 +0100, Matt Fleming a écrit :
> From: Matt Fleming <matt.f...@intel.com>
>
> The revision of the watchdog hardware in Sunrisepoint necessitates a new
> "version" inside the TCO watchdog driver because some of the register
> layouts have changed.
>
> drivers/watchdog/iTCO_wdt.c | 58 ++++++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 9a6e70976f64..17dfbc51b85a 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> (...)
> 1 file changed, 34 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 9a6e70976f64..17dfbc51b85a 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -503,7 +510,10 @@ static int iTCO_wdt_probe(struct platform_device *dev)
> pdata->name, pdata->iTCO_version, (u64)TCOBASE);
>
> /* Clear out the (probably old) status */
>
> /* Clear out the (probably old) status */
> - if (iTCO_wdt_private.iTCO_version == 3) {
> + if (iTCO_wdt_private.iTCO_version == 4) {
> + outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
> + outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
> + } else if (iTCO_wdt_private.iTCO_version == 3) {
> outl(0x20008, TCO1_STS);
> } else {
> outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
The "version == 4" branch is a subset of the "else" branch, so you could
> + if (iTCO_wdt_private.iTCO_version == 4) {
> + outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
> + outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
> + } else if (iTCO_wdt_private.iTCO_version == 3) {
> outl(0x20008, TCO1_STS);
> } else {
> outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
merge both with a conditional. If you prefer not to, then it probably
makes sense to change the whole block to a switch/case construct.
--
Jean Delvare
SUSE L3 Support
Jean Delvare
Jul 29, 2015, 2:50:05 AM7/29/15
to
Le Tuesday 28 July 2015 à 10:46 +0100, Lee Jones a écrit :
> On Mon, 27 Jul 2015, Matt Fleming wrote:
> > + strcpy(pdata->name, info->name);
>
> strncpy() is safer.
And strlcpy() is even better.
> On Mon, 27 Jul 2015, Matt Fleming wrote:
> > + strcpy(pdata->name, info->name);
>
> strncpy() is safer.
--
Jean Delvare
SUSE L3 Support
Jean Delvare
Jul 29, 2015, 3:30:06 AM7/29/15
to
Hi Matt,
On Mon, 27 Jul 2015 14:38:08 +0100, Matt Fleming wrote:
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafde42cb..5336fe2ff689 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -797,7 +797,7 @@ config ITCO_WDT
> tristate "Intel TCO Timer/Watchdog"
> depends on (X86 || IA64) && PCI
> select WATCHDOG_CORE
> - select LPC_ICH
> + depends on LPC_ICH || I2C_I801
> ---help---
> Hardware driver for the intel TCO timer based watchdog devices.
> These drivers are included in the Intel 82801 I/O Controller
I don't like that. Depending on the order of the subsystems in the
Kconfig tree, the user may not see the option and has no clue that the
option exists. When he/she later enables LPC_ICH or I2C_I801, the
option becomes visible but he/she has no reason to return here to
enable it.
I can think of several ways to address the issue. One of them is to
select both drivers, as this is what most users will want anyway:
select LPC_ICH if !EXPERT
select I2C_I801 if !EXPERT
Another possibility is to make config ITCO_WDT always visible, and add
sub-options ITCO_WDT_LPC and ITCO_WDT_I2C which depend on it. Selecting
either would select the driver it depends on:
config ITCO_WDT_LPC
bool "Intel TCO Timer/Watchdog on LPC"
depends on ITCO_WDT
select LPC_ICH
config ITCO_WDT_I2C
bool "Intel TCO Timer/Watchdog on I2C"
depends on ITCO_WDT
select I2C_I801
I did not test that, some care might be needed due to tristate vs.
boolean.
I personally prefer the first approach. It may not be as clean as the
second approach but it should be good enough in practice and avoids
cluttering Kconfig with even more options.
On Mon, 27 Jul 2015 14:38:08 +0100, Matt Fleming wrote:
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 241fafde42cb..5336fe2ff689 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -797,7 +797,7 @@ config ITCO_WDT
> tristate "Intel TCO Timer/Watchdog"
> depends on (X86 || IA64) && PCI
> select WATCHDOG_CORE
> - select LPC_ICH
> + depends on LPC_ICH || I2C_I801
> ---help---
> Hardware driver for the intel TCO timer based watchdog devices.
> These drivers are included in the Intel 82801 I/O Controller
Kconfig tree, the user may not see the option and has no clue that the
option exists. When he/she later enables LPC_ICH or I2C_I801, the
option becomes visible but he/she has no reason to return here to
enable it.
I can think of several ways to address the issue. One of them is to
select both drivers, as this is what most users will want anyway:
select LPC_ICH if !EXPERT
select I2C_I801 if !EXPERT
Another possibility is to make config ITCO_WDT always visible, and add
sub-options ITCO_WDT_LPC and ITCO_WDT_I2C which depend on it. Selecting
either would select the driver it depends on:
config ITCO_WDT_LPC
bool "Intel TCO Timer/Watchdog on LPC"
depends on ITCO_WDT
select LPC_ICH
config ITCO_WDT_I2C
bool "Intel TCO Timer/Watchdog on I2C"
depends on ITCO_WDT
select I2C_I801
I did not test that, some care might be needed due to tristate vs.
boolean.
I personally prefer the first approach. It may not be as clean as the
second approach but it should be good enough in practice and avoids
cluttering Kconfig with even more options.
Lee Jones
Jul 29, 2015, 3:30:06 AM7/29/15
to
On Tue, 28 Jul 2015, Jean Delvare wrote:
> Le Tuesday 28 July 2015 à 10:46 +0100, Lee Jones a écrit :
> > On Mon, 27 Jul 2015, Matt Fleming wrote:
> > > + strcpy(pdata->name, info->name);
> >
> > strncpy() is safer.
>
> And strlcpy() is even better.
+1, thanks for that.
> Le Tuesday 28 July 2015 à 10:46 +0100, Lee Jones a écrit :
> > On Mon, 27 Jul 2015, Matt Fleming wrote:
> > > + strcpy(pdata->name, info->name);
> >
> > strncpy() is safer.
>
> And strlcpy() is even better.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones
Jul 29, 2015, 3:40:05 AM7/29/15
to
Under what circumstance would an error occur and you'd wish to carry
on registering devices?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones
Jul 29, 2015, 3:40:05 AM7/29/15
to
awkward (backward) Maintainers, who for some reason thing 'no change'
("don't touch it, it works"?) is better than 'improvement'.
If/when you come across it, a link would be appreciated.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Jean Delvare
Jul 29, 2015, 5:10:11 AM7/29/15
to
Hi Lee,
On Tue, 28 Jul 2015 18:32:16 +0100, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > >>Plus, of course, there is always the element that some maintainers hate
> > >>that kind of cleanup,
> > >
> > >Really? Surely any kind of clean-up is good clean-up. Especially as
> > >Greg KH et. al, have been doing public presentations telling everyone
> > >that there is always kernel work for anyone who has the time; spelling
> > >corrections and all.
> >
> > Yes, really. Just try to submit cleanup patches to maintainers other than
> > Greg and myself, and you'll see. It is a minefield.
>
> Admittedly some of us have our quirks, but I'm happy to challenge
> anyone that won't accept clean-up patches that make things better.
I may be one of these, to some degree, under certain circumstances.
The problem is that what you call "better" may not actually sound
better to me. Or it might be better in some respect but worse in others.
Specifically, your proposal to rename a kernel driver to remove
capitals, obviously has upsides, but it may also have downsides. Are
modprobe and friends case-insensitive? If not then renaming the module
that way may break existing "options" or "blacklist" statements
in /etc/modprobe.conf, or initialization scripts.
Such a change may also require some changes on the distribution side,
from a packaging perspective.
Likewise, renaming variables in the code makes it look better, but at
the cost of making future backports to that driver more difficult.
And if nothing else, the time you (or others) spend on this, is time
you won't spend somewhere else where it may be more useful. Or fun.
So in the end there's always a balance between the costs and the
benefits. Which may explain why sometimes some maintainers aren't so
interested in certain clean-up patches.
--
Jean Delvare
SUSE L3 Support
On Tue, 28 Jul 2015 18:32:16 +0100, Lee Jones wrote:
> On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > >>Plus, of course, there is always the element that some maintainers hate
> > >>that kind of cleanup,
> > >
> > >Really? Surely any kind of clean-up is good clean-up. Especially as
> > >Greg KH et. al, have been doing public presentations telling everyone
> > >that there is always kernel work for anyone who has the time; spelling
> > >corrections and all.
> >
> > Yes, really. Just try to submit cleanup patches to maintainers other than
> > Greg and myself, and you'll see. It is a minefield.
>
> Admittedly some of us have our quirks, but I'm happy to challenge
> anyone that won't accept clean-up patches that make things better.
The problem is that what you call "better" may not actually sound
better to me. Or it might be better in some respect but worse in others.
Specifically, your proposal to rename a kernel driver to remove
capitals, obviously has upsides, but it may also have downsides. Are
modprobe and friends case-insensitive? If not then renaming the module
that way may break existing "options" or "blacklist" statements
in /etc/modprobe.conf, or initialization scripts.
Such a change may also require some changes on the distribution side,
from a packaging perspective.
Likewise, renaming variables in the code makes it look better, but at
the cost of making future backports to that driver more difficult.
And if nothing else, the time you (or others) spend on this, is time
you won't spend somewhere else where it may be more useful. Or fun.
So in the end there's always a balance between the costs and the
benefits. Which may explain why sometimes some maintainers aren't so
interested in certain clean-up patches.
--
Jean Delvare
SUSE L3 Support
Lee Jones
Jul 29, 2015, 6:10:07 AM7/29/15
to
On Wed, 29 Jul 2015, Jean Delvare wrote:
> Hi Lee,
>
> On Tue, 28 Jul 2015 18:32:16 +0100, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > > >>Plus, of course, there is always the element that some maintainers hate
> > > >>that kind of cleanup,
> > > >
> > > >Really? Surely any kind of clean-up is good clean-up. Especially as
> > > >Greg KH et. al, have been doing public presentations telling everyone
> > > >that there is always kernel work for anyone who has the time; spelling
> > > >corrections and all.
> > >
> > > Yes, really. Just try to submit cleanup patches to maintainers other than
> > > Greg and myself, and you'll see. It is a minefield.
> >
> > Admittedly some of us have our quirks, but I'm happy to challenge
> > anyone that won't accept clean-up patches that make things better.
>
> I may be one of these, to some degree, under certain circumstances.
>
> The problem is that what you call "better" may not actually sound
> better to me. Or it might be better in some respect but worse in others.
Granted, there must always be a certain degree of common sense
> Hi Lee,
>
> On Tue, 28 Jul 2015 18:32:16 +0100, Lee Jones wrote:
> > On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > On 07/28/2015 08:28 AM, Lee Jones wrote:
> > > >On Tue, 28 Jul 2015, Guenter Roeck wrote:
> > > >>Sure, we could have changed it to lowercase, but so far no one bothered.
> > > >>Plus, of course, there is always the element that some maintainers hate
> > > >>that kind of cleanup,
> > > >
> > > >Really? Surely any kind of clean-up is good clean-up. Especially as
> > > >Greg KH et. al, have been doing public presentations telling everyone
> > > >that there is always kernel work for anyone who has the time; spelling
> > > >corrections and all.
> > >
> > > Yes, really. Just try to submit cleanup patches to maintainers other than
> > > Greg and myself, and you'll see. It is a minefield.
> >
> > Admittedly some of us have our quirks, but I'm happy to challenge
> > anyone that won't accept clean-up patches that make things better.
>
> I may be one of these, to some degree, under certain circumstances.
>
> The problem is that what you call "better" may not actually sound
> better to me. Or it might be better in some respect but worse in others.
involved.
> Specifically, your proposal to rename a kernel driver to remove
> capitals, obviously has upsides, but it may also have downsides. Are
> modprobe and friends case-insensitive? If not then renaming the module
> that way may break existing "options" or "blacklist" statements
> in /etc/modprobe.conf, or initialization scripts.
you're speaking from a distribution PoV, but you guys have to take
these actions into account as a matter of course. When you move to a
new kernel, you'll have teams who re-generate the aforementioned
files, or breakages would occur on every single release.
> Such a change may also require some changes on the distribution side,
> from a packaging perspective.
package maintainer at any given distro. How is this any different to
any other kernel up-level?
> Likewise, renaming variables in the code makes it look better, but at
> the cost of making future backports to that driver more difficult.
Looking at this example, I can see 4 backports that happened in the
last 4 years, and each of them would have been trivial to fix.
> And if nothing else, the time you (or others) spend on this, is time
> you won't spend somewhere else where it may be more useful. Or fun.
and my time is worthless. ;)
> So in the end there's always a balance between the costs and the
> benefits. Which may explain why sometimes some maintainers aren't so
> interested in certain clean-up patches.
I absolutely agree with you, but when I said 'better' before, I really
did mean BETTER.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Matt Fleming
Jul 29, 2015, 6:50:06 AM7/29/15
to
update, thanks.
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Jul 29, 2015, 7:20:06 AM7/29/15
to
On Wed, 29 Jul, at 09:29:34AM, Jean Delvare wrote:
> Hi Matt,
>
> On Mon, 27 Jul 2015 14:38:08 +0100, Matt Fleming wrote:
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 241fafde42cb..5336fe2ff689 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -797,7 +797,7 @@ config ITCO_WDT
> > tristate "Intel TCO Timer/Watchdog"
> > depends on (X86 || IA64) && PCI
> > select WATCHDOG_CORE
> > - select LPC_ICH
> > + depends on LPC_ICH || I2C_I801
> > ---help---
> > Hardware driver for the intel TCO timer based watchdog devices.
> > These drivers are included in the Intel 82801 I/O Controller
>
> I don't like that. Depending on the order of the subsystems in the
> Kconfig tree, the user may not see the option and has no clue that the
> option exists. When he/she later enables LPC_ICH or I2C_I801, the
> option becomes visible but he/she has no reason to return here to
> enable it.
Good point!
> Hi Matt,
>
> On Mon, 27 Jul 2015 14:38:08 +0100, Matt Fleming wrote:
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 241fafde42cb..5336fe2ff689 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -797,7 +797,7 @@ config ITCO_WDT
> > tristate "Intel TCO Timer/Watchdog"
> > depends on (X86 || IA64) && PCI
> > select WATCHDOG_CORE
> > - select LPC_ICH
> > + depends on LPC_ICH || I2C_I801
> > ---help---
> > Hardware driver for the intel TCO timer based watchdog devices.
> > These drivers are included in the Intel 82801 I/O Controller
>
> I don't like that. Depending on the order of the subsystems in the
> Kconfig tree, the user may not see the option and has no clue that the
> option exists. When he/she later enables LPC_ICH or I2C_I801, the
> option becomes visible but he/she has no reason to return here to
> enable it.
> I can think of several ways to address the issue. One of them is to
> select both drivers, as this is what most users will want anyway:
>
> select LPC_ICH if !EXPERT
> select I2C_I801 if !EXPERT
>
> Another possibility is to make config ITCO_WDT always visible, and add
> sub-options ITCO_WDT_LPC and ITCO_WDT_I2C which depend on it. Selecting
> either would select the driver it depends on:
>
> config ITCO_WDT_LPC
> bool "Intel TCO Timer/Watchdog on LPC"
> depends on ITCO_WDT
> select LPC_ICH
>
> config ITCO_WDT_I2C
> bool "Intel TCO Timer/Watchdog on I2C"
> depends on ITCO_WDT
> select I2C_I801
>
> I did not test that, some care might be needed due to tristate vs.
> boolean.
>
> I personally prefer the first approach. It may not be as clean as the
> second approach but it should be good enough in practice and avoids
> cluttering Kconfig with even more options.
because it doesn't require users to know which bus the TCO watchdog
device lives on for their platforms.
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Jul 29, 2015, 8:20:06 AM7/29/15
to
From: Matt Fleming <matt.f...@intel.com>
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
Changes in v2:
- Use lowercase for all new files and data structures
- Allocate platform data with devm_kzalloc()
- Move Kconfig changes to final patch and use 'select', not 'depends'
- Swap strcpy() for strlcpy()
- Fixup use of lpc_ich_info in intel_pmc_ipc which was missed in v1
- Don't use bitops in i2c-i801
- Remove superfluous NULL checks in i2c-i801
- Explicitly list reboot bit versions in no_reboot_bit()
- Use switch/case construst instead of crazy if else
- Fold original fixups from PATCH 4 and 5 into PATCH 1 and 2
Matt Fleming (2):
iTCO_wdt: Expose watchdog properties using platform data
iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 120 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/platform/x86/intel_pmc_ipc.c | 9 ++-
drivers/watchdog/Kconfig | 3 +-
drivers/watchdog/iTCO_wdt.c | 77 ++++++++++++---------
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/itco_wdt.h | 19 ++++++
7 files changed, 219 insertions(+), 47 deletions(-)
create mode 100644 include/linux/platform_data/itco_wdt.h
--
2.1.0
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
Changes in v2:
- Use lowercase for all new files and data structures
- Allocate platform data with devm_kzalloc()
- Move Kconfig changes to final patch and use 'select', not 'depends'
- Swap strcpy() for strlcpy()
- Fixup use of lpc_ich_info in intel_pmc_ipc which was missed in v1
- Don't use bitops in i2c-i801
- Remove superfluous NULL checks in i2c-i801
- Explicitly list reboot bit versions in no_reboot_bit()
- Use switch/case construst instead of crazy if else
- Fold original fixups from PATCH 4 and 5 into PATCH 1 and 2
Matt Fleming (2):
iTCO_wdt: Expose watchdog properties using platform data
iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 120 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/platform/x86/intel_pmc_ipc.c | 9 ++-
drivers/watchdog/Kconfig | 3 +-
drivers/watchdog/iTCO_wdt.c | 77 ++++++++++++---------
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/itco_wdt.h | 19 ++++++
7 files changed, 219 insertions(+), 47 deletions(-)
create mode 100644 include/linux/platform_data/itco_wdt.h
--
2.1.0
Matt Fleming
Jul 29, 2015, 8:20:06 AM7/29/15
to
From: Matt Fleming <matt.f...@intel.com>
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
This change introduces a new 'struct itco_wdt_platform_data' for use
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
This change is overdue because lpc_ich_info has already found its way
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
into other TCO watchdog users, notably the intel_pmc_ipc driver where
the watchdog actually isn't on the LPC bus as far as I can see.
A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.
Cc: Peter Tyser <pty...@xes-inc.com>
Cc: Samuel Ortiz <sa...@linux.intel.com>
Cc: Lee Jones <lee....@linaro.org>
Cc: Wim Van Sebroeck <w...@iguana.be>
Cc: Zha Qipeng <qipen...@intel.com>
Cc: Guenter Roeck <li...@roeck-us.net>
Cc: Jean Delvare <jdel...@suse.com>
v2:
- Use lowercase "itco" in all new files and data structures.
- Use devm_kzalloc() to allocate platform data, fixing a memory leak
and saving us from having to free the allocation in error paths
- Move stray Kconfig hunk to PATCH 3 that accidentally snuck into v1
- Swap strcpy() for the safer strlcpy()
- Fix lpc_ich_info user in the intel_pmc_ipc driver that was merged for
v4.2.
drivers/mfd/lpc_ich.c | 32 +++++++++++++++++++++++++++++---
drivers/platform/x86/intel_pmc_ipc.c | 9 ++++-----
drivers/watchdog/iTCO_wdt.c | 11 +++++------
include/linux/mfd/lpc_ich.h | 6 ------
include/linux/platform_data/itco_wdt.h | 19 +++++++++++++++++++
include/linux/mfd/lpc_ich.h | 6 ------
5 files changed, 57 insertions(+), 20 deletions(-)
create mode 100644 include/linux/platform_data/itco_wdt.h
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 8de34398abc0..c5a9a08b5dfb 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -66,6 +66,7 @@
#include <linux/pci.h>
#include <linux/mfd/core.h>
#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/itco_wdt.h>
+++ b/drivers/mfd/lpc_ich.c
@@ -66,6 +66,7 @@
#include <linux/pci.h>
#include <linux/mfd/core.h>
#include <linux/mfd/lpc_ich.h>
#define ACPIBASE 0x40
#define ACPIBASE_GPE_OFF 0x28
@@ -835,9 +836,31 @@ static void lpc_ich_enable_pmc_space(struct pci_dev *dev)
priv->actrl_pbase_save = reg_save;
}
-static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
+static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
{
struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+ struct lpc_ich_info *info;
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
+
+ pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
+ struct lpc_ich_info *info;
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
+
+ if (!pdata)
+ return -ENOMEM;
+
+ return -ENOMEM;
+ info = &lpc_chipset_info[priv->chipset];
+
+ pdata->version = info->iTCO_version;
+
+ strlcpy(pdata->name, info->name, sizeof(pdata->name));
+
+ cell->platform_data = pdata;
+ cell->pdata_size = sizeof(*pdata);
+ return 0;
+}
+
+static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
+{
+ struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
cell->platform_data = &lpc_chipset_info[priv->chipset];
cell->pdata_size = sizeof(struct lpc_ich_info);
+ cell->pdata_size = sizeof(*pdata);
+ return 0;
+}
+
+static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
+{
+ struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+ struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
cell->platform_data = &lpc_chipset_info[priv->chipset];
cell->pdata_size = sizeof(struct lpc_ich_info);
@@ -933,7 +956,7 @@ gpe0_done:
lpc_chipset_info[priv->chipset].use_gpio = ret;
lpc_ich_enable_gpio_space(dev);
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+ lpc_ich_finalize_gpio_cell(dev);
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
@@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
res->end = base_addr + ACPIBASE_PMC_END;
}
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+ ret = lpc_ich_finalize_wdt_cell(dev);
+ if (ret)
+ goto wdt_done;
+
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
lpc_chipset_info[priv->chipset].use_gpio = ret;
lpc_ich_enable_gpio_space(dev);
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+ lpc_ich_finalize_gpio_cell(dev);
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
@@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
res->end = base_addr + ACPIBASE_PMC_END;
}
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+ ret = lpc_ich_finalize_wdt_cell(dev);
+ if (ret)
+ goto wdt_done;
+
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
index 105cfffe82c6..28b2a12bb26d 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -33,7 +33,7 @@
#include <linux/suspend.h>
#include <linux/acpi.h>
#include <asm/intel_pmc_ipc.h>
-#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/itco_wdt.h>
/*
* IPC registers
@@ -473,9 +473,9 @@ static struct resource tco_res[] = {
},
};
-static struct lpc_ich_info tco_info = {
+static struct itco_wdt_platform_data tco_info = {
.name = "Apollo Lake SoC",
- .iTCO_version = 3,
+ .version = 3,
};
static int ipc_create_punit_device(void)
@@ -552,8 +552,7 @@ static int ipc_create_tco_device(void)
goto err;
}
- ret = platform_device_add_data(pdev, &tco_info,
- sizeof(struct lpc_ich_info));
+ ret = platform_device_add_data(pdev, &tco_info, sizeof(tco_info));
if (ret) {
dev_err(ipcdev.dev, "Failed to add tco platform data\n");
goto err;
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3c3fd417ddeb..a94401b2deca 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -66,8 +66,7 @@
#include <linux/spinlock.h> /* For spin_lock/spin_unlock/... */
#include <linux/uaccess.h> /* For copy_to_user/put_user/... */
#include <linux/io.h> /* For inb/outb/... */
-#include <linux/mfd/core.h>
-#include <linux/mfd/lpc_ich.h>
+#include <linux/platform_data/itco_wdt.h>
#include "iTCO_vendor.h"
@@ -418,9 +417,9 @@ static int iTCO_wdt_probe(struct platform_device *dev)
{
int ret = -ENODEV;
unsigned long val32;
- struct lpc_ich_info *ich_info = dev_get_platdata(&dev->dev);
+ struct itco_wdt_platform_data *pdata = dev_get_platdata(&dev->dev);
- if (!ich_info)
+ if (!pdata)
goto out;
spin_lock_init(&iTCO_wdt_private.io_lock);
@@ -435,7 +434,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
if (!iTCO_wdt_private.smi_res)
goto out;
- iTCO_wdt_private.iTCO_version = ich_info->iTCO_version;
+ iTCO_wdt_private.iTCO_version = pdata->version;
iTCO_wdt_private.dev = dev;
iTCO_wdt_private.pdev = to_pci_dev(dev->dev.parent);
@@ -501,7 +500,7 @@ static int iTCO_wdt_probe(struct platform_device *dev)
}
pr_info("Found a %s TCO device (Version=%d, TCOBASE=0x%04llx)\n",
- ich_info->name, ich_info->iTCO_version, (u64)TCOBASE);
+ pdata->name, pdata->version, (u64)TCOBASE);
/* Clear out the (probably old) status */
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 8feac782fa83..2b300b44f994 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -20,12 +20,6 @@
#ifndef LPC_ICH_H
#define LPC_ICH_H
-/* Watchdog resources */
-#define ICH_RES_IO_TCO 0
-#define ICH_RES_IO_SMI 1
-#define ICH_RES_MEM_OFF 2
-#define ICH_RES_MEM_GCS_PMC 0
-
/* GPIO resources */
#define ICH_RES_GPIO 0
#define ICH_RES_GPE0 1
diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
new file mode 100644
index 000000000000..f16542c77ff7
--- /dev/null
+++ b/include/linux/platform_data/itco_wdt.h
@@ -0,0 +1,19 @@
+/*
+ * Platform data for the Intel TCO Watchdog
+ */
+
+#ifndef _ITCO_WDT_H_
+#define _ITCO_WDT_H_
+
+/* Watchdog resources */
+#define ICH_RES_IO_TCO 0
+#define ICH_RES_IO_SMI 1
+#define ICH_RES_MEM_OFF 2
+#define ICH_RES_MEM_GCS_PMC 0
+
+struct itco_wdt_platform_data {
+ char name[32];
+ unsigned int version;
+};
+
+#endif /* _ITCO_WDT_H_ */
Matt Fleming
Jul 29, 2015, 8:20:07 AM7/29/15
to
From: Mika Westerberg <mika.we...@linux.intel.com>
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Cc: <linu...@vger.kernel.org>
Cc: Guenter Roeck <li...@roeck-us.net>
Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
- Don't use bitops but same scheme used already
- Delete superfluous NULL-checks for platform_device_unregister()
drivers/i2c/busses/i2c-i801.c | 120 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 119 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3fdc27e..eaef9bc9d88c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -88,12 +88,13 @@
#include <linux/slab.h>
#include <linux/wait.h>
#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/itco_wdt.h>
#if (defined CONFIG_I2C_MUX_GPIO || defined CONFIG_I2C_MUX_GPIO_MODULE) && \
defined CONFIG_DMI
#include <linux/gpio.h>
#include <linux/i2c-mux-gpio.h>
-#include <linux/platform_device.h>
#endif
/* I801 SMBus address offsets */
@@ -113,6 +114,16 @@
#define SMBPCICTL 0x004
#define SMBPCISTS 0x006
#define SMBHSTCFG 0x040
+#define TCOBASE 0x050
+#define TCOCTL 0x054
+
+#define ACPIBASE 0x040
+#define ACPIBASE_SMI_OFF 0x030
+#define ACPICTRL 0x044
+#define ACPICTRL_EN 0x080
+
+#define SBREG_BAR 0x10
+#define SBREG_SMBCTRL 0xc6000c
/* Host status bits for SMBPCISTS */
#define SMBPCISTS_INTS 0x08
@@ -125,6 +136,9 @@
#define SMBHSTCFG_SMB_SMI_EN 2
#define SMBHSTCFG_I2C_EN 4
+/* TCO configuration bits for TCOCTL */
+#define TCOCTL_EN 0x0100
+
/* Auxiliary control register bits, ICH4+ only */
#define SMBAUXCTL_CRC 1
#define SMBAUXCTL_E32B 2
@@ -221,6 +235,7 @@ struct i801_priv {
const struct i801_mux_config *mux_drvdata;
struct platform_device *mux_pdev;
#endif
+ struct platform_device *tco_pdev;
};
#define FEATURE_SMBUS_PEC (1 << 0)
@@ -230,6 +245,7 @@ struct i801_priv {
#define FEATURE_IRQ (1 << 4)
/* Not really a feature, but it's convenient to handle it as such */
#define FEATURE_IDF (1 << 15)
+#define FEATURE_TCO (1 << 16)
static const char *i801_feature_names[] = {
"SMBus PEC",
@@ -1132,6 +1148,95 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
}
#endif
+static const struct itco_wdt_platform_data tco_platform_data = {
+ .name = "Intel PCH",
+ .version = 4,
+};
+
+static DEFINE_SPINLOCK(p2sb_spinlock);
+
+static void i801_add_tco(struct i801_priv *priv)
+{
+ struct pci_dev *pci_dev = priv->pci_dev;
+ struct resource tco_res[3], *res;
+ struct platform_device *pdev;
+ unsigned int devfn;
+ u32 tco_base, tco_ctl;
+ u32 base_addr, ctrl_val;
+ u64 base64_addr;
+
+ if (!(priv->features & FEATURE_TCO))
+ return;
+
+ pci_read_config_dword(pci_dev, TCOBASE, &tco_base);
+ pci_read_config_dword(pci_dev, TCOCTL, &tco_ctl);
+ if (!(tco_ctl & TCOCTL_EN))
+ return;
+
+ memset(tco_res, 0, sizeof(tco_res));
+
+ res = &tco_res[ICH_RES_IO_TCO];
+ res->start = tco_base & ~1;
+ res->end = res->start + 32 - 1;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Power Management registers.
+ */
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 2);
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPIBASE, &base_addr);
+
+ res = &tco_res[ICH_RES_IO_SMI];
+ res->start = (base_addr & ~1) + ACPIBASE_SMI_OFF;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Enable the ACPI I/O space.
+ */
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPICTRL, &ctrl_val);
+ ctrl_val |= ACPICTRL_EN;
+ pci_bus_write_config_dword(pci_dev->bus, devfn, ACPICTRL, ctrl_val);
+
+ /*
+ * We must access the NO_REBOOT bit over the Primary to Sideband
+ * bridge (P2SB). The BIOS prevents the P2SB device from being
+ * enumerated by the PCI subsystem, so we need to unhide/hide it
+ * to lookup the P2SB BAR.
+ */
+ spin_lock(&p2sb_spinlock);
+
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
+
+ /* Unhide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
+ base64_addr = base_addr & 0xfffffff0;
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
+ base64_addr |= (u64)base_addr << 32;
+
+ /* Hide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x1);
+ spin_unlock(&p2sb_spinlock);
+
+ res = &tco_res[ICH_RES_MEM_OFF];
+ res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_MEM;
+
+ pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+ tco_res, 3, &tco_platform_data,
+ sizeof(tco_platform_data));
+ if (IS_ERR(pdev)) {
+ dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
+ return;
+ }
+
+ priv->tco_pdev = pdev;
+}
+
static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
unsigned char temp;
@@ -1149,6 +1254,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
priv->pci_dev = dev;
switch (dev->device) {
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
+ priv->features |= FEATURE_I2C_BLOCK_READ;
+ priv->features |= FEATURE_IRQ;
+ priv->features |= FEATURE_SMBUS_PEC;
+ priv->features |= FEATURE_BLOCK_BUFFER;
+ priv->features |= FEATURE_TCO;
+ break;
+
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2:
@@ -1265,6 +1379,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
dev_info(&dev->dev, "SMBus using %s\n",
priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
+ i801_add_tco(priv);
+
/* set up the sysfs linkage to our parent device */
priv->adapter.dev.parent = &dev->dev;
@@ -1296,6 +1412,8 @@ static void i801_remove(struct pci_dev *dev)
i2c_del_adapter(&priv->adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+ platform_device_unregister(priv->tco_pdev);
+
/*
* do not call pci_disable_device(dev) since it can cause hard hangs on
* some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Cc: <linu...@vger.kernel.org>
Cc: Guenter Roeck <li...@roeck-us.net>
Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
- Don't use bitops but same scheme used already
- Delete superfluous NULL-checks for platform_device_unregister()
drivers/i2c/busses/i2c-i801.c | 120 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 119 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ecbb3fdc27e..eaef9bc9d88c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -88,12 +88,13 @@
#include <linux/slab.h>
#include <linux/wait.h>
#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/itco_wdt.h>
#if (defined CONFIG_I2C_MUX_GPIO || defined CONFIG_I2C_MUX_GPIO_MODULE) && \
defined CONFIG_DMI
#include <linux/gpio.h>
#include <linux/i2c-mux-gpio.h>
-#include <linux/platform_device.h>
#endif
/* I801 SMBus address offsets */
@@ -113,6 +114,16 @@
#define SMBPCICTL 0x004
#define SMBPCISTS 0x006
#define SMBHSTCFG 0x040
+#define TCOBASE 0x050
+#define TCOCTL 0x054
+
+#define ACPIBASE 0x040
+#define ACPIBASE_SMI_OFF 0x030
+#define ACPICTRL 0x044
+#define ACPICTRL_EN 0x080
+
+#define SBREG_BAR 0x10
+#define SBREG_SMBCTRL 0xc6000c
/* Host status bits for SMBPCISTS */
#define SMBPCISTS_INTS 0x08
@@ -125,6 +136,9 @@
#define SMBHSTCFG_SMB_SMI_EN 2
#define SMBHSTCFG_I2C_EN 4
+/* TCO configuration bits for TCOCTL */
+#define TCOCTL_EN 0x0100
+
/* Auxiliary control register bits, ICH4+ only */
#define SMBAUXCTL_CRC 1
#define SMBAUXCTL_E32B 2
@@ -221,6 +235,7 @@ struct i801_priv {
const struct i801_mux_config *mux_drvdata;
struct platform_device *mux_pdev;
#endif
+ struct platform_device *tco_pdev;
};
#define FEATURE_SMBUS_PEC (1 << 0)
@@ -230,6 +245,7 @@ struct i801_priv {
#define FEATURE_IRQ (1 << 4)
/* Not really a feature, but it's convenient to handle it as such */
#define FEATURE_IDF (1 << 15)
+#define FEATURE_TCO (1 << 16)
static const char *i801_feature_names[] = {
"SMBus PEC",
@@ -1132,6 +1148,95 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
}
#endif
+static const struct itco_wdt_platform_data tco_platform_data = {
+ .name = "Intel PCH",
+ .version = 4,
+};
+
+static DEFINE_SPINLOCK(p2sb_spinlock);
+
+static void i801_add_tco(struct i801_priv *priv)
+{
+ struct pci_dev *pci_dev = priv->pci_dev;
+ struct resource tco_res[3], *res;
+ struct platform_device *pdev;
+ unsigned int devfn;
+ u32 tco_base, tco_ctl;
+ u32 base_addr, ctrl_val;
+ u64 base64_addr;
+
+ if (!(priv->features & FEATURE_TCO))
+ return;
+
+ pci_read_config_dword(pci_dev, TCOBASE, &tco_base);
+ pci_read_config_dword(pci_dev, TCOCTL, &tco_ctl);
+ if (!(tco_ctl & TCOCTL_EN))
+ return;
+
+ memset(tco_res, 0, sizeof(tco_res));
+
+ res = &tco_res[ICH_RES_IO_TCO];
+ res->start = tco_base & ~1;
+ res->end = res->start + 32 - 1;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Power Management registers.
+ */
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 2);
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPIBASE, &base_addr);
+
+ res = &tco_res[ICH_RES_IO_SMI];
+ res->start = (base_addr & ~1) + ACPIBASE_SMI_OFF;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_IO;
+
+ /*
+ * Enable the ACPI I/O space.
+ */
+ pci_bus_read_config_dword(pci_dev->bus, devfn, ACPICTRL, &ctrl_val);
+ ctrl_val |= ACPICTRL_EN;
+ pci_bus_write_config_dword(pci_dev->bus, devfn, ACPICTRL, ctrl_val);
+
+ /*
+ * We must access the NO_REBOOT bit over the Primary to Sideband
+ * bridge (P2SB). The BIOS prevents the P2SB device from being
+ * enumerated by the PCI subsystem, so we need to unhide/hide it
+ * to lookup the P2SB BAR.
+ */
+ spin_lock(&p2sb_spinlock);
+
+ devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
+
+ /* Unhide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
+ base64_addr = base_addr & 0xfffffff0;
+
+ pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
+ base64_addr |= (u64)base_addr << 32;
+
+ /* Hide the P2SB device */
+ pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x1);
+ spin_unlock(&p2sb_spinlock);
+
+ res = &tco_res[ICH_RES_MEM_OFF];
+ res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
+ res->end = res->start + 3;
+ res->flags = IORESOURCE_MEM;
+
+ pdev = platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+ tco_res, 3, &tco_platform_data,
+ sizeof(tco_platform_data));
+ if (IS_ERR(pdev)) {
+ dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
+ return;
+ }
+
+ priv->tco_pdev = pdev;
+}
+
static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
unsigned char temp;
@@ -1149,6 +1254,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
priv->pci_dev = dev;
switch (dev->device) {
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
+ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
+ priv->features |= FEATURE_I2C_BLOCK_READ;
+ priv->features |= FEATURE_IRQ;
+ priv->features |= FEATURE_SMBUS_PEC;
+ priv->features |= FEATURE_BLOCK_BUFFER;
+ priv->features |= FEATURE_TCO;
+ break;
+
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1:
case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2:
@@ -1265,6 +1379,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
dev_info(&dev->dev, "SMBus using %s\n",
priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
+ i801_add_tco(priv);
+
/* set up the sysfs linkage to our parent device */
priv->adapter.dev.parent = &dev->dev;
@@ -1296,6 +1412,8 @@ static void i801_remove(struct pci_dev *dev)
i2c_del_adapter(&priv->adapter);
pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+ platform_device_unregister(priv->tco_pdev);
+
/*
* do not call pci_disable_device(dev) since it can cause hard hangs on
* some systems during power-off (eg. Fujitsu-Siemens Lifebook E8010)
Matt Fleming
Jul 29, 2015, 8:20:07 AM7/29/15
to
From: Matt Fleming <matt.f...@intel.com>
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
Also update the Kconfig entry to select both the LPC and SMBus drivers
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
since the TCO device is on the SMBus in Sunrisepoint.
- Explicitly list TCO versions and their "no reboot" bits in the switch
statement in no_reboot_bit()
- Use a switch/case statement when clearing status bits instead of
convoluted if/else branching
- Select both of the bus drivers instead of using depends
drivers/watchdog/Kconfig | 3 ++-
drivers/watchdog/iTCO_wdt.c | 66 ++++++++++++++++++++++++++++-----------------
2 files changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..55c4b5b0a317 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -797,7 +797,8 @@ config ITCO_WDT
tristate "Intel TCO Timer/Watchdog"
depends on (X86 || IA64) && PCI
select WATCHDOG_CORE
- select LPC_ICH
+ select LPC_ICH if !EXPERT
depends on (X86 || IA64) && PCI
select WATCHDOG_CORE
- select LPC_ICH
+ select I2C_I801 if !EXPERT
---help---
Hardware driver for the intel TCO timer based watchdog devices.
These drivers are included in the Intel 82801 I/O Controller
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
Hardware driver for the intel TCO timer based watchdog devices.
These drivers are included in the Intel 82801 I/O Controller
index a94401b2deca..7b8949d48029 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -145,58 +145,67 @@ static inline unsigned int ticks_to_seconds(int ticks)
return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 6) / 10;
}
+static inline u32 no_reboot_bit(void)
+{
+ u32 enable_bit;
+
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 3:
+ enable_bit = 0x00000010;
+ break;
+ case 2:
+ enable_bit = 0x00000020;
+ break;
+ case 4:
+ case 1:
+ default:
+ enable_bit = 0x00000002;
+ break;
+ }
+
+ return enable_bit;
+}
+
static void iTCO_wdt_set_NO_REBOOT_bit(void)
{
u32 val32;
/* Set the NO_REBOOT bit: this disables reboots */
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000010;
- writel(val32, iTCO_wdt_private.gcs_pmc);
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000020;
+ val32 |= no_reboot_bit();
writel(val32, iTCO_wdt_private.gcs_pmc);
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 |= 0x00000002;
+ val32 |= no_reboot_bit();
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
}
}
static int iTCO_wdt_unset_NO_REBOOT_bit(void)
{
+ u32 enable_bit = no_reboot_bit();
int ret = 0;
- u32 val32;
+ u32 val32 = 0;
/* Unset the NO_REBOOT bit: this enables reboots */
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffef;
- writel(val32, iTCO_wdt_private.gcs_pmc);
-
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000010)
- ret = -EIO;
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffdf;
+ val32 &= ~enable_bit;
writel(val32, iTCO_wdt_private.gcs_pmc);
val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000020)
- ret = -EIO;
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 &= 0xfffffffd;
+ val32 &= ~enable_bit;
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- if (val32 & 0x00000002)
- ret = -EIO;
}
+ if (val32 & enable_bit)
+ ret = -EIO;
+
return ret; /* returns: 0 = OK, -EIO = Error */
}
@@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct platform_device *dev)
pdata->name, pdata->version, (u64)TCOBASE);
/* Clear out the (probably old) status */
/* Clear out the (probably old) status */
- if (iTCO_wdt_private.iTCO_version == 3) {
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 4:
+ outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ break;
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ case 3:
outl(0x20008, TCO1_STS);
- } else {
+ break;
+ default:
outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
outw(0x0004, TCO2_STS); /* Clear BOOT_STS bit */
+ break;
}
iTCO_wdt_watchdog_dev.bootstatus = 0;
Aaron Sierra
Jul 29, 2015, 11:00:07 AM7/29/15
to
> From: "Lee Jones" <lee....@linaro.org>
> Sent: Wednesday, July 29, 2015 2:38:41 AM
Lee,
The two devices that this driver is responsible for are conceptually
independent; they simply are lumped together in one PCI device. No
failure while preparing resources for the watchdog device should
prevent the GPIO device from being registered.
The most common real world circumstance that I experience is when a
BIOS reserves resources associated with the GPIO device, thus
preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
being fully prepared.
I have not experienced issues with the watchdog device, but a similar
issue would exist if the RCBA were disabled in a "v2" device.
It seems like a dangerous change to simply attempt to register both
of these devices with a single call, when one or both of them could
be incomplete.
Perhaps your real issue with this driver structure is that these
cells are elements of a single lpc_ich_cells array for no clear
reason. If each had a dedicated mfd_cell variable, would that be
more acceptable to you?
-static struct mfd_cell lpc_ich_cells[] = {
+static struct mfd_cell lpc_ich_wdt_cell = {
...
+static struct mfd_cell lpc_ich_gpio_cell = {
That would eliminate the need for the lpc_cells enum, too.
-Aaron S.
> Sent: Wednesday, July 29, 2015 2:38:41 AM
The two devices that this driver is responsible for are conceptually
independent; they simply are lumped together in one PCI device. No
failure while preparing resources for the watchdog device should
prevent the GPIO device from being registered.
The most common real world circumstance that I experience is when a
BIOS reserves resources associated with the GPIO device, thus
preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
being fully prepared.
I have not experienced issues with the watchdog device, but a similar
issue would exist if the RCBA were disabled in a "v2" device.
It seems like a dangerous change to simply attempt to register both
of these devices with a single call, when one or both of them could
be incomplete.
Perhaps your real issue with this driver structure is that these
cells are elements of a single lpc_ich_cells array for no clear
reason. If each had a dedicated mfd_cell variable, would that be
more acceptable to you?
-static struct mfd_cell lpc_ich_cells[] = {
+static struct mfd_cell lpc_ich_wdt_cell = {
...
+static struct mfd_cell lpc_ich_gpio_cell = {
That would eliminate the need for the lpc_cells enum, too.
-Aaron S.
Lee Jones
Jul 29, 2015, 11:40:06 AM7/29/15
to
Acked-by: Lee Jones <lee....@linaro.org>
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones
Jul 29, 2015, 11:40:06 AM7/29/15
to
Perhaps I should invest some time to looking into that.
> The most common real world circumstance that I experience is when a
> BIOS reserves resources associated with the GPIO device, thus
> preventing the GPIO resources (ICH_RES_GPE0 and/or ICH_RES_GPIO) from
> being fully prepared.
>
> I have not experienced issues with the watchdog device, but a similar
> issue would exist if the RCBA were disabled in a "v2" device.
>
> It seems like a dangerous change to simply attempt to register both
> of these devices with a single call, when one or both of them could
> be incomplete.
>
> Perhaps your real issue with this driver structure is that these
> cells are elements of a single lpc_ich_cells array for no clear
> reason. If each had a dedicated mfd_cell variable, would that be
> more acceptable to you?
>
> -static struct mfd_cell lpc_ich_cells[] = {
> +static struct mfd_cell lpc_ich_wdt_cell = {
> ...
> +static struct mfd_cell lpc_ich_gpio_cell = {
>
> That would eliminate the need for the lpc_cells enum, too.
instead of mfd_add_devices(), as you are only attempting registration
for a single device.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Guenter Roeck
Jul 29, 2015, 12:00:07 PM7/29/15
to
bind two drivers to the same pci device, which is not currently
possible in Linux. How would you suggest to do that if not with
an mfd driver ?
Thanks,
Guenter
Aaron Sierra
Jul 29, 2015, 12:30:06 PM7/29/15
to
> From: "Lee Jones" <lee....@linaro.org>
> Sent: Wednesday, July 29, 2015 10:32:26 AM
I can submit a patch the splits up the array elements, but I
only see mfd_add_device() as a static function in mfd-core.c;
Is that being exported in a development branch somewhere?
-Aaron S.
> Sent: Wednesday, July 29, 2015 10:32:26 AM
only see mfd_add_device() as a static function in mfd-core.c;
Is that being exported in a development branch somewhere?
-Aaron S.
Guenter Roeck
Jul 29, 2015, 12:40:06 PM7/29/15
to
handling into the calling driver, and also provide mfd_remove_device().
Guenter
Matt Fleming
Jul 29, 2015, 12:50:06 PM7/29/15
to
On Wed, 29 Jul, at 04:35:56PM, Lee Jones wrote:
>
> For my own reference (I assume this will go through the MFD tree):
> Acked-by: Lee Jones <lee....@linaro.org>
Great, thanks Lee!
>
> For my own reference (I assume this will go through the MFD tree):
> Acked-by: Lee Jones <lee....@linaro.org>
I've no strong opinion on which tree these 3 patches go through but at
the very least patches 1 and 2 need to go through the same tree to avoid
build breakage (since PATCH 1 introduces 'struct itco_wdt_platform_data'
which is used in PATCH 2).
For reference, the other patches are here,
https://lkml.kernel.org/r/1438171844-24861-3-...@codeblueprint.co.uk
https://lkml.kernel.org/r/1438171844-24861-4-...@codeblueprint.co.uk
--
Matt Fleming, Intel Open Source Technology Center
Aaron Sierra
Jul 29, 2015, 1:10:06 PM7/29/15
to
-Aaron S.
Guenter Roeck
Jul 29, 2015, 1:10:06 PM7/29/15
to
On 07/29/2015 05:10 AM, Matt Fleming wrote:
> From: Matt Fleming <matt.f...@intel.com>
>
> The revision of the watchdog hardware in Sunrisepoint necessitates a new
> "version" inside the TCO watchdog driver because some of the register
> layouts have changed.
>
> Also update the Kconfig entry to select both the LPC and SMBus drivers
> since the TCO device is on the SMBus in Sunrisepoint.
>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Cc: Jean Delvare <jdel...@suse.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> From: Matt Fleming <matt.f...@intel.com>
>
> The revision of the watchdog hardware in Sunrisepoint necessitates a new
> "version" inside the TCO watchdog driver because some of the register
> layouts have changed.
>
> Also update the Kconfig entry to select both the LPC and SMBus drivers
> since the TCO device is on the SMBus in Sunrisepoint.
>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Cc: Jean Delvare <jdel...@suse.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Guenter Roeck
Jul 29, 2015, 1:10:07 PM7/29/15
to
On 07/29/2015 05:10 AM, Matt Fleming wrote:
> From: Mika Westerberg <mika.we...@linux.intel.com>
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Cc: <linu...@vger.kernel.org>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Cc: <linu...@vger.kernel.org>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Lee Jones
Jul 30, 2015, 5:00:06 AM7/30/15
to
On Wed, 29 Jul 2015, Matt Fleming wrote:
> On Wed, 29 Jul, at 04:35:56PM, Lee Jones wrote:
> >
> > For my own reference (I assume this will go through the MFD tree):
> > Acked-by: Lee Jones <lee....@linaro.org>
>
> Great, thanks Lee!
>
> I've no strong opinion on which tree these 3 patches go through but at
> the very least patches 1 and 2 need to go through the same tree to avoid
> build breakage (since PATCH 1 introduces 'struct itco_wdt_platform_data'
> which is used in PATCH 2).
>
> For reference, the other patches are here,
>
> https://lkml.kernel.org/r/1438171844-24861-3-...@codeblueprint.co.uk
> https://lkml.kernel.org/r/1438171844-24861-4-...@codeblueprint.co.uk
Obviously this makes things more difficult. Once you have the other
> >
> > For my own reference (I assume this will go through the MFD tree):
> > Acked-by: Lee Jones <lee....@linaro.org>
>
> Great, thanks Lee!
>
> I've no strong opinion on which tree these 3 patches go through but at
> the very least patches 1 and 2 need to go through the same tree to avoid
> build breakage (since PATCH 1 introduces 'struct itco_wdt_platform_data'
> which is used in PATCH 2).
>
> For reference, the other patches are here,
>
> https://lkml.kernel.org/r/1438171844-24861-3-...@codeblueprint.co.uk
> https://lkml.kernel.org/r/1438171844-24861-4-...@codeblueprint.co.uk
Acks you need, I suggest you re-submit the set using --threaded, so we
can all see what's going on.
FYI: I usually do this as a matter of course, as separated patches are
a pain to manage, particularly if there are dependencies between them.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones
Jul 30, 2015, 5:00:06 AM7/30/15
to
we have of managing these devices in Linux.
Or perhaps I was just trying to provoke some thought/discussion. ;)
The MFD driver for this device looks fairly well written, so I'm not
offended that it's located there. On the flip side, I am sensitive to
MFD becoming (more of?) a dumping ground for misfits that just don't
belong anywhere else.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Matt Fleming
Jul 30, 2015, 5:20:06 AM7/30/15
to
On Thu, 30 Jul, at 09:53:47AM, Lee Jones wrote:
> On Wed, 29 Jul 2015, Matt Fleming wrote:
>
> > On Wed, 29 Jul, at 04:35:56PM, Lee Jones wrote:
> > >
> > > For my own reference (I assume this will go through the MFD tree):
> > > Acked-by: Lee Jones <lee....@linaro.org>
> >
> > Great, thanks Lee!
> >
> > I've no strong opinion on which tree these 3 patches go through but at
> > the very least patches 1 and 2 need to go through the same tree to avoid
> > build breakage (since PATCH 1 introduces 'struct itco_wdt_platform_data'
> > which is used in PATCH 2).
> >
> > For reference, the other patches are here,
> >
> > https://lkml.kernel.org/r/1438171844-24861-3-...@codeblueprint.co.uk
> > https://lkml.kernel.org/r/1438171844-24861-4-...@codeblueprint.co.uk
>
> Obviously this makes things more difficult. Once you have the other
> Acks you need, I suggest you re-submit the set using --threaded, so we
> can all see what's going on.
Umm... I submitted *this* version using --threaded. In fact, that's
> On Wed, 29 Jul 2015, Matt Fleming wrote:
>
> > On Wed, 29 Jul, at 04:35:56PM, Lee Jones wrote:
> > >
> > > For my own reference (I assume this will go through the MFD tree):
> > > Acked-by: Lee Jones <lee....@linaro.org>
> >
> > Great, thanks Lee!
> >
> > I've no strong opinion on which tree these 3 patches go through but at
> > the very least patches 1 and 2 need to go through the same tree to avoid
> > build breakage (since PATCH 1 introduces 'struct itco_wdt_platform_data'
> > which is used in PATCH 2).
> >
> > For reference, the other patches are here,
> >
> > https://lkml.kernel.org/r/1438171844-24861-3-...@codeblueprint.co.uk
> > https://lkml.kernel.org/r/1438171844-24861-4-...@codeblueprint.co.uk
>
> Obviously this makes things more difficult. Once you have the other
> Acks you need, I suggest you re-submit the set using --threaded, so we
> can all see what's going on.
plain to see from the Message-Id format in the links above. It's just
that I only Cc'd you on the patch that touched the mfd code.
I can resubmit with the Ack/Review tags and copy you on the whole series
if you'd like.
--
Matt Fleming, Intel Open Source Technology Center
Lee Jones
Jul 30, 2015, 7:10:07 AM7/30/15
to
On Thu, 30 Jul 2015, Matt Fleming wrote:
> On Thu, 30 Jul, at 09:53:47AM, Lee Jones wrote:
> > On Wed, 29 Jul 2015, Matt Fleming wrote:
> >
> > > On Wed, 29 Jul, at 04:35:56PM, Lee Jones wrote:
> > > >
> > > > For my own reference (I assume this will go through the MFD tree):
> > > > Acked-by: Lee Jones <lee....@linaro.org>
> > >
> > > Great, thanks Lee!
> > >
> > > I've no strong opinion on which tree these 3 patches go through but at
> > > the very least patches 1 and 2 need to go through the same tree to avoid
> > > build breakage (since PATCH 1 introduces 'struct itco_wdt_platform_data'
> > > which is used in PATCH 2).
> > >
> > > For reference, the other patches are here,
> > >
> > > https://lkml.kernel.org/r/1438171844-24861-3-...@codeblueprint.co.uk
> > > https://lkml.kernel.org/r/1438171844-24861-4-...@codeblueprint.co.uk
> >
> > Obviously this makes things more difficult. Once you have the other
> > Acks you need, I suggest you re-submit the set using --threaded, so we
> > can all see what's going on.
>
> Umm... I submitted *this* version using --threaded. In fact, that's
> plain to see from the Message-Id format in the links above. It's just
> that I only Cc'd you on the patch that touched the mfd code.
Ah yes, similar result. Again, unless the set is particularlly large
> On Thu, 30 Jul, at 09:53:47AM, Lee Jones wrote:
> > On Wed, 29 Jul 2015, Matt Fleming wrote:
> >
> > > On Wed, 29 Jul, at 04:35:56PM, Lee Jones wrote:
> > > >
> > > > For my own reference (I assume this will go through the MFD tree):
> > > > Acked-by: Lee Jones <lee....@linaro.org>
> > >
> > > Great, thanks Lee!
> > >
> > > I've no strong opinion on which tree these 3 patches go through but at
> > > the very least patches 1 and 2 need to go through the same tree to avoid
> > > build breakage (since PATCH 1 introduces 'struct itco_wdt_platform_data'
> > > which is used in PATCH 2).
> > >
> > > For reference, the other patches are here,
> > >
> > > https://lkml.kernel.org/r/1438171844-24861-3-...@codeblueprint.co.uk
> > > https://lkml.kernel.org/r/1438171844-24861-4-...@codeblueprint.co.uk
> >
> > Obviously this makes things more difficult. Once you have the other
> > Acks you need, I suggest you re-submit the set using --threaded, so we
> > can all see what's going on.
>
> Umm... I submitted *this* version using --threaded. In fact, that's
> plain to see from the Message-Id format in the links above. It's just
> that I only Cc'd you on the patch that touched the mfd code.
or touches many subsystems, I tend to Cc all affected Maintainers (if
I'm sending, or like to be Cc'ed if I'm the recipient) on all of the
patches. Set status is much easier to track that way and issues which
might be relevant to more than one patch in the set can be seen by
all.
> I can resubmit with the Ack/Review tags and copy you on the whole series
> if you'd like.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Matt Fleming
Jul 30, 2015, 6:00:07 PM7/30/15
to
From: Matt Fleming <matt.f...@intel.com>
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
Also update the Kconfig entry to select both the LPC and SMBus drivers
since the TCO device is on the SMBus in Sunrisepoint.
Cc: Wim Van Sebroeck <w...@iguana.be>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
Also update the Kconfig entry to select both the LPC and SMBus drivers
since the TCO device is on the SMBus in Sunrisepoint.
Cc: Wim Van Sebroeck <w...@iguana.be>
---
v3:
- Added Guenter's Review tag
v2:
- Explicitly list TCO versions and their "no reboot" bits in the switch
statement in no_reboot_bit()
- Use a switch/case statement when clearing status bits instead of
convoluted if/else branching
- Select both of the bus drivers instead of using depends
drivers/watchdog/Kconfig | 3 ++-
drivers/watchdog/iTCO_wdt.c | 66 ++++++++++++++++++++++++++++-----------------
2 files changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..55c4b5b0a317 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -797,7 +797,8 @@ config ITCO_WDT
tristate "Intel TCO Timer/Watchdog"
depends on (X86 || IA64) && PCI
select WATCHDOG_CORE
- select LPC_ICH
+ select LPC_ICH if !EXPERT
+ select I2C_I801 if !EXPERT
---help---
Hardware driver for the intel TCO timer based watchdog devices.
These drivers are included in the Intel 82801 I/O Controller
index a94401b2deca..7b8949d48029 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
pdata->name, pdata->version, (u64)TCOBASE);
/* Clear out the (probably old) status */
/* Clear out the (probably old) status */
- if (iTCO_wdt_private.iTCO_version == 3) {
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 4:
+ outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ break;
+ case 3:
outl(0x20008, TCO1_STS);
- } else {
+ break;
+ default:
outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
outw(0x0004, TCO2_STS); /* Clear BOOT_STS bit */
+ break;
}
iTCO_wdt_watchdog_dev.bootstatus = 0;
--
2.1.0
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 4:
+ outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ break;
+ case 3:
outl(0x20008, TCO1_STS);
- } else {
+ break;
+ default:
outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
outw(0x0004, TCO2_STS); /* Clear BOOT_STS bit */
+ break;
}
iTCO_wdt_watchdog_dev.bootstatus = 0;
--
2.1.0
Matt Fleming
Jul 30, 2015, 6:00:07 PM7/30/15
to
From: Matt Fleming <matt.f...@intel.com>
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
Changes in v3:
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
- Added Ack/Review tags to patches
Changes in v2:
- Use lowercase for all new files and data structures
- Allocate platform data with devm_kzalloc()
- Move Kconfig changes to final patch and use 'select', not 'depends'
- Swap strcpy() for strlcpy()
- Fixup use of lpc_ich_info in intel_pmc_ipc which was missed in v1
- Don't use bitops in i2c-i801
- Remove superfluous NULL checks in i2c-i801
- Explicitly list reboot bit versions in no_reboot_bit()
- Use switch/case construst instead of crazy if else
- Fold original fixups from PATCH 4 and 5 into PATCH 1 and 2
Matt Fleming (2):
iTCO_wdt: Expose watchdog properties using platform data
iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 120 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/platform/x86/intel_pmc_ipc.c | 9 ++-
drivers/watchdog/Kconfig | 3 +-
drivers/watchdog/iTCO_wdt.c | 77 ++++++++++++---------
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/itco_wdt.h | 19 ++++++
create mode 100644 include/linux/platform_data/itco_wdt.h
Matt Fleming
Jul 30, 2015, 6:00:08 PM7/30/15
to
From: Matt Fleming <matt.f...@intel.com>
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
This change introduces a new 'struct itco_wdt_platform_data' for use
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
This change is overdue because lpc_ich_info has already found its way
into other TCO watchdog users, notably the intel_pmc_ipc driver where
the watchdog actually isn't on the LPC bus as far as I can see.
A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.
Cc: Peter Tyser <pty...@xes-inc.com>
Cc: Samuel Ortiz <sa...@linux.intel.com>
Acked-by: Lee Jones <lee....@linaro.org>
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
This change introduces a new 'struct itco_wdt_platform_data' for use
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
This change is overdue because lpc_ich_info has already found its way
into other TCO watchdog users, notably the intel_pmc_ipc driver where
the watchdog actually isn't on the LPC bus as far as I can see.
A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.
Cc: Peter Tyser <pty...@xes-inc.com>
Cc: Samuel Ortiz <sa...@linux.intel.com>
Cc: Wim Van Sebroeck <w...@iguana.be>
Cc: Zha Qipeng <qipen...@intel.com>
Cc: Guenter Roeck <li...@roeck-us.net>
- Added Lee's ACK
v2:
- Use lowercase "itco" in all new files and data structures.
- Use devm_kzalloc() to allocate platform data, fixing a memory leak
and saving us from having to free the allocation in error paths
- Move stray Kconfig hunk to PATCH 3 that accidentally snuck into v1
- Swap strcpy() for the safer strlcpy()
- Fix lpc_ich_info user in the intel_pmc_ipc driver that was merged for
v4.2.
drivers/mfd/lpc_ich.c | 32 +++++++++++++++++++++++++++++---
drivers/platform/x86/intel_pmc_ipc.c | 9 ++++-----
drivers/watchdog/iTCO_wdt.c | 11 +++++------
include/linux/mfd/lpc_ich.h | 6 ------
include/linux/platform_data/itco_wdt.h | 19 +++++++++++++++++++
create mode 100644 include/linux/platform_data/itco_wdt.h
@@ -933,7 +956,7 @@ gpe0_done:
lpc_chipset_info[priv->chipset].use_gpio = ret;
lpc_ich_enable_gpio_space(dev);
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+ lpc_ich_finalize_gpio_cell(dev);
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
@@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
res->end = base_addr + ACPIBASE_PMC_END;
}
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+ ret = lpc_ich_finalize_wdt_cell(dev);
+ if (ret)
+ goto wdt_done;
+
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
lpc_chipset_info[priv->chipset].use_gpio = ret;
lpc_ich_enable_gpio_space(dev);
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+ lpc_ich_finalize_gpio_cell(dev);
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
@@ -1007,7 +1030,10 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
res->end = base_addr + ACPIBASE_PMC_END;
}
- lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+ ret = lpc_ich_finalize_wdt_cell(dev);
+ if (ret)
+ goto wdt_done;
+
ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
&lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
index 3c3fd417ddeb..a94401b2deca 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
/* Clear out the (probably old) status */
Matt Fleming
Jul 30, 2015, 6:00:08 PM7/30/15
to
From: Mika Westerberg <mika.we...@linux.intel.com>
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Cc: <linu...@vger.kernel.org>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Cc: <linu...@vger.kernel.org>
Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
- Added Guenter's Review tag
v2:
v2:
Andy Shevchenko
Jul 31, 2015, 4:50:07 AM7/31/15
to
On Thu, 2015-07-30 at 22:59 +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.f...@intel.com>
>
> The revision of the watchdog hardware in Sunrisepoint necessitates a
> new
> "version" inside the TCO watchdog driver because some of the register
> layouts have changed.
>
> Also update the Kconfig entry to select both the LPC and SMBus
> drivers
> since the TCO device is on the SMBus in Sunrisepoint.
Few minor comments below, though I'm okay with current version as well.
> From: Matt Fleming <matt.f...@intel.com>
>
> The revision of the watchdog hardware in Sunrisepoint necessitates a
> new
> "version" inside the TCO watchdog driver because some of the register
> layouts have changed.
>
> Also update the Kconfig entry to select both the LPC and SMBus
> drivers
> since the TCO device is on the SMBus in Sunrisepoint.
if (val32 & enable_bit)
return -EIO;
return 0;
> }
>
> @@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct
> platform_device *dev)
> pdata->name, pdata->version, (u64)TCOBASE);
>
> /* Clear out the (probably old) status */
> - if (iTCO_wdt_private.iTCO_version == 3) {
> + switch (iTCO_wdt_private.iTCO_version) {
> + case 4:
> + outw(0x0008, TCO1_STS); /* Clear the Time
> Out Status bit */
> + outw(0x0002, TCO2_STS); /* Clear
> SECOND_TO_STS bit */
> + break;
> + case 3:
> outl(0x20008, TCO1_STS);
> - } else {
> + break;
> + default:
case 2:
case 1:
default:
> outw(0x0008, TCO1_STS); /* Clear the Time
> Out Status bit */
> outw(0x0002, TCO2_STS); /* Clear
> SECOND_TO_STS bit */
> outw(0x0004, TCO2_STS); /* Clear BOOT_STS
> bit */
> + break;
> }
>
> iTCO_wdt_watchdog_dev.bootstatus = 0;
--
Andy Shevchenko <andriy.s...@linux.intel.com>
> outw(0x0008, TCO1_STS); /* Clear the Time
> Out Status bit */
> outw(0x0002, TCO2_STS); /* Clear
> SECOND_TO_STS bit */
> outw(0x0004, TCO2_STS); /* Clear BOOT_STS
> bit */
> + break;
> }
>
> iTCO_wdt_watchdog_dev.bootstatus = 0;
--
Intel Finland Oy
Darren Hart
Aug 5, 2015, 4:40:07 PM8/5/15
to
On Thu, Jul 30, 2015 at 10:58:59PM +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.f...@intel.com>
>
> Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> the SMBus, unlike previous generations of PCH/ICH where it was on the
> LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> watchdog driver is kind of backwards anyway.
>
> This change introduces a new 'struct itco_wdt_platform_data' for use
> inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> code, which neatly avoids having to include lpc_ich headers in the i801
> i2c driver.
>
> This change is overdue because lpc_ich_info has already found its way
> into other TCO watchdog users, notably the intel_pmc_ipc driver where
> the watchdog actually isn't on the LPC bus as far as I can see.
>
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Cc: Peter Tyser <pty...@xes-inc.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> Acked-by: Lee Jones <lee....@linaro.org>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Cc: Zha Qipeng <qipen...@intel.com>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Cc: Jean Delvare <jdel...@suse.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
For platform/drivers/x86:
> From: Matt Fleming <matt.f...@intel.com>
>
> Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> the SMBus, unlike previous generations of PCH/ICH where it was on the
> LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> watchdog driver is kind of backwards anyway.
>
> This change introduces a new 'struct itco_wdt_platform_data' for use
> inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> code, which neatly avoids having to include lpc_ich headers in the i801
> i2c driver.
>
> This change is overdue because lpc_ich_info has already found its way
> into other TCO watchdog users, notably the intel_pmc_ipc driver where
> the watchdog actually isn't on the LPC bus as far as I can see.
>
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Cc: Peter Tyser <pty...@xes-inc.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> Acked-by: Lee Jones <lee....@linaro.org>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Cc: Zha Qipeng <qipen...@intel.com>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Cc: Jean Delvare <jdel...@suse.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Simple changes to accomodate refactoring.
Acked-by: Darren Hart <dvh...@linux.intel.com>
Darren Hart
Intel Open Source Technology Center
Matt Fleming
Aug 6, 2015, 8:10:06 AM8/6/15
to
(Sorry for the delay in replying)
On Fri, 31 Jul, at 11:41:04AM, Andy Shevchenko wrote:
> >
> > + if (val32 & enable_bit)
> > + ret = -EIO;
> > +
> > return ret; /* returns: 0 = OK, -EIO = Error */
>
> What about removing ret at all?
>
> if (val32 & enable_bit)
> return -EIO;
>
> return 0;
Yeah, good catch. I'll make this change.
> > }
> >
> > @@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct
> > platform_device *dev)
> > pdata->name, pdata->version, (u64)TCOBASE);
> >
> > /* Clear out the (probably old) status */
> > - if (iTCO_wdt_private.iTCO_version == 3) {
> > + switch (iTCO_wdt_private.iTCO_version) {
> > + case 4:
> > + outw(0x0008, TCO1_STS); /* Clear the Time
> > Out Status bit */
> > + outw(0x0002, TCO2_STS); /* Clear
> > SECOND_TO_STS bit */
> > + break;
> > + case 3:
> > outl(0x20008, TCO1_STS);
> > - } else {
> > + break;
> > + default:
>
> Same idea here: put cases explicitly for all existing versions?
>
> case 2:
> case 1:
> default:
I intentionally didn't do that because I figured it was implied that
versions 2 and 1 are included in the default case. But I've no problem
with making this change. I'll send a v4.
--
Matt Fleming, Intel Open Source Technology Center
On Fri, 31 Jul, at 11:41:04AM, Andy Shevchenko wrote:
> >
> > + if (val32 & enable_bit)
> > + ret = -EIO;
> > +
> > return ret; /* returns: 0 = OK, -EIO = Error */
>
> What about removing ret at all?
>
> if (val32 & enable_bit)
> return -EIO;
>
> return 0;
> > }
> >
> > @@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct
> > platform_device *dev)
> > pdata->name, pdata->version, (u64)TCOBASE);
> >
> > /* Clear out the (probably old) status */
> > - if (iTCO_wdt_private.iTCO_version == 3) {
> > + switch (iTCO_wdt_private.iTCO_version) {
> > + case 4:
> > + outw(0x0008, TCO1_STS); /* Clear the Time
> > Out Status bit */
> > + outw(0x0002, TCO2_STS); /* Clear
> > SECOND_TO_STS bit */
> > + break;
> > + case 3:
> > outl(0x20008, TCO1_STS);
> > - } else {
> > + break;
> > + default:
>
> Same idea here: put cases explicitly for all existing versions?
>
> case 2:
> case 1:
> default:
versions 2 and 1 are included in the default case. But I've no problem
with making this change. I'll send a v4.
--
Matt Fleming, Intel Open Source Technology Center
Matt Fleming
Aug 6, 2015, 8:50:07 AM8/6/15
to
From: Matt Fleming <matt.f...@intel.com>
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
Also update the Kconfig entry to select both the LPC and SMBus drivers
since the TCO device is on the SMBus in Sunrisepoint.
The revision of the watchdog hardware in Sunrisepoint necessitates a new
"version" inside the TCO watchdog driver because some of the register
layouts have changed.
Also update the Kconfig entry to select both the LPC and SMBus drivers
since the TCO device is on the SMBus in Sunrisepoint.
Cc: Wim Van Sebroeck <w...@iguana.be>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
---
v4:
- Explicitly list versions in iTCO_wdt_probe()
- Delete the now superfluous 'ret' variable in iTCO_wdt_unset_NO_REBOOT_bit()
v3:
- Added Guenter's Review tag
v2:
- Explicitly list TCO versions and their "no reboot" bits in the switch
statement in no_reboot_bit()
- Use a switch/case statement when clearing status bits instead of
convoluted if/else branching
- Select both of the bus drivers instead of using depends
drivers/watchdog/iTCO_wdt.c | 71 ++++++++++++++++++++++++++++-----------------
2 files changed, 46 insertions(+), 28 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 241fafde42cb..55c4b5b0a317 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -797,7 +797,8 @@ config ITCO_WDT
tristate "Intel TCO Timer/Watchdog"
depends on (X86 || IA64) && PCI
select WATCHDOG_CORE
- select LPC_ICH
+ select LPC_ICH if !EXPERT
+ select I2C_I801 if !EXPERT
---help---
Hardware driver for the intel TCO timer based watchdog devices.
These drivers are included in the Intel 82801 I/O Controller
index a94401b2deca..0acc6c5f729d 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -145,59 +145,67 @@ static inline unsigned int ticks_to_seconds(int ticks)
return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * 6) / 10;
}
+static inline u32 no_reboot_bit(void)
+{
+ u32 enable_bit;
+
+ switch (iTCO_wdt_private.iTCO_version) {
}
+static inline u32 no_reboot_bit(void)
+{
+ u32 enable_bit;
+
+ case 3:
+ enable_bit = 0x00000010;
+ break;
+ case 2:
+ enable_bit = 0x00000020;
+ break;
+ case 4:
+ case 1:
+ default:
+ enable_bit = 0x00000002;
+ break;
+ }
+
+ return enable_bit;
+}
+
static void iTCO_wdt_set_NO_REBOOT_bit(void)
{
u32 val32;
/* Set the NO_REBOOT bit: this disables reboots */
+ enable_bit = 0x00000010;
+ break;
+ case 2:
+ enable_bit = 0x00000020;
+ break;
+ case 4:
+ case 1:
+ default:
+ enable_bit = 0x00000002;
+ break;
+ }
+
+ return enable_bit;
+}
+
static void iTCO_wdt_set_NO_REBOOT_bit(void)
{
u32 val32;
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000010;
- writel(val32, iTCO_wdt_private.gcs_pmc);
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000020;
+ val32 |= no_reboot_bit();
writel(val32, iTCO_wdt_private.gcs_pmc);
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 |= 0x00000002;
+ val32 |= no_reboot_bit();
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
}
}
static int iTCO_wdt_unset_NO_REBOOT_bit(void)
{
- int ret = 0;
- val32 |= 0x00000010;
- writel(val32, iTCO_wdt_private.gcs_pmc);
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 |= 0x00000020;
+ val32 |= no_reboot_bit();
writel(val32, iTCO_wdt_private.gcs_pmc);
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 |= 0x00000002;
+ val32 |= no_reboot_bit();
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
}
}
static int iTCO_wdt_unset_NO_REBOOT_bit(void)
{
- u32 val32;
+ u32 enable_bit = no_reboot_bit();
+ u32 val32 = 0;
/* Unset the NO_REBOOT bit: this enables reboots */
- if (iTCO_wdt_private.iTCO_version == 3) {
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffef;
- writel(val32, iTCO_wdt_private.gcs_pmc);
-
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000010)
- ret = -EIO;
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffdf;
+ val32 &= ~enable_bit;
writel(val32, iTCO_wdt_private.gcs_pmc);
val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000020)
- ret = -EIO;
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 &= 0xfffffffd;
+ val32 &= ~enable_bit;
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- if (val32 & 0x00000002)
- ret = -EIO;
}
- return ret; /* returns: 0 = OK, -EIO = Error */
- val32 &= 0xffffffef;
- writel(val32, iTCO_wdt_private.gcs_pmc);
-
- val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000010)
- ret = -EIO;
- } else if (iTCO_wdt_private.iTCO_version == 2) {
+ if (iTCO_wdt_private.iTCO_version >= 2) {
val32 = readl(iTCO_wdt_private.gcs_pmc);
- val32 &= 0xffffffdf;
+ val32 &= ~enable_bit;
writel(val32, iTCO_wdt_private.gcs_pmc);
val32 = readl(iTCO_wdt_private.gcs_pmc);
- if (val32 & 0x00000020)
- ret = -EIO;
} else if (iTCO_wdt_private.iTCO_version == 1) {
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- val32 &= 0xfffffffd;
+ val32 &= ~enable_bit;
pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, val32);
pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, &val32);
- if (val32 & 0x00000002)
- ret = -EIO;
}
+ if (val32 & enable_bit)
+ return -EIO;
+
+ return 0;
}
static int iTCO_wdt_start(struct watchdog_device *wd_dev)
@@ -503,12 +511,21 @@ static int iTCO_wdt_probe(struct platform_device *dev)
pdata->name, pdata->version, (u64)TCOBASE);
/* Clear out the (probably old) status */
- if (iTCO_wdt_private.iTCO_version == 3) {
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 4:
+ outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ break;
+ case 3:
outl(0x20008, TCO1_STS);
- } else {
+ break;
+ case 2:
/* Clear out the (probably old) status */
- if (iTCO_wdt_private.iTCO_version == 3) {
+ switch (iTCO_wdt_private.iTCO_version) {
+ case 4:
+ outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
+ outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
+ break;
+ case 3:
outl(0x20008, TCO1_STS);
- } else {
+ break;
+ case 1:
+ default:
+ default:
outw(0x0008, TCO1_STS); /* Clear the Time Out Status bit */
outw(0x0002, TCO2_STS); /* Clear SECOND_TO_STS bit */
outw(0x0004, TCO2_STS); /* Clear BOOT_STS bit */
+ break;
}
iTCO_wdt_watchdog_dev.bootstatus = 0;
--
2.1.0
+ break;
}
iTCO_wdt_watchdog_dev.bootstatus = 0;
--
Matt Fleming
Aug 6, 2015, 8:50:07 AM8/6/15
to
From: Matt Fleming <matt.f...@intel.com>
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
This change introduces a new 'struct itco_wdt_platform_data' for use
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
This change is overdue because lpc_ich_info has already found its way
into other TCO watchdog users, notably the intel_pmc_ipc driver where
the watchdog actually isn't on the LPC bus as far as I can see.
A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.
Cc: Peter Tyser <pty...@xes-inc.com>
Cc: Samuel Ortiz <sa...@linux.intel.com>
Acked-by: Lee Jones <lee....@linaro.org>
Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
the SMBus, unlike previous generations of PCH/ICH where it was on the
LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
watchdog driver is kind of backwards anyway.
This change introduces a new 'struct itco_wdt_platform_data' for use
inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
code, which neatly avoids having to include lpc_ich headers in the i801
i2c driver.
This change is overdue because lpc_ich_info has already found its way
into other TCO watchdog users, notably the intel_pmc_ipc driver where
the watchdog actually isn't on the LPC bus as far as I can see.
A simple translation layer is provided for converting from the existing
'struct lpc_ich_info' inside the lpc_ich mfd driver.
Cc: Peter Tyser <pty...@xes-inc.com>
Cc: Samuel Ortiz <sa...@linux.intel.com>
Acked-by: Lee Jones <lee....@linaro.org>
Cc: Wim Van Sebroeck <w...@iguana.be>
Cc: Zha Qipeng <qipen...@intel.com>
Cc: Guenter Roeck <li...@roeck-us.net>
Cc: Jean Delvare <jdel...@suse.com>
Acked-by: Darren Hart <dvh...@linux.intel.com> [drivers/x86 refactoring]
- Added Darren's ACK
index 3c3fd417ddeb..a94401b2deca 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
/* Clear out the (probably old) status */
Matt Fleming
Aug 6, 2015, 8:50:09 AM8/6/15
to
From: Matt Fleming <matt.f...@intel.com>
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
Changes in v4:
Starting with Intel Sunrisepoint (Skylake PCH) the TCO watchdog device
is now on the SMBUS, whereas for previous ICH/PCH it was on the LPC bus.
Because iTCO_wdt devices may now appear on either the LPC bus or the
SMBUS we need to abstract the bus information into an agnostic structure
instead of the existing 'lpc_ich_info' which tightly integrates both the
lpc_ich and iTCO_wdt drivers.
The first patch introduces a platform data structure to handle this and
shuffles the existing code around. The other patches add the
device-specific information to the i2c-i801 and iTCO_wdt drivers.
Patches based against v4.2-rc4, if there's some other tree I should base
this on, please let me know.
Comments welcome!
- Added Darren's ACK for PATCH 1
- Explicitly list versions in iTCO_wdt_probe()
- Delete superfluous 'ret' variable in iTCO_wdt_unset_NO_REBOOT_bit()
Changes in v3:
- Added Ack/Review tags to patches
Changes in v2:
- Use lowercase for all new files and data structures
- Allocate platform data with devm_kzalloc()
- Move Kconfig changes to final patch and use 'select', not 'depends'
- Swap strcpy() for strlcpy()
- Fixup use of lpc_ich_info in intel_pmc_ipc which was missed in v1
- Don't use bitops in i2c-i801
- Remove superfluous NULL checks in i2c-i801
- Explicitly list reboot bit versions in no_reboot_bit()
- Use switch/case construst instead of crazy if else
- Fold original fixups from PATCH 4 and 5 into PATCH 1 and 2
Matt Fleming (2):
iTCO_wdt: Expose watchdog properties using platform data
iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 120 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/platform/x86/intel_pmc_ipc.c | 9 ++-
drivers/watchdog/Kconfig | 3 +-
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/itco_wdt.h | 19 ++++++
7 files changed, 222 insertions(+), 49 deletions(-)
include/linux/platform_data/itco_wdt.h | 19 ++++++
create mode 100644 include/linux/platform_data/itco_wdt.h
Matt Fleming
Aug 6, 2015, 8:50:08 AM8/6/15
to
From: Mika Westerberg <mika.we...@linux.intel.com>
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Cc: <linu...@vger.kernel.org>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
- No changes
Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
have been moved to reside under the i801 SMBus host controller whereas
previously they were under the LPC device.
In order to support the iTCO watchdog on newer PCHs we need to create the
platform device here in the SMBus driver and pass all known resources using
platform data.
Cc: Jean Delvare <jdel...@suse.com>
Cc: Wolfram Sang <w...@the-dreams.de>
Cc: <linu...@vger.kernel.org>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
v3:
- Added Guenter's Review tag
v2:
Wolfram Sang
Aug 6, 2015, 5:10:06 PM8/6/15
to
On Thu, Aug 06, 2015 at 01:00:24PM +0100, Matt Fleming wrote:
> (Sorry for the delay in replying)
>
> On Fri, 31 Jul, at 11:41:04AM, Andy Shevchenko wrote:
> > >
> > > + if (val32 & enable_bit)
> > > + ret = -EIO;
> > > +
> > > return ret; /* returns: 0 = OK, -EIO = Error */
> >
> > What about removing ret at all?
> >
> > if (val32 & enable_bit)
> > return -EIO;
> >
> > return 0;
>
> Yeah, good catch. I'll make this change.
I'd go for the ternary operator here.
> (Sorry for the delay in replying)
>
> On Fri, 31 Jul, at 11:41:04AM, Andy Shevchenko wrote:
> > >
> > > + if (val32 & enable_bit)
> > > + ret = -EIO;
> > > +
> > > return ret; /* returns: 0 = OK, -EIO = Error */
> >
> > What about removing ret at all?
> >
> > if (val32 & enable_bit)
> > return -EIO;
> >
> > return 0;
>
> Yeah, good catch. I'll make this change.
Guenter Roeck
Aug 7, 2015, 11:40:08 AM8/7/15
to
On 08/06/2015 05:46 AM, Matt Fleming wrote:
> From: Matt Fleming <matt.f...@intel.com>
>
> Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> the SMBus, unlike previous generations of PCH/ICH where it was on the
> LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> watchdog driver is kind of backwards anyway.
>
> This change introduces a new 'struct itco_wdt_platform_data' for use
> inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> code, which neatly avoids having to include lpc_ich headers in the i801
> i2c driver.
>
> This change is overdue because lpc_ich_info has already found its way
> into other TCO watchdog users, notably the intel_pmc_ipc driver where
> the watchdog actually isn't on the LPC bus as far as I can see.
>
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Cc: Peter Tyser <pty...@xes-inc.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> Acked-by: Lee Jones <lee....@linaro.org>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Cc: Zha Qipeng <qipen...@intel.com>
> Cc: Guenter Roeck <li...@roeck-us.net>
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> From: Matt Fleming <matt.f...@intel.com>
>
> Intel Sunrisepoint (Skylake PCH) has the iTCO watchdog accessible across
> the SMBus, unlike previous generations of PCH/ICH where it was on the
> LPC bus. Because it's on the SMBus, it doesn't make sense to pass around
> a 'struct lpc_ich_info', and leaking the type of bus into the iTCO
> watchdog driver is kind of backwards anyway.
>
> This change introduces a new 'struct itco_wdt_platform_data' for use
> inside the iTCO watchdog driver and by the upcoming Intel Sunrisepoint
> code, which neatly avoids having to include lpc_ich headers in the i801
> i2c driver.
>
> This change is overdue because lpc_ich_info has already found its way
> into other TCO watchdog users, notably the intel_pmc_ipc driver where
> the watchdog actually isn't on the LPC bus as far as I can see.
>
> A simple translation layer is provided for converting from the existing
> 'struct lpc_ich_info' inside the lpc_ich mfd driver.
>
> Cc: Peter Tyser <pty...@xes-inc.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> Acked-by: Lee Jones <lee....@linaro.org>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Cc: Zha Qipeng <qipen...@intel.com>
> Cc: Guenter Roeck <li...@roeck-us.net>
Wolfram Sang
Aug 7, 2015, 12:00:06 PM8/7/15
to
On Thu, Aug 06, 2015 at 01:46:25PM +0100, Matt Fleming wrote:
> From: Mika Westerberg <mika.we...@linux.intel.com>
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Cc: <linu...@vger.kernel.org>
> Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
Dunno which tree this should go through (probably MFD or watchdog), but
> From: Mika Westerberg <mika.we...@linux.intel.com>
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Cc: <linu...@vger.kernel.org>
> Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
doesn't look like I2C, so:
Acked-by: Wolfram Sang <w...@the-dreams.de>
Guenter Roeck
Aug 7, 2015, 12:30:07 PM8/7/15
to
Guenter
Lee Jones
Aug 11, 2015, 10:20:07 AM8/11/15
to
On Thu, 06 Aug 2015, Matt Fleming wrote:
> From: Mika Westerberg <mika.we...@linux.intel.com>
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Cc: <linu...@vger.kernel.org>
> Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
> ---
>
> v4:
> - No changes
>
> v3:
> - Added Guenter's Review tag
>
> v2:
> - Don't use bitops but same scheme used already
> - Delete superfluous NULL-checks for platform_device_unregister()
>
> drivers/i2c/busses/i2c-i801.c | 120 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 119 insertions(+), 1 deletion(-)
Applied, thanks.
> From: Mika Westerberg <mika.we...@linux.intel.com>
>
> Starting from Intel Sunrisepoint (Skylake PCH) the iTCO watchdog resources
> have been moved to reside under the i801 SMBus host controller whereas
> previously they were under the LPC device.
>
> In order to support the iTCO watchdog on newer PCHs we need to create the
> platform device here in the SMBus driver and pass all known resources using
> platform data.
>
> Cc: Jean Delvare <jdel...@suse.com>
> Cc: Wolfram Sang <w...@the-dreams.de>
> Cc: <linu...@vger.kernel.org>
> Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> Signed-off-by: Mika Westerberg <mika.we...@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
> ---
>
> v4:
> - No changes
>
> v3:
> - Added Guenter's Review tag
>
> v2:
> - Don't use bitops but same scheme used already
> - Delete superfluous NULL-checks for platform_device_unregister()
>
> drivers/i2c/busses/i2c-i801.c | 120 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 119 insertions(+), 1 deletion(-)
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones
Aug 11, 2015, 10:20:07 AM8/11/15
to
On Thu, 06 Aug 2015, Matt Fleming wrote:
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones
Aug 11, 2015, 10:20:07 AM8/11/15
to
On Thu, 06 Aug 2015, Matt Fleming wrote:
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones
Aug 12, 2015, 4:30:07 AM8/12/15
to
Enjoy!
The following changes since commit bc0195aad0daa2ad5b0d76cce22b167bc3435590:
Linux 4.2-rc2 (2015-07-12 15:10:30 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-i2c-x86-watchdog-v4.3
for you to fetch changes up to 2a7a0e9bf7b32e838d873226808ab8a6c00148f7:
watchdog: iTCO_wdt: Add support for TCO on Intel Sunrisepoint (2015-08-11 15:03:52 +0100)
----------------------------------------------------------------
Immutable branch between MFD, I2C, X86 and Watchdog due for v4.3
----------------------------------------------------------------
Matt Fleming (2):
mfd: watchdog: iTCO_wdt: Expose watchdog properties using platform data
watchdog: iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 120 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/platform/x86/intel_pmc_ipc.c | 9 ++-
drivers/watchdog/Kconfig | 3 +-
drivers/watchdog/iTCO_wdt.c | 82 +++++++++++++---------
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/itco_wdt.h | 19 ++++++
7 files changed, 222 insertions(+), 49 deletions(-)
create mode 100644 include/linux/platform_data/itco_wdt.h
--
The following changes since commit bc0195aad0daa2ad5b0d76cce22b167bc3435590:
Linux 4.2-rc2 (2015-07-12 15:10:30 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-i2c-x86-watchdog-v4.3
for you to fetch changes up to 2a7a0e9bf7b32e838d873226808ab8a6c00148f7:
watchdog: iTCO_wdt: Add support for TCO on Intel Sunrisepoint (2015-08-11 15:03:52 +0100)
----------------------------------------------------------------
Immutable branch between MFD, I2C, X86 and Watchdog due for v4.3
----------------------------------------------------------------
Matt Fleming (2):
mfd: watchdog: iTCO_wdt: Expose watchdog properties using platform data
watchdog: iTCO_wdt: Add support for TCO on Intel Sunrisepoint
Mika Westerberg (1):
i2c: i801: Create iTCO device on newer Intel PCHs
drivers/i2c/busses/i2c-i801.c | 120 ++++++++++++++++++++++++++++++++-
drivers/mfd/lpc_ich.c | 32 ++++++++-
drivers/platform/x86/intel_pmc_ipc.c | 9 ++-
drivers/watchdog/Kconfig | 3 +-
drivers/watchdog/iTCO_wdt.c | 82 +++++++++++++---------
include/linux/mfd/lpc_ich.h | 6 --
include/linux/platform_data/itco_wdt.h | 19 ++++++
7 files changed, 222 insertions(+), 49 deletions(-)
create mode 100644 include/linux/platform_data/itco_wdt.h
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
0 new messages