[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[Rasmus Villemoes]

Some boards don't work with the rate-limiting done in the generic
watchdog_reset() provided by wdt-uclass.

For example, on powerpc, get_timer() ceases working during bootm since
interrupts are disabled before the kernel image gets decompressed, and
when the decompression takes longer than the watchdog device
allows (or enough of the budget that the kernel doesn't get far enough
to assume responsibility for petting the watchdog), the result is a
non-booting board.

As a somewhat hacky workaround (because DT is supposed to describe
hardware), allow specifying hw_margin_ms=0 in device tree to
effectively disable the ratelimiting and actually ping the watchdog
every time watchdog_reset() is called. For that to work, the "has
enough time passed" check just needs to be tweaked a little to allow
the now==next_reset case as well.

Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---

It's the option I dislike the most (because of the DT abuse), but I
also do accept that it's the one with the minimal code impact, and
apparently the path of least resistance. So here it is.

 drivers/watchdog/wdt-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 0603ffbd36..2687135296 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -148,7 +148,7 @@ void watchdog_reset(void)
 
 	/* Do not reset the watchdog too often */
 	now = get_timer(0);
-	if (time_after(now, next_reset)) {
+	if (time_after_eq(now, next_reset)) {
 		next_reset = now + reset_period;
 		wdt_reset(gd->watchdog_dev);
 	}
-- 
2.29.2

[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[Stefan Roese]

On 13.04.21 16:43, Rasmus Villemoes wrote:
> Some boards don't work with the rate-limiting done in the generic
> watchdog_reset() provided by wdt-uclass.
> 
> For example, on powerpc, get_timer() ceases working during bootm since
> interrupts are disabled before the kernel image gets decompressed, and
> when the decompression takes longer than the watchdog device
> allows (or enough of the budget that the kernel doesn't get far enough
> to assume responsibility for petting the watchdog), the result is a
> non-booting board.
> 
> As a somewhat hacky workaround (because DT is supposed to describe
> hardware), allow specifying hw_margin_ms=0 in device tree to
> effectively disable the ratelimiting and actually ping the watchdog
> every time watchdog_reset() is called. For that to work, the "has
> enough time passed" check just needs to be tweaked a little to allow
> the now==next_reset case as well.
> 
> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> 
> It's the option I dislike the most (because of the DT abuse), but I
> also do accept that it's the one with the minimal code impact, and
> apparently the path of least resistance. So here it is.

Right. An alternative way would have been to add a new Kconfig symbol
to define the default value of "reset_period" so that it can be
configured to different values via Kconfig as well.

>   drivers/watchdog/wdt-uclass.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 0603ffbd36..2687135296 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -148,7 +148,7 @@ void watchdog_reset(void)
>   
>   	/* Do not reset the watchdog too often */
>   	now = get_timer(0);
> -	if (time_after(now, next_reset)) {
> +	if (time_after_eq(now, next_reset)) {
>   		next_reset = now + reset_period;
>   		wdt_reset(gd->watchdog_dev);
>   	}
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[Rasmus Villemoes]

On 15/04/2021 07.38, Stefan Roese wrote:
> On 13.04.21 16:43, Rasmus Villemoes wrote:
>> Some boards don't work with the rate-limiting done in the generic
>> watchdog_reset() provided by wdt-uclass.
>>
>> For example, on powerpc, get_timer() ceases working during bootm since
>> interrupts are disabled before the kernel image gets decompressed, and
>> when the decompression takes longer than the watchdog device
>> allows (or enough of the budget that the kernel doesn't get far enough
>> to assume responsibility for petting the watchdog), the result is a
>> non-booting board.
>>
>> As a somewhat hacky workaround (because DT is supposed to describe
>> hardware), allow specifying hw_margin_ms=0 in device tree to
>> effectively disable the ratelimiting and actually ping the watchdog
>> every time watchdog_reset() is called. For that to work, the "has
>> enough time passed" check just needs to be tweaked a little to allow
>> the now==next_reset case as well.
>>
>> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>
>> It's the option I dislike the most (because of the DT abuse), but I
>> also do accept that it's the one with the minimal code impact, and
>> apparently the path of least resistance. So here it is.
> 
> Right. An alternative way would have been to add a new Kconfig symbol
> to define the default value of "reset_period" so that it can be
> configured to different values via Kconfig as well.

No, I don't think we should not go in that direction.

Another thing I have on my todo-list is to rewrite the watchdog_reset()
in wdt-uclass to handle _all_ DM watchdogs, not just the first one. Some
boards make use of both the one in the CPU/SOC as well as some
gpio-triggered one. Both the hw_margin_ms/reset_period and the
last_reset data would then have to live with the device, not be static
variables. Last I looked, it does seem that the DM code supports having
a piece of class-owned, per-device data, so it shouldn't be too hard to do.

Rasmus

[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[Stefan Roese]

On 15.04.21 08:54, Rasmus Villemoes wrote:
> On 15/04/2021 07.38, Stefan Roese wrote:
>> On 13.04.21 16:43, Rasmus Villemoes wrote:
>>> Some boards don't work with the rate-limiting done in the generic
>>> watchdog_reset() provided by wdt-uclass.
>>>
>>> For example, on powerpc, get_timer() ceases working during bootm since
>>> interrupts are disabled before the kernel image gets decompressed, and
>>> when the decompression takes longer than the watchdog device
>>> allows (or enough of the budget that the kernel doesn't get far enough
>>> to assume responsibility for petting the watchdog), the result is a
>>> non-booting board.
>>>
>>> As a somewhat hacky workaround (because DT is supposed to describe
>>> hardware), allow specifying hw_margin_ms=0 in device tree to
>>> effectively disable the ratelimiting and actually ping the watchdog
>>> every time watchdog_reset() is called. For that to work, the "has
>>> enough time passed" check just needs to be tweaked a little to allow
>>> the now==next_reset case as well.
>>>
>>> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> ---
>>>
>>> It's the option I dislike the most (because of the DT abuse), but I
>>> also do accept that it's the one with the minimal code impact, and
>>> apparently the path of least resistance. So here it is.
>>
>> Right. An alternative way would have been to add a new Kconfig symbol
>> to define the default value of "reset_period" so that it can be
>> configured to different values via Kconfig as well.
> 
> No, I don't think we should not go in that direction.
> 
> Another thing I have on my todo-list is to rewrite the watchdog_reset()
> in wdt-uclass to handle _all_ DM watchdogs, not just the first one.

Great. This is a current flaw in the U-Boot WDT implementation.

> Some
> boards make use of both the one in the CPU/SOC as well as some
> gpio-triggered one. Both the hw_margin_ms/reset_period and the
> last_reset data would then have to live with the device, not be static
> variables. Last I looked, it does seem that the DM code supports having
> a piece of class-owned, per-device data, so it shouldn't be too hard to do.

Thanks for looking into this.

Thanks,
Stefan

[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[Christophe Leroy]

Le 15/04/2021 ? 08:54, Rasmus Villemoes a ?crit?:
> On 15/04/2021 07.38, Stefan Roese wrote:
>> On 13.04.21 16:43, Rasmus Villemoes wrote:
>>> Some boards don't work with the rate-limiting done in the generic
>>> watchdog_reset() provided by wdt-uclass.
>>>
>>> For example, on powerpc, get_timer() ceases working during bootm since
>>> interrupts are disabled before the kernel image gets decompressed, and
>>> when the decompression takes longer than the watchdog device
>>> allows (or enough of the budget that the kernel doesn't get far enough
>>> to assume responsibility for petting the watchdog), the result is a
>>> non-booting board.
>>>
>>> As a somewhat hacky workaround (because DT is supposed to describe
>>> hardware), allow specifying hw_margin_ms=0 in device tree to
>>> effectively disable the ratelimiting and actually ping the watchdog
>>> every time watchdog_reset() is called. For that to work, the "has
>>> enough time passed" check just needs to be tweaked a little to allow
>>> the now==next_reset case as well.
>>>
>>> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> ---
>>>
>>> It's the option I dislike the most (because of the DT abuse), but I
>>> also do accept that it's the one with the minimal code impact, and
>>> apparently the path of least resistance. So here it is.
>>
>> Right. An alternative way would have been to add a new Kconfig symbol
>> to define the default value of "reset_period" so that it can be
>> configured to different values via Kconfig as well.
> 
> No, I don't think we should not go in that direction.

Double negation ....

You mean: I think we should go ?

> 
> Another thing I have on my todo-list is to rewrite the watchdog_reset()
> in wdt-uclass to handle _all_ DM watchdogs, not just the first one. Some
> boards make use of both the one in the CPU/SOC as well as some
> gpio-triggered one. Both the hw_margin_ms/reset_period and the
> last_reset data would then have to live with the device, not be static
> variables. Last I looked, it does seem that the DM code supports having
> a piece of class-owned, per-device data, so it shouldn't be too hard to do.
> 
> Rasmus
> 

[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[Rasmus Villemoes]

On 15/04/2021 09.13, Christophe Leroy wrote:
> 
> 
> Le 15/04/2021 ? 08:54, Rasmus Villemoes a ?crit?:
>> On 15/04/2021 07.38, Stefan Roese wrote:
>>> On 13.04.21 16:43, Rasmus Villemoes wrote:
>>>> Some boards don't work with the rate-limiting done in the generic
>>>> watchdog_reset() provided by wdt-uclass.
>>>>
>>>> For example, on powerpc, get_timer() ceases working during bootm since
>>>> interrupts are disabled before the kernel image gets decompressed, and
>>>> when the decompression takes longer than the watchdog device
>>>> allows (or enough of the budget that the kernel doesn't get far enough
>>>> to assume responsibility for petting the watchdog), the result is a
>>>> non-booting board.
>>>>
>>>> As a somewhat hacky workaround (because DT is supposed to describe
>>>> hardware), allow specifying hw_margin_ms=0 in device tree to
>>>> effectively disable the ratelimiting and actually ping the watchdog
>>>> every time watchdog_reset() is called. For that to work, the "has
>>>> enough time passed" check just needs to be tweaked a little to allow
>>>> the now==next_reset case as well.
>>>>
>>>> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>> ---
>>>>
>>>> It's the option I dislike the most (because of the DT abuse), but I
>>>> also do accept that it's the one with the minimal code impact, and
>>>> apparently the path of least resistance. So here it is.
>>>
>>> Right. An alternative way would have been to add a new Kconfig symbol
>>> to define the default value of "reset_period" so that it can be
>>> configured to different values via Kconfig as well.
>>
>> No, I don't think we should not go in that direction.
> 
> Double negation ....
> 
> You mean: I think we should go ?

No, there's a "not" too many. "I don't think we should go in that
direction.". Thanks. Leftover from last-second rephrasing.

Rasmus

[PATCH] watchdog: use time_after_eq() in watchdog_reset()

[Stefan Roese]

On 13.04.21 16:43, Rasmus Villemoes wrote:
> Some boards don't work with the rate-limiting done in the generic
> watchdog_reset() provided by wdt-uclass.
> 
> For example, on powerpc, get_timer() ceases working during bootm since
> interrupts are disabled before the kernel image gets decompressed, and
> when the decompression takes longer than the watchdog device
> allows (or enough of the budget that the kernel doesn't get far enough
> to assume responsibility for petting the watchdog), the result is a
> non-booting board.
> 
> As a somewhat hacky workaround (because DT is supposed to describe
> hardware), allow specifying hw_margin_ms=0 in device tree to
> effectively disable the ratelimiting and actually ping the watchdog
> every time watchdog_reset() is called. For that to work, the "has
> enough time passed" check just needs to be tweaked a little to allow
> the now==next_reset case as well.
> 
> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
> 
> It's the option I dislike the most (because of the DT abuse), but I
> also do accept that it's the one with the minimal code impact, and
> apparently the path of least resistance. So here it is.
> 
>   drivers/watchdog/wdt-uclass.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 0603ffbd36..2687135296 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -148,7 +148,7 @@ void watchdog_reset(void)
>   
>   	/* Do not reset the watchdog too often */
>   	now = get_timer(0);
> -	if (time_after(now, next_reset)) {
> +	if (time_after_eq(now, next_reset)) {
>   		next_reset = now + reset_period;
>   		wdt_reset(gd->watchdog_dev);
>   	}
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de