[PATCH 0/2] watchdog: Correct iTCO for Cannon Lake and beyond

[PATCH 0/2] watchdog: Correct iTCO for Cannon Lake and beyond

[Mika Westerberg]

Hi,

Starting from Intel Cannon Lake PCH the NO_REBOOT bit which is required by
the iTCO driver was moved from private register space to be part of the
TCO1_CNT register. This series introduces a new iTCO version (6) that gets
set for Cannon Lake, Cedar Fork, Comet Lake, Elkhart Lake and Ice Lake
which are affected by this move.

Mika Westerberg (2):
  watchdog: iTCO: Add support for Cannon Lake PCH iTCO
  i2c: i801: Use iTCO version 6 in Cannon Lake PCH and beyond

 drivers/i2c/busses/i2c-i801.c | 138 +++++++++++++++++++++-------------
 drivers/watchdog/iTCO_wdt.c   |  25 +++++-
 2 files changed, 109 insertions(+), 54 deletions(-)

-- 
2.20.1

[PATCH 1/2] watchdog: iTCO: Add support for Cannon Lake PCH iTCO

[Mika Westerberg]

In Intel Cannon Lake PCH the NO_REBOOT bit was moved from the private
register space to be part of the TCO1_CNT register. For this reason
introduce another version (6) that uses this register to set and clear
NO_REBOOT bit.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/watchdog/iTCO_wdt.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index c559f706ae7e..505f2c837347 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -215,6 +215,23 @@ static int update_no_reboot_bit_mem(void *priv, bool set)
 	return 0;
 }
 
+static int update_no_reboot_bit_cnt(void *priv, bool set)
+{
+	struct iTCO_wdt_private *p = priv;
+	u16 val, newval;
+
+	val = inw(TCO1_CNT(p));
+	if (set)
+		val |= BIT(0);
+	else
+		val &= ~BIT(0);
+	outw(val, TCO1_CNT(p));
+	newval = inw(TCO1_CNT(p));
+
+	/* make sure the update is successful */
+	return val != newval ? -EIO : 0;
+}
+
 static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p,
 		struct itco_wdt_platform_data *pdata)
 {
@@ -224,7 +241,9 @@ static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p,
 		return;
 	}
 
-	if (p->iTCO_version >= 2)
+	if (p->iTCO_version >= 6)
+		p->update_no_reboot_bit = update_no_reboot_bit_cnt;
+	else if (p->iTCO_version >= 2)
 		p->update_no_reboot_bit = update_no_reboot_bit_mem;
 	else if (p->iTCO_version == 1)
 		p->update_no_reboot_bit = update_no_reboot_bit_pci;
@@ -452,7 +471,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
 	 */
