[PATCH] Watchdog: Adding support for ARM Primecell SP805 Watchdog

Post by Viresh KUMAR
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0270b/DDI0270.pdf

Please include Wim Van Sebroeck for watchdog stuff - his address is in
MAINTAINERS. Please also ensure that in future you're sending your
patches to relevant people - there isn't the capacity to review
every bit of code on this list.

There is a script - scripts/get_maintainer.pl - to help with this.

Viresh KUMAR

2010-06-02 07:57:39 UTC

Post by Russell King - ARM Linux

Post by Viresh KUMAR
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0270b/DDI0270.pdf

Thanks Russell.

I will send patch to Wim Van.

viresh

Linus Walleij

2010-07-18 20:29:40 UTC

Sorry for late review.

I was bored so I compiled this driver in the latest -next
and tested on ARM PB1176JZF-S that I've brought for
entertainment this vacation...

First: add
#include <linux/slab.h>
or compilation fails on newer codebases.

Then I get this:
sp805-wdt dev:wdog: Clock not found
sp805-wdt dev:wdog: Probe Failed!!!
sp805-wdt: probe of dev:wdog failed with error -2

So I added it to the realviews, even though the clocking
of the watchdog is not mentioned in the reference manual
it appears to be using the same 24MHz clock as the UARTs
and the RTC. (ARM people, can you confirm this?)

After this it seems to work...

Post by Viresh KUMAR
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0270b/DDI0270.pdf
---
drivers/watchdog/Kconfig | 7 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/sp805_wdt.c | 386 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 394 insertions(+), 0 deletions(-)
create mode 100644 drivers/watchdog/sp805_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b87ba23..2759a91 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -73,6 +73,13 @@ config WM8350_WATCHDOG
# ARM Architecture
+config ARM_SP805_WATCHDOG
+ tristate "ARM SP805 Watchdog"

Why would you want this as a module... Can't it
be just bool?

Post by Viresh KUMAR
+ depends on ARM_AMBA
+ help
+ ARM Primecell SP805 Watchdog timer. This will reboot your system when
+ the timeout is reached.
+
config AT91RM9200_WATCHDOG
tristate "AT91RM9200 watchdog"
depends on ARCH_AT91RM9200
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5e3cb95..28af87a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
# ALPHA Architecture
# ARM Architecture
+obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
new file mode 100644
index 0000000..85d86ac
--- /dev/null
+++ b/drivers/watchdog/sp805_wdt.c
@@ -0,0 +1,386 @@
+/*
+ * drivers/char/watchdog/sp805-wdt.c
+ *
+ * Watchdog driver for ARM SP805 watchdog module
+ *
+ * Copyright (C) 2010 ST Microelectronics
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/device.h>
+#include <linux/resource.h>
+#include <linux/amba/bus.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>

#include <linux/slab.h>

Post by Viresh KUMAR
+
+/* default timeout in seconds */
+#define DEFAULT_TIMEOUT 60
+
+#define MODULE_NAME "sp805-wdt"
+
+/* watchdog register offsets and masks */
+#define WDTLOAD 0x000
+ #define LOAD_MIN 0x00000001
+ #define LOAD_MAX 0xFFFFFFFF

I haven't seen this style of indenting the #defines for
stuff you can put into the registers before, but I sort of
like it. Is it common in the kernel?

