[PATCH] watchdog: Fix watchdog enablement in SPL and TPL

[Stefan Roese]

On 02.09.21 09:30, Marcel Ziswiler wrote:
> On Tue, 2021-08-31 at 11:17 +0200, Marek Vasut wrote:
>> On 8/31/21 8:49 AM, Marcel Ziswiler wrote:
>>> Hi Marek
>>>
>>> On Tue, 2021-08-31 at 00:03 +0200, Marek Vasut wrote:
>>>> Commit 830d29ac372 ("watchdog: Allow to use CONFIG_WDT without starting watchdog")
>>>> completely broke WDT operation in both SPL and TPL, in either case those
>>>> WDTs are never enabled. Fix it by filling in the missing Kconfig options
>>>> for SPL and TPL.
>>>>
>>>> Fixes: 830d29ac372 ("watchdog: Allow to use CONFIG_WDT without starting watchdog")
>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>> Cc: Pali Rohar <pali at kernel.org>
>>>> Cc: Stefan Roese <sr at denx.de>
>>>> ---
>>>>    drivers/watchdog/Kconfig | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index f0ff2612a6b..65d974c4dd5 100644
>>>> --- a/drivers/watchdog/Kconfig
>>>> +++ b/drivers/watchdog/Kconfig
>>>> @@ -273,4 +273,24 @@ config SPL_WDT
>>>>             Enable driver model for watchdog timer in SPL.
>>>>             This is similar to CONFIG_WDT in U-Boot.
>>>>    
>>>> +config SPL_WATCHDOG_AUTOSTART
>>>> +       bool "Automatically start watchdog timer in SPL"
>>>> +       depends on SPL && WDT
>>>> +       default y
>>>> +       help
>>>> +         Automatically start watchdog timer and start servicing it during
>>>> +         SPL phase. Enabled by default. Disable this option if you want
>>>> +         to compile U-Boot with CONFIG_WDT support but do not want to
>>>> +         activate watchdog, like when CONFIG_WDT option is disabled.
>>>> +
>>>> +config TPL_WATCHDOG_AUTOSTART
>>>> +       bool "Automatically start watchdog timer in TPL"
>>>> +       depends on TPL && WDT
>>>> +       default y
>>>> +       help
>>>> +         Automatically start watchdog timer and start servicing it during
>>>> +         TPL phase. Enabled by default. Disable this option if you want
>>>> +         to compile U-Boot with CONFIG_WDT support but do not want to
>>>> +         activate watchdog, like when CONFIG_WDT option is disabled.
>>>> +
>>>>    endmenu
>>>
>>> Those Kconfig entries look fine. However, I am wondering where exactly they get used. Am I missing
>>> anything?
>>
>> Have a look at the patch this Fixes:, if those Kconfig entries are not
>> present, the WDT is disabled in SPL and TPL unconditionally.
> 
> Yes, I did. But I guess I was not aware of how exactly that
> CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART) behaves.
> While I have not really found this documented or the implementation
> thereof I assume it pre-fixes config stuff
> with SPL_ resp. TPL_ when building those flavors, correct?

Yes.

> I am wondering how many others are unaware of this (just like the
> author of the patch this fixes). Maybe we
> should at least document this properly somewhere, not?

If this is not the case (I did not check this), then yes. A
documentation for these CONFIG_IS_ENABLED() and IS_ENABLED()
helpers would be very good.

>> It could be that if you're using some non-free or proprietary preloader
>> instead of SPL, you won't run into this problem.
> 
> No, don't worry. I am not running anything evil (;-p).

You won't run into any issues with this problem, as it is fixed in
a different way in master already, as Rasmus already pointed out:

https://lore.kernel.org/u-boot/f015f55e-3225-655e-2e8b-672e058a8ae5@denx.de/

$ git describe --contains 5fc094351381c4254098a25404d8712324b6918e
v2021.10-rc1~19^2

commit 5fc094351381c4254098a25404d8712324b6918e
Author: Teresa Remmet <t.remmet at phytec.de>
Date:   Thu Jul 15 13:26:32 2021 +0200

     drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

     There is no separate SPL/TPL config for WATCHDOG_AUTOSTART.
     So use IS_ENABLED instead of CONFIG_IS_ENABLED to make watchdog
     working in SPL again.

     Fixes: 830d29ac3721 ("watchdog: Allow to use CONFIG_WDT without 
starting watchdog")
     Signed-off-by: Teresa Remmet <t.remmet at phytec.de>
     Reviewed-by: Stefan Roese <sr at denx.de>

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index a0c2429e5a43..17334dbda6c9 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -53,7 +53,7 @@ int initr_watchdog(void)
                                                     4 * reset_period) / 4;
         }

-       if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
+       if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
                 printf("WDT:   Not starting\n");
                 return 0;
         }