On Mon, 2024-05-27 at 09:34 +0200, Michal Simek wrote:
>
>
> On 5/22/24 19:47, Vasileios Amoiridis wrote:
> > From: Vasileios Amoiridis <vasileios.amoiri...@cern.ch>
> >
> > Currently, if the environment is not in the current boot media, the
> > env_get_location() is returning ENVL_UNKNOWN or ENVL_NOWHERE which
> > is not true (i.e booting from FLASH with environment in eMMC). This
> > commit adds an extra check to find the environment in the other
> > supported boot media, keeping the same priority as of now.
> >
> > Signed-off-by: Vasileios Amoiridis <vasileios.amoiri...@cern.ch>
> > ---
> > board/xilinx/versal-net/board.c | 21 +++++++++++++++++++--
> > board/xilinx/versal/board.c | 23 ++++++++++++++++++++---
> > board/xilinx/zynq/board.c | 31 +++++++++++++++++++++++++++-
> > ---
> > board/xilinx/zynqmp/zynqmp.c | 31 +++++++++++++++++++++++++++-
> > ---
> > 4 files changed, 93 insertions(+), 13 deletions(-)
> >
> > diff --git a/board/xilinx/versal-net/board.c b/board/xilinx/versal-
> > net/board.c
> > index da03024e16..5648d6685e 100644
> > --- a/board/xilinx/versal-net/board.c
> > +++ b/board/xilinx/versal-net/board.c
> > @@ -372,6 +372,21 @@ void reset_cpu(void)
> > {
> > }
> >
> > +static enum env_location env_locations[] = {
> > +#ifdef CONFIG_ENV_IS_IN_FAT
> > + ENVL_FAT,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_EXT4
> > + ENVL_EXT4,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > + ENVL_SPI_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_NOWHERE
> > + ENVL_NOWHERE,
> > +#endif
> > +};
> > +
> > #if defined(CONFIG_ENV_IS_NOWHERE)
> > enum env_location env_get_location(enum env_operation op, int
> > prio)
> > {
> > @@ -389,17 +404,19 @@ enum env_location env_get_location(enum
> > env_operation op, int prio)
> > return ENVL_FAT;
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4))
> > return ENVL_EXT4;
> > - return ENVL_NOWHERE;
> > + break;
> > case OSPI_MODE:
> > case QSPI_MODE_24BIT:
> > case QSPI_MODE_32BIT:
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH))
> > return ENVL_SPI_FLASH;
> > - return ENVL_NOWHERE;
> > + break;
> > case JTAG_MODE:
> > case SELECTMAP_MODE:
> > default:
> > return ENVL_NOWHERE;
> > }
> > +
> > + return env_locations[prio];
> > }
> > #endif
> > diff --git a/board/xilinx/versal/board.c
> > b/board/xilinx/versal/board.c
> > index 4f6d56119d..8aed2e97df 100644
> > --- a/board/xilinx/versal/board.c
> > +++ b/board/xilinx/versal/board.c
> > @@ -291,12 +291,27 @@ void reset_cpu(void)
> > {
> > }
> >
> > +static enum env_location env_locations[] = {
> > +#ifdef CONFIG_ENV_IS_IN_FAT
> > + ENVL_FAT,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_EXT4
> > + ENVL_EXT4,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > + ENVL_SPI_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_NOWHERE
> > + ENVL_NOWHERE,
> > +#endif
> > +};
> > +
> > #if defined(CONFIG_ENV_IS_NOWHERE)
> > enum env_location env_get_location(enum env_operation op, int
> > prio)
> > {
> > u32 bootmode = versal_get_bootmode();
> >
> > - if (prio)
> > + if (prio >= ARRAY_SIZE(env_locations))
> > return ENVL_UNKNOWN;
> >
> > switch (bootmode) {
> > @@ -308,17 +323,19 @@ enum env_location env_get_location(enum
> > env_operation op, int prio)
> > return ENVL_FAT;
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4))
> > return ENVL_EXT4;
> > - return ENVL_NOWHERE;
> > + break;
> > case OSPI_MODE:
> > case QSPI_MODE_24BIT:
> > case QSPI_MODE_32BIT:
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH))
> > return ENVL_SPI_FLASH;
> > - return ENVL_NOWHERE;
> > + break;
> > case JTAG_MODE:
> > case SELECTMAP_MODE:
> > default:
> > return ENVL_NOWHERE;
> > }
> > +
> > + return env_locations[prio];
> > }
> > #endif
> > diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
> > index 6c36591001..6fa5016cdd 100644
> > --- a/board/xilinx/zynq/board.c
> > +++ b/board/xilinx/zynq/board.c
> > @@ -134,11 +134,32 @@ int dram_init(void)
> > }
> > #endif
> >
> > +static enum env_location env_locations[] = {
> > +#ifdef CONFIG_ENV_IS_IN_FAT
> > + ENVL_FAT,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_EXT4
> > + ENVL_EXT4,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_NAND
> > + ENVL_NAND,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_UBI
> > + ENVL_UBI,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > + ENVL_SPI_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_NOWHERE
> > + ENVL_NOWHERE,
> > +#endif
> > +};
> > +
> > enum env_location env_get_location(enum env_operation op, int
> > prio)
> > {
> > u32 bootmode = zynq_slcr_get_boot_mode() & ZYNQ_BM_MASK;
> >
> > - if (prio)
> > + if (prio >= ARRAY_SIZE(env_locations))
> > return ENVL_UNKNOWN;
> >
> > switch (bootmode) {
> > @@ -147,22 +168,24 @@ enum env_location env_get_location(enum
> > env_operation op, int prio)
> > return ENVL_FAT;
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4))
> > return ENVL_EXT4;
> > - return ENVL_NOWHERE;
> > + break;
> > case ZYNQ_BM_NAND:
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND))
> > return ENVL_NAND;
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI))
> > return ENVL_UBI;
> > - return ENVL_NOWHERE;
> > + break;
> > case ZYNQ_BM_NOR:
> > case ZYNQ_BM_QSPI:
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH))
> > return ENVL_SPI_FLASH;
> > - return ENVL_NOWHERE;
> > + break;
> > case ZYNQ_BM_JTAG:
> > default:
> > return ENVL_NOWHERE;
> > }
> > +
> > + return env_locations[prio];
> > }
> >
> > #if defined(CONFIG_SET_DFU_ALT_INFO)
> > diff --git a/board/xilinx/zynqmp/zynqmp.c
> > b/board/xilinx/zynqmp/zynqmp.c
> > index f370fb7347..7e646d342b 100644
> > --- a/board/xilinx/zynqmp/zynqmp.c
> > +++ b/board/xilinx/zynqmp/zynqmp.c
> > @@ -588,12 +588,33 @@ int mmc_get_env_dev(void)
> > return bootseq;
> > }
> >
> > +static enum env_location env_locations[] = {
> > +#ifdef CONFIG_ENV_IS_IN_FAT
> > + ENVL_FAT,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_EXT4
> > + ENVL_EXT4,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_NAND
> > + ENVL_NAND,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_UBI
> > + ENVL_UBI,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > + ENVL_SPI_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_NOWHERE
> > + ENVL_NOWHERE,
> > +#endif
> > +};
> > +
> > #if defined(CONFIG_ENV_IS_NOWHERE)
> > enum env_location env_get_location(enum env_operation op, int
> > prio)
> > {
> > u32 bootmode = zynqmp_get_bootmode();
> >
> > - if (prio)
> > + if (prio >= ARRAY_SIZE(env_locations))
> > return ENVL_UNKNOWN;
> >
>
> if (!prio) {
> switch ...
> }
>
> please look below.
> (NOTE: Above you are not fixing all handling around prio > 0)
>
>
Ok I see your point.
> > switch (bootmode) {
> > @@ -605,22 +626,24 @@ enum env_location env_get_location(enum
> > env_operation op, int prio)
> > return ENVL_FAT;
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4))
> > return ENVL_EXT4;
> > - return ENVL_NOWHERE;
> > + break;
> > case NAND_MODE:
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND))
> > return ENVL_NAND;
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI))
> > return ENVL_UBI;
> > - return ENVL_NOWHERE;
> > + break;
> > case QSPI_MODE_24BIT:
> > case QSPI_MODE_32BIT:
> > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH))
> > return ENVL_SPI_FLASH;
> > - return ENVL_NOWHERE;
> > + break;
> > case JTAG_MODE:
> > default:
> > return ENVL_NOWHERE;
> > }
> > +
> > + return env_locations[prio];
>
> And here call
>
> return arch_env_get_location(op, prio);
>
> This can avoid duplication of these env_locations[] across all our
> SOCs.
> And pretty much fallback to standard implementation which is IMHO
> good solution.
>
> What do you think?
>
> Thanks,
> Michal
That's a much cleaner approach, I was not aware of the
arch_env_get_location(op, prio). Thank you very much for pointing
this out, I will resubmit!
Cheers,
Vasilis
