[PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

Extend the driver core to perform lookup by both OF node and driver
bound to the node. Use this to look up specific device instances to
unbind from nodes in the unbind command. One example where this is
needed is USB peripheral controller, which may have multiple gadget
drivers bound to it. The unbind command has to select that specific
gadget driver instance to unbind from the controller, not unbind the
controller driver itself from the controller.

USB ethernet gadget usage looks as follows with this change. Notice
the extra 'usb_ether' addition in the 'unbind' command at the end.
"
bind /soc/usb-otg@49000000 usb_ether
setenv ethact usb_ether
setenv loadaddr 0xc2000000
setenv ipaddr 10.0.0.2
setenv serverip 10.0.0.1
setenv netmask 255.255.255.0
tftpboot 0xc2000000 10.0.0.1:test.file
unbind /soc/usb-otg@49000000 usb_ether
"

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Simon Glass <sjg@chromium.org>
---
V2: No change
V3: No change
V4: No change
---
 cmd/bind.c            | 10 +++++-----
 drivers/core/device.c | 20 +++++++++++++++-----
 include/dm/device.h   | 17 +++++++++++++++++
 3 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/cmd/bind.c b/cmd/bind.c
index 4d1b7885e60..3137cdf6cb5 100644
--- a/cmd/bind.c
+++ b/cmd/bind.c
@@ -162,7 +162,7 @@ static int bind_by_node_path(const char *path, const char *drv_name)
 	return 0;
 }
 
-static int unbind_by_node_path(const char *path)
+static int unbind_by_node_path(const char *path, const char *drv_name)
 {
 	struct udevice *dev;
 	int ret;
@@ -174,7 +174,7 @@ static int unbind_by_node_path(const char *path)
 		return -EINVAL;
 	}
 
