On Mon, Feb 10, 2014 at 11:40:25AM -0500, Alan Stern wrote:
> On Fri, 7 Feb 2014, Peter Chen wrote:
>
> > If the high-speed device does not enter full-speed idle after
> > wakeup on disconnect logic has effected, there will be an
> > unexpected disconnect wakeup interrupt due to the bus is still SE0.
> >
> > Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> >
> > Hi Alan,
> >
> > If you can't met this issue, it may due to your hardware enables
> > WKDN after detecting full-speed idle or the time between stop
> > SoF and enable WKDN is more than 3ms (You need to know when
> > the WKDN has really taken effect).
>
> Yes. However, I don't like the way your patch can increase the total
> time if several ports have to be put into suspend. Consider this patch
> instead.
>
> Alan Stern
>
>
>
> Index: usb-3.14/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- usb-3.14.orig/drivers/usb/host/ehci-hub.c
> +++ usb-3.14/drivers/usb/host/ehci-hub.c
> @@ -238,6 +238,7 @@ static int ehci_bus_suspend (struct usb_
> int port;
> int mask;
> int changed;
> + unsigned want_WKDISC;
Not sure mixed uppercase letters and lowercase letters into a
single variable is a good idea.
>
> ehci_dbg(ehci, "suspend root hub\n");
>
> @@ -272,6 +273,7 @@ static int ehci_bus_suspend (struct usb_
> ehci->bus_suspended = 0;
> ehci->owned_ports = 0;
> changed = 0;
> + want_WKDISC = 0;
> port = HCS_N_PORTS(ehci->hcs_params);
> while (port--) {
> u32 __iomem *reg = &ehci->regs->port_status [port];
> @@ -300,16 +302,48 @@ static int ehci_bus_suspend (struct usb_
> }
>
> if (t1 != t2) {
> + /*
> + * Don't enable wake-on-disconnect until the bus has
> + * switched over to full-speed idle.
> + */
> + if ((t2 & PORT_WKDISC_E) &&
> + ehci_port_speed(ehci, t2) ==
> + USB_PORT_STAT_HIGH_SPEED) {
> + t2 &= ~PORT_WKDISC_E;
> + want_WKDISC |= (1 << port);
> + }
> ehci_writel(ehci, t2, reg);
> changed = 1;
> }
> }
> + spin_unlock_irq(&ehci->lock);
>
> - if (changed && ehci->has_tdi_phy_lpm) {
> - spin_unlock_irq(&ehci->lock);
> - msleep(5); /* 5 ms for HCD to enter low-power mode */
> + if ((changed && ehci->has_tdi_phy_lpm) || want_WKDISC) {
> + /*
> + * Wait for HCD to enter low-power mode or for the bus
> + * to switch to full-speed idle.
> + */
> + usleep_range(5000, 5500);
> + }
> +
> + if (want_WKDISC) {
> spin_lock_irq(&ehci->lock);
> + port = HCS_N_PORTS(ehci->hcs_params);
> + while (port--) {
> + u32 __iomem *reg;
> + u32 t2;
>
> + if (!(want_WKDISC & (1 << port)))
> + continue;
> + reg = &ehci->regs->port_status[port];
> + t2 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
Why it needs to re-do clear PORT_RWC_BITS?
> + ehci_writel(ehci, t2 | PORT_WKDISC_E, reg);
> + }
> + spin_unlock_irq(&ehci->lock);
> + }
> +
> + if (changed && ehci->has_tdi_phy_lpm) {
> + spin_lock_irq(&ehci->lock);
> port = HCS_N_PORTS(ehci->hcs_params);
> while (port--) {
> u32 __iomem *hostpc_reg = &ehci->regs->hostpc[port];
> @@ -322,8 +356,8 @@ static int ehci_bus_suspend (struct usb_
> port, (t3 & HOSTPC_PHCD) ?
> "succeeded" : "failed");
> }
> + spin_unlock_irq(&ehci->lock);
> }
> - spin_unlock_irq(&ehci->lock);
How about adding spin_lock_irq and spin_unlock_irq only one time,
I mean not add them into the two if () conditional statement.
>
> /* Apparently some devices need a >= 1-uframe delay here */
> if (ehci->bus_suspended)
>
>
>
--
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
