* [PATCH 00/12] watchdog: sp5100_tco: Various improvements
@ 2017-12-24 21:03 Guenter Roeck
2017-12-24 21:03 ` [PATCH 01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG} Guenter Roeck
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Guenter Roeck @ 2017-12-24 21:03 UTC (permalink / raw
To: Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel,
Zoltán Böszörményi, Guenter Roeck
The sp5100_tco watchdog driver does not really support recent AMD CPUs,
even though it claims to do so. On top of that, it doesn't use the
watchdog subsystem, and various other problems have crept in. Let's
clean it up for good.
The code was tested on AMD Ryzen 1700 with motherboards from MSI and
Gigabyte. Tests on older hardware would be useful to make sure that
nothing broke.
----------------------------------------------------------------
Guenter Roeck (12):
watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG}
watchdog: sp5100_tco: Fix watchdog disable bit
watchdog: sp5100_tco: Use request_muxed_region where possible
watchdog: sp5100_tco: Use standard error codes
watchdog: sp5100_tco: Clean up sp5100_tco_setupdevice
watchdog: sp5100_tco: Match PCI device early
watchdog: sp5100_tco: Use dev_ print functions where possible
watchdog: sp5100_tco: Clean up function and variable names
watchdog: sp5100_tco: Convert to use watchdog subsystem
watchdog: sp5100_tco: Use bit operations
watchdog: sp5100-tco: Abort if watchdog is disabled by hardware
watchdog: sp5100_tco: Add support for recent FCH versions
drivers/watchdog/sp5100_tco.c | 710 ++++++++++++++++++------------------------
drivers/watchdog/sp5100_tco.h | 57 ++--
2 files changed, 344 insertions(+), 423 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG}
2017-12-24 21:03 [PATCH 00/12] watchdog: sp5100_tco: Various improvements Guenter Roeck
@ 2017-12-24 21:03 ` Guenter Roeck
2018-01-16 19:34 ` [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG} Lyude Paul
2017-12-24 21:03 ` [PATCH 02/12] watchdog: sp5100_tco: Fix watchdog disable bit Guenter Roeck
2018-01-04 11:41 ` [PATCH 00/12] watchdog: sp5100_tco: Various improvements Boszormenyi Zoltan
2 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2017-12-24 21:03 UTC (permalink / raw
To: Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel,
Zoltán Böszörményi, Guenter Roeck
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@pr.hu>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
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);
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
-
#define SB800_PM_ACPI_MMIO_EN 0x24
#define SB800_PM_WATCHDOG_CONTROL 0x48
#define SB800_PM_WATCHDOG_BASE 0x48
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 02/12] watchdog: sp5100_tco: Fix watchdog disable bit
2017-12-24 21:03 [PATCH 00/12] watchdog: sp5100_tco: Various improvements Guenter Roeck
2017-12-24 21:03 ` [PATCH 01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG} Guenter Roeck
@ 2017-12-24 21:03 ` Guenter Roeck
2018-01-16 19:34 ` [02/12] " Lyude Paul
2018-01-04 11:41 ` [PATCH 00/12] watchdog: sp5100_tco: Various improvements Boszormenyi Zoltan
2 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2017-12-24 21:03 UTC (permalink / raw
To: Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel,
Zoltán Böszörményi, Guenter Roeck
According to all published information, the watchdog disable bit for SB800
compatible controllers is bit 1 of PM register 0x48, not bit 2. For the
most part that doesn't matter in practice, since the bit has to be cleared
to enable watchdog address decoding, which is the default setting, but it
still needs to be fixed.
Cc: Zoltán Böszörményi <zboszor@pr.hu>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/watchdog/sp5100_tco.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
index f495fe03887a..2622cfe23dc1 100644
--- a/drivers/watchdog/sp5100_tco.h
+++ b/drivers/watchdog/sp5100_tco.h
@@ -52,7 +52,7 @@
#define SB800_PM_WATCHDOG_CONFIG 0x4C
#define SB800_PCI_WATCHDOG_DECODE_EN (1 << 0)
-#define SB800_PM_WATCHDOG_DISABLE (1 << 2)
+#define SB800_PM_WATCHDOG_DISABLE (1 << 1)
#define SB800_PM_WATCHDOG_SECOND_RES (3 << 0)
#define SB800_ACPI_MMIO_DECODE_EN (1 << 0)
#define SB800_ACPI_MMIO_SEL (1 << 1)
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 00/12] watchdog: sp5100_tco: Various improvements
2017-12-24 21:03 [PATCH 00/12] watchdog: sp5100_tco: Various improvements Guenter Roeck
2017-12-24 21:03 ` [PATCH 01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG} Guenter Roeck
2017-12-24 21:03 ` [PATCH 02/12] watchdog: sp5100_tco: Fix watchdog disable bit Guenter Roeck
@ 2018-01-04 11:41 ` Boszormenyi Zoltan
2 siblings, 0 replies; 10+ messages in thread
From: Boszormenyi Zoltan @ 2018-01-04 11:41 UTC (permalink / raw
To: Guenter Roeck, Wim Van Sebroeck; +Cc: linux-watchdog, linux-kernel
Thanks for taking up this work.
I will test it and the other small series for i2c-piix4 as soon as possible.
2017-12-24 22:03 keltezéssel, Guenter Roeck írta:
> The sp5100_tco watchdog driver does not really support recent AMD CPUs,
> even though it claims to do so. On top of that, it doesn't use the
> watchdog subsystem, and various other problems have crept in. Let's
> clean it up for good.
>
> The code was tested on AMD Ryzen 1700 with motherboards from MSI and
> Gigabyte. Tests on older hardware would be useful to make sure that
> nothing broke.
>
> ----------------------------------------------------------------
> Guenter Roeck (12):
> watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG}
> watchdog: sp5100_tco: Fix watchdog disable bit
> watchdog: sp5100_tco: Use request_muxed_region where possible
> watchdog: sp5100_tco: Use standard error codes
> watchdog: sp5100_tco: Clean up sp5100_tco_setupdevice
> watchdog: sp5100_tco: Match PCI device early
> watchdog: sp5100_tco: Use dev_ print functions where possible
> watchdog: sp5100_tco: Clean up function and variable names
> watchdog: sp5100_tco: Convert to use watchdog subsystem
> watchdog: sp5100_tco: Use bit operations
> watchdog: sp5100-tco: Abort if watchdog is disabled by hardware
> watchdog: sp5100_tco: Add support for recent FCH versions
>
> drivers/watchdog/sp5100_tco.c | 710 ++++++++++++++++++------------------------
> drivers/watchdog/sp5100_tco.h | 57 ++--
> 2 files changed, 344 insertions(+), 423 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}
2017-12-24 21:03 ` [PATCH 01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG} Guenter Roeck
@ 2018-01-16 19:34 ` Lyude Paul
2018-01-16 20:11 ` Guenter Roeck
2018-01-17 1:10 ` Guenter Roeck
0 siblings, 2 replies; 10+ messages in thread
From: Lyude Paul @ 2018-01-16 19:34 UTC (permalink / raw
To: Guenter Roeck, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel,
Zoltán Böszörményi
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@pr.hu>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 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?
> 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.
> #define SB800_PM_ACPI_MMIO_EN 0x24
> #define SB800_PM_WATCHDOG_CONTROL 0x48
> #define SB800_PM_WATCHDOG_BASE 0x48
--
Cheers,
Lyude Paul
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [02/12] watchdog: sp5100_tco: Fix watchdog disable bit
2017-12-24 21:03 ` [PATCH 02/12] watchdog: sp5100_tco: Fix watchdog disable bit Guenter Roeck
@ 2018-01-16 19:34 ` Lyude Paul
0 siblings, 0 replies; 10+ messages in thread
From: Lyude Paul @ 2018-01-16 19:34 UTC (permalink / raw
To: Guenter Roeck, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel,
Zoltán Böszörményi
On Sun, 2017-12-24 at 13:03 -0800, Guenter Roeck wrote:
> According to all published information, the watchdog disable bit for SB800
> compatible controllers is bit 1 of PM register 0x48, not bit 2. For the
> most part that doesn't matter in practice, since the bit has to be cleared
> to enable watchdog address decoding, which is the default setting, but it
> still needs to be fixed.
Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> Cc: Zoltán Böszörményi <zboszor@pr.hu>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/watchdog/sp5100_tco.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h
> index f495fe03887a..2622cfe23dc1 100644
> --- a/drivers/watchdog/sp5100_tco.h
> +++ b/drivers/watchdog/sp5100_tco.h
> @@ -52,7 +52,7 @@
> #define SB800_PM_WATCHDOG_CONFIG 0x4C
>
> #define SB800_PCI_WATCHDOG_DECODE_EN (1 << 0)
> -#define SB800_PM_WATCHDOG_DISABLE (1 << 2)
> +#define SB800_PM_WATCHDOG_DISABLE (1 << 1)
> #define SB800_PM_WATCHDOG_SECOND_RES (3 << 0)
> #define SB800_ACPI_MMIO_DECODE_EN (1 << 0)
> #define SB800_ACPI_MMIO_SEL (1 << 1)
--
Cheers,
Lyude Paul
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}
2018-01-16 19:34 ` [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG} Lyude Paul
@ 2018-01-16 20:11 ` Guenter Roeck
2018-01-16 20:17 ` Lyude Paul
2018-01-17 1:10 ` Guenter Roeck
1 sibling, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2018-01-16 20:11 UTC (permalink / raw
To: Lyude Paul
Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel,
Zoltán Böszörményi
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@pr.hu>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}
2018-01-16 20:11 ` Guenter Roeck
@ 2018-01-16 20:17 ` Lyude Paul
2018-01-16 22:37 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Lyude Paul @ 2018-01-16 20:17 UTC (permalink / raw
To: Guenter Roeck
Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel,
Zoltán Böszörményi
Oh has this patchset already been reviwed and pushed upstream? I checked
patchwork beforehand and it looked like it was still pending
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@pr.hu>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > 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
--
Cheers,
Lyude Paul
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}
2018-01-16 20:17 ` Lyude Paul
@ 2018-01-16 22:37 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2018-01-16 22:37 UTC (permalink / raw
To: Lyude Paul
Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel,
Zoltán Böszörményi
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@pr.hu>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG}
2018-01-16 19:34 ` [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG} Lyude Paul
2018-01-16 20:11 ` Guenter Roeck
@ 2018-01-17 1:10 ` Guenter Roeck
1 sibling, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2018-01-17 1:10 UTC (permalink / raw
To: Lyude Paul, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel,
Zoltán Böszörményi
On 01/16/2018 11:34 AM, 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@pr.hu>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> 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?
>> 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.
>
Wait a minute, you are tricking me here. SP5100_IO_PM_INDEX_REG and
SP5100_IO_PM_DATA_REG were introduced with commit 15e28bf130081 in 2010.
The SB800 defines were introduced with commit 740fbddf5c3f9 in 2012.
The common approach is not to change defines after they were introduced,
which is exactly what I already did. I'll leave the code as-is.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-01-17 1:10 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-24 21:03 [PATCH 00/12] watchdog: sp5100_tco: Various improvements Guenter Roeck
2017-12-24 21:03 ` [PATCH 01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,DATA_REG} Guenter Roeck
2018-01-16 19:34 ` [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG, DATA_REG} Lyude Paul
2018-01-16 20:11 ` Guenter Roeck
2018-01-16 20:17 ` Lyude Paul
2018-01-16 22:37 ` Guenter Roeck
2018-01-17 1:10 ` Guenter Roeck
2017-12-24 21:03 ` [PATCH 02/12] watchdog: sp5100_tco: Fix watchdog disable bit Guenter Roeck
2018-01-16 19:34 ` [02/12] " Lyude Paul
2018-01-04 11:41 ` [PATCH 00/12] watchdog: sp5100_tco: Various improvements Boszormenyi Zoltan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.