[U-Boot] [PATCH 0/5] RFC: usb: Drop requirement for USB unbinding

[U-Boot] [PATCH 0/5] RFC: usb: Drop requirement for USB unbinding

[Simon Glass]

There was quite a bit of discussion about the change that required the
unbinding of USB devices for the subsystem to function correctly. E.g.

https://patchwork.ozlabs.org/patch/485637/

The key issue is the usb_get_dev_index() function which is not a good API
for driver model. We can drop use of this function once everything is
converted to driver model. Then I believe the problems raised by Hans go
away. For now we can add a deprecation warning on the function.

It is easy to convert USB keyboards to driver model. This series includes
a patch for that.

This series also includes reverts for the three commits which as discussed
I would like to drop. U-Boot should be able to run normally and exit without
unbinding anything.

I cannot actually repeat the problem that Hans mentions in the above thread
so I may be missing a step. Hans, any ideas on this?


Simon Glass (5):
  Revert "dm: usb: Rename usb_find_child to usb_find_emul_child"
  Revert "dm: usb: Use device_unbind_children to clean up usb devs on
    stop"
  Revert "dm: Export device_remove_children / device_unbind_children"
  dm: usb: Add support for USB keyboards with driver model
  dm: usb: Deprecate usb_get_dev_index()

 common/cmd_usb.c              | 12 +++++-----
 common/usb_kbd.c              | 52 +++++++++++++++++++++++++++++++++++++++++--
 drivers/core/device-remove.c  | 22 ++++++++++++++----
 drivers/usb/host/usb-uclass.c | 31 ++++++++++++++++----------
 include/dm/device-internal.h  | 26 ----------------------
 include/dm/uclass-id.h        |  1 +
 6 files changed, 94 insertions(+), 50 deletions(-)

-- 
2.5.0.457.gab17608

[U-Boot] [PATCH 1/5] Revert "dm: usb: Rename usb_find_child to usb_find_emul_child"

[Simon Glass]

This reverts commit 9b510df703d282effba4f56ac567aa8011d56e6b.

We want to avoid having the USB stack rely on unbind.

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

 drivers/usb/host/usb-uclass.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index b17a7d7..3c45181 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -493,14 +493,15 @@ error:
 }
 
 /**
- * usb_find_emul_child() - Find an existing device for emulated devices
+ * usb_find_child() - Find an existing device which matches our needs
+ *
+ *
  */
-static int usb_find_emul_child(struct udevice *parent,
-			       struct usb_device_descriptor *desc,
-			       struct usb_interface_descriptor *iface,
-			       struct udevice **devp)
+static int usb_find_child(struct udevice *parent,
+			  struct usb_device_descriptor *desc,
+			  struct usb_interface_descriptor *iface,
+			  struct udevice **devp)
 {
-#ifdef CONFIG_SANDBOX
 	struct udevice *dev;
 
 	*devp = NULL;
@@ -519,7 +520,7 @@ static int usb_find_emul_child(struct udevice *parent,
 			return 0;
 		}
 	}
-#endif
+
 	return -ENOENT;
 }
 
@@ -579,8 +580,8 @@ int usb_scan_device(struct udevice *parent, int port,
 	debug("read_descriptor for '%s': ret=%d\n", parent->name, ret);
 	if (ret)
 		return ret;
-	ret = usb_find_emul_child(parent, &udev->descriptor, iface, &dev);
-	debug("** usb_find_emul_child returns %d\n", ret);
+	ret = usb_find_child(parent, &udev->descriptor, iface, &dev);
+	debug("** usb_find_child returns %d\n", ret);
 	if (ret) {
 		if (ret != -ENOENT)
 			return ret;
-- 
2.5.0.457.gab17608

[U-Boot] [PATCH 2/5] Revert "dm: usb: Use device_unbind_children to clean up usb devs on stop"

[Simon Glass]

This reverts commit 6cda369509e0d3fa5f9e33c9d71589c4523799fa.

We want to avoid having the USB stack rely on unbind.

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

 drivers/usb/host/usb-uclass.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 3c45181..ebe22d6 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -157,9 +157,6 @@ int usb_stop(void)
 		ret = device_remove(bus);
 		if (ret && !err)
 			err = ret;
-		ret = device_unbind_children(bus);
-		if (ret && !err)
-			err = ret;
 	}
 
 #ifdef CONFIG_SANDBOX
-- 
2.5.0.457.gab17608

[U-Boot] [PATCH 3/5] Revert "dm: Export device_remove_children / device_unbind_children"

[Simon Glass]

This reverts commit bb52b367f6ca4a3a918e77737f4ff6a1089912d9.

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

 drivers/core/device-remove.c | 22 ++++++++++++++++++----
 include/dm/device-internal.h | 26 --------------------------
 2 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index bd6d406..e1714b2 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -18,7 +18,16 @@
 #include <dm/uclass-internal.h>
 #include <dm/util.h>
 
-int device_unbind_children(struct udevice *dev)
+/**
+ * device_chld_unbind() - Unbind all device's children from the device
+ *
+ * On error, the function continues to unbind all children, and reports the
+ * first error.
+ *
+ * @dev:	The device that is to be stripped of its children
+ * @return 0 on success, -ve on error
+ */
+static int device_chld_unbind(struct udevice *dev)
 {
 	struct udevice *pos, *n;
 	int ret, saved_ret = 0;
@@ -34,7 +43,12 @@ int device_unbind_children(struct udevice *dev)
 	return saved_ret;
 }
 
-int device_remove_children(struct udevice *dev)
+/**
+ * device_chld_remove() - Stop all device's children
+ * @dev:	The device whose children are to be removed
+ * @return 0 on success, -ve on error
+ */
+static int device_chld_remove(struct udevice *dev)
 {
 	struct udevice *pos, *n;
 	int ret;
@@ -73,7 +87,7 @@ int device_unbind(struct udevice *dev)
 			return ret;
 	}
 
-	ret = device_unbind_children(dev);
+	ret = device_chld_unbind(dev);
 	if (ret)
 		return ret;
 
@@ -153,7 +167,7 @@ int device_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
-	ret = device_remove_children(dev);
+	ret = device_chld_remove(dev);
 	if (ret)
 		goto err;
 
diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
index 322d35a..9388870 100644
--- a/include/dm/device-internal.h
+++ b/include/dm/device-internal.h
@@ -107,32 +107,6 @@ int device_unbind(struct udevice *dev);
 static inline int device_unbind(struct udevice *dev) { return 0; }
 #endif
 
-/**
- * device_remove_children() - Stop all device's children
- * @dev:	The device whose children are to be removed
- * @return 0 on success, -ve on error
- */
-#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
-int device_remove_children(struct udevice *dev);
-#else
-static inline int device_remove_children(struct udevice *dev) { return 0; }
-#endif
-
-/**
- * device_unbind_children() - Unbind all device's children from the device
- *
- * On error, the function continues to unbind all children, and reports the
- * first error.
- *
- * @dev:	The device that is to be stripped of its children
- * @return 0 on success, -ve on error
- */
-#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
-int device_unbind_children(struct udevice *dev);
-#else
-static inline int device_unbind_children(struct udevice *dev) { return 0; }
-#endif
-
 #if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
 void device_free(struct udevice *dev);
 #else
-- 
2.5.0.457.gab17608

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Simon Glass]

