Re: [PATCH] bootstd: USB devtype detection for script boot

Simon Glass
2023-06-29T19:20:47Z

Hi John,

On Thu, 29 Jun 2023 at 01:03, John Clark <inindev@gmail.com> wrote:
>
> > I only just thought of this, but I think it is better to check
> blk->uclass_id == UCLASS_USB instead, since it avoids a string comparison.
>
> I am not finding anything as direct as your pseudo code above suggests.
> The closest I can find is a compare like this:
>
> UCLASS_MASS_STORAGE == device_get_uclass_id(dev_get_parent(bflow->blk))
>
> Which has the overhead of calling two new functions that otherwise would
> not be called and could match other mass storage devices other than USB
> (am I right to assume this?).
>
> Can you shed additional insight into "blk->uclass_id"?

Oh sorry, I mean the blk_desc structure attached to the block device.

struct blk_desc *desc = dev_get_uclass_plat(blk);

Then desc->uclass_id is what you want. It is the uclass of the media
device (typically the parent of the block device).

>
> blk is a udevice without a uclass_id member:
>
> ./dm/device.h
>
> struct udevice {
> const struct driver *driver;
> const char *name;
> void *plat_;
> void *parent_plat_;
> void *uclass_plat_;
> ulong driver_data;
> struct udevice *parent;
> void *priv_;
> struct uclass *uclass;
> void *uclass_priv_;
> void *parent_priv_;
> struct list_head uclass_node;
> struct list_head child_head;
> struct list_head sibling_node;
> #if !CONFIG_IS_ENABLED(OF_PLATDATA_RT)
> u32 flags_;
> #endif
> int seq_;
> #if CONFIG_IS_ENABLED(OF_REAL)
> ofnode node_;
> #endif
> #if CONFIG_IS_ENABLED(DEVRES)
> struct list_head devres_head;
> #endif
> #if CONFIG_IS_ENABLED(DM_DMA)
> ulong dma_offset;
> #endif
> #if CONFIG_IS_ENABLED(IOMMU)
> struct udevice *iommu;
> #endif
> };

You can use device_get_uclass_id(blk) to get the uclass ID of the
block device, but of course it will just be UCLASS_BLK.

>
>
> I am not a prolific open source contributor, so I am not sure if a
> side-bar conversation like this is one we should be having with the
> whole list? Something this trivial seems like it would just be noise to
> include hundreds of people on.
>
> I appreciate your advice here, and thanks for your time.

You can just copy the list on all emails. I've done it here.

Regards,
Simon

>
>
> John Clark
>
>
> On 6/28/23 3:42 AM, Simon Glass wrote:
> > Hi John,
> >
> > On Tue, 27 Jun 2023 at 15:39, John Clark <inindev@gmail.com> wrote:
> >> Change the device type from "usb_mass_storage" to "usb" when
> >> booting a script.
> >>
> >> Before this change:
> >> => printenv devtype
> >> devtype=usb_mass_storage
> >>
> >> After this change:
> >> => printenv devtype
> >> devtype=usb
> >>
> >> Signed-off-by: John Clark <inindev@gmail.com>
> >> ---
> >>
> >> boot/bootmeth_script.c | 8 ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/boot/bootmeth_script.c b/boot/bootmeth_script.c
> >> index 225eb18ee6..9fdadb3005 100644
> >> --- a/boot/bootmeth_script.c
> >> +++ b/boot/bootmeth_script.c
> >> @@ -187,10 +187,14 @@ static int script_set_bootflow(struct udevice *dev, struct bootflow *bflow,
> >> static int script_boot(struct udevice *dev, struct bootflow *bflow)
> >> {
> >> struct blk_desc *desc = dev_get_uclass_plat(bflow->blk);
> >> + const char *devtype = blk_get_devtype(bflow->blk);
> >> ulong addr;
> >> int ret;
> >>
> >> - ret = env_set("devtype", blk_get_devtype(bflow->blk));
> >> + if (!strcmp("usb_mass_storage", devtype))
> > I only just thought of this, but I think it is better to check
> > blk->uclass_id == UCLASS_USB instead, since it avoids a string
> > comparison.
> >
> >> + ret = env_set("devtype", "usb");
> >> + else
> >> + ret = env_set("devtype", devtype);
> >> if (!ret)
> >> ret = env_set_hex("devnum", desc->devnum);
> >> if (!ret)
> >> @@ -198,7 +202,7 @@ static int script_boot(struct udevice *dev, struct bootflow *bflow)
> >> if (!ret)
> >> ret = env_set("prefix", bflow->subdir);
> >> if (!ret && IS_ENABLED(CONFIG_ARCH_SUNXI) &&
> >> - !strcmp("mmc", blk_get_devtype(bflow->blk)))
> >> + !strcmp("mmc", devtype))
> >> ret = env_set_hex("mmc_bootdev", desc->devnum);
> >> if (ret)
> >> return log_msg_ret("env", ret);
> >> --
> >> 2.39.2
> >>
> > Regards,
> > Simon