[U-Boot] [PATCH 00/16] usb: hub: Support USB 3.0 hubs

[U-Boot] [PATCH 00/16] usb: hub: Support USB 3.0 hubs

[Bin Meng]

This series is a follow-up series to previous series [1] that adds
support for USB 3.0 hubs.

USB 3.0 hubs have slightly different hub descriptor format, as well
as different port status bit positions. These needs to be properly
handled by U-Boot USB core stack codes. xHCI driver has also been
updated to correctly set up context structures that are required
for devices behind a 3.0 hub to work.

Besides USB 3.0 hubs support, this series also adds support for low
speed/full speed devices connected behind a high speed hub with the
xHC controller. It turns out the 'Transaction Translator' part in
the xHC data structure is missing.

Note there is one error during testing USB keyboard when
CONFIG_USB_KEYBOARD is on.

  "Failed to get keyboard state from device 413c:2105"

The enumeration process did pass and the error was thrown in the
USB keyboard driver. This was due to the interrupt transfer is
still not supported by xHCI driver, as of today.

This series is available at u-boot-x86/xhci-working2 for testing.

[1] https://lists.denx.de/pipermail/u-boot/2017-June/296166.html


Bin Meng (16):
  usb: xhci-pci: Drop non-DM version of xhci-pci driver
  usb: xhci-pci: Clean up the driver a little bit
  usb: Remove unnecessary work in usb_setup_descriptor()
  usb: hub: Use 'struct usb_hub_device' as hub device's uclass_priv
  usb: hub: Add a new API to test if a hub device is root hub
  usb: xhci: Change to use usb_hub_is_root_hub() API
  usb: hub: Translate USB 3.0 hub port status into old version
  usb: hub: Support 'set hub depth' request for USB 3.0 hubs
  usb: xhci: Change xhci_setup_addressable_virt_dev() signature
  usb: xhci: Program 'route string' in the input slot context
  usb: hub: Parse and save TT details from device descriptor
  dm: usb: Add a new USB controller operation 'update_hub_device'
  usb: hub: Call usb_update_hub_device() after hub descriptor is fetched
  usb: xhci: Implement update_hub_device() operation
  usb: xhci: Correct TT_SLOT and TT_PORT macros
  usb: xhci: Enable TT to support LS/FS devices behind a HS hub

 common/usb.c                  |  40 +++++-----
 common/usb_hub.c              | 166 +++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/host/usb-uclass.c |  13 +++-
 drivers/usb/host/xhci-mem.c   |  40 +++++++++-
 drivers/usb/host/xhci-pci.c   |  68 +----------------
 drivers/usb/host/xhci.c       |  85 +++++++++++++++------
 drivers/usb/host/xhci.h       |   8 +-
 include/usb.h                 |  46 +++++++++++-
 include/usb_defs.h            |  15 ++++
 9 files changed, 357 insertions(+), 124 deletions(-)

-- 
2.9.2

[U-Boot] [PATCH 01/16] usb: xhci-pci: Drop non-DM version of xhci-pci driver

[Bin Meng]

As there is no board that currently uses xhci-pci driver without DM
USB, drop its support and leave only DM support.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci-pci.c | 52 ---------------------------------------------
 1 file changed, 52 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 63daaa6..5ad8452 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -14,56 +14,6 @@
 
 #include "xhci.h"
 
