[U-Boot,v3,2/2] env: mmc: add erase-function

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 env/mmc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

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

Hi Simon

thanks for your review..

> > +       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.

i had not understand the idea behind redundant-offset...you mean i should let user choose which offset to clear? save uses this way to determine the offset:

int copy = 0;
#ifdef CONFIG_ENV_OFFSET_REDUND
    if (gd->env_valid == ENV_VALID) //use redundant offset if default location holds valid environment (and ENV_OFFSET_REDUND is defined)??
        copy = 1;
#endif

    if (mmc_get_env_addr(mmc, copy, &offset)) {

should i use the same way or something like this (additional parameter to callback, also command "env erase" needs additional optional parameter):

#ifdef CONFIG_ENV_OFFSET_REDUND
    if (use_redund)
        blk = CONFIG_ENV_OFFSET_REDUND / mmc->read_bl_len;
    else
#endif
        blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;


> > +
> > +       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.

i first copied the save-function and removed/changed anything to match new usecase...so the flow is same as for save. i removed the gotos because they are not needed because i think i don't need "fini_mmc_for_env(mmc);", do i?

regards Frank

On Fri, Apr 26, 2019 at 5:18 PM Frank Wunderlich
<frank-w@public-files.de> wrote:
>
> Hi Simon
>
> thanks for your review..
>
> > > +       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.
>
> i had not understand the idea behind redundant-offset...you mean i should let user choose which offset to clear? save uses this way to determine the offset:

The idea is to have 2 copies of the env so if one gets corrupted, the older
one is loaded. I think what "save" does is to save to the one that hasn't been
used for loading.

In contrast to this, "erase" should probebly erase both environments to be
robust but might optionally erase one of the two copies...

By now I think it would be enough to just erase both copies as a start.

>
> int copy = 0;
> #ifdef CONFIG_ENV_OFFSET_REDUND
>     if (gd->env_valid == ENV_VALID) //use redundant offset if default location holds valid environment (and ENV_OFFSET_REDUND is defined)??
>         copy = 1;
> #endif
>
>     if (mmc_get_env_addr(mmc, copy, &offset)) {
>
> should i use the same way or something like this (additional parameter to callback, also command "env erase" needs additional optional parameter):

To make it easier to read, copy the "2-function style" used by save
where only the 2nd function calculates the block offset.

Then you can call this 2nd function twice with different offsets
when erasing redundant env.

Regards,
Simon

>
> #ifdef CONFIG_ENV_OFFSET_REDUND
>     if (use_redund)
>         blk = CONFIG_ENV_OFFSET_REDUND / mmc->read_bl_len;
>     else
> #endif
>         blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;
>
>
> > > +
> > > +       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.
>
> i first copied the save-function and removed/changed anything to match new usecase...so the flow is same as for save. i removed the gotos because they are not needed because i think i don't need "fini_mmc_for_env(mmc);", do i?
>
> regards Frank