[PATCH v5 1/2] usb: provide a device tree node to USB devices

[PATCH v5 1/2] usb: provide a device tree node to USB devices

[Michael Walle]

It is possible to specify a device tree node for an USB device. This is
useful if you have a static USB setup and want to use aliases which
point to these nodes, like on the Raspberry Pi.
The nodes are matched against their hub port number, the compatible
strings are not matched for now.

Signed-off-by: Michael Walle <michael@walle.cc>
---
This is a new patch in v5:
  Fixes the ethernet0 alias on Raspberry Pis. This has never been
  working, but wasn't a problem until recently. Patch 2/2 changes
  the allocation of the numbers and reserves possible aliases.

 drivers/usb/host/usb-uclass.c | 41 ++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index cb79dfbbd5..f42c0625cb 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -494,6 +494,35 @@ static int usb_match_one_id(struct usb_device_descriptor *desc,
 	return usb_match_one_id_intf(desc, int_desc, id);
 }
 
+static ofnode usb_get_ofnode(struct udevice *hub, int port)
+{
+	ofnode node;
+	u32 reg;
+
+	if (!dev_has_of_node(hub))
+		return ofnode_null();
+
+	/*
+	 * The USB controller and its USB hub are two different udevices,
+	 * but the device tree has only one node for both. Thus we are
+	 * assigning this node to both udevices.
+	 * If port is zero, the controller scans its root hub, thus we
+	 * are using the same ofnode as the controller here.
+	 */
+	if (!port)
+		return dev_ofnode(hub);
+
+	ofnode_for_each_subnode(node, dev_ofnode(hub)) {
+		if (ofnode_read_u32(node, "reg", &reg))
+			continue;
+
+		if (reg == port)
+			return node;
+	}
+
+	return ofnode_null();
+}
+
 /**
  * usb_find_and_bind_driver() - Find and bind the right USB driver
  *
@@ -502,13 +531,14 @@ static int usb_match_one_id(struct usb_device_descriptor *desc,
 static int usb_find_and_bind_driver(struct udevice *parent,
 				    struct usb_device_descriptor *desc,
 				    struct usb_interface_descriptor *iface,
-				    int bus_seq, int devnum,
+				    int bus_seq, int devnum, int port,
 				    struct udevice **devp)
 {
 	struct usb_driver_entry *start, *entry;
 	int n_ents;
 	int ret;
 	char name[30], *str;
+	ofnode node = usb_get_ofnode(parent, port);
 
 	*devp = NULL;
 	debug("%s: Searching for driver\n", __func__);
@@ -533,8 +563,8 @@ static int usb_find_and_bind_driver(struct udevice *parent,
 			 * find another driver. For now this doesn't seem
 			 * necesssary, so just bind the first match.
 			 */
-			ret = device_bind(parent, drv, drv->name, NULL, -1,
-					  &dev);
+			ret = device_bind_ofnode(parent, drv, drv->name, NULL,
+						 node, &dev);
 			if (ret)
 				goto error;
 			debug("%s: Match found: %s\n", __func__, drv->name);
