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?
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.
> 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.
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.
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.
> >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.
> +{
> + 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?
Sarah Sharp
--
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
