[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

The Synopsys DesignWare watchdog is found in several ARM based systems
and provides a choice of 16 timeout periods depending on the clock
input.  The watchdog cannot be disabled once started.

If the platform does not provide a clk for the watchdog then the user
can specify the input frequency with the struct dw_wdt_platform_data in
<linux/platform_data/dw_wdt.h>

v4:
	- cleanups as suggested by Viresh Kumar and Wim
	- provide a mechanism to handle NULL clks and allow
	  platform_data to specify the clk_rate
	- provide open-once protection

v3:
	- convert pm to dev_pm_ops
	- use devres for resource allocation

v2:
	- constify fops
	- request_mem_region() before ioremap()
	- disable clk if misc_register() fails

Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/watchdog/Kconfig             |   10 +
 drivers/watchdog/Makefile            |    1 +
 drivers/watchdog/dw_wdt.c            |  329 ++++++++++++++++++++++++++++++++++
 include/linux/platform_data/dw_wdt.h |   27 +++
 4 files changed, 367 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/dw_wdt.c
 create mode 100644 include/linux/platform_data/dw_wdt.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8a3aa2f..057bce0 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -331,6 +331,16 @@ config IMX2_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx2_wdt.
 
+config DW_WATCHDOG
+	tristate "Synopsys DesignWare watchdog"
+	depends on ARM
+        select WATCHDOG_NOWAYOUT
+	help
+	  Say Y here if to include support for the Synopsys DesignWare
+	  watchdog timer found in many ARM chips.
+	  To compile this driver as a module, choose M here: the
+	  module will be called dw_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 4b0ef38..3b3da4a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
 obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
 obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
+obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
new file mode 100644
index 0000000..b6d9b6e
--- /dev/null
+++ b/drivers/watchdog/dw_wdt.c
@@ -0,0 +1,329 @@
+/*
+ * Copyright 2010-2011 Picochip Ltd., Jamie Iles
+ * http://www.picochip.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This file implements a driver for the Synopsys DesignWare watchdog device
+ * in the many ARM subsystems. The watchdog has 16 different timeout periods
+ * and these are a function of the input clock frequency.
+ */
+#define pr_fmt(fmt) "dw_wdt: " fmt
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#include <linux/platform_data/dw_wdt.h>
+
+#define WDOG_CONTROL_REG_OFFSET             0x00
+#define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
+#define WDOG_TIMEOUT_RANGE_REG_OFFSET       0x04
+#define WDOG_CURRENT_COUNT_REG_OFFSET       0x08
+#define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
+#define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
+
+/* The maximum TOP (timeout period) value that can be set in the watchdog. */
+#define DW_WDT_MAX_TOP		15
+
+static struct {
+	spinlock_t		lock;
+	void __iomem		*regs;
+	struct clk		*clk;
+	unsigned long		clk_rate;
+	unsigned long		in_use;
+} dw_wdt;
+
+static inline int dw_wdt_is_enabled(void)
+{
+	return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
+		WDOG_CONTROL_REG_WDT_EN_MASK;
+}
+
+static inline int dw_wdt_top_in_seconds(unsigned top)
+{
+	/*
+	 * There are 16 possible timeout values in 0..15 where the number of
+	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
+	 */
+	return (1 << (16 + top)) / dw_wdt.clk_rate;
+}
+
+static int dw_wdt_set_top(unsigned top_s)
+{
+	int i, top_val = -1;
+
+	/*
+	 * Iterate over the timeout values until we find the closest match. We
+	 * always look for >=.
+	 */
+	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
+		if (dw_wdt_top_in_seconds(i) >= top_s) {
+			top_val = i;
+			break;
+		}
+
+	/*
+	 * If we didn't find a suitable value, it must have been too large. Go
+	 * with the biggest that we can.
+	 */
+	if (top_val < 0)
+		top_val = DW_WDT_MAX_TOP;
+
+	/* Set the new value in the watchdog. */
+	writel(top_val, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+
+	return dw_wdt_top_in_seconds(top_val);
+}
+
+static int dw_wdt_get_top(void)
+{
+	int top = readl(dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
+
+	return dw_wdt_top_in_seconds(top);
+}
+
+static void dw_wdt_keepalive(void)
+{
+	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+	       WDOG_COUNTER_RESTART_REG_OFFSET);
+}
+
+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+	if (test_and_set_bit(0, &dw_wdt.in_use))
+		return -EBUSY;
+
+	/* Make sure we don't get unloaded. */
+	__module_get(THIS_MODULE);
+
+	spin_lock(&dw_wdt.lock);
+	if (!dw_wdt_is_enabled()) {
+		/*
+		 * The watchdog is not currently enabled. Set the timeout to
+		 * the maximum and then start it.
+		 */
+		dw_wdt_set_top(DW_WDT_MAX_TOP);
+		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	}
+	spin_unlock(&dw_wdt.lock);
+
+	return nonseekable_open(inode, filp);
+}
+
+ssize_t dw_wdt_write(struct file *filp, const char __user *buf, size_t len,
+		     loff_t *offset)
+{
+	dw_wdt_keepalive();
+
+	return len;
+}
+
+static u32 dw_wdt_time_left(void)
+{
+	return readl(dw_wdt.regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
+		dw_wdt.clk_rate;
+}
+
+static const struct watchdog_info dw_wdt_ident = {
+	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+	.identity	= "Synopsys DesignWare Watchdog",
+};
+
+static long dw_wdt_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	unsigned long val;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user((struct watchdog_info *)arg, &dw_wdt_ident,
+				    sizeof(dw_wdt_ident)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, (int *)arg);
+
+	case WDIOC_KEEPALIVE:
+		dw_wdt_keepalive();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		return put_user(dw_wdt_set_top(val), (int __user *)arg);
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(dw_wdt_get_top(), (int __user *)arg);
+
+	case WDIOC_GETTIMELEFT:
+		/* Get the time left until expiry. */
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		return put_user(dw_wdt_time_left(), (int __user *)arg);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int dw_wdt_release(struct inode *inode, struct file *filp)
+{
+	pr_crit("WATCHDOG: device closed - timer will not stop\n");
+
+	clear_bit(0, &dw_wdt.in_use);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int dw_wdt_suspend(struct device *dev)
+{
+	clk_disable(dw_wdt.clk);
+
+	return 0;
+}
+
+static int dw_wdt_resume(struct device *dev)
+{
+	int err = clk_enable(dw_wdt.clk);
+
+	if (err)
+		return err;
+
+	dw_wdt_keepalive();
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_wdt_pm_ops = {
+	.suspend	= dw_wdt_suspend,
+	.resume		= dw_wdt_resume,
+};
+#endif /* CONFIG_PM */
+
+static const struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.open		= dw_wdt_open,
+	.write		= dw_wdt_write,
+	.unlocked_ioctl	= dw_wdt_ioctl,
+	.release	= dw_wdt_release
+};
+
+static struct miscdevice dw_wdt_miscdev = {
+	.fops		= &wdt_fops,
+	.name		= "watchdog",
+	.minor		= WATCHDOG_MINOR,
+};
+
+static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct dw_wdt_platform_data *pdata = pdev->dev.platform_data;
+
+	if (!mem)
+		return -EINVAL;
+
+	if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+				     "dw_wdt"))
+		return -ENOMEM;
+
+	dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
+					     resource_size(mem));
+	if (!dw_wdt.regs)
+		return -ENOMEM;
+
+	dw_wdt.clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dw_wdt.clk))
+		return -ENODEV;
+
+	ret = clk_enable(dw_wdt.clk);
+	if (ret)
+		goto out_put_clk;
+
+	/*
+	 * The timeout period of the watchdog is derived from the input clock
+	 * frequency.  For platforms that don't have a clk for the watchdog,
+	 * they can specify the WDT clock rate through the clk_rate field of
+	 * the struct dw_wdt_platform_data platform data.
+	 */
+	if (pdata && pdata->clk_rate > 0)
+		dw_wdt.clk_rate = pdata->clk_rate;
+	else
+		dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
+
+	if (!dw_wdt.clk_rate) {
+		dev_err(&pdev->dev, "no clk rate defined for watchdog, cannot enable\n");
+		ret = -EINVAL;
+		goto out_disable_clk;
+	}
+
+	spin_lock_init(&dw_wdt.lock);
+
+	ret = misc_register(&dw_wdt_miscdev);
+	if (ret)
+		goto out_put_clk;
+
+	return 0;
+
+out_disable_clk:
+	clk_disable(dw_wdt.clk);
+out_put_clk:
+	clk_put(dw_wdt.clk);
+
+	return ret;
+}
+
+static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
+{
+	misc_deregister(&dw_wdt_miscdev);
+
+	clk_disable(dw_wdt.clk);
+	clk_put(dw_wdt.clk);
+
+	return 0;
+}
+
+static struct platform_driver dw_wdt_driver = {
+	.probe		= dw_wdt_drv_probe,
+	.remove		= __devexit_p(dw_wdt_drv_remove),
+	.driver		= {
+		.name	= "dw_wdt",
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &dw_wdt_pm_ops,
+#endif /* CONFIG_PM */
+	},
+};
+
+static int __init dw_wdt_watchdog_init(void)
+{
+	return platform_driver_register(&dw_wdt_driver);
+}
+module_init(dw_wdt_watchdog_init);
+
+static void __exit dw_wdt_watchdog_exit(void)
+{
+	platform_driver_unregister(&dw_wdt_driver);
+}
+module_exit(dw_wdt_watchdog_exit);
+
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("Synopsys DesignWare Watchdog Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_data/dw_wdt.h b/include/linux/platform_data/dw_wdt.h
new file mode 100644
index 0000000..0af10ef
--- /dev/null
+++ b/include/linux/platform_data/dw_wdt.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2010 Picochip Ltd., Jamie Iles
+ * http://www.picochip.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This file implements a driver for the Synopsys DesignWare watchdog device
+ * in the many ARM subsystems. The watchdog has 16 different timeout periods
+ * and these are a function of the input clock frequency.
+ */
+#ifndef __DW_WDT_H__
+#define __DW_WDT_H__
+
+/**
+ * struct dw_wdt_platform_data - DesignWare WDT platform data
+ *
+ * @clk_rate: The frequency (HZ) at which the watchdog is driven.  The timeout
+ *	periods are derived from this frequency.
+ */
+struct dw_wdt_platform_data {
+	unsigned long		clk_rate;
+};
+
+#endif /* __DW_WDT_H__ */
-- 
1.7.3.4

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

The Synopsys DesignWare watchdog is found in several ARM based systems
and provides a choice of 16 timeout periods depending on the clock
input.  The watchdog cannot be disabled once started.

If the platform does not provide a clk for the watchdog then the user
can specify the input frequency with the struct dw_wdt_platform_data in
<linux/platform_data/dw_wdt.h>

v4:
	- cleanups as suggested by Viresh Kumar and Wim
	- provide a mechanism to handle NULL clks and allow
	  platform_data to specify the clk_rate
	- provide open-once protection

v3:
	- convert pm to dev_pm_ops
	- use devres for resource allocation

v2:
	- constify fops
	- request_mem_region() before ioremap()
	- disable clk if misc_register() fails

Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---

Viresh, Russell, Wim - thanks for your feedback.

include/linux/platform_data is used for the platform data as suggested
by Greg K-H in [1].

1. https://lkml.org/lkml/2011/1/7/323

 drivers/watchdog/Kconfig             |    9 +
 drivers/watchdog/Makefile            |    1 +
 drivers/watchdog/dw_wdt.c            |  329 ++++++++++++++++++++++++++++++++++
 include/linux/platform_data/dw_wdt.h |   27 +++
 4 files changed, 366 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/dw_wdt.c
 create mode 100644 include/linux/platform_data/dw_wdt.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8a3aa2f..0981aae 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -331,6 +331,15 @@ config IMX2_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx2_wdt.
 
+config DW_WATCHDOG
+	tristate "Synopsys DesignWare watchdog"
+        select WATCHDOG_NOWAYOUT
+	help
+	  Say Y here if to include support for the Synopsys DesignWare
+	  watchdog timer found in many ARM chips.
+	  To compile this driver as a module, choose M here: the
+	  module will be called dw_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 4b0ef38..3b3da4a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
 obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
 obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
+obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
new file mode 100644
index 0000000..b6d9b6e
--- /dev/null
+++ b/drivers/watchdog/dw_wdt.c
@@ -0,0 +1,329 @@
+/*
+ * Copyright 2010-2011 Picochip Ltd., Jamie Iles
+ * http://www.picochip.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This file implements a driver for the Synopsys DesignWare watchdog device
+ * in the many ARM subsystems. The watchdog has 16 different timeout periods
+ * and these are a function of the input clock frequency.
+ */
+#define pr_fmt(fmt) "dw_wdt: " fmt
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#include <linux/platform_data/dw_wdt.h>
+
+#define WDOG_CONTROL_REG_OFFSET             0x00
+#define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
+#define WDOG_TIMEOUT_RANGE_REG_OFFSET       0x04
+#define WDOG_CURRENT_COUNT_REG_OFFSET       0x08
+#define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
+#define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
+
+/* The maximum TOP (timeout period) value that can be set in the watchdog. */
+#define DW_WDT_MAX_TOP		15
+
+static struct {
+	spinlock_t		lock;
+	void __iomem		*regs;
+	struct clk		*clk;
+	unsigned long		clk_rate;
+	unsigned long		in_use;
+} dw_wdt;
+
+static inline int dw_wdt_is_enabled(void)
+{
+	return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
+		WDOG_CONTROL_REG_WDT_EN_MASK;
+}
+
+static inline int dw_wdt_top_in_seconds(unsigned top)
+{
+	/*
+	 * There are 16 possible timeout values in 0..15 where the number of
+	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
+	 */
+	return (1 << (16 + top)) / dw_wdt.clk_rate;
+}
+
+static int dw_wdt_set_top(unsigned top_s)
+{
+	int i, top_val = -1;
+
+	/*
+	 * Iterate over the timeout values until we find the closest match. We
+	 * always look for >=.
+	 */
+	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
+		if (dw_wdt_top_in_seconds(i) >= top_s) {
+			top_val = i;
+			break;
+		}
+
+	/*
+	 * If we didn't find a suitable value, it must have been too large. Go
+	 * with the biggest that we can.
+	 */
+	if (top_val < 0)
+		top_val = DW_WDT_MAX_TOP;
+
+	/* Set the new value in the watchdog. */
+	writel(top_val, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+
+	return dw_wdt_top_in_seconds(top_val);
+}
+
+static int dw_wdt_get_top(void)
+{
+	int top = readl(dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
+
+	return dw_wdt_top_in_seconds(top);
+}
+
+static void dw_wdt_keepalive(void)
+{
+	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+	       WDOG_COUNTER_RESTART_REG_OFFSET);
+}
+
+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+	if (test_and_set_bit(0, &dw_wdt.in_use))
+		return -EBUSY;
+
+	/* Make sure we don't get unloaded. */
+	__module_get(THIS_MODULE);
+
+	spin_lock(&dw_wdt.lock);
+	if (!dw_wdt_is_enabled()) {
+		/*
+		 * The watchdog is not currently enabled. Set the timeout to
+		 * the maximum and then start it.
+		 */
+		dw_wdt_set_top(DW_WDT_MAX_TOP);
+		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	}
+	spin_unlock(&dw_wdt.lock);
+
+	return nonseekable_open(inode, filp);
+}
+
+ssize_t dw_wdt_write(struct file *filp, const char __user *buf, size_t len,
+		     loff_t *offset)
+{
+	dw_wdt_keepalive();
+
+	return len;
+}
+
+static u32 dw_wdt_time_left(void)
+{
+	return readl(dw_wdt.regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
+		dw_wdt.clk_rate;
+}
+
+static const struct watchdog_info dw_wdt_ident = {
+	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+	.identity	= "Synopsys DesignWare Watchdog",
+};
+
+static long dw_wdt_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	unsigned long val;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user((struct watchdog_info *)arg, &dw_wdt_ident,
+				    sizeof(dw_wdt_ident)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, (int *)arg);
+
+	case WDIOC_KEEPALIVE:
+		dw_wdt_keepalive();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		return put_user(dw_wdt_set_top(val), (int __user *)arg);
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(dw_wdt_get_top(), (int __user *)arg);
+
+	case WDIOC_GETTIMELEFT:
+		/* Get the time left until expiry. */
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		return put_user(dw_wdt_time_left(), (int __user *)arg);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int dw_wdt_release(struct inode *inode, struct file *filp)
+{
+	pr_crit("WATCHDOG: device closed - timer will not stop\n");
+
+	clear_bit(0, &dw_wdt.in_use);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int dw_wdt_suspend(struct device *dev)
+{
+	clk_disable(dw_wdt.clk);
+
+	return 0;
+}
+
+static int dw_wdt_resume(struct device *dev)
+{
+	int err = clk_enable(dw_wdt.clk);
+
+	if (err)
+		return err;
+
+	dw_wdt_keepalive();
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_wdt_pm_ops = {
+	.suspend	= dw_wdt_suspend,
+	.resume		= dw_wdt_resume,
+};
+#endif /* CONFIG_PM */
+
+static const struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.open		= dw_wdt_open,
+	.write		= dw_wdt_write,
+	.unlocked_ioctl	= dw_wdt_ioctl,
+	.release	= dw_wdt_release
+};
+
+static struct miscdevice dw_wdt_miscdev = {
+	.fops		= &wdt_fops,
+	.name		= "watchdog",
+	.minor		= WATCHDOG_MINOR,
+};
+
+static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct dw_wdt_platform_data *pdata = pdev->dev.platform_data;
+
+	if (!mem)
+		return -EINVAL;
+
+	if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+				     "dw_wdt"))
+		return -ENOMEM;
+
+	dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
+					     resource_size(mem));
+	if (!dw_wdt.regs)
+		return -ENOMEM;
+
+	dw_wdt.clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dw_wdt.clk))
+		return -ENODEV;
+
+	ret = clk_enable(dw_wdt.clk);
+	if (ret)
+		goto out_put_clk;
+
+	/*
+	 * The timeout period of the watchdog is derived from the input clock
+	 * frequency.  For platforms that don't have a clk for the watchdog,
+	 * they can specify the WDT clock rate through the clk_rate field of
+	 * the struct dw_wdt_platform_data platform data.
+	 */
+	if (pdata && pdata->clk_rate > 0)
+		dw_wdt.clk_rate = pdata->clk_rate;
+	else
+		dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
+
+	if (!dw_wdt.clk_rate) {
+		dev_err(&pdev->dev, "no clk rate defined for watchdog, cannot enable\n");
+		ret = -EINVAL;
+		goto out_disable_clk;
+	}
+
+	spin_lock_init(&dw_wdt.lock);
+
+	ret = misc_register(&dw_wdt_miscdev);
+	if (ret)
+		goto out_put_clk;
+
+	return 0;
+
+out_disable_clk:
+	clk_disable(dw_wdt.clk);
+out_put_clk:
+	clk_put(dw_wdt.clk);
+
+	return ret;
+}
+
+static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
+{
+	misc_deregister(&dw_wdt_miscdev);
+
+	clk_disable(dw_wdt.clk);
+	clk_put(dw_wdt.clk);
+
+	return 0;
+}
+
+static struct platform_driver dw_wdt_driver = {
+	.probe		= dw_wdt_drv_probe,
+	.remove		= __devexit_p(dw_wdt_drv_remove),
+	.driver		= {
+		.name	= "dw_wdt",
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &dw_wdt_pm_ops,
+#endif /* CONFIG_PM */
+	},
+};
+
+static int __init dw_wdt_watchdog_init(void)
+{
+	return platform_driver_register(&dw_wdt_driver);
+}
+module_init(dw_wdt_watchdog_init);
+
+static void __exit dw_wdt_watchdog_exit(void)
+{
+	platform_driver_unregister(&dw_wdt_driver);
+}
+module_exit(dw_wdt_watchdog_exit);
+
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("Synopsys DesignWare Watchdog Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_data/dw_wdt.h b/include/linux/platform_data/dw_wdt.h
new file mode 100644
index 0000000..0af10ef
--- /dev/null
+++ b/include/linux/platform_data/dw_wdt.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2010 Picochip Ltd., Jamie Iles
+ * http://www.picochip.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This file implements a driver for the Synopsys DesignWare watchdog device
+ * in the many ARM subsystems. The watchdog has 16 different timeout periods
+ * and these are a function of the input clock frequency.
+ */
+#ifndef __DW_WDT_H__
+#define __DW_WDT_H__
+
+/**
+ * struct dw_wdt_platform_data - DesignWare WDT platform data
+ *
+ * @clk_rate: The frequency (HZ) at which the watchdog is driven.  The timeout
+ *	periods are derived from this frequency.
+ */
+struct dw_wdt_platform_data {
+	unsigned long		clk_rate;
+};
+
+#endif /* __DW_WDT_H__ */
-- 
1.7.3.4

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Wim Van Sebroeck]