Switch USB keyboards over to use driver model instead of scanning with the
horrible usb_get_dev_index() function. This involves creating a new uclass
for keyboards, although so far there is no API.

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

 common/cmd_usb.c       | 12 ++++++------
 common/usb_kbd.c       | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
 include/dm/uclass-id.h |  1 +
 3 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/common/cmd_usb.c b/common/cmd_usb.c
index 6874af7..665f8ec 100644
--- a/common/cmd_usb.c
+++ b/common/cmd_usb.c
@@ -526,11 +526,14 @@ static void do_usb_start(void)
 
 	/* Driver model will probe the devices as they are found */
 #ifndef CONFIG_DM_USB
-#ifdef CONFIG_USB_STORAGE
+# ifdef CONFIG_USB_STORAGE
 	/* try to recognize storage devices immediately */
 	usb_stor_curr_dev = usb_stor_scan(1);
-#endif
-#endif
+# endif
+# ifdef CONFIG_USB_KEYBOARD
+	drv_usb_kbd_init();
+# endif
+#endif /* !CONFIG_DM_USB */
 #ifdef CONFIG_USB_HOST_ETHER
 # ifdef CONFIG_DM_ETH
 #  ifndef CONFIG_DM_USB
@@ -541,9 +544,6 @@ static void do_usb_start(void)
 	usb_ether_curr_dev = usb_host_eth_scan(1);
 # endif
 #endif
-#ifdef CONFIG_USB_KEYBOARD
-	drv_usb_kbd_init();
-#endif
 }
 
 #ifdef CONFIG_DM_USB
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 0227024..8037ebf 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -398,7 +398,7 @@ static int usb_kbd_getc(struct stdio_dev *sdev)
 }
 
 /* probes the USB device dev for keyboard type. */
-static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
+static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
 {
 	struct usb_interface *iface;
 	struct usb_endpoint_descriptor *ep;
@@ -495,7 +495,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
 	int error;
 
 	/* Try probing the keyboard */
-	if (usb_kbd_probe(dev, 0) != 1)
+	if (usb_kbd_probe_dev(dev, 0) != 1)
 		return -ENOENT;
 
 	/* Register the keyboard */
@@ -532,6 +532,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
 	return 0;
 }
 
+#ifndef CONFIG_DM_USB
 /* Search for keyboard and register it if found. */
 int drv_usb_kbd_init(void)
 {
@@ -590,6 +591,7 @@ int drv_usb_kbd_init(void)
 	/* No USB Keyboard found */
 	return -1;
 }
+#endif
 
 /* Deregister the keyboard. */
 int usb_kbd_deregister(int force)
@@ -621,3 +623,49 @@ int usb_kbd_deregister(int force)
 	return 1;
 #endif
 }
+
+#ifdef CONFIG_DM_USB
+
+static int usb_kbd_probe(struct udevice *dev)
+{
+	struct usb_device *udev = dev_get_parentdata(dev);
+	int ret;
+
+	ret = probe_usb_keyboard(udev);
+
+	return ret;
+}
+
+static const struct udevice_id usb_kbd_ids[] = {
+	{ .compatible = "usb-keyboard" },
+	{ }
+};
+
+U_BOOT_DRIVER(usb_kbd) = {
+	.name	= "usb_kbd",
+	.id	= UCLASS_KEYBOARD,
+	.of_match = usb_kbd_ids,
+	.probe = usb_kbd_probe,
+};
+
+/* TODO(sjg at chromium.org): Move this into a common location */
+UCLASS_DRIVER(keyboard) = {
+	.id		= UCLASS_KEYBOARD,
+	.name		= "keyboard",
+};
+
+static const struct usb_device_id kbd_id_table[] = {
+	{
+		.match_flags = USB_DEVICE_ID_MATCH_INT_CLASS |
+			USB_DEVICE_ID_MATCH_INT_SUBCLASS |
+			USB_DEVICE_ID_MATCH_INT_PROTOCOL,
+		.bInterfaceClass = USB_CLASS_HID,
+		.bInterfaceSubClass = 1,
+		.bInterfaceProtocol = 1,
+	},
+	{ }		/* Terminating entry */
+};
+
+U_BOOT_USB_DEVICE(usb_kbd, kbd_id_table);
+
+#endif
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 1eeec74..bab0025 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -36,6 +36,7 @@ enum uclass_id {
 	UCLASS_I2C_EEPROM,	/* I2C EEPROM device */
 	UCLASS_I2C_GENERIC,	/* Generic I2C device */
 	UCLASS_I2C_MUX,		/* I2C multiplexer */
+	UCLASS_KEYBOARD,	/* Keyboard input device */
 	UCLASS_LED,		/* Light-emitting diode (LED) */
 	UCLASS_LPC,		/* x86 'low pin count' interface */
 	UCLASS_MASS_STORAGE,	/* Mass storage device */
-- 
2.5.0.457.gab17608

[U-Boot] [PATCH 5/5] dm: usb: Deprecate usb_get_dev_index()

[Simon Glass]

This function should not be used with driver model. While there are users
of USB Ethernet that use driver model for USB but not Ethernet, we have
to keep it around. Add a comment to that effect.

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

 drivers/usb/host/usb-uclass.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index ebe22d6..243e699 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -278,6 +278,14 @@ int usb_init(void)
 	return usb_started ? 0 : -1;
 }
 
+/*
+ * TODO(sjg at chromium.org): Remove this legacy function. At present it is needed
+ * to support boards which use driver model for USB but not Ethernet, and want
+ * to use USB Ethernet.
+ *
+ * The #if clause is here to ensure that remains the only case.
+ */
+#if !defined(CONFIG_DM_ETH) && defined(CONFIG_USB_HOST_ETHER)
 static struct usb_device *find_child_devnum(struct udevice *parent, int devnum)
 {
 	struct usb_device *udev;
@@ -311,6 +319,7 @@ struct usb_device *usb_get_dev_index(struct udevice *bus, int index)
 
 	return find_child_devnum(dev, devnum);
 }
+#endif
 
 int usb_post_bind(struct udevice *dev)
 {
-- 
2.5.0.457.gab17608

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Marek Vasut]

On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
> Switch USB keyboards over to use driver model instead of scanning with the
> horrible usb_get_dev_index() function. This involves creating a new uclass
> for keyboards, although so far there is no API.

Hi,

Why don't you create an UCLASS for generic input device instead ?

But in general, looks pretty standard/OK.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Simon Glass]

Hi Marek,

On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
> On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
>> Switch USB keyboards over to use driver model instead of scanning with the
>> horrible usb_get_dev_index() function. This involves creating a new uclass
>> for keyboards, although so far there is no API.
>
> Hi,
>
> Why don't you create an UCLASS for generic input device instead ?
>

I sent a series that does that later. My intent with this series is to
get something applied for this release.

> But in general, looks pretty standard/OK.

Regards,
Simon

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Marek Vasut]

On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
> > On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
> >> Switch USB keyboards over to use driver model instead of scanning with
> >> the horrible usb_get_dev_index() function. This involves creating a new
> >> uclass for keyboards, although so far there is no API.
> > 
> > Hi,
> > 
> > Why don't you create an UCLASS for generic input device instead ?
> 
> I sent a series that does that later. My intent with this series is to
> get something applied for this release.

Hi!

Aren't we pretty much post-RC3 now ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Simon Glass]

Hi Marek,

On 10 September 2015 at 04:40, Marek Vasut <marex@denx.de> wrote:
> On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
>> > On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
>> >> Switch USB keyboards over to use driver model instead of scanning with
>> >> the horrible usb_get_dev_index() function. This involves creating a new
>> >> uclass for keyboards, although so far there is no API.
>> >
>> > Hi,
>> >
>> > Why don't you create an UCLASS for generic input device instead ?
>>
>> I sent a series that does that later. My intent with this series is to
>> get something applied for this release.
>
> Hi!
>
> Aren't we pretty much post-RC3 now ?

Yes. It's not critical and I am late - let's see what Hans says.

Regards,
Simon

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Hans de Goede]

Hi,

On 09/11/2015 02:43 AM, Simon Glass wrote:
> Hi Marek,
>
> On 10 September 2015 at 04:40, Marek Vasut <marex@denx.de> wrote:
>> On Thursday, September 10, 2015 at 04:45:53 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 8 September 2015 at 12:33, Marek Vasut <marex@denx.de> wrote:
>>>> On Tuesday, September 08, 2015 at 07:15:11 PM, Simon Glass wrote:
>>>>> Switch USB keyboards over to use driver model instead of scanning with
>>>>> the horrible usb_get_dev_index() function. This involves creating a new
>>>>> uclass for keyboards, although so far there is no API.
>>>>
>>>> Hi,
>>>>
>>>> Why don't you create an UCLASS for generic input device instead ?
>>>
>>> I sent a series that does that later. My intent with this series is to
>>> get something applied for this release.
>>
>> Hi!
>>
>> Aren't we pretty much post-RC3 now ?
>
> Yes. It's not critical and I am late - let's see what Hans says.

I have looking into the RFC patchset on my todo, not sure if I will
get around to it this weekend though, and after that I'm travelling
for a week. Even if I get around to testing this I would prefer for
this to be delayed to post v2015.10. I'm fine with the concept of
the set but this needs some careful testing.

Regards,

Hans

[U-Boot] [PATCH 0/5] RFC: usb: Drop requirement for USB unbinding

[Hans de Goede]

Hi,

On 08-09-15 19:15, Simon Glass wrote:
> There was quite a bit of discussion about the change that required the
> unbinding of USB devices for the subsystem to function correctly. E.g.
>
> https://patchwork.ozlabs.org/patch/485637/
>
> The key issue is the usb_get_dev_index() function which is not a good API
> for driver model. We can drop use of this function once everything is
> converted to driver model. Then I believe the problems raised by Hans go
> away. For now we can add a deprecation warning on the function.
>
> It is easy to convert USB keyboards to driver model. This series includes
> a patch for that.
>
> This series also includes reverts for the three commits which as discussed
> I would like to drop. U-Boot should be able to run normally and exit without
> unbinding anything.
>
> I cannot actually repeat the problem that Hans mentions in the above thread
> so I may be missing a step. Hans, any ideas on this?

