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

[RFC PATCH v3 0/5] 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 imx8mn_evk,
imx8mp_evk, phycore-imx8mp and imx8mn_beacon boards instead of being completely dropped.

Tommaso Merciai (5):
  imx8m: drop env_get_location for imx8mn and imx8mp
  imx: imx8mn_evk: override env_get_location
  imx: imx8mp_evk: override env_get_location
  beacon: imx8mn: override env_get_location in imx8mn_beacon.c
  phytec: phycore_imx8mp: override env_get_location in phycore-imx8mp.c

 arch/arm/mach-imx/imx8m/soc.c                | 39 --------------------
 board/beacon/imx8mn/imx8mn_beacon.c          | 35 +++++++++++++++++-
 board/freescale/imx8mn_evk/imx8mn_evk.c      | 35 ++++++++++++++++++
 board/freescale/imx8mp_evk/imx8mp_evk.c      | 34 +++++++++++++++++
 board/phytec/phycore_imx8mp/phycore-imx8mp.c | 33 +++++++++++++++++
 5 files changed, 136 insertions(+), 40 deletions(-)

-- 
2.25.1

[RFC PATCH v3 1/5] 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>
---
 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 v3 2/5] 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 37d3e3bb95d7532e2503f115dd6c6762fd3b0262

Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
Changes since v1:
 - Fix code indentation using checkpatch as suggested by MBehún

 board/freescale/imx8mn_evk/imx8mn_evk.c | 35 +++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/board/freescale/imx8mn_evk/imx8mn_evk.c b/board/freescale/imx8mn_evk/imx8mn_evk.c
index 9a0a0488bf..ec1ab202a6 100644
--- a/board/freescale/imx8mn_evk/imx8mn_evk.c
+++ b/board/freescale/imx8mn_evk/imx8mn_evk.c
@@ -5,11 +5,46 @@
 
 #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;
+
+	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;
+}
+
 int board_init(void)
 {
 	return 0;
-- 
2.25.1

[RFC PATCH v3 3/5] 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 37d3e3bb95d7532e2503f115dd6c6762fd3b0262

Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
Changes since v1:
 - Fix code indentation using checkpatch as suggested by MBehún

 board/freescale/imx8mp_evk/imx8mp_evk.c | 34 +++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c b/board/freescale/imx8mp_evk/imx8mp_evk.c
index 62096c24fb..8548f606d2 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,38 @@ 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;
+
+	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;
+}
+
 int board_early_init_f(void)
 {
 	struct wdog_regs *wdog = (struct wdog_regs *)WDOG1_BASE_ADDR;
-- 
2.25.1

[RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Tommaso Merciai]

Override env_get_location function at board level, previously dropped
down from arch/arm/mach-imx/imx8m/soc.c

References:
 - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262

Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
 board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/board/beacon/imx8mn/imx8mn_beacon.c b/board/beacon/imx8mn/imx8mn_beacon.c
index 7fe252b262..05ab5613ee 100644
--- a/board/beacon/imx8mn/imx8mn_beacon.c
+++ b/board/beacon/imx8mn/imx8mn_beacon.c
@@ -6,14 +6,47 @@
 #include <common.h>
 #include <miiphy.h>
 #include <netdev.h>
-
+#include <env_internal.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/sys_proto.h>
+#include <asm/mach-imx/boot_mode.h>
 #include <asm/global_data.h>
 #include <asm/io.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;
+
+	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;
+}
+
 #if IS_ENABLED(CONFIG_FEC_MXC)
 static int setup_fec(void)
 {
-- 
2.25.1

[RFC PATCH v3 5/5] phytec: phycore_imx8mp: override env_get_location in phycore-imx8mp.c

[Tommaso Merciai]

Override env_get_location function at board level, previously dropped
down from arch/arm/mach-imx/imx8m/soc.c

References:
 - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262

Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
 board/phytec/phycore_imx8mp/phycore-imx8mp.c | 33 ++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/board/phytec/phycore_imx8mp/phycore-imx8mp.c b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
index a8f0821437..05926eefa3 100644
--- a/board/phytec/phycore_imx8mp/phycore-imx8mp.c
+++ b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
@@ -11,9 +11,42 @@
 #include <asm/mach-imx/boot_mode.h>
 #include <env.h>
 #include <miiphy.h>
+#include <env_internal.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;
+
+	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;
+}
+
 static int setup_fec(void)
 {
 	struct iomuxc_gpr_base_regs *gpr =
-- 
2.25.1

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Adam Ford]

