On Fri, Apr 12, 2019 at 11:38 AM Frank Wunderlich frank-w@public-files.de wrote:

Signed-off-by: Frank Wunderlich frank-w@public-files.de

cmd/nvedit.c | 13 +++++++++++++ env/env.c | 30 ++++++++++++++++++++++++++++++ include/environment.h | 17 +++++++++++++++++ 3 files changed, 60 insertions(+)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 24a6cf7824..3b5c62d629 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -756,11 +756,22 @@ static int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, return env_save() ? 1 : 0; }

+static int do_env_erase(cmd_tbl_t *cmdtp, int flag, int argc,

•                   char * const argv[])
  

+{

•   return env_erase() ? 1 : 0;
  

+}

U_BOOT_CMD( saveenv, 1, 0, do_env_save, "save environment variables to persistent storage", "" ); +U_BOOT_CMD(

•   eraseenv, 1, 0, do_env_erase,
  

•   "erase environment variables from persistent storage",
  

•   ""
  

+); #endif #endif /* CONFIG_SPL_BUILD */

@@ -1207,6 +1218,7 @@ static cmd_tbl_t cmd_env_sub[] = { #endif #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE) U_BOOT_CMD_MKENT(save, 1, 0, do_env_save, "", ""),

•   U_BOOT_CMD_MKENT(erase, 1, 0, do_env_erase, "", ""),
  

Guarding this with the same Kconfig option as 'saveenv' might be a bit confusing?

#endif U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 0, do_env_set, "", ""), #if defined(CONFIG_CMD_ENV_EXISTS) @@ -1282,6 +1294,7 @@ static char env_help_text[] = #endif #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE) "env save - save environment\n"

•   "env erase - erase environment\n"
  

#endif #if defined(CONFIG_CMD_NVEDIT_EFI) "env set -e name [arg ...] - set UEFI variable; unset if 'arg' not specified\n" diff --git a/env/env.c b/env/env.c index 4b417b90a2..25441370e6 100644 --- a/env/env.c +++ b/env/env.c @@ -24,6 +24,8 @@ void env_fix_drivers(void) entry->load += gd->reloc_off; if (entry->save) entry->save += gd->reloc_off;

•           if (entry->erase)
  

•                   entry->erase += gd->reloc_off;
            if (entry->init)
                    entry->init += gd->reloc_off;
    }
  

@@ -254,6 +256,34 @@ int env_save(void) return -ENODEV; }

+int env_erase(void) +{

•   struct env_driver *drv;
  

•   drv = env_driver_lookup(ENVOP_ERASE, gd->env_load_prio);
  

•   if (drv) {
  

•           int ret;
  

•           if (!drv->erase)
  

•                   return -ENODEV;
  

•           if (!env_has_inited(drv->location))
  

•                   return -ENODEV;
  

•           printf("Erase Environment on %s... ", drv->name);
  

Following the other prints, this should probably start with "Erasing".

•           ret = drv->erase();
  

•           if (ret)
  

•                   printf("Failed (%d)\n", ret);
  

•           else
  

•                   printf("OK\n");
  

•           if (!ret)
  

•                   return 0;
  

•   }
  

•   return -ENODEV;
  

+}

int env_init(void) { struct env_driver *drv; diff --git a/include/environment.h b/include/environment.h index cd96676141..5024cbea7e 100644 --- a/include/environment.h +++ b/include/environment.h @@ -200,6 +200,7 @@ enum env_operation { ENVOP_INIT, /* we want to call the init function */ ENVOP_LOAD, /* we want to call the load function */ ENVOP_SAVE, /* we want to call the save function */

•   ENVOP_ERASE,    /* we want to call the erase function */
  

};

struct env_driver { @@ -225,6 +226,15 @@ struct env_driver { */ int (*save)(void);

•   /**
  

•    * erase() - Erase the environment on storage
  

•    *
  

•    * This method is required for 'eraseenv' to work.
  

I know you copied this from 'save', but I think you should mention that it's optional.

Regards, Simon

•    *
  

•    * @return 0 if OK, -ve on error
  

•    */
  

•   int (*erase)(void);
  

•   /**
     * init() - Set up the initial pre-relocation environment
     *
  

@@ -303,6 +313,13 @@ int env_load(void); */ int env_save(void);

+/**

• • env_erase() - Erase the environment on storage
• • @return 0 if OK, -ve on error
• */

+int env_erase(void);

/**

• env_fix_drivers() - Updates envdriver as per relocation

*/

2.17.1

On Fri, Apr 12, 2019 at 11:38 AM Frank Wunderlich frank-w@public-files.de wrote:

Missing description here. I just saw I forgot this comment for the 1/2 patch, too. There's no description there as well.

Signed-off-by: Frank Wunderlich frank-w@public-files.de

env/mmc.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)

diff --git a/env/mmc.c b/env/mmc.c index c3cf35d01b..f387a53970 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -242,6 +242,33 @@ fini: fini_mmc_for_env(mmc); return ret; }

+static int env_mmc_erase(void) +{

•   int dev = mmc_get_env_dev();
  

•   struct mmc *mmc = find_mmc_device(dev);
  

•   int     n, blk, cnt;
  

Bogus indent?

•   if (!mmc)
  

•           return CMD_RET_FAILURE;
  

•   blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;
  

•   cnt = CONFIG_ENV_SIZE / mmc->read_bl_len;
  

Hmm, this doesn't work with redundant env. To fix this, you probably need to touch patch 1/2, add a parameter to the command specifying which env to erase (or both) and pass it to this callback.

•   printf("\nMMC erase env: dev # %d, block # %d (0x%x), count %d (0x%x)\n",
  

•          dev, blk, blk * mmc->read_bl_len,
  

•          cnt, cnt * mmc->read_bl_len);
  

•   if (mmc_getwp(mmc) == 1) {
  

•           printf("Error: card is write protected!\n");
  

•           return CMD_RET_FAILURE;
  

•   }
  

•   n = blk_derase(mmc_get_blk_desc(mmc), blk, cnt);
  

•   printf("%d blocks erased: %s\n", n, (n == cnt) ? "OK" : "ERROR");
  

•   return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
  

+}

Overall, I think this function should more resemble the save function. That should make it easier to follow for someone trying to understand this file.

Regards, Simon

#endif /* CONFIG_CMD_SAVEENV && !CONFIG_SPL_BUILD */

static inline int read_env(struct mmc *mmc, unsigned long size, @@ -351,5 +378,6 @@ U_BOOT_ENV_LOCATION(mmc) = { .load = env_mmc_load, #ifndef CONFIG_SPL_BUILD .save = env_save_ptr(env_mmc_save),

•   .erase          = env_mmc_erase,
  

#endif }; -- 2.17.1