[PATCH 00/25] Tidy up use of CONFIG_CMDLINE

[PATCH 00/25] Tidy up use of CONFIG_CMDLINE

[Simon Glass]

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 of CONFIG_CMDLINE is
disabled, e.g. networking and most booting options. Further work is
needed to make the option more generally useful.


Simon Glass (25):
  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
  treewide: Correct use of long help
  test: Make UNIT_TEST depend on CMDLINE
  tegra: Change #ifdef for nop
  fastboot: Avoid depending on CMDLINE
  cli: Always build cli_getch
  cmd: Use an #ifdef around run_commandf()
  Move bootmenu_conv_key() into its own file
  armffa: Correct command help condition
  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: Depend on CMDLINE for efi_loader
  cmd: Make all commands depend on CMDLINE
  sandbox: Avoid requiring cmdline
  sandbox: Add a test for disabling CONFIG_CMDLINE

 arch/Kconfig                               |   6 +-
 arch/arm/lib/bootm.c                       |   2 +
 arch/arm/mach-imx/cmd_dek.c                |   3 +-
 arch/arm/mach-imx/cmd_mfgprot.c            |   3 +-
 arch/arm/mach-imx/imx8/snvs_security_sc.c  |  10 ++
 arch/arm/mach-stm32mp/cmd_stm32key.c       |   2 +
 board/freescale/common/cmd_esbc_validate.c |   3 +-
 board/kontron/sl28/cmds.c                  |   2 +
 boot/Kconfig                               |  42 ++++---
 boot/bootm.c                               |  10 +-
 boot/fdt_support.c                         |   5 +
 cmd/Kconfig                                |  25 ++--
 cmd/Makefile                               |   2 +-
 cmd/adc.c                                  |   2 +
 cmd/arm/exception.c                        |   2 +
 cmd/arm/exception64.c                      |   2 +
 cmd/armffa.c                               |   2 +
 cmd/axi.c                                  |   2 +
 cmd/blob.c                                 |   2 +
 cmd/cyclic.c                               |   2 +
 cmd/fdt.c                                  |   5 -
 cmd/mux.c                                  |   2 +
 cmd/nvedit.c                               | 122 +------------------
 cmd/osd.c                                  |   2 +
 cmd/pcap.c                                 |   2 +
 cmd/riscv/exception.c                      |   2 +
 cmd/sandbox/exception.c                    |   2 +
 cmd/scp03.c                                |   2 +
 cmd/wdt.c                                  |   2 +
 cmd/x86/exception.c                        |   2 +
 common/Kconfig                             |   5 +
 common/Makefile                            |   3 +-
 common/cli.c                               |   2 +
 common/cli_getch.c                         |   1 +
 common/log.c                               |   4 +-
 common/menu.c                              |  40 -------
 common/menu_key.c                          |  49 ++++++++
 drivers/fastboot/fb_command.c              |   3 +-
 drivers/fastboot/fb_common.c               |  15 ++-
 drivers/video/Kconfig                      |   2 +-
 drivers/video/console_truetype.c           |   4 +
 env/Makefile                               |   1 +
 env/env_set.c                              | 132 +++++++++++++++++++++
 include/bootm.h                            |  15 ++-
 include/env_internal.h                     |  23 ++++
 include/k210/pll.h                         |   2 +-
 lib/efi_loader/Kconfig                     |   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 +-
 53 files changed, 388 insertions(+), 221 deletions(-)
 create mode 100644 common/menu_key.c
 create mode 100644 env/env_set.c
 create mode 100644 test/py/tests/test_sandbox_opts.py

-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 01/25] buildman: Use oldconfig when adjusting the config

[Simon Glass]

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

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

 tools/buildman/builder.py       | 2 +-
 tools/buildman/builderthread.py | 6 ++++++
 tools/buildman/func_test.py     | 4 +++-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index ecbd368c47a5..24a94e8ae3e5 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 25f460c207db..035d4e9f235c 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -423,6 +423,12 @@ class BuilderThread(threading.Thread):
 
         # Now do the build, if everything looks OK
         if result.return_code == 0:
+            if adjust_cfg:
+                oldc_args = list(args) + ['oldconfig']
+                oldc_result = self.make(commit, brd, 'oldconfig', cwd,
+                                        *oldc_args, env=env)
+                if oldc_result.return_code:
+                    return oldc_result
             result = self._build(commit, brd, cwd, args, env, cmd_list,
                                  config_only)
             if adjust_cfg:
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 55dd494fe8ee..6b88ed815d65 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -439,6 +439,8 @@ class TestFunctional(unittest.TestCase):
             tools.write_file(fname, b'CONFIG_SOMETHING=1')
             return command.CommandResult(return_code=0,
                     combined='Test configuration complete')
+        elif stage == 'oldconfig':
+            return command.CommandResult(return_code=0)
         elif stage == 'build':
             stderr = ''
             fname = os.path.join(cwd or '', out_dir, 'u-boot')
@@ -461,7 +463,7 @@ Some images are invalid'''
             return command.CommandResult(return_code=0)
 
         # Not handled, so abort
-        print('make', stage)
+        print('_HandleMake failure: make', stage)
         sys.exit(1)
 
     # Example function to print output lines
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 02/25] bootstd: Correct dependencies on CMDLINE

[Simon Glass]

With recent changes 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>
---

 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
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 03/25] autoboot: Correct dependencies on CMDLINE

[Simon Glass]

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

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

 boot/Kconfig | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index f74ac7e9cc72..41ec2c34bf74 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,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256
 	  includes a ":", the portion prior to the ":" will be treated
 	  as a salt value.
 
+endif  # AUTOBOOT_KEYED
+endif  # AUTOBOOT
+
 config AUTOBOOT_USE_MENUKEY
 	bool "Allow a specify key to run a menu from the environment"
 	depends on !AUTOBOOT_KEYED
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH 03/25] autoboot: Correct dependencies on CMDLINE

[Tom Rini]

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

On Sun, Sep 24, 2023 at 02:39:21PM -0600, Simon Glass wrote:

> Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  boot/Kconfig | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/boot/Kconfig b/boot/Kconfig
> index f74ac7e9cc72..41ec2c34bf74 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.

This is fine and correct.

> +if AUTOBOOT
> +
>  config BOOTDELAY
>  	int "delay in seconds before automatically booting"
>  	default 2
> -	depends on AUTOBOOT
[snip]
> @@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256
>  	  includes a ":", the portion prior to the ":" will be treated
>  	  as a salt value.
>  
> +endif  # AUTOBOOT_KEYED
> +endif  # AUTOBOOT

So it's ~200 lines, yes, hiding this under an if, or perhaps a menu
makes sense, however...

>  config AUTOBOOT_USE_MENUKEY
>  	bool "Allow a specify key to run a menu from the environment"
>  	depends on !AUTOBOOT_KEYED

It looks like there's more stuff to move under a menu/if here?

-- 
Tom

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

Re: [PATCH 03/25] autoboot: Correct dependencies on CMDLINE

[Simon Glass]

Hi Tom,

On Sun, 24 Sept 2023 at 18:40, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Sep 24, 2023 at 02:39:21PM -0600, Simon Glass wrote:
>
> > Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  boot/Kconfig | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/boot/Kconfig b/boot/Kconfig
> > index f74ac7e9cc72..41ec2c34bf74 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.
>
> This is fine and correct.
>
> > +if AUTOBOOT
> > +
> >  config BOOTDELAY
> >       int "delay in seconds before automatically booting"
> >       default 2
> > -     depends on AUTOBOOT
> [snip]
> > @@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256
> >         includes a ":", the portion prior to the ":" will be treated
> >         as a salt value.
> >
> > +endif  # AUTOBOOT_KEYED
> > +endif  # AUTOBOOT
>
> So it's ~200 lines, yes, hiding this under an if, or perhaps a menu
> makes sense, however...
>
> >  config AUTOBOOT_USE_MENUKEY
> >       bool "Allow a specify key to run a menu from the environment"
> >       depends on !AUTOBOOT_KEYED
>
> It looks like there's more stuff to move under a menu/if here?

Well this depends on !AUTOBOOT_KEYED so can't be in the 'if
AUTOBOOT_KEYED'. But yes I can create a new 'if !AUTOBOOT_KEYED' for
these two items. Normally we want 2-3 options to warrant an 'if', so I
don't see this as a strong case.

Regards,
Simon

Re: [PATCH 03/25] autoboot: Correct dependencies on CMDLINE

[Tom Rini]

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

On Tue, Oct 03, 2023 at 08:11:11PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, 24 Sept 2023 at 18:40, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Sep 24, 2023 at 02:39:21PM -0600, Simon Glass wrote:
> >
> > > Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  boot/Kconfig | 23 ++++++++++++++---------
> > >  1 file changed, 14 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/boot/Kconfig b/boot/Kconfig
> > > index f74ac7e9cc72..41ec2c34bf74 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.
> >
> > This is fine and correct.
> >
> > > +if AUTOBOOT
> > > +
> > >  config BOOTDELAY
> > >       int "delay in seconds before automatically booting"
> > >       default 2
> > > -     depends on AUTOBOOT
> > [snip]
> > > @@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256
> > >         includes a ":", the portion prior to the ":" will be treated
> > >         as a salt value.
> > >
> > > +endif  # AUTOBOOT_KEYED
> > > +endif  # AUTOBOOT
> >
> > So it's ~200 lines, yes, hiding this under an if, or perhaps a menu
> > makes sense, however...
> >
> > >  config AUTOBOOT_USE_MENUKEY
> > >       bool "Allow a specify key to run a menu from the environment"
> > >       depends on !AUTOBOOT_KEYED
> >
> > It looks like there's more stuff to move under a menu/if here?
> 
> Well this depends on !AUTOBOOT_KEYED so can't be in the 'if
> AUTOBOOT_KEYED'. But yes I can create a new 'if !AUTOBOOT_KEYED' for
> these two items. Normally we want 2-3 options to warrant an 'if', so I
> don't see this as a strong case.

Well, sometimes it seems like we abuse the "if" mechanic too.  It's not
short-hand for "avoid saying depends on" a few times, it's "hide these
things until we turn on another feature". 

But right here it looks like AUTOBOOT_USE_MENUKEY still needs to be
under "if AUTOBOOT" yes?

-- 
Tom

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

[PATCH 04/25] cmd: Add a few more dependencies on CMDLINE

[Simon Glass]

Add this to some more commands to avoid build errors with sandbox.

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

 cmd/Kconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 64d723bd483b..cdc22a067b27 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -224,6 +224,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.
@@ -413,6 +414,7 @@ source lib/efi_selftest/Kconfig
 
 config CMD_BOOTMENU
 	bool "bootmenu"
+	depends on CMDLINE
 	select MENU
 	select CHARSET
 	help
@@ -479,6 +481,7 @@ config CMD_GO
 
 config CMD_RUN
 	bool "run"
+	depends on CMDLINE
 	default y
 	help
 	  Run the command in the given environment variable.
@@ -574,6 +577,7 @@ menu "Environment commands"
 
 config CMD_ASKENV
 	bool "ask for env variable"
+	depends on CMDLINE
 	help
 	  Ask for environment variable
 
@@ -2125,6 +2129,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.
@@ -2231,6 +2236,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
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 05/25] treewide: Correct use of long help

[Simon Glass]

Some commands assume that CONFIG_SYS_LONGHELP is always defined.
Declaration of long help should be bracketed by an #ifdef to avoid an
'unused variable' warning.

Fix this treewide.

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

 arch/arm/mach-imx/cmd_dek.c                |  3 ++-
 arch/arm/mach-imx/cmd_mfgprot.c            |  3 ++-
 arch/arm/mach-imx/imx8/snvs_security_sc.c  | 10 ++++++++++
 arch/arm/mach-stm32mp/cmd_stm32key.c       |  2 ++
 board/freescale/common/cmd_esbc_validate.c |  3 ++-
 board/kontron/sl28/cmds.c                  |  2 ++
 cmd/adc.c                                  |  2 ++
 cmd/arm/exception.c                        |  2 ++
 cmd/arm/exception64.c                      |  2 ++
 cmd/axi.c                                  |  2 ++
 cmd/blob.c                                 |  2 ++
 cmd/cyclic.c                               |  2 ++
 cmd/mux.c                                  |  2 ++
 cmd/osd.c                                  |  2 ++
 cmd/pcap.c                                 |  2 ++
 cmd/riscv/exception.c                      |  2 ++
 cmd/sandbox/exception.c                    |  2 ++
 cmd/scp03.c                                |  2 ++
 cmd/wdt.c                                  |  2 ++
 cmd/x86/exception.c                        |  2 ++
 20 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
index 6fa5b41fcd38..25ea7d3b37da 100644
--- a/arch/arm/mach-imx/cmd_dek.c
+++ b/arch/arm/mach-imx/cmd_dek.c
@@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
 	return blob_encap_dek(src_addr, dst_addr, len);
 }
 
-/***************************************************/
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char dek_blob_help_text[] =
 	"src dst len            - Encapsulate and create blob of data\n"
 	"                         $len bits long at address $src and\n"
 	"                         store the result at address $dst.\n";
+#endif
 
 U_BOOT_CMD(
 	dek_blob, 4, 1, do_dek_blob,
diff --git a/arch/arm/mach-imx/cmd_mfgprot.c b/arch/arm/mach-imx/cmd_mfgprot.c
index 9576b48dde30..bf19f80dde9b 100644
--- a/arch/arm/mach-imx/cmd_mfgprot.c
+++ b/arch/arm/mach-imx/cmd_mfgprot.c
@@ -133,13 +133,14 @@ free_m:
 	return ret;
 }
 
-/***************************************************/
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char mfgprot_help_text[] =
 	"Usage:\n"
 	 "Print the public key for Manufacturing Protection\n"
 	 "\tmfgprot pubk\n"
 	 "Generates a Manufacturing Protection signature\n"
 	 "\tmfgprot sign <data_addr> <size>";
+#endif
 
 U_BOOT_CMD(
 	mfgprot, 4, 1, do_mfgprot,
diff --git a/arch/arm/mach-imx/imx8/snvs_security_sc.c b/arch/arm/mach-imx/imx8/snvs_security_sc.c
index 1eaa68f8d5ff..e14727d7ca0b 100644
--- a/arch/arm/mach-imx/imx8/snvs_security_sc.c
+++ b/arch/arm/mach-imx/imx8/snvs_security_sc.c
@@ -598,6 +598,7 @@ exit:
 }
 #endif /* CONFIG_IMX_SNVS_SEC_SC_AUTO */
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char snvs_cfg_help_text[] =
 	"snvs_cfg\n"
 	"\thp.lock\n"
@@ -620,6 +621,7 @@ static char snvs_cfg_help_text[] =
 	"\tlp.act_tamper_routing_ctl2\n"
 	"\n"
 	"ALL values should be in hexadecimal format";
+#endif
 
 #define NB_REGISTERS 18
 static int do_snvs_cfg(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -663,6 +665,7 @@ U_BOOT_CMD(snvs_cfg,
 	   snvs_cfg_help_text
 );
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char snvs_dgo_cfg_help_text[] =
 	"snvs_dgo_cfg\n"
 	"\ttamper_offset_ctl\n"
@@ -673,6 +676,7 @@ static char snvs_dgo_cfg_help_text[] =
 	"\ttamper_core_volt_mon_ctl\n"
 	"\n"
 	"ALL values should be in hexadecimal format";
+#endif
 
 static int do_snvs_dgo_cfg(struct cmd_tbl *cmdtp, int flag, int argc,
 			   char *const argv[])
@@ -703,12 +707,14 @@ U_BOOT_CMD(snvs_dgo_cfg,
 	   snvs_dgo_cfg_help_text
 );
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char tamper_pin_cfg_help_text[] =
 	"snvs_dgo_cfg\n"
 	"\tpad\n"
 	"\tvalue\n"
 	"\n"
 	"ALL values should be in hexadecimal format";
+#endif
 
 static int do_tamper_pin_cfg(struct cmd_tbl *cmdtp, int flag, int argc,
 			     char *const argv[])
@@ -735,6 +741,7 @@ U_BOOT_CMD(tamper_pin_cfg,
 	   tamper_pin_cfg_help_text
 );
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char snvs_clear_status_help_text[] =
 	"snvs_clear_status\n"
 	"\tHPSR\n"
@@ -744,6 +751,7 @@ static char snvs_clear_status_help_text[] =
 	"\n"
 	"Write the status registers with the value provided,"
 	" clearing the status";
+#endif
 
 static int do_snvs_clear_status(struct cmd_tbl *cmdtp, int flag, int argc,
 				char *const argv[])
@@ -779,9 +787,11 @@ U_BOOT_CMD(snvs_clear_status,
 	   snvs_clear_status_help_text
 );
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char snvs_sec_status_help_text[] =
 	"snvs_sec_status\n"
 	"Display information about the security related to tamper and secvio";
+#endif
 
 static int do_snvs_sec_status(struct cmd_tbl *cmdtp, int flag, int argc,
 			      char *const argv[])
diff --git a/arch/arm/mach-stm32mp/cmd_stm32key.c b/arch/arm/mach-stm32mp/cmd_stm32key.c
index 85be8e23bdba..0f27fa128148 100644
--- a/arch/arm/mach-stm32mp/cmd_stm32key.c
+++ b/arch/arm/mach-stm32mp/cmd_stm32key.c
@@ -419,12 +419,14 @@ static int do_stm32key_close(struct cmd_tbl *cmdtp, int flag, int argc, char *co
 	return CMD_RET_SUCCESS;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char stm32key_help_text[] =
 	"list : list the supported key with description\n"
 	"stm32key select [<key>] : Select the key identified by <key> or display the key used for read/fuse command\n"
 	"stm32key read [<addr> | -a ] : Read the curent key at <addr> or current / all (-a) key in OTP\n"
 	"stm32key fuse [-y] <addr> : Fuse the current key at addr in OTP\n"
 	"stm32key close [-y] : Close the device\n";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(stm32key, "Manage key on STM32", stm32key_help_text,
 	U_BOOT_SUBCMD_MKENT(list, 1, 0, do_stm32key_list),
diff --git a/board/freescale/common/cmd_esbc_validate.c b/board/freescale/common/cmd_esbc_validate.c
index 6c096266b484..e678a5768117 100644
--- a/board/freescale/common/cmd_esbc_validate.c
+++ b/board/freescale/common/cmd_esbc_validate.c
@@ -62,7 +62,7 @@ static int do_esbc_validate(struct cmd_tbl *cmdtp, int flag, int argc,
 	return 0;
 }
 
-/***************************************************/
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char esbc_validate_help_text[] =
 	"esbc_validate hdr_addr <hash_val> - Validates signature using\n"
 	"                          RSA verification\n"
@@ -71,6 +71,7 @@ static char esbc_validate_help_text[] =
 	"                          $hash_val -Optional\n"
 	"                          It provides Hash of public/srk key to be\n"
 	"                          used to verify signature.\n";
+#endif
 
 U_BOOT_CMD(
 	esbc_validate,	3,	0,	do_esbc_validate,
diff --git a/board/kontron/sl28/cmds.c b/board/kontron/sl28/cmds.c
index 08a22b5d01e0..c83f3245d5f8 100644
--- a/board/kontron/sl28/cmds.c
+++ b/board/kontron/sl28/cmds.c
@@ -171,8 +171,10 @@ out:
 	return CMD_RET_FAILURE;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char sl28_help_text[] =
 	"nvm [<hex>] - display/set the 16 non-volatile bits\n";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(sl28, "SMARC-sAL28 specific", sl28_help_text,
 			U_BOOT_SUBCMD_MKENT(nvm, 2, 1, do_sl28_nvm));
diff --git a/cmd/adc.c b/cmd/adc.c
index a739d9e46411..ffd112581797 100644
--- a/cmd/adc.c
+++ b/cmd/adc.c
@@ -152,11 +152,13 @@ static int do_adc_scan(struct cmd_tbl *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char adc_help_text[] =
 	"list - list ADC devices\n"
 	"adc info <name> - Get ADC device info\n"
 	"adc single <name> <channel> [varname] - Get Single data of ADC device channel\n"
 	"adc scan <name> [channel mask] - Scan all [or masked] ADC channels";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(adc, "ADC sub-system", adc_help_text,
 	U_BOOT_SUBCMD_MKENT(list, 1, 1, do_adc_list),
diff --git a/cmd/arm/exception.c b/cmd/arm/exception.c
index 522f6dff53f2..6f2cdfe17346 100644
--- a/cmd/arm/exception.c
+++ b/cmd/arm/exception.c
@@ -50,6 +50,7 @@ static struct cmd_tbl cmd_sub[] = {
 			 "", ""),
 };
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char exception_help_text[] =
 	"<ex>\n"
 	"  The following exceptions are available:\n"
@@ -57,5 +58,6 @@ static char exception_help_text[] =
 	"  unaligned  - data abort\n"
 	"  undefined  - undefined instruction\n"
 	;
+#endif
 
 #include <exception.h>
diff --git a/cmd/arm/exception64.c b/cmd/arm/exception64.c
index 589a23115b04..6afe5e2ab5b6 100644
--- a/cmd/arm/exception64.c
+++ b/cmd/arm/exception64.c
@@ -78,6 +78,7 @@ static struct cmd_tbl cmd_sub[] = {
 			 "", ""),
 };
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char exception_help_text[] =
 	"<ex>\n"
 	"  The following exceptions are available:\n"
@@ -85,5 +86,6 @@ static char exception_help_text[] =
 	"  unaligned  - unaligned LDAR data abort\n"
 	"  undefined  - undefined instruction exception\n"
 	;
+#endif
 
 #include <exception.h>
diff --git a/cmd/axi.c b/cmd/axi.c
index b97b43eb7d01..272fc6131f07 100644
--- a/cmd/axi.c
+++ b/cmd/axi.c
@@ -344,11 +344,13 @@ static int do_ihs_axi(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_USAGE;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char axi_help_text[] =
 	"bus  - show AXI bus info\n"
 	"axi dev [bus] - show or set current AXI bus to bus number [bus]\n"
 	"axi md size addr [# of objects] - read from AXI device at address [addr] and data width [size] (one of 8, 16, 32)\n"
 	"axi mw size addr value [count] - write data [value] to AXI device at address [addr] and data width [size] (one of 8, 16, 32)\n";
+#endif
 
 U_BOOT_CMD(axi, 7, 1, do_ihs_axi,
 	   "AXI sub-system",
diff --git a/cmd/blob.c b/cmd/blob.c
index 7c77c410d528..70b0df1114ba 100644
--- a/cmd/blob.c
+++ b/cmd/blob.c
@@ -99,6 +99,7 @@ static int do_blob(struct cmd_tbl *cmdtp, int flag, int argc,
 }
 
 /***************************************************/
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char blob_help_text[] =
 	"enc src dst len km - Encapsulate and create blob of data\n"
 	"                          $len bytes long at address $src and\n"
@@ -116,6 +117,7 @@ static char blob_help_text[] =
 	"                          The modifier is required for generation\n"
 	"                          /use as key for cryptographic operation.\n"
 	"                          Key modifier should be 16 byte long.\n";
+#endif
 
 U_BOOT_CMD(
 	blob, 6, 1, do_blob,
diff --git a/cmd/cyclic.c b/cmd/cyclic.c
index 946f1d78184d..789eba4eb046 100644
--- a/cmd/cyclic.c
+++ b/cmd/cyclic.c
@@ -76,9 +76,11 @@ static int do_cyclic_list(struct cmd_tbl *cmdtp, int flag, int argc,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char cyclic_help_text[] =
 	"demo <cycletime_ms> <delay_us> - register cyclic demo function\n"
 	"cyclic list - list cyclic functions\n";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(cyclic, "Cyclic", cyclic_help_text,
 	U_BOOT_SUBCMD_MKENT(demo, 3, 1, do_cyclic_demo),
diff --git a/cmd/mux.c b/cmd/mux.c
index c75907af7726..d20a151948dc 100644
--- a/cmd/mux.c
+++ b/cmd/mux.c
@@ -173,10 +173,12 @@ static int do_mux_deselect(struct cmd_tbl *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char mux_help_text[] =
 	"list - List all Muxes and their states\n"
 	"select <chip> <id> <state> - Select the given mux state\n"
 	"deselect <chip> <id> - Deselect the given mux and reset it to its idle state";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(mux, "List, select, and deselect muxes", mux_help_text,
 			U_BOOT_SUBCMD_MKENT(list, 1, 1, do_mux_list),
diff --git a/cmd/osd.c b/cmd/osd.c
index c8c62d4a2ab3..9cf7cc91b283 100644
--- a/cmd/osd.c
+++ b/cmd/osd.c
@@ -278,12 +278,14 @@ static int do_osd(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		return CMD_RET_USAGE;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char osd_help_text[] =
 	"show  - show OSD info\n"
 	"osd dev [dev] - show or set current OSD\n"
 	"write [pos_x] [pos_y] [buffer] [count] - write 8-bit hex encoded buffer to osd memory at a given position\n"
 	"print [pos_x] [pos_y] [color] [text] - write ASCII buffer (given by text data and driver-specific color information) to osd memory\n"
 	"size [size_x] [size_y] - set OSD XY size in characters\n";
+#endif
 
 U_BOOT_CMD(
 	osd, 6, 1, do_osd,
diff --git a/cmd/pcap.c b/cmd/pcap.c
index ab5c1a7e8737..ac09a0756102 100644
--- a/cmd/pcap.c
+++ b/cmd/pcap.c
@@ -48,6 +48,7 @@ static int do_pcap_clear(struct cmd_tbl *cmdtp, int flag, int argc,
 	return pcap_clear() ? CMD_RET_FAILURE : CMD_RET_SUCCESS;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char pcap_help_text[] =
 	"- network packet capture\n\n"
 	"pcap\n"
@@ -61,6 +62,7 @@ static char pcap_help_text[] =
 	"\t<addr>: user address to which pcap will be stored (hexedcimal)\n"
 	"\t<max_size>: Maximum size of pcap file (decimal)\n"
 	"\n";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(pcap, "pcap", pcap_help_text,
 			U_BOOT_SUBCMD_MKENT(init, 3, 0, do_pcap_init),
diff --git a/cmd/riscv/exception.c b/cmd/riscv/exception.c
index 7a08061d1203..f71d3e3252d7 100644
--- a/cmd/riscv/exception.c
+++ b/cmd/riscv/exception.c
@@ -43,6 +43,7 @@ static struct cmd_tbl cmd_sub[] = {
 			 "", ""),
 };
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char exception_help_text[] =
 	"<ex>\n"
 	"  The following exceptions are available:\n"
@@ -50,5 +51,6 @@ static char exception_help_text[] =
 	"  undefined - illegal instruction\n"
 	"  unaligned - load address misaligned\n"
 	;
+#endif
 
 #include <exception.h>
diff --git a/cmd/sandbox/exception.c b/cmd/sandbox/exception.c
index 1aa1d673aed4..7837f5de285f 100644
--- a/cmd/sandbox/exception.c
+++ b/cmd/sandbox/exception.c
@@ -31,11 +31,13 @@ static struct cmd_tbl cmd_sub[] = {
 			 "", ""),
 };
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char exception_help_text[] =
 	"<ex>\n"
 	"  The following exceptions are available:\n"
 	"  undefined  - undefined instruction\n"
 	"  sigsegv    - illegal memory access\n"
 	;
+#endif
 
 #include <exception.h>
diff --git a/cmd/scp03.c b/cmd/scp03.c
index 216c942dd48b..611112059a43 100644
--- a/cmd/scp03.c
+++ b/cmd/scp03.c
@@ -41,10 +41,12 @@ int do_scp03_provision(struct cmd_tbl *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char text[] =
 	"provides a command to enable SCP03 and provision the SCP03 keys\n"
 	" enable    - enable SCP03 on the TEE\n"
 	" provision - provision SCP03 on the TEE\n";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(scp03, "Secure Channel Protocol 03 control", text,
 	U_BOOT_SUBCMD_MKENT(enable, 1, 1, do_scp03_enable),
diff --git a/cmd/wdt.c b/cmd/wdt.c
index 27410981e7bf..255dfc0b0ffd 100644
--- a/cmd/wdt.c
+++ b/cmd/wdt.c
@@ -157,6 +157,7 @@ static int do_wdt_expire(struct cmd_tbl *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char wdt_help_text[] =
 	"list - list watchdog devices\n"
 	"wdt dev [<name>] - get/set current watchdog device\n"
@@ -164,6 +165,7 @@ static char wdt_help_text[] =
 	"wdt stop - stop watchdog timer\n"
 	"wdt reset - reset watchdog timer\n"
 	"wdt expire [flags] - expire watchdog timer immediately\n";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(wdt, "Watchdog sub-system", wdt_help_text,
 	U_BOOT_SUBCMD_MKENT(list, 1, 1, do_wdt_list),
diff --git a/cmd/x86/exception.c b/cmd/x86/exception.c
index 82faaa913e5c..8f2f6c135952 100644
--- a/cmd/x86/exception.c
+++ b/cmd/x86/exception.c
@@ -20,10 +20,12 @@ static struct cmd_tbl cmd_sub[] = {
 			 "", ""),
 };
 
+#if IS_ENABLED(CONFIG_SYS_LONGHELP)
 static char exception_help_text[] =
 	"<ex>\n"
 	"  The following exceptions are available:\n"
 	"  undefined  - undefined instruction\n"
 	;
+#endif
 
 #include <exception.h>
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH 05/25] treewide: Correct use of long help

[Tom Rini]

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

On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> Declaration of long help should be bracketed by an #ifdef to avoid an
> 'unused variable' warning.
> 
> Fix this treewide.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
[snip]
> diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> index 6fa5b41fcd38..25ea7d3b37da 100644
> --- a/arch/arm/mach-imx/cmd_dek.c
> +++ b/arch/arm/mach-imx/cmd_dek.c
> @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
>  	return blob_encap_dek(src_addr, dst_addr, len);
>  }
>  
> -/***************************************************/
> +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
>  static char dek_blob_help_text[] =
>  	"src dst len            - Encapsulate and create blob of data\n"
>  	"                         $len bits long at address $src and\n"
>  	"                         store the result at address $dst.\n";
> +#endif
>  
>  U_BOOT_CMD(
>  	dek_blob, 4, 1, do_dek_blob,

This really doesn't read nicely.  I would rather (globally and fix
existing users) __maybe_unused this instead.  I think there's just one
example today that isn't "foo_help_text".

-- 
Tom

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

Re: [PATCH 05/25] treewide: Correct use of long help

[Simon Glass]

Hi Tom,

On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > Declaration of long help should be bracketed by an #ifdef to avoid an
> > 'unused variable' warning.
> >
> > Fix this treewide.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> [snip]
> > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > index 6fa5b41fcd38..25ea7d3b37da 100644
> > --- a/arch/arm/mach-imx/cmd_dek.c
> > +++ b/arch/arm/mach-imx/cmd_dek.c
> > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> >       return blob_encap_dek(src_addr, dst_addr, len);
> >  }
> >
> > -/***************************************************/
> > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> >  static char dek_blob_help_text[] =
> >       "src dst len            - Encapsulate and create blob of data\n"
> >       "                         $len bits long at address $src and\n"
> >       "                         store the result at address $dst.\n";
> > +#endif
> >
> >  U_BOOT_CMD(
> >       dek_blob, 4, 1, do_dek_blob,
>
> This really doesn't read nicely.  I would rather (globally and fix
> existing users) __maybe_unused this instead.  I think there's just one
> example today that isn't "foo_help_text".

Hmm, what do you think about adding a __longhelp symbol to cause the
linker to discard it when not needed?

Regards,
Simon

Re: [PATCH 05/25] treewide: Correct use of long help

[Tom Rini]

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

On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > 'unused variable' warning.
> > >
> > > Fix this treewide.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > [snip]
> > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > >       return blob_encap_dek(src_addr, dst_addr, len);
> > >  }
> > >
> > > -/***************************************************/
> > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > >  static char dek_blob_help_text[] =
> > >       "src dst len            - Encapsulate and create blob of data\n"
> > >       "                         $len bits long at address $src and\n"
> > >       "                         store the result at address $dst.\n";
> > > +#endif
> > >
> > >  U_BOOT_CMD(
> > >       dek_blob, 4, 1, do_dek_blob,
> >
> > This really doesn't read nicely.  I would rather (globally and fix
> > existing users) __maybe_unused this instead.  I think there's just one
> > example today that isn't "foo_help_text".
> 
> Hmm, what do you think about adding a __longhelp symbol to cause the
> linker to discard it when not needed?

Well, I don't think we need linker list magic when __maybe_unused will
just have them be discarded normally.

-- 
Tom

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

Re: [PATCH 05/25] treewide: Correct use of long help

[Simon Glass]

Hi Tom,

On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > 'unused variable' warning.
> > > >
> > > > Fix this treewide.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > [snip]
> > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > >  }
> > > >
> > > > -/***************************************************/
> > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > >  static char dek_blob_help_text[] =
> > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > >       "                         $len bits long at address $src and\n"
> > > >       "                         store the result at address $dst.\n";
> > > > +#endif
> > > >
> > > >  U_BOOT_CMD(
> > > >       dek_blob, 4, 1, do_dek_blob,
> > >
> > > This really doesn't read nicely.  I would rather (globally and fix
> > > existing users) __maybe_unused this instead.  I think there's just one
> > > example today that isn't "foo_help_text".
> >
> > Hmm, what do you think about adding a __longhelp symbol to cause the
> > linker to discard it when not needed?
>
> Well, I don't think we need linker list magic when __maybe_unused will
> just have them be discarded normally.

Yes, perhaps things are in a better state than they used to be, but
there is a linker discard for commands at present.

SECTIONS
{
#ifndef CONFIG_CMDLINE
        /DISCARD/ : { *(__u_boot_list_2_cmd_*) }
#endif
...

from:

c1352119fd0 arm: x86: Drop command-line code when CONFIG_CMDLINE is disabled

I wonder if we can remove it? I suppose once this series is sorted out
we could have a test to make sure no command is making it into the
image when ~CMDLINE

Regards,
Simon

Re: [PATCH 05/25] treewide: Correct use of long help

[Tom Rini]

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

On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > 'unused variable' warning.
> > > > >
> > > > > Fix this treewide.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > [snip]
> > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > >  }
> > > > >
> > > > > -/***************************************************/
> > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > >  static char dek_blob_help_text[] =
> > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > >       "                         $len bits long at address $src and\n"
> > > > >       "                         store the result at address $dst.\n";
> > > > > +#endif
> > > > >
> > > > >  U_BOOT_CMD(
> > > > >       dek_blob, 4, 1, do_dek_blob,
> > > >
> > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > example today that isn't "foo_help_text".
> > >
> > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > linker to discard it when not needed?
> >
> > Well, I don't think we need linker list magic when __maybe_unused will
> > just have them be discarded normally.
> 
> Yes, perhaps things are in a better state than they used to be, but
> there is a linker discard for commands at present.

Yes, but __maybe_unused means we don't get a warning about it, and it's
literally discarded as part of --gc-sections as it done everywhere with
maybe 3 exceptions?

-- 
Tom

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

Re: [PATCH 05/25] treewide: Correct use of long help

[Simon Glass]

Hi Tom,

On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > 'unused variable' warning.
> > > > > >
> > > > > > Fix this treewide.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > [snip]
> > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > > >  }
> > > > > >
> > > > > > -/***************************************************/
> > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > >  static char dek_blob_help_text[] =
> > > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > > >       "                         $len bits long at address $src and\n"
> > > > > >       "                         store the result at address $dst.\n";
> > > > > > +#endif
> > > > > >
> > > > > >  U_BOOT_CMD(
> > > > > >       dek_blob, 4, 1, do_dek_blob,
> > > > >
> > > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > > example today that isn't "foo_help_text".
> > > >
> > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > linker to discard it when not needed?
> > >
> > > Well, I don't think we need linker list magic when __maybe_unused will
> > > just have them be discarded normally.
> >
> > Yes, perhaps things are in a better state than they used to be, but
> > there is a linker discard for commands at present.
>
> Yes, but __maybe_unused means we don't get a warning about it, and it's
> literally discarded as part of --gc-sections as it done everywhere with
> maybe 3 exceptions?

Actually with this series we get a lot closer to that. The problem
with the status quo is that disabling CMDLINE doesn't disable most
commands, relying instead on discarding them at link time.

With this series, it looks like we can in fact do that, so I should
remove the discards as well.

The one proviso is that this does drop a lot of things we want...e.g.
BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
that common filesystems are dropped. So we'll need more effort after
this, to allow (for example) bootmeths that don't need commands to
work correctly. But I think this series at least provides a better
starting point for teasing things apart.

So OK I'll go with __maybe_unused for the ones that need it, which
isn't too many given that many of the strings are just included
directly in the macro. It is considerably easier than trying to be to
clever about things.

Regards,
Simon

Re: [PATCH 05/25] treewide: Correct use of long help

[Tom Rini]

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

On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > > 'unused variable' warning.
> > > > > > >
> > > > > > > Fix this treewide.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > [snip]
> > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > > > >  }
> > > > > > >
> > > > > > > -/***************************************************/
> > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > > >  static char dek_blob_help_text[] =
> > > > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > > > >       "                         $len bits long at address $src and\n"
> > > > > > >       "                         store the result at address $dst.\n";
> > > > > > > +#endif
> > > > > > >
> > > > > > >  U_BOOT_CMD(
> > > > > > >       dek_blob, 4, 1, do_dek_blob,
> > > > > >
> > > > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > > > example today that isn't "foo_help_text".
> > > > >
> > > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > > linker to discard it when not needed?
> > > >
> > > > Well, I don't think we need linker list magic when __maybe_unused will
> > > > just have them be discarded normally.
> > >
> > > Yes, perhaps things are in a better state than they used to be, but
> > > there is a linker discard for commands at present.
> >
> > Yes, but __maybe_unused means we don't get a warning about it, and it's
> > literally discarded as part of --gc-sections as it done everywhere with
> > maybe 3 exceptions?
> 
> Actually with this series we get a lot closer to that. The problem
> with the status quo is that disabling CMDLINE doesn't disable most
> commands, relying instead on discarding them at link time.

I don't follow you here.  We're talking only about the long help.
There's already an option to enable/disable it.  When disabled all of
the long help text should be discarded, because we reference it via
U_BOOT_CMD macro which in turn cares about it, or not.  What's missing
is a U_BOOT_LONGHELP macro so that instead of:
#ifdef CONFIG_SYS_LONGHELP
static char cat_help_text[] =
        "<interface> <dev[:part]> <file>\n"
        "  - Print file from 'dev' on 'interface' to standard output\n";
#endif

We do:
U_BOOT_LONGHELP(cat,
        "<interface> <dev[:part]> <file>\n"
        "  - Print file from 'dev' on 'interface' to standard output\n"
);

Which then does:
static __maybe_unused char cat_help_text[] =
...
;

And we discard the text automatically due to --gc-sections.  We possibly
haven't already been doing this since back when we first started using
--gc-sections there was a bug in binutils that caused text like the
above to still get combined in to other objects and not discarded.
That's been fixed for ages.

And the above macro would also let us clean up U_BOOT_CMD macro slightly
to just omit the longhelp option and instead always do _CMD_HELP(_name)
inside the macros.  It'll also make it harder to add new commands
without a long help by accident.

> With this series, it looks like we can in fact do that, so I should
> remove the discards as well.
> 
> The one proviso is that this does drop a lot of things we want...e.g.
> BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
> that common filesystems are dropped. So we'll need more effort after
> this, to allow (for example) bootmeths that don't need commands to
> work correctly. But I think this series at least provides a better
> starting point for teasing things apart.
> 
> So OK I'll go with __maybe_unused for the ones that need it, which
> isn't too many given that many of the strings are just included
> directly in the macro. It is considerably easier than trying to be to
> clever about things.

Yes, this series itself has a lot of problems that need to be addressed
before reposting because I don't like "hiding" that network stuff no
longer works, and I also saw that DFU also silently failing.

-- 
Tom

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

Re: [PATCH 05/25] treewide: Correct use of long help

[Simon Glass]

Hi Tom,

On Fri, 6 Oct 2023 at 10:55, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > > > 'unused variable' warning.
> > > > > > > >
> > > > > > > > Fix this treewide.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > [snip]
> > > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > -/***************************************************/
> > > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > > > >  static char dek_blob_help_text[] =
> > > > > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > > > > >       "                         $len bits long at address $src and\n"
> > > > > > > >       "                         store the result at address $dst.\n";
> > > > > > > > +#endif
> > > > > > > >
> > > > > > > >  U_BOOT_CMD(
> > > > > > > >       dek_blob, 4, 1, do_dek_blob,
> > > > > > >
> > > > > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > > > > example today that isn't "foo_help_text".
> > > > > >
> > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > > > linker to discard it when not needed?
> > > > >
> > > > > Well, I don't think we need linker list magic when __maybe_unused will
> > > > > just have them be discarded normally.
> > > >
> > > > Yes, perhaps things are in a better state than they used to be, but
> > > > there is a linker discard for commands at present.
> > >
> > > Yes, but __maybe_unused means we don't get a warning about it, and it's
> > > literally discarded as part of --gc-sections as it done everywhere with
> > > maybe 3 exceptions?
> >
> > Actually with this series we get a lot closer to that. The problem
> > with the status quo is that disabling CMDLINE doesn't disable most
> > commands, relying instead on discarding them at link time.
>
> I don't follow you here.  We're talking only about the long help.

I was actually talking about how the command code is removed. This
series allows that to happen via Kconfig rather than needing a
linker-script discard rule, something I only just fully appreciated.

> There's already an option to enable/disable it.  When disabled all of
> the long help text should be discarded, because we reference it via
> U_BOOT_CMD macro which in turn cares about it, or not.  What's missing
> is a U_BOOT_LONGHELP macro so that instead of:
> #ifdef CONFIG_SYS_LONGHELP
> static char cat_help_text[] =
>         "<interface> <dev[:part]> <file>\n"
>         "  - Print file from 'dev' on 'interface' to standard output\n";
> #endif
>
> We do:
> U_BOOT_LONGHELP(cat,
>         "<interface> <dev[:part]> <file>\n"
>         "  - Print file from 'dev' on 'interface' to standard output\n"
> );
>
> Which then does:
> static __maybe_unused char cat_help_text[] =
> ...
> ;
>
> And we discard the text automatically due to --gc-sections.  We possibly
> haven't already been doing this since back when we first started using
> --gc-sections there was a bug in binutils that caused text like the
> above to still get combined in to other objects and not discarded.
> That's been fixed for ages.
>
> And the above macro would also let us clean up U_BOOT_CMD macro slightly
> to just omit the longhelp option and instead always do _CMD_HELP(_name)
> inside the macros.  It'll also make it harder to add new commands
> without a long help by accident.

Gosh this is complicated.

At present the U_BOOT_CMD() macro just drops the strings...the problem
with the unused var only happens in a small number of cases where an
explicit var is used. I don't see why creating a var (when none is
there today) helps anything? It doesn't detect missing longhelp, since
this is already an error (missing argument). Sure,  "" can be
provided, but your macro doesn't stop that either.

If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that
we already have quite a lot...each new 'top-level' macro results in
more combinations.

>
> > With this series, it looks like we can in fact do that, so I should
> > remove the discards as well.
> >
> > The one proviso is that this does drop a lot of things we want...e.g.
> > BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
> > that common filesystems are dropped. So we'll need more effort after
> > this, to allow (for example) bootmeths that don't need commands to
> > work correctly. But I think this series at least provides a better
> > starting point for teasing things apart.
> >
> > So OK I'll go with __maybe_unused for the ones that need it, which
> > isn't too many given that many of the strings are just included
> > directly in the macro. It is considerably easier than trying to be to
> > clever about things.
>
> Yes, this series itself has a lot of problems that need to be addressed
> before reposting because I don't like "hiding" that network stuff no
> longer works, and I also saw that DFU also silently failing.

Yes I saw your other comments. But I think this patch is the big area
of confusion.

Regards,
Simon

Re: [PATCH 05/25] treewide: Correct use of long help

[Tom Rini]

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

On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 6 Oct 2023 at 10:55, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > > > > 'unused variable' warning.
> > > > > > > > >
> > > > > > > > > Fix this treewide.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > [snip]
> > > > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > -/***************************************************/
> > > > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > > > > >  static char dek_blob_help_text[] =
> > > > > > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > > > > > >       "                         $len bits long at address $src and\n"
> > > > > > > > >       "                         store the result at address $dst.\n";
> > > > > > > > > +#endif
> > > > > > > > >
> > > > > > > > >  U_BOOT_CMD(
> > > > > > > > >       dek_blob, 4, 1, do_dek_blob,
> > > > > > > >
> > > > > > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > > > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > > > > > example today that isn't "foo_help_text".
> > > > > > >
> > > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > > > > linker to discard it when not needed?
> > > > > >
> > > > > > Well, I don't think we need linker list magic when __maybe_unused will
> > > > > > just have them be discarded normally.
> > > > >
> > > > > Yes, perhaps things are in a better state than they used to be, but
> > > > > there is a linker discard for commands at present.
> > > >
> > > > Yes, but __maybe_unused means we don't get a warning about it, and it's
> > > > literally discarded as part of --gc-sections as it done everywhere with
> > > > maybe 3 exceptions?
> > >
> > > Actually with this series we get a lot closer to that. The problem
> > > with the status quo is that disabling CMDLINE doesn't disable most
> > > commands, relying instead on discarding them at link time.
> >
> > I don't follow you here.  We're talking only about the long help.
> 
> I was actually talking about how the command code is removed. This
> series allows that to happen via Kconfig rather than needing a
> linker-script discard rule, something I only just fully appreciated.

OK.  But this series as-is has a lot of problems and I don't see it as
more than a proof of concept.

> > There's already an option to enable/disable it.  When disabled all of
> > the long help text should be discarded, because we reference it via
> > U_BOOT_CMD macro which in turn cares about it, or not.  What's missing
> > is a U_BOOT_LONGHELP macro so that instead of:
> > #ifdef CONFIG_SYS_LONGHELP
> > static char cat_help_text[] =
> >         "<interface> <dev[:part]> <file>\n"
> >         "  - Print file from 'dev' on 'interface' to standard output\n";
> > #endif
> >
> > We do:
> > U_BOOT_LONGHELP(cat,
> >         "<interface> <dev[:part]> <file>\n"
> >         "  - Print file from 'dev' on 'interface' to standard output\n"
> > );
> >
> > Which then does:
> > static __maybe_unused char cat_help_text[] =
> > ...
> > ;
> >
> > And we discard the text automatically due to --gc-sections.  We possibly
> > haven't already been doing this since back when we first started using
> > --gc-sections there was a bug in binutils that caused text like the
> > above to still get combined in to other objects and not discarded.
> > That's been fixed for ages.
> >
> > And the above macro would also let us clean up U_BOOT_CMD macro slightly
> > to just omit the longhelp option and instead always do _CMD_HELP(_name)
> > inside the macros.  It'll also make it harder to add new commands
> > without a long help by accident.
> 
> Gosh this is complicated.
> 
> At present the U_BOOT_CMD() macro just drops the strings...the problem
> with the unused var only happens in a small number of cases where an
> explicit var is used. I don't see why creating a var (when none is
> there today) helps anything? It doesn't detect missing longhelp, since
> this is already an error (missing argument). Sure,  "" can be
> provided, but your macro doesn't stop that either.

The problem today is that 95% of the cases surround the help text with
#ifdef CONFIG_SYS_LONGHELP ... #endif.  That's why the rest of the
macros are the way they are today.  And that in turn is due to (in part
at least) old compiler bugs.

> If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that
> we already have quite a lot...each new 'top-level' macro results in
> more combinations.

It really should just be a single macro.  I think I'll make an attempt
at this, to show what I'm talking about.

> > > With this series, it looks like we can in fact do that, so I should
> > > remove the discards as well.
> > >
> > > The one proviso is that this does drop a lot of things we want...e.g.
> > > BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
> > > that common filesystems are dropped. So we'll need more effort after
> > > this, to allow (for example) bootmeths that don't need commands to
> > > work correctly. But I think this series at least provides a better
> > > starting point for teasing things apart.
> > >
> > > So OK I'll go with __maybe_unused for the ones that need it, which
> > > isn't too many given that many of the strings are just included
> > > directly in the macro. It is considerably easier than trying to be to
> > > clever about things.
> >
> > Yes, this series itself has a lot of problems that need to be addressed
> > before reposting because I don't like "hiding" that network stuff no
> > longer works, and I also saw that DFU also silently failing.
> 
> Yes I saw your other comments. But I think this patch is the big area
> of confusion.

This isn't the tricky part of the series that I'm saying has problems,
that part is all of the functionality that's not being untangled and I
think leads to the question of what exactly is the use case where we do
remove the command line but don't need some of that functionality still.

-- 
Tom

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

Re: [PATCH 05/25] treewide: Correct use of long help

[Simon Glass]

Hi Tom,

On Fri, 6 Oct 2023 at 19:01, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 6 Oct 2023 at 10:55, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > > > > > 'unused variable' warning.
> > > > > > > > > >
> > > > > > > > > > Fix this treewide.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > [snip]
> > > > > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > > > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > -/***************************************************/
> > > > > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > > > > > >  static char dek_blob_help_text[] =
> > > > > > > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > > > > > > >       "                         $len bits long at address $src and\n"
> > > > > > > > > >       "                         store the result at address $dst.\n";
> > > > > > > > > > +#endif
> > > > > > > > > >
> > > > > > > > > >  U_BOOT_CMD(
> > > > > > > > > >       dek_blob, 4, 1, do_dek_blob,
> > > > > > > > >
> > > > > > > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > > > > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > > > > > > example today that isn't "foo_help_text".
> > > > > > > >
> > > > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > > > > > linker to discard it when not needed?
> > > > > > >
> > > > > > > Well, I don't think we need linker list magic when __maybe_unused will
> > > > > > > just have them be discarded normally.
> > > > > >
> > > > > > Yes, perhaps things are in a better state than they used to be, but
> > > > > > there is a linker discard for commands at present.
> > > > >
> > > > > Yes, but __maybe_unused means we don't get a warning about it, and it's
> > > > > literally discarded as part of --gc-sections as it done everywhere with
> > > > > maybe 3 exceptions?
> > > >
> > > > Actually with this series we get a lot closer to that. The problem
> > > > with the status quo is that disabling CMDLINE doesn't disable most
> > > > commands, relying instead on discarding them at link time.
> > >
> > > I don't follow you here.  We're talking only about the long help.
> >
> > I was actually talking about how the command code is removed. This
> > series allows that to happen via Kconfig rather than needing a
> > linker-script discard rule, something I only just fully appreciated.
>
> OK.  But this series as-is has a lot of problems and I don't see it as
> more than a proof of concept.

Probably I need to send a few version as this discussion is becoming a
bit theoretical.

>
> > > There's already an option to enable/disable it.  When disabled all of
> > > the long help text should be discarded, because we reference it via
> > > U_BOOT_CMD macro which in turn cares about it, or not.  What's missing
> > > is a U_BOOT_LONGHELP macro so that instead of:
> > > #ifdef CONFIG_SYS_LONGHELP
> > > static char cat_help_text[] =
> > >         "<interface> <dev[:part]> <file>\n"
> > >         "  - Print file from 'dev' on 'interface' to standard output\n";
> > > #endif
> > >
> > > We do:
> > > U_BOOT_LONGHELP(cat,
> > >         "<interface> <dev[:part]> <file>\n"
> > >         "  - Print file from 'dev' on 'interface' to standard output\n"
> > > );
> > >
> > > Which then does:
> > > static __maybe_unused char cat_help_text[] =
> > > ...
> > > ;
> > >
> > > And we discard the text automatically due to --gc-sections.  We possibly
> > > haven't already been doing this since back when we first started using
> > > --gc-sections there was a bug in binutils that caused text like the
> > > above to still get combined in to other objects and not discarded.
> > > That's been fixed for ages.
> > >
> > > And the above macro would also let us clean up U_BOOT_CMD macro slightly
> > > to just omit the longhelp option and instead always do _CMD_HELP(_name)
> > > inside the macros.  It'll also make it harder to add new commands
> > > without a long help by accident.
> >
> > Gosh this is complicated.
> >
> > At present the U_BOOT_CMD() macro just drops the strings...the problem
> > with the unused var only happens in a small number of cases where an
> > explicit var is used. I don't see why creating a var (when none is
> > there today) helps anything? It doesn't detect missing longhelp, since
> > this is already an error (missing argument). Sure,  "" can be
> > provided, but your macro doesn't stop that either.
>
> The problem today is that 95% of the cases surround the help text with
> #ifdef CONFIG_SYS_LONGHELP ... #endif.  That's why the rest of the
> macros are the way they are today.  And that in turn is due to (in part
> at least) old compiler bugs.

I see about 46 #idefs for that and nearly 300 commands that don't have one.

>
> > If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that
> > we already have quite a lot...each new 'top-level' macro results in
> > more combinations.
>
> It really should just be a single macro.  I think I'll make an attempt
> at this, to show what I'm talking about.

OK thanks.

>
> > > > With this series, it looks like we can in fact do that, so I should
> > > > remove the discards as well.
> > > >
> > > > The one proviso is that this does drop a lot of things we want...e.g.
> > > > BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
> > > > that common filesystems are dropped. So we'll need more effort after
> > > > this, to allow (for example) bootmeths that don't need commands to
> > > > work correctly. But I think this series at least provides a better
> > > > starting point for teasing things apart.
> > > >
> > > > So OK I'll go with __maybe_unused for the ones that need it, which
> > > > isn't too many given that many of the strings are just included
> > > > directly in the macro. It is considerably easier than trying to be to
> > > > clever about things.
> > >
> > > Yes, this series itself has a lot of problems that need to be addressed
> > > before reposting because I don't like "hiding" that network stuff no
> > > longer works, and I also saw that DFU also silently failing.
> >
> > Yes I saw your other comments. But I think this patch is the big area
> > of confusion.
>
> This isn't the tricky part of the series that I'm saying has problems,
> that part is all of the functionality that's not being untangled and I
> think leads to the question of what exactly is the use case where we do
> remove the command line but don't need some of that functionality still.

For security and code-size reasons it is useful to disable commands,
perhaps all of them. Quite a few features don't need it, but it
certainly would take more effort to get a usable version. The goal of
this series is to avoid people adding Kconfig mistakes which need to
be cleaned up later.

Regards,
Simon

Re: [PATCH 05/25] treewide: Correct use of long help

[Tom Rini]

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

On Sat, Oct 07, 2023 at 09:37:07AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 6 Oct 2023 at 19:01, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 6 Oct 2023 at 10:55, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > > > > > > 'unused variable' warning.
> > > > > > > > > > >
> > > > > > > > > > > Fix this treewide.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > [snip]
> > > > > > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > > > > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > > -/***************************************************/
> > > > > > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > > > > > > >  static char dek_blob_help_text[] =
> > > > > > > > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > > > > > > > >       "                         $len bits long at address $src and\n"
> > > > > > > > > > >       "                         store the result at address $dst.\n";
> > > > > > > > > > > +#endif
> > > > > > > > > > >
> > > > > > > > > > >  U_BOOT_CMD(
> > > > > > > > > > >       dek_blob, 4, 1, do_dek_blob,
> > > > > > > > > >
> > > > > > > > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > > > > > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > > > > > > > example today that isn't "foo_help_text".
> > > > > > > > >
> > > > > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > > > > > > linker to discard it when not needed?
> > > > > > > >
> > > > > > > > Well, I don't think we need linker list magic when __maybe_unused will
> > > > > > > > just have them be discarded normally.
> > > > > > >
> > > > > > > Yes, perhaps things are in a better state than they used to be, but
> > > > > > > there is a linker discard for commands at present.
> > > > > >
> > > > > > Yes, but __maybe_unused means we don't get a warning about it, and it's
> > > > > > literally discarded as part of --gc-sections as it done everywhere with
> > > > > > maybe 3 exceptions?
> > > > >
> > > > > Actually with this series we get a lot closer to that. The problem
> > > > > with the status quo is that disabling CMDLINE doesn't disable most
> > > > > commands, relying instead on discarding them at link time.
> > > >
> > > > I don't follow you here.  We're talking only about the long help.
> > >
> > > I was actually talking about how the command code is removed. This
> > > series allows that to happen via Kconfig rather than needing a
> > > linker-script discard rule, something I only just fully appreciated.
> >
> > OK.  But this series as-is has a lot of problems and I don't see it as
> > more than a proof of concept.
> 
> Probably I need to send a few version as this discussion is becoming a
> bit theoretical.
> 
> >
> > > > There's already an option to enable/disable it.  When disabled all of
> > > > the long help text should be discarded, because we reference it via
> > > > U_BOOT_CMD macro which in turn cares about it, or not.  What's missing
> > > > is a U_BOOT_LONGHELP macro so that instead of:
> > > > #ifdef CONFIG_SYS_LONGHELP
> > > > static char cat_help_text[] =
> > > >         "<interface> <dev[:part]> <file>\n"
> > > >         "  - Print file from 'dev' on 'interface' to standard output\n";
> > > > #endif
> > > >
> > > > We do:
> > > > U_BOOT_LONGHELP(cat,
> > > >         "<interface> <dev[:part]> <file>\n"
> > > >         "  - Print file from 'dev' on 'interface' to standard output\n"
> > > > );
> > > >
> > > > Which then does:
> > > > static __maybe_unused char cat_help_text[] =
> > > > ...
> > > > ;
> > > >
> > > > And we discard the text automatically due to --gc-sections.  We possibly
> > > > haven't already been doing this since back when we first started using
> > > > --gc-sections there was a bug in binutils that caused text like the
> > > > above to still get combined in to other objects and not discarded.
> > > > That's been fixed for ages.
> > > >
> > > > And the above macro would also let us clean up U_BOOT_CMD macro slightly
> > > > to just omit the longhelp option and instead always do _CMD_HELP(_name)
> > > > inside the macros.  It'll also make it harder to add new commands
> > > > without a long help by accident.
> > >
> > > Gosh this is complicated.
> > >
> > > At present the U_BOOT_CMD() macro just drops the strings...the problem
> > > with the unused var only happens in a small number of cases where an
> > > explicit var is used. I don't see why creating a var (when none is
> > > there today) helps anything? It doesn't detect missing longhelp, since
> > > this is already an error (missing argument). Sure,  "" can be
> > > provided, but your macro doesn't stop that either.
> >
> > The problem today is that 95% of the cases surround the help text with
> > #ifdef CONFIG_SYS_LONGHELP ... #endif.  That's why the rest of the
> > macros are the way they are today.  And that in turn is due to (in part
> > at least) old compiler bugs.
> 
> I see about 46 #idefs for that and nearly 300 commands that don't have one.
> 
> >
> > > If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that
> > > we already have quite a lot...each new 'top-level' macro results in
> > > more combinations.
> >
> > It really should just be a single macro.  I think I'll make an attempt
> > at this, to show what I'm talking about.
> 
> OK thanks.
> 
> >
> > > > > With this series, it looks like we can in fact do that, so I should
> > > > > remove the discards as well.
> > > > >
> > > > > The one proviso is that this does drop a lot of things we want...e.g.
> > > > > BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
> > > > > that common filesystems are dropped. So we'll need more effort after
> > > > > this, to allow (for example) bootmeths that don't need commands to
> > > > > work correctly. But I think this series at least provides a better
> > > > > starting point for teasing things apart.
> > > > >
> > > > > So OK I'll go with __maybe_unused for the ones that need it, which
> > > > > isn't too many given that many of the strings are just included
> > > > > directly in the macro. It is considerably easier than trying to be to
> > > > > clever about things.
> > > >
> > > > Yes, this series itself has a lot of problems that need to be addressed
> > > > before reposting because I don't like "hiding" that network stuff no
> > > > longer works, and I also saw that DFU also silently failing.
> > >
> > > Yes I saw your other comments. But I think this patch is the big area
> > > of confusion.
> >
> > This isn't the tricky part of the series that I'm saying has problems,
> > that part is all of the functionality that's not being untangled and I
> > think leads to the question of what exactly is the use case where we do
> > remove the command line but don't need some of that functionality still.
> 
> For security and code-size reasons it is useful to disable commands,
> perhaps all of them. Quite a few features don't need it, but it
> certainly would take more effort to get a usable version. The goal of
> this series is to avoid people adding Kconfig mistakes which need to
> be cleaned up later.

Maybe the biggest take away should be to do things smaller.  DFU
intentionally and fundamentally constructs something for the CLI parser
to use.  Rework that.  Same for the network stack.  Do some slight
re-org / fixing of more intentional dependencies on their own.

-- 
Tom

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

Re: [PATCH 05/25] treewide: Correct use of long help

[Simon Glass]

Hi Tom,

On Sat, 7 Oct 2023 at 11:25, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Oct 07, 2023 at 09:37:07AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 6 Oct 2023 at 19:01, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 6 Oct 2023 at 10:55, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > > > > > > > 'unused variable' warning.
> > > > > > > > > > > >
> > > > > > > > > > > > Fix this treewide.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > [snip]
> > > > > > > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > > > > > > > >       return blob_encap_dek(src_addr, dst_addr, len);
> > > > > > > > > > > >  }
> > > > > > > > > > > >
> > > > > > > > > > > > -/***************************************************/
> > > > > > > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > > > > > > > >  static char dek_blob_help_text[] =
> > > > > > > > > > > >       "src dst len            - Encapsulate and create blob of data\n"
> > > > > > > > > > > >       "                         $len bits long at address $src and\n"
> > > > > > > > > > > >       "                         store the result at address $dst.\n";
> > > > > > > > > > > > +#endif
> > > > > > > > > > > >
> > > > > > > > > > > >  U_BOOT_CMD(
> > > > > > > > > > > >       dek_blob, 4, 1, do_dek_blob,
> > > > > > > > > > >
> > > > > > > > > > > This really doesn't read nicely.  I would rather (globally and fix
> > > > > > > > > > > existing users) __maybe_unused this instead.  I think there's just one
> > > > > > > > > > > example today that isn't "foo_help_text".
> > > > > > > > > >
> > > > > > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > > > > > > > linker to discard it when not needed?
> > > > > > > > >
> > > > > > > > > Well, I don't think we need linker list magic when __maybe_unused will
> > > > > > > > > just have them be discarded normally.
> > > > > > > >
> > > > > > > > Yes, perhaps things are in a better state than they used to be, but
> > > > > > > > there is a linker discard for commands at present.
> > > > > > >
> > > > > > > Yes, but __maybe_unused means we don't get a warning about it, and it's
> > > > > > > literally discarded as part of --gc-sections as it done everywhere with
> > > > > > > maybe 3 exceptions?
> > > > > >
> > > > > > Actually with this series we get a lot closer to that. The problem
> > > > > > with the status quo is that disabling CMDLINE doesn't disable most
> > > > > > commands, relying instead on discarding them at link time.
> > > > >
> > > > > I don't follow you here.  We're talking only about the long help.
> > > >
> > > > I was actually talking about how the command code is removed. This
> > > > series allows that to happen via Kconfig rather than needing a
> > > > linker-script discard rule, something I only just fully appreciated.
> > >
> > > OK.  But this series as-is has a lot of problems and I don't see it as
> > > more than a proof of concept.
> >
> > Probably I need to send a few version as this discussion is becoming a
> > bit theoretical.
> >
> > >
> > > > > There's already an option to enable/disable it.  When disabled all of
> > > > > the long help text should be discarded, because we reference it via
> > > > > U_BOOT_CMD macro which in turn cares about it, or not.  What's missing
> > > > > is a U_BOOT_LONGHELP macro so that instead of:
> > > > > #ifdef CONFIG_SYS_LONGHELP
> > > > > static char cat_help_text[] =
> > > > >         "<interface> <dev[:part]> <file>\n"
> > > > >         "  - Print file from 'dev' on 'interface' to standard output\n";
> > > > > #endif
> > > > >
> > > > > We do:
> > > > > U_BOOT_LONGHELP(cat,
> > > > >         "<interface> <dev[:part]> <file>\n"
> > > > >         "  - Print file from 'dev' on 'interface' to standard output\n"
> > > > > );
> > > > >
> > > > > Which then does:
> > > > > static __maybe_unused char cat_help_text[] =
> > > > > ...
> > > > > ;
> > > > >
> > > > > And we discard the text automatically due to --gc-sections.  We possibly
> > > > > haven't already been doing this since back when we first started using
> > > > > --gc-sections there was a bug in binutils that caused text like the
> > > > > above to still get combined in to other objects and not discarded.
> > > > > That's been fixed for ages.
> > > > >
> > > > > And the above macro would also let us clean up U_BOOT_CMD macro slightly
> > > > > to just omit the longhelp option and instead always do _CMD_HELP(_name)
> > > > > inside the macros.  It'll also make it harder to add new commands
> > > > > without a long help by accident.
> > > >
> > > > Gosh this is complicated.
> > > >
> > > > At present the U_BOOT_CMD() macro just drops the strings...the problem
> > > > with the unused var only happens in a small number of cases where an
> > > > explicit var is used. I don't see why creating a var (when none is
> > > > there today) helps anything? It doesn't detect missing longhelp, since
> > > > this is already an error (missing argument). Sure,  "" can be
> > > > provided, but your macro doesn't stop that either.
> > >
> > > The problem today is that 95% of the cases surround the help text with
> > > #ifdef CONFIG_SYS_LONGHELP ... #endif.  That's why the rest of the
> > > macros are the way they are today.  And that in turn is due to (in part
> > > at least) old compiler bugs.
> >
> > I see about 46 #idefs for that and nearly 300 commands that don't have one.
> >
> > >
> > > > If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that
> > > > we already have quite a lot...each new 'top-level' macro results in
> > > > more combinations.
> > >
> > > It really should just be a single macro.  I think I'll make an attempt
> > > at this, to show what I'm talking about.
> >
> > OK thanks.
> >
> > >
> > > > > > With this series, it looks like we can in fact do that, so I should
> > > > > > remove the discards as well.
> > > > > >
> > > > > > The one proviso is that this does drop a lot of things we want...e.g.
> > > > > > BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
> > > > > > that common filesystems are dropped. So we'll need more effort after
> > > > > > this, to allow (for example) bootmeths that don't need commands to
> > > > > > work correctly. But I think this series at least provides a better
> > > > > > starting point for teasing things apart.
> > > > > >
> > > > > > So OK I'll go with __maybe_unused for the ones that need it, which
> > > > > > isn't too many given that many of the strings are just included
> > > > > > directly in the macro. It is considerably easier than trying to be to
> > > > > > clever about things.
> > > > >
> > > > > Yes, this series itself has a lot of problems that need to be addressed
> > > > > before reposting because I don't like "hiding" that network stuff no
> > > > > longer works, and I also saw that DFU also silently failing.
> > > >
> > > > Yes I saw your other comments. But I think this patch is the big area
> > > > of confusion.
> > >
> > > This isn't the tricky part of the series that I'm saying has problems,
> > > that part is all of the functionality that's not being untangled and I
> > > think leads to the question of what exactly is the use case where we do
> > > remove the command line but don't need some of that functionality still.
> >
> > For security and code-size reasons it is useful to disable commands,
> > perhaps all of them. Quite a few features don't need it, but it
> > certainly would take more effort to get a usable version. The goal of
> > this series is to avoid people adding Kconfig mistakes which need to
> > be cleaned up later.
>
> Maybe the biggest take away should be to do things smaller.  DFU
> intentionally and fundamentally constructs something for the CLI parser
> to use.  Rework that.  Same for the network stack.  Do some slight
> re-org / fixing of more intentional dependencies on their own.

I think it is better to do it the other way around, to avoid further
Kconfig regressions. At the moment we can't actually turn off CMDLINE,
so none of the work can actually be used.

The particular thing I am interested in is booting an OS without CONFIG_CMDLINE

Regards,
Simon

[PATCH 06/25] test: Make UNIT_TEST depend on CMDLINE

[Simon Glass]

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 ut
uses the command line. Leave that for later.

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

 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.
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 07/25] tegra: Change #ifdef for nop

[Simon Glass]

This code is normally compiled for Tegra, 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.

Correct the condition.

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

 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
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH 07/25] tegra: Change #ifdef for nop

[Tom Rini]

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

On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:

> This code is normally compiled for Tegra, 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.
> 
> Correct the condition.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  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

Tegra? Do you mean sifive?  That's where CLK_K210 stuff is... but it
also seems wrong, you can run unit test on real hardware, and this is a
test that could (should?) be run on that platform.

-- 
Tom

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

Re: [PATCH 07/25] tegra: Change #ifdef for nop

[Simon Glass]

Hi Tom.

On Sun, 24 Sept 2023 at 18:43, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
>
> > This code is normally compiled for Tegra, 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.
> >
> > Correct the condition.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  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
>
> Tegra? Do you mean sifive?  That's where CLK_K210 stuff is... but it

Oh yes, I got confused.

> also seems wrong, you can run unit test on real hardware, and this is a
> test that could (should?) be run on that platform.

Only if it enables UNIT_TEST. You cannot run unit tests without that.
The current tests are designed for sandbox.

>
> --
> Tom

Regards,
Simon

Re: [PATCH 07/25] tegra: Change #ifdef for nop

[Sean Anderson]

On 10/7/23 19:10, Simon Glass wrote:
> Hi Tom.
> 
> On Sun, 24 Sept 2023 at 18:43, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
>>
>>> This code is normally compiled for Tegra, 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.
>>>
>>> Correct the condition.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>>   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
>>
>> Tegra? Do you mean sifive?  That's where CLK_K210 stuff is... but it
> 
> Oh yes, I got confused.
> 
>> also seems wrong, you can run unit test on real hardware, and this is a
>> test that could (should?) be run on that platform.
> 
> Only if it enables UNIT_TEST. You cannot run unit tests without that.
> The current tests are designed for sandbox.

FWIW I have run this test on actual hardware. My intent here was to allow
unit tests to access functions which would otherwise be declared static.

--Sean

Re: [PATCH 07/25] tegra: Change #ifdef for nop

[Simon Glass]

Hi Sean,

On Sat, 7 Oct 2023 at 17:21, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 10/7/23 19:10, Simon Glass wrote:
> > Hi Tom.
> >
> > On Sun, 24 Sept 2023 at 18:43, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
> >>
> >>> This code is normally compiled for Tegra, 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.
> >>>
> >>> Correct the condition.
> >>>
> >>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>> ---
> >>>
> >>>   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
> >>
> >> Tegra? Do you mean sifive?  That's where CLK_K210 stuff is... but it
> >
> > Oh yes, I got confused.
> >
> >> also seems wrong, you can run unit test on real hardware, and this is a
> >> test that could (should?) be run on that platform.
> >
> > Only if it enables UNIT_TEST. You cannot run unit tests without that.
> > The current tests are designed for sandbox.
>
> FWIW I have run this test on actual hardware. My intent here was to allow
> unit tests to access functions which would otherwise be declared static.

Er, with or without UNIT_TEST enabled? How can it even build if this
declaration is only for sandbox?

Regards,
Simon

Re: [PATCH 07/25] tegra: Change #ifdef for nop

[Sean Anderson]

On 10/9/23 11:32, Simon Glass wrote:
> Hi Sean,
> 
> On Sat, 7 Oct 2023 at 17:21, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 10/7/23 19:10, Simon Glass wrote:
>>> Hi Tom.
>>>
>>> On Sun, 24 Sept 2023 at 18:43, Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
>>>>
>>>>> This code is normally compiled for Tegra, 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.
>>>>>
>>>>> Correct the condition.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>>    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
>>>>
>>>> Tegra? Do you mean sifive?  That's where CLK_K210 stuff is... but it
>>>
>>> Oh yes, I got confused.
>>>
>>>> also seems wrong, you can run unit test on real hardware, and this is a
>>>> test that could (should?) be run on that platform.
>>>
>>> Only if it enables UNIT_TEST. You cannot run unit tests without that.
>>> The current tests are designed for sandbox.
>>
>> FWIW I have run this test on actual hardware. My intent here was to allow
>> unit tests to access functions which would otherwise be declared static.
> 
> Er, with or without UNIT_TEST enabled? How can it even build if this
> declaration is only for sandbox?

With UNIT_TEST of course. Although since this is a forward-declaration, the
UNIT_TEST ifdef isn't really even necessary. If it's on actual hardware, nop
should already be defined. So maybe this should be something like

#if CONFIG_SANDBOX
#define nop()
#endif

--Sean

Re: [PATCH 07/25] tegra: Change #ifdef for nop

[Simon Glass]

Hi Sean,

On Mon, 9 Oct 2023 at 17:40, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 10/9/23 11:32, Simon Glass wrote:
> > Hi Sean,
> >
> > On Sat, 7 Oct 2023 at 17:21, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 10/7/23 19:10, Simon Glass wrote:
> >>> Hi Tom.
> >>>
> >>> On Sun, 24 Sept 2023 at 18:43, Tom Rini <trini@konsulko.com> wrote:
> >>>>
> >>>> On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
> >>>>
> >>>>> This code is normally compiled for Tegra, 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.
> >>>>>
> >>>>> Correct the condition.
> >>>>>
> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>>>> ---
> >>>>>
> >>>>>    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
> >>>>
> >>>> Tegra? Do you mean sifive?  That's where CLK_K210 stuff is... but it
> >>>
> >>> Oh yes, I got confused.
> >>>
> >>>> also seems wrong, you can run unit test on real hardware, and this is a
> >>>> test that could (should?) be run on that platform.
> >>>
> >>> Only if it enables UNIT_TEST. You cannot run unit tests without that.
> >>> The current tests are designed for sandbox.
> >>
> >> FWIW I have run this test on actual hardware. My intent here was to allow
> >> unit tests to access functions which would otherwise be declared static.
> >
> > Er, with or without UNIT_TEST enabled? How can it even build if this
> > declaration is only for sandbox?
>
> With UNIT_TEST of course. Although since this is a forward-declaration, the
> UNIT_TEST ifdef isn't really even necessary. If it's on actual hardware, nop
> should already be defined. So maybe this should be something like
>
> #if CONFIG_SANDBOX
> #define nop()
> #endif

It is the CONFIG_SANDBOX that I am trying to remove. Can it be
CONFIG_UNIT_TEST instead?

Regards,
Simon

Re: [PATCH 07/25] tegra: Change #ifdef for nop

[Sean Anderson]

On 10/10/23 10:42, Simon Glass wrote:
> Hi Sean,
> 
> On Mon, 9 Oct 2023 at 17:40, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 10/9/23 11:32, Simon Glass wrote:
>>> Hi Sean,
>>>
>>> On Sat, 7 Oct 2023 at 17:21, Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> On 10/7/23 19:10, Simon Glass wrote:
>>>>> Hi Tom.
>>>>>
>>>>> On Sun, 24 Sept 2023 at 18:43, Tom Rini <trini@konsulko.com> wrote:
>>>>>>
>>>>>> On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
>>>>>>
>>>>>>> This code is normally compiled for Tegra, 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.
>>>>>>>
>>>>>>> Correct the condition.
>>>>>>>
>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>>>> ---
>>>>>>>
>>>>>>>     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
>>>>>>
>>>>>> Tegra? Do you mean sifive?  That's where CLK_K210 stuff is... but it
>>>>>
>>>>> Oh yes, I got confused.
>>>>>
>>>>>> also seems wrong, you can run unit test on real hardware, and this is a
>>>>>> test that could (should?) be run on that platform.
>>>>>
>>>>> Only if it enables UNIT_TEST. You cannot run unit tests without that.
>>>>> The current tests are designed for sandbox.
>>>>
>>>> FWIW I have run this test on actual hardware. My intent here was to allow
>>>> unit tests to access functions which would otherwise be declared static.
>>>
>>> Er, with or without UNIT_TEST enabled? How can it even build if this
>>> declaration is only for sandbox?
>>
>> With UNIT_TEST of course. Although since this is a forward-declaration, the
>> UNIT_TEST ifdef isn't really even necessary. If it's on actual hardware, nop
>> should already be defined. So maybe this should be something like
>>
>> #if CONFIG_SANDBOX
>> #define nop()
>> #endif
> 
> It is the CONFIG_SANDBOX that I am trying to remove. Can it be
> CONFIG_UNIT_TEST instead?

Well, you can just remove the `ifdef UNIT_TEST` then.

--Sean

[PATCH 08/25] fastboot: Avoid depending on CMDLINE

[Simon Glass]

When CMDLINE is not enabled, this code fails to build. Correct this by
adding conditions.

Note that this should not happen in normal use, since the use of
'select CMDLINE' will cause a visible warning. But it is needed for the
sandbox build to pass without CMDLINE.

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

 drivers/fastboot/fb_command.c |  3 ++-
 drivers/fastboot/fb_common.c  | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index 71cfaec6e9dc..4e52e6f0f8bf 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -346,7 +346,8 @@ static char g_a_cmd_buff[64];
 
 void fastboot_acmd_complete(void)
 {
-	run_command(g_a_cmd_buff, 0);
+	if (IS_ENABLED(CONFIG_CMDLINE))
+		run_command(g_a_cmd_buff, 0);
 }
 
 /**
diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
index 4e9d9b719c6f..35b7aafe5af3 100644
--- a/drivers/fastboot/fb_common.c
+++ b/drivers/fastboot/fb_common.c
@@ -132,6 +132,13 @@ void fastboot_boot(void)
 {
 	char *s;
 
+	/*
+	 * Avoid a build error; this will always have generated a Kconfig
+	 * warning about CMDLINE not being enabled
+	 */
+	if (!IS_ENABLED(CONFIG_CMDLINE))
+		return;
+
 	s = env_get("fastboot_bootcmd");
 	if (s) {
 		run_command(s, CMD_FLAG_ENV);
@@ -170,8 +177,12 @@ void fastboot_handle_boot(int command, bool success)
 
 	switch (command) {
 	case FASTBOOT_COMMAND_BOOT:
-		fastboot_boot();
-		net_set_state(NETLOOP_SUCCESS);
+		if (IS_ENABLED(CONFIG_CMDLINE)) {
+			fastboot_boot();
+			net_set_state(NETLOOP_SUCCESS);
+		} else {
+			net_set_state(NETLOOP_FAIL);
+		}
 		break;
 
 	case FASTBOOT_COMMAND_CONTINUE:
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH 08/25] fastboot: Avoid depending on CMDLINE

[Tom Rini]

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

On Sun, Sep 24, 2023 at 02:39:26PM -0600, Simon Glass wrote:

> When CMDLINE is not enabled, this code fails to build. Correct this by
> adding conditions.
> 
> Note that this should not happen in normal use, since the use of
> 'select CMDLINE' will cause a visible warning. But it is needed for the
> sandbox build to pass without CMDLINE.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/fastboot/fb_command.c |  3 ++-
>  drivers/fastboot/fb_common.c  | 15 +++++++++++++--
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index 71cfaec6e9dc..4e52e6f0f8bf 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -346,7 +346,8 @@ static char g_a_cmd_buff[64];
>  
>  void fastboot_acmd_complete(void)
>  {
> -	run_command(g_a_cmd_buff, 0);
> +	if (IS_ENABLED(CONFIG_CMDLINE))
> +		run_command(g_a_cmd_buff, 0);
>  }
>  
>  /**
> diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> index 4e9d9b719c6f..35b7aafe5af3 100644
> --- a/drivers/fastboot/fb_common.c
> +++ b/drivers/fastboot/fb_common.c
> @@ -132,6 +132,13 @@ void fastboot_boot(void)
>  {
>  	char *s;
>  
> +	/*
> +	 * Avoid a build error; this will always have generated a Kconfig
> +	 * warning about CMDLINE not being enabled
> +	 */
> +	if (!IS_ENABLED(CONFIG_CMDLINE))
> +		return;
> +
>  	s = env_get("fastboot_bootcmd");
>  	if (s) {
>  		run_command(s, CMD_FLAG_ENV);
> @@ -170,8 +177,12 @@ void fastboot_handle_boot(int command, bool success)
>  
>  	switch (command) {
>  	case FASTBOOT_COMMAND_BOOT:
> -		fastboot_boot();
> -		net_set_state(NETLOOP_SUCCESS);
> +		if (IS_ENABLED(CONFIG_CMDLINE)) {
> +			fastboot_boot();
> +			net_set_state(NETLOOP_SUCCESS);
> +		} else {
> +			net_set_state(NETLOOP_FAIL);
> +		}
>  		break;
>  
>  	case FASTBOOT_COMMAND_CONTINUE:

All of this just means it now fails to work, yes?

-- 
Tom

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

Re: [PATCH 08/25] fastboot: Avoid depending on CMDLINE

[Simon Glass]

Hi Tom,

On Sun, 24 Sept 2023 at 16:59, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Sep 24, 2023 at 02:39:26PM -0600, Simon Glass wrote:
>
> > When CMDLINE is not enabled, this code fails to build. Correct this by
> > adding conditions.
> >
> > Note that this should not happen in normal use, since the use of
> > 'select CMDLINE' will cause a visible warning. But it is needed for the
> > sandbox build to pass without CMDLINE.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  drivers/fastboot/fb_command.c |  3 ++-
> >  drivers/fastboot/fb_common.c  | 15 +++++++++++++--
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> > index 71cfaec6e9dc..4e52e6f0f8bf 100644
> > --- a/drivers/fastboot/fb_command.c
> > +++ b/drivers/fastboot/fb_command.c
> > @@ -346,7 +346,8 @@ static char g_a_cmd_buff[64];
> >
> >  void fastboot_acmd_complete(void)
> >  {
> > -     run_command(g_a_cmd_buff, 0);
> > +     if (IS_ENABLED(CONFIG_CMDLINE))
> > +             run_command(g_a_cmd_buff, 0);
> >  }
> >
> >  /**
> > diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> > index 4e9d9b719c6f..35b7aafe5af3 100644
> > --- a/drivers/fastboot/fb_common.c
> > +++ b/drivers/fastboot/fb_common.c
> > @@ -132,6 +132,13 @@ void fastboot_boot(void)
> >  {
> >       char *s;
> >
> > +     /*
> > +      * Avoid a build error; this will always have generated a Kconfig
> > +      * warning about CMDLINE not being enabled
> > +      */
> > +     if (!IS_ENABLED(CONFIG_CMDLINE))
> > +             return;
> > +
> >       s = env_get("fastboot_bootcmd");
> >       if (s) {
> >               run_command(s, CMD_FLAG_ENV);
> > @@ -170,8 +177,12 @@ void fastboot_handle_boot(int command, bool success)
> >
> >       switch (command) {
> >       case FASTBOOT_COMMAND_BOOT:
> > -             fastboot_boot();
> > -             net_set_state(NETLOOP_SUCCESS);
> > +             if (IS_ENABLED(CONFIG_CMDLINE)) {
> > +                     fastboot_boot();
> > +                     net_set_state(NETLOOP_SUCCESS);
> > +             } else {
> > +                     net_set_state(NETLOOP_FAIL);
> > +             }
> >               break;
> >
> >       case FASTBOOT_COMMAND_CONTINUE:
>
> All of this just means it now fails to work, yes?

It actually fails to build, since there is a Kconfig conflict, as
mentioned in the commit message. The use of 'select FASTBOOT' when
CMDLINE is not enabled produces an error.

I will see if I can do this another way.

Regards,
Simon

[PATCH 09/25] cli: Always build cli_getch

[Simon Glass]

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>
---

 common/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/Makefile b/common/Makefile
index 5c1617206f07..15ae46f596d0 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
 
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 10/25] cmd: Use an #ifdef around run_commandf()

[Simon Glass]

This is not available if CMDLINE is disabled, so add an #ifdef to correct
this.

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

 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 */
 
 /****************************************************************************/
 
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 11/25] Move bootmenu_conv_key() into its own file

[Simon Glass]

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>
---

 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 15ae46f596d0..29a4c818a700 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;
+}
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 12/25] armffa: Correct command help condition

[Simon Glass]

The help text should not be build unless CONFIG_SYS_LONGHELP is
enabled. Add this as a condition.

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

 cmd/armffa.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cmd/armffa.c b/cmd/armffa.c
index 7e6eafc03ad7..28b65678a8e9 100644
--- a/cmd/armffa.c
+++ b/cmd/armffa.c
@@ -188,6 +188,7 @@ static int do_ffa_devlist(struct cmd_tbl *cmdtp, int flag, int argc, char *const
 	return CMD_RET_SUCCESS;
 }
 
+#ifdef CONFIG_SYS_LONGHELP
 static char armffa_help_text[] =
 	"getpart <partition UUID>\n"
 	"       - lists the partition(s) info\n"
@@ -195,6 +196,7 @@ static char armffa_help_text[] =
 	"       - sends a data pattern to the specified partition\n"
 	"devlist\n"
 	"       - displays information about the FF-A device/driver\n";
+#endif
 
 U_BOOT_CMD_WITH_SUBCMDS(armffa, "Arm FF-A test command", armffa_help_text,
 			U_BOOT_SUBCMD_MKENT(getpart, 2, 1, do_ffa_getpart),
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 13/25] pxe: Depend on CMDLINE

[Simon Glass]

We cannot use PXE or sysboot commands without CONFIG_CMDLINE so add the
required condition.

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

 cmd/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index cdc22a067b27..a254c41110ec 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1995,6 +1995,7 @@ config CMD_ETHSW
 
 config CMD_PXE
 	bool "pxe"
+	depends on CMDLINE
 	select PXE_UTILS
 	help
 	  Boot image via network using PXE protocol
@@ -2230,6 +2231,7 @@ config CMD_SOUND
 
 config CMD_SYSBOOT
 	bool "sysboot"
+	depends on CMDLINE
 	select PXE_UTILS
 	help
 	  Boot image via local extlinux.conf file
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 14/25] env: Split out non-command code into a new file

[Simon Glass]

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>
---

 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 9bebf321c397..5be2b4bd3800 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -240,7 +240,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 fe99157fd17b..0c9efa052baf 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -48,20 +48,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
@@ -194,108 +180,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[])
 {
@@ -455,7 +341,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,
@@ -528,7 +414,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
@@ -678,7 +564,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_ */
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 15/25] console: Move SYS_PBSIZE into common/

[Simon Glass]

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>
---

 cmd/Kconfig    | 5 -----
 common/Kconfig | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index a254c41110ec..5f6834b335dc 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 21eaa5e815f9..08d490ad4776 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
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 16/25] bootm: Allow building when cleanup functions are missing

[Simon Glass]

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>
---

 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 b1c3afe0a3ad..ee8c79a650c2 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
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 17/25] fdt: Move working_fdt into fdt_support

[Simon Glass]

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>
---

 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 2401ea8b44cb..f842fd84b4b2 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;
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 18/25] net: Depend on CONFIG_CMDLINE

[Simon Glass]

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>
---

 cmd/Kconfig | 1 +
 net/Kconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5f6834b335dc..c3428d19f31d 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1687,6 +1687,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
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH 18/25] net: Depend on CONFIG_CMDLINE

[Ramon Fried]

On Sun, Sep 24, 2023 at 11:40 PM Simon Glass <sjg@chromium.org> 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>
> ---
>
>  cmd/Kconfig | 1 +
>  net/Kconfig | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5f6834b335dc..c3428d19f31d 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1687,6 +1687,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
> --
> 2.42.0.515.g380fc7ccd1-goog
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

[PATCH 19/25] log: Allow use without CONFIG_CMDLINE

[Simon Glass]

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>
---

 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);
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 20/25] video: Allow use without CONFIG_CMDLINE

[Simon Glass]

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>
---

 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 0f9bb49e44f7..8186b5b49f0b 100644
--- a/drivers/video/console_truetype.c
+++ b/drivers/video/console_truetype.c
@@ -124,7 +124,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
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 21/25] video: Dont require the font command

[Simon Glass]

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>
---

 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.
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 22/25] efi: Depend on CMDLINE for efi_loader

[Simon Glass]

This features currently 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>
---

 lib/efi_loader/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 621ed5e5b0fb..2aef9336034e 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -12,6 +12,7 @@ config EFI_LOADER
 	depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT
 	depends on BLK
 	depends on !EFI_APP
+	depends on CMDLINE
 	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
 	select CHARSET
 	# We need to send DM events, dynamically, in the EFI block driver
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 23/25] cmd: Make all commands depend on CMDLINE

[Simon Glass]

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>
---

 cmd/Kconfig | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index c3428d19f31d..7aa689e91f6f 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1,7 +1,5 @@
-menu "Command line interface"
-
-config CMDLINE
-	bool "Support U-Boot commands"
+menuconfig CMDLINE
+	bool "Command line interface"
 	default y
 	help
 	  Enable U-Boot's command-line functions. This provides a means
@@ -11,9 +9,10 @@ config CMDLINE
 	  Depending on the number of commands enabled, this can add
 	  substantially to the size of U-Boot.
 
+if CMDLINE
+
 config HUSH_PARSER
 	bool "Use hush shell"
-	depends on CMDLINE
 	help
 	  This option enables the "hush" shell (from Busybox) as command line
 	  interpreter, thus enabling powerful command line syntax like
@@ -25,7 +24,6 @@ config HUSH_PARSER
 
 config CMDLINE_EDITING
 	bool "Enable command line editing"
-	depends on CMDLINE
 	default y
 	help
 	  Enable editing and History functions for interactive command line
@@ -40,15 +38,13 @@ config CMDLINE_PS_SUPPORT
 
 config AUTO_COMPLETE
 	bool "Enable auto complete using TAB"
-	depends on CMDLINE
 	default y
 	help
 	  Enable auto completion of commands using TAB.
 
 config SYS_LONGHELP
 	bool "Enable long help messages"
-	depends on CMDLINE
-	default y if CMDLINE
+	default y
 	help
 	  Defined when you want long help messages included
 	  Do not set this option when short of memory.
@@ -86,8 +82,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
@@ -219,7 +214,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.
@@ -409,7 +403,6 @@ source lib/efi_selftest/Kconfig
 
 config CMD_BOOTMENU
 	bool "bootmenu"
-	depends on CMDLINE
 	select MENU
 	select CHARSET
 	help
@@ -476,7 +469,6 @@ config CMD_GO
 
 config CMD_RUN
 	bool "run"
-	depends on CMDLINE
 	default y
 	help
 	  Run the command in the given environment variable.
@@ -572,7 +564,6 @@ menu "Environment commands"
 
 config CMD_ASKENV
 	bool "ask for env variable"
-	depends on CMDLINE
 	help
 	  Ask for environment variable
 
@@ -1687,7 +1678,6 @@ if NET
 
 menuconfig CMD_NET
 	bool "Network commands"
-	depends on CMDLINE
 	default y
 	imply NETDEVICES
 
@@ -1991,7 +1981,6 @@ config CMD_ETHSW
 
 config CMD_PXE
 	bool "pxe"
-	depends on CMDLINE
 	select PXE_UTILS
 	help
 	  Boot image via network using PXE protocol
@@ -2126,7 +2115,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.
@@ -2227,14 +2215,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
@@ -2883,4 +2869,5 @@ config CMD_MESON
 	default y
 	help
 	  Enable useful commands for the Meson Soc family developed by Amlogic Inc.
-endmenu
+
+endif # CMDLINE
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 24/25] sandbox: Avoid requiring cmdline

[Simon Glass]

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>
---

 arch/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 19f2891ba1c5..789be7a9f1e8 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,7 @@ config SANDBOX
 	imply CMD_IO
 	imply CMD_IOTRACE
 	imply CMD_LZMADEC
+	imply CMD_POWEROFF
 	imply CMD_SF
 	imply CMD_SF_TEST
 	imply CRC32_VERIFY
@@ -208,6 +206,8 @@ config SANDBOX
 	imply PHYSMEM
 	imply GENERATE_ACPI_TABLE
 	imply BINMAN
+	imply SYSRESET_CMD_POWEROFF
+	imply SUPPORT_EXTENSION_SCAN
 
 config SH
 	bool "SuperH architecture"
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH 25/25] sandbox: Add a test for disabling CONFIG_CMDLINE

[Simon Glass]

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

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

 test/py/tests/test_sandbox_opts.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 test/py/tests/test_sandbox_opts.py

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

Re: [PATCH 00/25] Tidy up use of CONFIG_CMDLINE

[Tom Rini]

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

On Sun, Sep 24, 2023 at 02:39:18PM -0600, Simon Glass wrote:

> 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 of CONFIG_CMDLINE is
> disabled, e.g. networking and most booting options. Further work is
> needed to make the option more generally useful.

I worry there's a lot of "make it compile, deal with it later" in here
rather than unwinding so that $X works with CMDLINE disabled or we truly
must have CMDLINE.  Perhaps it would be better to start with to take one
of the platforms that enables say networking in SPL, where we
functionally don't have a cmdline, and make that build with CMDLINE off.
Having *PL build and link and work with CMDLINE disabled would possibly
save some space too, which is always a good thing there.

-- 
Tom

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

Re: [PATCH 00/25] Tidy up use of CONFIG_CMDLINE

[Simon Glass]

Hi Tom,

On Sun, 24 Sept 2023 at 18:37, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Sep 24, 2023 at 02:39:18PM -0600, Simon Glass wrote:
>
> > 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 of CONFIG_CMDLINE is
> > disabled, e.g. networking and most booting options. Further work is
> > needed to make the option more generally useful.
>
> I worry there's a lot of "make it compile, deal with it later" in here
> rather than unwinding so that $X works with CMDLINE disabled or we truly
> must have CMDLINE.  Perhaps it would be better to start with to take one
> of the platforms that enables say networking in SPL, where we
> functionally don't have a cmdline, and make that build with CMDLINE off.
> Having *PL build and link and work with CMDLINE disabled would possibly
> save some space too, which is always a good thing there.

So long as it gets us closer to the goal, I think the work is valuable.
Also it prevents additional regressions from creeping in, at least for
sandbox. I am under no illusions about how big an effort this is, but we
have to start somewhere. To me the easiest first step would be to decouple
bootm functionality from the bootm command. Likely that would be <40
patches.

Regards,
Simon