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

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

[Regis Dargent]

Hi,

V3 of the patch allowing sunxi watchdog to remain enabled after probe.
This 3rd version is mainly about code cleanup.

Best regards,
Regis

Jernej Škrabec:
  I wouldn't add checks on CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED here.
  My understanding is that this config is already managed in the generic
  watchdog part, as long as the WDOG_HW_RUNNING bit matches the actual
  state of the HW watchdog timer.
  I only saw one reference in a HW driver, stm32_iwdg, where they cannot
  read the watchdog state, so they start it and set the WDOG_HW_RUNNING
  bit so that both match.

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

[Regis Dargent]

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>

--

Changelog v1..v2:
- add sunxi_wdt_read_timeout function
- add signed-off-by tag

Changelog v2..v3:
- WDIOF_SETTIMEOUT was set twice, and other code cleanup
---
 drivers/watchdog/sunxi_wdt.c | 45 ++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index b85354a99582..d509dbcb77ce 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,6 +219,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static bool sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt)
+{
+	void __iomem *wdt_base = wdt->wdt_base;
+	const struct sunxi_wdt_reg *regs = wdt->wdt_regs;
+	u32 reg;
+
+	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 |
@@ -275,8 +312,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 v3] watchdog: sunxi_wdt: Allow watchdog to remain enabled after probe

[Guenter Roeck]

On 2/25/25 06:36, Regis Dargent wrote:
> 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>
> 
> --
> 
> Changelog v1..v2:
> - add sunxi_wdt_read_timeout function
> - add signed-off-by tag
> 
> Changelog v2..v3:
> - WDIOF_SETTIMEOUT was set twice, and other code cleanup
> ---
>   drivers/watchdog/sunxi_wdt.c | 45 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index b85354a99582..d509dbcb77ce 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++)

Unnecessary (). On top of that, its complexity is unnecessary.
The timeout in seconds, except for reg == 0, is wdt_timeout_map[reg],
with values of 0x0c..0x0f undefined. Worse, the above code can access
beyond the size of wdt_timeout_map[] if reg >= 0x0c.

> +		;
> +	if (i == 0) {
> +		wdt_dev->timeout = 1;
> +		wdt_dev->max_hw_heartbeat_ms = 500;

This is an unacceptable API abuse. max_hw_heartbeat_ms, if set,
should be 16000, not 500. You could set the timeout to 1 second instead.

Guenter

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

[Régis DARGENT]

Le mar. 25 févr. 2025 à 22:51, Guenter Roeck <linux@roeck-us.net> a écrit :
>
> On 2/25/25 06:36, Regis Dargent wrote:
> > 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>
> >
> > --
> >
> > Changelog v1..v2:
> > - add sunxi_wdt_read_timeout function
> > - add signed-off-by tag
> >
> > Changelog v2..v3:
> > - WDIOF_SETTIMEOUT was set twice, and other code cleanup
> > ---
> >   drivers/watchdog/sunxi_wdt.c | 45 ++++++++++++++++++++++++++++++++++--
> >   1 file changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> > index b85354a99582..d509dbcb77ce 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++)
>
> Unnecessary (). On top of that, its complexity is unnecessary.
> The timeout in seconds, except for reg == 0, is wdt_timeout_map[reg],
> with values of 0x0c..0x0f undefined. Worse, the above code can access
> beyond the size of wdt_timeout_map[] if reg >= 0x0c.

Ok, I prefer parenthesis for (my) readability, but I will remove them.

wdt_timeout_map is not linear, so timeout in seconds is i when
wdt_timeout_map[i] == reg, some values of i not corresponding to any
reg value.
reg values of 0x0c and more will end with a timeout of 16s
(i == WDT_MAX_TIMEOUT) which will lead to unknown behaviour only
because these values are marked "reserved" in the HW timer documentation.

> > +             ;
> > +     if (i == 0) {
> > +             wdt_dev->timeout = 1;
> > +             wdt_dev->max_hw_heartbeat_ms = 500;
>
> This is an unacceptable API abuse. max_hw_heartbeat_ms, if set,
> should be 16000, not 500. You could set the timeout to 1 second instead.

sorry about that, my comprehension of the documentation on
min/max_hw_heartbeat_ms suggested that they define the time window during
which the HW timer must receive a heartbeat or it will reset.
0-'timeout' defines the time window for the userspace point of view,
meaning that if 'timeout' > max_hw_heartbeat_ms, extra heartbeats must be
sent to HW timer so that the reset can occur only after 'timeout' seconds.

Just to be clear, setting max_hw_heartbeat_ms to the maximum HW possible
value (here 16000ms) means that one cannot use sub-second HW timer values,
while all the watchdog API actually handles them well enough (my tests on
the 500ms HW timer worked good).

For this sunxi driver case, this implies that instead of just reading the
current timeout value from the HW timer (if it was enabled before the
kernel start), I also have to reconfigure the HW timer (to 1sec) if its
timeout is less than a second (the 0 register value).

> Guenter
>