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

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

[Simon Kagstrom]

Initialize by calling kw_watchdog_init() with the number of seconds for
the watchdog to timeout.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 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
 
 /* 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;
+void hw_watchdog_reset(void)
+{
+	unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
+
+	writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
+}
+
+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;
+	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);
+}
+#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);
+
 #endif /* __ASSEMBLY__ */
 #endif /* _KWCPU_H */
-- 
1.6.0.4

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

[Tom]

Simon Kagstrom wrote:
> Initialize by calling kw_watchdog_init() with the number of seconds for
> the watchdog to timeout.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  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
>  
>  /* 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;
> +void hw_watchdog_reset(void)
> +{
> +	unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
> +
> +	writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
> +}
> +
> +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;
> +	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);
> +}
> +#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);
> +

You should add hw_watchdog_reset to H the file or declare it static.

You may want to add #define stubs to handle the ifndef CONFIG_HW_WATCHDOG.

Tom

>  #endif /* __ASSEMBLY__ */
>  #endif /* _KWCPU_H */

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

[Simon Kagstrom]

On Mon, 28 Sep 2009 07:36:32 -0500
Tom <Tom.Rix@windriver.com> wrote:

> > +void hw_watchdog_reset(void)
> > +{
> > +	unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
>
> > 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);
> 
> You should add hw_watchdog_reset to H the file or declare it static.

It's a public interface and defined in  include/watchdog.h (this is
what's being called by WATCHDOG_RESET), but I'll submit a new patch
which includes this.

> You may want to add #define stubs to handle the ifndef CONFIG_HW_WATCHDOG.

I'm afraid I don't understand this comment though, what is the
suggestion here?

// Simon

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

[Tom]

Simon Kagstrom wrote:
> On Mon, 28 Sep 2009 07:36:32 -0500
> Tom <Tom.Rix@windriver.com> wrote:
> 
>>> +void hw_watchdog_reset(void)
>>> +{
>>> +	unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
>>> 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);
>> You should add hw_watchdog_reset to H the file or declare it static.
> 
> It's a public interface and defined in  include/watchdog.h (this is
> what's being called by WATCHDOG_RESET), but I'll submit a new patch
> which includes this.

Yes thanks.  My mistake.
A new patch is not needed.

> 
>> You may want to add #define stubs to handle the ifndef CONFIG_HW_WATCHDOG.
> 
> I'm afraid I don't understand this comment though, what is the
> suggestion here?
> 
> // Simon

Something like this is already being done in watchdog.h
So it it not needed.

#ifdef CONFIG_HW_WATCHDOG
	#if defined(__ASSEMBLY__)
		#define WATCHDOG_RESET bl hw_watchdog_reset
	#else
		extern void hw_watchdog_reset(void);

		#define WATCHDOG_RESET hw_watchdog_reset
	#endif /* __ASSEMBLY__ */
#else

<snip>
		/*
		 * No hardware or software watchdog.
		 */
		#if defined(__ASSEMBLY__)
			#define WATCHDOG_RESET /*XXX DO_NOT_DEL_THIS_COMMENT*/
		#else
----> stub 		#define WATCHDOG_RESET() {}
		#endif /* __ASSEMBLY__ */
	#endif /* CONFIG_WATCHDOG && !__ASSEMBLY__ */
#endif /* CONFIG_HW_WATCHDOG */

Thanks for making your changes clear to me.
Ack-ed

Tom

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

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Simon Kagstrom
> Sent: Monday, September 28, 2009 12:36 PM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH] arm:kirkwood: Add hardware watchdog 
> support for Marvell Kirkwood boards
> 
> Initialize by calling kw_watchdog_init() with the number of 
> seconds for
> the watchdog to timeout.

Hi Simon
It is good to have more support available for Kirkwood and thanks for your efforts.

But do you really need Watchdog support for u-boot?
Because if you want to use the watchdog, you will need to keep it running in
Entire code.

May you pls explain your objective for enabling it?

Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c and its implementation,
This will be a good way of implementation.

Regards..
Prafulla . .

Regards..
Prafulla . .

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

[Simon Kagstrom]

On Mon, 28 Sep 2009 19:16:16 -0700
Prafulla Wadaskar <prafulla@marvell.com> wrote:

> But do you really need Watchdog support for u-boot?

Paranoia really has no limits :-). The main objective for me personally
is to have the watchdog on when Linux starts, but if there is a risk
(for whatever reason) that U-boot hangs, it would also help there.

> Because if you want to use the watchdog, you will need to keep it running in
> Entire code.

