[PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
in [Kernel]
|
Prev: microblaze: re-enable interrupts before calling schedule
Next: USB mass storage kernel panic after two days sleep/wakeup
From: Alan Stern on 30 Apr 2010 16:00 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; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Du, Alek on 4 May 2010 01:40 >-----Original Message----- >From: Alan Stern [mailto:stern(a)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; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Du, Alek on 4 May 2010 23:20 On Fri, 30 Apr 2010 01:45:33 +0800 Alan Stern <stern(a)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; > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on 5 May 2010 12:00 On Wed, 5 May 2010, Du, Alek wrote: > On Fri, 30 Apr 2010 01:45:33 +0800 > Alan Stern <stern(a)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", -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Du, Alek on 5 May 2010 20:20
>-----Original Message----- >From: Alan Stern [mailto:stern(a)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(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |