[PATCH 0/3] watchdog: realtek-otto: Make use of regmap API and add RTL9607C support

[PATCH 0/3] watchdog: realtek-otto: Make use of regmap API and add RTL9607C support

[Rustam Adilov]

This patch series changes the driver to use regmap API for all of the
register access stuff instead of the ioread32 and iowrite32 with __iomem.

It also adds support for watchdog timer on RTL9607C SoCs and since it is
indentical to the already supported SoCs no major changes are needed.

Rustam Adilov (3):
  watchdog: realtek-otto: Change to use regmap API
  dt-bindings: watchdog: realtek,otto-wdt: add compatible for RTL9607C
  watchdog: realtek-otto: add RTL9607C support

 .../bindings/watchdog/realtek,otto-wdt.yaml   |  1 +
 drivers/watchdog/Kconfig                      |  1 +
 drivers/watchdog/realtek_otto_wdt.c           | 74 ++++++++++---------
 3 files changed, 40 insertions(+), 36 deletions(-)

-- 
2.54.0

[PATCH 1/3] watchdog: realtek-otto: Change to use regmap API

[Rustam Adilov]

Change all of the register access stuff to done through regmap API.
This helps us to simplify the code and allows to make use of specific
regmap functions like regmap_update_bits to replace read/modify/write
instances.

Add the REGMAP_MMIO as a select to REALTEK_OTTO_WDT now that the regmap
is used.

Signed-off-by: Rustam Adilov <adilov@disroot.org>
---
 drivers/watchdog/Kconfig            |  1 +
 drivers/watchdog/realtek_otto_wdt.c | 73 +++++++++++++++--------------
 2 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index dc78729ba2a5..5c32d79b126c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1076,6 +1076,7 @@ config REALTEK_OTTO_WDT
 	depends on MACH_REALTEK_RTL || COMPILE_TEST
 	depends on COMMON_CLK
 	select WATCHDOG_CORE
+	select REGMAP_MMIO
 	default MACH_REALTEK_RTL
 	help
 	  Say Y here to include support for the watchdog timer on Realtek
diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
index 2c30ddd574c5..e5e9cb480f4f 100644
--- a/drivers/watchdog/realtek_otto_wdt.c
+++ b/drivers/watchdog/realtek_otto_wdt.c
@@ -28,6 +28,7 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/reboot.h>
+#include <linux/regmap.h>
 #include <linux/watchdog.h>
 
 #define OTTO_WDT_REG_CNTR		0x0
@@ -66,7 +67,7 @@
 struct otto_wdt_ctrl {
 	struct watchdog_device wdev;
 	struct device *dev;
-	void __iomem *base;
+	struct regmap *regmap;
 	unsigned int clk_rate_khz;
 	int irq_phase1;
 };
@@ -74,24 +75,17 @@ struct otto_wdt_ctrl {
 static int otto_wdt_start(struct watchdog_device *wdev)
 {
 	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
-	u32 v;
-
-	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
-	v |= OTTO_WDT_CTRL_ENABLE;
-	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
 
+	regmap_set_bits(ctrl->regmap, OTTO_WDT_REG_CTRL, OTTO_WDT_CTRL_ENABLE);
 	return 0;
 }
 
 static int otto_wdt_stop(struct watchdog_device *wdev)
 {
 	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
-	u32 v;
-
-	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
-	v &= ~OTTO_WDT_CTRL_ENABLE;
-	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
 
+	regmap_clear_bits(ctrl->regmap, OTTO_WDT_REG_CTRL,
+			  OTTO_WDT_CTRL_ENABLE);
 	return 0;
 }
 
@@ -99,8 +93,7 @@ static int otto_wdt_ping(struct watchdog_device *wdev)
 {
 	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
 
-	iowrite32(OTTO_WDT_CNTR_PING, ctrl->base + OTTO_WDT_REG_CNTR);
-
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_CNTR, OTTO_WDT_CNTR_PING);
 	return 0;
 }
 
