Convert musb host mode code to the device-model

There is no harm in declaring the function prototypes even if nothing implements them, and when CONFIG_DM_USB=y the various usb functions are available regardless of any controller drivers being enabled.

This fixes compile warnings due to missing prototypes on ARCHs where the ARCH Kconfig always enables CONFIG_DM_USB and various usb drivers.

One could argue that in the case of no controllers CONFIG_DM_USB should not be set, but this problem is typically seen during bringup of boards which do actually have usb controllers.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Simply always define the function prototypes instead of adding yet another
condition to the already unwieldly #if def ... || def ... condition

---
 include/usb.h | 15 ---------------
 1 file changed, 15 deletions(-)

Quoted 11 lines · Hans de Goede

There is no harm in declaring the function prototypes even if nothing implements them, and when CONFIG_DM_USB=y the various usb functions are available regardless of any controller drivers being enabled.

This fixes compile warnings due to missing prototypes on ARCHs where the ARCH Kconfig always enables CONFIG_DM_USB and various usb drivers.

One could argue that in the case of no controllers CONFIG_DM_USB should not be set, but this problem is typically seen during bringup of boards which do actually have usb controllers.

Quoted 8 lines

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Simply always define the function prototypes instead of adding yet another
condition to the already unwieldly #if def ... || def ... condition

---
 include/usb.h | 15 ---------------
 1 file changed, 15 deletions(-)

Acked-by Simon Glass <[email protected]>

The device-model usb_legacy_port_reset function calls the device-model usb_port_reset function which is a 1 on 1 copy of the non dm usb_legacy_port_reset and this is the only use of usb_port_reset in all of u-boot.

Drop both, and alway use the usb_legacy_port_reset() version in common/usb.c .

Also while at it make it static as it is only used in common/usb.c .

Signed-off-by Hans de Goede <[email protected]>

---
 common/usb.c                  |  4 +---
 drivers/usb/host/usb-uclass.c | 29 -----------------------------
 include/usb.h                 |  8 --------
 3 files changed, 1 insertion(+), 40 deletions(-)

Quoted 1 line · Hans de Goede

The device-model usb_legacy_port_reset function cApplied to u-boot-dm, thanks!

Quoted 9 lines

usb_port_reset function which is a 1 on 1 copy of the non dm usb_legacy_port_reset and this is the only use of usb_port_reset in all of u-boot.

Drop both, and alway use the usb_legacy_port_reset() version in common/usb.c .

Also while at it make it static as it is only used in common/usb.c .

Quoted 6 lines

Signed-off-by Hans de Goede <[email protected]>

---
 common/usb.c                  |  4 +---
 drivers/usb/host/usb-uclass.c | 29 -----------------------------
 include/usb.h                 |  8 --------
 3 files changed, 1 insertion(+), 40 deletions(-)

Acked-by Simon Glass <[email protected]>

Drop the unneeded portnr function argument, the portnr is part of the usb_device struct which is passed via the dev argument.

Signed-off-by Hans de Goede <[email protected]>

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

Hi Hans.

Quoted 9 lines · Hans de Goede

Drop the unneeded portnr function argument, the portnr is part of the usb_device struct which is passed via the dev argument.

Signed-off-by Hans de Goede <[email protected]>

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

This was needed in the case where a fake usb_device was passed in. Has your previous refactoring changed that?

Hi,

Quoted 16 lines · Simon Glass

Hi Hans.

Quoted 10 lines · Hans de Goede

Drop the unneeded portnr function argument, the portnr is part of the usb_device struct which is passed via the dev argument.

Signed-off-by Hans de Goede <[email protected]>

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

Quoted 2 lines

This was needed in the case where a fake usb_device was passed in. Has your previous refactoring changed that?

The portnr is still passed but it is padded via the usb_device struct's portnr member. When doing a CONFIG_DM_USB=y build the only call site of usb_setup_device() is usb_scan_device() from drivers/usb/host/usb-uclass.c which does:

        udev->portnr = port;
        debug("Calling usb_setup_device(), portnr=%d\n", udev->portnr);
        parent_udev = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
                dev_get_parentdata(parent) : NULL;
        ret = usb_setup_device(udev, priv->desc_before_addr, parent_udev);

So portnr is always set in the usb_device strict, and that is what gets used after this patch.

Hi Hans,

Quoted 35 lines · Hans de Goede

Hi,

Quoted 16 lines · Simon Glass

Hi Hans.

Quoted 10 lines · Hans de Goede

Drop the unneeded portnr function argument, the portnr is part of the usb_device struct which is passed via the dev argument.

Signed-off-by Hans de Goede <[email protected]>

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

Quoted 2 lines

This was needed in the case where a fake usb_device was passed in. Has your previous refactoring changed that?

The portnr is still passed but it is padded via the usb_device struct's portnr member. When doing a CONFIG_DM_USB=y build the only call site of usb_setup_device() is usb_scan_device() from drivers/usb/host/usb-uclass.c which does:

        udev->portnr = port;
        debug("Calling usb_setup_device(), portnr=%d\n", udev->portnr);
        parent_udev = device_get_uclass_id(parent) == UCLASS_USB_HUB ?
                dev_get_parentdata(parent) : NULL;
        ret = usb_setup_device(udev, priv->desc_before_addr, parent_udev);

Quoted 2 lines

So portnr is always set in the usb_device strict, and that is what gets used after this patch.

OK thanks for explaining that.

Acked-by Simon Glass <[email protected]>

Pass the usb_device instead of the portnr to usb_legacy_port_reset and rename it to usb_hub_port_reset as there is nothing legacy about it.

Signed-off-by Hans de Goede <[email protected]>

---
 common/usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Hi Hans,

Quoted 3 lines · Hans de Goede

Pass the usb_device instead of the portnr to usb_legacy_port_reset and rename it to usb_hub_port_reset as there is nothing legacy about it.

Quoted 4 lines

Signed-off-by Hans de Goede <[email protected]>

---
  common/usb.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

Legacy as in not driver model.

Hi,

Quoted 14 lines · Simon Glass

Hi Hans,

Quoted 8 lines · Hans de Goede

Pass the usb_device instead of the portnr to usb_legacy_port_reset and rename it to usb_hub_port_reset as there is nothing legacy about it.

Signed-off-by Hans de Goede <[email protected]>

---
  common/usb.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

Quoted 1 line

Legacy as in not driver model.

Except that it gets used in both device-model and non device model builds of the usb-stack.

Regards,

Hans

p.s.

Thanks for reviewing this largish series!

Hi Hans,

Quoted 18 lines · Hans de Goede

Hi,

Quoted 14 lines · Simon Glass

Hi Hans,

Quoted 8 lines · Hans de Goede

Pass the usb_device instead of the portnr to usb_legacy_port_reset and rename it to usb_hub_port_reset as there is nothing legacy about it.

Signed-off-by Hans de Goede <[email protected]>

---
  common/usb.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

Quoted 5 lines

Quoted 1 line

Legacy as in not driver model.

Except that it gets used in both device-model and non device model builds of the usb-stack.

Yes, and the non-driver-model stuff can presumably be considered 'legacy' at this point?

Acked-by Simon Glass <[email protected]>

Quoted 5 lines

Regards,

Hans

Quoted 3 lines

p.s.

Thanks for reviewing this largish series!

You're welcome. It's a lot easier than writing it :-)

Regards,
Simon

