From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Mon, 14 Mar 2016 14:19:23 +0100 Subject: [U-Boot] [PATCH v3 4/4] usb: Change power-on / scanning timeout handling In-Reply-To: <56E6B264.3050802@redhat.com> References: <1457950693-564-1-git-send-email-sr@denx.de> <1457950693-564-5-git-send-email-sr@denx.de> <56E6B264.3050802@redhat.com> Message-ID: <56E6BA5B.6080702@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Hans, On 14.03.2016 13:45, Hans de Goede wrote: > On 14-03-16 11:18, Stefan Roese wrote: >> This patch changes the USB port scanning procedure and timeout >> handling in the following ways: >> >> a) >> The power-on delay in usb_hub_power_on() is now reduced to a value of >> max(100ms, "hub->desc.bPwrOn2PwrGood * 2"). The code does not wait >> using mdelay in this usb_hub_power_on() will wait before querying >> the device in the scanning loop later. The total timeout for this >> hub, which is 1 second + "hub->desc.bPwrOn2PwrGood * 2" is calculated >> and will be used in the following per-port scanning loop as the timeout >> to detect active USB devices on this hub. >> >> b) >> Don't delay the minimum delay (for power to stabalize) in >> usb_hub_power_on(). Instead skip querying these devices in the scannig >> loop until the delay time is reached. >> >> c) >> The ports are now scanned in a quasy parallel way. The current code did >> wait for each (unconnected) port to reach its timeout. And only then >> continue with the next port. This patch now changes this to scan all >> ports of all USB hubs quasi simultaniously. For this, all ports are added >> to a scanning list. This list is scanned until all ports are ready >> by either a) reaching the connection timeout (calculated earlier), or >> by b) detecting a USB device. Resulting in a faster USB scan time as the >> recursive scanning of USB hubs connected to the hub thats currently >> being scanned will start earlier. >> >> Without this patch: >> starting USB... >> USB0: USB EHCI 1.00 >> scanning bus 0 for devices... 9 USB Device(s) found >> >> time: 20.163 seconds >> >> With this patch: >> starting USB... >> USB0: USB EHCI 1.00 >> scanning bus 0 for devices... 9 USB Device(s) found >> >> time: 1.822 seconds >> >> So ~18.3 seconds of USB scanning time reduction. >> >> Signed-off-by: Stefan Roese >> >> --- >> >> Changes in v3: >> - Introduced scanning list containing all USB devices of one USB >> controller that need to get scanned >> - Don't delay in usb_hub_power_on(). Instead skip querying these devices >> until the delay time is reached. > > Oh I like :) Thanks! :) > >> >> Changes in v2: >> - Remove static USB port configuration patch (for now) >> >> common/usb_hub.c | 289 >> +++++++++++++++++++++++++++++++++++++------------------ >> include/usb.h | 3 + >> 2 files changed, 200 insertions(+), 92 deletions(-) >> >> diff --git a/common/usb_hub.c b/common/usb_hub.c >> index d621f50..1167217 100644 >> --- a/common/usb_hub.c >> +++ b/common/usb_hub.c >> @@ -30,6 +30,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #ifdef CONFIG_SANDBOX >> #include >> @@ -120,7 +121,21 @@ static void usb_hub_power_on(struct >> usb_hub_device *hub) >> pgood_delay = max(pgood_delay, >> (unsigned)simple_strtol(env, NULL, 0)); >> debug("pgood_delay=%dms\n", pgood_delay); >> - mdelay(pgood_delay + 1000); >> + >> + /* >> + * Do a minimum delay of the larger value of 100ms or pgood_delay >> + * so that the power can stablize before the devices are queried >> + */ >> + dev->query_delay = get_timer(0) + max(100, (int)pgood_delay); >> + >> + /* >> + * Record the power-on timeout here. The max. delay (timeout) >> + * will be done based on this value in the USB port loop in >> + * usb_hub_configure() later. >> + */ >> + dev->connect_timeout = get_timer(0) + pgood_delay + 1000; >> + debug("devnum=%d poweron: query_delay=%d connect_timeout=%d\n", >> + dev->devnum, max(100, (int)pgood_delay), pgood_delay + 1000); >> } >> >> void usb_hub_reset(void) >> @@ -332,6 +347,161 @@ int usb_hub_port_connect_change(struct >> usb_device *dev, int port) >> return ret; >> } >> >> +static LIST_HEAD(usb_scan_list); >> + >> +struct usb_device_scan { >> + struct usb_device *dev; /* USB hub device to scan */ >> + struct usb_hub_device *hub; /* USB hub struct */ >> + int port; /* USB port to scan */ >> + struct list_head list; >> +}; >> + >> +static int usb_scan_port(struct usb_device_scan *usb_scan) >> +{ >> + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); >> + unsigned short portstatus; >> + unsigned short portchange; >> + struct usb_device *dev; >> + struct usb_hub_device *hub; >> + int ret = 0; >> + int i; >> + >> + dev = usb_scan->dev; >> + hub = usb_scan->hub; >> + i = usb_scan->port; >> + >> +#ifdef CONFIG_DM_USB >> + debug("\n\nScanning '%s' port %d\n", dev->dev->name, i + 1); >> +#else >> + debug("\n\nScanning port %d\n", i + 1); >> +#endif >> + >> + ret = usb_get_port_status(dev, i + 1, portsts); >> + if (ret < 0) { >> + debug("get_port_status failed\n"); >> + return 0; >> + } >> + >> + portstatus = le16_to_cpu(portsts->wPortStatus); >> + portchange = le16_to_cpu(portsts->wPortChange); >> + >> + /* No connection change happened, wait a bit more. */ >> + if (!(portchange & USB_PORT_STAT_C_CONNECTION)) { >> + if (get_timer(0) >= dev->connect_timeout) { >> + debug("devnum=%d port=%d: timeout\n", >> + dev->devnum, i + 1); >> + /* Remove this device from scanning list */ >> + list_del(&usb_scan->list); >> + return 0; >> + } >> + return 0; >> + } >> + >> + /* Test if the connection came up, and if so, exit. */ >> + if (portstatus & USB_PORT_STAT_CONNECTION) { >> + debug("devnum=%d port=%d: USB dev found\n", dev->devnum, i + 1); >> + /* Remove this device from scanning list */ >> + list_del(&usb_scan->list); >> + >> + /* A new USB device is ready at this point */ >> + >> + debug("Port %d Status %X Change %X\n", >> + i + 1, portstatus, portchange); >> + >> + if (portchange & USB_PORT_STAT_C_CONNECTION) { >> + debug("port %d connection change\n", i + 1); >> + usb_hub_port_connect_change(dev, i); >> + } >> + if (portchange & USB_PORT_STAT_C_ENABLE) { >> + debug("port %d enable change, status %x\n", >> + i + 1, portstatus); >> + usb_clear_port_feature(dev, i + 1, >> + USB_PORT_FEAT_C_ENABLE); >> + /* >> + * The following hack causes a ghost device problem >> + * to Faraday EHCI >> + */ >> +#ifndef CONFIG_USB_EHCI_FARADAY >> + /* EM interference sometimes causes bad shielded USB >> + * devices to be shutdown by the hub, this hack enables >> + * them again. Works at least with mouse driver */ >> + if (!(portstatus & USB_PORT_STAT_ENABLE) && >> + (portstatus & USB_PORT_STAT_CONNECTION) && >> + usb_device_has_child_on_port(dev, i)) { >> + debug("already running port %i disabled by hub >> (EMI?), re-enabling...\n", >> + i + 1); >> + usb_hub_port_connect_change(dev, i); >> + } >> +#endif >> + } >> + if (portstatus & USB_PORT_STAT_SUSPEND) { >> + debug("port %d suspend change\n", i + 1); >> + usb_clear_port_feature(dev, i + 1, >> + USB_PORT_FEAT_SUSPEND); >> + } >> + >> + if (portchange & USB_PORT_STAT_C_OVERCURRENT) { >> + debug("port %d over-current change\n", i + 1); >> + usb_clear_port_feature(dev, i + 1, >> + USB_PORT_FEAT_C_OVER_CURRENT); >> + usb_hub_power_on(hub); >> + } >> + >> + if (portchange & USB_PORT_STAT_C_RESET) { >> + debug("port %d reset change\n", i + 1); >> + usb_clear_port_feature(dev, i + 1, >> + USB_PORT_FEAT_C_RESET); >> + } >> + } >> + >> + return 0; >> +} >> + >> +static int usb_device_list_scan(void) >> +{ >> + struct usb_device_scan *usb_scan; >> + struct usb_device_scan *tmp; >> + static int running; >> + int ret = 0; >> + >> + /* Only run this loop once for each controller */ >> + if (running) >> + return 0; >> + >> + running = 1; >> + >> + while (1) { >> + /* We're done, once the list is empty again */ >> + if (list_empty(&usb_scan_list)) >> + goto out; >> + >> + list_for_each_entry_safe(usb_scan, tmp, &usb_scan_list, list) { >> + int ret; >> + >> + /* >> + * Don't talk to the device before the query delay is >> + * expired. This is needed for voltages to stabalize. >> + */ >> + if (get_timer(0) < usb_scan->dev->query_delay) >> + continue; >> + > > I would prefer for this to be moved to usb_scan_port(). Yes, makes sense. I've moved it there for v4. >> + /* Scan this port */ >> + ret = usb_scan_port(usb_scan); >> + if (ret) >> + goto out; >> + } >> + } >> + >> +out: >> + /* >> + * This USB controller has has finished scanning all its connected >> + * USB devices. Set "running" back to 0, so that other USB >> controllers >> + * will scan their devices too. >> + */ >> + running = 0; >> + >> + return ret; >> +} >> >> static int usb_hub_configure(struct usb_device *dev) >> { >> @@ -466,104 +636,39 @@ static int usb_hub_configure(struct usb_device >> *dev) >> for (i = 0; i < dev->maxchild; i++) >> usb_hub_reset_devices(i + 1); >> >> - for (i = 0; i < dev->maxchild; i++) { >> - ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); >> - unsigned short portstatus, portchange; >> - int ret; >> - ulong start = get_timer(0); >> - uint delay = CONFIG_SYS_HZ; >> - >> #ifdef CONFIG_SANDBOX >> - if (state_get_skip_delays()) >> - delay = 0; >> -#endif >> -#ifdef CONFIG_DM_USB >> - debug("\n\nScanning '%s' port %d\n", dev->dev->name, i + 1); >> -#else >> - debug("\n\nScanning port %d\n", i + 1); >> + if (state_get_skip_delays()) >> + dev->poweron_timeout = 0; >> #endif >> - /* >> - * Wait for (whichever finishes first) >> - * - A maximum of 10 seconds >> - * This is a purely observational value driven by connecting >> - * a few broken pen drives and taking the max * 1.5 approach >> - * - connection_change and connection state to report same >> - * state >> - */ >> - do { >> - ret = usb_get_port_status(dev, i + 1, portsts); >> - if (ret < 0) { >> - debug("get_port_status failed\n"); >> - break; >> - } >> >> - portstatus = le16_to_cpu(portsts->wPortStatus); >> - portchange = le16_to_cpu(portsts->wPortChange); >> - >> - /* No connection change happened, wait a bit more. */ >> - if (!(portchange & USB_PORT_STAT_C_CONNECTION)) >> - continue; >> - >> - /* Test if the connection came up, and if so, exit. */ >> - if (portstatus & USB_PORT_STAT_CONNECTION) >> - break; >> - >> - } while (get_timer(start) < delay); >> - >> - if (ret < 0) >> - continue; >> - >> - debug("Port %d Status %X Change %X\n", >> - i + 1, portstatus, portchange); >> - >> - if (portchange & USB_PORT_STAT_C_CONNECTION) { >> - debug("port %d connection change\n", i + 1); >> - usb_hub_port_connect_change(dev, i); >> - } >> - if (portchange & USB_PORT_STAT_C_ENABLE) { >> - debug("port %d enable change, status %x\n", >> - i + 1, portstatus); >> - usb_clear_port_feature(dev, i + 1, >> - USB_PORT_FEAT_C_ENABLE); >> - /* >> - * The following hack causes a ghost device problem >> - * to Faraday EHCI >> - */ >> -#ifndef CONFIG_USB_EHCI_FARADAY >> - /* EM interference sometimes causes bad shielded USB >> - * devices to be shutdown by the hub, this hack enables >> - * them again. Works at least with mouse driver */ >> - if (!(portstatus & USB_PORT_STAT_ENABLE) && >> - (portstatus & USB_PORT_STAT_CONNECTION) && >> - usb_device_has_child_on_port(dev, i)) { >> - debug("already running port %i " \ >> - "disabled by hub (EMI?), " \ >> - "re-enabling...\n", i + 1); >> - usb_hub_port_connect_change(dev, i); >> - } >> -#endif >> - } >> - if (portstatus & USB_PORT_STAT_SUSPEND) { >> - debug("port %d suspend change\n", i + 1); >> - usb_clear_port_feature(dev, i + 1, >> - USB_PORT_FEAT_SUSPEND); >> - } >> + /* >> + * Only add the connected USB devices, including potential hubs, >> + * to a scanning list. This list will get scanned and devices that >> + * are detected (either via port connected or via port timeout) >> + * will get removed from this list. Scanning of the devices on this >> + * list will continue until all devices are removed. >> + */ >> + for (i = 0; i < dev->maxchild; i++) { >> + struct usb_device_scan *usb_scan; >> >> - if (portchange & USB_PORT_STAT_C_OVERCURRENT) { >> - debug("port %d over-current change\n", i + 1); >> - usb_clear_port_feature(dev, i + 1, >> - USB_PORT_FEAT_C_OVER_CURRENT); >> - usb_hub_power_on(hub); >> + usb_scan = calloc(1, sizeof(*usb_scan)); >> + if (!usb_scan) { >> + printf("Can't allocate memory for USB device!\n"); >> + return -ENOMEM; >> } >> + usb_scan->dev = dev; >> + usb_scan->hub = hub; >> + usb_scan->port = i; >> + INIT_LIST_HEAD(&usb_scan->list); > > This (INIT_LIST_HEAD) is not necessary, as this is not the HEAD of a > list, but just a list member, anything set here will immediately > be overridden by: > >> + list_add_tail(&usb_scan->list, &usb_scan_list); > > This list_add_tail call. Thanks. Fixed including the addition of some free() calls for v4. I'm waiting for a few more review comments before sending this out. >> + } >> >> - if (portchange & USB_PORT_STAT_C_RESET) { >> - debug("port %d reset change\n", i + 1); >> - usb_clear_port_feature(dev, i + 1, >> - USB_PORT_FEAT_C_RESET); >> - } >> - } /* end for i all ports */ >> + /* >> + * And now call the scanning code which loops over the generated >> list >> + */ >> + ret = usb_device_list_scan(); >> >> - return 0; >> + return ret; >> } >> >> static int usb_hub_check(struct usb_device *dev, int ifnum) >> diff --git a/include/usb.h b/include/usb.h >> index 0b410b6..0b57093 100644 >> --- a/include/usb.h >> +++ b/include/usb.h >> @@ -153,6 +153,9 @@ struct usb_device { >> struct udevice *dev; /* Pointer to associated device */ >> struct udevice *controller_dev; /* Pointer to associated >> controller */ >> #endif >> + >> + ulong connect_timeout; /* Device connection timeout in ms */ >> + ulong query_delay; /* Device query delay in ms */ >> }; >> >> struct int_queue; >> > > Otherwise I like this patch, in fact I like it a lot :) Thanks. I'm also pretty satisfied with this version. The result is astonishing fast - at least compared to the "old" implementation. Thanks for all your very valuable suggestions and comments. > With the above things fixed you can add my: > > Acked-by: Hans de Goede > > To it. Will do. > I guess this scratches your itch, so you're probably done with this, if > not having parallel controller scanning too would be awesome for > some boards I've which have up to 8 controllers (of which only 6 > are supporte d by u-boot atm, but that is a different issue). My current platform only has 1 USB controller. So I'm probably done with this optimization for now. Also, I think its good to get this version integrated into mainline first, before changing further things, like the parallel controller scanning. This can always be done in some follow-up patches (my me or someone else). Thanks, Stefan