This driver uses cd6h/cd7h port I/O to access the FCH::PM::DECODEEN and
FCH::PM::ISACONTROL registers during driver initialization. cd6h/cd7h port
I/O is no longer supported on later AMD processors and the recommended
method to access these registers 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.
Important:
This series includes patches with MMIO accesses to registers
FCH::PM::DECODEEN and FCH::PM::ISACONTROL. The same registers are also
accessed by the piix4_smbus driver. The registers are currently accessed
indirectly through cd6h/cd7h port I/O and both drivers use
request_muxed_region() to synchronize the accesses. It should be noted the
request_muxed_region() uses a wait queue to sleep and retry taking
exclusive access if the port I/O region is busy.
This series uses request_mem_region() to synchronize accesses to the MMIO
registers mentioned above. request_mem_region() is missing the retry
logic in the case the resource is busy. As a result, request_mem_region()
will fail immediately if the resource is busy. The 'muxed' variant is
needed here but request_muxed_mem_region() is not defined to use. I will
follow up with another patch series to define the
request_muxed_mem_region() and use in both drivers.
The piix4_smbus driver or the sp5100_tco driver can potentialy fail until
the muxed mem synchronization series is present in the tree. The potential
for failure is not likely because the sp5100_tco driver only accesses the
FCH::PM::DECODEEN and FCH::PM::ISACONTROL registers during driver
initialization.
Link: https://lore.kernel.org/all/[email protected]/#t
Based on v5.16
Testing:
Tested on AMD family 17h and family 19h processors using:
cat >> /dev/watchdog
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)
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 | 335 ++++++++++++++++++++++------------
drivers/watchdog/sp5100_tco.h | 6 +
2 files changed, 227 insertions(+), 114 deletions(-)
--
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]>
To: Guenter Roeck <[email protected]>
To: [email protected]
To: Jean Delvare <[email protected]>
To: [email protected]
To: Wolfram Sang <[email protected]>
To: Andy Shevchenko <[email protected]>
To: Rafael J. Wysocki <[email protected]>
Cc: [email protected]
Cc: Wim Van Sebroeck <[email protected]>
Cc: Robert Richter <[email protected]>
Cc: Thomas Lendacky <[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..ecc273b9b17f 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
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]>
To: Guenter Roeck <[email protected]>
To: [email protected]
To: Jean Delvare <[email protected]>
To: [email protected]
To: Wolfram Sang <[email protected]>
To: Andy Shevchenko <[email protected]>
To: Rafael J. Wysocki <[email protected]>
Cc: [email protected]
Cc: Wim Van Sebroeck <[email protected]>
Cc: Robert Richter <[email protected]>
Cc: Thomas Lendacky <[email protected]>
---
drivers/watchdog/sp5100_tco.c | 168 +++++++++++++++++++---------------
1 file changed, 95 insertions(+), 73 deletions(-)
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index ecc273b9b17f..64ecebd93403 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -223,6 +223,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;
@@ -264,6 +324,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 +343,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;
@@ -305,87 +390,24 @@ 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;
- }
- }
+ ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
+ if (!ret) {
+ /* Setup the watchdog timer */
+ tco_timer_enable(tco);
- 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_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;
}
--
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]>
To: Guenter Roeck <[email protected]>
To: [email protected]
To: Jean Delvare <[email protected]>
To: [email protected]
To: Wolfram Sang <[email protected]>
To: Andy Shevchenko <[email protected]>
To: Rafael J. Wysocki <[email protected]>
Cc: [email protected]
Cc: Wim Van Sebroeck <[email protected]>
Cc: Robert Richter <[email protected]>
Cc: Thomas Lendacky <[email protected]>
---
drivers/watchdog/sp5100_tco.c | 88 ++++++++++++++++++++++++++++++++++-
drivers/watchdog/sp5100_tco.h | 5 ++
2 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 64ecebd93403..36519a992ca1 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;
}
}
@@ -318,6 +320,87 @@ 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;
+
+ 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;
+ }
+
+ if (!(efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
+ EFCH_PM_DECODEEN_WDT_TMREN)) {
+ efch_update_pm_reg8(addr, EFCH_PM_DECODEEN,
+ 0xff,
+ EFCH_PM_DECODEEN_WDT_TMREN);
+ }
+
+ /* Determine MMIO base address */
+ if (efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
+ EFCH_PM_DECODEEN_WDT_TMREN)
+ mmio_addr = EFCH_PM_WDT_ADDR;
+
+ /* Determine alternate MMIO base address */
+ if (efch_read_pm_reg8(addr, 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) {
+ tco_timer_enable_mmio(addr);
+ ret = sp5100_tco_timer_init(tco);
+ }
+
+ iounmap(addr);
+ release_resource(res);
+
+ return ret;
+}
+
static int sp5100_tco_setupdevice(struct device *dev,
struct watchdog_device *wdd)
{
@@ -327,6 +410,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..2df8f8b2c55b 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -83,3 +83,8 @@
#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.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]>
To: Guenter Roeck <[email protected]>
To: [email protected]
To: Jean Delvare <[email protected]>
To: [email protected]
To: Wolfram Sang <[email protected]>
To: Andy Shevchenko <[email protected]>
To: Rafael J. Wysocki <[email protected]>
Cc: [email protected]
Cc: Wim Van Sebroeck <[email protected]>
Cc: Robert Richter <[email protected]>
Cc: Thomas Lendacky <[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 36519a992ca1..b949dcd9f780 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) ||
@@ -461,18 +465,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 2df8f8b2c55b..4fac39a2f12f 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -88,3 +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 0x51
--
2.30.2
On Tue, Jan 18, 2022 at 10:22 PM Terry Bowman <[email protected]> wrote:
>
> 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]>
> To: Guenter Roeck <[email protected]>
> To: [email protected]
> To: Jean Delvare <[email protected]>
> To: [email protected]
> To: Wolfram Sang <[email protected]>
> To: Andy Shevchenko <[email protected]>
> To: Rafael J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Robert Richter <[email protected]>
> Cc: Thomas Lendacky <[email protected]>
Please, do not pollute commit messages with this rather unnecessary
list of recipients. There are (at least?) two possibilities:
- use --cc and --to whe run `git send-email`
- move them under the cutter '--- ' line below
> ---
> + val = readl(SP5100_WDT_CONTROL(tco->tcobase));
> + if (val & SP5100_WDT_DISABLED) {
> + dev_err(dev, "Watchdog hardware is disabled\n");
> + return(-ENODEV);
Missed space, too many parentheses.
> + }
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <[email protected]> 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.
...
> To: Guenter Roeck <[email protected]>
> To: [email protected]
> To: Jean Delvare <[email protected]>
> To: [email protected]
> To: Wolfram Sang <[email protected]>
> To: Andy Shevchenko <[email protected]>
> To: Rafael J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Robert Richter <[email protected]>
> Cc: Thomas Lendacky <[email protected]>
Same comment to all your patches.
...
> +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;
Not really used variable.
> + 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);
Why? If it's a short live mapping, do not use devm.
> + return -ENOMEM;
> + }
> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n",
> + mmio_addr);
Unneeded noise.
> + return ret;
On top of above it's a NIH devm_ioremap_resource().
> +}
...
> + int ret = 0;
Redundant assignment.
...
> + /* Check MMIO address conflict */
> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
> +
> + /* Check alternate MMIO address conflict */
Unify this with the previous comment.
> + if (ret)
> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
> + dev_name);
...
> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN |
> + SB800_ACPI_MMIO_SEL) !=
> + SB800_ACPI_MMIO_DECODE_EN)) {
The split looks ugly. Consider to use temporary variables or somehow
rearrange the condition that it doesn't break in the middle of the one
logical token.
> + alt_mmio_addr &= ~0xFFF;
Why capital letters?
> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
> + }
...
> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN |
> + SB800_ACPI_MMIO_SEL)) !=
> + SB800_ACPI_MMIO_DECODE_EN))) {
Ditto.
> + alt_mmio_addr &= ~0xFFF;
Ditto.
> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
...
Okay, I see this is the original code like this... Perhaps it makes
sense to reshuffle them (indentation-wise) at the same time and
mention this in the changelog.
...
> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
Is it still needed? I have no context to say if devm_iomap() and this
are not colliding, please double check the correctness.
--
With Best Regards,
Andy Shevchenko
Hi Terry,
On Tue, 18 Jan 2022 14:22:30 -0600, Terry Bowman wrote:
> This series uses request_mem_region() to synchronize accesses to the MMIO
> registers mentioned above. request_mem_region() is missing the retry
> logic in the case the resource is busy. As a result, request_mem_region()
> will fail immediately if the resource is busy. The 'muxed' variant is
> needed here but request_muxed_mem_region() is not defined to use. I will
> follow up with another patch series to define the
> request_muxed_mem_region() and use in both drivers.
Shouldn't this be done the other way around, first introducing
request_muxed_mem_region() and then using it directly in both drivers,
rather than having a temporary situation where a failure can happen?
As far as I'm concerned, the patch series you just posted are
acceptable only if request_muxed_mem_region() gets accepted too.
Otherwise we end up with the situation where a driver could randomly
fail.
--
Jean Delvare
SUSE L3 Support
On 1/19/22 3:53 AM, Andy Shevchenko wrote:
> On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <[email protected]> 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.
>
> ...
>
>> To: Guenter Roeck <[email protected]>
>> To: [email protected]
>> To: Jean Delvare <[email protected]>
>> To: [email protected]
>> To: Wolfram Sang <[email protected]>
>> To: Andy Shevchenko <[email protected]>
>> To: Rafael J. Wysocki <[email protected]>
>> Cc: [email protected]
>> Cc: Wim Van Sebroeck <[email protected]>
>> Cc: Robert Richter <[email protected]>
>> Cc: Thomas Lendacky <[email protected]>
>
> Same comment to all your patches.
>
> ...
>
>> +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;
>
> Not really used variable.
>
>> + 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);
Talking about line splits, is this one necessary ?
>> + 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);
>
> Why? If it's a short live mapping, do not use devm.
>
This is not short lived; it is needed by the driver. The release
is an artifact of calling this function twice and ignoring the error
from devm_ioremap() if the first call fails. devm_release_mem_region()
isn't strictly needed but that would result in keeping the memory
region reserved even though it isn't used by the driver.
There is a functional difference to the original code, though.
The failing devm_ioremap() causes the code to try the alternate
address. I am not sure if that really adds value; devm_ioremap()
fails because the system is out of virtual memory, and calling
it again on a different address doesn't seem to add much value.
I preferred the original code, which would only call devm_ioremap()
after successfully reserving a memory region.
>> + return -ENOMEM;
>> + }
>
>> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n",
>> + mmio_addr);
>
> Unneeded noise.
>
>> + return ret;
>
> On top of above it's a NIH devm_ioremap_resource().
>
Not sure what NIH means, but if you refer to the lack of
devm_release_mem_region(), again, it isn't short lived.
>> +}
>
>
> ...
>
>> + int ret = 0;
>
> Redundant assignment.
>
> ...
>
>> + /* Check MMIO address conflict */
>> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
>
>> +
>> + /* Check alternate MMIO address conflict */
>
> Unify this with the previous comment.
>
Why ? It refers to the code below. If that is a single or two comments
is really just POV.
>> + if (ret)
>> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
>> + dev_name);
>
> ...
>
>> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN |
>> + SB800_ACPI_MMIO_SEL) !=
>> + SB800_ACPI_MMIO_DECODE_EN)) {
>
> The split looks ugly. Consider to use temporary variables or somehow
> rearrange the condition that it doesn't break in the middle of the one
> logical token.
Split at the &, maybe.
>
>> + alt_mmio_addr &= ~0xFFF;
>
> Why capital letters?
>
>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>> + }
>
> ...
>
>> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN |
>> + SB800_ACPI_MMIO_SEL)) !=
>> + SB800_ACPI_MMIO_DECODE_EN))) {
>
> Ditto.
>
>> + alt_mmio_addr &= ~0xFFF;
>
> Ditto.
>
>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>
> ...
>
> Okay, I see this is the original code like this... Perhaps it makes
> sense to reshuffle them (indentation-wise) at the same time and
> mention this in the changelog.
>
> ...
>
>> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>
> Is it still needed? I have no context to say if devm_iomap() and this
> are not colliding, please double check the correctness.
>
Not sure I understand. This is the release of the io region reserved with
request_muxed_region() at the beginning of this function. Why would it no
longer be necessary to release that region ?
Guenter
On 1/19/22 5:53 AM, Andy Shevchenko wrote:
> On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <[email protected]> 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.
>
> ...
>
>> To: Guenter Roeck <[email protected]>
>> To: [email protected]
>> To: Jean Delvare <[email protected]>
>> To: [email protected]
>> To: Wolfram Sang <[email protected]>
>> To: Andy Shevchenko <[email protected]>
>> To: Rafael J. Wysocki <[email protected]>
>> Cc: [email protected]
>> Cc: Wim Van Sebroeck <[email protected]>
>> Cc: Robert Richter <[email protected]>
>> Cc: Thomas Lendacky <[email protected]>
>
> Same comment to all your patches.
Ok. I'll reduce the patches' to/cc list to only contain maintainers owning
the current patch. I prefer to leave the lengthy list in the cover letter
if that is ok because it will not be added to the tree but will provide
context this series has multiple systems and may need communication
between maintainers. I'll use the -to & -cc commandline as you mentioned to
send to the longer list of recipients without cluttering the patch. Let me
know if you prefer otherwise.
>
> ...
>
>> +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;
>
> Not really used variable.
Yes, I'll remove 'ret' and set hardcoded 0 return value below.
>
>> + 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);
>
> Why? If it's a short live mapping, do not use devm.
This region isn't short lived. This is a region request for the
WDT registers used through the lifetime of the driver.
The short lived mapping you may be thinking of is in
sp5100_tco_setupdevice_mmio() from patch 3. The first register in
this region is FCH::PM::DECODEEN and is used to setup the mmio_addr
and alt_mmio_addr MMIO (longterm) regions.
>
>> + return -ENOMEM;
>> + }
>
>> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n",
>> + mmio_addr);
>
> Unneeded noise.
This was present pre-series. The current driver dmesg output with default
logging settings is:
dmesg | grep -i sp5100
[ 8.508515] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver
[ 8.525172] sp5100-tco sp5100-tco: Using 0xfeb00000 for watchdog MMIO address
[ 8.539912] sp5100-tco sp5100-tco: initialized. heartbeat=60 sec (nowayout=0)
I'll remove the dev_info.
>
>> + return ret;
>
> On top of above it's a NIH devm_ioremap_resource().
I'm not familiar with NIH term. My friends google and grep weren't much help.
>
>> +}
>
>
> ...
>
>> + int ret = 0;
>
> Redundant assignment.
Thanks. I'll leave the variable but remove the 0 assignment in the definition.
>
> ...
>
>> + /* Check MMIO address conflict */
>> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
>
>> +
>> + /* Check alternate MMIO address conflict */
>
> Unify this with the previous comment.
Ok
>
>> + if (ret)
>> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
>> + dev_name);
>
> ...
>
>> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN |
>> + SB800_ACPI_MMIO_SEL) !=
>> + SB800_ACPI_MMIO_DECODE_EN)) {
>
> The split looks ugly. Consider to use temporary variables or somehow
> rearrange the condition that it doesn't break in the middle of the one
> logical token.
I'll try splitting at the '&' as Guenter mentioned in other email.
>
>> + alt_mmio_addr &= ~0xFFF;
>
> Why capital letters?
This was already present pre-series. I'll change to lowercase to make it
consistent with the other constants in the file.
>
>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>> + }
>
> ...
>
>> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN |
>> + SB800_ACPI_MMIO_SEL)) !=
>> + SB800_ACPI_MMIO_DECODE_EN))) {
>
> Ditto.
My understanding is #defines should be capitalized. No?
>
>> + alt_mmio_addr &= ~0xFFF;
>
> Ditto.
Another uppercase constant I will make lowercase.
>
>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>
> ...
>
> Okay, I see this is the original code like this... Perhaps it makes
> sense to reshuffle them (indentation-wise) at the same time and
> mention this in the changelog.
>
> ...
>
>> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>
> Is it still needed? I have no context to say if devm_iomap() and this
> are not colliding, please double check the correctness.
>
Yes, this is needed. The release/request in sp5100_tco_setupdevice() is
for the non-efch mmio layout cases. It is using port I/O registers to
detect mmio_addr, alt_mmio_addr, and configure the device.
Regards,
Terry Bowman
On 1/19/22 8:57 AM, Terry Bowman wrote:
>
>
> On 1/19/22 5:53 AM, Andy Shevchenko wrote:
>> On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <[email protected]> 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.
>>
>> ...
>>
>>> To: Guenter Roeck <[email protected]>
>>> To: [email protected]
>>> To: Jean Delvare <[email protected]>
>>> To: [email protected]
>>> To: Wolfram Sang <[email protected]>
>>> To: Andy Shevchenko <[email protected]>
>>> To: Rafael J. Wysocki <[email protected]>
>>> Cc: [email protected]
>>> Cc: Wim Van Sebroeck <[email protected]>
>>> Cc: Robert Richter <[email protected]>
>>> Cc: Thomas Lendacky <[email protected]>
>>
>> Same comment to all your patches.
>
> Ok. I'll reduce the patches' to/cc list to only contain maintainers owning
> the current patch. I prefer to leave the lengthy list in the cover letter
> if that is ok because it will not be added to the tree but will provide
> context this series has multiple systems and may need communication
> between maintainers. I'll use the -to & -cc commandline as you mentioned to
> send to the longer list of recipients without cluttering the patch. Let me
> know if you prefer otherwise.
>>
>> ...
>>
>>> +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;
>>
>> Not really used variable.
>
> Yes, I'll remove 'ret' and set hardcoded 0 return value below.
>
>>
>>> + 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);
>>
>> Why? If it's a short live mapping, do not use devm.
>
> This region isn't short lived. This is a region request for the
> WDT registers used through the lifetime of the driver.
>
> The short lived mapping you may be thinking of is in
> sp5100_tco_setupdevice_mmio() from patch 3. The first register in
> this region is FCH::PM::DECODEEN and is used to setup the mmio_addr
> and alt_mmio_addr MMIO (longterm) regions.
>
>>
>>> + return -ENOMEM;
>>> + }
>>
>>> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n",
>>> + mmio_addr);
>>
>> Unneeded noise.
>
> This was present pre-series. The current driver dmesg output with default
> logging settings is:
>
> dmesg | grep -i sp5100
> [ 8.508515] sp5100_tco: SP5100/SB800 TCO WatchDog Timer Driver
> [ 8.525172] sp5100-tco sp5100-tco: Using 0xfeb00000 for watchdog MMIO address
> [ 8.539912] sp5100-tco sp5100-tco: initialized. heartbeat=60 sec (nowayout=0)
>
> I'll remove the dev_info.
>
>>
>>> + return ret;
>>
>> On top of above it's a NIH devm_ioremap_resource().
>
> I'm not familiar with NIH term. My friends google and grep weren't much help.
>
>>
>>> +}
>>
>>
>> ...
>>
>>> + int ret = 0;
>>
>> Redundant assignment.
>
> Thanks. I'll leave the variable but remove the 0 assignment in the definition.
>
>>
>> ...
>>
>>> + /* Check MMIO address conflict */
>>> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
>>
>>> +
>>> + /* Check alternate MMIO address conflict */
>>
>> Unify this with the previous comment.
>
> Ok
>
>>
>>> + if (ret)
>>> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
>>> + dev_name);
>>
>> ...
>>
>>> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN |
>>> + SB800_ACPI_MMIO_SEL) !=
>>> + SB800_ACPI_MMIO_DECODE_EN)) {
>>
>> The split looks ugly. Consider to use temporary variables or somehow
>> rearrange the condition that it doesn't break in the middle of the one
>> logical token.
>
> I'll try splitting at the '&' as Guenter mentioned in other email.
>
>>
>>> + alt_mmio_addr &= ~0xFFF;
>>
>> Why capital letters?
>
> This was already present pre-series. I'll change to lowercase to make it
> consistent with the other constants in the file.
>
>>
>>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>>> + }
>>
>> ...
>>
>>> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN |
>>> + SB800_ACPI_MMIO_SEL)) !=
>>> + SB800_ACPI_MMIO_DECODE_EN))) {
>>
>> Ditto.
>
> My understanding is #defines should be capitalized. No?
>
I think that Ditto referred to the line split comment.
Guenter
>>
>>> + alt_mmio_addr &= ~0xFFF;
>>
>> Ditto.
>
> Another uppercase constant I will make lowercase.
>
>>
>>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>>
>> ...
>>
>> Okay, I see this is the original code like this... Perhaps it makes
>> sense to reshuffle them (indentation-wise) at the same time and
>> mention this in the changelog.
>>
>> ...
>>
>>> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>>
>> Is it still needed? I have no context to say if devm_iomap() and this
>> are not colliding, please double check the correctness.
>>
>
> Yes, this is needed. The release/request in sp5100_tco_setupdevice() is
> for the non-efch mmio layout cases. It is using port I/O registers to
> detect mmio_addr, alt_mmio_addr, and configure the device.
>
> Regards,
> Terry Bowman
>
On 1/19/22 9:30 AM, Jean Delvare wrote:
> Hi Terry,
>
> On Tue, 18 Jan 2022 14:22:30 -0600, Terry Bowman wrote:
>> This series uses request_mem_region() to synchronize accesses to the MMIO
>> registers mentioned above. request_mem_region() is missing the retry
>> logic in the case the resource is busy. As a result, request_mem_region()
>> will fail immediately if the resource is busy. The 'muxed' variant is
>> needed here but request_muxed_mem_region() is not defined to use. I will
>> follow up with another patch series to define the
>> request_muxed_mem_region() and use in both drivers.
>
> Shouldn't this be done the other way around, first introducing
> request_muxed_mem_region() and then using it directly in both drivers,
> rather than having a temporary situation where a failure can happen?
>
> As far as I'm concerned, the patch series you just posted are
> acceptable only if request_muxed_mem_region() gets accepted too.
> Otherwise we end up with the situation where a driver could randomly
> fail.
>
Hi Jean,
I considered sending the request_muxed_mem_region() patch series first but
was concerned the patch might not be accepted without a need or usage. I
didn't see an obvious path forward for the order of submissions because of
the dependencies.
I need to make the review easy for you and the other maintainers. I can
send the request_muxed_mem_region() single patch series ASAP if you like.
Then I change the request_mem_region() -> request_muxed_mem_region() as
needed in the piix4_smbus v3 and sp5100_tco v4 and add dependency line as
well? Is their a risk the driver patches will take 2 merge windows before
added to the tree ? Is there anything I can do to avoid this?
Regards,
Terry
> I considered sending the request_muxed_mem_region() patch series first but
> was concerned the patch might not be accepted without a need or usage. I
> didn't see an obvious path forward for the order of submissions because of
> the dependencies.
My suggestion: make the request_muxed_mem_region() patch the new patch 1
of the piix4 series. Then, the user will directly come in the following
patches. From this series, I will create an immutable branch which can
be pulled in by the watchdog tree. It will then have the dependency for
your watchdog series. During next merge window, we (the maintainers)
will make sure that I2C will hit Linus' tree before the watchdog tree.
This works the other way around as well, if needed. Make
request_muxed_mem_region() the first patch of the watchdog series and
let me pull an immutable branch from watchdog into I2C.
On 1/19/22 9:47 AM, Wolfram Sang wrote:
>
>> I considered sending the request_muxed_mem_region() patch series first but
>> was concerned the patch might not be accepted without a need or usage. I
>> didn't see an obvious path forward for the order of submissions because of
>> the dependencies.
>
> My suggestion: make the request_muxed_mem_region() patch the new patch 1
> of the piix4 series. Then, the user will directly come in the following
> patches. From this series, I will create an immutable branch which can
> be pulled in by the watchdog tree. It will then have the dependency for
> your watchdog series. During next merge window, we (the maintainers)
> will make sure that I2C will hit Linus' tree before the watchdog tree.
>
> This works the other way around as well, if needed. Make
> request_muxed_mem_region() the first patch of the watchdog series and
> let me pull an immutable branch from watchdog into I2C.
>
Creating an immutable branch from i2c is fine. Also, typically Wim sends
his pull request late in the commit window, so i2c first should be no
problem either.
Also, if the immutable branch only includes the patch introducing
request_muxed_mem_region(), the pull order should not really matter.
Guenter
> Also, if the immutable branch only includes the patch introducing
> request_muxed_mem_region(), the pull order should not really matter.
Right, good point!
On 1/19/22 12:39 PM, Guenter Roeck wrote:
> On 1/19/22 9:47 AM, Wolfram Sang wrote:
>>
>>> I considered sending the request_muxed_mem_region() patch series first but
>>> was concerned the patch might not be accepted without a need or usage. I
>>> didn't see an obvious path forward for the order of submissions because of
>>> the dependencies.
>>
>> My suggestion: make the request_muxed_mem_region() patch the new patch 1
>> of the piix4 series. Then, the user will directly come in the following
>> patches. From this series, I will create an immutable branch which can
>> be pulled in by the watchdog tree. It will then have the dependency for
>> your watchdog series. During next merge window, we (the maintainers)
>> will make sure that I2C will hit Linus' tree before the watchdog tree.
>>
>> This works the other way around as well, if needed. Make
>> request_muxed_mem_region() the first patch of the watchdog series and
>> let me pull an immutable branch from watchdog into I2C.
>>
>
> Creating an immutable branch from i2c is fine. Also, typically Wim sends
> his pull request late in the commit window, so i2c first should be no
> problem either.
>
> Also, if the immutable branch only includes the patch introducing
> request_muxed_mem_region(), the pull order should not really matter.
>
> Guenter
Ok, I'll add the request_muxed_mem_region() patch to the i2c v3 series
as the first patch.
Reqards,
Terry
On Wed, Jan 19, 2022 at 6:57 PM Terry Bowman <[email protected]> wrote:
> On 1/19/22 5:53 AM, Andy Shevchenko wrote:
> > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <[email protected]> wrote:
> Ok. I'll reduce the patches' to/cc list to only contain maintainers owning
> the current patch. I prefer to leave the lengthy list in the cover letter
> if that is ok because it will not be added to the tree but will provide
> context this series has multiple systems and may need communication
> between maintainers. I'll use the -to & -cc commandline as you mentioned to
> send to the longer list of recipients without cluttering the patch. Let me
> know if you prefer otherwise.
My point is that: supply the list implicitly.
For the help of choosing the right people I have written a script [1]
that shows a very good heuristics approach to me.
[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
...
> >> + 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);
> > On top of above it's a NIH devm_ioremap_resource().
>
> I'm not familiar with NIH term. My friends google and grep weren't much help.
[2]: https://en.wikipedia.org/wiki/Not_invented_here
Means that you could very well simplify the code by using existing functions.
...
> > Okay, I see this is the original code like this... Perhaps it makes
> > sense to reshuffle them (indentation-wise) at the same time and
> > mention this in the changelog.
Here is the explanation that I noticed that the code you move is
original, and not written by you.
--
With Best Regards,
Andy Shevchenko
On Wed, Jan 19, 2022 at 5:46 PM Guenter Roeck <[email protected]> wrote:
> On 1/19/22 3:53 AM, Andy Shevchenko wrote:
> > On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <[email protected]> wrote:
...
> >> + devm_release_mem_region(dev, mmio_addr,
> >> + SP5100_WDT_MEM_MAP_SIZE);
> >
> > Why? If it's a short live mapping, do not use devm.
>
> This is not short lived; it is needed by the driver. The release
> is an artifact of calling this function twice and ignoring the error
> from devm_ioremap() if the first call fails. devm_release_mem_region()
> isn't strictly needed but that would result in keeping the memory
> region reserved even though it isn't used by the driver.
So, this seems like micro-optimization, but okay, at least it
justifies it. Thanks for explaining.
> There is a functional difference to the original code, though.
> The failing devm_ioremap() causes the code to try the alternate
> address. I am not sure if that really adds value; devm_ioremap()
> fails because the system is out of virtual memory, and calling
> it again on a different address doesn't seem to add much value.
> I preferred the original code, which would only call devm_ioremap()
> after successfully reserving a memory region.
...
> > On top of above it's a NIH devm_ioremap_resource().
>
> Not sure what NIH means
Not Invented Here (syndrome)
...
> >> + /* Check MMIO address conflict */
> >> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
> >
> >> +
> >> + /* Check alternate MMIO address conflict */
> >
> > Unify this with the previous comment.
>
> Why ? It refers to the code below. If that is a single or two comments
> is really just POV.
It depends on the angle from which you see it (i.o.w. like you said
POV). I considered it from the code perspective and personally found
the
/*
* Bla bla bla
*/
ret = foo();
if (ret)
bar();
better than above.
> >> + if (ret)
> >> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
> >> + dev_name);
...
> >> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
> >
> > Is it still needed? I have no context to say if devm_iomap() and this
> > are not colliding, please double check the correctness.
> >
> Not sure I understand. This is the release of the io region reserved with
> request_muxed_region() at the beginning of this function. Why would it no
> longer be necessary to release that region ?
Thank you for explaining, as I said I have no full context here, and I
simply asked for double check.
--
With Best Regards,
Andy Shevchenko
On Tue, 18 Jan 2022 14:22:30 -0600, Terry Bowman wrote:
> 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 | 335 ++++++++++++++++++++++------------
> drivers/watchdog/sp5100_tco.h | 6 +
> 2 files changed, 227 insertions(+), 114 deletions(-)
FWIW, I tested this patch series successfully on my AMD Ryzen 7 PRO
3700U-based laptop.
--
Jean Delvare
SUSE L3 Support
Hi Terry,
On Tue, 18 Jan 2022 14:22:33 -0600, 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. 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]>
> To: Guenter Roeck <[email protected]>
> To: [email protected]
> To: Jean Delvare <[email protected]>
> To: [email protected]
> To: Wolfram Sang <[email protected]>
> To: Andy Shevchenko <[email protected]>
> To: Rafael J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Robert Richter <[email protected]>
> Cc: Thomas Lendacky <[email protected]>
> ---
> drivers/watchdog/sp5100_tco.c | 88 ++++++++++++++++++++++++++++++++++-
> drivers/watchdog/sp5100_tco.h | 5 ++
> 2 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 64ecebd93403..36519a992ca1 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;
> }
> }
>
> @@ -318,6 +320,87 @@ 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;
> +
> + 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",
SMB -> SMBus
> + 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");
SMB -> SMBus
> + return -ENOMEM;
> + }
> +
A short comment saying what the next command is doing would be
appreciated.
> + if (!(efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
> + EFCH_PM_DECODEEN_WDT_TMREN)) {
I find such splits hard to read. If checkpatch complains when you don't
split it (but I think it no longer does, right?) then just introduce a
local variable to store the register value. Same for the 2 occurrences
below.
> + efch_update_pm_reg8(addr, EFCH_PM_DECODEEN,
> + 0xff,
> + EFCH_PM_DECODEEN_WDT_TMREN);
Easily fits in one fewer line.
> + }
> +
> + /* Determine MMIO base address */
> + if (efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
> + EFCH_PM_DECODEEN_WDT_TMREN)
> + mmio_addr = EFCH_PM_WDT_ADDR;
> +
> + /* Determine alternate MMIO base address */
> + if (efch_read_pm_reg8(addr, 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) {
> + tco_timer_enable_mmio(addr);
> + ret = sp5100_tco_timer_init(tco);
> + }
> +
> + iounmap(addr);
> + release_resource(res);
> +
> + return ret;
> +}
> +
> static int sp5100_tco_setupdevice(struct device *dev,
> struct watchdog_device *wdd)
> {
> @@ -327,6 +410,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..2df8f8b2c55b 100644
> --- a/drivers/watchdog/sp5100_tco.h
> +++ b/drivers/watchdog/sp5100_tco.h
> @@ -83,3 +83,8 @@
>
> #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
Other than these minor details, patch looks good to me, thanks.
Tested-by: Jean Delvare <[email protected]>
--
Jean Delvare
SUSE L3 Support
On 1/24/22 11:36 AM, Jean Delvare wrote:
> Hi Terry,
>
> On Tue, 18 Jan 2022 14:22:33 -0600, 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. 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]>
>> To: Guenter Roeck <[email protected]>
>> To: [email protected]
>> To: Jean Delvare <[email protected]>
>> To: [email protected]
>> To: Wolfram Sang <[email protected]>
>> To: Andy Shevchenko <[email protected]>
>> To: Rafael J. Wysocki <[email protected]>
>> Cc: [email protected]
>> Cc: Wim Van Sebroeck <[email protected]>
>> Cc: Robert Richter <[email protected]>
>> Cc: Thomas Lendacky <[email protected]>
>> ---
>> drivers/watchdog/sp5100_tco.c | 88 ++++++++++++++++++++++++++++++++++-
>> drivers/watchdog/sp5100_tco.h | 5 ++
>> 2 files changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>> index 64ecebd93403..36519a992ca1 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;
>> }
>> }
>>
>> @@ -318,6 +320,87 @@ 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;
>> +
>> + 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",
>
> SMB -> SMBus
>
>> + 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");
>
> SMB -> SMBus
>
>> + return -ENOMEM;
>> + }
>> +
>
> A short comment saying what the next command is doing would be
> appreciated.
>
>> + if (!(efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
>> + EFCH_PM_DECODEEN_WDT_TMREN)) {
>
> I find such splits hard to read. If checkpatch complains when you don't
> split it (but I think it no longer does, right?) then just introduce a
> local variable to store the register value. Same for the 2 occurrences
> below.
>
>> + efch_update_pm_reg8(addr, EFCH_PM_DECODEEN,
>> + 0xff,
>> + EFCH_PM_DECODEEN_WDT_TMREN);
>
> Easily fits in one fewer line.
>
>> + }
>> +
>> + /* Determine MMIO base address */
>> + if (efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
>> + EFCH_PM_DECODEEN_WDT_TMREN)
>> + mmio_addr = EFCH_PM_WDT_ADDR;
>> +
>> + /* Determine alternate MMIO base address */
>> + if (efch_read_pm_reg8(addr, 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) {
>> + tco_timer_enable_mmio(addr);
>> + ret = sp5100_tco_timer_init(tco);
>> + }
>> +
>> + iounmap(addr);
>> + release_resource(res);
>> +
>> + return ret;
>> +}
>> +
>> static int sp5100_tco_setupdevice(struct device *dev,
>> struct watchdog_device *wdd)
>> {
>> @@ -327,6 +410,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..2df8f8b2c55b 100644
>> --- a/drivers/watchdog/sp5100_tco.h
>> +++ b/drivers/watchdog/sp5100_tco.h
>> @@ -83,3 +83,8 @@
>>
>> #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
>
> Other than these minor details, patch looks good to me, thanks.
>
> Tested-by: Jean Delvare <[email protected]>
>
Hi Jean,
Is your "Tested-by" for patch 3/4 or the sp5100_tco series?
Regards,
Terry
On 1/24/22 11:36 AM, Jean Delvare wrote:
...
>> + 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",
>
> SMB -> SMBus
>
Yes, I will update.
>> + 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");
>
> SMB -> SMBus
>
Yes, I will update.
>> + return -ENOMEM;
>> + }
>> +
>
> A short comment saying what the next command is doing would be
> appreciated.
>
I will add a brief comment explaining.
>> + if (!(efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
>> + EFCH_PM_DECODEEN_WDT_TMREN)) {
>
> I find such splits hard to read. If checkpatch complains when you don't
> split it (but I think it no longer does, right?) then just introduce a
> local variable to store the register value. Same for the 2 occurrences
> below.
>
I will update for readability.
>> + efch_update_pm_reg8(addr, EFCH_PM_DECODEEN,
>> + 0xff,
>> + EFCH_PM_DECODEEN_WDT_TMREN);
>
> Easily fits in one fewer line.
>
I will change to 2 lines.
>> + }
>> +
>> + /* Determine MMIO base address */
>> + if (efch_read_pm_reg8(addr, EFCH_PM_DECODEEN) &
>> + EFCH_PM_DECODEEN_WDT_TMREN)
>> + mmio_addr = EFCH_PM_WDT_ADDR;
>> +
>> + /* Determine alternate MMIO base address */
>> + if (efch_read_pm_reg8(addr, 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) {
>> + tco_timer_enable_mmio(addr);
>> + ret = sp5100_tco_timer_init(tco);
>> + }
>> +
>> + iounmap(addr);
>> + release_resource(res);
>> +
>> + return ret;
>> +}
>> +
>> static int sp5100_tco_setupdevice(struct device *dev,
>> struct watchdog_device *wdd)
>> {
>> @@ -327,6 +410,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..2df8f8b2c55b 100644
>> --- a/drivers/watchdog/sp5100_tco.h
>> +++ b/drivers/watchdog/sp5100_tco.h
>> @@ -83,3 +83,8 @@
>>
>> #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
>
> Other than these minor details, patch looks good to me, thanks.
>
> Tested-by: Jean Delvare <[email protected]>
>
I'm finishing up revisions for this and the i2c patch. I expect to send both tomorrow.
Regards,
Teryr
On Mon, 24 Jan 2022 16:36:33 -0600, Terry Bowman wrote:
> Is your "Tested-by" for patch 3/4 or the sp5100_tco series?
For the whole series actually, I only tested with all patches applied,
not the individual patches.
--
Jean Delvare
SUSE L3 Support
Hi Terry,
On Tue, 18 Jan 2022 14:22:34 -0600, Terry Bowman wrote:
> 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]>
> To: Guenter Roeck <[email protected]>
> To: [email protected]
> To: Jean Delvare <[email protected]>
> To: [email protected]
> To: Wolfram Sang <[email protected]>
> To: Andy Shevchenko <[email protected]>
> To: Rafael J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Robert Richter <[email protected]>
> Cc: Thomas Lendacky <[email protected]>
> ---
> drivers/watchdog/sp5100_tco.c | 16 ++++------------
> drivers/watchdog/sp5100_tco.h | 1 +
> 2 files changed, 5 insertions(+), 12 deletions(-)
> (...)
Looks good to me.
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
SUSE L3 Support
On Tue, 18 Jan 2022 14:22:31 -0600, Terry Bowman wrote:
> 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]>
> To: Guenter Roeck <[email protected]>
> To: [email protected]
> To: Jean Delvare <[email protected]>
> To: [email protected]
> To: Wolfram Sang <[email protected]>
> To: Andy Shevchenko <[email protected]>
> To: Rafael J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: Wim Van Sebroeck <[email protected]>
> Cc: Robert Richter <[email protected]>
> Cc: Thomas Lendacky <[email protected]>
> ---
> drivers/watchdog/sp5100_tco.c | 65 +++++++++++++++++++----------------
> 1 file changed, 36 insertions(+), 29 deletions(-)
> (...)
Except for the issues already mentioned by Andy, looks good to me.
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
SUSE L3 Support
Hi Terry,
Sorry for the late review, I hope you did not send an updated series
already.
On Tue, 18 Jan 2022 14:22:32 -0600, 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.
>
> (...)
> +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;
Can this actually happen? If it does, is -ENOMEM really the best error
value?
And if it can happen, I think I would prefer if you would simply not
call this function, knowing it can only fail. In other words, I'd go
for something like the following in the function below:
/* Check MMIO address conflict */
if (mmio_addr)
ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
The intention is clearer and execution is faster too.
> +
> + 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);
Remove trailing dot for consistency with the other messages.
> + 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);
Format for the addresses is inconsistent with the other messages above,
please use 0x%08x for consistency. As for the return value (which
should be preceded by a comma rather than a dot), it should be printed
as a decimal, not hexadecimal, value.
(And nitpicking: I'd split after "dev," so as to not make the line
longer than needed.
> +
> + return ret;
> +}
> +
> static int sp5100_tco_timer_init(struct sp5100_tco *tco)
> {
> struct watchdog_device *wdd = &tco->wdd;
> @@ -264,6 +324,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 +343,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))) {
The condition is the opposite of the sp5100 case above, which looks
quite suspicious. As far as I can see, that wasn't the case in the
original code. Please double check. In any case, please avoid double
negations, they are too easy to get wrong.
> + alt_mmio_addr &= ~0xFFF;
> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
> + }
> break;
> case efch:
> dev_name = SB800_DEVNAME;
> @@ -305,87 +390,24 @@ 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;
> }
> (...)
Rest looks OK to me.
--
Jean Delvare
SUSE L3 Support
On 1/25/22 7:45 AM, Jean Delvare wrote:
> Hi Terry,
>
> Sorry for the late review, I hope you did not send an updated series
> already.
>
Hi Jean,
No problem. I have not sent another revision yet. I'll add your requested
changes in the next revision.
> On Tue, 18 Jan 2022 14:22:32 -0600, 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.
>>
>> (...)
>> +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;
>
> Can this actually happen? If it does, is -ENOMEM really the best error
> value?
>
This can happen if mmio_addr is not assigned in sp5100_tco_setupdevice_mmio()
before calling sp5100_tco_prepare_base() and __sp5100_tco_prepare_base().
I can move the NULL check out of __sp5100_tco_prepare_base() and into
sp5100_tco_prepare_base() before calling __sp5100_tco_prepare_base().
As you describe below.
The ENOMEM return value should be interpreted as the mmio_addr is not
available. EBUSY does not describe the failure correctly because EBUSY
implies the resource is present and normally available but not available
at this time. Do you have a return value preference ?
> And if it can happen, I think I would prefer if you would simply not
> call this function, knowing it can only fail. In other words, I'd go
> for something like the following in the function below:
>
> /* Check MMIO address conflict */
> if (mmio_addr)
> ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
>
> The intention is clearer and execution is faster too.
>
Ok
>> +
>> + 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);
>
> Remove trailing dot for consistency with the other messages.
>
Ok.
>> + 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);
>
> Format for the addresses is inconsistent with the other messages above,
> please use 0x%08x for consistency. As for the return value (which
> should be preceded by a comma rather than a dot), it should be printed
> as a decimal, not hexadecimal, value.
>
Ok, I'll correct the address format to use '0x', change the period to a comma,
and display the the return code as decimal.
> (And nitpicking: I'd split after "dev," so as to not make the line
> longer than needed.
>
I'll move the line break at 'dev,'.
>> +
>> + return ret;
>> +}
>> +
>> static int sp5100_tco_timer_init(struct sp5100_tco *tco)
>> {
>> struct watchdog_device *wdd = &tco->wdd;
>> @@ -264,6 +324,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 +343,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))) {
>
> The condition is the opposite of the sp5100 case above, which looks
> quite suspicious. As far as I can see, that wasn't the case in the
> original code. Please double check. In any case, please avoid double
> negations, they are too easy to get wrong.
>
Yes, I found this earlier. I have fix for this in the next revision.
>> + alt_mmio_addr &= ~0xFFF;
>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>> + }
>> break;
>> case efch:
>> dev_name = SB800_DEVNAME;
>> @@ -305,87 +390,24 @@ 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;
>> }
>> (...)
>
> Rest looks OK to me.
>
On Tue, 25 Jan 2022 09:18:59 -0600, Terry Bowman wrote:
> On 1/25/22 7:45 AM, Jean Delvare wrote:
> > On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote:
> >> +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;
> >
> > Can this actually happen? If it does, is -ENOMEM really the best error
> > value?
>
> This can happen if mmio_addr is not assigned in sp5100_tco_setupdevice_mmio()
> before calling sp5100_tco_prepare_base() and __sp5100_tco_prepare_base().
Ah yes, I can see it now.
> I can move the NULL check out of __sp5100_tco_prepare_base() and into
> sp5100_tco_prepare_base() before calling __sp5100_tco_prepare_base().
> As you describe below.
>
> The ENOMEM return value should be interpreted as the mmio_addr is not
> available. EBUSY does not describe the failure correctly because EBUSY
> implies the resource is present and normally available but not available
> at this time. Do you have a return value preference ?
Well, if one mmio_addr isn't set, you shouldn't call
__sp5100_tco_prepare_base() for it so there's no error to return. If
neither mmio_addr is set then the hardware is simply not configured to
be used, so that would be a -NODEV returned by
sp5100_tco_prepare_base() I suppose?
BTW...
> >> (...)
> >> + if (ret)
> >> + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X",
> >> + mmio_addr, alt_mmio_addr, ret);
... I think that should be a "or" rather than "and", and singular
"region", in this error message? I mean, the plan was never to
reserve-map both of them, if I understand correctly.
--
Jean Delvare
SUSE L3 Support
On 1/25/22 10:38 AM, Jean Delvare wrote:
> On Tue, 25 Jan 2022 09:18:59 -0600, Terry Bowman wrote:
>> On 1/25/22 7:45 AM, Jean Delvare wrote:
>>> On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote:
>>>> +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;
>>>
>>> Can this actually happen? If it does, is -ENOMEM really the best error
>>> value?
>>
>> This can happen if mmio_addr is not assigned in sp5100_tco_setupdevice_mmio()
>> before calling sp5100_tco_prepare_base() and __sp5100_tco_prepare_base().
>
> Ah yes, I can see it now.
>
>> I can move the NULL check out of __sp5100_tco_prepare_base() and into
>> sp5100_tco_prepare_base() before calling __sp5100_tco_prepare_base().
>> As you describe below.
>>
>> The ENOMEM return value should be interpreted as the mmio_addr is not
>> available. EBUSY does not describe the failure correctly because EBUSY
>> implies the resource is present and normally available but not available
>> at this time. Do you have a return value preference ?
>
> Well, if one mmio_addr isn't set, you shouldn't call
> __sp5100_tco_prepare_base() for it so there's no error to return. If
> neither mmio_addr is set then the hardware is simply not configured to
> be used, so that would be a -NODEV returned by
> sp5100_tco_prepare_base() I suppose?
I agree, -NODEV communicates the error status better.
>
> BTW...
>
>>>> (...)
>>>> + if (ret)
>>>> + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X",
>>>> + mmio_addr, alt_mmio_addr, ret);
>
> ... I think that should be a "or" rather than "and", and singular
> "region", in this error message? I mean, the plan was never to
> reserve-map both of them, if I understand correctly.
>
This dev_err() is executed when both mmio_addr and alt_mmio_addr addresses failed either
devm_request_mem_region() or failed devm_ioremap(). I think the following would be most accurate:
dev_err(dev,
"Failed to reserve or map the MMIO (0x%X) and alternate MMIO (0x%X) regions, ret=%d",
mmio_addr, alt_mmio_addr, ret);
Above is my preference but I don't have a strong opinion. Providing the address and error code
information in the message is most important. I will make the change as you requested.
Regards,
Terry
On Tue, 25 Jan 2022 12:02:45 -0600, Terry Bowman wrote:
> On 1/25/22 10:38 AM, Jean Delvare wrote:
> > On Tue, 25 Jan 2022 09:18:59 -0600, Terry Bowman wrote:
> >> On 1/25/22 7:45 AM, Jean Delvare wrote:
> >>> On Tue, 18 Jan 2022 14:22:32 -0600, Terry Bowman wrote:
> >>>> (...)
> >>>> + if (ret)
> >>>> + dev_err(dev, "Failed to reserve-map MMIO (%X) and alternate MMIO (%X) regions. ret=%X",
> >>>> + mmio_addr, alt_mmio_addr, ret);
> >
> > ... I think that should be a "or" rather than "and", and singular
> > "region", in this error message? I mean, the plan was never to
> > reserve-map both of them, if I understand correctly.
> >
>
> This dev_err() is executed when both mmio_addr and alt_mmio_addr addresses failed either
> devm_request_mem_region() or failed devm_ioremap(). I think the following would be most accurate:
>
> dev_err(dev,
> "Failed to reserve or map the MMIO (0x%X) and alternate MMIO (0x%X) regions, ret=%d",
> mmio_addr, alt_mmio_addr, ret);
>
> Above is my preference but I don't have a strong opinion. Providing the address and error code
> information in the message is most important. I will make the change as you requested.
I agree, fine with me, no worry.
--
Jean Delvare
SUSE L3 Support