[PATCH v2 0/3] watchdog: dw_wdt: fix reset line management

[PATCH v2 0/3] watchdog: dw_wdt: fix reset line management

[Artem Shimko]

Hello,

This is v2 of the series fixing reset line management in the dw_wdt
driver. Compared to v1, the series is reduced from 5 patches to 3:
the manual reset_control_deassert()/reset_control_assert() handling
is replaced by the device-managed _deasserted variant from the start.

Patch 1 replaces devm_reset_control_get_optional_shared() with
devm_reset_control_get_optional_shared_deasserted(), so the reset line
is deasserted on acquisition, before the first register access in
dw_wdt_update_mode(), and is asserted again automatically by devres
when the device is removed. This removes the out_assert_rst label and
the explicit reset_control_assert() calls in probe error paths and in
dw_wdt_drv_remove(), and uses dev_err_probe() for the acquisition error.

Patch 2 adds reset line management during system suspend/resume:
the reset is asserted before clocks are disabled in suspend, and
deasserted after clocks are re-enabled and before the register state
is restored in resume. Error handling is added for the reset operations
in resume, with a cleanup path that disables the clocks in reverse
order of enabling.

Patch 3 cleans up the error paths in dw_wdt_resume().

Thank you.

Testing:

$ ls /dev/watchdog*
/dev/watchdog   /dev/watchdog1  /dev/watchdog3
/dev/watchdog0  /dev/watchdog2

$ ls /sys/class/watchdog/
watchdog0  watchdog1  watchdog2  watchdog3

$ chmod +x watchdog-test
$ ./watchdog-test -f /dev/watchdog0 -i -s
watchdog_info:
 identity:              Synopsys DesignWare Watchdog
 firmware_version:      0
Support/Status: Set timeout (in seconds)
Support/Status: Supports magic close char
Support/Status: Pretimeout (in seconds), get/set
Support/Status: Keep alive ping reply
Support/Status: Keep alive ping reply

$ echo mem > /sys/power/state
...
...
[ 1560.147058] PM: suspend exit
[ 1560.371470] usb 1-1: new high-speed USB device number 3 using xhci-hcd
[ 1561.031825] cdc_ncm 1-1:2.0: MAC-Address: a4:2a:95:e8:fc:e4
[ 1561.037736] cdc_ncm 1-1:2.0: setting rx_max = 16384
[ 1561.054408] cdc_ncm 1-1:2.0: setting tx_max = 16384
[ 1561.084713] cdc_ncm 1-1:2.0 eth0: register 'cdc_ncm' at usb-xhci-hcd.2.auto-1, CDC NCM (NO ZLP), a4:2a:95:e8:fc:e4

$ ./watchdog-test -f /dev/watchdog0 -i -s
watchdog_info:
 identity:              Synopsys DesignWare Watchdog
 firmware_version:      0
Support/Status: Set timeout (in seconds)
Support/Status: Supports magic close char
Support/Status: Pretimeout (in seconds), get/set
Support/Status: Keep alive ping reply
Support/Status: Keep alive ping reply

$ ls /dev/watchdog*
/dev/watchdog   /dev/watchdog1  /dev/watchdog3
/dev/watchdog0  /dev/watchdog2

$ echo 1 > /dev/watchdog0
$ cat /sys/kernel/debug/1e012000.wdt/registers | grep ccvr
ccvr = 0x09a7baab
$ cat /sys/kernel/debug/1e012000.wdt/registers | grep ccvr
ccvr = 0x0599dce0
$ cat /sys/kernel/debug/1e012000.wdt/registers | grep ccvr
ccvr = 0x01926467
$ [  110.392536] watchdog0: pretimeout event

--
Best regards,
Artem

ChangeLog:
  v1:
    * https://lore.kernel.org/all/20260320135624.223863-1-a.shimko.dev@gmail.com/T/#t
  v2:
    * Reduced from 5 patches to 3: manual reset handling is replaced by
      the devm _deasserted variant from the start, so the intermediate
      states that could leak the deasserted reset line are gone and every
      commit is self-contained.

