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

[Stephen Warren <swarren@wwwdotorg.org>]

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

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

> a)
> The power-on delay in usb_hub_power_on() is now reduced to a value of
> max(100ms, "hub->desc.bPwrOn2PwrGood * 2"). The code does not wait
> using mdelay in this usb_hub_power_on() will wait before querying
               ^^^^^^^^ change to ", instead"? The existing text looks 
like an editing mistake.

> the device in the scanning loop later. The total timeout for this
> hub, which is 1 second + "hub->desc.bPwrOn2PwrGood * 2" is calculated
> and will be used in the following per-port scanning loop as the timeout
> to detect active USB devices on this hub.
>
> b)
> Don't delay the minimum delay (for power to stabalize) in

stabilize

> usb_hub_power_on(). Instead skip querying these devices in the scannig
> loop until the delay time is reached.
>
> c)
> The ports are now scanned in a quasy parallel way. The current code did

quasi

> wait for each (unconnected) port to reach its timeout. And only then
> continue with the next port. This patch now changes this to scan all
> ports of all USB hubs quasi simultaniously. For this, all ports are added

simultaneously

> to a scanning list. This list is scanned until all ports are ready
> by either a) reaching the connection timeout (calculated earlier), or
> by b) detecting a USB device. Resulting in a faster USB scan time as the
> recursive scanning of USB hubs connected to the hub thats currently

that's

> being scanned will start earlier.

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

> @@ -120,7 +121,21 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>   		pgood_delay = max(pgood_delay,
>   			          (unsigned)simple_strtol(env, NULL, 0));
>   	debug("pgood_delay=%dms\n", pgood_delay);
> -	mdelay(pgood_delay + 1000);
> +
> +	/*
> +	 * Do a minimum delay of the larger value of 100ms or pgood_delay
> +	 * so that the power can stablize before the devices are queried
> +	 */
> +	dev->query_delay = get_timer(0) + max(100, (int)pgood_delay);
> +
> +	/*
> +	 * Record the power-on timeout here. The max. delay (timeout)
> +	 * will be done based on this value in the USB port loop in
> +	 * usb_hub_configure() later.
> +	 */
> +	dev->connect_timeout = get_timer(0) + pgood_delay + 1000;

I'd be tempted to make that:

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

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

> static LIST_HEAD(usb_scan_list);

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

> +static int usb_scan_port(struct usb_device_scan *usb_scan)

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

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

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

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

> +		debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1);
> +		/* Remove this device from scanning list */
> +		list_del(&usb_scan->list);
> +
> +		/* A new USB device is ready at this point */
> +
> +		debug("Port %d Status %X Change %X\n",
> +		      i + 1, portstatus, portchange);

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

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

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

> +static int usb_device_list_scan(void)
...
> +	static int running;
> +	int ret = 0;
> +
> +	/* Only run this loop once for each controller */
> +	if (running)
> +		return 0;
> +
> +	running = 1;
...
> +out:
> +	/*
> +	 * This USB controller has has finished scanning all its connected
> +	 * USB devices. Set "running" back to 0, so that other USB controllers
> +	 * will scan their devices too.
> +	 */
> +	running = 0;

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

>   static int usb_hub_configure(struct usb_device *dev)

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

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