Add an usb_device parameter to usb_reset_root_port so that it knows which root-port it is resetting. This is necessary for proper device-model support for usb_reset_root_port.

Also remove a duplicate declaration of usb_reset_root_port() from usb.h .

Signed-off-by Hans de Goede <[email protected]>

---
 common/usb.c                      | 2 +-
 drivers/usb/host/usb-uclass.c     | 2 +-
 drivers/usb/musb-new/musb_uboot.c | 4 ++--
 include/usb.h                     | 8 ++------
 4 files changed, 6 insertions(+), 10 deletions(-)

Quoted 6 lines · Hans de Goede

Add an usb_device parameter to usb_reset_root_port so that it knows which root-port it is resetting. This is necessary for proper device-model support for usb_reset_root_port.

Also remove a duplicate declaration of usb_reset_root_port() from usb.h .

Quoted 7 lines

Signed-off-by Hans de Goede <[email protected]>

---
 common/usb.c                      | 2 +-
 drivers/usb/host/usb-uclass.c     | 2 +-
 drivers/usb/musb-new/musb_uboot.c | 4 ++--
 include/usb.h                     | 8 ++------
 4 files changed, 6 insertions(+), 10 deletions(-)

Acked-by Simon Glass <[email protected]>

These functions are useful to remove all children from an usb bus before rescanning the bus.

Signed-off-by Hans de Goede <[email protected]>

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

drivers/core/device-remove.c +0 -1

index 6a16b4f..06de7e3 100644
@@ -18,16 +18,7 @@
 #include <dm/uclass-internal.h>
 #include <dm/util.h>
 
-/**

drivers/core/device-remove.c +1 -6

 {
        struct udevice *pos, *n;
        int ret, saved_ret = 0;

@@ -43,12 +34,7 @@ static int device_chld_unbind(struct udevice *dev)
        return saved_ret;
 }
 
-/**
- * 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)
+int device_chld_remove(struct udevice *dev)
 {
        struct udevice *pos, *n;
        int ret;

include/dm/device-internal.h +26 -1

index 687462b..6c8fe23 100644
@@ -107,6 +107,32 @@ int device_unbind(struct udevice *dev);
 static inline int device_unbind(struct udevice *dev) { return 0; }
 #endif
 
+/**
+ * device_chld_remove() - Stop all device's children
+ * @dev:       The device whose children are to be removed
+ * @return 0 on success, -ve on error
+ */
+#ifdef CONFIG_DM_DEVICE_REMOVE
+int device_chld_remove(struct udevice *dev);
+#else
+static inline int device_chld_remove(struct udevice *dev) { return 0; }
+#endif
+
+/**
+ * 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
+ */
+#ifdef CONFIG_DM_DEVICE_REMOVE
+int device_chld_unbind(struct udevice *dev);
+#else
+static inline int device_chld_unbind(struct udevice *dev) { return 0; }
+#endif
+
 #ifdef CONFIG_DM_DEVICE_REMOVE
 void device_free(struct udevice *dev);
 #else
--

2.4.3

Hi Hans,

Quoted 17 lines · Hans de Goede

These functions are useful to remove all children from an usb bus before rescanning the bus.

Signed-off-by Hans de Goede <[email protected]>

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

drivers/core/device-remove.c +0 -0

index 6a16b4f..06de7e3 100644
@@ -18,16 +18,7 @@
  #include <dm/uclass-internal.h>
  #include <dm/util.h>
 

drivers/core/device-remove.c line 21

Quoted 11 lines

drivers/core/device-remove.c +1 -10

-/**
- * 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)
+int device_chld_unbind(struct udevice *dev)

If we are exporting this can we give it a better name? Maybe device_unbind_children()?

Quoted 56 lines

drivers/core/device-remove.c +1 -6

  {
         struct udevice *pos, *n;
         int ret, saved_ret = 0;

@@ -43,12 +34,7 @@ static int device_chld_unbind(struct udevice *dev)
         return saved_ret;
  }
 
-/**
- * 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)
+int device_chld_remove(struct udevice *dev)
  {
         struct udevice *pos, *n;
         int ret;

include/dm/device-internal.h +26 -1

index 687462b..6c8fe23 100644
@@ -107,6 +107,32 @@ int device_unbind(struct udevice *dev);
  static inline int device_unbind(struct udevice *dev) { return 0; }
  #endif
 
+/**
+ * device_chld_remove() - Stop all device's children
+ * @dev:       The device whose children are to be removed
+ * @return 0 on success, -ve on error
+ */
+#ifdef CONFIG_DM_DEVICE_REMOVE
+int device_chld_remove(struct udevice *dev);
+#else
+static inline int device_chld_remove(struct udevice *dev) { return 0; }
+#endif
+
+/**
+ * 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
+ */
+#ifdef CONFIG_DM_DEVICE_REMOVE
+int device_chld_unbind(struct udevice *dev);
+#else
+static inline int device_chld_unbind(struct udevice *dev) { return 0; }
+#endif
+
  #ifdef CONFIG_DM_DEVICE_REMOVE
  void device_free(struct udevice *dev);
  #else
--

Quoted 2 lines

2.4.3

Regards,
Simon

last_child was abused by the old usb code to first store 1 if the usb_device was not the root of the usb tree, and then later on re-used to store whether or not the usb_device is actually the last child.

The dm-usb code was always setting it to actually reflect the last-child status which is wrong for the last child leading to output like this:

USB device tree:
1 Hub (12 Mb/s, 100mA)

  |  ALCOR USB Hub 2.0
  |
  | 2  Mass Storage (12 Mb/s, 100mA)
  |    USB Flash Disk 4C0E960F
  |

+-3 Human Interface (1.5 Mb/s, 100mA)
SINO WEALTH USB Composite Device

Instead of this:

USB device tree:
1 Hub (12 Mb/s, 100mA)
| ALCOR USB Hub 2.0
|

  +-2  Mass Storage (12 Mb/s, 100mA)
  |    USB Flash Disk 4C0E960F
  |
  +-3  Human Interface (1.5 Mb/s, 100mA)

SINO WEALTH USB Composite Device

This commit fixes this by first checking that the device is not root, and then setting last_child. This commit also updates the old code to not abuse the last_child variable to store the root check result.

Signed-off-by Hans de Goede <[email protected]>

---
 common/cmd_usb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Quoted 33 lines · Hans de Goede

last_child was abused by the old usb code to first store 1 if the usb_device was not the root of the usb tree, and then later on re-used to store whether or not the usb_device is actually the last child.

The dm-usb code was always setting it to actually reflect the last-child status which is wrong for the last child leading to output like this:

USB device tree:
1 Hub (12 Mb/s, 100mA)

  |  ALCOR USB Hub 2.0
  |
  | 2  Mass Storage (12 Mb/s, 100mA)
  |    USB Flash Disk 4C0E960F
  |

+-3 Human Interface (1.5 Mb/s, 100mA)
SINO WEALTH USB Composite Device

Instead of this:

USB device tree:
1 Hub (12 Mb/s, 100mA)
| ALCOR USB Hub 2.0
|

  +-2  Mass Storage (12 Mb/s, 100mA)
  |    USB Flash Disk 4C0E960F
  |
  +-3  Human Interface (1.5 Mb/s, 100mA)

SINO WEALTH USB Composite Device

This commit fixes this by first checking that the device is not root, and then setting last_child. This commit also updates the old code to not abuse the last_child variable to store the root check result.

