On 28.05.21 00:00, Rasmus Villemoes wrote:

wdt_start() does the "no ->start? return -ENOSYS" check, don't open-code that in wdt_expire_now().

Also, wdt_start() maintains some global (and later some per-device) state, which would get out of sync with this direct method call - not that it matters much here since the board is supposed to reset very soon.

Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

drivers/watchdog/wdt-uclass.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 2687135296..634428fa24 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -118,10 +118,8 @@ int wdt_expire_now(struct udevice *dev, ulong flags) if (ops->expire_now) { return ops->expire_now(dev, flags); } else {

• if (!ops->start)
  

• 	return -ENOSYS;
  

• ret = wdt_start(dev, 1, flags);
  
  

• ret = ops->start(dev, 1, flags);
  

  if (ret < 0) return ret;

Viele Grüße, Stefan

On Thu, 27 May 2021 at 16:00, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:

As preparation for having the wdt-uclass provided watchdog_reset() function handle all DM watchdog devices, and not just the first such, introduce a uclass-owned struct to hold the reset_period and next_reset, so these become per-device instead of being static variables.

No functional change intended.

Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk

drivers/watchdog/wdt-uclass.c | 65 ++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 20 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

nit below

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 634428fa24..3b6aa3d586 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -18,15 +18,15 @@ DECLARE_GLOBAL_DATA_PTR;

#define WATCHDOG_TIMEOUT_SECS (CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)

-/*

• • Reset every 1000ms, or however often is required as indicated by a
• • hw_margin_ms property.
• */

-static ulong reset_period = 1000; +struct wdt_priv {

•   u32 timeout;
  

•   ulong reset_period;
  

•   ulong next_reset;
  

can you comment these?

+};

int initr_watchdog(void) {

•   u32 timeout = WATCHDOG_TIMEOUT_SECS;
  

•   struct wdt_priv *priv;
    int ret;
  
    /*
  

@@ -42,28 +42,21 @@ int initr_watchdog(void) return 0; } }

•   if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
  

•           timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
  

•                                          WATCHDOG_TIMEOUT_SECS);
  

•           reset_period = dev_read_u32_default(gd->watchdog_dev,
  

•                                               "hw_margin_ms",
  

•                                               4 * reset_period) / 4;
  

•   }
  

•   priv = dev_get_uclass_priv(gd->watchdog_dev);
  
    if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
            printf("WDT:   Not starting\n");
            return 0;
    }
  
  

•   ret = wdt_start(gd->watchdog_dev, timeout * 1000, 0);
  

•   ret = wdt_start(gd->watchdog_dev, priv->timeout * 1000, 0);
    if (ret != 0) {
            printf("WDT:   Failed to start\n");
            return 0;
    }
  
    printf("WDT:   Started with%s servicing (%ds timeout)\n",
  

•          IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
  

•          IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
  
    return 0;
  

} @@ -137,18 +130,21 @@ int wdt_expire_now(struct udevice *dev, ulong flags) */ void watchdog_reset(void) {

•   static ulong next_reset;
  

•   struct wdt_priv *priv;
  

•   struct udevice *dev;
    ulong now;
  
    /* Exit if GD is not ready or watchdog is not initialized yet */
    if (!gd || !(gd->flags & GD_FLG_WDT_READY))
            return;
  
  

•   dev = gd->watchdog_dev;
  

•   priv = dev_get_uclass_priv(dev);
    /* Do not reset the watchdog too often */
    now = get_timer(0);
  

•   if (time_after_eq(now, next_reset)) {
  

•           next_reset = now + reset_period;
  

•           wdt_reset(gd->watchdog_dev);
  

•   if (time_after_eq(now, priv->next_reset)) {
  

•           priv->next_reset = now + priv->reset_period;
  

•           wdt_reset(dev);
    }
  

} #endif @@ -175,9 +171,38 @@ static int wdt_post_bind(struct udevice *dev) return 0; }

+static int wdt_pre_probe(struct udevice *dev) +{

•   u32 timeout = WATCHDOG_TIMEOUT_SECS;
  

•   /*
  

•    * Reset every 1000ms, or however often is required as
  

•    * indicated by a hw_margin_ms property.
  

•    */
  

•   ulong reset_period = 1000;
  

•   struct wdt_priv *priv;
  

•   if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
  

•           timeout = dev_read_u32_default(dev, "timeout-sec", timeout);
  

•           reset_period = dev_read_u32_default(dev, "hw_margin_ms",
  

•                                               4 * reset_period) / 4;
  

•   }
  

•   priv = dev_get_uclass_priv(dev);
  

•   priv->timeout = timeout;
  

•   priv->reset_period = reset_period;
  

•   /*
  

•    * Pretend this device was last reset "long" ago so the first
  

•    * watchdog_reset will actually call its ->reset method.
  

•    */
  

•   priv->next_reset = get_timer(0);
  

•   return 0;
  

+}

UCLASS_DRIVER(wdt) = { .id = UCLASS_WDT, .name = "watchdog", .flags = DM_UC_FLAG_SEQ_ALIAS, .post_bind = wdt_post_bind,

•   .pre_probe              = wdt_pre_probe,
  

•   .per_device_auto        = sizeof(struct wdt_priv),
  

};

2.31.1

On 28.05.21 00:00, Rasmus Villemoes wrote:

The addition of .pre_probe and .per_device_auto made this look bad. Fix it.

Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

drivers/watchdog/wdt-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 3b6aa3d586..360f052227 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -199,10 +199,10 @@ static int wdt_pre_probe(struct udevice *dev) }

UCLASS_DRIVER(wdt) = {

• .id = UCLASS_WDT,
• .name = "watchdog",
• .flags = DM_UC_FLAG_SEQ_ALIAS,
• .post_bind = wdt_post_bind,

• .id = UCLASS_WDT,
• .name = "watchdog",
• .flags = DM_UC_FLAG_SEQ_ALIAS,
• .post_bind = wdt_post_bind, .pre_probe = wdt_pre_probe, .per_device_auto = sizeof(struct wdt_priv), };

Viele Grüße, Stefan

On 28.05.21 00:00, Rasmus Villemoes wrote:

As a step towards handling all DM watchdogs in watchdog_reset(), use a per-device flag to keep track of whether the device has been started instead of a bit in gd->flags.

We will still need that bit to know whether we are past initr_watchdog() and hence have populated gd->watchdog_dev - incidentally, that is how it was used prior to commit 9c44ff1c5f3c.

Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

drivers/watchdog/wdt-uclass.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 5d5dbcd0f7..026b6d1eb4 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -22,6 +22,7 @@ struct wdt_priv { u32 timeout; ulong reset_period; ulong next_reset;

• bool running; };

  static void init_watchdog_dev(struct udevice *dev)

@@ -63,6 +64,7 @@ int initr_watchdog(void) } init_watchdog_dev(gd->watchdog_dev);