-#ifndef CONFIG_DM_USB
-
-/*
- * Create the appropriate control structures to manage a new XHCI host
- * controller.
- */
-int xhci_hcd_init(int index, struct xhci_hccr **ret_hccr,
-		  struct xhci_hcor **ret_hcor)
-{
-	struct xhci_hccr *hccr;
-	struct xhci_hcor *hcor;
-	pci_dev_t pdev;
-	uint32_t cmd;
-	int len;
-
-	pdev = pci_find_class(PCI_CLASS_SERIAL_USB_XHCI, index);
-	if (pdev < 0) {
-		printf("XHCI host controller not found\n");
-		return -1;
-	}
-
-	hccr = (struct xhci_hccr *)pci_map_bar(pdev,
-			PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
-	len = HC_LENGTH(xhci_readl(&hccr->cr_capbase));
-	hcor = (struct xhci_hcor *)((uint32_t)hccr + len);
-
-	debug("XHCI-PCI init hccr 0x%x and hcor 0x%x hc_length %d\n",
-	      (uint32_t)hccr, (uint32_t)hcor, len);
-
-	*ret_hccr = hccr;
-	*ret_hcor = hcor;
-
-	/* enable busmaster */
-	pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
-	cmd |= PCI_COMMAND_MASTER;
-	pci_write_config_dword(pdev, PCI_COMMAND, cmd);
-
-	return 0;
-}
-
-/*
- * Destroy the appropriate control structures corresponding * to the XHCI host
- * controller
- */
-void xhci_hcd_stop(int index)
-{
-}
-
-#else
-
 struct xhci_pci_priv {
 	struct xhci_ctrl ctrl;	/* Needs to come first in this struct! */
 };
@@ -137,5 +87,3 @@ static struct pci_device_id xhci_pci_supported[] = {
 };
 
 U_BOOT_PCI_DEVICE(xhci_pci, xhci_pci_supported);
-
-#endif /* CONFIG_DM_USB */
-- 
2.9.2

[U-Boot] [PATCH 01/16] usb: xhci-pci: Drop non-DM version of xhci-pci driver

[Marek Vasut]

On 06/23/2017 11:54 AM, Bin Meng wrote:
> As there is no board that currently uses xhci-pci driver without DM
> USB, drop its support and leave only DM support.

You should add something into the Kconfig to make this driver depend on
DM_USB ; unless it's already there.

> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/usb/host/xhci-pci.c | 52 ---------------------------------------------
>  1 file changed, 52 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 63daaa6..5ad8452 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -14,56 +14,6 @@
>  
>  #include "xhci.h"
>  
> -#ifndef CONFIG_DM_USB
> -
> -/*
> - * Create the appropriate control structures to manage a new XHCI host
> - * controller.
> - */
> -int xhci_hcd_init(int index, struct xhci_hccr **ret_hccr,
> -		  struct xhci_hcor **ret_hcor)
> -{
> -	struct xhci_hccr *hccr;
> -	struct xhci_hcor *hcor;
> -	pci_dev_t pdev;
> -	uint32_t cmd;
> -	int len;
> -
> -	pdev = pci_find_class(PCI_CLASS_SERIAL_USB_XHCI, index);
> -	if (pdev < 0) {
> -		printf("XHCI host controller not found\n");
> -		return -1;
> -	}
> -
> -	hccr = (struct xhci_hccr *)pci_map_bar(pdev,
> -			PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
> -	len = HC_LENGTH(xhci_readl(&hccr->cr_capbase));
> -	hcor = (struct xhci_hcor *)((uint32_t)hccr + len);
> -
> -	debug("XHCI-PCI init hccr 0x%x and hcor 0x%x hc_length %d\n",
> -	      (uint32_t)hccr, (uint32_t)hcor, len);
> -
> -	*ret_hccr = hccr;
> -	*ret_hcor = hcor;
> -
> -	/* enable busmaster */
> -	pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
> -	cmd |= PCI_COMMAND_MASTER;
> -	pci_write_config_dword(pdev, PCI_COMMAND, cmd);
> -
> -	return 0;
> -}
> -
> -/*
> - * Destroy the appropriate control structures corresponding * to the XHCI host
> - * controller
> - */
> -void xhci_hcd_stop(int index)
> -{
> -}
> -
> -#else
> -
>  struct xhci_pci_priv {
>  	struct xhci_ctrl ctrl;	/* Needs to come first in this struct! */
>  };
> @@ -137,5 +87,3 @@ static struct pci_device_id xhci_pci_supported[] = {
>  };
>  
>  U_BOOT_PCI_DEVICE(xhci_pci, xhci_pci_supported);
> -
> -#endif /* CONFIG_DM_USB */
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 01/16] usb: xhci-pci: Drop non-DM version of xhci-pci driver

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> As there is no board that currently uses xhci-pci driver without DM
> USB, drop its support and leave only DM support.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/usb/host/xhci-pci.c | 52 ---------------------------------------------
>  1 file changed, 52 deletions(-)
>

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

Note for Marek: CONFIG_USB_XHCI_PCI is not in Kconfig yet.

[U-Boot] [PATCH 02/16] usb: xhci-pci: Clean up the driver a little bit

[Bin Meng]

This cleans up the driver a little bit.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci-pci.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5ad8452..56fd650 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -8,16 +8,10 @@
 
 #include <common.h>
 #include <dm.h>
-#include <errno.h>
 #include <pci.h>
 #include <usb.h>
-
 #include "xhci.h"
 
-struct xhci_pci_priv {
-	struct xhci_ctrl ctrl;	/* Needs to come first in this struct! */
-};
-
 static void xhci_pci_init(struct udevice *dev, struct xhci_hccr **ret_hccr,
 			  struct xhci_hcor **ret_hcor)
 {
@@ -55,13 +49,7 @@ static int xhci_pci_probe(struct udevice *dev)
 
 static int xhci_pci_remove(struct udevice *dev)
 {
-	int ret;
-
-	ret = xhci_deregister(dev);
-	if (ret)
-		return ret;
-
-	return 0;
+	return xhci_deregister(dev);
 }
 
 static const struct udevice_id xhci_pci_ids[] = {
@@ -77,7 +65,7 @@ U_BOOT_DRIVER(xhci_pci) = {
 	.of_match = xhci_pci_ids,
 	.ops	= &xhci_usb_ops,
 	.platdata_auto_alloc_size = sizeof(struct usb_platdata),
-	.priv_auto_alloc_size = sizeof(struct xhci_pci_priv),
+	.priv_auto_alloc_size = sizeof(struct xhci_ctrl),
 	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
 };
 
-- 
2.9.2

[U-Boot] [PATCH 02/16] usb: xhci-pci: Clean up the driver a little bit

[Marek Vasut]

On 06/23/2017 11:54 AM, Bin Meng wrote:
> This cleans up the driver a little bit.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/usb/host/xhci-pci.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 5ad8452..56fd650 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -8,16 +8,10 @@
>  
>  #include <common.h>
>  #include <dm.h>
> -#include <errno.h>
>  #include <pci.h>
>  #include <usb.h>
> -
>  #include "xhci.h"
>  
> -struct xhci_pci_priv {
> -	struct xhci_ctrl ctrl;	/* Needs to come first in this struct! */
> -};
> -
>  static void xhci_pci_init(struct udevice *dev, struct xhci_hccr **ret_hccr,
>  			  struct xhci_hcor **ret_hcor)
>  {
> @@ -55,13 +49,7 @@ static int xhci_pci_probe(struct udevice *dev)
>  
>  static int xhci_pci_remove(struct udevice *dev)
>  {
> -	int ret;
> -
> -	ret = xhci_deregister(dev);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return xhci_deregister(dev);

Can you insert xhci_deregister directly into the callbacks structure and
nuke xhci_pci_remove() altogether ?

>  }
>  
>  static const struct udevice_id xhci_pci_ids[] = {
> @@ -77,7 +65,7 @@ U_BOOT_DRIVER(xhci_pci) = {
>  	.of_match = xhci_pci_ids,
>  	.ops	= &xhci_usb_ops,
>  	.platdata_auto_alloc_size = sizeof(struct usb_platdata),
> -	.priv_auto_alloc_size = sizeof(struct xhci_pci_priv),
> +	.priv_auto_alloc_size = sizeof(struct xhci_ctrl),
>  	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
>  };
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 02/16] usb: xhci-pci: Clean up the driver a little bit

[Simon Glass]

On 23 June 2017 at 11:52, Marek Vasut <marex@denx.de> wrote:
> On 06/23/2017 11:54 AM, Bin Meng wrote:
>> This cleans up the driver a little bit.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  drivers/usb/host/xhci-pci.c | 16 ++--------------
>>  1 file changed, 2 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 5ad8452..56fd650 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -8,16 +8,10 @@
>>
>>  #include <common.h>
>>  #include <dm.h>
>> -#include <errno.h>
>>  #include <pci.h>
>>  #include <usb.h>
>> -
>>  #include "xhci.h"
>>
>> -struct xhci_pci_priv {
>> -     struct xhci_ctrl ctrl;  /* Needs to come first in this struct! */
>> -};
>> -
>>  static void xhci_pci_init(struct udevice *dev, struct xhci_hccr **ret_hccr,
>>                         struct xhci_hcor **ret_hcor)
>>  {
>> @@ -55,13 +49,7 @@ static int xhci_pci_probe(struct udevice *dev)
>>
>>  static int xhci_pci_remove(struct udevice *dev)
>>  {
>> -     int ret;
>> -
>> -     ret = xhci_deregister(dev);
>> -     if (ret)
>> -             return ret;
>> -
>> -     return 0;
>> +     return xhci_deregister(dev);
>
> Can you insert xhci_deregister directly into the callbacks structure and
> nuke xhci_pci_remove() altogether ?

Either way:

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

>
>>  }
>>
>>  static const struct udevice_id xhci_pci_ids[] = {
>> @@ -77,7 +65,7 @@ U_BOOT_DRIVER(xhci_pci) = {
>>       .of_match = xhci_pci_ids,
>>       .ops    = &xhci_usb_ops,
>>       .platdata_auto_alloc_size = sizeof(struct usb_platdata),
>> -     .priv_auto_alloc_size = sizeof(struct xhci_pci_priv),
>> +     .priv_auto_alloc_size = sizeof(struct xhci_ctrl),
>>       .flags  = DM_FLAG_ALLOC_PRIV_DMA,
>>  };
>>
>>
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH 03/16] usb: Remove unnecessary work in usb_setup_descriptor()

[Bin Meng]

The only work we need do in usb_setup_descriptor() is to initialize
dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps
are the same as do_read being true.

While we are here, update the comment block to be within 80 cols.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 15e1e4c..293d968 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
 	 * some more, or keeps on retransmitting the 8 byte header.
 	 */
 
-	if (dev->speed == USB_SPEED_LOW) {
-		dev->descriptor.bMaxPacketSize0 = 8;
-		dev->maxpacketsize = PACKET_SIZE_8;
-	} else {
-		dev->descriptor.bMaxPacketSize0 = 64;
-		dev->maxpacketsize = PACKET_SIZE_64;
-	}
-	dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
-	dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
-
 	if (do_read) {
 		int err;
 
 		/*
-		 * Validate we've received only at least 8 bytes, not that we've
-		 * received the entire descriptor. The reasoning is:
-		 * - The code only uses fields in the first 8 bytes, so that's all we
-		 *   need to have fetched at this stage.
-		 * - The smallest maxpacket size is 8 bytes. Before we know the actual
-		 *   maxpacket the device uses, the USB controller may only accept a
-		 *   single packet. Consequently we are only guaranteed to receive 1
-		 *   packet (at least 8 bytes) even in a non-error case.
+		 * Validate we've received only at least 8 bytes, not that
+		 * we've received the entire descriptor. The reasoning is:
+		 * - The code only uses fields in the first 8 bytes, so that's
+		 *   all we need to have fetched at this stage.
+		 * - The smallest maxpacket size is 8 bytes. Before we know
+		 *   the actual maxpacket the device uses, the USB controller
+		 *   may only accept a single packet. Consequently we are only
+		 *   guaranteed to receive 1 packet (at least 8 bytes) even in
+		 *   a non-error case.
 		 *
-		 * At least the DWC2 controller needs to be programmed with the number
-		 * of packets in addition to the number of bytes. A request for 64
-		 * bytes of data with the maxpacket guessed as 64 (above) yields a
-		 * request for 1 packet.
+		 * At least the DWC2 controller needs to be programmed with
+		 * the number of packets in addition to the number of bytes.
+		 * A request for 64 bytes of data with the maxpacket guessed
+		 * as 64 (above) yields a request for 1 packet.
 		 */
 		err = get_descriptor_len(dev, 64, 8);
 		if (err)
 			return err;
+	} else {
+		if (dev->speed == USB_SPEED_LOW)
+			dev->descriptor.bMaxPacketSize0 = 8;
+		else
+			dev->descriptor.bMaxPacketSize0 = 64;
 	}
 
 	dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
-- 
2.9.2

[U-Boot] [PATCH 03/16] usb: Remove unnecessary work in usb_setup_descriptor()

[Bin Meng]

Hi,

On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> The only work we need do in usb_setup_descriptor() is to initialize
> dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps
> are the same as do_read being true.
>
> While we are here, update the comment block to be within 80 cols.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  common/usb.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index 15e1e4c..293d968 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
>          * some more, or keeps on retransmitting the 8 byte header.
>          */
>
> -       if (dev->speed == USB_SPEED_LOW) {
> -               dev->descriptor.bMaxPacketSize0 = 8;
> -               dev->maxpacketsize = PACKET_SIZE_8;
> -       } else {
> -               dev->descriptor.bMaxPacketSize0 = 64;
> -               dev->maxpacketsize = PACKET_SIZE_64;
> -       }
> -       dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
> -       dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
> -
>         if (do_read) {
>                 int err;
>
>                 /*
> -                * Validate we've received only at least 8 bytes, not that we've
> -                * received the entire descriptor. The reasoning is:
> -                * - The code only uses fields in the first 8 bytes, so that's all we
> -                *   need to have fetched at this stage.
> -                * - The smallest maxpacket size is 8 bytes. Before we know the actual
> -                *   maxpacket the device uses, the USB controller may only accept a
> -                *   single packet. Consequently we are only guaranteed to receive 1
> -                *   packet (at least 8 bytes) even in a non-error case.
> +                * Validate we've received only at least 8 bytes, not that
> +                * we've received the entire descriptor. The reasoning is:
> +                * - The code only uses fields in the first 8 bytes, so that's
> +                *   all we need to have fetched at this stage.
> +                * - The smallest maxpacket size is 8 bytes. Before we know
> +                *   the actual maxpacket the device uses, the USB controller
> +                *   may only accept a single packet. Consequently we are only
> +                *   guaranteed to receive 1 packet (at least 8 bytes) even in
> +                *   a non-error case.
>                  *
> -                * At least the DWC2 controller needs to be programmed with the number
> -                * of packets in addition to the number of bytes. A request for 64
> -                * bytes of data with the maxpacket guessed as 64 (above) yields a
> -                * request for 1 packet.
> +                * At least the DWC2 controller needs to be programmed with
> +                * the number of packets in addition to the number of bytes.
> +                * A request for 64 bytes of data with the maxpacket guessed
> +                * as 64 (above) yields a request for 1 packet.
>                  */
>                 err = get_descriptor_len(dev, 64, 8);
>                 if (err)
>                         return err;
> +       } else {
> +               if (dev->speed == USB_SPEED_LOW)
> +                       dev->descriptor.bMaxPacketSize0 = 8;
> +               else
> +                       dev->descriptor.bMaxPacketSize0 = 64;
>         }
>
>         dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
> --

For some reason that I don't understand, this patch breaks EHCI.
dev->maxpacketsize is equal to zero with this patch, which leads to a
'divide error' exception in ehci_submit_async(). Not sure if anyone
has some hints?

For now, I will just drop this patch in v2. This needs be further revisited.

Regards,
Bin

[U-Boot] [PATCH 03/16] usb: Remove unnecessary work in usb_setup_descriptor()

[Lothar Waßmann]

Hi,

On Fri, 30 Jun 2017 11:49:56 +0800 Bin Meng wrote:
> Hi,
> 
> On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> > The only work we need do in usb_setup_descriptor() is to initialize
> > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps
> > are the same as do_read being true.
> >
> > While we are here, update the comment block to be within 80 cols.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  common/usb.c | 40 ++++++++++++++++++----------------------
> >  1 file changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/common/usb.c b/common/usb.c
> > index 15e1e4c..293d968 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
> >          * some more, or keeps on retransmitting the 8 byte header.
> >          */
> >
> > -       if (dev->speed == USB_SPEED_LOW) {
> > -               dev->descriptor.bMaxPacketSize0 = 8;
> > -               dev->maxpacketsize = PACKET_SIZE_8;
> > -       } else {
> > -               dev->descriptor.bMaxPacketSize0 = 64;
> > -               dev->maxpacketsize = PACKET_SIZE_64;
> > -       }
> > -       dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
> > -       dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
> > -
> >         if (do_read) {
> >                 int err;
> >
> >                 /*
> > -                * Validate we've received only at least 8 bytes, not that we've
> > -                * received the entire descriptor. The reasoning is:
> > -                * - The code only uses fields in the first 8 bytes, so that's all we
> > -                *   need to have fetched at this stage.
> > -                * - The smallest maxpacket size is 8 bytes. Before we know the actual
> > -                *   maxpacket the device uses, the USB controller may only accept a
> > -                *   single packet. Consequently we are only guaranteed to receive 1
> > -                *   packet (at least 8 bytes) even in a non-error case.
> > +                * Validate we've received only at least 8 bytes, not that
> > +                * we've received the entire descriptor. The reasoning is:
> > +                * - The code only uses fields in the first 8 bytes, so that's
> > +                *   all we need to have fetched at this stage.
> > +                * - The smallest maxpacket size is 8 bytes. Before we know
> > +                *   the actual maxpacket the device uses, the USB controller
> > +                *   may only accept a single packet. Consequently we are only
> > +                *   guaranteed to receive 1 packet (at least 8 bytes) even in
> > +                *   a non-error case.
> >                  *
> > -                * At least the DWC2 controller needs to be programmed with the number
> > -                * of packets in addition to the number of bytes. A request for 64
> > -                * bytes of data with the maxpacket guessed as 64 (above) yields a
> > -                * request for 1 packet.
> > +                * At least the DWC2 controller needs to be programmed with
> > +                * the number of packets in addition to the number of bytes.
> > +                * A request for 64 bytes of data with the maxpacket guessed
> > +                * as 64 (above) yields a request for 1 packet.
> >                  */
> >                 err = get_descriptor_len(dev, 64, 8);
> >                 if (err)
> >                         return err;
> > +       } else {
> > +               if (dev->speed == USB_SPEED_LOW)
> > +                       dev->descriptor.bMaxPacketSize0 = 8;
> > +               else
> > +                       dev->descriptor.bMaxPacketSize0 = 64;
> >         }
> >
> >         dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
> > --
> 
> For some reason that I don't understand, this patch breaks EHCI.
> dev->maxpacketsize is equal to zero with this patch, which leads to a
> 'divide error' exception in ehci_submit_async(). Not sure if anyone
> has some hints?
> 
In the do_read case the dev->descriptor.bMaxPacketSize0 is not set up.


Lothar Waßmann

[U-Boot] [PATCH 03/16] usb: Remove unnecessary work in usb_setup_descriptor()

[Bin Meng]

Hi Lothar,

On Fri, Jun 30, 2017 at 2:15 PM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Fri, 30 Jun 2017 11:49:56 +0800 Bin Meng wrote:
>> Hi,
>>
>> On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> > The only work we need do in usb_setup_descriptor() is to initialize
>> > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps
>> > are the same as do_read being true.
>> >
>> > While we are here, update the comment block to be within 80 cols.
>> >
>> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> > ---
>> >
>> >  common/usb.c | 40 ++++++++++++++++++----------------------
>> >  1 file changed, 18 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/common/usb.c b/common/usb.c
>> > index 15e1e4c..293d968 100644
>> > --- a/common/usb.c
>> > +++ b/common/usb.c
>> > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
>> >          * some more, or keeps on retransmitting the 8 byte header.
>> >          */
>> >
>> > -       if (dev->speed == USB_SPEED_LOW) {
>> > -               dev->descriptor.bMaxPacketSize0 = 8;
>> > -               dev->maxpacketsize = PACKET_SIZE_8;
>> > -       } else {
>> > -               dev->descriptor.bMaxPacketSize0 = 64;
>> > -               dev->maxpacketsize = PACKET_SIZE_64;
>> > -       }
>> > -       dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
>> > -       dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
>> > -
>> >         if (do_read) {
>> >                 int err;
>> >
>> >                 /*
>> > -                * Validate we've received only at least 8 bytes, not that we've
>> > -                * received the entire descriptor. The reasoning is:
>> > -                * - The code only uses fields in the first 8 bytes, so that's all we
>> > -                *   need to have fetched at this stage.
>> > -                * - The smallest maxpacket size is 8 bytes. Before we know the actual
>> > -                *   maxpacket the device uses, the USB controller may only accept a
>> > -                *   single packet. Consequently we are only guaranteed to receive 1
>> > -                *   packet (at least 8 bytes) even in a non-error case.
>> > +                * Validate we've received only at least 8 bytes, not that
>> > +                * we've received the entire descriptor. The reasoning is:
>> > +                * - The code only uses fields in the first 8 bytes, so that's
>> > +                *   all we need to have fetched at this stage.
>> > +                * - The smallest maxpacket size is 8 bytes. Before we know
>> > +                *   the actual maxpacket the device uses, the USB controller
>> > +                *   may only accept a single packet. Consequently we are only
>> > +                *   guaranteed to receive 1 packet (at least 8 bytes) even in
>> > +                *   a non-error case.
>> >                  *
>> > -                * At least the DWC2 controller needs to be programmed with the number
>> > -                * of packets in addition to the number of bytes. A request for 64
>> > -                * bytes of data with the maxpacket guessed as 64 (above) yields a
>> > -                * request for 1 packet.
>> > +                * At least the DWC2 controller needs to be programmed with
>> > +                * the number of packets in addition to the number of bytes.
>> > +                * A request for 64 bytes of data with the maxpacket guessed
>> > +                * as 64 (above) yields a request for 1 packet.
>> >                  */
>> >                 err = get_descriptor_len(dev, 64, 8);
>> >                 if (err)
>> >                         return err;
>> > +       } else {
>> > +               if (dev->speed == USB_SPEED_LOW)
>> > +                       dev->descriptor.bMaxPacketSize0 = 8;
>> > +               else
>> > +                       dev->descriptor.bMaxPacketSize0 = 64;
>> >         }
>> >
>> >         dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
>> > --
>>
>> For some reason that I don't understand, this patch breaks EHCI.
>> dev->maxpacketsize is equal to zero with this patch, which leads to a
>> 'divide error' exception in ehci_submit_async(). Not sure if anyone
>> has some hints?
>>
> In the do_read case the dev->descriptor.bMaxPacketSize0 is not set up.
>

In that case, dev->descriptor.bMaxPacketSize0 was read from device. Am
I missing anything?

Regards,
Bin

[U-Boot] [PATCH 03/16] usb: Remove unnecessary work in usb_setup_descriptor()

[stefan.bruens at rwth-aachen.de]

On Freitag, 30. Juni 2017 05:49:56 CEST Bin Meng wrote:
> Hi,
> 
> On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> > The only work we need do in usb_setup_descriptor() is to initialize
> > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps
> > are the same as do_read being true.
> > 
> > While we are here, update the comment block to be within 80 cols.
> > 
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> > 
> >  common/usb.c | 40 ++++++++++++++++++----------------------
> >  1 file changed, 18 insertions(+), 22 deletions(-)
> > 
> > diff --git a/common/usb.c b/common/usb.c
> > index 15e1e4c..293d968 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device
> > *dev, bool do_read)> 
> >          * some more, or keeps on retransmitting the 8 byte header.
> >          */
> > 
> > -       if (dev->speed == USB_SPEED_LOW) {
> > -               dev->descriptor.bMaxPacketSize0 = 8;
> > -               dev->maxpacketsize = PACKET_SIZE_8;
> > -       } else {
> > -               dev->descriptor.bMaxPacketSize0 = 64;
> > -               dev->maxpacketsize = PACKET_SIZE_64;
> > -       }
> > -       dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
> > -       dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
> > -

You remove the initialization of dev->maxpacketsize here ...

> > 
> >         if (do_read) {
> >         
[...]
> > 
> >                  */
> >                 
> >                 err = get_descriptor_len(dev, 64, 8);
> >                 if (err)
> >                 
> >                         return err;

... and probably return early here. Can you add some debug output to 
get_descriptor_len(...) (len, expected len, return code)?

> > 
> > +       } else {
> > +               if (dev->speed == USB_SPEED_LOW)
> > +                       dev->descriptor.bMaxPacketSize0 = 8;
> > +               else
> > +                       dev->descriptor.bMaxPacketSize0 = 64;
> > 
> >         }
> >         
> >         dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
> > 
> > --
> 
> For some reason that I don't understand, this patch breaks EHCI.
> dev->maxpacketsize is equal to zero with this patch, which leads to a
> 'divide error' exception in ehci_submit_async(). Not sure if anyone
> has some hints?
> 
> For now, I will just drop this patch in v2. This needs be further revisited.

Kind regards,

Stefan

[U-Boot] [PATCH 03/16] usb: Remove unnecessary work in usb_setup_descriptor()

[Bin Meng]

Hi Stefan,

On Sun, Jul 2, 2017 at 1:29 AM,  <stefan.bruens@rwth-aachen.de> wrote:
> On Freitag, 30. Juni 2017 05:49:56 CEST Bin Meng wrote:
>> Hi,
>>
>> On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> > The only work we need do in usb_setup_descriptor() is to initialize
>> > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps
>> > are the same as do_read being true.
>> >
>> > While we are here, update the comment block to be within 80 cols.
>> >
>> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> > ---
>> >
>> >  common/usb.c | 40 ++++++++++++++++++----------------------
>> >  1 file changed, 18 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/common/usb.c b/common/usb.c
>> > index 15e1e4c..293d968 100644
>> > --- a/common/usb.c
>> > +++ b/common/usb.c
>> > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device
>> > *dev, bool do_read)>
>> >          * some more, or keeps on retransmitting the 8 byte header.
>> >          */
>> >
>> > -       if (dev->speed == USB_SPEED_LOW) {
>> > -               dev->descriptor.bMaxPacketSize0 = 8;
>> > -               dev->maxpacketsize = PACKET_SIZE_8;
>> > -       } else {
>> > -               dev->descriptor.bMaxPacketSize0 = 64;
>> > -               dev->maxpacketsize = PACKET_SIZE_64;
>> > -       }
>> > -       dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
>> > -       dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
>> > -
>
> You remove the initialization of dev->maxpacketsize here ...
>
>> >
>> >         if (do_read) {
>> >
> [...]
>> >
>> >                  */
>> >
>> >                 err = get_descriptor_len(dev, 64, 8);
>> >                 if (err)
>> >
>> >                         return err;
>
> ... and probably return early here. Can you add some debug output to
> get_descriptor_len(...) (len, expected len, return code)?
>

The get_descriptor_len() was successful, so it is not caused by the
"if (err)" branch.

>> >
>> > +       } else {
>> > +               if (dev->speed == USB_SPEED_LOW)
>> > +                       dev->descriptor.bMaxPacketSize0 = 8;
>> > +               else
>> > +                       dev->descriptor.bMaxPacketSize0 = 64;
>> >
>> >         }
>> >
>> >         dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
>> >
>> > --
>>
>> For some reason that I don't understand, this patch breaks EHCI.
>> dev->maxpacketsize is equal to zero with this patch, which leads to a
>> 'divide error' exception in ehci_submit_async(). Not sure if anyone
>> has some hints?
>>
>> For now, I will just drop this patch in v2. This needs be further revisited.
>

Regards,
Bin

[U-Boot] [PATCH 04/16] usb: hub: Use 'struct usb_hub_device' as hub device's uclass_priv

[Bin Meng]

Use USB hub device's dev->uclass_priv to point to 'usb_hub_device'
so that with driver model usb_hub_reset() and usb_hub_allocate()
are no longer needed.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_hub.c              | 10 +++++++++-
 drivers/usb/host/usb-uclass.c |  2 --
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 090966b..18bd827 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -58,9 +58,10 @@ struct usb_device_scan {
 	struct list_head list;
 };
 
-/* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
+#ifndef CONFIG_DM_USB
 static struct usb_hub_device hub_dev[USB_MAX_HUB];
 static int usb_hub_index;
+#endif
 static LIST_HEAD(usb_scan_list);
 
 __weak void usb_hub_reset_devices(int port)
@@ -167,6 +168,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 	      max(100, (int)pgood_delay) + 1000);
 }
 
+#ifndef CONFIG_DM_USB
 void usb_hub_reset(void)
 {
 	usb_hub_index = 0;
@@ -183,6 +185,7 @@ static struct usb_hub_device *usb_hub_allocate(void)
 	printf("ERROR: USB_MAX_HUB (%d) reached\n", USB_MAX_HUB);
 	return NULL;
 }
+#endif
 
 #define MAX_TRIES 5
 
@@ -557,10 +560,14 @@ static int usb_hub_configure(struct usb_device *dev)
 	__maybe_unused struct usb_hub_status *hubsts;
 	int ret;
 
+#ifndef CONFIG_DM_USB
 	/* "allocate" Hub device */
 	hub = usb_hub_allocate();
 	if (hub == NULL)
 		return -ENOMEM;
+#else
+	hub = dev_get_uclass_priv(dev->dev);
+#endif
 	hub->pusb_dev = dev;
 	/* Get the the hub descriptor */
 	ret = usb_get_hub_descriptor(dev, buffer, 4);
@@ -795,6 +802,7 @@ UCLASS_DRIVER(usb_hub) = {
 	.child_pre_probe	= usb_child_pre_probe,
 	.per_child_auto_alloc_size = sizeof(struct usb_device),
 	.per_child_platdata_auto_alloc_size = sizeof(struct usb_dev_platdata),
+	.per_device_auto_alloc_size = sizeof(struct usb_hub_device),
 };
 
 static const struct usb_device_id hub_id_table[] = {
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 110ddc9..9bb8477 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -177,7 +177,6 @@ int usb_stop(void)
 #ifdef CONFIG_USB_STORAGE
 	usb_stor_reset();
 #endif
-	usb_hub_reset();
 	uc_priv->companion_device_count = 0;
 	usb_started = 0;
 
@@ -230,7 +229,6 @@ int usb_init(void)
 	int ret;
 
 	asynch_allowed = 1;
-	usb_hub_reset();
 
 	ret = uclass_get(UCLASS_USB, &uc);
 	if (ret)
-- 
2.9.2

[U-Boot] [PATCH 04/16] usb: hub: Use 'struct usb_hub_device' as hub device's uclass_priv

[Marek Vasut]

On 06/23/2017 11:54 AM, Bin Meng wrote:
> Use USB hub device's dev->uclass_priv to point to 'usb_hub_device'
> so that with driver model usb_hub_reset() and usb_hub_allocate()
> are no longer needed.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Can you trim down the ifdeffery somehow or keep it more contained ?
I don't like having the code polluted by ifdefs all over the place,
so maybe factor out the specialties into function and put ifdef around
that or somesuch.

> ---
> 
>  common/usb_hub.c              | 10 +++++++++-
>  drivers/usb/host/usb-uclass.c |  2 --
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 090966b..18bd827 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -58,9 +58,10 @@ struct usb_device_scan {
>  	struct list_head list;
>  };
>  
> -/* TODO(sjg at chromium.org): Remove this when CONFIG_DM_USB is defined */
> +#ifndef CONFIG_DM_USB
>  static struct usb_hub_device hub_dev[USB_MAX_HUB];
>  static int usb_hub_index;
> +#endif
>  static LIST_HEAD(usb_scan_list);
>  
>  __weak void usb_hub_reset_devices(int port)
> @@ -167,6 +168,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>  	      max(100, (int)pgood_delay) + 1000);
>  }
>  
> +#ifndef CONFIG_DM_USB
>  void usb_hub_reset(void)
>  {
>  	usb_hub_index = 0;
> @@ -183,6 +185,7 @@ static struct usb_hub_device *usb_hub_allocate(void)
>  	printf("ERROR: USB_MAX_HUB (%d) reached\n", USB_MAX_HUB);
>  	return NULL;
>  }
> +#endif
>  
>  #define MAX_TRIES 5
>  
> @@ -557,10 +560,14 @@ static int usb_hub_configure(struct usb_device *dev)
>  	__maybe_unused struct usb_hub_status *hubsts;
>  	int ret;
>  
> +#ifndef CONFIG_DM_USB
>  	/* "allocate" Hub device */
>  	hub = usb_hub_allocate();
>  	if (hub == NULL)
>  		return -ENOMEM;
> +#else
> +	hub = dev_get_uclass_priv(dev->dev);
> +#endif
>  	hub->pusb_dev = dev;
>  	/* Get the the hub descriptor */
>  	ret = usb_get_hub_descriptor(dev, buffer, 4);
> @@ -795,6 +802,7 @@ UCLASS_DRIVER(usb_hub) = {
>  	.child_pre_probe	= usb_child_pre_probe,
>  	.per_child_auto_alloc_size = sizeof(struct usb_device),
>  	.per_child_platdata_auto_alloc_size = sizeof(struct usb_dev_platdata),
> +	.per_device_auto_alloc_size = sizeof(struct usb_hub_device),
>  };
>  
>  static const struct usb_device_id hub_id_table[] = {
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index 110ddc9..9bb8477 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -177,7 +177,6 @@ int usb_stop(void)
>  #ifdef CONFIG_USB_STORAGE
>  	usb_stor_reset();
>  #endif
> -	usb_hub_reset();
>  	uc_priv->companion_device_count = 0;
>  	usb_started = 0;
>  
> @@ -230,7 +229,6 @@ int usb_init(void)
>  	int ret;
>  
>  	asynch_allowed = 1;
> -	usb_hub_reset();
>  
>  	ret = uclass_get(UCLASS_USB, &uc);
>  	if (ret)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 04/16] usb: hub: Use 'struct usb_hub_device' as hub device's uclass_priv

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> Use USB hub device's dev->uclass_priv to point to 'usb_hub_device'
> so that with driver model usb_hub_reset() and usb_hub_allocate()
> are no longer needed.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  common/usb_hub.c              | 10 +++++++++-
>  drivers/usb/host/usb-uclass.c |  2 --
>  2 files changed, 9 insertions(+), 3 deletions(-)

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

[U-Boot] [PATCH 05/16] usb: hub: Add a new API to test if a hub device is root hub

[Bin Meng]

Sometimes we need know if a given hub device is root hub or not.
Add a new API to test this.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_hub.c | 10 ++++++++++
 include/usb.h    |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 18bd827..d780251 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -74,6 +74,16 @@ static inline bool usb_hub_is_superspeed(struct usb_device *hdev)
 	return hdev->descriptor.bDeviceProtocol == 3;
 }
 
+#ifdef CONFIG_DM_USB
+bool usb_hub_is_root_hub(struct udevice *hub)
+{
+	if (device_get_uclass_id(hub->parent) != UCLASS_USB_HUB)
+		return true;
+
+	return false;
+}
+#endif
+
 static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
 {
 	unsigned short dtype = USB_DT_HUB;
diff --git a/include/usb.h b/include/usb.h
index eb82cc2..c504d71 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -776,6 +776,14 @@ int usb_setup_device(struct usb_device *dev, bool do_read,
 		     struct usb_device *parent);
 
 /**
+ * usb_hub_is_root_hub() - Test whether a hub device is root hub or not
+ *
+ * @hub:	USB hub device to test
+ * @return:	true if the hub device is root hub, false otherwise.
+ */
+bool usb_hub_is_root_hub(struct udevice *hub);
+
+/**
  * usb_hub_scan() - Scan a hub and find its devices
  *
  * @hub:	Hub device to scan
-- 
2.9.2

[U-Boot] [PATCH 05/16] usb: hub: Add a new API to test if a hub device is root hub

[Marek Vasut]

On 06/23/2017 11:54 AM, Bin Meng wrote:
> Sometimes we need know if a given hub device is root hub or not.
> Add a new API to test this.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  common/usb_hub.c | 10 ++++++++++
>  include/usb.h    |  8 ++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 18bd827..d780251 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -74,6 +74,16 @@ static inline bool usb_hub_is_superspeed(struct usb_device *hdev)
>  	return hdev->descriptor.bDeviceProtocol == 3;
>  }
>  
> +#ifdef CONFIG_DM_USB
> +bool usb_hub_is_root_hub(struct udevice *hub)
> +{
> +	if (device_get_uclass_id(hub->parent) != UCLASS_USB_HUB)

Can this call fail ?

> +		return true;
> +
> +	return false;
> +}
> +#endif
> +
>  static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
>  {
>  	unsigned short dtype = USB_DT_HUB;
> diff --git a/include/usb.h b/include/usb.h
> index eb82cc2..c504d71 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -776,6 +776,14 @@ int usb_setup_device(struct usb_device *dev, bool do_read,
>  		     struct usb_device *parent);
>  
>  /**
> + * usb_hub_is_root_hub() - Test whether a hub device is root hub or not
> + *
> + * @hub:	USB hub device to test
> + * @return:	true if the hub device is root hub, false otherwise.
> + */
> +bool usb_hub_is_root_hub(struct udevice *hub);
> +
> +/**
>   * usb_hub_scan() - Scan a hub and find its devices
>   *
>   * @hub:	Hub device to scan
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 05/16] usb: hub: Add a new API to test if a hub device is root hub

[Bin Meng]

Hi Marek,

On Sat, Jun 24, 2017 at 1:55 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/23/2017 11:54 AM, Bin Meng wrote:
>> Sometimes we need know if a given hub device is root hub or not.
>> Add a new API to test this.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  common/usb_hub.c | 10 ++++++++++
>>  include/usb.h    |  8 ++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index 18bd827..d780251 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -74,6 +74,16 @@ static inline bool usb_hub_is_superspeed(struct usb_device *hdev)
>>       return hdev->descriptor.bDeviceProtocol == 3;
>>  }
>>
>> +#ifdef CONFIG_DM_USB
>> +bool usb_hub_is_root_hub(struct udevice *hub)
>> +{
>> +     if (device_get_uclass_id(hub->parent) != UCLASS_USB_HUB)
>
> Can this call fail ?

No,

Regards,
Bin

[U-Boot] [PATCH 05/16] usb: hub: Add a new API to test if a hub device is root hub

[Marek Vasut]

On 06/23/2017 11:54 AM, Bin Meng wrote:
> Sometimes we need know if a given hub device is root hub or not.
> Add a new API to test this.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  common/usb_hub.c | 10 ++++++++++
>  include/usb.h    |  8 ++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 18bd827..d780251 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -74,6 +74,16 @@ static inline bool usb_hub_is_superspeed(struct usb_device *hdev)
>  	return hdev->descriptor.bDeviceProtocol == 3;
>  }
>  
> +#ifdef CONFIG_DM_USB
> +bool usb_hub_is_root_hub(struct udevice *hub)

Actually , this is the is_root_hub() from the 6/16 , right , not a new
API. If you want to factor out stuff , just do that , but also remove
the is_root_hub() and do the conversion in the same patch.

> +{
> +	if (device_get_uclass_id(hub->parent) != UCLASS_USB_HUB)
> +		return true;
> +
> +	return false;
> +}
> +#endif
> +
>  static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
>  {
>  	unsigned short dtype = USB_DT_HUB;
> diff --git a/include/usb.h b/include/usb.h
> index eb82cc2..c504d71 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -776,6 +776,14 @@ int usb_setup_device(struct usb_device *dev, bool do_read,
>  		     struct usb_device *parent);
>  
>  /**
> + * usb_hub_is_root_hub() - Test whether a hub device is root hub or not
> + *
> + * @hub:	USB hub device to test
> + * @return:	true if the hub device is root hub, false otherwise.
> + */
> +bool usb_hub_is_root_hub(struct udevice *hub);
> +
> +/**
>   * usb_hub_scan() - Scan a hub and find its devices
>   *
>   * @hub:	Hub device to scan
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 05/16] usb: hub: Add a new API to test if a hub device is root hub

[Bin Meng]

Hi Marek,

On Sat, Jun 24, 2017 at 1:57 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/23/2017 11:54 AM, Bin Meng wrote:
>> Sometimes we need know if a given hub device is root hub or not.
>> Add a new API to test this.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  common/usb_hub.c | 10 ++++++++++
>>  include/usb.h    |  8 ++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index 18bd827..d780251 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -74,6 +74,16 @@ static inline bool usb_hub_is_superspeed(struct usb_device *hdev)
>>       return hdev->descriptor.bDeviceProtocol == 3;
>>  }
>>
>> +#ifdef CONFIG_DM_USB
>> +bool usb_hub_is_root_hub(struct udevice *hub)
>
> Actually , this is the is_root_hub() from the 6/16 , right , not a new
> API. If you want to factor out stuff , just do that , but also remove
> the is_root_hub() and do the conversion in the same patch.
>

Correct, is_root_hub() is static within xhci.c and only used by part
of the xHCI driver. To other USB codes, this is a new API. The two
patches (5/16, 6/16) are still self-contained, as each is against a
single module. But if you would like to do the two in one patch, let
me know and I will do in v2.

[snip]

Regards,
Bin

[U-Boot] [PATCH 05/16] usb: hub: Add a new API to test if a hub device is root hub

[Marek Vasut]

On 06/24/2017 03:41 AM, Bin Meng wrote:
> Hi Marek,
> 
> On Sat, Jun 24, 2017 at 1:57 AM, Marek Vasut <marex@denx.de> wrote:
>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>> Sometimes we need know if a given hub device is root hub or not.
>>> Add a new API to test this.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  common/usb_hub.c | 10 ++++++++++
>>>  include/usb.h    |  8 ++++++++
>>>  2 files changed, 18 insertions(+)
>>>
>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> index 18bd827..d780251 100644
>>> --- a/common/usb_hub.c
>>> +++ b/common/usb_hub.c
>>> @@ -74,6 +74,16 @@ static inline bool usb_hub_is_superspeed(struct usb_device *hdev)
>>>       return hdev->descriptor.bDeviceProtocol == 3;
>>>  }
>>>
>>> +#ifdef CONFIG_DM_USB
>>> +bool usb_hub_is_root_hub(struct udevice *hub)
>>
>> Actually , this is the is_root_hub() from the 6/16 , right , not a new
>> API. If you want to factor out stuff , just do that , but also remove
>> the is_root_hub() and do the conversion in the same patch.
>>
> 
> Correct, is_root_hub() is static within xhci.c and only used by part
> of the xHCI driver. To other USB codes, this is a new API. The two
> patches (5/16, 6/16) are still self-contained, as each is against a
> single module. But if you would like to do the two in one patch, let
> me know and I will do in v2.

I'd like a patch which pulls this out of xhci driver, yes.

> [snip]
> 
> Regards,
> Bin
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 06/16] usb: xhci: Change to use usb_hub_is_root_hub() API

[Bin Meng]

Now that we have a generic public API, remove the xHCI driver's own
version is_root_hub() and use the new API.

While we are here, remove the unused/commented out get_usb_device().

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8631e27..a5b888a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1116,26 +1116,6 @@ int usb_lowlevel_stop(int index)
 #endif /* CONFIG_DM_USB */
 
 #ifdef CONFIG_DM_USB
-/*
-static struct usb_device *get_usb_device(struct udevice *dev)
-{
-	struct usb_device *udev;
-
-	if (device_get_uclass_id(dev) == UCLASS_USB)
-		udev = dev_get_uclass_priv(dev);
-	else
-		udev = dev_get_parent_priv(dev);
-
-	return udev;
-}
-*/
-static bool is_root_hub(struct udevice *dev)
-{
-	if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB)
-		return true;
-
-	return false;
-}
 
 static int xhci_submit_control_msg(struct udevice *dev, struct usb_device *udev,
 				   unsigned long pipe, void *buffer, int length,
@@ -1150,10 +1130,10 @@ static int xhci_submit_control_msg(struct udevice *dev, struct usb_device *udev,
 	hub = udev->dev;
 	if (device_get_uclass_id(hub) == UCLASS_USB_HUB) {
 		/* Figure out our port number on the root hub */
-		if (is_root_hub(hub)) {
+		if (usb_hub_is_root_hub(hub)) {
 			root_portnr = udev->portnr;
 		} else {
-			while (!is_root_hub(hub->parent))
+			while (!usb_hub_is_root_hub(hub->parent))
 				hub = hub->parent;
 			uhop = dev_get_parent_priv(hub);
 			root_portnr = uhop->portnr;
-- 
2.9.2

[U-Boot] [PATCH 06/16] usb: xhci: Change to use usb_hub_is_root_hub() API

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> Now that we have a generic public API, remove the xHCI driver's own
> version is_root_hub() and use the new API.
>
> While we are here, remove the unused/commented out get_usb_device().
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/usb/host/xhci.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)

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

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Bin Meng]

USB 3.0 hub port status field has different bit positions from 2.0
hubs. Since U-Boot only understands the old version, translate the
new one into the old one.

Since we are going to add USB 3.0 hub support, this feature is only
available with driver model USB.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_hub.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index d780251..835fac9 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -119,9 +119,40 @@ static int usb_get_hub_status(struct usb_device *dev, void *data)
 
 int usb_get_port_status(struct usb_device *dev, int port, void *data)
 {
-	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+	int ret;
+
+	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
 			USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, port,
 			data, sizeof(struct usb_port_status), USB_CNTL_TIMEOUT);
+
+#ifdef CONFIG_DM_USB
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Translate the USB 3.0 hub port status field into the old version
+	 * that U-Boot understands. Do this only when the hub is not root hub.
+	 * For root hub, the port status field has already been translated
+	 * in the host controller driver (see xhci_submit_root() in xhci.c).
+	 *
+	 * Note: this only supports driver model.
+	 */
+
+	if (!usb_hub_is_root_hub(dev->dev) && usb_hub_is_superspeed(dev)) {
+		struct usb_port_status *status = (struct usb_port_status *)data;
+		u16 tmp = (status->wPortStatus) & USB_SS_PORT_STAT_MASK;
+
+		if (status->wPortStatus & USB_SS_PORT_STAT_POWER)
+			tmp |= USB_PORT_STAT_POWER;
+		if ((status->wPortStatus & USB_SS_PORT_STAT_SPEED) ==
+		    USB_SS_PORT_STAT_SPEED_5GBPS)
+			tmp |= USB_PORT_STAT_SUPER_SPEED;
+
+		status->wPortStatus = tmp;
+	}
+#endif
+
+	return ret;
 }
 
 
-- 
2.9.2

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Marek Vasut]

On 06/23/2017 11:54 AM, Bin Meng wrote:
> USB 3.0 hub port status field has different bit positions from 2.0
> hubs. Since U-Boot only understands the old version, translate the
> new one into the old one.

This is quite hairy. I'd rather see some protocol version agnostic flag
field rather than patching the wPortStatus and co.

> Since we are going to add USB 3.0 hub support, this feature is only
> available with driver model USB.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  common/usb_hub.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index d780251..835fac9 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -119,9 +119,40 @@ static int usb_get_hub_status(struct usb_device *dev, void *data)
>  
>  int usb_get_port_status(struct usb_device *dev, int port, void *data)
>  {
> -	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
> +	int ret;
> +
> +	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
>  			USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, port,
>  			data, sizeof(struct usb_port_status), USB_CNTL_TIMEOUT);
> +
> +#ifdef CONFIG_DM_USB
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Translate the USB 3.0 hub port status field into the old version
> +	 * that U-Boot understands. Do this only when the hub is not root hub.
> +	 * For root hub, the port status field has already been translated
> +	 * in the host controller driver (see xhci_submit_root() in xhci.c).
> +	 *
> +	 * Note: this only supports driver model.
> +	 */
> +
> +	if (!usb_hub_is_root_hub(dev->dev) && usb_hub_is_superspeed(dev)) {
> +		struct usb_port_status *status = (struct usb_port_status *)data;
> +		u16 tmp = (status->wPortStatus) & USB_SS_PORT_STAT_MASK;
> +
> +		if (status->wPortStatus & USB_SS_PORT_STAT_POWER)
> +			tmp |= USB_PORT_STAT_POWER;
> +		if ((status->wPortStatus & USB_SS_PORT_STAT_SPEED) ==
> +		    USB_SS_PORT_STAT_SPEED_5GBPS)
> +			tmp |= USB_PORT_STAT_SUPER_SPEED;
> +
> +		status->wPortStatus = tmp;
> +	}
> +#endif
> +
> +	return ret;
>  }
>  
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Bin Meng]

