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 > <[email protected]> napsal: > > > > From: Vasileios Amoiridis <[email protected]> > > > > 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/[email protected]/ > > > > 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@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
