Message-ID: <20141002034345.GH3122@atomide.com>
Date:	Wed, 1 Oct 2014 20:43:46 -0700
From:	Tony Lindgren <tony@...mide.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Nishanth Menon <nm@...com>, lee.jones@...aro.org,
	LKML <linux-kernel@...r.kernel.org>, devicetree@...r.kernel.org,
	Keerthy <j-keerthy@...com>, Mark Brown <broonie@...aro.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	linux-omap@...r.kernel.org,
	LAK <linux-arm-kernel@...ts.infradead.org>,
	Kevin Hilman <khilman@...aro.org>
Subject: Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup

Hi Thomas,

* Thomas Gleixner <tglx@...utronix.de> [140919 12:47]:
> 
> The wakeup handler is supposed to bring the thing out of deep sleep
> and nothing else. All you want it to do is to mask itself and save the
> information that the real device irq is pending.
> 
> A stub handler for the wakeup irq is enough. We can have that in the
> irq/pm core and all it would do is simply:

Here's a patch along the lines of what you described, hopefully that's
fairly close to what you had in mind.

I also did play with the replaying of the interrupts but I don't think
that's needed. Well at least not for the omap case. I added some
comments about that to the code.

So far I've tested with the omap-serial and omap_hsmmc drivers. The
serial driver does not have any status as the device is powered off.
So replaying of the interrupt does not help there, we need to wait for
the next event anyways.

Then with omap_hsmmc the SDIO interrupt on dat1 line is level
sensitive and is noticed after the MMC controller is powered on
again. So no replaying of the device interrupt needed here either.

I still have not tested the MMC remux lines to GPIO for wake-up
events that's also needed for some omaps.

Regards,

Tony

8<-----------
From: Tony Lindgren <tony@...mide.com>
Date: Wed, 1 Oct 2014 14:56:35 -0700
Subject: [PATCH] genirq: Add support for wake-up interrupts to fix irq reentry issues in drivers

As pointed out by Thomas Gleixner, at least omap wake-up interrupts
have an issue with re-entrant interrupts because the wake-up interrupts
are now handled as a secondary interrupt controller. Further, the
wake-up interrupt just needs wake the system at least for omaps. So we
should just make the wake-up interrupt handling generic.

Note that at least initially we are keeping things simple by assuming the
wake-up interrupt is level sensitive, and the device pm_runtime_resume()
can deal with the situation, and no replaying of the lost device interrupts
is needed.

After tinkering with replaying of the lost device interrupts, my opinion is
that it should be avoided because of the issues listed in the comments of
this patch.

Signed-off-by: Tony Lindgren <tony@...mide.com>

--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -139,11 +139,15 @@ extern int __must_check
 request_percpu_irq(unsigned int irq, irq_handler_t handler,
 		   const char *devname, void __percpu *percpu_dev_id);
 
+struct device;
+
+extern int __must_check
+request_wake_irq(struct device *dev, unsigned int wakeirq,
+		 unsigned long irqflags);
+
 extern void free_irq(unsigned int, void *);
 extern void free_percpu_irq(unsigned int, void __percpu *);
 
