[U-Boot] [PATCH v2 3/5] cmd: Add bind/unbind commands to bind a device to a driver from the command line
Simon Glass
sjg at chromium.org
Wed Jun 20 17:52:03 UTC 2018
Hi Jean-Jacques,
On 18 June 2018 at 07:56, Jean-Jacques Hiblot <jjhiblot at ti.com> 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 at 48380000/usb at 48390000 usb_ether
> unbind /ocp/omap_dwc3 at 48380000/usb at 48390000
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
>
> ---
>
> 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 <jjhiblot at ti.com>
> + */
> +
> +#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
More information about the U-Boot
mailing list