Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform
From: Daniel Lezcano
Date: Fri Mar 28 2025 - 15:43:44 EST
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@xxxxxxx>
Cc: Thomas Fossati <thomas.fossati@xxxxxxxxxx>
Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
---
[ ... ]
--- /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