On Sat, Dec 25, 2021 at 8:26 PM Tommaso Merciai <tomm.merciai@gmail.com>
wrote:

> Override env_get_location function at board level, previously dropped
> down from arch/arm/mach-imx/imx8m/soc.c
>
> References:
>  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
>
>
I am traveling, so I can't test this. However, I question why not make the
function __weak and leave it where it was, so we don't have to copy this
code from board to board.  For those who need something that differs from
this, they can write their own function which will replace the standard
version.

adam

> Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> ---
>  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/board/beacon/imx8mn/imx8mn_beacon.c
> b/board/beacon/imx8mn/imx8mn_beacon.c
> index 7fe252b262..05ab5613ee 100644
> --- a/board/beacon/imx8mn/imx8mn_beacon.c
> +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> @@ -6,14 +6,47 @@
>  #include <common.h>
>  #include <miiphy.h>
>  #include <netdev.h>
> -
> +#include <env_internal.h>
>  #include <asm/arch/clock.h>
>  #include <asm/arch/sys_proto.h>
> +#include <asm/mach-imx/boot_mode.h>
>  #include <asm/global_data.h>
>  #include <asm/io.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;
> +
> +       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;
> +}
> +
>  #if IS_ENABLED(CONFIG_FEC_MXC)
>  static int setup_fec(void)
>  {
> --
> 2.25.1
>
>

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Tommaso Merciai]

On Sun, Dec 26, 2021 at 09:20:30AM +0000, Adam Ford wrote:
> On Sat, Dec 25, 2021 at 8:26 PM Tommaso Merciai <tomm.merciai@gmail.com>
> wrote:
> 
> > Override env_get_location function at board level, previously dropped
> > down from arch/arm/mach-imx/imx8m/soc.c
> >
> > References:
> >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> >
> >
> I am traveling, so I can't test this. However, I question why not make the
> function __weak and leave it where it was, so we don't have to copy this
> code from board to board.  For those who need something that differs from
> this, they can write their own function which will replace the standard
> version.
> 
> adam

Hi Adam,
Thanks for your reply. env_get_location function is already declared (__weak) in:

 - env/env.c

Override 2 times the same function, is that correct?

I think can be a good solution too, declare it as __weak in soc.c and override
it at board level.

Some References:
 - https://developer.arm.com/documentation/dui0491/i/Compiler-specific-Features/--weak

Let me know.

Thanks,
Tommaso

> 
> > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > ---
> >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c
> > b/board/beacon/imx8mn/imx8mn_beacon.c
> > index 7fe252b262..05ab5613ee 100644
> > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > @@ -6,14 +6,47 @@
> >  #include <common.h>
> >  #include <miiphy.h>
> >  #include <netdev.h>
> > -
> > +#include <env_internal.h>
> >  #include <asm/arch/clock.h>
> >  #include <asm/arch/sys_proto.h>
> > +#include <asm/mach-imx/boot_mode.h>
> >  #include <asm/global_data.h>
> >  #include <asm/io.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;
> > +
> > +       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;
> > +}
> > +
> >  #if IS_ENABLED(CONFIG_FEC_MXC)
> >  static int setup_fec(void)
> >  {
> > --
> > 2.25.1
> >
> >

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Adam Ford]

On Sun, Dec 26, 2021 at 11:17 AM Tommaso Merciai <tomm.merciai@gmail.com>
wrote:

