This driver uses cd6h/cd7h port I/O to access the FCH::PM::DECODEEN
register during driver initialization. cd6h/cd7h port I/O is no longer
supported on later AMD processors and the recommended method to access
this register is using MMIO. This series will replace the cd6h/cd7h port
I/O with MMIO accesses during initialization.
The first patch refactors watchdog timer initialization into a separate
function. This is needed to add support for new device layouts without
adding complexity.
The second patch moves region request/release into new functions. The
request/release functions provide a location for adding MMIO region
support.
The third patch introduces EFCH initialization using MMIO. This is
required because the registers are no longer accessible using cd6h/cd7h
port I/O.
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.
This series includes patches with MMIO accesses to register
FCH::PM::DECODEEN. The same register is also accessed by the piix4_smbus
driver. Both drivers use request_mem_region_muxed() to synchronize the
accesses. request_mem_region_muxed() definition is added in parallel
piix4_smbus patchset review with review URL provided below as a dependency.
Dependency:
Link: https://lore.kernel.org/linux-i2c/[email protected]/
Based on v5.16
Testing:
Tested on AMD family 17h and family 19h processors using:
cat >> /dev/watchdog
Hi Jean,
Please confirm to leave your reviewed-by and tested-by.
Changes in V4:
- Change to only call devm_ioremap() once. (Guenter Roeck, Jean Delvare)
- Remove trailing dot for consistency with the other messages.
(Jean Delvare)
- Update print formatting in sp5100_tco_prepare_base(). Change period to
a comma, use '0x%x', and change return code to decimal display.
(Jean Delvare)
- Move dev_err() linebreak to 'dev,' in sp5100_tco_prepare_base().
(Jean Delvare)
- Remove unused variable. (Andy Shevchenko)
- Remove unnecessary assignment in sp5100_tco_prepare_base().
(Andy Shevchenko)
- Unify comment in sp5100_tco_prepare_base(). (Andy Shevchenko)
- Fix line break for readability in 'if' in sp5100_tco_prepare_base().
(Andy Shevchenko)
- Fix logic issue in 'if' in sp5100_tco_setupdevice(). Added temp
variable val. (Terry Bowman, Jean Delvare)
- Change capitalized letters to lowercase in sp5100_tco_prepare_base().
(Andy Shevchenko)
- Add dependency note for piix4_smbus driver. (Andy Shevchenko)
- Change "SMB" -> "SMBus". (Jean Delvare)
- Add comment for logic in sp5100_tco_setupdevice_mmio(). (Jean Delvare)
- Fix 2 locations of line breaks in sp5100_tco_setupdevice_mmio().
(Jean Delvare)
Changes in V3:
- Remove 'addr' and 'res' variables from struct sp5100_tco.
(Guenter Roeck)
- Pass address directly to efch_read_pm_reg8() and
efch_update_pm_reg8(). (Guenter Roeck)
- Reword patch descriptions. (Terry Bowman)
- Change #define AMD_ZEN_SMBUS_PCI_REV value from 0x59 to 0x51. This was
determined after investigating programmers manual and testing.
(Robert Richter)
- Refactor efch_* functions() (Robert Richter)
- Remove trailing whitespace in patch. (Guenter Roeck)
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
Terry Bowman (4):
Watchdog: sp5100_tco: Move timer initialization into function
Watchdog: sp5100_tco: Refactor MMIO base address initialization
Watchdog: sp5100_tco: Add initialization using EFCH MMIO
Watchdog: sp5100_tco: Enable Family 17h+ CPUs
drivers/watchdog/sp5100_tco.c | 334 ++++++++++++++++++++++------------
drivers/watchdog/sp5100_tco.h | 7 +
2 files changed, 226 insertions(+), 115 deletions(-)
--
2.30.2
Combine MMIO base address and alternate base address detection. Combine
based on layout type. This will simplify the function by eliminating
a switch case.
Move existing request/release code into functions. This currently only
supports port I/O request/release. The move into a separate function
will make it ready for adding MMIO region support.
Co-developed-by: Robert Richter <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: Terry Bowman <[email protected]>
Tested-by: Jean Delvare <[email protected]>
Reviewed-by: Jean Delvare <[email protected]>
---
drivers/watchdog/sp5100_tco.c | 155 ++++++++++++++++++----------------
drivers/watchdog/sp5100_tco.h | 1 +
2 files changed, 82 insertions(+), 74 deletions(-)
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index b365bbc9ac36..16e122d5045e 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -223,6 +223,55 @@ static u32 sp5100_tco_read_pm_reg32(u8 index)
return val;
}
+static u32 sp5100_tco_request_region(struct device *dev,
+ u32 mmio_addr,
+ const char *dev_name)
+{
+ 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 0;
+ }
+
+ return mmio_addr;
+}
+
+static u32 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;
+
+ dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", mmio_addr);
+
+ if (!mmio_addr && !alt_mmio_addr)
+ return -ENODEV;
+
+ /* Check for MMIO address and alternate MMIO address conflicts */
+ if (mmio_addr)
+ mmio_addr = sp5100_tco_request_region(dev, mmio_addr, dev_name);
+
+ if (!mmio_addr && alt_mmio_addr)
+ mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
+
+ if (!mmio_addr) {
+ dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
+ return -EBUSY;
+ }
+
+ tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
+ if (!tco->tcobase) {
+ dev_err(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", tco->tcobase);
+
+ return 0;
+}
+
static int sp5100_tco_timer_init(struct sp5100_tco *tco)
{
struct watchdog_device *wdd = &tco->wdd;
@@ -264,6 +313,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 */
@@ -282,11 +332,32 @@ 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,
+ &val);
+
+ /* Verify MMIO is enabled and using bar0 */
+ if ((val & SB800_ACPI_MMIO_MASK) == SB800_ACPI_MMIO_DECODE_EN)
+ alt_mmio_addr = (val & ~0xfff) + 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) */
+ val = sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN);
+
+ /* Verify MMIO is enabled and using bar0 */
+ if ((val & SB800_ACPI_MMIO_MASK) == SB800_ACPI_MMIO_DECODE_EN)
+ alt_mmio_addr = (val & ~0xfff) + SB800_PM_WDT_MMIO_OFFSET;
break;
case efch:
dev_name = SB800_DEVNAME;
@@ -305,87 +376,23 @@ 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;
+
+ 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;
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;
- }
- }
-
- 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;
+ ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+ if (!ret) {
+ /* Setup the watchdog timer */
+ tco_timer_enable(tco);
+ ret = sp5100_tco_timer_init(tco);
}
- dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr);
-
- /* Setup the watchdog timer */
- tco_timer_enable(tco);
-
- ret = sp5100_tco_timer_init(tco);
-
-unreg_region:
release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
return ret;
}
diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
index adf015aa4126..daee872f9b71 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -58,6 +58,7 @@
#define SB800_PM_WATCHDOG_SECOND_RES GENMASK(1, 0)
#define SB800_ACPI_MMIO_DECODE_EN BIT(0)
#define SB800_ACPI_MMIO_SEL BIT(1)
+#define SB800_ACPI_MMIO_MASK GENMASK(1, 0)
#define SB800_PM_WDT_MMIO_OFFSET 0xB00
--
2.30.2
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 family 17h family check and add a check for SMBus PCI
revision ID 0x51 or greater. The MMIO access method has been available
since at least SMBus controllers using PCI revision 0x51. This revision
check will support family 17h 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]>
Tested-by: Jean Delvare <[email protected]>
Reviewed-by: Jean Delvare <[email protected]>
---
drivers/watchdog/sp5100_tco.c | 16 ++++------------
drivers/watchdog/sp5100_tco.h | 1 +
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 1a5e76d13c3c..b25b353fdf36 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -86,6 +86,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) ||
@@ -459,18 +463,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 8ca1b215e3ce..6a0986d2c94b 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -89,3 +89,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 0x51
--
2.30.2
Refactor driver's timer initialization into new function. This is needed
inorder to support adding new device layouts while using common timer
initialization.
Co-developed-by: Robert Richter <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: Terry Bowman <[email protected]>
Tested-by: Jean Delvare <[email protected]>
Reviewed-by: Jean Delvare <[email protected]>
---
drivers/watchdog/sp5100_tco.c | 65 +++++++++++++++++++----------------
1 file changed, 36 insertions(+), 29 deletions(-)
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index dd9a744f82f8..b365bbc9ac36 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -223,6 +223,41 @@ 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;
+
+ 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)
{
@@ -348,35 +383,7 @@ static int sp5100_tco_setupdevice(struct device *dev,
/* 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.30.2
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. It is recommended to replace the cd6h/cd7h
port I/O with MMIO.
Co-developed-by: Robert Richter <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: Terry Bowman <[email protected]>
Tested-by: Jean Delvare <[email protected]>
Reviewed-by: Jean Delvare <[email protected]>
---
drivers/watchdog/sp5100_tco.c | 100 +++++++++++++++++++++++++++++++++-
drivers/watchdog/sp5100_tco.h | 5 ++
2 files changed, 104 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 16e122d5045e..1a5e76d13c3c 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -49,7 +49,7 @@
/* internal variables */
enum tco_reg_layout {
- sp5100, sb800, efch
+ sp5100, sb800, efch, efch_mmio
};
struct sp5100_tco {
@@ -209,6 +209,8 @@ static void tco_timer_enable(struct sp5100_tco *tco)
~EFCH_PM_WATCHDOG_DISABLE,
EFCH_PM_DECODEEN_SECOND_RES);
break;
+ default:
+ break;
}
}
@@ -307,6 +309,99 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
return 0;
}
+static u8 efch_read_pm_reg8(void __iomem *addr, u8 index)
+{
+ return readb(addr + index);
+}
+
+static void efch_update_pm_reg8(void __iomem *addr, u8 index, u8 reset, u8 set)
+{
+ u8 val;
+
+ val = readb(addr + index);
+ val &= reset;
+ val |= set;
+ writeb(val, addr + index);
+}
+
+static void tco_timer_enable_mmio(void __iomem *addr)
+{
+ efch_update_pm_reg8(addr, EFCH_PM_DECODEEN3,
+ ~EFCH_PM_WATCHDOG_DISABLE,
+ EFCH_PM_DECODEEN_SECOND_RES);
+}
+
+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;
+ struct resource *res;
+ void __iomem *addr;
+ int ret;
+ u32 val;
+
+ res = request_mem_region_muxed(EFCH_PM_ACPI_MMIO_PM_ADDR,
+ EFCH_PM_ACPI_MMIO_PM_SIZE,
+ "sp5100_tco");
+
+ if (!res) {
+ dev_err(dev,
+ "Memory region 0x%08x 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) {
+ dev_err(dev, "Address mapping failed\n");
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ /*
+ * EFCH_PM_DECODEEN_WDT_TMREN is dual purpose. This bitfield
+ * enables sp5100_tco register MMIO space decoding. The bitfield
+ * also starts the timer operation. Enable if not already enabled.
+ */
+ val = efch_read_pm_reg8(addr, EFCH_PM_DECODEEN);
+ if (!(val & EFCH_PM_DECODEEN_WDT_TMREN)) {
+ efch_update_pm_reg8(addr, EFCH_PM_DECODEEN, 0xff,
+ EFCH_PM_DECODEEN_WDT_TMREN);
+ }
+
+ /* Error if the timer could not be enabled */
+ val = efch_read_pm_reg8(addr, EFCH_PM_DECODEEN);
+ if (!(val & EFCH_PM_DECODEEN_WDT_TMREN)) {
+ dev_err(dev, "Failed to enable the timer\n");
+ ret = -EFAULT;
+ goto out;
+ }
+
+ mmio_addr = EFCH_PM_WDT_ADDR;
+
+ /* Determine alternate MMIO base address */
+ val = efch_read_pm_reg8(addr, EFCH_PM_ISACONTROL);
+ if (val & 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) {
+ tco_timer_enable_mmio(addr);
+ ret = sp5100_tco_timer_init(tco);
+ }
+
+out:
+ if (addr)
+ iounmap(addr);
+
+ release_resource(res);
+
+ return ret;
+}
+
static int sp5100_tco_setupdevice(struct device *dev,
struct watchdog_device *wdd)
{
@@ -316,6 +411,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 daee872f9b71..8ca1b215e3ce 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -83,4 +83,9 @@
#define EFCH_PM_ISACONTROL_MMIOEN BIT(1)
#define EFCH_PM_ACPI_MMIO_ADDR 0xfed80000
+#define EFCH_PM_ACPI_MMIO_PM_OFFSET 0x00000300
#define EFCH_PM_ACPI_MMIO_WDT_OFFSET 0x00000b00
+
+#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.30.2
Hi Terry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-m001-20220131 (https://download.01.org/0day-ci/archive/20220131/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/e6855f66e7e915cd09a8f8dae411997df8a7c641
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
git checkout e6855f66e7e915cd09a8f8dae411997df8a7c641
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/watchdog/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/pci.h:37,
from drivers/watchdog/sp5100_tco.c:40:
drivers/watchdog/sp5100_tco.c: In function 'sp5100_tco_prepare_base':
>> drivers/watchdog/sp5100_tco.c:270:16: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'void *' [-Wformat=]
270 | dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:16: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:150:51: note: in expansion of macro 'dev_fmt'
150 | dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/watchdog/sp5100_tco.c:270:2: note: in expansion of macro 'dev_info'
270 | dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
| ^~~~~~~~
drivers/watchdog/sp5100_tco.c:270:28: note: format string is defined here
270 | dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
| ~~~^
| |
| unsigned int
| %08p
vim +270 drivers/watchdog/sp5100_tco.c
238
239 static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
240 u32 mmio_addr,
241 u32 alt_mmio_addr,
242 const char *dev_name)
243 {
244 struct device *dev = tco->wdd.parent;
245
246 dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", mmio_addr);
247
248 if (!mmio_addr && !alt_mmio_addr)
249 return -ENODEV;
250
251 /* Check for MMIO address and alternate MMIO address conflicts */
252 if (mmio_addr)
253 mmio_addr = sp5100_tco_request_region(dev, mmio_addr, dev_name);
254
255 if (!mmio_addr && alt_mmio_addr)
256 mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
257
258 if (!mmio_addr) {
259 dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
260 return -EBUSY;
261 }
262
263 tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
264 if (!tco->tcobase) {
265 dev_err(dev, "MMIO address 0x%08x failed mapping\n", mmio_addr);
266 devm_release_mem_region(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
267 return -ENOMEM;
268 }
269
> 270 dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
271
272 return 0;
273 }
274
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Terry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-a013-20220131 (https://download.01.org/0day-ci/archive/20220131/[email protected]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project f1c18acb07aa40f42b87b70462a6d1ab77a4825c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e6855f66e7e915cd09a8f8dae411997df8a7c641
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
git checkout e6855f66e7e915cd09a8f8dae411997df8a7c641
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/watchdog/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/watchdog/sp5100_tco.c:270:60: warning: format specifies type 'unsigned int' but the argument has type 'void *' [-Wformat]
dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
~~~~ ^~~~~~~~~~~~
include/linux/dev_printk.h:150:67: note: expanded from macro 'dev_info'
dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
_p_func(dev, fmt, ##__VA_ARGS__); \
~~~ ^~~~~~~~~~~
1 warning generated.
vim +270 drivers/watchdog/sp5100_tco.c
238
239 static u32 sp5100_tco_prepare_base(struct sp5100_tco *tco,
240 u32 mmio_addr,
241 u32 alt_mmio_addr,
242 const char *dev_name)
243 {
244 struct device *dev = tco->wdd.parent;
245
246 dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", mmio_addr);
247
248 if (!mmio_addr && !alt_mmio_addr)
249 return -ENODEV;
250
251 /* Check for MMIO address and alternate MMIO address conflicts */
252 if (mmio_addr)
253 mmio_addr = sp5100_tco_request_region(dev, mmio_addr, dev_name);
254
255 if (!mmio_addr && alt_mmio_addr)
256 mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
257
258 if (!mmio_addr) {
259 dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
260 return -EBUSY;
261 }
262
263 tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
264 if (!tco->tcobase) {
265 dev_err(dev, "MMIO address 0x%08x failed mapping\n", mmio_addr);
266 devm_release_mem_region(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
267 return -ENOMEM;
268 }
269
> 270 dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
271
272 return 0;
273 }
274
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Terry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-m001-20220131 (https://download.01.org/0day-ci/archive/20220131/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/92f6f8c644fc7df3d1f3f8e32f8b1f4efc3f321f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
git checkout 92f6f8c644fc7df3d1f3f8e32f8b1f4efc3f321f
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/watchdog/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/pci.h:37,
from drivers/watchdog/sp5100_tco.c:40:
drivers/watchdog/sp5100_tco.c: In function 'sp5100_tco_prepare_base':
drivers/watchdog/sp5100_tco.c:272:16: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'void *' [-Wformat=]
272 | dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:16: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:150:51: note: in expansion of macro 'dev_fmt'
150 | dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/watchdog/sp5100_tco.c:272:2: note: in expansion of macro 'dev_info'
272 | dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
| ^~~~~~~~
drivers/watchdog/sp5100_tco.c:272:28: note: format string is defined here
272 | dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
| ~~~^
| |
| unsigned int
| %08p
drivers/watchdog/sp5100_tco.c: In function 'sp5100_tco_setupdevice_mmio':
drivers/watchdog/sp5100_tco.c:345:8: error: implicit declaration of function 'request_mem_region_muxed'; did you mean 'request_mem_region'? [-Werror=implicit-function-declaration]
345 | res = request_mem_region_muxed(EFCH_PM_ACPI_MMIO_PM_ADDR,
| ^~~~~~~~~~~~~~~~~~~~~~~~
| request_mem_region
>> drivers/watchdog/sp5100_tco.c:345:6: warning: assignment to 'struct resource *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
345 | res = request_mem_region_muxed(EFCH_PM_ACPI_MMIO_PM_ADDR,
| ^
cc1: some warnings being treated as errors
vim +345 drivers/watchdog/sp5100_tco.c
333
334 static int sp5100_tco_setupdevice_mmio(struct device *dev,
335 struct watchdog_device *wdd)
336 {
337 struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
338 const char *dev_name = SB800_DEVNAME;
339 u32 mmio_addr = 0, alt_mmio_addr = 0;
340 struct resource *res;
341 void __iomem *addr;
342 int ret;
343 u32 val;
344
> 345 res = request_mem_region_muxed(EFCH_PM_ACPI_MMIO_PM_ADDR,
346 EFCH_PM_ACPI_MMIO_PM_SIZE,
347 "sp5100_tco");
348
349 if (!res) {
350 dev_err(dev,
351 "Memory region 0x%08x already in use\n",
352 EFCH_PM_ACPI_MMIO_PM_ADDR);
353 return -EBUSY;
354 }
355
356 addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, EFCH_PM_ACPI_MMIO_PM_SIZE);
357 if (!addr) {
358 dev_err(dev, "Address mapping failed\n");
359 ret = -ENOMEM;
360 goto out;
361 }
362
363 /*
364 * EFCH_PM_DECODEEN_WDT_TMREN is dual purpose. This bitfield
365 * enables sp5100_tco register MMIO space decoding. The bitfield
366 * also starts the timer operation. Enable if not already enabled.
367 */
368 val = efch_read_pm_reg8(addr, EFCH_PM_DECODEEN);
369 if (!(val & EFCH_PM_DECODEEN_WDT_TMREN)) {
370 efch_update_pm_reg8(addr, EFCH_PM_DECODEEN, 0xff,
371 EFCH_PM_DECODEEN_WDT_TMREN);
372 }
373
374 /* Error if the timer could not be enabled */
375 val = efch_read_pm_reg8(addr, EFCH_PM_DECODEEN);
376 if (!(val & EFCH_PM_DECODEEN_WDT_TMREN)) {
377 dev_err(dev, "Failed to enable the timer\n");
378 ret = -EFAULT;
379 goto out;
380 }
381
382 mmio_addr = EFCH_PM_WDT_ADDR;
383
384 /* Determine alternate MMIO base address */
385 val = efch_read_pm_reg8(addr, EFCH_PM_ISACONTROL);
386 if (val & EFCH_PM_ISACONTROL_MMIOEN)
387 alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
388 EFCH_PM_ACPI_MMIO_WDT_OFFSET;
389
390 ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
391 if (!ret) {
392 tco_timer_enable_mmio(addr);
393 ret = sp5100_tco_timer_init(tco);
394 }
395
396 out:
397 if (addr)
398 iounmap(addr);
399
400 release_resource(res);
401
402 return ret;
403 }
404
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Terry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-a013-20220131 (https://download.01.org/0day-ci/archive/20220131/[email protected]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project f1c18acb07aa40f42b87b70462a6d1ab77a4825c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/92f6f8c644fc7df3d1f3f8e32f8b1f4efc3f321f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Terry-Bowman/Watchdog-sp5100_tco-Replace-cd6h-cd7h-port-I-O-accesses-with-MMIO-accesses/20220131-031525
git checkout 92f6f8c644fc7df3d1f3f8e32f8b1f4efc3f321f
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/watchdog/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/watchdog/sp5100_tco.c:272:60: warning: format specifies type 'unsigned int' but the argument has type 'void *' [-Wformat]
dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", tco->tcobase);
~~~~ ^~~~~~~~~~~~
include/linux/dev_printk.h:150:67: note: expanded from macro 'dev_info'
dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
_p_func(dev, fmt, ##__VA_ARGS__); \
~~~ ^~~~~~~~~~~
drivers/watchdog/sp5100_tco.c:345:8: error: implicit declaration of function 'request_mem_region_muxed' [-Werror,-Wimplicit-function-declaration]
res = request_mem_region_muxed(EFCH_PM_ACPI_MMIO_PM_ADDR,
^
>> drivers/watchdog/sp5100_tco.c:345:6: warning: incompatible integer to pointer conversion assigning to 'struct resource *' from 'int' [-Wint-conversion]
res = request_mem_region_muxed(EFCH_PM_ACPI_MMIO_PM_ADDR,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings and 1 error generated.
vim +345 drivers/watchdog/sp5100_tco.c
333
334 static int sp5100_tco_setupdevice_mmio(struct device *dev,
335 struct watchdog_device *wdd)
336 {
337 struct sp5100_tco *tco = watchdog_get_drvdata(wdd);
338 const char *dev_name = SB800_DEVNAME;
339 u32 mmio_addr = 0, alt_mmio_addr = 0;
340 struct resource *res;
341 void __iomem *addr;
342 int ret;
343 u32 val;
344
> 345 res = request_mem_region_muxed(EFCH_PM_ACPI_MMIO_PM_ADDR,
346 EFCH_PM_ACPI_MMIO_PM_SIZE,
347 "sp5100_tco");
348
349 if (!res) {
350 dev_err(dev,
351 "Memory region 0x%08x already in use\n",
352 EFCH_PM_ACPI_MMIO_PM_ADDR);
353 return -EBUSY;
354 }
355
356 addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, EFCH_PM_ACPI_MMIO_PM_SIZE);
357 if (!addr) {
358 dev_err(dev, "Address mapping failed\n");
359 ret = -ENOMEM;
360 goto out;
361 }
362
363 /*
364 * EFCH_PM_DECODEEN_WDT_TMREN is dual purpose. This bitfield
365 * enables sp5100_tco register MMIO space decoding. The bitfield
366 * also starts the timer operation. Enable if not already enabled.
367 */
368 val = efch_read_pm_reg8(addr, EFCH_PM_DECODEEN);
369 if (!(val & EFCH_PM_DECODEEN_WDT_TMREN)) {
370 efch_update_pm_reg8(addr, EFCH_PM_DECODEEN, 0xff,
371 EFCH_PM_DECODEEN_WDT_TMREN);
372 }
373
374 /* Error if the timer could not be enabled */
375 val = efch_read_pm_reg8(addr, EFCH_PM_DECODEEN);
376 if (!(val & EFCH_PM_DECODEEN_WDT_TMREN)) {
377 dev_err(dev, "Failed to enable the timer\n");
378 ret = -EFAULT;
379 goto out;
380 }
381
382 mmio_addr = EFCH_PM_WDT_ADDR;
383
384 /* Determine alternate MMIO base address */
385 val = efch_read_pm_reg8(addr, EFCH_PM_ISACONTROL);
386 if (val & EFCH_PM_ISACONTROL_MMIOEN)
387 alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
388 EFCH_PM_ACPI_MMIO_WDT_OFFSET;
389
390 ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
391 if (!ret) {
392 tco_timer_enable_mmio(addr);
393 ret = sp5100_tco_timer_init(tco);
394 }
395
396 out:
397 if (addr)
398 iounmap(addr);
399
400 release_resource(res);
401
402 return ret;
403 }
404
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 2/1/22 12:32, Terry Bowman wrote:
>
>
> On 2/1/22 10:46, Terry Bowman wrote:
>>
>>
>> On 2/1/22 09:31, Guenter Roeck wrote:
>>> On 1/30/22 11:12, Terry Bowman wrote:
>>>> Combine MMIO base address and alternate base address detection. Combine
>>>> based on layout type. This will simplify the function by eliminating
>>>> a switch case.
>>>>
>>>> Move existing request/release code into functions. This currently only
>>>> supports port I/O request/release. The move into a separate function
>>>> will make it ready for adding MMIO region support.
>>>>
>>>> Co-developed-by: Robert Richter <[email protected]>
>>>> Signed-off-by: Robert Richter <[email protected]>
>>>> Signed-off-by: Terry Bowman <[email protected]>
>>>> Tested-by: Jean Delvare <[email protected]>
>>>> Reviewed-by: Jean Delvare <[email protected]>
>>>> ---
>>>> drivers/watchdog/sp5100_tco.c | 155 ++++++++++++++++++----------------
>>>> drivers/watchdog/sp5100_tco.h | 1 +
>>>> 2 files changed, 82 insertions(+), 74 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>>>> index b365bbc9ac36..16e122d5045e 100644
>>>> --- a/drivers/watchdog/sp5100_tco.c
>>>> +++ b/drivers/watchdog/sp5100_tco.c
>>>> @@ -223,6 +223,55 @@ static u32 sp5100_tco_read_pm_reg32(u8 index)
>>>> return val;
>>>> }
>>>> +static u32 sp5100_tco_request_region(struct device *dev,
>>>> + u32 mmio_addr,
>>>> + const char *dev_name)
>>>> +{
>>>> + 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 0;
>>>> + }
>>>> +
>>>> + return mmio_addr;
>>>> +}
>>>> +
>>>> +static u32 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;
>>>> +
>>>> + dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", mmio_addr);
>>>> +
>>>> + if (!mmio_addr && !alt_mmio_addr)
>>>> + return -ENODEV;
>>>> +
>>>> + /* Check for MMIO address and alternate MMIO address conflicts */
>>>> + if (mmio_addr)
>>>> + mmio_addr = sp5100_tco_request_region(dev, mmio_addr, dev_name);
>>>> +
>>>> + if (!mmio_addr && alt_mmio_addr)
>>>> + mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
>>>> +
>>>> + if (!mmio_addr) {
>>>> + dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
>>>> + return -EBUSY;
>>>> + }
>>>> +
>>>> + tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
>>>> + if (!tco->tcobase) {
>>>> + dev_err(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", tco->tcobase);
>>>> +
>>>
>>> I know this is the same as the old code, but I think it would make sense to change
>>> this as suggested by 0-day and use %px instead.
>>>
>>> Thanks,
>>> Guenter
>>
>>
> Hi Guenter,
>
> This line was changed in v4 and should be reverted. It should be using the mmio_addr
> physical address with '0x%08x' formatting instead of tco->tcobase. This would be:
>
> dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr);
>
> The dmesg then provides:
>
> [ 5.972921] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver
> [ 5.978238] sp5100-tco sp5100-tco: Using 0xfeb00000 for watchdog MMIO address
> [ 5.978252] sp5100-tco sp5100-tco: Watchdog hardware is disabled
>
> Do your agree?
>
Yes. Displaying the mapped address has zero if any value.
Guenter
On 1/30/22 11:12, Terry Bowman wrote:
> Combine MMIO base address and alternate base address detection. Combine
> based on layout type. This will simplify the function by eliminating
> a switch case.
>
> Move existing request/release code into functions. This currently only
> supports port I/O request/release. The move into a separate function
> will make it ready for adding MMIO region support.
>
> Co-developed-by: Robert Richter <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> Signed-off-by: Terry Bowman <[email protected]>
> Tested-by: Jean Delvare <[email protected]>
> Reviewed-by: Jean Delvare <[email protected]>
> ---
> drivers/watchdog/sp5100_tco.c | 155 ++++++++++++++++++----------------
> drivers/watchdog/sp5100_tco.h | 1 +
> 2 files changed, 82 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index b365bbc9ac36..16e122d5045e 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -223,6 +223,55 @@ static u32 sp5100_tco_read_pm_reg32(u8 index)
> return val;
> }
>
> +static u32 sp5100_tco_request_region(struct device *dev,
> + u32 mmio_addr,
> + const char *dev_name)
> +{
> + 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 0;
> + }
> +
> + return mmio_addr;
> +}
> +
> +static u32 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;
> +
> + dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", mmio_addr);
> +
> + if (!mmio_addr && !alt_mmio_addr)
> + return -ENODEV;
> +
> + /* Check for MMIO address and alternate MMIO address conflicts */
> + if (mmio_addr)
> + mmio_addr = sp5100_tco_request_region(dev, mmio_addr, dev_name);
> +
> + if (!mmio_addr && alt_mmio_addr)
> + mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
> +
> + if (!mmio_addr) {
> + dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
> + return -EBUSY;
> + }
> +
> + tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
> + if (!tco->tcobase) {
> + dev_err(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", tco->tcobase);
> +
I know this is the same as the old code, but I think it would make sense to change
this as suggested by 0-day and use %px instead.
Thanks,
Guenter
On 2/1/22 09:31, Guenter Roeck wrote:
> On 1/30/22 11:12, Terry Bowman wrote:
>> Combine MMIO base address and alternate base address detection. Combine
>> based on layout type. This will simplify the function by eliminating
>> a switch case.
>>
>> Move existing request/release code into functions. This currently only
>> supports port I/O request/release. The move into a separate function
>> will make it ready for adding MMIO region support.
>>
>> Co-developed-by: Robert Richter <[email protected]>
>> Signed-off-by: Robert Richter <[email protected]>
>> Signed-off-by: Terry Bowman <[email protected]>
>> Tested-by: Jean Delvare <[email protected]>
>> Reviewed-by: Jean Delvare <[email protected]>
>> ---
>> drivers/watchdog/sp5100_tco.c | 155 ++++++++++++++++++----------------
>> drivers/watchdog/sp5100_tco.h | 1 +
>> 2 files changed, 82 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>> index b365bbc9ac36..16e122d5045e 100644
>> --- a/drivers/watchdog/sp5100_tco.c
>> +++ b/drivers/watchdog/sp5100_tco.c
>> @@ -223,6 +223,55 @@ static u32 sp5100_tco_read_pm_reg32(u8 index)
>> return val;
>> }
>> +static u32 sp5100_tco_request_region(struct device *dev,
>> + u32 mmio_addr,
>> + const char *dev_name)
>> +{
>> + 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 0;
>> + }
>> +
>> + return mmio_addr;
>> +}
>> +
>> +static u32 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;
>> +
>> + dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", mmio_addr);
>> +
>> + if (!mmio_addr && !alt_mmio_addr)
>> + return -ENODEV;
>> +
>> + /* Check for MMIO address and alternate MMIO address conflicts */
>> + if (mmio_addr)
>> + mmio_addr = sp5100_tco_request_region(dev, mmio_addr, dev_name);
>> +
>> + if (!mmio_addr && alt_mmio_addr)
>> + mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
>> +
>> + if (!mmio_addr) {
>> + dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
>> + return -EBUSY;
>> + }
>> +
>> + tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
>> + if (!tco->tcobase) {
>> + dev_err(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", tco->tcobase);
>> +
>
> I know this is the same as the old code, but I think it would make sense to change
> this as suggested by 0-day and use %px instead.
>
> Thanks,
> Guenter
Hi Guenter,
I'll add the change to v5.
Regards,
Terry
On 2/1/22 10:46, Terry Bowman wrote:
>
>
> On 2/1/22 09:31, Guenter Roeck wrote:
>> On 1/30/22 11:12, Terry Bowman wrote:
>>> Combine MMIO base address and alternate base address detection. Combine
>>> based on layout type. This will simplify the function by eliminating
>>> a switch case.
>>>
>>> Move existing request/release code into functions. This currently only
>>> supports port I/O request/release. The move into a separate function
>>> will make it ready for adding MMIO region support.
>>>
>>> Co-developed-by: Robert Richter <[email protected]>
>>> Signed-off-by: Robert Richter <[email protected]>
>>> Signed-off-by: Terry Bowman <[email protected]>
>>> Tested-by: Jean Delvare <[email protected]>
>>> Reviewed-by: Jean Delvare <[email protected]>
>>> ---
>>> drivers/watchdog/sp5100_tco.c | 155 ++++++++++++++++++----------------
>>> drivers/watchdog/sp5100_tco.h | 1 +
>>> 2 files changed, 82 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>>> index b365bbc9ac36..16e122d5045e 100644
>>> --- a/drivers/watchdog/sp5100_tco.c
>>> +++ b/drivers/watchdog/sp5100_tco.c
>>> @@ -223,6 +223,55 @@ static u32 sp5100_tco_read_pm_reg32(u8 index)
>>> return val;
>>> }
>>> +static u32 sp5100_tco_request_region(struct device *dev,
>>> + u32 mmio_addr,
>>> + const char *dev_name)
>>> +{
>>> + 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 0;
>>> + }
>>> +
>>> + return mmio_addr;
>>> +}
>>> +
>>> +static u32 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;
>>> +
>>> + dev_dbg(dev, "Got 0x%08x from SBResource_MMIO register\n", mmio_addr);
>>> +
>>> + if (!mmio_addr && !alt_mmio_addr)
>>> + return -ENODEV;
>>> +
>>> + /* Check for MMIO address and alternate MMIO address conflicts */
>>> + if (mmio_addr)
>>> + mmio_addr = sp5100_tco_request_region(dev, mmio_addr, dev_name);
>>> +
>>> + if (!mmio_addr && alt_mmio_addr)
>>> + mmio_addr = sp5100_tco_request_region(dev, alt_mmio_addr, dev_name);
>>> +
>>> + if (!mmio_addr) {
>>> + dev_err(dev, "Failed to reserve MMIO or alternate MMIO region\n");
>>> + return -EBUSY;
>>> + }
>>> +
>>> + tco->tcobase = devm_ioremap(dev, mmio_addr, SP5100_WDT_MEM_MAP_SIZE);
>>> + if (!tco->tcobase) {
>>> + dev_err(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", tco->tcobase);
>>> +
>>
>> I know this is the same as the old code, but I think it would make sense to change
>> this as suggested by 0-day and use %px instead.
>>
>> Thanks,
>> Guenter
>
>
Hi Guenter,
This line was changed in v4 and should be reverted. It should be using the mmio_addr
physical address with '0x%08x' formatting instead of tco->tcobase. This would be:
dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr);
The dmesg then provides:
[ 5.972921] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver
[ 5.978238] sp5100-tco sp5100-tco: Using 0xfeb00000 for watchdog MMIO address
[ 5.978252] sp5100-tco sp5100-tco: Watchdog hardware is disabled
Do your agree?
Regards,
Terry
Hi Terry,
On Sun, 30 Jan 2022 13:12:21 -0600, Terry Bowman wrote:
> Please confirm to leave your reviewed-by and tested-by.
Confirmed. I reviewed the 4 patches of this version of the series and
I'm fine with them. I also tested the result successfully on my laptop.
Reviewed-by: Jean Delvare <[email protected]>
Tested-by: Jean Delvare <[email protected]>
Thanks for your work.
--
Jean Delvare
SUSE L3 Support
> I'm still reviewing these, sorry. I only picked the first patch of the
> series so that the sp5100_tco patches would build so I could test them.
Ah, I see. I thought more than the first patch was needed for testing.
> I hope to be done by the end of the day.
Awesome, thank you!
> Confirmed. I reviewed the 4 patches of this version of the series and
> I'm fine with them. I also tested the result successfully on my laptop.
>
> Reviewed-by: Jean Delvare <[email protected]>
> Tested-by: Jean Delvare <[email protected]>
Does that mean you are happy with the i2c-piix4 changes as well?
On Tue, 8 Feb 2022 11:00:31 +0100, Wolfram Sang wrote:
> > I'm still reviewing these, sorry. I only picked the first patch of the
> > series so that the sp5100_tco patches would build so I could test them.
>
> Ah, I see. I thought more than the first patch was needed for testing.
You need the full series to be on the safe side, otherwise there's a
risk that the two drivers will access the same registers using
different methods (legacy I/O vs MMIO) so there's no synchronization
and they could step on each other's toes.
However as I knew about this limitation, I was careful to not use the
SMBus driver while I was testing the watchdog driver :-)
--
Jean Delvare
SUSE L3 Support
On Mon, 7 Feb 2022 14:03:08 +0100, Wolfram Sang wrote:
> > Confirmed. I reviewed the 4 patches of this version of the series and
> > I'm fine with them. I also tested the result successfully on my laptop.
> >
> > Reviewed-by: Jean Delvare <[email protected]>
> > Tested-by: Jean Delvare <[email protected]>
>
> Does that mean you are happy with the i2c-piix4 changes as well?
I'm still reviewing these, sorry. I only picked the first patch of the
series so that the sp5100_tco patches would build so I could test them.
I hope to be done by the end of the day.
--
Jean Delvare
SUSE L3 Support