• gd->flags |= GD_FLG_WDT_READY; return 0; }

@@ -75,8 +77,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) return -ENOSYS;

ret = ops->start(dev, timeout_ms, flags);

• if (ret == 0)

• gd->flags |= GD_FLG_WDT_READY;
  

• if (ret == 0) {

• struct wdt_priv *priv = dev_get_uclass_priv(dev);
  

• priv->running = true;
  

• }

  return ret; }

@@ -90,8 +95,11 @@ int wdt_stop(struct udevice *dev) return -ENOSYS;

ret = ops->stop(dev);

• if (ret == 0)

• gd->flags &= ~GD_FLG_WDT_READY;
  

• if (ret == 0) {

• struct wdt_priv *priv = dev_get_uclass_priv(dev);
  

• priv->running = false;
  

• }

  return ret; }

@@ -145,6 +153,9 @@ void watchdog_reset(void)

dev = gd->watchdog_dev; priv = dev_get_uclass_priv(dev);

• if (!priv->running)

• return;
  

• /* Do not reset the watchdog too often */ now = get_timer(0); if (time_after_eq(now, priv->next_reset)) {

Viele Grüße, Stefan

Hi Simon,

Thanks for review.

...

•   if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
  

•           printf("WDT:   Not starting %s\n", dev->name);
  

•           return;
  

•   }
  

