[PATCH v2 0/4] Watchdog: sp5100_tco: Replace watchdog cd6h/cd7h port I/O accesses with MMIO accesses

[PATCH v2 0/4] Watchdog: sp5100_tco: Replace watchdog cd6h/cd7h port I/O accesses with MMIO accesses

[Terry Bowman]

Use MMIO instead of cd6h/cd7h port I/O during EFCH watchdog initialization.
EFCH cd6h/cd7h PIO can be disabled and the recommended workaround is to use
MMIO. As a result MMIO will be used for EFCH SMBus controller address
discovery and starting the EFCH watchdog timer.

Update EFCH detection to support future AMD processors. This will support
new AMD processors without requiring driver modifications.

Patch details:

The first patch refactors watchdog timer initialization into a separate
function. This is needed for future patches. New functional changes are
not added.

The second patch splits out existing memory reservation and address
mapping into new functions. New functional changes are not added.

The third patch introduces EFCH initialization using MMIO. This is
required because cd6h/cd7h port I/O can be disabled on recent AMD hardware.

The fourth patch adds SMBus controller PCI ID check to enable EFCH MMIO
initialization. This eliminates the need for driver updates to support
future processors supporting the same EFCH functionality.

Testing:
Tested on AMD Fam17h and Fam19h processors using:
cat  >> /dev/watchdog

Terry Bowman (4):
  Watchdog: sp5100_tco: Move timer initialization into function
  Watchdog: sp5100_tco: Refactor MMIO base address initialization
  Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using
    MMIO
  Watchdog: sp5100_tco: Enable Family 17h+ CPUs

 drivers/watchdog/sp5100_tco.c | 360 +++++++++++++++++++++++-----------
 drivers/watchdog/sp5100_tco.h |   6 +
 2 files changed, 251 insertions(+), 115 deletions(-)

Co-developed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
To: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Robert Richter <rrichter@amd.com>

Changes in V2:
   - Refactor into 4 patch series
   - Move MMIO reservation and mapping into helper functions
   - Combine mmio_addr and alternate mmio_addr base address discovery
   - Replace efch_use_mmio() with efch_mmio layout type
-- 
2.25.1

[PATCH v2 1/4] Watchdog: sp5100_tco: Move timer initialization into function

[Terry Bowman]

Refactor watchdog timer initialization by moving into new function.

Co-developed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
To: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Robert Richter <rrichter@amd.com>
---
 drivers/watchdog/sp5100_tco.c | 71 +++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index a730ecbf78cd..f5e845c3ecea 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -215,6 +215,44 @@ static u32 sp5100_tco_read_pm_reg32(u8 index)
 	return val;
 }
 
+static int sp5100_tco_timer_init(struct sp5100_tco *tco)
+{
+	struct watchdog_device *wdd = &tco->wdd;
+	struct device *dev = wdd->parent;
+	u32 val;
+
+	/* Setup the watchdog timer */
+	tco_timer_enable(tco);
+
+	val = readl(SP5100_WDT_CONTROL(tco->tcobase));
+	if (val & SP5100_WDT_DISABLED) {
+		dev_err(dev, "Watchdog hardware is disabled\n");
+		return(-ENODEV);
+	}
+
+	/*
+	 * Save WatchDogFired status, because WatchDogFired flag is
+	 * cleared here.
+	 */
+	if (val & SP5100_WDT_FIRED)
+		wdd->bootstatus = WDIOF_CARDRESET;
+
+	/* Set watchdog action to reset the system */
+	val &= ~SP5100_WDT_ACTION_RESET;
+	writel(val, SP5100_WDT_CONTROL(tco->tcobase));
+
+	/* Set a reasonable heartbeat before we stop the timer */
+	tco_timer_set_timeout(wdd, wdd->timeout);
+
+	/*
+	 * Stop the TCO before we change anything so we don't race with
+	 * a zeroed timer.
+	 */
+	tco_timer_stop(wdd);
+
+	return 0;
+}
+
 static int sp5100_tco_setupdevice(struct device *dev,
 				  struct watchdog_device *wdd)
 {
@@ -337,38 +375,7 @@ static int sp5100_tco_setupdevice(struct device *dev,
 
 	dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr);
 
-	/* Setup the watchdog timer */
-	tco_timer_enable(tco);
-
-	val = readl(SP5100_WDT_CONTROL(tco->tcobase));
-	if (val & SP5100_WDT_DISABLED) {
-		dev_err(dev, "Watchdog hardware is disabled\n");
-		ret = -ENODEV;
-		goto unreg_region;
-	}
-
-	/*
-	 * Save WatchDogFired status, because WatchDogFired flag is
-	 * cleared here.
-	 */
-	if (val & SP5100_WDT_FIRED)
-		wdd->bootstatus = WDIOF_CARDRESET;
-	/* Set watchdog action to reset the system */
-	val &= ~SP5100_WDT_ACTION_RESET;
-	writel(val, SP5100_WDT_CONTROL(tco->tcobase));
-
-	/* Set a reasonable heartbeat before we stop the timer */
-	tco_timer_set_timeout(wdd, wdd->timeout);
-
-	/*
-	 * Stop the TCO before we change anything so we don't race with
-	 * a zeroed timer.
-	 */
-	tco_timer_stop(wdd);
-
-	release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
-
-	return 0;
+	ret = sp5100_tco_timer_init(tco);
 
 unreg_region:
 	release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
-- 
2.25.1

[PATCH v2 2/4] Watchdog: sp5100_tco: Refactor MMIO base address initialization

[Terry Bowman]

Move existing MMIO reservation and mapping code into helper functions. The
move locates related MMIO code together.

Also, combine MMIO base address and alternate base address discovery.
Combine based on layout type.

Co-developed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
To: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Robert Richter <rrichter@amd.com>
---
 drivers/watchdog/sp5100_tco.c | 167 +++++++++++++++++++---------------
 1 file changed, 95 insertions(+), 72 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index f5e845c3ecea..80ae42ae7aaa 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -215,6 +215,66 @@ static u32 sp5100_tco_read_pm_reg32(u8 index)
 	return val;
 }
 
