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

[Caleb Connolly]

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...

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)