[PATCH v3 31/31] RFC: Switch rpi over to use bootstd

[Simon Glass]

()Hi Mark,

On Wed, 19 Jan 2022 at 09:38, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > Date: Wed, 19 Jan 2022 11:21:17 -0500
> > From: Tom Rini <trini at konsulko.com>
> >
> > On Wed, Jan 19, 2022 at 09:09:51AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 19 Jan 2022 at 08:01, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Wed, Jan 19, 2022 at 07:37:36AM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Wed, 19 Jan 2022 at 07:04, Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, Jan 18, 2022 at 06:43:15PM -0700, Simon Glass wrote:
> > > > > >
> > > > > > > Switch this over, for testing purposes.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v1)
> > > > > > >
> > > > > > >  boot/Kconfig          |  3 ++-
> > > > > > >  include/configs/rpi.h | 39 ++-------------------------------------
> > > > > > >  2 files changed, 4 insertions(+), 38 deletions(-)
> > > > > > >
> > > > > > > diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > index 9cf1d013f20..eab3c0f3467 100644
> > > > > > > --- a/boot/Kconfig
> > > > > > > +++ b/boot/Kconfig
> > > > > > > @@ -1124,7 +1124,8 @@ config USE_BOOTCOMMAND
> > > > > > >  config BOOTCOMMAND
> > > > > > >       string "bootcmd value"
> > > > > > >       depends on USE_BOOTCOMMAND && !USE_DEFAULT_ENV_FILE
> > > > > > > -     default "run distro_bootcmd" if DISTRO_DEFAULTS
> > > > > > > +     default "bootflow scan -lb" if BOOTSTD
> > > > > > > +     default "run distro_bootcmd" if !BOOTSTD && DISTRO_DEFAULTS
> > > > > > >       help
> > > > > > >         This is the string of commands that will be used as bootcmd and if
> > > > > > >         AUTOBOOT is set, automatically run.
> > > > > > > diff --git a/include/configs/rpi.h b/include/configs/rpi.h
> > > > > > > index d5e064fb379..ea373d0c221 100644
> > > > > > > --- a/include/configs/rpi.h
> > > > > > > +++ b/include/configs/rpi.h
> > > > > > > @@ -133,47 +133,12 @@
> > > > > > >       "fdt_addr_r=0x02600000\0" \
> > > > > > >       "ramdisk_addr_r=0x02700000\0"
> > > > > > >
> > > > > > > -#if CONFIG_IS_ENABLED(CMD_MMC)
> > > > > > > -     #define BOOT_TARGET_MMC(func) \
> > > > > > > -             func(MMC, mmc, 0) \
> > > > > > > -             func(MMC, mmc, 1) \
> > > > > > > -             func(MMC, mmc, 2)
> > > > > > > -#else
> > > > > > > -     #define BOOT_TARGET_MMC(func)
> > > > > > > -#endif
> > > > > > > -
> > > > > > > -#if CONFIG_IS_ENABLED(CMD_USB)
> > > > > > > -     #define BOOT_TARGET_USB(func) func(USB, usb, 0)
> > > > > > > -#else
> > > > > > > -     #define BOOT_TARGET_USB(func)
> > > > > > > -#endif
> > > > > > > -
> > > > > > > -#if CONFIG_IS_ENABLED(CMD_PXE)
> > > > > > > -     #define BOOT_TARGET_PXE(func) func(PXE, pxe, na)
> > > > > > > -#else
> > > > > > > -     #define BOOT_TARGET_PXE(func)
> > > > > > > -#endif
> > > > > > > -
> > > > > > > -#if CONFIG_IS_ENABLED(CMD_DHCP)
> > > > > > > -     #define BOOT_TARGET_DHCP(func) func(DHCP, dhcp, na)
> > > > > > > -#else
> > > > > > > -     #define BOOT_TARGET_DHCP(func)
> > > > > > > -#endif
> > > > > > > -
> > > > > > > -#define BOOT_TARGET_DEVICES(func) \
> > > > > > > -     BOOT_TARGET_MMC(func) \
> > > > > > > -     BOOT_TARGET_USB(func) \
> > > > > > > -     BOOT_TARGET_PXE(func) \
> > > > > > > -     BOOT_TARGET_DHCP(func)
> > > > > > > -
> > > > > > > -#include <config_distro_bootcmd.h>
> > > > > > > -
> > > > > > >  #define CONFIG_EXTRA_ENV_SETTINGS \
> > > > > > >       "dhcpuboot=usb start; dhcp u-boot.uimg; bootm\0" \
> > > > > > > +     "boot_targets=mmc0 mmc1 usb0 pxe dhcp\0" \
> > > > > >
> > > > > > We have the indirect defines to func(...) everywhere so that if a
> > > > > > feature is disabled we still build + function, as otherwise it's a loud
> > > > > > link error.  I assume with this we just get a try and fail move to the
> > > > > > next target at run time, yes?
> > > > >
> > > > > Yes, that's right. But I hope that boards actually set up the boot
> > > > > targets correctly so that only those that are actually supported are
> > > > > enabled.
> > > >
> > > > What do you mean?  They always do that, even today, which is why we have
> > > > those #ifdef's in the distro boot board code.  If the user ends up
> > > > disabling something, it should still build and work.  And honestly,
> > > > given that dependencies are now in Kconfig, the distro_boot.h stuff
> > > > could get away with dropping all of the
> > > > "Enabled_foo_without_CONFIG_CMD_foo" stuff I bet.
> > >
> > > Then I don't think I understand your original question. With bootstd
> > > if USB is disabled (for example) then the USB bootdev won't exist so
> > > it won't look at USB. It is not so much a link error, just that the
> > > device is not there.
> >
> > I mean, with "boot_targets=mmc0 mmc1 usb0 pxe dhcp0" but CONFIG_MMC
> > disabled, bootstd will gracefully fail, gracefully fail, boot from USB.
>
> Even if it gracefully fails, wouldn't it be better if the boot_targets
> environment variable would correctly reflect the devices that are
> actually avialable to boot from.  We already have the logic to
> construct that list.  So why are we throwing that logic away?

Well, the availability of bootdevs provides the same information. You
can see that by looking at the definition of BOOT_TARGET_DEVICES
above.

The bootdevs have a natural priority, based on the assumed speed of
the device, so the board would only need to intervene (with an env var
or a devicetree property) when that is wrong.

Regards,
Simon