Hi Marek,

On Sat, Jun 24, 2017 at 1:59 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/23/2017 11:54 AM, Bin Meng wrote:
>> USB 3.0 hub port status field has different bit positions from 2.0
>> hubs. Since U-Boot only understands the old version, translate the
>> new one into the old one.
>
> This is quite hairy. I'd rather see some protocol version agnostic flag
> field rather than patching the wPortStatus and co.
>

I am not sure how do do that in a clean way. If we return the raw 3.0
hub port status data, that means we need change lot of hub codes here
and there to do different parsing.

>> Since we are going to add USB 3.0 hub support, this feature is only
>> available with driver model USB.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  common/usb_hub.c | 33 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>

[snip]

Regards,
Bin

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Marek Vasut]

On 06/24/2017 03:53 AM, Bin Meng wrote:
> Hi Marek,
> 
> On Sat, Jun 24, 2017 at 1:59 AM, Marek Vasut <marex@denx.de> wrote:
>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>> USB 3.0 hub port status field has different bit positions from 2.0
>>> hubs. Since U-Boot only understands the old version, translate the
>>> new one into the old one.
>>
>> This is quite hairy. I'd rather see some protocol version agnostic flag
>> field rather than patching the wPortStatus and co.
>>
> 
> I am not sure how do do that in a clean way. If we return the raw 3.0
> hub port status data, that means we need change lot of hub codes here
> and there to do different parsing.

