> -----Original Message----- > From: Simon Kagstrom [mailto:simon.kagst...@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.kagst...@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.kagst...@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 */ > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot