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

Reply via email to