On 8/22/21 22:41, Samuel Holland wrote:

Add an option to automatically register the first watchdog device with the wdt_reboot driver for use with sysreset. This allows sysreset to be a drop-in replacement for platform-specific watchdog reset code, without needing any device tree changes.

Signed-off-by: Samuel Holland samuel@sholland.org

drivers/sysreset/Kconfig | 7 +++++++ drivers/sysreset/sysreset_watchdog.c | 24 ++++++++++++++++++++++++ drivers/watchdog/wdt-uclass.c | 5 +++++ include/sysreset.h | 14 ++++++++++++++ 4 files changed, 50 insertions(+)

diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index fdc858ccbac..49354b1b27a 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -119,6 +119,13 @@ config SYSRESET_WATCHDOG help Reboot support for generic watchdog reset.

+config SYSRESET_WATCHDOG_AUTO

• bool "Automatically register first watchdog with sysreset"
• depends on SYSRESET_WATCHDOG
• help

•  If enabled, the first watchdog (as selected by the watchdog uclass)
  

•  will automatically be registered with the watchdog reboot driver.
  

• config SYSRESET_RESETCTL bool "Enable support for reset controller reboot driver" select DM_RESET

diff --git a/drivers/sysreset/sysreset_watchdog.c b/drivers/sysreset/sysreset_watchdog.c index b723f5647cd..35efcac59dd 100644 --- a/drivers/sysreset/sysreset_watchdog.c +++ b/drivers/sysreset/sysreset_watchdog.c @@ -5,7 +5,9 @@

#include <common.h> #include <dm.h> +#include <dm/device-internal.h> #include <errno.h> +#include <malloc.h> #include <sysreset.h> #include <wdt.h>

@@ -57,3 +59,25 @@ U_BOOT_DRIVER(wdt_reboot) = { .plat_auto = sizeof(struct wdt_reboot_plat), .ops = &wdt_reboot_ops, };

+#if IS_ENABLED(CONFIG_SYSRESET_WATCHDOG_AUTO) +int sysreset_register_wdt(struct udevice *dev) +{

• struct wdt_reboot_plat *plat = malloc(sizeof(*plat));
• int ret;
• if (!plat)

• return -ENOMEM;
  

• plat->wdt = dev;
• ret = device_bind(dev, DM_DRIVER_GET(wdt_reboot),

• 	  dev->name, plat, ofnode_null(), NULL);
  

• if (ret) {

• free(plat);
  

• return ret;
  

• }
• return 0;

+} +#endif diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 17334dbda6c..3170ef9d945 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -10,6 +10,7 @@ #include <errno.h> #include <hang.h> #include <log.h> +#include <sysreset.h> #include <time.h> #include <wdt.h> #include <asm/global_data.h> @@ -53,6 +54,10 @@ int initr_watchdog(void)

This function has been changed in upstream: @@ -44,6 +45,10 @@ static void init_watchdog_dev(struct udevice *dev)

				    4 * reset_period) / 4;

}

• ret = sysreset_register_wdt(gd->watchdog_dev);

ret = sysreset_register_wdt(dev);

I applied the following series to enable testing on the sandbox:

[PATCH 0/2] sandbox: enable testing SYSRESET_WATCHDOG https://lists.denx.de/pipermail/u-boot/2021-October/465270.html

I found no issues.

Please, resend the rebased series.

Best regards

Heinrich

• if (ret)

• printf("WDT:   Failed to register sysreset\n");
  