Post by Viresh KUMAR
+#define WDTVALUE 0x004
+#define WDTCONTROL 0x008
+ /* control register masks */
+ #define INT_ENABLE (1 << 0)
+ #define RESET_ENABLE (1 << 1)
+#define WDTINTCLR 0x00C
+#define WDTRIS 0x010
+#define WDTMIS 0x014
+ #define INT_MASK (1 << 0)
+#define WDTLOCK 0xC00
+ #define UNLOCK 0x1ACCE551
+ #define LOCK 0x00000001
+
+/**
+ * struct sp805_wdt: sp805 wdt device structure
+ *
+ * lock: spin lock protecting dev structure and io access
+ * base: base address of wdt
+ * clk: clock structure of wdt
+ * dev: amba device structure of wdt
+ * status: current status of wdt
+ * load_val: load value to be set for current timeout
+ * timeout: current programmed timeout
+ */
+struct sp805_wdt {
+ spinlock_t lock;
+ void __iomem *base;
+ struct clk *clk;
+ struct amba_device *adev;
+ unsigned long status;
+ #define WDT_BUSY 0
+ #define WDT_CAN_BE_CLOSED 1
+ unsigned int load_val;
+ unsigned int timeout;
+};
+
+/* local variables */
+static struct sp805_wdt *wdt;
+static int nowayout = WATCHDOG_NOWAYOUT;
+
+/* This routine finds load value that will reset system in required timout */
+static void wdt_setload(unsigned int timeout)
+{
+ u64 load, rate;
+
+ rate = clk_get_rate(wdt->clk);
+
+ /*
+ * sp805 runs counter with given value twice, after the end of first
+ * counter it gives an interrupt and then starts counter again. If
+ * interrupt already occured then it resets the system. This is why
+ * load is half of what should be required.
+ */
+ load = div_u64(rate, 2) * timeout - 1;
+
+ load = (load > LOAD_MAX) ? LOAD_MAX : load;
+ load = (load < LOAD_MIN) ? LOAD_MIN : load;
+
+ spin_lock(&wdt->lock);
+ wdt->load_val = load;
+ /* roundup timeout to closest positive integer value */
+ wdt->timeout = div_u64((load + 1) * 2 + (rate / 2), rate);

Look in linux/kernel.h, use the
DIV_ROUND_CLOSEST() macro instead of this.

Post by Viresh KUMAR
+ spin_unlock(&wdt->lock);
+}
+
+/* returns number of seconds left for reset to occur */
+static u32 wdt_timeleft(void)
+{
+ u64 load, rate;
+
+ rate = clk_get_rate(wdt->clk);
+
+ spin_lock(&wdt->lock);
+ load = readl(wdt->base + WDTVALUE);
+
+ /*If the interrupt is inactive then time left is WDTValue + WDTLoad. */
+ if (!(readl(wdt->base + WDTRIS) & INT_MASK))
+ load += wdt->load_val + 1;
+ spin_unlock(&wdt->lock);
+
+ return div_u64(load, rate);
+}
+
+/* enables watchdog timers reset */
+static void wdt_enable(void)
+{
+ spin_lock(&wdt->lock);
+
+ writel(UNLOCK, wdt->base + WDTLOCK);
+ writel(wdt->load_val, wdt->base + WDTLOAD);
+ writel(INT_MASK, wdt->base + WDTINTCLR);
+ writel(INT_ENABLE | RESET_ENABLE, wdt->base + WDTCONTROL);
+ writel(LOCK, wdt->base + WDTLOCK);
+
+ spin_unlock(&wdt->lock);
+}
+
+/* disables watchdog timers reset */
+static void wdt_disable(void)
+{
+ spin_lock(&wdt->lock);
+
+ writel(UNLOCK, wdt->base + WDTLOCK);
+ writel(0, wdt->base + WDTCONTROL);
+ writel(0, wdt->base + WDTLOAD);
+ writel(LOCK, wdt->base + WDTLOCK);
+
+ spin_unlock(&wdt->lock);
+}
+
+static ssize_t sp805_wdt_write(struct file *file, const char *data,
+ size_t len, loff_t *ppos)
+{
+ if (len) {
+ if (!nowayout) {
+ size_t i;
+
+ clear_bit(WDT_CAN_BE_CLOSED, &wdt->status);
+
+ for (i = 0; i != len; i++) {
+ char c;
+
+ if (get_user(c, data + i))
+ return -EFAULT;
+ /* Check for Magic Close character */
+ if (c == 'V') {
+ set_bit(WDT_CAN_BE_CLOSED,
+ &wdt->status);
+ break;
+ }
+ }
+ }
+ wdt_enable();
+ }
+ return len;
+}
+
+static const struct watchdog_info ident = {
+ .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+ .identity = MODULE_NAME,
+};
+
+static long sp805_wdt_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret = -ENOTTY;
+ unsigned int timeout;
+
+ switch (cmd) {
+ ret = copy_to_user((struct watchdog_info *)arg, &ident,
+ sizeof(ident)) ? -EFAULT : 0;
+ break;
+
+ ret = put_user(0, (int *)arg);
+ break;
+
+ wdt_enable();
+ ret = 0;
+ break;
+
+ ret = get_user(timeout, (unsigned int *)arg);
+ if (ret)
+ break;
+
+ wdt_setload(timeout);
+
+ wdt_enable();
+ /* Fall through */
+
+ ret = put_user(wdt->timeout, (unsigned int *)arg);
+ break;
+ ret = put_user(wdt_timeleft(), (unsigned int *)arg);
+ break;
+ }
+ return ret;
+}
+
+static int sp805_wdt_open(struct inode *inode, struct file *file)
+{
+ int ret = 0;
+
+ if (test_and_set_bit(WDT_BUSY, &wdt->status))
+ return -EBUSY;
+
+ ret = clk_enable(wdt->clk);
+ if (ret) {
+ dev_err(&wdt->adev->dev, "clock enable fail");
+ goto err;
+ }
+
+ wdt_enable();
+
+ /* can not be closed, once enabled */
+ clear_bit(WDT_CAN_BE_CLOSED, &wdt->status);
+ return nonseekable_open(inode, file);
+
+ clear_bit(WDT_BUSY, &wdt->status);
+ return ret;
+}
+
+static int sp805_wdt_release(struct inode *inode, struct file *file)
+{
+ if (!test_bit(WDT_CAN_BE_CLOSED, &wdt->status)) {
+ clear_bit(WDT_BUSY, &wdt->status);
+ dev_warn(&wdt->adev->dev, "Device closed unexpectedly\n");
+ return 0;
+ }
+
+ wdt_disable();
+ clk_disable(wdt->clk);
+ clear_bit(WDT_BUSY, &wdt->status);
+
+ return 0;
+}
+
+static const struct file_operations sp805_wdt_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .write = sp805_wdt_write,
+ .unlocked_ioctl = sp805_wdt_ioctl,
+ .open = sp805_wdt_open,
+ .release = sp805_wdt_release,
+};
+
+static struct miscdevice sp805_wdt_miscdev = {
+ .minor = WATCHDOG_MINOR,
+ .name = "watchdog",
+ .fops = &sp805_wdt_fops,
+};
+
+static int __devinit
+sp805_wdt_probe(struct amba_device *adev, struct amba_id *id)
+{
+ int ret = 0;
+
+ if (!request_mem_region(adev->res.start, resource_size(&adev->res),
+ "sp805_wdt")) {
+ dev_warn(&adev->dev, "Failed to get memory region resource\n");
+ ret = -ENOENT;
+ goto err;
+ }
+
+ wdt = kzalloc(sizeof(*wdt), GFP_KERNEL);
+ if (!wdt) {
+ dev_warn(&adev->dev, "Kzalloc failed\n");
+ ret = -ENOMEM;
+ goto err_kzalloc;
+ }
+
+ wdt->clk = clk_get(&adev->dev, NULL);
+ if (IS_ERR(wdt->clk)) {
+ dev_warn(&adev->dev, "Clock not found\n");
+ ret = PTR_ERR(wdt->clk);
+ goto err_clk_get;
+ }
+
+ wdt->base = ioremap(adev->res.start, resource_size(&adev->res));
+ if (!wdt->base) {
+ ret = -ENOMEM;
+ dev_warn(&adev->dev, "ioremap fail\n");
+ goto err_ioremap;
+ }
+
+ wdt->adev = adev;
+ spin_lock_init(&wdt->lock);
+ wdt_setload(DEFAULT_TIMEOUT);
+
+ ret = misc_register(&sp805_wdt_miscdev);
+ if (ret < 0) {
+ dev_warn(&adev->dev, "cannot register misc device\n");
+ goto err_misc_register;
+ }
+
+ dev_info(&adev->dev, "registration successful\n");
+ return 0;
+
+ iounmap(wdt->base);
+ clk_put(wdt->clk);
+ kfree(wdt);
+ wdt = NULL;
+ release_mem_region(adev->res.start, resource_size(&adev->res));
+ dev_err(&adev->dev, "Probe Failed!!!\n");

Drop the exclamation marks, that it's dev_err() is serious enough.

Post by Viresh KUMAR
+ return ret;
+}
+
+static int __devexit sp805_wdt_remove(struct amba_device *adev)
+{
+ misc_deregister(&sp805_wdt_miscdev);
+ iounmap(wdt->base);
+ clk_put(wdt->clk);
+ kfree(wdt);
+ release_mem_region(adev->res.start, resource_size(&adev->res));
+
+ return 0;
+}
+
+static struct amba_id sp805_wdt_ids[] __initdata = {
+ {
+ .id = 0x00141805,
+ .mask = 0x00ffffff,
+ },
+ { 0, 0 },
+};
+
+static struct amba_driver sp805_wdt_driver = {
+ .drv = {
+ .name = MODULE_NAME,
+ },
+ .id_table = sp805_wdt_ids,
+ .probe = sp805_wdt_probe,
+ .remove = __devexit_p(sp805_wdt_remove),
+};
+
+static int __init sp805_wdt_init(void)
+{
+ return amba_driver_register(&sp805_wdt_driver);
+}
+module_init(sp805_wdt_init);
+
+static void __exit sp805_wdt_exit(void)
+{
+ amba_driver_unregister(&sp805_wdt_driver);
+}
+module_exit(sp805_wdt_exit);
+
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout,
+ "Set to 1 to keep watchdog running after device release");
+
+MODULE_DESCRIPTION("ARM SP805 Watchdog Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Yours,
Linus Walleij

Viresh KUMAR

2010-07-19 05:07:10 UTC

Post by Linus Walleij
Sorry for late review.
I was bored so I compiled this driver in the latest -next
and tested on ARM PB1176JZF-S that I've brought for
entertainment this vacation...
First: add
#include <linux/slab.h>
or compilation fails on newer codebases.

OK.

Post by Linus Walleij
sp805-wdt dev:wdog: Clock not found
sp805-wdt dev:wdog: Probe Failed!!!
sp805-wdt: probe of dev:wdog failed with error -2
So I added it to the realviews, even though the clocking
of the watchdog is not mentioned in the reference manual
it appears to be using the same 24MHz clock as the UARTs
and the RTC. (ARM people, can you confirm this?)

Yes, it is using 24 Mhz on spear and is its functional clock.
So this time we need clock support in driver.

Post by Linus Walleij
After this it seems to work...

Why would you want this as a module... Can't it
be just bool?

I think watchdog can be present as a module. It can be inserted at
runtime. Other drivers in drivers/watchdog/Kconfig are also present
in modules.

#include <linux/slab.h>

OK.

I haven't seen this style of indenting the #defines for
stuff you can put into the registers before, but I sort of
like it. Is it common in the kernel?

I am not sure if it is common, but i thought it would be much more readable
now. so did it.

Look in linux/kernel.h, use the
DIV_ROUND_CLOSEST() macro instead of this.

OK.

Drop the exclamation marks, that it's dev_err() is serious enough.

OK.

I will send V2 after Wim's review comments.

Viresh KUMAR

2010-07-27 04:50:44 UTC

Post by Viresh KUMAR
+/* This routine finds load value that will reset system in required timout */

Post by Viresh KUMAR
+static void wdt_setload(unsigned int timeout)
+{
+ u64 load, rate;
+
+ rate = clk_get_rate(wdt->clk);
+
+ /*
+ * sp805 runs counter with given value twice, after the end of first
+ * counter it gives an interrupt and then starts counter again. If
+ * interrupt already occured then it resets the system. This is why
+ * load is half of what should be required.
+ */
+ load = div_u64(rate, 2) * timeout - 1;
+
+ load = (load > LOAD_MAX) ? LOAD_MAX : load;
+ load = (load < LOAD_MIN) ? LOAD_MIN : load;
+
+ spin_lock(&wdt->lock);
+ wdt->load_val = load;
+ /* roundup timeout to closest positive integer value */
+ wdt->timeout = div_u64((load + 1) * 2 + (rate / 2), rate);

Look in linux/kernel.h, use the
DIV_ROUND_CLOSEST() macro instead of this.

OK.

Linus,

Now i remember why i didn't choose DIV_ROUND_CLOSEST in V1 of this patch.
As it is a u64 division, i get following compilation error if i use
DIV_ROUND_CLOSEST. I wasn't sure if inclusion of some header file can remove
this error, and so i used div_u64.

error:
"undefined reference to '__aeabi_uldivmod'"

Do you have some other solution to fix this error??

viresh.

Linus Walleij

2010-07-27 22:25:20 UTC

Post by Viresh KUMAR
+ /* roundup timeout to closest positive integer value */
+ wdt->timeout = div_u64((load + 1) * 2 + (rate / 2), rate);

Look in linux/kernel.h, use the
DIV_ROUND_CLOSEST() macro instead of this.

Now i remember why i didn't choose DIV_ROUND_CLOSEST in V1 of this patch.
As it is a u64 division, i get following compilation error if i use
DIV_ROUND_CLOSEST. I wasn't sure if inclusion of some header file can remove
this error, and so i used div_u64.

Aha, I have no solution to that, but I'd recommend adding the comment
/* Cannot use DIV_ROUND_CLOSEST() due to the 64bit size */

Yours,
Linus Walleij

Viresh KUMAR

2010-07-28 04:02:22 UTC

Post by Viresh KUMAR
+ /* roundup timeout to closest positive integer value */
+ wdt->timeout = div_u64((load + 1) * 2 + (rate / 2), rate);

Look in linux/kernel.h, use the
DIV_ROUND_CLOSEST() macro instead of this.

Aha, I have no solution to that, but I'd recommend adding the comment
/* Cannot use DIV_ROUND_CLOSEST() due to the 64bit size */

I thought that also, but i didn't like that. There is nothing wrong or
special in code. It is fine to do divisions the way it is done. So i didn't
give any comment here. You still want me to give a comment there?

viresh.

Linus Walleij

2010-07-28 23:14:38 UTC