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

[U-Boot] [PATCH 0/6] 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
- Introduce a static USB configuration method to skip specific
  USB ports while scanning

Even without the new static configuration (which can disable specified
ports), the resulting USB scanning time is greatly reduced:

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

time: 3.777 seconds

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

And with the static configuration, which can disable specific ports,
the time can be reduced even further.

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

This patch introduces the new configuration option CONFIG_USB_FAST_SCAN
to enable most of the speed enhancements. I'm actually not sure, if we
should introduce this config option at all. Or better unconditionally
apply all these changes to make this speedup default for all boards.
As this could break some USB configurations, it needs thorough testing
on many platforms before it becomes the default configuration.

Testing and comments welcome!

Thanks,
Stefan


Stefan Roese (6):
  usb: legacy_hub_port_reset(): Speedup hub reset handling
  usb: Remove 200 ms delay in usb_hub_port_connect_change()
  usb: Remove 1 second per port timeout in usb_hub_configure()
  usb: usb_hub_power_on(): Use 100ms power-on delay instead of 1 sec
    (optionally)
  usb: Don't reset the USB hub a 2nd time
  usb: Implement static USB port configuration to speed up USB scanning

 common/usb.c     |  6 ++++++
 common/usb_hub.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 2 deletions(-)

-- 
2.7.2

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

[Stefan Roese]

Start with a short USB hub reset delay of 10ms. 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>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Marek Vasut <marex@denx.de>
---

 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..10fdd3c 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	10
+#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.2

[U-Boot] [PATCH 2/6] 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.811 seconds

So ~3.5 seconds of USB scanning time reduction.

These mdelay calls are removed if CONFIG_USB_FAST_SCAN is defined. They
are not removed per default yet. It would be good to test with this
option enabled on many other boards. And once we have a good testing
base we can decide to remove these delays completely, including this
macro.

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

 common/usb_hub.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 10fdd3c..660f4f4 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -275,7 +275,9 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
 		if (!(portstatus & USB_PORT_STAT_CONNECTION))
 			return -ENOTCONN;
 	}
+#if !defined(CONFIG_USB_FAST_SCAN)
 	mdelay(200);
+#endif
 
 	/* Reset the port */
 	ret = legacy_hub_port_reset(dev, port, &portstatus);
@@ -285,7 +287,9 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
 		return ret;
 	}
 
+#if !defined(CONFIG_USB_FAST_SCAN)
 	mdelay(200);
+#endif
 
 	switch (portstatus & USB_PORT_STAT_SPEED_MASK) {
 	case USB_PORT_STAT_SUPER_SPEED:
-- 
2.7.2

[U-Boot] [PATCH 3/6] usb: Remove 1 second per port timeout in usb_hub_configure()

[Stefan Roese]

On complex USB infrastructures, with many hubs and therefore many
(perhaps unconnected) ports, current U-Boot has a very long USB scanning
time. On my current custom x86 board, this time is over 20 seconds!!!
One of the biggest problems here is a 1 second delay (timeout) in
usb_hub_configure() to check, if a USB device is connected to a
port. As this is done for each port, this timeout sums up over
all unconnected ports.

This patch removes this 1 seconds per port timeout from
usb_hub_configure(). All my USB devices are still detected correctly
on my platform. And I'm using 4 different USB keys right now.
Including a problematic "Intenso Alu Line" key.

Here the numbers for my current board:

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

time: 24.811 seconds

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

time: 10.822 seconds

So ~14 seconds of USB scanning time reduction.

Again, this timeout is only removed if CONFIG_USB_FAST_SCAN is
defined. Once more tests are done on multiple other platforms we
can decide to remove this timeout completely.

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

 common/usb_hub.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 660f4f4..780291f 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -478,7 +478,11 @@ static int usb_hub_configure(struct usb_device *dev)
 		unsigned short portstatus, portchange;
 		int ret;
 		ulong start = get_timer(0);
+#if defined(CONFIG_USB_FAST_SCAN)
+		uint delay = 0;
+#else
 		uint delay = CONFIG_SYS_HZ;
+#endif
 
 #ifdef CONFIG_SANDBOX
 		if (state_get_skip_delays())
-- 
2.7.2

[U-Boot] [PATCH 4/6] usb: usb_hub_power_on(): Use 100ms power-on delay instead of 1 sec (optionally)

[Stefan Roese]

In a system with a complex USB infrastrcture (many USB hubs), the
power-on delay of mininimum 1 second for each USB hub results in a quite
big USB scanning time. Many USB devices can deal with much lower
power-on delays. In my test system, even 10ms seems to be enough
for the USB device to get detected correctly. This patch now
reduces the minimum 1 second delay to a minumum of 100ms instead.
This results in a big USB scan time reduction:

Here the numbers for my current board:

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

time: 10.822 seconds

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

time: 6.319 seconds

So ~4.5 seconds of USB scanning time reduction.

I'm not really sure if this patch can be accepted in general as is.

In patch 0d437bca [usb: hub: fix power good delay timing] by Stephen
Warren, this delay was changes according to "Connect Timing ECN.pdf"
to a total of 1 second plus the hub port's power good time. Now its
changes back to 100ms which is a violation of this spec. But still,
for most USB devices this 100ms is more than enough. So its a valid
use-case to lower this time in special (perhaps static) USB
environments.

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

 common/usb_hub.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 780291f..d5fcd27 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -120,7 +120,11 @@ 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);
+#if defined(CONFIG_USB_FAST_SCAN)
+	mdelay(pgood_delay + 100);
+#else
 	mdelay(pgood_delay + 1000);
+#endif
 }
 
 void usb_hub_reset(void)
-- 
2.7.2

[U-Boot] [PATCH 5/6] 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 if CONFIG_USB_FAST_SCAN
is defined. Resulting in faster USB scan time. Here the current
number:

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

time: 6.319 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: 3.777 seconds

So ~2.5 seconds of USB scanning time reduction.

Again, this 2nd reset is only removed if CONFIG_USB_FAST_SCAN is
defined. Once more tests are done on multiple other platforms we
can decide to remove this 2nd reset completely.

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

 common/usb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/usb.c b/common/usb.c
index c7b8b0e..b96a2f6 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -920,6 +920,11 @@ __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) {
+#if !defined(CONFIG_USB_FAST_SCAN)
+		/*
+		 * Is this 2nd hub port reset necessary? The port has already
+		 * been reset in usb_hub_port_connect_change() before.
+		 */
 		unsigned short portstatus;
 		int err;
 
@@ -929,6 +934,7 @@ static int usb_hub_port_reset(struct usb_device *dev, struct usb_device *hub)
 			printf("\n     Couldn't reset port %i\n", dev->portnr);
 			return err;
 		}
+#endif
 	} else {
 		usb_reset_root_port(dev);
 	}
-- 
2.7.2

[U-Boot] [PATCH 6/6] usb: Implement static USB port configuration to speed up USB scanning

[Stefan Roese]

This patch implements an optionally quasi static USB port configuration.
This is done by using an environment variable, that describes the ports
that shall be scanned at the next USB scans (usb start, usb reset).

The "usb_port_use" env variable is used to describe this static USB
configuration. For this, each USB hub is represented by a 8-bit value.
Making it possible to configure a maximum of 8 ports for each USB hub.
A 64-bit representation is used, therefore 8 USB hubs can be described
in total.

Here an example for this "usb_port_use" environment variable:

usb_port_use = 0000000000040301
-------------------------------
1st USB hub: 0x01 -> Use port 1 (first port)
2nd USB hub: 0x03 -> Use port 1 and 2
3rd USB hub: 0x04 -> Use port 3
4th USB hub: 0x00 -> Use no ports
...
8th USB hub: 0x00 -> Use no ports

To make this procedure of configuring this env variable less error prone
and less painful, this patch also introduces another env variable that
is dynamically generated at each USB scan: "usb_port_detected". This
variable is similar to "usb_port_use". It has a bit enabled for each
port that has been detected. This can be easily used on a new system,
where the USB configuration is static in this way:

Run the USB scan (usb start, usb reset) without the "usb_port_use"
variable set. This will result in all ports being scanned and the result
written into the "usb_port_detected" variable. To configure the USB
subsystem to only scan these specific USB ports upon the next
scans, you only need to write the value from "usb_port_detected"
into the "usb_port_use" variable:

=> setenv usb_port_use ${usb_port_detected}
=> saveenv

The next scans will only scan those enabled ports.

Its of course also possible to manually "tune" this env variable. If
some ports are not needed in U-Boot, they can be disabled this way.
This will result in less USB hub ports getting scanned and therefore
in a faster USB scan time. Here an example:

With all USB devices enabled (usb_port_use not set):

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

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

With only some USB devices enabled:

=> setenv usb_port_use 0000000000000c01
=> time usb start
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 4 USB Device(s) found

time: 1.354 seconds
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |  u-boot EHCI Host Controller
  |
  +-2  Hub (480 Mb/s, 0mA)
    |
    +-3  Mass Storage (480 Mb/s, 200mA)
    |    6989 Intenso Alu Line EE706F5E
    |
    +-4  Mass Storage (480 Mb/s, 200mA)
         JetFlash Mass Storage Device 3281440601

So this feature of USB port enabling via environment variable is very
helpful to further reduce the USB scanning time in some configurations.

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>

---

 common/usb_hub.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index d5fcd27..b600cfa 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -353,6 +353,10 @@ static int usb_hub_configure(struct usb_device *dev)
 	struct usb_hub_descriptor *descriptor;
 	struct usb_hub_device *hub;
 	__maybe_unused struct usb_hub_status *hubsts;
+	static u64 port_active;
+	u32 port_use = 0xff;	/* Default: use all ports */
+	const char *env;
+	char str[18];
 	int ret;
 
 	/* "allocate" Hub device */
@@ -477,6 +481,18 @@ static int usb_hub_configure(struct usb_device *dev)
 	for (i = 0; i < dev->maxchild; i++)
 		usb_hub_reset_devices(i + 1);
 
+	/* Check if only configured ports shall be scanned / enabled */
+	env = getenv("usb_port_use");
+	if (env) {
+		port_use = (simple_strtoull(env, NULL, 16) >>
+			    ((dev->devnum - 1) * 8)) & 0xff;
+		debug("port_usb[%d]=0x%02x\n", dev->devnum, port_use);
+	}
+
+	/* Reset port_active variable on the scan of the 1st USB hub */
+	if (dev->devnum == 1)
+		port_active = 0;
+
 	for (i = 0; i < dev->maxchild; i++) {
 		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 		unsigned short portstatus, portchange;
@@ -497,6 +513,23 @@ static int usb_hub_configure(struct usb_device *dev)
 #else
 		debug("\n\nScanning port %d\n", i + 1);
 #endif
+
+		/*
+		 * Check if this port should be used. This can be configured
+		 * via the "usb_port_use" env variable in a flexible way. Here
+		 * an example:
+		 *
+		 * usb_port_use = 0000000004000301
+		 * 1st USB hub: 0x01 -> Use port 1 (first port)
+		 * 2nd USB hub: 0x03 -> Use port 1 and 2
+		 * 3rd USB hub: 0x00 -> Use no ports
+		 * 4th USB hub: 0x04 -> Use port 3
+		 */
+		if (!(port_use & BIT(i))) {
+			debug("Skipping port %d\n", i + 1);
+			continue;
+		}
+
 		/*
 		 * Wait for (whichever finishes first)
 		 *  - A maximum of 10 seconds
@@ -534,6 +567,9 @@ static int usb_hub_configure(struct usb_device *dev)
 		if (portchange & USB_PORT_STAT_C_CONNECTION) {
 			debug("port %d connection change\n", i + 1);
 			usb_hub_port_connect_change(dev, i);
+
+			/* Save the port as active */
+			port_active |= (u64)(BIT(i)) << ((dev->devnum - 1) * 8);
 		}
 		if (portchange & USB_PORT_STAT_C_ENABLE) {
 			debug("port %d enable change, status %x\n",
@@ -578,6 +614,14 @@ static int usb_hub_configure(struct usb_device *dev)
 		}
 	} /* end for i all ports */
 
+	/*
+	 * Save the active ports in the "usb_port_detected" env variable.
+	 * This can be used by the user to save this into the "usb_port_use"
+	 * variable to only scan the statically available ports.
+	 */
+	sprintf(str, "%016llx", port_active);
+	setenv("usb_port_detected", str);
+
 	return 0;
 }
 
-- 
2.7.2

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

[Hans de Goede]

Hi,

On 10-03-16 16:50, Stefan Roese wrote:
> Start with a short USB hub reset delay of 10ms. 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>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Marek Vasut <marex@denx.de>

