Unable to select a different ENV location due env_get_location on zynqmp

[Michal Simek]

On 2/17/22 01:43, Ricardo Salveti wrote:
> On Tue, Feb 15, 2022 at 12:39 PM Michal Simek <michal.simek at xilinx.com> wrote:
>> On 2/15/22 16:02, Sean Anderson wrote:
>>> 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:
>>
>> I am not quite sure I am following you how you want to extend it. Definitely
>> send a patch I will take a look.
>>
>>>>            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.
>>
>> IIRC backup locations have own offset or values which are saying where they are
>> stored. It means you can tune it to way you like.
>> And as above I am ok with Kconfig option not to call this function at all. Then
>> you can select options via Kconfig based on product you target.
> 
> Then a generic Kconfig entry that could be used by any board, because
> this same issue applies on other socs.
> 
> It is just more annoying here as most zynqmp targets are based out of
> the same config and machine, so the generic target ends up being way
> too opinionated from my perspective.

None is stopping you to fix it properly for all SOCs. Feel free to join that 
other threads to have generic description for this functionality. I am 
definitely happy to switch to it.

Thanks,
Michal