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

On Sun, 28 Nov 2021 18:47:11 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:

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.merciai@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.merciai@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; }

Thanks, this looks ok, just run it through checkpatch to correct the indentation of 'case' statements and put 'else if' on the same line as '}':

if () { } else if () { } ...

Marek