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

[PATCH v2 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: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
V2: 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 v2 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: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
V2: 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 v2 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: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
V2: 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 v2 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_initialize()/usb_gadget_release(),
which are helper functions implemented in the UDC uclass to bind and
unbind UDC drivers to controllers from U-Boot command line. 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: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
V2: Fix up return value handling in probe
---
 drivers/usb/gadget/ether.c | 87 +++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 2040b5b5081..9e1a45ad9df 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)
@@ -2525,6 +2481,48 @@ 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;
 }
 
@@ -2532,6 +2530,7 @@ U_BOOT_DRIVER(eth_usb) = {
 	.name	= "usb_ether",
 	.id	= UCLASS_ETH,
 	.probe	= usb_eth_probe,
+	.remove	= usb_eth_remove,
 	.ops	= &usb_eth_ops,
 	.priv_auto	= sizeof(struct ether_priv),
 	.plat_auto	= sizeof(struct eth_pdata),
-- 
2.40.1

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

[Simon Glass]

On Mon, 17 Jul 2023 at 05:21, 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: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> V2: No change
> ---
>  cmd/bind.c            | 10 +++++-----
>  drivers/core/device.c | 20 +++++++++++++++-----
>  include/dm/device.h   | 17 +++++++++++++++++
>  3 files changed, 37 insertions(+), 10 deletions(-)

Can we have a test?

Regards,
Simon

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

[Marek Vasut]

On 7/19/23 03:08, Simon Glass wrote:
> On Mon, 17 Jul 2023 at 05:21, 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: Marek Vasut <marex@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>> V2: No change
>> ---
>>   cmd/bind.c            | 10 +++++-----
>>   drivers/core/device.c | 20 +++++++++++++++-----
>>   include/dm/device.h   | 17 +++++++++++++++++
>>   3 files changed, 37 insertions(+), 10 deletions(-)
> 
> Can we have a test?

I added it into my todo queue.

Anything else or is the patch OK ?

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

[Simon Glass]

Hi Marek,

On Wed, 19 Jul 2023 at 08:23, Marek Vasut <marex@denx.de> wrote:
>
> On 7/19/23 03:08, Simon Glass wrote:
> > On Mon, 17 Jul 2023 at 05:21, 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: Marek Vasut <marex@denx.de>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> ---
> >> V2: No change
> >> ---
> >>   cmd/bind.c            | 10 +++++-----
> >>   drivers/core/device.c | 20 +++++++++++++++-----
> >>   include/dm/device.h   | 17 +++++++++++++++++
> >>   3 files changed, 37 insertions(+), 10 deletions(-)
> >
> > Can we have a test?
>
> I added it into my todo queue.
>
> Anything else or is the patch OK ?

That's my only comment.

Regards,
Simon

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

[Tom Rini]

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

On Mon, Jul 17, 2023 at 01:21:34PM +0200, Marek Vasut 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: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> V2: No change
> ---
>  cmd/bind.c            | 10 +++++-----
>  drivers/core/device.c | 20 +++++++++++++++-----
>  include/dm/device.h   | 17 +++++++++++++++++
>  3 files changed, 37 insertions(+), 10 deletions(-)

Conceptually this seems fine to me.  Simon?

-- 
Tom

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

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

[Tom Rini]

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

On Mon, Jul 17, 2023 at 01:21:36PM +0200, Marek Vasut wrote:

> 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>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

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

[Tom Rini]

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

On Mon, Jul 17, 2023 at 01:21:35PM +0200, Marek Vasut wrote:

> 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>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

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

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +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

I don't really get why this is needed? Yes, having proper bind and
unbind methods and having them called internally is relevant, but when
you have a single OTG controller, why is this needed? It basically
breaks the CLI, making bisects more painful and all updates just fail.

> 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: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>

I've tested the whole series, unfortunately is does not work on
AM335x/BBBW:

* Any recovery attempted using the network will now fail in
  the SPL, where, AFAIK, there is no way to manually bind:

U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
Trying to boot from USB eth
Could not get PHY for eth_cpsw: addr 0
eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
MAC de:ad:be:ef:00:01
HOST MAC de:ad:be:ef:00:00
RNDIS ready
, eth1: usb_ether

* The bind command was not available on my default configuration,
  making it even difficult for people unaware that this command is
  now required to fix their common commands.

* Any command that expects the usb_ether driver will now fail badly
  even after the bind call:

=> bind /ocp/usb@47400000/usb@47401000 usb_ether
=> fastboot usb 0                                
couldn't find an available UDC
g_dnl_register: failed!, error: -19
exit not allowed from main input shell.
=> tftp 0x81000000 zImage
dev_get_priv: null device
data abort
pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
sp : 9df2f920  ip : 00000000     fp : 00000003
r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
r7 : 00004a53  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
r3 : 9ff97653  r2 : fff1102c     r1 : 00000000  r0 : 00000000
Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
Code: 9ffd b508 f7e6 fc4b (6801) 2000 
Resetting CPU ...

Is there anything I missed?

Thanks,
Miquèl

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

[Marek Vasut]

On 7/23/23 19:49, Miquel Raynal wrote:
> Hi Marek,

Hi,

> marex@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +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
> 
> I don't really get why this is needed? Yes, having proper bind and
> unbind methods and having them called internally is relevant, but when
> you have a single OTG controller, why is this needed?

Because you do need to bind the usb_ether driver to a controller.

I suspect you are trying to point in the direction of usb_ether_init() , 
right ? That is bogus and should be removed, because that is hard-coding 
one specific (ethernet) function to an OTG controller.

> It basically
> breaks the CLI

Can you elaborate on this ?

How does calling a 'bind' command breaks CLI ?

> , making bisects more painful and all updates just fail.

This is just utter nonsense, sorry.

>> 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: Marek Vasut <marex@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
> 
> I've tested the whole series, unfortunately is does not work on
> AM335x/BBBW:
> 
> * Any recovery attempted using the network will now fail in
>    the SPL, where, AFAIK, there is no way to manually bind:
> 
> U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> Trying to boot from USB eth
> Could not get PHY for eth_cpsw: addr 0
> eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> MAC de:ad:be:ef:00:01
> HOST MAC de:ad:be:ef:00:00
> RNDIS ready
> , eth1: usb_ether

It seems the RNDIS adapter is actually ready based on the above ^ ?

> * The bind command was not available on my default configuration,
>    making it even difficult for people unaware that this command is
>    now required to fix their common commands.

It has been required ever since, it's just that many ignored the need to 
move over to DM and depended on board-specific hackage to set up their 
USB ethernet. Now that hackage broke, time to switch to the proper way.

> * Any command that expects the usb_ether driver will now fail badly
>    even after the bind call:
> 
> => bind /ocp/usb@47400000/usb@47401000 usb_ether
> => fastboot usb 0
> couldn't find an available UDC

You already have the ethernet function bound to the UDC and now you are 
trying to bind another function, fastboot, right ? This is likely the 
problem. Unbind the ethernet first, then use fastboot.

> g_dnl_register: failed!, error: -19
> exit not allowed from main input shell.
> => tftp 0x81000000 zImage
> dev_get_priv: null device
> data abort
> pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
> reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
> sp : 9df2f920  ip : 00000000     fp : 00000003
> r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
> r7 : 00004a53  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
> r3 : 9ff97653  r2 : fff1102c     r1 : 00000000  r0 : 00000000
> Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
> Code: 9ffd b508 f7e6 fc4b (6801) 2000
> Resetting CPU ...
> 
> Is there anything I missed?

Please see above.

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

[Tom Rini]

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

On Sun, Jul 23, 2023 at 07:49:55PM +0200, Miquel Raynal wrote:
> Hi Marek,
> 
> marex@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +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
> 
> I don't really get why this is needed? Yes, having proper bind and
> unbind methods and having them called internally is relevant, but when
> you have a single OTG controller, why is this needed? It basically
> breaks the CLI, making bisects more painful and all updates just fail.

I think part of the issue here is how usb_ether didn't act like the rest
of the gadget do, for example fastboot.

> > 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: Marek Vasut <marex@denx.de>
> > Cc: Simon Glass <sjg@chromium.org>
> 
> I've tested the whole series, unfortunately is does not work on
> AM335x/BBBW:
> 
> * Any recovery attempted using the network will now fail in
>   the SPL, where, AFAIK, there is no way to manually bind:
> 
> U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> Trying to boot from USB eth
> Could not get PHY for eth_cpsw: addr 0
> eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> MAC de:ad:be:ef:00:01
> HOST MAC de:ad:be:ef:00:00
> RNDIS ready
> , eth1: usb_ether

For testing, what happens if you disable CPSW? Does it both bind (as it
shows here) and then use the expected device?

> * The bind command was not available on my default configuration,
>   making it even difficult for people unaware that this command is
>   now required to fix their common commands.

Yes, we'll need to make that default y if USB_ETHER.

> * Any command that expects the usb_ether driver will now fail badly
>   even after the bind call:
> 
> => bind /ocp/usb@47400000/usb@47401000 usb_ether
> => fastboot usb 0                                
> couldn't find an available UDC
> g_dnl_register: failed!, error: -19
> exit not allowed from main input shell.
> => tftp 0x81000000 zImage
> dev_get_priv: null device

Well, does it work if you do:
bind /ocp/usb@47400000/usb@47401000 usb_ether
tftp 0x81000000 zImage
?

-- 
Tom

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

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

[Miquel Raynal]

Hi Tom,

trini@konsulko.com wrote on Mon, 24 Jul 2023 14:13:45 -0400:

> On Sun, Jul 23, 2023 at 07:49:55PM +0200, Miquel Raynal wrote:
> > Hi Marek,
> > 
> > marex@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +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  
> > 
> > I don't really get why this is needed? Yes, having proper bind and
> > unbind methods and having them called internally is relevant, but when
> > you have a single OTG controller, why is this needed? It basically
> > breaks the CLI, making bisects more painful and all updates just fail.  
> 
> I think part of the issue here is how usb_ether didn't act like the rest
> of the gadget do, for example fastboot.

It definitely is. "before it was working, now it does not anymore".
That's the gut feeling many people get when the community breaks common
habits. If we break something like this, we must at least be very clear
on what is the new behavior.

> > > 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: Marek Vasut <marex@denx.de>
> > > Cc: Simon Glass <sjg@chromium.org>  
> > 
> > I've tested the whole series, unfortunately is does not work on
> > AM335x/BBBW:
> > 
> > * Any recovery attempted using the network will now fail in
> >   the SPL, where, AFAIK, there is no way to manually bind:
> > 
> > U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> > Trying to boot from USB eth
> > Could not get PHY for eth_cpsw: addr 0
> > eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> > MAC de:ad:be:ef:00:01
> > HOST MAC de:ad:be:ef:00:00
> > RNDIS ready
> > , eth1: usb_ether  
> 
> For testing, what happens if you disable CPSW? Does it both bind (as it
> shows here) and then use the expected device?

Disabling CPSW breaks the link, I had to ensure ft_board_setup in the
am335x arch folder was still available (and returned 0). But once I
managed to start I did not observe any improvements.

U-Boot SPL 2023.07-00806-gac80e6de9cf-dirty (Jul 28 2023 - 14:49:48 +0200)
Trying to boot from USB eth
Could not get PHY for eth_cpsw: addr 0
eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
MAC de:ad:be:ef:00:01
HOST MAC de:ad:be:ef:00:00
RNDIS ready
, eth1: usb_ether

> > * The bind command was not available on my default configuration,
> >   making it even difficult for people unaware that this command is
> >   now required to fix their common commands.  
> 
> Yes, we'll need to make that default y if USB_ETHER.

Agreed.

> > * Any command that expects the usb_ether driver will now fail badly
> >   even after the bind call:
> >   
> > => bind /ocp/usb@47400000/usb@47401000 usb_ether
> > => fastboot usb 0                                  
> > couldn't find an available UDC
> > g_dnl_register: failed!, error: -19
> > exit not allowed from main input shell.  
> > => tftp 0x81000000 zImage  
> > dev_get_priv: null device  
> 
> Well, does it work if you do:
> bind /ocp/usb@47400000/usb@47401000 usb_ether
> tftp 0x81000000 zImage
> ?

No, I already tried both:

U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
Trying to boot from MMC2


U-Boot 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)

CPU  : AM335X-GP rev 2.1
Model: TI AM335x BeagleBone Black
DRAM:  512 MiB
Core:  160 devices, 18 uclasses, devicetree: separate
WDT:   Started wdt@44e35000 with servicing every 1000ms (60s timeout)
NAND:  0 MiB
MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
Loading Environment from FAT... Unable to read "uboot.env" from mmc1:1... 
<ethaddr> not set. Validating first E-fuse MAC
Net:   Could not get PHY for ethernet@4a100000: addr 0
eth2: ethernet@4a100000using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
MAC de:ad:be:ef:00:01
HOST MAC de:ad:be:ef:00:00
RNDIS ready
, eth3: usb_ether
=> bind /ocp/usb@47400000/usb@47401000 usb_ether
=> tftp 0x81000000 zImage
dev_get_priv: null device
data abort
pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
sp : 9df2f920  ip : 00000000     fp : 00000003
r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
r7 : 0000538c  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
r3 : 9ff97653  r2 : fff69249     r1 : 00000000  r0 : 00000000
Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
Code: 9ffd b508 f7e6 fc4b (6801) 2000 
Resetting CPU ...

resetting ...

Thanks,
Miquèl

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

[Miquel Raynal]

Hi Marek,

marex@denx.de wrote on Sun, 23 Jul 2023 23:45:55 +0200:

