* [PATCH v4 0/4] imx8m: move env_get_location for imx8mn and imx8mp at board level
@ 2022-01-31 16:58 Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp Tommaso Merciai
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Tommaso Merciai @ 2022-01-31 16:58 UTC (permalink / raw)
Cc: michael, aford173, tommaso.merciai, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Peng Fan, Ye Li, Simon Glass, Marek Vasut,
Frieder Schrempf, Ying-Chun Liu (PaulLiu),
u-boot
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.
Note for all imx8mp/imx8mn users:
This patch move only the env_get_location function at board level in this way
every users can override it depending on own needs. Test it.
Tommaso Merciai (4):
imx8m: drop env_get_location for imx8mn and imx8mp
imx: imx8mn_evk: override env_get_location
imx: imx8mp_evk: override env_get_location
phytec: phycore_imx8mp: override env_get_location
arch/arm/mach-imx/imx8m/soc.c | 39 --------------------
board/freescale/imx8mn_evk/imx8mn_evk.c | 35 ++++++++++++++++++
board/freescale/imx8mp_evk/imx8mp_evk.c | 34 +++++++++++++++++
board/phytec/phycore_imx8mp/phycore-imx8mp.c | 33 +++++++++++++++++
4 files changed, 102 insertions(+), 39 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp
2022-01-31 16:58 [PATCH v4 0/4] imx8m: move env_get_location for imx8mn and imx8mp at board level Tommaso Merciai
@ 2022-01-31 16:58 ` Tommaso Merciai
2022-01-31 17:03 ` Marek Vasut
2022-01-31 17:06 ` Adam Ford
2022-01-31 16:58 ` [PATCH v4 2/4] imx: imx8mn_evk: override env_get_location Tommaso Merciai
` (2 subsequent siblings)
3 siblings, 2 replies; 26+ messages in thread
From: Tommaso Merciai @ 2022-01-31 16:58 UTC (permalink / raw)
Cc: michael, aford173, tommaso.merciai, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Peng Fan, Ye Li, Marek Vasut, Simon Glass,
Frieder Schrempf, Marek Behún, Harald Seiler, u-boot
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 <tommaso.merciai@amarulasolutions.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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 2/4] imx: imx8mn_evk: override env_get_location
2022-01-31 16:58 [PATCH v4 0/4] imx8m: move env_get_location for imx8mn and imx8mp at board level Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp Tommaso Merciai
@ 2022-01-31 16:58 ` Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 3/4] imx: imx8mp_evk: " Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 4/4] phytec: phycore_imx8mp: " Tommaso Merciai
3 siblings, 0 replies; 26+ messages in thread
From: Tommaso Merciai @ 2022-01-31 16:58 UTC (permalink / raw)
Cc: michael, aford173, tommaso.merciai, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Peng Fan, Ye Li, Marek Vasut, Simon Glass,
Frieder Schrempf, Harald Seiler, Ying-Chun Liu (PaulLiu),
u-boot
Override env_get_location function at board level, previously dropped
down from soc.c
References:
- commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 3/4] imx: imx8mp_evk: override env_get_location
2022-01-31 16:58 [PATCH v4 0/4] imx8m: move env_get_location for imx8mn and imx8mp at board level Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 2/4] imx: imx8mn_evk: override env_get_location Tommaso Merciai
@ 2022-01-31 16:58 ` Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 4/4] phytec: phycore_imx8mp: " Tommaso Merciai
3 siblings, 0 replies; 26+ messages in thread
From: Tommaso Merciai @ 2022-01-31 16:58 UTC (permalink / raw)
Cc: michael, aford173, tommaso.merciai, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Peng Fan, Ye Li, Simon Glass, Marek Vasut,
Frieder Schrempf, Ying-Chun Liu (PaulLiu),
u-boot
Override env_get_location function at board level, previously dropped
down from soc.c
References:
- commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-01-31 16:58 [PATCH v4 0/4] imx8m: move env_get_location for imx8mn and imx8mp at board level Tommaso Merciai
` (2 preceding siblings ...)
2022-01-31 16:58 ` [PATCH v4 3/4] imx: imx8mp_evk: " Tommaso Merciai
@ 2022-01-31 16:58 ` Tommaso Merciai
2022-01-31 17:03 ` Marek Vasut
3 siblings, 1 reply; 26+ messages in thread
From: Tommaso Merciai @ 2022-01-31 16:58 UTC (permalink / raw)
Cc: michael, aford173, tommaso.merciai, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Marek Vasut, Frieder Schrempf, Harald Seiler,
Marek Behún, u-boot
Override env_get_location function at board level, previously dropped
down from arch/arm/mach-imx/imx8m/soc.c
References:
- commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp
2022-01-31 16:58 ` [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp Tommaso Merciai
@ 2022-01-31 17:03 ` Marek Vasut
2022-01-31 17:06 ` Adam Ford
1 sibling, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2022-01-31 17:03 UTC (permalink / raw)
To: Tommaso Merciai
Cc: michael, aford173, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Peng Fan, Ye Li, Simon Glass,
Frieder Schrempf, Marek Behún, Harald Seiler, u-boot
On 1/31/22 17:58, Tommaso Merciai wrote:
> 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 <tommaso.merciai@amarulasolutions.com>
Does git bisect still work with only this patch applied ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-01-31 16:58 ` [PATCH v4 4/4] phytec: phycore_imx8mp: " Tommaso Merciai
@ 2022-01-31 17:03 ` Marek Vasut
2022-01-31 22:15 ` Tommaso Merciai
0 siblings, 1 reply; 26+ messages in thread
From: Marek Vasut @ 2022-01-31 17:03 UTC (permalink / raw)
To: Tommaso Merciai
Cc: michael, aford173, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
u-boot
On 1/31/22 17:58, Tommaso Merciai wrote:
> Override env_get_location function at board level, previously dropped
> down from arch/arm/mach-imx/imx8m/soc.c
>
> References:
> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> +{
Why don't you just turn this into default __weak function and override
it on board level when it is really needed to be overridden ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp
2022-01-31 16:58 ` [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp Tommaso Merciai
2022-01-31 17:03 ` Marek Vasut
@ 2022-01-31 17:06 ` Adam Ford
1 sibling, 0 replies; 26+ messages in thread
From: Adam Ford @ 2022-01-31 17:06 UTC (permalink / raw)
To: Tommaso Merciai
Cc: Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Peng Fan, Ye Li, Marek Vasut, Simon Glass,
Frieder Schrempf, Marek Behún, Harald Seiler,
U-Boot Mailing List
On Mon, Jan 31, 2022 at 10:59 AM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> This function defined for two architecture is not really generic
> and can generate problem when people add a new board.
>
This actually fixes some issues I was trying to solve on the
imx8mn-beacon board, so thank you for doing this.
Tested-by: Adam Ford <aford173@gmail.com> #imx8mn-beacon
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-01-31 17:03 ` Marek Vasut
@ 2022-01-31 22:15 ` Tommaso Merciai
2022-02-01 0:20 ` Adam Ford
2022-02-01 3:16 ` Marek Vasut
0 siblings, 2 replies; 26+ messages in thread
From: Tommaso Merciai @ 2022-01-31 22:15 UTC (permalink / raw)
To: Marek Vasut
Cc: michael, aford173, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
u-boot
On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> On 1/31/22 17:58, Tommaso Merciai wrote:
> > Override env_get_location function at board level, previously dropped
> > down from arch/arm/mach-imx/imx8m/soc.c
> >
> > References:
> > - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> >
> > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> > +{
>
> Why don't you just turn this into default __weak function and override it on
> board level when it is really needed to be overridden ?
Hi Marek,
env_get_location is already declared as __weak, check env/env.c. We
can't override it 2 times.
References:
- https://developer.arm.com/documentation/dui0491/i/Compiler-specific-Features/--weak
--
Tommaso Merciai
Embedded Linux Developer
tommaso.merciai@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-01-31 22:15 ` Tommaso Merciai
@ 2022-02-01 0:20 ` Adam Ford
2022-02-01 3:18 ` Marek Vasut
2022-02-01 3:16 ` Marek Vasut
1 sibling, 1 reply; 26+ messages in thread
From: Adam Ford @ 2022-02-01 0:20 UTC (permalink / raw)
To: Tommaso Merciai
Cc: Marek Vasut, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On Mon, Jan 31, 2022 at 4:16 PM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> > On 1/31/22 17:58, Tommaso Merciai wrote:
> > > Override env_get_location function at board level, previously dropped
> > > down from arch/arm/mach-imx/imx8m/soc.c
> > >
> > > References:
> > > - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> > >
> > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> > > +{
> >
> > Why don't you just turn this into default __weak function and override it on
> > board level when it is really needed to be overridden ?
>
> Hi Marek,
> env_get_location is already declared as __weak, check env/env.c. We
> can't override it 2 times.
The original version (before it was added in 2707faf01f04
("imx8mn/imx8mp: override env_get_offset and env_get_location") is
located in env/env.c and for my board, this is the preferred method.
This replacement method actually is the opposite behavior from what I
want, which is to force the environment to a fixed location regardless
of the boot device.
I think Tommaso's method is better, because as it is, the users cannot
override it any more.
adam
>
> References:
> - https://developer.arm.com/documentation/dui0491/i/Compiler-specific-Features/--weak
>
> --
> Tommaso Merciai
> Embedded Linux Developer
> tommaso.merciai@amarulasolutions.com
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> info@amarulasolutions.com
> www.amarulasolutions.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-01-31 22:15 ` Tommaso Merciai
2022-02-01 0:20 ` Adam Ford
@ 2022-02-01 3:16 ` Marek Vasut
2022-02-01 9:09 ` Tommaso Merciai
1 sibling, 1 reply; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 3:16 UTC (permalink / raw)
To: Tommaso Merciai
Cc: michael, aford173, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
u-boot
On 1/31/22 23:15, Tommaso Merciai wrote:
> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>> Override env_get_location function at board level, previously dropped
>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>
>>> References:
>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>
>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>> +{
>>
>> Why don't you just turn this into default __weak function and override it on
>> board level when it is really needed to be overridden ?
>
> Hi Marek,
> env_get_location is already declared as __weak, check env/env.c. We
> can't override it 2 times.
Oh, it is this problem with missing ability to define multiple levels of
symbol strength.
__weak enum env_location arch_env_get_location(enum env_operation op,
int prio)
{
if (prio >= ARRAY_SIZE(env_locations))
return ENVL_UNKNOWN;
return env_locations[prio];
}
__weak enum env_location board_env_get_location(enum env_operation op,
int prio)
{
return arch_env_get_location(op, prio);
}
__weak enum env_location env_get_location(enum env_operation op, int prio)
{
return board_env_get_location(op, prio);
}
By default, the compiler will optimize it all out. If you have
arch-specific default (like imx does), implement
arch_env_get_location(), if you have even board-specific default (like
your board likely does), implement board_env_get_location(), if you need
to override both, then override env_get_location() (unlikely).
This is also inline with all the other arch_*() and board_*() functions
we have, and you won't have much duplication either.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 0:20 ` Adam Ford
@ 2022-02-01 3:18 ` Marek Vasut
2022-02-01 11:16 ` Adam Ford
0 siblings, 1 reply; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 3:18 UTC (permalink / raw)
To: Adam Ford, Tommaso Merciai
Cc: Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On 2/1/22 01:20, Adam Ford wrote:
> On Mon, Jan 31, 2022 at 4:16 PM Tommaso Merciai
> <tommaso.merciai@amarulasolutions.com> wrote:
>>
>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>> Override env_get_location function at board level, previously dropped
>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>
>>>> References:
>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>
>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>> +{
>>>
>>> Why don't you just turn this into default __weak function and override it on
>>> board level when it is really needed to be overridden ?
>>
>> Hi Marek,
>> env_get_location is already declared as __weak, check env/env.c. We
>> can't override it 2 times.
>
> The original version (before it was added in 2707faf01f04
> ("imx8mn/imx8mp: override env_get_offset and env_get_location") is
> located in env/env.c and for my board, this is the preferred method.
> This replacement method actually is the opposite behavior from what I
> want, which is to force the environment to a fixed location regardless
> of the boot device.
>
> I think Tommaso's method is better, because as it is, the users cannot
> override it any more.
Doesn't 1/4 patch break env on pretty much every single imx8m board ?
See my other reply regarding arch and board level __weak symbols, that's
likely the way to go here.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 3:16 ` Marek Vasut
@ 2022-02-01 9:09 ` Tommaso Merciai
2022-02-01 11:22 ` Adam Ford
2022-02-01 11:22 ` Marek Vasut
0 siblings, 2 replies; 26+ messages in thread
From: Tommaso Merciai @ 2022-02-01 9:09 UTC (permalink / raw)
To: Marek Vasut
Cc: michael, aford173, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
u-boot
On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
> On 1/31/22 23:15, Tommaso Merciai wrote:
> > On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> > > On 1/31/22 17:58, Tommaso Merciai wrote:
> > > > Override env_get_location function at board level, previously dropped
> > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > >
> > > > References:
> > > > - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> > > >
> > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> > > > +{
> > >
> > > Why don't you just turn this into default __weak function and override it on
> > > board level when it is really needed to be overridden ?
> >
> > Hi Marek,
> > env_get_location is already declared as __weak, check env/env.c. We
> > can't override it 2 times.
>
> Oh, it is this problem with missing ability to define multiple levels of
> symbol strength.
>
> __weak enum env_location arch_env_get_location(enum env_operation op, int
> prio)
> {
> if (prio >= ARRAY_SIZE(env_locations))
> return ENVL_UNKNOWN;
>
> return env_locations[prio];
> }
>
> __weak enum env_location board_env_get_location(enum env_operation op, int
> prio)
> {
> return arch_env_get_location(op, prio);
> }
>
> __weak enum env_location env_get_location(enum env_operation op, int prio)
> {
> return board_env_get_location(op, prio);
> }
>
> By default, the compiler will optimize it all out. If you have arch-specific
> default (like imx does), implement arch_env_get_location(), if you have even
> board-specific default (like your board likely does), implement
> board_env_get_location(), if you need to override both, then override
> env_get_location() (unlikely).
>
> This is also inline with all the other arch_*() and board_*() functions we
> have, and you won't have much duplication either.
Hi Marek,
Thanks for the tips, then if I understand correctly, your idea is: use:
arch_env_get_location in (soc.c)
In this way imx8m users can override this function at board level using:
board_env_get_location
right?
Thanks,
Tommaso
--
Tommaso Merciai
Embedded Linux Developer
tommaso.merciai@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 3:18 ` Marek Vasut
@ 2022-02-01 11:16 ` Adam Ford
2022-02-01 11:21 ` Marek Vasut
0 siblings, 1 reply; 26+ messages in thread
From: Adam Ford @ 2022-02-01 11:16 UTC (permalink / raw)
To: Marek Vasut
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On Mon, Jan 31, 2022 at 9:18 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/1/22 01:20, Adam Ford wrote:
> > On Mon, Jan 31, 2022 at 4:16 PM Tommaso Merciai
> > <tommaso.merciai@amarulasolutions.com> wrote:
> >>
> >> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> >>> On 1/31/22 17:58, Tommaso Merciai wrote:
> >>>> Override env_get_location function at board level, previously dropped
> >>>> down from arch/arm/mach-imx/imx8m/soc.c
> >>>>
> >>>> References:
> >>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> >>>>
> >>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> >>>> +{
> >>>
> >>> Why don't you just turn this into default __weak function and override it on
> >>> board level when it is really needed to be overridden ?
> >>
> >> Hi Marek,
> >> env_get_location is already declared as __weak, check env/env.c. We
> >> can't override it 2 times.
> >
> > The original version (before it was added in 2707faf01f04
> > ("imx8mn/imx8mp: override env_get_offset and env_get_location") is
> > located in env/env.c and for my board, this is the preferred method.
> > This replacement method actually is the opposite behavior from what I
> > want, which is to force the environment to a fixed location regardless
> > of the boot device.
> >
> > I think Tommaso's method is better, because as it is, the users cannot
> > override it any more.
>
> Doesn't 1/4 patch break env on pretty much every single imx8m board ?
For me, patch 1/4 fixed the issue of not being able to define a fixed
environment location. It now sits where I put it. It also allows me
to boot over USB without having to define ENV_IS_NOWHERE.
>
> See my other reply regarding arch and board level __weak symbols, that's
> likely the way to go here.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:16 ` Adam Ford
@ 2022-02-01 11:21 ` Marek Vasut
2022-02-01 11:23 ` Adam Ford
0 siblings, 1 reply; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 11:21 UTC (permalink / raw)
To: Adam Ford
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On 2/1/22 12:16, Adam Ford wrote:
> On Mon, Jan 31, 2022 at 9:18 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/1/22 01:20, Adam Ford wrote:
>>> On Mon, Jan 31, 2022 at 4:16 PM Tommaso Merciai
>>> <tommaso.merciai@amarulasolutions.com> wrote:
>>>>
>>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>>>> Override env_get_location function at board level, previously dropped
>>>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>>>
>>>>>> References:
>>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>>>
>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>>>> +{
>>>>>
>>>>> Why don't you just turn this into default __weak function and override it on
>>>>> board level when it is really needed to be overridden ?
>>>>
>>>> Hi Marek,
>>>> env_get_location is already declared as __weak, check env/env.c. We
>>>> can't override it 2 times.
>>>
>>> The original version (before it was added in 2707faf01f04
>>> ("imx8mn/imx8mp: override env_get_offset and env_get_location") is
>>> located in env/env.c and for my board, this is the preferred method.
>>> This replacement method actually is the opposite behavior from what I
>>> want, which is to force the environment to a fixed location regardless
>>> of the boot device.
>>>
>>> I think Tommaso's method is better, because as it is, the users cannot
>>> override it any more.
>>
>> Doesn't 1/4 patch break env on pretty much every single imx8m board ?
>
> For me, patch 1/4 fixed the issue of not being able to define a fixed
> environment location. It now sits where I put it. It also allows me
> to boot over USB without having to define ENV_IS_NOWHERE.
Sure, it also likely breaks every other MX8M board which does not define
that env_get_location() on board level, i.e. every MX8M board.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 9:09 ` Tommaso Merciai
@ 2022-02-01 11:22 ` Adam Ford
2022-02-01 11:45 ` Marek Vasut
2022-02-01 11:56 ` Tommaso Merciai
2022-02-01 11:22 ` Marek Vasut
1 sibling, 2 replies; 26+ messages in thread
From: Adam Ford @ 2022-02-01 11:22 UTC (permalink / raw)
To: Tommaso Merciai
Cc: Marek Vasut, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
> > On 1/31/22 23:15, Tommaso Merciai wrote:
> > > On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> > > > On 1/31/22 17:58, Tommaso Merciai wrote:
> > > > > Override env_get_location function at board level, previously dropped
> > > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > > >
> > > > > References:
> > > > > - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> > > > >
> > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> > > > > +{
> > > >
> > > > Why don't you just turn this into default __weak function and override it on
> > > > board level when it is really needed to be overridden ?
> > >
> > > Hi Marek,
> > > env_get_location is already declared as __weak, check env/env.c. We
> > > can't override it 2 times.
> >
> > Oh, it is this problem with missing ability to define multiple levels of
> > symbol strength.
> >
> > __weak enum env_location arch_env_get_location(enum env_operation op, int
> > prio)
> > {
> > if (prio >= ARRAY_SIZE(env_locations))
> > return ENVL_UNKNOWN;
> >
> > return env_locations[prio];
> > }
> >
> > __weak enum env_location board_env_get_location(enum env_operation op, int
> > prio)
> > {
> > return arch_env_get_location(op, prio);
> > }
> >
> > __weak enum env_location env_get_location(enum env_operation op, int prio)
> > {
> > return board_env_get_location(op, prio);
> > }
> >
> > By default, the compiler will optimize it all out. If you have arch-specific
> > default (like imx does), implement arch_env_get_location(), if you have even
> > board-specific default (like your board likely does), implement
> > board_env_get_location(), if you need to override both, then override
> > env_get_location() (unlikely).
> >
> > This is also inline with all the other arch_*() and board_*() functions we
> > have, and you won't have much duplication either.
>
> Hi Marek,
> Thanks for the tips, then if I understand correctly, your idea is: use:
>
> arch_env_get_location in (soc.c)
>
> In this way imx8m users can override this function at board level using:
>
> board_env_get_location
>
> right?
What about those of us who want to use the default option found in
env.c? It seems like we're creating more abstraction to address the
abstraction we don't all want. From my interpretation, the whole
point of creating the default in env.c was to let people define the
location of their environment, and this function in soc.c undid that.
If people want it for their boards, put this function in their boards,
otherwise, just use the default, or write your own.
adam
>
> Thanks,
> Tommaso
>
> --
> Tommaso Merciai
> Embedded Linux Developer
> tommaso.merciai@amarulasolutions.com
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> info@amarulasolutions.com
> www.amarulasolutions.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 9:09 ` Tommaso Merciai
2022-02-01 11:22 ` Adam Ford
@ 2022-02-01 11:22 ` Marek Vasut
1 sibling, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 11:22 UTC (permalink / raw)
To: Tommaso Merciai
Cc: michael, aford173, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
u-boot
On 2/1/22 10:09, Tommaso Merciai wrote:
> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
>> On 1/31/22 23:15, Tommaso Merciai wrote:
>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>>> Override env_get_location function at board level, previously dropped
>>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>>
>>>>> References:
>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>>
>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>>> +{
>>>>
>>>> Why don't you just turn this into default __weak function and override it on
>>>> board level when it is really needed to be overridden ?
>>>
>>> Hi Marek,
>>> env_get_location is already declared as __weak, check env/env.c. We
>>> can't override it 2 times.
>>
>> Oh, it is this problem with missing ability to define multiple levels of
>> symbol strength.
>>
>> __weak enum env_location arch_env_get_location(enum env_operation op, int
>> prio)
>> {
>> if (prio >= ARRAY_SIZE(env_locations))
>> return ENVL_UNKNOWN;
>>
>> return env_locations[prio];
>> }
>>
>> __weak enum env_location board_env_get_location(enum env_operation op, int
>> prio)
>> {
>> return arch_env_get_location(op, prio);
>> }
>>
>> __weak enum env_location env_get_location(enum env_operation op, int prio)
>> {
>> return board_env_get_location(op, prio);
>> }
>>
>> By default, the compiler will optimize it all out. If you have arch-specific
>> default (like imx does), implement arch_env_get_location(), if you have even
>> board-specific default (like your board likely does), implement
>> board_env_get_location(), if you need to override both, then override
>> env_get_location() (unlikely).
>>
>> This is also inline with all the other arch_*() and board_*() functions we
>> have, and you won't have much duplication either.
>
> Hi Marek,
> Thanks for the tips, then if I understand correctly, your idea is: use:
>
> arch_env_get_location in (soc.c)
>
> In this way imx8m users can override this function at board level using:
>
> board_env_get_location
>
> right?
Yes, I think that is the most flexible way with least duplication.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:21 ` Marek Vasut
@ 2022-02-01 11:23 ` Adam Ford
2022-02-01 11:43 ` Marek Vasut
0 siblings, 1 reply; 26+ messages in thread
From: Adam Ford @ 2022-02-01 11:23 UTC (permalink / raw)
To: Marek Vasut
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On Tue, Feb 1, 2022 at 5:22 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/1/22 12:16, Adam Ford wrote:
> > On Mon, Jan 31, 2022 at 9:18 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/1/22 01:20, Adam Ford wrote:
> >>> On Mon, Jan 31, 2022 at 4:16 PM Tommaso Merciai
> >>> <tommaso.merciai@amarulasolutions.com> wrote:
> >>>>
> >>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> >>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
> >>>>>> Override env_get_location function at board level, previously dropped
> >>>>>> down from arch/arm/mach-imx/imx8m/soc.c
> >>>>>>
> >>>>>> References:
> >>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> >>>>>>
> >>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> >>>>>> +{
> >>>>>
> >>>>> Why don't you just turn this into default __weak function and override it on
> >>>>> board level when it is really needed to be overridden ?
> >>>>
> >>>> Hi Marek,
> >>>> env_get_location is already declared as __weak, check env/env.c. We
> >>>> can't override it 2 times.
> >>>
> >>> The original version (before it was added in 2707faf01f04
> >>> ("imx8mn/imx8mp: override env_get_offset and env_get_location") is
> >>> located in env/env.c and for my board, this is the preferred method.
> >>> This replacement method actually is the opposite behavior from what I
> >>> want, which is to force the environment to a fixed location regardless
> >>> of the boot device.
> >>>
> >>> I think Tommaso's method is better, because as it is, the users cannot
> >>> override it any more.
> >>
> >> Doesn't 1/4 patch break env on pretty much every single imx8m board ?
> >
> > For me, patch 1/4 fixed the issue of not being able to define a fixed
> > environment location. It now sits where I put it. It also allows me
> > to boot over USB without having to define ENV_IS_NOWHERE.
>
> Sure, it also likely breaks every other MX8M board which does not define
> that env_get_location() on board level, i.e. every MX8M board.
env_get_location is defined in env/env.c
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:23 ` Adam Ford
@ 2022-02-01 11:43 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 11:43 UTC (permalink / raw)
To: Adam Ford
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On 2/1/22 12:23, Adam Ford wrote:
> On Tue, Feb 1, 2022 at 5:22 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/1/22 12:16, Adam Ford wrote:
>>> On Mon, Jan 31, 2022 at 9:18 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 2/1/22 01:20, Adam Ford wrote:
>>>>> On Mon, Jan 31, 2022 at 4:16 PM Tommaso Merciai
>>>>> <tommaso.merciai@amarulasolutions.com> wrote:
>>>>>>
>>>>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>>>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>>>>>> Override env_get_location function at board level, previously dropped
>>>>>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>>>>>
>>>>>>>> References:
>>>>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>>>>>
>>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>>>>>> +{
>>>>>>>
>>>>>>> Why don't you just turn this into default __weak function and override it on
>>>>>>> board level when it is really needed to be overridden ?
>>>>>>
>>>>>> Hi Marek,
>>>>>> env_get_location is already declared as __weak, check env/env.c. We
>>>>>> can't override it 2 times.
>>>>>
>>>>> The original version (before it was added in 2707faf01f04
>>>>> ("imx8mn/imx8mp: override env_get_offset and env_get_location") is
>>>>> located in env/env.c and for my board, this is the preferred method.
>>>>> This replacement method actually is the opposite behavior from what I
>>>>> want, which is to force the environment to a fixed location regardless
>>>>> of the boot device.
>>>>>
>>>>> I think Tommaso's method is better, because as it is, the users cannot
>>>>> override it any more.
>>>>
>>>> Doesn't 1/4 patch break env on pretty much every single imx8m board ?
>>>
>>> For me, patch 1/4 fixed the issue of not being able to define a fixed
>>> environment location. It now sits where I put it. It also allows me
>>> to boot over USB without having to define ENV_IS_NOWHERE.
>>
>> Sure, it also likely breaks every other MX8M board which does not define
>> that env_get_location() on board level, i.e. every MX8M board.
>
> env_get_location is defined in env/env.c
Except not the NXP SoC-specific variant of it which is pulled in by most
of the MX8M boards, and which is removed in 1/4, thus 1/4 changes the
behavior of many boards.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:22 ` Adam Ford
@ 2022-02-01 11:45 ` Marek Vasut
2022-02-01 11:51 ` Adam Ford
2022-02-01 11:56 ` Tommaso Merciai
1 sibling, 1 reply; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 11:45 UTC (permalink / raw)
To: Adam Ford, Tommaso Merciai
Cc: Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On 2/1/22 12:22, Adam Ford wrote:
> On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
> <tommaso.merciai@amarulasolutions.com> wrote:
>>
>> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
>>> On 1/31/22 23:15, Tommaso Merciai wrote:
>>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>>>> Override env_get_location function at board level, previously dropped
>>>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>>>
>>>>>> References:
>>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>>>
>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>>>> +{
>>>>>
>>>>> Why don't you just turn this into default __weak function and override it on
>>>>> board level when it is really needed to be overridden ?
>>>>
>>>> Hi Marek,
>>>> env_get_location is already declared as __weak, check env/env.c. We
>>>> can't override it 2 times.
>>>
>>> Oh, it is this problem with missing ability to define multiple levels of
>>> symbol strength.
>>>
>>> __weak enum env_location arch_env_get_location(enum env_operation op, int
>>> prio)
>>> {
>>> if (prio >= ARRAY_SIZE(env_locations))
>>> return ENVL_UNKNOWN;
>>>
>>> return env_locations[prio];
>>> }
>>>
>>> __weak enum env_location board_env_get_location(enum env_operation op, int
>>> prio)
>>> {
>>> return arch_env_get_location(op, prio);
>>> }
>>>
>>> __weak enum env_location env_get_location(enum env_operation op, int prio)
>>> {
>>> return board_env_get_location(op, prio);
>>> }
>>>
>>> By default, the compiler will optimize it all out. If you have arch-specific
>>> default (like imx does), implement arch_env_get_location(), if you have even
>>> board-specific default (like your board likely does), implement
>>> board_env_get_location(), if you need to override both, then override
>>> env_get_location() (unlikely).
>>>
>>> This is also inline with all the other arch_*() and board_*() functions we
>>> have, and you won't have much duplication either.
>>
>> Hi Marek,
>> Thanks for the tips, then if I understand correctly, your idea is: use:
>>
>> arch_env_get_location in (soc.c)
>>
>> In this way imx8m users can override this function at board level using:
>>
>> board_env_get_location
>>
>> right?
>
> What about those of us who want to use the default option found in
> env.c? It seems like we're creating more abstraction to address the
> abstraction we don't all want.
On MX8M platforms which have their own arch-specific default, you
override the env settings on your board level with the same stuff that
is in env/env.c already. Most of the MX8M users will likely stick to the
arch-specific MX8M default as they do so far.
> From my interpretation, the whole
> point of creating the default in env.c was to let people define the
> location of their environment, and this function in soc.c undid that.
> If people want it for their boards, put this function in their boards,
> otherwise, just use the default, or write your own.
[...]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:45 ` Marek Vasut
@ 2022-02-01 11:51 ` Adam Ford
2022-02-01 11:52 ` Marek Vasut
0 siblings, 1 reply; 26+ messages in thread
From: Adam Ford @ 2022-02-01 11:51 UTC (permalink / raw)
To: Marek Vasut
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On Tue, Feb 1, 2022 at 5:46 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/1/22 12:22, Adam Ford wrote:
> > On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
> > <tommaso.merciai@amarulasolutions.com> wrote:
> >>
> >> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
> >>> On 1/31/22 23:15, Tommaso Merciai wrote:
> >>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> >>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
> >>>>>> Override env_get_location function at board level, previously dropped
> >>>>>> down from arch/arm/mach-imx/imx8m/soc.c
> >>>>>>
> >>>>>> References:
> >>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> >>>>>>
> >>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> >>>>>> +{
> >>>>>
> >>>>> Why don't you just turn this into default __weak function and override it on
> >>>>> board level when it is really needed to be overridden ?
> >>>>
> >>>> Hi Marek,
> >>>> env_get_location is already declared as __weak, check env/env.c. We
> >>>> can't override it 2 times.
> >>>
> >>> Oh, it is this problem with missing ability to define multiple levels of
> >>> symbol strength.
> >>>
> >>> __weak enum env_location arch_env_get_location(enum env_operation op, int
> >>> prio)
> >>> {
> >>> if (prio >= ARRAY_SIZE(env_locations))
> >>> return ENVL_UNKNOWN;
> >>>
> >>> return env_locations[prio];
> >>> }
> >>>
> >>> __weak enum env_location board_env_get_location(enum env_operation op, int
> >>> prio)
> >>> {
> >>> return arch_env_get_location(op, prio);
> >>> }
> >>>
> >>> __weak enum env_location env_get_location(enum env_operation op, int prio)
> >>> {
> >>> return board_env_get_location(op, prio);
> >>> }
> >>>
> >>> By default, the compiler will optimize it all out. If you have arch-specific
> >>> default (like imx does), implement arch_env_get_location(), if you have even
> >>> board-specific default (like your board likely does), implement
> >>> board_env_get_location(), if you need to override both, then override
> >>> env_get_location() (unlikely).
> >>>
> >>> This is also inline with all the other arch_*() and board_*() functions we
> >>> have, and you won't have much duplication either.
> >>
> >> Hi Marek,
> >> Thanks for the tips, then if I understand correctly, your idea is: use:
> >>
> >> arch_env_get_location in (soc.c)
> >>
> >> In this way imx8m users can override this function at board level using:
> >>
> >> board_env_get_location
> >>
> >> right?
> >
> > What about those of us who want to use the default option found in
> > env.c? It seems like we're creating more abstraction to address the
> > abstraction we don't all want.
>
> On MX8M platforms which have their own arch-specific default, you
> override the env settings on your board level with the same stuff that
> is in env/env.c already. Most of the MX8M users will likely stick to the
> arch-specific MX8M default as they do so far.
This function is encapsulated in an ifdef which is only true for 8MN
and 8MP. It's not defined for 8MM or 8MQ, so those boards already
default back to the __weak function in env.c
adam
>
> > From my interpretation, the whole
> > point of creating the default in env.c was to let people define the
> > location of their environment, and this function in soc.c undid that.
> > If people want it for their boards, put this function in their boards,
> > otherwise, just use the default, or write your own.
>
> [...]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:51 ` Adam Ford
@ 2022-02-01 11:52 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 11:52 UTC (permalink / raw)
To: Adam Ford
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On 2/1/22 12:51, Adam Ford wrote:
> On Tue, Feb 1, 2022 at 5:46 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/1/22 12:22, Adam Ford wrote:
>>> On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
>>> <tommaso.merciai@amarulasolutions.com> wrote:
>>>>
>>>> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
>>>>> On 1/31/22 23:15, Tommaso Merciai wrote:
>>>>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>>>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>>>>>> Override env_get_location function at board level, previously dropped
>>>>>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>>>>>
>>>>>>>> References:
>>>>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>>>>>
>>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>>>>>> +{
>>>>>>>
>>>>>>> Why don't you just turn this into default __weak function and override it on
>>>>>>> board level when it is really needed to be overridden ?
>>>>>>
>>>>>> Hi Marek,
>>>>>> env_get_location is already declared as __weak, check env/env.c. We
>>>>>> can't override it 2 times.
>>>>>
>>>>> Oh, it is this problem with missing ability to define multiple levels of
>>>>> symbol strength.
>>>>>
>>>>> __weak enum env_location arch_env_get_location(enum env_operation op, int
>>>>> prio)
>>>>> {
>>>>> if (prio >= ARRAY_SIZE(env_locations))
>>>>> return ENVL_UNKNOWN;
>>>>>
>>>>> return env_locations[prio];
>>>>> }
>>>>>
>>>>> __weak enum env_location board_env_get_location(enum env_operation op, int
>>>>> prio)
>>>>> {
>>>>> return arch_env_get_location(op, prio);
>>>>> }
>>>>>
>>>>> __weak enum env_location env_get_location(enum env_operation op, int prio)
>>>>> {
>>>>> return board_env_get_location(op, prio);
>>>>> }
>>>>>
>>>>> By default, the compiler will optimize it all out. If you have arch-specific
>>>>> default (like imx does), implement arch_env_get_location(), if you have even
>>>>> board-specific default (like your board likely does), implement
>>>>> board_env_get_location(), if you need to override both, then override
>>>>> env_get_location() (unlikely).
>>>>>
>>>>> This is also inline with all the other arch_*() and board_*() functions we
>>>>> have, and you won't have much duplication either.
>>>>
>>>> Hi Marek,
>>>> Thanks for the tips, then if I understand correctly, your idea is: use:
>>>>
>>>> arch_env_get_location in (soc.c)
>>>>
>>>> In this way imx8m users can override this function at board level using:
>>>>
>>>> board_env_get_location
>>>>
>>>> right?
>>>
>>> What about those of us who want to use the default option found in
>>> env.c? It seems like we're creating more abstraction to address the
>>> abstraction we don't all want.
>>
>> On MX8M platforms which have their own arch-specific default, you
>> override the env settings on your board level with the same stuff that
>> is in env/env.c already. Most of the MX8M users will likely stick to the
>> arch-specific MX8M default as they do so far.
>
> This function is encapsulated in an ifdef which is only true for 8MN
> and 8MP. It's not defined for 8MM or 8MQ, so those boards already
> default back to the __weak function in env.c
So, it is also inconsistent, very nice.
Anyway, if we have arch and board level overrides, then we cover all the
requirements on all MX8Ms, without breaking existing boards.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:22 ` Adam Ford
2022-02-01 11:45 ` Marek Vasut
@ 2022-02-01 11:56 ` Tommaso Merciai
2022-02-01 11:57 ` Marek Vasut
1 sibling, 1 reply; 26+ messages in thread
From: Tommaso Merciai @ 2022-02-01 11:56 UTC (permalink / raw)
To: Adam Ford
Cc: Marek Vasut, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On Tue, Feb 01, 2022 at 05:22:06AM -0600, Adam Ford wrote:
> On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
> <tommaso.merciai@amarulasolutions.com> wrote:
> >
> > On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
> > > On 1/31/22 23:15, Tommaso Merciai wrote:
> > > > On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> > > > > On 1/31/22 17:58, Tommaso Merciai wrote:
> > > > > > Override env_get_location function at board level, previously dropped
> > > > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > > > >
> > > > > > References:
> > > > > > - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> > > > > >
> > > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> > > > > > +{
> > > > >
> > > > > Why don't you just turn this into default __weak function and override it on
> > > > > board level when it is really needed to be overridden ?
> > > >
> > > > Hi Marek,
> > > > env_get_location is already declared as __weak, check env/env.c. We
> > > > can't override it 2 times.
> > >
> > > Oh, it is this problem with missing ability to define multiple levels of
> > > symbol strength.
> > >
> > > __weak enum env_location arch_env_get_location(enum env_operation op, int
> > > prio)
> > > {
> > > if (prio >= ARRAY_SIZE(env_locations))
> > > return ENVL_UNKNOWN;
> > >
> > > return env_locations[prio];
> > > }
> > >
> > > __weak enum env_location board_env_get_location(enum env_operation op, int
> > > prio)
> > > {
> > > return arch_env_get_location(op, prio);
> > > }
> > >
> > > __weak enum env_location env_get_location(enum env_operation op, int prio)
> > > {
> > > return board_env_get_location(op, prio);
> > > }
> > >
> > > By default, the compiler will optimize it all out. If you have arch-specific
> > > default (like imx does), implement arch_env_get_location(), if you have even
> > > board-specific default (like your board likely does), implement
> > > board_env_get_location(), if you need to override both, then override
> > > env_get_location() (unlikely).
> > >
> > > This is also inline with all the other arch_*() and board_*() functions we
> > > have, and you won't have much duplication either.
> >
> > Hi Marek,
> > Thanks for the tips, then if I understand correctly, your idea is: use:
> >
> > arch_env_get_location in (soc.c)
> >
> > In this way imx8m users can override this function at board level using:
> >
> > board_env_get_location
> >
> > right?
>
> What about those of us who want to use the default option found in
> env.c?
Hi Adam,
You are right. Mmm in this way you have to duplicate the code of env.c
into your board.c. This doesn't look very functional.
I think remove it from soc.c is the right way.
> It seems like we're creating more abstraction to address the
> abstraction we don't all want. From my interpretation, the whole
> point of creating the default in env.c was to let people define the
> location of their environment, and this function in soc.c undid that.
> If people want it for their boards, put this function in their boards,
> otherwise, just use the default, or write your own.
Ack.
tommaso
>
> adam
> >
> > Thanks,
> > Tommaso
> >
> > --
> > Tommaso Merciai
> > Embedded Linux Developer
> > tommaso.merciai@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > T. +39 042 243 5310
> > info@amarulasolutions.com
> > www.amarulasolutions.com
--
Tommaso Merciai
Embedded Linux Developer
tommaso.merciai@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:56 ` Tommaso Merciai
@ 2022-02-01 11:57 ` Marek Vasut
2022-02-01 12:26 ` Adam Ford
0 siblings, 1 reply; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 11:57 UTC (permalink / raw)
To: Tommaso Merciai, Adam Ford
Cc: Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On 2/1/22 12:56, Tommaso Merciai wrote:
> On Tue, Feb 01, 2022 at 05:22:06AM -0600, Adam Ford wrote:
>> On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
>> <tommaso.merciai@amarulasolutions.com> wrote:
>>>
>>> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
>>>> On 1/31/22 23:15, Tommaso Merciai wrote:
>>>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>>>>> Override env_get_location function at board level, previously dropped
>>>>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>>>>
>>>>>>> References:
>>>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>>>>
>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>>>>> +{
>>>>>>
>>>>>> Why don't you just turn this into default __weak function and override it on
>>>>>> board level when it is really needed to be overridden ?
>>>>>
>>>>> Hi Marek,
>>>>> env_get_location is already declared as __weak, check env/env.c. We
>>>>> can't override it 2 times.
>>>>
>>>> Oh, it is this problem with missing ability to define multiple levels of
>>>> symbol strength.
>>>>
>>>> __weak enum env_location arch_env_get_location(enum env_operation op, int
>>>> prio)
>>>> {
>>>> if (prio >= ARRAY_SIZE(env_locations))
>>>> return ENVL_UNKNOWN;
>>>>
>>>> return env_locations[prio];
>>>> }
>>>>
>>>> __weak enum env_location board_env_get_location(enum env_operation op, int
>>>> prio)
>>>> {
>>>> return arch_env_get_location(op, prio);
>>>> }
>>>>
>>>> __weak enum env_location env_get_location(enum env_operation op, int prio)
>>>> {
>>>> return board_env_get_location(op, prio);
>>>> }
>>>>
>>>> By default, the compiler will optimize it all out. If you have arch-specific
>>>> default (like imx does), implement arch_env_get_location(), if you have even
>>>> board-specific default (like your board likely does), implement
>>>> board_env_get_location(), if you need to override both, then override
>>>> env_get_location() (unlikely).
>>>>
>>>> This is also inline with all the other arch_*() and board_*() functions we
>>>> have, and you won't have much duplication either.
>>>
>>> Hi Marek,
>>> Thanks for the tips, then if I understand correctly, your idea is: use:
>>>
>>> arch_env_get_location in (soc.c)
>>>
>>> In this way imx8m users can override this function at board level using:
>>>
>>> board_env_get_location
>>>
>>> right?
>>
>> What about those of us who want to use the default option found in
>> env.c?
>
> Hi Adam,
> You are right. Mmm in this way you have to duplicate the code of env.c
> into your board.c. This doesn't look very functional.
> I think remove it from soc.c is the right way.
How do you propose to fix all the boards which depend on the current
arch-specific behavior, duplicate that code in multiple copies into
board code ? That will be a lot of duplication too.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 11:57 ` Marek Vasut
@ 2022-02-01 12:26 ` Adam Ford
2022-02-01 12:38 ` Marek Vasut
0 siblings, 1 reply; 26+ messages in thread
From: Adam Ford @ 2022-02-01 12:26 UTC (permalink / raw)
To: Marek Vasut
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On Tue, Feb 1, 2022 at 5:58 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/1/22 12:56, Tommaso Merciai wrote:
> > On Tue, Feb 01, 2022 at 05:22:06AM -0600, Adam Ford wrote:
> >> On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
> >> <tommaso.merciai@amarulasolutions.com> wrote:
> >>>
> >>> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
> >>>> On 1/31/22 23:15, Tommaso Merciai wrote:
> >>>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> >>>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
> >>>>>>> Override env_get_location function at board level, previously dropped
> >>>>>>> down from arch/arm/mach-imx/imx8m/soc.c
> >>>>>>>
> >>>>>>> References:
> >>>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> >>>>>>>
> >>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
> >>>>>>> +{
> >>>>>>
> >>>>>> Why don't you just turn this into default __weak function and override it on
> >>>>>> board level when it is really needed to be overridden ?
> >>>>>
> >>>>> Hi Marek,
> >>>>> env_get_location is already declared as __weak, check env/env.c. We
> >>>>> can't override it 2 times.
> >>>>
> >>>> Oh, it is this problem with missing ability to define multiple levels of
> >>>> symbol strength.
> >>>>
> >>>> __weak enum env_location arch_env_get_location(enum env_operation op, int
> >>>> prio)
> >>>> {
> >>>> if (prio >= ARRAY_SIZE(env_locations))
> >>>> return ENVL_UNKNOWN;
> >>>>
> >>>> return env_locations[prio];
> >>>> }
> >>>>
> >>>> __weak enum env_location board_env_get_location(enum env_operation op, int
> >>>> prio)
> >>>> {
> >>>> return arch_env_get_location(op, prio);
> >>>> }
> >>>>
> >>>> __weak enum env_location env_get_location(enum env_operation op, int prio)
> >>>> {
> >>>> return board_env_get_location(op, prio);
> >>>> }
> >>>>
> >>>> By default, the compiler will optimize it all out. If you have arch-specific
> >>>> default (like imx does), implement arch_env_get_location(), if you have even
> >>>> board-specific default (like your board likely does), implement
> >>>> board_env_get_location(), if you need to override both, then override
> >>>> env_get_location() (unlikely).
> >>>>
> >>>> This is also inline with all the other arch_*() and board_*() functions we
> >>>> have, and you won't have much duplication either.
> >>>
> >>> Hi Marek,
> >>> Thanks for the tips, then if I understand correctly, your idea is: use:
> >>>
> >>> arch_env_get_location in (soc.c)
> >>>
> >>> In this way imx8m users can override this function at board level using:
> >>>
> >>> board_env_get_location
> >>>
> >>> right?
> >>
> >> What about those of us who want to use the default option found in
> >> env.c?
> >
> > Hi Adam,
> > You are right. Mmm in this way you have to duplicate the code of env.c
> > into your board.c. This doesn't look very functional.
> > I think remove it from soc.c is the right way.
>
> How do you propose to fix all the boards which depend on the current
> arch-specific behavior, duplicate that code in multiple copies into
> board code ? That will be a lot of duplication too.
I would think we should ask the users of 8MP and 8MN to test theirs
without this patch to see if it impacts them. The original patch was
done by NXP, so I assume they want it for their EVK's. This code
explicitly makes it impossible to set the environment location to a
device that was not a boot device. From what I can tell, we're
talking about a small number of boards since it's just 8MN and 8MP.
What about a compromise:
Rename env_get_location() in the soc.c to imx_env_get_location().
From the 8MN and 8MP boards (for people who want it or need it), write
a board function called env_get_location() which calls
imx_env_get_location(). This way the code remains, and people can
choose whether they want this behavior, or the behavior or the default
behavior used by 8MM and 8MQ. It's less duplicated code, and it
doesn't break the default behavior for those who want that.
adam
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location
2022-02-01 12:26 ` Adam Ford
@ 2022-02-01 12:38 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2022-02-01 12:38 UTC (permalink / raw)
To: Adam Ford
Cc: Tommaso Merciai, Michael Trimarchi, Stefano Babic, Fabio Estevam,
NXP i.MX U-Boot Team, Teresa Remmet, Peng Fan, Ye Li,
Simon Glass, Frieder Schrempf, Harald Seiler, Marek Behún,
U-Boot Mailing List
On 2/1/22 13:26, Adam Ford wrote:
> On Tue, Feb 1, 2022 at 5:58 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/1/22 12:56, Tommaso Merciai wrote:
>>> On Tue, Feb 01, 2022 at 05:22:06AM -0600, Adam Ford wrote:
>>>> On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
>>>> <tommaso.merciai@amarulasolutions.com> wrote:
>>>>>
>>>>> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
>>>>>> On 1/31/22 23:15, Tommaso Merciai wrote:
>>>>>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
>>>>>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
>>>>>>>>> Override env_get_location function at board level, previously dropped
>>>>>>>>> down from arch/arm/mach-imx/imx8m/soc.c
>>>>>>>>>
>>>>>>>>> References:
>>>>>>>>> - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.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)
>>>>>>>>> +{
>>>>>>>>
>>>>>>>> Why don't you just turn this into default __weak function and override it on
>>>>>>>> board level when it is really needed to be overridden ?
>>>>>>>
>>>>>>> Hi Marek,
>>>>>>> env_get_location is already declared as __weak, check env/env.c. We
>>>>>>> can't override it 2 times.
>>>>>>
>>>>>> Oh, it is this problem with missing ability to define multiple levels of
>>>>>> symbol strength.
>>>>>>
>>>>>> __weak enum env_location arch_env_get_location(enum env_operation op, int
>>>>>> prio)
>>>>>> {
>>>>>> if (prio >= ARRAY_SIZE(env_locations))
>>>>>> return ENVL_UNKNOWN;
>>>>>>
>>>>>> return env_locations[prio];
>>>>>> }
>>>>>>
>>>>>> __weak enum env_location board_env_get_location(enum env_operation op, int
>>>>>> prio)
>>>>>> {
>>>>>> return arch_env_get_location(op, prio);
>>>>>> }
>>>>>>
>>>>>> __weak enum env_location env_get_location(enum env_operation op, int prio)
>>>>>> {
>>>>>> return board_env_get_location(op, prio);
>>>>>> }
>>>>>>
>>>>>> By default, the compiler will optimize it all out. If you have arch-specific
>>>>>> default (like imx does), implement arch_env_get_location(), if you have even
>>>>>> board-specific default (like your board likely does), implement
>>>>>> board_env_get_location(), if you need to override both, then override
>>>>>> env_get_location() (unlikely).
>>>>>>
>>>>>> This is also inline with all the other arch_*() and board_*() functions we
>>>>>> have, and you won't have much duplication either.
>>>>>
>>>>> Hi Marek,
>>>>> Thanks for the tips, then if I understand correctly, your idea is: use:
>>>>>
>>>>> arch_env_get_location in (soc.c)
>>>>>
>>>>> In this way imx8m users can override this function at board level using:
>>>>>
>>>>> board_env_get_location
>>>>>
>>>>> right?
>>>>
>>>> What about those of us who want to use the default option found in
>>>> env.c?
>>>
>>> Hi Adam,
>>> You are right. Mmm in this way you have to duplicate the code of env.c
>>> into your board.c. This doesn't look very functional.
>>> I think remove it from soc.c is the right way.
>>
>> How do you propose to fix all the boards which depend on the current
>> arch-specific behavior, duplicate that code in multiple copies into
>> board code ? That will be a lot of duplication too.
>
> I would think we should ask the users of 8MP and 8MN to test theirs
> without this patch to see if it impacts them. The original patch was
> done by NXP, so I assume they want it for their EVK's. This code
> explicitly makes it impossible to set the environment location to a
> device that was not a boot device. From what I can tell, we're
> talking about a small number of boards since it's just 8MN and 8MP.
>
> What about a compromise:
>
> Rename env_get_location() in the soc.c to imx_env_get_location().
>
>>From the 8MN and 8MP boards (for people who want it or need it), write
> a board function called env_get_location() which calls
> imx_env_get_location(). This way the code remains, and people can
> choose whether they want this behavior, or the behavior or the default
> behavior used by 8MM and 8MQ. It's less duplicated code, and it
> doesn't break the default behavior for those who want that.
We are still talking about 40 lines of code being copied to 12 different
places, or, 12 boards which need to be fixed up -- instead of -- one
board which needs to be configured correctly to pick env from wherever
it should pick it from instead of the NXP default.
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-02-01 12:38 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-31 16:58 [PATCH v4 0/4] imx8m: move env_get_location for imx8mn and imx8mp at board level Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 1/4] imx8m: drop env_get_location for imx8mn and imx8mp Tommaso Merciai
2022-01-31 17:03 ` Marek Vasut
2022-01-31 17:06 ` Adam Ford
2022-01-31 16:58 ` [PATCH v4 2/4] imx: imx8mn_evk: override env_get_location Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 3/4] imx: imx8mp_evk: " Tommaso Merciai
2022-01-31 16:58 ` [PATCH v4 4/4] phytec: phycore_imx8mp: " Tommaso Merciai
2022-01-31 17:03 ` Marek Vasut
2022-01-31 22:15 ` Tommaso Merciai
2022-02-01 0:20 ` Adam Ford
2022-02-01 3:18 ` Marek Vasut
2022-02-01 11:16 ` Adam Ford
2022-02-01 11:21 ` Marek Vasut
2022-02-01 11:23 ` Adam Ford
2022-02-01 11:43 ` Marek Vasut
2022-02-01 3:16 ` Marek Vasut
2022-02-01 9:09 ` Tommaso Merciai
2022-02-01 11:22 ` Adam Ford
2022-02-01 11:45 ` Marek Vasut
2022-02-01 11:51 ` Adam Ford
2022-02-01 11:52 ` Marek Vasut
2022-02-01 11:56 ` Tommaso Merciai
2022-02-01 11:57 ` Marek Vasut
2022-02-01 12:26 ` Adam Ford
2022-02-01 12:38 ` Marek Vasut
2022-02-01 11:22 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.