ehci_hcd causes immediate wakeup from suspend to RAM or disk on Asus P4P800-VM

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

Does anyone know why is this enabled by default?

No other USB host driver
(except oxu210hp-hcd which is based on EHCI) does that.

This also means that
suspended laptop wakes up when disconnecting a mouse?

I don't have any to test
right now. Wakeup on USB connect makes a bit more sense but I'd say that it's still not a good idea to enable it by default.

If we don't need that, perhaps the following patch should be applied.

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

Does anyone know why is this enabled by default?

No other USB host driver
(except oxu210hp-hcd which is based on EHCI) does that.

This also means that
suspended laptop wakes up when disconnecting a mouse?

I don't have any to test
right now. Wakeup on USB connect makes a bit more sense but I'd say that it's still not a good idea to enable it by default.

If we don't need that, perhaps the following patch should be applied.

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

Quoted 3 lines · Ondrej Zary

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Quoted 1 line

Does anyone know why is this enabled by default?

Why _what_ is enabled? Detection of disconnects? Because otherwise your computer wouldn't realize anything had happened when a suspended USB device was unplugged from a suspended root hub.

Quoted 2 lines

No other USB host driver
(except oxu210hp-hcd which is based on EHCI) does that.

Look again -- they all do. (All the HCDs that support suspend/resume, anyway.)

Quoted 2 lines

This also means that
suspended laptop wakes up when disconnecting a mouse?

No, for the reason I described above. The controller is aware of the wakeup request but doesn't generate a PME# event. Likewise for desktop systems.

Quoted 7 lines

I don't have any to test
right now. Wakeup on USB connect makes a bit more sense but I'd say that it's still not a good idea to enable it by default.

If we don't need that, perhaps the following patch should be applied.

Quoted 5 lines

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Quoted 3 lines · Ondrej Zary

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Quoted 1 line

Does anyone know why is this enabled by default?

Why _what_ is enabled? Detection of disconnects? Because otherwise your computer wouldn't realize anything had happened when a suspended USB device was unplugged from a suspended root hub.

Quoted 2 lines

No other USB host driver
(except oxu210hp-hcd which is based on EHCI) does that.

Look again -- they all do. (All the HCDs that support suspend/resume, anyway.)

Quoted 2 lines

This also means that
suspended laptop wakes up when disconnecting a mouse?

No, for the reason I described above. The controller is aware of the wakeup request but doesn't generate a PME# event. Likewise for desktop systems.

Quoted 7 lines

I don't have any to test
right now. Wakeup on USB connect makes a bit more sense but I'd say that it's still not a good idea to enable it by default.

If we don't need that, perhaps the following patch should be applied.

Quoted 5 lines

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Quoted 2 lines

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Maybe because I don't want all my USB devices to be powered when the system is turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power to USB ports.

Quoted 2 lines

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Maybe because I don't want all my USB devices to be powered when the system is turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power to USB ports.

Quoted 10 lines · On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:

Quoted 3 lines · Ondrej Zary

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Quoted 5 lines

Quoted 1 line

Does anyone know why is this enabled by default?

Why _what_ is enabled? Detection of disconnects? Because otherwise your computer wouldn't realize anything had happened when a suspended USB device was unplugged from a suspended root hub.

That's not disconnect detection - that's wakeup on disconnect.

If I understand
EHCI 1.0 specification correctly, disconnect detection should work regardless of the state of this bit:

| PORTSC bit 21: Wake on Disconnect Enable (WKDSCNNT_E):
| R/W. Default = 0b.
| Writing this bit to a one enables the port to be sensitive to device
| disconnects as wake-up events. See Section 4.3 for effects of this bit on
| resume event behavior. Refer to Section 4.3.1 for operational model.  

