[U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv

[Joe Hershberger]

Hi Christian,

On Thu, Jan 5, 2012 at 6:30 PM, Christian Daudt <csd_b at daudt.org> wrote:
> Changes from previous:
> - Changed // to /* */
> - Ran through checkpatch.pl, cleaned up a number of line-too-big and
> extra space in the code that was shifted due to being in the new 'if'.
>
>  Thanks,
>     csd
>
>
> Subject: [PATCH] Add support for MMC to fw_printenv/setenv
>
> This patch checks if the fd is MTD and if not (using an MTD-specific IOCTL)
> and skips the flash unlock/erase/lock sequence if it is not MTD.
> - fd_is_mtd function added to determine MTD/MMC
> - flash_write_block made to not try MTD operations if mtd_type == MTD_ABSENT
> - flash_read works with MMC devices now.
>
> Signed-off-by: Christian Daudt <csd at broadcom.com>
> ---
>  tools/env/fw_env.c |  103 ++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 71 insertions(+), 32 deletions(-)
>
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 996682e..c760429 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -211,6 +211,27 @@ static int flash_io (int mode);
>  static char *envmatch (char * s1, char * s2);
>  static int parse_config (void);
>
> +
> +/*
> + * Returns 1 if it is MTD and 0 if it is not MTD
> + */
> +static int fd_is_mtd(int fd)
> +{
> +       struct mtd_info_user mtdinfo;
> +       int rc;
> +
> +       rc = ioctl(fd, MEMGETINFO, &mtdinfo);
> +       if (rc < 0) {
> +               /* Failed MEMGETINFO, not MTD */
> +               return 0;
> +       } else {
> +               /* Succeeded, MTD */
> +               return 1;
> +       }
> +}
> +
> +
> +
>  #if defined(CONFIG_FILE)
>  static int get_config (char *);
>  #endif
> @@ -836,31 +857,35 @@ static int flash_write_buf (int dev, int fd,
> void *buf, size_t count,
>
>         /* This only runs once on NOR flash and SPI-dataflash */
>         while (processed < write_total) {
> -               rc = flash_bad_block (fd, mtd_type, &blockstart);
> -               if (rc < 0)             /* block test failed */
> -                       return rc;
> +               if (mtd_type != MTD_ABSENT)  {

It would be really great if you organize this so as not to fully add
another level of indentation here.  It would be better to include all
the accesses that need to check for mtd_type to functions (like
flash_bad_block() and do the tests in them.  Use a case structure in
them where it makes sense (checking multiple states).  Then you can
simply call the functions and they may not do anything for a given
flash type.

So maybe add a flash_unlock(), flash_lock(), and flash_erase().

> +                       rc = flash_bad_block(fd, mtd_type, &blockstart);
> +                       if (rc < 0)             /* block test failed */
> +                               return rc;
>
> -               if (blockstart + erasesize > top_of_range) {
> -                       fprintf (stderr, "End of range reached, aborting\n");
> -                       return -1;
> -               }
> +                       if (blockstart + erasesize > top_of_range) {
> +                               fprintf(stderr,
> +                                       "End of range reached, aborting\n");
> +                               return -1;
> +                       }
>
> -               if (rc) {               /* block is bad */
> -                       blockstart += blocklen;
> -                       continue;
> -               }
> +                       if (rc) {               /* block is bad */
> +                               blockstart += blocklen;
> +                               continue;
> +                       }
>
> -               erase.start = blockstart;
> -               ioctl (fd, MEMUNLOCK, &erase);
> +                       erase.start = blockstart;
> +                       ioctl(fd, MEMUNLOCK, &erase);
>
> -               /* Dataflash does not need an explicit erase cycle */
> -               if (mtd_type != MTD_DATAFLASH)
> -                       if (ioctl (fd, MEMERASE, &erase) != 0) {
> -                               fprintf (stderr, "MTD erase error on %s: %s\n",
> -                                        DEVNAME (dev),
> -                                        strerror (errno));
> -                               return -1;
> -                       }
> +                       /* Dataflash does not need an explicit erase cycle */
> +                       if (mtd_type != MTD_DATAFLASH)
> +                               if (ioctl(fd, MEMERASE, &erase) != 0) {
> +                                       fprintf(stderr,
> +                                               "MTD erase error on %s: %s\n",
> +                                               DEVNAME(dev),
> +                                               strerror(errno));
> +                                       return -1;
> +                               }
> +               }
>
>                 if (lseek (fd, blockstart, SEEK_SET) == -1) {
>                         fprintf (stderr,
> @@ -878,7 +903,8 @@ static int flash_write_buf (int dev, int fd, void
> *buf, size_t count,
>                         return -1;
>                 }
>
> -               ioctl (fd, MEMLOCK, &erase);
> +               if (mtd_type != MTD_ABSENT)
> +                       ioctl(fd, MEMLOCK, &erase);
>
>                 processed  += blocklen;
>                 block_seek = 0;
> @@ -964,18 +990,31 @@ static int flash_read (int fd)
>  {
>         struct mtd_info_user mtdinfo;
>         int rc;
> +       int is_mtd;
>
> -       rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> -       if (rc < 0) {
> -               perror ("Cannot get MTD information");
> -               return -1;
> -       }
> +       is_mtd = fd_is_mtd(fd);

Why assign this to a variable when you only ever use it in the if statement?

>
> -       if (mtdinfo.type != MTD_NORFLASH &&
> -           mtdinfo.type != MTD_NANDFLASH &&
> -           mtdinfo.type != MTD_DATAFLASH) {
> -               fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type);
> -               return -1;
> +       if (is_mtd) {

The value of this is simply the result of the function you are about
to call.  Why do you need a separate function for this?  It's only
called this one place.

> +               rc = ioctl(fd, MEMGETINFO, &mtdinfo);
> +               if (rc < 0) {
> +                       perror("Cannot get MTD information");
> +                       return -1;

How about instead of erroring, you just mtdinfo.type = MTD_ABSENT;  As
is, it is impossible to ever reach this case.

> +               }
> +
> +               if (mtdinfo.type != MTD_NORFLASH &&
> +                   mtdinfo.type != MTD_NANDFLASH &&
> +                   mtdinfo.type != MTD_DATAFLASH) {
> +                       fprintf(stderr, "Unsupported flash type %u\n",
> +                               mtdinfo.type);
> +                       return -1;
> +               }
> +       } else {
> +               /*
> +                * Kinda hacky assuming !MTD means == MMC
> +                * but seems to be the easiest way to
> +                * determine that.
> +                */

This comment seems inappropriate... you aren't setting the type to
MMC, you are setting it to MTD_ABSENT.  Delete this comment.

> +               mtdinfo.type = MTD_ABSENT;
>         }
>
>         DEVTYPE(dev_current) = mtdinfo.type;
> --
> 1.7.1

-Joe