Quoted 4 lines

Signed-off-by Hans de Goede <[email protected]>

---
 common/cmd_usb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Acked-by Simon Glass <[email protected]>

On an usb stop instead of leaving orphan usb devices behind simply remove

them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build usb_stop() when that is set.

The result of this commit is best seen in the output of "dm tree" after plugging out an usb hub with 2 devices plugges in and plugging in a keyb. instead, before this commit the output would be:

usb [ + ] `-- sunxi-musb

 usb_hub     [   ]        |-- usb_hub
 usb_mass_st [   ]        |   |-- usb_mass_storage
 usb_dev_gen [   ]        |   `-- generic_bus_0_dev_3
 usb_dev_gen [ + ]        `-- generic_bus_0_dev_1

Notice the non active usb_hub child and its 2 non active children. The first child being non-active as in this example also causes usb_get_dev_index to return NULL when probing the first child, which results in the usb kbd code not binding to the keyboard.

With this commit in place the output after swapping and "usb reset" is:

usb [ + ] `-- sunxi-musb
usb_dev_gen [ + ] `-- generic_bus_0_dev_1

As expected, and usb_get_dev_index works properly and the keyboard works.

After this commit usb_find_child() is only necessary for emulated usb devices, so make its body #ifdef CONFIG_USB_EMUL.

Signed-off-by Hans de Goede <[email protected]>

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

drivers/usb/host/usb-uclass.c +8 -0

index bce6cec..8f26e35 100644
@@ -128,6 +128,7 @@ int usb_alloc_device(struct usb_device *udev)
        return ops->alloc_device(bus, udev);
 }
 
+#ifdef CONFIG_DM_DEVICE_REMOVE
 int usb_stop(void)
 {
        struct udevice *bus;

@@ -143,6 +144,12 @@ int usb_stop(void)
        uc_priv = uc->priv;
 
        uclass_foreach_dev(bus, uc) {
+               ret = device_chld_remove(bus);
+               if (ret && !err)
+                       err = ret;
+               ret = device_chld_unbind(bus);
+               if (ret && !err)
+                       err = ret;
                ret = device_remove(bus);
                if (ret && !err)
                        err = ret;

@@ -166,6 +173,7 @@ int usb_stop(void)
 
        return err;
 }
+#endif
 
 static void usb_scan_bus(struct udevice *bus, bool recurse)
 {

drivers/usb/host/usb-uclass.c +1 -1

        struct udevice *dev;
 
        *devp = NULL;

@@ -509,6 +518,7 @@ static int usb_find_child(struct udevice *parent,
                        return 0;
                }
        }
+#endif
 
        return -ENOENT;
 }
--

2.4.3

Hi Hans,

Quoted 1 line · Hans de Goede

On an usb stop instead of leaving orphan usb devices behind simply remove

On a usb_stop()

or

On a 'usb stop' command ?

Quoted 2 lines

them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build usb_stop() when that is set.

This seems a little unfortunate. I can see the reasoning, but do you think this is necessary? I suspect people chasing code size may remove that option and still want to use USB properly.

Quoted 12 lines

The result of this commit is best seen in the output of "dm tree" after plugging out an usb hub with 2 devices plugges in and plugging in a keyb. instead, before this commit the output would be:

usb [ + ] `-- sunxi-musb

  usb_hub     [   ]        |-- usb_hub
  usb_mass_st [   ]        |   |-- usb_mass_storage
  usb_dev_gen [   ]        |   `-- generic_bus_0_dev_3
  usb_dev_gen [ + ]        `-- generic_bus_0_dev_1

Quoted 4 lines

Notice the non active usb_hub child and its 2 non active children. The first child being non-active as in this example also causes usb_get_dev_index to return NULL when probing the first child, which results in the usb kbd code not binding to the keyboard.

Although I suspect that could be fixed.

Quoted 48 lines

With this commit in place the output after swapping and "usb reset" is:

usb [ + ] `-- sunxi-musb
usb_dev_gen [ + ] `-- generic_bus_0_dev_1

As expected, and usb_get_dev_index works properly and the keyboard works.

After this commit usb_find_child() is only necessary for emulated usb devices, so make its body #ifdef CONFIG_USB_EMUL.

Signed-off-by Hans de Goede <[email protected]>

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

drivers/usb/host/usb-uclass.c +8 -0

index bce6cec..8f26e35 100644
@@ -128,6 +128,7 @@ int usb_alloc_device(struct usb_device *udev)
         return ops->alloc_device(bus, udev);
  }
 