And it really does. With this patch applied, system does not wake up when a device is disconnected during suspend. When I wake up the system manually, the disconnect is detected immediately (does not matter

Quoted 9 lines

Quoted 8 lines

If we don't need that, perhaps the following patch should be applied.

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

Quoted 2 lines

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Maybe because I don't want all my USB devices to be powered when the system is turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power to USB ports.

Quoted 10 lines · On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:

Quoted 3 lines · Ondrej Zary

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Quoted 5 lines

Quoted 1 line

Does anyone know why is this enabled by default?

Why _what_ is enabled? Detection of disconnects? Because otherwise your computer wouldn't realize anything had happened when a suspended USB device was unplugged from a suspended root hub.

That's not disconnect detection - that's wakeup on disconnect.

If I understand
EHCI 1.0 specification correctly, disconnect detection should work regardless of the state of this bit:

| PORTSC bit 21: Wake on Disconnect Enable (WKDSCNNT_E):
| R/W. Default = 0b.
| Writing this bit to a one enables the port to be sensitive to device
| disconnects as wake-up events. See Section 4.3 for effects of this bit on
| resume event behavior. Refer to Section 4.3.1 for operational model.  

And it really does. With this patch applied, system does not wake up when a device is disconnected during suspend. When I wake up the system manually, the disconnect is detected immediately (does not matter

Quoted 9 lines

Quoted 8 lines

If we don't need that, perhaps the following patch should be applied.

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

Quoted 2 lines

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Maybe because I don't want all my USB devices to be powered when the system is turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power to USB ports.

Quoted 12 lines · Ondrej Zary

Quoted 10 lines · On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:

Quoted 3 lines · Ondrej Zary

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Quoted 3 lines

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Okay, evidently the hardware or firmware on these boards is buggy. Other systems do not have the same problem, as far as I know.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Does anyone know why is this enabled by default?

Why _what_ is enabled? Detection of disconnects? Because otherwise your computer wouldn't realize anything had happened when a suspended USB device was unplugged from a suspended root hub.

That's not disconnect detection - that's wakeup on disconnect.

True; I oversimplified. Furthermore, starting in 2.6.34, the wakeup settings during system sleep (suspend or hibernation) can be different from the settings during autosuspend, so you can have root hubs enabled for wakeup during autosuspend but not during system sleep.

Quoted 11 lines

If I understand
EHCI 1.0 specification correctly, disconnect detection should work regardless

of the state of this bit:

| PORTSC bit 21: Wake on Disconnect Enable (WKDSCNNT_E):
| R/W. Default = 0b.
| Writing this bit to a one enables the port to be sensitive to device
| disconnects as wake-up events. See Section 4.3 for effects of this bit
| on resume event behavior. Refer to Section 4.3.1 for operational model.

Quoted 3 lines

And it really does. With this patch applied, system does not wake up when a device is disconnected during suspend. When I wake up the system manually, the disconnect is detected immediately (does not matter

It's worth pointing out that EHCI is different in this respect from OHCI and UHCI; the older controllers do not have the capability to enable or disable wakeup independently for connect, disconnect, and overcurrent events. They are all or nothing. So are external USB hubs.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

If we don't need that, perhaps the following patch should be applied.

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

Quoted 6 lines

Quoted 2 lines

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Maybe because I don't want all my USB devices to be powered when the system is turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power to USB ports.

This depends on how your system was turned off. During suspend or hibernation, you _should_ want USB devices to be powered (and some people do, as Greg pointed out). During a normal system shutdown, the USB buses should not be powered.

Regardless, I don't think a kernel patch is the way to solve your problem. Changing the wakeup setting for the root hub will do what you want, and that setting is explicitly intended to be controlled by userspace (after all, that's why it is exposed in sysfs). The initial value is only a reasonable default; you can certainly add scripts or udev rules to disable wakeup on your EHCI root hub.

Quoted 12 lines · Ondrej Zary

Quoted 10 lines · On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:

Quoted 3 lines · Ondrej Zary

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Quoted 3 lines

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Okay, evidently the hardware or firmware on these boards is buggy. Other systems do not have the same problem, as far as I know.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Does anyone know why is this enabled by default?

Why _what_ is enabled? Detection of disconnects? Because otherwise your computer wouldn't realize anything had happened when a suspended USB device was unplugged from a suspended root hub.

That's not disconnect detection - that's wakeup on disconnect.

True; I oversimplified. Furthermore, starting in 2.6.34, the wakeup settings during system sleep (suspend or hibernation) can be different from the settings during autosuspend, so you can have root hubs enabled for wakeup during autosuspend but not during system sleep.

Quoted 11 lines

If I understand
EHCI 1.0 specification correctly, disconnect detection should work regardless

of the state of this bit:

| PORTSC bit 21: Wake on Disconnect Enable (WKDSCNNT_E):
| R/W. Default = 0b.
| Writing this bit to a one enables the port to be sensitive to device
| disconnects as wake-up events. See Section 4.3 for effects of this bit
| on resume event behavior. Refer to Section 4.3.1 for operational model.

Quoted 3 lines

And it really does. With this patch applied, system does not wake up when a device is disconnected during suspend. When I wake up the system manually, the disconnect is detected immediately (does not matter

It's worth pointing out that EHCI is different in this respect from OHCI and UHCI; the older controllers do not have the capability to enable or disable wakeup independently for connect, disconnect, and overcurrent events. They are all or nothing. So are external USB hubs.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

If we don't need that, perhaps the following patch should be applied.

Disable wake on overcurrent and disconnect in EHCI.
This fixes immediate resume from standby on boards where port power is lost in standby which triggers overcurrent detection and disconnect if a bus-powered device is connected. At least Asus P4P800 boards are affected when any of the USBPWxx (e.g. USBPW12) jumpers is set to 5V.

Quoted 6 lines

Quoted 2 lines

Why would you want to change the jumper settings? Host controllers are _supposed_ to supply 5V power during system suspend.

Maybe because I don't want all my USB devices to be powered when the system is turned off. I doubt that laptop in suspend-to-RAM (on battery) provides power to USB ports.

This depends on how your system was turned off. During suspend or hibernation, you _should_ want USB devices to be powered (and some people do, as Greg pointed out). During a normal system shutdown, the USB buses should not be powered.

Regardless, I don't think a kernel patch is the way to solve your problem. Changing the wakeup setting for the root hub will do what you want, and that setting is explicitly intended to be controlled by userspace (after all, that's why it is exposed in sysfs). The initial value is only a reasonable default; you can certainly add scripts or udev rules to disable wakeup on your EHCI root hub.

Quoted 13 lines · On Wednesday 28 April 2010 17:41:30 Alan Stern wrote:

Quoted 12 lines · Ondrej Zary

Quoted 10 lines · On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:

Quoted 3 lines · Ondrej Zary

The previous patch was not enough as it worked only when there were no USB devices connected. With a bus-powered device connected, power loss causes disconnect which wakes up the machine immediately again.

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Quoted 6 lines

Quoted 3 lines

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Okay, evidently the hardware or firmware on these boards is buggy. Other systems do not have the same problem, as far as I know.

It works fine in Windows.

Quoted 6 lines

Regardless, I don't think a kernel patch is the way to solve your problem. Changing the wakeup setting for the root hub will do what you want, and that setting is explicitly intended to be controlled by userspace (after all, that's why it is exposed in sysfs). The initial value is only a reasonable default; you can certainly add scripts or udev rules to disable wakeup on your EHCI root hub.

Yes, I can work around that. But many users can't. This is an attempt to make it "just work".

I'm trying to say that the "wakeup on everything" is not a good thing (if not a bug). Who needs it?

I can't imagine any real use for it. It clearly breaks suspend on some systems and is annoying on other.

Who expects that
disconnecting a device should wake up sleeping machine?

When all these three wakeups (overcurrent, connect, disconnect) are disabled, we lose nothing. Connect/disconnect detection works fine after wakeup. Wakeup by USB devices (not by connect/disconnect but by the device itself signaling a resume) is completely independent of this (according to EHCI specification).

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Quoted 2 lines

Quoted 2 lines

Okay, evidently the hardware or firmware on these boards is buggy. Other systems do not have the same problem, as far as I know.

I take this back. The same thing happens on my machine (Intel ICH5 chipset). I'd guess there is a bug in the PCI or ACPI subsystem, but more testing is needed before I can be sure. Somehow the PCI or platform wakeup mechanism gets activated even when it is supposed to be disabled.

Quoted 11 lines · Alan Stern

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is disabled by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to the PCI bus and it should not cause the system to wake up.

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Okay, evidently the hardware or firmware on these boards is buggy. Other systems do not have the same problem, as far as I know.

I take this back. The same thing happens on my machine (Intel ICH5 chipset). I'd guess there is a bug in the PCI or ACPI subsystem, but more testing is needed before I can be sure. Somehow the PCI or platform wakeup mechanism gets activated even when it is supposed to be disabled.

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +42 -26

@@ -106,6 +106,47 @@ static void ehci_handover_companion_port
 	ehci->owned_ports = 0;
 }
 
+static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	/* enable remote wakeup on all ports, if allowed */
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
+
+		/* only enable appropriate wake bits, otherwise the
+		 * hardware can not go phy low power mode. If a race
+		 * condition happens here(connection change during bits
+		 * set), the port change detection will finally fix it.
+		 */
+		if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
+			if (t1 & PORT_CONNECT)
+				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+			else
+				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+			ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
+					port + 1, t1, t2);
+		}
+		ehci_writel(ehci, t2, reg);
+	}
+}
+
+static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+
+		ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
+	}
+}
+
 static int ehci_bus_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);

@@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
 			t2 |= PORT_SUSPEND;
 			set_bit(port, &ehci->bus_suspended);
-		}
-
-		/* enable remote wakeup on all ports */
-		if (hcd->self.root_hub->do_remote_wakeup) {
-			/* only enable appropriate wake bits, otherwise the
-			 * hardware can not go phy low power mode. If a race
-			 * condition happens here(connection change during bits
-			 * set), the port change detection will finally fix it.
-			 */
-			if (t1 & PORT_CONNECT) {
-				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
-				t2 &= ~PORT_WKCONN_E;
-			} else {
-				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
-				t2 &= ~PORT_WKDISC_E;
-			}
-		} else
-			t2 &= ~PORT_WAKE_BITS;
-
-		if (t1 != t2) {
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
 				port + 1, t1, t2);
 			ehci_writel(ehci, t2, reg);

