[PATCH v3 00/32] Tidy up use of CONFIG_CMDLINE
It should be possible to disable CONFIG_CMDLINE and have all commands and related functionality dropped from U-Boot. This is useful when trying to reduce the size of U-Boot.
Recent changes have stopped this from working.
This series repairs the feature for sandbox and adds a test to stop it breaking again.
Note that quite a lot of functionality is lost if CONFIG_CMDLINE is disabled, e.g. networking and most booting options. Further work is needed to make the option more generally useful.
Changes in v3: - Reword commit message slightly - Add an explation as to why this patch is here - Fix 'ut' typo - Just drop the condition instead - Add new patch to rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR - Add new patch with a Kconfig for command-line entry - Add new patch with a Kconfig for command-line entry - Reorder the Kconfig options a little - Add new patch to tidy up semicolon after command macros - Rebase on Tom's LONGHELP series - Correct 'of' typo
Changes in v2: - Move AUTOBOOT_USE_MENUKEY under AUTOBOOT - Change this to use a Kconfig dependency instead of code failing - Adjust the depends on the commands, instead of EFI_LOADER - Add new patch to update EFI_LOADER to depend on DM_ETH - Add new patch to drop discarding of command linker-lists - Add new patch to unify the U_BOOT_ENV_LOCATION conditions
Simon Glass (32): buildman: Use oldconfig when adjusting the config bootstd: Correct dependencies on CMDLINE autoboot: Correct dependencies on CMDLINE cmd: Add a few more dependencies on CMDLINE test: Make UNIT_TEST depend on CMDLINE sifive: Drop an unnecessary #ifdef fastboot: Declare a dependency on CMDLINE cli: Always build cli_getch cmd: Use an #ifdef around run_commandf() Move bootmenu_conv_key() into its own file pxe: Depend on CMDLINE env: Split out non-command code into a new file console: Move SYS_PBSIZE into common/ bootm: Allow building when cleanup functions are missing fdt: Move working_fdt into fdt_support net: Depend on CONFIG_CMDLINE log: Allow use without CONFIG_CMDLINE video: Allow use without CONFIG_CMDLINE video: Dont require the font command efi: Make commands depend on CMDLINE efi: Rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR efi: Update EFI_LOADER to depend on DM_ETH cli: Split command-line-editing into its own file Add a new Kconfig for command-line entry Add a new Kconfig for command-line history sandbox: Disable CONFIG_DISTRO_DEFAULTS cmd: Make all commands depend on CMDLINE sandbox: Avoid requiring cmdline arm: x86: Drop discarding of command linker-lists mmc: env: Unify the U_BOOT_ENV_LOCATION conditions treewide: Tidy up semicolon after command macros sandbox: Add a test for disabling CONFIG_CMDLINE
arch/Kconfig | 11 +- arch/arm/cpu/u-boot.lds | 3 - arch/arm/lib/bootm.c | 2 + arch/x86/cpu/u-boot-64.lds | 4 - arch/x86/cpu/u-boot-spl.lds | 4 - arch/x86/cpu/u-boot.lds | 4 - board/freescale/common/vid.c | 2 +- board/xilinx/common/fru.c | 2 +- board/xilinx/versal/cmds.c | 2 +- board/xilinx/zynqmp/cmds.c | 2 +- boot/Kconfig | 58 ++-- boot/bootm.c | 10 +- boot/fdt_support.c | 5 + cmd/Kconfig | 50 ++-- cmd/Makefile | 2 +- cmd/btrfs.c | 2 +- cmd/eeprom.c | 2 +- cmd/ext2.c | 4 +- cmd/fdt.c | 5 - cmd/fs.c | 8 +- cmd/nvedit.c | 122 +-------- cmd/pinmux.c | 2 +- cmd/qfw.c | 2 +- common/Kconfig | 12 + common/Makefile | 10 +- common/cli.c | 2 + common/cli_cread.c | 407 +++++++++++++++++++++++++++++ common/cli_getch.c | 1 + common/cli_readline.c | 384 +-------------------------- common/log.c | 4 +- common/menu.c | 40 --- common/menu_key.c | 49 ++++ 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 - drivers/fastboot/Kconfig | 1 + drivers/video/Kconfig | 2 +- drivers/video/console_truetype.c | 4 + env/Makefile | 1 + env/env_set.c | 132 ++++++++++ env/mmc.c | 2 +- include/bootm.h | 15 +- include/cli.h | 31 +++ include/command.h | 2 +- include/env_internal.h | 23 ++ include/k210/pll.h | 2 +- lib/efi_loader/Kconfig | 7 +- lib/efi_loader/Makefile | 2 +- net/Kconfig | 1 + test/Kconfig | 1 + test/py/tests/test_sandbox_opts.py | 20 ++ tools/buildman/builder.py | 2 +- tools/buildman/builderthread.py | 6 + tools/buildman/func_test.py | 4 +- 57 files changed, 833 insertions(+), 648 deletions(-) create mode 100644 common/cli_cread.c create mode 100644 common/menu_key.c create mode 100644 env/env_set.c create mode 100644 test/py/tests/test_sandbox_opts.py
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 ---
(no changes since v1)
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
With recent changes over the last few years in boot/Kconfig it is no-longer possible to disable CMDLINE. It results in various link errors because some options which require CMDLINE are enabled regardless of whether it is available.
Add the necessary conditions to fix this.
Note that it would be better to have all commands depend on CMDLINE, but that is extremely difficult at present, since some functions use CMD_xxx to enable feature xxx. For example networking and environment have a number of problems to tease apart.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Reword commit message slightly
boot/Kconfig | 19 ++++++++++++------- lib/efi_loader/Kconfig | 1 + 2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index a01e6cb8aafe..f74ac7e9cc72 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -342,6 +342,7 @@ endif # FIT
config PXE_UTILS bool + depends on CMDLINE select MENU help Utilities for parsing PXE file formats. @@ -357,7 +358,7 @@ config BOOT_DEFAULTS select CMD_PART if PARTITIONS select CMD_DHCP if CMD_NET select CMD_PING if CMD_NET - select CMD_PXE if CMD_NET + select CMD_PXE if CMDLINE && CMD_NET select SUPPORT_RAW_INITRD select ENV_VARS_UBOOT_CONFIG select CMD_BOOTI if ARM64 @@ -461,6 +462,7 @@ config BOOTMETH_GLOBAL
config BOOTMETH_CROS bool "Bootdev support for Chromium OS" + depends on CMDLINE depends on X86 || ARM || SANDBOX default y if !ARM select EFI_PARTITION @@ -475,6 +477,7 @@ config BOOTMETH_CROS
config BOOTMETH_EXTLINUX bool "Bootdev support for extlinux boot" + depends on CMDLINE select PXE_UTILS default y help @@ -490,7 +493,7 @@ config BOOTMETH_EXTLINUX
config BOOTMETH_EXTLINUX_PXE bool "Bootdev support for extlinux boot over network" - depends on CMD_PXE && CMD_NET && DM_ETH + depends on CMDLINE && CMD_PXE && CMD_NET && DM_ETH default y help Enables support for extlinux boot using bootdevs. This makes the @@ -504,7 +507,7 @@ config BOOTMETH_EXTLINUX_PXE
config BOOTMETH_EFILOADER bool "Bootdev support for EFI boot" - depends on EFI_LOADER + depends on EFI_LOADER && CMDLINE default y help Enables support for EFI boot using bootdevs. This makes the @@ -536,10 +539,10 @@ config BOOTMETH_VBE
config BOOTMETH_DISTRO bool # Options needed to boot any 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_SCRIPT if CMDLINE # E.g. Armbian uses scripts + select BOOTMETH_EXTLINUX if CMDLINE # E.g. Debian uses these + select BOOTMETH_EXTLINUX_PXE if CMDLINE && CMD_PXE && CMD_NET && DM_ETH + select BOOTMETH_EFILOADER if CMDLINE && EFI_LOADER # E.g. Ubuntu uses this
config SPL_BOOTMETH_VBE bool "Bootdev support for Verified Boot for Embedded (SPL)" @@ -663,6 +666,7 @@ config BOOTMETH_SANDBOX
config BOOTMETH_SCRIPT bool "Bootdev support for U-Boot scripts" + depends on CMDLINE default y if BOOTSTD_FULL select HUSH_PARSER help @@ -777,6 +781,7 @@ endmenu # Boot images
config DISTRO_DEFAULTS bool "(deprecated) Script-based booting of Linux distributions" + depends on CMDLINE select BOOT_DEFAULTS select AUTO_COMPLETE select CMDLINE_EDITING diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index d20aaab6dba4..621ed5e5b0fb 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -34,6 +34,7 @@ if EFI_LOADER
config CMD_BOOTEFI_BOOTMGR bool "UEFI Boot Manager" + depends on CMDLINE default y select BOOTMETH_GLOBAL if BOOTSTD help
On Mon, Oct 16, 2023 at 04:27:53PM -0600, Simon Glass wrote:
With recent changes over the last few years in boot/Kconfig it is no-longer possible to disable CMDLINE. It results in various link errors because some options which require CMDLINE are enabled regardless of whether it is available.
Add the necessary conditions to fix this.
Note that it would be better to have all commands depend on CMDLINE, but that is extremely difficult at present, since some functions use CMD_xxx to enable feature xxx. For example networking and environment have a number of problems to tease apart.
Signed-off-by: Simon Glass sjg@chromium.org
Since you later rework things to tease apart most, if not all of this, please split out the patches which tease things apart in to their own series so that the rest of this becomes reviewable and also reasonable looking.
Hi Tom,
On Tue, 17 Oct 2023 at 07:32, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:27:53PM -0600, Simon Glass wrote:
With recent changes over the last few years in boot/Kconfig it is no-longer possible to disable CMDLINE. It results in various link errors because some options which require CMDLINE are enabled regardless of whether it is available.
Add the necessary conditions to fix this.
Note that it would be better to have all commands depend on CMDLINE, but that is extremely difficult at present, since some functions use CMD_xxx to enable feature xxx. For example networking and environment have a number of problems to tease apart.
Signed-off-by: Simon Glass sjg@chromium.org
Since you later rework things to tease apart most, if not all of this, please split out the patches which tease things apart in to their own series so that the rest of this becomes reviewable and also reasonable looking.
At this point I fear I have lost track of things, since there is just too much going on. Just in this version I have dealt with:
- some EFI stuff (which needs tweaking) - teasing apart the CLI handling (which apparently doesn't go far enough?)
Before this version I already looked at environment (which you say doesn't go far enough, and fair enough, but please review the code as sent, not what you wish it was).
Perhaps you could apply whatever seems reasonable and then I can take another look? Or suggest some small changes that would allow progress to be made.
But do note that supporting booting without CONFIG_CMDLINE is a large effort, the subject of 5-10 series, perhaps more. It is not my intention to undertake that in this series, or before this series lands
The problem, also, is that without this series, it isn't possible to start that work. With this series, one can look at what code is missing and find a way to call boot functions (for example) bypassing the command line. Once you get it booting you are good. Without this series, you don't even know where to look, since calls to the command line happen silently.
So I think this series is a big step forward and urge another look, please.
Regards, Simon
On Tue, Oct 17, 2023 at 09:30:30PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 17 Oct 2023 at 07:32, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:27:53PM -0600, Simon Glass wrote:
With recent changes over the last few years in boot/Kconfig it is no-longer possible to disable CMDLINE. It results in various link errors because some options which require CMDLINE are enabled regardless of whether it is available.
Add the necessary conditions to fix this.
Note that it would be better to have all commands depend on CMDLINE, but that is extremely difficult at present, since some functions use CMD_xxx to enable feature xxx. For example networking and environment have a number of problems to tease apart.
Signed-off-by: Simon Glass sjg@chromium.org
Since you later rework things to tease apart most, if not all of this, please split out the patches which tease things apart in to their own series so that the rest of this becomes reviewable and also reasonable looking.
At this point I fear I have lost track of things, since there is just too much going on.
Yes, there is too much going on, which is why I wanted you to split this up further. I think we both agree that's a problem with this series.
Just in this version I have dealt with:
- some EFI stuff (which needs tweaking)
- teasing apart the CLI handling (which apparently doesn't go far enough?)
I don't think it's fair to call out EFI here, it's fairly well behaved and no worse / oddly using CONFIG symbols than video or networking or just about everything else is.
Before this version I already looked at environment (which you say doesn't go far enough, and fair enough, but please review the code as sent, not what you wish it was).
Well, that's just it. I did, and I wasn't entirely sure how you were splitting things was right. There was more stuff that either didn't belong or should also have been moved, it wasn't clear. The split I'm suggesting there should make it clear.
Perhaps you could apply whatever seems reasonable and then I can take another look? Or suggest some small changes that would allow progress to be made.
I guess I'll see what I can make some sense out of, yes.
But do note that supporting booting without CONFIG_CMDLINE is a large effort, the subject of 5-10 series, perhaps more. It is not my intention to undertake that in this series, or before this series lands
Well, one of my issues here, especially with the idea of cycling back and cleaning things up later is that I kind of assume you had intended to do that in some places already, having done the good and important task of breaking up the old common directory.
The problem, also, is that without this series, it isn't possible to start that work. With this series, one can look at what code is missing and find a way to call boot functions (for example) bypassing the command line. Once you get it booting you are good. Without this series, you don't even know where to look, since calls to the command line happen silently.
That's where I disagree. Step one is working towards being able to build with CMDLINE disabled, and doing it in small reviewable chunks. This is one big series that gives you "can build with CMDLINE disabled", but while I'm usually not a fan of the Linux kernel's strict "break it down per subsystem", I'd be a lot happier here, and we'd have quicker progress too I bet, if instead of 1 30 part series we had 5 or 6 smaller series.
So I think this series is a big step forward and urge another look, please.
That's what I did with v3 yesterday and why I had a bunch of other feedback.
But, yes, I'll take some parts of v3 and see what I can come up with.
On Mon, Oct 16, 2023 at 04:27:53PM -0600, Simon Glass wrote:
With recent changes over the last few years in boot/Kconfig it is no-longer possible to disable CMDLINE. It results in various link errors because some options which require CMDLINE are enabled regardless of whether it is available.
Add the necessary conditions to fix this.
Note that it would be better to have all commands depend on CMDLINE, but that is extremely difficult at present, since some functions use CMD_xxx to enable feature xxx. For example networking and environment have a number of problems to tease apart.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
- Reword commit message slightly
boot/Kconfig | 19 ++++++++++++------- lib/efi_loader/Kconfig | 1 + 2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index a01e6cb8aafe..f74ac7e9cc72 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -342,6 +342,7 @@ endif # FIT
config PXE_UTILS bool
- depends on CMDLINE select MENU help Utilities for parsing PXE file formats.
So, this part here is because of CONFIG_SYS_PBSIZE, which I think we can / should solve another way.
Hi Tom,
On Wed, 18 Oct 2023 at 08:42, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:27:53PM -0600, Simon Glass wrote:
With recent changes over the last few years in boot/Kconfig it is no-longer possible to disable CMDLINE. It results in various link errors because some options which require CMDLINE are enabled regardless of whether it is available.
Add the necessary conditions to fix this.
Note that it would be better to have all commands depend on CMDLINE, but that is extremely difficult at present, since some functions use CMD_xxx to enable feature xxx. For example networking and environment have a number of problems to tease apart.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
- Reword commit message slightly
boot/Kconfig | 19 ++++++++++++------- lib/efi_loader/Kconfig | 1 + 2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index a01e6cb8aafe..f74ac7e9cc72 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -342,6 +342,7 @@ endif # FIT
config PXE_UTILS bool
depends on CMDLINE select MENU help Utilities for parsing PXE file formats.So, this part here is because of CONFIG_SYS_PBSIZE, which I think we can / should solve another way.
Perhaps it would help if you just clarify your goal.
My goal with this series is to allow CMDLINE to be disabled without build errors, so we can start the work booting via bootstd (with CMDLINE disabled).
This isn't a straight-line activity. It involves changes that will become redundant later, simply because I haven't written the 100s of patches needed to figure it all out. Even if I did it would be unreviewable and require lots of rework based on feedback.
Regards, Simon
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 ---
(no changes since v2)
Changes in v2: - Move AUTOBOOT_USE_MENUKEY under AUTOBOOT
boot/Kconfig | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index f74ac7e9cc72..6461f7ebd040 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -1167,14 +1167,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. @@ -1196,9 +1198,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. @@ -1207,7 +1211,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 @@ -1223,7 +1226,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. @@ -1250,7 +1252,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 @@ -1262,7 +1264,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 @@ -1274,7 +1276,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". @@ -1283,7 +1285,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 @@ -1291,7 +1293,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. @@ -1322,9 +1324,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. @@ -1339,6 +1344,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
Add this to some more commands to avoid build errors with sandbox.
Note that this is a temporary solution to expose more problems. A later patch puts these behind an 'if CMDLINE'
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Add an explation as to why this patch is here
cmd/Kconfig | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5bc0a92d57ad..00a7f84bce99 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -231,6 +231,7 @@ menu "Boot commands"
config CMD_BOOTD bool "bootd" + depends on CMDLINE default y help Run the command stored in the environment "bootcmd", i.e. @@ -420,6 +421,7 @@ source lib/efi_selftest/Kconfig
config CMD_BOOTMENU bool "bootmenu" + depends on CMDLINE select MENU select CHARSET help @@ -486,6 +488,7 @@ config CMD_GO
config CMD_RUN bool "run" + depends on CMDLINE default y help Run the command in the given environment variable. @@ -576,6 +579,7 @@ menu "Environment commands"
config CMD_ASKENV bool "ask for env variable" + depends on CMDLINE help Ask for environment variable
@@ -2132,6 +2136,7 @@ config CMD_EFICONFIG
config CMD_EXCEPTION bool "exception - raise exception" + depends on CMDLINE depends on ARM || RISCV || SANDBOX || X86 help Enable the 'exception' command which allows to raise an exception. @@ -2238,6 +2243,7 @@ config CMD_SYSBOOT
config CMD_QFW bool "qfw" + depends on CMDLINE select QFW help This provides access to the QEMU firmware interface. The main
On Mon, Oct 16, 2023 at 04:27:55PM -0600, Simon Glass wrote:
Add this to some more commands to avoid build errors with sandbox.
Note that this is a temporary solution to expose more problems. A later patch puts these behind an 'if CMDLINE'
Signed-off-by: Simon Glass sjg@chromium.org
We're doing a bunch of churn here that I'd rather just get done at once, make the current #27 which guards all commands with if CMDLINE happen earlier in the series instead.
Hi Tom,
On Tue, 17 Oct 2023 at 07:32, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:27:55PM -0600, Simon Glass wrote:
Add this to some more commands to avoid build errors with sandbox.
Note that this is a temporary solution to expose more problems. A later patch puts these behind an 'if CMDLINE'
Signed-off-by: Simon Glass sjg@chromium.org
We're doing a bunch of churn here that I'd rather just get done at once, make the current #27 which guards all commands with if CMDLINE happen earlier in the series instead.
Yes I did consider this when reviewing this patch for v3, but got build-bisect failures. I cannot move #27 earlier, since all the builds fail. Allowing #27 to pass is the point of this series, in fact.
If we can make progress on the thrust of this series then I will see if I can somehow reorder some other patches to drop this patch, or move it later, or cut it down, or something else...
I also don't agree with 'bunch of'. This is 6 lines, which enables another 20 patches which clean everything up. Are there other patches with churn?
Regards, Simon
On Tue, Oct 17, 2023 at 09:30:46PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 17 Oct 2023 at 07:32, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:27:55PM -0600, Simon Glass wrote:
Add this to some more commands to avoid build errors with sandbox.
Note that this is a temporary solution to expose more problems. A later patch puts these behind an 'if CMDLINE'
Signed-off-by: Simon Glass sjg@chromium.org
We're doing a bunch of churn here that I'd rather just get done at once, make the current #27 which guards all commands with if CMDLINE happen earlier in the series instead.
Yes I did consider this when reviewing this patch for v3, but got build-bisect failures. I cannot move #27 earlier, since all the builds fail. Allowing #27 to pass is the point of this series, in fact.
Yes, I suspect a number of the other patches need to be shuffled around as well.
If we can make progress on the thrust of this series then I will see if I can somehow reorder some other patches to drop this patch, or move it later, or cut it down, or something else...
I also don't agree with 'bunch of'. This is 6 lines, which enables another 20 patches which clean everything up. Are there other patches with churn?
Other patches in the series also added if CMDLINE or similar sets of dependencies that then get pulled out in #27.
On 10/17/23 00:27, Simon Glass wrote:
Add this to some more commands to avoid build errors with sandbox.
Note that this is a temporary solution to expose more problems. A later patch puts these behind an 'if CMDLINE'
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
Add an explation as to why this patch is here
cmd/Kconfig | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5bc0a92d57ad..00a7f84bce99 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -231,6 +231,7 @@ menu "Boot commands"
config CMD_BOOTD bool "bootd"
- depends on CMDLINE
Why don't we simply use a single "if" for all commands? Adding the same dependency to 200+ commands does not feel right.
If there is any CONFIG_CMD* that is needed without CMDLINE, then that code should move to lib/ or drivers/.
Best regards
Heinrich
default y help Run the command stored in the environment "bootcmd", i.e. @@ -420,6 +421,7 @@ source lib/efi_selftest/Kconfig
config CMD_BOOTMENU bool "bootmenu"
- depends on CMDLINE select MENU select CHARSET help
@@ -486,6 +488,7 @@ config CMD_GO
config CMD_RUN bool "run"
- depends on CMDLINE default y help Run the command in the given environment variable.
@@ -576,6 +579,7 @@ menu "Environment commands"
config CMD_ASKENV bool "ask for env variable"
- depends on CMDLINE help Ask for environment variable
@@ -2132,6 +2136,7 @@ config CMD_EFICONFIG
config CMD_EXCEPTION bool "exception - raise exception"
- depends on CMDLINE depends on ARM || RISCV || SANDBOX || X86 help Enable the 'exception' command which allows to raise an exception.
@@ -2238,6 +2243,7 @@ config CMD_SYSBOOT
config CMD_QFW bool "qfw"
- depends on CMDLINE select QFW help This provides access to the QEMU firmware interface. The main
Hi Heinrich,
On Wed, 18 Oct 2023 at 07:14, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/17/23 00:27, Simon Glass wrote:
Add this to some more commands to avoid build errors with sandbox.
Note that this is a temporary solution to expose more problems. A later patch puts these behind an 'if CMDLINE'
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
Add an explation as to why this patch is here
cmd/Kconfig | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5bc0a92d57ad..00a7f84bce99 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -231,6 +231,7 @@ menu "Boot commands"
config CMD_BOOTD bool "bootd"
depends on CMDLINEWhy don't we simply use a single "if" for all commands? Adding the same dependency to 200+ commands does not feel right.
If there is any CONFIG_CMD* that is needed without CMDLINE, then that code should move to lib/ or drivers/.
This is here to fix build failures at this point in the series.
One of the later patches does what you suggest (27 I think).
Regards, Simon
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 ---
Changes in v3: - Fix 'ut' typo
test/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/test/Kconfig b/test/Kconfig index 830245b6f9a9..2b4036704f91 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.
This code is normally compiled for sifive, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Drop the condition since it isn't needed.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Sean Anderson seanga2@gmail.com ---
Changes in v3: - Just drop the condition instead
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
On 10/17/23 00:27, Simon Glass wrote:
This code is normally compiled for sifive, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Drop the condition since it isn't needed.
This is not what the patch does. It introduces another condition.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Sean Anderson seanga2@gmail.com
Changes in v3:
Just drop the condition instead
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX
We should be able to compile with and without unit tests on the MAIX boards. Why should CONFIG_SANDBOX have any relevance here?
Why don't we make k210_pll_calc_config() non-static in all cases?
Best regards
Heinrich
TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
On 10/18/23 09:51, Heinrich Schuchardt wrote:
On 10/17/23 00:27, Simon Glass wrote:
This code is normally compiled for sifive, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Drop the condition since it isn't needed.
This is not what the patch does. It introduces another condition.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Sean Anderson seanga2@gmail.com
Changes in v3:
- Just drop the condition instead
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX
We should be able to compile with and without unit tests on the MAIX boards. Why should CONFIG_SANDBOX have any relevance here?
Why don't we make k210_pll_calc_config() non-static in all cases?
U-Boot is smaller when it is static. But the forward declaration can be made unconditionally, as long as it is unused.
--Sean
On Wed, Oct 18, 2023 at 01:23:13PM -0400, Sean Anderson wrote:
On 10/18/23 09:51, Heinrich Schuchardt wrote:
On 10/17/23 00:27, Simon Glass wrote:
This code is normally compiled for sifive, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Drop the condition since it isn't needed.
This is not what the patch does. It introduces another condition.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Sean Anderson seanga2@gmail.com
Changes in v3:
- Just drop the condition instead
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX
We should be able to compile with and without unit tests on the MAIX boards. Why should CONFIG_SANDBOX have any relevance here?
Why don't we make k210_pll_calc_config() non-static in all cases?
U-Boot is smaller when it is static. But the forward declaration can be made unconditionally, as long as it is unused.
I think part of the problem with this patch is confusing what it's doing. The issue here is that we can (and this is good!) and do compile drivers/clk/clk_k210.c for sandbox, so it gets coverity scans and so forth. However, if we build sandbox with UNIT_TEST=n then we get an undefined reference to 'nop'. Because sandbox doesn't define nop(). This header then provides nop for sandbox. But, we can do something clearer, now that this is spelled out.
When CMDLINE is not enabled, this code fails to build. Correct this by adding a condition.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Change this to use a Kconfig dependency instead of code failing
drivers/fastboot/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig index 837c6f1180da..24b100f381fe 100644 --- a/drivers/fastboot/Kconfig +++ b/drivers/fastboot/Kconfig @@ -2,6 +2,7 @@ menu "Fastboot support"
config FASTBOOT bool + depends on CMDLINE imply ANDROID_BOOT_IMAGE imply CMD_FASTBOOT help
This module is used for user input with menus, not just with the command line. Compile it always, so it is available even when CMDLINE is disabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
common/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/common/Makefile b/common/Makefile index cdeadf72026c..b21916f15340 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 obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o
@@ -37,7 +38,7 @@ 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 +obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
endif # !CONFIG_SPL_BUILD
This is not available if CMDLINE is disabled, so add an #ifdef to correct this.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
common/cli.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/common/cli.c b/common/cli.c index 3916a7b10a7d..4d0fea4387f2 100644 --- a/common/cli.c +++ b/common/cli.c @@ -129,6 +129,7 @@ int run_command_list(const char *cmd, int len, int flag) return rcode; }
+#ifdef CONFIG_CMDLINE int run_commandf(const char *fmt, ...) { va_list args; @@ -153,6 +154,7 @@ int run_commandf(const char *fmt, ...) } return run_command(console_buffer, 0); } +#endif /* CMDLINE */
/****************************************************************************/
On Mon, Oct 16, 2023 at 04:28:00PM -0600, Simon Glass wrote:
This is not available if CMDLINE is disabled, so add an #ifdef to correct this.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
common/cli.c | 2 ++ 1 file changed, 2 insertions(+)
NAK, please tease this file apart in to CMDLINE code, and whatever inadvertent utility code is also in there.
This conversion function is used by expo which does not require CMDLINE. The menu feature does require CMDLINE.
Move the function into a separate file so that it can be used even when CMDLINE is not enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
common/Makefile | 2 +- common/cli_getch.c | 1 + common/menu.c | 40 ------------------------------------- common/menu_key.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 41 deletions(-) create mode 100644 common/menu_key.c
diff --git a/common/Makefile b/common/Makefile index b21916f15340..637066ae6682 100644 --- a/common/Makefile +++ b/common/Makefile @@ -8,7 +8,7 @@ ifndef CONFIG_SPL_BUILD obj-y += init/ obj-y += main.o obj-y += exports.o -obj-y += cli_getch.o +obj-y += cli_getch.o menu_key.o obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o
diff --git a/common/cli_getch.c b/common/cli_getch.c index 61d4cb261b81..c3332dc27fae 100644 --- a/common/cli_getch.c +++ b/common/cli_getch.c @@ -8,6 +8,7 @@
#include <common.h> #include <cli.h> +#include <menu.h>
/** * enum cli_esc_state_t - indicates what to do with an escape character diff --git a/common/menu.c b/common/menu.c index b55cf7b99967..844d0ec52af3 100644 --- a/common/menu.c +++ b/common/menu.c @@ -483,46 +483,6 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu, return key; }
-enum bootmenu_key bootmenu_conv_key(int ichar) -{ - enum bootmenu_key key; - - switch (ichar) { - case '\n': - /* enter key was pressed */ - key = BKEY_SELECT; - break; - case CTL_CH('c'): - case '\e': - /* ^C was pressed */ - key = BKEY_QUIT; - break; - case CTL_CH('p'): - key = BKEY_UP; - break; - case CTL_CH('n'): - key = BKEY_DOWN; - break; - case CTL_CH('s'): - key = BKEY_SAVE; - break; - case '+': - key = BKEY_PLUS; - break; - case '-': - key = BKEY_MINUS; - break; - case ' ': - key = BKEY_SPACE; - break; - default: - key = BKEY_NONE; - break; - } - - return key; -} - enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, struct cli_ch_state *cch) { diff --git a/common/menu_key.c b/common/menu_key.c new file mode 100644 index 000000000000..4e9c3b426b0c --- /dev/null +++ b/common/menu_key.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2010-2011 Calxeda, Inc. + * Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. + */ + +#include <common.h> +#include <cli.h> +#include <menu.h> + +enum bootmenu_key bootmenu_conv_key(int ichar) +{ + enum bootmenu_key key; + + switch (ichar) { + case '\n': + /* enter key was pressed */ + key = BKEY_SELECT; + break; + case CTL_CH('c'): + case '\e': + /* ^C was pressed */ + key = BKEY_QUIT; + break; + case CTL_CH('p'): + key = BKEY_UP; + break; + case CTL_CH('n'): + key = BKEY_DOWN; + break; + case CTL_CH('s'): + key = BKEY_SAVE; + break; + case '+': + key = BKEY_PLUS; + break; + case '-': + key = BKEY_MINUS; + break; + case ' ': + key = BKEY_SPACE; + break; + default: + key = BKEY_NONE; + break; + } + + return key; +}
We cannot use PXE or sysboot commands without CONFIG_CMDLINE so add the required condition.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
cmd/Kconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 00a7f84bce99..18be3da972d2 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -2002,6 +2002,7 @@ config CMD_ETHSW
config CMD_PXE bool "pxe" + depends on CMDLINE select PXE_UTILS help Boot image via network using PXE protocol @@ -2237,6 +2238,7 @@ config CMD_SOUND
config CMD_SYSBOOT bool "sysboot" + depends on CMDLINE select PXE_UTILS help Boot image via local extlinux.conf file
It is not possible to set environment variables without having CONFIG_CMD_NVEDIT enabled. When CONFIG_CMDLINE is disabled, we need a way to set variables.
Split the setting code out into its own file, so that env_set() is available even when CONFIG_CMDLINE is not. If it is never called, the code will be dropped at link time.
Update the Makefile rule to only include the env commands when CONFIG_CMD_NVEDIT is enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
cmd/Makefile | 2 +- cmd/nvedit.c | 122 ++----------------------------------- env/Makefile | 1 + env/env_set.c | 132 +++++++++++++++++++++++++++++++++++++++++ include/env_internal.h | 23 +++++++ 5 files changed, 161 insertions(+), 119 deletions(-) create mode 100644 env/env_set.c
diff --git a/cmd/Makefile b/cmd/Makefile index 44db5f22861e..29d7c500c66e 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -245,7 +245,7 @@ endif # !CONFIG_SPL_BUILD obj-$(CONFIG_$(SPL_)CMD_TLV_EEPROM) += tlv_eeprom.o
# core command -obj-y += nvedit.o +obj-$(CONFIG_$(SPL_)CMDLINE) += nvedit.o
obj-$(CONFIG_CMD_BCM_EXT_UTILS) += broadcom/
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index daf1ad37f9be..812d82b157f4 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 @@ -195,108 +181,8 @@ DONE:
return 0; } -#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); -} +#endif /* CONFIG_CMD_GREPENV */
-#ifndef CONFIG_SPL_BUILD static int do_env_set(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { @@ -456,7 +342,7 @@ int do_env_callback(struct cmd_tbl *cmdtp, int flag, int argc, hwalk_r(&env_htab, print_active_callback); return 0; } -#endif +#endif /* CONFIG_CMD_ENV_CALLBACK */
#if defined(CONFIG_CMD_ENV_FLAGS) static int print_static_flags(const char *var_name, const char *flags, @@ -529,7 +415,7 @@ int do_env_flags(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) hwalk_r(&env_htab, print_active_flags); return 0; } -#endif +#endif /* CONFIG_CMD_ENV_FLAGS */
/* * Interactively edit an environment variable @@ -679,7 +565,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/Makefile b/env/Makefile index 673b979fdfa9..5250b6df2cfc 100644 --- a/env/Makefile +++ b/env/Makefile @@ -5,6 +5,7 @@
obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += common.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env_set.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o
diff --git a/env/env_set.c b/env/env_set.c new file mode 100644 index 000000000000..eccbda6a791c --- /dev/null +++ b/env/env_set.c @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) Copyright 2000-2013 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * (C) Copyright 2001 Sysgo Real-Time Solutions, GmbH <www.elinos.com> + * Andreas Heppel aheppel@sysgo.de + * + * Copyright 2011 Freescale Semiconductor, Inc. + */ + +#include <common.h> +#include <command.h> +#include <env.h> +#include <env_internal.h> +#include <malloc.h> +#include <asm/global_data.h> + +DECLARE_GLOBAL_DATA_PTR; + +/* + * 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; +} + +void env_inc_id(void) +{ + env_id++; +} + +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]) { + 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) { + 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 || !varvalue[0]) + return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC); + else + return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC); +} diff --git a/include/env_internal.h b/include/env_internal.h index 6a6949464689..99e358b6d8a5 100644 --- a/include/env_internal.h +++ b/include/env_internal.h @@ -260,6 +260,29 @@ const char *env_fat_get_intf(void); * Return: string of device and partition */ char *env_fat_get_dev_part(void); + +/* + * _do_env_set() - Add / replace / delete an environment variable + * + * This implements the bulk of the 'env set' command: + * + * env set [-f] name [value] + * + * Sets the value of variable <name> + * If <value> is NULL, it removes the variable + * Use the -f flag to overwrite read-only/write-once variables + * + * @flag: CMD_FLAG_... value + * @argc: Number of arguments + * @args: List of arguments + * @env_flag: H_... flags from search.h + * Return: 0 if OK, 1 on failure, or CMD_RET_USAGE for invalid flag + */ +int _do_env_set(int flag, int argc, char *const argv[], int env_flag); + +/** env_inc_id() - Increment the environment ID */ +void env_inc_id(void); + #endif /* DO_DEPS_ONLY */
#endif /* _ENV_INTERNAL_H_ */
On Mon, Oct 16, 2023 at 04:28:03PM -0600, Simon Glass wrote:
It is not possible to set environment variables without having CONFIG_CMD_NVEDIT enabled. When CONFIG_CMDLINE is disabled, we need a way to set variables.
Split the setting code out into its own file, so that env_set() is available even when CONFIG_CMDLINE is not. If it is never called, the code will be dropped at link time.
Update the Makefile rule to only include the env commands when CONFIG_CMD_NVEDIT is enabled.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
cmd/Makefile | 2 +- cmd/nvedit.c | 122 ++----------------------------------- env/Makefile | 1 + env/env_set.c | 132 +++++++++++++++++++++++++++++++++++++++++ include/env_internal.h | 23 +++++++ 5 files changed, 161 insertions(+), 119 deletions(-) create mode 100644 env/env_set.c
This feels like it's partly but not entirely correct. We need to split cmd/nvedit.c in to cmd/env.c and env/nvedit.c instead I think. And keep in mind that SPL + ENV (and so - CMDLINE I believe) does work today.
Hi Tom,
On Tue, 17 Oct 2023 at 07:40, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:03PM -0600, Simon Glass wrote:
It is not possible to set environment variables without having CONFIG_CMD_NVEDIT enabled. When CONFIG_CMDLINE is disabled, we need a way to set variables.
Split the setting code out into its own file, so that env_set() is available even when CONFIG_CMDLINE is not. If it is never called, the code will be dropped at link time.
Update the Makefile rule to only include the env commands when CONFIG_CMD_NVEDIT is enabled.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
cmd/Makefile | 2 +- cmd/nvedit.c | 122 ++----------------------------------- env/Makefile | 1 + env/env_set.c | 132 +++++++++++++++++++++++++++++++++++++++++ include/env_internal.h | 23 +++++++ 5 files changed, 161 insertions(+), 119 deletions(-) create mode 100644 env/env_set.c
This feels like it's partly but not entirely correct. We need to split cmd/nvedit.c in to cmd/env.c and env/nvedit.c instead I think. And keep in mind that SPL + ENV (and so - CMDLINE I believe) does work today.
OK, fine, but can I do it later? That seems like an excellent follow-on but is it essential for the purpose of this already-large series?
Regards, Simon
This relates to printing output and does not need a command line. Move it next to the other console-related options.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
cmd/Kconfig | 5 ----- common/Kconfig | 5 +++++ 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 18be3da972d2..148414f011df 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -84,11 +84,6 @@ config SYS_CBSIZE 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..1ffb055744d9 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -52,6 +52,11 @@ config CONSOLE_RECORD_IN_SIZE The buffer is allocated immediately after the malloc() region is ready.
+config SYS_PBSIZE + int "Buffer size for console output" + default 1024 if ARCH_SUNXI + default 1044 + config DISABLE_CONSOLE bool "Add functionality to disable console completely" help
There are two cleanup functions needed during boot which depend on CMD_BOOTM: bootm_disable_interrupts() and board_quiesce_devices()
Provide static inline versions of these for when commands are not enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
arch/arm/lib/bootm.c | 2 ++ boot/bootm.c | 10 ++++------ include/bootm.h | 15 +++++++++++++-- 3 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index c56285738a26..db8df57cb56e 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR;
static struct tag *params;
+#ifdef CONFIG_CMD_BOOTM __weak void board_quiesce_devices(void) { } +#endif
/** * announce_and_cleanup() - Print message and prepare for kernel boot diff --git a/boot/bootm.c b/boot/bootm.c index 8f96a80d4259..7deae058552a 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -47,9 +47,11 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], struct bootm_headers *images, ulong *os_data, ulong *os_len);
+#ifdef CONFIG_CMD_BOOTM __weak void board_quiesce_devices(void) { } +#endif
#ifdef CONFIG_LMB static void boot_start_lmb(struct bootm_headers *images) @@ -470,12 +472,7 @@ static int bootm_load_os(struct bootm_headers *images, int boot_progress) return 0; }
-/** - * bootm_disable_interrupts() - Disable interrupts in preparation for load/boot - * - * Return: interrupt flag (0 if interrupts were disabled, non-zero if they were - * enabled) - */ +#ifdef CONFIG_CMD_BOOTM ulong bootm_disable_interrupts(void) { ulong iflag; @@ -505,6 +502,7 @@ ulong bootm_disable_interrupts(void) #endif return iflag; } +#endif
#define CONSOLE_ARG "console=" #define NULL_CONSOLE (CONSOLE_ARG "ttynull") diff --git a/include/bootm.h b/include/bootm.h index c3c7336207b1..17c740449efd 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -50,8 +50,6 @@ int bootm_host_load_images(const void *fit, int cfg_noffset); int boot_selected_os(int argc, char *const argv[], int state, struct bootm_headers *images, boot_os_fn *boot_fn);
-ulong bootm_disable_interrupts(void); - /* This is a special function used by booti/bootz */ int bootm_find_images(int flag, int argc, char *const argv[], ulong start, ulong size); @@ -62,6 +60,15 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
void arch_preboot_os(void);
+#ifdef CONFIG_CMD_BOOTM +/** + * bootm_disable_interrupts() - Disable interrupts, stop Ethernet and USB + * + * Return: interrupt flag (0 if interrupts were disabled, non-zero if they were + * enabled) + */ +ulong bootm_disable_interrupts(void); + /* * boards should define this to disable devices when EFI exits from boot * services. @@ -69,6 +76,10 @@ void arch_preboot_os(void); * TODO(sjg@chromium.org>): Update this to use driver model's device_remove(). */ void board_quiesce_devices(void); +#else +static inline ulong bootm_disable_interrupts(void) { return 0; } +static inline void board_quiesce_devices(void) {} +#endif
/** * switch_to_non_secure_mode() - switch to non-secure mode
On Mon, Oct 16, 2023 at 04:28:05PM -0600, Simon Glass wrote:
There are two cleanup functions needed during boot which depend on CMD_BOOTM: bootm_disable_interrupts() and board_quiesce_devices()
Provide static inline versions of these for when commands are not enabled.
Signed-off-by: Simon Glass sjg@chromium.org
NAK, these functions need to be available to boot the OS, regardless of command line existing or not. Unwind things in the other direction please.
Hi Tom,
On Tue, 17 Oct 2023 at 08:02, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:05PM -0600, Simon Glass wrote:
There are two cleanup functions needed during boot which depend on CMD_BOOTM: bootm_disable_interrupts() and board_quiesce_devices()
Provide static inline versions of these for when commands are not enabled.
Signed-off-by: Simon Glass sjg@chromium.org
NAK, these functions need to be available to boot the OS, regardless of command line existing or not. Unwind things in the other direction please.
That won't be a successful strategy. See my other reply on this. I have had this idea since 2016 and have looked at it every now and then. With bootstd it makes more sense since we actually have a framework to boot without the command line. This series enables such work, but cannot include it.
Regards, Simon
On Tue, Oct 17, 2023 at 09:31:04PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 17 Oct 2023 at 08:02, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:05PM -0600, Simon Glass wrote:
There are two cleanup functions needed during boot which depend on CMD_BOOTM: bootm_disable_interrupts() and board_quiesce_devices()
Provide static inline versions of these for when commands are not enabled.
Signed-off-by: Simon Glass sjg@chromium.org
NAK, these functions need to be available to boot the OS, regardless of command line existing or not. Unwind things in the other direction please.
That won't be a successful strategy. See my other reply on this. I have had this idea since 2016 and have looked at it every now and then. With bootstd it makes more sense since we actually have a framework to boot without the command line. This series enables such work, but cannot include it.
I don't understand. The "cleanup" functions here are to ensure that any devices that we've touched are in the state the OS needs them to be. It's not "we ran a command", it's "we loaded something off of USB (or network or MMC or ..)" and so we must have those called.
Hi Tom,
On Wed, 18 Oct 2023 at 06:39, Tom Rini trini@konsulko.com wrote:
On Tue, Oct 17, 2023 at 09:31:04PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 17 Oct 2023 at 08:02, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:05PM -0600, Simon Glass wrote:
There are two cleanup functions needed during boot which depend on CMD_BOOTM: bootm_disable_interrupts() and board_quiesce_devices()
Provide static inline versions of these for when commands are not enabled.
Signed-off-by: Simon Glass sjg@chromium.org
NAK, these functions need to be available to boot the OS, regardless of command line existing or not. Unwind things in the other direction please.
That won't be a successful strategy. See my other reply on this. I have had this idea since 2016 and have looked at it every now and then. With bootstd it makes more sense since we actually have a framework to boot without the command line. This series enables such work, but cannot include it.
I don't understand. The "cleanup" functions here are to ensure that any devices that we've touched are in the state the OS needs them to be. It's not "we ran a command", it's "we loaded something off of USB (or network or MMC or ..)" and so we must have those called.
Right, but they are in with the bootm command code. We could pull them out, but it is only a partialsolution. Really we need another series to allow 'bootm' booting without CMDLINE. In other words, we cannot boot most things without cmdline today. That has been the case before this series and unfortunately this series doesn't change that.
At present we cannot even build without ~CMDLINE - that has regressed. I am trying to patch things up and make it build and add a test.
Regards, Simon
On Thu, Oct 19, 2023 at 07:59:29AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 06:39, Tom Rini trini@konsulko.com wrote:
On Tue, Oct 17, 2023 at 09:31:04PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 17 Oct 2023 at 08:02, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:05PM -0600, Simon Glass wrote:
There are two cleanup functions needed during boot which depend on CMD_BOOTM: bootm_disable_interrupts() and board_quiesce_devices()
Provide static inline versions of these for when commands are not enabled.
Signed-off-by: Simon Glass sjg@chromium.org
NAK, these functions need to be available to boot the OS, regardless of command line existing or not. Unwind things in the other direction please.
That won't be a successful strategy. See my other reply on this. I have had this idea since 2016 and have looked at it every now and then. With bootstd it makes more sense since we actually have a framework to boot without the command line. This series enables such work, but cannot include it.
I don't understand. The "cleanup" functions here are to ensure that any devices that we've touched are in the state the OS needs them to be. It's not "we ran a command", it's "we loaded something off of USB (or network or MMC or ..)" and so we must have those called.
Right, but they are in with the bootm command code. We could pull them
I don't think that's right. For bootm, things are split correctly. I'm not quite sure about booti/bootz, and I can see what bootefi needs to be split more. But bootm is fine.
[snip]
At present we cannot even build without ~CMDLINE - that has regressed. I am trying to patch things up and make it build and add a test.
To be clear here, I prefer build failure in that case over having broken functionality. That way leads to developers expecting something to work that will not. Those prep functions are critical. It's also a moot point because we can have them today with CMDLINE=n.
This can be accessed even when commands are not enabled. Move it into the fdt_support.c file, which is where most of the FDT helpers are.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
boot/fdt_support.c | 5 +++++ cmd/fdt.c | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/boot/fdt_support.c b/boot/fdt_support.c index 5e49078f8c35..6ae7b8e20f65 100644 --- a/boot/fdt_support.c +++ b/boot/fdt_support.c @@ -23,6 +23,11 @@ #include <fdtdec.h> #include <version.h>
+/* + * The working_fdt points to our working flattened device tree. + */ +struct fdt_header *working_fdt; + /** * fdt_getprop_u32_default_node - Return a node's property or a default * diff --git a/cmd/fdt.c b/cmd/fdt.c index 331564c13be9..86e847a41826 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -31,11 +31,6 @@ static int fdt_parse_prop(char *const*newval, int count, char *data, int *len); static int fdt_print(const char *pathp, char *prop, int depth); static int is_printable_string(const void *data, int len);
-/* - * The working_fdt points to our working flattened device tree. - */ -struct fdt_header *working_fdt; - static void set_working_fdt_addr_quiet(ulong addr) { void *buf;
At present it isn't possible to use networking without the command line enabled. Add this as a condition.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ramon Fried rfried.dev@gmail.com ---
(no changes since v1)
cmd/Kconfig | 1 + net/Kconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 148414f011df..c6ea5c860e33 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1694,6 +1694,7 @@ if NET
menuconfig CMD_NET bool "Network commands" + depends on CMDLINE default y imply NETDEVICES
diff --git a/net/Kconfig b/net/Kconfig index 4215889127c9..25d494e1db46 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -4,6 +4,7 @@
menuconfig NET bool "Networking support" + depends on CMDLINE default y
if NET
On Mon, Oct 16, 2023 at 04:28:07PM -0600, Simon Glass wrote:
At present it isn't possible to use networking without the command line enabled. Add this as a condition.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
cmd/Kconfig | 1 + net/Kconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 148414f011df..c6ea5c860e33 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1694,6 +1694,7 @@ if NET
menuconfig CMD_NET bool "Network commands"
- depends on CMDLINE default y imply NETDEVICES
diff --git a/net/Kconfig b/net/Kconfig index 4215889127c9..25d494e1db46 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -4,6 +4,7 @@
menuconfig NET bool "Networking support"
- depends on CMDLINE default y
if NET
Please unwind this in the other direction. What in network is depending on cmdline in a non-trivial way?
Hi Tom,
On Tue, 17 Oct 2023 at 08:04, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:07PM -0600, Simon Glass wrote:
At present it isn't possible to use networking without the command line enabled. Add this as a condition.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
cmd/Kconfig | 1 + net/Kconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 148414f011df..c6ea5c860e33 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1694,6 +1694,7 @@ if NET
menuconfig CMD_NET bool "Network commands"
depends on CMDLINE default y imply NETDEVICESdiff --git a/net/Kconfig b/net/Kconfig index 4215889127c9..25d494e1db46 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -4,6 +4,7 @@
menuconfig NET bool "Networking support"
depends on CMDLINE default yif NET
Please unwind this in the other direction. What in network is depending on cmdline in a non-trivial way?
I can look at this as a follow-on, but I need a starting point. See my other comment.
Regards, Simon
When CONFIG_SYS_CBSIZE is not used we need an alternative. For logging it seems that CONFIG_SYS_PBSIZE is a better choice anyway, so update this.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
common/log.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/log.c b/common/log.c index b2de57fcb3b8..72a4de3274c7 100644 --- a/common/log.c +++ b/common/log.c @@ -206,7 +206,7 @@ static bool log_passes_filters(struct log_device *ldev, struct log_rec *rec) static int log_dispatch(struct log_rec *rec, const char *fmt, va_list args) { struct log_device *ldev; - char buf[CONFIG_SYS_CBSIZE]; + char buf[CONFIG_SYS_PBSIZE];
/* * When a log driver writes messages (e.g. via the network stack) this @@ -268,7 +268,7 @@ int _log(enum log_category_t cat, enum log_level_t level, const char *file, /* display dropped traces with console puts and DEBUG_UART */ if (rec.level <= CONFIG_LOG_DEFAULT_LEVEL || rec.flags & LOGRECF_FORCE_DEBUG) { - char buf[CONFIG_SYS_CBSIZE]; + char buf[CONFIG_SYS_PBSIZE];
va_start(args, fmt); vsnprintf(buf, sizeof(buf), fmt, args);
On Mon, Oct 16, 2023 at 04:28:08PM -0600, Simon Glass wrote:
When CONFIG_SYS_CBSIZE is not used we need an alternative. For logging it seems that CONFIG_SYS_PBSIZE is a better choice anyway, so update this.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Tom Rini trini@konsulko.com
Provide a fallback for when CONFIG_SYS_CBSIZE is not provided, so that the console can still be used.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
drivers/video/console_truetype.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c index 14fb81e9563c..e4dad3f9a191 100644 --- a/drivers/video/console_truetype.c +++ b/drivers/video/console_truetype.c @@ -125,7 +125,11 @@ struct pos_info { * Allow one for each character on the command line plus one for each newline. * This is just an estimate, but it should not be exceeded. */ +#ifdef CONFIG_SYS_CBSIZE #define POS_HISTORY_SIZE (CONFIG_SYS_CBSIZE * 11 / 10) +#else +#define POS_HISTORY_SIZE (250 * 11 / 10) +#endif
/** * struct console_tt_metrics - Information about a font / size combination
On Mon, Oct 16, 2023 at 04:28:09PM -0600, Simon Glass wrote:
Provide a fallback for when CONFIG_SYS_CBSIZE is not provided, so that the console can still be used.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
drivers/video/console_truetype.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c index 14fb81e9563c..e4dad3f9a191 100644 --- a/drivers/video/console_truetype.c +++ b/drivers/video/console_truetype.c @@ -125,7 +125,11 @@ struct pos_info {
- Allow one for each character on the command line plus one for each newline.
- This is just an estimate, but it should not be exceeded.
*/ +#ifdef CONFIG_SYS_CBSIZE #define POS_HISTORY_SIZE (CONFIG_SYS_CBSIZE * 11 / 10) +#else +#define POS_HISTORY_SIZE (250 * 11 / 10) +#endif
/**
- struct console_tt_metrics - Information about a font / size combination
NAK, this either should be SYS_PBSIZE (output buffer) instead which you move out from under CMDLINE or SYS_CBSIZE (input buffer) should also be outside, and both get a help text to explain them a bit better.
Hi Tom,
On Tue, 17 Oct 2023 at 08:07, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:09PM -0600, Simon Glass wrote:
Provide a fallback for when CONFIG_SYS_CBSIZE is not provided, so that the console can still be used.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
drivers/video/console_truetype.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c index 14fb81e9563c..e4dad3f9a191 100644 --- a/drivers/video/console_truetype.c +++ b/drivers/video/console_truetype.c @@ -125,7 +125,11 @@ struct pos_info {
- Allow one for each character on the command line plus one for each newline.
- This is just an estimate, but it should not be exceeded.
*/ +#ifdef CONFIG_SYS_CBSIZE #define POS_HISTORY_SIZE (CONFIG_SYS_CBSIZE * 11 / 10) +#else +#define POS_HISTORY_SIZE (250 * 11 / 10) +#endif
/**
- struct console_tt_metrics - Information about a font / size combination
NAK, this either should be SYS_PBSIZE (output buffer) instead which you move out from under CMDLINE or SYS_CBSIZE (input buffer) should also be outside, and both get a help text to explain them a bit better.
I think PBSIZE would be better. Even better (eventually) will be to drop this truetype buffer if input is not needed.
Regards, Simon
On Tue, Oct 17, 2023 at 09:31:22PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 17 Oct 2023 at 08:07, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:09PM -0600, Simon Glass wrote:
Provide a fallback for when CONFIG_SYS_CBSIZE is not provided, so that the console can still be used.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
drivers/video/console_truetype.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c index 14fb81e9563c..e4dad3f9a191 100644 --- a/drivers/video/console_truetype.c +++ b/drivers/video/console_truetype.c @@ -125,7 +125,11 @@ struct pos_info {
- Allow one for each character on the command line plus one for each newline.
- This is just an estimate, but it should not be exceeded.
*/ +#ifdef CONFIG_SYS_CBSIZE #define POS_HISTORY_SIZE (CONFIG_SYS_CBSIZE * 11 / 10) +#else +#define POS_HISTORY_SIZE (250 * 11 / 10) +#endif
/**
- struct console_tt_metrics - Information about a font / size combination
NAK, this either should be SYS_PBSIZE (output buffer) instead which you move out from under CMDLINE or SYS_CBSIZE (input buffer) should also be outside, and both get a help text to explain them a bit better.
I think PBSIZE would be better. Even better (eventually) will be to drop this truetype buffer if input is not needed.
Then lets switch this to PBSIZE.
While it is nice to have the font command, using 'select' makes it impossible to build the console code without it. Change this to use 'imply' instead.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
drivers/video/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index ab927641bb7a..21ea5c860cca 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -180,7 +180,7 @@ config CONSOLE_ROTATION
config CONSOLE_TRUETYPE bool "Support a console that uses TrueType fonts" - select CMD_SELECT_FONT + imply 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.
On Mon, Oct 16, 2023 at 04:28:10PM -0600, Simon Glass wrote:
While it is nice to have the font command, using 'select' makes it impossible to build the console code without it. Change this to use 'imply' instead.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
drivers/video/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index ab927641bb7a..21ea5c860cca 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -180,7 +180,7 @@ config CONSOLE_ROTATION
config CONSOLE_TRUETYPE bool "Support a console that uses TrueType fonts"
- select CMD_SELECT_FONT
- imply 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.
This is one of those cases where "if CMDLINE" makes sense to add somewhere.
Hi Tom,
On Tue, 17 Oct 2023 at 08:07, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:10PM -0600, Simon Glass wrote:
While it is nice to have the font command, using 'select' makes it impossible to build the console code without it. Change this to use 'imply' instead.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
drivers/video/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index ab927641bb7a..21ea5c860cca 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -180,7 +180,7 @@ config CONSOLE_ROTATION
config CONSOLE_TRUETYPE bool "Support a console that uses TrueType fonts"
select CMD_SELECT_FONT
imply 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.This is one of those cases where "if CMDLINE" makes sense to add somewhere.
Maybe, if you can explain it a bit more. I want boards to be able to enable or disable this command, independently of whether truetype fonts are supported. Using 'select' here seems quite inflexible.
Regards, Simon
On Tue, Oct 17, 2023 at 09:31:29PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 17 Oct 2023 at 08:07, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:10PM -0600, Simon Glass wrote:
While it is nice to have the font command, using 'select' makes it impossible to build the console code without it. Change this to use 'imply' instead.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
drivers/video/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index ab927641bb7a..21ea5c860cca 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -180,7 +180,7 @@ config CONSOLE_ROTATION
config CONSOLE_TRUETYPE bool "Support a console that uses TrueType fonts"
select CMD_SELECT_FONT
imply 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.This is one of those cases where "if CMDLINE" makes sense to add somewhere.
Maybe, if you can explain it a bit more. I want boards to be able to enable or disable this command, independently of whether truetype fonts are supported. Using 'select' here seems quite inflexible.
Ah, OK. Checking the command itself, we should drop the select here and make CMD_SELECT_FONT default y if CONSOLE_TRUETYPE (and drop the default n line it has).
Most of the EFI functionality requires the command line, so make this explicit. Future work could adjust this, but it needs effort within the booting support first, like the bootm command.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: AKASHI Takahiro takahiro.akashi@linaro.org ---
(no changes since v2)
Changes in v2: - Adjust the depends on the commands, instead of EFI_LOADER
cmd/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index c6ea5c860e33..46e484fc08b6 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -384,7 +384,7 @@ config SYS_BOOTM_LEN
config CMD_BOOTEFI bool "bootefi" - depends on EFI_LOADER + depends on EFI_LOADER && CMDLINE default y help Boot an EFI image from memory.
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 ---
Changes in v3: - Add new patch to rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR
cmd/Kconfig | 9 +++++++++ lib/efi_loader/Kconfig | 7 +++---- lib/efi_loader/Makefile | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 46e484fc08b6..3b4112d9f319 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -306,6 +306,15 @@ config CMD_BOOTI help Boot an AArch64 Linux Kernel image from memory.
+config CMD_BOOTEFI_BOOTMGR + bool "UEFI Boot Manager command" + depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI + default y + 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. + config BOOTM_LINUX bool "Support booting Linux OS images" depends on CMD_BOOTM || CMD_BOOTZ || CMD_BOOTI diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 621ed5e5b0fb..13cad6342c36 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -32,15 +32,14 @@ config EFI_LOADER
if EFI_LOADER
-config CMD_BOOTEFI_BOOTMGR +config BOOTEFI_BOOTMGR bool "UEFI Boot Manager" - depends on CMDLINE 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
Hi Simon,
Thank you for taking my idea here.
On Mon, Oct 16, 2023 at 04:28:12PM -0600, Simon Glass wrote:
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
Changes in v3:
- Add new patch to rearrange the Kconfig for CMD_BOOTEFI_BOOTMGR
cmd/Kconfig | 9 +++++++++ lib/efi_loader/Kconfig | 7 +++---- lib/efi_loader/Makefile | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 46e484fc08b6..3b4112d9f319 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -306,6 +306,15 @@ config CMD_BOOTI help Boot an AArch64 Linux Kernel image from memory.
+config CMD_BOOTEFI_BOOTMGR
- bool "UEFI Boot Manager command"
- depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
- default y
- help
Select this option if you want to select the UEFI binary to be bootedvia UEFI variables Boot####, BootOrder, and BootNext. This enables the'bootefi bootmgr' command.config BOOTM_LINUX
Do you have any intention to put this configuration here? Otherwise, it should be placed just after CMD_BOOTEFI.
In addition, CMD_EFICONFIG should have a dependency on BOOTEFI_BOOTMGR rather than CMD_BOOTEFI_BOOTMGR as it is a separate command.
Thanks, -Takahiro Akashi
bool "Support booting Linux OS images" depends on CMD_BOOTM || CMD_BOOTZ || CMD_BOOTI diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 621ed5e5b0fb..13cad6342c36 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -32,15 +32,14 @@ config EFI_LOADER
if EFI_LOADER
-config CMD_BOOTEFI_BOOTMGR +config BOOTEFI_BOOTMGR bool "UEFI Boot Manager"
- depends on CMDLINE 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 alsonormally 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.42.0.655.g421f12c284-goog
On Mon, Oct 16, 2023 at 04:28:12PM -0600, Simon Glass wrote:
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
[snip]
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 621ed5e5b0fb..13cad6342c36 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -32,15 +32,14 @@ config EFI_LOADER
if EFI_LOADER
-config CMD_BOOTEFI_BOOTMGR +config BOOTEFI_BOOTMGR bool "UEFI Boot Manager"
- depends on CMDLINE
This is another example of why I'm asking for re-ordering things so that first you clean / re-order things then you make all of CMD depend on CMDLINE. This patch, aside from other feedback, is standalone, if you do that.
Hi Tom,
On Tue, Oct 17, 2023, 07:13 Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:12PM -0600, Simon Glass wrote:
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
[snip]
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 621ed5e5b0fb..13cad6342c36 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -32,15 +32,14 @@ config EFI_LOADER
if EFI_LOADER
-config CMD_BOOTEFI_BOOTMGR +config BOOTEFI_BOOTMGR bool "UEFI Boot Manager"
depends on CMDLINEThis is another example of why I'm asking for re-ordering things so that first you clean / re-order things then you make all of CMD depend on CMDLINE. This patch, aside from other feedback, is standalone, if you do that.
This patch is before the one that makes all of CMD depend on CMDLINE.
Regards, Simon
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK + depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
- depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
Does this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
Hi,
On Tue, 17 Oct 2023 at 08:09, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETDoes this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
It would be great to tidy up networking in lots of ways...perhaps we should add ~CMDLINE support to the list of post-lwip tasks if it lands
Regards, Simon
On Tue, Oct 17, 2023 at 09:31:34PM -0600, Simon Glass wrote:
Hi,
On Tue, 17 Oct 2023 at 08:09, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETDoes this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
It would be great to tidy up networking in lots of ways...perhaps we should add ~CMDLINE support to the list of post-lwip tasks if it lands
But this isn't even directly networking. It's I believe that the network related portion of EFI loader isn't optional today. But perhaps it could / should be?
On 10/17/23 16:09, Tom Rini wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
- depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
Does this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
We should be able to boot via EFI on devices without U-Boot network support.
We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects CONFIG_DM_ETH.
Why is this not sufficient? Is there a configuration that does not build?
Best regards
Heinrich
Hi Heinrich,
On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/17/23 16:09, Tom Rini wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
- depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
Does this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
We should be able to boot via EFI on devices without U-Boot network support.
We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects CONFIG_DM_ETH.
Why is this not sufficient? Is there a configuration that does not build?
The point of this series is to disable CMDLINE and fix up what breaks.
In this case we have some sort of breakage...perhaps Tom has already found it, but otherwise could you take a look?
We should be able to disable NET and LTO in sandbox and still build. But this fails at present[1]. You can try it on -master
Regards, Simon
[1] sjg@sjg1:~/u$ crosfw sandbox -L cmd: make -j4 'CROSS_COMPILE=' --no-print-directory 'HOSTSTRIP=true' 'QEMU_ARCH=' 'KCONFIG_NOSILENTUPDATE=1' 'O=/tmp/b/sandbox' 'NO_LTO=1' -s 'BUILD_ROM=1' all /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_eth': /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' collect2: error: ld returned 1 exit status make[1]: *** [/home/sjg/c/src/third_party/u-boot/files/Makefile:1765: u-boot] Error 1 make: *** [Makefile:177: sub-make] Error 2
sjg@sjg1:~/u$ crosfw sandbox sjg@sjg1:~/u$ (passes)
'crosfw xx' is just 'buildman --bo xxx'
diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c index a92bb896c63e..4e9996a92342 100644 --- a/boot/pxe_utils.c +++ b/boot/pxe_utils.c @@ -48,7 +48,7 @@ int pxe_get_file_size(ulong *sizep)
return 0; } - +#if 0 /** * format_mac_pxe() - obtain a MAC address in the PXE format * @@ -82,7 +82,7 @@ int format_mac_pxe(char *outbuf, size_t outbuf_len)
return 1; } - +#endif /** * get_relfile() - read a file relative to the PXE file * diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 47417cb0391d..57bb6202dbb5 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -349,3 +349,4 @@ CONFIG_TEST_FDTDEC=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y +# CONFIG_NET is not set
Regards, Simon
On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
Hi Heinrich,
On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/17/23 16:09, Tom Rini wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
- depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
Does this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
We should be able to boot via EFI on devices without U-Boot network support.
We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects CONFIG_DM_ETH.
Why is this not sufficient? Is there a configuration that does not build?
The point of this series is to disable CMDLINE and fix up what breaks.
In this case we have some sort of breakage...perhaps Tom has already found it, but otherwise could you take a look?
We should be able to disable NET and LTO in sandbox and still build. But this fails at present[1]. You can try it on -master
Obviously, it would be necessary to enclose efi_dp_from_eth() with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). Then, we could drop "depends on DM_ETH".
Another possible place for completeness is "case UCLASS_ETH" clause in dp_size(), but it seems to be harmless.
-Takahiro Akashi
Regards, Simon
[1] sjg@sjg1:~/u$ crosfw sandbox -L cmd: make -j4 'CROSS_COMPILE=' --no-print-directory 'HOSTSTRIP=true' 'QEMU_ARCH=' 'KCONFIG_NOSILENTUPDATE=1' 'O=/tmp/b/sandbox' 'NO_LTO=1' -s 'BUILD_ROM=1' all /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_eth': /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' collect2: error: ld returned 1 exit status make[1]: *** [/home/sjg/c/src/third_party/u-boot/files/Makefile:1765: u-boot] Error 1 make: *** [Makefile:177: sub-make] Error 2
sjg@sjg1:~/u$ crosfw sandbox sjg@sjg1:~/u$ (passes)
'crosfw xx' is just 'buildman --bo xxx'
diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c index a92bb896c63e..4e9996a92342 100644 --- a/boot/pxe_utils.c +++ b/boot/pxe_utils.c @@ -48,7 +48,7 @@ int pxe_get_file_size(ulong *sizep)
return 0; }
+#if 0 /**
- format_mac_pxe() - obtain a MAC address in the PXE format
@@ -82,7 +82,7 @@ int format_mac_pxe(char *outbuf, size_t outbuf_len)
return 1; }
+#endif /**
- get_relfile() - read a file relative to the PXE file
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 47417cb0391d..57bb6202dbb5 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -349,3 +349,4 @@ CONFIG_TEST_FDTDEC=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y +# CONFIG_NET is not set
Regards, Simon
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
Hi Heinrich,
On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/17/23 16:09, Tom Rini wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
- depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
Does this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
We should be able to boot via EFI on devices without U-Boot network support.
We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects CONFIG_DM_ETH.
Why is this not sufficient? Is there a configuration that does not build?
The point of this series is to disable CMDLINE and fix up what breaks.
In this case we have some sort of breakage...perhaps Tom has already found it, but otherwise could you take a look?
We should be able to disable NET and LTO in sandbox and still build. But this fails at present[1]. You can try it on -master
Obviously, it would be necessary to enclose efi_dp_from_eth() with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
Another possible place for completeness is "case UCLASS_ETH" clause in dp_size(), but it seems to be harmless.
OK
Regards, Simon
-Takahiro Akashi
Regards, Simon
[1] sjg@sjg1:~/u$ crosfw sandbox -L cmd: make -j4 'CROSS_COMPILE=' --no-print-directory 'HOSTSTRIP=true' 'QEMU_ARCH=' 'KCONFIG_NOSILENTUPDATE=1' 'O=/tmp/b/sandbox' 'NO_LTO=1' -s 'BUILD_ROM=1' all /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_eth': /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' collect2: error: ld returned 1 exit status make[1]: *** [/home/sjg/c/src/third_party/u-boot/files/Makefile:1765: u-boot] Error 1 make: *** [Makefile:177: sub-make] Error 2
sjg@sjg1:~/u$ crosfw sandbox sjg@sjg1:~/u$ (passes)
'crosfw xx' is just 'buildman --bo xxx'
diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c index a92bb896c63e..4e9996a92342 100644 --- a/boot/pxe_utils.c +++ b/boot/pxe_utils.c @@ -48,7 +48,7 @@ int pxe_get_file_size(ulong *sizep)
return 0; }
+#if 0 /**
- format_mac_pxe() - obtain a MAC address in the PXE format
@@ -82,7 +82,7 @@ int format_mac_pxe(char *outbuf, size_t outbuf_len)
return 1; }
+#endif /**
- get_relfile() - read a file relative to the PXE file
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 47417cb0391d..57bb6202dbb5 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -349,3 +349,4 @@ CONFIG_TEST_FDTDEC=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y +# CONFIG_NET is not set
Regards, Simon
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
Hi Heinrich,
On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/17/23 16:09, Tom Rini wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
- depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
Does this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
We should be able to boot via EFI on devices without U-Boot network support.
We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects CONFIG_DM_ETH.
Why is this not sufficient? Is there a configuration that does not build?
The point of this series is to disable CMDLINE and fix up what breaks.
In this case we have some sort of breakage...perhaps Tom has already found it, but otherwise could you take a look?
We should be able to disable NET and LTO in sandbox and still build. But this fails at present[1]. You can try it on -master
Obviously, it would be necessary to enclose efi_dp_from_eth() with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
Hi Heinrich,
On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/17/23 16:09, Tom Rini wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
> Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > available, add it as an explicit dependency. > > Signed-off-by: Simon Glass sjg@chromium.org > --- > > (no changes since v2) > > Changes in v2: > - Add new patch to update EFI_LOADER to depend on DM_ETH > > lib/efi_loader/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > index 13cad6342c36..fca4b3eef270 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -11,6 +11,7 @@ config EFI_LOADER > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > depends on BLK > + depends on DM_ETH > depends on !EFI_APP > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > select CHARSET
Does this work for you Heinrich, or do you want to clarify the dependencies (and re-organize the code as needed) around networking?
We should be able to boot via EFI on devices without U-Boot network support.
We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects CONFIG_DM_ETH.
Why is this not sufficient? Is there a configuration that does not build?
The point of this series is to disable CMDLINE and fix up what breaks.
In this case we have some sort of breakage...perhaps Tom has already found it, but otherwise could you take a look?
We should be able to disable NET and LTO in sandbox and still build. But this fails at present[1]. You can try it on -master
Obviously, it would be necessary to enclose efi_dp_from_eth() with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Best regards
Heinrich
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
Hi Heinrich,
On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/17/23 16:09, Tom Rini wrote: > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > available, add it as an explicit dependency. > > > > Signed-off-by: Simon Glass sjg@chromium.org > > --- > > > > (no changes since v2) > > > > Changes in v2: > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > lib/efi_loader/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > index 13cad6342c36..fca4b3eef270 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -11,6 +11,7 @@ config EFI_LOADER > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > depends on BLK > > + depends on DM_ETH > > depends on !EFI_APP > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > select CHARSET > > Does this work for you Heinrich, or do you want to clarify the > dependencies (and re-organize the code as needed) around networking? >
We should be able to boot via EFI on devices without U-Boot network support.
We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects CONFIG_DM_ETH.
Why is this not sufficient? Is there a configuration that does not build?
The point of this series is to disable CMDLINE and fix up what breaks.
In this case we have some sort of breakage...perhaps Tom has already found it, but otherwise could you take a look?
We should be able to disable NET and LTO in sandbox and still build. But this fails at present[1]. You can try it on -master
Obviously, it would be necessary to enclose efi_dp_from_eth() with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
Hi Heinrich,
On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > On 10/17/23 16:09, Tom Rini wrote: > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > available, add it as an explicit dependency. > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > --- > > > > > > (no changes since v2) > > > > > > Changes in v2: > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > lib/efi_loader/Kconfig | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > index 13cad6342c36..fca4b3eef270 100644 > > > --- a/lib/efi_loader/Kconfig > > > +++ b/lib/efi_loader/Kconfig > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > depends on BLK > > > + depends on DM_ETH > > > depends on !EFI_APP > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > select CHARSET > > > > Does this work for you Heinrich, or do you want to clarify the > > dependencies (and re-organize the code as needed) around networking? > > > > We should be able to boot via EFI on devices without U-Boot network support. > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > CONFIG_DM_ETH. > > Why is this not sufficient? > Is there a configuration that does not build?
The point of this series is to disable CMDLINE and fix up what breaks.
In this case we have some sort of breakage...perhaps Tom has already found it, but otherwise could you take a look?
We should be able to disable NET and LTO in sandbox and still build. But this fails at present[1]. You can try it on -master
Obviously, it would be necessary to enclose efi_dp_from_eth() with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > Hi Heinrich, > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > On 10/17/23 16:09, Tom Rini wrote: > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > available, add it as an explicit dependency. > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > --- > > > > > > > > (no changes since v2) > > > > > > > > Changes in v2: > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > --- a/lib/efi_loader/Kconfig > > > > +++ b/lib/efi_loader/Kconfig > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > depends on BLK > > > > + depends on DM_ETH > > > > depends on !EFI_APP > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > select CHARSET > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > CONFIG_DM_ETH. > > > > Why is this not sufficient? > > Is there a configuration that does not build? > > The point of this series is to disable CMDLINE and fix up what breaks. > > In this case we have some sort of breakage...perhaps Tom has already > found it, but otherwise could you take a look? > > We should be able to disable NET and LTO in sandbox and still build. > But this fails at present[1]. You can try it on -master
Obviously, it would be necessary to enclose efi_dp_from_eth() with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
At least for me 'u-boot -D' doesn't work with Heinrich's patch. Do we really need to make this change? It will rapidly devolve to the point that coming back would be extremely painful.
Regards, Simon
On 10/22/23 23:55, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: >> Hi Heinrich, >> >> On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: >>> >>> On 10/17/23 16:09, Tom Rini wrote: >>>> On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: >>>> >>>>> Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is >>>>> available, add it as an explicit dependency. >>>>> >>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>> --- >>>>> >>>>> (no changes since v2) >>>>> >>>>> Changes in v2: >>>>> - Add new patch to update EFI_LOADER to depend on DM_ETH >>>>> >>>>> lib/efi_loader/Kconfig | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig >>>>> index 13cad6342c36..fca4b3eef270 100644 >>>>> --- a/lib/efi_loader/Kconfig >>>>> +++ b/lib/efi_loader/Kconfig >>>>> @@ -11,6 +11,7 @@ config EFI_LOADER >>>>> # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB >>>>> depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT >>>>> depends on BLK >>>>> + depends on DM_ETH >>>>> depends on !EFI_APP >>>>> default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 >>>>> select CHARSET >>>> >>>> Does this work for you Heinrich, or do you want to clarify the >>>> dependencies (and re-organize the code as needed) around networking? >>>> >>> >>> We should be able to boot via EFI on devices without U-Boot network support. >>> >>> We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking >>> eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects >>> CONFIG_DM_ETH. >>> >>> Why is this not sufficient? >>> Is there a configuration that does not build? >> >> The point of this series is to disable CMDLINE and fix up what breaks. >> >> In this case we have some sort of breakage...perhaps Tom has already >> found it, but otherwise could you take a look? >> >> We should be able to disable NET and LTO in sandbox and still build. >> But this fails at present[1]. You can try it on -master > > Obviously, it would be necessary to enclose efi_dp_from_eth() > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
At least for me 'u-boot -D' doesn't work with Heinrich's patch. Do we really need to make this change? It will rapidly devolve to the point that coming back would be extremely painful.
It just shows where the code is not clean.
Linker generated lists referenced by the corresponding macros are compiled in (as shows up in u-boot.map). But you have decided not to use those macros to access the sandbox command line options. Now they are removed as unreferenced garbage.
Can't we use the linker generated list macros instead of duplicating code?
* define SANDBOX_CMDLINE_OPT via ll_entry_declare(). * replace ll_entry_count() instead of __u_boot_sandbox_option_count() * use ll_start(), ll_end() for iterating
Best regards
Heinrich
On Mon, Oct 23, 2023 at 12:55:34AM +0200, Heinrich Schuchardt wrote:
On 10/22/23 23:55, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote: > Hi, > > On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro > takahiro.akashi@linaro.org wrote: > > > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > On 10/17/23 16:09, Tom Rini wrote: > > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > > > available, add it as an explicit dependency. > > > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > > --- > > > > > > > > > > > > (no changes since v2) > > > > > > > > > > > > Changes in v2: > > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > > > depends on BLK > > > > > > + depends on DM_ETH > > > > > > depends on !EFI_APP > > > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > > > select CHARSET > > > > > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > > > CONFIG_DM_ETH. > > > > > > > > Why is this not sufficient? > > > > Is there a configuration that does not build? > > > > > > The point of this series is to disable CMDLINE and fix up what breaks. > > > > > > In this case we have some sort of breakage...perhaps Tom has already > > > found it, but otherwise could you take a look? > > > > > > We should be able to disable NET and LTO in sandbox and still build. > > > But this fails at present[1]. You can try it on -master > > > > Obviously, it would be necessary to enclose efi_dp_from_eth() > > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > > Then, we could drop "depends on DM_ETH". > > Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
At least for me 'u-boot -D' doesn't work with Heinrich's patch. Do we really need to make this change? It will rapidly devolve to the point that coming back would be extremely painful.
It just shows where the code is not clean.
Linker generated lists referenced by the corresponding macros are compiled in (as shows up in u-boot.map). But you have decided not to use those macros to access the sandbox command line options. Now they are removed as unreferenced garbage.
Can't we use the linker generated list macros instead of duplicating code?
- define SANDBOX_CMDLINE_OPT via ll_entry_declare().
- replace ll_entry_count() instead of __u_boot_sandbox_option_count()
- use ll_start(), ll_end() for iterating
Oh, it comes down to something like this? I don't recall if we had __used the first time we put this on sandbox, but we do now and we use it for cases where LTO thinks it knows best. We should be able to do that too for sandbox.
On Sun, Oct 22, 2023 at 02:55:32PM -0700, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
Hi,
On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro takahiro.akashi@linaro.org wrote: > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > > Hi Heinrich, > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > On 10/17/23 16:09, Tom Rini wrote: > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > > available, add it as an explicit dependency. > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > --- > > > > > > > > > > (no changes since v2) > > > > > > > > > > Changes in v2: > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > > --- a/lib/efi_loader/Kconfig > > > > > +++ b/lib/efi_loader/Kconfig > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > > depends on BLK > > > > > + depends on DM_ETH > > > > > depends on !EFI_APP > > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > > select CHARSET > > > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > > CONFIG_DM_ETH. > > > > > > Why is this not sufficient? > > > Is there a configuration that does not build? > > > > The point of this series is to disable CMDLINE and fix up what breaks. > > > > In this case we have some sort of breakage...perhaps Tom has already > > found it, but otherwise could you take a look? > > > > We should be able to disable NET and LTO in sandbox and still build. > > But this fails at present[1]. You can try it on -master > > Obviously, it would be necessary to enclose efi_dp_from_eth() > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > Then, we could drop "depends on DM_ETH".
Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
It doesn't predate gc-sections, we've just never moved it to the top-level Makefiles (and disabled on LTO) like we should have a long time ago. It's supported by all the compilers we support, and it's a required feature (or, LTO) for U-Boot.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
That's not the point of the feature. The point is that we can discard unused code without having to rely on #ifdef and __maybe_unused.
At least for me 'u-boot -D' doesn't work with Heinrich's patch. Do we really need to make this change? It will rapidly devolve to the point that coming back would be extremely painful.
Do we absolutely need which, gc-sections or LTO? Yes, we do. And I don't see immediately how that changes --default_fdt ?
Hi Tom,
On Sun, 22 Oct 2023 at 16:45, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 02:55:32PM -0700, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote:
On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote: > Hi, > > On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro > takahiro.akashi@linaro.org wrote: > > > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > On 10/17/23 16:09, Tom Rini wrote: > > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > > > available, add it as an explicit dependency. > > > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > > --- > > > > > > > > > > > > (no changes since v2) > > > > > > > > > > > > Changes in v2: > > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > > > depends on BLK > > > > > > + depends on DM_ETH > > > > > > depends on !EFI_APP > > > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > > > select CHARSET > > > > > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > > > CONFIG_DM_ETH. > > > > > > > > Why is this not sufficient? > > > > Is there a configuration that does not build? > > > > > > The point of this series is to disable CMDLINE and fix up what breaks. > > > > > > In this case we have some sort of breakage...perhaps Tom has already > > > found it, but otherwise could you take a look? > > > > > > We should be able to disable NET and LTO in sandbox and still build. > > > But this fails at present[1]. You can try it on -master > > > > Obviously, it would be necessary to enclose efi_dp_from_eth() > > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > > Then, we could drop "depends on DM_ETH". > > Strange that it only happens on the non-LTO board, though?
There's two issues. The first of which is that I think you need to re-check your error exactly? With my series, and LTO also disabled the problem is a call to efi_get_image_parameters() as that's defined in cmd/bootefi.c, but also only used with cmdline invocations. So we can fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) around that, and then discard efi_dp_from_name() entirely.
The second issue is that with LTO we more completely find the cases where if x() calls y() and y() is undefined but nothing calls x() we can just discard x() and not care that y() is undefined.
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
It doesn't predate gc-sections, we've just never moved it to the top-level Makefiles (and disabled on LTO) like we should have a long time ago. It's supported by all the compilers we support, and it's a required feature (or, LTO) for U-Boot.
Remember that the sandbox linker script is special in that it adds to the existing default one provided by the toolchain
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
That's not the point of the feature. The point is that we can discard unused code without having to rely on #ifdef and __maybe_unused.
That doesn't match with my understanding. An unused static will still produce a warning if it is unused.
Re #ifdef, you will still see code discarded with sandbox if 'if IS_ENABLED()' is used. It's just that the link-time
Sandbox doesn't really have a need to discard unused code. It doesn't make much difference to the binary size, from what I can see, partly because we enable as much as we can.
No LTO: before: 3652159 220840 126576 3999575 3d0757 /tmp/b/sandbox/u-boot after: 3515872 204016 124528 3844416 3aa940 /tmp/b/sandbox/u-boot Saving 3.7%
With LTO: before: 3391148 212224 124704 3728076 38e2cc /tmp/b/sandbox/u-boot after: 3356324 200664 124704 3681692 382d9c /tmp/b/sandbox/u-boot Saving 1%
That reflects the fact that LTO is quite a bit more powerful than linker hacks.
The other thing I have noticed over the years is that linker discarding can change behaviour when unrelated changes are made. For example, if I refactor a function the linker decides that a function it calls is now needed, whereas before it was not. That sort of thing is annoying with sandbox.
At least for me 'u-boot -D' doesn't work with Heinrich's patch. Do we really need to make this change? It will rapidly devolve to the point that coming back would be extremely painful.
Do we absolutely need which, gc-sections or LTO? Yes, we do. And I don't see immediately how that changes --default_fdt ?
I hope that with all the clang effort we might be closer to having sandbox behave well with this change. But I still don't see the point.
Regards, SImon
On Mon, Oct 23, 2023 at 12:05:28AM -0700, Simon Glass wrote:
Hi Tom,
On Sun, 22 Oct 2023 at 16:45, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 02:55:32PM -0700, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
On 10/21/23 20:26, Tom Rini wrote: > On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote: > > Hi, > > > > On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro > > takahiro.akashi@linaro.org wrote: > > > > > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > > > > Hi Heinrich, > > > > > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > On 10/17/23 16:09, Tom Rini wrote: > > > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > > > > available, add it as an explicit dependency. > > > > > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > > > --- > > > > > > > > > > > > > > (no changes since v2) > > > > > > > > > > > > > > Changes in v2: > > > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > > > > depends on BLK > > > > > > > + depends on DM_ETH > > > > > > > depends on !EFI_APP > > > > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > > > > select CHARSET > > > > > > > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > > > > CONFIG_DM_ETH. > > > > > > > > > > Why is this not sufficient? > > > > > Is there a configuration that does not build? > > > > > > > > The point of this series is to disable CMDLINE and fix up what breaks. > > > > > > > > In this case we have some sort of breakage...perhaps Tom has already > > > > found it, but otherwise could you take a look? > > > > > > > > We should be able to disable NET and LTO in sandbox and still build. > > > > But this fails at present[1]. You can try it on -master > > > > > > Obviously, it would be necessary to enclose efi_dp_from_eth() > > > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > > > Then, we could drop "depends on DM_ETH". > > > > Strange that it only happens on the non-LTO board, though? > > There's two issues. The first of which is that I think you need to > re-check your error exactly? With my series, and LTO also disabled the > problem is a call to efi_get_image_parameters() as that's defined in > cmd/bootefi.c, but also only used with cmdline invocations. So we can > fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) > around that, and then discard efi_dp_from_name() entirely. > > The second issue is that with LTO we more completely find the cases > where if x() calls y() and y() is undefined but nothing calls x() we can > just discard x() and not care that y() is undefined. >
I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
@Simon
One thing that I don't understand is why we don't let the linker eliminate the unused functions on the sandbox.
On other architectures we put each function into a separate text section and let the linker eliminate the unused text sections:
arch/riscv/config.mk:29: PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections LDFLAGS_u-boot += --gc-sections -static -pie
Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
It doesn't predate gc-sections, we've just never moved it to the top-level Makefiles (and disabled on LTO) like we should have a long time ago. It's supported by all the compilers we support, and it's a required feature (or, LTO) for U-Boot.
Remember that the sandbox linker script is special in that it adds to the existing default one provided by the toolchain
Yes, and we've also been using gc-sections since 2008, from a quick look at the logs.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
That's not the point of the feature. The point is that we can discard unused code without having to rely on #ifdef and __maybe_unused.
That doesn't match with my understanding. An unused static will still produce a warning if it is unused.
Yes, you're misunderstanding how this feature works and why it's required. With v4 of my series, on sandbox and CMDLINE=n and LTO=n: /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_send_key': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:157: undefined reference to `cread_line_process_ch' /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_open': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:222: undefined reference to `cli_cread_init' /usr/bin/ld: common/bootstage.o: in function `bootstage_fdt_add_report': /home/trini/work/u-boot/u-boot/common/bootstage.c:323: undefined reference to `working_fdt' /usr/bin/ld: common/cli.o: in function `run_commandf': /home/trini/work/u-boot/u-boot/common/cli.c:154: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_command.o: in function `fastboot_acmd_complete': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_command.c:350: undefined reference to `run_command' /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': /home/trini/work/u-boot/u-boot/lib/efi_loader/efi_device_path.c:1095: undefined reference to `efi_get_image_parameters' /usr/bin/ld: net/net.o: in function `net_auto_load': /home/trini/work/u-boot/u-boot/net/net.c:349: undefined reference to `tftp_start' collect2: error: ld returned 1 exit status
And with re-enabling gc-sections: /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' collect2: error: ld returned 1 exit status
Because all of those other link failures are in unreachable code and the rest of the code base is already setup to know that x() can call y() and y() can just be undefined at link time if x() is never called and ends up discarded.
I honestly didn't realize sandbox hadn't had this critical feature enabled and I hope we haven't been doing too many odd shuffles of the codebase because of it.
Hi Tom,
On Mon, 23 Oct 2023 at 06:16, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 23, 2023 at 12:05:28AM -0700, Simon Glass wrote:
Hi Tom,
On Sun, 22 Oct 2023 at 16:45, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 02:55:32PM -0700, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote: > On 10/21/23 20:26, Tom Rini wrote: > > On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote: > > > Hi, > > > > > > On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro > > > takahiro.akashi@linaro.org wrote: > > > > > > > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > > > > > Hi Heinrich, > > > > > > > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > > > On 10/17/23 16:09, Tom Rini wrote: > > > > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > > > > > available, add it as an explicit dependency. > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > > > > --- > > > > > > > > > > > > > > > > (no changes since v2) > > > > > > > > > > > > > > > > Changes in v2: > > > > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > > > > > depends on BLK > > > > > > > > + depends on DM_ETH > > > > > > > > depends on !EFI_APP > > > > > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > > > > > select CHARSET > > > > > > > > > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > > > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > > > > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > > > > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > > > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > > > > > CONFIG_DM_ETH. > > > > > > > > > > > > Why is this not sufficient? > > > > > > Is there a configuration that does not build? > > > > > > > > > > The point of this series is to disable CMDLINE and fix up what breaks. > > > > > > > > > > In this case we have some sort of breakage...perhaps Tom has already > > > > > found it, but otherwise could you take a look? > > > > > > > > > > We should be able to disable NET and LTO in sandbox and still build. > > > > > But this fails at present[1]. You can try it on -master > > > > > > > > Obviously, it would be necessary to enclose efi_dp_from_eth() > > > > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > > > > Then, we could drop "depends on DM_ETH". > > > > > > Strange that it only happens on the non-LTO board, though? > > > > There's two issues. The first of which is that I think you need to > > re-check your error exactly? With my series, and LTO also disabled the > > problem is a call to efi_get_image_parameters() as that's defined in > > cmd/bootefi.c, but also only used with cmdline invocations. So we can > > fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) > > around that, and then discard efi_dp_from_name() entirely. > > > > The second issue is that with LTO we more completely find the cases > > where if x() calls y() and y() is undefined but nothing calls x() we can > > just discard x() and not care that y() is undefined. > > > > I will send a patch for function efi_dp_from_eth().
There's no problem with efi_dp_from_eth as far as I can tell.
> @Simon > > One thing that I don't understand is why we don't let the linker > eliminate the unused functions on the sandbox. > > On other architectures we put each function into a separate text section > and let the linker eliminate the unused text sections: > > arch/riscv/config.mk:29: > PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections > LDFLAGS_u-boot += --gc-sections -static -pie > > Shouldn't we keep the sandbox close to what other architectures do?
Oh my, I didn't realize that sandbox was missing the garbage collection stuff. Yes, that needs to be fixed first, then we can see what's next to change, as there are some issues (my series first fixed CMDLINE=n on qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
It doesn't predate gc-sections, we've just never moved it to the top-level Makefiles (and disabled on LTO) like we should have a long time ago. It's supported by all the compilers we support, and it's a required feature (or, LTO) for U-Boot.
Remember that the sandbox linker script is special in that it adds to the existing default one provided by the toolchain
Yes, and we've also been using gc-sections since 2008, from a quick look at the logs.
Right, I just mean that sandbox does not use a standalone linker script. It is not common to garbage-collect sections in executables, so far as I know.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
That's not the point of the feature. The point is that we can discard unused code without having to rely on #ifdef and __maybe_unused.
That doesn't match with my understanding. An unused static will still produce a warning if it is unused.
Yes, you're misunderstanding how this feature works and why it's required. With v4 of my series, on sandbox and CMDLINE=n and LTO=n: /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_send_key': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:157: undefined reference to `cread_line_process_ch' /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_open': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:222: undefined reference to `cli_cread_init' /usr/bin/ld: common/bootstage.o: in function `bootstage_fdt_add_report': /home/trini/work/u-boot/u-boot/common/bootstage.c:323: undefined reference to `working_fdt' /usr/bin/ld: common/cli.o: in function `run_commandf': /home/trini/work/u-boot/u-boot/common/cli.c:154: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_command.o: in function `fastboot_acmd_complete': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_command.c:350: undefined reference to `run_command' /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': /home/trini/work/u-boot/u-boot/lib/efi_loader/efi_device_path.c:1095: undefined reference to `efi_get_image_parameters' /usr/bin/ld: net/net.o: in function `net_auto_load': /home/trini/work/u-boot/u-boot/net/net.c:349: undefined reference to `tftp_start' collect2: error: ld returned 1 exit status
And with re-enabling gc-sections: /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' collect2: error: ld returned 1 exit status
Because all of those other link failures are in unreachable code and the rest of the code base is already setup to know that x() can call y() and y() can just be undefined at link time if x() is never called and ends up discarded.
All of those problems were corrected by my series. Perhaps that accounts for the difference between them?
Just to take one example, working_fdt, while it might seem convenient to leave it in cmd/fdt.c it doesn't actually make sense, since the var is set when booting. Without that var DT fixups can't happen. So why not pick up my patch that moves it to boot/fdt_support.c where it belongs?
Looking at the list of errors, we should just fix them. They all indicate real problems with the structure of the code. Another example is fastboot which needs a condition for CMDLINE.
I honestly didn't realize sandbox hadn't had this critical feature enabled and I hope we haven't been doing too many odd shuffles of the codebase because of it.
I'm a bit unsure where you are heading with this. Ultimate we don't want to disable all the functionality when CMDLINE is disabled. I think with the addition of 5-6 patches in your series you can fix it.
Regards, Simon
On Mon, Oct 23, 2023 at 10:13:54AM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 23 Oct 2023 at 06:16, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 23, 2023 at 12:05:28AM -0700, Simon Glass wrote:
Hi Tom,
On Sun, 22 Oct 2023 at 16:45, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 02:55:32PM -0700, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote: > On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote: > > On 10/21/23 20:26, Tom Rini wrote: > > > On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote: > > > > Hi, > > > > > > > > On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro > > > > takahiro.akashi@linaro.org wrote: > > > > > > > > > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > > > > > > Hi Heinrich, > > > > > > > > > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > > > > > On 10/17/23 16:09, Tom Rini wrote: > > > > > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > > > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > > > > > > available, add it as an explicit dependency. > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > > > > > --- > > > > > > > > > > > > > > > > > > (no changes since v2) > > > > > > > > > > > > > > > > > > Changes in v2: > > > > > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > > > > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > > > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > > > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > > > > > > depends on BLK > > > > > > > > > + depends on DM_ETH > > > > > > > > > depends on !EFI_APP > > > > > > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > > > > > > select CHARSET > > > > > > > > > > > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > > > > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > > > > > > > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > > > > > > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > > > > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > > > > > > CONFIG_DM_ETH. > > > > > > > > > > > > > > Why is this not sufficient? > > > > > > > Is there a configuration that does not build? > > > > > > > > > > > > The point of this series is to disable CMDLINE and fix up what breaks. > > > > > > > > > > > > In this case we have some sort of breakage...perhaps Tom has already > > > > > > found it, but otherwise could you take a look? > > > > > > > > > > > > We should be able to disable NET and LTO in sandbox and still build. > > > > > > But this fails at present[1]. You can try it on -master > > > > > > > > > > Obviously, it would be necessary to enclose efi_dp_from_eth() > > > > > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > > > > > Then, we could drop "depends on DM_ETH". > > > > > > > > Strange that it only happens on the non-LTO board, though? > > > > > > There's two issues. The first of which is that I think you need to > > > re-check your error exactly? With my series, and LTO also disabled the > > > problem is a call to efi_get_image_parameters() as that's defined in > > > cmd/bootefi.c, but also only used with cmdline invocations. So we can > > > fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) > > > around that, and then discard efi_dp_from_name() entirely. > > > > > > The second issue is that with LTO we more completely find the cases > > > where if x() calls y() and y() is undefined but nothing calls x() we can > > > just discard x() and not care that y() is undefined. > > > > > > > I will send a patch for function efi_dp_from_eth(). > > There's no problem with efi_dp_from_eth as far as I can tell. > > > @Simon > > > > One thing that I don't understand is why we don't let the linker > > eliminate the unused functions on the sandbox. > > > > On other architectures we put each function into a separate text section > > and let the linker eliminate the unused text sections: > > > > arch/riscv/config.mk:29: > > PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections > > LDFLAGS_u-boot += --gc-sections -static -pie > > > > Shouldn't we keep the sandbox close to what other architectures do? > > Oh my, I didn't realize that sandbox was missing the garbage collection > stuff. Yes, that needs to be fixed first, then we can see what's next > to change, as there are some issues (my series first fixed CMDLINE=n on > qemu_arm64).
My super quick pass at enabling function-sections/data-sections/gc-sections shows there's nothing further needed for CMDLINE=n and LTO=n on sandbox, not even the part I was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
It doesn't predate gc-sections, we've just never moved it to the top-level Makefiles (and disabled on LTO) like we should have a long time ago. It's supported by all the compilers we support, and it's a required feature (or, LTO) for U-Boot.
Remember that the sandbox linker script is special in that it adds to the existing default one provided by the toolchain
Yes, and we've also been using gc-sections since 2008, from a quick look at the logs.
Right, I just mean that sandbox does not use a standalone linker script.
Yes it does, arch/sandbox/cpu/u-boot.lds.
It is not common to garbage-collect sections in executables, so far as I know.
I'm not sure how that's relevant here, honestly. And we didn't ask the toolchain community to invent this for us, it's a standard feature that we've found helpful for a number of reasons.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
That's not the point of the feature. The point is that we can discard unused code without having to rely on #ifdef and __maybe_unused.
That doesn't match with my understanding. An unused static will still produce a warning if it is unused.
Yes, you're misunderstanding how this feature works and why it's required. With v4 of my series, on sandbox and CMDLINE=n and LTO=n: /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_send_key': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:157: undefined reference to `cread_line_process_ch' /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_open': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:222: undefined reference to `cli_cread_init' /usr/bin/ld: common/bootstage.o: in function `bootstage_fdt_add_report': /home/trini/work/u-boot/u-boot/common/bootstage.c:323: undefined reference to `working_fdt' /usr/bin/ld: common/cli.o: in function `run_commandf': /home/trini/work/u-boot/u-boot/common/cli.c:154: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_command.o: in function `fastboot_acmd_complete': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_command.c:350: undefined reference to `run_command' /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': /home/trini/work/u-boot/u-boot/lib/efi_loader/efi_device_path.c:1095: undefined reference to `efi_get_image_parameters' /usr/bin/ld: net/net.o: in function `net_auto_load': /home/trini/work/u-boot/u-boot/net/net.c:349: undefined reference to `tftp_start' collect2: error: ld returned 1 exit status
And with re-enabling gc-sections: /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' collect2: error: ld returned 1 exit status
Because all of those other link failures are in unreachable code and the rest of the code base is already setup to know that x() can call y() and y() can just be undefined at link time if x() is never called and ends up discarded.
All of those problems were corrected by my series. Perhaps that accounts for the difference between them?
All of these are not real problems at this point in time is my point.
Just to take one example, working_fdt, while it might seem convenient to leave it in cmd/fdt.c it doesn't actually make sense, since the var is set when booting. Without that var DT fixups can't happen. So why not pick up my patch that moves it to boot/fdt_support.c where it belongs?
And the issue is that when the command line is disabled NOTHING uses "working_fdt". We don't need it. It would be discarded by real platforms, if we moved it.
To rephrase things another way, skimming the code, that bootm_find_images() does: if (IS_ENABLED(CONFIG_CMD_FDT)) set_working_fdt_addr(map_to_sysmem(images.ft_addr));
And so we never set "working_fdt" when CMDLINE is off (since CMD_FDT is off) IS the bug, and that needs to be fixed. That's when you should figure out where "working_fdt" should be (and also maybe re-name it or the local variable named "working_fdt" that the pxe_utils.c code uses) and not before then.
Looking at the list of errors, we should just fix them. They all indicate real problems with the structure of the code.
No, they do not. The codebase assume that we discard unreachable functions. This allows us to NOT have to use #ifdefs and also not have to make otherwise unreasonable code splits.
Another example is fastboot which needs a condition for CMDLINE.
This is an example where yes, the functionality requires CMDLINE. Because reading the code shows that fastboot will literally run "bootcmd", and so requires that to be available.
I honestly didn't realize sandbox hadn't had this critical feature enabled and I hope we haven't been doing too many odd shuffles of the codebase because of it.
I'm a bit unsure where you are heading with this. Ultimate we don't want to disable all the functionality when CMDLINE is disabled. I think with the addition of 5-6 patches in your series you can fix it.
Where I'm heading with this is to say that sandbox must work like a real platform does and discard unreachable code. You don't know what code is or is not really needed for functionality X and needs to be moved for it to not rely on what's in the command file vs elsewhere until you act like other platforms do.
Hi Tom,
On Mon, 23 Oct 2023 at 10:45, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 23, 2023 at 10:13:54AM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 23 Oct 2023 at 06:16, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 23, 2023 at 12:05:28AM -0700, Simon Glass wrote:
Hi Tom,
On Sun, 22 Oct 2023 at 16:45, Tom Rini trini@konsulko.com wrote:
On Sun, Oct 22, 2023 at 02:55:32PM -0700, Simon Glass wrote:
Hi,
On Sun, 22 Oct 2023 at 07:59, Tom Rini trini@konsulko.com wrote: > > On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote: > > On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote: > > > On 10/21/23 20:26, Tom Rini wrote: > > > > On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro > > > > > takahiro.akashi@linaro.org wrote: > > > > > > > > > > > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote: > > > > > > > Hi Heinrich, > > > > > > > > > > > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > > > > > > > On 10/17/23 16:09, Tom Rini wrote: > > > > > > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote: > > > > > > > > > > > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is > > > > > > > > > > available, add it as an explicit dependency. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > (no changes since v2) > > > > > > > > > > > > > > > > > > > > Changes in v2: > > > > > > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH > > > > > > > > > > > > > > > > > > > > lib/efi_loader/Kconfig | 1 + > > > > > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > > > > > index 13cad6342c36..fca4b3eef270 100644 > > > > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER > > > > > > > > > > # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > > > > > > > > > > depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > > > > > > > > > > depends on BLK > > > > > > > > > > + depends on DM_ETH > > > > > > > > > > depends on !EFI_APP > > > > > > > > > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 > > > > > > > > > > select CHARSET > > > > > > > > > > > > > > > > > > Does this work for you Heinrich, or do you want to clarify the > > > > > > > > > dependencies (and re-organize the code as needed) around networking? > > > > > > > > > > > > > > > > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support. > > > > > > > > > > > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking > > > > > > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects > > > > > > > > CONFIG_DM_ETH. > > > > > > > > > > > > > > > > Why is this not sufficient? > > > > > > > > Is there a configuration that does not build? > > > > > > > > > > > > > > The point of this series is to disable CMDLINE and fix up what breaks. > > > > > > > > > > > > > > In this case we have some sort of breakage...perhaps Tom has already > > > > > > > found it, but otherwise could you take a look? > > > > > > > > > > > > > > We should be able to disable NET and LTO in sandbox and still build. > > > > > > > But this fails at present[1]. You can try it on -master > > > > > > > > > > > > Obviously, it would be necessary to enclose efi_dp_from_eth() > > > > > > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH). > > > > > > Then, we could drop "depends on DM_ETH". > > > > > > > > > > Strange that it only happens on the non-LTO board, though? > > > > > > > > There's two issues. The first of which is that I think you need to > > > > re-check your error exactly? With my series, and LTO also disabled the > > > > problem is a call to efi_get_image_parameters() as that's defined in > > > > cmd/bootefi.c, but also only used with cmdline invocations. So we can > > > > fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE) > > > > around that, and then discard efi_dp_from_name() entirely. > > > > > > > > The second issue is that with LTO we more completely find the cases > > > > where if x() calls y() and y() is undefined but nothing calls x() we can > > > > just discard x() and not care that y() is undefined. > > > > > > > > > > I will send a patch for function efi_dp_from_eth(). > > > > There's no problem with efi_dp_from_eth as far as I can tell. > > > > > @Simon > > > > > > One thing that I don't understand is why we don't let the linker > > > eliminate the unused functions on the sandbox. > > > > > > On other architectures we put each function into a separate text section > > > and let the linker eliminate the unused text sections: > > > > > > arch/riscv/config.mk:29: > > > PLATFORM_RELFLAGS += -fno-common -ffunction-sections -fdata-sections > > > LDFLAGS_u-boot += --gc-sections -static -pie > > > > > > Shouldn't we keep the sandbox close to what other architectures do? > > > > Oh my, I didn't realize that sandbox was missing the garbage collection > > stuff. Yes, that needs to be fixed first, then we can see what's next > > to change, as there are some issues (my series first fixed CMDLINE=n on > > qemu_arm64). > > My super quick pass at enabling > function-sections/data-sections/gc-sections shows there's nothing > further needed for CMDLINE=n and LTO=n on sandbox, not even the part I > was looking at before.
I am not sure about the original reason, but sandbox probably pre-dates wholesale enabling of gc-sections (I haven't looked). There is also the issue of whether the host toolchain supports it. Sandbox is supposed to run on a variety of OS types.
It doesn't predate gc-sections, we've just never moved it to the top-level Makefiles (and disabled on LTO) like we should have a long time ago. It's supported by all the compilers we support, and it's a required feature (or, LTO) for U-Boot.
Remember that the sandbox linker script is special in that it adds to the existing default one provided by the toolchain
Yes, and we've also been using gc-sections since 2008, from a quick look at the logs.
Right, I just mean that sandbox does not use a standalone linker script.
Yes it does, arch/sandbox/cpu/u-boot.lds.
It is not common to garbage-collect sections in executables, so far as I know.
I'm not sure how that's relevant here, honestly. And we didn't ask the toolchain community to invent this for us, it's a standard feature that we've found helpful for a number of reasons.
I am also not sure of the point ot it...since it is normally pretty easy to use Kconfig to control features.
That's not the point of the feature. The point is that we can discard unused code without having to rely on #ifdef and __maybe_unused.
That doesn't match with my understanding. An unused static will still produce a warning if it is unused.
Yes, you're misunderstanding how this feature works and why it's required. With v4 of my series, on sandbox and CMDLINE=n and LTO=n: /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_send_key': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:157: undefined reference to `cread_line_process_ch' /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_open': /home/trini/work/u-boot/u-boot/boot/scene_textline.c:222: undefined reference to `cli_cread_init' /usr/bin/ld: common/bootstage.o: in function `bootstage_fdt_add_report': /home/trini/work/u-boot/u-boot/common/bootstage.c:323: undefined reference to `working_fdt' /usr/bin/ld: common/cli.o: in function `run_commandf': /home/trini/work/u-boot/u-boot/common/cli.c:154: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' /usr/bin/ld: drivers/fastboot/fb_command.o: in function `fastboot_acmd_complete': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_command.c:350: undefined reference to `run_command' /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': /home/trini/work/u-boot/u-boot/lib/efi_loader/efi_device_path.c:1095: undefined reference to `efi_get_image_parameters' /usr/bin/ld: net/net.o: in function `net_auto_load': /home/trini/work/u-boot/u-boot/net/net.c:349: undefined reference to `tftp_start' collect2: error: ld returned 1 exit status
And with re-enabling gc-sections: /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot': /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command' collect2: error: ld returned 1 exit status
Because all of those other link failures are in unreachable code and the rest of the code base is already setup to know that x() can call y() and y() can just be undefined at link time if x() is never called and ends up discarded.
All of those problems were corrected by my series. Perhaps that accounts for the difference between them?
All of these are not real problems at this point in time is my point.
Just to take one example, working_fdt, while it might seem convenient to leave it in cmd/fdt.c it doesn't actually make sense, since the var is set when booting. Without that var DT fixups can't happen. So why not pick up my patch that moves it to boot/fdt_support.c where it belongs?
And the issue is that when the command line is disabled NOTHING uses "working_fdt". We don't need it. It would be discarded by real platforms, if we moved it.
To rephrase things another way, skimming the code, that bootm_find_images() does: if (IS_ENABLED(CONFIG_CMD_FDT)) set_working_fdt_addr(map_to_sysmem(images.ft_addr));
And so we never set "working_fdt" when CMDLINE is off (since CMD_FDT is off) IS the bug, and that needs to be fixed. That's when you should figure out where "working_fdt" should be (and also maybe re-name it or the local variable named "working_fdt" that the pxe_utils.c code uses) and not before then.
Looking at the list of errors, we should just fix them. They all indicate real problems with the structure of the code.
No, they do not. The codebase assume that we discard unreachable functions. This allows us to NOT have to use #ifdefs and also not have to make otherwise unreasonable code splits.
Another example is fastboot which needs a condition for CMDLINE.
This is an example where yes, the functionality requires CMDLINE. Because reading the code shows that fastboot will literally run "bootcmd", and so requires that to be available.
I honestly didn't realize sandbox hadn't had this critical feature enabled and I hope we haven't been doing too many odd shuffles of the codebase because of it.
I'm a bit unsure where you are heading with this. Ultimate we don't want to disable all the functionality when CMDLINE is disabled. I think with the addition of 5-6 patches in your series you can fix it.
Where I'm heading with this is to say that sandbox must work like a real platform does and discard unreachable code. You don't know what code is or is not really needed for functionality X and needs to be moved for it to not rely on what's in the command file vs elsewhere until you act like other platforms do.
It is remarkable that this sandbox 'feature' remained undiscovered for so long.
I'm going to let this thread die. I think I have said everything worth saying at this point.
Regards, Simon
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
- depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET
I don't think this is needed. After reconfiguring qemu_arm64 to be able to disable networking entirely, we still are able to build with EFI_LOADER enabled, and no warning / link errors.
Hi Tom,
On Wed, 18 Oct 2023 at 07:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETI don't think this is needed. After reconfiguring qemu_arm64 to be able to disable networking entirely, we still are able to build with EFI_LOADER enabled, and no warning / link errors.
Fair enough.
As of this patch the following errors are generated with -a ~CMDLINE :
+lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': +lib/efi_loader/efi_device_path.c:1095:(.text+0xe00): undefined reference to `efi_get_image_parameters'
which is why I added this patch. A later patch disables networking entirely with ~CMDLINE so that is why this problem goes away.
This series was basically built up by disabling CMDLINE and then fixing the errors one by one. I didn't go back and check whether (at the end) all the patches were needed.
I could do that and send a v4, if your more general concerns can be sorted out.
Regards, Simon
On Wed, Oct 18, 2023 at 09:23:42AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 07:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETI don't think this is needed. After reconfiguring qemu_arm64 to be able to disable networking entirely, we still are able to build with EFI_LOADER enabled, and no warning / link errors.
Fair enough.
As of this patch the following errors are generated with -a ~CMDLINE :
+lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': +lib/efi_loader/efi_device_path.c:1095:(.text+0xe00): undefined reference to `efi_get_image_parameters'
which is why I added this patch. A later patch disables networking entirely with ~CMDLINE so that is why this problem goes away.
This series was basically built up by disabling CMDLINE and then fixing the errors one by one. I didn't go back and check whether (at the end) all the patches were needed.
Yes, part of the issue with the series is that you didn't go and rework things after getting CMDLINE to link for sandbox, to see what else is or is not still needed.
I could do that and send a v4, if your more general concerns can be sorted out.
Don't worry about a v4 currently please, I'm working through the issues now picking out parts of v3 and re-working others.
Hi Tom,
On Wed, 18 Oct 2023 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 18, 2023 at 09:23:42AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 07:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETI don't think this is needed. After reconfiguring qemu_arm64 to be able to disable networking entirely, we still are able to build with EFI_LOADER enabled, and no warning / link errors.
Fair enough.
As of this patch the following errors are generated with -a ~CMDLINE :
+lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': +lib/efi_loader/efi_device_path.c:1095:(.text+0xe00): undefined reference to `efi_get_image_parameters'
which is why I added this patch. A later patch disables networking entirely with ~CMDLINE so that is why this problem goes away.
This series was basically built up by disabling CMDLINE and then fixing the errors one by one. I didn't go back and check whether (at the end) all the patches were needed.
Yes, part of the issue with the series is that you didn't go and rework things after getting CMDLINE to link for sandbox, to see what else is or is not still needed.
I could do that and send a v4, if your more general concerns can be sorted out.
Don't worry about a v4 currently please, I'm working through the issues now picking out parts of v3 and re-working others.
OK, thanks.
Regards, Simon
On Wed, Oct 18, 2023 at 4:29 PM Simon Glass sjg@chromium.org wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 07:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETI don't think this is needed. After reconfiguring qemu_arm64 to be able to disable networking entirely, we still are able to build with EFI_LOADER enabled, and no warning / link errors.
Fair enough.
As of this patch the following errors are generated with -a ~CMDLINE :
+lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': +lib/efi_loader/efi_device_path.c:1095:(.text+0xe00): undefined reference to `efi_get_image_parameters'
which is why I added this patch. A later patch disables networking entirely with ~CMDLINE so that is why this problem goes away.
This series was basically built up by disabling CMDLINE and then fixing the errors one by one. I didn't go back and check whether (at the end) all the patches were needed.
Was this just tested on sandbox? How do we know it's not going to regress a vast amount of actual devices?
I could do that and send a v4, if your more general concerns can be sorted out.
Regards, Simon
On Thu, Oct 19, 2023 at 11:39:49AM +0100, Peter Robinson wrote:
On Wed, Oct 18, 2023 at 4:29 PM Simon Glass sjg@chromium.org wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 07:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETI don't think this is needed. After reconfiguring qemu_arm64 to be able to disable networking entirely, we still are able to build with EFI_LOADER enabled, and no warning / link errors.
Fair enough.
As of this patch the following errors are generated with -a ~CMDLINE :
+lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': +lib/efi_loader/efi_device_path.c:1095:(.text+0xe00): undefined reference to `efi_get_image_parameters'
which is why I added this patch. A later patch disables networking entirely with ~CMDLINE so that is why this problem goes away.
This series was basically built up by disabling CMDLINE and then fixing the errors one by one. I didn't go back and check whether (at the end) all the patches were needed.
Was this just tested on sandbox? How do we know it's not going to regress a vast amount of actual devices?
It both shouldn't (DM_ETH is required), and this patch isn't moving forward either.
Hi Peter,
On Thu, 19 Oct 2023 at 04:40, Peter Robinson pbrobinson@gmail.com wrote:
On Wed, Oct 18, 2023 at 4:29 PM Simon Glass sjg@chromium.org wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 07:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is available, add it as an explicit dependency.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to update EFI_LOADER to depend on DM_ETH
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 13cad6342c36..fca4b3eef270 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -11,6 +11,7 @@ config EFI_LOADER # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK
depends on DM_ETH depends on !EFI_APP default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSETI don't think this is needed. After reconfiguring qemu_arm64 to be able to disable networking entirely, we still are able to build with EFI_LOADER enabled, and no warning / link errors.
Fair enough.
As of this patch the following errors are generated with -a ~CMDLINE :
+lib/efi_loader/efi_device_path.c:985:(.text+0xca4): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:987:(.text+0xca9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.c:993:(.text+0xcc9): undefined reference to `eth_get_dev' +/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name': +lib/efi_loader/efi_device_path.c:1095:(.text+0xe00): undefined reference to `efi_get_image_parameters'
which is why I added this patch. A later patch disables networking entirely with ~CMDLINE so that is why this problem goes away.
This series was basically built up by disabling CMDLINE and then fixing the errors one by one. I didn't go back and check whether (at the end) all the patches were needed.
Was this just tested on sandbox? How do we know it's not going to regress a vast amount of actual devices?
This was tested with buildman to check for config changes. The purpose of this series.is to get back to what worked a few years ago (CMDLINE could be disabled). If you don't disable CMDLINE, then it doesn't really change anything.
Perhaps we should add a build test for non-network EFI_LOADER if people are going to use it? I don't see any boards that do:
$ ./tools/qconfig.py -f EFI_LOADER ~NET 0 matches
That is a great tool for checking what is used in the code base.
Regards, Simon
This features has buffers and code which is behind an #ifdef at present. Move it into its own file, exporting functions as needed.
Drop add_idx_minus_one() since it is not used.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
boot/Kconfig | 3 +- common/Makefile | 7 +- common/cli_cread.c | 403 ++++++++++++++++++++++++++++++++++++++++++ common/cli_readline.c | 384 +--------------------------------------- include/cli.h | 30 ++++ 5 files changed, 441 insertions(+), 386 deletions(-) create mode 100644 common/cli_cread.c
diff --git a/boot/Kconfig b/boot/Kconfig index 6461f7ebd040..2fbe70245ec9 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -349,7 +349,8 @@ config PXE_UTILS
config BOOT_DEFAULTS bool # Common defaults for standard boot and distroboot - imply USE_BOOTCOMMAND + depends on CMDLINE + imply USE_BOOTCOMMAND if !SANDBOX select CMD_ENV_EXISTS select CMD_EXT2 select CMD_EXT4 diff --git a/common/Makefile b/common/Makefile index 637066ae6682..e22ced0c507f 100644 --- a/common/Makefile +++ b/common/Makefile @@ -38,6 +38,7 @@ 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_EDITING) += cli_cread.o obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
endif # !CONFIG_SPL_BUILD @@ -90,8 +91,10 @@ obj-y += eeprom/eeprom_field.o eeprom/eeprom_layout.o endif
obj-y += cli.o -obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o -obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o +obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_getch.o cli_simple.o cli_readline.o \ + cli_cread.o +obj-$(CONFIG_STM32MP1_DDR_INTERACTIVE) += cli_getch.o cli_simple.o \ + cli_readline.o cli_cread.o obj-$(CONFIG_DFU_OVER_USB) += dfu.o obj-y += command.o obj-$(CONFIG_$(SPL_TPL_)LOG) += log.o diff --git a/common/cli_cread.c b/common/cli_cread.c new file mode 100644 index 000000000000..19af27303cfc --- /dev/null +++ b/common/cli_cread.c @@ -0,0 +1,403 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) Copyright 2000 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * Add to readline cmdline-editing by + * (C) Copyright 2005 + * JinHua Luo, GuangDong Linux Center, luo.jinhua@gd-linux.com + */ + +#include <common.h> +#include <bootretry.h> +#include <cli.h> +#include <command.h> +#include <time.h> +#include <watchdog.h> +#include <asm/global_data.h> + +DECLARE_GLOBAL_DATA_PTR; + +/* + * cmdline-editing related codes from vivi. + * Author: Janghoon Lyu nandy@mizi.com + */ + +#define putnstr(str, n) printf("%.*s", (int)n, str) + +#define CTL_BACKSPACE ('\b') +#define DEL ((char)255) +#define DEL7 ((char)127) +#define CREAD_HIST_CHAR ('!') + +#define getcmd_putch(ch) putc(ch) +#define getcmd_cbeep() getcmd_putch('\a') + +#ifdef CONFIG_SPL_BUILD +#define HIST_MAX 3 +#define HIST_SIZE 32 +#else +#define HIST_MAX 20 +#define HIST_SIZE CONFIG_SYS_CBSIZE +#endif + +static int hist_max; +static int hist_add_idx; +static int hist_cur = -1; +static uint hist_num; + +static char *hist_list[HIST_MAX]; +static char hist_lines[HIST_MAX][HIST_SIZE + 1]; /* Save room for NULL */ + +static void getcmd_putchars(int count, int ch) +{ + int i; + + for (i = 0; i < count; i++) + getcmd_putch(ch); +} + +void hist_init(void) +{ + int i; + + hist_max = 0; + hist_add_idx = 0; + hist_cur = -1; + hist_num = 0; + + for (i = 0; i < HIST_MAX; i++) { + hist_list[i] = hist_lines[i]; + hist_list[i][0] = '\0'; + } +} + +void cread_add_to_hist(char *line) +{ + if (line[0] && line[0] != CREAD_HIST_CHAR) { + strcpy(hist_list[hist_add_idx], line); + + if (++hist_add_idx >= HIST_MAX) + hist_add_idx = 0; + + if (hist_add_idx > hist_max) + hist_max = hist_add_idx; + + hist_num++; + } + + hist_cur = hist_add_idx; +} + +char *hist_prev(void) +{ + char *ret; + int old_cur; + + if (hist_cur < 0) + return NULL; + + old_cur = hist_cur; + if (--hist_cur < 0) + hist_cur = hist_max; + + if (hist_cur == hist_add_idx) { + hist_cur = old_cur; + ret = NULL; + } else { + ret = hist_list[hist_cur]; + } + + return ret; +} + +char *hist_next(void) +{ + char *ret; + + if (hist_cur < 0) + return NULL; + + if (hist_cur == hist_add_idx) + return NULL; + + if (++hist_cur > hist_max) + hist_cur = 0; + + if (hist_cur == hist_add_idx) + ret = ""; + else + ret = hist_list[hist_cur]; + + return ret; +} + +void cread_print_hist_list(void) +{ + int i; + uint n; + + n = hist_num - hist_max; + + i = hist_add_idx + 1; + while (1) { + if (i > hist_max) + i = 0; + if (i == hist_add_idx) + break; + printf("%s\n", hist_list[i]); + n++; + i++; + } +} + +#define BEGINNING_OF_LINE() { \ + while (cls->num) { \ + getcmd_putch(CTL_BACKSPACE); \ + cls->num--; \ + } \ +} + +#define ERASE_TO_EOL() { \ + if (cls->num < cls->eol_num) { \ + printf("%*s", (int)(cls->eol_num - cls->num), ""); \ + do { \ + getcmd_putch(CTL_BACKSPACE); \ + } while (--cls->eol_num > cls->num); \ + } \ +} + +#define REFRESH_TO_EOL() { \ + if (cls->num < cls->eol_num) { \ + uint wlen = cls->eol_num - cls->num; \ + putnstr(buf + cls->num, wlen); \ + cls->num = cls->eol_num; \ + } \ +} + +static void cread_add_char(char ichar, int insert, uint *num, + uint *eol_num, char *buf, uint len) +{ + uint wlen; + + /* room ??? */ + if (insert || *num == *eol_num) { + if (*eol_num > len - 1) { + getcmd_cbeep(); + return; + } + (*eol_num)++; + } + + if (insert) { + wlen = *eol_num - *num; + if (wlen > 1) + memmove(&buf[*num + 1], &buf[*num], wlen - 1); + + buf[*num] = ichar; + putnstr(buf + *num, wlen); + (*num)++; + while (--wlen) + getcmd_putch(CTL_BACKSPACE); + } else { + /* echo the character */ + wlen = 1; + buf[*num] = ichar; + putnstr(buf + *num, wlen); + (*num)++; + } +} + +static void cread_add_str(char *str, int strsize, int insert, + uint *num, uint *eol_num, char *buf, uint len) +{ + while (strsize--) { + cread_add_char(*str, insert, num, eol_num, buf, len); + str++; + } +} + +int cread_line_process_ch(struct cli_line_state *cls, char ichar) +{ + char *buf = cls->buf; + + /* ichar=0x0 when error occurs in U-Boot getc */ + if (!ichar) + return -EAGAIN; + + if (ichar == '\n') { + putc('\n'); + buf[cls->eol_num] = '\0'; /* terminate the string */ + return 0; + } + + switch (ichar) { + case CTL_CH('a'): + BEGINNING_OF_LINE(); + break; + case CTL_CH('c'): /* ^C - break */ + *buf = '\0'; /* discard input */ + return -EINTR; + case CTL_CH('f'): + if (cls->num < cls->eol_num) { + getcmd_putch(buf[cls->num]); + cls->num++; + } + break; + case CTL_CH('b'): + if (cls->num) { + getcmd_putch(CTL_BACKSPACE); + cls->num--; + } + break; + case CTL_CH('d'): + if (cls->num < cls->eol_num) { + uint wlen; + + wlen = cls->eol_num - cls->num - 1; + if (wlen) { + memmove(&buf[cls->num], &buf[cls->num + 1], + wlen); + putnstr(buf + cls->num, wlen); + } + + getcmd_putch(' '); + do { + getcmd_putch(CTL_BACKSPACE); + } while (wlen--); + cls->eol_num--; + } + break; + case CTL_CH('k'): + ERASE_TO_EOL(); + break; + case CTL_CH('e'): + REFRESH_TO_EOL(); + break; + case CTL_CH('o'): + cls->insert = !cls->insert; + break; + case CTL_CH('w'): + if (cls->num) { + uint base, wlen; + + for (base = cls->num - 1; + base >= 0 && buf[base] == ' ';) + base--; + for (; base > 0 && buf[base - 1] != ' ';) + base--; + + /* now delete chars from base to cls->num */ + wlen = cls->num - base; + cls->eol_num -= wlen; + memmove(&buf[base], &buf[cls->num], + cls->eol_num - base + 1); + cls->num = base; + getcmd_putchars(wlen, CTL_BACKSPACE); + puts(buf + base); + getcmd_putchars(wlen, ' '); + getcmd_putchars(wlen + cls->eol_num - cls->num, + CTL_BACKSPACE); + } + break; + case CTL_CH('x'): + case CTL_CH('u'): + BEGINNING_OF_LINE(); + ERASE_TO_EOL(); + break; + case DEL: + case DEL7: + case 8: + if (cls->num) { + uint wlen; + + wlen = cls->eol_num - cls->num; + cls->num--; + memmove(&buf[cls->num], &buf[cls->num + 1], wlen); + getcmd_putch(CTL_BACKSPACE); + putnstr(buf + cls->num, wlen); + getcmd_putch(' '); + do { + getcmd_putch(CTL_BACKSPACE); + } while (wlen--); + cls->eol_num--; + } + break; + case CTL_CH('p'): + case CTL_CH('n'): + if (cls->history) { + char *hline; + + if (ichar == CTL_CH('p')) + hline = hist_prev(); + else + hline = hist_next(); + + if (!hline) { + getcmd_cbeep(); + break; + } + + /* nuke the current line */ + /* first, go home */ + BEGINNING_OF_LINE(); + + /* erase to end of line */ + ERASE_TO_EOL(); + + /* copy new line into place and display */ + strcpy(buf, hline); + cls->eol_num = strlen(buf); + REFRESH_TO_EOL(); + break; + } + break; + case '\t': + if (IS_ENABLED(CONFIG_AUTO_COMPLETE) && cls->cmd_complete) { + int num2, col; + + /* do not autocomplete when in the middle */ + if (cls->num < cls->eol_num) { + getcmd_cbeep(); + break; + } + + buf[cls->num] = '\0'; + col = strlen(cls->prompt) + cls->eol_num; + num2 = cls->num; + if (cmd_auto_complete(cls->prompt, buf, &num2, &col)) { + col = num2 - cls->num; + cls->num += col; + cls->eol_num += col; + } + break; + } + fallthrough; + default: + cread_add_char(ichar, cls->insert, &cls->num, &cls->eol_num, + buf, cls->len); + break; + } + + /* + * keep the string terminated...if we added a char at the end then we + * want a \0 after it + */ + buf[cls->eol_num] = '\0'; + + return -EAGAIN; +} + +void cli_cread_init(struct cli_line_state *cls, char *buf, uint buf_size) +{ + int init_len = strlen(buf); + + memset(cls, '\0', sizeof(struct cli_line_state)); + cls->insert = true; + cls->buf = buf; + cls->len = buf_size; + + if (init_len) + cread_add_str(buf, init_len, 0, &cls->num, &cls->eol_num, buf, + buf_size); +} diff --git a/common/cli_readline.c b/common/cli_readline.c index 06b8d4650447..ebbc979af6a9 100644 --- a/common/cli_readline.c +++ b/common/cli_readline.c @@ -60,383 +60,7 @@ static char *delete_char (char *buffer, char *p, int *colp, int *np, int plen) * Author: Janghoon Lyu nandy@mizi.com */
-#define putnstr(str, n) printf("%.*s", (int)n, str) - -#define CTL_BACKSPACE ('\b') -#define DEL ((char)255) -#define DEL7 ((char)127) -#define CREAD_HIST_CHAR ('!') - -#define getcmd_putch(ch) putc(ch) #define getcmd_getch() getchar() -#define getcmd_cbeep() getcmd_putch('\a') - -#ifdef CONFIG_SPL_BUILD -#define HIST_MAX 3 -#define HIST_SIZE 32 -#else -#define HIST_MAX 20 -#define HIST_SIZE CONFIG_SYS_CBSIZE -#endif - -static int hist_max; -static int hist_add_idx; -static int hist_cur = -1; -static unsigned hist_num; - -static char *hist_list[HIST_MAX]; -static char hist_lines[HIST_MAX][HIST_SIZE + 1]; /* Save room for NULL */ - -#define add_idx_minus_one() ((hist_add_idx == 0) ? hist_max : hist_add_idx-1) - -static void getcmd_putchars(int count, int ch) -{ - int i; - - for (i = 0; i < count; i++) - getcmd_putch(ch); -} - -static void hist_init(void) -{ - int i; - - hist_max = 0; - hist_add_idx = 0; - hist_cur = -1; - hist_num = 0; - - for (i = 0; i < HIST_MAX; i++) { - hist_list[i] = hist_lines[i]; - hist_list[i][0] = '\0'; - } -} - -static void cread_add_to_hist(char *line) -{ - strcpy(hist_list[hist_add_idx], line); - - if (++hist_add_idx >= HIST_MAX) - hist_add_idx = 0; - - if (hist_add_idx > hist_max) - hist_max = hist_add_idx; - - hist_num++; -} - -static char *hist_prev(void) -{ - char *ret; - int old_cur; - - if (hist_cur < 0) - return NULL; - - old_cur = hist_cur; - if (--hist_cur < 0) - hist_cur = hist_max; - - if (hist_cur == hist_add_idx) { - hist_cur = old_cur; - ret = NULL; - } else { - ret = hist_list[hist_cur]; - } - - return ret; -} - -static char *hist_next(void) -{ - char *ret; - - if (hist_cur < 0) - return NULL; - - if (hist_cur == hist_add_idx) - return NULL; - - if (++hist_cur > hist_max) - hist_cur = 0; - - if (hist_cur == hist_add_idx) - ret = ""; - else - ret = hist_list[hist_cur]; - - return ret; -} - -void cread_print_hist_list(void) -{ - int i; - uint n; - - n = hist_num - hist_max; - - i = hist_add_idx + 1; - while (1) { - if (i > hist_max) - i = 0; - if (i == hist_add_idx) - break; - printf("%s\n", hist_list[i]); - n++; - i++; - } -} - -#define BEGINNING_OF_LINE() { \ - while (cls->num) { \ - getcmd_putch(CTL_BACKSPACE); \ - cls->num--; \ - } \ -} - -#define ERASE_TO_EOL() { \ - if (cls->num < cls->eol_num) { \ - printf("%*s", (int)(cls->eol_num - cls->num), ""); \ - do { \ - getcmd_putch(CTL_BACKSPACE); \ - } while (--cls->eol_num > cls->num); \ - } \ -} - -#define REFRESH_TO_EOL() { \ - if (cls->num < cls->eol_num) { \ - uint wlen = cls->eol_num - cls->num; \ - putnstr(buf + cls->num, wlen); \ - cls->num = cls->eol_num; \ - } \ -} - -static void cread_add_char(char ichar, int insert, uint *num, - uint *eol_num, char *buf, uint len) -{ - uint wlen; - - /* room ??? */ - if (insert || *num == *eol_num) { - if (*eol_num > len - 1) { - getcmd_cbeep(); - return; - } - (*eol_num)++; - } - - if (insert) { - wlen = *eol_num - *num; - if (wlen > 1) - memmove(&buf[*num+1], &buf[*num], wlen-1); - - buf[*num] = ichar; - putnstr(buf + *num, wlen); - (*num)++; - while (--wlen) - getcmd_putch(CTL_BACKSPACE); - } else { - /* echo the character */ - wlen = 1; - buf[*num] = ichar; - putnstr(buf + *num, wlen); - (*num)++; - } -} - -static void cread_add_str(char *str, int strsize, int insert, - uint *num, uint *eol_num, char *buf, uint len) -{ - while (strsize--) { - cread_add_char(*str, insert, num, eol_num, buf, len); - str++; - } -} - -int cread_line_process_ch(struct cli_line_state *cls, char ichar) -{ - char *buf = cls->buf; - - /* ichar=0x0 when error occurs in U-Boot getc */ - if (!ichar) - return -EAGAIN; - - if (ichar == '\n') { - putc('\n'); - buf[cls->eol_num] = '\0'; /* terminate the string */ - return 0; - } - - switch (ichar) { - case CTL_CH('a'): - BEGINNING_OF_LINE(); - break; - case CTL_CH('c'): /* ^C - break */ - *buf = '\0'; /* discard input */ - return -EINTR; - case CTL_CH('f'): - if (cls->num < cls->eol_num) { - getcmd_putch(buf[cls->num]); - cls->num++; - } - break; - case CTL_CH('b'): - if (cls->num) { - getcmd_putch(CTL_BACKSPACE); - cls->num--; - } - break; - case CTL_CH('d'): - if (cls->num < cls->eol_num) { - uint wlen; - - wlen = cls->eol_num - cls->num - 1; - if (wlen) { - memmove(&buf[cls->num], &buf[cls->num + 1], - wlen); - putnstr(buf + cls->num, wlen); - } - - getcmd_putch(' '); - do { - getcmd_putch(CTL_BACKSPACE); - } while (wlen--); - cls->eol_num--; - } - break; - case CTL_CH('k'): - ERASE_TO_EOL(); - break; - case CTL_CH('e'): - REFRESH_TO_EOL(); - break; - case CTL_CH('o'): - cls->insert = !cls->insert; - break; - case CTL_CH('w'): - if (cls->num) { - uint base, wlen; - - for (base = cls->num - 1; - base >= 0 && buf[base] == ' ';) - base--; - for (; base > 0 && buf[base - 1] != ' ';) - base--; - - /* now delete chars from base to cls->num */ - wlen = cls->num - base; - cls->eol_num -= wlen; - memmove(&buf[base], &buf[cls->num], - cls->eol_num - base + 1); - cls->num = base; - getcmd_putchars(wlen, CTL_BACKSPACE); - puts(buf + base); - getcmd_putchars(wlen, ' '); - getcmd_putchars(wlen + cls->eol_num - cls->num, - CTL_BACKSPACE); - } - break; - case CTL_CH('x'): - case CTL_CH('u'): - BEGINNING_OF_LINE(); - ERASE_TO_EOL(); - break; - case DEL: - case DEL7: - case 8: - if (cls->num) { - uint wlen; - - wlen = cls->eol_num - cls->num; - cls->num--; - memmove(&buf[cls->num], &buf[cls->num + 1], wlen); - getcmd_putch(CTL_BACKSPACE); - putnstr(buf + cls->num, wlen); - getcmd_putch(' '); - do { - getcmd_putch(CTL_BACKSPACE); - } while (wlen--); - cls->eol_num--; - } - break; - case CTL_CH('p'): - case CTL_CH('n'): - if (cls->history) { - char *hline; - - if (ichar == CTL_CH('p')) - hline = hist_prev(); - else - hline = hist_next(); - - if (!hline) { - getcmd_cbeep(); - break; - } - - /* nuke the current line */ - /* first, go home */ - BEGINNING_OF_LINE(); - - /* erase to end of line */ - ERASE_TO_EOL(); - - /* copy new line into place and display */ - strcpy(buf, hline); - cls->eol_num = strlen(buf); - REFRESH_TO_EOL(); - break; - } - break; - case '\t': - if (IS_ENABLED(CONFIG_AUTO_COMPLETE) && cls->cmd_complete) { - int num2, col; - - /* do not autocomplete when in the middle */ - if (cls->num < cls->eol_num) { - getcmd_cbeep(); - break; - } - - buf[cls->num] = '\0'; - col = strlen(cls->prompt) + cls->eol_num; - num2 = cls->num; - if (cmd_auto_complete(cls->prompt, buf, &num2, &col)) { - col = num2 - cls->num; - cls->num += col; - cls->eol_num += col; - } - break; - } - fallthrough; - default: - cread_add_char(ichar, cls->insert, &cls->num, &cls->eol_num, - buf, cls->len); - break; - } - - /* - * keep the string terminated...if we added a char at the end then we - * want a \0 after it - */ - buf[cls->eol_num] = '\0'; - - return -EAGAIN; -} - -void cli_cread_init(struct cli_line_state *cls, char *buf, uint buf_size) -{ - int init_len = strlen(buf); - - memset(cls, '\0', sizeof(struct cli_line_state)); - cls->insert = true; - cls->buf = buf; - cls->len = buf_size; - - if (init_len) - cread_add_str(buf, init_len, 0, &cls->num, &cls->eol_num, buf, - buf_size); -}
static int cread_line(const char *const prompt, char *buf, unsigned int *len, int timeout) @@ -484,19 +108,13 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, } *len = cls->eol_num;
- if (buf[0] && buf[0] != CREAD_HIST_CHAR) - cread_add_to_hist(buf); - hist_cur = hist_add_idx; + cread_add_to_hist(buf);
return 0; }
#else /* !CONFIG_CMDLINE_EDITING */
-static inline void hist_init(void) -{ -} - static int cread_line(const char *const prompt, char *buf, unsigned int *len, int timeout) { diff --git a/include/cli.h b/include/cli.h index e183d5613697..bca24c8c565f 100644 --- a/include/cli.h +++ b/include/cli.h @@ -280,4 +280,34 @@ void cli_cread_init(struct cli_line_state *cls, char *buf, uint buf_size); /** cread_print_hist_list() - Print the command-line history list */ void cread_print_hist_list(void);
+/** + * hist_prev() - Get the previous line of history + * + * Returns: Previous history line, or NULL if none + */ +char *hist_prev(void); + +/** + * hist_next() - Get the next line of history + * + * Returns: Next history line, or NULL if none + */ +char *hist_next(void); + +/** + * cread_add_to_hist() - Add a line to the history buffer + * + * The line is added if it is non-empty and doesn't start with CREAD_HIST_CHAR + * This also sets hist_cur to hist_add_idx whether or not anything was added + * + * @line: Line to add + */ +void cread_add_to_hist(char *line); + +#ifdef CONFIG_CMDLINE_EDITING +void hist_init(void); +#else +static inline void hist_init(void) {} +#endif + #endif
At present EXPO requires CMDLINE since it uses cli_readline, which is enabled by CMDLINE. It should be possible to enter lines of text to an expo without having CMDLINE available.
Fix this dependency by creating a new Kconfig for cli_readline
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Add new patch with a Kconfig for command-line entry
boot/Kconfig | 1 + cmd/Kconfig | 1 + common/Kconfig | 7 +++++++ common/Makefile | 2 +- 4 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/boot/Kconfig b/boot/Kconfig index 2fbe70245ec9..e71de0647bc5 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -649,6 +649,7 @@ config EXPO bool "Support for expos - groups of scenes displaying a UI" depends on VIDEO default y if BOOTMETH_VBE + select CLI_READLINE help An expo is a way of presenting and collecting information from the user. It consists of a collection of 'scenes' of which only one is diff --git a/cmd/Kconfig b/cmd/Kconfig index 3b4112d9f319..5cb45f9c025e 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -3,6 +3,7 @@ menu "Command line interface" config CMDLINE bool "Support U-Boot commands" default y + select CLI_READLINE help Enable U-Boot's command-line functions. This provides a means to enter commands into U-Boot for a wide variety of purposes. It diff --git a/common/Kconfig b/common/Kconfig index 1ffb055744d9..df5bac646491 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -1164,3 +1164,10 @@ endif
config IO_TRACE bool + +config CLI_READLINE + bool + help + Enables support for reading a line of text from the user, This + feature is used by the command-line interpreter and also by expo, + which needs to read text when textline objects are used. diff --git a/common/Makefile b/common/Makefile index e22ced0c507f..a9c18c61895c 100644 --- a/common/Makefile +++ b/common/Makefile @@ -38,7 +38,7 @@ 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_EDITING) += cli_cread.o +obj-$(CONFIG_CLI_READLINE) += cli_cread.o obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
endif # !CONFIG_SPL_BUILD
At present EXPO requires CMDLINE since it uses cli_readline, which needs history enabled. It should be possible to enter lines of text into an expo without having history available.
Fix this dependency by creating a new Kconfig for cmdline history. Adjust the code to use the correct condiition.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Add new patch with a Kconfig for command-line entry
cmd/Kconfig | 15 +++++++++++++-- common/cli_cread.c | 24 ++++++++++++++---------- include/cli.h | 3 ++- 3 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5cb45f9c025e..491737ca8ba7 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -29,8 +29,19 @@ config CMDLINE_EDITING depends on CMDLINE default y help - Enable editing and History functions for interactive command line - input operations + Enable an editing function for interactive, command-line-input + operations. This allows moving the cursor back and forth within + the line, inserting and deleting characters, etc. + +config CMDLINE_HISTORY + bool "Enable command-line history" + depends on CMDLINE_EDITING + default y + help + Enable a history function for interactive, command-line-input + operations. This maintains a small buffer of previously entered + commands, allowing the user to enter or edit old commands without + having to retype them.
config CMDLINE_PS_SUPPORT bool "Enable support for changing the command prompt string at run-time" diff --git a/common/cli_cread.c b/common/cli_cread.c index 19af27303cfc..33c678e89f3d 100644 --- a/common/cli_cread.c +++ b/common/cli_cread.c @@ -28,11 +28,12 @@ DECLARE_GLOBAL_DATA_PTR; #define CTL_BACKSPACE ('\b') #define DEL ((char)255) #define DEL7 ((char)127) -#define CREAD_HIST_CHAR ('!')
#define getcmd_putch(ch) putc(ch) #define getcmd_cbeep() getcmd_putch('\a')
+#define CREAD_HIST_CHAR ('!') + #ifdef CONFIG_SPL_BUILD #define HIST_MAX 3 #define HIST_SIZE 32 @@ -41,14 +42,6 @@ DECLARE_GLOBAL_DATA_PTR; #define HIST_SIZE CONFIG_SYS_CBSIZE #endif
-static int hist_max; -static int hist_add_idx; -static int hist_cur = -1; -static uint hist_num; - -static char *hist_list[HIST_MAX]; -static char hist_lines[HIST_MAX][HIST_SIZE + 1]; /* Save room for NULL */ - static void getcmd_putchars(int count, int ch) { int i; @@ -57,6 +50,16 @@ static void getcmd_putchars(int count, int ch) getcmd_putch(ch); }
+#ifdef CONFIG_CMDLINE_HISTORY + +static int hist_max; +static int hist_add_idx; +static int hist_cur = -1; +static uint hist_num; + +static char *hist_list[HIST_MAX]; +static char hist_lines[HIST_MAX][HIST_SIZE + 1]; /* Save room for NULL */ + void hist_init(void) { int i; @@ -150,6 +153,7 @@ void cread_print_hist_list(void) i++; } } +#endif /* CMDLINE_HISTORY */
#define BEGINNING_OF_LINE() { \ while (cls->num) { \ @@ -325,7 +329,7 @@ int cread_line_process_ch(struct cli_line_state *cls, char ichar) break; case CTL_CH('p'): case CTL_CH('n'): - if (cls->history) { + if (IS_ENABLED(CONFIG_CMDLINE_HISTORY) && cls->history) { char *hline;
if (ichar == CTL_CH('p')) diff --git a/include/cli.h b/include/cli.h index bca24c8c565f..45a338f709f7 100644 --- a/include/cli.h +++ b/include/cli.h @@ -294,6 +294,7 @@ char *hist_prev(void); */ char *hist_next(void);
+#ifdef CONFIG_CMDLINE_HISTORY /** * cread_add_to_hist() - Add a line to the history buffer * @@ -304,10 +305,10 @@ char *hist_next(void); */ void cread_add_to_hist(char *line);
-#ifdef CONFIG_CMDLINE_EDITING void hist_init(void); #else static inline void hist_init(void) {} +static inline void cread_add_to_hist(char *line) {} #endif
#endif
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 ---
(no changes since v1)
arch/Kconfig | 5 +++++ boot/Kconfig | 6 +++--- 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 - 8 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig index 19f2891ba1c5..64d0a5548942 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -208,6 +208,11 @@ config SANDBOX imply PHYSMEM imply GENERATE_ACPI_TABLE imply BINMAN + imply SYSRESET_CMD_POWEROFF + imply SUPPORT_EXTENSION_SCAN + 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/boot/Kconfig b/boot/Kconfig index e71de0647bc5..bab646bcac34 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -433,8 +433,8 @@ config BOOTSTD_FULL config BOOTSTD_DEFAULTS bool "Select some common defaults for standard boot" depends on BOOTSTD - imply USE_BOOTCOMMAND - select BOOT_DEFAULTS + imply USE_BOOTCOMMAND if !SANDBOX + select BOOT_DEFAULTS if CMDLINE select BOOTMETH_DISTRO help These are not required but are commonly needed to support a good @@ -443,7 +443,7 @@ config BOOTSTD_DEFAULTS
config BOOTSTD_BOOTCOMMAND bool "Use bootstd to boot" - default y if !DISTRO_DEFAULTS + default y if !DISTRO_DEFAULTS && !SANDBOX help Enable this to select a default boot-command suitable for booting with standard boot. This can be overridden by the board if needed, 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 d39e54f98d25..263cc013d68e 100644 --- a/configs/sandbox_noinst_defconfig +++ b/configs/sandbox_noinst_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_spl_defconfig b/configs/sandbox_spl_defconfig index 4a67af2f088f..a84c6d2ba248 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 8d76f19729b9..60fb1701708a 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
If this option is disabled, commands should not be available. Convert the CMDLINE option into a menuconfig and make every command in cmd/Kconfig depend on it.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
cmd/Kconfig | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 491737ca8ba7..b6fdbc6ff3b1 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 select CLI_READLINE help @@ -12,9 +10,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 @@ -26,7 +25,6 @@ config HUSH_PARSER
config CMDLINE_EDITING bool "Enable command line editing" - depends on CMDLINE default y help Enable an editing function for interactive, command-line-input @@ -52,15 +50,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. @@ -98,8 +94,7 @@ config SYS_CBSIZE
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 @@ -238,7 +233,6 @@ menu "Boot commands"
config CMD_BOOTD bool "bootd" - depends on CMDLINE default y help Run the command stored in the environment "bootcmd", i.e. @@ -405,7 +399,7 @@ config SYS_BOOTM_LEN
config CMD_BOOTEFI bool "bootefi" - depends on EFI_LOADER && CMDLINE + depends on EFI_LOADER default y help Boot an EFI image from memory. @@ -437,7 +431,6 @@ source lib/efi_selftest/Kconfig
config CMD_BOOTMENU bool "bootmenu" - depends on CMDLINE select MENU select CHARSET help @@ -504,7 +497,6 @@ config CMD_GO
config CMD_RUN bool "run" - depends on CMDLINE default y help Run the command in the given environment variable. @@ -595,7 +587,6 @@ menu "Environment commands"
config CMD_ASKENV bool "ask for env variable" - depends on CMDLINE help Ask for environment variable
@@ -1715,7 +1706,6 @@ if NET
menuconfig CMD_NET bool "Network commands" - depends on CMDLINE default y imply NETDEVICES
@@ -2019,7 +2009,6 @@ config CMD_ETHSW
config CMD_PXE bool "pxe" - depends on CMDLINE select PXE_UTILS help Boot image via network using PXE protocol @@ -2154,7 +2143,6 @@ config CMD_EFICONFIG
config CMD_EXCEPTION bool "exception - raise exception" - depends on CMDLINE depends on ARM || RISCV || SANDBOX || X86 help Enable the 'exception' command which allows to raise an exception. @@ -2255,14 +2243,12 @@ config CMD_SOUND
config CMD_SYSBOOT bool "sysboot" - depends on CMDLINE select PXE_UTILS help Boot image via local extlinux.conf file
config CMD_QFW bool "qfw" - depends on CMDLINE select QFW help This provides access to the QEMU firmware interface. The main @@ -2911,4 +2897,5 @@ config CMD_MESON default y help Enable useful commands for the Meson Soc family developed by Amlogic Inc. -endmenu + +endif # CMDLINE
Use 'imply' rather than 'select' for command-related options, so that it is possible to build sandbox without CONFIG_CMDLINE enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
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 64d0a5548942..e85134e9346c 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -134,7 +134,6 @@ config SANDBOX select ARCH_SUPPORTS_LTO select BOARD_LATE_INIT select BZIP2 - select CMD_POWEROFF select DM select DM_EVENT select DM_FUZZING_ENGINE @@ -152,10 +151,8 @@ config SANDBOX select PCI_ENDPOINT select SPI select SUPPORT_OF_CONTROL - select SYSRESET_CMD_POWEROFF select SYS_CACHE_SHIFT_4 select IRQ - select SUPPORT_EXTENSION_SCAN select SUPPORT_ACPI imply BITREVERSE select BLOBLIST @@ -167,6 +164,9 @@ config SANDBOX imply CMD_IO imply CMD_IOTRACE imply CMD_LZMADEC + imply CMD_POWEROFF + imply SYSRESET_CMD_POWEROFF + imply SUPPORT_EXTENSION_SCAN imply CMD_SF imply CMD_SF_TEST imply CRC32_VERIFY
Since we can now cleanly disable CMDLINE when needed, drop the rules which discard the command code. It will not be built in the first place.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Add new patch to drop discarding of command linker-lists
arch/arm/cpu/u-boot.lds | 3 --- arch/x86/cpu/u-boot-64.lds | 4 ---- arch/x86/cpu/u-boot-spl.lds | 4 ---- arch/x86/cpu/u-boot.lds | 4 ---- 4 files changed, 15 deletions(-)
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index fc4f63d83489..7724c9332c3b 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -14,9 +14,6 @@ OUTPUT_ARCH(arm) ENTRY(_start) SECTIONS { -#ifndef CONFIG_CMDLINE - /DISCARD/ : { *(__u_boot_list_2_cmd_*) } -#endif #if defined(CONFIG_ARMV7_SECURE_BASE) && defined(CONFIG_ARMV7_NONSEC) /* * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not diff --git a/arch/x86/cpu/u-boot-64.lds b/arch/x86/cpu/u-boot-64.lds index d0398ff00d71..00a6d8691211 100644 --- a/arch/x86/cpu/u-boot-64.lds +++ b/arch/x86/cpu/u-boot-64.lds @@ -11,10 +11,6 @@ ENTRY(_start)
SECTIONS { -#ifndef CONFIG_CMDLINE - /DISCARD/ : { *(__u_boot_list_2_cmd_*) } -#endif - #ifdef CONFIG_TEXT_BASE . = CONFIG_TEXT_BASE; /* Location of bootcode in flash */ #endif diff --git a/arch/x86/cpu/u-boot-spl.lds b/arch/x86/cpu/u-boot-spl.lds index a0a2a06a18cd..50b4b1608552 100644 --- a/arch/x86/cpu/u-boot-spl.lds +++ b/arch/x86/cpu/u-boot-spl.lds @@ -11,10 +11,6 @@ ENTRY(_start)
SECTIONS { -#ifndef CONFIG_CMDLINE - /DISCARD/ : { *(__u_boot_list_2_cmd_*) } -#endif - . = IMAGE_TEXT_BASE; /* Location of bootcode in flash */ __text_start = .; .text : { diff --git a/arch/x86/cpu/u-boot.lds b/arch/x86/cpu/u-boot.lds index a31f4220a000..c418ff44aa08 100644 --- a/arch/x86/cpu/u-boot.lds +++ b/arch/x86/cpu/u-boot.lds @@ -11,10 +11,6 @@ ENTRY(_start)
SECTIONS { -#ifndef CONFIG_CMDLINE - /DISCARD/ : { *(__u_boot_list_2_cmd_*) } -#endif - . = CONFIG_TEXT_BASE; /* Location of bootcode in flash */ __text_start = .;
The declaration of U_BOOT_ENV_LOCATION() uses a different #ifdef condition from the code it calls. Use the same condition to avoid a build warning if CONFIG_CMD_SAVEENV is disabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Add new patch to unify the U_BOOT_ENV_LOCATION conditions
env/mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c index cb14bbb58f13..da84cddd74f0 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -495,7 +495,7 @@ U_BOOT_ENV_LOCATION(mmc) = { .location = ENVL_MMC, ENV_NAME("MMC") .load = env_mmc_load, -#ifndef CONFIG_SPL_BUILD +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD) .save = env_save_ptr(env_mmc_save), .erase = ENV_ERASE_PTR(env_mmc_erase) #endif
The U_BOOT_CMD_COMPLETE() macro has a semicolon at the end, perhaps inadvertently. Some code has taken advantage of this.
Tidy this up by dropping the semicolon from the macro and adding it to macro invocations as required.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Add new patch to tidy up semicolon after command macros
board/freescale/common/vid.c | 2 +- board/xilinx/common/fru.c | 2 +- board/xilinx/versal/cmds.c | 2 +- board/xilinx/zynqmp/cmds.c | 2 +- cmd/btrfs.c | 2 +- cmd/eeprom.c | 2 +- cmd/ext2.c | 4 ++-- cmd/fs.c | 8 ++++---- cmd/pinmux.c | 2 +- cmd/qfw.c | 2 +- include/command.h | 2 +- 11 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/board/freescale/common/vid.c b/board/freescale/common/vid.c index 5ec3f2a76b19..fc5d400cfe18 100644 --- a/board/freescale/common/vid.c +++ b/board/freescale/common/vid.c @@ -793,4 +793,4 @@ U_BOOT_CMD( vdd_read, 1, 0, do_vdd_read, "read VDD", " - Read the voltage specified in mV" -) +); diff --git a/board/xilinx/common/fru.c b/board/xilinx/common/fru.c index c916c3d6b4c8..12b21317496a 100644 --- a/board/xilinx/common/fru.c +++ b/board/xilinx/common/fru.c @@ -85,4 +85,4 @@ U_BOOT_CMD( fru, 8, 1, do_fru, "FRU table info", fru_help_text -) +); diff --git a/board/xilinx/versal/cmds.c b/board/xilinx/versal/cmds.c index 9cc2cdcebf1c..2a74e49aedec 100644 --- a/board/xilinx/versal/cmds.c +++ b/board/xilinx/versal/cmds.c @@ -98,4 +98,4 @@ U_BOOT_LONGHELP(versal, U_BOOT_CMD(versal, 4, 1, do_versal, "versal sub-system", versal_help_text -) +); diff --git a/board/xilinx/zynqmp/cmds.c b/board/xilinx/zynqmp/cmds.c index f1f3eff501e1..9524688f27d9 100644 --- a/board/xilinx/zynqmp/cmds.c +++ b/board/xilinx/zynqmp/cmds.c @@ -427,4 +427,4 @@ U_BOOT_CMD( zynqmp, 9, 1, do_zynqmp, "ZynqMP sub-system", zynqmp_help_text -) +); diff --git a/cmd/btrfs.c b/cmd/btrfs.c index 98daea99e9ed..2843835d08b8 100644 --- a/cmd/btrfs.c +++ b/cmd/btrfs.c @@ -24,4 +24,4 @@ U_BOOT_CMD(btrsubvol, 3, 1, do_btrsubvol, "list subvolumes of a BTRFS filesystem", "<interface> <dev[:part]>\n" " - List subvolumes of a BTRFS filesystem." -) +); diff --git a/cmd/eeprom.c b/cmd/eeprom.c index 0b6ca8c505fb..322765ad02a0 100644 --- a/cmd/eeprom.c +++ b/cmd/eeprom.c @@ -435,4 +435,4 @@ U_BOOT_CMD( "The values which can be provided with the -l option are:\n" CONFIG_EEPROM_LAYOUT_HELP_STRING"\n" #endif -) +); diff --git a/cmd/ext2.c b/cmd/ext2.c index 57a99516a6ac..a0ce0cf5796b 100644 --- a/cmd/ext2.c +++ b/cmd/ext2.c @@ -42,7 +42,7 @@ U_BOOT_CMD( "list files in a directory (default /)", "<interface> <dev[:part]> [directory]\n" " - list files from 'dev' on 'interface' in a 'directory'" -) +);
U_BOOT_CMD( ext2load, 6, 0, do_ext2load, @@ -50,4 +50,4 @@ U_BOOT_CMD( "<interface> [<dev[:part]> [addr [filename [bytes [pos]]]]]\n" " - load binary file 'filename' from 'dev' on 'interface'\n" " to address 'addr' from ext2 filesystem." -) +); diff --git a/cmd/fs.c b/cmd/fs.c index 6044f73af5b4..46cb43dcdb5b 100644 --- a/cmd/fs.c +++ b/cmd/fs.c @@ -39,7 +39,7 @@ U_BOOT_CMD( " If 'bytes' is 0 or omitted, the file is read until the end.\n" " 'pos' gives the file byte position to start reading from.\n" " If 'pos' is 0 or omitted, the file is read from the start." -) +);
static int do_save_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -56,7 +56,7 @@ U_BOOT_CMD( " 'bytes' gives the size to save in bytes and is mandatory.\n" " 'pos' gives the file byte position to start writing to.\n" " If 'pos' is 0 or omitted, the file is written from the start." -) +);
static int do_ls_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -70,7 +70,7 @@ U_BOOT_CMD( "<interface> [<dev[:part]> [directory]]\n" " - List files in directory 'directory' of partition 'part' on\n" " device type 'interface' instance 'dev'." -) +);
static int do_ln_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -84,7 +84,7 @@ U_BOOT_CMD( "<interface> <dev[:part]> target linkname\n" " - create a symbolic link to 'target' with the name 'linkname' on\n" " device type 'interface' instance 'dev'." -) +);
static int do_fstype_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) diff --git a/cmd/pinmux.c b/cmd/pinmux.c index f17cf4110d9f..105f01eaafff 100644 --- a/cmd/pinmux.c +++ b/cmd/pinmux.c @@ -178,4 +178,4 @@ U_BOOT_CMD(pinmux, CONFIG_SYS_MAXARGS, 1, do_pinmux, "list - list UCLASS_PINCTRL devices\n" "pinmux dev [pincontroller-name] - select pin-controller device\n" "pinmux status [-a | pin-name] - print pin-controller muxing [for all | for pin-name]\n" -) +); diff --git a/cmd/qfw.c b/cmd/qfw.c index d6ecfa60d5a7..1b8c775ebf5a 100644 --- a/cmd/qfw.c +++ b/cmd/qfw.c @@ -121,4 +121,4 @@ U_BOOT_CMD( " - list : print firmware(s) currently loaded\n" " - cpus : print online cpu number\n" " - load <kernel addr> <initrd addr> : load kernel and initrd (if any), and setup for zboot\n" -) +); diff --git a/include/command.h b/include/command.h index 6262365e128f..5bd3ecbe8f91 100644 --- a/include/command.h +++ b/include/command.h @@ -390,7 +390,7 @@ int cmd_source_script(ulong addr, const char *fit_uname, const char *confname); #define U_BOOT_CMD_COMPLETE(_name, _maxargs, _rep, _cmd, _usage, _help, _comp) \ ll_entry_declare(struct cmd_tbl, _name, cmd) = \ U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd, \ - _usage, _help, _comp); + _usage, _help, _comp)
#define U_BOOT_CMDREP_COMPLETE(_name, _maxargs, _cmd_rep, _usage, \ _help, _comp) \
Now that everything is working, add a test to make sure that this builds correctly.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Rebase on Tom's LONGHELP series - Correct 'of' typo
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])
On Mon, Oct 16, 2023 at 04:28:23PM -0600, Simon Glass wrote:
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
On Mon, Oct 16, 2023 at 04:28:23PM -0600, Simon Glass wrote:
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
Changes in v3:
- Rebase on Tom's LONGHELP series
- Correct 'of' typo
test/py/tests/test_sandbox_opts.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/py/tests/test_sandbox_opts.py
This is not doing the equivalent of: make sandbox_config sed -i -e 's/CONFIG_CMDLINE=y/CONFIG_CMDLINE=n/' .config make oldconfig make all
Hi Tom,
On Wed, 18 Oct 2023 at 20:32, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:23PM -0600, Simon Glass wrote:
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
Changes in v3:
- Rebase on Tom's LONGHELP series
- Correct 'of' typo
test/py/tests/test_sandbox_opts.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/py/tests/test_sandbox_opts.py
This is not doing the equivalent of: make sandbox_config sed -i -e 's/CONFIG_CMDLINE=y/CONFIG_CMDLINE=n/' .config make oldconfig make all
Buildman should do that automatically...is something failing?
Regards, Simon
On Thu, Oct 19, 2023 at 08:00:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 20:32, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:23PM -0600, Simon Glass wrote:
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
Changes in v3:
- Rebase on Tom's LONGHELP series
- Correct 'of' typo
test/py/tests/test_sandbox_opts.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/py/tests/test_sandbox_opts.py
This is not doing the equivalent of: make sandbox_config sed -i -e 's/CONFIG_CMDLINE=y/CONFIG_CMDLINE=n/' .config make oldconfig make all
Buildman should do that automatically...is something failing?
Yes. https://source.denx.de/u-boot/u-boot/-/jobs/716843 fails, but shouldn't, and it's only the no LTO job, so maybe there's some conflict there?
Hi Tom,
On Thu, 19 Oct 2023 at 07:57, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 19, 2023 at 08:00:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 18 Oct 2023 at 20:32, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 16, 2023 at 04:28:23PM -0600, Simon Glass wrote:
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
Changes in v3:
- Rebase on Tom's LONGHELP series
- Correct 'of' typo
test/py/tests/test_sandbox_opts.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/py/tests/test_sandbox_opts.py
This is not doing the equivalent of: make sandbox_config sed -i -e 's/CONFIG_CMDLINE=y/CONFIG_CMDLINE=n/' .config make oldconfig make all
Buildman should do that automatically...is something failing?
Yes. https://source.denx.de/u-boot/u-boot/-/jobs/716843 fails, but shouldn't, and it's only the no LTO job, so maybe there's some conflict there?
For me this was the EFI patch to make it depend on net. Perhaps you hit a different one?
I am seeing build errors with ~CMDLINE ~LTO with your series.
Regards, Simon
participants (6)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Peter Robinson -
Sean Anderson -
Simon Glass -
Tom Rini