[PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Paul Walmsley]

The OMAP watchdog timer driver directly calls a function exported by
code in arch/arm/mach-omap2.  This is not good; it tightly couples
this driver to the mach-omap2 integration code.  Instead, add a
temporary platform_data function pointer to abstract this function
call.  A subsequent patch will convert the watchdog driver to use this
function pointer.

This patch also moves the device creation code out of
arch/arm/mach-omap2/devices.c and into arch/arm/mach-omap2/wd_timer.c.
This is another step towards the removal of
arch/arm/mach-omap2/devices.c.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
---
 arch/arm/mach-omap1/devices.c               |   21 +++++++++++++--
 arch/arm/mach-omap2/devices.c               |   26 ------------------
 arch/arm/mach-omap2/wd_timer.c              |   33 +++++++++++++++++++++++
 include/linux/platform_data/omap-wd-timer.h |   38 +++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/platform_data/omap-wd-timer.h

diff --git a/arch/arm/mach-omap1/devices.c b/arch/arm/mach-omap1/devices.c
index d3fec92..c3408e7 100644
--- a/arch/arm/mach-omap1/devices.c
+++ b/arch/arm/mach-omap1/devices.c
@@ -17,6 +17,8 @@
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
 
+#include <linux/platform_data/omap-wd-timer.h>
+
 #include <asm/mach/map.h>
 
 #include <plat/tc.h>
@@ -439,18 +441,31 @@ static struct resource wdt_resources[] = {
 };
 
 static struct platform_device omap_wdt_device = {
-	.name	   = "omap_wdt",
-	.id	     = -1,
+	.name		= "omap_wdt",
+	.id		= -1,
 	.num_resources	= ARRAY_SIZE(wdt_resources),
 	.resource	= wdt_resources,
 };
 
 static int __init omap_init_wdt(void)
 {
+	struct omap_wd_timer_platform_data pdata;
+	int ret;
+
 	if (!cpu_is_omap16xx())
 		return -ENODEV;
 
-	return platform_device_register(&omap_wdt_device);
+	pdata.read_reset_sources = omap1_read_reset_sources;
+
+	ret = platform_device_register(&omap_wdt_device);
+	if (!ret) {
+		ret = platform_device_add_data(&omap_wdt_device, &pdata,
+					       sizeof(pdata));
+		if (ret)
+			platform_device_del(&omap_wdt_device);
+	}
+
+	return ret;
 }
 subsys_initcall(omap_init_wdt);
 #endif
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index c8c2117..2ab2c99 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -644,29 +644,3 @@ static int __init omap2_init_devices(void)
 	return 0;
 }
 arch_initcall(omap2_init_devices);
-
-#if defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
-static int __init omap_init_wdt(void)
-{
-	int id = -1;
-	struct platform_device *pdev;
-	struct omap_hwmod *oh;
-	char *oh_name = "wd_timer2";
-	char *dev_name = "omap_wdt";
-
-	if (!cpu_class_is_omap2() || of_have_populated_dt())
-		return 0;
-
-	oh = omap_hwmod_lookup(oh_name);
-	if (!oh) {
-		pr_err("Could not look up wd_timer%d hwmod\n", id);
-		return -EINVAL;
-	}
-
-	pdev = omap_device_build(dev_name, id, oh, NULL, 0, NULL, 0, 0);
-	WARN(IS_ERR(pdev), "Can't build omap_device for %s:%s.\n",
-				dev_name, oh->name);
-	return 0;
-}
-subsys_initcall(omap_init_wdt);
-#endif
diff --git a/arch/arm/mach-omap2/wd_timer.c b/arch/arm/mach-omap2/wd_timer.c
index b2f1c67..00ef54c 100644
--- a/arch/arm/mach-omap2/wd_timer.c
+++ b/arch/arm/mach-omap2/wd_timer.c
@@ -11,10 +11,14 @@
 #include <linux/io.h>
 #include <linux/err.h>
 
+#include <linux/platform_data/omap-wd-timer.h>
+
 #include <plat/omap_hwmod.h>
+#include <plat/omap_device.h>
 
 #include "wd_timer.h"
 #include "common.h"
