* [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv
@ 2012-01-06 0:30 Christian Daudt
2012-01-06 6:53 ` Mike Frysinger
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Christian Daudt @ 2012-01-06 0:30 UTC (permalink / raw)
To: u-boot
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) {
+ 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);
- 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;
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv
2012-01-06 0:30 [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv Christian Daudt
@ 2012-01-06 6:53 ` Mike Frysinger
2012-10-16 0:53 ` Joe Hershberger
2012-10-16 1:19 ` Joe Hershberger
2 siblings, 0 replies; 4+ messages in thread
From: Mike Frysinger @ 2012-01-06 6:53 UTC (permalink / raw)
To: u-boot
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120106/3a31e19c/attachment.pgp>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv
2012-01-06 0:30 [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv Christian Daudt
2012-01-06 6:53 ` Mike Frysinger
@ 2012-10-16 0:53 ` Joe Hershberger
2012-10-16 1:19 ` Joe Hershberger
2 siblings, 0 replies; 4+ messages in thread
From: Joe Hershberger @ 2012-10-16 0:53 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv
2012-01-06 0:30 [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv Christian Daudt
2012-01-06 6:53 ` Mike Frysinger
2012-10-16 0:53 ` Joe Hershberger
@ 2012-10-16 1:19 ` Joe Hershberger
2 siblings, 0 replies; 4+ messages in thread
From: Joe Hershberger @ 2012-10-16 1:19 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-10-16 1:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-06 0:30 [U-Boot] [PATCH v2] Add support for MMC to fw_printenv/setenv Christian Daudt
2012-01-06 6:53 ` Mike Frysinger
2012-10-16 0:53 ` Joe Hershberger
2012-10-16 1:19 ` Joe Hershberger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox