[PATCH v3 1/2] timer: mpc83xx_timer: fix build with CONFIG_{HW_, }WATCHDOG

[PATCH v3 1/2] timer: mpc83xx_timer: fix build with CONFIG_{HW_, }WATCHDOG

[Rasmus Villemoes]

The code, which is likely copied from arch/powerpc/lib/interrupts.c,
lacks a fallback definition of CONFIG_SYS_WATCHDOG_FREQ and refers to
a non-existing timestamp variable - obviously priv->timestamp is
meant.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/timer/mpc83xx_timer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c
index 72cb58b693..da516c9d74 100644
--- a/drivers/timer/mpc83xx_timer.c
+++ b/drivers/timer/mpc83xx_timer.c
@@ -16,6 +16,10 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+#ifndef CONFIG_SYS_WATCHDOG_FREQ
+#define CONFIG_SYS_WATCHDOG_FREQ (CONFIG_SYS_HZ / 2)
+#endif
+
 /**
  * struct mpc83xx_timer_priv - Private data structure for MPC83xx timer driver
  * @decrementer_count: Value to which the decrementer register should be re-set
@@ -167,7 +171,7 @@ void timer_interrupt(struct pt_regs *regs)
 	priv->timestamp++;
 
 #if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG)
-	if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
+	if ((priv->timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
 		WATCHDOG_RESET();
 #endif    /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */
 
-- 
2.23.0

[PATCH v3 2/2] allow opting out of WATCHDOG_RESET() from timer interrupt

[Rasmus Villemoes]

When using CONFIG_(SPL_)WDT, the watchdog_reset function is a lot more
complicated than just poking a few SOC-specific registers - it
involves accessing all kinds of global data, and if the interrupt
happens at the wrong time (say, in the middle of an WATCHDOG_RESET()
call from ordinary code), that can end up corrupting said global data.

Also, having WATCHDOG_RESET() called automatically from the timer
interrupt runs counter to the idea of a watchdog device - if the board
runs into an infinite loops with interrupts still enabled, the
watchdog will never fire.

Allow the board to opt out of this behaviour by setting
CONFIG_SYS_WATCHDOG_FREQ to 0 - as that setting is currently
nonsensical, it cannot affect any existing boards.

Add documentation for both the existing and extended meaning of
CONFIG_SYS_WATCHDOG_FREQ.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 README                        | 9 +++++++++
 arch/m68k/lib/time.c          | 2 +-
 arch/powerpc/lib/interrupts.c | 2 +-
 drivers/timer/mpc83xx_timer.c | 2 +-
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/README b/README
index 8cfa92fac9..79e8da4adc 100644
--- a/README
+++ b/README
@@ -768,6 +768,15 @@ The following options need to be configured:
 		SoC, then define this variable and provide board
 		specific code for the "hw_watchdog_reset" function.
 
+		CONFIG_SYS_WATCHDOG_FREQ
+		Some platforms automatically call WATCHDOG_RESET()
+		from the timer interrupt handler every
+		CONFIG_SYS_WATCHDOG_FREQ interrupts. If not set by the
+		board configuration file, a default of CONFIG_SYS_HZ/2
+		(i.e. 500) is used. Setting CONFIG_SYS_WATCHDOG_FREQ
+		to 0 disables calling WATCHDOG_RESET() from the timer
+		interrupt.
+
 - Real-Time Clock:
 
 		When CONFIG_CMD_DATE is selected, the type of the RTC
diff --git a/arch/m68k/lib/time.c b/arch/m68k/lib/time.c
index bde1f4c228..038019ff36 100644
--- a/arch/m68k/lib/time.c
+++ b/arch/m68k/lib/time.c
@@ -68,7 +68,7 @@ void dtimer_interrupt(void *not_used)
 		timestamp++;
 
 		#if defined(CONFIG_WATCHDOG) || defined (CONFIG_HW_WATCHDOG)
-		if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) {
+		if (CONFIG_SYS_WATCHDOG_FREQ && (timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0) {
 			WATCHDOG_RESET ();
 		}
 		#endif    /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */
diff --git a/arch/powerpc/lib/interrupts.c b/arch/powerpc/lib/interrupts.c
index 64ee0cc210..23ac5bca1e 100644
--- a/arch/powerpc/lib/interrupts.c
+++ b/arch/powerpc/lib/interrupts.c
@@ -79,7 +79,7 @@ void timer_interrupt(struct pt_regs *regs)
 	timestamp++;
 
 #if defined(CONFIG_WATCHDOG) || defined (CONFIG_HW_WATCHDOG)
-	if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
+	if (CONFIG_SYS_WATCHDOG_FREQ && (timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
 		WATCHDOG_RESET ();
 #endif    /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */
 
diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c
index da516c9d74..b85f10ad99 100644
--- a/drivers/timer/mpc83xx_timer.c
+++ b/drivers/timer/mpc83xx_timer.c
@@ -171,7 +171,7 @@ void timer_interrupt(struct pt_regs *regs)
 	priv->timestamp++;
 
 #if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG)
-	if ((priv->timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
+	if (CONFIG_SYS_WATCHDOG_FREQ && (priv->timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
 		WATCHDOG_RESET();
 #endif    /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */
 
-- 
2.23.0

[PATCH v3 1/2] timer: mpc83xx_timer: fix build with CONFIG_{HW_, }WATCHDOG

[Wolfgang Denk]

Dear Rasmus,

In message <20200320105248.24518-1-rasmus.villemoes@prevas.dk> you wrote:
> The code, which is likely copied from arch/powerpc/lib/interrupts.c,
> lacks a fallback definition of CONFIG_SYS_WATCHDOG_FREQ and refers to
> a non-existing timestamp variable - obviously priv->timestamp is
> meant.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/timer/mpc83xx_timer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Could you _please_ get used to add some patch histroy below this
line "---", too? I. e. some information so we can see easily what
has changed between patch version and and 2, and between version 2
and 3?

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Boss, n.: According to the Oxford English Dictionary, in  the  Middle
Ages  the  words  "boss"  and "botch" were largely synonymous, except
that boss, in addition to meaning  "a  supervisor  of  workers"  also
meant "an ornamental stud."

[PATCH v3 1/2] timer: mpc83xx_timer: fix build with CONFIG_{HW_,}WATCHDOG

[Rasmus Villemoes]

On 20/03/2020 12.22, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <20200320105248.24518-1-rasmus.villemoes@prevas.dk> you wrote:
>> The code, which is likely copied from arch/powerpc/lib/interrupts.c,
>> lacks a fallback definition of CONFIG_SYS_WATCHDOG_FREQ and refers to
>> a non-existing timestamp variable - obviously priv->timestamp is
>> meant.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  drivers/timer/mpc83xx_timer.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Could you _please_ get used to add some patch histroy below this
> line "---", too? I. e. some information so we can see easily what
> has changed between patch version and and 2, and between version 2
> and 3?

Sorry about that. When this changed from a single patch to multiple ones
I should have prepended a cover letter as well.

FWIW, patch 1/2 is new in v3, while 2/2 has been extended with
documentation of both the existing meaning of CONFIG_SYS_WATCHDOG_FREQ
as well as the semantics of setting that to 0, while also making it
consistent across (the two implementations on) ppc and m68k.

Can I get you to review v3 as is, or should I rebase to master and
resend a v4?

Thanks,
Rasmus