| Message ID | 20210302163309.25528-1-henning.schild@siemens.com |
|---|---|
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
On 3/2/21 8:33 AM, Henning Schild wrote: > From: Henning Schild <henning.schild@siemens.com> > > This driver adds initial support for several devices from Siemens. It is > based on a platform driver introduced in an earlier commit. > > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > Signed-off-by: Henning Schild <henning.schild@siemens.com> > --- > drivers/watchdog/Kconfig | 11 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/simatic-ipc-wdt.c | 305 +++++++++++++++++++++++++++++ > 3 files changed, 317 insertions(+) > create mode 100644 drivers/watchdog/simatic-ipc-wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 1fe0042a48d2..948497eb4bef 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -1575,6 +1575,17 @@ config NIC7018_WDT > To compile this driver as a module, choose M here: the module will be > called nic7018_wdt. > > +config SIEMENS_SIMATIC_IPC_WDT > + tristate "Siemens Simatic IPC Watchdog" > + depends on SIEMENS_SIMATIC_IPC > + select WATCHDOG_CORE > + help > + This driver adds support for several watchdogs found in Industrial > + PCs from Siemens. > + > + To compile this driver as a module, choose M here: the module will be > + called simatic-ipc-wdt. > + > # M68K Architecture > > config M54xx_WATCHDOG > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index f3a6540e725e..7f5c73ec058c 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o > obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o > obj-$(CONFIG_MLX_WDT) += mlx_wdt.o > obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o > > # M68K Architecture > obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o > diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c > new file mode 100644 > index 000000000000..b5c8b7ceb404 > --- /dev/null > +++ b/drivers/watchdog/simatic-ipc-wdt.c > @@ -0,0 +1,305 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Siemens SIMATIC IPC driver for Watchdogs > + * > + * Copyright (c) Siemens AG, 2020-2021 > + * > + * Authors: > + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. Covered by SPDX-License-Identifier > + */ > + > +#include <linux/device.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/errno.h> > +#include <linux/watchdog.h> > +#include <linux/ioport.h> > +#include <linux/sizes.h>> +#include <linux/io.h> > +#include <linux/platform_data/x86/simatic-ipc-base.h> Alphabetic order please > + > +#define WD_ENABLE_IOADR 0x62 > +#define WD_TRIGGER_IOADR 0x66 > +#define GPIO_COMMUNITY0_PORT_ID 0xaf > +#define PAD_CFG_DW0_GPP_A_23 0x4b8 Please increase indentation and spare another tab > +#define SAFE_EN_N_427E 0x01 > +#define SAFE_EN_N_227E 0x04 > +#define WD_ENABLED 0x01 > + > +#define TIMEOUT_MIN 2 > +#define TIMEOUT_DEF 64 > +#define TIMEOUT_MAX 64 > + > +#define GP_STATUS_REG_227E 0x404D /* IO PORT for SAFE_EN_N on 227E */ > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +module_param(nowayout, bool, 0000); > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > + > +static DEFINE_SPINLOCK(io_lock); /* the lock for io operations */ > +static struct watchdog_device wdd; > + Having two variables named 'wdd' is confusing. Please chose another name. > +static struct resource gp_status_reg_227e_res = > + DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, KBUILD_MODNAME); > + > +static struct resource io_resource = > + DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1, > + KBUILD_MODNAME " WD_ENABLE_IOADR"); > + > +/* the actual start will be discovered with pci, 0 is a placeholder */ > +static struct resource mem_resource = > + DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR"); > + > +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 }; > +static void __iomem *wd_reset_base_addr; > + > +static int get_timeout_idx(u32 timeout) > +{ > + int i; > + > + i = ARRAY_SIZE(wd_timeout_table) - 1; > + for (; i >= 0; i--) { > + if (timeout >= wd_timeout_table[i]) > + break; > + } > + > + return i; > +} Please add a comment explaining why you don't use find_closest(). > + > +static int wd_start(struct watchdog_device *wdd) > +{ > + u8 regval; > + > + spin_lock(&io_lock); > + The watchdog subsystem already provides locking since the watchdog device can only be opened once. Why is the additional lock needed ? > + regval = inb(WD_ENABLE_IOADR); > + regval |= WD_ENABLED; > + outb(regval, WD_ENABLE_IOADR); > + > + spin_unlock(&io_lock); > + > + return 0; > +} > + > +static int wd_stop(struct watchdog_device *wdd) > +{ > + u8 regval; > + > + spin_lock(&io_lock); > + > + regval = inb(WD_ENABLE_IOADR); > + regval &= ~WD_ENABLED; > + outb(regval, WD_ENABLE_IOADR); > + > + spin_unlock(&io_lock); > + > + return 0; > +} > + > +static int wd_ping(struct watchdog_device *wdd) > +{ > + inb(WD_TRIGGER_IOADR); > + return 0; > +} > + > +static int wd_set_timeout(struct watchdog_device *wdd, unsigned int t) > +{ > + u8 regval; > + int timeout_idx = get_timeout_idx(t); > + > + spin_lock(&io_lock); > + > + regval = inb(WD_ENABLE_IOADR) & 0xc7; > + regval |= timeout_idx << 3; > + outb(regval, WD_ENABLE_IOADR); > + > + spin_unlock(&io_lock); > + wdd->timeout = wd_timeout_table[timeout_idx]; > + > + return 0; > +} > + > +static const struct watchdog_info wdt_ident = { > + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | > + WDIOF_SETTIMEOUT, > + .identity = KBUILD_MODNAME, > +}; > + > +static const struct watchdog_ops wdt_ops = { > + .owner = THIS_MODULE, > + .start = wd_start, > + .stop = wd_stop, > + .ping = wd_ping, > + .set_timeout = wd_set_timeout, > +}; > + > +static void wd_set_safe_en_n(u32 wdtmode, bool safe_en_n) > +{ > + u16 resetbit; > + > + if (wdtmode == SIMATIC_IPC_DEVICE_227E) { > + /* enable SAFE_EN_N on GP_STATUS_REG_227E */ > + resetbit = inw(GP_STATUS_REG_227E); > + if (safe_en_n) > + resetbit &= ~SAFE_EN_N_227E; > + else > + resetbit |= SAFE_EN_N_227E; > + > + outw(resetbit, GP_STATUS_REG_227E); > + } else { > + /* enable SAFE_EN_N on PCH D1600 */ > + resetbit = ioread16(wd_reset_base_addr); > + > + if (safe_en_n) > + resetbit &= ~SAFE_EN_N_427E; > + else > + resetbit |= SAFE_EN_N_427E; > + > + iowrite16(resetbit, wd_reset_base_addr); > + } > +} > + > +static int wd_setup(u32 wdtmode, bool safe_en_n) > +{ > + u8 regval; > + int timeout_idx = 0; Unnecessary initialization > + bool alarm_active; > + > + timeout_idx = get_timeout_idx(TIMEOUT_DEF); > + > + wd_set_safe_en_n(wdtmode, safe_en_n); > + > + /* read wd register to determine alarm state */ > + regval = inb(WD_ENABLE_IOADR); > + if (regval & 0x80) { > + pr_warn("Watchdog alarm active.\n"); Why does that warrant a warning, and what does it mean ? The context suggests that it means the previous reset was caused by the watchdog, but that is not what the message says. > + regval = 0x82; /* use only macro mode, reset alarm bit */ > + alarm_active = true; > + } else { > + regval = 0x02; /* use only macro mode */ > + alarm_active = false; > + } Would it hurt to just always write 0x82 ? alarm_active = regval & 0x80; regval = 0x82 | timeout_idx << 3; would be much simpler. Or, if you prefer, alarm_active = !!(regval & 0x80); regval = 0x82 | timeout_idx << 3; Actually, regval isn't even needed in that case. alarm_active = !!(regval & 0x80); outb(0x82 | timeout_idx << 3, WD_ENABLE_IOADR); Either case, please use defines for the bits. WD_ENABLED is already defined, thus the other bits should be set using defines as well. > + > + regval |= timeout_idx << 3; > + if (nowayout) > + regval |= WD_ENABLED; This is not the purpose of nowayout. nowayout prevents stopping the watchdog after it has been started. It is not expected to start the watchdog on boot. > + > + outb(regval, WD_ENABLE_IOADR); > + > + return alarm_active; > +} > + > +static int simatic_ipc_wdt_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + int rc = 0; > + struct simatic_ipc_platform *plat = pdev->dev.platform_data; > + struct resource *res; > + Is it guaranteed that the device will always be instantiated only once ? If so, how it it guaranteed ? Because if there are ever multiple instances the various static variables will cause major trouble (which is why it is always better to not use static variables). > + pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n", > + __func__, __LINE__, plat->devmode); > + This is a platform device. Please use dev_ messages (dev_warn, dev_dbg etc) throughout. > + switch (plat->devmode) { > + case SIMATIC_IPC_DEVICE_227E: > + if (!devm_request_region(dev, gp_status_reg_227e_res.start, > + resource_size(&gp_status_reg_227e_res), > + KBUILD_MODNAME)) { > + dev_err(dev, > + "Unable to register IO resource at %pR\n", > + &gp_status_reg_227e_res); > + return -EBUSY; > + } > + fallthrough; > + case SIMATIC_IPC_DEVICE_427E: > + wdd.info = &wdt_ident; > + wdd.ops = &wdt_ops; > + wdd.min_timeout = TIMEOUT_MIN; > + wdd.max_timeout = TIMEOUT_MAX; Why not use static initialization ? > + wdd.parent = NULL; parent should be the platform device. > + break; > + default: > + return -EINVAL; > + } > + > + if (!devm_request_region(dev, io_resource.start, > + resource_size(&io_resource), > + io_resource.name)) { > + dev_err(dev, > + "Unable to register IO resource at %#x\n", > + WD_ENABLE_IOADR); > + return -EBUSY; > + } If this is what prevents multiple registrations, it is too late: wdd is already overwritten. > + > + if (plat->devmode == SIMATIC_IPC_DEVICE_427E) { > + res = &mem_resource; > + > + /* get GPIO base from PCI */ > + res->start = simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1)); > + if (res->start == 0) > + return -ENODEV; > + > + /* do the final address calculation */ > + res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) + > + PAD_CFG_DW0_GPP_A_23; > + res->end += res->start; > + > + wd_reset_base_addr = devm_ioremap_resource(dev, res); > + if (IS_ERR(wd_reset_base_addr)) > + return -ENOMEM; return PTR_ERR(wd_reset_base_addr); > + } > + > + wdd.bootstatus = wd_setup(plat->devmode, true); bootstatus does not report a boolean. This translates to WDIOF_OVERHEAT which is almost certainly wrong. > + if (wdd.bootstatus) > + pr_warn(KBUILD_MODNAME ": last reboot caused by watchdog reset\n"); Why two messages ? > + > + watchdog_set_nowayout(&wdd, nowayout); > + watchdog_stop_on_reboot(&wdd); > + > + rc = devm_watchdog_register_device(dev, &wdd); > + Extra empty line not needed > + if (rc == 0) > + pr_debug("initialized. nowayout=%d\n", > + nowayout); What is the value of this message (especially since there is no message if there is an error) ? > + > + return rc; > +} > + > +static int simatic_ipc_wdt_remove(struct platform_device *pdev) > +{ > + struct simatic_ipc_platform *plat = pdev->dev.platform_data; > + > + wd_setup(plat->devmode, false); This warrants an explanation. What is the point of updating the timeout here ? And what does SAFE_EN actually do ? The watchdog is stopped on reboot, but this function won't be called in that case, making this call even more questionable. Please document what it does and why it is needed here (but not when rebooting). > + return 0; > +} > + > +static struct platform_driver wdt_driver = { > + .probe = simatic_ipc_wdt_probe, > + .remove = simatic_ipc_wdt_remove, > + .driver = { > + .name = KBUILD_MODNAME, > + }, > +}; > + > +static int __init simatic_ipc_wdt_init(void) > +{ > + return platform_driver_register(&wdt_driver); > +} > + > +static void __exit simatic_ipc_wdt_exit(void) > +{ > + platform_driver_unregister(&wdt_driver); > +} > + > +module_init(simatic_ipc_wdt_init); > +module_exit(simatic_ipc_wdt_exit); Why not module_platform_driver() ? > + > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>"); >
Hi, thanks for the fast and thorough review! Am Tue, 2 Mar 2021 10:38:19 -0800 schrieb Guenter Roeck <linux@roeck-us.net>: > On 3/2/21 8:33 AM, Henning Schild wrote: > > From: Henning Schild <henning.schild@siemens.com> > > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > > > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > Signed-off-by: Henning Schild <henning.schild@siemens.com> > > --- > > drivers/watchdog/Kconfig | 11 ++ > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/simatic-ipc-wdt.c | 305 > > +++++++++++++++++++++++++++++ 3 files changed, 317 insertions(+) > > create mode 100644 drivers/watchdog/simatic-ipc-wdt.c > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > index 1fe0042a48d2..948497eb4bef 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -1575,6 +1575,17 @@ config NIC7018_WDT > > To compile this driver as a module, choose M here: the > > module will be called nic7018_wdt. > > > > +config SIEMENS_SIMATIC_IPC_WDT > > + tristate "Siemens Simatic IPC Watchdog" > > + depends on SIEMENS_SIMATIC_IPC > > + select WATCHDOG_CORE > > + help > > + This driver adds support for several watchdogs found in > > Industrial > > + PCs from Siemens. > > + > > + To compile this driver as a module, choose M here: the > > module will be > > + called simatic-ipc-wdt. > > + > > # M68K Architecture > > > > config M54xx_WATCHDOG > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > index f3a6540e725e..7f5c73ec058c 100644 > > --- a/drivers/watchdog/Makefile > > +++ b/drivers/watchdog/Makefile > > @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o > > obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o > > obj-$(CONFIG_MLX_WDT) += mlx_wdt.o > > obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o > > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o > > > > # M68K Architecture > > obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o > > diff --git a/drivers/watchdog/simatic-ipc-wdt.c > > b/drivers/watchdog/simatic-ipc-wdt.c new file mode 100644 > > index 000000000000..b5c8b7ceb404 > > --- /dev/null > > +++ b/drivers/watchdog/simatic-ipc-wdt.c > > @@ -0,0 +1,305 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Siemens SIMATIC IPC driver for Watchdogs > > + * > > + * Copyright (c) Siemens AG, 2020-2021 > > + * > > + * Authors: > > + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > + * > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > Covered by SPDX-License-Identifier > > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/errno.h> > > +#include <linux/watchdog.h> > > +#include <linux/ioport.h> > > +#include <linux/sizes.h>> +#include <linux/io.h> > > +#include <linux/platform_data/x86/simatic-ipc-base.h> > > Alphabetic order please > > > + > > +#define WD_ENABLE_IOADR 0x62 > > +#define WD_TRIGGER_IOADR 0x66 > > +#define GPIO_COMMUNITY0_PORT_ID 0xaf > > +#define PAD_CFG_DW0_GPP_A_23 0x4b8 > > Please increase indentation and spare another tab > > > +#define SAFE_EN_N_427E 0x01 > > +#define SAFE_EN_N_227E 0x04 > > +#define WD_ENABLED 0x01 > > + > > +#define TIMEOUT_MIN 2 > > +#define TIMEOUT_DEF 64 > > +#define TIMEOUT_MAX 64 > > + > > +#define GP_STATUS_REG_227E 0x404D /* IO PORT for > > SAFE_EN_N on 227E */ + > > +static bool nowayout = WATCHDOG_NOWAYOUT; > > +module_param(nowayout, bool, 0000); > > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once > > started (default=" > > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > > + > > +static DEFINE_SPINLOCK(io_lock); /* the lock for io > > operations */ +static struct watchdog_device wdd; > > + > > Having two variables named 'wdd' is confusing. Please chose another > name. > > > +static struct resource gp_status_reg_227e_res = > > + DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, > > KBUILD_MODNAME); + > > +static struct resource io_resource = > > + DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1, > > + KBUILD_MODNAME " WD_ENABLE_IOADR"); > > + > > +/* the actual start will be discovered with pci, 0 is a > > placeholder */ +static struct resource mem_resource = > > + DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR"); > > + > > +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 }; > > +static void __iomem *wd_reset_base_addr; > > + > > +static int get_timeout_idx(u32 timeout) > > +{ > > + int i; > > + > > + i = ARRAY_SIZE(wd_timeout_table) - 1; > > + for (; i >= 0; i--) { > > + if (timeout >= wd_timeout_table[i]) > > + break; > > + } > > + > > + return i; > > +} > > Please add a comment explaining why you don't use find_closest(). Will not be a comment but we will switch to using this, thanks for pointing it out. > > + > > +static int wd_start(struct watchdog_device *wdd) > > +{ > > + u8 regval; > > + > > + spin_lock(&io_lock); > > + > The watchdog subsystem already provides locking > since the watchdog device can only be opened once. > > Why is the additional lock needed ? We had this under internal review and somehow came to the conclusion that we "might" need it. I think we will remove it or come back with reasons. > > + regval = inb(WD_ENABLE_IOADR); > > + regval |= WD_ENABLED; > > + outb(regval, WD_ENABLE_IOADR); > > + > > + spin_unlock(&io_lock); > > + > > + return 0; > > +} > > + > > +static int wd_stop(struct watchdog_device *wdd) > > +{ > > + u8 regval; > > + > > + spin_lock(&io_lock); > > + > > + regval = inb(WD_ENABLE_IOADR); > > + regval &= ~WD_ENABLED; > > + outb(regval, WD_ENABLE_IOADR); > > + > > + spin_unlock(&io_lock); > > + > > + return 0; > > +} > > + > > +static int wd_ping(struct watchdog_device *wdd) > > +{ > > + inb(WD_TRIGGER_IOADR); > > + return 0; > > +} > > + > > +static int wd_set_timeout(struct watchdog_device *wdd, unsigned > > int t) +{ > > + u8 regval; > > + int timeout_idx = get_timeout_idx(t); > > + > > + spin_lock(&io_lock); > > + > > + regval = inb(WD_ENABLE_IOADR) & 0xc7; > > + regval |= timeout_idx << 3; > > + outb(regval, WD_ENABLE_IOADR); > > + > > + spin_unlock(&io_lock); > > + wdd->timeout = wd_timeout_table[timeout_idx]; > > + > > + return 0; > > +} > > + > > +static const struct watchdog_info wdt_ident = { > > + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | > > + WDIOF_SETTIMEOUT, > > + .identity = KBUILD_MODNAME, > > +}; > > + > > +static const struct watchdog_ops wdt_ops = { > > + .owner = THIS_MODULE, > > + .start = wd_start, > > + .stop = wd_stop, > > + .ping = wd_ping, > > + .set_timeout = wd_set_timeout, > > +}; > > + > > +static void wd_set_safe_en_n(u32 wdtmode, bool safe_en_n) > > +{ > > + u16 resetbit; > > + > > + if (wdtmode == SIMATIC_IPC_DEVICE_227E) { > > + /* enable SAFE_EN_N on GP_STATUS_REG_227E */ > > + resetbit = inw(GP_STATUS_REG_227E); > > + if (safe_en_n) > > + resetbit &= ~SAFE_EN_N_227E; > > + else > > + resetbit |= SAFE_EN_N_227E; > > + > > + outw(resetbit, GP_STATUS_REG_227E); > > + } else { > > + /* enable SAFE_EN_N on PCH D1600 */ > > + resetbit = ioread16(wd_reset_base_addr); > > + > > + if (safe_en_n) > > + resetbit &= ~SAFE_EN_N_427E; > > + else > > + resetbit |= SAFE_EN_N_427E; > > + > > + iowrite16(resetbit, wd_reset_base_addr); > > + } > > +} > > + > > +static int wd_setup(u32 wdtmode, bool safe_en_n) > > +{ > > + u8 regval; > > + int timeout_idx = 0; > > Unnecessary initialization > > > + bool alarm_active; > > + > > + timeout_idx = get_timeout_idx(TIMEOUT_DEF); > > + > > + wd_set_safe_en_n(wdtmode, safe_en_n); > > + > > + /* read wd register to determine alarm state */ > > + regval = inb(WD_ENABLE_IOADR); > > + if (regval & 0x80) { > > + pr_warn("Watchdog alarm active.\n"); > > Why does that warrant a warning, and what does it mean ? The context > suggests that it means the previous reset was caused by the watchdog, > but that is not what the message says. > > > + regval = 0x82; /* use only macro mode, > > reset alarm bit */ > > + alarm_active = true; > > + } else { > > + regval = 0x02; /* use only macro mode */ > > + alarm_active = false; > > + } > > Would it hurt to just always write 0x82 ? > alarm_active = regval & 0x80; > regval = 0x82 | timeout_idx << 3; > > would be much simpler. Or, if you prefer, > alarm_active = !!(regval & 0x80); > regval = 0x82 | timeout_idx << 3; > > Actually, regval isn't even needed in that case. > alarm_active = !!(regval & 0x80); > outb(0x82 | timeout_idx << 3, WD_ENABLE_IOADR); > > > Either case, please use defines for the bits. WD_ENABLED is already > defined, thus the other bits should be set using defines as well. > > > + > > + regval |= timeout_idx << 3; > > + if (nowayout) > > + regval |= WD_ENABLED; > > This is not the purpose of nowayout. nowayout prevents stopping > the watchdog after it has been started. It is not expected to start > the watchdog on boot. Thanks, that was misunderstood by the author, will fix. > > + > > + outb(regval, WD_ENABLE_IOADR); > > + > > + return alarm_active; > > +} > > + > > +static int simatic_ipc_wdt_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + int rc = 0; > > + struct simatic_ipc_platform *plat = > > pdev->dev.platform_data; > > + struct resource *res; > > + > > Is it guaranteed that the device will always be instantiated only > once ? If so, how it it guaranteed ? I suppose if anything did register two platform devices on the platform bus this might not be guaranteed. The assumption is that simatic-ipc will only ever register one, and the machines always have 0-1 "Siemens watchdogs" so at the moment there will never be a need for more than one. > Because if there are ever multiple instances the various static > variables will cause major trouble (which is why it is always better > to not use static variables). > > > + pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n", > > + __func__, __LINE__, plat->devmode); > > + > > This is a platform device. Please use dev_ messages (dev_warn, > dev_dbg etc) throughout. > > > + switch (plat->devmode) { > > + case SIMATIC_IPC_DEVICE_227E: > > + if (!devm_request_region(dev, > > gp_status_reg_227e_res.start, > > + > > resource_size(&gp_status_reg_227e_res), > > + KBUILD_MODNAME)) { > > + dev_err(dev, > > + "Unable to register IO resource at > > %pR\n", > > + &gp_status_reg_227e_res); > > + return -EBUSY; > > + } > > + fallthrough; > > + case SIMATIC_IPC_DEVICE_427E: > > + wdd.info = &wdt_ident; > > + wdd.ops = &wdt_ops; > > + wdd.min_timeout = TIMEOUT_MIN; > > + wdd.max_timeout = TIMEOUT_MAX; > > Why not use static initialization ? > > > + wdd.parent = NULL; > > parent should be the platform device. > > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + if (!devm_request_region(dev, io_resource.start, > > + resource_size(&io_resource), > > + io_resource.name)) { > > + dev_err(dev, > > + "Unable to register IO resource at %#x\n", > > + WD_ENABLE_IOADR); > > + return -EBUSY; > > + } > > If this is what prevents multiple registrations, it is too late: wdd > is already overwritten. As said, there will be no double-registration. > > + > > + if (plat->devmode == SIMATIC_IPC_DEVICE_427E) { > > + res = &mem_resource; > > + > > + /* get GPIO base from PCI */ > > + res->start = > > simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1)); > > + if (res->start == 0) > > + return -ENODEV; > > + > > + /* do the final address calculation */ > > + res->start = res->start + (GPIO_COMMUNITY0_PORT_ID > > << 16) + > > + PAD_CFG_DW0_GPP_A_23; > > + res->end += res->start; > > + > > + wd_reset_base_addr = devm_ioremap_resource(dev, > > res); > > + if (IS_ERR(wd_reset_base_addr)) > > + return -ENOMEM; > > return PTR_ERR(wd_reset_base_addr); > > > + } > > + > > + wdd.bootstatus = wd_setup(plat->devmode, true); > > bootstatus does not report a boolean. This translates to > WDIOF_OVERHEAT which is almost certainly wrong. > > > + if (wdd.bootstatus) > > + pr_warn(KBUILD_MODNAME ": last reboot caused by > > watchdog reset\n"); > > Why two messages ? > > > + > > + watchdog_set_nowayout(&wdd, nowayout); > > + watchdog_stop_on_reboot(&wdd); > > + > > + rc = devm_watchdog_register_device(dev, &wdd); > > + > Extra empty line not needed > > > + if (rc == 0) > > + pr_debug("initialized. nowayout=%d\n", > > + nowayout); > > What is the value of this message (especially since there is no > message if there is an error) ? > > > + > > + return rc; > > +} > > + > > +static int simatic_ipc_wdt_remove(struct platform_device *pdev) > > +{ > > + struct simatic_ipc_platform *plat = > > pdev->dev.platform_data; + > > + wd_setup(plat->devmode, false); > > This warrants an explanation. What is the point of updating the > timeout here ? And what does SAFE_EN actually do ? The idea was that module unloading should disable the watchdog, but that code will be removed and aligned with other watchdogs. SAFE_EN is a second enable bit, if it is not set the watchdog will fire into the void. Pretty pointless will keep that always armed or set it always when toggling "enable". All the other open points are pretty clear, and will all be dealt with in v2. regards, Henning > The watchdog is stopped on reboot, but this function won't > be called in that case, making this call even more questionable. > Please document what it does and why it is needed here (but not > when rebooting). > > > + return 0; > > +} > > + > > +static struct platform_driver wdt_driver = { > > + .probe = simatic_ipc_wdt_probe, > > + .remove = simatic_ipc_wdt_remove, > > + .driver = { > > + .name = KBUILD_MODNAME, > > + }, > > +}; > > + > > +static int __init simatic_ipc_wdt_init(void) > > +{ > > + return platform_driver_register(&wdt_driver); > > +} > > + > > +static void __exit simatic_ipc_wdt_exit(void) > > +{ > > + platform_driver_unregister(&wdt_driver); > > +} > > + > > +module_init(simatic_ipc_wdt_init); > > +module_exit(simatic_ipc_wdt_exit); > > Why not module_platform_driver() ? > > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>"); > > >
On 03.03.21 15:21, Henning Schild wrote: > Hi, > > thanks for the fast and thorough review! > > Am Tue, 2 Mar 2021 10:38:19 -0800 > schrieb Guenter Roeck <linux@roeck-us.net>: > >> On 3/2/21 8:33 AM, Henning Schild wrote: >>> From: Henning Schild <henning.schild@siemens.com> >>> >>> This driver adds initial support for several devices from Siemens. >>> It is based on a platform driver introduced in an earlier commit. >>> >>> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> >>> Signed-off-by: Henning Schild <henning.schild@siemens.com> >>> --- >>> drivers/watchdog/Kconfig | 11 ++ >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/simatic-ipc-wdt.c | 305 >>> +++++++++++++++++++++++++++++ 3 files changed, 317 insertions(+) >>> create mode 100644 drivers/watchdog/simatic-ipc-wdt.c >>> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>> index 1fe0042a48d2..948497eb4bef 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -1575,6 +1575,17 @@ config NIC7018_WDT >>> To compile this driver as a module, choose M here: the >>> module will be called nic7018_wdt. >>> >>> +config SIEMENS_SIMATIC_IPC_WDT >>> + tristate "Siemens Simatic IPC Watchdog" >>> + depends on SIEMENS_SIMATIC_IPC >>> + select WATCHDOG_CORE >>> + help >>> + This driver adds support for several watchdogs found in >>> Industrial >>> + PCs from Siemens. >>> + >>> + To compile this driver as a module, choose M here: the >>> module will be >>> + called simatic-ipc-wdt. >>> + >>> # M68K Architecture >>> >>> config M54xx_WATCHDOG >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>> index f3a6540e725e..7f5c73ec058c 100644 >>> --- a/drivers/watchdog/Makefile >>> +++ b/drivers/watchdog/Makefile >>> @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o >>> obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o >>> obj-$(CONFIG_MLX_WDT) += mlx_wdt.o >>> obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o >>> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o >>> >>> # M68K Architecture >>> obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o >>> diff --git a/drivers/watchdog/simatic-ipc-wdt.c >>> b/drivers/watchdog/simatic-ipc-wdt.c new file mode 100644 >>> index 000000000000..b5c8b7ceb404 >>> --- /dev/null >>> +++ b/drivers/watchdog/simatic-ipc-wdt.c >>> @@ -0,0 +1,305 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Siemens SIMATIC IPC driver for Watchdogs >>> + * >>> + * Copyright (c) Siemens AG, 2020-2021 >>> + * >>> + * Authors: >>> + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> >>> + * >>> + * This program is free software; you can redistribute it and/or >>> modify >>> + * it under the terms of the GNU General Public License version 2 >>> as >>> + * published by the Free Software Foundation. >> >> Covered by SPDX-License-Identifier >> >>> + */ >>> + >>> +#include <linux/device.h> >>> +#include <linux/init.h> >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/errno.h> >>> +#include <linux/watchdog.h> >>> +#include <linux/ioport.h> >>> +#include <linux/sizes.h>> +#include <linux/io.h> >>> +#include <linux/platform_data/x86/simatic-ipc-base.h> >> >> Alphabetic order please >> >>> + >>> +#define WD_ENABLE_IOADR 0x62 >>> +#define WD_TRIGGER_IOADR 0x66 >>> +#define GPIO_COMMUNITY0_PORT_ID 0xaf >>> +#define PAD_CFG_DW0_GPP_A_23 0x4b8 >> >> Please increase indentation and spare another tab >> >>> +#define SAFE_EN_N_427E 0x01 >>> +#define SAFE_EN_N_227E 0x04 >>> +#define WD_ENABLED 0x01 >>> + >>> +#define TIMEOUT_MIN 2 >>> +#define TIMEOUT_DEF 64 >>> +#define TIMEOUT_MAX 64 >>> + >>> +#define GP_STATUS_REG_227E 0x404D /* IO PORT for >>> SAFE_EN_N on 227E */ + >>> +static bool nowayout = WATCHDOG_NOWAYOUT; >>> +module_param(nowayout, bool, 0000); >>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once >>> started (default=" >>> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); >>> + >>> +static DEFINE_SPINLOCK(io_lock); /* the lock for io >>> operations */ +static struct watchdog_device wdd; >>> + >> >> Having two variables named 'wdd' is confusing. Please chose another >> name. >> >>> +static struct resource gp_status_reg_227e_res = >>> + DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, >>> KBUILD_MODNAME); + >>> +static struct resource io_resource = >>> + DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1, >>> + KBUILD_MODNAME " WD_ENABLE_IOADR"); >>> + >>> +/* the actual start will be discovered with pci, 0 is a >>> placeholder */ +static struct resource mem_resource = >>> + DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR"); >>> + >>> +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 }; >>> +static void __iomem *wd_reset_base_addr; >>> + >>> +static int get_timeout_idx(u32 timeout) >>> +{ >>> + int i; >>> + >>> + i = ARRAY_SIZE(wd_timeout_table) - 1; >>> + for (; i >= 0; i--) { >>> + if (timeout >= wd_timeout_table[i]) >>> + break; >>> + } >>> + >>> + return i; >>> +} >> >> Please add a comment explaining why you don't use find_closest(). > > Will not be a comment but we will switch to using this, thanks for > pointing it out. > >>> + >>> +static int wd_start(struct watchdog_device *wdd) >>> +{ >>> + u8 regval; >>> + >>> + spin_lock(&io_lock); >>> + >> The watchdog subsystem already provides locking >> since the watchdog device can only be opened once. >> >> Why is the additional lock needed ? > > We had this under internal review and somehow came to the conclusion > that we "might" need it. I think we will remove it or come back with > reasons. Probably my fault. I quickly scanned for locking usage in other watchdog drivers and found plenty of those RMW-protecting locks. I didn't check then what the actual locking model of the watchdog core is, and possibly some of the other use cases need to protect register against other users or multiple watchdog instances. But if the core locks start/stop/set_timeout internally against each other, this is obviously pointless without out inherently single-instance use case here. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
Am Thu, 4 Mar 2021 10:27:07 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Tuesday, March 2, 2021, Henning Schild <henning.schild@siemens.com> > wrote: > > > From: Henning Schild <henning.schild@siemens.com> > > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > > > > > Is it ACPI based? Why it can not provide WDAT table instead? Yes it is. We strongly urged the BIOS guys to look into that for future products and BIOS updates for current products. But for existing products they will not add "features". One fear is breaking Windows where custom drivers have been the way they did it so far. So we are dealing with legacy here, that is why. Hope that is not a show-stopper. Maybe we can carry ACPI quirk snippets instead, did not yet look into this. And i am not sure which version would be easier to maintain and more acceptable to the kernel. regards, Henning > > > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > Signed-off-by: Henning Schild <henning.schild@siemens.com> > > --- > > drivers/watchdog/Kconfig | 11 ++ > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/simatic-ipc-wdt.c | 305 > > +++++++++++++++++++++++++++++ 3 files changed, 317 insertions(+) > > create mode 100644 drivers/watchdog/simatic-ipc-wdt.c > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > index 1fe0042a48d2..948497eb4bef 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -1575,6 +1575,17 @@ config NIC7018_WDT > > To compile this driver as a module, choose M here: the > > module will be > > called nic7018_wdt. > > > > +config SIEMENS_SIMATIC_IPC_WDT > > + tristate "Siemens Simatic IPC Watchdog" > > + depends on SIEMENS_SIMATIC_IPC > > + select WATCHDOG_CORE > > + help > > + This driver adds support for several watchdogs found in > > Industrial > > + PCs from Siemens. > > + > > + To compile this driver as a module, choose M here: the > > module will be > > + called simatic-ipc-wdt. > > + > > # M68K Architecture > > > > config M54xx_WATCHDOG > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > index f3a6540e725e..7f5c73ec058c 100644 > > --- a/drivers/watchdog/Makefile > > +++ b/drivers/watchdog/Makefile > > @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o > > obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o > > obj-$(CONFIG_MLX_WDT) += mlx_wdt.o > > obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o > > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o > > > > # M68K Architecture > > obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o > > diff --git a/drivers/watchdog/simatic-ipc-wdt.c > > b/drivers/watchdog/simatic-ipc-wdt.c > > new file mode 100644 > > index 000000000000..b5c8b7ceb404 > > --- /dev/null > > +++ b/drivers/watchdog/simatic-ipc-wdt.c > > @@ -0,0 +1,305 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Siemens SIMATIC IPC driver for Watchdogs > > + * > > + * Copyright (c) Siemens AG, 2020-2021 > > + * > > + * Authors: > > + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > + * > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/errno.h> > > +#include <linux/watchdog.h> > > +#include <linux/ioport.h> > > +#include <linux/sizes.h> > > +#include <linux/io.h> > > +#include <linux/platform_data/x86/simatic-ipc-base.h> > > + > > +#define WD_ENABLE_IOADR 0x62 > > +#define WD_TRIGGER_IOADR 0x66 > > +#define GPIO_COMMUNITY0_PORT_ID 0xaf > > +#define PAD_CFG_DW0_GPP_A_23 0x4b8 > > +#define SAFE_EN_N_427E 0x01 > > +#define SAFE_EN_N_227E 0x04 > > +#define WD_ENABLED 0x01 > > + > > +#define TIMEOUT_MIN 2 > > +#define TIMEOUT_DEF 64 > > +#define TIMEOUT_MAX 64 > > + > > +#define GP_STATUS_REG_227E 0x404D /* IO PORT for SAFE_EN_N on > > 227E */ + > > +static bool nowayout = WATCHDOG_NOWAYOUT; > > +module_param(nowayout, bool, 0000); > > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started > > (default=" > > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > > + > > +static DEFINE_SPINLOCK(io_lock); /* the lock for io > > operations */ +static struct watchdog_device wdd; > > + > > +static struct resource gp_status_reg_227e_res = > > + DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, > > KBUILD_MODNAME); + > > +static struct resource io_resource = > > + DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1, > > + KBUILD_MODNAME " WD_ENABLE_IOADR"); > > + > > +/* the actual start will be discovered with pci, 0 is a > > placeholder */ +static struct resource mem_resource = > > + DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR"); > > + > > +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 }; > > +static void __iomem *wd_reset_base_addr; > > + > > +static int get_timeout_idx(u32 timeout) > > +{ > > + int i; > > + > > + i = ARRAY_SIZE(wd_timeout_table) - 1; > > + for (; i >= 0; i--) { > > + if (timeout >= wd_timeout_table[i]) > > + break; > > + } > > + > > + return i; > > +} > > + > > +static int wd_start(struct watchdog_device *wdd) > > +{ > > + u8 regval; > > + > > + spin_lock(&io_lock); > > + > > + regval = inb(WD_ENABLE_IOADR); > > + regval |= WD_ENABLED; > > + outb(regval, WD_ENABLE_IOADR); > > + > > + spin_unlock(&io_lock); > > + > > + return 0; > > +} > > + > > +static int wd_stop(struct watchdog_device *wdd) > > +{ > > + u8 regval; > > + > > + spin_lock(&io_lock); > > + > > + regval = inb(WD_ENABLE_IOADR); > > + regval &= ~WD_ENABLED; > > + outb(regval, WD_ENABLE_IOADR); > > + > > + spin_unlock(&io_lock); > > + > > + return 0; > > +} > > + > > +static int wd_ping(struct watchdog_device *wdd) > > +{ > > + inb(WD_TRIGGER_IOADR); > > + return 0; > > +} > > + > > +static int wd_set_timeout(struct watchdog_device *wdd, unsigned > > int t) +{ > > + u8 regval; > > + int timeout_idx = get_timeout_idx(t); > > + > > + spin_lock(&io_lock); > > + > > + regval = inb(WD_ENABLE_IOADR) & 0xc7; > > + regval |= timeout_idx << 3; > > + outb(regval, WD_ENABLE_IOADR); > > + > > + spin_unlock(&io_lock); > > + wdd->timeout = wd_timeout_table[timeout_idx]; > > + > > + return 0; > > +} > > + > > +static const struct watchdog_info wdt_ident = { > > + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | > > + WDIOF_SETTIMEOUT, > > + .identity = KBUILD_MODNAME, > > +}; > > + > > +static const struct watchdog_ops wdt_ops = { > > + .owner = THIS_MODULE, > > + .start = wd_start, > > + .stop = wd_stop, > > + .ping = wd_ping, > > + .set_timeout = wd_set_timeout, > > +}; > > + > > +static void wd_set_safe_en_n(u32 wdtmode, bool safe_en_n) > > +{ > > + u16 resetbit; > > + > > + if (wdtmode == SIMATIC_IPC_DEVICE_227E) { > > + /* enable SAFE_EN_N on GP_STATUS_REG_227E */ > > + resetbit = inw(GP_STATUS_REG_227E); > > + if (safe_en_n) > > + resetbit &= ~SAFE_EN_N_227E; > > + else > > + resetbit |= SAFE_EN_N_227E; > > + > > + outw(resetbit, GP_STATUS_REG_227E); > > + } else { > > + /* enable SAFE_EN_N on PCH D1600 */ > > + resetbit = ioread16(wd_reset_base_addr); > > + > > + if (safe_en_n) > > + resetbit &= ~SAFE_EN_N_427E; > > + else > > + resetbit |= SAFE_EN_N_427E; > > + > > + iowrite16(resetbit, wd_reset_base_addr); > > + } > > +} > > + > > +static int wd_setup(u32 wdtmode, bool safe_en_n) > > +{ > > + u8 regval; > > + int timeout_idx = 0; > > + bool alarm_active; > > + > > + timeout_idx = get_timeout_idx(TIMEOUT_DEF); > > + > > + wd_set_safe_en_n(wdtmode, safe_en_n); > > + > > + /* read wd register to determine alarm state */ > > + regval = inb(WD_ENABLE_IOADR); > > + if (regval & 0x80) { > > + pr_warn("Watchdog alarm active.\n"); > > + regval = 0x82; /* use only macro mode, reset alarm > > bit */ > > + alarm_active = true; > > + } else { > > + regval = 0x02; /* use only macro mode */ > > + alarm_active = false; > > + } > > + > > + regval |= timeout_idx << 3; > > + if (nowayout) > > + regval |= WD_ENABLED; > > + > > + outb(regval, WD_ENABLE_IOADR); > > + > > + return alarm_active; > > +} > > + > > +static int simatic_ipc_wdt_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + int rc = 0; > > + struct simatic_ipc_platform *plat = pdev->dev.platform_data; > > + struct resource *res; > > + > > + pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n", > > + __func__, __LINE__, plat->devmode); > > + > > + switch (plat->devmode) { > > + case SIMATIC_IPC_DEVICE_227E: > > + if (!devm_request_region(dev, > > gp_status_reg_227e_res.start, > > + > > resource_size(&gp_status_reg_ 227e_res), > > + KBUILD_MODNAME)) { > > + dev_err(dev, > > + "Unable to register IO resource at > > %pR\n", > > + &gp_status_reg_227e_res); > > + return -EBUSY; > > + } > > + fallthrough; > > + case SIMATIC_IPC_DEVICE_427E: > > + wdd.info = &wdt_ident; > > + wdd.ops = &wdt_ops; > > + wdd.min_timeout = TIMEOUT_MIN; > > + wdd.max_timeout = TIMEOUT_MAX; > > + wdd.parent = NULL; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + if (!devm_request_region(dev, io_resource.start, > > + resource_size(&io_resource), > > + io_resource.name)) { > > + dev_err(dev, > > + "Unable to register IO resource at %#x\n", > > + WD_ENABLE_IOADR); > > + return -EBUSY; > > + } > > + > > + if (plat->devmode == SIMATIC_IPC_DEVICE_427E) { > > + res = &mem_resource; > > + > > + /* get GPIO base from PCI */ > > + res->start = > > simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1)); > > + if (res->start == 0) > > + return -ENODEV; > > + > > + /* do the final address calculation */ > > + res->start = res->start + (GPIO_COMMUNITY0_PORT_ID > > << 16) + > > + PAD_CFG_DW0_GPP_A_23; > > + res->end += res->start; > > + > > + wd_reset_base_addr = devm_ioremap_resource(dev, > > res); > > + if (IS_ERR(wd_reset_base_addr)) > > + return -ENOMEM; > > + } > > + > > + wdd.bootstatus = wd_setup(plat->devmode, true); > > + if (wdd.bootstatus) > > + pr_warn(KBUILD_MODNAME ": last reboot caused by > > watchdog reset\n"); > > + > > + watchdog_set_nowayout(&wdd, nowayout); > > + watchdog_stop_on_reboot(&wdd); > > + > > + rc = devm_watchdog_register_device(dev, &wdd); > > + > > + if (rc == 0) > > + pr_debug("initialized. nowayout=%d\n", > > + nowayout); > > + > > + return rc; > > +} > > + > > +static int simatic_ipc_wdt_remove(struct platform_device *pdev) > > +{ > > + struct simatic_ipc_platform *plat = pdev->dev.platform_data; > > + > > + wd_setup(plat->devmode, false); > > + return 0; > > +} > > + > > +static struct platform_driver wdt_driver = { > > + .probe = simatic_ipc_wdt_probe, > > + .remove = simatic_ipc_wdt_remove, > > + .driver = { > > + .name = KBUILD_MODNAME, > > + }, > > +}; > > + > > +static int __init simatic_ipc_wdt_init(void) > > +{ > > + return platform_driver_register(&wdt_driver); > > +} > > + > > +static void __exit simatic_ipc_wdt_exit(void) > > +{ > > + platform_driver_unregister(&wdt_driver); > > +} > > + > > +module_init(simatic_ipc_wdt_init); > > +module_exit(simatic_ipc_wdt_exit); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>"); > > -- > > 2.26.2 > > > > >
On Thu, Mar 4, 2021 at 8:36 AM Henning Schild <henning.schild@siemens.com> wrote: > > From: Henning Schild <henning.schild@siemens.com> > > This mainly implements detection of these devices and will allow > secondary drivers to work on such machines. > > The identification is DMI-based with a vendor specific way to tell them > apart in a reliable way. > > Drivers for LEDs and Watchdogs will follow to make use of that platform > detection. > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > Signed-off-by: Henning Schild <henning.schild@siemens.com> The order is wrong taking into account the From line in the body. So, it's not clear who is the author, who is a co-developer, and who is the committer (one person may utilize few roles). Check for the rest of the series as well (basically this is the rule of thumb to recheck entire code for the comment you have got at any single place of it). ... > +config SIEMENS_SIMATIC_IPC > + tristate "Siemens Simatic IPC Class driver" > + depends on PCI > + help > + This Simatic IPC class driver is the central of several drivers. It > + is mainly used for system identification, after which drivers in other > + classes will take care of driving specifics of those machines. > + i.e. leds and watchdogs LEDs watchdog. (missed period and singular form) Module name? > endif # X86_PLATFORM_DEVICES Not sure about the ordering of the section in Kconfig, but it is up to maintainers. ... > +# Siemens Simatic Industrial PCs > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o Ditto. ... > + * Siemens SIMATIC IPC driver for LEDs It seems to be used in patch 4 which is not LED related. Also, why is it here if it's for LEDs? ... > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. Replace with SPDX ... > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/platform_device.h> Ordered? > +#include <linux/platform_data/x86/simatic-ipc.h> ... > +static int register_platform_devices(u32 station_id) > +{ > + int i; > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; Reversed xmas tree order? > + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE; > + > + for (i = 0; i < ARRAY_SIZE(device_modes); i++) { > + if (device_modes[i].station_id == station_id) { > + ledmode = device_modes[i].led_mode; > + wdtmode = device_modes[i].wdt_mode; > + break; > + } > + } > + > + if (ledmode != SIMATIC_IPC_DEVICE_NONE) { > + platform_data.devmode = ledmode; > + ipc_led_platform_device = platform_device_register_data > + (NULL, KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE, > + &platform_data, sizeof(struct simatic_ipc_platform)); Strange indentation (second line). > + if (IS_ERR(ipc_led_platform_device)) > + return PTR_ERR(ipc_led_platform_device); > + pr_debug(KBUILD_MODNAME ": device=%s created\n", > + ipc_led_platform_device->name); Utilize pr_fmt() instead of adding prefixes like this. > + } > + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) { > + platform_data.devmode = wdtmode; > + ipc_wdt_platform_device = platform_device_register_data > + (NULL, KBUILD_MODNAME "_wdt", PLATFORM_DEVID_NONE, > + &platform_data, sizeof(struct simatic_ipc_platform)); > + if (IS_ERR(ipc_wdt_platform_device)) > + return PTR_ERR(ipc_wdt_platform_device); > + > + pr_debug(KBUILD_MODNAME ": device=%s created\n", > + ipc_wdt_platform_device->name); > + } Same comments as above. > + if (ledmode == SIMATIC_IPC_DEVICE_NONE && > + wdtmode == SIMATIC_IPC_DEVICE_NONE) { > + pr_warn(KBUILD_MODNAME > + ": unsupported IPC detected, station id=%08x\n", > + station_id); Ugly indentation. With pr_fmt() in use will be much better. > + return -EINVAL; > + } else { Redundant. > + return 0; > + } > +} ... > +/* > + * Get membase address from PCI, used in leds and wdt modul. Here we read > + * the bar0. The final address calculation is done in the appropriate modules bar -> BAR Missed period. > + */ > + Unneeded blank line. > +u32 simatic_ipc_get_membase0(unsigned int p2sb) > +{ > + u32 bar0 = 0; > +#ifdef CONFIG_PCI It's ugly besides the fact that you have a dependency. > + struct pci_bus *bus; Missed blank line. > + /* > + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device > + * to have a quick look at it, before we hide it again. > + * Also grab the pci rescan lock so that device does not get discovered > + * and remapped while it is visible. > + * This code is inspired by drivers/mfd/lpc_ich.c > + */ > + bus = pci_find_bus(0, 0); > + pci_lock_rescan_remove(); > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0); > + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0); > + > + bar0 &= ~0xf; > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1); > + pci_unlock_rescan_remove(); > +#endif /* CONFIG_PCI */ > + return bar0; > +} > +EXPORT_SYMBOL(simatic_ipc_get_membase0); Oy vey! I know what this is and let's do it differently. I have some (relatively old) patch series I can send you privately for testing. ... > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. Not needed when you have SPDX. ... > +#include <linux/pci.h> Wrong header. Should be types.h ... > +#include <linux/dmi.h> > +#include <linux/platform_data/x86/simatic-ipc-base.h> Missed headers. You need to include ones that the code below is a direct user of. Like types.h
On Thu, Mar 4, 2021 at 9:29 AM Henning Schild <henning.schild@siemens.com> wrote: > This series adds support for watchdogs and leds of several x86 devices > from Siemens. > > It is structured with a platform driver that mainly does identification > of the machines. It might trigger loading of the actual device drivers > by attaching devices to the platform bus. > > The identification is vendor specific, parsing a special binary DMI > entry. The implementation of that platform identification is applied on > pmc_atom clock quirks in the final patch. > > It is all structured in a way that we can easily add more devices and > more platform drivers later. Internally we have some more code for > hardware monitoring, more leds, watchdogs etc. This will follow some > day. > > But the idea here is to share early, and hopefully not fail early. I have given a few comments here and there, so please check the entire series and address them in _all_ similar locations. As I have noticed, I have different approach about P2SB code, I have to give the series a dust and see if you can utilize it. I would like to be Cc'ed on the next version. -- With Best Regards, Andy Shevchenko
On Thu, Mar 4, 2021 at 12:19 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Mar 4, 2021 at 9:29 AM Henning Schild > <henning.schild@siemens.com> wrote: > I have given a few comments here and there, so please check the entire > series and address them in _all_ similar locations. As I have noticed, > I have different approach about P2SB code, I have to give the series a > dust and see if you can utilize it. > > I would like to be Cc'ed on the next version. One more thing, is it Apollo Lake based? -- With Best Regards, Andy Shevchenko
Hi, On 3/4/21 11:11 AM, Andy Shevchenko wrote: > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > <henning.schild@siemens.com> wrote: >> >> From: Henning Schild <henning.schild@siemens.com> >> >> This mainly implements detection of these devices and will allow >> secondary drivers to work on such machines. >> >> The identification is DMI-based with a vendor specific way to tell them >> apart in a reliable way. >> >> Drivers for LEDs and Watchdogs will follow to make use of that platform >> detection. > >> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> >> Signed-off-by: Henning Schild <henning.schild@siemens.com> > > The order is wrong taking into account the From line in the body. So, > it's not clear who is the author, who is a co-developer, and who is > the committer (one person may utilize few roles). > Check for the rest of the series as well (basically this is the rule > of thumb to recheck entire code for the comment you have got at any > single place of it). > > ... > >> +config SIEMENS_SIMATIC_IPC >> + tristate "Siemens Simatic IPC Class driver" >> + depends on PCI >> + help >> + This Simatic IPC class driver is the central of several drivers. It >> + is mainly used for system identification, after which drivers in other >> + classes will take care of driving specifics of those machines. >> + i.e. leds and watchdogs > > LEDs > watchdog. (missed period and singular form) > > Module name? > >> endif # X86_PLATFORM_DEVICES > > Not sure about the ordering of the section in Kconfig, but it is up to > maintainers. > > ... > >> +# Siemens Simatic Industrial PCs >> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o > > Ditto. > > ... > >> + * Siemens SIMATIC IPC driver for LEDs > > It seems to be used in patch 4 which is not LED related. Also, why is > it here if it's for LEDs? > > ... > >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. > > Replace with SPDX > > ... > >> +#include <linux/module.h> >> +#include <linux/kernel.h> >> +#include <linux/platform_device.h> > > Ordered? > >> +#include <linux/platform_data/x86/simatic-ipc.h> > > ... > >> +static int register_platform_devices(u32 station_id) >> +{ >> + int i; >> + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; >> + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > Reversed xmas tree order? > >> + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE; >> + >> + for (i = 0; i < ARRAY_SIZE(device_modes); i++) { >> + if (device_modes[i].station_id == station_id) { >> + ledmode = device_modes[i].led_mode; >> + wdtmode = device_modes[i].wdt_mode; >> + break; >> + } >> + } >> + >> + if (ledmode != SIMATIC_IPC_DEVICE_NONE) { >> + platform_data.devmode = ledmode; > >> + ipc_led_platform_device = platform_device_register_data >> + (NULL, KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE, >> + &platform_data, sizeof(struct simatic_ipc_platform)); > > Strange indentation (second line). > >> + if (IS_ERR(ipc_led_platform_device)) >> + return PTR_ERR(ipc_led_platform_device); > >> + pr_debug(KBUILD_MODNAME ": device=%s created\n", >> + ipc_led_platform_device->name); > > Utilize pr_fmt() instead of adding prefixes like this. > >> + } > >> + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) { >> + platform_data.devmode = wdtmode; >> + ipc_wdt_platform_device = platform_device_register_data >> + (NULL, KBUILD_MODNAME "_wdt", PLATFORM_DEVID_NONE, >> + &platform_data, sizeof(struct simatic_ipc_platform)); >> + if (IS_ERR(ipc_wdt_platform_device)) >> + return PTR_ERR(ipc_wdt_platform_device); >> + >> + pr_debug(KBUILD_MODNAME ": device=%s created\n", >> + ipc_wdt_platform_device->name); >> + } > > Same comments as above. > >> + if (ledmode == SIMATIC_IPC_DEVICE_NONE && >> + wdtmode == SIMATIC_IPC_DEVICE_NONE) { >> + pr_warn(KBUILD_MODNAME >> + ": unsupported IPC detected, station id=%08x\n", >> + station_id); > > Ugly indentation. With pr_fmt() in use will be much better. > >> + return -EINVAL; > >> + } else { > > Redundant. > >> + return 0; >> + } >> +} > > ... > >> +/* >> + * Get membase address from PCI, used in leds and wdt modul. Here we read >> + * the bar0. The final address calculation is done in the appropriate modules > > bar -> BAR > Missed period. > >> + */ > >> + > > Unneeded blank line. > >> +u32 simatic_ipc_get_membase0(unsigned int p2sb) >> +{ >> + u32 bar0 = 0; > >> +#ifdef CONFIG_PCI > > It's ugly besides the fact that you have a dependency. > >> + struct pci_bus *bus; > > Missed blank line. > >> + /* >> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device >> + * to have a quick look at it, before we hide it again. >> + * Also grab the pci rescan lock so that device does not get discovered >> + * and remapped while it is visible. >> + * This code is inspired by drivers/mfd/lpc_ich.c >> + */ >> + bus = pci_find_bus(0, 0); >> + pci_lock_rescan_remove(); >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0); >> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0); >> + >> + bar0 &= ~0xf; >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1); >> + pci_unlock_rescan_remove(); >> +#endif /* CONFIG_PCI */ >> + return bar0; >> +} >> +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > Oy vey! I know what this is and let's do it differently. I have some > (relatively old) patch series I can send you privately for testing. This bit stood out the most to me too, it would be good if we can this fixed in some cleaner work. So I'm curious how things will look with Andy's work integrated. Also I don't think this should be exported. Instead this (or its replacement) should be used to get the address for an IOMEM resource to add the platform devices when they are instantiated. Then the platform-dev drivers can just use the regular functions to get their resources instead of relying on this module. Regards, Hans > > ... > >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. > > Not needed when you have SPDX. > > ... > >> +#include <linux/pci.h> > > Wrong header. Should be types.h > > ... > >> +#include <linux/dmi.h> >> +#include <linux/platform_data/x86/simatic-ipc-base.h> > > Missed headers. You need to include ones that the code below is a > direct user of. > > Like types.h >
Hi, On 3/2/21 5:33 PM, Henning Schild wrote: <snip> > +static inline u32 simatic_ipc_get_station_id(u8 *data) > +{ > + u32 station_id = SIMATIC_IPC_INVALID_STATION_ID; > + int i; > + struct { > + u8 type; /* type (0xff = binary) */ > + u8 len; /* len of data entry */ > + u8 reserved[3]; > + u32 station_id; /* station id (LE) */ > + } __packed * data_entry = (void *)data; > + > + /* find 4th entry in OEM data */ > + for (i = 0; i < 3; i++) > + data_entry = (void *)((u8 *)(data_entry) + data_entry->len); > + > + /* decode station id */ > + if (data_entry && data_entry->type == 0xff && data_entry->len == 9) > + station_id = le32_to_cpu(data_entry->station_id); > + > + return station_id; > +} > + > +static inline void > +simatic_ipc_find_dmi_entry_helper(const struct dmi_header *dh, void *_data) > +{ > + u32 *id = _data; > + > + if (dh->type != DMI_ENTRY_OEM) > + return; > + > + *id = simatic_ipc_get_station_id((u8 *)dh + sizeof(struct dmi_header)); > +} Please take dh->length into account here and make sure that you don't walk past the end of the DMI tables during the parsing here. Regards, Hans > + > +#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */ >
Thanks Andy, Am Thu, 4 Mar 2021 12:19:44 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 9:29 AM Henning Schild > <henning.schild@siemens.com> wrote: > > > This series adds support for watchdogs and leds of several x86 > > devices from Siemens. > > > > It is structured with a platform driver that mainly does > > identification of the machines. It might trigger loading of the > > actual device drivers by attaching devices to the platform bus. > > > > The identification is vendor specific, parsing a special binary DMI > > entry. The implementation of that platform identification is > > applied on pmc_atom clock quirks in the final patch. > > > > It is all structured in a way that we can easily add more devices > > and more platform drivers later. Internally we have some more code > > for hardware monitoring, more leds, watchdogs etc. This will follow > > some day. > > > > But the idea here is to share early, and hopefully not fail early. > > I have given a few comments here and there, so please check the entire > series and address them in _all_ similar locations. As I have noticed, > I have different approach about P2SB code, I have to give the series a > dust and see if you can utilize it. You did find some things that others found as well. SPDX vs blabla, header ordering, some other style. Some things are already done and will be in v2. Other findings are new, and we will look into them. The only thing that did stick out is P2SB, also was a point in internal pre-review. Let us see what you have, i can include a patch of yours into the q. But i am kind of afraid once it is there, several existing users should be touched with it, and this series would come later. Or this series comes first and you come later and clean up our "mess". Not sure i would want to take over all P2SB unhiders, but with you on board it will work. > I would like to be Cc'ed on the next version. Sure thing. regards, Henning >
Am Thu, 4 Mar 2021 12:20:22 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 12:19 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Mar 4, 2021 at 9:29 AM Henning Schild > > <henning.schild@siemens.com> wrote: > > > I have given a few comments here and there, so please check the > > entire series and address them in _all_ similar locations. As I > > have noticed, I have different approach about P2SB code, I have to > > give the series a dust and see if you can utilize it. > > > > I would like to be Cc'ed on the next version. > > One more thing, is it Apollo Lake based? Not sure i can answer that, or what you even refer to. The whole series is about a range of devices, some even have sub-models with differing CPUs and Lakes regards, Henning
Am Thu, 4 Mar 2021 12:11:12 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > <henning.schild@siemens.com> wrote: > > > > From: Henning Schild <henning.schild@siemens.com> > > > > This mainly implements detection of these devices and will allow > > secondary drivers to work on such machines. > > > > The identification is DMI-based with a vendor specific way to tell > > them apart in a reliable way. > > > > Drivers for LEDs and Watchdogs will follow to make use of that > > platform detection. > > > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > Signed-off-by: Henning Schild <henning.schild@siemens.com> > > The order is wrong taking into account the From line in the body. So, > it's not clear who is the author, who is a co-developer, and who is > the committer (one person may utilize few roles). > Check for the rest of the series as well (basically this is the rule > of thumb to recheck entire code for the comment you have got at any > single place of it). For some code Gerd is the author, and i am the co-Author. We even have Jan in the mix at places. At least in copyright headers. I will remain the committer for the three of us. And since i do not know exactly what the problem is i will add only my Signed-off to avoid confusion. Please speak up it that would be wrong as well and point me to the docs i missed. Or tell me how it should be done. > ... > > > +config SIEMENS_SIMATIC_IPC > > + tristate "Siemens Simatic IPC Class driver" > > + depends on PCI > > + help > > + This Simatic IPC class driver is the central of several > > drivers. It > > + is mainly used for system identification, after which > > drivers in other > > + classes will take care of driving specifics of those > > machines. > > + i.e. leds and watchdogs > > LEDs > watchdog. (missed period and singular form) > > Module name? > > > endif # X86_PLATFORM_DEVICES > > Not sure about the ordering of the section in Kconfig, but it is up to > maintainers. > > ... > > > +# Siemens Simatic Industrial PCs > > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o > > Ditto. I will check both again if a find a pattern, otherwise will wait for maintainers to complain and hopefully suggest. > ... > > > + * Siemens SIMATIC IPC driver for LEDs > > It seems to be used in patch 4 which is not LED related. Also, why is > it here if it's for LEDs? > > ... > > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > Replace with SPDX > > ... > > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/platform_device.h> > > Ordered? > > > +#include <linux/platform_data/x86/simatic-ipc.h> > > ... > > > +static int register_platform_devices(u32 station_id) > > +{ > > + int i; > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > Reversed xmas tree order? I do not get this, it is almost easter egg order time. Please explain. > > + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE; > > + > > + for (i = 0; i < ARRAY_SIZE(device_modes); i++) { > > + if (device_modes[i].station_id == station_id) { > > + ledmode = device_modes[i].led_mode; > > + wdtmode = device_modes[i].wdt_mode; > > + break; > > + } > > + } > > + > > + if (ledmode != SIMATIC_IPC_DEVICE_NONE) { > > + platform_data.devmode = ledmode; > > > + ipc_led_platform_device = > > platform_device_register_data > > + (NULL, KBUILD_MODNAME "_leds", > > PLATFORM_DEVID_NONE, > > + &platform_data, sizeof(struct > > simatic_ipc_platform)); > > Strange indentation (second line). > > > + if (IS_ERR(ipc_led_platform_device)) > > + return PTR_ERR(ipc_led_platform_device); > > > + pr_debug(KBUILD_MODNAME ": device=%s created\n", > > + ipc_led_platform_device->name); > > Utilize pr_fmt() instead of adding prefixes like this. > > > + } > > > + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) { > > + platform_data.devmode = wdtmode; > > + ipc_wdt_platform_device = > > platform_device_register_data > > + (NULL, KBUILD_MODNAME "_wdt", > > PLATFORM_DEVID_NONE, > > + &platform_data, sizeof(struct > > simatic_ipc_platform)); > > + if (IS_ERR(ipc_wdt_platform_device)) > > + return PTR_ERR(ipc_wdt_platform_device); > > + > > + pr_debug(KBUILD_MODNAME ": device=%s created\n", > > + ipc_wdt_platform_device->name); > > + } > > Same comments as above. > > > + if (ledmode == SIMATIC_IPC_DEVICE_NONE && > > + wdtmode == SIMATIC_IPC_DEVICE_NONE) { > > + pr_warn(KBUILD_MODNAME > > + ": unsupported IPC detected, station > > id=%08x\n", > > + station_id); > > Ugly indentation. With pr_fmt() in use will be much better. > > > + return -EINVAL; > > > + } else { > > Redundant. > > > + return 0; > > + } > > +} > > ... > > > +/* > > + * Get membase address from PCI, used in leds and wdt modul. Here > > we read > > + * the bar0. The final address calculation is done in the > > appropriate modules > > bar -> BAR > Missed period. > > > + */ > > > + > > Unneeded blank line. > > > +u32 simatic_ipc_get_membase0(unsigned int p2sb) > > +{ > > + u32 bar0 = 0; > > > +#ifdef CONFIG_PCI > > It's ugly besides the fact that you have a dependency. left over from out-of-tree, will be removed rest is clear, Thanks! Henning > > + struct pci_bus *bus; > > Missed blank line. > > > + /* > > + * The GPIO memory is bar0 of the hidden P2SB device. > > Unhide the device > > + * to have a quick look at it, before we hide it again. > > + * Also grab the pci rescan lock so that device does not > > get discovered > > + * and remapped while it is visible. > > + * This code is inspired by drivers/mfd/lpc_ich.c > > + */ > > + bus = pci_find_bus(0, 0); > > + pci_lock_rescan_remove(); > > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0); > > + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, > > &bar0); + > > + bar0 &= ~0xf; > > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1); > > + pci_unlock_rescan_remove(); > > +#endif /* CONFIG_PCI */ > > + return bar0; > > +} > > +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > Oy vey! I know what this is and let's do it differently. I have some > (relatively old) patch series I can send you privately for testing. > > ... > > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > Not needed when you have SPDX. > > ... > > > +#include <linux/pci.h> > > Wrong header. Should be types.h > > ... > > > +#include <linux/dmi.h> > > +#include <linux/platform_data/x86/simatic-ipc-base.h> > > Missed headers. You need to include ones that the code below is a > direct user of. > > Like types.h >
On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 3/4/21 11:11 AM, Andy Shevchenko wrote: > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > > <henning.schild@siemens.com> wrote: ... > >> +u32 simatic_ipc_get_membase0(unsigned int p2sb) > >> +{ > >> + u32 bar0 = 0; > > > >> +#ifdef CONFIG_PCI > > > > It's ugly besides the fact that you have a dependency. > > > >> + struct pci_bus *bus; > > > > Missed blank line. > > > >> + /* > >> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device > >> + * to have a quick look at it, before we hide it again. > >> + * Also grab the pci rescan lock so that device does not get discovered > >> + * and remapped while it is visible. > >> + * This code is inspired by drivers/mfd/lpc_ich.c > >> + */ > >> + bus = pci_find_bus(0, 0); > >> + pci_lock_rescan_remove(); > >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0); > >> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0); > >> + > >> + bar0 &= ~0xf; > >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1); > >> + pci_unlock_rescan_remove(); > >> +#endif /* CONFIG_PCI */ > >> + return bar0; > >> +} > >> +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > > > Oy vey! I know what this is and let's do it differently. I have some > > (relatively old) patch series I can send you privately for testing. > > This bit stood out the most to me too, it would be good if we can this fixed > in some cleaner work. So I'm curious how things will look with Andy's work > integrated. > > Also I don't think this should be exported. Instead this (or its replacement) > should be used to get the address for an IOMEM resource to add the platform > devices when they are instantiated. Then the platform-dev drivers can just > use the regular functions to get their resources instead of relying on this > module. I have published a WIP branch [1]. I have no means to test (I don't know what hardware at hand I can use right now), but I made it compile after 4 years of gathering dust... Feel free to give any kind of comments or share your ideas on how it can be improved (the above idea on IOMEM resource is interesting, but devices are PCI, not sure how this can be done). [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb -- With Best Regards, Andy Shevchenko
On Thu, Mar 4, 2021 at 9:52 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Thu, 4 Mar 2021 12:11:12 +0200 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > > <henning.schild@siemens.com> wrote: ... > > Check for the rest of the series as well (basically this is the rule > > of thumb to recheck entire code for the comment you have got at any > > single place of it). > > For some code Gerd is the author, and i am the co-Author. We even have > Jan in the mix at places. At least in copyright headers. > > I will remain the committer for the three of us. And since i do not > know exactly what the problem is i will add only my Signed-off to avoid > confusion. > > Please speak up it that would be wrong as well and point me to the docs > i missed. Or tell me how it should be done. Then make sure that you have From line with the Author (`git commit --amend --author="..."`) and add your Co-developed-by tag. ... > > > + int i; > > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > > > Reversed xmas tree order? > > I do not get this, it is almost easter egg order time. Please explain. Longer lines usually go first. See above :-)
Hi, On 3/5/21 4:42 PM, Andy Shevchenko wrote: > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 3/4/21 11:11 AM, Andy Shevchenko wrote: >>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild >>> <henning.schild@siemens.com> wrote: > > ... > >>>> +u32 simatic_ipc_get_membase0(unsigned int p2sb) >>>> +{ >>>> + u32 bar0 = 0; >>> >>>> +#ifdef CONFIG_PCI >>> >>> It's ugly besides the fact that you have a dependency. >>> >>>> + struct pci_bus *bus; >>> >>> Missed blank line. >>> >>>> + /* >>>> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device >>>> + * to have a quick look at it, before we hide it again. >>>> + * Also grab the pci rescan lock so that device does not get discovered >>>> + * and remapped while it is visible. >>>> + * This code is inspired by drivers/mfd/lpc_ich.c >>>> + */ >>>> + bus = pci_find_bus(0, 0); >>>> + pci_lock_rescan_remove(); >>>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0); >>>> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0); >>>> + >>>> + bar0 &= ~0xf; >>>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1); >>>> + pci_unlock_rescan_remove(); >>>> +#endif /* CONFIG_PCI */ >>>> + return bar0; >>>> +} >>>> +EXPORT_SYMBOL(simatic_ipc_get_membase0); >>> >>> Oy vey! I know what this is and let's do it differently. I have some >>> (relatively old) patch series I can send you privately for testing. >> >> This bit stood out the most to me too, it would be good if we can this fixed >> in some cleaner work. So I'm curious how things will look with Andy's work >> integrated. >> >> Also I don't think this should be exported. Instead this (or its replacement) >> should be used to get the address for an IOMEM resource to add the platform >> devices when they are instantiated. Then the platform-dev drivers can just >> use the regular functions to get their resources instead of relying on this >> module. > > I have published a WIP branch [1]. I have no means to test (I don't > know what hardware at hand I can use right now), but I made it compile > after 4 years of gathering dust... So I took a quick look at the following 2 commits: "platform/x86: p2sb: New Primary to Sideband bridge support library" "mfd: lpc_ich: Switch to generic p2sb_bar()" And this looks good to me, although compared to the code from this patch-set you are missing the pci_lock_rescan_remove(); and pci_unlock_rescan_remove(); calls. > Feel free to give any kind of comments or share your ideas on how it > can be improved (the above idea on IOMEM resource is interesting, but > devices are PCI, not sure how this can be done). The code added by this patch introduces a register_platform_devices() function which creates a bunch of platform-devices; and then the device-drivers for those call simatic_ipc_get_membase0() to get their base-address. My suggestion was to instead put the simatic_ipc_get_membase0() call inside the code instantiating the platform devices and to add the base-address for that pdev as IOMEM resource to the instantiated platform-devices. I hope this helps to clarify what I was trying to say. Regards, Hans
On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 3/5/21 4:42 PM, Andy Shevchenko wrote: > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 3/4/21 11:11 AM, Andy Shevchenko wrote: > >>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > >>> <henning.schild@siemens.com> wrote: ... > >>> Oy vey! I know what this is and let's do it differently. I have some > >>> (relatively old) patch series I can send you privately for testing. > >> > >> This bit stood out the most to me too, it would be good if we can this fixed > >> in some cleaner work. So I'm curious how things will look with Andy's work > >> integrated. > >> > >> Also I don't think this should be exported. Instead this (or its replacement) > >> should be used to get the address for an IOMEM resource to add the platform > >> devices when they are instantiated. Then the platform-dev drivers can just > >> use the regular functions to get their resources instead of relying on this > >> module. > > > > I have published a WIP branch [1]. I have no means to test (I don't > > know what hardware at hand I can use right now), but I made it compile > > after 4 years of gathering dust... > > So I took a quick look at the following 2 commits: (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, do you think it's better like that? The idea is to deduplicate __pci_bus_read_base() call) > "platform/x86: p2sb: New Primary to Sideband bridge support library" > "mfd: lpc_ich: Switch to generic p2sb_bar()" > > And this looks good to me, although compared to the code from this > patch-set you are missing the pci_lock_rescan_remove(); and > pci_unlock_rescan_remove(); calls. Oh, indeed. > > Feel free to give any kind of comments or share your ideas on how it > > can be improved (the above idea on IOMEM resource is interesting, but > > devices are PCI, not sure how this can be done). > > The code added by this patch introduces a register_platform_devices() > function which creates a bunch of platform-devices; and then the > device-drivers for those call simatic_ipc_get_membase0() to get their > base-address. Sounds like an MFD approach... > My suggestion was to instead put the simatic_ipc_get_membase0() call > inside the code instantiating the platform devices and to add the > base-address for that pdev as IOMEM resource to the instantiated > platform-devices. > > I hope this helps to clarify what I was trying to say. Yes, thanks!
Am Fri, 5 Mar 2021 17:46:08 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 9:52 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Thu, 4 Mar 2021 12:11:12 +0200 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > > > <henning.schild@siemens.com> wrote: > > ... > > > > Check for the rest of the series as well (basically this is the > > > rule of thumb to recheck entire code for the comment you have got > > > at any single place of it). > > > > For some code Gerd is the author, and i am the co-Author. We even > > have Jan in the mix at places. At least in copyright headers. > > > > I will remain the committer for the three of us. And since i do not > > know exactly what the problem is i will add only my Signed-off to > > avoid confusion. > > > > Please speak up it that would be wrong as well and point me to the > > docs i missed. Or tell me how it should be done. > > Then make sure that you have From line with the Author (`git commit > --amend --author="..."`) and add your Co-developed-by tag. > > ... > > > > > + int i; > > > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > > > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > > > > > Reversed xmas tree order? > > > > I do not get this, it is almost easter egg order time. Please > > explain. > > Longer lines > usually go > first. Henning see i > See above :-) >
Hi, On 3/5/21 5:25 PM, Andy Shevchenko wrote: > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 3/5/21 4:42 PM, Andy Shevchenko wrote: >>> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> On 3/4/21 11:11 AM, Andy Shevchenko wrote: >>>>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild >>>>> <henning.schild@siemens.com> wrote: > > ... > >>>>> Oy vey! I know what this is and let's do it differently. I have some >>>>> (relatively old) patch series I can send you privately for testing. >>>> >>>> This bit stood out the most to me too, it would be good if we can this fixed >>>> in some cleaner work. So I'm curious how things will look with Andy's work >>>> integrated. >>>> >>>> Also I don't think this should be exported. Instead this (or its replacement) >>>> should be used to get the address for an IOMEM resource to add the platform >>>> devices when they are instantiated. Then the platform-dev drivers can just >>>> use the regular functions to get their resources instead of relying on this >>>> module. >>> >>> I have published a WIP branch [1]. I have no means to test (I don't >>> know what hardware at hand I can use right now), but I made it compile >>> after 4 years of gathering dust... >> >> So I took a quick look at the following 2 commits: > > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, > do you think it's better like that? The idea is to deduplicate > __pci_bus_read_base() call) > >> "platform/x86: p2sb: New Primary to Sideband bridge support library" >> "mfd: lpc_ich: Switch to generic p2sb_bar()" >> >> And this looks good to me, although compared to the code from this >> patch-set you are missing the pci_lock_rescan_remove(); and >> pci_unlock_rescan_remove(); calls. > > Oh, indeed. > >>> Feel free to give any kind of comments or share your ideas on how it >>> can be improved (the above idea on IOMEM resource is interesting, but >>> devices are PCI, not sure how this can be done). >> >> The code added by this patch introduces a register_platform_devices() >> function which creates a bunch of platform-devices; and then the >> device-drivers for those call simatic_ipc_get_membase0() to get their >> base-address. > > Sounds like an MFD approach... Yes except that there does not seem to be a clear parent for these devices, so it is MFD-ish. IOW I'm not sure this should be changed to use the MFD framework, but I was thinking along those line myself too. Henning did you look into using the MFD framework + MFD cell descriptions to instantiate the platform-devices for you ? Regards, Hans
On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 3/5/21 4:42 PM, Andy Shevchenko wrote: ... > > So I took a quick look at the following 2 commits: > > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, > do you think it's better like that? The idea is to deduplicate > __pci_bus_read_base() call) > > > "platform/x86: p2sb: New Primary to Sideband bridge support library" > > "mfd: lpc_ich: Switch to generic p2sb_bar()" > > > > And this looks good to me, although compared to the code from this > > patch-set you are missing the pci_lock_rescan_remove(); and > > pci_unlock_rescan_remove(); calls. > > Oh, indeed. Correction here, I'm using pci_bus_sem in the latest version. -- With Best Regards, Andy Shevchenko
On Fri, Mar 5, 2021 at 6:41 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > On 3/5/21 4:42 PM, Andy Shevchenko wrote: > > ... > > > > So I took a quick look at the following 2 commits: > > > > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, > > do you think it's better like that? The idea is to deduplicate > > __pci_bus_read_base() call) > > > > > "platform/x86: p2sb: New Primary to Sideband bridge support library" > > > "mfd: lpc_ich: Switch to generic p2sb_bar()" > > > > > > And this looks good to me, although compared to the code from this > > > patch-set you are missing the pci_lock_rescan_remove(); and > > > pci_unlock_rescan_remove(); calls. > > > > Oh, indeed. > > Correction here, I'm using pci_bus_sem in the latest version. Okay, it seems that semaphore is not what I need and PCI rescan lock is what is mandatory to take here. -- With Best Regards, Andy Shevchenko
On Fri, Mar 5, 2021 at 6:47 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Fri, 5 Mar 2021 17:42:42 +0200 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> > > wrote: ... > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb > > That is a little weird, might be a good idea to RFC reply to the cover > letter of this one. To allow review and discussion in a central place. I'm now rebasing it to be more presentable. If you can test this approach and it works for you, I'll send a formal RFC series.
On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Fri, 5 Mar 2021 17:42:42 +0200 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> > > > wrote: > > ... > > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb > > > > That is a little weird, might be a good idea to RFC reply to the cover > > letter of this one. To allow review and discussion in a central place. > > I'm now rebasing it to be more presentable. > If you can test this approach and it works for you, I'll send a formal > RFC series. Okay, [1] now is in presentable shape, each patch with a proper commit message and authorship, also all patches are compiled without issues. -- With Best Regards, Andy Shevchenko
Am Fri, 5 Mar 2021 19:44:57 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild > > <henning.schild@siemens.com> wrote: > > > Am Fri, 5 Mar 2021 17:42:42 +0200 > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede > > > > <hdegoede@redhat.com> wrote: > > > > ... > > > > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb > > > > > > That is a little weird, might be a good idea to RFC reply to the > > > cover letter of this one. To allow review and discussion in a > > > central place. > > > > I'm now rebasing it to be more presentable. > > If you can test this approach and it works for you, I'll send a > > formal RFC series. > > Okay, [1] now is in presentable shape, each patch with a proper commit > message and authorship, also all patches are compiled without issues. Thank you so much, i will base v2 on that and let you know how that works. regards, Henning
Am Wed, 3 Mar 2021 15:21:05 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Hi, > > thanks for the fast and thorough review! > > Am Tue, 2 Mar 2021 10:38:19 -0800 > schrieb Guenter Roeck <linux@roeck-us.net>: > > > On 3/2/21 8:33 AM, Henning Schild wrote: > > > From: Henning Schild <henning.schild@siemens.com> > > > > > > This driver adds initial support for several devices from Siemens. > > > It is based on a platform driver introduced in an earlier commit. > > > > > > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > > Signed-off-by: Henning Schild <henning.schild@siemens.com> > > > --- > > > drivers/watchdog/Kconfig | 11 ++ > > > drivers/watchdog/Makefile | 1 + > > > drivers/watchdog/simatic-ipc-wdt.c | 305 > > > +++++++++++++++++++++++++++++ 3 files changed, 317 insertions(+) > > > create mode 100644 drivers/watchdog/simatic-ipc-wdt.c > > > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > > index 1fe0042a48d2..948497eb4bef 100644 > > > --- a/drivers/watchdog/Kconfig > > > +++ b/drivers/watchdog/Kconfig > > > @@ -1575,6 +1575,17 @@ config NIC7018_WDT > > > To compile this driver as a module, choose M here: the > > > module will be called nic7018_wdt. > > > > > > +config SIEMENS_SIMATIC_IPC_WDT > > > + tristate "Siemens Simatic IPC Watchdog" > > > + depends on SIEMENS_SIMATIC_IPC > > > + select WATCHDOG_CORE > > > + help > > > + This driver adds support for several watchdogs found in > > > Industrial > > > + PCs from Siemens. > > > + > > > + To compile this driver as a module, choose M here: the > > > module will be > > > + called simatic-ipc-wdt. > > > + > > > # M68K Architecture > > > > > > config M54xx_WATCHDOG > > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > > index f3a6540e725e..7f5c73ec058c 100644 > > > --- a/drivers/watchdog/Makefile > > > +++ b/drivers/watchdog/Makefile > > > @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o > > > obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o > > > obj-$(CONFIG_MLX_WDT) += mlx_wdt.o > > > obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o > > > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o > > > > > > # M68K Architecture > > > obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o > > > diff --git a/drivers/watchdog/simatic-ipc-wdt.c > > > b/drivers/watchdog/simatic-ipc-wdt.c new file mode 100644 > > > index 000000000000..b5c8b7ceb404 > > > --- /dev/null > > > +++ b/drivers/watchdog/simatic-ipc-wdt.c > > > @@ -0,0 +1,305 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Siemens SIMATIC IPC driver for Watchdogs > > > + * > > > + * Copyright (c) Siemens AG, 2020-2021 > > > + * > > > + * Authors: > > > + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > > + * > > > + * This program is free software; you can redistribute it and/or > > > modify > > > + * it under the terms of the GNU General Public License version 2 > > > as > > > + * published by the Free Software Foundation. > > > > Covered by SPDX-License-Identifier > > > > > + */ > > > + > > > +#include <linux/device.h> > > > +#include <linux/init.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/errno.h> > > > +#include <linux/watchdog.h> > > > +#include <linux/ioport.h> > > > +#include <linux/sizes.h>> +#include <linux/io.h> > > > +#include <linux/platform_data/x86/simatic-ipc-base.h> > > > > Alphabetic order please > > > > > + > > > +#define WD_ENABLE_IOADR 0x62 > > > +#define WD_TRIGGER_IOADR 0x66 > > > +#define GPIO_COMMUNITY0_PORT_ID 0xaf > > > +#define PAD_CFG_DW0_GPP_A_23 0x4b8 > > > > Please increase indentation and spare another tab > > > > > +#define SAFE_EN_N_427E 0x01 > > > +#define SAFE_EN_N_227E 0x04 > > > +#define WD_ENABLED 0x01 > > > + > > > +#define TIMEOUT_MIN 2 > > > +#define TIMEOUT_DEF 64 > > > +#define TIMEOUT_MAX 64 > > > + > > > +#define GP_STATUS_REG_227E 0x404D /* IO PORT for > > > SAFE_EN_N on 227E */ + > > > +static bool nowayout = WATCHDOG_NOWAYOUT; > > > +module_param(nowayout, bool, 0000); > > > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once > > > started (default=" > > > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > > > + > > > +static DEFINE_SPINLOCK(io_lock); /* the lock for io > > > operations */ +static struct watchdog_device wdd; > > > + > > > > Having two variables named 'wdd' is confusing. Please chose another > > name. > > > > > +static struct resource gp_status_reg_227e_res = > > > + DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, > > > KBUILD_MODNAME); + > > > +static struct resource io_resource = > > > + DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1, > > > + KBUILD_MODNAME " WD_ENABLE_IOADR"); > > > + > > > +/* the actual start will be discovered with pci, 0 is a > > > placeholder */ +static struct resource mem_resource = > > > + DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR"); > > > + > > > +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 }; > > > +static void __iomem *wd_reset_base_addr; > > > + > > > +static int get_timeout_idx(u32 timeout) > > > +{ > > > + int i; > > > + > > > + i = ARRAY_SIZE(wd_timeout_table) - 1; > > > + for (; i >= 0; i--) { > > > + if (timeout >= wd_timeout_table[i]) > > > + break; > > > + } > > > + > > > + return i; > > > +} > > > > Please add a comment explaining why you don't use find_closest(). > > Will not be a comment but we will switch to using this, thanks for > pointing it out. > > > > + > > > +static int wd_start(struct watchdog_device *wdd) > > > +{ > > > + u8 regval; > > > + > > > + spin_lock(&io_lock); > > > + > > The watchdog subsystem already provides locking > > since the watchdog device can only be opened once. > > > > Why is the additional lock needed ? > > We had this under internal review and somehow came to the conclusion > that we "might" need it. I think we will remove it or come back with > reasons. > > > > + regval = inb(WD_ENABLE_IOADR); > > > + regval |= WD_ENABLED; > > > + outb(regval, WD_ENABLE_IOADR); > > > + > > > + spin_unlock(&io_lock); > > > + > > > + return 0; > > > +} > > > + > > > +static int wd_stop(struct watchdog_device *wdd) > > > +{ > > > + u8 regval; > > > + > > > + spin_lock(&io_lock); > > > + > > > + regval = inb(WD_ENABLE_IOADR); > > > + regval &= ~WD_ENABLED; > > > + outb(regval, WD_ENABLE_IOADR); > > > + > > > + spin_unlock(&io_lock); > > > + > > > + return 0; > > > +} > > > + > > > +static int wd_ping(struct watchdog_device *wdd) > > > +{ > > > + inb(WD_TRIGGER_IOADR); > > > + return 0; > > > +} > > > + > > > +static int wd_set_timeout(struct watchdog_device *wdd, unsigned > > > int t) +{ > > > + u8 regval; > > > + int timeout_idx = get_timeout_idx(t); > > > + > > > + spin_lock(&io_lock); > > > + > > > + regval = inb(WD_ENABLE_IOADR) & 0xc7; > > > + regval |= timeout_idx << 3; > > > + outb(regval, WD_ENABLE_IOADR); > > > + > > > + spin_unlock(&io_lock); > > > + wdd->timeout = wd_timeout_table[timeout_idx]; > > > + > > > + return 0; > > > +} > > > + > > > +static const struct watchdog_info wdt_ident = { > > > + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING > > > | > > > + WDIOF_SETTIMEOUT, > > > + .identity = KBUILD_MODNAME, > > > +}; > > > + > > > +static const struct watchdog_ops wdt_ops = { > > > + .owner = THIS_MODULE, > > > + .start = wd_start, > > > + .stop = wd_stop, > > > + .ping = wd_ping, > > > + .set_timeout = wd_set_timeout, > > > +}; > > > + > > > +static void wd_set_safe_en_n(u32 wdtmode, bool safe_en_n) > > > +{ > > > + u16 resetbit; > > > + > > > + if (wdtmode == SIMATIC_IPC_DEVICE_227E) { > > > + /* enable SAFE_EN_N on GP_STATUS_REG_227E */ > > > + resetbit = inw(GP_STATUS_REG_227E); > > > + if (safe_en_n) > > > + resetbit &= ~SAFE_EN_N_227E; > > > + else > > > + resetbit |= SAFE_EN_N_227E; > > > + > > > + outw(resetbit, GP_STATUS_REG_227E); > > > + } else { > > > + /* enable SAFE_EN_N on PCH D1600 */ > > > + resetbit = ioread16(wd_reset_base_addr); > > > + > > > + if (safe_en_n) > > > + resetbit &= ~SAFE_EN_N_427E; > > > + else > > > + resetbit |= SAFE_EN_N_427E; > > > + > > > + iowrite16(resetbit, wd_reset_base_addr); > > > + } > > > +} > > > + > > > +static int wd_setup(u32 wdtmode, bool safe_en_n) > > > +{ > > > + u8 regval; > > > + int timeout_idx = 0; > > > > Unnecessary initialization > > > > > + bool alarm_active; > > > + > > > + timeout_idx = get_timeout_idx(TIMEOUT_DEF); > > > + > > > + wd_set_safe_en_n(wdtmode, safe_en_n); > > > + > > > + /* read wd register to determine alarm state */ > > > + regval = inb(WD_ENABLE_IOADR); > > > + if (regval & 0x80) { > > > + pr_warn("Watchdog alarm active.\n"); > > > > Why does that warrant a warning, and what does it mean ? The context > > suggests that it means the previous reset was caused by the > > watchdog, but that is not what the message says. > > > > > + regval = 0x82; /* use only macro mode, > > > reset alarm bit */ > > > + alarm_active = true; > > > + } else { > > > + regval = 0x02; /* use only macro mode */ > > > + alarm_active = false; > > > + } > > > > Would it hurt to just always write 0x82 ? > > alarm_active = regval & 0x80; > > regval = 0x82 | timeout_idx << 3; > > > > would be much simpler. Or, if you prefer, > > alarm_active = !!(regval & 0x80); > > regval = 0x82 | timeout_idx << 3; > > > > Actually, regval isn't even needed in that case. > > alarm_active = !!(regval & 0x80); > > outb(0x82 | timeout_idx << 3, WD_ENABLE_IOADR); > > > > > > Either case, please use defines for the bits. WD_ENABLED is already > > defined, thus the other bits should be set using defines as well. > > > > > + > > > + regval |= timeout_idx << 3; > > > + if (nowayout) > > > + regval |= WD_ENABLED; > > > > This is not the purpose of nowayout. nowayout prevents stopping > > the watchdog after it has been started. It is not expected to start > > the watchdog on boot. > > Thanks, that was misunderstood by the author, will fix. > > > > + > > > + outb(regval, WD_ENABLE_IOADR); > > > + > > > + return alarm_active; > > > +} > > > + > > > +static int simatic_ipc_wdt_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + int rc = 0; > > > + struct simatic_ipc_platform *plat = > > > pdev->dev.platform_data; > > > + struct resource *res; > > > + > > > > Is it guaranteed that the device will always be instantiated only > > once ? If so, how it it guaranteed ? > > I suppose if anything did register two platform devices on the > platform bus this might not be guaranteed. The assumption is that > simatic-ipc will only ever register one, and the machines always have > 0-1 "Siemens watchdogs" so at the moment there will never be a need > for more than one. Turns out the platform bus would not even allow registering the same device twice. So it is guaranteed but nobody every attempting it in the first place, and it never working in the second. Henning > > > Because if there are ever multiple instances the various static > > variables will cause major trouble (which is why it is always better > > to not use static variables). > > > > > + pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n", > > > + __func__, __LINE__, plat->devmode); > > > + > > > > This is a platform device. Please use dev_ messages (dev_warn, > > dev_dbg etc) throughout. > > > > > + switch (plat->devmode) { > > > + case SIMATIC_IPC_DEVICE_227E: > > > + if (!devm_request_region(dev, > > > gp_status_reg_227e_res.start, > > > + > > > resource_size(&gp_status_reg_227e_res), > > > + KBUILD_MODNAME)) { > > > + dev_err(dev, > > > + "Unable to register IO resource > > > at %pR\n", > > > + &gp_status_reg_227e_res); > > > + return -EBUSY; > > > + } > > > + fallthrough; > > > + case SIMATIC_IPC_DEVICE_427E: > > > + wdd.info = &wdt_ident; > > > + wdd.ops = &wdt_ops; > > > + wdd.min_timeout = TIMEOUT_MIN; > > > + wdd.max_timeout = TIMEOUT_MAX; > > > > Why not use static initialization ? > > > > > + wdd.parent = NULL; > > > > parent should be the platform device. > > > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + if (!devm_request_region(dev, io_resource.start, > > > + resource_size(&io_resource), > > > + io_resource.name)) { > > > + dev_err(dev, > > > + "Unable to register IO resource at > > > %#x\n", > > > + WD_ENABLE_IOADR); > > > + return -EBUSY; > > > + } > > > > If this is what prevents multiple registrations, it is too late: wdd > > is already overwritten. > > As said, there will be no double-registration. > > > > + > > > + if (plat->devmode == SIMATIC_IPC_DEVICE_427E) { > > > + res = &mem_resource; > > > + > > > + /* get GPIO base from PCI */ > > > + res->start = > > > simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1)); > > > + if (res->start == 0) > > > + return -ENODEV; > > > + > > > + /* do the final address calculation */ > > > + res->start = res->start + > > > (GPIO_COMMUNITY0_PORT_ID << 16) + > > > + PAD_CFG_DW0_GPP_A_23; > > > + res->end += res->start; > > > + > > > + wd_reset_base_addr = devm_ioremap_resource(dev, > > > res); > > > + if (IS_ERR(wd_reset_base_addr)) > > > + return -ENOMEM; > > > > return PTR_ERR(wd_reset_base_addr); > > > > > + } > > > + > > > + wdd.bootstatus = wd_setup(plat->devmode, true); > > > > bootstatus does not report a boolean. This translates to > > WDIOF_OVERHEAT which is almost certainly wrong. > > > > > + if (wdd.bootstatus) > > > + pr_warn(KBUILD_MODNAME ": last reboot caused by > > > watchdog reset\n"); > > > > Why two messages ? > > > > > + > > > + watchdog_set_nowayout(&wdd, nowayout); > > > + watchdog_stop_on_reboot(&wdd); > > > + > > > + rc = devm_watchdog_register_device(dev, &wdd); > > > + > > Extra empty line not needed > > > > > + if (rc == 0) > > > + pr_debug("initialized. nowayout=%d\n", > > > + nowayout); > > > > What is the value of this message (especially since there is no > > message if there is an error) ? > > > > > + > > > + return rc; > > > +} > > > + > > > +static int simatic_ipc_wdt_remove(struct platform_device *pdev) > > > +{ > > > + struct simatic_ipc_platform *plat = > > > pdev->dev.platform_data; + > > > + wd_setup(plat->devmode, false); > > > > This warrants an explanation. What is the point of updating the > > timeout here ? And what does SAFE_EN actually do ? > > The idea was that module unloading should disable the watchdog, but > that code will be removed and aligned with other watchdogs. > > SAFE_EN is a second enable bit, if it is not set the watchdog will > fire into the void. Pretty pointless will keep that always armed or > set it always when toggling "enable". > > All the other open points are pretty clear, and will all be dealt with > in v2. > > regards, > Henning > > > The watchdog is stopped on reboot, but this function won't > > be called in that case, making this call even more questionable. > > Please document what it does and why it is needed here (but not > > when rebooting). > > > > > + return 0; > > > +} > > > + > > > +static struct platform_driver wdt_driver = { > > > + .probe = simatic_ipc_wdt_probe, > > > + .remove = simatic_ipc_wdt_remove, > > > + .driver = { > > > + .name = KBUILD_MODNAME, > > > + }, > > > +}; > > > + > > > +static int __init simatic_ipc_wdt_init(void) > > > +{ > > > + return platform_driver_register(&wdt_driver); > > > +} > > > + > > > +static void __exit simatic_ipc_wdt_exit(void) > > > +{ > > > + platform_driver_unregister(&wdt_driver); > > > +} > > > + > > > +module_init(simatic_ipc_wdt_init); > > > +module_exit(simatic_ipc_wdt_exit); > > > > Why not module_platform_driver() ? > > > > > + > > > +MODULE_LICENSE("GPL"); > > > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>"); > > > > > >
From: Henning Schild <henning.schild@siemens.com> This series adds support for watchdogs and leds of several x86 devices from Siemens. It is structured with a platform driver that mainly does identification of the machines. It might trigger loading of the actual device drivers by attaching devices to the platform bus. The identification is vendor specific, parsing a special binary DMI entry. The implementation of that platform identification is applied on pmc_atom clock quirks in the final patch. It is all structured in a way that we can easily add more devices and more platform drivers later. Internally we have some more code for hardware monitoring, more leds, watchdogs etc. This will follow some day. But the idea here is to share early, and hopefully not fail early. Henning Schild (4): platform/x86: simatic-ipc: add main driver for Siemens devices leds: simatic-ipc-leds: add new driver for Siemens Industial PCs watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs drivers/leds/Kconfig | 11 + drivers/leds/Makefile | 1 + drivers/leds/simatic-ipc-leds.c | 224 +++++++++++++ drivers/platform/x86/Kconfig | 9 + drivers/platform/x86/Makefile | 3 + drivers/platform/x86/pmc_atom.c | 39 +-- drivers/platform/x86/simatic-ipc.c | 166 ++++++++++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/simatic-ipc-wdt.c | 305 ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h | 33 ++ include/linux/platform_data/x86/simatic-ipc.h | 68 ++++ 12 files changed, 853 insertions(+), 18 deletions(-) create mode 100644 drivers/leds/simatic-ipc-leds.c create mode 100644 drivers/platform/x86/simatic-ipc.c create mode 100644 drivers/watchdog/simatic-ipc-wdt.c create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h create mode 100644 include/linux/platform_data/x86/simatic-ipc.h