[PATCH 0/4] Improved sysreset/watchdog uclass integration

[PATCH 0/4] Improved sysreset/watchdog uclass integration

[Samuel Holland]

This series hooks up the watchdog uclass to automatically register the
first watchdog device for use with sysreset, doing a bit of minor
cleanup along the way.

The goal is for this to replace the sunxi board-level non-DM reset_cpu()
function. I was surprised to find that the wdt_reboot driver requires
its own undocumented device tree node, which references the watchdog
device by phandle. This is problematic for us, because sunxi-u-boot.dtsi
file covers 20 different SoCs with varying watchdog node phandle names.
So it would have required adding a -u-boot.dtsi file for each board.

Hooking things up automatically makes sense to me; this is what Linux
does. (In fact, Linux does this for every watchdog device.) However, I
put the code behind a new option to avoid surprises for other platforms.


Samuel Holland (4):
  sysreset: Add uclass Kconfig dependency to drivers
  sysreset: Mark driver probe functions as static
  sysreset: watchdog: Move watchdog reference to plat data
  watchdog: Automatically register device with sysreset

 drivers/sysreset/Kconfig             | 11 ++++++--
 drivers/sysreset/sysreset_resetctl.c |  2 +-
 drivers/sysreset/sysreset_syscon.c   |  2 +-
 drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------
 drivers/watchdog/wdt-uclass.c        |  5 ++++
 include/sysreset.h                   | 14 ++++++++++
 6 files changed, 62 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCH 1/4] sysreset: Add uclass Kconfig dependency to drivers

[Samuel Holland]

The drivers enabled by SYSRESET_SYSCON, SYSRESET_WATCHDOG, and
SYSRESET_RESETCTL do nothing beyond providing sysreset uclass ops.
Therefore, they should depend on the sysreset uclass.

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

 drivers/sysreset/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig
index ac77ffbc8be..fdc858ccbac 100644
--- a/drivers/sysreset/Kconfig
+++ b/drivers/sysreset/Kconfig
@@ -106,8 +106,6 @@ config SYSRESET_TI_SCI
 	  This enables the system reset driver support over TI System Control
 	  Interface available on some new TI's SoCs.
 
-endif
-
 config SYSRESET_SYSCON
 	bool "Enable support for mfd syscon reboot driver"
 	select REGMAP
@@ -127,6 +125,8 @@ config SYSRESET_RESETCTL
 	help
 	  Reboot support using generic reset controller.
 
+endif
+
 config SYSRESET_X86
 	bool "Enable support for x86 processor reboot driver"
 	depends on X86
-- 
2.31.1

[PATCH 2/4] sysreset: Mark driver probe functions as static

[Samuel Holland]

These driver probe functions are not (and should not be) called from
outside the respective driver source files. Therefore, the functions
should be marked static.

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

 drivers/sysreset/sysreset_resetctl.c | 2 +-
 drivers/sysreset/sysreset_syscon.c   | 2 +-
 drivers/sysreset/sysreset_watchdog.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/sysreset/sysreset_resetctl.c b/drivers/sysreset/sysreset_resetctl.c
index c039521eb43..25bd5c9a7ff 100644
--- a/drivers/sysreset/sysreset_resetctl.c
+++ b/drivers/sysreset/sysreset_resetctl.c
@@ -26,7 +26,7 @@ static struct sysreset_ops resetctl_reboot_ops = {
 	.request = resetctl_reboot_request,
 };
 