+static int __sp5100_tco_prepare_base(struct sp5100_tco *tco,
+				     u32 mmio_addr,
+				     const char *dev_name)
+{
+	struct device *dev = tco->wdd.parent;
+	int ret = 0;
+
+	if (!mmio_addr)
+		return -ENOMEM;
+
+	if (!devm_request_mem_region(dev, mmio_addr,
+				    SP5100_WDT_MEM_MAP_SIZE,
+				    dev_name)) {
+		dev_dbg(dev, "MMIO address 0x%08x already in use\n",
+			mmio_addr);
+		return -EBUSY;
+	}
+
+	tco->tcobase = devm_ioremap(dev, mmio_addr,
+				    SP5100_WDT_MEM_MAP_SIZE);
+	if (!tco->tcobase) {
+		dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n",
+			mmio_addr);
+		devm_release_mem_region(dev, mmio_addr,
+					SP5100_WDT_MEM_MAP_SIZE);
+		return -ENOMEM;
+	}
+
+	dev_info(dev, "Using 0x%08x for watchdog MMIO address\n",
+		 mmio_addr);
+
+	return ret;
+}
+
+static int sp5100_tco_prepare_base(struct sp5100_tco *tco,
+				   u32 mmio_addr,
+				   u32 alt_mmio_addr,
+				   const char *dev_name)
+{
+	struct device *dev = tco->wdd.parent;
+	int ret = 0;
+
+	dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n",
+		mmio_addr);
+
+	/* Check MMIO address conflict */
+	ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
+
+	/* Check alternate MMIO address conflict */
+	if (ret)
+		ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
+						dev_name);
+
+	if (ret)
+		dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X",
+			mmio_addr, alt_mmio_addr, ret);
+
+	return ret;
+}
+
 static int sp5100_tco_timer_init(struct sp5100_tco *tco)
 {
 	struct watchdog_device *wdd = &tco->wdd;
@@ -259,6 +319,7 @@ static int sp5100_tco_setupdevice(struct device *dev,
 	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
 	const char *dev_name;
 	u32 mmio_addr = 0, val;
+	u32 alt_mmio_addr = 0;
 	int ret;
 
 	/* Request the IO ports used by this driver */
@@ -277,11 +338,35 @@ static int sp5100_tco_setupdevice(struct device *dev,
 		dev_name = SP5100_DEVNAME;
 		mmio_addr = sp5100_tco_read_pm_reg32(SP5100_PM_WATCHDOG_BASE) &
 								0xfffffff8;
+
+		/*
+		 * Secondly, Find the watchdog timer MMIO address
+		 * from SBResource_MMIO register.
+		 */
+		/* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */
+		pci_read_config_dword(sp5100_tco_pci,
+				      SP5100_SB_RESOURCE_MMIO_BASE,
+				      &alt_mmio_addr);
+		if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN |
+				      SB800_ACPI_MMIO_SEL) !=
+				     SB800_ACPI_MMIO_DECODE_EN)) {
+			alt_mmio_addr &= ~0xFFF;
+			alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
+		}
 		break;
 	case sb800:
 		dev_name = SB800_DEVNAME;
 		mmio_addr = sp5100_tco_read_pm_reg32(SB800_PM_WATCHDOG_BASE) &
 								0xfffffff8;
+		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
+		alt_mmio_addr =
+			sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN);
+		if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN |
+				       SB800_ACPI_MMIO_SEL)) !=
+		      SB800_ACPI_MMIO_DECODE_EN))) {
+			alt_mmio_addr &= ~0xFFF;
+			alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
+		}
 		break;
 	case efch:
 		dev_name = SB800_DEVNAME;
@@ -300,84 +385,22 @@ static int sp5100_tco_setupdevice(struct device *dev,
 		val = sp5100_tco_read_pm_reg8(EFCH_PM_DECODEEN);
 		if (val & EFCH_PM_DECODEEN_WDT_TMREN)
 			mmio_addr = EFCH_PM_WDT_ADDR;
-		break;
-	default:
-		return -ENODEV;
-	}
 
-	/* Check MMIO address conflict */
-	if (!mmio_addr ||
-	    !devm_request_mem_region(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE,
-				     dev_name)) {
-		if (mmio_addr)
-			dev_dbg(dev, "MMIO address 0x%08x already in use\n",
-				mmio_addr);
-		switch (tco->tco_reg_layout) {
-		case sp5100:
-			/*
-			 * Secondly, Find the watchdog timer MMIO address
-			 * from SBResource_MMIO register.
-			 */
-			/* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */
-			pci_read_config_dword(sp5100_tco_pci,
-					      SP5100_SB_RESOURCE_MMIO_BASE,
-					      &mmio_addr);
-			if ((mmio_addr & (SB800_ACPI_MMIO_DECODE_EN |
-					  SB800_ACPI_MMIO_SEL)) !=
-						  SB800_ACPI_MMIO_DECODE_EN) {
-				ret = -ENODEV;
-				goto unreg_region;
-			}
-			mmio_addr &= ~0xFFF;
-			mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
-			break;
-		case sb800:
-			/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
-			mmio_addr =
-				sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN);
-			if ((mmio_addr & (SB800_ACPI_MMIO_DECODE_EN |
-					  SB800_ACPI_MMIO_SEL)) !=
-						  SB800_ACPI_MMIO_DECODE_EN) {
-				ret = -ENODEV;
-				goto unreg_region;
-			}
-			mmio_addr &= ~0xFFF;
-			mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
-			break;
-		case efch:
-			val = sp5100_tco_read_pm_reg8(EFCH_PM_ISACONTROL);
-			if (!(val & EFCH_PM_ISACONTROL_MMIOEN)) {
-				ret = -ENODEV;
-				goto unreg_region;
-			}
-			mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
-				    EFCH_PM_ACPI_MMIO_WDT_OFFSET;
-			break;
-		}
-		dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n",
-			mmio_addr);
-		if (!devm_request_mem_region(dev, mmio_addr,
-					     SP5100_WDT_MEM_MAP_SIZE,
-					     dev_name)) {
-			dev_dbg(dev, "MMIO address 0x%08x already in use\n",
-				mmio_addr);
-			ret = -EBUSY;
-			goto unreg_region;
+		val = sp5100_tco_read_pm_reg8(EFCH_PM_ISACONTROL);
+		if (val & EFCH_PM_ISACONTROL_MMIOEN) {
+			alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
+				EFCH_PM_ACPI_MMIO_WDT_OFFSET;
 		}
-	}
 
