Re: [U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined
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
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
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 thewatchdog will not be programmed upon u-boot start.
Please explain what that means. It may be clear to you, but...
Best regards,
Wolfgang Denk
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
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 thewatchdog will not be programmed upon u-boot start.On AT91 the watchdog mode register can only be writtenonce after reset. If this register is written by u-boote.g. a Linux driver can't reconfigure the watchdog later. Ifthe watchdog is left untouched this is possible.Without touching the mode register the watchdog has a defaultsetup 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
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
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
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 thewatchdog will not be programmed upon u-boot start.On AT91 the watchdog mode register can only be writtenonce after reset. If this register is written by u-boote.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
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
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
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:
- 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
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:
- 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
participants (4)
-
Alexander Stein -
Mike Frysinger -
Reinhard Meyer -
Wolfgang Denk