[U-Boot] [PATCH v5 6/9] dfu: NAND specific routines for DFU operation

Tom Rini
2013-03-13T20:04:11Z

On Fri, Mar 08, 2013 at 07:08:27PM -0600, Scott Wood wrote:
> On 03/08/2013 11:37:25 AM, Tom Rini wrote:
> >From: Pantelis Antoniou <panto@antoniou-consulting.com>
> >
> >Support for NAND storage devices to work with the DFU framework.
> >
> >Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >Signed-off-by: Tom Rini <trini@ti.com>
> >---
> >Changes in v5:
> >- Document CONFIG_DFU_NAND in README
> >
> >Changes in v4: None
> >Changes in v3:
> >- Rework logic in nand_block_op for nand_(read|write)_skip_bad
> >returning
> > just a size for actual used length.
> >- Remove unused externs from drivers/dfu/dfu_nand.c
> >
> >Changes in v2:
> >- nand_block_op calls nand_(read|write)_skip_bad directly.
> >- Bugfix in dfu_nand to make sure we set dfu->skip_bad to 0 on each
> > iteration.
> >
> > README | 3 +
> > drivers/dfu/Makefile | 1 +
> > drivers/dfu/dfu.c | 8 ++
> > drivers/dfu/dfu_nand.c | 195
> >++++++++++++++++++++++++++++++++++++++++++++++++
> > include/dfu.h | 23 ++++++
> > 5 files changed, 230 insertions(+)
> > create mode 100644 drivers/dfu/dfu_nand.c
> >
> >diff --git a/README b/README
> >index 154b82f..c70251e 100644
> >--- a/README
> >+++ b/README
> >@@ -1338,6 +1338,9 @@ The following options need to be configured:
> > CONFIG_DFU_MMC
> > This enables support for exposing (e)MMC devices via DFU.
> >
> >+ CONFIG_DFU_NAND
> >+ This enables support for exposing NAND devices via DFU.
> >+
> > CONFIG_SYS_DFU_MAX_FILE_SIZE
> > When updating files rather than the raw storage device,
> > we use a static buffer to copy the file into and then write
> >diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile
> >index 7b717bc..153095d 100644
> >--- a/drivers/dfu/Makefile
> >+++ b/drivers/dfu/Makefile
> >@@ -27,6 +27,7 @@ LIB = $(obj)libdfu.o
> >
> > COBJS-$(CONFIG_DFU_FUNCTION) += dfu.o
> > COBJS-$(CONFIG_DFU_MMC) += dfu_mmc.o
> >+COBJS-$(CONFIG_DFU_NAND) += dfu_nand.o
> >
> > SRCS := $(COBJS-y:.o=.c)
> > OBJS := $(addprefix $(obj),$(COBJS-y))
> >diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> >index 2fecf77..84139bd 100644
> >--- a/drivers/dfu/dfu.c
> >+++ b/drivers/dfu/dfu.c
> >@@ -86,6 +86,7 @@ int dfu_write(struct dfu_entity *dfu, void *buf,
> >int size, int blk_seq_num)
> > /* initial state */
> > dfu->crc = 0;
> > dfu->offset = 0;
> >+ dfu->bad_skip = 0;
> > dfu->i_blk_seq_num = 0;
> > dfu->i_buf_start = dfu_buf;
> > dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
> >@@ -235,6 +236,8 @@ int dfu_read(struct dfu_entity *dfu, void
> >*buf, int size, int blk_seq_num)
> > dfu->i_buf = dfu->i_buf_start;
> > dfu->b_left = 0;
> >
> >+ dfu->bad_skip = 0;
> >+
> > dfu->inited = 1;
> > }
> >
> >@@ -264,6 +267,8 @@ int dfu_read(struct dfu_entity *dfu, void
> >*buf, int size, int blk_seq_num)
> > dfu->i_buf = dfu->i_buf_start;
> > dfu->b_left = 0;
> >
> >+ dfu->bad_skip = 0;
> >+
> > dfu->inited = 0;
> > }
> >
> >@@ -286,6 +291,9 @@ static int dfu_fill_entity(struct dfu_entity
> >*dfu, char *s, int alt,
> > if (strcmp(interface, "mmc") == 0) {
> > if (dfu_fill_entity_mmc(dfu, s))
> > return -1;
> >+ } else if (strcmp(interface, "nand") == 0) {
> >+ if (dfu_fill_entity_nand(dfu, s))
> >+ return -1;
> > } else {
> > printf("%s: Device %s not (yet) supported!\n",
> > __func__, interface);
> >diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
> >new file mode 100644
> >index 0000000..b7f60dd
> >--- /dev/null
> >+++ b/drivers/dfu/dfu_nand.c
> >@@ -0,0 +1,195 @@
> >+/*
> >+ * dfu_nand.c -- DFU for NAND routines.
> >+ *
> >+ * Copyright (C) 2012-2013 Texas Instruments, Inc.
> >+ *
> >+ * Based on dfu_mmc.c which is:
> >+ * Copyright (C) 2012 Samsung Electronics
> >+ * author: Lukasz Majewski <l.majewski@samsung.com>
> >+ *
> >+ * This program is free software; you can redistribute it and/or
> >modify
> >+ * it under the terms of the GNU General Public License as
> >published by
> >+ * the Free Software Foundation; either version 2 of the License, or
> >+ * (at your option) any later version.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >+ * GNU General Public License for more details.
> >+ *
> >+ * You should have received a copy of the GNU General Public License
> >+ * along with this program; if not, write to the Free Software
> >+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> >02111-1307 USA
> >+ */
> >+
> >+#include <common.h>
> >+#include <malloc.h>
> >+#include <errno.h>
> >+#include <div64.h>
> >+#include <dfu.h>
> >+#include <linux/mtd/mtd.h>
> >+#include <jffs2/load_kernel.h>
> >+#include <nand.h>
> >+
> >+enum dfu_nand_op {
> >+ DFU_OP_READ = 1,
> >+ DFU_OP_WRITE,
> >+};
> >+
> >+static int nand_block_op(enum dfu_nand_op op, struct dfu_entity *dfu,
> >+ u64 offset, void *buf, long *len)
> >+{
> >+ loff_t start;
> >+ size_t count, actual;
> >+ int ret;
> >+ int dev;
> >+ nand_info_t *nand;
> >+
> >+ /* if buf == NULL return total size of the area */
> >+ if (buf == NULL) {
> >+ *len = dfu->data.nand.size;
> >+ return 0;
> >+ }
> >+
> >+ start = dfu->data.nand.start + offset + dfu->bad_skip;
> >+ count = *len;
> >+ if (start + count >
> >+ dfu->data.nand.start + dfu->data.nand.size) {
> >+ printf("%s: block_op out of bounds\n", __func__);
> >+ return -1;
> >+ }
> >+
> >+ dev = nand_curr_device;
> >+ if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE ||
> >+ !nand_info[dev].name) {
> >+ printf("%s: invalid nand device\n", __func__);
> >+ return -1;
> >+ }
> >+
> >+ nand = &nand_info[dev];
> >+
> >+ if (op == DFU_OP_READ)
> >+ ret = nand_read_skip_bad(nand, start, &count, &actual,
> >+ nand->size, buf);
> >+ else
> >+ ret = nand_write_skip_bad(nand, start, &count, &actual,
> >+ nand->size, buf, 0);
>
> Shouldn't "lim" be "dfu->data.nand.start + dfu->data.nand.size - start"?
>
> This should let you get rid of the bounds check above, which doesn't
> factor in bad blocks that have yet to be encountered.

Good point, will re-work.

> >+ if (ret != 0) {
> >+ printf("%s: nand_%s_skip_bad call failed at %llx!\n",
> >+ __func__, op == DFU_OP_READ ? "read" : "write",
> >+ start);
> >+ return ret;
> >+ }
> >+
> >+ /*
> >+ * Find out where we stopped writing data. This can be deeper
> >into
> >+ * the NAND than we expected due to having to skip bad blocks.
> >So
> >+ * we must take this into account for the next write, if any.
> >+ */
> >+ if (actual > count) {
> >+ printf("%s: skipped 0x%x bad bytes at 0x%llx\n", __func__,
> >+ actual - count, start);
> >+ dfu->bad_skip += actual - count;
> >+ }
>
> Hmm, the message suggests the bad blocks are at "start" rather than
> somewhere between "start" and "start + actual".
>
> nand_write_skip_bad() already prints a message when actually
> skipping the block, so you probably don't need any print here.

OK, I'll just drop that. Thanks!

--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130313/044180dd/attachment.pgp>