[PATCH 4/4] watchdog: Automatically register device with sysreset

[Samuel Holland]

On 10/31/21 6:46 PM, Simon Glass wrote:
> Hi Samuel,
> 
> On Sun, 22 Aug 2021 at 14:41, Samuel Holland <samuel at 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 at 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 at 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
>