Fixes/Addition to use the USB Ethernet gadget with the DWC3 gadget controller

When a USB ethernet device is halted, the device driver is removed. When this happens the uclass private memory is freed and uclass_priv is set to NULL. This causes a data abort when uclass_priv->state is then set to ETH_STATE_PASSIVE.

Fix it by checking if uclass_priv is NULL before setting uclass_priv->state

Signed-off-by Jean-Jacques Hiblot <[email protected]>

Most of the time the UDC is bound to a driver when a dedicated command is executed, like 'dfu'. But the ethernet gadget driver must be bound by calling usb_ether_init() in the code otherwise the USB ethernet adapter is not visible to the ethernet core.

In DM context, the platform code should not be used to bind UDC to a particular driver, so adding a new command to bind a USB device port to a driver.

usage example:
usbdev bind 0 usb_ether
usbdev unbind 0

Signed-off-by Jean-Jacques Hiblot <[email protected]>

---

 cmd/usb.c                    | 71

+++++++++++++++++++++++++++++++++++++++++++-
drivers/core/device-remove.c | 11 +------ include/dm/device-internal.h | 15 ++++++++++ 3 files changed, 86 insertions(+), 11 deletions(-)

cmd/usb.c +5 -1

index 0ccb1b5..03245cb 100644
@@ -14,6 +14,8 @@
 #include <command.h>
 #include <console.h>
 #include <dm.h>
+#include <dm/lists.h>
+#include <dm/device-internal.h>
 #include <dm/uclass-internal.h>
 #include <memalign.h>
 #include <asm/byteorder.h>

@@ -753,7 +755,6 @@ U_BOOT_CMD(
 #endif /* CONFIG_USB_STORAGE */
 );
 
