All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] env: Access Environment in SPI flashes before relocation
@ 2020-10-10  8:28 Heiko Schocher
  2020-10-10  8:28 ` [PATCH v4 1/3] env: split env_import_redund() into 2 functions Heiko Schocher
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Heiko Schocher @ 2020-10-10  8:28 UTC (permalink / raw)
  To: u-boot

Add the possibility to access Environment stored in SPI NOR
Flash before relocation. This is used in DM/DTS rework for
the aristainetos boards. There mutliple DTS are packed into
a FIT image and through a variable in the Environment
the correct DTS will be selected. For this we need very
early access to the Environment. Also may this is good
for selecting the console baudrate again through Environment
which I think currently does not work for any board based on
DM/DTS and with Environment in SPI NOR flash.

Add this to aristainetos board, as aristainetos DM board
updates are now in mainline.

travis build:
https://travis-ci.org/github/hsdenx/u-boot-test/builds/733559949

Changes in v4:
- rebased to current master 5dcf7cc590

Changes in v3:
- env_sf_init_early() always return 0 now. If we do not return
  0 in this function, env_set_inited() never get called,
  which has the consequence that env_load/save/erase never
  work, because they check if the init bit is set.
- add comment from Simon Glass
  - add missing function comments
  - use if(IS_ENABLED...)
  - drop extra brackets
  - let env_sf_init() decide, which function to call
    add comment that it is necessary to return env_sf_init()
    with 0.
- new in v3 as aristainetos board DM changes are in
  mainline now

Changes in v2:
- patch "env: split env_import_redund into 2 functions"
  is new in version 2. Idea is to not duplicate too much
  code as Simon suggested.

Heiko Schocher (3):
  env: split env_import_redund() into 2 functions
  env: Access Environment in SPI flashes before relocation
  imx6: enable early spi environment access on aristainetos boards

 configs/aristainetos2_defconfig     |   1 +
 configs/aristainetos2b_defconfig    |   1 +
 configs/aristainetos2bcsl_defconfig |   1 +
 configs/aristainetos2c_defconfig    |   1 +
 env/Kconfig                         |   8 +++
 env/common.c                        |  42 +++++++++---
 env/sf.c                            | 100 +++++++++++++++++++++++++++-
 include/env.h                       |  18 +++++
 8 files changed, 159 insertions(+), 13 deletions(-)

-- 
2.25.4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 1/3] env: split env_import_redund() into 2 functions
  2020-10-10  8:28 [PATCH v4 0/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
@ 2020-10-10  8:28 ` Heiko Schocher
  2020-10-12  3:35   ` Simon Glass
  2020-10-30 18:46   ` Tom Rini
  2020-10-10  8:28 ` [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
  2020-10-10  8:28 ` [PATCH v4 3/3] imx6: enable early spi environment access on aristainetos boards Heiko Schocher
  2 siblings, 2 replies; 13+ messages in thread
From: Heiko Schocher @ 2020-10-10  8:28 UTC (permalink / raw)
  To: u-boot

split from env_import_redund() the part which checks
which Environment is valid into a separate function
called env_check_redund() and call it from env_import_redund().

So env_check_redund() can be used from places which also
need to do this checks.

Signed-off-by: Heiko Schocher <hs@denx.de>

---
The return values from env_check_redund() for the cases
only one read buffer is OK are not perfect, may we should
find better return codes?

(no changes since v2)

Changes in v2:
- patch "env: split env_import_redund into 2 functions"
  is new in version 2. Idea is to not duplicate too much
  code as Simon suggested.

 env/common.c  | 42 ++++++++++++++++++++++++++++++++----------
 include/env.h | 18 ++++++++++++++++++
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/env/common.c b/env/common.c
index ed18378000..6c32a9b479 100644
--- a/env/common.c
+++ b/env/common.c
@@ -141,12 +141,11 @@ int env_import(const char *buf, int check, int flags)
 #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
 static unsigned char env_flags;
 
-int env_import_redund(const char *buf1, int buf1_read_fail,
-		      const char *buf2, int buf2_read_fail,
-		      int flags)
+int env_check_redund(const char *buf1, int buf1_read_fail,
+		     const char *buf2, int buf2_read_fail)
 {
 	int crc1_ok, crc2_ok;
-	env_t *ep, *tmp_env1, *tmp_env2;
+	env_t *tmp_env1, *tmp_env2;
 
 	tmp_env1 = (env_t *)buf1;
 	tmp_env2 = (env_t *)buf2;
@@ -159,14 +158,13 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
 	}
 
 	if (buf1_read_fail && buf2_read_fail) {
-		env_set_default("bad env area", 0);
 		return -EIO;
 	} else if (!buf1_read_fail && buf2_read_fail) {
 		gd->env_valid = ENV_VALID;
-		return env_import((char *)tmp_env1, 1, flags);
+		return -EINVAL;
 	} else if (buf1_read_fail && !buf2_read_fail) {
 		gd->env_valid = ENV_REDUND;
-		return env_import((char *)tmp_env2, 1, flags);
+		return -ENOENT;
 	}
 
 	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
@@ -175,7 +173,6 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
 			tmp_env2->crc;
 
 	if (!crc1_ok && !crc2_ok) {
-		env_set_default("bad CRC", 0);
 		return -ENOMSG; /* needed for env_load() */
 	} else if (crc1_ok && !crc2_ok) {
 		gd->env_valid = ENV_VALID;
@@ -195,12 +192,37 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
 			gd->env_valid = ENV_VALID;
 	}
 