-	tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
-	if (!tco->tcobase) {
-		dev_err(dev, "failed to get tcobase address\n");
-		ret = -ENOMEM;
-		goto unreg_region;
+		break;
+	default:
+		return -ENODEV;
 	}
 
-	dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr);
-
-	ret = sp5100_tco_timer_init(tco);
+	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+	if (!ret)
+		ret = sp5100_tco_timer_init(tco);
 
-unreg_region:
 	release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
-- 
2.25.1

[PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

[Terry Bowman]

cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to
disabled cd6h/cd7h port I/O will return F's and written data is dropped.

The recommended workaround to handle disabled cd6h/cd7h port I/O is
replacing port I/O with MMIO accesses. The MMIO access method has been
available since at least SMBus controllers using PCI revision 0x59.

The EFCH MMIO path is enabled in later patch.

Co-developed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
To: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Robert Richter <rrichter@amd.com>
---
 drivers/watchdog/sp5100_tco.c | 104 +++++++++++++++++++++++++++++++++-
 drivers/watchdog/sp5100_tco.h |   6 ++
 2 files changed, 109 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 80ae42ae7aaa..4777e672a8ad 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -48,12 +48,14 @@
 /* internal variables */
 
 enum tco_reg_layout {
-	sp5100, sb800, efch
+	sp5100, sb800, efch, efch_mmio
 };
 
 struct sp5100_tco {
 	struct watchdog_device wdd;
 	void __iomem *tcobase;
+	void __iomem *addr;
+	struct resource *res;
 	enum tco_reg_layout tco_reg_layout;
 };
 
@@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set)
 	outb(val, SP5100_IO_PM_DATA_REG);
 }
 
+static int sp5100_request_region_mmio(struct device *dev,
+				      struct watchdog_device *wdd)
+{
+	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
+	struct resource *res;
+	void __iomem *addr;
+
+	res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR,
+				 EFCH_PM_ACPI_MMIO_PM_SIZE,
+				 "sp5100_tco");
+
+	if (!res) {
+		dev_err(dev,
+			"SMB base address memory region 0x%x already in use.\n",
+			EFCH_PM_ACPI_MMIO_PM_ADDR);
+		return -EBUSY;
+	}
+
+	addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR,
+		       EFCH_PM_ACPI_MMIO_PM_SIZE);
+	if (!addr) {
+		release_resource(res);
+		dev_err(dev, "SMB base address mapping failed.\n");
+		return -ENOMEM;
+	}
+
+	tco->res = res;
+	tco->addr = addr;
+	return 0;
+}
+
+static void sp5100_release_region_mmio(struct sp5100_tco *tco)
+{
+	iounmap(tco->addr);
+	release_resource(tco->res);
+}
+
+static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index)
+{
+	return readb(tco->addr + index);
+}
+
+static void efch_update_pm_reg8(struct sp5100_tco *tco,
+				u8 index, u8 reset, u8 set)
+{
+	u8 val;
+
+	val = readb(tco->addr + index);
+	val &= reset;
+	val |= set;
+	writeb(val, tco->addr + index);
+}
+
 static void tco_timer_enable(struct sp5100_tco *tco)
 {
 	u32 val;
@@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco)
 					  ~EFCH_PM_WATCHDOG_DISABLE,
 					  EFCH_PM_DECODEEN_SECOND_RES);
 		break;
+	case efch_mmio:
+		/* Set the Watchdog timer resolution to 1 sec and enable */
+		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3,
+				    ~EFCH_PM_WATCHDOG_DISABLE,
+				    EFCH_PM_DECODEEN_SECOND_RES);
+		break;
 	}
 }
 
@@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
 	return 0;
 }
 
