diff mbox series

[v2,08/10] watchdog: add gpio watchdog driver

Message ID 20210527220017.1266765-9-rasmus.villemoes@prevas.dk
State Superseded
Delegated to: Stefan Roese
Headers show
Series handling all DM watchdogs in watchdog_reset() | expand

Commit Message

Rasmus Villemoes May 27, 2021, 10 p.m. UTC 
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 corresponding linux driver apparently has support for some
watchdog circuits which can be disabled by tri-stating the gpio, but I
have never actually encountered such a chip in the wild; the whole
point of adding an external watchdog is usually that it is not in any
way under software control. For forward-compatibility, and to make DT
describe the hardware, the current driver only supports devices that
have the always-running property. I went a little back and forth on
whether I should fail ->probe or only ->start, and ended up deciding
->start was the right place.

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                     | 19 +++++
 drivers/watchdog/Kconfig                      |  9 +++
 drivers/watchdog/Makefile                     |  1 +
 drivers/watchdog/gpio_wdt.c                   | 69 +++++++++++++++++++
 4 files changed, 98 insertions(+)
 create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
 create mode 100644 drivers/watchdog/gpio_wdt.c

Comments

Simon Glass June 22, 2021, 1:31 p.m. UTC | #1 
On Thu, 27 May 2021 at 16:00, 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 corresponding linux driver apparently has support for some
> watchdog circuits which can be disabled by tri-stating the gpio, but I
> have never actually encountered such a chip in the wild; the whole
> point of adding an external watchdog is usually that it is not in any
> way under software control. For forward-compatibility, and to make DT
> describe the hardware, the current driver only supports devices that
> have the always-running property. I went a little back and forth on
> whether I should fail ->probe or only ->start, and ended up deciding
> ->start was the right place.
>
> 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                     | 19 +++++
>  drivers/watchdog/Kconfig                      |  9 +++
>  drivers/watchdog/Makefile                     |  1 +
>  drivers/watchdog/gpio_wdt.c                   | 69 +++++++++++++++++++
>  4 files changed, 98 insertions(+)
>  create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
>  create mode 100644 drivers/watchdog/gpio_wdt.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>
Stefan Roese June 29, 2021, 6:07 a.m. UTC | #2 
On 28.05.21 00:00, 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 corresponding linux driver apparently has support for some
> watchdog circuits which can be disabled by tri-stating the gpio, but I
> have never actually encountered such a chip in the wild; the whole
> point of adding an external watchdog is usually that it is not in any
> way under software control. For forward-compatibility, and to make DT
> describe the hardware, the current driver only supports devices that
> have the always-running property. I went a little back and forth on
> whether I should fail ->probe or only ->start, and ended up deciding
> ->start was the right place.
> 
> 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                     | 19 +++++
>   drivers/watchdog/Kconfig                      |  9 +++
>   drivers/watchdog/Makefile                     |  1 +
>   drivers/watchdog/gpio_wdt.c                   | 69 +++++++++++++++++++
>   4 files changed, 98 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..c9a8559a3e
> --- /dev/null
> +++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
> @@ -0,0 +1,19 @@
> +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.
> +- always-running: Boolean property indicating that the watchdog cannot
> +  be disabled. At present, U-Boot only supports this kind of GPIO
> +  watchdog.
> +
> +Example:
> +
> +	gpio-wdt {
> +		gpios = <&gpio0 1 0>;
> +		compatible = "linux,wdt-gpio";
> +		always-running;
> +	};
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index f0ff2612a6..6fbb5c1b6d 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -147,6 +147,15 @@ 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. See
> +	  doc/device-tree-bindings/watchdog/gpio-wdt.txt for
> +	  information on how to describe the watchdog in device tree.
> +
>   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..091af1d36e
> --- /dev/null
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <common.h>

AFAIK, including "common.h" is deprecated. Other than this:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <wdt.h>
> +#include <asm/gpio.h>
> +
> +struct gpio_wdt_priv {
> +	struct gpio_desc gpio;
> +	bool always_running;
> +	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 gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
> +{
> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +
> +	if (priv->always_running)
> +		return 0;
> +
> +	return -ENOSYS;
> +}
> +
> +static int dm_probe(struct udevice *dev)
> +{
> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	priv->always_running = dev_read_bool(dev, "always-running");
> +	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;
> +	}
> +
> +	if (priv->always_running)
> +		ret = gpio_wdt_reset(dev);
> +
> +	return ret;
> +}
> +
> +static const struct wdt_ops gpio_wdt_ops = {
> +	.start = gpio_wdt_start,
> +	.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),
> +};
> 


Viele Grüße,
Stefan
diff mbox series

Patch

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..c9a8559a3e
--- /dev/null
+++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
@@ -0,0 +1,19 @@ 
+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.
+- always-running: Boolean property indicating that the watchdog cannot
+  be disabled. At present, U-Boot only supports this kind of GPIO
+  watchdog.
+
+Example:
+
+	gpio-wdt {
+		gpios = <&gpio0 1 0>;
+		compatible = "linux,wdt-gpio";
+		always-running;
+	};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f0ff2612a6..6fbb5c1b6d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -147,6 +147,15 @@  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. See
+	  doc/device-tree-bindings/watchdog/gpio-wdt.txt for
+	  information on how to describe the watchdog in device tree.
+
 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..091af1d36e
--- /dev/null
+++ b/drivers/watchdog/gpio_wdt.c
@@ -0,0 +1,69 @@ 
+// 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;
+	bool always_running;
+	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 gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+
+	if (priv->always_running)
+		return 0;
+
+	return -ENOSYS;
+}
+
+static int dm_probe(struct udevice *dev)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	priv->always_running = dev_read_bool(dev, "always-running");
+	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;
+	}
+
+	if (priv->always_running)
+		ret = gpio_wdt_reset(dev);
+
+	return ret;
+}
+
+static const struct wdt_ops gpio_wdt_ops = {
+	.start = gpio_wdt_start,
+	.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),
+};