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

[Du, Alek]

>-----Original Message-----
>From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx]
>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;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/