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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux