[PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Daniel Lezcano]

Describe the Software Watchdog Timer available on the S32G platforms.

Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Cc: Thomas Fossati <thomas.fossati@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 .../bindings/watchdog/nxp,s32g-wdt.yaml       | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml b/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
new file mode 100644
index 000000000000..06ead743d5c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/nxp,s32g-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP Software Watchdog Timer
+
+maintainers:
+  - Daniel Lezcano <daniel.lezcano@kernel.org>
+
+description:
+  The System Timer Module supports commonly required system and
+  application software timing functions. STM includes a 32-bit
+  count-up timer and four 32-bit compare channels with a separate
+  interrupt source for each channel. The timer is driven by the STM
+
+allOf:
+  - $ref: watchdog.yaml#
+
+properties:
+  compatible:
+    enum:
+      - nxp,s32g-wdt
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    watchdog@0x40100000 {
+        compatible = "nxp,s32g-wdt";
+        reg = <0x40100000 0x1000>;
+        clocks = <&clks 0x3a>;
+        timeout-sec = <10>;
+    };
-- 
2.43.0

[PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Daniel Lezcano]

The S32 platform has several Software Watchdog Timer available and
tied with a CPU. The SWT0 is the only one which directly asserts the
reset line, other SWT require an external setup to configure the reset
behavior which is not part of this change.

The maximum watchdog value depends on the clock feeding the SWT
counter which is 32bits wide. On the s32g274-rb2, the clock has a rate
of 51MHz which lead to 83 seconds maximum timeout.

The timeout can be specified via the device tree with the usual
existing bindings 'timeout-sec' or via the module param timeout.

The watchdog can be loaded with the 'nowayout' option, preventing the
watchdog to be stopped.

The watchdog can be started at boot time with the 'early-enable'
option, thus letting the watchdog framework to service the watchdog
counter.

the watchdog support the magic character to stop when the userspace
releases the device.

Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Cc: Thomas Fossati <thomas.fossati@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/watchdog/Kconfig    |   9 +
 drivers/watchdog/Makefile   |   1 +
 drivers/watchdog/s32g_wdt.c | 362 ++++++++++++++++++++++++++++++++++++
 3 files changed, 372 insertions(+)
 create mode 100644 drivers/watchdog/s32g_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f81705f8539a..4ab4275ef49f 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -792,6 +792,15 @@ config IMX7ULP_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx7ulp_wdt.
 
+config S32G_WDT
+	tristate "S32G Watchdog"
+	depends on ARCH_S32 || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  This is the driver for the hardware watchdog on the NXP
+	  S32G platforms. If you wish to have watchdog support
+	  enabled, say Y, otherwise say N.
+
 config DB500_WATCHDOG
 	tristate "ST-Ericsson DB800 watchdog"
 	depends on MFD_DB8500_PRCMU
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 8411626fa162..d0f9826e32c3 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
 obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
 obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
+obj-$(CONFIG_S32G_WDT) += s32g_wdt.o
 obj-$(CONFIG_DB500_WATCHDOG) += db8500_wdt.o
 obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
 obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
diff --git a/drivers/watchdog/s32g_wdt.c b/drivers/watchdog/s32g_wdt.c
new file mode 100644
index 000000000000..87207b134c3e
--- /dev/null
+++ b/drivers/watchdog/s32g_wdt.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Watchdog driver for S32G SoC
+ *
+ * Copyright (C) 2014 Freescale Semiconductor, Inc.
+ * Copyright 2017-2019, 2021-2025 NXP.
+ *
+ */
+#include <linux/debugfs.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#define DRIVER_NAME "s32g-wdt"
+
+#define S32G_SWT_CR(__base)	(__base + 0x00)		/* Control Register offset	*/
+#define S32G_SWT_CR_SM		BIT(9) | BIT(10)	/* -> Service Mode		*/
+#define S32G_SWT_CR_STP		BIT(2)			/* -> Stop Mode Control		*/
+#define S32G_SWT_CR_FRZ		BIT(1)			/* -> Debug Mode Control	*/
+#define S32G_SWT_CR_WEN		BIT(0)			/* -> Watchdog Enable		*/
+
+#define S32G_SWT_TO(__base)	(__base + 0x08)		/* Timeout Register offset	*/
+
+#define S32G_SWT_SR(__base)	(__base + 0x10)		/* Service Register offset	*/
+#define S32G_WDT_SEQ1		0xA602			/* -> service sequence number 1	*/
+#define S32G_WDT_SEQ2		0xB480			/* -> service sequence number 2	*/
+
+#define S32G_SWT_CO(__base)	(__base + 0x14)		/* Counter output register	*/
+
+#define S32G_WDT_DEFAULT_TIMEOUT	30
+
+struct s32g_wdt_device {
+	int rate;
+	void __iomem *base;
+	struct watchdog_device wdog;
+};
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static unsigned int timeout_param = S32G_WDT_DEFAULT_TIMEOUT;
+module_param(timeout_param, uint, 0);
+MODULE_PARM_DESC(timeout_param, "Watchdog timeout in seconds (default="
+		 __MODULE_STRING(S32G_WDT_DEFAULT_TIMEOUT) ")");
+
+static bool early_enable = false;
+module_param(early_enable, bool, 0);
+MODULE_PARM_DESC(early_enable,
+		 "Watchdog is started on module insertion (default=false)");
+
+static const struct watchdog_info s32g_wdt_info = {
+	.identity = "s32g watchdog",
+	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
+	WDIOC_GETTIMEOUT | WDIOC_GETTIMELEFT,
+};
+
+#ifdef CONFIG_DEBUG_FS
+#define S32G_WDT_DEBUG_FS_REGS(__reg)		\
+{						\
+	.name = __stringify(__reg),		\
+	.offset = __reg(0),			\
+}
+
+static const struct debugfs_reg32 wdt_regs[] = {
+	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CR),
+	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_TO),
+	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CO),
+};
+
+static void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
+{
+	struct debugfs_regset32 *regset;
+	static struct dentry *dentry = NULL;
+
+	if (!dentry)
+		dentry = debugfs_create_dir("watchdog", NULL);
+
+	dentry = debugfs_create_dir(dev_name(dev), dentry);
+
+	regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
+	if (!regset)
+		return;
+
+	regset->base = wdev->base;
+	regset->regs = wdt_regs;
+	regset->nregs = ARRAY_SIZE(wdt_regs);
+
+	debugfs_create_regset32("registers", 0400, dentry, regset);
+}
+#else
+static inline void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
+{
+}
+#endif
+
+static struct s32g_wdt_device *wdd_to_s32g_wdt(struct watchdog_device *wdd)
+{
+	return container_of(wdd, struct s32g_wdt_device, wdog);
+}
+
+static unsigned int wdog_sec_to_count(struct s32g_wdt_device *wdev, unsigned int timeout)
+{
+	return wdev->rate * timeout;
+}
+
+static int s32g_wdt_ping(struct watchdog_device *wdog)
+{
+	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
+
+	__raw_writel(S32G_WDT_SEQ1, S32G_SWT_SR(wdev->base));
+	__raw_writel(S32G_WDT_SEQ2, S32G_SWT_SR(wdev->base));
+
+	return 0;
+}
+
+static int s32g_wdt_start(struct watchdog_device *wdog)
+{
+	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
+	unsigned long val;
+
+	val = __raw_readl(S32G_SWT_CR(wdev->base));
+
+	val |= S32G_SWT_CR_WEN;
+
+	__raw_writel(val, S32G_SWT_CR(wdev->base));
+
+	return 0;
+}
+
+static int s32g_wdt_stop(struct watchdog_device *wdog)
+{
+	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
+	unsigned long val;
+
+	val = __raw_readl(S32G_SWT_CR(wdev->base));
+
+	val &= ~S32G_SWT_CR_WEN;
+
+	__raw_writel(val, S32G_SWT_CR(wdev->base));
+
+	return 0;
+}
+
+static int s32g_wdt_set_timeout(struct watchdog_device *wdog, unsigned int timeout)
+{
+	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
+
+	__raw_writel(wdog_sec_to_count(wdev, timeout), S32G_SWT_TO(wdev->base));
+	
+	/*
+	 * Conforming to the documentation, the timeout counter is
+	 * loaded when servicing is operated or when the counter is
+	 * enabled. In case the watchdog is already started it must be
+	 * stopped and started again to update the timeout
+	 * register. Here we choose to service the watchdog for
+	 * simpler code.
+	 */
+	return s32g_wdt_ping(wdog);
+}
+
+static unsigned int s32g_wdt_get_timeleft(struct watchdog_device *wdog)
+{
+	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
+	unsigned long val, counter;
+
+	/*
+	 * The counter output can be read only if the SWT is
+	 * disabled. Given the latency between the internal counter
+	 * and the counter output update, there can be very small
+	 * difference. However, we can accept this matter of fact
+	 * given the resolution is a second based unit for the output.
+	 */
+	val = __raw_readl(S32G_SWT_CR(wdev->base));
+
+	if (test_bit(S32G_SWT_CR_WEN, &val))
+		s32g_wdt_stop(wdog);
+
+	counter = __raw_readl(S32G_SWT_CO(wdev->base));
+
+	if (test_bit(S32G_SWT_CR_WEN, &val))
+		s32g_wdt_start(wdog);
+
+	return counter / wdev->rate;
+}
+
+static const struct watchdog_ops s32g_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= s32g_wdt_start,
+	.stop		= s32g_wdt_stop,
+	.ping		= s32g_wdt_ping,
+	.set_timeout	= s32g_wdt_set_timeout,
+	.get_timeleft	= s32g_wdt_get_timeleft,
+};
+
+static void s32g_wdt_init(struct s32g_wdt_device *wdev)
+{
+	unsigned long val;
+
+	/* Set the watchdog's Time-Out value */
+	val = wdog_sec_to_count(wdev, wdev->wdog.timeout);
+
+	__raw_writel(val, S32G_SWT_TO(wdev->base));
+
+	/*
+	 * Get the control register content. We are at init time, the
+	 * watchdog should not be started.
+	 */
+	val = __raw_readl(S32G_SWT_CR(wdev->base));
+
+	/*
+	 * We want to allow the watchdog timer to be stopped when
+	 * device enters debug mode.
+	 */
+	val |= S32G_SWT_CR_FRZ;
+
+	/*
+	 * However, when the CPU is in WFI or suspend mode, the
+	 * watchdog must continue. The documentation refers it as the
+	 * stopped mode.
+	 */
+	val &= ~S32G_SWT_CR_STP;
+
+	/*
+	 * Use Fixed Service Sequence to ping the watchdog which is
+	 * 0x00 configuration value for the service mode. It should be
+	 * already set because it is the default value but we reset it
+	 * in case.
+	 */
+	val &= ~S32G_SWT_CR_SM;
+
+	__raw_writel(val, S32G_SWT_CR(wdev->base));
+
+	/*
+	 * The watchdog must be started when the module is loaded,
+	 * leading to getting ride of the userspace control. The
+	 * watchdog framework will handle the pings. It is especially
+	 * handy for kernel development.
+	 */
+	if (early_enable) {
+		s32g_wdt_start(&wdev->wdog);
+		set_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
+	}
+}
+
+static int s32g_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct clk *clk;
+	struct s32g_wdt_device *wdev;
+	struct watchdog_device *wdog;
+	int ret;
+
+	wdev = devm_kzalloc(dev, sizeof(struct s32g_wdt_device), GFP_KERNEL);
+	if (!wdev)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdev->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(wdev->base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(wdev->base), "Can not get resource\n");
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "Can't get Watchdog clock\n");
+
+	wdev->rate = clk_get_rate(clk);
+	if (!wdev->rate) {
+		dev_err(dev, "Input clock rate is not valid\n");
+		return -EINVAL;
+	}
+
+	wdog = &wdev->wdog;
+	wdog->info = &s32g_wdt_info;
+	wdog->ops = &s32g_wdt_ops;
+
+	/*
+	 * The code converts the timeout into a counter a value, if
+	 * the value is less than 0x100, then it is clamped by the SWT
+	 * module, so it is safe to specify a zero value as the
+	 * minimum timeout.
+	 */
+	wdog->min_timeout = 0;
+
+	/*
+	 * The counter register is a 32 bits long, so the maximum
+	 * counter value is UINT_MAX and the timeout in second is the
+	 * value divided by the rate.
+	 *
+	 * For instance, a rate of 51MHz lead to 84 seconds maximum
+	 * timeout.
+	 */
+	wdog->max_timeout = UINT_MAX / wdev->rate;
+
+	/*
+	 * The module param and the DT 'timeout-sec' property will
+	 * override the default value if they are specified.
+	 */
+	ret = watchdog_init_timeout(wdog, timeout_param, dev);
+	if (ret)
+		return ret;
+
+	/*
+	 * As soon as the watchdog is started, there is no way to stop
+	 * it if the 'nowayout' option is set at boot time
+	 */
+	watchdog_set_nowayout(wdog, nowayout);
+
+	/*
+	 * The devm_ version of the watchdog_register_device()
+	 * function will call watchdog_unregister_device() when the
+	 * device is removed.
+	 */
+	watchdog_stop_on_unregister(wdog);
+
+	s32g_wdt_init(wdev);
+
+	/*
+	 * The debugfs will create a directory with the configured
+	 * watchdogs on the platform and a register file to give some
+	 * register content.
+	 */
+	s32g_wdt_debugfs_init(dev, wdev);
+
+	ret = devm_watchdog_register_device(dev, wdog);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot register watchdog device\n");
+
+	dev_info(dev, "S32G Watchdog Timer Registered. "
+		 "timeout=%ds, nowayout=%d, early_enable=%d\n",
+		 wdog->timeout, nowayout, early_enable);
+
+	return 0;
+}
+
+static const struct of_device_id s32g_wdt_dt_ids[] = {
+	{ .compatible = "nxp,s32g-wdt" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver s32g_wdt_driver = {
+	.probe = s32g_wdt_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = s32g_wdt_dt_ids,
+	},
+};
+
+module_platform_driver(s32g_wdt_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("Watchdog driver for S32G SoC");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRIVER_NAME);
-- 
2.43.0

Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Guenter Roeck]

On 3/28/25 08:15, Daniel Lezcano wrote:
> The S32 platform has several Software Watchdog Timer available and

Why "Software" ? This is a hardware watchdog, or am I missing something ?

> tied with a CPU. The SWT0 is the only one which directly asserts the
> reset line, other SWT require an external setup to configure the reset
> behavior which is not part of this change.
> 
> The maximum watchdog value depends on the clock feeding the SWT
> counter which is 32bits wide. On the s32g274-rb2, the clock has a rate
> of 51MHz which lead to 83 seconds maximum timeout.
> 
> The timeout can be specified via the device tree with the usual
> existing bindings 'timeout-sec' or via the module param timeout.
> 
> The watchdog can be loaded with the 'nowayout' option, preventing the
> watchdog to be stopped.
> 
> The watchdog can be started at boot time with the 'early-enable'
> option, thus letting the watchdog framework to service the watchdog
> counter.
> 
> the watchdog support the magic character to stop when the userspace
> releases the device.
> 
> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Cc: Thomas Fossati <thomas.fossati@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   drivers/watchdog/Kconfig    |   9 +
>   drivers/watchdog/Makefile   |   1 +
>   drivers/watchdog/s32g_wdt.c | 362 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 372 insertions(+)
>   create mode 100644 drivers/watchdog/s32g_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index f81705f8539a..4ab4275ef49f 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -792,6 +792,15 @@ config IMX7ULP_WDT
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called imx7ulp_wdt.
>   
> +config S32G_WDT
> +	tristate "S32G Watchdog"
> +	depends on ARCH_S32 || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  This is the driver for the hardware watchdog on the NXP
> +	  S32G platforms. If you wish to have watchdog support
> +	  enabled, say Y, otherwise say N.
> +
>   config DB500_WATCHDOG
>   	tristate "ST-Ericsson DB800 watchdog"
>   	depends on MFD_DB8500_PRCMU
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 8411626fa162..d0f9826e32c3 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>   obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
>   obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
>   obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
> +obj-$(CONFIG_S32G_WDT) += s32g_wdt.o
>   obj-$(CONFIG_DB500_WATCHDOG) += db8500_wdt.o
>   obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
>   obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
> diff --git a/drivers/watchdog/s32g_wdt.c b/drivers/watchdog/s32g_wdt.c
> new file mode 100644
> index 000000000000..87207b134c3e
> --- /dev/null
> +++ b/drivers/watchdog/s32g_wdt.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Watchdog driver for S32G SoC
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Copyright 2017-2019, 2021-2025 NXP.

