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