-
 #ifdef CONFIG_USB_STORAGE
 U_BOOT_CMD(
 	usbboot,	3,	1,	do_usbboot,

@@ -761,3 +762,71 @@ U_BOOT_CMD(
 	"loadAddr dev:part"
 );
 #endif /* CONFIG_USB_STORAGE */
+
+#ifdef CONFIG_DM_USB_DEV
+int do_usbdev(cmd_tbl_t *cmdtp, int flag, int argc, char * const

argv[]) +{

cmd/usb.c +19 -0

+	struct udevice *dev;
+	struct udevice *usb_dev;
+	int port;
+	int ret;
+	bool bind;
+	static const char * const supported_drivers[] = {
+#ifdef CONFIG_USB_ETHER
+		"usb_ether",
+#endif
+	};
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	if ((strncmp(argv[1], "bind", 4) == 0) && (argc == 4)) {
+		port = simple_strtoul(argv[2], NULL, 10);
+		printf("Binding USB port %d to %s\n", port, argv[3]);
+		bind = true;
+	} else if ((strncmp(argv[1], "unbind", 6) == 0) && (argc ==

3)) {

cmd/usb.c +4 -0

+		port = simple_strtoul(argv[2], NULL, 10);
+		printf("Unbinding USB port %d\n", port);
+		bind = false;
+	} else if ((strncmp(argv[1], "list", 4) == 0) && (argc ==

2)) {

cmd/usb.c +11 -0

+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(supported_drivers); i++)
+			printf("%s\n", supported_drivers[i]);
+
+		return CMD_RET_SUCCESS;
+	} else {
+		return CMD_RET_USAGE;
+	}
+
+	ret = uclass_find_device(UCLASS_USB_DEV_GENERIC, port,

&usb_dev);

cmd/usb.c +7 -0

+	if (!usb_dev || ret) {
+		printf("Cannot find UDC %d\n", port);
+		return CMD_RET_FAILURE;
+	}
+
+	if (bind) {
+		ret = device_bind_driver(usb_dev, argv[3], "gadget",

&dev);

cmd/usb.c +23 -0

+		if (!dev || ret) {
+			printf("Unable to bind. err:%d\n", ret);
+			return CMD_RET_FAILURE;
+		}
+	} else {
+		ret = device_chld_unbind(usb_dev);
+		if (ret) {
+			printf("Unable to bind. err:%d\n", ret);
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+U_BOOT_CMD(
+	usbdev, 4, 0, do_usbdev,
+	"USB gadget driver",
+	"bind dev# driver- bind the USB device port to a driver\n"
+	"unbind dev# - unbind the USB device port to a driver\n"
+	"list - display the list of available gadget drivers"
+);
+#endif /*  CONFIG_DM_USB_DEV */

(quoted diff) +0 -0

b/drivers/core/device-remove.c index 1cf2278..b0b5ea3 100644

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

@@ -17,16 +17,7 @@
 #include <dm/uclass-internal.h>
 #include <dm/util.h>
 
-/**
- * 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
- */

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

-static int device_chld_unbind(struct udevice *dev)
+int device_chld_unbind(struct udevice *dev)
 {
 	struct udevice *pos, *n;
 	int ret, saved_ret = 0;

(quoted diff) +0 -0

b/include/dm/device-internal.h index 5a4d50c..b4f44c8 100644

include/dm/device-internal.h +2 -0

@@ -120,6 +120,21 @@ int device_unbind(struct udevice *dev);
 static inline int device_unbind(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
+ */

include/dm/device-internal.h +4 -0

+#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
+int device_chld_unbind(struct udevice *dev);
+#else
+static inline int device_chld_unbind(struct udevice *dev) { return

0; } +#endif
+
#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
void device_free(struct udevice *dev);
#else

Hi Jean-Jacques,

Quoted 9 lines

Most of the time the UDC is bound to a driver when a dedicated command is executed, like 'dfu'. But the ethernet gadget driver must be bound by calling usb_ether_init() in the code otherwise the USB ethernet adapter is not visible to the ethernet core.

In DM context, the platform code should not be used to bind UDC to a particular driver, so adding a new command to bind a USB device port to a driver.

Quoted 3 lines

usage example:
usbdev bind 0 usb_ether
usbdev unbind 0

I would prefer a comment from Simon (so adding him to CC) - as it looks to me that we shall try to use DM to avoid adding separate commands for binding.

Hi,

Quoted 11 lines · Lukasz Majewski

Hi Jean-Jacques,

Quoted 9 lines

Most of the time the UDC is bound to a driver when a dedicated command is executed, like 'dfu'. But the ethernet gadget driver must be bound by calling usb_ether_init() in the code otherwise the USB ethernet adapter is not visible to the ethernet core.

In DM context, the platform code should not be used to bind UDC to a particular driver, so adding a new command to bind a USB device port to a driver.

Quoted 6 lines

Quoted 3 lines

usage example:
usbdev bind 0 usb_ether
usbdev unbind 0

I would prefer a comment from Simon (so adding him to CC) - as it looks to me that we shall try to use DM to avoid adding separate commands for binding.

We could perhaps introduce 'bind' and 'unbind' commands with similar arguments?

Regards,
Simon

Hi,

Quoted 20 lines · Simon Glass

Hi,

Quoted 16 lines · Lukasz Majewski

Hi Jean-Jacques,

Quoted 12 lines

Most of the time the UDC is bound to a driver when a dedicated command is executed, like 'dfu'. But the ethernet gadget driver must be bound by calling usb_ether_init() in the code otherwise the USB ethernet adapter is not visible to the ethernet core.

In DM context, the platform code should not be used to bind UDC to a particular driver, so adding a new command to bind a USB device port to a driver.

usage example:
usbdev bind 0 usb_ether
usbdev unbind 0

Quoted 6 lines

Quoted 3 lines

I would prefer a comment from Simon (so adding him to CC) - as it looks to me that we shall try to use DM to avoid adding separate commands for binding.

We could perhaps introduce 'bind' and 'unbind' commands with similar arguments?

What kind of parameters should be used to identify the device to bind ? I see 2 possible options:
- node paths: bind /opc/omap_dwc3 at 48380000/usb at 483d0000 usb_ether
- device class + index: bind usb_dev 0 usb_ether.

The last one looks a lot like what I proposed, only more generic, but requires creating a table to match a string with a UCLASS id. The first is more precise but IMO less user friendly.

Hi Jean-Jacques,

Quoted 26 lines · Jean-Jacques Hiblot

Hi,

Quoted 14 lines · Simon Glass

Hi,

Quoted 11 lines · Lukasz Majewski

Hi Jean-Jacques,

Quoted 9 lines

Most of the time the UDC is bound to a driver when a dedicated command is executed, like 'dfu'. But the ethernet gadget driver must be bound by calling usb_ether_init() in the code otherwise the USB ethernet adapter is not visible to the ethernet core.

In DM context, the platform code should not be used to bind UDC to a particular driver, so adding a new command to bind a USB device port to a driver.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

usage example:
usbdev bind 0 usb_ether
usbdev unbind 0

I would prefer a comment from Simon (so adding him to CC) - as it looks to me that we shall try to use DM to avoid adding separate commands for binding.

We could perhaps introduce 'bind' and 'unbind' commands with similar arguments?

Quoted 8 lines

What kind of parameters should be used to identify the device to bind ? I see 2 possible options:
- node paths: bind /opc/omap_dwc3 at 48380000/usb at 483d0000 usb_ether
- device class + index: bind usb_dev 0 usb_ether.

The last one looks a lot like what I proposed, only more generic, but requires creating a table to match a string with a UCLASS id. The first is more precise but IMO less user friendly.

We can look up a uclass by name, so your second open should work OK. It matches the current U-Boot approach pretty well two since most commands work this way.

However, I have sometimes thought (with driver model) of supporting the first option as a fallback.

You could in fact have a function that supports both:

1. Option 1 - it does a global search for a device with that DT node
2. Option 2 - it does a uclass_get_device_by_seq() call

I agree that option 2 is likely to be much preferred in normal use.

Regards,
Simon

Quoted 52 lines · Simon Glass

Hi Jean-Jacques,

Quoted 32 lines · Jean-Jacques Hiblot

Hi,

Quoted 20 lines · Simon Glass

Hi,

Quoted 16 lines · Lukasz Majewski

Hi Jean-Jacques,

Quoted 12 lines

Most of the time the UDC is bound to a driver when a dedicated command is executed, like 'dfu'. But the ethernet gadget driver must be bound by calling usb_ether_init() in the code otherwise the USB ethernet adapter is not visible to the ethernet core.

In DM context, the platform code should not be used to bind UDC to a particular driver, so adding a new command to bind a USB device port to a driver.

usage example:
usbdev bind 0 usb_ether
usbdev unbind 0

Quoted 6 lines

Quoted 3 lines

I would prefer a comment from Simon (so adding him to CC) - as it looks to me that we shall try to use DM to avoid adding separate commands for binding.

We could perhaps introduce 'bind' and 'unbind' commands with similar arguments?

Quoted 8 lines

What kind of parameters should be used to identify the device to bind ? I see 2 possible options:
- node paths: bind /opc/omap_dwc3 at 48380000/usb at 483d0000 usb_ether
- device class + index: bind usb_dev 0 usb_ether.

The last one looks a lot like what I proposed, only more generic, but requires creating a table to match a string with a UCLASS id. The first is more precise but IMO less user friendly.

We can look up a uclass by name, so your second open should work OK. It matches the current U-Boot approach pretty well two since most commands work this way.

However, I have sometimes thought (with driver model) of supporting the first option as a fallback.

Quoted 6 lines

You could in fact have a function that supports both:

1. Option 1 - it does a global search for a device with that DT node
2. Option 2 - it does a uclass_get_device_by_seq() call

I agree that option 2 is likely to be much preferred in normal use.

I've been working on this and have come up with a slightly different interface:

bind <node path> <driver name>
unbind <node path>
bind <class name> <index> <driver name>
unbind <class name> <index>

Interface with node path:
It is a symmetric interface:
example:
bind /ocp/ocp2scp at 483e8000 generic_simple_bus
unbind /ocp/ocp2scp at 483e8000

Interface with class/index:
This is by essence an asymmetric interface: the class/index pair is not the same for binding as for unbinding example:
bind usb_dev_generic 0 usb_ether
unbind eth 1

While it makes sense, it may be a bit harder to use because of the asymmetry

Quoted 84 lines · Jean-Jacques Hiblot

Quoted 52 lines · Simon Glass

Hi Jean-Jacques,

Quoted 32 lines · Jean-Jacques Hiblot

Hi,

Quoted 20 lines · Simon Glass

Hi,

Quoted 16 lines · Lukasz Majewski

Hi Jean-Jacques,

Quoted 12 lines

Most of the time the UDC is bound to a driver when a dedicated command is executed, like 'dfu'. But the ethernet gadget driver must be bound by calling usb_ether_init() in the code otherwise the USB ethernet adapter is not visible to the ethernet core.

In DM context, the platform code should not be used to bind UDC to a particular driver, so adding a new command to bind a USB device port to a driver.

usage example:
usbdev bind 0 usb_ether
usbdev unbind 0

Quoted 6 lines

Quoted 3 lines

I would prefer a comment from Simon (so adding him to CC) - as it looks to me that we shall try to use DM to avoid adding separate commands for binding.

We could perhaps introduce 'bind' and 'unbind' commands with similar arguments?

Quoted 8 lines

What kind of parameters should be used to identify the device to bind ? I see 2 possible options:
- node paths: bind /opc/omap_dwc3 at 48380000/usb at 483d0000 usb_ether
- device class + index: bind usb_dev 0 usb_ether.

The last one looks a lot like what I proposed, only more generic, but requires creating a table to match a string with a UCLASS id. The first is more precise but IMO less user friendly.

We can look up a uclass by name, so your second open should work OK. It matches the current U-Boot approach pretty well two since most commands work this way.

However, I have sometimes thought (with driver model) of supporting the first option as a fallback.

Quoted 6 lines

You could in fact have a function that supports both:

1. Option 1 - it does a global search for a device with that DT node
2. Option 2 - it does a uclass_get_device_by_seq() call

I agree that option 2 is likely to be much preferred in normal use.

I've been working on this and have come up with a slightly different interface:

bind <node path> <driver name>
unbind <node path>
bind <class name> <index> <driver name>
unbind <class name> <index>

Interface with node path:
It is a symmetric interface:
example:
bind /ocp/ocp2scp at 483e8000 generic_simple_bus
unbind /ocp/ocp2scp at 483e8000

Interface with class/index:
This is by essence an asymmetric interface: the class/index pair is not the same for binding as for unbinding example:
bind usb_dev_generic 0 usb_ether
unbind eth 1

Quoted 1 line

While it makes sense, it may be a bit harder to use because of the asymmetry

That seems OK to me. I added some more people for comment. Please add anyone else you can think of who might have ideas.