> On 7/23/23 19:49, Miquel Raynal wrote:
> > Hi Marek,  
> 
> Hi,
> 
> > marex@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +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  
> > 
> > I don't really get why this is needed? Yes, having proper bind and
> > unbind methods and having them called internally is relevant, but when
> > you have a single OTG controller, why is this needed?  
> 
> Because you do need to bind the usb_ether driver to a controller.

Right now it seems that binding is kind of performed in the context of
a command execution (and then everything is cleaned up). I also think
it is not optimal, but I would like to avoid breaking common commands
like you propose. At the *very least* the error messages should be very
clear about what has changed and what the user needs to do now in order
to workaround it. But the best would be to avoid user interaction as
much as possible. So why not load the first driver needed and expect
the user the run the correct unbind/bind commands if it wants to
change?

> I suspect you are trying to point in the direction of usb_ether_init() , right ? That is bogus and should be removed, because that is hard-coding one specific (ethernet) function to an OTG controller.

Well, hardcoding is bad, but setting a default is often useful and much
more user friendly, IMHO.

> 
> > It basically
> > breaks the CLI  
> 
> Can you elaborate on this ?
> 
> How does calling a 'bind' command breaks CLI ?

I've attempted a bisect between U-Boot 2018 and 2023, it was already
very painful. But if I try again in 2024 and need to cope with the new
rules added in 2023 for binding drivers which were not needed until,
then it becomes even more painful. So yes, it breaks the CLI. Before:
=> tftp xxx
=> fastboot xxx
just worked, after you patchset these simple operations won't work
anymore.

