[v4 01/24] buildman: Use oldconfig when adjusting the config

[v4 01/24] buildman: Use oldconfig when adjusting the config

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

We cannot be sure that the new config is consistent, particularly when
changing a major item like CONFIG_CMDLINE. Use 'make oldconfig' to
check that and avoid any such problems.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/buildman/builder.py       | 2 +-
 tools/buildman/builderthread.py | 6 ++++++
 tools/buildman/func_test.py     | 4 +++-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index 5305477c5be6..782e59dd5cca 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -476,7 +476,7 @@ class Builder:
         Args:
             commit: Commit object that is being built
             brd: Board object that is being built
-            stage: Stage that we are at (mrproper, config, build)
+            stage: Stage that we are at (mrproper, config, oldconfig, build)
             cwd: Directory where make should be run
             args: Arguments to pass to make
             kwargs: Arguments to pass to command.run_pipe()
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 6a61f64da1d4..a8599c0bb2a8 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -426,6 +426,12 @@ class BuilderThread(threading.Thread):
 
         # Now do the build, if everything looks OK
         if result.return_code == 0:
+            if adjust_cfg:
+                oldc_args = list(args) + ['oldconfig']
+                oldc_result = self.make(commit, brd, 'oldconfig', cwd,
+                                        *oldc_args, env=env)
+                if oldc_result.return_code:
+                    return oldc_result
             result = self._build(commit, brd, cwd, args, env, cmd_list,
                                  config_only)
             if adjust_cfg:
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 55dd494fe8ee..6b88ed815d65 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -439,6 +439,8 @@ class TestFunctional(unittest.TestCase):
             tools.write_file(fname, b'CONFIG_SOMETHING=1')
             return command.CommandResult(return_code=0,
                     combined='Test configuration complete')
+        elif stage == 'oldconfig':
+            return command.CommandResult(return_code=0)
         elif stage == 'build':
             stderr = ''
             fname = os.path.join(cwd or '', out_dir, 'u-boot')
