: watchdog: Allow watchdog to remain enabled after probe

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

Posted by Regis Dargent 10 months, 1 week ago

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

Posted by Chen-Yu Tsai 10 months, 1 week ago

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
>

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

Posted by Regis Dargent 10 months ago

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.

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

Posted by Regis Dargent 10 months ago

If the watchdog is already running during probe, let it run on, read its
configured timeout, and set its status so that it is correctly handled by the
kernel.

Signed-off-by: Regis Dargent <regis.dargent@gmail.com>
---
 drivers/watchdog/sunxi_wdt.c | 48 +++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index b85354a99582..0094bcd063c5 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -140,6 +140,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
 		timeout++;
 
 	sunxi_wdt->wdt_dev.timeout = timeout;
+	sunxi_wdt->wdt_dev.max_hw_heartbeat_ms = 0;
 
 	reg = readl(wdt_base + regs->wdt_mode);
 	reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
@@ -152,6 +153,32 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
 	return 0;
 }
 
+static int sunxi_wdt_read_timeout(struct watchdog_device *wdt_dev)
+{
+	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
+	void __iomem *wdt_base = sunxi_wdt->wdt_base;
+	const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
+	int i;
+	u32 reg;
+
+	reg = readl(wdt_base + regs->wdt_mode);
+	reg >>= regs->wdt_timeout_shift;
+	reg &= WDT_TIMEOUT_MASK;
+
+	/* Start at 0 which actually means 0.5s */
+	for (i = 0; ((i < WDT_MAX_TIMEOUT) && (wdt_timeout_map[i] != reg)); i++)
+		;
+	if (i == 0) {
+		wdt_dev->timeout = 1;
+		wdt_dev->max_hw_heartbeat_ms = 500;
+	} else {
+		wdt_dev->timeout = i;
+		wdt_dev->max_hw_heartbeat_ms = 0;
+	}
+
+	return 0;
+}
+
 static int sunxi_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
@@ -192,11 +219,22 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static bool 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 |
 			  WDIOF_KEEPALIVEPING |
-			  WDIOF_MAGICCLOSE,
+			  WDIOF_MAGICCLOSE |
+			  WDIOF_SETTIMEOUT,
 };
 
 static const struct watchdog_ops sunxi_wdt_ops = {
@@ -275,8 +313,12 @@ 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 (sunxi_wdt_enabled(sunxi_wdt)) {
+		sunxi_wdt_read_timeout(&sunxi_wdt->wdt_dev);
+		set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
+	} 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);
 	if (unlikely(err))
-- 
2.25.1

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

Posted by Jernej Škrabec 9 months, 3 weeks ago

Hi,

please send new versions as separate e-mail thread.

Dne petek, 14. februar 2025 ob 12:22:54 Srednjeevropski standardni čas je Regis Dargent napisal(a):
> If the watchdog is already running during probe, let it run on, read its
> configured timeout, and set its status so that it is correctly handled by the
> kernel.
> 
> Signed-off-by: Regis Dargent <regis.dargent@gmail.com>
> ---
>  drivers/watchdog/sunxi_wdt.c | 48 +++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 3 deletions(-)

You should add changelog here.

> 
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index b85354a99582..0094bcd063c5 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -140,6 +140,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>  		timeout++;
>  
>  	sunxi_wdt->wdt_dev.timeout = timeout;
> +	sunxi_wdt->wdt_dev.max_hw_heartbeat_ms = 0;
>  
>  	reg = readl(wdt_base + regs->wdt_mode);
>  	reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> @@ -152,6 +153,32 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>  	return 0;
>  }
>  
> +static int sunxi_wdt_read_timeout(struct watchdog_device *wdt_dev)
> +{
> +	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> +	void __iomem *wdt_base = sunxi_wdt->wdt_base;
> +	const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> +	int i;
> +	u32 reg;
> +
> +	reg = readl(wdt_base + regs->wdt_mode);
> +	reg >>= regs->wdt_timeout_shift;
> +	reg &= WDT_TIMEOUT_MASK;
> +
> +	/* Start at 0 which actually means 0.5s */
> +	for (i = 0; ((i < WDT_MAX_TIMEOUT) && (wdt_timeout_map[i] != reg)); i++)

Drop parenthesis in condition.

> +		;
> +	if (i == 0) {
> +		wdt_dev->timeout = 1;
> +		wdt_dev->max_hw_heartbeat_ms = 500;
> +	} else {
> +		wdt_dev->timeout = i;
> +		wdt_dev->max_hw_heartbeat_ms = 0;
> +	}
> +
> +	return 0;
> +}
> +
>  static int sunxi_wdt_stop(struct watchdog_device *wdt_dev)
>  {
>  	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> @@ -192,11 +219,22 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
>  	return 0;
>  }
>  
> +static bool 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;

Inverse christmas tree.

> +
> +	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 |
>  			  WDIOF_KEEPALIVEPING |
> -			  WDIOF_MAGICCLOSE,
> +			  WDIOF_MAGICCLOSE |
> +			  WDIOF_SETTIMEOUT,

Drop this change. WDIOF_SETTIMEOUT is already defined.

>  };
>  
>  static const struct watchdog_ops sunxi_wdt_ops = {
> @@ -275,8 +313,12 @@ 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 (sunxi_wdt_enabled(sunxi_wdt)) {

Also check for IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED). This will
allow user to select behaviour via Kconfig.

Best regards,
Jernej

> +		sunxi_wdt_read_timeout(&sunxi_wdt->wdt_dev);
> +		set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
> +	} 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);
>  	if (unlikely(err))
>