> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Thursday, July 07, 2011 12:35 AM > To: Xu, Andiry > Cc: stern@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; He, Alex; > Nguyen, Dong; Alek Du; Jacob Pan > Subject: Re: Question about xHCI USB2 LPM > > On Wed, Jul 06, 2011 at 06:27:59PM +0800, Andiry Xu wrote: > > > > Hi Sarah, > > > > I met some issues when trying to implement xHCI 1.0 hardware LPM for > > USB2. I think that before setting the hardware LPM, we need to make > sure > > the device supports LPM, so a test of software LPM is necessary > before > > enable hardware LPM. > > Can you do that safely, before the driver(s) are bound to the > interfaces? Also, do we know if certain devices can't handle LPM > commands during certain activities? Could your test succeed, and then > the device disconnects when asked to go into L1 during, say, a disk > spin > down? Does the EHCI driver do a similar test? > What's the safe test time? Currently it's checked at the end of hub_port_init(), after the device is addressed. EHCI driver checks LPM at this point. > Perhaps we need only enable LPM if userspace allows it, using the > 'control' file in sysfs? Do the devices that don't support LPM also > fail to come back from device suspend (U3)? If some devices support U3 > while not supporting LPM, we would need a separate file. > L1 is an optional feature for USB2 devices. Devices should support U3(L2) but not forced to support L1. Software LPM may be enabled only if userspace allows it. But if the host and device support LPM, I think we can enable hardware LPM for the host, and let the host decide when to put device into L1. > > The patch below is to test software LPM. The logic > > is mainly the same as commit 48f24970144479c29b8cee6d2e1dbedf6dcf9cfb: > > USB: EHCI: EHCI 1.1 addendum: Basic LPM feature support. > > > > However, the test patch failed on some host controllers which claim > to > > support software LPM. With a LPM-capable device, the L1 status > reports > > 0(Invalid) or 4(Timeout/Error). Maybe something is wrong with the > test > > patch? Can you give some hint? > > Have you tried several different types of xHCI hosts? Or contacted the > company reps to ask them if they actually support LPM? I know it was a > requirement for hosts to pass LPM certification tests, but who knows if > the manufacturers left the logic in. > I'll have a double check. I know that the host I used passed USB-IF LPM test on Windows, so it should support LPM. Maybe I need to figure out the Exactly steps of the USB-IF LPM test. > Have you tried the same device under an LPM capable EHCI host to make > sure it actually supports LPM? I know devices have to advertise LPM > in their descriptors, but it would be good to double check. Do you > check the USB 2.0 extension descriptor before trying to enable LPM for > a > device? It should be easy to dig out of the extra descriptors, or have > the USB core track which devices can do LPM and set a flag in the > usb_device structure. > Yes, that's a good idea. I'll try to get BOS and extra descriptors during device enumeration, when getting device descriptors. EHCI LPM driver tests LPM without checking the descriptors. > I looked at the patch (couple comments below), but I'm not familiar > enough with the L1 process to know if it's correct. I think Jacob Pan > and Alex Du have worked on USB 2.0 LPM, so maybe they have some ideas? > > > Another question is raised when test with a none-LPM-capable device. > > When driver sets the port link state to U2, the xHC reports L1 status > > as Timeout/Error. In this case a port status change event is > occurred, > > and the PORT SC is 0x4206e1. Note that port is disabled(xHCI spec > only > > states PLC set in this case, not disable the port. I don't know if > the > > behavior is right), and the device failed to response to any > following > > commands, usbcore will disconnect this device and enumerate it again. > > This time we must skip the LPM test, or it will cycle the > > disconnect-enumeration again and again. However, from the driver side, > > it's a whole new device with different slot id and we don't know if > the > > device has been tested LPM before. Do you have some idea for this? > > Can the USB core or xHCI perhaps dynamically keep track of which > devices > it has run the LPM test on by storing vendor and device IDs? If we get > past the enumeration stage, to the point where the first configuration > is being added, the USB core has the descriptors. You could run your > LPM test there, but only if the vendor/device ID isn't in the table. > Hmm, I'll have a try. > > >From e79fe9cdb2ec1abc8a3fa882fc9a064f7be7bf7b Mon Sep 17 00:00:00 > 2001 > > From: Andiry Xu <andiry.xu@xxxxxxx> > > Date: Wed, 6 Jul 2011 16:00:12 +0800 > > Subject: [PATCH] xHCI: software lpm test > > > > --- > > drivers/usb/host/xhci-ext-caps.h | 6 ++++ > > drivers/usb/host/xhci-hub.c | 2 +- > > drivers/usb/host/xhci-mem.c | 17 ++++++++++ > > drivers/usb/host/xhci-pci.c | 4 ++ > > drivers/usb/host/xhci.c | 61 > ++++++++++++++++++++++++++++++++++++++ > > drivers/usb/host/xhci.h | 12 +++++++- > > 6 files changed, 100 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/host/xhci-ext-caps.h > b/drivers/usb/host/xhci-ext-caps.h > > index ce5c9e5..cc040b4 100644 > > --- a/drivers/usb/host/xhci-ext-caps.h > > +++ b/drivers/usb/host/xhci-ext-caps.h > > @@ -65,6 +65,12 @@ > > /* bits 1:2, 5:12, and 17:19 need to be preserved; bits 21:28 should > be zero */ > > #define XHCI_LEGACY_DISABLE_SMI ((0x3 << 1) + (0xff << 5) + > (0x7 << 17)) > > > > +/* USB 2.0 xHCI 0.96 L1C capability - section 7.2.2.1.3.2 */ > > +#define XHCI_L1C (1 << 16) > > + > > +/* USB 2.0 xHCI 1.0 hardware LMP capability - section 7.2.2.1.3.2 */ > > +#define XHCI_HLC (1 << 19) > > + > > /* command register values to disable interrupts and halt the HC */ > > /* start/stop HC execution - do not write unless HC is halted*/ > > #define XHCI_CMD_RUN (1 << 0) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci- > hub.c > > index 0be788c..c0f8bb3 100644 > > --- a/drivers/usb/host/xhci-hub.c > > +++ b/drivers/usb/host/xhci-hub.c > > @@ -247,7 +247,7 @@ int xhci_find_slot_id_by_port(struct usb_hcd *hcd, > struct xhci_hcd *xhci, > > * to complete. > > * suspend will set to 1, if suspend bit need to set in command. > > */ > > -static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int > suspend) > > +int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) > > { > > struct xhci_virt_device *virt_dev; > > struct xhci_command *cmd; > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci- > mem.c > > index fcb7f7e..15da2ff 100644 > > --- a/drivers/usb/host/xhci-mem.c > > +++ b/drivers/usb/host/xhci-mem.c > > @@ -1758,10 +1758,27 @@ static void xhci_add_in_port(struct xhci_hcd > *xhci, unsigned int num_ports, > > xhci_dbg(xhci, "Ext Cap %p, port offset = %u, " > > "count = %u, revision = 0x%x\n", > > addr, port_offset, port_count, major_revision); > > + > > /* Port count includes the current port offset */ > > if (port_offset == 0 || (port_offset + port_count - 1) > > num_ports) > > /* WTF? "Valid values are ‘1’ to MaxPorts" */ > > return; > > + > > + if ((xhci->hci_version == 0x96) && (major_revision != 0x03) && > > + (temp & XHCI_L1C)) { > > + xhci_dbg(xhci, "xHCI 0.96: support software lpm\n"); > > + xhci->sw_lpm_support = 1; > > + } > > + > > + if ((xhci->hci_version >= 0x100) && (major_revision != 0x03)) { > > + xhci_dbg(xhci, "xHCI 1.0: support software lpm\n"); > > + xhci->sw_lpm_support = 1; > > + if (temp & XHCI_HLC) { > > + xhci_dbg(xhci, "xHCI 1.0: support hardware lpm\n"); > > + xhci->hw_lpm_support = 1; > > + } > > + } > > + > > port_offset--; > > for (i = port_offset; i < (port_offset + port_count); i++) { > > /* Duplicate entry. Ignore the port if the revisions > differ. */ > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci- > pci.c > > index cb16de2..1516fe5 100644 > > --- a/drivers/usb/host/xhci-pci.c > > +++ b/drivers/usb/host/xhci-pci.c > > @@ -344,6 +344,10 @@ static const struct hc_driver xhci_pci_hc_driver > = { > > .hub_status_data = xhci_hub_status_data, > > .bus_suspend = xhci_bus_suspend, > > .bus_resume = xhci_bus_resume, > > + /* > > + * call back when device connected and addressed > > + */ > > + .update_device = xhci_update_device, > > }; > > > > /*------------------------------------------------------------------ > -------*/ > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index f5fe1ac..6af9fbd 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -2930,6 +2930,67 @@ int xhci_address_device(struct usb_hcd *hcd, > struct usb_device *udev) > > return 0; > > } > > > > +int xhci_update_device(struct usb_hcd *hcd, struct usb_device *udev) > > You probably want to call this something other than xhci_update_device, > since you're not just updating internal xHCI structures, you're > potentially disabling the device with that broken host controller. > Maybe call it xhci_test_device_lpm? Later we might need to add a test > for USB 3.0 devices, since I know some vendors have been passing LPM > certification tests and then turning LPM off. > Update_device is a hc_driver function. Basically it comes from EHCI LPM driver so I used the same hook. Yes, I will write a single Function xhci_test_device_pm() and call it in xhci_update_device(). > > +{ > > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > + __le32 __iomem **port_array; > > + __le32 __iomem *addr, *pm_addr; > > + u32 temp; > > + unsigned int port_num; > > + int ret; > > + > > + if (hcd->speed == HCD_USB3 || !xhci->sw_lpm_support) > > + return 0; > > + > > + /* we only support lpm device connected to root hub yet */ > > + if (!udev->parent || udev->parent->parent) > > + return 0; > > + > > + port_array = xhci->usb2_ports; > > + port_num = udev->portnum - 1; > > + > > + if (port_num > HCS_MAX_PORTS(xhci->hcs_params1)) { > > + xhci_dbg(xhci, "invalid port number %d\n", udev->portnum); > > + return -EINVAL; > > + } > > + > > + /* Test USB 2.0 software LPM */ > > + xhci_stop_device(xhci, udev->slot_id, 1); > > + > > + addr = port_array[port_num]; > > + temp = xhci_readl(xhci, addr); > > + xhci_dbg(xhci, "set software lpm for port %d\n", port_num); > > + temp = xhci_port_state_to_neutral(temp); > > + temp &= ~PORT_PLS_MASK; > > + temp |= PORT_LINK_STROBE | XDEV_U2; > > + xhci_writel(xhci, temp, addr); > > + temp = xhci_readl(xhci, addr); > > + /* wait for ACK */ > > + msleep(20); > > + > > + pm_addr = port_array[port_num] + 1; > > + ret = handshake(xhci, pm_addr, PORT_L1S_MASK, PORT_L1S_SUCCESS, > 125); > > + if (ret != -ETIMEDOUT) { > > + xhci_dbg(xhci, "software lpm succeed\n"); > > + temp = xhci_readl(xhci, addr); > > + xhci_dbg(xhci, "resume port %d from L1 state\n", port_num); > > + temp = xhci_port_state_to_neutral(temp); > > + temp &= ~PORT_PLS_MASK; > > + temp |= PORT_LINK_STROBE | XDEV_U0; > > + xhci_writel(xhci, temp, addr); > > + temp = xhci_readl(xhci, addr); > > + msleep(20); > > + } else { > > + temp = xhci_readl(xhci, pm_addr); > > + xhci_dbg(xhci, "software lpm failed, L1 status %d\n", > > + temp & PORT_L1S_MASK); > > + } > > + > > + xhci_ring_device(xhci, udev->slot_id); > > + > > + return 0; > > +} > > + > > /* Once a hub descriptor is fetched for a device, we need to update > the xHC's > > * internal data structures for the device. > > */ > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > > index d8bbf5c..db0a032 100644 > > --- a/drivers/usb/host/xhci.h > > +++ b/drivers/usb/host/xhci.h > > @@ -272,6 +272,7 @@ struct xhci_op_regs { > > */ > > #define PORT_PLS_MASK (0xf << 5) > > #define XDEV_U0 (0x0 << 5) > > +#define XDEV_U2 (0x2 << 5) > > #define XDEV_U3 (0x3 << 5) > > #define XDEV_RESUME (0xf << 5) > > /* true: port has power (see HCC_PPC) */ > > @@ -362,7 +363,10 @@ struct xhci_op_regs { > > /* Bits 24:31 for port testing */ > > > > /* USB2 Protocol PORTSPMSC */ > > -#define PORT_RWE (1 << 0x3) > > +#define PORT_L1S_MASK 7 > > +#define PORT_L1S_SUCCESS 1 > > +#define PORT_RWE (1 << 0x3) > > +#define PORT_HLE (1 << 16) > > > > /** > > * struct xhci_intr_reg - Interrupt Register Set > > @@ -1323,6 +1327,10 @@ struct xhci_hcd { > > /* Array of pointers to USB 2.0 PORTSC registers */ > > __le32 __iomem **usb2_ports; > > unsigned int num_usb2_ports; > > + /* support xHCI 0.96 spec software LPM */ > > + unsigned sw_lpm_support:1; > > + /* support xHCI 1.0 spec hardware LPM */ > > + unsigned hw_lpm_support:1; > > }; > > > > /* convert between an HCD pointer and the corresponding EHCI_HCD */ > > @@ -1507,6 +1515,7 @@ int xhci_free_streams(struct usb_hcd *hcd, > struct usb_device *udev, > > struct usb_host_endpoint **eps, unsigned int num_eps, > > gfp_t mem_flags); > > int xhci_address_device(struct usb_hcd *hcd, struct usb_device > *udev); > > +int xhci_update_device(struct usb_hcd *hcd, struct usb_device *udev); > > int xhci_update_hub_device(struct usb_hcd *hcd, struct usb_device > *hdev, > > struct usb_tt *tt, gfp_t mem_flags); > > int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t > mem_flags); > > @@ -1565,6 +1574,7 @@ void xhci_ring_ep_doorbell(struct xhci_hcd > *xhci, unsigned int slot_id, > > unsigned int ep_index, unsigned int stream_id); > > > > /* xHCI roothub code */ > > +int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int > suspend); > > int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > u16 wIndex, > > char *buf, u16 wLength); > > int xhci_hub_status_data(struct usb_hcd *hcd, char *buf); > > -- > > 1.7.1 > > > > > > > > You're missing the USB core code that calls xhci_update_device. Where > are you testing LPM? > The update_device calling is added by EHCI LPM driver. It's called at the end of hub_port_init(), after the device is addressed. Thanks, Andiry ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
