[PATCH 0/3] watchdog: at91/sama5d4: header cleanup and driver refactor

[PATCH 0/3] watchdog: at91/sama5d4: header cleanup and driver refactor

[Balakrishnan Sambath]

This series cleans up the AT91 watchdog header and refactors the
sama5d4 watchdog driver.

The header reorganization introduces consistent register naming and
makes the WDDIS bit handling explicit for modern (SAM9X60, SAMA7G5,
SAM9X7) and legacy (SAMA5, AT91SAM9261) SoCs. The driver refactor
improves readability and fixes the reset regression introduced by
commit 266da53c35fc ("watchdog: sama5d4: readout initial state").

Andrei Simion (2):
  watchdog: at91sam9_wdt.h: Cleanup the header file
  watchdog: sama5d4_wdt: Refactor the driver

Balakrishnan Sambath (1):
  watchdog: at91sam9_wdt: Rename AT91_WDT_WDDIS to AT91_WDT_WDDIS_LEGACY

 drivers/watchdog/at91sam9_wdt.c |   8 +-
 drivers/watchdog/at91sam9_wdt.h |  65 +++++++------
 drivers/watchdog/sama5d4_wdt.c  | 156 ++++++++++++++++----------------
 3 files changed, 113 insertions(+), 116 deletions(-)

-- 
2.34.1

[PATCH 1/3] watchdog: at91sam9_wdt.h: Cleanup the header file

[Balakrishnan Sambath]

From: Andrei Simion <andrei.simion@microchip.com>

This patch reorganizes the header file by renaming the registers using
a general pattern also this patch simplifies the watchdog disable logic
in the at91sam9_wdt.h header by differentiating between modern
(SAM9X60, SAMA7G5, SAM9X7) and legacy (SAMA5, AT91SAM9261) chips based
on the watchdog disable bit.
For modern chips, the disable bit is at bit 12, while for legacy chips
it is at bit 15.

Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
[Remove Kconfig-based WDDIS selection and define explicit legacy and
modern masks]
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/watchdog/at91sam9_wdt.h | 65 ++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
index 298d545df1a1..1e0aeecb489f 100644
--- a/drivers/watchdog/at91sam9_wdt.h
+++ b/drivers/watchdog/at91sam9_wdt.h
@@ -3,59 +3,58 @@
  * drivers/watchdog/at91sam9_wdt.h
  *
  * Copyright (C) 2007 Andrew Victor
  * Copyright (C) 2007 Atmel Corporation.
  * Copyright (C) 2019 Microchip Technology Inc. and its subsidiaries
  *
  * Watchdog Timer (WDT) - System peripherals regsters.
  * Based on AT91SAM9261 datasheet revision D.
  * Based on SAM9X60 datasheet.
+ * Based on SAMA7G5 datasheet.
+ * Based on SAM9X75 datasheet.
  *
  */
 
 #ifndef AT91_WDT_H
 #define AT91_WDT_H
 
 #include <linux/bits.h>
 
-#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
-#define  AT91_WDT_WDRSTT	BIT(0)			/* Restart */
-#define  AT91_WDT_KEY		(0xa5UL << 24)		/* KEY Password */
-
-#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
-#define  AT91_WDT_WDV		(0xfffUL << 0)		/* Counter Value */
-#define  AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
-#define  AT91_SAM9X60_PERIODRST	BIT(4)		/* Period Reset */
-#define  AT91_SAM9X60_RPTHRST	BIT(5)		/* Minimum Restart Period */
+#define AT91_WDT_CR		0x00		/* Watchdog Control Register */
+#define  AT91_WDT_WDRSTT	BIT(0)		/* Restart */
+#define  AT91_WDT_KEY		(0xa5UL << 24)	/* KEY Password */
+#define AT91_WDT_MR		0x04		/* Watchdog Mode Register */
+#define  AT91_WDT_WDV		(0xfffUL << 0)	/* Counter Value */
 #define  AT91_WDT_WDFIEN	BIT(12)		/* Fault Interrupt Enable */
-#define  AT91_SAM9X60_WDDIS	BIT(12)		/* Watchdog Disable */
-#define  AT91_WDT_WDRSTEN	BIT(13)		/* Reset Processor */
-#define  AT91_WDT_WDRPROC	BIT(14)		/* Timer Restart */
-#define  AT91_WDT_WDDIS		BIT(15)		/* Watchdog Disable */
-#define  AT91_WDT_WDD		(0xfffUL << 16)		/* Delta Value */
-#define  AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
-#define  AT91_WDT_WDDBGHLT	BIT(28)		/* Debug Halt */
-#define  AT91_WDT_WDIDLEHLT	BIT(29)		/* Idle Halt */
-
+#define  AT91_WDT_WDRSTEN	BIT(13)
+#define  AT91_WDT_WDRPROC	BIT(14)
+#define  AT91_WDT_WDD		(0xfffUL << 16)
+#define  AT91_WDT_WDDBGHLT	BIT(28)
+#define  AT91_WDT_WDIDLEHLT	BIT(29)
 #define AT91_WDT_SR		0x08		/* Watchdog Status Register */
 #define  AT91_WDT_WDUNF		BIT(0)		/* Watchdog Underflow */
 #define  AT91_WDT_WDERR		BIT(1)		/* Watchdog Error */
 
-/* Watchdog Timer Value Register */
-#define AT91_SAM9X60_VR		0x08
+#define  AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
+#define  AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
 
-/* Watchdog Window Level Register */
-#define AT91_SAM9X60_WLR	0x0c
-/* Watchdog Period Value */
-#define  AT91_SAM9X60_COUNTER	(0xfffUL << 0)
-#define  AT91_SAM9X60_SET_COUNTER(x)	((x) & AT91_SAM9X60_COUNTER)
+#define AT91_WDT_VR		0x08	/* Watchdog Timer Value Register */
+#define AT91_WDT_ISR		0x1c	/* Interrupt Status Register */
+#define AT91_WDT_IER		0x14	/* Interrupt Enable Register */
+#define AT91_WDT_IDR		0x18	/* Interrupt Disable Register */
+#define AT91_WDT_WLR		0x0c	/* Watchdog Window Level Register */
+#define AT91_WDT_PERIODRST	BIT(4)	/* Period Reset */
+#define AT91_WDT_RPTHRST	BIT(5)		/* Minimum Restart Period */
+#define  AT91_WDT_PERINT	BIT(0)	/* Period Interrupt Enable */
+#define  AT91_WDT_COUNTER	(0xfffUL << 0)	/* Watchdog Period Value */
+#define  AT91_WDT_SET_COUNTER(x)	((x) & AT91_WDT_COUNTER)
 
-/* Interrupt Enable Register */
-#define AT91_SAM9X60_IER	0x14
-/* Period Interrupt Enable */
-#define  AT91_SAM9X60_PERINT	BIT(0)
-/* Interrupt Disable Register */
-#define AT91_SAM9X60_IDR	0x18
-/* Interrupt Status Register */
-#define AT91_SAM9X60_ISR	0x1c
+/*
+ * WDDIS bit differs by SoC:
+ *   - SAMA5, AT91SAM9261: bit 15
+ *   - SAM9X60, SAMA7G5, SAM9X75: bit 12
+ * Select at runtime via compatible string.
+ */
+#define AT91_WDT_WDDIS_LEGACY   BIT(15)
+#define AT91_WDT_WDDIS_MODERN   BIT(12)
 
 #endif
-- 
2.34.1

[PATCH 2/3] watchdog: sama5d4_wdt: Refactor the driver

[Balakrishnan Sambath]

From: Andrei Simion <andrei.simion@microchip.com>

This patch cleans up and refactors the Atmel SAMA5D4 Watchdog Driver
to be more readable and to fixup Reset issue introduced
by commit 266da53c35fc ("watchdog: sama5d4: readout initial state").

Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
[Use per-device WDDIS mask and sync MR shadow WDDIS state]
Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/watchdog/sama5d4_wdt.c | 156 ++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 79 deletions(-)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 13e72918338a..b422d0bd75f9 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -24,44 +24,58 @@
 #define WDT_DEFAULT_TIMEOUT	MAX_WDT_TIMEOUT
 
 #define WDT_SEC2TICKS(s)	((s) ? (((s) << 8) - 1) : 0)
 
 struct sama5d4_wdt {
 	struct watchdog_device	wdd;
 	void __iomem		*reg_base;
 	u32			mr;
 	u32			ir;
+	u32			wddis_mask;
 	unsigned long		last_ping;
 	bool			need_irq;
-	bool			sam9x60_support;
+	bool			is_modern_chip;
 };
 