> > , making bisects more painful and all updates just fail.  
> 
> This is just utter nonsense, sorry.

Well, I am a U-Boot user, and I expect this change to break
a good amount of boot scripts after an update.

> >> 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: Marek Vasut <marex@denx.de>
> >> Cc: Simon Glass <sjg@chromium.org>  
> > 
> > I've tested the whole series, unfortunately is does not work on
> > AM335x/BBBW:
> > 
> > * Any recovery attempted using the network will now fail in
> >    the SPL, where, AFAIK, there is no way to manually bind:
> > 
> > U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> > Trying to boot from USB eth
> > Could not get PHY for eth_cpsw: addr 0
> > eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> > MAC de:ad:be:ef:00:01
> > HOST MAC de:ad:be:ef:00:00
> > RNDIS ready
> > , eth1: usb_ether  
> 
> It seems the RNDIS adapter is actually ready based on the above ^ ?
> 
> > * The bind command was not available on my default configuration,
> >    making it even difficult for people unaware that this command is
> >    now required to fix their common commands.  
> 
> It has been required ever since, it's just that many ignored the need to move over to DM and depended on board-specific hackage to set up their USB ethernet. Now that hackage broke, time to switch to the proper way.

It breaks because there is something wrong elsewhere. It's not at all
the fault of the board hack (if any, I have no idea, I'll trust your
judgment regarding the BBB support). My series fixes a real problem in
the current code. The fact that it won't be visible after your series
is almost anecdotic... until it triggers again for whatever reason.

