[PATCH] Define default CONFIG_PREBOOT with right config option

[Patrick DELAUNAY]

Hi Simon and Tom

> From: Tom Rini <trini at konsulko.com>
> Sent: mercredi 7 octobre 2020 17:45
> 
> On Wed, Oct 07, 2020 at 07:26:55AM -0600, Simon Glass wrote:
> > Hi Patrick,
> >
> > On Wed, 7 Oct 2020 at 02:38, Patrick DELAUNAY <patrick.delaunay at st.com>
> wrote:
> > >
> > > Hi,
> > >
> > > > From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Peter
> > > > Robinson
> > > > Sent: mardi 29 septembre 2020 11:48
> > > >
> > > > The 44758771ee commit removes CONFIG_PREBOOT but actually sets the
> > > > USE_PREBOOT Kconfig option which isn't CONFIG_PREBOOT and is also
> > > > a bool option which means we regress because 'usb start' isn't run
> > > > when expected, it should also be run for devices that have USB
> > > > storage because keyboards aren't the only thing we might need the USB
> bus for.
> > > >
> > > > Fixes: 44758771ee ("arm: move CONFIG_PREBOOT="usb start" to
> > > > KConfig")
> > > > Signed-off-by: Peter Robinson <pbrobinson at gmail.com>
> > > > Cc: Jonas Smedegaard <dr at jones.dk>
> > > > Cc: Neil Armstrong <narmstrong at baylibre.com>
> > > > ---
> > > >  common/Kconfig | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/common/Kconfig b/common/Kconfig index
> > > > b1934b3a9c..9c20a9738e
> > > > 100644
> > > > --- a/common/Kconfig
> > > > +++ b/common/Kconfig
> > > > @@ -403,7 +403,6 @@ config BOOTCOMMAND
> > > >
> > > >  config USE_PREBOOT
> > > >       bool "Enable preboot"
> > > > -     default "usb start" if USB_KEYBOARD
> > > >       help
> > > >         When this option is enabled, the existence of the environment
> > > >         variable "preboot" will be checked immediately before
> > > > starting the @@ -
> > > > 417,6 +416,7 @@ config USE_PREBOOT  config PREBOOT
> > > >       string "preboot default value"
> > > >       depends on USE_PREBOOT && !USE_DEFAULT_ENV_FILE
> > > > +     default "usb start" if USB_KEYBOARD || USB_STORAGE
> > > >       default ""
> > > >       help
> > > >         This is the default of "preboot" environment variable.
> > > > --
> > > > 2.26.2
> > >
> > > For information, this patch cause unexpected 'usb start' on
> > > STM32MP15x boards and slow down the start-up in realease v2020.10.
> > >
> > > For me it is unexpected because
> > > - USB keyboard is not activated
> > > - USB storage is activated but USB boot is not supported (not
> > > managed by distro boot command)
> > >
> > > I sent a patch [1] for the associated defconfig but I'm afraid that other boards
> are impacted.
> > >
> > > As the USB storage boot initialization is correctly managed by distro boot
> command 'usb_boot'
> > > (defined in include/config_distro_bootcmd.h, it already include 'usb
> > > start'), I think that the USB_STORAGE test should be removed or limited by
> !DISTRO_DEFAULTS.
> >
> > Perhaps PREBOOT should depend on USE_PREBOOT?
> 
> It does.  And ARCH_STM32MP does "imply USE_PREBOOT" which is maybe the
> root problem?
> --
> Tom

I activate CONFIG_USE_PREBOOT to handle the "preboot" variable in
common/main.c::main_loop()

	if (IS_ENABLED(CONFIG_USE_PREBOOT))
		run_preboot_environment_command();

In my case the preboot variable is dynamically build in
arch/arm/mach-stm32mp/cpu.c::setup_boot_mode()
to handle the reboot reason provided by Linux kernel in a TAMPER register

And it is why I activate the USE_PREBOOT with imply in mach-stm32mp Kconfig.

So the expected configuration for me is 
- CONFIG_USE_PREBOOT=y		=> variable preboot is handle
- CONFIG_PREBOOT=""		=> default value of variable

After this patch, the value CONFIG_PREBOOT change because
CONFIG_ USB_STORAGE is also activated in stm32mp15 defconfig.

Anyway I will solve the issue for my defconfig with [1]
But I am still confuse with this this patch:

1/ I understood why preboot = "usb start" when the USB keyboard is activated

     keyboard can be use to interrupt the boot and enter new command,

2/ I don't understood why it is need for USB storage by default....

    'usb start' can't be handle in bootcmd, as it is done in distro bootcmd ? 

    Why usb device had to be available before the bootcmd is executed in main loop ?

    I don't found boot use-case where it is needed when distro bootcmd is used...

I just ask some clarification on this part:

+     default "usb start" iUSB_STORAGE
,

And raise a potential issue for other platforms with
- CONFIG_USB_STORAGE=y
- CONFIG_USE_PREBOOT=y
- CONFIG_PREBOOT not defined
- preboot build dynamically:

arch/arm/mach-imx/mx6/opos6ul.c:68:		env_set("preboot", "");
arch/arm/mach-rockchip/boot_mode.c:98:		env_set("preboot", "setenv preboot; fastboot usb0");
arch/arm/mach-rockchip/boot_mode.c:102:		env_set("preboot", "setenv preboot; ums mmc 0");
board/syteco/zmx25/zmx25.c:162:				env_set("preboot", "run gs_slow_boot");
board/syteco/zmx25/zmx25.c:164:				env_set("preboot", "run gs_fast_boot");
board/boundary/nitrogen6x/nitrogen6x.c:966:				env_set("preboot", cmd);
board/rockchip/kylin_rk3036/kylin_rk3036.c:46:		env_set("preboot", "setenv preboot; fastboot usb0");

[1] = "configs: stm32mp: force empty PREBOOT"
http://patchwork.ozlabs.org/project/uboot/patch/20201007081020.30635-1-patrick.delaunay@st.com/

Regards
Patrick