+#ifdef CONFIG_DM_DEVICE_REMOVE
  int usb_stop(void)
  {
         struct udevice *bus;

@@ -143,6 +144,12 @@ int usb_stop(void)
         uc_priv = uc->priv;
 
         uclass_foreach_dev(bus, uc) {
+               ret = device_chld_remove(bus);
+               if (ret && !err)
+                       err = ret;
+               ret = device_chld_unbind(bus);
+               if (ret && !err)
+                       err = ret;
                 ret = device_remove(bus);
                 if (ret && !err)
                         err = ret;

@@ -166,6 +173,7 @@ int usb_stop(void)
 
         return err;
  }
+#endif
 

drivers/usb/host/usb-uclass.c static int usb_find_child(struct udevice *parent, · line 502

Quoted 7 lines

drivers/usb/host/usb-uclass.c +1 -0

  static void usb_scan_bus(struct udevice *bus, bool recurse)
  {

@@ -491,6 +499,7 @@ static int usb_find_child(struct udevice *parent,
                           struct usb_interface_descriptor *iface,
                           struct udevice **devp)
  {
+#ifdef CONFIG_USB_EMUL /* Emulated devices are explictily bound */

explicitly

Can you add a comment about this? It seems that we should rename this function to usb_find_emul_child() and have it present only when CONFIG_USB_EMUL is around?

Also, why bother with the #ifdef if this function is never called outside sandbox?

Quoted 10 lines

drivers/usb/host/usb-uclass.c +1 -0

         struct udevice *dev;
 
         *devp = NULL;

@@ -509,6 +518,7 @@ static int usb_find_child(struct udevice *parent,
                         return 0;
                 }
         }
+#endif
 

Quoted 5 lines

drivers/usb/host/usb-uclass.c +0 -1

         return -ENOENT;
  }
--

2.4.3

Regards,
Simon

Hi,

Quoted 5 lines · Simon Glass

Hi Hans,

Quoted 1 line · Hans de Goede

Quoted 8 lines

Quoted 1 line

On an usb stop instead of leaving orphan usb devices behind simply remove

On a usb_stop()

or

On a 'usb stop' command ?

My intention was for both, since I was under the assumption that "usb stop" on the cmdline, was the only caller of usb_stop(), but a quick grep to the sources show that I'm wrong ...

Quoted 7 lines

Quoted 2 lines

them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build usb_stop() when that is set.

This seems a little unfortunate. I can see the reasoning, but do you think this is necessary? I suspect people chasing code size may remove that option and still want to use USB properly.

This was mostly a result of my thinking that usb_stop() is only used on "usb stop" at the cmdline, which I know realize is wrong.

However my quick grep has learned that we do really need CONFIG_DM_DEVICE_REMOVE
to properly implement usb_stop():

From common/bootm.c :

#if defined(CONFIG_CMD_USB)
/*
* turn off USB to prevent the host controller from writing to the
* SDRAM while Linux is booting. This could happen (at least for
OHCI
* controller), because the HCCA (Host Controller Communication
Area)
* lies within the SDRAM and the host controller writes continously
to
* this area (as busmaster!). The HccaFrameNumber is for example
* updated every 1 ms within the HCCA structure in SDRAM! For more
* details see the OpenHCI specification.
*/
usb_stop();
#endif

And without CONFIG_DM_DEVICE_REMOVE we end up never calling the hcd's remove callback and thus do not properly stop the usb controller.

So this problem of usb_stop() needing CONFIG_DM_DEVICE_REMOVE already exists before this patch. If you want I can split out the adding of the #ifdef in a separate commit, spelling out why usb_stop() MUST have CONFIG_DM_DEVICE_REMOVE in the commit message. Or maybe just move this all to
Kconfig and make DM_USB conflict with CONFIG_DM_DEVICE_REMOVE?

Quoted 21 lines

Quoted 19 lines

The result of this commit is best seen in the output of "dm tree" after plugging out an usb hub with 2 devices plugges in and plugging in a keyb. instead, before this commit the output would be:

usb [ + ] `-- sunxi-musb

  usb_hub     [   ]        |-- usb_hub
  usb_mass_st [   ]        |   |-- usb_mass_storage
  usb_dev_gen [   ]        |   `-- generic_bus_0_dev_3
  usb_dev_gen [ + ]        `-- generic_bus_0_dev_1

Notice the non active usb_hub child and its 2 non active children. The first child being non-active as in this example also causes usb_get_dev_index
to return NULL when probing the first child, which results in the usb kbd code not binding to the keyboard.

Quoted 1 line

Although I suspect that could be fixed.

Right, but just removing the children is a much cleaner solution, and also makes the output of "dm tree" properly reflect reality.

Quoted 51 lines

Quoted 51 lines

With this commit in place the output after swapping and "usb reset" is:

usb [ + ] `-- sunxi-musb
usb_dev_gen [ + ] `-- generic_bus_0_dev_1

As expected, and usb_get_dev_index works properly and the keyboard works.

After this commit usb_find_child() is only necessary for emulated usb devices, so make its body #ifdef CONFIG_USB_EMUL.

Signed-off-by Hans de Goede <[email protected]>

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

drivers/usb/host/usb-uclass.c +8 -0

index bce6cec..8f26e35 100644
@@ -128,6 +128,7 @@ int usb_alloc_device(struct usb_device *udev)
         return ops->alloc_device(bus, udev);
  }
 
+#ifdef CONFIG_DM_DEVICE_REMOVE
  int usb_stop(void)
  {
         struct udevice *bus;

@@ -143,6 +144,12 @@ int usb_stop(void)
         uc_priv = uc->priv;
 
         uclass_foreach_dev(bus, uc) {
+               ret = device_chld_remove(bus);
+               if (ret && !err)
+                       err = ret;
+               ret = device_chld_unbind(bus);
+               if (ret && !err)
+                       err = ret;
                 ret = device_remove(bus);
                 if (ret && !err)
                         err = ret;

@@ -166,6 +173,7 @@ int usb_stop(void)
 
         return err;
  }
+#endif
 

Quoted 10 lines

drivers/usb/host/usb-uclass.c static int usb_find_child(struct udevice *parent, · line 502

Quoted 7 lines

drivers/usb/host/usb-uclass.c +1 -0

  static void usb_scan_bus(struct udevice *bus, bool recurse)
  {

@@ -491,6 +499,7 @@ static int usb_find_child(struct udevice *parent,
                           struct usb_interface_descriptor *iface,
                           struct udevice **devp)
  {
+#ifdef CONFIG_USB_EMUL /* Emulated devices are explictily bound */

explicitly

Ack.

Quoted 3 lines

Can you add a comment about this? It seems that we should rename this function to usb_find_emul_child() and have it present only when CONFIG_USB_EMUL is around?

Renaming it to usb_find_emul_child() and only defining the function when CONFIG_USB_EMUL works for me I will do that for v2.

Quoted 2 lines

Also, why bother with the #ifdef if this function is never called outside sandbox?

Because its call site does not have a #ifdef, I'll add an #ifdef at its call site to make it more clear that this is only used for emulated devices.

Quoted 10 lines

Quoted 10 lines

drivers/usb/host/usb-uclass.c +1 -0

         struct udevice *dev;
 
         *devp = NULL;

@@ -509,6 +518,7 @@ static int usb_find_child(struct udevice *parent,
                         return 0;
                 }
         }
+#endif
 

Quoted 5 lines

Quoted 5 lines

drivers/usb/host/usb-uclass.c +0 -1

         return -ENOENT;
  }
--

2.4.3

Hi Hans,

Quoted 59 lines · Hans de Goede

Hi,

Quoted 5 lines · Simon Glass

Hi Hans,

Quoted 1 line · Hans de Goede

Quoted 8 lines

Quoted 1 line

On an usb stop instead of leaving orphan usb devices behind simply remove

On a usb_stop()

or

On a 'usb stop' command ?

My intention was for both, since I was under the assumption that "usb stop" on the cmdline, was the only caller of usb_stop(), but a quick grep to the sources show that I'm wrong ...

Quoted 7 lines

Quoted 2 lines

them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build usb_stop() when that is set.

This seems a little unfortunate. I can see the reasoning, but do you think this is necessary? I suspect people chasing code size may remove that option and still want to use USB properly.

This was mostly a result of my thinking that usb_stop() is only used on "usb stop" at the cmdline, which I know realize is wrong.

However my quick grep has learned that we do really need CONFIG_DM_DEVICE_REMOVE
to properly implement usb_stop():

From common/bootm.c :

#if defined(CONFIG_CMD_USB)
/*
* turn off USB to prevent the host controller from writing to the
* SDRAM while Linux is booting. This could happen (at least for
OHCI
* controller), because the HCCA (Host Controller Communication
Area)
* lies within the SDRAM and the host controller writes continously
to
* this area (as busmaster!). The HccaFrameNumber is for example
* updated every 1 ms within the HCCA structure in SDRAM! For more
* details see the OpenHCI specification.
*/
usb_stop();
#endif

And without CONFIG_DM_DEVICE_REMOVE we end up never calling the hcd's remove callback and thus do not properly stop the usb controller.

Quoted 7 lines

So this problem of usb_stop() needing CONFIG_DM_DEVICE_REMOVE already exists before this patch. If you want I can split out the adding of the #ifdef in a separate commit, spelling out why usb_stop() MUST have CONFIG_DM_DEVICE_REMOVE in the commit message. Or maybe just move this all to
Kconfig and make DM_USB conflict with CONFIG_DM_DEVICE_REMOVE?

I don't think that is necessary, it feels a bit too inflexible. But perhaps you could add a comment to the Kconfig help for CONFIG_DM_DEVICE_REMOVE?

It is remove() that is needed, not unbind(). Actually I think it is quite unfortunate to make usb_stop() call unbind. It is a waste of time to do this just before booting the kernel - the current design leaves all devices bound (but I hope we can remove() them at some point).

Instead, I wonder if we can remove the children when we probe the bus?

Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today.

Quoted 25 lines

Quoted 23 lines

Quoted 15 lines

The result of this commit is best seen in the output of "dm tree" after plugging out an usb hub with 2 devices plugges in and plugging in a keyb. instead, before this commit the output would be:

usb [ + ] `-- sunxi-musb

   usb_hub     [   ]        |-- usb_hub
   usb_mass_st [   ]        |   |-- usb_mass_storage
   usb_dev_gen [   ]        |   `-- generic_bus_0_dev_3
   usb_dev_gen [ + ]        `-- generic_bus_0_dev_1

Quoted 5 lines

Notice the non active usb_hub child and its 2 non active children. The first child being non-active as in this example also causes usb_get_dev_index
to return NULL when probing the first child, which results in the usb kbd code not binding to the keyboard.

Although I suspect that could be fixed.

Quoted 2 lines

Right, but just removing the children is a much cleaner solution, and also makes the output of "dm tree" properly reflect reality.

True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan.

Hi,

Quoted 87 lines · Simon Glass

Hi Hans,

Quoted 73 lines · Hans de Goede

Hi,

Quoted 12 lines · Simon Glass

Hi Hans,

Quoted 4 lines · Hans de Goede

On an usb stop instead of leaving orphan usb devices behind simply remove

Quoted 5 lines

On a usb_stop()

or

On a 'usb stop' command ?

My intention was for both, since I was under the assumption that "usb stop"
on the cmdline, was the only caller of usb_stop(), but a quick grep to the
sources show that I'm wrong ...

Quoted 8 lines

Quoted 2 lines

them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build usb_stop() when that is set.

This seems a little unfortunate. I can see the reasoning, but do you think this is necessary? I suspect people chasing code size may remove that option and still want to use USB properly.

This was mostly a result of my thinking that usb_stop() is only used on "usb stop" at the cmdline, which I know realize is wrong.

However my quick grep has learned that we do really need CONFIG_DM_DEVICE_REMOVE
to properly implement usb_stop():

From common/bootm.c :

#if defined(CONFIG_CMD_USB)
/*
* turn off USB to prevent the host controller from writing to
the
* SDRAM while Linux is booting. This could happen (at least for
OHCI
* controller), because the HCCA (Host Controller Communication
Area)
* lies within the SDRAM and the host controller writes
continously
to
* this area (as busmaster!). The HccaFrameNumber is for example
* updated every 1 ms within the HCCA structure in SDRAM! For
more

          * details see the OpenHCI specification.
          */
         usb_stop();

#endif

And without CONFIG_DM_DEVICE_REMOVE we end up never calling the hcd's remove
callback and thus do not properly stop the usb controller.

Quoted 9 lines

So this problem of usb_stop() needing CONFIG_DM_DEVICE_REMOVE already exists
before this patch. If you want I can split out the adding of the #ifdef in a separate commit, spelling out why usb_stop() MUST have CONFIG_DM_DEVICE_REMOVE in the commit message. Or maybe just move this all
to
Kconfig and make DM_USB conflict with CONFIG_DM_DEVICE_REMOVE?

Quoted 3 lines

I don't think that is necessary, it feels a bit too inflexible. But perhaps you could add a comment to the Kconfig help for CONFIG_DM_DEVICE_REMOVE?

Ok will do.

Quoted 6 lines

It is remove() that is needed, not unbind(). Actually I think it is quite unfortunate to make usb_stop() call unbind. It is a waste of time to do this just before booting the kernel - the current design leaves all devices bound (but I hope we can remove() them at some point).

Quoted 1 line

Instead, I wonder if we can remove the children when we probe the bus?

That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge.

Quoted 5 lines

Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today.

AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus.

Quoted 32 lines

Quoted 32 lines

Quoted 29 lines

Quoted 19 lines

The result of this commit is best seen in the output of "dm tree" after plugging out an usb hub with 2 devices plugges in and plugging in a keyb.
instead, before this commit the output would be:

usb [ + ] `-- sunxi-musb

   usb_hub     [   ]        |-- usb_hub
   usb_mass_st [   ]        |   |-- usb_mass_storage
   usb_dev_gen [   ]        |   `-- generic_bus_0_dev_3
   usb_dev_gen [ + ]        `-- generic_bus_0_dev_1

Quoted 6 lines

Notice the non active usb_hub child and its 2 non active children. The first child being non-active as in this example also causes usb_get_dev_index
to return NULL when probing the first child, which results in the usb kbd
code not binding to the keyboard.

Although I suspect that could be fixed.

Quoted 8 lines

Quoted 3 lines

Right, but just removing the children is a much cleaner solution, and also
makes the output of "dm tree" properly reflect reality.

True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan.

That seems like needless complexity. I believe that simply removing + unbinding
the children on usb_stop is the right thing to do, and it also is the KISS solution.

Quoted 6 lines

Quoted 1 line

Instead, I wonder if we can remove the children when we probe the bus?

That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge.

That's how it currently works, from what I can see in the code. But since there is a 'usb_started' boolean this is irrelevant.

Quoted 7 lines

Quoted 5 lines

Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today.

Quoted 2 lines

AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus.

Sort-of. But as with PCI it is useful to be able to add settings for the devices in some cases. You can match them using vendor/device or interface IDs. Then the driver can access its settings.

That's why I'm suggesting we unbind the devices that are no-longer present.

Quoted 43 lines

Quoted 41 lines

Quoted 29 lines

Quoted 29 lines

Quoted 26 lines

The result of this commit is best seen in the output of "dm tree" after plugging out an usb hub with 2 devices plugges in and plugging in a keyb.
instead, before this commit the output would be:

usb [ + ] `-- sunxi-musb
usb_hub [ ] |-- usb_hub
usb_mass_st [ ] | |-- usb_mass_storage
usb_dev_gen [ ] | `-- generic_bus_0_dev_3
usb_dev_gen [ + ] `-- generic_bus_0_dev_1

Notice the non active usb_hub child and its 2 non active children. The first child being non-active as in this example also causes usb_get_dev_index
to return NULL when probing the first child, which results in the usb kbd
code not binding to the keyboard.

Quoted 7 lines

Quoted 1 line

Although I suspect that could be fixed.

Right, but just removing the children is a much cleaner solution, and also
makes the output of "dm tree" properly reflect reality.

True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan.

Quoted 4 lines

That seems like needless complexity. I believe that simply removing + unbinding
the children on usb_stop is the right thing to do, and it also is the KISS solution.

I'm good with the remove(), but less sure about the unbind().

The
state of 'dm tree' does not bother me,

and I worry that we then limit
our ability to use usb_find_child() to locate a device's parameters (i.e. support for more complex devices which need settings might be harder).

For now, can we just leave this alone? I really don't want to re-visit this later.

Hi,

On 06/30/2015 06:07 PM, Simon Glass wrote:

<snip>

Quoted 20 lines

Quoted 7 lines

Quoted 1 line

Instead, I wonder if we can remove the children when we probe the bus?

That should work, but I do not really see any advantage in that, removing the children is not that expensive and it feels like a kludge.

That's how it currently works, from what I can see in the code. But since there is a 'usb_started' boolean this is irrelevant.

Quoted 8 lines

Quoted 5 lines

Also, what happens to children that are in the device tree - i.e. static USB devices like WiFi? The device tree might have parameters for them. Still, that might not matter as I'm not sure that case is handled correctly today.

Quoted 7 lines

Quoted 2 lines

AFAIK there is no such thing as usb devices in devicetree, which makes sense as usb is a fully discoverable bus.

Sort-of. But as with PCI it is useful to be able to add settings for the devices in some cases. You can match them using vendor/device or interface IDs. Then the driver can access its settings.

AFAIK there is not a single example of having settings in devicetree for an usb device, since usb-devices are always 100% self describing since usb is a bus designed for hot(un)plug from the outset.

Quoted 2 lines

That's why I'm suggesting we unbind the devices that are no-longer present.

You're asking to make the code more complicated here using a what if reasoning with a "what if" which is likely to never happen.

Quoted 49 lines

Quoted 49 lines

Quoted 46 lines

Quoted 32 lines

Quoted 32 lines

Quoted 28 lines

The result of this commit is best seen in the output of "dm tree" after
plugging out an usb hub with 2 devices plugges in and plugging in a keyb.
instead, before this commit the output would be:

usb [ + ] `-- sunxi-musb
usb_hub [ ] |-- usb_hub
usb_mass_st [ ] | |-- usb_mass_storage
usb_dev_gen [ ] | `-- generic_bus_0_dev_3
usb_dev_gen [ + ] `-- generic_bus_0_dev_1

Notice the non active usb_hub child and its 2 non active children. The
first child being non-active as in this example also causes usb_get_dev_index
to return NULL when probing the first child, which results in the usb kbd
code not binding to the keyboard.

Quoted 8 lines

Quoted 1 line

Although I suspect that could be fixed.

Right, but just removing the children is a much cleaner solution, and also
makes the output of "dm tree" properly reflect reality.

True, although you also have 'usb tree' for that. Another option would be to mark devices that were found and remove the others after the scan.

Quoted 8 lines

Quoted 5 lines

That seems like needless complexity. I believe that simply removing + unbinding
the children on usb_stop is the right thing to do, and it also is the KISS
solution.

I'm good with the remove(), but less sure about the unbind().

The unbind is necessary for usb_get_dev_index() to work properly, which is necessary for proper output of "usb tree" and for driver binding to work properly, without the unbind usb-keyboards will e.g. not work in certain circumstances.

Quoted 2 lines

The
state of 'dm tree' does not bother me,

The state if dm tree is what usb_get_dev_index() works from, so if it is not in a good state, then usb_get_dev_index() will not work.

Quoted 4 lines

and I worry that we then limit
our ability to use usb_find_child() to locate a device's parameters (i.e. support for more complex devices which need settings might be harder).

Again this is a what if reasoning for a hypothetical future problem which will likely never happen, where as the broken state of the dm tree after a "usb reset" is causing real problems.

Quoted 2 lines

For now, can we just leave this alone? I really don't want to re-visit this later.

Nope we cannot leave this alone, without unbinding usb devices which no longer exist, the dm tree will be broken and with it usb_get_dev_index() and through usb_get_dev_index() the keyboard driver.

Allow usb uclass host drivers to implement usb_reset_root_port, this is used by single port usb hosts which do not emulate a hub, such as otg controllers.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/host/usb-uclass.c | 16 +++++++++++-----
 include/usb.h                 |  5 +++++
 2 files changed, 16 insertions(+), 5 deletions(-)

Quoted 4 lines · Hans de Goede

Allow usb uclass host drivers to implement usb_reset_root_port, this is used by single port usb hosts which do not emulate a hub, such as otg controllers.

Quoted 5 lines

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/host/usb-uclass.c | 16 +++++++++++-----
 include/usb.h                 |  5 +++++
 2 files changed, 16 insertions(+), 5 deletions(-)

Acked-by Simon Glass <[email protected]>

On some single port (otg) controllers there is no emulated root hub, so the first child (if any) may be one of: UCLASS_MASS_STORAGE, UCLASS_USB_DEV_GENERIC or UCLASS_USB_HUB.

All three of these (and in the future others) are suitable for our purposes, remove the check for the device being a hub, and add a check to deal with the fact that there may be no child-dev.

Signed-off-by Hans de Goede <[email protected]>

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

Quoted 8 lines · Hans de Goede

On some single port (otg) controllers there is no emulated root hub, so the first child (if any) may be one of: UCLASS_MASS_STORAGE, UCLASS_USB_DEV_GENERIC or UCLASS_USB_HUB.

All three of these (and in the future others) are suitable for our purposes, remove the check for the device being a hub, and add a check to deal with the fact that there may be no child-dev.

Quoted 5 lines

Signed-off-by Hans de Goede <[email protected]>

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

Acked-by Simon Glass <[email protected]>

Allow musb_platform_enable to return an error code and propagate it up to usb_lowlevel_init().

This allows moving the checks for an external vbus being present to be moved from platform_init to platform_enable, so that the user can unplug a charger, plug in a host adapter with a usb-device, do a "usb reset" and have things working.

This also allows adding a check for the id-pin to platform_enable, so that it can short circuit the 1s delay in usb_lowlevel_init() when no host cable is plugged in and thus waiting for a device to show up is useless.

Note that all the changes to code shared with the kernel are wrapped in the kernel.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb-new/am35x.c      |  7 +++++++
 drivers/usb/musb-new/musb_core.c  | 20 ++++++++++++++++++++
 drivers/usb/musb-new/musb_core.h  | 18 ++++++++++++++++++
 drivers/usb/musb-new/musb_dsps.c  |  6 ++++++
 drivers/usb/musb-new/musb_uboot.c |  6 +++++-
 drivers/usb/musb-new/omap2430.c   |  5 +++++
 drivers/usb/musb-new/sunxi.c      |  5 +++--
 7 files changed, 64 insertions(+), 3 deletions(-)

Quoted 16 lines · Hans de Goede

Allow musb_platform_enable to return an error code and propagate it up to usb_lowlevel_init().

This allows moving the checks for an external vbus being present to be moved from platform_init to platform_enable, so that the user can unplug a charger, plug in a host adapter with a usb-device, do a "usb reset" and have things working.

This also allows adding a check for the id-pin to platform_enable, so that it can short circuit the 1s delay in usb_lowlevel_init() when no host cable is plugged in and thus waiting for a device to show up is useless.

Note that all the changes to code shared with the kernel are wrapped in the kernel.

Signed-off-by Hans de Goede <[email protected]>

Quoted 9 lines

---
 drivers/usb/musb-new/am35x.c      |  7 +++++++
 drivers/usb/musb-new/musb_core.c  | 20 ++++++++++++++++++++
 drivers/usb/musb-new/musb_core.h  | 18 ++++++++++++++++++
 drivers/usb/musb-new/musb_dsps.c  |  6 ++++++
 drivers/usb/musb-new/musb_uboot.c |  6 +++++-
 drivers/usb/musb-new/omap2430.c   |  5 +++++
 drivers/usb/musb-new/sunxi.c      |  5 +++--
 7 files changed, 64 insertions(+), 3 deletions(-)

Acked-by Simon Glass <[email protected]>

When building with CONFIG_DM_USB=y struct usb_device does not have a parent pointer. This commit adds support to the musb code to deal with this.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb-new/musb_host.c  |  4 +++
 drivers/usb/musb-new/musb_uboot.c |  2 +-
 drivers/usb/musb-new/usb-compat.h | 70 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)

drivers/usb/musb-new/musb_host.c +4 -0

index 437309c..40b9c66 100644
@@ -2067,7 +2067,11 @@ int musb_urb_enqueue(
 
        /* precompute addressing for external hub/tt ports */
        if (musb->is_multipoint) {
+#ifndef __UBOOT__
                struct usb_device       *parent = urb->dev->parent;
+#else
+               struct usb_device       *parent = usb_dev_get_parent(urb->dev);
+#endif
 
 #ifndef __UBOOT__
                if (parent != hcd->self.root_hub) {

drivers/usb/musb-new/musb_uboot.c +1 -1

index 70e87c9..a96e8d2 100644
@@ -97,7 +97,7 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe,
                      buffer, len, setup, 0);
 
        /* Fix speed for non hub-attached devices */
-       if (!dev->parent)
+       if (!usb_dev_get_parent(dev))
                dev->speed = host_speed;
 
        return submit_urb(&hcd, &urb);

drivers/usb/musb-new/usb-compat.h +32 -0

index 50bad37..53fe4ff 100644
@@ -1,6 +1,7 @@
 #ifndef __USB_COMPAT_H__
 #define __USB_COMPAT_H__
 
+#include <dm.h>
 #include "usb.h"
 
 struct usb_hcd {

@@ -66,6 +67,68 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
        return 0;
 }
 
+#ifdef CONFIG_DM_USB
+static inline u16 find_tt(struct usb_device *udev)
+{
+       struct udevice *parent;
+       struct usb_device *uparent, *ttdev;
+
+       /*
+        * When called from usb-uclass.c: usb_scan_device() udev->dev points
+        * to the parent udevice, not the actual udevice belonging to the
+        * udev as the device is not instantiated yet. So when searching
+        * for the first usb-2 parent start with udev->dev not
+        * udev->dev->parent .
+        */
+       ttdev = udev;
+       parent = udev->dev;
+       uparent = dev_get_parentdata(parent);
+
+       while (uparent->speed != USB_SPEED_HIGH) {
+               struct udevice *dev = parent;
+
+               if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
+                       printf("musb: Error cannot find high speed parent of usb-1 device\n");
+                       return 0;
+               }
+
+               ttdev = dev_get_parentdata(dev);
+               parent = dev->parent;
+               uparent = dev_get_parentdata(parent);
+       }
+
+       return (uparent->devnum << 8) | (ttdev->portnr - 1);

drivers/usb/musb-new/usb-compat.h +28 -1

+        *
+        * If dev is an usb-bus, then we are called from usb_scan_device() for
+        * an usb-device plugged directly into the root port, return NULL.
+        */
+       if (device_get_uclass_id(udev->dev) == UCLASS_USB)
+               return NULL;
+
+       /*
+        * If these 2 are not the same we are being called from
+        * usb_scan_device() and udev itself is the parent.
+        */
+       if (dev_get_parentdata(udev->dev) != udev)
+               return udev;
+
+       /* We are being called normally, use the parent pointer */
+       if (device_get_uclass_id(parent) == UCLASS_USB_HUB)
+               return dev_get_parentdata(parent);
+
+       return NULL;
+}
+#else
 static inline u16 find_tt(struct usb_device *dev)
 {
        u8 chid;

@@ -86,4 +149,11 @@ static inline u16 find_tt(struct usb_device *dev)
 
        return (hub << 8) | chid;
 }
+
+static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
+{
+       return dev->parent;
+}
+#endif
+
 #endif /* __USB_COMPAT_H__ */
--

2.4.3

Hi Hans,

Quoted 10 lines · Hans de Goede

When building with CONFIG_DM_USB=y struct usb_device does not have a parent pointer. This commit adds support to the musb code to deal with this.

Signed-off-by Hans de Goede <[email protected]>

---
  drivers/usb/musb-new/musb_host.c  |  4 +++
  drivers/usb/musb-new/musb_uboot.c |  2 +-
  drivers/usb/musb-new/usb-compat.h | 70 +++++++++++++++++++++++++++++++++++++++
  3 files changed, 75 insertions(+), 1 deletion(-)

Acked-by Simon Glass <[email protected]>

See note below.

Quoted 85 lines

drivers/usb/musb-new/musb_host.c +4 -0

index 437309c..40b9c66 100644
@@ -2067,7 +2067,11 @@ int musb_urb_enqueue(
 
         /* precompute addressing for external hub/tt ports */
         if (musb->is_multipoint) {
+#ifndef __UBOOT__
                 struct usb_device       *parent = urb->dev->parent;
+#else
+               struct usb_device       *parent = usb_dev_get_parent(urb->dev);
+#endif
 
  #ifndef __UBOOT__
                 if (parent != hcd->self.root_hub) {

drivers/usb/musb-new/musb_uboot.c +1 -1

index 70e87c9..a96e8d2 100644
@@ -97,7 +97,7 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe,
                       buffer, len, setup, 0);
 
         /* Fix speed for non hub-attached devices */
-       if (!dev->parent)
+       if (!usb_dev_get_parent(dev))
                 dev->speed = host_speed;
 
         return submit_urb(&hcd, &urb);

drivers/usb/musb-new/usb-compat.h +41 -0

index 50bad37..53fe4ff 100644
@@ -1,6 +1,7 @@
  #ifndef __USB_COMPAT_H__
  #define __USB_COMPAT_H__
 
+#include <dm.h>
  #include "usb.h"
 
  struct usb_hcd {

@@ -66,6 +67,68 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
         return 0;
  }
 
+#ifdef CONFIG_DM_USB
+static inline u16 find_tt(struct usb_device *udev)
+{
+       struct udevice *parent;
+       struct usb_device *uparent, *ttdev;
+
+       /*
+        * When called from usb-uclass.c: usb_scan_device() udev->dev points
+        * to the parent udevice, not the actual udevice belonging to the
+        * udev as the device is not instantiated yet. So when searching
+        * for the first usb-2 parent start with udev->dev not
+        * udev->dev->parent .
+        */
+       ttdev = udev;
+       parent = udev->dev;
+       uparent = dev_get_parentdata(parent);
+
+       while (uparent->speed != USB_SPEED_HIGH) {
+               struct udevice *dev = parent;
+
+               if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
+                       printf("musb: Error cannot find high speed parent of usb-1 device\n");
+                       return 0;
+               }
+
+               ttdev = dev_get_parentdata(dev);
+               parent = dev->parent;
+               uparent = dev_get_parentdata(parent);
+       }
+
+       return (uparent->devnum << 8) | (ttdev->portnr - 1);
+}
+
+static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
+{
+       struct udevice *parent = udev->dev->parent;
+
+       /*
+        * When called from usb-uclass.c: usb_scan_device() udev->dev points
+        * to the parent udevice, not the actual udevice belonging to the

drivers/usb/musb-new/usb-compat.h static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, · line 110

Quoted 1 line

drivers/usb/musb-new/usb-compat.h +1 -0

+        * udev as the device is not instantiated yet.

Another option here is to somehow allow devices to be added before we know what they are. In this case we could bind a 'generic' USB device (UCLASS_USB_DEV_GENERIC). Then when we work out what it is, we could unbind it (without throwing away the udevice and usb_device) and have it bind again as the correct device. Something like device_morph_child().

Hi,

Quoted 121 lines · Simon Glass

Hi Hans,

Quoted 5 lines · Hans de Goede

When building with CONFIG_DM_USB=y struct usb_device does not have a parent
pointer. This commit adds support to the musb code to deal with this.

Quoted 8 lines

Signed-off-by Hans de Goede <[email protected]>

---
  drivers/usb/musb-new/musb_host.c  |  4 +++
  drivers/usb/musb-new/musb_uboot.c |  2 +-
  drivers/usb/musb-new/usb-compat.h | 70
+++++++++++++++++++++++++++++++++++++++
  3 files changed, 75 insertions(+), 1 deletion(-)

Acked-by Simon Glass <[email protected]>

See note below.

Quoted 95 lines

drivers/usb/musb-new/musb_host.c +4 -0

index 437309c..40b9c66 100644
@@ -2067,7 +2067,11 @@ int musb_urb_enqueue(
 
         /* precompute addressing for external hub/tt ports */
         if (musb->is_multipoint) {
+#ifndef __UBOOT__
                 struct usb_device       *parent = urb->dev->parent;
+#else
+               struct usb_device       *parent =
 usb_dev_get_parent(urb->dev);
+#endif
 
  #ifndef __UBOOT__

                 if (parent != hcd->self.root_hub) {

drivers/usb/musb-new/musb_uboot.c +1 -1

index 70e87c9..a96e8d2 100644
@@ -97,7 +97,7 @@ int submit_control_msg(struct usb_device *dev, unsigned
 long pipe,
                       buffer, len, setup, 0);
 
         /* Fix speed for non hub-attached devices */
-       if (!dev->parent)
+       if (!usb_dev_get_parent(dev))
                 dev->speed = host_speed;
 

         return submit_urb(&hcd, &urb);

drivers/usb/musb-new/usb-compat.h +9 -0

index 50bad37..53fe4ff 100644
@@ -1,6 +1,7 @@
  #ifndef __USB_COMPAT_H__
  #define __USB_COMPAT_H__
 
+#include <dm.h>
  #include "usb.h"
 
  struct usb_hcd {

@@ -66,6 +67,68 @@ static inline int usb_hcd_unmap_urb_for_dma(struct
 usb_hcd *hcd,
         return 0;
  }
 
+#ifdef CONFIG_DM_USB
+static inline u16 find_tt(struct usb_device *udev)
+{
+       struct udevice *parent;
+       struct usb_device *uparent, *ttdev;
+
+       /*
+        * When called from usb-uclass.c: usb_scan_device() udev->dev

points

drivers/usb/musb-new/usb-compat.h +13 -0

+        * to the parent udevice, not the actual udevice belonging to the
+        * udev as the device is not instantiated yet. So when searching
+        * for the first usb-2 parent start with udev->dev not
+        * udev->dev->parent .
+        */
+       ttdev = udev;
+       parent = udev->dev;
+       uparent = dev_get_parentdata(parent);
+
+       while (uparent->speed != USB_SPEED_HIGH) {
+               struct udevice *dev = parent;
+
+               if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB)

{
+ printf("musb: Error cannot find high speed parent
of usb-1 device\n");

drivers/usb/musb-new/usb-compat.h +12 -0

+                       return 0;
+               }
+
+               ttdev = dev_get_parentdata(dev);
+               parent = dev->parent;
+               uparent = dev_get_parentdata(parent);
+       }
+
+       return (uparent->devnum << 8) | (ttdev->portnr - 1);
+}
+
+static inline struct usb_device *usb_dev_get_parent(struct usb_device

*udev)

drivers/usb/musb-new/usb-compat.h +5 -0

+{
+       struct udevice *parent = udev->dev->parent;
+
+       /*
+        * When called from usb-uclass.c: usb_scan_device() udev->dev

points

drivers/usb/musb-new/usb-compat.h

Quoted 2 lines

drivers/usb/musb-new/usb-compat.h +2 -0

+        * to the parent udevice, not the actual udevice belonging to the
+        * udev as the device is not instantiated yet.

Quoted 6 lines

Another option here is to somehow allow devices to be added before we know what they are. In this case we could bind a 'generic' USB device (UCLASS_USB_DEV_GENERIC). Then when we work out what it is, we could unbind it (without throwing away the udevice and usb_device) and have it bind again as the correct device. Something like device_morph_child().

Right, I think that may end up being cleaner in the long term.

Regards,

Hans

Rename and wrap the usb host API public functions, this is a preparation patch for adding device-model support.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb-new/musb_uboot.c | 70 +++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 11 deletions(-)

Quoted 7 lines · Hans de Goede

Rename and wrap the usb host API public functions, this is a preparation patch for adding device-model support.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb-new/musb_uboot.c | 70 +++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 11 deletions(-)

Acked-by Simon Glass <[email protected]>

Add a musb_host_data struct to hold all the global data host related musb data. This is a preparation patch for adding device-model support.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb-new/musb_uboot.c | 107 +++++++++++++++++++-------------------
 drivers/usb/musb-new/musb_uboot.h |  24 +++++++++
 2 files changed, 77 insertions(+), 54 deletions(-)
 create mode 100644 drivers/usb/musb-new/musb_uboot.h

Hi Hans,

Quoted 9 lines · Hans de Goede

Add a musb_host_data struct to hold all the global data host related musb data. This is a preparation patch for adding device-model support.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb-new/musb_uboot.c | 107 +++++++++++++++++++-------------------
 drivers/usb/musb-new/musb_uboot.h |  24 +++++++++
 2 files changed, 77 insertions(+), 54 deletions(-)
 create mode 100644 drivers/usb/musb-new/musb_uboot.h

Acked-by Simon Glass <[email protected]>

Regards,
Simon

Add device-model support to the musb-host u-boot glue, note this only adds device-model support to the musb-core glue code, it does not add support for device-model to any of the SoC specific musb glue code.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/usb/musb-new/musb_uboot.c | 70 ++++++++++++++++++++++++++++++++++++++-
 drivers/usb/musb-new/musb_uboot.h |  4 +++
 2 files changed, 73 insertions(+), 1 deletion(-)

drivers/usb/musb-new/musb_uboot.c +24 -0

index 9875100..9b56e90 100644
@@ -21,7 +21,9 @@ struct int_queue {
        struct urb urb;
 };
 
+#ifndef CONFIG_DM_USB
 struct musb_host_data musb_host;
+#endif
 
 static void musb_host_complete_urb(struct urb *urb)
 {

@@ -244,6 +246,7 @@ int musb_lowlevel_init(struct musb_host_data *host)
        return 0;
 }
 
+#ifndef CONFIG_DM_USB
 int usb_lowlevel_stop(int index)
 {
        if (!musb_host.host) {

@@ -300,6 +303,71 @@ int usb_lowlevel_init(int index, enum usb_init_type init, void **controller)
 {
        return musb_lowlevel_init(&musb_host);
 }
+#endif /* !CONFIG_DM_USB */
+
+#ifdef CONFIG_DM_USB
+static int musb_submit_control_msg(struct udevice *dev, struct usb_device *udev,
+                                  unsigned long pipe, void *buffer, int length,
+                                  struct devrequest *setup)
+{
+       struct musb_host_data *host = dev_get_priv(dev);
+       return _musb_submit_control_msg(host, udev, pipe, buffer, length, setup);
+}
+
+static int musb_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
+                               unsigned long pipe, void *buffer, int length)
+{
+       struct musb_host_data *host = dev_get_priv(dev);
+       return _musb_submit_bulk_msg(host, udev, pipe, buffer, length);
+}
+
+static int musb_submit_int_msg(struct udevice *dev, struct usb_device *udev,
+                              unsigned long pipe, void *buffer, int length,
+                              int interval)