u-boot leaves watchdog enabled by default

[Michael Walle]

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 is 
> using
>           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

-michael