•   ret = wdt_start(dev, priv->timeout * 1000, 0);
  

•   if (ret != 0) {
  

•           printf("WDT:   Failed to start %s\n", dev->name);
  

•           return;
  

•   ret = uclass_get(UCLASS_WDT, &uc);
  

•   if (ret) {
  

•           printf("Error getting UCLASS_WDT: %d\n", ret);
  

log_debug()as this should not happen

OK. [I assume the rationale is: don't add .text which will most likely never be used, but allow the debug statements to be easily turned on per-TU if one should ever need it.]

...

•           return 0;
  

Should return error here

And have the initr sequence stop completely instead of trying to limp along? Why? Note that I touched upon this in the commit message: The existing code doesn't pass on an error from uclass_get_device*() [which would most likely come from an underlying probe call], and returns 0 regardless from initr_watchdog(). I see no point changing that.

...

•   }
  

•   init_watchdog_dev(gd->watchdog_dev);
  

•   uclass_foreach_dev(dev, uc)
  

•           device_probe(dev);
  

If watchdog fails, should we not stop? Even if we don't, I think some sort of message should be shown so people know to fix it.

I'd assume device_probe() would print an error message. If not, sure, I can add some [log_debug?] message.

...

    gd->flags |= GD_FLG_WDT_READY;
    return 0;

@@ -145,22 +121,26 @@ void watchdog_reset(void) { struct wdt_priv *priv; struct udevice *dev;

•   struct uclass *uc;
    ulong now;
  
    /* Exit if GD is not ready or watchdog is not initialized yet */
    if (!gd || !(gd->flags & GD_FLG_WDT_READY))
            return;
  
  

•   dev = gd->watchdog_dev;
  

•   priv = dev_get_uclass_priv(dev);
  

•   if (!priv->running)
  

•   if (uclass_get(UCLASS_WDT, &uc))
            return;
  
  

•   /* Do not reset the watchdog too often */
  

•   now = get_timer(0);
  

•   if (time_after_eq(now, priv->next_reset)) {
  

•           priv->next_reset = now + priv->reset_period;
  

•           wdt_reset(dev);
  

•   uclass_foreach_dev(dev, uc) {
  

Don't you need to use foreach_dev_probe() ?

Why? They've all been probed in initr_watchdog(). And the guts of WATHCDOG_RESET(), which can be and is called from everywhere, is certainly not the place to do anything other than actually pinging the watchdog devices.

...

+static int wdt_post_probe(struct udevice *dev) +{

•   struct wdt_priv *priv;
  

•   int ret;
  

•   priv = dev_get_uclass_priv(dev);
  

•   if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
  

•           printf("WDT:   Not starting %s\n", dev->name);
  

log_debug()

Perhaps, but this is just existing code I've moved around.

...

•           return 0;
  

•   }
  

•   ret = wdt_start(dev, priv->timeout * 1000, 0);
  

•   if (ret != 0) {
  

•           printf("WDT:   Failed to start %s\n", dev->name);
  

log_debug()

Ditto.

...

•           return 0;
  

•   }
  

I don't think it is good to start it in the post-probe. Can you do it separately, afterwards?

Eh, yes, of course I could do this in the loop in initr_watchdog() where I probe all watchdog devices, but the end result is exactly the same, and it seemed that this was a perfect fit for DM since it provided this post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, and if it is, that precisely means the developer wanted to start handling the device(s) ASAP.

Probing is supposed to just probe it and it should be safe to do that without side effects.

I agree in general, but watchdog devices are a bit special compared to some random USB controller or LCD display or spi master - those devices generally don't do anything unless asked to by the CPU, while a watchdog device is the other way around, it does its thing _unless_ the CPU asks it not to (often enough). Which in turn, I suppose, is the whole reason wdt-uclass has its own hook into the initr sequence - one needs to probe and possibly start handling the watchdog(s) ASAP.

BTW, next on my agenda is hooking in even earlier so the few boards that use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely random places in init_sequence_f[] is a bit silly.

...

•   printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
  

•          IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
  

log_debug() I think

Ditto, existing code moved around.

Thanks, Rasmus

Hi Rasmus,

On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:

Hi Simon,

Thanks for review.

...

...

•   if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
  

•           printf("WDT:   Not starting %s\n", dev->name);
  

•           return;
  

•   }
  

•   ret = wdt_start(dev, priv->timeout * 1000, 0);
  

•   if (ret != 0) {
  

•           printf("WDT:   Failed to start %s\n", dev->name);
  

•           return;
  

•   ret = uclass_get(UCLASS_WDT, &uc);
  

•   if (ret) {
  

•           printf("Error getting UCLASS_WDT: %d\n", ret);
  

log_debug()as this should not happen

OK. [I assume the rationale is: don't add .text which will most likely never be used, but allow the debug statements to be easily turned on per-TU if one should ever need it.]

Yes, also the error return value should give you a clue.

Please, use log_msg_ret() on your return values.

...

...

•           return 0;
  

Should return error here

And have the initr sequence stop completely instead of trying to limp along? Why? Note that I touched upon this in the commit message: The existing code doesn't pass on an error from uclass_get_device*() [which would most likely come from an underlying probe call], and returns 0 regardless from initr_watchdog(). I see no point changing that.

OK, I'll leave it to you. My feeling is that failure is a good way to get a bug fixed.

...

...

•   }
  

•   init_watchdog_dev(gd->watchdog_dev);
  

•   uclass_foreach_dev(dev, uc)
  

•           device_probe(dev);
  

If watchdog fails, should we not stop? Even if we don't, I think some sort of message should be shown so people know to fix it.

I'd assume device_probe() would print an error message. If not, sure, I can add some [log_debug?] message.

No driver model never prints messages. Yes you can use log_debuig().

...

...

    gd->flags |= GD_FLG_WDT_READY;
    return 0;

@@ -145,22 +121,26 @@ void watchdog_reset(void) { struct wdt_priv *priv; struct udevice *dev;

•   struct uclass *uc;
    ulong now;
  
    /* Exit if GD is not ready or watchdog is not initialized yet */
    if (!gd || !(gd->flags & GD_FLG_WDT_READY))
            return;
  
  

•   dev = gd->watchdog_dev;
  

•   priv = dev_get_uclass_priv(dev);
  

•   if (!priv->running)
  

•   if (uclass_get(UCLASS_WDT, &uc))
            return;
  
  

•   /* Do not reset the watchdog too often */
  

•   now = get_timer(0);
  

•   if (time_after_eq(now, priv->next_reset)) {
  

•           priv->next_reset = now + priv->reset_period;
  

•           wdt_reset(dev);
  

•   uclass_foreach_dev(dev, uc) {
  

Don't you need to use foreach_dev_probe() ?

Why? They've all been probed in initr_watchdog(). And the guts of WATHCDOG_RESET(), which can be and is called from everywhere, is certainly not the place to do anything other than actually pinging the watchdog devices.

Then you should add a comment. You are using a low-level iterator, then assuming the devices are probed.

...

...

+static int wdt_post_probe(struct udevice *dev) +{

•   struct wdt_priv *priv;
  

•   int ret;
  

•   priv = dev_get_uclass_priv(dev);
  

•   if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
  

•           printf("WDT:   Not starting %s\n", dev->name);
  

log_debug()

Perhaps, but this is just existing code I've moved around.

OK, well then you can leave it alone.

...

...

•           return 0;
  

•   }
  

•   ret = wdt_start(dev, priv->timeout * 1000, 0);
  

•   if (ret != 0) {
  

•           printf("WDT:   Failed to start %s\n", dev->name);
  

log_debug()

Ditto.

...

...

•           return 0;
  

•   }
  

I don't think it is good to start it in the post-probe. Can you do it separately, afterwards?

Eh, yes, of course I could do this in the loop in initr_watchdog() where I probe all watchdog devices, but the end result is exactly the same, and it seemed that this was a perfect fit for DM since it provided this post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, and if it is, that precisely means the developer wanted to start handling the device(s) ASAP.

...

Probing is supposed to just probe it and it should be safe to do that without side effects.

I agree in general, but watchdog devices are a bit special compared to some random USB controller or LCD display or spi master - those devices generally don't do anything unless asked to by the CPU, while a watchdog device is the other way around, it does its thing _unless_ the CPU asks it not to (often enough). Which in turn, I suppose, is the whole reason wdt-uclass has its own hook into the initr sequence - one needs to probe and possibly start handling the watchdog(s) ASAP.

It still needs a 'start' method to make it start. Having it start on probe means the board will reset at the command line if the device is probed. Yuck.

BTW, next on my agenda is hooking in even earlier so the few boards that use INIT_FUNC_WATCHDOG_INIT/INIT_FUNC_WATCHDOG_RESET can just use DM and be done with it - having those INIT_FUNC_WATCHDOG_RESET in completely random places in init_sequence_f[] is a bit silly.

OK.

...

...

•   printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
  

•          IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
  

log_debug() I think

Ditto, existing code moved around.

OK.

Regards, Simon

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

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

On 28.05.21 00:00, Rasmus Villemoes wrote:

Check that the watchdog_reset() implementation in wdt-uclass behaves as expected:

• resets all activated watchdog devices
• leaves unactivated/stopped devices alone
• that the rate-limiting works, with a per-device threshold

Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

arch/sandbox/dts/test.dts | 2 ++ test/dm/wdt.c | 54 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index cee5b14ecb..1853b5f29a 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -758,6 +758,7 @@ gpio-wdt { gpios = <&gpio_a 7 0>; compatible = "linux,wdt-gpio";

• hw_margin_ms = <100>;
  

  always-running; };

@@ -1239,6 +1240,7 @@

wdt0: wdt@0 { compatible = "sandbox,wdt";

• hw_margin_ms = <200>;
  

  };

  axi: axi@0 {

diff --git a/test/dm/wdt.c b/test/dm/wdt.c index abff853a02..8997aaebc8 100644 --- a/test/dm/wdt.c +++ b/test/dm/wdt.c @@ -12,6 +12,8 @@ #include <dm/test.h> #include <test/test.h> #include <test/ut.h> +#include <linux/delay.h> +#include <watchdog.h>

/* Test that watchdog driver functions are called */ static int dm_test_wdt_base(struct unit_test_state *uts) @@ -73,3 +75,55 @@ static int dm_test_wdt_gpio(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT);

+static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts) +{

• struct sandbox_state *state = state_get_current();
• struct udevice *gpio_wdt, *sandbox_wdt;
• struct udevice *gpio;
• const u64 timeout = 42;
• const int offset = 7;
• uint reset_count;
• int val;
• ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,

• 				DM_DRIVER_GET(wdt_gpio), &gpio_wdt));
  

• ut_assertnonnull(gpio_wdt);
• ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,

• 				DM_DRIVER_GET(wdt_sandbox), &sandbox_wdt));
  

• ut_assertnonnull(sandbox_wdt);
• ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
• ut_assertnonnull(gpio);
• /* Neither device should be "started", so watchdog_reset() should be a no-op. */
• reset_count = state->wdt.reset_count;
• val = sandbox_gpio_get_value(gpio, offset);
• watchdog_reset();
• ut_asserteq(reset_count, state->wdt.reset_count);
• ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
• /* Start both devices. */
• ut_assertok(wdt_start(gpio_wdt, timeout, 0));
• ut_assertok(wdt_start(sandbox_wdt, timeout, 0));
• /* Make sure both devices have just been pinged. */
• mdelay(100);
• watchdog_reset();
• reset_count = state->wdt.reset_count;
• val = sandbox_gpio_get_value(gpio, offset);
• /* The gpio watchdog should be pinged, the sandbox one not. */
• mdelay(30);
• watchdog_reset();
• ut_asserteq(reset_count, state->wdt.reset_count);
• ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset));
• /* After another ~30ms, both devices should get pinged. */
• mdelay(30);
• watchdog_reset();
• ut_asserteq(reset_count + 1, state->wdt.reset_count);
• ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
• return 0;

+} +DM_TEST(dm_test_wdt_watchdog_reset, UT_TESTF_SCAN_FDT);

Viele Grüße, Stefan