login
Search
Hi all, Following are patches syncing, cleaning up and fixing omap watchdog driver. Patches 1-3 could be applied as is. Patches 4 and 5, on the other hand, needs some discussion since I'd be creating a new directory (include/linux/watchdog) and changing the clock handling. Russel King told me, he'd like to hold on patch 5/5 as he'd have a better solution for the clock handling soon. I'm sending that patch away anyway so we can start the discussion on the mainling list about that. Felipe Balbi (5): watchdog: sync linux-omap changes watchdog: another ioremap() fix watchdog: cleanup a bit omap_wdt.c watchdog: move omap_wdt.h to include/linux/watchdog watchdog: introduce platform_data and remove cpu conditional code arch/arm/plat-omap/devices.c | 76 ++++++++- drivers/watchdog/omap_wdt.c | 333 ++++++++++++++++++++++--------------- drivers/watchdog/omap_wdt.h | 64 ------- include/linux/watchdog/omap_wdt.h | 83 +++++++++ 4 files changed, 352 insertions(+), 204 deletions(-) delete mode 100644 drivers/watchdog/omap_wdt.h create mode 100644 include/linux/watchdog/omap_wdt.h --
These are changes that have been sitting in linux-omap
and were never sent upstream.
Hopefully, it'll never happen again at least for this
driver.
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
arch/arm/plat-omap/devices.c | 21 ++--
drivers/watchdog/omap_wdt.c | 287 +++++++++++++++++++++++++++---------------
drivers/watchdog/omap_wdt.h | 28 ++---
3 files changed, 205 insertions(+), 131 deletions(-)
diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index bc1cf30..f1eaa44 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -454,16 +454,8 @@ static inline void omap_init_uwire(void) {}
#if defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
-#ifdef CONFIG_ARCH_OMAP24XX
-#define OMAP_WDT_BASE 0x48022000
-#else
-#define OMAP_WDT_BASE 0xfffeb000
-#endif
-
static struct resource wdt_resources[] = {
{
- .start = OMAP_WDT_BASE,
- .end = OMAP_WDT_BASE + 0x4f,
.flags = IORESOURCE_MEM,
},
};
@@ -477,6 +469,19 @@ static struct platform_device omap_wdt_device = {
static void omap_init_wdt(void)
{
+ if (cpu_is_omap16xx())
+ wdt_resources[0].start = 0xfffeb000;
+ else if (cpu_is_omap2420())
+ wdt_resources[0].start = 0x48022000; /* WDT2 */
+ else if (cpu_is_omap2430())
+ wdt_resources[0].start = 0x49016000; /* WDT2 */
+ else if (cpu_is_omap343x())
+ wdt_resources[0].start = 0x48314000; /* WDT2 */
+ else
+ return;
+
+ wdt_resources[0].end = wdt_resources[0].start + 0x4f;
+
(void) platform_device_register(&omap_wdt_device);
}
#else
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 3a11dad..e55f2cc 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -1,7 +1,7 @@
/*
- * linux/drivers/char/watchdog/omap_wdt.c
+ * linux/drivers/watchdog/omap_wdt.c
*
- * Watchdog driver for the TI OMAP 16xx & 24xx 32KHz (non-secure) watchdog
+ * Watchdog driver for the TI OMAP 16xx & 24xx/34xx 32KHz ...convert to use ioremap() and __raw_{read/write} friends.
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
Signed-off-by: George G. Davis <gdavis@mvista.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
drivers/watchdog/omap_wdt.c | 50 +++++++++++++++++++++++++------------------
1 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index e55f2cc..4efb87b 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -72,12 +72,12 @@ static void omap_wdt_ping(struct omap_wdt_dev *wdev)
{
void __iomem *base = wdev->base;
/* wait for posted write to complete */
- while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
+ while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
cpu_relax();
wdt_trgr_pattern = ~wdt_trgr_pattern;
- omap_writel(wdt_trgr_pattern, (base + OMAP_WATCHDOG_TGR));
+ __raw_writel(wdt_trgr_pattern, (base + OMAP_WATCHDOG_TGR));
/* wait for posted write to complete */
- while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
+ while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
cpu_relax();
/* reloaded WCRR from WLDR */
}
@@ -87,11 +87,11 @@ static void omap_wdt_enable(struct omap_wdt_dev *wdev)
void __iomem *base;
base = wdev->base;
/* Sequence to enable the watchdog */
- omap_writel(0xBBBB, base + OMAP_WATCHDOG_SPR);
- while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
+ __raw_writel(0xBBBB, base + OMAP_WATCHDOG_SPR);
+ while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
cpu_relax();
- omap_writel(0x4444, base + OMAP_WATCHDOG_SPR);
- while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
+ __raw_writel(0x4444, base + OMAP_WATCHDOG_SPR);
+ while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
cpu_relax();
}
@@ -100,11 +100,11 @@ static void omap_wdt_disable(struct omap_wdt_dev *wdev)
void __iomem *base;
base = wdev->base;
/* sequence required to disable watchdog */
- omap_writel(0xAAAA, base + ...Trivial cleanup patch.
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
drivers/watchdog/omap_wdt.c | 133 +++++++++++++++++++++++++-----------------
1 files changed, 79 insertions(+), 54 deletions(-)
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 4efb87b..b670ad5 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -71,11 +71,14 @@ struct omap_wdt_dev {
static void omap_wdt_ping(struct omap_wdt_dev *wdev)
{
void __iomem *base = wdev->base;
+
/* wait for posted write to complete */
while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
cpu_relax();
+
wdt_trgr_pattern = ~wdt_trgr_pattern;
__raw_writel(wdt_trgr_pattern, (base + OMAP_WATCHDOG_TGR));
+
/* wait for posted write to complete */
while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
cpu_relax();
@@ -84,12 +87,13 @@ static void omap_wdt_ping(struct omap_wdt_dev *wdev)
static void omap_wdt_enable(struct omap_wdt_dev *wdev)
{
- void __iomem *base;
- base = wdev->base;
+ void __iomem *base = wdev->base;
+
/* Sequence to enable the watchdog */
__raw_writel(0xBBBB, base + OMAP_WATCHDOG_SPR);
while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
cpu_relax();
+
__raw_writel(0x4444, base + OMAP_WATCHDOG_SPR);
while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
cpu_relax();
@@ -97,12 +101,13 @@ static void omap_wdt_enable(struct omap_wdt_dev *wdev)
static void omap_wdt_disable(struct omap_wdt_dev *wdev)
{
- void __iomem *base;
- base = wdev->base;
+ void __iomem *base = wdev->base;
+
/* sequence required to disable watchdog */
__raw_writel(0xAAAA, base + OMAP_WATCHDOG_SPR); /* TIMER_MODE */
while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
cpu_relax();
+
__raw_writel(0x5555, base + OMAP_WATCHDOG_SPR); /* TIMER_MODE */
while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
cpu_relax();
@@ -120,12 +125,12 @@ static void omap_wdt_adjust_timeout(unsigned new_timeout)
...Create a new include/linux/watchdog directory for holding watchdog chips headers, move omap_wdt.h to the new location and update the include path in the driver source. Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com> --- drivers/watchdog/omap_wdt.c | 3 +- drivers/watchdog/omap_wdt.h | 54 ------------------------------------- include/linux/watchdog/omap_wdt.h | 54 +++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 56 deletions(-) delete mode 100644 drivers/watchdog/omap_wdt.h create mode 100644 include/linux/watchdog/omap_wdt.h diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c index b670ad5..8b68bc0 100644 --- a/drivers/watchdog/omap_wdt.c +++ b/drivers/watchdog/omap_wdt.c @@ -33,6 +33,7 @@ #include <linux/mm.h> #include <linux/miscdevice.h> #include <linux/watchdog.h> +#include <linux/watchdog/omap_wdt.h> #include <linux/reboot.h> #include <linux/init.h> #include <linux/err.h> @@ -46,8 +47,6 @@ #include <mach/hardware.h> #include <mach/prcm.h> -#include "omap_wdt.h" - static struct platform_device *omap_wdt_dev; static unsigned timer_margin; diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h deleted file mode 100644 index fc02ec6..0000000 --- a/drivers/watchdog/omap_wdt.h +++ /dev/null @@ -1,54 +0,0 @@ -/* - * linux/drivers/char/watchdog/omap_wdt.h - * - * BRIEF MODULE DESCRIPTION - * OMAP Watchdog timer register definitions - * - * Copyright (C) 2004 Texas Instruments. - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License as published by the - * Free Software Foundation; either version 2 of the License, or (at your - * option) any later version. - * - * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED - * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF - * MERCHANTABILITY AND FITNESS FOR A PARTICULAR ...
Get rid of cpu conditional code.
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
arch/arm/plat-omap/devices.c | 65 +++++++++++++++++--
drivers/watchdog/omap_wdt.c | 126 ++++++++++++------------------------
include/linux/watchdog/omap_wdt.h | 29 +++++++++
3 files changed, 131 insertions(+), 89 deletions(-)
diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index f1eaa44..d756743 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -13,6 +13,7 @@
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/platform_device.h>
+#include <linux/watchdog/omap_wdt.h>
#include <mach/hardware.h>
#include <asm/io.h>
@@ -460,6 +461,51 @@ static struct resource wdt_resources[] = {
},
};
+static int omap_wdt_set_clock(struct omap_wdt_dev *wdev, int status)
+{
+ if (status) {
+ if (cpu_is_omap16xx())
+ clk_disable(wdev->fck);
+
+ if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+ clk_disable(wdev->ick);
+ clk_disable(wdev->fck);
+ }
+ } else {
+ if (cpu_is_omap16xx())
+ clk_disable(wdev->fck);
+
+ if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+ clk_disable(wdev->ick);
+ clk_disable(wdev->fck);
+ }
+ }
+
+ return 0;
+}
+
+static int omap_wdt_get_bootstatus(void)
+{
+ if (cpu_is_omap16xx())
+ return __raw_readw(ARM_SYSST);
+
+#if 0
+ /* REVISIT: When the patch introducing the following function
+ * gets merged upstream, uncomment this code to enable omap2
+ * support on omap watchdog driver.
+ */
+ if (cpu_is_omap24xx())
+ return omap_prcm_get_reset_sources();
+#endif
+
+ return 0;
+}
+
+static struct omap_wdt_platform_data omap_wdt_pdata = {
+ .set_clock = omap_wdt_set_clock,
+ .get_bootstatus = omap_wdt_get_bootstatus,
+};
+
static struct platform_device omap_wdt_device = {
.name = "omap_wdt",
.id = -1,
@@ -469,17 +515,26 @@ static struct platform_device omap_wdt_device = {
static void omap_init_wdt(void)
{
- if ...What happened to leaving this stuff inside omap_wdt.c as I said during the previous review? I really don't want to see such cleanups when the real answer is to fix the OMAP clock API implementation. It just makes for more unnecessary noise when doing this, and then yet more noise when we fix the OMAP clock API. Please get rid of this and leave the clock naming crap inside omap_wdt.c. --
Well, patches 4 and 5 should be ignored. Should I resend or could I rely on the fact that people won't pick them up ? -- balbi --
Given my comments on patch 1, it's probably a good idea to resend just 1 to 3. We can then talk about 4 and 5 some more. Wim - can you let me know when you merge the followup patches into your tree please? --
Will add patches 1 to 3 when everyone is OK with them. I still saw some comments from David. Kind Regards, Wim. --
I will not pick them up (to add them in the watchdog tree). Kind regards, Wim. --
On Fri, 19 Sep 2008 13:32:38 +0300 Headers that are driver private belong with the driver. In fact in many cases they belong *in* the driver C file but that depends how much is You could just drop these into the C file given how small they are. Alan --
Sure, I'll update the patch. But if you look at patch 5/5, I'll need the structures to define the set_clock() function. Should I create then under <mach/omap_wdt.h> ?? -- balbi --
I think that would be better - you are not creating a public general interface to watchdogs but a specific interface between the OMAP platform code and the OMAP watchdog. Alan --
Oh, I see where "omap_wdt_dev" (global) gets used. The normal way to do stuff like that is using void* pointers placed in the inode and file structures for exactly that purpose. --
You don't have an inode or a file structure until open() is called - at which point it _is_ placed in file->private_data. So this driver is doing the right thing. --
Well, the conventional thing for misc drivers, at any rate. In various other drivers, inode->i_private is set up earlier, just to avoid such a need for globals (or equivalent). One could argue that this idiom is ugly ... and fix it by having misc_open() in drivers/char/misc.c initialize i_private before delegating to the miscdevice->fops->open(). Even just setting it to the miscdevice pointer would suffice with this driver; container_of(i_private, struct omap_wdt_dev, omap_wdt_miscdev) would then return what get_drvdata() returns, sans global. But that wouldn't be just cleaning up this watchdog. = Dave --
None that I know about - generally other drivers look up their private data in some kind of array and assign to file->private_data in their open() method - in much the same way that watchdog and misc drivers do. See __ptmx_open, __tty_open, pp_open, apm_open, hpet_open, mbcs_open, raw_open, etc. If you look at data structure lifetimes, the lifetime of 'file' is for the duration that any one particular instance of the file is open, and when closed, it's destroyed. It is not shared between separate opens of the same node. The 'inode' is shared between separate opens, and can be discarded when the node is not open by anyone - in other words, it's not persistent. So the only time that an inode structure is guaranteed to be present is just before the node is opened - useless for passing private pointers from the registration function through to the open() function. So, you need to store the private data pointer somewhere no matter what. The simplest solution is as the watchdog drivers are doing. You can only have one watchdog driver anyway, so there's no problem having a single static global device pointer to allow you to carry your private data across to the open() function. And anyway, the point of these patches is not to fix issues like this. It's to get what's in mainline updated to what's in the OMAP tree so stuff can move forwards. So, let's not go down rabbit warrens trying to find obscure new issues which lots of other code already "suffers" from. We're into the third day on this one driver. If every OMAP driver takes this long, we're still going to be struggling with this beyond Christmas, probably no further forward since other stuff will have regressed, and we'll have to start over again. Yes, make sure what we're submitting is correct. But don't introduce any _new_ unnecessary changes while we're trying to merge stuff upstream. Do that only once it's upstream and send those changes upstream. --
Yet i_private gets set up *somewhere* else that field wouldn't exist.
Look around; you'll see it gets used. (My quick grep started in the
USB tree, where it turns out both usbfs and gadgetfs use it.)
When the inode stays permanently in memory that's simple to manage.
Regardless, for misc drivers the scheme I mentioned is equally simple:
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -147,6 +147,7 @@ static int misc_open(struct inode * inode, struct file * file)
err = 0;
old_fops = file->f_op;
file->f_op = new_fops;
+ inode->i_private = c;
if (file->f_op->open) {
err=file->f_op->open(inode,file);
I'll disagree. It requires extra state even in this simple case;
state of a generically undesirable flavor (global). And likewise,
it requires lookup mechanisms ... which if you look around, tend
to be rather error prone, locking often gets goofed up there.
Familiar and simple != simplest, != best. It will often become
Right, my original comment pointed out one thing that was clearly
wrong (extra/unused struct field) and one odd thing (the global).
You said "not that odd, here's why"; I said "hmm, well OK but it's
still got problem X, which isn't for fixing in this patchset".
(Leaving the extra/unused struct field still an issue. It came
from the original d99241c86f6ccd1226935f8f59d3bb17a4186b85 patch
which made the OMAP tree diverge from upstream. I suspect that
one didn't get much review, partly because at that time OMAP3 was
much less available than now. Pushing that upstream may not even
That seems like a strange way to account things. It presumes
that the only time review comments should be accepted is within
a day of the patch getting posted. Regardless of whether the
reviewer has time at that point.
If you *really* wanted to avoid wasting time you wouldn't have
replied to my previous post, which agreed that one point I raised
was a non-issue for this particular patch series.
- Dave
--
Both of which are filesystems which have more control over the lifetime Well, are you going to manufacture a patch to update all the watchdog drivers to use your new i_private method, and get that merged into Wim's tree now, so that then the omap watchdog drivers can satisfy your apparant objection (which Wim _has_ taken as an objection against The "well OK" didn't come over at all - neither I nor Wim seem to have You define accounting for things in real time as "strange" - lol. Your following sentences don't follow either. My point is that we currently have a BIG problem, and that is the OMAP fork being so far out of line with mainline, it isn't funny. It's causing lots of pain for everyone here. Folk are screaming for mainline to be buildable for OMAP. There are two approaches to achieve that: take each driver, polish it for weeks on end until it's nice and shiney, and then submit it upstream. Eventually, given enough man hours, you'll get to the point where you've pushed everything upstream, but in the mean time, new work has been queued so you need to start at the beginning again. You've got a job for life constantly polishing code. The other approach is to decide that we have what we have, and that in the interests of efficiently reducing divergence, merging the upstream changes with the downstream changes and pushing the result upstream ASAP. Once merged, further improvements and cleanups can be made by pushing them separately upstream along with any other bug fixes. Given the amount of divergence, the only approach which gives realistic progress is the second one. If you think the first approach is the way to go, then please join in with Tony and myself reviewing the _entire_ OMAP tree, polishing every patch, and pushing it upstream. And I mean _everything_. Not just the USB stuff. Encourage everyone else to do the same - because it will Wim said: "Will add patches 1 to 3 when everyone is OK with them. I still saw some comments from ...
Hey, you gotta give Dave some credit! Dave's been polishing tons of omap code in addition to the USB, at least I2C, gpio, SPI come to mind. Not to mention all the blinking leds! Regarding getting and army of people to fix code, we need to start following another standard policy: All drivers must have a MAINTAINER who is capable of fixing things, and ideally doing things the right way from the start. It's not going to be enough that few people try to fix stuff and get burnt out on it over and over again when new omaps come around every 1.5 years. <snip snip> Tony --
Which includes pushing their code into mainline ... and getting acks (as you noted elsewhere) from the subsystem maintainer to help avoid going too far astray. In some cases that implies getting some new frameworks into mainline, or updating the ones that are there. The ALSA-SOC stuff has been a win there, in terms of maintainable code ... the previous solution involved very little reuse of the funky codec and data stream code, while now that's far more practical. Similarly I2C driver model support, GPIO expander infrastructure, and SPI: instead of scattering board-specific code in drivers all over the source tree, it can now be split out fairly cleanly. That makes maintainers much happier. - Dave --
Both of them are the *kernel part* of a *user mode device driver* ... but regardless, that doesn't invalidate what I said (and showed!) If Alan Cox hadn't reminded us about pending cleanups to create more of a real watchdog framework, I might well have done so. The core update was trivial; driver updates could trickle in when convenient. Redefining my words doesn't help, unless you're intent on creating arguments instead of consensus. Review rarely happens all at once, unless very few people look at the code. Discouraging review is *extremely* strange. Most developers want loads more than they ever get. Yet you complained about me not reviewing at the same time you did (I happened to be traveling) I call it a "branch" myself; "fork" sounds confrontational. When more of the arch/arm/* core bits merge -- like the clock and power domain updates ISTR you wanted to hold back -- then the rest starts to make sense upstream. Things like board support, and the Those are two ends of a *spectrum* reflecting how much work to put in, not two different "approaches". Between those ends are MANY other options. Those two ends are rarely used. Those options include what I've always thought is the norm: review drivers as part of upstream submission, and resolve most issues before they get merged. (It's an acknowledged issue that resolving them later tends to be quite difficult...) There's also the issue of strategy. One can randomly take changes and merge them ... possibly leaving upstream a bit chaotic. One could try to merge everything at once ... unrealistic. One could merge enough to meet specific functional targets ... which makes more sense to me. (Example: "2.6.28 should boot beagleboard.org Yes, there are two unresolved issues in patch #1 which you seem to have successfully buried with your flamage. Easy fixes, just strike a line and truncate a path. The sort of thing that often gets queued in the MM tree as a "fixup" and then merged into a main ...
I'm not discouraging review. I'm saying that making inappropriate comments isn't helpful. Yes, your comments are right, but are they appropriate to getting the OMAP watchdog drivers updated in mainline, or are they more appropriate in a general sense to all watchdog drivers, and therefore Yet again you use confrontational language, inflaming this discussion. Okay, I give up. Folk here can carry on struggling to get their code into mainline with endless reviews and getting fed up with having to constantly rework the code over and over again. Clearly my views aren't welcome. --
Hey, please don't give up. You two easily get caught into infinite Not true, we _really_ appreciate your comments and help. Same goes for Dave. Tony --
There are a couple of takes are reworking all the watchdog core code to get rid of that stuff - so its probably not worth worrying about as pretty soon I'd hope watchdogs are mostly using one of the new interfaces and taking struct watchdog for ops (and in the case of the watchdog core code I proposed don't even usually need open methods at all) Alan -- #include <stdsig.h> --
You're getting there with this patch, but still not completely up to snuff. And this return statement shouldn't be required. Apart from those three points, nice work. --
current style omits the paths (just use "omap_wdt.c") since they change periodically... --
You don't need both omap_wdt_dev (platform device) and omap_wdt_dev.dev (hmm, never used). In fact the former isn't needed either ... its role seems to be ensure only one watchdog device gets bound, which is more naturally done by not registering more than one such platform device. --
