From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Mon, 14 Mar 2016 13:45:24 +0100 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: <56E6B264.3050802@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, 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 :) > > 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(). > + /* 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. > + } > > - 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 :) With the above things fixed you can add my: Acked-by: Hans de Goede To it. 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). Regards, Hans