| Message ID | 20200103084008.3579-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] xhci: Ensure link state is U3 after setting USB_SS_PORT_LS_U3 | expand |
On 3.1.2020 10.40, Kai-Heng Feng wrote: > The xHCI spec doesn't specify the upper bound of U3 transition time. For > some devices 20ms is not enough, so we need to make sure the link state > is in U3 before further actions. Looking at the USB 3.2 spec (7.2.4.2 Link Power management and Flow) it seems most timers related to U3 entry are a lot less than a millisecond (4-16us) If port is in U1/U2, Ux_EXIT_TIMER is 6ms which seems to be the longest timeout. If we anyway are polling for the U3 state we could shorten the initial 20ms sleep down to 10ms. I think many devices, especially if they are already in U0 could manage this. are > > I've tried to use U3 Entry Capability by setting U3 Entry Enable in > config register, however the port change event for U3 transition > interrupts the system suspend process. > > For now let's use the less ideal method by polling PLS. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/usb/host/xhci-hub.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index 7a3a29e5e9d2..2b2e9d004dbf 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -1228,6 +1228,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > break; > case USB_PORT_FEAT_LINK_STATE: > temp = readl(ports[wIndex]->addr); > + xhci_dbg(xhci, "before setting link state, actual port %d-%d status = 0x%0x\n", hcd->self.busnum, wIndex + 1, temp); xhci_set_link_state() already shows similar debugging, > /* Disable port */ > if (link_state == USB_SS_PORT_LS_SS_DISABLED) { > xhci_dbg(xhci, "Disable port %d\n", wIndex); > @@ -1316,9 +1317,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > msleep(20); /* wait device to enter */ > spin_lock_irqsave(&xhci->lock, flags); > > - temp = readl(ports[wIndex]->addr); > - if (link_state == USB_SS_PORT_LS_U3) > + if (link_state == USB_SS_PORT_LS_U3) { > + retval = xhci_handshake(ports[wIndex]->addr, PORT_PLS_MASK, XDEV_U3, 80 * 1000); > + if (retval) > + xhci_dbg(xhci, "polling XDEV_U3 on port %d-%d timeout\n", hcd->self.busnum, wIndex + 1); In worst case we are busylooping for 80ms here, keeping the cpu busy. It should be ok to sleep here, so how about just reading the register every 10ms max 10 times, sleeping in between. -Mathias
On Fri, Jan 10, 2020 at 5:33 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 3.1.2020 10.40, Kai-Heng Feng wrote: > > The xHCI spec doesn't specify the upper bound of U3 transition time. For > > some devices 20ms is not enough, so we need to make sure the link state > > is in U3 before further actions. > > Looking at the USB 3.2 spec (7.2.4.2 Link Power management and Flow) it seems > most timers related to U3 entry are a lot less than a millisecond (4-16us) > If port is in U1/U2, Ux_EXIT_TIMER is 6ms which seems to be the longest timeout. > > If we anyway are polling for the U3 state we could shorten the initial 20ms sleep > down to 10ms. I think many devices, especially if they are already in U0 > could manage this. Ok. > > are > > > I've tried to use U3 Entry Capability by setting U3 Entry Enable in > > config register, however the port change event for U3 transition > > interrupts the system suspend process. > > > > For now let's use the less ideal method by polling PLS. > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/usb/host/xhci-hub.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > > index 7a3a29e5e9d2..2b2e9d004dbf 100644 > > --- a/drivers/usb/host/xhci-hub.c > > +++ b/drivers/usb/host/xhci-hub.c > > @@ -1228,6 +1228,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > > break; > > case USB_PORT_FEAT_LINK_STATE: > > temp = readl(ports[wIndex]->addr); > > + xhci_dbg(xhci, "before setting link state, actual port %d-%d status = 0x%0x\n", hcd->self.busnum, wIndex + 1, temp); > > xhci_set_link_state() already shows similar debugging, Ok, will remove it. > > > /* Disable port */ > > if (link_state == USB_SS_PORT_LS_SS_DISABLED) { > > xhci_dbg(xhci, "Disable port %d\n", wIndex); > > @@ -1316,9 +1317,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > > msleep(20); /* wait device to enter */ > > spin_lock_irqsave(&xhci->lock, flags); > > > > - temp = readl(ports[wIndex]->addr); > > - if (link_state == USB_SS_PORT_LS_U3) > > + if (link_state == USB_SS_PORT_LS_U3) { > > + retval = xhci_handshake(ports[wIndex]->addr, PORT_PLS_MASK, XDEV_U3, 80 * 1000); > > + if (retval) > > + xhci_dbg(xhci, "polling XDEV_U3 on port %d-%d timeout\n", hcd->self.busnum, wIndex + 1); > > In worst case we are busylooping for 80ms here, keeping the cpu busy. > It should be ok to sleep here, so how about just reading the register > every 10ms max 10 times, sleeping in between. Ok. Is the polling safe outside of spin_lock_irqsave()? Kai-Heng > > -Mathias
On 13.1.2020 11.10, Kai-Heng Feng wrote: > On Fri, Jan 10, 2020 at 5:33 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: >>> @@ -1316,9 +1317,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, >>> msleep(20); /* wait device to enter */ >>> spin_lock_irqsave(&xhci->lock, flags); >>> >>> - temp = readl(ports[wIndex]->addr); >>> - if (link_state == USB_SS_PORT_LS_U3) >>> + if (link_state == USB_SS_PORT_LS_U3) { >>> + retval = xhci_handshake(ports[wIndex]->addr, PORT_PLS_MASK, XDEV_U3, 80 * 1000); >>> + if (retval) >>> + xhci_dbg(xhci, "polling XDEV_U3 on port %d-%d timeout\n", hcd->self.busnum, wIndex + 1); >> >> In worst case we are busylooping for 80ms here, keeping the cpu busy. >> It should be ok to sleep here, so how about just reading the register >> every 10ms max 10 times, sleeping in between. > > Ok. Is the polling safe outside of spin_lock_irqsave()? > Should be, we only read one 32 bit register, and we anyway used to release and re-acquire the lock right before this anyway. -Mathias
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 7a3a29e5e9d2..2b2e9d004dbf 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -1228,6 +1228,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, break; case USB_PORT_FEAT_LINK_STATE: temp = readl(ports[wIndex]->addr); + xhci_dbg(xhci, "before setting link state, actual port %d-%d status = 0x%0x\n", hcd->self.busnum, wIndex + 1, temp); /* Disable port */ if (link_state == USB_SS_PORT_LS_SS_DISABLED) { xhci_dbg(xhci, "Disable port %d\n", wIndex); @@ -1316,9 +1317,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, msleep(20); /* wait device to enter */ spin_lock_irqsave(&xhci->lock, flags); - temp = readl(ports[wIndex]->addr); - if (link_state == USB_SS_PORT_LS_U3) + if (link_state == USB_SS_PORT_LS_U3) { + retval = xhci_handshake(ports[wIndex]->addr, PORT_PLS_MASK, XDEV_U3, 80 * 1000); + if (retval) + xhci_dbg(xhci, "polling XDEV_U3 on port %d-%d timeout\n", hcd->self.busnum, wIndex + 1); + bus_state->suspended_ports |= 1 << wIndex; + } + + temp = readl(ports[wIndex]->addr); + xhci_dbg(xhci, "after setting link state, port %d-%d status = 0x%0x\n", hcd->self.busnum, wIndex + 1, temp); + break; case USB_PORT_FEAT_POWER: /*
The xHCI spec doesn't specify the upper bound of U3 transition time. For some devices 20ms is not enough, so we need to make sure the link state is in U3 before further actions. I've tried to use U3 Entry Capability by setting U3 Entry Enable in config register, however the port change event for U3 transition interrupts the system suspend process. For now let's use the less ideal method by polling PLS. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/usb/host/xhci-hub.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)