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