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.waschk...@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.waschk...@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
--
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Straße 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott -
Handelsregister AG Nuernberg HRB 5540
