On Wed, Mar 03, 2010 at 06:11:30PM +0800, Libin wrote:
> >From c59d7180fc61161d5a14fc8e66e1524d91678b91 Mon Sep 17 00:00:00 2001
> From: Andiry Xu <andiry.xu@xxxxxxx>
> Date: Wed, 3 Mar 2010 16:47:04 +0800
> Subject: [PATCH 5/5] xHCI port PM: set U1/U2 timeout
>
> This patch sets U1/U2 timeout field of PORT PMSC when a device is addressed.
> Note that PORT PMSC has different meanings for USB2/USB3 protocol ports.
>
> Disable the actual setting by now, since some chips may have HW issues.
All hosts and devices must support U1/U2 to pass USB-IF certification.
If you have a specific host that doesn't support it (even if it's a
prototype), then skip setting the U1/U2 timeout based on that host's PCI
vendor and product ID. But let products that have passed certification
get better power management under Linux by setting the U1/U2 timeout.
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> Signed-off-by: Libin Yang <libin.yang@xxxxxxx>
> ---
> drivers/usb/host/xhci-hcd.c | 38 +++++++++++++++++++++++++++++++++++++-
> 1 files changed, 37 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hcd.c b/drivers/usb/host/xhci-hcd.c
> index adb29df..bf35c11 100644
> --- a/drivers/usb/host/xhci-hcd.c
> +++ b/drivers/usb/host/xhci-hcd.c
> @@ -1954,6 +1954,11 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> struct xhci_slot_ctx *slot_ctx;
> struct xhci_input_control_ctx *ctrl_ctx;
> u64 temp_64;
> +#if 0
> + u32 __iomem *addr;
> + u32 temp;
> + int ports, port;
> +#endif
>
> if (!udev->slot_id) {
> xhci_dbg(xhci, "Bad Slot ID %d\n", udev->slot_id);
> @@ -2046,7 +2051,38 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> xhci_dbg(xhci, "Device address = %d\n", udev->devnum);
> /* XXX Meh, not sure if anyone else but choose_address uses this. */
> set_bit(udev->devnum, udev->bus->devmap.devicemap);
> -
> +#if 0
> + /* Set PORT PMSC */
> + ports = HCS_MAX_PORTS(xhci->hcs_params1);
> + port = udev->portnum;
> + if ((port <= 0) || (port > ports)) {
> + xhci_warn(xhci, "Invalid port id %d\n", port);
> + return 0;
> + }
> + addr = &xhci->op_regs->port_power_base + NUM_PORT_REGS * (port - 1);
> + temp = xhci_readl(xhci, addr);
> +
> + switch (udev->speed) {
> + case USB_SPEED_SUPER:
> + /* Set U1/U2 timeout */
> + /* FIXME: how to decide appropriate value? */
Yes, that is the issue. You need to do more gathering of information,
which is why this should be in the USB core, not the xHCI driver. khubd
in the USB core should set the policy for U1 and U2 timeouts for all the
hubs, including the root hub.
You need to look at several factors when setting the U1/U2 timeout for a
device. Each device under a hub has a max exit latency for coming out
of U1 and U2. You don't want the hub to come out of U2 (to U0) and then
try to go back into U1 when the devices below it are still exiting U2.
So you have to propagate the highest exit latency up the device tree and
set the hub's timeouts based on that. Each hub also has a Hub Header
Decode Latency that you have to take into account.
You also need to take into account the periodic endpoints on all the
devices below the hub. If you're polling an endpoint at 1ms, but the
maximum exit latency for that path to the device is high (close to 1ms),
it may not save any power to ping-pong the link between U0 and U2.
All this makes it complicated enough to push into khubd. I have to NAK
this patch for now until there's a better solution. It's a moot point,
since the patch doesn't do anything.
> + temp |= PORT_U1_TIMEOUT(0x7f);
> + temp |= PORT_U2_TIMEOUT(0xff);
> + xhci_writel(xhci, temp, addr);
> + xhci_dbg(xhci, "Set SS port %d U1/U2 timeout\n", port);
> + break;
> + case USB_SPEED_HIGH:
> + case USB_SPEED_FULL:
> + case USB_SPEED_LOW:
> + /* Enable Remote Wakeup */
> + temp |= PORT_RWE;
> + xhci_writel(xhci, temp, addr);
> + xhci_dbg(xhci, "Enable HS port %d remote wakeup\n", port);
> + break;
Did you mean to say HS port here? This will fall through for FS and LS
ports too. Also, shouldn't enabling remote wakeup go into the port
remote wakeup patch (patch 2)? I don't understand what remote wakeup
has to do with link power management.
> + default:
> + break;
> + }
> +#endif
> return 0;
> }
>
> --
> 1.6.0.4
>
>
>
--
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