+#include "prm.h"
 
 /*
  * In order to avoid any assumptions from bootloader regarding WDT
@@ -99,3 +103,32 @@ int omap2_wd_timer_reset(struct omap_hwmod *oh)
 	return (c == MAX_MODULE_SOFTRESET_WAIT) ? -ETIMEDOUT :
 		omap2_wd_timer_disable(oh);
 }
+
+static int __init omap_init_wdt(void)
+{
+	int id = -1;
+	struct platform_device *pdev;
+	struct omap_hwmod *oh;
+	char *oh_name = "wd_timer2";
+	char *dev_name = "omap_wdt";
+	struct omap_wd_timer_platform_data pdata;
+
+	if (!cpu_class_is_omap2())
+		return 0;
+
+	oh = omap_hwmod_lookup(oh_name);
+	if (!oh) {
+		pr_err("Could not look up wd_timer%d hwmod\n", id);
+		return -EINVAL;
+	}
+
+	pdata.read_reset_sources = prm_read_reset_sources;
+
+	pdev = omap_device_build(dev_name, id, oh, &pdata,
+				 sizeof(struct omap_wd_timer_platform_data),
+				 NULL, 0, 0);
+	WARN(IS_ERR(pdev), "Can't build omap_device for %s:%s.\n",
+	     dev_name, oh->name);
+	return 0;
+}
+subsys_initcall(omap_init_wdt);
diff --git a/include/linux/platform_data/omap-wd-timer.h b/include/linux/platform_data/omap-wd-timer.h
new file mode 100644
index 0000000..d75f5f8
--- /dev/null
+++ b/include/linux/platform_data/omap-wd-timer.h
@@ -0,0 +1,38 @@
+/*
+ * OMAP2+ WDTIMER-specific function prototypes
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ * Paul Walmsley
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_PLATFORM_DATA_OMAP_WD_TIMER_H
+#define __LINUX_PLATFORM_DATA_OMAP_WD_TIMER_H
+
+#include <linux/types.h>
+
+/*
+ * Standardized OMAP reset source bits
+ *
+ * This is a subset of the ones listed in arch/arm/mach-omap2/prm.h
+ * and are the only ones needed in the watchdog driver.
+ */
+#define OMAP_MPU_WD_RST_SRC_ID_SHIFT				3
+
+/**
+ * struct omap_wd_timer_platform_data - WDTIMER integration to the host SoC
+ * @read_reset_sources - fn ptr for the SoC to indicate the last reset cause
+ *
+ * The function pointed to by @read_reset_sources must return its data
+ * in a standard format - search for RST_SRC_ID_SHIFT in
+ * arch/arm/mach-omap2
+ */
+struct omap_wd_timer_platform_data {
+	u32 (*read_reset_sources)(void);
+};
+
+#endif

[PATCH 6/7] watchdog: OMAP: use standard GETBOOTSTATUS interface; use platform_data fn ptr

[Paul Walmsley]

Previously the OMAP watchdog driver used a non-standard way to report
the chip reset source via the GETBOOTSTATUS ioctl.  This patch
converts the driver to use the standard WDIOF_* flags for this
purpose.

This patch may break existing userspace code that uses the existing
non-standard data format returned by the OMAP watchdog driver's
GETBOOTSTATUS ioctl.  To fetch detailed reset source information,
userspace code will need to retrieve it directly from the CGRM or PRM
drivers when those are completed.

Previously, to fetch the reset source, the driver either read a
register outside the watchdog IP block (OMAP1), or called a function
exported directly from arch/arm/mach-omap2.  Both approaches are
broken.  This patch also converts the driver to use a platform_data
function pointer.  This approach is temporary, and is due to the lack
of drivers for the OMAP16xx+ Clock Generation and Reset Management IP
block and the OMAP2+ Power and Reset Management IP block.  Once
drivers are available for those IP blocks, the watchdog driver can be
converted to call exported drivers from those functions directly.
At that point, the platform_data function pointer can be removed.

In the short term, this patch is needed to allow the PRM code to be
removed from arch/arm/mach-omap2 (it is being moved to a driver).

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
---
 drivers/watchdog/omap_wdt.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index f5db18db..5d33bc0 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -46,8 +46,8 @@
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
 #include <mach/hardware.h>
-#include <plat/cpu.h>
-#include <plat/prcm.h>
+
+#include <linux/platform_data/omap-wd-timer.h>
 
 #include "omap_wdt.h"
 
@@ -202,8 +202,10 @@ static ssize_t omap_wdt_write(struct file *file, const char __user *data,
 static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 						unsigned long arg)
 {
+	struct omap_wd_timer_platform_data *pdata;
 	struct omap_wdt_dev *wdev;
-	int new_margin;
+	u32 rs;
+	int new_margin, bs;
 	static const struct watchdog_info ident = {
 		.identity = "OMAP Watchdog",
 		.options = WDIOF_SETTIMEOUT,
@@ -211,6 +213,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 	};
 
 	wdev = file->private_data;
+	pdata = wdev->dev->platform_data;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
@@ -219,17 +222,12 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 	case WDIOC_GETSTATUS:
 		return put_user(0, (int __user *)arg);
 	case WDIOC_GETBOOTSTATUS:
-#ifdef CONFIG_ARCH_OMAP1
-		if (cpu_is_omap16xx())
-			return put_user(__raw_readw(ARM_SYSST),
-					(int __user *)arg);
-#endif
-#ifdef CONFIG_ARCH_OMAP2PLUS
-		if (cpu_is_omap24xx())
-			return put_user(omap_prcm_get_reset_sources(),
-					(int __user *)arg);
-#endif
-		return put_user(0, (int __user *)arg);
+		if (!pdata->read_reset_sources)
+			return put_user(0, (int __user *)arg);
+		rs = pdata->read_reset_sources();
+		bs = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
+			WDIOF_CARDRESET : 0;
+		return put_user(bs, (int __user *)arg);
 	case WDIOC_KEEPALIVE:
 		spin_lock(&wdt_lock);
 		omap_wdt_ping(wdev);

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Wim Van Sebroeck]

Hi Paul,

