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