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

[U-Boot] [PATCH v4 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 v4:
- Minor rewording / fixes of the commit text
- Add Acked-by / Tested-by from Hans and Stephen
- Minor rewording / fixes of the commit text
- Add Acked-by from Hans
- Moved check for query_delay into usb_scan_port() as suggested by Hans
- Correct list handling (drop INIT_LIST_HEAD)
- Added some missing free() calls
- Changed connect_timeout calculation as suggested by Stephen
- Moved usb_scan_list to other globals to be cleaned up in a later patch
- Added timeout check for non-functional ports (usb_get_port_status
  return error
- Reverted if logic in loop to remove an indentation level
- Moved debug() output
- Removed unnecessary if when already connected
- Added Hans's Acked-by
- Added Stephen's Tested-by
- Minor rewording / fixes of the commit text

Changes in v3:
- 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 | 305 +++++++++++++++++++++++++++++++++++++------------------
 include/usb.h    |   3 +
 3 files changed, 212 insertions(+), 109 deletions(-)

-- 
2.7.3

[U-Boot] [PATCH v4 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 end 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 v4:
- Minor rewording / fixes of the commit text

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 v4 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>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
Cc: Marek Vasut <marex@denx.de>

---

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

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 v4 3/4] usb: Don't reset the USB hub a 2nd time

[Stefan Roese]

Debugging has shown, that all USB hubs are being reset 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>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
Cc: Marek Vasut <marex@denx.de>

---

Changes in v4:
- Minor rewording / fixes of the commit text
- Add Acked-by from Hans

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 v4 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, instead usb_hub_power_on() will wait before querying
the device in the scanning loop later. The total timeout for this
hub, which is 1 second + "hub->desc.bPwrOn2PwrGood * 2" is calculated
and will be used in the following per-port scanning loop as the timeout
to detect active USB devices on this hub.

b)
Don't delay the minimum delay (for power to stabilize) in
usb_hub_power_on(). Instead skip querying these devices in the scannig
loop until the delay time is reached.

c)
The ports are now scanned in a quasi parallel way. The current code did
wait for each (unconnected) port to reach its timeout and only then
continue with the next port. This patch now changes this to scan all
ports of all USB hubs quasi simultaneously. For this, all ports are added
to a scanning list. This list is scanned until all ports are ready
by either a) reaching the connection timeout (calculated earlier), or
by b) detecting a USB device. This results in a faster USB scan time as
the recursive scanning of USB hubs connected to the hub that's currently
being scanned will start earlier.

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>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stephen Warren <swarren@nvidia.com>

---

