| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0b467896-17d4-c355-da86-437b45f089d4@roeck-us.net>
Date: Wed, 8 Mar 2023 07:17:45 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Emil Renner Berthing <emil.renner.berthing@...onical.com>,
Xingyu Wu <xingyu.wu@...rfivetech.com>
Cc: linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
linux-watchdog@...r.kernel.org,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Emil Renner Berthing <kernel@...il.dk>,
Conor Dooley <conor@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Philipp Zabel <p.zabel@...gutronix.de>,
Samin Guo <samin.guo@...rfivetech.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] drivers: watchdog: Add StarFive Watchdog driver
On 3/8/23 07:07, Emil Renner Berthing wrote:
> On Wed, 8 Mar 2023 at 04:43, Xingyu Wu <xingyu.wu@...rfivetech.com> wrote:
>>
>> Add watchdog driver for the StarFive JH7100 and JH7110 SoC.
>>
>> Signed-off-by: Xingyu Wu <xingyu.wu@...rfivetech.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@...rfivetech.com>
>> +M: Samin Guo <samin.guo@...rfivetech.com>
>> +S: Supported
>> +F: Documentation/devicetree/bindings/watchdog/starfive*
>> +F: drivers/watchdog/starfive-wdt.c
>> +
>> STATIC BRANCH/CALL
>> M: Peter Zijlstra <peterz@...radead.org>
>> M: Josh Poimboeuf <jpoimboe@...nel.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
Powered by blists - more mailing lists