@@ -907,12 +928,7 @@ static int ehci_hub_control (
 
 					|| (temp & PORT_RESET) != 0)
 
 				goto error;
 
-			/* After above check the port must be connected.
-			 * Set appropriate bit thus could put phy into low power
-			 * mode if we have hostpc feature
-			 */
-			temp &= ~PORT_WKCONN_E;
-			temp |= PORT_WKDISC_E | PORT_WKOC_E;
+			temp &= ~PORT_WAKE_BITS;
 			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);

drivers/usb/host/ehci-hub.c +0 -0

 			if (hostpc_reg) {
 				spin_unlock_irqrestore(&ehci->lock, flags);

Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================

drivers/usb/host/ehci-pci.c +4 -11

@@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave (&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- bail:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
 				!hibernated) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
===================================================================

drivers/usb/host/ehci-au1xxx.c +4 -12

@@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave(&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
 	au1xxx_stop_ehc();
-
-bail:
 	spin_unlock_irqrestore(&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-fsl.c
===================================================================

drivers/usb/host/ehci-fsl.c +2 -0

@@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
 	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_set_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

@@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_clear_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;

-----Original Message-----
From: Alan Stern [mailto:[email protected]]
Sent: Friday, April 30, 2010 1:46 AM
To: Ondrej Zary; Du, Alek
Cc: Linux-pm mailing list; USB list; Kernel development list
Subject: Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

Quoted 2 lines · Alan Stern

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is

disabled

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to

the

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

PCI bus and it should not cause the system to wake up.

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Okay, evidently the hardware or firmware on these boards is buggy. Other systems do not have the same problem, as far as I know.

I take this back. The same thing happens on my machine (Intel ICH5 chipset). I'd guess there is a bug in the PCI or ACPI subsystem, but more testing is needed before I can be sure. Somehow the PCI or platform wakeup mechanism gets activated even when it is supposed to be disabled.

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +42 -26

@@ -106,6 +106,47 @@ static void ehci_handover_companion_port
 	ehci->owned_ports = 0;
 }
 
+static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	/* enable remote wakeup on all ports, if allowed */
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
+
+		/* only enable appropriate wake bits, otherwise the
+		 * hardware can not go phy low power mode. If a race
+		 * condition happens here(connection change during bits
+		 * set), the port change detection will finally fix it.
+		 */
+		if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
+			if (t1 & PORT_CONNECT)
+				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+			else
+				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+			ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
+					port + 1, t1, t2);
+		}
+		ehci_writel(ehci, t2, reg);
+	}
+}
+
+static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+
+		ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
+	}
+}
+
 static int ehci_bus_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);

@@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
 			t2 |= PORT_SUSPEND;
 			set_bit(port, &ehci->bus_suspended);
-		}
-
-		/* enable remote wakeup on all ports */
-		if (hcd->self.root_hub->do_remote_wakeup) {
-			/* only enable appropriate wake bits, otherwise the
-			 * hardware can not go phy low power mode. If a race
-			 * condition happens here(connection change during bits
-			 * set), the port change detection will finally fix it.
-			 */
-			if (t1 & PORT_CONNECT) {
-				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
-				t2 &= ~PORT_WKCONN_E;
-			} else {
-				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
-				t2 &= ~PORT_WKDISC_E;
-			}
-		} else
-			t2 &= ~PORT_WAKE_BITS;
-
-		if (t1 != t2) {
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
 				port + 1, t1, t2);
 			ehci_writel(ehci, t2, reg);

@@ -907,12 +928,7 @@ static int ehci_hub_control (
 					|| (temp & PORT_RESET) != 0)
 				goto error;
 
-			/* After above check the port must be connected.
-			 * Set appropriate bit thus could put phy into low power
-			 * mode if we have hostpc feature
-			 */
-			temp &= ~PORT_WKCONN_E;
-			temp |= PORT_WKDISC_E | PORT_WKOC_E;
+			temp &= ~PORT_WAKE_BITS;
 			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
 			if (hostpc_reg) {
 				spin_unlock_irqrestore(&ehci->lock, flags);

Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================

drivers/usb/host/ehci-pci.c +4 -11

@@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave (&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- bail:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
 				!hibernated) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
===================================================================

drivers/usb/host/ehci-au1xxx.c +4 -12

@@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave(&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
 	au1xxx_stop_ehc();
-
-bail:
 	spin_unlock_irqrestore(&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-fsl.c
===================================================================

drivers/usb/host/ehci-fsl.c +2 -0

@@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
 	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_set_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

@@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_clear_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

-----Original Message-----
From: Alan Stern [mailto:[email protected]]
Sent: Friday, April 30, 2010 1:46 AM
To: Ondrej Zary; Du, Alek
Cc: Linux-pm mailing list; USB list; Kernel development list
Subject: Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

Quoted 2 lines · Alan Stern

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

You said earlier that the host controller was disabled for remote wakeup ("/sys/devices/pci0000:00/0000:00:1d.7/power/wakeup is

disabled

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

by default"). So even though the root hub might issue a wakeup request, the controller hardware should not forward that request to

the

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

PCI bus and it should not cause the system to wake up.

Maybe it should not - but it wakes up this board and probably also P4P800, P4P800-E and P4C800-E at least:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/75497

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Okay, evidently the hardware or firmware on these boards is buggy. Other systems do not have the same problem, as far as I know.

I take this back. The same thing happens on my machine (Intel ICH5 chipset). I'd guess there is a bug in the PCI or ACPI subsystem, but more testing is needed before I can be sure. Somehow the PCI or platform wakeup mechanism gets activated even when it is supposed to be disabled.

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +42 -26

@@ -106,6 +106,47 @@ static void ehci_handover_companion_port
 	ehci->owned_ports = 0;
 }
 
+static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	/* enable remote wakeup on all ports, if allowed */
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
+
+		/* only enable appropriate wake bits, otherwise the
+		 * hardware can not go phy low power mode. If a race
+		 * condition happens here(connection change during bits
+		 * set), the port change detection will finally fix it.
+		 */
+		if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
+			if (t1 & PORT_CONNECT)
+				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+			else
+				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+			ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
+					port + 1, t1, t2);
+		}
+		ehci_writel(ehci, t2, reg);
+	}
+}
+
+static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+
+		ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
+	}
+}
+
 static int ehci_bus_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);

@@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
 			t2 |= PORT_SUSPEND;
 			set_bit(port, &ehci->bus_suspended);
-		}
-
-		/* enable remote wakeup on all ports */
-		if (hcd->self.root_hub->do_remote_wakeup) {
-			/* only enable appropriate wake bits, otherwise the
-			 * hardware can not go phy low power mode. If a race
-			 * condition happens here(connection change during bits
-			 * set), the port change detection will finally fix it.
-			 */
-			if (t1 & PORT_CONNECT) {
-				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
-				t2 &= ~PORT_WKCONN_E;
-			} else {
-				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
-				t2 &= ~PORT_WKDISC_E;
-			}
-		} else
-			t2 &= ~PORT_WAKE_BITS;
-
-		if (t1 != t2) {
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
 				port + 1, t1, t2);
 			ehci_writel(ehci, t2, reg);