Changes in v4:
- Moved check for query_delay into usb_scan_port() as suggested by Hans
- Correct list handling (drop INIT_LIST_HEAD)
- Added some missing free() calls
- Changed connect_timeout calculation as suggested by Stephen
- Moved usb_scan_list to other globals to be cleaned up in a later patch
- Added timeout check for non-functional ports (usb_get_port_status
  return error
- Reverted if logic in loop to remove an indentation level
- Moved debug() output
- Removed unnecessary if when already connected
- Added Hans's Acked-by
- Added Stephen's Tested-by
- Minor rewording / fixes of the commit text

Changes in v3:
- Introduced scanning list containing all USB devices of one USB
  controller that need to get scanned
- Don't delay in usb_hub_power_on(). Instead skip querying these devices
  until the delay time is reached.

Changes in v2:
- Remove static USB port configuration patch (for now)

 common/usb_hub.c | 293 ++++++++++++++++++++++++++++++++++++++-----------------
 include/usb.h    |   3 +
 2 files changed, 204 insertions(+), 92 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index d621f50..e8cfc7e 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -30,6 +30,7 @@
 #include <asm/processor.h>
 #include <asm/unaligned.h>
 #include <linux/ctype.h>
+#include <linux/list.h>
 #include <asm/byteorder.h>
 #ifdef CONFIG_SANDBOX
 #include <asm/state.h>
@@ -49,9 +50,17 @@ DECLARE_GLOBAL_DATA_PTR;
 #define HUB_SHORT_RESET_TIME	20
 #define HUB_LONG_RESET_TIME	200
 
+struct usb_device_scan {
+	struct usb_device *dev;		/* USB hub device to scan */
+	struct usb_hub_device *hub;	/* USB hub struct */
+	int port;			/* USB port to scan */
+	struct list_head list;
+};
+
 /* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
 static struct usb_hub_device hub_dev[USB_MAX_HUB];
 static int usb_hub_index;
+static LIST_HEAD(usb_scan_list);
 
 __weak void usb_hub_reset_devices(int port)
 {
@@ -120,7 +129,22 @@ 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 = dev->query_delay + 1000;
+	debug("devnum=%d poweron: query_delay=%d connect_timeout=%d\n",
+	      dev->devnum, max(100, (int)pgood_delay),
+	      max(100, (int)pgood_delay) + 1000);
 }
 
 void usb_hub_reset(void)
@@ -332,6 +356,157 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
 	return ret;
 }
 
+static int usb_scan_port(struct usb_device_scan *usb_scan)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
+	unsigned short portstatus;
+	unsigned short portchange;
+	struct usb_device *dev;
+	struct usb_hub_device *hub;
+	int ret = 0;
+	int i;
+
+	dev = usb_scan->dev;
+	hub = usb_scan->hub;
+	i = usb_scan->port;
+
+	/*
+	 * Don't talk to the device before the query delay is expired.
+	 * This is needed for voltages to stabalize.
+	 */
+	if (get_timer(0) < dev->query_delay)
+		return 0;
+
+#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");
+		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);
+			free(usb_scan);
+			return 0;
+		}
+	}
+
+	portstatus = le16_to_cpu(portsts->wPortStatus);
+	portchange = le16_to_cpu(portsts->wPortChange);
+	debug("Port %d Status %X Change %X\n", i + 1, portstatus, portchange);
+
+	/* No connection change happened, wait a bit more. */
+	if (!(portchange & USB_PORT_STAT_C_CONNECTION)) {
+		if (get_timer(0) >= 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);
+			free(usb_scan);
+			return 0;
+		}
+		return 0;
+	}
+
+	/* Test if the connection came up, and if not exit */
+	if (!(portstatus & USB_PORT_STAT_CONNECTION))
+		return 0;
+
+	/* A new USB device is ready at this point */
+	debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1);
+
+	/* Remove this device from scanning list */
+	list_del(&usb_scan->list);
+	free(usb_scan);
+
+	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;
+
+			/* 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 +641,38 @@ 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;
+		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 v4 4/4] usb: Change power-on / scanning timeout handling

[Hans de Goede]

Hi,

Just noticed a few more things which need a minor
tweak, so looks like we're going to need a v5, sorry.

On 15-03-16 10:46, Stefan Roese wrote:
> This patch changes the USB port scanning procedure and timeout
> handling in the following ways:
>
> a)
> The power-on delay in usb_hub_power_on() is now reduced to a value of
> max(100ms, "hub->desc.bPwrOn2PwrGood * 2"). The code does not wait
> using mdelay, instead usb_hub_power_on() will wait before querying
> the device in the scanning loop later. The total timeout for this
> hub, which is 1 second + "hub->desc.bPwrOn2PwrGood * 2" is calculated
> and will be used in the following per-port scanning loop as the timeout
> to detect active USB devices on this hub.
>
> b)
> Don't delay the minimum delay (for power to stabilize) in
> usb_hub_power_on(). Instead skip querying these devices in the scannig
> loop until the delay time is reached.
>
> c)
> The ports are now scanned in a quasi parallel way. The current code did
> wait for each (unconnected) port to reach its timeout and only then
> continue with the next port. This patch now changes this to scan all
> ports of all USB hubs quasi simultaneously. For this, all ports are added
> to a scanning list. This list is scanned until all ports are ready
> by either a) reaching the connection timeout (calculated earlier), or
> by b) detecting a USB device. This results in a faster USB scan time as
> the recursive scanning of USB hubs connected to the hub that's currently
> being scanned will start earlier.
>
> 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>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>
>
> ---
>
> Changes in v4:
> - Moved check for query_delay into usb_scan_port() as suggested by Hans
> - Correct list handling (drop INIT_LIST_HEAD)
> - Added some missing free() calls
> - Changed connect_timeout calculation as suggested by Stephen
> - Moved usb_scan_list to other globals to be cleaned up in a later patch
> - Added timeout check for non-functional ports (usb_get_port_status
>    return error
> - Reverted if logic in loop to remove an indentation level
> - Moved debug() output
> - Removed unnecessary if when already connected
> - Added Hans's Acked-by
> - Added Stephen's Tested-by
> - Minor rewording / fixes of the commit text
>
> Changes in v3:
> - Introduced scanning list containing all USB devices of one USB
>    controller that need to get scanned
> - Don't delay in usb_hub_power_on(). Instead skip querying these devices
>    until the delay time is reached.
>
> Changes in v2:
> - Remove static USB port configuration patch (for now)
>
>   common/usb_hub.c | 293 ++++++++++++++++++++++++++++++++++++++-----------------
>   include/usb.h    |   3 +
>   2 files changed, 204 insertions(+), 92 deletions(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index d621f50..e8cfc7e 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -30,6 +30,7 @@
>   #include <asm/processor.h>
>   #include <asm/unaligned.h>
>   #include <linux/ctype.h>
> +#include <linux/list.h>
>   #include <asm/byteorder.h>
>   #ifdef CONFIG_SANDBOX
>   #include <asm/state.h>
> @@ -49,9 +50,17 @@ DECLARE_GLOBAL_DATA_PTR;
>   #define HUB_SHORT_RESET_TIME	20
>   #define HUB_LONG_RESET_TIME	200
>
> +struct usb_device_scan {
> +	struct usb_device *dev;		/* USB hub device to scan */
> +	struct usb_hub_device *hub;	/* USB hub struct */
> +	int port;			/* USB port to scan */
> +	struct list_head list;
> +};
> +
>   /* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
>   static struct usb_hub_device hub_dev[USB_MAX_HUB];
>   static int usb_hub_index;
> +static LIST_HEAD(usb_scan_list);
>
>   __weak void usb_hub_reset_devices(int port)
>   {
> @@ -120,7 +129,22 @@ 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 = dev->query_delay + 1000;
> +	debug("devnum=%d poweron: query_delay=%d connect_timeout=%d\n",
> +	      dev->devnum, max(100, (int)pgood_delay),
> +	      max(100, (int)pgood_delay) + 1000);
>   }
>
>   void usb_hub_reset(void)
> @@ -332,6 +356,157 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
>   	return ret;
>   }
>
> +static int usb_scan_port(struct usb_device_scan *usb_scan)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
> +	unsigned short portstatus;
> +	unsigned short portchange;
> +	struct usb_device *dev;
> +	struct usb_hub_device *hub;
> +	int ret = 0;
> +	int i;
> +
> +	dev = usb_scan->dev;
> +	hub = usb_scan->hub;
> +	i = usb_scan->port;
> +
> +	/*
> +	 * Don't talk to the device before the query delay is expired.
> +	 * This is needed for voltages to stabalize.
> +	 */
> +	if (get_timer(0) < dev->query_delay)
> +		return 0;
> +
> +#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