Hi Jamie,

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 8a3aa2f..0981aae 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -331,6 +331,15 @@ config IMX2_WDT
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called imx2_wdt.
>  
> +config DW_WATCHDOG
> +	tristate "Synopsys DesignWare watchdog"
> +        select WATCHDOG_NOWAYOUT
> +	help
> +	  Say Y here if to include support for the Synopsys DesignWare
> +	  watchdog timer found in many ARM chips.
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called dw_wdt.
> +
>  # AVR32 Architecture
>  
>  config AT32AP700X_WDT

Could you add the correct dependancy also in Kconfig?

Thanks in advance,
Wim.

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

On Mon, Jan 10, 2011 at 02:21:15PM +0100, Wim Van Sebroeck wrote:
> Hi Jamie,
> 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 8a3aa2f..0981aae 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -331,6 +331,15 @@ config IMX2_WDT
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called imx2_wdt.
> >  
> > +config DW_WATCHDOG
> > +	tristate "Synopsys DesignWare watchdog"
> > +        select WATCHDOG_NOWAYOUT
> > +	help
> > +	  Say Y here if to include support for the Synopsys DesignWare
> > +	  watchdog timer found in many ARM chips.
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called dw_wdt.
> > +
> >  # AVR32 Architecture
> >  
> >  config AT32AP700X_WDT
> 
> Could you add the correct dependancy also in Kconfig?

Sure, updated patch below with a dependency on ARM.

Jamie

8<----------------------------------------------------------------------

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Wim Van Sebroeck]

Hi Jamie,

> +config DW_WATCHDOG
> +	tristate "Synopsys DesignWare watchdog"
> +        select WATCHDOG_NOWAYOUT

I overlooked something: If your watchdog driver cannot be stopped once it's started,
then it's best to add a comment about that in your code. Selecting the WATCHDOG_NOWAYOUT
feauture (which is overall for all drivers) is not the way to do it properly.
What you should do is add a timer that ticks the watchdog if /dev/watchdog is closed.
Examples for this functionality can be found in at91sam9_wdt.c, pika_wdt.c, ...

Sorry,
Wim.

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

The Synopsys DesignWare watchdog is found in several ARM based systems
and provides a choice of 16 timeout periods depending on the clock
input.  The watchdog cannot be disabled once started.

If the platform does not provide a clk for the watchdog then the user
can specify the input frequency with the struct dw_wdt_platform_data in
<linux/platform_data/dw_wdt.h>

v5:
	- don't select CONFIG_WATCHDOG_NOWAYOUT, use a software
	  heartbeat to keep the wdt alive if we allow close.
v4:
	- cleanups as suggested by Viresh Kumar and Wim
	- provide a mechanism to handle NULL clks and allow
	  platform_data to specify the clk_rate
	- provide open-once protection

v3:
	- convert pm to dev_pm_ops
	- use devres for resource allocation

v2:
	- constify fops
	- request_mem_region() before ioremap()
	- disable clk if misc_register() fails

Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/watchdog/Kconfig             |    9 +
 drivers/watchdog/Makefile            |    1 +
 drivers/watchdog/dw_wdt.c            |  405 ++++++++++++++++++++++++++++++++++
 include/linux/platform_data/dw_wdt.h |   27 +++
 4 files changed, 442 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/dw_wdt.c
 create mode 100644 include/linux/platform_data/dw_wdt.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8a3aa2f..62c607a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -331,6 +331,15 @@ config IMX2_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx2_wdt.
 
+config DW_WATCHDOG
+	tristate "Synopsys DesignWare watchdog"
+	depends on ARM
+	help
+	  Say Y here if to include support for the Synopsys DesignWare
+	  watchdog timer found in many ARM chips.
+	  To compile this driver as a module, choose M here: the
+	  module will be called dw_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 4b0ef38..3b3da4a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
 obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
 obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
+obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
new file mode 100644
index 0000000..45fa1fb
--- /dev/null
+++ b/drivers/watchdog/dw_wdt.c
@@ -0,0 +1,405 @@
+/*
+ * Copyright 2010-2011 Picochip Ltd., Jamie Iles
+ * http://www.picochip.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This file implements a driver for the Synopsys DesignWare watchdog device
+ * in the many ARM subsystems. The watchdog has 16 different timeout periods
+ * and these are a function of the input clock frequency.
+ *
+ * The DesignWare watchdog cannot be stopped once it has been started so we
+ * use a software timer to implement a ping that will keep the watchdog alive.
+ * If we receive an expected close for the watchdog then we keep the timer
+ * running, otherwise the timer is stopped and the watchdog will expire.
+ */
+#define pr_fmt(fmt) "dw_wdt: " fmt
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/pm.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#include <linux/platform_data/dw_wdt.h>
+
+#define WDOG_CONTROL_REG_OFFSET             0x00
+#define WDOG_CONTROL_REG_WDT_EN_MASK	    0x01
+#define WDOG_TIMEOUT_RANGE_REG_OFFSET       0x04
+#define WDOG_CURRENT_COUNT_REG_OFFSET       0x08
+#define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
+#define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
+
+/* The maximum TOP (timeout period) value that can be set in the watchdog. */
+#define DW_WDT_MAX_TOP		15
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
+		 "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+#define WDT_TIMEOUT		(HZ / 2)
+
+static struct {
+	spinlock_t		lock;
+	void __iomem		*regs;
+	struct clk		*clk;
+	unsigned long		clk_rate;
+	unsigned long		in_use;
+	unsigned long		next_heartbeat;
+	struct timer_list	timer;
+	int			expect_close;
+} dw_wdt;
+
+static inline int dw_wdt_is_enabled(void)
+{
+	return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
+		WDOG_CONTROL_REG_WDT_EN_MASK;
+}
+
+static inline int dw_wdt_top_in_seconds(unsigned top)
+{
+	/*
+	 * There are 16 possible timeout values in 0..15 where the number of
+	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
+	 */
+	return (1 << (16 + top)) / dw_wdt.clk_rate;
+}
+
+static int dw_wdt_get_top(void)
+{
+	int top = readl(dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
+
+	return dw_wdt_top_in_seconds(top);
+}
+
+static inline void dw_wdt_set_next_heartbeat(void)
+{
+	dw_wdt.next_heartbeat = jiffies + dw_wdt_get_top() * HZ;
+}
+
+static int dw_wdt_set_top(unsigned top_s)
+{
+	int i, top_val = -1;
+
+	/*
+	 * Iterate over the timeout values until we find the closest match. We
+	 * always look for >=.
+	 */
+	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
+		if (dw_wdt_top_in_seconds(i) >= top_s) {
+			top_val = i;
+			break;
+		}
+
+	/*
+	 * If we didn't find a suitable value, it must have been too large. Go
+	 * with the biggest that we can.
+	 */
+	if (top_val < 0)
+		top_val = DW_WDT_MAX_TOP;
+
+	/* Set the new value in the watchdog. */
+	writel(top_val, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+
+	dw_wdt_set_next_heartbeat();
+
+	return dw_wdt_top_in_seconds(top_val);
+}
+
+static void dw_wdt_keepalive(void)
+{
+	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+	       WDOG_COUNTER_RESTART_REG_OFFSET);
+}
+
+static void dw_wdt_ping(unsigned long data)
+{
+	if (time_before(jiffies, dw_wdt.next_heartbeat) ||
+	    (!nowayout && !dw_wdt.in_use)) {
+		dw_wdt_keepalive();
+		mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
+	} else
+		pr_crit("keepalive missed, machine will reset\n");
+}
+
+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+	if (test_and_set_bit(0, &dw_wdt.in_use))
+		return -EBUSY;
+
+	/* Make sure we don't get unloaded. */
+	__module_get(THIS_MODULE);
+
+	spin_lock(&dw_wdt.lock);
+	if (!dw_wdt_is_enabled()) {
+		/*
+		 * The watchdog is not currently enabled. Set the timeout to
+		 * the maximum and then start it.
+		 */
+		dw_wdt_set_top(DW_WDT_MAX_TOP);
+		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	}
+
+	dw_wdt_set_next_heartbeat();
+
+	spin_unlock(&dw_wdt.lock);
+
+	return nonseekable_open(inode, filp);
+}
+
+ssize_t dw_wdt_write(struct file *filp, const char __user *buf, size_t len,
+		     loff_t *offset)
+{
+	if (!len)
+		return 0;
+
+	if (!nowayout) {
+		size_t i;
+
+		dw_wdt.expect_close = 0;
+
+		for (i = 0; i < len; ++i) {
+			char c;
+
+			if (get_user(c, buf + i))
+				return -EFAULT;
+
+			if (c == 'V') {
+				dw_wdt.expect_close = 1;
+				break;
+			}
+		}
+	}
+
+	dw_wdt_set_next_heartbeat();
+	mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
+
+	return len;
+}
+
+static u32 dw_wdt_time_left(void)
+{
+	return readl(dw_wdt.regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
+		dw_wdt.clk_rate;
+}
+
+static const struct watchdog_info dw_wdt_ident = {
+	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
+			  WDIOF_MAGICCLOSE,
+	.identity	= "Synopsys DesignWare Watchdog",
+};
+
+static long dw_wdt_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	unsigned long val;
+	int timeout;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user((struct watchdog_info *)arg, &dw_wdt_ident,
+				    sizeof(dw_wdt_ident)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, (int *)arg);
+
+	case WDIOC_KEEPALIVE:
+		dw_wdt_set_next_heartbeat();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		timeout = dw_wdt_set_top(val);
+		return put_user(timeout , (int __user *)arg);
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(dw_wdt_get_top(), (int __user *)arg);
+
+	case WDIOC_GETTIMELEFT:
+		/* Get the time left until expiry. */
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		return put_user(dw_wdt_time_left(), (int __user *)arg);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int dw_wdt_release(struct inode *inode, struct file *filp)
+{
+	clear_bit(0, &dw_wdt.in_use);
+
+	if (!dw_wdt.expect_close) {
+		del_timer(&dw_wdt.timer);
+
+		if (!nowayout)
+			pr_crit("unexpected close, system will reboot soon\n");
+		else
+			pr_crit("watchdog cannot be disabled, system will reboot soon\n");
+	}
+
+	dw_wdt.expect_close = 0;
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int dw_wdt_suspend(struct device *dev)
+{
+	clk_disable(dw_wdt.clk);
+
+	return 0;
+}
+
+static int dw_wdt_resume(struct device *dev)
+{
+	int err = clk_enable(dw_wdt.clk);
+
+	if (err)
+		return err;
+
+	dw_wdt_keepalive();
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_wdt_pm_ops = {
+	.suspend	= dw_wdt_suspend,
+	.resume		= dw_wdt_resume,
+};
+#endif /* CONFIG_PM */
+
+static const struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.open		= dw_wdt_open,
+	.write		= dw_wdt_write,
+	.unlocked_ioctl	= dw_wdt_ioctl,
+	.release	= dw_wdt_release
+};
+
+static struct miscdevice dw_wdt_miscdev = {
+	.fops		= &wdt_fops,
+	.name		= "watchdog",
+	.minor		= WATCHDOG_MINOR,
+};
+
+static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct dw_wdt_platform_data *pdata = pdev->dev.platform_data;
+
+	if (!mem)
+		return -EINVAL;
+
+	if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+				     "dw_wdt"))
+		return -ENOMEM;
+
+	dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
+					     resource_size(mem));
+	if (!dw_wdt.regs)
+		return -ENOMEM;
+
+	dw_wdt.clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dw_wdt.clk))
+		return -ENODEV;
+
+	ret = clk_enable(dw_wdt.clk);
+	if (ret)
+		goto out_put_clk;
+
+	/*
+	 * The timeout period of the watchdog is derived from the input clock
+	 * frequency.  For platforms that don't have a clk for the watchdog,
+	 * they can specify the WDT clock rate through the clk_rate field of
+	 * the struct dw_wdt_platform_data platform data.
+	 */
+	if (pdata && pdata->clk_rate > 0)
+		dw_wdt.clk_rate = pdata->clk_rate;
+	else
+		dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
+
+	if (!dw_wdt.clk_rate) {
+		dev_err(&pdev->dev, "no clk rate defined for watchdog, cannot enable\n");
+		ret = -EINVAL;
+		goto out_disable_clk;
+	}
+
+	spin_lock_init(&dw_wdt.lock);
+
+	ret = misc_register(&dw_wdt_miscdev);
+	if (ret)
+		goto out_put_clk;
+
+	dw_wdt_set_next_heartbeat();
+	setup_timer(&dw_wdt.timer, dw_wdt_ping, 0);
+	mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
+
+	return 0;
+
+out_disable_clk:
+	clk_disable(dw_wdt.clk);
+out_put_clk:
+	clk_put(dw_wdt.clk);
+
+	return ret;
+}
+
+static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
+{
+	misc_deregister(&dw_wdt_miscdev);
+
+	clk_disable(dw_wdt.clk);
+	clk_put(dw_wdt.clk);
+
+	return 0;
+}
+
+static struct platform_driver dw_wdt_driver = {
+	.probe		= dw_wdt_drv_probe,
+	.remove		= __devexit_p(dw_wdt_drv_remove),
+	.driver		= {
+		.name	= "dw_wdt",
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &dw_wdt_pm_ops,
+#endif /* CONFIG_PM */
+	},
+};
+
+static int __init dw_wdt_watchdog_init(void)
+{
+	return platform_driver_register(&dw_wdt_driver);
+}
+module_init(dw_wdt_watchdog_init);
+
+static void __exit dw_wdt_watchdog_exit(void)
+{
+	platform_driver_unregister(&dw_wdt_driver);
+}
+module_exit(dw_wdt_watchdog_exit);
+
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("Synopsys DesignWare Watchdog Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
diff --git a/include/linux/platform_data/dw_wdt.h b/include/linux/platform_data/dw_wdt.h
new file mode 100644
index 0000000..0af10ef
--- /dev/null
+++ b/include/linux/platform_data/dw_wdt.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2010 Picochip Ltd., Jamie Iles
+ * http://www.picochip.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This file implements a driver for the Synopsys DesignWare watchdog device
+ * in the many ARM subsystems. The watchdog has 16 different timeout periods
+ * and these are a function of the input clock frequency.
+ */
+#ifndef __DW_WDT_H__
+#define __DW_WDT_H__
+
+/**
+ * struct dw_wdt_platform_data - DesignWare WDT platform data
+ *
+ * @clk_rate: The frequency (HZ) at which the watchdog is driven.  The timeout
+ *	periods are derived from this frequency.
+ */
+struct dw_wdt_platform_data {
+	unsigned long		clk_rate;
+};
+
+#endif /* __DW_WDT_H__ */
-- 
1.7.3.4

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[viresh kumar]

Jamie,

Sorry for sending comments on V5 !!

I don't know why, but checkpatch used to give few errors which it is
not giving now.
Like:
- Mixing spaces and tabs
- Line over 80 columns.

There are few places in this patch where i have seen these issues, but
checkpatch doesn't
report them at all.

On Mon, Jan 10, 2011 at 9:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 8a3aa2f..62c607a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -331,6 +331,15 @@ config IMX2_WDT
> ? ? ? ? ?To compile this driver as a module, choose M here: the
> ? ? ? ? ?module will be called imx2_wdt.
>
> +config DW_WATCHDOG
> + ? ? ? tristate "Synopsys DesignWare watchdog"
> + ? ? ? depends on ARM

Correct me if i am wrong, but can't this peripheral be used for any
other architecture.
We are adding it as a platform device and not amba device.
Might be synopsys give some other interface here, other than AMBA.

> + ? ? ? help
> + ? ? ? ? Say Y here if to include support for the Synopsys DesignWare
> + ? ? ? ? watchdog timer found in many ARM chips.
> + ? ? ? ? To compile this driver as a module, choose M here: the
> + ? ? ? ? module will be called dw_wdt.
> +
> ?# AVR32 Architecture
>

(...)

> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> new file mode 100644
> index 0000000..45fa1fb
> --- /dev/null
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -0,0 +1,405 @@
> +/*
> + * Copyright 2010-2011 Picochip Ltd., Jamie Iles
> + * http://www.picochip.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * This file implements a driver for the Synopsys DesignWare watchdog device
> + * in the many ARM subsystems. The watchdog has 16 different timeout periods
> + * and these are a function of the input clock frequency.
> + *
> + * The DesignWare watchdog cannot be stopped once it has been started so we
> + * use a software timer to implement a ping that will keep the watchdog alive.
> + * If we receive an expected close for the watchdog then we keep the timer
> + * running, otherwise the timer is stopped and the watchdog will expire.
> + */
> +#define pr_fmt(fmt) "dw_wdt: " fmt
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
> +#include <linux/uaccess.h>
> +#include <linux/watchdog.h>
> +

can remove this blank line.

> +#include <linux/platform_data/dw_wdt.h>
> +
> +#define WDOG_CONTROL_REG_OFFSET ? ? ? ? ? ? 0x00
> +#define WDOG_CONTROL_REG_WDT_EN_MASK ? ? ? 0x01
> +#define WDOG_TIMEOUT_RANGE_REG_OFFSET ? ? ? 0x04
> +#define WDOG_CURRENT_COUNT_REG_OFFSET ? ? ? 0x08
> +#define WDOG_COUNTER_RESTART_REG_OFFSET ? ? 0x0c
> +#define WDOG_COUNTER_RESTART_KICK_VALUE ? ? ? ? ? ?0x76

I don't know why, but i have seen a lot of comment that people give to
align macro's.
Use tabs instead of spaces between macro name and its value. Also keep
them aligned.
I am not sure if its absolutely necessary.

(...)

> +static int dw_wdt_set_top(unsigned top_s)
> +{
> + ? ? ? int i, top_val = -1;

We can assign top_val = DW_WDT_MAX_TOP, here and

> +
> + ? ? ? /*
> + ? ? ? ?* Iterate over the timeout values until we find the closest match. We
> + ? ? ? ?* always look for >=.
> + ? ? ? ?*/
> + ? ? ? for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
> + ? ? ? ? ? ? ? if (dw_wdt_top_in_seconds(i) >= top_s) {
> + ? ? ? ? ? ? ? ? ? ? ? top_val = i;
> + ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? /*
> + ? ? ? ?* If we didn't find a suitable value, it must have been too large. Go
> + ? ? ? ?* with the biggest that we can.
> + ? ? ? ?*/
> + ? ? ? if (top_val < 0)
> + ? ? ? ? ? ? ? top_val = DW_WDT_MAX_TOP;

remove this check.

> +
> + ? ? ? /* Set the new value in the watchdog. */
> + ? ? ? writel(top_val, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> +
> + ? ? ? dw_wdt_set_next_heartbeat();
> +
> + ? ? ? return dw_wdt_top_in_seconds(top_val);
> +}
> +

(...)

> +static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
> +{
> + ? ? ? int ret;
> + ? ? ? struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ? ? ? struct dw_wdt_platform_data *pdata = pdev->dev.platform_data;
> +
> + ? ? ? if (!mem)
> + ? ? ? ? ? ? ? return -EINVAL;
> +
> + ? ? ? if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"dw_wdt"))
> + ? ? ? ? ? ? ? return -ENOMEM;
> +
> + ? ? ? dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?resource_size(mem));

this can come in single line.

> + ? ? ? if (!dw_wdt.regs)
> + ? ? ? ? ? ? ? return -ENOMEM;
> +
> + ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
> + ? ? ? if (IS_ERR(dw_wdt.clk))
> + ? ? ? ? ? ? ? return -ENODEV;

shouldn't we use the error value returned from clk_get here instead of defining
a new value -ENODEV?

> +
> + ? ? ? ret = clk_enable(dw_wdt.clk);

should we call this routine if clk is NULL? Probably we will get error here.
But as we discussed earlier, we should support machines who don't support clk
framework. So, shouldn't we check clk for NULL here and call this
routine only if
we have a valid clk pointer, otherwise continue instead of returning error.

> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? goto out_put_clk;
> +
> + ? ? ? /*
> + ? ? ? ?* The timeout period of the watchdog is derived from the input clock
> + ? ? ? ?* frequency. ?For platforms that don't have a clk for the watchdog,
> + ? ? ? ?* they can specify the WDT clock rate through the clk_rate field of
> + ? ? ? ?* the struct dw_wdt_platform_data platform data.
> + ? ? ? ?*/
> + ? ? ? if (pdata && pdata->clk_rate > 0)
> + ? ? ? ? ? ? ? dw_wdt.clk_rate = pdata->clk_rate;
> + ? ? ? else
> + ? ? ? ? ? ? ? dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
> +

I know it doesn't make sense for a platform to have support for clk framework
and pass rate through plat data.
But this code will always take pdata->rate, if it is passed.
Wouldn't it be better if we reverse the sequence.

if (clk)
 rate = clk_get_rate(...);
else {
 pdata = pdev->dev.platform_data;
 if (pdata)
  rate = pdata->rate;
}

and then following code

> + ? ? ? if (!dw_wdt.clk_rate) {
> + ? ? ? ? ? ? ? dev_err(&pdev->dev, "no clk rate defined for watchdog, cannot enable\n");
> + ? ? ? ? ? ? ? ret = -EINVAL;
> + ? ? ? ? ? ? ? goto out_disable_clk;
> + ? ? ? }
> +
> + ? ? ? spin_lock_init(&dw_wdt.lock);
> +
> + ? ? ? ret = misc_register(&dw_wdt_miscdev);
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? goto out_put_clk;

shouldn't it be out_disable_clk??

> +
> + ? ? ? dw_wdt_set_next_heartbeat();
> + ? ? ? setup_timer(&dw_wdt.timer, dw_wdt_ping, 0);
> + ? ? ? mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
> +
> + ? ? ? return 0;
> +
> +out_disable_clk:
> + ? ? ? clk_disable(dw_wdt.clk);
> +out_put_clk:
> + ? ? ? clk_put(dw_wdt.clk);
> +
> + ? ? ? return ret;
> +}
> +
> +static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
> +{
> + ? ? ? misc_deregister(&dw_wdt_miscdev);
> +
> + ? ? ? clk_disable(dw_wdt.clk);
> + ? ? ? clk_put(dw_wdt.clk);
> +
> + ? ? ? return 0;
> +}
> +
> +static struct platform_driver dw_wdt_driver = {
> + ? ? ? .probe ? ? ? ? ?= dw_wdt_drv_probe,
> + ? ? ? .remove ? ? ? ? = __devexit_p(dw_wdt_drv_remove),
> + ? ? ? .driver ? ? ? ? = {
> + ? ? ? ? ? ? ? .name ? = "dw_wdt",
> + ? ? ? ? ? ? ? .owner ?= THIS_MODULE,
> +#ifdef CONFIG_PM
> + ? ? ? ? ? ? ? .pm ? ? = &dw_wdt_pm_ops,
> +#endif /* CONFIG_PM */
> + ? ? ? },
> +};
> +
> +static int __init dw_wdt_watchdog_init(void)
> +{
> + ? ? ? return platform_driver_register(&dw_wdt_driver);
> +}
> +module_init(dw_wdt_watchdog_init);
> +
> +static void __exit dw_wdt_watchdog_exit(void)
> +{
> + ? ? ? platform_driver_unregister(&dw_wdt_driver);
> +}
> +module_exit(dw_wdt_watchdog_exit);
> +
> +MODULE_AUTHOR("Jamie Iles");
> +MODULE_DESCRIPTION("Synopsys DesignWare Watchdog Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> diff --git a/include/linux/platform_data/dw_wdt.h b/include/linux/platform_data/dw_wdt.h
> new file mode 100644
> index 0000000..0af10ef
> --- /dev/null
> +++ b/include/linux/platform_data/dw_wdt.h

It looks you have created a separate generic folder platform_data.
But generally we put these files directly into existing folders.
I am not quite sure if that's fine or we should avoid this.

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

Hi Viresh,

On Wed, Jan 12, 2011 at 10:46:28AM +0530, viresh kumar wrote:
> Jamie,
> 
> Sorry for sending comments on V5 !!

No problem!  All feedback appreciated!

> I don't know why, but checkpatch used to give few errors which it is
> not giving now.
> Like:
> - Mixing spaces and tabs
> - Line over 80 columns.
> 
> There are few places in this patch where i have seen these issues, but
> checkpatch doesn't
> report them at all.

Hmm, the only lines over 80 chars are printk lines and I can't see any 
with mixed spaces and tabs...


> On Mon, Jan 10, 2011 at 9:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 8a3aa2f..62c607a 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -331,6 +331,15 @@ config IMX2_WDT
> > ? ? ? ? ?To compile this driver as a module, choose M here: the
> > ? ? ? ? ?module will be called imx2_wdt.
> >
> > +config DW_WATCHDOG
> > + ? ? ? tristate "Synopsys DesignWare watchdog"
> > + ? ? ? depends on ARM
> 
> Correct me if i am wrong, but can't this peripheral be used for any
> other architecture.
> We are adding it as a platform device and not amba device.
> Might be synopsys give some other interface here, other than AMBA.

Well this is an APB peripheral so I think that it is fair to have the 
dependency on ARM for now.  If someone wants to use it on another 
architecture then we can easily widen this if we need to.

> > + ? ? ? help
> > + ? ? ? ? Say Y here if to include support for the Synopsys DesignWare
> > + ? ? ? ? watchdog timer found in many ARM chips.
> > + ? ? ? ? To compile this driver as a module, choose M here: the
> > + ? ? ? ? module will be called dw_wdt.
> > +
> > ?# AVR32 Architecture
> >
> 
> (...)
> 
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > new file mode 100644
> > index 0000000..45fa1fb
> > --- /dev/null
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -0,0 +1,405 @@
> > +/*
> > + * Copyright 2010-2011 Picochip Ltd., Jamie Iles
> > + * http://www.picochip.com
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + *
> > + * This file implements a driver for the Synopsys DesignWare watchdog device
> > + * in the many ARM subsystems. The watchdog has 16 different timeout periods
> > + * and these are a function of the input clock frequency.
> > + *
> > + * The DesignWare watchdog cannot be stopped once it has been started so we
> > + * use a software timer to implement a ping that will keep the watchdog alive.
> > + * If we receive an expected close for the watchdog then we keep the timer
> > + * running, otherwise the timer is stopped and the watchdog will expire.
> > + */
> > +#define pr_fmt(fmt) "dw_wdt: " fmt
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/fs.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/pm.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/timer.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/watchdog.h>
> > +
> 
> can remove this blank line.

Ok, will do.

> > +#include <linux/platform_data/dw_wdt.h>
> > +
> > +#define WDOG_CONTROL_REG_OFFSET ? ? ? ? ? ? 0x00
> > +#define WDOG_CONTROL_REG_WDT_EN_MASK ? ? ? 0x01
> > +#define WDOG_TIMEOUT_RANGE_REG_OFFSET ? ? ? 0x04
> > +#define WDOG_CURRENT_COUNT_REG_OFFSET ? ? ? 0x08
> > +#define WDOG_COUNTER_RESTART_REG_OFFSET ? ? 0x0c
> > +#define WDOG_COUNTER_RESTART_KICK_VALUE ? ? ? ? ? ?0x76
> 
> I don't know why, but i have seen a lot of comment that people give to
> align macro's.
> Use tabs instead of spaces between macro name and its value. Also keep
> them aligned.
> I am not sure if its absolutely necessary.

Ahh, yes, there are the spaces.  I'll fix this up, they should be 
aligned.

> (...)
> 
> > +static int dw_wdt_set_top(unsigned top_s)
> > +{
> > + ? ? ? int i, top_val = -1;
> 
> We can assign top_val = DW_WDT_MAX_TOP, here and
> 
> > +
> > + ? ? ? /*
> > + ? ? ? ?* Iterate over the timeout values until we find the closest match. We
> > + ? ? ? ?* always look for >=.
> > + ? ? ? ?*/
> > + ? ? ? for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
> > + ? ? ? ? ? ? ? if (dw_wdt_top_in_seconds(i) >= top_s) {
> > + ? ? ? ? ? ? ? ? ? ? ? top_val = i;
> > + ? ? ? ? ? ? ? ? ? ? ? break;
> > + ? ? ? ? ? ? ? }
> > +
> > + ? ? ? /*
> > + ? ? ? ?* If we didn't find a suitable value, it must have been too large. Go
> > + ? ? ? ?* with the biggest that we can.
> > + ? ? ? ?*/
> > + ? ? ? if (top_val < 0)
> > + ? ? ? ? ? ? ? top_val = DW_WDT_MAX_TOP;
> 
> remove this check.

That's cleaner.  Thanks.

> > + ? ? ? /* Set the new value in the watchdog. */
> > + ? ? ? writel(top_val, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
> > +
> > + ? ? ? dw_wdt_set_next_heartbeat();
> > +
> > + ? ? ? return dw_wdt_top_in_seconds(top_val);
> > +}
> > +
> 
> (...)
> 
> > +static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
> > +{
> > + ? ? ? int ret;
> > + ? ? ? struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + ? ? ? struct dw_wdt_platform_data *pdata = pdev->dev.platform_data;
> > +
> > + ? ? ? if (!mem)
> > + ? ? ? ? ? ? ? return -EINVAL;
> > +
> > + ? ? ? if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"dw_wdt"))
> > + ? ? ? ? ? ? ? return -ENOMEM;
> > +
> > + ? ? ? dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?resource_size(mem));
> 
> this can come in single line.

Ok.

> > + ? ? ? if (!dw_wdt.regs)
> > + ? ? ? ? ? ? ? return -ENOMEM;
> > +
> > + ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
> > + ? ? ? if (IS_ERR(dw_wdt.clk))
> > + ? ? ? ? ? ? ? return -ENODEV;
> 
> shouldn't we use the error value returned from clk_get here instead of defining
> a new value -ENODEV?

Yes, that's probably better.

> > +
> > + ? ? ? ret = clk_enable(dw_wdt.clk);
> 
> should we call this routine if clk is NULL? Probably we will get error here.
> But as we discussed earlier, we should support machines who don't support clk
> framework. So, shouldn't we check clk for NULL here and call this
> routine only if we have a valid clk pointer, otherwise continue 
> instead of returning error.

I think this is right, it's perfectly valid for a platform to return 
NULL as a clk and you can pass that to clk_enable().

> > + ? ? ? if (ret)
> > + ? ? ? ? ? ? ? goto out_put_clk;
> > +
> > + ? ? ? /*
> > + ? ? ? ?* The timeout period of the watchdog is derived from the input clock
> > + ? ? ? ?* frequency. ?For platforms that don't have a clk for the watchdog,
> > + ? ? ? ?* they can specify the WDT clock rate through the clk_rate field of
> > + ? ? ? ?* the struct dw_wdt_platform_data platform data.
> > + ? ? ? ?*/
> > + ? ? ? if (pdata && pdata->clk_rate > 0)
> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = pdata->clk_rate;
> > + ? ? ? else
> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
> > +
> 
> I know it doesn't make sense for a platform to have support for clk framework
> and pass rate through plat data. But this code will always take 
> pdata->rate, if it is passed.
> Wouldn't it be better if we reverse the sequence.
> 
> if (clk)
>  rate = clk_get_rate(...);
> else {
>  pdata = pdev->dev.platform_data;
>  if (pdata)
>   rate = pdata->rate;
> }
> 
> and then following code