Since this is good enough for the kernel it should be good enough for us:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>
>   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..10fdd3c 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	10
> +#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) {
>

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

[Hans de Goede]

Hi,

On 10-03-16 16:50, 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.811 seconds
>
> So ~3.5 seconds of USB scanning time reduction.
>
> These mdelay calls are removed if CONFIG_USB_FAST_SCAN is defined. They
> are not removed per default yet. It would be good to test with this
> option enabled on many other boards. And once we have a good testing
> base we can decide to remove these delays completely, including this
> macro.

There indeed is no reason at all to delay before the reset and the kernel
does not wait with checking the USB_PORT_STAT_SPEED_MASK once the
reset completes, so I see no reason why we should:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

>
> 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>
> ---
>
>   common/usb_hub.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 10fdd3c..660f4f4 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -275,7 +275,9 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
>   		if (!(portstatus & USB_PORT_STAT_CONNECTION))
>   			return -ENOTCONN;
>   	}
> +#if !defined(CONFIG_USB_FAST_SCAN)
>   	mdelay(200);
> +#endif
>
>   	/* Reset the port */
>   	ret = legacy_hub_port_reset(dev, port, &portstatus);
> @@ -285,7 +287,9 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port)
>   		return ret;
>   	}
>
> +#if !defined(CONFIG_USB_FAST_SCAN)
>   	mdelay(200);
> +#endif
>
>   	switch (portstatus & USB_PORT_STAT_SPEED_MASK) {
>   	case USB_PORT_STAT_SUPER_SPEED:
>

[U-Boot] [PATCH 3/6] usb: Remove 1 second per port timeout in usb_hub_configure()

[Hans de Goede]

Hi,

On 10-03-16 16:50, Stefan Roese wrote:
> On complex USB infrastructures, with many hubs and therefore many
> (perhaps unconnected) ports, current U-Boot has a very long USB scanning
> time. On my current custom x86 board, this time is over 20 seconds!!!
> One of the biggest problems here is a 1 second delay (timeout) in
> usb_hub_configure() to check, if a USB device is connected to a
> port. As this is done for each port, this timeout sums up over
> all unconnected ports.
>
> This patch removes this 1 seconds per port timeout from
> usb_hub_configure().

If you look at the commit which added the delay, you will see the
need for this delay clearly documented. The USB spec gives devices
up to one second to connect after they get power, and some devices
actually use almost that entire second.

Sure what you're doing will work most of the time, but works most
of the time is not really helpful, and will just cause hard to
debug problems.

Instead what we need to do is implement parallel usb scanning,
I've done some suggestions on how to do this here:

http://lists.denx.de/pipermail/u-boot/2016-February/245243.html

Regards,

Hans


> All my USB devices are still detected correctly
> on my platform. And I'm using 4 different USB keys right now.
> Including a problematic "Intenso Alu Line" key.
>
> Here the numbers for my current board:
>
> Without this patch:
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 24.811 seconds
>
> With this patch:
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 10.822 seconds
>
> So ~14 seconds of USB scanning time reduction.
>
> Again, this timeout is only removed if CONFIG_USB_FAST_SCAN is
> defined. Once more tests are done on multiple other platforms we
> can decide to remove this timeout completely.
>
> 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>
> ---
>
>   common/usb_hub.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 660f4f4..780291f 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -478,7 +478,11 @@ static int usb_hub_configure(struct usb_device *dev)
>   		unsigned short portstatus, portchange;
>   		int ret;
>   		ulong start = get_timer(0);
> +#if defined(CONFIG_USB_FAST_SCAN)
> +		uint delay = 0;
> +#else
>   		uint delay = CONFIG_SYS_HZ;
> +#endif
>
>   #ifdef CONFIG_SANDBOX
>   		if (state_get_skip_delays())
>

[U-Boot] [PATCH 4/6] usb: usb_hub_power_on(): Use 100ms power-on delay instead of 1 sec (optionally)

[Hans de Goede]

Hi,

On 10-03-16 16:50, Stefan Roese wrote:
> In a system with a complex USB infrastrcture (many USB hubs), the
> power-on delay of mininimum 1 second for each USB hub results in a quite
> big USB scanning time. Many USB devices can deal with much lower
> power-on delays. In my test system, even 10ms seems to be enough
> for the USB device to get detected correctly. This patch now
> reduces the minimum 1 second delay to a minumum of 100ms instead.
> This results in a big USB scan time reduction:
>
> Here the numbers for my current board:
>
> Without this patch:
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 10.822 seconds
>
> With this patch:
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 6.319 seconds
>
> So ~4.5 seconds of USB scanning time reduction.
>
> I'm not really sure if this patch can be accepted in general as is.
>
> In patch 0d437bca [usb: hub: fix power good delay timing] by Stephen
> Warren, this delay was changes according to "Connect Timing ECN.pdf"
> to a total of 1 second plus the hub port's power good time. Now its
> changes back to 100ms which is a violation of this spec. But still,
> for most USB devices this 100ms is more than enough. So its a valid
> use-case to lower this time in special (perhaps static) USB
> environments.

Actually that 1 second + poweron delay is why we had the 1 sec
delay in usb_hub_configure(), that is where we wait for a device
to show up. Note that we can already save a lot of time, by
first powering up all ports and then doing the 1 sec wait
and then checking all ports without needing to delay any further.

Or even better:

1) turn on all ports
2) do power-on-delay (either 2ms * bPwrOn2PwrGood from descriptor, or
    100ms which ever one is LARGER)
3) set a timeout val 1 sec from now, loop over all ports and quit
    the loop when either all ports are connected (we already skip the
    per port delay in this connected case now), or when the 1 sec
    expires. There is no reason for the 1 sec per port delay,
    one sec + bPwrOn2PwrGood delay is enough for all ports

###

Note that even better would be to in 3 first calls some sort of
probe_prepare function, which if the child is a hub itself, does
the poweron, and records the timeout for its loop and runs its
loop once (to probe_prepare its children if they come up immediately).

And then only once the loop over all ports ends, call the real
probe which will then do usb_hub_configure() for its (hub)
children using the timeout recorded during the probe_prepare,
and will thus likely be able to skip a large part of that
1 sec waiting since that was already waited in the parent
hub's usb_hub_configure() call.

