On Wed, Jan 25, 2023 at 06:04:56PM +0100, Heinrich Schuchardt wrote:
> On 1/25/23 17:15, Tom Rini wrote:
> > On Wed, Jan 25, 2023 at 04:38:53PM +0100, Heinrich Schuchardt wrote:
> > > On 1/25/23 04:11, Simon Glass wrote:
> > > > For EFI, the distro boot scripts search in three different directories
> > > > for the .dtb file. The SOC-based filename fallback is supported only for
> > > > 32-bit ARM.
> > > >
> > > > Adjust the code to mirror this behaviour.
> > > >
> > > > Signed-off-by: Simon Glass <s...@chromium.org>
> > > > Suggested-by: Mark Kettenis <kette...@openbsd.org>
> > > > ---
> > > >
> > > > boot/bootmeth_efi.c | 63
> > > > ++++++++++++++++++++++++++++++++++++++-------
> > > > 1 file changed, 54 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> > > > index 67c972e3fe4..f9544846e68 100644
> > > > --- a/boot/bootmeth_efi.c
> > > > +++ b/boot/bootmeth_efi.c
> > > > @@ -147,25 +147,57 @@ static int distro_efi_check(struct udevice *dev,
> > > > struct bootflow_iter *iter)
> > > > return 0;
> > > > }
> > > > -static void distro_efi_get_fdt_name(char *fname, int size)
> > > > +/**
> > > > + * distro_efi_get_fdt_name() - Get the filename for reading the .dtb
> > > > file
> > > > + *
> > > > + * @fname: Place to put filename
> > > > + * @size: Max size of filename
> > > > + * @seq: Sequence number, to cycle through options (0=first)
> > > > + * Returns: 0 on success, -ENOENT if the "fdtfile" env var does not
> > > > exist,
> > > > + * -EINVAL if there are no more options
> > > > + */
> > > > +static int distro_efi_get_fdt_name(char *fname, int size, int seq)
> > > > {
> > > > const char *fdt_fname;
> > > > + const char *prefix;
> > > > +
> > > > + /* select the prefix */
> > > > + switch (seq) {
> > > > + case 0:
> > > > + /* this is the default */
> > > > + prefix = "/dtb";
> > > > + break;
> > > > + case 1:
> > > > + prefix = "";
> > > > + break;
> > > > + case 2:
> > > > + prefix = "/dtb/current";
> > > > + break;
> > > > + default:
> > > > + return log_msg_ret("pref", -EINVAL);
> > > > + }
> > > > fdt_fname = env_get("fdtfile");
> > > > if (fdt_fname) {
> > > > - snprintf(fname, size, "dtb/%s", fdt_fname);
> > > > + snprintf(fname, size, "%s/%s", prefix, fdt_fname);
> > > > log_debug("Using device tree: %s\n", fname);
> > > > - } else {
> > > > +
> > > > + /* Use this fallback only for 32-bit ARM */
> > > > + } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64))
> > > > {
> > > > const char *soc = env_get("soc");
> > > > const char *board = env_get("board");
> > > > const char *boardver = env_get("boardver");
> > > > /* cf the code in label_boot() which seems very complex
> > > > */
> > > > - snprintf(fname, size, "dtb/%s%s%s%s.dtb",
> > > > + snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix,
> > > > soc ? soc : "", soc ? "-" : "", board ? board
> > > > : "",
> > > > boardver ? boardver : "");
> > > > log_debug("Using default device tree: %s\n", fname);
> > > > + } else {
> > > > + return log_msg_ret("env", -ENOENT);
> > > > }
> > > > +
> > > > + return 0;
> > > > }
> > > > static int distro_efi_read_bootflow_file(struct udevice *dev,
> > > > @@ -174,7 +206,7 @@ static int distro_efi_read_bootflow_file(struct
> > > > udevice *dev,
> > > > struct blk_desc *desc = NULL;
> > > > ulong fdt_addr, size;
> > > > char fname[256];
> > > > - int ret;
> > > > + int ret, seq;
> > > > /* We require a partition table */
> > > > if (!bflow->part)
> > > > @@ -196,13 +228,22 @@ static int distro_efi_read_bootflow_file(struct
> > > > udevice *dev,
> > > > if (ret)
> > > > return log_msg_ret("read", -EINVAL);
> > > > - distro_efi_get_fdt_name(fname, sizeof(fname));
> > > > + fdt_addr = env_get_hex("fdt_addr_r", 0);
> > > > +
> > > > + /* try the various available names */
> > > > + ret = -ENOENT;
> > > > + for (seq = 0; ret; seq++)
> > >
> > > There are boards that don't have a dtb file available and that is ok.
> > > Don't
> > > loop past seq = 2.
> > >
> > > {
> > > > + ret = distro_efi_get_fdt_name(fname, sizeof(fname),
> > > > seq);
> > > > + if (ret)
> > > > + return log_msg_ret("nam", ret);
> > > > + ret = bootmeth_common_read_file(dev, bflow, fname,
> > > > fdt_addr,
> > > > + &size);
> > > > + }
> > > > +
> > > > bflow->fdt_fname = strdup(fname);
> > > > if (!bflow->fdt_fname)
> > > > return log_msg_ret("fil", -ENOMEM);
> > >
> > > This should not be an error. The Distroboot fallback is the device-tree at
> > > $fdtcontroladdr.
> >
> > But it should note that it's using that because that's still quite often
> > an unexpected feature to people.
> >
>
> On QEMU it is just what is needed. Other boards supply the device-tree via
> TF-A or OpenSBI. We should not start breaking boards.
>
> A message "fil: returning err= -12" signals not caring about users.Yes, I'm saying we need a positive message when using the in memory device tree. -- Tom
signature.asc
Description: PGP signature
