Unable to select a different ENV location due env_get_location on zynqmp

[Sean Anderson]

On 2/15/22 8:54 AM, Michal Simek wrote:
> 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.

Couldn't this just be extended? E.g. exactly the way Ricardo described:

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

Here the first priority is SPI, then FAT, then EXT4. The default is sane,
but there are still backup locations.

--Sean