Hi Simon,
On Mon, Aug 21, 2023 at 09:22:54PM -0600, Simon Glass wrote:
> Hi AKASHI,
>
> On Mon, 21 Aug 2023 at 20:46, AKASHI Takahiro
> <takahiro.aka...@linaro.org> wrote:
> >
> > The output from "dm tree" or "dm uclass" is a bit annoying
> > if the number of devices available on the system is huge.
> > (This is especially true on sandbox when I debug some DM code.)
> >
> > With this patch, we can specify the uclass or the device
> > that we are interested in in order to limit the output.
> >
> > For instance,
> >
> > => dm uclass usb
> > uclass 121: usb
> > 0 usb@1 @ 0bd008b0, seq 1
> >
> > => dm tree usb:usb@1
>
> Perhaps this should just provide a substring to search for and it can
> find everything with a uclass name or device name that contains the
> string?
Well, I wanted to implement the feature with minimum effort,
using the existing API, like uclass_find_device_by_name().
Well, I'll try.
> Otherwise, can you drop the usb. part ? Is it needed?
>
> > Class Index Probed Driver Name
> > -----------------------------------------------------------
> > usb 0 [ ] usb_sandbox usb@1
> > usb_hub 0 [ ] usb_hub `-- hub
> > usb_emul 0 [ ] usb_sandbox_hub `-- hub-emul
> > usb_emul 1 [ ] usb_sandbox_flash |-- flash-stick@0
> > usb_emul 2 [ ] usb_sandbox_flash |-- flash-stick@1
> > usb_emul 3 [ ] usb_sandbox_flash |-- flash-stick@2
> > usb_emul 4 [ ] usb_sandbox_keyb `-- keyb@3
> >
> > Please note that, at "dm tree", the uclass name (usb) as well as
> > the device name (usb@1) is required.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> > ---
> > cmd/dm.c | 30 ++++++++++++++++++++--------
> > drivers/core/dump.c | 48 +++++++++++++++++++++++++++++++++++++++------
> > include/dm/util.h | 13 ++++++++----
> > 3 files changed, 73 insertions(+), 18 deletions(-)
>
> Thanks, I've been wanting this for ages.
Me, too :)
> Can you please add doc/ as well?
Yes.
> >
> > diff --git a/cmd/dm.c b/cmd/dm.c
> > index 3263547cbec6..99268df2f87a 100644
> > --- a/cmd/dm.c
> > +++ b/cmd/dm.c
> > @@ -59,11 +59,20 @@ static int do_dm_dump_static_driver_info(struct cmd_tbl
> > *cmdtp, int flag,
> > static int do_dm_dump_tree(struct cmd_tbl *cmdtp, int flag, int argc,
> > char *const argv[])
> > {
> > - bool sort;
> > + bool sort = false;
> > + char *device = NULL;
> >
> > - sort = argc > 1 && !strcmp(argv[1], "-s");
> > + if (argc > 1) {
> > + if (!strcmp(argv[1], "-s")) {
> > + sort = true;
> > + argc--;
> > + argv++;
> > + }
> > + if (argc > 1)
> > + device = argv[1];
> > + }
> >
> > - dm_dump_tree(sort);
> > + dm_dump_tree(device, sort);
> >
> > return 0;
> > }
> > @@ -71,7 +80,12 @@ static int do_dm_dump_tree(struct cmd_tbl *cmdtp, int
> > flag, int argc,
> > static int do_dm_dump_uclass(struct cmd_tbl *cmdtp, int flag, int argc,
> > char *const argv[])
> > {
> > - dm_dump_uclass();
> > + char *uclass = NULL;
> > +
> > + if (argc > 1)
> > + uclass = argv[1];
> > +
> > + dm_dump_uclass(uclass);
> >
> > return 0;
> > }
> > @@ -91,8 +105,8 @@ static char dm_help_text[] =
> > "dm drivers Dump list of drivers with uclass and instances\n"
> > DM_MEM_HELP
> > "dm static Dump list of drivers with static platform data\n"
> > - "dm tree [-s] Dump tree of driver model devices (-s=sort)\n"
> > - "dm uclass Dump list of instances for each uclass"
> > + "dm tree [-s][name] Dump tree of driver model devices (-s=sort)\n"
> > + "dm uclass [name] Dump list of instances for each uclass"
> > ;
> > #endif
> >
> > @@ -102,5 +116,5 @@ U_BOOT_CMD_WITH_SUBCMDS(dm, "Driver model low level
> > access", dm_help_text,
> > U_BOOT_SUBCMD_MKENT(drivers, 1, 1, do_dm_dump_drivers),
> > DM_MEM
> > U_BOOT_SUBCMD_MKENT(static, 1, 1, do_dm_dump_static_driver_info),
> > - U_BOOT_SUBCMD_MKENT(tree, 2, 1, do_dm_dump_tree),
> > - U_BOOT_SUBCMD_MKENT(uclass, 1, 1, do_dm_dump_uclass));
> > + U_BOOT_SUBCMD_MKENT(tree, 3, 1, do_dm_dump_tree),
> > + U_BOOT_SUBCMD_MKENT(uclass, 2, 1, do_dm_dump_uclass));
> > diff --git a/drivers/core/dump.c b/drivers/core/dump.c
> > index 3e77832a3a00..855d6ca002b7 100644
> > --- a/drivers/core/dump.c
> > +++ b/drivers/core/dump.c
> > @@ -85,11 +85,35 @@ static void show_devices(struct udevice *dev, int
> > depth, int last_flag,
> > }
> > }
> >
> > -void dm_dump_tree(bool sort)
> > +void dm_dump_tree(char *uclass, bool sort)
> > {
> > struct udevice *root;
> >
> > - root = dm_root();
> > + if (uclass) {
> > + char *device;
> > + enum uclass_id id;
> > +
> > + device = strchr(uclass, ':');
> > + if (!device) {
> > + printf("name %s: invalid format\n", uclass);
> > + return;
> > + }
>
> Can the parsing needs to happen in the caller? This might be called
> from SPL, for example. We don't want it returning an error
Do you mean that all the messages should be suppressed
in dm_dump_tree()?
> > + *device = '\0';
> > + device++;
> > +
> > + id = uclass_get_by_name(uclass);
> > + if (id == UCLASS_INVALID) {
> > + printf("uclass %s: not found\n", uclass);
> > + return;
> > + }
> > + if (uclass_find_device_by_name(id, device, &root)) {
> > + printf("device %s:%s: not found\n", uclass, device);
> > + return;
> > + }
> > + } else {
> > + root = dm_root();
> > + }
> > +
> > if (root) {
> > int dev_count, uclasses;
> > struct udevice **devs = NULL;
> > @@ -127,13 +151,25 @@ static void dm_display_line(struct udevice *dev, int
> > index)
> > puts("\n");
> > }
> >
> > -void dm_dump_uclass(void)
> > +void dm_dump_uclass(char *uclass)
> > {
> > struct uclass *uc;
> > - int ret;
> > - int id;
> > + enum uclass_id id;
> > + int count, ret;
> > +
> > + if (uclass) {
> > + id = uclass_get_by_name(uclass);
> > + if (id == UCLASS_INVALID) {
> > + printf("uclass %s: not found\n", uclass);
> > + return;
> > + }
> > + count = 1;
> > + } else {
> > + id = 0;
> > + count = UCLASS_COUNT;
> > + }
> >
> > - for (id = 0; id < UCLASS_COUNT; id++) {
> > + for ( ; count; id++, count--) {
> > struct udevice *dev;
> > int i = 0;
> >
> > diff --git a/include/dm/util.h b/include/dm/util.h
> > index 4bb49e9e8c01..ee1b34c103e3 100644
> > --- a/include/dm/util.h
> > +++ b/include/dm/util.h
> > @@ -27,14 +27,19 @@ struct list_head;
> > int list_count_items(struct list_head *head);
> >
> > /**
> > - * Dump out a tree of all devices
> > + * Dump out a tree of all devices starting @uclass
> > *
> > + * @uclass: uclass+device name
>
> yes, but what does it mean?
In v1, the command expects "<uclass name>:<udevice name>" here
so that we can use uclass_find_device_by_name().
I will relax this restriction, by allowing "forward-matching"
against a device name, in v2.
> > * @sort: Sort by uclass name
> > */
> > -void dm_dump_tree(bool sort);
> > +void dm_dump_tree(char *uclass, bool sort);
> >
> > -/* Dump out a list of uclasses and their devices */
> > -void dm_dump_uclass(void);
> > +/*
> > + * Dump out a list of uclasses and their devices
> > + *
> > + * @uclass: uclass name
>
> That's a bit vague. What is it for?
Literally, a uclass name like "usb".
Does it sound ambiguous?
-Takahiro Akashi
> > + */
> > +void dm_dump_uclass(char *uclass);
> >
> > #ifdef CONFIG_DEBUG_DEVRES
> > /* Dump out a list of device resources */
> > --
> > 2.34.1
> >
>
> Regards,
> Simon