> On Sun, Dec 26, 2021 at 09:20:30AM +0000, Adam Ford wrote:
> > On Sat, Dec 25, 2021 at 8:26 PM Tommaso Merciai <tomm.merciai@gmail.com>
> > wrote:
> >
> > > Override env_get_location function at board level, previously dropped
> > > down from arch/arm/mach-imx/imx8m/soc.c
> > >
> > > References:
> > >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> > >
> > >
> > I am traveling, so I can't test this. However, I question why not make
> the
> > function __weak and leave it where it was, so we don't have to copy this
> > code from board to board.  For those who need something that differs from
> > this, they can write their own function which will replace the standard
> > version.
> >
> > adam
>
> Hi Adam,
> Thanks for your reply. env_get_location function is already declared
> (__weak) in:
>
>  - env/env.c
>
> Override 2 times the same function, is that correct?
>

I didn't know it was already _weak.  I am not sure it can be done in two
places.  i'm traveling, so i dont have the source code to review it.


>
> I think can be a good solution too, declare it as __weak in soc.c and
> override
> it at board level.
>
> Some References:
>  -
> https://developer.arm.com/documentation/dui0491/i/Compiler-specific-Features/--weak
>
> Let me know.
>
> Thanks,
> Tommaso
>
> >
> > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > ---
> > >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c
> > > b/board/beacon/imx8mn/imx8mn_beacon.c
> > > index 7fe252b262..05ab5613ee 100644
> > > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > > @@ -6,14 +6,47 @@
> > >  #include <common.h>
> > >  #include <miiphy.h>
> > >  #include <netdev.h>
> > > -
> > > +#include <env_internal.h>
> > >  #include <asm/arch/clock.h>
> > >  #include <asm/arch/sys_proto.h>
> > > +#include <asm/mach-imx/boot_mode.h>
> > >  #include <asm/global_data.h>
> > >  #include <asm/io.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;
> > > +
> > > +       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;
> > > +}
> > > +
> > >  #if IS_ENABLED(CONFIG_FEC_MXC)
> > >  static int setup_fec(void)
> > >  {
> > > --
> > > 2.25.1
> > >
> > >
>

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Tommaso Merciai]

On Sun, Dec 26, 2021 at 11:23:41AM +0000, Adam Ford wrote:
> On Sun, Dec 26, 2021 at 11:17 AM Tommaso Merciai <tomm.merciai@gmail.com>
> wrote:
> 
> > On Sun, Dec 26, 2021 at 09:20:30AM +0000, Adam Ford wrote:
> > > On Sat, Dec 25, 2021 at 8:26 PM Tommaso Merciai <tomm.merciai@gmail.com>
> > > wrote:
> > >
> > > > Override env_get_location function at board level, previously dropped
> > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > >
> > > > References:
> > > >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> > > >
> > > >
> > > I am traveling, so I can't test this. However, I question why not make
> > the
> > > function __weak and leave it where it was, so we don't have to copy this
> > > code from board to board.  For those who need something that differs from
> > > this, they can write their own function which will replace the standard
> > > version.
> > >
> > > adam
> >
> > Hi Adam,
> > Thanks for your reply. env_get_location function is already declared
> > (__weak) in:
> >
> >  - env/env.c
> >
> > Override 2 times the same function, is that correct?
> >
> 
> I didn't know it was already _weak.  I am not sure it can be done in two
> places.  i'm traveling, so i dont have the source code to review it.

 No problem. Take your time.

 Thanks,
 Tommaso
