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

[Simon Glass]

Hi Rasmus,

On Mon, 10 May 2021 at 12:45, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 10/05/2021 18.28, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Mon, 10 May 2021 at 09:47, Rasmus Villemoes
> > <rasmus.villemoes at 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@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 at 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).
>
> You mean add a reference to
> doc/device-tree-bindings/watchdog/gpio-wdt.txt which is added in the
> very same patch? I can do that.
>
> No other config option in drivers/watchdog/Kconfig has that, but perhaps
> that's because there's no mention anywhere of wdt under
> doc/device-tree-bindings/. And more generally, very few Kconfig files
> have any such reference.

Well, the thing is....there is a patman warning about adding at least
three lines of text to next Kconfig options. I think people should
really sit and think for a few minutes about what useful information
can be imparted, so users don't need to go to the source code to
figure out basic details.

If you don't want to add a mention of the binding, perhaps you could
mention that the pin needs to be toggled each xx ms otherwise the
board will be reset? I'm not sure, but what you have provided gives me
very little idea about how it works :-)

>
> >> +
> >>  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 ?
>
> Perhaps, though I don't tend to think of the two values a gpio can have
> as true/false; it's more like an uint:1, so any integer type that can
> represent 0 and 1 (i.e., any) would do. I don't really care either way.

Well it's up to you.

>
> >> +};
> >> +
> >> +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)
> >
>
> OK, but I don't think that improves readability in such a tiny function
> - and checkpatch didn't complain.

It's a U-Boot thing.

Regards,
Simon