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

login
register
mail settings
Submitter Christian Daudt
Date Jan. 6, 2012, 12:30 a.m.
Message ID <CAGFynZ83-pJTEB6XcPV4Dvv=zAaXk7aqkN3TQG1_WvPCvX0WjQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/134561/
State Changes Requested
Delegated to: Joe Hershberger
Headers show

Comments

Christian Daudt - Jan. 6, 2012, 12:30 a.m.
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@broadcom.com>
---
 tools/env/fw_env.c |  103 ++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 71 insertions(+), 32 deletions(-)

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

-	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) {
+		rc = ioctl(fd, MEMGETINFO, &mtdinfo);
+		if (rc < 0) {
+			perror("Cannot get MTD information");
+			return -1;
+		}
+
+		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.
+		 */
+		mtdinfo.type = MTD_ABSENT;
 	}

 	DEVTYPE(dev_current) = mtdinfo.type;
Mike Frysinger - Jan. 6, 2012, 6:53 a.m.
On Thursday 05 January 2012 19:30:57 Christian Daudt wrote:
> 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.

seems reasonable, but i don't know anything about this code ;)
-mike
Joe Hershberger - Oct. 16, 2012, 12:53 a.m.
Hi Christian,

On Thu, Jan 5, 2012 at 6:30 PM, Christian Daudt <csd_b@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'.

Your patch appears to be corrupt.  Patchwork doesn't even find all the
hunks.  http://patchwork.ozlabs.org/patch/134561/

Please resend directly from git send-email or preferably with tools/patman.

Thanks,
-Joe
Joe Hershberger - Oct. 16, 2012, 1:19 a.m.
Hi Christian,

On Thu, Jan 5, 2012 at 6:30 PM, Christian Daudt <csd_b@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@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

Patch

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)  {
+			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) {