| Message ID | 20200929094814.1229177-1-pbrobinson@gmail.com |
|---|---|
| State | Accepted |
| Commit | 324d77998ed68aa3ac53bfb6d3dcfb958a79c6a9 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Define default CONFIG_PREBOOT with right config option | expand |
On Tue, Sep 29, 2020 at 10:48:14AM +0100, Peter Robinson wrote: > 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@gmail.com> > Cc: Jonas Smedegaard <dr@jones.dk> > Cc: Neil Armstrong <narmstrong@baylibre.com> Applied to u-boot/master, thanks!
On Tue, 29 Sep 2020 at 03:48, Peter Robinson <pbrobinson@gmail.com> wrote: > > 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@gmail.com> > Cc: Jonas Smedegaard <dr@jones.dk> > Cc: Neil Armstrong <narmstrong@baylibre.com> > --- > common/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Simon Glass <sjg@chromium.org>
Hi, > From: U-Boot <u-boot-bounces@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@gmail.com> > Cc: Jonas Smedegaard <dr@jones.dk> > Cc: Neil Armstrong <narmstrong@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. [1] = "configs: stm32mp: force empty PREBOOT" http://patchwork.ozlabs.org/project/uboot/patch/20201007081020.30635-1-patrick.delaunay@st.com/ Regards Patrick
Hi Patrick, On Wed, 7 Oct 2020 at 02:38, Patrick DELAUNAY <patrick.delaunay@st.com> wrote: > > Hi, > > > From: U-Boot <u-boot-bounces@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@gmail.com> > > Cc: Jonas Smedegaard <dr@jones.dk> > > Cc: Neil Armstrong <narmstrong@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? Regards, Simon > > [1] = "configs: stm32mp: force empty PREBOOT" > http://patchwork.ozlabs.org/project/uboot/patch/20201007081020.30635-1-patrick.delaunay@st.com/ >
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@st.com> wrote: > > > > Hi, > > > > > From: U-Boot <u-boot-bounces@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@gmail.com> > > > Cc: Jonas Smedegaard <dr@jones.dk> > > > Cc: Neil Armstrong <narmstrong@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?
Hi Simon and Tom > From: Tom Rini <trini@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@st.com> > wrote: > > > > > > Hi, > > > > > > > From: U-Boot <u-boot-bounces@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@gmail.com> > > > > Cc: Jonas Smedegaard <dr@jones.dk> > > > > Cc: Neil Armstrong <narmstrong@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
On Wed, Oct 07, 2020 at 08:37:57AM +0000, Patrick DELAUNAY wrote: > Hi, > > > From: U-Boot <u-boot-bounces@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@gmail.com> > > Cc: Jonas Smedegaard <dr@jones.dk> > > Cc: Neil Armstrong <narmstrong@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. > > [1] = "configs: stm32mp: force empty PREBOOT" > http://patchwork.ozlabs.org/project/uboot/patch/20201007081020.30635-1-patrick.delaunay@st.com/ Re-re-reading everything and this is a helpful explanation. This commit is wrong as it did more than just fix 44758771ee, which put the default in the wrong place, but added new logic that shouldn't be required. Patrick, can you please send a new patch to fix this commit and in turn NOT also default usb start on USB_STORAGE, only USB_KEYBOARD? Thanks!
Hi Tom > From: Tom Rini <trini@konsulko.com> > Sent: vendredi 9 octobre 2020 15:18 > > On Wed, Oct 07, 2020 at 08:37:57AM +0000, Patrick DELAUNAY wrote: > > Hi, > > > > > From: U-Boot <u-boot-bounces@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@gmail.com> > > > Cc: Jonas Smedegaard <dr@jones.dk> > > > Cc: Neil Armstrong <narmstrong@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. > > > > [1] = "configs: stm32mp: force empty PREBOOT" > > http://patchwork.ozlabs.org/project/uboot/patch/20201007081020.30635-1 > > -patrick.delaunay@st.com/ > > Re-re-reading everything and this is a helpful explanation. This commit is wrong > as it did more than just fix 44758771ee, which put the default in the wrong place, > but added new logic that shouldn't be required. > > Patrick, can you please send a new patch to fix this commit and in turn NOT also > default usb start on USB_STORAGE, only USB_KEYBOARD? Thanks! Done in http://patchwork.ozlabs.org/project/uboot/list/?series=207245 "Remove default value of CONFIG_PREBOOT for CONFIG_USB_STORAGE" Patrick
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.
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@gmail.com> Cc: Jonas Smedegaard <dr@jones.dk> Cc: Neil Armstrong <narmstrong@baylibre.com> --- common/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)