[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Alexander Stein]

This allows Linux to initialize and use the watchdog with the included
driver.
CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG should be defined to make
u-boot trigger the watchdog

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 arch/arm/cpu/arm926ejs/at91/lowlevel_init.S |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
index 559c35c..21ebae9 100644
--- a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
+++ b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
@@ -186,8 +186,10 @@ SDRAM_setup_end:
 	.ltorg
 
 SMRDATA:
+#if defined(CONFIG_SYS_WDTC_WDMR_VAL)
 	.word AT91_ASM_WDT_MR
 	.word CONFIG_SYS_WDTC_WDMR_VAL
+#endif
 	/* configure PIOx as EBI0 D[16-31] */
 #if defined(CONFIG_AT91SAM9263)
 	.word AT91_ASM_PIOD_PDR
-- 
1.7.1

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Wolfgang Denk]

Dear Alexander Stein,

In message <1279713664-20353-1-git-send-email-alexander.stein@systec-electronic.com> you wrote:
> This allows Linux to initialize and use the watchdog with the included
> driver.
> CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG should be defined to make
> u-boot trigger the watchdog
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  arch/arm/cpu/arm926ejs/at91/lowlevel_init.S |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
> index 559c35c..21ebae9 100644
> --- a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
> +++ b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
> @@ -186,8 +186,10 @@ SDRAM_setup_end:
>  	.ltorg
>  
>  SMRDATA:
> +#if defined(CONFIG_SYS_WDTC_WDMR_VAL)
>  	.word AT91_ASM_WDT_MR
>  	.word CONFIG_SYS_WDTC_WDMR_VAL
> +#endif

This seems the wrong approach to me - when CONFIG_HW_WATCHDOG is
defined and requires CONFIG_SYS_WDTC_WDMR_VAL, but the latter is
missing, than this should raise n error condition. We must not
silently ignore errors.

NAK.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
"The POP3 server service depends on the SMTP  server  service,  which
failed to start because of the following error: The operation comple-
ted successfully."                         -- Windows NT Server v3.51

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Alexander Stein]

Hello,

Am Mittwoch, 21. Juli 2010, 20:20:48 schrieb Wolfgang Denk:
> > --- a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
> > +++ b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
> > 
> > @@ -186,8 +186,10 @@ SDRAM_setup_end:
> >  	.ltorg
> >  
> >  SMRDATA:
> > +#if defined(CONFIG_SYS_WDTC_WDMR_VAL)
> > 
> >  	.word AT91_ASM_WDT_MR
> >  	.word CONFIG_SYS_WDTC_WDMR_VAL
> > 
> > +#endif
> 
> This seems the wrong approach to me - when CONFIG_HW_WATCHDOG is
> defined and requires CONFIG_SYS_WDTC_WDMR_VAL, but the latter is
> missing, than this should raise n error condition. We must not
> silently ignore errors.

Well, my problem is, that CONFIG_SYS_WDTC_WDMR_VAL is used to program the 
internal watchdog. But this watchdog can only be programmed once until a reset 
occurs. So there is no possibility for linux to reprogram it.
So, if CONFIG_SYS_WDTC_WDMR_VAL is not defined and the watchdog is not 
programed using my patch, the watchdog still runs with default settings 
(timeout of 16s). So a user may choose to trigger the watchdog from u-boot 
(define CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG) or let it run 
silently. To summarize, CONFIG_SYS_WDTC_WDMR_VAL is not needed to use 
CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG.

Best regards,
Alexander Stein

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Wolfgang Denk]

Dear Alexander Stein,

In message <201007220817.17441.alexander.stein@systec-electronic.com> you wrote:
> 
> > This seems the wrong approach to me - when CONFIG_HW_WATCHDOG is
> > defined and requires CONFIG_SYS_WDTC_WDMR_VAL, but the latter is
> > missing, than this should raise n error condition. We must not
> > silently ignore errors.
> 
> Well, my problem is, that CONFIG_SYS_WDTC_WDMR_VAL is used to program the 
> internal watchdog. But this watchdog can only be programmed once until a reset 
> occurs. So there is no possibility for linux to reprogram it.

This is normal. Any watchdog that is worth the name will behave
similar.

> So, if CONFIG_SYS_WDTC_WDMR_VAL is not defined and the watchdog is not 
> programed using my patch, the watchdog still runs with default settings 
> (timeout of 16s). So a user may choose to trigger the watchdog from u-boot 
> (define CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG) or let it run 
> silently. To summarize, CONFIG_SYS_WDTC_WDMR_VAL is not needed to use 
> CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG.

Then the subject is misleading - it suggests you do not initialize /
let run the watchdog at all if CONFIG_SYS_WDTC_WDMR_VAL is undefined.


I think we should make sure that everything is in a sane and
consistent state - if CONFIG_HW_WATCHDOG (and
CONFIG_AT91SAM9_WATCHDOG) are set, this indicates that U-Boot is
supposed to use the watchdog, which in turn means they should
initialize it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
An optimist believes we live in the best world possible; a  pessimist
fears this is true.

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Alexander Stein]

Dear Wolfgang Denk,

Am Donnerstag, 22. Juli 2010, 09:09:12 schrieb Wolfgang Denk:
> > Well, my problem is, that CONFIG_SYS_WDTC_WDMR_VAL is used to program the
> > internal watchdog. But this watchdog can only be programmed once until a
> > reset occurs. So there is no possibility for linux to reprogram it.
> 
> This is normal. Any watchdog that is worth the name will behave
> similar.

Well, I encountered several watchdog which start only after the first trigger.

> > So, if CONFIG_SYS_WDTC_WDMR_VAL is not defined and the watchdog is not
> > programed using my patch, the watchdog still runs with default settings
> > (timeout of 16s). So a user may choose to trigger the watchdog from
> > u-boot (define CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG) or let
> > it run silently. To summarize, CONFIG_SYS_WDTC_WDMR_VAL is not needed to
> > use CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG.
> 
> Then the subject is misleading - it suggests you do not initialize /
> let run the watchdog at all if CONFIG_SYS_WDTC_WDMR_VAL is undefined.

Ok, maybe the subject need some rework.

> I think we should make sure that everything is in a sane and
> consistent state - if CONFIG_HW_WATCHDOG (and
> CONFIG_AT91SAM9_WATCHDOG) are set, this indicates that U-Boot is
> supposed to use the watchdog, which in turn means they should
> initialize it.

Of course, if CONFIG_HW_WATCHDOG and CONFIG_AT91SAM9_WATCHDOG is set, u-boot 
should use/trigger the watchdog. But (re-)programming the watchdog on AT91 is 
not _always_ necessary. Without programming (using CONFIG_SYS_WDTC_WDMR_VAL), 
the watchdog just runs with default settings, allowing e.g. Linux to reprogram 
it later.
This patch allows to program the watchdog from u-boot, if wanted, but also 
allows to reprogram it later from Linux. While triggering the watchdog from u-
boot itself does still work, wrt. the default watchdog settings.

Best regards,
Alexander Stein

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Wolfgang Denk]

Dear Alexander,

In message <201007220924.51614.alexander.stein@systec-electronic.com> you wrote:
> 
> > This is normal. Any watchdog that is worth the name will behave
> > similar.
> 
> Well, I encountered several watchdog which start only after the first trigger.

Me too. They are ok in many applications, but completely unacceptable
for advanced security requirements.

> Of course, if CONFIG_HW_WATCHDOG and CONFIG_AT91SAM9_WATCHDOG is set, u-boot 
> should use/trigger the watchdog. But (re-)programming the watchdog on AT91 is 
> not _always_ necessary. Without programming (using CONFIG_SYS_WDTC_WDMR_VAL), 
> the watchdog just runs with default settings, allowing e.g. Linux to reprogram 
> it later.
> This patch allows to program the watchdog from u-boot, if wanted, but also 
> allows to reprogram it later from Linux. While triggering the watchdog from u-
> boot itself does still work, wrt. the default watchdog settings.