@@ -132,7 +125,7 @@ static int otto_wdt_determine_timeouts(struct watchdog_device *wdev, unsigned in
 	unsigned int total_ticks;
 	unsigned int prescale;
 	unsigned int tick_ms;
-	u32 v;
+	u32 mask, val;
 
 	do {
 		prescale = prescale_next;
@@ -148,14 +141,11 @@ static int otto_wdt_determine_timeouts(struct watchdog_device *wdev, unsigned in
 	} while (phase1_ticks > OTTO_WDT_PHASE_TICKS_MAX
 		|| phase2_ticks > OTTO_WDT_PHASE_TICKS_MAX);
 
-	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
-
-	v &= ~(OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 | OTTO_WDT_CTRL_PHASE2);
-	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
-	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
-	v |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
-
-	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+	mask = OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 | OTTO_WDT_CTRL_PHASE2;
+	val = FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
+	val |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
+	val |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
+	regmap_update_bits(ctrl->regmap, OTTO_WDT_REG_CTRL, mask, val);
 
 	timeout_ms = total_ticks * tick_ms;
 	ctrl->wdev.timeout = timeout_ms / 1000;
@@ -199,7 +189,7 @@ static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_m
 
 	/* Configure for shortest timeout and wait for reset to occur */
 	v = FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, reset_mode) | OTTO_WDT_CTRL_ENABLE;
-	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, v);
 
 	mdelay(3 * otto_wdt_tick_ms(ctrl, 0));
 
@@ -210,7 +200,7 @@ static irqreturn_t otto_wdt_phase1_isr(int irq, void *dev_id)
 {
 	struct otto_wdt_ctrl *ctrl = dev_id;
 
-	iowrite32(OTTO_WDT_INTR_PHASE_1, ctrl->base + OTTO_WDT_REG_INTR);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_INTR, OTTO_WDT_INTR_PHASE_1);
 	dev_crit(ctrl->dev, "phase 1 timeout\n");
 	watchdog_notify_pretimeout(&ctrl->wdev);
 
@@ -256,7 +246,6 @@ static int otto_wdt_probe_reset_mode(struct otto_wdt_ctrl *ctrl)
 	const struct fwnode_handle *node = ctrl->dev->fwnode;
 	int mode_count;
 	u32 mode;
-	u32 v;
 
 	if (!node)
 		return -ENXIO;
@@ -278,19 +267,24 @@ static int otto_wdt_probe_reset_mode(struct otto_wdt_ctrl *ctrl)
 	else
 		return -EINVAL;
 
-	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
-	v &= ~OTTO_WDT_CTRL_RST_MODE;
-	v |= FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, mode);
-	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
-
+	regmap_update_bits(ctrl->regmap, OTTO_WDT_REG_CTRL,
+			   OTTO_WDT_CTRL_RST_MODE,
+			   FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, mode));
 	return 0;
 }
 
+static const struct regmap_config realtek_otto_wdt_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+};
+
 static int otto_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct otto_wdt_ctrl *ctrl;
 	unsigned int max_tick_ms;
+	void __iomem *base;
 	int ret;
 
 	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
@@ -298,14 +292,21 @@ static int otto_wdt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctrl->dev = dev;
-	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(ctrl->base))
-		return PTR_ERR(ctrl->base);
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	ctrl->regmap = devm_regmap_init_mmio(dev, base,
+					     &realtek_otto_wdt_regmap_config);
+	if (IS_ERR(ctrl->regmap)) {
+		dev_err(dev, "regmap init failed\n");
+		return PTR_ERR(ctrl->regmap);
+	}
 
 	/* Clear any old interrupts and reset initial state */
-	iowrite32(OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2,
-			ctrl->base + OTTO_WDT_REG_INTR);
-	iowrite32(OTTO_WDT_CTRL_DEFAULT, ctrl->base + OTTO_WDT_REG_CTRL);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_INTR,
+		     OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2);
+	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, OTTO_WDT_CTRL_DEFAULT);
 
 	ret = otto_wdt_probe_clk(ctrl);
 	if (ret)
-- 
2.54.0

Re: [PATCH 1/3] watchdog: realtek-otto: Change to use regmap API

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deadlock risk in the atomic restart handler due to regmap's internal spinlock.
--

commit a520c8e2203154ba331a6ae28f00daec074e62ee
Author: Rustam Adilov <adilov@disroot.org>

watchdog: realtek-otto: Change to use regmap API

This commit converts the realtek-otto watchdog driver to use the regmap API
instead of direct mmio accesses. It replaces direct read/modify/write
sequences with regmap functions and selects REGMAP_MMIO.

> diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
> index 2c30ddd574c59..e5e9cb480f4f9 100644
> --- a/drivers/watchdog/realtek_otto_wdt.c
> +++ b/drivers/watchdog/realtek_otto_wdt.c
[ ... ]
> @@ -199,7 +189,7 @@ static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_m
>  
>  	/* Configure for shortest timeout and wait for reset to occur */
>  	v = FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, reset_mode) | OTTO_WDT_CTRL_ENABLE;
> -	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, v);

Could this regmap_write() cause a deadlock during system restart?

The restart handler is often called during system shutdown or kernel panic
in an atomic context with local interrupts disabled. Since
devm_regmap_init_mmio() configures the regmap to use a spinlock by default,
if a panic occurs while another cpu holds the regmap lock (e.g., during
a ping or timeout update) and that cpu is halted by smp_send_stop(), this
regmap_write() could spin endlessly and prevent the system from rebooting.

Additionally, on preempt-rt kernels, the regmap spinlock becomes a sleepable
rt_mutex, which could cause a sleeping in atomic context bug when acquired
with interrupts disabled.

>  	mdelay(3 * otto_wdt_tick_ms(ctrl, 0));
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509163101.722793-1-adilov@disroot.org?part=1

[PATCH 2/3] dt-bindings: watchdog: realtek,otto-wdt: add compatible for RTL9607C

[Rustam Adilov]

Add the realtek,rtl9607-wdt compatible to the Realtek Otto watchdog
binding.

Signed-off-by: Rustam Adilov <adilov@disroot.org>
---
 Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
index 1f5390a67cdb..ac9db40b12dc 100644
--- a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
@@ -30,6 +30,7 @@ properties:
       - realtek,rtl8390-wdt
       - realtek,rtl9300-wdt
       - realtek,rtl9310-wdt
+      - realtek,rtl9607-wdt
 
   reg:
     maxItems: 1
-- 
2.54.0

Re: [PATCH 2/3] dt-bindings: watchdog: realtek,otto-wdt: add compatible for RTL9607C

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

On Sat, May 09, 2026 at 09:31:00PM +0500, Rustam Adilov wrote:
> Add the realtek,rtl9607-wdt compatible to the Realtek Otto watchdog
> binding.
> 
> Signed-off-by: Rustam Adilov <adilov@disroot.org>
> ---
>  Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> index 1f5390a67cdb..ac9db40b12dc 100644
> --- a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> @@ -30,6 +30,7 @@ properties:
>        - realtek,rtl8390-wdt
>        - realtek,rtl9300-wdt
>        - realtek,rtl9310-wdt
> +      - realtek,rtl9607-wdt

Please explain in your commit message why this new device is not
compatible with the existing ones, particularly given the driver patch
implies that it would be.
pw-bot: changes-requested

Thanks,
Conor.

>  
>    reg:
>      maxItems: 1
> -- 
> 2.54.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] dt-bindings: watchdog: realtek,otto-wdt: add compatible for RTL9607C

[Rustam Adilov]

Hello,
On 2026-05-09 18:13, Conor Dooley wrote:
> On Sat, May 09, 2026 at 09:31:00PM +0500, Rustam Adilov wrote:
>> Add the realtek,rtl9607-wdt compatible to the Realtek Otto watchdog
>> binding.
>> 
>> Signed-off-by: Rustam Adilov <adilov@disroot.org>
>> ---
>>  Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
>> index 1f5390a67cdb..ac9db40b12dc 100644
>> --- a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
>> +++ b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
>> @@ -30,6 +30,7 @@ properties:
>>        - realtek,rtl8390-wdt
>>        - realtek,rtl9300-wdt
>>        - realtek,rtl9310-wdt
>> +      - realtek,rtl9607-wdt
> 
> Please explain in your commit message why this new device is not
> compatible with the existing ones, particularly given the driver patch
> implies that it would be.
> pw-bot: changes-requested