Note this would speed up scanning deep trees of hubs, if
we have multiple usb controllers, it would also be really
good to probe them in parallel as described here:

http://lists.denx.de/pipermail/u-boot/2016-February/245243.html

(yes I'm repeating myself :)

Regards,

Hans







>
> 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>
> ---
>
>   common/usb_hub.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 780291f..d5fcd27 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -120,7 +120,11 @@ 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);
> +#if defined(CONFIG_USB_FAST_SCAN)
> +	mdelay(pgood_delay + 100);
> +#else
>   	mdelay(pgood_delay + 1000);
> +#endif
>   }
>
>   void usb_hub_reset(void)
>

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

[Hans de Goede]

Hi,

On 10-03-16 16:50, Stefan Roese wrote:
> 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 if CONFIG_USB_FAST_SCAN
> is defined. Resulting in faster USB scan time. Here the current
> number:
>
> Without this patch:
> => time usb start
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 6.319 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: 3.777 seconds
>
> So ~2.5 seconds of USB scanning time reduction.
>
> Again, this 2nd reset is only removed if CONFIG_USB_FAST_SCAN is
> defined. Once more tests are done on multiple other platforms we
> can decide to remove this 2nd reset completely.

I see no reason to make the removal conditional, I believe that
this is some relic workaround for likely long fixed bugs and
we should just remove it completely (for v2016.07).

Regards,

Hans


>
> 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>
> ---
>
>   common/usb.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/common/usb.c b/common/usb.c
> index c7b8b0e..b96a2f6 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -920,6 +920,11 @@ __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) {
> +#if !defined(CONFIG_USB_FAST_SCAN)
> +		/*
> +		 * Is this 2nd hub port reset necessary? The port has already
> +		 * been reset in usb_hub_port_connect_change() before.
> +		 */
>   		unsigned short portstatus;
>   		int err;
>
> @@ -929,6 +934,7 @@ static int usb_hub_port_reset(struct usb_device *dev, struct usb_device *hub)
>   			printf("\n     Couldn't reset port %i\n", dev->portnr);
>   			return err;
>   		}
> +#endif
>   	} else {
>   		usb_reset_root_port(dev);
>   	}
>

[U-Boot] [PATCH 6/6] usb: Implement static USB port configuration to speed up USB scanning

[Hans de Goede]

Hi,

On 10-03-16 16:50, Stefan Roese wrote:
> This patch implements an optionally quasi static USB port configuration.
> This is done by using an environment variable, that describes the ports
> that shall be scanned at the next USB scans (usb start, usb reset).
>
> The "usb_port_use" env variable is used to describe this static USB
> configuration. For this, each USB hub is represented by a 8-bit value.
> Making it possible to configure a maximum of 8 ports for each USB hub.
> A 64-bit representation is used, therefore 8 USB hubs can be described
> in total.
>
> Here an example for this "usb_port_use" environment variable:
>
> usb_port_use = 0000000000040301
> -------------------------------
> 1st USB hub: 0x01 -> Use port 1 (first port)
> 2nd USB hub: 0x03 -> Use port 1 and 2
> 3rd USB hub: 0x04 -> Use port 3
> 4th USB hub: 0x00 -> Use no ports
> ...
> 8th USB hub: 0x00 -> Use no ports
>
> To make this procedure of configuring this env variable less error prone
> and less painful, this patch also introduces another env variable that
> is dynamically generated at each USB scan: "usb_port_detected". This
> variable is similar to "usb_port_use". It has a bit enabled for each
> port that has been detected. This can be easily used on a new system,
> where the USB configuration is static in this way:
>
> Run the USB scan (usb start, usb reset) without the "usb_port_use"
> variable set. This will result in all ports being scanned and the result
> written into the "usb_port_detected" variable. To configure the USB
> subsystem to only scan these specific USB ports upon the next
> scans, you only need to write the value from "usb_port_detected"
> into the "usb_port_use" variable:
>
> => setenv usb_port_use ${usb_port_detected}
> => saveenv
>
> The next scans will only scan those enabled ports.
>
> Its of course also possible to manually "tune" this env variable. If
> some ports are not needed in U-Boot, they can be disabled this way.
> This will result in less USB hub ports getting scanned and therefore
> in a faster USB scan time. Here an example:
>
> With all USB devices enabled (usb_port_use not set):

This will fall apart when you get multiple root hubs,
if you want to do this (I believe there is much more low hanging fruit
see my previous mails), you somehow need to describe the entire
path to the hub in the env variable, currently if one hub gets removed,
other hubs which are children of the same parent will get a different
number in your hub-numbering scheme and things go awry, also what about hubs
on a second host controller, do those number on from the hub-numbering of
the first hcd ? That seems vary fragile and will break when we add
(semi) parallel scanning.

So NACK because this disallows later implementing parallel scanning
without regressing this feature.

Regards,

Hans