> The OMAP watchdog timer driver directly calls a function exported by
> code in arch/arm/mach-omap2.  This is not good; it tightly couples
> this driver to the mach-omap2 integration code.  Instead, add a
> temporary platform_data function pointer to abstract this function
> call.  A subsequent patch will convert the watchdog driver to use this
> function pointer.
> 
> This patch also moves the device creation code out of
> arch/arm/mach-omap2/devices.c and into arch/arm/mach-omap2/wd_timer.c.
> This is another step towards the removal of
> arch/arm/mach-omap2/devices.c.
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>

Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

Kind regards,
Wim.

Re: [PATCH 6/7] watchdog: OMAP: use standard GETBOOTSTATUS interface; use platform_data fn ptr

[Wim Van Sebroeck]

Hi Paul,

> Previously the OMAP watchdog driver used a non-standard way to report
> the chip reset source via the GETBOOTSTATUS ioctl.  This patch
> converts the driver to use the standard WDIOF_* flags for this
> purpose.
> 
> This patch may break existing userspace code that uses the existing
> non-standard data format returned by the OMAP watchdog driver's
> GETBOOTSTATUS ioctl.  To fetch detailed reset source information,
> userspace code will need to retrieve it directly from the CGRM or PRM
> drivers when those are completed.
> 
> Previously, to fetch the reset source, the driver either read a
> register outside the watchdog IP block (OMAP1), or called a function
> exported directly from arch/arm/mach-omap2.  Both approaches are
> broken.  This patch also converts the driver to use a platform_data
> function pointer.  This approach is temporary, and is due to the lack
> of drivers for the OMAP16xx+ Clock Generation and Reset Management IP
> block and the OMAP2+ Power and Reset Management IP block.  Once
> drivers are available for those IP blocks, the watchdog driver can be
> converted to call exported drivers from those functions directly.
> At that point, the platform_data function pointer can be removed.
> 
> In the short term, this patch is needed to allow the PRM code to be
> removed from arch/arm/mach-omap2 (it is being moved to a driver).
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>

Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

Kind regards,
Wim.

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Aaro Koskinen]

Hi,

On Mon, Oct 15, 2012 at 07:32:33PM -0600, Paul Walmsley wrote:
> The OMAP watchdog timer driver directly calls a function exported by
> code in arch/arm/mach-omap2.  This is not good; it tightly couples
> this driver to the mach-omap2 integration code.  Instead, add a
> temporary platform_data function pointer to abstract this function
> call.  A subsequent patch will convert the watchdog driver to use this
> function pointer.

Why a function is needed? Reset cause won't change until the next reset,
so it should be enough to read it once during the init and store it into
the platform data.

A.

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Paul Walmsley]

Terve,

On Thu, 25 Oct 2012, Aaro Koskinen wrote:

> On Mon, Oct 15, 2012 at 07:32:33PM -0600, Paul Walmsley wrote:
> > The OMAP watchdog timer driver directly calls a function exported by
> > code in arch/arm/mach-omap2.  This is not good; it tightly couples
> > this driver to the mach-omap2 integration code.  Instead, add a
> > temporary platform_data function pointer to abstract this function
> > call.  A subsequent patch will convert the watchdog driver to use this
> > function pointer.
> 
> Why a function is needed? Reset cause won't change until the next reset,
> so it should be enough to read it once during the init and store it into
> the platform data.

Once the PRM/CM drivers and DT conversion is complete, this driver won't 
have any platform_data.  There won't be any watchdog driver initialization 
code in arch/arm/mach-omap2.  At that point in time, the driver will just 
call a function from the PRM driver that provides the reset source data.  

As you say, the watchdog can certainly do this from its probe code; it 
would be more efficient.  Just didn't want to make that change now; seems 
like it would be best done as a separate optimization patch.


- Paul

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [121025 11:53]:
> Terve,
> 
> On Thu, 25 Oct 2012, Aaro Koskinen wrote:
> 
> > On Mon, Oct 15, 2012 at 07:32:33PM -0600, Paul Walmsley wrote:
> > > The OMAP watchdog timer driver directly calls a function exported by
> > > code in arch/arm/mach-omap2.  This is not good; it tightly couples
> > > this driver to the mach-omap2 integration code.  Instead, add a
> > > temporary platform_data function pointer to abstract this function
> > > call.  A subsequent patch will convert the watchdog driver to use this
> > > function pointer.
> > 
> > Why a function is needed? Reset cause won't change until the next reset,
> > so it should be enough to read it once during the init and store it into
> > the platform data.
> 
> Once the PRM/CM drivers and DT conversion is complete, this driver won't 
> have any platform_data.  There won't be any watchdog driver initialization 
> code in arch/arm/mach-omap2.  At that point in time, the driver will just 
> call a function from the PRM driver that provides the reset source data.  
> 
> As you say, the watchdog can certainly do this from its probe code; it 
> would be more efficient.  Just didn't want to make that change now; seems 
> like it would be best done as a separate optimization patch.

Ideally we'd have some Linux generic function to implement in the PRM/CM
drivers for getting the bootreason. But I guess we don't?

Regards,

Tony

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Paul Walmsley]