> > * Any command that expects the usb_ether driver will now fail badly
> >    even after the bind call:
> >   
> > => bind /ocp/usb@47400000/usb@47401000 usb_ether
> > => fastboot usb 0  
> > couldn't find an available UDC  
> 
> You already have the ethernet function bound to the UDC and now you are trying to bind another function, fastboot, right ? This is likely the problem. Unbind the ethernet first, then use fastboot.
> 
> > g_dnl_register: failed!, error: -19
> > exit not allowed from main input shell.  
> > => tftp 0x81000000 zImage  
> > dev_get_priv: null device
> > data abort
> > pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
> > reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
> > sp : 9df2f920  ip : 00000000     fp : 00000003
> > r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
> > r7 : 00004a53  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
> > r3 : 9ff97653  r2 : fff1102c     r1 : 00000000  r0 : 00000000
> > Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
> > Code: 9ffd b508 f7e6 fc4b (6801) 2000
> > Resetting CPU ...
> > 
> > Is there anything I missed?  
> 
> Please see above.

Sorry for disturbing you with the fastboot command. Can you advise a
couple of commands to test fastboot then? I don't find the relevant
driver name to use.

Here is a boot without anything else than a tftp, no change:

U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
Trying to boot from MMC2


U-Boot 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)

CPU  : AM335X-GP rev 2.1
Model: TI AM335x BeagleBone Black
DRAM:  512 MiB
Core:  160 devices, 18 uclasses, devicetree: separate
WDT:   Started wdt@44e35000 with servicing every 1000ms (60s timeout)
NAND:  0 MiB
MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
Loading Environment from FAT... Unable to read "uboot.env" from mmc1:1... 
<ethaddr> not set. Validating first E-fuse MAC
Net:   Could not get PHY for ethernet@4a100000: addr 0
eth2: ethernet@4a100000using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
MAC de:ad:be:ef:00:01
HOST MAC de:ad:be:ef:00:00
RNDIS ready
, eth3: usb_ether
=> bind /ocp/usb@47400000/usb@47401000 usb_ether
=> tftp 0x81000000 zImage
dev_get_priv: null device
data abort
pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
sp : 9df2f920  ip : 00000000     fp : 00000003
r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
r7 : 0000538c  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
r3 : 9ff97653  r2 : fff69249     r1 : 00000000  r0 : 00000000
Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
Code: 9ffd b508 f7e6 fc4b (6801) 2000 
Resetting CPU ...


Thanks,
Miquèl

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

[Tom Rini]

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

On Fri, Jul 28, 2023 at 02:55:23PM +0200, Miquel Raynal wrote:
> Hi Tom,
> 
> trini@konsulko.com wrote on Mon, 24 Jul 2023 14:13:45 -0400:
> 
> > On Sun, Jul 23, 2023 at 07:49:55PM +0200, Miquel Raynal wrote:
> > > Hi Marek,
> > > 
> > > marex@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +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  
> > > 
> > > I don't really get why this is needed? Yes, having proper bind and
> > > unbind methods and having them called internally is relevant, but when
> > > you have a single OTG controller, why is this needed? It basically
> > > breaks the CLI, making bisects more painful and all updates just fail.  
> > 
> > I think part of the issue here is how usb_ether didn't act like the rest
> > of the gadget do, for example fastboot.
> 
> It definitely is. "before it was working, now it does not anymore".
> That's the gut feeling many people get when the community breaks common
> habits. If we break something like this, we must at least be very clear
> on what is the new behavior.