+static inline bool wdt_enabled(struct sama5d4_wdt *wdt, u32 mr)
+{
+	return !(mr & wdt->wddis_mask);
+}
+
 static int wdt_timeout;
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
 module_param(wdt_timeout, int, 0);
 MODULE_PARM_DESC(wdt_timeout,
 	"Watchdog timeout in seconds. (default = "
 	__MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
 
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout,
 	"Watchdog cannot be stopped once started (default="
 	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
 
 #define wdt_read(wdt, field) \
 	readl_relaxed((wdt)->reg_base + (field))
 
 /* 4 slow clock periods is 4/32768 = 122.07µs*/
 #define WDT_DELAY	usecs_to_jiffies(123)
 
+static bool check_is_modern_chip_by_compatible(struct device *dev)
+{
+	if (of_device_is_compatible(dev->of_node, "microchip,sam9x60-wdt") ||
+	    of_device_is_compatible(dev->of_node, "microchip,sama7g5-wdt"))
+		return true;
+
+	return false;
+}
+
 static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
 {
 	/*
 	 * WDT_CR and WDT_MR must not be modified within three slow clock
 	 * periods following a restart of the watchdog performed by a write
 	 * access in WDT_CR.
 	 */
 	while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
 		usleep_range(30, 125);
@@ -71,322 +85,306 @@ static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
 
 static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
 {
 	if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
 		udelay(123);
 	writel_relaxed(val, wdt->reg_base + field);
 	wdt->last_ping = jiffies;
 }
 
-static int sama5d4_wdt_start(struct watchdog_device *wdd)
+static int wdt_start(struct watchdog_device *wdd)
 {
 	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
 
-	if (wdt->sam9x60_support) {
-		writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IER);
-		wdt->mr &= ~AT91_SAM9X60_WDDIS;
-	} else {
-		wdt->mr &= ~AT91_WDT_WDDIS;
-	}
+	if (wdt->is_modern_chip)
+		writel_relaxed(wdt->ir, wdt->reg_base + AT91_WDT_IER);
+	wdt->mr &= ~wdt->wddis_mask;
 	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
 
 	return 0;
 }
 
-static int sama5d4_wdt_stop(struct watchdog_device *wdd)
+static int wdt_stop(struct watchdog_device *wdd)
 {
 	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
 
-	if (wdt->sam9x60_support) {
-		writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IDR);
-		wdt->mr |= AT91_SAM9X60_WDDIS;
-	} else {
-		wdt->mr |= AT91_WDT_WDDIS;
-	}
+	if (wdt->is_modern_chip)
+		writel_relaxed(wdt->ir, wdt->reg_base + AT91_WDT_IDR);
+	wdt->mr |= wdt->wddis_mask;
 	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
 
 	return 0;
 }
 
-static int sama5d4_wdt_ping(struct watchdog_device *wdd)
+static int wdt_ping(struct watchdog_device *wdd)
 {
 	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
 
 	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 
 	return 0;
 }
 
-static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
-				 unsigned int timeout)
+static int wdt_set_timeout(struct watchdog_device *wdd,
+			   unsigned int timeout)
 {
 	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
 	u32 value = WDT_SEC2TICKS(timeout);
 
-	if (wdt->sam9x60_support) {
-		wdt_write(wdt, AT91_SAM9X60_WLR,
-			  AT91_SAM9X60_SET_COUNTER(value));
+	if (wdt->is_modern_chip) {
+		wdt_write(wdt, AT91_WDT_WLR,
+			  AT91_WDT_SET_COUNTER(value));
 
 		wdd->timeout = timeout;
 		return 0;
 	}
 
 	wdt->mr &= ~AT91_WDT_WDV;
 	wdt->mr |= AT91_WDT_SET_WDV(value);
 
 	/*
 	 * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: When
 	 * setting the WDDIS bit, and while it is set, the fields WDV and WDD
 	 * must not be modified.
 	 * If the watchdog is enabled, then the timeout can be updated. Else,
 	 * wait that the user enables it.
 	 */
-	if (wdt_enabled)
-		wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
+	if (wdt_enabled(wdt, wdt->mr))
+		wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~wdt->wddis_mask);
 
 	wdd->timeout = timeout;
 
 	return 0;
 }
 
 static const struct watchdog_info sama5d4_wdt_info = {
 	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
 	.identity = "Atmel SAMA5D4 Watchdog",
 };
 
 static const struct watchdog_ops sama5d4_wdt_ops = {
 	.owner = THIS_MODULE,
-	.start = sama5d4_wdt_start,
-	.stop = sama5d4_wdt_stop,
-	.ping = sama5d4_wdt_ping,
-	.set_timeout = sama5d4_wdt_set_timeout,
+	.start = wdt_start,
+	.stop = wdt_stop,
+	.ping = wdt_ping,
+	.set_timeout = wdt_set_timeout,
 };
 
-static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
+static irqreturn_t wdt_irq_handler(int irq, void *dev_id)
 {
 	struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
 	u32 reg;
 
-	if (wdt->sam9x60_support)
-		reg = wdt_read(wdt, AT91_SAM9X60_ISR);
+	if (wdt->is_modern_chip)
+		reg = wdt_read(wdt, AT91_WDT_ISR);
 	else
 		reg = wdt_read(wdt, AT91_WDT_SR);
 
 	if (reg) {
 		pr_crit("Atmel Watchdog Software Reset\n");
 		emergency_restart();
 		pr_crit("Reboot didn't succeed\n");
 	}
 
 	return IRQ_HANDLED;
 }
 
-static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
+static int of_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
 {
 	const char *tmp;
 
-	if (wdt->sam9x60_support)
-		wdt->mr = AT91_SAM9X60_WDDIS;
-	else
-		wdt->mr = AT91_WDT_WDDIS;
+	wdt->mr = wdt->wddis_mask;
 
 	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
 	    !strcmp(tmp, "software"))
 		wdt->need_irq = true;
 
 	if (of_property_read_bool(np, "atmel,idle-halt"))
 		wdt->mr |= AT91_WDT_WDIDLEHLT;
 
 	if (of_property_read_bool(np, "atmel,dbg-halt"))
 		wdt->mr |= AT91_WDT_WDDBGHLT;
 
 	return 0;
 }
 
-static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
+static int wdt_init(struct sama5d4_wdt *wdt)
 {
+	struct watchdog_device *wdd = &wdt->wdd;
 	u32 reg, val;
 
 	val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT);
+
 	/*
 	 * When booting and resuming, the bootloader may have changed the
 	 * watchdog configuration.
-	 * If the watchdog is already running, we can safely update it.
-	 * Else, we have to disable it properly.
+	 * If the watchdog is not running, we need to disable it properly.
+	 * Otherwise, we can safely update it.
 	 */
-	if (!wdt_enabled) {
-		reg = wdt_read(wdt, AT91_WDT_MR);
-		if (wdt->sam9x60_support && (!(reg & AT91_SAM9X60_WDDIS)))
-			wdt_write_nosleep(wdt, AT91_WDT_MR,
-					  reg | AT91_SAM9X60_WDDIS);
-		else if (!wdt->sam9x60_support &&
-			 (!(reg & AT91_WDT_WDDIS)))
-			wdt_write_nosleep(wdt, AT91_WDT_MR,
-					  reg | AT91_WDT_WDDIS);
+	reg = wdt_read(wdt, AT91_WDT_MR);
+	if (wdt_enabled(wdt, reg)) {
+		wdt->mr &= ~wdt->wddis_mask;
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+	} else {
+		wdt->mr |= wdt->wddis_mask;
 	}
 
-	if (wdt->sam9x60_support) {
+	if (wdt->is_modern_chip) {
 		if (wdt->need_irq)
-			wdt->ir = AT91_SAM9X60_PERINT;
+			wdt->ir = AT91_WDT_PERINT;
 		else
-			wdt->mr |= AT91_SAM9X60_PERIODRST;
+			wdt->mr |= AT91_WDT_PERIODRST;
 
-		wdt_write(wdt, AT91_SAM9X60_IER, wdt->ir);
-		wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(val));
+		wdt_write(wdt, AT91_WDT_IER, wdt->ir);
+		wdt_write(wdt, AT91_WDT_WLR, AT91_WDT_SET_COUNTER(val));
 	} else {
 		wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT));
 		wdt->mr |= AT91_WDT_SET_WDV(val);
 
 		if (wdt->need_irq)
 			wdt->mr |= AT91_WDT_WDFIEN;
 		else
 			wdt->mr |= AT91_WDT_WDRSTEN;
 	}
 
 	wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
 
 	return 0;
 }
 
-static int sama5d4_wdt_probe(struct platform_device *pdev)
+static int wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct watchdog_device *wdd;
 	struct sama5d4_wdt *wdt;
 	void __iomem *regs;
 	u32 irq = 0;
-	u32 reg;
 	int ret;
 
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
 		return -ENOMEM;
 
 	wdd = &wdt->wdd;
 	wdd->timeout = WDT_DEFAULT_TIMEOUT;
 	wdd->info = &sama5d4_wdt_info;
 	wdd->ops = &sama5d4_wdt_ops;
 	wdd->min_timeout = MIN_WDT_TIMEOUT;
 	wdd->max_timeout = MAX_WDT_TIMEOUT;
 	wdt->last_ping = jiffies;
-
-	if (of_device_is_compatible(dev->of_node, "microchip,sam9x60-wdt") ||
-	    of_device_is_compatible(dev->of_node, "microchip,sama7g5-wdt"))
-		wdt->sam9x60_support = true;
+	wdt->is_modern_chip = check_is_modern_chip_by_compatible(dev);
+	if (wdt->is_modern_chip)
+		wdt->wddis_mask = AT91_WDT_WDDIS_MODERN;
+	else
+		wdt->wddis_mask = AT91_WDT_WDDIS_LEGACY;
 
 	watchdog_set_drvdata(wdd, wdt);
 
 	regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
 	wdt->reg_base = regs;
 
-	ret = of_sama5d4_wdt_init(dev->of_node, wdt);
+	ret = of_wdt_init(dev->of_node, wdt);
 	if (ret)
 		return ret;
 
 	if (wdt->need_irq) {
 		irq = irq_of_parse_and_map(dev->of_node, 0);
 		if (!irq) {
 			dev_warn(dev, "failed to get IRQ from DT\n");
 			wdt->need_irq = false;
 		}
 	}
 
 	if (wdt->need_irq) {
-		ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
+		ret = devm_request_irq(dev, irq, wdt_irq_handler,
 				       IRQF_SHARED | IRQF_IRQPOLL |
 				       IRQF_NO_SUSPEND, pdev->name, pdev);
 		if (ret) {
 			dev_err(dev, "cannot register interrupt handler\n");
 			return ret;
 		}
 	}
 
 	watchdog_init_timeout(wdd, wdt_timeout, dev);
 
-	reg = wdt_read(wdt, AT91_WDT_MR);
-	if (!(reg & AT91_WDT_WDDIS)) {
-		wdt->mr &= ~AT91_WDT_WDDIS;
-		set_bit(WDOG_HW_RUNNING, &wdd->status);
-	}
-
-	ret = sama5d4_wdt_init(wdt);
+	ret = wdt_init(wdt);
 	if (ret)
 		return ret;
 
 	watchdog_set_nowayout(wdd, nowayout);
 
 	watchdog_stop_on_unregister(wdd);
 	ret = devm_watchdog_register_device(dev, wdd);
 	if (ret)
 		return ret;
 
 	platform_set_drvdata(pdev, wdt);
 
 	dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
 		 wdd->timeout, nowayout);
 
 	return 0;
 }
 
-static const struct of_device_id sama5d4_wdt_of_match[] = {
+static const struct of_device_id wdt_of_match[] = {
 	{
 		.compatible = "atmel,sama5d4-wdt",
 	},
 	{
 		.compatible = "microchip,sam9x60-wdt",
 	},
 	{
 		.compatible = "microchip,sama7g5-wdt",
 	},
 
 	{ }
 };
-MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
+MODULE_DEVICE_TABLE(of, wdt_of_match);
 
-static int sama5d4_wdt_suspend_late(struct device *dev)
+static int wdt_suspend_late(struct device *dev)
 {
 	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
 
 	if (watchdog_active(&wdt->wdd))
-		sama5d4_wdt_stop(&wdt->wdd);
+		wdt_stop(&wdt->wdd);
 
 	return 0;
 }
 
-static int sama5d4_wdt_resume_early(struct device *dev)
+static int wdt_resume_early(struct device *dev)
 {
 	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
 
 	/*
 	 * FIXME: writing MR also pings the watchdog which may not be desired.
 	 * This should only be done when the registers are lost on suspend but
 	 * there is no way to get this information right now.
 	 */
-	sama5d4_wdt_init(wdt);
+	wdt_init(wdt);
 
 	if (watchdog_active(&wdt->wdd))
-		sama5d4_wdt_start(&wdt->wdd);
+		wdt_start(&wdt->wdd);
 
 	return 0;
 }
 
 static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
-	LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
-				 sama5d4_wdt_resume_early)
+	LATE_SYSTEM_SLEEP_PM_OPS(wdt_suspend_late,
+				 wdt_resume_early)
 };
 
 static struct platform_driver sama5d4_wdt_driver = {
-	.probe		= sama5d4_wdt_probe,
+	.probe		= wdt_probe,
 	.driver		= {
 		.name	= "sama5d4_wdt",
 		.pm	= pm_sleep_ptr(&sama5d4_wdt_pm_ops),
-		.of_match_table = sama5d4_wdt_of_match,
+		.of_match_table = wdt_of_match,
 	}
 };
 module_platform_driver(sama5d4_wdt_driver);
 
 MODULE_AUTHOR("Atmel Corporation");
 MODULE_DESCRIPTION("Atmel SAMA5D4 Watchdog Timer driver");
 MODULE_LICENSE("GPL v2");
-- 
2.34.1

[PATCH 3/3] watchdog: at91sam9_wdt: Rename AT91_WDT_WDDIS to AT91_WDT_WDDIS_LEGACY

[Balakrishnan Sambath]

Replace AT91_WDT_WDDIS with AT91_WDT_WDDIS_LEGACY to match the updated
bit definition naming.

Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
---
 drivers/watchdog/at91sam9_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index aba66b8e9d03..88a4c1ec99bd 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -162,20 +162,20 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 
 	tmp = wdt_read(wdt, AT91_WDT_MR);
 	if ((tmp & mask) != (wdt->mr & mask)) {
 		if (tmp == WDT_MR_RESET) {
 			wdt_write(wdt, AT91_WDT_MR, wdt->mr);
 			tmp = wdt_read(wdt, AT91_WDT_MR);
 		}
 	}
 
-	if (tmp & AT91_WDT_WDDIS) {
-		if (wdt->mr & AT91_WDT_WDDIS)
+	if (tmp & AT91_WDT_WDDIS_LEGACY) {
+		if (wdt->mr & AT91_WDT_WDDIS_LEGACY)
 			return 0;
 		dev_err(dev, "watchdog is disabled\n");
 		return -EINVAL;
 	}
 
 	value = tmp & AT91_WDT_WDV;
 	delta = (tmp & AT91_WDT_WDD) >> 16;
 
 	if (delta < value)
@@ -297,20 +297,20 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 	} else {
 		wdt->mr |= AT91_WDT_WDRSTEN;
 	}
 
 	if (!of_property_read_string(np, "atmel,reset-type", &tmp) &&
 	    !strcmp(tmp, "proc"))
 		wdt->mr |= AT91_WDT_WDRPROC;
 
 	if (of_property_read_bool(np, "atmel,disable")) {
-		wdt->mr |= AT91_WDT_WDDIS;
-		wdt->mr_mask &= AT91_WDT_WDDIS;
+		wdt->mr |= AT91_WDT_WDDIS_LEGACY;
+		wdt->mr_mask &= AT91_WDT_WDDIS_LEGACY;
 	}
 
 	if (of_property_read_bool(np, "atmel,idle-halt"))
 		wdt->mr |= AT91_WDT_WDIDLEHLT;
 
 	if (of_property_read_bool(np, "atmel,dbg-halt"))
 		wdt->mr |= AT91_WDT_WDDBGHLT;
 
 	wdt->mr |= max | ((max - min) << 16);
-- 
2.34.1

Re: [PATCH 0/3] watchdog: at91/sama5d4: header cleanup and driver refactor

[Guenter Roeck]

On 2/26/26 23:31, Balakrishnan Sambath wrote:
> This series cleans up the AT91 watchdog header and refactors the
> sama5d4 watchdog driver.
> 
> The header reorganization introduces consistent register naming and
> makes the WDDIS bit handling explicit for modern (SAM9X60, SAMA7G5,
> SAM9X7) and legacy (SAMA5, AT91SAM9261) SoCs. The driver refactor
> improves readability and fixes the reset regression introduced by
> commit 266da53c35fc ("watchdog: sama5d4: readout initial state").
> 

That is inappropriate as a bug fix. Ther bug fix should come first,
in a form that can be backported, followed by an optional cleanup.

Guenter

> Andrei Simion (2):
>    watchdog: at91sam9_wdt.h: Cleanup the header file
>    watchdog: sama5d4_wdt: Refactor the driver
> 
> Balakrishnan Sambath (1):
>    watchdog: at91sam9_wdt: Rename AT91_WDT_WDDIS to AT91_WDT_WDDIS_LEGACY
> 
>   drivers/watchdog/at91sam9_wdt.c |   8 +-
>   drivers/watchdog/at91sam9_wdt.h |  65 +++++++------
>   drivers/watchdog/sama5d4_wdt.c  | 156 ++++++++++++++++----------------
>   3 files changed, 113 insertions(+), 116 deletions(-)
> 

Re: [PATCH 1/3] watchdog: at91sam9_wdt.h: Cleanup the header file

[Alexandre Belloni]