> 
> 
> >
> > I think can be a good solution too, declare it as __weak in soc.c and
> > override
> > it at board level.
> >
> > Some References:
> >  -
> > https://developer.arm.com/documentation/dui0491/i/Compiler-specific-Features/--weak
> >
> > Let me know.
> >
> > Thanks,
> > Tommaso
> >
> > >
> > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > > ---
> > > >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> > > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c
> > > > b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > index 7fe252b262..05ab5613ee 100644
> > > > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > > > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > @@ -6,14 +6,47 @@
> > > >  #include <common.h>
> > > >  #include <miiphy.h>
> > > >  #include <netdev.h>
> > > > -
> > > > +#include <env_internal.h>
> > > >  #include <asm/arch/clock.h>
> > > >  #include <asm/arch/sys_proto.h>
> > > > +#include <asm/mach-imx/boot_mode.h>
> > > >  #include <asm/global_data.h>
> > > >  #include <asm/io.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;
> > > > +
> > > > +       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;
> > > > +}
> > > > +
> > > >  #if IS_ENABLED(CONFIG_FEC_MXC)
> > > >  static int setup_fec(void)
> > > >  {
> > > > --
> > > > 2.25.1
> > > >
> > > >
> >

Re: [RFC PATCH v3 5/5] phytec: phycore_imx8mp: override env_get_location in phycore-imx8mp.c

[Teresa Remmet]

Hello Tommaso,

thank you for working on this.

Am Samstag, dem 25.12.2021 um 21:25 +0100 schrieb Tommaso Merciai:
> Override env_get_location function at board level, previously dropped
> down from arch/arm/mach-imx/imx8m/soc.c
> 
> References:
>  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> 
> Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> ---
>  board/phytec/phycore_imx8mp/phycore-imx8mp.c | 33
> ++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> index a8f0821437..05926eefa3 100644
> --- a/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> +++ b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> @@ -11,9 +11,42 @@
>  #include <asm/mach-imx/boot_mode.h>
>  #include <env.h>
>  #include <miiphy.h>
> +#include <env_internal.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;
> +
> +	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;

For phyCORE-i.MX8MP board code you could just remove the NAND part. As
there is no NAND flash available on the hardware.

Thanks,
Teresa


> +	} 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;
> +}
> +
>  static int setup_fec(void)
>  {
>  	struct iomuxc_gpr_base_regs *gpr =
-- 
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany

Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855

Re: [RFC PATCH v3 1/5] imx8m: drop env_get_location for imx8mn and imx8mp

[Teresa Remmet]

Hello Tommaso,

Am Samstag, dem 25.12.2021 um 21:25 +0100 schrieb 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>
> ---
>  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)

would it not make sense to move also env_get_offset() to board level?

Regards,
Teresa


>  {
-- 
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany

Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855

Re: [RFC PATCH v3 1/5] imx8m: drop env_get_location for imx8mn and imx8mp

[Michael Nazzareno Trimarchi]

Hi Teresa

On Tue, Jan 4, 2022 at 12:04 PM Teresa Remmet <T.Remmet@phytec.de> wrote:
>
> Hello Tommaso,
>
> Am Samstag, dem 25.12.2021 um 21:25 +0100 schrieb 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>
> > ---
> >  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)
>
> would it not make sense to move also env_get_offset() to board level?
>

Drop it in another patch. This is not reference in uboot

Michael

> Regards,
> Teresa
>
>
> >  {
> --
> PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
>
> Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
> Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
> 149059855



-- 
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 v3 1/5] imx8m: drop env_get_location for imx8mn and imx8mp

[Teresa Remmet]

Hello Michael,

Am Dienstag, dem 04.01.2022 um 12:06 +0100 schrieb Michael Nazzareno
Trimarchi:
> Hi Teresa
> 
> On Tue, Jan 4, 2022 at 12:04 PM Teresa Remmet <T.Remmet@phytec.de>
> wrote:
> > Hello Tommaso,
> > 
> > Am Samstag, dem 25.12.2021 um 21:25 +0100 schrieb 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>
> > > ---
> > >  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)
> > 
> > would it not make sense to move also env_get_offset() to board
> > level?
> > 
> 
> Drop it in another patch. This is not reference in uboot

Ah thanks!

Teresa

> 
> Michael
> 
> > Regards,
> > Teresa
> > 
> > 
> > >  {
> > --
> > PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz,
> > Germany
> > 
> > Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber
> > |
> > Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr.
> > 266500608, DE
> > 149059855
> 
> 
-- 
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany

Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855

Re: [RFC PATCH v3 1/5] imx8m: drop env_get_location for imx8mn and imx8mp

[Tommaso Merciai]

On Tue, Jan 04, 2022 at 11:04:10AM +0000, Teresa Remmet wrote:
> Hello Tommaso,
> 
> Am Samstag, dem 25.12.2021 um 21:25 +0100 schrieb 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>
> > ---
> >  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)
> 
> would it not make sense to move also env_get_offset() to board level?