Is the fact "RTL9607C is different SoC compared to the others in the list" not
enough of a reason to include even though the all of them have an identical watchdog
timer device? But yes, i can use any other compatible just as fine.

Maybe its just difference in maintainers because the RTL9310 [1] was accepted and
had the identical case to this one.

[1]- https://lore.kernel.org/linux-watchdog/84d873d7dd375cd2392f89fa6bd9e0fe5dda4e1c.1656356377.git.sander@svanheule.net/

> 
> Thanks,
> Conor.

Best,
Rustam

Re: [PATCH 2/3] dt-bindings: watchdog: realtek,otto-wdt: add compatible for RTL9607C

[Sander Vanheule]

Hi,

On Sat, 2026-05-09 at 19:13 +0100, Conor Dooley wrote:
> On Sat, May 09, 2026 at 09:31:00PM +0500, Rustam Adilov wrote:
> > Add the realtek,rtl9607-wdt compatible to the Realtek Otto watchdog
> > binding.
> > 
> > Signed-off-by: Rustam Adilov <adilov@disroot.org>
> > ---
> >  Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/realtek,otto-
> > wdt.yaml b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> > index 1f5390a67cdb..ac9db40b12dc 100644
> > --- a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> > @@ -30,6 +30,7 @@ properties:
> >        - realtek,rtl8390-wdt
> >        - realtek,rtl9300-wdt
> >        - realtek,rtl9310-wdt
> > +      - realtek,rtl9607-wdt
> 
> Please explain in your commit message why this new device is not
> compatible with the existing ones, particularly given the driver patch
> implies that it would be.
> pw-bot: changes-requested

Back when I originally wrote this driver, device-specific compatibles were
encouraged "just in case" the hardware would turn out to behave differently.
Has this changed to minimizing the number of compatibles?

FWIW this platform's GPIO driver does have both SoC-specific compatibles and a
fallback, but for reasons lost to time this binding has no fallback. I'll submit
a patch to add a fallback for this binding too.


Best,
Sander

[PATCH 3/3] watchdog: realtek-otto: add RTL9607C support

[Rustam Adilov]

The RTL9607C SoC has a watchdog timer identical to the already
supported SoCs. The peripheral is memory mapped at 0x18003260
and driven by the Lexra bus clock.

Signed-off-by: Rustam Adilov <adilov@disroot.org>
---
 drivers/watchdog/realtek_otto_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
