[Cbe-oss-dev] [PATCH 1/5] usb: Fix PS3 EHCI suspend
Alan Stern
stern at rowland.harvard.edu
Sat Nov 19 03:19:08 EST 2011
On Thu, 17 Nov 2011, Geoff Levand wrote:
> 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.
Yes indeed. So it appears the root hub really does get stopped when
all the ports are suspended. Can you add a call to dbg_status() in
_ps3_frame_index_check()? I'd like to know if the STS_HALT bit is set.
> > 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.
It would be nice to know. Is there any way you can test it? If those
hubs are internal and permanently connected then I guess you can't. Do
the controllers really have only two ports?
> > > 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.
I would expect the IAA watchdog timer to catch something like that.
Alan Stern
More information about the cbe-oss-dev
mailing list