[RFC PATCH 0/4] dm: Duplicate uclass name fix and alias improvements
Patch 1 is a tentative fix for duplicate uclass name issue met in https://lists.denx.de/pipermail/u-boot/2024-July/560189.html
The idea is to use orignal class name only for sequence alias to keep this alias function working and rename the class something else.
Patch 2 and 3 make "dm tree", "bind" and "unbind" commands to take care of alias sequence numbering. As the alias sequence numbering is more meanful than uclass index.
Patch 4 is the actual fix for usb gadet class.
There are some situations where uclass index and device sequence number are misused which is not convered by this patch set, for example in drivers/net/sandbox.c, uclass_get_device() is used which is based on uclass index, while in the comments it says "index - The alias index (also DM seq number)"
Zixun LI (4): dm: core: Add a way to specify an alt name for alias sequence numbering dm: core: Show device sequence instead in dm_dump_tree() cmd: bind: Use device sequence instead for driver bind/unbind usb: gadget: udc: Fix duplicate uclass name
cmd/bind.c | 4 ++-- drivers/core/device.c | 3 ++- drivers/core/dump.c | 2 +- drivers/core/read.c | 7 ++++++- drivers/core/uclass.c | 11 +++++++++-- drivers/usb/gadget/udc/udc-uclass.c | 3 ++- include/dm/read.h | 9 ++++++++- include/dm/uclass.h | 2 ++ 8 files changed, 32 insertions(+), 9 deletions(-)
-- 2.45.2
A new field name_seq_alias is added to uclass_driver structure, which allows an uclass driver to use an alternate name for alias sequence numbering.
For example an uclass named "usb_gadget" can share alias with "usb" uclass : UCLASS_DRIVER(usb_gadget_generic) = { .id = UCLASS_USB_GADGET_GENERIC, .name = "usb_gadget", .name_seq_alias = "usb", .flags = DM_UC_FLAG_SEQ_ALIAS, };
Currently there are some uclasses with duplicate name which break uclass functions like uclass_get_by_name(), with this patch it's now possible to rename these classes without break the existing alias function.
Signed-off-by: Zixun LI admin@hifiphile.com --- drivers/core/device.c | 3 ++- drivers/core/read.c | 7 ++++++- drivers/core/uclass.c | 11 +++++++++-- include/dm/read.h | 9 ++++++++- include/dm/uclass.h | 2 ++ 5 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 779f371b9d..a98e75b0b8 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -89,7 +89,8 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv, */ if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) { - if (uc->uc_drv->name && ofnode_valid(node)) { + if ((uc->uc_drv->name || uc->uc_drv->name_seq_alias) && + ofnode_valid(node)) { if (!dev_read_alias_seq(dev, &dev->seq_)) { auto_seq = false; log_debug(" - seq=%d\n", dev->seq_); diff --git a/drivers/core/read.c b/drivers/core/read.c index 55c19f335a..97b12fb7a9 100644 --- a/drivers/core/read.c +++ b/drivers/core/read.c @@ -347,9 +347,14 @@ const void *dev_read_prop_by_prop(struct ofprop *prop, int dev_read_alias_seq(const struct udevice *dev, int *devnump) { ofnode node = dev_ofnode(dev); - const char *uc_name = dev->uclass->uc_drv->name; + const char *uc_name; int ret = -ENOTSUPP;
+ if (dev->uclass->uc_drv->name_seq_alias) + uc_name = dev->uclass->uc_drv->name_seq_alias; + else + uc_name = dev->uclass->uc_drv->name; + if (ofnode_is_np(node)) { ret = of_alias_get_id(ofnode_to_np(node), uc_name); if (ret >= 0) { diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index 7ae0884a75..6dfbee2e78 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -308,11 +308,18 @@ int uclass_find_next_free_seq(struct uclass *uc) { struct udevice *dev; int max = -1; + const char *uc_name;
/* If using aliases, start with the highest alias value */ if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) && - (uc->uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) - max = dev_read_alias_highest_id(uc->uc_drv->name); + (uc->uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) { + if (uc->uc_drv->name_seq_alias) + uc_name = uc->uc_drv->name_seq_alias; + else + uc_name = uc->uc_drv->name; + + max = dev_read_alias_highest_id(uc_name); + }
/* Avoid conflict with existing devices */ list_for_each_entry(dev, &uc->dev_head, uclass_node) { diff --git a/include/dm/read.h b/include/dm/read.h index 894bc698bb..5a86e43d86 100644 --- a/include/dm/read.h +++ b/include/dm/read.h @@ -1169,7 +1169,14 @@ static inline const void *dev_read_prop_by_prop(struct ofprop *prop, static inline int dev_read_alias_seq(const struct udevice *dev, int *devnump) { #if CONFIG_IS_ENABLED(OF_CONTROL) - return fdtdec_get_alias_seq(gd->fdt_blob, dev->uclass->uc_drv->name, + const char *uc_name; + + if (dev->uclass->uc_drv->name_seq_alias) + uc_name = dev->uclass->uc_drv->name_seq_alias; + else + uc_name = dev->uclass->uc_drv->name; + + return fdtdec_get_alias_seq(gd->fdt_blob, uc_name, dev_of_offset(dev), devnump); #else return -ENOTSUPP; diff --git a/include/dm/uclass.h b/include/dm/uclass.h index 456eef7f2f..8339e0d88b 100644 --- a/include/dm/uclass.h +++ b/include/dm/uclass.h @@ -57,6 +57,7 @@ struct udevice; * drivers. * * @name: Name of uclass driver + * @name_seq_alias: Name used for alias sequence numbering * @id: ID number of this uclass * @post_bind: Called after a new device is bound to this uclass * @pre_unbind: Called before a device is unbound from this uclass @@ -88,6 +89,7 @@ struct udevice; */ struct uclass_driver { const char *name; + const char *name_seq_alias; enum uclass_id id; int (*post_bind)(struct udevice *dev); int (*pre_unbind)(struct udevice *dev);
Currently uclass index is shown in DM tree dump which ignores alias sequence numbering. The result could be confusing since these 2 numbers could be different. Show device sequence number instead.
Signed-off-by: Zixun LI admin@hifiphile.com --- drivers/core/dump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/core/dump.c b/drivers/core/dump.c index 5ec30d5b3c..c812d87fa4 100644 --- a/drivers/core/dump.c +++ b/drivers/core/dump.c @@ -40,7 +40,7 @@ static void show_devices(struct udevice *dev, int depth, int last_flag, /* print the first 20 characters to not break the tree-format. */ printf(CONFIG_IS_ENABLED(USE_TINY_PRINTF) ? " %s %d [ %c ] %s " : " %-10.10s %3d [ %c ] %-20.20s ", dev->uclass->uc_drv->name, - dev_get_uclass_index(dev, NULL), + dev->seq_, flags & DM_FLAG_ACTIVATED ? '+' : ' ', dev->driver->name);
for (i = depth; i >= 0; i--) {
Hi Zixun,
On Wed, 31 Jul 2024 at 07:43, Zixun LI admin@hifiphile.com wrote:
Currently uclass index is shown in DM tree dump which ignores alias sequence numbering. The result could be confusing since these 2 numbers could be different. Show device sequence number instead.
I think it makes sense to show the seq instead of the index, but please update the header for the table to say 'Seq'
Signed-off-by: Zixun LI admin@hifiphile.com
drivers/core/dump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/core/dump.c b/drivers/core/dump.c index 5ec30d5b3c..c812d87fa4 100644 --- a/drivers/core/dump.c +++ b/drivers/core/dump.c @@ -40,7 +40,7 @@ static void show_devices(struct udevice *dev, int depth, int last_flag, /* print the first 20 characters to not break the tree-format. */ printf(CONFIG_IS_ENABLED(USE_TINY_PRINTF) ? " %s %d [ %c ] %s " : " %-10.10s %3d [ %c ] %-20.20s ", dev->uclass->uc_drv->name,
dev_get_uclass_index(dev, NULL),
dev->seq_, flags & DM_FLAG_ACTIVATED ? '+' : ' ', dev->driver->name); for (i = depth; i >= 0; i--) {-- 2.45.2
Regards, Simon
Currently uclass index is used for bind/unbind which ignores alias sequence numbering. Use device sequence number instead as it's the number explicitly set in the DT.
Signed-off-by: Zixun LI admin@hifiphile.com --- cmd/bind.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/bind.c b/cmd/bind.c index 3a59eefd5c..30f1163e17 100644 --- a/cmd/bind.c +++ b/cmd/bind.c @@ -31,7 +31,7 @@ static int bind_by_class_index(const char *uclass, int index, return -EINVAL; }
- ret = uclass_find_device(uclass_id, index, &parent); + ret = uclass_find_device_by_seq(uclass_id, index, &parent); if (!parent || ret) { printf("Cannot find device %d of class %s\n", index, uclass); return ret; @@ -58,7 +58,7 @@ static int find_dev(const char *uclass, int index, struct udevice **devp) return -EINVAL; }
- rc = uclass_find_device(uclass_id, index, devp); + rc = uclass_find_device_by_seq(uclass_id, index, devp); if (!*devp || rc) { printf("Cannot find device %d of class %s\n", index, uclass); return rc;
Hi Zixun,
On Wed, 31 Jul 2024 at 07:43, Zixun LI admin@hifiphile.com wrote:
Currently uclass index is used for bind/unbind which ignores alias sequence numbering. Use device sequence number instead as it's the number explicitly set in the DT.
Signed-off-by: Zixun LI admin@hifiphile.com
cmd/bind.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I assume no tests need updating, but please do add a note to doc/usage/cmd/bind.rst
diff --git a/cmd/bind.c b/cmd/bind.c index 3a59eefd5c..30f1163e17 100644 --- a/cmd/bind.c +++ b/cmd/bind.c @@ -31,7 +31,7 @@ static int bind_by_class_index(const char *uclass, int index, return -EINVAL; }
ret = uclass_find_device(uclass_id, index, &parent);
ret = uclass_find_device_by_seq(uclass_id, index, &parent); if (!parent || ret) { printf("Cannot find device %d of class %s\n", index, uclass); return ret;@@ -58,7 +58,7 @@ static int find_dev(const char *uclass, int index, struct udevice **devp) return -EINVAL; }
rc = uclass_find_device(uclass_id, index, devp);
rc = uclass_find_device_by_seq(uclass_id, index, devp); if (!*devp || rc) { printf("Cannot find device %d of class %s\n", index, uclass); return rc;-- 2.45.2
Regards, Simon
Currently both USB host uclass and USB gadget uclass are using the same name "usb" which break uclass functions like uclass_get_by_name().
Rename the uclass to "usb_gadget" while using "usb" as sequence alias naming to keep the alias function working.
Signed-off-by: Zixun LI admin@hifiphile.com --- drivers/usb/gadget/udc/udc-uclass.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/udc-uclass.c b/drivers/usb/gadget/udc/udc-uclass.c index fbe62bbce4..b14f84e818 100644 --- a/drivers/usb/gadget/udc/udc-uclass.c +++ b/drivers/usb/gadget/udc/udc-uclass.c @@ -83,7 +83,8 @@ __weak int dm_usb_gadget_handle_interrupts(struct udevice *dev) #if CONFIG_IS_ENABLED(DM) UCLASS_DRIVER(usb_gadget_generic) = { .id = UCLASS_USB_GADGET_GENERIC, - .name = "usb", + .name = "usb_gadget", + .name_seq_alias = "usb", .flags = DM_UC_FLAG_SEQ_ALIAS, }; #endif
Hi Zixun,
On Wed, 31 Jul 2024 at 07:43, Zixun LI admin@hifiphile.com wrote:
Patch 1 is a tentative fix for duplicate uclass name issue met in https://lists.denx.de/pipermail/u-boot/2024-July/560189.html
The idea is to use orignal class name only for sequence alias to keep this alias function working and rename the class something else.
Can we rename the gadget uclass to usb_gadget or similar, then update the aliases?
Patch 2 and 3 make "dm tree", "bind" and "unbind" commands to take care of alias sequence numbering. As the alias sequence numbering is more meanful than uclass index.
Patch 4 is the actual fix for usb gadet class.
There are some situations where uclass index and device sequence number are misused which is not convered by this patch set, for example in drivers/net/sandbox.c, uclass_get_device() is used which is based on uclass index, while in the comments it says "index - The alias index (also DM seq number)"
Zixun LI (4): dm: core: Add a way to specify an alt name for alias sequence numbering dm: core: Show device sequence instead in dm_dump_tree() cmd: bind: Use device sequence instead for driver bind/unbind usb: gadget: udc: Fix duplicate uclass name
cmd/bind.c | 4 ++-- drivers/core/device.c | 3 ++- drivers/core/dump.c | 2 +- drivers/core/read.c | 7 ++++++- drivers/core/uclass.c | 11 +++++++++-- drivers/usb/gadget/udc/udc-uclass.c | 3 ++- include/dm/read.h | 9 ++++++++- include/dm/uclass.h | 2 ++ 8 files changed, 32 insertions(+), 9 deletions(-)
-- 2.45.2
Regards, Simon
Hi Simon,
On Thu, Aug 1, 2024 at 4:42 PM Simon Glass sjg@chromium.org wrote:
Can we rename the gadget uclass to usb_gadget or similar, then update the aliases?
You mean split patch no.4 into 2 parts, first rename the class to usb_gadget before doing the alias job ?
I think it makes sense to show the seq instead of the index, but please update the header for the table to say 'Seq'
Will do.
I assume no tests need updating, but please do add a note to doc/usage/cmd/bind.rst
Will do.
Regards, Zixun LI
Hi Zixun,
On Thu, 1 Aug 2024 at 12:37, Zixun LI admin@hifiphile.com wrote:
Hi Simon,
On Thu, Aug 1, 2024 at 4:42 PM Simon Glass sjg@chromium.org wrote:
Can we rename the gadget uclass to usb_gadget or similar, then update the aliases?
You mean split patch no.4 into 2 parts, first rename the class to usb_gadget before doing the alias job ?
Yes I suppose so. I am trying to avoid adding the extra field to the uclass, just for the use of one uclass.
I think it makes sense to show the seq instead of the index, but please update the header for the table to say 'Seq'
Will do.
I assume no tests need updating, but please do add a note to doc/usage/cmd/bind.rst
Will do.
Regards, Zixun LI
Regards, Simon
participants (2)
-
Simon Glass -
Zixun LI