[Cbe-oss-dev] [PATCH 1/5] usb: Fix PS3 EHCI suspend
Geoff Levand
geoff at infradead.org
Fri Nov 18 09:14:03 EST 2011
Hi Alan,
I tried to give some verbose comments to help you understand the problem. I
would greatly appreciate any advice or test suggestions you have.
On Tue, 2011-11-15 at 11:35 -0500, Alan Stern wrote:
On Tue, 15 Nov 2011, Geoff Levand wrote:
>
> > The PS3 EHCI HC stops the root hub after both root hub ports are
> > suspended. The current EHCI driver can't handle this behavior.
> > Three workarounds are needed in the PS3 bus glue.
>
> Can you be more specific? What exactly do you mean by "stops the root
> hub"?
Maybe it is not the root hub, but the HC that is 'stopped'. Here is what I
observe:
This is 100% repeatable. With a kernel build with PM support and USB debug
messages enabled, and with no external USB devices attached to the system soon
after startup messages like these are seen:
ps3-ehci-driver sb_08: port 1 high speed
ps3-ehci-driver sb_08: GetStatus port:1 status 001005 0 ACK POWER sig=se0 PE CONNECT
ps3-ehci-driver sb_08: port 2 high speed
ps3-ehci-driver sb_08: GetStatus port:2 status 001005 0 ACK POWER sig=se0 PE CONNECT
usb usb3: bus auto-suspend, wakeup 1
usb usb4: bus auto-suspend, wakeup 1
usb 1-1: unlink qh256-0001/c000000006238180 start 1 [1/0 us]
usb 1-2: unlink qh256-0001/c000000006229580 start 2 [1/0 us]
usb usb1: bus auto-suspend, wakeup 1
ps3-ehci-driver sb_06: suspend root hub
ps3-ehci-driver sb_06: force halt; handshake d000080080020014 0000c000 00000000 -> -110
Adding a dump_stack() call to handshake_on_error_set_halt() gives this:
ps3-ehci-driver sb_06: suspend root hub
Call Trace:
[c00000000610b610] [c000000000011248] .show_stack+0x6c/0x16c (unreliable)
[c00000000610b6c0] [c0000000002e5fdc] .handshake_on_error_set_halt+0x48/0xa8
[c00000000610b760] [c0000000002e60c4] .ehci_quiesce+0x88/0x148
[c00000000610b7e0] [c0000000002e9810] .ehci_bus_suspend+0x11c/0x514
[c00000000610b890] [c0000000002cecd0] .hcd_bus_suspend+0xec/0x1a8
[c00000000610b930] [c0000000002df390] .generic_suspend+0x20/0x58
[c00000000610b9a0] [c0000000002d6110] .usb_suspend_both+0x1d0/0x2c4
[c00000000610ba60] [c0000000002d62dc] .usb_runtime_suspend+0xd8/0x120
[c00000000610bae0] [c00000000029b304] .__rpm_callback+0x5c/0xa4
[c00000000610bb70] [c00000000029b9a8] .rpm_suspend+0x318/0x5a4
[c00000000610bc80] [c00000000029cda4] .pm_runtime_work+0xa8/0xdc
[c00000000610bd10] [c000000000069edc] .process_one_work+0x2cc/0x4ac
[c00000000610bdf0] [c00000000006a53c] .worker_thread+0x268/0x3f4
[c00000000610bea0] [c000000000071dcc] .kthread+0xb0/0xbc
[c00000000610bf90] [c00000000001c138] .kernel_thread+0x54/0x70
ps3-ehci-driver sb_06: force halt; handshake d000080080020014 0000c000 00000000 -> -110
The first call to handshake_on_error_set_halt() in ehci_quiesce() is failing
with ETIMEDOUT (-100). After a bit of searching I found what seemed like odd
behavior that was related to the handshake failure. If I add this routine to
ehci-hcd.c:
+#define ps3_frame_index_check(_a) _ps3_frame_index_check(_a, __func__, __LINE__)
+static int _ps3_frame_index_check(struct ehci_hcd *ehci, const char *func, int line)
+{
+ unsigned int old_stamp = ehci_read_frame_index(ehci);
+ unsigned int i;
+
+ udelay(125);
+
+ for (i = 20; i; i--) {
+ if (ehci_read_frame_index(ehci) == old_stamp)
+ printk(".");
+ else
+ break;
+ udelay(125);
+ }
+
+ if (i) {
+ printk("%s:%d: frame_index: ok\n", func, line);
+ return 0;
+ }
+
+ printk("\n%s:%d: frame_index: stopped\n", func, line);
+
+ return -1;
+}
Then add these to the SetPortFeature + USB_PORT_FEAT_SUSPEND case of
ehci_hub_control():
case USB_PORT_FEAT_SUSPEND:
+ ehci_dbg(ehci, "port %d SUSPEND\n", wIndex + 1);
+ ps3_frame_index_check(ehci);
if (ehci->no_selective_suspend)
break;
if ((temp & PORT_PE) == 0
|| (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;
ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
if (hostpc_reg) {
spin_unlock_irqrestore(&ehci->lock, flags);
msleep(5);/* 5ms for HCD enter low pwr mode */
spin_lock_irqsave(&ehci->lock, flags);
temp1 = ehci_readl(ehci, hostpc_reg);
ehci_writel(ehci, temp1 | HOSTPC_PHCD,
hostpc_reg);
temp1 = ehci_readl(ehci, hostpc_reg);
ehci_dbg(ehci, "Port%d phy low pwr mode %s\n",
wIndex, (temp1 & HOSTPC_PHCD) ?
"succeeded" : "failed");
}
set_bit(wIndex, &ehci->suspended_ports);
+ udelay(3000);
+ ps3_frame_index_check(ehci);
break;
I get this output:
ps3-ehci-driver sb_06: port 1 SUSPEND
ehci_hub_control:976: frame_index: ok
ehci_hub_control:1004: frame_index: ok
ps3-ehci-driver sb_06: port 2 SUSPEND
ehci_hub_control:976: frame_index: ok
....................
ehci_hub_control:1004: frame_index: stopped
usb usb1: bus auto-suspend, wakeup 1
ps3-ehci-driver sb_06: suspend root hub
ps3-ehci-driver sb_06: force halt; handshake d000080080020014 0000c000 00000000 -> -110
If I change the delay from udelay(3000) to udelay(2500) the final
ps3_frame_index_check() will succeed, but the ehci_quiesce handshake will
fail.
I don't think the frame index should stop incrementing after the second port is
suspended, and I think the cause is the reason the ehci_quiesce handshake
fails.
> Does the controller do this if only one port is suspended and the other
> is not enabled? What if neither port is enabled?
I have not tested these.
> > 1) The EHCI root hub driver expects the root hub to still be running when
> > ehci_bus_suspend() is called.
>
> In what respect does it expect the root hub still to be running? That
> is, what goes wrong?
As above, the handshake in ehci_quiesce() fails.
> > Add a new routine ps3_ehci_bus_suspend()
> > that forces the HC into the HALT state before calling ehci_bus_suspend().
> > This will allow ehci_bus_suspend() to succeed.
>
> Are you saying that currently the HC remains in the RUN state but the
> root hub doesn't actually run?
If the HC is in the HALT state, or at least if ehci->state == HC_STATE_SUSPENDED
a different code path will be taken and no handshake will be done, so the
ehci_bus_suspend() call will be successful.
> > 2) Add a new routine ps3_ehci_bus_resume() to get the HC back into a
> > running state before ehci_bus_resume() is called.
>
> Is this to counteract the effect of the previous paragraph, in which
> you force the HC into the HALT state? Aren't there better ways of
> handling this?
Yes, this was to undo the HC HALT. I would appreciate any suggestions for a
better way.
> > 3) Add a call to ps3_ehci_bus_resume() in ps3_ehci_remove() to
> > assure usb_remove_hcd() is not called when the HC is suspended.
>
> This doesn't sound quite right. ps3_ehci_bus_resume() is responsible
> for resuming the root hub, not resuming the HC. In fact, I don't see
> any code in ehci-ps3.c for suspending the HC -- so how can the HC get
> suspended in the first place?
Maybe 'HC is suspended' is not quite the correct term, when
ehci->state == HC_STATE_SUSPENDED.
> > Fixes runtime errors similar to these:
> >
> > ps3-ehci-driver: force halt; handhake d0000800802e0014 0000c000 00000000 -> -110
>
> This means the controller refused to turn off the periodic and async
> schedules. Why does it do that?
As above, the handshake in ehci_quiesce() fails. The hardware is broken
I guess.
> > kernel BUG at drivers/usb/host/ehci-mem.c:74!
>
> This means some qh was still linked (or something similar) when it was
> removed. I don't see how these are connected to the root hub stopping.
I think it is because when the HC frame index stops incrementing the qh
processing does not finish up, so it is still linked.
-Geoff
More information about the cbe-oss-dev
mailing list