Re: [PATCH] watchdog: mediatek: Enable pretimeout support

[Tzung-Bi Shih]

On Mon, Aug 24, 2026 at 09:29:12PM +0800, Wanming Gao wrote:
> The watchdog pretimeout interrupt needs to be deasserted after the
> hardware pretimeout event is received. Add IRQ level control handling
> to the MediaTek watchdog interrupt handler so the interrupt is cleared
> before notifying the watchdog core.

This could be clearer.  Please merge this with the third paragraph to
explain the why rather than just the what (e.g., explain that the hardware
uses level triggering and requires an edge transition to clear the
interrupt, if that is the case).

> Serialize all WDT_MODE read-modify-write operations using the existing
> watchdog spinlock. Initialize the lock before registering the interrupt
> handler and hold it across the complete 70us IRQ level toggle sequence.
> This prevents concurrent watchdog start, stop, or pretimeout updates
> from overwriting the temporary IRQ level state and disrupting the
> deassert pulse.

Please expand on the why here as well.  The 70us delay specifically needs
to be explained in the commit message (e.g., waiting for the clock domain
crossing to the 32KHz clock).

> 
> This avoids leaving the watchdog pretimeout interrupt asserted, prevents
> a potential interrupt storm, and allows the kernel to handle the
> pretimeout event properly before the final watchdog reset.

As mentioned above, please merge this with the first paragraph.

> 
> Test: Boot ok, trigger watchdog pretimeout case and verify the
>       pretimeout interrupt is received and handled before watchdog reset.

It is expected that patches are tested before submission.  Unless you are
specifying the exact board/platform you tested against, please drop this
section.

> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
...
> +static void mtk_wdt_deassert_irq(struct watchdog_device *wdd)
> +{
> +	struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdd);
> +	void __iomem *wdt_base = mtk_wdt->wdt_base;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	spin_lock_irqsave(&mtk_wdt->lock, flags);
> +	reg = ioread32(wdt_base + WDT_MODE);
> +
> +	if (reg & WDT_MODE_IRQ_LEVEL_EN) {
> +		reg &= ~WDT_MODE_IRQ_LEVEL_EN;
> +		iowrite32(reg | WDT_MODE_KEY, wdt_base + WDT_MODE);
> +		/*
> +		 * Wait for two 32KHz watchdog clock cycles so the
> +		 * hardware can latch the IRQ level change across the clock
> +		 * domain.
> +		 */
> +		udelay(WDT_IRQ_LEVEL_SYNC_US);
> +		reg = ioread32(wdt_base + WDT_MODE);
> +		reg |= WDT_MODE_IRQ_LEVEL_EN;
> +	} else {
> +		reg |= WDT_MODE_IRQ_LEVEL_EN;
> +		iowrite32(reg | WDT_MODE_KEY, wdt_base + WDT_MODE);
> +		udelay(WDT_IRQ_LEVEL_SYNC_US);
> +		reg = ioread32(wdt_base + WDT_MODE);
> +		reg &= ~WDT_MODE_IRQ_LEVEL_EN;
> +	}

Is the main purpose of toggling WDT_MODE_IRQ_LEVEL_EN simply to clear the
interrupt?  Since the value of the bit doesn't seem to matter.

> +	iowrite32(reg | WDT_MODE_KEY, wdt_base + WDT_MODE);
> +	ioread32(wdt_base + WDT_MODE);

This read deserves a comment.  Otherwise, it looks like a NOP and might be
accidentally removed by someone in the future.

> +	spin_unlock_irqrestore(&mtk_wdt->lock, flags);
> +}

The function can be simplified a bit, e.g.:

    /* Trigger a state change */
    reg ^= WDT_MODE_IRQ_LEVEL_EN;
    iowrite32(...);

    /*
     * Wait for two 32KHz watchdog clock cycles ...
     */
    udelay(...);

    /* Restore to its original state */
    reg = ioread32(...)
    reg ^= WDT_MODE_IRQ_LEVEL_EN;
    iowrite32(...)