[PATCH, RFC] wake up from a serial port

[PATCH, RFC] wake up from a serial port

[Guennadi Liakhovetski]

A number of Linkstation models from Buffalo Technology with PPC, ARM, and 
also MIPS (I think) CPUs have a power-management controller connected to a 
UART. Among other things that chip controlls power and reset buttons. 
Working on a standby support for one of these systems (ppc mpc8241 based), 
the only suitable wakeup source there is the power button, which means, I 
have to configure one of the two system UARTs to not be suspendsd. Using 
the device_*_wakeup API doesn't quite work because both serial ports share 
one device. The below patch proposes a new port flag UPF_MAY_WAKEUP to 
configure such UARTs. It also adds support for a new "can-wakeup" serial 
node property to the legacy_serial driver.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index cea8045..888d9bb 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -51,6 +51,9 @@ static int __init add_legacy_port(struct device_node *np, int want_index,
 	/* get default speed if present */
 	spd = of_get_property(np, "current-speed", NULL);
 
+	if (of_find_property(np, "can-wakeup", NULL))
+		flags |= UPF_MAY_WAKEUP;
+
 	/* If we have a location index, then try to use it */
 	if (want_index >= 0 && want_index < MAX_LEGACY_SERIAL_PORTS)
 		index = want_index;
diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 0b3ec38..77dd552 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -130,6 +130,7 @@ struct uart_8250_port {
 	unsigned char		mcr_mask;	/* mask of user bits */
 	unsigned char		mcr_force;	/* mask of forced bits */
 	unsigned char		lsr_break_flag;
+	char			suspended;
 
 	/*
 	 * We provide a per-port pm hook.
@@ -2680,8 +2681,14 @@ static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
 	for (i = 0; i < UART_NR; i++) {
 		struct uart_8250_port *up = &serial8250_ports[i];
 
-		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
-			uart_suspend_port(&serial8250_reg, &up->port);
+		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev) {
+			if (up->port.flags & UPF_MAY_WAKEUP)
+				enable_irq_wake(up->port.irq);
+			else {
+				uart_suspend_port(&serial8250_reg, &up->port);
+				up->suspended = 1;
+			}
+		}
 	}
 
 	return 0;
@@ -2694,8 +2701,13 @@ static int serial8250_resume(struct platform_device *dev)
 	for (i = 0; i < UART_NR; i++) {
 		struct uart_8250_port *up = &serial8250_ports[i];
 
-		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
-			serial8250_resume_port(i);
+		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev) {
+			if (up->suspended) {
+				serial8250_resume_port(i);
+				up->suspended = 0;
+			} else
+				disable_irq_wake(up->port.irq);
+		}
 	}
 
 	return 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 773d8d8..d585967 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -272,6 +272,7 @@ struct uart_port {
 #define UPF_LOW_LATENCY		((__force upf_t) (1 << 13))
 #define UPF_BUGGY_UART		((__force upf_t) (1 << 14))
 #define UPF_MAGIC_MULTIPLIER	((__force upf_t) (1 << 16))
+#define UPF_MAY_WAKEUP		((__force upf_t) (1 << 17))
 #define UPF_CONS_FLOW		((__force upf_t) (1 << 23))
 #define UPF_SHARE_IRQ		((__force upf_t) (1 << 24))
 #define UPF_BOOT_AUTOCONF	((__force upf_t) (1 << 28))

Re: [PATCH, RFC] wake up from a serial port

[Scott Wood]

On Mon, Aug 13, 2007 at 12:27:30AM +0200, Guennadi Liakhovetski wrote:
> A number of Linkstation models from Buffalo Technology with PPC, ARM, and 
> also MIPS (I think) CPUs have a power-management controller connected to a 
> UART. Among other things that chip controlls power and reset buttons. 
> Working on a standby support for one of these systems (ppc mpc8241 based), 
> the only suitable wakeup source there is the power button, which means, I 
> have to configure one of the two system UARTs to not be suspendsd. Using 
> the device_*_wakeup API doesn't quite work because both serial ports share 
> one device. The below patch proposes a new port flag UPF_MAY_WAKEUP to 
> configure such UARTs. It also adds support for a new "can-wakeup" serial 
> node property to the legacy_serial driver.

Shouldn't the ability to wakeup be configured through sysfs, rather than
encoded into the device tree?  I'm assuming this is just a matter of
configuration, and not that the hardware supports waking from one and not
the other.

-Scott

Re: [PATCH, RFC] wake up from a serial port

[Guennadi Liakhovetski]

On Mon, 13 Aug 2007, Scott Wood wrote:

> On Mon, Aug 13, 2007 at 12:27:30AM +0200, Guennadi Liakhovetski wrote:
> > A number of Linkstation models from Buffalo Technology with PPC, ARM, and 
> > also MIPS (I think) CPUs have a power-management controller connected to a 
> > UART. Among other things that chip controlls power and reset buttons. 
> > Working on a standby support for one of these systems (ppc mpc8241 based), 
> > the only suitable wakeup source there is the power button, which means, I 
> > have to configure one of the two system UARTs to not be suspendsd. Using 
> > the device_*_wakeup API doesn't quite work because both serial ports share 
> > one device. The below patch proposes a new port flag UPF_MAY_WAKEUP to 
> > configure such UARTs. It also adds support for a new "can-wakeup" serial 
> > node property to the legacy_serial driver.
> 
> Shouldn't the ability to wakeup be configured through sysfs, rather than
> encoded into the device tree?  I'm assuming this is just a matter of
> configuration, and not that the hardware supports waking from one and not
> the other.

Well, sort of. One of them is more "natural" - it has a button on the 
front panel, to use the other one you have to modify the hardware. 
However, I like the idea - generally it does seem to be a better approach 
to have it run-time configurable over sysfs... Only - how? The only 
differentitaion ATM between the two ports are these two links:

# ls -l /sys/devices/platform/serial8250.0/tty*
lrwxrwxrwx 1 root root 0 Aug 13 22:05 /sys/devices/platform/serial8250.0/tty:ttyS0 -> ../../../class/tty/ttyS0
lrwxrwxrwx 1 root root 0 Aug 13 22:05 /sys/devices/platform/serial8250.0/tty:ttyS1 -> ../../../class/tty/ttyS1

And placing some wakeup file under the class/tty/ directory doesn't seem 
very consistent with the current policy - until now they only live under 
devices/... (Greg added to cc:).

Actually, it is good you replied, Scott:-) I wanted to ask you about the 
following: I've switched to your generic suspend/resume routines using the 
_TLF_NAPPING bit, the arch_suspend_{dis,en}able_irqs() hooks... On wakeup 
your _TLF_NAPPING trick should bypass calling the ISR and jump directly to 
the resume code. However, on wakeup, it looks like I do get the wakeup 
interrupt too. Is it the correct behaviour and is this the (approximately) 
correct explanation why:

1. the AVR connected to ttyS0 sends 1 byte on button press and 1 byte on 
button release. So, normally you would get 2 bytes and 2 interupts for one 
such button down-up.

2. Interrupt is configured as edge (is it correct - haven't found in 
mpc8245um, UARTs are usually edge), so, 

--- button down -> byte #1 -> IRQ line active -> IC interrupts
--- on resume interrupts are disabled, an EOI is performed (the line is 
	still active)
--- interrupts are re-enabled 

3. a second interrupt for the same byte is delivered.

I'm just trying to understand whether this is the correct and expected 
behaviour or something is wrong here.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH, RFC] wake up from a serial port

[Scott Wood]

Guennadi Liakhovetski wrote:
> Well, sort of. One of them is more "natural" - it has a button on the 
> front panel, to use the other one you have to modify the hardware. 
> However, I like the idea - generally it does seem to be a better approach 
> to have it run-time configurable over sysfs... Only - how? The only 
> differentitaion ATM between the two ports are these two links:
> 
> # ls -l /sys/devices/platform/serial8250.0/tty*
> lrwxrwxrwx 1 root root 0 Aug 13 22:05 /sys/devices/platform/serial8250.0/tty:ttyS0 -> ../../../class/tty/ttyS0
> lrwxrwxrwx 1 root root 0 Aug 13 22:05 /sys/devices/platform/serial8250.0/tty:ttyS1 -> ../../../class/tty/ttyS1
> 
> And placing some wakeup file under the class/tty/ directory doesn't seem 
> very consistent with the current policy - until now they only live under 
> devices/... (Greg added to cc:).

Hmm... I'd assumed each port would have its own device directory.  Would 
anything break horribly if it were changed so that each tty:ttySx is a 
directory, which contains both a wakeup file and the symlink?

> Actually, it is good you replied, Scott:-) I wanted to ask you about the 
> following: I've switched to your generic suspend/resume routines using the 
> _TLF_NAPPING bit, the arch_suspend_{dis,en}able_irqs() hooks... On wakeup 
> your _TLF_NAPPING trick should bypass calling the ISR and jump directly to 
> the resume code. However, on wakeup, it looks like I do get the wakeup 
> interrupt too.

You should get the interrupt, but not until after the PM code enables 
IRQs.  Are you saying that the interrupt handler runs before then?

> Is it the correct behaviour and is this the (approximately) 
> correct explanation why:
> 
> 1. the AVR connected to ttyS0 sends 1 byte on button press and 1 byte on 
> button release. So, normally you would get 2 bytes and 2 interupts for one 
> such button down-up.
> 
> 2. Interrupt is configured as edge (is it correct - haven't found in 
> mpc8245um, UARTs are usually edge), so, 
> 
> --- button down -> byte #1 -> IRQ line active -> IC interrupts
> --- on resume interrupts are disabled, an EOI is performed (the line is 
> 	still active)
> --- interrupts are re-enabled 
> 
> 3. a second interrupt for the same byte is delivered.

No EOI is performed -- the idea is to defer the interrupt, not swallow 
it.  All that is done to defer the interrupt is clearing MSR[EE].

-Scott

Re: [PATCH, RFC] wake up from a serial port

[Guennadi Liakhovetski]

On Mon, 13 Aug 2007, Scott Wood wrote:

> Guennadi Liakhovetski wrote:
> > 
> > # ls -l /sys/devices/platform/serial8250.0/tty*
> > lrwxrwxrwx 1 root root 0 Aug 13 22:05
> > /sys/devices/platform/serial8250.0/tty:ttyS0 -> ../../../class/tty/ttyS0
> > lrwxrwxrwx 1 root root 0 Aug 13 22:05
> > /sys/devices/platform/serial8250.0/tty:ttyS1 -> ../../../class/tty/ttyS1
> > 
> > And placing some wakeup file under the class/tty/ directory doesn't seem
> > very consistent with the current policy - until now they only live under
> > devices/... (Greg added to cc:).
> 
> Hmm... I'd assumed each port would have its own device directory.  Would
> anything break horribly if it were changed so that each tty:ttySx is a
> directory, which contains both a wakeup file and the symlink?

Yeah, I'd love to know the answer too:-) As you see, atm it is one 
platform device as defined in arch/powerpc/kernel/legacy_serial.c:

static struct platform_device serial_device = {
	.name	= "serial8250",
	.id	= PLAT8250_DEV_PLATFORM,
	.dev	= {
		.platform_data = legacy_serial_ports,
	},
};

with a list of ports in platform data:

static struct plat_serial8250_port legacy_serial_ports[MAX_LEGACY_SERIAL_PORTS+1];

Hence one device directory. Same on a PC

$ ls -l /sys/devices/platform/serial8250/tty*
lrwxrwxrwx  1 root root 0 Aug 13 23:10 /sys/devices/platform/serial8250/tty:ttyS0 -> ../../../class/tty/ttyS0
lrwxrwxrwx  1 root root 0 Aug 13 23:10 /sys/devices/platform/serial8250/tty:ttyS1 -> ../../../class/tty/ttyS1
lrwxrwxrwx  1 root root 0 Aug 13 23:10 /sys/devices/platform/serial8250/tty:ttyS2 -> ../../../class/tty/ttyS2
lrwxrwxrwx  1 root root 0 Aug 13 23:10 /sys/devices/platform/serial8250/tty:ttyS3 -> ../../../class/tty/ttyS3

> You should get the interrupt, but not until after the PM code enables IRQs.
> Are you saying that the interrupt handler runs before then?

Great, it is working correctly then! Don't think the ISR runs return from 
PM, no.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH, RFC] wake up from a serial port

[Greg KH]

On Mon, Aug 13, 2007 at 11:14:22PM +0200, Guennadi Liakhovetski wrote:
> On Mon, 13 Aug 2007, Scott Wood wrote:
> 
> > Guennadi Liakhovetski wrote:
> > > 
> > > # ls -l /sys/devices/platform/serial8250.0/tty*
> > > lrwxrwxrwx 1 root root 0 Aug 13 22:05
> > > /sys/devices/platform/serial8250.0/tty:ttyS0 -> ../../../class/tty/ttyS0
> > > lrwxrwxrwx 1 root root 0 Aug 13 22:05
> > > /sys/devices/platform/serial8250.0/tty:ttyS1 -> ../../../class/tty/ttyS1
> > > 
> > > And placing some wakeup file under the class/tty/ directory doesn't seem
> > > very consistent with the current policy - until now they only live under
> > > devices/... (Greg added to cc:).
> > 
> > Hmm... I'd assumed each port would have its own device directory.  Would
> > anything break horribly if it were changed so that each tty:ttySx is a
> > directory, which contains both a wakeup file and the symlink?

No, you are already in the tty device directory in the first place, the
tty:ttyS1 is just a symlink to the class in case you need the thing.

Let's follow things around:

	~ $ cd /sys/class/tty
	/sys/class/tty $ ls -l | grep ttyS
	lrwxrwxrwx 1 root root 0 Aug 12 20:12 ttyS0 -> ../../devices/platform/serial8250/tty/ttyS0
	lrwxrwxrwx 1 root root 0 Aug 12 20:12 ttyS1 -> ../../devices/platform/serial8250/tty/ttyS1
	lrwxrwxrwx 1 root root 0 Aug 12 20:12 ttyS2 -> ../../devices/platform/serial8250/tty/ttyS2
	lrwxrwxrwx 1 root root 0 Aug 12 20:12 ttyS3 -> ../../devices/platform/serial8250/tty/ttyS3
	/sys/class/tty $ cd ../../devices/platform/serial8250/tty/ttyS0
	/sys/devices/platform/serial8250/tty/ttyS0 $ ls
	dev  device  power  subsystem  uevent
	/sys/devices/platform/serial8250/tty/ttyS0 $ cd ..
	/sys/devices/platform/serial8250/tty $ $ ls -l
	total 0
	drwxr-xr-x 3 root root 0 Aug 12 20:12 ttyS0
	drwxr-xr-x 3 root root 0 Aug 12 20:12 ttyS1
	drwxr-xr-x 3 root root 0 Aug 12 20:12 ttyS2
	drwxr-xr-x 3 root root 0 Aug 12 20:12 ttyS3
	/sys/devices/platform/serial8250/tty $ cd ..
	gregkh@mini /sys/devices/platform/serial8250 $ ls -l
	total 0
	lrwxrwxrwx 1 root root    0 Aug 12 20:13 driver -> ../../../bus/platform/drivers/serial8250
	-r--r--r-- 1 root root 4096 Aug 13 15:24 modalias
	drwxr-xr-x 2 root root    0 Aug 13 15:24 power
	lrwxrwxrwx 1 root root    0 Aug 12 20:13 subsystem -> ../../../bus/platform
	drwxr-xr-x 6 root root    0 Aug 12 20:12 tty
	-rw-r--r-- 1 root root 4096 Aug 12 20:12 uevent

So, the serial8250 device is the "bridge" for the 4 different serial
ports in my machine.  You have the tty:ttyS? symlinks in that directory
as you have CONFIG_SYSFS_DEPRECATED still enabled, but the directory
structure should all still be the same for you.

So, if you want to put things into the tty device's directory, you can,
they will just show up in the proper place, under
/sys/devices/platform/serial8250/tty/ttyS0 for the first serial port.

Does that make sense?

thanks,

greg k-h

Re: [PATCH, RFC] wake up from a serial port

[Guennadi Liakhovetski]

Enable wakeup from serial ports, make it run-time configurable over sysfs, 
e.g.,

echo enabled > /sys/devices/platform/serial8250.0/tty/ttyS0/power/wakeup

Requires

# CONFIG_SYSFS_DEPRECATED is not set

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

---

(I still find it strange having to put a formal patch description above 
and its discussion below, anyway)

On Mon, 13 Aug 2007, Greg KH wrote:

> So, the serial8250 device is the "bridge" for the 4 different serial
> ports in my machine.  You have the tty:ttyS? symlinks in that directory
> as you have CONFIG_SYSFS_DEPRECATED still enabled, but the directory
> structure should all still be the same for you.
> 
> So, if you want to put things into the tty device's directory, you can,
> they will just show up in the proper place, under
> /sys/devices/platform/serial8250/tty/ttyS0 for the first serial port.
> 
> Does that make sense?

I think it does... Following this, and a suggestion from Scott to switch 
wakeup-enable on and off at run-time over sysfs, here's another attempt... 
Well, easy to see I'm out in the wild here in this area (pm/sysfs/tty), 
so, this might well be way off... Please, comment.

Thanks
Guennadi

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 0b3ec38..5118914 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -130,6 +130,7 @@ struct uart_8250_port {
 	unsigned char		mcr_mask;	/* mask of user bits */
 	unsigned char		mcr_force;	/* mask of forced bits */
 	unsigned char		lsr_break_flag;
+	char			suspended;
 
 	/*
 	 * We provide a per-port pm hook.
@@ -2673,6 +2674,14 @@ static int __devexit serial8250_remove(struct platform_device *dev)
 	return 0;
 }
 
+static int serial8250_match_port(struct device *dev, void *data)
+{
+	struct uart_port *port = data;
+	dev_t devt = MKDEV(serial8250_reg.major, serial8250_reg.minor) + port->line;
+
+	return dev->devt == devt; /* Actually, only one tty per port */
+}
+
 static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
 {
 	int i;
@@ -2680,8 +2689,16 @@ static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
 	for (i = 0; i < UART_NR; i++) {
 		struct uart_8250_port *up = &serial8250_ports[i];
 
-		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
-			uart_suspend_port(&serial8250_reg, &up->port);
+		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev) {
+			struct device *tty_dev = device_find_child(up->port.dev, &up->port,
+								   serial8250_match_port);
+			if (device_may_wakeup(tty_dev))
+				enable_irq_wake(up->port.irq);
+			else {
+				uart_suspend_port(&serial8250_reg, &up->port);
+				up->suspended = 1;
+			}
+		}
 	}
 
 	return 0;
@@ -2694,8 +2711,13 @@ static int serial8250_resume(struct platform_device *dev)
 	for (i = 0; i < UART_NR; i++) {
 		struct uart_8250_port *up = &serial8250_ports[i];
 
-		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
-			serial8250_resume_port(i);
+		if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev) {
+			if (up->suspended) {
+				serial8250_resume_port(i);
+				up->suspended = 0;
+			} else
+				disable_irq_wake(up->port.irq);
+		}
 	}
 
 	return 0;
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 9c57486..716fbe2 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2266,6 +2266,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *port)
 {
 	struct uart_state *state;
 	int ret = 0;
+	struct device *tty_dev;
 
 	BUG_ON(in_interrupt());
 
@@ -2301,7 +2302,13 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *port)
 	 * Register the port whether it's detected or not.  This allows
 	 * setserial to be used to alter this ports parameters.
 	 */
-	tty_register_device(drv->tty_driver, port->line, port->dev);
+	tty_dev = tty_register_device(drv->tty_driver, port->line, port->dev);
+	if (likely(!IS_ERR(tty_dev))) {
+		device_can_wakeup(tty_dev) = 1;
+		device_set_wakeup_enable(tty_dev, 0);
+	} else
+		printk(KERN_ERR "Cannot register tty device on line %d\n",
+		       port->line);
 
 	/*
 	 * If this driver supports console, and it hasn't been