On 02.02.21 18:09, Harry Waschkeit wrote:
On 02.02.21 15:54, Stefan Roese wrote:
On 02.02.21 15:43, Harry Waschkeit wrote:

On 02.02.21 10:30, Stefan Roese wrote:
On 02.02.21 09:21, Harry Waschkeit wrote:
Instead of implementing redundant environments in two very similar
functions env_sf_save(), handle redundancy in one function, placing the few differences in appropriate pre-compiler sections depending on config
option CONFIG_ENV_OFFSET_REDUND.

Additionally, several checkpatch complaints were addressed.

This patch is in preparation for adding support for env erase.

Signed-off-by: Harry Waschkeit <harry.waschk...@men.de>
Reviewed-by: Stefan Roese <s...@denx.de>
---
Change in v3:
  - no change in patch, only added "reviewed-by" to commit log

JFYI:
No need to re-send this patch with this added RB tag, as I already did
send a new RB to the last mail as reply. Patchwork collects these tags
when sent to the list. So you only need to include them, if you send
a new patch version.

thanks for this hint, obviously I step into it all the time ...

Ok, lesson learnt, let's see what I can do wrong next time ... ;-/

Back on topic: I guess that was all I needed to do so that the patch will get merged when its time comes.

Yes, now we (you) need a bit of patience, so that other people might
review this patch as well. And usually it will get handled after some
time (depending on the development stage of U-Boot, merge window open
or shortly before release etc).

Ok, no problem with that.

That also means that I should wait with submission of the other patch (adding "env erase" support) until this one is merged.
Otherwise the patch would need to be significantly different ...

No, you don't have to wait. This would be counter-productive. Please
just add the dependancy to this patch in the commit text of the new
one to make this clear. Best would be below the "---" line so that
it will not be included in the git history.

Hmm, I guess it would have been better to not send that patch standalone but instead as a first one in a two-patch series where the second one adds the new functionality on top of the clean-up.
Anyway, something to keep in mind for next time :-)

Yep, that would have been even better. ;)

Thanks,
Stefan

It does not hurt of course to check this from time to time and
"trigger" the maintainer of the subsystem or the custodian this
patch is delegated to nothing is happening here for a too long
time (like more than 1 month).

Alright, good to know :-)

Thanks,
Harry

Thanks,
Stefan

If not, please let me know.

Thanks,
Harry

Thanks,
Stefan

Change in v2:
  - remove one more #ifdef, instead take advantage of compiler attribute
    __maybe_unused for one variable used only in case of redundant
    environments

  env/sf.c | 130 ++++++++++++++++++-------------------------------------
  1 file changed, 41 insertions(+), 89 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index 937778aa37..199114fd3b 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -66,13 +66,14 @@ static int setup_flash_device(void)
      return 0;
  }