@@ -651,9 +681,10 @@ int usb_scan_device(struct udevice *parent, int port,
 	if (ret) {
 		if (ret != -ENOENT)
 			return ret;
-		ret = usb_find_and_bind_driver(parent, &udev->descriptor, iface,
+		ret = usb_find_and_bind_driver(parent, &udev->descriptor,
+					       iface,
 					       udev->controller_dev->seq,
-					       udev->devnum, &dev);
+					       udev->devnum, port, &dev);
 		if (ret)
 			return ret;
 		created = true;
-- 
2.20.1

[PATCH v5 2/2] dm: uclass: don't assign aliased seq numbers

[Michael Walle]

If there are aliases for an uclass, set the base for the "dynamically"
allocated numbers next to the highest alias.

Please note, that this might lead to holes in the sequences, depending
on the device tree. For example if there is only an alias "ethernet1",
the next device seq number would be 2.

In particular this fixes a problem with boards which are using ethernet
aliases but also might have network add-in cards like the E1000. If the
board is started with the add-in card and depending on the order of the
drivers, the E1000 might occupy the first ethernet device and mess up
all the hardware addresses, because the devices are now shifted by one.

Also adapt the test cases to the new handling and add test cases
checking the holes in the seq numbers.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com>
Tested-by: Alex Marginean <alexandru.marginean@nxp.com>
Acked-by: Vladimir Oltean <olteanv@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Michal Simek <michal.simek@xilinx.com> [on zcu102-revA]
---
 arch/sandbox/dts/test.dts |  4 ++--
 drivers/core/uclass.c     | 21 +++++++++++++++------
 include/configs/sandbox.h |  6 +++---
 test/dm/eth.c             | 14 +++++++-------
 test/dm/test-fdt.c        | 22 +++++++++++++++++-----
 5 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 5ce5e28476..37dbd86017 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -23,8 +23,8 @@
 		pci0 = &pci0;
 		pci1 = &pci1;
 		pci2 = &pci2;
-		remoteproc1 = &rproc_1;
-		remoteproc2 = &rproc_2;
+		remoteproc0 = &rproc_1;
+		remoteproc1 = &rproc_2;
 		rtc0 = &rtc_0;
 		rtc1 = &rtc_1;
 		spi0 = "/spi at 0";
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index 2ab419cfe4..c3f1b73cd6 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -689,13 +689,14 @@ int uclass_unbind_device(struct udevice *dev)
 
 int uclass_resolve_seq(struct udevice *dev)
 {
+	struct uclass *uc = dev->uclass;
+	struct uclass_driver *uc_drv = uc->uc_drv;
 	struct udevice *dup;
-	int seq;
+	int seq = 0;
 	int ret;
 
 	assert(dev->seq == -1);
-	ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, dev->req_seq,
-					false, &dup);
+	ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false, &dup);
 	if (!ret) {
 		dm_warn("Device '%s': seq %d is in use by '%s'\n",
 			dev->name, dev->req_seq, dup->name);
@@ -707,9 +708,17 @@ int uclass_resolve_seq(struct udevice *dev)
 		return ret;
 	}
 
-	for (seq = 0; seq < DM_MAX_SEQ; seq++) {
-		ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq,
-						false, &dup);
+	if (CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(DM_SEQ_ALIAS) &&
+	    (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) {
+		/*
+		 * dev_read_alias_highest_id() will return -1 if there no
+		 * alias. Thus we can always add one.
+		 */
+		seq = dev_read_alias_highest_id(uc_drv->name) + 1;
+	}
+
+	for (; seq < DM_MAX_SEQ; seq++) {
+		ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup);
 		if (ret == -ENODEV)
 			break;
 		if (ret)
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 7fda63f71a..4549c81169 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -95,9 +95,9 @@
 #endif
 
 #define SANDBOX_ETH_SETTINGS		"ethaddr=00:00:11:22:33:44\0" \
-					"eth1addr=00:00:11:22:33:45\0" \
-					"eth3addr=00:00:11:22:33:46\0" \
-					"eth5addr=00:00:11:22:33:47\0" \
+					"eth3addr=00:00:11:22:33:45\0" \
+					"eth5addr=00:00:11:22:33:46\0" \
+					"eth6addr=00:00:11:22:33:47\0" \
 					"ipaddr=1.2.3.4\0"
 
 #define MEM_LAYOUT_ENV_SETTINGS \
diff --git a/test/dm/eth.c b/test/dm/eth.c
index 1fddcaabb0..b58c9640a2 100644
--- a/test/dm/eth.c
+++ b/test/dm/eth.c
@@ -48,7 +48,7 @@ static int dm_test_eth_alias(struct unit_test_state *uts)
 	ut_assertok(net_loop(PING));
 	ut_asserteq_str("eth at 10002000", env_get("ethact"));
 
-	env_set("ethact", "eth1");
+	env_set("ethact", "eth6");
 	ut_assertok(net_loop(PING));
 	ut_asserteq_str("eth at 10004000", env_get("ethact"));
 
@@ -105,7 +105,7 @@ static int dm_test_eth_act(struct unit_test_state *uts)
 	const char *ethname[DM_TEST_ETH_NUM] = {"eth at 10002000", "eth at 10003000",
 						"sbe5", "eth at 10004000"};
 	const char *addrname[DM_TEST_ETH_NUM] = {"ethaddr", "eth5addr",
-						 "eth3addr", "eth1addr"};
+						 "eth3addr", "eth6addr"};
 	char ethaddr[DM_TEST_ETH_NUM][18];
 	int i;
 
@@ -188,15 +188,15 @@ static int dm_test_eth_rotate(struct unit_test_state *uts)
 
 	/* Invalidate eth1's MAC address */
 	memset(ethaddr, '\0', sizeof(ethaddr));
-	strncpy(ethaddr, env_get("eth1addr"), 17);
-	/* Must disable access protection for eth1addr before clearing */
-	env_set(".flags", "eth1addr");
-	env_set("eth1addr", NULL);
+	strncpy(ethaddr, env_get("eth6addr"), 17);
+	/* Must disable access protection for eth6addr before clearing */
+	env_set(".flags", "eth6addr");
+	env_set("eth6addr", NULL);
 
 	retval = _dm_test_eth_rotate1(uts);
 
 	/* Restore the env */
-	env_set("eth1addr", ethaddr);
+	env_set("eth6addr", ethaddr);
 	env_set("ethrotate", NULL);
 
 	if (!retval) {
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index 4fcae03dc5..51f2547409 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -361,20 +361,32 @@ static int dm_test_fdt_uclass_seq(struct unit_test_state *uts)
 	ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 2, &dev));
 	ut_asserteq_str("d-test", dev->name);
 
-	/* d-test actually gets 0 */
-	ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 0, &dev));
+	/*
+	 * d-test actually gets 9, because thats the next free one after the
+	 * aliases.
+	 */
+	ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 9, &dev));
 	ut_asserteq_str("d-test", dev->name);
 
-	/* initially no one wants seq 1 */
-	ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 1,
+	/* initially no one wants seq 10 */
+	ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 10,
 						      &dev));
 	ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev));
 	ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 4, &dev));
 
 	/* But now that it is probed, we can find it */
-	ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 1, &dev));
+	ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 10, &dev));
 	ut_asserteq_str("f-test", dev->name);
 
