linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/6] Update Davinci watchdog driver
@ 2013-11-25 14:04 Ivan Khoronzhuk
  2013-11-25 14:04 ` [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core Ivan Khoronzhuk
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Ivan Khoronzhuk @ 2013-11-25 14:04 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko,
	Ivan Khoronzhuk

These patches are intended to update Davinci watchdog to use WDT core
and reuse driver for keystone arch, because Keystone uses the similar
IP like Davinci.

See Documentation:
Davinci DM646x - http://www.ti.com/lit/ug/spruer5b/spruer5b.pdf
Keystone - http://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf

Also:
 - improved to support GET_TIMELEFT option.
 - added "clocks" and "timeout-sec" properties to DT.

Based on
git://git.kernel.org/pub/scm/linux/kernel/git/ssantosh/linux-keystone.git
keystone/master

v2..v3:
- watchdog: davinci: change driver to use WDT core
	removed "ti,keystone-wdt" compatible from driver
	renamed wdt device names for mach-davinci from
		"watchdog" to "davinci-wdt"

- watchdog: davinci: reuse driver for keystone arch
	moved "clocks" property under "Optional properties"
	improved decsription of "clocks" property

- arm: dts: keystone: add watchdog entry
	replaced "ti,keystone-wdt" compatible on
		 "ti,keystone-wdt","ti,davinci-wdt"

Ivan Khoronzhuk (6):
  watchdog: davinci: change driver to use WDT core
  watchdog: davinci: use davinci_wdt_device structure to hold device
    data
  watchdog: davinci: add GET_TIMELEFT option support
  watchdog: davinci: add "timeout-sec" property
  watchdog: davinci: reuse driver for keystone arch
  arm: dts: keystone: add watchdog entry

 .../devicetree/bindings/watchdog/davinci-wdt.txt   |   16 +-
 arch/arm/boot/dts/keystone.dtsi                    |    6 +
 arch/arm/mach-davinci/da8xx-dt.c                   |    2 +-
 arch/arm/mach-davinci/devices-da8xx.c              |    4 +-
 arch/arm/mach-davinci/devices.c                    |    2 +-
 drivers/watchdog/Kconfig                           |    6 +-
 drivers/watchdog/davinci_wdt.c                     |  225 +++++++++-----------
 7 files changed, 126 insertions(+), 135 deletions(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
@ 2013-11-25 14:04 ` Ivan Khoronzhuk
  2013-11-25 14:16   ` Santosh Shilimkar
                     ` (2 more replies)
  2013-11-25 14:04 ` [PATCH v3 2/6] watchdog: davinci: use davinci_wdt_device structure to hold device data Ivan Khoronzhuk
                   ` (5 subsequent siblings)
  6 siblings, 3 replies; 16+ messages in thread
From: Ivan Khoronzhuk @ 2013-11-25 14:04 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko,
	Ivan Khoronzhuk

To reduce code duplicate and increase code readability use WDT core
code to handle WDT interface.

Remove io_lock as the WDT core uses mutex to lock each wdt device.
Remove wdt_state as the WDT core tracks state with its own variable.

The watchdog_init_timeout() can read timeout value from timeout-sec
property if the passed value is out of bounds. The heartbeat is
initialized in next way. If heartbeat is not set thought module
parameter, try to read it's value from WDT node timeout-sec property.
If node has no one, use default value.

The heartbeat is hold in wdd->timeout by WDT core, so use it in
order to set timeout period.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/mach-davinci/da8xx-dt.c      |    2 +-
 arch/arm/mach-davinci/devices-da8xx.c |    4 +-
 arch/arm/mach-davinci/devices.c       |    2 +-
 drivers/watchdog/Kconfig              |    2 +
 drivers/watchdog/davinci_wdt.c        |  151 ++++++++-------------------------
 5 files changed, 43 insertions(+), 118 deletions(-)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index d2bc574..ed19287 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -32,7 +32,7 @@ static void __init da8xx_init_irq(void)
 
 static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("ti,davinci-i2c", 0x01c22000, "i2c_davinci.1", NULL),
