* [PATCH] watchdog: Fix watchdog enablement in SPL and TPL
@ 2021-08-30 22:03 Marek Vasut
2021-08-31 6:49 ` Marcel Ziswiler
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2021-08-30 22:03 UTC (permalink / raw)
To: u-boot; +Cc: Marek Vasut, Pali Rohar, Stefan Roese
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@denx.de>
Cc: Pali Rohar <pali@kernel.org>
Cc: Stefan Roese <sr@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
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] watchdog: Fix watchdog enablement in SPL and TPL
2021-08-30 22:03 [PATCH] watchdog: Fix watchdog enablement in SPL and TPL Marek Vasut
@ 2021-08-31 6:49 ` Marcel Ziswiler
2021-08-31 9:17 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Ziswiler @ 2021-08-31 6:49 UTC (permalink / raw)
To: marex, u-boot; +Cc: sr, pali
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@denx.de>
> Cc: Pali Rohar <pali@kernel.org>
> Cc: Stefan Roese <sr@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?
Cheers
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] watchdog: Fix watchdog enablement in SPL and TPL
2021-08-31 6:49 ` Marcel Ziswiler
@ 2021-08-31 9:17 ` Marek Vasut
2021-09-02 7:30 ` Marcel Ziswiler
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2021-08-31 9:17 UTC (permalink / raw)
To: Marcel Ziswiler, u-boot; +Cc: sr, pali
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@denx.de>
>> Cc: Pali Rohar <pali@kernel.org>
>> Cc: Stefan Roese <sr@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.
It could be that if you're using some non-free or proprietary preloader
instead of SPL, you won't run into this problem.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] watchdog: Fix watchdog enablement in SPL and TPL
2021-08-31 9:17 ` Marek Vasut
@ 2021-09-02 7:30 ` Marcel Ziswiler
2021-09-02 7:49 ` Stefan Roese
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Ziswiler @ 2021-09-02 7:30 UTC (permalink / raw)
To: marex, u-boot; +Cc: sr, pali
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@denx.de>
> > > Cc: Pali Rohar <pali@kernel.org>
> > > Cc: Stefan Roese <sr@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?
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?
> 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).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] watchdog: Fix watchdog enablement in SPL and TPL
2021-09-02 7:30 ` Marcel Ziswiler
@ 2021-09-02 7:49 ` Stefan Roese
2021-09-02 9:29 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2021-09-02 7:49 UTC (permalink / raw)
To: Marcel Ziswiler, marex, u-boot; +Cc: pali
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@denx.de>
>>>> Cc: Pali Rohar <pali@kernel.org>
>>>> Cc: Stefan Roese <sr@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@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@phytec.de>
Reviewed-by: Stefan Roese <sr@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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] watchdog: Fix watchdog enablement in SPL and TPL
2021-09-02 7:49 ` Stefan Roese
@ 2021-09-02 9:29 ` Marek Vasut
0 siblings, 0 replies; 6+ messages in thread
From: Marek Vasut @ 2021-09-02 9:29 UTC (permalink / raw)
To: Stefan Roese, Marcel Ziswiler, u-boot; +Cc: pali
On 9/2/21 9:49 AM, Stefan Roese wrote:
> 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@denx.de>
>>>>> Cc: Pali Rohar <pali@kernel.org>
>>>>> Cc: Stefan Roese <sr@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.
You can possibly even auto-lint for
CONFIG_IS_ENABLED(...)/IS_ENABLED(...) usage that is missing the Kconfig
entries and warn about that, to prevent patches that break SPL/TPL .
>>> 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@phytec.de>
> Date: Thu Jul 15 13:26:32 2021 +0200
Yes.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-02 9:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-30 22:03 [PATCH] watchdog: Fix watchdog enablement in SPL and TPL Marek Vasut
2021-08-31 6:49 ` Marcel Ziswiler
2021-08-31 9:17 ` Marek Vasut
2021-09-02 7:30 ` Marcel Ziswiler
2021-09-02 7:49 ` Stefan Roese
2021-09-02 9:29 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.