+static int sp5100_tco_setupdevice_mmio(struct device *dev,
+				       struct watchdog_device *wdd)
+{
+	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
+	const char *dev_name = SB800_DEVNAME;
+	u32 mmio_addr = 0, alt_mmio_addr = 0;
+	int ret;
+
+	ret = sp5100_request_region_mmio(dev, wdd);
+	if (ret)
+		return ret;
+
+	/* Determine MMIO base address */
+	if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
+	      EFCH_PM_DECODEEN_WDT_TMREN)) {
+		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN,
+				    0xff,
+				    EFCH_PM_DECODEEN_WDT_TMREN);
+	}
+
+	if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
+	    EFCH_PM_DECODEEN_WDT_TMREN)
+		mmio_addr = EFCH_PM_WDT_ADDR;
+
+	/* Determine alternate MMIO base address */
+	if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) &
+	    EFCH_PM_ISACONTROL_MMIOEN)
+		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
+			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
+
+	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+	if (!ret)
+		ret = sp5100_tco_timer_init(tco);
+
+	sp5100_release_region_mmio(tco);
+	return ret;
+}
+
 static int sp5100_tco_setupdevice(struct device *dev,
 				  struct watchdog_device *wdd)
 {
@@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev,
 	u32 alt_mmio_addr = 0;
 	int ret;
 
+	if (tco->tco_reg_layout == efch_mmio)
+		return sp5100_tco_setupdevice_mmio(dev, wdd);
+
 	/* Request the IO ports used by this driver */
 	if (!request_muxed_region(SP5100_IO_PM_INDEX_REG,
 				  SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) {
diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
index adf015aa4126..73f179a1d6e5 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -83,3 +83,9 @@
 
 #define EFCH_PM_ACPI_MMIO_ADDR		0xfed80000
 #define EFCH_PM_ACPI_MMIO_WDT_OFFSET	0x00000b00
+#define EFCH_PM_ACPI_MMIO_PM_OFFSET	0x00000300
+
+#define EFCH_PM_ACPI_MMIO_PM_ADDR	(EFCH_PM_ACPI_MMIO_ADDR +	\
+					 EFCH_PM_ACPI_MMIO_PM_OFFSET)
+#define EFCH_PM_ACPI_MMIO_PM_SIZE       8
+
-- 
2.25.1

Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

[Guenter Roeck]

On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:
> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to
> disabled cd6h/cd7h port I/O will return F's and written data is dropped.
> 
> The recommended workaround to handle disabled cd6h/cd7h port I/O is
> replacing port I/O with MMIO accesses. The MMIO access method has been
> available since at least SMBus controllers using PCI revision 0x59.
> 
> The EFCH MMIO path is enabled in later patch.
> 
> Co-developed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> To: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Robert Richter <rrichter@amd.com>
> ---
>  drivers/watchdog/sp5100_tco.c | 104 +++++++++++++++++++++++++++++++++-
>  drivers/watchdog/sp5100_tco.h |   6 ++
>  2 files changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 80ae42ae7aaa..4777e672a8ad 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -48,12 +48,14 @@
>  /* internal variables */
>  
>  enum tco_reg_layout {
> -	sp5100, sb800, efch
> +	sp5100, sb800, efch, efch_mmio
>  };
>  
>  struct sp5100_tco {
>  	struct watchdog_device wdd;
>  	void __iomem *tcobase;
> +	void __iomem *addr;
> +	struct resource *res;
>  	enum tco_reg_layout tco_reg_layout;
>  };
>  
> @@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set)
>  	outb(val, SP5100_IO_PM_DATA_REG);
>  }
>  
> +static int sp5100_request_region_mmio(struct device *dev,
> +				      struct watchdog_device *wdd)
> +{
> +	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
> +	struct resource *res;
> +	void __iomem *addr;
> +
> +	res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR,
> +				 EFCH_PM_ACPI_MMIO_PM_SIZE,
> +				 "sp5100_tco");
> +
> +	if (!res) {
> +		dev_err(dev,
> +			"SMB base address memory region 0x%x already in use.\n",
> +			EFCH_PM_ACPI_MMIO_PM_ADDR);
> +		return -EBUSY;
> +	}
> +
> +	addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR,
> +		       EFCH_PM_ACPI_MMIO_PM_SIZE);
> +	if (!addr) {
> +		release_resource(res);
> +		dev_err(dev, "SMB base address mapping failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	tco->res = res;
> +	tco->addr = addr;
> +	return 0;
> +}
> +
> +static void sp5100_release_region_mmio(struct sp5100_tco *tco)
> +{
> +	iounmap(tco->addr);
> +	release_resource(tco->res);
> +}
> +
> +static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index)
> +{
> +	return readb(tco->addr + index);
> +}
> +
> +static void efch_update_pm_reg8(struct sp5100_tco *tco,
> +				u8 index, u8 reset, u8 set)
> +{
> +	u8 val;
> +
> +	val = readb(tco->addr + index);
> +	val &= reset;
> +	val |= set;
> +	writeb(val, tco->addr + index);
> +}
> +
>  static void tco_timer_enable(struct sp5100_tco *tco)
>  {
>  	u32 val;
> @@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco)
>  					  ~EFCH_PM_WATCHDOG_DISABLE,
>  					  EFCH_PM_DECODEEN_SECOND_RES);
>  		break;
> +	case efch_mmio:
> +		/* Set the Watchdog timer resolution to 1 sec and enable */
> +		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3,
> +				    ~EFCH_PM_WATCHDOG_DISABLE,
> +				    EFCH_PM_DECODEEN_SECOND_RES);
> +		break;
>  	}
>  }
>  
> @@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
>  	return 0;
>  }
>  
> +static int sp5100_tco_setupdevice_mmio(struct device *dev,
> +				       struct watchdog_device *wdd)
> +{
> +	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
> +	const char *dev_name = SB800_DEVNAME;
> +	u32 mmio_addr = 0, alt_mmio_addr = 0;
> +	int ret;
> +
> +	ret = sp5100_request_region_mmio(dev, wdd);
> +	if (ret)
> +		return ret;
> +
> +	/* Determine MMIO base address */
> +	if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
> +	      EFCH_PM_DECODEEN_WDT_TMREN)) {
> +		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN,
> +				    0xff,
> +				    EFCH_PM_DECODEEN_WDT_TMREN);
> +	}
> +
> +	if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
> +	    EFCH_PM_DECODEEN_WDT_TMREN)
> +		mmio_addr = EFCH_PM_WDT_ADDR;
> +
> +	/* Determine alternate MMIO base address */
> +	if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) &
> +	    EFCH_PM_ISACONTROL_MMIOEN)
> +		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
> +			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
> +
> +	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
> +	if (!ret)
> +		ret = sp5100_tco_timer_init(tco);
> +
> +	sp5100_release_region_mmio(tco);
> +	return ret;
> +}
> +
>  static int sp5100_tco_setupdevice(struct device *dev,
>  				  struct watchdog_device *wdd)
>  {
> @@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev,
>  	u32 alt_mmio_addr = 0;
>  	int ret;
>  
> +	if (tco->tco_reg_layout == efch_mmio)
> +		return sp5100_tco_setupdevice_mmio(dev, wdd);
> +
>  	/* Request the IO ports used by this driver */
>  	if (!request_muxed_region(SP5100_IO_PM_INDEX_REG,
>  				  SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) {
> diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
> index adf015aa4126..73f179a1d6e5 100644
> --- a/drivers/watchdog/sp5100_tco.h
> +++ b/drivers/watchdog/sp5100_tco.h
> @@ -83,3 +83,9 @@
>  
>  #define EFCH_PM_ACPI_MMIO_ADDR		0xfed80000
>  #define EFCH_PM_ACPI_MMIO_WDT_OFFSET	0x00000b00
> +#define EFCH_PM_ACPI_MMIO_PM_OFFSET	0x00000300
> +
> +#define EFCH_PM_ACPI_MMIO_PM_ADDR	(EFCH_PM_ACPI_MMIO_ADDR +	\
> +					 EFCH_PM_ACPI_MMIO_PM_OFFSET)
> +#define EFCH_PM_ACPI_MMIO_PM_SIZE       8
> +

git complains about an empty line at the end of file when
applying this patch.

Guenter

Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

[Guenter Roeck]

On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:
> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to
> disabled cd6h/cd7h port I/O will return F's and written data is dropped.
> 
> The recommended workaround to handle disabled cd6h/cd7h port I/O is
> replacing port I/O with MMIO accesses. The MMIO access method has been
> available since at least SMBus controllers using PCI revision 0x59.
> 
> The EFCH MMIO path is enabled in later patch.
> 
> Co-developed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> To: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Robert Richter <rrichter@amd.com>
> ---
>  drivers/watchdog/sp5100_tco.c | 104 +++++++++++++++++++++++++++++++++-
>  drivers/watchdog/sp5100_tco.h |   6 ++
>  2 files changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 80ae42ae7aaa..4777e672a8ad 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -48,12 +48,14 @@
>  /* internal variables */
>  
>  enum tco_reg_layout {
> -	sp5100, sb800, efch
> +	sp5100, sb800, efch, efch_mmio
>  };
>  
>  struct sp5100_tco {
>  	struct watchdog_device wdd;
>  	void __iomem *tcobase;
> +	void __iomem *addr;
> +	struct resource *res;

I must admit that I really don't like this code. Both res and
addr are only used during initialization, yet their presence suggests
runtime usage. Any chance to reqork this to not require those variables ?

Guenter

>  	enum tco_reg_layout tco_reg_layout;
>  };
>  
> @@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set)
>  	outb(val, SP5100_IO_PM_DATA_REG);
>  }
>  
> +static int sp5100_request_region_mmio(struct device *dev,
> +				      struct watchdog_device *wdd)
> +{
> +	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
> +	struct resource *res;
> +	void __iomem *addr;
> +
> +	res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR,
> +				 EFCH_PM_ACPI_MMIO_PM_SIZE,
> +				 "sp5100_tco");
> +
> +	if (!res) {
> +		dev_err(dev,
> +			"SMB base address memory region 0x%x already in use.\n",
> +			EFCH_PM_ACPI_MMIO_PM_ADDR);
> +		return -EBUSY;
> +	}
> +
> +	addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR,
> +		       EFCH_PM_ACPI_MMIO_PM_SIZE);
> +	if (!addr) {
> +		release_resource(res);
> +		dev_err(dev, "SMB base address mapping failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	tco->res = res;
> +	tco->addr = addr;
> +	return 0;
> +}
> +
> +static void sp5100_release_region_mmio(struct sp5100_tco *tco)
> +{
> +	iounmap(tco->addr);
> +	release_resource(tco->res);
> +}
> +
> +static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index)
> +{
> +	return readb(tco->addr + index);
> +}
> +
> +static void efch_update_pm_reg8(struct sp5100_tco *tco,
> +				u8 index, u8 reset, u8 set)
> +{
> +	u8 val;
> +
> +	val = readb(tco->addr + index);
> +	val &= reset;
> +	val |= set;
> +	writeb(val, tco->addr + index);
> +}
> +
>  static void tco_timer_enable(struct sp5100_tco *tco)
>  {
>  	u32 val;
> @@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco)
>  					  ~EFCH_PM_WATCHDOG_DISABLE,
>  					  EFCH_PM_DECODEEN_SECOND_RES);
>  		break;
> +	case efch_mmio:
> +		/* Set the Watchdog timer resolution to 1 sec and enable */
> +		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3,
> +				    ~EFCH_PM_WATCHDOG_DISABLE,
> +				    EFCH_PM_DECODEEN_SECOND_RES);
> +		break;
>  	}
>  }
>  
> @@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
>  	return 0;
>  }
>  
> +static int sp5100_tco_setupdevice_mmio(struct device *dev,
> +				       struct watchdog_device *wdd)
> +{
> +	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
> +	const char *dev_name = SB800_DEVNAME;
> +	u32 mmio_addr = 0, alt_mmio_addr = 0;
> +	int ret;
> +
> +	ret = sp5100_request_region_mmio(dev, wdd);
> +	if (ret)
> +		return ret;
> +
> +	/* Determine MMIO base address */
> +	if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
> +	      EFCH_PM_DECODEEN_WDT_TMREN)) {
> +		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN,
> +				    0xff,
> +				    EFCH_PM_DECODEEN_WDT_TMREN);
> +	}
> +
> +	if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
> +	    EFCH_PM_DECODEEN_WDT_TMREN)
> +		mmio_addr = EFCH_PM_WDT_ADDR;
> +
> +	/* Determine alternate MMIO base address */
> +	if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) &
> +	    EFCH_PM_ISACONTROL_MMIOEN)
> +		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
> +			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
> +
> +	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
> +	if (!ret)
> +		ret = sp5100_tco_timer_init(tco);
> +
> +	sp5100_release_region_mmio(tco);
> +	return ret;
> +}
> +
>  static int sp5100_tco_setupdevice(struct device *dev,
>  				  struct watchdog_device *wdd)
>  {
> @@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev,
>  	u32 alt_mmio_addr = 0;
>  	int ret;
>  
> +	if (tco->tco_reg_layout == efch_mmio)
> +		return sp5100_tco_setupdevice_mmio(dev, wdd);
> +
>  	/* Request the IO ports used by this driver */
>  	if (!request_muxed_region(SP5100_IO_PM_INDEX_REG,
>  				  SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) {
> diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
> index adf015aa4126..73f179a1d6e5 100644
> --- a/drivers/watchdog/sp5100_tco.h
> +++ b/drivers/watchdog/sp5100_tco.h
> @@ -83,3 +83,9 @@
>  
>  #define EFCH_PM_ACPI_MMIO_ADDR		0xfed80000
>  #define EFCH_PM_ACPI_MMIO_WDT_OFFSET	0x00000b00
> +#define EFCH_PM_ACPI_MMIO_PM_OFFSET	0x00000300
> +
> +#define EFCH_PM_ACPI_MMIO_PM_ADDR	(EFCH_PM_ACPI_MMIO_ADDR +	\
> +					 EFCH_PM_ACPI_MMIO_PM_OFFSET)
> +#define EFCH_PM_ACPI_MMIO_PM_SIZE       8
> +

Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

[Terry Bowman]

+ Tom Lendacky

On 1/6/22 12:18 PM, Guenter Roeck wrote:
> On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:
>> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to
>> disabled cd6h/cd7h port I/O will return F's and written data is dropped.
>>
>> The recommended workaround to handle disabled cd6h/cd7h port I/O is
>> replacing port I/O with MMIO accesses. The MMIO access method has been
>> available since at least SMBus controllers using PCI revision 0x59.
>>
>> The EFCH MMIO path is enabled in later patch.
>>
>> Co-developed-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> To: linux-watchdog@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Robert Richter <rrichter@amd.com>
>> ---
>>  drivers/watchdog/sp5100_tco.c | 104 +++++++++++++++++++++++++++++++++-
>>  drivers/watchdog/sp5100_tco.h |   6 ++
>>  2 files changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>> index 80ae42ae7aaa..4777e672a8ad 100644
>> --- a/drivers/watchdog/sp5100_tco.c
>> +++ b/drivers/watchdog/sp5100_tco.c
>> @@ -48,12 +48,14 @@
>>  /* internal variables */
>>  
>>  enum tco_reg_layout {
>> -	sp5100, sb800, efch
>> +	sp5100, sb800, efch, efch_mmio
>>  };
>>  
>>  struct sp5100_tco {
>>  	struct watchdog_device wdd;
>>  	void __iomem *tcobase;
>> +	void __iomem *addr;
>> +	struct resource *res;
> 
> I must admit that I really don't like this code. Both res and
> addr are only used during initialization, yet their presence suggests
> runtime usage. Any chance to reqork this to not require those variables ?
> 
> Guenter
> 

Hi Guenter,

Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also 
correct the trailing newline you mentioned in an earlier email.

Regards,
Terry

>>  	enum tco_reg_layout tco_reg_layout;
>>  };
>>  
>> @@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set)
>>  	outb(val, SP5100_IO_PM_DATA_REG);
>>  }
>>  
>> +static int sp5100_request_region_mmio(struct device *dev,
>> +				      struct watchdog_device *wdd)
>> +{
>> +	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
>> +	struct resource *res;
>> +	void __iomem *addr;
>> +
>> +	res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR,
>> +				 EFCH_PM_ACPI_MMIO_PM_SIZE,
>> +				 "sp5100_tco");
>> +
>> +	if (!res) {
>> +		dev_err(dev,
>> +			"SMB base address memory region 0x%x already in use.\n",
>> +			EFCH_PM_ACPI_MMIO_PM_ADDR);
>> +		return -EBUSY;
>> +	}
>> +
>> +	addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR,
>> +		       EFCH_PM_ACPI_MMIO_PM_SIZE);
>> +	if (!addr) {
>> +		release_resource(res);
>> +		dev_err(dev, "SMB base address mapping failed.\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	tco->res = res;
>> +	tco->addr = addr;
>> +	return 0;
>> +}
>> +
>> +static void sp5100_release_region_mmio(struct sp5100_tco *tco)
>> +{
>> +	iounmap(tco->addr);
>> +	release_resource(tco->res);
>> +}
>> +
>> +static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index)
>> +{
>> +	return readb(tco->addr + index);
>> +}
>> +
>> +static void efch_update_pm_reg8(struct sp5100_tco *tco,
>> +				u8 index, u8 reset, u8 set)
>> +{
>> +	u8 val;
>> +
>> +	val = readb(tco->addr + index);
>> +	val &= reset;
>> +	val |= set;
>> +	writeb(val, tco->addr + index);
>> +}
>> +
>>  static void tco_timer_enable(struct sp5100_tco *tco)
>>  {
>>  	u32 val;
>> @@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco)
>>  					  ~EFCH_PM_WATCHDOG_DISABLE,
>>  					  EFCH_PM_DECODEEN_SECOND_RES);
>>  		break;
>> +	case efch_mmio:
>> +		/* Set the Watchdog timer resolution to 1 sec and enable */
>> +		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3,
>> +				    ~EFCH_PM_WATCHDOG_DISABLE,
>> +				    EFCH_PM_DECODEEN_SECOND_RES);
>> +		break;
>>  	}
>>  }
>>  
>> @@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
>>  	return 0;
>>  }
>>  
>> +static int sp5100_tco_setupdevice_mmio(struct device *dev,
>> +				       struct watchdog_device *wdd)
>> +{
>> +	struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
>> +	const char *dev_name = SB800_DEVNAME;
>> +	u32 mmio_addr = 0, alt_mmio_addr = 0;
>> +	int ret;
>> +
>> +	ret = sp5100_request_region_mmio(dev, wdd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Determine MMIO base address */
>> +	if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
>> +	      EFCH_PM_DECODEEN_WDT_TMREN)) {
>> +		efch_update_pm_reg8(tco, EFCH_PM_DECODEEN,
>> +				    0xff,
>> +				    EFCH_PM_DECODEEN_WDT_TMREN);
>> +	}
>> +
>> +	if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) &
>> +	    EFCH_PM_DECODEEN_WDT_TMREN)
>> +		mmio_addr = EFCH_PM_WDT_ADDR;
>> +
>> +	/* Determine alternate MMIO base address */
>> +	if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) &
>> +	    EFCH_PM_ISACONTROL_MMIOEN)
>> +		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
>> +			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
>> +
>> +	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
>> +	if (!ret)
>> +		ret = sp5100_tco_timer_init(tco);
>> +
>> +	sp5100_release_region_mmio(tco);
>> +	return ret;
>> +}
>> +
>>  static int sp5100_tco_setupdevice(struct device *dev,
>>  				  struct watchdog_device *wdd)
>>  {
>> @@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev,
>>  	u32 alt_mmio_addr = 0;
>>  	int ret;
>>  
>> +	if (tco->tco_reg_layout == efch_mmio)
>> +		return sp5100_tco_setupdevice_mmio(dev, wdd);
>> +
>>  	/* Request the IO ports used by this driver */
>>  	if (!request_muxed_region(SP5100_IO_PM_INDEX_REG,
>>  				  SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) {
>> diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
>> index adf015aa4126..73f179a1d6e5 100644
>> --- a/drivers/watchdog/sp5100_tco.h
>> +++ b/drivers/watchdog/sp5100_tco.h
>> @@ -83,3 +83,9 @@
>>  
>>  #define EFCH_PM_ACPI_MMIO_ADDR		0xfed80000
>>  #define EFCH_PM_ACPI_MMIO_WDT_OFFSET	0x00000b00
>> +#define EFCH_PM_ACPI_MMIO_PM_OFFSET	0x00000300
>> +
>> +#define EFCH_PM_ACPI_MMIO_PM_ADDR	(EFCH_PM_ACPI_MMIO_ADDR +	\
>> +					 EFCH_PM_ACPI_MMIO_PM_OFFSET)
>> +#define EFCH_PM_ACPI_MMIO_PM_SIZE       8
>> +

Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

[Robert Richter]

On 06.01.22 13:07:11, Terry Bowman wrote:
> On 1/6/22 12:18 PM, Guenter Roeck wrote:
> > On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:

> >> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> >> index 80ae42ae7aaa..4777e672a8ad 100644
> >> --- a/drivers/watchdog/sp5100_tco.c
> >> +++ b/drivers/watchdog/sp5100_tco.c
> >> @@ -48,12 +48,14 @@
> >>  /* internal variables */
> >>  
> >>  enum tco_reg_layout {
> >> -	sp5100, sb800, efch
> >> +	sp5100, sb800, efch, efch_mmio
> >>  };
> >>  
> >>  struct sp5100_tco {
> >>  	struct watchdog_device wdd;
> >>  	void __iomem *tcobase;
> >> +	void __iomem *addr;
> >> +	struct resource *res;
> > 
> > I must admit that I really don't like this code. Both res and
> > addr are only used during initialization, yet their presence suggests
> > runtime usage. Any chance to reqork this to not require those variables ?

We did that in an earlier version, see struct efch_cfg of:

 https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/

The motivation of it was the same as you suggested to only use it
during init.

Having it in struct sp5100_tco made things simpler esp. in the
definition of the function interfaces where those new members are
used.

If that init vars are no longer in struct sp5100_tco then callers of
efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a
pointer to them. To avoid this I see those options:

1) Implement them as global (or a single global struct) and possibly
protect it by a mutex. There is only a single device anyway and we
wouldn't need a protection.

2) Have an own mmio implementation of tco_timer_enable() and/or
sp5100_tco_timer_init().

> Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also 
> correct the trailing newline you mentioned in an earlier email.
> 
> Regards,
> Terry
> 
> >>  	enum tco_reg_layout tco_reg_layout;

While at it, tco_reg_layout is also only used during initialization
and can be moved there too. This would raise option 3:

3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that
struct instead in init funtions only. But that causes the most rework
(which would be ok to me).

Going with 3) looks the cleanest way, I would try that. But all
options have its advantages.