That's fine, the code is already sprinkled with WATCHDOG_RESET() in
many places (which calls hw_watchdog_reset()). Also note that this
patch doesn't actually turn it on, you'll have to call
kw_watchdog_init() first to do that.

> Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c and its implementation,
> This will be a good way of implementation.

Well, I did look at that, and I believe the implementation is fairly
similar.

What I wonder about in that context is the use of hw_watchdog_init(). I
first thought this was generic, but it's not exported via watchdog.h
(like hw_watchdog_reset()). I think it would be nice to have a generic
interface which exports

  void hw_watchdog_init(unsigned long timeout_ms);

to initialize the watchdog and timeout. The timeout would be a bit
crude since hardware have limits to how long the timeouts would be, but
anyway.


Another good feature would be a command-line interface to turn it on
and configure it, i.e., something like

  watchdog on 5000   # Set timeout to 5000 ms
  watchdog off       # Turn off (if possible)

// Simon

[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: Tuesday, September 29, 2009 1:59 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] arm:kirkwood: Add hardware 
> watchdog support for Marvell Kirkwood boards
> 
> On Mon, 28 Sep 2009 19:16:16 -0700
> Prafulla Wadaskar <prafulla@marvell.com> wrote:
> 
> > But do you really need Watchdog support for u-boot?
> 
> Paranoia really has no limits :-). The main objective for me 
> personally
> is to have the watchdog on when Linux starts, but if there is a risk
> (for whatever reason) that U-boot hangs, it would also help there.

Its good to have watchdog, it will be very useful for some applications.
But I don't think we should do it at u-boot level.
Secondly If it is supported on Kirkwood platforms in Linux,
then the same can be triggered from OS too.

In u-boot I didn't find much watchdog implementation for other arm architectures.

> 
> > Because if you want to use the watchdog, you will need to 
> keep it running in
> > Entire code.
> 
> That's fine, the code is already sprinkled with WATCHDOG_RESET() in
> many places (which calls hw_watchdog_reset()). Also note that this
> patch doesn't actually turn it on, you'll have to call
> kw_watchdog_init() first to do that.

newly added code for Kirkwood may not, we need to check and add

> 
> > Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c 
> and its implementation,
> > This will be a good way of implementation.
> 
> Well, I did look at that, and I believe the implementation is fairly
> similar.

I think you should follow the same method to keep it as "add Kirkwood watchdog driver"

> 
> What I wonder about in that context is the use of 
> hw_watchdog_init(). I
> first thought this was generic, but it's not exported via watchdog.h
> (like hw_watchdog_reset()). I think it would be nice to have a generic
> interface which exports
> 
>   void hw_watchdog_init(unsigned long timeout_ms);
> 
> to initialize the watchdog and timeout. The timeout would be a bit
> crude since hardware have limits to how long the timeouts 
> would be, but
> anyway.
> 
> 
> Another good feature would be a command-line interface to turn it on
> and configure it, i.e., something like
> 
>   watchdog on 5000   # Set timeout to 5000 ms
>   watchdog off       # Turn off (if possible)
> 

These are good enhancement, you may post these generic patches too :-)

Regards..
Prafulla . .


> // Simon
> 

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

[Stefan Roese]

On Tuesday 29 September 2009 15:45:44 Prafulla Wadaskar wrote:
> > > But do you really need Watchdog support for u-boot?
> >
> > Paranoia really has no limits :-). The main objective for me
> > personally
> > is to have the watchdog on when Linux starts, but if there is a risk
> > (for whatever reason) that U-boot hangs, it would also help there.
> 
> Its good to have watchdog, it will be very useful for some applications.
> But I don't think we should do it at u-boot level.

I think it's perfectly legal to enable the watchdog already in U-Boot. And as 
Simon already mentioned, it has some advantages doing this as early as 
possible.

> Secondly If it is supported on Kirkwood platforms in Linux,
> then the same can be triggered from OS too.

But things could go wrong while booting Linux.
 
> In u-boot I didn't find much watchdog implementation for other arm
>  architectures.

It doesn't matter if there are many, but there are at least some. And the 
infrastructure is there. Why not use it. You don't have to enable the watchdog 
support for your boards of course. :)
 
Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

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

[Simon Kagstrom]

On Tue, 29 Sep 2009 06:45:44 -0700
Prafulla Wadaskar <prafulla@marvell.com> wrote:

> > The main objective for me personally is to have the watchdog on when Linux starts,
> > but if there is a risk (for whatever reason) that U-boot hangs, it would also help there.
> 
> Its good to have watchdog, it will be very useful for some applications.
> But I don't think we should do it at u-boot level.
> Secondly If it is supported on Kirkwood platforms in Linux,
> then the same can be triggered from OS too.