>
> => setenv usb_port_use
> => time usb start
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 3.776 seconds
> => usb tree
> USB device tree:
>    1  Hub (480 Mb/s, 0mA)
>    |  u-boot EHCI Host Controller
>    |
>    +-2  Hub (480 Mb/s, 0mA)
>      |
>      +-8  Mass Storage (480 Mb/s, 200mA)
>      |    6989 Intenso Alu Line EE706F5E
>      |
>      +-9  Mass Storage (480 Mb/s, 200mA)
>      |    JetFlash Mass Storage Device 3281440601
>      |
>      +-3  Hub (480 Mb/s, 100mA)
>      | |
>      | +-4  Hub (12 Mb/s, 100mA)
>      |
>      +-5  Hub (480 Mb/s, 0mA)
>        |
>        +-6  Mass Storage (480 Mb/s, 200mA)
>          |    Kingston DataTraveler 2.0 50E549C688C4BE7189942766
>          |
>          +-7  Mass Storage (480 Mb/s, 98mA)
>               USBest Technology USB Mass Storage Device 09092207fbf0c4
> => printenv usb_port_detected
> usb_port_detected=0000000501080f01
>
> With only some USB devices enabled:
>
> => setenv usb_port_use 0000000000000c01
> => time usb start
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 4 USB Device(s) found
>
> time: 1.354 seconds
> => usb tree
> USB device tree:
>    1  Hub (480 Mb/s, 0mA)
>    |  u-boot EHCI Host Controller
>    |
>    +-2  Hub (480 Mb/s, 0mA)
>      |
>      +-3  Mass Storage (480 Mb/s, 200mA)
>      |    6989 Intenso Alu Line EE706F5E
>      |
>      +-4  Mass Storage (480 Mb/s, 200mA)
>           JetFlash Mass Storage Device 3281440601
>
> So this feature of USB port enabling via environment variable is very
> helpful to further reduce the USB scanning time in some configurations.
>
> 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>
>
> ---
>
>   common/usb_hub.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index d5fcd27..b600cfa 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -353,6 +353,10 @@ static int usb_hub_configure(struct usb_device *dev)
>   	struct usb_hub_descriptor *descriptor;
>   	struct usb_hub_device *hub;
>   	__maybe_unused struct usb_hub_status *hubsts;
> +	static u64 port_active;
> +	u32 port_use = 0xff;	/* Default: use all ports */
> +	const char *env;
> +	char str[18];
>   	int ret;
>
>   	/* "allocate" Hub device */
> @@ -477,6 +481,18 @@ static int usb_hub_configure(struct usb_device *dev)
>   	for (i = 0; i < dev->maxchild; i++)
>   		usb_hub_reset_devices(i + 1);
>
> +	/* Check if only configured ports shall be scanned / enabled */
> +	env = getenv("usb_port_use");
> +	if (env) {
> +		port_use = (simple_strtoull(env, NULL, 16) >>
> +			    ((dev->devnum - 1) * 8)) & 0xff;
> +		debug("port_usb[%d]=0x%02x\n", dev->devnum, port_use);
> +	}
> +
> +	/* Reset port_active variable on the scan of the 1st USB hub */
> +	if (dev->devnum == 1)
> +		port_active = 0;
> +
>   	for (i = 0; i < dev->maxchild; i++) {
>   		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>   		unsigned short portstatus, portchange;
> @@ -497,6 +513,23 @@ static int usb_hub_configure(struct usb_device *dev)
>   #else
>   		debug("\n\nScanning port %d\n", i + 1);
>   #endif
> +
> +		/*
> +		 * Check if this port should be used. This can be configured
> +		 * via the "usb_port_use" env variable in a flexible way. Here
> +		 * an example:
> +		 *
> +		 * usb_port_use = 0000000004000301
> +		 * 1st USB hub: 0x01 -> Use port 1 (first port)
> +		 * 2nd USB hub: 0x03 -> Use port 1 and 2
> +		 * 3rd USB hub: 0x00 -> Use no ports
> +		 * 4th USB hub: 0x04 -> Use port 3
> +		 */
> +		if (!(port_use & BIT(i))) {
> +			debug("Skipping port %d\n", i + 1);
> +			continue;
> +		}
> +
>   		/*
>   		 * Wait for (whichever finishes first)
>   		 *  - A maximum of 10 seconds
> @@ -534,6 +567,9 @@ static int usb_hub_configure(struct usb_device *dev)
>   		if (portchange & USB_PORT_STAT_C_CONNECTION) {
>   			debug("port %d connection change\n", i + 1);
>   			usb_hub_port_connect_change(dev, i);
> +
> +			/* Save the port as active */
> +			port_active |= (u64)(BIT(i)) << ((dev->devnum - 1) * 8);
>   		}
>   		if (portchange & USB_PORT_STAT_C_ENABLE) {
>   			debug("port %d enable change, status %x\n",
> @@ -578,6 +614,14 @@ static int usb_hub_configure(struct usb_device *dev)
>   		}
>   	} /* end for i all ports */
>
> +	/*
> +	 * Save the active ports in the "usb_port_detected" env variable.
> +	 * This can be used by the user to save this into the "usb_port_use"
> +	 * variable to only scan the statically available ports.
> +	 */
> +	sprintf(str, "%016llx", port_active);
> +	setenv("usb_port_detected", str);
> +
>   	return 0;
>   }
>
>

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

[Stephen Warren]

On 03/10/2016 08:50 AM, Stefan Roese wrote:
> Start with a short USB hub reset delay of 10ms. 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().

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

(Tested with 1 USB SD card reader, 1 USB Ethernet dongle, 4 USB flash 
drives, which IIRC includes some of the problematic devices that 
triggered previous changes to the USB enumeration code in U-Boot)

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

[Stephen Warren]

On 03/10/2016 08:50 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.811 seconds
>
> So ~3.5 seconds of USB scanning time reduction.
>
> These mdelay calls are removed if CONFIG_USB_FAST_SCAN is defined. They
> are not removed per default yet. It would be good to test with this
> option enabled on many other boards. And once we have a good testing
> base we can decide to remove these delays completely, including this
> macro.

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

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

[Stephen Warren]

On 03/10/2016 08:50 AM, Stefan Roese wrote:
> 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 if CONFIG_USB_FAST_SCAN
> is defined. Resulting in faster USB scan time. Here the current
> number:
>
> Without this patch:
> => time usb start
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
>
> time: 6.319 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: 3.777 seconds
>
> So ~2.5 seconds of USB scanning time reduction.
>
> Again, this 2nd reset is only removed if CONFIG_USB_FAST_SCAN is
> defined. Once more tests are done on multiple other platforms we
> can decide to remove this 2nd reset completely.

This patch, after modifying the patch to unconditionally remove the code 
rather than only conditionally,
Tested-by: Stephen Warren <swarren@nvidia.com>

For the other patches I agree with Hans' comments.

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

[Stefan Roese]

On 10.03.2016 19:55, Hans de Goede wrote:
> Hi,
>
> On 10-03-16 16:50, 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.811 seconds
>>
>> So ~3.5 seconds of USB scanning time reduction.
>>
>> These mdelay calls are removed if CONFIG_USB_FAST_SCAN is defined. They
>> are not removed per default yet. It would be good to test with this
>> option enabled on many other boards. And once we have a good testing
>> base we can decide to remove these delays completely, including this
>> macro.
>
> There indeed is no reason at all to delay before the reset and the kernel
> does not wait with checking the USB_PORT_STAT_SPEED_MASK once the
> reset completes, so I see no reason why we should:
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks. I'll remove the #if !defined(CONFIG_USB_FAST_SCAN) to make
this change default in the next patchset version.

Thanks,
Stefan

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

[Stefan Roese]