Hi Teresa,
I think is better to put this function at board level. In this way
others boards that use i.MX8MN/i.MX8MM SOC can customize env_get_location
function. For example: maybe one user want store U-Boot env on a device
other than the boot device.

Tommaso
> 
> Regards,
> Teresa
> 
> 
> >  {
> -- 
> PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
> 
> Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
> Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
> 149059855

Re: [RFC PATCH v3 1/5] imx8m: drop env_get_location for imx8mn and imx8mp

[Teresa Remmet]

Hello Tommaso,

Am Samstag, dem 08.01.2022 um 20:08 +0100 schrieb Tommaso Merciai:
> On Tue, Jan 04, 2022 at 11:04:10AM +0000, Teresa Remmet wrote:
> > Hello Tommaso,
> > 
> > Am Samstag, dem 25.12.2021 um 21:25 +0100 schrieb 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>
> > > ---
> > >  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)
> > 
> > would it not make sense to move also env_get_offset() to board
> > level?
> 
> Hi Teresa,
> I think is better to put this function at board level. In this way
> others boards that use i.MX8MN/i.MX8MM SOC can customize
> env_get_location
> function. For example: maybe one user want store U-Boot env on a
> device
> other than the boot device.

Michael send a patch to remove the function. Which I missed. See:
https://lore.kernel.org/u-boot/20211117143456.34441-1-michael@amarulasolutions.com/

So everything is fine then.

Thanks,
Teresa

> 
> Tommaso
> > Regards,
> > Teresa
> > 
> > 
> > >  {
> > -- 
> > PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz,
> > Germany
> > 
> > Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber
> > |
> > Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr.
> > 266500608, DE
> > 149059855
-- 
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany

Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855

Re: [RFC PATCH v3 1/5] imx8m: drop env_get_location for imx8mn and imx8mp

[Tommaso Merciai]

On Tue, Jan 11, 2022 at 09:35:54AM +0000, Teresa Remmet wrote:
> Hello Tommaso,
> 
> Am Samstag, dem 08.01.2022 um 20:08 +0100 schrieb Tommaso Merciai:
> > On Tue, Jan 04, 2022 at 11:04:10AM +0000, Teresa Remmet wrote:
> > > Hello Tommaso,
> > > 
> > > Am Samstag, dem 25.12.2021 um 21:25 +0100 schrieb 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>
> > > > ---
> > > >  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)
> > > 
> > > would it not make sense to move also env_get_offset() to board
> > > level?
> > 
> > Hi Teresa,
> > I think is better to put this function at board level. In this way
> > others boards that use i.MX8MN/i.MX8MM SOC can customize
> > env_get_location
> > function. For example: maybe one user want store U-Boot env on a
> > device
> > other than the boot device.
> 
> Michael send a patch to remove the function. Which I missed. See:
> https://lore.kernel.org/u-boot/20211117143456.34441-1-michael@amarulasolutions.com/
> 
> So everything is fine then.
> 
> Thanks,
> Teresa

Hi Teresa,
Perfect.

Thanks,
Tommaso

> 
> > 
> > Tommaso
> > > Regards,
> > > Teresa
> > > 
> > > 
> > > >  {
> > > -- 
> > > PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz,
> > > Germany
> > > 
> > > Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber
> > > |
> > > Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr.
> > > 266500608, DE
> > > 149059855
> -- 
> PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
> 
> Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
> Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
> 149059855

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Adam Ford]

On Sat, Dec 25, 2021 at 2:26 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
>
> Override env_get_location function at board level, previously dropped
> down from arch/arm/mach-imx/imx8m/soc.c
>
> References:
>  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
>
> Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>

I am going to re-do the patch for the beacon board, because I talked
it over with my colleagues, and we'd like to force the environment to
one location or another and not base it on whether or not it was the
boot device.

