Hi Tom,
On Mon, 5 May 2025 at 18:41, Tom Rini <tr...@konsulko.com> wrote:
>
> On Mon, May 05, 2025 at 05:37:33PM +0200, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Mon, 5 May 2025 at 10:34, Quentin Schulz <quentin.sch...@cherry.de>
> > wrote:
> > >
> > > Hi Simon,
> > >
> > > On 5/1/25 3:37 PM, Simon Glass wrote:
> > > [...]
> > > > diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
> > > > index 9bee73ead58..294865feb64 100644
> > > > --- a/boot/bootstd-uclass.c
> > > > +++ b/boot/bootstd-uclass.c
> > > > @@ -6,6 +6,8 @@
> > > > * Written by Simon Glass <s...@chromium.org>
> > > > */
> > > >
> > > > +#define LOG_CATEGORY UCLASS_BOOTSTD
> > > > +
> > > > #include <alist.h>
> > > > #include <blk.h>
> > > > #include <bootdev.h>
> > > > @@ -132,6 +134,22 @@ const char *const *const
> > > > bootstd_get_bootdev_order(struct udevice *dev,
> > > > return std->bootdev_order;
> > > > }
> > > >
> > > > +void bootstd_set_bootdev_order(struct udevice *dev, const char
> > > > **order_str)
> > > > +{
> > > > + struct bootstd_priv *std = dev_get_priv(dev);
> > > > + const char **name;
> > > > +
> > > > + free(std->bootdev_order); /* leak; convert to use alist */
> > > > +
> > >
> > > leak? and aren't you using alist already?
> >
> > Not for this, yet.
> >
> > >
> > > [...]
> > >
> > > > diff --git a/doc/usage/cmd/bootdev.rst b/doc/usage/cmd/bootdev.rst
> > > > index 98a0f43c580..abede194cba 100644
> > > > --- a/doc/usage/cmd/bootdev.rst
> > > > +++ b/doc/usage/cmd/bootdev.rst
> > > > @@ -13,6 +13,7 @@ Synopsis
> > > >
> > > > bootdev list [-p] - list all available bootdevs (-p to
> > > > probe)
> > > > bootdev hunt [-l|<spec>] - use hunt drivers to find bootdevs
> > > > + bootdev order [clear] | [<spec> ...] - view or update bootdev
> > > > order
> > > > bootdev select <bm> - select a bootdev by name
> > > > bootdev info [-p] - show information about a bootdev
> > > >
> > > > @@ -78,6 +79,27 @@ To run hunters, specify the name of the hunter to
> > > > run, e.g. "mmc". If no
> > > > name is provided, all hunters are run.
> > > >
> > > >
> > > > +bootdev order
> > > > +~~~~~~~~~~~~~
> > > > +
> > > > +This allows the bootdev order to be examined or set. With no argument
> > > > the
> > > > +current ordering is shown, one item per line.
> > > > +
> > > > +The argument can either be 'clear' or a space-separated list of
> > > > labels. Each
> > > > +label can be the name of a bootdev (e.g. "mmc1.bootdev"), a bootdev
> > > > sequence
> > > > +number ("3") or a media uclass ("mmc") with an optional sequence
> > > > number (mmc2).
> > > > +
> > > > +Use `bootdev order clear` to clear any ordering and use the default.
> > > > +
> > > > +By default, the ordering is defined by the `boot_targets` environment
> > > > variable
> > > > +or, failing that, the bootstd node in the devicetree ("bootdev-order"
> > > > property).
> > > > +If no ordering is provided, then a default one is used.
> > > > +
> > >
> > > Not sure what's the benefit if we can simply set the environment variable?
> >
> > The environment variable is there to maintain backwards compatibility
> > with the distro scripts. It now seems that we are unlikely to ever
> > drop those, but you never know.
>
> Repeating myself from some other discussion before, it's not just "distro
> scripts", managing things with the environment is an intentional
> feature, not a backwards compatible legacy thing. Solutions to "boot
> this device" that do not have some support for using the environment to
> modify things as needed are unlikely to be flexible enough for all use
> cases.
For bootstd I would rather define something designed for that purpose,
than use a boot_targets environment variable. If we are relying on an
environment variable to have a particular value, then erasing the
environment might stop the board from booting. The idea with bootstd
is that it should always be able to boot.
But in any case, we have this variable today. Removing it would
require some discussion and thought and is not likely in the
foreseeable future.
Regards,
Simon
