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 <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: Terry Bowman <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: Wim Van Sebroeck <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Robert Richter <[email protected]>
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
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 <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: Terry Bowman <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: Wim Van Sebroeck <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Robert Richter <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: Terry Bowman <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: Wim Van Sebroeck <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Robert Richter <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> Signed-off-by: Terry Bowman <[email protected]>
> To: [email protected]
> Cc: [email protected]
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Robert Richter <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> Signed-off-by: Terry Bowman <[email protected]>
> To: [email protected]
> Cc: [email protected]
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Robert Richter <[email protected]>
> ---
> 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
> +
+ 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 <[email protected]>
>> Signed-off-by: Robert Richter <[email protected]>
>> Signed-off-by: Terry Bowman <[email protected]>
>> To: [email protected]
>> Cc: [email protected]
>> Cc: Wim Van Sebroeck <[email protected]>
>> Cc: Guenter Roeck <[email protected]>
>> Cc: Robert Richter <[email protected]>
>> ---
>> 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
>> +
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/[email protected]/
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
> >> };
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/[email protected]/
>
> 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
>
>>>> };
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/[email protected]/
> >
> > 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
> >
> > > > > };
>