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