Patchwork [3/3] rtc: pl031: fix up IRQ flags

login
register
mail settings
Submitter Linus Walleij
Date June 18, 2012, 9:29 a.m.
Message ID <1340011750-5113-1-git-send-email-linus.walleij@stericsson.com>
Download mbox | patch
Permalink /patch/165434/
State New
Headers show

Comments

Linus Walleij - June 18, 2012, 9:29 a.m.
From: Mattias Wallin <mattias.wallin@stericsson.com>

The pl031 interrupt is shared between the timer part
and the clockwatch part of the same HW block on the ux500,
so mark it IRQF_SHARED on this variant.

This patch also adds the IRQF_NO_SUSPEND flag to the rtc
irq on all variants as we don't want this pretty important
IRQ to be disabled in suspend.

Signed-off-by: Mattias Wallin <mattias.wallin@stericsson.com>
Reviewed-by: Rickard Andersson <rickard.andersson@stericsson.com>
Reviewed-by: Jonas Aberg <jonas.aberg@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/rtc/rtc-pl031.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
Andrew Morton - June 20, 2012, 12:03 a.m.
On Mon, 18 Jun 2012 11:29:10 +0200
Linus Walleij <linus.walleij@stericsson.com> wrote:

> The pl031 interrupt is shared between the timer part
> and the clockwatch part of the same HW block on the ux500,
> so mark it IRQF_SHARED on this variant.
> 
> This patch also adds the IRQF_NO_SUSPEND flag to the rtc
> irq on all variants as we don't want this pretty important
> IRQ to be disabled in suspend.

This patch appears to fix a bug, so I need to decide whether we should
merge the fix into 3.5 or earlier kernels.

To do that, I (and others!) need to be told what is the end-user
impact of the bug.  But this info is absent.  Please always include it!
Linus Walleij - June 20, 2012, 8:48 a.m.
On Wed, Jun 20, 2012 at 2:03 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 18 Jun 2012 11:29:10 +0200
> Linus Walleij <linus.walleij@stericsson.com> wrote:

> This patch appears to fix a bug, so I need to decide whether we should
> merge the fix into 3.5 or earlier kernels.

Basically none of the in-tree systems that use them have support
for system wide suspend/resume so they will not run into it.
So it's not critical.

I know of some out-of-tree systems using it but that's another
issue...

> To do that, I (and others!) need to be told what is the end-user
> impact of the bug.  But this info is absent.  Please always include it!

Sorry, basically on a system suspending you would expect it
to wake up if there is an RTC alarm. For example John Stultz
recently did some interesting work on alarm timers that will
exercise this on systems that support suspend and has an
always-on RTC.

Without this patch the alarm IRQ will be masked off and the
system will not wake up, or it will wake up (for purely electric
reasons) and then shut down when it can't find an IRQ.

It would probably make sense to take a swipe around
drivers/rtc and check them all for this shortcoming at some
point, but I don't think the issue is that big since only so many
systems really support suspend.

Yours,
Linus Walleij

Patch

diff --git a/drivers/rtc/rtc-pl031.c b/drivers/rtc/rtc-pl031.c
index e66afb8..08378e3 100644
--- a/drivers/rtc/rtc-pl031.c
+++ b/drivers/rtc/rtc-pl031.c
@@ -75,11 +75,13 @@ 
  *	clockwatch function
  * @st_weekday: if this is an ST Microelectronics silicon version that need
  *	the weekday fix
+ * @irqflags: special IRQ flags per variant
  */
 struct pl031_vendor_data {
 	struct rtc_class_ops ops;
 	bool clockwatch;
 	bool st_weekday;
+	unsigned long irqflags;
 };
 
 struct pl031_local {
@@ -373,7 +375,7 @@  static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 
 	if (request_irq(adev->irq[0], pl031_interrupt,
-			0, "rtc-pl031", ldata)) {
+			vendor->irqflags, "rtc-pl031", ldata)) {
 		ret = -EIO;
 		goto out_no_irq;
 	}
@@ -403,6 +405,7 @@  static struct pl031_vendor_data arm_pl031 = {
 		.set_alarm = pl031_set_alarm,
 		.alarm_irq_enable = pl031_alarm_irq_enable,
 	},
+	.irqflags = IRQF_NO_SUSPEND,
 };
 
 /* The First ST derivative */
@@ -416,6 +419,7 @@  static struct pl031_vendor_data stv1_pl031 = {
 	},
 	.clockwatch = true,
 	.st_weekday = true,
+	.irqflags = IRQF_NO_SUSPEND,
 };
 
 /* And the second ST derivative */
@@ -429,6 +433,11 @@  static struct pl031_vendor_data stv2_pl031 = {
 	},
 	.clockwatch = true,
 	.st_weekday = true,
+	/*
+	 * This variant shares the IRQ with another block and must not
+	 * suspend that IRQ line.
+	 */
+	.irqflags = IRQF_SHARED | IRQF_NO_SUSPEND,
 };
 
 static struct amba_id pl031_ids[] = {