| Message ID | 20211103161521.43447-4-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 Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote: > cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to > disabled cd6h/cd7h port I/O will return F's and written data is dropped. > > The recommended workaround to handle disabled cd6h/cd7h port I/O is > replacing port I/O with MMIO accesses. The MMIO access method has been > available since at least SMBus controllers using PCI revision 0x59. > > The EFCH MMIO path is enabled in later patch. > > Co-developed-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > To: linux-watchdog@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Robert Richter <rrichter@amd.com> > --- > drivers/watchdog/sp5100_tco.c | 104 +++++++++++++++++++++++++++++++++- > drivers/watchdog/sp5100_tco.h | 6 ++ > 2 files changed, 109 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > index 80ae42ae7aaa..4777e672a8ad 100644 > --- a/drivers/watchdog/sp5100_tco.c > +++ b/drivers/watchdog/sp5100_tco.c > @@ -48,12 +48,14 @@ > /* internal variables */ > > enum tco_reg_layout { > - sp5100, sb800, efch > + sp5100, sb800, efch, efch_mmio > }; > > struct sp5100_tco { > struct watchdog_device wdd; > void __iomem *tcobase; > + void __iomem *addr; > + struct resource *res; > enum tco_reg_layout tco_reg_layout; > }; > > @@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set) > outb(val, SP5100_IO_PM_DATA_REG); > } > > +static int sp5100_request_region_mmio(struct device *dev, > + struct watchdog_device *wdd) > +{ > + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); > + struct resource *res; > + void __iomem *addr; > + > + res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR, > + EFCH_PM_ACPI_MMIO_PM_SIZE, > + "sp5100_tco"); > + > + if (!res) { > + dev_err(dev, > + "SMB base address memory region 0x%x already in use.\n", > + EFCH_PM_ACPI_MMIO_PM_ADDR); > + return -EBUSY; > + } > + > + addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, > + EFCH_PM_ACPI_MMIO_PM_SIZE); > + if (!addr) { > + release_resource(res); > + dev_err(dev, "SMB base address mapping failed.\n"); > + return -ENOMEM; > + } > + > + tco->res = res; > + tco->addr = addr; > + return 0; > +} > + > +static void sp5100_release_region_mmio(struct sp5100_tco *tco) > +{ > + iounmap(tco->addr); > + release_resource(tco->res); > +} > + > +static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index) > +{ > + return readb(tco->addr + index); > +} > + > +static void efch_update_pm_reg8(struct sp5100_tco *tco, > + u8 index, u8 reset, u8 set) > +{ > + u8 val; > + > + val = readb(tco->addr + index); > + val &= reset; > + val |= set; > + writeb(val, tco->addr + index); > +} > + > static void tco_timer_enable(struct sp5100_tco *tco) > { > u32 val; > @@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco) > ~EFCH_PM_WATCHDOG_DISABLE, > EFCH_PM_DECODEEN_SECOND_RES); > break; > + case efch_mmio: > + /* Set the Watchdog timer resolution to 1 sec and enable */ > + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3, > + ~EFCH_PM_WATCHDOG_DISABLE, > + EFCH_PM_DECODEEN_SECOND_RES); > + break; > } > } > > @@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco) > return 0; > } > > +static int sp5100_tco_setupdevice_mmio(struct device *dev, > + struct watchdog_device *wdd) > +{ > + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); > + const char *dev_name = SB800_DEVNAME; > + u32 mmio_addr = 0, alt_mmio_addr = 0; > + int ret; > + > + ret = sp5100_request_region_mmio(dev, wdd); > + if (ret) > + return ret; > + > + /* Determine MMIO base address */ > + if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & > + EFCH_PM_DECODEEN_WDT_TMREN)) { > + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN, > + 0xff, > + EFCH_PM_DECODEEN_WDT_TMREN); > + } > + > + if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & > + EFCH_PM_DECODEEN_WDT_TMREN) > + mmio_addr = EFCH_PM_WDT_ADDR; > + > + /* Determine alternate MMIO base address */ > + if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) & > + EFCH_PM_ISACONTROL_MMIOEN) > + alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + > + EFCH_PM_ACPI_MMIO_WDT_OFFSET; > + > + ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name); > + if (!ret) > + ret = sp5100_tco_timer_init(tco); > + > + sp5100_release_region_mmio(tco); > + return ret; > +} > + > static int sp5100_tco_setupdevice(struct device *dev, > struct watchdog_device *wdd) > { > @@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev, > u32 alt_mmio_addr = 0; > int ret; > > + if (tco->tco_reg_layout == efch_mmio) > + return sp5100_tco_setupdevice_mmio(dev, wdd); > + > /* Request the IO ports used by this driver */ > if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, > SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { > diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h > index adf015aa4126..73f179a1d6e5 100644 > --- a/drivers/watchdog/sp5100_tco.h > +++ b/drivers/watchdog/sp5100_tco.h > @@ -83,3 +83,9 @@ > > #define EFCH_PM_ACPI_MMIO_ADDR 0xfed80000 > #define EFCH_PM_ACPI_MMIO_WDT_OFFSET 0x00000b00 > +#define EFCH_PM_ACPI_MMIO_PM_OFFSET 0x00000300 > + > +#define EFCH_PM_ACPI_MMIO_PM_ADDR (EFCH_PM_ACPI_MMIO_ADDR + \ > + EFCH_PM_ACPI_MMIO_PM_OFFSET) > +#define EFCH_PM_ACPI_MMIO_PM_SIZE 8 > + git complains about an empty line at the end of file when applying this patch. Guenter
On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote: > cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to > disabled cd6h/cd7h port I/O will return F's and written data is dropped. > > The recommended workaround to handle disabled cd6h/cd7h port I/O is > replacing port I/O with MMIO accesses. The MMIO access method has been > available since at least SMBus controllers using PCI revision 0x59. > > The EFCH MMIO path is enabled in later patch. > > Co-developed-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > To: linux-watchdog@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Robert Richter <rrichter@amd.com> > --- > drivers/watchdog/sp5100_tco.c | 104 +++++++++++++++++++++++++++++++++- > drivers/watchdog/sp5100_tco.h | 6 ++ > 2 files changed, 109 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > index 80ae42ae7aaa..4777e672a8ad 100644 > --- a/drivers/watchdog/sp5100_tco.c > +++ b/drivers/watchdog/sp5100_tco.c > @@ -48,12 +48,14 @@ > /* internal variables */ > > enum tco_reg_layout { > - sp5100, sb800, efch > + sp5100, sb800, efch, efch_mmio > }; > > struct sp5100_tco { > struct watchdog_device wdd; > void __iomem *tcobase; > + void __iomem *addr; > + struct resource *res; I must admit that I really don't like this code. Both res and addr are only used during initialization, yet their presence suggests runtime usage. Any chance to reqork this to not require those variables ? Guenter > enum tco_reg_layout tco_reg_layout; > }; > > @@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set) > outb(val, SP5100_IO_PM_DATA_REG); > } > > +static int sp5100_request_region_mmio(struct device *dev, > + struct watchdog_device *wdd) > +{ > + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); > + struct resource *res; > + void __iomem *addr; > + > + res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR, > + EFCH_PM_ACPI_MMIO_PM_SIZE, > + "sp5100_tco"); > + > + if (!res) { > + dev_err(dev, > + "SMB base address memory region 0x%x already in use.\n", > + EFCH_PM_ACPI_MMIO_PM_ADDR); > + return -EBUSY; > + } > + > + addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, > + EFCH_PM_ACPI_MMIO_PM_SIZE); > + if (!addr) { > + release_resource(res); > + dev_err(dev, "SMB base address mapping failed.\n"); > + return -ENOMEM; > + } > + > + tco->res = res; > + tco->addr = addr; > + return 0; > +} > + > +static void sp5100_release_region_mmio(struct sp5100_tco *tco) > +{ > + iounmap(tco->addr); > + release_resource(tco->res); > +} > + > +static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index) > +{ > + return readb(tco->addr + index); > +} > + > +static void efch_update_pm_reg8(struct sp5100_tco *tco, > + u8 index, u8 reset, u8 set) > +{ > + u8 val; > + > + val = readb(tco->addr + index); > + val &= reset; > + val |= set; > + writeb(val, tco->addr + index); > +} > + > static void tco_timer_enable(struct sp5100_tco *tco) > { > u32 val; > @@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco) > ~EFCH_PM_WATCHDOG_DISABLE, > EFCH_PM_DECODEEN_SECOND_RES); > break; > + case efch_mmio: > + /* Set the Watchdog timer resolution to 1 sec and enable */ > + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3, > + ~EFCH_PM_WATCHDOG_DISABLE, > + EFCH_PM_DECODEEN_SECOND_RES); > + break; > } > } > > @@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco) > return 0; > } > > +static int sp5100_tco_setupdevice_mmio(struct device *dev, > + struct watchdog_device *wdd) > +{ > + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); > + const char *dev_name = SB800_DEVNAME; > + u32 mmio_addr = 0, alt_mmio_addr = 0; > + int ret; > + > + ret = sp5100_request_region_mmio(dev, wdd); > + if (ret) > + return ret; > + > + /* Determine MMIO base address */ > + if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & > + EFCH_PM_DECODEEN_WDT_TMREN)) { > + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN, > + 0xff, > + EFCH_PM_DECODEEN_WDT_TMREN); > + } > + > + if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & > + EFCH_PM_DECODEEN_WDT_TMREN) > + mmio_addr = EFCH_PM_WDT_ADDR; > + > + /* Determine alternate MMIO base address */ > + if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) & > + EFCH_PM_ISACONTROL_MMIOEN) > + alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + > + EFCH_PM_ACPI_MMIO_WDT_OFFSET; > + > + ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name); > + if (!ret) > + ret = sp5100_tco_timer_init(tco); > + > + sp5100_release_region_mmio(tco); > + return ret; > +} > + > static int sp5100_tco_setupdevice(struct device *dev, > struct watchdog_device *wdd) > { > @@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev, > u32 alt_mmio_addr = 0; > int ret; > > + if (tco->tco_reg_layout == efch_mmio) > + return sp5100_tco_setupdevice_mmio(dev, wdd); > + > /* Request the IO ports used by this driver */ > if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, > SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { > diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h > index adf015aa4126..73f179a1d6e5 100644 > --- a/drivers/watchdog/sp5100_tco.h > +++ b/drivers/watchdog/sp5100_tco.h > @@ -83,3 +83,9 @@ > > #define EFCH_PM_ACPI_MMIO_ADDR 0xfed80000 > #define EFCH_PM_ACPI_MMIO_WDT_OFFSET 0x00000b00 > +#define EFCH_PM_ACPI_MMIO_PM_OFFSET 0x00000300 > + > +#define EFCH_PM_ACPI_MMIO_PM_ADDR (EFCH_PM_ACPI_MMIO_ADDR + \ > + EFCH_PM_ACPI_MMIO_PM_OFFSET) > +#define EFCH_PM_ACPI_MMIO_PM_SIZE 8 > +
+ Tom Lendacky On 1/6/22 12:18 PM, Guenter Roeck wrote: > On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote: >> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to >> disabled cd6h/cd7h port I/O will return F's and written data is dropped. >> >> The recommended workaround to handle disabled cd6h/cd7h port I/O is >> replacing port I/O with MMIO accesses. The MMIO access method has been >> available since at least SMBus controllers using PCI revision 0x59. >> >> The EFCH MMIO path is enabled in later patch. >> >> Co-developed-by: Robert Richter <rrichter@amd.com> >> Signed-off-by: Robert Richter <rrichter@amd.com> >> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >> To: linux-watchdog@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: Robert Richter <rrichter@amd.com> >> --- >> drivers/watchdog/sp5100_tco.c | 104 +++++++++++++++++++++++++++++++++- >> drivers/watchdog/sp5100_tco.h | 6 ++ >> 2 files changed, 109 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c >> index 80ae42ae7aaa..4777e672a8ad 100644 >> --- a/drivers/watchdog/sp5100_tco.c >> +++ b/drivers/watchdog/sp5100_tco.c >> @@ -48,12 +48,14 @@ >> /* internal variables */ >> >> enum tco_reg_layout { >> - sp5100, sb800, efch >> + sp5100, sb800, efch, efch_mmio >> }; >> >> struct sp5100_tco { >> struct watchdog_device wdd; >> void __iomem *tcobase; >> + void __iomem *addr; >> + struct resource *res; > > I must admit that I really don't like this code. Both res and > addr are only used during initialization, yet their presence suggests > runtime usage. Any chance to reqork this to not require those variables ? > > Guenter > Hi Guenter, Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also correct the trailing newline you mentioned in an earlier email. Regards, Terry >> enum tco_reg_layout tco_reg_layout; >> }; >> >> @@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set) >> outb(val, SP5100_IO_PM_DATA_REG); >> } >> >> +static int sp5100_request_region_mmio(struct device *dev, >> + struct watchdog_device *wdd) >> +{ >> + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); >> + struct resource *res; >> + void __iomem *addr; >> + >> + res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR, >> + EFCH_PM_ACPI_MMIO_PM_SIZE, >> + "sp5100_tco"); >> + >> + if (!res) { >> + dev_err(dev, >> + "SMB base address memory region 0x%x already in use.\n", >> + EFCH_PM_ACPI_MMIO_PM_ADDR); >> + return -EBUSY; >> + } >> + >> + addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, >> + EFCH_PM_ACPI_MMIO_PM_SIZE); >> + if (!addr) { >> + release_resource(res); >> + dev_err(dev, "SMB base address mapping failed.\n"); >> + return -ENOMEM; >> + } >> + >> + tco->res = res; >> + tco->addr = addr; >> + return 0; >> +} >> + >> +static void sp5100_release_region_mmio(struct sp5100_tco *tco) >> +{ >> + iounmap(tco->addr); >> + release_resource(tco->res); >> +} >> + >> +static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index) >> +{ >> + return readb(tco->addr + index); >> +} >> + >> +static void efch_update_pm_reg8(struct sp5100_tco *tco, >> + u8 index, u8 reset, u8 set) >> +{ >> + u8 val; >> + >> + val = readb(tco->addr + index); >> + val &= reset; >> + val |= set; >> + writeb(val, tco->addr + index); >> +} >> + >> static void tco_timer_enable(struct sp5100_tco *tco) >> { >> u32 val; >> @@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco) >> ~EFCH_PM_WATCHDOG_DISABLE, >> EFCH_PM_DECODEEN_SECOND_RES); >> break; >> + case efch_mmio: >> + /* Set the Watchdog timer resolution to 1 sec and enable */ >> + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3, >> + ~EFCH_PM_WATCHDOG_DISABLE, >> + EFCH_PM_DECODEEN_SECOND_RES); >> + break; >> } >> } >> >> @@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco) >> return 0; >> } >> >> +static int sp5100_tco_setupdevice_mmio(struct device *dev, >> + struct watchdog_device *wdd) >> +{ >> + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); >> + const char *dev_name = SB800_DEVNAME; >> + u32 mmio_addr = 0, alt_mmio_addr = 0; >> + int ret; >> + >> + ret = sp5100_request_region_mmio(dev, wdd); >> + if (ret) >> + return ret; >> + >> + /* Determine MMIO base address */ >> + if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & >> + EFCH_PM_DECODEEN_WDT_TMREN)) { >> + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN, >> + 0xff, >> + EFCH_PM_DECODEEN_WDT_TMREN); >> + } >> + >> + if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & >> + EFCH_PM_DECODEEN_WDT_TMREN) >> + mmio_addr = EFCH_PM_WDT_ADDR; >> + >> + /* Determine alternate MMIO base address */ >> + if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) & >> + EFCH_PM_ISACONTROL_MMIOEN) >> + alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + >> + EFCH_PM_ACPI_MMIO_WDT_OFFSET; >> + >> + ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name); >> + if (!ret) >> + ret = sp5100_tco_timer_init(tco); >> + >> + sp5100_release_region_mmio(tco); >> + return ret; >> +} >> + >> static int sp5100_tco_setupdevice(struct device *dev, >> struct watchdog_device *wdd) >> { >> @@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev, >> u32 alt_mmio_addr = 0; >> int ret; >> >> + if (tco->tco_reg_layout == efch_mmio) >> + return sp5100_tco_setupdevice_mmio(dev, wdd); >> + >> /* Request the IO ports used by this driver */ >> if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, >> SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { >> diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h >> index adf015aa4126..73f179a1d6e5 100644 >> --- a/drivers/watchdog/sp5100_tco.h >> +++ b/drivers/watchdog/sp5100_tco.h >> @@ -83,3 +83,9 @@ >> >> #define EFCH_PM_ACPI_MMIO_ADDR 0xfed80000 >> #define EFCH_PM_ACPI_MMIO_WDT_OFFSET 0x00000b00 >> +#define EFCH_PM_ACPI_MMIO_PM_OFFSET 0x00000300 >> + >> +#define EFCH_PM_ACPI_MMIO_PM_ADDR (EFCH_PM_ACPI_MMIO_ADDR + \ >> + EFCH_PM_ACPI_MMIO_PM_OFFSET) >> +#define EFCH_PM_ACPI_MMIO_PM_SIZE 8 >> +
On 06.01.22 13:07:11, Terry Bowman wrote: > On 1/6/22 12:18 PM, Guenter Roeck wrote: > > On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote: > >> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > >> index 80ae42ae7aaa..4777e672a8ad 100644 > >> --- a/drivers/watchdog/sp5100_tco.c > >> +++ b/drivers/watchdog/sp5100_tco.c > >> @@ -48,12 +48,14 @@ > >> /* internal variables */ > >> > >> enum tco_reg_layout { > >> - sp5100, sb800, efch > >> + sp5100, sb800, efch, efch_mmio > >> }; > >> > >> struct sp5100_tco { > >> struct watchdog_device wdd; > >> void __iomem *tcobase; > >> + void __iomem *addr; > >> + struct resource *res; > > > > I must admit that I really don't like this code. Both res and > > addr are only used during initialization, yet their presence suggests > > runtime usage. Any chance to reqork this to not require those variables ? We did that in an earlier version, see struct efch_cfg of: https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/ The motivation of it was the same as you suggested to only use it during init. Having it in struct sp5100_tco made things simpler esp. in the definition of the function interfaces where those new members are used. If that init vars are no longer in struct sp5100_tco then callers of efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a pointer to them. To avoid this I see those options: 1) Implement them as global (or a single global struct) and possibly protect it by a mutex. There is only a single device anyway and we wouldn't need a protection. 2) Have an own mmio implementation of tco_timer_enable() and/or sp5100_tco_timer_init(). > Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also > correct the trailing newline you mentioned in an earlier email. > > Regards, > Terry > > >> enum tco_reg_layout tco_reg_layout; While at it, tco_reg_layout is also only used during initialization and can be moved there too. This would raise option 3: 3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that struct instead in init funtions only. But that causes the most rework (which would be ok to me). Going with 3) looks the cleanest way, I would try that. But all options have its advantages. -Robert > >> };
On 1/7/22 3:05 AM, Robert Richter wrote: > On 06.01.22 13:07:11, Terry Bowman wrote: >> On 1/6/22 12:18 PM, Guenter Roeck wrote: >>> On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote: > >>>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c >>>> index 80ae42ae7aaa..4777e672a8ad 100644 >>>> --- a/drivers/watchdog/sp5100_tco.c >>>> +++ b/drivers/watchdog/sp5100_tco.c >>>> @@ -48,12 +48,14 @@ >>>> /* internal variables */ >>>> >>>> enum tco_reg_layout { >>>> - sp5100, sb800, efch >>>> + sp5100, sb800, efch, efch_mmio >>>> }; >>>> >>>> struct sp5100_tco { >>>> struct watchdog_device wdd; >>>> void __iomem *tcobase; >>>> + void __iomem *addr; >>>> + struct resource *res; >>> >>> I must admit that I really don't like this code. Both res and >>> addr are only used during initialization, yet their presence suggests >>> runtime usage. Any chance to reqork this to not require those variables ? > > We did that in an earlier version, see struct efch_cfg of: > > https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/ > > The motivation of it was the same as you suggested to only use it > during init. > > Having it in struct sp5100_tco made things simpler esp. in the > definition of the function interfaces where those new members are > used. > 'res' is only used in the context of sp5100_request_region_mmio() and sp5100_release_region_mmio(), and both are called from sp5100_tco_setupdevice_mmio(). I do not see a need for carrying it around anywhere else, including efch_read_pm_reg8() and efch_update_pm_reg8(). efch_read_pm_reg8() is only called from sp5100_tco_setupdevice_mmio(), and it only uses struct sp5100_tco *tco to get the address. I don't see why the address can not be passed to it directly. efch_update_pm_reg8() also uses tco only to get the address. Again, passing it instead of a pointer to sp5100_tco *tco would easily be possible. efch_update_pm_reg8() is only called fromm sp5100_tco_setupdevice_mmio(), where the change would be easy. It is also called from tco_timer_enable(), which in turn is called from sp5100_tco_timer_init(). This, again, is called from sp5100_tco_setupdevice_mmio(). Since the first operation in sp5100_tco_timer_init() is to call tco_timer_enable() and that function does nothing but calling efch_update_pm_reg8(), it would easily be possible to pull out that code from tco_timer_enable() and move it into sp5100_tco_setupdevice_mmio(). So, no, I neither see the need for having the information in struct sp5100_tco nor for keeping it in its own structure. If you'd merge sp5100_request_region_mmio() and sp5100_release_region_mmio() into sp5100_tco_setupdevice_mmio() you would not even need any pointers to pass the values from sp5100_request_region_mmio(). Otherwise you could have sp5100_request_region_mmio() return a pointer to res or an ERR_PTR, and pass the address as pointer parameter. Guenter > If that init vars are no longer in struct sp5100_tco then callers of > efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a > pointer to them. To avoid this I see those options: > > 1) Implement them as global (or a single global struct) and possibly > protect it by a mutex. There is only a single device anyway and we > wouldn't need a protection. > > 2) Have an own mmio implementation of tco_timer_enable() and/or > sp5100_tco_timer_init(). > >> Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also >> correct the trailing newline you mentioned in an earlier email. >> >> Regards, >> Terry >> >>>> enum tco_reg_layout tco_reg_layout; > > While at it, tco_reg_layout is also only used during initialization > and can be moved there too. This would raise option 3: > > 3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that > struct instead in init funtions only. But that causes the most rework > (which would be ok to me). > > Going with 3) looks the cleanest way, I would try that. But all > options have its advantages. > > -Robert > >>>> };
On 07.01.22 09:12:32, Guenter Roeck wrote: > On 1/7/22 3:05 AM, Robert Richter wrote: > > On 06.01.22 13:07:11, Terry Bowman wrote: > > > On 1/6/22 12:18 PM, Guenter Roeck wrote: > > > > On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote: > > > > > > > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > > > > > index 80ae42ae7aaa..4777e672a8ad 100644 > > > > > --- a/drivers/watchdog/sp5100_tco.c > > > > > +++ b/drivers/watchdog/sp5100_tco.c > > > > > @@ -48,12 +48,14 @@ > > > > > /* internal variables */ > > > > > enum tco_reg_layout { > > > > > - sp5100, sb800, efch > > > > > + sp5100, sb800, efch, efch_mmio > > > > > }; > > > > > struct sp5100_tco { > > > > > struct watchdog_device wdd; > > > > > void __iomem *tcobase; > > > > > + void __iomem *addr; > > > > > + struct resource *res; > > > > > > > > I must admit that I really don't like this code. Both res and > > > > addr are only used during initialization, yet their presence suggests > > > > runtime usage. Any chance to reqork this to not require those variables ? > > > > We did that in an earlier version, see struct efch_cfg of: > > > > https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/ > > > > The motivation of it was the same as you suggested to only use it > > during init. > > > > Having it in struct sp5100_tco made things simpler esp. in the > > definition of the function interfaces where those new members are > > used. > So, no, I neither see the need for having the information in struct > sp5100_tco nor for keeping it in its own structure. If you'd merge > sp5100_request_region_mmio() and sp5100_release_region_mmio() into > sp5100_tco_setupdevice_mmio() you would not even need any pointers > to pass the values from sp5100_request_region_mmio(). Otherwise you > could have sp5100_request_region_mmio() return a pointer to res or > an ERR_PTR, and pass the address as pointer parameter. Yes, that is feasible, in fact it is option 2) I suggested. -Robert > > If that init vars are no longer in struct sp5100_tco then callers of > > efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a > > pointer to them. To avoid this I see those options: > > > > 1) Implement them as global (or a single global struct) and possibly > > protect it by a mutex. There is only a single device anyway and we > > wouldn't need a protection. > > > > 2) Have an own mmio implementation of tco_timer_enable() and/or > > sp5100_tco_timer_init(). > > > > > Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also > > > correct the trailing newline you mentioned in an earlier email. > > > > > > Regards, > > > Terry > > > > > > > > enum tco_reg_layout tco_reg_layout; > > > > While at it, tco_reg_layout is also only used during initialization > > and can be moved there too. This would raise option 3: > > > > 3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that > > struct instead in init funtions only. But that causes the most rework > > (which would be ok to me). > > > > Going with 3) looks the cleanest way, I would try that. But all > > options have its advantages. > > > > -Robert > > > > > > > }; >
diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c index 80ae42ae7aaa..4777e672a8ad 100644 --- a/drivers/watchdog/sp5100_tco.c +++ b/drivers/watchdog/sp5100_tco.c @@ -48,12 +48,14 @@ /* internal variables */ enum tco_reg_layout { - sp5100, sb800, efch + sp5100, sb800, efch, efch_mmio }; struct sp5100_tco { struct watchdog_device wdd; void __iomem *tcobase; + void __iomem *addr; + struct resource *res; enum tco_reg_layout tco_reg_layout; }; @@ -161,6 +163,59 @@ static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set) outb(val, SP5100_IO_PM_DATA_REG); } +static int sp5100_request_region_mmio(struct device *dev, + struct watchdog_device *wdd) +{ + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); + struct resource *res; + void __iomem *addr; + + res = request_mem_region(EFCH_PM_ACPI_MMIO_PM_ADDR, + EFCH_PM_ACPI_MMIO_PM_SIZE, + "sp5100_tco"); + + if (!res) { + dev_err(dev, + "SMB base address memory region 0x%x already in use.\n", + EFCH_PM_ACPI_MMIO_PM_ADDR); + return -EBUSY; + } + + addr = ioremap(EFCH_PM_ACPI_MMIO_PM_ADDR, + EFCH_PM_ACPI_MMIO_PM_SIZE); + if (!addr) { + release_resource(res); + dev_err(dev, "SMB base address mapping failed.\n"); + return -ENOMEM; + } + + tco->res = res; + tco->addr = addr; + return 0; +} + +static void sp5100_release_region_mmio(struct sp5100_tco *tco) +{ + iounmap(tco->addr); + release_resource(tco->res); +} + +static u8 efch_read_pm_reg8(struct sp5100_tco *tco, u8 index) +{ + return readb(tco->addr + index); +} + +static void efch_update_pm_reg8(struct sp5100_tco *tco, + u8 index, u8 reset, u8 set) +{ + u8 val; + + val = readb(tco->addr + index); + val &= reset; + val |= set; + writeb(val, tco->addr + index); +} + static void tco_timer_enable(struct sp5100_tco *tco) { u32 val; @@ -201,6 +256,12 @@ static void tco_timer_enable(struct sp5100_tco *tco) ~EFCH_PM_WATCHDOG_DISABLE, EFCH_PM_DECODEEN_SECOND_RES); break; + case efch_mmio: + /* Set the Watchdog timer resolution to 1 sec and enable */ + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN3, + ~EFCH_PM_WATCHDOG_DISABLE, + EFCH_PM_DECODEEN_SECOND_RES); + break; } } @@ -313,6 +374,44 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco) return 0; } +static int sp5100_tco_setupdevice_mmio(struct device *dev, + struct watchdog_device *wdd) +{ + struct sp5100_tco *tco = watchdog_get_drvdata(wdd); + const char *dev_name = SB800_DEVNAME; + u32 mmio_addr = 0, alt_mmio_addr = 0; + int ret; + + ret = sp5100_request_region_mmio(dev, wdd); + if (ret) + return ret; + + /* Determine MMIO base address */ + if (!(efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & + EFCH_PM_DECODEEN_WDT_TMREN)) { + efch_update_pm_reg8(tco, EFCH_PM_DECODEEN, + 0xff, + EFCH_PM_DECODEEN_WDT_TMREN); + } + + if (efch_read_pm_reg8(tco, EFCH_PM_DECODEEN) & + EFCH_PM_DECODEEN_WDT_TMREN) + mmio_addr = EFCH_PM_WDT_ADDR; + + /* Determine alternate MMIO base address */ + if (efch_read_pm_reg8(tco, EFCH_PM_ISACONTROL) & + EFCH_PM_ISACONTROL_MMIOEN) + alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR + + EFCH_PM_ACPI_MMIO_WDT_OFFSET; + + ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name); + if (!ret) + ret = sp5100_tco_timer_init(tco); + + sp5100_release_region_mmio(tco); + return ret; +} + static int sp5100_tco_setupdevice(struct device *dev, struct watchdog_device *wdd) { @@ -322,6 +421,9 @@ static int sp5100_tco_setupdevice(struct device *dev, u32 alt_mmio_addr = 0; int ret; + if (tco->tco_reg_layout == efch_mmio) + return sp5100_tco_setupdevice_mmio(dev, wdd); + /* Request the IO ports used by this driver */ if (!request_muxed_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE, "sp5100_tco")) { diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h index adf015aa4126..73f179a1d6e5 100644 --- a/drivers/watchdog/sp5100_tco.h +++ b/drivers/watchdog/sp5100_tco.h @@ -83,3 +83,9 @@ #define EFCH_PM_ACPI_MMIO_ADDR 0xfed80000 #define EFCH_PM_ACPI_MMIO_WDT_OFFSET 0x00000b00 +#define EFCH_PM_ACPI_MMIO_PM_OFFSET 0x00000300 + +#define EFCH_PM_ACPI_MMIO_PM_ADDR (EFCH_PM_ACPI_MMIO_ADDR + \ + EFCH_PM_ACPI_MMIO_PM_OFFSET) +#define EFCH_PM_ACPI_MMIO_PM_SIZE 8 +