Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}

From: Guenter Roeck
Date: Tue Jan 16 2018 - 17:37:50 EST

Next message: Wolfram Sang: "Re: [PATCH v2 20/22] mmc: tmio: move clk_enable/disable out of tmio_mmc_host_probe()"
Previous message: Andy Lutomirski: "Re: [PATCH 02/16] x86/entry/32: Enter the kernel via trampoline stack"
In reply to: Lyude Paul: "Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}"
Next in thread: Guenter Roeck: "Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 01/16/2018 12:17 PM, Lyude Paul wrote:

Oh has this patchset already been reviwed and pushed upstream? I checked
patchwork beforehand and it looked like it was still pending

Depends on Wim what he wants to do with it. We'll see when the commit window
opens.

Guenter

On Tue, 2018-01-16 at 12:11 -0800, Guenter Roeck wrote:

On Tue, Jan 16, 2018 at 02:34:19PM -0500, Lyude Paul wrote:

Sorry this review took so long! Just sent off a big patchset myself to
nouveau's mailing list.

Anyway, unfortunately it should be noted that as I've learned from this
thread
that I have no machines that i know of with an actual sp5100_tco. Might
double
check though to see if I'm wrong, but for now this is all solely review.

Overall, nice patch! I'm glad to see ugly corners of the kernel being
cleaned
up like this :). Some comments below

On Sun, 2017-12-24 at 13:03 -0800, Guenter Roeck wrote:

SP5100_IO_PM_INDEX_REG and SB800_IO_PM_INDEX_REG are used inconsistently
and define the same value. Just use SP5100_IO_PM_INDEX_REG throughout.
Do the same for SP5100_IO_PM_DATA_REG and SB800_IO_PM_DATA_REG.
Use helper functions to access the indexed registers.

