[U-Boot] [PATCH v3 0/4] usb: Reduce USB scanning time

[U-Boot] [PATCH v3 0/4] usb: Reduce USB scanning time

[Stefan Roese]

My current x86 platform (Bay Trail, not in mainline yet) has a quite
complex USB infrastructure with many USB hubs. Here the USB scan takes
an incredible huge amount of time:

starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 28.415 seconds

This is of course not acceptable on platforms, where USB needs to get
scanned at every bootup. As this increases the bootup time of this
device by nearly 30 seconds!

This patch series greatly reduces the USB scanning time. This is done
by multiple means:

- Remove or reduce delays and timeouts
- Remove a 2nd reset of the USB hubs
- Change USB port timeout handling and introduce quasi parallel USB
  port scanning

As a result, the USB scanning time is greatly reduced:

starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 1.822 seconds

As you can see, the time is reduced from 28.4 to 1.8 seconds!

Please find more details to the changes in the patch description.

Testing and comments welcome!

Thanks,
Stefan

Changes in v3:
- Changed small timeout from 10ms to 20ms as this results in a
  much faster USB scanning time (10ms too small and 20ms enough
  in many cases)
- 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:
- Add Acked-by / Tested-by from Hans and Stephen
- Make this change unconditional
- Add Acked-by / Tested-by from Hans and Stephen
- Make this change unconditional
- Add Tested-by from Stephen
- Remove static USB port configuration patch (for now)

Stefan Roese (4):
  usb: legacy_hub_port_reset(): Speedup hub reset handling
  usb: Remove 200 ms delay in usb_hub_port_connect_change()
  usb: Don't reset the USB hub a 2nd time
  usb: Change power-on / scanning timeout handling

 common/usb.c     |  13 +--
 common/usb_hub.c | 301 +++++++++++++++++++++++++++++++++++++------------------
 include/usb.h    |   3 +
 3 files changed, 208 insertions(+), 109 deletions(-)

-- 
2.7.3

[U-Boot] [PATCH v3 1/4] usb: legacy_hub_port_reset(): Speedup hub reset handling

[Stefan Roese]

Start with a short USB hub reset delay of 20ms. This can be enough for
some configurations.

The 2nd delay at the of the loop is completely removed. Since the delay
hasn't been long enough, a longer delay time of 200ms is assigned. And
will be used in the next loop round.

This hub reset handling is also used in the v4.4 Linux USB driver,
hub_port_reset().

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
Cc: Marek Vasut <marex@denx.de>

---

Changes in v3:
- Changed small timeout from 10ms to 20ms as this results in a
  much faster USB scanning time (10ms too small and 20ms enough
  in many cases)

Changes in v2:
- Add Acked-by / Tested-by from Hans and Stephen

 common/usb_hub.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index e1de813..2089e20 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -46,6 +46,9 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define USB_BUFSIZ	512
 
