Hi Jean-Jacques, On 18 June 2018 at 07:56, Jean-Jacques Hiblot <[email protected]> wrote: > In some cases it can be useful to be able to bind a device to a driver from > the command line. > The obvious example is for versatile devices such as USB gadget. > Another use case is when the devices are not yet ready at startup and > require some setup before the drivers are bound (ex: FPGA which bitsream is > fetched from a mass storage or ethernet) > > usage example: > > bind usb_dev_generic 0 usb_ether > unbind usb_dev_generic 0 usb_ether > or > unbind eth 1 > > bind /ocp/omap_dwc3@48380000/usb@48390000 usb_ether > unbind /ocp/omap_dwc3@48380000/usb@48390000 > > Signed-off-by: Jean-Jacques Hiblot <[email protected]> > > --- > > Changes in v2: > - Make the bind/unbind command generic, not specific to usb device. > - Update the API to be able to bind/unbind based on DTS node path > - Add a Kconfig option to select the bind/unbind commands > > cmd/Kconfig | 9 +++ > cmd/Makefile | 1 + > cmd/bind.c | 256 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 266 insertions(+) > create mode 100644 cmd/bind.c
This looks good to me, with a few comments below. I think it is great to get this sort of functionality in U-Boot. Please can you add a test which calls issues a few of these commands? You might find test_shell_basics.py useful as a starting point. > > diff --git a/cmd/Kconfig b/cmd/Kconfig > index 1eb55e5..d6bbfba 100644 > --- a/cmd/Kconfig > +++ b/cmd/Kconfig > @@ -607,6 +607,15 @@ config CMD_ADC > Shows ADC device info and permit printing one-shot analog converted > data from a named Analog to Digital Converter. > > +config CMD_BIND > + bool "bind/unbind - Bind or unbind a device to/from a driver" > + depends on DM > + help > + Bind or unbind a device to/from a driver from the command line. > + This is useful in situations where a device may be handled by > several > + drivers. For example, this can be used to bind a UDC to the usb > ether > + gadget driver from the command line. > + > config CMD_CLK > bool "clk - Show clock frequencies" > help > diff --git a/cmd/Makefile b/cmd/Makefile > index e0088df..b12aca3 100644 > --- a/cmd/Makefile > +++ b/cmd/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_SOURCE) += source.o > obj-$(CONFIG_CMD_SOURCE) += source.o > obj-$(CONFIG_CMD_BDI) += bdinfo.o > obj-$(CONFIG_CMD_BEDBUG) += bedbug.o > +obj-$(CONFIG_CMD_BIND) += bind.o > obj-$(CONFIG_CMD_BINOP) += binop.o > obj-$(CONFIG_CMD_BLOCK_CACHE) += blkcache.o > obj-$(CONFIG_CMD_BMP) += bmp.o > diff --git a/cmd/bind.c b/cmd/bind.c > new file mode 100644 > index 0000000..60d84d6 > --- /dev/null > +++ b/cmd/bind.c > @@ -0,0 +1,256 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2018 JJ Hiblot <[email protected]> > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <dm/device-internal.h> > +#include <dm/lists.h> > +#include <dm/uclass-internal.h> > + > +static int bind_by_class_index(const char *uclass, int index, > + const char *drv_name) > +{ > + static enum uclass_id uclass_id; > + struct udevice *dev; > + struct udevice *parent; > + int ret; > + struct driver *drv; > + > + drv = lists_driver_lookup_name(drv_name); > + if (!drv) { > + printf("Cannot find driver '%s'\n", drv_name); > + return -ENOENT; > + } > + > + uclass_id = uclass_get_by_name(uclass); > + if (uclass_id == UCLASS_INVALID) { > + printf("%s is not a valid uclass\n", uclass); > + return -EINVAL; > + } > + > + ret = uclass_find_device(uclass_id, index, &parent); > + if (!parent || ret) { > + printf("Cannot find device %d of class %s\n", index, uclass); > + return ret; > + } > + > + ret = device_bind_with_driver_data(parent, drv, drv->name, 0, > + ofnode_null(), &dev); > + if (!dev || ret) { > + printf("Unable to bind. err:%d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int unbind_by_class_index(const char *uclass, int index) > +{ > + static enum uclass_id uclass_id; > + struct udevice *dev; > + int ret; > + > + uclass_id = uclass_get_by_name(uclass); > + if (uclass_id == UCLASS_INVALID) { > + printf("%s is not a valid uclass\n", uclass); > + return -EINVAL; > + } > + > + ret = uclass_find_device(uclass_id, index, &dev); > + if (!dev || ret) { > + printf("Cannot find device %d of class %s\n", index, uclass); > + return ret; > + } > + > + ret = device_unbind(dev); > + if (ret) { > + printf("Unable to unbind. err:%d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int unbind_child_by_class_index(const char *uclass, int index, > + const char *drv_name) > +{ > + static enum uclass_id uclass_id; > + struct udevice *parent; > + struct udevice *dev, *n; > + int ret; > + struct driver *drv; > + > + drv = lists_driver_lookup_name(drv_name); > + if (!drv) { > + printf("Cannot find driver '%s'\n", drv_name); > + return -ENOENT; > + } > + > + uclass_id = uclass_get_by_name(uclass); > + if (uclass_id == UCLASS_INVALID) { > + printf("%s is not a valid uclass\n", uclass); > + return -EINVAL; > + } > + > + ret = uclass_find_device(uclass_id, index, &parent); > + if (!parent || ret) { > + printf("Cannot find device %d of class %s\n", index, uclass); > + return ret; > + } The above two blocks duplicate code in the function above. Can you put then in a separate function can call it here? > + > + list_for_each_entry_safe(dev, n, &parent->child_head, sibling_node) { This feels like code that should be in drivers/core since it is messing with the internal lists and driver names. But I suppose for now we can keep it here. Let's see how things expand later. > + int rc; > + > + if (dev->driver != drv) > + continue; > + > + rc = device_unbind(dev); > + if (rc && !ret) { > + printf("failed to unbind %s/%s\n", dev->name, > + drv->name); > + ret = rc; > + } > + } > + > + return ret; > +} > + > +static int bind_by_node_path(const char *path, const char *drv_name) > +{ > + struct udevice *dev; > + struct udevice *parent = NULL; > + int ret; > + int id; > + ofnode ofnode; > + struct driver *drv; > + > + drv = lists_driver_lookup_name(drv_name); > + if (!drv) { > + printf("%s is not a valid driver name\n", drv_name); > + return -ENOENT; > + } > + > + ofnode = ofnode_path(path); > + if (!ofnode_valid(ofnode)) { > + printf("%s is not a valid node path\n", path); > + return -EINVAL; > + } > + > + while (ofnode_valid(ofnode)) { > + for (id = 0; id < UCLASS_COUNT; id++) { > + if (!uclass_find_device_by_ofnode(id, ofnode, > &parent)) > + break; > + } Can you create a function in drivers/core that finds an ofnode globally? I don't like having this logic here. You could convert device_get_global_by_of_offset() to use an ofnode. Remember that any printf() calls should be here, not in drivers/core. > + if (parent) > + break; > + ofnode = ofnode_get_parent(ofnode); > + } > + > + if (!parent) { > + printf("Cannot find a parent device for node path %s\n", > path); > + return -ENODEV; > + } > + > + ret = device_bind_with_driver_data(parent, drv, drv->name, 0, > + ofnode_path(path), &dev); > + if (!dev || ret) { > + printf("Unable to bind. err:%d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int unbind_by_node_path(const char *path) > +{ > + struct udevice *dev; > + int ret; > + int id; > + ofnode ofnode; > + > + ofnode = ofnode_path(path); > + if (!ofnode_valid(ofnode)) { > + printf("%s is not a valid node path\n", path); > + return -EINVAL; > + } > + > + for (id = 0; id < UCLASS_COUNT; id++) { > + if (uclass_find_device_by_ofnode(id, ofnode, &dev) == 0) > + break; > + } Same comment as above - move this logic to drivers/core > + > + if (!dev) { > + printf("Cannot find a device with path %s\n", path); > + return -ENODEV; > + } > + > + ret = device_unbind(dev); > + if (ret) { > + printf("Unable to unbind. err:%d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int do_bind_unbind(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int ret; > + bool bind; > + bool by_node; > + > + if (argc < 2) > + return CMD_RET_USAGE; > + > + bind = (argv[0][0] == 'b'); > + by_node = (argv[1][0] == '/'); > + > + if (by_node && bind) { > + if (argc != 3) > + return CMD_RET_USAGE; > + ret = bind_by_node_path(argv[1], argv[2]); > + } else if (by_node && !bind) { > + if (argc != 2) > + return CMD_RET_USAGE; > + ret = unbind_by_node_path(argv[1]); > + } else if (!by_node && bind) { > + int index = (argc > 2) ? simple_strtoul(argv[2], NULL, 10) : > 0; > + > + if (argc != 4) > + return CMD_RET_USAGE; > + ret = bind_by_class_index(argv[1], index, argv[3]); > + } else if (!by_node && !bind) { > + int index = (argc > 2) ? simple_strtoul(argv[2], NULL, 10) : > 0; > + > + if (argc == 3) > + ret = unbind_by_class_index(argv[1], index); > + else if (argc == 4) > + ret = unbind_child_by_class_index(argv[1], index, > + argv[3]); > + else > + return CMD_RET_USAGE; > + } > + > + if (ret) > + return CMD_RET_FAILURE; > + else > + return CMD_RET_SUCCESS; > +} > + > +U_BOOT_CMD( > + bind, 4, 0, do_bind_unbind, > + "Bind a device to a driver", > + "<node path> <driver>\n" > + "bind <class> <index> <driver>\n" > +); > + > +U_BOOT_CMD( > + unbind, 4, 0, do_bind_unbind, > + "Unbind a device from a driver", > + "<node path>\n" > + "unbind <class> <index>\n" > + "unbind <class> <index> <driver>\n" > +); > -- > 2.7.4 > Regards, Simon _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