-	if (p->iTCO_version >= 2 && !pdata->update_no_reboot_bit) {
+	if (p->iTCO_version >= 2 && p->iTCO_version < 6 &&
+	    !pdata->update_no_reboot_bit) {
 		p->gcs_pmc_res = platform_get_resource(pdev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
@@ -502,6 +522,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 
 	/* Clear out the (probably old) status */
 	switch (p->iTCO_version) {
+	case 6:
 	case 5:
 	case 4:
 		outw(0x0008, TCO1_STS(p)); /* Clear the Time Out Status bit */
-- 
2.20.1

Re: [PATCH 1/2] watchdog: iTCO: Add support for Cannon Lake PCH iTCO

[Guenter Roeck]

On Fri, Aug 09, 2019 at 03:45:52PM +0300, Mika Westerberg wrote:
> In Intel Cannon Lake PCH the NO_REBOOT bit was moved from the private
> register space to be part of the TCO1_CNT register. For this reason
> introduce another version (6) that uses this register to set and clear
> NO_REBOOT bit.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

I assume this will be applied through i2c together with the other patch
of the series.

Guenter

> ---
>  drivers/watchdog/iTCO_wdt.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index c559f706ae7e..505f2c837347 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -215,6 +215,23 @@ static int update_no_reboot_bit_mem(void *priv, bool set)
>  	return 0;
>  }
>  
> +static int update_no_reboot_bit_cnt(void *priv, bool set)
> +{
> +	struct iTCO_wdt_private *p = priv;
> +	u16 val, newval;
> +
> +	val = inw(TCO1_CNT(p));
> +	if (set)
> +		val |= BIT(0);
> +	else
> +		val &= ~BIT(0);
> +	outw(val, TCO1_CNT(p));
> +	newval = inw(TCO1_CNT(p));
> +
> +	/* make sure the update is successful */
> +	return val != newval ? -EIO : 0;
> +}
> +
>  static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p,
>  		struct itco_wdt_platform_data *pdata)
>  {
> @@ -224,7 +241,9 @@ static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p,
>  		return;
>  	}
>  
> -	if (p->iTCO_version >= 2)
> +	if (p->iTCO_version >= 6)
> +		p->update_no_reboot_bit = update_no_reboot_bit_cnt;
> +	else if (p->iTCO_version >= 2)
>  		p->update_no_reboot_bit = update_no_reboot_bit_mem;
>  	else if (p->iTCO_version == 1)
>  		p->update_no_reboot_bit = update_no_reboot_bit_pci;
> @@ -452,7 +471,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	 * Get the Memory-Mapped GCS or PMC register, we need it for the
>  	 * NO_REBOOT flag (TCO v2 and v3).
>  	 */
> -	if (p->iTCO_version >= 2 && !pdata->update_no_reboot_bit) {
> +	if (p->iTCO_version >= 2 && p->iTCO_version < 6 &&
> +	    !pdata->update_no_reboot_bit) {
>  		p->gcs_pmc_res = platform_get_resource(pdev,
>  						       IORESOURCE_MEM,
>  						       ICH_RES_MEM_GCS_PMC);
> @@ -502,6 +522,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  
>  	/* Clear out the (probably old) status */
>  	switch (p->iTCO_version) {
> +	case 6:
>  	case 5:
>  	case 4:
>  		outw(0x0008, TCO1_STS(p)); /* Clear the Time Out Status bit */
> -- 
> 2.20.1
> 

Re: [PATCH 1/2] watchdog: iTCO: Add support for Cannon Lake PCH iTCO

[Jean Delvare]

Hi Mika,

On Fri, 9 Aug 2019 15:45:52 +0300, Mika Westerberg wrote:
> In Intel Cannon Lake PCH the NO_REBOOT bit was moved from the private
> register space to be part of the TCO1_CNT register. For this reason
> introduce another version (6) that uses this register to set and clear
> NO_REBOOT bit.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/watchdog/iTCO_wdt.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index c559f706ae7e..505f2c837347 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -215,6 +215,23 @@ static int update_no_reboot_bit_mem(void *priv, bool set)
>  	return 0;
>  }
>  
> +static int update_no_reboot_bit_cnt(void *priv, bool set)
> +{
> +	struct iTCO_wdt_private *p = priv;
> +	u16 val, newval;
> +
> +	val = inw(TCO1_CNT(p));
> +	if (set)
> +		val |= BIT(0);
> +	else
> +		val &= ~BIT(0);

Are you not supposed to include <linux/bits.h> before you use BIT()?

> +	outw(val, TCO1_CNT(p));
> +	newval = inw(TCO1_CNT(p));
> +
> +	/* make sure the update is successful */
> +	return val != newval ? -EIO : 0;
> +}

Other than this minor nitpick, looks good to me.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] watchdog: iTCO: Add support for Cannon Lake PCH iTCO

[Mika Westerberg]

On Wed, Aug 28, 2019 at 02:50:34PM +0200, Jean Delvare wrote:
> Hi Mika,

Hi,

> On Fri, 9 Aug 2019 15:45:52 +0300, Mika Westerberg wrote:
> > In Intel Cannon Lake PCH the NO_REBOOT bit was moved from the private
> > register space to be part of the TCO1_CNT register. For this reason
> > introduce another version (6) that uses this register to set and clear
> > NO_REBOOT bit.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/watchdog/iTCO_wdt.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index c559f706ae7e..505f2c837347 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > @@ -215,6 +215,23 @@ static int update_no_reboot_bit_mem(void *priv, bool set)
> >  	return 0;
> >  }
> >  
> > +static int update_no_reboot_bit_cnt(void *priv, bool set)
> > +{
> > +	struct iTCO_wdt_private *p = priv;
> > +	u16 val, newval;
> > +
> > +	val = inw(TCO1_CNT(p));
> > +	if (set)
> > +		val |= BIT(0);
> > +	else
> > +		val &= ~BIT(0);
> 
> Are you not supposed to include <linux/bits.h> before you use BIT()?

Apparently not because it compiles just fine without ;-) Probably gets
includes via another header. I'll add it in v2.

> > +	outw(val, TCO1_CNT(p));
> > +	newval = inw(TCO1_CNT(p));
> > +
> > +	/* make sure the update is successful */
> > +	return val != newval ? -EIO : 0;
> > +}
> 
> Other than this minor nitpick, looks good to me.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>

Thanks!

[PATCH 2/2] i2c: i801: Use iTCO version 6 in Cannon Lake PCH and beyond

[Mika Westerberg]

Intel Cannon Lake PCH moved the NO_REBOOT bit to reside as part of the
TCO registers instead so update the i2c-i801 driver so that for Cannon
Lake and beyond register platform device for iTCO using version 6. The
affected PCHs are Cannon Lake, Cedar Fork, Comet Lake, Elkhart Lake and
Ice Lake.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-i801.c | 138 +++++++++++++++++++++-------------
 1 file changed, 86 insertions(+), 52 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index f2956936c3f2..6918406d00a5 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -292,7 +292,8 @@ struct i801_priv {
 #define FEATURE_HOST_NOTIFY	BIT(5)
 /* Not really a feature, but it's convenient to handle it as such */
 #define FEATURE_IDF		BIT(15)
-#define FEATURE_TCO		BIT(16)
+#define FEATURE_TCO_SPT		BIT(16)
+#define FEATURE_TCO_CNL		BIT(17)
 
 static const char *i801_feature_names[] = {
 	"SMBus PEC",
@@ -1491,57 +1492,37 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
 }
 #endif
 
-static const struct itco_wdt_platform_data tco_platform_data = {
+static const struct itco_wdt_platform_data cnl_tco_platform_data = {
+	.name = "Intel PCH",
+	.version = 6,
+};
+
+static struct platform_device *
+i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
+		 struct resource *tco_res)
+{
+	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+					tco_res, 2, &cnl_tco_platform_data,
+					sizeof(cnl_tco_platform_data));
+}
+
+static const struct itco_wdt_platform_data spt_tco_platform_data = {
 	.name = "Intel PCH",
 	.version = 4,
 };
 
 static DEFINE_SPINLOCK(p2sb_spinlock);
 
-static void i801_add_tco(struct i801_priv *priv)
+static struct platform_device *
+i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
+		 struct resource *tco_res)
 {
-	struct pci_dev *pci_dev = priv->pci_dev;
-	struct resource tco_res[3], *res;
-	struct platform_device *pdev;
+	struct resource *res;
 	unsigned int devfn;
-	u32 tco_base, tco_ctl;
-	u32 base_addr, ctrl_val;
 	u64 base64_addr;
+	u32 base_addr;
 	u8 hidden;
 
-	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
@@ -1577,15 +1558,58 @@ static void i801_add_tco(struct i801_priv *priv)
 	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 platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+					tco_res, 3, &spt_tco_platform_data,
+					sizeof(spt_tco_platform_data));
+}
+
+static void i801_add_tco(struct i801_priv *priv)
+{
+	u32 base_addr, tco_base, tco_ctl, ctrl_val;
+	struct pci_dev *pci_dev = priv->pci_dev;
+	struct resource tco_res[3], *res;
+	unsigned int devfn;
+
+	if (!(priv->features & (FEATURE_TCO_SPT | FEATURE_TCO_CNL)))
 		return;
-	}
 