-int resetctl_reboot_probe(struct udevice *dev)
+static int resetctl_reboot_probe(struct udevice *dev)
 {
 	struct resetctl_reboot_priv *priv = dev_get_priv(dev);
 
diff --git a/drivers/sysreset/sysreset_syscon.c b/drivers/sysreset/sysreset_syscon.c
index 28fdfb09781..525faf2f89e 100644
--- a/drivers/sysreset/sysreset_syscon.c
+++ b/drivers/sysreset/sysreset_syscon.c
@@ -39,7 +39,7 @@ static struct sysreset_ops syscon_reboot_ops = {
 	.request = syscon_reboot_request,
 };
 
-int syscon_reboot_probe(struct udevice *dev)
+static int syscon_reboot_probe(struct udevice *dev)
 {
 	struct syscon_reboot_priv *priv = dev_get_priv(dev);
 	int err;
diff --git a/drivers/sysreset/sysreset_watchdog.c b/drivers/sysreset/sysreset_watchdog.c
index 0dc2d8b9b65..c7ae368d41a 100644
--- a/drivers/sysreset/sysreset_watchdog.c
+++ b/drivers/sysreset/sysreset_watchdog.c
@@ -29,7 +29,7 @@ static struct sysreset_ops wdt_reboot_ops = {
 	.request = wdt_reboot_request,
 };
 
-int wdt_reboot_probe(struct udevice *dev)
+static int wdt_reboot_probe(struct udevice *dev)
 {
 	struct wdt_reboot_priv *priv = dev_get_priv(dev);
 	int err;
-- 
2.31.1

[PATCH 3/4] sysreset: watchdog: Move watchdog reference to plat data

[Samuel Holland]

Currently, the wdt_reboot driver always gets its watchdog device
reference from an OF node. This prevents selecting a watchdog at
runtime. Move the watchdog device reference to the plat data, so
the driver can be bound with the reference pre-provided. The
reference will still be acquired from the OF node if it is not
already provided.

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

 drivers/sysreset/sysreset_watchdog.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/sysreset/sysreset_watchdog.c b/drivers/sysreset/sysreset_watchdog.c
index c7ae368d41a..b723f5647cd 100644
--- a/drivers/sysreset/sysreset_watchdog.c
+++ b/drivers/sysreset/sysreset_watchdog.c
@@ -9,16 +9,16 @@
 #include <sysreset.h>
 #include <wdt.h>
 
-struct wdt_reboot_priv {
+struct wdt_reboot_plat {
 	struct udevice *wdt;
 };
 
 static int wdt_reboot_request(struct udevice *dev, enum sysreset_t type)
 {
-	struct wdt_reboot_priv *priv = dev_get_priv(dev);
+	struct wdt_reboot_plat *plat = dev_get_plat(dev);
 	int ret;
 
-	ret = wdt_expire_now(priv->wdt, 0);
+	ret = wdt_expire_now(plat->wdt, 0);
 	if (ret)
 		return ret;
 
@@ -29,13 +29,13 @@ static struct sysreset_ops wdt_reboot_ops = {
 	.request = wdt_reboot_request,
 };
 
-static int wdt_reboot_probe(struct udevice *dev)
+static int wdt_reboot_of_to_plat(struct udevice *dev)
 {
-	struct wdt_reboot_priv *priv = dev_get_priv(dev);
+	struct wdt_reboot_plat *plat = dev_get_plat(dev);
 	int err;
 
 	err = uclass_get_device_by_phandle(UCLASS_WDT, dev,
-					   "wdt", &priv->wdt);
+					   "wdt", &plat->wdt);
 	if (err) {
 		pr_err("unable to find wdt device\n");
 		return err;
@@ -53,7 +53,7 @@ U_BOOT_DRIVER(wdt_reboot) = {
 	.name = "wdt_reboot",
 	.id = UCLASS_SYSRESET,
 	.of_match = wdt_reboot_ids,
+	.of_to_plat	= wdt_reboot_of_to_plat,
+	.plat_auto	= sizeof(struct wdt_reboot_plat),
 	.ops = &wdt_reboot_ops,
-	.priv_auto	= sizeof(struct wdt_reboot_priv),
-	.probe = wdt_reboot_probe,
 };
-- 
2.31.1

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

[Samuel Holland]

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

Re: [PATCH 1/4] sysreset: Add uclass Kconfig dependency to drivers

[Heinrich Schuchardt]

On 8/22/21 22:41, Samuel Holland wrote:
> The drivers enabled by SYSRESET_SYSCON, SYSRESET_WATCHDOG, and
> SYSRESET_RESETCTL do nothing beyond providing sysreset uclass ops.
> Therefore, they should depend on the sysreset uclass.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>   drivers/sysreset/Kconfig | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig
> index ac77ffbc8be..fdc858ccbac 100644
> --- a/drivers/sysreset/Kconfig
> +++ b/drivers/sysreset/Kconfig
> @@ -106,8 +106,6 @@ config SYSRESET_TI_SCI
>   	  This enables the system reset driver support over TI System Control
>   	  Interface available on some new TI's SoCs.
>   
> -endif
> -
>   config SYSRESET_SYSCON
>   	bool "Enable support for mfd syscon reboot driver"
>   	select REGMAP
> @@ -127,6 +125,8 @@ config SYSRESET_RESETCTL
>   	help
>   	  Reboot support using generic reset controller.
>   
> +endif
> +

All sysreset drivers in directory drivers/sysreset/ require 
sysreset-uclass.o. Please, move this endif to the very end of the 
Kconfig file.

Best regards

Heinrich

>   config SYSRESET_X86
>   	bool "Enable support for x86 processor reboot driver"
>   	depends on X86
> 

Re: [PATCH 2/4] sysreset: Mark driver probe functions as static

[Heinrich Schuchardt]

On 8/22/21 22:41, Samuel Holland wrote:
> These driver probe functions are not (and should not be) called from
> outside the respective driver source files. Therefore, the functions
> should be marked static.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Re: [PATCH 3/4] sysreset: watchdog: Move watchdog reference to plat data

[Heinrich Schuchardt]

On 8/22/21 22:41, Samuel Holland wrote:
> Currently, the wdt_reboot driver always gets its watchdog device
> reference from an OF node. This prevents selecting a watchdog at
> runtime. Move the watchdog device reference to the plat data, so
> the driver can be bound with the reference pre-provided. The
> reference will still be acquired from the OF node if it is not
> already provided.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

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

[Heinrich Schuchardt]

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
> 

Re: [PATCH 3/4] sysreset: watchdog: Move watchdog reference to plat data

[Simon Glass]

On Thu, 28 Oct 2021 at 05:18, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 8/22/21 22:41, Samuel Holland wrote:
> > Currently, the wdt_reboot driver always gets its watchdog device
> > reference from an OF node. This prevents selecting a watchdog at
> > runtime. Move the watchdog device reference to the plat data, so
> > the driver can be bound with the reference pre-provided. The
> > reference will still be acquired from the OF node if it is not
> > already provided.
> >
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

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

> +{
> +       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

Re: [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@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
> 

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

[Simon Glass]

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

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