How does Linux handle this?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Bin Meng]

Hi Marek,

On Tue, Jun 27, 2017 at 2:06 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/24/2017 03:53 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Sat, Jun 24, 2017 at 1:59 AM, Marek Vasut <marex@denx.de> wrote:
>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>> USB 3.0 hub port status field has different bit positions from 2.0
>>>> hubs. Since U-Boot only understands the old version, translate the
>>>> new one into the old one.
>>>
>>> This is quite hairy. I'd rather see some protocol version agnostic flag
>>> field rather than patching the wPortStatus and co.
>>>
>>
>> I am not sure how do do that in a clean way. If we return the raw 3.0
>> hub port status data, that means we need change lot of hub codes here
>> and there to do different parsing.
>
> How does Linux handle this?

Looks Linux is doing different parsing depending on hub is 3.0 or 2.0,
at least for the power bit that I was just looking at. Do you want to
do that?

Regards,
Bin

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Bin Meng]

Hi Marek,

On Tue, Jun 27, 2017 at 7:57 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Marek,
>
> On Tue, Jun 27, 2017 at 2:06 AM, Marek Vasut <marex@denx.de> wrote:
>> On 06/24/2017 03:53 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Sat, Jun 24, 2017 at 1:59 AM, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>> USB 3.0 hub port status field has different bit positions from 2.0
>>>>> hubs. Since U-Boot only understands the old version, translate the
>>>>> new one into the old one.
>>>>
>>>> This is quite hairy. I'd rather see some protocol version agnostic flag
>>>> field rather than patching the wPortStatus and co.
>>>>
>>>
>>> I am not sure how do do that in a clean way. If we return the raw 3.0
>>> hub port status data, that means we need change lot of hub codes here
>>> and there to do different parsing.
>>
>> How does Linux handle this?
>
> Looks Linux is doing different parsing depending on hub is 3.0 or 2.0,
> at least for the power bit that I was just looking at. Do you want to
> do that?

OK, I will do something similar like Linux in v2.

Regards,
Bin

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Marek Vasut]

On 06/28/2017 10:28 AM, Bin Meng wrote:
> Hi Marek,
> 
> On Tue, Jun 27, 2017 at 7:57 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Marek,
>>
>> On Tue, Jun 27, 2017 at 2:06 AM, Marek Vasut <marex@denx.de> wrote:
>>> On 06/24/2017 03:53 AM, Bin Meng wrote:
>>>> Hi Marek,
>>>>
>>>> On Sat, Jun 24, 2017 at 1:59 AM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>> USB 3.0 hub port status field has different bit positions from 2.0
>>>>>> hubs. Since U-Boot only understands the old version, translate the
>>>>>> new one into the old one.
>>>>>
>>>>> This is quite hairy. I'd rather see some protocol version agnostic flag
>>>>> field rather than patching the wPortStatus and co.
>>>>>
>>>>
>>>> I am not sure how do do that in a clean way. If we return the raw 3.0
>>>> hub port status data, that means we need change lot of hub codes here
>>>> and there to do different parsing.
>>>
>>> How does Linux handle this?
>>
>> Looks Linux is doing different parsing depending on hub is 3.0 or 2.0,
>> at least for the power bit that I was just looking at. Do you want to
>> do that?
> 
> OK, I will do something similar like Linux in v2.

Thanks

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Bin Meng]

Hi Marek,

On Thu, Jun 29, 2017 at 1:25 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/28/2017 10:28 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Tue, Jun 27, 2017 at 7:57 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Marek,
>>>
>>> On Tue, Jun 27, 2017 at 2:06 AM, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/24/2017 03:53 AM, Bin Meng wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On Sat, Jun 24, 2017 at 1:59 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>> USB 3.0 hub port status field has different bit positions from 2.0
>>>>>>> hubs. Since U-Boot only understands the old version, translate the
>>>>>>> new one into the old one.
>>>>>>
>>>>>> This is quite hairy. I'd rather see some protocol version agnostic flag
>>>>>> field rather than patching the wPortStatus and co.
>>>>>>
>>>>>
>>>>> I am not sure how do do that in a clean way. If we return the raw 3.0
>>>>> hub port status data, that means we need change lot of hub codes here
>>>>> and there to do different parsing.
>>>>
>>>> How does Linux handle this?
>>>
>>> Looks Linux is doing different parsing depending on hub is 3.0 or 2.0,
>>> at least for the power bit that I was just looking at. Do you want to
>>> do that?
>>
>> OK, I will do something similar like Linux in v2.
>
> Thanks

After I checked Linux codes in depth, it looks current way of handling
3.0 port status translation in this patch is the minimum way to fix
the 3.0 hub port status problem in U-Boot. What Linux does is, one
xHCI controller registers two buses, that one is USB 3.0 and the other
is USB 2.0. All 3.0 devices will only show on the 3.0 bus, while all
1.0/2.0 devices will show on the 2.0 bus. We can certainly do similar
thing in U-Boot, but I am afraid that will lead to a overhaul to
current USB stack and xHCI driver. And most important, that support
will be only under DM, which is something you don't like to see.

Regards,
Bin

[U-Boot] [PATCH 07/16] usb: hub: Translate USB 3.0 hub port status into old version

[Simon Glass]

On 5 July 2017 at 23:56, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Marek,
>
> On Thu, Jun 29, 2017 at 1:25 AM, Marek Vasut <marex@denx.de> wrote:
> > On 06/28/2017 10:28 AM, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Tue, Jun 27, 2017 at 7:57 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> >>> Hi Marek,
> >>>
> >>> On Tue, Jun 27, 2017 at 2:06 AM, Marek Vasut <marex@denx.de> wrote:
> >>>> On 06/24/2017 03:53 AM, Bin Meng wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On Sat, Jun 24, 2017 at 1:59 AM, Marek Vasut <marex@denx.de> wrote:
> >>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
> >>>>>>> USB 3.0 hub port status field has different bit positions from 2.0
> >>>>>>> hubs. Since U-Boot only understands the old version, translate the
> >>>>>>> new one into the old one.
> >>>>>>
> >>>>>> This is quite hairy. I'd rather see some protocol version agnostic flag
> >>>>>> field rather than patching the wPortStatus and co.
> >>>>>>
> >>>>>
> >>>>> I am not sure how do do that in a clean way. If we return the raw 3.0
> >>>>> hub port status data, that means we need change lot of hub codes here
> >>>>> and there to do different parsing.
> >>>>
> >>>> How does Linux handle this?
> >>>
> >>> Looks Linux is doing different parsing depending on hub is 3.0 or 2.0,
> >>> at least for the power bit that I was just looking at. Do you want to
> >>> do that?
> >>
> >> OK, I will do something similar like Linux in v2.
> >
> > Thanks
>
> After I checked Linux codes in depth, it looks current way of handling
> 3.0 port status translation in this patch is the minimum way to fix
> the 3.0 hub port status problem in U-Boot. What Linux does is, one
> xHCI controller registers two buses, that one is USB 3.0 and the other
> is USB 2.0. All 3.0 devices will only show on the 3.0 bus, while all
> 1.0/2.0 devices will show on the 2.0 bus. We can certainly do similar
> thing in U-Boot, but I am afraid that will lead to a overhaul to
> current USB stack and xHCI driver. And most important, that support
> will be only under DM, which is something you don't like to see.

IMO we should not be adding features to non-DM USB.

Anyway:

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

[U-Boot] [PATCH 08/16] usb: hub: Support 'set hub depth' request for USB 3.0 hubs

[Bin Meng]

USB 3.0 hub uses a hub depth value multiplied by four as an offset
into the 'route string' to locate the bits it uses to determine the
downstream port number. We shall set the hub depth value of a USB
3.0 hub after it is configured.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_hub.c   | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/usb.h      |  1 +
 include/usb_defs.h |  3 +++
 3 files changed, 56 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 835fac9..63358cd 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -82,6 +82,16 @@ bool usb_hub_is_root_hub(struct udevice *hub)
 
 	return false;
 }
+
+static int usb_set_hub_depth(struct usb_device *dev, int depth)
+{
+	if (depth < 0 || depth > 4)
+		return -EINVAL;
+
+	return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+		USB_REQ_SET_HUB_DEPTH, USB_DIR_OUT | USB_RT_HUB,
+		depth, 0, NULL, 0, USB_CNTL_TIMEOUT);
+}
 #endif
 
 static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
@@ -719,6 +729,48 @@ static int usb_hub_configure(struct usb_device *dev)
 	debug("%sover-current condition exists\n",
 	      (le16_to_cpu(hubsts->wHubStatus) & HUB_STATUS_OVERCURRENT) ? \
 	      "" : "no ");
+
+#ifdef CONFIG_DM_USB
+	/*
+	 * A maximum of seven tiers are allowed in a USB topology, and the
+	 * root hub occupies the first tier. The last tier ends with a normal
+	 * USB device. USB 3.0 hubs use a 20-bit field called 'route string'
+	 * to route packet to the designated downstream port. The hub uses a
+	 * hub depth value multiplied by four as an offset into the 'route
+	 * string' to locate the bits it uses to determine the downstream
+	 * port number.
+	 */
+	if (usb_hub_is_root_hub(dev->dev)) {
+		hub->hub_depth = -1;
+	} else {
+		struct udevice *hdev;
+		int depth = 0;
+
+		hdev = dev->dev->parent;
+		while (!usb_hub_is_root_hub(hdev)) {
+			depth++;
+			hdev = hdev->parent;
+		}
+
+		hub->hub_depth = depth;
+
+		if (usb_hub_is_superspeed(dev)) {
+			debug("set hub (%p) depth to %d\n", dev, depth);
+			/*
+			 * This request sets the value that the hub uses to
+			 * determine the index into the 'route string index'
+			 * for this hub.
+			 */
+			ret = usb_set_hub_depth(dev, depth);
+			if (ret < 0) {
+				debug("%s: failed to set hub depth (%lX)\n",
+				      __func__, dev->status);
+				return ret;
+			}
+		}
+	}
+#endif
+
 	usb_hub_power_on(hub);
 
 	/*
diff --git a/include/usb.h b/include/usb.h
index c504d71..40e44f4 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -570,6 +570,7 @@ struct usb_hub_device {
 	ulong connect_timeout;		/* Device connection timeout in ms */
 	ulong query_delay;		/* Device query delay in ms */
 	int overcurrent_count[USB_MAXCHILDREN];	/* Over-current counter */
+	int hub_depth;			/* USB 3.0 hub depth */
 };
 
 #ifdef CONFIG_DM_USB
diff --git a/include/usb_defs.h b/include/usb_defs.h
index 6b4385a..273337f 100644
--- a/include/usb_defs.h
+++ b/include/usb_defs.h
@@ -306,6 +306,9 @@
 /* Mask for wIndex in get/set port feature */
 #define USB_HUB_PORT_MASK	0xf
 
+/* Hub class request codes */
+#define USB_REQ_SET_HUB_DEPTH	0x0c
+
 /*
  * CBI style
  */
-- 
2.9.2

[U-Boot] [PATCH 08/16] usb: hub: Support 'set hub depth' request for USB 3.0 hubs

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> USB 3.0 hub uses a hub depth value multiplied by four as an offset
> into the 'route string' to locate the bits it uses to determine the
> downstream port number. We shall set the hub depth value of a USB
> 3.0 hub after it is configured.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  common/usb_hub.c   | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/usb.h      |  1 +
>  include/usb_defs.h |  3 +++
>  3 files changed, 56 insertions(+)

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

nit below

>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 835fac9..63358cd 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -82,6 +82,16 @@ bool usb_hub_is_root_hub(struct udevice *hub)
>
>         return false;
>  }
> +
> +static int usb_set_hub_depth(struct usb_device *dev, int depth)
> +{
> +       if (depth < 0 || depth > 4)
> +               return -EINVAL;
> +
> +       return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> +               USB_REQ_SET_HUB_DEPTH, USB_DIR_OUT | USB_RT_HUB,
> +               depth, 0, NULL, 0, USB_CNTL_TIMEOUT);
> +}
>  #endif
>
>  static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
> @@ -719,6 +729,48 @@ static int usb_hub_configure(struct usb_device *dev)
>         debug("%sover-current condition exists\n",
>               (le16_to_cpu(hubsts->wHubStatus) & HUB_STATUS_OVERCURRENT) ? \
>               "" : "no ");
> +
> +#ifdef CONFIG_DM_USB
> +       /*
> +        * A maximum of seven tiers are allowed in a USB topology, and the
> +        * root hub occupies the first tier. The last tier ends with a normal
> +        * USB device. USB 3.0 hubs use a 20-bit field called 'route string'
> +        * to route packet to the designated downstream port. The hub uses a

should this be 'packets'?

[..]

[U-Boot] [PATCH 09/16] usb: xhci: Change xhci_setup_addressable_virt_dev() signature

[Bin Meng]

For future extension, change xhci_setup_addressable_virt_dev()
signature to accept a pointer to 'struct usb_device', instead
of its members slot_id & speed, as the struct already contains
these two plus some other useful information of the device.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci-mem.c | 6 ++++--
 drivers/usb/host/xhci.c     | 3 +--
 drivers/usb/host/xhci.h     | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 12e277a..9aa3092 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -713,14 +713,16 @@ void xhci_slot_copy(struct xhci_ctrl *ctrl, struct xhci_container_ctx *in_ctx,
  * @param udev pointer to the Device Data Structure
  * @return returns negative value on failure else 0 on success
  */
-void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl, int slot_id,
-				     int speed, int hop_portnr)
+void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
+				     struct usb_device *udev, int hop_portnr)
 {
 	struct xhci_virt_device *virt_dev;
 	struct xhci_ep_ctx *ep0_ctx;
 	struct xhci_slot_ctx *slot_ctx;
 	u32 port_num = 0;
 	u64 trb_64 = 0;
+	int slot_id = udev->slot_id;
+	int speed = udev->speed;
 
 	virt_dev = ctrl->devs[slot_id];
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a5b888a..1148127 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -415,8 +415,7 @@ static int xhci_address_device(struct usb_device *udev, int root_portnr)
 	 * so setting up the slot context.
 	 */
 	debug("Setting up addressable devices %p\n", ctrl->dcbaa);
-	xhci_setup_addressable_virt_dev(ctrl, udev->slot_id, udev->speed,
-					root_portnr);
+	xhci_setup_addressable_virt_dev(ctrl, udev, root_portnr);
 
 	ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
 	ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG | EP0_FLAG);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index b9602ba..cdce67c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1247,8 +1247,8 @@ void xhci_endpoint_copy(struct xhci_ctrl *ctrl,
 void xhci_slot_copy(struct xhci_ctrl *ctrl,
 		    struct xhci_container_ctx *in_ctx,
 		    struct xhci_container_ctx *out_ctx);
-void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl, int slot_id,
-				     int speed, int hop_portnr);
+void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
+				     struct usb_device *udev, int hop_portnr);
 void xhci_queue_command(struct xhci_ctrl *ctrl, u8 *ptr,
 			u32 slot_id, u32 ep_index, trb_type cmd);
 void xhci_acknowledge_event(struct xhci_ctrl *ctrl);
-- 
2.9.2

[U-Boot] [PATCH 09/16] usb: xhci: Change xhci_setup_addressable_virt_dev() signature

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> For future extension, change xhci_setup_addressable_virt_dev()
> signature to accept a pointer to 'struct usb_device', instead
> of its members slot_id & speed, as the struct already contains
> these two plus some other useful information of the device.

I am guessing this is because you need the other info in a subsequent patch.

>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/usb/host/xhci-mem.c | 6 ++++--
>  drivers/usb/host/xhci.c     | 3 +--
>  drivers/usb/host/xhci.h     | 4 ++--
>  3 files changed, 7 insertions(+), 6 deletions(-)

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

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

xHCI spec says: the values of the 'route string' field shall be
initialized by the first 'Address Device' command issued to a
device slot, and shall not be modified by any other command.

So far U-Boot does not program this field, and it does not prevent
SS device directly attached to root port, or HS device behind an HS
hub, from working, due to the fact that 'route string' is used by
the xHC to target SS packets. But in order to enumerate devices
behind an SS hub, this field must be programmed.

With this commit and along with previous commits, now SS & HS devices
attached to a USB 3.0 hub can be enumerated by U-Boot.

