diff mbox series

[v2,2/2] bootstd: Replicate the dtb-filename quirks of distroboot

Message ID 20230129012715.83657-2-sjg@chromium.org
State Changes Requested
Delegated to: Tom Rini
Headers show
Series [v2,1/2] bootflow: Rename bootflow_flags_t | expand

Commit Message

Simon Glass Jan. 29, 2023, 1:27 a.m. UTC 
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.

Also some boards can use a prior-stage FDT if one is not found in the
normal way. Support this and show a warning in that case.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Mark Kettenis <kettenis@openbsd.org>
---

Changes in v2:
- Allow use of the prior-stage FDT if nothing else is found
- Warn about using a prior-stage FDT

 boot/bootflow.c     |  3 ++
 boot/bootmeth_efi.c | 70 +++++++++++++++++++++++++++++++++++++++------
 include/bootflow.h  | 14 +++++++++
 3 files changed, 78 insertions(+), 9 deletions(-)

Comments

Tom Rini Jan. 30, 2023, 5:10 p.m. UTC | #1 
On Sat, Jan 28, 2023 at 06:27:15PM -0700, 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.
> 
> Also some boards can use a prior-stage FDT if one is not found in the
> normal way. Support this and show a warning in that case.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Mark Kettenis <kettenis@openbsd.org>
> ---
> 
> Changes in v2:
> - Allow use of the prior-stage FDT if nothing else is found
> - Warn about using a prior-stage FDT
> 
>  boot/bootflow.c     |  3 ++
>  boot/bootmeth_efi.c | 70 +++++++++++++++++++++++++++++++++++++++------
>  include/bootflow.h  | 14 +++++++++
>  3 files changed, 78 insertions(+), 9 deletions(-)
> 
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 4999018e36e..5ee12eb2bab 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -463,6 +463,9 @@ int bootflow_run_boot(struct bootflow_iter *iter, struct bootflow *bflow)
>  
>  	printf("** Booting bootflow '%s' with %s\n", bflow->name,
>  	       bflow->method->name);
> +	if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE) &&
> +	    (bflow->flags & BOOTFLOWF_USE_PRIOR_FDT))
> +		printf("** Warning: Using prior-stage device tree\n");

It should not be a warning. It should just be a statement. It's not an
inherent failure or problem, but it is something the user should be
aware of as it may be unexpected. Or simply because they should be just
as aware here as when it's loaded from $file at $location. A huge
general stumbling block when working on custom / new hardware is "did
the device tree I want really get used?" so being clear where the one
being used is from is important.
Simon Glass Feb. 7, 2023, 4:02 a.m. UTC | #2 
On Mon, 30 Jan 2023 at 10:10, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Jan 28, 2023 at 06:27:15PM -0700, 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.
> >
> > Also some boards can use a prior-stage FDT if one is not found in the
> > normal way. Support this and show a warning in that case.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Suggested-by: Mark Kettenis <kettenis@openbsd.org>
> > ---
> >
> > Changes in v2:
> > - Allow use of the prior-stage FDT if nothing else is found
> > - Warn about using a prior-stage FDT
> >
> >  boot/bootflow.c     |  3 ++
> >  boot/bootmeth_efi.c | 70 +++++++++++++++++++++++++++++++++++++++------
> >  include/bootflow.h  | 14 +++++++++
> >  3 files changed, 78 insertions(+), 9 deletions(-)
> >
> > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > index 4999018e36e..5ee12eb2bab 100644
> > --- a/boot/bootflow.c
> > +++ b/boot/bootflow.c
> > @@ -463,6 +463,9 @@ int bootflow_run_boot(struct bootflow_iter *iter, struct bootflow *bflow)
> >
> >       printf("** Booting bootflow '%s' with %s\n", bflow->name,
> >              bflow->method->name);
> > +     if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE) &&
> > +         (bflow->flags & BOOTFLOWF_USE_PRIOR_FDT))
> > +             printf("** Warning: Using prior-stage device tree\n");
>
> It should not be a warning. It should just be a statement. It's not an
> inherent failure or problem, but it is something the user should be
> aware of as it may be unexpected. Or simply because they should be just
> as aware here as when it's loaded from $file at $location. A huge
> general stumbling block when working on custom / new hardware is "did
> the device tree I want really get used?" so being clear where the one
> being used is from is important.