Artem Shimko (3):
  watchdog: dw_wdt: use
    devm_reset_control_get_optional_shared_deasserted
  watchdog: dw_wdt: manage reset line during system suspend/resume
  watchdog: dw_wdt: clean up error paths in resume function

 drivers/watchdog/dw_wdt.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] watchdog: dw_wdt: use devm_reset_control_get_optional_shared_deasserted

[Artem Shimko]

Replace devm_reset_control_get_optional_shared() with
devm_reset_control_get_optional_shared_deasserted() to let the
device-managed API handle reset line deassertion and assertion
automatically.

The driver currently manages the reset control manually: it calls
reset_control_deassert() during probe and reset_control_assert() in
error paths and remove. This is error-prone and requires explicit
cleanup in multiple places.

Switching to the _deasserted variant deasserts the reset line on
acquisition and asserts it again when the device is removed, which
allows removing the out_assert_rst label along with the related
assertions in the probe error paths and in dw_wdt_drv_remove().

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
 drivers/watchdog/dw_wdt.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 39031445d754..2cffe337b4ea 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -588,9 +588,9 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	if (IS_ERR(dw_wdt->pclk))
 		return PTR_ERR(dw_wdt->pclk);
 
-	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+	dw_wdt->rst = devm_reset_control_get_optional_shared_deasserted(dev, NULL);
 	if (IS_ERR(dw_wdt->rst))
-		return PTR_ERR(dw_wdt->rst);
+		return dev_err_probe(dev, PTR_ERR(dw_wdt->rst), "failed to acquire reset\n");
 
 	/* Enable normal reset without pre-timeout by default. */
 	dw_wdt_update_mode(dw_wdt, DW_WDT_RMOD_RESET);
@@ -617,11 +617,9 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 		dw_wdt->wdd.info = &dw_wdt_ident;
 	}
 
-	reset_control_deassert(dw_wdt->rst);
-
 	ret = dw_wdt_init_timeouts(dw_wdt, dev);
 	if (ret)
-		goto out_assert_rst;
+		return ret;
 
 	wdd = &dw_wdt->wdd;
 	wdd->ops = &dw_wdt_ops;
@@ -655,15 +653,11 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 
 	ret = watchdog_register_device(wdd);
 	if (ret)
-		goto out_assert_rst;
+		return ret;
 
 	dw_wdt_dbgfs_init(dw_wdt);
 
 	return 0;
-
-out_assert_rst:
-	reset_control_assert(dw_wdt->rst);
-	return ret;
 }
 
 static void dw_wdt_drv_remove(struct platform_device *pdev)
@@ -673,7 +667,6 @@ static void dw_wdt_drv_remove(struct platform_device *pdev)
 	dw_wdt_dbgfs_clear(dw_wdt);
 
 	watchdog_unregister_device(&dw_wdt->wdd);
-	reset_control_assert(dw_wdt->rst);
 }
 
 #ifdef CONFIG_OF
-- 
2.43.0

Re: [PATCH v2 1/3] watchdog: dw_wdt: use devm_reset_control_get_optional_shared_deasserted

[Tzung-Bi Shih]

On Thu, Oct 22, 2026 at 04:43:49PM +0300, Artem Shimko wrote:
> Replace devm_reset_control_get_optional_shared() with
> devm_reset_control_get_optional_shared_deasserted() to let the
> device-managed API handle reset line deassertion and assertion
> automatically.
> 
> The driver currently manages the reset control manually: it calls
> reset_control_deassert() during probe and reset_control_assert() in
> error paths and remove. This is error-prone and requires explicit
> cleanup in multiple places.
> 
> Switching to the _deasserted variant deasserts the reset line on
> acquisition and asserts it again when the device is removed, which
> allows removing the out_assert_rst label along with the related
> assertions in the probe error paths and in dw_wdt_drv_remove().
> 
> Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>