As usual, this new feature is only available when DM is on.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---
Test logs: two USB 3.0 hubs (one tier2, one tier3)
=> usb tree
USB device tree:
  1  Hub (5 Gb/s, 0mA)
  |  U-Boot XHCI Host Controller
  |
  +-2  Hub (5 Gb/s, 0mA)
  | |  GenesysLogic USB3.0 Hub
  | |
  | +-4  Hub (5 Gb/s, 0mA)
  | | |  VIA Labs, Inc.          USB3.0 Hub
  | | |
  | | +-7  Mass Storage (5 Gb/s, 76mA)
  | |      JetFlash Mass Storage Device 16Q6ZPH20GF3E8UQ
  | |
  | +-8  Vendor specific (5 Gb/s, 36mA)
  |      Realtek USB 10/100/1000 LAN 00E04C680977
  |
  +-3  Hub (480 Mb/s, 100mA)
    |  GenesysLogic USB2.0 Hub
    |
    +-5  Mass Storage (480 Mb/s, 200mA)
    |    Netac OnlyDisk FF00ECB60800FFFF1526
    |
    +-6  Hub (480 Mb/s, 0mA)
         VIA Labs, Inc.          USB2.0 Hub

 drivers/usb/host/xhci-mem.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 9aa3092..03874db 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -723,6 +723,9 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
 	u64 trb_64 = 0;
 	int slot_id = udev->slot_id;
 	int speed = udev->speed;
+	int route = 0;
+	struct usb_device *dev = udev;
+	struct usb_hub_device *hub;
 
 	virt_dev = ctrl->devs[slot_id];
 
@@ -733,7 +736,22 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
 	slot_ctx = xhci_get_slot_ctx(ctrl, virt_dev->in_ctx);
 
 	/* Only the control endpoint is valid - one endpoint context */
-	slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(1) | 0);
+	slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(1));
+
+#ifdef CONFIG_DM_USB
+	/* Calculate the route string for this device */
+	port_num = dev->portnr;
+	while (!usb_hub_is_root_hub(dev->dev)) {
+		hub = dev_get_uclass_priv(dev->dev);
+		route |= port_num << (hub->hub_depth * 4);
+		dev = dev_get_parent_priv(dev->dev);
+		port_num = dev->portnr;
+		dev = dev_get_parent_priv(dev->dev->parent);
+	}
+
+	debug("hub (%p) route string %x\n", udev, route);
+#endif
+	slot_ctx->dev_info |= route;
 
 	switch (speed) {
 	case USB_SPEED_SUPER:
-- 
2.9.2

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Marek Vasut]

On 06/23/2017 11:54 AM, Bin Meng wrote:
> xHCI spec says: the values of the 'route string' field shall be
> initialized by the first 'Address Device' command issued to a
> device slot, and shall not be modified by any other command.
> 
> So far U-Boot does not program this field, and it does not prevent
> SS device directly attached to root port, or HS device behind an HS
> hub, from working, due to the fact that 'route string' is used by
> the xHC to target SS packets. But in order to enumerate devices
> behind an SS hub, this field must be programmed.
> 
> With this commit and along with previous commits, now SS & HS devices
> attached to a USB 3.0 hub can be enumerated by U-Boot.
> 
> As usual, this new feature is only available when DM is on.

Great, but I really dislike the ifdef pollution, so this needs to be
sorted out.

> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> 
> ---
> Test logs: two USB 3.0 hubs (one tier2, one tier3)
> => usb tree
> USB device tree:
>   1  Hub (5 Gb/s, 0mA)
>   |  U-Boot XHCI Host Controller
>   |
>   +-2  Hub (5 Gb/s, 0mA)
>   | |  GenesysLogic USB3.0 Hub
>   | |
>   | +-4  Hub (5 Gb/s, 0mA)
>   | | |  VIA Labs, Inc.          USB3.0 Hub
>   | | |
>   | | +-7  Mass Storage (5 Gb/s, 76mA)
>   | |      JetFlash Mass Storage Device 16Q6ZPH20GF3E8UQ
>   | |
>   | +-8  Vendor specific (5 Gb/s, 36mA)
>   |      Realtek USB 10/100/1000 LAN 00E04C680977
>   |
>   +-3  Hub (480 Mb/s, 100mA)
>     |  GenesysLogic USB2.0 Hub
>     |
>     +-5  Mass Storage (480 Mb/s, 200mA)
>     |    Netac OnlyDisk FF00ECB60800FFFF1526
>     |
>     +-6  Hub (480 Mb/s, 0mA)
>          VIA Labs, Inc.          USB2.0 Hub
> 
>  drivers/usb/host/xhci-mem.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 9aa3092..03874db 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -723,6 +723,9 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
>  	u64 trb_64 = 0;
>  	int slot_id = udev->slot_id;
>  	int speed = udev->speed;
> +	int route = 0;
> +	struct usb_device *dev = udev;
> +	struct usb_hub_device *hub;
>  
>  	virt_dev = ctrl->devs[slot_id];
>  
> @@ -733,7 +736,22 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
>  	slot_ctx = xhci_get_slot_ctx(ctrl, virt_dev->in_ctx);
>  
>  	/* Only the control endpoint is valid - one endpoint context */
> -	slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(1) | 0);
> +	slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(1));
> +
> +#ifdef CONFIG_DM_USB
> +	/* Calculate the route string for this device */
> +	port_num = dev->portnr;
> +	while (!usb_hub_is_root_hub(dev->dev)) {
> +		hub = dev_get_uclass_priv(dev->dev);
> +		route |= port_num << (hub->hub_depth * 4);
> +		dev = dev_get_parent_priv(dev->dev);
> +		port_num = dev->portnr;
> +		dev = dev_get_parent_priv(dev->dev->parent);
> +	}
> +
> +	debug("hub (%p) route string %x\n", udev, route);
> +#endif
> +	slot_ctx->dev_info |= route;
>  
>  	switch (speed) {
>  	case USB_SPEED_SUPER:
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

Hi Marek,

On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/23/2017 11:54 AM, Bin Meng wrote:
>> xHCI spec says: the values of the 'route string' field shall be
>> initialized by the first 'Address Device' command issued to a
>> device slot, and shall not be modified by any other command.
>>
>> So far U-Boot does not program this field, and it does not prevent
>> SS device directly attached to root port, or HS device behind an HS
>> hub, from working, due to the fact that 'route string' is used by
>> the xHC to target SS packets. But in order to enumerate devices
>> behind an SS hub, this field must be programmed.
>>
>> With this commit and along with previous commits, now SS & HS devices
>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>
>> As usual, this new feature is only available when DM is on.
>
> Great, but I really dislike the ifdef pollution, so this needs to be
> sorted out.

The ifdef was needed due to it calls DM APIs or access DM udevice. I
have no intention to add a new feature to the non-DM driver.
Eventually we can get rid of all these ifdefs when all boards are
converted to use DM USB.

>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>
>> ---
>> Test logs: two USB 3.0 hubs (one tier2, one tier3)
>> => usb tree
>> USB device tree:
>>   1  Hub (5 Gb/s, 0mA)
>>   |  U-Boot XHCI Host Controller
>>   |
>>   +-2  Hub (5 Gb/s, 0mA)
>>   | |  GenesysLogic USB3.0 Hub
>>   | |
>>   | +-4  Hub (5 Gb/s, 0mA)
>>   | | |  VIA Labs, Inc.          USB3.0 Hub
>>   | | |
>>   | | +-7  Mass Storage (5 Gb/s, 76mA)
>>   | |      JetFlash Mass Storage Device 16Q6ZPH20GF3E8UQ
>>   | |
>>   | +-8  Vendor specific (5 Gb/s, 36mA)
>>   |      Realtek USB 10/100/1000 LAN 00E04C680977
>>   |
>>   +-3  Hub (480 Mb/s, 100mA)
>>     |  GenesysLogic USB2.0 Hub
>>     |
>>     +-5  Mass Storage (480 Mb/s, 200mA)
>>     |    Netac OnlyDisk FF00ECB60800FFFF1526
>>     |
>>     +-6  Hub (480 Mb/s, 0mA)
>>          VIA Labs, Inc.          USB2.0 Hub
>>

[snip]

Regards,
Bin

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Marek Vasut]

On 06/24/2017 03:57 AM, Bin Meng wrote:
> Hi Marek,
> 
> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>> xHCI spec says: the values of the 'route string' field shall be
>>> initialized by the first 'Address Device' command issued to a
>>> device slot, and shall not be modified by any other command.
>>>
>>> So far U-Boot does not program this field, and it does not prevent
>>> SS device directly attached to root port, or HS device behind an HS
>>> hub, from working, due to the fact that 'route string' is used by
>>> the xHC to target SS packets. But in order to enumerate devices
>>> behind an SS hub, this field must be programmed.
>>>
>>> With this commit and along with previous commits, now SS & HS devices
>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>
>>> As usual, this new feature is only available when DM is on.
>>
>> Great, but I really dislike the ifdef pollution, so this needs to be
>> sorted out.
> 
> The ifdef was needed due to it calls DM APIs or access DM udevice. I
> have no intention to add a new feature to the non-DM driver.

But then this creates a massive disparity, it's like we're growing two
USB stacks.

> Eventually we can get rid of all these ifdefs when all boards are
> converted to use DM USB.
> 
>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>
>>> ---
>>> Test logs: two USB 3.0 hubs (one tier2, one tier3)
>>> => usb tree
>>> USB device tree:
>>>   1  Hub (5 Gb/s, 0mA)
>>>   |  U-Boot XHCI Host Controller
>>>   |
>>>   +-2  Hub (5 Gb/s, 0mA)
>>>   | |  GenesysLogic USB3.0 Hub
>>>   | |
>>>   | +-4  Hub (5 Gb/s, 0mA)
>>>   | | |  VIA Labs, Inc.          USB3.0 Hub
>>>   | | |
>>>   | | +-7  Mass Storage (5 Gb/s, 76mA)
>>>   | |      JetFlash Mass Storage Device 16Q6ZPH20GF3E8UQ
>>>   | |
>>>   | +-8  Vendor specific (5 Gb/s, 36mA)
>>>   |      Realtek USB 10/100/1000 LAN 00E04C680977
>>>   |
>>>   +-3  Hub (480 Mb/s, 100mA)
>>>     |  GenesysLogic USB2.0 Hub
>>>     |
>>>     +-5  Mass Storage (480 Mb/s, 200mA)
>>>     |    Netac OnlyDisk FF00ECB60800FFFF1526
>>>     |
>>>     +-6  Hub (480 Mb/s, 0mA)
>>>          VIA Labs, Inc.          USB2.0 Hub
>>>
> 
> [snip]
> 
> Regards,
> Bin
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

Hi Marek,

On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/24/2017 03:57 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>> xHCI spec says: the values of the 'route string' field shall be
>>>> initialized by the first 'Address Device' command issued to a
>>>> device slot, and shall not be modified by any other command.
>>>>
>>>> So far U-Boot does not program this field, and it does not prevent
>>>> SS device directly attached to root port, or HS device behind an HS
>>>> hub, from working, due to the fact that 'route string' is used by
>>>> the xHC to target SS packets. But in order to enumerate devices
>>>> behind an SS hub, this field must be programmed.
>>>>
>>>> With this commit and along with previous commits, now SS & HS devices
>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>
>>>> As usual, this new feature is only available when DM is on.
>>>
>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>> sorted out.
>>
>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>> have no intention to add a new feature to the non-DM driver.
>
> But then this creates a massive disparity, it's like we're growing two
> USB stacks.
>

Yep, unfortunately. But if we continue adding new features/fixes to
the old non-DM stuff, I am not sure how this can encourage people to
switch to DM. Maybe I can check all boards that use xHCI to see if
they are switched to DM?

[snip]

Regards,
Bin

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Stefan Roese]

Hi Bin,

On 27.06.2017 02:01, Bin Meng wrote:
> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>> initialized by the first 'Address Device' command issued to a
>>>>> device slot, and shall not be modified by any other command.
>>>>>
>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>> behind an SS hub, this field must be programmed.
>>>>>
>>>>> With this commit and along with previous commits, now SS & HS devices
>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>
>>>>> As usual, this new feature is only available when DM is on.
>>>>
>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>> sorted out.
>>>
>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>> have no intention to add a new feature to the non-DM driver.
>>
>> But then this creates a massive disparity, it's like we're growing two
>> USB stacks.
>>
> 
> Yep, unfortunately. But if we continue adding new features/fixes to
> the old non-DM stuff, I am not sure how this can encourage people to
> switch to DM.

Correct. We definitely don't want to add new features to non-DM
drivers / IF, if this is non-trivial.

> Maybe I can check all boards that use xHCI to see if
> they are switched to DM?

xHCI is still quite new in U-Boot, so I would expect that all
platforms using it, are using DM or at least not far away from using
it. Yes, please check all xHCI "users", if they use DM. Then we
know if and which users need some "persuasion" to switch over to
DM soon. ;)

Thanks,
Stefan

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

Hi Stefan,

On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
>
> On 27.06.2017 02:01, Bin Meng wrote:
>>
>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>
>>>> Hi Marek,
>>>>
>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>
>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>> device slot, and shall not be modified by any other command.
>>>>>>
>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>> behind an SS hub, this field must be programmed.
>>>>>>
>>>>>> With this commit and along with previous commits, now SS & HS devices
>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>
>>>>>> As usual, this new feature is only available when DM is on.
>>>>>
>>>>>
>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>> sorted out.
>>>>
>>>>
>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>> have no intention to add a new feature to the non-DM driver.
>>>
>>>
>>> But then this creates a massive disparity, it's like we're growing two
>>> USB stacks.
>>>
>>
>> Yep, unfortunately. But if we continue adding new features/fixes to
>> the old non-DM stuff, I am not sure how this can encourage people to
>> switch to DM.
>
>
> Correct. We definitely don't want to add new features to non-DM
> drivers / IF, if this is non-trivial.
>
>> Maybe I can check all boards that use xHCI to see if
>> they are switched to DM?
>
>
> xHCI is still quite new in U-Boot, so I would expect that all
> platforms using it, are using DM or at least not far away from using
> it. Yes, please check all xHCI "users", if they use DM. Then we
> know if and which users need some "persuasion" to switch over to
> DM soon. ;)

I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
CONFIG_DM_USB. Here is the list.

configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y

So it looks we have lots of conversion work to be done by many board
maintainers. I am not sure how to proceed on this.

Regards,
Bin

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Marek Vasut]

On 06/27/2017 10:27 AM, Bin Meng wrote:
> Hi Stefan,
> 
> On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Bin,
>>
>>
>> On 27.06.2017 02:01, Bin Meng wrote:
>>>
>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>>
>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>
>>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>
>>>>>>> With this commit and along with previous commits, now SS & HS devices
>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>
>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>
>>>>>>
>>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>>> sorted out.
>>>>>
>>>>>
>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>>> have no intention to add a new feature to the non-DM driver.
>>>>
>>>>
>>>> But then this creates a massive disparity, it's like we're growing two
>>>> USB stacks.
>>>>
>>>
>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>> the old non-DM stuff, I am not sure how this can encourage people to
>>> switch to DM.
>>
>>
>> Correct. We definitely don't want to add new features to non-DM
>> drivers / IF, if this is non-trivial.
>>
>>> Maybe I can check all boards that use xHCI to see if
>>> they are switched to DM?
>>
>>
>> xHCI is still quite new in U-Boot, so I would expect that all
>> platforms using it, are using DM or at least not far away from using
>> it. Yes, please check all xHCI "users", if they use DM. Then we
>> know if and which users need some "persuasion" to switch over to
>> DM soon. ;)
> 
> I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
> CONFIG_DM_USB. Here is the list.
> 
> configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
> configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
> configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
> configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
> configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
> configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
> configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
> configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
> configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
> configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
> configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
> configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
> configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
> configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
> configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
> configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
> configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
> 
> So it looks we have lots of conversion work to be done by many board
> maintainers. I am not sure how to proceed on this.

Could the USB_DM be implied on these boards ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Stefan Roese]

Hi Bin,

On 27.06.2017 10:27, Bin Meng wrote:
> Hi Stefan,
> 
> On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Bin,
>>
>>
>> On 27.06.2017 02:01, Bin Meng wrote:
>>>
>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>>
>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>
>>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>
>>>>>>> With this commit and along with previous commits, now SS & HS devices
>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>
>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>
>>>>>>
>>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>>> sorted out.
>>>>>
>>>>>
>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>>> have no intention to add a new feature to the non-DM driver.
>>>>
>>>>
>>>> But then this creates a massive disparity, it's like we're growing two
>>>> USB stacks.
>>>>
>>>
>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>> the old non-DM stuff, I am not sure how this can encourage people to
>>> switch to DM.
>>
>>
>> Correct. We definitely don't want to add new features to non-DM
>> drivers / IF, if this is non-trivial.
>>
>>> Maybe I can check all boards that use xHCI to see if
>>> they are switched to DM?
>>
>>
>> xHCI is still quite new in U-Boot, so I would expect that all
>> platforms using it, are using DM or at least not far away from using
>> it. Yes, please check all xHCI "users", if they use DM. Then we
>> know if and which users need some "persuasion" to switch over to
>> DM soon. ;)
> 
> I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
> CONFIG_DM_USB. Here is the list.
> 
> configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
> configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
> configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
> configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
> configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
> configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
> configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
> configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
> configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
> configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
> configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
> configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
> configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
> configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
> configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
> configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
> configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
> configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
> configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
> configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
> configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
> configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
> 
> So it looks we have lots of conversion work to be done by many board
> maintainers. I am not sure how to proceed on this.

