From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick DELAUNAY Date: Fri, 9 Oct 2020 08:10:06 +0000 Subject: [PATCH] Define default CONFIG_PREBOOT with right config option In-Reply-To: <20201007154459.GV14816@bill-the-cat> References: <20200929094814.1229177-1-pbrobinson@gmail.com> <3ed621e519354113a0113c1c20829a89@SFHDAG2NODE3.st.com> <20201007154459.GV14816@bill-the-cat> Message-ID: <7c10371b8b574d239c68a952bfa91023@SFHDAG2NODE3.st.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon and Tom > From: Tom Rini > 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 > wrote: > > > > > > Hi, > > > > > > > From: U-Boot 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 > > > > Cc: Jonas Smedegaard > > > > Cc: Neil Armstrong > > > > --- > > > > 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 at st.com/ Regards Patrick