Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Wed, 28 Apr 2010, Ondrej Zary wrote:

> On Wednesday 28 April 2010 17:41:30 Alan Stern wrote:
> > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> > > > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > > > 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
> >
> > 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.

> It works fine in Windows.

How can you tell?  How do you know what values Windows writes into the 
EHCI port control registers?

> > 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".

It should "just work" already.  The fact that it doesn't means there is 
a bug.  At the moment I can't be sure where the bug is -- but even if 
it is in ehci-hcd, your suggested patch isn't the right fix.

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

I don't know, and neither do you.  But the USB spec requires this
behavior from external hubs, so evidently _somebody_ thought it was a
good idea.

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

If everything was working properly, the machine wouldn't wake up when a
disconnect occurred.

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

Perhaps the same people who expect that plugging in a device should
wake up a 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).

What about UHCI or OHCI?  What about external hubs?

In short, why should EHCI behave differently from everything else?

Alan Stern

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Thu, 29 Apr 2010, Alan Stern wrote:

> > > > > 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
> > >
> > > 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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
+++ usb-2.6/drivers/usb/host/ehci-hub.c
@@ -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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-pci.c
+++ usb-2.6/drivers/usb/host/ehci-pci.c
@@ -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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
+++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
@@ -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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
+++ usb-2.6/drivers/usb/host/ehci-fsl.c
@@ -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;
 

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Ondrej Zary]

On Thursday 29 April 2010 19:45:33 Alan Stern wrote:
> On Thu, 29 Apr 2010, Alan Stern wrote:
> > > > > > 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
> > > >
> > > > 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.

Thanks for the patch. When applied to 2.6.33 kernel, the ehci-fsl.c part fails 
but that does not matter. Asus P4P800-VM now suspends correctly even when the 
jumpers are set to 5V, both with and without USB device. Also device (un)plug 
does not wake up the system anymore. So the patch works fine.

Yes, this wakeup seems to work differently on different systems. For example, 
Asus A7V8X-X [VIA] (which has these 5V/5VSB jumpers too) is not affected by 
this (it wakes up dead but that's another problem). Also Asus Eee PC 701 
(ICH6) and ASRock 939Dual-VSTA (ULi) are not affected. So maybe it affects 
only ICH4/5/6 chips (only some revisions?). Or it's also board-dependent.

> 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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-hub.c
> +++ usb-2.6/drivers/usb/host/ehci-hub.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-pci.c
> +++ usb-2.6/drivers/usb/host/ehci-pci.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
> +++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
> +++ usb-2.6/drivers/usb/host/ehci-fsl.c
> @@ -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;



-- 
Ondrej Zary

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>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)
>
>On Thu, 29 Apr 2010, Alan Stern wrote:
>
>> > > > > 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
>> > >
>> > > 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,

I will test this patch when back to office, probably at 4 May.

Thanks,
Alek

>
>Alan Stern
>
>
>
>Index: usb-2.6/drivers/usb/host/ehci-hub.c
>===================================================================
>--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
>+++ usb-2.6/drivers/usb/host/ehci-hub.c
>@@ -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
>===================================================================
>--- usb-2.6.orig/drivers/usb/host/ehci-pci.c
>+++ usb-2.6/drivers/usb/host/ehci-pci.c
>@@ -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
>===================================================================
>--- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
>+++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
>@@ -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
>===================================================================
>--- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
>+++ usb-2.6/drivers/usb/host/ehci-fsl.c
>@@ -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;
>

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

On Fri, 30 Apr 2010 01:45:33 +0800
Alan Stern <stern@rowland.harvard.edu> 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;
 -			}




Thanks,
Alek


> 
> 
> 
> Index: usb-2.6/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-hub.c
> +++ usb-2.6/drivers/usb/host/ehci-hub.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-pci.c
> +++ usb-2.6/drivers/usb/host/ehci-pci.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
> +++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
> +++ usb-2.6/drivers/usb/host/ehci-fsl.c
> @@ -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;
>  
> 

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Tue, 11 May 2010, Du, Alek wrote:

> > 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
> > 
> 
> Alan,
> 
> It looks good and works for my hardware. Thanks a lot!
> Will you submit it soon?

Thank you.  Yes, I will.

Alan Stern

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

Alek:

By the way, does the Moorestown platform support multiple ports 
on the root hub?  If it does then using a single time delay (instead of 
a separate delay for each port) makes sense.  But if it doesn't then 
there's no reason to move the power-up and power-down delays outside 
the port loops.

Alan Stern

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Tuesday, May 11, 2010 11:58 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)
>
>Alek:
>
>By the way, does the Moorestown platform support multiple ports
>on the root hub?  If it does then using a single time delay (instead of
>a separate delay for each port) makes sense.  But if it doesn't then
>there's no reason to move the power-up and power-down delays outside
>the port loops.
>
>Alan Stern

Alan,