I guess my questions is what's the status of the remaining patch
series, and how do I integrate my desired changes into it?  I can send
you what I want for my board if you're going to resubmit the series.
I wonder if a sub-function could be called where the existing
env_get_location() is renamed to something else, but declared as weak
can called from env_get_location.  Anyone who wants a their own
function can write their own so we don't have two functions with the
same name declared as weak.

What are your thoughts on that?

adam
> ---
>  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/board/beacon/imx8mn/imx8mn_beacon.c b/board/beacon/imx8mn/imx8mn_beacon.c
> index 7fe252b262..05ab5613ee 100644
> --- a/board/beacon/imx8mn/imx8mn_beacon.c
> +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> @@ -6,14 +6,47 @@
>  #include <common.h>
>  #include <miiphy.h>
>  #include <netdev.h>
> -
> +#include <env_internal.h>
>  #include <asm/arch/clock.h>
>  #include <asm/arch/sys_proto.h>
> +#include <asm/mach-imx/boot_mode.h>
>  #include <asm/global_data.h>
>  #include <asm/io.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;
> +
> +       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;
> +}
> +
>  #if IS_ENABLED(CONFIG_FEC_MXC)
>  static int setup_fec(void)
>  {
> --
> 2.25.1
>

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Tommaso Merciai]

On Wed, Jan 26, 2022 at 12:05:22PM -0600, Adam Ford wrote:
> On Sat, Dec 25, 2021 at 2:26 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> >
> > Override env_get_location function at board level, previously dropped
> > down from arch/arm/mach-imx/imx8m/soc.c
> >
> > References:
> >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> >
> > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> 

Hi Adam,

> I am going to re-do the patch for the beacon board, because I talked
> it over with my colleagues, and we'd like to force the environment to
> one location or another and not base it on whether or not it was the
> boot device.
> 
> I guess my questions is what's the status of the remaining patch
> series, and how do I integrate my desired changes into it?  I can send
> you what I want for my board if you're going to resubmit the series.

It's ok for me. Send me your changes.
Thanks.

> I wonder if a sub-function could be called where the existing
> env_get_location() is renamed to something else, but declared as weak
> can called from env_get_location.  Anyone who wants a their own
> function can write their own so we don't have two functions with the
> same name declared as weak.
> 
> What are your thoughts on that?

Right. In this way every boards that use imx8mp/imx8mn can override this
function according to their own needs.

Tommaso

> 
> adam
> > ---
> >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c b/board/beacon/imx8mn/imx8mn_beacon.c
> > index 7fe252b262..05ab5613ee 100644
> > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > @@ -6,14 +6,47 @@
> >  #include <common.h>
> >  #include <miiphy.h>
> >  #include <netdev.h>
> > -
> > +#include <env_internal.h>
> >  #include <asm/arch/clock.h>
> >  #include <asm/arch/sys_proto.h>
> > +#include <asm/mach-imx/boot_mode.h>
> >  #include <asm/global_data.h>
> >  #include <asm/io.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;
> > +
> > +       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;
> > +}
> > +
> >  #if IS_ENABLED(CONFIG_FEC_MXC)
> >  static int setup_fec(void)
> >  {
> > --
> > 2.25.1
> >

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Adam Ford]

On Wed, Jan 26, 2022 at 2:58 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
>
> On Wed, Jan 26, 2022 at 12:05:22PM -0600, Adam Ford wrote:
> > On Sat, Dec 25, 2021 at 2:26 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> > >
> > > Override env_get_location function at board level, previously dropped
> > > down from arch/arm/mach-imx/imx8m/soc.c
> > >
> > > References:
> > >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> > >
> > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> >
>
> Hi Adam,
>
> > I am going to re-do the patch for the beacon board, because I talked
> > it over with my colleagues, and we'd like to force the environment to
> > one location or another and not base it on whether or not it was the
> > boot device.
> >
> > I guess my questions is what's the status of the remaining patch
> > series, and how do I integrate my desired changes into it?  I can send
> > you what I want for my board if you're going to resubmit the series.
>
> It's ok for me. Send me your changes.
> Thanks.
>
> > I wonder if a sub-function could be called where the existing
> > env_get_location() is renamed to something else, but declared as weak
> > can called from env_get_location.  Anyone who wants a their own
> > function can write their own so we don't have two functions with the
> > same name declared as weak.
> >
> > What are your thoughts on that?
>
> Right. In this way every boards that use imx8mp/imx8mn can override this
> function according to their own needs.
>