On 27/02/2026 13:01:14+0530, Balakrishnan Sambath wrote:
> From: Andrei Simion <andrei.simion@microchip.com>
> 
> This patch reorganizes the header file by renaming the registers using
> a general pattern also this patch simplifies the watchdog disable logic
> in the at91sam9_wdt.h header by differentiating between modern
> (SAM9X60, SAMA7G5, SAM9X7) and legacy (SAMA5, AT91SAM9261) chips based
> on the watchdog disable bit.
> For modern chips, the disable bit is at bit 12, while for legacy chips
> it is at bit 15.
> 
> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> [Remove Kconfig-based WDDIS selection and define explicit legacy and
> modern masks]
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  drivers/watchdog/at91sam9_wdt.h | 65 ++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
> index 298d545df1a1..1e0aeecb489f 100644
> --- a/drivers/watchdog/at91sam9_wdt.h
> +++ b/drivers/watchdog/at91sam9_wdt.h
> @@ -3,59 +3,58 @@
>   * drivers/watchdog/at91sam9_wdt.h
>   *
>   * Copyright (C) 2007 Andrew Victor
>   * Copyright (C) 2007 Atmel Corporation.
>   * Copyright (C) 2019 Microchip Technology Inc. and its subsidiaries
>   *
>   * Watchdog Timer (WDT) - System peripherals regsters.
>   * Based on AT91SAM9261 datasheet revision D.
>   * Based on SAM9X60 datasheet.
> + * Based on SAMA7G5 datasheet.
> + * Based on SAM9X75 datasheet.
>   *
>   */
>  
>  #ifndef AT91_WDT_H
>  #define AT91_WDT_H
>  
>  #include <linux/bits.h>
>  
> -#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
> -#define  AT91_WDT_WDRSTT	BIT(0)			/* Restart */
> -#define  AT91_WDT_KEY		(0xa5UL << 24)		/* KEY Password */
> -
> -#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
> -#define  AT91_WDT_WDV		(0xfffUL << 0)		/* Counter Value */
> -#define  AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
> -#define  AT91_SAM9X60_PERIODRST	BIT(4)		/* Period Reset */
> -#define  AT91_SAM9X60_RPTHRST	BIT(5)		/* Minimum Restart Period */
> +#define AT91_WDT_CR		0x00		/* Watchdog Control Register */
> +#define  AT91_WDT_WDRSTT	BIT(0)		/* Restart */
> +#define  AT91_WDT_KEY		(0xa5UL << 24)	/* KEY Password */
> +#define AT91_WDT_MR		0x04		/* Watchdog Mode Register */
> +#define  AT91_WDT_WDV		(0xfffUL << 0)	/* Counter Value */
>  #define  AT91_WDT_WDFIEN	BIT(12)		/* Fault Interrupt Enable */
> -#define  AT91_SAM9X60_WDDIS	BIT(12)		/* Watchdog Disable */
> -#define  AT91_WDT_WDRSTEN	BIT(13)		/* Reset Processor */
> -#define  AT91_WDT_WDRPROC	BIT(14)		/* Timer Restart */
> -#define  AT91_WDT_WDDIS		BIT(15)		/* Watchdog Disable */
> -#define  AT91_WDT_WDD		(0xfffUL << 16)		/* Delta Value */
> -#define  AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
> -#define  AT91_WDT_WDDBGHLT	BIT(28)		/* Debug Halt */
> -#define  AT91_WDT_WDIDLEHLT	BIT(29)		/* Idle Halt */
> -
> +#define  AT91_WDT_WDRSTEN	BIT(13)
> +#define  AT91_WDT_WDRPROC	BIT(14)
> +#define  AT91_WDT_WDD		(0xfffUL << 16)
> +#define  AT91_WDT_WDDBGHLT	BIT(28)
> +#define  AT91_WDT_WDIDLEHLT	BIT(29)
>  #define AT91_WDT_SR		0x08		/* Watchdog Status Register */
>  #define  AT91_WDT_WDUNF		BIT(0)		/* Watchdog Underflow */
>  #define  AT91_WDT_WDERR		BIT(1)		/* Watchdog Error */
>  
> -/* Watchdog Timer Value Register */
> -#define AT91_SAM9X60_VR		0x08
> +#define  AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
> +#define  AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
>  
> -/* Watchdog Window Level Register */
> -#define AT91_SAM9X60_WLR	0x0c
> -/* Watchdog Period Value */
> -#define  AT91_SAM9X60_COUNTER	(0xfffUL << 0)
> -#define  AT91_SAM9X60_SET_COUNTER(x)	((x) & AT91_SAM9X60_COUNTER)
> +#define AT91_WDT_VR		0x08	/* Watchdog Timer Value Register */
> +#define AT91_WDT_ISR		0x1c	/* Interrupt Status Register */
> +#define AT91_WDT_IER		0x14	/* Interrupt Enable Register */
> +#define AT91_WDT_IDR		0x18	/* Interrupt Disable Register */
> +#define AT91_WDT_WLR		0x0c	/* Watchdog Window Level Register */
> +#define AT91_WDT_PERIODRST	BIT(4)	/* Period Reset */
> +#define AT91_WDT_RPTHRST	BIT(5)		/* Minimum Restart Period */
> +#define  AT91_WDT_PERINT	BIT(0)	/* Period Interrupt Enable */
> +#define  AT91_WDT_COUNTER	(0xfffUL << 0)	/* Watchdog Period Value */
> +#define  AT91_WDT_SET_COUNTER(x)	((x) & AT91_WDT_COUNTER)
>  
> -/* Interrupt Enable Register */
> -#define AT91_SAM9X60_IER	0x14
> -/* Period Interrupt Enable */
> -#define  AT91_SAM9X60_PERINT	BIT(0)
> -/* Interrupt Disable Register */
> -#define AT91_SAM9X60_IDR	0x18
> -/* Interrupt Status Register */
> -#define AT91_SAM9X60_ISR	0x1c
> +/*
> + * WDDIS bit differs by SoC:
> + *   - SAMA5, AT91SAM9261: bit 15
> + *   - SAM9X60, SAMA7G5, SAM9X75: bit 12
> + * Select at runtime via compatible string.
> + */
> +#define AT91_WDT_WDDIS_LEGACY   BIT(15)
> +#define AT91_WDT_WDDIS_MODERN   BIT(12)

This is bad naming, we are going to end up with
AT91_WDT_WDDIS_LEGACY_LEGACY, AT91_WDT_WDDIS_MODERN_LEGACY and
AT91_WDT_WDDIS_MODERN next time. The proper name would use the name of
the SoC introducing the new position.

>  
>  #endif
> -- 
> 2.34.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] watchdog: sama5d4_wdt: Refactor the driver

[Alexandre Belloni]

On 27/02/2026 13:01:15+0530, Balakrishnan Sambath wrote:
> From: Andrei Simion <andrei.simion@microchip.com>
> 
> This patch cleans up and refactors the Atmel SAMA5D4 Watchdog Driver
> to be more readable and to fixup Reset issue introduced
> by commit 266da53c35fc ("watchdog: sama5d4: readout initial state").
> 
> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> [Use per-device WDDIS mask and sync MR shadow WDDIS state]
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  drivers/watchdog/sama5d4_wdt.c | 156 ++++++++++++++++-----------------
>  1 file changed, 77 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 13e72918338a..b422d0bd75f9 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -24,44 +24,58 @@
>  #define WDT_DEFAULT_TIMEOUT	MAX_WDT_TIMEOUT
>  
>  #define WDT_SEC2TICKS(s)	((s) ? (((s) << 8) - 1) : 0)
>  
>  struct sama5d4_wdt {
>  	struct watchdog_device	wdd;
>  	void __iomem		*reg_base;
>  	u32			mr;
>  	u32			ir;
> +	u32			wddis_mask;

Introducing this should simply remove the need for 3/3, use this instead
of AT91_WDT_WDDIS everywhere.

>  	unsigned long		last_ping;
>  	bool			need_irq;
> -	bool			sam9x60_support;
> +	bool			is_modern_chip;

The previous name was better.

>  };
>  
> +static inline bool wdt_enabled(struct sama5d4_wdt *wdt, u32 mr)
> +{
> +	return !(mr & wdt->wddis_mask);
> +}
> +

This is unrelated to the actual fix. This patch does to much and is
difficult to review and to backport, please separate the actual fix
about wddis and the rest.


>  static int wdt_timeout;
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  
>  module_param(wdt_timeout, int, 0);
>  MODULE_PARM_DESC(wdt_timeout,
>  	"Watchdog timeout in seconds. (default = "
>  	__MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
>  
>  module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout,
>  	"Watchdog cannot be stopped once started (default="
>  	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> -#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
>  
>  #define wdt_read(wdt, field) \
>  	readl_relaxed((wdt)->reg_base + (field))
>  
>  /* 4 slow clock periods is 4/32768 = 122.07µs*/
>  #define WDT_DELAY	usecs_to_jiffies(123)
>  
> +static bool check_is_modern_chip_by_compatible(struct device *dev)
> +{
> +	if (of_device_is_compatible(dev->of_node, "microchip,sam9x60-wdt") ||
> +	    of_device_is_compatible(dev->of_node, "microchip,sama7g5-wdt"))
> +		return true;
> +
> +	return false;
> +}
> +
>  static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
>  {
>  	/*
>  	 * WDT_CR and WDT_MR must not be modified within three slow clock
>  	 * periods following a restart of the watchdog performed by a write
>  	 * access in WDT_CR.
>  	 */
>  	while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
>  		usleep_range(30, 125);
> @@ -71,322 +85,306 @@ static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
>  
>  static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
>  {
>  	if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
>  		udelay(123);
>  	writel_relaxed(val, wdt->reg_base + field);
>  	wdt->last_ping = jiffies;
>  }
>  
> -static int sama5d4_wdt_start(struct watchdog_device *wdd)
> +static int wdt_start(struct watchdog_device *wdd)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>  
> -	if (wdt->sam9x60_support) {
> -		writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IER);
> -		wdt->mr &= ~AT91_SAM9X60_WDDIS;
> -	} else {
> -		wdt->mr &= ~AT91_WDT_WDDIS;
> -	}
> +	if (wdt->is_modern_chip)
> +		writel_relaxed(wdt->ir, wdt->reg_base + AT91_WDT_IER);
> +	wdt->mr &= ~wdt->wddis_mask;
>  	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>  
>  	return 0;
>  }
>  
> -static int sama5d4_wdt_stop(struct watchdog_device *wdd)
> +static int wdt_stop(struct watchdog_device *wdd)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>  
> -	if (wdt->sam9x60_support) {
> -		writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IDR);
> -		wdt->mr |= AT91_SAM9X60_WDDIS;
> -	} else {
> -		wdt->mr |= AT91_WDT_WDDIS;
> -	}
> +	if (wdt->is_modern_chip)
> +		writel_relaxed(wdt->ir, wdt->reg_base + AT91_WDT_IDR);
> +	wdt->mr |= wdt->wddis_mask;
>  	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>  
>  	return 0;
>  }
>  
> -static int sama5d4_wdt_ping(struct watchdog_device *wdd)
> +static int wdt_ping(struct watchdog_device *wdd)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>  
>  	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>  
>  	return 0;
>  }
>  
> -static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> -				 unsigned int timeout)
> +static int wdt_set_timeout(struct watchdog_device *wdd,
> +			   unsigned int timeout)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>  	u32 value = WDT_SEC2TICKS(timeout);
>  
> -	if (wdt->sam9x60_support) {
> -		wdt_write(wdt, AT91_SAM9X60_WLR,
> -			  AT91_SAM9X60_SET_COUNTER(value));
> +	if (wdt->is_modern_chip) {
> +		wdt_write(wdt, AT91_WDT_WLR,
> +			  AT91_WDT_SET_COUNTER(value));
>  
>  		wdd->timeout = timeout;
>  		return 0;
>  	}
>  
>  	wdt->mr &= ~AT91_WDT_WDV;
>  	wdt->mr |= AT91_WDT_SET_WDV(value);
>  
>  	/*
>  	 * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: When
>  	 * setting the WDDIS bit, and while it is set, the fields WDV and WDD
>  	 * must not be modified.
>  	 * If the watchdog is enabled, then the timeout can be updated. Else,
>  	 * wait that the user enables it.
>  	 */
> -	if (wdt_enabled)
> -		wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> +	if (wdt_enabled(wdt, wdt->mr))
> +		wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~wdt->wddis_mask);
>  
>  	wdd->timeout = timeout;
>  
>  	return 0;
>  }
>  
>  static const struct watchdog_info sama5d4_wdt_info = {
>  	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
>  	.identity = "Atmel SAMA5D4 Watchdog",
>  };
>  
>  static const struct watchdog_ops sama5d4_wdt_ops = {
>  	.owner = THIS_MODULE,
> -	.start = sama5d4_wdt_start,
> -	.stop = sama5d4_wdt_stop,
> -	.ping = sama5d4_wdt_ping,
> -	.set_timeout = sama5d4_wdt_set_timeout,
> +	.start = wdt_start,
> +	.stop = wdt_stop,
> +	.ping = wdt_ping,
> +	.set_timeout = wdt_set_timeout,
>  };
>  
> -static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
> +static irqreturn_t wdt_irq_handler(int irq, void *dev_id)
>  {
>  	struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
>  	u32 reg;
>  
> -	if (wdt->sam9x60_support)
> -		reg = wdt_read(wdt, AT91_SAM9X60_ISR);
> +	if (wdt->is_modern_chip)
> +		reg = wdt_read(wdt, AT91_WDT_ISR);
>  	else
>  		reg = wdt_read(wdt, AT91_WDT_SR);
>  
>  	if (reg) {
>  		pr_crit("Atmel Watchdog Software Reset\n");
>  		emergency_restart();
>  		pr_crit("Reboot didn't succeed\n");
>  	}
>  
>  	return IRQ_HANDLED;
>  }
>  
> -static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
> +static int of_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
>  {
>  	const char *tmp;
>  
> -	if (wdt->sam9x60_support)
> -		wdt->mr = AT91_SAM9X60_WDDIS;
> -	else
> -		wdt->mr = AT91_WDT_WDDIS;
> +	wdt->mr = wdt->wddis_mask;
>  
>  	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>  	    !strcmp(tmp, "software"))
>  		wdt->need_irq = true;
>  
>  	if (of_property_read_bool(np, "atmel,idle-halt"))
>  		wdt->mr |= AT91_WDT_WDIDLEHLT;
>  
>  	if (of_property_read_bool(np, "atmel,dbg-halt"))
>  		wdt->mr |= AT91_WDT_WDDBGHLT;
>  
>  	return 0;
>  }
>  
> -static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> +static int wdt_init(struct sama5d4_wdt *wdt)
>  {
> +	struct watchdog_device *wdd = &wdt->wdd;
>  	u32 reg, val;
>  
>  	val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT);
> +
>  	/*
>  	 * When booting and resuming, the bootloader may have changed the
>  	 * watchdog configuration.
> -	 * If the watchdog is already running, we can safely update it.
> -	 * Else, we have to disable it properly.
> +	 * If the watchdog is not running, we need to disable it properly.
> +	 * Otherwise, we can safely update it.
>  	 */
> -	if (!wdt_enabled) {
> -		reg = wdt_read(wdt, AT91_WDT_MR);
> -		if (wdt->sam9x60_support && (!(reg & AT91_SAM9X60_WDDIS)))
> -			wdt_write_nosleep(wdt, AT91_WDT_MR,
> -					  reg | AT91_SAM9X60_WDDIS);
> -		else if (!wdt->sam9x60_support &&
> -			 (!(reg & AT91_WDT_WDDIS)))
> -			wdt_write_nosleep(wdt, AT91_WDT_MR,
> -					  reg | AT91_WDT_WDDIS);
> +	reg = wdt_read(wdt, AT91_WDT_MR);
> +	if (wdt_enabled(wdt, reg)) {
> +		wdt->mr &= ~wdt->wddis_mask;
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +	} else {
> +		wdt->mr |= wdt->wddis_mask;
>  	}
>  
> -	if (wdt->sam9x60_support) {
> +	if (wdt->is_modern_chip) {
>  		if (wdt->need_irq)
> -			wdt->ir = AT91_SAM9X60_PERINT;
> +			wdt->ir = AT91_WDT_PERINT;
>  		else
> -			wdt->mr |= AT91_SAM9X60_PERIODRST;
> +			wdt->mr |= AT91_WDT_PERIODRST;
>  
> -		wdt_write(wdt, AT91_SAM9X60_IER, wdt->ir);
> -		wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(val));
> +		wdt_write(wdt, AT91_WDT_IER, wdt->ir);
> +		wdt_write(wdt, AT91_WDT_WLR, AT91_WDT_SET_COUNTER(val));
>  	} else {
>  		wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT));
>  		wdt->mr |= AT91_WDT_SET_WDV(val);
>  
>  		if (wdt->need_irq)
>  			wdt->mr |= AT91_WDT_WDFIEN;
>  		else
>  			wdt->mr |= AT91_WDT_WDRSTEN;
>  	}
>  
>  	wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
>  
>  	return 0;
>  }
>  
> -static int sama5d4_wdt_probe(struct platform_device *pdev)
> +static int wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct watchdog_device *wdd;
>  	struct sama5d4_wdt *wdt;
>  	void __iomem *regs;
>  	u32 irq = 0;
> -	u32 reg;
>  	int ret;
>  
>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>  	if (!wdt)
>  		return -ENOMEM;
>  
>  	wdd = &wdt->wdd;
>  	wdd->timeout = WDT_DEFAULT_TIMEOUT;
>  	wdd->info = &sama5d4_wdt_info;
>  	wdd->ops = &sama5d4_wdt_ops;
>  	wdd->min_timeout = MIN_WDT_TIMEOUT;
>  	wdd->max_timeout = MAX_WDT_TIMEOUT;
>  	wdt->last_ping = jiffies;
> -
> -	if (of_device_is_compatible(dev->of_node, "microchip,sam9x60-wdt") ||
> -	    of_device_is_compatible(dev->of_node, "microchip,sama7g5-wdt"))
> -		wdt->sam9x60_support = true;
> +	wdt->is_modern_chip = check_is_modern_chip_by_compatible(dev);
> +	if (wdt->is_modern_chip)
> +		wdt->wddis_mask = AT91_WDT_WDDIS_MODERN;
> +	else
> +		wdt->wddis_mask = AT91_WDT_WDDIS_LEGACY;
>  
>  	watchdog_set_drvdata(wdd, wdt);
>  
>  	regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(regs))
>  		return PTR_ERR(regs);
>  
>  	wdt->reg_base = regs;
>  
> -	ret = of_sama5d4_wdt_init(dev->of_node, wdt);
> +	ret = of_wdt_init(dev->of_node, wdt);
>  	if (ret)
>  		return ret;
>  
>  	if (wdt->need_irq) {
>  		irq = irq_of_parse_and_map(dev->of_node, 0);
>  		if (!irq) {
>  			dev_warn(dev, "failed to get IRQ from DT\n");
>  			wdt->need_irq = false;
>  		}
>  	}
>  
>  	if (wdt->need_irq) {
> -		ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
> +		ret = devm_request_irq(dev, irq, wdt_irq_handler,
>  				       IRQF_SHARED | IRQF_IRQPOLL |
>  				       IRQF_NO_SUSPEND, pdev->name, pdev);
>  		if (ret) {
>  			dev_err(dev, "cannot register interrupt handler\n");
>  			return ret;
>  		}
>  	}
>  
>  	watchdog_init_timeout(wdd, wdt_timeout, dev);
>  
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	if (!(reg & AT91_WDT_WDDIS)) {
> -		wdt->mr &= ~AT91_WDT_WDDIS;
> -		set_bit(WDOG_HW_RUNNING, &wdd->status);
> -	}
> -
> -	ret = sama5d4_wdt_init(wdt);
> +	ret = wdt_init(wdt);
>  	if (ret)
>  		return ret;
>  
>  	watchdog_set_nowayout(wdd, nowayout);
>  
>  	watchdog_stop_on_unregister(wdd);
>  	ret = devm_watchdog_register_device(dev, wdd);
>  	if (ret)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, wdt);
>  
>  	dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
>  		 wdd->timeout, nowayout);
>  
>  	return 0;
>  }
>  
> -static const struct of_device_id sama5d4_wdt_of_match[] = {
> +static const struct of_device_id wdt_of_match[] = {
>  	{
>  		.compatible = "atmel,sama5d4-wdt",
>  	},
>  	{
>  		.compatible = "microchip,sam9x60-wdt",
>  	},
>  	{
>  		.compatible = "microchip,sama7g5-wdt",
>  	},
>  
>  	{ }
>  };
> -MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
> +MODULE_DEVICE_TABLE(of, wdt_of_match);
>  
> -static int sama5d4_wdt_suspend_late(struct device *dev)
> +static int wdt_suspend_late(struct device *dev)
>  {
>  	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>  
>  	if (watchdog_active(&wdt->wdd))
> -		sama5d4_wdt_stop(&wdt->wdd);
> +		wdt_stop(&wdt->wdd);
>  
>  	return 0;
>  }
>  
> -static int sama5d4_wdt_resume_early(struct device *dev)
> +static int wdt_resume_early(struct device *dev)
>  {
>  	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>  
>  	/*
>  	 * FIXME: writing MR also pings the watchdog which may not be desired.
>  	 * This should only be done when the registers are lost on suspend but
>  	 * there is no way to get this information right now.
>  	 */
> -	sama5d4_wdt_init(wdt);
> +	wdt_init(wdt);
>  
>  	if (watchdog_active(&wdt->wdd))
> -		sama5d4_wdt_start(&wdt->wdd);
> +		wdt_start(&wdt->wdd);
>  
>  	return 0;
>  }
>  
>  static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
> -	LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
> -				 sama5d4_wdt_resume_early)
> +	LATE_SYSTEM_SLEEP_PM_OPS(wdt_suspend_late,
> +				 wdt_resume_early)
>  };
>  
>  static struct platform_driver sama5d4_wdt_driver = {
> -	.probe		= sama5d4_wdt_probe,
> +	.probe		= wdt_probe,
>  	.driver		= {
>  		.name	= "sama5d4_wdt",
>  		.pm	= pm_sleep_ptr(&sama5d4_wdt_pm_ops),
> -		.of_match_table = sama5d4_wdt_of_match,
> +		.of_match_table = wdt_of_match,
>  	}
>  };
>  module_platform_driver(sama5d4_wdt_driver);
>  
>  MODULE_AUTHOR("Atmel Corporation");
>  MODULE_DESCRIPTION("Atmel SAMA5D4 Watchdog Timer driver");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.34.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/3] watchdog: at91sam9_wdt.h: Cleanup the header file

