[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling
Hans de Goede
hdegoede at redhat.com
Mon Mar 14 13:45:24 CET 2016
Hi,
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 :)
>
> 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().
> + /* 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.
> + }
>
> - 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 :)
With the above things fixed you can add my:
Acked-by: Hans de Goede <hdegoede at redhat.com>
To it.
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).
Regards,
Hans
More information about the U-Boot
mailing list