-Robert

> >>  };

Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

[Guenter Roeck]

On 1/7/22 3:05 AM, Robert Richter wrote:
> On 06.01.22 13:07:11, Terry Bowman wrote:
>> On 1/6/22 12:18 PM, Guenter Roeck wrote:
>>> On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:
> 
>>>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>>>> index 80ae42ae7aaa..4777e672a8ad 100644
>>>> --- a/drivers/watchdog/sp5100_tco.c
>>>> +++ b/drivers/watchdog/sp5100_tco.c
>>>> @@ -48,12 +48,14 @@
>>>>   /* internal variables */
>>>>   
>>>>   enum tco_reg_layout {
>>>> -	sp5100, sb800, efch
>>>> +	sp5100, sb800, efch, efch_mmio
>>>>   };
>>>>   
>>>>   struct sp5100_tco {
>>>>   	struct watchdog_device wdd;
>>>>   	void __iomem *tcobase;
>>>> +	void __iomem *addr;
>>>> +	struct resource *res;
>>>
>>> I must admit that I really don't like this code. Both res and
>>> addr are only used during initialization, yet their presence suggests
>>> runtime usage. Any chance to reqork this to not require those variables ?
> 
> We did that in an earlier version, see struct efch_cfg of:
> 
>   https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/
> 
> The motivation of it was the same as you suggested to only use it
> during init.
> 
> Having it in struct sp5100_tco made things simpler esp. in the
> definition of the function interfaces where those new members are
> used.
> 

'res' is only used in the context of sp5100_request_region_mmio()
and sp5100_release_region_mmio(), and both are called from
sp5100_tco_setupdevice_mmio(). I do not see a need for carrying it around
anywhere else, including efch_read_pm_reg8() and efch_update_pm_reg8().

efch_read_pm_reg8() is only called from sp5100_tco_setupdevice_mmio(),
and it only uses struct sp5100_tco *tco to get the address. I don't see
why the address can not be passed to it directly.

efch_update_pm_reg8() also uses tco only to get the address. Again, passing
it instead of a pointer to sp5100_tco *tco would easily be possible.

efch_update_pm_reg8() is only called fromm sp5100_tco_setupdevice_mmio(),
where the change would be easy. It is also called from tco_timer_enable(),
which in turn is called from sp5100_tco_timer_init(). This, again, is called
from sp5100_tco_setupdevice_mmio(). Since the first operation in
sp5100_tco_timer_init() is to call tco_timer_enable() and that function
does nothing but calling efch_update_pm_reg8(), it would easily be possible
to pull out that code from tco_timer_enable() and move it into
sp5100_tco_setupdevice_mmio().

So, no, I neither see the need for having the information in struct
sp5100_tco nor for keeping it in its own structure. If you'd merge
sp5100_request_region_mmio() and sp5100_release_region_mmio() into
sp5100_tco_setupdevice_mmio() you would not even need any pointers
to pass the values from sp5100_request_region_mmio(). Otherwise you
could have sp5100_request_region_mmio() return a pointer to res or
an ERR_PTR, and pass the address as pointer parameter.

Guenter

> If that init vars are no longer in struct sp5100_tco then callers of
> efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a
> pointer to them. To avoid this I see those options:
> 
> 1) Implement them as global (or a single global struct) and possibly
> protect it by a mutex. There is only a single device anyway and we
> wouldn't need a protection.
> 
> 2) Have an own mmio implementation of tco_timer_enable() and/or
> sp5100_tco_timer_init().
> 
>> Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also
>> correct the trailing newline you mentioned in an earlier email.
>>
>> Regards,
>> Terry
>>
>>>>   	enum tco_reg_layout tco_reg_layout;
> 
> While at it, tco_reg_layout is also only used during initialization
> and can be moved there too. This would raise option 3:
> 
> 3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that
> struct instead in init funtions only. But that causes the most rework
> (which would be ok to me).
> 
> Going with 3) looks the cleanest way, I would try that. But all
> options have its advantages.
> 
> -Robert
> 
>>>>   };

Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

