[PATCH] distro_bootcmd: Add support for loading user environment

[Soeren Moch]

On 31.01.20 19:18, Stephen Warren wrote:
> On 1/31/20 7:54 AM, Tom Rini wrote:
>> On Fri, Jan 31, 2020 at 02:42:25PM +0000, Jon Hunter wrote:
>>>
>>> On 31/01/2020 14:05, Tom Rini wrote:
>>>> On Fri, Jan 31, 2020 at 01:24:21PM +0000, Jon Hunter wrote:
>>>>>
>>>>> On 31/01/2020 12:42, Soeren Moch wrote:
>>>>>> On 31.01.20 11:59, Jon Hunter wrote:
>>>>>>> U-Boot supports loading a user environment from a file in the
>>>>>>> file-system. Therefore to make it easier for users to override the
>>>>>>> default environment, add support to the 'distro_bootcmd' to look
>>>>>>> for
>>>>>>> and load a user environment in a file called 'uEnv.txt' in the same
>>>>>>> locations where an extlinux.conf or boot script might be found.
>>>>>>
>>>>>> We already have boot script support, which can easily be used to
>>>>>> modify
>>>>>> the environment. Do we really need to bloat the distro_boot
>>>>>> machinery
>>>>>> further with environment import, that is quite limited in
>>>>>> contrast to
>>>>>> boot scripts?
>>>>>
>>>>> If you are booting with an extlinux.conf file, as we do by
>>>>> default, then
>>>>> if this file is found, this is always booted from before you have the
>>>>> opportunity to run the boot.scr script.
>>>>>
>>>>> Furthermore if you did switch the order to boot from a boot.scr
>>>>> script
>>>>> before the extlinux.conf, but you just wanted to do some simple
>>>>> modifications of the environment before booting (ie. so the boot.scr
>>>>> does not actually boot the system), then as the code is today you
>>>>> get a
>>>>> 'echo SCRIPT FAILED: continuing...' message.
>>>>>
>>>>> Yes this could be changed, but just seems cleaner and simpler to add
>>>>> support to make changes to the environment before the
>>>>> extlinux.conf is
>>>>> loaded.
>>>>>
>>>>> Yes this does add more to the environment, but it is hardly
>>>>> significant
>>>>> bloat, but if that is a concern then we could always disable this by
>>>>> default and allow users to enable it.
>>>>
>>>> This is something I think we had talked about and rejected doing
>>>> initially.  Can you please expand on the use-case here, and why it's
>>>> perhaps not better handled via PREBOOT on some platforms?  Thanks!
>>>
>>> We have one specific use-case in a downstream version of u-boot
>>> where we
>>> store some parameters in the u-boot environment for tweaking the DT
>>> firmware on boot [0]. Now there are cases where we do not wish to
>>> modify
>>> DT at all and so want to clear these variables and I would like a
>>> way to
>>> automate the re-configuration of the u-boot environment without the
>>> user
>>> having to manually set these just by the presence of a uEnv.txt file on
>>> the disk.
>>>
>>> Admittedly, this particular use-case/scenario is not directly
>>> applicable
>>> to the mainline u-boot branch as there is no support for these
>>> environment variables for Tegra (yet). However, it seemed that this
>>> change would have some value in upstream u-boot as well to allow the
>>> user to change any default environment variables without having to
>>> re-compile and hence, I decided submit this change here.
>>>
>>> For example, if we wanted to tweak any of the default environment
>>> variables that are currently in 'include/configs/tegra210-common.h' we
>>> could. Now you may say if we are not tweaking these very often, then
>>> why
>>> not recompile, and we could, but given that we have the ability to load
>>> environment parameters from a file on disk, it seems as shame that in
>>> the current boot flow there is no way to make use of this extremely
>>> useful feature.
>>
>> I really think this is a good case for CONFIG_PREBOOT to execute a
>> script (and we have other examples of that today) rather than something
>> to put in to distro boot for everyone.  Thanks!
>
> I can definitely imagine there are many reasons a user might want to
> have a way to modify U-Boot's environment easily via creating/editing
> uEnv.txt rather than having to interactively run setenv/saveenv on the
> U-Boot console.
>
> So, I think the main question is where to place uEnv.txt. If we use
> CONFIG_PREBOOT, we'd either have to:
>
> a) Hard-code a single storage device to load it from, which involved
> deciding on a platform-by-platform basis which is the best location to
> load it from.
>
> OR:
>
> b) Duplicate the existing device scanning logic from the distro boot
> scripts into CONFIG_PREBOOT. I'd rather not see the duplication.
>
> This patch achieves the auto-scanning for uEnv.txt without the
> disadvantage of (b) above. Also, it allows uEnv.txt to follow a
> removable storage device, which can have advantages. In the specific
> use-case Jon is working on, we'll want to edit the environment to
> disable some U-Boot features when using an upstream kernel, and not do
> that editing when booting a downstream kernel. On the particular
> platform Jon is using, the primary storage device is an SD card. If
> uEnv.txt can follow the SD card that'd be very useful. Of course, we
> could just hard-code the SD card as the location of uEnv.txt, but that
> wouldn't work well for systems that don't use removable media as the
> primary storage location (so we'd choose to look for uEnv.txt on e.g.
> fixed eMMC) yet the user has actually chosen to boot from SD card
> (which is earlier in boot_targets to allow booting from SD).
>
> In summary, I think doing this in the distro boot commands probably
> does make sense.
Everything what you put into the config_distro_bootcmd.h include file
you can put as well in your board specific config file after including
this include file. If several boards want to use this uEnv logic, they
can create a separate include file for that.

Please do not bloat the distro_boot machinery with something that is not
part of what distros use to configure and boot the kernel.

Thanks,
Soeren