| Message ID | 20210329174928.18816-4-henning.schild@siemens.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On 29.03.21 19:49, Henning Schild wrote: Hi, > This driver adds initial support for several devices from Siemens. It is > based on a platform driver introduced in an earlier commit. Where does the wdt actually come from ? Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty usual case. Or some external chip ? The code smells a bit like two entirely different wdt's that just have some similarities. If that's the case, I'd rather split it into two separate drivers and let the parent driver (board file) instantiate the correct one. --mtx -- --- Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren GPG/PGP-Schlüssel zu. --- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
Am Thu, 1 Apr 2021 18:15:41 +0200 schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > On 29.03.21 19:49, Henning Schild wrote: > > Hi, > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > Where does the wdt actually come from ? > > Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty > usual case. > > Or some external chip ? I guess external chip, but again we are talking about multiple machines. And the manuals i read so far do not go into that sort of detail. In fact on some of the machines you will have two watchdogs, one from the SoC and that "special" one. That has several reasons, probably not too important here. The HW guys are adding another wd not just for fun, and it would be nice to have a driver. > The code smells a bit like two entirely different wdt's that just have > some similarities. If that's the case, I'd rather split it into two > separate drivers and let the parent driver (board file) instantiate > the correct one. Yes, it is two. Just like for the LEDs. One version PIO-based another version gpio/p2sb/mmio based. In fact the latter should very likely be based on that gpio pinctl, whether it really needs to be a separate driver will have to be seen. There are probably pros and cons for both options. regards, Henning > > --mtx >
On Tue, Apr 6, 2021 at 5:56 PM Henning Schild <henning.schild@siemens.com> wrote: > > Am Thu, 1 Apr 2021 18:15:41 +0200 > schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > > > On 29.03.21 19:49, Henning Schild wrote: > > > > Hi, > > > > > This driver adds initial support for several devices from Siemens. > > > It is based on a platform driver introduced in an earlier commit. > > > > Where does the wdt actually come from ? > > > > Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty > > usual case. > > > > Or some external chip ? > > I guess external chip, but again we are talking about multiple > machines. And the manuals i read so far do not go into that sort of > detail. In fact on some of the machines you will have two watchdogs, > one from the SoC and that "special" one. > That has several reasons, probably not too important here. The HW guys > are adding another wd not just for fun, and it would be nice to have a > driver. > > > The code smells a bit like two entirely different wdt's that just have > > some similarities. If that's the case, I'd rather split it into two > > separate drivers and let the parent driver (board file) instantiate > > the correct one. > > Yes, it is two. Just like for the LEDs. One version PIO-based another > version gpio/p2sb/mmio based. I tend to agree with Enrico that this should be two separate drivers. > In fact the latter should very likely be based on that gpio pinctl, > whether it really needs to be a separate driver will have to be seen. > There are probably pros and cons for both options. -- With Best Regards, Andy Shevchenko
On 4/7/21 1:53 AM, Andy Shevchenko wrote: > On Tue, Apr 6, 2021 at 5:56 PM Henning Schild > <henning.schild@siemens.com> wrote: >> >> Am Thu, 1 Apr 2021 18:15:41 +0200 >> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: >> >>> On 29.03.21 19:49, Henning Schild wrote: >>> >>> Hi, >>> >>>> This driver adds initial support for several devices from Siemens. >>>> It is based on a platform driver introduced in an earlier commit. >>> >>> Where does the wdt actually come from ? >>> >>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty >>> usual case. >>> >>> Or some external chip ? >> >> I guess external chip, but again we are talking about multiple >> machines. And the manuals i read so far do not go into that sort of >> detail. In fact on some of the machines you will have two watchdogs, >> one from the SoC and that "special" one. >> That has several reasons, probably not too important here. The HW guys >> are adding another wd not just for fun, and it would be nice to have a >> driver. >> >>> The code smells a bit like two entirely different wdt's that just have >>> some similarities. If that's the case, I'd rather split it into two >>> separate drivers and let the parent driver (board file) instantiate >>> the correct one. >> >> Yes, it is two. Just like for the LEDs. One version PIO-based another >> version gpio/p2sb/mmio based. > > I tend to agree with Enrico that this should be two separate drivers. > Agreed. Guenter >> In fact the latter should very likely be based on that gpio pinctl, >> whether it really needs to be a separate driver will have to be seen. >> There are probably pros and cons for both options. > >
Am Thu, 1 Apr 2021 18:15:41 +0200 schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > On 29.03.21 19:49, Henning Schild wrote: > > Hi, > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > Where does the wdt actually come from ? > > Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty > usual case. > > Or some external chip ? > > The code smells a bit like two entirely different wdt's that just have > some similarities. If that's the case, I'd rather split it into two > separate drivers and let the parent driver (board file) instantiate > the correct one. In fact they are the same watchdog device. The only difference is the "secondary enable" which controls whether the watchdog causes a reboot or just raises an alarm. The alarm feature is not even implemented in the given driver, we just enable that secondary enable regardless. In one range of devices (227E) that second enable is part of a pio-based control register. On the other range (427E) it unfortunately is a P2SB gpio, which gets us right into the discussion we have around the LEDs. With that i have my doubts that two drivers would be the way to go, most likely not. Only that i have no clue which pinctrl driver should be used here. My guess is "sunrisepoint" because the CPUs are "SkyLake" i.e. i5-6442EQ, i3-6102E And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted a kernel patched with the series from Andy but the "pinctrl-sunrisepoint" does not seem to even claim the memory. Still trying to understand how to make use of these pinctrl drivers they are in place but i lack example users (drivers). If they should be available in sysfs, i might be looking at the wrong place ... /sys/class/gpio/ does not list anything regards, Henning > > --mtx >
On 4/12/21 8:35 AM, Henning Schild wrote: > Am Thu, 1 Apr 2021 18:15:41 +0200 > schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > >> On 29.03.21 19:49, Henning Schild wrote: >> >> Hi, >> >>> This driver adds initial support for several devices from Siemens. >>> It is based on a platform driver introduced in an earlier commit. >> >> Where does the wdt actually come from ? >> >> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty >> usual case. >> >> Or some external chip ? >> >> The code smells a bit like two entirely different wdt's that just have >> some similarities. If that's the case, I'd rather split it into two >> separate drivers and let the parent driver (board file) instantiate >> the correct one. > > In fact they are the same watchdog device. The only difference is the > "secondary enable" which controls whether the watchdog causes a reboot > or just raises an alarm. The alarm feature is not even implemented in > the given driver, we just enable that secondary enable regardless. > Confusing statement; I can't parse "we just enable that secondary enable regardless". What secondary enable do you enable ? The code says "set safe_en_n so we are not just WDIOF_ALARMONLY", which suggests that it disables the alarm feature, and does make sense. > In one range of devices (227E) that second enable is part of a > pio-based control register. On the other range (427E) it unfortunately > is a P2SB gpio, which gets us right into the discussion we have around > the LEDs. > With that i have my doubts that two drivers would be the way to go, > most likely not. > Reading the code again, I agree. Still, you'll need to sort out how to determine if the watchdog or the LED driver should be enabled, and how to access the gpio port. The GPIO pin detection and use for 427E is a bit awkward. Thanks, Guenter > Only that i have no clue which pinctrl driver should be used here. My > guess is "sunrisepoint" because the CPUs are "SkyLake" i.e. i5-6442EQ, > i3-6102E > And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted a > kernel patched with the series from Andy but the "pinctrl-sunrisepoint" > does not seem to even claim the memory. Still trying to understand how > to make use of these pinctrl drivers they are in place but i lack > example users (drivers). If they should be available in sysfs, i might > be looking at the wrong place ... /sys/class/gpio/ does not list > anything > > regards, > Henning > > > >> >> --mtx >> >
Am Mon, 12 Apr 2021 09:06:10 -0700 schrieb Guenter Roeck <linux@roeck-us.net>: > On 4/12/21 8:35 AM, Henning Schild wrote: > > Am Thu, 1 Apr 2021 18:15:41 +0200 > > schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > > > >> On 29.03.21 19:49, Henning Schild wrote: > >> > >> Hi, > >> > >>> This driver adds initial support for several devices from Siemens. > >>> It is based on a platform driver introduced in an earlier commit. > >>> > >> > >> Where does the wdt actually come from ? > >> > >> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a > >> pretty usual case. > >> > >> Or some external chip ? > >> > >> The code smells a bit like two entirely different wdt's that just > >> have some similarities. If that's the case, I'd rather split it > >> into two separate drivers and let the parent driver (board file) > >> instantiate the correct one. > > > > In fact they are the same watchdog device. The only difference is > > the "secondary enable" which controls whether the watchdog causes a > > reboot or just raises an alarm. The alarm feature is not even > > implemented in the given driver, we just enable that secondary > > enable regardless. > > Confusing statement; I can't parse "we just enable that secondary > enable regardless". What secondary enable do you enable ? > > The code says "set safe_en_n so we are not just WDIOF_ALARMONLY", > which suggests that it disables the alarm feature, and does make > sense. Yes go with the second statement. But the alarm is the default after boot, and turning it off needs to be done with p2sb gpio on the 427. > > In one range of devices (227E) that second enable is part of a > > pio-based control register. On the other range (427E) it > > unfortunately is a P2SB gpio, which gets us right into the > > discussion we have around the LEDs. > > With that i have my doubts that two drivers would be the way to go, > > most likely not. > > > > Reading the code again, I agree. Still, you'll need to sort out how > to determine if the watchdog or the LED driver should be enabled, > and how to access the gpio port. The GPIO pin detection and use > for 427E is a bit awkward. Yes it is awkward, and that is exactly the discussion happening for the LEDs. Using generic GPIO code, the mail was more to Andy as i am hoping he might help me connect the dots here. On the other hand i wanted wdt discussions in the wdt thread, and not talk about that one gpio-pin in the LED thread. regards, Henning > Thanks, > Guenter > > > Only that i have no clue which pinctrl driver should be used here. > > My guess is "sunrisepoint" because the CPUs are "SkyLake" i.e. > > i5-6442EQ, i3-6102E > > And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted > > a kernel patched with the series from Andy but the > > "pinctrl-sunrisepoint" does not seem to even claim the memory. > > Still trying to understand how to make use of these pinctrl drivers > > they are in place but i lack example users (drivers). If they > > should be available in sysfs, i might be looking at the wrong place > > ... /sys/class/gpio/ does not list anything > > > > regards, > > Henning > > > > > > > >> > >> --mtx > >> > > >
Am Mon, 29 Mar 2021 19:49:27 +0200 schrieb 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: Henning Schild <henning.schild@siemens.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/Kconfig | 11 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/simatic-ipc-wdt.c | 215 > +++++++++++++++++++++++++++++ 3 files changed, 227 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..e901718d05b9 > --- /dev/null > +++ b/drivers/watchdog/simatic-ipc-wdt.c > @@ -0,0 +1,215 @@ > +// 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> > + */ > + > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/ioport.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/platform_data/x86/simatic-ipc-base.h> > +#include <linux/platform_device.h> > +#include <linux/sizes.h> > +#include <linux/util_macros.h> > +#include <linux/watchdog.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 WD_TRIGGERED 0x80 > +#define WD_MACROMODE 0x02 > + > +#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 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"); WD_TRIGGER_IOADR, SZ_1 missing here and in request_region part Henning > +/* 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 wd_start(struct watchdog_device *wdd) > +{ > + outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR); > + return 0; > +} > + > +static int wd_stop(struct watchdog_device *wdd) > +{ > + outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR); > + 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) +{ > + int timeout_idx = find_closest(t, wd_timeout_table, > + ARRAY_SIZE(wd_timeout_table)); > + > + outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3, > WD_ENABLE_IOADR); > + 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_secondary_enable(u32 wdtmode) > +{ > + u16 resetbit; > + > + /* set safe_en_n so we are not just WDIOF_ALARMONLY */ > + if (wdtmode == SIMATIC_IPC_DEVICE_227E) { > + /* enable SAFE_EN_N on GP_STATUS_REG_227E */ > + resetbit = inw(GP_STATUS_REG_227E); > + outw(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E); > + } else { > + /* enable SAFE_EN_N on PCH D1600 */ > + resetbit = ioread16(wd_reset_base_addr); > + iowrite16(resetbit & ~SAFE_EN_N_427E, > wd_reset_base_addr); > + } > +} > + > +static int wd_setup(u32 wdtmode) > +{ > + unsigned int bootstatus = 0; > + int timeout_idx; > + > + timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table, > + ARRAY_SIZE(wd_timeout_table)); > + > + if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED) > + bootstatus |= WDIOF_CARDRESET; > + > + /* reset alarm bit, set macro mode, and set timeout */ > + outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3, > WD_ENABLE_IOADR); + > + wd_secondary_enable(wdtmode); > + > + return bootstatus; > +} > + > +static struct watchdog_device wdd_data = { > + .info = &wdt_ident, > + .ops = &wdt_ops, > + .min_timeout = TIMEOUT_MIN, > + .max_timeout = TIMEOUT_MAX > +}; > + > +static int simatic_ipc_wdt_probe(struct platform_device *pdev) > +{ > + struct simatic_ipc_platform *plat = pdev->dev.platform_data; > + struct device *dev = &pdev->dev; > + struct resource *res; > + > + 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_data.parent = dev; > + 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 PTR_ERR(wd_reset_base_addr); > + } > + > + wdd_data.bootstatus = wd_setup(plat->devmode); > + if (wdd_data.bootstatus) > + dev_warn(dev, "last reboot caused by watchdog > reset\n"); + > + watchdog_set_nowayout(&wdd_data, nowayout); > + watchdog_stop_on_reboot(&wdd_data); > + return devm_watchdog_register_device(dev, &wdd_data); > +} > + > +static struct platform_driver simatic_ipc_wdt_driver = { > + .probe = simatic_ipc_wdt_probe, > + .driver = { > + .name = KBUILD_MODNAME, > + }, > +}; > + > +module_platform_driver(simatic_ipc_wdt_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
Am Mon, 29 Mar 2021 19:49:27 +0200 schrieb 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: Henning Schild <henning.schild@siemens.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/Kconfig | 11 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/simatic-ipc-wdt.c | 215 > +++++++++++++++++++++++++++++ 3 files changed, 227 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..e901718d05b9 > --- /dev/null > +++ b/drivers/watchdog/simatic-ipc-wdt.c > @@ -0,0 +1,215 @@ > +// 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> > + */ > + > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/ioport.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/platform_data/x86/simatic-ipc-base.h> > +#include <linux/platform_device.h> > +#include <linux/sizes.h> > +#include <linux/util_macros.h> > +#include <linux/watchdog.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 WD_TRIGGERED 0x80 > +#define WD_MACROMODE 0x02 > + > +#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 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 wd_start(struct watchdog_device *wdd) > +{ > + outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR); > + return 0; > +} > + > +static int wd_stop(struct watchdog_device *wdd) > +{ > + outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR); > + 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) +{ > + int timeout_idx = find_closest(t, wd_timeout_table, > + ARRAY_SIZE(wd_timeout_table)); > + > + outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3, > WD_ENABLE_IOADR); > + 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_secondary_enable(u32 wdtmode) > +{ > + u16 resetbit; > + > + /* set safe_en_n so we are not just WDIOF_ALARMONLY */ > + if (wdtmode == SIMATIC_IPC_DEVICE_227E) { > + /* enable SAFE_EN_N on GP_STATUS_REG_227E */ > + resetbit = inw(GP_STATUS_REG_227E); > + outw(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E); Should be inb/outb we just have an SZ_1 region. Henning > + } else { > + /* enable SAFE_EN_N on PCH D1600 */ > + resetbit = ioread16(wd_reset_base_addr); > + iowrite16(resetbit & ~SAFE_EN_N_427E, > wd_reset_base_addr); > + } > +} > + > +static int wd_setup(u32 wdtmode) > +{ > + unsigned int bootstatus = 0; > + int timeout_idx; > + > + timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table, > + ARRAY_SIZE(wd_timeout_table)); > + > + if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED) > + bootstatus |= WDIOF_CARDRESET; > + > + /* reset alarm bit, set macro mode, and set timeout */ > + outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3, > WD_ENABLE_IOADR); + > + wd_secondary_enable(wdtmode); > + > + return bootstatus; > +} > + > +static struct watchdog_device wdd_data = { > + .info = &wdt_ident, > + .ops = &wdt_ops, > + .min_timeout = TIMEOUT_MIN, > + .max_timeout = TIMEOUT_MAX > +}; > + > +static int simatic_ipc_wdt_probe(struct platform_device *pdev) > +{ > + struct simatic_ipc_platform *plat = pdev->dev.platform_data; > + struct device *dev = &pdev->dev; > + struct resource *res; > + > + 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_data.parent = dev; > + 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 PTR_ERR(wd_reset_base_addr); > + } > + > + wdd_data.bootstatus = wd_setup(plat->devmode); > + if (wdd_data.bootstatus) > + dev_warn(dev, "last reboot caused by watchdog > reset\n"); + > + watchdog_set_nowayout(&wdd_data, nowayout); > + watchdog_stop_on_reboot(&wdd_data); > + return devm_watchdog_register_device(dev, &wdd_data); > +} > + > +static struct platform_driver simatic_ipc_wdt_driver = { > + .probe = simatic_ipc_wdt_probe, > + .driver = { > + .name = KBUILD_MODNAME, > + }, > +}; > + > +module_platform_driver(simatic_ipc_wdt_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
Am Wed, 7 Apr 2021 05:17:12 -0700 schrieb Guenter Roeck <linux@roeck-us.net>: > On 4/7/21 1:53 AM, Andy Shevchenko wrote: > > On Tue, Apr 6, 2021 at 5:56 PM Henning Schild > > <henning.schild@siemens.com> wrote: > >> > >> Am Thu, 1 Apr 2021 18:15:41 +0200 > >> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > >> > >>> On 29.03.21 19:49, Henning Schild wrote: > >>> > >>> Hi, > >>> > >>>> This driver adds initial support for several devices from > >>>> Siemens. It is based on a platform driver introduced in an > >>>> earlier commit. > >>> > >>> Where does the wdt actually come from ? > >>> > >>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a > >>> pretty usual case. > >>> > >>> Or some external chip ? > >> > >> I guess external chip, but again we are talking about multiple > >> machines. And the manuals i read so far do not go into that sort of > >> detail. In fact on some of the machines you will have two > >> watchdogs, one from the SoC and that "special" one. > >> That has several reasons, probably not too important here. The HW > >> guys are adding another wd not just for fun, and it would be nice > >> to have a driver. > >> > >>> The code smells a bit like two entirely different wdt's that just > >>> have some similarities. If that's the case, I'd rather split it > >>> into two separate drivers and let the parent driver (board file) > >>> instantiate the correct one. > >> > >> Yes, it is two. Just like for the LEDs. One version PIO-based > >> another version gpio/p2sb/mmio based. > > > > I tend to agree with Enrico that this should be two separate > > drivers. > > Agreed. > > Guenter I will ignore the wish for a split in v4. Reason is that it would cause a lot of duplication are spreading code over many files. like i.e. a -common.c Internally we have that driver in fact already support a few more machines, which could call a split at some point. Or could also upset people of the many CONFIG_ knobs and files as we keep pushing machine support forward in the upstream drivers. But i would like to discuss that in patch qs coming after a merge and not split (maybe not yet). Also splitting wdt and having leds in one file would be inconsistent. So when there will be a split it should be on both ends. But please allow me to postpone that. regards, Henning > >> In fact the latter should very likely be based on that gpio pinctl, > >> whether it really needs to be a separate driver will have to be > >> seen. There are probably pros and cons for both options. > > > > >
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..e901718d05b9 --- /dev/null +++ b/drivers/watchdog/simatic-ipc-wdt.c @@ -0,0 +1,215 @@ +// 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> + */ + +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/platform_data/x86/simatic-ipc-base.h> +#include <linux/platform_device.h> +#include <linux/sizes.h> +#include <linux/util_macros.h> +#include <linux/watchdog.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 WD_TRIGGERED 0x80 +#define WD_MACROMODE 0x02 + +#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 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 wd_start(struct watchdog_device *wdd) +{ + outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR); + return 0; +} + +static int wd_stop(struct watchdog_device *wdd) +{ + outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR); + 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) +{ + int timeout_idx = find_closest(t, wd_timeout_table, + ARRAY_SIZE(wd_timeout_table)); + + outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3, WD_ENABLE_IOADR); + 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_secondary_enable(u32 wdtmode) +{ + u16 resetbit; + + /* set safe_en_n so we are not just WDIOF_ALARMONLY */ + if (wdtmode == SIMATIC_IPC_DEVICE_227E) { + /* enable SAFE_EN_N on GP_STATUS_REG_227E */ + resetbit = inw(GP_STATUS_REG_227E); + outw(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E); + } else { + /* enable SAFE_EN_N on PCH D1600 */ + resetbit = ioread16(wd_reset_base_addr); + iowrite16(resetbit & ~SAFE_EN_N_427E, wd_reset_base_addr); + } +} + +static int wd_setup(u32 wdtmode) +{ + unsigned int bootstatus = 0; + int timeout_idx; + + timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table, + ARRAY_SIZE(wd_timeout_table)); + + if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED) + bootstatus |= WDIOF_CARDRESET; + + /* reset alarm bit, set macro mode, and set timeout */ + outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3, WD_ENABLE_IOADR); + + wd_secondary_enable(wdtmode); + + return bootstatus; +} + +static struct watchdog_device wdd_data = { + .info = &wdt_ident, + .ops = &wdt_ops, + .min_timeout = TIMEOUT_MIN, + .max_timeout = TIMEOUT_MAX +}; + +static int simatic_ipc_wdt_probe(struct platform_device *pdev) +{ + struct simatic_ipc_platform *plat = pdev->dev.platform_data; + struct device *dev = &pdev->dev; + struct resource *res; + + 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_data.parent = dev; + 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 PTR_ERR(wd_reset_base_addr); + } + + wdd_data.bootstatus = wd_setup(plat->devmode); + if (wdd_data.bootstatus) + dev_warn(dev, "last reboot caused by watchdog reset\n"); + + watchdog_set_nowayout(&wdd_data, nowayout); + watchdog_stop_on_reboot(&wdd_data); + return devm_watchdog_register_device(dev, &wdd_data); +} + +static struct platform_driver simatic_ipc_wdt_driver = { + .probe = simatic_ipc_wdt_probe, + .driver = { + .name = KBUILD_MODNAME, + }, +}; + +module_platform_driver(simatic_ipc_wdt_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" KBUILD_MODNAME); +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");