With a minor concern shown below,

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 39031445d754..2cffe337b4ea 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -588,9 +588,9 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>  	if (IS_ERR(dw_wdt->pclk))
>  		return PTR_ERR(dw_wdt->pclk);
>  
> -	dw_wdt->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
> +	dw_wdt->rst = devm_reset_control_get_optional_shared_deasserted(dev, NULL);
>  	if (IS_ERR(dw_wdt->rst))
> -		return PTR_ERR(dw_wdt->rst);
> +		return dev_err_probe(dev, PTR_ERR(dw_wdt->rst), "failed to acquire reset\n");

The driver has been "quiet" in probe().  After the change, it starts to be
less quiet.

Re: [PATCH v2 1/3] watchdog: dw_wdt: use devm_reset_control_get_optional_shared_deasserted

[Tzung-Bi Shih]

On Fri, Oct 23, 2026 at 07:51:00PM +0300, Artem Shimko wrote:
> On Fri, Oct 23, 2026 at 6:05 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> 
> > The driver has been "quiet" in probe().  After the change, it starts to be
> > less quiet.
> 
> Thank you for your review.
> 
> It was based on other drivers (as example [1][2]) which use
> dev_err_probe() after
> devm_reset_control_get_optional_exclusive_deasserted() / reset_deassert().
> If you think this dev_err_probe() isn't needed, i can replace it with a return.

That would be great.  It seems v3 still preserves it.  I have no strong
opinion but just wanted to be consistent in the driver.

> 
> [1] drivers/i2c/busses/i2c-designware-platdrv.c:163
> [2] drivers/tty/serial/8250/8250_dw.c:731

---
Please reply-all next time so that the discussion remains visible to folks
on the mailing lists.

[PATCH v2 2/3] watchdog: dw_wdt: manage reset line during system suspend/resume

[Artem Shimko]

The watchdog controller reset line is not currently managed during system
suspend/resume. This can lead to an inconsistent hardware state when
resuming from suspend, as the controller may not be properly reinitialized.

Assert the reset line before disabling clocks during suspend to ensure
the controller is held in reset while power is gated. During resume,
deassert the reset after clocks are enabled and before restoring register
state to guarantee the controller is properly initialized. Add proper
error handling for reset control operations during resume with appropriate
cleanup path on failure.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
 drivers/watchdog/dw_wdt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 2cffe337b4ea..ec6a09673da4 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -382,6 +382,8 @@ static int dw_wdt_suspend(struct device *dev)
 	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 
+	reset_control_assert(dw_wdt->rst);
+
 	clk_disable_unprepare(dw_wdt->pclk);
 	clk_disable_unprepare(dw_wdt->clk);
 
@@ -402,12 +404,22 @@ static int dw_wdt_resume(struct device *dev)
 		return err;
 	}
 
+	err = reset_control_deassert(dw_wdt->rst);
+	if (err)
+		goto unprepare_pclk;
+
 	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
 	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
 
 	dw_wdt_ping(&dw_wdt->wdd);
 
 	return 0;
+
+unprepare_pclk:
+	clk_disable_unprepare(dw_wdt->pclk);
+	clk_disable_unprepare(dw_wdt->clk);
+
+	return err;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
-- 
2.43.0

Re: [PATCH v2 2/3] watchdog: dw_wdt: manage reset line during system suspend/resume

[Tzung-Bi Shih]

