* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
@ 2010-04-29 17:45 ` Alan Stern
0 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-04-29 17:45 UTC (permalink / raw)
To: Ondrej Zary, Alek Du
Cc: Linux-pm mailing list, USB list, Kernel development list
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;
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-04-29 17:45 ` [linux-pm] " Alan Stern
@ 2010-04-29 21:14 ` Ondrej Zary
-1 siblings, 0 replies; 54+ messages in thread
From: Ondrej Zary @ 2010-04-29 21:14 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Kernel development list, Alek Du
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
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
@ 2010-04-29 21:14 ` Ondrej Zary
0 siblings, 0 replies; 54+ messages in thread
From: Ondrej Zary @ 2010-04-29 21:14 UTC (permalink / raw)
To: Alan Stern
Cc: Alek Du, Linux-pm mailing list, USB list, Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-04-29 17:45 ` [linux-pm] " Alan Stern
(?)
(?)
@ 2010-05-04 5:37 ` Du, Alek
-1 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-04 5:37 UTC (permalink / raw)
To: Alan Stern, Ondrej Zary
Cc: Linux-pm mailing list, USB list, Kernel development list
>-----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;
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-04-29 17:45 ` [linux-pm] " Alan Stern
` (2 preceding siblings ...)
(?)
@ 2010-05-04 5:37 ` Du, Alek
-1 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-04 5:37 UTC (permalink / raw)
To: Alan Stern, Ondrej Zary
Cc: Linux-pm mailing list, USB list, Kernel development list
>-----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;
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-04-29 17:45 ` [linux-pm] " Alan Stern
` (3 preceding siblings ...)
(?)
@ 2010-05-05 3:12 ` Du, Alek
-1 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-05 3:12 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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;
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-04-29 17:45 ` [linux-pm] " Alan Stern
` (4 preceding siblings ...)
(?)
@ 2010-05-05 3:12 ` Du, Alek
2010-05-05 15:55 ` Alan Stern
2010-05-05 15:55 ` Alan Stern
-1 siblings, 2 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-05 3:12 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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;
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-05 3:12 ` [linux-pm] " Du, Alek
@ 2010-05-05 15:55 ` Alan Stern
2010-05-06 0:11 ` Du, Alek
` (3 more replies)
2010-05-05 15:55 ` Alan Stern
1 sibling, 4 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-05 15:55 UTC (permalink / raw)
To: Du, Alek
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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",
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-05 15:55 ` Alan Stern
@ 2010-05-06 0:11 ` Du, Alek
2010-05-06 0:11 ` [linux-pm] " Du, Alek
` (2 subsequent siblings)
3 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-06 0:11 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
>-----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
^ permalink raw reply [flat|nested] 54+ messages in thread* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-05 15:55 ` Alan Stern
2010-05-06 0:11 ` Du, Alek
@ 2010-05-06 0:11 ` Du, Alek
2010-05-06 8:23 ` Du, Alek
2010-05-06 8:23 ` [linux-pm] " Du, Alek
3 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-06 0:11 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
>-----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
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-05 15:55 ` Alan Stern
2010-05-06 0:11 ` Du, Alek
2010-05-06 0:11 ` [linux-pm] " Du, Alek
@ 2010-05-06 8:23 ` Du, Alek
2010-05-06 8:23 ` [linux-pm] " Du, Alek
3 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-06 8:23 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
>> 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
^ permalink raw reply [flat|nested] 54+ messages in thread* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-05 15:55 ` Alan Stern
` (2 preceding siblings ...)
2010-05-06 8:23 ` Du, Alek
@ 2010-05-06 8:23 ` Du, Alek
2010-05-06 14:46 ` Alan Stern
2010-05-06 14:46 ` [linux-pm] " Alan Stern
3 siblings, 2 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-06 8:23 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
>> 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
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 8:23 ` [linux-pm] " Du, Alek
@ 2010-05-06 14:46 ` Alan Stern
2010-05-06 14:46 ` [linux-pm] " Alan Stern
1 sibling, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-06 14:46 UTC (permalink / raw)
To: Du, Alek
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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);
^ permalink raw reply [flat|nested] 54+ messages in thread* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 8:23 ` [linux-pm] " Du, Alek
2010-05-06 14:46 ` Alan Stern
@ 2010-05-06 14:46 ` Alan Stern
2010-05-06 15:20 ` Du, Alek
2010-05-06 15:20 ` [linux-pm] " Du, Alek
1 sibling, 2 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-06 14:46 UTC (permalink / raw)
To: Du, Alek
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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);
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 14:46 ` [linux-pm] " Alan Stern
@ 2010-05-06 15:20 ` Du, Alek
2010-05-06 15:20 ` [linux-pm] " Du, Alek
1 sibling, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-06 15:20 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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);
^ permalink raw reply [flat|nested] 54+ messages in thread* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 14:46 ` [linux-pm] " Alan Stern
2010-05-06 15:20 ` Du, Alek
@ 2010-05-06 15:20 ` Du, Alek
2010-05-06 16:06 ` Alan Stern
2010-05-06 16:06 ` Alan Stern
1 sibling, 2 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-06 15:20 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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);
^ permalink raw reply [flat|nested] 54+ messages in thread* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 15:20 ` [linux-pm] " Du, Alek
@ 2010-05-06 16:06 ` Alan Stern
2010-05-07 1:32 ` Du, Alek
2010-05-07 1:32 ` Du, Alek
2010-05-06 16:06 ` Alan Stern
1 sibling, 2 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-06 16:06 UTC (permalink / raw)
To: Du, Alek
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 16:06 ` Alan Stern
@ 2010-05-07 1:32 ` Du, Alek
2010-05-07 15:20 ` Alan Stern
2010-05-07 15:20 ` Alan Stern
2010-05-07 1:32 ` Du, Alek
1 sibling, 2 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-07 1:32 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-07 1:32 ` Du, Alek
@ 2010-05-07 15:20 ` Alan Stern
2010-05-08 2:00 ` Du, Alek
` (2 more replies)
2010-05-07 15:20 ` Alan Stern
1 sibling, 3 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-07 15:20 UTC (permalink / raw)
To: Du, Alek
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-07 15:20 ` Alan Stern
@ 2010-05-08 2:00 ` Du, Alek
2010-05-08 2:00 ` Du, Alek
[not found] ` <6C44CD31806DCD4BB88546DAB7AFC9A201EB5A39FA@shsmsx502.ccr.corp.intel.com>
2 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-08 2:00 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
>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
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-07 15:20 ` Alan Stern
2010-05-08 2:00 ` Du, Alek
@ 2010-05-08 2:00 ` Du, Alek
[not found] ` <6C44CD31806DCD4BB88546DAB7AFC9A201EB5A39FA@shsmsx502.ccr.corp.intel.com>
2 siblings, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-08 2:00 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
>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
^ permalink raw reply [flat|nested] 54+ messages in thread[parent not found: <6C44CD31806DCD4BB88546DAB7AFC9A201EB5A39FA@shsmsx502.ccr.corp.intel.com>]
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
[not found] ` <6C44CD31806DCD4BB88546DAB7AFC9A201EB5A39FA@shsmsx502.ccr.corp.intel.com>
@ 2010-05-10 2:25 ` Du, Alek
2010-05-10 2:25 ` [linux-pm] " Du, Alek
1 sibling, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-10 2:25 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list, 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);
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
[not found] ` <6C44CD31806DCD4BB88546DAB7AFC9A201EB5A39FA@shsmsx502.ccr.corp.intel.com>
2010-05-10 2:25 ` Du, Alek
@ 2010-05-10 2:25 ` Du, Alek
2010-05-10 21:05 ` Alan Stern
2010-05-10 21:05 ` Alan Stern
1 sibling, 2 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-10 2:25 UTC (permalink / raw)
To: Alan Stern
Cc: Du, Alek, Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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);
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-10 2:25 ` [linux-pm] " Du, Alek
@ 2010-05-10 21:05 ` Alan Stern
2010-05-11 3:31 ` Du, Alek
2010-05-11 3:31 ` [linux-pm] " Du, Alek
2010-05-10 21:05 ` Alan Stern
1 sibling, 2 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-10 21:05 UTC (permalink / raw)
To: Du, Alek
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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;
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-10 21:05 ` Alan Stern
@ 2010-05-11 3:31 ` Du, Alek
2010-05-11 3:31 ` [linux-pm] " Du, Alek
1 sibling, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-11 3:31 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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;
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-10 21:05 ` Alan Stern
2010-05-11 3:31 ` Du, Alek
@ 2010-05-11 3:31 ` Du, Alek
2010-05-11 14:01 ` Alan Stern
` (3 more replies)
1 sibling, 4 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-11 3:31 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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;
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-11 3:31 ` [linux-pm] " Du, Alek
@ 2010-05-11 14:01 ` Alan Stern
2010-05-11 14:01 ` [linux-pm] " Alan Stern
` (2 subsequent siblings)
3 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-11 14:01 UTC (permalink / raw)
To: Du, Alek
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-11 3:31 ` [linux-pm] " Du, Alek
2010-05-11 14:01 ` Alan Stern
@ 2010-05-11 14:01 ` Alan Stern
2010-05-11 15:58 ` Alan Stern
2010-05-11 15:58 ` Alan Stern
3 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-11 14:01 UTC (permalink / raw)
To: Du, Alek
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-11 3:31 ` [linux-pm] " Du, Alek
2010-05-11 14:01 ` Alan Stern
2010-05-11 14:01 ` [linux-pm] " Alan Stern
@ 2010-05-11 15:58 ` Alan Stern
2010-05-11 16:10 ` Du, Alek
2010-05-11 16:10 ` [linux-pm] " Du, Alek
2010-05-11 15:58 ` Alan Stern
3 siblings, 2 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-11 15:58 UTC (permalink / raw)
To: Du, Alek
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-11 15:58 ` Alan Stern
@ 2010-05-11 16:10 ` Du, Alek
2010-05-11 16:10 ` [linux-pm] " Du, Alek
1 sibling, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-11 16:10 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
>-----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
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [linux-pm] [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-11 15:58 ` Alan Stern
2010-05-11 16:10 ` Du, Alek
@ 2010-05-11 16:10 ` Du, Alek
1 sibling, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-11 16:10 UTC (permalink / raw)
To: Alan Stern
Cc: Ondrej Zary, Linux-pm mailing list, USB list,
Kernel development list
>-----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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-11 3:31 ` [linux-pm] " Du, Alek
` (2 preceding siblings ...)
2010-05-11 15:58 ` Alan Stern
@ 2010-05-11 15:58 ` Alan Stern
3 siblings, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-11 15:58 UTC (permalink / raw)
To: Du, Alek
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-10 2:25 ` [linux-pm] " Du, Alek
2010-05-10 21:05 ` Alan Stern
@ 2010-05-10 21:05 ` Alan Stern
1 sibling, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-10 21:05 UTC (permalink / raw)
To: Du, Alek
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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;
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-07 1:32 ` Du, Alek
2010-05-07 15:20 ` Alan Stern
@ 2010-05-07 15:20 ` Alan Stern
1 sibling, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-07 15:20 UTC (permalink / raw)
To: Du, Alek
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 16:06 ` Alan Stern
2010-05-07 1:32 ` Du, Alek
@ 2010-05-07 1:32 ` Du, Alek
1 sibling, 0 replies; 54+ messages in thread
From: Du, Alek @ 2010-05-07 1:32 UTC (permalink / raw)
To: Alan Stern
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-06 15:20 ` [linux-pm] " Du, Alek
2010-05-06 16:06 ` Alan Stern
@ 2010-05-06 16:06 ` Alan Stern
1 sibling, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-06 16:06 UTC (permalink / raw)
To: Du, Alek
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] [RFC] ehci: Disable wake on overcurrent (WKOC_E) and disconnect (WKDISC_E)
2010-05-05 3:12 ` [linux-pm] " Du, Alek
2010-05-05 15:55 ` Alan Stern
@ 2010-05-05 15:55 ` Alan Stern
1 sibling, 0 replies; 54+ messages in thread
From: Alan Stern @ 2010-05-05 15:55 UTC (permalink / raw)
To: Du, Alek
Cc: Linux-pm mailing list, USB list, Ondrej Zary,
Kernel development list
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",
^ permalink raw reply [flat|nested] 54+ messages in thread