+	return 0;
+}
+
+int env_import_redund(const char *buf1, int buf1_read_fail,
+		      const char *buf2, int buf2_read_fail,
+		      int flags)
+{
+	env_t *ep;
+	int ret;
+
+	ret = env_check_redund(buf1, buf1_read_fail, buf2, buf2_read_fail);
+
+	if (ret == -EIO) {
+		env_set_default("bad env area", 0);
+		return -EIO;
+	} else if (ret == -EINVAL) {
+		return env_import((char *)buf1, 1, flags);
+	} else if (ret == -ENOENT) {
+		return env_import((char *)buf2, 1, flags);
+	} else if (ret == -ENOMSG) {
+		env_set_default("bad CRC", 0);
+		return -ENOMSG;
+	}
+
 	if (gd->env_valid == ENV_VALID)
-		ep = tmp_env1;
+		ep = (env_t *)buf1;
 	else
-		ep = tmp_env2;
+		ep = (env_t *)buf2;
 
 	env_flags = ep->flags;
+
 	return env_import((char *)ep, 0, flags);
 }
 #endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
diff --git a/include/env.h b/include/env.h
index af405955b0..c15339a93f 100644
--- a/include/env.h
+++ b/include/env.h
@@ -318,6 +318,24 @@ int env_import(const char *buf, int check, int flags);
  */
 int env_export(struct environment_s *env_out);
 
+/**
+ * env_check_redund() - check the two redundant environments
+ *   and find out, which is the valid one.
+ *
+ * @buf1: First environment (struct environemnt_s *)
+ * @buf1_read_fail: 0 if buf1 is valid, non-zero if invalid
+ * @buf2: Second environment (struct environemnt_s *)
+ * @buf2_read_fail: 0 if buf2 is valid, non-zero if invalid
+ * @return 0 if OK,
+ *	-EIO if no environment is valid,
+ *	-EINVAL if read of second entry is good
+ *	-ENOENT if read of first entry is good
+ *	-ENOMSG if the CRC was bad
+ */
+
+int env_check_redund(const char *buf1, int buf1_read_fail,
+		     const char *buf2, int buf2_read_fail);
+
 /**
  * env_import_redund() - Select and import one of two redundant environments
  *
-- 
2.25.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
  2020-10-10  8:28 [PATCH v4 0/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
  2020-10-10  8:28 ` [PATCH v4 1/3] env: split env_import_redund() into 2 functions Heiko Schocher
@ 2020-10-10  8:28 ` Heiko Schocher
  2020-10-12  3:35   ` Simon Glass
                     ` (2 more replies)
  2020-10-10  8:28 ` [PATCH v4 3/3] imx6: enable early spi environment access on aristainetos boards Heiko Schocher
  2 siblings, 3 replies; 13+ messages in thread
From: Heiko Schocher @ 2020-10-10  8:28 UTC (permalink / raw)
  To: u-boot

Enable the new Kconfig option ENV_SPI_EARLY if you want
to use Environment in SPI flash before relocation.
Call env_init() and than you can use env_get_f() for
accessing Environment variables.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

Changes in v4:
- rebased to current master 5dcf7cc590

Changes in v3:
- env_sf_init_early() always return 0 now. If we do not return
  0 in this function, env_set_inited() never get called,
  which has the consequence that env_load/save/erase never
  work, because they check if the init bit is set.
- add comment from Simon Glass
  - add missing function comments
  - use if(IS_ENABLED...)
  - drop extra brackets
  - let env_sf_init() decide, which function to call
    add comment that it is necessary to return env_sf_init()
    with 0.

 env/Kconfig |   8 +++++
 env/sf.c    | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index c6ba08878d..393b191a30 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -376,6 +376,14 @@ config ENV_SPI_MODE
 	  Value of the SPI work mode for environment.
 	  See include/spi.h for value.
 
+config ENV_SPI_EARLY
+	bool "Access Environment in SPI flashes before relocation"
+	depends on ENV_IS_IN_SPI_FLASH
+	help
+	  Enable this if you want to use Environment in SPI flash
+	  before relocation. Call env_init() and than you can use
+	  env_get_f() for accessing Environment variables.
+
 config ENV_IS_IN_UBI
 	bool "Environment in a UBI volume"
 	depends on !CHAIN_OF_TRUST
diff --git a/env/sf.c b/env/sf.c
index 937778aa37..2eb2de1a4e 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void)
 #endif
 
 #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
-static int env_sf_init(void)
+/*
+ * check if Environment on CONFIG_ENV_ADDR is valid.
+ */
+static int env_sf_init_addr(void)
 {
 	env_t *env_ptr = (env_t *)env_sf_get_env_addr();
 
@@ -303,12 +306,103 @@ static int env_sf_init(void)
 }
 #endif
 
+#if defined(CONFIG_ENV_SPI_EARLY)
+/*
+ * early load environment from SPI flash (before relocation)
+ * and check if it is valid.
+ */
+static int env_sf_init_early(void)
+{
+	int ret;
+	int read1_fail;
+	int read2_fail;
+	int crc1_ok;
+	env_t *tmp_env2 = NULL;
+	env_t *tmp_env1;
+
+	/*
+	 * if malloc is not ready yet, we cannot use
+	 * this part yet.
+	 */
+	if (!gd->malloc_limit)
+		return -ENOENT;
+
+	tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+			CONFIG_ENV_SIZE);
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+		tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+					     CONFIG_ENV_SIZE);
+
+	if (!tmp_env1 || !tmp_env2)
+		goto out;
+
+	ret = setup_flash_device();
+	if (ret)
+		goto out;
+
+	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
+				    CONFIG_ENV_SIZE, tmp_env1);
+
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
+		read2_fail = spi_flash_read(env_flash,
+					    CONFIG_ENV_OFFSET_REDUND,
+					    CONFIG_ENV_SIZE, tmp_env2);
+		ret = env_check_redund((char *)tmp_env1, read1_fail,
+				       (char *)tmp_env2, read2_fail);
+
+		if (ret == -EIO || ret == -ENOMSG)
+			goto err_read;
+
+		if (gd->env_valid == ENV_VALID)
+			gd->env_addr = (unsigned long)&tmp_env1->data;
+		else
+			gd->env_addr = (unsigned long)&tmp_env2->data;
+	} else {
+		if (read1_fail)
+			goto err_read;
+
+		crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
+				tmp_env1->crc;
+		if (!crc1_ok)
+			goto err_read;
+
+		/* if valid -> this is our env */
+		gd->env_valid = ENV_VALID;
+		gd->env_addr = (unsigned long)&tmp_env1->data;
+	}
+
+	return 0;
+err_read:
+	spi_flash_free(env_flash);
+	env_flash = NULL;
+	free(tmp_env1);
+	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
+		free(tmp_env2);
+out:
+	/* env is not valid. always return 0 */
+	gd->env_valid = ENV_INVALID;
+	return 0;
+}
+#endif
+
+static int env_sf_init(void)
+{
+#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
+	return env_sf_init_addr();
+#elif defined(CONFIG_ENV_SPI_EARLY)
+	return env_sf_init_early();
+#endif
+	/*
+	 * we must return with 0 if there is nothing done,
+	 * else env_set_inited() get not called in env_init()
+	 */
+	return 0;
+}
+
 U_BOOT_ENV_LOCATION(sf) = {
 	.location	= ENVL_SPI_FLASH,
 	ENV_NAME("SPIFlash")
 	.load		= env_sf_load,
 	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL,
-#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
 	.init		= env_sf_init,
-#endif
 };
