| Message ID | 1522250043-8065-5-git-send-email-tharvey@gateworks.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote: > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > drivers/watchdog/Kconfig | 10 ++++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 157 insertions(+) > create mode 100644 drivers/watchdog/gsc_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index ca200d1..c9d4b2e 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -150,6 +150,16 @@ config GPIO_WATCHDOG_ARCH_INITCALL > arch_initcall. > If in doubt, say N. > > +config GSC_WATCHDOG > + tristate "Gateworks System Controller (GSC) Watchdog support" > + depends on MFD_GATEWORKS_GSC > + select WATCHDOG_CORE > + help > + Say Y here to include support for the GSC Watchdog. > + > + This driver can also be built as a module. If so the module > + will be called gsc_wdt. > + > config MENF21BMC_WATCHDOG > tristate "MEN 14F021P00 BMC Watchdog" > depends on MFD_MENF21BMC || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 715a210..499327e 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -215,6 +215,7 @@ obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o > obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o > obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o > obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o > +obj-$(CONFIG_GSC_WATCHDOG) += gsc_wdt.o > obj-$(CONFIG_TANGOX_WATCHDOG) += tangox_wdt.o > obj-$(CONFIG_WDAT_WDT) += wdat_wdt.o > obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o > diff --git a/drivers/watchdog/gsc_wdt.c b/drivers/watchdog/gsc_wdt.c > new file mode 100644 > index 0000000..b43d083 > --- /dev/null > +++ b/drivers/watchdog/gsc_wdt.c > @@ -0,0 +1,146 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * Copyright (C) 2018 Gateworks Corporation > + * > + * This driver registers a Linux Watchdog for the GSC > + */ > +#include <linux/mfd/gsc.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/watchdog.h> > + > +#define WDT_DEFAULT_TIMEOUT 60 > + > +struct gsc_wdt { > + struct watchdog_device wdt_dev; > + struct gsc_dev *gsc; > +}; > + > +static int gsc_wdt_start(struct watchdog_device *wdd) > +{ > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); > + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE); Please use BIT(). > + int ret; > + > + dev_dbg(wdd->parent, "%s timeout=%d\n", __func__, wdd->timeout); I don't think those debug messages add any value. > + > + /* clear first as regmap_update_bits will not write if no change */ > + ret = regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0); > + if (ret) > + return ret; > + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, reg); > +} > + > +static int gsc_wdt_stop(struct watchdog_device *wdd) > +{ > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); > + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE); > + BIT(). You might as well drop the variable and just use BIT(GSC_CTRL_1_WDT_ENABLE) below. > + dev_dbg(wdd->parent, "%s\n", __func__); > + > + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0); > +} > + > +static int gsc_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); > + unsigned int long_sel = 0; > + > + dev_dbg(wdd->parent, "%s: %d\n", __func__, timeout); > + > + switch (timeout) { > + case 60: > + long_sel = (1 << GSC_CTRL_1_WDT_TIME); > + case 30: > + regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, > + (1 << GSC_CTRL_1_WDT_TIME), BIT() > + (long_sel << GSC_CTRL_1_WDT_TIME)); > + wdd->timeout = timeout; > + return 0; > + } Please use rounding and accept other values as well. We don't want to let user space guessing valid timeouts. > + > + return -EINVAL; > +} > + > +static const struct watchdog_info gsc_wdt_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, Please confirm that WDIOF_MAGICCLOSE is not set on purpose. > + .identity = "GSC Watchdog" > +}; > + > +static const struct watchdog_ops gsc_wdt_ops = { > + .owner = THIS_MODULE, > + .start = gsc_wdt_start, > + .stop = gsc_wdt_stop, > + .set_timeout = gsc_wdt_set_timeout, > +}; > + > +static int gsc_wdt_probe(struct platform_device *pdev) > +{ > + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); > + struct device *dev = &pdev->dev; > + struct gsc_wdt *wdt; > + int ret; > + unsigned int reg; > + > + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + /* ensure GSC fw supports WD functionality */ > + if (gsc->fwver < 44) { What if gsc is NULL ? > + dev_err(dev, "fw v44 or newer required for wdt function\n"); > + return -EINVAL; > + } > + > + /* ensure WD bit enabled */ > + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®)) > + return -EIO; > + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) { BIT() > + dev_err(dev, "not enabled - must be manually enabled\n"); This doesn't make sense. Bail out if the watchdog is disabled ? Why ? > + return -EINVAL; > + } > + > + platform_set_drvdata(pdev, wdt); > + > + wdt->gsc = gsc; > + wdt->wdt_dev.info = &gsc_wdt_info; > + wdt->wdt_dev.ops = &gsc_wdt_ops; > + wdt->wdt_dev.status = 0; Initializing a data structure which is known to be 0 is not needed and discouraged. > + wdt->wdt_dev.min_timeout = 30; > + wdt->wdt_dev.max_timeout = 60; > + wdt->wdt_dev.parent = dev; > + > + watchdog_set_nowayout(&wdt->wdt_dev, 1); WATCHDOG_NOWAYOUT ? > + watchdog_init_timeout(&wdt->wdt_dev, WDT_DEFAULT_TIMEOUT, dev); This is quite pointless. If you don't want to support setting the timeout through devicetree or through a module parameter, just set the timeout directly above. > + > + watchdog_set_drvdata(&wdt->wdt_dev, wdt); > + ret = devm_watchdog_register_device(dev, &wdt->wdt_dev); > + if (ret) > + return ret; > + > + dev_info(dev, "watchdog driver (timeout=%d sec)\n", > + wdt->wdt_dev.timeout); > + > + return 0; > +} > + > +static const struct of_device_id gsc_wdt_dt_ids[] = { > + { .compatible = "gw,gsc-watchdog", }, > + {} > +}; > + > +static struct platform_driver gsc_wdt_driver = { > + .probe = gsc_wdt_probe, > + .driver = { > + .name = "gsc-wdt", > + .of_match_table = gsc_wdt_dt_ids, > + }, > +}; > + > +module_platform_driver(gsc_wdt_driver); > + > +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>"); > +MODULE_DESCRIPTION("Gateworks System Controller Watchdog driver"); > +MODULE_LICENSE("GPL v2"); -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 29, 2018 at 06:07:57PM -0700, Guenter Roeck wrote: > On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote: > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > --- > > drivers/watchdog/Kconfig | 10 ++++ > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 157 insertions(+) > > create mode 100644 drivers/watchdog/gsc_wdt.c > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > index ca200d1..c9d4b2e 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -150,6 +150,16 @@ config GPIO_WATCHDOG_ARCH_INITCALL > > arch_initcall. > > If in doubt, say N. > > > > +config GSC_WATCHDOG > > + tristate "Gateworks System Controller (GSC) Watchdog support" > > + depends on MFD_GATEWORKS_GSC > > + select WATCHDOG_CORE > > + help > > + Say Y here to include support for the GSC Watchdog. > > + > > + This driver can also be built as a module. If so the module > > + will be called gsc_wdt. > > + > > config MENF21BMC_WATCHDOG > > tristate "MEN 14F021P00 BMC Watchdog" > > depends on MFD_MENF21BMC || COMPILE_TEST > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > index 715a210..499327e 100644 > > --- a/drivers/watchdog/Makefile > > +++ b/drivers/watchdog/Makefile > > @@ -215,6 +215,7 @@ obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o > > obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o > > obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o > > obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o > > +obj-$(CONFIG_GSC_WATCHDOG) += gsc_wdt.o > > obj-$(CONFIG_TANGOX_WATCHDOG) += tangox_wdt.o > > obj-$(CONFIG_WDAT_WDT) += wdat_wdt.o > > obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o > > diff --git a/drivers/watchdog/gsc_wdt.c b/drivers/watchdog/gsc_wdt.c > > new file mode 100644 > > index 0000000..b43d083 > > --- /dev/null > > +++ b/drivers/watchdog/gsc_wdt.c > > @@ -0,0 +1,146 @@ > > +/* SPDX-License-Identifier: GPL-2.0 > > + * > > + * Copyright (C) 2018 Gateworks Corporation > > + * > > + * This driver registers a Linux Watchdog for the GSC > > + */ > > +#include <linux/mfd/gsc.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/watchdog.h> > > + > > +#define WDT_DEFAULT_TIMEOUT 60 > > + > > +struct gsc_wdt { > > + struct watchdog_device wdt_dev; > > + struct gsc_dev *gsc; > > +}; > > + > > +static int gsc_wdt_start(struct watchdog_device *wdd) > > +{ > > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); > > + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE); > > Please use BIT(). > > > + int ret; > > + > > + dev_dbg(wdd->parent, "%s timeout=%d\n", __func__, wdd->timeout); > > I don't think those debug messages add any value. > > > + > > + /* clear first as regmap_update_bits will not write if no change */ > > + ret = regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0); > > + if (ret) > > + return ret; > > + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, reg); > > +} > > + > > +static int gsc_wdt_stop(struct watchdog_device *wdd) > > +{ > > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); > > + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE); > > + > > BIT(). You might as well drop the variable and just use > BIT(GSC_CTRL_1_WDT_ENABLE) below. > > > + dev_dbg(wdd->parent, "%s\n", __func__); > > + > > + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0); > > +} > > + > > +static int gsc_wdt_set_timeout(struct watchdog_device *wdd, > > + unsigned int timeout) > > +{ > > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); > > + unsigned int long_sel = 0; > > + > > + dev_dbg(wdd->parent, "%s: %d\n", __func__, timeout); > > + > > + switch (timeout) { > > + case 60: > > + long_sel = (1 << GSC_CTRL_1_WDT_TIME); > > + case 30: > > + regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, > > + (1 << GSC_CTRL_1_WDT_TIME), > > BIT() > > > + (long_sel << GSC_CTRL_1_WDT_TIME)); This looks wrong. Are you sure that in case of 60 timeout you want to actually write ((1 << GSC_CTRL_1_WDT_TIME) << GSC_CTRL_1_WDT_TIME) to the register? Or you meant to write: long_sel = timeout > 30; regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, (long_sel << GSC_CTRL_1_WDT_TIME), Thanks.
On Thu, Mar 29, 2018 at 6:07 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote: >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> --- >> drivers/watchdog/Kconfig | 10 ++++ >> drivers/watchdog/Makefile | 1 + >> drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 157 insertions(+) >> create mode 100644 drivers/watchdog/gsc_wdt.c >> <snip> >> + >> +static const struct watchdog_info gsc_wdt_info = { >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > > Please confirm that WDIOF_MAGICCLOSE is not set on purpose. > >> + .identity = "GSC Watchdog" >> +}; >> + <snip> >> + >> +static int gsc_wdt_probe(struct platform_device *pdev) >> +{ >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); >> + struct device *dev = &pdev->dev; >> + struct gsc_wdt *wdt; >> + int ret; >> + unsigned int reg; >> + <snip> >> + /* ensure WD bit enabled */ >> + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®)) >> + return -EIO; >> + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) { > > BIT() > >> + dev_err(dev, "not enabled - must be manually enabled\n"); > > This doesn't make sense. Bail out if the watchdog is disabled ? Why ? > >> + return -EINVAL; >> + } >> + <snip> >> + >> + watchdog_set_nowayout(&wdt->wdt_dev, 1); > > WATCHDOG_NOWAYOUT ? > Guenter, Thanks for the review! The watchdog implementation of the GSC is such that it is enabled and reset via a single non-volatile I2C register bit. If this bit is set the watchdog will start ticking down automatically on board power up. The register definitions don't provide a condition where it can be enabled in a volatile way such that after board power-cycle it is disabled again nor do they provide a separate register for enable vs reset. In the typical case the user boots the board, driver registers watchdog, userspace watchdog daemon enables watchdog and it starts ticking. User now powers down the board and later powers it back up. The watchdog was enabled previously by userspace and the register is non-volatile so the watchdog starts ticking before the kernel driver and watchdog daemon yet the user breaks out into the bootloader or boots a different OS without a watchdog daemon and the board resets without them expecting it. The feature that the watchdog starts ticking at board power-up before the CPU has even fetched code was part of its design and was put there to work around some SoC errata that can cause the CPU to fail to fetch boot code. This has caused me to implement a watchdog driver that never actually 'enables' or 'disables' the watchdog which is why there is no MAGIC CLOSE and why I always set nowayout. Its possible this is a fairly unique case of a watchdog. The probe failure if the watchdog isn't enabled is because I don't want a non-enabled watchdog to get enabled just because the driver/daemon were there. I agree it's a very strange behavior and I'm not sure how to best document or support it with the Linux watchdog API. I welcome any recomendations! Regards, Tim -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 30, 2018 at 10:49:38AM -0700, Tim Harvey wrote: > On Thu, Mar 29, 2018 at 6:07 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote: > >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> > >> --- > >> drivers/watchdog/Kconfig | 10 ++++ > >> drivers/watchdog/Makefile | 1 + > >> drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 157 insertions(+) > >> create mode 100644 drivers/watchdog/gsc_wdt.c > >> > <snip> > >> + > >> +static const struct watchdog_info gsc_wdt_info = { > >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > > > > Please confirm that WDIOF_MAGICCLOSE is not set on purpose. > > > >> + .identity = "GSC Watchdog" > >> +}; > >> + > <snip> > >> + > >> +static int gsc_wdt_probe(struct platform_device *pdev) > >> +{ > >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); > >> + struct device *dev = &pdev->dev; > >> + struct gsc_wdt *wdt; > >> + int ret; > >> + unsigned int reg; > >> + > <snip> > >> + /* ensure WD bit enabled */ > >> + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®)) > >> + return -EIO; > >> + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) { > > > > BIT() > > > >> + dev_err(dev, "not enabled - must be manually enabled\n"); > > > > This doesn't make sense. Bail out if the watchdog is disabled ? Why ? > > > >> + return -EINVAL; > >> + } > >> + > <snip> > >> + > >> + watchdog_set_nowayout(&wdt->wdt_dev, 1); > > > > WATCHDOG_NOWAYOUT ? > > > > Guenter, > > Thanks for the review! > > The watchdog implementation of the GSC is such that it is enabled and > reset via a single non-volatile I2C register bit. If this bit is set > the watchdog will start ticking down automatically on board power up. > The register definitions don't provide a condition where it can be > enabled in a volatile way such that after board power-cycle it is > disabled again nor do they provide a separate register for enable vs > reset. > > In the typical case the user boots the board, driver registers > watchdog, userspace watchdog daemon enables watchdog and it starts > ticking. User now powers down the board and later powers it back up. > The watchdog was enabled previously by userspace and the register is > non-volatile so the watchdog starts ticking before the kernel driver > and watchdog daemon yet the user breaks out into the bootloader or > boots a different OS without a watchdog daemon and the board resets > without them expecting it. The feature that the watchdog starts > ticking at board power-up before the CPU has even fetched code was > part of its design and was put there to work around some SoC errata > that can cause the CPU to fail to fetch boot code. This has caused me > to implement a watchdog driver that never actually 'enables' or > 'disables' the watchdog which is why there is no MAGIC CLOSE and why I Yet the driver does enable and disable the watchdog in its start and stop functions. And I have no idea what that has to do with the MAGICCLOSE functionality, which is quite orthogonal to the start/stop functionality. > always set nowayout. Its possible this is a fairly unique case of a > watchdog. The probe failure if the watchdog isn't enabled is because I > don't want a non-enabled watchdog to get enabled just because the > driver/daemon were there. > Huh ? The whole purpose of a watchdog is for it to be enabled when the watchdog device is opened. > I agree it's a very strange behavior and I'm not sure how to best > document or support it with the Linux watchdog API. I welcome any > recomendations! > Sorry, I fail to understand your logic. You do not explain why your code bails out if the watchdog is not already running. That does not make sense. You are saying that you don't want the watchdog driver to enable the watchdog. Since its whole purpose is to enable the watchdog if/when the watchdog device is opened, that doesn't make sense either. At the same time, you do not tell the watchdog core that the watchdog is already running, meaning the system _will_ reboot unless the watchdog device is opened within the watchdog timeout period. Again, that does not make sense. Maybe it all makes sense to you, but not to me, sorry. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 30, 2018 at 11:19 AM, Guenter Roeck <linux@roeck-us.net> wrote: > On Fri, Mar 30, 2018 at 10:49:38AM -0700, Tim Harvey wrote: >> On Thu, Mar 29, 2018 at 6:07 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> > On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote: >> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> >> --- >> >> drivers/watchdog/Kconfig | 10 ++++ >> >> drivers/watchdog/Makefile | 1 + >> >> drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 157 insertions(+) >> >> create mode 100644 drivers/watchdog/gsc_wdt.c >> >> >> <snip> >> >> + >> >> +static const struct watchdog_info gsc_wdt_info = { >> >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, >> > >> > Please confirm that WDIOF_MAGICCLOSE is not set on purpose. >> > >> >> + .identity = "GSC Watchdog" >> >> +}; >> >> + >> <snip> >> >> + >> >> +static int gsc_wdt_probe(struct platform_device *pdev) >> >> +{ >> >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); >> >> + struct device *dev = &pdev->dev; >> >> + struct gsc_wdt *wdt; >> >> + int ret; >> >> + unsigned int reg; >> >> + >> <snip> >> >> + /* ensure WD bit enabled */ >> >> + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®)) >> >> + return -EIO; >> >> + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) { >> > >> > BIT() >> > >> >> + dev_err(dev, "not enabled - must be manually enabled\n"); >> > >> > This doesn't make sense. Bail out if the watchdog is disabled ? Why ? >> > >> >> + return -EINVAL; >> >> + } >> >> + >> <snip> >> >> + >> >> + watchdog_set_nowayout(&wdt->wdt_dev, 1); >> > >> > WATCHDOG_NOWAYOUT ? >> > >> >> Guenter, >> >> Thanks for the review! >> >> The watchdog implementation of the GSC is such that it is enabled and >> reset via a single non-volatile I2C register bit. If this bit is set >> the watchdog will start ticking down automatically on board power up. >> The register definitions don't provide a condition where it can be >> enabled in a volatile way such that after board power-cycle it is >> disabled again nor do they provide a separate register for enable vs >> reset. >> >> In the typical case the user boots the board, driver registers >> watchdog, userspace watchdog daemon enables watchdog and it starts >> ticking. User now powers down the board and later powers it back up. >> The watchdog was enabled previously by userspace and the register is >> non-volatile so the watchdog starts ticking before the kernel driver >> and watchdog daemon yet the user breaks out into the bootloader or >> boots a different OS without a watchdog daemon and the board resets >> without them expecting it. The feature that the watchdog starts >> ticking at board power-up before the CPU has even fetched code was >> part of its design and was put there to work around some SoC errata >> that can cause the CPU to fail to fetch boot code. This has caused me >> to implement a watchdog driver that never actually 'enables' or >> 'disables' the watchdog which is why there is no MAGIC CLOSE and why I > > Yet the driver does enable and disable the watchdog in its start and stop > functions. And I have no idea what that has to do with the MAGICCLOSE > functionality, which is quite orthogonal to the start/stop functionality. > >> always set nowayout. Its possible this is a fairly unique case of a >> watchdog. The probe failure if the watchdog isn't enabled is because I >> don't want a non-enabled watchdog to get enabled just because the >> driver/daemon were there. >> > Huh ? The whole purpose of a watchdog is for it to be enabled when > the watchdog device is opened. > >> I agree it's a very strange behavior and I'm not sure how to best >> document or support it with the Linux watchdog API. I welcome any >> recomendations! >> > > Sorry, I fail to understand your logic. > > You do not explain why your code bails out if the watchdog is not already > running. That does not make sense. > > You are saying that you don't want the watchdog driver to enable the watchdog. > Since its whole purpose is to enable the watchdog if/when the watchdog device > is opened, that doesn't make sense either. > > At the same time, you do not tell the watchdog core that the watchdog is > already running, meaning the system _will_ reboot unless the watchdog > device is opened within the watchdog timeout period. Again, that does not > make sense. > > Maybe it all makes sense to you, but not to me, sorry. Guenter, Right, I'm likely not explaining it well. Let me show the registers and describe the feature from the GSC perspective: I2C registers: non-volatile registers (battery backed) 0x01: GSC_CTRL_1: Sleep Wakeup Timer Control bit 4: WATCHDOG_TIME: 0=30 second timeout, 1=60 second timeout bit 5: WATCHDOG_ENABLE: 0=disable watchdog, 1=enable/reset watchdog timer The GSC has the ability to enable/disable the primary board power supply. In the event that the watchdog timer is enabled and reaches 0 it will power cycle the board by disabling the primary power supply for 1 second then enabling it again. The GSC_CTRL_1 bits retain their state during power cycles thus if WATCHDOG_ENABLE=1 and the board power cycles the watchdog starts counting down immediately and host software must either disable it or start resetting it before the timeout period. The 'use case' we have been using this in for a couple years is that users who want to use this watchdog will enable it externally (we have a command in the bootloader) and if enabled the kernel driver (that I'm proposing here which we've been using out-of-tree) will register the watchdog device and the userspace watchdog process can open the device and start tickling it. If the watchdog is never enabled (or disabled via the bootloader command) the kernel driver fails to probe and the SoC's watchdog can be used. The reason this feature was added to the GSC is that we had some errata with one of the SoC's we use such that it's internal reset was not resetting enough of the chip and in some cases we also wanted an external PMIC to be reset as well which is accomplished by cycling the primary power supply. What I'm proposing here is a watchdog driver that only has the ability to 'reset' the watchdog timer 'if enabled'. Because the same register is used to enable as well as reset the timer I don't want to enable/reset it if it isn't already enabled. Because start/stop are mandatory I suppose I could make stop a nop and make start only set WATCHDOG_TIME if it's already set. I was thinking that I would simply have the driver probe fail if the watchdog wasn't externally already enabled and let /dev/watchdog0 be the sometimes lesser valued SoC watchdog. Regards, Tim -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> The 'use case' we have been using this in for a couple years is that > users who want to use this watchdog will enable it externally (we have > a command in the bootloader) and if enabled the kernel driver (that > I'm proposing here which we've been using out-of-tree) will register > the watchdog device and the userspace watchdog process can open the > device and start tickling it. If the watchdog is never enabled (or > disabled via the bootloader command) the kernel driver fails to probe > and the SoC's watchdog can be used. Hi Tim Is there any reason not to give the user the choice to use both watchdogs? Normally you write drivers to expose the hardware, and then let the users choice if they want to use it. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 2, 2018 at 9:32 AM, Andrew Lunn <andrew@lunn.ch> wrote: >> The 'use case' we have been using this in for a couple years is that >> users who want to use this watchdog will enable it externally (we have >> a command in the bootloader) and if enabled the kernel driver (that >> I'm proposing here which we've been using out-of-tree) will register >> the watchdog device and the userspace watchdog process can open the >> device and start tickling it. If the watchdog is never enabled (or >> disabled via the bootloader command) the kernel driver fails to probe >> and the SoC's watchdog can be used. > > Hi Tim > > Is there any reason not to give the user the choice to use both > watchdogs? Normally you write drivers to expose the hardware, and then > let the users choice if they want to use it. > Andrew, I don't know what value the SoC watchdog has if you have the GSC watchdog available that can power cycle the board but I would agree normally you would expose all of them. I suppose this may be a different case because I'm expecting the user to go and manually enable the watchdog instead of using the kernel to do it which means if its not enabled its not really available to be used. Tim -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index ca200d1..c9d4b2e 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -150,6 +150,16 @@ config GPIO_WATCHDOG_ARCH_INITCALL arch_initcall. If in doubt, say N. +config GSC_WATCHDOG + tristate "Gateworks System Controller (GSC) Watchdog support" + depends on MFD_GATEWORKS_GSC + select WATCHDOG_CORE + help + Say Y here to include support for the GSC Watchdog. + + This driver can also be built as a module. If so the module + will be called gsc_wdt. + config MENF21BMC_WATCHDOG tristate "MEN 14F021P00 BMC Watchdog" depends on MFD_MENF21BMC || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 715a210..499327e 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -215,6 +215,7 @@ obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o +obj-$(CONFIG_GSC_WATCHDOG) += gsc_wdt.o obj-$(CONFIG_TANGOX_WATCHDOG) += tangox_wdt.o obj-$(CONFIG_WDAT_WDT) += wdat_wdt.o obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o diff --git a/drivers/watchdog/gsc_wdt.c b/drivers/watchdog/gsc_wdt.c new file mode 100644 index 0000000..b43d083 --- /dev/null +++ b/drivers/watchdog/gsc_wdt.c @@ -0,0 +1,146 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (C) 2018 Gateworks Corporation + * + * This driver registers a Linux Watchdog for the GSC + */ +#include <linux/mfd/gsc.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/watchdog.h> + +#define WDT_DEFAULT_TIMEOUT 60 + +struct gsc_wdt { + struct watchdog_device wdt_dev; + struct gsc_dev *gsc; +}; + +static int gsc_wdt_start(struct watchdog_device *wdd) +{ + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE); + int ret; + + dev_dbg(wdd->parent, "%s timeout=%d\n", __func__, wdd->timeout); + + /* clear first as regmap_update_bits will not write if no change */ + ret = regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0); + if (ret) + return ret; + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, reg); +} + +static int gsc_wdt_stop(struct watchdog_device *wdd) +{ + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE); + + dev_dbg(wdd->parent, "%s\n", __func__); + + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0); +} + +static int gsc_wdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd); + unsigned int long_sel = 0; + + dev_dbg(wdd->parent, "%s: %d\n", __func__, timeout); + + switch (timeout) { + case 60: + long_sel = (1 << GSC_CTRL_1_WDT_TIME); + case 30: + regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, + (1 << GSC_CTRL_1_WDT_TIME), + (long_sel << GSC_CTRL_1_WDT_TIME)); + wdd->timeout = timeout; + return 0; + } + + return -EINVAL; +} + +static const struct watchdog_info gsc_wdt_info = { + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, + .identity = "GSC Watchdog" +}; + +static const struct watchdog_ops gsc_wdt_ops = { + .owner = THIS_MODULE, + .start = gsc_wdt_start, + .stop = gsc_wdt_stop, + .set_timeout = gsc_wdt_set_timeout, +}; + +static int gsc_wdt_probe(struct platform_device *pdev) +{ + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); + struct device *dev = &pdev->dev; + struct gsc_wdt *wdt; + int ret; + unsigned int reg; + + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); + if (!wdt) + return -ENOMEM; + + /* ensure GSC fw supports WD functionality */ + if (gsc->fwver < 44) { + dev_err(dev, "fw v44 or newer required for wdt function\n"); + return -EINVAL; + } + + /* ensure WD bit enabled */ + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®)) + return -EIO; + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) { + dev_err(dev, "not enabled - must be manually enabled\n"); + return -EINVAL; + } + + platform_set_drvdata(pdev, wdt); + + wdt->gsc = gsc; + wdt->wdt_dev.info = &gsc_wdt_info; + wdt->wdt_dev.ops = &gsc_wdt_ops; + wdt->wdt_dev.status = 0; + wdt->wdt_dev.min_timeout = 30; + wdt->wdt_dev.max_timeout = 60; + wdt->wdt_dev.parent = dev; + + watchdog_set_nowayout(&wdt->wdt_dev, 1); + watchdog_init_timeout(&wdt->wdt_dev, WDT_DEFAULT_TIMEOUT, dev); + + watchdog_set_drvdata(&wdt->wdt_dev, wdt); + ret = devm_watchdog_register_device(dev, &wdt->wdt_dev); + if (ret) + return ret; + + dev_info(dev, "watchdog driver (timeout=%d sec)\n", + wdt->wdt_dev.timeout); + + return 0; +} + +static const struct of_device_id gsc_wdt_dt_ids[] = { + { .compatible = "gw,gsc-watchdog", }, + {} +}; + +static struct platform_driver gsc_wdt_driver = { + .probe = gsc_wdt_probe, + .driver = { + .name = "gsc-wdt", + .of_match_table = gsc_wdt_dt_ids, + }, +}; + +module_platform_driver(gsc_wdt_driver); + +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>"); +MODULE_DESCRIPTION("Gateworks System Controller Watchdog driver"); +MODULE_LICENSE("GPL v2");
Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/watchdog/Kconfig | 10 ++++ drivers/watchdog/Makefile | 1 + drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 drivers/watchdog/gsc_wdt.c