• if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) { printf("WDT: Not starting\n"); return 0;

diff --git a/include/sysreset.h b/include/sysreset.h index 701e4f5c86e..ad45fe0db28 100644 --- a/include/sysreset.h +++ b/include/sysreset.h @@ -118,4 +118,18 @@ void sysreset_walk_halt(enum sysreset_t type); */ void reset_cpu(void);

+/**

• • sysreset_register_wdt() - register a watchdog for use with sysreset
• • This registers the given watchdog timer to be used to reset the system.
• • @dev: WDT device
• • @return: 0 if OK, -errno if error
• */

+#if IS_ENABLED(CONFIG_SYSRESET_WATCHDOG_AUTO) +int sysreset_register_wdt(struct udevice *dev); +#else +static inline int sysreset_register_wdt(struct udevice *dev) { return 0; } +#endif

• #endif

On 10/31/21 6:46 PM, Simon Glass wrote:

Hi Samuel,

On Sun, 22 Aug 2021 at 14:41, Samuel Holland samuel@sholland.org wrote:

...

Add an option to automatically register the first watchdog device with the wdt_reboot driver for use with sysreset. This allows sysreset to be a drop-in replacement for platform-specific watchdog reset code, without needing any device tree changes.

Signed-off-by: Samuel Holland samuel@sholland.org

drivers/sysreset/Kconfig | 7 +++++++ drivers/sysreset/sysreset_watchdog.c | 24 ++++++++++++++++++++++++ drivers/watchdog/wdt-uclass.c | 5 +++++ include/sysreset.h | 14 ++++++++++++++ 4 files changed, 50 insertions(+)

This is a bit odd, but I suppose it is OK.

Reviewed-by: Simon Glass sjg@chromium.org

...

diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index fdc858ccbac..49354b1b27a 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -119,6 +119,13 @@ config SYSRESET_WATCHDOG help Reboot support for generic watchdog reset.

+config SYSRESET_WATCHDOG_AUTO

•   bool "Automatically register first watchdog with sysreset"
  

•   depends on SYSRESET_WATCHDOG
  

•   help
  

•     If enabled, the first watchdog (as selected by the watchdog uclass)
  

•     will automatically be registered with the watchdog reboot driver.
  

config SYSRESET_RESETCTL bool "Enable support for reset controller reboot driver" select DM_RESET diff --git a/drivers/sysreset/sysreset_watchdog.c b/drivers/sysreset/sysreset_watchdog.c index b723f5647cd..35efcac59dd 100644 --- a/drivers/sysreset/sysreset_watchdog.c +++ b/drivers/sysreset/sysreset_watchdog.c @@ -5,7 +5,9 @@

#include <common.h> #include <dm.h> +#include <dm/device-internal.h> #include <errno.h> +#include <malloc.h> #include <sysreset.h> #include <wdt.h>

@@ -57,3 +59,25 @@ U_BOOT_DRIVER(wdt_reboot) = { .plat_auto = sizeof(struct wdt_reboot_plat), .ops = &wdt_reboot_ops, };

+#if IS_ENABLED(CONFIG_SYSRESET_WATCHDOG_AUTO) +int sysreset_register_wdt(struct udevice *dev)

Could you pass in plat here instead of allocating it? You could add it to wdt-uclass.c as some private data for the uclass...

Yes, that is a good idea, especially now that all watchdogs get initialized during boot (so this code also applies to all watchdogs).

It would be simplest to use the plat pointer directly as the watchdog reference (otherwise the watchdog needs to store a pointer to itself in its uclass priv, which is a bit strange). But that would require using dev_set_plat in the wdt_reboot of_to_plat case. I wonder if that is an appropriate use of this API? Not much else uses dev_set_plat.

So I'd like to do this as a follow-up.

Regards, Samuel

...

+{

•   struct wdt_reboot_plat *plat = malloc(sizeof(*plat));
  

•   int ret;
  

•   if (!plat)
  

•           return -ENOMEM;
  

•   plat->wdt = dev;
  

•   ret = device_bind(dev, DM_DRIVER_GET(wdt_reboot),
  

•                     dev->name, plat, ofnode_null(), NULL);
  

•   if (ret) {
  

•           free(plat);
  

•           return ret;
  

•   }
  

•   return 0;
  

+} +#endif diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 17334dbda6c..3170ef9d945 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -10,6 +10,7 @@ #include <errno.h> #include <hang.h> #include <log.h> +#include <sysreset.h> #include <time.h> #include <wdt.h> #include <asm/global_data.h> @@ -53,6 +54,10 @@ int initr_watchdog(void) 4 * reset_period) / 4; }

•   ret = sysreset_register_wdt(gd->watchdog_dev);
  

•   if (ret)
  

•           printf("WDT:   Failed to register sysreset\n");
  

•   if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
            printf("WDT:   Not starting\n");
            return 0;
  

diff --git a/include/sysreset.h b/include/sysreset.h index 701e4f5c86e..ad45fe0db28 100644 --- a/include/sysreset.h +++ b/include/sysreset.h @@ -118,4 +118,18 @@ void sysreset_walk_halt(enum sysreset_t type); */ void reset_cpu(void);

+/**

• • sysreset_register_wdt() - register a watchdog for use with sysreset
• • This registers the given watchdog timer to be used to reset the system.
• • @dev: WDT device
• • @return: 0 if OK, -errno if error
• */

+#if IS_ENABLED(CONFIG_SYSRESET_WATCHDOG_AUTO) +int sysreset_register_wdt(struct udevice *dev); +#else +static inline int sysreset_register_wdt(struct udevice *dev) { return 0; } +#endif

#endif

2.31.1

Regards, Simon