On 10.03.2016 19:55, Hans de Goede wrote:
> Hi,
>
> On 10-03-16 16:50, 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.811 seconds
>>
>> So ~3.5 seconds of USB scanning time reduction.
>>
>> These mdelay calls are removed if CONFIG_USB_FAST_SCAN is defined. They
>> are not removed per default yet. It would be good to test with this
>> option enabled on many other boards. And once we have a good testing
>> base we can decide to remove these delays completely, including this
>> macro.
>
> There indeed is no reason at all to delay before the reset and the kernel
> does not wait with checking the USB_PORT_STAT_SPEED_MASK once the
> reset completes, so I see no reason why we should:
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks. Again, I'll make this change unconditionally in the next
patchset version.

Thanks,
Stefan

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

[Stefan Roese]

On 10.03.2016 19:51, Hans de Goede wrote:
> Hi,
>
> On 10-03-16 16:50, Stefan Roese wrote:
>> Start with a short USB hub reset delay of 10ms. 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>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> Cc: Marek Vasut <marex@denx.de>
>
> Since this is good enough for the kernel it should be good enough for us:
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks, I'll will make this unconditional in the next patchset version.

Thanks,
Stefan

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

[Stefan Roese]

On 10.03.2016 20:13, Hans de Goede wrote:
> Hi,
>
> On 10-03-16 16:50, Stefan Roese wrote:
>> 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 if CONFIG_USB_FAST_SCAN
>> is defined. Resulting in faster USB scan time. Here the current
>> number:
>>
>> Without this patch:
>> => time usb start
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 6.319 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: 3.777 seconds
>>
>> So ~2.5 seconds of USB scanning time reduction.
>>
>> Again, this 2nd reset is only removed if CONFIG_USB_FAST_SCAN is
>> defined. Once more tests are done on multiple other platforms we
>> can decide to remove this 2nd reset completely.
>
> I see no reason to make the removal conditional, I believe that
> this is some relic workaround for likely long fixed bugs and
> we should just remove it completely (for v2016.07).

Thanks. I'll make this change unconditional in the next patch version
as well.

Thanks,
Stefan

[U-Boot] [PATCH 4/6] usb: usb_hub_power_on(): Use 100ms power-on delay instead of 1 sec (optionally)

[Stefan Roese]

On 10.03.2016 20:12, Hans de Goede wrote:
> Hi,
>
> On 10-03-16 16:50, Stefan Roese wrote:
>> In a system with a complex USB infrastrcture (many USB hubs), the
>> power-on delay of mininimum 1 second for each USB hub results in a quite
>> big USB scanning time. Many USB devices can deal with much lower
>> power-on delays. In my test system, even 10ms seems to be enough
>> for the USB device to get detected correctly. This patch now
>> reduces the minimum 1 second delay to a minumum of 100ms instead.
>> This results in a big USB scan time reduction:
>>
>> Here the numbers for my current board:
>>
>> Without this patch:
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 10.822 seconds
>>
>> With this patch:
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 6.319 seconds
>>
>> So ~4.5 seconds of USB scanning time reduction.
>>
>> I'm not really sure if this patch can be accepted in general as is.
>>
>> In patch 0d437bca [usb: hub: fix power good delay timing] by Stephen
>> Warren, this delay was changes according to "Connect Timing ECN.pdf"
>> to a total of 1 second plus the hub port's power good time. Now its
>> changes back to 100ms which is a violation of this spec. But still,
>> for most USB devices this 100ms is more than enough. So its a valid
>> use-case to lower this time in special (perhaps static) USB
>> environments.
>
> Actually that 1 second + poweron delay is why we had the 1 sec
> delay in usb_hub_configure(), that is where we wait for a device
> to show up. Note that we can already save a lot of time, by
> first powering up all ports and then doing the 1 sec wait
> and then checking all ports without needing to delay any further.
>
> Or even better:
>
> 1) turn on all ports
> 2) do power-on-delay (either 2ms * bPwrOn2PwrGood from descriptor, or
>     100ms which ever one is LARGER)
> 3) set a timeout val 1 sec from now, loop over all ports and quit
>     the loop when either all ports are connected (we already skip the
>     per port delay in this connected case now), or when the 1 sec
>     expires. There is no reason for the 1 sec per port delay,
>     one sec + bPwrOn2PwrGood delay is enough for all ports

Right. There's definitely room for improvement and optimization
here.

> ###
>
> Note that even better would be to in 3 first calls some sort of
> probe_prepare function, which if the child is a hub itself, does
> the poweron, and records the timeout for its loop and runs its
> loop once (to probe_prepare its children if they come up immediately).
>
> And then only once the loop over all ports ends, call the real
> probe which will then do usb_hub_configure() for its (hub)
> children using the timeout recorded during the probe_prepare,
> and will thus likely be able to skip a large part of that
> 1 sec waiting since that was already waited in the parent
> hub's usb_hub_configure() call.
>
> Note this would speed up scanning deep trees of hubs, if
> we have multiple usb controllers, it would also be really
> good to probe them in parallel as described here:
>
> http://lists.denx.de/pipermail/u-boot/2016-February/245243.html
>
> (yes I'm repeating myself :)

Thanks for your comments Hans. Lets see, what I can come up with in
next patchset version.

Thanks,
Stefan

[U-Boot] [PATCH 4/6] usb: usb_hub_power_on(): Use 100ms power-on delay instead of 1 sec (optionally)

[Stefan Roese]

Hi Hans,

On 10.03.2016 20:12, Hans de Goede wrote:
> On 10-03-16 16:50, Stefan Roese wrote:
>> In a system with a complex USB infrastrcture (many USB hubs), the
>> power-on delay of mininimum 1 second for each USB hub results in a quite
>> big USB scanning time. Many USB devices can deal with much lower
>> power-on delays. In my test system, even 10ms seems to be enough
>> for the USB device to get detected correctly. This patch now
>> reduces the minimum 1 second delay to a minumum of 100ms instead.
>> This results in a big USB scan time reduction:
>>
>> Here the numbers for my current board:
>>
>> Without this patch:
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 10.822 seconds
>>
>> With this patch:
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 6.319 seconds
>>
>> So ~4.5 seconds of USB scanning time reduction.
>>
>> I'm not really sure if this patch can be accepted in general as is.
>>
>> In patch 0d437bca [usb: hub: fix power good delay timing] by Stephen
>> Warren, this delay was changes according to "Connect Timing ECN.pdf"
>> to a total of 1 second plus the hub port's power good time. Now its
>> changes back to 100ms which is a violation of this spec. But still,
>> for most USB devices this 100ms is more than enough. So its a valid
>> use-case to lower this time in special (perhaps static) USB
>> environments.
>
> Actually that 1 second + poweron delay is why we had the 1 sec
> delay in usb_hub_configure(), that is where we wait for a device
> to show up. Note that we can already save a lot of time, by
> first powering up all ports and then doing the 1 sec wait
> and then checking all ports without needing to delay any further.
>
> Or even better:
>
> 1) turn on all ports
> 2) do power-on-delay (either 2ms * bPwrOn2PwrGood from descriptor, or
>     100ms which ever one is LARGER)
> 3) set a timeout val 1 sec from now, loop over all ports and quit
>     the loop when either all ports are connected (we already skip the
>     per port delay in this connected case now), or when the 1 sec
>     expires. There is no reason for the 1 sec per port delay,
>     one sec + bPwrOn2PwrGood delay is enough for all ports