We may now end up printing this a lot of times when debug printing
is enabled, probably best to remove it (less is more :)

> +	ret = usb_get_port_status(dev, i + 1, portsts);
> +	if (ret < 0) {
> +		debug("get_port_status failed\n");
> +		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);
> +			free(usb_scan);
> +			return 0;
> +		}
> +	}
> +
> +	portstatus = le16_to_cpu(portsts->wPortStatus);
> +	portchange = le16_to_cpu(portsts->wPortChange);
> +	debug("Port %d Status %X Change %X\n", i + 1, portstatus, portchange);
> +
> +	/* No connection change happened, wait a bit more. */
> +	if (!(portchange & USB_PORT_STAT_C_CONNECTION)) {
> +		if (get_timer(0) >= 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);
> +			free(usb_scan);
> +			return 0;
> +		}
> +		return 0;
> +	}
> +
> +	/* Test if the connection came up, and if not exit */
> +	if (!(portstatus & USB_PORT_STAT_CONNECTION))
> +		return 0;
> +
> +	/* A new USB device is ready at this point */
> +	debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1);

Just printing this in debug mode should be enough.

> +	/* Remove this device from scanning list */
> +	list_del(&usb_scan->list);
> +	free(usb_scan);
> +
> +	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);

This is the one why we need a v5, so usb_hub_power_on(hub) will reset the
timeouts! That by-itself is fine, if the over-current was caused by
a spike, but if the over-current is permanent, we will just get
an overcurrent report again after powering on the port again,
rinse repeat -> never ending loop.

So there are 2 options:

1) On overcurrent remove the device from the list of devices to scan,
the old code did the power-on after the loop with the delay, so it would
actually not rescan the device after an overcurrent condition.
Doing the power_on as the old code does seems useless in this case, we
should probably still do it though, to reduce the amount of behavior changes
and then remove it in a separate patch.