-struct device;
-
 extern int __must_check
 devm_request_threaded_irq(struct device *dev, unsigned int irq,
 			  irq_handler_t handler, irq_handler_t thread_fn,
@@ -163,6 +167,10 @@ devm_request_any_context_irq(struct device *dev, unsigned int irq,
 		 irq_handler_t handler, unsigned long irqflags,
 		 const char *devname, void *dev_id);
 
+extern int __must_check
+devm_request_wake_irq(struct device *dev, unsigned int wakeirq,
+		      unsigned long irqflags);
+
 extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
 
 /*
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/random.h>
 #include <linux/interrupt.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/sched/rt.h>
@@ -1578,6 +1579,111 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler,
 }
 EXPORT_SYMBOL_GPL(request_any_context_irq);
 
+/**
+ *	handle_wakeirq_thread - call device runtime pm calls on wake-up interrupt
+ *	@wakeirq: device specific wake-up interrupt
+ *	@dev_id: struct device entry
+ */
+static irqreturn_t handle_wakeirq_thread(int wakeirq, void *dev_id)
+{
+	struct device *dev = dev_id;
+	irqreturn_t ret = IRQ_NONE;
+
+	if (pm_runtime_suspended(dev)) {
+		pm_runtime_mark_last_busy(dev);
+		pm_request_resume(dev);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+/**
+ *	setup_wakeirq - allocate a wake-up interrupt for a device
+ *	@dev: device to wake up
+ *	@wakeirq: interrupt that wakes up the device
+ *	@wakeflags: flags to pass to the interrupt handler
+ *	@devm: use devm
+ *
+ *	Note that the wake-up interrupt starts disabled. The wake-up interrupt
+ *	is typically enabled from the device pm_runtime_suspend() and disabled
+ *	again in the device pm_runtime_resume(). For runtime PM, the wake-up
+ *	interrupt should be always enabled, and for device suspend and resume,
+ *	the wake-up interrupt should be enabled depending on the device specific
+ *	configuration for device_can_wakeup().
+ *
+ *	Note also that we are not resending the lost device interrupts.
+ *	We assume that the wake-up interrupt just needs to wake-up the device,
+ *	and then device pm_runtime_resume() can deal with the situation.
+ *
+ *	There are at least the following reasons to not resend the lost device
+ *	interrupts automatically based on the wake-up interrupt:
+ *
+ *	1. There can be interrupt reentry issues calling the device interrupt
+ *	   based on the wake-up interrupt if done in the device driver. It
+ *	   could be done with check_irq_resend() after checking the device
+ *	   interrupt mask if we really wanted to though.
+ *
+ *	2. The device interrupt handler would need to be set up properly with
+ *	   pm_runtime_irq_safe(). Ideally you don't want to call pm_runtime
+ *	   calls from the device interrupt handler at all.
+ *
+ *	3. The IRQ subsystem may not know if it's safe to call the device
+ *	   interrupt unless the driver updates the interrupt status with
+ *	   disable_irq() and enable_irq() in addition to just disabling the
+ *	   interrupt at the hardware level in the device registers.
+ *
+ *	So if replaying the lost device interrupts is absolutely needed from the
+ *	hardware point of view, it's probably best to set up a completely
+ *	separate wake-up interrupt handler for the wake-up interrupt in the
+ *	device driver because of the reasons above.
+ */
+static int setup_wakeirq(struct device *dev, unsigned int wakeirq,
+			 unsigned long wakeflags, bool devm)
+{
+	int ret;
+
+	if (!(dev && wakeirq)) {
+		pr_err("Missing device or wakeirq for %s irq %d\n",
+		       dev_name(dev), wakeirq);
+		return -EINVAL;
+	}
+
+	if (!(wakeflags &
+	      (IRQF_TRIGGER_LOW | IRQF_TRIGGER_HIGH | IRQF_ONESHOT))) {
+		pr_err("Invalid wakeirq for %s irq %d, must be level oneshot\n",
+		       dev_name(dev), wakeirq);
+		return -EINVAL;
+	}
+
+	irq_set_status_flags(wakeirq, _IRQ_NOAUTOEN);
+
+	if (devm)
+		ret = devm_request_threaded_irq(dev, wakeirq, NULL,
+						handle_wakeirq_thread,
+						wakeflags, dev_name(dev), dev);
+	else
+		ret = request_threaded_irq(wakeirq, NULL,
+					   handle_wakeirq_thread,
+					   wakeflags, dev_name(dev), dev);
+
+	return ret;
+}
+
+int request_wake_irq(struct device *dev, unsigned int wakeirq,
+		     unsigned long wakeflags)
+{
+	return setup_wakeirq(dev, wakeirq, wakeflags, false);
+}
+EXPORT_SYMBOL(request_wake_irq);
+
+int devm_request_wake_irq(struct device *dev, unsigned int wakeirq,
+			  unsigned long wakeflags)
+{
+	return setup_wakeirq(dev, wakeirq, wakeflags, false);
+}
+EXPORT_SYMBOL(devm_request_wake_irq);
+
 void enable_percpu_irq(unsigned int irq, unsigned int type)
 {
 	unsigned int cpu = smp_processor_id();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists