| Message ID | 20220202153525.1693378-1-terry.bowman@amd.com |
|---|---|
| Headers | show |
| Series | Watchdog: sp5100_tco: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand |
On Wed, Feb 02, 2022 at 09:35:22AM -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 <rrichter@amd.com> > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Tested-by: Jean Delvare <jdelvare@suse.de> > Reviewed-by: Jean Delvare <jdelvare@suse.de> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > 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 >
On Wed, Feb 02, 2022 at 09:35:24AM -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 <rrichter@amd.com> > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Tested-by: Jean Delvare <jdelvare@suse.de> > Reviewed-by: Jean Delvare <jdelvare@suse.de> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > 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 8db7504f0aa4..e02399ea8730 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 >
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/20220130184130.176646-2-terry.bowman@amd.com/ Based on v5.17-rc2 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 v5: - Updated dev_info() in sp5100_tco_prepare_base() to use physical address. Patch 2. (Guenter Roeck) - Rebased to v5.17-rc2 (Guenter Roeck) 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(-)