After trying some different options, I wonder if we can just delete
env_get_location() completely from arch/arm/mach-imx/imx8m/soc.c.

For my board, it does exactly what I want when I delete that function
since the default from env.c is sufficient.  I think it's probably
safest to use your original approach to move them to individual board
files with the idea that some people may not need it at all.  When
placed into their board files, they can choose what they want for
themselves.

I would suggest sending a patch without the RFC and place the function
into the various imx8mn and imx8mp boards, but know that you can skip
the beacon board.  I would also suggest that you add a note that
people should test this to see if it's really what they want or if
they can live without the code and fall back on the default.

adam

> Tommaso
>
> >
> > adam
> > > ---
> > >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c b/board/beacon/imx8mn/imx8mn_beacon.c
> > > index 7fe252b262..05ab5613ee 100644
> > > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > > @@ -6,14 +6,47 @@
> > >  #include <common.h>
> > >  #include <miiphy.h>
> > >  #include <netdev.h>
> > > -
> > > +#include <env_internal.h>
> > >  #include <asm/arch/clock.h>
> > >  #include <asm/arch/sys_proto.h>
> > > +#include <asm/mach-imx/boot_mode.h>
> > >  #include <asm/global_data.h>
> > >  #include <asm/io.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;
> > > +
> > > +       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;
> > > +}
> > > +
> > >  #if IS_ENABLED(CONFIG_FEC_MXC)
> > >  static int setup_fec(void)
> > >  {
> > > --
> > > 2.25.1
> > >

Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Michael Nazzareno Trimarchi]

Hi

On Thu, Jan 27, 2022 at 6:00 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Wed, Jan 26, 2022 at 2:58 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 12:05:22PM -0600, Adam Ford wrote:
> > > On Sat, Dec 25, 2021 at 2:26 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> > > >
> > > > Override env_get_location function at board level, previously dropped
> > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > >
> > > > References:
> > > >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> > > >
> > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > >
> >
> > Hi Adam,
> >
> > > I am going to re-do the patch for the beacon board, because I talked
> > > it over with my colleagues, and we'd like to force the environment to
> > > one location or another and not base it on whether or not it was the
> > > boot device.
> > >
> > > I guess my questions is what's the status of the remaining patch
> > > series, and how do I integrate my desired changes into it?  I can send
> > > you what I want for my board if you're going to resubmit the series.
> >
> > It's ok for me. Send me your changes.
> > Thanks.
> >
> > > I wonder if a sub-function could be called where the existing
> > > env_get_location() is renamed to something else, but declared as weak
> > > can called from env_get_location.  Anyone who wants a their own
> > > function can write their own so we don't have two functions with the
> > > same name declared as weak.
> > >
> > > What are your thoughts on that?
> >
> > Right. In this way every boards that use imx8mp/imx8mn can override this
> > function according to their own needs.
> >
>
> After trying some different options, I wonder if we can just delete
> env_get_location() completely from arch/arm/mach-imx/imx8m/soc.c.
>

Totally agree. I sent patches to remove in the beginning. That
function must be removed

Michael