One question here: Do we really need to do this power-on-delay in
step 2) at all? Shouldn't it be sufficient to do the port scanning
loop with a "2ms * bPwrOn2PwrGood + 1 sec" timeout instead? As
I've done in the patch I've attached (which works just fine on
my platform).

Note that I will also dig into the parallelizing idea as well. I'm
just trying to implement the timeout handling correctly first.

Thanks,
Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-usb-Change-power-on-scanning-timeout-handling.patch
Type: text/x-diff
Size: 2656 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160311/0d17a2ab/attachment.patch>

[U-Boot] [PATCH 4/6] usb: usb_hub_power_on(): Use 100ms power-on delay instead of 1 sec (optionally)

[Hans de Goede]

Hi,

On 11-03-16 11:13, Stefan Roese wrote:
> Hi Hans,
>
> On 10.03.2016 20:12, Hans de Goede wrote:
>> On 10-03-16 16:50, Stefan Roese wrote:
>>> In a system with a complex USB infrastrcture (many USB hubs), the
>>> power-on delay of mininimum 1 second for each USB hub results in a quite
>>> big USB scanning time. Many USB devices can deal with much lower
>>> power-on delays. In my test system, even 10ms seems to be enough
>>> for the USB device to get detected correctly. This patch now
>>> reduces the minimum 1 second delay to a minumum of 100ms instead.
>>> This results in a big USB scan time reduction:
>>>
>>> Here the numbers for my current board:
>>>
>>> Without this patch:
>>> starting USB...
>>> USB0:   USB EHCI 1.00
>>> scanning bus 0 for devices... 9 USB Device(s) found
>>>
>>> time: 10.822 seconds
>>>
>>> With this patch:
>>> starting USB...
>>> USB0:   USB EHCI 1.00
>>> scanning bus 0 for devices... 9 USB Device(s) found
>>>
>>> time: 6.319 seconds
>>>
>>> So ~4.5 seconds of USB scanning time reduction.
>>>
>>> I'm not really sure if this patch can be accepted in general as is.
>>>
>>> In patch 0d437bca [usb: hub: fix power good delay timing] by Stephen
>>> Warren, this delay was changes according to "Connect Timing ECN.pdf"
>>> to a total of 1 second plus the hub port's power good time. Now its
>>> changes back to 100ms which is a violation of this spec. But still,
>>> for most USB devices this 100ms is more than enough. So its a valid
>>> use-case to lower this time in special (perhaps static) USB
>>> environments.
>>
>> Actually that 1 second + poweron delay is why we had the 1 sec
>> delay in usb_hub_configure(), that is where we wait for a device
>> to show up. Note that we can already save a lot of time, by
>> first powering up all ports and then doing the 1 sec wait
>> and then checking all ports without needing to delay any further.
>>
>> Or even better:
>>
>> 1) turn on all ports
>> 2) do power-on-delay (either 2ms * bPwrOn2PwrGood from descriptor, or
>>     100ms which ever one is LARGER)
>> 3) set a timeout val 1 sec from now, loop over all ports and quit
>>     the loop when either all ports are connected (we already skip the
>>     per port delay in this connected case now), or when the 1 sec
>>     expires. There is no reason for the 1 sec per port delay,
>>     one sec + bPwrOn2PwrGood delay is enough for all ports
>
> One question here: Do we really need to do this power-on-delay in
> step 2) at all?

Yes the power may bounce during this time, causing a device connect
+ disconnect + connect again, we really need to wait for the power
to be stable before looking at / talking to connected devices.

Note this is also what the kernel does, it ignores the hub after
powering its ports for this time.

> Shouldn't it be sufficient to do the port scanning
> loop with a "2ms * bPwrOn2PwrGood + 1 sec" timeout instead? As
> I've done in the patch I've attached (which works just fine on
> my platform).
>
> Note that I will also dig into the parallelizing idea as well. I'm
> just trying to implement the timeout handling correctly first.

Cool :)

About the patch, you're waiting up to 1 sec for the first port and
then scanning the other ports without waiting. For the initial
implementation this is fine, but for the parallelization of child
probing you will want to scan all ports as fast as possible, skipping
already connected and handled ports for the duration of 1 sec, so as to
also scan the children as soon as they are connected (and not delay
scanning a hub on port 2 because port 1 does not have a device connected).

Nice improvement in scan time from this simple patch btw :)

Regards,

Hans

[U-Boot] [PATCH 4/6] usb: usb_hub_power_on(): Use 100ms power-on delay instead of 1 sec (optionally)

[Stefan Roese]

Hi Hans,

On 11.03.2016 11:32, Hans de Goede wrote:
> Hi,
>
> On 11-03-16 11:13, Stefan Roese wrote:
>> Hi Hans,
>>
>> On 10.03.2016 20:12, Hans de Goede wrote:
>>> On 10-03-16 16:50, Stefan Roese wrote:
>>>> In a system with a complex USB infrastrcture (many USB hubs), the
>>>> power-on delay of mininimum 1 second for each USB hub results in a
>>>> quite
>>>> big USB scanning time. Many USB devices can deal with much lower
>>>> power-on delays. In my test system, even 10ms seems to be enough
>>>> for the USB device to get detected correctly. This patch now
>>>> reduces the minimum 1 second delay to a minumum of 100ms instead.
>>>> This results in a big USB scan time reduction:
>>>>
>>>> Here the numbers for my current board:
>>>>
>>>> Without this patch:
>>>> starting USB...
>>>> USB0:   USB EHCI 1.00
>>>> scanning bus 0 for devices... 9 USB Device(s) found
>>>>
>>>> time: 10.822 seconds
>>>>
>>>> With this patch:
>>>> starting USB...
>>>> USB0:   USB EHCI 1.00
>>>> scanning bus 0 for devices... 9 USB Device(s) found
>>>>
>>>> time: 6.319 seconds
>>>>
>>>> So ~4.5 seconds of USB scanning time reduction.
>>>>
>>>> I'm not really sure if this patch can be accepted in general as is.
>>>>
>>>> In patch 0d437bca [usb: hub: fix power good delay timing] by Stephen
>>>> Warren, this delay was changes according to "Connect Timing ECN.pdf"
>>>> to a total of 1 second plus the hub port's power good time. Now its
>>>> changes back to 100ms which is a violation of this spec. But still,
>>>> for most USB devices this 100ms is more than enough. So its a valid
>>>> use-case to lower this time in special (perhaps static) USB
>>>> environments.
>>>
>>> Actually that 1 second + poweron delay is why we had the 1 sec
>>> delay in usb_hub_configure(), that is where we wait for a device
>>> to show up. Note that we can already save a lot of time, by
>>> first powering up all ports and then doing the 1 sec wait
>>> and then checking all ports without needing to delay any further.
>>>
>>> Or even better:
>>>
>>> 1) turn on all ports
>>> 2) do power-on-delay (either 2ms * bPwrOn2PwrGood from descriptor, or
>>>     100ms which ever one is LARGER)
>>> 3) set a timeout val 1 sec from now, loop over all ports and quit
>>>     the loop when either all ports are connected (we already skip the
>>>     per port delay in this connected case now), or when the 1 sec
>>>     expires. There is no reason for the 1 sec per port delay,
>>>     one sec + bPwrOn2PwrGood delay is enough for all ports
>>
>> One question here: Do we really need to do this power-on-delay in
>> step 2) at all?
>
> Yes the power may bounce during this time, causing a device connect
> + disconnect + connect again, we really need to wait for the power
> to be stable before looking at / talking to connected devices.
>
> Note this is also what the kernel does, it ignores the hub after
> powering its ports for this time.

Okay. I'll change it to include this initial delay here as well.

>> Shouldn't it be sufficient to do the port scanning
>> loop with a "2ms * bPwrOn2PwrGood + 1 sec" timeout instead? As
>> I've done in the patch I've attached (which works just fine on
>> my platform).
>>
>> Note that I will also dig into the parallelizing idea as well. I'm
>> just trying to implement the timeout handling correctly first.
>
> Cool :)
>
> About the patch, you're waiting up to 1 sec for the first port and
> then scanning the other ports without waiting. For the initial
> implementation this is fine, but for the parallelization of child
> probing you will want to scan all ports as fast as possible, skipping
> already connected and handled ports for the duration of 1 sec, so as to
> also scan the children as soon as they are connected (and not delay
> scanning a hub on port 2 because port 1 does not have a device connected).

Right.

> Nice improvement in scan time from this simple patch btw :)

Yep! I was also pretty impressed by this. :)

Thanks,
Stefan

[U-Boot] [PATCH 6/6] usb: Implement static USB port configuration to speed up USB scanning

[Stefan Roese]

Hi Hans,

On 10.03.2016 20:18, Hans de Goede wrote:
> Hi,
>
> On 10-03-16 16:50, Stefan Roese wrote:
>> This patch implements an optionally quasi static USB port configuration.
>> This is done by using an environment variable, that describes the ports
>> that shall be scanned at the next USB scans (usb start, usb reset).
>>
>> The "usb_port_use" env variable is used to describe this static USB
>> configuration. For this, each USB hub is represented by a 8-bit value.
>> Making it possible to configure a maximum of 8 ports for each USB hub.
>> A 64-bit representation is used, therefore 8 USB hubs can be described
>> in total.
>>
>> Here an example for this "usb_port_use" environment variable:
>>
>> usb_port_use = 0000000000040301
>> -------------------------------
>> 1st USB hub: 0x01 -> Use port 1 (first port)
>> 2nd USB hub: 0x03 -> Use port 1 and 2
>> 3rd USB hub: 0x04 -> Use port 3
>> 4th USB hub: 0x00 -> Use no ports
>> ...
>> 8th USB hub: 0x00 -> Use no ports
>>
>> To make this procedure of configuring this env variable less error prone
>> and less painful, this patch also introduces another env variable that
>> is dynamically generated at each USB scan: "usb_port_detected". This
>> variable is similar to "usb_port_use". It has a bit enabled for each
>> port that has been detected. This can be easily used on a new system,
>> where the USB configuration is static in this way:
>>
>> Run the USB scan (usb start, usb reset) without the "usb_port_use"
>> variable set. This will result in all ports being scanned and the result
>> written into the "usb_port_detected" variable. To configure the USB
>> subsystem to only scan these specific USB ports upon the next
>> scans, you only need to write the value from "usb_port_detected"
>> into the "usb_port_use" variable:
>>
>> => setenv usb_port_use ${usb_port_detected}
>> => saveenv
>>
>> The next scans will only scan those enabled ports.
>>
>> Its of course also possible to manually "tune" this env variable. If
>> some ports are not needed in U-Boot, they can be disabled this way.
>> This will result in less USB hub ports getting scanned and therefore
>> in a faster USB scan time. Here an example:
>>
>> With all USB devices enabled (usb_port_use not set):
>
> This will fall apart when you get multiple root hubs,
> if you want to do this (I believe there is much more low hanging fruit
> see my previous mails), you somehow need to describe the entire
> path to the hub in the env variable, currently if one hub gets removed,
> other hubs which are children of the same parent will get a different
> number in your hub-numbering scheme and things go awry, also what about
> hubs
> on a second host controller, do those number on from the hub-numbering of
> the first hcd ? That seems vary fragile and will break when we add
> (semi) parallel scanning.
>
> So NACK because this disallows later implementing parallel scanning
> without regressing this feature.

Yes, I see your point. But I still like the idea of this static USB
configuration. Best in a similar way as done in this patch (via some
env variables) to have a flexible means to configure the USB system.
I'll need to re-think this a bit more and will probably get back to
this at some point.

For now, I've removed this patch from the patch series (as you will
have noticed by now).

Thanks for all you comments so far.

Thanks,
Stefan