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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Marek,

marex@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +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
> "
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---

I am no longer getting wrong pointer dereferences, the SPL is working in
recovery mode, TFTP "File not found" errors are no longer a problem and
I did not experience any reset while tftp'ing regular files.

One last remaining request on my side is the need for using fastboot as
well which does no longer work as-is:

=> fastboot usb 0
couldn't find an available UDC
g_dnl_register: failed!, error: -19
exit not allowed from main input shell.

Can you advise what bind/unbind command would be necessary here?

Thanks,
Miquèl

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