[PATCH] mach-snapdragon: move default environment to a file

[Sumit Garg]

On Fri, 21 Jun 2024 at 15:14, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
>
>
> On 21/06/2024 10:07, Sumit Garg wrote:
> > On Fri, 21 Jun 2024 at 13:19, Caleb Connolly <caleb.connolly at linaro.org> wrote:
> >>
> >>
> >>
> >> On 21/06/2024 09:35, Sumit Garg wrote:
> >>> Hi Caleb,
> >>>
> >>> On Mon, 17 Jun 2024 at 14:26, Caleb Connolly <caleb.connolly at linaro.org> wrote:
> >>>>
> >>>> Make use of CONFIG_DEFAULT_ENV_FILE and move the default qcom
> >>>> environment to a file under board/qualcomm.
> >>>>
> >>>> This is much cleaner and means we don't need to recompile on changing
> >>>> the environment.
> >>>>
> >>>> Additionally, extend the environment to include a boot menu and
> >>>> auto-boot using EFI instead of bootm. Since we now support MMC and USB
> >>>> on most platforms, these are much more useful defaults.
> >>>>
> >>>> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >>>> ---
> >>>>    board/qualcomm/default.env | 12 ++++++++++++
> >>>>    configs/qcom_defconfig     |  2 ++
> >>>>    include/configs/qcom.h     |  7 -------
> >>>>    3 files changed, 14 insertions(+), 7 deletions(-)
> >>>>    create mode 100644 board/qualcomm/default.env
> >>>>
> >>>> diff --git a/board/qualcomm/default.env b/board/qualcomm/default.env
> >>>> new file mode 100644
> >>>> index 000000000000..243aede77be7
> >>>> --- /dev/null
> >>>> +++ b/board/qualcomm/default.env
> >>>> @@ -0,0 +1,12 @@
> >>>> +stdin=serial,button-kbd
> >>>> +stdout=serial,vidconsole
> >>>> +stderr=serial,vidconsole
> >>>> +bootfile=/extlinux/extlinux.conf
> >>>
> >>> Is this file used to perform EFI boot or is it redundantly copied from
> >>> somewhere?
> >>
> >> This is here because there's a bunch of codepaths in U-Boot that
> >> implicitly expect the bootfile environment variable to have something
> >> useful in. It stops a whole class of null pointer exceptions.
> >>
> >> extlinux isn't used for EFI booting, but it's supported, and having a
> >> sensible default here is nice-to-have regardless.
> >
> > extlinux is a different boot method as compared to EFI, so it looks
> > confusing to use extlinux as default when we want EFI to be the
> > default. AFAICS, bootfile simply means the file to load and boot which
> > makes it more suitable for Linux image ("Image") to be the default
> > option.
>
> Image? Why not Image.gz, vmlinuz, or vmlinuz.efi?
>
> The value is meaningless, since all this does is stop U-Boot crashing if
> you run some commands without arguments.
>
> The only times I've ever encountered bootfile has been in pxe or
> something related, where extlinux.conf was expected. Obviously this is a
> heavily overloaded variable...
>
> Maybe I should just leave it unset to encourage folks to fix more of
> those unchecked accesses...

That was my original suggestion too, drop it and feel free to keep a
review tag with that.

-Sumit

>
> I don't think changing this to Image would be any more sensible than its
> current value.
> >
> > With that change, feel free to add
> >
> > Reviewed-by: Sumit Garg <sumit.garg at linaro.org>
> >
> > -Sumit
> >
> >>
> >> Kind regards,
> >>>
> >>> -Sumit
> >>>
> >>>> +preboot=scsi scan; usb start
> >>>> +fastboot=fastboot -l $fastboot_addr_r usb 0
> >>>> +do_boot=bootefi bootmgr
> >>>> +bootmenu_0=Boot first available device=run do_boot
> >>>> +bootmenu_1=Enable fastboot mode=run fastboot
> >>>> +bootmenu_2=Reset device=reset
> >>>> +menucmd=bootmenu
> >>>> +bootcmd=run do_boot
> >>>> diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig
> >>>> index 80ad3b32e139..a9e3797bb39a 100644
> >>>> --- a/configs/qcom_defconfig
> >>>> +++ b/configs/qcom_defconfig
> >>>> @@ -35,8 +35,10 @@ CONFIG_CMD_BMP=y
> >>>>    CONFIG_CMD_LOG=y
> >>>>    CONFIG_OF_LIVE=y
> >>>>    CONFIG_OF_BOARD_SETUP=y
> >>>>    CONFIG_BUTTON_QCOM_PMIC=y
> >>>> +CONFIG_USE_DEFAULT_ENV_FILE=y
> >>>> +CONFIG_DEFAULT_ENV_FILE="board/qualcomm/default.env"
> >>>>    CONFIG_CLK=y
> >>>>    CONFIG_CLK_QCOM_QCM2290=y
> >>>>    CONFIG_CLK_QCOM_QCS404=y
> >>>>    CONFIG_CLK_QCOM_SDM845=y
> >>>> diff --git a/include/configs/qcom.h b/include/configs/qcom.h
> >>>> index e50b3bce5cdd..5b5ebbd844df 100644
> >>>> --- a/include/configs/qcom.h
> >>>> +++ b/include/configs/qcom.h
> >>>> @@ -10,12 +10,5 @@
> >>>>    #define __CONFIGS_SNAPDRAGON_H
> >>>>
> >>>>    #define CFG_SYS_BAUDRATE_TABLE { 115200, 230400, 460800, 921600 }
> >>>>
> >>>> -/* Load addressed are calculated during board_late_init(). See arm/mach-snapdragon/board.c */
> >>>> -#define CFG_EXTRA_ENV_SETTINGS \
> >>>> -       "stdin=serial,button-kbd\0"     \
> >>>> -       "stdout=serial,vidconsole\0"    \
> >>>> -       "stderr=serial,vidconsole\0" \
> >>>> -       "bootcmd=bootm $prevbl_initrd_start_addr\0"
> >>>> -
> >>>>    #endif
> >>>> --
> >>>> 2.45.0
> >>>>
> >>
> >> --
> >> // Caleb (they/them)
>
> --
> // Caleb (they/them)