Unable to select a different ENV location due env_get_location on zynqmp

Unable to select a different ENV location due env_get_location on zynqmp

[Ricardo Salveti]

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.

Thanks,
-- 
Ricardo Salveti

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

[Michal Simek]

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

Thanks,
Michal

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

[Rafał Miłecki]

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.

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

[Jorge Ramirez-Ortiz, Foundries]

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?

if so, I'd like to undestand why. As I said, it seems biased and
unreasoanble hence why I think it deserves further justification.

> 
> Thanks,
> Michal

Re: 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

Re: 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

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

[Michal Simek]

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.

Thanks,
Michal

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

[Ricardo Salveti]

On Tue, Feb 15, 2022 at 12:39 PM Michal Simek <michal.simek@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.

Cheers,
-- 
Ricardo Salveti

Re: 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@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