From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Tue, 11 May 2021 09:20:28 +0200 Subject: [PATCH 1/2] watchdog: add gpio watchdog driver In-Reply-To: <69be9b42-042f-8461-c581-62f9da1157da@prevas.dk> References: <20210510154738.3385393-1-rasmus.villemoes@prevas.dk> <6538a9d2-0747-f320-9844-c79238743d63@denx.de> <69be9b42-042f-8461-c581-62f9da1157da@prevas.dk> Message-ID: <07d6797b-5d5d-51ee-0546-3e21cf7ce6fc@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 11.05.21 08:40, Rasmus Villemoes wrote: > On 11/05/2021 07.55, Stefan Roese wrote: >> On 10.05.21 17:47, Rasmus Villemoes wrote: >>> A rather common kind of external watchdog circuit is one that is kept >>> alive by toggling a gpio. Add a driver for handling such a watchdog. >>> >>> The compatible string is probably a little odd as it has nothing to do >>> with linux per se - however, I chose that to make .dts snippets >>> reusable between device trees used with U-Boot and linux, and this is >>> the (only) compatible string that linux' corresponding driver and DT >>> binding accepts. I have asked whether one should/could add "wdt-gpio" >>> to that binding, but the answer was no: >>> >>> >>> https://lore.kernel.org/lkml/CAL_JsqKEGaFpiFV_oAtE+S_bnHkg4qry+bhx2EDs=NSbVf_giA at mail.gmail.com/ >>> >>> >>> If someone feels strongly about this, I can certainly remove the >>> "linux," part from the string - it probably wouldn't the only place where >>> one can't reuse a DT snippet as-is between linux and U-Boot. >>> >>> Signed-off-by: Rasmus Villemoes >>> --- >>> ? .../watchdog/gpio-wdt.txt???????????????????? | 15 ++++++ >>> ? drivers/watchdog/Kconfig????????????????????? |? 7 +++ >>> ? drivers/watchdog/Makefile???????????????????? |? 1 + >>> ? drivers/watchdog/gpio_wdt.c?????????????????? | 51 +++++++++++++++++++ >>> ? 4 files changed, 74 insertions(+) >>> ? create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt >>> ? create mode 100644 drivers/watchdog/gpio_wdt.c >>> >>> diff --git a/doc/device-tree-bindings/watchdog/gpio-wdt.txt >>> b/doc/device-tree-bindings/watchdog/gpio-wdt.txt >>> new file mode 100644 >>> index 0000000000..2283b7ba6e >>> --- /dev/null >>> +++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt >>> @@ -0,0 +1,15 @@ >>> +GPIO watchdog timer >>> + >>> +Describes a simple watchdog timer which is reset by toggling a gpio. >>> + >>> +Required properties: >>> + >>> +- compatible: must be "linux,wdt-gpio" >>> +- gpios: gpio to toggle when wdt driver reset method is called >>> + >>> +Example: >>> + >>> +??? gpio-wdt { >>> +??????? gpios = <&gpio0 1 0>; >>> +??????? compatible = "linux,wdt-gpio"; >>> +??? }; >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>> index f0ff2612a6..2cf378db29 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -147,6 +147,13 @@ config WDT_CORTINA >>> ??????? This driver support all CPU ISAs supported by Cortina >>> ??????? Access CAxxxx SoCs. >>> ? +config WDT_GPIO >>> +??? bool "External gpio watchdog support" >>> +??? depends on WDT >>> +??? depends on DM_GPIO >>> +??? help >>> +?????? Support for external watchdog fed by toggling a gpio. >>> + >>> ? config WDT_MPC8xx >>> ????? bool "MPC8xx watchdog timer support" >>> ????? depends on WDT && MPC8xx >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>> index 5c7ef593fe..f14415bb8e 100644 >>> --- a/drivers/watchdog/Makefile >>> +++ b/drivers/watchdog/Makefile >>> @@ -25,6 +25,7 @@ obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o >>> ? obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o >>> ? obj-$(CONFIG_WDT_ORION) += orion_wdt.o >>> ? obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o >>> +obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o >>> ? obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o >>> ? obj-$(CONFIG_WDT_MT7620) += mt7620_wdt.o >>> ? obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o >>> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c >>> new file mode 100644 >>> index 0000000000..9dba9c254e >>> --- /dev/null >>> +++ b/drivers/watchdog/gpio_wdt.c >>> @@ -0,0 +1,51 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +struct gpio_wdt_priv { >>> +??? struct gpio_desc gpio; >>> +??? int state; >>> +}; >>> + >>> +static int gpio_wdt_reset(struct udevice *dev) >>> +{ >>> +??? struct gpio_wdt_priv *priv = dev_get_priv(dev); >>> + >>> +??? priv->state = !priv->state; >>> +??? return dm_gpio_set_value(&priv->gpio, priv->state); >>> +} >>> + >>> +static int dm_probe(struct udevice *dev) >>> +{ >>> +??? struct gpio_wdt_priv *priv = dev_get_priv(dev); >>> +??? int ret; >>> + >>> +??? ret = gpio_request_by_name(dev, "gpios", 0, &priv->gpio, >>> GPIOD_IS_OUT); >>> +??? if (ret < 0) { >>> +??????? dev_err(dev, "Request for wdt gpio failed: %d\n", ret); >>> +??????? return ret; >>> +??? } >>> +??? return gpio_wdt_reset(dev); >>> +} >>> + >>> +static const struct wdt_ops gpio_wdt_ops = { >>> +??? .reset = gpio_wdt_reset, >>> +}; >> >> I'm noticing that you don't have a "start" function. How is this GPIO >> watchdog enabled? Is it "always on"? Or does it start with the first >> triggering? >> > > I have yet to encounter one of these that isn't always-running - it's > more or less the only reason one would add an external watchdog circuit > instead of relying on whatever the SOC has. Okay. >> Perhaps it makes sense to add a note about this into the driver to make >> this clear. > > Will do. But I now see a much bigger problem due to some refactoring in > wdt-uclass that basically requires a ->start method or GD_FLG_WDT_READY > won't be set. Sigh. > > I was actually hoping to get this small driver in first, then start > doing the promised "handle _all_ registered watchdogs in wdt_uclass' > watchdog_reset", because testing that is much easier once the sandbox > test has two distinct "devices". I guess I'll just provide a dummy start > method to work around that. Then when I equip wdt_uclass with some > uclass-priv data that will contain not only the reset_period and > last_reset metadata, but also keep track of whether the device is > running (and then wdt_start() can be a no-op in that case, while the > gpio driver can set that flag in its probe function). Sounds very promising. Looking forward to your work on this. Thanks, Stefan