[PATCH] modify pl011 driver to let it work as wakeup source

[PATCH] modify pl011 driver to let it work as wakeup source

[Zhaoyang Huang]

the commit use the latest dev_pm_set_wake_irq API instead
of the enable_irq_wake and IRQF_NO_SUSPEND to configure the
ttyAMA device to work as the wakeup source

Signed-off-by: Zhaoyang Huang <zhaoyang.huang@linaro.org>
---
 drivers/tty/serial/amba-pl011.c |   47 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index fd27e98..1923b94 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -59,6 +59,11 @@
 #include <linux/sizes.h>
 #include <linux/io.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/irq.h>
+
+static struct device *uart_dev;
 
 #define UART_NR			14
 
@@ -2353,11 +2358,29 @@ static int pl011_register_port(struct uart_amba_port *uap)
 	return ret;
 }
 
+struct uart_match {
+	struct uart_port *port;
+	struct uart_driver *driver;
+};
+
+static int match_uart_port(struct device *dev, void *data)
+{
+	struct uart_match *match = data;
+
+	dev_t devt = MKDEV(match->driver->major, match->driver->minor) +
+		match->port->line;
+
+	pr_info("the match data of ttyAMA0 is %d %d\n",
+			(int)devt, (int)dev->devt);
+
+	return dev->devt == devt; /* Actually, only one tty per port */
+}
 static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 {
 	struct uart_amba_port *uap;
 	struct vendor_data *vendor = id->data;
 	int portnr, ret;
+	struct uart_match match;
 
 	portnr = pl011_find_free_port();
 	if (portnr < 0)
@@ -2387,13 +2410,35 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 
 	amba_set_drvdata(dev, uap);
 
-	return pl011_register_port(uap);
+	ret = pl011_register_port(uap);
+
+	if (!of_find_property(dev->dev.of_node, "linux,wakeup", NULL)) {
+		uart_dev = NULL;
+		return ret;
+	}
+
+	match.port = &uap->port;
+	match.driver = &amba_reg;
+
+	uart_dev = device_find_child(&dev->dev, &match, match_uart_port);
+
+	if (uart_dev) {
+		device_init_wakeup(uart_dev, true);
+		dev_pm_set_wake_irq(uart_dev, uap->port.irq);
+	}
+
+	return ret;
 }
 
 static int pl011_remove(struct amba_device *dev)
 {
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
 
+	if (uart_dev) {
+		dev_pm_clear_wake_irq(uart_dev);
+		device_init_wakeup(uart_dev, false);
+	}
+
 	uart_remove_one_port(&amba_reg, &uap->port);
 	pl011_unregister_port(uap);
 	return 0;
-- 
1.7.9.5

Re: [PATCH] modify pl011 driver to let it work as wakeup source

[Russell King - ARM Linux]

On Thu, Sep 17, 2015 at 04:31:56PM +0800, Zhaoyang Huang wrote:
> +struct uart_match {
> +	struct uart_port *port;
> +	struct uart_driver *driver;
> +};
> +
> +static int match_uart_port(struct device *dev, void *data)
> +{
> +	struct uart_match *match = data;
> +
> +	dev_t devt = MKDEV(match->driver->major, match->driver->minor) +
> +		match->port->line;
> +
> +	pr_info("the match data of ttyAMA0 is %d %d\n",
> +			(int)devt, (int)dev->devt);
> +
> +	return dev->devt == devt; /* Actually, only one tty per port */
> +}
...
> +	ret = pl011_register_port(uap);
> +
> +	if (!of_find_property(dev->dev.of_node, "linux,wakeup", NULL)) {
> +		uart_dev = NULL;
> +		return ret;
> +	}
> +
> +	match.port = &uap->port;
> +	match.driver = &amba_reg;
> +
> +	uart_dev = device_find_child(&dev->dev, &match, match_uart_port);
> +
> +	if (uart_dev) {
> +		device_init_wakeup(uart_dev, true);
> +		dev_pm_set_wake_irq(uart_dev, uap->port.irq);
> +	}
> +
> +	return ret;

I can only describe this code as a hack.  This looks like something
which should be handled by generic infrastructure at an appropriate
point (somewhere inside uart_add_one_port()), not by hacky code in
each driver.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] modify pl011 driver to let it work as wakeup source

[Sudeep Holla]

On 17/09/15 09:31, Zhaoyang Huang wrote:
> the commit use the latest dev_pm_set_wake_irq API instead
> of the enable_irq_wake and IRQF_NO_SUSPEND to configure the
> ttyAMA device to work as the wakeup source
>

As I mentioned earlier[1], if this a based on Juno platform, then
it gets NACK. Please add this feature when it's needed on a real
platform on which hardware supports UART/PL011 as a wake source.

Do you have any such platform to test ? On Juno, it can't be used as
wakeup source.

Regards,
Sudeep

[1] https://www.marc.info/?l=linux-pm&m=144239396227080&w=2

Re: [PATCH] modify pl011 driver to let it work as wakeup source

[Sudeep Holla]

On 17/09/15 12:01, Zhaoyang Huang wrote:
>
>
> On 17 September 2015 at 18:39, Sudeep Holla <sudeep.holla@arm.com
> <mailto:sudeep.holla@arm.com>> wrote:
>
>
>
>     On 17/09/15 09:31, Zhaoyang Huang wrote:
>
>         the commit use the latest dev_pm_set_wake_irq API instead
>         of the enable_irq_wake and IRQF_NO_SUSPEND to configure the
>         ttyAMA device to work as the wakeup source
>
>
>     As I mentioned earlier[1], if this a based on Juno platform, then
>     it gets NACK. Please add this feature when it's needed on a real
>     platform on which hardware supports UART/PL011 as a wake source.
>
>     Do you have any such platform to test ? On Juno, it can't be used as
>     wakeup source.
>
>     Regards,
>     Sudeep
>
>     [1] https://www.marc.info/?l=linux-pm&m=144239396227080&w=2
>
> Hi Sudeep,
> I think the wakeup source you mean is restricted to the device which can
> wake S2R. However, I think the S2I should be also considered, which need
> not the hardware support for powering the core up, but just need the isr
> to be keep active etc.

Hmm, I disagree as we have single control for wakeup and what if you
have enabled PL011 as wakeup and S2R is entered assuming there's one
active wakeup anyway. IMO w.r.t wakeup source we *must* assume it also
wakes up from S2R state always.

You didn't answer to my earlier query, why didn't you choose other
interrupts like ethernet, gpio or just pick one from the lot of
interrupts on Juno as wakeup from S2I. What was the rationale behind
your choice especially on Juno ? Just because you can send and receive
chars via the tty/console, what if I don't have access to console.
We need much better and hardware supported wakeup source like RTC or GPIO.

Also a SoC can enter deeper idle states in S2I where UART can't wakeup.
How would you know that ? I re-iterate that hardware should have support
to ensure it can wakeup the system.

Regards,
Sudeep