That's what I did first (without the test that clk is not NULL) but 
switched it the other way round so that the platform can override the 
frequency by setting platform_data->clk_rate to nonzero.  That just 
seems a little more flexible and easy for testing but I'm happy to 
switch the order if you feel that's important.

> > + ? ? ? if (!dw_wdt.clk_rate) {
> > + ? ? ? ? ? ? ? dev_err(&pdev->dev, "no clk rate defined for watchdog, cannot enable\n");
> > + ? ? ? ? ? ? ? ret = -EINVAL;
> > + ? ? ? ? ? ? ? goto out_disable_clk;
> > + ? ? ? }
> > +
> > + ? ? ? spin_lock_init(&dw_wdt.lock);
> > +
> > + ? ? ? ret = misc_register(&dw_wdt_miscdev);
> > + ? ? ? if (ret)
> > + ? ? ? ? ? ? ? goto out_put_clk;
> 
> shouldn't it be out_disable_clk??

Yes, thanks.

> > +
> > + ? ? ? dw_wdt_set_next_heartbeat();
> > + ? ? ? setup_timer(&dw_wdt.timer, dw_wdt_ping, 0);
> > + ? ? ? mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
> > +
> > + ? ? ? return 0;
> > +
> > +out_disable_clk:
> > + ? ? ? clk_disable(dw_wdt.clk);
> > +out_put_clk:
> > + ? ? ? clk_put(dw_wdt.clk);
> > +
> > + ? ? ? return ret;
> > +}
> > +
> > +static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
> > +{
> > + ? ? ? misc_deregister(&dw_wdt_miscdev);
> > +
> > + ? ? ? clk_disable(dw_wdt.clk);
> > + ? ? ? clk_put(dw_wdt.clk);
> > +
> > + ? ? ? return 0;
> > +}
> > +
> > +static struct platform_driver dw_wdt_driver = {
> > + ? ? ? .probe ? ? ? ? ?= dw_wdt_drv_probe,
> > + ? ? ? .remove ? ? ? ? = __devexit_p(dw_wdt_drv_remove),
> > + ? ? ? .driver ? ? ? ? = {
> > + ? ? ? ? ? ? ? .name ? = "dw_wdt",
> > + ? ? ? ? ? ? ? .owner ?= THIS_MODULE,
> > +#ifdef CONFIG_PM
> > + ? ? ? ? ? ? ? .pm ? ? = &dw_wdt_pm_ops,
> > +#endif /* CONFIG_PM */
> > + ? ? ? },
> > +};
> > +
> > +static int __init dw_wdt_watchdog_init(void)
> > +{
> > + ? ? ? return platform_driver_register(&dw_wdt_driver);
> > +}
> > +module_init(dw_wdt_watchdog_init);
> > +
> > +static void __exit dw_wdt_watchdog_exit(void)
> > +{
> > + ? ? ? platform_driver_unregister(&dw_wdt_driver);
> > +}
> > +module_exit(dw_wdt_watchdog_exit);
> > +
> > +MODULE_AUTHOR("Jamie Iles");
> > +MODULE_DESCRIPTION("Synopsys DesignWare Watchdog Driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> > diff --git a/include/linux/platform_data/dw_wdt.h b/include/linux/platform_data/dw_wdt.h
> > new file mode 100644
> > index 0000000..0af10ef
> > --- /dev/null
> > +++ b/include/linux/platform_data/dw_wdt.h
> 
> It looks you have created a separate generic folder platform_data.
> But generally we put these files directly into existing folders.
> I am not quite sure if that's fine or we should avoid this.

Yes, this was suggested by Greg K-H to stop include/linux getting filled 
with platform data (https://lkml.org/lkml/2011/1/7/323).

Thanks again!

Jamie

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[viresh kumar]

Jamie,

On Wed, Jan 12, 2011 at 1:54 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>> I don't know why, but checkpatch used to give few errors which it is
>> not giving now.
>> Like:
>> - Mixing spaces and tabs
>> - Line over 80 columns.
>>
>> There are few places in this patch where i have seen these issues, but
>> checkpatch doesn't
>> report them at all.
>
> Hmm, the only lines over 80 chars are printk lines and I can't see any
> with mixed spaces and tabs...

There are few in file drivers/watchdog/dw_wdt.c
line 40-45, 127, 133, 156, 167, 204, 216, 315, 319.

I can check it easily as i have used following in my vimrc file:
au FileType c match error /\s\+$\|\%>80v.\+\|[ ][ ]\+\|\n\n\n\+\|,[^
]\|^[ ]\+[^\*]\|(\s\+\|\s\+)/
au FileType c highlight error ctermbg=red guibg=red ctermfg=blue guifg=blue
au FileType h match error /\s\+$\|\%>80v.\+\|[ ][ ]\+\|\n\n\n\+\|,[^
]\|^[ ]\+[^\*]\|(\s\+\|\s\+)/
au FileType h highlight error ctermbg=red guibg=red ctermfg=blue guifg=blue

and so these sections are highlighted.

(...)

>> > + ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
>> > + ? ? ? if (IS_ERR(dw_wdt.clk))
>> > + ? ? ? ? ? ? ? return -ENODEV;
>> > +
>> > + ? ? ? ret = clk_enable(dw_wdt.clk);
>>
>> should we call this routine if clk is NULL? Probably we will get error here.
>> But as we discussed earlier, we should support machines who don't support clk
>> framework. So, shouldn't we check clk for NULL here and call this
>> routine only if we have a valid clk pointer, otherwise continue
>> instead of returning error.
>
> I think this is right, it's perfectly valid for a platform to return
> NULL as a clk and you can pass that to clk_enable().
>

I agree on the first part: NULL can be returned from clk_get(), but
passing NULL to clk_enable and clk_get_rate doesn't look fine to me.
As they will always return error in that case and code will return from below
error check. That's why i suggest we should check for NULL before
clk_enable/disable and clk_get_rate.
What do you say?

>> > + ? ? ? if (ret)
>> > + ? ? ? ? ? ? ? goto out_put_clk;
>> > +
>> > + ? ? ? /*
>> > + ? ? ? ?* The timeout period of the watchdog is derived from the input clock
>> > + ? ? ? ?* frequency. ?For platforms that don't have a clk for the watchdog,
>> > + ? ? ? ?* they can specify the WDT clock rate through the clk_rate field of
>> > + ? ? ? ?* the struct dw_wdt_platform_data platform data.
>> > + ? ? ? ?*/
>> > + ? ? ? if (pdata && pdata->clk_rate > 0)
>> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = pdata->clk_rate;
>> > + ? ? ? else
>> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
>> > +
>>
>> I know it doesn't make sense for a platform to have support for clk framework
>> and pass rate through plat data. But this code will always take
>> pdata->rate, if it is passed.
>> Wouldn't it be better if we reverse the sequence.
>>
>> if (clk)
>> ?rate = clk_get_rate(...);
>> else {
>> ?pdata = pdev->dev.platform_data;
>> ?if (pdata)
>> ? rate = pdata->rate;
>> }
>>
>> and then following code
>
> That's what I did first (without the test that clk is not NULL) but
> switched it the other way round so that the platform can override the
> frequency by setting platform_data->clk_rate to nonzero. ?That just
> seems a little more flexible and easy for testing but I'm happy to
> switch the order if you feel that's important.
>

Problem will occur if rate is dynamically changed and we are still believing
on platform code's clk_rate.
Would be better if we switch order. i.e. give priority to clk_get_rate over
pdata->rate.

>> > diff --git a/include/linux/platform_data/dw_wdt.h b/include/linux/platform_data/dw_wdt.h
>> > new file mode 100644
>> > index 0000000..0af10ef
>> > --- /dev/null
>> > +++ b/include/linux/platform_data/dw_wdt.h
>>
>> It looks you have created a separate generic folder platform_data.
>> But generally we put these files directly into existing folders.
>> I am not quite sure if that's fine or we should avoid this.
>
> Yes, this was suggested by Greg K-H to stop include/linux getting filled
> with platform data (https://lkml.org/lkml/2011/1/7/323).
>

Oh. That's good.

--
viresh
ST Microelectronics

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Russell King - ARM Linux]

On Wed, Jan 12, 2011 at 02:27:47PM +0530, viresh kumar wrote:
> > I think this is right, it's perfectly valid for a platform to return
> > NULL as a clk and you can pass that to clk_enable().
> >
> 
> I agree on the first part: NULL can be returned from clk_get(), but
> passing NULL to clk_enable and clk_get_rate doesn't look fine to me.

Whatever non-error cookie clk_get() passes out, clk_enable() etc must
eat, even if it contains dead flies rather than currants.  It's not
for the driver to make decisions on what clk_enable() et.al. accept,
other than as documented by the clk API - which says that errors from
clk_get() are indicated by IS_ERR(clk).

If IS_ERR(clk) is false, the driver _must_ assume that there is no error
and clk_enable() etc will accept the clk.

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

On Wed, Jan 12, 2011 at 02:27:47PM +0530, viresh kumar wrote:
> Jamie,
> 
> On Wed, Jan 12, 2011 at 1:54 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> >> I don't know why, but checkpatch used to give few errors which it is
> >> not giving now.
> >> Like:
> >> - Mixing spaces and tabs
> >> - Line over 80 columns.
> >>
> >> There are few places in this patch where i have seen these issues, but
> >> checkpatch doesn't
> >> report them at all.
> >
> > Hmm, the only lines over 80 chars are printk lines and I can't see any
> > with mixed spaces and tabs...
> 
> There are few in file drivers/watchdog/dw_wdt.c
> line 40-45, 127, 133, 156, 167, 204, 216, 315, 319.

I do need to fix up the register locations (40-45) but the others are ok 
- they are tab indented to get to a multiple of 8 then spaces to align 
to the '(' brackets etc.

> >> I know it doesn't make sense for a platform to have support for clk framework
> >> and pass rate through plat data. But this code will always take
> >> pdata->rate, if it is passed.
> >> Wouldn't it be better if we reverse the sequence.
> >>
> >> if (clk)
> >> ?rate = clk_get_rate(...);
> >> else {
> >> ?pdata = pdev->dev.platform_data;
> >> ?if (pdata)
> >> ? rate = pdata->rate;
> >> }
> >>
> >> and then following code
> >
> > That's what I did first (without the test that clk is not NULL) but
> > switched it the other way round so that the platform can override the
> > frequency by setting platform_data->clk_rate to nonzero. ?That just
> > seems a little more flexible and easy for testing but I'm happy to
> > switch the order if you feel that's important.
> >
> 
> Problem will occur if rate is dynamically changed and we are still believing
> on platform code's clk_rate.
> Would be better if we switch order. i.e. give priority to clk_get_rate over
> pdata->rate.

If the platform can change the rate then I don't see why it would define 
the rate in the platform data though.  Anyway, I can make the change and 
issue a warning and fail the probe if we're using the rate from 
clk_get_rate() and there is a non-zero rate in the platform data.

Jamie

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[viresh kumar]

On Wed, Jan 12, 2011 at 5:00 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> On Wed, Jan 12, 2011 at 02:27:47PM +0530, viresh kumar wrote:
>> On Wed, Jan 12, 2011 at 1:54 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>> >> I don't know why, but checkpatch used to give few errors which it is
>> >> not giving now.
>> >> Like:
>> >> - Mixing spaces and tabs
>> >> - Line over 80 columns.
>> >>
> I do need to fix up the register locations (40-45) but the others are ok
> - they are tab indented to get to a multiple of 8 then spaces to align
> to the '(' brackets etc.

That's what i said. Checkpatch used to give warning for them earlier.
I don't know if it is correct or not. Anyway, others will comment if it is
still a issue. You can keep that as it is.

>> Problem will occur if rate is dynamically changed and we are still believing
>> on platform code's clk_rate.
>> Would be better if we switch order. i.e. give priority to clk_get_rate over
>> pdata->rate.
>
> If the platform can change the rate then I don't see why it would define
> the rate in the platform data though. ?Anyway, I can make the change and
> issue a warning and fail the probe if we're using the rate from
> clk_get_rate() and there is a non-zero rate in the platform data.

that's better.

--
viresh

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

On Wed, Jan 12, 2011 at 05:08:42PM +0530, viresh kumar wrote:
> On Wed, Jan 12, 2011 at 5:00 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> > On Wed, Jan 12, 2011 at 02:27:47PM +0530, viresh kumar wrote:
> >> On Wed, Jan 12, 2011 at 1:54 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> >> >> I don't know why, but checkpatch used to give few errors which it is
> >> >> not giving now.
> >> >> Like:
> >> >> - Mixing spaces and tabs
> >> >> - Line over 80 columns.
> >> >>
> > I do need to fix up the register locations (40-45) but the others are ok
> > - they are tab indented to get to a multiple of 8 then spaces to align
> > to the '(' brackets etc.
> 
> That's what i said. Checkpatch used to give warning for them earlier.
> I don't know if it is correct or not. Anyway, others will comment if it is
> still a issue. You can keep that as it is.
> 
> >> Problem will occur if rate is dynamically changed and we are still believing
> >> on platform code's clk_rate.
> >> Would be better if we switch order. i.e. give priority to clk_get_rate over
> >> pdata->rate.
> >
> > If the platform can change the rate then I don't see why it would define
> > the rate in the platform data though. ?Anyway, I can make the change and
> > issue a warning and fail the probe if we're using the rate from
> > clk_get_rate() and there is a non-zero rate in the platform data.
> 
> that's better.

Ok, I'll give Wim chance to have a look over v5 then I'll integrate 
these changes.

Thanks again for the review.

Jamie

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[Jamie Iles]

On Wed, Jan 12, 2011 at 10:46:28AM +0530, viresh kumar wrote:
> > + ? ? ? if (ret)
> > + ? ? ? ? ? ? ? goto out_put_clk;
> > +
> > + ? ? ? /*
> > + ? ? ? ?* The timeout period of the watchdog is derived from the input clock
> > + ? ? ? ?* frequency. ?For platforms that don't have a clk for the watchdog,
> > + ? ? ? ?* they can specify the WDT clock rate through the clk_rate field of
> > + ? ? ? ?* the struct dw_wdt_platform_data platform data.
> > + ? ? ? ?*/
> > + ? ? ? if (pdata && pdata->clk_rate > 0)
> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = pdata->clk_rate;
> > + ? ? ? else
> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
> > +
> 
> I know it doesn't make sense for a platform to have support for clk framework
> and pass rate through plat data.
> But this code will always take pdata->rate, if it is passed.
> Wouldn't it be better if we reverse the sequence.
> 
> if (clk)
>  rate = clk_get_rate(...);
> else {
>  pdata = pdev->dev.platform_data;
>  if (pdata)
>   rate = pdata->rate;
> }

Hmm, looking back at this again, I wonder if the correct thing to do is 
just add a dependency in Kconfig for the driver on HAVE_CLK like 
drivers/spi/dw_spi_mmio and ditch the ability to set the rate through 
platform data?  The platform has to at least provide clk_get(), 
clk_put() and clk_get_rate() at a minimum or the driver won't even 
build...

This would make things a lot simpler and it's not as if this is going to 
break anything as there aren't currently any users of the driver (and 
the one that will be does support the clk framework).

Jamie

[PATCH] watchdog: add support for the Synopsys DesignWare WDT

[viresh kumar]

On Fri, Jan 21, 2011 at 11:38 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> On Wed, Jan 12, 2011 at 10:46:28AM +0530, viresh kumar wrote:
>> > + ? ? ? if (ret)
>> > + ? ? ? ? ? ? ? goto out_put_clk;
>> > +
>> > + ? ? ? /*
>> > + ? ? ? ?* The timeout period of the watchdog is derived from the input clock
>> > + ? ? ? ?* frequency. ?For platforms that don't have a clk for the watchdog,
>> > + ? ? ? ?* they can specify the WDT clock rate through the clk_rate field of
>> > + ? ? ? ?* the struct dw_wdt_platform_data platform data.
>> > + ? ? ? ?*/
>> > + ? ? ? if (pdata && pdata->clk_rate > 0)
>> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = pdata->clk_rate;
>> > + ? ? ? else
>> > + ? ? ? ? ? ? ? dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
>> > +
>>
>> I know it doesn't make sense for a platform to have support for clk framework
>> and pass rate through plat data.
>> But this code will always take pdata->rate, if it is passed.
>> Wouldn't it be better if we reverse the sequence.
>>
>> if (clk)
>> ?rate = clk_get_rate(...);
>> else {
>> ?pdata = pdev->dev.platform_data;
>> ?if (pdata)
>> ? rate = pdata->rate;
>> }
>
> Hmm, looking back at this again, I wonder if the correct thing to do is
> just add a dependency in Kconfig for the driver on HAVE_CLK like
> drivers/spi/dw_spi_mmio and ditch the ability to set the rate through
> platform data? ?The platform has to at least provide clk_get(),
> clk_put() and clk_get_rate() at a minimum or the driver won't even
> build...

Yes, this has to be done from compilation point of view, we need
HAVE_CLK in kconfig.

>
> This would make things a lot simpler and it's not as if this is going to
> break anything as there aren't currently any users of the driver (and
> the one that will be does support the clk framework).
>

Ok.