Hi Samuel,
On Wed, 3 Nov 2021 at 21:49, Samuel Holland samuel@sholland.org wrote:
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_WATCHDOGhelpIf 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.
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(...&priv->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