@@ -907,12 +928,7 @@ static int ehci_hub_control (
 					|| (temp & PORT_RESET) != 0)
 				goto error;
 
-			/* After above check the port must be connected.
-			 * Set appropriate bit thus could put phy into low power
-			 * mode if we have hostpc feature
-			 */
-			temp &= ~PORT_WKCONN_E;
-			temp |= PORT_WKDISC_E | PORT_WKOC_E;
+			temp &= ~PORT_WAKE_BITS;
 			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
 			if (hostpc_reg) {
 				spin_unlock_irqrestore(&ehci->lock, flags);

Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================

drivers/usb/host/ehci-pci.c +4 -11

@@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave (&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- bail:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
 				!hibernated) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
===================================================================

drivers/usb/host/ehci-au1xxx.c +4 -12

@@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave(&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
 	au1xxx_stop_ehc();
-
-bail:
 	spin_unlock_irqrestore(&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-fsl.c
===================================================================

drivers/usb/host/ehci-fsl.c +2 -0

@@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
 	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_set_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

@@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_clear_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +42 -26

@@ -106,6 +106,47 @@ static void ehci_handover_companion_port
 	ehci->owned_ports = 0;
 }
 
+static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	/* enable remote wakeup on all ports, if allowed */
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
+
+		/* only enable appropriate wake bits, otherwise the
+		 * hardware can not go phy low power mode. If a race
+		 * condition happens here(connection change during bits
+		 * set), the port change detection will finally fix it.
+		 */
+		if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
+			if (t1 & PORT_CONNECT)
+				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+			else
+				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+			ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
+					port + 1, t1, t2);
+		}
+		ehci_writel(ehci, t2, reg);
+	}
+}
+
+static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+
+		ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
+	}
+}
+
 static int ehci_bus_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);

@@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
 			t2 |= PORT_SUSPEND;
 			set_bit(port, &ehci->bus_suspended);
-		}
-
-		/* enable remote wakeup on all ports */
-		if (hcd->self.root_hub->do_remote_wakeup) {
-			/* only enable appropriate wake bits, otherwise the
-			 * hardware can not go phy low power mode. If a race
-			 * condition happens here(connection change during bits
-			 * set), the port change detection will finally fix it.
-			 */
-			if (t1 & PORT_CONNECT) {
-				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
-				t2 &= ~PORT_WKCONN_E;
-			} else {
-				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
-				t2 &= ~PORT_WKDISC_E;
-			}
-		} else
-			t2 &= ~PORT_WAKE_BITS;
-
-		if (t1 != t2) {
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
 				port + 1, t1, t2);
 			ehci_writel(ehci, t2, reg);

@@ -907,12 +928,7 @@ static int ehci_hub_control (
 					|| (temp & PORT_RESET) != 0)
 				goto error;
 
-			/* After above check the port must be connected.
-			 * Set appropriate bit thus could put phy into low power
-			 * mode if we have hostpc feature
-			 */
-			temp &= ~PORT_WKCONN_E;
-			temp |= PORT_WKDISC_E | PORT_WKOC_E;
+			temp &= ~PORT_WAKE_BITS;
 			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
 			if (hostpc_reg) {
 				spin_unlock_irqrestore(&ehci->lock, flags);

Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================

drivers/usb/host/ehci-pci.c +4 -11

@@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave (&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- bail:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
 				!hibernated) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
===================================================================

drivers/usb/host/ehci-au1xxx.c +4 -12

@@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave(&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
 	au1xxx_stop_ehc();
-
-bail:
 	spin_unlock_irqrestore(&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-fsl.c
===================================================================

drivers/usb/host/ehci-fsl.c +2 -0

@@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
 	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_set_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

@@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_clear_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +42 -26

@@ -106,6 +106,47 @@ static void ehci_handover_companion_port
 	ehci->owned_ports = 0;
 }
 
+static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	/* enable remote wakeup on all ports, if allowed */
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
+
+		/* only enable appropriate wake bits, otherwise the
+		 * hardware can not go phy low power mode. If a race
+		 * condition happens here(connection change during bits
+		 * set), the port change detection will finally fix it.
+		 */
+		if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
+			if (t1 & PORT_CONNECT)
+				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+			else
+				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+			ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
+					port + 1, t1, t2);
+		}
+		ehci_writel(ehci, t2, reg);
+	}
+}
+
+static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
+{
+	int	port;
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+
+		ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
+	}
+}
+
 static int ehci_bus_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);

@@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
 			t2 |= PORT_SUSPEND;
 			set_bit(port, &ehci->bus_suspended);
-		}
-
-		/* enable remote wakeup on all ports */
-		if (hcd->self.root_hub->do_remote_wakeup) {
-			/* only enable appropriate wake bits, otherwise the
-			 * hardware can not go phy low power mode. If a race
-			 * condition happens here(connection change during bits
-			 * set), the port change detection will finally fix it.
-			 */
-			if (t1 & PORT_CONNECT) {
-				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
-				t2 &= ~PORT_WKCONN_E;
-			} else {
-				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
-				t2 &= ~PORT_WKDISC_E;
-			}
-		} else
-			t2 &= ~PORT_WAKE_BITS;
-
-		if (t1 != t2) {
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
 				port + 1, t1, t2);
 			ehci_writel(ehci, t2, reg);

@@ -907,12 +928,7 @@ static int ehci_hub_control (
 					|| (temp & PORT_RESET) != 0)
 				goto error;
 
-			/* After above check the port must be connected.
-			 * Set appropriate bit thus could put phy into low power
-			 * mode if we have hostpc feature
-			 */
-			temp &= ~PORT_WKCONN_E;
-			temp |= PORT_WKDISC_E | PORT_WKOC_E;
+			temp &= ~PORT_WAKE_BITS;
 			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
 			if (hostpc_reg) {
 				spin_unlock_irqrestore(&ehci->lock, flags);

Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================

drivers/usb/host/ehci-pci.c +4 -11

@@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave (&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- bail:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
 				!hibernated) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
===================================================================

drivers/usb/host/ehci-au1xxx.c +4 -12

@@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave(&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_set_wakeup_flags(ehci);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
 	au1xxx_stop_ehc();
-
-bail:
 	spin_unlock_irqrestore(&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
 		int	mask = INTR_MASK;
 
+		ehci_clear_wakeup_flags(ehci);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-fsl.c
===================================================================

drivers/usb/host/ehci-fsl.c +2 -0

@@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
 	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_set_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

@@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_clear_wakeup_flags(ehci);
 	if (!fsl_deep_sleep())
 		return 0;
 

Quoted 9 lines · On Fri, 30 Apr 2010 01:45:33 +0800 Alan Stern <[email protected]> wrote:

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Alan Stern

Alan,

As I tested, the patch breaks phy low power mode, the EHCI works, but it never enter phy low power mode when suspending port...

I guess this part of the diff breaks it.

- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }

Quoted 9 lines · On Fri, 30 Apr 2010 01:45:33 +0800 Alan Stern <[email protected]> wrote:

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Alan Stern

Alan,

As I tested, the patch breaks phy low power mode, the EHCI works, but it never enter phy low power mode when suspending port...

I guess this part of the diff breaks it.

- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }

-----Original Message-----
From: Alan Stern [mailto:[email protected]]
Sent: Wednesday, May 05, 2010 11:56 PM
To: Du, Alek
Cc: Ondrej Zary; Linux-pm mailing list; USB list; Kernel development list
Subject: Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

Quoted 12 lines · Alek

Quoted 8 lines · On Fri, 30 Apr 2010 01:45:33 +0800 Alan Stern <[email protected]> wrote:

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Quoted 6 lines

Quoted 1 line

Alan Stern

Alan,

As I tested, the patch breaks phy low power mode, the EHCI works, but it

never

Quoted 5 lines

enter phy low power mode when suspending port...

I guess this part of the diff breaks it.

Quoted 12 lines

- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }

That seems very odd. Why should removing code that enables wakeup bits cause the suspend to fail? Can't the phy go into low-power mode when all the wakeup bits are disabled?

Does applying this additional patch on top of the previous one help?

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +1 -1

@@ -200,7 +200,7 @@ static int ehci_bus_suspend (struct usb_
 	while (port--) {
 		u32 __iomem	*reg = &ehci->regs->port_status [port];
 		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
-		u32		t2 = t1;
+		u32		t2;
 
 		if (ehci->has_hostpc)
 			hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs

-----Original Message-----
From: Alan Stern [mailto:[email protected]]
Sent: Wednesday, May 05, 2010 11:56 PM
To: Du, Alek
Cc: Ondrej Zary; Linux-pm mailing list; USB list; Kernel development list
Subject: Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

Quoted 12 lines · Alek

Quoted 8 lines · On Fri, 30 Apr 2010 01:45:33 +0800 Alan Stern <[email protected]> wrote:

This patch fixes the problem on my system. Does it work for you? I still think that disabling wakeup at the PCI or platform level should override the port wakeup flags, but apparently it doesn't.

Alek, can you confirm that the changes in this patch are okay for the Moorestown EHCI controller? I had to guess at how to handle the HOSTPCx register settings.

Quoted 6 lines

Quoted 1 line

Alan Stern

Alan,

As I tested, the patch breaks phy low power mode, the EHCI works, but it

never

Quoted 5 lines

enter phy low power mode when suspending port...

I guess this part of the diff breaks it.

Quoted 12 lines

- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }

That seems very odd. Why should removing code that enables wakeup bits cause the suspend to fail? Can't the phy go into low-power mode when all the wakeup bits are disabled?

Does applying this additional patch on top of the previous one help?

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +1 -1

@@ -200,7 +200,7 @@ static int ehci_bus_suspend (struct usb_
 	while (port--) {
 		u32 __iomem	*reg = &ehci->regs->port_status [port];
 		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
-		u32		t2 = t1;
+		u32		t2;
 
 		if (ehci->has_hostpc)
 			hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs

Quoted 3 lines

Alan,

As I tested, the patch breaks phy low power mode, the EHCI works, but it

never

Quoted 5 lines

enter phy low power mode when suspending port...

I guess this part of the diff breaks it.

Quoted 12 lines

- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }

That seems very odd. Why should removing code that enables wakeup bits cause the suspend to fail? Can't the phy go into low-power mode when all the wakeup bits are disabled?

Does applying this additional patch on top of the previous one help?

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +1 -1

@@ -200,7 +200,7 @@ static int ehci_bus_suspend (struct usb_
 	while (port--) {
 		u32 __iomem	*reg = &ehci->regs->port_status [port];
 		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
-		u32		t2 = t1;
+		u32		t2;
 
 		if (ehci->has_hostpc)
 			hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs

Quoted 3 lines

Alan,

As I tested, the patch breaks phy low power mode, the EHCI works, but it

never

Quoted 5 lines

enter phy low power mode when suspending port...

I guess this part of the diff breaks it.

Quoted 12 lines

- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }

That seems very odd. Why should removing code that enables wakeup bits cause the suspend to fail? Can't the phy go into low-power mode when all the wakeup bits are disabled?

Does applying this additional patch on top of the previous one help?

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +1 -1

@@ -200,7 +200,7 @@ static int ehci_bus_suspend (struct usb_
 	while (port--) {
 		u32 __iomem	*reg = &ehci->regs->port_status [port];
 		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
-		u32		t2 = t1;
+		u32		t2;
 
 		if (ehci->has_hostpc)
 			hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs

Alan,

This would be better, but before entering phy low power mode, the HW need the exact wakeup bits to be set, that's why we have:

/* only enable appropriate wake bits, otherwise the
* hardware can not go phy low power mode. If a race
* condition happens here(connection change during bits
* set), the port change detection will finally fix it. */
In the code. When port is connected, we must only enable PORT_WKOC_E | PORT_WKDISC_E, and when port is disconnected, we must only enable PORT_WKOC_E | PORT_WKCONN_E. Enable all wakeup bits won't work.

I think the problem is: For the original code, once t2 != t1, the HCD will try to put into phy low power mode. While after the patch, the HCD will only enter phy low power mode if PORT_PE is set and PORT_SUSPEND is not set.

Alan,

This would be better, but before entering phy low power mode, the HW need the exact wakeup bits to be set, that's why we have:

/* only enable appropriate wake bits, otherwise the
* hardware can not go phy low power mode. If a race
* condition happens here(connection change during bits
* set), the port change detection will finally fix it. */
In the code. When port is connected, we must only enable PORT_WKOC_E | PORT_WKDISC_E, and when port is disconnected, we must only enable PORT_WKOC_E | PORT_WKCONN_E. Enable all wakeup bits won't work.

I think the problem is: For the original code, once t2 != t1, the HCD will try to put into phy low power mode. While after the patch, the HCD will only enter phy low power mode if PORT_PE is set and PORT_SUSPEND is not set.

With this patch, _none_ of the wakeup bits are enabled. That should work, right?

But it leads to another question: Later on, when the controller is put is put into low-power mode, I wanted to check with you about it.

Quoted 4 lines

I think the problem is: For the original code, once t2 != t1, the HCD will try to put into phy low power mode. While after the patch, the HCD will only enter phy low power mode if PORT_PE is set and PORT_SUSPEND is not set.

Okay, I understand. Then consider _this_ patch on top of the first one. It sets the hostpc register for every port that wasn't already suspended, so each phy should end up in low-power mode.

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +16 -15

@@ -152,7 +152,6 @@ static int ehci_bus_suspend (struct usb_
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
 	int			port;
 	int			mask;
-	u32 __iomem		*hostpc_reg = NULL;
 
 	ehci_dbg(ehci, "suspend root hub\n");
 

@@ -200,23 +199,25 @@ static int ehci_bus_suspend (struct usb_
 	while (port--) {
 		u32 __iomem	*reg = &ehci->regs->port_status [port];
 		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
-		u32		t2 = t1;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
 
-		if (ehci->has_hostpc)
-			hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs
-				+ HOSTPC0 + 4 * (port & 0xff));
 		/* keep track of which ports we suspend */
-		if (t1 & PORT_OWNER)
-			set_bit(port, &ehci->owned_ports);
-		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
-			t2 |= PORT_SUSPEND;
-			set_bit(port, &ehci->bus_suspended);
-			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
-				port + 1, t1, t2);
-			ehci_writel(ehci, t2, reg);
-			if (hostpc_reg) {
-				u32	t3;
+		if (!(t1 & PORT_SUSPEND)) {
+			if (t1 & PORT_OWNER) {
+				set_bit(port, &ehci->owned_ports);
+			} else if (t1 & PORT_PE) {
+				t2 |= PORT_SUSPEND;
+				set_bit(port, &ehci->bus_suspended);
+				ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
+					port + 1, t1, t2);
+				ehci_writel(ehci, t2, reg);
+			}
+			if (ehci->has_hostpc) {
+				u32 __iomem	*hostpc_reg;
+				u32		t3;
 
+				hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs
+					+ HOSTPC0 + 4 * (port & 0xff));
 				spin_unlock_irq(&ehci->lock);
 				msleep(5);/* 5ms for HCD enter low pwr mode */
 				spin_lock_irq(&ehci->lock);

With this patch, _none_ of the wakeup bits are enabled. That should work, right?

But it leads to another question: Later on, when the controller is put is put into low-power mode, I wanted to check with you about it.

Quoted 4 lines

I think the problem is: For the original code, once t2 != t1, the HCD will try to put into phy low power mode. While after the patch, the HCD will only enter phy low power mode if PORT_PE is set and PORT_SUSPEND is not set.

Okay, I understand. Then consider _this_ patch on top of the first one. It sets the hostpc register for every port that wasn't already suspended, so each phy should end up in low-power mode.

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +16 -15

@@ -152,7 +152,6 @@ static int ehci_bus_suspend (struct usb_
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
 	int			port;
 	int			mask;
-	u32 __iomem		*hostpc_reg = NULL;
 
 	ehci_dbg(ehci, "suspend root hub\n");
 

@@ -200,23 +199,25 @@ static int ehci_bus_suspend (struct usb_
 	while (port--) {
 		u32 __iomem	*reg = &ehci->regs->port_status [port];
 		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
-		u32		t2 = t1;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
 
-		if (ehci->has_hostpc)
-			hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs
-				+ HOSTPC0 + 4 * (port & 0xff));
 		/* keep track of which ports we suspend */
-		if (t1 & PORT_OWNER)
-			set_bit(port, &ehci->owned_ports);
-		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
-			t2 |= PORT_SUSPEND;
-			set_bit(port, &ehci->bus_suspended);
-			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
-				port + 1, t1, t2);
-			ehci_writel(ehci, t2, reg);
-			if (hostpc_reg) {
-				u32	t3;
+		if (!(t1 & PORT_SUSPEND)) {
+			if (t1 & PORT_OWNER) {
+				set_bit(port, &ehci->owned_ports);
+			} else if (t1 & PORT_PE) {
+				t2 |= PORT_SUSPEND;
+				set_bit(port, &ehci->bus_suspended);
+				ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
+					port + 1, t1, t2);
+				ehci_writel(ehci, t2, reg);
+			}
+			if (ehci->has_hostpc) {
+				u32 __iomem	*hostpc_reg;
+				u32		t3;
 
+				hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs
+					+ HOSTPC0 + 4 * (port & 0xff));
 				spin_unlock_irq(&ehci->lock);
 				msleep(5);/* 5ms for HCD enter low pwr mode */
 				spin_lock_irq(&ehci->lock);

Hi Alan,

Quoted 3 lines

With this patch, _none_ of the wakeup bits are enabled. That should work, right?

I guess so, but if no wakeup bits set, how to handle remote wakeup case? Seems you removed remote wakeup case?

Hi Alan,

Quoted 3 lines

With this patch, _none_ of the wakeup bits are enabled. That should work, right?

I guess so, but if no wakeup bits set, how to handle remote wakeup case? Seems you removed remote wakeup case?

Quoted 6 lines · Alek

Hi Alan,

Quoted 3 lines

With this patch, _none_ of the wakeup bits are enabled. That should work, right?

I guess so, but if no wakeup bits set, how to handle remote wakeup case? Seems you removed remote wakeup case?

The wakeup bits get set later, in ehci_set_wakeup_flags().

The point is that the wakeup bits take effect only when the controller leaves D0. But ehci_bus_suspend() is called when the root hub is suspended, which happens first. So at that time the wakeup bits aren't needed.

Alan Stern

Quoted 6 lines · Alek

Hi Alan,

Quoted 3 lines

With this patch, _none_ of the wakeup bits are enabled. That should work, right?

I guess so, but if no wakeup bits set, how to handle remote wakeup case? Seems you removed remote wakeup case?

The wakeup bits get set later, in ehci_set_wakeup_flags().

The point is that the wakeup bits take effect only when the controller leaves D0. But ehci_bus_suspend() is called when the root hub is suspended, which happens first. So at that time the wakeup bits aren't needed.

Alan Stern

Alan,

As I tested, with the second patch, after entering phy low power mode, the port cannot detect any USB devices that plugged in.

I also confirm that after set PHCD flag, any write to PORTSC is illegal. (I have another patch will send out soon named "Clear PHCD before resuming", but the patch is not related with this one)

So it seems wakeup bits must set before set PHCD. So we may revise the flowchart a little bit, for example, if the HCD has hostpc, then set the wakeup bits like before, if the HCD is the normal one, then go your new way?

Alan,

As I tested, with the second patch, after entering phy low power mode, the port cannot detect any USB devices that plugged in.

I also confirm that after set PHCD flag, any write to PORTSC is illegal. (I have another patch will send out soon named "Clear PHCD before resuming", but the patch is not related with this one)

So it seems wakeup bits must set before set PHCD. So we may revise the flowchart a little bit, for example, if the HCD has hostpc, then set the wakeup bits like before, if the HCD is the normal one, then go your new way?

Quoted 4 lines

Alan,

As I tested, with the second patch, after entering phy low power mode, the

port cannot detect any USB devices that plugged in.

Ooh, that's not good. It almost violates the EHCI spec (see the last line on p.60 in table 4.4).

Quoted 2 lines

I also confirm that after set PHCD flag, any write to PORTSC is illegal. (I have

another patch will send out soon named "Clear PHCD before resuming",

Quoted 1 line

but the patch is not related with this one)

I'm afraid we are going to have to modify the wakeup bits in PORTSC after PHCD is set. Can we clear PHCD, modify PORTSC, and then set PHCD again?

Quoted 2 lines

So it seems wakeup bits must set before set PHCD. So we may revise the

flowchart a little bit, for example, if the HCD has hostpc,

Quoted 1 line

then set the wakeup bits like before, if the HCD is the normal one, then go

your new way?

For normal HCDs, the wakeup bits don't matter when the controller is in D0. Therefore we can continue to set them as before during bus or port suspend.

But during controller suspend, if the controller isn't supposed to be remote-wakeup-enabled then we will need to clear the wakeup bits (and set them again during controller resume). Will that work?

Alan Stern

Quoted 4 lines

Alan,

As I tested, with the second patch, after entering phy low power mode, the

port cannot detect any USB devices that plugged in.

Ooh, that's not good. It almost violates the EHCI spec (see the last line on p.60 in table 4.4).

Quoted 2 lines

I also confirm that after set PHCD flag, any write to PORTSC is illegal. (I have

another patch will send out soon named "Clear PHCD before resuming",

Quoted 1 line

but the patch is not related with this one)

I'm afraid we are going to have to modify the wakeup bits in PORTSC after PHCD is set. Can we clear PHCD, modify PORTSC, and then set PHCD again?

Quoted 2 lines

So it seems wakeup bits must set before set PHCD. So we may revise the

flowchart a little bit, for example, if the HCD has hostpc,

Quoted 1 line

then set the wakeup bits like before, if the HCD is the normal one, then go

your new way?

For normal HCDs, the wakeup bits don't matter when the controller is in D0. Therefore we can continue to set them as before during bus or port suspend.

But during controller suspend, if the controller isn't supposed to be remote-wakeup-enabled then we will need to clear the wakeup bits (and set them again during controller resume). Will that work?

Alan Stern

Quoted 9 lines · Alek

Quoted 4 lines

Alan,

As I tested, with the second patch, after entering phy low power mode, the

port cannot detect any USB devices that plugged in.

Ooh, that's not good. It almost violates the EHCI spec (see the last line on p.60 in table 4.4).

Quoted 9 lines · Alek

Quoted 4 lines

Alan,

As I tested, with the second patch, after entering phy low power mode, the

port cannot detect any USB devices that plugged in.

Ooh, that's not good. It almost violates the EHCI spec (see the last line on p.60 in table 4.4).

Alan,

The following patch (based on your previous two patches) works for me, could you help to review and test on your machine? Basically, this will only affect those "has_hostpc" HCDs.

Alan,

The following patch (based on your previous two patches) works for me, could you help to review and test on your machine? Basically, this will only affect those "has_hostpc" HCDs.

Quoted 5 lines · Alek

Alan,

The following patch (based on your previous two patches) works for me, could you help to review and test on your machine? Basically, this will only affect those "has_hostpc" HCDs.

Based on your suggestion, I completely redid both of my patches. They are now combined into a single patch, which is meant to go on top of the patch you submitted this morning. It adds the stuff we talked about, and it cleans up some of the changes you made.

Does it look good to you?

Alan Stern

Index: usb-2.6/drivers/usb/host/ehci-hub.c
===================================================================

drivers/usb/host/ehci-hub.c +123 -49

@@ -106,12 +106,72 @@ static void ehci_handover_companion_port
 	ehci->owned_ports = 0;
 }
 
+static void ehci_adjust_port_wakeup_flags(struct ehci_hcd *ehci, bool resuming)
+{
+	int		port;
+	u32		temp;
+
+	/* If remote wakeup is enabled for the root hub but disabled
+	 * for the controller, we must adjust all the port wakeup flags.
+	 */
+	if (!ehci_to_hcd(ehci)->self.root_hub->do_remote_wakeup ||
+			device_may_wakeup(ehci_to_hcd(ehci)->self.controller))
+		return;
+
+	/* clear phy low-power mode before changing wakeup flags */
+	if (ehci->has_hostpc) {
+		port = HCS_N_PORTS(ehci->hcs_params);
+		while (port--) {
+			u32 __iomem	*hostpc_reg;
+
+			hostpc_reg = (u32 __iomem *)((u8 *) ehci->regs
+					+ HOSTPC0 + 4 * port);
+			temp = ehci_readl(ehci, hostpc_reg);
+			ehci_writel(ehci, temp & ~HOSTPC_PHCD, hostpc_reg);
+		}
+		msleep(5);
+	}
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem	*reg = &ehci->regs->port_status[port];
+		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
+
+		/* If we are suspending the controller, clear the flags.
+		 * If we are resuming the controller, set the wakeup flags.
+		 */
+		if (resuming) {
+			if (t1 & PORT_CONNECT)
+				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
+			else
+				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
+		}
+		ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
+				port + 1, t1, t2);
+		ehci_writel(ehci, t2, reg);
+	}
+
+	/* enter phy low-power mode again */
+	if (ehci->has_hostpc) {
+		port = HCS_N_PORTS(ehci->hcs_params);
+		while (port--) {
+			u32 __iomem	*hostpc_reg;
+
+			hostpc_reg = (u32 __iomem *)((u8 *) ehci->regs
+					+ HOSTPC0 + 4 * port);
+			temp = ehci_readl(ehci, hostpc_reg);
+			ehci_writel(ehci, temp | HOSTPC_PHCD, hostpc_reg);
+		}
+	}
+}
+
 static int ehci_bus_suspend (struct usb_hcd *hcd)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
 	int			port;
 	int			mask;
-	u32 __iomem		*hostpc_reg = NULL;
+	int			changed;
 
 	ehci_dbg(ehci, "suspend root hub\n");
 

@@ -155,15 +215,13 @@ static int ehci_bus_suspend (struct usb_
 	 */
 	ehci->bus_suspended = 0;
 	ehci->owned_ports = 0;
+	changed = 0;
 	port = HCS_N_PORTS(ehci->hcs_params);
 	while (port--) {
 		u32 __iomem	*reg = &ehci->regs->port_status [port];
 		u32		t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
-		u32		t2 = t1;
+		u32		t2 = t1 & ~PORT_WAKE_BITS;
 
-		if (ehci->has_hostpc)
-			hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs
-				+ HOSTPC0 + 4 * (port & 0xff));
 		/* keep track of which ports we suspend */
 		if (t1 & PORT_OWNER)
 			set_bit(port, &ehci->owned_ports);

@@ -172,40 +230,45 @@ static int ehci_bus_suspend (struct usb_
 			set_bit(port, &ehci->bus_suspended);
 		}
 
-		/* enable remote wakeup on all ports */
+		/* enable remote wakeup on all ports, if told to do so */
 		if (hcd->self.root_hub->do_remote_wakeup) {
 			/* only enable appropriate wake bits, otherwise the
 			 * hardware can not go phy low power mode. If a race
 			 * condition happens here(connection change during bits
 			 * set), the port change detection will finally fix it.
 			 */
-			if (t1 & PORT_CONNECT) {
+			if (t1 & PORT_CONNECT)
 				t2 |= PORT_WKOC_E | PORT_WKDISC_E;
-				t2 &= ~PORT_WKCONN_E;
-			} else {
+			else
 				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
-				t2 &= ~PORT_WKDISC_E;
-			}
-		} else
-			t2 &= ~PORT_WAKE_BITS;
+		}
 
 		if (t1 != t2) {
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
 				port + 1, t1, t2);
 			ehci_writel(ehci, t2, reg);
-			if (hostpc_reg) {
-				u32	t3;
+			changed = 1;
+		}
+	}
 
-				spin_unlock_irq(&ehci->lock);
-				msleep(5);/* 5ms for HCD enter low pwr mode */
-				spin_lock_irq(&ehci->lock);
-				t3 = ehci_readl(ehci, hostpc_reg);
-				ehci_writel(ehci, t3 | HOSTPC_PHCD, hostpc_reg);
-				t3 = ehci_readl(ehci, hostpc_reg);
-				ehci_dbg(ehci, "Port%d phy low pwr mode %s\n",
+	if (changed && ehci->has_hostpc) {
+		spin_unlock_irq(&ehci->lock);
+		msleep(5);	/* 5 ms for HCD to enter low-power mode */
+		spin_lock_irq(&ehci->lock);
+
+		port = HCS_N_PORTS(ehci->hcs_params);
+		while (port--) {
+			u32 __iomem	*hostpc_reg;
+			u32		t3;
+
+			hostpc_reg = (u32 __iomem *)((u8 *) ehci->regs
+					+ HOSTPC0 + 4 * port);
+			t3 = ehci_readl(ehci, hostpc_reg);
+			ehci_writel(ehci, t3 | HOSTPC_PHCD, hostpc_reg);
+			t3 = ehci_readl(ehci, hostpc_reg);
+			ehci_dbg(ehci, "Port %d phy low-power mode %s\n",
 					port, (t3 & HOSTPC_PHCD) ?
 					"succeeded" : "failed");
-			}
 		}
 	}
 

@@ -291,19 +354,28 @@ static int ehci_bus_resume (struct usb_h
 	msleep(8);
 	spin_lock_irq(&ehci->lock);
 
+	/* clear phy low-power mode before resume */
+	if (ehci->bus_suspended && ehci->has_hostpc) {
+		i = HCS_N_PORTS (ehci->hcs_params);
+		while (i--) {
+			if (test_bit(i, &ehci->bus_suspended)) {
+				u32 __iomem	*hostpc_reg;
+
+				hostpc_reg = (u32 __iomem *)((u8 *) ehci->regs
+						+ HOSTPC0 + 4 * i);
+				temp = ehci_readl(ehci, hostpc_reg);
+				ehci_writel(ehci, temp & ~HOSTPC_PHCD,
+						hostpc_reg);
+			}
+		}
+		spin_unlock_irq(&ehci->lock);
+		msleep(5);
+		spin_lock_irq(&ehci->lock);
+	}
+
 	/* manually resume the ports we suspended during bus_suspend() */
 	i = HCS_N_PORTS (ehci->hcs_params);
 	while (i--) {
-		/* clear phy low power mode before resume */
-		if (ehci->has_hostpc) {
-			u32 __iomem	*hostpc_reg =
-				(u32 __iomem *)((u8 *)ehci->regs
-				+ HOSTPC0 + 4 * (i & 0xff));
-			temp = ehci_readl(ehci, hostpc_reg);
-			ehci_writel(ehci, temp & ~HOSTPC_PHCD,
-				hostpc_reg);
-			mdelay(5);
-		}
 		temp = ehci_readl(ehci, &ehci->regs->port_status [i]);
 		temp &= ~(PORT_RWC_BITS | PORT_WAKE_BITS);
 		if (test_bit(i, &ehci->bus_suspended) &&

@@ -685,23 +757,25 @@ static int ehci_hub_control (
 				goto error;
 			if (ehci->no_selective_suspend)
 				break;
-			if (temp & PORT_SUSPEND) {
-				if ((temp & PORT_PE) == 0)
-					goto error;
-				/* clear phy low power mode before resume */
-				if (hostpc_reg) {
-					temp1 = ehci_readl(ehci, hostpc_reg);
-					ehci_writel(ehci, temp1 & ~HOSTPC_PHCD,
-						hostpc_reg);
-					mdelay(5);
-				}
-				/* resume signaling for 20 msec */
-				temp &= ~(PORT_RWC_BITS | PORT_WAKE_BITS);
-				ehci_writel(ehci, temp | PORT_RESUME,
-						status_reg);
-				ehci->reset_done [wIndex] = jiffies
-						+ msecs_to_jiffies (20);
+			if (!(temp & PORT_SUSPEND))
+				break;
+			if ((temp & PORT_PE) == 0)
+				goto error;
+
+			/* clear phy low-power mode before resume */
+			if (hostpc_reg) {
+				temp1 = ehci_readl(ehci, hostpc_reg);
+				ehci_writel(ehci, temp1 & ~HOSTPC_PHCD,
+					hostpc_reg);
+				spin_unlock_irqrestore(&ehci->lock, flags);
+				msleep(5);/* wait to leave low-power mode */
+				spin_lock_irqsave(&ehci->lock, flags);
 			}
+			/* resume signaling for 20 msec */
+			temp &= ~(PORT_RWC_BITS | PORT_WAKE_BITS);
+			ehci_writel(ehci, temp | PORT_RESUME, status_reg);
+			ehci->reset_done[wIndex] = jiffies
+					+ msecs_to_jiffies(20);
 			break;
 		case USB_PORT_FEAT_C_SUSPEND:
 			clear_bit(wIndex, &ehci->port_c_suspend);

Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================

drivers/usb/host/ehci-pci.c +4 -11

@@ -287,23 +287,15 @@ static int ehci_pci_suspend(struct usb_h
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave (&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_adjust_port_wakeup_flags(ehci, false);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- bail:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -333,6 +325,7 @@ static int ehci_pci_resume(struct usb_hc
 				!hibernated) {
 		int	mask = INTR_MASK;
 
+		ehci_adjust_port_wakeup_flags(ehci, true);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
===================================================================

drivers/usb/host/ehci-au1xxx.c +4 -12

@@ -224,26 +224,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
 		msleep(10);
 
 	/* Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 *
-	 * This is still racy as hcd->state is manipulated outside of
-	 * any locks =P But that will be a different fix.
+	 * mark HW unaccessible.  The PM and USB cores make sure that
+	 * the root hub is either suspended or stopped.
 	 */
 	spin_lock_irqsave(&ehci->lock, flags);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
-		goto bail;
-	}
+	ehci_adjust_port_wakeup_flags(ehci, false);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
 
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
 	au1xxx_stop_ehc();
-
-bail:
 	spin_unlock_irqrestore(&ehci->lock, flags);
 
 	// could save FLADJ in case of Vaux power loss

@@ -273,6 +264,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
 	if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
 		int	mask = INTR_MASK;
 
+		ehci_adjust_port_wakeup_flags(ehci, true);
 		if (!hcd->self.root_hub->do_remote_wakeup)
 			mask &= ~STS_PCD;
 		ehci_writel(ehci, mask, &ehci->regs->intr_enable);

Index: usb-2.6/drivers/usb/host/ehci-fsl.c
===================================================================

drivers/usb/host/ehci-fsl.c +2 -0

@@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
 	struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_adjust_port_wakeup_flags(ehci, false);
 	if (!fsl_deep_sleep())
 		return 0;
 

@@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 
+	ehci_adjust_port_wakeup_flags(ehci, true);
 	if (!fsl_deep_sleep())
 		return 0;