Cc: ZoltÃn BÃszÃrmÃnyi <zboszor@xxxxx>
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
drivers/watchdog/sp5100_tco.c | 94 ++++++++++++++++++++++------------
----
-----
drivers/watchdog/sp5100_tco.h | 7 +---
2 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c
b/drivers/watchdog/sp5100_tco.c
index 028618c5eeba..05f9d27a306a 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -48,7 +48,6 @@
static u32 tcobase_phys;
static u32 tco_wdt_fired;
static void __iomem *tcobase;
-static unsigned int pm_iobase;
static DEFINE_SPINLOCK(tco_lock); /* Guards the hardware */
static unsigned long timer_alive;
static char tco_expect_close;
@@ -132,25 +131,38 @@ static int tco_timer_set_heartbeat(int t)
return 0;
}
-static void tco_timer_enable(void)
+static u8 sp5100_tco_read_pm_reg8(u8 index)
+{
+ outb(index, SP5100_IO_PM_INDEX_REG);
+ return inb(SP5100_IO_PM_DATA_REG);
+}
+
+static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set)
{
- int val;
+ u8 val;
+ outb(index, SP5100_IO_PM_INDEX_REG);
+ val = inb(SP5100_IO_PM_DATA_REG);
+ val &= reset;
+ val |= set;
+ outb(val, SP5100_IO_PM_DATA_REG);
+}
+
+static void tco_timer_enable(void)
+{
if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
/* For SB800 or later */
/* Set the Watchdog timer resolution to 1 sec */
- outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
- val = inb(SB800_IO_PM_DATA_REG);
- val |= SB800_PM_WATCHDOG_SECOND_RES;
- outb(val, SB800_IO_PM_DATA_REG);
+ sp5100_tco_update_pm_reg8(SB800_PM_WATCHDOG_CONFIG,
+ 0xff,
SB800_PM_WATCHDOG_SECOND_RES);
/* Enable watchdog decode bit and watchdog timer */
- outb(SB800_PM_WATCHDOG_CONTROL, SB800_IO_PM_INDEX_REG);
- val = inb(SB800_IO_PM_DATA_REG);
- val |= SB800_PCI_WATCHDOG_DECODE_EN;
- val &= ~SB800_PM_WATCHDOG_DISABLE;
- outb(val, SB800_IO_PM_DATA_REG);
+ sp5100_tco_update_pm_reg8(SB800_PM_WATCHDOG_CONTROL,
+ ~SB800_PM_WATCHDOG_DISABLE,
+ SB800_PCI_WATCHDOG_DECODE_EN)
;
} else {
+ u32 val;
+
/* For SP5100 or SB7x0 */
/* Enable watchdog decode bit */
pci_read_config_dword(sp5100_tco_pci,
@@ -164,11 +176,9 @@ static void tco_timer_enable(void)
val);
/* Enable Watchdog timer and set the resolution to 1
sec */
- outb(SP5100_PM_WATCHDOG_CONTROL,
SP5100_IO_PM_INDEX_REG);
- val = inb(SP5100_IO_PM_DATA_REG);
- val |= SP5100_PM_WATCHDOG_SECOND_RES;
- val &= ~SP5100_PM_WATCHDOG_DISABLE;
- outb(val, SP5100_IO_PM_DATA_REG);
+ sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
+ ~SP5100_PM_WATCHDOG_DISABLE,
+ SP5100_PM_WATCHDOG_SECOND_RES
);
}
}
@@ -321,6 +331,17 @@ static const struct pci_device_id
sp5100_tco_pci_tbl[]
= {
};
MODULE_DEVICE_TABLE(pci, sp5100_tco_pci_tbl);
+static u8 sp5100_tco_read_pm_reg32(u8 index)
+{
+ u32 val = 0;
+ int i;
+
+ for (i = 3; i >= 0; i--)
+ val = (val << 8) + sp5100_tco_read_pm_reg8(index + i);
+
+ return val;
+}
+
/*
* Init & exit routines
*/
@@ -329,7 +350,7 @@ static unsigned char sp5100_tco_setupdevice(void)
struct pci_dev *dev = NULL;
const char *dev_name = NULL;
u32 val;
- u32 index_reg, data_reg, base_addr;
+ u8 base_addr;
/* Match the PCI device */
for_each_pci_dev(dev) {
@@ -351,35 +372,25 @@ static unsigned char sp5100_tco_setupdevice(void)
*/
if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
dev_name = SP5100_DEVNAME;
- index_reg = SP5100_IO_PM_INDEX_REG;
- data_reg = SP5100_IO_PM_DATA_REG;
base_addr = SP5100_PM_WATCHDOG_BASE;
} else {
dev_name = SB800_DEVNAME;
- index_reg = SB800_IO_PM_INDEX_REG;
- data_reg = SB800_IO_PM_DATA_REG;
base_addr = SB800_PM_WATCHDOG_BASE;
}
/* Request the IO ports used by this driver */
- pm_iobase = SP5100_IO_PM_INDEX_REG;
- if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE,
dev_name)) {
- pr_err("I/O address 0x%04x already in use\n",
pm_iobase);
+ if (!request_region(SP5100_IO_PM_INDEX_REG,
SP5100_PM_IOPORTS_SIZE,
+ dev_name)) {
+ pr_err("I/O address 0x%04x already in use\n",
+ SP5100_IO_PM_INDEX_REG);

It's good we're printing an error here, but I also don't think (since this
has
happened a couple times already even just on this thread) we should simply
leave the user with the impression that the driver actually failed if the
real
situation is that the user is supposed to use w83627hf_wdt or some other
watchdog driver. Maybe leave a suggestion in this message to the user to
try
another driver, or see if we could improve this module's ability to
realize
that it's not actually supported on the system it's being loaded on?

Keep in mind that this is not the only driver in the system which doesn't
apply for a given hardware. We would end up with an endless amount of
messages if we were to do that.

goto exit;
}
/*
* First, Find the watchdog timer MMIO address from indirect
I/O.
+ * Low three bits of BASE are reserved.
*/
- outb(base_addr+3, index_reg);
- val = inb(data_reg);
- outb(base_addr+2, index_reg);
- val = val << 8 | inb(data_reg);
- outb(base_addr+1, index_reg);
- val = val << 8 | inb(data_reg);
- outb(base_addr+0, index_reg);
- /* Low three bits of BASE are reserved */
- val = val << 8 | (inb(data_reg) & 0xf8);
+ val = sp5100_tco_read_pm_reg32(base_addr) & 0xfffffff8;
pr_debug("Got 0x%04x from indirect I/O\n", val);
@@ -400,14 +411,7 @@ static unsigned char sp5100_tco_setupdevice(void)
SP5100_SB_RESOURCE_MMIO_BASE,
&val);
} else {
/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
- outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
- val = inb(SB800_IO_PM_DATA_REG);
- outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
- val = val << 8 | inb(SB800_IO_PM_DATA_REG);
- outb(SB800_PM_ACPI_MMIO_EN+1, SB800_IO_PM_INDEX_REG);
- val = val << 8 | inb(SB800_IO_PM_DATA_REG);
- outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
- val = val << 8 | inb(SB800_IO_PM_DATA_REG);
+ val = sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN);
}
/* The SBResource_MMIO is enabled and mapped memory space? */
@@ -470,7 +474,7 @@ static unsigned char sp5100_tco_setupdevice(void)
unreg_mem_region:
release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
unreg_region:
- release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
+ release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
exit:
return 0;
}
@@ -517,7 +521,7 @@ static int sp5100_tco_init(struct platform_device
*dev)
exit:
iounmap(tcobase);
release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
- release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
+ release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
return ret;
}
@@ -531,7 +535,7 @@ static void sp5100_tco_cleanup(void)
misc_deregister(&sp5100_tco_miscdev);
iounmap(tcobase);
release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
- release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
+ release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
}
static int sp5100_tco_remove(struct platform_device *dev)
diff --git a/drivers/watchdog/sp5100_tco.h
b/drivers/watchdog/sp5100_tco.h
index 1af4dee71337..f495fe03887a 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -24,10 +24,11 @@
* read them from a register.
*/
-/* For SP5100/SB7x0 chipset */
+/* For SP5100/SB7x0/SB8x0 chipset */
#define SP5100_IO_PM_INDEX_REG 0xCD6
#define SP5100_IO_PM_DATA_REG 0xCD7
+/* For SP5100/SB7x0 chipset */
#define SP5100_SB_RESOURCE_MMIO_BASE 0x9C
#define SP5100_PM_WATCHDOG_CONTROL 0x69
@@ -44,11 +45,7 @@
#define SP5100_DEVNAME "SP5100 TCO"
-
/* For SB8x0(or later) chipset */
-#define SB800_IO_PM_INDEX_REG 0xCD6
-#define SB800_IO_PM_DATA_REG 0xCD7
-

IMO I would leave these macro definitions as SB800. For DRM drivers at
least,
we usually take the route of naming commonly used registers/methods after
the
first generation they appeared in, not the last.

That may apply if there is only one set of defines. We have two
sets here. Agreed, I could have removed SP5100 instead.
As it is, I'd rather have the series applied to v4.16 instead
of making cosmetic changes. I'll consider your suggestion if the
series does not make it into 4.16.

Thanks,
Guenter

#define SB800_PM_ACPI_MMIO_EN 0x24
#define SB800_PM_WATCHDOG_CONTROL 0x48
#define SB800_PM_WATCHDOG_BASE 0x48

--
Cheers,
Lyude Paul

Next message: Wolfram Sang: "Re: [PATCH v2 20/22] mmc: tmio: move clk_enable/disable out of tmio_mmc_host_probe()"
Previous message: Andy Lutomirski: "Re: [PATCH 02/16] x86/entry/32: Enter the kernel via trampoline stack"
In reply to: Lyude Paul: "Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}"
Next in thread: Guenter Roeck: "Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]