[U-Boot] [PATCH] usb: Move determination of TT hub address/port into seperate function

Hans de Goede
2015-12-18T10:00:19Z

Hi,

On 18-12-15 02:11, Stefan Br?ns wrote:
> Start split and complete split tokens need the hub address and the
> downstream port of the first HS hub (device view).
>
> The core of the function was duplicated in both host/ehci_hcd and
> musb-new/usb-compat.h.
>
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Thanks for working on this, I think I've spotted one small bug though, see comments inline.

> ---
> Obsoletes [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactions
>
> common/usb.c | 54 +++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/ehci-hcd.c | 50 ++++--------------------------------
> drivers/usb/musb-new/musb_host.c | 8 +++---
> drivers/usb/musb-new/usb-compat.h | 53 --------------------------------------
> include/usb.h | 10 ++++++++
> 5 files changed, 74 insertions(+), 101 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index 700bfc3..1d0a151 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1200,4 +1200,58 @@ bool usb_device_has_child_on_port(struct usb_device *parent, int port)
> #endif
> }
>
> +#ifdef CONFIG_DM_USB
> +void usb_find_hub_address_port(struct usb_device *udev,
> + uint8_t *hub_address, uint8_t *hub_port)
> +{
> + struct udevice *parent;
> + struct usb_device *uparent, *ttdev;
> +
> + /*
> + * 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 .
> + */
> + ttdev = udev;
> + parent = udev->dev;
> + uparent = dev_get_parent_priv(parent);
> +
> + while (uparent->speed != USB_SPEED_HIGH) {
> + struct udevice *dev = parent;
> +
> + if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> + printf("Error: Cannot find high speed parent of usb-1 device\n");
> + *hub_address = *hub_port = 0;
> + return;
> + }
> +
> + ttdev = dev_get_parent_priv(dev);
> + parent = dev->parent;
> + uparent = dev_get_parent_priv(parent);
> + }
> + *hub_address = uparent->devnum;
> + *hub_port = ttdev->portnr;

This is correct to replace the ehci-code, but lets take a closer look at
the musb-new code below.

> +}
> +#else
> +void usb_find_hub_address_port(struct usb_device *udev,
> + uint8_t *hub_address, uint8_t *hub_port)
> +{
> + /* Find out the nearest parent which is high speed */
> + while (udev->parent->parent != NULL)
> + if (udev->parent->speed != USB_SPEED_HIGH) {
> + udev = udev->parent;
> + } else {
> + *hub_address = udev->parent->devnum;
> + *hub_port = udev->portnr;

Same here.

> + return;
> + }
> +
> + printf("Error: Cannot find high speed parent of usb-1 device\n");
> + *hub_address = *hub_port = 0;
> +}
> +#endif
> +
> +
> /* EOF */
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index c85dbce..0a279e7 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -279,56 +279,16 @@ static inline u8 ehci_encode_speed(enum usb_device_speed speed)
> static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
> struct QH *qh)
> {
> - struct usb_device *ttdev;
> - int parent_devnum;
> + uint8_t portnr = 0;
> + uint8_t hubaddr = 0;
>
> if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
> return;
>
> - /*
> - * For full / low speed devices we need to get the devnum and portnr of
> - * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
> - * 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_parent_priv(parent);
> -
> - while (uparent->speed != USB_SPEED_HIGH) {
> - struct udevice *dev = 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_parent_priv(dev);
> - parent = dev->parent;
> - uparent = dev_get_parent_priv(parent);
> - }
> - parent_devnum = uparent->devnum;
> -#else
> - ttdev = udev;
> - while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
> - ttdev = ttdev->parent;
> - if (!ttdev->parent)
> - return;
> - parent_devnum = ttdev->parent->devnum;
> -#endif
> + usb_find_hub_address_port(udev, &hubaddr, &portnr)
>
> - qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
> - QH_ENDPT2_HUBADDR(parent_devnum));
> + qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(portnr) |
> + QH_ENDPT2_HUBADDR(hubaddr));
> }
>
> static int

<note moved the changes to musb_host.c from here to behind other changes to make
my comments easier to read>

> diff --git a/drivers/usb/musb-new/usb-compat.h b/drivers/usb/musb-new/usb-compat.h
> index 1c41e2a..760bd78 100644
> --- a/drivers/usb/musb-new/usb-compat.h
> +++ b/drivers/usb/musb-new/usb-compat.h
> @@ -68,38 +68,6 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
> }
>
> #ifdef CONFIG_DM_USB
> -static inline u16 find_tt(struct usb_device *udev)
> -{
> - struct udevice *parent;
> - struct usb_device *uparent, *ttdev;
> -
> - /*
> - * 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 .
> - */
> - ttdev = udev;
> - parent = udev->dev;
> - uparent = dev_get_parent_priv(parent);
> -
> - while (uparent->speed != USB_SPEED_HIGH) {
> - struct udevice *dev = parent;
> -
> - if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> - printf("musb: Error cannot find high speed parent of usb-1 device\n");
> - return 0;
> - }
> -
> - ttdev = dev_get_parent_priv(dev);
> - parent = dev->parent;
> - uparent = dev_get_parent_priv(parent);
> - }
> -
> - return (uparent->devnum << 8) | (ttdev->portnr - 1);
> -}
> -

Here in the original musb_new code "ttdev->portnr - 1" is used rather then "just "ttdev->portnr".

> static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
> {
> struct udevice *parent = udev->dev->parent;
> @@ -129,27 +97,6 @@ static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
> return NULL;
> }
> #else
> -static inline u16 find_tt(struct usb_device *dev)
> -{
> - u8 chid;
> - u8 hub;
> -
> - /* Find out the nearest parent which is high speed */
> - while (dev->parent->parent != NULL)
> - if (dev->parent->speed != USB_SPEED_HIGH)
> - dev = dev->parent;
> - else
> - break;
> -
> - /* determine the port address at that hub */
> - hub = dev->parent->devnum;
> - for (chid = 0; chid < USB_MAXCHILDREN; chid++)
> - if (dev->parent->children[chid] == dev)
> - break;
> -
> - return (hub << 8) | chid;
> -}
> -

And here for the non DM code the same in a convoluted way (the index is
returned which starts at 0, where as portnr-s start at 1).

> static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
> {
> return dev->parent;
> diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
> index 40b9c66..352fa8c 100644
> --- a/drivers/usb/musb-new/musb_host.c
> +++ b/drivers/usb/musb-new/musb_host.c
> @@ -2092,9 +2092,11 @@ int musb_urb_enqueue(
> }
> #else
> if (tt_needed(musb, urb->dev)) {
> - u16 hub_port = find_tt(urb->dev);
> - qh->h_addr_reg = (u8) (hub_port >> 8);
> - qh->h_port_reg = (u8) (hub_port & 0xff);
> + uint8_t portnr = 0;
> + uint8_t hubaddr = 0;
> + usb_find_hub_address_port(udev, &hubaddr, &portnr)
> + qh->h_addr_reg = hubaddr;
> + qh->h_port_reg = portnr;
> }
> #endif
> }

So here "qh->h_port_reg = portnr" should be "qh->h_port_reg = portnr - 1"
to compensate for the portnr returned by the new usb_find_hub_address_port
being one higher then the one returned by the old find_tt helper.

> diff --git a/include/usb.h b/include/usb.h
> index 55b9268..6e12876 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -874,6 +874,16 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
>
> int hub_port_reset(struct udevice *dev, int port, unsigned short *portstat);
>
> +/*
> + * Searches for the first HS hub above the given device. If a
> + * HS hub is found, the hub address and the port the device is
> + * connected to is return, as required for SPLIT transactions
> + *
> + * @param: udev full speed or low speed device
> + */
> +void usb_find_hub_address_port(struct usb_device *udev,
> + uint8_t *hub_address, uint8_t *hub_port);
> +
> /**
> * usb_alloc_new_device() - Allocate a new device
> *
>

This function is usb-2 controller / hub specific, maybe rename it to:
"usb_find_usb2_hub_address_port" to reflect this ?

Regards,

Hans