From 4da7347d85aa4409945201279d71ae501cd5c164 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 14 Apr 2021 20:51:42 +0200 Subject: [PATCH 01/11] env/sf.c: use a variable to hold the sector size As preparation for the next patch, use a local variable to represent the sector size. No functional change. Signed-off-by: Rasmus Villemoes --- env/sf.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/env/sf.c b/env/sf.c index 88ec1108b6..d9ed08a78e 100644 --- a/env/sf.c +++ b/env/sf.c @@ -73,6 +73,7 @@ static int env_sf_save(void) env_t env_new; char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; u32 saved_size, saved_offset, sector; + u32 sect_size = CONFIG_ENV_SECT_SIZE; int ret; ret = setup_flash_device(); @@ -93,8 +94,8 @@ static int env_sf_save(void) } /* Is the sector larger than the env (i.e. embedded) */ - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; + if (sect_size > CONFIG_ENV_SIZE) { + saved_size = sect_size - CONFIG_ENV_SIZE; saved_offset = env_new_offset + CONFIG_ENV_SIZE; saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); if (!saved_buffer) { @@ -107,11 +108,11 @@ static int env_sf_save(void) goto done; } - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); + sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, sect_size); puts("Erasing SPI flash..."); ret = spi_flash_erase(env_flash, env_new_offset, - sector * CONFIG_ENV_SECT_SIZE); + sector * sect_size); if (ret) goto done; @@ -122,7 +123,7 @@ static int env_sf_save(void) if (ret) goto done; - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { + if (sect_size > CONFIG_ENV_SIZE) { ret = spi_flash_write(env_flash, saved_offset, saved_size, saved_buffer); if (ret) @@ -187,6 +188,7 @@ out: static int env_sf_save(void) { u32 saved_size, saved_offset, sector; + u32 sect_size = CONFIG_ENV_SECT_SIZE; char *saved_buffer = NULL; int ret = 1; env_t env_new; @@ -196,8 +198,8 @@ static int env_sf_save(void) return ret; /* Is the sector larger than the env (i.e. embedded) */ - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; + if (sect_size > CONFIG_ENV_SIZE) { + saved_size = sect_size - CONFIG_ENV_SIZE; saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; saved_buffer = malloc(saved_size); if (!saved_buffer) @@ -213,11 +215,11 @@ static int env_sf_save(void) if (ret) goto done; - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); + sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, sect_size); puts("Erasing SPI flash..."); ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, - sector * CONFIG_ENV_SECT_SIZE); + sector * sect_size); if (ret) goto done; @@ -227,7 +229,7 @@ static int env_sf_save(void) if (ret) goto done; - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { + if (sect_size > CONFIG_ENV_SIZE) { ret = spi_flash_write(env_flash, saved_offset, saved_size, saved_buffer); if (ret) From bcb44f62b21e88cc74bc26939eb1dac95d2f430b Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 14 Apr 2021 20:51:43 +0200 Subject: [PATCH 02/11] env: add CONFIG_ENV_SECT_SIZE_AUTO This is roughly the U-Boot side equivalent to commit e282c422e0 (tools: fw_env: use erasesize from MEMGETINFO ioctl). The motivation is the case where one has a board with several revisions, where the SPI flashes have different erase sizes. In our case, we have an 8K environment, and the flashes have erase sizes of 4K (newer boards) and 64K (older boards). Currently, we must set CONFIG_ENV_SECT_SIZE to 64K to make the code work on the older boards, but for the newer ones, that ends up wasting quite a bit of time reading/erasing/restoring the last 56K. At first, I wanted to allow setting CONFIG_ENV_SECT_SIZE to 0 to mean "use the erase size the chip reports", but that config option is used in a number of preprocessor conditionals, and shared between ENV_IS_IN_FLASH and ENV_IS_IN_SPI_FLASH. So instead, introduce a new boolean config option, which for now can only be used with ENV_IS_IN_SPI_FLASH. If left off, there's no change in behaviour. The only slightly annoying detail is that, when selected, the compiler is apparently not smart enough to see that the the saved_size and saved_offset variables are only used under the same "if (sect_size > CONFIG_ENV_SIZE)" condition as where they are computed, so we need to initialize them to 0 to avoid "may be used uninitialized" warnings. On our newer boards with the 4K erase size, saving the environment now takes 0.080 seconds instead of 0.53 seconds, which directly translates to that much faster boot time since our logic always causes the environment to be written during boot. Signed-off-by: Rasmus Villemoes --- env/Kconfig | 14 ++++++++++++++ env/sf.c | 10 ++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/env/Kconfig b/env/Kconfig index b473d7cfe1..844c312870 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -324,6 +324,20 @@ config ENV_IS_IN_SPI_FLASH during a "saveenv" operation. CONFIG_ENV_OFFSET_REDUND must be aligned to an erase sector boundary. +config ENV_SECT_SIZE_AUTO + bool "Use automatically detected sector size" + depends on ENV_IS_IN_SPI_FLASH + help + Some boards exist in multiple variants, with different + flashes having different sector sizes. In such cases, you + can select this option to make U-Boot use the actual sector + size when figuring out how much to erase, which can thus be + more efficient on the flashes with smaller erase size. Since + the environment must always be aligned on a sector boundary, + CONFIG_ENV_OFFSET must be aligned to the largest of the + different sector sizes, and CONFIG_ENV_SECT_SIZE should be + set to that value. + config USE_ENV_SPI_BUS bool "SPI flash bus for environment" depends on ENV_IS_IN_SPI_FLASH diff --git a/env/sf.c b/env/sf.c index d9ed08a78e..06cc62e005 100644 --- a/env/sf.c +++ b/env/sf.c @@ -72,7 +72,7 @@ static int env_sf_save(void) { env_t env_new; char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; - u32 saved_size, saved_offset, sector; + u32 saved_size = 0, saved_offset = 0, sector; u32 sect_size = CONFIG_ENV_SECT_SIZE; int ret; @@ -80,6 +80,9 @@ static int env_sf_save(void) if (ret) return ret; + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) + sect_size = env_flash->mtd.erasesize; + ret = env_export(&env_new); if (ret) return -EIO; @@ -187,7 +190,7 @@ out: #else static int env_sf_save(void) { - u32 saved_size, saved_offset, sector; + u32 saved_size = 0, saved_offset = 0, sector; u32 sect_size = CONFIG_ENV_SECT_SIZE; char *saved_buffer = NULL; int ret = 1; @@ -197,6 +200,9 @@ static int env_sf_save(void) if (ret) return ret; + if (IS_ENABLED(CONFIG_ENV_SECT_SIZE_AUTO)) + sect_size = env_flash->mtd.erasesize; + /* Is the sector larger than the env (i.e. embedded) */ if (sect_size > CONFIG_ENV_SIZE) { saved_size = sect_size - CONFIG_ENV_SIZE; From b9c3052fbb25bff26702e6c16abfd0a5ec92040c Mon Sep 17 00:00:00 2001 From: Brandon Maier Date: Thu, 17 Dec 2020 17:19:18 -0600 Subject: [PATCH 03/11] env: increment redund flag on read fail If one of the reads fails when importing redundant environments (a single read failure), the env_flags wouldn't get initialized in env_import_redund(). If a user then calls saveenv, the new environment will have the wrong flags value. So on the next load the new environment will be ignored. While debugging this, I also noticed that env/sf.c was not correctly handling a single read failure, as it would not check the crc before assigning it to gd->env_addr. Having a special error path for when there is a single read failure seems unnecessary and may lead to future bugs. Instead collapse the 'single read failure' error to be the same as a 'single crc failure'. That way env_check_redund() either passes or fails, and if it passes we are guaranteed to have checked the CRC. Signed-off-by: Brandon Maier CC: Joe Hershberger CC: Wolfgang Denk CC: Heiko Schocher Reviewed-by: Tom Rini --- env/common.c | 27 ++++++++------------------- env/sf.c | 2 +- include/env.h | 2 -- 3 files changed, 9 insertions(+), 22 deletions(-) diff --git a/env/common.c b/env/common.c index 2ee423beb5..49bbb05eec 100644 --- a/env/common.c +++ b/env/common.c @@ -145,7 +145,7 @@ static unsigned char env_flags; int env_check_redund(const char *buf1, int buf1_read_fail, const char *buf2, int buf2_read_fail) { - int crc1_ok, crc2_ok; + int crc1_ok = 0, crc2_ok = 0; env_t *tmp_env1, *tmp_env2; tmp_env1 = (env_t *)buf1; @@ -153,25 +153,18 @@ int env_check_redund(const char *buf1, int buf1_read_fail, if (buf1_read_fail && buf2_read_fail) { puts("*** Error - No Valid Environment Area found\n"); + return -EIO; } else if (buf1_read_fail || buf2_read_fail) { puts("*** Warning - some problems detected "); puts("reading environment; recovered successfully\n"); } - if (buf1_read_fail && buf2_read_fail) { - return -EIO; - } else if (!buf1_read_fail && buf2_read_fail) { - gd->env_valid = ENV_VALID; - return -EINVAL; - } else if (buf1_read_fail && !buf2_read_fail) { - gd->env_valid = ENV_REDUND; - return -ENOENT; - } - - crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == - tmp_env1->crc; - crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == - tmp_env2->crc; + if (!buf1_read_fail) + crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == + tmp_env1->crc; + if (!buf2_read_fail) + crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == + tmp_env2->crc; if (!crc1_ok && !crc2_ok) { return -ENOMSG; /* needed for env_load() */ @@ -208,10 +201,6 @@ int env_import_redund(const char *buf1, int buf1_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; diff --git a/env/sf.c b/env/sf.c index 06cc62e005..e13d41478b 100644 --- a/env/sf.c +++ b/env/sf.c @@ -359,7 +359,7 @@ static int env_sf_init_early(void) ret = env_check_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2, read2_fail); - if (ret == -EIO || ret == -ENOMSG) + if (ret < 0) goto err_read; if (gd->env_valid == ENV_VALID) diff --git a/include/env.h b/include/env.h index c15339a93f..b5731e4b9a 100644 --- a/include/env.h +++ b/include/env.h @@ -328,8 +328,6 @@ int env_export(struct environment_s *env_out); * @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 */ From 9636bf8b2e319c0f43453f71131ba70856571d05 Mon Sep 17 00:00:00 2001 From: Martin Fuzzey Date: Mon, 11 Jan 2021 11:27:20 +0100 Subject: [PATCH 04/11] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE Since commit 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set") a warning message is displayed when setenv -f is used WITHOUT CONFIG_ENV_ACCESS_IGNORE_FORCE, but the variable is set anyway, resulting in lots of log pollution. env_flags_validate() returns 0 if the access is accepted, or non zero if it is refused. So the original code #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE if (flag & H_FORCE) return 0; #endif was correct, it returns 0 (accepts the modification) if forced UNLESS IGNORE_FORCE is set (in which case access checks in the following code are applied). The broken patch just added a printf to the force accepted case. To obtain the intent of the patch we need this: if (flag & H_FORCE) { #ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE printf("## Error: Can't force access to \"%s\"\n", name); #else return 0; #endif } Fixes: 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set") Signed-off-by: Martin Fuzzey --- env/flags.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/env/flags.c b/env/flags.c index df4aed26b2..e3e833c433 100644 --- a/env/flags.c +++ b/env/flags.c @@ -563,12 +563,13 @@ int env_flags_validate(const struct env_entry *item, const char *newval, return 1; #endif -#ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE if (flag & H_FORCE) { +#ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE printf("## Error: Can't force access to \"%s\"\n", name); +#else return 0; - } #endif + } switch (op) { case env_op_delete: if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_DELETE) { From 2339f01af6478f93aada713fa33b5e04c3497724 Mon Sep 17 00:00:00 2001 From: Brandon Maier Date: Sat, 16 Jan 2021 15:14:43 -0600 Subject: [PATCH 05/11] env/fat.c: support redund environment Signed-off-by: Brandon Maier CC: Joe Hershberger CC: Wolfgang Denk --- env/Kconfig | 8 ++++++++ env/fat.c | 40 ++++++++++++++++++++++++++++++++-------- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/env/Kconfig b/env/Kconfig index 844c312870..08e49c2a47 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -476,6 +476,14 @@ config ENV_FAT_FILE It's a string of the FAT file name. This file use to store the environment. +config ENV_FAT_FILE_REDUND + string "Name of the FAT file to use for the environment" + depends on ENV_IS_IN_FAT && SYS_REDUNDAND_ENVIRONMENT + default "uboot-redund.env" + help + It's a string of the FAT file name. This file use to store the + redundant environment. + config ENV_EXT4_INTERFACE string "Name of the block device for the environment" depends on ENV_IS_IN_EXT4 diff --git a/env/fat.c b/env/fat.c index 653a38fd93..9d37d26f9e 100644 --- a/env/fat.c +++ b/env/fat.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #ifdef CONFIG_SPL_BUILD @@ -29,6 +30,8 @@ # define LOADENV #endif +DECLARE_GLOBAL_DATA_PTR; + static char *env_fat_device_and_part(void) { #ifdef CONFIG_MMC @@ -53,6 +56,7 @@ static int env_fat_save(void) env_t __aligned(ARCH_DMA_MINALIGN) env_new; struct blk_desc *dev_desc = NULL; struct disk_partition info; + const char *file = CONFIG_ENV_FAT_FILE; int dev, part; int err; loff_t size; @@ -78,29 +82,41 @@ static int env_fat_save(void) return 1; } - err = file_fat_write(CONFIG_ENV_FAT_FILE, (void *)&env_new, 0, sizeof(env_t), - &size); +#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT + if (gd->env_valid == ENV_VALID) + file = CONFIG_ENV_FAT_FILE_REDUND; +#endif + + err = file_fat_write(file, (void *)&env_new, 0, sizeof(env_t), &size); if (err == -1) { /* * This printf is embedded in the messages from env_save that * will calling it. The missing \n is intentional. */ printf("Unable to write \"%s\" from %s%d:%d... ", - CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part); + file, CONFIG_ENV_FAT_INTERFACE, dev, part); return 1; } +#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT + gd->env_valid = (gd->env_valid == ENV_REDUND) ? ENV_VALID : ENV_REDUND; +#endif + return 0; } #ifdef LOADENV static int env_fat_load(void) { - ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE); + ALLOC_CACHE_ALIGN_BUFFER(char, buf1, CONFIG_ENV_SIZE); +#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT + ALLOC_CACHE_ALIGN_BUFFER(char, buf2, CONFIG_ENV_SIZE); + int err2; +#endif struct blk_desc *dev_desc = NULL; struct disk_partition info; int dev, part; - int err; + int err1; #ifdef CONFIG_MMC if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc")) @@ -124,8 +140,15 @@ static int env_fat_load(void) goto err_env_relocate; } - err = file_fat_read(CONFIG_ENV_FAT_FILE, buf, CONFIG_ENV_SIZE); - if (err == -1) { + err1 = file_fat_read(CONFIG_ENV_FAT_FILE, buf1, CONFIG_ENV_SIZE); +#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT + err2 = file_fat_read(CONFIG_ENV_FAT_FILE_REDUND, buf2, CONFIG_ENV_SIZE); + + err1 = (err1 >= 0) ? 0 : -1; + err2 = (err2 >= 0) ? 0 : -1; + return env_import_redund(buf1, err1, buf2, err2, H_EXTERNAL); +#else + if (err1 < 0) { /* * This printf is embedded in the messages from env_save that * will calling it. The missing \n is intentional. @@ -135,7 +158,8 @@ static int env_fat_load(void) goto err_env_relocate; } - return env_import(buf, 1, H_EXTERNAL); + return env_import(buf1, 1, H_EXTERNAL); +#endif err_env_relocate: env_set_default(NULL, 0); From 5557eec01cbfb0e415775434f29542dffb1a4423 Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Wed, 20 Jan 2021 15:45:16 +0100 Subject: [PATCH 06/11] env: Fix invalid env handling in env_init() This fixes the case where there are multiple environment drivers, one of them is the default environment one, and it is followed by an environment driver which does not implement .init() callback. The default environment driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init() callback implementation, which is valid behavior for default environment. Since the subsequent environment driver does not implement .init(), it also does not modify the $ret variable in the loop. Therefore, the loop is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the code further down in env_init() will not reset the environment to the default one, which is incorrect. This patch sets the $ret variable back to -ENOENT in case the env_valid is set to ENV_INVALID by an environment driver, so that the environment would be correctly reset back to default one, unless a subsequent driver loads a valid environment. Signed-off-by: Marek Vasut Tested-By: Tim Harvey --- env/env.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/env/env.c b/env/env.c index caefa33e1d..e534008006 100644 --- a/env/env.c +++ b/env/env.c @@ -335,6 +335,9 @@ int env_init(void) debug("%s: Environment %s init done (ret=%d)\n", __func__, drv->name, ret); + + if (gd->env_valid == ENV_INVALID) + ret = -ENOENT; } if (!prio) From 1af031ac3e3e75ea1ae58da093db956a8c9bc144 Mon Sep 17 00:00:00 2001 From: Patrick Delaunay Date: Tue, 9 Feb 2021 11:48:50 +0100 Subject: [PATCH 07/11] env: add ENV_ERASE_PTR macro Add ENV_ERASE_PTR macro to handle erase opts and remove the associated ifdef. This patch is a extension of previous commit 82b2f4135719 ("env_internal.h: add alternative ENV_SAVE_PTR macro"). Signed-off-by: Patrick Delaunay --- env/ext4.c | 3 +-- env/mmc.c | 6 +----- include/env_internal.h | 1 + 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/env/ext4.c b/env/ext4.c index ec643f2226..9f65afb8a4 100644 --- a/env/ext4.c +++ b/env/ext4.c @@ -188,6 +188,5 @@ U_BOOT_ENV_LOCATION(ext4) = { ENV_NAME("EXT4") .load = env_ext4_load, .save = ENV_SAVE_PTR(env_ext4_save), - .erase = CONFIG_IS_ENABLED(CMD_ERASEENV) ? env_ext4_erase : - NULL, + .erase = ENV_ERASE_PTR(env_ext4_erase), }; diff --git a/env/mmc.c b/env/mmc.c index 9b226be1d5..09e94f0bd3 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -233,7 +233,6 @@ fini: return ret; } -#if defined(CONFIG_CMD_ERASEENV) static inline int erase_env(struct mmc *mmc, unsigned long size, unsigned long offset) { @@ -279,7 +278,6 @@ static int env_mmc_erase(void) return ret; } -#endif /* CONFIG_CMD_ERASEENV */ #endif /* CONFIG_CMD_SAVEENV && !CONFIG_SPL_BUILD */ static inline int read_env(struct mmc *mmc, unsigned long size, @@ -394,8 +392,6 @@ U_BOOT_ENV_LOCATION(mmc) = { .load = env_mmc_load, #ifndef CONFIG_SPL_BUILD .save = env_save_ptr(env_mmc_save), -#if defined(CONFIG_CMD_ERASEENV) - .erase = env_mmc_erase, -#endif + .erase = ENV_ERASE_PTR(env_mmc_erase) #endif }; diff --git a/include/env_internal.h b/include/env_internal.h index 708c833a55..b7bddcb00d 100644 --- a/include/env_internal.h +++ b/include/env_internal.h @@ -211,6 +211,7 @@ struct env_driver { #endif #define ENV_SAVE_PTR(x) (CONFIG_IS_ENABLED(SAVEENV) ? (x) : NULL) +#define ENV_ERASE_PTR(x) (CONFIG_IS_ENABLED(CMD_ERASEENV) ? (x) : NULL) extern struct hsearch_data env_htab; From e41f55b32e0ac38da77d8f86792164faac5ef7c5 Mon Sep 17 00:00:00 2001 From: Patrick Delaunay Date: Tue, 9 Feb 2021 11:48:51 +0100 Subject: [PATCH 08/11] env: sf: update the use of macro ENV_SAVE_PTR Remove CONFIG_IS_ENABLED(SAVEENV) as it is already tested in the ENV_SAVE_PTR macro. Signed-off-by: Patrick Delaunay --- env/sf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/env/sf.c b/env/sf.c index e13d41478b..20358f5c3f 100644 --- a/env/sf.c +++ b/env/sf.c @@ -414,6 +414,6 @@ 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, + .save = ENV_SAVE_PTR(env_sf_save), .init = env_sf_init, }; From 25d90ad45ab336bab6a21f0668b8c98a2939ff32 Mon Sep 17 00:00:00 2001 From: Patrick Delaunay Date: Tue, 9 Feb 2021 11:48:52 +0100 Subject: [PATCH 09/11] env: sf: add support of command env erase Add support of opts erase for env in SPI flash; this opts is used by command 'env erase'. This command only fills the env offset by 0x0 (bit flip to 0) and the saved environment becomes invalid (with bad CRC). It doesn't erase the sector here to avoid issue when the sector is larger than the env (i.e. embedded when CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE). The needed sector erase will be managed in the next "env save" command, using the opt ".save", before to update the environment in SPI flash. Signed-off-by: Patrick Delaunay --- env/sf.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/env/sf.c b/env/sf.c index 20358f5c3f..1c2ab9da9b 100644 --- a/env/sf.c +++ b/env/sf.c @@ -28,9 +28,18 @@ #define INITENV #endif +#define OFFSET_INVALID (~(u32)0) + #ifdef CONFIG_ENV_OFFSET_REDUND +#define ENV_OFFSET_REDUND CONFIG_ENV_OFFSET_REDUND + static ulong env_offset = CONFIG_ENV_OFFSET; static ulong env_new_offset = CONFIG_ENV_OFFSET_REDUND; + +#else + +#define ENV_OFFSET_REDUND OFFSET_INVALID + #endif /* CONFIG_ENV_OFFSET_REDUND */ DECLARE_GLOBAL_DATA_PTR; @@ -288,6 +297,30 @@ out: } #endif +static int env_sf_erase(void) +{ + int ret; + env_t env; + + ret = setup_flash_device(); + if (ret) + return ret; + + memset(&env, 0, sizeof(env_t)); + ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, &env); + if (ret) + goto done; + + if (ENV_OFFSET_REDUND != OFFSET_INVALID) + ret = spi_flash_write(env_flash, ENV_OFFSET_REDUND, CONFIG_ENV_SIZE, &env); + +done: + spi_flash_free(env_flash); + env_flash = NULL; + + return ret; +} + #if CONFIG_ENV_ADDR != 0x0 __weak void *env_sf_get_env_addr(void) { @@ -415,5 +448,6 @@ U_BOOT_ENV_LOCATION(sf) = { ENV_NAME("SPIFlash") .load = env_sf_load, .save = ENV_SAVE_PTR(env_sf_save), + .erase = ENV_ERASE_PTR(env_sf_erase), .init = env_sf_init, }; From c2d00364c1d638eaa85d5f1384170f610b3c4beb Mon Sep 17 00:00:00 2001 From: Patrick Delaunay Date: Wed, 24 Feb 2021 11:52:35 +0100 Subject: [PATCH 10/11] env: sf: add missing spi_flash_free Free the SPI resources by calling spi_flash_free() in each env sf function to avoid issue for other SPI users. Signed-off-by: Patrick Delaunay --- env/sf.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/env/sf.c b/env/sf.c index 1c2ab9da9b..ba5f7919c1 100644 --- a/env/sf.c +++ b/env/sf.c @@ -154,6 +154,9 @@ static int env_sf_save(void) printf("Valid environment: %d\n", (int)gd->env_valid); done: + spi_flash_free(env_flash); + env_flash = NULL; + if (saved_buffer) free(saved_buffer); @@ -255,6 +258,9 @@ static int env_sf_save(void) puts("done\n"); done: + spi_flash_free(env_flash); + env_flash = NULL; + if (saved_buffer) free(saved_buffer); @@ -413,6 +419,9 @@ static int env_sf_init_early(void) gd->env_addr = (unsigned long)&tmp_env1->data; } + spi_flash_free(env_flash); + env_flash = NULL; + return 0; err_read: spi_flash_free(env_flash); From ecf154423258cb4f0d7ee1ff29d26b0f69f67ac4 Mon Sep 17 00:00:00 2001 From: Patrick Delaunay Date: Wed, 24 Feb 2021 11:52:36 +0100 Subject: [PATCH 11/11] env: sf: remove the static env_flash variable As the the SPI flash is probed and is released in each ENV sf function the env_flash no more need to be static. This patch move this device handle as local variable of each function and simplify the associated code (env_flash is never == NULL when setup_flash_device is called). Signed-off-by: Patrick Delaunay --- env/sf.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/env/sf.c b/env/sf.c index ba5f7919c1..e4b7ca9e04 100644 --- a/env/sf.c +++ b/env/sf.c @@ -44,9 +44,7 @@ static ulong env_new_offset = CONFIG_ENV_OFFSET_REDUND; DECLARE_GLOBAL_DATA_PTR; -static struct spi_flash *env_flash; - -static int setup_flash_device(void) +static int setup_flash_device(struct spi_flash **env_flash) { #if CONFIG_IS_ENABLED(DM_SPI_FLASH) struct udevice *new; @@ -61,14 +59,11 @@ static int setup_flash_device(void) return ret; } - env_flash = dev_get_uclass_priv(new); + *env_flash = dev_get_uclass_priv(new); #else - if (env_flash) - spi_flash_free(env_flash); - - env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, - CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); - if (!env_flash) { + *env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, + CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); + if (!*env_flash) { env_set_default("spi_flash_probe() failed", 0); return -EIO; } @@ -84,8 +79,9 @@ static int env_sf_save(void) u32 saved_size = 0, saved_offset = 0, sector; u32 sect_size = CONFIG_ENV_SECT_SIZE; int ret; + struct spi_flash *env_flash; - ret = setup_flash_device(); + ret = setup_flash_device(&env_flash); if (ret) return ret; @@ -155,7 +151,6 @@ static int env_sf_save(void) done: spi_flash_free(env_flash); - env_flash = NULL; if (saved_buffer) free(saved_buffer); @@ -168,6 +163,7 @@ static int env_sf_load(void) int ret; int read1_fail, read2_fail; env_t *tmp_env1, *tmp_env2; + struct spi_flash *env_flash; tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN, CONFIG_ENV_SIZE); @@ -179,7 +175,7 @@ static int env_sf_load(void) goto out; } - ret = setup_flash_device(); + ret = setup_flash_device(&env_flash); if (ret) goto out; @@ -192,7 +188,6 @@ static int env_sf_load(void) read2_fail, H_EXTERNAL); spi_flash_free(env_flash); - env_flash = NULL; out: free(tmp_env1); free(tmp_env2); @@ -207,8 +202,9 @@ static int env_sf_save(void) char *saved_buffer = NULL; int ret = 1; env_t env_new; + struct spi_flash *env_flash; - ret = setup_flash_device(); + ret = setup_flash_device(&env_flash); if (ret) return ret; @@ -259,7 +255,6 @@ static int env_sf_save(void) done: spi_flash_free(env_flash); - env_flash = NULL; if (saved_buffer) free(saved_buffer); @@ -271,6 +266,7 @@ static int env_sf_load(void) { int ret; char *buf = NULL; + struct spi_flash *env_flash; buf = (char *)memalign(ARCH_DMA_MINALIGN, CONFIG_ENV_SIZE); if (!buf) { @@ -278,7 +274,7 @@ static int env_sf_load(void) return -EIO; } - ret = setup_flash_device(); + ret = setup_flash_device(&env_flash); if (ret) goto out; @@ -295,7 +291,6 @@ static int env_sf_load(void) err_read: spi_flash_free(env_flash); - env_flash = NULL; out: free(buf); @@ -307,8 +302,9 @@ static int env_sf_erase(void) { int ret; env_t env; + struct spi_flash *env_flash; - ret = setup_flash_device(); + ret = setup_flash_device(&env_flash); if (ret) return ret; @@ -322,7 +318,6 @@ static int env_sf_erase(void) done: spi_flash_free(env_flash); - env_flash = NULL; return ret; } @@ -367,6 +362,7 @@ static int env_sf_init_early(void) int crc1_ok; env_t *tmp_env2 = NULL; env_t *tmp_env1; + struct spi_flash *env_flash; /* * if malloc is not ready yet, we cannot use @@ -384,7 +380,7 @@ static int env_sf_init_early(void) if (!tmp_env1 || !tmp_env2) goto out; - ret = setup_flash_device(); + ret = setup_flash_device(&env_flash); if (ret) goto out; @@ -420,12 +416,11 @@ static int env_sf_init_early(void) } spi_flash_free(env_flash); - env_flash = NULL; 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);