diff mbox

[U-Boot,v2,4/9] dm: usb: Fix finding of first upstream usb-2 hub in the ehci dm code

Message ID 1430474699-9019-5-git-send-email-hdegoede@redhat.com
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

Hans de Goede May 1, 2015, 10:04 a.m. UTC
The ehci driver model code for finding the first upstream usb-2 hub before
this commit has a number of issues:

1) "if (!ttdev->speed != USB_SPEED_HIGH)" does not work because the '!'
   takes presedence over the '!=' this should simply be
   "if (ttdev->speed == USB_SPEED_HIGH)"
2) It makes ttdev point to the first upstream usb-2 hub, but ttdev should
   point to the last usb-1 device before the first usb-2 hub (when going
   upstream from the device), as ttdev is used to find the port of the
   first usb-2 hub to which the the last usb-1 device is connected.
3) parent_devnum however should be set to the devnum of the first usb-2
   hub, so we need to keep pointers around to both usb_device structs.

To complicate things further during enumeration usb_device.dev will point
to the parent udevice, where as during normal use it will point to
the actual udevice, we must handle both cases correctly.

This commit fixes all this making usb-1 devices attached to usb-2 hubs,
including usb-1 devices attached to usb-1 hubs attached to usb-2 hubs, work.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/ehci-hcd.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Simon Glass May 3, 2015, 4:59 p.m. UTC | #1
On 1 May 2015 at 04:04, Hans de Goede <hdegoede@redhat.com> wrote:
> The ehci driver model code for finding the first upstream usb-2 hub before
> this commit has a number of issues:
>
> 1) "if (!ttdev->speed != USB_SPEED_HIGH)" does not work because the '!'
>    takes presedence over the '!=' this should simply be
>    "if (ttdev->speed == USB_SPEED_HIGH)"
> 2) It makes ttdev point to the first upstream usb-2 hub, but ttdev should
>    point to the last usb-1 device before the first usb-2 hub (when going
>    upstream from the device), as ttdev is used to find the port of the
>    first usb-2 hub to which the the last usb-1 device is connected.
> 3) parent_devnum however should be set to the devnum of the first usb-2
>    hub, so we need to keep pointers around to both usb_device structs.
>
> To complicate things further during enumeration usb_device.dev will point
> to the parent udevice, where as during normal use it will point to
> the actual udevice, we must handle both cases correctly.
>
> This commit fixes all this making usb-1 devices attached to usb-2 hubs,
> including usb-1 devices attached to usb-1 hubs attached to usb-2 hubs, work.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I'm pleased that you understand this logic.

Acked-by: Simon Glass <sjg@chromium.org>

> ---
>  drivers/usb/host/ehci-hcd.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 85adbf4..9471bcb 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -303,23 +303,33 @@ static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>          * in the tree before that one!
>          */
>  #ifdef CONFIG_DM_USB
> +       /*
> +        * When called from usb-uclass.c: usb_scan_device() udev->dev points
> +        * to the parent udevice, not the actual udevice belonging to the
> +        * udev as the device is not instantiated yet. So when searching
> +        * for the first usb-2 parent start with udev->dev not
> +        * udev->dev->parent .
> +        */
>         struct udevice *parent;
> +       struct usb_device *uparent;
> +
> +       ttdev = udev;
> +       parent = udev->dev;
> +       uparent = dev_get_parentdata(parent);
>
> -       for (ttdev = udev; ; ) {
> -               struct udevice *dev = ttdev->dev;
> +       while (uparent->speed != USB_SPEED_HIGH) {
> +               struct udevice *dev = parent;
>
> -               if (dev->parent &&
> -                   device_get_uclass_id(dev->parent) == UCLASS_USB_HUB)
> -                       parent = dev->parent;
> -               else
> -                       parent = NULL;
> -               if (!parent)
> +               if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> +                       printf("ehci: Error cannot find high speed parent of usb-1 device\n");
>                         return;
> -               ttdev = dev_get_parentdata(parent);
> -               if (!ttdev->speed != USB_SPEED_HIGH)
> -                       break;
> +               }
> +
> +               ttdev = dev_get_parentdata(dev);
> +               parent = dev->parent;
> +               uparent = dev_get_parentdata(parent);
>         }
> -       parent_devnum = ttdev->devnum;
> +       parent_devnum = uparent->devnum;
>  #else
>         ttdev = udev;
>         while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
> --
> 2.3.6
>
diff mbox

Patch

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 85adbf4..9471bcb 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -303,23 +303,33 @@  static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
 	 * in the tree before that one!
 	 */
 #ifdef CONFIG_DM_USB
+	/*
+	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
+	 * to the parent udevice, not the actual udevice belonging to the
+	 * udev as the device is not instantiated yet. So when searching
+	 * for the first usb-2 parent start with udev->dev not
+	 * udev->dev->parent .
+	 */
 	struct udevice *parent;
+	struct usb_device *uparent;
+
+	ttdev = udev;
+	parent = udev->dev;
+	uparent = dev_get_parentdata(parent);
 
-	for (ttdev = udev; ; ) {
-		struct udevice *dev = ttdev->dev;
+	while (uparent->speed != USB_SPEED_HIGH) {
+		struct udevice *dev = parent;
 
-		if (dev->parent &&
-		    device_get_uclass_id(dev->parent) == UCLASS_USB_HUB)
-			parent = dev->parent;
-		else
-			parent = NULL;
-		if (!parent)
+		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
+			printf("ehci: Error cannot find high speed parent of usb-1 device\n");
 			return;
-		ttdev = dev_get_parentdata(parent);
-		if (!ttdev->speed != USB_SPEED_HIGH)
-			break;
+		}
+
+		ttdev = dev_get_parentdata(dev);
+		parent = dev->parent;
+		uparent = dev_get_parentdata(parent);
 	}
-	parent_devnum = ttdev->devnum;
+	parent_devnum = uparent->devnum;
 #else
 	ttdev = udev;
 	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)