Marek reminded me, that he thinks that most of these platforms
above will most likely select DM_USB implicitly via Kconfig.

I did a quick check and it seems that at least these platforms
have DM_USB enabled:

ARCH_ZYNQ
ARCH_ZYNQMP
ARCH_UNIPHIER
ARCH_ROCKCHIP

and other from above very likely as well.

Before you invest more time and effort into implementing the xHCI
additions in a "non-DM cleaner way", I suggest to find out which
targets really use xHCI without USB_DM. An easy check would be to
add some #error to the non-DM part and run this commit through
buildman / travis.

What do you think?

Thanks,
Stefan

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

Hi Stefan,

On Wed, Jun 28, 2017 at 7:28 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
>
> On 27.06.2017 10:27, Bin Meng wrote:
>>
>> Hi Stefan,
>>
>> On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
>>>
>>> Hi Bin,
>>>
>>>
>>> On 27.06.2017 02:01, Bin Meng wrote:
>>>>
>>>>
>>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>>
>>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>>>
>>>>>>
>>>>>> Hi Marek,
>>>>>>
>>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>>
>>>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>>
>>>>>>>> With this commit and along with previous commits, now SS & HS
>>>>>>>> devices
>>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>>
>>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>>>> sorted out.
>>>>>>
>>>>>>
>>>>>>
>>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>>>> have no intention to add a new feature to the non-DM driver.
>>>>>
>>>>>
>>>>>
>>>>> But then this creates a massive disparity, it's like we're growing two
>>>>> USB stacks.
>>>>>
>>>>
>>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>>> the old non-DM stuff, I am not sure how this can encourage people to
>>>> switch to DM.
>>>
>>>
>>>
>>> Correct. We definitely don't want to add new features to non-DM
>>> drivers / IF, if this is non-trivial.
>>>
>>>> Maybe I can check all boards that use xHCI to see if
>>>> they are switched to DM?
>>>
>>>
>>>
>>> xHCI is still quite new in U-Boot, so I would expect that all
>>> platforms using it, are using DM or at least not far away from using
>>> it. Yes, please check all xHCI "users", if they use DM. Then we
>>> know if and which users need some "persuasion" to switch over to
>>> DM soon. ;)
>>
>>
>> I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
>> CONFIG_DM_USB. Here is the list.
>>
>> configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
>> configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
>> configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
>> configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
>> configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
>> configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>
>> configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
>> configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>> configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
>> configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>> configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
>> configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
>> configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
>> configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
>> configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
>> configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
>> configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>> configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
>> configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
>> configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
>> configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
>> configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
>> configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>> configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
>> configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
>> configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
>> configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>> configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
>> configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>> configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>> configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>> configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
>> configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>
>> So it looks we have lots of conversion work to be done by many board
>> maintainers. I am not sure how to proceed on this.
>
>
> Marek reminded me, that he thinks that most of these platforms
> above will most likely select DM_USB implicitly via Kconfig.
>
> I did a quick check and it seems that at least these platforms
> have DM_USB enabled:
>
> ARCH_ZYNQ
> ARCH_ZYNQMP
> ARCH_UNIPHIER
> ARCH_ROCKCHIP
>
> and other from above very likely as well.
>
> Before you invest more time and effort into implementing the xHCI
> additions in a "non-DM cleaner way", I suggest to find out which
> targets really use xHCI without USB_DM. An easy check would be to
> add some #error to the non-DM part and run this commit through
> buildman / travis.
>
> What do you think?

Ah, that's really a good idea. Thanks for the hints! I will launch a
buildman testing soon.

Regards,
Bin

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

Hi Stefan,

On Wed, Jun 28, 2017 at 8:27 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Stefan,
>
> On Wed, Jun 28, 2017 at 7:28 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Bin,
>>
>>
>> On 27.06.2017 10:27, Bin Meng wrote:
>>>
>>> Hi Stefan,
>>>
>>> On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Hi Bin,
>>>>
>>>>
>>>> On 27.06.2017 02:01, Bin Meng wrote:
>>>>>
>>>>>
>>>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>>
>>>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>>>
>>>>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>>>
>>>>>>>>> With this commit and along with previous commits, now SS & HS
>>>>>>>>> devices
>>>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>>>
>>>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>>>>> sorted out.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>>>>> have no intention to add a new feature to the non-DM driver.
>>>>>>
>>>>>>
>>>>>>
>>>>>> But then this creates a massive disparity, it's like we're growing two
>>>>>> USB stacks.
>>>>>>
>>>>>
>>>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>>>> the old non-DM stuff, I am not sure how this can encourage people to
>>>>> switch to DM.
>>>>
>>>>
>>>>
>>>> Correct. We definitely don't want to add new features to non-DM
>>>> drivers / IF, if this is non-trivial.
>>>>
>>>>> Maybe I can check all boards that use xHCI to see if
>>>>> they are switched to DM?
>>>>
>>>>
>>>>
>>>> xHCI is still quite new in U-Boot, so I would expect that all
>>>> platforms using it, are using DM or at least not far away from using
>>>> it. Yes, please check all xHCI "users", if they use DM. Then we
>>>> know if and which users need some "persuasion" to switch over to
>>>> DM soon. ;)
>>>
>>>
>>> I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
>>> CONFIG_DM_USB. Here is the list.
>>>
>>> configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>> configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
>>> configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
>>> configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>> configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>
>>> configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>> configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>> configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
>>> configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>> configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>> configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>> configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
>>> configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>> configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
>>> configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
>>> configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>> configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>> configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>> configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>> configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>> configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>> configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>> configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>> configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>> configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>> configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>> configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
>>> configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>> configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>> configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>> configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
>>> configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>
>>> So it looks we have lots of conversion work to be done by many board
>>> maintainers. I am not sure how to proceed on this.
>>
>>
>> Marek reminded me, that he thinks that most of these platforms
>> above will most likely select DM_USB implicitly via Kconfig.
>>
>> I did a quick check and it seems that at least these platforms
>> have DM_USB enabled:
>>
>> ARCH_ZYNQ
>> ARCH_ZYNQMP
>> ARCH_UNIPHIER
>> ARCH_ROCKCHIP
>>
>> and other from above very likely as well.
>>
>> Before you invest more time and effort into implementing the xHCI
>> additions in a "non-DM cleaner way", I suggest to find out which
>> targets really use xHCI without USB_DM. An easy check would be to
>> add some #error to the non-DM part and run this commit through
>> buildman / travis.
>>
>> What do you think?
>
> Ah, that's really a good idea. Thanks for the hints! I will launch a
> buildman testing soon.
>

Looks we have a smaller list indeed. Here is the buildman results:

ls1012ardb_qspi_SECURE_BOOT
ls1021atwr_nor_SECURE_BOOT
am43xx_hs_evm
am57xx_hs_evm
ls1021aqds_nand
ls1021atwr_nor
ls1021atwr_qspi
cm_t43
ls1021atwr_nor_lpuart
ls1021aqds_sdcard_qspi
k2hk_hs_evm
am43xx_evm
ls1021aqds_qspi
am57xx_evm_nodt
k2g_hs_evm
ls1021atwr_sdcard_qspi
am43xx_evm_ethboot
ls1021aqds_sdcard_ifc
k2l_evm
am43xx_evm_usbhost_boot
am43xx_evm_qspiboot
k2g_evm
am57xx_evm
ls1021atwr_sdcard_ifc
cl-som-am57x
k2hk_evm
k2e_evm
ls1021atwr_sdcard_ifc_SECURE_BOOT
ls1021aqds_nor_SECURE_BOOT
k2e_hs_evm

Regards,
Bin

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Stefan Roese]

Hi Bin,

On 29.06.2017 01:00, Bin Meng wrote:
> Hi Stefan,
> 
> On Wed, Jun 28, 2017 at 8:27 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Stefan,
>>
>> On Wed, Jun 28, 2017 at 7:28 PM, Stefan Roese <sr@denx.de> wrote:
>>> Hi Bin,
>>>
>>>
>>> On 27.06.2017 10:27, Bin Meng wrote:
>>>>
>>>> Hi Stefan,
>>>>
>>>> On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> Hi Bin,
>>>>>
>>>>>
>>>>> On 27.06.2017 02:01, Bin Meng wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>>>>
>>>>>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>>>>
>>>>>>>>>> With this commit and along with previous commits, now SS & HS
>>>>>>>>>> devices
>>>>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>>>>
>>>>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>>>>>> sorted out.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>>>>>> have no intention to add a new feature to the non-DM driver.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> But then this creates a massive disparity, it's like we're growing two
>>>>>>> USB stacks.
>>>>>>>
>>>>>>
>>>>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>>>>> the old non-DM stuff, I am not sure how this can encourage people to
>>>>>> switch to DM.
>>>>>
>>>>>
>>>>>
>>>>> Correct. We definitely don't want to add new features to non-DM
>>>>> drivers / IF, if this is non-trivial.
>>>>>
>>>>>> Maybe I can check all boards that use xHCI to see if
>>>>>> they are switched to DM?
>>>>>
>>>>>
>>>>>
>>>>> xHCI is still quite new in U-Boot, so I would expect that all
>>>>> platforms using it, are using DM or at least not far away from using
>>>>> it. Yes, please check all xHCI "users", if they use DM. Then we
>>>>> know if and which users need some "persuasion" to switch over to
>>>>> DM soon. ;)
>>>>
>>>>
>>>> I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
>>>> CONFIG_DM_USB. Here is the list.
>>>>
>>>> configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>>> configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
>>>> configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
>>>> configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>> configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>
>>>> configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>>> configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>> configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
>>>> configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>>> configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>>> configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>> configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
>>>> configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>>> configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
>>>> configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
>>>> configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>> configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>> configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>> configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>>> configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>>> configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>>> configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>>> configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>>> configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>>> configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>>> configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>> configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
>>>> configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>> configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>> configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>> configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
>>>> configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>
>>>> So it looks we have lots of conversion work to be done by many board
>>>> maintainers. I am not sure how to proceed on this.
>>>
>>>
>>> Marek reminded me, that he thinks that most of these platforms
>>> above will most likely select DM_USB implicitly via Kconfig.
>>>
>>> I did a quick check and it seems that at least these platforms
>>> have DM_USB enabled:
>>>
>>> ARCH_ZYNQ
>>> ARCH_ZYNQMP
>>> ARCH_UNIPHIER
>>> ARCH_ROCKCHIP
>>>
>>> and other from above very likely as well.
>>>
>>> Before you invest more time and effort into implementing the xHCI
>>> additions in a "non-DM cleaner way", I suggest to find out which
>>> targets really use xHCI without USB_DM. An easy check would be to
>>> add some #error to the non-DM part and run this commit through
>>> buildman / travis.
>>>
>>> What do you think?
>>
>> Ah, that's really a good idea. Thanks for the hints! I will launch a
>> buildman testing soon.
>>
> 
> Looks we have a smaller list indeed. Here is the buildman results:
> 
> ls1012ardb_qspi_SECURE_BOOT
> ls1021atwr_nor_SECURE_BOOT
> am43xx_hs_evm
> am57xx_hs_evm
> ls1021aqds_nand
> ls1021atwr_nor
> ls1021atwr_qspi
> cm_t43
> ls1021atwr_nor_lpuart
> ls1021aqds_sdcard_qspi
> k2hk_hs_evm
> am43xx_evm
> ls1021aqds_qspi
> am57xx_evm_nodt
> k2g_hs_evm
> ls1021atwr_sdcard_qspi
> am43xx_evm_ethboot
> ls1021aqds_sdcard_ifc
> k2l_evm
> am43xx_evm_usbhost_boot
> am43xx_evm_qspiboot
> k2g_evm
> am57xx_evm
> ls1021atwr_sdcard_ifc
> cl-som-am57x
> k2hk_evm
> k2e_evm
> ls1021atwr_sdcard_ifc_SECURE_BOOT
> ls1021aqds_nor_SECURE_BOOT
> k2e_hs_evm

Thanks. So this leaves only some platforms using xHCI without DM_USB
enabled indeed. I suspect that most of them don't have DM_USB enabled
just because of oversight. Let me write a short mail to the
maintainers, to see if they can enable DM_USB to make the way free
for a complete DM based xHCI support.

Thanks,
Stefan

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

Hi Stefan,

On Thu, Jun 29, 2017 at 1:29 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
>
> On 29.06.2017 01:00, Bin Meng wrote:
>>
>> Hi Stefan,
>>
>> On Wed, Jun 28, 2017 at 8:27 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> Hi Stefan,
>>>
>>> On Wed, Jun 28, 2017 at 7:28 PM, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Hi Bin,
>>>>
>>>>
>>>> On 27.06.2017 10:27, Bin Meng wrote:
>>>>>
>>>>>
>>>>> Hi Stefan,
>>>>>
>>>>> On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>>
>>>>>> Hi Bin,
>>>>>>
>>>>>>
>>>>>> On 27.06.2017 02:01, Bin Meng wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>>>>>
>>>>>>>>>>> So far U-Boot does not program this field, and it does not
>>>>>>>>>>> prevent
>>>>>>>>>>> SS device directly attached to root port, or HS device behind an
>>>>>>>>>>> HS
>>>>>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>>>>>
>>>>>>>>>>> With this commit and along with previous commits, now SS & HS
>>>>>>>>>>> devices
>>>>>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>>>>>
>>>>>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Great, but I really dislike the ifdef pollution, so this needs to
>>>>>>>>>> be
>>>>>>>>>> sorted out.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice.
>>>>>>>>> I
>>>>>>>>> have no intention to add a new feature to the non-DM driver.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> But then this creates a massive disparity, it's like we're growing
>>>>>>>> two
>>>>>>>> USB stacks.
>>>>>>>>
>>>>>>>
>>>>>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>>>>>> the old non-DM stuff, I am not sure how this can encourage people to
>>>>>>> switch to DM.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Correct. We definitely don't want to add new features to non-DM
>>>>>> drivers / IF, if this is non-trivial.
>>>>>>
>>>>>>> Maybe I can check all boards that use xHCI to see if
>>>>>>> they are switched to DM?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> xHCI is still quite new in U-Boot, so I would expect that all
>>>>>> platforms using it, are using DM or at least not far away from using
>>>>>> it. Yes, please check all xHCI "users", if they use DM. Then we
>>>>>> know if and which users need some "persuasion" to switch over to
>>>>>> DM soon. ;)
>>>>>
>>>>>
>>>>>
>>>>> I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
>>>>> CONFIG_DM_USB. Here is the list.
>>>>>
>>>>> configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>>>>
>>>>> configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
>>>>> configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>> configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>
>>>>>
>>>>> configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>>>> configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
>>>>> configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>>>> configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>>>> configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
>>>>> configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
>>>>> configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>> configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>
>>>>> configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>> configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>>>> configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>>>> configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>>>> configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>>>> configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>>>> configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>> configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
>>>>> configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>> configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>>> configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>> configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
>>>>> configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>
>>>>> So it looks we have lots of conversion work to be done by many board
>>>>> maintainers. I am not sure how to proceed on this.
>>>>
>>>>
>>>>
>>>> Marek reminded me, that he thinks that most of these platforms
>>>> above will most likely select DM_USB implicitly via Kconfig.
>>>>
>>>> I did a quick check and it seems that at least these platforms
>>>> have DM_USB enabled:
>>>>
>>>> ARCH_ZYNQ
>>>> ARCH_ZYNQMP
>>>> ARCH_UNIPHIER
>>>> ARCH_ROCKCHIP
>>>>
>>>> and other from above very likely as well.
>>>>
>>>> Before you invest more time and effort into implementing the xHCI
>>>> additions in a "non-DM cleaner way", I suggest to find out which
>>>> targets really use xHCI without USB_DM. An easy check would be to
>>>> add some #error to the non-DM part and run this commit through
>>>> buildman / travis.
>>>>
>>>> What do you think?
>>>
>>>
>>> Ah, that's really a good idea. Thanks for the hints! I will launch a
>>> buildman testing soon.
>>>
>>
>> Looks we have a smaller list indeed. Here is the buildman results:
>>
>> ls1012ardb_qspi_SECURE_BOOT
>> ls1021atwr_nor_SECURE_BOOT
>> am43xx_hs_evm
>> am57xx_hs_evm
>> ls1021aqds_nand
>> ls1021atwr_nor
>> ls1021atwr_qspi
>> cm_t43
>> ls1021atwr_nor_lpuart
>> ls1021aqds_sdcard_qspi
>> k2hk_hs_evm
>> am43xx_evm
>> ls1021aqds_qspi
>> am57xx_evm_nodt
>> k2g_hs_evm
>> ls1021atwr_sdcard_qspi
>> am43xx_evm_ethboot
>> ls1021aqds_sdcard_ifc
>> k2l_evm
>> am43xx_evm_usbhost_boot
>> am43xx_evm_qspiboot
>> k2g_evm
>> am57xx_evm
>> ls1021atwr_sdcard_ifc
>> cl-som-am57x
>> k2hk_evm
>> k2e_evm
>> ls1021atwr_sdcard_ifc_SECURE_BOOT
>> ls1021aqds_nor_SECURE_BOOT
>> k2e_hs_evm
>
>
> Thanks. So this leaves only some platforms using xHCI without DM_USB
> enabled indeed. I suspect that most of them don't have DM_USB enabled
> just because of oversight. Let me write a short mail to the
> maintainers, to see if they can enable DM_USB to make the way free
> for a complete DM based xHCI support.

