From mboxrd@z Thu Jan 1 00:00:00 1970 From: Harry Waschkeit Date: Fri, 9 Oct 2020 18:43:22 +0200 Subject: [PATCH] env: sf: add support for env erase In-Reply-To: References: <66c89baf-b52a-81cb-2846-112a7925a256@men.de> Message-ID: <821145d6-a221-96be-9dd7-5123870118d3@men.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Sean, thanks for your try and sorry for the inconvenience my beginner's mistakes have caused :-( It is definitely no good idea to use copy&paste with patch data, I should have guessed that beforehand ... Anyway, concerning the complaints from checkpatch.pl: I indeed ran checkpatch.pl on my patch prior to sending it - the real one, not the one as pasted into the mail ;-/ The alignment warnings simply result from the fact that I adhered to the style used in that file already, you can easily verify that by running checkpatch.pl on the complete file. For the warnings with "IS_ENABLED(CONFIG...)" I don't see what I could do to get around them: all three occurrences are about compiling functions into the code depending on CONFIG_CMD_ERASEENV. Two times it is the new function env_sf_erase(), one variant for normal and the other for redundand environment handling. The third time it is used to define this new method into the structure U_BOOT_ENV_LOCATION(sf). The sign-off problem I guess is probably caused by the check not accepting name in reverse order, isn't it? Anyway, I will change my user.name to match the order in the mail address so next patch is hopefully correct. So please let me know what else I should do beside sending a properly formatted patch ;-/ I will take care of that before resending my patch (v2 then, right?). On 10/9/20 3:55 PM, Sean Anderson wrote: > On 10/8/20 1:27 PM, Harry Waschkeit wrote: >> Command "env erase" didn't work even though CONFIG_CMD_ERASEENV was >> defined, because serial flash environment routines didn't implement >> erase method. >> >> Signed-off-by: Waschkeit, Harry >> --- > > Hi Harry, > > I wanted to test out your patch, but I couldn't get it to apply. It > appears that your mail program has replaced the tabs with spaces, so git > can't figure out how to apply it. I tried to fix it by performing the > substitutions > > s/^\(.\) /\1\t/g > s/ /\t/g > > but it still wouldn't apply. In addition, checkpatch has a few warnings: > >> $ scripts/checkpatch.pl env-sf-add-support-for-env-erase.patch >> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible >> #152: FILE: env/sf.c:149: >> +#ifdef CONFIG_CMD_ERASEENV >> >> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible >> #240: FILE: env/sf.c:318: >> +#ifdef CONFIG_CMD_ERASEENV >> >> CHECK: Alignment should match open parenthesis >> #260: FILE: env/sf.c:338: >> + ret = spi_flash_read(env_flash, saved_offset, >> + saved_size, saved_buffer); >> >> CHECK: Alignment should match open parenthesis >> #269: FILE: env/sf.c:347: >> + ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >> + sector * CONFIG_ENV_SECT_SIZE); >> >> CHECK: Alignment should match open parenthesis >> #276: FILE: env/sf.c:354: >> + ret = spi_flash_write(env_flash, saved_offset, >> + saved_size, saved_buffer); >> >> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible >> #307: FILE: env/sf.c:437: >> +#if defined(CONFIG_CMD_ERASEENV) && defined(CONFIG_ENV_ADDR) >> >> WARNING: Missing Signed-off-by: line by nominal patch author 'Harry Waschkeit ' >> >> total: 0 errors, 4 warnings, 3 checks, 158 lines checked >> >> NOTE: For some of the reported defects, checkpatch may be able to >> mechanically convert to the typical style using --fix or --fix-inplace. >> >> env-sf-add-support-for-env-erase.patch has style problems, please review. >> >> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_ETHER_ADDR_COPY USLEEP_RANGE >> >> NOTE: If any of the errors are false positives, please report >> them to the maintainer, see CHECKPATCH in MAINTAINERS. > > Please fix these issues and resend, thanks! > > --Sean > >> env/sf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 128 insertions(+), 2 deletions(-) >> >> diff --git a/env/sf.c b/env/sf.c >> index 937778aa37..9cda192a73 100644 >> --- a/env/sf.c >> +++ b/env/sf.c >> @@ -146,6 +146,78 @@ static int env_sf_save(void) >> return ret; >> } >> >> +#ifdef CONFIG_CMD_ERASEENV >> +static int env_sf_erase(void) >> +{ >> + char *saved_buffer = NULL; >> + u32 saved_size, saved_offset, sector; >> + ulong offset; >> + ulong offsets[2] = { CONFIG_ENV_OFFSET, CONFIG_ENV_OFFSET_REDUND }; >> + int i; >> + int ret; >> + >> + ret = setup_flash_device(); >> + if (ret) >> + return ret; >> + >> + /* get temporary storage if sector is larger than env (i.e. embedded) */ >> + if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> + saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >> + saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >> + if (!saved_buffer) { >> + ret = -ENOMEM; >> + goto done; >> + } >> + } >> + >> + sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >> + >> + /* simply erase both environments, retaining non-env data (if any) */ >> + for (i = 0; i < ARRAY_SIZE(offsets); i++) { >> + offset = offsets[i]; >> + >> + if (saved_buffer) { >> + saved_offset = offset + CONFIG_ENV_SIZE; >> + ret = spi_flash_read(env_flash, saved_offset, >> + saved_size, saved_buffer); >> + if (ret) >> + goto done; >> + } >> + >> + if (i) >> + puts("Redund:"); >> + >> + puts("Erasing SPI flash..."); >> + ret = spi_flash_erase(env_flash, offset, >> + sector * CONFIG_ENV_SECT_SIZE); >> + if (ret) >> + goto done; >> + >> + if (saved_buffer) { >> + puts("Writing non-environment data to SPI flash..."); >> + ret = spi_flash_write(env_flash, saved_offset, >> + saved_size, saved_buffer); >> + if (ret) >> + goto done; >> + } >> + >> + puts("done\n"); >> + } >> + >> + /* here we know that both env sections are cleared */ >> + env_new_offset = CONFIG_ENV_OFFSET; >> + env_offset = CONFIG_ENV_OFFSET_REDUND; >> + >> + gd->env_valid = ENV_INVALID; >> + >> + done: >> + if (saved_buffer) >> + free(saved_buffer); >> + >> + return ret; >> +} >> +#endif /* CONFIG_CMD_ERASEENV */ >> + >> static int env_sf_load(void) >> { >> int ret; >> @@ -182,7 +254,7 @@ out: >> >> return ret; >> } >> -#else >> +#else /* #if defined(CONFIG_ENV_OFFSET_REDUND) */ >> static int env_sf_save(void) >> { >> u32 saved_size, saved_offset, sector; >> @@ -243,6 +315,57 @@ static int env_sf_save(void) >> return ret; >> } >> >> +#ifdef CONFIG_CMD_ERASEENV >> +static int env_sf_erase(void) >> +{ >> + u32 saved_size, saved_offset, sector; >> + char *saved_buffer = NULL; >> + int ret = 1; >> + >> + ret = setup_flash_device(); >> + if (ret) >> + 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; >> + saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >> + saved_buffer = malloc(saved_size); >> + if (!saved_buffer) >> + goto done; >> + >> + ret = spi_flash_read(env_flash, saved_offset, >> + saved_size, saved_buffer); >> + if (ret) >> + goto done; >> + } >> + >> + sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >> + >> + puts("Erasing SPI flash..."); >> + ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >> + sector * CONFIG_ENV_SECT_SIZE); >> + if (ret) >> + goto done; >> + >> + if (saved_buffer) { >> + puts("Writing non-environment data to SPI flash..."); >> + ret = spi_flash_write(env_flash, saved_offset, >> + saved_size, saved_buffer); >> + if (ret) >> + goto done; >> + } >> + >> + puts("done\n"); >> + >> + done: >> + if (saved_buffer) >> + free(saved_buffer); >> + >> + return ret; >> +} >> +#endif /* CONFIG_CMD_ERASEENV */ >> + >> static int env_sf_load(void) >> { >> int ret; >> @@ -277,7 +400,7 @@ out: >> >> return ret; >> } >> -#endif >> +#endif /* #if defined(CONFIG_ENV_OFFSET_REDUND) #else */ >> >> #if CONFIG_ENV_ADDR != 0x0 >> __weak void *env_sf_get_env_addr(void) >> @@ -311,4 +434,7 @@ U_BOOT_ENV_LOCATION(sf) = { >> #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) >> .init = env_sf_init, >> #endif >> +#if defined(CONFIG_CMD_ERASEENV) && defined(CONFIG_ENV_ADDR) >> + .erase = env_sf_erase, >> +#endif >> }; > -- Harry Waschkeit - Software Engineer