[Balakrishnan.S]

Hi,

Thanks for the comments.

On 27/02/26 1:22 pm, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/02/2026 13:01:14+0530, Balakrishnan Sambath wrote:
>> From: Andrei Simion <andrei.simion@microchip.com>
>>
>> This patch reorganizes the header file by renaming the registers using
>> a general pattern also this patch simplifies the watchdog disable logic
>> in the at91sam9_wdt.h header by differentiating between modern
>> (SAM9X60, SAMA7G5, SAM9X7) and legacy (SAMA5, AT91SAM9261) chips based
>> on the watchdog disable bit.
>> For modern chips, the disable bit is at bit 12, while for legacy chips
>> it is at bit 15.
>>
>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>> [Remove Kconfig-based WDDIS selection and define explicit legacy and
>> modern masks]
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/watchdog/at91sam9_wdt.h | 65 ++++++++++++++++-----------------
>>   1 file changed, 32 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
>> index 298d545df1a1..1e0aeecb489f 100644
>> --- a/drivers/watchdog/at91sam9_wdt.h
>> +++ b/drivers/watchdog/at91sam9_wdt.h
>> @@ -3,59 +3,58 @@
>>    * drivers/watchdog/at91sam9_wdt.h
>>    *
>>    * Copyright (C) 2007 Andrew Victor
>>    * Copyright (C) 2007 Atmel Corporation.
>>    * Copyright (C) 2019 Microchip Technology Inc. and its subsidiaries
>>    *
>>    * Watchdog Timer (WDT) - System peripherals regsters.
>>    * Based on AT91SAM9261 datasheet revision D.
>>    * Based on SAM9X60 datasheet.
>> + * Based on SAMA7G5 datasheet.
>> + * Based on SAM9X75 datasheet.
>>    *
>>    */
>>
>>   #ifndef AT91_WDT_H
>>   #define AT91_WDT_H
>>
>>   #include <linux/bits.h>
>>
>> -#define AT91_WDT_CR          0x00                    /* Watchdog Control Register */
>> -#define  AT91_WDT_WDRSTT     BIT(0)                  /* Restart */
>> -#define  AT91_WDT_KEY                (0xa5UL << 24)          /* KEY Password */
>> -
>> -#define AT91_WDT_MR          0x04                    /* Watchdog Mode Register */
>> -#define  AT91_WDT_WDV                (0xfffUL << 0)          /* Counter Value */
>> -#define  AT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV)
>> -#define  AT91_SAM9X60_PERIODRST      BIT(4)          /* Period Reset */
>> -#define  AT91_SAM9X60_RPTHRST        BIT(5)          /* Minimum Restart Period */
>> +#define AT91_WDT_CR          0x00            /* Watchdog Control Register */
>> +#define  AT91_WDT_WDRSTT     BIT(0)          /* Restart */
>> +#define  AT91_WDT_KEY                (0xa5UL << 24)  /* KEY Password */
>> +#define AT91_WDT_MR          0x04            /* Watchdog Mode Register */
>> +#define  AT91_WDT_WDV                (0xfffUL << 0)  /* Counter Value */
>>   #define  AT91_WDT_WDFIEN     BIT(12)         /* Fault Interrupt Enable */
>> -#define  AT91_SAM9X60_WDDIS  BIT(12)         /* Watchdog Disable */
>> -#define  AT91_WDT_WDRSTEN    BIT(13)         /* Reset Processor */
>> -#define  AT91_WDT_WDRPROC    BIT(14)         /* Timer Restart */
>> -#define  AT91_WDT_WDDIS              BIT(15)         /* Watchdog Disable */
>> -#define  AT91_WDT_WDD                (0xfffUL << 16)         /* Delta Value */
>> -#define  AT91_WDT_SET_WDD(x) (((x) << 16) & AT91_WDT_WDD)
>> -#define  AT91_WDT_WDDBGHLT   BIT(28)         /* Debug Halt */
>> -#define  AT91_WDT_WDIDLEHLT  BIT(29)         /* Idle Halt */
>> -
>> +#define  AT91_WDT_WDRSTEN    BIT(13)
>> +#define  AT91_WDT_WDRPROC    BIT(14)
>> +#define  AT91_WDT_WDD                (0xfffUL << 16)
>> +#define  AT91_WDT_WDDBGHLT   BIT(28)
>> +#define  AT91_WDT_WDIDLEHLT  BIT(29)
>>   #define AT91_WDT_SR          0x08            /* Watchdog Status Register */
>>   #define  AT91_WDT_WDUNF              BIT(0)          /* Watchdog Underflow */
>>   #define  AT91_WDT_WDERR              BIT(1)          /* Watchdog Error */
>>
>> -/* Watchdog Timer Value Register */
>> -#define AT91_SAM9X60_VR              0x08
>> +#define  AT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV)
>> +#define  AT91_WDT_SET_WDD(x) (((x) << 16) & AT91_WDT_WDD)
>>
>> -/* Watchdog Window Level Register */
>> -#define AT91_SAM9X60_WLR     0x0c
>> -/* Watchdog Period Value */
>> -#define  AT91_SAM9X60_COUNTER        (0xfffUL << 0)
>> -#define  AT91_SAM9X60_SET_COUNTER(x) ((x) & AT91_SAM9X60_COUNTER)
>> +#define AT91_WDT_VR          0x08    /* Watchdog Timer Value Register */
>> +#define AT91_WDT_ISR         0x1c    /* Interrupt Status Register */
>> +#define AT91_WDT_IER         0x14    /* Interrupt Enable Register */
>> +#define AT91_WDT_IDR         0x18    /* Interrupt Disable Register */
>> +#define AT91_WDT_WLR         0x0c    /* Watchdog Window Level Register */
>> +#define AT91_WDT_PERIODRST   BIT(4)  /* Period Reset */
>> +#define AT91_WDT_RPTHRST     BIT(5)          /* Minimum Restart Period */
>> +#define  AT91_WDT_PERINT     BIT(0)  /* Period Interrupt Enable */
>> +#define  AT91_WDT_COUNTER    (0xfffUL << 0)  /* Watchdog Period Value */
>> +#define  AT91_WDT_SET_COUNTER(x)     ((x) & AT91_WDT_COUNTER)
>>
>> -/* Interrupt Enable Register */
>> -#define AT91_SAM9X60_IER     0x14
>> -/* Period Interrupt Enable */
>> -#define  AT91_SAM9X60_PERINT BIT(0)
>> -/* Interrupt Disable Register */
>> -#define AT91_SAM9X60_IDR     0x18
>> -/* Interrupt Status Register */
>> -#define AT91_SAM9X60_ISR     0x1c
>> +/*
>> + * WDDIS bit differs by SoC:
>> + *   - SAMA5, AT91SAM9261: bit 15
>> + *   - SAM9X60, SAMA7G5, SAM9X75: bit 12
>> + * Select at runtime via compatible string.
>> + */
>> +#define AT91_WDT_WDDIS_LEGACY   BIT(15)
>> +#define AT91_WDT_WDDIS_MODERN   BIT(12)
> 
> This is bad naming, we are going to end up with
> AT91_WDT_WDDIS_LEGACY_LEGACY, AT91_WDT_WDDIS_MODERN_LEGACY and
> AT91_WDT_WDDIS_MODERN next time. The proper name would use the name of
> the SoC introducing the new position.

Okay got it. If that's the case I'll retain the soC specific namings 
everywhere in v2.

> 
>>
>>   #endif
>> --
>> 2.34.1
>>
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH 2/3] watchdog: sama5d4_wdt: Refactor the driver

[Balakrishnan.S]