Does this originate from out-of-tree code ?
If so, a reference would be helpful.

> + *
> + */
> +#include <linux/debugfs.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>

Alphabetic include file order, please.

> +
> +#define DRIVER_NAME "s32g-wdt"
> +
> +#define S32G_SWT_CR(__base)	(__base + 0x00)		/* Control Register offset	*/

checkpatch:
	CHECK: Macro argument '__base' may be better as '(__base)' to avoid precedence issues

> +#define S32G_SWT_CR_SM		BIT(9) | BIT(10)	/* -> Service Mode		*/

checkpatch:
	ERROR: Macros with complex values should be enclosed in parentheses

I am not going to comment on the other issues reported by checkpatch,
but I expect them to be fixed in the next version. I would strongly suggest
to run "checkpatch o--strict" on the patch and fix what it reports.


> +#define S32G_SWT_CR_STP		BIT(2)			/* -> Stop Mode Control		*/
> +#define S32G_SWT_CR_FRZ		BIT(1)			/* -> Debug Mode Control	*/
> +#define S32G_SWT_CR_WEN		BIT(0)			/* -> Watchdog Enable		*/
> +
> +#define S32G_SWT_TO(__base)	(__base + 0x08)		/* Timeout Register offset	*/
> +
> +#define S32G_SWT_SR(__base)	(__base + 0x10)		/* Service Register offset	*/
> +#define S32G_WDT_SEQ1		0xA602			/* -> service sequence number 1	*/
> +#define S32G_WDT_SEQ2		0xB480			/* -> service sequence number 2	*/
> +
> +#define S32G_SWT_CO(__base)	(__base + 0x14)		/* Counter output register	*/
> +
> +#define S32G_WDT_DEFAULT_TIMEOUT	30
> +
> +struct s32g_wdt_device {
> +	int rate;
> +	void __iomem *base;
> +	struct watchdog_device wdog;
> +};
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static unsigned int timeout_param = S32G_WDT_DEFAULT_TIMEOUT;
> +module_param(timeout_param, uint, 0);
> +MODULE_PARM_DESC(timeout_param, "Watchdog timeout in seconds (default="
> +		 __MODULE_STRING(S32G_WDT_DEFAULT_TIMEOUT) ")");
> +
> +static bool early_enable = false;
> +module_param(early_enable, bool, 0);
> +MODULE_PARM_DESC(early_enable,
> +		 "Watchdog is started on module insertion (default=false)");
> +
> +static const struct watchdog_info s32g_wdt_info = {
> +	.identity = "s32g watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +	WDIOC_GETTIMEOUT | WDIOC_GETTIMELEFT,
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +#define S32G_WDT_DEBUG_FS_REGS(__reg)		\
> +{						\
> +	.name = __stringify(__reg),		\
> +	.offset = __reg(0),			\
> +}
> +
> +static const struct debugfs_reg32 wdt_regs[] = {
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CR),
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_TO),
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CO),
> +};
> +
> +static void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
> +{
> +	struct debugfs_regset32 *regset;
> +	static struct dentry *dentry = NULL;
> +
> +	if (!dentry)
> +		dentry = debugfs_create_dir("watchdog", NULL);

That is a terribly generic debugfs directory name. That is unacceptable.
Pick a name that is driver specific.

> +
> +	dentry = debugfs_create_dir(dev_name(dev), dentry);
> +

Where is this removed if the driver is unloaded ?

Also, if the driver is built into the kernel, it seems to me that a second
instance will create a nested directory. That seems odd.

> +	regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
> +	if (!regset)
> +		return;
> +
> +	regset->base = wdev->base;
> +	regset->regs = wdt_regs;
> +	regset->nregs = ARRAY_SIZE(wdt_regs);
> +
> +	debugfs_create_regset32("registers", 0400, dentry, regset);
> +}
> +#else
> +static inline void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
> +{
> +}
> +#endif
> +
> +static struct s32g_wdt_device *wdd_to_s32g_wdt(struct watchdog_device *wdd)
> +{
> +	return container_of(wdd, struct s32g_wdt_device, wdog);
> +}
> +
> +static unsigned int wdog_sec_to_count(struct s32g_wdt_device *wdev, unsigned int timeout)
> +{
> +	return wdev->rate * timeout;
> +}
> +
> +static int s32g_wdt_ping(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +
> +	__raw_writel(S32G_WDT_SEQ1, S32G_SWT_SR(wdev->base));
> +	__raw_writel(S32G_WDT_SEQ2, S32G_SWT_SR(wdev->base));
> +
> +	return 0;
> +}
> +
> +static int s32g_wdt_start(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +	unsigned long val;
> +
> +	val = __raw_readl(S32G_SWT_CR(wdev->base));
> +
> +	val |= S32G_SWT_CR_WEN;
> +
> +	__raw_writel(val, S32G_SWT_CR(wdev->base));
> +
> +	return 0;
> +}
> +
> +static int s32g_wdt_stop(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +	unsigned long val;
> +
> +	val = __raw_readl(S32G_SWT_CR(wdev->base));
> +
> +	val &= ~S32G_SWT_CR_WEN;
> +
> +	__raw_writel(val, S32G_SWT_CR(wdev->base));
> +
> +	return 0;
> +}
> +
> +static int s32g_wdt_set_timeout(struct watchdog_device *wdog, unsigned int timeout)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +
> +	__raw_writel(wdog_sec_to_count(wdev, timeout), S32G_SWT_TO(wdev->base));
> +	
> +	/*
> +	 * Conforming to the documentation, the timeout counter is
> +	 * loaded when servicing is operated or when the counter is
> +	 * enabled. In case the watchdog is already started it must be
> +	 * stopped and started again to update the timeout
> +	 * register. Here we choose to service the watchdog for
> +	 * simpler code.
> +	 */
> +	return s32g_wdt_ping(wdog);

Either check if the watchdog is running, or add a note explaining that a ping
on a stopped watchdog does not have adverse effect.