So I've just tried this series on an allwinner tablet which uses a musb
controller in host mode. Note that this has no root hub, so the first
child of the controller is an actual device not a root hub.

When I first plug in a usb keyboard directly into the otg port and then do
usb tree + dm tree (output shortened) I get:

USB device tree:
   1  Human Interface (1.5 Mb/s, 100mA)
      SINO WEALTH USB Composite Device

=> dm tree
  Class       Probed   Name
----------------------------------------
  usb         [ + ]    `-- sunxi-musb
  keyboard    [ + ]        `-- usb_kbd

If I then unplug the keyboard, plug in a hub and plug the
keyboard into the hub and do a usb reset I get:

=> usb tree
USB device tree:
=> dm tree
  Class       Probed   Name
----------------------------------------
  usb         [ + ]    `-- sunxi-musb
  keyboard    [   ]        |-- usb_kbd
  usb_hub     [ + ]        `-- usb_hub
  keyboard    [ + ]            `-- usb_kbd

Notice how the "usb tree" command output is
empty, that is because the usb tree code stops
at the first removed usb-device. As discussed before
if we go the route this patch-set is going we need
to teach *all* code iterating over devices to skip
removed devices, rather then to see a removed device
as the end of the list.

At least thanks to the conversion of the usb-kbd driver
to the dm the keyboard still works (which it did not in
the past). That conversion has issues of its own btw,
I will reply to that patch with my comments.

###

Related to the above (failed) test I believe that
the first set of this patch:

Revert "dm: usb: Rename usb_find_child to usb_find_emul_child"

Is wrong and is the beginning of various problems, last time
we discussed not making the usb code depend on the dm unbind
code you suggested to simply remove the devices, and not re-use
them ever (which means not using usb_find_child in non sandbox
builds).

I believe that this is the right approach, re-using them will
result in all kind of weirdness, let me give an example:

Plug in a hub, plug a keyb into port 1 and a storage device into
port 2, do "usb tree" :

=> usb tree
USB device tree:
   1  Hub (480 Mb/s, 100mA)
   |   USB2.0 Hub
   |
   +-2  Human Interface (1.5 Mb/s, 100mA)
   |    SINO WEALTH USB Composite Device
   |
   +-3  Mass Storage (480 Mb/s, 100mA)
        USB Flash Disk 4C0E960F

No swap the 2 devices, so usb storage into port 1, keyb into
port 2:

USB device tree:
   1  Hub (480 Mb/s, 100mA)
   |   USB2.0 Hub
   |
   +-3  Human Interface (1.5 Mb/s, 100mA)
   |    SINO WEALTH USB Composite Device
   |
   +-2  Mass Storage (480 Mb/s, 100mA)
        USB Flash Disk 4C0E960F

And here we see that the usb stack now scans the
storage-dev first and assigs it usb addr 2, but usb tree
shows it after the keyb because the existing dm-device
structs were re-used, and they appear out of order in
the list now making the tree output no longer an accurate
representation of the actual physical topology.

And if we add more levels of hubs etc, things will likely
only get worse, not better, possibly leading to non
cosmetic problems.


I believe that the way to make the dm usb code work
without dm-unbind is to simply keep the removed devices,
and make sure that all code going over the device tree
ignores these removed usb devices (with the exception
of core dm functions), and to NEVER re-use them.

That and in the case of not building without dm-unbind
actually call device_unbind_children(bus), iow instead
of reverting "dm: usb: Use device_unbind_children to
clean up usb devs on stop" wrap the device_unbind_children(bus)
call it adds in #ifdef CONFIG_DM_DEVICE_REMOVE

###

How ever we end up fixing this, I believe that this set
certainly is not ready for merging into v2015.10.

Regards,

Hans

[U-Boot] [PATCH 0/5] RFC: usb: Drop requirement for USB unbinding

[Simon Glass]

Hi Hans,

On 12 September 2015 at 08:00, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 08-09-15 19:15, Simon Glass wrote:
>>
>> There was quite a bit of discussion about the change that required the
>> unbinding of USB devices for the subsystem to function correctly. E.g.
>>
>> https://patchwork.ozlabs.org/patch/485637/
>>
>> The key issue is the usb_get_dev_index() function which is not a good API
>> for driver model. We can drop use of this function once everything is
>> converted to driver model. Then I believe the problems raised by Hans go
>> away. For now we can add a deprecation warning on the function.
>>
>> It is easy to convert USB keyboards to driver model. This series includes
>> a patch for that.
>>
>> This series also includes reverts for the three commits which as discussed
>> I would like to drop. U-Boot should be able to run normally and exit without
>> unbinding anything.
>>
>> I cannot actually repeat the problem that Hans mentions in the above thread
>> so I may be missing a step. Hans, any ideas on this?
>
>
> So I've just tried this series on an allwinner tablet which uses a musb
> controller in host mode. Note that this has no root hub, so the first
> child of the controller is an actual device not a root hub.
>
> When I first plug in a usb keyboard directly into the otg port and then do
> usb tree + dm tree (output shortened) I get:
>
> USB device tree:
>   1  Human Interface (1.5 Mb/s, 100mA)
>      SINO WEALTH USB Composite Device
>
> => dm tree
>  Class       Probed   Name
> ----------------------------------------
>  usb         [ + ]    `-- sunxi-musb
>  keyboard    [ + ]        `-- usb_kbd
>
> If I then unplug the keyboard, plug in a hub and plug the
> keyboard into the hub and do a usb reset I get:
>
> => usb tree
> USB device tree:
> => dm tree
>  Class       Probed   Name
> ----------------------------------------
>  usb         [ + ]    `-- sunxi-musb
>  keyboard    [   ]        |-- usb_kbd
>  usb_hub     [ + ]        `-- usb_hub
>  keyboard    [ + ]            `-- usb_kbd
>
> Notice how the "usb tree" command output is
> empty, that is because the usb tree code stops
> at the first removed usb-device. As discussed before
> if we go the route this patch-set is going we need
> to teach *all* code iterating over devices to skip
> removed devices, rather then to see a removed device
> as the end of the list.
>
> At least thanks to the conversion of the usb-kbd driver
> to the dm the keyboard still works (which it did not in
> the past). That conversion has issues of its own btw,
> I will reply to that patch with my comments.
>
> ###
>
> Related to the above (failed) test I believe that
> the first set of this patch:
>
> Revert "dm: usb: Rename usb_find_child to usb_find_emul_child"
>
> Is wrong and is the beginning of various problems, last time
> we discussed not making the usb code depend on the dm unbind
> code you suggested to simply remove the devices, and not re-use
> them ever (which means not using usb_find_child in non sandbox
> builds).
>
> I believe that this is the right approach, re-using them will
> result in all kind of weirdness, let me give an example:
>
> Plug in a hub, plug a keyb into port 1 and a storage device into
> port 2, do "usb tree" :
>
> => usb tree
> USB device tree:
>   1  Hub (480 Mb/s, 100mA)
>   |   USB2.0 Hub
>   |
>   +-2  Human Interface (1.5 Mb/s, 100mA)
>   |    SINO WEALTH USB Composite Device
>   |
>   +-3  Mass Storage (480 Mb/s, 100mA)
>        USB Flash Disk 4C0E960F
>
> No swap the 2 devices, so usb storage into port 1, keyb into
> port 2:
>
> USB device tree:
>   1  Hub (480 Mb/s, 100mA)
>   |   USB2.0 Hub
>   |
>   +-3  Human Interface (1.5 Mb/s, 100mA)
>   |    SINO WEALTH USB Composite Device
>   |
>   +-2  Mass Storage (480 Mb/s, 100mA)
>        USB Flash Disk 4C0E960F
>
> And here we see that the usb stack now scans the
> storage-dev first and assigs it usb addr 2, but usb tree
> shows it after the keyb because the existing dm-device
> structs were re-used, and they appear out of order in
> the list now making the tree output no longer an accurate
> representation of the actual physical topology.
>
> And if we add more levels of hubs etc, things will likely
> only get worse, not better, possibly leading to non
> cosmetic problems.
>
>
> I believe that the way to make the dm usb code work
> without dm-unbind is to simply keep the removed devices,
> and make sure that all code going over the device tree
> ignores these removed usb devices (with the exception
> of core dm functions), and to NEVER re-use them.
>
> That and in the case of not building without dm-unbind
> actually call device_unbind_children(bus), iow instead
> of reverting "dm: usb: Use device_unbind_children to
> clean up usb devs on stop" wrap the device_unbind_children(bus)
> call it adds in #ifdef CONFIG_DM_DEVICE_REMOVE
>
> ###
>
> How ever we end up fixing this, I believe that this set
> certainly is not ready for merging into v2015.10.