Thank you for your help. I see at least ls1021a_xxx boards are because
of oversight as some other ls1021a boards have DM_USB.

Regards,
Bin

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Stefan Roese]

Hi Bin,

On 29.06.2017 07:42, Bin Meng wrote:
> Hi Stefan,
> 
> On Thu, Jun 29, 2017 at 1:29 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Bin,
>>
>>
>> On 29.06.2017 01:00, Bin Meng wrote:
>>>
>>> Hi Stefan,
>>>
>>> On Wed, Jun 28, 2017 at 8:27 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> Hi Stefan,
>>>>
>>>> On Wed, Jun 28, 2017 at 7:28 PM, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> Hi Bin,
>>>>>
>>>>>
>>>>> On 27.06.2017 10:27, Bin Meng wrote:
>>>>>>
>>>>>>
>>>>>> Hi Stefan,
>>>>>>
>>>>>> On Tue, Jun 27, 2017 at 1:23 PM, Stefan Roese <sr@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Bin,
>>>>>>>
>>>>>>>
>>>>>>> On 27.06.2017 02:01, Bin Meng wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>>>>>>
>>>>>>>>>>>> So far U-Boot does not program this field, and it does not
>>>>>>>>>>>> prevent
>>>>>>>>>>>> SS device directly attached to root port, or HS device behind an
>>>>>>>>>>>> HS
>>>>>>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>>>>>>
>>>>>>>>>>>> With this commit and along with previous commits, now SS & HS
>>>>>>>>>>>> devices
>>>>>>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>>>>>>
>>>>>>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Great, but I really dislike the ifdef pollution, so this needs to
>>>>>>>>>>> be
>>>>>>>>>>> sorted out.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice.
>>>>>>>>>> I
>>>>>>>>>> have no intention to add a new feature to the non-DM driver.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> But then this creates a massive disparity, it's like we're growing
>>>>>>>>> two
>>>>>>>>> USB stacks.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>>>>>>> the old non-DM stuff, I am not sure how this can encourage people to
>>>>>>>> switch to DM.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Correct. We definitely don't want to add new features to non-DM
>>>>>>> drivers / IF, if this is non-trivial.
>>>>>>>
>>>>>>>> Maybe I can check all boards that use xHCI to see if
>>>>>>>> they are switched to DM?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> xHCI is still quite new in U-Boot, so I would expect that all
>>>>>>> platforms using it, are using DM or at least not far away from using
>>>>>>> it. Yes, please check all xHCI "users", if they use DM. Then we
>>>>>>> know if and which users need some "persuasion" to switch over to
>>>>>>> DM soon. ;)
>>>>>>
>>>>>>
>>>>>>
>>>>>> I checked all boards that have CONFIG_USB_XHCI_HCD defined but without
>>>>>> CONFIG_DM_USB. Here is the list.
>>>>>>
>>>>>> configs/uniphier_v8_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>>>>>
>>>>>> configs/xilinx_zynqmp_zc1751_xm015_dc1_defconfig:62:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am57xx_evm_nodt_defconfig:53:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/evb-rk3328_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021atwr_nor_lpuart_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/uniphier_pxs2_ld6b_defconfig:44:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1012ardb_qspi_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>>
>>>>>>
>>>>>> configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/k2e_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021aqds_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am43xx_evm_ethboot_defconfig:48:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/xilinx_zynqmp_ep_defconfig:70:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021aqds_nand_defconfig:57:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021atwr_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/k2g_evm_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am57xx_evm_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am43xx_hs_evm_defconfig:49:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am43xx_evm_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021atwr_nor_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/firefly-rk3399_defconfig:59:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/puma-rk3399_defconfig:78:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/cl-som-am57x_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021aqds_nor_SECURE_BOOT_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/uniphier_pro4_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>>
>>>>>> configs/xilinx_zynqmp_zc1751_xm016_dc2_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/xilinx_zynqmp_zcu102_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021atwr_nor_SECURE_BOOT_defconfig:42:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/cm_t43_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/k2g_hs_evm_defconfig:36:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am43xx_evm_qspiboot_defconfig:45:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021aqds_qspi_defconfig:50:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am57xx_hs_evm_defconfig:67:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/xilinx_zynqmp_zcu102_revB_defconfig:63:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021aqds_sdcard_ifc_defconfig:55:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/uniphier_ld20_defconfig:39:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/am43xx_evm_usbhost_boot_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021atwr_sdcard_qspi_defconfig:61:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/evb-rk3399_defconfig:60:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/k2hk_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/k2hk_hs_evm_defconfig:34:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/k2e_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/ls1021atwr_sdcard_ifc_defconfig:54:CONFIG_USB_XHCI_HCD=y
>>>>>> configs/k2l_evm_defconfig:43:CONFIG_USB_XHCI_HCD=y
>>>>>>
>>>>>> So it looks we have lots of conversion work to be done by many board
>>>>>> maintainers. I am not sure how to proceed on this.
>>>>>
>>>>>
>>>>>
>>>>> Marek reminded me, that he thinks that most of these platforms
>>>>> above will most likely select DM_USB implicitly via Kconfig.
>>>>>
>>>>> I did a quick check and it seems that at least these platforms
>>>>> have DM_USB enabled:
>>>>>
>>>>> ARCH_ZYNQ
>>>>> ARCH_ZYNQMP
>>>>> ARCH_UNIPHIER
>>>>> ARCH_ROCKCHIP
>>>>>
>>>>> and other from above very likely as well.
>>>>>
>>>>> Before you invest more time and effort into implementing the xHCI
>>>>> additions in a "non-DM cleaner way", I suggest to find out which
>>>>> targets really use xHCI without USB_DM. An easy check would be to
>>>>> add some #error to the non-DM part and run this commit through
>>>>> buildman / travis.
>>>>>
>>>>> What do you think?
>>>>
>>>>
>>>> Ah, that's really a good idea. Thanks for the hints! I will launch a
>>>> buildman testing soon.
>>>>
>>>
>>> Looks we have a smaller list indeed. Here is the buildman results:
>>>
>>> ls1012ardb_qspi_SECURE_BOOT
>>> ls1021atwr_nor_SECURE_BOOT
>>> am43xx_hs_evm
>>> am57xx_hs_evm
>>> ls1021aqds_nand
>>> ls1021atwr_nor
>>> ls1021atwr_qspi
>>> cm_t43
>>> ls1021atwr_nor_lpuart
>>> ls1021aqds_sdcard_qspi
>>> k2hk_hs_evm
>>> am43xx_evm
>>> ls1021aqds_qspi
>>> am57xx_evm_nodt
>>> k2g_hs_evm
>>> ls1021atwr_sdcard_qspi
>>> am43xx_evm_ethboot
>>> ls1021aqds_sdcard_ifc
>>> k2l_evm
>>> am43xx_evm_usbhost_boot
>>> am43xx_evm_qspiboot
>>> k2g_evm
>>> am57xx_evm
>>> ls1021atwr_sdcard_ifc
>>> cl-som-am57x
>>> k2hk_evm
>>> k2e_evm
>>> ls1021atwr_sdcard_ifc_SECURE_BOOT
>>> ls1021aqds_nor_SECURE_BOOT
>>> k2e_hs_evm
>>
>>
>> Thanks. So this leaves only some platforms using xHCI without DM_USB
>> enabled indeed. I suspect that most of them don't have DM_USB enabled
>> just because of oversight. Let me write a short mail to the
>> maintainers, to see if they can enable DM_USB to make the way free
>> for a complete DM based xHCI support.
> 
> Thank you for your help. I see at least ls1021a_xxx boards are because
> of oversight as some other ls1021a boards have DM_USB.

Yes, I noticed this as well. I just sent a mail to all the maintainers
of those boards. Lets see, what they respond... ;)

Thanks,
Stefan

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Marek Vasut]

On 06/27/2017 07:23 AM, Stefan Roese wrote:
> Hi Bin,
> 
> On 27.06.2017 02:01, Bin Meng wrote:
>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>> Hi Marek,
>>>>
>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>> device slot, and shall not be modified by any other command.
>>>>>>
>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>> behind an SS hub, this field must be programmed.
>>>>>>
>>>>>> With this commit and along with previous commits, now SS & HS devices
>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>
>>>>>> As usual, this new feature is only available when DM is on.
>>>>>
>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>> sorted out.
>>>>
>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>> have no intention to add a new feature to the non-DM driver.
>>>
>>> But then this creates a massive disparity, it's like we're growing two
>>> USB stacks.
>>>
>>
>> Yep, unfortunately. But if we continue adding new features/fixes to
>> the old non-DM stuff, I am not sure how this can encourage people to
>> switch to DM.
> 
> Correct. We definitely don't want to add new features to non-DM
> drivers / IF, if this is non-trivial.

Fine, but we also don't want to grow two distinct stacks with a boatload
of ifdefs all over the place. That's a nightmare to maintain.
I think I mentioned that already, but I'd be far more accepting to this
solution if we could at least keep the added ifdefs to minimum and
somehow keep them in one place instead of adding them all over the code.

>> Maybe I can check all boards that use xHCI to see if
>> they are switched to DM?
> 
> xHCI is still quite new in U-Boot, so I would expect that all
> platforms using it, are using DM or at least not far away from using
> it. Yes, please check all xHCI "users", if they use DM. Then we
> know if and which users need some "persuasion" to switch over to
> DM soon. ;)
> 
> Thanks,
> Stefan


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Bin Meng]

Hi Marek,

On Tue, Jun 27, 2017 at 5:31 PM, Marek Vasut <marex@denx.de> wrote:
> On 06/27/2017 07:23 AM, Stefan Roese wrote:
>> Hi Bin,
>>
>> On 27.06.2017 02:01, Bin Meng wrote:
>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>
>>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>
>>>>>>> With this commit and along with previous commits, now SS & HS devices
>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>
>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>
>>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>>> sorted out.
>>>>>
>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>>> have no intention to add a new feature to the non-DM driver.
>>>>
>>>> But then this creates a massive disparity, it's like we're growing two
>>>> USB stacks.
>>>>
>>>
>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>> the old non-DM stuff, I am not sure how this can encourage people to
>>> switch to DM.
>>
>> Correct. We definitely don't want to add new features to non-DM
>> drivers / IF, if this is non-trivial.
>
> Fine, but we also don't want to grow two distinct stacks with a boatload
> of ifdefs all over the place. That's a nightmare to maintain.
> I think I mentioned that already, but I'd be far more accepting to this
> solution if we could at least keep the added ifdefs to minimum and
> somehow keep them in one place instead of adding them all over the code.
>

OK, I will see if I can do some additional work to remove the #ifdefs
or limit them in a minimum way, even if that means I have to bring
(part of) this feature to the non-DM driver.

>>> Maybe I can check all boards that use xHCI to see if
>>> they are switched to DM?
>>
>> xHCI is still quite new in U-Boot, so I would expect that all
>> platforms using it, are using DM or at least not far away from using
>> it. Yes, please check all xHCI "users", if they use DM. Then we
>> know if and which users need some "persuasion" to switch over to
>> DM soon. ;)

[snip]

Regards,
Bin

[U-Boot] [PATCH 10/16] usb: xhci: Program 'route string' in the input slot context

[Simon Glass]

Hi Marek,

On 27 June 2017 at 03:31, Marek Vasut <marex@denx.de> wrote:
> On 06/27/2017 07:23 AM, Stefan Roese wrote:
>> Hi Bin,
>>
>> On 27.06.2017 02:01, Bin Meng wrote:
>>> On Tue, Jun 27, 2017 at 2:07 AM, Marek Vasut <marex@denx.de> wrote:
>>>> On 06/24/2017 03:57 AM, Bin Meng wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On Sat, Jun 24, 2017 at 2:02 AM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 06/23/2017 11:54 AM, Bin Meng wrote:
>>>>>>> xHCI spec says: the values of the 'route string' field shall be
>>>>>>> initialized by the first 'Address Device' command issued to a
>>>>>>> device slot, and shall not be modified by any other command.
>>>>>>>
>>>>>>> So far U-Boot does not program this field, and it does not prevent
>>>>>>> SS device directly attached to root port, or HS device behind an HS
>>>>>>> hub, from working, due to the fact that 'route string' is used by
>>>>>>> the xHC to target SS packets. But in order to enumerate devices
>>>>>>> behind an SS hub, this field must be programmed.
>>>>>>>
>>>>>>> With this commit and along with previous commits, now SS & HS devices
>>>>>>> attached to a USB 3.0 hub can be enumerated by U-Boot.
>>>>>>>
>>>>>>> As usual, this new feature is only available when DM is on.
>>>>>>
>>>>>> Great, but I really dislike the ifdef pollution, so this needs to be
>>>>>> sorted out.
>>>>>
>>>>> The ifdef was needed due to it calls DM APIs or access DM udevice. I
>>>>> have no intention to add a new feature to the non-DM driver.
>>>>
>>>> But then this creates a massive disparity, it's like we're growing two
>>>> USB stacks.
>>>>
>>>
>>> Yep, unfortunately. But if we continue adding new features/fixes to
>>> the old non-DM stuff, I am not sure how this can encourage people to
>>> switch to DM.
>>
>> Correct. We definitely don't want to add new features to non-DM
>> drivers / IF, if this is non-trivial.
>
> Fine, but we also don't want to grow two distinct stacks with a boatload
> of ifdefs all over the place. That's a nightmare to maintain.
> I think I mentioned that already, but I'd be far more accepting to this
> solution if we could at least keep the added ifdefs to minimum and
> somehow keep them in one place instead of adding them all over the code.

One idea is to set a deadline (e.g. end of year) for all boards to
move to DM_USB, and turn off USB for those that don't make it? In some
areas the old code is quite a significant impediment to progress /
productivity.

>
>>> Maybe I can check all boards that use xHCI to see if
>>> they are switched to DM?
>>
>> xHCI is still quite new in U-Boot, so I would expect that all
>> platforms using it, are using DM or at least not far away from using
>> it. Yes, please check all xHCI "users", if they use DM. Then we
>> know if and which users need some "persuasion" to switch over to
>> DM soon. ;)

Regards,
Simon

[U-Boot] [PATCH 11/16] usb: hub: Parse and save TT details from device descriptor

[Bin Meng]

A high speed hub has a special responsibility to handle full speed/
low speed devices connected on downstream ports. In this case, the
hub must isolate the high speed signaling environment from the full
speed/low speed signaling environment with the help of Transaction
Translator (TT). TT details are provided by hub descriptors and we
parse and save it to hub uclass_priv for later use.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_hub.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/usb.h      | 16 ++++++++++++++++
 include/usb_defs.h | 12 ++++++++++++
 3 files changed, 78 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 63358cd..4911981 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -693,6 +693,56 @@ static int usb_hub_configure(struct usb_device *dev)
 		break;
 	}
 