-	priv->tco_pdev = pdev;
+	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);
+
+	if (priv->features & FEATURE_TCO_CNL)
+		priv->tco_pdev = i801_add_tco_cnl(priv, pci_dev, tco_res);
+	else
+		priv->tco_pdev = i801_add_tco_spt(priv, pci_dev, tco_res);
+
+	if (IS_ERR(priv->tco_pdev))
+		dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
 }
 
 #ifdef CONFIG_ACPI
@@ -1695,13 +1719,23 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	switch (dev->device) {
 	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
 	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
-	case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS:
-	case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS:
 	case PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS:
 	case PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS:
-	case PCI_DEVICE_ID_INTEL_CDF_SMBUS:
 	case PCI_DEVICE_ID_INTEL_DNV_SMBUS:
 	case PCI_DEVICE_ID_INTEL_KABYLAKE_PCH_H_SMBUS:
+		priv->features |= FEATURE_I2C_BLOCK_READ;
+		priv->features |= FEATURE_IRQ;
+		priv->features |= FEATURE_SMBUS_PEC;
+		priv->features |= FEATURE_BLOCK_BUFFER;
+		/* If we have ACPI based watchdog use that instead */
+		if (!acpi_has_watchdog())
+			priv->features |= FEATURE_TCO_SPT;
+		priv->features |= FEATURE_HOST_NOTIFY;
+		break;
+
+	case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS:
+	case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS:
+	case PCI_DEVICE_ID_INTEL_CDF_SMBUS:
 	case PCI_DEVICE_ID_INTEL_ICELAKE_LP_SMBUS:
 	case PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS:
 	case PCI_DEVICE_ID_INTEL_ELKHART_LAKE_SMBUS:
@@ -1713,7 +1747,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		priv->features |= FEATURE_BLOCK_BUFFER;
 		/* If we have ACPI based watchdog use that instead */
 		if (!acpi_has_watchdog())
-			priv->features |= FEATURE_TCO;
+			priv->features |= FEATURE_TCO_CNL;
 		priv->features |= FEATURE_HOST_NOTIFY;
 		break;
 
-- 
2.20.1

Re: [PATCH 2/2] i2c: i801: Use iTCO version 6 in Cannon Lake PCH and beyond

[Jean Delvare]

Hi Mika,

On Fri, 9 Aug 2019 15:45:53 +0300, Mika Westerberg wrote:
> Intel Cannon Lake PCH moved the NO_REBOOT bit to reside as part of the
> TCO registers instead so update the i2c-i801 driver so that for Cannon
> Lake and beyond register platform device for iTCO using version 6. The
> affected PCHs are Cannon Lake, Cedar Fork, Comet Lake, Elkhart Lake and
> Ice Lake.

Looks overall good, with just minor comments inline below.

> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 138 +++++++++++++++++++++-------------
>  1 file changed, 86 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index f2956936c3f2..6918406d00a5 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -292,7 +292,8 @@ struct i801_priv {
>  #define FEATURE_HOST_NOTIFY	BIT(5)
>  /* Not really a feature, but it's convenient to handle it as such */
>  #define FEATURE_IDF		BIT(15)
> -#define FEATURE_TCO		BIT(16)
> +#define FEATURE_TCO_SPT		BIT(16)
> +#define FEATURE_TCO_CNL		BIT(17)
>  
>  static const char *i801_feature_names[] = {
>  	"SMBus PEC",
> @@ -1491,57 +1492,37 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
>  }
>  #endif
>  
> -static const struct itco_wdt_platform_data tco_platform_data = {
> +static const struct itco_wdt_platform_data cnl_tco_platform_data = {
> +	.name = "Intel PCH",
> +	.version = 6,
> +};
> +
> +static struct platform_device *
> +i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
> +		 struct resource *tco_res)
> +{
> +	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> +					tco_res, 2, &cnl_tco_platform_data,
> +					sizeof(cnl_tco_platform_data));
> +}
> +
> +static const struct itco_wdt_platform_data spt_tco_platform_data = {
>  	.name = "Intel PCH",
>  	.version = 4,
>  };

I think it would make more sense to put the oldest code (v4) first and
then the newest code (v6)?

> (...)
> @@ -1695,13 +1719,23 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	switch (dev->device) {
>  	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
> -	case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS:
> -	case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS:
> -	case PCI_DEVICE_ID_INTEL_CDF_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_DNV_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_KABYLAKE_PCH_H_SMBUS:
> +		priv->features |= FEATURE_I2C_BLOCK_READ;
> +		priv->features |= FEATURE_IRQ;
> +		priv->features |= FEATURE_SMBUS_PEC;
> +		priv->features |= FEATURE_BLOCK_BUFFER;
> +		/* If we have ACPI based watchdog use that instead */
> +		if (!acpi_has_watchdog())

What about setting the flag unconditionally and moving the call to
acpi_has_watchdog() inside i801_add_tco()? To avoid the duplication of
comment and call, and make future v7 easier too.

> +			priv->features |= FEATURE_TCO_SPT;
> +		priv->features |= FEATURE_HOST_NOTIFY;
> +		break;
> +
> +	case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS:
> +	case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS:
> +	case PCI_DEVICE_ID_INTEL_CDF_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_ICELAKE_LP_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS:
>  	case PCI_DEVICE_ID_INTEL_ELKHART_LAKE_SMBUS:
> @@ -1713,7 +1747,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		priv->features |= FEATURE_BLOCK_BUFFER;
>  		/* If we have ACPI based watchdog use that instead */
>  		if (!acpi_has_watchdog())
> -			priv->features |= FEATURE_TCO;
> +			priv->features |= FEATURE_TCO_CNL;
>  		priv->features |= FEATURE_HOST_NOTIFY;
>  		break;
>  

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c: i801: Use iTCO version 6 in Cannon Lake PCH and beyond

[Mika Westerberg]

On Wed, Aug 28, 2019 at 03:24:20PM +0200, Jean Delvare wrote:
> Hi Mika,

Hi,

> On Fri, 9 Aug 2019 15:45:53 +0300, Mika Westerberg wrote:
> > Intel Cannon Lake PCH moved the NO_REBOOT bit to reside as part of the
> > TCO registers instead so update the i2c-i801 driver so that for Cannon
> > Lake and beyond register platform device for iTCO using version 6. The
> > affected PCHs are Cannon Lake, Cedar Fork, Comet Lake, Elkhart Lake and
> > Ice Lake.
> 
> Looks overall good, with just minor comments inline below.
> 
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 138 +++++++++++++++++++++-------------
> >  1 file changed, 86 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index f2956936c3f2..6918406d00a5 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -292,7 +292,8 @@ struct i801_priv {
> >  #define FEATURE_HOST_NOTIFY	BIT(5)
> >  /* Not really a feature, but it's convenient to handle it as such */
> >  #define FEATURE_IDF		BIT(15)
> > -#define FEATURE_TCO		BIT(16)
> > +#define FEATURE_TCO_SPT		BIT(16)
> > +#define FEATURE_TCO_CNL		BIT(17)
> >  
> >  static const char *i801_feature_names[] = {
> >  	"SMBus PEC",
> > @@ -1491,57 +1492,37 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
> >  }
> >  #endif
> >  
> > -static const struct itco_wdt_platform_data tco_platform_data = {
> > +static const struct itco_wdt_platform_data cnl_tco_platform_data = {
> > +	.name = "Intel PCH",
> > +	.version = 6,
> > +};
> > +
> > +static struct platform_device *
> > +i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
> > +		 struct resource *tco_res)
> > +{
> > +	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> > +					tco_res, 2, &cnl_tco_platform_data,
> > +					sizeof(cnl_tco_platform_data));
> > +}
> > +
> > +static const struct itco_wdt_platform_data spt_tco_platform_data = {
> >  	.name = "Intel PCH",
> >  	.version = 4,
> >  };
> 
> I think it would make more sense to put the oldest code (v4) first and
> then the newest code (v6)?

Makes sense. I'll do that in v2.

> > (...)
> > @@ -1695,13 +1719,23 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  	switch (dev->device) {
> >  	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS:
> > -	case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS:
> > -	case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS:
> > -	case PCI_DEVICE_ID_INTEL_CDF_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_DNV_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_KABYLAKE_PCH_H_SMBUS:
> > +		priv->features |= FEATURE_I2C_BLOCK_READ;
> > +		priv->features |= FEATURE_IRQ;
> > +		priv->features |= FEATURE_SMBUS_PEC;
> > +		priv->features |= FEATURE_BLOCK_BUFFER;
> > +		/* If we have ACPI based watchdog use that instead */
> > +		if (!acpi_has_watchdog())
> 
> What about setting the flag unconditionally and moving the call to
> acpi_has_watchdog() inside i801_add_tco()? To avoid the duplication of
> comment and call, and make future v7 easier too.

Yes, works for me.

> > +			priv->features |= FEATURE_TCO_SPT;
> > +		priv->features |= FEATURE_HOST_NOTIFY;
> > +		break;
> > +
> > +	case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS:
> > +	case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS:
> > +	case PCI_DEVICE_ID_INTEL_CDF_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_ICELAKE_LP_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS:
> >  	case PCI_DEVICE_ID_INTEL_ELKHART_LAKE_SMBUS:
> > @@ -1713,7 +1747,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  		priv->features |= FEATURE_BLOCK_BUFFER;
> >  		/* If we have ACPI based watchdog use that instead */
> >  		if (!acpi_has_watchdog())
> > -			priv->features |= FEATURE_TCO;
> > +			priv->features |= FEATURE_TCO_CNL;
> >  		priv->features |= FEATURE_HOST_NOTIFY;
> >  		break;
> >  
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>

Thanks!