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

[Simon Goldschmidt]

On Fri, Apr 26, 2019 at 5:18 PM Frank Wunderlich
<frank-w at 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