On Thu, 25 Oct 2012, Tony Lindgren wrote:

> Ideally we'd have some Linux generic function to implement in the PRM/CM
> drivers for getting the bootreason. But I guess we don't?

Do you know of one, apart from WDIOC_GETBOOTSTATUS?


- Paul

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [121025 12:11]:
> On Thu, 25 Oct 2012, Tony Lindgren wrote:
> 
> > Ideally we'd have some Linux generic function to implement in the PRM/CM
> > drivers for getting the bootreason. But I guess we don't?
> 
> Do you know of one, apart from WDIOC_GETBOOTSTATUS?

I wonder if we can now with multiple watchdogs supported to
have also PRM/CM implement omap_prcm_wdt_ioctl() that just
handles WDIOC_GETBOOTSTATUS and then just remove that handling
from the other omap watchdog drivers?

Regards,

Tony

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Paul Walmsley]

On Thu, 25 Oct 2012, Tony Lindgren wrote:

> I wonder if we can now with multiple watchdogs supported to
> have also PRM/CM implement omap_prcm_wdt_ioctl() that just
> handles WDIOC_GETBOOTSTATUS and then just remove that handling
> from the other omap watchdog drivers?

The watchdog ioctl code needs access to internal watchdog state and 
functions, for example  WDIOC_KEEPALIVE and  WDIOC_SETTIMEOUT.  How would 
the approach you're thinking of implement those operations?



- Paul

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [121025 12:32]:
> On Thu, 25 Oct 2012, Tony Lindgren wrote:
> 
> > I wonder if we can now with multiple watchdogs supported to
> > have also PRM/CM implement omap_prcm_wdt_ioctl() that just
> > handles WDIOC_GETBOOTSTATUS and then just remove that handling
> > from the other omap watchdog drivers?
> 
> The watchdog ioctl code needs access to internal watchdog state and 
> functions, for example  WDIOC_KEEPALIVE and  WDIOC_SETTIMEOUT.  How would 
> the approach you're thinking of implement those operations?

I have not looked how the watchdog subsystem handles multiple
watchdogs, but.. Can't we have the omap watchdog and twl watchdog
return -ENOTSUPP for WDIOC_GETBOOTSTATUS and have the watchdog
core fail over to the third wathcdog omap_prcm_wdt_ioctl() that
only implements WDIOC_GETBOOTSTATUS? Or something like that.

Regards,

Tony

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [121025 12:35]:
> * Paul Walmsley <paul@pwsan.com> [121025 12:32]:
> > On Thu, 25 Oct 2012, Tony Lindgren wrote:
> > 
> > > I wonder if we can now with multiple watchdogs supported to
> > > have also PRM/CM implement omap_prcm_wdt_ioctl() that just
> > > handles WDIOC_GETBOOTSTATUS and then just remove that handling
> > > from the other omap watchdog drivers?
> > 
> > The watchdog ioctl code needs access to internal watchdog state and 
> > functions, for example  WDIOC_KEEPALIVE and  WDIOC_SETTIMEOUT.  How would 
> > the approach you're thinking of implement those operations?
> 
> I have not looked how the watchdog subsystem handles multiple
> watchdogs, but.. Can't we have the omap watchdog and twl watchdog
> return -ENOTSUPP for WDIOC_GETBOOTSTATUS and have the watchdog
> core fail over to the third wathcdog omap_prcm_wdt_ioctl() that
> only implements WDIOC_GETBOOTSTATUS? Or something like that.

After poking around a bit, probably the way to do this would
be to have watchdog core bootstatus in addition to the
bootstatus in struct watchdog_device?

Then the omap_prcm watchdog could just initialize the
watchdog core bootstatus, and the other omap watchdog drivers
would just return the watchdog core bootstatus.

Not that it's related to this patchset, just trying to figure
out how it could be done in a generic way :)

Regards,

Tony 

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Paul Walmsley]

On Thu, 25 Oct 2012, Tony Lindgren wrote:

> I have not looked how the watchdog subsystem handles multiple
> watchdogs, but..

In the new watchdog core code, each watchdog driver gets a separate 
/dev/watchdog* character device.  The ioctls are called on those device 
nodes.

[ As an aside, neither the OMAP watchdog driver, nor the TWL watchdog 
driver have been updated to use the new watchdog core code.  So they both 
can't be loaded at the same time until one or both are fixed. ]

> Can't we have the omap watchdog and twl watchdog return -ENOTSUPP for 
> WDIOC_GETBOOTSTATUS and have the watchdog core fail over to the third 
> wathcdog omap_prcm_wdt_ioctl() that only implements WDIOC_GETBOOTSTATUS? 
> Or something like that.

Sounds like a question better asked of Alan Cox and Wim, who wrote the 
watchdog core code.

Two other observations:

- It's possible that two different watchdogs could report different boot 
reasons.  The TWL might store its own watchdog boot reason which could be 
different from what's reported via the OMAP PRM.  For example, the TWL can 
record a thermal shutdown reset event, STS_BOOT.TS, which wouldn't be 
reflected in the PRM reset source data, which would just see some kind of 
external reset or power-off event.

- The PRM doesn't contain a hardware watchdog, so not sure it makes sense 
to create a watchdog driver for it.


- Paul

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Aaro Koskinen]

Hi,

On Thu, Oct 25, 2012 at 07:57:31PM +0000, Paul Walmsley wrote:
> [ As an aside, neither the OMAP watchdog driver, nor the TWL watchdog 
> driver have been updated to use the new watchdog core code.  So they both 
> can't be loaded at the same time until one or both are fixed. ]

FYI, this is being done currently:

	- OMAP wdt: https://lkml.org/lkml/2012/10/10/402
	- TWL wdt: http://marc.info/?l=linux-omap&m=134734329319385&w=2

A.

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Paul Walmsley]

On Thu, 25 Oct 2012, Aaro Koskinen wrote:

> On Thu, Oct 25, 2012 at 07:57:31PM +0000, Paul Walmsley wrote:
> > [ As an aside, neither the OMAP watchdog driver, nor the TWL watchdog 
> > driver have been updated to use the new watchdog core code.  So they both 
> > can't be loaded at the same time until one or both are fixed. ]
> 
> FYI, this is being done currently:
> 
> 	- OMAP wdt: https://lkml.org/lkml/2012/10/10/402
> 	- TWL wdt: http://marc.info/?l=linux-omap&m=134734329319385&w=2

Thanks, that's good news.


- Paul

Re: [PATCH 6/7] watchdog: OMAP: use standard GETBOOTSTATUS interface; use platform_data fn ptr

[Jon Hunter]

Hi Paul,

On 10/15/2012 08:32 PM, Paul Walmsley wrote:
> Previously the OMAP watchdog driver used a non-standard way to report
> the chip reset source via the GETBOOTSTATUS ioctl.  This patch
> converts the driver to use the standard WDIOF_* flags for this
> purpose.
> 
> This patch may break existing userspace code that uses the existing
> non-standard data format returned by the OMAP watchdog driver's
> GETBOOTSTATUS ioctl.  To fetch detailed reset source information,
> userspace code will need to retrieve it directly from the CGRM or PRM
> drivers when those are completed.
> 
> Previously, to fetch the reset source, the driver either read a
> register outside the watchdog IP block (OMAP1), or called a function
> exported directly from arch/arm/mach-omap2.  Both approaches are
> broken.  This patch also converts the driver to use a platform_data
> function pointer.  This approach is temporary, and is due to the lack
> of drivers for the OMAP16xx+ Clock Generation and Reset Management IP
> block and the OMAP2+ Power and Reset Management IP block.  Once
> drivers are available for those IP blocks, the watchdog driver can be
> converted to call exported drivers from those functions directly.
> At that point, the platform_data function pointer can be removed.
> 
> In the short term, this patch is needed to allow the PRM code to be
> removed from arch/arm/mach-omap2 (it is being moved to a driver).
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> ---
>  drivers/watchdog/omap_wdt.c |   26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index f5db18db..5d33bc0 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -46,8 +46,8 @@
>  #include <linux/slab.h>
>  #include <linux/pm_runtime.h>
>  #include <mach/hardware.h>
> -#include <plat/cpu.h>
> -#include <plat/prcm.h>
> +
> +#include <linux/platform_data/omap-wd-timer.h>
>  
>  #include "omap_wdt.h"
>  
> @@ -202,8 +202,10 @@ static ssize_t omap_wdt_write(struct file *file, const char __user *data,
>  static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
>  						unsigned long arg)
>  {
> +	struct omap_wd_timer_platform_data *pdata;
>  	struct omap_wdt_dev *wdev;
> -	int new_margin;
> +	u32 rs;
> +	int new_margin, bs;
>  	static const struct watchdog_info ident = {
>  		.identity = "OMAP Watchdog",
>  		.options = WDIOF_SETTIMEOUT,
> @@ -211,6 +213,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
>  	};
>  
>  	wdev = file->private_data;
> +	pdata = wdev->dev->platform_data;
>  
>  	switch (cmd) {
>  	case WDIOC_GETSUPPORT:
> @@ -219,17 +222,12 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
>  	case WDIOC_GETSTATUS:
>  		return put_user(0, (int __user *)arg);
>  	case WDIOC_GETBOOTSTATUS:
> -#ifdef CONFIG_ARCH_OMAP1
> -		if (cpu_is_omap16xx())
> -			return put_user(__raw_readw(ARM_SYSST),
> -					(int __user *)arg);
> -#endif
> -#ifdef CONFIG_ARCH_OMAP2PLUS
> -		if (cpu_is_omap24xx())
> -			return put_user(omap_prcm_get_reset_sources(),
> -					(int __user *)arg);
> -#endif
> -		return put_user(0, (int __user *)arg);
> +		if (!pdata->read_reset_sources)
> +			return put_user(0, (int __user *)arg);

In the case of booting with device-tree, pdata could be null and so
should we check for this too? In other words ...

+		if (!pdata || !pdata->read_reset_sources)
+			return put_user(0, (int __user *)arg);

Cheers
Jon

Re: [PATCH 6/7] watchdog: OMAP: use standard GETBOOTSTATUS interface; use platform_data fn ptr

[Paul Walmsley]

On Thu, 25 Oct 2012, Jon Hunter wrote:

> > +		if (!pdata->read_reset_sources)
> > +			return put_user(0, (int __user *)arg);
> 
> In the case of booting with device-tree, pdata could be null and so
> should we check for this too? In other words ...
> 
> +		if (!pdata || !pdata->read_reset_sources)
> +			return put_user(0, (int __user *)arg);

Thanks, good catch, will integrate that fix.


- Paul

Re: [PATCH 6/7] watchdog: OMAP: use standard GETBOOTSTATUS interface; use platform_data fn ptr

[Paul Walmsley]

On Thu, 25 Oct 2012, Paul Walmsley wrote:

> On Thu, 25 Oct 2012, Jon Hunter wrote:
> 
> > In the case of booting with device-tree, pdata could be null and so
> > should we check for this too? In other words ...
> > 
> > +		if (!pdata || !pdata->read_reset_sources)
> > +			return put_user(0, (int __user *)arg);
> 
> Thanks, good catch, will integrate that fix.

Here's the updated patch.

- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Sun, 7 Oct 2012 20:13:26 -0600
Subject: [PATCH] watchdog: OMAP: use standard GETBOOTSTATUS interface; use
 platform_data fn ptr

Previously the OMAP watchdog driver used a non-standard way to report
the chip reset source via the GETBOOTSTATUS ioctl.  This patch
converts the driver to use the standard WDIOF_* flags for this
purpose.

This patch may break existing userspace code that uses the existing
non-standard data format returned by the OMAP watchdog driver's
GETBOOTSTATUS ioctl.  To fetch detailed reset source information,
userspace code will need to retrieve it directly from the CGRM or PRM
drivers when those are completed.

Previously, to fetch the reset source, the driver either read a
register outside the watchdog IP block (OMAP1), or called a function
exported directly from arch/arm/mach-omap2.  Both approaches are
broken.  This patch also converts the driver to use a platform_data
function pointer.  This approach is temporary, and is due to the lack
of drivers for the OMAP16xx+ Clock Generation and Reset Management IP
block and the OMAP2+ Power and Reset Management IP block.  Once
drivers are available for those IP blocks, the watchdog driver can be
converted to call exported drivers from those functions directly.
At that point, the platform_data function pointer can be removed.

In the short term, this patch is needed to allow the PRM code to be
removed from arch/arm/mach-omap2 (it is being moved to a driver).

This version integrates a fix from Jon Hunter <jon-hunter@ti.com>
that avoids a NULL pointer dereference in a DT-only boot.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Jon Hunter <jon-hunter@ti.com>
---
 drivers/watchdog/omap_wdt.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index f5db18db..477a1d4 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -46,8 +46,8 @@
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
 #include <mach/hardware.h>
-#include <plat/cpu.h>
-#include <plat/prcm.h>
+
+#include <linux/platform_data/omap-wd-timer.h>
 
 #include "omap_wdt.h"
 
@@ -202,8 +202,10 @@ static ssize_t omap_wdt_write(struct file *file, const char __user *data,
 static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 						unsigned long arg)
 {
+	struct omap_wd_timer_platform_data *pdata;
 	struct omap_wdt_dev *wdev;
-	int new_margin;
+	u32 rs;
+	int new_margin, bs;
 	static const struct watchdog_info ident = {
 		.identity = "OMAP Watchdog",
 		.options = WDIOF_SETTIMEOUT,
@@ -211,6 +213,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 	};
 
 	wdev = file->private_data;
+	pdata = wdev->dev->platform_data;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
@@ -219,17 +222,12 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 	case WDIOC_GETSTATUS:
 		return put_user(0, (int __user *)arg);
 	case WDIOC_GETBOOTSTATUS:
-#ifdef CONFIG_ARCH_OMAP1
-		if (cpu_is_omap16xx())
-			return put_user(__raw_readw(ARM_SYSST),
-					(int __user *)arg);
-#endif
-#ifdef CONFIG_ARCH_OMAP2PLUS
-		if (cpu_is_omap24xx())
-			return put_user(omap_prcm_get_reset_sources(),
-					(int __user *)arg);
-#endif
-		return put_user(0, (int __user *)arg);
+		if (!pdata || !pdata->read_reset_sources)
+			return put_user(0, (int __user *)arg);
+		rs = pdata->read_reset_sources();
+		bs = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
+			WDIOF_CARDRESET : 0;
+		return put_user(bs, (int __user *)arg);
 	case WDIOC_KEEPALIVE:
 		spin_lock(&wdt_lock);
 		omap_wdt_ping(wdev);
-- 
1.7.10.4

Re: [PATCH 6/7] watchdog: OMAP: use standard GETBOOTSTATUS interface; use platform_data fn ptr

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On Thu, Oct 25, 2012 at 08:29:26PM +0000, Paul Walmsley wrote:
> On Thu, 25 Oct 2012, Paul Walmsley wrote:
> 
> > On Thu, 25 Oct 2012, Jon Hunter wrote:
> > 
> > > In the case of booting with device-tree, pdata could be null and so
> > > should we check for this too? In other words ...
> > > 
> > > +		if (!pdata || !pdata->read_reset_sources)
> > > +			return put_user(0, (int __user *)arg);
> > 
> > Thanks, good catch, will integrate that fix.
> 
> Here's the updated patch.
> 
> - Paul
> 
> From: Paul Walmsley <paul@pwsan.com>
> Date: Sun, 7 Oct 2012 20:13:26 -0600
> Subject: [PATCH] watchdog: OMAP: use standard GETBOOTSTATUS interface; use
>  platform_data fn ptr
> 
> Previously the OMAP watchdog driver used a non-standard way to report
> the chip reset source via the GETBOOTSTATUS ioctl.  This patch
> converts the driver to use the standard WDIOF_* flags for this
> purpose.
> 
> This patch may break existing userspace code that uses the existing
> non-standard data format returned by the OMAP watchdog driver's
> GETBOOTSTATUS ioctl.  To fetch detailed reset source information,
> userspace code will need to retrieve it directly from the CGRM or PRM
> drivers when those are completed.
> 
> Previously, to fetch the reset source, the driver either read a
> register outside the watchdog IP block (OMAP1), or called a function
> exported directly from arch/arm/mach-omap2.  Both approaches are
> broken.  This patch also converts the driver to use a platform_data
> function pointer.  This approach is temporary, and is due to the lack
> of drivers for the OMAP16xx+ Clock Generation and Reset Management IP
> block and the OMAP2+ Power and Reset Management IP block.  Once
> drivers are available for those IP blocks, the watchdog driver can be
> converted to call exported drivers from those functions directly.

should be "exported functions from those drivers directly."

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 6/7] watchdog: OMAP: use standard GETBOOTSTATUS interface; use platform_data fn ptr

[Paul Walmsley]

On Thu, 25 Oct 2012, Felipe Balbi wrote:

> should be "exported functions from those drivers directly."

Thanks, will update the patch description.


- Paul

Re: [PATCH 5/7] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata function pointer

[Paul Walmsley]

On Mon, 15 Oct 2012, Paul Walmsley wrote:

> The OMAP watchdog timer driver directly calls a function exported by
> code in arch/arm/mach-omap2.  This is not good; it tightly couples
> this driver to the mach-omap2 integration code.  Instead, add a
> temporary platform_data function pointer to abstract this function
> call.  A subsequent patch will convert the watchdog driver to use this
> function pointer.
> 
> This patch also moves the device creation code out of
> arch/arm/mach-omap2/devices.c and into arch/arm/mach-omap2/wd_timer.c.
> This is another step towards the removal of
> arch/arm/mach-omap2/devices.c.
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>

This patch missed the change from commit 
6e152231995aa4ed5eafd87a6a8348563248f843 ("ARM: OMAP: avoid build wdt 
platform device if with dt support") to skip the mach-omap2/ device build 
if the DT blob was present.  This resulted in the watchdog getting 
probed twice.  This didn't result in any problems, aside from a 
message in the kernel log.  

Anyway, here's an update with that change added.


- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Mon, 29 Oct 2012 20:49:44 -0600
Subject: [PATCH] ARM: OMAP2+: WDT: move init; add read_reset_sources pdata
 function pointer

The OMAP watchdog timer driver directly calls a function exported by
code in arch/arm/mach-omap2.  This is not good; it tightly couples
this driver to the mach-omap2 integration code.  Instead, add a
temporary platform_data function pointer to abstract this function
call.  A subsequent patch will convert the watchdog driver to use this
function pointer.

This patch also moves the device creation code out of
arch/arm/mach-omap2/devices.c and into arch/arm/mach-omap2/wd_timer.c.
This is another step towards the removal of
arch/arm/mach-omap2/devices.c.

Cc: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Wim Van Sebroeck <wim@iguana.be>
[paul@pwsan.com: skip wd_timer device creation when DT blob is present]
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap1/devices.c               |   21 ++++++++++++---
 arch/arm/mach-omap2/devices.c               |   26 ------------------
 arch/arm/mach-omap2/wd_timer.c              |   35 +++++++++++++++++++++++-
 include/linux/platform_data/omap-wd-timer.h |   38 +++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 30 deletions(-)
 create mode 100644 include/linux/platform_data/omap-wd-timer.h

diff --git a/arch/arm/mach-omap1/devices.c b/arch/arm/mach-omap1/devices.c
index 645668e..7450318 100644
--- a/arch/arm/mach-omap1/devices.c
+++ b/arch/arm/mach-omap1/devices.c
@@ -17,6 +17,8 @@
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
 
+#include <linux/platform_data/omap-wd-timer.h>
+
 #include <asm/mach/map.h>
 
 #include <mach/tc.h>
@@ -448,18 +450,31 @@ static struct resource wdt_resources[] = {
 };
 
 static struct platform_device omap_wdt_device = {
-	.name	   = "omap_wdt",
-	.id	     = -1,
+	.name		= "omap_wdt",
+	.id		= -1,
 	.num_resources	= ARRAY_SIZE(wdt_resources),
 	.resource	= wdt_resources,
 };
 
 static int __init omap_init_wdt(void)
 {
+	struct omap_wd_timer_platform_data pdata;
+	int ret;
+
 	if (!cpu_is_omap16xx())
 		return -ENODEV;
 
-	return platform_device_register(&omap_wdt_device);
+	pdata.read_reset_sources = omap1_get_reset_sources;
+
+	ret = platform_device_register(&omap_wdt_device);
+	if (!ret) {
+		ret = platform_device_add_data(&omap_wdt_device, &pdata,
+					       sizeof(pdata));
+		if (ret)
+			platform_device_del(&omap_wdt_device);
+	}
+
+	return ret;
 }
 subsys_initcall(omap_init_wdt);
 #endif
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 2ad491d..cf365c3 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -646,29 +646,3 @@ static int __init omap2_init_devices(void)
 	return 0;
 }
 arch_initcall(omap2_init_devices);
-
-#if defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
-static int __init omap_init_wdt(void)
-{
-	int id = -1;
-	struct platform_device *pdev;
-	struct omap_hwmod *oh;
-	char *oh_name = "wd_timer2";
-	char *dev_name = "omap_wdt";
-
-	if (!cpu_class_is_omap2() || of_have_populated_dt())
-		return 0;
-
-	oh = omap_hwmod_lookup(oh_name);
-	if (!oh) {
-		pr_err("Could not look up wd_timer%d hwmod\n", id);
-		return -EINVAL;
-	}
-
-	pdev = omap_device_build(dev_name, id, oh, NULL, 0, NULL, 0, 0);
-	WARN(IS_ERR(pdev), "Can't build omap_device for %s:%s.\n",
-				dev_name, oh->name);
-	return 0;
-}
-subsys_initcall(omap_init_wdt);
-#endif
diff --git a/arch/arm/mach-omap2/wd_timer.c b/arch/arm/mach-omap2/wd_timer.c
index f6b6c37..5a8629f 100644
--- a/arch/arm/mach-omap2/wd_timer.c
+++ b/arch/arm/mach-omap2/wd_timer.c
@@ -11,10 +11,14 @@
 #include <linux/io.h>
 #include <linux/err.h>
 
-#include "omap_hwmod.h"
+#include <linux/platform_data/omap-wd-timer.h>
 
+#include "omap_hwmod.h"
+#include "omap_device.h"
 #include "wd_timer.h"
 #include "common.h"
+#include "prm.h"
+#include "soc.h"
 
 /*
  * In order to avoid any assumptions from bootloader regarding WDT
@@ -99,3 +103,32 @@ int omap2_wd_timer_reset(struct omap_hwmod *oh)
 	return (c == MAX_MODULE_SOFTRESET_WAIT) ? -ETIMEDOUT :
 		omap2_wd_timer_disable(oh);
 }
+
+static int __init omap_init_wdt(void)
+{
+	int id = -1;
+	struct platform_device *pdev;
+	struct omap_hwmod *oh;
+	char *oh_name = "wd_timer2";
+	char *dev_name = "omap_wdt";
+	struct omap_wd_timer_platform_data pdata;
+
+	if (!cpu_class_is_omap2() || of_have_populated_dt())
+		return 0;
+
+	oh = omap_hwmod_lookup(oh_name);
+	if (!oh) {
+		pr_err("Could not look up wd_timer%d hwmod\n", id);
+		return -EINVAL;
+	}
+
+	pdata.read_reset_sources = prm_read_reset_sources;
+
+	pdev = omap_device_build(dev_name, id, oh, &pdata,
+				 sizeof(struct omap_wd_timer_platform_data),
+				 NULL, 0, 0);
+	WARN(IS_ERR(pdev), "Can't build omap_device for %s:%s.\n",
+	     dev_name, oh->name);
+	return 0;
+}
+subsys_initcall(omap_init_wdt);
diff --git a/include/linux/platform_data/omap-wd-timer.h b/include/linux/platform_data/omap-wd-timer.h
new file mode 100644
index 0000000..d75f5f8
--- /dev/null
+++ b/include/linux/platform_data/omap-wd-timer.h
@@ -0,0 +1,38 @@
+/*
+ * OMAP2+ WDTIMER-specific function prototypes
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ * Paul Walmsley
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_PLATFORM_DATA_OMAP_WD_TIMER_H
+#define __LINUX_PLATFORM_DATA_OMAP_WD_TIMER_H
+
+#include <linux/types.h>
+
+/*
+ * Standardized OMAP reset source bits
+ *
+ * This is a subset of the ones listed in arch/arm/mach-omap2/prm.h
+ * and are the only ones needed in the watchdog driver.
+ */
+#define OMAP_MPU_WD_RST_SRC_ID_SHIFT				3
+
+/**
+ * struct omap_wd_timer_platform_data - WDTIMER integration to the host SoC
+ * @read_reset_sources - fn ptr for the SoC to indicate the last reset cause
+ *
+ * The function pointed to by @read_reset_sources must return its data
+ * in a standard format - search for RST_SRC_ID_SHIFT in
+ * arch/arm/mach-omap2
+ */
+struct omap_wd_timer_platform_data {
+	u32 (*read_reset_sources)(void);
+};
+
+#endif
-- 
1.7.10.4