Messages in this thread | | | Date | Wed, 8 Mar 2023 07:17:45 -0800 | Subject | Re: [PATCH v4 2/3] drivers: watchdog: Add StarFive Watchdog driver | From | Guenter Roeck <> |
| |
On 3/8/23 07:07, Emil Renner Berthing wrote: > On Wed, 8 Mar 2023 at 04:43, Xingyu Wu <xingyu.wu@starfivetech.com> wrote: >> >> Add watchdog driver for the StarFive JH7100 and JH7110 SoC. >> >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> > > 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@starfivetech.com> >> +M: Samin Guo <samin.guo@starfivetech.com> >> +S: Supported >> +F: Documentation/devicetree/bindings/watchdog/starfive* >> +F: drivers/watchdog/starfive-wdt.c >> + >> STATIC BRANCH/CALL >> M: Peter Zijlstra <peterz@infradead.org> >> M: Josh Poimboeuf <jpoimboe@kernel.org> >> 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
| |