[U-Boot] [PATCH v5 4/4] usb: Change power-on / scanning timeout handling
Hans de Goede
hdegoede at redhat.com
Tue Mar 15 14:07:21 CET 2016
Hi,
On 15-03-16 13:59, 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, instead 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 stabilize) 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 quasi 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 simultaneously. 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. This results in a faster USB scan time as
> the recursive scanning of USB hubs connected to the hub that's currently
> being scanned will start earlier.
>
> One small functional change to the original code is, that ports with
> overcurrent detection will now get rescanned multiple times
> (PORT_OVERCURRENT_MAX_SCAN_COUNT).
>
> 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>
> Acked-by: Hans de Goede <hdegoede at redhat.com>
> Tested-by: Stephen Warren <swarren at nvidia.com>
>
> ---
>
> Changes in v5:
> - Removed superfluous debug output from usb_scan_port()
> - Replaced usb_hub_power_on() with usb_set_port_feature() in case
> of overcurrent detection. This is whats really needed here.
> - Added a per-port overcurrent counter and stop re-scanning of
> a device that exceeds the PORT_OVERCURRENT_MAX_SCAN_COUNT
> - Moved list_del() to end of loop in usb_scan_port()
> - Moved timeout / delay variables from USB dev to hub struct
> as they are hub specific
> - Moved state_get_skip_delays() to usb_hub_power_on() so that
> the timeouts are not set there
Looks good to me, my Acked-by still stands. Thanks for doing a v5.
Regards,
Hans
> Changes in v4:
> - Moved check for query_delay into usb_scan_port() as suggested by Hans
> - Correct list handling (drop INIT_LIST_HEAD)
> - Added some missing free() calls
> - Changed connect_timeout calculation as suggested by Stephen
> - Moved usb_scan_list to other globals to be cleaned up in a later patch
> - Added timeout check for non-functional ports (usb_get_port_status
> return error
> - Reverted if logic in loop to remove an indentation level
> - Moved debug() output
> - Removed unnecessary if when already connected
> - Added Hans's Acked-by
> - Added Stephen's Tested-by
> - Minor rewording / fixes of the commit text
>
> 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.
>
> Changes in v2:
> - Remove static USB port configuration patch (for now)
>
> common/usb_hub.c | 317 ++++++++++++++++++++++++++++++++++++++-----------------
> include/usb.h | 4 +
> 2 files changed, 227 insertions(+), 94 deletions(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index d621f50..e6a2cdb 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>
> @@ -49,9 +50,19 @@ DECLARE_GLOBAL_DATA_PTR;
> #define HUB_SHORT_RESET_TIME 20
> #define HUB_LONG_RESET_TIME 200
>
> +#define PORT_OVERCURRENT_MAX_SCAN_COUNT 3
> +
> +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;
> +};
> +
> /* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
> static struct usb_hub_device hub_dev[USB_MAX_HUB];
> static int usb_hub_index;
> +static LIST_HEAD(usb_scan_list);
>
> __weak void usb_hub_reset_devices(int port)
> {
> @@ -109,6 +120,15 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
> debug("port %d returns %lX\n", i + 1, dev->status);
> }
>
> +#ifdef CONFIG_SANDBOX
> + /*
> + * Don't set timeout / delay values here. This results
> + * in these values still being reset to 0.
> + */
> + if (state_get_skip_delays())
> + return;
> +#endif
> +
> /*
> * Wait for power to become stable,
> * plus spec-defined max time for device to connect
> @@ -120,12 +140,30 @@ 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
> + */
> + hub->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.
> + */
> + hub->connect_timeout = hub->query_delay + 1000;
> + debug("devnum=%d poweron: query_delay=%d connect_timeout=%d\n",
> + dev->devnum, max(100, (int)pgood_delay),
> + max(100, (int)pgood_delay) + 1000);
> }
>
> void usb_hub_reset(void)
> {
> usb_hub_index = 0;
> +
> + /* Zero out global hub_dev in case its re-used again */
> + memset(hub_dev, 0, sizeof(hub_dev));
> }
>
> static struct usb_hub_device *usb_hub_allocate(void)
> @@ -332,6 +370,168 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
> return ret;
> }
>
> +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;
> +
> + /*
> + * Don't talk to the device before the query delay is expired.
> + * This is needed for voltages to stabalize.
> + */
> + if (get_timer(0) < hub->query_delay)
> + return 0;
> +
> + ret = usb_get_port_status(dev, i + 1, portsts);
> + if (ret < 0) {
> + debug("get_port_status failed\n");
> + if (get_timer(0) >= hub->connect_timeout) {
> + debug("devnum=%d port=%d: timeout\n",
> + dev->devnum, i + 1);
> + /* Remove this device from scanning list */
> + list_del(&usb_scan->list);
> + free(usb_scan);
> + return 0;
> + }
> + }
> +
> + portstatus = le16_to_cpu(portsts->wPortStatus);
> + portchange = le16_to_cpu(portsts->wPortChange);
> + debug("Port %d Status %X Change %X\n", i + 1, portstatus, portchange);
> +
> + /* No connection change happened, wait a bit more. */
> + if (!(portchange & USB_PORT_STAT_C_CONNECTION)) {
> + if (get_timer(0) >= hub->connect_timeout) {
> + debug("devnum=%d port=%d: timeout\n",
> + dev->devnum, i + 1);
> + /* Remove this device from scanning list */
> + list_del(&usb_scan->list);
> + free(usb_scan);
> + return 0;
> + }
> + return 0;
> + }
> +
> + /* Test if the connection came up, and if not exit */
> + if (!(portstatus & USB_PORT_STAT_CONNECTION))
> + return 0;
> +
> + /* A new USB device is ready at this point */
> + debug("devnum=%d port=%d: USB dev found\n", dev->devnum, 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);
> + /* Only power-on this one port */
> + usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
> + hub->overcurrent_count[i]++;
> +
> + /*
> + * If the max-scan-count is not reached, return without removing
> + * the device from scan-list. This will re-issue a new scan.
> + */
> + if (hub->overcurrent_count[i] <=
> + PORT_OVERCURRENT_MAX_SCAN_COUNT)
> + return 0;
> +
> + /* Otherwise the device will get removed */
> + printf("Port %d over-current occured %d times\n", i + 1,
> + hub->overcurrent_count[i]);
> + }
> +
> + 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);
> + }
> +
> + /*
> + * We're done with this device, so let's remove this device from
> + * scanning list
> + */
> + list_del(&usb_scan->list);
> + free(usb_scan);
> +
> + 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;
> +
> + /* Scan this port */
> + ret = usb_scan_port(usb_scan);
> + if (ret)
> + goto out;
> + }
> + }
> +
> +out:
> + /*
> + * This USB controller 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 +666,33 @@ static int usb_hub_configure(struct usb_device *dev)
> for (i = 0; i < dev->maxchild; i++)
> usb_hub_reset_devices(i + 1);
>
> + /*
> + * 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++) {
> - 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);
> -#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;
> + struct usb_device_scan *usb_scan;
>
> - 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);
> + 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;
> + list_add_tail(&usb_scan->list, &usb_scan_list);
> + }
>
> - 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..7a82fb2 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -556,6 +556,10 @@ struct usb_hub_descriptor {
> struct usb_hub_device {
> struct usb_device *pusb_dev;
> struct usb_hub_descriptor desc;
> +
> + ulong connect_timeout; /* Device connection timeout in ms */
> + ulong query_delay; /* Device query delay in ms */
> + int overcurrent_count[USB_MAXCHILDREN]; /* Over-current counter */
> };
>
> #ifdef CONFIG_DM_USB
>
More information about the U-Boot
mailing list