From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 14 Mar 2016 11:31:01 -0600 Subject: [U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling In-Reply-To: <1457950693-564-5-git-send-email-sr@denx.de> References: <1457950693-564-1-git-send-email-sr@denx.de> <1457950693-564-5-git-send-email-sr@denx.de> Message-ID: <56E6F555.5020807@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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?