[RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level

[RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level

[Tommaso Merciai]

This series move env_get_location from soc to board level. As suggested
by Michael <michael@amarulasolutions.com> make no sense to define an 
unique way for multiple board. One board can boot from emmc and having
env on spi flash etc.. Anyways, this function is kept in both imx8mn 
and imx8mp evk boards instead of being completely dropped.
(as suggested by Andrey <andrey.zhizhikin@leica-geosystems.com>)

Tommaso Merciai (3):
  imx8m: drop env_get_location for imx8mn and imx8mp
  imx: imx8mn_evk: override env_get_location
  imx: imx8mp_evk: override env_get_location

 arch/arm/mach-imx/imx8m/soc.c           | 39 -----------------------
 board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++
 board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 39 deletions(-)

-- 
2.25.1

[RFC PATCH 1/3] imx8m: drop env_get_location for imx8mn and imx8mp

[Tommaso Merciai]

This function defined for two architecture is not really generic
and can generate problem when people add a new board.

Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
Changes since v1:
 - Fix commit: drop down only env_get_location

 arch/arm/mach-imx/imx8m/soc.c | 39 -----------------------------------
 1 file changed, 39 deletions(-)

diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index 863508776d..f0030a557a 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -1313,45 +1313,6 @@ void do_error(struct pt_regs *pt_regs, unsigned int esr)
 #endif
 
 #if defined(CONFIG_IMX8MN) || defined(CONFIG_IMX8MP)
-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
-		break;
-	}
-
-	return env_loc;
-}
-
 #ifndef ENV_IS_EMBEDDED
 long long env_get_offset(long long defautl_offset)
 {
-- 
2.25.1

[RFC PATCH 2/3] imx: imx8mn_evk: override env_get_location

[Tommaso Merciai]

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/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/board/freescale/imx8mn_evk/imx8mn_evk.c b/board/freescale/imx8mn_evk/imx8mn_evk.c
index 9a0a0488bf..ef89a91ea2 100644
--- a/board/freescale/imx8mn_evk/imx8mn_evk.c
+++ b/board/freescale/imx8mn_evk/imx8mn_evk.c
@@ -5,11 +5,53 @@
 
 #include <common.h>
 #include <env.h>
+#include <env_internal.h>
 #include <init.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/mach-imx/boot_mode.h>
 #include <asm/global_data.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
+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
+		break;
+	}
+
+	return env_loc;
+}
+
 int board_init(void)
 {
 	return 0;
-- 
2.25.1

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

[Tommaso Merciai]

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
+		break;
+	}
+
+	return env_loc;
+}
+
 int board_early_init_f(void)
 {
 	struct wdog_regs *wdog = (struct wdog_regs *)WDOG1_BASE_ADDR;
-- 
2.25.1

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

[Marek Behún]

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

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

Let me know.

Thanks,
tommaso

RE: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level

[ZHIZHIKIN Andrey]

Hello Tommaso,

> -----Original Message-----
> From: Tommaso Merciai <tomm.merciai@gmail.com>
> Sent: Friday, November 26, 2021 6:43 PM
> Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica-
> geosystems.com>; Tommaso Merciai <tomm.merciai@gmail.com>; Stefano Babic
> <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; NXP i.MX U-Boot Team
> <uboot-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Ye Li <ye.li@nxp.com>; Marek
> Vasut <marex@denx.de>; Simon Glass <sjg@chromium.org>; Frieder Schrempf
> <frieder.schrempf@kontron.de>; Marek Behún <marek.behun@nic.cz>; Ying-Chun Liu
> (PaulLiu) <paulliu@debian.org>; u-boot@lists.denx.de
> Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at
> board level
> 
> 
> This series move env_get_location from soc to board level. As suggested
> by Michael <michael@amarulasolutions.com> make no sense to define an
> unique way for multiple board. One board can boot from emmc and having
> env on spi flash etc.. Anyways, this function is kept in both imx8mn
> and imx8mp evk boards instead of being completely dropped.
> (as suggested by Andrey <andrey.zhizhikin@leica-geosystems.com>)

I believe there has been another suggestion from my side regarding this patch:
Since it look like that Michael Trimarchi submitted another part to drop
env_get_offset() in [1], combined with the first patch in this series - it is
a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and
env_get_location").

I suggest you to submit a revert instead of your first patch and deprecate the
patch from Michael, instead of having 2 separate patches for this.

> 
> Tommaso Merciai (3):
>   imx8m: drop env_get_location for imx8mn and imx8mp
>   imx: imx8mn_evk: override env_get_location
>   imx: imx8mp_evk: override env_get_location
> 
>  arch/arm/mach-imx/imx8m/soc.c           | 39 -----------------------
>  board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++
>  board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++
>  3 files changed, 83 insertions(+), 39 deletions(-)
> 
> --
> 2.25.1

Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-michael@amarulasolutions.com/

-- andrey

Re: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level

[Michael Nazzareno Trimarchi]

HI

On Mon, Nov 29, 2021 at 9:38 AM ZHIZHIKIN Andrey
<andrey.zhizhikin@leica-geosystems.com> wrote:
>
> Hello Tommaso,
>
> > -----Original Message-----
> > From: Tommaso Merciai <tomm.merciai@gmail.com>
> > Sent: Friday, November 26, 2021 6:43 PM
> > Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica-
> > geosystems.com>; Tommaso Merciai <tomm.merciai@gmail.com>; Stefano Babic
> > <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; NXP i.MX U-Boot Team
> > <uboot-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Ye Li <ye.li@nxp.com>; Marek
> > Vasut <marex@denx.de>; Simon Glass <sjg@chromium.org>; Frieder Schrempf
> > <frieder.schrempf@kontron.de>; Marek Behún <marek.behun@nic.cz>; Ying-Chun Liu
> > (PaulLiu) <paulliu@debian.org>; u-boot@lists.denx.de
> > Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at
> > board level
> >
> >
> > This series move env_get_location from soc to board level. As suggested
> > by Michael <michael@amarulasolutions.com> make no sense to define an
> > unique way for multiple board. One board can boot from emmc and having
> > env on spi flash etc.. Anyways, this function is kept in both imx8mn
> > and imx8mp evk boards instead of being completely dropped.
> > (as suggested by Andrey <andrey.zhizhikin@leica-geosystems.com>)
>
> I believe there has been another suggestion from my side regarding this patch:
> Since it look like that Michael Trimarchi submitted another part to drop
> env_get_offset() in [1], combined with the first patch in this series - it is
> a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and
> env_get_location").
>
> I suggest you to submit a revert instead of your first patch and deprecate the
> patch from Michael, instead of having 2 separate patches for this.
>

I think they are totally different problems. One is code not used and
the other moves that implementation
in specific parts.

Michael

> >
> > Tommaso Merciai (3):
> >   imx8m: drop env_get_location for imx8mn and imx8mp
> >   imx: imx8mn_evk: override env_get_location
> >   imx: imx8mp_evk: override env_get_location
> >
> >  arch/arm/mach-imx/imx8m/soc.c           | 39 -----------------------
> >  board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++
> >  board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++
> >  3 files changed, 83 insertions(+), 39 deletions(-)
> >
> > --
> > 2.25.1
>
> Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-michael@amarulasolutions.com/
>
> -- andrey



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

RE: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level

[ZHIZHIKIN Andrey]

Hello Michael,

> -----Original Message-----
> From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com>
> Sent: Monday, November 29, 2021 9:40 AM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> Cc: Tommaso Merciai <tomm.merciai@gmail.com>; Stefano Babic <sbabic@denx.de>;
> Fabio Estevam <festevam@gmail.com>; NXP i.MX U-Boot Team <uboot-imx@nxp.com>;
> Peng Fan <peng.fan@nxp.com>; Ye Li <ye.li@nxp.com>; Marek Vasut <marex@denx.de>;
> Simon Glass <sjg@chromium.org>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> Marek Behún <marek.behun@nic.cz>; Ying-Chun Liu (PaulLiu) <paulliu@debian.org>;
> u-boot@lists.denx.de
> Subject: Re: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp
> at board level
> 
> 
> HI
> 
> On Mon, Nov 29, 2021 at 9:38 AM ZHIZHIKIN Andrey
> <andrey.zhizhikin@leica-geosystems.com> wrote:
> >
> > Hello Tommaso,
> >
> > > -----Original Message-----
> > > From: Tommaso Merciai <tomm.merciai@gmail.com>
> > > Sent: Friday, November 26, 2021 6:43 PM
> > > Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica-
> > > geosystems.com>; Tommaso Merciai <tomm.merciai@gmail.com>; Stefano Babic
> > > <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; NXP i.MX U-Boot Team
> > > <uboot-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Ye Li <ye.li@nxp.com>;
> Marek
> > > Vasut <marex@denx.de>; Simon Glass <sjg@chromium.org>; Frieder Schrempf
> > > <frieder.schrempf@kontron.de>; Marek Behún <marek.behun@nic.cz>; Ying-Chun
> Liu
> > > (PaulLiu) <paulliu@debian.org>; u-boot@lists.denx.de
> > > Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp
> at
> > > board level
> > >
> > >
> > > This series move env_get_location from soc to board level. As suggested
> > > by Michael <michael@amarulasolutions.com> make no sense to define an
> > > unique way for multiple board. One board can boot from emmc and having
> > > env on spi flash etc.. Anyways, this function is kept in both imx8mn
> > > and imx8mp evk boards instead of being completely dropped.
> > > (as suggested by Andrey <andrey.zhizhikin@leica-geosystems.com>)
> >
> > I believe there has been another suggestion from my side regarding this patch:
> > Since it look like that Michael Trimarchi submitted another part to drop
> > env_get_offset() in [1], combined with the first patch in this series - it is
> > a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and
> > env_get_location").
> >
> > I suggest you to submit a revert instead of your first patch and deprecate the
> > patch from Michael, instead of having 2 separate patches for this.
> >
> 
> I think they are totally different problems. One is code not used and
> the other moves that implementation
> in specific parts.

They might be logically different, but 2 commits combined together - is a full
revert to me.

I'd leave this up to maintainer to decide, but for me it would be logical to see
a revert instead of 2 separate commits - this makes tracking more transparent.

> 
> Michael
> 
> > >
> > > Tommaso Merciai (3):
> > >   imx8m: drop env_get_location for imx8mn and imx8mp
> > >   imx: imx8mn_evk: override env_get_location
> > >   imx: imx8mp_evk: override env_get_location
> > >
> > >  arch/arm/mach-imx/imx8m/soc.c           | 39 -----------------------
> > >  board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++
> > >  board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++
> > >  3 files changed, 83 insertions(+), 39 deletions(-)
> > >
> > > --
> > > 2.25.1
> >
> > Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-michael@amarulasolutions.com/
> >
> > -- andrey
> 
> 
> 
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> https://eur02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolut
> ions.com%2F&amp;data=04%7C01%7C%7C666e97d4bb5f425c9a0408d9b313dd84%7C1b16ab3eb8f6
> 4fe39f3e2db7fe549f6a%7C0%7C0%7C637737720180120994%7CUnknown%7CTWFpbGZsb3d8eyJWIjo
> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=NJQ7
> qjpMWu%2BhGoIcqwmD%2BLc4Ekq1oHjqPSCqkiCr4DA%3D&amp;reserved=0

-- andrey

Re: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level

[Michael Nazzareno Trimarchi]

Hi

On Mon, Nov 29, 2021 at 9:46 AM ZHIZHIKIN Andrey
<andrey.zhizhikin@leica-geosystems.com> wrote:
>
> Hello Michael,
>
> > -----Original Message-----
> > From: Michael Nazzareno Trimarchi <michael@amarulasolutions.com>
> > Sent: Monday, November 29, 2021 9:40 AM
> > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > Cc: Tommaso Merciai <tomm.merciai@gmail.com>; Stefano Babic <sbabic@denx.de>;
> > Fabio Estevam <festevam@gmail.com>; NXP i.MX U-Boot Team <uboot-imx@nxp.com>;
> > Peng Fan <peng.fan@nxp.com>; Ye Li <ye.li@nxp.com>; Marek Vasut <marex@denx.de>;
> > Simon Glass <sjg@chromium.org>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> > Marek Behún <marek.behun@nic.cz>; Ying-Chun Liu (PaulLiu) <paulliu@debian.org>;
> > u-boot@lists.denx.de
> > Subject: Re: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp
> > at board level
> >
> >
> > HI
> >
> > On Mon, Nov 29, 2021 at 9:38 AM ZHIZHIKIN Andrey
> > <andrey.zhizhikin@leica-geosystems.com> wrote:
> > >
> > > Hello Tommaso,
> > >
> > > > -----Original Message-----
> > > > From: Tommaso Merciai <tomm.merciai@gmail.com>
> > > > Sent: Friday, November 26, 2021 6:43 PM
> > > > Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica-
> > > > geosystems.com>; Tommaso Merciai <tomm.merciai@gmail.com>; Stefano Babic
> > > > <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; NXP i.MX U-Boot Team
> > > > <uboot-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Ye Li <ye.li@nxp.com>;
> > Marek
> > > > Vasut <marex@denx.de>; Simon Glass <sjg@chromium.org>; Frieder Schrempf
> > > > <frieder.schrempf@kontron.de>; Marek Behún <marek.behun@nic.cz>; Ying-Chun
> > Liu
> > > > (PaulLiu) <paulliu@debian.org>; u-boot@lists.denx.de
> > > > Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp
> > at
> > > > board level
> > > >
> > > >
> > > > This series move env_get_location from soc to board level. As suggested
> > > > by Michael <michael@amarulasolutions.com> make no sense to define an
> > > > unique way for multiple board. One board can boot from emmc and having
> > > > env on spi flash etc.. Anyways, this function is kept in both imx8mn
> > > > and imx8mp evk boards instead of being completely dropped.
> > > > (as suggested by Andrey <andrey.zhizhikin@leica-geosystems.com>)
> > >
> > > I believe there has been another suggestion from my side regarding this patch:
> > > Since it look like that Michael Trimarchi submitted another part to drop
> > > env_get_offset() in [1], combined with the first patch in this series - it is
> > > a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and
> > > env_get_location").
> > >
> > > I suggest you to submit a revert instead of your first patch and deprecate the
> > > patch from Michael, instead of having 2 separate patches for this.
> > >
> >
> > I think they are totally different problems. One is code not used and
> > the other moves that implementation
> > in specific parts.
>
> They might be logically different, but 2 commits combined together - is a full
> revert to me.
>
> I'd leave this up to maintainer to decide, but for me it would be logical to see
> a revert instead of 2 separate commits - this makes tracking more transparent.
>

The first one (mine) is not a logical change. It means that nothing
get wrong. The other
is anywway some change so if you needs to revert then you can pick
specific part.

Michael

> >
> > Michael
> >
> > > >
> > > > Tommaso Merciai (3):
> > > >   imx8m: drop env_get_location for imx8mn and imx8mp
> > > >   imx: imx8mn_evk: override env_get_location
> > > >   imx: imx8mp_evk: override env_get_location
> > > >
> > > >  arch/arm/mach-imx/imx8m/soc.c           | 39 -----------------------
> > > >  board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++
> > > >  board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++
> > > >  3 files changed, 83 insertions(+), 39 deletions(-)
> > > >
> > > > --
> > > > 2.25.1
> > >
> > > Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-michael@amarulasolutions.com/
> > >
> > > -- andrey
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > https://eur02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolut
> > ions.com%2F&amp;data=04%7C01%7C%7C666e97d4bb5f425c9a0408d9b313dd84%7C1b16ab3eb8f6
> > 4fe39f3e2db7fe549f6a%7C0%7C0%7C637737720180120994%7CUnknown%7CTWFpbGZsb3d8eyJWIjo
> > iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=NJQ7
> > qjpMWu%2BhGoIcqwmD%2BLc4Ekq1oHjqPSCqkiCr4DA%3D&amp;reserved=0
>
> -- andrey



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

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

[Marek Behún]

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

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

[Tommaso Merciai]

On Mon, Nov 29, 2021 at 01:17:44PM +0100, Marek Behún wrote:
> 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

Thanks Marek for your review. Sent v2.

Let me know,
Tommaso

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

[Tommaso Merciai]

On Tue, Nov 30, 2021 at 09:23:18PM +0100, Tommaso Merciai wrote:
> On Mon, Nov 29, 2021 at 01:17:44PM +0100, Marek Behún wrote:
> > 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
> 
> Thanks Marek for your review. Sent v2.
> 
> Let me know,
> Tommaso

Hi Marek,
Have you had the time to check v2 yet?


Thanks,
tommaso