Driver to support the internal watchdog peripheral found on Atmel's
AT91SAM9 and AT91CAP9 processors (ARM).
Signed-off-by: Renaud Cerrato <[email protected]>
Signed-off-by: Andrew Victor <[email protected]>
> +static long at91_wdt_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + void __user *argp = (void __user *)arg;
> + int __user *p = argp;
No locking.. so you could get two set timeout calls in parallel. Probably
you need a simple mutex in at91_wdt_settimeout();
> + res = misc_register(&at91wdt_miscdev);
> + if (res)
> + return res;
> +
> + /* Set watchdog */
> + if (at91_wdt_settimeout(wdt_timeout) == -EINVAL) {
> + pr_info("at91sam9_wdt: timeout must be between 1 and %d.\n",
> + WDT_MAX_TIME);
> + return 0;
At the moment those two are safe. When the open lock_kernel
goes away it will be possible to get
misc_register
open
ioctl
wdt_settimout()
So you may want to swap those two around (and disable the timer if the
register fails ?), or lock the open against the register routine.
Otherwise looks good.
Alan
On Sun, 1 Jun 2008 18:40:46 +0200
"Andrew Victor" <[email protected]> wrote:
> +static int __init at91wdt_probe(struct platform_device *pdev)
> +{
> + int res;
> +
> + if (at91wdt_miscdev.parent)
> + return -EBUSY;
> + at91wdt_miscdev.parent = &pdev->dev;
> +
> + res = misc_register(&at91wdt_miscdev);
> + if (res)
> + return res;
> +
> + /* Set watchdog */
> + if (at91_wdt_settimeout(wdt_timeout) == -EINVAL) {
> + pr_info("at91sam9_wdt: timeout must be between 1 and %d.\n",
> + WDT_MAX_TIME);
Would printk(KERN_ERR be more appropriate here?
> + return 0;
That looks like the wrong return value?
> + }
> +
> + return 0;
> +}
hi Andrew,
>> + return 0;
>
> That looks like the wrong return value?
Since the watchdog registers are Write-Once, if the user does not
specify a "wdt_timeout" parameter we'd still like the driver to load
even if it's not enabled.
The user can then always use ioctl(WDIOC_SETTIMEOUT:) to set the timeout later.
Regards,
Andrew Victor
On Wed, 4 Jun 2008 09:31:50 +0200
"Andrew Victor" <[email protected]> wrote:
> hi Andrew,
>
> >> + return 0;
> >
> > That looks like the wrong return value?
>
> Since the watchdog registers are Write-Once, if the user does not
> specify a "wdt_timeout" parameter we'd still like the driver to load
> even if it's not enabled.
Surely you can avoid doing the register writes until after you check the
validity of arguments ?
hi Alan,
>> Since the watchdog registers are Write-Once, if the user does not
>> specify a "wdt_timeout" parameter we'd still like the driver to load
>> even if it's not enabled.
>
> Surely you can avoid doing the register writes until after you check the
> validity of arguments ?
The validity of the timeout value is checked in at91_wdt_settimeout().
It returns -EINVAL if the timeout value is invalid.
The issue above is rather that probe() should not fail if
at91_wdt_settimeout() returns -EINVAL, since we'd like the
driver/module to still load to allow the user to later specify a valid
timeout via ioctl(WDIOC_SETTIMEOUT).
Regards,
Andrew Victor
> The issue above is rather that probe() should not fail if
> at91_wdt_settimeout() returns -EINVAL, since we'd like the
> driver/module to still load to allow the user to later specify a valid
> timeout via ioctl(WDIOC_SETTIMEOUT).
Every other watchdog behaves as follows
- If you specify a bogus value it doesn't load
- If you specify no value you get a valid default
- If you specify a valid value you get that
I don't believe yours should be different.
Alan
hi Alan ,
> Every other watchdog behaves as follows
>
> - If you specify a bogus value it doesn't load
> - If you specify no value you get a valid default
> - If you specify a valid value you get that
>
> I don't believe yours should be different.
I don't think any other in-kernel watchdog driver has to deal with
write-once hardware. On these processors once the watchdog register
is programmed, it cannot be disabled or re-programmed.
If the above behaviour is required, then we might aswell remove the
ioctl(WDIOC_SETTIMEOUT) interface for this driver since if the user
wants anything other than the default timeout they would need to pass
it via the kernel command-line or module parameters.
Regards,
Andrew Victor
> > - If you specify a bogus value it doesn't load
> > - If you specify no value you get a valid default
> > - If you specify a valid value you get that
> >
> > I don't believe yours should be different.
>
> I don't think any other in-kernel watchdog driver has to deal with
> write-once hardware. On these processors once the watchdog register
That would not be the case.
> is programmed, it cannot be disabled or re-programmed.
> If the above behaviour is required, then we might aswell remove the
> ioctl(WDIOC_SETTIMEOUT) interface for this driver since if the user
> wants anything other than the default timeout they would need to pass
> it via the kernel command-line or module parameters.
Actually quit a few of them deal with various hardware limits by using a
software timer to maintain the hardware timer poking.
Alan
I thought I'd finally get around to trying this driver.
No luck.
The proximate cause is that Atmel's second stage "at91boot"
loader disables the watchdog. That's because the reset state
of this watchdog is "reset after 16 seconds" ... so it must
be tended carefully until the Linux "watchdog" daemon takes
over. Since there's no code in at91boot, U-Boot, or Linux to
do that, it gets disabled. (How was this driver tested??)
Needless to say, this isn't exactly my model of how to make
a useful watchdog. That "write once" rule is pure trouble.
I suggest merging the following patch, to reduce confusion.
- Dave
======== CUT HERE
The at91sam9 watchdog is a kind of annoying bit of hardware: since its
mode register is write-once, it can't be reconfigured. Moreover, Atmel's
standard second stage loader "at91boot" always this watchdog (at least
on Atmel's reference boards), ensuring that Linux normally can't use it.
This patch removes some confusion by not registering the watchdog device
on systems where it was disabled before Linux starts.
Signed-off-by: David Brownell <[email protected]>
--- a/arch/arm/mach-at91/at91sam9260_devices.c 2008-06-06 15:00:06.000000000 -0700
+++ b/arch/arm/mach-at91/at91sam9260_devices.c 2008-06-06 20:40:38.000000000 -0700
@@ -21,6 +21,7 @@
#include <asm/arch/at91sam9260.h>
#include <asm/arch/at91sam9260_matrix.h>
#include <asm/arch/at91sam9_smc.h>
+#include <asm/arch/at91_wdt.h>
#include "generic.h"
@@ -666,6 +667,9 @@ static struct platform_device at91sam926
static void __init at91_add_device_watchdog(void)
{
+ /* WDT_MR is write-once; if it was disabled, we're stuck */
+ if (at91_sys_read(AT91_WDT_MR) & AT91_WDT_WDDIS)
+ return;
platform_device_register(&at91sam9260_wdt_device);
}
#else
--- a/arch/arm/mach-at91/at91sam9261_devices.c 2008-06-06 15:00:06.000000000 -0700
+++ b/arch/arm/mach-at91/at91sam9261_devices.c 2008-06-06 20:40:45.000000000 -0700
@@ -25,6 +25,7 @@
#include <asm/arch/at91sam9261.h>
#include <asm/arch/at91sam9261_matrix.h>
#include <asm/arch/at91sam9_smc.h>
+#include <asm/arch/at91_wdt.h>
#include "generic.h"
@@ -654,6 +655,9 @@ static struct platform_device at91sam926
static void __init at91_add_device_watchdog(void)
{
+ /* WDT_MR is write-once; if it was disabled, we're stuck */
+ if (at91_sys_read(AT91_WDT_MR) & AT91_WDT_WDDIS)
+ return;
platform_device_register(&at91sam9261_wdt_device);
}
#else
--- a/arch/arm/mach-at91/at91sam9263_devices.c 2008-06-06 15:00:06.000000000 -0700
+++ b/arch/arm/mach-at91/at91sam9263_devices.c 2008-06-06 20:40:48.000000000 -0700
@@ -24,6 +24,7 @@
#include <asm/arch/at91sam9263.h>
#include <asm/arch/at91sam9263_matrix.h>
#include <asm/arch/at91sam9_smc.h>
+#include <asm/arch/at91_wdt.h>
#include "generic.h"
@@ -911,6 +912,9 @@ static struct platform_device at91sam926
static void __init at91_add_device_watchdog(void)
{
+ /* WDT_MR is write-once; if it was disabled, we're stuck */
+ if (at91_sys_read(AT91_WDT_MR) & AT91_WDT_WDDIS)
+ return;
platform_device_register(&at91sam9263_wdt_device);
}
#else
--- a/arch/arm/mach-at91/at91sam9rl_devices.c 2008-06-06 15:00:06.000000000 -0700
+++ b/arch/arm/mach-at91/at91sam9rl_devices.c 2008-06-06 20:40:52.000000000 -0700
@@ -21,6 +21,7 @@
#include <asm/arch/at91sam9rl.h>
#include <asm/arch/at91sam9rl_matrix.h>
#include <asm/arch/at91sam9_smc.h>
+#include <asm/arch/at91_wdt.h>
#include "generic.h"
@@ -501,6 +502,9 @@ static struct platform_device at91sam9rl
static void __init at91_add_device_watchdog(void)
{
+ /* WDT_MR is write-once; if it was disabled, we're stuck */
+ if (at91_sys_read(AT91_WDT_MR) & AT91_WDT_WDDIS)
+ return;
platform_device_register(&at91sam9rl_wdt_device);
}
#else