u-boot leaves watchdog enabled by default

[Tom Rini]

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

Can you code this up as a patch please?  I think that's likely the best
path forward to covering all cases.  Thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200922/3fccda2a/attachment.sig>