[PATCH 1/2] watchdog: add gpio watchdog driver

[Simon Glass <sjg@chromium.org>]

Hi Rasmus,

On Mon, 10 May 2021 at 09:47, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> 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.

It seems fine to me. We share DT bindings with Linux anyway.

>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  .../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.

please add a bit more detail. How do you configure it? e.g. point to
the binding file (which you should add to U-Boot if not there).

> +
>  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 <common.h>
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <wdt.h>
> +#include <asm/gpio.h>
> +
> +struct gpio_wdt_priv {
> +       struct gpio_desc gpio;
> +       int state;

bool ?

> +};
> +
> +static int gpio_wdt_reset(struct udevice *dev)
> +{
> +       struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +
> +       priv->state = !priv->state;

blank line before final return (please fix below also)

> +       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,
> +};
> +
> +static const struct udevice_id gpio_wdt_ids[] = {
> +       { .compatible = "linux,wdt-gpio" },
> +       {}
> +};
> +
> +U_BOOT_DRIVER(wdt_gpio) = {
> +       .name = "wdt_gpio",
> +       .id = UCLASS_WDT,
> +       .of_match = gpio_wdt_ids,
> +       .ops = &gpio_wdt_ops,
> +       .probe  = dm_probe,
> +       .priv_auto = sizeof(struct gpio_wdt_priv),
> +};
> --
> 2.29.2
>

Regards,
Simon