From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rasmus Villemoes Date: Tue, 11 May 2021 08:40:58 +0200 Subject: [PATCH 1/2] watchdog: add gpio watchdog driver In-Reply-To: <6538a9d2-0747-f320-9844-c79238743d63@denx.de> References: <20210510154738.3385393-1-rasmus.villemoes@prevas.dk> <6538a9d2-0747-f320-9844-c79238743d63@denx.de> Message-ID: <69be9b42-042f-8461-c581-62f9da1157da@prevas.dk> 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/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. > 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). Rasmus