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...
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 WDTdepends on DM_GPIOhelpSupport 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