[PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
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 *
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);
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,
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),
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
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.
[...]
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
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 ?
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?
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.
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
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 .
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
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.
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
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.
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 :)
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
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.
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
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.
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
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 ?
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.
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
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.
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
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
participants (4)
-
Marek Vasut -
Miquel Raynal -
Simon Glass -
Tom Rini