-- 
2.25.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 3/3] imx6: enable early spi environment access on aristainetos boards
  2020-10-10  8:28 [PATCH v4 0/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
  2020-10-10  8:28 ` [PATCH v4 1/3] env: split env_import_redund() into 2 functions Heiko Schocher
  2020-10-10  8:28 ` [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
@ 2020-10-10  8:28 ` Heiko Schocher
  2020-10-30 18:46   ` Tom Rini
  2 siblings, 1 reply; 13+ messages in thread
From: Heiko Schocher @ 2020-10-10  8:28 UTC (permalink / raw)
  To: u-boot

On aristianetos boards the display type is detected
through "panel" environment variable. Dependend on the
display type we detect the board type and this decides which
DTB we have to use for the board.

So we need early spi environment access.

Signed-off-by: Heiko Schocher <hs@denx.de>

---

(no changes since v3)

Changes in v3:
- new in v3 as aristainetos board DM changes are in
  mainline now

 configs/aristainetos2_defconfig     | 1 +
 configs/aristainetos2b_defconfig    | 1 +
 configs/aristainetos2bcsl_defconfig | 1 +
 configs/aristainetos2c_defconfig    | 1 +
 4 files changed, 4 insertions(+)

diff --git a/configs/aristainetos2_defconfig b/configs/aristainetos2_defconfig
index 241b3fea5e..23e5acec5e 100644
--- a/configs/aristainetos2_defconfig
+++ b/configs/aristainetos2_defconfig
@@ -58,6 +58,7 @@ CONFIG_DTB_RESELECT=y
 CONFIG_MULTI_DTB_FIT=y
 CONFIG_ENV_OVERWRITE=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
+CONFIG_ENV_SPI_EARLY=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_VERSION_VARIABLE=y
diff --git a/configs/aristainetos2b_defconfig b/configs/aristainetos2b_defconfig
index 25a712cfd9..b7530e9ffe 100644
--- a/configs/aristainetos2b_defconfig
+++ b/configs/aristainetos2b_defconfig
@@ -56,6 +56,7 @@ CONFIG_DTB_RESELECT=y
 CONFIG_MULTI_DTB_FIT=y
 CONFIG_ENV_OVERWRITE=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
+CONFIG_ENV_SPI_EARLY=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_VERSION_VARIABLE=y
diff --git a/configs/aristainetos2bcsl_defconfig b/configs/aristainetos2bcsl_defconfig
index 460a5cf0e7..034d2267fc 100644
--- a/configs/aristainetos2bcsl_defconfig
+++ b/configs/aristainetos2bcsl_defconfig
@@ -56,6 +56,7 @@ CONFIG_DTB_RESELECT=y
 CONFIG_MULTI_DTB_FIT=y
 CONFIG_ENV_OVERWRITE=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
+CONFIG_ENV_SPI_EARLY=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_VERSION_VARIABLE=y
diff --git a/configs/aristainetos2c_defconfig b/configs/aristainetos2c_defconfig
index 6c708b8e95..d0f4b99eb0 100644
--- a/configs/aristainetos2c_defconfig
+++ b/configs/aristainetos2c_defconfig
@@ -56,6 +56,7 @@ CONFIG_DTB_RESELECT=y
 CONFIG_MULTI_DTB_FIT=y
 CONFIG_ENV_OVERWRITE=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
+CONFIG_ENV_SPI_EARLY=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_VERSION_VARIABLE=y
-- 
2.25.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 1/3] env: split env_import_redund() into 2 functions
  2020-10-10  8:28 ` [PATCH v4 1/3] env: split env_import_redund() into 2 functions Heiko Schocher
@ 2020-10-12  3:35   ` Simon Glass
  2020-10-30 18:46   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Simon Glass @ 2020-10-12  3:35 UTC (permalink / raw)
  To: u-boot

On Sat, 10 Oct 2020 at 02:28, Heiko Schocher <hs@denx.de> wrote:
>
> split from env_import_redund() the part which checks
> which Environment is valid into a separate function
> called env_check_redund() and call it from env_import_redund().
>
> So env_check_redund() can be used from places which also
> need to do this checks.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
>
> ---
> The return values from env_check_redund() for the cases
> only one read buffer is OK are not perfect, may we should
> find better return codes?
>
> (no changes since v2)
>
> Changes in v2:
> - patch "env: split env_import_redund into 2 functions"
>   is new in version 2. Idea is to not duplicate too much
>   code as Simon suggested.
>
>  env/common.c  | 42 ++++++++++++++++++++++++++++++++----------
>  include/env.h | 18 ++++++++++++++++++
>  2 files changed, 50 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
  2020-10-10  8:28 ` [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
@ 2020-10-12  3:35   ` Simon Glass
  2020-10-30 18:46   ` Tom Rini
  2020-10-30 23:51   ` Michael Walle
  2 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2020-10-12  3:35 UTC (permalink / raw)
  To: u-boot

On Sat, 10 Oct 2020 at 02:28, Heiko Schocher <hs@denx.de> wrote:
>
> Enable the new Kconfig option ENV_SPI_EARLY if you want
> to use Environment in SPI flash before relocation.
> Call env_init() and than you can use env_get_f() for
> accessing Environment variables.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>
> Changes in v4:
> - rebased to current master 5dcf7cc590
>
> Changes in v3:
> - env_sf_init_early() always return 0 now. If we do not return
>   0 in this function, env_set_inited() never get called,
>   which has the consequence that env_load/save/erase never
>   work, because they check if the init bit is set.
> - add comment from Simon Glass
>   - add missing function comments
>   - use if(IS_ENABLED...)
>   - drop extra brackets
>   - let env_sf_init() decide, which function to call
>     add comment that it is necessary to return env_sf_init()
>     with 0.
>
>  env/Kconfig |   8 +++++
>  env/sf.c    | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 105 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 1/3] env: split env_import_redund() into 2 functions
  2020-10-10  8:28 ` [PATCH v4 1/3] env: split env_import_redund() into 2 functions Heiko Schocher
  2020-10-12  3:35   ` Simon Glass
