Re: Unable to select a different ENV location due env_get_location on zynqmp

["Rafał Miłecki" <zajec5@gmail.com>]

Hi,

On 15.02.2022 08:41, Michal Simek wrote:
> On 2/14/22 21:10, Ricardo Salveti wrote:
>> This is a bit similar to the issue raised on iMX8-based targets a few
>> days ago, which is forcing the environment location based on the boot
>> mode and not allowing the user to use a different option via other
>> CONFIG options.
>>
>> Should we really force the env location based on boot mode? Currently
>> there is no way to boot out of QSPI and save the environment on
>> emmc/fat/ext4, and removing CONFIG_ENV_IS_IN_SPI_FLASH causes the env
>> location to be set as ENVL_NOWHERE, which is not ideal, especially
>> when other env target locations are available at build time.
>>
>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
>> index f0be9c022a7..08afb49570a 100644
>> --- a/board/xilinx/zynqmp/zynqmp.c
>> +++ b/board/xilinx/zynqmp/zynqmp.c
>> @@ -969,6 +969,10 @@ enum env_location env_get_location(enum
>> env_operation op, int prio)
>>          case QSPI_MODE_32BIT:
>>                  if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH))
>>                          return ENVL_SPI_FLASH;
>> +               if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT))
>> +                       return ENVL_FAT;
>> +               if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4))
>> +                       return ENVL_EXT4;
>>                  return ENVL_NOWHERE;
>>          case JTAG_MODE:
>>          default:
>>
>> A change like this one would allow other locations to be set, and just
>> use the boot mode to assign the priority instead.
>>
>> Since I couldn't really find out what is the expected behavior on
>> functions defined at soc/board level (env.c sets based on priority,
>> depending on what is enabled at build time), I was wondering if we
>> shouldn't just drop this function entirely.
>>
>> While I agree the board should have a set of defaults, not allowing
>> the user to change something like env location via CONFIGs seems wrong
>> to me.
>>
>> Let me know what you think.
> 
> Default location is setup based on how Xilinx sees where that variables should be saved for the most cases that's why that function was done like this.
> That's why I think that it is very reasonable default setup.
> And as is hopefully known we are using pretty generic u-boot which should work on all configurations it means default defconfig have all options enabled.
> 
> Does it make sense to have an option to change it to any configuration?
> Definitely.
> 
> How to do it?
> DT is for us source of truth that's why I prefer to have DT description which is providing all information. It means say where variables should be stored, where redundant variables should be stored. IIRC as of today I think they have to be on the same device which can be also more flexible. You can specify different start/end in qspis, etc.
> 
> What has to happen?
> Someone has to take a lead and come up with generic universal DT binding to be able describe it. Some days ago linaro had similar issue with DT in connection to A/B update via GPT partition. And description for it should be pretty much the same as is for variables.

I'm not very familiar with U-Boot & iMX but I actually started working
on DT binding for U-Boot env.

Please check:
[PATCH V2 2/3] dt-bindings: nvmem: add U-Boot environment variables binding
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211230090449.11808-2-zajec5@gmail.com/

I received feedback from Rob on January and I'm working on next version
of my patch this very moment. I'll cc U-Boot this time obviously.