@@ -461,7 +463,7 @@ Some images are invalid'''
             return command.CommandResult(return_code=0)
 
         # Not handled, so abort
-        print('make', stage)
+        print('_HandleMake failure: make', stage)
         sys.exit(1)
 
     # Example function to print output lines
-- 
2.34.1

[v4 02/24] virtio: Make VIRTIO_NET depend on NETDEVICES

[Tom Rini]

As VIRTIO_NET is the symbol for enabling network devices, make this
depend on NETDEVICES

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/virtio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 852f6735b602..1de68867d52e 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -56,7 +56,7 @@ config VIRTIO_SANDBOX
 
 config VIRTIO_NET
 	bool "virtio net driver"
-	depends on VIRTIO
+	depends on VIRTIO && NETDEVICES
 	help
 	  This is the virtual net driver for virtio. It can be used with
 	  QEMU based targets.
-- 
2.34.1

[v4 03/24] dfu: Make DFU_TFTP depend on NETDEVICES

[Tom Rini]

In order to do a DFU update over TFTP we need to have some network
device available, so make this depend on NETDEVICES

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 drivers/dfu/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
index 4e80e85d10d7..8771678ca5a0 100644
--- a/drivers/dfu/Kconfig
+++ b/drivers/dfu/Kconfig
@@ -19,6 +19,7 @@ config DFU_WRITE_ALT
 
 config DFU_TFTP
 	bool "DFU via TFTP"
+	depends on NETDEVICES
 	select UPDATE_COMMON
 	select DFU_OVER_TFTP
 	help
-- 
2.34.1

[v4 04/24] version: Separate our version string from the version command

[Tom Rini]

In order to be able to disable all commands we need to construct our
version string in a common file, and have the version command reference
that string, like the other users of it do.  Create common/version.c
with just the strings.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 cmd/version.c    |  9 ---------
 common/Makefile  |  1 +
 common/version.c | 16 ++++++++++++++++
 3 files changed, 17 insertions(+), 9 deletions(-)
 create mode 100644 common/version.c

diff --git a/cmd/version.c b/cmd/version.c
index 87e1fa4159c1..d99a44f19fb3 100644
--- a/cmd/version.c
+++ b/cmd/version.c
@@ -7,21 +7,12 @@
 #include <common.h>
 #include <command.h>
 #include <display_options.h>
-#include <timestamp.h>
-#include <version.h>
 #include <version_string.h>
 #include <linux/compiler.h>
 #ifdef CONFIG_SYS_COREBOOT
 #include <asm/cb_sysinfo.h>
 #endif
 
-#define U_BOOT_VERSION_STRING U_BOOT_VERSION " (" U_BOOT_DATE " - " \
-	U_BOOT_TIME " " U_BOOT_TZ ")" CONFIG_IDENT_STRING
-
-const char version_string[] = U_BOOT_VERSION_STRING;
-const unsigned short version_num = U_BOOT_VERSION_NUM;
-const unsigned char version_num_patch = U_BOOT_VERSION_NUM_PATCH;
-
 static int do_version(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[])
 {
diff --git a/common/Makefile b/common/Makefile
index cdeadf72026c..b711dc29b65e 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -10,6 +10,7 @@ obj-y += main.o
 obj-y += exports.o
 obj-$(CONFIG_HUSH_PARSER) += cli_hush.o
 obj-$(CONFIG_AUTOBOOT) += autoboot.o
+obj-y += version.o
 
 # # boards
 obj-y += board_f.o
diff --git a/common/version.c b/common/version.c
new file mode 100644
index 000000000000..6e27bb80e398
--- /dev/null
+++ b/common/version.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2000-2009
+ * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
+ */
+
+#include <timestamp.h>
+#include <version.h>
+#include <version_string.h>
+
+#define U_BOOT_VERSION_STRING U_BOOT_VERSION " (" U_BOOT_DATE " - " \
+	U_BOOT_TIME " " U_BOOT_TZ ")" CONFIG_IDENT_STRING
+
+const char version_string[] = U_BOOT_VERSION_STRING;
+const unsigned short version_num = U_BOOT_VERSION_NUM;
+const unsigned char version_num_patch = U_BOOT_VERSION_NUM_PATCH;
-- 
2.34.1

[v4 05/24] qemu: Correct CMD_QFW dependencies in Kconfig

[Tom Rini]

Rather than selecting CMD_QFW, we should make the option itself by
enabled by default on these platforms.  Then in the board-specific
Kconfig we should select the appropriate back-end as needed if the
command is enabled.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
Cc: Bin Meng <bmeng.cn@gmail.com>
---
 board/emulation/qemu-arm/Kconfig | 3 +--
 board/emulation/qemu-x86/Kconfig | 2 +-
 cmd/Kconfig                      | 2 ++
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/board/emulation/qemu-arm/Kconfig b/board/emulation/qemu-arm/Kconfig
index 09c95413a541..ac2d078f42a1 100644
--- a/board/emulation/qemu-arm/Kconfig
+++ b/board/emulation/qemu-arm/Kconfig
@@ -5,8 +5,7 @@ config TEXT_BASE
 
 config BOARD_SPECIFIC_OPTIONS # dummy
 	def_bool y
-	select CMD_QFW
-	select QFW_MMIO
+	select QFW_MMIO if CMD_QFW
 	imply VIRTIO_MMIO
 	imply VIRTIO_PCI
 	imply VIRTIO_NET
diff --git a/board/emulation/qemu-x86/Kconfig b/board/emulation/qemu-x86/Kconfig
index 787751abba4f..01dc1d497aec 100644
--- a/board/emulation/qemu-x86/Kconfig
+++ b/board/emulation/qemu-x86/Kconfig
@@ -20,7 +20,7 @@ config BOARD_SPECIFIC_OPTIONS # dummy
 	def_bool y
 	select X86_RESET_VECTOR
 	select QEMU
-	select QFW_PIO
+	select QFW_PIO if CMD_QFW
 	select BOARD_ROMSIZE_KB_1024
 	imply VIRTIO_PCI
 	imply VIRTIO_NET
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5bc0a92d57ad..5554bd14e2ef 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2239,6 +2239,8 @@ config CMD_SYSBOOT
 config CMD_QFW
 	bool "qfw"
 	select QFW
+	default y if TARGET_QEMU_ARM_32BIT || TARGET_QEMU_ARM_64BIT || \
+		TARGET_QEMU_X86 || TARGET_QEMU_X86_64
 	help
 	  This provides access to the QEMU firmware interface.  The main
 	  feature is to allow easy loading of files passed to qemu-system
-- 
2.34.1

[v4 06/24] Kconfig: Move CONFIG_SYS_[CP]BSIZE to common/Kconfig

[Tom Rini]

Move CONFIG_SYS_CBSIZE (console buffer size) and CONFIG_SYS_PBSIZE
(console print buffer size) out of cmd/Kconfig and in to common/Kconfig.
Create help entries for both which explain their usage and why they are
both not entirely command centric.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 cmd/Kconfig    | 14 --------------
 common/Kconfig | 23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5554bd14e2ef..ecf25b062ad6 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -75,20 +75,6 @@ config SYS_MAXARGS
 	int "Maximum number arguments accepted by commands"
 	default 16
 
-config SYS_CBSIZE
-	int "Console input buffer size"
-	default 2048 if ARCH_TEGRA || ARCH_VERSAL || ARCH_ZYNQ || ARCH_ZYNQMP || \
-		RCAR_GEN3 || TARGET_SOCFPGA_SOC64
-	default 512 if ARCH_MX5 || ARCH_MX6 || ARCH_MX7 || FSL_LSCH2 || \
-		FSL_LSCH3 || X86
-	default 256 if M68K || PPC
-	default 1024
-
-config SYS_PBSIZE
-	int "Buffer size for console output"
-	default 1024 if ARCH_SUNXI
-	default 1044
-
 config SYS_XTRACE
 	bool "Command execution tracer"
 	depends on CMDLINE
diff --git a/common/Kconfig b/common/Kconfig
index 93c96f23b013..fba1548e135b 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -52,6 +52,29 @@ config CONSOLE_RECORD_IN_SIZE
 	  The buffer is allocated immediately after the malloc() region is
 	  ready.
 
+config SYS_CBSIZE
+	int "Console input buffer size"
+	default 2048 if ARCH_TEGRA || ARCH_VERSAL || ARCH_ZYNQ || ARCH_ZYNQMP || \
+		RCAR_GEN3 || TARGET_SOCFPGA_SOC64
+	default 512 if ARCH_MX5 || ARCH_MX6 || ARCH_MX7 || FSL_LSCH2 || \
+		FSL_LSCH3 || X86
+	default 256 if M68K || PPC
+	default 1024
+	help
+	  Set the size of the console input buffer. This is used both in the
+	  case of reading input literally from the user in some manner as well
+	  as when we need to construct or modify that type of input, for
+	  example when constructing "bootargs" for the OS.
+
+config SYS_PBSIZE
+	int "Console output buffer size"
+	default 1024 if ARCH_SUNXI
+	default 1044
+	help
+	  Set the size of the console output buffer. This is used when we need
+	  to work with some form of a buffer while providing output in some
+	  form to the user.
+
 config DISABLE_CONSOLE
 	bool "Add functionality to disable console completely"
 	help
-- 
2.34.1

[v4 07/24] env: Move env_set() out of cmd/nvedit.c and in to env/common.c

[Tom Rini]

Inside of env/common.c we already have our helper env_set_xxx functions,
and even have a comment that explains why env_set() itself wasn't moved.
We now handle that move. This requires that we rename the previous
_do_env_set() to env_do_env_set() and note it as an internal env
function. Add comments about this function to explain why we do this
when we add the prototype. Add a new function, env_inc_id() to allow for
the counter to be updated by both commands and callers, and document
this as well by the prototype.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 cmd/Makefile           |   4 +-
 cmd/nvedit.c           | 122 ++---------------------------------------
 env/common.c           | 113 ++++++++++++++++++++++++++++++++++++--
 include/env.h          |   8 +++
 include/env_internal.h |  12 ++++
 5 files changed, 135 insertions(+), 124 deletions(-)

diff --git a/cmd/Makefile b/cmd/Makefile
index 44db5f22861e..27a0045e7f94 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -128,6 +128,7 @@ endif
 obj-$(CONFIG_CMD_MUX) += mux.o
 obj-$(CONFIG_CMD_NAND) += nand.o
 obj-$(CONFIG_CMD_NET) += net.o
+obj-$(CONFIG_ENV_SUPPORT) += nvedit.o
 obj-$(CONFIG_CMD_NVEDIT_EFI) += nvedit_efi.o
 obj-$(CONFIG_CMD_ONENAND) += onenand.o
 obj-$(CONFIG_CMD_OSD) += osd.o
@@ -244,9 +245,6 @@ endif # !CONFIG_SPL_BUILD
 
 obj-$(CONFIG_$(SPL_)CMD_TLV_EEPROM) += tlv_eeprom.o
 
-# core command
-obj-y += nvedit.o
-
 obj-$(CONFIG_CMD_BCM_EXT_UTILS) += broadcom/
 
 filechk_data_gz = (echo "static const char data_gz[] ="; cat $< | scripts/bin2c; echo ";")
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index daf1ad37f9be..e77338f81394 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -49,20 +49,6 @@ DECLARE_GLOBAL_DATA_PTR;
  */
 #define	MAX_ENV_SIZE	(1 << 20)	/* 1 MiB */
 
-/*
- * This variable is incremented on each do_env_set(), so it can
- * be used via env_get_id() as an indication, if the environment
- * has changed or not. So it is possible to reread an environment
- * variable only if the environment was changed ... done so for
- * example in NetInitLoop()
- */
-static int env_id = 1;
-
-int env_get_id(void)
-{
-	return env_id;
-}
-
 #ifndef CONFIG_SPL_BUILD
 /*
  * Command interface: print one or all environment variables
@@ -198,104 +184,6 @@ DONE:
 #endif
 #endif /* CONFIG_SPL_BUILD */
 
-/*
- * Set a new environment variable,
- * or replace or delete an existing one.
- */
-static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
-{
-	int   i, len;
-	char  *name, *value, *s;
-	struct env_entry e, *ep;
-
-	debug("Initial value for argc=%d\n", argc);
-
-#if !IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_CMD_NVEDIT_EFI)
-	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
-		return do_env_set_efi(NULL, flag, --argc, ++argv);
-#endif
-
-	while (argc > 1 && **(argv + 1) == '-') {
-		char *arg = *++argv;
-
-		--argc;
-		while (*++arg) {
-			switch (*arg) {
-			case 'f':		/* force */
-				env_flag |= H_FORCE;
-				break;
-			default:
-				return CMD_RET_USAGE;
-			}
-		}
-	}
-	debug("Final value for argc=%d\n", argc);
-	name = argv[1];
-
-	if (strchr(name, '=')) {
-		printf("## Error: illegal character '='"
-		       "in variable name \"%s\"\n", name);
-		return 1;
-	}
-
-	env_id++;
-
-	/* Delete only ? */
-	if (argc < 3 || argv[2] == NULL) {
-		int rc = hdelete_r(name, &env_htab, env_flag);
-
-		/* If the variable didn't exist, don't report an error */
-		return rc && rc != -ENOENT ? 1 : 0;
-	}
-
-	/*
-	 * Insert / replace new value
-	 */
-	for (i = 2, len = 0; i < argc; ++i)
-		len += strlen(argv[i]) + 1;
-
-	value = malloc(len);
-	if (value == NULL) {
-		printf("## Can't malloc %d bytes\n", len);
-		return 1;
-	}
-	for (i = 2, s = value; i < argc; ++i) {
-		char *v = argv[i];
-
-		while ((*s++ = *v++) != '\0')
-			;
-		*(s - 1) = ' ';
-	}
-	if (s != value)
-		*--s = '\0';
-
-	e.key	= name;
-	e.data	= value;
-	hsearch_r(e, ENV_ENTER, &ep, &env_htab, env_flag);
-	free(value);
-	if (!ep) {
-		printf("## Error inserting \"%s\" variable, errno=%d\n",
-			name, errno);
-		return 1;
-	}
-
-	return 0;
-}
-
-int env_set(const char *varname, const char *varvalue)
-{
-	const char * const argv[4] = { "setenv", varname, varvalue, NULL };
-
-	/* before import into hashtable */
-	if (!(gd->flags & GD_FLG_ENV_READY))
-		return 1;
-
-	if (varvalue == NULL || varvalue[0] == '\0')
-		return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC);
-	else
-		return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC);
-}
-
 #ifndef CONFIG_SPL_BUILD
 static int do_env_set(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[])
@@ -303,7 +191,7 @@ static int do_env_set(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	return _do_env_set(flag, argc, argv, H_INTERACTIVE);
+	return env_do_env_set(flag, argc, argv, H_INTERACTIVE);
 }
 
 /*
@@ -381,7 +269,7 @@ int do_env_ask(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 
 	/* Continue calling setenv code */
-	return _do_env_set(flag, len, local_args, H_INTERACTIVE);
+	return env_do_env_set(flag, len, local_args, H_INTERACTIVE);
 }
 #endif
 
@@ -561,12 +449,12 @@ static int do_env_edit(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (buffer[0] == '\0') {
 		const char * const _argv[3] = { "setenv", argv[1], NULL };
 
-		return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE);
+		return env_do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE);
 	} else {
 		const char * const _argv[4] = { "setenv", argv[1], buffer,
 			NULL };
 
-		return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE);
+		return env_do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE);
 	}
 }
 #endif /* CONFIG_CMD_EDITENV */
@@ -679,7 +567,7 @@ static int do_env_delete(struct cmd_tbl *cmdtp, int flag,
 	}
 	debug("Final value for argc=%d\n", argc);
 
-	env_id++;
+	env_inc_id();
 
 	while (--argc > 0) {
 		char *name = *++argv;
diff --git a/env/common.c b/env/common.c
index eb1a91379539..656748c1f5b7 100644
--- a/env/common.c
+++ b/env/common.c
@@ -37,11 +37,116 @@ struct hsearch_data env_htab = {
 };
 
 /*
- * This env_set() function is defined in cmd/nvedit.c, since it calls
- * _do_env_set(), whis is a static function in that file.
- *
- * int env_set(const char *varname, const char *varvalue);
+ * This variable is incremented each time we set an environment variable so we
+ * can be check via env_get_id() to see if the environment has changed or not.
+ * This makes it possible to reread an environment variable only if the
+ * environment was changed, typically used by networking code.
  */
+static int env_id = 1;
+
+int env_get_id(void)
+{
+	return env_id;
+}
+
+void env_inc_id(void)
+{
+	env_id++;
+}
+
+int env_do_env_set(int flag, int argc, char *const argv[], int env_flag)
+{
+	int   i, len;
+	char  *name, *value, *s;
+	struct env_entry e, *ep;
+
+	debug("Initial value for argc=%d\n", argc);
+
+#if !IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_CMD_NVEDIT_EFI)
+	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
+		return do_env_set_efi(NULL, flag, --argc, ++argv);
+#endif
+
+	while (argc > 1 && **(argv + 1) == '-') {
+		char *arg = *++argv;
+
+		--argc;
+		while (*++arg) {
+			switch (*arg) {
+			case 'f':		/* force */
+				env_flag |= H_FORCE;
+				break;
+			default:
+				return CMD_RET_USAGE;
+			}
+		}
+	}
+	debug("Final value for argc=%d\n", argc);
+	name = argv[1];
+
+	if (strchr(name, '=')) {
+		printf("## Error: illegal character '='"
+		       "in variable name \"%s\"\n", name);
+		return 1;
+	}
+
+	env_inc_id();
+
+	/* Delete only ? */
+	if (argc < 3 || argv[2] == NULL) {
+		int rc = hdelete_r(name, &env_htab, env_flag);
+
+		/* If the variable didn't exist, don't report an error */
+		return rc && rc != -ENOENT ? 1 : 0;
+	}
+
+	/*
+	 * Insert / replace new value
+	 */
+	for (i = 2, len = 0; i < argc; ++i)
+		len += strlen(argv[i]) + 1;
+
+	value = malloc(len);
+	if (value == NULL) {
+		printf("## Can't malloc %d bytes\n", len);
+		return 1;
+	}
+	for (i = 2, s = value; i < argc; ++i) {
+		char *v = argv[i];
+
+		while ((*s++ = *v++) != '\0')
+			;
+		*(s - 1) = ' ';
+	}
+	if (s != value)
+		*--s = '\0';
+
+	e.key	= name;
+	e.data	= value;
+	hsearch_r(e, ENV_ENTER, &ep, &env_htab, env_flag);
+	free(value);
+	if (!ep) {
+		printf("## Error inserting \"%s\" variable, errno=%d\n",
+			name, errno);
+		return 1;
+	}
+
+	return 0;
+}
+
+int env_set(const char *varname, const char *varvalue)
+{
+	const char * const argv[4] = { "setenv", varname, varvalue, NULL };
+
+	/* before import into hashtable */
+	if (!(gd->flags & GD_FLG_ENV_READY))
+		return 1;
+
+	if (varvalue == NULL || varvalue[0] == '\0')
+		return env_do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC);
+	else
+		return env_do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC);
+}
 
 /**
  * Set an environment variable to an integer value
diff --git a/include/env.h b/include/env.h
index 430c4fa94a42..9778e3e4f2ce 100644
--- a/include/env.h
+++ b/include/env.h
@@ -72,6 +72,14 @@ enum env_redund_flags {
  */
 int env_get_id(void);
 
+/**
+ * env_inc_id() - Increase the sequence number for the environment
+ *
+ * Increment the value that is used by env_get_id() to inform callers
+ * if the environment has changed since they last checked.
+ */
+void env_inc_id(void);
+
 /**
  * env_init() - Set up the pre-relocation environment
  *
diff --git a/include/env_internal.h b/include/env_internal.h
index 6a6949464689..ae7816d38e58 100644
--- a/include/env_internal.h
+++ b/include/env_internal.h
@@ -193,6 +193,18 @@ struct env_driver {
 
 extern struct hsearch_data env_htab;
 
+/**
+ * env_do_env_set() - Perform the actual setting of an environment variable
+ *
+ * Due to the number of places we may need to set an environmental variable
+ * from we have an exposed internal function that performs the real work and
+ * then call this from both the command line function as well as other
+ * locations.
+ *
+ * Return: 0 on success or 1 on failure
+ */
+int env_do_env_set(int flag, int argc, char *const argv[], int env_flag);
+
 /**
  * env_ext4_get_intf() - Provide the interface for env in EXT4
  *
-- 
2.34.1

[v4 08/24] test: Make UNIT_TEST depend on CMDLINE

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

Many tests make some use of the command line, so require it for all test
code.

This could be teased apart, perhaps with a test flag indicating that it
uses the command line. Leave that for later.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
 test/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/Kconfig b/test/Kconfig
index ca648d23376f..c3db727c58e3 100644
--- a/test/Kconfig
+++ b/test/Kconfig
@@ -2,6 +2,7 @@ menu "Testing"
 
 config UNIT_TEST
 	bool "Unit tests"
+	depends on CMDLINE
 	help
 	  Select this to compile in unit tests for various parts of
 	  U-Boot. Test suites will be subcommands of the "ut" command.
-- 
2.34.1

[v4 09/24] video: Don't require the font command

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

While it is nice to have the font command, using 'select' makes it
impossible to build the console code without it. Stop using 'select' and
make it default if CONSOLE_TRUETYPE is enabled when asking the command.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
- Rework to have the command itself be default y if CONSOLE_TRUETYPE
  instead of selecting it. (Tom)
---
 cmd/Kconfig           | 2 +-
 drivers/video/Kconfig | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index ecf25b062ad6..16e5cb8f0633 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2340,7 +2340,7 @@ config CMD_VIDCONSOLE
 config CMD_SELECT_FONT
 	bool "select font size"
 	depends on VIDEO
-	default n
+	default y if CONSOLE_TRUETYPE
 	help
 	  Enabling this will provide 'font' command.
 	  Allows font selection at runtime.
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index ab927641bb7a..6f319ba0d544 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -180,7 +180,6 @@ config CONSOLE_ROTATION
 
 config CONSOLE_TRUETYPE
 	bool "Support a console that uses TrueType fonts"
-	select CMD_SELECT_FONT
 	help
 	  TrueTrype fonts can provide outline-drawing capability rather than
 	  needing to provide a bitmap for each font and size that is needed.
-- 
2.34.1

[v4 10/24] cli_simple: Rework this support slightly

[Tom Rini]

The interactive portion of our non-HUSH 'simple' parser is guarded by
CONFIG_CMDLINE already.  Much of the code behind this simple parser is
also used as "input" parser, such as from menu interfaces and so forth
and not strictly command line input.  To support this, always build the
assorted cli object files, but guard the interactive portions of
cli_simple.c with a CMDLINE check.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 common/Makefile     |  2 +-
 common/cli_simple.c | 77 +++++++++++++++++++++++----------------------
 2 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/common/Makefile b/common/Makefile
index b711dc29b65e..1495436d5d45 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -8,6 +8,7 @@ ifndef CONFIG_SPL_BUILD
 obj-y += init/
 obj-y += main.o
 obj-y += exports.o
+obj-y += cli_getch.o cli_simple.o cli_readline.o
 obj-$(CONFIG_HUSH_PARSER) += cli_hush.o
 obj-$(CONFIG_AUTOBOOT) += autoboot.o
 obj-y += version.o
@@ -38,7 +39,6 @@ obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o
 obj-$(CONFIG_MENU) += menu.o
 obj-$(CONFIG_UPDATE_COMMON) += update.o
 obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
-obj-$(CONFIG_CMDLINE) += cli_getch.o cli_readline.o cli_simple.o
 
 endif # !CONFIG_SPL_BUILD
 
diff --git a/common/cli_simple.c b/common/cli_simple.c
index e80ba488a5eb..f89ba92d1b05 100644
--- a/common/cli_simple.c
+++ b/common/cli_simple.c
@@ -22,44 +22,6 @@
 #define debug_parser(fmt, args...)		\
 	debug_cond(DEBUG_PARSER, fmt, ##args)
 
-
-int cli_simple_parse_line(char *line, char *argv[])
-{
-	int nargs = 0;
-
-	debug_parser("%s: \"%s\"\n", __func__, line);
-	while (nargs < CONFIG_SYS_MAXARGS) {
-		/* skip any white space */
-		while (isblank(*line))
-			++line;
-
-		if (*line == '\0') {	/* end of line, no more args	*/
-			argv[nargs] = NULL;
-			debug_parser("%s: nargs=%d\n", __func__, nargs);
-			return nargs;
-		}
-
-		argv[nargs++] = line;	/* begin of argument string	*/
-
-		/* find end of string */
-		while (*line && !isblank(*line))
-			++line;
-
-		if (*line == '\0') {	/* end of line, no more args	*/
-			argv[nargs] = NULL;
-			debug_parser("parse_line: nargs=%d\n", nargs);
-			return nargs;
-		}
-
-		*line++ = '\0';		/* terminate current arg	 */
-	}
-
-	printf("** Too many args (max. %d) **\n", CONFIG_SYS_MAXARGS);
-
-	debug_parser("%s: nargs=%d\n", __func__, nargs);
-	return nargs;
-}
-
 int cli_simple_process_macros(const char *input, char *output, int max_size)
 {
 	char c, prev;
@@ -172,6 +134,44 @@ int cli_simple_process_macros(const char *input, char *output, int max_size)
 	return ret;
 }
 
+#ifdef CONFIG_CMDLINE
+int cli_simple_parse_line(char *line, char *argv[])
+{
+	int nargs = 0;
+
+	debug_parser("%s: \"%s\"\n", __func__, line);
+	while (nargs < CONFIG_SYS_MAXARGS) {
+		/* skip any white space */
+		while (isblank(*line))
+			++line;
+
+		if (*line == '\0') {	/* end of line, no more args	*/
+			argv[nargs] = NULL;
+			debug_parser("%s: nargs=%d\n", __func__, nargs);
+			return nargs;
+		}
+
+		argv[nargs++] = line;	/* begin of argument string	*/
+
+		/* find end of string */
+		while (*line && !isblank(*line))
+			++line;
+
+		if (*line == '\0') {	/* end of line, no more args	*/
+			argv[nargs] = NULL;
+			debug_parser("parse_line: nargs=%d\n", nargs);
+			return nargs;
+		}
+
+		*line++ = '\0';		/* terminate current arg	 */
+	}
+
+	printf("** Too many args (max. %d) **\n", CONFIG_SYS_MAXARGS);
+
+	debug_parser("%s: nargs=%d\n", __func__, nargs);
+	return nargs;
+}
+
  /*
  * WARNING:
  *
@@ -346,3 +346,4 @@ int cli_simple_run_command_list(char *cmd, int flag)
 
 	return rcode;
 }
+#endif
-- 
2.34.1

[v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

The command should not be used to enable library functionality. Add a
new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
same code is built.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Changes in v4:
- Integrate AKASHI Takahiro's feedback from v3
- Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
---
 cmd/Kconfig             | 11 ++++++++++-
 lib/efi_loader/Kconfig  |  6 +++---
 lib/efi_loader/Makefile |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 16e5cb8f0633..872cb49150cc 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -379,6 +379,15 @@ config CMD_BOOTEFI
 	help
 	  Boot an EFI image from memory.
 
+config CMD_BOOTEFI_BOOTMGR
+	bool "UEFI Boot Manager command"
+	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
+	default y
+	help
+	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
+	  This subcommand will allow you to select the UEFI binary to be booted
+	  via UEFI variables Boot####, BootOrder, and BootNext.
+
 config CMD_BOOTEFI_HELLO_COMPILE
 	bool "Compile a standard EFI hello world binary for testing"
 	depends on CMD_BOOTEFI && !CPU_V7M
@@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
 config CMD_EFICONFIG
 	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
 	default y if !HAS_BOARD_SIZE_LIMIT
-	depends on CMD_BOOTEFI_BOOTMGR
+	depends on BOOTEFI_BOOTMGR
 	select MENU
 	help
 	  Enable the 'eficonfig' command which provides the menu-driven UEFI
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index d20aaab6dba4..13cad6342c36 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -32,14 +32,14 @@ config EFI_LOADER
 
 if EFI_LOADER
 
-config CMD_BOOTEFI_BOOTMGR
+config BOOTEFI_BOOTMGR
 	bool "UEFI Boot Manager"
 	default y
 	select BOOTMETH_GLOBAL if BOOTSTD
 	help
 	  Select this option if you want to select the UEFI binary to be booted
-	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
-	  'bootefi bootmgr' command.
+	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
+	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
 
 choice
 	prompt "Store for non-volatile UEFI variables"
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8d31fc61c601..0a2cb6e3c476 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -42,7 +42,7 @@ targets += initrddump.o
 endif
 
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
-obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
+obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
 obj-y += efi_boottime.o
 obj-y += efi_helper.o
 obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
-- 
2.34.1

[v4 12/24] bootmeth: Make BOOTMETH_EFILOADER depend on CMD_BOOTEFI

[Tom Rini]

Today, the bootmeth for using the EFI loader via bootefi depends on
calling the bootefi command directly, so make this in turn depend on
CMD_BOOTEFI.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 boot/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index a01e6cb8aafe..e0ded3249343 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -504,7 +504,7 @@ config BOOTMETH_EXTLINUX_PXE
 
 config BOOTMETH_EFILOADER
 	bool "Bootdev support for EFI boot"
-	depends on EFI_LOADER
+	depends on CMD_BOOTEFI
 	default y
 	help
 	  Enables support for EFI boot using bootdevs. This makes the
@@ -539,7 +539,7 @@ config BOOTMETH_DISTRO
 	select BOOTMETH_SCRIPT  # E.g. Armbian uses scripts
 	select BOOTMETH_EXTLINUX  # E.g. Debian uses these
 	select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
-	select BOOTMETH_EFILOADER if EFI_LOADER # E.g. Ubuntu uses this
+	select BOOTMETH_EFILOADER if CMD_BOOTEFI # E.g. Ubuntu uses this
 
 config SPL_BOOTMETH_VBE
 	bool "Bootdev support for Verified Boot for Embedded (SPL)"
-- 
2.34.1

[v4 13/24] autoboot: Correct dependencies on CMDLINE

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
 boot/Kconfig | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index e0ded3249343..0dbd10a93469 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -1162,14 +1162,16 @@ menu "Autoboot options"
 
 config AUTOBOOT
 	bool "Autoboot"
+	depends on CMDLINE
 	default y
 	help
 	  This enables the autoboot.  See doc/README.autoboot for detail.
 
+if AUTOBOOT
+
 config BOOTDELAY
 	int "delay in seconds before automatically booting"
 	default 2
-	depends on AUTOBOOT
 	help
 	  Delay before automatically running bootcmd;
 	  set to 0 to autoboot with no delay, but you can stop it by key input.
@@ -1191,9 +1193,11 @@ config AUTOBOOT_KEYED
 	  U-Boot automatic booting process and bring the device
 	  to the U-Boot prompt for user input.
 
+if AUTOBOOT_KEYED
+
 config AUTOBOOT_FLUSH_STDIN
 	bool "Enable flushing stdin before starting to read the password"
-	depends on AUTOBOOT_KEYED && !SANDBOX
+	depends on !SANDBOX
 	help
 	  When this option is enabled stdin buffer will be flushed before
 	  starting to read the password.
@@ -1202,7 +1206,6 @@ config AUTOBOOT_FLUSH_STDIN
 
 config AUTOBOOT_PROMPT
 	string "Autoboot stop prompt"
-	depends on AUTOBOOT_KEYED
 	default "Autoboot in %d seconds\\n"
 	help
 	  This string is displayed before the boot delay selected by
@@ -1218,7 +1221,6 @@ config AUTOBOOT_PROMPT
 
 config AUTOBOOT_ENCRYPTION
 	bool "Enable encryption in autoboot stopping"
-	depends on AUTOBOOT_KEYED
 	help
 	  This option allows a string to be entered into U-Boot to stop the
 	  autoboot.
@@ -1245,7 +1247,7 @@ config AUTOBOOT_SHA256_FALLBACK
 
 config AUTOBOOT_DELAY_STR
 	string "Delay autobooting via specific input key / string"
-	depends on AUTOBOOT_KEYED && !AUTOBOOT_ENCRYPTION
+	depends on !AUTOBOOT_ENCRYPTION
 	help
 	  This option delays the automatic boot feature by issuing
 	  a specific input key or string. If CONFIG_AUTOBOOT_DELAY_STR
@@ -1257,7 +1259,7 @@ config AUTOBOOT_DELAY_STR
 
 config AUTOBOOT_STOP_STR
 	string "Stop autobooting via specific input key / string"
-	depends on AUTOBOOT_KEYED && !AUTOBOOT_ENCRYPTION
+	depends on !AUTOBOOT_ENCRYPTION
 	help
 	  This option enables stopping (aborting) of the automatic
 	  boot feature only by issuing a specific input key or
@@ -1269,7 +1271,7 @@ config AUTOBOOT_STOP_STR
 
 config AUTOBOOT_KEYED_CTRLC
 	bool "Enable Ctrl-C autoboot interruption"
-	depends on AUTOBOOT_KEYED && !AUTOBOOT_ENCRYPTION
+	depends on !AUTOBOOT_ENCRYPTION
 	help
 	  This option allows for the boot sequence to be interrupted
 	  by ctrl-c, in addition to the "bootdelaykey" and "bootstopkey".
@@ -1278,7 +1280,7 @@ config AUTOBOOT_KEYED_CTRLC
 
 config AUTOBOOT_NEVER_TIMEOUT
 	bool "Make the password entry never time-out"
-	depends on AUTOBOOT_KEYED && AUTOBOOT_ENCRYPTION && CRYPT_PW
+	depends on AUTOBOOT_ENCRYPTION && CRYPT_PW
 	help
 	  This option removes the timeout from the password entry
 	  when the user first presses the <Enter> key before entering
@@ -1286,7 +1288,7 @@ config AUTOBOOT_NEVER_TIMEOUT
 
 config AUTOBOOT_STOP_STR_ENABLE
 	bool "Enable fixed string to stop autobooting"
-	depends on AUTOBOOT_KEYED && AUTOBOOT_ENCRYPTION
+	depends on AUTOBOOT_ENCRYPTION
 	help
 	  This option enables the feature to add a fixed stop
 	  string that is defined at compile time.
@@ -1317,9 +1319,12 @@ config AUTOBOOT_STOP_STR_SHA256
 	  includes a ":", the portion prior to the ":" will be treated
 	  as a salt value.
 
+endif  # AUTOBOOT_KEYED
+
+if !AUTOBOOT_KEYED
+
 config AUTOBOOT_USE_MENUKEY
 	bool "Allow a specify key to run a menu from the environment"
-	depends on !AUTOBOOT_KEYED
 	help
 	  If a specific key is pressed to stop autoboot, then the commands in
 	  the environment variable 'menucmd' are executed before boot starts.
@@ -1334,6 +1339,10 @@ config AUTOBOOT_MENUKEY
 	  For example, 33 means "!" in ASCII, so pressing ! at boot would take
 	  this action.
 
+endif
+
+endif  # AUTOBOOT
+
 config AUTOBOOT_MENU_SHOW
 	bool "Show a menu on boot"
 	depends on CMD_BOOTMENU
-- 
2.34.1

[v4 14/24] boot: Make DISTRO_DEFAULTS select CMDLINE

[Tom Rini]

The implementation of DISTRO_DEFAULTS is done in environment scripts and
requires the command line in order to work. Because of this, select
CMDLINE here.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 boot/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/boot/Kconfig b/boot/Kconfig
index 0dbd10a93469..2075ecd34b1f 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -777,6 +777,7 @@ endmenu		# Boot images
 
 config DISTRO_DEFAULTS
 	bool "(deprecated) Script-based booting of Linux distributions"
+	select CMDLINE
 	select BOOT_DEFAULTS
 	select AUTO_COMPLETE
 	select CMDLINE_EDITING
-- 
2.34.1

[v4 15/24] boot: Rework BOOT_DEFAULTS to allow for CMDLINE to be disabled

[Tom Rini]

We split BOOT_DEFAULTS to have BOOT_DEFAULTS_FEATURES and
BOOT_DEFAULTS_CMDS that in turn list general features or commands that
we want enabled when BOOT_DEFAULTS is selected.  We only select
BOOT_DEFAULTS_CMDS if CMDLINE is set.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 boot/Kconfig | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index 2075ecd34b1f..88b9296ee1bf 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -346,8 +346,16 @@ config PXE_UTILS
 	help
 	  Utilities for parsing PXE file formats.
 
-config BOOT_DEFAULTS
-	bool  # Common defaults for standard boot and distroboot
+config BOOT_DEFAULTS_FEATURES
+	bool
+	select SUPPORT_RAW_INITRD
+	select ENV_VARS_UBOOT_CONFIG
+	imply USB_STORAGE
+	imply EFI_PARTITION
+	imply ISO_PARTITION
+
+config BOOT_DEFAULTS_CMDS
+	bool
 	imply USE_BOOTCOMMAND
 	select CMD_ENV_EXISTS
 	select CMD_EXT2
@@ -358,14 +366,14 @@ config BOOT_DEFAULTS
 	select CMD_DHCP if CMD_NET
 	select CMD_PING if CMD_NET
 	select CMD_PXE if CMD_NET
-	select SUPPORT_RAW_INITRD
-	select ENV_VARS_UBOOT_CONFIG
 	select CMD_BOOTI if ARM64
 	select CMD_BOOTZ if ARM && !ARM64
 	imply CMD_MII if NET
-	imply USB_STORAGE
-	imply EFI_PARTITION
-	imply ISO_PARTITION
+
+config BOOT_DEFAULTS
+	bool  # Common defaults for standard boot and distroboot
+	select BOOT_DEFAULTS_FEATURES
+	select BOOT_DEFAULTS_CMDS if CMDLINE
 	help
 	  These are not required but are commonly needed to support a good
 	  selection of booting methods. Enable this to improve the capability
@@ -431,7 +439,6 @@ config BOOTSTD_FULL
 config BOOTSTD_DEFAULTS
 	bool "Select some common defaults for standard boot"
 	depends on BOOTSTD
-	imply USE_BOOTCOMMAND
 	select BOOT_DEFAULTS
 	select BOOTMETH_DISTRO
 	help
-- 
2.34.1

[v4 16/24] boot: Move SYS_BOOTM_LEN to be by LEGACY_IMAGE_FORMAT

[Tom Rini]

This particular option is required for booting all image types,
regardless of if we are starting an OS via command line or something
else.  Move the question for SYS_BOOTM_LEN to be by the question for
LEGACY_IMAGE_FORMAT, as that's where our generic OS questions start.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 boot/Kconfig | 11 +++++++++++
 cmd/Kconfig  | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index 88b9296ee1bf..7c92e0974c5f 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -692,6 +692,17 @@ config LEGACY_IMAGE_FORMAT
 	  loaded. If a board needs the legacy image format support in this
 	  case, enable it here.
 
+config SYS_BOOTM_LEN
+	hex "Maximum size of a decompresed OS image"
+	depends on CMD_BOOTM || CMD_BOOTI || CMD_BOOTZ || \
+		LEGACY_IMAGE_FORMAT || SPL_LEGACY_IMAGE_FORMAT
+	default 0x4000000 if PPC || ARM64
+	default 0x1000000 if X86 || ARCH_MX6 || ARCH_MX7
+	default 0x800000
+	help
+	  This is the maximum size of the buffer that is used to decompress the OS
+	  image in to if attempting to boot a compressed image.
+
 config SUPPORT_RAW_INITRD
 	bool "Enable raw initrd images"
 	help
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 872cb49150cc..099ec444ae99 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -361,17 +361,6 @@ config BOOTM_VXWORKS
 	help
 	  Support booting VxWorks images via the bootm command.
 
-config SYS_BOOTM_LEN
-	hex "Maximum size of a decompresed OS image"
-	depends on CMD_BOOTM || CMD_BOOTI || CMD_BOOTZ || \
-		   LEGACY_IMAGE_FORMAT || SPL_LEGACY_IMAGE_FORMAT
-	default 0x4000000 if PPC || ARM64
-	default 0x1000000 if X86 || ARCH_MX6 || ARCH_MX7
-	default 0x800000
-	help
-	  This is the maximum size of the buffer that is used to decompress the OS
-	  image in to, if passing a compressed image to bootm/booti/bootz.
-
 config CMD_BOOTEFI
 	bool "bootefi"
 	depends on EFI_LOADER
-- 
2.34.1

[v4 17/24] bootmeth_cros: Require bootm.o and bootm_os.o

[Tom Rini]

In order to use bootmeth_cros, at least on non-X86, we need to be able
to start any type of kernel that the "bootm" code paths can handle.  Add
these objects to the required list for this option.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Simon Glass <sjg@chromium.org>
---
 boot/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/Makefile b/boot/Makefile
index ad608598d298..3fd048bb41ab 100644
--- a/boot/Makefile
+++ b/boot/Makefile
@@ -28,7 +28,7 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootstd-uclass.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EXTLINUX) += bootmeth_extlinux.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EXTLINUX_PXE) += bootmeth_pxe.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EFILOADER) += bootmeth_efi.o
-obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_CROS) += bootmeth_cros.o
+obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_CROS) += bootm.o bootm_os.o bootmeth_cros.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) += bootmeth_script.o
 ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL
-- 
2.34.1

[v4 18/24] bootmeth_script: Depend on CMDLINE

[Tom Rini]

As this particular bootmeth requires the command line and assorted
commands to function, make sure we have CMDLINE enabled.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 boot/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index 7c92e0974c5f..40a04f43ee3d 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -543,7 +543,7 @@ config BOOTMETH_VBE
 
 config BOOTMETH_DISTRO
 	bool  # Options needed to boot any distro
-	select BOOTMETH_SCRIPT  # E.g. Armbian uses scripts
+	select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
 	select BOOTMETH_EXTLINUX  # E.g. Debian uses these
 	select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
 	select BOOTMETH_EFILOADER if CMD_BOOTEFI # E.g. Ubuntu uses this
@@ -671,6 +671,7 @@ config BOOTMETH_SANDBOX
 config BOOTMETH_SCRIPT
 	bool "Bootdev support for U-Boot scripts"
 	default y if BOOTSTD_FULL
+	depends on CMDLINE
 	select HUSH_PARSER
 	help
 	  Enables support for booting a distro via a U-Boot script. This makes
-- 
2.34.1

[v4 19/24] boot: Make preboot and bootcmd require CMDLINE

[Tom Rini]

In order for a predefined "preboot" or "bootcmd" to be executed by the
running system we must have a command line.  Add CMDLINE as a
dependency.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 boot/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/boot/Kconfig b/boot/Kconfig
index 40a04f43ee3d..fabf6fec2195 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -1558,6 +1558,7 @@ config BOOTARGS_SUBST
 
 config USE_BOOTCOMMAND
 	bool "Enable a default value for bootcmd"
+	depends on CMDLINE
 	help
 	  Provide a default value for the bootcmd entry in the environment.  If
 	  autoboot is enabled this is what will be run automatically.  Enable
@@ -1577,6 +1578,7 @@ config BOOTCOMMAND
 
 config USE_PREBOOT
 	bool "Enable preboot"
+	depends on CMDLINE
 	help
 	  When this option is enabled, the existence of the environment
 	  variable "preboot" will be checked immediately before starting the
-- 
2.34.1

[v4 20/24] cmd: Make most commands depend on CMDLINE

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

If we disable CMDLINE then we should not ask about enabling the hush
parser nor any of the commands that would be run on the command line as
it is no longer available. Convert the CMDLINE option into a menuconfig
and make every command referenced under cmd/Kconfig depend on it.

This leaves as future work moving the commands that are not under the
cmd/ hierarchy as future work.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
- Reword the commit message slightly.
- Make this not depend on other patches
---
 Makefile    |  2 +-
 cmd/Kconfig | 20 ++++++++------------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index e0040a40d330..969f3f74a74d 100644
--- a/Makefile
+++ b/Makefile
@@ -851,7 +851,7 @@ HAVE_VENDOR_COMMON_LIB = $(if $(wildcard $(srctree)/board/$(VENDOR)/common/Makef
 libs-$(CONFIG_API) += api/
 libs-$(HAVE_VENDOR_COMMON_LIB) += board/$(VENDOR)/common/
 libs-y += boot/
-libs-y += cmd/
+libs-$(CONFIG_CMDLINE) += cmd/
 libs-y += common/
 libs-$(CONFIG_OF_EMBED) += dts/
 libs-y += env/
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 099ec444ae99..91297cb53f9a 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1,7 +1,5 @@
-menu "Command line interface"
-
-config CMDLINE
-	bool "Support U-Boot commands"
+menuconfig CMDLINE
+	bool "Command line interface"
 	default y
 	help
 	  Enable U-Boot's command-line functions. This provides a means
@@ -11,9 +9,10 @@ config CMDLINE
 	  Depending on the number of commands enabled, this can add
 	  substantially to the size of U-Boot.
 
+if CMDLINE
+
 config HUSH_PARSER
 	bool "Use hush shell"
-	depends on CMDLINE
 	help
 	  This option enables the "hush" shell (from Busybox) as command line
 	  interpreter, thus enabling powerful command line syntax like
@@ -25,7 +24,6 @@ config HUSH_PARSER
 
 config CMDLINE_EDITING
 	bool "Enable command line editing"
-	depends on CMDLINE
 	default y
 	help
 	  Enable editing and History functions for interactive command line
@@ -40,15 +38,13 @@ config CMDLINE_PS_SUPPORT
 
 config AUTO_COMPLETE
 	bool "Enable auto complete using TAB"
-	depends on CMDLINE
 	default y
 	help
 	  Enable auto completion of commands using TAB.
 
 config SYS_LONGHELP
 	bool "Enable long help messages"
-	depends on CMDLINE
-	default y if CMDLINE
+	default y
 	help
 	  Defined when you want long help messages included
 	  Do not set this option when short of memory.
@@ -77,8 +73,7 @@ config SYS_MAXARGS
 
 config SYS_XTRACE
 	bool "Command execution tracer"
-	depends on CMDLINE
-	default y if CMDLINE
+	default y
 	help
 	  This option enables the possiblity to print all commands before
 	  executing them and after all variables are evaluated (similar
@@ -2872,4 +2867,5 @@ config CMD_MESON
 	default y
 	help
 	  Enable useful commands for the Meson Soc family developed by Amlogic Inc.
-endmenu
+
+endif
-- 
2.34.1

[v4 21/24] sandbox: Disable CONFIG_DISTRO_DEFAULTS

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

This is not used for sandbox, so drop it. Enable the things that it
controls to avoid dstrastic changes in the config settings for
sandbox builds.

The end result is that these are enabled:

   BOOTMETH_DISTRO
   BOOTSTD_DEFAULTS

and these are disabled:

   USE_BOOTCOMMAND
   BOOTCOMMAND (was "run distro_bootcmd")
   DISTRO_DEFAULTS

Note that the tools-only build has already disabled DISTRO_DEFAULTS
and BOOTSTD_FULL

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
- Only modify sandbox and restrict the changes to only DISTRO_DEFAULTS
---
 arch/Kconfig                       | 3 +++
 configs/sandbox64_defconfig        | 1 -
 configs/sandbox_defconfig          | 1 -
 configs/sandbox_flattree_defconfig | 1 -
 configs/sandbox_noinst_defconfig   | 1 -
 configs/sandbox_spl_defconfig      | 1 -
 configs/sandbox_vpl_defconfig      | 1 -
 7 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 19f2891ba1c5..edfa3f7f83aa 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -208,6 +208,9 @@ config SANDBOX
 	imply PHYSMEM
 	imply GENERATE_ACPI_TABLE
 	imply BINMAN
+	imply BOOTSTD_DEFAULTS if BOOTSTD_FULL && CMDLINE
+	imply BOOTMETH_DISTRO if BOOTSTD_FULL && CMDLINE
+	imply CMD_SYSBOOT if BOOTSTD_FULL
 
 config SH
 	bool "SuperH architecture"
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 1a033b22018b..ff895b930170 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -14,7 +14,6 @@ CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_LEGACY_IMAGE_FORMAT=y
-CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_BOOTSTAGE_FDT=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 47417cb0391d..5230b81be2c4 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -14,7 +14,6 @@ CONFIG_FIT_RSASSA_PSS=y
 CONFIG_FIT_CIPHER=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_LEGACY_IMAGE_FORMAT=y
-CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_BOOTSTAGE_FDT=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index 29ae4532c508..8df2a82c521c 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -12,7 +12,6 @@ CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_LEGACY_IMAGE_FORMAT=y
-CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_BOOTSTAGE_FDT=y
diff --git a/configs/sandbox_noinst_defconfig b/configs/sandbox_noinst_defconfig
index db05e6308329..44e55f4452f3 100644
--- a/configs/sandbox_noinst_defconfig
+++ b/configs/sandbox_noinst_defconfig
@@ -25,7 +25,6 @@ CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_SPL_LOAD_FIT=y
-CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_BOOTSTAGE_FDT=y
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index 56072b15ad2d..015e0a59d085 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -20,7 +20,6 @@ CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_SPL_LOAD_FIT=y
-CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_BOOTSTAGE_FDT=y
diff --git a/configs/sandbox_vpl_defconfig b/configs/sandbox_vpl_defconfig
index 5bd0281796d4..9935575352b2 100644
--- a/configs/sandbox_vpl_defconfig
+++ b/configs/sandbox_vpl_defconfig
@@ -27,7 +27,6 @@ CONFIG_FIT=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_FIT_BEST_MATCH=y
 CONFIG_SPL_LOAD_FIT=y
-CONFIG_DISTRO_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_BOOTSTAGE_FDT=y
-- 
2.34.1

[v4 22/24] sandbox: Avoid requiring CMDLINE

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

Add some dependencies on features that we had been selecting so that we
can still disable CMDLINE.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
- Reword the commit slightly (Tom)
- Rework overall to select if CMDLINE

Changes in v3:
- Reorder the Kconfig options a little
---
 arch/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index edfa3f7f83aa..78727050aecd 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -134,7 +134,7 @@ config SANDBOX
 	select ARCH_SUPPORTS_LTO
 	select BOARD_LATE_INIT
 	select BZIP2
-	select CMD_POWEROFF
+	select CMD_POWEROFF if CMDLINE
 	select DM
 	select DM_EVENT
 	select DM_FUZZING_ENGINE
@@ -152,10 +152,10 @@ config SANDBOX
 	select PCI_ENDPOINT
 	select SPI
 	select SUPPORT_OF_CONTROL
-	select SYSRESET_CMD_POWEROFF
+	select SYSRESET_CMD_POWEROFF if CMD_POWEROFF
 	select SYS_CACHE_SHIFT_4
 	select IRQ
-	select SUPPORT_EXTENSION_SCAN
+	select SUPPORT_EXTENSION_SCAN if CMDLINE
 	select SUPPORT_ACPI
 	imply BITREVERSE
 	select BLOBLIST
-- 
2.34.1

[v4 23/24] sandbox: Add <asm/barrier.h>

[Tom Rini]

Add a mostly empty asm/barrier.h file for sandbox where we define nop() to
be an empty function.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 arch/sandbox/include/asm/barrier.h | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 arch/sandbox/include/asm/barrier.h

diff --git a/arch/sandbox/include/asm/barrier.h b/arch/sandbox/include/asm/barrier.h
new file mode 100644
index 000000000000..0928a78cbf8b
--- /dev/null
+++ b/arch/sandbox/include/asm/barrier.h
@@ -0,0 +1,3 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#define nop()
-- 
2.34.1

[v4 24/24] clk_k210.c: Clean up how we handle nop

[Tom Rini]

Now that sandbox has <asm/barrier.h> and defines nop() there we should
include that in our driver for clarity and then remove our local nop()
from <k210/pll.h>.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
I can see that our ARM <asm/barriers.h> should be <asm/barrier.h> and
updated in a few other ways to match how the kernel is currently.  This
is not a big deal yet as this driver is only for sandbox for risc-v

Cc: Sean Anderson <seanga2@gmail.com>
---
 drivers/clk/clk_k210.c | 1 +
 include/k210/pll.h     | 3 ---
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/clk/clk_k210.c b/drivers/clk/clk_k210.c
index c534cc07e092..b9469b93853b 100644
--- a/drivers/clk/clk_k210.c
+++ b/drivers/clk/clk_k210.c
@@ -16,6 +16,7 @@
 #include <dt-bindings/mfd/k210-sysctl.h>
 #include <k210/pll.h>
 #include <linux/bitfield.h>
+#include <asm/barrier.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
diff --git a/include/k210/pll.h b/include/k210/pll.h
index fd16a89cb203..175c47f6f233 100644
--- a/include/k210/pll.h
+++ b/include/k210/pll.h
@@ -16,9 +16,6 @@ struct k210_pll_config {
 #ifdef CONFIG_UNIT_TEST
 TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
 				     struct k210_pll_config *best);
-#ifndef nop
-#define nop()
-#endif
 
 #endif
 #endif /* K210_PLL_H */
-- 
2.34.1

Re: [v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[Heinrich Schuchardt]

On 19.10.23 17:00, Tom Rini wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> The command should not be used to enable library functionality. Add a
> new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
> same code is built.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Changes in v4:
> - Integrate AKASHI Takahiro's feedback from v3
> - Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
> ---
>   cmd/Kconfig             | 11 ++++++++++-
>   lib/efi_loader/Kconfig  |  6 +++---
>   lib/efi_loader/Makefile |  2 +-
>   3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 16e5cb8f0633..872cb49150cc 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -379,6 +379,15 @@ config CMD_BOOTEFI
>   	help
>   	  Boot an EFI image from memory.
>   
> +config CMD_BOOTEFI_BOOTMGR
> +	bool "UEFI Boot Manager command"
> +	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
> +	default y
> +	help
> +	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
> +	  This subcommand will allow you to select the UEFI binary to be booted
> +	  via UEFI variables Boot####, BootOrder, and BootNext.
> +
>   config CMD_BOOTEFI_HELLO_COMPILE
>   	bool "Compile a standard EFI hello world binary for testing"
>   	depends on CMD_BOOTEFI && !CPU_V7M
> @@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
>   config CMD_EFICONFIG
>   	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
>   	default y if !HAS_BOARD_SIZE_LIMIT
> -	depends on CMD_BOOTEFI_BOOTMGR
> +	depends on BOOTEFI_BOOTMGR
>   	select MENU
>   	help
>   	  Enable the 'eficonfig' command which provides the menu-driven UEFI
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index d20aaab6dba4..13cad6342c36 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -32,14 +32,14 @@ config EFI_LOADER
>   
>   if EFI_LOADER
>   
> -config CMD_BOOTEFI_BOOTMGR
> +config BOOTEFI_BOOTMGR
>   	bool "UEFI Boot Manager"
>   	default y
>   	select BOOTMETH_GLOBAL if BOOTSTD
>   	help
>   	  Select this option if you want to select the UEFI binary to be booted
> -	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
> -	  'bootefi bootmgr' command.
> +	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
> +	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
>   
>   choice
>   	prompt "Store for non-volatile UEFI variables"
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 8d31fc61c601..0a2cb6e3c476 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -42,7 +42,7 @@ targets += initrddump.o
>   endif
>   
>   obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> -obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> +obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
>   obj-y += efi_boottime.o
>   obj-y += efi_helper.o
>   obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o

This patch looks wrong.

Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is 
not related to the 'bootefi bootmgr' subcommand.

I see no benefit in two separate symbols. If you want to rename the 
symbol, please, replace *all* occurrences:

%s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/

Best regards

Heinrich

Re: [v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4175 bytes --]

On Thu, Oct 19, 2023 at 05:16:28PM +0200, Heinrich Schuchardt wrote:
> On 19.10.23 17:00, Tom Rini wrote:
> > From: Simon Glass <sjg@chromium.org>
> > 
> > The command should not be used to enable library functionality. Add a
> > new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
> > same code is built.
> > 
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Changes in v4:
> > - Integrate AKASHI Takahiro's feedback from v3
> > - Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
> > ---
> >   cmd/Kconfig             | 11 ++++++++++-
> >   lib/efi_loader/Kconfig  |  6 +++---
> >   lib/efi_loader/Makefile |  2 +-
> >   3 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 16e5cb8f0633..872cb49150cc 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -379,6 +379,15 @@ config CMD_BOOTEFI
> >   	help
> >   	  Boot an EFI image from memory.
> > +config CMD_BOOTEFI_BOOTMGR
> > +	bool "UEFI Boot Manager command"
> > +	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
> > +	default y
> > +	help
> > +	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
> > +	  This subcommand will allow you to select the UEFI binary to be booted
> > +	  via UEFI variables Boot####, BootOrder, and BootNext.
> > +
> >   config CMD_BOOTEFI_HELLO_COMPILE
> >   	bool "Compile a standard EFI hello world binary for testing"
> >   	depends on CMD_BOOTEFI && !CPU_V7M
> > @@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
> >   config CMD_EFICONFIG
> >   	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
> >   	default y if !HAS_BOARD_SIZE_LIMIT
> > -	depends on CMD_BOOTEFI_BOOTMGR
> > +	depends on BOOTEFI_BOOTMGR
> >   	select MENU
> >   	help
> >   	  Enable the 'eficonfig' command which provides the menu-driven UEFI
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index d20aaab6dba4..13cad6342c36 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -32,14 +32,14 @@ config EFI_LOADER
> >   if EFI_LOADER
> > -config CMD_BOOTEFI_BOOTMGR
> > +config BOOTEFI_BOOTMGR
> >   	bool "UEFI Boot Manager"
> >   	default y
> >   	select BOOTMETH_GLOBAL if BOOTSTD
> >   	help
> >   	  Select this option if you want to select the UEFI binary to be booted
> > -	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
> > -	  'bootefi bootmgr' command.
> > +	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
> > +	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
> >   choice
> >   	prompt "Store for non-volatile UEFI variables"
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index 8d31fc61c601..0a2cb6e3c476 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -42,7 +42,7 @@ targets += initrddump.o
> >   endif
> >   obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > -obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > +obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> >   obj-y += efi_boottime.o
> >   obj-y += efi_helper.o
> >   obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> 
> This patch looks wrong.
> 
> Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is not
> related to the 'bootefi bootmgr' subcommand.
> 
> I see no benefit in two separate symbols. If you want to rename the symbol,
> please, replace *all* occurrences:
> 
> %s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/

Yes, there's work on the EFI_LOADER side of things to support the use
case of "boot to menu" (or, "boot to efi bootmgr") of which this is the
starting point. The follow-up work that I'm hoping you or someone else
with more EFI_LOADER experience will pick up is splitting cmd/bootefi.c
such that we can call in to starting an EFI payload (or bootmgr) without
the command line.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[Heinrich Schuchardt]

On 19.10.23 17:19, Tom Rini wrote:
> On Thu, Oct 19, 2023 at 05:16:28PM +0200, Heinrich Schuchardt wrote:
>> On 19.10.23 17:00, Tom Rini wrote:
>>> From: Simon Glass <sjg@chromium.org>
>>>
>>> The command should not be used to enable library functionality. Add a
>>> new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
>>> same code is built.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> Changes in v4:
>>> - Integrate AKASHI Takahiro's feedback from v3
>>> - Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
>>> ---
>>>    cmd/Kconfig             | 11 ++++++++++-
>>>    lib/efi_loader/Kconfig  |  6 +++---
>>>    lib/efi_loader/Makefile |  2 +-
>>>    3 files changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index 16e5cb8f0633..872cb49150cc 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -379,6 +379,15 @@ config CMD_BOOTEFI
>>>    	help
>>>    	  Boot an EFI image from memory.
>>> +config CMD_BOOTEFI_BOOTMGR
>>> +	bool "UEFI Boot Manager command"
>>> +	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
>>> +	default y
>>> +	help
>>> +	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
>>> +	  This subcommand will allow you to select the UEFI binary to be booted
>>> +	  via UEFI variables Boot####, BootOrder, and BootNext.
>>> +
>>>    config CMD_BOOTEFI_HELLO_COMPILE
>>>    	bool "Compile a standard EFI hello world binary for testing"
>>>    	depends on CMD_BOOTEFI && !CPU_V7M
>>> @@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
>>>    config CMD_EFICONFIG
>>>    	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
>>>    	default y if !HAS_BOARD_SIZE_LIMIT
>>> -	depends on CMD_BOOTEFI_BOOTMGR
>>> +	depends on BOOTEFI_BOOTMGR
>>>    	select MENU
>>>    	help
>>>    	  Enable the 'eficonfig' command which provides the menu-driven UEFI
>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>> index d20aaab6dba4..13cad6342c36 100644
>>> --- a/lib/efi_loader/Kconfig
>>> +++ b/lib/efi_loader/Kconfig
>>> @@ -32,14 +32,14 @@ config EFI_LOADER
>>>    if EFI_LOADER
>>> -config CMD_BOOTEFI_BOOTMGR
>>> +config BOOTEFI_BOOTMGR
>>>    	bool "UEFI Boot Manager"
>>>    	default y
>>>    	select BOOTMETH_GLOBAL if BOOTSTD
>>>    	help
>>>    	  Select this option if you want to select the UEFI binary to be booted
>>> -	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
>>> -	  'bootefi bootmgr' command.
>>> +	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
>>> +	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
>>>    choice
>>>    	prompt "Store for non-volatile UEFI variables"
>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>> index 8d31fc61c601..0a2cb6e3c476 100644
>>> --- a/lib/efi_loader/Makefile
>>> +++ b/lib/efi_loader/Makefile
>>> @@ -42,7 +42,7 @@ targets += initrddump.o
>>>    endif
>>>    obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>>> -obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
>>> +obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
>>>    obj-y += efi_boottime.o
>>>    obj-y += efi_helper.o
>>>    obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
>>
>> This patch looks wrong.
>>
>> Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is not
>> related to the 'bootefi bootmgr' subcommand.
>>
>> I see no benefit in two separate symbols. If you want to rename the symbol,
>> please, replace *all* occurrences:
>>
>> %s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/
> 
> Yes, there's work on the EFI_LOADER side of things to support the use
> case of "boot to menu" (or, "boot to efi bootmgr") of which this is the
> starting point. The follow-up work that I'm hoping you or someone else
> with more EFI_LOADER experience will pick up is splitting cmd/bootefi.c
> such that we can call in to starting an EFI payload (or bootmgr) without
> the command line.
> 

Even after factoring out the boot functionality I would not know why we 
should have two separate symbols. I am fine with a rename which makes it 
clear that this symbol is about a library functionality.

Best regards

Heinrich

Re: [v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 38519 bytes --]

On Thu, Oct 19, 2023 at 05:24:33PM +0200, Heinrich Schuchardt wrote:
> On 19.10.23 17:19, Tom Rini wrote:
> > On Thu, Oct 19, 2023 at 05:16:28PM +0200, Heinrich Schuchardt wrote:
> > > On 19.10.23 17:00, Tom Rini wrote:
> > > > From: Simon Glass <sjg@chromium.org>
> > > > 
> > > > The command should not be used to enable library functionality. Add a
> > > > new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
> > > > same code is built.
> > > > 
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > > Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > Changes in v4:
> > > > - Integrate AKASHI Takahiro's feedback from v3
> > > > - Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
> > > > ---
> > > >    cmd/Kconfig             | 11 ++++++++++-
> > > >    lib/efi_loader/Kconfig  |  6 +++---
> > > >    lib/efi_loader/Makefile |  2 +-
> > > >    3 files changed, 14 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > index 16e5cb8f0633..872cb49150cc 100644
> > > > --- a/cmd/Kconfig
> > > > +++ b/cmd/Kconfig
> > > > @@ -379,6 +379,15 @@ config CMD_BOOTEFI
> > > >    	help
> > > >    	  Boot an EFI image from memory.
> > > > +config CMD_BOOTEFI_BOOTMGR
> > > > +	bool "UEFI Boot Manager command"
> > > > +	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
> > > > +	default y
> > > > +	help
> > > > +	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
> > > > +	  This subcommand will allow you to select the UEFI binary to be booted
> > > > +	  via UEFI variables Boot####, BootOrder, and BootNext.
> > > > +
> > > >    config CMD_BOOTEFI_HELLO_COMPILE
> > > >    	bool "Compile a standard EFI hello world binary for testing"
> > > >    	depends on CMD_BOOTEFI && !CPU_V7M
> > > > @@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
> > > >    config CMD_EFICONFIG
> > > >    	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
> > > >    	default y if !HAS_BOARD_SIZE_LIMIT
> > > > -	depends on CMD_BOOTEFI_BOOTMGR
> > > > +	depends on BOOTEFI_BOOTMGR
> > > >    	select MENU
> > > >    	help
> > > >    	  Enable the 'eficonfig' command which provides the menu-driven UEFI
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index d20aaab6dba4..13cad6342c36 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -32,14 +32,14 @@ config EFI_LOADER
> > > >    if EFI_LOADER
> > > > -config CMD_BOOTEFI_BOOTMGR
> > > > +config BOOTEFI_BOOTMGR
> > > >    	bool "UEFI Boot Manager"
> > > >    	default y
> > > >    	select BOOTMETH_GLOBAL if BOOTSTD
> > > >    	help
> > > >    	  Select this option if you want to select the UEFI binary to be booted
> > > > -	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
> > > > -	  'bootefi bootmgr' command.
> > > > +	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
> > > > +	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
> > > >    choice
> > > >    	prompt "Store for non-volatile UEFI variables"
> > > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > > index 8d31fc61c601..0a2cb6e3c476 100644
> > > > --- a/lib/efi_loader/Makefile
> > > > +++ b/lib/efi_loader/Makefile
> > > > @@ -42,7 +42,7 @@ targets += initrddump.o
> > > >    endif
> > > >    obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > > > -obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > > > +obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > > >    obj-y += efi_boottime.o
> > > >    obj-y += efi_helper.o
> > > >    obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> > > 
> > > This patch looks wrong.
> > > 
> > > Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is not
> > > related to the 'bootefi bootmgr' subcommand.
> > > 
> > > I see no benefit in two separate symbols. If you want to rename the symbol,
> > > please, replace *all* occurrences:
> > > 
> > > %s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/
> > 
> > Yes, there's work on the EFI_LOADER side of things to support the use
> > case of "boot to menu" (or, "boot to efi bootmgr") of which this is the
> > starting point. The follow-up work that I'm hoping you or someone else
> > with more EFI_LOADER experience will pick up is splitting cmd/bootefi.c
> > such that we can call in to starting an EFI payload (or bootmgr) without
> > the command line.
> > 
> 
> Even after factoring out the boot functionality I would not know why we
> should have two separate symbols. I am fine with a rename which makes it
> clear that this symbol is about a library functionality.

Because there's the library functionality and there's the literal
command code. If you're arguing that long term we should have the
command side of all library functionality be automatic based on
CMDLINE=y/CMDLINE=n, that's another discussion for later.  This just
start the split.  I had made a very very rough pass at starting the
split:

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 20e5c94a33a4..fe4f3b4fe4bc 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -27,379 +27,6 @@
 #include <asm-generic/sections.h>
 #include <linux/linkage.h>
 
-DECLARE_GLOBAL_DATA_PTR;
-
-static struct efi_device_path *bootefi_image_path;
-static struct efi_device_path *bootefi_device_path;
-static void *image_addr;
-static size_t image_size;
-
-/**
- * efi_get_image_parameters() - return image parameters
- *
- * @img_addr:		address of loaded image in memory
- * @img_size:		size of loaded image
- */
-void efi_get_image_parameters(void **img_addr, size_t *img_size)
-{
-	*img_addr = image_addr;
-	*img_size = image_size;
-}
-
-/**
- * efi_clear_bootdev() - clear boot device
- */
-static void efi_clear_bootdev(void)
-{
-	efi_free_pool(bootefi_device_path);
-	efi_free_pool(bootefi_image_path);
-	bootefi_device_path = NULL;
-	bootefi_image_path = NULL;
-	image_addr = NULL;
-	image_size = 0;
-}
-
-/**
- * efi_set_bootdev() - set boot device
- *
- * This function is called when a file is loaded, e.g. via the 'load' command.
- * We use the path to this file to inform the UEFI binary about the boot device.
- *
- * @dev:		device, e.g. "MMC"
- * @devnr:		number of the device, e.g. "1:2"
- * @path:		path to file loaded
- * @buffer:		buffer with file loaded
- * @buffer_size:	size of file loaded
- */
-void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
-		     void *buffer, size_t buffer_size)
-{
-	struct efi_device_path *device, *image;
-	efi_status_t ret;
-
-	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
-		  devnr, path, buffer, buffer_size);
-
-	/* Forget overwritten image */
-	if (buffer + buffer_size >= image_addr &&
-	    image_addr + image_size >= buffer)
-		efi_clear_bootdev();
-
-	/* Remember only PE-COFF and FIT images */
-	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
-		if (IS_ENABLED(CONFIG_FIT) &&
-		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
-			/*
-			 * FIT images of type EFI_OS are started via command
-			 * bootm. We should not use their boot device with the
-			 * bootefi command.
-			 */
-			buffer = 0;
-			buffer_size = 0;
-		} else {
-			log_debug("- not remembering image\n");
-			return;
-		}
-	}
-
-	/* efi_set_bootdev() is typically called repeatedly, recover memory */
-	efi_clear_bootdev();
-
-	image_addr = buffer;
-	image_size = buffer_size;
-
-	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
-	if (ret == EFI_SUCCESS) {
-		bootefi_device_path = device;
-		if (image) {
-			/* FIXME: image should not contain device */
-			struct efi_device_path *image_tmp = image;
-
-			efi_dp_split_file_path(image, &device, &image);
-			efi_free_pool(image_tmp);
-		}
-		bootefi_image_path = image;
-		log_debug("- boot device %pD\n", device);
-		if (image)
-			log_debug("- image %pD\n", image);
-	} else {
-		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
-		efi_clear_bootdev();
-	}
-}
-
-/**
- * efi_env_set_load_options() - set load options from environment variable
- *
- * @handle:		the image handle
- * @env_var:		name of the environment variable
- * @load_options:	pointer to load options (output)
- * Return:		status code
- */
-static efi_status_t efi_env_set_load_options(efi_handle_t handle,
-					     const char *env_var,
-					     u16 **load_options)
-{
-	const char *env = env_get(env_var);
-	size_t size;
-	u16 *pos;
-	efi_status_t ret;
-
-	*load_options = NULL;
-	if (!env)
-		return EFI_SUCCESS;
-	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
-	pos = calloc(size, 1);
-	if (!pos)
-		return EFI_OUT_OF_RESOURCES;
-	*load_options = pos;
-	utf8_utf16_strcpy(&pos, env);
-	ret = efi_set_load_options(handle, size, *load_options);
-	if (ret != EFI_SUCCESS) {
-		free(*load_options);
-		*load_options = NULL;
-	}
-	return ret;
-}
-
-#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
-
-/**
- * copy_fdt() - Copy the device tree to a new location available to EFI
- *
- * The FDT is copied to a suitable location within the EFI memory map.
- * Additional 12 KiB are added to the space in case the device tree needs to be
- * expanded later with fdt_open_into().
- *
- * @fdtp:	On entry a pointer to the flattened device tree.
- *		On exit a pointer to the copy of the flattened device tree.
- *		FDT start
- * Return:	status code
- */
-static efi_status_t copy_fdt(void **fdtp)
-{
-	unsigned long fdt_ram_start = -1L, fdt_pages;
-	efi_status_t ret = 0;
-	void *fdt, *new_fdt;
-	u64 new_fdt_addr;
-	uint fdt_size;
-	int i;
-
-	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
-		u64 ram_start = gd->bd->bi_dram[i].start;
-		u64 ram_size = gd->bd->bi_dram[i].size;
-
-		if (!ram_size)
-			continue;
-
-		if (ram_start < fdt_ram_start)
-			fdt_ram_start = ram_start;
-	}
-
-	/*
-	 * Give us at least 12 KiB of breathing room in case the device tree
-	 * needs to be expanded later.
-	 */
-	fdt = *fdtp;
-	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
-	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
-
-	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
-				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
-				 &new_fdt_addr);
-	if (ret != EFI_SUCCESS) {
-		log_err("ERROR: Failed to reserve space for FDT\n");
-		goto done;
-	}
-	new_fdt = (void *)(uintptr_t)new_fdt_addr;
-	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
-	fdt_set_totalsize(new_fdt, fdt_size);
-
-	*fdtp = (void *)(uintptr_t)new_fdt_addr;
-done:
-	return ret;
-}
-
-/**
- * get_config_table() - get configuration table
- *
- * @guid:	GUID of the configuration table
- * Return:	pointer to configuration table or NULL
- */
-static void *get_config_table(const efi_guid_t *guid)
-{
-	size_t i;
-
-	for (i = 0; i < systab.nr_tables; i++) {
-		if (!guidcmp(guid, &systab.tables[i].guid))
-			return systab.tables[i].table;
-	}
-	return NULL;
-}
-
-#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
-
-/**
- * efi_install_fdt() - install device tree
- *
- * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
- * address will will be installed as configuration table, otherwise the device
- * tree located at the address indicated by environment variable fdt_addr or as
- * fallback fdtcontroladdr will be used.
- *
- * On architectures using ACPI tables device trees shall not be installed as
- * configuration table.
- *
- * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use the
- *		the hardware device tree as indicated by environment variable
- *		fdt_addr or as fallback the internal device tree as indicated by
- *		the environment variable fdtcontroladdr
- * Return:	status code
- */
-efi_status_t efi_install_fdt(void *fdt)
-{
-	/*
-	 * The EBBR spec requires that we have either an FDT or an ACPI table
-	 * but not both.
-	 */
-#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
-	if (fdt) {
-		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
-		return EFI_SUCCESS;
-	}
-#else
-	struct bootm_headers img = { 0 };
-	efi_status_t ret;
-
-	if (fdt == EFI_FDT_USE_INTERNAL) {
-		const char *fdt_opt;
-		uintptr_t fdt_addr;
-
-		/* Look for device tree that is already installed */
-		if (get_config_table(&efi_guid_fdt))
-			return EFI_SUCCESS;
-		/* Check if there is a hardware device tree */
-		fdt_opt = env_get("fdt_addr");
-		/* Use our own device tree as fallback */
-		if (!fdt_opt) {
-			fdt_opt = env_get("fdtcontroladdr");
-			if (!fdt_opt) {
-				log_err("ERROR: need device tree\n");
-				return EFI_NOT_FOUND;
-			}
-		}
-		fdt_addr = hextoul(fdt_opt, NULL);
-		if (!fdt_addr) {
-			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
-			return EFI_LOAD_ERROR;
-		}
-		fdt = map_sysmem(fdt_addr, 0);
-	}
-
-	/* Install device tree */
-	if (fdt_check_header(fdt)) {
-		log_err("ERROR: invalid device tree\n");
-		return EFI_LOAD_ERROR;
-	}
-
-	/* Prepare device tree for payload */
-	ret = copy_fdt(&fdt);
-	if (ret) {
-		log_err("ERROR: out of memory\n");
-		return EFI_OUT_OF_RESOURCES;
-	}
-
-	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
-		log_err("ERROR: failed to process device tree\n");
-		return EFI_LOAD_ERROR;
-	}
-
-	/* Create memory reservations as indicated by the device tree */
-	efi_carve_out_dt_rsv(fdt);
-
-	efi_try_purge_kaslr_seed(fdt);
-
-	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
-		ret = efi_tcg2_measure_dtb(fdt);
-		if (ret == EFI_SECURITY_VIOLATION) {
-			log_err("ERROR: failed to measure DTB\n");
-			return ret;
-		}
-	}
-
-	/* Install device tree as UEFI table */
-	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
-	if (ret != EFI_SUCCESS) {
-		log_err("ERROR: failed to install device tree\n");
-		return ret;
-	}
-#endif /* GENERATE_ACPI_TABLE */
-
-	return EFI_SUCCESS;
-}
-
-/**
- * do_bootefi_exec() - execute EFI binary
- *
- * The image indicated by @handle is started. When it returns the allocated
- * memory for the @load_options is freed.
- *
- * @handle:		handle of loaded image
- * @load_options:	load options
- * Return:		status code
- *
- * Load the EFI binary into a newly assigned memory unwinding the relocation
- * information, install the loaded image protocol, and call the binary.
- */
-static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
-{
-	efi_status_t ret;
-	efi_uintn_t exit_data_size = 0;
-	u16 *exit_data = NULL;
-
-	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
-	switch_to_non_secure_mode();
-
-	/*
-	 * The UEFI standard requires that the watchdog timer is set to five
-	 * minutes when invoking an EFI boot option.
-	 *
-	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
-	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
-	 */
-	ret = efi_set_watchdog(300);
-	if (ret != EFI_SUCCESS) {
-		log_err("ERROR: Failed to set watchdog timer\n");
-		goto out;
-	}
-
-	/* Call our payload! */
-	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
-	if (ret != EFI_SUCCESS) {
-		log_err("## Application failed, r = %lu\n",
-			ret & ~EFI_ERROR_MASK);
-		if (exit_data) {
-			log_err("## %ls\n", exit_data);
-			efi_free_pool(exit_data);
-		}
-	}
-
-	efi_restore_gd();
-
-out:
-	free(load_options);
-
-	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
-		if (efi_initrd_deregister() != EFI_SUCCESS)
-			log_err("Failed to remove loadfile2 for initrd\n");
-	}
-
-	/* Control is returned to U-Boot, disable EFI watchdog */
-	efi_set_watchdog(0);
-
-	return ret;
-}
-
 /**
  * do_efibootmgr() - execute EFI boot manager
  *
@@ -461,6 +88,9 @@ static int do_bootefi_image(const char *image_opt, const char *size_opt)
 				return CMD_RET_USAGE;
 			efi_clear_bootdev();
 		} else {
+			size_t image_size;
+			void *image_addr;
+			efi_get_image_parameters(&image_addr, &image_size);
 			if (image_buf != image_addr) {
 				log_err("No UEFI binary known at %s\n",
 					image_opt);
@@ -477,135 +107,7 @@ static int do_bootefi_image(const char *image_opt, const char *size_opt)
 	return CMD_RET_SUCCESS;
 }
 
-/**
- * efi_run_image() - run loaded UEFI image
- *
- * @source_buffer:	memory address of the UEFI image
- * @source_size:	size of the UEFI image
- * Return:		status code
- */
-efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
-{
-	efi_handle_t mem_handle = NULL, handle;
-	struct efi_device_path *file_path = NULL;
-	struct efi_device_path *msg_path;
-	efi_status_t ret, ret2;
-	u16 *load_options;
-
-	if (!bootefi_device_path || !bootefi_image_path) {
-		log_debug("Not loaded from disk\n");
-		/*
-		 * Special case for efi payload not loaded from disk,
-		 * such as 'bootefi hello' or for example payload
-		 * loaded directly into memory via JTAG, etc:
-		 */
-		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-					    (uintptr_t)source_buffer,
-					    source_size);
-		/*
-		 * Make sure that device for device_path exist
-		 * in load_image(). Otherwise, shell and grub will fail.
-		 */
-		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
-							       &efi_guid_device_path,
-							       file_path, NULL);
-		if (ret != EFI_SUCCESS)
-			goto out;
-		msg_path = file_path;
-	} else {
-		file_path = efi_dp_append(bootefi_device_path,
-					  bootefi_image_path);
-		msg_path = bootefi_image_path;
-		log_debug("Loaded from disk\n");
-	}
-
-	log_info("Booting %pD\n", msg_path);
-
-	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
-				      source_size, &handle));
-	if (ret != EFI_SUCCESS) {
-		log_err("Loading image failed\n");
-		goto out;
-	}
-
-	/* Transfer environment variable as load options */
-	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	ret = do_bootefi_exec(handle, load_options);
-
-out:
-	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
-							  &efi_guid_device_path,
-							  file_path, NULL);
-	efi_free_pool(file_path);
-	return (ret != EFI_SUCCESS) ? ret : ret2;
-}
-
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
-static efi_status_t bootefi_run_prepare(const char *load_options_path,
-		struct efi_device_path *device_path,
-		struct efi_device_path *image_path,
-		struct efi_loaded_image_obj **image_objp,
-		struct efi_loaded_image **loaded_image_infop)
-{
-	efi_status_t ret;
-	u16 *load_options;
-
-	ret = efi_setup_loaded_image(device_path, image_path, image_objp,
-				     loaded_image_infop);
-	if (ret != EFI_SUCCESS)
-		return ret;
-
-	/* Transfer environment variable as load options */
-	return efi_env_set_load_options((efi_handle_t)*image_objp,
-					load_options_path,
-					&load_options);
-}
-
-/**
- * bootefi_test_prepare() - prepare to run an EFI test
- *
- * Prepare to run a test as if it were provided by a loaded image.
- *
- * @image_objp:		pointer to be set to the loaded image handle
- * @loaded_image_infop:	pointer to be set to the loaded image protocol
- * @path:		dummy file path used to construct the device path
- *			set in the loaded image protocol
- * @load_options_path:	name of a U-Boot environment variable. Its value is
- *			set as load options in the loaded image protocol.
- * Return:		status code
- */
-static efi_status_t bootefi_test_prepare
-		(struct efi_loaded_image_obj **image_objp,
-		 struct efi_loaded_image **loaded_image_infop, const char *path,
-		 const char *load_options_path)
-{
-	efi_status_t ret;
-
-	/* Construct a dummy device path */
-	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
-	if (!bootefi_device_path)
-		return EFI_OUT_OF_RESOURCES;
-
-	bootefi_image_path = efi_dp_from_file(NULL, path);
-	if (!bootefi_image_path) {
-		ret = EFI_OUT_OF_RESOURCES;
-		goto failure;
-	}
-
-	ret = bootefi_run_prepare(load_options_path, bootefi_device_path,
-				  bootefi_image_path, image_objp,
-				  loaded_image_infop);
-	if (ret == EFI_SUCCESS)
-		return ret;
-
-failure:
-	efi_clear_bootdev();
-	return ret;
-}
-
 /**
  * do_efi_selftest() - execute EFI selftest
  *
diff --git a/include/efi_loader.h b/include/efi_loader.h
index e24410505f40..6663a972c124 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -91,6 +91,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
  * back to u-boot world
  */
 void efi_restore_gd(void);
+void efi_clear_bootdev(void);
 /* Call this to set the current device name */
 void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
 		     void *buffer, size_t buffer_size);
@@ -526,12 +527,15 @@ efi_status_t efi_bootmgr_get_unused_bootoption(u16 *buf,
 efi_status_t efi_bootmgr_update_media_device_boot_option(void);
 /* Delete selected boot option */
 efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
+efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var,
+				      u16 **load_options);
 /* search the boot option index in BootOrder */
 bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index);
 /* Set up console modes */
 void efi_setup_console_size(void);
 /* Install device tree */
 efi_status_t efi_install_fdt(void *fdt);
+efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options);
 /* Run loaded UEFI image */
 efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
 /* Initialize variable services */
@@ -885,6 +889,9 @@ efi_status_t __efi_runtime EFIAPI efi_set_time(struct efi_time *time);
  */
 efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
 				 struct efi_system_table *systab);
+efi_status_t bootefi_test_prepare (struct efi_loaded_image_obj **image_objp,
+		 struct efi_loaded_image **loaded_image_infop, const char *path,
+		 const char *load_options_path);
 #endif
 
 efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8d31fc61c601..529a9756a98c 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -44,6 +44,7 @@ endif
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
 obj-y += efi_boottime.o
+obj-y += bootefi.o
 obj-y += efi_helper.o
 obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
 obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o
diff --git a/lib/efi_loader/bootefi.c b/lib/efi_loader/bootefi.c
new file mode 100644
index 000000000000..7b02b6f46eb5
--- /dev/null
+++ b/lib/efi_loader/bootefi.c
@@ -0,0 +1,530 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  EFI application loader
+ *
+ *  Copyright (c) 2016 Alexander Graf
+ */
+
+#define LOG_CATEGORY LOGC_EFI
+
+#include <common.h>
+#include <bootm.h>
+#include <charset.h>
+#include <command.h>
+#include <dm.h>
+#include <efi_loader.h>
+#include <efi_selftest.h>
+#include <env.h>
+#include <errno.h>
+#include <image.h>
+#include <log.h>
+#include <malloc.h>
+#include <asm/global_data.h>
+#include <linux/libfdt.h>
+#include <linux/libfdt_env.h>
+#include <mapmem.h>
+#include <memalign.h>
+#include <asm-generic/sections.h>
+#include <linux/linkage.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static struct efi_device_path *bootefi_image_path;
+static struct efi_device_path *bootefi_device_path;
+static void *image_addr;
+static size_t image_size;
+
+/**
+ * efi_get_image_parameters() - return image parameters
+ *
+ * @img_addr:		address of loaded image in memory
+ * @img_size:		size of loaded image
+ */
+void efi_get_image_parameters(void **img_addr, size_t *img_size)
+{
+	*img_addr = image_addr;
+	*img_size = image_size;
+}
+
+/**
+ * efi_clear_bootdev() - clear boot device
+ */
+void efi_clear_bootdev(void)
+{
+	efi_free_pool(bootefi_device_path);
+	efi_free_pool(bootefi_image_path);
+	bootefi_device_path = NULL;
+	bootefi_image_path = NULL;
+	image_addr = NULL;
+	image_size = 0;
+}
+
+/**
+ * efi_set_bootdev() - set boot device
+ *
+ * This function is called when a file is loaded, e.g. via the 'load' command.
+ * We use the path to this file to inform the UEFI binary about the boot device.
+ *
+ * @dev:		device, e.g. "MMC"
+ * @devnr:		number of the device, e.g. "1:2"
+ * @path:		path to file loaded
+ * @buffer:		buffer with file loaded
+ * @buffer_size:	size of file loaded
+ */
+void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
+		     void *buffer, size_t buffer_size)
+{
+	struct efi_device_path *device, *image;
+	efi_status_t ret;
+
+	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
+		  devnr, path, buffer, buffer_size);
+
+	/* Forget overwritten image */
+	if (buffer + buffer_size >= image_addr &&
+	    image_addr + image_size >= buffer)
+		efi_clear_bootdev();
+
+	/* Remember only PE-COFF and FIT images */
+	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
+		if (IS_ENABLED(CONFIG_FIT) &&
+		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
+			/*
+			 * FIT images of type EFI_OS are started via command
+			 * bootm. We should not use their boot device with the
+			 * bootefi command.
+			 */
+			buffer = 0;
+			buffer_size = 0;
+		} else {
+			log_debug("- not remembering image\n");
+			return;
+		}
+	}
+
+	/* efi_set_bootdev() is typically called repeatedly, recover memory */
+	efi_clear_bootdev();
+
+	image_addr = buffer;
+	image_size = buffer_size;
+
+	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
+	if (ret == EFI_SUCCESS) {
+		bootefi_device_path = device;
+		if (image) {
+			/* FIXME: image should not contain device */
+			struct efi_device_path *image_tmp = image;
+
+			efi_dp_split_file_path(image, &device, &image);
+			efi_free_pool(image_tmp);
+		}
+		bootefi_image_path = image;
+		log_debug("- boot device %pD\n", device);
+		if (image)
+			log_debug("- image %pD\n", image);
+	} else {
+		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
+		efi_clear_bootdev();
+	}
+}
+
+/**
+ * efi_env_set_load_options() - set load options from environment variable
+ *
+ * @handle:		the image handle
+ * @env_var:		name of the environment variable
+ * @load_options:	pointer to load options (output)
+ * Return:		status code
+ */
+efi_status_t efi_env_set_load_options(efi_handle_t handle,
+					     const char *env_var,
+					     u16 **load_options)
+{
+	const char *env = env_get(env_var);
+	size_t size;
+	u16 *pos;
+	efi_status_t ret;
+
+	*load_options = NULL;
+	if (!env)
+		return EFI_SUCCESS;
+	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
+	pos = calloc(size, 1);
+	if (!pos)
+		return EFI_OUT_OF_RESOURCES;
+	*load_options = pos;
+	utf8_utf16_strcpy(&pos, env);
+	ret = efi_set_load_options(handle, size, *load_options);
+	if (ret != EFI_SUCCESS) {
+		free(*load_options);
+		*load_options = NULL;
+	}
+	return ret;
+}
+
+#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
+
+/**
+ * copy_fdt() - Copy the device tree to a new location available to EFI
+ *
+ * The FDT is copied to a suitable location within the EFI memory map.
+ * Additional 12 KiB are added to the space in case the device tree needs to be
+ * expanded later with fdt_open_into().
+ *
+ * @fdtp:	On entry a pointer to the flattened device tree.
+ *		On exit a pointer to the copy of the flattened device tree.
+ *		FDT start
+ * Return:	status code
+ */
+static efi_status_t copy_fdt(void **fdtp)
+{
+	unsigned long fdt_ram_start = -1L, fdt_pages;
+	efi_status_t ret = 0;
+	void *fdt, *new_fdt;
+	u64 new_fdt_addr;
+	uint fdt_size;
+	int i;
+
+	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
+		u64 ram_start = gd->bd->bi_dram[i].start;
+		u64 ram_size = gd->bd->bi_dram[i].size;
+
+		if (!ram_size)
+			continue;
+
+		if (ram_start < fdt_ram_start)
+			fdt_ram_start = ram_start;
+	}
+
+	/*
+	 * Give us at least 12 KiB of breathing room in case the device tree
+	 * needs to be expanded later.
+	 */
+	fdt = *fdtp;
+	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
+	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
+
+	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
+				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
+				 &new_fdt_addr);
+	if (ret != EFI_SUCCESS) {
+		log_err("ERROR: Failed to reserve space for FDT\n");
+		goto done;
+	}
+	new_fdt = (void *)(uintptr_t)new_fdt_addr;
+	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
+	fdt_set_totalsize(new_fdt, fdt_size);
+
+	*fdtp = (void *)(uintptr_t)new_fdt_addr;
+done:
+	return ret;
+}
+
+/**
+ * get_config_table() - get configuration table
+ *
+ * @guid:	GUID of the configuration table
+ * Return:	pointer to configuration table or NULL
+ */
+static void *get_config_table(const efi_guid_t *guid)
+{
+	size_t i;
+
+	for (i = 0; i < systab.nr_tables; i++) {
+		if (!guidcmp(guid, &systab.tables[i].guid))
+			return systab.tables[i].table;
+	}
+	return NULL;
+}
+
+#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
+
+/**
+ * efi_install_fdt() - install device tree
+ *
+ * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
+ * address will will be installed as configuration table, otherwise the device
+ * tree located at the address indicated by environment variable fdt_addr or as
+ * fallback fdtcontroladdr will be used.
+ *
+ * On architectures using ACPI tables device trees shall not be installed as
+ * configuration table.
+ *
+ * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use the
+ *		the hardware device tree as indicated by environment variable
+ *		fdt_addr or as fallback the internal device tree as indicated by
+ *		the environment variable fdtcontroladdr
+ * Return:	status code
+ */
+efi_status_t efi_install_fdt(void *fdt)
+{
+	/*
+	 * The EBBR spec requires that we have either an FDT or an ACPI table
+	 * but not both.
+	 */
+#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
+	if (fdt) {
+		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
+		return EFI_SUCCESS;
+	}
+#else
+	struct bootm_headers img = { 0 };
+	efi_status_t ret;
+
+	if (fdt == EFI_FDT_USE_INTERNAL) {
+		const char *fdt_opt;
+		uintptr_t fdt_addr;
+
+		/* Look for device tree that is already installed */
+		if (get_config_table(&efi_guid_fdt))
+			return EFI_SUCCESS;
+		/* Check if there is a hardware device tree */
+		fdt_opt = env_get("fdt_addr");
+		/* Use our own device tree as fallback */
+		if (!fdt_opt) {
+			fdt_opt = env_get("fdtcontroladdr");
+			if (!fdt_opt) {
+				log_err("ERROR: need device tree\n");
+				return EFI_NOT_FOUND;
+			}
+		}
+		fdt_addr = hextoul(fdt_opt, NULL);
+		if (!fdt_addr) {
+			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
+			return EFI_LOAD_ERROR;
+		}
+		fdt = map_sysmem(fdt_addr, 0);
+	}
+
+	/* Install device tree */
+	if (fdt_check_header(fdt)) {
+		log_err("ERROR: invalid device tree\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	/* Prepare device tree for payload */
+	ret = copy_fdt(&fdt);
+	if (ret) {
+		log_err("ERROR: out of memory\n");
+		return EFI_OUT_OF_RESOURCES;
+	}
+
+	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
+		log_err("ERROR: failed to process device tree\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	/* Create memory reservations as indicated by the device tree */
+	efi_carve_out_dt_rsv(fdt);
+
+	efi_try_purge_kaslr_seed(fdt);
+
+	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
+		ret = efi_tcg2_measure_dtb(fdt);
+		if (ret == EFI_SECURITY_VIOLATION) {
+			log_err("ERROR: failed to measure DTB\n");
+			return ret;
+		}
+	}
+
+	/* Install device tree as UEFI table */
+	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
+	if (ret != EFI_SUCCESS) {
+		log_err("ERROR: failed to install device tree\n");
+		return ret;
+	}
+#endif /* GENERATE_ACPI_TABLE */
+
+	return EFI_SUCCESS;
+}
+
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+static efi_status_t bootefi_run_prepare(const char *load_options_path,
+		struct efi_device_path *device_path,
+		struct efi_device_path *image_path,
+		struct efi_loaded_image_obj **image_objp,
+		struct efi_loaded_image **loaded_image_infop)
+{
+	efi_status_t ret;
+	u16 *load_options;
+
+	ret = efi_setup_loaded_image(device_path, image_path, image_objp,
+				     loaded_image_infop);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	/* Transfer environment variable as load options */
+	return efi_env_set_load_options((efi_handle_t)*image_objp,
+					load_options_path,
+					&load_options);
+}
+
+/**
+ * bootefi_test_prepare() - prepare to run an EFI test
+ *
+ * Prepare to run a test as if it were provided by a loaded image.
+ *
+ * @image_objp:		pointer to be set to the loaded image handle
+ * @loaded_image_infop:	pointer to be set to the loaded image protocol
+ * @path:		dummy file path used to construct the device path
+ *			set in the loaded image protocol
+ * @load_options_path:	name of a U-Boot environment variable. Its value is
+ *			set as load options in the loaded image protocol.
+ * Return:		status code
+ */
+efi_status_t bootefi_test_prepare (struct efi_loaded_image_obj **image_objp,
+		 struct efi_loaded_image **loaded_image_infop, const char *path,
+		 const char *load_options_path)
+{
+	efi_status_t ret;
+
+	/* Construct a dummy device path */
+	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
+	if (!bootefi_device_path)
+		return EFI_OUT_OF_RESOURCES;
+
+	bootefi_image_path = efi_dp_from_file(NULL, path);
+	if (!bootefi_image_path) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto failure;
+	}
+
+	ret = bootefi_run_prepare(load_options_path, bootefi_device_path,
+				  bootefi_image_path, image_objp,
+				  loaded_image_infop);
+	if (ret == EFI_SUCCESS)
+		return ret;
+
+failure:
+	efi_clear_bootdev();
+	return ret;
+}
+#endif
+
+/**
+ * do_bootefi_exec() - execute EFI binary
+ *
+ * The image indicated by @handle is started. When it returns the allocated
+ * memory for the @load_options is freed.
+ *
+ * @handle:		handle of loaded image
+ * @load_options:	load options
+ * Return:		status code
+ *
+ * Load the EFI binary into a newly assigned memory unwinding the relocation
+ * information, install the loaded image protocol, and call the binary.
+ */
+efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
+{
+	efi_status_t ret;
+	efi_uintn_t exit_data_size = 0;
+	u16 *exit_data = NULL;
+
+	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
+	switch_to_non_secure_mode();
+
+	/*
+	 * The UEFI standard requires that the watchdog timer is set to five
+	 * minutes when invoking an EFI boot option.
+	 *
+	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
+	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
+	 */
+	ret = efi_set_watchdog(300);
+	if (ret != EFI_SUCCESS) {
+		log_err("ERROR: Failed to set watchdog timer\n");
+		goto out;
+	}
+
+	/* Call our payload! */
+	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
+	if (ret != EFI_SUCCESS) {
+		log_err("## Application failed, r = %lu\n",
+			ret & ~EFI_ERROR_MASK);
+		if (exit_data) {
+			log_err("## %ls\n", exit_data);
+			efi_free_pool(exit_data);
+		}
+	}
+
+	efi_restore_gd();
+
+out:
+	free(load_options);
+
+	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
+		if (efi_initrd_deregister() != EFI_SUCCESS)
+			log_err("Failed to remove loadfile2 for initrd\n");
+	}
+
+	/* Control is returned to U-Boot, disable EFI watchdog */
+	efi_set_watchdog(0);
+
+	return ret;
+}
+
+/**
+ * efi_run_image() - run loaded UEFI image
+ *
+ * @source_buffer:	memory address of the UEFI image
+ * @source_size:	size of the UEFI image
+ * Return:		status code
+ */
+efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
+{
+	efi_handle_t mem_handle = NULL, handle;
+	struct efi_device_path *file_path = NULL;
+	struct efi_device_path *msg_path;
+	efi_status_t ret, ret2;
+	u16 *load_options;
+
+	if (!bootefi_device_path || !bootefi_image_path) {
+		log_debug("Not loaded from disk\n");
+		/*
+		 * Special case for efi payload not loaded from disk,
+		 * such as 'bootefi hello' or for example payload
+		 * loaded directly into memory via JTAG, etc:
+		 */
+		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+					    (uintptr_t)source_buffer,
+					    source_size);
+		/*
+		 * Make sure that device for device_path exist
+		 * in load_image(). Otherwise, shell and grub will fail.
+		 */
+		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
+							       &efi_guid_device_path,
+							       file_path, NULL);
+		if (ret != EFI_SUCCESS)
+			goto out;
+		msg_path = file_path;
+	} else {
+		file_path = efi_dp_append(bootefi_device_path,
+					  bootefi_image_path);
+		msg_path = bootefi_image_path;
+		log_debug("Loaded from disk\n");
+	}
+
+	log_info("Booting %pD\n", msg_path);
+
+	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
+				      source_size, &handle));
+	if (ret != EFI_SUCCESS) {
+		log_err("Loading image failed\n");
+		goto out;
+	}
+
+	/* Transfer environment variable as load options */
+	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = do_bootefi_exec(handle, load_options);
+
+out:
+	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
+							  &efi_guid_device_path,
+							  file_path, NULL);
+	efi_free_pool(file_path);
+	return (ret != EFI_SUCCESS) ? ret : ret2;
+}

But that left needing to implement an API to start bootefi / bootefi
bootmgr itself as we still end up in C making a string and calling the
command, and ideally we would not be doing that with CMDLINE disabled.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[AKASHI Takahiro]

[-- Attachment #1: Type: text/plain, Size: 4621 bytes --]

On Thu, Oct 19, 2023 at 11:19:33AM -0400, Tom Rini wrote:
> On Thu, Oct 19, 2023 at 05:16:28PM +0200, Heinrich Schuchardt wrote:
> > On 19.10.23 17:00, Tom Rini wrote:
> > > From: Simon Glass <sjg@chromium.org>
> > > 
> > > The command should not be used to enable library functionality. Add a
> > > new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
> > > same code is built.
> > > 
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > > Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > Changes in v4:
> > > - Integrate AKASHI Takahiro's feedback from v3
> > > - Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
> > > ---
> > >   cmd/Kconfig             | 11 ++++++++++-
> > >   lib/efi_loader/Kconfig  |  6 +++---
> > >   lib/efi_loader/Makefile |  2 +-
> > >   3 files changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > index 16e5cb8f0633..872cb49150cc 100644
> > > --- a/cmd/Kconfig
> > > +++ b/cmd/Kconfig
> > > @@ -379,6 +379,15 @@ config CMD_BOOTEFI
> > >   	help
> > >   	  Boot an EFI image from memory.
> > > +config CMD_BOOTEFI_BOOTMGR
> > > +	bool "UEFI Boot Manager command"
> > > +	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
> > > +	default y
> > > +	help
> > > +	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
> > > +	  This subcommand will allow you to select the UEFI binary to be booted
> > > +	  via UEFI variables Boot####, BootOrder, and BootNext.
> > > +
> > >   config CMD_BOOTEFI_HELLO_COMPILE
> > >   	bool "Compile a standard EFI hello world binary for testing"
> > >   	depends on CMD_BOOTEFI && !CPU_V7M
> > > @@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
> > >   config CMD_EFICONFIG
> > >   	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
> > >   	default y if !HAS_BOARD_SIZE_LIMIT
> > > -	depends on CMD_BOOTEFI_BOOTMGR
> > > +	depends on BOOTEFI_BOOTMGR
> > >   	select MENU
> > >   	help
> > >   	  Enable the 'eficonfig' command which provides the menu-driven UEFI
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index d20aaab6dba4..13cad6342c36 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -32,14 +32,14 @@ config EFI_LOADER
> > >   if EFI_LOADER
> > > -config CMD_BOOTEFI_BOOTMGR
> > > +config BOOTEFI_BOOTMGR
> > >   	bool "UEFI Boot Manager"
> > >   	default y
> > >   	select BOOTMETH_GLOBAL if BOOTSTD
> > >   	help
> > >   	  Select this option if you want to select the UEFI binary to be booted
> > > -	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
> > > -	  'bootefi bootmgr' command.
> > > +	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
> > > +	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
> > >   choice
> > >   	prompt "Store for non-volatile UEFI variables"
> > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > index 8d31fc61c601..0a2cb6e3c476 100644
> > > --- a/lib/efi_loader/Makefile
> > > +++ b/lib/efi_loader/Makefile
> > > @@ -42,7 +42,7 @@ targets += initrddump.o
> > >   endif
> > >   obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > > -obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > > +obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > >   obj-y += efi_boottime.o
> > >   obj-y += efi_helper.o
> > >   obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> > 
> > This patch looks wrong.
> > 
> > Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is not
> > related to the 'bootefi bootmgr' subcommand.
> > 
> > I see no benefit in two separate symbols. If you want to rename the symbol,
> > please, replace *all* occurrences:
> > 
> > %s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/
> 
> Yes, there's work on the EFI_LOADER side of things to support the use
> case of "boot to menu" (or, "boot to efi bootmgr") of which this is the
> starting point. The follow-up work that I'm hoping you or someone else
> with more EFI_LOADER experience will pick up is splitting cmd/bootefi.c
> such that we can call in to starting an EFI payload (or bootmgr) without
> the command line.

I will be able to take care unless Heinrich wants to do by himself.
# Test would be another issue now that "bootefi" may handle various
  boot scenarios.

-Takahiro Akashi

> 
> -- 
> Tom



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [v4 11/24] efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5256 bytes --]

On Fri, Oct 20, 2023 at 09:21:00PM +0900, AKASHI Takahiro wrote:
> On Thu, Oct 19, 2023 at 11:19:33AM -0400, Tom Rini wrote:
> > On Thu, Oct 19, 2023 at 05:16:28PM +0200, Heinrich Schuchardt wrote:
> > > On 19.10.23 17:00, Tom Rini wrote:
> > > > From: Simon Glass <sjg@chromium.org>
> > > > 
> > > > The command should not be used to enable library functionality. Add a
> > > > new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
> > > > same code is built.
> > > > 
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > > Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > Changes in v4:
> > > > - Integrate AKASHI Takahiro's feedback from v3
> > > > - Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
> > > > ---
> > > >   cmd/Kconfig             | 11 ++++++++++-
> > > >   lib/efi_loader/Kconfig  |  6 +++---
> > > >   lib/efi_loader/Makefile |  2 +-
> > > >   3 files changed, 14 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > index 16e5cb8f0633..872cb49150cc 100644
> > > > --- a/cmd/Kconfig
> > > > +++ b/cmd/Kconfig
> > > > @@ -379,6 +379,15 @@ config CMD_BOOTEFI
> > > >   	help
> > > >   	  Boot an EFI image from memory.
> > > > +config CMD_BOOTEFI_BOOTMGR
> > > > +	bool "UEFI Boot Manager command"
> > > > +	depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
> > > > +	default y
> > > > +	help
> > > > +	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
> > > > +	  This subcommand will allow you to select the UEFI binary to be booted
> > > > +	  via UEFI variables Boot####, BootOrder, and BootNext.
> > > > +
> > > >   config CMD_BOOTEFI_HELLO_COMPILE
> > > >   	bool "Compile a standard EFI hello world binary for testing"
> > > >   	depends on CMD_BOOTEFI && !CPU_V7M
> > > > @@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
> > > >   config CMD_EFICONFIG
> > > >   	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
> > > >   	default y if !HAS_BOARD_SIZE_LIMIT
> > > > -	depends on CMD_BOOTEFI_BOOTMGR
> > > > +	depends on BOOTEFI_BOOTMGR
> > > >   	select MENU
> > > >   	help
> > > >   	  Enable the 'eficonfig' command which provides the menu-driven UEFI
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index d20aaab6dba4..13cad6342c36 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -32,14 +32,14 @@ config EFI_LOADER
> > > >   if EFI_LOADER
> > > > -config CMD_BOOTEFI_BOOTMGR
> > > > +config BOOTEFI_BOOTMGR
> > > >   	bool "UEFI Boot Manager"
> > > >   	default y
> > > >   	select BOOTMETH_GLOBAL if BOOTSTD
> > > >   	help
> > > >   	  Select this option if you want to select the UEFI binary to be booted
> > > > -	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
> > > > -	  'bootefi bootmgr' command.
> > > > +	  via UEFI variables Boot####, BootOrder, and BootNext. You should also
> > > > +	  normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
> > > >   choice
> > > >   	prompt "Store for non-volatile UEFI variables"
> > > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > > index 8d31fc61c601..0a2cb6e3c476 100644
> > > > --- a/lib/efi_loader/Makefile
> > > > +++ b/lib/efi_loader/Makefile
> > > > @@ -42,7 +42,7 @@ targets += initrddump.o
> > > >   endif
> > > >   obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > > > -obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > > > +obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > > >   obj-y += efi_boottime.o
> > > >   obj-y += efi_helper.o
> > > >   obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> > > 
> > > This patch looks wrong.
> > > 
> > > Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is not
> > > related to the 'bootefi bootmgr' subcommand.
> > > 
> > > I see no benefit in two separate symbols. If you want to rename the symbol,
> > > please, replace *all* occurrences:
> > > 
> > > %s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/
> > 
> > Yes, there's work on the EFI_LOADER side of things to support the use
> > case of "boot to menu" (or, "boot to efi bootmgr") of which this is the
> > starting point. The follow-up work that I'm hoping you or someone else
> > with more EFI_LOADER experience will pick up is splitting cmd/bootefi.c
> > such that we can call in to starting an EFI payload (or bootmgr) without
> > the command line.
> 
> I will be able to take care unless Heinrich wants to do by himself.
> # Test would be another issue now that "bootefi" may handle various
>   boot scenarios.

At the very high level, this is solved for "bootm" today, and I _think_
booti/bootz also but I'm less sure of that.  The non-command side logic
is in boot/bootm.c rather than cmd/bootm.c and so we exercise the core
still via the normal tests. I don't know if the best answer is
boot/bootefi.c or lib/efi_loader/something.c. Tests of new functionality
will require some thought tho, yes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[v4.1 1/2] sandbox: Add a test for disabling CONFIG_CMDLINE

[Tom Rini]

From: Simon Glass <sjg@chromium.org>

Now that everything is working, add a test to make sure that this
builds correctly.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
 test/py/tests/test_sandbox_opts.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 test/py/tests/test_sandbox_opts.py

diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
new file mode 100644
index 000000000000..91790b3374b4
--- /dev/null
+++ b/test/py/tests/test_sandbox_opts.py
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2022 Google LLC
+# Written by Simon Glass <sjg@chromium.org>
+
+import pytest
+
+import u_boot_utils as util
+
+# This is needed for Azure, since the default '..' directory is not writeable
+TMPDIR = '/tmp/test_cmdline'
+
+@pytest.mark.slow
+@pytest.mark.boardspec('sandbox')
+def test_sandbox_cmdline(u_boot_console):
+    """Test building sandbox without CONFIG_CMDLINE"""
+    cons = u_boot_console
+
+    out = util.run_and_log(
+        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
+               '-a', '~CMDLINE', '-o', TMPDIR])
-- 
2.34.1

[v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Tom Rini]

The primary motivation for having a sandbox without LTO build in CI is
to ensure that we don't have that option break. We now have the ability
to run tests of specific options being enabled/disabled, so drop the
parts of CI that build and test that configuration specifically and add
a build test instead. We still test that "NO_LTO=1" rather than editing
the config file works via the ftrace tests.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
This creates a small bisectability gap in CI itself, but I think is more
reasonable than reworking the introduction of
test/py/tests/test_sandbox_opts.py

Cc: Simon Glass <sjg@chromium.org>
---
 .azure-pipelines.yml               |  3 ---
 .gitlab-ci.yml                     | 12 ------------
 test/py/tests/test_sandbox_opts.py | 10 ++++++++++
 3 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
index 6f91553e8613..65533b36dde4 100644
--- a/.azure-pipelines.yml
+++ b/.azure-pipelines.yml
@@ -287,9 +287,6 @@ stages:
         sandbox64_clang:
           TEST_PY_BD: "sandbox64"
           OVERRIDE: "-O clang-16"
-        sandbox_nolto:
-          TEST_PY_BD: "sandbox"
-          BUILD_ENV: "NO_LTO=1"
         sandbox_spl:
           TEST_PY_BD: "sandbox_spl"
           TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 6decdfdee334..97c964fb8079 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -258,12 +258,6 @@ sandbox with clang test.py:
     OVERRIDE: "-O clang-16"
   <<: *buildman_and_testpy_dfn
 
-sandbox without LTO test.py:
-  variables:
-    TEST_PY_BD: "sandbox"
-    BUILD_ENV: "NO_LTO=1"
-  <<: *buildman_and_testpy_dfn
-
 sandbox64 test.py:
   variables:
     TEST_PY_BD: "sandbox64"
@@ -275,12 +269,6 @@ sandbox64 with clang test.py:
     OVERRIDE: "-O clang-16"
   <<: *buildman_and_testpy_dfn
 
-sandbox64 without LTO test.py:
-  variables:
-    TEST_PY_BD: "sandbox64"
-    BUILD_ENV: "NO_LTO=1"
-  <<: *buildman_and_testpy_dfn
-
 sandbox_spl test.py:
   variables:
     TEST_PY_BD: "sandbox_spl"
diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
index 91790b3374b4..422b43cb3bc1 100644
--- a/test/py/tests/test_sandbox_opts.py
+++ b/test/py/tests/test_sandbox_opts.py
@@ -18,3 +18,13 @@ def test_sandbox_cmdline(u_boot_console):
     out = util.run_and_log(
         cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
                '-a', '~CMDLINE', '-o', TMPDIR])
+
+@pytest.mark.slow
+@pytest.mark.boardspec('sandbox')
+def test_sandbox_lto(u_boot_console):
+    """Test building sandbox without CONFIG_LTO"""
+    cons = u_boot_console
+
+    out = util.run_and_log(
+        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
+               '-a', '~LTO', '-o', TMPDIR])
-- 
2.34.1

Re: [v4 24/24] clk_k210.c: Clean up how we handle nop

[Sean Anderson]

On 10/19/23 11:01, Tom Rini wrote:
> Now that sandbox has <asm/barrier.h> and defines nop() there we should
> include that in our driver for clarity and then remove our local nop()
> from <k210/pll.h>.
> 
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> I can see that our ARM <asm/barriers.h> should be <asm/barrier.h> and
> updated in a few other ways to match how the kernel is currently.  This
> is not a big deal yet as this driver is only for sandbox for risc-v
> 
> Cc: Sean Anderson <seanga2@gmail.com>
> ---
>   drivers/clk/clk_k210.c | 1 +
>   include/k210/pll.h     | 3 ---
>   2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk_k210.c b/drivers/clk/clk_k210.c
> index c534cc07e092..b9469b93853b 100644
> --- a/drivers/clk/clk_k210.c
> +++ b/drivers/clk/clk_k210.c
> @@ -16,6 +16,7 @@
>   #include <dt-bindings/mfd/k210-sysctl.h>
>   #include <k210/pll.h>
>   #include <linux/bitfield.h>
> +#include <asm/barrier.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> diff --git a/include/k210/pll.h b/include/k210/pll.h
> index fd16a89cb203..175c47f6f233 100644
> --- a/include/k210/pll.h
> +++ b/include/k210/pll.h
> @@ -16,9 +16,6 @@ struct k210_pll_config {
>   #ifdef CONFIG_UNIT_TEST
>   TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>   				     struct k210_pll_config *best);
> -#ifndef nop
> -#define nop()
> -#endif
>   
>   #endif
>   #endif /* K210_PLL_H */

Reviewed-by: Sean Anderson <seanga2@gmail.com>

Re: [v4 23/24] sandbox: Add <asm/barrier.h>

[Sean Anderson]

On 10/19/23 11:01, Tom Rini wrote:
> Add a mostly empty asm/barrier.h file for sandbox where we define nop() to
> be an empty function.
> 
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>   arch/sandbox/include/asm/barrier.h | 3 +++
>   1 file changed, 3 insertions(+)
>   create mode 100644 arch/sandbox/include/asm/barrier.h
> 
> diff --git a/arch/sandbox/include/asm/barrier.h b/arch/sandbox/include/asm/barrier.h
> new file mode 100644
> index 000000000000..0928a78cbf8b
> --- /dev/null
> +++ b/arch/sandbox/include/asm/barrier.h
> @@ -0,0 +1,3 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#define nop()

Reviewed-by: Sean Anderson <seanga2@gmail.com>

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Simon Glass]

On Fri, 20 Oct 2023 at 14:53, Tom Rini <trini@konsulko.com> wrote:
>
> The primary motivation for having a sandbox without LTO build in CI is
> to ensure that we don't have that option break. We now have the ability
> to run tests of specific options being enabled/disabled, so drop the
> parts of CI that build and test that configuration specifically and add
> a build test instead. We still test that "NO_LTO=1" rather than editing
> the config file works via the ftrace tests.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> This creates a small bisectability gap in CI itself, but I think is more
> reasonable than reworking the introduction of
> test/py/tests/test_sandbox_opts.py
>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  .azure-pipelines.yml               |  3 ---
>  .gitlab-ci.yml                     | 12 ------------
>  test/py/tests/test_sandbox_opts.py | 10 ++++++++++
>  3 files changed, 10 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Q below

>
> diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> index 6f91553e8613..65533b36dde4 100644
> --- a/.azure-pipelines.yml
> +++ b/.azure-pipelines.yml
> @@ -287,9 +287,6 @@ stages:
>          sandbox64_clang:
>            TEST_PY_BD: "sandbox64"
>            OVERRIDE: "-O clang-16"
> -        sandbox_nolto:
> -          TEST_PY_BD: "sandbox"
> -          BUILD_ENV: "NO_LTO=1"
>          sandbox_spl:
>            TEST_PY_BD: "sandbox_spl"
>            TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl"
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 6decdfdee334..97c964fb8079 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -258,12 +258,6 @@ sandbox with clang test.py:
>      OVERRIDE: "-O clang-16"
>    <<: *buildman_and_testpy_dfn
>
> -sandbox without LTO test.py:
> -  variables:
> -    TEST_PY_BD: "sandbox"
> -    BUILD_ENV: "NO_LTO=1"
> -  <<: *buildman_and_testpy_dfn
> -
>  sandbox64 test.py:
>    variables:
>      TEST_PY_BD: "sandbox64"
> @@ -275,12 +269,6 @@ sandbox64 with clang test.py:
>      OVERRIDE: "-O clang-16"
>    <<: *buildman_and_testpy_dfn
>
> -sandbox64 without LTO test.py:
> -  variables:
> -    TEST_PY_BD: "sandbox64"
> -    BUILD_ENV: "NO_LTO=1"
> -  <<: *buildman_and_testpy_dfn
> -
>  sandbox_spl test.py:
>    variables:
>      TEST_PY_BD: "sandbox_spl"
> diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
> index 91790b3374b4..422b43cb3bc1 100644
> --- a/test/py/tests/test_sandbox_opts.py
> +++ b/test/py/tests/test_sandbox_opts.py
> @@ -18,3 +18,13 @@ def test_sandbox_cmdline(u_boot_console):
>      out = util.run_and_log(
>          cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
>                 '-a', '~CMDLINE', '-o', TMPDIR])
> +
> +@pytest.mark.slow
> +@pytest.mark.boardspec('sandbox')
> +def test_sandbox_lto(u_boot_console):
> +    """Test building sandbox without CONFIG_LTO"""
> +    cons = u_boot_console
> +
> +    out = util.run_and_log(
> +        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> +               '-a', '~LTO', '-o', TMPDIR])

Don't you need sandbox64 here?

> --
> 2.34.1
>

Regards,
Simon

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3818 bytes --]

On Sat, Oct 21, 2023 at 08:43:00AM -0700, Simon Glass wrote:
> On Fri, 20 Oct 2023 at 14:53, Tom Rini <trini@konsulko.com> wrote:
> >
> > The primary motivation for having a sandbox without LTO build in CI is
> > to ensure that we don't have that option break. We now have the ability
> > to run tests of specific options being enabled/disabled, so drop the
> > parts of CI that build and test that configuration specifically and add
> > a build test instead. We still test that "NO_LTO=1" rather than editing
> > the config file works via the ftrace tests.
> >
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > This creates a small bisectability gap in CI itself, but I think is more
> > reasonable than reworking the introduction of
> > test/py/tests/test_sandbox_opts.py
> >
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> >  .azure-pipelines.yml               |  3 ---
> >  .gitlab-ci.yml                     | 12 ------------
> >  test/py/tests/test_sandbox_opts.py | 10 ++++++++++
> >  3 files changed, 10 insertions(+), 15 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Q below
> 
> >
> > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > index 6f91553e8613..65533b36dde4 100644
> > --- a/.azure-pipelines.yml
> > +++ b/.azure-pipelines.yml
> > @@ -287,9 +287,6 @@ stages:
> >          sandbox64_clang:
> >            TEST_PY_BD: "sandbox64"
> >            OVERRIDE: "-O clang-16"
> > -        sandbox_nolto:
> > -          TEST_PY_BD: "sandbox"
> > -          BUILD_ENV: "NO_LTO=1"
> >          sandbox_spl:
> >            TEST_PY_BD: "sandbox_spl"
> >            TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl"
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index 6decdfdee334..97c964fb8079 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -258,12 +258,6 @@ sandbox with clang test.py:
> >      OVERRIDE: "-O clang-16"
> >    <<: *buildman_and_testpy_dfn
> >
> > -sandbox without LTO test.py:
> > -  variables:
> > -    TEST_PY_BD: "sandbox"
> > -    BUILD_ENV: "NO_LTO=1"
> > -  <<: *buildman_and_testpy_dfn
> > -
> >  sandbox64 test.py:
> >    variables:
> >      TEST_PY_BD: "sandbox64"
> > @@ -275,12 +269,6 @@ sandbox64 with clang test.py:
> >      OVERRIDE: "-O clang-16"
> >    <<: *buildman_and_testpy_dfn
> >
> > -sandbox64 without LTO test.py:
> > -  variables:
> > -    TEST_PY_BD: "sandbox64"
> > -    BUILD_ENV: "NO_LTO=1"
> > -  <<: *buildman_and_testpy_dfn
> > -
> >  sandbox_spl test.py:
> >    variables:
> >      TEST_PY_BD: "sandbox_spl"
> > diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
> > index 91790b3374b4..422b43cb3bc1 100644
> > --- a/test/py/tests/test_sandbox_opts.py
> > +++ b/test/py/tests/test_sandbox_opts.py
> > @@ -18,3 +18,13 @@ def test_sandbox_cmdline(u_boot_console):
> >      out = util.run_and_log(
> >          cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> >                 '-a', '~CMDLINE', '-o', TMPDIR])
> > +
> > +@pytest.mark.slow
> > +@pytest.mark.boardspec('sandbox')
> > +def test_sandbox_lto(u_boot_console):
> > +    """Test building sandbox without CONFIG_LTO"""
> > +    cons = u_boot_console
> > +
> > +    out = util.run_and_log(
> > +        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > +               '-a', '~LTO', '-o', TMPDIR])
> 
> Don't you need sandbox64 here?

No, I don't think it's providing further value to just build sandbox64
without LTO. I'm also not 100% sure this patch is really needed in that we're
trying to test for "did we disable LTO via make arguments" and in turn
only the ftrace test really catches that, as it will fail if we do have
LTO enabled, yes?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Simon Glass]

Hi Tom,

On Sat, 21 Oct 2023 at 11:34, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Oct 21, 2023 at 08:43:00AM -0700, Simon Glass wrote:
> > On Fri, 20 Oct 2023 at 14:53, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > The primary motivation for having a sandbox without LTO build in CI is
> > > to ensure that we don't have that option break. We now have the ability
> > > to run tests of specific options being enabled/disabled, so drop the
> > > parts of CI that build and test that configuration specifically and add
> > > a build test instead. We still test that "NO_LTO=1" rather than editing
> > > the config file works via the ftrace tests.
> > >
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > > This creates a small bisectability gap in CI itself, but I think is more
> > > reasonable than reworking the introduction of
> > > test/py/tests/test_sandbox_opts.py
> > >
> > > Cc: Simon Glass <sjg@chromium.org>
> > > ---
> > >  .azure-pipelines.yml               |  3 ---
> > >  .gitlab-ci.yml                     | 12 ------------
> > >  test/py/tests/test_sandbox_opts.py | 10 ++++++++++
> > >  3 files changed, 10 insertions(+), 15 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Q below
> >
> > >
> > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > index 6f91553e8613..65533b36dde4 100644
> > > --- a/.azure-pipelines.yml
> > > +++ b/.azure-pipelines.yml
> > > @@ -287,9 +287,6 @@ stages:
> > >          sandbox64_clang:
> > >            TEST_PY_BD: "sandbox64"
> > >            OVERRIDE: "-O clang-16"
> > > -        sandbox_nolto:
> > > -          TEST_PY_BD: "sandbox"
> > > -          BUILD_ENV: "NO_LTO=1"
> > >          sandbox_spl:
> > >            TEST_PY_BD: "sandbox_spl"
> > >            TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl"
> > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > > index 6decdfdee334..97c964fb8079 100644
> > > --- a/.gitlab-ci.yml
> > > +++ b/.gitlab-ci.yml
> > > @@ -258,12 +258,6 @@ sandbox with clang test.py:
> > >      OVERRIDE: "-O clang-16"
> > >    <<: *buildman_and_testpy_dfn
> > >
> > > -sandbox without LTO test.py:
> > > -  variables:
> > > -    TEST_PY_BD: "sandbox"
> > > -    BUILD_ENV: "NO_LTO=1"
> > > -  <<: *buildman_and_testpy_dfn
> > > -
> > >  sandbox64 test.py:
> > >    variables:
> > >      TEST_PY_BD: "sandbox64"
> > > @@ -275,12 +269,6 @@ sandbox64 with clang test.py:
> > >      OVERRIDE: "-O clang-16"
> > >    <<: *buildman_and_testpy_dfn
> > >
> > > -sandbox64 without LTO test.py:
> > > -  variables:
> > > -    TEST_PY_BD: "sandbox64"
> > > -    BUILD_ENV: "NO_LTO=1"
> > > -  <<: *buildman_and_testpy_dfn
> > > -
> > >  sandbox_spl test.py:
> > >    variables:
> > >      TEST_PY_BD: "sandbox_spl"
> > > diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
> > > index 91790b3374b4..422b43cb3bc1 100644
> > > --- a/test/py/tests/test_sandbox_opts.py
> > > +++ b/test/py/tests/test_sandbox_opts.py
> > > @@ -18,3 +18,13 @@ def test_sandbox_cmdline(u_boot_console):
> > >      out = util.run_and_log(
> > >          cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > >                 '-a', '~CMDLINE', '-o', TMPDIR])
> > > +
> > > +@pytest.mark.slow
> > > +@pytest.mark.boardspec('sandbox')
> > > +def test_sandbox_lto(u_boot_console):
> > > +    """Test building sandbox without CONFIG_LTO"""
> > > +    cons = u_boot_console
> > > +
> > > +    out = util.run_and_log(
> > > +        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > > +               '-a', '~LTO', '-o', TMPDIR])
> >
> > Don't you need sandbox64 here?
>
> No, I don't think it's providing further value to just build sandbox64
> without LTO. I'm also not 100% sure this patch is really needed in that we're
> trying to test for "did we disable LTO via make arguments" and in turn
> only the ftrace test really catches that, as it will fail if we do have
> LTO enabled, yes?

Really the test is to make sure that sandbox builds without LTO. With
the build time being. For development, LTO serves no useful purpose
and just triples the incremental build time.

I suggest we keep sandbox64 just to prevent a regression on non-LTO,
but I suspect it is unlikely to happen in practice.

Regards,
Simon

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5038 bytes --]

On Mon, Oct 23, 2023 at 12:05:34AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 21 Oct 2023 at 11:34, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Oct 21, 2023 at 08:43:00AM -0700, Simon Glass wrote:
> > > On Fri, 20 Oct 2023 at 14:53, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > The primary motivation for having a sandbox without LTO build in CI is
> > > > to ensure that we don't have that option break. We now have the ability
> > > > to run tests of specific options being enabled/disabled, so drop the
> > > > parts of CI that build and test that configuration specifically and add
> > > > a build test instead. We still test that "NO_LTO=1" rather than editing
> > > > the config file works via the ftrace tests.
> > > >
> > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > ---
> > > > This creates a small bisectability gap in CI itself, but I think is more
> > > > reasonable than reworking the introduction of
> > > > test/py/tests/test_sandbox_opts.py
> > > >
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >  .azure-pipelines.yml               |  3 ---
> > > >  .gitlab-ci.yml                     | 12 ------------
> > > >  test/py/tests/test_sandbox_opts.py | 10 ++++++++++
> > > >  3 files changed, 10 insertions(+), 15 deletions(-)
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > Q below
> > >
> > > >
> > > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > > index 6f91553e8613..65533b36dde4 100644
> > > > --- a/.azure-pipelines.yml
> > > > +++ b/.azure-pipelines.yml
> > > > @@ -287,9 +287,6 @@ stages:
> > > >          sandbox64_clang:
> > > >            TEST_PY_BD: "sandbox64"
> > > >            OVERRIDE: "-O clang-16"
> > > > -        sandbox_nolto:
> > > > -          TEST_PY_BD: "sandbox"
> > > > -          BUILD_ENV: "NO_LTO=1"
> > > >          sandbox_spl:
> > > >            TEST_PY_BD: "sandbox_spl"
> > > >            TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl"
> > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > > > index 6decdfdee334..97c964fb8079 100644
> > > > --- a/.gitlab-ci.yml
> > > > +++ b/.gitlab-ci.yml
> > > > @@ -258,12 +258,6 @@ sandbox with clang test.py:
> > > >      OVERRIDE: "-O clang-16"
> > > >    <<: *buildman_and_testpy_dfn
> > > >
> > > > -sandbox without LTO test.py:
> > > > -  variables:
> > > > -    TEST_PY_BD: "sandbox"
> > > > -    BUILD_ENV: "NO_LTO=1"
> > > > -  <<: *buildman_and_testpy_dfn
> > > > -
> > > >  sandbox64 test.py:
> > > >    variables:
> > > >      TEST_PY_BD: "sandbox64"
> > > > @@ -275,12 +269,6 @@ sandbox64 with clang test.py:
> > > >      OVERRIDE: "-O clang-16"
> > > >    <<: *buildman_and_testpy_dfn
> > > >
> > > > -sandbox64 without LTO test.py:
> > > > -  variables:
> > > > -    TEST_PY_BD: "sandbox64"
> > > > -    BUILD_ENV: "NO_LTO=1"
> > > > -  <<: *buildman_and_testpy_dfn
> > > > -
> > > >  sandbox_spl test.py:
> > > >    variables:
> > > >      TEST_PY_BD: "sandbox_spl"
> > > > diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
> > > > index 91790b3374b4..422b43cb3bc1 100644
> > > > --- a/test/py/tests/test_sandbox_opts.py
> > > > +++ b/test/py/tests/test_sandbox_opts.py
> > > > @@ -18,3 +18,13 @@ def test_sandbox_cmdline(u_boot_console):
> > > >      out = util.run_and_log(
> > > >          cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > > >                 '-a', '~CMDLINE', '-o', TMPDIR])
> > > > +
> > > > +@pytest.mark.slow
> > > > +@pytest.mark.boardspec('sandbox')
> > > > +def test_sandbox_lto(u_boot_console):
> > > > +    """Test building sandbox without CONFIG_LTO"""
> > > > +    cons = u_boot_console
> > > > +
> > > > +    out = util.run_and_log(
> > > > +        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > > > +               '-a', '~LTO', '-o', TMPDIR])
> > >
> > > Don't you need sandbox64 here?
> >
> > No, I don't think it's providing further value to just build sandbox64
> > without LTO. I'm also not 100% sure this patch is really needed in that we're
> > trying to test for "did we disable LTO via make arguments" and in turn
> > only the ftrace test really catches that, as it will fail if we do have
> > LTO enabled, yes?
> 
> Really the test is to make sure that sandbox builds without LTO. With
> the build time being. For development, LTO serves no useful purpose
> and just triples the incremental build time.
> 
> I suggest we keep sandbox64 just to prevent a regression on non-LTO,
> but I suspect it is unlikely to happen in practice.

I don't follow you here. This adds the build time test for disabling
LTO, as that's at least an option for sandbox (other platforms require
it because otherwise we won't fit on the hardware). I'm mainly not sure
if this test is right in that what we really want to know is "does
NO_LTO=1" pass things as expected, and the ftrace test covers that.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Simon Glass]

Hi Tom,

On Mon, 23 Oct 2023 at 06:37, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Oct 23, 2023 at 12:05:34AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sat, 21 Oct 2023 at 11:34, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, Oct 21, 2023 at 08:43:00AM -0700, Simon Glass wrote:
> > > > On Fri, 20 Oct 2023 at 14:53, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > The primary motivation for having a sandbox without LTO build in CI is
> > > > > to ensure that we don't have that option break. We now have the ability
> > > > > to run tests of specific options being enabled/disabled, so drop the
> > > > > parts of CI that build and test that configuration specifically and add
> > > > > a build test instead. We still test that "NO_LTO=1" rather than editing
> > > > > the config file works via the ftrace tests.
> > > > >
> > > > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > > > ---
> > > > > This creates a small bisectability gap in CI itself, but I think is more
> > > > > reasonable than reworking the introduction of
> > > > > test/py/tests/test_sandbox_opts.py
> > > > >
> > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >  .azure-pipelines.yml               |  3 ---
> > > > >  .gitlab-ci.yml                     | 12 ------------
> > > > >  test/py/tests/test_sandbox_opts.py | 10 ++++++++++
> > > > >  3 files changed, 10 insertions(+), 15 deletions(-)
> > > >
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > Q below
> > > >
> > > > >
> > > > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > > > index 6f91553e8613..65533b36dde4 100644
> > > > > --- a/.azure-pipelines.yml
> > > > > +++ b/.azure-pipelines.yml
> > > > > @@ -287,9 +287,6 @@ stages:
> > > > >          sandbox64_clang:
> > > > >            TEST_PY_BD: "sandbox64"
> > > > >            OVERRIDE: "-O clang-16"
> > > > > -        sandbox_nolto:
> > > > > -          TEST_PY_BD: "sandbox"
> > > > > -          BUILD_ENV: "NO_LTO=1"
> > > > >          sandbox_spl:
> > > > >            TEST_PY_BD: "sandbox_spl"
> > > > >            TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl"
> > > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > > > > index 6decdfdee334..97c964fb8079 100644
> > > > > --- a/.gitlab-ci.yml
> > > > > +++ b/.gitlab-ci.yml
> > > > > @@ -258,12 +258,6 @@ sandbox with clang test.py:
> > > > >      OVERRIDE: "-O clang-16"
> > > > >    <<: *buildman_and_testpy_dfn
> > > > >
> > > > > -sandbox without LTO test.py:
> > > > > -  variables:
> > > > > -    TEST_PY_BD: "sandbox"
> > > > > -    BUILD_ENV: "NO_LTO=1"
> > > > > -  <<: *buildman_and_testpy_dfn
> > > > > -
> > > > >  sandbox64 test.py:
> > > > >    variables:
> > > > >      TEST_PY_BD: "sandbox64"
> > > > > @@ -275,12 +269,6 @@ sandbox64 with clang test.py:
> > > > >      OVERRIDE: "-O clang-16"
> > > > >    <<: *buildman_and_testpy_dfn
> > > > >
> > > > > -sandbox64 without LTO test.py:
> > > > > -  variables:
> > > > > -    TEST_PY_BD: "sandbox64"
> > > > > -    BUILD_ENV: "NO_LTO=1"
> > > > > -  <<: *buildman_and_testpy_dfn
> > > > > -
> > > > >  sandbox_spl test.py:
> > > > >    variables:
> > > > >      TEST_PY_BD: "sandbox_spl"
> > > > > diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
> > > > > index 91790b3374b4..422b43cb3bc1 100644
> > > > > --- a/test/py/tests/test_sandbox_opts.py
> > > > > +++ b/test/py/tests/test_sandbox_opts.py
> > > > > @@ -18,3 +18,13 @@ def test_sandbox_cmdline(u_boot_console):
> > > > >      out = util.run_and_log(
> > > > >          cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > > > >                 '-a', '~CMDLINE', '-o', TMPDIR])
> > > > > +
> > > > > +@pytest.mark.slow
> > > > > +@pytest.mark.boardspec('sandbox')
> > > > > +def test_sandbox_lto(u_boot_console):
> > > > > +    """Test building sandbox without CONFIG_LTO"""
> > > > > +    cons = u_boot_console
> > > > > +
> > > > > +    out = util.run_and_log(
> > > > > +        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > > > > +               '-a', '~LTO', '-o', TMPDIR])
> > > >
> > > > Don't you need sandbox64 here?
> > >
> > > No, I don't think it's providing further value to just build sandbox64
> > > without LTO. I'm also not 100% sure this patch is really needed in that we're
> > > trying to test for "did we disable LTO via make arguments" and in turn
> > > only the ftrace test really catches that, as it will fail if we do have
> > > LTO enabled, yes?
> >
> > Really the test is to make sure that sandbox builds without LTO. With
> > the build time being. For development, LTO serves no useful purpose
> > and just triples the incremental build time.
> >
> > I suggest we keep sandbox64 just to prevent a regression on non-LTO,
> > but I suspect it is unlikely to happen in practice.
>
> I don't follow you here. This adds the build time test for disabling
> LTO, as that's at least an option for sandbox (other platforms require
> it because otherwise we won't fit on the hardware). I'm mainly not sure
> if this test is right in that what we really want to know is "does
> NO_LTO=1" pass things as expected, and the ftrace test covers that.

OK, I see. Well let's run with it and see how we go. I have wanted to
have these sorts of tests for a while, so we can check combinations
that are not in common use.

BTW buildman supports -L which disabled LTO using the NO_LTO=1 option

Regards,
Simon

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

On Mon, Oct 23, 2023 at 10:13:52AM -0700, Simon Glass wrote:

[snip]
> BTW buildman supports -L which disabled LTO using the NO_LTO=1 option

I worry about putting sandbox-specific flags in buildman.  Outside of
sandbox, targets that enable LTO require LTO, just like any other CONFIG
option.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Simon Glass]

Hi Tom,

On Mon, 23 Oct 2023 at 10:28, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Oct 23, 2023 at 10:13:52AM -0700, Simon Glass wrote:
>
> [snip]
> > BTW buildman supports -L which disabled LTO using the NO_LTO=1 option
>
> I worry about putting sandbox-specific flags in buildman.  Outside of
> sandbox, targets that enable LTO require LTO, just like any other CONFIG
> option.

Some problems with LTO and why I don't normally develop with it enabled:

- build time
- code moves around all over the place so it is hard to compare size growth

At least for my IDE flow, I use -L in most cases. Yes there are some
boards which won't fit without LTO, but I don't see them much.

So this is mostly a dev convenience / productivity tool.

Regards,
Simon

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

On Tue, Oct 24, 2023 at 11:02:06AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 23 Oct 2023 at 10:28, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Oct 23, 2023 at 10:13:52AM -0700, Simon Glass wrote:
> >
> > [snip]
> > > BTW buildman supports -L which disabled LTO using the NO_LTO=1 option
> >
> > I worry about putting sandbox-specific flags in buildman.  Outside of
> > sandbox, targets that enable LTO require LTO, just like any other CONFIG
> > option.
> 
> Some problems with LTO and why I don't normally develop with it enabled:
> 
> - build time
> - code moves around all over the place so it is hard to compare size growth
> 
> At least for my IDE flow, I use -L in most cases. Yes there are some
> boards which won't fit without LTO, but I don't see them much.
> 
> So this is mostly a dev convenience / productivity tool.

Yes, it does take longer to link.  And yes, a more complex optimization
does make some size tracking harder to understand (since growth or
shrinkage allows for different optimizations to be made around it). But
everything in configs/ that enables LTO needs LTO.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Simon Glass]

Hi Tom,

On Tue, 24 Oct 2023 at 11:07, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Oct 24, 2023 at 11:02:06AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 23 Oct 2023 at 10:28, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Oct 23, 2023 at 10:13:52AM -0700, Simon Glass wrote:
> > >
> > > [snip]
> > > > BTW buildman supports -L which disabled LTO using the NO_LTO=1 option
> > >
> > > I worry about putting sandbox-specific flags in buildman.  Outside of
> > > sandbox, targets that enable LTO require LTO, just like any other CONFIG
> > > option.
> >
> > Some problems with LTO and why I don't normally develop with it enabled:
> >
> > - build time
> > - code moves around all over the place so it is hard to compare size growth
> >
> > At least for my IDE flow, I use -L in most cases. Yes there are some
> > boards which won't fit without LTO, but I don't see them much.
> >
> > So this is mostly a dev convenience / productivity tool.
>
> Yes, it does take longer to link.  And yes, a more complex optimization
> does make some size tracking harder to understand (since growth or
> shrinkage allows for different optimizations to be made around it). But
> everything in configs/ that enables LTO needs LTO.

I thought we were planning to enable it for all of ARM, though?
Clearly most of those boards don't *need* it.

Regards,
Simon

Re: [v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]

On Tue, Oct 24, 2023 at 02:39:49PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 24 Oct 2023 at 11:07, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Oct 24, 2023 at 11:02:06AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 23 Oct 2023 at 10:28, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Oct 23, 2023 at 10:13:52AM -0700, Simon Glass wrote:
> > > >
> > > > [snip]
> > > > > BTW buildman supports -L which disabled LTO using the NO_LTO=1 option
> > > >
> > > > I worry about putting sandbox-specific flags in buildman.  Outside of
> > > > sandbox, targets that enable LTO require LTO, just like any other CONFIG
> > > > option.
> > >
> > > Some problems with LTO and why I don't normally develop with it enabled:
> > >
> > > - build time
> > > - code moves around all over the place so it is hard to compare size growth
> > >
> > > At least for my IDE flow, I use -L in most cases. Yes there are some
> > > boards which won't fit without LTO, but I don't see them much.
> > >
> > > So this is mostly a dev convenience / productivity tool.
> >
> > Yes, it does take longer to link.  And yes, a more complex optimization
> > does make some size tracking harder to understand (since growth or
> > shrinkage allows for different optimizations to be made around it). But
> > everything in configs/ that enables LTO needs LTO.
> 
> I thought we were planning to enable it for all of ARM, though?
> Clearly most of those boards don't *need* it.

Yes, closer to when it was introduced the ideal was to make it default,
because there are so many "on the edge" platforms, with respect to size
limits.  And, we could possibly make it default now and see what that
does to CI build times too.  But no, today, platforms opt-in for it
because they need it, and the most common need is because of size rather
than speed (or, speed gained through size reduction).

And also yes, that it does make size comparison trickier is another
reason I've not aggressively pushed for it globally.

But yes, I still don't think disabling LTO is a general feature, and only
sandbox has it architecture-wide enabled by default (for testing) and
that -L/--no-lto is not a good generic feature for buildman.

But I too don't wish to argue points around LTO nor gc-sections either
anymore.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]