@ 2020-10-30 18:46   ` Tom Rini
  1 sibling, 0 replies; 13+ messages in thread
From: Tom Rini @ 2020-10-30 18:46 UTC (permalink / raw)
  To: u-boot

On Sat, Oct 10, 2020 at 10:28:04AM +0200, Heiko Schocher wrote:

> split from env_import_redund() the part which checks
> which Environment is valid into a separate function
> called env_check_redund() and call it from env_import_redund().
> 
> So env_check_redund() can be used from places which also
> need to do this checks.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201030/c5bbe380/attachment.sig>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
  2020-10-10  8:28 ` [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
  2020-10-12  3:35   ` Simon Glass
@ 2020-10-30 18:46   ` Tom Rini
  2020-10-30 23:51   ` Michael Walle
  2 siblings, 0 replies; 13+ messages in thread
From: Tom Rini @ 2020-10-30 18:46 UTC (permalink / raw)
  To: u-boot

On Sat, Oct 10, 2020 at 10:28:05AM +0200, Heiko Schocher wrote:

> Enable the new Kconfig option ENV_SPI_EARLY if you want
> to use Environment in SPI flash before relocation.
> Call env_init() and than you can use env_get_f() for
> accessing Environment variables.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201030/8618f732/attachment.sig>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 3/3] imx6: enable early spi environment access on aristainetos boards
  2020-10-10  8:28 ` [PATCH v4 3/3] imx6: enable early spi environment access on aristainetos boards Heiko Schocher
@ 2020-10-30 18:46   ` Tom Rini
  0 siblings, 0 replies; 13+ messages in thread
From: Tom Rini @ 2020-10-30 18:46 UTC (permalink / raw)
  To: u-boot

On Sat, Oct 10, 2020 at 10:28:06AM +0200, Heiko Schocher wrote:

> On aristianetos boards the display type is detected
> through "panel" environment variable. Dependend on the
> display type we detect the board type and this decides which
> DTB we have to use for the board.
> 
> So we need early spi environment access.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201030/9b5e88e6/attachment.sig>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
  2020-10-10  8:28 ` [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
  2020-10-12  3:35   ` Simon Glass
  2020-10-30 18:46   ` Tom Rini
@ 2020-10-30 23:51   ` Michael Walle
  2020-10-31  0:07     ` Tom Rini
  2 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-10-30 23:51 UTC (permalink / raw)
  To: u-boot

Hi Heiko, Hi Tom,

Am 2020-10-10 10:28, schrieb Heiko Schocher:
> Enable the new Kconfig option ENV_SPI_EARLY if you want
> to use Environment in SPI flash before relocation.
> Call env_init() and than you can use env_get_f() for
> accessing Environment variables.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> 
> Changes in v4:
> - rebased to current master 5dcf7cc590
> 
> Changes in v3:
> - env_sf_init_early() always return 0 now. If we do not return
>   0 in this function, env_set_inited() never get called,
>   which has the consequence that env_load/save/erase never
>   work, because they check if the init bit is set.
> - add comment from Simon Glass
>   - add missing function comments
>   - use if(IS_ENABLED...)
>   - drop extra brackets
>   - let env_sf_init() decide, which function to call
>     add comment that it is necessary to return env_sf_init()
>     with 0.
> 
>  env/Kconfig |   8 +++++
>  env/sf.c    | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/env/Kconfig b/env/Kconfig
> index c6ba08878d..393b191a30 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -376,6 +376,14 @@ config ENV_SPI_MODE
>  	  Value of the SPI work mode for environment.
>  	  See include/spi.h for value.
> 
> +config ENV_SPI_EARLY
> +	bool "Access Environment in SPI flashes before relocation"
> +	depends on ENV_IS_IN_SPI_FLASH
> +	help
> +	  Enable this if you want to use Environment in SPI flash
> +	  before relocation. Call env_init() and than you can use
> +	  env_get_f() for accessing Environment variables.
> +
>  config ENV_IS_IN_UBI
>  	bool "Environment in a UBI volume"
>  	depends on !CHAIN_OF_TRUST
> diff --git a/env/sf.c b/env/sf.c
> index 937778aa37..2eb2de1a4e 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void)
>  #endif
> 
>  #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> -static int env_sf_init(void)
> +/*
> + * check if Environment on CONFIG_ENV_ADDR is valid.
> + */
> +static int env_sf_init_addr(void)
>  {
>  	env_t *env_ptr = (env_t *)env_sf_get_env_addr();
> 
> @@ -303,12 +306,103 @@ static int env_sf_init(void)
>  }
>  #endif
> 
> +#if defined(CONFIG_ENV_SPI_EARLY)
> +/*
> + * early load environment from SPI flash (before relocation)
> + * and check if it is valid.
> + */
> +static int env_sf_init_early(void)
> +{
> +	int ret;
> +	int read1_fail;
> +	int read2_fail;
> +	int crc1_ok;
> +	env_t *tmp_env2 = NULL;
> +	env_t *tmp_env1;
> +
> +	/*
> +	 * if malloc is not ready yet, we cannot use
> +	 * this part yet.
> +	 */
> +	if (!gd->malloc_limit)
> +		return -ENOENT;
> +
> +	tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> +			CONFIG_ENV_SIZE);
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> +		tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> +					     CONFIG_ENV_SIZE);
> +
> +	if (!tmp_env1 || !tmp_env2)
> +		goto out;
> +
> +	ret = setup_flash_device();
> +	if (ret)
> +		goto out;
> +
> +	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
> +				    CONFIG_ENV_SIZE, tmp_env1);
> +
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
> +		read2_fail = spi_flash_read(env_flash,
> +					    CONFIG_ENV_OFFSET_REDUND,
> +					    CONFIG_ENV_SIZE, tmp_env2);
> +		ret = env_check_redund((char *)tmp_env1, read1_fail,
> +				       (char *)tmp_env2, read2_fail);
> +
> +		if (ret == -EIO || ret == -ENOMSG)
> +			goto err_read;
> +
> +		if (gd->env_valid == ENV_VALID)
> +			gd->env_addr = (unsigned long)&tmp_env1->data;
> +		else
> +			gd->env_addr = (unsigned long)&tmp_env2->data;
> +	} else {
> +		if (read1_fail)
> +			goto err_read;
> +
> +		crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
> +				tmp_env1->crc;
> +		if (!crc1_ok)
> +			goto err_read;
> +
> +		/* if valid -> this is our env */
> +		gd->env_valid = ENV_VALID;
> +		gd->env_addr = (unsigned long)&tmp_env1->data;
> +	}
> +
> +	return 0;
> +err_read:
> +	spi_flash_free(env_flash);
> +	env_flash = NULL;
> +	free(tmp_env1);
> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> +		free(tmp_env2);
> +out:
> +	/* env is not valid. always return 0 */
> +	gd->env_valid = ENV_INVALID;
> +	return 0;
> +}
> +#endif
> +
> +static int env_sf_init(void)
> +{
> +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> +	return env_sf_init_addr();
> +#elif defined(CONFIG_ENV_SPI_EARLY)
> +	return env_sf_init_early();
> +#endif
> +	/*
> +	 * we must return with 0 if there is nothing done,
> +	 * else env_set_inited() get not called in env_init()
> +	 */
> +	return 0;
> +}
> +
>  U_BOOT_ENV_LOCATION(sf) = {
>  	.location	= ENVL_SPI_FLASH,
>  	ENV_NAME("SPIFlash")
>  	.load		= env_sf_load,
>  	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : 
> NULL,
> -#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>  	.init		= env_sf_init,
> -#endif

This will break my board (the new kontron sl28). Before the
change, the environment is loaded from SPI, after this patch
the environment won't be loaded anymore. If I delete the
.init callback, the behavior is the same as before.

The problem here is that returning 0 in env_sf_init() is not
enough because if the .init callback is not set, env_init() will
have ret defaulting to -ENOENT and in this case will load the
default environment and setting env_valid to ENV_VALID. I didn't
follow the whole call chain then. But this will then eventually
lead to loading the actual environment in a later stage.

-michael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
  2020-10-30 23:51   ` Michael Walle
@ 2020-10-31  0:07     ` Tom Rini
  2020-10-31  0:25       ` Michael Walle
  2020-11-02  5:20       ` Heiko Schocher
  0 siblings, 2 replies; 13+ messages in thread
From: Tom Rini @ 2020-10-31  0:07 UTC (permalink / raw)
  To: u-boot

On Sat, Oct 31, 2020 at 12:51:27AM +0100, Michael Walle wrote:
> Hi Heiko, Hi Tom,
> 
> Am 2020-10-10 10:28, schrieb Heiko Schocher:
> > Enable the new Kconfig option ENV_SPI_EARLY if you want
> > to use Environment in SPI flash before relocation.
> > Call env_init() and than you can use env_get_f() for
> > accessing Environment variables.
> > 
> > Signed-off-by: Heiko Schocher <hs@denx.de>
> > ---
> > 
> > Changes in v4:
> > - rebased to current master 5dcf7cc590
> > 
> > Changes in v3:
> > - env_sf_init_early() always return 0 now. If we do not return
> >   0 in this function, env_set_inited() never get called,
> >   which has the consequence that env_load/save/erase never
> >   work, because they check if the init bit is set.
> > - add comment from Simon Glass
> >   - add missing function comments
> >   - use if(IS_ENABLED...)
> >   - drop extra brackets
> >   - let env_sf_init() decide, which function to call
> >     add comment that it is necessary to return env_sf_init()
> >     with 0.
> > 
> >  env/Kconfig |   8 +++++
> >  env/sf.c    | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 105 insertions(+), 3 deletions(-)
> > 
> > diff --git a/env/Kconfig b/env/Kconfig
> > index c6ba08878d..393b191a30 100644
> > --- a/env/Kconfig
> > +++ b/env/Kconfig
> > @@ -376,6 +376,14 @@ config ENV_SPI_MODE
> >  	  Value of the SPI work mode for environment.
> >  	  See include/spi.h for value.
> > 
> > +config ENV_SPI_EARLY
> > +	bool "Access Environment in SPI flashes before relocation"
> > +	depends on ENV_IS_IN_SPI_FLASH
> > +	help
> > +	  Enable this if you want to use Environment in SPI flash
> > +	  before relocation. Call env_init() and than you can use
> > +	  env_get_f() for accessing Environment variables.
> > +
> >  config ENV_IS_IN_UBI
> >  	bool "Environment in a UBI volume"
> >  	depends on !CHAIN_OF_TRUST
> > diff --git a/env/sf.c b/env/sf.c
> > index 937778aa37..2eb2de1a4e 100644
> > --- a/env/sf.c
> > +++ b/env/sf.c
> > @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void)
> >  #endif
> > 
> >  #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> > -static int env_sf_init(void)
> > +/*
> > + * check if Environment on CONFIG_ENV_ADDR is valid.
> > + */
> > +static int env_sf_init_addr(void)
> >  {
> >  	env_t *env_ptr = (env_t *)env_sf_get_env_addr();
> > 
> > @@ -303,12 +306,103 @@ static int env_sf_init(void)
> >  }
> >  #endif
> > 
> > +#if defined(CONFIG_ENV_SPI_EARLY)
> > +/*
> > + * early load environment from SPI flash (before relocation)
> > + * and check if it is valid.
> > + */
> > +static int env_sf_init_early(void)
> > +{
> > +	int ret;
> > +	int read1_fail;
> > +	int read2_fail;
> > +	int crc1_ok;
> > +	env_t *tmp_env2 = NULL;
> > +	env_t *tmp_env1;
> > +
> > +	/*
> > +	 * if malloc is not ready yet, we cannot use
> > +	 * this part yet.
> > +	 */
> > +	if (!gd->malloc_limit)
> > +		return -ENOENT;
> > +
> > +	tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> > +			CONFIG_ENV_SIZE);
> > +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> > +		tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
> > +					     CONFIG_ENV_SIZE);
> > +
> > +	if (!tmp_env1 || !tmp_env2)
> > +		goto out;
> > +
> > +	ret = setup_flash_device();
> > +	if (ret)
> > +		goto out;
> > +
> > +	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
> > +				    CONFIG_ENV_SIZE, tmp_env1);
> > +
> > +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
> > +		read2_fail = spi_flash_read(env_flash,
> > +					    CONFIG_ENV_OFFSET_REDUND,
> > +					    CONFIG_ENV_SIZE, tmp_env2);
> > +		ret = env_check_redund((char *)tmp_env1, read1_fail,
> > +				       (char *)tmp_env2, read2_fail);
> > +
> > +		if (ret == -EIO || ret == -ENOMSG)
> > +			goto err_read;
> > +
> > +		if (gd->env_valid == ENV_VALID)
> > +			gd->env_addr = (unsigned long)&tmp_env1->data;
> > +		else
> > +			gd->env_addr = (unsigned long)&tmp_env2->data;
> > +	} else {
> > +		if (read1_fail)
> > +			goto err_read;
> > +
> > +		crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
> > +				tmp_env1->crc;
> > +		if (!crc1_ok)
> > +			goto err_read;
> > +
> > +		/* if valid -> this is our env */
> > +		gd->env_valid = ENV_VALID;
> > +		gd->env_addr = (unsigned long)&tmp_env1->data;
> > +	}
> > +
> > +	return 0;
> > +err_read:
> > +	spi_flash_free(env_flash);
> > +	env_flash = NULL;
> > +	free(tmp_env1);
> > +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
> > +		free(tmp_env2);
> > +out:
> > +	/* env is not valid. always return 0 */
> > +	gd->env_valid = ENV_INVALID;
> > +	return 0;
> > +}
> > +#endif
> > +
> > +static int env_sf_init(void)
> > +{
> > +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> > +	return env_sf_init_addr();
> > +#elif defined(CONFIG_ENV_SPI_EARLY)
> > +	return env_sf_init_early();
> > +#endif
> > +	/*
> > +	 * we must return with 0 if there is nothing done,
> > +	 * else env_set_inited() get not called in env_init()
> > +	 */
> > +	return 0;
> > +}
> > +
> >  U_BOOT_ENV_LOCATION(sf) = {
> >  	.location	= ENVL_SPI_FLASH,
> >  	ENV_NAME("SPIFlash")
> >  	.load		= env_sf_load,
> >  	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) :
> > NULL,
> > -#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> >  	.init		= env_sf_init,
> > -#endif
> 
> This will break my board (the new kontron sl28). Before the
> change, the environment is loaded from SPI, after this patch
> the environment won't be loaded anymore. If I delete the
> .init callback, the behavior is the same as before.
> 
> The problem here is that returning 0 in env_sf_init() is not
> enough because if the .init callback is not set, env_init() will
> have ret defaulting to -ENOENT and in this case will load the
> default environment and setting env_valid to ENV_VALID. I didn't
> follow the whole call chain then. But this will then eventually
> lead to loading the actual environment in a later stage.