[Robert Richter]

On 07.01.22 09:12:32, Guenter Roeck wrote:
> On 1/7/22 3:05 AM, Robert Richter wrote:
> > On 06.01.22 13:07:11, Terry Bowman wrote:
> > > On 1/6/22 12:18 PM, Guenter Roeck wrote:
> > > > On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:
> > 
> > > > > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> > > > > index 80ae42ae7aaa..4777e672a8ad 100644
> > > > > --- a/drivers/watchdog/sp5100_tco.c
> > > > > +++ b/drivers/watchdog/sp5100_tco.c
> > > > > @@ -48,12 +48,14 @@
> > > > >   /* internal variables */
> > > > >   enum tco_reg_layout {
> > > > > -	sp5100, sb800, efch
> > > > > +	sp5100, sb800, efch, efch_mmio
> > > > >   };
> > > > >   struct sp5100_tco {
> > > > >   	struct watchdog_device wdd;
> > > > >   	void __iomem *tcobase;
> > > > > +	void __iomem *addr;
> > > > > +	struct resource *res;
> > > > 
> > > > I must admit that I really don't like this code. Both res and
> > > > addr are only used during initialization, yet their presence suggests
> > > > runtime usage. Any chance to reqork this to not require those variables ?
> > 
> > We did that in an earlier version, see struct efch_cfg of:
> > 
> >   https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/
> > 
> > The motivation of it was the same as you suggested to only use it
> > during init.
> > 
> > Having it in struct sp5100_tco made things simpler esp. in the
> > definition of the function interfaces where those new members are
> > used.

> So, no, I neither see the need for having the information in struct
> sp5100_tco nor for keeping it in its own structure. If you'd merge
> sp5100_request_region_mmio() and sp5100_release_region_mmio() into
> sp5100_tco_setupdevice_mmio() you would not even need any pointers
> to pass the values from sp5100_request_region_mmio(). Otherwise you
> could have sp5100_request_region_mmio() return a pointer to res or
> an ERR_PTR, and pass the address as pointer parameter.

Yes, that is feasible, in fact it is option 2) I suggested.