+#define HUB_SHORT_RESET_TIME	20
+#define HUB_LONG_RESET_TIME	200
+
 /* 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;
@@ -164,6 +167,7 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
 	int err, tries;
 	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 	unsigned short portstatus, portchange;
+	int delay = HUB_SHORT_RESET_TIME; /* start with short reset delay */
 
 #ifdef CONFIG_DM_USB
 	debug("%s: resetting '%s' port %d...\n", __func__, dev->dev->name,
@@ -176,7 +180,7 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
 		if (err < 0)
 			return err;
 
-		mdelay(200);
+		mdelay(delay);
 
 		if (usb_get_port_status(dev, port + 1, portsts) < 0) {
 			debug("get_port_status failed status %lX\n",
@@ -215,7 +219,8 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
 		if (portstatus & USB_PORT_STAT_ENABLE)
 			break;
 
-		mdelay(200);
+		/* Switch to long reset delay for the next round */
+		delay = HUB_LONG_RESET_TIME;
 	}
 
 	if (tries == MAX_TRIES) {
-- 
2.7.3

[U-Boot] [PATCH v3 2/4] usb: Remove 200 ms delay in usb_hub_port_connect_change()

[Stefan Roese]

This patch removes 2 mdelay(200) calls from usb_hub_port_connect_change().
These delays don't seem to be necessary. At least not in my tests. Here
the number for a custom x86 Bay Trail board (not in mainline yet) with
a quite large and complex USB hub infrastructure.

Without this patch:
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 28.415 seconds

With this patch:
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 24.003 seconds

So ~4.5 seconds of USB scanning time reduction.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Marek Vasut <marex@denx.de>

---

Changes in v3: None
Changes in v2:
- Make this change unconditional
- Add Acked-by / Tested-by from Hans and Stephen

 common/usb_hub.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 2089e20..d621f50 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -275,7 +275,6 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
 		if (!(portstatus & USB_PORT_STAT_CONNECTION))
 			return -ENOTCONN;
 	}
-	mdelay(200);
 
 	/* Reset the port */
 	ret = legacy_hub_port_reset(dev, port, &portstatus);
@@ -285,8 +284,6 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
 		return ret;
 	}
 
-	mdelay(200);
-
 	switch (portstatus & USB_PORT_STAT_SPEED_MASK) {
 	case USB_PORT_STAT_SUPER_SPEED:
 		speed = USB_SPEED_SUPER;
-- 
2.7.3

[U-Boot] [PATCH v3 3/4] usb: Don't reset the USB hub a 2nd time

[Stefan Roese]

Debugging has shown, that all USB hubs are being resetted twice while
USB scanning. This introduces additional delays and makes USB scanning
even more slow. Testing has shown that this 2nd USB hub reset doesn't
seem to be necessary.

This patch now removes this 2nd USB hub reset. Resulting in faster USB
scan time. Here the current numbers:

Without this patch:
=> time usb start
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 24.003 seconds

With this patch:
=> time usb start
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 20.392 seconds

So ~3.6 seconds of USB scanning time reduction.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
Cc: Marek Vasut <marex@denx.de>

---

Changes in v3: None
Changes in v2:
- Make this change unconditional
- Add Tested-by from Stephen

 common/usb.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index c7b8b0e..45a5a0f 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -919,19 +919,8 @@ __weak int usb_alloc_device(struct usb_device *udev)
 
 static int usb_hub_port_reset(struct usb_device *dev, struct usb_device *hub)
 {
-	if (hub) {
-		unsigned short portstatus;
-		int err;
-
-		/* reset the port for the second time */
-		err = legacy_hub_port_reset(hub, dev->portnr - 1, &portstatus);
-		if (err < 0) {
-			printf("\n     Couldn't reset port %i\n", dev->portnr);
-			return err;
-		}
-	} else {
+	if (!hub)
 		usb_reset_root_port(dev);
-	}
 
 	return 0;
 }
-- 
2.7.3

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stefan Roese]

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.

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;
+
+			/* 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@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);
+		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..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;
-- 
2.7.3

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Hans de Goede]

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;
>   #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@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

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stefan Roese]

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@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@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

[U-Boot] [PATCH v3 2/4] usb: Remove 200 ms delay in usb_hub_port_connect_change()

[Stephen Warren]

On 03/14/2016 04:18 AM, Stefan Roese wrote:
> This patch removes 2 mdelay(200) calls from usb_hub_port_connect_change().
> These delays don't seem to be necessary. At least not in my tests. Here
> the number for a custom x86 Bay Trail board (not in mainline yet) with
> a quite large and complex USB hub infrastructure.
>
> Without this patch:
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 28.415 seconds
>
> With this patch:
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 24.003 seconds
>
> So ~4.5 seconds of USB scanning time reduction.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Marek Vasut <marex@denx.de>
>
> ---
>
> Changes in v3: None
> Changes in v2:
> - Make this change unconditional
> - Add Acked-by / Tested-by from Hans and Stephen

I meant to mention: I think you forgot to actually do that. There aren't 
any acks in the commit description.

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stephen Warren]

On 03/14/2016 04:18 AM, Stefan Roese wrote:
> This patch changes the USB port scanning procedure and timeout
> handling in the following ways:

A few nits/typos in the description, and some review comments below.

> 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
               ^^^^^^^^ change to ", instead"? The existing text looks 
like an editing mistake.

> 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

stabilize

> 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

quasi

> 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

simultaneously

> 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

that's

> being scanned will start earlier.

> diff --git a/common/usb_hub.c b/common/usb_hub.c

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

I'd be tempted to make that:

dev->connect_timeout = dev->query_delay + 1000;

That way, if the max() used in the calculation of dev->query_delay was 
dominated by the 100 not the pgood_delay, then we still get a 1000ms 
time for the device to connect after the power is stable. Currently, if 
pgood_delay<=100 (is that legal?) then the delta might be as little as 
900ms (for pgood_delay==0).

> static LIST_HEAD(usb_scan_list);

You could put that onto the stack in usb_hub_configure() and pass it as 
a parameter to usb_device_list_scan(). That would avoid some globals, 
which might make it easier to apply this technique across multiple 
controllers/hubs/... in the future?

> +static int usb_scan_port(struct usb_device_scan *usb_scan)

> +	ret = usb_get_port_status(dev, i + 1, portsts);
> +	if (ret < 0) {
> +		debug("get_port_status failed\n");
> +		return 0;

Shouldn't this cause a timeout eventually if it repeats forever?

> +	/* Test if the connection came up, and if so, exit. */
> +	if (portstatus & USB_PORT_STAT_CONNECTION) {

If you invert that test, you can return early and remove and indentation 
level from the rest of the function.

> +		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);

It might be nice to print this a little earlier (outside the 
USB_PORT_STAT_CONNECTION if block) so that any 
USB_PORT_STAT_C_CONNECTION triggers the print, not just if C_CONNECTION 
&& CONNECTION. That might help debug, and match the existing code.

> +		if (portchange & USB_PORT_STAT_C_CONNECTION) {
 > +			debug("port %d connection change\n", i + 1);
 > +			usb_hub_port_connect_change(dev, i);
 > +		}

That test is always true, or the function would have returned earlier.

> +static int usb_device_list_scan(void)
...
> +	static int running;
> +	int ret = 0;
> +
> +	/* Only run this loop once for each controller */
> +	if (running)
> +		return 0;
> +
> +	running = 1;
...
> +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;

Doesn't this function only get called a single time from 
usb_hub_configure()? If so, I don't think the "running" variable is 
required.

>   static int usb_hub_configure(struct usb_device *dev)

> +	for (i = 0; i < dev->maxchild; i++) {
> +		struct usb_device_scan *usb_scan;
>
> +		usb_scan = calloc(1, sizeof(*usb_scan));
> +		if (!usb_scan) {
> +			printf("Can't allocate memory for USB device!\n");
> +			return -ENOMEM;

I think there should be some resource cleanup for the previously 
allocated list entries here. Perhaps that's what you meant in your other 
email where you talked about some missing free()s?

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stephen Warren]

On 03/14/2016 04:18 AM, Stefan Roese wrote:
> This patch changes the USB port scanning procedure and timeout
> handling in the following ways:
 >..

Tested-by: Stephen Warren <swarren@nvidia.com>

(including some tests with a 7-port (i.e. 2 nested 4-port) USB hub maxed 
out with devices).

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Hans de Goede]

Hi,

On 14-03-16 18:31, Stephen Warren wrote:
> On 03/14/2016 04:18 AM, Stefan Roese wrote:

<snip>

>> @@ -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;
>
> I'd be tempted to make that:
>
> dev->connect_timeout = dev->query_delay + 1000;
>
> That way, if the max() used in the calculation of dev->query_delay was dominated by the 100 not the pgood_delay, then we still get a 1000ms time for the device to connect after the power is stable.

Ack, good catch.

> Currently, if pgood_delay<=100 (is that legal?) then the delta might be as little as 900ms (for pgood_delay==0).

AFAIK it is not legal, but guess what the firmware authors of cheap hubs put in the
descriptor when 0 seems to work just fine ?

Rule of thumb: Never trust usb descriptors.

Regards,

Hans



>
>> static LIST_HEAD(usb_scan_list);
>
> You could put that onto the stack in usb_hub_configure() and pass it as a parameter to usb_device_list_scan(). That would avoid some globals, which might make it easier to apply this technique across multiple controllers/hubs/... in the future?
>
>> +static int usb_scan_port(struct usb_device_scan *usb_scan)
>
>> +    ret = usb_get_port_status(dev, i + 1, portsts);
>> +    if (ret < 0) {
>> +        debug("get_port_status failed\n");
>> +        return 0;
>
> Shouldn't this cause a timeout eventually if it repeats forever?
>
>> +    /* Test if the connection came up, and if so, exit. */
>> +    if (portstatus & USB_PORT_STAT_CONNECTION) {
>
> If you invert that test, you can return early and remove and indentation level from the rest of the function.
>
>> +        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);
>
> It might be nice to print this a little earlier (outside the USB_PORT_STAT_CONNECTION if block) so that any USB_PORT_STAT_C_CONNECTION triggers the print, not just if C_CONNECTION && CONNECTION. That might help debug, and match the existing code.
>
>> +        if (portchange & USB_PORT_STAT_C_CONNECTION) {
>  > +            debug("port %d connection change\n", i + 1);
>  > +            usb_hub_port_connect_change(dev, i);
>  > +        }
>
> That test is always true, or the function would have returned earlier.
>
>> +static int usb_device_list_scan(void)
> ...
>> +    static int running;
>> +    int ret = 0;
>> +
>> +    /* Only run this loop once for each controller */
>> +    if (running)
>> +        return 0;
>> +
>> +    running = 1;
> ...
>> +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;
>
> Doesn't this function only get called a single time from usb_hub_configure()? If so, I don't think the "running" variable is required.
>
>>   static int usb_hub_configure(struct usb_device *dev)
>
>> +    for (i = 0; i < dev->maxchild; i++) {
>> +        struct usb_device_scan *usb_scan;
>>
>> +        usb_scan = calloc(1, sizeof(*usb_scan));
>> +        if (!usb_scan) {
>> +            printf("Can't allocate memory for USB device!\n");
>> +            return -ENOMEM;
>
> I think there should be some resource cleanup for the previously allocated list entries here. Perhaps that's what you meant in your other email where you talked about some missing free()s?
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stefan Roese]

Hi Stephan,

On 14.03.2016 18:31, Stephen Warren wrote:
> On 03/14/2016 04:18 AM, Stefan Roese wrote:
>> This patch changes the USB port scanning procedure and timeout
>> handling in the following ways:
> 
> A few nits/typos in the description, and some review comments below.

Thanks, will update the commit text in v4.

<snip>
 
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> 
>> @@ -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;
> 
> I'd be tempted to make that:
> 
> dev->connect_timeout = dev->query_delay + 1000;
> 
> That way, if the max() used in the calculation of dev->query_delay was 
> dominated by the 100 not the pgood_delay, then we still get a 1000ms 
> time for the device to connect after the power is stable. Currently, if 
> pgood_delay<=100 (is that legal?) then the delta might be as little as 
> 900ms (for pgood_delay==0).

Fixed.
 
>> static LIST_HEAD(usb_scan_list);
> 
> You could put that onto the stack in usb_hub_configure() and pass it as 
> a parameter to usb_device_list_scan(). That would avoid some globals, 
> which might make it easier to apply this technique across multiple 
> controllers/hubs/... in the future?

Multiple controllers/hubs should be supported currently (at least
in my tests). Its the parallel scanning of those controllers which
might be problematic. usb_hub.c needs some general rework, as it
has some more globals:

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

I've moved "usb_scan_list" to these declarations for now, so that
all of them can be cleaned up once somebody works on task.
 
>> +static int usb_scan_port(struct usb_device_scan *usb_scan)
> 
>> +    ret = usb_get_port_status(dev, i + 1, portsts);
>> +    if (ret < 0) {
>> +        debug("get_port_status failed\n");
>> +        return 0;
> 
> Shouldn't this cause a timeout eventually if it repeats forever?

Okay. I've added a timeout check here to remove such a non-functional
device.
 
>> +    /* Test if the connection came up, and if so, exit. */
>> +    if (portstatus & USB_PORT_STAT_CONNECTION) {
> 
> If you invert that test, you can return early and remove and indentation 
> level from the rest of the function.

Good catch. Changed in v4.
 
>> +        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);
> 
> It might be nice to print this a little earlier (outside the 
> USB_PORT_STAT_CONNECTION if block) so that any 
> USB_PORT_STAT_C_CONNECTION triggers the print, not just if C_CONNECTION 
> && CONNECTION. That might help debug, and match the existing code.

Changed in v4.
 
>> +        if (portchange & USB_PORT_STAT_C_CONNECTION) {
>  > +            debug("port %d connection change\n", i + 1);
>  > +            usb_hub_port_connect_change(dev, i);
>  > +        }
> 
> That test is always true, or the function would have returned earlier.

Changed in v4.
 
>> +static int usb_device_list_scan(void)
> ...
>> +    static int running;
>> +    int ret = 0;
>> +
>> +    /* Only run this loop once for each controller */
>> +    if (running)
>> +        return 0;
>> +
>> +    running = 1;
> ...
>> +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;
> 
> Doesn't this function only get called a single time from 
> usb_hub_configure()? If so, I don't think the "running" variable is 
> required.

Its called recursively, if hubs are stacked. So its called e.g.
5 times in this setup:

=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |  u-boot EHCI Host Controller 
  |
  +-2  Hub (480 Mb/s, 0mA)
    |
    +-3  Hub (480 Mb/s, 100mA)
    | |
    | +-7  Hub (12 Mb/s, 100mA)
    |    
    +-4  Hub (480 Mb/s, 0mA)
    | |
    | +-8  Mass Storage (480 Mb/s, 200mA)
    | |    Kingston DataTraveler 2.0 50E549C688C4BE7189942766
    | |  
    | +-9  Mass Storage (480 Mb/s, 98mA)
    |      USBest Technology USB Mass Storage Device 09092207fbf0c4
    |    
    +-5  Mass Storage (480 Mb/s, 200mA)
    |    6989 Intenso Alu Line EE706F5E
    |  
    +-6  Mass Storage (480 Mb/s, 200mA)
         JetFlash Mass Storage Device 3281440601
 
>>   static int usb_hub_configure(struct usb_device *dev)
> 
>> +    for (i = 0; i < dev->maxchild; i++) {
>> +        struct usb_device_scan *usb_scan;
>>
>> +        usb_scan = calloc(1, sizeof(*usb_scan));
>> +        if (!usb_scan) {
>> +            printf("Can't allocate memory for USB device!\n");
>> +            return -ENOMEM;
> 
> I think there should be some resource cleanup for the previously 
> allocated list entries here. Perhaps that's what you meant in your other 
> email where you talked about some missing free()s?

Correct. I already noticed that the malloc is missing the free part.
Its added in v4.

Thanks for the review,
Stefan

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stefan Roese]

Hi Stephen,

On 14.03.2016 18:51, Stephen Warren wrote:
> On 03/14/2016 04:18 AM, Stefan Roese wrote:
>> This patch changes the USB port scanning procedure and timeout
>> handling in the following ways:
>  >..
>
> Tested-by: Stephen Warren <swarren@nvidia.com>
>
> (including some tests with a 7-port (i.e. 2 nested 4-port) USB hub maxed
> out with devices).

Thanks for doing those tests on your platform. I'm interested, how
big the time difference between stock U-Boot and the patched
version is though. Could you please post those numbers once for
reference, if its not too much trouble?

Thanks,
Stefan

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stephen Warren]

On 03/15/2016 03:46 AM, Stefan Roese wrote:
> Hi Stephan,
>
> On 14.03.2016 18:31, Stephen Warren wrote:
>> On 03/14/2016 04:18 AM, Stefan Roese wrote:
>>> This patch changes the USB port scanning procedure and timeout
>>> handling in the following ways:

>>> +static int usb_device_list_scan(void)
>> ...
>>> +    static int running;
>>> +    int ret = 0;
>>> +
>>> +    /* Only run this loop once for each controller */
>>> +    if (running)
>>> +        return 0;
>>> +
>>> +    running = 1;
>> ...
>>> +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;
>>
>> Doesn't this function only get called a single time from
>> usb_hub_configure()? If so, I don't think the "running" variable is
>> required.
>
> Its called recursively, if hubs are stacked. So its called e.g.
> 5 times in this setup:
>
> => usb tree
> USB device tree:
>    1  Hub (480 Mb/s, 0mA)
>    |  u-boot EHCI Host Controller
>    |
>    +-2  Hub (480 Mb/s, 0mA)
>      |
>      +-3  Hub (480 Mb/s, 100mA)
 >...

Ah right. Mentioning recursion in the variable name or comment would be 
useful.

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stephen Warren]

On 03/15/2016 07:16 AM, Stefan Roese wrote:
> Hi Stephen,
>
> On 14.03.2016 18:51, Stephen Warren wrote:
>> On 03/14/2016 04:18 AM, Stefan Roese wrote:
>>> This patch changes the USB port scanning procedure and timeout
>>> handling in the following ways:
>>  >..
>>
>> Tested-by: Stephen Warren <swarren@nvidia.com>
>>
>> (including some tests with a 7-port (i.e. 2 nested 4-port) USB hub maxed
>> out with devices).
>
> Thanks for doing those tests on your platform. I'm interested, how
> big the time difference between stock U-Boot and the patched
> version is though. Could you please post those numbers once for
> reference, if its not too much trouble?

It looks like ~17s for v2016.01, and ~6s for u-boot/master from 
yesterday plus these patches, with the following configuration:

Tegra124 (Jetson TK1) # usb tree
USB device tree:
   1  Hub (480 Mb/s, 0mA)
      u-boot EHCI Host Controller

   1  Hub (480 Mb/s, 0mA)
   |  u-boot EHCI Host Controller
   |
   +-2  Hub (480 Mb/s, 100mA)
     |   USB2.0 Hub
     |
     +-3  Hub (480 Mb/s, 100mA)
     | |   USB2.0 Hub
     | |
     | +-6  Mass Storage (480 Mb/s, 2mA)
     | |    Sunplus Innovation Technology. USB to Serial-ATA bridge 
FF980813AF0000000000005FF16BFF
     | |
     | +-7  Mass Storage (480 Mb/s, 100mA)
     | |    Generic    Mass Storage Device 00000000000006
     | |
     | +-8  Human Interface (1.5 Mb/s, 70mA)
     | |    LITEON Technology USB Multimedia Keyboard
     | |
     | +-9  Human Interface (1.5 Mb/s, 100mA)
     |      Microsoft Microsoft IntelliMouse? Explore
     |
     +-4  Vendor specific (480 Mb/s, 250mA)
     |    ASIX Elec. Corp. AX88x72A 000001
     |
     +-5  Mass Storage (480 Mb/s, 200mA)
          SanDisk Extreme AA010114140242512567

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stefan Roese]

Hi Stephen,

On 15.03.2016 16:52, Stephen Warren wrote:
> On 03/15/2016 07:16 AM, Stefan Roese wrote:
>> Hi Stephen,
>>
>> On 14.03.2016 18:51, Stephen Warren wrote:
>>> On 03/14/2016 04:18 AM, Stefan Roese wrote:
>>>> This patch changes the USB port scanning procedure and timeout
>>>> handling in the following ways:
>>>  >..
>>>
>>> Tested-by: Stephen Warren <swarren@nvidia.com>
>>>
>>> (including some tests with a 7-port (i.e. 2 nested 4-port) USB hub maxed
>>> out with devices).
>>
>> Thanks for doing those tests on your platform. I'm interested, how
>> big the time difference between stock U-Boot and the patched
>> version is though. Could you please post those numbers once for
>> reference, if its not too much trouble?
> 
> It looks like ~17s for v2016.01, and ~6s for u-boot/master from 
> yesterday plus these patches, with the following configuration:
> 
> Tegra124 (Jetson TK1) # usb tree
> USB device tree:
>    1  Hub (480 Mb/s, 0mA)
>       u-boot EHCI Host Controller
> 
>    1  Hub (480 Mb/s, 0mA)
>    |  u-boot EHCI Host Controller
>    |
>    +-2  Hub (480 Mb/s, 100mA)
>      |   USB2.0 Hub
>      |
>      +-3  Hub (480 Mb/s, 100mA)
>      | |   USB2.0 Hub
>      | |
>      | +-6  Mass Storage (480 Mb/s, 2mA)
>      | |    Sunplus Innovation Technology. USB to Serial-ATA bridge 
> FF980813AF0000000000005FF16BFF
>      | |
>      | +-7  Mass Storage (480 Mb/s, 100mA)
>      | |    Generic    Mass Storage Device 00000000000006
>      | |
>      | +-8  Human Interface (1.5 Mb/s, 70mA)
>      | |    LITEON Technology USB Multimedia Keyboard
>      | |
>      | +-9  Human Interface (1.5 Mb/s, 100mA)
>      |      Microsoft Microsoft IntelliMouse? Explore
>      |
>      +-4  Vendor specific (480 Mb/s, 250mA)
>      |    ASIX Elec. Corp. AX88x72A 000001
>      |
>      +-5  Mass Storage (480 Mb/s, 200mA)
>           SanDisk Extreme AA010114140242512567

Thanks.

I'm wondering why your configuration takes that much longer to
scan than mine (x86 platform). My USB configuration is not
that different:

=> time usb start
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 1.815 seconds
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |  u-boot EHCI Host Controller 
  |
  +-2  Hub (480 Mb/s, 0mA)
    |
    +-3  Hub (480 Mb/s, 100mA)
    | |
    | +-7  Hub (12 Mb/s, 100mA)
    |    
    +-4  Hub (480 Mb/s, 0mA)
    | |
    | +-8  Mass Storage (480 Mb/s, 200mA)
    | |    Kingston DataTraveler 2.0 50E549C688C4BE7189942766
    | |  
    | +-9  Mass Storage (480 Mb/s, 98mA)
    |      USBest Technology USB Mass Storage Device 09092207fbf0c4
    |    
    +-5  Mass Storage (480 Mb/s, 200mA)
    |    6989 Intenso Alu Line EE706F5E
    |  
    +-6  Mass Storage (480 Mb/s, 200mA)
         JetFlash Mass Storage Device 3281440601


And I'm down to less than 2 seconds scanning time. Perhaps
worth digging into at some point. But lets get this version
upstreamed first.

Thanks,
Stefan

[U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling

[Stephen Warren]

On 03/15/2016 10:00 AM, Stefan Roese wrote:
> Hi Stephen,
>
> On 15.03.2016 16:52, Stephen Warren wrote:
>> On 03/15/2016 07:16 AM, Stefan Roese wrote:
>>> Hi Stephen,
>>>
>>> On 14.03.2016 18:51, Stephen Warren wrote:
>>>> On 03/14/2016 04:18 AM, Stefan Roese wrote:
>>>>> This patch changes the USB port scanning procedure and timeout
>>>>> handling in the following ways:
>>>>   >..
>>>>
>>>> Tested-by: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> (including some tests with a 7-port (i.e. 2 nested 4-port) USB hub maxed
>>>> out with devices).
>>>
>>> Thanks for doing those tests on your platform. I'm interested, how
>>> big the time difference between stock U-Boot and the patched
>>> version is though. Could you please post those numbers once for
>>> reference, if its not too much trouble?
>>
>> It looks like ~17s for v2016.01, and ~6s for u-boot/master from
>> yesterday plus these patches, with the following configuration:
>>
>> Tegra124 (Jetson TK1) # usb tree
>> USB device tree:
>>     1  Hub (480 Mb/s, 0mA)
>>        u-boot EHCI Host Controller
>>
>>     1  Hub (480 Mb/s, 0mA)
>>     |  u-boot EHCI Host Controller
>>     |
>>     +-2  Hub (480 Mb/s, 100mA)
>>       |   USB2.0 Hub
>>       |
>>       +-3  Hub (480 Mb/s, 100mA)
>>       | |   USB2.0 Hub
>>       | |
>>       | +-6  Mass Storage (480 Mb/s, 2mA)
>>       | |    Sunplus Innovation Technology. USB to Serial-ATA bridge
>> FF980813AF0000000000005FF16BFF
>>       | |
>>       | +-7  Mass Storage (480 Mb/s, 100mA)
>>       | |    Generic    Mass Storage Device 00000000000006
>>       | |
>>       | +-8  Human Interface (1.5 Mb/s, 70mA)
>>       | |    LITEON Technology USB Multimedia Keyboard
>>       | |
>>       | +-9  Human Interface (1.5 Mb/s, 100mA)
>>       |      Microsoft Microsoft IntelliMouse? Explore
>>       |
>>       +-4  Vendor specific (480 Mb/s, 250mA)
>>       |    ASIX Elec. Corp. AX88x72A 000001
>>       |
>>       +-5  Mass Storage (480 Mb/s, 200mA)
>>            SanDisk Extreme AA010114140242512567
>
> Thanks.
>
> I'm wondering why your configuration takes that much longer to
> scan than mine (x86 platform). My USB configuration is not
> that different:
>
> => time usb start
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 1.815 seconds
> => usb tree
> USB device tree:
>    1  Hub (480 Mb/s, 0mA)
>    |  u-boot EHCI Host Controller
>    |
>    +-2  Hub (480 Mb/s, 0mA)
>      |
>      +-3  Hub (480 Mb/s, 100mA)
>      | |
>      | +-7  Hub (12 Mb/s, 100mA)
>      |
>      +-4  Hub (480 Mb/s, 0mA)
>      | |
>      | +-8  Mass Storage (480 Mb/s, 200mA)
>      | |    Kingston DataTraveler 2.0 50E549C688C4BE7189942766
>      | |
>      | +-9  Mass Storage (480 Mb/s, 98mA)
>      |      USBest Technology USB Mass Storage Device 09092207fbf0c4
>      |
>      +-5  Mass Storage (480 Mb/s, 200mA)
>      |    6989 Intenso Alu Line EE706F5E
>      |
>      +-6  Mass Storage (480 Mb/s, 200mA)
>           JetFlash Mass Storage Device 3281440601
>
>
> And I'm down to less than 2 seconds scanning time. Perhaps
> worth digging into at some point. But lets get this version
> upstreamed first.

I suspect my storage devices are slow; during USB enumeration I hear the 
HDD quickly spinning down/up as it's reset, and I know my SD card reader 
is rather slow to react (and probably requires some retries) since it 
experiences:

Tegra124 (Jetson TK1) # usb start
starting USB...
USB0:   USB EHCI 1.10
USB1:   USB EHCI 1.10
scanning bus 0 for devices... 1 USB Device(s) found
scanning bus 1 for devices... Device NOT ready
    Request Sense returned 02 3A 00
2 USB Device(s) found

I also have 2 USB controllers, one of which has nothing attached, which 
might almost double the root port scan timeout alone?