Thanks for looking at this. I'm sorry I didn't get to it before. But
your original patches fixed a bug so I was keen to to avoid holding
things up. Let's target the next release.

My main objective is to make the unbinding optional, rather than
required for USB to function.

I'll see if I can repeat your test case. It is the other way around
from what I was trying, and from how I understood your original bug
report: here you are plugging in a device, then removing it and
plugging it in via a hub. I was doing it the other way around.

I think the best approach may be to create a sandbox test which checks
this behaviour as well as the output of 'usb tree'.

Were you able to test the driver model USB keyboard support?

Regards,
Simon

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Hans de Goede]

Hi,

On 08-09-15 19:15, Simon Glass wrote:
> Switch USB keyboards over to use driver model instead of scanning with the
> horrible usb_get_dev_index() function. This involves creating a new uclass
> for keyboards, although so far there is no API.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

In general I like this patch, so ack for the principle, but the
implementation has issues.

You're now allowing the registration of multiple usb keyb stdio
input devices, but you are still relying on usb_kbd_deregister()
to remove them from the stdio devices list, and when multiple
or used that one will only remove the first one.

This can be fixed by switching to stdio_register_dev, and
store the returned struct stdio_dev pointer for the new dev,
and add a dm remove callback which deregisters that specific
stdio_dev that will also remove the ugliness of looking up
the device by its name to unregister it.

The name which in itself is another, harder to fix issue,
when using iomux, and the stdin string contains usbkbd we
really want to get all usbkbd-s to work, but iomux will
only take the first one.

This can be fixed in 2 ways:

1) in the usbkbd driver by registering a shared stdio_dev
for all usbkbd's and deregistering that only when the
last usbkbd is removed (in the case of dm), this will
require a global list of usbkbd devices, and stdio
callbacks to walk over this list, I believe that this
is likely the best approach

2) Fix this in iomux, and make it look for multiple
devs with the same name and mux them all.

This seems cleaner at a conceptual level, but likely
somewhat hard to implement.

Regards,

Hans