> For my board, it does exactly what I want when I delete that function
> since the default from env.c is sufficient.  I think it's probably
> safest to use your original approach to move them to individual board
> files with the idea that some people may not need it at all.  When
> placed into their board files, they can choose what they want for
> themselves.
>
> I would suggest sending a patch without the RFC and place the function
> into the various imx8mn and imx8mp boards, but know that you can skip
> the beacon board.  I would also suggest that you add a note that
> people should test this to see if it's really what they want or if
> they can live without the code and fall back on the default.
>
> adam
>
> > Tommaso
> >
> > >
> > > adam
> > > > ---
> > > >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> > > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > index 7fe252b262..05ab5613ee 100644
> > > > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > > > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > @@ -6,14 +6,47 @@
> > > >  #include <common.h>
> > > >  #include <miiphy.h>
> > > >  #include <netdev.h>
> > > > -
> > > > +#include <env_internal.h>
> > > >  #include <asm/arch/clock.h>
> > > >  #include <asm/arch/sys_proto.h>
> > > > +#include <asm/mach-imx/boot_mode.h>
> > > >  #include <asm/global_data.h>
> > > >  #include <asm/io.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;
> > > > +
> > > > +       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;
> > > > +}
> > > > +
> > > >  #if IS_ENABLED(CONFIG_FEC_MXC)
> > > >  static int setup_fec(void)
> > > >  {
> > > > --
> > > > 2.25.1
> > > >



-- 
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 v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Tommaso Merciai]

On Thu, Jan 27, 2022 at 10:59:51AM -0600, Adam Ford wrote:
> On Wed, Jan 26, 2022 at 2:58 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 12:05:22PM -0600, Adam Ford wrote:
> > > On Sat, Dec 25, 2021 at 2:26 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> > > >
> > > > Override env_get_location function at board level, previously dropped
> > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > >
> > > > References:
> > > >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> > > >
> > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > >
> >
> > Hi Adam,
> >
> > > I am going to re-do the patch for the beacon board, because I talked
> > > it over with my colleagues, and we'd like to force the environment to
> > > one location or another and not base it on whether or not it was the
> > > boot device.
> > >
> > > I guess my questions is what's the status of the remaining patch
> > > series, and how do I integrate my desired changes into it?  I can send
> > > you what I want for my board if you're going to resubmit the series.
> >
> > It's ok for me. Send me your changes.
> > Thanks.
> >
> > > I wonder if a sub-function could be called where the existing
> > > env_get_location() is renamed to something else, but declared as weak
> > > can called from env_get_location.  Anyone who wants a their own
> > > function can write their own so we don't have two functions with the
> > > same name declared as weak.
> > >
> > > What are your thoughts on that?
> >
> > Right. In this way every boards that use imx8mp/imx8mn can override this
> > function according to their own needs.
> >
> 

Hi Adam,

> After trying some different options, I wonder if we can just delete
> env_get_location() completely from arch/arm/mach-imx/imx8m/soc.c.
> 
> For my board, it does exactly what I want when I delete that function
> since the default from env.c is sufficient.  I think it's probably
> safest to use your original approach to move them to individual board
> files with the idea that some people may not need it at all.  When
> placed into their board files, they can choose what they want for
> themselves.

Ack.

> 
> I would suggest sending a patch without the RFC and place the function
> into the various imx8mn and imx8mp boards, but know that you can skip
> the beacon board.  I would also suggest that you add a note that
> people should test this to see if it's really what they want or if
> they can live without the code and fall back on the default.

Ok, I'll send v4 of the series (without RFC label) in these days.

Thanks,
Tommaso

> 
> adam
> 
> > Tommaso
> >
> > >
> > > adam
> > > > ---
> > > >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> > > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > index 7fe252b262..05ab5613ee 100644
> > > > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > > > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > @@ -6,14 +6,47 @@
> > > >  #include <common.h>
> > > >  #include <miiphy.h>
> > > >  #include <netdev.h>
> > > > -
> > > > +#include <env_internal.h>
> > > >  #include <asm/arch/clock.h>
> > > >  #include <asm/arch/sys_proto.h>
> > > > +#include <asm/mach-imx/boot_mode.h>
> > > >  #include <asm/global_data.h>
> > > >  #include <asm/io.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;
> > > > +
> > > > +       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;
> > > > +}
> > > > +
> > > >  #if IS_ENABLED(CONFIG_FEC_MXC)
> > > >  static int setup_fec(void)
> > > >  {
> > > > --
> > > > 2.25.1
> > > >