On Wed, Oct 10, 2012 at 10:48:39AM -0400, Alan Stern wrote:
> 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.
I think the condition should like below, as we need to consider remote wakeup.
When remote wakeup occurs, it doesn't need this delay either.
if (portsc & PORT_PE) && ~(portsc & (PORT_SUSPEND | PORT_RESUME)))
do_delay;
>
> > > > 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.
I mean the port status before roothub sends resume, only at remote wakeup
situation, that port status will be unsuspended enabled.
>
> > 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.
If you have only delay above 20ms, and then clear PORT_RESUME, you
still need delay TRSMRCY, as TRSMRCY is the time after PORT_RESUME is
cleared.
>
> 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.
Is it ok to set one hcd flag, like hcd->unsuspended_device_on_port
to get condition at xxx_bus_suspend?
>
> 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
>
>
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
