[U-Boot] [PATCH 0/5] dm: usb: Add companion controller support + ehci fixes

[U-Boot] [PATCH 0/5] dm: usb: Add companion controller support + ehci fixes

[Hans de Goede]

Hi Simon, Marek,

I'm making steady progress with adding ohci / companion controller support
to the sunxi usb code. Things are progressing a bit slower then I would
have liked as I've been hitting bugs in the usb-core, ehci and ohci code,
but I've things mostly working now I just need to do some more polishing.

Patches 1-3 add a (primitive) notion of companion controllers to the dm-usb
code.

Patch 4 contains a bug-fix for the ehci companion related code

Patch 5 adds integration with the dm-usb companion code to the ehci driver

Since patch 5 depends on patches 3 and 4 I believe it would be best for
this entire series to go upstream through the dm tree (Marek, we will
need your ack for patch 4 and 5 for this).

This series sits on top of my previous dm-usb fixes + sunxi ehci dm support
series. I hope to be able to post yet another series sitting on top of this
one tonight which will add the actual ohci support, that one will make a
lot of ohci driver changes as well as some usb core changes and thus should
go upstream through Marek's tree once this series has landed.

Regards,

Hans

[U-Boot] [PATCH 1/5] dm: usb: Move printing of usb scan status to usb_scan_bus()

[Hans de Goede]

Move printing of usb scan status to usb_scan_bus().

This is a preparation patch for adding companion controller support to the
usb uclass.

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

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 9ee25ed..ad778b4 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -147,7 +147,7 @@ int usb_stop(void)
 	return err;
 }
 
-static int usb_scan_bus(struct udevice *bus, bool recurse)
+static void usb_scan_bus(struct udevice *bus, bool recurse)
 {
 	struct usb_bus_priv *priv;
 	struct udevice *dev;
@@ -157,11 +157,15 @@ static int usb_scan_bus(struct udevice *bus, bool recurse)
 
 	assert(recurse);	/* TODO: Support non-recusive */
 
+	printf("scanning bus %d for devices... ", bus->seq);
+	debug("\n");
 	ret = usb_scan_device(bus, 0, USB_SPEED_FULL, &dev);
 	if (ret)
-		return ret;
-
-	return priv->next_addr;
+		printf("failed, error %d\n", ret);
+	else if (priv->next_addr == 0)
+		printf("No USB Device found\n");
+	else
+		printf("%d USB Device(s) found\n", priv->next_addr);
 }
 
 int usb_init(void)
@@ -199,15 +203,7 @@ int usb_init(void)
 		 * i.e. search HUBs and configure them
 		 */
 		controllers_initialized++;
-		printf("scanning bus %d for devices... ", bus->seq);
-		debug("\n");
-		ret = usb_scan_bus(bus, true);
-		if (ret < 0)
-			printf("failed, error %d\n", ret);
-		else if (!ret)
-			printf("No USB Device found\n");
-		else
-			printf("%d USB Device(s) found\n", ret);
+		usb_scan_bus(bus, true);
 		usb_started = true;
 	}
 
-- 
2.3.6

[U-Boot] [PATCH 2/5] dm: usb: Add support for companion controllers

[Hans de Goede]

USB companion controllers must be scanned after the main controller has
been scanned, so that any devices which the main controller which to hand
over to the companion have actually been handed over before we scan the
companion.

As there are no guarantees that this will magically happen in the right
order, split the scanning of the busses in 2 phases, first main controllers,
and then companion controllers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/host/usb-uclass.c | 30 +++++++++++++++++++++++++-----
 include/usb.h                 |  2 ++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index ad778b4..d745c1c 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -157,6 +157,9 @@ static void usb_scan_bus(struct udevice *bus, bool recurse)
 
 	assert(recurse);	/* TODO: Support non-recusive */
 
+	if (!device_active(bus))
+		return;
+
 	printf("scanning bus %d for devices... ", bus->seq);
 	debug("\n");
 	ret = usb_scan_device(bus, 0, USB_SPEED_FULL, &dev);
