-----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 */