2) Do not reset the timeouts, do the power_on as the old code did and
try to enumerate the device again.

3) reset the timeouts, but keep a per hub (or per port?) over-current counter
and if it reaches say 3 then quit powering the port back on, and remove the
port from the list of devices to scan instead.

1. keeps us closest to the original code behavior, but IMHO 3. is better.
I think we can be lazy and just make the over-current counter a per hub
thing rather then per port. Note that the current code does:

                 usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);

On all ports on over-current, that is something which we may want to fix
too, but I believe that belongs in a separate follow-up patch.


> +	}
> +
> +	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;
> +
> +			/* 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 +641,38 @@ 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;

1) poweron_timeout does not exist, looks like you did not do a test
sandbox build

2) You should clear both connect_timeout and query_delay here, or better
simply put:

	if (state_get_skip_delays())
		return;

In usb_hub_power_on() before setting these (this assumes that usb_device
gets kzalloc-ed).


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

I just realized: wouldn't it be better to put these 2 in struct usb_hub_device?
you are tracking them per hub not per port, so they seem to belong there.

Regards,

Hans

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

[Stefan Roese]

Hi Hans,

On 15.03.2016 11:44, Hans de Goede wrote:
> Just noticed a few more things which need a minor
> tweak, so looks like we're going to need a v5, sorry.
>
> On 15-03-16 10:46, Stefan Roese wrote:
>> This patch changes the USB port scanning procedure and timeout
>> handling in the following ways:
>>
>> a)
>> The power-on delay in usb_hub_power_on() is now reduced to a value of
>> max(100ms, "hub->desc.bPwrOn2PwrGood * 2"). The code does not wait
>> using mdelay, instead usb_hub_power_on() will wait before querying
>> the device in the scanning loop later. The total timeout for this
>> hub, which is 1 second + "hub->desc.bPwrOn2PwrGood * 2" is calculated
>> and will be used in the following per-port scanning loop as the timeout
>> to detect active USB devices on this hub.
>>
>> b)
>> Don't delay the minimum delay (for power to stabilize) in
>> usb_hub_power_on(). Instead skip querying these devices in the scannig
>> loop until the delay time is reached.
>>
>> c)
>> The ports are now scanned in a quasi parallel way. The current code did
>> wait for each (unconnected) port to reach its timeout and only then
>> continue with the next port. This patch now changes this to scan all
>> ports of all USB hubs quasi simultaneously. For this, all ports are added
>> to a scanning list. This list is scanned until all ports are ready
>> by either a) reaching the connection timeout (calculated earlier), or
>> by b) detecting a USB device. This results in a faster USB scan time as
>> the recursive scanning of USB hubs connected to the hub that's currently
>> being scanned will start earlier.
>>
>> 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>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Stephen Warren <swarren@nvidia.com>
>>
>> ---
>>
>> Changes in v4:
>> - Moved check for query_delay into usb_scan_port() as suggested by Hans
>> - Correct list handling (drop INIT_LIST_HEAD)
>> - Added some missing free() calls
>> - Changed connect_timeout calculation as suggested by Stephen
>> - Moved usb_scan_list to other globals to be cleaned up in a later patch
>> - Added timeout check for non-functional ports (usb_get_port_status
>>    return error
>> - Reverted if logic in loop to remove an indentation level
>> - Moved debug() output
>> - Removed unnecessary if when already connected
>> - Added Hans's Acked-by
>> - Added Stephen's Tested-by
>> - Minor rewording / fixes of the commit text
>>
>> Changes in v3:
>> - Introduced scanning list containing all USB devices of one USB
>>    controller that need to get scanned
>> - Don't delay in usb_hub_power_on(). Instead skip querying these devices
>>    until the delay time is reached.
>>
>> Changes in v2:
>> - Remove static USB port configuration patch (for now)
>>
>>   common/usb_hub.c | 293
>> ++++++++++++++++++++++++++++++++++++++-----------------
>>   include/usb.h    |   3 +
>>   2 files changed, 204 insertions(+), 92 deletions(-)
>>
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index d621f50..e8cfc7e 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -30,6 +30,7 @@
>>   #include <asm/processor.h>
>>   #include <asm/unaligned.h>
>>   #include <linux/ctype.h>
>> +#include <linux/list.h>
>>   #include <asm/byteorder.h>
>>   #ifdef CONFIG_SANDBOX
>>   #include <asm/state.h>
>> @@ -49,9 +50,17 @@ DECLARE_GLOBAL_DATA_PTR;
>>   #define HUB_SHORT_RESET_TIME    20
>>   #define HUB_LONG_RESET_TIME    200
>>
>> +struct usb_device_scan {
>> +    struct usb_device *dev;        /* USB hub device to scan */
>> +    struct usb_hub_device *hub;    /* USB hub struct */
>> +    int port;            /* USB port to scan */
>> +    struct list_head list;
>> +};
>> +
>>   /* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
>>   static struct usb_hub_device hub_dev[USB_MAX_HUB];
>>   static int usb_hub_index;
>> +static LIST_HEAD(usb_scan_list);
>>
>>   __weak void usb_hub_reset_devices(int port)
>>   {
>> @@ -120,7 +129,22 @@ 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 = dev->query_delay + 1000;
>> +    debug("devnum=%d poweron: query_delay=%d connect_timeout=%d\n",
>> +          dev->devnum, max(100, (int)pgood_delay),
>> +          max(100, (int)pgood_delay) + 1000);
>>   }
>>
>>   void usb_hub_reset(void)
>> @@ -332,6 +356,157 @@ int usb_hub_port_connect_change(struct
>> usb_device *dev, int port)
>>       return ret;
>>   }
>>
>> +static int usb_scan_port(struct usb_device_scan *usb_scan)
>> +{
>> +    ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>> +    unsigned short portstatus;
>> +    unsigned short portchange;
>> +    struct usb_device *dev;
>> +    struct usb_hub_device *hub;
>> +    int ret = 0;
>> +    int i;
>> +
>> +    dev = usb_scan->dev;
>> +    hub = usb_scan->hub;
>> +    i = usb_scan->port;
>> +
>> +    /*
>> +     * Don't talk to the device before the query delay is expired.
>> +     * This is needed for voltages to stabalize.
>> +     */
>> +    if (get_timer(0) < dev->query_delay)
>> +        return 0;
>> +
>> +#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
>
> We may now end up printing this a lot of times when debug printing
> is enabled, probably best to remove it (less is more :)

I already thought about it but wasn't brave enough. ;)

>> +    ret = usb_get_port_status(dev, i + 1, portsts);
>> +    if (ret < 0) {
>> +        debug("get_port_status failed\n");
>> +        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);
>> +            free(usb_scan);
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    portstatus = le16_to_cpu(portsts->wPortStatus);
>> +    portchange = le16_to_cpu(portsts->wPortChange);
>> +    debug("Port %d Status %X Change %X\n", i + 1, portstatus,
>> portchange);
>> +
>> +    /* No connection change happened, wait a bit more. */
>> +    if (!(portchange & USB_PORT_STAT_C_CONNECTION)) {
>> +        if (get_timer(0) >= 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);
>> +            free(usb_scan);
>> +            return 0;
>> +        }
>> +        return 0;
>> +    }
>> +
>> +    /* Test if the connection came up, and if not exit */
>> +    if (!(portstatus & USB_PORT_STAT_CONNECTION))
>> +        return 0;
>> +
>> +    /* A new USB device is ready at this point */
>> +    debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1);
>
> Just printing this in debug mode should be enough.

Yes. I've removed the 1st debug above in v5.

>> +    /* Remove this device from scanning list */
>> +    list_del(&usb_scan->list);
>> +    free(usb_scan);
>> +
>> +    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);
>
> This is the one why we need a v5, so usb_hub_power_on(hub) will reset the
> timeouts!

Good catch, thx.

> That by-itself is fine, if the over-current was caused by
> a spike, but if the over-current is permanent, we will just get
> an overcurrent report again after powering on the port again,
> rinse repeat -> never ending loop.

Not quite. Looking at the code, the device is already removed
from the list when this code patch is reached. So it would not be
scanned a 2nd time at all. Instead the scanning time for the other
devices connected to this hub (and not yet detected) are reset,
which is not quite what we want to achieve.

I should probably move the list_del()/free() call to the end of
the loop. This makes things clearer and more flexible. What do
you think?

> So there are 2 options:
>
> 1) On overcurrent remove the device from the list of devices to scan,
> the old code did the power-on after the loop with the delay, so it would
> actually not rescan the device after an overcurrent condition.
> Doing the power_on as the old code does seems useless in this case, we
> should probably still do it though, to reduce the amount of behavior
> changes
> and then remove it in a separate patch.