> ---
>
>   common/cmd_usb.c       | 12 ++++++------
>   common/usb_kbd.c       | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
>   include/dm/uclass-id.h |  1 +
>   3 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> index 6874af7..665f8ec 100644
> --- a/common/cmd_usb.c
> +++ b/common/cmd_usb.c
> @@ -526,11 +526,14 @@ static void do_usb_start(void)
>
>   	/* Driver model will probe the devices as they are found */
>   #ifndef CONFIG_DM_USB
> -#ifdef CONFIG_USB_STORAGE
> +# ifdef CONFIG_USB_STORAGE
>   	/* try to recognize storage devices immediately */
>   	usb_stor_curr_dev = usb_stor_scan(1);
> -#endif
> -#endif
> +# endif
> +# ifdef CONFIG_USB_KEYBOARD
> +	drv_usb_kbd_init();
> +# endif
> +#endif /* !CONFIG_DM_USB */
>   #ifdef CONFIG_USB_HOST_ETHER
>   # ifdef CONFIG_DM_ETH
>   #  ifndef CONFIG_DM_USB
> @@ -541,9 +544,6 @@ static void do_usb_start(void)
>   	usb_ether_curr_dev = usb_host_eth_scan(1);
>   # endif
>   #endif
> -#ifdef CONFIG_USB_KEYBOARD
> -	drv_usb_kbd_init();
> -#endif
>   }
>
>   #ifdef CONFIG_DM_USB
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 0227024..8037ebf 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -398,7 +398,7 @@ static int usb_kbd_getc(struct stdio_dev *sdev)
>   }
>
>   /* probes the USB device dev for keyboard type. */
> -static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
> +static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
>   {
>   	struct usb_interface *iface;
>   	struct usb_endpoint_descriptor *ep;
> @@ -495,7 +495,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
>   	int error;
>
>   	/* Try probing the keyboard */
> -	if (usb_kbd_probe(dev, 0) != 1)
> +	if (usb_kbd_probe_dev(dev, 0) != 1)
>   		return -ENOENT;
>
>   	/* Register the keyboard */
> @@ -532,6 +532,7 @@ static int probe_usb_keyboard(struct usb_device *dev)
>   	return 0;
>   }
>
> +#ifndef CONFIG_DM_USB
>   /* Search for keyboard and register it if found. */
>   int drv_usb_kbd_init(void)
>   {
> @@ -590,6 +591,7 @@ int drv_usb_kbd_init(void)
>   	/* No USB Keyboard found */
>   	return -1;
>   }
> +#endif
>
>   /* Deregister the keyboard. */
>   int usb_kbd_deregister(int force)
> @@ -621,3 +623,49 @@ int usb_kbd_deregister(int force)
>   	return 1;
>   #endif
>   }
> +
> +#ifdef CONFIG_DM_USB
> +
> +static int usb_kbd_probe(struct udevice *dev)
> +{
> +	struct usb_device *udev = dev_get_parentdata(dev);
> +	int ret;
> +
> +	ret = probe_usb_keyboard(udev);



> +
> +	return ret;
> +}
> +
> +static const struct udevice_id usb_kbd_ids[] = {
> +	{ .compatible = "usb-keyboard" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(usb_kbd) = {
> +	.name	= "usb_kbd",
> +	.id	= UCLASS_KEYBOARD,
> +	.of_match = usb_kbd_ids,
> +	.probe = usb_kbd_probe,
> +};
> +
> +/* TODO(sjg at chromium.org): Move this into a common location */
> +UCLASS_DRIVER(keyboard) = {
> +	.id		= UCLASS_KEYBOARD,
> +	.name		= "keyboard",
> +};
> +
> +static const struct usb_device_id kbd_id_table[] = {
> +	{
> +		.match_flags = USB_DEVICE_ID_MATCH_INT_CLASS |
> +			USB_DEVICE_ID_MATCH_INT_SUBCLASS |
> +			USB_DEVICE_ID_MATCH_INT_PROTOCOL,
> +		.bInterfaceClass = USB_CLASS_HID,
> +		.bInterfaceSubClass = 1,
> +		.bInterfaceProtocol = 1,
> +	},
> +	{ }		/* Terminating entry */
> +};
> +
> +U_BOOT_USB_DEVICE(usb_kbd, kbd_id_table);
> +
> +#endif
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 1eeec74..bab0025 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -36,6 +36,7 @@ enum uclass_id {
>   	UCLASS_I2C_EEPROM,	/* I2C EEPROM device */
>   	UCLASS_I2C_GENERIC,	/* Generic I2C device */
>   	UCLASS_I2C_MUX,		/* I2C multiplexer */
> +	UCLASS_KEYBOARD,	/* Keyboard input device */
>   	UCLASS_LED,		/* Light-emitting diode (LED) */
>   	UCLASS_LPC,		/* x86 'low pin count' interface */
>   	UCLASS_MASS_STORAGE,	/* Mass storage device */
>

[U-Boot] [PATCH 0/5] RFC: usb: Drop requirement for USB unbinding

[Hans de Goede]

Hi,

On 12-09-15 17:11, Simon Glass wrote:

<snip>

> Were you able to test the driver model USB keyboard support?

Yep as said that at least keeps the keyboard working with my test-case,
where as before my test case not only made "usb tree" misbehave but
also made the keyb no longer work.

I do believe we need to think a bit about how to deal with multiple
keyboards though (no tested). I realize this is something the old
code did not handle either, but that does not mean that it is not
something which we should fix while switching to the dm.

Regards,

Hans

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Simon Glass]

Hi Hans,

On 12 September 2015 at 09:15, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 08-09-15 19:15, Simon Glass wrote:
>>
>> Switch USB keyboards over to use driver model instead of scanning with the
>> horrible usb_get_dev_index() function. This involves creating a new uclass
>> for keyboards, although so far there is no API.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>
> In general I like this patch, so ack for the principle, but the
> implementation has issues.
>
> You're now allowing the registration of multiple usb keyb stdio
> input devices, but you are still relying on usb_kbd_deregister()
> to remove them from the stdio devices list, and when multiple
> or used that one will only remove the first one.
>
> This can be fixed by switching to stdio_register_dev, and
> store the returned struct stdio_dev pointer for the new dev,
> and add a dm remove callback which deregisters that specific
> stdio_dev that will also remove the ugliness of looking up
> the device by its name to unregister it.
>
> The name which in itself is another, harder to fix issue,
> when using iomux, and the stdin string contains usbkbd we
> really want to get all usbkbd-s to work, but iomux will
> only take the first one.
>
> This can be fixed in 2 ways:
>
> 1) in the usbkbd driver by registering a shared stdio_dev
> for all usbkbd's and deregistering that only when the
> last usbkbd is removed (in the case of dm), this will
> require a global list of usbkbd devices, and stdio
> callbacks to walk over this list, I believe that this
> is likely the best approach
>
> 2) Fix this in iomux, and make it look for multiple
> devs with the same name and mux them all.
>
> This seems cleaner at a conceptual level, but likely
> somewhat hard to implement.
>