-	OF_DEV_AUXDATA("ti,davinci-wdt", 0x01c21000, "watchdog", NULL),
+	OF_DEV_AUXDATA("ti,davinci-wdt", 0x01c21000, "davinci-wdt", NULL),
 	OF_DEV_AUXDATA("ti,da830-mmc", 0x01c40000, "da830-mmc.0", NULL),
 	OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f00000, "ehrpwm", NULL),
 	OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f02000, "ehrpwm", NULL),
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index c46eccb..f9ba74b 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -389,7 +389,7 @@ static struct resource da8xx_watchdog_resources[] = {
 };
 
 static struct platform_device da8xx_wdt_device = {
-	.name		= "watchdog",
+	.name		= "davinci-wdt",
 	.id		= -1,
 	.num_resources	= ARRAY_SIZE(da8xx_watchdog_resources),
 	.resource	= da8xx_watchdog_resources,
@@ -399,7 +399,7 @@ void da8xx_restart(enum reboot_mode mode, const char *cmd)
 {
 	struct device *dev;
 
-	dev = bus_find_device_by_name(&platform_bus_type, NULL, "watchdog");
+	dev = bus_find_device_by_name(&platform_bus_type, NULL, "davinci-wdt");
 	if (!dev) {
 		pr_err("%s: failed to find watchdog device\n", __func__);
 		return;
diff --git a/arch/arm/mach-davinci/devices.c b/arch/arm/mach-davinci/devices.c
index 3996e98..5cf9a02 100644
--- a/arch/arm/mach-davinci/devices.c
+++ b/arch/arm/mach-davinci/devices.c
@@ -302,7 +302,7 @@ static struct resource wdt_resources[] = {
 };
 
 struct platform_device davinci_wdt_device = {
-	.name		= "watchdog",
+	.name		= "davinci-wdt",
 	.id		= -1,
 	.num_resources	= ARRAY_SIZE(wdt_resources),
 	.resource	= wdt_resources,
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 5be6e91..eb8c89d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -271,6 +271,8 @@ config IOP_WATCHDOG
 config DAVINCI_WATCHDOG
 	tristate "DaVinci watchdog"
 	depends on ARCH_DAVINCI
+	select WATCHDOG_CORE
+	select WATCHDOG_NOWAYOUT
 	help
 	  Say Y here if to include support for the watchdog timer
 	  in the DaVinci DM644x/DM646x processors.
diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
index dd625cc..2a94dde 100644
--- a/drivers/watchdog/davinci_wdt.c
+++ b/drivers/watchdog/davinci_wdt.c
@@ -3,7 +3,7 @@
  *
  * Watchdog driver for DaVinci DM644x/DM646x processors
  *
- * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2006-2013 Texas Instruments.
  *
  * 2007 (c) MontaVista Software, Inc. This file is licensed under
  * the terms of the GNU General Public License version 2. This program
@@ -15,18 +15,12 @@
 #include <linux/moduleparam.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
-#include <linux/fs.h>
-#include <linux/miscdevice.h>
 #include <linux/watchdog.h>
 #include <linux/init.h>
-#include <linux/bitops.h>
 #include <linux/platform_device.h>
-#include <linux/spinlock.h>
-#include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/device.h>
 #include <linux/clk.h>
-#include <linux/slab.h>
 #include <linux/err.h>
 
 #define MODULE_NAME "DAVINCI-WDT: "
@@ -61,31 +55,12 @@
 #define WDKEY_SEQ0		(0xa5c6 << 16)
 #define WDKEY_SEQ1		(0xda7e << 16)
 
-static int heartbeat = DEFAULT_HEARTBEAT;
-
-static DEFINE_SPINLOCK(io_lock);
-static unsigned long wdt_status;
-#define WDT_IN_USE        0
-#define WDT_OK_TO_CLOSE   1
-#define WDT_REGION_INITED 2
-#define WDT_DEVICE_INITED 3
-
+static int heartbeat;
 static void __iomem	*wdt_base;
 struct clk		*wdt_clk;
+static struct watchdog_device	wdt_wdd;
 
-static void wdt_service(void)
-{
-	spin_lock(&io_lock);
-
-	/* put watchdog in service state */
-	iowrite32(WDKEY_SEQ0, wdt_base + WDTCR);
-	/* put watchdog in active state */
-	iowrite32(WDKEY_SEQ1, wdt_base + WDTCR);
-
-	spin_unlock(&io_lock);
-}
-
-static void wdt_enable(void)
+static int davinci_wdt_start(struct watchdog_device *wdd)
 {
 	u32 tgcr;
 	u32 timer_margin;
@@ -93,8 +68,6 @@ static void wdt_enable(void)
 
 	wdt_freq = clk_get_rate(wdt_clk);
 
-	spin_lock(&io_lock);
-
 	/* disable, internal clock source */
 	iowrite32(0, wdt_base + TCR);
 	/* reset timer, set mode to 64-bit watchdog, and unreset */
@@ -105,9 +78,9 @@ static void wdt_enable(void)
 	iowrite32(0, wdt_base + TIM12);
 	iowrite32(0, wdt_base + TIM34);
 	/* set timeout period */
-	timer_margin = (((u64)heartbeat * wdt_freq) & 0xffffffff);
+	timer_margin = (((u64)wdd->timeout * wdt_freq) & 0xffffffff);
 	iowrite32(timer_margin, wdt_base + PRD12);
-	timer_margin = (((u64)heartbeat * wdt_freq) >> 32);
+	timer_margin = (((u64)wdd->timeout * wdt_freq) >> 32);
 	iowrite32(timer_margin, wdt_base + PRD34);
 	/* enable run continuously */
 	iowrite32(ENAMODE12_PERIODIC, wdt_base + TCR);
@@ -119,84 +92,28 @@ static void wdt_enable(void)
 	iowrite32(WDKEY_SEQ0 | WDEN, wdt_base + WDTCR);
 	/* put watchdog in active state */
 	iowrite32(WDKEY_SEQ1 | WDEN, wdt_base + WDTCR);
-
-	spin_unlock(&io_lock);
-}
-
-static int davinci_wdt_open(struct inode *inode, struct file *file)
-{
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
-		return -EBUSY;
-
-	wdt_enable();
-
-	return nonseekable_open(inode, file);
+	return 0;
 }
 
-static ssize_t
-davinci_wdt_write(struct file *file, const char *data, size_t len,
-		  loff_t *ppos)
+static int davinci_wdt_ping(struct watchdog_device *wdd)
 {
-	if (len)
-		wdt_service();
-
-	return len;
+	/* put watchdog in service state */
+	iowrite32(WDKEY_SEQ0, wdt_base + WDTCR);
+	/* put watchdog in active state */
+	iowrite32(WDKEY_SEQ1, wdt_base + WDTCR);
+	return 0;
 }
 
-static const struct watchdog_info ident = {
+static const struct watchdog_info davinci_wdt_info = {
 	.options = WDIOF_KEEPALIVEPING,
 	.identity = "DaVinci Watchdog",
 };
 
-static long davinci_wdt_ioctl(struct file *file,
-					unsigned int cmd, unsigned long arg)
-{
-	int ret = -ENOTTY;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info *)arg, &ident,
-				   sizeof(ident)) ? -EFAULT : 0;
-		break;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(0, (int *)arg);
-		break;
-
-	case WDIOC_KEEPALIVE:
-		wdt_service();
-		ret = 0;
-		break;
-
-	case WDIOC_GETTIMEOUT:
-		ret = put_user(heartbeat, (int *)arg);
-		break;
-	}
-	return ret;
-}
-
-static int davinci_wdt_release(struct inode *inode, struct file *file)
-{
-	wdt_service();
-	clear_bit(WDT_IN_USE, &wdt_status);
-
-	return 0;
-}
-
-static const struct file_operations davinci_wdt_fops = {
-	.owner = THIS_MODULE,
-	.llseek = no_llseek,
-	.write = davinci_wdt_write,
-	.unlocked_ioctl = davinci_wdt_ioctl,
-	.open = davinci_wdt_open,
-	.release = davinci_wdt_release,
-};
-
-static struct miscdevice davinci_wdt_miscdev = {
-	.minor = WATCHDOG_MINOR,
-	.name = "watchdog",
-	.fops = &davinci_wdt_fops,
+static const struct watchdog_ops davinci_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= davinci_wdt_start,
+	.stop		= davinci_wdt_ping,
+	.ping		= davinci_wdt_ping,
 };
 
 static int davinci_wdt_probe(struct platform_device *pdev)
@@ -204,6 +121,7 @@ static int davinci_wdt_probe(struct platform_device *pdev)
 	int ret = 0;
 	struct device *dev = &pdev->dev;
 	struct resource  *wdt_mem;
+	struct watchdog_device *wdd;
 
 	wdt_clk = devm_clk_get(dev, NULL);
 	if (WARN_ON(IS_ERR(wdt_clk)))
@@ -211,29 +129,34 @@ static int davinci_wdt_probe(struct platform_device *pdev)
 
 	clk_prepare_enable(wdt_clk);
 
-	if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
-		heartbeat = DEFAULT_HEARTBEAT;
+	wdd			= &wdt_wdd;
+	wdd->info		= &davinci_wdt_info;
+	wdd->ops		= &davinci_wdt_ops;
+	wdd->min_timeout	= 1;
+	wdd->max_timeout	= MAX_HEARTBEAT;
+	wdd->timeout		= DEFAULT_HEARTBEAT;
+
+	watchdog_init_timeout(wdd, heartbeat, dev);
+
+	dev_info(dev, "heartbeat %d sec\n", wdd->timeout);
 
-	dev_info(dev, "heartbeat %d sec\n", heartbeat);
+	watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
 
 	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	wdt_base = devm_ioremap_resource(dev, wdt_mem);
 	if (IS_ERR(wdt_base))
 		return PTR_ERR(wdt_base);
 
-	ret = misc_register(&davinci_wdt_miscdev);
-	if (ret < 0) {
-		dev_err(dev, "cannot register misc device\n");
-	} else {
-		set_bit(WDT_DEVICE_INITED, &wdt_status);
-	}
+	ret = watchdog_register_device(wdd);
+	if (ret < 0)
+		dev_err(dev, "cannot register watchdog device\n");
 
 	return ret;
 }
 
 static int davinci_wdt_remove(struct platform_device *pdev)
 {
-	misc_deregister(&davinci_wdt_miscdev);
+	watchdog_unregister_device(&wdt_wdd);
 	clk_disable_unprepare(wdt_clk);
 
 	return 0;
@@ -247,7 +170,7 @@ MODULE_DEVICE_TABLE(of, davinci_wdt_of_match);
 
 static struct platform_driver platform_wdt_driver = {
 	.driver = {
-		.name = "watchdog",
+		.name = "davinci-wdt",
 		.owner	= THIS_MODULE,
 		.of_match_table = davinci_wdt_of_match,
 	},
@@ -267,4 +190,4 @@ MODULE_PARM_DESC(heartbeat,
 		 __MODULE_STRING(DEFAULT_HEARTBEAT));
 
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:watchdog");
+MODULE_ALIAS("platform:davinci-wdt");
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 2/6] watchdog: davinci: use davinci_wdt_device structure to hold device data
  2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
  2013-11-25 14:04 ` [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core Ivan Khoronzhuk
@ 2013-11-25 14:04 ` Ivan Khoronzhuk
  2013-11-25 14:04 ` [PATCH v3 3/6] watchdog: davinci: add GET_TIMELEFT option support Ivan Khoronzhuk
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Ivan Khoronzhuk @ 2013-11-25 14:04 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko,
	Ivan Khoronzhuk

Some SoCs, like Keystone 2, can support more than one WDT and each
watchdog device has to use it's own base address, clock source,
watchdog device, so add new davinci_wdt_device structure to hold
device data.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Guenter roeck <linux@roeck-us.net>
---
 drivers/watchdog/davinci_wdt.c |   74 ++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
index 2a94dde..4c3f417 100644
--- a/drivers/watchdog/davinci_wdt.c
+++ b/drivers/watchdog/davinci_wdt.c
@@ -56,51 +56,63 @@
 #define WDKEY_SEQ1		(0xda7e << 16)
 
 static int heartbeat;
-static void __iomem	*wdt_base;
-struct clk		*wdt_clk;
-static struct watchdog_device	wdt_wdd;
+
+/*
+ * struct to hold data for each WDT device
+ * @base - base io address of WD device
+ * @clk - source clock of WDT
+ * @wdd - hold watchdog device as is in WDT core
+ */
+struct davinci_wdt_device {
+	void __iomem		*base;
+	struct clk		*clk;
+	struct watchdog_device	wdd;
+};
 
 static int davinci_wdt_start(struct watchdog_device *wdd)
 {
 	u32 tgcr;
 	u32 timer_margin;
 	unsigned long wdt_freq;
+	struct davinci_wdt_device *davinci_wdt = watchdog_get_drvdata(wdd);
 
-	wdt_freq = clk_get_rate(wdt_clk);
+	wdt_freq = clk_get_rate(davinci_wdt->clk);
 
 	/* disable, internal clock source */
-	iowrite32(0, wdt_base + TCR);
+	iowrite32(0, davinci_wdt->base + TCR);
 	/* reset timer, set mode to 64-bit watchdog, and unreset */
-	iowrite32(0, wdt_base + TGCR);
+	iowrite32(0, davinci_wdt->base + TGCR);
 	tgcr = TIMMODE_64BIT_WDOG | TIM12RS_UNRESET | TIM34RS_UNRESET;
-	iowrite32(tgcr, wdt_base + TGCR);
+	iowrite32(tgcr, davinci_wdt->base + TGCR);
 	/* clear counter regs */
-	iowrite32(0, wdt_base + TIM12);
-	iowrite32(0, wdt_base + TIM34);
+	iowrite32(0, davinci_wdt->base + TIM12);
+	iowrite32(0, davinci_wdt->base + TIM34);
 	/* set timeout period */
 	timer_margin = (((u64)wdd->timeout * wdt_freq) & 0xffffffff);
-	iowrite32(timer_margin, wdt_base + PRD12);
+	iowrite32(timer_margin, davinci_wdt->base + PRD12);
 	timer_margin = (((u64)wdd->timeout * wdt_freq) >> 32);
-	iowrite32(timer_margin, wdt_base + PRD34);
+	iowrite32(timer_margin, davinci_wdt->base + PRD34);
 	/* enable run continuously */
-	iowrite32(ENAMODE12_PERIODIC, wdt_base + TCR);
+	iowrite32(ENAMODE12_PERIODIC, davinci_wdt->base + TCR);
 	/* Once the WDT is in pre-active state write to
 	 * TIM12, TIM34, PRD12, PRD34, TCR, TGCR, WDTCR are
 	 * write protected (except for the WDKEY field)
 	 */
 	/* put watchdog in pre-active state */
-	iowrite32(WDKEY_SEQ0 | WDEN, wdt_base + WDTCR);
+	iowrite32(WDKEY_SEQ0 | WDEN, davinci_wdt->base + WDTCR);
 	/* put watchdog in active state */
-	iowrite32(WDKEY_SEQ1 | WDEN, wdt_base + WDTCR);
+	iowrite32(WDKEY_SEQ1 | WDEN, davinci_wdt->base + WDTCR);
 	return 0;
 }
 
 static int davinci_wdt_ping(struct watchdog_device *wdd)
 {
+	struct davinci_wdt_device *davinci_wdt = watchdog_get_drvdata(wdd);
+
 	/* put watchdog in service state */
-	iowrite32(WDKEY_SEQ0, wdt_base + WDTCR);
+	iowrite32(WDKEY_SEQ0, davinci_wdt->base + WDTCR);
 	/* put watchdog in active state */
-	iowrite32(WDKEY_SEQ1, wdt_base + WDTCR);
+	iowrite32(WDKEY_SEQ1, davinci_wdt->base + WDTCR);
 	return 0;
 }
 
@@ -122,14 +134,21 @@ static int davinci_wdt_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource  *wdt_mem;
 	struct watchdog_device *wdd;
+	struct davinci_wdt_device *davinci_wdt;
+
+	davinci_wdt = devm_kzalloc(dev, sizeof(*davinci_wdt), GFP_KERNEL);
+	if (!davinci_wdt)
+		return -ENOMEM;
 
-	wdt_clk = devm_clk_get(dev, NULL);
-	if (WARN_ON(IS_ERR(wdt_clk)))
-		return PTR_ERR(wdt_clk);
+	davinci_wdt->clk = devm_clk_get(dev, NULL);
+	if (WARN_ON(IS_ERR(davinci_wdt->clk)))
+		return PTR_ERR(davinci_wdt->clk);
 
-	clk_prepare_enable(wdt_clk);
+	clk_prepare_enable(davinci_wdt->clk);
 
-	wdd			= &wdt_wdd;
+	platform_set_drvdata(pdev, davinci_wdt);
+
+	wdd			= &davinci_wdt->wdd;
 	wdd->info		= &davinci_wdt_info;
 	wdd->ops		= &davinci_wdt_ops;
 	wdd->min_timeout	= 1;
@@ -140,12 +159,13 @@ static int davinci_wdt_probe(struct platform_device *pdev)
 
 	dev_info(dev, "heartbeat %d sec\n", wdd->timeout);
 
+	watchdog_set_drvdata(wdd, davinci_wdt);
 	watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
 
 	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	wdt_base = devm_ioremap_resource(dev, wdt_mem);
-	if (IS_ERR(wdt_base))
-		return PTR_ERR(wdt_base);
+	davinci_wdt->base = devm_ioremap_resource(dev, wdt_mem);
+	if (IS_ERR(davinci_wdt->base))
+		return PTR_ERR(davinci_wdt->base);
 
 	ret = watchdog_register_device(wdd);
 	if (ret < 0)
@@ -156,8 +176,10 @@ static int davinci_wdt_probe(struct platform_device *pdev)
 
 static int davinci_wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&wdt_wdd);
-	clk_disable_unprepare(wdt_clk);
+	struct davinci_wdt_device *davinci_wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&davinci_wdt->wdd);
+	clk_disable_unprepare(davinci_wdt->clk);
 
 	return 0;
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 3/6] watchdog: davinci: add GET_TIMELEFT option support
  2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
  2013-11-25 14:04 ` [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core Ivan Khoronzhuk
  2013-11-25 14:04 ` [PATCH v3 2/6] watchdog: davinci: use davinci_wdt_device structure to hold device data Ivan Khoronzhuk
@ 2013-11-25 14:04 ` Ivan Khoronzhuk
  2013-11-25 14:04 ` [PATCH v3 4/6] watchdog: davinci: add "timeout-sec" property Ivan Khoronzhuk
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Ivan Khoronzhuk @ 2013-11-25 14:04 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko,
	Ivan Khoronzhuk

Currently, the davinci watchdog can be read while counting,
so we can add ability to report the remaining time before
the system will reboot.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/davinci_wdt.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
index 4c3f417..d342fc2 100644
--- a/drivers/watchdog/davinci_wdt.c
+++ b/drivers/watchdog/davinci_wdt.c
@@ -116,6 +116,31 @@ static int davinci_wdt_ping(struct watchdog_device *wdd)
 	return 0;
 }
 
+static unsigned int davinci_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	u64 timer_counter;
+	unsigned long freq;
+	u32 val;
+	struct davinci_wdt_device *davinci_wdt = watchdog_get_drvdata(wdd);
+
+	/* if timeout has occured then return 0 */
+	val = ioread32(davinci_wdt->base + WDTCR);
+	if (val & WDFLAG)
+		return 0;
+
+	freq = clk_get_rate(davinci_wdt->clk);
+
+	if (!freq)
+		return 0;
+
+	timer_counter = ioread32(davinci_wdt->base + TIM12);
+	timer_counter |= ((u64)ioread32(davinci_wdt->base + TIM34) << 32);
+
+	do_div(timer_counter, freq);
+
+	return wdd->timeout - timer_counter;
+}
+
 static const struct watchdog_info davinci_wdt_info = {
 	.options = WDIOF_KEEPALIVEPING,
 	.identity = "DaVinci Watchdog",
@@ -126,6 +151,7 @@ static const struct watchdog_ops davinci_wdt_ops = {
 	.start		= davinci_wdt_start,
 	.stop		= davinci_wdt_ping,
 	.ping		= davinci_wdt_ping,
+	.get_timeleft	= davinci_wdt_get_timeleft,
 };
 
 static int davinci_wdt_probe(struct platform_device *pdev)
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 4/6] watchdog: davinci: add "timeout-sec" property
  2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
                   ` (2 preceding siblings ...)
  2013-11-25 14:04 ` [PATCH v3 3/6] watchdog: davinci: add GET_TIMELEFT option support Ivan Khoronzhuk
@ 2013-11-25 14:04 ` Ivan Khoronzhuk
  2013-11-25 14:48 ` [PATCH v3 5/6] watchdog: davinci: reuse driver for keystone arch Ivan Khoronzhuk
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Ivan Khoronzhuk @ 2013-11-25 14:04 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko,
	Ivan Khoronzhuk

Since Davinci WDT has been switched to use WDT core, it became able
to support timeout-sec property, so add it to it's binding description.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 .../devicetree/bindings/watchdog/davinci-wdt.txt   |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
index 75558cc..e450134 100644
--- a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
@@ -4,9 +4,13 @@ Required properties:
 - compatible : Should be "ti,davinci-wdt"
 - reg : Should contain WDT registers location and length
 
+Optional properties:
+- timeout-sec : Contains the watchdog timeout in seconds
+
 Examples:
 
 wdt: wdt@2320000 {
 	compatible = "ti,davinci-wdt";
 	reg = <0x02320000 0x80>;
+	timeout-sec = <30>;
 };
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-25 14:04 ` [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core Ivan Khoronzhuk
@ 2013-11-25 14:16   ` Santosh Shilimkar
  2013-11-25 14:56     ` ivan.khoronzhuk
  2013-11-26 11:09   ` Grygorii Strashko
  2013-11-27  4:31   ` Sekhar Nori
  2 siblings, 1 reply; 16+ messages in thread
From: Santosh Shilimkar @ 2013-11-25 14:16 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko

Ivan,

On Monday 25 November 2013 09:04 AM, Ivan Khoronzhuk wrote:
> To reduce code duplicate and increase code readability use WDT core
> code to handle WDT interface.
> 
> Remove io_lock as the WDT core uses mutex to lock each wdt device.
> Remove wdt_state as the WDT core tracks state with its own variable.
> 
> The watchdog_init_timeout() can read timeout value from timeout-sec
> property if the passed value is out of bounds. The heartbeat is
> initialized in next way. If heartbeat is not set thought module
> parameter, try to read it's value from WDT node timeout-sec property.
> If node has no one, use default value.
> 
> The heartbeat is hold in wdd->timeout by WDT core, so use it in
> order to set timeout period.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Minor suggestion regarding the SOB line which applies to
rest of the series as well. Typically your SOB line
should be the last one like below.

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>

No need to repost the series just for that. The last
patch as I commented earlier, needs to be separate
patch which will go via linux-keystone tree.

Once the watchdog maintainer is happy with rest of the
series and applies it, I will pick last patch.

Regards,
Santosh


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 5/6] watchdog: davinci: reuse driver for keystone arch
  2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
                   ` (3 preceding siblings ...)
  2013-11-25 14:04 ` [PATCH v3 4/6] watchdog: davinci: add "timeout-sec" property Ivan Khoronzhuk
@ 2013-11-25 14:48 ` Ivan Khoronzhuk
  2013-11-25 14:54 ` [PATCH v3 6/6] arm: dts: keystone: add watchdog entry Ivan Khoronzhuk
  2013-11-26 19:40 ` [PATCH v3 0/6] Update Davinci watchdog driver Grygorii Strashko
  6 siblings, 0 replies; 16+ messages in thread
From: Ivan Khoronzhuk @ 2013-11-25 14:48 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel.Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko,
	Ivan Khoronzhuk

The keystone arch uses the same IP watchdog, so add "ti,keystone-wdt"
compatible and correct identity.

The Keystone arch is using clocks in DT and source clock for watchdog
has to be specified, so add this to binding.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 .../devicetree/bindings/watchdog/davinci-wdt.txt   |   12 ++++++++++--
 drivers/watchdog/Kconfig                           |    4 ++--
 drivers/watchdog/davinci_wdt.c                     |    2 +-
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
index e450134..4d89317 100644
--- a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
@@ -1,11 +1,18 @@
-DaVinci Watchdog Timer (WDT) Controller
+Texas Instruments DaVinci/Keystone Watchdog Timer (WDT) Controller
 
 Required properties:
-- compatible : Should be "ti,davinci-wdt"
+- compatible : Should be "ti,davinci-wdt" or "ti,keystone-wdt"
 - reg : Should contain WDT registers location and length
 
 Optional properties:
 - timeout-sec : Contains the watchdog timeout in seconds
+- clocks : the clock feeding the watchdog timer.
+	   Needed if platform uses clocks.
+	   See clock-bindings.txt
+
+Documentation:
+Davinci DM646x - http://www.ti.com/lit/ug/spruer5b/spruer5b.pdf
+Keystone - http://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf
 
 Examples:
 
@@ -13,4 +20,5 @@ wdt: wdt@2320000 {
 	compatible = "ti,davinci-wdt";
 	reg = <0x02320000 0x80>;
 	timeout-sec = <30>;
+	clocks = <&clkwdtimer0>;
 };
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index eb8c89d..01f3f81 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -270,12 +270,12 @@ config IOP_WATCHDOG
 
 config DAVINCI_WATCHDOG
 	tristate "DaVinci watchdog"
-	depends on ARCH_DAVINCI
+	depends on ARCH_DAVINCI || ARCH_KEYSTONE
 	select WATCHDOG_CORE
 	select WATCHDOG_NOWAYOUT
 	help
 	  Say Y here if to include support for the watchdog timer
-	  in the DaVinci DM644x/DM646x processors.
+	  in the DaVinci DM644x/DM646x or Keystone processors.
 	  To compile this driver as a module, choose M here: the
 	  module will be called davinci_wdt.
 
diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
index d342fc2..9128c74 100644
--- a/drivers/watchdog/davinci_wdt.c
+++ b/drivers/watchdog/davinci_wdt.c
@@ -143,7 +143,7 @@ static unsigned int davinci_wdt_get_timeleft(struct watchdog_device *wdd)
 
 static const struct watchdog_info davinci_wdt_info = {
 	.options = WDIOF_KEEPALIVEPING,
-	.identity = "DaVinci Watchdog",
+	.identity = "DaVinci/Keystone Watchdog",
 };
 
 static const struct watchdog_ops davinci_wdt_ops = {
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 6/6] arm: dts: keystone: add watchdog entry
  2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
                   ` (4 preceding siblings ...)
  2013-11-25 14:48 ` [PATCH v3 5/6] watchdog: davinci: reuse driver for keystone arch Ivan Khoronzhuk
@ 2013-11-25 14:54 ` Ivan Khoronzhuk
  2013-11-26 19:40 ` [PATCH v3 0/6] Update Davinci watchdog driver Grygorii Strashko
  6 siblings, 0 replies; 16+ messages in thread
From: Ivan Khoronzhuk @ 2013-11-25 14:54 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel.Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko,
	Ivan Khoronzhuk

Add watchdog entry to keystone device tree.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/arm/boot/dts/keystone.dtsi |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index f6d6d9e..54a8ff7 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -181,5 +181,11 @@
 			interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
 			clocks = <&clkspi>;
 		};
+
+		wdt: wdt@022f0080 {
+			compatible = "ti,keystone-wdt","ti,davinci-wdt";
+			reg = <0x022f0080 0x80>;
+			clocks = <&clkwdtimer0>;
+		};
 	};
 };
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-25 14:16   ` Santosh Shilimkar
@ 2013-11-25 14:56     ` ivan.khoronzhuk
  0 siblings, 0 replies; 16+ messages in thread
From: ivan.khoronzhuk @ 2013-11-25 14:56 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely, grygorii.strashko

On 11/25/2013 04:16 PM, Santosh Shilimkar wrote:
> Ivan,
>
> On Monday 25 November 2013 09:04 AM, Ivan Khoronzhuk wrote:
>> To reduce code duplicate and increase code readability use WDT core
>> code to handle WDT interface.
>>
>> Remove io_lock as the WDT core uses mutex to lock each wdt device.
>> Remove wdt_state as the WDT core tracks state with its own variable.
>>
>> The watchdog_init_timeout() can read timeout value from timeout-sec
>> property if the passed value is out of bounds. The heartbeat is
>> initialized in next way. If heartbeat is not set thought module
>> parameter, try to read it's value from WDT node timeout-sec property.
>> If node has no one, use default value.
>>
>> The heartbeat is hold in wdd->timeout by WDT core, so use it in
>> order to set timeout period.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> Minor suggestion regarding the SOB line which applies to
> rest of the series as well. Typically your SOB line
> should be the last one like below.
>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>
> No need to repost the series just for that. The last
> patch as I commented earlier, needs to be separate
> patch which will go via linux-keystone tree.
>
> Once the watchdog maintainer is happy with rest of the
> series and applies it, I will pick last patch.
>
> Regards,
> Santosh
>

Thanks, Santosh
I'll take note.

-- 
Regards,
Ivan Khoronzhuk

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-25 14:04 ` [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core Ivan Khoronzhuk
  2013-11-25 14:16   ` Santosh Shilimkar
@ 2013-11-26 11:09   ` Grygorii Strashko
  2013-11-26 12:10     ` Guenter Roeck
  2013-11-27  4:31   ` Sekhar Nori
  2 siblings, 1 reply; 16+ messages in thread
From: Grygorii Strashko @ 2013-11-26 11:09 UTC (permalink / raw)
  To: Ivan Khoronzhuk, Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely

On 11/25/2013 04:04 PM, Ivan Khoronzhuk wrote:
> To reduce code duplicate and increase code readability use WDT core
> code to handle WDT interface.
>
> Remove io_lock as the WDT core uses mutex to lock each wdt device.
> Remove wdt_state as the WDT core tracks state with its own variable.
>
> The watchdog_init_timeout() can read timeout value from timeout-sec
> property if the passed value is out of bounds. The heartbeat is
> initialized in next way. If heartbeat is not set thought module
> parameter, try to read it's value from WDT node timeout-sec property.
> If node has no one, use default value.
>
> The heartbeat is hold in wdd->timeout by WDT core, so use it in
> order to set timeout period.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   arch/arm/mach-davinci/da8xx-dt.c      |    2 +-
>   arch/arm/mach-davinci/devices-da8xx.c |    4 +-
>   arch/arm/mach-davinci/devices.c       |    2 +-
>   drivers/watchdog/Kconfig              |    2 +
>   drivers/watchdog/davinci_wdt.c        |  151 ++++++++-------------------------
>   5 files changed, 43 insertions(+), 118 deletions(-)

Pls note, that this patch contains both changes in platform and driver 
code to fix regression reported by Sekhar Nori on v2 of this series and 
caused by driver name changing.

Is it ok?

It can be splitted, but then both patches will introduce regression by 
itself and thing will work only if both of them will be applied together.

>
> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
> index d2bc574..ed19287 100644
[...]
> -MODULE_ALIAS("platform:watchdog");
> +MODULE_ALIAS("platform:davinci-wdt");
>
Regards,
-Grygorii


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-26 11:09   ` Grygorii Strashko
@ 2013-11-26 12:10     ` Guenter Roeck
  2013-11-26 12:30       ` Grygorii Strashko
  0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2013-11-26 12:10 UTC (permalink / raw)
  To: Grygorii Strashko, Ivan Khoronzhuk, Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely

On 11/26/2013 03:09 AM, Grygorii Strashko wrote:
> On 11/25/2013 04:04 PM, Ivan Khoronzhuk wrote:
>> To reduce code duplicate and increase code readability use WDT core
>> code to handle WDT interface.
>>
>> Remove io_lock as the WDT core uses mutex to lock each wdt device.
>> Remove wdt_state as the WDT core tracks state with its own variable.
>>
>> The watchdog_init_timeout() can read timeout value from timeout-sec
>> property if the passed value is out of bounds. The heartbeat is
>> initialized in next way. If heartbeat is not set thought module
>> parameter, try to read it's value from WDT node timeout-sec property.
>> If node has no one, use default value.
>>
>> The heartbeat is hold in wdd->timeout by WDT core, so use it in
>> order to set timeout period.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   arch/arm/mach-davinci/da8xx-dt.c      |    2 +-
>>   arch/arm/mach-davinci/devices-da8xx.c |    4 +-
>>   arch/arm/mach-davinci/devices.c       |    2 +-
>>   drivers/watchdog/Kconfig              |    2 +
>>   drivers/watchdog/davinci_wdt.c        |  151 ++++++++-------------------------
>>   5 files changed, 43 insertions(+), 118 deletions(-)
>
> Pls note, that this patch contains both changes in platform and driver code to fix regression reported by Sekhar Nori on v2 of this series and caused by driver name changing.
>
> Is it ok?
>
> It can be splitted, but then both patches will introduce regression by itself and thing will work only if both of them will be applied together.
>
Splitting it would break bisect, even if applied together, so I don't think this would be a good idea.

Guenter


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-26 12:10     ` Guenter Roeck
@ 2013-11-26 12:30       ` Grygorii Strashko
  0 siblings, 0 replies; 16+ messages in thread
From: Grygorii Strashko @ 2013-11-26 12:30 UTC (permalink / raw)
  To: Guenter Roeck, Ivan Khoronzhuk, Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely

On 11/26/2013 02:10 PM, Guenter Roeck wrote:
> On 11/26/2013 03:09 AM, Grygorii Strashko wrote:
>> On 11/25/2013 04:04 PM, Ivan Khoronzhuk wrote:
>>> To reduce code duplicate and increase code readability use WDT core
>>> code to handle WDT interface.
>>>
>>> Remove io_lock as the WDT core uses mutex to lock each wdt device.
>>> Remove wdt_state as the WDT core tracks state with its own variable.
>>>
>>> The watchdog_init_timeout() can read timeout value from timeout-sec
>>> property if the passed value is out of bounds. The heartbeat is
>>> initialized in next way. If heartbeat is not set thought module
>>> parameter, try to read it's value from WDT node timeout-sec property.
>>> If node has no one, use default value.
>>>
>>> The heartbeat is hold in wdd->timeout by WDT core, so use it in
>>> order to set timeout period.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   arch/arm/mach-davinci/da8xx-dt.c      |    2 +-
>>>   arch/arm/mach-davinci/devices-da8xx.c |    4 +-
>>>   arch/arm/mach-davinci/devices.c       |    2 +-
>>>   drivers/watchdog/Kconfig              |    2 +
>>>   drivers/watchdog/davinci_wdt.c        |  151
>>> ++++++++-------------------------
>>>   5 files changed, 43 insertions(+), 118 deletions(-)
>>
>> Pls note, that this patch contains both changes in platform and driver
>> code to fix regression reported by Sekhar Nori on v2 of this series
>> and caused by driver name changing.
>>
>> Is it ok?
>>
>> It can be splitted, but then both patches will introduce regression by
>> itself and thing will work only if both of them will be applied together.
>>
> Splitting it would break bisect, even if applied together, so I don't
> think this would be a good idea.

Thanks

Regards,
-grygorii

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/6] Update Davinci watchdog driver
  2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
                   ` (5 preceding siblings ...)
  2013-11-25 14:54 ` [PATCH v3 6/6] arm: dts: keystone: add watchdog entry Ivan Khoronzhuk
@ 2013-11-26 19:40 ` Grygorii Strashko
  6 siblings, 0 replies; 16+ messages in thread
From: Grygorii Strashko @ 2013-11-26 19:40 UTC (permalink / raw)
  To: Ivan Khoronzhuk, Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, nsekhar, linux-watchdog,
	devicetree, Pawel Moll, swarren, ijc+devicetree, galak,
	rob.herring, linux-kernel, grant.likely

Hi Sekhar,

On 11/25/2013 04:04 PM, Ivan Khoronzhuk wrote:
> These patches are intended to update Davinci watchdog to use WDT core
> and reuse driver for keystone arch, because Keystone uses the similar
> IP like Davinci.
> 
> See Documentation:
> Davinci DM646x - http://www.ti.com/lit/ug/spruer5b/spruer5b.pdf
> Keystone - http://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf
> 
> Also:
>   - improved to support GET_TIMELEFT option.
>   - added "clocks" and "timeout-sec" properties to DT.
> 
> Based on
> git://git.kernel.org/pub/scm/linux/kernel/git/ssantosh/linux-keystone.git
> keystone/master
> 

Could you confirm that regression detected in v2 of series is gone?

Also would it be acceptable if this series will go via WDT tree?
- The first patch patch in series contains changes in both
 Davinci platform & wdt driver.

> v2..v3:
> - watchdog: davinci: change driver to use WDT core
> 	removed "ti,keystone-wdt" compatible from driver
> 	renamed wdt device names for mach-davinci from
> 		"watchdog" to "davinci-wdt"
> 
> - watchdog: davinci: reuse driver for keystone arch
> 	moved "clocks" property under "Optional properties"
> 	improved decsription of "clocks" property
> 
> - arm: dts: keystone: add watchdog entry
> 	replaced "ti,keystone-wdt" compatible on
> 		 "ti,keystone-wdt","ti,davinci-wdt"
> 
> Ivan Khoronzhuk (6):
>    watchdog: davinci: change driver to use WDT core
>    watchdog: davinci: use davinci_wdt_device structure to hold device
>      data
>    watchdog: davinci: add GET_TIMELEFT option support
>    watchdog: davinci: add "timeout-sec" property
>    watchdog: davinci: reuse driver for keystone arch
>    arm: dts: keystone: add watchdog entry
> 
>   .../devicetree/bindings/watchdog/davinci-wdt.txt   |   16 +-
>   arch/arm/boot/dts/keystone.dtsi                    |    6 +
>   arch/arm/mach-davinci/da8xx-dt.c                   |    2 +-
>   arch/arm/mach-davinci/devices-da8xx.c              |    4 +-
>   arch/arm/mach-davinci/devices.c                    |    2 +-
>   drivers/watchdog/Kconfig                           |    6 +-
>   drivers/watchdog/davinci_wdt.c                     |  225 +++++++++-----------
>   7 files changed, 126 insertions(+), 135 deletions(-)
> 

Thanks,
- grygorii

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-25 14:04 ` [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core Ivan Khoronzhuk
  2013-11-25 14:16   ` Santosh Shilimkar
  2013-11-26 11:09   ` Grygorii Strashko
@ 2013-11-27  4:31   ` Sekhar Nori
  2013-11-27  7:04     ` Guenter Roeck
  2 siblings, 1 reply; 16+ messages in thread
From: Sekhar Nori @ 2013-11-27  4:31 UTC (permalink / raw)
  To: Ivan Khoronzhuk, Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, linux-watchdog, devicetree,
	Pawel Moll, swarren, ijc+devicetree, galak, rob.herring,
	linux-kernel, grant.likely, grygorii.strashko

On Monday 25 November 2013 07:34 PM, Ivan Khoronzhuk wrote:
> To reduce code duplicate and increase code readability use WDT core
> code to handle WDT interface.
> 
> Remove io_lock as the WDT core uses mutex to lock each wdt device.
> Remove wdt_state as the WDT core tracks state with its own variable.
> 
> The watchdog_init_timeout() can read timeout value from timeout-sec
> property if the passed value is out of bounds. The heartbeat is
> initialized in next way. If heartbeat is not set thought module
> parameter, try to read it's value from WDT node timeout-sec property.
> If node has no one, use default value.
> 
> The heartbeat is hold in wdd->timeout by WDT core, so use it in
> order to set timeout period.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

So this still causes a regression because the clk_get()
fails due to the changed device name. Please fold this
patch in (tested on DM365, compiled on rest).

Also, the change in device name (while needed) does not seem
to be related to usage of wdt core. Can you may be split
that change into a separate patch?

Thanks,
Sekhar

---8<---
diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
index 0813b51..82c6013 100644
--- a/arch/arm/mach-davinci/da830.c
+++ b/arch/arm/mach-davinci/da830.c
@@ -385,7 +385,7 @@ static struct clk_lookup da830_clks[] = {
 	CLK(NULL,		"pll0_sysclk7",	&pll0_sysclk7),
 	CLK("i2c_davinci.1",	NULL,		&i2c0_clk),
 	CLK(NULL,		"timer0",	&timerp64_0_clk),
-	CLK("watchdog",		NULL,		&timerp64_1_clk),
+	CLK("davinci-wdt",	NULL,		&timerp64_1_clk),
 	CLK(NULL,		"arm_rom",	&arm_rom_clk),
 	CLK(NULL,		"scr0_ss",	&scr0_ss_clk),
 	CLK(NULL,		"scr1_ss",	&scr1_ss_clk),
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 352984e..ccb2f58 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -443,7 +443,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK(NULL,		"pll1_sysclk3",	&pll1_sysclk3),
 	CLK("i2c_davinci.1",	NULL,		&i2c0_clk),
 	CLK(NULL,		"timer0",	&timerp64_0_clk),
-	CLK("watchdog",		NULL,		&timerp64_1_clk),
+	CLK("davinci-wdt",	NULL,		&timerp64_1_clk),
 	CLK(NULL,		"arm_rom",	&arm_rom_clk),
 	CLK(NULL,		"tpcc0",	&tpcc0_clk),
 	CLK(NULL,		"tptc0",	&tptc0_clk),
diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
index ef9ff1f..4bb8132 100644
--- a/arch/arm/mach-davinci/dm355.c
+++ b/arch/arm/mach-davinci/dm355.c
@@ -375,7 +375,7 @@ static struct clk_lookup dm355_clks[] = {
 	CLK(NULL, "pwm3", &pwm3_clk),
 	CLK(NULL, "timer0", &timer0_clk),
 	CLK(NULL, "timer1", &timer1_clk),
-	CLK("watchdog", NULL, &timer2_clk),
+	CLK("davinci-wdt", NULL, &timer2_clk),
 	CLK(NULL, "timer3", &timer3_clk),
 	CLK(NULL, "rto", &rto_clk),
 	CLK(NULL, "usb", &usb_clk),
diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
index 1511a06..b0e8df3 100644
--- a/arch/arm/mach-davinci/dm365.c
+++ b/arch/arm/mach-davinci/dm365.c
@@ -473,7 +473,7 @@ static struct clk_lookup dm365_clks[] = {
 	CLK(NULL, "pwm3", &pwm3_clk),
 	CLK(NULL, "timer0", &timer0_clk),
 	CLK(NULL, "timer1", &timer1_clk),
-	CLK("watchdog", NULL, &timer2_clk),
+	CLK("davinci-wdt", NULL, &timer2_clk),
 	CLK(NULL, "timer3", &timer3_clk),
 	CLK(NULL, "usb", &usb_clk),
 	CLK("davinci_emac.1", NULL, &emac_clk),
diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
index 143a321..aeaad95 100644
--- a/arch/arm/mach-davinci/dm644x.c
+++ b/arch/arm/mach-davinci/dm644x.c
@@ -322,7 +322,7 @@ static struct clk_lookup dm644x_clks[] = {
 	CLK(NULL, "pwm2", &pwm2_clk),
 	CLK(NULL, "timer0", &timer0_clk),
 	CLK(NULL, "timer1", &timer1_clk),
-	CLK("watchdog", NULL, &timer2_clk),
+	CLK("davinci-wdt", NULL, &timer2_clk),
 	CLK(NULL, NULL, NULL),
 };
 
diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
index 2a73f29..a56779b 100644
--- a/arch/arm/mach-davinci/dm646x.c
+++ b/arch/arm/mach-davinci/dm646x.c
@@ -356,7 +356,7 @@ static struct clk_lookup dm646x_clks[] = {
 	CLK(NULL, "pwm1", &pwm1_clk),
 	CLK(NULL, "timer0", &timer0_clk),
 	CLK(NULL, "timer1", &timer1_clk),
-	CLK("watchdog", NULL, &timer2_clk),
+	CLK("davinci-wdt", NULL, &timer2_clk),
 	CLK("palm_bk3710", NULL, &ide_clk),
 	CLK(NULL, "vpif0", &vpif0_clk),
 	CLK(NULL, "vpif1", &vpif1_clk),


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-27  4:31   ` Sekhar Nori
@ 2013-11-27  7:04     ` Guenter Roeck
  2013-11-27 10:03       ` ivan.khoronzhuk
  0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2013-11-27  7:04 UTC (permalink / raw)
  To: Sekhar Nori, Ivan Khoronzhuk, Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, linux-watchdog, devicetree,
	Pawel Moll, swarren, ijc+devicetree, galak, rob.herring,
	linux-kernel, grant.likely, grygorii.strashko

On 11/26/2013 08:31 PM, Sekhar Nori wrote:
> On Monday 25 November 2013 07:34 PM, Ivan Khoronzhuk wrote:
>> To reduce code duplicate and increase code readability use WDT core
>> code to handle WDT interface.
>>
>> Remove io_lock as the WDT core uses mutex to lock each wdt device.
>> Remove wdt_state as the WDT core tracks state with its own variable.
>>
>> The watchdog_init_timeout() can read timeout value from timeout-sec
>> property if the passed value is out of bounds. The heartbeat is
>> initialized in next way. If heartbeat is not set thought module
>> parameter, try to read it's value from WDT node timeout-sec property.
>> If node has no one, use default value.
>>
>> The heartbeat is hold in wdd->timeout by WDT core, so use it in
>> order to set timeout period.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> So this still causes a regression because the clk_get()
> fails due to the changed device name. Please fold this
> patch in (tested on DM365, compiled on rest).
>
> Also, the change in device name (while needed) does not seem
> to be related to usage of wdt core. Can you may be split
> that change into a separate patch?
>

Given all the trouble with it, I think that would be a good idea.

Guenter


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core
  2013-11-27  7:04     ` Guenter Roeck
@ 2013-11-27 10:03       ` ivan.khoronzhuk
  0 siblings, 0 replies; 16+ messages in thread
From: ivan.khoronzhuk @ 2013-11-27 10:03 UTC (permalink / raw)
  To: Guenter Roeck, Sekhar Nori, Santosh Shilimkar
  Cc: linux-arm-kernel, mark.rutland, wim, linux-watchdog, devicetree,
	Pawel Moll, swarren, ijc+devicetree, galak, rob.herring,
	linux-kernel, grant.likely, grygorii.strashko

On 11/27/2013 09:04 AM, Guenter Roeck wrote:
> On 11/26/2013 08:31 PM, Sekhar Nori wrote:
>> On Monday 25 November 2013 07:34 PM, Ivan Khoronzhuk wrote:
>>> To reduce code duplicate and increase code readability use WDT core
>>> code to handle WDT interface.
>>>
>>> Remove io_lock as the WDT core uses mutex to lock each wdt device.
>>> Remove wdt_state as the WDT core tracks state with its own variable.
>>>
>>> The watchdog_init_timeout() can read timeout value from timeout-sec
>>> property if the passed value is out of bounds. The heartbeat is
>>> initialized in next way. If heartbeat is not set thought module
>>> parameter, try to read it's value from WDT node timeout-sec property.
>>> If node has no one, use default value.
>>>
>>> The heartbeat is hold in wdd->timeout by WDT core, so use it in
>>> order to set timeout period.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> So this still causes a regression because the clk_get()
>> fails due to the changed device name. Please fold this
>> patch in (tested on DM365, compiled on rest).
>>
>> Also, the change in device name (while needed) does not seem
>> to be related to usage of wdt core. Can you may be split
>> that change into a separate patch?
>>
>
> Given all the trouble with it, I think that would be a good idea.
>
> Guenter
>

Ok, I'll split it for convenience.

-- 
Regards,
Ivan Khoronzhuk

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-11-27 10:04 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-25 14:04 [PATCH v3 0/6] Update Davinci watchdog driver Ivan Khoronzhuk
2013-11-25 14:04 ` [PATCH v3 1/6] watchdog: davinci: change driver to use WDT core Ivan Khoronzhuk
2013-11-25 14:16   ` Santosh Shilimkar
2013-11-25 14:56     ` ivan.khoronzhuk
2013-11-26 11:09   ` Grygorii Strashko
2013-11-26 12:10     ` Guenter Roeck
2013-11-26 12:30       ` Grygorii Strashko
2013-11-27  4:31   ` Sekhar Nori
2013-11-27  7:04     ` Guenter Roeck
2013-11-27 10:03       ` ivan.khoronzhuk
2013-11-25 14:04 ` [PATCH v3 2/6] watchdog: davinci: use davinci_wdt_device structure to hold device data Ivan Khoronzhuk
2013-11-25 14:04 ` [PATCH v3 3/6] watchdog: davinci: add GET_TIMELEFT option support Ivan Khoronzhuk
2013-11-25 14:04 ` [PATCH v3 4/6] watchdog: davinci: add "timeout-sec" property Ivan Khoronzhuk
2013-11-25 14:48 ` [PATCH v3 5/6] watchdog: davinci: reuse driver for keystone arch Ivan Khoronzhuk
2013-11-25 14:54 ` [PATCH v3 6/6] arm: dts: keystone: add watchdog entry Ivan Khoronzhuk
2013-11-26 19:40 ` [PATCH v3 0/6] Update Davinci watchdog driver Grygorii Strashko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).