Ugh.  The games we play in the env area really just need to be
rewritten.  So at this point I've merged the series, should I revert or
do you see an easy fix for your platform?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201030/1e3897bd/attachment.sig>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
  2020-10-31  0:07     ` Tom Rini
@ 2020-10-31  0:25       ` Michael Walle
  2020-11-02  5:20       ` Heiko Schocher
  1 sibling, 0 replies; 13+ messages in thread
From: Michael Walle @ 2020-10-31  0:25 UTC (permalink / raw)
  To: u-boot

Hi,

Am 2020-10-31 01:07, schrieb Tom Rini:
[..]
>> > +static int env_sf_init(void)
>> > +{
>> > +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>> > +	return env_sf_init_addr();
>> > +#elif defined(CONFIG_ENV_SPI_EARLY)
>> > +	return env_sf_init_early();
>> > +#endif
>> > +	/*
>> > +	 * we must return with 0 if there is nothing done,
>> > +	 * else env_set_inited() get not called in env_init()
>> > +	 */
>> > +	return 0;
>> > +}
>> > +
>> >  U_BOOT_ENV_LOCATION(sf) = {
>> >  	.location	= ENVL_SPI_FLASH,
>> >  	ENV_NAME("SPIFlash")
>> >  	.load		= env_sf_load,
>> >  	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) :
>> > NULL,
>> > -#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>> >  	.init		= env_sf_init,
>> > -#endif
>> 
>> This will break my board (the new kontron sl28). Before the
>> change, the environment is loaded from SPI, after this patch
>> the environment won't be loaded anymore. If I delete the
>> .init callback, the behavior is the same as before.
>> 
>> The problem here is that returning 0 in env_sf_init() is not
>> enough because if the .init callback is not set, env_init() will
>> have ret defaulting to -ENOENT and in this case will load the
>> default environment and setting env_valid to ENV_VALID. I didn't
>> follow the whole call chain then. But this will then eventually
>> lead to loading the actual environment in a later stage.
> 
> Ugh.  The games we play in the env area really just need to be
> rewritten.  So at this point I've merged the series, should I revert or
> do you see an easy fix for your platform?  Thanks!

Mh, my board cannot be the only one with a late environment
from SPI flash, can it?

Returning 0 will call env_set_inited() and returning -ENOENT
will call the second part. I can't tell why it was done that
way. So I don't have a simple fix.

But I guess the following ugly ifdeffery could do it:

#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || 
defined(CONFIG_ENV_SPI_EARLY)
	.init		= env_sf_init,
#endif

I can try that tomorrow. Also the comment about the return
value should be removed (?).

-michael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation
  2020-10-31  0:07     ` Tom Rini
  2020-10-31  0:25       ` Michael Walle
@ 2020-11-02  5:20       ` Heiko Schocher
  1 sibling, 0 replies; 13+ messages in thread