+	switch (dev->descriptor.bDeviceProtocol) {
+	case USB_HUB_PR_FS:
+		break;
+	case USB_HUB_PR_HS_SINGLE_TT:
+		debug("Single TT\n");
+		break;
+	case USB_HUB_PR_HS_MULTI_TT:
+		ret = usb_set_interface(dev, 0, 1);
+		if (ret == 0) {
+			debug("TT per port\n");
+			hub->tt.multi = true;
+		} else {
+			debug("Using single TT (err %d)\n", ret);
+		}
+		break;
+	case USB_HUB_PR_SS:
+		/* USB 3.0 hubs don't have a TT */
+		break;
+	default:
+		debug("Unrecognized hub protocol %d\n",
+		      dev->descriptor.bDeviceProtocol);
+		break;
+	}
+
+	/* Note 8 FS bit times == (8 bits / 12000000 bps) ~= 666ns */
+	switch (hubCharacteristics & HUB_CHAR_TTTT) {
+	case HUB_TTTT_8_BITS:
+		if (dev->descriptor.bDeviceProtocol != 0) {
+			hub->tt.think_time = 666;
+			debug("TT requires at most %d FS bit times (%d ns)\n",
+			      8, hub->tt.think_time);
+		}
+		break;
+	case HUB_TTTT_16_BITS:
+		hub->tt.think_time = 666 * 2;
+		debug("TT requires at most %d FS bit times (%d ns)\n",
+		      16, hub->tt.think_time);
+		break;
+	case HUB_TTTT_24_BITS:
+		hub->tt.think_time = 666 * 3;
+		debug("TT requires at most %d FS bit times (%d ns)\n",
+		      24, hub->tt.think_time);
+		break;
+	case HUB_TTTT_32_BITS:
+		hub->tt.think_time = 666 * 4;
+		debug("TT requires at most %d FS bit times (%d ns)\n",
+		      32, hub->tt.think_time);
+		break;
+	}
+
 	debug("power on to power good time: %dms\n",
 	      descriptor->bPwrOn2PwrGood * 2);
 	debug("hub controller current requirement: %dmA\n",
diff --git a/include/usb.h b/include/usb.h
index 40e44f4..6e42be5 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -537,6 +537,21 @@ struct usb_hub_status {
 	unsigned short wHubChange;
 } __attribute__ ((packed));
 
+/*
+ * Hub Device descriptor
+ * USB Hub class device protocols
+ */
+#define USB_HUB_PR_FS		0 /* Full speed hub */
+#define USB_HUB_PR_HS_NO_TT	0 /* Hi-speed hub without TT */
+#define USB_HUB_PR_HS_SINGLE_TT	1 /* Hi-speed hub with single TT */
+#define USB_HUB_PR_HS_MULTI_TT	2 /* Hi-speed hub with multiple TT */
+#define USB_HUB_PR_SS		3 /* Super speed hub */
+
+/* Transaction Translator Think Times, in bits */
+#define HUB_TTTT_8_BITS		0x00
+#define HUB_TTTT_16_BITS	0x20
+#define HUB_TTTT_24_BITS	0x40
+#define HUB_TTTT_32_BITS	0x60
 
 /* Hub descriptor */
 struct usb_hub_descriptor {
@@ -571,6 +586,7 @@ struct usb_hub_device {
 	ulong query_delay;		/* Device query delay in ms */
 	int overcurrent_count[USB_MAXCHILDREN];	/* Over-current counter */
 	int hub_depth;			/* USB 3.0 hub depth */
+	struct usb_tt tt;		/* Transaction Translator */
 };
 
 #ifdef CONFIG_DM_USB
diff --git a/include/usb_defs.h b/include/usb_defs.h
index 273337f..b7f2ead 100644
--- a/include/usb_defs.h
+++ b/include/usb_defs.h
@@ -293,6 +293,7 @@
 #define HUB_CHAR_LPSM               0x0003
 #define HUB_CHAR_COMPOUND           0x0004
 #define HUB_CHAR_OCPM               0x0018
+#define HUB_CHAR_TTTT               0x0060 /* TT Think Time mask */
 
 /*
  * Hub Status & Hub Change bit masks
@@ -310,6 +311,17 @@
 #define USB_REQ_SET_HUB_DEPTH	0x0c
 
 /*
+ * As of USB 2.0, full/low speed devices are segregated into trees.
+ * One type grows from USB 1.1 host controllers (OHCI, UHCI etc).
+ * The other type grows from high speed hubs when they connect to
+ * full/low speed devices using "Transaction Translators" (TTs).
+ */
+struct usb_tt {
+	bool		multi;		/* true means one TT per port */
+	unsigned	think_time;	/* think time in ns */
+};
+
+/*
  * CBI style
  */
 
-- 
2.9.2

[U-Boot] [PATCH 11/16] usb: hub: Parse and save TT details from device descriptor

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> A high speed hub has a special responsibility to handle full speed/
> low speed devices connected on downstream ports. In this case, the
> hub must isolate the high speed signaling environment from the full
> speed/low speed signaling environment with the help of Transaction
> Translator (TT). TT details are provided by hub descriptors and we
> parse and save it to hub uclass_priv for later use.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  common/usb_hub.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/usb.h      | 16 ++++++++++++++++
>  include/usb_defs.h | 12 ++++++++++++
>  3 files changed, 78 insertions(+)

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

[U-Boot] [PATCH 12/16] dm: usb: Add a new USB controller operation 'update_hub_device'

[Bin Meng]

For USB host controllers like xHC, its internal representation of
hub needs to be updated after the hub descriptor is fetched. This
adds a new op that does this.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/usb-uclass.c | 11 +++++++++++
 include/usb.h                 | 21 ++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 9bb8477..1078b8c 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -139,6 +139,17 @@ int usb_reset_root_port(struct usb_device *udev)
 	return ops->reset_root_port(bus, udev);
 }
 
+int usb_update_hub_device(struct usb_device *udev)
+{
+	struct udevice *bus = udev->controller_dev;
+	struct dm_usb_ops *ops = usb_get_ops(bus);
+
+	if (!ops->update_hub_device)
+		return -ENOSYS;
+
+	return ops->update_hub_device(bus, udev);
+}
+
 int usb_stop(void)
 {
 	struct udevice *bus;
diff --git a/include/usb.h b/include/usb.h
index 6e42be5..d43be7e 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -758,6 +758,14 @@ struct dm_usb_ops {
 	 * reset_root_port() - Reset usb root port
 	 */
 	int (*reset_root_port)(struct udevice *bus, struct usb_device *udev);
+
+	/**
+	 * update_hub_device() - Update HCD's internal representation of hub
+	 *
+	 * After a hub descriptor is fetched, notify HCD so that its internal
+	 * representation of this hub can be updated (xHCI)
+	 */
+	int (*update_hub_device)(struct udevice *bus, struct usb_device *udev);
 };
 
 #define usb_get_ops(dev)	((struct dm_usb_ops *)(dev)->driver->ops)
@@ -949,6 +957,17 @@ int usb_new_device(struct usb_device *dev);
 int usb_alloc_device(struct usb_device *dev);
 
 /**
+ * update_hub_device() - Update HCD's internal representation of hub
+ *
+ * After a hub descriptor is fetched, notify HCD so that its internal
+ * representation of this hub can be updated.
+ *
+ * @dev:		Hub device
+ * @return 0 if OK, -ve on error
+ */
+int usb_update_hub_device(struct usb_device *dev);
+
+/**
  * usb_emul_setup_device() - Set up a new USB device emulation
  *
  * This is normally called when a new emulation device is bound. It tells
@@ -961,7 +980,7 @@ int usb_alloc_device(struct usb_device *dev);
  * @desc_list:		List of points or USB descriptors, terminated by NULL.
  *			The first entry must be struct usb_device_descriptor,
  *			and others follow on after that.
- * @return 0 if OK, -ve on error
+ * @return 0 if OK, -ENOSYS if not implemented, other -ve on error
  */
 int usb_emul_setup_device(struct udevice *dev, int maxpacketsize,
 			  struct usb_string *strings, void **desc_list);
-- 
2.9.2

[U-Boot] [PATCH 12/16] dm: usb: Add a new USB controller operation 'update_hub_device'

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> For USB host controllers like xHC, its internal representation of
> hub needs to be updated after the hub descriptor is fetched. This
> adds a new op that does this.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/usb/host/usb-uclass.c | 11 +++++++++++
>  include/usb.h                 | 21 ++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
>

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

[U-Boot] [PATCH 13/16] usb: hub: Call usb_update_hub_device() after hub descriptor is fetched

[Bin Meng]

After fetching hub descriptor, we need call USB uclass operation
update_hub_device() to notify HCD to do some preparation work.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/usb_hub.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 4911981..2fc544e 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -782,6 +782,17 @@ static int usb_hub_configure(struct usb_device *dev)
 
 #ifdef CONFIG_DM_USB
 	/*
+	 * Update USB host controller's internal representation of this hub
+	 * after the hub descriptor is fetched.
+	 */
+	ret = usb_update_hub_device(dev);
+	if (ret < 0 && ret != -ENOSYS) {
+		debug("%s: failed to update hub device for HCD (%x)\n",
+		      __func__, ret);
+		return ret;
+	}
+
+	/*
 	 * A maximum of seven tiers are allowed in a USB topology, and the
 	 * root hub occupies the first tier. The last tier ends with a normal
 	 * USB device. USB 3.0 hubs use a 20-bit field called 'route string'
-- 
2.9.2

[U-Boot] [PATCH 13/16] usb: hub: Call usb_update_hub_device() after hub descriptor is fetched

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> After fetching hub descriptor, we need call USB uclass operation

need to call

> update_hub_device() to notify HCD to do some preparation work.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  common/usb_hub.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

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

[U-Boot] [PATCH 14/16] usb: xhci: Implement update_hub_device() operation

[Bin Meng]

There is no way to know whether the attached device is a hub or
not in advance before device's descriptor is fetched. But once
we know it's a high speed hub, per xHCI spec, we need tell xHC
it's a hub device by initializing hub related fields in the
input slot context.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 1148127..a82502c 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1170,6 +1170,63 @@ static int xhci_alloc_device(struct udevice *dev, struct usb_device *udev)
 	return _xhci_alloc_device(udev);
 }
 
+static int xhci_update_hub_device(struct udevice *dev, struct usb_device *udev)
+{
+	struct xhci_ctrl *ctrl = dev_get_priv(dev);
+	struct usb_hub_device *hub = dev_get_uclass_priv(udev->dev);
+	struct xhci_virt_device *virt_dev;
+	struct xhci_input_control_ctx *ctrl_ctx;
+	struct xhci_container_ctx *out_ctx;
+	struct xhci_container_ctx *in_ctx;
+	struct xhci_slot_ctx *slot_ctx;
+	int slot_id = udev->slot_id;
+	unsigned think_time;
+
+	debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
+
+	/* Ignore root hubs */
+	if (usb_hub_is_root_hub(udev->dev))
+		return 0;
+
+	virt_dev = ctrl->devs[slot_id];
+	BUG_ON(!virt_dev);
+
+	out_ctx = virt_dev->out_ctx;
+	in_ctx = virt_dev->in_ctx;
+
+	ctrl_ctx = xhci_get_input_control_ctx(in_ctx);
+	/* Initialize the input context control */
+	ctrl_ctx->add_flags |= cpu_to_le32(SLOT_FLAG);
+	ctrl_ctx->drop_flags = 0;
+
+	xhci_inval_cache((uintptr_t)out_ctx->bytes, out_ctx->size);
+
+	/* slot context */
+	xhci_slot_copy(ctrl, in_ctx, out_ctx);
+	slot_ctx = xhci_get_slot_ctx(ctrl, in_ctx);
+
+	/* Update hub related fields */
+	slot_ctx->dev_info |= cpu_to_le32(DEV_HUB);
+	if (hub->tt.multi && udev->speed == USB_SPEED_HIGH)
+		slot_ctx->dev_info |= cpu_to_le32(DEV_MTT);
+	slot_ctx->dev_info2 |= cpu_to_le32(XHCI_MAX_PORTS(udev->maxchild));
+	/*
+	 * Set TT think time - convert from ns to FS bit times.
+	 *
+	 * 0 =  8 FS bit times, 1 = 16 FS bit times,
+	 * 2 = 24 FS bit times, 3 = 32 FS bit times.
+	 *
+	 * This field shall be 0 if the device is not a high-spped hub.
+	 */
+	think_time = hub->tt.think_time;
+	if (think_time != 0)
+		think_time = (think_time / 666) - 1;
+	if (udev->speed == USB_SPEED_HIGH)
+		slot_ctx->tt_info |= cpu_to_le32(TT_THINK_TIME(think_time));
+
+	return xhci_configure_endpoints(udev, false);
+}
+
 int xhci_register(struct udevice *dev, struct xhci_hccr *hccr,
 		  struct xhci_hcor *hcor)
 {
@@ -1222,6 +1279,7 @@ struct dm_usb_ops xhci_usb_ops = {
 	.bulk = xhci_submit_bulk_msg,
 	.interrupt = xhci_submit_int_msg,
 	.alloc_device = xhci_alloc_device,
+	.update_hub_device = xhci_update_hub_device,
 };
 
 #endif
-- 
2.9.2

[U-Boot] [PATCH 14/16] usb: xhci: Implement update_hub_device() operation

[Simon Glass]

Hi Bin,

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> There is no way to know whether the attached device is a hub or
> not in advance before device's descriptor is fetched. But once

nits:

before the device's

> we know it's a high speed hub, per xHCI spec, we need tell xHC

per the xHCI

we need to tell

> it's a hub device by initializing hub related fields in the

hub-related

> input slot context.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/usb/host/xhci.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)

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

>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 1148127..a82502c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1170,6 +1170,63 @@ static int xhci_alloc_device(struct udevice *dev, struct usb_device *udev)
>         return _xhci_alloc_device(udev);
>  }
>
> +static int xhci_update_hub_device(struct udevice *dev, struct usb_device *udev)
> +{
> +       struct xhci_ctrl *ctrl = dev_get_priv(dev);
> +       struct usb_hub_device *hub = dev_get_uclass_priv(udev->dev);
> +       struct xhci_virt_device *virt_dev;
> +       struct xhci_input_control_ctx *ctrl_ctx;
> +       struct xhci_container_ctx *out_ctx;
> +       struct xhci_container_ctx *in_ctx;
> +       struct xhci_slot_ctx *slot_ctx;
> +       int slot_id = udev->slot_id;
> +       unsigned think_time;
> +
> +       debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
> +
> +       /* Ignore root hubs */
> +       if (usb_hub_is_root_hub(udev->dev))
> +               return 0;
> +
> +       virt_dev = ctrl->devs[slot_id];
> +       BUG_ON(!virt_dev);
> +
> +       out_ctx = virt_dev->out_ctx;
> +       in_ctx = virt_dev->in_ctx;
> +
> +       ctrl_ctx = xhci_get_input_control_ctx(in_ctx);
> +       /* Initialize the input context control */
> +       ctrl_ctx->add_flags |= cpu_to_le32(SLOT_FLAG);
> +       ctrl_ctx->drop_flags = 0;
> +
> +       xhci_inval_cache((uintptr_t)out_ctx->bytes, out_ctx->size);
> +
> +       /* slot context */
> +       xhci_slot_copy(ctrl, in_ctx, out_ctx);
> +       slot_ctx = xhci_get_slot_ctx(ctrl, in_ctx);
> +
> +       /* Update hub related fields */
> +       slot_ctx->dev_info |= cpu_to_le32(DEV_HUB);
> +       if (hub->tt.multi && udev->speed == USB_SPEED_HIGH)
> +               slot_ctx->dev_info |= cpu_to_le32(DEV_MTT);
> +       slot_ctx->dev_info2 |= cpu_to_le32(XHCI_MAX_PORTS(udev->maxchild));
> +       /*
> +        * Set TT think time - convert from ns to FS bit times.
> +        *
> +        * 0 =  8 FS bit times, 1 = 16 FS bit times,
> +        * 2 = 24 FS bit times, 3 = 32 FS bit times.
> +        *
> +        * This field shall be 0 if the device is not a high-spped hub.
> +        */
> +       think_time = hub->tt.think_time;
> +       if (think_time != 0)
> +               think_time = (think_time / 666) - 1;

What is 666? Can you add a comment?

> +       if (udev->speed == USB_SPEED_HIGH)
> +               slot_ctx->tt_info |= cpu_to_le32(TT_THINK_TIME(think_time));
> +
> +       return xhci_configure_endpoints(udev, false);
> +}
> +
>  int xhci_register(struct udevice *dev, struct xhci_hccr *hccr,
>                   struct xhci_hcor *hcor)
>  {
> @@ -1222,6 +1279,7 @@ struct dm_usb_ops xhci_usb_ops = {
>         .bulk = xhci_submit_bulk_msg,
>         .interrupt = xhci_submit_int_msg,
>         .alloc_device = xhci_alloc_device,
> +       .update_hub_device = xhci_update_hub_device,
>  };
>
>  #endif
> --
> 2.9.2
>

Regards,
Simon

[U-Boot] [PATCH 15/16] usb: xhci: Correct TT_SLOT and TT_PORT macros

[Bin Meng]

These two macros really need a parameter to make them useful.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index cdce67c..a497d9d 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -548,12 +548,12 @@ struct xhci_slot_ctx {
  * The Slot ID of the hub that isolates the high speed signaling from
  * this low or full-speed device.  '0' if attached to root hub port.
  */
-#define TT_SLOT			(0xff)
+#define TT_SLOT(p)		(((p) & 0xff) << 0)
 /*
  * The number of the downstream facing port of the high-speed hub
  * '0' if the device is not low or full speed.
  */
-#define TT_PORT			(0xff << 8)
+#define TT_PORT(p)		(((p) & 0xff) << 8)
 #define TT_THINK_TIME(p)	(((p) & 0x3) << 16)
 
 /* dev_state bitmasks */
-- 
2.9.2

[U-Boot] [PATCH 15/16] usb: xhci: Correct TT_SLOT and TT_PORT macros

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> These two macros really need a parameter to make them useful.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/usb/host/xhci.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

[U-Boot] [PATCH 16/16] usb: xhci: Enable TT to support LS/FS devices behind a HS hub

[Bin Meng]

So far LS/FS devices directly attached to xHC root port can be
successfully enumerated by xHCI driver, but if they are connected
behind a hub, the enumeration process fails to address the device.

It turns out xHCI driver still misses a part that in the device's
input slot context, all Transaction Translator (TT) related fields
are not programmed. The xHCI spec defines how to enable TT.

Now LS/FS devices like USB keyboard/mouse can be enumerated behind
a high speed hub.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/usb/host/xhci-mem.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 03874db..bac2cee 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -771,6 +771,20 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
 		BUG();
 	}
 
+#ifdef CONFIG_DM_USB
+	/* Set up TT fields to support FS/LS devices */
+	if (speed == USB_SPEED_LOW || speed == USB_SPEED_FULL) {
+		dev = dev_get_parent_priv(udev->dev);
+		if (dev->speed == USB_SPEED_HIGH) {
+			hub = dev_get_uclass_priv(udev->dev);
+			if (hub->tt.multi)
+				slot_ctx->dev_info |= cpu_to_le32(DEV_MTT);
+			slot_ctx->tt_info |= cpu_to_le32(TT_PORT(udev->portnr));
+			slot_ctx->tt_info |= cpu_to_le32(TT_SLOT(dev->slot_id));
+		}
+	}
+#endif
+
 	port_num = hop_portnr;
 	debug("port_num = %d\n", port_num);
 
-- 
2.9.2

[U-Boot] [PATCH 16/16] usb: xhci: Enable TT to support LS/FS devices behind a HS hub

[Simon Glass]

On 23 June 2017 at 03:54, Bin Meng <bmeng.cn@gmail.com> wrote:
> So far LS/FS devices directly attached to xHC root port can be
> successfully enumerated by xHCI driver, but if they are connected
> behind a hub, the enumeration process fails to address the device.
>
> It turns out xHCI driver still misses a part that in the device's
> input slot context, all Transaction Translator (TT) related fields
> are not programmed. The xHCI spec defines how to enable TT.
>
> Now LS/FS devices like USB keyboard/mouse can be enumerated behind
> a high speed hub.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/usb/host/xhci-mem.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

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