index e5e9cb480f4f..7ad196802eeb 100644
--- a/drivers/watchdog/realtek_otto_wdt.c
+++ b/drivers/watchdog/realtek_otto_wdt.c
@@ -355,6 +355,7 @@ static const struct of_device_id otto_wdt_ids[] = {
 	{ .compatible = "realtek,rtl8390-wdt" },
 	{ .compatible = "realtek,rtl9300-wdt" },
 	{ .compatible = "realtek,rtl9310-wdt" },
+	{ .compatible = "realtek,rtl9607-wdt" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, otto_wdt_ids);
-- 
2.54.0

Re: [PATCH 3/3] watchdog: realtek-otto: add RTL9607C support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Registers accessed before peripheral clock is enabled
- [High] Sleeping function `disable_irq()` called in atomic restart handler
- [High] Integer underflow in timeout calculation causes massively delayed watchdog reset
--

commit be1a33571f33f241ada324d2648ec90cdf1e9b91
Author: Rustam Adilov <adilov@disroot.org>

watchdog: realtek-otto: add RTL9607C support

The RTL9607C SoC has a watchdog timer identical to the already
supported SoCs. The peripheral is memory mapped at 0x18003260
and driven by the Lexra bus clock.

> diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
> index e5e9cb480f4f9..7ad196802eeb5 100644
> --- a/drivers/watchdog/realtek_otto_wdt.c
> +++ b/drivers/watchdog/realtek_otto_wdt.c
> @@ -355,6 +355,7 @@ static const struct of_device_id otto_wdt_ids[] = {
>  	{ .compatible = "realtek,rtl8390-wdt" },
>  	{ .compatible = "realtek,rtl9300-wdt" },
>  	{ .compatible = "realtek,rtl9310-wdt" },
> +	{ .compatible = "realtek,rtl9607-wdt" },

This isn't a bug introduced by this patch, but enabling this driver for the
new SoC exposes a few existing regressions in the code.

For example, can the driver safely access the peripheral's memory-mapped
registers before its clock domain is enabled?

otto_wdt_probe() {
[ ... ]
	/* Clear any old interrupts and reset initial state */
	regmap_write(ctrl->regmap, OTTO_WDT_REG_INTR,
		     OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2);
	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, OTTO_WDT_CTRL_DEFAULT);

	ret = otto_wdt_probe_clk(ctrl);
[ ... ]
}

Writing to OTTO_WDT_REG_INTR and OTTO_WDT_REG_CTRL before calling
otto_wdt_probe_clk() might violate hardware access requirements and could
cause a synchronous external bus abort or system hang during driver probe.

Also, does the system restart handler sleep in an atomic context?

otto_wdt_restart() {
[ ... ]
	disable_irq(ctrl->irq_phase1);
[ ... ]
}

The otto_wdt_restart() function is a system restart handler executed in an
atomic context with local interrupts disabled. Since disable_irq() internally
invokes synchronize_irq(), which uses wait_event() to wait for executing
interrupt handlers to finish, it can sleep.

Calling a sleeping function with local interrupts disabled triggers a kernel
panic, and waiting for an IRQ handler to complete while other CPUs are stopped
during a machine restart can result in a hard deadlock. Should this use
disable_irq_nosync() instead?

Finally, is there an integer underflow in the timeout calculation?

otto_wdt_determine_timeouts() {
[ ... ]
		total_ticks = div_round_ticks(timeout_ms, tick_ms, 2);
		phase1_ticks = div_round_ticks(timeout_ms - pretimeout_ms, tick_ms, 1);
		phase2_ticks = total_ticks - phase1_ticks;
[ ... ]
	val |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
[ ... ]
}

If the pretimeout is configured to 0 (or a value small enough that it maps to
the same tick interval), total_ticks and phase1_ticks evaluate to the exact
same value, resulting in phase2_ticks = 0.

Since phase2_ticks is an unsigned integer, 0 - 1 underflows to 0xFFFFFFFF,
which FIELD_PREP masks to the maximum field value of 31. Does this configure
the hardware to delay the system reset by 32 ticks (over 5 seconds) instead
of the intended minimum of 1 tick?

>  	{ }
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509163101.722793-1-adilov@disroot.org?part=3

Re: [PATCH 3/3] watchdog: realtek-otto: add RTL9607C support

[Sander Vanheule]

As a general remark, it would be good if this bot could use the original To/Cc
list of the message this was sent in reply to.


On Sat, 2026-05-09 at 17:27 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
> - [High] Registers accessed before peripheral clock is enabled
> - [High] Sleeping function `disable_irq()` called in atomic restart handler
> - [High] Integer underflow in timeout calculation causes massively delayed
> watchdog reset
> --
> 
> commit be1a33571f33f241ada324d2648ec90cdf1e9b91
> Author: Rustam Adilov <adilov@disroot.org>
> 
> watchdog: realtek-otto: add RTL9607C support
> 
> The RTL9607C SoC has a watchdog timer identical to the already
> supported SoCs. The peripheral is memory mapped at 0x18003260
> and driven by the Lexra bus clock.
> 
> > diff --git a/drivers/watchdog/realtek_otto_wdt.c
> > b/drivers/watchdog/realtek_otto_wdt.c
> > index e5e9cb480f4f9..7ad196802eeb5 100644
> > --- a/drivers/watchdog/realtek_otto_wdt.c
> > +++ b/drivers/watchdog/realtek_otto_wdt.c
> > @@ -355,6 +355,7 @@ static const struct of_device_id otto_wdt_ids[] = {
> >  	{ .compatible = "realtek,rtl8390-wdt" },
> >  	{ .compatible = "realtek,rtl9300-wdt" },
> >  	{ .compatible = "realtek,rtl9310-wdt" },
> > +	{ .compatible = "realtek,rtl9607-wdt" },
> 
> This isn't a bug introduced by this patch, but enabling this driver for the
> new SoC exposes a few existing regressions in the code.
> 
> For example, can the driver safely access the peripheral's memory-mapped
> registers before its clock domain is enabled?
> 
> otto_wdt_probe() {
> [ ... ]
> 	/* Clear any old interrupts and reset initial state */
> 	regmap_write(ctrl->regmap, OTTO_WDT_REG_INTR,
> 		     OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2);
> 	regmap_write(ctrl->regmap, OTTO_WDT_REG_CTRL, OTTO_WDT_CTRL_DEFAULT);
> 
> 	ret = otto_wdt_probe_clk(ctrl);
> [ ... ]
> }
> 
> Writing to OTTO_WDT_REG_INTR and OTTO_WDT_REG_CTRL before calling
> otto_wdt_probe_clk() might violate hardware access requirements and could
> cause a synchronous external bus abort or system hang during driver probe.

otto_wdt_probe_clk() can be moved up to come before the first bus accesses.

This would not cause issues on real hardware, as the bootloader will have
ensured the watchdog's bus is up since it is shared with the UART console.



> Also, does the system restart handler sleep in an atomic context?
> 
> otto_wdt_restart() {
> [ ... ]
> 	disable_irq(ctrl->irq_phase1);
> [ ... ]
> }
> 
> The otto_wdt_restart() function is a system restart handler executed in an
> atomic context with local interrupts disabled. Since disable_irq() internally
> invokes synchronize_irq(), which uses wait_event() to wait for executing
> interrupt handlers to finish, it can sleep.
> 
> Calling a sleeping function with local interrupts disabled triggers a kernel
> panic, and waiting for an IRQ handler to complete while other CPUs are stopped
> during a machine restart can result in a hard deadlock. Should this use
> disable_irq_nosync() instead?

I don't see why it couldn't use disable_irq_nosync().


> Finally, is there an integer underflow in the timeout calculation?
> 
> otto_wdt_determine_timeouts() {
> [ ... ]
> 		total_ticks = div_round_ticks(timeout_ms, tick_ms, 2);
> 		phase1_ticks = div_round_ticks(timeout_ms - pretimeout_ms,
> tick_ms, 1);
> 		phase2_ticks = total_ticks - phase1_ticks;
> [ ... ]
> 	val |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
> [ ... ]
> }
> 
> If the pretimeout is configured to 0 (or a value small enough that it maps to
> the same tick interval), total_ticks and phase1_ticks evaluate to the exact
> same value, resulting in phase2_ticks = 0.
> 
> Since phase2_ticks is an unsigned integer, 0 - 1 underflows to 0xFFFFFFFF,
> which FIELD_PREP masks to the maximum field value of 31. Does this configure
> the hardware to delay the system reset by 32 ticks (over 5 seconds) instead
> of the intended minimum of 1 tick?

While this does indeed delay the system reset to the maximum time, the
pretimeout interrupt (end of PHASE1) would still occur at the intended time.
During PHASE2, the watchdog cannot be pinged anymore and system reset remains
inevitable.

I'll send a set of patches to resolve these issues, as they don't really relate
to adding RTL9706C support.

Best,
Sander

Re: [PATCH 0/3] watchdog: realtek-otto: Make use of regmap API and add RTL9607C support

[Guenter Roeck]

On 5/9/26 09:30, Rustam Adilov wrote:
> This patch series changes the driver to use regmap API for all of the
> register access stuff instead of the ioread32 and iowrite32 with __iomem.
> 
> It also adds support for watchdog timer on RTL9607C SoCs and since it is
> indentical to the already supported SoCs no major changes are needed.
> 

Please address the concerns raised by Sashiko.

Thanks,
Guenter