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@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;
/** 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@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