[U-Boot] [PATCH] arm:kirkwood: Add hardware watchdog support for Marvell Kirkwood boards

[Prafulla Wadaskar]

> -----Original Message-----
> From: Simon Kagstrom [mailto:simon.kagstrom at netinsight.net] 
> Sent: Wednesday, October 28, 2009 1:47 PM
> To: Prafulla Wadaskar; u-boot at 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 at 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 at 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 */
> 
>