Hello Simon,
Am 10.12.2019 um 13:39 schrieb Simon Glass:
Hi Heiko,
On Fri, 15 Nov 2019 at 00:28, Heiko Schocher <[email protected]> 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 <[email protected]>
---
Changes in v2:
- 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.
env/Kconfig | 8 ++++++
env/sf.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)
diff --git a/env/Kconfig b/env/Kconfig
index 090cc795f9..76e4f30839 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -370,6 +370,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 590d0cedd8..be2e7fc01c 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -308,6 +308,85 @@ static int env_sf_init(void)
}
#endif
+#if defined(CONFIG_ENV_SPI_EARLY)
+static int env_sf_init_early(void)
Function comment to explain what it does?
Ok, added.
+{
+ int ret;
+ int read1_fail;
+#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
+ int read2_fail;
+#else
+ int crc1_ok;
+#endif
+
+ /*
+ * if malloc is not ready yet, we cannot use
+ * this part yet.
+ */
+ if (!gd->malloc_limit)
+ return -ENOENT;
+
+ env_t *tmp_env2 = NULL;
+ env_t *tmp_env1;
+
+ tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+ CONFIG_ENV_SIZE);
+#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
Can the #ifdefs in this function be if(IS_ENABLED...) instead?
I think yes.
+ tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
+ CONFIG_ENV_SIZE);
+#endif
+ 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);
+#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
+ read2_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET_REDUND,
+ CONFIG_ENV_SIZE, tmp_env2);
Hmm... if porting to "if(IS_ENABLED...)" may here is CONFIG_ENV_OFFSET_REDUND
not defined ... I will see on world build ...
+ ret = env_check_redund((char *)tmp_env1, read1_fail,
+ (char *)tmp_env2, read2_fail);
+
+ if ((ret == -EIO) || (ret == -ENOMSG))
Can drop extra brackets
Without this bracket I get:
/home/hs/data/Entwicklung/abb/uboot-rework/u-boot/env/sf.c: In function
‘env_sf_init_early’:
/home/hs/data/Entwicklung/abb/uboot-rework/u-boot/env/sf.c:355:20: error: expected expression before
‘||’ token
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;
+#endif
+
+ return 0;
+err_read:
+ spi_flash_free(env_flash);
+ env_flash = NULL;
+ free(tmp_env1);
+#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
Unrelated to your patch, but should that be REDUNDANT?
Hmm.. currently the define is REDUNDAND ... but you are correct ...
+ free(tmp_env2);
+#endif
+out:
+ /* env is not valid. always return 0 */
+ gd->env_valid = ENV_INVALID;
+ return 0;
+}
+#endif
+
U_BOOT_ENV_LOCATION(sf) = {
.location = ENVL_SPI_FLASH,
ENV_NAME("SPI Flash")
@@ -317,5 +396,7 @@ U_BOOT_ENV_LOCATION(sf) = {
#endif
#if defined(INITENV) && defined(CONFIG_ENV_ADDR)
.init = env_sf_init,
+#elif defined(CONFIG_ENV_SPI_EARLY)
+ .init = env_sf_init_early,
#endif
It seems like we should have two drivers here, only one of which is
enabled. Perhaps for testing sandbox would want to enable both?
Alternatively I think env_sf_init() should decide which init function to call.
Ok, I can change this, but this leads into change, that we now always
call ".init" ...
in env/env.c env_init():
for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
if (!drv->init || !(ret = drv->init()))
env_set_inited(drv->location);
So, if env_sf_init() return 0 in case nothing ToDo, it is OK to change
this. I added a comment in env_sf_init()
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: [email protected]