-#if defined(CONFIG_ENV_OFFSET_REDUND)
  static int env_sf_save(void)
  {
      env_t    env_new;
-    char    *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
+    char    *saved_buffer = NULL;
      u32    saved_size, saved_offset, sector;
+    ulong    offset;
      int    ret;
+    __maybe_unused char flag = ENV_REDUND_OBSOLETE;
      ret = setup_flash_device();
      if (ret)
@@ -81,27 +82,33 @@ static int env_sf_save(void)
      ret = env_export(&env_new);
      if (ret)
          return -EIO;
-    env_new.flags    = ENV_REDUND_ACTIVE;
-    if (gd->env_valid == ENV_VALID) {
-        env_new_offset = CONFIG_ENV_OFFSET_REDUND;
-        env_offset = CONFIG_ENV_OFFSET;
-    } else {
-        env_new_offset = CONFIG_ENV_OFFSET;
-        env_offset = CONFIG_ENV_OFFSET_REDUND;
-    }
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
+        env_new.flags    = ENV_REDUND_ACTIVE;
+
+        if (gd->env_valid == ENV_VALID) {
+            env_new_offset = CONFIG_ENV_OFFSET_REDUND;
+            env_offset = CONFIG_ENV_OFFSET;
+        } else {
+            env_new_offset = CONFIG_ENV_OFFSET;
+            env_offset = CONFIG_ENV_OFFSET_REDUND;
+        }
+        offset = env_new_offset;
+#else
+        offset = CONFIG_ENV_OFFSET;
+#endif
      /* 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 = env_new_offset + CONFIG_ENV_SIZE;
+        saved_offset = offset + CONFIG_ENV_SIZE;
          saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
          if (!saved_buffer) {
              ret = -ENOMEM;
              goto done;
          }
-        ret = spi_flash_read(env_flash, saved_offset,
-                    saved_size, saved_buffer);
+        ret = spi_flash_read(env_flash, saved_offset, saved_size,
+                     saved_buffer);
          if (ret)
              goto done;
      }
@@ -109,35 +116,39 @@ static int env_sf_save(void)
      sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
      puts("Erasing SPI flash...");
-    ret = spi_flash_erase(env_flash, env_new_offset,
-                sector * CONFIG_ENV_SECT_SIZE);
+    ret = spi_flash_erase(env_flash, offset,
+                  sector * CONFIG_ENV_SECT_SIZE);
      if (ret)
          goto done;
      puts("Writing to SPI flash...");
-    ret = spi_flash_write(env_flash, env_new_offset,
-        CONFIG_ENV_SIZE, &env_new);
+    ret = spi_flash_write(env_flash, offset,
+                  CONFIG_ENV_SIZE, &env_new);
      if (ret)
          goto done;
      if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
-        ret = spi_flash_write(env_flash, saved_offset,
-                    saved_size, saved_buffer);
+        ret = spi_flash_write(env_flash, saved_offset, saved_size,
+                      saved_buffer);
          if (ret)
              goto done;
      }
-    ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
-                sizeof(env_new.flags), &flag);
-    if (ret)
-        goto done;
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
+        ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
+                      sizeof(env_new.flags), &flag);
+        if (ret)
+            goto done;
-    puts("done\n");
+        puts("done\n");
-    gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; +        gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
-    printf("Valid environment: %d\n", (int)gd->env_valid);
+        printf("Valid environment: %d\n", (int)gd->env_valid);
+#else
+        puts("done\n");
+#endif
   done:
      if (saved_buffer)
@@ -146,6 +157,7 @@ static int env_sf_save(void)
      return ret;
  }
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
  static int env_sf_load(void)
  {
      int ret;
@@ -183,66 +195,6 @@ out:
      return ret;
  }
  #else
-static int env_sf_save(void)
-{
-    u32    saved_size, saved_offset, sector;
-    char    *saved_buffer = NULL;
-    int    ret = 1;
-    env_t    env_new;
-
-    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;
-    }
-
-    ret = env_export(&env_new);
-    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;
-
-    puts("Writing to SPI flash...");
-    ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
-        CONFIG_ENV_SIZE, &env_new);
-    if (ret)
-        goto done;
-
-    if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
-        ret = spi_flash_write(env_flash, saved_offset,
-            saved_size, saved_buffer);
-        if (ret)
-            goto done;
-    }
-
-    ret = 0;
-    puts("done\n");
-
- done:
-    if (saved_buffer)
-        free(saved_buffer);
-
-    return ret;
-}
-
  static int env_sf_load(void)
  {
      int ret;
@@ -258,8 +210,8 @@ static int env_sf_load(void)
      if (ret)
          goto out;
-    ret = spi_flash_read(env_flash,
-        CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
+    ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
+                 buf);
      if (ret) {
          env_set_default("spi_flash_read() failed", 0);
          goto err_read;
@@ -292,7 +244,7 @@ static int env_sf_init(void)
      env_t *env_ptr = (env_t *)env_sf_get_env_addr();
      if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
-        gd->env_addr    = (ulong)&(env_ptr->data);
+        gd->env_addr    = (ulong)&env_ptr->data;
          gd->env_valid    = 1;
      } else {
          gd->env_addr = (ulong)&default_environment[0];



Viele Grüße,
Stefan





Viele Grüße,
Stefan





Viele Grüße,
Stefan

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de

Reply via email to