Hi,
On 14-03-16 18:31, Stephen Warren wrote:
On 03/14/2016 04:18 AM, Stefan Roese wrote:
<snip>
@@ -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.
Ack, good catch.
Currently, if pgood_delay<=100 (is that legal?) then the delta might be as little as 900ms (for pgood_delay==0).
AFAIK it is not legal, but guess what the firmware authors of cheap hubs put in the descriptor when 0 seems to work just fine ?
Rule of thumb: Never trust usb descriptors.
Regards,
Hans
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? _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot