Hi Andiry,
I'm trying to get the general overview of this patchset, so please
correct me if I have any assumptions wrong. AFAIK, the USB core doesn't
support software controlled USB 2.0 LPM. Unless the xHCI host
supports hardware controlled USB 2.0 LPM, the devices won't actually
enter U2, correct?
You say this patchset only currently supports devices attached directly
to the roothub. What's your plan for supporting devices behind hubs?
Does it make sense at that point to put the testing for LPM support in
khubd? The USB core can always keep track of the "bad" devices in a
list per host controller, in case someone has two xHCI host controllers,
and LPM works on some of them and not on others.
I think it might also make sense to move the clearing of the link state
into the USB core. External USB 3.0 hubs will send a port status change
event for link changes, and the USB core will need to handle that. I'm
not quite sure how to handle the fact that the xHCI driver will report
link status changes for USB 2.0 ports, when external USB 2.0 hubs won't.
Maybe it makes sense to clear the link state in the xHCI driver only for
USB 2.0 ports?
I have a couple specific questions on other patches, and I'll send those
separately.
Sarah Sharp
p.s. In general, can you please change this type of style:
if (condition) {
/* multi-line */
/* body */
} else
/* single-line body with no { } */
to include square brackets around the else body? I know it adds one
more line, but it makes it easier to identify corresponding if-else
blocks, and I'd like to stick with the current style in the driver.
On Tue, Jul 26, 2011 at 08:37:38AM +0800, Andiry Xu wrote:
> If the device pass the USB2 software LPM and the host supports hardware
> LPM, enable hardware LPM for the device to let the host decide when to
> put the link into lower power state.
>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-hub.c | 8 ++++--
> drivers/usb/host/xhci-ring.c | 6 +++++
> drivers/usb/host/xhci.c | 48 +++++++++++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci.h | 3 ++
> 4 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index f248933..12a6f32 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -583,9 +583,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> goto error;
> }
> /* unlock to execute stop endpoint commands */
> - spin_unlock_irqrestore(&xhci->lock, flags);
> - xhci_stop_device(xhci, slot_id, 1);
> - spin_lock_irqsave(&xhci->lock, flags);
> + if ((temp & PORT_PLS_MASK) == XDEV_U0) {
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + xhci_stop_device(xhci, slot_id, 1);
> + spin_lock_irqsave(&xhci->lock, flags);
> + }
>
> xhci_set_link_state(xhci, port_array, wIndex, XDEV_U3);
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d9d70b6..c108bc4 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1337,6 +1337,12 @@ static void handle_port_status(struct xhci_hcd *xhci,
> }
> }
>
> + /*
> + * If hardware LPM is enabled, host may put port into lower power
> + * state and set PLC. Clear it.
> + */
> + xhci_test_and_clear_bit(xhci, port_array, faked_port_index, PORT_PLC);
> +
> cleanup:
> /* Update event ring dequeue pointer before dropping the lock */
> inc_deq(xhci, xhci->event_ring, true);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index fb636ed..bb9f3ff 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -2628,6 +2628,9 @@ command_cleanup:
> return ret;
> }
>
> +static int xhci_set_hardware_lpm(struct xhci_hcd *xhci,
> + struct usb_device *udev, int enable);
> +
> /*
> * At this point, the struct usb_device is about to go away, the device has
> * disconnected, and all traffic has been stopped and the endpoints have been
> @@ -2654,6 +2657,13 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
> }
>
> spin_lock_irqsave(&xhci->lock, flags);
> +
> + if (virt_dev->hw_lpm_enabled) {
> + xhci_dbg(xhci, "disable hardware lpm\n");
> + xhci_set_hardware_lpm(xhci, udev, 0);
> + virt_dev->hw_lpm_enabled = 0;
> + }
> +
> /* Don't disable the slot if the host controller is dead. */
> state = xhci_readl(xhci, &xhci->op_regs->status);
> if (state == 0xffffffff || (xhci->xhc_state & XHCI_STATE_DYING)) {
> @@ -3004,17 +3014,53 @@ static int xhci_usb2_software_lpm_test(struct xhci_hcd *xhci,
> return ret;
> }
>
> +static int xhci_set_hardware_lpm(struct xhci_hcd *xhci,
> + struct usb_device *udev, int enable)
> +{
> + __le32 __iomem **port_array;
> + __le32 __iomem *pm_addr;
> + u32 temp;
> + unsigned int port_num;
> +
> + port_array = xhci->usb2_ports;
> + port_num = udev->portnum - 1;
> + pm_addr = port_array[port_num] + 1;
> + temp = xhci_readl(xhci, pm_addr);
> +
> + if (enable) {
> + temp &= ~PORT_HIRD_MASK;
> + temp |= PORT_HIRD(xhci->hird) | PORT_RWE;
> + xhci_writel(xhci, temp, pm_addr);
> + temp = xhci_readl(xhci, pm_addr);
> + temp |= PORT_HLE;
> + xhci_writel(xhci, temp, pm_addr);
> + } else {
> + temp &= ~(PORT_HLE | PORT_RWE | PORT_HIRD_MASK);
> + xhci_writel(xhci, temp, pm_addr);
> + }
> +
> + return 0;
> +}
> +
> int xhci_update_device(struct usb_hcd *hcd, struct usb_device *udev)
> {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + struct xhci_virt_device *virt_dev;
> unsigned long flags;
> int ret;
>
> spin_lock_irqsave(&xhci->lock, flags);
>
> ret = xhci_usb2_software_lpm_test(xhci, udev, flags);
> - if (!ret)
> + if (!ret) {
> xhci_dbg(xhci, "software LPM test succeed\n");
> + if (xhci->hw_lpm_support) {
> + xhci_dbg(xhci, "enable hardware LPM\n");
> + xhci_set_hardware_lpm(xhci, udev, 1);
> + virt_dev = xhci->devs[udev->slot_id];
> + virt_dev->hw_lpm_enabled = 1;
> + }
> + }
>
> spin_unlock_irqrestore(&xhci->lock, flags);
> return 0;
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index bbaeddb..a40f278 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -367,7 +367,9 @@ struct xhci_op_regs {
> #define PORT_L1S_SUCCESS 1
> #define PORT_RWE (1 << 3)
> #define PORT_HIRD(p) (((p) & 0xf) << 4)
> +#define PORT_HIRD_MASK (0xf << 4)
> #define PORT_L1DS(p) (((p) & 0xff) << 8)
> +#define PORT_HLE (1 << 16)
>
> /**
> * struct xhci_intr_reg - Interrupt Register Set
> @@ -804,6 +806,7 @@ struct xhci_virt_device {
> u32 cmd_status;
> struct list_head cmd_list;
> u8 port;
> + unsigned hw_lpm_enabled:1;
> };
>
>
> --
> 1.7.1
>
>
--
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
