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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
