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/gbut 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