From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Fri, 9 Oct 2020 13:00:20 -0400 Subject: [PATCH] env: sf: add support for env erase In-Reply-To: <821145d6-a221-96be-9dd7-5123870118d3@men.de> References: <66c89baf-b52a-81cb-2846-112a7925a256@men.de> <821145d6-a221-96be-9dd7-5123870118d3@men.de> Message-ID: <4215e7c7-068f-ad7a-9c68-4bb8efb30407@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 10/9/20 12:43 PM, Harry Waschkeit wrote: > 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 ... You *can* do it, you just have to configure your mail client correctly. However, it gets pretty tedious when you have a lot of patches :) I suggest configuring git send-email. If you are going to be making more patch series, also check out tools/patman. > > 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. Please keep new code in the correct style. For example, you have > + ret = spi_flash_read(env_flash, saved_offset, > + saved_size, saved_buffer); which is aligned properly, but later on you have >>> + ret = spi_flash_read(env_flash, saved_offset, >>> + saved_size, saved_buffer); which is not. > > 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 macro IS_ENABLED can be used both in C code and in preprocessor directives. See include/linux/kconfig.h for details. > > The sign-off problem I guess is probably caused by the check not accepting name > in reverse order, isn't it? Yes. The format is "First Last ". > 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 ;-/ See below. > I will take care of that before resending my patch (v2 then, right?). Yes. > > 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; >>> + } >>> + } Can this logic be split out into a separate function, since it is shared with env_sf_save? Perhaps make a function like env_sf_do_erase() and call it from env_sf_save and env_sf_erase? >>> + >>> + 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; >>> + } Same thing here; can this be a separate function? >>> + >>> + 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) Why does this depend on ENV_ADDR, when above we only depend on CMD_ERASEENV? >>> + .erase = env_sf_erase, >>> +#endif >>> }; >> > > --Sean