Unable to select a different ENV location due env_get_location on zynqmp

[Michal Simek]

Hi,

On 2/15/22 14:27, Jorge Ramirez-Ortiz, Foundries wrote:
> On 15/02/22, Michal Simek wrote:
>> Hi,
>>
>> On 2/14/22 21:10, Ricardo Salveti wrote:
>>> Hi Michal,
>>>
>>> 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.
> 
> I disagree. I see no actual need to hardcode the environment location
> the way it is done. It serves no purpose.
 >
 >

>> 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.
> 
> are you saying that unless you have a DT change you wont let  the user
> choose where the place the environment?

I am not saying that. I have really not a problem to disable this code under any 
Kconfig option that user have option to disable it. But I want to have it 
enabled by default.

And DT changes are pretty much the way what I think is the right way for 
enabling multiple locations where vars can be stored.

Thanks,
Michal