I can see the intention, but this should not depend on "val" being
defined or not - and it needs explicit documentation (in the README,
for example).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
"In matrimony, to hesitate is sometimes to be saved."        - Butler

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Alexander Stein]

Dear Wolfgang,

Am Donnerstag, 22. Juli 2010, 15:53:25 schrieben Sie:
> > Of course, if CONFIG_HW_WATCHDOG and CONFIG_AT91SAM9_WATCHDOG is set,
> > u-boot should use/trigger the watchdog. But (re-)programming the
> > watchdog on AT91 is not always necessary. Without programming (using
> > CONFIG_SYS_WDTC_WDMR_VAL), the watchdog just runs with default settings,
> > allowing e.g. Linux to reprogram it later.
> > This patch allows to program the watchdog from u-boot, if wanted, but
> > also allows to reprogram it later from Linux. While triggering the
> > watchdog from u- boot itself does still work, wrt. the default watchdog
> > settings.
> 
> I can see the intention, but this should not depend on "val" being
> defined or not - and it needs explicit documentation (in the README,
> for example).

So, you would be ok with an option e.g. CONFIG_SKIP_WATCHDOG_PROGRAM with a 
documentation in README.at91?

Best regards,
Alexander

[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined

[Wolfgang Denk]

Dear Alexander Stein,

please always keep the mailing list on Cc: - thanks.

In message <201007260810.47268.alexander.stein@systec-electronic.com> you wrote:
> 
> > I can see the intention, but this should not depend on "val" being
> > defined or not - and it needs explicit documentation (in the README,
> > for example).
> 
> So, you would be ok with an option e.g. CONFIG_SKIP_WATCHDOG_PROGRAM with a 
> documentation in README.at91?

CONFIG_SKIP_WATCHDOG_INIT would probably be better, and it needs to be
documented in the central README, like all other CONFIG_ options as
well.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is a nanocentury.
                                                - Tom Duff, Bell Labs

[U-Boot] [PATCH v2] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is undefined

[Alexander Stein]

This allows Linux to initialize and use the watchdog with the included
driver.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v2:
* Add a new specific option CONFIG_SKIP_WATCHDOG_INIT
* Add some documentation

 README                                      |    5 +++++
 arch/arm/cpu/arm926ejs/at91/lowlevel_init.S |    2 ++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/README b/README
index a9c98f2..502054d 100644
--- a/README
+++ b/README
@@ -2817,6 +2817,11 @@ Low Level (hardware related) configuration options:
 		some other boot loader or by a debugger which
 		performs these initializations itself.
 
+- CONFIG_SKIP_WATCHDOG_INIT
+
+		[arm AT91 only] If this variable is defined, then the
+		watchdog will not be programmed upon u-boot start.
+
 - CONFIG_PRELOADER
 
 		Modifies the behaviour of start.S when compiling a loader
diff --git a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
index 559c35c..0645ba8 100644
--- a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
+++ b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
@@ -186,8 +186,10 @@ SDRAM_setup_end:
 	.ltorg
 
 SMRDATA:
+#ifndef CONFIG_SKIP_WATCHDOG_INIT
 	.word AT91_ASM_WDT_MR
 	.word CONFIG_SYS_WDTC_WDMR_VAL
+#endif
 	/* configure PIOx as EBI0 D[16-31] */
 #if defined(CONFIG_AT91SAM9263)
 	.word AT91_ASM_PIOD_PDR
-- 
1.7.1

[U-Boot] [PATCH v2] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is undefined

[Wolfgang Denk]

Dear Alexander Stein,

In message <1280136874-20303-1-git-send-email-alexander.stein@systec-electronic.com> you wrote:
> This allows Linux to initialize and use the watchdog with the included
> driver.

Please explain why this is needed. It may be clear to you, but no to
most other readers.

> +- CONFIG_SKIP_WATCHDOG_INIT
> +
> +		[arm AT91 only] If this variable is defined, then the
> +		watchdog will not be programmed upon u-boot start.

Please explain what that means. It may be clear to you, but...


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
"On two occasions I have been  asked  [by  members  of  Parliament!],
'Pray,  Mr.  Babbage, if you put into the machine wrong figures, will
the right answers come out?' I am not able rightly to  apprehend  the
kind of confusion of ideas that could provoke such a question."
- Charles Babbage

[U-Boot] [PATCH v3] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is undefined

[Alexander Stein]

On AT91 the watchdog mode register can only be written once after reset.
If this register is written by u-boot e.g. a Linux driver can't
reconfigure the watchdog later. If the watchdog is left untouched this
is possible. Without touching the mode register the watchdog has a default
setup and u-boot is still able to trigger the watchdog.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v2:
* Add a new specific option CONFIG_SKIP_WATCHDOG_INIT
* Add some documentation

Changes in v3:
* Be more verbose in describing the problem in the commit message and README

 README                                      |   11 +++++++++++
 arch/arm/cpu/arm926ejs/at91/lowlevel_init.S |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/README b/README
index a9c98f2..110668d 100644
--- a/README
+++ b/README
@@ -2817,6 +2817,17 @@ Low Level (hardware related) configuration options:
 		some other boot loader or by a debugger which
 		performs these initializations itself.
 
+- CONFIG_SKIP_WATCHDOG_INIT
+
+		[arm AT91 only] If this variable is defined, then the
+		watchdog will not be programmed upon u-boot start.
+		On AT91 the watchdog mode register can only be written
+		once after reset. If this register is written by u-boot
+		e.g. a Linux driver can't reconfigure the watchdog later. If
+		the watchdog is left untouched this is possible.
+		Without touching the mode register the watchdog has a default
+		setup and u-boot is still able to trigger the watchdog.
+
 - CONFIG_PRELOADER
 
 		Modifies the behaviour of start.S when compiling a loader
diff --git a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
index 559c35c..0645ba8 100644
--- a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
+++ b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
@@ -186,8 +186,10 @@ SDRAM_setup_end:
 	.ltorg
 
 SMRDATA:
+#ifndef CONFIG_SKIP_WATCHDOG_INIT
 	.word AT91_ASM_WDT_MR
 	.word CONFIG_SYS_WDTC_WDMR_VAL
+#endif
 	/* configure PIOx as EBI0 D[16-31] */
 #if defined(CONFIG_AT91SAM9263)
 	.word AT91_ASM_PIOD_PDR
-- 
1.7.1

[U-Boot] [PATCH v3] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is undefined

[Wolfgang Denk]

Dear Alexander Stein,

In message <1280145784-18909-1-git-send-email-alexander.stein@systec-electronic.com> you wrote:
> On AT91 the watchdog mode register can only be written once after reset.
> If this register is written by u-boot e.g. a Linux driver can't
> reconfigure the watchdog later. If the watchdog is left untouched this
> is possible. Without touching the mode register the watchdog has a default
> setup and u-boot is still able to trigger the watchdog.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
> Changes in v2:
> * Add a new specific option CONFIG_SKIP_WATCHDOG_INIT
> * Add some documentation
> 
> Changes in v3:
> * Be more verbose in describing the problem in the commit message and README
> 
>  README                                      |   11 +++++++++++
>  arch/arm/cpu/arm926ejs/at91/lowlevel_init.S |    2 ++
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/README b/README
> index a9c98f2..110668d 100644
> --- a/README
> +++ b/README
> @@ -2817,6 +2817,17 @@ Low Level (hardware related) configuration options:
>  		some other boot loader or by a debugger which
>  		performs these initializations itself.
>  
> +- CONFIG_SKIP_WATCHDOG_INIT
> +
> +		[arm AT91 only] If this variable is defined, then the
> +		watchdog will not be programmed upon u-boot start.
> +		On AT91 the watchdog mode register can only be written
> +		once after reset. If this register is written by u-boot
> +		e.g. a Linux driver can't reconfigure the watchdog later. If
> +		the watchdog is left untouched this is possible.
> +		Without touching the mode register the watchdog has a default
> +		setup and u-boot is still able to trigger the watchdog.

I think the Subject: is wrong. It reads:

	Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is
	undefined

I think you mean "if CONFIG_SKIP_WATCHDOG_INIT is _DEFINED_" ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
You've no idea of what a poor opinion  I  have  of  myself,  and  how
little I deserve it.                                  - W. S. Gilbert

[U-Boot] [PATCH v4] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is defined

[Alexander Stein]

On AT91 the watchdog mode register can only be written once after reset.
If this register is written by u-boot e.g. a Linux driver can't
reconfigure the watchdog later. If the watchdog is left untouched this
is possible. Without touching the mode register the watchdog has a default
setup and u-boot is still able to trigger the watchdog.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v2:
* Add a new specific option CONFIG_SKIP_WATCHDOG_INIT
* Add some documentation

Changes in v3:
* Be more verbose in describing the problem in the commit message and README

Changes in v4:
* Fix commit message

 README                                      |   11 +++++++++++
 arch/arm/cpu/arm926ejs/at91/lowlevel_init.S |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/README b/README
index b6bf451..9fbb6c9 100644
--- a/README
+++ b/README
@@ -2827,6 +2827,17 @@ Low Level (hardware related) configuration options:
 		some other boot loader or by a debugger which
 		performs these initializations itself.
 
+- CONFIG_SKIP_WATCHDOG_INIT
+
+		[arm AT91 only] If this variable is defined, then the
+		watchdog will not be programmed upon u-boot start.
+		On AT91 the watchdog mode register can only be written
+		once after reset. If this register is written by u-boot
+		e.g. a Linux driver can't reconfigure the watchdog later. If
+		the watchdog is left untouched this is possible.
+		Without touching the mode register the watchdog has a default
+		setup and u-boot is still able to trigger the watchdog.
+
 - CONFIG_PRELOADER
 
 		Modifies the behaviour of start.S when compiling a loader
diff --git a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
index 559c35c..0645ba8 100644
--- a/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
+++ b/arch/arm/cpu/arm926ejs/at91/lowlevel_init.S
@@ -186,8 +186,10 @@ SDRAM_setup_end:
 	.ltorg
 
 SMRDATA:
+#ifndef CONFIG_SKIP_WATCHDOG_INIT
 	.word AT91_ASM_WDT_MR
 	.word CONFIG_SYS_WDTC_WDMR_VAL
+#endif
 	/* configure PIOx as EBI0 D[16-31] */
 #if defined(CONFIG_AT91SAM9263)
 	.word AT91_ASM_PIOD_PDR
-- 
1.7.1

[U-Boot] [PATCH v4] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is defined

[Mike Frysinger]

On Mon, Aug 9, 2010 at 2:40 AM, Alexander Stein wrote:
> On AT91 the watchdog mode register can only be written once after reset.
> If this register is written by u-boot e.g. a Linux driver can't
> reconfigure the watchdog later. If the watchdog is left untouched this
> is possible. Without touching the mode register the watchdog has a default
> setup and u-boot is still able to trigger the watchdog.
>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
> Changes in v2:
> * Add a new specific option CONFIG_SKIP_WATCHDOG_INIT
> * Add some documentation
>
> Changes in v3:
> * Be more verbose in describing the problem in the commit message and README
>
> Changes in v4:
> * Fix commit message
>
> ?README ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 11 +++++++++++
> ?arch/arm/cpu/arm926ejs/at91/lowlevel_init.S | ? ?2 ++
> ?2 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/README b/README
> index b6bf451..9fbb6c9 100644
> --- a/README
> +++ b/README
> @@ -2827,6 +2827,17 @@ Low Level (hardware related) configuration options:
> ? ? ? ? ? ? ? ?some other boot loader or by a debugger which
> ? ? ? ? ? ? ? ?performs these initializations itself.
>
> +- CONFIG_SKIP_WATCHDOG_INIT
> +
> + ? ? ? ? ? ? ? [arm AT91 only] If this variable is defined, then the
> + ? ? ? ? ? ? ? watchdog will not be programmed upon u-boot start.
> + ? ? ? ? ? ? ? On AT91 the watchdog mode register can only be written
> + ? ? ? ? ? ? ? once after reset. If this register is written by u-boot
> + ? ? ? ? ? ? ? e.g. a Linux driver can't reconfigure the watchdog later. If
> + ? ? ? ? ? ? ? the watchdog is left untouched this is possible.
> + ? ? ? ? ? ? ? Without touching the mode register the watchdog has a default
> + ? ? ? ? ? ? ? setup and u-boot is still able to trigger the watchdog.

isnt the at91 logic inverted ?  shouldnt the watchdog programming only
be done when someone has opted in to it via some watchdog define ?
-mike

[U-Boot] [PATCH v4] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is defined

[Alexander Stein]

Hello Mike,

Am Montag, 9. August 2010, 09:13:45 schrieb Mike Frysinger:
> > On AT91 the watchdog mode register can only be written once after reset.
> > If this register is written by u-boot e.g. a Linux driver can't
> > reconfigure the watchdog later. If the watchdog is left untouched this
> > is possible. Without touching the mode register the watchdog has a
> > default setup and u-boot is still able to trigger the watchdog.
> >
> > [...]
> >
> > +- CONFIG_SKIP_WATCHDOG_INIT
> > +
> > +               [arm AT91 only] If this variable is defined, then the
> > +               watchdog will not be programmed upon u-boot start.
> > +               On AT91 the watchdog mode register can only be written
> > +               once after reset. If this register is written by u-boot
> > +               e.g. a Linux driver can't reconfigure the watchdog later.
> > If +               the watchdog is left untouched this is possible. +  
> >             Without touching the mode register the watchdog has a
> > default +               setup and u-boot is still able to trigger the
> > watchdog.
> 
> isnt the at91 logic inverted ?  shouldnt the watchdog programming only
> be done when someone has opted in to it via some watchdog define ?

This was my first version, but Wolfgang NAK'ed it as he see this as the wrong 
approach. See http://article.gmane.org/gmane.comp.boot-loaders.u-boot/81589

Best regards,
Alexander

[U-Boot] [PATCH v4] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is defined

[Mike Frysinger]

On Mon, Aug 9, 2010 at 7:42 AM, Alexander Stein wrote:
> Am Montag, 9. August 2010, 09:13:45 schrieb Mike Frysinger:
>> > On AT91 the watchdog mode register can only be written once after reset.
>> > If this register is written by u-boot e.g. a Linux driver can't
>> > reconfigure the watchdog later. If the watchdog is left untouched this
>> > is possible. Without touching the mode register the watchdog has a
>> > default setup and u-boot is still able to trigger the watchdog.
>> >
>> > [...]
>> >
>> > +- CONFIG_SKIP_WATCHDOG_INIT
>> > +
>> > + ? ? ? ? ? ? ? [arm AT91 only] If this variable is defined, then the
>> > + ? ? ? ? ? ? ? watchdog will not be programmed upon u-boot start.
>> > + ? ? ? ? ? ? ? On AT91 the watchdog mode register can only be written
>> > + ? ? ? ? ? ? ? once after reset. If this register is written by u-boot
>> > + ? ? ? ? ? ? ? e.g. a Linux driver can't reconfigure the watchdog later.
>> > If + ? ? ? ? ? ? ? the watchdog is left untouched this is possible. +
>> > ? ? ? ? ? ? Without touching the mode register the watchdog has a
>> > default + ? ? ? ? ? ? ? setup and u-boot is still able to trigger the
>> > watchdog.
>>
>> isnt the at91 logic inverted ? ?shouldnt the watchdog programming only
>> be done when someone has opted in to it via some watchdog define ?
>
> This was my first version, but Wolfgang NAK'ed it as he see this as the wrong
> approach. See http://article.gmane.org/gmane.comp.boot-loaders.u-boot/81589

i think you interpreted his statement incorrectly ?  he said it should
raise an error, not that supporting CONFIG_HW_WATCHDOG is wrong.
-mike

[U-Boot] [PATCH v4] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is defined

[Reinhard Meyer]

Dear Alexander Stein,

just to bring in my thoughts to this watchdog issue,
and to explain what I think the issue is here:

1. on (all?) AT91SAM9 devices the watchdog is initially enabled
(after Reset) with a 16 second timeout (provides a 32kHz Xtal
is used).

2. the watchdog mode register can only be written once, then it
becomes read-only.

3. on (all?) systems without NOR flash u-boot is a secondary
boot loader. That primary bootloader in that case _could_ have
written the mode register.

4. usually systems would leave the watchdog untouched until the
final operating systems takes over.

That means that we should have two, positively acting defines that

1. make u-boot retrigger the watchdog within the 16 second interval
(if NOT defined, u-boot will NOT retrigger the watchdog)

2. make u-boot write the mode register with any user defined value
(watchdog disabled (forever), or enabled with different timeout or
action)

The define for 1. could essentially be on for every system, because
it would not hurt to retrigger a disabled watchdog; the define for 2.
would require the define 1., if the watchdog stays enabled.

So... that being said, can we go forward as follows:

CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG
need both be defined so u-boot will periodically retrigger the watchdog
independant of its mode.

CONFIG_SYS_WDTC_WDMR_VAL, _IF_ defined will make u-boot write that
value into the watchdog mode register.

I know that is exactly Alexander's original proposal, and with proper
README it should be understandable that this is the right way to do it.

Best Regards,
Reinhard

[U-Boot] [PATCH v4] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is defined

[Mike Frysinger]

On Friday, August 20, 2010 08:31:22 Reinhard Meyer wrote:
> just to bring in my thoughts to this watchdog issue,
> and to explain what I think the issue is here:
> 
> 1. on (all?) AT91SAM9 devices the watchdog is initially enabled
> (after Reset) with a 16 second timeout (provides a 32kHz Xtal
> is used).

ah, i think that's the kicker and what needs to be noted.  i wasnt aware of 
devices that did this sort of thing.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100820/86c8a6e0/attachment.pgp 

[U-Boot] [PATCH v4] at91: Don't initialize watchdog if CONFIG_SKIP_WATCHDOG_INIT is defined

[Reinhard Meyer]

Dear Mike Frysinger,
> On Friday, August 20, 2010 08:31:22 Reinhard Meyer wrote:
>> just to bring in my thoughts to this watchdog issue,
>> and to explain what I think the issue is here:
>>
>> 1. on (all?) AT91SAM9 devices the watchdog is initially enabled
>> (after Reset) with a 16 second timeout (provides a 32kHz Xtal
>> is used).
>
> ah, i think that's the kicker and what needs to be noted.  i wasnt aware of
> devices that did this sort of thing.
I think that's rather ingenious, with a little hardware one
could toggle between boot devices or toggle some higher address
line of a NOR flash to try several bootloaders.

So... lets proceed here...

For the doc/README.at91-watchdog (new file):

"<explanation of AT91 watchdog function, like in my previous mail>

If CONFIG_SYS_WDTC_WDMR_VAL is defined u-boot will write that
value into the watchdog mode register. Otherwise the watchdog
is left in its initial state: active with 16 second timeout.

Note that the watchdog mode register can only be written once.

If the watchdog is left running or programmed to be running,
you need to enable its retriggering by defining
CONFIG_AT91SAM9_WATCHDOG and CONFIG_HW_WATCHDOG."

Alexander, can you prepare a revised patch with README like above
and a better (positive logic) subject like
"AT91: program WDMR only if CONFIG_SYS_WDTC_WDMR_VAL is defined"?

Maybe change CONFIG_SYS_WDTC_WDMR_VAL to CONFIG_AT91_WDTC_WDMR_VAL,
because its AT91 specific but user changeable?

Best Regards,
Reinhard