Re: [U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
On 06/14/2018 09:11 AM, Simon Glass wrote: On 14 June 2018 at 09:02, Jean-Jacques Hiblot wrote: On 13/06/2018 03:29, Simon Glass wrote: Hi Jean-Jacques, On 12 June 2018 at 03:31, Jean-Jacques Hiblot wrote: Hi, On 08/06/2018 23:59, Simon Glass wrote: Hi, On 7 June 2018 at 01:39, Lukasz Majewski wrote: Hi Jean-Jacques, 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 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@4838/usb@483d 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. I've been working on this and have come up with a slightly different interface: bind unbind bind unbind Interface with node path: It is a symmetric interface: example: bind /ocp/ocp2scp@483e8000 generic_simple_bus unbind /ocp/ocp2scp@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 That seems OK to me. I added some more people for comment. Please add anyone else you can think of who might have ideas. Why wouldn't the unbind arguments always match the bind arguments: bind /ocp/ocp2scp@483e8000 generic_simple_bus unbind /ocp/ocp2scp@483e8000 bind usb_dev_generic 0 usb_ether unbind usb_dev_generic 0 usb_ether One benefit here is that it's completely symmetric, so easier to specify and understand. Also, one might imagine a future where we could do: bind usb_dev_generic 0 usb_ether bind usb_dev_generic 0 usb_acm # Here, can use both Ethernet and serial (console) over this port unbind usb_dev_generic 0 usb_acm unbind usb_dev_generic 0 usb_ether (Although perhaps in this case, should we actually be binding not usb_ether and usb_acm directly, but rather binding usb_gadget, and somehow configuring what protocols usb_gadget exposes separately beforehand? For example, see how the kernel's USB gadget sysfs control works.) ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
On 14 June 2018 at 09:02, Jean-Jacques Hiblot wrote: > > > > On 13/06/2018 03:29, Simon Glass wrote: >> >> Hi Jean-Jacques, >> >> On 12 June 2018 at 03:31, Jean-Jacques Hiblot wrote: >>> >>> Hi, >>> >>> >>> On 08/06/2018 23:59, Simon Glass wrote: Hi, On 7 June 2018 at 01:39, Lukasz Majewski wrote: > > Hi Jean-Jacques, > >> 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 > > 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@4838/usb@483d 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. > > I've been working on this and have come up with a slightly different > interface: > > bind > unbind > bind > unbind > > > Interface with node path: > It is a symmetric interface: > example: > bind /ocp/ocp2scp@483e8000 generic_simple_bus > unbind /ocp/ocp2scp@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 That seems OK to me. I added some more people for comment. Please add anyone else you can think of who might have ideas. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
On 13/06/2018 03:29, Simon Glass wrote: Hi Jean-Jacques, On 12 June 2018 at 03:31, Jean-Jacques Hiblot wrote: Hi, On 08/06/2018 23:59, Simon Glass wrote: Hi, On 7 June 2018 at 01:39, Lukasz Majewski wrote: Hi Jean-Jacques, 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 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@4838/usb@483d 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. I've been working on this and have come up with a slightly different interface: bind unbind bind unbind Interface with node path: It is a symmetric interface: example: bind /ocp/ocp2scp@483e8000 generic_simple_bus unbind /ocp/ocp2scp@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 JJ Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
Hi Jean-Jacques, On 12 June 2018 at 03:31, Jean-Jacques Hiblot wrote: > Hi, > > > On 08/06/2018 23:59, Simon Glass wrote: >> >> Hi, >> >> On 7 June 2018 at 01:39, Lukasz Majewski wrote: >>> >>> Hi Jean-Jacques, >>> 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 >>> >>> 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@4838/usb@483d 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
Hi, On 08/06/2018 23:59, Simon Glass wrote: Hi, On 7 June 2018 at 01:39, Lukasz Majewski wrote: Hi Jean-Jacques, 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 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@4838/usb@483d 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. JJ Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
Hi, On 7 June 2018 at 01:39, Lukasz Majewski wrote: > Hi Jean-Jacques, > >> 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 > > 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
Hi Jean-Jacques,
> 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
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.
>
> Signed-off-by: Jean-Jacques Hiblot
>
> ---
>
> cmd/usb.c| 71
> +++-
> drivers/core/device-remove.c | 11 +--
> include/dm/device-internal.h | 15 ++ 3 files changed, 86
> insertions(+), 11 deletions(-)
>
> diff --git a/cmd/usb.c b/cmd/usb.c
> index 0ccb1b5..03245cb 100644
> --- a/cmd/usb.c
> +++ b/cmd/usb.c
> @@ -14,6 +14,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -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[]) +{
> + 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)) {
> + 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)) {
> + 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,
> _dev);
> + 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",
> );
> + 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 */
> diff --git a/drivers/core/device-remove.c
> b/drivers/core/device-remove.c index 1cf2278..b0b5ea3 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -17,16 +17,7 @@
> #include
> #include
>
> -/**
> - * 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)
> {
> struct udevice *pos, *n;
> int ret, saved_ret = 0;
> diff --git a/include/dm/device-internal.h
> b/include/dm/device-internal.h index 5a4d50c..b4f44c8 100644
> --- a/include/dm/device-internal.h
> +++ b/include/dm/device-internal.h
> @@ -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
[U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port
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
---
cmd/usb.c| 71 +++-
drivers/core/device-remove.c | 11 +--
include/dm/device-internal.h | 15 ++
3 files changed, 86 insertions(+), 11 deletions(-)
diff --git a/cmd/usb.c b/cmd/usb.c
index 0ccb1b5..03245cb 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -14,6 +14,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -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[])
+{
+ 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)) {
+ 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)) {
+ 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, _dev);
+ 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", );
+ 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 */
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index 1cf2278..b0b5ea3 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -17,16 +17,7 @@
#include
#include
-/**
- * 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)
{
struct udevice *pos, *n;
int ret, saved_ret = 0;
diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
index 5a4d50c..b4f44c8 100644
--- a/include/dm/device-internal.h
+++ b/include/dm/device-internal.h
@@ -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
+ */
+#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
--
2.7.4
