[RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level
This series move env_get_location from soc to board level. As suggested by Michael michael@amarulasolutions.com make no sense to define an unique way for multiple board. One board can boot from emmc and having env on spi flash etc.. Anyways, this function is kept in both imx8mn and imx8mp evk boards instead of being completely dropped. (as suggested by Andrey andrey.zhizhikin@leica-geosystems.com)
Tommaso Merciai (3): imx8m: drop env_get_location for imx8mn and imx8mp imx: imx8mn_evk: override env_get_location imx: imx8mp_evk: override env_get_location
arch/arm/mach-imx/imx8m/soc.c | 39 ----------------------- board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++ board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 39 deletions(-)
This function defined for two architecture is not really generic and can generate problem when people add a new board.
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com --- Changes since v1: - Fix commit: drop down only env_get_location
arch/arm/mach-imx/imx8m/soc.c | 39 ----------------------------------- 1 file changed, 39 deletions(-)
diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c index 863508776d..f0030a557a 100644 --- a/arch/arm/mach-imx/imx8m/soc.c +++ b/arch/arm/mach-imx/imx8m/soc.c @@ -1313,45 +1313,6 @@ void do_error(struct pt_regs *pt_regs, unsigned int esr) #endif
#if defined(CONFIG_IMX8MN) || defined(CONFIG_IMX8MP) -enum env_location env_get_location(enum env_operation op, int prio) -{ - enum boot_device dev = get_boot_device(); - enum env_location env_loc = ENVL_UNKNOWN; - - if (prio) - return env_loc; - - switch (dev) { -#ifdef CONFIG_ENV_IS_IN_SPI_FLASH - case QSPI_BOOT: - env_loc = ENVL_SPI_FLASH; - break; -#endif -#ifdef CONFIG_ENV_IS_IN_NAND - case NAND_BOOT: - env_loc = ENVL_NAND; - break; -#endif -#ifdef CONFIG_ENV_IS_IN_MMC - case SD1_BOOT: - case SD2_BOOT: - case SD3_BOOT: - case MMC1_BOOT: - case MMC2_BOOT: - case MMC3_BOOT: - env_loc = ENVL_MMC; - break; -#endif - default: -#if defined(CONFIG_ENV_IS_NOWHERE) - env_loc = ENVL_NOWHERE; -#endif - break; - } - - return env_loc; -} - #ifndef ENV_IS_EMBEDDED long long env_get_offset(long long defautl_offset) {
Override env_get_location function at board level, previously dropped down from soc.c
References: - commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com --- Changes since v1: - Remove wrong env_get_offset function from commit
board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
diff --git a/board/freescale/imx8mn_evk/imx8mn_evk.c b/board/freescale/imx8mn_evk/imx8mn_evk.c index 9a0a0488bf..ef89a91ea2 100644 --- a/board/freescale/imx8mn_evk/imx8mn_evk.c +++ b/board/freescale/imx8mn_evk/imx8mn_evk.c @@ -5,11 +5,53 @@
#include <common.h> #include <env.h> +#include <env_internal.h> #include <init.h> +#include <asm/arch/sys_proto.h> +#include <asm/mach-imx/boot_mode.h> #include <asm/global_data.h>
DECLARE_GLOBAL_DATA_PTR;
+enum env_location env_get_location(enum env_operation op, int prio) +{ + enum boot_device dev = get_boot_device(); + enum env_location env_loc = ENVL_UNKNOWN; + + if (prio) + return env_loc; + + switch (dev) { +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH + case QSPI_BOOT: + env_loc = ENVL_SPI_FLASH; + break; +#endif +#ifdef CONFIG_ENV_IS_IN_NAND + case NAND_BOOT: + env_loc = ENVL_NAND; + break; +#endif +#ifdef CONFIG_ENV_IS_IN_MMC + case SD1_BOOT: + case SD2_BOOT: + case SD3_BOOT: + case MMC1_BOOT: + case MMC2_BOOT: + case MMC3_BOOT: + env_loc = ENVL_MMC; + break; +#endif + default: +#if defined(CONFIG_ENV_IS_NOWHERE) + env_loc = ENVL_NOWHERE; +#endif + break; + } + + return env_loc; +} + int board_init(void) { return 0;
Override env_get_location function at board level, previously dropped down from soc.c
References: - commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com --- Changes since v1: - Remove wrong env_get_offset function from commit
board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c b/board/freescale/imx8mp_evk/imx8mp_evk.c index 62096c24fb..6b2eeaf152 100644 --- a/board/freescale/imx8mp_evk/imx8mp_evk.c +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c @@ -5,6 +5,7 @@
#include <common.h> #include <env.h> +#include <env_internal.h> #include <errno.h> #include <init.h> #include <miiphy.h> @@ -17,6 +18,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/mach-imx/gpio.h> +#include <asm/mach-imx/boot_mode.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = { MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B | MUX_PAD_CTRL(WDOG_PAD_CTRL), };
+enum env_location env_get_location(enum env_operation op, int prio) +{ + enum boot_device dev = get_boot_device(); + enum env_location env_loc = ENVL_UNKNOWN; + + if (prio) + return env_loc; + + switch (dev) { +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH + case QSPI_BOOT: + env_loc = ENVL_SPI_FLASH; + break; +#endif +#ifdef CONFIG_ENV_IS_IN_NAND + case NAND_BOOT: + env_loc = ENVL_NAND; + break; +#endif +#ifdef CONFIG_ENV_IS_IN_MMC + case SD1_BOOT: + case SD2_BOOT: + case SD3_BOOT: + case MMC1_BOOT: + case MMC2_BOOT: + case MMC3_BOOT: + env_loc = ENVL_MMC; + break; +#endif + default: +#if defined(CONFIG_ENV_IS_NOWHERE) + env_loc = ENVL_NOWHERE; +#endif + break; + } + + return env_loc; +} + int board_early_init_f(void) { struct wdog_regs *wdog = (struct wdog_regs *)WDOG1_BASE_ADDR;
On Fri, 26 Nov 2021 18:43:31 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
Override env_get_location function at board level, previously dropped down from soc.c
References:
- commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com
Changes since v1:
- Remove wrong env_get_offset function from commit
board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c b/board/freescale/imx8mp_evk/imx8mp_evk.c index 62096c24fb..6b2eeaf152 100644 --- a/board/freescale/imx8mp_evk/imx8mp_evk.c +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c @@ -5,6 +5,7 @@
#include <common.h> #include <env.h> +#include <env_internal.h> #include <errno.h> #include <init.h> #include <miiphy.h> @@ -17,6 +18,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/mach-imx/gpio.h> +#include <asm/mach-imx/boot_mode.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = { MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B | MUX_PAD_CTRL(WDOG_PAD_CTRL), };
+enum env_location env_get_location(enum env_operation op, int prio) +{
- enum boot_device dev = get_boot_device();
- enum env_location env_loc = ENVL_UNKNOWN;
- if (prio)
return env_loc;- switch (dev) {
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
- case QSPI_BOOT:
env_loc = ENVL_SPI_FLASH;break;+#endif +#ifdef CONFIG_ENV_IS_IN_NAND
- case NAND_BOOT:
env_loc = ENVL_NAND;break;+#endif +#ifdef CONFIG_ENV_IS_IN_MMC
- case SD1_BOOT:
- case SD2_BOOT:
- case SD3_BOOT:
- case MMC1_BOOT:
- case MMC2_BOOT:
- case MMC3_BOOT:
env_loc = ENVL_MMC;break;+#endif
- default:
+#if defined(CONFIG_ENV_IS_NOWHERE)
env_loc = ENVL_NOWHERE;+#endif
Using if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) instead of #ifdefs is preferred because the compiler then warns about bugs even in the disabled codepaths (that's why checkpatch complains about #ifdefs).
I know that this cannot be combined with switch() in a simple way, though.
Do you need to use switch? Can't you rewrite it to use if()s and use IS_ENABLED()?
Marek
On Fri, Nov 26, 2021 at 07:00:21PM +0100, Marek Behún wrote:
On Fri, 26 Nov 2021 18:43:31 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
Override env_get_location function at board level, previously dropped down from soc.c
References:
- commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com
Changes since v1:
- Remove wrong env_get_offset function from commit
board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c b/board/freescale/imx8mp_evk/imx8mp_evk.c index 62096c24fb..6b2eeaf152 100644 --- a/board/freescale/imx8mp_evk/imx8mp_evk.c +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c @@ -5,6 +5,7 @@
#include <common.h> #include <env.h> +#include <env_internal.h> #include <errno.h> #include <init.h> #include <miiphy.h> @@ -17,6 +18,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/mach-imx/gpio.h> +#include <asm/mach-imx/boot_mode.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = { MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B | MUX_PAD_CTRL(WDOG_PAD_CTRL), };
+enum env_location env_get_location(enum env_operation op, int prio) +{
- enum boot_device dev = get_boot_device();
- enum env_location env_loc = ENVL_UNKNOWN;
- if (prio)
return env_loc;- switch (dev) {
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
- case QSPI_BOOT:
env_loc = ENVL_SPI_FLASH;break;+#endif +#ifdef CONFIG_ENV_IS_IN_NAND
- case NAND_BOOT:
env_loc = ENVL_NAND;break;+#endif +#ifdef CONFIG_ENV_IS_IN_MMC
- case SD1_BOOT:
- case SD2_BOOT:
- case SD3_BOOT:
- case MMC1_BOOT:
- case MMC2_BOOT:
- case MMC3_BOOT:
env_loc = ENVL_MMC;break;+#endif
- default:
+#if defined(CONFIG_ENV_IS_NOWHERE)
env_loc = ENVL_NOWHERE;+#endif
Using if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) instead of #ifdefs is preferred because the compiler then warns about bugs even in the disabled codepaths (that's why checkpatch complains about #ifdefs).
I know that this cannot be combined with switch() in a simple way, though.
Do you need to use switch? Can't you rewrite it to use if()s and use IS_ENABLED()?
Marek
Hi Marek, Thanks for your review. What do you think about this solution?
enum env_location env_get_location(enum env_operation op, int prio) { enum boot_device dev = get_boot_device(); enum env_location env_loc = ENVL_UNKNOWN;
if (prio) return env_loc;
if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) && dev == QSPI_BOOT) { env_loc = ENVL_SPI_FLASH; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND) && dev == NAND_BOOT) { env_loc = ENVL_NAND; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) { switch (dev) { case SD1_BOOT: case SD2_BOOT: case SD3_BOOT: case MMC1_BOOT: case MMC2_BOOT: case MMC3_BOOT: env_loc = ENVL_MMC; break; default: break; } } else if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) { env_loc = ENVL_MMC; }
return env_loc; }
Let me know.
Thanks, tommaso
On Sun, 28 Nov 2021 18:47:11 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
On Fri, Nov 26, 2021 at 07:00:21PM +0100, Marek Behún wrote:
On Fri, 26 Nov 2021 18:43:31 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
Override env_get_location function at board level, previously dropped down from soc.c
References:
- commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com
Changes since v1:
- Remove wrong env_get_offset function from commit
board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c b/board/freescale/imx8mp_evk/imx8mp_evk.c index 62096c24fb..6b2eeaf152 100644 --- a/board/freescale/imx8mp_evk/imx8mp_evk.c +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c @@ -5,6 +5,7 @@
#include <common.h> #include <env.h> +#include <env_internal.h> #include <errno.h> #include <init.h> #include <miiphy.h> @@ -17,6 +18,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/mach-imx/gpio.h> +#include <asm/mach-imx/boot_mode.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = { MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B | MUX_PAD_CTRL(WDOG_PAD_CTRL), };
+enum env_location env_get_location(enum env_operation op, int prio) +{
- enum boot_device dev = get_boot_device();
- enum env_location env_loc = ENVL_UNKNOWN;
- if (prio)
return env_loc;- switch (dev) {
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
- case QSPI_BOOT:
env_loc = ENVL_SPI_FLASH;break;+#endif +#ifdef CONFIG_ENV_IS_IN_NAND
- case NAND_BOOT:
env_loc = ENVL_NAND;break;+#endif +#ifdef CONFIG_ENV_IS_IN_MMC
- case SD1_BOOT:
- case SD2_BOOT:
- case SD3_BOOT:
- case MMC1_BOOT:
- case MMC2_BOOT:
- case MMC3_BOOT:
env_loc = ENVL_MMC;break;+#endif
- default:
+#if defined(CONFIG_ENV_IS_NOWHERE)
env_loc = ENVL_NOWHERE;+#endif
Using if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) instead of #ifdefs is preferred because the compiler then warns about bugs even in the disabled codepaths (that's why checkpatch complains about #ifdefs).
I know that this cannot be combined with switch() in a simple way, though.
Do you need to use switch? Can't you rewrite it to use if()s and use IS_ENABLED()?
Marek
Hi Marek, Thanks for your review. What do you think about this solution?
enum env_location env_get_location(enum env_operation op, int prio) { enum boot_device dev = get_boot_device(); enum env_location env_loc = ENVL_UNKNOWN;
if (prio) return env_loc;
if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) && dev == QSPI_BOOT) { env_loc = ENVL_SPI_FLASH; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND) && dev == NAND_BOOT) { env_loc = ENVL_NAND; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) { switch (dev) { case SD1_BOOT: case SD2_BOOT: case SD3_BOOT: case MMC1_BOOT: case MMC2_BOOT: case MMC3_BOOT: env_loc = ENVL_MMC; break; default: break; } } else if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) { env_loc = ENVL_MMC; }
return env_loc; }
Thanks, this looks ok, just run it through checkpatch to correct the indentation of 'case' statements and put 'else if' on the same line as '}':
if () { } else if () { } ...
Marek
On Mon, Nov 29, 2021 at 01:17:44PM +0100, Marek Behún wrote:
On Sun, 28 Nov 2021 18:47:11 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
On Fri, Nov 26, 2021 at 07:00:21PM +0100, Marek Behún wrote:
On Fri, 26 Nov 2021 18:43:31 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
Override env_get_location function at board level, previously dropped down from soc.c
References:
- commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com
Changes since v1:
- Remove wrong env_get_offset function from commit
board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c b/board/freescale/imx8mp_evk/imx8mp_evk.c index 62096c24fb..6b2eeaf152 100644 --- a/board/freescale/imx8mp_evk/imx8mp_evk.c +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c @@ -5,6 +5,7 @@
#include <common.h> #include <env.h> +#include <env_internal.h> #include <errno.h> #include <init.h> #include <miiphy.h> @@ -17,6 +18,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/mach-imx/gpio.h> +#include <asm/mach-imx/boot_mode.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = { MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B | MUX_PAD_CTRL(WDOG_PAD_CTRL), };
+enum env_location env_get_location(enum env_operation op, int prio) +{
- enum boot_device dev = get_boot_device();
- enum env_location env_loc = ENVL_UNKNOWN;
- if (prio)
return env_loc;- switch (dev) {
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
- case QSPI_BOOT:
env_loc = ENVL_SPI_FLASH;break;+#endif +#ifdef CONFIG_ENV_IS_IN_NAND
- case NAND_BOOT:
env_loc = ENVL_NAND;break;+#endif +#ifdef CONFIG_ENV_IS_IN_MMC
- case SD1_BOOT:
- case SD2_BOOT:
- case SD3_BOOT:
- case MMC1_BOOT:
- case MMC2_BOOT:
- case MMC3_BOOT:
env_loc = ENVL_MMC;break;+#endif
- default:
+#if defined(CONFIG_ENV_IS_NOWHERE)
env_loc = ENVL_NOWHERE;+#endif
Using if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) instead of #ifdefs is preferred because the compiler then warns about bugs even in the disabled codepaths (that's why checkpatch complains about #ifdefs).
I know that this cannot be combined with switch() in a simple way, though.
Do you need to use switch? Can't you rewrite it to use if()s and use IS_ENABLED()?
Marek
Hi Marek, Thanks for your review. What do you think about this solution?
enum env_location env_get_location(enum env_operation op, int prio) { enum boot_device dev = get_boot_device(); enum env_location env_loc = ENVL_UNKNOWN;
if (prio) return env_loc;
if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) && dev == QSPI_BOOT) { env_loc = ENVL_SPI_FLASH; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND) && dev == NAND_BOOT) { env_loc = ENVL_NAND; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) { switch (dev) { case SD1_BOOT: case SD2_BOOT: case SD3_BOOT: case MMC1_BOOT: case MMC2_BOOT: case MMC3_BOOT: env_loc = ENVL_MMC; break; default: break; } } else if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) { env_loc = ENVL_MMC; }
return env_loc; }
Thanks, this looks ok, just run it through checkpatch to correct the indentation of 'case' statements and put 'else if' on the same line as '}':
if () { } else if () { } ...
Marek
Thanks Marek for your review. Sent v2.
Let me know, Tommaso
On Tue, Nov 30, 2021 at 09:23:18PM +0100, Tommaso Merciai wrote:
On Mon, Nov 29, 2021 at 01:17:44PM +0100, Marek Behún wrote:
On Sun, 28 Nov 2021 18:47:11 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
On Fri, Nov 26, 2021 at 07:00:21PM +0100, Marek Behún wrote:
On Fri, 26 Nov 2021 18:43:31 +0100 Tommaso Merciai tomm.merciai@gmail.com wrote:
Override env_get_location function at board level, previously dropped down from soc.c
References:
- commit f1575f23df1ef704051f218d5bc4aeeb20c2c542
Signed-off-by: Tommaso Merciai tomm.merciai@gmail.com
Changes since v1:
- Remove wrong env_get_offset function from commit
board/freescale/imx8mp_evk/imx8mp_evk.c | 41 +++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/board/freescale/imx8mp_evk/imx8mp_evk.c b/board/freescale/imx8mp_evk/imx8mp_evk.c index 62096c24fb..6b2eeaf152 100644 --- a/board/freescale/imx8mp_evk/imx8mp_evk.c +++ b/board/freescale/imx8mp_evk/imx8mp_evk.c @@ -5,6 +5,7 @@
#include <common.h> #include <env.h> +#include <env_internal.h> #include <errno.h> #include <init.h> #include <miiphy.h> @@ -17,6 +18,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/mach-imx/gpio.h> +#include <asm/mach-imx/boot_mode.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -32,6 +34,45 @@ static iomux_v3_cfg_t const wdog_pads[] = { MX8MP_PAD_GPIO1_IO02__WDOG1_WDOG_B | MUX_PAD_CTRL(WDOG_PAD_CTRL), };
+enum env_location env_get_location(enum env_operation op, int prio) +{
- enum boot_device dev = get_boot_device();
- enum env_location env_loc = ENVL_UNKNOWN;
- if (prio)
return env_loc;- switch (dev) {
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
- case QSPI_BOOT:
env_loc = ENVL_SPI_FLASH;break;+#endif +#ifdef CONFIG_ENV_IS_IN_NAND
- case NAND_BOOT:
env_loc = ENVL_NAND;break;+#endif +#ifdef CONFIG_ENV_IS_IN_MMC
- case SD1_BOOT:
- case SD2_BOOT:
- case SD3_BOOT:
- case MMC1_BOOT:
- case MMC2_BOOT:
- case MMC3_BOOT:
env_loc = ENVL_MMC;break;+#endif
- default:
+#if defined(CONFIG_ENV_IS_NOWHERE)
env_loc = ENVL_NOWHERE;+#endif
Using if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) instead of #ifdefs is preferred because the compiler then warns about bugs even in the disabled codepaths (that's why checkpatch complains about #ifdefs).
I know that this cannot be combined with switch() in a simple way, though.
Do you need to use switch? Can't you rewrite it to use if()s and use IS_ENABLED()?
Marek
Hi Marek, Thanks for your review. What do you think about this solution?
enum env_location env_get_location(enum env_operation op, int prio) { enum boot_device dev = get_boot_device(); enum env_location env_loc = ENVL_UNKNOWN;
if (prio) return env_loc;
if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) && dev == QSPI_BOOT) { env_loc = ENVL_SPI_FLASH; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND) && dev == NAND_BOOT) { env_loc = ENVL_NAND; } else if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) { switch (dev) { case SD1_BOOT: case SD2_BOOT: case SD3_BOOT: case MMC1_BOOT: case MMC2_BOOT: case MMC3_BOOT: env_loc = ENVL_MMC; break; default: break; } } else if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) { env_loc = ENVL_MMC; }
return env_loc; }
Thanks, this looks ok, just run it through checkpatch to correct the indentation of 'case' statements and put 'else if' on the same line as '}':
if () { } else if () { } ...
Marek
Thanks Marek for your review. Sent v2.
Let me know, Tommaso
Hi Marek, Have you had the time to check v2 yet?
Thanks, tommaso
Hello Tommaso,
-----Original Message----- From: Tommaso Merciai tomm.merciai@gmail.com Sent: Friday, November 26, 2021 6:43 PM Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica- geosystems.com>; Tommaso Merciai tomm.merciai@gmail.com; Stefano Babic sbabic@denx.de; Fabio Estevam festevam@gmail.com; NXP i.MX U-Boot Team uboot-imx@nxp.com; Peng Fan peng.fan@nxp.com; Ye Li ye.li@nxp.com; Marek Vasut marex@denx.de; Simon Glass sjg@chromium.org; Frieder Schrempf frieder.schrempf@kontron.de; Marek Behún marek.behun@nic.cz; Ying-Chun Liu (PaulLiu) paulliu@debian.org; u-boot@lists.denx.de Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level
This series move env_get_location from soc to board level. As suggested by Michael michael@amarulasolutions.com make no sense to define an unique way for multiple board. One board can boot from emmc and having env on spi flash etc.. Anyways, this function is kept in both imx8mn and imx8mp evk boards instead of being completely dropped. (as suggested by Andrey andrey.zhizhikin@leica-geosystems.com)
I believe there has been another suggestion from my side regarding this patch: Since it look like that Michael Trimarchi submitted another part to drop env_get_offset() in [1], combined with the first patch in this series - it is a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and env_get_location").
I suggest you to submit a revert instead of your first patch and deprecate the patch from Michael, instead of having 2 separate patches for this.
Tommaso Merciai (3): imx8m: drop env_get_location for imx8mn and imx8mp imx: imx8mn_evk: override env_get_location imx: imx8mp_evk: override env_get_location
arch/arm/mach-imx/imx8m/soc.c | 39 ----------------------- board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++ board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 39 deletions(-)
-- 2.25.1
Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-micha...
-- andrey
HI
On Mon, Nov 29, 2021 at 9:38 AM ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com wrote:
Hello Tommaso,
-----Original Message----- From: Tommaso Merciai tomm.merciai@gmail.com Sent: Friday, November 26, 2021 6:43 PM Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica- geosystems.com>; Tommaso Merciai tomm.merciai@gmail.com; Stefano Babic sbabic@denx.de; Fabio Estevam festevam@gmail.com; NXP i.MX U-Boot Team uboot-imx@nxp.com; Peng Fan peng.fan@nxp.com; Ye Li ye.li@nxp.com; Marek Vasut marex@denx.de; Simon Glass sjg@chromium.org; Frieder Schrempf frieder.schrempf@kontron.de; Marek Behún marek.behun@nic.cz; Ying-Chun Liu (PaulLiu) paulliu@debian.org; u-boot@lists.denx.de Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level
This series move env_get_location from soc to board level. As suggested by Michael michael@amarulasolutions.com make no sense to define an unique way for multiple board. One board can boot from emmc and having env on spi flash etc.. Anyways, this function is kept in both imx8mn and imx8mp evk boards instead of being completely dropped. (as suggested by Andrey andrey.zhizhikin@leica-geosystems.com)
I believe there has been another suggestion from my side regarding this patch: Since it look like that Michael Trimarchi submitted another part to drop env_get_offset() in [1], combined with the first patch in this series - it is a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and env_get_location").
I suggest you to submit a revert instead of your first patch and deprecate the patch from Michael, instead of having 2 separate patches for this.
I think they are totally different problems. One is code not used and the other moves that implementation in specific parts.
Michael
Tommaso Merciai (3): imx8m: drop env_get_location for imx8mn and imx8mp imx: imx8mn_evk: override env_get_location imx: imx8mp_evk: override env_get_location
arch/arm/mach-imx/imx8m/soc.c | 39 ----------------------- board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++ board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 39 deletions(-)
-- 2.25.1
Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-micha...
-- andrey
Hello Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Monday, November 29, 2021 9:40 AM To: ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com Cc: Tommaso Merciai tomm.merciai@gmail.com; Stefano Babic sbabic@denx.de; Fabio Estevam festevam@gmail.com; NXP i.MX U-Boot Team uboot-imx@nxp.com; Peng Fan peng.fan@nxp.com; Ye Li ye.li@nxp.com; Marek Vasut marex@denx.de; Simon Glass sjg@chromium.org; Frieder Schrempf frieder.schrempf@kontron.de; Marek Behún marek.behun@nic.cz; Ying-Chun Liu (PaulLiu) paulliu@debian.org; u-boot@lists.denx.de Subject: Re: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level
HI
On Mon, Nov 29, 2021 at 9:38 AM ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com wrote:
Hello Tommaso,
-----Original Message----- From: Tommaso Merciai tomm.merciai@gmail.com Sent: Friday, November 26, 2021 6:43 PM Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica- geosystems.com>; Tommaso Merciai tomm.merciai@gmail.com; Stefano Babic sbabic@denx.de; Fabio Estevam festevam@gmail.com; NXP i.MX U-Boot Team uboot-imx@nxp.com; Peng Fan peng.fan@nxp.com; Ye Li ye.li@nxp.com;
Marek
Vasut marex@denx.de; Simon Glass sjg@chromium.org; Frieder Schrempf frieder.schrempf@kontron.de; Marek Behún marek.behun@nic.cz; Ying-Chun
Liu
(PaulLiu) paulliu@debian.org; u-boot@lists.denx.de Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp
at
board level
This series move env_get_location from soc to board level. As suggested by Michael michael@amarulasolutions.com make no sense to define an unique way for multiple board. One board can boot from emmc and having env on spi flash etc.. Anyways, this function is kept in both imx8mn and imx8mp evk boards instead of being completely dropped. (as suggested by Andrey andrey.zhizhikin@leica-geosystems.com)
I believe there has been another suggestion from my side regarding this patch: Since it look like that Michael Trimarchi submitted another part to drop env_get_offset() in [1], combined with the first patch in this series - it is a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and env_get_location").
I suggest you to submit a revert instead of your first patch and deprecate the patch from Michael, instead of having 2 separate patches for this.
I think they are totally different problems. One is code not used and the other moves that implementation in specific parts.
They might be logically different, but 2 commits combined together - is a full revert to me.
I'd leave this up to maintainer to decide, but for me it would be logical to see a revert instead of 2 separate commits - this makes tracking more transparent.
Michael
Tommaso Merciai (3): imx8m: drop env_get_location for imx8mn and imx8mp imx: imx8mn_evk: override env_get_location imx: imx8mp_evk: override env_get_location
arch/arm/mach-imx/imx8m/soc.c | 39 ----------------------- board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++ board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 39 deletions(-)
-- 2.25.1
Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-micha...
-- andrey
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com https://eur02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarula... ions.com%2F&data=04%7C01%7C%7C666e97d4bb5f425c9a0408d9b313dd84%7C1b16ab3eb8f6 4fe39f3e2db7fe549f6a%7C0%7C0%7C637737720180120994%7CUnknown%7CTWFpbGZsb3d8eyJWIjo iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NJQ7 qjpMWu%2BhGoIcqwmD%2BLc4Ekq1oHjqPSCqkiCr4DA%3D&reserved=0
-- andrey
Hi
On Mon, Nov 29, 2021 at 9:46 AM ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com wrote:
Hello Michael,
-----Original Message----- From: Michael Nazzareno Trimarchi michael@amarulasolutions.com Sent: Monday, November 29, 2021 9:40 AM To: ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com Cc: Tommaso Merciai tomm.merciai@gmail.com; Stefano Babic sbabic@denx.de; Fabio Estevam festevam@gmail.com; NXP i.MX U-Boot Team uboot-imx@nxp.com; Peng Fan peng.fan@nxp.com; Ye Li ye.li@nxp.com; Marek Vasut marex@denx.de; Simon Glass sjg@chromium.org; Frieder Schrempf frieder.schrempf@kontron.de; Marek Behún marek.behun@nic.cz; Ying-Chun Liu (PaulLiu) paulliu@debian.org; u-boot@lists.denx.de Subject: Re: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp at board level
HI
On Mon, Nov 29, 2021 at 9:38 AM ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com wrote:
Hello Tommaso,
-----Original Message----- From: Tommaso Merciai tomm.merciai@gmail.com Sent: Friday, November 26, 2021 6:43 PM Cc: michael@amarulasolutions.com; ZHIZHIKIN Andrey <andrey.zhizhikin@leica- geosystems.com>; Tommaso Merciai tomm.merciai@gmail.com; Stefano Babic sbabic@denx.de; Fabio Estevam festevam@gmail.com; NXP i.MX U-Boot Team uboot-imx@nxp.com; Peng Fan peng.fan@nxp.com; Ye Li ye.li@nxp.com;
Marek
Vasut marex@denx.de; Simon Glass sjg@chromium.org; Frieder Schrempf frieder.schrempf@kontron.de; Marek Behún marek.behun@nic.cz; Ying-Chun
Liu
(PaulLiu) paulliu@debian.org; u-boot@lists.denx.de Subject: [RFC PATCH 0/3] imx8m: move env_get_location for imx8mn and imx8mp
at
board level
This series move env_get_location from soc to board level. As suggested by Michael michael@amarulasolutions.com make no sense to define an unique way for multiple board. One board can boot from emmc and having env on spi flash etc.. Anyways, this function is kept in both imx8mn and imx8mp evk boards instead of being completely dropped. (as suggested by Andrey andrey.zhizhikin@leica-geosystems.com)
I believe there has been another suggestion from my side regarding this patch: Since it look like that Michael Trimarchi submitted another part to drop env_get_offset() in [1], combined with the first patch in this series - it is a complete revert of 2707faf01f ("imx8mn/imx8mp: override env_get_offset and env_get_location").
I suggest you to submit a revert instead of your first patch and deprecate the patch from Michael, instead of having 2 separate patches for this.
I think they are totally different problems. One is code not used and the other moves that implementation in specific parts.
They might be logically different, but 2 commits combined together - is a full revert to me.
I'd leave this up to maintainer to decide, but for me it would be logical to see a revert instead of 2 separate commits - this makes tracking more transparent.
The first one (mine) is not a logical change. It means that nothing get wrong. The other is anywway some change so if you needs to revert then you can pick specific part.
Michael
Michael
Tommaso Merciai (3): imx8m: drop env_get_location for imx8mn and imx8mp imx: imx8mn_evk: override env_get_location imx: imx8mp_evk: override env_get_location
arch/arm/mach-imx/imx8m/soc.c | 39 ----------------------- board/freescale/imx8mn_evk/imx8mn_evk.c | 42 +++++++++++++++++++++++++ board/freescale/imx8mp_evk/imx8mp_evk.c | 41 ++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 39 deletions(-)
-- 2.25.1
Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211117143456.34441-1-micha...
-- andrey
-- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________
Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com https://eur02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarula... ions.com%2F&data=04%7C01%7C%7C666e97d4bb5f425c9a0408d9b313dd84%7C1b16ab3eb8f6 4fe39f3e2db7fe549f6a%7C0%7C0%7C637737720180120994%7CUnknown%7CTWFpbGZsb3d8eyJWIjo iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NJQ7 qjpMWu%2BhGoIcqwmD%2BLc4Ekq1oHjqPSCqkiCr4DA%3D&reserved=0
-- andrey
participants (4)
-
Marek Behún -
Michael Nazzareno Trimarchi -
Tommaso Merciai -
ZHIZHIKIN Andrey