diff mbox

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

Message ID 20121016013213.21844.20016.stgit@dusk.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Walmsley Oct. 16, 2012, 1:32 a.m. UTC
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

Comments

Aaro Koskinen Oct. 25, 2012, 3:38 p.m. UTC | #1
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.
Paul Walmsley Oct. 25, 2012, 6:51 p.m. UTC | #2
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
Tony Lindgren Oct. 25, 2012, 6:57 p.m. UTC | #3
* 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
Paul Walmsley Oct. 25, 2012, 7:09 p.m. UTC | #4
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
Tony Lindgren Oct. 25, 2012, 7:19 p.m. UTC | #5
* 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
Paul Walmsley Oct. 25, 2012, 7:31 p.m. UTC | #6
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
Tony Lindgren Oct. 25, 2012, 7:34 p.m. UTC | #7
* 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
Tony Lindgren Oct. 25, 2012, 7:42 p.m. UTC | #8
* 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
Paul Walmsley Oct. 25, 2012, 7:57 p.m. UTC | #9
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
Aaro Koskinen Oct. 25, 2012, 8:08 p.m. UTC | #10
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.
Paul Walmsley Oct. 25, 2012, 8:09 p.m. UTC | #11
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
diff mbox

Patch

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