+	/*
+	 * And we should still have holes in our sequence numbers, that is 2
+	 * and 4 should not be used.
+	 */
+	ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 2,
+						       true, &dev));
+	ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 4,
+						       true, &dev));
+
 	return 0;
 }
 DM_TEST(dm_test_fdt_uclass_seq, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
-- 
2.20.1

[PATCH v5 1/2] usb: provide a device tree node to USB devices

[Marek Vasut]

On 5/20/20 6:40 PM, Michael Walle wrote:
[...]
>  /**
>   * usb_find_and_bind_driver() - Find and bind the right USB driver
>   *
> @@ -502,13 +531,14 @@ static int usb_match_one_id(struct usb_device_descriptor *desc,
>  static int usb_find_and_bind_driver(struct udevice *parent,
>  				    struct usb_device_descriptor *desc,
>  				    struct usb_interface_descriptor *iface,
> -				    int bus_seq, int devnum,
> +				    int bus_seq, int devnum, int port,
>  				    struct udevice **devp)

Do we really need all these parameters passed to this function ? Can't
we somehow infer the port number from one of them OR isn't there some
structure we can already pass in it reduce the number of parameters ? It
feels there is way too many of them.

The patch looks good though, this is more of a general design question.

Reviewed-by: Marek Vasut <marex@denx.de>

[PATCH v5 1/2] usb: provide a device tree node to USB devices

[Bin Meng]

On Thu, May 21, 2020 at 12:40 AM Michael Walle <michael@walle.cc> wrote:
>
> It is possible to specify a device tree node for an USB device. This is
> useful if you have a static USB setup and want to use aliases which
> point to these nodes, like on the Raspberry Pi.
> The nodes are matched against their hub port number, the compatible
> strings are not matched for now.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> This is a new patch in v5:
>   Fixes the ethernet0 alias on Raspberry Pis. This has never been
>   working, but wasn't a problem until recently. Patch 2/2 changes
>   the allocation of the numbers and reserves possible aliases.
>
>  drivers/usb/host/usb-uclass.c | 41 ++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index cb79dfbbd5..f42c0625cb 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -494,6 +494,35 @@ static int usb_match_one_id(struct usb_device_descriptor *desc,
>         return usb_match_one_id_intf(desc, int_desc, id);
>  }
>
> +static ofnode usb_get_ofnode(struct udevice *hub, int port)
> +{
> +       ofnode node;
> +       u32 reg;
> +
> +       if (!dev_has_of_node(hub))
> +               return ofnode_null();
> +
> +       /*
> +        * The USB controller and its USB hub are two different udevices,
> +        * but the device tree has only one node for both. Thus we are
> +        * assigning this node to both udevices.
> +        * If port is zero, the controller scans its root hub, thus we
> +        * are using the same ofnode as the controller here.
> +        */
> +       if (!port)
> +               return dev_ofnode(hub);
> +
> +       ofnode_for_each_subnode(node, dev_ofnode(hub)) {
> +               if (ofnode_read_u32(node, "reg", &reg))
> +                       continue;
> +
> +               if (reg == port)
> +                       return node;
> +       }
> +
> +       return ofnode_null();
> +}
> +
>  /**
>   * usb_find_and_bind_driver() - Find and bind the right USB driver
>   *
> @@ -502,13 +531,14 @@ static int usb_match_one_id(struct usb_device_descriptor *desc,
>  static int usb_find_and_bind_driver(struct udevice *parent,
>                                     struct usb_device_descriptor *desc,
>                                     struct usb_interface_descriptor *iface,
> -                                   int bus_seq, int devnum,
> +                                   int bus_seq, int devnum, int port,
>                                     struct udevice **devp)
>  {
>         struct usb_driver_entry *start, *entry;
>         int n_ents;
>         int ret;
>         char name[30], *str;
> +       ofnode node = usb_get_ofnode(parent, port);
>
>         *devp = NULL;
>         debug("%s: Searching for driver\n", __func__);
> @@ -533,8 +563,8 @@ static int usb_find_and_bind_driver(struct udevice *parent,
>                          * find another driver. For now this doesn't seem
>                          * necesssary, so just bind the first match.
>                          */
> -                       ret = device_bind(parent, drv, drv->name, NULL, -1,
> -                                         &dev);
> +                       ret = device_bind_ofnode(parent, drv, drv->name, NULL,
> +                                                node, &dev);
>                         if (ret)
>                                 goto error;
>                         debug("%s: Match found: %s\n", __func__, drv->name);
> @@ -651,9 +681,10 @@ int usb_scan_device(struct udevice *parent, int port,
>         if (ret) {
>                 if (ret != -ENOENT)
>                         return ret;
> -               ret = usb_find_and_bind_driver(parent, &udev->descriptor, iface,
> +               ret = usb_find_and_bind_driver(parent, &udev->descriptor,
> +                                              iface,
>                                                udev->controller_dev->seq,
> -                                              udev->devnum, &dev);
> +                                              udev->devnum, port, &dev);
>                 if (ret)
>                         return ret;
>                 created = true;
> --

Do we have tests added ?

Regards,
Bin

[PATCH v5 1/2] usb: provide a device tree node to USB devices

[Michael Walle]

Am 2020-05-21 16:13, schrieb Bin Meng:
> On Thu, May 21, 2020 at 12:40 AM Michael Walle <michael@walle.cc> 
> wrote:
>> 
>> It is possible to specify a device tree node for an USB device. This 
>> is
>> useful if you have a static USB setup and want to use aliases which
>> point to these nodes, like on the Raspberry Pi.
>> The nodes are matched against their hub port number, the compatible
>> strings are not matched for now.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> This is a new patch in v5:
>>   Fixes the ethernet0 alias on Raspberry Pis. This has never been
>>   working, but wasn't a problem until recently. Patch 2/2 changes
>>   the allocation of the numbers and reserves possible aliases.
>> 
>>  drivers/usb/host/usb-uclass.c | 41 
>> ++++++++++++++++++++++++++++++-----
>>  1 file changed, 36 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/usb/host/usb-uclass.c 
>> b/drivers/usb/host/usb-uclass.c
>> index cb79dfbbd5..f42c0625cb 100644
>> --- a/drivers/usb/host/usb-uclass.c
>> +++ b/drivers/usb/host/usb-uclass.c
>> @@ -494,6 +494,35 @@ static int usb_match_one_id(struct 
>> usb_device_descriptor *desc,
>>         return usb_match_one_id_intf(desc, int_desc, id);
>>  }
>> 
>> +static ofnode usb_get_ofnode(struct udevice *hub, int port)
>> +{
>> +       ofnode node;
>> +       u32 reg;
>> +
>> +       if (!dev_has_of_node(hub))
>> +               return ofnode_null();
>> +
>> +       /*
>> +        * The USB controller and its USB hub are two different 
>> udevices,
>> +        * but the device tree has only one node for both. Thus we are
>> +        * assigning this node to both udevices.
>> +        * If port is zero, the controller scans its root hub, thus we
>> +        * are using the same ofnode as the controller here.
>> +        */
>> +       if (!port)
>> +               return dev_ofnode(hub);
>> +
>> +       ofnode_for_each_subnode(node, dev_ofnode(hub)) {
>> +               if (ofnode_read_u32(node, "reg", &reg))
>> +                       continue;
>> +
>> +               if (reg == port)
>> +                       return node;
>> +       }
>> +
>> +       return ofnode_null();
>> +}
>> +
>>  /**
>>   * usb_find_and_bind_driver() - Find and bind the right USB driver
>>   *
>> @@ -502,13 +531,14 @@ static int usb_match_one_id(struct 
>> usb_device_descriptor *desc,
>>  static int usb_find_and_bind_driver(struct udevice *parent,
>>                                     struct usb_device_descriptor 
>> *desc,
>>                                     struct usb_interface_descriptor 
>> *iface,
>> -                                   int bus_seq, int devnum,
>> +                                   int bus_seq, int devnum, int port,
>>                                     struct udevice **devp)
>>  {
>>         struct usb_driver_entry *start, *entry;
>>         int n_ents;
>>         int ret;
>>         char name[30], *str;
>> +       ofnode node = usb_get_ofnode(parent, port);
>> 
>>         *devp = NULL;
>>         debug("%s: Searching for driver\n", __func__);
>> @@ -533,8 +563,8 @@ static int usb_find_and_bind_driver(struct udevice 
>> *parent,
>>                          * find another driver. For now this doesn't 
>> seem
>>                          * necesssary, so just bind the first match.
>>                          */
>> -                       ret = device_bind(parent, drv, drv->name, 
>> NULL, -1,
>> -                                         &dev);
>> +                       ret = device_bind_ofnode(parent, drv, 
>> drv->name, NULL,
>> +                                                node, &dev);
>>                         if (ret)
>>                                 goto error;
>>                         debug("%s: Match found: %s\n", __func__, 
>> drv->name);
>> @@ -651,9 +681,10 @@ int usb_scan_device(struct udevice *parent, int 
>> port,
>>         if (ret) {
>>                 if (ret != -ENOENT)
>>                         return ret;
>> -               ret = usb_find_and_bind_driver(parent, 
>> &udev->descriptor, iface,
>> +               ret = usb_find_and_bind_driver(parent, 
>> &udev->descriptor,
>> +                                              iface,
>>                                                
>> udev->controller_dev->seq,
>> -                                              udev->devnum, &dev);
>> +                                              udev->devnum, port, 
>> &dev);
>>                 if (ret)
>>                         return ret;
>>                 created = true;
>> --
> 
> Do we have tests added ?

Adding tests for this isn't straight forward. Mostly because the device 
tree
is used to add the emulated USB devices. OTOH we try to match the device 
tree
to the scanned devices. To make things worse, the hierarchy of the USB 
hubs
and usb devices doesn't seem to fit a "normal" device tree.

Eg. in sandbox/dts/test.dts it is:
usb at 1 {
   /* this is the controller */
   hub {
     /* I don't know what this is */
     hub-emul {
       /* this is the root hub */
       flash-stick at 0 {
         reg = <0>;
         /* this is an usb device on port _1_ of the root hub */
       };
     };
   };
};

On a real device tree (eg. the raspberry pi one) it is:
usb {
   /* this is the controller & root hub */
   usb1 at 1 {
     /* this is another _external_ hub on port 1 of the root hub */
     reg = <1>;
     usbether at 1 {
       /* this is an usb device on port 1 of the external hub */
       reg = <1>;
     };
   };
};

They don't match at all. Thus the new code won't match any device
of the emulated usb hierarchy.

-michael

[PATCH v5 1/2] usb: provide a device tree node to USB devices

[Simon Glass]

Hi Michael,

On Thu, 21 May 2020 at 17:28, Michael Walle <michael@walle.cc> wrote:
>
> Am 2020-05-21 16:13, schrieb Bin Meng:
> > On Thu, May 21, 2020 at 12:40 AM Michael Walle <michael@walle.cc>
> > wrote:
> >>
> >> It is possible to specify a device tree node for an USB device. This
> >> is
> >> useful if you have a static USB setup and want to use aliases which
> >> point to these nodes, like on the Raspberry Pi.
> >> The nodes are matched against their hub port number, the compatible
> >> strings are not matched for now.
> >>
> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >> ---
> >> This is a new patch in v5:
> >>   Fixes the ethernet0 alias on Raspberry Pis. This has never been
> >>   working, but wasn't a problem until recently. Patch 2/2 changes
> >>   the allocation of the numbers and reserves possible aliases.
> >>
> >>  drivers/usb/host/usb-uclass.c | 41
> >> ++++++++++++++++++++++++++++++-----
> >>  1 file changed, 36 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/usb-uclass.c
> >> b/drivers/usb/host/usb-uclass.c
> >> index cb79dfbbd5..f42c0625cb 100644
> >> --- a/drivers/usb/host/usb-uclass.c
> >> +++ b/drivers/usb/host/usb-uclass.c
> >> @@ -494,6 +494,35 @@ static int usb_match_one_id(struct
> >> usb_device_descriptor *desc,
> >>         return usb_match_one_id_intf(desc, int_desc, id);
> >>  }
> >>
> >> +static ofnode usb_get_ofnode(struct udevice *hub, int port)
> >> +{
> >> +       ofnode node;
> >> +       u32 reg;
> >> +
> >> +       if (!dev_has_of_node(hub))
> >> +               return ofnode_null();
> >> +
> >> +       /*
> >> +        * The USB controller and its USB hub are two different
> >> udevices,
> >> +        * but the device tree has only one node for both. Thus we are
> >> +        * assigning this node to both udevices.
> >> +        * If port is zero, the controller scans its root hub, thus we
> >> +        * are using the same ofnode as the controller here.
> >> +        */
> >> +       if (!port)
> >> +               return dev_ofnode(hub);
> >> +
> >> +       ofnode_for_each_subnode(node, dev_ofnode(hub)) {
> >> +               if (ofnode_read_u32(node, "reg", &reg))
> >> +                       continue;
> >> +
> >> +               if (reg == port)
> >> +                       return node;
> >> +       }
> >> +
> >> +       return ofnode_null();
> >> +}
> >> +
> >>  /**
> >>   * usb_find_and_bind_driver() - Find and bind the right USB driver
> >>   *
> >> @@ -502,13 +531,14 @@ static int usb_match_one_id(struct
> >> usb_device_descriptor *desc,
> >>  static int usb_find_and_bind_driver(struct udevice *parent,
> >>                                     struct usb_device_descriptor
> >> *desc,
> >>                                     struct usb_interface_descriptor
> >> *iface,
> >> -                                   int bus_seq, int devnum,
> >> +                                   int bus_seq, int devnum, int port,
> >>                                     struct udevice **devp)
> >>  {
> >>         struct usb_driver_entry *start, *entry;
> >>         int n_ents;
> >>         int ret;
> >>         char name[30], *str;
> >> +       ofnode node = usb_get_ofnode(parent, port);
> >>
> >>         *devp = NULL;
> >>         debug("%s: Searching for driver\n", __func__);
> >> @@ -533,8 +563,8 @@ static int usb_find_and_bind_driver(struct udevice
> >> *parent,
> >>                          * find another driver. For now this doesn't
> >> seem
> >>                          * necesssary, so just bind the first match.
> >>                          */
> >> -                       ret = device_bind(parent, drv, drv->name,
> >> NULL, -1,
> >> -                                         &dev);
> >> +                       ret = device_bind_ofnode(parent, drv,
> >> drv->name, NULL,
> >> +                                                node, &dev);
> >>                         if (ret)
> >>                                 goto error;
> >>                         debug("%s: Match found: %s\n", __func__,
> >> drv->name);
> >> @@ -651,9 +681,10 @@ int usb_scan_device(struct udevice *parent, int
> >> port,
> >>         if (ret) {
> >>                 if (ret != -ENOENT)
> >>                         return ret;
> >> -               ret = usb_find_and_bind_driver(parent,
> >> &udev->descriptor, iface,
> >> +               ret = usb_find_and_bind_driver(parent,
> >> &udev->descriptor,
> >> +                                              iface,
> >>
> >> udev->controller_dev->seq,
> >> -                                              udev->devnum, &dev);
> >> +                                              udev->devnum, port,
> >> &dev);
> >>                 if (ret)
> >>                         return ret;
> >>                 created = true;
> >> --
> >
> > Do we have tests added ?
>
> Adding tests for this isn't straight forward. Mostly because the device
> tree
> is used to add the emulated USB devices. OTOH we try to match the device
> tree
> to the scanned devices. To make things worse, the hierarchy of the USB
> hubs
> and usb devices doesn't seem to fit a "normal" device tree.

This is described in usb-info.rst

> Eg. in sandbox/dts/test.dts it is:
> usb at 1 {
>    /* this is the controller */
>    hub {
>      /* I don't know what this is */

The root hub

>      hub-emul {
>        /* this is the root hub */

No, this is the emulation driver. I is added so that the root hub works.

>        flash-stick at 0 {
>          reg = <0>;
>          /* this is an usb device on port _1_ of the root hub */

No, this is an emulation device. So when we probe the hub we will see
this emulation driver and create a device.


>        };
>      };
>    };
> };
>
> On a real device tree (eg. the raspberry pi one) it is:
> usb {
>    /* this is the controller & root hub */
>    usb1 at 1 {
>      /* this is another _external_ hub on port 1 of the root hub */
>      reg = <1>;
>      usbether at 1 {
>        /* this is an usb device on port 1 of the external hub */
>        reg = <1>;
>      };
>    };
> };
>
> They don't match at all. Thus the new code won't match any device
> of the emulated usb hierarchy.

If you ignore the emulation nodes, they match. The emulation nodes are
added so that there is actually something on the bus for your test
code to see.

REgards,
SImon

[PATCH v5 1/2] usb: provide a device tree node to USB devices

[Michael Walle]

Hi Simon,

Am 2020-05-22 04:34, schrieb Simon Glass:
> Hi Michael,
> 
> On Thu, 21 May 2020 at 17:28, Michael Walle <michael@walle.cc> wrote:
>> 
>> Am 2020-05-21 16:13, schrieb Bin Meng:
>> > On Thu, May 21, 2020 at 12:40 AM Michael Walle <michael@walle.cc>
>> > wrote:
>> >>
>> >> It is possible to specify a device tree node for an USB device. This
>> >> is
>> >> useful if you have a static USB setup and want to use aliases which
>> >> point to these nodes, like on the Raspberry Pi.
>> >> The nodes are matched against their hub port number, the compatible
>> >> strings are not matched for now.
>> >>
>> >> Signed-off-by: Michael Walle <michael@walle.cc>
>> >> ---
>> >> This is a new patch in v5:
>> >>   Fixes the ethernet0 alias on Raspberry Pis. This has never been
>> >>   working, but wasn't a problem until recently. Patch 2/2 changes
>> >>   the allocation of the numbers and reserves possible aliases.
>> >>
>> >>  drivers/usb/host/usb-uclass.c | 41
>> >> ++++++++++++++++++++++++++++++-----
>> >>  1 file changed, 36 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/host/usb-uclass.c
>> >> b/drivers/usb/host/usb-uclass.c
>> >> index cb79dfbbd5..f42c0625cb 100644
>> >> --- a/drivers/usb/host/usb-uclass.c
>> >> +++ b/drivers/usb/host/usb-uclass.c
>> >> @@ -494,6 +494,35 @@ static int usb_match_one_id(struct
>> >> usb_device_descriptor *desc,
>> >>         return usb_match_one_id_intf(desc, int_desc, id);
>> >>  }
>> >>
>> >> +static ofnode usb_get_ofnode(struct udevice *hub, int port)
>> >> +{
>> >> +       ofnode node;
>> >> +       u32 reg;
>> >> +
>> >> +       if (!dev_has_of_node(hub))
>> >> +               return ofnode_null();
>> >> +
>> >> +       /*
>> >> +        * The USB controller and its USB hub are two different
>> >> udevices,
>> >> +        * but the device tree has only one node for both. Thus we are
>> >> +        * assigning this node to both udevices.
>> >> +        * If port is zero, the controller scans its root hub, thus we
>> >> +        * are using the same ofnode as the controller here.
>> >> +        */
>> >> +       if (!port)
>> >> +               return dev_ofnode(hub);
>> >> +
>> >> +       ofnode_for_each_subnode(node, dev_ofnode(hub)) {
>> >> +               if (ofnode_read_u32(node, "reg", &reg))
>> >> +                       continue;
>> >> +
>> >> +               if (reg == port)
>> >> +                       return node;
>> >> +       }
>> >> +
>> >> +       return ofnode_null();
>> >> +}
>> >> +
>> >>  /**
>> >>   * usb_find_and_bind_driver() - Find and bind the right USB driver
>> >>   *
>> >> @@ -502,13 +531,14 @@ static int usb_match_one_id(struct
>> >> usb_device_descriptor *desc,
>> >>  static int usb_find_and_bind_driver(struct udevice *parent,
>> >>                                     struct usb_device_descriptor
>> >> *desc,
>> >>                                     struct usb_interface_descriptor
>> >> *iface,
>> >> -                                   int bus_seq, int devnum,
>> >> +                                   int bus_seq, int devnum, int port,
>> >>                                     struct udevice **devp)
>> >>  {
>> >>         struct usb_driver_entry *start, *entry;
>> >>         int n_ents;
>> >>         int ret;
>> >>         char name[30], *str;
>> >> +       ofnode node = usb_get_ofnode(parent, port);
>> >>
>> >>         *devp = NULL;
>> >>         debug("%s: Searching for driver\n", __func__);
>> >> @@ -533,8 +563,8 @@ static int usb_find_and_bind_driver(struct udevice
>> >> *parent,
>> >>                          * find another driver. For now this doesn't
>> >> seem
>> >>                          * necesssary, so just bind the first match.
>> >>                          */
>> >> -                       ret = device_bind(parent, drv, drv->name,
>> >> NULL, -1,
>> >> -                                         &dev);
>> >> +                       ret = device_bind_ofnode(parent, drv,
>> >> drv->name, NULL,
>> >> +                                                node, &dev);
>> >>                         if (ret)
>> >>                                 goto error;
>> >>                         debug("%s: Match found: %s\n", __func__,
>> >> drv->name);
>> >> @@ -651,9 +681,10 @@ int usb_scan_device(struct udevice *parent, int
>> >> port,
>> >>         if (ret) {
>> >>                 if (ret != -ENOENT)
>> >>                         return ret;
>> >> -               ret = usb_find_and_bind_driver(parent,
>> >> &udev->descriptor, iface,
>> >> +               ret = usb_find_and_bind_driver(parent,
>> >> &udev->descriptor,
>> >> +                                              iface,
>> >>
>> >> udev->controller_dev->seq,
>> >> -                                              udev->devnum, &dev);
>> >> +                                              udev->devnum, port,
>> >> &dev);
>> >>                 if (ret)
>> >>                         return ret;
>> >>                 created = true;
>> >> --
>> >
>> > Do we have tests added ?
>> 
>> Adding tests for this isn't straight forward. Mostly because the 
>> device
>> tree
>> is used to add the emulated USB devices. OTOH we try to match the 
>> device
>> tree
>> to the scanned devices. To make things worse, the hierarchy of the USB
>> hubs
>> and usb devices doesn't seem to fit a "normal" device tree.
> 
> This is described in usb-info.rst
> 
>> Eg. in sandbox/dts/test.dts it is:
>> usb at 1 {
>>    /* this is the controller */
>>    hub {
>>      /* I don't know what this is */
> 
> The root hub
> 
>>      hub-emul {
>>        /* this is the root hub */
> 
> No, this is the emulation driver. I is added so that the root hub 
> works.
> 
>>        flash-stick at 0 {
>>          reg = <0>;
>>          /* this is an usb device on port _1_ of the root hub */
> 
> No, this is an emulation device. So when we probe the hub we will see
> this emulation driver and create a device.

Seems like my comments were missleading, they belong to the context
(i.e. the comment above) not the comment below. Let me rephrase:


usb at 1 /* this is the controller */
usb at 1/hub /* I don't know what this is */
usb at 1/hub/hub-emul /* this is the root hub */
usb at 1/hub/hub-emul/flash-stick at 0 /* this is an usb device on port _1_ of 
the root hub */

vs.

usb /* this is the controller & root hub */
usb/usb1 at 1 /* this is another _external_ hub on port 1 of the root hub 
*/
usb/usb1 at 1/usbether at 1 /* this is an usb device on port 1 of the external 
hub */

>> 
>> They don't match at all. Thus the new code won't match any device
>> of the emulated usb hierarchy.
> 
> If you ignore the emulation nodes, they match. The emulation nodes are
> added so that there is actually something on the bus for your test
> code to see.

How do they match? You'd also have to ignore the root hub, as this
is implicitly in the device controller. Additionally, port numbers
are zero based for the emulation stuff vs one-based in the real
device tree.

If I'm not mistaken, the equivalent to
   usb at 1/hub/hub-emul/flash-stick at 0
should be
   usb at 1/flash-stick at 1

-michael

[PATCH v5 1/2] usb: provide a device tree node to USB devices

[Simon Glass]

Hi Michael,

On Fri, 22 May 2020 at 01:32, Michael Walle <michael@walle.cc> wrote:
>
> Hi Simon,
>
> Am 2020-05-22 04:34, schrieb Simon Glass:
> > Hi Michael,
> >
> > On Thu, 21 May 2020 at 17:28, Michael Walle <michael@walle.cc> wrote:
> >>
> >> Am 2020-05-21 16:13, schrieb Bin Meng:
> >> > On Thu, May 21, 2020 at 12:40 AM Michael Walle <michael@walle.cc>
> >> > wrote:
> >> >>
> >> >> It is possible to specify a device tree node for an USB device. This
> >> >> is
> >> >> useful if you have a static USB setup and want to use aliases which
> >> >> point to these nodes, like on the Raspberry Pi.
> >> >> The nodes are matched against their hub port number, the compatible
> >> >> strings are not matched for now.
> >> >>
> >> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >> >> ---
> >> >> This is a new patch in v5:
> >> >>   Fixes the ethernet0 alias on Raspberry Pis. This has never been
> >> >>   working, but wasn't a problem until recently. Patch 2/2 changes
> >> >>   the allocation of the numbers and reserves possible aliases.
> >> >>
> >> >>  drivers/usb/host/usb-uclass.c | 41
> >> >> ++++++++++++++++++++++++++++++-----
> >> >>  1 file changed, 36 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/drivers/usb/host/usb-uclass.c
> >> >> b/drivers/usb/host/usb-uclass.c
> >> >> index cb79dfbbd5..f42c0625cb 100644
> >> >> --- a/drivers/usb/host/usb-uclass.c
> >> >> +++ b/drivers/usb/host/usb-uclass.c
> >> >> @@ -494,6 +494,35 @@ static int usb_match_one_id(struct
> >> >> usb_device_descriptor *desc,
> >> >>         return usb_match_one_id_intf(desc, int_desc, id);
> >> >>  }
> >> >>
> >> >> +static ofnode usb_get_ofnode(struct udevice *hub, int port)
> >> >> +{
> >> >> +       ofnode node;
> >> >> +       u32 reg;
> >> >> +
> >> >> +       if (!dev_has_of_node(hub))
> >> >> +               return ofnode_null();
> >> >> +
> >> >> +       /*
> >> >> +        * The USB controller and its USB hub are two different
> >> >> udevices,
> >> >> +        * but the device tree has only one node for both. Thus we are
> >> >> +        * assigning this node to both udevices.
> >> >> +        * If port is zero, the controller scans its root hub, thus we
> >> >> +        * are using the same ofnode as the controller here.
> >> >> +        */
> >> >> +       if (!port)
> >> >> +               return dev_ofnode(hub);
> >> >> +
> >> >> +       ofnode_for_each_subnode(node, dev_ofnode(hub)) {
> >> >> +               if (ofnode_read_u32(node, "reg", &reg))
> >> >> +                       continue;
> >> >> +
> >> >> +               if (reg == port)
> >> >> +                       return node;
> >> >> +       }
> >> >> +
> >> >> +       return ofnode_null();
> >> >> +}
> >> >> +
> >> >>  /**
> >> >>   * usb_find_and_bind_driver() - Find and bind the right USB driver
> >> >>   *
> >> >> @@ -502,13 +531,14 @@ static int usb_match_one_id(struct
> >> >> usb_device_descriptor *desc,
> >> >>  static int usb_find_and_bind_driver(struct udevice *parent,
> >> >>                                     struct usb_device_descriptor
> >> >> *desc,
> >> >>                                     struct usb_interface_descriptor
> >> >> *iface,
> >> >> -                                   int bus_seq, int devnum,
> >> >> +                                   int bus_seq, int devnum, int port,
> >> >>                                     struct udevice **devp)
> >> >>  {
> >> >>         struct usb_driver_entry *start, *entry;
> >> >>         int n_ents;
> >> >>         int ret;
> >> >>         char name[30], *str;
> >> >> +       ofnode node = usb_get_ofnode(parent, port);
> >> >>
> >> >>         *devp = NULL;
> >> >>         debug("%s: Searching for driver\n", __func__);
> >> >> @@ -533,8 +563,8 @@ static int usb_find_and_bind_driver(struct udevice
> >> >> *parent,
> >> >>                          * find another driver. For now this doesn't
> >> >> seem
> >> >>                          * necesssary, so just bind the first match.
> >> >>                          */
> >> >> -                       ret = device_bind(parent, drv, drv->name,
> >> >> NULL, -1,
> >> >> -                                         &dev);
> >> >> +                       ret = device_bind_ofnode(parent, drv,
> >> >> drv->name, NULL,
> >> >> +                                                node, &dev);
> >> >>                         if (ret)
> >> >>                                 goto error;
> >> >>                         debug("%s: Match found: %s\n", __func__,
> >> >> drv->name);
> >> >> @@ -651,9 +681,10 @@ int usb_scan_device(struct udevice *parent, int
> >> >> port,
> >> >>         if (ret) {
> >> >>                 if (ret != -ENOENT)
> >> >>                         return ret;
> >> >> -               ret = usb_find_and_bind_driver(parent,
> >> >> &udev->descriptor, iface,
> >> >> +               ret = usb_find_and_bind_driver(parent,
> >> >> &udev->descriptor,
> >> >> +                                              iface,
> >> >>
> >> >> udev->controller_dev->seq,
> >> >> -                                              udev->devnum, &dev);
> >> >> +                                              udev->devnum, port,
> >> >> &dev);
> >> >>                 if (ret)
> >> >>                         return ret;
> >> >>                 created = true;
> >> >> --
> >> >
> >> > Do we have tests added ?
> >>
> >> Adding tests for this isn't straight forward. Mostly because the
> >> device
> >> tree
> >> is used to add the emulated USB devices. OTOH we try to match the
> >> device
> >> tree
> >> to the scanned devices. To make things worse, the hierarchy of the USB
> >> hubs
> >> and usb devices doesn't seem to fit a "normal" device tree.
> >
> > This is described in usb-info.rst
> >
> >> Eg. in sandbox/dts/test.dts it is:
> >> usb at 1 {
> >>    /* this is the controller */
> >>    hub {
> >>      /* I don't know what this is */
> >
> > The root hub
> >
> >>      hub-emul {
> >>        /* this is the root hub */
> >
> > No, this is the emulation driver. I is added so that the root hub
> > works.
> >
> >>        flash-stick at 0 {
> >>          reg = <0>;
> >>          /* this is an usb device on port _1_ of the root hub */
> >
> > No, this is an emulation device. So when we probe the hub we will see
> > this emulation driver and create a device.
>
> Seems like my comments were missleading, they belong to the context
> (i.e. the comment above) not the comment below. Let me rephrase:
>
>
> usb at 1 /* this is the controller */
> usb at 1/hub /* I don't know what this is */
> usb at 1/hub/hub-emul /* this is the root hub */
> usb at 1/hub/hub-emul/flash-stick at 0 /* this is an usb device on port _1_ of
> the root hub */
>
> vs.
>
> usb /* this is the controller & root hub */
> usb/usb1 at 1 /* this is another _external_ hub on port 1 of the root hub
> */
> usb/usb1 at 1/usbether at 1 /* this is an usb device on port 1 of the external
> hub */
>
> >>
> >> They don't match at all. Thus the new code won't match any device
> >> of the emulated usb hierarchy.
> >
> > If you ignore the emulation nodes, they match. The emulation nodes are
> > added so that there is actually something on the bus for your test
> > code to see.
>
> How do they match? You'd also have to ignore the root hub, as this
> is implicitly in the device controller. Additionally, port numbers
> are zero based for the emulation stuff vs one-based in the real
> device tree.
>
> If I'm not mistaken, the equivalent to
>    usb at 1/hub/hub-emul/flash-stick at 0
> should be
>    usb at 1/flash-stick at 1

The device tree here is to specify the emulation nodes. If you try it:

u-boot -D
=> dm tree
..
 usb           0  [   ]   usb_sandbox           |-- usb at 1
 usb_hub       0  [   ]   usb_hub               |   `-- hub
 usb_emul      0  [   ]   usb_sandbox_hub       |       `-- hub-emul
 usb_emul      1  [   ]   usb_sandbox_flash     |           `-- flash-stick
..
=> usb start
...
=> dm tree
...
 usb           0  [ + ]   usb_sandbox           |-- usb at 1
 usb_hub       0  [ + ]   usb_hub               |   `-- hub
 usb_emul      0  [ + ]   usb_sandbox_hub       |       |-- hub-emul
 usb_emul      1  [ + ]   usb_sandbox_flash     |       |   `-- flash-stick
 usb_mass_s    0  [ + ]   usb_mass_storage      |       `-- usb_mass_storage
 blk           0  [   ]   usb_storage_blk       |           `--
usb_mass_storage.lun0
...
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 100mA)
  |  sandbox hub 2345
  |
  +-2  Mass Storage (480 Mb/s, 100mA)
       sandbox flash flash-stick


You can see that it creates the USB devices when the emulators respond
to requests about what is on the bus. That is the only reason the
emulators are there. The 'reg' properties are not related to the USB
port that is eventually assigned.

What you are doing is different. You are actually creating devices
that you know to exist on the bus. Sandbox should support this too
although I'm not sure if there is a test for it. That's the point of
the emulators with sandbox - to test the USB stack.

You can also use 'u-boot -T' to run with the test device tree. This
has 3 flash sticks and a fake USB keyboard.

Here's the docs I am referring to:

<<<
Sandbox
-------

All driver model uclasses must have tests and USB is no exception. To
achieve this, a sandbox USB controller is provided. This can make use of
emulation drivers which pretend to be USB devices. Emulations are provided
for a hub and a flash stick. These are enough to create a pretend USB bus
(defined by the sandbox device tree sandbox.dts) which can be scanned and
used.

Tests in test/dm/usb.c make use of this feature. It allows much of the USB
stack to be tested without real hardware being needed.

Here is an example device tree fragment:

.. code-block:: none

usb@1 {
compatible = "sandbox,usb";
hub {
compatible = "usb-hub";
usb,device-class = <USB_CLASS_HUB>;
hub-emul {
compatible = "sandbox,usb-hub";
#address-cells = <1>;
#size-cells = <0>;
flash-stick {
reg = <0>;
compatible = "sandbox,usb-flash";
sandbox,filepath = "flash.bin";
};
};
};
};

This defines a single controller, containing a root hub (which is required).
The hub is emulated by a hub emulator, and the emulated hub has a single
flash stick to emulate on one of its ports.

When 'usb start' is used, the following 'dm tree' output will be available::

   usb         [ + ]    `-- usb at 1
   usb_hub     [ + ]        `-- hub
   usb_emul    [ + ]            |-- hub-emul
   usb_emul    [ + ]            |   `-- flash-stick
   usb_mass_st [ + ]            `-- usb_mass_storage


This may look confusing. Most of it mirrors the device tree, but the
'usb_mass_storage' device is not in the device tree. This is created by
usb_find_and_bind_driver() based on the USB_DRIVER in usb_storage.c. While
'flash-stick' is the emulation device, 'usb_mass_storage' is the real U-Boot
USB device driver that talks to it.
>>>


Regards,
Simon