[PATCH]: watchdog: Allow watchdog to remain enabled after probe

[PATCH]: watchdog: Allow watchdog to remain enabled after probe

[Regis Dargent]

---
The sunxi_wdt watchdog unconditionally stops the watchdog during probe (on my Allwinner H616).
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

Re: [PATCH]: watchdog: Allow watchdog to remain enabled after probe

[Andre Przywara]

Hi Regis,

first: can you say which version of U-Boot this should apply against? I 
cannot find a struct watchdog_info anywhere in the mainline source.

But more importantly:

On 03/02/2025 17:19, Regis Dargent wrote:
> ---
> The sunxi_wdt watchdog unconditionally stops the watchdog during probe (on my Allwinner H616).

This is expected. The probe routine does this to reset the watchdog.
The reason it stays disabled is CONFIG_WATCHDOG_AUTOSTART, which 
defaults to "n" on sunxi, see commit 86798ee0c148 for the reasons.
Also there were mailing list threads with more background info:

https://lore.kernel.org/u-boot/20211109220953.57b94ce8@slackpad.fritz.box/
https://lore.kernel.org/u-boot/20211109101901.24015-1-heinrich.schuchardt@canonical.com/

So long story short: if you really want to enable the watchdog in your 
particular case, you should just configure CONFIG_WATCHDOG_AUTOSTART to 
"y", in your local config.

Cheers,
Andre

> 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);

[PATCH]: watchdog: Allow watchdog to remain enabled after probe

[Regis Dargent]

---
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

Re: [PATCH]: watchdog: Allow watchdog to remain enabled after probe

[Chen-Yu Tsai]

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
>