[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 3:23 PM
> To: Prafulla Wadaskar
> Cc: u-boot at 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 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.

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
>