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
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
