Re: [PATCH 4/4] watchdog: Automatically register device with sysreset
Hi Samuel,
On Wed, 3 Nov 2021 at 21:49, Samuel Holland wrote:
>
> On 10/31/21 6:46 PM, Simon Glass wrote:
> > Hi Samuel,
> >
> > On Sun, 22 Aug 2021 at 14: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
> >> ---
> >>
> >> 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
> >
> >>
> >> 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
> >> #include
> >> +#include
> >> #include
> >> +#include
> >> #include
> >> #include
> >>
> >> @@ -57,3 +59,25 @@ U_BOOT_DRIVER(wdt_reboot) = {
> >> .plat_auto = sizeof(struct wdt_reboot_plat),
> >> .ops = _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.
Yes that's fine, you have the review tag. I was thinking something like:
/* private data for WDT uclass */
struct wdt_priv {
struct wdt_reboot_plat wdt_reboot;
};
/* pass plat to device_bind()
device_bind(...>wdt_reboot...)
Needs a little thought of course, as you say. I am not keen on using
dev_set_plat() after binding if we can help it.
[..]
Regards,
Simon
Re: [PATCH 4/4] watchdog: Automatically register device with sysreset
On 10/31/21 6:46 PM, Simon Glass wrote:
> Hi Samuel,
>
> On Sun, 22 Aug 2021 at 14: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
>> ---
>>
>> 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
>
>>
>> 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
>> #include
>> +#include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -57,3 +59,25 @@ U_BOOT_DRIVER(wdt_reboot) = {
>> .plat_auto = sizeof(struct wdt_reboot_plat),
>> .ops = _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
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -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
>
Re: [PATCH 4/4] watchdog: Automatically register device with sysreset
Hi Samuel,
On Sun, 22 Aug 2021 at 14: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
> ---
>
> 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
>
> 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
> #include
> +#include
> #include
> +#include
> #include
> #include
>
> @@ -57,3 +59,25 @@ U_BOOT_DRIVER(wdt_reboot) = {
> .plat_auto = sizeof(struct wdt_reboot_plat),
> .ops = _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...
> +{
> + 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
Re: [PATCH 4/4] watchdog: Automatically register device with sysreset
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
---
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
#include
+#include
#include
+#include
#include
#include
@@ -57,3 +59,25 @@ U_BOOT_DRIVER(wdt_reboot) = {
.plat_auto = sizeof(struct wdt_reboot_plat),
.ops = _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
#include
#include
+#include
#include
#include
#include
@@ -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