From: Heiko Schocher @ 2020-11-02  5:20 UTC (permalink / raw)
  To: u-boot

Hello Tom, Michael,

Am 31.10.2020 um 01:07 schrieb Tom Rini:
> On Sat, Oct 31, 2020 at 12:51:27AM +0100, Michael Walle wrote:
>> Hi Heiko, Hi Tom,
>>
>> Am 2020-10-10 10:28, schrieb Heiko Schocher:
>>> Enable the new Kconfig option ENV_SPI_EARLY if you want
>>> to use Environment in SPI flash before relocation.
>>> Call env_init() and than you can use env_get_f() for
>>> accessing Environment variables.
>>>
>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>> ---
>>>
>>> Changes in v4:
>>> - rebased to current master 5dcf7cc590
>>>
>>> Changes in v3:
>>> - env_sf_init_early() always return 0 now. If we do not return
>>>    0 in this function, env_set_inited() never get called,
>>>    which has the consequence that env_load/save/erase never
>>>    work, because they check if the init bit is set.
>>> - add comment from Simon Glass
>>>    - add missing function comments
>>>    - use if(IS_ENABLED...)
>>>    - drop extra brackets
>>>    - let env_sf_init() decide, which function to call
>>>      add comment that it is necessary to return env_sf_init()
>>>      with 0.
>>>
>>>   env/Kconfig |   8 +++++
>>>   env/sf.c    | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>   2 files changed, 105 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/env/Kconfig b/env/Kconfig
>>> index c6ba08878d..393b191a30 100644
>>> --- a/env/Kconfig
>>> +++ b/env/Kconfig
>>> @@ -376,6 +376,14 @@ config ENV_SPI_MODE
>>>   	  Value of the SPI work mode for environment.
>>>   	  See include/spi.h for value.
>>>
>>> +config ENV_SPI_EARLY
>>> +	bool "Access Environment in SPI flashes before relocation"
>>> +	depends on ENV_IS_IN_SPI_FLASH
>>> +	help
>>> +	  Enable this if you want to use Environment in SPI flash
>>> +	  before relocation. Call env_init() and than you can use
>>> +	  env_get_f() for accessing Environment variables.
>>> +
>>>   config ENV_IS_IN_UBI
>>>   	bool "Environment in a UBI volume"
>>>   	depends on !CHAIN_OF_TRUST
>>> diff --git a/env/sf.c b/env/sf.c
>>> index 937778aa37..2eb2de1a4e 100644
>>> --- a/env/sf.c
>>> +++ b/env/sf.c
>>> @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void)
>>>   #endif
>>>
>>>   #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>>> -static int env_sf_init(void)
>>> +/*
>>> + * check if Environment on CONFIG_ENV_ADDR is valid.
>>> + */
>>> +static int env_sf_init_addr(void)
>>>   {
>>>   	env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>
>>> @@ -303,12 +306,103 @@ static int env_sf_init(void)
>>>   }
>>>   #endif
>>>
>>> +#if defined(CONFIG_ENV_SPI_EARLY)
>>> +/*
>>> + * early load environment from SPI flash (before relocation)
>>> + * and check if it is valid.
>>> + */
>>> +static int env_sf_init_early(void)
>>> +{
>>> +	int ret;
>>> +	int read1_fail;
>>> +	int read2_fail;
>>> +	int crc1_ok;
>>> +	env_t *tmp_env2 = NULL;
>>> +	env_t *tmp_env1;
>>> +
>>> +	/*
>>> +	 * if malloc is not ready yet, we cannot use
>>> +	 * this part yet.
>>> +	 */
>>> +	if (!gd->malloc_limit)
>>> +		return -ENOENT;
>>> +
>>> +	tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
>>> +			CONFIG_ENV_SIZE);
>>> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
>>> +		tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
>>> +					     CONFIG_ENV_SIZE);
>>> +
>>> +	if (!tmp_env1 || !tmp_env2)
>>> +		goto out;
>>> +
>>> +	ret = setup_flash_device();
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
>>> +				    CONFIG_ENV_SIZE, tmp_env1);
>>> +
>>> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {
>>> +		read2_fail = spi_flash_read(env_flash,
>>> +					    CONFIG_ENV_OFFSET_REDUND,
>>> +					    CONFIG_ENV_SIZE, tmp_env2);
>>> +		ret = env_check_redund((char *)tmp_env1, read1_fail,
>>> +				       (char *)tmp_env2, read2_fail);
>>> +
>>> +		if (ret == -EIO || ret == -ENOMSG)
>>> +			goto err_read;
>>> +
>>> +		if (gd->env_valid == ENV_VALID)
>>> +			gd->env_addr = (unsigned long)&tmp_env1->data;
>>> +		else
>>> +			gd->env_addr = (unsigned long)&tmp_env2->data;
>>> +	} else {
>>> +		if (read1_fail)
>>> +			goto err_read;
>>> +
>>> +		crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
>>> +				tmp_env1->crc;
>>> +		if (!crc1_ok)
>>> +			goto err_read;
>>> +
>>> +		/* if valid -> this is our env */
>>> +		gd->env_valid = ENV_VALID;
>>> +		gd->env_addr = (unsigned long)&tmp_env1->data;
>>> +	}
>>> +
>>> +	return 0;
>>> +err_read:
>>> +	spi_flash_free(env_flash);
>>> +	env_flash = NULL;
>>> +	free(tmp_env1);
>>> +	if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))
>>> +		free(tmp_env2);
>>> +out:
>>> +	/* env is not valid. always return 0 */
>>> +	gd->env_valid = ENV_INVALID;
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>> +static int env_sf_init(void)
>>> +{
>>> +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>>> +	return env_sf_init_addr();
>>> +#elif defined(CONFIG_ENV_SPI_EARLY)
>>> +	return env_sf_init_early();
>>> +#endif
>>> +	/*
>>> +	 * we must return with 0 if there is nothing done,
>>> +	 * else env_set_inited() get not called in env_init()
>>> +	 */
>>> +	return 0;
>>> +}
>>> +
>>>   U_BOOT_ENV_LOCATION(sf) = {
>>>   	.location	= ENVL_SPI_FLASH,
>>>   	ENV_NAME("SPIFlash")
>>>   	.load		= env_sf_load,
>>>   	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) :
>>> NULL,
>>> -#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>>>   	.init		= env_sf_init,
>>> -#endif
>>
>> This will break my board (the new kontron sl28). Before the
>> change, the environment is loaded from SPI, after this patch
>> the environment won't be loaded anymore. If I delete the
>> .init callback, the behavior is the same as before.
>>
>> The problem here is that returning 0 in env_sf_init() is not
>> enough because if the .init callback is not set, env_init() will
>> have ret defaulting to -ENOENT and in this case will load the
>> default environment and setting env_valid to ENV_VALID. I didn't
>> follow the whole call chain then. But this will then eventually
>> lead to loading the actual environment in a later stage.
> 
> Ugh.  The games we play in the env area really just need to be
> rewritten.  So at this point I've merged the series, should I revert or
> do you see an easy fix for your platform?  Thanks!
> 

