[PATCH 1/2] watchdog: add gpio watchdog driver
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.
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), +};
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);
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
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).
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
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
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
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 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,
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
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...
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
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...
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
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...
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
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...
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
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
participants (3)
-
Rasmus Villemoes -
Simon Glass -
Stefan Roese