| Message ID | 20250206093956.46593-1-regis.dargent@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | : watchdog: Allow watchdog to remain enabled after probe | expand |
On Thu, Feb 6, 2025 at 5:40 PM Regis Dargent <regis.dargent@gmail.com> wrote: Please format your commit subject correctly. See previous commits for examples. Please provide a commit message including justification for the change. The message below already makes sense, though it could be made more concise. Please also include your Signed-off-by. > --- > The sunxi_wdt watchdog unconditionally stops the watchdog during probe (on my Allwinner H616). > > What I want to achieve with this patch is to start the watchdog in the bootloader (either manually or automatically), then boot Linux. > The watchdog is about 16sec timeout maximum, while the full boot to userland lasts about 90sec, and I want the board to reset if, eg. > the rootfs cannot be mounted. So I need the watchdog to be handled by the kernel during boot (which it can do pretty well). > > The thing is, the current driver stops the watchdog during probe, so it does not run during boot, and it also does not manages the "status" > field, so the kernel would know that it must handle the HW watchdog. > This avoids automatic reboot in case a problem occurs during boot (and for example handling in the bootloader). > > The driver should detect if the HW watchdog is already running during probe and set its appropriate status bit to allow the kernel to handle the watchdog pings itself. > The call to sunxi_wdt_start/stop allows for proper driver and device configuration. > By default, the kernel will then ping the HW watchdog at apropriate frequency, but the user can have it stop after a time with open_timeout parameter. ^ appropriate Please wrap your messages under 80 characters per line. > drivers/watchdog/sunxi_wdt.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c > index b85354a99582..20fe7da445ea 100644 > --- a/drivers/watchdog/sunxi_wdt.c > +++ b/drivers/watchdog/sunxi_wdt.c > @@ -192,6 +192,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev) > return 0; > } > > +static int sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt) Make it return bool. > +{ > + u32 reg; > + void __iomem *wdt_base = wdt->wdt_base; > + const struct sunxi_wdt_reg *regs = wdt->wdt_regs; > + > + reg = readl(wdt_base + regs->wdt_mode); > + return (reg & WDT_MODE_EN); return !!(reg & WDT_MODE_EN); > +} > + > static const struct watchdog_info sunxi_wdt_info = { > .identity = DRV_NAME, > .options = WDIOF_SETTIMEOUT | > @@ -268,6 +278,11 @@ static int sunxi_wdt_probe(struct platform_device *pdev) > sunxi_wdt->wdt_dev.max_timeout = WDT_MAX_TIMEOUT; > sunxi_wdt->wdt_dev.min_timeout = WDT_MIN_TIMEOUT; > sunxi_wdt->wdt_dev.parent = dev; > + if (sunxi_wdt_enabled(sunxi_wdt)) { > + set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status); Maybe also read back the current timeout? This should be done after watchdog_init_timeout(), so ... > + } else { > + clear_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status); > + } ... you should just combine the if-else block with the one below. > > watchdog_init_timeout(&sunxi_wdt->wdt_dev, timeout, dev); > watchdog_set_nowayout(&sunxi_wdt->wdt_dev, nowayout); > @@ -275,7 +290,10 @@ static int sunxi_wdt_probe(struct platform_device *pdev) > > watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt); > > - sunxi_wdt_stop(&sunxi_wdt->wdt_dev); > + if (watchdog_hw_running(&sunxi_wdt->wdt_dev)) > + sunxi_wdt_start(&sunxi_wdt->wdt_dev); > + else > + sunxi_wdt_stop(&sunxi_wdt->wdt_dev); FWIW this is what the dw_wdt driver does as well. Thanks ChenYu > watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev); > err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev); > -- > 2.25.1 > On Thu, Feb 6, 2025 at 5:40 PM Regis Dargent <regis.dargent@gmail.com> wrote: > > --- > The sunxi_wdt watchdog unconditionally stops the watchdog during probe (on my Allwinner H616). > > What I want to achieve with this patch is to start the watchdog in the bootloader (either manually or automatically), then boot Linux. > The watchdog is about 16sec timeout maximum, while the full boot to userland lasts about 90sec, and I want the board to reset if, eg. > the rootfs cannot be mounted. So I need the watchdog to be handled by the kernel during boot (which it can do pretty well). > > The thing is, the current driver stops the watchdog during probe, so it does not run during boot, and it also does not manages the "status" > field, so the kernel would know that it must handle the HW watchdog. > This avoids automatic reboot in case a problem occurs during boot (and for example handling in the bootloader). > > The driver should detect if the HW watchdog is already running during probe and set its appropriate status bit to allow the kernel to handle the watchdog pings itself. > The call to sunxi_wdt_start/stop allows for proper driver and device configuration. > By default, the kernel will then ping the HW watchdog at apropriate frequency, but the user can have it stop after a time with open_timeout parameter. > > drivers/watchdog/sunxi_wdt.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c > index b85354a99582..20fe7da445ea 100644 > --- a/drivers/watchdog/sunxi_wdt.c > +++ b/drivers/watchdog/sunxi_wdt.c > @@ -192,6 +192,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev) > return 0; > } > > +static int sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt) > +{ > + u32 reg; > + void __iomem *wdt_base = wdt->wdt_base; > + const struct sunxi_wdt_reg *regs = wdt->wdt_regs; > + > + reg = readl(wdt_base + regs->wdt_mode); > + return (reg & WDT_MODE_EN); > +} > + > static const struct watchdog_info sunxi_wdt_info = { > .identity = DRV_NAME, > .options = WDIOF_SETTIMEOUT | > @@ -268,6 +278,11 @@ static int sunxi_wdt_probe(struct platform_device *pdev) > sunxi_wdt->wdt_dev.max_timeout = WDT_MAX_TIMEOUT; > sunxi_wdt->wdt_dev.min_timeout = WDT_MIN_TIMEOUT; > sunxi_wdt->wdt_dev.parent = dev; > + if (sunxi_wdt_enabled(sunxi_wdt)) { > + set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status); > + } else { > + clear_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status); > + } > > watchdog_init_timeout(&sunxi_wdt->wdt_dev, timeout, dev); > watchdog_set_nowayout(&sunxi_wdt->wdt_dev, nowayout); > @@ -275,7 +290,10 @@ static int sunxi_wdt_probe(struct platform_device *pdev) > > watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt); > > - sunxi_wdt_stop(&sunxi_wdt->wdt_dev); > + if (watchdog_hw_running(&sunxi_wdt->wdt_dev)) > + sunxi_wdt_start(&sunxi_wdt->wdt_dev); > + else > + sunxi_wdt_stop(&sunxi_wdt->wdt_dev); > > watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev); > err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev); > -- > 2.25.1 >
I (tried) to fix commit format (still learning, please comment is something is missing). I made the suggested code fixes and added a sunxi_wdt_read_timeout function; it was a bit more touchy, as the HW timeout could be set to less than a second (really?), so I had to manage max_hw_heartbeat_ms too.
diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c index b85354a99582..20fe7da445ea 100644 --- a/drivers/watchdog/sunxi_wdt.c +++ b/drivers/watchdog/sunxi_wdt.c @@ -192,6 +192,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev) return 0; } +static int sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt) +{ + u32 reg; + void __iomem *wdt_base = wdt->wdt_base; + const struct sunxi_wdt_reg *regs = wdt->wdt_regs; + + reg = readl(wdt_base + regs->wdt_mode); + return (reg & WDT_MODE_EN); +} + static const struct watchdog_info sunxi_wdt_info = { .identity = DRV_NAME, .options = WDIOF_SETTIMEOUT | @@ -268,6 +278,11 @@ static int sunxi_wdt_probe(struct platform_device *pdev) sunxi_wdt->wdt_dev.max_timeout = WDT_MAX_TIMEOUT; sunxi_wdt->wdt_dev.min_timeout = WDT_MIN_TIMEOUT; sunxi_wdt->wdt_dev.parent = dev; + if (sunxi_wdt_enabled(sunxi_wdt)) { + set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status); + } else { + clear_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status); + } watchdog_init_timeout(&sunxi_wdt->wdt_dev, timeout, dev); watchdog_set_nowayout(&sunxi_wdt->wdt_dev, nowayout); @@ -275,7 +290,10 @@ static int sunxi_wdt_probe(struct platform_device *pdev) watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt); - sunxi_wdt_stop(&sunxi_wdt->wdt_dev); + if (watchdog_hw_running(&sunxi_wdt->wdt_dev)) + sunxi_wdt_start(&sunxi_wdt->wdt_dev); + else + sunxi_wdt_stop(&sunxi_wdt->wdt_dev); watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev); err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev);