[U-Boot] [PATCH] arm:kirkwood: Add hardware watchdog support for Marvell Kirkwood boards
Initialize by calling kw_watchdog_init() with the number of seconds for the watchdog to timeout.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- cpu/arm926ejs/kirkwood/timer.c | 29 +++++++++++++++++++++++++++++ include/asm-arm/arch-kirkwood/cpu.h | 2 ++ 2 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/cpu/arm926ejs/kirkwood/timer.c b/cpu/arm926ejs/kirkwood/timer.c index 817ff42..f3397e7 100644 --- a/cpu/arm926ejs/kirkwood/timer.c +++ b/cpu/arm926ejs/kirkwood/timer.c @@ -25,6 +25,7 @@ #include <asm/arch/kirkwood.h>
#define UBOOT_CNTR 0 /* counter to use for uboot timer */ +#define WATCHDOG_CNTR 2
/* Timer reload and current value registers */ struct kwtmr_val { @@ -166,3 +167,31 @@ int timer_init(void)
return 0; } + +#if defined(CONFIG_HW_WATCHDOG) +static unsigned long watchdog_timeout = 5; +void hw_watchdog_reset(void) +{ + unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout; + + writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR)); +} + +void kw_watchdog_init(unsigned long timeout_secs) +{ + struct kwcpu_registers *cpureg = + (struct kwcpu_registers *)KW_CPU_REG_BASE; + unsigned int cntmrctrl; + + watchdog_timeout = timeout_secs; + /* Enable CPU reset if watchdog expires */ + cpureg->rstoutn_mask |= WATCHDOG_CNTR; + hw_watchdog_reset(); + + /* Enable the watchdog */ + cntmrctrl = readl(CNTMR_CTRL_REG); + cntmrctrl |= CTCR_ARM_TIMER_EN(WATCHDOG_CNTR); + cntmrctrl |= CTCR_ARM_TIMER_AUTO_EN(WATCHDOG_CNTR); + writel(cntmrctrl, CNTMR_CTRL_REG); +} +#endif diff --git a/include/asm-arm/arch-kirkwood/cpu.h b/include/asm-arm/arch-kirkwood/cpu.h index b3022a3..df49c3f 100644 --- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7, unsigned int mpp8_15, unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55); unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs); + #endif /* __ASSEMBLY__ */ #endif /* _KWCPU_H */
Simon Kagstrom wrote:
Initialize by calling kw_watchdog_init() with the number of seconds for the watchdog to timeout.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
cpu/arm926ejs/kirkwood/timer.c | 29 +++++++++++++++++++++++++++++ include/asm-arm/arch-kirkwood/cpu.h | 2 ++ 2 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/cpu/arm926ejs/kirkwood/timer.c b/cpu/arm926ejs/kirkwood/timer.c index 817ff42..f3397e7 100644 --- a/cpu/arm926ejs/kirkwood/timer.c +++ b/cpu/arm926ejs/kirkwood/timer.c @@ -25,6 +25,7 @@ #include <asm/arch/kirkwood.h>
#define UBOOT_CNTR 0 /* counter to use for uboot timer */ +#define WATCHDOG_CNTR 2
/* Timer reload and current value registers */ struct kwtmr_val { @@ -166,3 +167,31 @@ int timer_init(void)
return 0; }
+#if defined(CONFIG_HW_WATCHDOG) +static unsigned long watchdog_timeout = 5; +void hw_watchdog_reset(void) +{
- unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
- writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
+}
+void kw_watchdog_init(unsigned long timeout_secs) +{
- struct kwcpu_registers *cpureg =
(struct kwcpu_registers *)KW_CPU_REG_BASE;- unsigned int cntmrctrl;
- watchdog_timeout = timeout_secs;
- /* Enable CPU reset if watchdog expires */
- cpureg->rstoutn_mask |= WATCHDOG_CNTR;
- hw_watchdog_reset();
- /* Enable the watchdog */
- cntmrctrl = readl(CNTMR_CTRL_REG);
- cntmrctrl |= CTCR_ARM_TIMER_EN(WATCHDOG_CNTR);
- cntmrctrl |= CTCR_ARM_TIMER_AUTO_EN(WATCHDOG_CNTR);
- writel(cntmrctrl, CNTMR_CTRL_REG);
+} +#endif diff --git a/include/asm-arm/arch-kirkwood/cpu.h b/include/asm-arm/arch-kirkwood/cpu.h index b3022a3..df49c3f 100644 --- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7, unsigned int mpp8_15, unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55); unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs);
You should add hw_watchdog_reset to H the file or declare it static.
You may want to add #define stubs to handle the ifndef CONFIG_HW_WATCHDOG.
Tom
#endif /* __ASSEMBLY__ */ #endif /* _KWCPU_H */
On Mon, 28 Sep 2009 07:36:32 -0500 Tom Tom.Rix@windriver.com wrote:
+void hw_watchdog_reset(void) +{
- unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
diff --git a/include/asm-arm/arch-kirkwood/cpu.h b/include/asm-arm/arch-kirkwood/cpu.h index b3022a3..df49c3f 100644 --- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7, unsigned int mpp8_15, unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55); unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs);
You should add hw_watchdog_reset to H the file or declare it static.
It's a public interface and defined in include/watchdog.h (this is what's being called by WATCHDOG_RESET), but I'll submit a new patch which includes this.
You may want to add #define stubs to handle the ifndef CONFIG_HW_WATCHDOG.
I'm afraid I don't understand this comment though, what is the suggestion here?
// Simon
Simon Kagstrom wrote:
On Mon, 28 Sep 2009 07:36:32 -0500 Tom Tom.Rix@windriver.com wrote:
+void hw_watchdog_reset(void) +{
- unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
diff --git a/include/asm-arm/arch-kirkwood/cpu.h b/include/asm-arm/arch-kirkwood/cpu.h index b3022a3..df49c3f 100644 --- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7, unsigned int mpp8_15, unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55); unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs);
You should add hw_watchdog_reset to H the file or declare it static.
It's a public interface and defined in include/watchdog.h (this is what's being called by WATCHDOG_RESET), but I'll submit a new patch which includes this.
Yes thanks. My mistake. A new patch is not needed.
You may want to add #define stubs to handle the ifndef CONFIG_HW_WATCHDOG.
I'm afraid I don't understand this comment though, what is the suggestion here?
// Simon
Something like this is already being done in watchdog.h So it it not needed.
#ifdef CONFIG_HW_WATCHDOG #if defined(__ASSEMBLY__) #define WATCHDOG_RESET bl hw_watchdog_reset #else extern void hw_watchdog_reset(void);
#define WATCHDOG_RESET hw_watchdog_reset #endif /* __ASSEMBLY__ */ #else
<snip> /* * No hardware or software watchdog. */ #if defined(__ASSEMBLY__) #define WATCHDOG_RESET /*XXX DO_NOT_DEL_THIS_COMMENT*/ #else ----> stub #define WATCHDOG_RESET() {} #endif /* __ASSEMBLY__ */ #endif /* CONFIG_WATCHDOG && !__ASSEMBLY__ */ #endif /* CONFIG_HW_WATCHDOG */
Thanks for making your changes clear to me. Ack-ed
Tom
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Simon Kagstrom Sent: Monday, September 28, 2009 12:36 PM To: u-boot@lists.denx.de Subject: [U-Boot] [PATCH] arm:kirkwood: Add hardware watchdog support for Marvell Kirkwood boards
Initialize by calling kw_watchdog_init() with the number of seconds for the watchdog to timeout.
Hi Simon It is good to have more support available for Kirkwood and thanks for your efforts.
But do you really need Watchdog support for u-boot? Because if you want to use the watchdog, you will need to keep it running in Entire code.
May you pls explain your objective for enabling it?
Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c and its implementation, This will be a good way of implementation.
Regards.. Prafulla . .
Regards.. Prafulla . .
On Mon, 28 Sep 2009 19:16:16 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
But do you really need Watchdog support for u-boot?
Paranoia really has no limits :-). The main objective for me personally is to have the watchdog on when Linux starts, but if there is a risk (for whatever reason) that U-boot hangs, it would also help there.
Because if you want to use the watchdog, you will need to keep it running in Entire code.
That's fine, the code is already sprinkled with WATCHDOG_RESET() in many places (which calls hw_watchdog_reset()). Also note that this patch doesn't actually turn it on, you'll have to call kw_watchdog_init() first to do that.
Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c and its implementation, This will be a good way of implementation.
Well, I did look at that, and I believe the implementation is fairly similar.
What I wonder about in that context is the use of hw_watchdog_init(). I first thought this was generic, but it's not exported via watchdog.h (like hw_watchdog_reset()). I think it would be nice to have a generic interface which exports
void hw_watchdog_init(unsigned long timeout_ms);
to initialize the watchdog and timeout. The timeout would be a bit crude since hardware have limits to how long the timeouts would be, but anyway.
Another good feature would be a command-line interface to turn it on and configure it, i.e., something like
watchdog on 5000 # Set timeout to 5000 ms watchdog off # Turn off (if possible)
// Simon
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Tuesday, September 29, 2009 1:59 PM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH] arm:kirkwood: Add hardware watchdog support for Marvell Kirkwood boards
On Mon, 28 Sep 2009 19:16:16 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
But do you really need Watchdog support for u-boot?
Paranoia really has no limits :-). The main objective for me personally is to have the watchdog on when Linux starts, but if there is a risk (for whatever reason) that U-boot hangs, it would also help there.
Its good to have watchdog, it will be very useful for some applications. But I don't think we should do it at u-boot level. Secondly If it is supported on Kirkwood platforms in Linux, then the same can be triggered from OS too.
In u-boot I didn't find much watchdog implementation for other arm architectures.
Because if you want to use the watchdog, you will need to
keep it running in
Entire code.
That's fine, the code is already sprinkled with WATCHDOG_RESET() in many places (which calls hw_watchdog_reset()). Also note that this patch doesn't actually turn it on, you'll have to call kw_watchdog_init() first to do that.
newly added code for Kirkwood may not, we need to check and add
Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c
and its implementation,
This will be a good way of implementation.
Well, I did look at that, and I believe the implementation is fairly similar.
I think you should follow the same method to keep it as "add Kirkwood watchdog driver"
What I wonder about in that context is the use of hw_watchdog_init(). I first thought this was generic, but it's not exported via watchdog.h (like hw_watchdog_reset()). I think it would be nice to have a generic interface which exports
void hw_watchdog_init(unsigned long timeout_ms);
to initialize the watchdog and timeout. The timeout would be a bit crude since hardware have limits to how long the timeouts would be, but anyway.
Another good feature would be a command-line interface to turn it on and configure it, i.e., something like
watchdog on 5000 # Set timeout to 5000 ms watchdog off # Turn off (if possible)
These are good enhancement, you may post these generic patches too :-)
Regards.. Prafulla . .
// Simon
On Tuesday 29 September 2009 15:45:44 Prafulla Wadaskar wrote:
But do you really need Watchdog support for u-boot?
Paranoia really has no limits :-). The main objective for me personally is to have the watchdog on when Linux starts, but if there is a risk (for whatever reason) that U-boot hangs, it would also help there.
Its good to have watchdog, it will be very useful for some applications. But I don't think we should do it at u-boot level.
I think it's perfectly legal to enable the watchdog already in U-Boot. And as Simon already mentioned, it has some advantages doing this as early as possible.
Secondly If it is supported on Kirkwood platforms in Linux, then the same can be triggered from OS too.
But things could go wrong while booting Linux.
In u-boot I didn't find much watchdog implementation for other arm architectures.
It doesn't matter if there are many, but there are at least some. And the infrastructure is there. Why not use it. You don't have to enable the watchdog support for your boards of course. :)
Cheers, Stefan
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
On Tue, 29 Sep 2009 06:45:44 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
The main objective for me personally is to have the watchdog on when Linux starts, but if there is a risk (for whatever reason) that U-boot hangs, it would also help there.
Its good to have watchdog, it will be very useful for some applications. But I don't think we should do it at u-boot level. Secondly If it is supported on Kirkwood platforms in Linux, then the same can be triggered from OS too.
Sure, it's just nice to have it running when Linux is started, so that the board is properly rebooted if it hangs during kernel startup. And in the same way as a watchdog can be useful for other boards, it should be useful for Kirkwood-based ones.
In u-boot I didn't find much watchdog implementation for other arm architectures.
Well, you pointed at at91sam9_wdt.c, which I believe is for an ARM :-)
That's fine, the code is already sprinkled with WATCHDOG_RESET() in many places (which calls hw_watchdog_reset()). Also note that this patch doesn't actually turn it on, you'll have to call kw_watchdog_init() first to do that.
newly added code for Kirkwood may not, we need to check and add
It works fine for me at least. I don't think any of the kirkwood code has delays of several seconds so far :-). And again, boards which don't need it or don't want to use it doesn't need to turn the watchdog on - it will simply be compiled out in that case.
Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c
and its implementation,
This will be a good way of implementation.
Well, I did look at that, and I believe the implementation is fairly similar.
I think you should follow the same method to keep it as "add Kirkwood watchdog driver"
Sorry, I'm not sure what this comment means. Do you mean to move it out of timer.c? I put it there since it uses the timer registers and is really just a special use of the built-in timer support.
// Simon
Hi again Prafulla and the list!
On Mon, 28 Sep 2009 09:06:26 +0200 Simon Kagstrom simon.kagstrom@netinsight.net wrote:
Initialize by calling kw_watchdog_init() with the number of seconds for the watchdog to timeout.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
Were there any particular problems with this patch that I should rework? It's not enabled by default.
// Simon
cpu/arm926ejs/kirkwood/timer.c | 29 +++++++++++++++++++++++++++++ include/asm-arm/arch-kirkwood/cpu.h | 2 ++ 2 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/cpu/arm926ejs/kirkwood/timer.c b/cpu/arm926ejs/kirkwood/timer.c index 817ff42..f3397e7 100644 --- a/cpu/arm926ejs/kirkwood/timer.c +++ b/cpu/arm926ejs/kirkwood/timer.c @@ -25,6 +25,7 @@ #include <asm/arch/kirkwood.h>
#define UBOOT_CNTR 0 /* counter to use for uboot timer */ +#define WATCHDOG_CNTR 2
/* Timer reload and current value registers */ struct kwtmr_val { @@ -166,3 +167,31 @@ int timer_init(void)
return 0; }
+#if defined(CONFIG_HW_WATCHDOG) +static unsigned long watchdog_timeout = 5; +void hw_watchdog_reset(void) +{
- unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
- writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
+}
+void kw_watchdog_init(unsigned long timeout_secs) +{
- struct kwcpu_registers *cpureg =
(struct kwcpu_registers *)KW_CPU_REG_BASE;- unsigned int cntmrctrl;
- watchdog_timeout = timeout_secs;
- /* Enable CPU reset if watchdog expires */
- cpureg->rstoutn_mask |= WATCHDOG_CNTR;
- hw_watchdog_reset();
- /* Enable the watchdog */
- cntmrctrl = readl(CNTMR_CTRL_REG);
- cntmrctrl |= CTCR_ARM_TIMER_EN(WATCHDOG_CNTR);
- cntmrctrl |= CTCR_ARM_TIMER_AUTO_EN(WATCHDOG_CNTR);
- writel(cntmrctrl, CNTMR_CTRL_REG);
+} +#endif diff --git a/include/asm-arm/arch-kirkwood/cpu.h b/include/asm-arm/arch-kirkwood/cpu.h index b3022a3..df49c3f 100644 --- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7, unsigned int mpp8_15, unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55); unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs);
#endif /* __ASSEMBLY__ */ #endif /* _KWCPU_H */
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Wednesday, October 28, 2009 1:47 PM To: Prafulla Wadaskar; u-boot@lists.denx.de Subject: Re: [PATCH] arm:kirkwood: Add hardware watchdog support for Marvell Kirkwood boards
Hi again Prafulla and the list!
On Mon, 28 Sep 2009 09:06:26 +0200 Simon Kagstrom simon.kagstrom@netinsight.net wrote:
Initialize by calling kw_watchdog_init() with the number of
seconds for
the watchdog to timeout.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
Were there any particular problems with this patch that I should rework? It's not enabled by default.
Hi Simon We can enable this support Please see my in lined comments below
// Simon
cpu/arm926ejs/kirkwood/timer.c | 29
+++++++++++++++++++++++++++++
include/asm-arm/arch-kirkwood/cpu.h | 2 ++ 2 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/cpu/arm926ejs/kirkwood/timer.c
b/cpu/arm926ejs/kirkwood/timer.c
index 817ff42..f3397e7 100644 --- a/cpu/arm926ejs/kirkwood/timer.c +++ b/cpu/arm926ejs/kirkwood/timer.c @@ -25,6 +25,7 @@ #include <asm/arch/kirkwood.h>
#define UBOOT_CNTR 0 /* counter to use for uboot timer */ +#define WATCHDOG_CNTR 2
BTW, this declaration will not be required if you see struct kwtmr_register
/* Timer reload and current value registers */ struct kwtmr_val { @@ -166,3 +167,31 @@ int timer_init(void)
return 0; }
+#if defined(CONFIG_HW_WATCHDOG) +static unsigned long watchdog_timeout = 5;
Please get rid of this magic number, Pls provide some comments I think just u8 are sufficient here since the time is in seconds. I suggest variable name as "wdt_tout" to keep it small
Some comments for function...
+void hw_watchdog_reset(void) +{
- unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
Please use u32 here to avoid typecast in writel Pls provide comments for this calculations
- writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
Please check struct kwtmr_registers, wdt regs are named differently, pls use them
+}
+void kw_watchdog_init(unsigned long timeout_secs) +{
- struct kwcpu_registers *cpureg =
(struct kwcpu_registers *)KW_CPU_REG_BASE;- unsigned int cntmrctrl;
- watchdog_timeout = timeout_secs;
- /* Enable CPU reset if watchdog expires */
- cpureg->rstoutn_mask |= WATCHDOG_CNTR;
access any arm registers through readl/writel only Using WATCHDOG_CNTR is confusing here, pls use something like this (1 << 1) (ref reset_cpu in cpu.c)
- hw_watchdog_reset();
- /* Enable the watchdog */
- cntmrctrl = readl(CNTMR_CTRL_REG);
- cntmrctrl |= CTCR_ARM_TIMER_EN(WATCHDOG_CNTR);
- cntmrctrl |= CTCR_ARM_TIMER_AUTO_EN(WATCHDOG_CNTR);
- writel(cntmrctrl, CNTMR_CTRL_REG);
This need to be updated as per struct kwtmr_register
+} +#endif diff --git a/include/asm-arm/arch-kirkwood/cpu.h
b/include/asm-arm/arch-kirkwood/cpu.h
index b3022a3..df49c3f 100644 --- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7,
unsigned int mpp8_15,
unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55);unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs);
Functions declared here are suppose to be in cpu.c Moreover I think we don't need this function at all, You can club kw_watchdog_init with hw_watchdog_reset so that at very first WATCHDOG_RESET() function call, watchdog timer it will be initialized.
Regards.. Prafulla . .
#endif /* __ASSEMBLY__ */ #endif /* _KWCPU_H */
Thanks for the comments!
On Wed, 28 Oct 2009 02:24:43 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
#define UBOOT_CNTR 0 /* counter to use for uboot timer */ +#define WATCHDOG_CNTR 2
BTW, this declaration will not be required if you see struct kwtmr_register
Well, to me it makes the code more clear, so I'd prefer to keep it.
/* Timer reload and current value registers */ struct kwtmr_val { @@ -166,3 +167,31 @@ int timer_init(void)
return 0; }
+#if defined(CONFIG_HW_WATCHDOG) +static unsigned long watchdog_timeout = 5;
Please get rid of this magic number, Pls provide some comments I think just u8 are sufficient here since the time is in seconds. I suggest variable name as "wdt_tout" to keep it small
I'll make it configurable through config.h, and a u8. However, I think watchdog_timeout is a more descriptive name here.
- writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
Please check struct kwtmr_registers, wdt regs are named differently, pls use them
I can do that, but CNTMR_VAL_REG is actually defined higher up in the file as
#define CNTMR_VAL_REG(tmrnum) &kwtmr_regs->tmr[tmrnum].val
and used for the regular timer support. I'm not sure I like that, but at least the file should be internally consistent.
--- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7,
unsigned int mpp8_15,
unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55);unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs);
Functions declared here are suppose to be in cpu.c Moreover I think we don't need this function at all, You can club kw_watchdog_init with hw_watchdog_reset so that at very first WATCHDOG_RESET() function call, watchdog timer it will be initialized.
But then it's unconditionally turned on as soon as the first WATCHDOG_RESET() is called, which might not be what you want.
In the long run, we should probably add command line support for enabling the watchdog (some might want to do it just before starting Linux for example).
// Simon
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Wednesday, October 28, 2009 3:23 PM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de Subject: Re: [PATCH] arm:kirkwood: Add hardware watchdog support for Marvell Kirkwood boards
Thanks for the comments!
On Wed, 28 Oct 2009 02:24:43 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
#define UBOOT_CNTR 0 /* counter to use for
uboot timer */
+#define WATCHDOG_CNTR 2
BTW, this declaration will not be required if you see
struct kwtmr_register
Well, to me it makes the code more clear, so I'd prefer to keep it.
So I would like to suggest rename it as WATCHDOG_TMR
/* Timer reload and current value registers */ struct kwtmr_val { @@ -166,3 +167,31 @@ int timer_init(void)
return 0; }
+#if defined(CONFIG_HW_WATCHDOG) +static unsigned long watchdog_timeout = 5;
Please get rid of this magic number, Pls provide some comments I think just u8 are sufficient here since the time is in seconds. I suggest variable name as "wdt_tout" to keep it small
I'll make it configurable through config.h, and a u8. However, I think watchdog_timeout is a more descriptive name here.
- writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
Please check struct kwtmr_registers, wdt regs are named
differently, pls use them
I can do that, but CNTMR_VAL_REG is actually defined higher up in the file as
#define CNTMR_VAL_REG(tmrnum) &kwtmr_regs->tmr[tmrnum].val
and used for the regular timer support. I'm not sure I like that, but at least the file should be internally consistent.
You can update the structure to use WDT timer in the same way as other timers, there is no sense putting additional names in structure.
--- a/include/asm-arm/arch-kirkwood/cpu.h +++ b/include/asm-arm/arch-kirkwood/cpu.h @@ -165,5 +165,7 @@ int kw_config_mpp(unsigned int mpp0_7,
unsigned int mpp8_15,
unsigned int mpp32_39, unsigned int mpp40_47, unsigned int mpp48_55);unsigned int kw_winctrl_calcsize(unsigned int sizeval); +void kw_watchdog_init(unsigned long timeout_secs);
Functions declared here are suppose to be in cpu.c Moreover I think we don't need this function at all, You can club kw_watchdog_init with hw_watchdog_reset so
that at very first WATCHDOG_RESET() function call,
watchdog timer it will be initialized.
But then it's unconditionally turned on as soon as the first WATCHDOG_RESET() is called, which might not be what you want.
In the long run, we should probably add command line support for enabling the watchdog (some might want to do it just before starting Linux for example).
You can even call WATCHDOG_RESET() from wherever from your code to enable it
Regards.. Prafulla . .
// Simon
On Wed, 28 Oct 2009 04:34:10 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
#define CNTMR_VAL_REG(tmrnum) &kwtmr_regs->tmr[tmrnum].val
and used for the regular timer support. I'm not sure I like that, but at least the file should be internally consistent.
You can update the structure to use WDT timer in the same way as other timers, there is no sense putting additional names in structure.
But I'm not - the WDT timer is used in the same way as the other timer. The only difference is the added WATCHDOG_TMR define which specifies which Kirkwood timer to use as a watchdog.
But then it's unconditionally turned on as soon as the first WATCHDOG_RESET() is called, which might not be what you want.
In the long run, we should probably add command line support for enabling the watchdog (some might want to do it just before starting Linux for example).
You can even call WATCHDOG_RESET() from wherever from your code to enable it
Sure, but WATCHDOG_RESET() will be called anyway (and probably before my code), so it will be enabled anyhow in that case. My point is that sometimes you don't want the watchdog to get started directly, hence the function to enable it.
// Simon
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Wednesday, October 28, 2009 6:14 PM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de Subject: Re: [PATCH] arm:kirkwood: Add hardware watchdog support for Marvell Kirkwood boards
On Wed, 28 Oct 2009 04:34:10 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
#define CNTMR_VAL_REG(tmrnum) &kwtmr_regs->tmr[tmrnum].val
and used for the regular timer support. I'm not sure I
like that, but
at least the file should be internally consistent.
You can update the structure to use WDT timer in the same
way as other timers,
there is no sense putting additional names in structure.
But I'm not - the WDT timer is used in the same way as the other timer.
So the kwtmr_register structure clean up can be a separate patch.
The only difference is the added WATCHDOG_TMR define which specifies which Kirkwood timer to use as a watchdog.
Ack
But then it's unconditionally turned on as soon as the first WATCHDOG_RESET() is called, which might not be what you want.
In the long run, we should probably add command line support for enabling the watchdog (some might want to do it just
before starting
Linux for example).
You can even call WATCHDOG_RESET() from wherever from your
code to enable it
Sure, but WATCHDOG_RESET() will be called anyway (and probably before my code), so it will be enabled anyhow in that case. My point is that sometimes you don't want the watchdog to get started directly, hence the function to enable it.
That is also valid point, This will be the generic need for all architectures. Lets introduce WATCHDOG_INIT() as new generic interface.
What do you think?
Regards.. Prafulla . .
// Simon
On Wed, 28 Oct 2009 05:57:34 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
Sure, but WATCHDOG_RESET() will be called anyway (and probably before my code), so it will be enabled anyhow in that case. My point is that sometimes you don't want the watchdog to get started directly, hence the function to enable it.
That is also valid point, This will be the generic need for all architectures. Lets introduce WATCHDOG_INIT() as new generic interface.
Yes, something like that. What I was thinking was a
void watchdog_enable(unsigned int timeout_secs);
void watchdog_disable(void);
and a command-line interface to go with these. I'm cooking up a patch with this.
// Simon
participants (4)
-
Prafulla Wadaskar -
Simon Kagstrom -
Stefan Roese -
Tom