Hi Tom, On Tue, 7 Feb 2023 at 07:50, Tom Rini <[email protected]> wrote: > > On Tue, Feb 07, 2023 at 08:39:38AM +0100, Heinrich Schuchardt wrote: > > > > > > On 2/7/23 01:00, Tom Rini wrote: > > > On Tue, Feb 07, 2023 at 12:54:03AM +0100, Heinrich Schuchardt wrote: > > > > > > > > > > > > On 2/6/23 01:53, Simon Glass wrote: > > > > > This converts 3 usages of this option to the non-SPL form, since > > > > > there is > > > > > no SPL_CMD_BOOTEFI_BOOTMGR defined in Kconfig > > > > > > > > > > Signed-off-by: Simon Glass <[email protected]> > > > > > --- > > > > > > > > > > (no changes since v1) > > > > > > > > > > boot/Makefile | 2 +- > > > > > cmd/bootmenu.c | 4 ++-- > > > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/boot/Makefile b/boot/Makefile > > > > > index 69c31adb77d..73b5b19816b 100644 > > > > > --- a/boot/Makefile > > > > > +++ b/boot/Makefile > > > > > @@ -29,7 +29,7 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EFILOADER) += > > > > > bootmeth_efi.o > > > > > obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o > > > > > obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) += bootmeth_script.o > > > > > ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL > > > > > -obj-$(CONFIG_$(SPL_TPL_)CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o > > > > > +obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o > > > > > obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootflow_menu.o > > > > > endif > > > > > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c > > > > > index 3236ca5d799..422ab411252 100644 > > > > > --- a/cmd/bootmenu.c > > > > > +++ b/cmd/bootmenu.c > > > > > @@ -223,7 +223,7 @@ static int prepare_bootmenu_entry(struct > > > > > bootmenu_data *menu, > > > > > return 1; > > > > > } > > > > > -#if (CONFIG_IS_ENABLED(CMD_BOOTEFI_BOOTMGR)) && > > > > > (CONFIG_IS_ENABLED(CMD_EFICONFIG)) > > > > > +#if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && > > > > > (CONFIG_IS_ENABLED(CMD_EFICONFIG)) > > > > > > > > There is no reason whatsoever for using different macros for the two > > > > options. > > > > > > Here and elsewhere, one CONFIG is being fixed at a time. If at the end > > > of the series this is not fixed, then that's an issue to address. > > > > This cannot be reviewed easily. I never received the complete series. > > This, and the related series, are among the most reviewed we've had in > quite some time. Just FWIW. > > > CONFIG_IS_ENABLED() is more restrictive than IS_ENABLED(). No motivation is > > provided why the condition should be relaxed in the commit message. > > The idea of "restrictive" is not how either of those macros should be > evaluated. > > > Cover-letters are not in the commit history. But anyway the cover-letter > > does not provide any motivation for the change either. > > > > NAK to this patch. > > It's incorrect to use CONFIG_IS_ENABLED() instead of IS_ENABLED() > outside of: > - CONFIG_FOO, CONFIG_SPL_FOO (etc) exist > - The code in question is compiled in the SPL (etc) context and we do > need to know if the code block in question is required here and the > implicit value of SPL_FOO being false is useful. > This case is why Simon insists that adding def_bool n for > SPL_EFI_LOADER, etc, is correct, but I'm not convinced.
The issue here is that we need a way to determine whether an option like CONFIG_FOO should apply to all build phases, or only U-Boot proper. The def_bool business creates an SPL symbol, so U-Boot then knows that the option applies only to U-Boot proper, with a separate one for CONFIG_SPL_FOO There is a file call scripts/conf_nospl (in splc) which lists options that are exceptions. Otherwise we would need more of these def_bool things. The thing is, I don't really like the conf_nospl file, since it is configuring the operation of Kconfig but is not actually in the Kconfig description. So for things where I thought it was defensible, I added a def_bool. > > I will be taking most of these patches in the next day or two, but I can > avoid the EFI ones if you insist. They are however I believe correct. > > -- > Tom Regards, Simon