I'm not sure that in the end there will be a behavior change here, but
the first step to figuring out if things do or don't automatically bind
right in the end is to fix the MUSB problem (see below).  Then we can
worry if there's a real behavior change or not.

> > > > 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: Marek Vasut <marex@denx.de>
> > > > Cc: Simon Glass <sjg@chromium.org>  
> > > 
> > > I've tested the whole series, unfortunately is does not work on
> > > AM335x/BBBW:
> > > 
> > > * Any recovery attempted using the network will now fail in
> > >   the SPL, where, AFAIK, there is no way to manually bind:
> > > 
> > > U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> > > Trying to boot from USB eth
> > > Could not get PHY for eth_cpsw: addr 0
> > > eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> > > MAC de:ad:be:ef:00:01
> > > HOST MAC de:ad:be:ef:00:00
> > > RNDIS ready
> > > , eth1: usb_ether  
> > 
> > For testing, what happens if you disable CPSW? Does it both bind (as it
> > shows here) and then use the expected device?
> 
> Disabling CPSW breaks the link, I had to ensure ft_board_setup in the
> am335x arch folder was still available (and returned 0). But once I
> managed to start I did not observe any improvements.
> 
> U-Boot SPL 2023.07-00806-gac80e6de9cf-dirty (Jul 28 2023 - 14:49:48 +0200)
> Trying to boot from USB eth
> Could not get PHY for eth_cpsw: addr 0
> eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> MAC de:ad:be:ef:00:01
> HOST MAC de:ad:be:ef:00:00
> RNDIS ready
> , eth1: usb_ether

So it's just not working, is what we both observe, on this platform.

> > > * The bind command was not available on my default configuration,
> > >   making it even difficult for people unaware that this command is
> > >   now required to fix their common commands.  
> > 
> > Yes, we'll need to make that default y if USB_ETHER.
> 
> Agreed.
> 
> > > * Any command that expects the usb_ether driver will now fail badly
> > >   even after the bind call:
> > >   
> > > => bind /ocp/usb@47400000/usb@47401000 usb_ether
> > > => fastboot usb 0                                  
> > > couldn't find an available UDC
> > > g_dnl_register: failed!, error: -19
> > > exit not allowed from main input shell.  
> > > => tftp 0x81000000 zImage  
> > > dev_get_priv: null device  
> > 
> > Well, does it work if you do:
> > bind /ocp/usb@47400000/usb@47401000 usb_ether
> > tftp 0x81000000 zImage
> > ?
> 
> No, I already tried both:
> 
> U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> Trying to boot from MMC2
> 
> 
> U-Boot 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> 
> CPU  : AM335X-GP rev 2.1
> Model: TI AM335x BeagleBone Black
> DRAM:  512 MiB
> Core:  160 devices, 18 uclasses, devicetree: separate
> WDT:   Started wdt@44e35000 with servicing every 1000ms (60s timeout)
> NAND:  0 MiB
> MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
> Loading Environment from FAT... Unable to read "uboot.env" from mmc1:1... 
> <ethaddr> not set. Validating first E-fuse MAC
> Net:   Could not get PHY for ethernet@4a100000: addr 0
> eth2: ethernet@4a100000using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> MAC de:ad:be:ef:00:01
> HOST MAC de:ad:be:ef:00:00
> RNDIS ready
> , eth3: usb_ether
> => bind /ocp/usb@47400000/usb@47401000 usb_ether
> => tftp 0x81000000 zImage
> dev_get_priv: null device
> data abort
> pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
> reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
> sp : 9df2f920  ip : 00000000     fp : 00000003
> r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
> r7 : 0000538c  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
> r3 : 9ff97653  r2 : fff69249     r1 : 00000000  r0 : 00000000
> Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
> Code: 9ffd b508 f7e6 fc4b (6801) 2000 
> Resetting CPU ...

I had talked with Marek a bit on IRC as well and found that yes, there's
some additional problem seemingly with MUSB.  Perhaps you can debug
what's going on here?

-- 
Tom

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

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

[Marek Vasut]

On 7/28/23 15:23, Miquel Raynal wrote:
> Hi Marek,

Hi,

Please test V3