So thats what we should have with v4 already, right?

> 2) Do not reset the timeouts, do the power_on as the old code did and
> try to enumerate the device again.
>
> 3) reset the timeouts, but keep a per hub (or per port?) over-current
> counter
> and if it reaches say 3 then quit powering the port back on, and remove the
> port from the list of devices to scan instead.
>
> 1. keeps us closest to the original code behavior, but IMHO 3. is better.
> I think we can be lazy and just make the over-current counter a per hub
> thing rather then per port.

Once I move list_del() to the end of the loop, this should be easy to
implement. Should I aim for 3 with the per-hub counter?

> Note that the current code does:
>
>                  usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
>
> On all ports on over-current, that is something which we may want to fix
> too, but I believe that belongs in a separate follow-up patch.

Yes, it makes sense to do this not in this patchset.

>
>> +    }
>> +
>> +    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;
>> +
>> +            /* 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 +641,38 @@ 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;
>
> 1) poweron_timeout does not exist, looks like you did not do a test
> sandbox build

No, I missed this.

> 2) You should clear both connect_timeout and query_delay here, or better
> simply put:
>
>      if (state_get_skip_delays())
>          return;
>
> In usb_hub_power_on() before setting these (this assumes that usb_device
> gets kzalloc-ed).

Okay, will do.

>
>>   #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;
>> +        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;
>>
>
> I just realized: wouldn't it be better to put these 2 in struct
> usb_hub_device?
> you are tracking them per hub not per port, so they seem to belong there.

Good idea. Moved to hub in v5.

Thanks,
Stefan

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

[Hans de Goede]

Hi,

On 15-03-16 12:29, Stefan Roese wrote:
> Hi Hans,
>
> On 15.03.2016 11:44, Hans de Goede wrote:
>> Just noticed a few more things which need a minor
>> tweak, so looks like we're going to need a v5, sorry.
>>
>> On 15-03-16 10:46, Stefan Roese wrote:
>>> This patch changes the USB port scanning procedure and timeout
>>> handling in the following ways:
>>>
>>> a)
>>> The power-on delay in usb_hub_power_on() is now reduced to a value of
>>> max(100ms, "hub->desc.bPwrOn2PwrGood * 2"). The code does not wait
>>> using mdelay, instead usb_hub_power_on() will wait before querying
>>> the device in the scanning loop later. The total timeout for this
>>> hub, which is 1 second + "hub->desc.bPwrOn2PwrGood * 2" is calculated
>>> and will be used in the following per-port scanning loop as the timeout
>>> to detect active USB devices on this hub.
>>>
>>> b)
>>> Don't delay the minimum delay (for power to stabilize) in
>>> usb_hub_power_on(). Instead skip querying these devices in the scannig
>>> loop until the delay time is reached.
>>>
>>> c)
>>> The ports are now scanned in a quasi parallel way. The current code did
>>> wait for each (unconnected) port to reach its timeout and only then
>>> continue with the next port. This patch now changes this to scan all
>>> ports of all USB hubs quasi simultaneously. For this, all ports are added
>>> to a scanning list. This list is scanned until all ports are ready
>>> by either a) reaching the connection timeout (calculated earlier), or
>>> by b) detecting a USB device. This results in a faster USB scan time as
>>> the recursive scanning of USB hubs connected to the hub that's currently
>>> being scanned will start earlier.
>>>
>>> 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>
>>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>>> Tested-by: Stephen Warren <swarren@nvidia.com>
>>>
>>> ---
>>>
>>> Changes in v4:
>>> - Moved check for query_delay into usb_scan_port() as suggested by Hans
>>> - Correct list handling (drop INIT_LIST_HEAD)
>>> - Added some missing free() calls
>>> - Changed connect_timeout calculation as suggested by Stephen
>>> - Moved usb_scan_list to other globals to be cleaned up in a later patch
>>> - Added timeout check for non-functional ports (usb_get_port_status
>>>    return error
>>> - Reverted if logic in loop to remove an indentation level
>>> - Moved debug() output
>>> - Removed unnecessary if when already connected
>>> - Added Hans's Acked-by
>>> - Added Stephen's Tested-by
>>> - Minor rewording / fixes of the commit text
>>>
>>> Changes in v3:
>>> - Introduced scanning list containing all USB devices of one USB
>>>    controller that need to get scanned
>>> - Don't delay in usb_hub_power_on(). Instead skip querying these devices
>>>    until the delay time is reached.
>>>
>>> Changes in v2:
>>> - Remove static USB port configuration patch (for now)
>>>
>>>   common/usb_hub.c | 293
>>> ++++++++++++++++++++++++++++++++++++++-----------------
>>>   include/usb.h    |   3 +
>>>   2 files changed, 204 insertions(+), 92 deletions(-)
>>>
>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> index d621f50..e8cfc7e 100644
>>> --- a/common/usb_hub.c
>>> +++ b/common/usb_hub.c
>>> @@ -30,6 +30,7 @@
>>>   #include <asm/processor.h>
>>>   #include <asm/unaligned.h>
>>>   #include <linux/ctype.h>
>>> +#include <linux/list.h>
>>>   #include <asm/byteorder.h>
>>>   #ifdef CONFIG_SANDBOX
>>>   #include <asm/state.h>
>>> @@ -49,9 +50,17 @@ DECLARE_GLOBAL_DATA_PTR;
>>>   #define HUB_SHORT_RESET_TIME    20
>>>   #define HUB_LONG_RESET_TIME    200
>>>
>>> +struct usb_device_scan {
>>> +    struct usb_device *dev;        /* USB hub device to scan */
>>> +    struct usb_hub_device *hub;    /* USB hub struct */
>>> +    int port;            /* USB port to scan */
>>> +    struct list_head list;
>>> +};
>>> +
>>>   /* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
>>>   static struct usb_hub_device hub_dev[USB_MAX_HUB];
>>>   static int usb_hub_index;
>>> +static LIST_HEAD(usb_scan_list);
>>>
>>>   __weak void usb_hub_reset_devices(int port)
>>>   {
>>> @@ -120,7 +129,22 @@ 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 = dev->query_delay + 1000;
>>> +    debug("devnum=%d poweron: query_delay=%d connect_timeout=%d\n",
>>> +          dev->devnum, max(100, (int)pgood_delay),
>>> +          max(100, (int)pgood_delay) + 1000);
>>>   }
>>>
>>>   void usb_hub_reset(void)
>>> @@ -332,6 +356,157 @@ int usb_hub_port_connect_change(struct
>>> usb_device *dev, int port)
>>>       return ret;
>>>   }
>>>
>>> +static int usb_scan_port(struct usb_device_scan *usb_scan)
>>> +{
>>> +    ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>>> +    unsigned short portstatus;
>>> +    unsigned short portchange;
>>> +    struct usb_device *dev;
>>> +    struct usb_hub_device *hub;
>>> +    int ret = 0;
>>> +    int i;
>>> +
>>> +    dev = usb_scan->dev;
>>> +    hub = usb_scan->hub;
>>> +    i = usb_scan->port;
>>> +
>>> +    /*
>>> +     * Don't talk to the device before the query delay is expired.
>>> +     * This is needed for voltages to stabalize.
>>> +     */
>>> +    if (get_timer(0) < dev->query_delay)
>>> +        return 0;
>>> +
>>> +#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
>>
>> We may now end up printing this a lot of times when debug printing
>> is enabled, probably best to remove it (less is more :)
>
> I already thought about it but wasn't brave enough. ;)
>
>>> +    ret = usb_get_port_status(dev, i + 1, portsts);
>>> +    if (ret < 0) {
>>> +        debug("get_port_status failed\n");
>>> +        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);
>>> +            free(usb_scan);
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +    portstatus = le16_to_cpu(portsts->wPortStatus);
>>> +    portchange = le16_to_cpu(portsts->wPortChange);
>>> +    debug("Port %d Status %X Change %X\n", i + 1, portstatus,
>>> portchange);
>>> +
>>> +    /* No connection change happened, wait a bit more. */
>>> +    if (!(portchange & USB_PORT_STAT_C_CONNECTION)) {
>>> +        if (get_timer(0) >= 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);
>>> +            free(usb_scan);
>>> +            return 0;
>>> +        }
>>> +        return 0;
>>> +    }
>>> +
>>> +    /* Test if the connection came up, and if not exit */
>>> +    if (!(portstatus & USB_PORT_STAT_CONNECTION))
>>> +        return 0;
>>> +
>>> +    /* A new USB device is ready at this point */
>>> +    debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1);
>>
>> Just printing this in debug mode should be enough.
>
> Yes. I've removed the 1st debug above in v5.
>
>>> +    /* Remove this device from scanning list */
>>> +    list_del(&usb_scan->list);
>>> +    free(usb_scan);
>>> +
>>> +    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);
>>
>> This is the one why we need a v5, so usb_hub_power_on(hub) will reset the
>> timeouts!
>
> Good catch, thx.
>
>> That by-itself is fine, if the over-current was caused by
>> a spike, but if the over-current is permanent, we will just get
>> an overcurrent report again after powering on the port again,
>> rinse repeat -> never ending loop.
>
> Not quite. Looking at the code, the device is already removed
> from the list when this code patch is reached. So it would not be
> scanned a 2nd time at all. Instead the scanning time for the other
> devices connected to this hub (and not yet detected) are reset,
> which is not quite what we want to achieve.

