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

this adds erase environment for mmc storage

squashed fixes:
 - add CONFIG_CMD_ERASEENV
 - env: erase redundant offset if defined

Suggested-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 env/mmc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

--
2.17.1

On Wed, Jun 26, 2019 at 12:29 PM Frank Wunderlich
<frank-w@public-files.de> wrote:
>
> this adds erase environment for mmc storage
>
> squashed fixes:
>  - add CONFIG_CMD_ERASEENV
>  - env: erase redundant offset if defined
>
> Suggested-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  env/mmc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/env/mmc.c b/env/mmc.c
> index c3cf35d01b..da5cf21637 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -242,6 +242,55 @@ fini:
>         fini_mmc_for_env(mmc);
>         return ret;
>  }
> +
> +#if defined(CONFIG_CMD_ERASEENV)

Seems you missed the CONFIG_SPL_BUILD guard here?

> +static inline int erase_env(struct mmc *mmc, unsigned long size,
> +                           unsigned long offset)
> +{
> +       uint blk_start, blk_cnt, n;
> +       struct blk_desc *desc = mmc_get_blk_desc(mmc);
> +
> +       blk_start       = ALIGN(offset, mmc->write_bl_len) / mmc->write_bl_len;
> +       blk_cnt         = ALIGN(size, mmc->write_bl_len) / mmc->write_bl_len;
> +
> +       n = blk_derase(desc, blk_start, blk_cnt);
> +       printf("%d blocks erased: %s\n", n, (n == blk_cnt) ? "OK" : "ERROR");
> +
> +       return (n == blk_cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;

This is no command, don't use the CMD_RET_ macros.

> +}
> +
> +static int env_mmc_erase(void)
> +{
> +       int dev = mmc_get_env_dev();
> +       struct mmc *mmc = find_mmc_device(dev);
> +       int     ret, copy = 0;
> +       u32     offset;
> +
> +       if (!mmc)
> +               return CMD_RET_FAILURE;
> +
> +       if (mmc_getwp(mmc) == 1) {

Why is this needed when save doesn't use it?

> +               printf("Error: card is write protected!\n");
> +               return CMD_RET_FAILURE;
> +       }

Is 'init_mmc_for_env' not needed here?

Regards,
Simon

> +
> +       if (mmc_get_env_addr(mmc, copy, &offset))
> +               return CMD_RET_FAILURE;
> +
> +       ret = erase_env(mmc, CONFIG_ENV_SIZE, offset);
> +
> +#ifdef CONFIG_ENV_OFFSET_REDUND
> +       copy = 1;
> +
> +       if (mmc_get_env_addr(mmc, copy, &offset))
> +               return CMD_RET_FAILURE;
> +
> +       ret |= erase_env(mmc, CONFIG_ENV_SIZE, offset);
> +#endif
> +
> +       return ret;
> +}
> +#endif /* CONFIG_CMD_ERASEENV */
>  #endif /* CONFIG_CMD_SAVEENV && !CONFIG_SPL_BUILD */
>
>  static inline int read_env(struct mmc *mmc, unsigned long size,
> @@ -351,5 +400,8 @@ U_BOOT_ENV_LOCATION(mmc) = {
>         .load           = env_mmc_load,
>  #ifndef CONFIG_SPL_BUILD
>         .save           = env_save_ptr(env_mmc_save),
> +#if defined(CONFIG_CMD_ERASEENV)
> +       .erase          = env_mmc_erase,
> +#endif
>  #endif
>  };
> --
> 2.17.1
>

Hi,

> Gesendet: Donnerstag, 27. Juni 2019 um 11:32 Uhr
> Von: "Simon Goldschmidt" <simon.k.r.goldschmidt@gmail.com>
> > diff --git a/env/mmc.c b/env/mmc.c
> > index c3cf35d01b..da5cf21637 100644
> > --- a/env/mmc.c
> > +++ b/env/mmc.c
> > @@ -242,6 +242,55 @@ fini:
> >         fini_mmc_for_env(mmc);
> >         return ret;
> >  }
> > +
> > +#if defined(CONFIG_CMD_ERASEENV)
>
> Seems you missed the CONFIG_SPL_BUILD guard here?

my block is inside of this (eraseenv only selectable if saveenv is selected)

#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)

> > +static inline int erase_env(struct mmc *mmc, unsigned long size,
> > +                           unsigned long offset)
> > +{
> > +       uint blk_start, blk_cnt, n;
> > +       struct blk_desc *desc = mmc_get_blk_desc(mmc);
> > +
> > +       blk_start       = ALIGN(offset, mmc->write_bl_len) / mmc->write_bl_len;
> > +       blk_cnt         = ALIGN(size, mmc->write_bl_len) / mmc->write_bl_len;
> > +
> > +       n = blk_derase(desc, blk_start, blk_cnt);
> > +       printf("%d blocks erased: %s\n", n, (n == blk_cnt) ? "OK" : "ERROR");
> > +
> > +       return (n == blk_cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
>
> This is no command, don't use the CMD_RET_ macros.

ok, i will return 0/1 similar to write_env (not -1 because of return-value in env_mmc_erase)

> > +}
> > +
> > +static int env_mmc_erase(void)
> > +{
> > +       int dev = mmc_get_env_dev();
> > +       struct mmc *mmc = find_mmc_device(dev);
> > +       int     ret, copy = 0;
> > +       u32     offset;
> > +
> > +       if (!mmc)
> > +               return CMD_RET_FAILURE;
> > +
> > +       if (mmc_getwp(mmc) == 1) {
>
> Why is this needed when save doesn't use it?

thats a point...i'll remove it. added it from do_mmc_write in first version and had not removed it.

> > +               printf("Error: card is write protected!\n");
> > +               return CMD_RET_FAILURE;
> > +       }
>
> Is 'init_mmc_for_env' not needed here?

should be initialized too

> Regards,
> Simon

thanks for review

regards Frank