From: Du, Alek on
>> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
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);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Du, Alek on
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);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Du, Alek on
On Fri, 7 May 2010 00:06:33 +0800
Alan Stern <stern(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/