Oh, right, so other then resetting the timeouts the current code
is behaving as the old one did on case of overcurrent.

I would replace the usb_hub_power_on(hub), with:

	usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);

That way you avoid resetting the timeout, and you fix the needlessly
re-setting of USB_PORT_FEAT_POWER on the other ports, and then the
code will behave just as before.

> I should probably move the list_del()/free() call to the end of
> the loop. This makes things clearer and more flexible. What do
> you think?

Ack for moving the list_del to the end of the loop.

>> So there are 2 options:
>>
>> 1) On overcurrent remove the device from the list of devices to scan,
>> the old code did the power-on after the loop with the delay, so it would
>> actually not rescan the device after an overcurrent condition.
>> Doing the power_on as the old code does seems useless in this case, we
>> should probably still do it though, to reduce the amount of behavior
>> changes
>> and then remove it in a separate patch.
>
> So thats what we should have with v4 already, right?

Right, I missed that.

>> 2) Do not reset the timeouts, do the power_on as the old code did and
>> try to enumerate the device again.
>>
>> 3) reset the timeouts, but keep a per hub (or per port?) over-current
>> counter
>> and if it reaches say 3 then quit powering the port back on, and remove the
>> port from the list of devices to scan instead.
>>
>> 1. keeps us closest to the original code behavior, but IMHO 3. is better.
>> I think we can be lazy and just make the over-current counter a per hub
>> thing rather then per port.
>
> Once I move list_del() to the end of the loop, this should be easy to
> implement.