@@ -171,6 +174,7 @@ static void usb_scan_bus(struct udevice *bus, bool recurse)
 int usb_init(void)
 {
 	int controllers_initialized = 0;
+	struct usb_bus_priv *priv;
 	struct udevice *bus;
 	struct uclass *uc;
 	int count = 0;
@@ -198,15 +202,31 @@ int usb_init(void)
 			printf("probe failed, error %d\n", ret);
 			continue;
 		}
-		/*
-		 * lowlevel init is OK, now scan the bus for devices
-		 * i.e. search HUBs and configure them
-		 */
 		controllers_initialized++;
-		usb_scan_bus(bus, true);
 		usb_started = true;
 	}
 
+	/*
+	 * lowlevel init done, now scan the bus for devices i.e. search HUBs
+	 * and configure them, first scan primary controllers.
+	 */
+	uclass_foreach_dev(bus, uc) {
+		priv = dev_get_uclass_priv(bus);
+		if (!priv->companion)
+			usb_scan_bus(bus, true);
+	}
+
+	/*
+	 * Now that the primary controllers have been scanned and have handed
+	 * over any devices they do not understand to their companions, scan
+	 * the companions.
+	 */
+	uclass_foreach_dev(bus, uc) {
+		priv = dev_get_uclass_priv(bus);
+		if (priv->companion)
+			usb_scan_bus(bus, true);
+	}
+
 	debug("scan end\n");
 	/* if we were not able to find at least one working bus, bail out */
 	if (!count)
diff --git a/include/usb.h b/include/usb.h
index 7b55844..b81e796 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -608,10 +608,12 @@ struct usb_dev_platdata {
  * @desc_before_addr:	true if we can read a device descriptor before it
  *		has been assigned an address. For XHCI this is not possible
  *		so this will be false.
+ * @companion:  True if this is a companion controler to another USB controller
  */
 struct usb_bus_priv {
 	int next_addr;
 	bool desc_before_addr;
+	bool companion;
 };
 
 /**
-- 
2.3.6

[U-Boot] [PATCH 3/5] dm: usb: Do not scan companion busses if no devices where handed over

[Hans de Goede]

USB scanning is slow, and there is no need to scan the companion busses
if no usb devices where handed over to the companinon controllers by any
of the main controllers.

This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1
devices plugged into the root usb ports.

The use of a global variable for the companion handover counting is somewhat
unfortunate, but we do not have the necessary info to link companions and
main controllers together in devicetree, and since this is just an
optimization adding a custom devicetree extenstion for this seems undesirable.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/usb.c                  |  2 ++
 drivers/usb/host/usb-uclass.c | 12 +++++++-----
 include/usb.h                 |  3 +++
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 1b26bfa..4a09583 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -44,6 +44,8 @@
 
 static int asynch_allowed;
 char usb_started; /* flag for the started/stopped USB status */
+/* Tracks how much devices were handed over to companion controllers */
+int usb_companion_device_count;
 
 #ifndef CONFIG_DM_USB
 static struct usb_device usb_dev[USB_MAX_DEVICE];
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index d745c1c..877d307 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -219,12 +219,14 @@ int usb_init(void)
 	/*
 	 * Now that the primary controllers have been scanned and have handed
 	 * over any devices they do not understand to their companions, scan
-	 * the companions.
+	 * the companions if necessary.
 	 */
-	uclass_foreach_dev(bus, uc) {
-		priv = dev_get_uclass_priv(bus);
-		if (priv->companion)
-			usb_scan_bus(bus, true);
+	if (usb_companion_device_count) {
+		uclass_foreach_dev(bus, uc) {
+			priv = dev_get_uclass_priv(bus);
+			if (priv->companion)
+				usb_scan_bus(bus, true);
+		}
 	}
 
 	debug("scan end\n");
diff --git a/include/usb.h b/include/usb.h
index b81e796..d4c9f44 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -171,6 +171,9 @@ enum usb_init_type {
  * this is how the lowlevel part communicate with the outer world
  */
 
+/* Tracks how much devices were handed over to companion controllers */
+extern int usb_companion_device_count;
+
 #if defined(CONFIG_USB_UHCI) || defined(CONFIG_USB_OHCI) || \
 	defined(CONFIG_USB_EHCI) || defined(CONFIG_USB_OHCI_NEW) || \
 	defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \
-- 
2.3.6

[U-Boot] [PATCH 4/5] usb: ehci: Fix handover of full-speed devices to companion

[Hans de Goede]

When after a reset the port status connection bit is still set and the enable
bit is not then we're dealing with a full-speed device and should hand it over
to the companion controller.

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

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 0e84265..25981ef 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -897,11 +897,21 @@ static int ehci_submit_root(struct usb_device *dev, unsigned long pipe,
 				 */
 				ret = handshake(status_reg, EHCI_PS_PR, 0,
 						2 * 1000);
-				if (!ret)
-					ctrl->portreset |= 1 << port;
-				else
+				if (!ret) {
+					reg = ehci_readl(status_reg);
+					if ((reg & (EHCI_PS_PE | EHCI_PS_CS))
+					    == EHCI_PS_CS && !ehci_is_TDI()) {
+						debug("port %d full speed --> companion\n", port - 1);
+						reg &= ~EHCI_PS_CLEAR;
+						reg |= EHCI_PS_PO;
+						ehci_writel(status_reg, reg);
+					} else {
+						ctrl->portreset |= 1 << port;
+					}
+				} else {
 					printf("port(%d) reset error\n",
 					       port - 1);
+				}
 			}
 			break;
 		case USB_PORT_FEAT_TEST:
-- 
2.3.6

[U-Boot] [PATCH 5/5] usb: ehci: Increase usb_companion_device_count when handing over devices

[Hans de Goede]

Increase usb_companion_device_count when handing over devices to the ehci's
companion controller. usb_init() will use this to short-circuit companion
bus scanning when no devices were handed over at all.

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

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 25981ef..40d6dc1 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -873,6 +873,7 @@ static int ehci_submit_root(struct usb_device *dev, unsigned long pipe,
 				/* Low speed device, give up ownership. */
 				debug("port %d low speed --> companion\n",
 				      port - 1);
+				usb_companion_device_count++;
 				reg |= EHCI_PS_PO;
 				ehci_writel(status_reg, reg);
 				break;
@@ -902,6 +903,7 @@ static int ehci_submit_root(struct usb_device *dev, unsigned long pipe,
 					if ((reg & (EHCI_PS_PE | EHCI_PS_CS))
 					    == EHCI_PS_CS && !ehci_is_TDI()) {
 						debug("port %d full speed --> companion\n", port - 1);
+						usb_companion_device_count++;
 						reg &= ~EHCI_PS_CLEAR;
 						reg |= EHCI_PS_PO;
 						ehci_writel(status_reg, reg);
-- 
2.3.6

[U-Boot] [PATCH 1/5] dm: usb: Move printing of usb scan status to usb_scan_bus()

[Simon Glass]

Hi Hans,

On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote:
> Move printing of usb scan status to usb_scan_bus().
>
> This is a preparation patch for adding companion controller support to the
> usb uclass.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/usb-uclass.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)

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

>
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index 9ee25ed..ad778b4 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -147,7 +147,7 @@ int usb_stop(void)
>         return err;
>  }
>
> -static int usb_scan_bus(struct udevice *bus, bool recurse)
> +static void usb_scan_bus(struct udevice *bus, bool recurse)
>  {
>         struct usb_bus_priv *priv;
>         struct udevice *dev;
> @@ -157,11 +157,15 @@ static int usb_scan_bus(struct udevice *bus, bool recurse)
>
>         assert(recurse);        /* TODO: Support non-recusive */
>
> +       printf("scanning bus %d for devices... ", bus->seq);
> +       debug("\n");
>         ret = usb_scan_device(bus, 0, USB_SPEED_FULL, &dev);
>         if (ret)
> -               return ret;
> -
> -       return priv->next_addr;
> +               printf("failed, error %d\n", ret);
> +       else if (priv->next_addr == 0)
> +               printf("No USB Device found\n");
> +       else
> +               printf("%d USB Device(s) found\n", priv->next_addr);

I'd like to keep printf() out of the uclasses and this buries it two
levels deep. Still I don't think it is too hard to unpick. I think
perhaps one day we should move usb_init() back to usb.c and put all
the logic there.

>  }
>
>  int usb_init(void)
> @@ -199,15 +203,7 @@ int usb_init(void)
>                  * i.e. search HUBs and configure them
>                  */
>                 controllers_initialized++;
> -               printf("scanning bus %d for devices... ", bus->seq);
> -               debug("\n");
> -               ret = usb_scan_bus(bus, true);
> -               if (ret < 0)
> -                       printf("failed, error %d\n", ret);
> -               else if (!ret)
> -                       printf("No USB Device found\n");
> -               else
> -                       printf("%d USB Device(s) found\n", ret);
> +               usb_scan_bus(bus, true);
>                 usb_started = true;
>         }
>
> --
> 2.3.6
>
Regards,
Simon

[U-Boot] [PATCH 2/5] dm: usb: Add support for companion controllers

[Simon Glass]

Hi Hans,

On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote:
> USB companion controllers must be scanned after the main controller has
> been scanned, so that any devices which the main controller which to hand
> over to the companion have actually been handed over before we scan the
> companion.
>
> As there are no guarantees that this will magically happen in the right
> order, split the scanning of the busses in 2 phases, first main controllers,

buses

> and then companion controllers.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/host/usb-uclass.c | 30 +++++++++++++++++++++++++-----
>  include/usb.h                 |  2 ++
>  2 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index ad778b4..d745c1c 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -157,6 +157,9 @@ static void usb_scan_bus(struct udevice *bus, bool recurse)
>
>         assert(recurse);        /* TODO: Support non-recusive */
>
> +       if (!device_active(bus))
> +               return;
> +
>         printf("scanning bus %d for devices... ", bus->seq);
>         debug("\n");
>         ret = usb_scan_device(bus, 0, USB_SPEED_FULL, &dev);
> @@ -171,6 +174,7 @@ static void usb_scan_bus(struct udevice *bus, bool recurse)
>  int usb_init(void)
>  {
>         int controllers_initialized = 0;
> +       struct usb_bus_priv *priv;
>         struct udevice *bus;
>         struct uclass *uc;
>         int count = 0;
> @@ -198,15 +202,31 @@ int usb_init(void)
>                         printf("probe failed, error %d\n", ret);
>                         continue;
>                 }
> -               /*
> -                * lowlevel init is OK, now scan the bus for devices
> -                * i.e. search HUBs and configure them
> -                */
>                 controllers_initialized++;
> -               usb_scan_bus(bus, true);
>                 usb_started = true;
>         }
>
> +       /*
> +        * lowlevel init done, now scan the bus for devices i.e. search HUBs
> +        * and configure them, first scan primary controllers.
> +        */
> +       uclass_foreach_dev(bus, uc) {
> +               priv = dev_get_uclass_priv(bus);

Need to check device_active() first in case the probe failed.

Also are the companions associated with a separate device, or the same
one? I'm a bit confused...

> +               if (!priv->companion)
> +                       usb_scan_bus(bus, true);
> +       }
> +
> +       /*
> +        * Now that the primary controllers have been scanned and have handed
> +        * over any devices they do not understand to their companions, scan
> +        * the companions.
> +        */
> +       uclass_foreach_dev(bus, uc) {
> +               priv = dev_get_uclass_priv(bus);
> +               if (priv->companion)
> +                       usb_scan_bus(bus, true);
> +       }
> +
>         debug("scan end\n");
>         /* if we were not able to find at least one working bus, bail out */
>         if (!count)
> diff --git a/include/usb.h b/include/usb.h
> index 7b55844..b81e796 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -608,10 +608,12 @@ struct usb_dev_platdata {
>   * @desc_before_addr:  true if we can read a device descriptor before it
>   *             has been assigned an address. For XHCI this is not possible
>   *             so this will be false.
> + * @companion:  True if this is a companion controler to another USB controller

controller

>   */
>  struct usb_bus_priv {
>         int next_addr;
>         bool desc_before_addr;
> +       bool companion;
>  };
>
>  /**
> --
> 2.3.6
>

Regards,
Simon

[U-Boot] [PATCH 3/5] dm: usb: Do not scan companion busses if no devices where handed over

[Simon Glass]

Hi Hans,

On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote:
> USB scanning is slow, and there is no need to scan the companion busses
> if no usb devices where handed over to the companinon controllers by any
> of the main controllers.
>
> This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1
> devices plugged into the root usb ports.
>
> The use of a global variable for the companion handover counting is somewhat
> unfortunate, but we do not have the necessary info to link companions and
> main controllers together in devicetree, and since this is just an
> optimization adding a custom devicetree extenstion for this seems undesirable.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/usb.c                  |  2 ++
>  drivers/usb/host/usb-uclass.c | 12 +++++++-----
>  include/usb.h                 |  3 +++
>  3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index 1b26bfa..4a09583 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -44,6 +44,8 @@
>
>  static int asynch_allowed;
>  char usb_started; /* flag for the started/stopped USB status */
> +/* Tracks how much devices were handed over to companion controllers */
> +int usb_companion_device_count;
>
>  #ifndef CONFIG_DM_USB
>  static struct usb_device usb_dev[USB_MAX_DEVICE];
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index d745c1c..877d307 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -219,12 +219,14 @@ int usb_init(void)
>         /*
>          * Now that the primary controllers have been scanned and have handed
>          * over any devices they do not understand to their companions, scan
> -        * the companions.
> +        * the companions if necessary.
>          */
> -       uclass_foreach_dev(bus, uc) {
> -               priv = dev_get_uclass_priv(bus);
> -               if (priv->companion)
> -                       usb_scan_bus(bus, true);
> +       if (usb_companion_device_count) {
> +               uclass_foreach_dev(bus, uc) {
> +                       priv = dev_get_uclass_priv(bus);
> +                       if (priv->companion)
> +                               usb_scan_bus(bus, true);
> +               }
>         }
>
>         debug("scan end\n");
> diff --git a/include/usb.h b/include/usb.h
> index b81e796..d4c9f44 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -171,6 +171,9 @@ enum usb_init_type {
>   * this is how the lowlevel part communicate with the outer world
>   */
>
> +/* Tracks how much devices were handed over to companion controllers */
> +extern int usb_companion_device_count;

Not keen on a global.

Could this be a per-bus value, and go in the controller's private
data? If not, uclass private data?

> +
>  #if defined(CONFIG_USB_UHCI) || defined(CONFIG_USB_OHCI) || \
>         defined(CONFIG_USB_EHCI) || defined(CONFIG_USB_OHCI_NEW) || \
>         defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \
> --
> 2.3.6
>

Regards,
Simon

[U-Boot] [PATCH 2/5] dm: usb: Add support for companion controllers

[Hans de Goede]

Hi Simon,

On 05/05/2015 11:46 PM, Simon Glass wrote:
> Hi Hans,
>
> On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote:
>> USB companion controllers must be scanned after the main controller has
>> been scanned, so that any devices which the main controller which to hand
>> over to the companion have actually been handed over before we scan the
>> companion.
>>
>> As there are no guarantees that this will magically happen in the right
>> order, split the scanning of the busses in 2 phases, first main controllers,
>
> buses
>
>> and then companion controllers.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/host/usb-uclass.c | 30 +++++++++++++++++++++++++-----
>>   include/usb.h                 |  2 ++
>>   2 files changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>> index ad778b4..d745c1c 100644
>> --- a/drivers/usb/host/usb-uclass.c
>> +++ b/drivers/usb/host/usb-uclass.c
>> @@ -157,6 +157,9 @@ static void usb_scan_bus(struct udevice *bus, bool recurse)
>>
>>          assert(recurse);        /* TODO: Support non-recusive */
>>
>> +       if (!device_active(bus))
>> +               return;
>> +
>>          printf("scanning bus %d for devices... ", bus->seq);
>>          debug("\n");
>>          ret = usb_scan_device(bus, 0, USB_SPEED_FULL, &dev);
>> @@ -171,6 +174,7 @@ static void usb_scan_bus(struct udevice *bus, bool recurse)
>>   int usb_init(void)
>>   {
>>          int controllers_initialized = 0;
>> +       struct usb_bus_priv *priv;
>>          struct udevice *bus;
>>          struct uclass *uc;
>>          int count = 0;
>> @@ -198,15 +202,31 @@ int usb_init(void)
>>                          printf("probe failed, error %d\n", ret);
>>                          continue;
>>                  }
>> -               /*
>> -                * lowlevel init is OK, now scan the bus for devices
>> -                * i.e. search HUBs and configure them
>> -                */
>>                  controllers_initialized++;
>> -               usb_scan_bus(bus, true);
>>                  usb_started = true;
>>          }
>>
>> +       /*
>> +        * lowlevel init done, now scan the bus for devices i.e. search HUBs
>> +        * and configure them, first scan primary controllers.
>> +        */
>> +       uclass_foreach_dev(bus, uc) {
>> +               priv = dev_get_uclass_priv(bus);
>
> Need to check device_active() first in case the probe failed.

Ah right, I cannot deref priv before the check, so adding the check to
usb_scan_bus() is no good, will fix.

> Also are the companions associated with a separate device, or the same
> one? I'm a bit confused...

With a traditional usb-2 setup each usb-2 controller (ehci controller)
has one or more usb-1 companions controllers (ohci or uhci) to deal with
usb-1 devices which get handed over to the companion by the usb-2 controller
on a per port basis.

E.g. older intel chipsets have a 6 port ehci controller with 3 2 port uhci
companions. For a total of 4 pci devices for the entire usb cluster.

I hope this clarifies things.

Regards,

Hans

>
>> +               if (!priv->companion)
>> +                       usb_scan_bus(bus, true);
>> +       }
>> +
>> +       /*
>> +        * Now that the primary controllers have been scanned and have handed
>> +        * over any devices they do not understand to their companions, scan
>> +        * the companions.
>> +        */
>> +       uclass_foreach_dev(bus, uc) {
>> +               priv = dev_get_uclass_priv(bus);
>> +               if (priv->companion)
>> +                       usb_scan_bus(bus, true);
>> +       }
>> +
>>          debug("scan end\n");
>>          /* if we were not able to find at least one working bus, bail out */
>>          if (!count)
>> diff --git a/include/usb.h b/include/usb.h
>> index 7b55844..b81e796 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -608,10 +608,12 @@ struct usb_dev_platdata {
>>    * @desc_before_addr:  true if we can read a device descriptor before it
>>    *             has been assigned an address. For XHCI this is not possible
>>    *             so this will be false.
>> + * @companion:  True if this is a companion controler to another USB controller
>
> controller
>
>>    */
>>   struct usb_bus_priv {
>>          int next_addr;
>>          bool desc_before_addr;
>> +       bool companion;
>>   };
>>
>>   /**
>> --
>> 2.3.6
>>
>
> Regards,
> Simon
>

[U-Boot] [PATCH 3/5] dm: usb: Do not scan companion busses if no devices where handed over

[Hans de Goede]

Hi,

On 05/05/2015 11:46 PM, Simon Glass wrote:
> Hi Hans,
>
> On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote:
>> USB scanning is slow, and there is no need to scan the companion busses
>> if no usb devices where handed over to the companinon controllers by any
>> of the main controllers.
>>
>> This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1
>> devices plugged into the root usb ports.
>>
>> The use of a global variable for the companion handover counting is somewhat
>> unfortunate, but we do not have the necessary info to link companions and
>> main controllers together in devicetree, and since this is just an
>> optimization adding a custom devicetree extenstion for this seems undesirable.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   common/usb.c                  |  2 ++
>>   drivers/usb/host/usb-uclass.c | 12 +++++++-----
>>   include/usb.h                 |  3 +++
>>   3 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index 1b26bfa..4a09583 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -44,6 +44,8 @@
>>
>>   static int asynch_allowed;
>>   char usb_started; /* flag for the started/stopped USB status */
>> +/* Tracks how much devices were handed over to companion controllers */
>> +int usb_companion_device_count;
>>
>>   #ifndef CONFIG_DM_USB
>>   static struct usb_device usb_dev[USB_MAX_DEVICE];
>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>> index d745c1c..877d307 100644
>> --- a/drivers/usb/host/usb-uclass.c
>> +++ b/drivers/usb/host/usb-uclass.c
>> @@ -219,12 +219,14 @@ int usb_init(void)
>>          /*
>>           * Now that the primary controllers have been scanned and have handed
>>           * over any devices they do not understand to their companions, scan
>> -        * the companions.
>> +        * the companions if necessary.
>>           */
>> -       uclass_foreach_dev(bus, uc) {
>> -               priv = dev_get_uclass_priv(bus);
>> -               if (priv->companion)
>> -                       usb_scan_bus(bus, true);
>> +       if (usb_companion_device_count) {
>> +               uclass_foreach_dev(bus, uc) {
>> +                       priv = dev_get_uclass_priv(bus);
>> +                       if (priv->companion)
>> +                               usb_scan_bus(bus, true);
>> +               }
>>          }
>>
>>          debug("scan end\n");
>> diff --git a/include/usb.h b/include/usb.h
>> index b81e796..d4c9f44 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -171,6 +171,9 @@ enum usb_init_type {
>>    * this is how the lowlevel part communicate with the outer world
>>    */
>>
>> +/* Tracks how much devices were handed over to companion controllers */
>> +extern int usb_companion_device_count;
>
> Not keen on a global.
>
> Could this be a per-bus value, and go in the controller's private
> data?

No ideally it would be per main + companion-controller(s) cluster, but
we do not know which controllers belong to each other, normal interrupt
driven operation does not care, as the companions do not generate any
events until devices are handed over.  Since we however do everything
synchronous it is a worthwhile optimization to skip the companions scan,
I believe that the best balance here is to just use a single var so that we
at least get the boot time gain when no handover is done at all.

> If not, uclass private data?

That would be tricky, as the hand over is deep inside the ehci code,
I can sure we can arrange a way to get to the uclass priv data there
and make it #ifdef CONFIG_DM_USB, but that does add yet another #ifdef.

And I've found another use for the global outside the uclass, see
the patch I've just posted titled:

"usb: Stop reset procedure when a dev is handed over to a companion hcd"

At first I wanted to use a -ENXIO return value from usb_control_msg to
indicate that a handover has happened, rather then do the compare
usb_companion_device_count before and after thing I'm doing now, the
problem is that currently usb_control_msg always returns 0 or -1 and
not any other error.

Oh wait I've just done a double check and I see that that is not true
actually. So I could do something like propagate an error instead of
the somewhat clunky usb_companion_device_count before and after thing,
and then this can indeed become a uclass priv value, would that be acceptable ?

Regards,

Hans


>
>> +
>>   #if defined(CONFIG_USB_UHCI) || defined(CONFIG_USB_OHCI) || \
>>          defined(CONFIG_USB_EHCI) || defined(CONFIG_USB_OHCI_NEW) || \
>>          defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \
>> --
>> 2.3.6
>>
>
> Regards,
> Simon
>

[U-Boot] [PATCH 3/5] dm: usb: Do not scan companion busses if no devices where handed over

[Simon Glass]

Hi Hans,

On 5 May 2015 at 16:14, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 05/05/2015 11:46 PM, Simon Glass wrote:
>>
>> Hi Hans,
>>
>> On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> USB scanning is slow, and there is no need to scan the companion busses
>>> if no usb devices where handed over to the companinon controllers by any
>>> of the main controllers.
>>>
>>> This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1
>>> devices plugged into the root usb ports.
>>>
>>> The use of a global variable for the companion handover counting is
>>> somewhat
>>> unfortunate, but we do not have the necessary info to link companions and
>>> main controllers together in devicetree, and since this is just an
>>> optimization adding a custom devicetree extenstion for this seems
>>> undesirable.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   common/usb.c                  |  2 ++
>>>   drivers/usb/host/usb-uclass.c | 12 +++++++-----
>>>   include/usb.h                 |  3 +++
>>>   3 files changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/common/usb.c b/common/usb.c
>>> index 1b26bfa..4a09583 100644
>>> --- a/common/usb.c
>>> +++ b/common/usb.c
>>> @@ -44,6 +44,8 @@
>>>
>>>   static int asynch_allowed;
>>>   char usb_started; /* flag for the started/stopped USB status */
>>> +/* Tracks how much devices were handed over to companion controllers */
>>> +int usb_companion_device_count;
>>>
>>>   #ifndef CONFIG_DM_USB
>>>   static struct usb_device usb_dev[USB_MAX_DEVICE];
>>> diff --git a/drivers/usb/host/usb-uclass.c
>>> b/drivers/usb/host/usb-uclass.c
>>> index d745c1c..877d307 100644
>>> --- a/drivers/usb/host/usb-uclass.c
>>> +++ b/drivers/usb/host/usb-uclass.c
>>> @@ -219,12 +219,14 @@ int usb_init(void)
>>>          /*
>>>           * Now that the primary controllers have been scanned and have
>>> handed
>>>           * over any devices they do not understand to their companions,
>>> scan
>>> -        * the companions.
>>> +        * the companions if necessary.
>>>           */
>>> -       uclass_foreach_dev(bus, uc) {
>>> -               priv = dev_get_uclass_priv(bus);
>>> -               if (priv->companion)
>>> -                       usb_scan_bus(bus, true);
>>> +       if (usb_companion_device_count) {
>>> +               uclass_foreach_dev(bus, uc) {
>>> +                       priv = dev_get_uclass_priv(bus);
>>> +                       if (priv->companion)
>>> +                               usb_scan_bus(bus, true);
>>> +               }
>>>          }
>>>
>>>          debug("scan end\n");
>>> diff --git a/include/usb.h b/include/usb.h
>>> index b81e796..d4c9f44 100644
>>> --- a/include/usb.h
>>> +++ b/include/usb.h
>>> @@ -171,6 +171,9 @@ enum usb_init_type {
>>>    * this is how the lowlevel part communicate with the outer world
>>>    */
>>>
>>> +/* Tracks how much devices were handed over to companion controllers */
>>> +extern int usb_companion_device_count;
>>
>>
>> Not keen on a global.
>>
>> Could this be a per-bus value, and go in the controller's private
>> data?
>
>
> No ideally it would be per main + companion-controller(s) cluster, but
> we do not know which controllers belong to each other, normal interrupt
> driven operation does not care, as the companions do not generate any
> events until devices are handed over.  Since we however do everything
> synchronous it is a worthwhile optimization to skip the companions scan,
> I believe that the best balance here is to just use a single var so that we
> at least get the boot time gain when no handover is done at all.
>
>> If not, uclass private data?
>
>
> That would be tricky, as the hand over is deep inside the ehci code,
> I can sure we can arrange a way to get to the uclass priv data there
> and make it #ifdef CONFIG_DM_USB, but that does add yet another #ifdef.
>
> And I've found another use for the global outside the uclass, see
> the patch I've just posted titled:
>
> "usb: Stop reset procedure when a dev is handed over to a companion hcd"
>
> At first I wanted to use a -ENXIO return value from usb_control_msg to
> indicate that a handover has happened, rather then do the compare
> usb_companion_device_count before and after thing I'm doing now, the
> problem is that currently usb_control_msg always returns 0 or -1 and
> not any other error.
>
> Oh wait I've just done a double check and I see that that is not true
> actually. So I could do something like propagate an error instead of
> the somewhat clunky usb_companion_device_count before and after thing,
> and then this can indeed become a uclass priv value, would that be
> acceptable ?

Let's try that, it seems better.

I do understand the challenge in moving this stuff over.

Regards,
Simon