Sure, it's just nice to have it running when Linux is started, so that
the board is properly rebooted if it hangs during kernel startup. And
in the same way as a watchdog can be useful for other boards, it should
be useful for Kirkwood-based ones.

> In u-boot I didn't find much watchdog implementation for other arm architectures.

Well, you pointed at at91sam9_wdt.c, which I believe is for an ARM :-)

> > That's fine, the code is already sprinkled with WATCHDOG_RESET() in
> > many places (which calls hw_watchdog_reset()). Also note that this
> > patch doesn't actually turn it on, you'll have to call
> > kw_watchdog_init() first to do that.
> 
> newly added code for Kirkwood may not, we need to check and add

It works fine for me at least. I don't think any of the kirkwood code
has delays of several seconds so far :-). And again, boards which don't
need it or don't want to use it doesn't need to turn the watchdog on -
it will simply be compiled out in that case.

> > > Secondly Pls have a look at drivers/watchdog/at91sam9_wdt.c 
> > and its implementation,
> > > This will be a good way of implementation.
> > 
> > Well, I did look at that, and I believe the implementation is fairly
> > similar.
> 
> I think you should follow the same method to keep it as "add Kirkwood watchdog driver"

Sorry, I'm not sure what this comment means. Do you mean to move it out
of timer.c? I put it there since it uses the timer registers and is
really just a special use of the built-in timer support.

// Simon

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

[Simon Kagstrom]

Hi again Prafulla and the list!

On Mon, 28 Sep 2009 09:06:26 +0200
Simon Kagstrom <simon.kagstrom@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@netinsight.net>

Were there any particular problems with this patch that I should
rework? It's not enabled by default.

// 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
>  
>  /* 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;
> +void hw_watchdog_reset(void)
> +{
> +	unsigned long time = CONFIG_SYS_TCLK * watchdog_timeout;
> +
> +	writel(time, CNTMR_VAL_REG(WATCHDOG_CNTR));
> +}
> +
> +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;
> +	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);
> +}
> +#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);
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* _KWCPU_H */

[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@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@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] [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@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

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

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

[Simon Kagstrom]

On Wed, 28 Oct 2009 04:34:10 -0700
Prafulla Wadaskar <prafulla@marvell.com> wrote:

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

But I'm not - the WDT timer is used in the same way as the other timer.
The only difference is the added WATCHDOG_TMR define which specifies
which Kirkwood timer to use as a watchdog.

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

Sure, but WATCHDOG_RESET() will be called anyway (and probably before
my code), so it will be enabled anyhow in that case. My point is that
sometimes you don't want the watchdog to get started directly, hence
the function to enable it.

// Simon

[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 6:14 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de
> Subject: Re: [PATCH] arm:kirkwood: Add hardware watchdog 
> support for Marvell Kirkwood boards
> 
> On Wed, 28 Oct 2009 04:34:10 -0700
> Prafulla Wadaskar <prafulla@marvell.com> wrote:
> 
> > >    #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.
> 
> But I'm not - the WDT timer is used in the same way as the 
> other timer.

So the kwtmr_register structure clean up can be a separate patch.

> The only difference is the added WATCHDOG_TMR define which specifies
> which Kirkwood timer to use as a watchdog.

Ack

> 
> > > 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
> 
> Sure, but WATCHDOG_RESET() will be called anyway (and probably before
> my code), so it will be enabled anyhow in that case. My point is that
> sometimes you don't want the watchdog to get started directly, hence
> the function to enable it.

That is also valid point,
This will be the generic need for all architectures.
Lets introduce WATCHDOG_INIT() as new generic interface.

What do you think?

Regards..
Prafulla . .

> 
> // Simon
> 

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

[Simon Kagstrom]

On Wed, 28 Oct 2009 05:57:34 -0700
Prafulla Wadaskar <prafulla@marvell.com> wrote:

> > Sure, but WATCHDOG_RESET() will be called anyway (and probably before
> > my code), so it will be enabled anyhow in that case. My point is that
> > sometimes you don't want the watchdog to get started directly, hence
> > the function to enable it.
> 
> That is also valid point,
> This will be the generic need for all architectures.
> Lets introduce WATCHDOG_INIT() as new generic interface.

Yes, something like that. What I was thinking was a 

   void watchdog_enable(unsigned int timeout_secs);

   void watchdog_disable(void);

and a command-line interface to go with these. I'm cooking up a patch
with this.

// Simon