| Message ID | 20210813213216.54780-1-Terry.Bowman@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Watchdog: sp5100_tco: Replace watchdog cd6h/cd7h port I/O accesses with MMIO accesses | expand |
On 8/13/21 2:32 PM, Terry Bowman wrote: > Use MMIO instead of port I/O during SMBus controller address discovery. > Also, update how EFCH capability is determined by replacing a family check > with a PCI revision ID check. > > cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to > disabled cd6h/cd7h port I/O will return F's and written data is dropped. > The recommended workaround to handle disabled cd6h/cd7h port I/O is > replacing port I/O with MMIO accesses. The MMIO access method has been > available since at least SMBus controllers using PCI revision 0x59. > > The sp5100_tco driver uses a CPU family match of 17h to determine > EFCH_PM_DECODEEN_WDT_TMREN register support. Using a family check requires > driver updates for each new AMD CPU family following 17h. This patch > replaces the family check with a check for SMBus PCI revision ID 0x59 and > later. Note: Family 17h processors use SMBus PCI revision ID 0x59. The > intent is to use the PCI revision ID check to support future AMD processors > while minimizing required driver changes. The caveat with this change is > the sp5100_tco driver must be updated if a new AMD processor family changes > the EFCH design or the SMBus PCI ID value doesn't follow this pattern. > > Tested with forced WDT reset using `cat >> /dev/watchdog`. > I am sorry, I don't understand why the new code can not use devm functions, why the new data structure is necessary in the first place, and why it is not possible to improve alignment with the existing code. This will require a substantial amount of time to review to ensure that the changes are not excessive (at first glance it for sure looks like that to me). Guenter > Signed-off-by: Terry Bowman <Terry.Bowman@amd.com> > Reviewed-by: Robert Richter <rrichter@amd.com> > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: linux-kernel@vger.kernel.org > --- > drivers/watchdog/sp5100_tco.c | 180 +++++++++++++++++++++++++++------- > drivers/watchdog/sp5100_tco.h | 5 + > 2 files changed, 148 insertions(+), 37 deletions(-) > > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > index a730ecbf78cd..cf1d0d96a731 100644 > --- a/drivers/watchdog/sp5100_tco.c > +++ b/drivers/watchdog/sp5100_tco.c > @@ -44,6 +44,7 @@ > #include "sp5100_tco.h" > > #define TCO_DRIVER_NAME "sp5100-tco" > +#define AMD_PCI_SMBUS_REVISION_MMIO 0x59 > > /* internal variables */ > > @@ -51,6 +52,11 @@ enum tco_reg_layout { > sp5100, sb800, efch > }; > > +struct efch_cfg { > + void __iomem *addr; > + struct resource *res; > +}; > + > struct sp5100_tco { > struct watchdog_device wdd; > void __iomem *tcobase; > @@ -161,7 +167,133 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set) > outb(val, SP5100_IO_PM_DATA_REG); > } > > -static void tco_timer_enable(struct sp5100_tco *tco) > +static bool efch_use_mmio(void) > +{ > + return (sp5100_tco_pci->vendor == PCI_VENDOR_ID_AMD && > + sp5100_tco_pci->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS && > + sp5100_tco_pci->revision >= AMD_PCI_SMBUS_REVISION_MMIO); > +} > + > +static u8 efch_read_pm_reg8(struct efch_cfg *cfg, u8 index) > +{ > + if (!cfg->addr) > + return sp5100_tco_read_pm_reg8(index); > + > + return readb(cfg->addr + index); > +} > + > +static void efch_update_pm_reg8(struct efch_cfg *cfg, > + u8 index, u8 reset, u8 set) > +{ > + u8 val; > + > + if (!cfg->addr) { > + sp5100_tco_update_pm_reg8(index, reset, set); > + return; > + } > + > + val = readb(cfg->addr + index); > + val &= reset; > + val |= set; > + writeb(val, cfg->addr + index); > +} > + > +/* Return SMBus controller's MMIO address on success and 0 on error. */ > +static u32 efch_setup_mmio_addr(struct efch_cfg *cfg) > +{ > + /* > + * On EFCH 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 mmio is enbaled then the WDT needs to be started if not > + * already started. > + */ > + if (cfg->addr) { > + if (!(efch_read_pm_reg8(cfg, EFCH_PM_DECODEEN) & > + EFCH_PM_DECODEEN_WDT_TMREN)) { > + efch_update_pm_reg8(cfg, EFCH_PM_DECODEEN, > + 0xff, > + EFCH_PM_DECODEEN_WDT_TMREN); > + } > + } > + > + if (!(efch_read_pm_reg8(cfg, EFCH_PM_DECODEEN) & > + EFCH_PM_DECODEEN_WDT_TMREN)) > + return 0; > + > + return EFCH_PM_WDT_ADDR; > +} > + > +/* > + * Return SMBus controller's alternate MMIO address on success and 0 on > + * error. > + * > + * The alternate SMBus MMIO address is necessary if the address > + * returned from efch_setup_mmio_addr() is unreadable or the space > + * reservation fails. > + */ > +static u32 efch_setup_alt_mmio_addr(struct efch_cfg *cfg) > +{ > + if (!(efch_read_pm_reg8(cfg, EFCH_PM_ISACONTROL) & > + EFCH_PM_ISACONTROL_MMIOEN)) > + return 0; > + > + return EFCH_PM_ACPI_MMIO_ADDR + EFCH_PM_ACPI_MMIO_WDT_OFFSET; > +} > + > +static int sp5100_region_setup(struct device *dev, struct efch_cfg *cfg) > +{ > + if (efch_use_mmio()) { > + struct resource *res; > + void __iomem *addr; > + > + res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR, > + EFCH_PM_ACPI_MMIO_PM_SIZE, > + "sp5100_tco"); > + if (!res) { > + dev_err(dev, > + "SMB base address memory region 0x%x already in use.\n", > + EFCH_PM_ACPI_MMIO_PM_ADDR); > + return -EBUSY; > + } > + > + addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, > + EFCH_PM_ACPI_MMIO_PM_SIZE); > + if (!addr) { > + release_resource(res); > + dev_err(dev, "SMB base address mapping failed.\n"); > + return -ENOMEM; > + } > + > + cfg->res = res; > + cfg->addr = addr; > + return 0; > + } > + > + /* Request the IO ports used by this driver */ > + if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, > + SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { > + dev_err(dev, "I/O address 0x%04x already in use\n", > + SP5100_IO_PM_INDEX_REG); > + return -EBUSY; > + } > + > + return 0; > +} > + > +static void sp5100_region_release(struct efch_cfg *cfg) > +{ > + if (!cfg->addr) { > + release_region(SP5100_IO_PM_INDEX_REG, > + SP5100_PM_IOPORTS_SIZE); > + return; > + } > + > + iounmap(cfg->addr); > + release_resource(cfg->res); > +} > + > +static void tco_timer_enable(struct sp5100_tco *tco, struct efch_cfg *cfg) > { > u32 val; > > @@ -197,9 +326,9 @@ static void tco_timer_enable(struct sp5100_tco *tco) > break; > case efch: > /* Set the Watchdog timer resolution to 1 sec and enable */ > - sp5100_tco_update_pm_reg8(EFCH_PM_DECODEEN3, > - ~EFCH_PM_WATCHDOG_DISABLE, > - EFCH_PM_DECODEEN_SECOND_RES); > + efch_update_pm_reg8(cfg, EFCH_PM_DECODEEN3, > + ~EFCH_PM_WATCHDOG_DISABLE, > + EFCH_PM_DECODEEN_SECOND_RES); > break; > } > } > @@ -219,17 +348,14 @@ static int sp5100_tco_setupdevice(struct device *dev, > struct watchdog_device *wdd) > { > struct sp5100_tco *tco = watchdog_get_drvdata(wdd); > + struct efch_cfg cfg = {0}; > const char *dev_name; > - u32 mmio_addr = 0, val; > + u32 mmio_addr, val; > int ret; > > - /* Request the IO ports used by this driver */ > - if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, > - SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { > - dev_err(dev, "I/O address 0x%04x already in use\n", > - SP5100_IO_PM_INDEX_REG); > - return -EBUSY; > - } > + ret = sp5100_region_setup(dev, &cfg); > + if (ret) > + return ret; > > /* > * Determine type of southbridge chipset. > @@ -247,21 +373,7 @@ 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; > + mmio_addr = efch_setup_mmio_addr(&cfg); > break; > default: > return -ENODEV; > @@ -307,13 +419,11 @@ static int sp5100_tco_setupdevice(struct device *dev, > 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)) { > + mmio_addr = efch_setup_alt_mmio_addr(&cfg); > + if (!mmio_addr) { > 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", > @@ -338,7 +448,7 @@ static int sp5100_tco_setupdevice(struct device *dev, > dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr); > > /* Setup the watchdog timer */ > - tco_timer_enable(tco); > + tco_timer_enable(tco, &cfg); > > val = readl(SP5100_WDT_CONTROL(tco->tcobase)); > if (val & SP5100_WDT_DISABLED) { > @@ -366,12 +476,8 @@ static int sp5100_tco_setupdevice(struct device *dev, > */ > tco_timer_stop(wdd); > > - release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); > - > - return 0; > - > unreg_region: > - release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); > + sp5100_region_release(&cfg); > return ret; > } > > 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 >
On 8/13/21 5:37 PM, Guenter Roeck wrote: > On 8/13/21 2:32 PM, Terry Bowman wrote: >> Use MMIO instead of port I/O during SMBus controller address discovery. >> Also, update how EFCH capability is determined by replacing a family >> check >> with a PCI revision ID check. >> >> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read >> accesses to >> disabled cd6h/cd7h port I/O will return F's and written data is dropped. >> The recommended workaround to handle disabled cd6h/cd7h port I/O is >> replacing port I/O with MMIO accesses. The MMIO access method has been >> available since at least SMBus controllers using PCI revision 0x59. >> >> The sp5100_tco driver uses a CPU family match of 17h to determine >> EFCH_PM_DECODEEN_WDT_TMREN register support. Using a family check >> requires >> driver updates for each new AMD CPU family following 17h. This patch >> replaces the family check with a check for SMBus PCI revision ID 0x59 and >> later. Note: Family 17h processors use SMBus PCI revision ID 0x59. The >> intent is to use the PCI revision ID check to support future AMD >> processors >> while minimizing required driver changes. The caveat with this change is >> the sp5100_tco driver must be updated if a new AMD processor family >> changes >> the EFCH design or the SMBus PCI ID value doesn't follow this pattern. >> >> Tested with forced WDT reset using `cat >> /dev/watchdog`. >> > > I am sorry, I don't understand why the new code can not use devm functions, > why the new data structure is necessary in the first place, and why it is > not possible to improve alignment with the existing code. This will require > a substantial amount of time to review to ensure that the changes are not > excessive (at first glance it for sure looks like that to me). > > Guenter > Hi Guenter, I can change the patch to use devm functions as you mentioned. My understanding is the patch's reservation and mapping related functions are the focus. I originally chose not to use devm functions because the patch's MMIO reserved and mapped resources are not held for the driver lifetime as is the case for most device managed resources. The sp5100_tco driver must only hold these MMIO resources briefly because other drivers use the same EFCH MMIO registers. An example of another driver using the same registers is the piix4_smbus driver (drivers/i2c /busses/i2c-piix4.c). This patch can be changed to use the devm functions but the driver may not benefit from the device management. The 'struct efch_cfg' addition is needed for MMIO reads/writes as well as during cleanup when leaving sp5100_region_setup(). This structure was chosen to contain the data instead of passing multiple parameters to each EFCH function called. Do you have any recommendations for how to best improve the alignment? Regards, Terry >> Signed-off-by: Terry Bowman <Terry.Bowman@amd.com> >> Reviewed-by: Robert Richter <rrichter@amd.com> >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/watchdog/sp5100_tco.c | 180 +++++++++++++++++++++++++++------- >> drivers/watchdog/sp5100_tco.h | 5 + >> 2 files changed, 148 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/watchdog/sp5100_tco.c >> b/drivers/watchdog/sp5100_tco.c >> index a730ecbf78cd..cf1d0d96a731 100644 >> --- a/drivers/watchdog/sp5100_tco.c >> +++ b/drivers/watchdog/sp5100_tco.c >> @@ -44,6 +44,7 @@ >> #include "sp5100_tco.h" >> #define TCO_DRIVER_NAME "sp5100-tco" >> +#define AMD_PCI_SMBUS_REVISION_MMIO 0x59 >> /* internal variables */ >> @@ -51,6 +52,11 @@ enum tco_reg_layout { >> sp5100, sb800, efch >> }; >> +struct efch_cfg { >> + void __iomem *addr; >> + struct resource *res; >> +}; >> + >> struct sp5100_tco { >> struct watchdog_device wdd; >> void __iomem *tcobase; >> @@ -161,7 +167,133 @@ static void sp5100_tco_update_pm_reg8(u8 index, >> u8 reset, u8 set) >> outb(val, SP5100_IO_PM_DATA_REG); >> } >> -static void tco_timer_enable(struct sp5100_tco *tco) >> +static bool efch_use_mmio(void) >> +{ >> + return (sp5100_tco_pci->vendor == PCI_VENDOR_ID_AMD && >> + sp5100_tco_pci->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS && >> + sp5100_tco_pci->revision >= AMD_PCI_SMBUS_REVISION_MMIO); >> +} >> + >> +static u8 efch_read_pm_reg8(struct efch_cfg *cfg, u8 index) >> +{ >> + if (!cfg->addr) >> + return sp5100_tco_read_pm_reg8(index); >> + >> + return readb(cfg->addr + index); >> +} >> + >> +static void efch_update_pm_reg8(struct efch_cfg *cfg, >> + u8 index, u8 reset, u8 set) >> +{ >> + u8 val; >> + >> + if (!cfg->addr) { >> + sp5100_tco_update_pm_reg8(index, reset, set); >> + return; >> + } >> + >> + val = readb(cfg->addr + index); >> + val &= reset; >> + val |= set; >> + writeb(val, cfg->addr + index); >> +} >> + >> +/* Return SMBus controller's MMIO address on success and 0 on error. */ >> +static u32 efch_setup_mmio_addr(struct efch_cfg *cfg) >> +{ >> + /* >> + * On EFCH 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 mmio is enbaled then the WDT needs to be started if not >> + * already started. >> + */ >> + if (cfg->addr) { >> + if (!(efch_read_pm_reg8(cfg, EFCH_PM_DECODEEN) & >> + EFCH_PM_DECODEEN_WDT_TMREN)) { >> + efch_update_pm_reg8(cfg, EFCH_PM_DECODEEN, >> + 0xff, >> + EFCH_PM_DECODEEN_WDT_TMREN); >> + } >> + } >> + >> + if (!(efch_read_pm_reg8(cfg, EFCH_PM_DECODEEN) & >> + EFCH_PM_DECODEEN_WDT_TMREN)) >> + return 0; >> + >> + return EFCH_PM_WDT_ADDR; >> +} >> + >> +/* >> + * Return SMBus controller's alternate MMIO address on success and 0 on >> + * error. >> + * >> + * The alternate SMBus MMIO address is necessary if the address >> + * returned from efch_setup_mmio_addr() is unreadable or the space >> + * reservation fails. >> + */ >> +static u32 efch_setup_alt_mmio_addr(struct efch_cfg *cfg) >> +{ >> + if (!(efch_read_pm_reg8(cfg, EFCH_PM_ISACONTROL) & >> + EFCH_PM_ISACONTROL_MMIOEN)) >> + return 0; >> + >> + return EFCH_PM_ACPI_MMIO_ADDR + EFCH_PM_ACPI_MMIO_WDT_OFFSET; >> +} >> + >> +static int sp5100_region_setup(struct device *dev, struct efch_cfg *cfg) >> +{ >> + if (efch_use_mmio()) { >> + struct resource *res; >> + void __iomem *addr; >> + >> + res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR, >> + EFCH_PM_ACPI_MMIO_PM_SIZE, >> + "sp5100_tco"); >> + if (!res) { >> + dev_err(dev, >> + "SMB base address memory region 0x%x already in use.\n", >> + EFCH_PM_ACPI_MMIO_PM_ADDR); >> + return -EBUSY; >> + } >> + >> + addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, >> + EFCH_PM_ACPI_MMIO_PM_SIZE); >> + if (!addr) { >> + release_resource(res); >> + dev_err(dev, "SMB base address mapping failed.\n"); >> + return -ENOMEM; >> + } >> + >> + cfg->res = res; >> + cfg->addr = addr; >> + return 0; >> + } >> + >> + /* Request the IO ports used by this driver */ >> + if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, >> + SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { >> + dev_err(dev, "I/O address 0x%04x already in use\n", >> + SP5100_IO_PM_INDEX_REG); >> + return -EBUSY; >> + } >> + >> + return 0; >> +} >> + >> +static void sp5100_region_release(struct efch_cfg *cfg) >> +{ >> + if (!cfg->addr) { >> + release_region(SP5100_IO_PM_INDEX_REG, >> + SP5100_PM_IOPORTS_SIZE); >> + return; >> + } >> + >> + iounmap(cfg->addr); >> + release_resource(cfg->res); >> +} >> + >> +static void tco_timer_enable(struct sp5100_tco *tco, struct efch_cfg >> *cfg) >> { >> u32 val; >> @@ -197,9 +326,9 @@ static void tco_timer_enable(struct sp5100_tco *tco) >> break; >> case efch: >> /* Set the Watchdog timer resolution to 1 sec and enable */ >> - sp5100_tco_update_pm_reg8(EFCH_PM_DECODEEN3, >> - ~EFCH_PM_WATCHDOG_DISABLE, >> - EFCH_PM_DECODEEN_SECOND_RES); >> + efch_update_pm_reg8(cfg, EFCH_PM_DECODEEN3, >> + ~EFCH_PM_WATCHDOG_DISABLE, >> + EFCH_PM_DECODEEN_SECOND_RES); >> break; >> } >> } >> @@ -219,17 +348,14 @@ static int sp5100_tco_setupdevice(struct device >> *dev, >> struct watchdog_device *wdd) >> { >> struct sp5100_tco *tco = watchdog_get_drvdata(wdd); >> + struct efch_cfg cfg = {0}; >> const char *dev_name; >> - u32 mmio_addr = 0, val; >> + u32 mmio_addr, val; >> int ret; >> - /* Request the IO ports used by this driver */ >> - if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, >> - SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { >> - dev_err(dev, "I/O address 0x%04x already in use\n", >> - SP5100_IO_PM_INDEX_REG); >> - return -EBUSY; >> - } >> + ret = sp5100_region_setup(dev, &cfg); >> + if (ret) >> + return ret; >> /* >> * Determine type of southbridge chipset. >> @@ -247,21 +373,7 @@ 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; >> + mmio_addr = efch_setup_mmio_addr(&cfg); >> break; >> default: >> return -ENODEV; >> @@ -307,13 +419,11 @@ static int sp5100_tco_setupdevice(struct device >> *dev, >> 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)) { >> + mmio_addr = efch_setup_alt_mmio_addr(&cfg); >> + if (!mmio_addr) { >> 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", >> @@ -338,7 +448,7 @@ static int sp5100_tco_setupdevice(struct device *dev, >> dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", >> mmio_addr); >> /* Setup the watchdog timer */ >> - tco_timer_enable(tco); >> + tco_timer_enable(tco, &cfg); >> val = readl(SP5100_WDT_CONTROL(tco->tcobase)); >> if (val & SP5100_WDT_DISABLED) { >> @@ -366,12 +476,8 @@ static int sp5100_tco_setupdevice(struct device >> *dev, >> */ >> tco_timer_stop(wdd); >> - release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); >> - >> - return 0; >> - >> unreg_region: >> - release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); >> + sp5100_region_release(&cfg); >> return ret; >> } >> 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 >> >
On 8/16/21 2:29 PM, Terry Bowman wrote: > > > On 8/13/21 5:37 PM, Guenter Roeck wrote: >> On 8/13/21 2:32 PM, Terry Bowman wrote: >>> Use MMIO instead of port I/O during SMBus controller address discovery. >>> Also, update how EFCH capability is determined by replacing a family check >>> with a PCI revision ID check. >>> >>> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to >>> disabled cd6h/cd7h port I/O will return F's and written data is dropped. >>> The recommended workaround to handle disabled cd6h/cd7h port I/O is >>> replacing port I/O with MMIO accesses. The MMIO access method has been >>> available since at least SMBus controllers using PCI revision 0x59. >>> >>> The sp5100_tco driver uses a CPU family match of 17h to determine >>> EFCH_PM_DECODEEN_WDT_TMREN register support. Using a family check requires >>> driver updates for each new AMD CPU family following 17h. This patch >>> replaces the family check with a check for SMBus PCI revision ID 0x59 and >>> later. Note: Family 17h processors use SMBus PCI revision ID 0x59. The >>> intent is to use the PCI revision ID check to support future AMD processors >>> while minimizing required driver changes. The caveat with this change is >>> the sp5100_tco driver must be updated if a new AMD processor family changes >>> the EFCH design or the SMBus PCI ID value doesn't follow this pattern. >>> >>> Tested with forced WDT reset using `cat >> /dev/watchdog`. >>> >> >> I am sorry, I don't understand why the new code can not use devm functions, >> why the new data structure is necessary in the first place, and why it is >> not possible to improve alignment with the existing code. This will require >> a substantial amount of time to review to ensure that the changes are not >> excessive (at first glance it for sure looks like that to me). >> >> Guenter >> > > Hi Guenter, > > I can change the patch to use devm functions as you mentioned. My > understanding is the patch's reservation and mapping related functions > are the focus. I originally chose not to use devm functions because the > patch's MMIO reserved and mapped resources are not held for the driver > lifetime as is the case for most device managed resources. The > sp5100_tco driver must only hold these MMIO resources briefly because > other drivers use the same EFCH MMIO registers. An example of another > driver using the same registers is the piix4_smbus driver (drivers/i2c > /busses/i2c-piix4.c). This patch can be changed to use the devm > functions but the driver may not benefit from the device management. > > The 'struct efch_cfg' addition is needed for MMIO reads/writes as well > as during cleanup when leaving sp5100_region_setup(). This structure was > chosen to contain the data instead of passing multiple parameters to > each EFCH function called. > > Do you have any recommendations for how to best improve the alignment? > Overall it seems to me that it might make more sense to implement this as new driver instead of messing with the existing driver. Have you thought about that ? Guenter
On 8/18/21 3:34 PM, Guenter Roeck wrote: > On 8/16/21 2:29 PM, Terry Bowman wrote: >> >> >> On 8/13/21 5:37 PM, Guenter Roeck wrote: >>> On 8/13/21 2:32 PM, Terry Bowman wrote: >>>> Use MMIO instead of port I/O during SMBus controller address discovery. >>>> Also, update how EFCH capability is determined by replacing a family check >>>> with a PCI revision ID check. >>>> >>>> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to >>>> disabled cd6h/cd7h port I/O will return F's and written data is dropped. >>>> The recommended workaround to handle disabled cd6h/cd7h port I/O is >>>> replacing port I/O with MMIO accesses. The MMIO access method has been >>>> available since at least SMBus controllers using PCI revision 0x59. >>>> >>>> The sp5100_tco driver uses a CPU family match of 17h to determine >>>> EFCH_PM_DECODEEN_WDT_TMREN register support. Using a family check requires >>>> driver updates for each new AMD CPU family following 17h. This patch >>>> replaces the family check with a check for SMBus PCI revision ID 0x59 and >>>> later. Note: Family 17h processors use SMBus PCI revision ID 0x59. The >>>> intent is to use the PCI revision ID check to support future AMD processors >>>> while minimizing required driver changes. The caveat with this change is >>>> the sp5100_tco driver must be updated if a new AMD processor family changes >>>> the EFCH design or the SMBus PCI ID value doesn't follow this pattern. >>>> >>>> Tested with forced WDT reset using `cat >> /dev/watchdog`. >>>> >>> >>> I am sorry, I don't understand why the new code can not use devm functions, >>> why the new data structure is necessary in the first place, and why it is >>> not possible to improve alignment with the existing code. This will require >>> a substantial amount of time to review to ensure that the changes are not >>> excessive (at first glance it for sure looks like that to me). >>> >>> Guenter >>> >> >> Hi Guenter, >> >> I can change the patch to use devm functions as you mentioned. My >> understanding is the patch's reservation and mapping related functions >> are the focus. I originally chose not to use devm functions because the >> patch's MMIO reserved and mapped resources are not held for the driver >> lifetime as is the case for most device managed resources. The >> sp5100_tco driver must only hold these MMIO resources briefly because >> other drivers use the same EFCH MMIO registers. An example of another >> driver using the same registers is the piix4_smbus driver (drivers/i2c >> /busses/i2c-piix4.c). This patch can be changed to use the devm >> functions but the driver may not benefit from the device management. >> >> The 'struct efch_cfg' addition is needed for MMIO reads/writes as well >> as during cleanup when leaving sp5100_region_setup(). This structure was >> chosen to contain the data instead of passing multiple parameters to >> each EFCH function called. >> >> Do you have any recommendations for how to best improve the alignment? >> > > Overall it seems to me that it might make more sense to implement this > as new driver instead of messing with the existing driver. Have you > thought about that ? > > Guenter A new driver was initially considered for this patch. I decided to update the sp5100_tco driver instead because the changes are limited to initialization and post-initialization code is not modified. Also, updating the existing sp5100_tco driver allows for code reuse. This patch can be split into 2 or more patches to simplify review and testing. Would this help? Regards, Terry
Guenter, On 13.08.21 15:37:30, Guenter Roeck wrote: > I am sorry, I don't understand why the new code can not use devm functions, devm was not chosen here as the iomem is only used during initialization. The resource is released at the end of it. Using devm would keep it open and carry it around including its data until the device is released which we wanted to avoid. This would also add unnecessary data to struct sp5100_tco and/or the device's data. > why the new data structure is necessary in the first place, Instead we used struct efch_cfg to carry all that temporary data required during initialization. This is destroyed at the end of sp5100_tco_setupdevice(). > and why it is > not possible to improve alignment with the existing code. The driver serves 3 to 4 platforms now. To refactor it a core driver is needed plus sub-drivers for each platform. We decided not to change existing code here for older platforms and keep it as it is for them. Moreover, the efch driver only needs changes to its initialization code (using mmio instead of ioports), everything else remains the same. > This will require > a substantial amount of time to review to ensure that the changes are not > excessive (at first glance it for sure looks like that to me). This patch implements mostly helper functions and adds alternative code paths for the mmio case. For an easier review we could split the patch in about 3-4 patches, would that help? Thanks, -Robert
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c index a730ecbf78cd..cf1d0d96a731 100644 --- a/drivers/watchdog/sp5100_tco.c +++ b/drivers/watchdog/sp5100_tco.c @@ -44,6 +44,7 @@ #include "sp5100_tco.h" #define TCO_DRIVER_NAME "sp5100-tco" +#define AMD_PCI_SMBUS_REVISION_MMIO 0x59 /* internal variables */ @@ -51,6 +52,11 @@ enum tco_reg_layout { sp5100, sb800, efch }; +struct efch_cfg { + void __iomem *addr; + struct resource *res; +}; + struct sp5100_tco { struct watchdog_device wdd; void __iomem *tcobase; @@ -161,7 +167,133 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set) outb(val, SP5100_IO_PM_DATA_REG); } -static void tco_timer_enable(struct sp5100_tco *tco) +static bool efch_use_mmio(void) +{ + return (sp5100_tco_pci->vendor == PCI_VENDOR_ID_AMD && + sp5100_tco_pci->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS && + sp5100_tco_pci->revision >= AMD_PCI_SMBUS_REVISION_MMIO); +} + +static u8 efch_read_pm_reg8(struct efch_cfg *cfg, u8 index) +{ + if (!cfg->addr) + return sp5100_tco_read_pm_reg8(index); + + return readb(cfg->addr + index); +} + +static void efch_update_pm_reg8(struct efch_cfg *cfg, + u8 index, u8 reset, u8 set) +{ + u8 val; + + if (!cfg->addr) { + sp5100_tco_update_pm_reg8(index, reset, set); + return; + } + + val = readb(cfg->addr + index); + val &= reset; + val |= set; + writeb(val, cfg->addr + index); +} + +/* Return SMBus controller's MMIO address on success and 0 on error. */ +static u32 efch_setup_mmio_addr(struct efch_cfg *cfg) +{ + /* + * On EFCH 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 mmio is enbaled then the WDT needs to be started if not + * already started. + */ + if (cfg->addr) { + if (!(efch_read_pm_reg8(cfg, EFCH_PM_DECODEEN) & + EFCH_PM_DECODEEN_WDT_TMREN)) { + efch_update_pm_reg8(cfg, EFCH_PM_DECODEEN, + 0xff, + EFCH_PM_DECODEEN_WDT_TMREN); + } + } + + if (!(efch_read_pm_reg8(cfg, EFCH_PM_DECODEEN) & + EFCH_PM_DECODEEN_WDT_TMREN)) + return 0; + + return EFCH_PM_WDT_ADDR; +} + +/* + * Return SMBus controller's alternate MMIO address on success and 0 on + * error. + * + * The alternate SMBus MMIO address is necessary if the address + * returned from efch_setup_mmio_addr() is unreadable or the space + * reservation fails. + */ +static u32 efch_setup_alt_mmio_addr(struct efch_cfg *cfg) +{ + if (!(efch_read_pm_reg8(cfg, EFCH_PM_ISACONTROL) & + EFCH_PM_ISACONTROL_MMIOEN)) + return 0; + + return EFCH_PM_ACPI_MMIO_ADDR + EFCH_PM_ACPI_MMIO_WDT_OFFSET; +} + +static int sp5100_region_setup(struct device *dev, struct efch_cfg *cfg) +{ + if (efch_use_mmio()) { + struct resource *res; + void __iomem *addr; + + res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR, + EFCH_PM_ACPI_MMIO_PM_SIZE, + "sp5100_tco"); + if (!res) { + dev_err(dev, + "SMB base address memory region 0x%x already in use.\n", + EFCH_PM_ACPI_MMIO_PM_ADDR); + return -EBUSY; + } + + addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, + EFCH_PM_ACPI_MMIO_PM_SIZE); + if (!addr) { + release_resource(res); + dev_err(dev, "SMB base address mapping failed.\n"); + return -ENOMEM; + } + + cfg->res = res; + cfg->addr = addr; + return 0; + } + + /* Request the IO ports used by this driver */ + if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, + SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { + dev_err(dev, "I/O address 0x%04x already in use\n", + SP5100_IO_PM_INDEX_REG); + return -EBUSY; + } + + return 0; +} + +static void sp5100_region_release(struct efch_cfg *cfg) +{ + if (!cfg->addr) { + release_region(SP5100_IO_PM_INDEX_REG, + SP5100_PM_IOPORTS_SIZE); + return; + } + + iounmap(cfg->addr); + release_resource(cfg->res); +} + +static void tco_timer_enable(struct sp5100_tco *tco, struct efch_cfg *cfg) { u32 val; @@ -197,9 +326,9 @@ static void tco_timer_enable(struct sp5100_tco *tco) break; case efch: /* Set the Watchdog timer resolution to 1 sec and enable */ - sp5100_tco_update_pm_reg8(EFCH_PM_DECODEEN3, - ~EFCH_PM_WATCHDOG_DISABLE, - EFCH_PM_DECODEEN_SECOND_RES); + efch_update_pm_reg8(cfg, EFCH_PM_DECODEEN3, + ~EFCH_PM_WATCHDOG_DISABLE, + EFCH_PM_DECODEEN_SECOND_RES); break; } } @@ -219,17 +348,14 @@ static int sp5100_tco_setupdevice(struct device *dev, struct watchdog_device *wdd) { struct sp5100_tco *tco = watchdog_get_drvdata(wdd); + struct efch_cfg cfg = {0}; const char *dev_name; - u32 mmio_addr = 0, val; + u32 mmio_addr, val; int ret; - /* Request the IO ports used by this driver */ - if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, - SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { - dev_err(dev, "I/O address 0x%04x already in use\n", - SP5100_IO_PM_INDEX_REG); - return -EBUSY; - } + ret = sp5100_region_setup(dev, &cfg); + if (ret) + return ret; /* * Determine type of southbridge chipset. @@ -247,21 +373,7 @@ 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; + mmio_addr = efch_setup_mmio_addr(&cfg); break; default: return -ENODEV; @@ -307,13 +419,11 @@ static int sp5100_tco_setupdevice(struct device *dev, 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)) { + mmio_addr = efch_setup_alt_mmio_addr(&cfg); + if (!mmio_addr) { 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", @@ -338,7 +448,7 @@ static int sp5100_tco_setupdevice(struct device *dev, dev_info(dev, "Using 0x%08x for watchdog MMIO address\n", mmio_addr); /* Setup the watchdog timer */ - tco_timer_enable(tco); + tco_timer_enable(tco, &cfg); val = readl(SP5100_WDT_CONTROL(tco->tcobase)); if (val & SP5100_WDT_DISABLED) { @@ -366,12 +476,8 @@ static int sp5100_tco_setupdevice(struct device *dev, */ tco_timer_stop(wdd); - release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); - - return 0; - unreg_region: - release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE); + sp5100_region_release(&cfg); return ret; } 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