-	ret = device_find_global_by_ofnode(ofnode, &dev);
+	ret = device_find_global_by_ofnode_driver(ofnode, drv_name, &dev);
 
 	if (!dev || ret) {
 		printf("Cannot find a device with path %s\n", path);
@@ -214,9 +214,9 @@ static int do_bind_unbind(struct cmd_tbl *cmdtp, int flag, int argc,
 			return CMD_RET_USAGE;
 		ret = bind_by_node_path(argv[1], argv[2]);
 	} else if (by_node && !bind) {
-		if (argc != 2)
+		if (argc != 2 && argc != 3)
 			return CMD_RET_USAGE;
-		ret = unbind_by_node_path(argv[1]);
+		ret = unbind_by_node_path(argv[1], argv[2]);
 	} else if (!by_node && bind) {
 		int index = (argc > 2) ? dectoul(argv[2], NULL) : 0;
 
@@ -251,7 +251,7 @@ U_BOOT_CMD(
 U_BOOT_CMD(
 	unbind,	4,	0,	do_bind_unbind,
 	"Unbind a device from a driver",
-	"<node path>\n"
+	"<node path> [<driver>]\n"
 	"unbind <class> <index>\n"
 	"unbind <class> <index> <driver>\n"
 );
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 6e26b64fb81..52fceb69341 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -877,15 +877,17 @@ int device_get_child_by_of_offset(const struct udevice *parent, int node,
 }
 
 static struct udevice *_device_find_global_by_ofnode(struct udevice *parent,
-						     ofnode ofnode)
+						     ofnode ofnode,
+						     const char *drv)
 {
 	struct udevice *dev, *found;
 
-	if (ofnode_equal(dev_ofnode(parent), ofnode))
+	if (ofnode_equal(dev_ofnode(parent), ofnode) &&
+	    (!drv || (drv && !strcmp(parent->driver->name, drv))))
 		return parent;
 
 	device_foreach_child(dev, parent) {
-		found = _device_find_global_by_ofnode(dev, ofnode);
+		found = _device_find_global_by_ofnode(dev, ofnode, drv);
 		if (found)
 			return found;
 	}
@@ -895,7 +897,15 @@ static struct udevice *_device_find_global_by_ofnode(struct udevice *parent,
 
 int device_find_global_by_ofnode(ofnode ofnode, struct udevice **devp)
 {
-	*devp = _device_find_global_by_ofnode(gd->dm_root, ofnode);
+	*devp = _device_find_global_by_ofnode(gd->dm_root, ofnode, NULL);
+
+	return *devp ? 0 : -ENOENT;
+}
+
+int device_find_global_by_ofnode_driver(ofnode ofnode, const char *drv,
+					struct udevice **devp)
+{
+	*devp = _device_find_global_by_ofnode(gd->dm_root, ofnode, drv);
 
 	return *devp ? 0 : -ENOENT;
 }
@@ -904,7 +914,7 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
 {
 	struct udevice *dev;
 
-	dev = _device_find_global_by_ofnode(gd->dm_root, ofnode);
+	dev = _device_find_global_by_ofnode(gd->dm_root, ofnode, NULL);
 	return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
 }
 
diff --git a/include/dm/device.h b/include/dm/device.h
index b86bf90609b..5f05ae0924f 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -748,6 +748,23 @@ int device_get_child_by_of_offset(const struct udevice *parent, int of_offset,
 
 int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
 
+/**
+ * device_find_global_by_ofnode_driver() - Get a device based on ofnode and driver
+ *
+ * Locates a device by its device tree ofnode and driver currently bound to
+ * it, searching globally throughout the all driver model devices.
+ *
+ * The device is NOT probed
+ *
+ * @node: Device tree ofnode to find
+ * @drv: Driver name bound to device
+ * @devp: Returns pointer to device if found, otherwise this is set to NULL
+ * Return: 0 if OK, -ve on error
+ */
+
+int device_find_global_by_ofnode_driver(ofnode node, const char *drv,
+					struct udevice **devp);
+
 /**
  * device_get_global_by_ofnode() - Get a device based on ofnode
  *
-- 
2.40.1

[PATCH v4 2/4] usb: gadget: ether: Inline functions used once

[Marek Vasut]

These functions here are only ever called once since drop of non-DM
networking code. Inline them. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Simon Glass <sjg@chromium.org>
---
V2: No change
V3: No change
V4: No change
---
 drivers/usb/gadget/ether.c | 48 +++++++-------------------------------
 1 file changed, 9 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 85c971e4c43..88c656c4dc0 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -2273,10 +2273,11 @@ fail:
 }
 
 /*-------------------------------------------------------------------------*/
-static void _usb_eth_halt(struct ether_priv *priv);
+static void usb_eth_stop(struct udevice *dev);
 
-static int _usb_eth_init(struct ether_priv *priv)
+static int usb_eth_start(struct udevice *udev)
 {
+	struct ether_priv *priv = dev_get_priv(udev);
 	struct eth_dev *dev = &priv->ethdev;
 	struct usb_gadget *gadget;
 	unsigned long ts;
@@ -2347,12 +2348,13 @@ static int _usb_eth_init(struct ether_priv *priv)
 	rx_submit(dev, dev->rx_req, 0);
 	return 0;
 fail:
-	_usb_eth_halt(priv);
+	usb_eth_stop(udev);
 	return -1;
 }
 
-static int _usb_eth_send(struct ether_priv *priv, void *packet, int length)
+static int usb_eth_send(struct udevice *udev, void *packet, int length)
 {
+	struct ether_priv *priv = dev_get_priv(udev);
 	int			retval;
 	void			*rndis_pkt = NULL;
 	struct eth_dev		*dev = &priv->ethdev;
@@ -2419,15 +2421,9 @@ drop:
 	return -ENOMEM;
 }
 
-static int _usb_eth_recv(struct ether_priv *priv)
-{
-	usb_gadget_handle_interrupts(0);
-
-	return 0;
-}
-
-static void _usb_eth_halt(struct ether_priv *priv)
+static void usb_eth_stop(struct udevice *udev)
 {
+	struct ether_priv *priv = dev_get_priv(udev);
 	struct eth_dev *dev = &priv->ethdev;
 
 	/* If the gadget not registered, simple return */
@@ -2459,31 +2455,12 @@ static void _usb_eth_halt(struct ether_priv *priv)
 	usb_gadget_release(0);
 }
 
-static int usb_eth_start(struct udevice *dev)
-{
-	struct ether_priv *priv = dev_get_priv(dev);
-
-	return _usb_eth_init(priv);
-}
-
-static int usb_eth_send(struct udevice *dev, void *packet, int length)
-{
-	struct ether_priv *priv = dev_get_priv(dev);
-
-	return _usb_eth_send(priv, packet, length);
-}
-
 static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 {
 	struct ether_priv *priv = dev_get_priv(dev);
 	struct eth_dev *ethdev = &priv->ethdev;
-	int ret;
 
-	ret = _usb_eth_recv(priv);
-	if (ret) {
-		pr_err("error packet receive\n");
-		return ret;
-	}
+	usb_gadget_handle_interrupts(0);
 
 	if (packet_received) {
 		if (ethdev->rx_req) {
@@ -2509,13 +2486,6 @@ static int usb_eth_free_pkt(struct udevice *dev, uchar *packet,
 	return rx_submit(ethdev, ethdev->rx_req, 0);
 }
 
-static void usb_eth_stop(struct udevice *dev)
-{
-	struct ether_priv *priv = dev_get_priv(dev);
-
-	_usb_eth_halt(priv);
-}
-
 static int usb_eth_probe(struct udevice *dev)
 {
 	struct ether_priv *priv = dev_get_priv(dev);
-- 
2.40.1

[PATCH v4 3/4] usb: gadget: ether: Move probe function above driver structure

[Marek Vasut]

Move the driver probe function above the driver structure, so it
can be placed alongside other related functions, like upcoming
remove function. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Simon Glass <sjg@chromium.org>
---
V2: No change
V3: No change
V4: No change
---
 drivers/usb/gadget/ether.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 88c656c4dc0..2040b5b5081 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -2486,20 +2486,6 @@ static int usb_eth_free_pkt(struct udevice *dev, uchar *packet,
 	return rx_submit(ethdev, ethdev->rx_req, 0);
 }
 
-static int usb_eth_probe(struct udevice *dev)
-{
-	struct ether_priv *priv = dev_get_priv(dev);
-	struct eth_pdata *pdata = dev_get_plat(dev);
-
-	priv->netdev = dev;
-	l_priv = priv;
-
-	get_ether_addr(CONFIG_USBNET_DEV_ADDR, pdata->enetaddr);
-	eth_env_set_enetaddr("usbnet_devaddr", pdata->enetaddr);
-
-	return 0;
-}
-
 static const struct eth_ops usb_eth_ops = {
 	.start		= usb_eth_start,
 	.send		= usb_eth_send,
@@ -2528,6 +2514,20 @@ int usb_ether_init(void)
 	return 0;
 }
 
+static int usb_eth_probe(struct udevice *dev)
+{
+	struct ether_priv *priv = dev_get_priv(dev);
+	struct eth_pdata *pdata = dev_get_plat(dev);
+
+	priv->netdev = dev;
+	l_priv = priv;
+
+	get_ether_addr(CONFIG_USBNET_DEV_ADDR, pdata->enetaddr);
+	eth_env_set_enetaddr("usbnet_devaddr", pdata->enetaddr);
+
+	return 0;
+}
+
 U_BOOT_DRIVER(eth_usb) = {
 	.name	= "usb_ether",
 	.id	= UCLASS_ETH,
-- 
2.40.1

[PATCH v4 4/4] usb: gadget: ether: Handle gadget driver registration in probe and remove

[Marek Vasut]

Move the ethernet gadget driver registration and removal from ethernet
bind and unbind callbacks into driver DM probe and remove callbacks.
This way, when the driver is bound, which is triggered deliberately
using 'bind' command, the USB ethernet gadget driver is instantiated
and bound to the matching UDC. In reverse, when the driver is unbound,
which is again triggered deliberately using 'unbind' command, the USB
ethernet gadget driver instance is removed.

Effectively, this now behaves like running either 'ums' or 'dfu' or
any other commands utilizing USB gadget functionality.

This also drops use of usb_gadget_release() and moves the use of
usb_gadget_initialize() into usb_ether_init() used only by legacy
platforms that do not use 'bind' command properly yet. Those have
no place in drivers.

Signed-off-by: Marek Vasut <marex@denx.de>
---
NOTE: This must be thoroughly tested.
---
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Simon Glass <sjg@chromium.org>
---
V2: Fix up return value handling in probe
V3: Reinstate usb_gadget_initialize() in usb_ether_init() to retain
    this obscure workaround for legacy platforms that fail to use
    bind command
V4: Call usb_gadget_release() in unbind callback to further help
    legacy platforms
---
 drivers/usb/gadget/ether.c | 97 ++++++++++++++++++++------------------
 1 file changed, 52 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 2040b5b5081..5ff06d3814b 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -2281,49 +2281,8 @@ static int usb_eth_start(struct udevice *udev)
 	struct eth_dev *dev = &priv->ethdev;
 	struct usb_gadget *gadget;
 	unsigned long ts;
-	int ret;
 	unsigned long timeout = USB_CONNECT_TIMEOUT;
 
-	ret = usb_gadget_initialize(0);
-	if (ret)
-		return ret;
-
-	/* Configure default mac-addresses for the USB ethernet device */
-#ifdef CONFIG_USBNET_DEV_ADDR
-	strlcpy(dev_addr, CONFIG_USBNET_DEV_ADDR, sizeof(dev_addr));
-#endif
-#ifdef CONFIG_USBNET_HOST_ADDR
-	strlcpy(host_addr, CONFIG_USBNET_HOST_ADDR, sizeof(host_addr));
-#endif
-	/* Check if the user overruled the MAC addresses */
-	if (env_get("usbnet_devaddr"))
-		strlcpy(dev_addr, env_get("usbnet_devaddr"),
-			sizeof(dev_addr));
-
-	if (env_get("usbnet_hostaddr"))
-		strlcpy(host_addr, env_get("usbnet_hostaddr"),
-			sizeof(host_addr));
-
-	if (!is_eth_addr_valid(dev_addr)) {
-		pr_err("Need valid 'usbnet_devaddr' to be set");
-		goto fail;
-	}
-	if (!is_eth_addr_valid(host_addr)) {
-		pr_err("Need valid 'usbnet_hostaddr' to be set");
-		goto fail;
-	}
-
-	priv->eth_driver.speed		= DEVSPEED;
-	priv->eth_driver.bind		= eth_bind;
-	priv->eth_driver.unbind		= eth_unbind;
-	priv->eth_driver.setup		= eth_setup;
-	priv->eth_driver.reset		= eth_disconnect;
-	priv->eth_driver.disconnect	= eth_disconnect;
-	priv->eth_driver.suspend	= eth_suspend;
-	priv->eth_driver.resume		= eth_resume;
-	if (usb_gadget_register_driver(&priv->eth_driver) < 0)
-		goto fail;
-
 	dev->network_started = 0;
 
 	packet_received = 0;
@@ -2450,9 +2409,6 @@ static void usb_eth_stop(struct udevice *udev)
 		usb_gadget_handle_interrupts(0);
 		dev->network_started = 0;
 	}
-
-	usb_gadget_unregister_driver(&priv->eth_driver);
-	usb_gadget_release(0);
 }
 
 static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
@@ -2511,7 +2467,7 @@ int usb_ether_init(void)
 		return ret;
 	}
 
-	return 0;
+	return usb_gadget_initialize(0);
 }
 
 static int usb_eth_probe(struct udevice *dev)
@@ -2525,6 +2481,55 @@ static int usb_eth_probe(struct udevice *dev)
 	get_ether_addr(CONFIG_USBNET_DEV_ADDR, pdata->enetaddr);
 	eth_env_set_enetaddr("usbnet_devaddr", pdata->enetaddr);
 
+	/* Configure default mac-addresses for the USB ethernet device */
+#ifdef CONFIG_USBNET_DEV_ADDR
+	strlcpy(dev_addr, CONFIG_USBNET_DEV_ADDR, sizeof(dev_addr));
+#endif
+#ifdef CONFIG_USBNET_HOST_ADDR
+	strlcpy(host_addr, CONFIG_USBNET_HOST_ADDR, sizeof(host_addr));
+#endif
+	/* Check if the user overruled the MAC addresses */
+	if (env_get("usbnet_devaddr"))
+		strlcpy(dev_addr, env_get("usbnet_devaddr"),
+			sizeof(dev_addr));
+
+	if (env_get("usbnet_hostaddr"))
+		strlcpy(host_addr, env_get("usbnet_hostaddr"),
+			sizeof(host_addr));
+
+	if (!is_eth_addr_valid(dev_addr)) {
+		pr_err("Need valid 'usbnet_devaddr' to be set");
+		return -EINVAL;
+	}
+	if (!is_eth_addr_valid(host_addr)) {
+		pr_err("Need valid 'usbnet_hostaddr' to be set");
+		return -EINVAL;
+	}
+
+	priv->eth_driver.speed		= DEVSPEED;
+	priv->eth_driver.bind		= eth_bind;
+	priv->eth_driver.unbind		= eth_unbind;
+	priv->eth_driver.setup		= eth_setup;
+	priv->eth_driver.reset		= eth_disconnect;
+	priv->eth_driver.disconnect	= eth_disconnect;
+	priv->eth_driver.suspend	= eth_suspend;
+	priv->eth_driver.resume		= eth_resume;
+	return usb_gadget_register_driver(&priv->eth_driver);
+}
+
+static int usb_eth_remove(struct udevice *dev)
+{
+	struct ether_priv *priv = dev_get_priv(dev);
+
+	usb_gadget_unregister_driver(&priv->eth_driver);
+
+	return 0;
+}
+
+static int usb_eth_unbind(struct udevice *dev)
+{
+	usb_gadget_release(0);
+
 	return 0;
 }
 
@@ -2532,6 +2537,8 @@ U_BOOT_DRIVER(eth_usb) = {
 	.name	= "usb_ether",
 	.id	= UCLASS_ETH,
 	.probe	= usb_eth_probe,
+	.remove	= usb_eth_remove,
+	.unbind	= usb_eth_unbind,
 	.ops	= &usb_eth_ops,
 	.priv_auto	= sizeof(struct ether_priv),
 	.plat_auto	= sizeof(struct eth_pdata),
-- 
2.40.1

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Simon Glass]

Hi Marek,

On Wed, 2 Aug 2023 at 06:47, Marek Vasut <marex@denx.de> wrote:
>
> Extend the driver core to perform lookup by both OF node and driver
> bound to the node. Use this to look up specific device instances to
> unbind from nodes in the unbind command. One example where this is
> needed is USB peripheral controller, which may have multiple gadget
> drivers bound to it. The unbind command has to select that specific
> gadget driver instance to unbind from the controller, not unbind the
> controller driver itself from the controller.
>
> USB ethernet gadget usage looks as follows with this change. Notice
> the extra 'usb_ether' addition in the 'unbind' command at the end.
> "
> bind /soc/usb-otg@49000000 usb_ether
> setenv ethact usb_ether
> setenv loadaddr 0xc2000000
> setenv ipaddr 10.0.0.2
> setenv serverip 10.0.0.1
> setenv netmask 255.255.255.0
> tftpboot 0xc2000000 10.0.0.1:test.file
> unbind /soc/usb-otg@49000000 usb_ether
> "
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> V2: No change
> V3: No change
> V4: No change
> ---
>  cmd/bind.c            | 10 +++++-----
>  drivers/core/device.c | 20 +++++++++++++++-----
>  include/dm/device.h   | 17 +++++++++++++++++
>  3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/cmd/bind.c b/cmd/bind.c
> index 4d1b7885e60..3137cdf6cb5 100644
> --- a/cmd/bind.c
> +++ b/cmd/bind.c
> @@ -162,7 +162,7 @@ static int bind_by_node_path(const char *path, const char *drv_name)
>         return 0;
>  }
>
> -static int unbind_by_node_path(const char *path)
> +static int unbind_by_node_path(const char *path, const char *drv_name)

Can you add a function comment? I am wondering what drm_name is for

>  {
>         struct udevice *dev;
>         int ret;
> @@ -174,7 +174,7 @@ static int unbind_by_node_path(const char *path)
>                 return -EINVAL;
>         }
>
> -       ret = device_find_global_by_ofnode(ofnode, &dev);
> +       ret = device_find_global_by_ofnode_driver(ofnode, drv_name, &dev);
>
>         if (!dev || ret) {
>                 printf("Cannot find a device with path %s\n", path);
> @@ -214,9 +214,9 @@ static int do_bind_unbind(struct cmd_tbl *cmdtp, int flag, int argc,
>                         return CMD_RET_USAGE;
>                 ret = bind_by_node_path(argv[1], argv[2]);
>         } else if (by_node && !bind) {
> -               if (argc != 2)
> +               if (argc != 2 && argc != 3)

how about: argv < 2

>                         return CMD_RET_USAGE;
> -               ret = unbind_by_node_path(argv[1]);
> +               ret = unbind_by_node_path(argv[1], argv[2]);

but if argc is 2, how can we access argv[2]? Is it because we accept
NULL? In that case, please add a comment.

>         } else if (!by_node && bind) {
>                 int index = (argc > 2) ? dectoul(argv[2], NULL) : 0;
>
> @@ -251,7 +251,7 @@ U_BOOT_CMD(
>  U_BOOT_CMD(
>         unbind, 4,      0,      do_bind_unbind,
>         "Unbind a device from a driver",
> -       "<node path>\n"
> +       "<node path> [<driver>]\n"
>         "unbind <class> <index>\n"
>         "unbind <class> <index> <driver>\n"
>  );
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 6e26b64fb81..52fceb69341 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -877,15 +877,17 @@ int device_get_child_by_of_offset(const struct udevice *parent, int node,
>  }
>
>  static struct udevice *_device_find_global_by_ofnode(struct udevice *parent,
> -                                                    ofnode ofnode)
> +                                                    ofnode ofnode,
> +                                                    const char *drv)
>  {
>         struct udevice *dev, *found;
>
> -       if (ofnode_equal(dev_ofnode(parent), ofnode))
> +       if (ofnode_equal(dev_ofnode(parent), ofnode) &&
> +           (!drv || (drv && !strcmp(parent->driver->name, drv))))
>                 return parent;
>
>         device_foreach_child(dev, parent) {
> -               found = _device_find_global_by_ofnode(dev, ofnode);
> +               found = _device_find_global_by_ofnode(dev, ofnode, drv);
>                 if (found)
>                         return found;
>         }
> @@ -895,7 +897,15 @@ static struct udevice *_device_find_global_by_ofnode(struct udevice *parent,
>
>  int device_find_global_by_ofnode(ofnode ofnode, struct udevice **devp)
>  {
> -       *devp = _device_find_global_by_ofnode(gd->dm_root, ofnode);
> +       *devp = _device_find_global_by_ofnode(gd->dm_root, ofnode, NULL);
> +
> +       return *devp ? 0 : -ENOENT;
> +}
> +
> +int device_find_global_by_ofnode_driver(ofnode ofnode, const char *drv,
> +                                       struct udevice **devp)
> +{
> +       *devp = _device_find_global_by_ofnode(gd->dm_root, ofnode, drv);
>
>         return *devp ? 0 : -ENOENT;
>  }
> @@ -904,7 +914,7 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
>  {
>         struct udevice *dev;
>
> -       dev = _device_find_global_by_ofnode(gd->dm_root, ofnode);
> +       dev = _device_find_global_by_ofnode(gd->dm_root, ofnode, NULL);
>         return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
>  }
>
> diff --git a/include/dm/device.h b/include/dm/device.h
> index b86bf90609b..5f05ae0924f 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -748,6 +748,23 @@ int device_get_child_by_of_offset(const struct udevice *parent, int of_offset,
>
>  int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
>
> +/**
> + * device_find_global_by_ofnode_driver() - Get a device based on ofnode and driver
> + *
> + * Locates a device by its device tree ofnode and driver currently bound to
> + * it, searching globally throughout the all driver model devices.
> + *
> + * The device is NOT probed
> + *
> + * @node: Device tree ofnode to find
> + * @drv: Driver name bound to device
> + * @devp: Returns pointer to device if found, otherwise this is set to NULL
> + * Return: 0 if OK, -ve on error
> + */
> +
> +int device_find_global_by_ofnode_driver(ofnode node, const char *drv,
> +                                       struct udevice **devp);
> +
>  /**
>   * device_get_global_by_ofnode() - Get a device based on ofnode
>   *
> --
> 2.40.1
>

I suppose the test is in another patch, but it is often better to put
it in the same patch.

Regards,
Simon

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/2/23 23:31, Simon Glass wrote:
> Hi Marek,
> 
> On Wed, 2 Aug 2023 at 06:47, Marek Vasut <marex@denx.de> wrote:
>>
>> Extend the driver core to perform lookup by both OF node and driver
>> bound to the node. Use this to look up specific device instances to
>> unbind from nodes in the unbind command. One example where this is
>> needed is USB peripheral controller, which may have multiple gadget
>> drivers bound to it. The unbind command has to select that specific
>> gadget driver instance to unbind from the controller, not unbind the
>> controller driver itself from the controller.
>>
>> USB ethernet gadget usage looks as follows with this change. Notice
>> the extra 'usb_ether' addition in the 'unbind' command at the end.
>> "
>> bind /soc/usb-otg@49000000 usb_ether
>> setenv ethact usb_ether
>> setenv loadaddr 0xc2000000
>> setenv ipaddr 10.0.0.2
>> setenv serverip 10.0.0.1
>> setenv netmask 255.255.255.0
>> tftpboot 0xc2000000 10.0.0.1:test.file
>> unbind /soc/usb-otg@49000000 usb_ether
>> "
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Kevin Hilman <khilman@baylibre.com>
>> Cc: Lukasz Majewski <lukma@denx.de>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>> V2: No change
>> V3: No change
>> V4: No change
>> ---
>>   cmd/bind.c            | 10 +++++-----
>>   drivers/core/device.c | 20 +++++++++++++++-----
>>   include/dm/device.h   | 17 +++++++++++++++++
>>   3 files changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/cmd/bind.c b/cmd/bind.c
>> index 4d1b7885e60..3137cdf6cb5 100644
>> --- a/cmd/bind.c
>> +++ b/cmd/bind.c
>> @@ -162,7 +162,7 @@ static int bind_by_node_path(const char *path, const char *drv_name)
>>          return 0;
>>   }
>>
>> -static int unbind_by_node_path(const char *path)
>> +static int unbind_by_node_path(const char *path, const char *drv_name)
> 
> Can you add a function comment? I am wondering what drm_name is for

drv_name means driver name.

>>   {
>>          struct udevice *dev;
>>          int ret;
>> @@ -174,7 +174,7 @@ static int unbind_by_node_path(const char *path)
>>                  return -EINVAL;
>>          }
>>
>> -       ret = device_find_global_by_ofnode(ofnode, &dev);
>> +       ret = device_find_global_by_ofnode_driver(ofnode, drv_name, &dev);
>>
>>          if (!dev || ret) {
>>                  printf("Cannot find a device with path %s\n", path);
>> @@ -214,9 +214,9 @@ static int do_bind_unbind(struct cmd_tbl *cmdtp, int flag, int argc,
>>                          return CMD_RET_USAGE;
>>                  ret = bind_by_node_path(argv[1], argv[2]);
>>          } else if (by_node && !bind) {
>> -               if (argc != 2)
>> +               if (argc != 2 && argc != 3)
> 
> how about: argv < 2

No.

>>                          return CMD_RET_USAGE;
>> -               ret = unbind_by_node_path(argv[1]);
>> +               ret = unbind_by_node_path(argv[1], argv[2]);
> 
> but if argc is 2, how can we access argv[2]? Is it because we accept
> NULL? In that case, please add a comment.

We accept NULL.

[...]

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:

> Extend the driver core to perform lookup by both OF node and driver
> bound to the node. Use this to look up specific device instances to
> unbind from nodes in the unbind command. One example where this is
> needed is USB peripheral controller, which may have multiple gadget
> drivers bound to it. The unbind command has to select that specific
> gadget driver instance to unbind from the controller, not unbind the
> controller driver itself from the controller.
> 
> USB ethernet gadget usage looks as follows with this change. Notice
> the extra 'usb_ether' addition in the 'unbind' command at the end.
> "
> bind /soc/usb-otg@49000000 usb_ether
> setenv ethact usb_ether
> setenv loadaddr 0xc2000000
> setenv ipaddr 10.0.0.2
> setenv serverip 10.0.0.1
> setenv netmask 255.255.255.0
> tftpboot 0xc2000000 10.0.0.1:test.file
> unbind /soc/usb-otg@49000000 usb_ether

This extra parameter does not seem to work on the BBBW. I cannot select
the "usb_ether" driver like you do.

Good news though, I am now able to use fastboot, but it is not
straightforward:

Here is my sequence right after the boot (reducing the dm tree output
to the usb nodes for clarity):

=> dm tree     
 misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
 usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
 ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
 bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
 usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
=> fastboot usb 0
couldn't find an available UDC
g_dnl_register: failed!, error: -19
exit not allowed from main input shell.
=> unbind /ocp/usb@47400000/usb@47401000 usb_ether
Cannot find a device with path /ocp/usb@47400000/usb@47401000
=> unbind /ocp/usb@47400000/usb@47401000          
=> dm tree
 misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
 usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
=> fastboot usb 0                                 
=> bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral 
=> dm tree                                               
 misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
 usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
 usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000
=> fastboot usb 0                                        
musb-hdrc: peripheral reset irq lost!
# works! (the irq-related line above as always been there)

So now, how do we make this process easy/understandable?

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/4/23 09:00, Miquel Raynal wrote:
> Hi Marek,
> 
> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:
> 
>> Extend the driver core to perform lookup by both OF node and driver
>> bound to the node. Use this to look up specific device instances to
>> unbind from nodes in the unbind command. One example where this is
>> needed is USB peripheral controller, which may have multiple gadget
>> drivers bound to it. The unbind command has to select that specific
>> gadget driver instance to unbind from the controller, not unbind the
>> controller driver itself from the controller.
>>
>> USB ethernet gadget usage looks as follows with this change. Notice
>> the extra 'usb_ether' addition in the 'unbind' command at the end.
>> "
>> bind /soc/usb-otg@49000000 usb_ether
>> setenv ethact usb_ether
>> setenv loadaddr 0xc2000000
>> setenv ipaddr 10.0.0.2
>> setenv serverip 10.0.0.1
>> setenv netmask 255.255.255.0
>> tftpboot 0xc2000000 10.0.0.1:test.file
>> unbind /soc/usb-otg@49000000 usb_ether
> 
> This extra parameter does not seem to work on the BBBW. I cannot select
> the "usb_ether" driver like you do.
> 
> Good news though, I am now able to use fastboot, but it is not
> straightforward:
> 
> Here is my sequence right after the boot (reducing the dm tree output
> to the usb nodes for clarity):
> 
> => dm tree
>   misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>   usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
>   ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
>   bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
>   usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
> => fastboot usb 0
> couldn't find an available UDC
> g_dnl_register: failed!, error: -19

That is expected and not a bug, since the beagle explicitly binds USB 
ethernet to MUSB gadget in board file, which is legacy deprecated way.

> exit not allowed from main input shell.
> => unbind /ocp/usb@47400000/usb@47401000 usb_ether

Does

=> unbind ethernet 0

work ?

If so, 1/4 in this series can be skipped altogether.

You likely won't even need the rebinding of ti-musb-peripheral anymore.

> Cannot find a device with path /ocp/usb@47400000/usb@47401000
> => unbind /ocp/usb@47400000/usb@47401000
> => dm tree
>   misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>   usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
> => fastboot usb 0
> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> => dm tree
>   misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>   usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
>   usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000
> => fastboot usb 0
> musb-hdrc: peripheral reset irq lost!
> # works! (the irq-related line above as always been there)
> 
> So now, how do we make this process easy/understandable?

What would be your proposal ?

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3667 bytes --]

On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:
> On 8/4/23 09:00, Miquel Raynal wrote:
> > Hi Marek,
> > 
> > marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:
> > 
> > > Extend the driver core to perform lookup by both OF node and driver
> > > bound to the node. Use this to look up specific device instances to
> > > unbind from nodes in the unbind command. One example where this is
> > > needed is USB peripheral controller, which may have multiple gadget
> > > drivers bound to it. The unbind command has to select that specific
> > > gadget driver instance to unbind from the controller, not unbind the
> > > controller driver itself from the controller.
> > > 
> > > USB ethernet gadget usage looks as follows with this change. Notice
> > > the extra 'usb_ether' addition in the 'unbind' command at the end.
> > > "
> > > bind /soc/usb-otg@49000000 usb_ether
> > > setenv ethact usb_ether
> > > setenv loadaddr 0xc2000000
> > > setenv ipaddr 10.0.0.2
> > > setenv serverip 10.0.0.1
> > > setenv netmask 255.255.255.0
> > > tftpboot 0xc2000000 10.0.0.1:test.file
> > > unbind /soc/usb-otg@49000000 usb_ether
> > 
> > This extra parameter does not seem to work on the BBBW. I cannot select
> > the "usb_ether" driver like you do.
> > 
> > Good news though, I am now able to use fastboot, but it is not
> > straightforward:
> > 
> > Here is my sequence right after the boot (reducing the dm tree output
> > to the usb nodes for clarity):
> > 
> > => dm tree
> >   misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >   usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
> >   ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
> >   bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
> >   usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
> > => fastboot usb 0
> > couldn't find an available UDC
> > g_dnl_register: failed!, error: -19
> 
> That is expected and not a bug, since the beagle explicitly binds USB
> ethernet to MUSB gadget in board file, which is legacy deprecated way.

So, we should do away with, probably all of arch_misc_init() in
arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.

> > exit not allowed from main input shell.
> > => unbind /ocp/usb@47400000/usb@47401000 usb_ether
> 
> Does
> 
> => unbind ethernet 0
> 
> work ?
> 
> If so, 1/4 in this series can be skipped altogether.
> 
> You likely won't even need the rebinding of ti-musb-peripheral anymore.
> 
> > Cannot find a device with path /ocp/usb@47400000/usb@47401000
> > => unbind /ocp/usb@47400000/usb@47401000
> > => dm tree
> >   misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >   usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
> > => fastboot usb 0
> > => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> > => dm tree
> >   misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >   usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> >   usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000
> > => fastboot usb 0
> > musb-hdrc: peripheral reset irq lost!
> > # works! (the irq-related line above as always been there)
> > 
> > So now, how do we make this process easy/understandable?
> 
> What would be your proposal ?

Well, what's needed / is it possible to get to the point where we don't
_need_ to call bind/unbind for each of these cases? Is there something
we're supposed to be setting in the DT that we aren't?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/4/23 17:01, Tom Rini wrote:
> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:
>> On 8/4/23 09:00, Miquel Raynal wrote:
>>> Hi Marek,
>>>
>>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:
>>>
>>>> Extend the driver core to perform lookup by both OF node and driver
>>>> bound to the node. Use this to look up specific device instances to
>>>> unbind from nodes in the unbind command. One example where this is
>>>> needed is USB peripheral controller, which may have multiple gadget
>>>> drivers bound to it. The unbind command has to select that specific
>>>> gadget driver instance to unbind from the controller, not unbind the
>>>> controller driver itself from the controller.
>>>>
>>>> USB ethernet gadget usage looks as follows with this change. Notice
>>>> the extra 'usb_ether' addition in the 'unbind' command at the end.
>>>> "
>>>> bind /soc/usb-otg@49000000 usb_ether
>>>> setenv ethact usb_ether
>>>> setenv loadaddr 0xc2000000
>>>> setenv ipaddr 10.0.0.2
>>>> setenv serverip 10.0.0.1
>>>> setenv netmask 255.255.255.0
>>>> tftpboot 0xc2000000 10.0.0.1:test.file
>>>> unbind /soc/usb-otg@49000000 usb_ether
>>>
>>> This extra parameter does not seem to work on the BBBW. I cannot select
>>> the "usb_ether" driver like you do.
>>>
>>> Good news though, I am now able to use fastboot, but it is not
>>> straightforward:
>>>
>>> Here is my sequence right after the boot (reducing the dm tree output
>>> to the usb nodes for clarity):
>>>
>>> => dm tree
>>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>    usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
>>>    ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
>>>    bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
>>>    usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
>>> => fastboot usb 0
>>> couldn't find an available UDC
>>> g_dnl_register: failed!, error: -19
>>
>> That is expected and not a bug, since the beagle explicitly binds USB
>> ethernet to MUSB gadget in board file, which is legacy deprecated way.
> 
> So, we should do away with, probably all of arch_misc_init() in
> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.

Yes

>>> exit not allowed from main input shell.
>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether
>>
>> Does
>>
>> => unbind ethernet 0
>>
>> work ?
>>
>> If so, 1/4 in this series can be skipped altogether.
>>
>> You likely won't even need the rebinding of ti-musb-peripheral anymore.
>>
>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000
>>> => unbind /ocp/usb@47400000/usb@47401000
>>> => dm tree
>>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>    usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
>>> => fastboot usb 0
>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
>>> => dm tree
>>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>    usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
>>>    usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000
>>> => fastboot usb 0
>>> musb-hdrc: peripheral reset irq lost!
>>> # works! (the irq-related line above as always been there)
>>>
>>> So now, how do we make this process easy/understandable?
>>
>> What would be your proposal ?
> 
> Well, what's needed / is it possible to get to the point where we don't
> _need_ to call bind/unbind for each of these cases? Is there something
> we're supposed to be setting in the DT that we aren't?

You do need to unbind the ethernet before using fastboot, always, with 
the 'unbind ethernet 0', you dont need the peripheral unbind/rebind 
part, so it should behave like before.

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi,

marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200:

> On 8/4/23 17:01, Tom Rini wrote:
> > On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:  
> >> On 8/4/23 09:00, Miquel Raynal wrote:  
> >>> Hi Marek,
> >>>
> >>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:
> >>>  
> >>>> Extend the driver core to perform lookup by both OF node and driver
> >>>> bound to the node. Use this to look up specific device instances to
> >>>> unbind from nodes in the unbind command. One example where this is
> >>>> needed is USB peripheral controller, which may have multiple gadget
> >>>> drivers bound to it. The unbind command has to select that specific
> >>>> gadget driver instance to unbind from the controller, not unbind the
> >>>> controller driver itself from the controller.
> >>>>
> >>>> USB ethernet gadget usage looks as follows with this change. Notice
> >>>> the extra 'usb_ether' addition in the 'unbind' command at the end.
> >>>> "
> >>>> bind /soc/usb-otg@49000000 usb_ether
> >>>> setenv ethact usb_ether
> >>>> setenv loadaddr 0xc2000000
> >>>> setenv ipaddr 10.0.0.2
> >>>> setenv serverip 10.0.0.1
> >>>> setenv netmask 255.255.255.0
> >>>> tftpboot 0xc2000000 10.0.0.1:test.file
> >>>> unbind /soc/usb-otg@49000000 usb_ether  
> >>>
> >>> This extra parameter does not seem to work on the BBBW. I cannot select
> >>> the "usb_ether" driver like you do.
> >>>
> >>> Good news though, I am now able to use fastboot, but it is not
> >>> straightforward:
> >>>
> >>> Here is my sequence right after the boot (reducing the dm tree output
> >>> to the usb nodes for clarity):
> >>>  
> >>> => dm tree  
> >>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>    usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
> >>>    ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
> >>>    bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
> >>>    usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> >>> => fastboot usb 0  
> >>> couldn't find an available UDC
> >>> g_dnl_register: failed!, error: -19  
> >>
> >> That is expected and not a bug, since the beagle explicitly binds USB
> >> ethernet to MUSB gadget in board file, which is legacy deprecated way.  
> > 
> > So, we should do away with, probably all of arch_misc_init() in
> > arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.  
> 
> Yes
> 
> >>> exit not allowed from main input shell.  
> >>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether  
> >>
> >> Does
> >>  
> >> => unbind ethernet 0  
> >>
> >> work ?
> >>
> >> If so, 1/4 in this series can be skipped altogether.
> >>
> >> You likely won't even need the rebinding of ti-musb-peripheral anymore.
> >>  
> >>> Cannot find a device with path /ocp/usb@47400000/usb@47401000  
> >>> => unbind /ocp/usb@47400000/usb@47401000
> >>> => dm tree  
> >>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>    usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> >>> => fastboot usb 0
> >>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> >>> => dm tree  
> >>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>    usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> >>>    usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000  
> >>> => fastboot usb 0  
> >>> musb-hdrc: peripheral reset irq lost!
> >>> # works! (the irq-related line above as always been there)
> >>>
> >>> So now, how do we make this process easy/understandable?  
> >>
> >> What would be your proposal ? 

At least I would appreciate:
- to select CMD_BIND "by default" when relevant
- to make the fastboot error more readable for the regular user

If you want to get rid of all the legacy code, I am not opposed,
really, but that must not be the user who is responsible for
understanding what changed in the "core". It must be very easily
accessible to understand that now:
- manual binding/unbinding is needed
- which driver must be used when

> > Well, what's needed / is it possible to get to the point where we don't
> > _need_ to call bind/unbind for each of these cases? Is there something
> > we're supposed to be setting in the DT that we aren't?  
> 
> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.

And for my own understanding, why don't we need to bind a fastboot
gadget?

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/4/23 17:12, Miquel Raynal wrote:
> Hi,
> 
> marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200:
> 
>> On 8/4/23 17:01, Tom Rini wrote:
>>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:
>>>> On 8/4/23 09:00, Miquel Raynal wrote:
>>>>> Hi Marek,
>>>>>
>>>>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:
>>>>>   
>>>>>> Extend the driver core to perform lookup by both OF node and driver
>>>>>> bound to the node. Use this to look up specific device instances to
>>>>>> unbind from nodes in the unbind command. One example where this is
>>>>>> needed is USB peripheral controller, which may have multiple gadget
>>>>>> drivers bound to it. The unbind command has to select that specific
>>>>>> gadget driver instance to unbind from the controller, not unbind the
>>>>>> controller driver itself from the controller.
>>>>>>
>>>>>> USB ethernet gadget usage looks as follows with this change. Notice
>>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end.
>>>>>> "
>>>>>> bind /soc/usb-otg@49000000 usb_ether
>>>>>> setenv ethact usb_ether
>>>>>> setenv loadaddr 0xc2000000
>>>>>> setenv ipaddr 10.0.0.2
>>>>>> setenv serverip 10.0.0.1
>>>>>> setenv netmask 255.255.255.0
>>>>>> tftpboot 0xc2000000 10.0.0.1:test.file
>>>>>> unbind /soc/usb-otg@49000000 usb_ether
>>>>>
>>>>> This extra parameter does not seem to work on the BBBW. I cannot select
>>>>> the "usb_ether" driver like you do.
>>>>>
>>>>> Good news though, I am now able to use fastboot, but it is not
>>>>> straightforward:
>>>>>
>>>>> Here is my sequence right after the boot (reducing the dm tree output
>>>>> to the usb nodes for clarity):
>>>>>   
>>>>> => dm tree
>>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>>>     usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
>>>>>     ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
>>>>>     bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
>>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
>>>>> => fastboot usb 0
>>>>> couldn't find an available UDC
>>>>> g_dnl_register: failed!, error: -19
>>>>
>>>> That is expected and not a bug, since the beagle explicitly binds USB
>>>> ethernet to MUSB gadget in board file, which is legacy deprecated way.
>>>
>>> So, we should do away with, probably all of arch_misc_init() in
>>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.
>>
>> Yes
>>
>>>>> exit not allowed from main input shell.
>>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether
>>>>
>>>> Does
>>>>   
>>>> => unbind ethernet 0
>>>>
>>>> work ?
>>>>
>>>> If so, 1/4 in this series can be skipped altogether.
>>>>
>>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.

Did you test this yet ?

>>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000
>>>>> => unbind /ocp/usb@47400000/usb@47401000
>>>>> => dm tree
>>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
>>>>> => fastboot usb 0
>>>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
>>>>> => dm tree
>>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>>>     usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
>>>>>     usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000
>>>>> => fastboot usb 0
>>>>> musb-hdrc: peripheral reset irq lost!
>>>>> # works! (the irq-related line above as always been there)
>>>>>
>>>>> So now, how do we make this process easy/understandable?
>>>>
>>>> What would be your proposal ?
> 
> At least I would appreciate:
> - to select CMD_BIND "by default" when relevant
> - to make the fastboot error more readable for the regular user

Since with this 'unbind ethernet 0' this is orthogonal to this series, 
send separate patches, thanks.

> If you want to get rid of all the legacy code, I am not opposed,
> really, but that must not be the user who is responsible for
> understanding what changed in the "core". It must be very easily
> accessible to understand that now:
> - manual binding/unbinding is needed
> - which driver must be used when

When my spare time permits.

>>> Well, what's needed / is it possible to get to the point where we don't
>>> _need_ to call bind/unbind for each of these cases? Is there something
>>> we're supposed to be setting in the DT that we aren't?
>>
>> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.
> 
> And for my own understanding, why don't we need to bind a fastboot
> gadget?

The fastboot command does that internally .

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200:

> On 8/4/23 17:12, Miquel Raynal wrote:
> > Hi,
> > 
> > marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200:
> >   
> >> On 8/4/23 17:01, Tom Rini wrote:  
> >>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:  
> >>>> On 8/4/23 09:00, Miquel Raynal wrote:  
> >>>>> Hi Marek,
> >>>>>
> >>>>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:  
> >>>>>   >>>>>> Extend the driver core to perform lookup by both OF node and driver  
> >>>>>> bound to the node. Use this to look up specific device instances to
> >>>>>> unbind from nodes in the unbind command. One example where this is
> >>>>>> needed is USB peripheral controller, which may have multiple gadget
> >>>>>> drivers bound to it. The unbind command has to select that specific
> >>>>>> gadget driver instance to unbind from the controller, not unbind the
> >>>>>> controller driver itself from the controller.
> >>>>>>
> >>>>>> USB ethernet gadget usage looks as follows with this change. Notice
> >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end.
> >>>>>> "
> >>>>>> bind /soc/usb-otg@49000000 usb_ether
> >>>>>> setenv ethact usb_ether
> >>>>>> setenv loadaddr 0xc2000000
> >>>>>> setenv ipaddr 10.0.0.2
> >>>>>> setenv serverip 10.0.0.1
> >>>>>> setenv netmask 255.255.255.0
> >>>>>> tftpboot 0xc2000000 10.0.0.1:test.file
> >>>>>> unbind /soc/usb-otg@49000000 usb_ether  
> >>>>>
> >>>>> This extra parameter does not seem to work on the BBBW. I cannot select
> >>>>> the "usb_ether" driver like you do.
> >>>>>
> >>>>> Good news though, I am now able to use fastboot, but it is not
> >>>>> straightforward:
> >>>>>
> >>>>> Here is my sequence right after the boot (reducing the dm tree output
> >>>>> to the usb nodes for clarity):  
> >>>>>   >>>>> => dm tree  
> >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>>>     usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
> >>>>>     ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
> >>>>>     bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
> >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> >>>>> => fastboot usb 0  
> >>>>> couldn't find an available UDC
> >>>>> g_dnl_register: failed!, error: -19  
> >>>>
> >>>> That is expected and not a bug, since the beagle explicitly binds USB
> >>>> ethernet to MUSB gadget in board file, which is legacy deprecated way.  
> >>>
> >>> So, we should do away with, probably all of arch_misc_init() in
> >>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.  
> >>
> >> Yes
> >>  
> >>>>> exit not allowed from main input shell.  
> >>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether  
> >>>>
> >>>> Does  
> >>>>   >>>> => unbind ethernet 0  
> >>>>
> >>>> work ?
> >>>>
> >>>> If so, 1/4 in this series can be skipped altogether.
> >>>>
> >>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.  
> 
> Did you test this yet ?

Not yet, I'll send you the feedback once done.

> >>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000  
> >>>>> => unbind /ocp/usb@47400000/usb@47401000
> >>>>> => dm tree  
> >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> >>>>> => fastboot usb 0
> >>>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> >>>>> => dm tree  
> >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>>>     usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> >>>>>     usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000  
> >>>>> => fastboot usb 0  
> >>>>> musb-hdrc: peripheral reset irq lost!
> >>>>> # works! (the irq-related line above as always been there)
> >>>>>
> >>>>> So now, how do we make this process easy/understandable?  
> >>>>
> >>>> What would be your proposal ?  
> > 
> > At least I would appreciate:
> > - to select CMD_BIND "by default" when relevant
> > - to make the fastboot error more readable for the regular user  
> 
> Since with this 'unbind ethernet 0' this is orthogonal to this series, send separate patches, thanks.

This is not orthogonal, I am sorry.

version X:
- tftp works "out of the box"
- fastboot works "out of the box"
version X+1:
- tftp works "out of the box"
- fastboot returns an obscure error

1/ If we now *need* the bind/unbind commands, the series must take care
   of it.
2/ Without proper error message you just break fastboot for most
   regular users (basically everyone but few U-Boot devs).

> > If you want to get rid of all the legacy code, I am not opposed,
> > really, but that must not be the user who is responsible for
> > understanding what changed in the "core". It must be very easily
> > accessible to understand that now:
> > - manual binding/unbinding is needed
> > - which driver must be used when  
> 
> When my spare time permits.

I understand. But I strongly disagree with this approach. We want to
make U-Boot better. I am fine with all internal changes you wish, even
if it breaks the CLI at some point because it is more accurate and
drops a ton of legacy code. Again, this is okay as long as the CLI
tells the user what changed in the behavior. So when I run
tftp/dhcp/ping/whatever, if you don't want to automatically bind the
ethernet gadget I'm okay, but just tell the user "Please make sure the
Ethernet gadget is bound, eg: unbind xxx; bind yyy" (possibly giving
automatic shortcuts to avoid the pain of finding the path of the
controller). And when fastboot fails, same idea.

Could you please consider enhancing these parts as well?

> >>> Well, what's needed / is it possible to get to the point where we don't
> >>> _need_ to call bind/unbind for each of these cases? Is there something
> >>> we're supposed to be setting in the DT that we aren't?  
> >>
> >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.  
> > 
> > And for my own understanding, why don't we need to bind a fastboot
> > gadget?  
> 
> The fastboot command does that internally .

This is not visible with dm tree and I did not find how to bind the
fastboot gadget manually.

This makes the CLI clearly uneven and the internal code badly different
between gadgets/commands. Why can't we have them both
autoloaded/unloaded like before? I think I missed something which
explains the rationale behind this series.

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7469 bytes --]

On Fri, Aug 04, 2023 at 06:00:12PM +0200, Miquel Raynal wrote:
> Hi Marek,
> 
> marex@denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200:
> 
> > On 8/4/23 17:12, Miquel Raynal wrote:
> > > Hi,
> > > 
> > > marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200:
> > >   
> > >> On 8/4/23 17:01, Tom Rini wrote:  
> > >>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:  
> > >>>> On 8/4/23 09:00, Miquel Raynal wrote:  
> > >>>>> Hi Marek,
> > >>>>>
> > >>>>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:  
> > >>>>>   >>>>>> Extend the driver core to perform lookup by both OF node and driver  
> > >>>>>> bound to the node. Use this to look up specific device instances to
> > >>>>>> unbind from nodes in the unbind command. One example where this is
> > >>>>>> needed is USB peripheral controller, which may have multiple gadget
> > >>>>>> drivers bound to it. The unbind command has to select that specific
> > >>>>>> gadget driver instance to unbind from the controller, not unbind the
> > >>>>>> controller driver itself from the controller.
> > >>>>>>
> > >>>>>> USB ethernet gadget usage looks as follows with this change. Notice
> > >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end.
> > >>>>>> "
> > >>>>>> bind /soc/usb-otg@49000000 usb_ether
> > >>>>>> setenv ethact usb_ether
> > >>>>>> setenv loadaddr 0xc2000000
> > >>>>>> setenv ipaddr 10.0.0.2
> > >>>>>> setenv serverip 10.0.0.1
> > >>>>>> setenv netmask 255.255.255.0
> > >>>>>> tftpboot 0xc2000000 10.0.0.1:test.file
> > >>>>>> unbind /soc/usb-otg@49000000 usb_ether  
> > >>>>>
> > >>>>> This extra parameter does not seem to work on the BBBW. I cannot select
> > >>>>> the "usb_ether" driver like you do.
> > >>>>>
> > >>>>> Good news though, I am now able to use fastboot, but it is not
> > >>>>> straightforward:
> > >>>>>
> > >>>>> Here is my sequence right after the boot (reducing the dm tree output
> > >>>>> to the usb nodes for clarity):  
> > >>>>>   >>>>> => dm tree  
> > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > >>>>>     usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
> > >>>>>     ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
> > >>>>>     bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
> > >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> > >>>>> => fastboot usb 0  
> > >>>>> couldn't find an available UDC
> > >>>>> g_dnl_register: failed!, error: -19  
> > >>>>
> > >>>> That is expected and not a bug, since the beagle explicitly binds USB
> > >>>> ethernet to MUSB gadget in board file, which is legacy deprecated way.  
> > >>>
> > >>> So, we should do away with, probably all of arch_misc_init() in
> > >>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.  
> > >>
> > >> Yes
> > >>  
> > >>>>> exit not allowed from main input shell.  
> > >>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether  
> > >>>>
> > >>>> Does  
> > >>>>   >>>> => unbind ethernet 0  
> > >>>>
> > >>>> work ?
> > >>>>
> > >>>> If so, 1/4 in this series can be skipped altogether.
> > >>>>
> > >>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.  
> > 
> > Did you test this yet ?
> 
> Not yet, I'll send you the feedback once done.
> 
> > >>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000  
> > >>>>> => unbind /ocp/usb@47400000/usb@47401000
> > >>>>> => dm tree  
> > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> > >>>>> => fastboot usb 0
> > >>>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> > >>>>> => dm tree  
> > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > >>>>>     usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> > >>>>>     usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000  
> > >>>>> => fastboot usb 0  
> > >>>>> musb-hdrc: peripheral reset irq lost!
> > >>>>> # works! (the irq-related line above as always been there)
> > >>>>>
> > >>>>> So now, how do we make this process easy/understandable?  
> > >>>>
> > >>>> What would be your proposal ?  
> > > 
> > > At least I would appreciate:
> > > - to select CMD_BIND "by default" when relevant
> > > - to make the fastboot error more readable for the regular user  
> > 
> > Since with this 'unbind ethernet 0' this is orthogonal to this series, send separate patches, thanks.
> 
> This is not orthogonal, I am sorry.
> 
> version X:
> - tftp works "out of the box"
> - fastboot works "out of the box"
> version X+1:
> - tftp works "out of the box"
> - fastboot returns an obscure error
> 
> 1/ If we now *need* the bind/unbind commands, the series must take care
>    of it.
> 2/ Without proper error message you just break fastboot for most
>    regular users (basically everyone but few U-Boot devs).
> 
> > > If you want to get rid of all the legacy code, I am not opposed,
> > > really, but that must not be the user who is responsible for
> > > understanding what changed in the "core". It must be very easily
> > > accessible to understand that now:
> > > - manual binding/unbinding is needed
> > > - which driver must be used when  
> > 
> > When my spare time permits.
> 
> I understand. But I strongly disagree with this approach. We want to
> make U-Boot better. I am fine with all internal changes you wish, even
> if it breaks the CLI at some point because it is more accurate and
> drops a ton of legacy code. Again, this is okay as long as the CLI
> tells the user what changed in the behavior. So when I run
> tftp/dhcp/ping/whatever, if you don't want to automatically bind the
> ethernet gadget I'm okay, but just tell the user "Please make sure the
> Ethernet gadget is bound, eg: unbind xxx; bind yyy" (possibly giving
> automatic shortcuts to avoid the pain of finding the path of the
> controller). And when fastboot fails, same idea.
> 
> Could you please consider enhancing these parts as well?
> 
> > >>> Well, what's needed / is it possible to get to the point where we don't
> > >>> _need_ to call bind/unbind for each of these cases? Is there something
> > >>> we're supposed to be setting in the DT that we aren't?  
> > >>
> > >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.  
> > > 
> > > And for my own understanding, why don't we need to bind a fastboot
> > > gadget?  
> > 
> > The fastboot command does that internally .
> 
> This is not visible with dm tree and I did not find how to bind the
> fastboot gadget manually.
> 
> This makes the CLI clearly uneven and the internal code badly different
> between gadgets/commands. Why can't we have them both
> autoloaded/unloaded like before? I think I missed something which
> explains the rationale behind this series.

They aren't both auto-loaded currently. We have a legacy call,
usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
But this leads to the ref counting problems you encountered and
re-posted the rejected series for.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Tom,

> > > >>> Well, what's needed / is it possible to get to the point where we don't
> > > >>> _need_ to call bind/unbind for each of these cases? Is there something
> > > >>> we're supposed to be setting in the DT that we aren't?    
> > > >>
> > > >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.    
> > > > 
> > > > And for my own understanding, why don't we need to bind a fastboot
> > > > gadget?    
> > > 
> > > The fastboot command does that internally .  
> > 
> > This is not visible with dm tree and I did not find how to bind the
> > fastboot gadget manually.
> > 
> > This makes the CLI clearly uneven and the internal code badly different
> > between gadgets/commands. Why can't we have them both
> > autoloaded/unloaded like before? I think I missed something which
> > explains the rationale behind this series.  
> 
> They aren't both auto-loaded currently. We have a legacy call,
> usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
> But this leads to the ref counting problems you encountered and
> re-posted the rejected series for.

Ok, thanks for the additional details.

I don't understand why fastboot autoloads the correct gadget driver if
there is none bound, while all network commands just fail to do that if
we don't make the usb_ether_init() call manually.

I also don't understand why I need to unbind the ethernet gadget but I
cannot bind the fastboot gadget.

My underlying question is: can we have a single approach for all
drivers, or is it too complex today? Could it be possible, when we
perform these autoloads, to look up the registered gadget and
potentially unbind the one already in place before?

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/4/23 19:01, Miquel Raynal wrote:
> Hi Tom,
> 
>>>>>>> Well, what's needed / is it possible to get to the point where we don't
>>>>>>> _need_ to call bind/unbind for each of these cases? Is there something
>>>>>>> we're supposed to be setting in the DT that we aren't?
>>>>>>
>>>>>> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.
>>>>>
>>>>> And for my own understanding, why don't we need to bind a fastboot
>>>>> gadget?
>>>>
>>>> The fastboot command does that internally .
>>>
>>> This is not visible with dm tree and I did not find how to bind the
>>> fastboot gadget manually.
>>>
>>> This makes the CLI clearly uneven and the internal code badly different
>>> between gadgets/commands. Why can't we have them both
>>> autoloaded/unloaded like before? I think I missed something which
>>> explains the rationale behind this series.
>>
>> They aren't both auto-loaded currently. We have a legacy call,
>> usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
>> But this leads to the ref counting problems you encountered and
>> re-posted the rejected series for.
> 
> Ok, thanks for the additional details.
> 
> I don't understand why fastboot autoloads the correct gadget driver if
> there is none bound, while all network commands just fail to do that if
> we don't make the usb_ether_init() call manually.

Look into cmd/fastboot.c , it does usb_gadget_initialize(0) , just like 
usb_ether_init() does.

> I also don't understand why I need to unbind the ethernet gadget but I
> cannot bind the fastboot gadget.

Look into cmd/fastboot.c , it does usb_gadget_release() at the end of 
its operation, just like usb_eth_unbind() which is triggered by the 
bind/unbind commands.

> My underlying question is: can we have a single approach for all
> drivers, or is it too complex today? Could it be possible, when we
> perform these autoloads, to look up the registered gadget and
> potentially unbind the one already in place before?

The usb ethernet is special as there is no "ethernet" command like the 
"fastboot" command , which is why it has to be special handled by the 
bind/unbind commands.

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3196 bytes --]

On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote:
> Hi Tom,
> 
> > > > >>> Well, what's needed / is it possible to get to the point where we don't
> > > > >>> _need_ to call bind/unbind for each of these cases? Is there something
> > > > >>> we're supposed to be setting in the DT that we aren't?    
> > > > >>
> > > > >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.    
> > > > > 
> > > > > And for my own understanding, why don't we need to bind a fastboot
> > > > > gadget?    
> > > > 
> > > > The fastboot command does that internally .  
> > > 
> > > This is not visible with dm tree and I did not find how to bind the
> > > fastboot gadget manually.
> > > 
> > > This makes the CLI clearly uneven and the internal code badly different
> > > between gadgets/commands. Why can't we have them both
> > > autoloaded/unloaded like before? I think I missed something which
> > > explains the rationale behind this series.  
> > 
> > They aren't both auto-loaded currently. We have a legacy call,
> > usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
> > But this leads to the ref counting problems you encountered and
> > re-posted the rejected series for.
> 
> Ok, thanks for the additional details.
> 
> I don't understand why fastboot autoloads the correct gadget driver if
> there is none bound, while all network commands just fail to do that if
> we don't make the usb_ether_init() call manually.

Because "fastboot" or "dfu" are both being told (as part of their call)
"find usb gadget controller number X".  That's doing the bind.  Calling
usb_ether_init just takes the first controller and that's that (and so
could be / is wrong depending on the platform).

> I also don't understand why I need to unbind the ethernet gadget but I
> cannot bind the fastboot gadget.

You can't bind fastboot while ethernet is still configured.  It's in
use.  And we aren't a full blown operating system with the logic to have
multiple end points and devices configured and exposed.  I mean heck, we
don't keep the gadget interface up between network calls so on the host
side you need to re-configure the interface (or have something that's
bringing it up there again each time).  Which is why DFU or fastboot or
other "treat USB like USB" options tend to be more popular than "treat
USB like ethernet" work flows.

> My underlying question is: can we have a single approach for all
> drivers, or is it too complex today? Could it be possible, when we
> perform these autoloads, to look up the registered gadget and
> potentially unbind the one already in place before?

I would invite you to look in to this, yes.  No one objects to enhancing
the code, but the "functionality" you see on am33xx is as you've also
seen very broken in other ways, which is why it's not used virtually
anywhere else and instead the "bind" command is.  For example, if you
wanted to do this workflow on the new beagle devices, that's DWC3 and
where the "bind" command came from :)

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Tom,

trini@konsulko.com wrote on Fri, 4 Aug 2023 13:20:29 -0400:

> On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote:
> > Hi Tom,
> >   
> > > > > >>> Well, what's needed / is it possible to get to the point where we don't
> > > > > >>> _need_ to call bind/unbind for each of these cases? Is there something
> > > > > >>> we're supposed to be setting in the DT that we aren't?      
> > > > > >>
> > > > > >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.      
> > > > > > 
> > > > > > And for my own understanding, why don't we need to bind a fastboot
> > > > > > gadget?      
> > > > > 
> > > > > The fastboot command does that internally .    
> > > > 
> > > > This is not visible with dm tree and I did not find how to bind the
> > > > fastboot gadget manually.
> > > > 
> > > > This makes the CLI clearly uneven and the internal code badly different
> > > > between gadgets/commands. Why can't we have them both
> > > > autoloaded/unloaded like before? I think I missed something which
> > > > explains the rationale behind this series.    
> > > 
> > > They aren't both auto-loaded currently. We have a legacy call,
> > > usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
> > > But this leads to the ref counting problems you encountered and
> > > re-posted the rejected series for.  
> > 
> > Ok, thanks for the additional details.
> > 
> > I don't understand why fastboot autoloads the correct gadget driver if
> > there is none bound, while all network commands just fail to do that if
> > we don't make the usb_ether_init() call manually.  
> 
> Because "fastboot" or "dfu" are both being told (as part of their call)
> "find usb gadget controller number X".  That's doing the bind.  Calling
> usb_ether_init just takes the first controller and that's that (and so
> could be / is wrong depending on the platform).

This makes total sense, thanks for pointing it out.

> > I also don't understand why I need to unbind the ethernet gadget but I
> > cannot bind the fastboot gadget.  
> 
> You can't bind fastboot while ethernet is still configured.

I guess "before this series", the ethernet would not be kept
configured, because I could use both fastboot and ethernet without
caring about which driver had to be bound. And that's maybe what lead to
the bug reports also. So you want to get rid of this. Do I get the
situation right now?

>  It's in
> use.  And we aren't a full blown operating system with the logic to have
> multiple end points and devices configured and exposed.  I mean heck, we
> don't keep the gadget interface up between network calls so on the host
> side you need to re-configure the interface (or have something that's
> bringing it up there again each time).  Which is why DFU or fastboot or
> other "treat USB like USB" options tend to be more popular than "treat
> USB like ethernet" work flows.

Yes of course.

> > My underlying question is: can we have a single approach for all
> > drivers, or is it too complex today? Could it be possible, when we
> > perform these autoloads, to look up the registered gadget and
> > potentially unbind the one already in place before?  
> 
> I would invite you to look in to this, yes.

Sounds reasonably complex now, with the reasoning you made above.

>  No one objects to enhancing
> the code, but the "functionality" you see on am33xx is as you've also
> seen very broken in other ways, which is why it's not used virtually
> anywhere else and instead the "bind" command is.  For example, if you
> wanted to do this workflow on the new beagle devices, that's DWC3 and
> where the "bind" command came from :)

Again to be very clear, while I felt misunderstood at the beginning and
did not accept Marek's series because it was replacing a data abort
with a non-functional setup, I now get a better understanding of the
problem (I believe) and, as said before, I am always in favor of
writing better code, easier to maintain, clearer to review. I am in
favor of such change. I just want the user life to be eased when this
happens if we break the CLI. So if you think the right approach is to
get rid of the usb_ether_init() call, alright, let's drop it off. But
we should not let the users in the dark by providing proper doc or
error messages which should compensate the extra step now required.

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5015 bytes --]

On Fri, Aug 04, 2023 at 08:01:56PM +0200, Miquel Raynal wrote:
> Hi Tom,
> 
> trini@konsulko.com wrote on Fri, 4 Aug 2023 13:20:29 -0400:
> 
> > On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote:
> > > Hi Tom,
> > >   
> > > > > > >>> Well, what's needed / is it possible to get to the point where we don't
> > > > > > >>> _need_ to call bind/unbind for each of these cases? Is there something
> > > > > > >>> we're supposed to be setting in the DT that we aren't?      
> > > > > > >>
> > > > > > >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.      
> > > > > > > 
> > > > > > > And for my own understanding, why don't we need to bind a fastboot
> > > > > > > gadget?      
> > > > > > 
> > > > > > The fastboot command does that internally .    
> > > > > 
> > > > > This is not visible with dm tree and I did not find how to bind the
> > > > > fastboot gadget manually.
> > > > > 
> > > > > This makes the CLI clearly uneven and the internal code badly different
> > > > > between gadgets/commands. Why can't we have them both
> > > > > autoloaded/unloaded like before? I think I missed something which
> > > > > explains the rationale behind this series.    
> > > > 
> > > > They aren't both auto-loaded currently. We have a legacy call,
> > > > usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
> > > > But this leads to the ref counting problems you encountered and
> > > > re-posted the rejected series for.  
> > > 
> > > Ok, thanks for the additional details.
> > > 
> > > I don't understand why fastboot autoloads the correct gadget driver if
> > > there is none bound, while all network commands just fail to do that if
> > > we don't make the usb_ether_init() call manually.  
> > 
> > Because "fastboot" or "dfu" are both being told (as part of their call)
> > "find usb gadget controller number X".  That's doing the bind.  Calling
> > usb_ether_init just takes the first controller and that's that (and so
> > could be / is wrong depending on the platform).
> 
> This makes total sense, thanks for pointing it out.
> 
> > > I also don't understand why I need to unbind the ethernet gadget but I
> > > cannot bind the fastboot gadget.  
> > 
> > You can't bind fastboot while ethernet is still configured.
> 
> I guess "before this series", the ethernet would not be kept
> configured, because I could use both fastboot and ethernet without
> caring about which driver had to be bound. And that's maybe what lead to
> the bug reports also. So you want to get rid of this. Do I get the
> situation right now?

We're getting rid of this path because it leads to failures, yes.

> >  It's in
> > use.  And we aren't a full blown operating system with the logic to have
> > multiple end points and devices configured and exposed.  I mean heck, we
> > don't keep the gadget interface up between network calls so on the host
> > side you need to re-configure the interface (or have something that's
> > bringing it up there again each time).  Which is why DFU or fastboot or
> > other "treat USB like USB" options tend to be more popular than "treat
> > USB like ethernet" work flows.
> 
> Yes of course.
> 
> > > My underlying question is: can we have a single approach for all
> > > drivers, or is it too complex today? Could it be possible, when we
> > > perform these autoloads, to look up the registered gadget and
> > > potentially unbind the one already in place before?  
> > 
> > I would invite you to look in to this, yes.
> 
> Sounds reasonably complex now, with the reasoning you made above.
> 
> >  No one objects to enhancing
> > the code, but the "functionality" you see on am33xx is as you've also
> > seen very broken in other ways, which is why it's not used virtually
> > anywhere else and instead the "bind" command is.  For example, if you
> > wanted to do this workflow on the new beagle devices, that's DWC3 and
> > where the "bind" command came from :)
> 
> Again to be very clear, while I felt misunderstood at the beginning and
> did not accept Marek's series because it was replacing a data abort
> with a non-functional setup, I now get a better understanding of the
> problem (I believe) and, as said before, I am always in favor of
> writing better code, easier to maintain, clearer to review. I am in
> favor of such change. I just want the user life to be eased when this
> happens if we break the CLI. So if you think the right approach is to
> get rid of the usb_ether_init() call, alright, let's drop it off. But
> we should not let the users in the dark by providing proper doc or
> error messages which should compensate the extra step now required.

I think it would be good if you posted a patch to update
doc/board/ti/am335x_evm.rst to explain how to use the various gadget
device cases.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Tom,

trini@konsulko.com wrote on Fri, 4 Aug 2023 14:51:07 -0400:

> On Fri, Aug 04, 2023 at 08:01:56PM +0200, Miquel Raynal wrote:
> > Hi Tom,
> > 
> > trini@konsulko.com wrote on Fri, 4 Aug 2023 13:20:29 -0400:
> >   
> > > On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote:  
> > > > Hi Tom,
> > > >     
> > > > > > > >>> Well, what's needed / is it possible to get to the point where we don't
> > > > > > > >>> _need_ to call bind/unbind for each of these cases? Is there something
> > > > > > > >>> we're supposed to be setting in the DT that we aren't?        
> > > > > > > >>
> > > > > > > >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.        
> > > > > > > > 
> > > > > > > > And for my own understanding, why don't we need to bind a fastboot
> > > > > > > > gadget?        
> > > > > > > 
> > > > > > > The fastboot command does that internally .      
> > > > > > 
> > > > > > This is not visible with dm tree and I did not find how to bind the
> > > > > > fastboot gadget manually.
> > > > > > 
> > > > > > This makes the CLI clearly uneven and the internal code badly different
> > > > > > between gadgets/commands. Why can't we have them both
> > > > > > autoloaded/unloaded like before? I think I missed something which
> > > > > > explains the rationale behind this series.      
> > > > > 
> > > > > They aren't both auto-loaded currently. We have a legacy call,
> > > > > usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
> > > > > But this leads to the ref counting problems you encountered and
> > > > > re-posted the rejected series for.    
> > > > 
> > > > Ok, thanks for the additional details.
> > > > 
> > > > I don't understand why fastboot autoloads the correct gadget driver if
> > > > there is none bound, while all network commands just fail to do that if
> > > > we don't make the usb_ether_init() call manually.    
> > > 
> > > Because "fastboot" or "dfu" are both being told (as part of their call)
> > > "find usb gadget controller number X".  That's doing the bind.  Calling
> > > usb_ether_init just takes the first controller and that's that (and so
> > > could be / is wrong depending on the platform).  
> > 
> > This makes total sense, thanks for pointing it out.
> >   
> > > > I also don't understand why I need to unbind the ethernet gadget but I
> > > > cannot bind the fastboot gadget.    
> > > 
> > > You can't bind fastboot while ethernet is still configured.  
> > 
> > I guess "before this series", the ethernet would not be kept
> > configured, because I could use both fastboot and ethernet without
> > caring about which driver had to be bound. And that's maybe what lead to
> > the bug reports also. So you want to get rid of this. Do I get the
> > situation right now?  
> 
> We're getting rid of this path because it leads to failures, yes.
> 
> > >  It's in
> > > use.  And we aren't a full blown operating system with the logic to have
> > > multiple end points and devices configured and exposed.  I mean heck, we
> > > don't keep the gadget interface up between network calls so on the host
> > > side you need to re-configure the interface (or have something that's
> > > bringing it up there again each time).  Which is why DFU or fastboot or
> > > other "treat USB like USB" options tend to be more popular than "treat
> > > USB like ethernet" work flows.  
> > 
> > Yes of course.
> >   
> > > > My underlying question is: can we have a single approach for all
> > > > drivers, or is it too complex today? Could it be possible, when we
> > > > perform these autoloads, to look up the registered gadget and
> > > > potentially unbind the one already in place before?    
> > > 
> > > I would invite you to look in to this, yes.  
> > 
> > Sounds reasonably complex now, with the reasoning you made above.
> >   
> > >  No one objects to enhancing
> > > the code, but the "functionality" you see on am33xx is as you've also
> > > seen very broken in other ways, which is why it's not used virtually
> > > anywhere else and instead the "bind" command is.  For example, if you
> > > wanted to do this workflow on the new beagle devices, that's DWC3 and
> > > where the "bind" command came from :)  
> > 
> > Again to be very clear, while I felt misunderstood at the beginning and
> > did not accept Marek's series because it was replacing a data abort
> > with a non-functional setup, I now get a better understanding of the
> > problem (I believe) and, as said before, I am always in favor of
> > writing better code, easier to maintain, clearer to review. I am in
> > favor of such change. I just want the user life to be eased when this
> > happens if we break the CLI. So if you think the right approach is to
> > get rid of the usb_ether_init() call, alright, let's drop it off. But
> > we should not let the users in the dark by providing proper doc or
> > error messages which should compensate the extra step now required.  
> 
> I think it would be good if you posted a patch to update
> doc/board/ti/am335x_evm.rst to explain how to use the various gadget
> device cases.

Good idea, I've tried something.

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5739 bytes --]

On Fri, Aug 04, 2023 at 06:00:12PM +0200, Miquel Raynal wrote:
> Hi Marek,
> 
> marex@denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200:
> 
> > On 8/4/23 17:12, Miquel Raynal wrote:
> > > Hi,
> > > 
> > > marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200:
> > >   
> > >> On 8/4/23 17:01, Tom Rini wrote:  
> > >>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:  
> > >>>> On 8/4/23 09:00, Miquel Raynal wrote:  
> > >>>>> Hi Marek,
> > >>>>>
> > >>>>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:  
> > >>>>>   >>>>>> Extend the driver core to perform lookup by both OF node and driver  
> > >>>>>> bound to the node. Use this to look up specific device instances to
> > >>>>>> unbind from nodes in the unbind command. One example where this is
> > >>>>>> needed is USB peripheral controller, which may have multiple gadget
> > >>>>>> drivers bound to it. The unbind command has to select that specific
> > >>>>>> gadget driver instance to unbind from the controller, not unbind the
> > >>>>>> controller driver itself from the controller.
> > >>>>>>
> > >>>>>> USB ethernet gadget usage looks as follows with this change. Notice
> > >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end.
> > >>>>>> "
> > >>>>>> bind /soc/usb-otg@49000000 usb_ether
> > >>>>>> setenv ethact usb_ether
> > >>>>>> setenv loadaddr 0xc2000000
> > >>>>>> setenv ipaddr 10.0.0.2
> > >>>>>> setenv serverip 10.0.0.1
> > >>>>>> setenv netmask 255.255.255.0
> > >>>>>> tftpboot 0xc2000000 10.0.0.1:test.file
> > >>>>>> unbind /soc/usb-otg@49000000 usb_ether  
> > >>>>>
> > >>>>> This extra parameter does not seem to work on the BBBW. I cannot select
> > >>>>> the "usb_ether" driver like you do.
> > >>>>>
> > >>>>> Good news though, I am now able to use fastboot, but it is not
> > >>>>> straightforward:
> > >>>>>
> > >>>>> Here is my sequence right after the boot (reducing the dm tree output
> > >>>>> to the usb nodes for clarity):  
> > >>>>>   >>>>> => dm tree  
> > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > >>>>>     usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
> > >>>>>     ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
> > >>>>>     bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
> > >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> > >>>>> => fastboot usb 0  
> > >>>>> couldn't find an available UDC
> > >>>>> g_dnl_register: failed!, error: -19  
> > >>>>
> > >>>> That is expected and not a bug, since the beagle explicitly binds USB
> > >>>> ethernet to MUSB gadget in board file, which is legacy deprecated way.  
> > >>>
> > >>> So, we should do away with, probably all of arch_misc_init() in
> > >>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.  
> > >>
> > >> Yes
> > >>  
> > >>>>> exit not allowed from main input shell.  
> > >>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether  
> > >>>>
> > >>>> Does  
> > >>>>   >>>> => unbind ethernet 0  
> > >>>>
> > >>>> work ?
> > >>>>
> > >>>> If so, 1/4 in this series can be skipped altogether.
> > >>>>
> > >>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.  
> > 
> > Did you test this yet ?
> 
> Not yet, I'll send you the feedback once done.
> 
> > >>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000  
> > >>>>> => unbind /ocp/usb@47400000/usb@47401000
> > >>>>> => dm tree  
> > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> > >>>>> => fastboot usb 0
> > >>>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> > >>>>> => dm tree  
> > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > >>>>>     usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> > >>>>>     usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000  
> > >>>>> => fastboot usb 0  
> > >>>>> musb-hdrc: peripheral reset irq lost!
> > >>>>> # works! (the irq-related line above as always been there)
> > >>>>>
> > >>>>> So now, how do we make this process easy/understandable?  
> > >>>>
> > >>>> What would be your proposal ?  
> > > 
> > > At least I would appreciate:
> > > - to select CMD_BIND "by default" when relevant
> > > - to make the fastboot error more readable for the regular user  
> > 
> > Since with this 'unbind ethernet 0' this is orthogonal to this series, send separate patches, thanks.
> 
> This is not orthogonal, I am sorry.
> 
> version X:
> - tftp works "out of the box"
> - fastboot works "out of the box"
> version X+1:
> - tftp works "out of the box"
> - fastboot returns an obscure error
> 
> 1/ If we now *need* the bind/unbind commands, the series must take care
>    of it.
> 2/ Without proper error message you just break fastboot for most
>    regular users (basically everyone but few U-Boot devs).

You're missing the class of users that will be impacted here.  In order
for there to be a change here, you have to already be in the case where
you have CONFIG_USB_ETHER=y and gadget ethernet device isn't just
enabled but also initialized by default by calling usb_ether_init().
That's a very small list.  It's basically am33xx, two mediatek reference
platforms and xilinx_zynqmp_virt.  Given that am33xx defconfigs also
setup DFU, I'm not really sure just how many people use gadget ethernet.
The normal flow on modern devices is to be calling bind/unbind here
already.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Tom,

> > > >>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000    
> > > >>>>> => unbind /ocp/usb@47400000/usb@47401000
> > > >>>>> => dm tree    
> > > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > > >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800    
> > > >>>>> => fastboot usb 0
> > > >>>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> > > >>>>> => dm tree    
> > > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > > >>>>>     usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> > > >>>>>     usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000    
> > > >>>>> => fastboot usb 0    
> > > >>>>> musb-hdrc: peripheral reset irq lost!
> > > >>>>> # works! (the irq-related line above as always been there)
> > > >>>>>
> > > >>>>> So now, how do we make this process easy/understandable?    
> > > >>>>
> > > >>>> What would be your proposal ?    
> > > > 
> > > > At least I would appreciate:
> > > > - to select CMD_BIND "by default" when relevant
> > > > - to make the fastboot error more readable for the regular user    
> > > 
> > > Since with this 'unbind ethernet 0' this is orthogonal to this series, send separate patches, thanks.  
> > 
> > This is not orthogonal, I am sorry.
> > 
> > version X:
> > - tftp works "out of the box"
> > - fastboot works "out of the box"
> > version X+1:
> > - tftp works "out of the box"
> > - fastboot returns an obscure error
> > 
> > 1/ If we now *need* the bind/unbind commands, the series must take care
> >    of it.
> > 2/ Without proper error message you just break fastboot for most
> >    regular users (basically everyone but few U-Boot devs).  
> 
> You're missing the class of users that will be impacted here.  In order
> for there to be a change here, you have to already be in the case where
> you have CONFIG_USB_ETHER=y and gadget ethernet device isn't just
> enabled but also initialized by default by calling usb_ether_init().
> That's a very small list.  It's basically am33xx, two mediatek reference
> platforms and xilinx_zynqmp_virt.  Given that am33xx defconfigs also
> setup DFU, I'm not really sure just how many people use gadget ethernet.
> The normal flow on modern devices is to be calling bind/unbind here
> already.

Can we make this behavior explicit to the user? I am sorry, maybe it is
the normal flow for you, but I am a regular U-Boot user and I totally
missed that requirement.

Typical situation: one needs to use <whatever-gadget> but none is bound
to the UDC (or another is bound), could we make the error messages more
explicit if we decide not to unbind/bind the right one automatically
because it is too "costly"?

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/4/23 19:04, Miquel Raynal wrote:
> Hi Tom,
> 
>>>>>>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000
>>>>>>>>> => unbind /ocp/usb@47400000/usb@47401000
>>>>>>>>> => dm tree
>>>>>>>>>      misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>>>>>>>      usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
>>>>>>>>> => fastboot usb 0
>>>>>>>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
>>>>>>>>> => dm tree
>>>>>>>>>      misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>>>>>>>>>      usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
>>>>>>>>>      usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000
>>>>>>>>> => fastboot usb 0
>>>>>>>>> musb-hdrc: peripheral reset irq lost!
>>>>>>>>> # works! (the irq-related line above as always been there)
>>>>>>>>>
>>>>>>>>> So now, how do we make this process easy/understandable?
>>>>>>>>
>>>>>>>> What would be your proposal ?
>>>>>
>>>>> At least I would appreciate:
>>>>> - to select CMD_BIND "by default" when relevant
>>>>> - to make the fastboot error more readable for the regular user
>>>>
>>>> Since with this 'unbind ethernet 0' this is orthogonal to this series, send separate patches, thanks.
>>>
>>> This is not orthogonal, I am sorry.
>>>
>>> version X:
>>> - tftp works "out of the box"
>>> - fastboot works "out of the box"
>>> version X+1:
>>> - tftp works "out of the box"
>>> - fastboot returns an obscure error
>>>
>>> 1/ If we now *need* the bind/unbind commands, the series must take care
>>>     of it.
>>> 2/ Without proper error message you just break fastboot for most
>>>     regular users (basically everyone but few U-Boot devs).
>>
>> You're missing the class of users that will be impacted here.  In order
>> for there to be a change here, you have to already be in the case where
>> you have CONFIG_USB_ETHER=y and gadget ethernet device isn't just
>> enabled but also initialized by default by calling usb_ether_init().
>> That's a very small list.  It's basically am33xx, two mediatek reference
>> platforms and xilinx_zynqmp_virt.  Given that am33xx defconfigs also
>> setup DFU, I'm not really sure just how many people use gadget ethernet.
>> The normal flow on modern devices is to be calling bind/unbind here
>> already.
> 
> Can we make this behavior explicit to the user? I am sorry, maybe it is
> the normal flow for you, but I am a regular U-Boot user and I totally
> missed that requirement.
> 
> Typical situation: one needs to use <whatever-gadget> but none is bound
> to the UDC (or another is bound), could we make the error messages more
> explicit if we decide not to unbind/bind the right one automatically
> because it is too "costly"?

Maybe you could implement a documentation patch, since you are not yet 
tainted by all the "I work on in day in day out, so it is obvious to me" 
thing ?

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3349 bytes --]

On Fri, Aug 04, 2023 at 07:04:31PM +0200, Miquel Raynal wrote:
> Hi Tom,
> 
> > > > >>>>> Cannot find a device with path /ocp/usb@47400000/usb@47401000    
> > > > >>>>> => unbind /ocp/usb@47400000/usb@47401000
> > > > >>>>> => dm tree    
> > > > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > > > >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800    
> > > > >>>>> => fastboot usb 0
> > > > >>>>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> > > > >>>>> => dm tree    
> > > > >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> > > > >>>>>     usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> > > > >>>>>     usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000    
> > > > >>>>> => fastboot usb 0    
> > > > >>>>> musb-hdrc: peripheral reset irq lost!
> > > > >>>>> # works! (the irq-related line above as always been there)
> > > > >>>>>
> > > > >>>>> So now, how do we make this process easy/understandable?    
> > > > >>>>
> > > > >>>> What would be your proposal ?    
> > > > > 
> > > > > At least I would appreciate:
> > > > > - to select CMD_BIND "by default" when relevant
> > > > > - to make the fastboot error more readable for the regular user    
> > > > 
> > > > Since with this 'unbind ethernet 0' this is orthogonal to this series, send separate patches, thanks.  
> > > 
> > > This is not orthogonal, I am sorry.
> > > 
> > > version X:
> > > - tftp works "out of the box"
> > > - fastboot works "out of the box"
> > > version X+1:
> > > - tftp works "out of the box"
> > > - fastboot returns an obscure error
> > > 
> > > 1/ If we now *need* the bind/unbind commands, the series must take care
> > >    of it.
> > > 2/ Without proper error message you just break fastboot for most
> > >    regular users (basically everyone but few U-Boot devs).  
> > 
> > You're missing the class of users that will be impacted here.  In order
> > for there to be a change here, you have to already be in the case where
> > you have CONFIG_USB_ETHER=y and gadget ethernet device isn't just
> > enabled but also initialized by default by calling usb_ether_init().
> > That's a very small list.  It's basically am33xx, two mediatek reference
> > platforms and xilinx_zynqmp_virt.  Given that am33xx defconfigs also
> > setup DFU, I'm not really sure just how many people use gadget ethernet.
> > The normal flow on modern devices is to be calling bind/unbind here
> > already.
> 
> Can we make this behavior explicit to the user? I am sorry, maybe it is
> the normal flow for you, but I am a regular U-Boot user and I totally
> missed that requirement.
> 
> Typical situation: one needs to use <whatever-gadget> but none is bound
> to the UDC (or another is bound), could we make the error messages more
> explicit if we decide not to unbind/bind the right one automatically
> because it is too "costly"?

If you would like to improve this, yes, please do.  The uncommon case is
where gadget ethernet and fastboot/dfu/etc are also being used one after
the other.  The typical case is you just fastboot or dfu or whatever,
and that command requires you to say what usb controller you're binding
to and so works.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200:

> On 8/4/23 17:12, Miquel Raynal wrote:
> > Hi,
> > 
> > marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200:
> >   
> >> On 8/4/23 17:01, Tom Rini wrote:  
> >>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:  
> >>>> On 8/4/23 09:00, Miquel Raynal wrote:  
> >>>>> Hi Marek,
> >>>>>
> >>>>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +0200:  
> >>>>>   >>>>>> Extend the driver core to perform lookup by both OF node and driver  
> >>>>>> bound to the node. Use this to look up specific device instances to
> >>>>>> unbind from nodes in the unbind command. One example where this is
> >>>>>> needed is USB peripheral controller, which may have multiple gadget
> >>>>>> drivers bound to it. The unbind command has to select that specific
> >>>>>> gadget driver instance to unbind from the controller, not unbind the
> >>>>>> controller driver itself from the controller.
> >>>>>>
> >>>>>> USB ethernet gadget usage looks as follows with this change. Notice
> >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end.
> >>>>>> "
> >>>>>> bind /soc/usb-otg@49000000 usb_ether
> >>>>>> setenv ethact usb_ether
> >>>>>> setenv loadaddr 0xc2000000
> >>>>>> setenv ipaddr 10.0.0.2
> >>>>>> setenv serverip 10.0.0.1
> >>>>>> setenv netmask 255.255.255.0
> >>>>>> tftpboot 0xc2000000 10.0.0.1:test.file
> >>>>>> unbind /soc/usb-otg@49000000 usb_ether  
> >>>>>
> >>>>> This extra parameter does not seem to work on the BBBW. I cannot select
> >>>>> the "usb_ether" driver like you do.
> >>>>>
> >>>>> Good news though, I am now able to use fastboot, but it is not
> >>>>> straightforward:
> >>>>>
> >>>>> Here is my sequence right after the boot (reducing the dm tree output
> >>>>> to the usb nodes for clarity):  
> >>>>>   >>>>> => dm tree  
> >>>>>     misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>>>     usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
> >>>>>     ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
> >>>>>     bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
> >>>>>     usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> >>>>> => fastboot usb 0  
> >>>>> couldn't find an available UDC
> >>>>> g_dnl_register: failed!, error: -19  
> >>>>
> >>>> That is expected and not a bug, since the beagle explicitly binds USB
> >>>> ethernet to MUSB gadget in board file, which is legacy deprecated way.  
> >>>
> >>> So, we should do away with, probably all of arch_misc_init() in
> >>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.  
> >>
> >> Yes
> >>  
> >>>>> exit not allowed from main input shell.  
> >>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether  
> >>>>
> >>>> Does  
> >>>>   >>>> => unbind ethernet 0  
> >>>>
> >>>> work ?
> >>>>
> >>>> If so, 1/4 in this series can be skipped altogether.
> >>>>
> >>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.  
> 
> Did you test this yet ?

Unfortunately it does not work. Indeed it would be much simpler than
using the node path. Any idea why?

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/4/23 19:24, Miquel Raynal wrote:

Hi,

>>>>>>> exit not allowed from main input shell.
>>>>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether
>>>>>>
>>>>>> Does
>>>>>>    >>>> => unbind ethernet 0
>>>>>>
>>>>>> work ?
>>>>>>
>>>>>> If so, 1/4 in this series can be skipped altogether.
>>>>>>
>>>>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.
>>
>> Did you test this yet ?
> 
> Unfortunately it does not work. Indeed it would be much simpler than
> using the node path. Any idea why?

Since you provided literally zero information, no.

Console log would be a good starting point.

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Fri, 4 Aug 2023 19:31:50 +0200:

> On 8/4/23 19:24, Miquel Raynal wrote:
> 
> Hi,
> 
> >>>>>>> exit not allowed from main input shell.  
> >>>>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether  
> >>>>>>
> >>>>>> Does  
> >>>>>>    >>>> => unbind ethernet 0  
> >>>>>>
> >>>>>> work ?
> >>>>>>
> >>>>>> If so, 1/4 in this series can be skipped altogether.
> >>>>>>
> >>>>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.  
> >>
> >> Did you test this yet ?  
> > 
> > Unfortunately it does not work. Indeed it would be much simpler than
> > using the node path. Any idea why?  
> 
> Since you provided literally zero information, no.
> 
> Console log would be a good starting point.

Here it is, the unbind command itself does not complain has it seems to
catch the regular Ethernet controller (there is one in the SoC, but it
is not wired on the board). So the first time it does nothing, but the
second time it works as the USB gadget get dropped! And after the
second call, fastboot works without the bind call.

=> dm tree
 misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
 usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
 ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
 bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
 usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
 ethernet      0  [ + ]   eth_cpsw              |   |-- ethernet@4a100000
 bootdev       2  [   ]   eth_bootdev           |   |   `-- ethernet@4a100000.bootdev
=> unbind ethernet 0
=> dm tree
 misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
 usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
 ethernet      0  [ + ]   usb_ether             |   |   |   `-- usb_ether
 bootdev       2  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
 usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800
=> unbind ethernet 0
=> dm tree
 misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
 usb           0  [   ]   ti-musb-peripheral    |   |   |-- usb@47401000
 usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800

So actually the unbind works, but was not targeting the right
controller, because it's listed as the second Ethernet controller on
this board. Hence this actually works:

=> unbind ethernet 1
=> fastboot usb 0

\o/

Thanks,
Miquèl

Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Marek Vasut]

On 8/4/23 19:46, Miquel Raynal wrote:
> Hi Marek,
> 
> marex@denx.de wrote on Fri, 4 Aug 2023 19:31:50 +0200:
> 
>> On 8/4/23 19:24, Miquel Raynal wrote:
>>
>> Hi,
>>
>>>>>>>>> exit not allowed from main input shell.
>>>>>>>>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether
>>>>>>>>
>>>>>>>> Does
>>>>>>>>     >>>> => unbind ethernet 0
>>>>>>>>
>>>>>>>> work ?
>>>>>>>>
>>>>>>>> If so, 1/4 in this series can be skipped altogether.
>>>>>>>>
>>>>>>>> You likely won't even need the rebinding of ti-musb-peripheral anymore.
>>>>
>>>> Did you test this yet ?
>>>
>>> Unfortunately it does not work. Indeed it would be much simpler than
>>> using the node path. Any idea why?
>>
>> Since you provided literally zero information, no.
>>
>> Console log would be a good starting point.
> 
> Here it is, the unbind command itself does not complain has it seems to
> catch the regular Ethernet controller (there is one in the SoC, but it
> is not wired on the board). So the first time it does nothing, but the
> second time it works as the USB gadget get dropped! And after the
> second call, fastboot works without the bind call.
> 
> => dm tree
>   misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
>   usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
>   ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
     ^^^^^^^^      ^

unbind ethernet 1