[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling
Stefan Roese
sr at denx.de
Mon Mar 14 14:19:23 CET 2016
Hi Hans,
On 14.03.2016 13:45, Hans de Goede wrote:
> On 14-03-16 11:18, Stefan Roese wrote:
>> This patch changes the USB port scanning procedure and timeout
>> handling in the following ways:
>>
>> a)
>> The power-on delay in usb_hub_power_on() is now reduced to a value of
>> max(100ms, "hub->desc.bPwrOn2PwrGood * 2"). The code does not wait
>> using mdelay in this usb_hub_power_on() will wait before querying
>> the device in the scanning loop later. The total timeout for this
>> hub, which is 1 second + "hub->desc.bPwrOn2PwrGood * 2" is calculated
>> and will be used in the following per-port scanning loop as the timeout
>> to detect active USB devices on this hub.
>>
>> b)
>> Don't delay the minimum delay (for power to stabalize) in
>> usb_hub_power_on(). Instead skip querying these devices in the scannig
>> loop until the delay time is reached.
>>
>> c)
>> The ports are now scanned in a quasy parallel way. The current code did
>> wait for each (unconnected) port to reach its timeout. And only then
>> continue with the next port. This patch now changes this to scan all
>> ports of all USB hubs quasi simultaniously. For this, all ports are added
>> to a scanning list. This list is scanned until all ports are ready
>> by either a) reaching the connection timeout (calculated earlier), or
>> by b) detecting a USB device. Resulting in a faster USB scan time as the
>> recursive scanning of USB hubs connected to the hub thats currently
>> being scanned will start earlier.
>>
>> Without this patch:
>> starting USB...
>> USB0: USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 20.163 seconds
>>
>> With this patch:
>> starting USB...
>> USB0: USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 1.822 seconds
>>
>> So ~18.3 seconds of USB scanning time reduction.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>>
>> ---
>>
>> Changes in v3:
>> - Introduced scanning list containing all USB devices of one USB
>> controller that need to get scanned
>> - Don't delay in usb_hub_power_on(). Instead skip querying these devices
>> until the delay time is reached.
>
> Oh I like :)
Thanks! :)
>
>>
>> Changes in v2:
>> - Remove static USB port configuration patch (for now)
>>
>> common/usb_hub.c | 289
>> +++++++++++++++++++++++++++++++++++++------------------
>> include/usb.h | 3 +
>> 2 files changed, 200 insertions(+), 92 deletions(-)
>>
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index d621f50..1167217 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -30,6 +30,7 @@
>> #include <asm/processor.h>
>> #include <asm/unaligned.h>
>> #include <linux/ctype.h>
>> +#include <linux/list.h>
>> #include <asm/byteorder.h>
>> #ifdef CONFIG_SANDBOX
>> #include <asm/state.h>
>> @@ -120,7 +121,21 @@ static void usb_hub_power_on(struct
>> usb_hub_device *hub)
>> pgood_delay = max(pgood_delay,
>> (unsigned)simple_strtol(env, NULL, 0));
>> debug("pgood_delay=%dms\n", pgood_delay);
>> - mdelay(pgood_delay + 1000);
>> +
>> + /*
>> + * Do a minimum delay of the larger value of 100ms or pgood_delay
>> + * so that the power can stablize before the devices are queried
>> + */
>> + dev->query_delay = get_timer(0) + max(100, (int)pgood_delay);
>> +
>> + /*
>> + * Record the power-on timeout here. The max. delay (timeout)
>> + * will be done based on this value in the USB port loop in
>> + * usb_hub_configure() later.
>> + */
>> + dev->connect_timeout = get_timer(0) + pgood_delay + 1000;
>> + debug("devnum=%d poweron: query_delay=%d connect_timeout=%d\n",
>> + dev->devnum, max(100, (int)pgood_delay), pgood_delay + 1000);
>> }
>>
>> void usb_hub_reset(void)
>> @@ -332,6 +347,161 @@ int usb_hub_port_connect_change(struct
>> usb_device *dev, int port)
>> return ret;
>> }
>>
>> +static LIST_HEAD(usb_scan_list);
>> +
>> +struct usb_device_scan {
>> + struct usb_device *dev; /* USB hub device to scan */
>> + struct usb_hub_device *hub; /* USB hub struct */
>> + int port; /* USB port to scan */
>> + struct list_head list;
>> +};
>> +
>> +static int usb_scan_port(struct usb_device_scan *usb_scan)
>> +{
>> + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>> + unsigned short portstatus;
>> + unsigned short portchange;
>> + struct usb_device *dev;
>> + struct usb_hub_device *hub;
>> + int ret = 0;
>> + int i;
>> +
>> + dev = usb_scan->dev;
>> + hub = usb_scan->hub;
>> + i = usb_scan->port;
>> +
>> +#ifdef CONFIG_DM_USB
>> + debug("\n\nScanning '%s' port %d\n", dev->dev->name, i + 1);
>> +#else
>> + debug("\n\nScanning port %d\n", i + 1);
>> +#endif
>> +
>> + ret = usb_get_port_status(dev, i + 1, portsts);
>> + if (ret < 0) {
>> + debug("get_port_status failed\n");
>> + return 0;
>> + }
>> +
>> + portstatus = le16_to_cpu(portsts->wPortStatus);
>> + portchange = le16_to_cpu(portsts->wPortChange);
>> +
>> + /* No connection change happened, wait a bit more. */
>> + if (!(portchange & USB_PORT_STAT_C_CONNECTION)) {
>> + if (get_timer(0) >= dev->connect_timeout) {
>> + debug("devnum=%d port=%d: timeout\n",
>> + dev->devnum, i + 1);
>> + /* Remove this device from scanning list */
>> + list_del(&usb_scan->list);
>> + return 0;
>> + }
>> + return 0;
>> + }
>> +
>> + /* Test if the connection came up, and if so, exit. */
>> + if (portstatus & USB_PORT_STAT_CONNECTION) {
>> + debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1);
>> + /* Remove this device from scanning list */
>> + list_del(&usb_scan->list);
>> +
>> + /* A new USB device is ready at this point */
>> +
>> + debug("Port %d Status %X Change %X\n",
>> + i + 1, portstatus, portchange);
>> +
>> + if (portchange & USB_PORT_STAT_C_CONNECTION) {
>> + debug("port %d connection change\n", i + 1);
>> + usb_hub_port_connect_change(dev, i);
>> + }
>> + if (portchange & USB_PORT_STAT_C_ENABLE) {
>> + debug("port %d enable change, status %x\n",
>> + i + 1, portstatus);
>> + usb_clear_port_feature(dev, i + 1,
>> + USB_PORT_FEAT_C_ENABLE);
>> + /*
>> + * The following hack causes a ghost device problem
>> + * to Faraday EHCI
>> + */
>> +#ifndef CONFIG_USB_EHCI_FARADAY
>> + /* EM interference sometimes causes bad shielded USB
>> + * devices to be shutdown by the hub, this hack enables
>> + * them again. Works at least with mouse driver */
>> + if (!(portstatus & USB_PORT_STAT_ENABLE) &&
>> + (portstatus & USB_PORT_STAT_CONNECTION) &&
>> + usb_device_has_child_on_port(dev, i)) {
>> + debug("already running port %i disabled by hub
>> (EMI?), re-enabling...\n",
>> + i + 1);
>> + usb_hub_port_connect_change(dev, i);
>> + }
>> +#endif
>> + }
>> + if (portstatus & USB_PORT_STAT_SUSPEND) {
>> + debug("port %d suspend change\n", i + 1);
>> + usb_clear_port_feature(dev, i + 1,
>> + USB_PORT_FEAT_SUSPEND);
>> + }
>> +
>> + if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
>> + debug("port %d over-current change\n", i + 1);
>> + usb_clear_port_feature(dev, i + 1,
>> + USB_PORT_FEAT_C_OVER_CURRENT);
>> + usb_hub_power_on(hub);
>> + }
>> +
>> + if (portchange & USB_PORT_STAT_C_RESET) {
>> + debug("port %d reset change\n", i + 1);
>> + usb_clear_port_feature(dev, i + 1,
>> + USB_PORT_FEAT_C_RESET);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int usb_device_list_scan(void)
>> +{
>> + struct usb_device_scan *usb_scan;
>> + struct usb_device_scan *tmp;
>> + static int running;
>> + int ret = 0;
>> +
>> + /* Only run this loop once for each controller */
>> + if (running)
>> + return 0;
>> +
>> + running = 1;
>> +
>> + while (1) {
>> + /* We're done, once the list is empty again */
>> + if (list_empty(&usb_scan_list))
>> + goto out;
>> +
>> + list_for_each_entry_safe(usb_scan, tmp, &usb_scan_list, list) {
>> + int ret;
>> +
>> + /*
>> + * Don't talk to the device before the query delay is
>> + * expired. This is needed for voltages to stabalize.
>> + */
>> + if (get_timer(0) < usb_scan->dev->query_delay)
>> + continue;
>> +
>
> I would prefer for this to be moved to usb_scan_port().
Yes, makes sense. I've moved it there for v4.
>> + /* Scan this port */
>> + ret = usb_scan_port(usb_scan);
>> + if (ret)
>> + goto out;
>> + }
>> + }
>> +
>> +out:
>> + /*
>> + * This USB controller has has finished scanning all its connected
>> + * USB devices. Set "running" back to 0, so that other USB
>> controllers
>> + * will scan their devices too.
>> + */
>> + running = 0;
>> +
>> + return ret;
>> +}
>>
>> static int usb_hub_configure(struct usb_device *dev)
>> {
>> @@ -466,104 +636,39 @@ static int usb_hub_configure(struct usb_device
>> *dev)
>> for (i = 0; i < dev->maxchild; i++)
>> usb_hub_reset_devices(i + 1);
>>
>> - for (i = 0; i < dev->maxchild; i++) {
>> - ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>> - unsigned short portstatus, portchange;
>> - int ret;
>> - ulong start = get_timer(0);
>> - uint delay = CONFIG_SYS_HZ;
>> -
>> #ifdef CONFIG_SANDBOX
>> - if (state_get_skip_delays())
>> - delay = 0;
>> -#endif
>> -#ifdef CONFIG_DM_USB
>> - debug("\n\nScanning '%s' port %d\n", dev->dev->name, i + 1);
>> -#else
>> - debug("\n\nScanning port %d\n", i + 1);
>> + if (state_get_skip_delays())
>> + dev->poweron_timeout = 0;
>> #endif
>> - /*
>> - * Wait for (whichever finishes first)
>> - * - A maximum of 10 seconds
>> - * This is a purely observational value driven by connecting
>> - * a few broken pen drives and taking the max * 1.5 approach
>> - * - connection_change and connection state to report same
>> - * state
>> - */
>> - do {
>> - ret = usb_get_port_status(dev, i + 1, portsts);
>> - if (ret < 0) {
>> - debug("get_port_status failed\n");
>> - break;
>> - }
>>
>> - portstatus = le16_to_cpu(portsts->wPortStatus);
>> - portchange = le16_to_cpu(portsts->wPortChange);
>> -
>> - /* No connection change happened, wait a bit more. */
>> - if (!(portchange & USB_PORT_STAT_C_CONNECTION))
>> - continue;
>> -
>> - /* Test if the connection came up, and if so, exit. */
>> - if (portstatus & USB_PORT_STAT_CONNECTION)
>> - break;
>> -
>> - } while (get_timer(start) < delay);
>> -
>> - if (ret < 0)
>> - continue;
>> -
>> - debug("Port %d Status %X Change %X\n",
>> - i + 1, portstatus, portchange);
>> -
>> - if (portchange & USB_PORT_STAT_C_CONNECTION) {
>> - debug("port %d connection change\n", i + 1);
>> - usb_hub_port_connect_change(dev, i);
>> - }
>> - if (portchange & USB_PORT_STAT_C_ENABLE) {
>> - debug("port %d enable change, status %x\n",
>> - i + 1, portstatus);
>> - usb_clear_port_feature(dev, i + 1,
>> - USB_PORT_FEAT_C_ENABLE);
>> - /*
>> - * The following hack causes a ghost device problem
>> - * to Faraday EHCI
>> - */
>> -#ifndef CONFIG_USB_EHCI_FARADAY
>> - /* EM interference sometimes causes bad shielded USB
>> - * devices to be shutdown by the hub, this hack enables
>> - * them again. Works at least with mouse driver */
>> - if (!(portstatus & USB_PORT_STAT_ENABLE) &&
>> - (portstatus & USB_PORT_STAT_CONNECTION) &&
>> - usb_device_has_child_on_port(dev, i)) {
>> - debug("already running port %i " \
>> - "disabled by hub (EMI?), " \
>> - "re-enabling...\n", i + 1);
>> - usb_hub_port_connect_change(dev, i);
>> - }
>> -#endif
>> - }
>> - if (portstatus & USB_PORT_STAT_SUSPEND) {
>> - debug("port %d suspend change\n", i + 1);
>> - usb_clear_port_feature(dev, i + 1,
>> - USB_PORT_FEAT_SUSPEND);
>> - }
>> + /*
>> + * Only add the connected USB devices, including potential hubs,
>> + * to a scanning list. This list will get scanned and devices that
>> + * are detected (either via port connected or via port timeout)
>> + * will get removed from this list. Scanning of the devices on this
>> + * list will continue until all devices are removed.
>> + */
>> + for (i = 0; i < dev->maxchild; i++) {
>> + struct usb_device_scan *usb_scan;
>>
>> - if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
>> - debug("port %d over-current change\n", i + 1);
>> - usb_clear_port_feature(dev, i + 1,
>> - USB_PORT_FEAT_C_OVER_CURRENT);
>> - usb_hub_power_on(hub);
>> + usb_scan = calloc(1, sizeof(*usb_scan));
>> + if (!usb_scan) {
>> + printf("Can't allocate memory for USB device!\n");
>> + return -ENOMEM;
>> }
>> + usb_scan->dev = dev;
>> + usb_scan->hub = hub;
>> + usb_scan->port = i;
>> + INIT_LIST_HEAD(&usb_scan->list);
>
> This (INIT_LIST_HEAD) is not necessary, as this is not the HEAD of a
> list, but just a list member, anything set here will immediately
> be overridden by:
>
>> + list_add_tail(&usb_scan->list, &usb_scan_list);
>
> This list_add_tail call.
Thanks. Fixed including the addition of some free() calls for v4.
I'm waiting for a few more review comments before sending this
out.
>> + }
>>
>> - if (portchange & USB_PORT_STAT_C_RESET) {
>> - debug("port %d reset change\n", i + 1);
>> - usb_clear_port_feature(dev, i + 1,
>> - USB_PORT_FEAT_C_RESET);
>> - }
>> - } /* end for i all ports */
>> + /*
>> + * And now call the scanning code which loops over the generated
>> list
>> + */
>> + ret = usb_device_list_scan();
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int usb_hub_check(struct usb_device *dev, int ifnum)
>> diff --git a/include/usb.h b/include/usb.h
>> index 0b410b6..0b57093 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -153,6 +153,9 @@ struct usb_device {
>> struct udevice *dev; /* Pointer to associated device */
>> struct udevice *controller_dev; /* Pointer to associated
>> controller */
>> #endif
>> +
>> + ulong connect_timeout; /* Device connection timeout in ms */
>> + ulong query_delay; /* Device query delay in ms */
>> };
>>
>> struct int_queue;
>>
>
> Otherwise I like this patch, in fact I like it a lot :)
Thanks. I'm also pretty satisfied with this version. The result
is astonishing fast - at least compared to the "old"
implementation. Thanks for all your very valuable suggestions
and comments.
> With the above things fixed you can add my:
>
> Acked-by: Hans de Goede <hdegoede at redhat.com>
>
> To it.
Will do.
> I guess this scratches your itch, so you're probably done with this, if
> not having parallel controller scanning too would be awesome for
> some boards I've which have up to 8 controllers (of which only 6
> are supporte d by u-boot atm, but that is a different issue).
My current platform only has 1 USB controller. So I'm probably
done with this optimization for now. Also, I think its good to get
this version integrated into mainline first, before changing
further things, like the parallel controller scanning. This can
always be done in some follow-up patches (my me or someone
else).
Thanks,
Stefan
More information about the U-Boot
mailing list