-Robert

> > If that init vars are no longer in struct sp5100_tco then callers of
> > efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a
> > pointer to them. To avoid this I see those options:
> > 
> > 1) Implement them as global (or a single global struct) and possibly
> > protect it by a mutex. There is only a single device anyway and we
> > wouldn't need a protection.
> > 
> > 2) Have an own mmio implementation of tco_timer_enable() and/or
> > sp5100_tco_timer_init().
> > 
> > > Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also
> > > correct the trailing newline you mentioned in an earlier email.
> > > 
> > > Regards,
> > > Terry
> > > 
> > > > >   	enum tco_reg_layout tco_reg_layout;
> > 
> > While at it, tco_reg_layout is also only used during initialization
> > and can be moved there too. This would raise option 3:
> > 
> > 3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that
> > struct instead in init funtions only. But that causes the most rework
> > (which would be ok to me).
> > 
> > Going with 3) looks the cleanest way, I would try that. But all
> > options have its advantages.
> > 
> > -Robert
> > 
> > > > >   };
> 

[PATCH v2 4/4] Watchdog: sp5100_tco: Enable Family 17h+ CPUs

[Terry Bowman]

The driver currently uses a CPU family match of 17h to determine
EFCH_PM_DECODEEN_WDT_TMREN register support. This family check will not
support future AMD CPUs and instead will require driver updates to add
support.

