RE: [PATCH v2 1/1] USB: EHCI: wait more than 3ms until the device enters full-speed idle
What happens if you start putting a different PHY on the board, one that takes longer to enter low-power mode? A little difficult to change a PHY on the board. Just like I said above, it depends on when the hardware enables wake on disconnect, full-speed idle, full-speed idle + PHY enters low power, or only just PHY enters low power? The code should work correctly no matter when the hardware enables wake-on-disconnect. So far I have not seen any complaints about this happening from any user except you. I just tried doing the experiment on my own computer (enable wakeup for the root hub, plug in a high-speed device, and suspend the computer). It worked correctly. Have you enabled wakeup on your high speed device? No. This problem has not occurred for wakeup enabled device or old kernel (before you enable global suspend), since there is a 10ms delay at usb_port_suspend. I just tried the test again on a different computer. This time I was running 3.13-rc8 (before was 3.12.something). In both cases, the device does not support wakeup (it is a flash drive). Also, there already is a 5-ms sleep just below the code you changed. It depends on ehci-has_tdi_phy_lpm. Is that flag set for your system? If it isn't, you could simply remove the test for has_tdi_phy_lpm. That should have the same effect as your patch. It is just for a specific platform which has tdi phy. This case may be generic, in a word, do we need to make sure the bus enters full- speed idle after ehci_bus_suspend has finished? You mean _before_ ehci_bus_suspend has finished. Yes As far as I can see, it doesn't matter. The important thing is whether the bus enters full-speed idle before we enable wake-on-disconnect. If we have not guaranteed it, platform code needs to make sure the bus will not change before the wakeup logic takes effect, of cos, these kinds of platform have no hardware logic to make sure above. So ehci-hcd has to take care of it. Any changes I need to do for my current patch? I will work on it according to your suggestion after China New year holiday (1 week later) Peter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] USB: EHCI: wait more than 3ms until the device enters full-speed idle
On Tue, Jan 28, 2014 at 11:32:28AM -0500, Alan Stern wrote:
On Tue, 28 Jan 2014, Peter Chen wrote:
It sounds like this is a bug in your EHCI hardware. The
wake-on-disconnect logic should never take effect until after the port
goes into full-speed idle.
Where EHCI spec said that? I can't find it at 2.3.9 and 4.3.
It doesn't say that. Not explicitly.
On the other hand, it doesn't say that you have to wait for the port to
enter full-speed idle after turning on the Suspend bit before you can
turn on WKDSCNNT_E bit.
So, it is vendor defined, some vendors enable wakeup on disconnect
after full speed idle, some may not wait.
@@ -301,6 +301,18 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
if (t1 != t2) {
ehci_writel(ehci, t2, reg);
+ if ((t2 PORT_WKDISC_E)
+(ehci_port_speed(ehci, t2) ==
+ USB_PORT_STAT_HIGH_SPEED)) {
+ /*
+* If the high-speed device has not
switched
+* to full-speed idle before WKDISC_E
has
+* effected, there will be a WKDISC
event.
+*/
+ spin_unlock_irq(ehci-lock);
+ usleep_range(3500, 4000);
+ spin_lock_irq(ehci-lock);
+ }
This doesn't look like it will do what you want. t2 already has the
PORT_WKDISC_E bit set, so once the
ehci_writel(ehci, t2, reg);
has executed, it is already too late. Instead, you should write (t2
~PORT_WKDISC_E), then wait a few ms, and then write t2.
Yes, it is safe for writing suspendM first, wait 3-4ms, then write
PORT_WKDISC_E. The reason why my proposal change is ok is the wakeup logic
has still not taken effect before the PHY enters low power mode.
What happens if you start putting a different PHY on the board, one
that takes longer to enter low-power mode?
A little difficult to change a PHY on the board.
Just like I said above, it depends on when the hardware enables wake on
disconnect, full-speed idle, full-speed idle + PHY enters low power,
or only just PHY enters low power?
Since this bug seems to affect only a few EHCI controllers, we should
have a quirk flag for it. There's no need to make everybody wait 3.5-4
ms just for the sake of a few pieces of buggy hardware.
OK, I will add the quirk if you can point me it is not a standard ehci
operation.
So far I have not seen any complaints about this happening from any
user except you. I just tried doing the experiment on my own computer
(enable wakeup for the root hub, plug in a high-speed device, and
suspend the computer). It worked correctly.
Have you enabled wakeup on your high speed device?
This problem has not occurred for wakeup enabled device or old kernel
(before you enable global suspend), since there is a 10ms delay at
usb_port_suspend.
Also, there already is a 5-ms sleep just below the code you changed.
It depends on ehci-has_tdi_phy_lpm. Is that flag set for your system?
If it isn't, you could simply remove the test for has_tdi_phy_lpm.
That should have the same effect as your patch.
It is just for a specific platform which has tdi phy. This case may
be generic, in a word, do we need to make sure the bus enters full-speed
idle after ehci_bus_suspend has finished? If we have not guaranteed
it, platform code needs to make sure the bus will not change before
the wakeup logic takes effect, of cos, these kinds of platform have
no hardware logic to make sure above.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] USB: EHCI: wait more than 3ms until the device enters full-speed idle
On Wed, 29 Jan 2014, Peter Chen wrote: On Tue, Jan 28, 2014 at 11:32:28AM -0500, Alan Stern wrote: On Tue, 28 Jan 2014, Peter Chen wrote: It sounds like this is a bug in your EHCI hardware. The wake-on-disconnect logic should never take effect until after the port goes into full-speed idle. Where EHCI spec said that? I can't find it at 2.3.9 and 4.3. It doesn't say that. Not explicitly. On the other hand, it doesn't say that you have to wait for the port to enter full-speed idle after turning on the Suspend bit before you can turn on WKDSCNNT_E bit. So, it is vendor defined, some vendors enable wakeup on disconnect after full speed idle, some may not wait. Okay. This doesn't look like it will do what you want. t2 already has the PORT_WKDISC_E bit set, so once the ehci_writel(ehci, t2, reg); has executed, it is already too late. Instead, you should write (t2 ~PORT_WKDISC_E), then wait a few ms, and then write t2. Yes, it is safe for writing suspendM first, wait 3-4ms, then write PORT_WKDISC_E. The reason why my proposal change is ok is the wakeup logic has still not taken effect before the PHY enters low power mode. What happens if you start putting a different PHY on the board, one that takes longer to enter low-power mode? A little difficult to change a PHY on the board. Just like I said above, it depends on when the hardware enables wake on disconnect, full-speed idle, full-speed idle + PHY enters low power, or only just PHY enters low power? The code should work correctly no matter when the hardware enables wake-on-disconnect. So far I have not seen any complaints about this happening from any user except you. I just tried doing the experiment on my own computer (enable wakeup for the root hub, plug in a high-speed device, and suspend the computer). It worked correctly. Have you enabled wakeup on your high speed device? No. This problem has not occurred for wakeup enabled device or old kernel (before you enable global suspend), since there is a 10ms delay at usb_port_suspend. I just tried the test again on a different computer. This time I was running 3.13-rc8 (before was 3.12.something). In both cases, the device does not support wakeup (it is a flash drive). Also, there already is a 5-ms sleep just below the code you changed. It depends on ehci-has_tdi_phy_lpm. Is that flag set for your system? If it isn't, you could simply remove the test for has_tdi_phy_lpm. That should have the same effect as your patch. It is just for a specific platform which has tdi phy. This case may be generic, in a word, do we need to make sure the bus enters full-speed idle after ehci_bus_suspend has finished? You mean _before_ ehci_bus_suspend has finished. As far as I can see, it doesn't matter. The important thing is whether the bus enters full-speed idle before we enable wake-on-disconnect. If we have not guaranteed it, platform code needs to make sure the bus will not change before the wakeup logic takes effect, of cos, these kinds of platform have no hardware logic to make sure above. So ehci-hcd has to take care of it. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] USB: EHCI: wait more than 3ms until the device enters full-speed idle
On Tue, 28 Jan 2014, Peter Chen wrote:
It sounds like this is a bug in your EHCI hardware. The
wake-on-disconnect logic should never take effect until after the port
goes into full-speed idle.
Where EHCI spec said that? I can't find it at 2.3.9 and 4.3.
It doesn't say that. Not explicitly.
On the other hand, it doesn't say that you have to wait for the port to
enter full-speed idle after turning on the Suspend bit before you can
turn on WKDSCNNT_E bit.
@@ -301,6 +301,18 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
if (t1 != t2) {
ehci_writel(ehci, t2, reg);
+ if ((t2 PORT_WKDISC_E)
+ (ehci_port_speed(ehci, t2) ==
+ USB_PORT_STAT_HIGH_SPEED)) {
+ /*
+ * If the high-speed device has not switched
+ * to full-speed idle before WKDISC_E has
+ * effected, there will be a WKDISC event.
+ */
+ spin_unlock_irq(ehci-lock);
+ usleep_range(3500, 4000);
+ spin_lock_irq(ehci-lock);
+ }
This doesn't look like it will do what you want. t2 already has the
PORT_WKDISC_E bit set, so once the
ehci_writel(ehci, t2, reg);
has executed, it is already too late. Instead, you should write (t2
~PORT_WKDISC_E), then wait a few ms, and then write t2.
Yes, it is safe for writing suspendM first, wait 3-4ms, then write
PORT_WKDISC_E. The reason why my proposal change is ok is the wakeup logic
has still not taken effect before the PHY enters low power mode.
What happens if you start putting a different PHY on the board, one
that takes longer to enter low-power mode?
Since this bug seems to affect only a few EHCI controllers, we should
have a quirk flag for it. There's no need to make everybody wait 3.5-4
ms just for the sake of a few pieces of buggy hardware.
OK, I will add the quirk if you can point me it is not a standard ehci
operation.
So far I have not seen any complaints about this happening from any
user except you. I just tried doing the experiment on my own computer
(enable wakeup for the root hub, plug in a high-speed device, and
suspend the computer). It worked correctly.
Also, there already is a 5-ms sleep just below the code you changed.
It depends on ehci-has_tdi_phy_lpm. Is that flag set for your system?
If it isn't, you could simply remove the test for has_tdi_phy_lpm.
That should have the same effect as your patch.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] USB: EHCI: wait more than 3ms until the device enters full-speed idle
On Mon, 27 Jan 2014, Peter Chen wrote:
If the high-speed device does not enter full-speed idle after
wakeup on disconnect logic has effected, there will be an
unexpected disconnect wakeup interrupt due to the bus is still SE0.
I think you mean If the high-speed device does not enter full-speed
idle _before_ wakeup on disconnect logic has taken effect...
It sounds like this is a bug in your EHCI hardware. The
wake-on-disconnect logic should never take effect until after the port
goes into full-speed idle.
Signed-off-by: Peter Chen peter.c...@freescale.com
---
Changes for v2:
Using usleep_range instead of mdelay
drivers/usb/host/ehci-hub.c | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 47b858f..976294c 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -301,6 +301,18 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
if (t1 != t2) {
ehci_writel(ehci, t2, reg);
+ if ((t2 PORT_WKDISC_E)
+ (ehci_port_speed(ehci, t2) ==
+ USB_PORT_STAT_HIGH_SPEED)) {
+ /*
+ * If the high-speed device has not switched
+ * to full-speed idle before WKDISC_E has
+ * effected, there will be a WKDISC event.
+ */
+ spin_unlock_irq(ehci-lock);
+ usleep_range(3500, 4000);
+ spin_lock_irq(ehci-lock);
+ }
This doesn't look like it will do what you want. t2 already has the
PORT_WKDISC_E bit set, so once the
ehci_writel(ehci, t2, reg);
has executed, it is already too late. Instead, you should write (t2
~PORT_WKDISC_E), then wait a few ms, and then write t2.
Since this bug seems to affect only a few EHCI controllers, we should
have a quirk flag for it. There's no need to make everybody wait 3.5-4
ms just for the sake of a few pieces of buggy hardware.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] USB: EHCI: wait more than 3ms until the device enters full-speed idle
On Mon, Jan 27, 2014 at 10:52:48AM -0500, Alan Stern wrote:
On Mon, 27 Jan 2014, Peter Chen wrote:
If the high-speed device does not enter full-speed idle after
wakeup on disconnect logic has effected, there will be an
unexpected disconnect wakeup interrupt due to the bus is still SE0.
I think you mean If the high-speed device does not enter full-speed
idle _before_ wakeup on disconnect logic has taken effect...
Yes, thanks.
It sounds like this is a bug in your EHCI hardware. The
wake-on-disconnect logic should never take effect until after the port
goes into full-speed idle.
Where EHCI spec said that? I can't find it at 2.3.9 and 4.3.
Signed-off-by: Peter Chen peter.c...@freescale.com
---
Changes for v2:
Using usleep_range instead of mdelay
drivers/usb/host/ehci-hub.c | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 47b858f..976294c 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -301,6 +301,18 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
if (t1 != t2) {
ehci_writel(ehci, t2, reg);
+ if ((t2 PORT_WKDISC_E)
+(ehci_port_speed(ehci, t2) ==
+ USB_PORT_STAT_HIGH_SPEED)) {
+ /*
+* If the high-speed device has not switched
+* to full-speed idle before WKDISC_E has
+* effected, there will be a WKDISC event.
+*/
+ spin_unlock_irq(ehci-lock);
+ usleep_range(3500, 4000);
+ spin_lock_irq(ehci-lock);
+ }
This doesn't look like it will do what you want. t2 already has the
PORT_WKDISC_E bit set, so once the
ehci_writel(ehci, t2, reg);
has executed, it is already too late. Instead, you should write (t2
~PORT_WKDISC_E), then wait a few ms, and then write t2.
Yes, it is safe for writing suspendM first, wait 3-4ms, then write
PORT_WKDISC_E. The reason why my proposal change is ok is the wakeup logic
has still not taken effect before the PHY enters low power mode.
Since this bug seems to affect only a few EHCI controllers, we should
have a quirk flag for it. There's no need to make everybody wait 3.5-4
ms just for the sake of a few pieces of buggy hardware.
OK, I will add the quirk if you can point me it is not a standard ehci
operation.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/1] USB: EHCI: wait more than 3ms until the device enters full-speed idle
If the high-speed device does not enter full-speed idle after
wakeup on disconnect logic has effected, there will be an
unexpected disconnect wakeup interrupt due to the bus is still SE0.
Signed-off-by: Peter Chen peter.c...@freescale.com
---
Changes for v2:
Using usleep_range instead of mdelay
drivers/usb/host/ehci-hub.c | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 47b858f..976294c 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -301,6 +301,18 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
if (t1 != t2) {
ehci_writel(ehci, t2, reg);
+ if ((t2 PORT_WKDISC_E)
+(ehci_port_speed(ehci, t2) ==
+ USB_PORT_STAT_HIGH_SPEED)) {
+ /*
+* If the high-speed device has not switched
+* to full-speed idle before WKDISC_E has
+* effected, there will be a WKDISC event.
+*/
+ spin_unlock_irq(ehci-lock);
+ usleep_range(3500, 4000);
+ spin_lock_irq(ehci-lock);
+ }
changed = 1;
}
}
--
1.7.8
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