On Thu, Oct 22, 2026 at 04:43:50PM +0300, Artem Shimko wrote:
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 2cffe337b4ea..ec6a09673da4 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -382,6 +382,8 @@ static int dw_wdt_suspend(struct device *dev)
>  	dw_wdt->control = readl(dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>  	dw_wdt->timeout = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  
> +	reset_control_assert(dw_wdt->rst);
> +

Doesn't it need to check the return value and propagate the error just
like what dw_wdt_resume() does in the patch?

> @@ -402,12 +404,22 @@ static int dw_wdt_resume(struct device *dev)
>  		return err;
>  	}
>  
> +	err = reset_control_deassert(dw_wdt->rst);
> +	if (err)
> +		goto unprepare_pclk;
> +
>  	writel(dw_wdt->timeout, dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>  	writel(dw_wdt->control, dw_wdt->regs + WDOG_CONTROL_REG_OFFSET);
>  
>  	dw_wdt_ping(&dw_wdt->wdd);
>  
>  	return 0;
> +
> +unprepare_pclk:
> +	clk_disable_unprepare(dw_wdt->pclk);
> +	clk_disable_unprepare(dw_wdt->clk);
> +
> +	return err;

There is only one user of `unprepare_pclk`.  Two options:
A) Drop the goto label and inline the cleanup within `if (err) ...`.
B) Refactor error handling for clk_prepare_enable() to use stacked goto
   labels.

I just realized [PATCH v2 3/3] actually does option B.  Please consider
squashing patch 3/3 into this patch.

[PATCH v2 3/3] watchdog: dw_wdt: clean up error paths in resume function

[Artem Shimko]

Clean up of dw_wdt_resume() function.

Rework the error handling to use properly named labels that match the
cleanup order. Use unprepare_clk for clock disable and unprepare_pclk
for pclk disable path.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
 drivers/watchdog/dw_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index ec6a09673da4..11caf40d6771 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -393,16 +393,15 @@ static int dw_wdt_suspend(struct device *dev)
 static int dw_wdt_resume(struct device *dev)
 {
 	struct dw_wdt *dw_wdt = dev_get_drvdata(dev);
-	int err = clk_prepare_enable(dw_wdt->clk);
+	int err;
 
+	err = clk_prepare_enable(dw_wdt->clk);
 	if (err)
 		return err;
 
 	err = clk_prepare_enable(dw_wdt->pclk);
-	if (err) {
-		clk_disable_unprepare(dw_wdt->clk);
-		return err;
-	}
+	if (err)
+		goto unprepare_clk;
 
 	err = reset_control_deassert(dw_wdt->rst);
 	if (err)
@@ -417,6 +416,7 @@ static int dw_wdt_resume(struct device *dev)
 
 unprepare_pclk:
 	clk_disable_unprepare(dw_wdt->pclk);
+unprepare_clk:
 	clk_disable_unprepare(dw_wdt->clk);
 
 	return err;
-- 
2.43.0

Re: [PATCH v2 3/3] watchdog: dw_wdt: clean up error paths in resume function

[Tzung-Bi Shih]

On Thu, Oct 22, 2026 at 04:43:51PM +0300, Artem Shimko wrote:
> Clean up of dw_wdt_resume() function.
> 
> Rework the error handling to use properly named labels that match the
> cleanup order. Use unprepare_clk for clock disable and unprepare_pclk
> for pclk disable path.
> 
> Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>

See [PATCH v2 2/3].  I'd suggest squashing this patch into [v2 2/3].

> @@ -393,16 +393,15 @@ static int dw_wdt_suspend(struct device *dev)
>  static int dw_wdt_resume(struct device *dev)
>  {
>  	struct dw_wdt *dw_wdt = dev_get_drvdata(dev);
> -	int err = clk_prepare_enable(dw_wdt->clk);
> +	int err;
>  
> +	err = clk_prepare_enable(dw_wdt->clk);

This is an additional change, but I think it's fine to squash.

Re: [PATCH v2 0/3] watchdog: dw_wdt: fix reset line management

[Igor Velkov]

Tested on Helios64 and ROCK Pi 4A (RK3399) on top of 7.3, with
resets = <&cru SRST_P_WDT0> added to the watchdog node locally (not in
mainline yet). With the watchdog daemon at 60 s timeout, s2idle for 90 s:
3/3 on ROCK Pi 4A and 4/4 on Helios64 without a reset; after resume the
watchdog is alive (killing the daemon without magic close resets the board
after ~60 s on both).

Tested-by: Igor Velkov <iav@iav.lv>