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

[Simon Kagstrom]

Thanks for the comments!

On Wed, 28 Oct 2009 02:24:43 -0700
Prafulla Wadaskar <prafulla at 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