[PATCH v3 0/6] Improved sysreset/watchdog uclass integration
This series hooks up the watchdog uclass to automatically register watchdog devices 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. However, I put the code behind a new option to avoid surprises for other platforms.
Changes in v3: - Move condition to wdt-uclass.c to fix build errors. - Include watchdog name in error message.
Changes in v2: - Extend the "if SYSRESET" block to the end of the file. - Also make gpio_reboot_probe function static. - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). - Added patches 5-6 as an example of how the new option will be used.
Samuel Holland (6): 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 sunxi: Avoid duplicate reset_cpu with SYSRESET enabled sunxi: Use sysreset framework for poweroff/reset
arch/arm/Kconfig | 3 +++ arch/arm/mach-sunxi/board.c | 2 ++ drivers/sysreset/Kconfig | 11 ++++++-- drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ drivers/watchdog/wdt-uclass.c | 8 ++++++ include/sysreset.h | 10 +++++++ 9 files changed, 67 insertions(+), 13 deletions(-)
None of the sysreset drivers do anything beyond providing sysreset uclass ops. They should depend on the sysreset uclass.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org ---
(no changes since v2)
Changes in v2: - Extend the "if SYSRESET" block to the end of the file.
drivers/sysreset/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index 43a948cfcd..de75c9cccc 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -118,8 +118,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 @@ -162,4 +160,6 @@ config SYSRESET_MPC83XX help Reboot support for NXP MPC83xx SoCs.
+endif + endmenu
On 04.11.21 04:55, Samuel Holland wrote:
None of the sysreset drivers do anything beyond providing sysreset uclass ops. They should depend on the sysreset uclass.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
(no changes since v2)
Changes in v2:
- Extend the "if SYSRESET" block to the end of the file.
drivers/sysreset/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index 43a948cfcd..de75c9cccc 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -118,8 +118,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
@@ -162,4 +160,6 @@ config SYSRESET_MPC83XX help Reboot support for NXP MPC83xx SoCs.
+endif
- endmenu
Viele Grüße, Stefan
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.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org ---
(no changes since v2)
Changes in v2: - Also make gpio_reboot_probe function static.
drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/sysreset/sysreset_gpio.c b/drivers/sysreset/sysreset_gpio.c index 680b759eb3..dfca10ccc8 100644 --- a/drivers/sysreset/sysreset_gpio.c +++ b/drivers/sysreset/sysreset_gpio.c @@ -33,7 +33,7 @@ static struct sysreset_ops gpio_reboot_ops = { .request = gpio_reboot_request, };
-int gpio_reboot_probe(struct udevice *dev) +static int gpio_reboot_probe(struct udevice *dev) { struct gpio_reboot_priv *priv = dev_get_priv(dev);
diff --git a/drivers/sysreset/sysreset_resetctl.c b/drivers/sysreset/sysreset_resetctl.c index c039521eb4..25bd5c9a7f 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 28fdfb0978..525faf2f89 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 0dc2d8b9b6..c7ae368d41 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;
On 04.11.21 04:55, 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.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
(no changes since v2)
Changes in v2:
- Also make gpio_reboot_probe function static.
drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/sysreset/sysreset_gpio.c b/drivers/sysreset/sysreset_gpio.c index 680b759eb3..dfca10ccc8 100644 --- a/drivers/sysreset/sysreset_gpio.c +++ b/drivers/sysreset/sysreset_gpio.c @@ -33,7 +33,7 @@ static struct sysreset_ops gpio_reboot_ops = { .request = gpio_reboot_request, };
-int gpio_reboot_probe(struct udevice *dev) +static int gpio_reboot_probe(struct udevice *dev) { struct gpio_reboot_priv *priv = dev_get_priv(dev);
diff --git a/drivers/sysreset/sysreset_resetctl.c b/drivers/sysreset/sysreset_resetctl.c index c039521eb4..25bd5c9a7f 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 28fdfb0978..525faf2f89 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 0dc2d8b9b6..c7ae368d41 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;
Viele Grüße, Stefan
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.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Samuel Holland samuel@sholland.org ---
(no changes since v1)
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 c7ae368d41..b723f5647c 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, };
On 04.11.21 04:55, 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.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
(no changes since v1)
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 c7ae368d41..b723f5647c 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);@@ -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, };
Viele Grüße, Stefan
Add an option to automatically register watchdog devices 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 ---
Changes in v3: - Move condition to wdt-uclass.c to fix build errors. - Include watchdog name in error message.
Changes in v2: - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
drivers/sysreset/Kconfig | 7 +++++++ drivers/sysreset/sysreset_watchdog.c | 24 ++++++++++++++++++++++++ drivers/watchdog/wdt-uclass.c | 8 ++++++++ include/sysreset.h | 10 ++++++++++ 4 files changed, 49 insertions(+)
diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index de75c9cccc..f6d60038b8 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -131,6 +131,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 b723f5647c..35efcac59d 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 7570710c4d..6d0f473867 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> @@ -44,6 +45,13 @@ static void init_watchdog_dev(struct udevice *dev)
priv = dev_get_uclass_priv(dev);
+ if (IS_ENABLED(CONFIG_SYSRESET_WATCHDOG_AUTO)) { + ret = sysreset_register_wdt(dev); + if (ret) + printf("WDT: Failed to register %s for sysreset\n", + dev->name); + } + if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) { printf("WDT: Not starting %s\n", dev->name); return; diff --git a/include/sysreset.h b/include/sysreset.h index 9d4ed87cea..ff20abdeed 100644 --- a/include/sysreset.h +++ b/include/sysreset.h @@ -133,4 +133,14 @@ 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 + */ +int sysreset_register_wdt(struct udevice *dev); + #endif
On 04.11.21 04:55, Samuel Holland wrote:
Add an option to automatically register watchdog devices 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
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
Changes in v3:
- Move condition to wdt-uclass.c to fix build errors.
- Include watchdog name in error message.
Changes in v2:
- Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
drivers/sysreset/Kconfig | 7 +++++++ drivers/sysreset/sysreset_watchdog.c | 24 ++++++++++++++++++++++++ drivers/watchdog/wdt-uclass.c | 8 ++++++++ include/sysreset.h | 10 ++++++++++ 4 files changed, 49 insertions(+)
diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig index de75c9cccc..f6d60038b8 100644 --- a/drivers/sysreset/Kconfig +++ b/drivers/sysreset/Kconfig @@ -131,6 +131,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 b723f5647c..35efcac59d 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 7570710c4d..6d0f473867 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> @@ -44,6 +45,13 @@ static void init_watchdog_dev(struct udevice *dev)
priv = dev_get_uclass_priv(dev);
- if (IS_ENABLED(CONFIG_SYSRESET_WATCHDOG_AUTO)) {
ret = sysreset_register_wdt(dev);if (ret)printf("WDT: Failed to register %s for sysreset\n",dev->name);- }
- if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) { printf("WDT: Not starting %s\n", dev->name); return;
diff --git a/include/sysreset.h b/include/sysreset.h index 9d4ed87cea..ff20abdeed 100644 --- a/include/sysreset.h +++ b/include/sysreset.h @@ -133,4 +133,14 @@ 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
- */
+int sysreset_register_wdt(struct udevice *dev);
- #endif
Viele Grüße, Stefan
On 11/4/21 08:48, Stefan Roese wrote:
On 04.11.21 04:55, Samuel Holland wrote:
Add an option to automatically register watchdog devices 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
Reviewed-by: Stefan Roese sr@denx.de
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
The sysreset uclass unconditionally provides a definition of the reset_cpu() function. So does the sunxi board code. Fix the build with SYSRESET enabled by omitting the function from the board code in that case. The code still needs to be kept around for use in SPL.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org ---
(no changes since v2)
Changes in v2: - New patch
arch/arm/mach-sunxi/board.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c index b4ba2a72c4..3ef179742c 100644 --- a/arch/arm/mach-sunxi/board.c +++ b/arch/arm/mach-sunxi/board.c @@ -346,6 +346,7 @@ void board_init_f(ulong dummy) } #endif
+#if !CONFIG_IS_ENABLED(SYSRESET) void reset_cpu(void) { #if defined(CONFIG_SUNXI_GEN_SUN4I) || defined(CONFIG_MACH_SUN8I_R40) @@ -376,6 +377,7 @@ void reset_cpu(void) while (1) { } #endif } +#endif
#if !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) && !defined(CONFIG_ARM64) void enable_caches(void)
On 04.11.21 04:55, Samuel Holland wrote:
The sysreset uclass unconditionally provides a definition of the reset_cpu() function. So does the sunxi board code. Fix the build with SYSRESET enabled by omitting the function from the board code in that case. The code still needs to be kept around for use in SPL.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
(no changes since v2)
Changes in v2:
- New patch
arch/arm/mach-sunxi/board.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c index b4ba2a72c4..3ef179742c 100644 --- a/arch/arm/mach-sunxi/board.c +++ b/arch/arm/mach-sunxi/board.c @@ -346,6 +346,7 @@ void board_init_f(ulong dummy) } #endif
+#if !CONFIG_IS_ENABLED(SYSRESET) void reset_cpu(void) { #if defined(CONFIG_SUNXI_GEN_SUN4I) || defined(CONFIG_MACH_SUN8I_R40) @@ -376,6 +377,7 @@ void reset_cpu(void) while (1) { } #endif } +#endif
#if !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) && !defined(CONFIG_ARM64) void enable_caches(void)
Viele Grüße, Stefan
Instead of hardcoding the watchdog for reset, and the PMIC for poweroff, use the sysreset framework to manage the available poweroff/reset backends. This allows (as examples) using the PMIC to do a cold reset, and using a GPIO to power off H3/H5 boards lacking a PMIC. Furthermore, it removes the need to hardcode watchdog MMIO addresses, since the sysreset backends can be discovered using the device tree.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org ---
(no changes since v2)
Changes in v2: - New patch
arch/arm/Kconfig | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index b4808d4c75..ae911d6e35 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1084,6 +1084,9 @@ config ARCH_SUNXI imply SPL_MMC if MMC imply SPL_POWER imply SPL_SERIAL + imply SYSRESET + imply SYSRESET_WATCHDOG + imply SYSRESET_WATCHDOG_AUTO imply USB_GADGET imply WDT
On 04.11.21 04:55, Samuel Holland wrote:
Instead of hardcoding the watchdog for reset, and the PMIC for poweroff, use the sysreset framework to manage the available poweroff/reset backends. This allows (as examples) using the PMIC to do a cold reset, and using a GPIO to power off H3/H5 boards lacking a PMIC. Furthermore, it removes the need to hardcode watchdog MMIO addresses, since the sysreset backends can be discovered using the device tree.
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
(no changes since v2)
Changes in v2:
- New patch
arch/arm/Kconfig | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index b4808d4c75..ae911d6e35 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1084,6 +1084,9 @@ config ARCH_SUNXI imply SPL_MMC if MMC imply SPL_POWER imply SPL_SERIAL
- imply SYSRESET
- imply SYSRESET_WATCHDOG
- imply SYSRESET_WATCHDOG_AUTO imply USB_GADGET imply WDT
Viele Grüße, Stefan
On 04.11.21 04:55, Samuel Holland wrote:
This series hooks up the watchdog uclass to automatically register watchdog devices 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. However, I put the code behind a new option to avoid surprises for other platforms.
Changes in v3:
- Move condition to wdt-uclass.c to fix build errors.
- Include watchdog name in error message.
Changes in v2:
- Extend the "if SYSRESET" block to the end of the file.
- Also make gpio_reboot_probe function static.
- Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
- Added patches 5-6 as an example of how the new option will be used.
Samuel Holland (6): 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 sunxi: Avoid duplicate reset_cpu with SYSRESET enabled sunxi: Use sysreset framework for poweroff/reset
arch/arm/Kconfig | 3 +++ arch/arm/mach-sunxi/board.c | 2 ++ drivers/sysreset/Kconfig | 11 ++++++-- drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ drivers/watchdog/wdt-uclass.c | 8 ++++++ include/sysreset.h | 10 +++++++ 9 files changed, 67 insertions(+), 13 deletions(-)
Applied to u-boot-marvell
Thanks, Stefan
On Thu, 4 Nov 2021 11:37:57 +0100 Stefan Roese sr@denx.de wrote:
Hi Stefan,
On 04.11.21 04:55, Samuel Holland wrote:
This series hooks up the watchdog uclass to automatically register watchdog devices 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. However, I put the code behind a new option to avoid surprises for other platforms.
Changes in v3:
- Move condition to wdt-uclass.c to fix build errors.
- Include watchdog name in error message.
Changes in v2:
- Extend the "if SYSRESET" block to the end of the file.
- Also make gpio_reboot_probe function static.
- Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
- Added patches 5-6 as an example of how the new option will be used.
Samuel Holland (6): 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 sunxi: Avoid duplicate reset_cpu with SYSRESET enabled sunxi: Use sysreset framework for poweroff/reset
arch/arm/Kconfig | 3 +++ arch/arm/mach-sunxi/board.c | 2 ++ drivers/sysreset/Kconfig | 11 ++++++-- drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ drivers/watchdog/wdt-uclass.c | 8 ++++++ include/sysreset.h | 10 +++++++ 9 files changed, 67 insertions(+), 13 deletions(-)
Applied to u-boot-marvell
Mmmh, why u-boot-marvell, and why did this end up already in master? Isn't that material for the next merge window? After all this changes quite a bit, for a lot of boards, and I did not have a closer look at the sunxi parts yet.
Cheers, Andre
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote:
On Thu, 4 Nov 2021 11:37:57 +0100 Stefan Roese sr@denx.de wrote:
Hi Stefan,
On 04.11.21 04:55, Samuel Holland wrote:
This series hooks up the watchdog uclass to automatically register watchdog devices 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. However, I put the code behind a new option to avoid surprises for other platforms.
Changes in v3:
- Move condition to wdt-uclass.c to fix build errors.
- Include watchdog name in error message.
Changes in v2:
- Extend the "if SYSRESET" block to the end of the file.
- Also make gpio_reboot_probe function static.
- Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
- Added patches 5-6 as an example of how the new option will be used.
Samuel Holland (6): 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 sunxi: Avoid duplicate reset_cpu with SYSRESET enabled sunxi: Use sysreset framework for poweroff/reset
arch/arm/Kconfig | 3 +++ arch/arm/mach-sunxi/board.c | 2 ++ drivers/sysreset/Kconfig | 11 ++++++-- drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ drivers/watchdog/wdt-uclass.c | 8 ++++++ include/sysreset.h | 10 +++++++ 9 files changed, 67 insertions(+), 13 deletions(-)
Applied to u-boot-marvell
Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
and why did this end up already in master? Isn't that material for the next merge window? After all this changes quite a bit, for a lot of boards, and I did not have a closer look at the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Do you see any specific issues?
Thanks, Stefan
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote:
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote:
On Thu, 4 Nov 2021 11:37:57 +0100 Stefan Roese sr@denx.de wrote:
Hi Stefan,
On 04.11.21 04:55, Samuel Holland wrote:
This series hooks up the watchdog uclass to automatically register watchdog devices 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. However, I put the code behind a new option to avoid surprises for other platforms.
Changes in v3:
- Move condition to wdt-uclass.c to fix build errors.
- Include watchdog name in error message.
Changes in v2:
- Extend the "if SYSRESET" block to the end of the file.
- Also make gpio_reboot_probe function static.
- Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
- Added patches 5-6 as an example of how the new option will be used.
Samuel Holland (6): 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 sunxi: Avoid duplicate reset_cpu with SYSRESET enabled sunxi: Use sysreset framework for poweroff/reset
arch/arm/Kconfig | 3 +++ arch/arm/mach-sunxi/board.c | 2 ++ drivers/sysreset/Kconfig | 11 ++++++-- drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ drivers/watchdog/wdt-uclass.c | 8 ++++++ include/sysreset.h | 10 +++++++ 9 files changed, 67 insertions(+), 13 deletions(-)
Applied to u-boot-marvell
Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
and why did this end up already in master? Isn't that material for the next merge window? After all this changes quite a bit, for a lot of boards, and I did not have a closer look at the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Yes I agree, that should be plenty of time for people to review it.
Do you see any specific issues?
Regards, Simon
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote:
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote:
On Thu, 4 Nov 2021 11:37:57 +0100 Stefan Roese sr@denx.de wrote:
Hi Stefan,
On 04.11.21 04:55, Samuel Holland wrote:
This series hooks up the watchdog uclass to automatically register watchdog devices 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. However, I put the code behind a new option to avoid surprises for other platforms.
Changes in v3:
- Move condition to wdt-uclass.c to fix build errors.
- Include watchdog name in error message.
Changes in v2:
- Extend the "if SYSRESET" block to the end of the file.
- Also make gpio_reboot_probe function static.
- Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
- Added patches 5-6 as an example of how the new option will be used.
Samuel Holland (6): 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 sunxi: Avoid duplicate reset_cpu with SYSRESET enabled sunxi: Use sysreset framework for poweroff/reset
arch/arm/Kconfig | 3 +++ arch/arm/mach-sunxi/board.c | 2 ++ drivers/sysreset/Kconfig | 11 ++++++-- drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ drivers/watchdog/wdt-uclass.c | 8 ++++++ include/sysreset.h | 10 +++++++ 9 files changed, 67 insertions(+), 13 deletions(-)
Applied to u-boot-marvell
Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
and why did this end up already in master? Isn't that material for the next merge window? After all this changes quite a bit, for a lot of boards, and I did not have a closer look at the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I will do as much testing now as possible, but I am not happy about that situation.
Cheers, Andre
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote:
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote:
On Thu, 4 Nov 2021 11:37:57 +0100 Stefan Roese sr@denx.de wrote:
Hi Stefan,
On 04.11.21 04:55, Samuel Holland wrote:
This series hooks up the watchdog uclass to automatically register watchdog devices 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. However, I put the code behind a new option to avoid surprises for other platforms.
Changes in v3: - Move condition to wdt-uclass.c to fix build errors. - Include watchdog name in error message.
Changes in v2: - Extend the "if SYSRESET" block to the end of the file. - Also make gpio_reboot_probe function static. - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). - Added patches 5-6 as an example of how the new option will be used.
Samuel Holland (6): 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 sunxi: Avoid duplicate reset_cpu with SYSRESET enabled sunxi: Use sysreset framework for poweroff/reset
arch/arm/Kconfig | 3 +++ arch/arm/mach-sunxi/board.c | 2 ++ drivers/sysreset/Kconfig | 11 ++++++-- drivers/sysreset/sysreset_gpio.c | 2 +- drivers/sysreset/sysreset_resetctl.c | 2 +- drivers/sysreset/sysreset_syscon.c | 2 +- drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ drivers/watchdog/wdt-uclass.c | 8 ++++++ include/sysreset.h | 10 +++++++ 9 files changed, 67 insertions(+), 13 deletions(-)Applied to u-boot-marvell
Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
and why did this end up already in master? Isn't that material for the next merge window? After all this changes quite a bit, for a lot of boards, and I did not have a closer look at the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
Thanks, Stefan
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote:
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote:
On Thu, 4 Nov 2021 11:37:57 +0100 Stefan Roese sr@denx.de wrote:
Hi Stefan,
On 04.11.21 04:55, Samuel Holland wrote: > This series hooks up the watchdog uclass to automatically register > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > other platforms. > > Changes in v3: > - Move condition to wdt-uclass.c to fix build errors. > - Include watchdog name in error message. > > Changes in v2: > - Extend the "if SYSRESET" block to the end of the file. > - Also make gpio_reboot_probe function static. > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > - Added patches 5-6 as an example of how the new option will be used. > > Samuel Holland (6): > 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 > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > sunxi: Use sysreset framework for poweroff/reset > > arch/arm/Kconfig | 3 +++ > arch/arm/mach-sunxi/board.c | 2 ++ > drivers/sysreset/Kconfig | 11 ++++++-- > drivers/sysreset/sysreset_gpio.c | 2 +- > drivers/sysreset/sysreset_resetctl.c | 2 +- > drivers/sysreset/sysreset_syscon.c | 2 +- > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > drivers/watchdog/wdt-uclass.c | 8 ++++++ > include/sysreset.h | 10 +++++++ > 9 files changed, 67 insertions(+), 13 deletions(-)
Applied to u-boot-marvell
Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
and why did this end up already in master? Isn't that material for the next merge window? After all this changes quite a bit, for a lot of boards, and I did not have a closer look at the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote:
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote:
On Thu, 4 Nov 2021 11:37:57 +0100 Stefan Roese sr@denx.de wrote:
Hi Stefan, > On 04.11.21 04:55, Samuel Holland wrote: > > This series hooks up the watchdog uclass to automatically register > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > other platforms. > > > > Changes in v3: > > - Move condition to wdt-uclass.c to fix build errors. > > - Include watchdog name in error message. > > > > Changes in v2: > > - Extend the "if SYSRESET" block to the end of the file. > > - Also make gpio_reboot_probe function static. > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > - Added patches 5-6 as an example of how the new option will be used. > > > > Samuel Holland (6): > > 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 > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > sunxi: Use sysreset framework for poweroff/reset > > > > arch/arm/Kconfig | 3 +++ > > arch/arm/mach-sunxi/board.c | 2 ++ > > drivers/sysreset/Kconfig | 11 ++++++-- > > drivers/sysreset/sysreset_gpio.c | 2 +- > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > drivers/sysreset/sysreset_syscon.c | 2 +- > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > include/sysreset.h | 10 +++++++ > > 9 files changed, 67 insertions(+), 13 deletions(-) > > Applied to u-boot-marvell
Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
and why did this end up already in master? Isn't that material for the next merge window? After all this changes quite a bit, for a lot of boards, and I did not have a closer look at the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
On Fri, 5 Nov 2021 10:12:11 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote:
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote: > On Thu, 4 Nov 2021 11:37:57 +0100 > Stefan Roese sr@denx.de wrote: > > Hi Stefan, > > On 04.11.21 04:55, Samuel Holland wrote: > > > This series hooks up the watchdog uclass to automatically register > > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > > other platforms. > > > > > > Changes in v3: > > > - Move condition to wdt-uclass.c to fix build errors. > > > - Include watchdog name in error message. > > > > > > Changes in v2: > > > - Extend the "if SYSRESET" block to the end of the file. > > > - Also make gpio_reboot_probe function static. > > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > > - Added patches 5-6 as an example of how the new option will be used. > > > > > > Samuel Holland (6): > > > 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 > > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > > sunxi: Use sysreset framework for poweroff/reset > > > > > > arch/arm/Kconfig | 3 +++ > > > arch/arm/mach-sunxi/board.c | 2 ++ > > > drivers/sysreset/Kconfig | 11 ++++++-- > > > drivers/sysreset/sysreset_gpio.c | 2 +- > > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > > drivers/sysreset/sysreset_syscon.c | 2 +- > > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > > include/sysreset.h | 10 +++++++ > > > 9 files changed, 67 insertions(+), 13 deletions(-) > > > > Applied to u-boot-marvell > > Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
No, that's fine, I was just briefly confused about the Marvell part.
> and why did this end up already in master? > Isn't that material for the next merge window? After all this changes > quite a bit, for a lot of boards, and I did not have a closer look at > the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
Understood.
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
To avoid churn, I am fine with keeping it in. If my testing reveals issue, we can still revert later.
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
That's all true, but it would still be nice to get at least some ACK or confirmation from the platform maintainers before merging patches that affect that many boards.
Cheers, Andre
Hi Andre,
On Fri, 5 Nov 2021 at 11:07, Andre Przywara andre.przywara@arm.com wrote:
On Fri, 5 Nov 2021 10:12:11 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: > > Hi Andre, > > On 05.11.21 00:11, Andre Przywara wrote: > > On Thu, 4 Nov 2021 11:37:57 +0100 > > Stefan Roese sr@denx.de wrote: > > > > Hi Stefan, > > > On 04.11.21 04:55, Samuel Holland wrote: > > > > This series hooks up the watchdog uclass to automatically register > > > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > > > other platforms. > > > > > > > > Changes in v3: > > > > - Move condition to wdt-uclass.c to fix build errors. > > > > - Include watchdog name in error message. > > > > > > > > Changes in v2: > > > > - Extend the "if SYSRESET" block to the end of the file. > > > > - Also make gpio_reboot_probe function static. > > > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > > > - Added patches 5-6 as an example of how the new option will be used. > > > > > > > > Samuel Holland (6): > > > > 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 > > > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > > > sunxi: Use sysreset framework for poweroff/reset > > > > > > > > arch/arm/Kconfig | 3 +++ > > > > arch/arm/mach-sunxi/board.c | 2 ++ > > > > drivers/sysreset/Kconfig | 11 ++++++-- > > > > drivers/sysreset/sysreset_gpio.c | 2 +- > > > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > > > drivers/sysreset/sysreset_syscon.c | 2 +- > > > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > > > include/sysreset.h | 10 +++++++ > > > > 9 files changed, 67 insertions(+), 13 deletions(-) > > > > > > Applied to u-boot-marvell > > > > Mmmh, why u-boot-marvell, > > Because I'm handling watchdog related changed since a few years and we > did not create a specific subsystem repo for this and I'm usually > using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
No, that's fine, I was just briefly confused about the Marvell part.
> > and why did this end up already in master? > > Isn't that material for the next merge window? After all this changes > > quite a bit, for a lot of boards, and I did not have a closer look at > > the sunxi parts yet. > > I was hesitating also a bit. But since this patchset is on the list in > v1 since over 2 months now (2021-08-21) I thought it was "ready" for > inclusion now. We are at -rc1 and I think we still have enough time to > fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
Understood.
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
> Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
To avoid churn, I am fine with keeping it in. If my testing reveals issue, we can still revert later.
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
That's all true, but it would still be nice to get at least some ACK or confirmation from the platform maintainers before merging patches that affect that many boards.
It would be nice, but after a few weeks we have to assume that it is OK, otherwise patches would just sit there. The release cycle should provide a way to resolve any issues in plenty of time. We have already extended the cycle from 2 months to 3 months...
Regards, Simon
On 11/5/21 17:12, Simon Glass wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote:
Hi Andre,
On 05.11.21 00:11, Andre Przywara wrote: > On Thu, 4 Nov 2021 11:37:57 +0100 > Stefan Roese sr@denx.de wrote: > > Hi Stefan, >> On 04.11.21 04:55, Samuel Holland wrote: >>> This series hooks up the watchdog uclass to automatically register >>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>> other platforms. >>> >>> Changes in v3: >>> - Move condition to wdt-uclass.c to fix build errors. >>> - Include watchdog name in error message. >>> >>> Changes in v2: >>> - Extend the "if SYSRESET" block to the end of the file. >>> - Also make gpio_reboot_probe function static. >>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>> - Added patches 5-6 as an example of how the new option will be used. >>> >>> Samuel Holland (6): >>> 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 >>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>> sunxi: Use sysreset framework for poweroff/reset >>> >>> arch/arm/Kconfig | 3 +++ >>> arch/arm/mach-sunxi/board.c | 2 ++ >>> drivers/sysreset/Kconfig | 11 ++++++-- >>> drivers/sysreset/sysreset_gpio.c | 2 +- >>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>> drivers/sysreset/sysreset_syscon.c | 2 +- >>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>> include/sysreset.h | 10 +++++++ >>> 9 files changed, 67 insertions(+), 13 deletions(-) >> >> Applied to u-boot-marvell > > Mmmh, why u-boot-marvell,
Because I'm handling watchdog related changed since a few years and we did not create a specific subsystem repo for this and I'm usually using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
> and why did this end up already in master? > Isn't that material for the next merge window? After all this changes > quite a bit, for a lot of boards, and I did not have a closer look at > the sunxi parts yet.
I was hesitating also a bit. But since this patchset is on the list in v1 since over 2 months now (2021-08-21) I thought it was "ready" for inclusion now. We are at -rc1 and I think we still have enough time to fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run
=> wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
Best regards
Heinrich
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 17:12, Simon Glass wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: > > Hi Andre, > > On 05.11.21 00:11, Andre Przywara wrote: > > On Thu, 4 Nov 2021 11:37:57 +0100 > > Stefan Roese sr@denx.de wrote: > > > > Hi Stefan, > > > On 04.11.21 04:55, Samuel Holland wrote: > > > > This series hooks up the watchdog uclass to automatically register > > > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > > > other platforms. > > > > > > > > Changes in v3: > > > > - Move condition to wdt-uclass.c to fix build errors. > > > > - Include watchdog name in error message. > > > > > > > > Changes in v2: > > > > - Extend the "if SYSRESET" block to the end of the file. > > > > - Also make gpio_reboot_probe function static. > > > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > > > - Added patches 5-6 as an example of how the new option will be used. > > > > > > > > Samuel Holland (6): > > > > 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 > > > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > > > sunxi: Use sysreset framework for poweroff/reset > > > > > > > > arch/arm/Kconfig | 3 +++ > > > > arch/arm/mach-sunxi/board.c | 2 ++ > > > > drivers/sysreset/Kconfig | 11 ++++++-- > > > > drivers/sysreset/sysreset_gpio.c | 2 +- > > > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > > > drivers/sysreset/sysreset_syscon.c | 2 +- > > > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > > > include/sysreset.h | 10 +++++++ > > > > 9 files changed, 67 insertions(+), 13 deletions(-) > > > > > > Applied to u-boot-marvell > > > > Mmmh, why u-boot-marvell, > > Because I'm handling watchdog related changed since a few years and we > did not create a specific subsystem repo for this and I'm usually > using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
> > and why did this end up already in master? > > Isn't that material for the next merge window? After all this changes > > quite a bit, for a lot of boards, and I did not have a closer look at > > the sunxi parts yet. > > I was hesitating also a bit. But since this patchset is on the list in > v1 since over 2 months now (2021-08-21) I thought it was "ready" for > inclusion now. We are at -rc1 and I think we still have enough time to > fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
> Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run
=> wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
On 11/5/21 20:17, Tom Rini wrote:
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 17:12, Simon Glass wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote:
On Thu, 4 Nov 2021 20:02:41 -0600 Simon Glass sjg@chromium.org wrote:
Hi,
> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >> >> Hi Andre, >> >> On 05.11.21 00:11, Andre Przywara wrote: >>> On Thu, 4 Nov 2021 11:37:57 +0100 >>> Stefan Roese sr@denx.de wrote: >>> >>> Hi Stefan, >>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>> This series hooks up the watchdog uclass to automatically register >>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>> other platforms. >>>>> >>>>> Changes in v3: >>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>> - Include watchdog name in error message. >>>>> >>>>> Changes in v2: >>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>> - Also make gpio_reboot_probe function static. >>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>> >>>>> Samuel Holland (6): >>>>> 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 >>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>> sunxi: Use sysreset framework for poweroff/reset >>>>> >>>>> arch/arm/Kconfig | 3 +++ >>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>> include/sysreset.h | 10 +++++++ >>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>> >>>> Applied to u-boot-marvell >>> >>> Mmmh, why u-boot-marvell, >> >> Because I'm handling watchdog related changed since a few years and we >> did not create a specific subsystem repo for this and I'm usually >> using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
>>> and why did this end up already in master? >>> Isn't that material for the next merge window? After all this changes >>> quite a bit, for a lot of boards, and I did not have a closer look at >>> the sunxi parts yet. >> >> I was hesitating also a bit. But since this patchset is on the list in >> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >> inclusion now. We are at -rc1 and I think we still have enough time to >> fix any resulting problems in this release cycle.
Why do we have the merge window then? This is clearly not a regression or general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
> Yes I agree, that should be plenty of time for people to review it.
Well, if there would be people to review the sunxi parts :-( I am totally fine with the generic patches (as they have been reviewed), but the sunxi integration is somewhat risky. I was explicitly deprioritising that in my queue, as it really doesn't change, add or fix anything, it's mere refactoring, from the user's point of view.
>> Do you see any specific issues?
Patch 6/6 changes the config for all 157 Allwinner boards, so I think that deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
I will do as much testing now as possible, but I am not happy about that situation.
Understood. Should we revert patch 6/6 for now?
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run
=> wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
Best regards
Heinrich
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 20:17, Tom Rini wrote:
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 17:12, Simon Glass wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
Hi Andre,
Added Tom to Cc.
On 05.11.21 11:04, Andre Przywara wrote: > On Thu, 4 Nov 2021 20:02:41 -0600 > Simon Glass sjg@chromium.org wrote: > > Hi, > > > On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: > > > > > > Hi Andre, > > > > > > On 05.11.21 00:11, Andre Przywara wrote: > > > > On Thu, 4 Nov 2021 11:37:57 +0100 > > > > Stefan Roese sr@denx.de wrote: > > > > > > > > Hi Stefan, > > > > > On 04.11.21 04:55, Samuel Holland wrote: > > > > > > This series hooks up the watchdog uclass to automatically register > > > > > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > > > > > other platforms. > > > > > > > > > > > > Changes in v3: > > > > > > - Move condition to wdt-uclass.c to fix build errors. > > > > > > - Include watchdog name in error message. > > > > > > > > > > > > Changes in v2: > > > > > > - Extend the "if SYSRESET" block to the end of the file. > > > > > > - Also make gpio_reboot_probe function static. > > > > > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > > > > > - Added patches 5-6 as an example of how the new option will be used. > > > > > > > > > > > > Samuel Holland (6): > > > > > > 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 > > > > > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > > > > > sunxi: Use sysreset framework for poweroff/reset > > > > > > > > > > > > arch/arm/Kconfig | 3 +++ > > > > > > arch/arm/mach-sunxi/board.c | 2 ++ > > > > > > drivers/sysreset/Kconfig | 11 ++++++-- > > > > > > drivers/sysreset/sysreset_gpio.c | 2 +- > > > > > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > > > > > drivers/sysreset/sysreset_syscon.c | 2 +- > > > > > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > > > > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > > > > > include/sysreset.h | 10 +++++++ > > > > > > 9 files changed, 67 insertions(+), 13 deletions(-) > > > > > > > > > > Applied to u-boot-marvell > > > > > > > > Mmmh, why u-boot-marvell, > > > > > > Because I'm handling watchdog related changed since a few years and we > > > did not create a specific subsystem repo for this and I'm usually > > > using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
> > > > and why did this end up already in master? > > > > Isn't that material for the next merge window? After all this changes > > > > quite a bit, for a lot of boards, and I did not have a closer look at > > > > the sunxi parts yet. > > > > > > I was hesitating also a bit. But since this patchset is on the list in > > > v1 since over 2 months now (2021-08-21) I thought it was "ready" for > > > inclusion now. We are at -rc1 and I think we still have enough time to > > > fix any resulting problems in this release cycle. > > Why do we have the merge window then? This is clearly not a regression or > general fix.
AFAIU, we are a bit less strict here in U-Boot. Patches that were posted before the merge-window and skipped the review process (most likely because of lack of time) are often still integrated in the early rcX cycles. At least this is how I handle it usually.
Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
> > Yes I agree, that should be plenty of time for people to review it. > > Well, if there would be people to review the sunxi parts :-( > I am totally fine with the generic patches (as they have been reviewed), > but the sunxi integration is somewhat risky. > I was explicitly deprioritising that in my queue, as it really doesn't > change, add or fix anything, it's mere refactoring, from the user's point > of view. > > > > Do you see any specific issues? > > Patch 6/6 changes the config for all 157 Allwinner boards, so I think that > deserves at least some testing, *before* merging it.
I expect that Samuel did some testing. But still, I agree that it would be much better, if these patches - especially the Allwinner parts got more extensive testing.
> I will do as much testing now as possible, but I am not happy about that > situation.
Understood. Should we revert patch 6/6 for now?
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run
=> wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable. Now, if the Andre says he's fine just disabling watchdog by default for sunxi, fine. But yes, we never did come up with a reasonable solution to UEFI saying 5 minute timeout for watchdog servicing vs other platforms using a much shorter watchdog period.
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 20:17, Tom Rini wrote:
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 17:12, Simon Glass wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: > Hi Andre, > > Added Tom to Cc. > > On 05.11.21 11:04, Andre Przywara wrote: > > On Thu, 4 Nov 2021 20:02:41 -0600 > > Simon Glass sjg@chromium.org wrote: > > > > Hi, > > > > > On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: > > > > > > > > Hi Andre, > > > > > > > > On 05.11.21 00:11, Andre Przywara wrote: > > > > > On Thu, 4 Nov 2021 11:37:57 +0100 > > > > > Stefan Roese sr@denx.de wrote: > > > > > > > > > > Hi Stefan, > > > > > > On 04.11.21 04:55, Samuel Holland wrote: > > > > > > > This series hooks up the watchdog uclass to automatically register > > > > > > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > > > > > > other platforms. > > > > > > > > > > > > > > Changes in v3: > > > > > > > - Move condition to wdt-uclass.c to fix build errors. > > > > > > > - Include watchdog name in error message. > > > > > > > > > > > > > > Changes in v2: > > > > > > > - Extend the "if SYSRESET" block to the end of the file. > > > > > > > - Also make gpio_reboot_probe function static. > > > > > > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > > > > > > - Added patches 5-6 as an example of how the new option will be used. > > > > > > > > > > > > > > Samuel Holland (6): > > > > > > > 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 > > > > > > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > > > > > > sunxi: Use sysreset framework for poweroff/reset > > > > > > > > > > > > > > arch/arm/Kconfig | 3 +++ > > > > > > > arch/arm/mach-sunxi/board.c | 2 ++ > > > > > > > drivers/sysreset/Kconfig | 11 ++++++-- > > > > > > > drivers/sysreset/sysreset_gpio.c | 2 +- > > > > > > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > > > > > > drivers/sysreset/sysreset_syscon.c | 2 +- > > > > > > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > > > > > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > > > > > > include/sysreset.h | 10 +++++++ > > > > > > > 9 files changed, 67 insertions(+), 13 deletions(-) > > > > > > > > > > > > Applied to u-boot-marvell > > > > > > > > > > Mmmh, why u-boot-marvell, > > > > > > > > Because I'm handling watchdog related changed since a few years and we > > > > did not create a specific subsystem repo for this and I'm usually > > > > using my "marvell" one for this.
And fwiw, there's a few other cases like this. If it's too confusing, maybe we should just roll out a few more repositories, I think it's easier to do that now than pre-gitlab?
> > > > > and why did this end up already in master? > > > > > Isn't that material for the next merge window? After all this changes > > > > > quite a bit, for a lot of boards, and I did not have a closer look at > > > > > the sunxi parts yet. > > > > > > > > I was hesitating also a bit. But since this patchset is on the list in > > > > v1 since over 2 months now (2021-08-21) I thought it was "ready" for > > > > inclusion now. We are at -rc1 and I think we still have enough time to > > > > fix any resulting problems in this release cycle. > > > > Why do we have the merge window then? This is clearly not a regression or > > general fix. > > AFAIU, we are a bit less strict here in U-Boot. Patches that were posted > before the merge-window and skipped the review process (most likely > because of lack of time) are often still integrated in the early rcX > cycles. At least this is how I handle it usually. > > Tom, is my understanding here correct?
Yes. We are not as strict as the kernel is about what can come in between rc1 and rc2 (and to a certain degree, post rc2). I leave things up to the discretion of the custodians. People tend of have less time to handle U-Boot changes than other stuff, so I try and be flexible in picking things up.
> > > Yes I agree, that should be plenty of time for people to review it. > > > > Well, if there would be people to review the sunxi parts :-( > > I am totally fine with the generic patches (as they have been reviewed), > > but the sunxi integration is somewhat risky. > > I was explicitly deprioritising that in my queue, as it really doesn't > > change, add or fix anything, it's mere refactoring, from the user's point > > of view. > > > > > > Do you see any specific issues? > > > > Patch 6/6 changes the config for all 157 Allwinner boards, so I think that > > deserves at least some testing, *before* merging it. > > I expect that Samuel did some testing. But still, I agree that it > would be much better, if these patches - especially the Allwinner parts > got more extensive testing. > > > I will do as much testing now as possible, but I am not happy about that > > situation. > > Understood. Should we revert patch 6/6 for now?
FWIW, given Samuel has been doing a number of allwinner changes, I had also assumed it was sufficiently tested, which is why I didn't raise a further concern when I saw the widespread nature of the overall changes, just figured it was a few more ready-to-go cleanups that weren't quite picked up in time. Please do speak up if you want me to revert the last part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run
=> wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
Now, if the Andre says he's fine just disabling watchdog by default for sunxi, fine. But yes, we never did come up with a reasonable solution to UEFI saying 5 minute timeout for watchdog servicing vs other platforms using a much shorter watchdog period.
On 11/6/21 02:52, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 20:17, Tom Rini wrote:
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 17:12, Simon Glass wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: > > On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >> Hi Andre, >> >> Added Tom to Cc. >> >> On 05.11.21 11:04, Andre Przywara wrote: >>> On Thu, 4 Nov 2021 20:02:41 -0600 >>> Simon Glass sjg@chromium.org wrote: >>> >>> Hi, >>> >>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>> >>>>> Hi Andre, >>>>> >>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>> Stefan Roese sr@denx.de wrote: >>>>>> >>>>>> Hi Stefan, >>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>> other platforms. >>>>>>>> >>>>>>>> Changes in v3: >>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>> - Include watchdog name in error message. >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>> >>>>>>>> Samuel Holland (6): >>>>>>>> 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 >>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>> >>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> Applied to u-boot-marvell >>>>>> >>>>>> Mmmh, why u-boot-marvell, >>>>> >>>>> Because I'm handling watchdog related changed since a few years and we >>>>> did not create a specific subsystem repo for this and I'm usually >>>>> using my "marvell" one for this. > > And fwiw, there's a few other cases like this. If it's too confusing, > maybe we should just roll out a few more repositories, I think it's > easier to do that now than pre-gitlab? > >>>>>> and why did this end up already in master? >>>>>> Isn't that material for the next merge window? After all this changes >>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>> the sunxi parts yet. >>>>> >>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>> fix any resulting problems in this release cycle. >>> >>> Why do we have the merge window then? This is clearly not a regression or >>> general fix. >> >> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >> before the merge-window and skipped the review process (most likely >> because of lack of time) are often still integrated in the early rcX >> cycles. At least this is how I handle it usually. >> >> Tom, is my understanding here correct? > > Yes. We are not as strict as the kernel is about what can come in > between rc1 and rc2 (and to a certain degree, post rc2). I leave things > up to the discretion of the custodians. People tend of have less time > to handle U-Boot changes than other stuff, so I try and be flexible in > picking things up. > >>>> Yes I agree, that should be plenty of time for people to review it. >>> >>> Well, if there would be people to review the sunxi parts :-( >>> I am totally fine with the generic patches (as they have been reviewed), >>> but the sunxi integration is somewhat risky. >>> I was explicitly deprioritising that in my queue, as it really doesn't >>> change, add or fix anything, it's mere refactoring, from the user's point >>> of view. >>> >>>>> Do you see any specific issues? >>> >>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>> deserves at least some testing, *before* merging it. >> >> I expect that Samuel did some testing. But still, I agree that it >> would be much better, if these patches - especially the Allwinner parts >> got more extensive testing. >> >>> I will do as much testing now as possible, but I am not happy about that >>> situation. >> >> Understood. Should we revert patch 6/6 for now? > > FWIW, given Samuel has been doing a number of allwinner changes, I had > also assumed it was sufficiently tested, which is why I didn't raise a > further concern when I saw the widespread nature of the overall changes, > just figured it was a few more ready-to-go cleanups that weren't quite > picked up in time. Please do speak up if you want me to revert the last > part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run
=> wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
Best regards
Heinrich
Now, if the Andre says he's fine just disabling watchdog by default for sunxi, fine. But yes, we never did come up with a reasonable solution to UEFI saying 5 minute timeout for watchdog servicing vs other platforms using a much shorter watchdog period.
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 20:17, Tom Rini wrote:
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 17:12, Simon Glass wrote: > Hi, > > On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: > > > > On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: > > > Hi Andre, > > > > > > Added Tom to Cc. > > > > > > On 05.11.21 11:04, Andre Przywara wrote: > > > > On Thu, 4 Nov 2021 20:02:41 -0600 > > > > Simon Glass sjg@chromium.org wrote: > > > > > > > > Hi, > > > > > On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: > > > > > > > > > > > > Hi Andre, > > > > > > > > > > > > On 05.11.21 00:11, Andre Przywara wrote: > > > > > > > On Thu, 4 Nov 2021 11:37:57 +0100 > > > > > > > Stefan Roese sr@denx.de wrote: > > > > > > > > > > > > > > Hi Stefan, > > > > > > > > On 04.11.21 04:55, Samuel Holland wrote: > > > > > > > > > This series hooks up the watchdog uclass to automatically register > > > > > > > > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > > > > > > > > other platforms. > > > > > > > > > > > > > > > > > > Changes in v3: > > > > > > > > > - Move condition to wdt-uclass.c to fix build errors. > > > > > > > > > - Include watchdog name in error message. > > > > > > > > > > > > > > > > > > Changes in v2: > > > > > > > > > - Extend the "if SYSRESET" block to the end of the file. > > > > > > > > > - Also make gpio_reboot_probe function static. > > > > > > > > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > > > > > > > > - Added patches 5-6 as an example of how the new option will be used. > > > > > > > > > > > > > > > > > > Samuel Holland (6): > > > > > > > > > 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 > > > > > > > > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > > > > > > > > sunxi: Use sysreset framework for poweroff/reset > > > > > > > > > > > > > > > > > > arch/arm/Kconfig | 3 +++ > > > > > > > > > arch/arm/mach-sunxi/board.c | 2 ++ > > > > > > > > > drivers/sysreset/Kconfig | 11 ++++++-- > > > > > > > > > drivers/sysreset/sysreset_gpio.c | 2 +- > > > > > > > > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > > > > > > > > drivers/sysreset/sysreset_syscon.c | 2 +- > > > > > > > > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > > > > > > > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > > > > > > > > include/sysreset.h | 10 +++++++ > > > > > > > > > 9 files changed, 67 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > > > Applied to u-boot-marvell > > > > > > > > > > > > > > Mmmh, why u-boot-marvell, > > > > > > > > > > > > Because I'm handling watchdog related changed since a few years and we > > > > > > did not create a specific subsystem repo for this and I'm usually > > > > > > using my "marvell" one for this. > > > > And fwiw, there's a few other cases like this. If it's too confusing, > > maybe we should just roll out a few more repositories, I think it's > > easier to do that now than pre-gitlab? > > > > > > > and why did this end up already in master? > > > > > > > Isn't that material for the next merge window? After all this changes > > > > > > > quite a bit, for a lot of boards, and I did not have a closer look at > > > > > > > the sunxi parts yet. > > > > > > > > > > > > I was hesitating also a bit. But since this patchset is on the list in > > > > > > v1 since over 2 months now (2021-08-21) I thought it was "ready" for > > > > > > inclusion now. We are at -rc1 and I think we still have enough time to > > > > > > fix any resulting problems in this release cycle. > > > > > > > > Why do we have the merge window then? This is clearly not a regression or > > > > general fix. > > > > > > AFAIU, we are a bit less strict here in U-Boot. Patches that were posted > > > before the merge-window and skipped the review process (most likely > > > because of lack of time) are often still integrated in the early rcX > > > cycles. At least this is how I handle it usually. > > > > > > Tom, is my understanding here correct? > > > > Yes. We are not as strict as the kernel is about what can come in > > between rc1 and rc2 (and to a certain degree, post rc2). I leave things > > up to the discretion of the custodians. People tend of have less time > > to handle U-Boot changes than other stuff, so I try and be flexible in > > picking things up. > > > > > Yes I agree, that should be plenty of time for people to review it. > > > > > > > > Well, if there would be people to review the sunxi parts :-( > > > > I am totally fine with the generic patches (as they have been reviewed), > > > > but the sunxi integration is somewhat risky. > > > > I was explicitly deprioritising that in my queue, as it really doesn't > > > > change, add or fix anything, it's mere refactoring, from the user's point > > > > of view. > > > > > > Do you see any specific issues? > > > > > > > > Patch 6/6 changes the config for all 157 Allwinner boards, so I think that > > > > deserves at least some testing, *before* merging it. > > > > > > I expect that Samuel did some testing. But still, I agree that it > > > would be much better, if these patches - especially the Allwinner parts > > > got more extensive testing. > > > > I will do as much testing now as possible, but I am not happy about that > > > > situation. > > > > > > Understood. Should we revert patch 6/6 for now? > > > > FWIW, given Samuel has been doing a number of allwinner changes, I had > > also assumed it was sufficiently tested, which is why I didn't raise a > > further concern when I saw the widespread nature of the overall changes, > > just figured it was a few more ready-to-go cleanups that weren't quite > > picked up in time. Please do speak up if you want me to revert the last > > part. > > Also it is often true that people find problems by testing on master > so applying it helps to shake the tree a bit. > > Regards, > Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run => wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 20:17, Tom Rini wrote:
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: > On 11/5/21 17:12, Simon Glass wrote: >> Hi, >> >> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>> >>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>> Hi Andre, >>>> >>>> Added Tom to Cc. >>>> >>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>> Simon Glass sjg@chromium.org wrote: >>>>> >>>>> Hi, >>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>> >>>>>>> Hi Andre, >>>>>>> >>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>> >>>>>>>> Hi Stefan, >>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>> other platforms. >>>>>>>>>> >>>>>>>>>> Changes in v3: >>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>> >>>>>>>>>> Changes in v2: >>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>> >>>>>>>>>> Samuel Holland (6): >>>>>>>>>> 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 >>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>> >>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>> >>>>>>>>> Applied to u-boot-marvell >>>>>>>> >>>>>>>> Mmmh, why u-boot-marvell, >>>>>>> >>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>> using my "marvell" one for this. >>> >>> And fwiw, there's a few other cases like this. If it's too confusing, >>> maybe we should just roll out a few more repositories, I think it's >>> easier to do that now than pre-gitlab? >>>>>>>> and why did this end up already in master? >>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>> the sunxi parts yet. >>>>>>> >>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>> fix any resulting problems in this release cycle. >>>>> >>>>> Why do we have the merge window then? This is clearly not a regression or >>>>> general fix. >>>> >>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>> before the merge-window and skipped the review process (most likely >>>> because of lack of time) are often still integrated in the early rcX >>>> cycles. At least this is how I handle it usually. >>>> >>>> Tom, is my understanding here correct? >>> >>> Yes. We are not as strict as the kernel is about what can come in >>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>> up to the discretion of the custodians. People tend of have less time >>> to handle U-Boot changes than other stuff, so I try and be flexible in >>> picking things up. >>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>> >>>>> Well, if there would be people to review the sunxi parts :-( >>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>> but the sunxi integration is somewhat risky. >>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>> of view. >>>>>>> Do you see any specific issues? >>>>> >>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>> deserves at least some testing, *before* merging it. >>>> >>>> I expect that Samuel did some testing. But still, I agree that it >>>> would be much better, if these patches - especially the Allwinner parts >>>> got more extensive testing. >>>>> I will do as much testing now as possible, but I am not happy about that >>>>> situation. >>>> >>>> Understood. Should we revert patch 6/6 for now? >>> >>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>> also assumed it was sufficiently tested, which is why I didn't raise a >>> further concern when I saw the widespread nature of the overall changes, >>> just figured it was a few more ready-to-go cleanups that weren't quite >>> picked up in time. Please do speak up if you want me to revert the last >>> part. >> >> Also it is often true that people find problems by testing on master >> so applying it helps to shake the tree a bit. >> >> Regards, >> Simon > > We don't actually have a problem with this series but with a previous > watchdog patch. The culprit according to bisecting is: > > b147bd3607f8 ("sunxi: Enable watchdog timer support by default") > > When booting the OrangePi PC the watchdog triggers while Linux is booting, > ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in > drivers/watchdog/sunxi_wdt.c. > > If I run > => wdt dev watchdog@1c20ca0 > => wdt stop > > before the bootefi command booting succeeds. > > We don't disarm the watchdog and Linux does not do it for us in time. > > The UEFI specification requires that the default watchdog reset time is 300 > s. We should never arm the Sunxi hardware watchdog except within the > watchdog reset driver. > > The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See > > [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards > https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Best regards
Heinrich
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 20:17, Tom Rini wrote: > On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >> On 11/5/21 17:12, Simon Glass wrote: >>> Hi, >>> >>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>> >>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>> Hi Andre, >>>>> >>>>> Added Tom to Cc. >>>>> >>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>> Simon Glass sjg@chromium.org wrote: >>>>>> >>>>>> Hi, >>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>> >>>>>>>> Hi Andre, >>>>>>>> >>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>> >>>>>>>>> Hi Stefan, >>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>> other platforms. >>>>>>>>>>> >>>>>>>>>>> Changes in v3: >>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>> >>>>>>>>>>> Changes in v2: >>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>> >>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>> 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 >>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>> >>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>> >>>>>>>>>> Applied to u-boot-marvell >>>>>>>>> >>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>> >>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>> using my "marvell" one for this. >>>> >>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>> maybe we should just roll out a few more repositories, I think it's >>>> easier to do that now than pre-gitlab? >>>>>>>>> and why did this end up already in master? >>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>> the sunxi parts yet. >>>>>>>> >>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>> fix any resulting problems in this release cycle. >>>>>> >>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>> general fix. >>>>> >>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>> before the merge-window and skipped the review process (most likely >>>>> because of lack of time) are often still integrated in the early rcX >>>>> cycles. At least this is how I handle it usually. >>>>> >>>>> Tom, is my understanding here correct? >>>> >>>> Yes. We are not as strict as the kernel is about what can come in >>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>> up to the discretion of the custodians. People tend of have less time >>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>> picking things up. >>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>> >>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>> but the sunxi integration is somewhat risky. >>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>> of view. >>>>>>>> Do you see any specific issues? >>>>>> >>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>> deserves at least some testing, *before* merging it. >>>>> >>>>> I expect that Samuel did some testing. But still, I agree that it >>>>> would be much better, if these patches - especially the Allwinner parts >>>>> got more extensive testing. >>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>> situation. >>>>> >>>>> Understood. Should we revert patch 6/6 for now? >>>> >>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>> further concern when I saw the widespread nature of the overall changes, >>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>> picked up in time. Please do speak up if you want me to revert the last >>>> part. >>> >>> Also it is often true that people find problems by testing on master >>> so applying it helps to shake the tree a bit. >>> >>> Regards, >>> Simon >> >> We don't actually have a problem with this series but with a previous >> watchdog patch. The culprit according to bisecting is: >> >> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >> >> When booting the OrangePi PC the watchdog triggers while Linux is booting, >> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >> drivers/watchdog/sunxi_wdt.c. >> >> If I run >> => wdt dev watchdog@1c20ca0 >> => wdt stop >> >> before the bootefi command booting succeeds. >> >> We don't disarm the watchdog and Linux does not do it for us in time. >> >> The UEFI specification requires that the default watchdog reset time is 300 >> s. We should never arm the Sunxi hardware watchdog except within the >> watchdog reset driver. >> >> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >> >> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html > > This means we never did come up with a satisfactory to everyone solution > to what UEFI thinks a watchdog should do, and what other types of > deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
Regards, Simon
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: > On 11/5/21 20:17, Tom Rini wrote: >> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>> On 11/5/21 17:12, Simon Glass wrote: >>>> Hi, >>>> >>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>> >>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>> Hi Andre, >>>>>> >>>>>> Added Tom to Cc. >>>>>> >>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>> >>>>>>> Hi, >>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>> >>>>>>>>> Hi Andre, >>>>>>>>> >>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>> >>>>>>>>>> Hi Stefan, >>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>> other platforms. >>>>>>>>>>>> >>>>>>>>>>>> Changes in v3: >>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>> >>>>>>>>>>>> Changes in v2: >>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>> >>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>> 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 >>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>> >>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>> >>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>> >>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>> >>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>> using my "marvell" one for this. >>>>> >>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>> maybe we should just roll out a few more repositories, I think it's >>>>> easier to do that now than pre-gitlab? >>>>>>>>>> and why did this end up already in master? >>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>> the sunxi parts yet. >>>>>>>>> >>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>> fix any resulting problems in this release cycle. >>>>>>> >>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>> general fix. >>>>>> >>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>> before the merge-window and skipped the review process (most likely >>>>>> because of lack of time) are often still integrated in the early rcX >>>>>> cycles. At least this is how I handle it usually. >>>>>> >>>>>> Tom, is my understanding here correct? >>>>> >>>>> Yes. We are not as strict as the kernel is about what can come in >>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>> up to the discretion of the custodians. People tend of have less time >>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>> picking things up. >>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>> >>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>> but the sunxi integration is somewhat risky. >>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>> of view. >>>>>>>>> Do you see any specific issues? >>>>>>> >>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>> deserves at least some testing, *before* merging it. >>>>>> >>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>> got more extensive testing. >>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>> situation. >>>>>> >>>>>> Understood. Should we revert patch 6/6 for now? >>>>> >>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>> further concern when I saw the widespread nature of the overall changes, >>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>> picked up in time. Please do speak up if you want me to revert the last >>>>> part. >>>> >>>> Also it is often true that people find problems by testing on master >>>> so applying it helps to shake the tree a bit. >>>> >>>> Regards, >>>> Simon >>> >>> We don't actually have a problem with this series but with a previous >>> watchdog patch. The culprit according to bisecting is: >>> >>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>> >>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>> drivers/watchdog/sunxi_wdt.c. >>> >>> If I run >>> => wdt dev watchdog@1c20ca0 >>> => wdt stop >>> >>> before the bootefi command booting succeeds. >>> >>> We don't disarm the watchdog and Linux does not do it for us in time. >>> >>> The UEFI specification requires that the default watchdog reset time is 300 >>> s. We should never arm the Sunxi hardware watchdog except within the >>> watchdog reset driver. >>> >>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>> >>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >> >> This means we never did come up with a satisfactory to everyone solution >> to what UEFI thinks a watchdog should do, and what other types of >> deployment think a watchdog should do, yes? > > Dear Tom, > > The issue is *not* UEFI specific. > > A watchdog timeout of 16 seconds is too short for Linux to boot no matter > whether you use the EFI stub or the legacy entry point. > > I only referred to the UEFI specification as it indicates what can be > considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Hi Tom,
On Mon, 8 Nov 2021 at 09:05, Tom Rini trini@konsulko.com wrote:
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
> On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: >> On 11/5/21 20:17, Tom Rini wrote: >>> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>>> On 11/5/21 17:12, Simon Glass wrote: >>>>> Hi, >>>>> >>>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>>> >>>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>>> Hi Andre, >>>>>>> >>>>>>> Added Tom to Cc. >>>>>>> >>>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>>> >>>>>>>>>> Hi Andre, >>>>>>>>>> >>>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Stefan, >>>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>>> other platforms. >>>>>>>>>>>>> >>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>>> >>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>>> >>>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>>> 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 >>>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>>> >>>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>>> >>>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>>> >>>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>>> using my "marvell" one for this. >>>>>> >>>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>>> maybe we should just roll out a few more repositories, I think it's >>>>>> easier to do that now than pre-gitlab? >>>>>>>>>>> and why did this end up already in master? >>>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>>> the sunxi parts yet. >>>>>>>>>> >>>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>>> fix any resulting problems in this release cycle. >>>>>>>> >>>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>>> general fix. >>>>>>> >>>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>>> before the merge-window and skipped the review process (most likely >>>>>>> because of lack of time) are often still integrated in the early rcX >>>>>>> cycles. At least this is how I handle it usually. >>>>>>> >>>>>>> Tom, is my understanding here correct? >>>>>> >>>>>> Yes. We are not as strict as the kernel is about what can come in >>>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>>> up to the discretion of the custodians. People tend of have less time >>>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>>> picking things up. >>>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>>> >>>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>>> but the sunxi integration is somewhat risky. >>>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>>> of view. >>>>>>>>>> Do you see any specific issues? >>>>>>>> >>>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>>> deserves at least some testing, *before* merging it. >>>>>>> >>>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>>> got more extensive testing. >>>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>>> situation. >>>>>>> >>>>>>> Understood. Should we revert patch 6/6 for now? >>>>>> >>>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>>> further concern when I saw the widespread nature of the overall changes, >>>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>>> picked up in time. Please do speak up if you want me to revert the last >>>>>> part. >>>>> >>>>> Also it is often true that people find problems by testing on master >>>>> so applying it helps to shake the tree a bit. >>>>> >>>>> Regards, >>>>> Simon >>>> >>>> We don't actually have a problem with this series but with a previous >>>> watchdog patch. The culprit according to bisecting is: >>>> >>>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>>> >>>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>>> drivers/watchdog/sunxi_wdt.c. >>>> >>>> If I run >>>> => wdt dev watchdog@1c20ca0 >>>> => wdt stop >>>> >>>> before the bootefi command booting succeeds. >>>> >>>> We don't disarm the watchdog and Linux does not do it for us in time. >>>> >>>> The UEFI specification requires that the default watchdog reset time is 300 >>>> s. We should never arm the Sunxi hardware watchdog except within the >>>> watchdog reset driver. >>>> >>>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>>> >>>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >>> >>> This means we never did come up with a satisfactory to everyone solution >>> to what UEFI thinks a watchdog should do, and what other types of >>> deployment think a watchdog should do, yes? >> >> Dear Tom, >> >> The issue is *not* UEFI specific. >> >> A watchdog timeout of 16 seconds is too short for Linux to boot no matter >> whether you use the EFI stub or the legacy entry point. >> >> I only referred to the UEFI specification as it indicates what can be >> considered as a reasonable timeout interval: 300 seconds. > > 16 seconds from the last time we pet the watchdog in U-Boot to the > kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Well at least we could have a hook to display a warning message. I don't understand why 16 seconds is too long, actually. Is this because it is going via grub?
Regards, Simon
On Mon, Nov 08, 2021 at 09:09:20AM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 8 Nov 2021 at 09:05, Tom Rini trini@konsulko.com wrote:
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote: > On Fri, 5 Nov 2021 18:56:34 -0400 > Tom Rini trini@konsulko.com wrote: > >> On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: >>> On 11/5/21 20:17, Tom Rini wrote: >>>> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>>>> On 11/5/21 17:12, Simon Glass wrote: >>>>>> Hi, >>>>>> >>>>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>>>> >>>>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>>>> Hi Andre, >>>>>>>> >>>>>>>> Added Tom to Cc. >>>>>>>> >>>>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Andre, >>>>>>>>>>> >>>>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Stefan, >>>>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>>>> other platforms. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>>>> 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 >>>>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>>>> >>>>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>>>> >>>>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>>>> >>>>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>>>> using my "marvell" one for this. >>>>>>> >>>>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>>>> maybe we should just roll out a few more repositories, I think it's >>>>>>> easier to do that now than pre-gitlab? >>>>>>>>>>>> and why did this end up already in master? >>>>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>>>> the sunxi parts yet. >>>>>>>>>>> >>>>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>>>> fix any resulting problems in this release cycle. >>>>>>>>> >>>>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>>>> general fix. >>>>>>>> >>>>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>>>> before the merge-window and skipped the review process (most likely >>>>>>>> because of lack of time) are often still integrated in the early rcX >>>>>>>> cycles. At least this is how I handle it usually. >>>>>>>> >>>>>>>> Tom, is my understanding here correct? >>>>>>> >>>>>>> Yes. We are not as strict as the kernel is about what can come in >>>>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>>>> up to the discretion of the custodians. People tend of have less time >>>>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>>>> picking things up. >>>>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>>>> >>>>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>>>> but the sunxi integration is somewhat risky. >>>>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>>>> of view. >>>>>>>>>>> Do you see any specific issues? >>>>>>>>> >>>>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>>>> deserves at least some testing, *before* merging it. >>>>>>>> >>>>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>>>> got more extensive testing. >>>>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>>>> situation. >>>>>>>> >>>>>>>> Understood. Should we revert patch 6/6 for now? >>>>>>> >>>>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>>>> further concern when I saw the widespread nature of the overall changes, >>>>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>>>> picked up in time. Please do speak up if you want me to revert the last >>>>>>> part. >>>>>> >>>>>> Also it is often true that people find problems by testing on master >>>>>> so applying it helps to shake the tree a bit. >>>>>> >>>>>> Regards, >>>>>> Simon >>>>> >>>>> We don't actually have a problem with this series but with a previous >>>>> watchdog patch. The culprit according to bisecting is: >>>>> >>>>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>>>> >>>>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>>>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>>>> drivers/watchdog/sunxi_wdt.c. >>>>> >>>>> If I run >>>>> => wdt dev watchdog@1c20ca0 >>>>> => wdt stop >>>>> >>>>> before the bootefi command booting succeeds. >>>>> >>>>> We don't disarm the watchdog and Linux does not do it for us in time. >>>>> >>>>> The UEFI specification requires that the default watchdog reset time is 300 >>>>> s. We should never arm the Sunxi hardware watchdog except within the >>>>> watchdog reset driver. >>>>> >>>>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>>>> >>>>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>>>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >>>> >>>> This means we never did come up with a satisfactory to everyone solution >>>> to what UEFI thinks a watchdog should do, and what other types of >>>> deployment think a watchdog should do, yes? >>> >>> Dear Tom, >>> >>> The issue is *not* UEFI specific. >>> >>> A watchdog timeout of 16 seconds is too short for Linux to boot no matter >>> whether you use the EFI stub or the legacy entry point. >>> >>> I only referred to the UEFI specification as it indicates what can be >>> considered as a reasonable timeout interval: 300 seconds. >> >> 16 seconds from the last time we pet the watchdog in U-Boot to the >> kernel being able to take over is quite reasonable. > > How do we know that the kernel takes over? What if the kernel/EFI > payload doesn't have a watchdog driver? I was assuming that the > watchdog would be disabled as soon as we boot a kernel or an EFI app > calls ExitBootServices (maybe even earlier). > But this sounds like a generic problem, not sunxi specific. So how do > other platforms solve this? > > Cheers, > Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Well at least we could have a hook to display a warning message. I don't understand why 16 seconds is too long, actually. Is this because it is going via grub?
As Heinrich noted, it takes from kernel start more than 16 seconds to reach the point in the boot sequence where the initrd is found (so no, not EFI slowing things down), loaded and the watchdog module loaded. And since I was surprised here, yes, all watchdogs are done as modules in the upstream Debian kernel. Other platforms I gather just have a longer maximum period.
On 11/8/21 17:05, Tom Rini wrote:
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
> On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: >> On 11/5/21 20:17, Tom Rini wrote: >>> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>>> On 11/5/21 17:12, Simon Glass wrote: >>>>> Hi, >>>>> >>>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>>> >>>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>>> Hi Andre, >>>>>>> >>>>>>> Added Tom to Cc. >>>>>>> >>>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>>> >>>>>>>>>> Hi Andre, >>>>>>>>>> >>>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Stefan, >>>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>>> other platforms. >>>>>>>>>>>>> >>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>>> >>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>>> >>>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>>> 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 >>>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>>> >>>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>>> >>>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>>> >>>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>>> using my "marvell" one for this. >>>>>> >>>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>>> maybe we should just roll out a few more repositories, I think it's >>>>>> easier to do that now than pre-gitlab? >>>>>>>>>>> and why did this end up already in master? >>>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>>> the sunxi parts yet. >>>>>>>>>> >>>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>>> fix any resulting problems in this release cycle. >>>>>>>> >>>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>>> general fix. >>>>>>> >>>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>>> before the merge-window and skipped the review process (most likely >>>>>>> because of lack of time) are often still integrated in the early rcX >>>>>>> cycles. At least this is how I handle it usually. >>>>>>> >>>>>>> Tom, is my understanding here correct? >>>>>> >>>>>> Yes. We are not as strict as the kernel is about what can come in >>>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>>> up to the discretion of the custodians. People tend of have less time >>>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>>> picking things up. >>>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>>> >>>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>>> but the sunxi integration is somewhat risky. >>>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>>> of view. >>>>>>>>>> Do you see any specific issues? >>>>>>>> >>>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>>> deserves at least some testing, *before* merging it. >>>>>>> >>>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>>> got more extensive testing. >>>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>>> situation. >>>>>>> >>>>>>> Understood. Should we revert patch 6/6 for now? >>>>>> >>>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>>> further concern when I saw the widespread nature of the overall changes, >>>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>>> picked up in time. Please do speak up if you want me to revert the last >>>>>> part. >>>>> >>>>> Also it is often true that people find problems by testing on master >>>>> so applying it helps to shake the tree a bit. >>>>> >>>>> Regards, >>>>> Simon >>>> >>>> We don't actually have a problem with this series but with a previous >>>> watchdog patch. The culprit according to bisecting is: >>>> >>>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>>> >>>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>>> drivers/watchdog/sunxi_wdt.c. >>>> >>>> If I run >>>> => wdt dev watchdog@1c20ca0 >>>> => wdt stop >>>> >>>> before the bootefi command booting succeeds. >>>> >>>> We don't disarm the watchdog and Linux does not do it for us in time. >>>> >>>> The UEFI specification requires that the default watchdog reset time is 300 >>>> s. We should never arm the Sunxi hardware watchdog except within the >>>> watchdog reset driver. >>>> >>>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>>> >>>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >>> >>> This means we never did come up with a satisfactory to everyone solution >>> to what UEFI thinks a watchdog should do, and what other types of >>> deployment think a watchdog should do, yes? >> >> Dear Tom, >> >> The issue is *not* UEFI specific. >> >> A watchdog timeout of 16 seconds is too short for Linux to boot no matter >> whether you use the EFI stub or the legacy entry point. >> >> I only referred to the UEFI specification as it indicates what can be >> considered as a reasonable timeout interval: 300 seconds. > > 16 seconds from the last time we pet the watchdog in U-Boot to the > kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
Cheers, Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I have no problem with people switching on the SUNXI hardware watchdog for their specific embedded solution. But here it was switched on for all SUNXI boards and breaks booting into Linux distributions.
If the Linux distribution resets the watchdog *after* file system checks because it is managed by systemd (as is true for Debian), 5 minutes is realistic.
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
The problem is not UEFI related.
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Yes, Sunxi is limited to 16 seconds.
Best regards
Heinrich
Hi Heinrich,
On Mon, 8 Nov 2021 at 09:17, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/8/21 17:05, Tom Rini wrote:
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote: > On Fri, 5 Nov 2021 18:56:34 -0400 > Tom Rini trini@konsulko.com wrote: > >> On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: >>> On 11/5/21 20:17, Tom Rini wrote: >>>> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>>>> On 11/5/21 17:12, Simon Glass wrote: >>>>>> Hi, >>>>>> >>>>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>>>> >>>>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>>>> Hi Andre, >>>>>>>> >>>>>>>> Added Tom to Cc. >>>>>>>> >>>>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Andre, >>>>>>>>>>> >>>>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Stefan, >>>>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>>>> other platforms. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>>>> 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 >>>>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>>>> >>>>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>>>> >>>>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>>>> >>>>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>>>> using my "marvell" one for this. >>>>>>> >>>>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>>>> maybe we should just roll out a few more repositories, I think it's >>>>>>> easier to do that now than pre-gitlab? >>>>>>>>>>>> and why did this end up already in master? >>>>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>>>> the sunxi parts yet. >>>>>>>>>>> >>>>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>>>> fix any resulting problems in this release cycle. >>>>>>>>> >>>>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>>>> general fix. >>>>>>>> >>>>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>>>> before the merge-window and skipped the review process (most likely >>>>>>>> because of lack of time) are often still integrated in the early rcX >>>>>>>> cycles. At least this is how I handle it usually. >>>>>>>> >>>>>>>> Tom, is my understanding here correct? >>>>>>> >>>>>>> Yes. We are not as strict as the kernel is about what can come in >>>>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>>>> up to the discretion of the custodians. People tend of have less time >>>>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>>>> picking things up. >>>>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>>>> >>>>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>>>> but the sunxi integration is somewhat risky. >>>>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>>>> of view. >>>>>>>>>>> Do you see any specific issues? >>>>>>>>> >>>>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>>>> deserves at least some testing, *before* merging it. >>>>>>>> >>>>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>>>> got more extensive testing. >>>>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>>>> situation. >>>>>>>> >>>>>>>> Understood. Should we revert patch 6/6 for now? >>>>>>> >>>>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>>>> further concern when I saw the widespread nature of the overall changes, >>>>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>>>> picked up in time. Please do speak up if you want me to revert the last >>>>>>> part. >>>>>> >>>>>> Also it is often true that people find problems by testing on master >>>>>> so applying it helps to shake the tree a bit. >>>>>> >>>>>> Regards, >>>>>> Simon >>>>> >>>>> We don't actually have a problem with this series but with a previous >>>>> watchdog patch. The culprit according to bisecting is: >>>>> >>>>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>>>> >>>>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>>>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>>>> drivers/watchdog/sunxi_wdt.c. >>>>> >>>>> If I run >>>>> => wdt dev watchdog@1c20ca0 >>>>> => wdt stop >>>>> >>>>> before the bootefi command booting succeeds. >>>>> >>>>> We don't disarm the watchdog and Linux does not do it for us in time. >>>>> >>>>> The UEFI specification requires that the default watchdog reset time is 300 >>>>> s. We should never arm the Sunxi hardware watchdog except within the >>>>> watchdog reset driver. >>>>> >>>>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>>>> >>>>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>>>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >>>> >>>> This means we never did come up with a satisfactory to everyone solution >>>> to what UEFI thinks a watchdog should do, and what other types of >>>> deployment think a watchdog should do, yes? >>> >>> Dear Tom, >>> >>> The issue is *not* UEFI specific. >>> >>> A watchdog timeout of 16 seconds is too short for Linux to boot no matter >>> whether you use the EFI stub or the legacy entry point. >>> >>> I only referred to the UEFI specification as it indicates what can be >>> considered as a reasonable timeout interval: 300 seconds. >> >> 16 seconds from the last time we pet the watchdog in U-Boot to the >> kernel being able to take over is quite reasonable. > > How do we know that the kernel takes over? What if the kernel/EFI > payload doesn't have a watchdog driver? I was assuming that the > watchdog would be disabled as soon as we boot a kernel or an EFI app > calls ExitBootServices (maybe even earlier). > But this sounds like a generic problem, not sunxi specific. So how do > other platforms solve this? > > Cheers, > Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I have no problem with people switching on the SUNXI hardware watchdog for their specific embedded solution. But here it was switched on for all SUNXI boards and breaks booting into Linux distributions.
If the Linux distribution resets the watchdog *after* file system checks because it is managed by systemd (as is true for Debian), 5 minutes is realistic.
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
The problem is not UEFI related.
Oh dear, that is definitely quite slow.
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Yes, Sunxi is limited to 16 seconds.
I suppose it isn't possible to disable the watchdog once it is enabled?
Regards, Simon
On Mon, Nov 08, 2021 at 05:09:14PM -0700, Simon Glass wrote:
Hi Heinrich,
On Mon, 8 Nov 2021 at 09:17, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/8/21 17:05, Tom Rini wrote:
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote: > > > On 11/6/21 02:52, Andre Przywara wrote: >> On Fri, 5 Nov 2021 18:56:34 -0400 >> Tom Rini trini@konsulko.com wrote: >> >>> On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: >>>> On 11/5/21 20:17, Tom Rini wrote: >>>>> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>>>>> On 11/5/21 17:12, Simon Glass wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>>>>> >>>>>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>>>>> Hi Andre, >>>>>>>>> >>>>>>>>> Added Tom to Cc. >>>>>>>>> >>>>>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Andre, >>>>>>>>>>>> >>>>>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Stefan, >>>>>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>>>>> other platforms. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>>>>> >>>>>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>>>>> >>>>>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>>>>> using my "marvell" one for this. >>>>>>>> >>>>>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>>>>> maybe we should just roll out a few more repositories, I think it's >>>>>>>> easier to do that now than pre-gitlab? >>>>>>>>>>>>> and why did this end up already in master? >>>>>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>>>>> the sunxi parts yet. >>>>>>>>>>>> >>>>>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>>>>> fix any resulting problems in this release cycle. >>>>>>>>>> >>>>>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>>>>> general fix. >>>>>>>>> >>>>>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>>>>> before the merge-window and skipped the review process (most likely >>>>>>>>> because of lack of time) are often still integrated in the early rcX >>>>>>>>> cycles. At least this is how I handle it usually. >>>>>>>>> >>>>>>>>> Tom, is my understanding here correct? >>>>>>>> >>>>>>>> Yes. We are not as strict as the kernel is about what can come in >>>>>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>>>>> up to the discretion of the custodians. People tend of have less time >>>>>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>>>>> picking things up. >>>>>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>>>>> >>>>>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>>>>> but the sunxi integration is somewhat risky. >>>>>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>>>>> of view. >>>>>>>>>>>> Do you see any specific issues? >>>>>>>>>> >>>>>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>>>>> deserves at least some testing, *before* merging it. >>>>>>>>> >>>>>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>>>>> got more extensive testing. >>>>>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>>>>> situation. >>>>>>>>> >>>>>>>>> Understood. Should we revert patch 6/6 for now? >>>>>>>> >>>>>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>>>>> further concern when I saw the widespread nature of the overall changes, >>>>>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>>>>> picked up in time. Please do speak up if you want me to revert the last >>>>>>>> part. >>>>>>> >>>>>>> Also it is often true that people find problems by testing on master >>>>>>> so applying it helps to shake the tree a bit. >>>>>>> >>>>>>> Regards, >>>>>>> Simon >>>>>> >>>>>> We don't actually have a problem with this series but with a previous >>>>>> watchdog patch. The culprit according to bisecting is: >>>>>> >>>>>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>>>>> >>>>>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>>>>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>>>>> drivers/watchdog/sunxi_wdt.c. >>>>>> >>>>>> If I run >>>>>> => wdt dev watchdog@1c20ca0 >>>>>> => wdt stop >>>>>> >>>>>> before the bootefi command booting succeeds. >>>>>> >>>>>> We don't disarm the watchdog and Linux does not do it for us in time. >>>>>> >>>>>> The UEFI specification requires that the default watchdog reset time is 300 >>>>>> s. We should never arm the Sunxi hardware watchdog except within the >>>>>> watchdog reset driver. >>>>>> >>>>>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>>>>> >>>>>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>>>>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >>>>> >>>>> This means we never did come up with a satisfactory to everyone solution >>>>> to what UEFI thinks a watchdog should do, and what other types of >>>>> deployment think a watchdog should do, yes? >>>> >>>> Dear Tom, >>>> >>>> The issue is *not* UEFI specific. >>>> >>>> A watchdog timeout of 16 seconds is too short for Linux to boot no matter >>>> whether you use the EFI stub or the legacy entry point. >>>> >>>> I only referred to the UEFI specification as it indicates what can be >>>> considered as a reasonable timeout interval: 300 seconds. >>> >>> 16 seconds from the last time we pet the watchdog in U-Boot to the >>> kernel being able to take over is quite reasonable. >> >> How do we know that the kernel takes over? What if the kernel/EFI >> payload doesn't have a watchdog driver? I was assuming that the >> watchdog would be disabled as soon as we boot a kernel or an EFI app >> calls ExitBootServices (maybe even earlier). >> But this sounds like a generic problem, not sunxi specific. So how do >> other platforms solve this? >> >> Cheers, >> Andre > > The UEFI specification has this requirement in chapter "3.1.2 Load Option > Processing": > > "... the boot manager must enable the watchdog timer for 5 minutes by using > the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling > EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot > manager, the boot manager must disable the watchdog timer with an additional > call to the SetWatchdogTimer() boot service." > > This means that having an armed watchdog when starting the kernel is > correct. > > If you start a watchdog in the firmware which is not disabled or reset by > the operating system, you are out of luck and won't be able to boot. > > Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to > "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by > CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It > originally had compatible "allwinner,sun4i-wdt" only. > > Debian Bullseye has the driver enabled as a module. In the bootlog of the > Orange Pi PC I find: > [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, > nowayout=0) > This message appears approximately *20 seconds* after the EFI stub hands > over to the main kernel. Adding the driver to initrd shortens this to *18 > seconds*. The message occurs after file system checks which can be a lengthy > operation. In Debian systemd manages the watchdog. > > As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I have no problem with people switching on the SUNXI hardware watchdog for their specific embedded solution. But here it was switched on for all SUNXI boards and breaks booting into Linux distributions.
If the Linux distribution resets the watchdog *after* file system checks because it is managed by systemd (as is true for Debian), 5 minutes is realistic.
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
The problem is not UEFI related.
Oh dear, that is definitely quite slow.
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Yes, Sunxi is limited to 16 seconds.
I suppose it isn't possible to disable the watchdog once it is enabled?
You can, it's just not a general solution because, well, other platforms with a more reasonable overall period may still violate the UEFI spec but not get tripped up. And globally disabling the watchdog before "bootefi" or similar is also not a great idea. It probably is, sadly, the best option to just disable by default (so not enable until the kernel does) watchdog on sunxi.
On Mon, 8 Nov 2021 17:17:33 +0100 Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/8/21 17:05, Tom Rini wrote:
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote:
On 11/6/21 02:52, Andre Przywara wrote: > On Fri, 5 Nov 2021 18:56:34 -0400 > Tom Rini trini@konsulko.com wrote: > >> On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: >>> On 11/5/21 20:17, Tom Rini wrote: >>>> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>>>> On 11/5/21 17:12, Simon Glass wrote: >>>>>> Hi, >>>>>> >>>>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>>>> >>>>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>>>> Hi Andre, >>>>>>>> >>>>>>>> Added Tom to Cc. >>>>>>>> >>>>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Andre, >>>>>>>>>>> >>>>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Stefan, >>>>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>>>> other platforms. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>>>> 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 >>>>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>>>> >>>>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>>>> >>>>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>>>> >>>>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>>>> using my "marvell" one for this. >>>>>>> >>>>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>>>> maybe we should just roll out a few more repositories, I think it's >>>>>>> easier to do that now than pre-gitlab? >>>>>>>>>>>> and why did this end up already in master? >>>>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>>>> the sunxi parts yet. >>>>>>>>>>> >>>>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>>>> fix any resulting problems in this release cycle. >>>>>>>>> >>>>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>>>> general fix. >>>>>>>> >>>>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>>>> before the merge-window and skipped the review process (most likely >>>>>>>> because of lack of time) are often still integrated in the early rcX >>>>>>>> cycles. At least this is how I handle it usually. >>>>>>>> >>>>>>>> Tom, is my understanding here correct? >>>>>>> >>>>>>> Yes. We are not as strict as the kernel is about what can come in >>>>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>>>> up to the discretion of the custodians. People tend of have less time >>>>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>>>> picking things up. >>>>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>>>> >>>>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>>>> but the sunxi integration is somewhat risky. >>>>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>>>> of view. >>>>>>>>>>> Do you see any specific issues? >>>>>>>>> >>>>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>>>> deserves at least some testing, *before* merging it. >>>>>>>> >>>>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>>>> got more extensive testing. >>>>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>>>> situation. >>>>>>>> >>>>>>>> Understood. Should we revert patch 6/6 for now? >>>>>>> >>>>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>>>> further concern when I saw the widespread nature of the overall changes, >>>>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>>>> picked up in time. Please do speak up if you want me to revert the last >>>>>>> part. >>>>>> >>>>>> Also it is often true that people find problems by testing on master >>>>>> so applying it helps to shake the tree a bit. >>>>>> >>>>>> Regards, >>>>>> Simon >>>>> >>>>> We don't actually have a problem with this series but with a previous >>>>> watchdog patch. The culprit according to bisecting is: >>>>> >>>>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>>>> >>>>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>>>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>>>> drivers/watchdog/sunxi_wdt.c. >>>>> >>>>> If I run >>>>> => wdt dev watchdog@1c20ca0 >>>>> => wdt stop >>>>> >>>>> before the bootefi command booting succeeds. >>>>> >>>>> We don't disarm the watchdog and Linux does not do it for us in time. >>>>> >>>>> The UEFI specification requires that the default watchdog reset time is 300 >>>>> s. We should never arm the Sunxi hardware watchdog except within the >>>>> watchdog reset driver. >>>>> >>>>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>>>> >>>>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>>>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >>>> >>>> This means we never did come up with a satisfactory to everyone solution >>>> to what UEFI thinks a watchdog should do, and what other types of >>>> deployment think a watchdog should do, yes? >>> >>> Dear Tom, >>> >>> The issue is *not* UEFI specific. >>> >>> A watchdog timeout of 16 seconds is too short for Linux to boot no matter >>> whether you use the EFI stub or the legacy entry point. >>> >>> I only referred to the UEFI specification as it indicates what can be >>> considered as a reasonable timeout interval: 300 seconds. >> >> 16 seconds from the last time we pet the watchdog in U-Boot to the >> kernel being able to take over is quite reasonable. > > How do we know that the kernel takes over? What if the kernel/EFI > payload doesn't have a watchdog driver? I was assuming that the > watchdog would be disabled as soon as we boot a kernel or an EFI app > calls ExitBootServices (maybe even earlier). > But this sounds like a generic problem, not sunxi specific. So how do > other platforms solve this? > > Cheers, > Andre
The UEFI specification has this requirement in chapter "3.1.2 Load Option Processing":
"... the boot manager must enable the watchdog timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot manager, the boot manager must disable the watchdog timer with an additional call to the SetWatchdogTimer() boot service."
This means that having an armed watchdog when starting the kernel is correct.
If you start a watchdog in the firmware which is not disabled or reset by the operating system, you are out of luck and won't be able to boot.
Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It originally had compatible "allwinner,sun4i-wdt" only.
Debian Bullseye has the driver enabled as a module. In the bootlog of the Orange Pi PC I find: [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0) This message appears approximately *20 seconds* after the EFI stub hands over to the main kernel. Adding the driver to initrd shortens this to *18 seconds*. The message occurs after file system checks which can be a lengthy operation. In Debian systemd manages the watchdog.
As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I have no problem with people switching on the SUNXI hardware watchdog for their specific embedded solution. But here it was switched on for all SUNXI boards and breaks booting into Linux distributions.
I agree here, and will take Heinrich's patch to keep it disabled on sunxi, avoiding the regression.
If the Linux distribution resets the watchdog *after* file system checks because it is managed by systemd (as is true for Debian), 5 minutes is realistic.
I am still puzzled about this, if I read the UEFI spec correctly, the 5 minutes watchdog timer is for EFI applications using boot services? So grub, for instance. But the description of ExitBootServices tells me that the: "boot services watchdog timer is disabled"? So it should not affect Linux booting (after the EFI stub is done)?
Cheers, Andre
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
The problem is not UEFI related.
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Yes, Sunxi is limited to 16 seconds.
Best regards
Heinrich
On 11/9/21 02:37, Andre Przywara wrote:
On Mon, 8 Nov 2021 17:17:33 +0100 Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/8/21 17:05, Tom Rini wrote:
On Mon, Nov 08, 2021 at 08:58:33AM -0700, Simon Glass wrote:
Hi,
On Sun, 7 Nov 2021 at 04:18, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/6/21 14:53, Tom Rini wrote:
On Sat, Nov 06, 2021 at 04:55:44AM +0100, Heinrich Schuchardt wrote: > > > On 11/6/21 02:52, Andre Przywara wrote: >> On Fri, 5 Nov 2021 18:56:34 -0400 >> Tom Rini trini@konsulko.com wrote: >> >>> On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote: >>>> On 11/5/21 20:17, Tom Rini wrote: >>>>> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote: >>>>>> On 11/5/21 17:12, Simon Glass wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: >>>>>>>> >>>>>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: >>>>>>>>> Hi Andre, >>>>>>>>> >>>>>>>>> Added Tom to Cc. >>>>>>>>> >>>>>>>>> On 05.11.21 11:04, Andre Przywara wrote: >>>>>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600 >>>>>>>>>> Simon Glass sjg@chromium.org wrote: >>>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Andre, >>>>>>>>>>>> >>>>>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote: >>>>>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100 >>>>>>>>>>>>> Stefan Roese sr@denx.de wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Stefan, >>>>>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote: >>>>>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register >>>>>>>>>>>>>>> watchdog devices 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. However, I put the code behind a new option to avoid surprises for >>>>>>>>>>>>>>> other platforms. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>>>> - Move condition to wdt-uclass.c to fix build errors. >>>>>>>>>>>>>>> - Include watchdog name in error message. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>>>> - Extend the "if SYSRESET" block to the end of the file. >>>>>>>>>>>>>>> - Also make gpio_reboot_probe function static. >>>>>>>>>>>>>>> - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). >>>>>>>>>>>>>>> - Added patches 5-6 as an example of how the new option will be used. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Samuel Holland (6): >>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>> sunxi: Avoid duplicate reset_cpu with SYSRESET enabled >>>>>>>>>>>>>>> sunxi: Use sysreset framework for poweroff/reset >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> arch/arm/Kconfig | 3 +++ >>>>>>>>>>>>>>> arch/arm/mach-sunxi/board.c | 2 ++ >>>>>>>>>>>>>>> drivers/sysreset/Kconfig | 11 ++++++-- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_gpio.c | 2 +- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_resetctl.c | 2 +- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_syscon.c | 2 +- >>>>>>>>>>>>>>> drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ >>>>>>>>>>>>>>> drivers/watchdog/wdt-uclass.c | 8 ++++++ >>>>>>>>>>>>>>> include/sysreset.h | 10 +++++++ >>>>>>>>>>>>>>> 9 files changed, 67 insertions(+), 13 deletions(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> Applied to u-boot-marvell >>>>>>>>>>>>> >>>>>>>>>>>>> Mmmh, why u-boot-marvell, >>>>>>>>>>>> >>>>>>>>>>>> Because I'm handling watchdog related changed since a few years and we >>>>>>>>>>>> did not create a specific subsystem repo for this and I'm usually >>>>>>>>>>>> using my "marvell" one for this. >>>>>>>> >>>>>>>> And fwiw, there's a few other cases like this. If it's too confusing, >>>>>>>> maybe we should just roll out a few more repositories, I think it's >>>>>>>> easier to do that now than pre-gitlab? >>>>>>>>>>>>> and why did this end up already in master? >>>>>>>>>>>>> Isn't that material for the next merge window? After all this changes >>>>>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at >>>>>>>>>>>>> the sunxi parts yet. >>>>>>>>>>>> >>>>>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in >>>>>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for >>>>>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to >>>>>>>>>>>> fix any resulting problems in this release cycle. >>>>>>>>>> >>>>>>>>>> Why do we have the merge window then? This is clearly not a regression or >>>>>>>>>> general fix. >>>>>>>>> >>>>>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted >>>>>>>>> before the merge-window and skipped the review process (most likely >>>>>>>>> because of lack of time) are often still integrated in the early rcX >>>>>>>>> cycles. At least this is how I handle it usually. >>>>>>>>> >>>>>>>>> Tom, is my understanding here correct? >>>>>>>> >>>>>>>> Yes. We are not as strict as the kernel is about what can come in >>>>>>>> between rc1 and rc2 (and to a certain degree, post rc2). I leave things >>>>>>>> up to the discretion of the custodians. People tend of have less time >>>>>>>> to handle U-Boot changes than other stuff, so I try and be flexible in >>>>>>>> picking things up. >>>>>>>>>>> Yes I agree, that should be plenty of time for people to review it. >>>>>>>>>> >>>>>>>>>> Well, if there would be people to review the sunxi parts :-( >>>>>>>>>> I am totally fine with the generic patches (as they have been reviewed), >>>>>>>>>> but the sunxi integration is somewhat risky. >>>>>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't >>>>>>>>>> change, add or fix anything, it's mere refactoring, from the user's point >>>>>>>>>> of view. >>>>>>>>>>>> Do you see any specific issues? >>>>>>>>>> >>>>>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that >>>>>>>>>> deserves at least some testing, *before* merging it. >>>>>>>>> >>>>>>>>> I expect that Samuel did some testing. But still, I agree that it >>>>>>>>> would be much better, if these patches - especially the Allwinner parts >>>>>>>>> got more extensive testing. >>>>>>>>>> I will do as much testing now as possible, but I am not happy about that >>>>>>>>>> situation. >>>>>>>>> >>>>>>>>> Understood. Should we revert patch 6/6 for now? >>>>>>>> >>>>>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had >>>>>>>> also assumed it was sufficiently tested, which is why I didn't raise a >>>>>>>> further concern when I saw the widespread nature of the overall changes, >>>>>>>> just figured it was a few more ready-to-go cleanups that weren't quite >>>>>>>> picked up in time. Please do speak up if you want me to revert the last >>>>>>>> part. >>>>>>> >>>>>>> Also it is often true that people find problems by testing on master >>>>>>> so applying it helps to shake the tree a bit. >>>>>>> >>>>>>> Regards, >>>>>>> Simon >>>>>> >>>>>> We don't actually have a problem with this series but with a previous >>>>>> watchdog patch. The culprit according to bisecting is: >>>>>> >>>>>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default") >>>>>> >>>>>> When booting the OrangePi PC the watchdog triggers while Linux is booting, >>>>>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in >>>>>> drivers/watchdog/sunxi_wdt.c. >>>>>> >>>>>> If I run >>>>>> => wdt dev watchdog@1c20ca0 >>>>>> => wdt stop >>>>>> >>>>>> before the bootefi command booting succeeds. >>>>>> >>>>>> We don't disarm the watchdog and Linux does not do it for us in time. >>>>>> >>>>>> The UEFI specification requires that the default watchdog reset time is 300 >>>>>> s. We should never arm the Sunxi hardware watchdog except within the >>>>>> watchdog reset driver. >>>>>> >>>>>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See >>>>>> >>>>>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards >>>>>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html >>>>> >>>>> This means we never did come up with a satisfactory to everyone solution >>>>> to what UEFI thinks a watchdog should do, and what other types of >>>>> deployment think a watchdog should do, yes? >>>> >>>> Dear Tom, >>>> >>>> The issue is *not* UEFI specific. >>>> >>>> A watchdog timeout of 16 seconds is too short for Linux to boot no matter >>>> whether you use the EFI stub or the legacy entry point. >>>> >>>> I only referred to the UEFI specification as it indicates what can be >>>> considered as a reasonable timeout interval: 300 seconds. >>> >>> 16 seconds from the last time we pet the watchdog in U-Boot to the >>> kernel being able to take over is quite reasonable. >> >> How do we know that the kernel takes over? What if the kernel/EFI >> payload doesn't have a watchdog driver? I was assuming that the >> watchdog would be disabled as soon as we boot a kernel or an EFI app >> calls ExitBootServices (maybe even earlier). >> But this sounds like a generic problem, not sunxi specific. So how do >> other platforms solve this? >> >> Cheers, >> Andre > > The UEFI specification has this requirement in chapter "3.1.2 Load Option > Processing": > > "... the boot manager must enable the watchdog timer for 5 minutes by using > the EFI_BOOT_SERVICES.SetWatchdogTimer() boot service prior to calling > EFI_BOOT_SERVICES.StartImage(). If a boot option returns control to the boot > manager, the boot manager must disable the watchdog timer with an additional > call to the SetWatchdogTimer() boot service." > > This means that having an armed watchdog when starting the kernel is > correct. > > If you start a watchdog in the firmware which is not disabled or reset by > the operating system, you are out of luck and won't be able to boot. > > Current Linux has driver drivers/watchdog/sunxi_wdt.c compatible to > "allwinner,sun4i-a10-wdt","allwinner,sun6i-a31-wdt" and enabled by > CONFIG_SUNXI_WATCHDOG. This driver was introduced in Linux v3.12. It > originally had compatible "allwinner,sun4i-wdt" only. > > Debian Bullseye has the driver enabled as a module. In the bootlog of the > Orange Pi PC I find: > [ 12.321909] sunxi-wdt 1c20ca0.watchdog: Watchdog enabled (timeout=16 sec, > nowayout=0) > This message appears approximately *20 seconds* after the EFI stub hands > over to the main kernel. Adding the driver to initrd shortens this to *18 > seconds*. The message occurs after file system checks which can be a lengthy > operation. In Debian systemd manages the watchdog. > > As I said: 16 seconds is way too short for a hardware watchdog timeout.
What's the time if you build it in?
For sure you will find some board and configuration that is faster.
But why should I care? This series breaks booting Debian on my board. So it needs to be fixed. So, please, apply my patch that is doing so.
Five minutes sounds completely unacceptable for embedded platforms. The user will surely have packaged the item up and will be just heading out to drop it off for return...
I have no problem with people switching on the SUNXI hardware watchdog for their specific embedded solution. But here it was switched on for all SUNXI boards and breaks booting into Linux distributions.
I agree here, and will take Heinrich's patch to keep it disabled on sunxi, avoiding the regression.
If the Linux distribution resets the watchdog *after* file system checks because it is managed by systemd (as is true for Debian), 5 minutes is realistic.
I am still puzzled about this, if I read the UEFI spec correctly, the 5 minutes watchdog timer is for EFI applications using boot services? So grub, for instance. But the description of ExitBootServices tells me that the: "boot services watchdog timer is disabled"? So it should not affect Linux booting (after the EFI stub is done)?
Currently we only disable the software watchdog (efi_tpl = TPL_HIGH_LEVEL;) We should call wdt_stop_all() too. I will create a patch for that.
Best regards
Heinrich
Cheers, Andre
I'm trying to avoid bringing up the long discussion from the previous thread about this :)
Do we need to add a special case for UEFI here? E.g. bootefi could use a hook to lengthen the watchdog?
The problem is not UEFI related.
Well, the problem is that the hardware watchdog has a maximum period of 16 seconds, I believe.
Yes, Sunxi is limited to 16 seconds.
Best regards
Heinrich
On Tue, Nov 09, 2021 at 09:00:05AM +0100, Heinrich Schuchardt wrote:
[snip]
I am still puzzled about this, if I read the UEFI spec correctly, the 5 minutes watchdog timer is for EFI applications using boot services? So grub, for instance. But the description of ExitBootServices tells me that the: "boot services watchdog timer is disabled"? So it should not affect Linux booting (after the EFI stub is done)?
Currently we only disable the software watchdog (efi_tpl = TPL_HIGH_LEVEL;) We should call wdt_stop_all() too. I will create a patch for that.
Lets use this as a chance to bring up the issue with the relevant part of the UEFI forum. Turning off a running watchdog is a bad idea in places where Arm is pushing SystemReady IR (and I would argue other specs as well, but..).
On Tue, 9 Nov 2021 08:50:37 -0500 Tom Rini trini@konsulko.com wrote:
Hi,
On Tue, Nov 09, 2021 at 09:00:05AM +0100, Heinrich Schuchardt wrote:
[snip]
thanks for that ;-)
I am still puzzled about this, if I read the UEFI spec correctly, the 5 minutes watchdog timer is for EFI applications using boot services? So grub, for instance. But the description of ExitBootServices tells me that the: "boot services watchdog timer is disabled"? So it should not affect Linux booting (after the EFI stub is done)?
Currently we only disable the software watchdog (efi_tpl = TPL_HIGH_LEVEL;) We should call wdt_stop_all() too. I will create a patch for that.
Lets use this as a chance to bring up the issue with the relevant part of the UEFI forum. Turning off a running watchdog is a bad idea in places where Arm is pushing SystemReady IR (and I would argue other specs as well, but..).
I think architecturally you have no other chance than turning it off at boot. You do not know what your payload is (Linux? BSD? Xen? homebrew kernel?), which watchdog it is using, or if it's using one at all (no driver). Also for instance sunxi has typically two watchdogs, which one is it that needs petting? And even an opt-in from the EFI application (the kernel's EFI stub) sounds hard, as the Linux EFI stub for instance has no insight into the watchdog configuration, so can't say whether we have a driver or whether that would work (because of a missing firmware table).
But it indeed sounds like a rather generic problem, and there might indeed be a solution generic enough for UEFI.
Do you have anything in mind?
Cheers, Andre
On 11/9/21 15:26, Andre Przywara wrote:
On Tue, 9 Nov 2021 08:50:37 -0500 Tom Rini trini@konsulko.com wrote:
Hi,
On Tue, Nov 09, 2021 at 09:00:05AM +0100, Heinrich Schuchardt wrote:
[snip]
thanks for that ;-)
I am still puzzled about this, if I read the UEFI spec correctly, the 5 minutes watchdog timer is for EFI applications using boot services? So grub, for instance. But the description of ExitBootServices tells me that the: "boot services watchdog timer is disabled"? So it should not affect Linux booting (after the EFI stub is done)?
Currently we only disable the software watchdog (efi_tpl = TPL_HIGH_LEVEL;) We should call wdt_stop_all() too. I will create a patch for that.
Lets use this as a chance to bring up the issue with the relevant part of the UEFI forum. Turning off a running watchdog is a bad idea in places where Arm is pushing SystemReady IR (and I would argue other specs as well, but..).
I think architecturally you have no other chance than turning it off at boot. You do not know what your payload is (Linux? BSD? Xen? homebrew kernel?), which watchdog it is using, or if it's using one at all (no driver). Also for instance sunxi has typically two watchdogs, which one is it that needs petting? And even an opt-in from the EFI application (the kernel's EFI stub) sounds hard, as the Linux EFI stub for instance has no insight into the watchdog configuration, so can't say whether we have a driver or whether that would work (because of a missing firmware table).
But it indeed sounds like a rather generic problem, and there might indeed be a solution generic enough for UEFI.
Do you have anything in mind?
Cheers, Andre
Hello Grant, hello Ozog,
according to the UEFI spec the watchdog should be shut down in ExitBootServices(). In an IoT scenario this may not always make sense. E.g. if A/B boot fails you want to reset the board to its previous state.
Is this something to discuss in the EBBR context? Is there any requirement in SystemReady ES?
Best regards
Heinrich
On 09/11/2021 14:34, Heinrich Schuchardt wrote:
On 11/9/21 15:26, Andre Przywara wrote:
On Tue, 9 Nov 2021 08:50:37 -0500 Tom Rini trini@konsulko.com wrote:
Hi,
On Tue, Nov 09, 2021 at 09:00:05AM +0100, Heinrich Schuchardt wrote:
[snip]
thanks for that ;-)
I am still puzzled about this, if I read the UEFI spec correctly, the 5 minutes watchdog timer is for EFI applications using boot services? So grub, for instance. But the description of ExitBootServices tells me that the: "boot services watchdog timer is disabled"? So it should not affect Linux booting (after the EFI stub is done)?
Currently we only disable the software watchdog (efi_tpl = TPL_HIGH_LEVEL;) We should call wdt_stop_all() too. I will create a patch for that.
Lets use this as a chance to bring up the issue with the relevant part of the UEFI forum. Turning off a running watchdog is a bad idea in places where Arm is pushing SystemReady IR (and I would argue other specs as well, but..).
I think architecturally you have no other chance than turning it off at boot. You do not know what your payload is (Linux? BSD? Xen? homebrew kernel?), which watchdog it is using, or if it's using one at all (no driver). Also for instance sunxi has typically two watchdogs, which one is it that needs petting? And even an opt-in from the EFI application (the kernel's EFI stub) sounds hard, as the Linux EFI stub for instance has no insight into the watchdog configuration, so can't say whether we have a driver or whether that would work (because of a missing firmware table).
But it indeed sounds like a rather generic problem, and there might indeed be a solution generic enough for UEFI.
Do you have anything in mind?
Cheers, Andre
Hello Grant, hello Ozog,
according to the UEFI spec the watchdog should be shut down in ExitBootServices(). In an IoT scenario this may not always make sense. E.g. if A/B boot fails you want to reset the board to its previous state.
Is this something to discuss in the EBBR context? Is there any requirement in SystemReady ES?
There is no requirement in ES as far as I'm aware. Yes, it makes sense to discuss this w.r.t the EBBR spec. I agree that the watchdog should not be turned off at EBS() time for most IR class platforms.
g.
Best regards
Heinrich
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Sat, Nov 06, 2021 at 01:52:30AM +0000, Andre Przywara wrote:
On Fri, 5 Nov 2021 18:56:34 -0400 Tom Rini trini@konsulko.com wrote:
On Fri, Nov 05, 2021 at 09:38:50PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 20:17, Tom Rini wrote:
On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
On 11/5/21 17:12, Simon Glass wrote:
Hi,
On Fri, 5 Nov 2021 at 08:21, Tom Rini trini@konsulko.com wrote: > > On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote: > > Hi Andre, > > > > Added Tom to Cc. > > > > On 05.11.21 11:04, Andre Przywara wrote: > > > On Thu, 4 Nov 2021 20:02:41 -0600 > > > Simon Glass sjg@chromium.org wrote: > > > > > > Hi, > > > > > > > On Thu, 4 Nov 2021 at 19:22, Stefan Roese sr@denx.de wrote: > > > > > > > > > > Hi Andre, > > > > > > > > > > On 05.11.21 00:11, Andre Przywara wrote: > > > > > > On Thu, 4 Nov 2021 11:37:57 +0100 > > > > > > Stefan Roese sr@denx.de wrote: > > > > > > > > > > > > Hi Stefan, > > > > > > > On 04.11.21 04:55, Samuel Holland wrote: > > > > > > > > This series hooks up the watchdog uclass to automatically register > > > > > > > > watchdog devices 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. However, I put the code behind a new option to avoid surprises for > > > > > > > > other platforms. > > > > > > > > > > > > > > > > Changes in v3: > > > > > > > > - Move condition to wdt-uclass.c to fix build errors. > > > > > > > > - Include watchdog name in error message. > > > > > > > > > > > > > > > > Changes in v2: > > > > > > > > - Extend the "if SYSRESET" block to the end of the file. > > > > > > > > - Also make gpio_reboot_probe function static. > > > > > > > > - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs). > > > > > > > > - Added patches 5-6 as an example of how the new option will be used. > > > > > > > > > > > > > > > > Samuel Holland (6): > > > > > > > > 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 > > > > > > > > sunxi: Avoid duplicate reset_cpu with SYSRESET enabled > > > > > > > > sunxi: Use sysreset framework for poweroff/reset > > > > > > > > > > > > > > > > arch/arm/Kconfig | 3 +++ > > > > > > > > arch/arm/mach-sunxi/board.c | 2 ++ > > > > > > > > drivers/sysreset/Kconfig | 11 ++++++-- > > > > > > > > drivers/sysreset/sysreset_gpio.c | 2 +- > > > > > > > > drivers/sysreset/sysreset_resetctl.c | 2 +- > > > > > > > > drivers/sysreset/sysreset_syscon.c | 2 +- > > > > > > > > drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------ > > > > > > > > drivers/watchdog/wdt-uclass.c | 8 ++++++ > > > > > > > > include/sysreset.h | 10 +++++++ > > > > > > > > 9 files changed, 67 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > Applied to u-boot-marvell > > > > > > > > > > > > Mmmh, why u-boot-marvell, > > > > > > > > > > Because I'm handling watchdog related changed since a few years and we > > > > > did not create a specific subsystem repo for this and I'm usually > > > > > using my "marvell" one for this. > > And fwiw, there's a few other cases like this. If it's too confusing, > maybe we should just roll out a few more repositories, I think it's > easier to do that now than pre-gitlab? > > > > > > > and why did this end up already in master? > > > > > > Isn't that material for the next merge window? After all this changes > > > > > > quite a bit, for a lot of boards, and I did not have a closer look at > > > > > > the sunxi parts yet. > > > > > > > > > > I was hesitating also a bit. But since this patchset is on the list in > > > > > v1 since over 2 months now (2021-08-21) I thought it was "ready" for > > > > > inclusion now. We are at -rc1 and I think we still have enough time to > > > > > fix any resulting problems in this release cycle. > > > > > > Why do we have the merge window then? This is clearly not a regression or > > > general fix. > > > > AFAIU, we are a bit less strict here in U-Boot. Patches that were posted > > before the merge-window and skipped the review process (most likely > > because of lack of time) are often still integrated in the early rcX > > cycles. At least this is how I handle it usually. > > > > Tom, is my understanding here correct? > > Yes. We are not as strict as the kernel is about what can come in > between rc1 and rc2 (and to a certain degree, post rc2). I leave things > up to the discretion of the custodians. People tend of have less time > to handle U-Boot changes than other stuff, so I try and be flexible in > picking things up. > > > > > Yes I agree, that should be plenty of time for people to review it. > > > > > > Well, if there would be people to review the sunxi parts :-( > > > I am totally fine with the generic patches (as they have been reviewed), > > > but the sunxi integration is somewhat risky. > > > I was explicitly deprioritising that in my queue, as it really doesn't > > > change, add or fix anything, it's mere refactoring, from the user's point > > > of view. > > > > > > > > Do you see any specific issues? > > > > > > Patch 6/6 changes the config for all 157 Allwinner boards, so I think that > > > deserves at least some testing, *before* merging it. > > > > I expect that Samuel did some testing. But still, I agree that it > > would be much better, if these patches - especially the Allwinner parts > > got more extensive testing. > > > > > I will do as much testing now as possible, but I am not happy about that > > > situation. > > > > Understood. Should we revert patch 6/6 for now? > > FWIW, given Samuel has been doing a number of allwinner changes, I had > also assumed it was sufficiently tested, which is why I didn't raise a > further concern when I saw the widespread nature of the overall changes, > just figured it was a few more ready-to-go cleanups that weren't quite > picked up in time. Please do speak up if you want me to revert the last > part.
Also it is often true that people find problems by testing on master so applying it helps to shake the tree a bit.
Regards, Simon
We don't actually have a problem with this series but with a previous watchdog patch. The culprit according to bisecting is:
b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
When booting the OrangePi PC the watchdog triggers while Linux is booting, ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in drivers/watchdog/sunxi_wdt.c.
If I run
=> wdt dev watchdog@1c20ca0 => wdt stop
before the bootefi command booting succeeds.
We don't disarm the watchdog and Linux does not do it for us in time.
The UEFI specification requires that the default watchdog reset time is 300 s. We should never arm the Sunxi hardware watchdog except within the watchdog reset driver.
The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
[PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
This means we never did come up with a satisfactory to everyone solution to what UEFI thinks a watchdog should do, and what other types of deployment think a watchdog should do, yes?
Dear Tom,
The issue is *not* UEFI specific.
A watchdog timeout of 16 seconds is too short for Linux to boot no matter whether you use the EFI stub or the legacy entry point.
I only referred to the UEFI specification as it indicates what can be considered as a reasonable timeout interval: 300 seconds.
16 seconds from the last time we pet the watchdog in U-Boot to the kernel being able to take over is quite reasonable.
How do we know that the kernel takes over? What if the kernel/EFI payload doesn't have a watchdog driver? I was assuming that the watchdog would be disabled as soon as we boot a kernel or an EFI app calls ExitBootServices (maybe even earlier). But this sounds like a generic problem, not sunxi specific. So how do other platforms solve this?
It depends on the deployment situation. Heinrich has quoted absolutely correctly what the UEFI says and expects. Other deployment situations are going to demand that the watchdog reset the system if it can't be "petted" every few seconds and disabling the watchdog because we're now booting the kernel would be entirely unacceptable. How exactly other platforms solve this is either: - The kernel will also enable the watchdog driver and there is sufficient time between U-Boot and Kernel for the handover. - The watchdog is disabled in general because there's not enough time or distributions don't enable the watchdog driver.
Getting on my soap box for a moment, it feels like we're seeing a shift from the former in arm32 being the default to the latter in arm64 being the default.
participants (7)
-
Andre Przywara -
Grant Likely -
Heinrich Schuchardt -
Samuel Holland -
Simon Glass -
Stefan Roese -
Tom Rini