I've had another look at this and here are my comments so far:

1. The existing driver does not support multiple keyboards. It
implements this limitation in multiple ways which would be a real pain
to fix while keeping the old code. I think it makes much more sense to
remove this limitation when we have either a) moved all uses of USB
keyboard to driver model, or perhaps b) moved stdio to driver model.
For now the driver model approach provides the same functionality as
before so I think it is fine.

2. The point about out-of-order devices in the 'usb tree'
display....well if you disable unbinding that is what you get. I'm
sure we could fix it by sorting the devices before displaying them,
but it does not seem that important to me. It is more likely that the
unbind support will be enabled in U-Boot proper, and perhaps disabled
in SPL, which doesn't have commands anyway.

[snip]

Regards,
Simon

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Hans de Goede]

Hi,

On 19-10-15 01:17, Simon Glass wrote:
> Hi Hans,
>
> On 12 September 2015 at 09:15, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 08-09-15 19:15, Simon Glass wrote:
>>>
>>> Switch USB keyboards over to use driver model instead of scanning with the
>>> horrible usb_get_dev_index() function. This involves creating a new uclass
>>> for keyboards, although so far there is no API.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>>
>> In general I like this patch, so ack for the principle, but the
>> implementation has issues.
>>
>> You're now allowing the registration of multiple usb keyb stdio
>> input devices, but you are still relying on usb_kbd_deregister()
>> to remove them from the stdio devices list, and when multiple
>> or used that one will only remove the first one.
>>
>> This can be fixed by switching to stdio_register_dev, and
>> store the returned struct stdio_dev pointer for the new dev,
>> and add a dm remove callback which deregisters that specific
>> stdio_dev that will also remove the ugliness of looking up
>> the device by its name to unregister it.
>>
>> The name which in itself is another, harder to fix issue,
>> when using iomux, and the stdin string contains usbkbd we
>> really want to get all usbkbd-s to work, but iomux will
>> only take the first one.
>>
>> This can be fixed in 2 ways:
>>
>> 1) in the usbkbd driver by registering a shared stdio_dev
>> for all usbkbd's and deregistering that only when the
>> last usbkbd is removed (in the case of dm), this will
>> require a global list of usbkbd devices, and stdio
>> callbacks to walk over this list, I believe that this
>> is likely the best approach
>>
>> 2) Fix this in iomux, and make it look for multiple
>> devs with the same name and mux them all.
>>
>> This seems cleaner at a conceptual level, but likely
>> somewhat hard to implement.
>>
>
> I've had another look at this and here are my comments so far:
>
> 1. The existing driver does not support multiple keyboards. It
> implements this limitation in multiple ways which would be a real pain
> to fix while keeping the old code. I think it makes much more sense to
> remove this limitation when we have either a) moved all uses of USB
> keyboard to driver model, or perhaps b) moved stdio to driver model.
> For now the driver model approach provides the same functionality as
> before so I think it is fine.

I think that supporting multiple keyboards the way I've outlined
above as "1)" should not be that hard. But I do not plan to make time
for this anytime soon, and as such I can hardly ask you to do this.

So I reluctantly agree to keep this as is (I was hoping the move
to dm would fix this).

> 2. The point about out-of-order devices in the 'usb tree'
> display....well if you disable unbinding that is what you get. I'm
> sure we could fix it by sorting the devices before displaying them,
> but it does not seem that important to me. It is more likely that the
> unbind support will be enabled in U-Boot proper, and perhaps disabled
> in SPL, which doesn't have commands anyway.

I'm fine with "usb tree" showing things the wrong way on builds where
unbinding is disabled. But if I remember the patch-set this thread is
about correctly, it completely removed unbinding from the usb code.

If you do a new version where unbinding is only skipped when compiled
out then that is fine with me.

Regards,

Hans

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Simon Glass]

Hi Hans,

