[U-Boot] [PATCH v1 0/3] Fixes/Addition to use the USB Ethernet gadget with the DWC3 gadget controller

[U-Boot] [PATCH v1 0/3] Fixes/Addition to use the USB Ethernet gadget with the DWC3 gadget controller

[Jean-Jacques Hiblot]

This series adds a new command to bind the USB ether driver to a UDC, and 2
fixes to be able to use USB Ethernet gadget with the dwc3 driver.


Jean-Jacques Hiblot (3):
  usb: gadget: Add bcdDevice for the DWC3 USB Gadget Controller
  net: eth-uclass: Fix for DM USB ethernet support
  cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC
    port

 cmd/usb.c                         | 71 ++++++++++++++++++++++++++++++++++++++-
 drivers/core/device-remove.c      | 11 +-----
 drivers/usb/gadget/gadget_chips.h |  2 ++
 include/dm/device-internal.h      | 15 +++++++++
 net/eth-uclass.c                  |  3 +-
 5 files changed, 90 insertions(+), 12 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH v1 1/3] usb: gadget: Add bcdDevice for the DWC3 USB Gadget Controller

[Jean-Jacques Hiblot]

Add an entry in usb_gadget_controller_number() for the DWC3 gadget
controller. Without it, it is not possible to bind the USB Ethernet driver.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

 drivers/usb/gadget/gadget_chips.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
index f320708..9b0ad2e 100644
--- a/drivers/usb/gadget/gadget_chips.h
+++ b/drivers/usb/gadget/gadget_chips.h
@@ -214,5 +214,7 @@ static inline int usb_gadget_controller_number(struct usb_gadget *gadget)
 		return 0x21;
 	else if (gadget_is_fotg210(gadget))
 		return 0x22;
+	else if (gadget_is_dwc3(gadget))
+		return 0x23;
 	return -ENOENT;
 }
-- 
2.7.4

[U-Boot] [PATCH v1 2/3] net: eth-uclass: Fix for DM USB ethernet support

[Jean-Jacques Hiblot]

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 <jjhiblot@ti.com>
---

 net/eth-uclass.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index d20a1cf..f71b5cd 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -312,7 +312,8 @@ void eth_halt(void)
 
 	eth_get_ops(current)->stop(current);
 	priv = current->uclass_priv;
-	priv->state = ETH_STATE_PASSIVE;
+	if (priv)
+		priv->state = ETH_STATE_PASSIVE;
 }
 
 int eth_is_active(struct udevice *dev)
-- 
2.7.4

[U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Jean-Jacques Hiblot]

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 <jjhiblot@ti.com>

---

 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 <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[])
+{
+	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, &usb_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", &dev);
+		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 <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
- */
-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

[U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Lukasz Majewski]

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 <jjhiblot@ti.com>
> 
> ---
> 
>  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 <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[]) +{
> +	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,
> &usb_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",
> &dev);
> +		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 <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
> - */
> -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




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180607/ca93dfc6/attachment.sig>

[U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Simon Glass]

Hi,

On 7 June 2018 at 01:39, Lukasz Majewski <lukma@denx.de> 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] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Jean-Jacques Hiblot]

Hi,


On 08/06/2018 23:59, Simon Glass wrote:
> Hi,
>
> On 7 June 2018 at 01:39, Lukasz Majewski <lukma@denx.de> 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 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.

JJ
>
> Regards,
> Simon
>

[U-Boot] [PATCH v1 2/3] net: eth-uclass: Fix for DM USB ethernet support

[Joe Hershberger]

On Mon, Jun 4, 2018 at 5:31 AM, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> 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 <jjhiblot@ti.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Simon Glass]

Hi Jean-Jacques,

On 12 June 2018 at 03:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> Hi,
>
>
> On 08/06/2018 23:59, Simon Glass wrote:
>>
>> Hi,
>>
>> On 7 June 2018 at 01:39, Lukasz Majewski <lukma@denx.de> 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 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

[U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Jean-Jacques Hiblot]

On 13/06/2018 03:29, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On 12 June 2018 at 03:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> Hi,
>>
>>
>> On 08/06/2018 23:59, Simon Glass wrote:
>>> Hi,
>>>
>>> On 7 June 2018 at 01:39, Lukasz Majewski <lukma@denx.de> 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 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.
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

JJ

>
> Regards,
> Simon
>

[U-Boot] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Simon Glass]

On 14 June 2018 at 09:02, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
>
>
> On 13/06/2018 03:29, Simon Glass wrote:
>>
>> Hi Jean-Jacques,
>>
>> On 12 June 2018 at 03:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 08/06/2018 23:59, Simon Glass wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 7 June 2018 at 01:39, Lukasz Majewski <lukma@denx.de> 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 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.
>
> 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

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] [PATCH v1 3/3] cmd: usb gadget: Add a command to bind a USB gadget driver to a UDC port

[Stephen Warren]

On 06/14/2018 09:11 AM, Simon Glass wrote:
> On 14 June 2018 at 09:02, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>
>>
>>
>> On 13/06/2018 03:29, Simon Glass wrote:
>>>
>>> Hi Jean-Jacques,
>>>
>>> On 12 June 2018 at 03:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>> On 08/06/2018 23:59, Simon Glass wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 7 June 2018 at 01:39, Lukasz Majewski <lukma@denx.de> 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 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.
>>
>> 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
> 
> 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 at 483e8000 generic_simple_bus
unbind /ocp/ocp2scp at 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.)