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

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

[Rasmus Villemoes]

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

[PATCH 2/2] sandbox: add test of wdt_gpio driver

[Rasmus Villemoes]

It seems that no other test has claimed gpio_a:7 yet, so use that.

The only small wrinkle is modifying the existing wdt test to use
uclass_get_device_by_driver() since we now have to UCLASS_WDT
instances in play, so it's a little more robust to fetch the device by
driver and not merely uclass+index.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/sandbox/dts/test.dts   |  5 +++++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 test/dm/wdt.c               | 32 +++++++++++++++++++++++++++++++-
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index fe26ced31d..e6e1f50bd0 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -756,6 +756,11 @@
 		};
 	};
 
+	gpio-wdt {
+		gpios = <&gpio_a 7 0>;
+		compatible = "linux,wdt-gpio";
+	};
+
 	mbox: mbox {
 		compatible = "sandbox,mbox";
 		#mbox-cells = <1>;
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 9a373bab6f..ed41c6c897 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -224,6 +224,7 @@ CONFIG_SANDBOX_OSD=y
 CONFIG_SPLASH_SCREEN_ALIGN=y
 CONFIG_VIDEO_BMP_RLE8=y
 CONFIG_WDT=y
+CONFIG_WDT_GPIO=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
 CONFIG_FS_CRAMFS=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index bdbf714e2b..fef2d8777a 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -271,6 +271,7 @@ CONFIG_W1_GPIO=y
 CONFIG_W1_EEPROM=y
 CONFIG_W1_EEPROM_SANDBOX=y
 CONFIG_WDT=y
+CONFIG_WDT_GPIO=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
 CONFIG_FS_CRAMFS=y
diff --git a/test/dm/wdt.c b/test/dm/wdt.c
index 24b991dff6..fe75cbfea7 100644
--- a/test/dm/wdt.c
+++ b/test/dm/wdt.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <dm.h>
 #include <wdt.h>
+#include <asm/gpio.h>
 #include <asm/state.h>
 #include <asm/test.h>
 #include <dm/test.h>
@@ -19,7 +20,8 @@ static int dm_test_wdt_base(struct unit_test_state *uts)
 	struct udevice *dev;
 	const u64 timeout = 42;
 
-	ut_assertok(uclass_get_device(UCLASS_WDT, 0, &dev));
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_sandbox), &dev));
 	ut_assertnonnull(dev);
 	ut_asserteq(0, state->wdt.counter);
 	ut_asserteq(false, state->wdt.running);
@@ -39,3 +41,31 @@ static int dm_test_wdt_base(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_wdt_base, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+static int dm_test_wdt_gpio(struct unit_test_state *uts)
+{
+	/*
+	 * The sandbox wdt gpio is "connected" to gpio bank a, offset
+	 * 7. Use the sandbox back door to verify that the gpio-wdt
+	 * driver behaves as expected.
+	 */
+	struct udevice *wdt, *gpio;
+	const int offset = 7;
+	int val;
+
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_gpio), &wdt));
+	ut_assertnonnull(wdt);
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
+	ut_assertnonnull(gpio);
+
+	val = sandbox_gpio_get_value(gpio, offset);
+	ut_assertok(wdt_reset(wdt));
+	ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset));
+	ut_assertok(wdt_reset(wdt));
+	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
+
+	return 0;
+}
+DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT);
-- 
2.29.2

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

[Simon Glass]

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

[PATCH 2/2] sandbox: add test of wdt_gpio driver

[Simon Glass]

On Mon, 10 May 2021 at 09:47, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> It seems that no other test has claimed gpio_a:7 yet, so use that.
>
> The only small wrinkle is modifying the existing wdt test to use
> uclass_get_device_by_driver() since we now have to UCLASS_WDT
> instances in play, so it's a little more robust to fetch the device by
> driver and not merely uclass+index.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  arch/sandbox/dts/test.dts   |  5 +++++
>  configs/sandbox64_defconfig |  1 +
>  configs/sandbox_defconfig   |  1 +
>  test/dm/wdt.c               | 32 +++++++++++++++++++++++++++++++-
>  4 files changed, 38 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Rasmus Villemoes]

On 10/05/2021 18.28, Simon Glass wrote:
> 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).

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.

>> +
>>  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.

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

Thanks,
Rasmus

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

[Simon Glass]

Hi Rasmus,

On Mon, 10 May 2021 at 12:45, Rasmus Villemoes
<rasmus.villemoes@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@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).
>
> 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

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

[Rasmus Villemoes]

On 10/05/2021 21.19, Simon Glass wrote:
> Hi Rasmus,
> 
> On Mon, 10 May 2021 at 12:45, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>>>> 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, 

It's not that I don't want to, I was just pointing out that no other WDT
drivers have that, nor a binding to refer to. I'll add a ref.

perhaps you could
> mention that the pin needs to be toggled each xx ms otherwise the
> board will be reset? 

Well, that part depends on which particular watchdog circuit the
hardware guys have decided to put on the board - these are very generic
components. I'll add the hw_margin_ms property to the .txt file.

Rasmus

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

[Stefan Roese]

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 <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.
> +
>   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?

Perhaps it makes sense to add a note about this into the driver to make
this clear.

Thanks,
Stefan

> +
> +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),
> +};
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

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

[Rasmus Villemoes]

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 <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.
>> +
>> ? 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.

> 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

[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 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 <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.
>>> +
>>>  ? 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

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

[Stefan Roese]

On 11.05.21 09:20, Stefan Roese wrote:
> 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@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'm looking through the pending watchdog patches right now. Do you plan
to send an updated version of this patchset soon (mostly updates to
documentation)?

Thanks,
Stefan

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

[Rasmus Villemoes]

On 22/05/2021 09.18, Stefan Roese wrote:
> On 11.05.21 09:20, Stefan Roese wrote:
>> On 11.05.21 08:40, Rasmus Villemoes wrote:

>>> 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'm looking through the pending watchdog patches right now. Do you plan
> to send an updated version of this patchset soon (mostly updates to
> documentation)?

Yes, I have a larger set of patches pending that includes these, but
also the "handle all DM watchdogs in watchdog_reset()". Will send in the
next few days.

Thanks,
Rasmus