I agree that it should be easy to implement, but since we only get in
this code-path when a device has actually already been detected, there
is no reason to re-probe I guess, so I would just replace the
usb_hub_power_on(hub), with:

	usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);

As I suggested above and be done with it. Esp. since we have no way to test
this.

 > Should I aim for 3 with the per-hub counter?

If you we're to implement some rescan then yes, but I would not bother.

>> Note that the current code does:
>>
>>                  usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
>>
>> On all ports on over-current, that is something which we may want to fix
>> too, but I believe that belongs in a separate follow-up patch.
>
> Yes, it makes sense to do this not in this patchset.

Except that doing it in this patch-set fixes the problem of
usb_hub_power_on() resetting the timeouts, so I vote for doing it
(and probably mention it in the commit msg).

An added advantage of just replacing the usb_hub_power_on() call
is that it does not invalidate all testing done with this patch-set
before now, whereas other fixes would require some more re-testing.

>>> +    }
>>> +
>>> +    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;
>>> +
>>> +            /* 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 +641,38 @@ 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;
>>
>> 1) poweron_timeout does not exist, looks like you did not do a test
>> sandbox build
>
> No, I missed this.
>
>> 2) You should clear both connect_timeout and query_delay here, or better
>> simply put:
>>
>>      if (state_get_skip_delays())
>>          return;
>>
>> In usb_hub_power_on() before setting these (this assumes that usb_device
>> gets kzalloc-ed).
>
> Okay, will do.

Please double-check that usb_device gets kzalloc-ed, or ...

>>>   #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;
>>> +        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;
>>>
>>
>> I just realized: wouldn't it be better to put these 2 in struct
>> usb_hub_device?
>> you are tracking them per hub not per port, so they seem to belong there.
>
> Good idea. Moved to hub in v5.

In that case you need to make sure that usb_hub_device gets zero-ed somewhere
explicitly (in case an array entry gets re-used on e.g. a "usb reset") or that the
timeouts always get set, even in the state_get_skip_delays() case.

Probably better to just zero it somewhere as that may avoid other surprises too,
usb_hub_reset() seems like a good candidate to just zero the entire array.

Regards,

Hans