On 19 October 2015 at 02:34, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
>
> On 19-10-15 01:17, Simon Glass wrote:
>>
>> Hi Hans,
>>
>> On 12 September 2015 at 09:15, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 08-09-15 19:15, Simon Glass wrote:
>>>>
>>>>
>>>> Switch USB keyboards over to use driver model instead of scanning with the
>>>> horrible usb_get_dev_index() function. This involves creating a new uclass
>>>> for keyboards, although so far there is no API.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>
>>>
>>>
>>> In general I like this patch, so ack for the principle, but the
>>> implementation has issues.
>>>
>>> You're now allowing the registration of multiple usb keyb stdio
>>> input devices, but you are still relying on usb_kbd_deregister()
>>> to remove them from the stdio devices list, and when multiple
>>> or used that one will only remove the first one.
>>>
>>> This can be fixed by switching to stdio_register_dev, and
>>> store the returned struct stdio_dev pointer for the new dev,
>>> and add a dm remove callback which deregisters that specific
>>> stdio_dev that will also remove the ugliness of looking up
>>> the device by its name to unregister it.
>>>
>>> The name which in itself is another, harder to fix issue,
>>> when using iomux, and the stdin string contains usbkbd we
>>> really want to get all usbkbd-s to work, but iomux will
>>> only take the first one.
>>>
>>> This can be fixed in 2 ways:
>>>
>>> 1) in the usbkbd driver by registering a shared stdio_dev
>>> for all usbkbd's and deregistering that only when the
>>> last usbkbd is removed (in the case of dm), this will
>>> require a global list of usbkbd devices, and stdio
>>> callbacks to walk over this list, I believe that this
>>> is likely the best approach
>>>
>>> 2) Fix this in iomux, and make it look for multiple
>>> devs with the same name and mux them all.
>>>
>>> This seems cleaner at a conceptual level, but likely
>>> somewhat hard to implement.
>>>
>>
>> I've had another look at this and here are my comments so far:
>>
>> 1. The existing driver does not support multiple keyboards. It
>> implements this limitation in multiple ways which would be a real pain
>> to fix while keeping the old code. I think it makes much more sense to
>> remove this limitation when we have either a) moved all uses of USB
>> keyboard to driver model, or perhaps b) moved stdio to driver model.
>> For now the driver model approach provides the same functionality as
>> before so I think it is fine.
>
>
> I think that supporting multiple keyboards the way I've outlined
> above as "1)" should not be that hard. But I do not plan to make time
> for this anytime soon, and as such I can hardly ask you to do this.
>
> So I reluctantly agree to keep this as is (I was hoping the move
> to dm would fix this).
>
>> 2. The point about out-of-order devices in the 'usb tree'
>> display....well if you disable unbinding that is what you get. I'm
>> sure we could fix it by sorting the devices before displaying them,
>> but it does not seem that important to me. It is more likely that the
>> unbind support will be enabled in U-Boot proper, and perhaps disabled
>> in SPL, which doesn't have commands anyway.
>
>
> I'm fine with "usb tree" showing things the wrong way on builds where
> unbinding is disabled. But if I remember the patch-set this thread is
> about correctly, it completely removed unbinding from the usb code.
>
> If you do a new version where unbinding is only skipped when compiled
> out then that is fine with me.

OK, for now I'm going to apply just this patch from the series, to
unblock the input uclass.

I'll then redo this series to allow the unbinding as, indeed, that is
what I want to happen too.

Regards,
Simon

[U-Boot] [PATCH 4/5] dm: usb: Add support for USB keyboards with driver model

[Simon Glass]

On 29 October 2015 at 13:09, Simon Glass <sjg@chromium.org> wrote:
> Hi Hans,
>
> On 19 October 2015 at 02:34, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>>
>> On 19-10-15 01:17, Simon Glass wrote:
>>>
>>> Hi Hans,
>>>
>>> On 12 September 2015 at 09:15, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 08-09-15 19:15, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Switch USB keyboards over to use driver model instead of scanning with the
>>>>> horrible usb_get_dev_index() function. This involves creating a new uclass
>>>>> for keyboards, although so far there is no API.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>
>>>>
>>>>
>>>> In general I like this patch, so ack for the principle, but the
>>>> implementation has issues.
>>>>
>>>> You're now allowing the registration of multiple usb keyb stdio
>>>> input devices, but you are still relying on usb_kbd_deregister()
>>>> to remove them from the stdio devices list, and when multiple
>>>> or used that one will only remove the first one.
>>>>
>>>> This can be fixed by switching to stdio_register_dev, and
>>>> store the returned struct stdio_dev pointer for the new dev,
>>>> and add a dm remove callback which deregisters that specific
>>>> stdio_dev that will also remove the ugliness of looking up
>>>> the device by its name to unregister it.
>>>>
>>>> The name which in itself is another, harder to fix issue,
>>>> when using iomux, and the stdin string contains usbkbd we
>>>> really want to get all usbkbd-s to work, but iomux will
>>>> only take the first one.
>>>>
>>>> This can be fixed in 2 ways:
>>>>
>>>> 1) in the usbkbd driver by registering a shared stdio_dev
>>>> for all usbkbd's and deregistering that only when the
>>>> last usbkbd is removed (in the case of dm), this will
>>>> require a global list of usbkbd devices, and stdio
>>>> callbacks to walk over this list, I believe that this
>>>> is likely the best approach
>>>>
>>>> 2) Fix this in iomux, and make it look for multiple
>>>> devs with the same name and mux them all.
>>>>
>>>> This seems cleaner at a conceptual level, but likely
>>>> somewhat hard to implement.
>>>>
>>>
>>> I've had another look at this and here are my comments so far:
>>>
>>> 1. The existing driver does not support multiple keyboards. It
>>> implements this limitation in multiple ways which would be a real pain
>>> to fix while keeping the old code. I think it makes much more sense to
>>> remove this limitation when we have either a) moved all uses of USB
>>> keyboard to driver model, or perhaps b) moved stdio to driver model.
>>> For now the driver model approach provides the same functionality as
>>> before so I think it is fine.
>>
>>
>> I think that supporting multiple keyboards the way I've outlined
>> above as "1)" should not be that hard. But I do not plan to make time
>> for this anytime soon, and as such I can hardly ask you to do this.
>>
>> So I reluctantly agree to keep this as is (I was hoping the move
>> to dm would fix this).
>>
>>> 2. The point about out-of-order devices in the 'usb tree'
>>> display....well if you disable unbinding that is what you get. I'm
>>> sure we could fix it by sorting the devices before displaying them,
>>> but it does not seem that important to me. It is more likely that the
>>> unbind support will be enabled in U-Boot proper, and perhaps disabled
>>> in SPL, which doesn't have commands anyway.
>>
>>
>> I'm fine with "usb tree" showing things the wrong way on builds where
>> unbinding is disabled. But if I remember the patch-set this thread is
>> about correctly, it completely removed unbinding from the usb code.
>>
>> If you do a new version where unbinding is only skipped when compiled
>> out then that is fine with me.
>
> OK, for now I'm going to apply just this patch from the series, to
> unblock the input uclass.
>
> I'll then redo this series to allow the unbinding as, indeed, that is
> what I want to happen too.

Applied to u-boot-dm.