Message ID | 66c89baf-b52a-81cb-2846-112a7925a256@men.de |
---|---|
State | Deferred |
Delegated to: | Tom Rini |
Headers | show |
Series | env: sf: add support for env erase | expand |
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 <Harry.Waschkeit@men.de> > --- 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 <harry.waschkeit@men.de>' > > 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 > };
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 <Harry.Waschkeit@men.de> >> --- > > 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 <harry.waschkeit@men.de>' >> >> 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 >> }; >
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 <email@address>". > 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 <Harry.Waschkeit@men.de> >>> --- >> >> 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 <harry.waschkeit@men.de>' >>> >>> 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
On 10/9/20 7:00 PM, Sean Anderson wrote: > 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 :) Yeah, guess so ... ;-/ > I suggest configuring git send-email. If you are going to be making more > patch series, also check out tools/patman. I'll definitely have a look at that, sooner or later. >> >> 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. Ok, got it. >> >> 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. Hmm, that's strange, I tried that one but the complaints remained. Chances are I did something wrong so I will have a look at kconfig.h to get also around that. >> >> 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 <email@address>". This will be the easiest part then :-) >> 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 <Harry.Waschkeit@men.de> >>>> --- >>> >>> 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 <harry.waschkeit@men.de>' >>>> >>>> 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? Funnily enough, I did think about that for a short moment but cowardly didn't dare restructuring such a central file with my first U-Boot patch ever ... But yeah, I fully agree, code replication of non-trivial things deserve appropriate refactoring, I'll give that a try in my next patch. >>>> + >>>> + 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? Sure, when I am at it anyway for the other path :-) Actually, the two paths (w/ and w/o redundancy) look to me as if they probably could be cleaned up even more so that no separate implementations for these functions would be necessary, but I am not sure how welcome that would be ... I don't want to give the impression being a know-it-all, you know, just want to help a little bit improving things ;-) >>>> + >>>> + 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? Good catch, will have another thorough look at that. >>>> + .erase = env_sf_erase, >>>> +#endif >>>> }; >>> >> >> > > --Sean >
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 };
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 <Harry.Waschkeit@men.de> --- env/sf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 128 insertions(+), 2 deletions(-)