Yes, the Moorestown platform supports multi-ports, actually my test board has three ports on the root hub.

Thanks,
Alek

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Mon, 10 May 2010, Du, Alek wrote:

> 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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
+++ usb-2.6/drivers/usb/host/ehci-hub.c
@@ -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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-pci.c
+++ usb-2.6/drivers/usb/host/ehci-pci.c
@@ -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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
+++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
@@ -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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
+++ usb-2.6/drivers/usb/host/ehci-fsl.c
@@ -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;
 

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

On Tue, 11 May 2010 05:05:34 +0800
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Mon, 10 May 2010, Du, Alek wrote:
> 
> > 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
> 

Alan,

It looks good and works for my hardware. Thanks a lot!
Will you submit it soon?

Alek

> 
> 
> Index: usb-2.6/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-hub.c
> +++ usb-2.6/drivers/usb/host/ehci-hub.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-pci.c
> +++ usb-2.6/drivers/usb/host/ehci-pci.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
> +++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
> @@ -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
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
> +++ usb-2.6/drivers/usb/host/ehci-fsl.c
> @@ -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;
>  
> 

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Fri, 7 May 2010, Du, Alek wrote:

> 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).

> 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) 

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?

> 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?

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

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

>On Fri, 7 May 2010, Du, Alek wrote:
>
>> 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).

Ok, that's the new behavior that PHCD brings to us, It shouldn't a big deal if we
take care of the wakeup bits before PHCD, right? The spec may be modified if
PHCD become a normal feature to EHCI, just like why we have EHCI1.1 addendum
spec. (We have some ehci1.1 addendum patches that will submit soon, but the
PHCD is not in EHCI1.1 addendum currently)

>
>> 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)
>
>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?

This is doable. It is confirmed by the HW designer, before accessing PORTSC, just
Clear PHCD. But we still need additional 5ms for the second time PHCD setting to
ensure it enter phy low power mode.

>
>> 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?
>
>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?

That should be ok. For Moorestown, the controller should always enable remote
wakeup, so that won't impact us.

>
>Alan Stern

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

On Sat, 8 May 2010 10:00:59 +0800
"Du, Alek" <alek.du@intel.com> wrote:

> >On Fri, 7 May 2010, Du, Alek wrote:
> >
> >> 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.

Thanks,
Alek


diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 004379b..a2d2fbe 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -116,6 +116,17 @@ static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
                u32 __iomem     *reg = &ehci->regs->port_status[port];
                u32             t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
                u32             t2 = t1 & ~PORT_WAKE_BITS;
+               u32 __iomem     *hostpc_reg;
+               u32             temp;
+
+               /* clear phy low power mode first*/
+               if (ehci->has_hostpc) {
+                       hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs
+                                       + HOSTPC0 + 4 * (port & 0xff));
+                       temp = ehci_readl(ehci, hostpc_reg);
+                       ehci_writel(ehci, temp & ~HOSTPC_PHCD, hostpc_reg);
+                       mdelay(5);
+               }
 
                /* only enable appropriate wake bits, otherwise the
                 * hardware can not go phy low power mode. If a race
@@ -131,6 +142,16 @@ static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
                                        port + 1, t1, t2);
                }
                ehci_writel(ehci, t2, reg);
+
+               /* enable phy low power mode again */
+               if (ehci->has_hostpc) {
+                       temp = ehci_readl(ehci, hostpc_reg);
+                       ehci_writel(ehci, temp | HOSTPC_PHCD, hostpc_reg);
+                       temp = ehci_readl(ehci, hostpc_reg);
+                       ehci_dbg(ehci, "Port%d phy low pwr mode %s\n",
+                                       port, (temp & HOSTPC_PHCD) ?
+                                       "succeeded" : "failed");
+               }
        }
 }
 
@@ -217,6 +238,14 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
                                u32 __iomem     *hostpc_reg;
                                u32             t3;
 
+                               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;
+                               }
+                               ehci_writel(ehci, t2, reg);
                                hostpc_reg = (u32 __iomem *)((u8 *)ehci->regs
                                        + HOSTPC0 + 4 * (port & 0xff));
                                spin_unlock_irq(&ehci->lock);

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Thu, 6 May 2010, Du, Alek wrote:

> Hi Alan,
> 
> >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

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

On Fri, 7 May 2010 00:06:33 +0800
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 6 May 2010, Du, Alek wrote:
> 
> > Hi Alan,
> > 
> > >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?


Thanks,
Alek

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Thu, 6 May 2010, Du, Alek wrote:

> 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.

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
into D3hot, the new code will try to turn on wakeup bits for ports that
have already been suspended.  It should be safe because
ehci_set_wakeup_flags() will enable PORT_WKDISC_E | PORT_WKOC_E for
connected ports, and PORT_WKCONN_E | PORT_WKOC_E for unconnected ports.  
Still, since this happens _after_ the ports are suspended and the phy
is put into low-power mode, I wanted to check with you about it.