Autsch ... sorry ...

Yes, env code is really ugly ...

env/env.c:
323 int env_init(void)
324 {
325         struct env_driver *drv;
326         int ret = -ENOENT;
327         int prio;
328
329         for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
330                 if (!drv->init || !(ret = drv->init()))
331                         env_set_inited(drv->location);
332
333                 debug("%s: Environment %s init done (ret=%d)\n", __func__,
334                       drv->name, ret);
335         }
336
337         if (!prio)
338                 return -ENODEV;
339
340         if (ret == -ENOENT) {
341                 gd->env_addr = (ulong)&default_environment[0];
342                 gd->env_valid = ENV_VALID;
343
344                 return 0;
345         }
346
347         return ret;

So if now the init function returns 0, env_set_inited() sets the init
bit, but

gd->env_valid = ENV_VALID;

never set ... which leads in the error Michael see... as in

env/common.c
230 void env_relocate(void)
[...]
237         if (gd->env_valid == ENV_INVALID) {
238 #if defined(CONFIG_ENV_IS_NOWHERE) || defined(CONFIG_SPL_BUILD)
239                 /* Environment not changable */
240                 env_set_default(NULL, 0);
241 #else
242                 bootstage_error(BOOTSTAGE_ID_NET_CHECKSUM);
243                 env_set_default("bad CRC", 0);
244 #endif
245         } else {
246                 env_load();
247         }

