[PATCH v2 0/1] xilinx: Add option to load environment from outside of

[Vasileios Amoiridis]

On Wed, 2024-06-12 at 16:47 +0200, Michal Simek wrote:
> 
> 
> On 6/12/24 15:57, Vasileios Amoiridis wrote:
> > On Wed, 2024-06-12 at 15:42 +0200, Michal Simek wrote:
> > > Hi Vasileios,
> > > 
> > > út 4. 6. 2024 v 15:21 odesílatel Vasileios Amoiridis
> > > <vassilisamir at gmail.com> napsal:
> > > > 
> > > > From: Vasileios Amoiridis <vasileios.amoiridis at cern.ch>
> > > > 
> > > > Changes in v2:
> > > >          - Remove duplication of custom hardcoded
> > > > env_locations[]
> > > > code.
> > > >          - Add implementation with general
> > > > arch_env_get_location(op,
> > > > prio)
> > > > 
> > > > v1:
> > > > https://lore.kernel.org/u-boot/20240522174738.73522-1-vassilisamir@gmail.com/
> > > > 
> > > > Vasileios Amoiridis (1):
> > > >    xilinx: Add option to load environment from outside of boot
> > > > media
> > > > 
> > > >   board/xilinx/versal-net/board.c | 47 ++++++++++++++----------
> > > > ----
> > > >   board/xilinx/versal/board.c     | 47 ++++++++++++++----------
> > > > ----
> > > >   board/xilinx/zynq/board.c       | 49 +++++++++++++++---------
> > > > -----
> > > >   board/xilinx/zynqmp/zynqmp.c    | 55 +++++++++++++++++-------
> > > > -----
> > > > ----
> > > >   4 files changed, 101 insertions(+), 97 deletions(-)
> > > > 
> > > > --
> > > > 2.34.1
> > > > 
> > > 
> > > I have to remove this patch from my queue because it is actually
> > > breaking current behavior.
> > > CI is reporting an issue with it and I did a closer look and what
> > > is
> > > happening is that if one location has no valid
> > > data it goes and looks at another location from the list.
> > > 
> > 
> > But that is the desired behavior, if the environment is not in one
> > location to check to the others.
> 
> That's how u-boot is written but if device doesn't exist or not
> accessible by 
> u-boot it is clear that it shouldn't be used and then behavior is
> correct.
> 

Hmmm, how can a device be not-accessible by u-boot but u-boot still
tries to use it? How could that happen?

Also, I think that this is a problem of the user, no? If for example
you have configure ENV_IS_IN_NAND but there is no NAND device, then
with the patch you get a visible error that U-Boot tries to access
NAND which is not there.

> If device exists and simple varibles are not yet saved there going to
> different 
> device is IMHO problematic.
> 

Maybe I am still a bit confused, but I think that if you have defined
ENV_IS_IN_QSPI and ENV_IS_IN_NAND but the environment is not there then
the behavior that we see is actually correct. U-Boot tries:

a) Get env from QSPI but fails because env is not there,
b) Get env from NAND but fails because NAND is not there,
c) Get env from nowhere.

To me, this looks like the correct behavior. Isn't it?


> > 
> > > On Zynq it behaves like this.
> > > 
> > > MMC:   mmc at e0100000: 0
> > > prio 0
> > > Loading Environment from SPIFlash... SF: Detected n25q128a13 with
> > > page
> > > size 256 Bytes, erase size 64 KiB, total 16 MiB
> > > *** Warning - bad CRC, using default environment
> > > 
> > > prio 1
> > > Loading Environment from NAND... *** Error - No Valid Environment
> > > Area found
> > > *** Warning - bad env area, using default environment
> > > 
> > > prio 2
> > > Loading Environment from SPIFlash... *** Warning - bad CRC, using
> > > default environment
> > > 
> > > prio 3
> > > Loading Environment from nowhere... OK
> > > 
> > > 
> > 
> > This is the message that we get as well when this patch is not
> > added.
> 
> It means you are booting out of QSPI but qspi is not accessible by u-
> boot. Correct?
> 
> 

Well, we are booting from QSPI but environment is in eMMC. In our
config, we have defined ENV_IS_IN_FAT and ENV_IS_NOWHERE but we have
not defined ENV_IS_IN_QSPI. 

> > > prio is my print to show where code is. Qemu boots out in QSPI
> > > boot
> > > mode and SPI is tried first and because
> > > this is xilinx_zynq_virt defconfig drivers/env locations for
> > > other
> > > devices are present too. That's why it goes over the list
> > > and it always ends in nowhere which never fails.
> > > 
> > > If this runs on real HW then the same behavior is visible and I
> > > don't
> > > think it is right.
> > > I think this solution should be rethought.
> > > In product current behavior from our code is not the best and
> > > current
> > > U-Boot implementation is not allowing
> > > flexibility too. We are enabling redundant variables which can be
> > > only
> > > on the same device but not that you have
> > > one copy in QSPI and second in EMMC.
> > > That's why I think location should be pretty much read from DT.
> > > There is options/u-boot node where location of variables should
> > > be
> > > described and this should replace
> > > env_get_location().
> > 
> > You mean that there should be some type of new U-Boot node that
> > describes where the environment should be located and go and
> > search in this device?
> 
> Not sure if node or just dt property which points where that
> variables are stored.
> 
> 

This doesn't sound too difficult to do. Still, even if this was done,
what would the priority be for looking for the environment? For now,
the code is written in a way to check for the environment only in the
bootmedia. My patch added a functionality that if the environment is
not in the bootmedia to check somewhere else. This should be the order
also if we had this u-boot node? Check first in the bootmedia and then
check if there is a dt property?

> > > It is a lot of work that's why I think second solution is more
> > > reasonable for you which is simply create new Kconfig symbol
> > > to disable board env_get_location() implementation.
> > > 
> > > Thanks,
> > > Michal
> > > 
> > 
> > You mean to basically disable the board env_get_location() that
> > exists in board/xilinx/zynqmp/zynqmp.c for example?
> 
> yes. Something like this.
> 
> config BOARD_ENV_LOCATION
>         bool "Use board defined ENV location"
>         default y
>         ...
> 
> if defined (BOARD_ENV_SETUP/LOCATION)
> enum env_location env_get_location(enum env_operation op, int prio)
> {
> ...
> }
> endif
> 
> M
> 
> 

This will be accepted by you in upstream?

Cheers,
Vasilis