On Tue, Sep 22, 2020 at 05:41:47PM +0200, Michael Walle wrote:
Hi Tom,
Am 2020-09-22 16:41, schrieb Tom Rini:
On Tue, Sep 22, 2020 at 03:18:58PM +0200, Michael Walle wrote:
[..]
But all that said, since we have "wdt stop", perhaps you can find a place to put that in the boot script? Or just declare that if we get far enough to run preboot cmd then it's good enough, and update your call to "wdt expire 1" to be "wdt start && wdt expire 1" ?
"wdt expire 1" will automatically start the watchdog, won't it? Anyway, it was just an example why I need the CONFIG_WDT.
Right, OK. I'm just wondering if we can use the existing "wdt stop" functionality to cover what you're aiming for.
See below.
Just to get your opinion correct on this topic: you say as soon as the CONFIG_WDT is enabled u-boot will start it and never stop it, although CONFIG_WDT is just "Enable driver model watchdog timer drivers" and the help text is just about that, too.
I will agree that the help text and symbols can use further cleaning up still. CONFIG_WDT implies in CONFIG_WATCHDOG which says:
This option enables U-Boot watchdog support where U-Boot isusing watchdog_reset function to service watchdog device in U-Boot. Enable this option if you want to service enabled watchdog by U-Boot. Disable this option if you want U-Boot to start watchdog but never service it.
Which is what we've done (to the best of my knowledge) "forever".
Yes CONFIG_WATCHDOG is implied, but if you disable CONFIG_WATCHDOG it will still be started. Just not serviced.
IMHO it is wrong to enable the watchdog together with that option. There should be another one (even defaulting to 'yes') which tells u-boot whether it should be enabled by default.
config WDT_AUTOSTART boot "Start the (first) watchdog by default" default y help Upon u-boot startup the first watchdog will be started automatically. Be aware, that it will also kept enabled after the bootloader starts the operation system!
Now, given what I said above looking at commit 06985289d452 ("watchdog: Implement generic watchdog_reset() version") is where we get the current behavior, symbol-wise. At this point, I'm not quite sure how best to do what you're looking for, or if we just have a bug in terms of which symbols are used. It sounds like you just want to stop the watchdog in U-Boot (outside of a specific start-and-trigger case) and let the OS decide if it's going to enable it. And if the OS is going to enable it and you want the watchdog started before OS boot, preboot could be setf in the environment to "wdt start" to get it going again (and you enable CONFIG_USE_PREBOOT and set CONFIG_PREBOOT to an empty string, or wdt stop?). Or does that still not cover what you're trying to do?
There are two things I want(ed) to achieve: (1) While booting the d-i I noticed my board does watchdog resets. So I digged into this and noticed that since the commit in question, any watchdog will be started unconditionally, which looked wrong to me and is a bit of a suprising behaviour. Esp. when you inherit the device trees from linux where the SoC watchdogs are usually enabled. Also I looked into how you could disable this behavior per-board. I didn't find a reliable method that worked for any boot command. Therefore, I've started this discussion, to find out if this is the intended behavior.
(2) To be able to boot an operating system with the boards default environment, that isn't aware of any watchdog.
For my specific use-case there are the following solutions so far: (a) stop the watchdog via "wdt stop" sometime (b) disable CONFIG_WDT (c) don't start the watchdog at all (d) have a runtime switch in the environment to stop it before booting the OS.
(a) and (b) is currently possible, (c) and (d) would need a patch. (b) is out of question for me, because I need the u-boot wdt commands. (a) sounds like a hack to me, why would you stop it even if I don't want it to be started in the first place. So I'd prefer (c).
I see that someone might prefer (d) as it gives the user the choice without having to recompile u-boot.
But apart from my use case, I could think of others and IMHO we should leave the choice up to the board user (and making it as easy as possible to configure it). So what do about the following:
choice prompt "Watchdog behaviour" default WDT_SUPERVISE_OS
config WDT_SUPERVISE_NOTHING boot "Supervise nothing" help No watchdog will be started.
config WDT_SUPERVISE_U_BOOT boot "Supervise u-boot" help Upon u-boot startup the first watchdog will be started automatically and stopped as soon as an operating system is booted.
config WDT_SUPERVISE_OS boot "Supervise u-boot and operating system" help Upon u-boot startup the first watchdog will be started automatically and kept running even after booting the operating system. Be aware, that the operating system needs to service the watchdog!
endchoice
Can you code this up as a patch please? I think that's likely the best path forward to covering all cases. Thanks.