> +}
> +
> +static unsigned int s32g_wdt_get_timeleft(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +	unsigned long val, counter;
> +
> +	/*
> +	 * The counter output can be read only if the SWT is
> +	 * disabled. Given the latency between the internal counter
> +	 * and the counter output update, there can be very small
> +	 * difference. However, we can accept this matter of fact
> +	 * given the resolution is a second based unit for the output.
> +	 */
> +	val = __raw_readl(S32G_SWT_CR(wdev->base));
> +
> +	if (test_bit(S32G_SWT_CR_WEN, &val))
> +		s32g_wdt_stop(wdog);

The watchdog core provides wdt_is_running() which would avoid the
extra i/o access.

> +
> +	counter = __raw_readl(S32G_SWT_CO(wdev->base));
> +
> +	if (test_bit(S32G_SWT_CR_WEN, &val))
> +		s32g_wdt_start(wdog);
> +
> +	return counter / wdev->rate;
> +}
> +
> +static const struct watchdog_ops s32g_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= s32g_wdt_start,
> +	.stop		= s32g_wdt_stop,
> +	.ping		= s32g_wdt_ping,
> +	.set_timeout	= s32g_wdt_set_timeout,
> +	.get_timeleft	= s32g_wdt_get_timeleft,
> +};
> +
> +static void s32g_wdt_init(struct s32g_wdt_device *wdev)
> +{
> +	unsigned long val;
> +
> +	/* Set the watchdog's Time-Out value */
> +	val = wdog_sec_to_count(wdev, wdev->wdog.timeout);
> +
> +	__raw_writel(val, S32G_SWT_TO(wdev->base));
> +
> +	/*
> +	 * Get the control register content. We are at init time, the
> +	 * watchdog should not be started.
> +	 */
> +	val = __raw_readl(S32G_SWT_CR(wdev->base));
> +
> +	/*
> +	 * We want to allow the watchdog timer to be stopped when
> +	 * device enters debug mode.
> +	 */
> +	val |= S32G_SWT_CR_FRZ;
> +
> +	/*
> +	 * However, when the CPU is in WFI or suspend mode, the
> +	 * watchdog must continue. The documentation refers it as the
> +	 * stopped mode.
> +	 */
> +	val &= ~S32G_SWT_CR_STP;
> +
> +	/*
> +	 * Use Fixed Service Sequence to ping the watchdog which is
> +	 * 0x00 configuration value for the service mode. It should be
> +	 * already set because it is the default value but we reset it
> +	 * in case.
> +	 */
> +	val &= ~S32G_SWT_CR_SM;
> +
> +	__raw_writel(val, S32G_SWT_CR(wdev->base));
> +
> +	/*
> +	 * The watchdog must be started when the module is loaded,
> +	 * leading to getting ride of the userspace control. The

ride ? And why does it _have_ to be started when the module is loaded ?

> +	 * watchdog framework will handle the pings. It is especially
> +	 * handy for kernel development.
> +	 */
> +	if (early_enable) {
> +		s32g_wdt_start(&wdev->wdog);
> +		set_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
> +	}
> +}
> +
> +static int s32g_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct clk *clk;
> +	struct s32g_wdt_device *wdev;
> +	struct watchdog_device *wdog;
> +	int ret;
> +
> +	wdev = devm_kzalloc(dev, sizeof(struct s32g_wdt_device), GFP_KERNEL);
> +	if (!wdev)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	wdev->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(wdev->base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(wdev->base), "Can not get resource\n");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "Can't get Watchdog clock\n");
> +
> +	wdev->rate = clk_get_rate(clk);
> +	if (!wdev->rate) {
> +		dev_err(dev, "Input clock rate is not valid\n");
> +		return -EINVAL;
> +	}
> +
> +	wdog = &wdev->wdog;
> +	wdog->info = &s32g_wdt_info;
> +	wdog->ops = &s32g_wdt_ops;
> +
> +	/*
> +	 * The code converts the timeout into a counter a value, if
> +	 * the value is less than 0x100, then it is clamped by the SWT
> +	 * module, so it is safe to specify a zero value as the
> +	 * minimum timeout.
> +	 */
> +	wdog->min_timeout = 0;
> +
> +	/*
> +	 * The counter register is a 32 bits long, so the maximum
> +	 * counter value is UINT_MAX and the timeout in second is the
> +	 * value divided by the rate.
> +	 *
> +	 * For instance, a rate of 51MHz lead to 84 seconds maximum
> +	 * timeout.
> +	 */
> +	wdog->max_timeout = UINT_MAX / wdev->rate;
> +
> +	/*
> +	 * The module param and the DT 'timeout-sec' property will
> +	 * override the default value if they are specified.
> +	 */
> +	ret = watchdog_init_timeout(wdog, timeout_param, dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * As soon as the watchdog is started, there is no way to stop
> +	 * it if the 'nowayout' option is set at boot time
> +	 */
> +	watchdog_set_nowayout(wdog, nowayout);
> +
> +	/*
> +	 * The devm_ version of the watchdog_register_device()
> +	 * function will call watchdog_unregister_device() when the
> +	 * device is removed.
> +	 */
> +	watchdog_stop_on_unregister(wdog);
> +
> +	s32g_wdt_init(wdev);
> +
> +	/*
> +	 * The debugfs will create a directory with the configured
> +	 * watchdogs on the platform and a register file to give some
> +	 * register content.
> +	 */
> +	s32g_wdt_debugfs_init(dev, wdev);
> +
> +	ret = devm_watchdog_register_device(dev, wdog);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot register watchdog device\n");
> +
> +	dev_info(dev, "S32G Watchdog Timer Registered. "
> +		 "timeout=%ds, nowayout=%d, early_enable=%d\n",
> +		 wdog->timeout, nowayout, early_enable);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id s32g_wdt_dt_ids[] = {
> +	{ .compatible = "nxp,s32g-wdt" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver s32g_wdt_driver = {
> +	.probe = s32g_wdt_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +		.of_match_table = s32g_wdt_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(s32g_wdt_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("Watchdog driver for S32G SoC");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);

Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Daniel Lezcano]

Hi Guenter,

thanks for your review

On 28/03/2025 19:10, Guenter Roeck wrote:
> On 3/28/25 08:15, Daniel Lezcano wrote:
>> The S32 platform has several Software Watchdog Timer available and
> 
> Why "Software" ? This is a hardware watchdog, or am I missing something ?

I have no idea why it is called 'Software' because it is indeed a 
hardware watchdog. It is how NXP called it in their technical reference 
manual.

>> tied with a CPU. The SWT0 is the only one which directly asserts the
>> reset line, other SWT require an external setup to configure the reset
>> behavior which is not part of this change.
>>
>> The maximum watchdog value depends on the clock feeding the SWT
>> counter which is 32bits wide. On the s32g274-rb2, the clock has a rate
>> of 51MHz which lead to 83 seconds maximum timeout.
>>
>> The timeout can be specified via the device tree with the usual
>> existing bindings 'timeout-sec' or via the module param timeout.
>>
>> The watchdog can be loaded with the 'nowayout' option, preventing the
>> watchdog to be stopped.
>>
>> The watchdog can be started at boot time with the 'early-enable'
>> option, thus letting the watchdog framework to service the watchdog
>> counter.
>>
>> the watchdog support the magic character to stop when the userspace
>> releases the device.
>>
>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>> Cc: Thomas Fossati <thomas.fossati@linaro.org>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---

[ ... ]

>> --- /dev/null
>> +++ b/drivers/watchdog/s32g_wdt.c
>> @@ -0,0 +1,362 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Watchdog driver for S32G SoC
>> + *
>> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
>> + * Copyright 2017-2019, 2021-2025 NXP.
> 
> Does this originate from out-of-tree code ?
> If so, a reference would be helpful.

Well, I kept the copyright but this implementation is mostly from scratch.

>> +#include <linux/debugfs.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/watchdog.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
> 
> Alphabetic include file order, please.
> 
>> +
>> +#define DRIVER_NAME "s32g-wdt"
>> +
>> +#define S32G_SWT_CR(__base)    (__base + 0x00)        /* Control 
>> Register offset    */
> 
> checkpatch:
>      CHECK: Macro argument '__base' may be better as '(__base)' to avoid 
> precedence issues

I'm not sure to get this one.

>> +#define S32G_SWT_CR_SM        BIT(9) | BIT(10)    /* -> Service 
>> Mode        */
> 
> checkpatch:
>      ERROR: Macros with complex values should be enclosed in parentheses
> 
> I am not going to comment on the other issues reported by checkpatch,
> but I expect them to be fixed in the next version. I would strongly suggest
> to run "checkpatch o--strict" on the patch and fix what it reports.

Sure, I will do that, thanks

[ ... ]

>> +static void s32g_wdt_debugfs_init(struct device *dev, struct 
>> s32g_wdt_device *wdev)
>> +{
>> +    struct debugfs_regset32 *regset;
>> +    static struct dentry *dentry = NULL;
>> +
>> +    if (!dentry)
>> +        dentry = debugfs_create_dir("watchdog", NULL);
> 
> That is a terribly generic debugfs directory name. That is unacceptable.
> Pick a name that is driver specific.

Why is it terrible ? We end up with:

watchdog/40100000.watchdog

There are 7 watchdogs on the platform, the directory is there to group 
them all. It seems to me it is self-explanatory, no ?

>> +
>> +    dentry = debugfs_create_dir(dev_name(dev), dentry);
>> +
> 
> Where is this removed if the driver is unloaded ?

Oh right, I missed it. Thanks for pointing this out.

> Also, if the driver is built into the kernel, it seems to me that a second
> instance will create a nested directory. That seems odd.

No, because there is the test above if (!dentry) which is a static variable.

[ ... ]

>> +static int s32g_wdt_set_timeout(struct watchdog_device *wdog, 
>> unsigned int timeout)
>> +{
>> +    struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
>> +
>> +    __raw_writel(wdog_sec_to_count(wdev, timeout), S32G_SWT_TO(wdev- 
>> >base));
>> +
>> +    /*
>> +     * Conforming to the documentation, the timeout counter is
>> +     * loaded when servicing is operated or when the counter is
>> +     * enabled. In case the watchdog is already started it must be
>> +     * stopped and started again to update the timeout
>> +     * register. Here we choose to service the watchdog for
>> +     * simpler code.
>> +     */
>> +    return s32g_wdt_ping(wdog);
> 
> Either check if the watchdog is running, or add a note explaining that a 
> ping
> on a stopped watchdog does not have adverse effect.

Ok, I think the comment is unclear. I'll provide a clarified version 
based on the documentation.

>> +}
>> +
>> +static unsigned int s32g_wdt_get_timeleft(struct watchdog_device *wdog)
>> +{
>> +    struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
>> +    unsigned long val, counter;
>> +
>> +    /*
>> +     * The counter output can be read only if the SWT is
>> +     * disabled. Given the latency between the internal counter
>> +     * and the counter output update, there can be very small
>> +     * difference. However, we can accept this matter of fact
>> +     * given the resolution is a second based unit for the output.
>> +     */
>> +    val = __raw_readl(S32G_SWT_CR(wdev->base));
>> +
>> +    if (test_bit(S32G_SWT_CR_WEN, &val))
>> +        s32g_wdt_stop(wdog);
> 
> The watchdog core provides wdt_is_running() which would avoid the
> extra i/o access.

Ok, thanks for the suggestion

>> +
>> +    counter = __raw_readl(S32G_SWT_CO(wdev->base));
>> +
>> +    if (test_bit(S32G_SWT_CR_WEN, &val))
>> +        s32g_wdt_start(wdog);
>> +
>> +    return counter / wdev->rate;
>> +}
>> +
>> +static const struct watchdog_ops s32g_wdt_ops = {
>> +    .owner        = THIS_MODULE,
>> +    .start        = s32g_wdt_start,
>> +    .stop        = s32g_wdt_stop,
>> +    .ping        = s32g_wdt_ping,
>> +    .set_timeout    = s32g_wdt_set_timeout,
>> +    .get_timeleft    = s32g_wdt_get_timeleft,
>> +};
>> +
>> +static void s32g_wdt_init(struct s32g_wdt_device *wdev)
>> +{
>> +    unsigned long val;
>> +
>> +    /* Set the watchdog's Time-Out value */
>> +    val = wdog_sec_to_count(wdev, wdev->wdog.timeout);
>> +
>> +    __raw_writel(val, S32G_SWT_TO(wdev->base));
>> +
>> +    /*
>> +     * Get the control register content. We are at init time, the
>> +     * watchdog should not be started.
>> +     */
>> +    val = __raw_readl(S32G_SWT_CR(wdev->base));
>> +
>> +    /*
>> +     * We want to allow the watchdog timer to be stopped when
>> +     * device enters debug mode.
>> +     */
>> +    val |= S32G_SWT_CR_FRZ;
>> +
>> +    /*
>> +     * However, when the CPU is in WFI or suspend mode, the
>> +     * watchdog must continue. The documentation refers it as the
>> +     * stopped mode.
>> +     */
>> +    val &= ~S32G_SWT_CR_STP;
>> +
>> +    /*
>> +     * Use Fixed Service Sequence to ping the watchdog which is
>> +     * 0x00 configuration value for the service mode. It should be
>> +     * already set because it is the default value but we reset it
>> +     * in case.
>> +     */
>> +    val &= ~S32G_SWT_CR_SM;
>> +
>> +    __raw_writel(val, S32G_SWT_CR(wdev->base));
>> +
>> +    /*
>> +     * The watchdog must be started when the module is loaded,
>> +     * leading to getting ride of the userspace control. The
> 
> ride ? And why does it _have_ to be started when the module is loaded ?

The comment is misleading. I meant when the 'early_enable' option is 
set, then the watchdog must be started.

>> +     * watchdog framework will handle the pings. It is especially
>> +     * handy for kernel development.
>> +     */
>> +    if (early_enable) {
>> +        s32g_wdt_start(&wdev->wdog);
>> +        set_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
>> +    }
>> +}
>> +

[ ... ]

Thanks

   -- Daniel


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Guenter Roeck]

On 3/28/25 12:42, Daniel Lezcano wrote:
> 
> Hi Guenter,
> 
> thanks for your review
> 
> On 28/03/2025 19:10, Guenter Roeck wrote:
>> On 3/28/25 08:15, Daniel Lezcano wrote:
>>> The S32 platform has several Software Watchdog Timer available and
>>
>> Why "Software" ? This is a hardware watchdog, or am I missing something ?
> 
> I have no idea why it is called 'Software' because it is indeed a hardware watchdog. It is how NXP called it in their technical reference manual.
> 

I guess it is because it resets the software. Please drop the term;
it is misleading.

>>> tied with a CPU. The SWT0 is the only one which directly asserts the
>>> reset line, other SWT require an external setup to configure the reset
>>> behavior which is not part of this change.
>>>
>>> The maximum watchdog value depends on the clock feeding the SWT
>>> counter which is 32bits wide. On the s32g274-rb2, the clock has a rate
>>> of 51MHz which lead to 83 seconds maximum timeout.
>>>
>>> The timeout can be specified via the device tree with the usual
>>> existing bindings 'timeout-sec' or via the module param timeout.
>>>
>>> The watchdog can be loaded with the 'nowayout' option, preventing the
>>> watchdog to be stopped.
>>>
>>> The watchdog can be started at boot time with the 'early-enable'
>>> option, thus letting the watchdog framework to service the watchdog
>>> counter.
>>>
>>> the watchdog support the magic character to stop when the userspace
>>> releases the device.
>>>
>>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>> Cc: Thomas Fossati <thomas.fossati@linaro.org>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
> 
> [ ... ]
> 
>>> --- /dev/null
>>> +++ b/drivers/watchdog/s32g_wdt.c
>>> @@ -0,0 +1,362 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Watchdog driver for S32G SoC
>>> + *
>>> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
>>> + * Copyright 2017-2019, 2021-2025 NXP.
>>
>> Does this originate from out-of-tree code ?
>> If so, a reference would be helpful.
> 
> Well, I kept the copyright but this implementation is mostly from scratch.
> 
I am not a copyright expert, but if this isn't derived from some other driver
it should not include old copyrights.

>>> +#include <linux/debugfs.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/moduleparam.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/watchdog.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>
>> Alphabetic include file order, please.
>>
>>> +
>>> +#define DRIVER_NAME "s32g-wdt"
>>> +
>>> +#define S32G_SWT_CR(__base)    (__base + 0x00)        /* Control Register offset    */
>>
>> checkpatch:
>>      CHECK: Macro argument '__base' may be better as '(__base)' to avoid precedence issues
> 
> I'm not sure to get this one.
> 

#define S32G_SWT_CR(__base)    ((__base) + 0x00)

>>> +#define S32G_SWT_CR_SM        BIT(9) | BIT(10)    /* -> Service Mode        */
>>
>> checkpatch:
>>      ERROR: Macros with complex values should be enclosed in parentheses
>>
>> I am not going to comment on the other issues reported by checkpatch,
>> but I expect them to be fixed in the next version. I would strongly suggest
>> to run "checkpatch o--strict" on the patch and fix what it reports.
> 
> Sure, I will do that, thanks
> 
> [ ... ]
> 
>>> +static void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
>>> +{
>>> +    struct debugfs_regset32 *regset;
>>> +    static struct dentry *dentry = NULL;
>>> +
>>> +    if (!dentry)
>>> +        dentry = debugfs_create_dir("watchdog", NULL);
>>
>> That is a terribly generic debugfs directory name. That is unacceptable.
>> Pick a name that is driver specific.
> 
> Why is it terrible ? We end up with:
> 
> watchdog/40100000.watchdog
> 
> There are 7 watchdogs on the platform, the directory is there to group them all. It seems to me it is self-explanatory, no ?
> 

It should be something like "s32g/40100000.watchdog". Someone could have some other watchdog
(say, a real software watchdog) in the system and decide to use the top level directory name
for whatever reason. The top level should be something driver specific, not a generic name.

>>> +
>>> +    dentry = debugfs_create_dir(dev_name(dev), dentry);
>>> +
>>
>> Where is this removed if the driver is unloaded ?
> 
> Oh right, I missed it. Thanks for pointing this out.
> 
>> Also, if the driver is built into the kernel, it seems to me that a second
>> instance will create a nested directory. That seems odd.
> 
> No, because there is the test above if (!dentry) which is a static variable.
> 

Yes, and then the second watchdog will create "watchdog/40100000.watchdog/40200000.watchdog"
or similar because dentry is overwritten with the reference to "40100000.watchdog"

Thanks,
Guenter

Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Daniel Lezcano]

On 28/03/2025 20:56, Guenter Roeck wrote:
> On 3/28/25 12:42, Daniel Lezcano wrote:
>>
>> Hi Guenter,
>>
>> thanks for your review
>>
>> On 28/03/2025 19:10, Guenter Roeck wrote:
>>> On 3/28/25 08:15, Daniel Lezcano wrote:
>>>> The S32 platform has several Software Watchdog Timer available and
>>>
>>> Why "Software" ? This is a hardware watchdog, or am I missing 
>>> something ?
>>
>> I have no idea why it is called 'Software' because it is indeed a 
>> hardware watchdog. It is how NXP called it in their technical 
>> reference manual.
>>
> 
> I guess it is because it resets the software. Please drop the term;
> it is misleading.

Ok, I will drop the term but keep a reference to the NXP documentation.

>>>> tied with a CPU. The SWT0 is the only one which directly asserts the
>>>> reset line, other SWT require an external setup to configure the reset
>>>> behavior which is not part of this change.
>>>>
>>>> The maximum watchdog value depends on the clock feeding the SWT
>>>> counter which is 32bits wide. On the s32g274-rb2, the clock has a rate
>>>> of 51MHz which lead to 83 seconds maximum timeout.
>>>>
>>>> The timeout can be specified via the device tree with the usual
>>>> existing bindings 'timeout-sec' or via the module param timeout.
>>>>
>>>> The watchdog can be loaded with the 'nowayout' option, preventing the
>>>> watchdog to be stopped.
>>>>
>>>> The watchdog can be started at boot time with the 'early-enable'
>>>> option, thus letting the watchdog framework to service the watchdog
>>>> counter.
>>>>
>>>> the watchdog support the magic character to stop when the userspace
>>>> releases the device.
>>>>
>>>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>>> Cc: Thomas Fossati <thomas.fossati@linaro.org>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>
>> [ ... ]
>>
>>>> --- /dev/null
>>>> +++ b/drivers/watchdog/s32g_wdt.c
>>>> @@ -0,0 +1,362 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +/*
>>>> + * Watchdog driver for S32G SoC
>>>> + *
>>>> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
>>>> + * Copyright 2017-2019, 2021-2025 NXP.
>>>
>>> Does this originate from out-of-tree code ?
>>> If so, a reference would be helpful.
>>
>> Well, I kept the copyright but this implementation is mostly from 
>> scratch.
>>
> I am not a copyright expert, but if this isn't derived from some other 
> driver
> it should not include old copyrights.

Ok, I'll put only the NXP copyright

[ ... ]

>>>> +static void s32g_wdt_debugfs_init(struct device *dev, struct 
>>>> s32g_wdt_device *wdev)
>>>> +{
>>>> +    struct debugfs_regset32 *regset;
>>>> +    static struct dentry *dentry = NULL;
>>>> +
>>>> +    if (!dentry)
>>>> +        dentry = debugfs_create_dir("watchdog", NULL);
>>>
>>> That is a terribly generic debugfs directory name. That is unacceptable.
>>> Pick a name that is driver specific.
>>
>> Why is it terrible ? We end up with:
>>
>> watchdog/40100000.watchdog
>>
>> There are 7 watchdogs on the platform, the directory is there to group 
>> them all. It seems to me it is self-explanatory, no ?
>>
> 
> It should be something like "s32g/40100000.watchdog". Someone could have 
> some other watchdog
> (say, a real software watchdog) in the system and decide to use the top 
> level directory name
> for whatever reason. The top level should be something driver specific, 
> not a generic name.

Apparently we can have a subsystem creating the subsystem directory.

For example regmap is created by the core framework:

ls /sys/kernel/debug/regmap
4009c240.pinctrl-map0  4009c240.pinctrl-map1  4009c240.pinctrl-map2 
4009c240.pinctrl-map3  4009c240.pinctrl-map4  4009c240.pinctrl-map5

drivers/base/regmap/regmap-debugfs.c
   regmap_debugfs_root = debugfs_create_dir("regmap", NULL);

Or pinctrl:

ls /sys/kernel/debug/pinctrl
4009c240.pinctrl  pinctrl-devices   pinctrl-handles   pinctrl-maps

drivers/pinctrl/core.c:
  debugfs_root = debugfs_create_dir("pinctrl", NULL);


Would it make sense to have the watchdog framework to create this 
topmost directory and have the driver to use an API like:

int watchdog_debugfs_add(struct watchdog_device *wdog);

with wdog->debugfs set by the caller.

No need to have more API. When the driver is unloaded it can destroy its 
own directory. And when the core watchdog framework exits, it can 
recursively destroy the topmost directory.


>>>> +
>>>> +    dentry = debugfs_create_dir(dev_name(dev), dentry);
>>>> +
>>>
>>> Where is this removed if the driver is unloaded ?
>>
>> Oh right, I missed it. Thanks for pointing this out.
>>
>>> Also, if the driver is built into the kernel, it seems to me that a 
>>> second
>>> instance will create a nested directory. That seems odd.
>>
>> No, because there is the test above if (!dentry) which is a static 
>> variable.
>>
> 
> Yes, and then the second watchdog will create 
> "watchdog/40100000.watchdog/40200000.watchdog"
> or similar because dentry is overwritten with the reference to 
> "40100000.watchdog"
> 
> Thanks,
> Guenter
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Rob Herring (Arm)]

On Fri, 28 Mar 2025 16:15:13 +0100, Daniel Lezcano wrote:
> Describe the Software Watchdog Timer available on the S32G platforms.
> 
> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Cc: Thomas Fossati <thomas.fossati@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  .../bindings/watchdog/nxp,s32g-wdt.yaml       | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.example.dts:18.29-23.11: Warning (unit_address_format): /example-0/watchdog@0x40100000: unit name should not have leading "0x"
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.example.dtb: watchdog@0x40100000: 'timeout-sec' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/watchdog/nxp,s32g-wdt.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250328151516.2219971-1-daniel.lezcano@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Krzysztof Kozlowski]

On 28/03/2025 16:15, Daniel Lezcano wrote:
> +
> +struct s32g_wdt_device {
> +	int rate;
> +	void __iomem *base;
> +	struct watchdog_device wdog;
> +};
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static unsigned int timeout_param = S32G_WDT_DEFAULT_TIMEOUT;
> +module_param(timeout_param, uint, 0);
> +MODULE_PARM_DESC(timeout_param, "Watchdog timeout in seconds (default="
> +		 __MODULE_STRING(S32G_WDT_DEFAULT_TIMEOUT) ")");

Timeout is provided by DT.

> +
> +static bool early_enable = false;
> +module_param(early_enable, bool, 0);
> +MODULE_PARM_DESC(early_enable,
> +		 "Watchdog is started on module insertion (default=false)");
> +
> +static const struct watchdog_info s32g_wdt_info = {
> +	.identity = "s32g watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +	WDIOC_GETTIMEOUT | WDIOC_GETTIMELEFT,
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +#define S32G_WDT_DEBUG_FS_REGS(__reg)		\
> +{						\
> +	.name = __stringify(__reg),		\
> +	.offset = __reg(0),			\
> +}
> +
> +static const struct debugfs_reg32 wdt_regs[] = {
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CR),
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_TO),
> +	S32G_WDT_DEBUG_FS_REGS(S32G_SWT_CO),
> +};
> +
> +static void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
> +{
> +	struct debugfs_regset32 *regset;
> +	static struct dentry *dentry = NULL;
> +
> +	if (!dentry)
> +		dentry = debugfs_create_dir("watchdog", NULL);
> +
> +	dentry = debugfs_create_dir(dev_name(dev), dentry);
> +
> +	regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
> +	if (!regset)
> +		return;
> +
> +	regset->base = wdev->base;
> +	regset->regs = wdt_regs;
> +	regset->nregs = ARRAY_SIZE(wdt_regs);
> +
> +	debugfs_create_regset32("registers", 0400, dentry, regset);
> +}
> +#else
> +static inline void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
> +{
> +}
> +#endif
> +
> +static struct s32g_wdt_device *wdd_to_s32g_wdt(struct watchdog_device *wdd)
> +{
> +	return container_of(wdd, struct s32g_wdt_device, wdog);
> +}
> +
> +static unsigned int wdog_sec_to_count(struct s32g_wdt_device *wdev, unsigned int timeout)
> +{
> +	return wdev->rate * timeout;
> +}
> +
> +static int s32g_wdt_ping(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +
> +	__raw_writel(S32G_WDT_SEQ1, S32G_SWT_SR(wdev->base));
> +	__raw_writel(S32G_WDT_SEQ2, S32G_SWT_SR(wdev->base));

I am confused why you do not use standard writel or don't have any
barriers here. I think this is very error prone and in general
discouraged practice (was for example raised by Arnd multiple times on
the lists).

> +
> +	return 0;
> +}
> +
> +static int s32g_wdt_start(struct watchdog_device *wdog)
> +{
> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
> +	unsigned long val;
> +
> +	val = __raw_readl(S32G_SWT_CR(wdev->base));
> +
> +	val |= S32G_SWT_CR_WEN;
> +
> +	__raw_writel(val, S32G_SWT_CR(wdev->base));
> +
> +	return 0;
> +}
> +

...

> +
> +static int s32g_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct clk *clk;
> +	struct s32g_wdt_device *wdev;
> +	struct watchdog_device *wdog;
> +	int ret;
> +
> +	wdev = devm_kzalloc(dev, sizeof(struct s32g_wdt_device), GFP_KERNEL);

sizeof(*)

> +	if (!wdev)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	wdev->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(wdev->base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(wdev->base), "Can not get resource\n");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "Can't get Watchdog clock\n");
> +
> +	wdev->rate = clk_get_rate(clk);
> +	if (!wdev->rate) {
> +		dev_err(dev, "Input clock rate is not valid\n");
> +		return -EINVAL;
> +	}
> +
> +	wdog = &wdev->wdog;
> +	wdog->info = &s32g_wdt_info;
> +	wdog->ops = &s32g_wdt_ops;
> +
> +	/*
> +	 * The code converts the timeout into a counter a value, if
> +	 * the value is less than 0x100, then it is clamped by the SWT
> +	 * module, so it is safe to specify a zero value as the
> +	 * minimum timeout.
> +	 */
> +	wdog->min_timeout = 0;
> +
> +	/*
> +	 * The counter register is a 32 bits long, so the maximum
> +	 * counter value is UINT_MAX and the timeout in second is the
> +	 * value divided by the rate.
> +	 *
> +	 * For instance, a rate of 51MHz lead to 84 seconds maximum
> +	 * timeout.
> +	 */
> +	wdog->max_timeout = UINT_MAX / wdev->rate;
> +
> +	/*
> +	 * The module param and the DT 'timeout-sec' property will
> +	 * override the default value if they are specified.
> +	 */
> +	ret = watchdog_init_timeout(wdog, timeout_param, dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * As soon as the watchdog is started, there is no way to stop
> +	 * it if the 'nowayout' option is set at boot time
> +	 */
> +	watchdog_set_nowayout(wdog, nowayout);
> +
> +	/*
> +	 * The devm_ version of the watchdog_register_device()
> +	 * function will call watchdog_unregister_device() when the
> +	 * device is removed.
> +	 */
> +	watchdog_stop_on_unregister(wdog);
> +
> +	s32g_wdt_init(wdev);
> +
> +	/*
> +	 * The debugfs will create a directory with the configured
> +	 * watchdogs on the platform and a register file to give some
> +	 * register content.
> +	 */
> +	s32g_wdt_debugfs_init(dev, wdev);
> +
> +	ret = devm_watchdog_register_device(dev, wdog);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot register watchdog device\n");
> +
> +	dev_info(dev, "S32G Watchdog Timer Registered. "
> +		 "timeout=%ds, nowayout=%d, early_enable=%d\n",
> +		 wdog->timeout, nowayout, early_enable);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id s32g_wdt_dt_ids[] = {
> +	{ .compatible = "nxp,s32g-wdt" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver s32g_wdt_driver = {
> +	.probe = s32g_wdt_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,

Drop, that's some ancient downstream code.

> +		.of_match_table = s32g_wdt_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(s32g_wdt_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("Watchdog driver for S32G SoC");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRIVER_NAME);

Drop, not needed. Fix your table module device Id instead... or start
from other recent driver as a skeleton to avoid repeating the same
issues we already fixed.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Krzysztof Kozlowski]

On 28/03/2025 16:15, Daniel Lezcano wrote:
> +description:
> +  The System Timer Module supports commonly required system and
> +  application software timing functions. STM includes a 32-bit
> +  count-up timer and four 32-bit compare channels with a separate
> +  interrupt source for each channel. The timer is driven by the STM
> +
> +allOf:
> +  - $ref: watchdog.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,s32g-wdt
This wasn't tested, so limited review - this also has wrong compatible,
There is no such soc as s32g in the kernel. If that's a new soc, come
with proper top-level bindings and some explanation, because I am sure
previously we talked with NXP that this is not s32g but something else.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Rob Herring]

On Fri, Mar 28, 2025 at 08:37:03PM -0500, Rob Herring (Arm) wrote:
> 
> On Fri, 28 Mar 2025 16:15:13 +0100, Daniel Lezcano wrote:
> > Describe the Software Watchdog Timer available on the S32G platforms.
> > 
> > Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Cc: Thomas Fossati <thomas.fossati@linaro.org>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  .../bindings/watchdog/nxp,s32g-wdt.yaml       | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.example.dts:18.29-23.11: Warning (unit_address_format): /example-0/watchdog@0x40100000: unit name should not have leading "0x"
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.example.dtb: watchdog@0x40100000: 'timeout-sec' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	from schema $id: http://devicetree.org/schemas/watchdog/nxp,s32g-wdt.yaml#

You need unevaluatedProperties instead of additionalProperties.

Rob

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Daniel Lezcano]

On 29/03/2025 06:04, Krzysztof Kozlowski wrote:
> On 28/03/2025 16:15, Daniel Lezcano wrote:
>> +description:
>> +  The System Timer Module supports commonly required system and
>> +  application software timing functions. STM includes a 32-bit
>> +  count-up timer and four 32-bit compare channels with a separate
>> +  interrupt source for each channel. The timer is driven by the STM
>> +
>> +allOf:
>> +  - $ref: watchdog.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - nxp,s32g-wdt
> This wasn't tested, so limited review - this also has wrong compatible,
> There is no such soc as s32g in the kernel. If that's a new soc, come
> with proper top-level bindings and some explanation, because I am sure
> previously we talked with NXP that this is not s32g but something else.

If I refer to Documentation/devicetree/bindings/arm/fsl.yaml

We found the entries:

       - description: S32G2 based Boards
         items:
           - enum:
               - nxp,s32g274a-evb
               - nxp,s32g274a-rdb2
           - const: nxp,s32g2

       - description: S32G3 based Boards
         items:
           - enum:
               - nxp,s32g399a-rdb3
           - const: nxp,s32g3

I guess it should nxp,s32g2-wdt and nxp,s32g3-wdt

Right ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Daniel Lezcano]

On 29/03/2025 18:12, Rob Herring wrote:
> On Fri, Mar 28, 2025 at 08:37:03PM -0500, Rob Herring (Arm) wrote:
>>
>> On Fri, 28 Mar 2025 16:15:13 +0100, Daniel Lezcano wrote:
>>> Describe the Software Watchdog Timer available on the S32G platforms.
>>>
>>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>> Cc: Thomas Fossati <thomas.fossati@linaro.org>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>>   .../bindings/watchdog/nxp,s32g-wdt.yaml       | 46 +++++++++++++++++++
>>>   1 file changed, 46 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
>>>
>>
>> My bot found errors running 'make dt_binding_check' on your patch:
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.example.dts:18.29-23.11: Warning (unit_address_format): /example-0/watchdog@0x40100000: unit name should not have leading "0x"
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.example.dtb: watchdog@0x40100000: 'timeout-sec' does not match any of the regexes: 'pinctrl-[0-9]+'
>> 	from schema $id: http://devicetree.org/schemas/watchdog/nxp,s32g-wdt.yaml#
> 
> You need unevaluatedProperties instead of additionalProperties.

Thanks!

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

[Daniel Lezcano]

On 29/03/2025 05:55, Krzysztof Kozlowski wrote:
> On 28/03/2025 16:15, Daniel Lezcano wrote:
>> +
>> +struct s32g_wdt_device {
>> +	int rate;
>> +	void __iomem *base;
>> +	struct watchdog_device wdog;
>> +};
>> +
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +module_param(nowayout, bool, 0);
>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +
>> +static unsigned int timeout_param = S32G_WDT_DEFAULT_TIMEOUT;
>> +module_param(timeout_param, uint, 0);
>> +MODULE_PARM_DESC(timeout_param, "Watchdog timeout in seconds (default="
>> +		 __MODULE_STRING(S32G_WDT_DEFAULT_TIMEOUT) ")");
> 
> Timeout is provided by DT.

Yes, but we may want to change the default timeout when loading the driver.

[ ... ]

>> +static int s32g_wdt_ping(struct watchdog_device *wdog)
>> +{
>> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
>> +
>> +	__raw_writel(S32G_WDT_SEQ1, S32G_SWT_SR(wdev->base));
>> +	__raw_writel(S32G_WDT_SEQ2, S32G_SWT_SR(wdev->base));
> 
> I am confused why you do not use standard writel or don't have any
> barriers here. I think this is very error prone and in general
> discouraged practice (was for example raised by Arnd multiple times on
> the lists).

Yes, that's a good point. I'll sort it out with Arnd

>> +static int s32g_wdt_start(struct watchdog_device *wdog)
>> +{
>> +	struct s32g_wdt_device *wdev = wdd_to_s32g_wdt(wdog);
>> +	unsigned long val;
>> +
>> +	val = __raw_readl(S32G_SWT_CR(wdev->base));
>> +
>> +	val |= S32G_SWT_CR_WEN;
>> +
>> +	__raw_writel(val, S32G_SWT_CR(wdev->base));
>> +
>> +	return 0;
>> +}
>> +
> 
> ...

[ ... ]

>> +static struct platform_driver s32g_wdt_driver = {
>> +	.probe = s32g_wdt_probe,
>> +	.driver = {
>> +		.name = DRIVER_NAME,
>> +		.owner = THIS_MODULE,
> 
> Drop, that's some ancient downstream code.
> 
>> +		.of_match_table = s32g_wdt_dt_ids,
>> +	},
>> +};
>> +
>> +module_platform_driver(s32g_wdt_driver);
>> +
>> +MODULE_AUTHOR("NXP");
>> +MODULE_DESCRIPTION("Watchdog driver for S32G SoC");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
> 
> Drop, not needed. Fix your table module device Id instead... or start
> from other recent driver as a skeleton to avoid repeating the same
> issues we already fixed.

Ok, thanks for spotting it



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Ghennadi Procopciuc]

On 3/28/2025 5:15 PM, Daniel Lezcano wrote:
> Describe the Software Watchdog Timer available on the S32G platforms.
> 
> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Cc: Thomas Fossati <thomas.fossati@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  .../bindings/watchdog/nxp,s32g-wdt.yaml       | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml b/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
> new file mode 100644
> index 000000000000..06ead743d5c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/nxp,s32g-wdt.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/nxp,s32g-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP Software Watchdog Timer
> +
> +maintainers:
> +  - Daniel Lezcano <daniel.lezcano@kernel.org>
> +
> +description:
> +  The System Timer Module supports commonly required system and
> +  application software timing functions. STM includes a 32-bit
> +  count-up timer and four 32-bit compare channels with a separate
> +  interrupt source for each channel. The timer is driven by the STM
> +

Please update the description, as this one refers to STM instead of SWT.

-- 
Regards,
Ghennadi

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Krzysztof Kozlowski]

On 31/03/2025 09:57, Daniel Lezcano wrote:
> On 29/03/2025 06:04, Krzysztof Kozlowski wrote:
>> On 28/03/2025 16:15, Daniel Lezcano wrote:
>>> +description:
>>> +  The System Timer Module supports commonly required system and
>>> +  application software timing functions. STM includes a 32-bit
>>> +  count-up timer and four 32-bit compare channels with a separate
>>> +  interrupt source for each channel. The timer is driven by the STM
>>> +
>>> +allOf:
>>> +  - $ref: watchdog.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - nxp,s32g-wdt
>> This wasn't tested, so limited review - this also has wrong compatible,
>> There is no such soc as s32g in the kernel. If that's a new soc, come
>> with proper top-level bindings and some explanation, because I am sure
>> previously we talked with NXP that this is not s32g but something else.
> 
> If I refer to Documentation/devicetree/bindings/arm/fsl.yaml
> 
> We found the entries:
> 
>        - description: S32G2 based Boards
>          items:
>            - enum:
>                - nxp,s32g274a-evb
>                - nxp,s32g274a-rdb2
>            - const: nxp,s32g2
> 
>        - description: S32G3 based Boards
>          items:
>            - enum:
>                - nxp,s32g399a-rdb3
>            - const: nxp,s32g3
> 
> I guess it should nxp,s32g2-wdt and nxp,s32g3-wdt
> 
Yes, with one being the fallback.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Daniel Lezcano]

On 31/03/2025 13:42, Krzysztof Kozlowski wrote:
> On 31/03/2025 09:57, Daniel Lezcano wrote:
>> On 29/03/2025 06:04, Krzysztof Kozlowski wrote:
>>> On 28/03/2025 16:15, Daniel Lezcano wrote:
>>>> +description:
>>>> +  The System Timer Module supports commonly required system and
>>>> +  application software timing functions. STM includes a 32-bit
>>>> +  count-up timer and four 32-bit compare channels with a separate
>>>> +  interrupt source for each channel. The timer is driven by the STM
>>>> +
>>>> +allOf:
>>>> +  - $ref: watchdog.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - nxp,s32g-wdt
>>> This wasn't tested, so limited review - this also has wrong compatible,
>>> There is no such soc as s32g in the kernel. If that's a new soc, come
>>> with proper top-level bindings and some explanation, because I am sure
>>> previously we talked with NXP that this is not s32g but something else.
>>
>> If I refer to Documentation/devicetree/bindings/arm/fsl.yaml
>>
>> We found the entries:
>>
>>         - description: S32G2 based Boards
>>           items:
>>             - enum:
>>                 - nxp,s32g274a-evb
>>                 - nxp,s32g274a-rdb2
>>             - const: nxp,s32g2
>>
>>         - description: S32G3 based Boards
>>           items:
>>             - enum:
>>                 - nxp,s32g399a-rdb3
>>             - const: nxp,s32g3
>>
>> I guess it should nxp,s32g2-wdt and nxp,s32g3-wdt
>>
> Yes, with one being the fallback.

Like that ?

properties:
   compatible:
     oneOf:
       - const: nxp,s32g2-wdt
       - items:
           - const: nxp,s32g3-wdt
           - const: nxp,s32g2-wdt

Why a fallback is needed for this case ? It is exactly the same hardware 
for s32g2 and s32g3. Could it be:

properties:
   compatible:
     oneOf:
       - const: nxp,s32g2-wdt
       - const: nxp,s32g3-wdt

?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] dt-bindings: watchdog: Add NXP Software Watchdog Timer

[Krzysztof Kozlowski]

On 01/04/2025 10:46, Daniel Lezcano wrote:
>> Yes, with one being the fallback.
> 
> Like that ?
> 
> properties:
>    compatible:
>      oneOf:
>        - const: nxp,s32g2-wdt
>        - items:
>            - const: nxp,s32g3-wdt
>            - const: nxp,s32g2-wdt

Yes

> 
> Why a fallback is needed for this case ? It is exactly the same hardware 
> for s32g2 and s32g3. Could it be:

Fallback is needed because it is exactly the same hardware.

> 
> properties:
>    compatible:
>      oneOf:
>        - const: nxp,s32g2-wdt
>        - const: nxp,s32g3-wdt

This tells hardware is different.

Best regards,
Krzysztof