On Wed, 10 Oct 2012, Peter Chen wrote:
> On Tue, Oct 09, 2012 at 11:03:47AM -0400, Alan Stern wrote:
> > On Tue, 9 Oct 2012, Peter Chen wrote:
> >
> > > Hi Alan,
> > >
> > > When I try to optimize system resume time, I find bus resume routine
> > > cost much time (> 20ms), even there is no device at any ports.
> > > Let's take ehci bus resume as an example.
> > >
> > > 1. At ehci_bus_resume
> > >
> > > /* Some controller/firmware combinations need a delay during which
> > > * they set up the port statuses. See Bugzilla #8190. */
> > > spin_unlock_irq(&ehci->lock);
> > > msleep(8);
> > > spin_lock_irq(&ehci->lock);
> > > Is it possible to add condition?
> >
> > If you can come up with a good condition, sure.
> According to Bugzilla #8190, the port status is not correct during
> the suspend, can we use this as condition?, But we may can't find the tester.
>
> See below comment:
>
> Comment #18 From Alan Stern 2007-03-14 13:42:40
>
> Created an attachment (id=10761) [details]
> Diagnose EHCI port resume
>
> This is very bizarre. Here's the important part of the log, leaving out
> everything except for port 3:
>
> [ 0.971514] ehci_hcd 0000:00:1d.7: port 3 status1 1005
> [ 0.991544] ehci_hcd 0000:00:1d.7: port 3 status2 1085
> [ 0.991546] ehci_hcd 0000:00:1d.7: resume done
>
> The status1 value should be 1085 and the status2 value should be 10c5, meaning
> that the port was suspended at first but then the resume bit was turned on.
> Instead the port was unsuspended at first and then somehow it got suspended!
> That's why the scanner wasn't working; it was still suspended.
>
> Try using this new patch and see if it makes any difference.
Yes, I remember writing that.
You could test all the port status registers. If any of them have the
PORT_PE bit set and the PORT_SUSPEND bit clear then the delay is
needed; otherwise it can be skipped.
> > > 2. At hcd_bus_resume
> > > if (status == 0) {
> > > /* TRSMRCY = 10 msec */
> > > msleep(10);
> > > This 10ms delay is needed when the device is connected and CONFIG_USB_SUSPEND
> > > is not defined, can we add condition like:
> >
> > You should not depend on this. In the future, the delay will be needed
> > even when CONFIG_USB_SUSPEND is defined.
> >
> > > if (status == 0) {
> > > #ifndef CONFIG_USB_SUSPEND
> > > if (no_device_on_port)
> > > /* TRSMRCY = 10 msec */
> > > msleep(10);
> > > #endif
> > > For the no_device_on_port, it needs to add flag at struct usb_hcd.
>
> Sorry, should be if (device_on_port)
> >
> > In fact, with ehci-hcd this delay isn't needed at all.
> > ehci_bus_resume() does its own 20-ms delay if there are any unsuspended
> > enabled ports.
>
> unsuspended enabled ports? According to my knowledge, only the port receives
> remote-wakeup is unsuspended enabled port at that situation.
This has nothing to do with remote wakeup.
> According USB Spec 2.0, ch7.1.7.7:
> The USB System Software must provide a 10 ms resume recovery time (TRSMRCY)
> during which it will not attempt to access any device connected to the
> affected (just-activated) bus segment.
>
> The TRSMRCY is needed after resume signal ends up, if the port is not
> companion port, there is no 20ms delay at ehci_bus_resume.
You must be reading the wrong part of the code. I'm talking about this
part:
/* manually resume the ports we suspended during bus_suspend() */
i = HCS_N_PORTS (ehci->hcs_params);
while (i--) {
temp = ehci_readl(ehci, &ehci->regs->port_status [i]);
temp &= ~(PORT_RWC_BITS | PORT_WAKE_BITS);
if (test_bit(i, &ehci->bus_suspended) &&
(temp & PORT_SUSPEND)) {
temp |= PORT_RESUME;
set_bit(i, &resume_needed);
}
ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
}
/* msleep for 20ms only if code is trying to resume port */
if (resume_needed) {
spin_unlock_irq(&ehci->lock);
msleep(20);
spin_lock_irq(&ehci->lock);
if (ehci->shutdown)
goto shutdown;
}
That's the 20-ms delay, and it's not related to the companion
controller.
> If CONFIG_USB_SUSPEND is defined, and ehci->no_selective_suspend is not
> defined (only one Nvidia pci usb hcd defines it)
> the usb_port_suspend/usb_port_resume will
> do port suspend/resume(as well as msleep (10)) if there is a devivce
> on the port, the ehci_bus_suspend/ehci_bus_resume will not send suspend/resume.
> so, the msleep (10) is not needed.
That's true now, but it won't be true in the future. In the future,
usb_port_suspend() won't set the port's suspend feature when entering
system sleep, if the bus is USB-1 or USB-2.
> So, this 10ms delay at hcd_bus_resume is only needed meets below conditions:
> - device on the port
> - The resume is sent by bus resume routine
> - There is no delay after resume signal ends up
It is needed under these conditions:
The HCD is not ehci-hcd.
There is a port on the root hub with suspend status clear
and enabled status set (which implies there must be a device
attached to the port, because disconnected ports can't be
enabled).
Obviously if a port isn't enabled then it doesn't need any delay.
And if a port's suspend status is set then the port will remain
suspended after the root hub resumes, so again it doesn't need a delay.
In fact the second condition above works for every hub, not just root
hubs.
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
