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

[Stefan Roese]

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@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 <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.
>>> +
>>>    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;
>>> +};
>>> +
>>> +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