[U-Boot] [PATCH] at91: Don't initialize watchdog if CONFIG_SYS_WDTC_WDMR_VAL is undefined
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
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
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
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
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
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
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
participants (2)
-
Alexander Stein -
Wolfgang Denk