On 27/02/26 1:35 pm, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/02/2026 13:01:15+0530, Balakrishnan Sambath wrote:
>> From: Andrei Simion <andrei.simion@microchip.com>
>>
>> This patch cleans up and refactors the Atmel SAMA5D4 Watchdog Driver
>> to be more readable and to fixup Reset issue introduced
>> by commit 266da53c35fc ("watchdog: sama5d4: readout initial state").
>>
>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>> [Use per-device WDDIS mask and sync MR shadow WDDIS state]
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/watchdog/sama5d4_wdt.c | 156 ++++++++++++++++-----------------
>>   1 file changed, 77 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
>> index 13e72918338a..b422d0bd75f9 100644
>> --- a/drivers/watchdog/sama5d4_wdt.c
>> +++ b/drivers/watchdog/sama5d4_wdt.c
>> @@ -24,44 +24,58 @@
>>   #define WDT_DEFAULT_TIMEOUT  MAX_WDT_TIMEOUT
>>
>>   #define WDT_SEC2TICKS(s)     ((s) ? (((s) << 8) - 1) : 0)
>>
>>   struct sama5d4_wdt {
>>        struct watchdog_device  wdd;
>>        void __iomem            *reg_base;
>>        u32                     mr;
>>        u32                     ir;
>> +     u32                     wddis_mask;
> 
> Introducing this should simply remove the need for 3/3, use this instead
> of AT91_WDT_WDDIS everywhere.

Okay, I'll keep this.

> 
>>        unsigned long           last_ping;
>>        bool                    need_irq;
>> -     bool                    sam9x60_support;
>> +     bool                    is_modern_chip;
> 
> The previous name was better.

Got it, I'll address this as it seems meaningful to retain Soc level 
details too.
> 
>>   };
>>
>> +static inline bool wdt_enabled(struct sama5d4_wdt *wdt, u32 mr)
>> +{
>> +     return !(mr & wdt->wddis_mask);
>> +}
>> +
> 
> This is unrelated to the actual fix. This patch does to much and is
> difficult to review and to backport, please separate the actual fix
> about wddis and the rest.

My bad, I'll refactor the fix and refactor as separate chunks in v2.

> 
> 
>>   static int wdt_timeout;
>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>>
>>   module_param(wdt_timeout, int, 0);
>>   MODULE_PARM_DESC(wdt_timeout,
>>        "Watchdog timeout in seconds. (default = "
>>        __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
>>
>>   module_param(nowayout, bool, 0);
>>   MODULE_PARM_DESC(nowayout,
>>        "Watchdog cannot be stopped once started (default="
>>        __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>
>> -#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
>>
>>   #define wdt_read(wdt, field) \
>>        readl_relaxed((wdt)->reg_base + (field))
>>
>>   /* 4 slow clock periods is 4/32768 = 122.07µs*/
>>   #define WDT_DELAY    usecs_to_jiffies(123)
>>
>> +static bool check_is_modern_chip_by_compatible(struct device *dev)
>> +{
>> +     if (of_device_is_compatible(dev->of_node, "microchip,sam9x60-wdt") ||
>> +         of_device_is_compatible(dev->of_node, "microchip,sama7g5-wdt"))
>> +             return true;
>> +
>> +     return false;
>> +}
>> +
>>   static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
>>   {
>>        /*
>>         * WDT_CR and WDT_MR must not be modified within three slow clock
>>         * periods following a restart of the watchdog performed by a write
>>         * access in WDT_CR.
>>         */
>>        while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
>>                usleep_range(30, 125);
>> @@ -71,322 +85,306 @@ static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val)
>>
>>   static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val)
>>   {
>>        if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
>>                udelay(123);
>>        writel_relaxed(val, wdt->reg_base + field);
>>        wdt->last_ping = jiffies;
>>   }
>>
>> -static int sama5d4_wdt_start(struct watchdog_device *wdd)
>> +static int wdt_start(struct watchdog_device *wdd)
>>   {
>>        struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>>
>> -     if (wdt->sam9x60_support) {
>> -             writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IER);
>> -             wdt->mr &= ~AT91_SAM9X60_WDDIS;
>> -     } else {
>> -             wdt->mr &= ~AT91_WDT_WDDIS;
>> -     }
>> +     if (wdt->is_modern_chip)
>> +             writel_relaxed(wdt->ir, wdt->reg_base + AT91_WDT_IER);
>> +     wdt->mr &= ~wdt->wddis_mask;
>>        wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>>
>>        return 0;
>>   }
>>
>> -static int sama5d4_wdt_stop(struct watchdog_device *wdd)
>> +static int wdt_stop(struct watchdog_device *wdd)
>>   {
>>        struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>>
>> -     if (wdt->sam9x60_support) {
>> -             writel_relaxed(wdt->ir, wdt->reg_base + AT91_SAM9X60_IDR);
>> -             wdt->mr |= AT91_SAM9X60_WDDIS;
>> -     } else {
>> -             wdt->mr |= AT91_WDT_WDDIS;
>> -     }
>> +     if (wdt->is_modern_chip)
>> +             writel_relaxed(wdt->ir, wdt->reg_base + AT91_WDT_IDR);
>> +     wdt->mr |= wdt->wddis_mask;
>>        wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>>
>>        return 0;
>>   }
>>
>> -static int sama5d4_wdt_ping(struct watchdog_device *wdd)
>> +static int wdt_ping(struct watchdog_device *wdd)
>>   {
>>        struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>>
>>        wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>>
>>        return 0;
>>   }
>>
>> -static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
>> -                              unsigned int timeout)
>> +static int wdt_set_timeout(struct watchdog_device *wdd,
>> +                        unsigned int timeout)
>>   {
>>        struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>>        u32 value = WDT_SEC2TICKS(timeout);
>>
>> -     if (wdt->sam9x60_support) {
>> -             wdt_write(wdt, AT91_SAM9X60_WLR,
>> -                       AT91_SAM9X60_SET_COUNTER(value));
>> +     if (wdt->is_modern_chip) {
>> +             wdt_write(wdt, AT91_WDT_WLR,
>> +                       AT91_WDT_SET_COUNTER(value));
>>
>>                wdd->timeout = timeout;
>>                return 0;
>>        }
>>
>>        wdt->mr &= ~AT91_WDT_WDV;
>>        wdt->mr |= AT91_WDT_SET_WDV(value);
>>
>>        /*
>>         * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: When
>>         * setting the WDDIS bit, and while it is set, the fields WDV and WDD
>>         * must not be modified.
>>         * If the watchdog is enabled, then the timeout can be updated. Else,
>>         * wait that the user enables it.
>>         */
>> -     if (wdt_enabled)
>> -             wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>> +     if (wdt_enabled(wdt, wdt->mr))
>> +             wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~wdt->wddis_mask);
>>
>>        wdd->timeout = timeout;
>>
>>        return 0;
>>   }
>>
>>   static const struct watchdog_info sama5d4_wdt_info = {
>>        .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
>>        .identity = "Atmel SAMA5D4 Watchdog",
>>   };
>>
>>   static const struct watchdog_ops sama5d4_wdt_ops = {
>>        .owner = THIS_MODULE,
>> -     .start = sama5d4_wdt_start,
>> -     .stop = sama5d4_wdt_stop,
>> -     .ping = sama5d4_wdt_ping,
>> -     .set_timeout = sama5d4_wdt_set_timeout,
>> +     .start = wdt_start,
>> +     .stop = wdt_stop,
>> +     .ping = wdt_ping,
>> +     .set_timeout = wdt_set_timeout,
>>   };
>>
>> -static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
>> +static irqreturn_t wdt_irq_handler(int irq, void *dev_id)
>>   {
>>        struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
>>        u32 reg;
>>
>> -     if (wdt->sam9x60_support)
>> -             reg = wdt_read(wdt, AT91_SAM9X60_ISR);
>> +     if (wdt->is_modern_chip)
>> +             reg = wdt_read(wdt, AT91_WDT_ISR);
>>        else
>>                reg = wdt_read(wdt, AT91_WDT_SR);
>>
>>        if (reg) {
>>                pr_crit("Atmel Watchdog Software Reset\n");
>>                emergency_restart();
>>                pr_crit("Reboot didn't succeed\n");
>>        }
>>
>>        return IRQ_HANDLED;
>>   }
>>
>> -static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
>> +static int of_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
>>   {
>>        const char *tmp;
>>
>> -     if (wdt->sam9x60_support)
>> -             wdt->mr = AT91_SAM9X60_WDDIS;
>> -     else
>> -             wdt->mr = AT91_WDT_WDDIS;
>> +     wdt->mr = wdt->wddis_mask;
>>
>>        if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>>            !strcmp(tmp, "software"))
>>                wdt->need_irq = true;
>>
>>        if (of_property_read_bool(np, "atmel,idle-halt"))
>>                wdt->mr |= AT91_WDT_WDIDLEHLT;
>>
>>        if (of_property_read_bool(np, "atmel,dbg-halt"))
>>                wdt->mr |= AT91_WDT_WDDBGHLT;
>>
>>        return 0;
>>   }
>>
>> -static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
>> +static int wdt_init(struct sama5d4_wdt *wdt)
>>   {
>> +     struct watchdog_device *wdd = &wdt->wdd;
>>        u32 reg, val;
>>
>>        val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT);
>> +
>>        /*
>>         * When booting and resuming, the bootloader may have changed the
>>         * watchdog configuration.
>> -      * If the watchdog is already running, we can safely update it.
>> -      * Else, we have to disable it properly.
>> +      * If the watchdog is not running, we need to disable it properly.
>> +      * Otherwise, we can safely update it.
>>         */
>> -     if (!wdt_enabled) {
>> -             reg = wdt_read(wdt, AT91_WDT_MR);
>> -             if (wdt->sam9x60_support && (!(reg & AT91_SAM9X60_WDDIS)))
>> -                     wdt_write_nosleep(wdt, AT91_WDT_MR,
>> -                                       reg | AT91_SAM9X60_WDDIS);
>> -             else if (!wdt->sam9x60_support &&
>> -                      (!(reg & AT91_WDT_WDDIS)))
>> -                     wdt_write_nosleep(wdt, AT91_WDT_MR,
>> -                                       reg | AT91_WDT_WDDIS);
>> +     reg = wdt_read(wdt, AT91_WDT_MR);
>> +     if (wdt_enabled(wdt, reg)) {
>> +             wdt->mr &= ~wdt->wddis_mask;
>> +             set_bit(WDOG_HW_RUNNING, &wdd->status);
>> +     } else {
>> +             wdt->mr |= wdt->wddis_mask;
>>        }
>>
>> -     if (wdt->sam9x60_support) {
>> +     if (wdt->is_modern_chip) {
>>                if (wdt->need_irq)
>> -                     wdt->ir = AT91_SAM9X60_PERINT;
>> +                     wdt->ir = AT91_WDT_PERINT;
>>                else
>> -                     wdt->mr |= AT91_SAM9X60_PERIODRST;
>> +                     wdt->mr |= AT91_WDT_PERIODRST;
>>
>> -             wdt_write(wdt, AT91_SAM9X60_IER, wdt->ir);
>> -             wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(val));
>> +             wdt_write(wdt, AT91_WDT_IER, wdt->ir);
>> +             wdt_write(wdt, AT91_WDT_WLR, AT91_WDT_SET_COUNTER(val));
>>        } else {
>>                wdt->mr |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT));
>>                wdt->mr |= AT91_WDT_SET_WDV(val);
>>
>>                if (wdt->need_irq)
>>                        wdt->mr |= AT91_WDT_WDFIEN;
>>                else
>>                        wdt->mr |= AT91_WDT_WDRSTEN;
>>        }
>>
>>        wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
>>
>>        return 0;
>>   }
>>
>> -static int sama5d4_wdt_probe(struct platform_device *pdev)
>> +static int wdt_probe(struct platform_device *pdev)
>>   {
>>        struct device *dev = &pdev->dev;
>>        struct watchdog_device *wdd;
>>        struct sama5d4_wdt *wdt;
>>        void __iomem *regs;
>>        u32 irq = 0;
>> -     u32 reg;
>>        int ret;
>>
>>        wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>        if (!wdt)
>>                return -ENOMEM;
>>
>>        wdd = &wdt->wdd;
>>        wdd->timeout = WDT_DEFAULT_TIMEOUT;
>>        wdd->info = &sama5d4_wdt_info;
>>        wdd->ops = &sama5d4_wdt_ops;
>>        wdd->min_timeout = MIN_WDT_TIMEOUT;
>>        wdd->max_timeout = MAX_WDT_TIMEOUT;
>>        wdt->last_ping = jiffies;
>> -
>> -     if (of_device_is_compatible(dev->of_node, "microchip,sam9x60-wdt") ||
>> -         of_device_is_compatible(dev->of_node, "microchip,sama7g5-wdt"))
>> -             wdt->sam9x60_support = true;
>> +     wdt->is_modern_chip = check_is_modern_chip_by_compatible(dev);
>> +     if (wdt->is_modern_chip)
>> +             wdt->wddis_mask = AT91_WDT_WDDIS_MODERN;
>> +     else
>> +             wdt->wddis_mask = AT91_WDT_WDDIS_LEGACY;
>>
>>        watchdog_set_drvdata(wdd, wdt);
>>
>>        regs = devm_platform_ioremap_resource(pdev, 0);
>>        if (IS_ERR(regs))
>>                return PTR_ERR(regs);
>>
>>        wdt->reg_base = regs;
>>
>> -     ret = of_sama5d4_wdt_init(dev->of_node, wdt);
>> +     ret = of_wdt_init(dev->of_node, wdt);
>>        if (ret)
>>                return ret;
>>
>>        if (wdt->need_irq) {
>>                irq = irq_of_parse_and_map(dev->of_node, 0);
>>                if (!irq) {
>>                        dev_warn(dev, "failed to get IRQ from DT\n");
>>                        wdt->need_irq = false;
>>                }
>>        }
>>
>>        if (wdt->need_irq) {
>> -             ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
>> +             ret = devm_request_irq(dev, irq, wdt_irq_handler,
>>                                       IRQF_SHARED | IRQF_IRQPOLL |
>>                                       IRQF_NO_SUSPEND, pdev->name, pdev);
>>                if (ret) {
>>                        dev_err(dev, "cannot register interrupt handler\n");
>>                        return ret;
>>                }
>>        }
>>
>>        watchdog_init_timeout(wdd, wdt_timeout, dev);
>>
>> -     reg = wdt_read(wdt, AT91_WDT_MR);
>> -     if (!(reg & AT91_WDT_WDDIS)) {
>> -             wdt->mr &= ~AT91_WDT_WDDIS;
>> -             set_bit(WDOG_HW_RUNNING, &wdd->status);
>> -     }
>> -
>> -     ret = sama5d4_wdt_init(wdt);
>> +     ret = wdt_init(wdt);
>>        if (ret)
>>                return ret;
>>
>>        watchdog_set_nowayout(wdd, nowayout);
>>
>>        watchdog_stop_on_unregister(wdd);
>>        ret = devm_watchdog_register_device(dev, wdd);
>>        if (ret)
>>                return ret;
>>
>>        platform_set_drvdata(pdev, wdt);
>>
>>        dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
>>                 wdd->timeout, nowayout);
>>
>>        return 0;
>>   }
>>
>> -static const struct of_device_id sama5d4_wdt_of_match[] = {
>> +static const struct of_device_id wdt_of_match[] = {
>>        {
>>                .compatible = "atmel,sama5d4-wdt",
>>        },
>>        {
>>                .compatible = "microchip,sam9x60-wdt",
>>        },
>>        {
>>                .compatible = "microchip,sama7g5-wdt",
>>        },
>>
>>        { }
>>   };
>> -MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
>> +MODULE_DEVICE_TABLE(of, wdt_of_match);
>>
>> -static int sama5d4_wdt_suspend_late(struct device *dev)
>> +static int wdt_suspend_late(struct device *dev)
>>   {
>>        struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>>
>>        if (watchdog_active(&wdt->wdd))
>> -             sama5d4_wdt_stop(&wdt->wdd);
>> +             wdt_stop(&wdt->wdd);
>>
>>        return 0;
>>   }
>>
>> -static int sama5d4_wdt_resume_early(struct device *dev)
>> +static int wdt_resume_early(struct device *dev)
>>   {
>>        struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>>
>>        /*
>>         * FIXME: writing MR also pings the watchdog which may not be desired.
>>         * This should only be done when the registers are lost on suspend but
>>         * there is no way to get this information right now.
>>         */
>> -     sama5d4_wdt_init(wdt);
>> +     wdt_init(wdt);
>>
>>        if (watchdog_active(&wdt->wdd))
>> -             sama5d4_wdt_start(&wdt->wdd);
>> +             wdt_start(&wdt->wdd);
>>
>>        return 0;
>>   }
>>
>>   static const struct dev_pm_ops sama5d4_wdt_pm_ops = {
>> -     LATE_SYSTEM_SLEEP_PM_OPS(sama5d4_wdt_suspend_late,
>> -                              sama5d4_wdt_resume_early)
>> +     LATE_SYSTEM_SLEEP_PM_OPS(wdt_suspend_late,
>> +                              wdt_resume_early)
>>   };
>>
>>   static struct platform_driver sama5d4_wdt_driver = {
>> -     .probe          = sama5d4_wdt_probe,
>> +     .probe          = wdt_probe,
>>        .driver         = {
>>                .name   = "sama5d4_wdt",
>>                .pm     = pm_sleep_ptr(&sama5d4_wdt_pm_ops),
>> -             .of_match_table = sama5d4_wdt_of_match,
>> +             .of_match_table = wdt_of_match,
>>        }
>>   };
>>   module_platform_driver(sama5d4_wdt_driver);
>>
>>   MODULE_AUTHOR("Atmel Corporation");
>>   MODULE_DESCRIPTION("Atmel SAMA5D4 Watchdog Timer driver");
>>   MODULE_LICENSE("GPL v2");
>> --
>> 2.34.1
>>
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH 0/3] watchdog: at91/sama5d4: header cleanup and driver refactor

