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