Re: [RFC PATCH 3/3] imx: imx8mp_evk: override env_get_location

[Tommaso Merciai]

On Fri, Nov 26, 2021 at 07:00:21PM +0100, Marek Behún wrote:
> On Fri, 26 Nov 2021 18:43:31 +0100
> Tommaso Merciai <tomm.merc...@gmail.com> wrote:
> 
> > Override env_get_location function at board level, previously dropped
> > down from soc.c
> > 
> > References:
> >  - commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
> > 
> > Signed-off-by: Tommaso Merciai <tomm.merc...@gmail.com>
> > ---
> > Changes since v1:
> >  - Remove wrong env_get_offset function from commit
> > 
> >  board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c 
> > b/board/freescale/imx8mp_evk/imx8mp_evk.c
> > index 62096c24fb..6b2eeaf152 100644
> > --- a/board/freescale/imx8mp_evk/imx8mp_evk.c
> > +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c
> > @@ -5,6 +5,7 @@
> >  
> >  #include <common.h>
> >  #include <env.h>
> > +#include <env_internal.h>
> >  #include <errno.h>
> >  #include <init.h>
> >  #include <miiphy.h>
> > @@ -17,6 +18,7 @@
> >  #include <asm/arch/clock.h>
> >  #include <asm/arch/sys_proto.h>
> >  #include <asm/mach-imx/gpio.h>
> > +#include <asm/mach-imx/boot_mode.h>
> >  
> >  DECLARE_GLOBAL_DATA_PTR;
> >  
> > @@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = {
> >     MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B  | MUX_PAD_CTRL(WDOG_PAD_CTRL),
> >  };
> >  
> > +enum env_location env_get_location(enum env_operation op, int prio)
> > +{
> > +   enum boot_device dev = get_boot_device();
> > +   enum env_location env_loc = ENVL_UNKNOWN;
> > +
> > +   if (prio)
> > +           return env_loc;
> > +
> > +   switch (dev) {
> > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > +   case QSPI_BOOT:
> > +           env_loc = ENVL_SPI_FLASH;
> > +           break;
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_NAND
> > +   case NAND_BOOT:
> > +           env_loc = ENVL_NAND;
> > +           break;
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_MMC
> > +   case SD1_BOOT:
> > +   case SD2_BOOT:
> > +   case SD3_BOOT:
> > +   case MMC1_BOOT:
> > +   case MMC2_BOOT:
> > +   case MMC3_BOOT:
> > +           env_loc =  ENVL_MMC;
> > +           break;
> > +#endif
> > +   default:
> > +#if defined(CONFIG_ENV_IS_NOWHERE)
> > +           env_loc = ENVL_NOWHERE;
> > +#endif
> 
> Using
>   if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE))
> instead of #ifdefs is preferred because the compiler then warns about
> bugs even in the disabled codepaths (that's why checkpatch complains
> about #ifdefs).
> 
> I know that this cannot be combined with switch() in a simple way,
> though.
> 
> Do you need to use switch? Can't you rewrite it to use if()s and use
> IS_ENABLED()?
> 
> Marek

Hi Marek,
Thanks for your review. What do you think about this solution?

enum env_location env_get_location(enum env_operation op, int prio)
{
        enum boot_device dev = get_boot_device();
        enum env_location env_loc = ENVL_UNKNOWN;

        if (prio)
                return env_loc;

        if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) && dev == QSPI_BOOT) {
                env_loc = ENVL_SPI_FLASH;
        }
        else if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND) && dev == NAND_BOOT) {
                env_loc = ENVL_NAND;
        }
        else if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) {
                switch (dev) {
                        case SD1_BOOT:
                        case SD2_BOOT:
                        case SD3_BOOT:
                        case MMC1_BOOT:
                        case MMC2_BOOT:
                        case MMC3_BOOT:
                                env_loc = ENVL_MMC;
                                break;
                default:
                        break;
                }
        }
        else if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) {
                env_loc = ENVL_MMC;
        }

        return env_loc;
}

Let me know.

Thanks,
tommaso