Re: [PATCH v4 2/3] drivers: watchdog: Add StarFive Watchdog driver

From: Guenter Roeck
Date: Wed Mar 08 2023 - 10:18:12 EST

On 3/8/23 07:07, Emil Renner Berthing wrote:
On Wed, 8 Mar 2023 at 04:43, Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> wrote:

Add watchdog driver for the StarFive JH7100 and JH7110 SoC.

Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>

Hi Xingyu,

Thanks for adding the JH7100 support. I tried it on my Starlight board
and it seems to work fine except systemd complains about not being
able to set a 10min timeout on reboot:
systemd-shutdown[1]: Using hardware watchdog 'StarFive Watchdog',
version 0, device /dev/watchdog0
systemd-shutdown[1]: Failed to set timeout to 10min: Invalid argument
systemd-shutdown[1]: Syncing filesystems and block devices.
systemd-shutdown[1]: Sending SIGTERM to remaining processes...

The systemd runtime watchdog seems to work, so I guess this is just
because 10min is too long a timeout for the StarFive watchdog.


Correct, the driver would have to be implemented slightly differently
for the watchdog subsystem to accept larger timeout values.

More comments below.

---
MAINTAINERS | 7 +
drivers/watchdog/Kconfig | 9 +
drivers/watchdog/Makefile | 2 +
drivers/watchdog/starfive-wdt.c | 675 ++++++++++++++++++++++++++++++++
4 files changed, 693 insertions(+)
create mode 100644 drivers/watchdog/starfive-wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d5bc223f305..721d0e4e8a0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19962,6 +19962,13 @@ S: Supported
F: Documentation/devicetree/bindings/rng/starfive*
F: drivers/char/hw_random/jh7110-trng.c

+STARFIVE WATCHDOG DRIVER
+M: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
+M: Samin Guo <samin.guo@xxxxxxxxxxxxxxxx>
+S: Supported
+F: Documentation/devicetree/bindings/watchdog/starfive*
+F: drivers/watchdog/starfive-wdt.c
+
STATIC BRANCH/CALL
M: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
M: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f0872970daf9..9d825ffaf292 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -2090,6 +2090,15 @@ config UML_WATCHDOG
tristate "UML watchdog"
depends on UML || COMPILE_TEST

+config STARFIVE_WATCHDOG
+ tristate "StarFive Watchdog support"
+ depends on ARCH_STARFIVE || COMPILE_TEST
+ select WATCHDOG_CORE
+ default ARCH_STARFIVE
+ help
+ Say Y here to support the watchdog of StarFive JH7100 and JH7110
+ SoC. This driver can also be built as a module if choose M.

This file seems to be sorted by architecture, so you probably want to
add something like this at the appropriate place

# RISC-V Architecture

config STARFIVE_WATCHDOG
...


#
# ISA-based Watchdog Cards
#
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9cbf6580f16c..4c0bd377e92a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -211,6 +211,8 @@ obj-$(CONFIG_WATCHDOG_SUN4V) += sun4v_wdt.o
# Xen
obj-$(CONFIG_XEN_WDT) += xen_wdt.o

+obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o

Again please follow the layout of the file. Eg. something like this at
the appropriate place

# RISC-V Architecture
obj-$(CONFIG_STARFIVE_WATCHDOG) += starfive-wdt.o

# Architecture Independent
obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
new file mode 100644
index 000000000000..8ce9f985f068
--- /dev/null
+++ b/drivers/watchdog/starfive-wdt.c
@@ -0,0 +1,675 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Starfive Watchdog driver
+ *
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/watchdog.h>
+
+/* JH7100 Watchdog register define */
+#define STARFIVE_WDT_JH7100_INTSTAUS 0x000 /* RO: [4]: Watchdog Interrupt status */
+#define STARFIVE_WDT_JH7100_CONTROL 0x104 /* RW: reset enable */
+#define STARFIVE_WDT_JH7100_LOAD 0x108 /* RW: Watchdog Load register */
+#define STARFIVE_WDT_JH7100_EN 0x110 /* RW: Watchdog Enable Register */
+#define STARFIVE_WDT_JH7100_RELOAD 0x114 /* RW: Write 0 or 1 to reload preset value */
+#define STARFIVE_WDT_JH7100_VALUE 0x118 /* RO: The current value for watchdog counter */
+#define STARFIVE_WDT_JH7100_INTCLR 0x120 /*
+ * RW: Watchdog Clear Interrupt Register
+ * [0]: Write 1 to clear interrupt
+ * [1]: 1 mean clearing and 0 mean complete
+ */
+#define STARFIVE_WDT_JH7100_LOCK 0x13c /* RW: Lock Register, write 0x378f0765 to unlock */
+
+/* JH7110 Watchdog register define */
+#define STARFIVE_WDT_JH7110_LOAD 0x000 /* RW: Watchdog Load register */
+#define STARFIVE_WDT_JH7110_VALUE 0x004 /* RO: The current value for watchdog counter */
+#define STARFIVE_WDT_JH7110_CONTROL 0x008 /*
+ * RW:
+ * [0]: reset enable;
+ * [1]: int enable/wdt enable/reload counter;
+ * [31:2]: reserved.
+ */
+#define STARFIVE_WDT_JH7110_INTCLR 0x00c /* WO: clear intterupt && reload the counter */
+#define STARFIVE_WDT_JH7110_IMS 0x014 /* RO: Enabled interrupt status from the counter */
+#define STARFIVE_WDT_JH7110_LOCK 0xc00 /* RW: Lock Register, write 0x1ACCE551 to unlock */

Since these register offsets are only used to fill in the
starfive_wdt_variant structures, consider just adding them directly
there with the comments.


As maintainer, I prefer defines, even if only used oonce.

Guenter