Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs

Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs

[Guenter Roeck]

On 07/27/2015 06:38 AM, Matt Fleming wrote:
> From: Mika Westerberg <mika.westerberg@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 <jdelvare@suse.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

That really asks for an mfd driver, but that might be a bit overkill.
Copying the i2c mailing list for additional feedback.

> ---
>   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)

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.

> +
> +#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() 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.

> +	}
> +}
> +
>   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)
>

Re: [PATCH 2/5] i2c: i801: Create iTCO device on newer Intel PCHs

[Matt Fleming]

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
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.
 
Good point, I'll remove this check since it makes the code simpler too.

> >+		platform_device_unregister(priv->tco_pdev);
> >+		priv->tco_pdev = NULL;
> 
> Unnecessary; priv is going to be freed right afterwards.
 
I'll drop this.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html