> 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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
+++ usb-2.6/drivers/usb/host/ehci-hub.c
@@ -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);

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

Hi Alan,

>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?

>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.
>
>> 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

I will test this patch ...

Thanks,
Alek

>
>
>
>Index: usb-2.6/drivers/usb/host/ehci-hub.c
>===================================================================
>--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
>+++ usb-2.6/drivers/usb/host/ehci-hub.c
>@@ -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);

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Wed, 5 May 2010, Du, Alek wrote:

> On Fri, 30 Apr 2010 01:45:33 +0800
> Alan Stern <stern@rowland.harvard.edu> 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;
>  -			}

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
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
+++ usb-2.6/drivers/usb/host/ehci-hub.c
@@ -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
@@ -209,6 +209,7 @@ static int ehci_bus_suspend (struct usb_
 		if (t1 & PORT_OWNER)
 			set_bit(port, &ehci->owned_ports);
 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
+			t2 = t1 & ~PORT_WAKE_BITS;
 			t2 |= PORT_SUSPEND;
 			set_bit(port, &ehci->bus_suspended);
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>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)
>
>On Wed, 5 May 2010, Du, Alek wrote:
>
>> On Fri, 30 Apr 2010 01:45:33 +0800
>> Alan Stern <stern@rowland.harvard.edu> 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;
>>  -			}
>
>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
>===================================================================
>--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
>+++ usb-2.6/drivers/usb/host/ehci-hub.c
>@@ -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
>@@ -209,6 +209,7 @@ static int ehci_bus_suspend (struct usb_
> 		if (t1 & PORT_OWNER)
> 			set_bit(port, &ehci->owned_ports);
> 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
>+			t2 = t1 & ~PORT_WAKE_BITS;
> 			t2 |= PORT_SUSPEND;
> 			set_bit(port, &ehci->bus_suspended);
> 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",

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.

Thanks,
Alek

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Du, Alek]

>> 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;
>>  -			}
>
>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
>===================================================================
>--- usb-2.6.orig/drivers/usb/host/ehci-hub.c
>+++ usb-2.6/drivers/usb/host/ehci-hub.c
>@@ -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
>@@ -209,6 +209,7 @@ static int ehci_bus_suspend (struct usb_
> 		if (t1 & PORT_OWNER)
> 			set_bit(port, &ehci->owned_ports);
> 		else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
>+			t2 = t1 & ~PORT_WAKE_BITS;
> 			t2 |= PORT_SUSPEND;
> 			set_bit(port, &ehci->bus_suspended);
> 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",


Alan,

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.

Thanks,
Alek

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Ondrej Zary]

On Wednesday 28 April 2010 17:41:30 Alan Stern wrote:
> On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> > > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > > 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
>
> 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.

Now I took another machine - IBM ThinkCentre M51 (i915+ICH6). USB ports are 
powered in suspend here so it does not resume immediately. But 
connecting/disconnecting an USB device wakes it up from suspend. Only in 
Linux, not in Windows.

> > > > 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.
>
> >  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
>
> 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.
>
> > > > 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.
> > >
> > > 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.

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).


-- 
Ondrej Zary

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Ondrej Zary]

On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > 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

> > 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

> > 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.)
>
> > 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.
>
> > 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.
>
> 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.

-- 
Ondrej Zary

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Greg KH]

On Tue, Apr 27, 2010 at 10:46:48PM +0200, Ondrej Zary wrote:
> > 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.

On the contrary, it does.  Some of us use this mode to charge devices :)

thanks,

greg k-h

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Tue, 27 Apr 2010, Ondrej Zary wrote:

> On Tuesday 27 April 2010 21:21:23 Alan Stern wrote:
> > On Tue, 27 Apr 2010, Ondrej Zary wrote:
> > > 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

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

> > > 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.

>  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

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.


> > > 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.
> >
> > 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.

Alan Stern

Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[Alan Stern]

On Tue, 27 Apr 2010, Ondrej Zary wrote:

> 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.

> 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.

> 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.)

> 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.

> 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.

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

Alan Stern

[PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)

[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.

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.
Tested on Asus P4P800-VM.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

--- linux-source-2.6.33-orig/drivers/usb/host/ehci-hub.c	2010-02-24 19:52:17.000000000 +0100
+++ linux-source-2.6.33/drivers/usb/host/ehci-hub.c	2010-04-27 18:17:36.000000000 +0200
@@ -173,21 +173,16 @@ static int ehci_bus_suspend (struct usb_
 		}
 
 		/* enable remote wakeup on all ports */
+		t2 &= ~PORT_WAKE_BITS;
 		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 & PORT_CONNECT))
+				t2 |= PORT_WKCONN_E;
+		}
 
 		if (t1 != t2) {
 			ehci_vdbg (ehci, "port %d, %08x -> %08x\n",


-- 
Ondrej Zary