env_load() never called ... on the other hand in env_load()

181 int env_load(void)
182 {
183         struct env_driver *drv;
184         int best_prio = -1;
185         int prio;
186
187         for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {
188                 int ret;
189
190                 if (!env_has_inited(drv->location))
191                         continue;

if the init bit is not set, it never loads from storage device here.

:-(


So I tend to say, we must prevent registering ".init" with some
ifdeffery ... as I had it in v1 of this series ...

:-(

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-11-02  5:20 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10  8:28 [PATCH v4 0/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
2020-10-10  8:28 ` [PATCH v4 1/3] env: split env_import_redund() into 2 functions Heiko Schocher
2020-10-12  3:35   ` Simon Glass
2020-10-30 18:46   ` Tom Rini
2020-10-10  8:28 ` [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation Heiko Schocher
2020-10-12  3:35   ` Simon Glass
2020-10-30 18:46   ` Tom Rini
2020-10-30 23:51   ` Michael Walle
2020-10-31  0:07     ` Tom Rini
2020-10-31  0:25       ` Michael Walle
2020-11-02  5:20       ` Heiko Schocher
2020-10-10  8:28 ` [PATCH v4 3/3] imx6: enable early spi environment access on aristainetos boards Heiko Schocher
2020-10-30 18:46   ` Tom Rini

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.