Thanks Tom.


- Simon
diff mbox series

Patch

diff --git a/boot/bootflow.c b/boot/bootflow.c
index 4999018e36e..5ee12eb2bab 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -463,6 +463,9 @@  int bootflow_run_boot(struct bootflow_iter *iter, struct bootflow *bflow)
 
 	printf("** Booting bootflow '%s' with %s\n", bflow->name,
 	       bflow->method->name);
+	if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE) &&
+	    (bflow->flags & BOOTFLOWF_USE_PRIOR_FDT))
+		printf("** Warning: Using prior-stage device tree\n");
 	ret = bootflow_boot(bflow);
 	if (!IS_ENABLED(CONFIG_BOOTSTD_FULL)) {
 		printf("Boot failed (err=%d)\n", ret);
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index 67c972e3fe4..6a97ac02ff5 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -147,25 +147,60 @@  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, -EALREADY if the control FDT should be
+ * used
+ */
+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 {
+	} else if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE)) {
+		strcpy(fname, "<prior>");
+		return log_msg_ret("pref", -EALREADY);
+	/* 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 +209,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 +231,26 @@  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++) {
+		ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq);
+		if (ret == -EALREADY) {
+			bflow->flags = BOOTFLOWF_USE_PRIOR_FDT;
+			break;
+		}
+		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);
 
-	fdt_addr = env_get_hex("fdt_addr_r", 0);
-	ret = bootmeth_common_read_file(dev, bflow, fname, fdt_addr, &size);
 	if (!ret) {
 		bflow->fdt_size = size;
 		bflow->fdt_addr = fdt_addr;
@@ -277,7 +325,11 @@  static int distro_efi_read_bootflow_net(struct bootflow *bflow)
 	fdt_addr = hextoul(fdt_addr_str, NULL);
 	sprintf(file_addr, "%lx", fdt_addr);
 
-	distro_efi_get_fdt_name(fname, sizeof(fname));
+	/* We only allow the first prefix with PXE */
+	ret = distro_efi_get_fdt_name(fname, sizeof(fname), 0);
+	if (ret)
+		return log_msg_ret("nam", ret);
+
 	bflow->fdt_fname = strdup(fname);
 	if (!bflow->fdt_fname)
 		return log_msg_ret("fil", -ENOMEM);
diff --git a/include/bootflow.h b/include/bootflow.h
index e5fdf5f29d1..f20f575030f 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -36,6 +36,18 @@  enum bootflow_state_t {
 	BOOTFLOWST_COUNT
 };
 
+/**
+ * enum bootflow_flags_t - flags for bootflows
+ *
+ * @BOOTFLOWF_USE_PRIOR_FDT: Indicates that an FDT was not found by the bootmeth
+ *	and it is using the prior-stage FDT, which is the U-Boot control FDT.
+ *	This is only possible with the EFI bootmeth (distro-efi) and only when
+ *	CONFIG_OF_HAS_PRIOR_STAGE is enabled
+ */
+enum bootflow_flags_t {
+	BOOTFLOWF_USE_PRIOR_FDT	= 1 << 0,
+};
+
 /**
  * struct bootflow - information about a bootflow
  *
@@ -68,6 +80,7 @@  enum bootflow_state_t {
  * @fdt_fname: Filename of FDT file
  * @fdt_size: Size of FDT file
  * @fdt_addr: Address of loaded fdt
+ * @flags: Flags for the bootflow (see enum bootflow_flags_t)
  */
 struct bootflow {
 	struct list_head bm_node;
@@ -90,6 +103,7 @@  struct bootflow {
 	char *fdt_fname;
 	int fdt_size;
 	ulong fdt_addr;
+	int flags;
 };
 
 /**