Remove the Fam17h family check and add a check for SMBus PCI revision ID
0x59. This will support Fam17h and future AMD processors including EFCH
functionality without requiring driver changes.

Co-developed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
To: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Robert Richter <rrichter@amd.com>
---
 drivers/watchdog/sp5100_tco.c | 16 ++++------------
 drivers/watchdog/sp5100_tco.h |  2 +-
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 4777e672a8ad..8930b94aae47 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -87,6 +87,10 @@ static enum tco_reg_layout tco_reg_layout(struct pci_dev *dev)
 	    dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
 	    dev->revision < 0x40) {
 		return sp5100;
+	} else if (dev->vendor == PCI_VENDOR_ID_AMD &&
+	    sp5100_tco_pci->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS &&
+	    sp5100_tco_pci->revision >= AMD_ZEN_SMBUS_PCI_REV) {
+		return efch_mmio;
 	} else if (dev->vendor == PCI_VENDOR_ID_AMD &&
 	    ((dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
 	     dev->revision >= 0x41) ||
@@ -472,18 +476,6 @@ static int sp5100_tco_setupdevice(struct device *dev,
 		break;
 	case efch:
 		dev_name = SB800_DEVNAME;
-		/*
-		 * On Family 17h devices, the EFCH_PM_DECODEEN_WDT_TMREN bit of
-		 * EFCH_PM_DECODEEN not only enables the EFCH_PM_WDT_ADDR memory
-		 * region, it also enables the watchdog itself.
-		 */
-		if (boot_cpu_data.x86 == 0x17) {
-			val = sp5100_tco_read_pm_reg8(EFCH_PM_DECODEEN);
-			if (!(val & EFCH_PM_DECODEEN_WDT_TMREN)) {
-				sp5100_tco_update_pm_reg8(EFCH_PM_DECODEEN, 0xff,
-							  EFCH_PM_DECODEEN_WDT_TMREN);
-			}
-		}
 		val = sp5100_tco_read_pm_reg8(EFCH_PM_DECODEEN);
 		if (val & EFCH_PM_DECODEEN_WDT_TMREN)
 			mmio_addr = EFCH_PM_WDT_ADDR;
diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
index 73f179a1d6e5..3a37bffc96b4 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -88,4 +88,4 @@
 #define EFCH_PM_ACPI_MMIO_PM_ADDR	(EFCH_PM_ACPI_MMIO_ADDR +	\
 					 EFCH_PM_ACPI_MMIO_PM_OFFSET)
 #define EFCH_PM_ACPI_MMIO_PM_SIZE       8
-
+#define AMD_ZEN_SMBUS_PCI_REV           0x59
-- 
2.25.1