| Message ID | 20220118202234.410555-4-terry.bowman@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Watchdog: sp5100_tco: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand |
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 <rrichter@amd.com> > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > To: Guenter Roeck <linux@roeck-us.net> > To: linux-watchdog@vger.kernel.org > To: Jean Delvare <jdelvare@suse.com> > To: linux-i2c@vger.kernel.org > To: Wolfram Sang <wsa@kernel.org> > To: Andy Shevchenko <andy.shevchenko@gmail.com> > To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: linux-kernel@vger.kernel.org > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Robert Richter <rrichter@amd.com> > Cc: Thomas Lendacky <thomas.lendacky@amd.com> > --- > 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 <jdelvare@suse.de>
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 <jdelvare@suse.de> > I'm finishing up revisions for this and the i2c patch. I expect to send both tomorrow. Regards, Teryr
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 <rrichter@amd.com> >> Signed-off-by: Robert Richter <rrichter@amd.com> >> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >> To: Guenter Roeck <linux@roeck-us.net> >> To: linux-watchdog@vger.kernel.org >> To: Jean Delvare <jdelvare@suse.com> >> To: linux-i2c@vger.kernel.org >> To: Wolfram Sang <wsa@kernel.org> >> To: Andy Shevchenko <andy.shevchenko@gmail.com> >> To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Cc: linux-kernel@vger.kernel.org >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >> Cc: Robert Richter <rrichter@amd.com> >> Cc: Thomas Lendacky <thomas.lendacky@amd.com> >> --- >> 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 <jdelvare@suse.de> > Hi Jean, Is your "Tested-by" for patch 3/4 or the sp5100_tco series? Regards, Terry
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.
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