[Balakrishnan.S]

Hi,

On 27/02/26 1:14 pm, Guenter Roeck wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> On 2/26/26 23:31, Balakrishnan Sambath wrote:
>> This series cleans up the AT91 watchdog header and refactors the
>> sama5d4 watchdog driver.
>>
>> The header reorganization introduces consistent register naming and
>> makes the WDDIS bit handling explicit for modern (SAM9X60, SAMA7G5,
>> SAM9X7) and legacy (SAMA5, AT91SAM9261) SoCs. The driver refactor
>> improves readability and fixes the reset regression introduced by
>> commit 266da53c35fc ("watchdog: sama5d4: readout initial state").
>>
> 
> That is inappropriate as a bug fix. Ther bug fix should come first,
> in a form that can be backported, followed by an optional cleanup.

Thanks for the comments. I'll quickly fix this in v2 as suggested.
> 
> Guenter
> 
>> Andrei Simion (2):
>>    watchdog: at91sam9_wdt.h: Cleanup the header file
>>    watchdog: sama5d4_wdt: Refactor the driver
>>
>> Balakrishnan Sambath (1):
>>    watchdog: at91sam9_wdt: Rename AT91_WDT_WDDIS to AT91_WDT_WDDIS_LEGACY
>>
>>   drivers/watchdog/at91sam9_wdt.c |   8 +-
>>   drivers/watchdog/at91sam9_wdt.h |  65 +++++++------
>>   drivers/watchdog/sama5d4_wdt.c  | 156 ++++++++++++++++----------------
>>   3 files changed, 113 insertions(+), 116 deletions(-)
>>
>