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

[Vasileios Amoiridis]

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.

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

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

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

Cheers,
Vasilis