[U-Boot] [PATCH v2 0/4] Add NAND support to DFU

[U-Boot] [PATCH v2 0/4] Add NAND support to DFU

[Tom Rini]

Hey all,

This is a v2 of Pantelis' DFU for NAND patches.  Based on Scott's
feedback on the list as well as some further discussion, things have
been reworked so that we are not using the command interface but instead
calling nand_(read|write)_skip_bad directly.  This needed some changes
to that code so that we can find out how much space was used for our
buffer (as we write in chunks).  The NAND changes have been
compile-tested on all ARM and PowerPC targets and run-time tested on
ARM.  A pleasant side-effect of these changes is that we can now, when
writing to mtdparts defined partitions at least, check for and prevent
cases when badblocks would cause us to previously silently overwrite
part of the next partition.

For practical reasons, this series depends on Pantelis' previous series
of generic DFU changes.

Changes since v1:
- NAND skip_check_len changes reworked to allow
  nand_(read|write)_skip_bad to return this information to the caller.
- dfu_nand calls nand_(read|write)_skip_bad directly.
- Bugfix in dfu_nand to make sure we set dfu->skip_bad to 0 on each
  iteration.
- Define mtdparts for am335x_evm
- Set both dfu_alt_info_mmc and dfu_alt_info_nand on am335x_evm so that
  there working examples for both by default.

-- 
Tom

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini]

We make these two functions take a size_t pointer to how much space
was used on NAND to read or write the buffer (when reads/writes happen)
so that bad blocks can be accounted for.  We also make them take an
loff_t limit on how much data can be read or written.  This means that
we can now catch the case of when writing to a partition would exceed
the partition size due to bad blocks.  To do this we also need to make
check_skip_len care about total actual size used rather than block_size
chunks used.  All callers of nand_(read|write)_skip_bad are adjusted to
call these with the most sensible limits available.

The changes were started by Pantelis and finished by Tom.

Cc: Scott Wood <scottwood@freescale.com>
Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
 common/cmd_nand.c            |   61 +++++++++++++++++++++--------------------
 common/env_nand.c            |    5 ++--
 drivers/mtd/nand/nand_util.c |   62 +++++++++++++++++++++++++++++++-----------
 include/nand.h               |    4 +--
 4 files changed, 82 insertions(+), 50 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 1568594..e091e02 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -137,7 +137,8 @@ static inline int str2long(const char *p, ulong *num)
 	return *p != '\0' && *endptr == '\0';
 }
 
-static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size)
+static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size,
+		loff_t *maxsize)
 {
 #ifdef CONFIG_CMD_MTDPARTS
 	struct mtd_device *dev;
@@ -160,6 +161,7 @@ static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size)
 
 	*off = part->offset;
 	*size = part->size;
+	*maxsize = part->offset + part->size;
 	*idx = dev->id->num;
 
 	ret = set_dev(*idx);
@@ -173,10 +175,11 @@ static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size)
 #endif
 }
 
-static int arg_off(const char *arg, int *idx, loff_t *off, loff_t *maxsize)
+static int arg_off(const char *arg, int *idx, loff_t *off, loff_t *size,
+		loff_t *maxsize)
 {
 	if (!str2off(arg, off))
-		return get_part(arg, idx, off, maxsize);
+		return get_part(arg, idx, off, size, maxsize);
 
 	if (*off >= nand_info[*idx].size) {
 		puts("Offset exceeds device limit\n");
@@ -188,36 +191,29 @@ static int arg_off(const char *arg, int *idx, loff_t *off, loff_t *maxsize)
 }
 
 static int arg_off_size(int argc, char *const argv[], int *idx,
-			loff_t *off, loff_t *size)
+			loff_t *off, loff_t *size, loff_t *maxsize)
 {
 	int ret;
-	loff_t maxsize = 0;
 
 	if (argc == 0) {
 		*off = 0;
 		*size = nand_info[*idx].size;
+		*maxsize = *size;
 		goto print;
 	}
 
-	ret = arg_off(argv[0], idx, off, &maxsize);
+	ret = arg_off(argv[0], idx, off, size, maxsize);
 	if (ret)
 		return ret;
 
-	if (argc == 1) {
-		*size = maxsize;
+	if (argc == 1)
 		goto print;
-	}
 
 	if (!str2off(argv[1], size)) {
 		printf("'%s' is not a number\n", argv[1]);
 		return -1;
 	}
 
-	if (*size > maxsize) {
-		puts("Size exceeds partition or device limit\n");
-		return -1;
-	}
-
 print:
 	printf("device %d ", *idx);
 	if (*size == nand_info[*idx].size)
@@ -299,15 +295,14 @@ int do_nand_env_oob(cmd_tbl_t *cmdtp, int argc, char *const argv[])
 
 		printf("0x%08lx\n", nand_env_oob_offset);
 	} else if (!strcmp(cmd, "set")) {
-		loff_t addr;
-		loff_t maxsize;
+		loff_t addr, size, maxsize;
 		struct mtd_oob_ops ops;
 		int idx = 0;
 
 		if (argc < 3)
 			goto usage;
 
-		if (arg_off(argv[2], &idx, &addr, &maxsize)) {
+		if (arg_off(argv[2], &idx, &addr, &size, &maxsize)) {
 			puts("Offset or partition name expected\n");
 			return 1;
 		}
@@ -432,7 +427,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i, ret = 0;
 	ulong addr;
-	loff_t off, size;
+	loff_t off, size, maxsize;
 	char *cmd, *s;
 	nand_info_t *nand;
 #ifdef CONFIG_SYS_NAND_QUIET
@@ -557,7 +552,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 		printf("\nNAND %s: ", cmd);
 		/* skip first two or three arguments, look for offset and size */
-		if (arg_off_size(argc - o, argv + o, &dev, &off, &size) != 0)
+		if (arg_off_size(argc - o, argv + o, &dev, &off, &size,
+					&maxsize) != 0)
 			return 1;
 
 		nand = &nand_info[dev];
@@ -605,7 +601,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	if (strncmp(cmd, "read", 4) == 0 || strncmp(cmd, "write", 5) == 0) {
-		size_t rwsize;
+		size_t rwsize, actual;
 		ulong pagecount = 1;
 		int read;
 		int raw;
@@ -625,7 +621,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (s && !strcmp(s, ".raw")) {
 			raw = 1;
 
-			if (arg_off(argv[3], &dev, &off, &size))
+			if (arg_off(argv[3], &dev, &off, &size, &maxsize))
 				return 1;
 
 			if (argc > 4 && !str2long(argv[4], &pagecount)) {
@@ -641,7 +637,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			rwsize = pagecount * (nand->writesize + nand->oobsize);
 		} else {
 			if (arg_off_size(argc - 3, argv + 3, &dev,
-						&off, &size) != 0)
+						&off, &size, &maxsize) != 0)
 				return 1;
 
 			rwsize = size;
@@ -651,9 +647,11 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		    !strcmp(s, ".e") || !strcmp(s, ".i")) {
 			if (read)
 				ret = nand_read_skip_bad(nand, off, &rwsize,
+							 &actual, maxsize,
 							 (u_char *)addr);
 			else
 				ret = nand_write_skip_bad(nand, off, &rwsize,
+							  &actual, maxsize,
 							  (u_char *)addr, 0);
 #ifdef CONFIG_CMD_NAND_TRIMFFS
 		} else if (!strcmp(s, ".trimffs")) {
@@ -661,8 +659,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 				printf("Unknown nand command suffix '%s'\n", s);
 				return 1;
 			}
-			ret = nand_write_skip_bad(nand, off, &rwsize,
-						(u_char *)addr,
+			ret = nand_write_skip_bad(nand, off, &rwsize, &actual,
+						maxsize, (u_char *)addr,
 						WITH_DROP_FFS);
 #endif
 #ifdef CONFIG_CMD_NAND_YAFFS
@@ -671,8 +669,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 				printf("Unknown nand command suffix '%s'.\n", s);
 				return 1;
 			}
-			ret = nand_write_skip_bad(nand, off, &rwsize,
-						(u_char *)addr,
+			ret = nand_write_skip_bad(nand, off, &rwsize, &actual,
+						maxsize, (u_char *)addr,
 						WITH_INLINE_OOB);
 #endif
 		} else if (!strcmp(s, ".oob")) {
@@ -781,7 +779,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (s && !strcmp(s, ".allexcept"))
 			allexcept = 1;
 
-		if (arg_off_size(argc - 2, argv + 2, &dev, &off, &size) < 0)
+		if (arg_off_size(argc - 2, argv + 2, &dev, &off, &size,
+					&maxsize) < 0)
 			return 1;
 
 		if (!nand_unlock(&nand_info[dev], off, size, allexcept)) {
@@ -862,7 +861,7 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand,
 {
 	int r;
 	char *s;
-	size_t cnt;
+	size_t cnt, actual;
 	image_header_t *hdr;
 #if defined(CONFIG_FIT)
 	const void *fit_hdr = NULL;
@@ -879,7 +878,8 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand,
 	printf("\nLoading from %s, offset 0x%lx\n", nand->name, offset);
 
 	cnt = nand->writesize;
-	r = nand_read_skip_bad(nand, offset, &cnt, (u_char *) addr);
+	r = nand_read_skip_bad(nand, offset, &cnt, &actual, nand->size,
+			(u_char *) addr);
 	if (r) {
 		puts("** Read error\n");
 		bootstage_error(BOOTSTAGE_ID_NAND_HDR_READ);
@@ -911,7 +911,8 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand,
 	}
 	bootstage_mark(BOOTSTAGE_ID_NAND_TYPE);
 
-	r = nand_read_skip_bad(nand, offset, &cnt, (u_char *) addr);
+	r = nand_read_skip_bad(nand, offset, &cnt, &actual, nand->size,
+			(u_char *) addr);
 	if (r) {
 		puts("** Read error\n");
 		bootstage_error(BOOTSTAGE_ID_NAND_READ);
diff --git a/common/env_nand.c b/common/env_nand.c
index 22e72a2..05efbf5 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -266,7 +266,7 @@ int readenv(size_t offset, u_char *buf)
 {
 	size_t end = offset + CONFIG_ENV_RANGE;
 	size_t amount_loaded = 0;
-	size_t blocksize, len;
+	size_t blocksize, len, actual;
 	u_char *char_ptr;
 
 	blocksize = nand_info[0].erasesize;
@@ -281,7 +281,8 @@ int readenv(size_t offset, u_char *buf)
 		} else {
 			char_ptr = &buf[amount_loaded];
 			if (nand_read_skip_bad(&nand_info[0], offset,
-					       &len, char_ptr))
+					       &len, &actual,
+					       nand_info[0].size, char_ptr))
 				return 1;
 
 			offset += blocksize;
diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
index 2ba0c5e..5ed5b1d 100644
--- a/drivers/mtd/nand/nand_util.c
+++ b/drivers/mtd/nand/nand_util.c
@@ -397,34 +397,38 @@ int nand_unlock(struct mtd_info *mtd, loff_t start, size_t length,
  * blocks fits into device
  *
  * @param nand NAND device
- * @param offset offset in flash
+ * @param offsetp offset in flash (on exit offset where it's ending)
  * @param length image length
  * @return 0 if the image fits and there are no bad blocks
  *         1 if the image fits, but there are bad blocks
  *        -1 if the image does not fit
  */
-static int check_skip_len(nand_info_t *nand, loff_t offset, size_t length)
+static int check_skip_len(nand_info_t *nand, loff_t *offset, size_t length)
 {
-	size_t len_excl_bad = 0;
 	int ret = 0;
 
-	while (len_excl_bad < length) {
+	while (length > 0) {
 		size_t block_len, block_off;
 		loff_t block_start;
 
-		if (offset >= nand->size)
+		if (*offset >= nand->size)
 			return -1;
 
-		block_start = offset & ~(loff_t)(nand->erasesize - 1);
-		block_off = offset & (nand->erasesize - 1);
+		block_start = *offset & ~(loff_t)(nand->erasesize - 1);
+		block_off = *offset & (nand->erasesize - 1);
 		block_len = nand->erasesize - block_off;
 
-		if (!nand_block_isbad(nand, block_start))
-			len_excl_bad += block_len;
-		else
+		if (!nand_block_isbad(nand, block_start)) {
+			if (block_len > length) {
+				/* Final chunk is smaller than block. */
+				*offset += length;
+				return ret;
+			} else
+				length -= block_len;
+		} else
 			ret = 1;
 
-		offset += block_len;
+		*offset += block_len;
 	}
 
 	return ret;
@@ -459,22 +463,26 @@ static size_t drop_ffs(const nand_info_t *nand, const u_char *buf,
  * Write image to NAND flash.
  * Blocks that are marked bad are skipped and the is written to the next
  * block instead as long as the image is short enough to fit even after
- * skipping the bad blocks.
+ * skipping the bad blocks.  Note that the actual size needed may exceed
+ * both the length and available NAND due to bad blocks.
  *
  * @param nand  	NAND device
  * @param offset	offset in flash
  * @param length	buffer length
+ * @param actual	size required to write length worth of buffer
+ * @param lim		end location of where data in the buffer may be written.
  * @param buffer        buffer to read from
  * @param flags		flags modifying the behaviour of the write to NAND
  * @return		0 in case of success
  */
 int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
-			u_char *buffer, int flags)
+			size_t *actual, loff_t lim, u_char *buffer, int flags)
 {
 	int rval = 0, blocksize;
 	size_t left_to_write = *length;
 	u_char *p_buffer = buffer;
 	int need_skip;
+	loff_t tmp_offset;
 
 #ifdef CONFIG_CMD_NAND_YAFFS
 	if (flags & WITH_YAFFS_OOB) {
@@ -509,16 +517,25 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 	if ((offset & (nand->writesize - 1)) != 0) {
 		printf("Attempt to write non page-aligned data\n");
 		*length = 0;
+		*actual = 0;
 		return -EINVAL;
 	}
 
-	need_skip = check_skip_len(nand, offset, *length);
+	tmp_offset = offset;
+	need_skip = check_skip_len(nand, &tmp_offset, *length);
+	*actual = tmp_offset;
 	if (need_skip < 0) {
 		printf("Attempt to write outside the flash area\n");
 		*length = 0;
 		return -EINVAL;
 	}
 
+	if (*actual > lim) {
+		puts("Size of write exceeds partition or device limit\n");
+		*length = 0;
+		return -EFBIG;
+	}
+
 	if (!need_skip && !(flags & WITH_DROP_FFS)) {
 		rval = nand_write(nand, offset, length, buffer);
 		if (rval == 0)
@@ -610,35 +627,48 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
  * Read image from NAND flash.
  * Blocks that are marked bad are skipped and the next block is read
  * instead as long as the image is short enough to fit even after skipping the
+ * bad blocks.  Note that the actual size needed may exceed the length due to
  * bad blocks.
  *
  * @param nand NAND device
  * @param offset offset in flash
  * @param length buffer length, on return holds number of read bytes
+ * @param actual size required to read length worth of buffer
+ * @param lim end location of where data in the buffer may be written.
  * @param buffer buffer to write to
  * @return 0 in case of success
  */
 int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
-		       u_char *buffer)
+		       size_t *actual, loff_t lim, u_char *buffer)
 {
 	int rval;
 	size_t left_to_read = *length;
 	u_char *p_buffer = buffer;
 	int need_skip;
+	loff_t tmp_offset;
 
 	if ((offset & (nand->writesize - 1)) != 0) {
 		printf("Attempt to read non page-aligned data\n");
 		*length = 0;
+		*actual = 0;
 		return -EINVAL;
 	}
 
-	need_skip = check_skip_len(nand, offset, *length);
+	tmp_offset = offset;
+	need_skip = check_skip_len(nand, &tmp_offset, *length);
+	*actual = tmp_offset;
 	if (need_skip < 0) {
 		printf("Attempt to read outside the flash area\n");
 		*length = 0;
 		return -EINVAL;
 	}
 
+	if (*actual > lim) {
+		puts("Size of read exceeds partition or device limit\n");
+		*length = 0;
+		return -EFBIG;
+	}
+
 	if (!need_skip) {
 		rval = nand_read(nand, offset, length, buffer);
 		if (!rval || rval == -EUCLEAN)
diff --git a/include/nand.h b/include/nand.h
index dded4e2..f0f3bf9 100644
--- a/include/nand.h
+++ b/include/nand.h
@@ -129,7 +129,7 @@ struct nand_erase_options {
 typedef struct nand_erase_options nand_erase_options_t;
 
 int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
-		       u_char *buffer);
+		       size_t *actual, loff_t lim, u_char *buffer);
 
 #define WITH_YAFFS_OOB	(1 << 0) /* whether write with yaffs format. This flag
 				  * is a 'mode' meaning it cannot be mixed with
@@ -137,7 +137,7 @@ int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 #define WITH_DROP_FFS	(1 << 1) /* drop trailing all-0xff pages */
 
 int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
-			u_char *buffer, int flags);
+			size_t *actual, loff_t lim, u_char *buffer, int flags);
 int nand_erase_opts(nand_info_t *meminfo, const nand_erase_options_t *opts);
 int nand_torture(nand_info_t *nand, loff_t offset);
 
-- 
1.7.9.5

[U-Boot] [PATCH v2 2/4] dfu: NAND specific routines for DFU operation

[Tom Rini]

From: Pantelis Antoniou <panto@antoniou-consulting.com>

Support for NAND storage devices to work with the DFU framework.

---
Changes in v2: Use nand_(read|write)_skip_bad directly rather than
abusing run_command.  Reword a comment in nand_block_op and commit
message.  Initalize bad_skip on start.

Cc: Scott Wood <scottwood@freescale.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
 drivers/dfu/Makefile   |    1 +
 drivers/dfu/dfu.c      |    8 ++
 drivers/dfu/dfu_nand.c |  201 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/dfu.h          |   23 ++++++
 4 files changed, 233 insertions(+)
 create mode 100644 drivers/dfu/dfu_nand.c

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 fb9b417..44d29de 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);
@@ -234,6 +235,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;
 	}
 
@@ -263,6 +266,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;
 	}
 
@@ -285,6 +290,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..e5d39d9
--- /dev/null
+++ b/drivers/dfu/dfu_nand.c
@@ -0,0 +1,201 @@
+/*
+ * 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);
+
+	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 > (start + count)) {
+		printf("%s: skipped %llx bad bytes at %llx\n", __func__,
+				actual - (start + count), start);
+		dfu->bad_skip += (u32)(actual - (start + count));
+	}
+
+	return ret;
+}
+
+static inline int nand_block_write(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
+{
+	return nand_block_op(DFU_OP_WRITE, dfu, offset, buf, len);
+}
+
+static inline int nand_block_read(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
+{
+	return nand_block_op(DFU_OP_READ, dfu, offset, buf, len);
+}
+
+static int dfu_write_medium_nand(struct dfu_entity *dfu,
+		u64 offset, void *buf, long *len)
+{
+	int ret = -1;
+
+	switch (dfu->layout) {
+	case DFU_RAW_ADDR:
+		ret = nand_block_write(dfu, offset, buf, len);
+		break;
+	default:
+		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
+		       dfu_get_layout(dfu->layout));
+	}
+
+	return ret;
+}
+
+static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, void *buf,
+		long *len)
+{
+	int ret = -1;
+
+	switch (dfu->layout) {
+	case DFU_RAW_ADDR:
+		ret = nand_block_read(dfu, offset, buf, len);
+		break;
+	default:
+		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
+		       dfu_get_layout(dfu->layout));
+	}
+
+	return ret;
+}
+
+extern int mtdparts_init(void);
+extern struct part_info* mtd_part_info(struct mtd_device *dev, unsigned int part_num);
+extern int find_dev_and_part(const char *id, struct mtd_device **dev,
+		u8 *part_num, struct part_info **part);
+
+
+int dfu_fill_entity_nand(struct dfu_entity *dfu, char *s)
+{
+	char *st;
+	int ret, dev, part;
+
+	dfu->dev_type = DFU_DEV_NAND;
+	st = strsep(&s, " ");
+	if (!strcmp(st, "raw")) {
+		dfu->layout = DFU_RAW_ADDR;
+		dfu->data.nand.start = simple_strtoul(s, &s, 16);
+		s++;
+		dfu->data.nand.size = simple_strtoul(s, &s, 16);
+	} else if (!strcmp(st, "part")) {
+		char mtd_id[32];
+		struct mtd_device *mtd_dev;
+		u8 part_num;
+		struct part_info *pi;
+
+		dfu->layout = DFU_RAW_ADDR;
+
+		dev = simple_strtoul(s, &s, 10);
+		s++;
+		part = simple_strtoul(s, &s, 10);
+
+		sprintf(mtd_id, "%s%d,%d", "nand", dev, part - 1);
+		printf("using id '%s'\n", mtd_id);
+
+		mtdparts_init();
+
+		ret = find_dev_and_part(mtd_id, &mtd_dev, &part_num, &pi);
+		if (ret != 0) {
+			printf("Could not locate '%s'\n", mtd_id);
+			return -1;
+		}
+
+		dfu->data.nand.start = pi->offset;
+		dfu->data.nand.size = pi->size;
+
+	} else {
+		printf("%s: Memory layout (%s) not supported!\n", __func__, st);
+		return -1;
+	}
+
+	dfu->read_medium = dfu_read_medium_nand;
+	dfu->write_medium = dfu_write_medium_nand;
+
+	/* initial state */
+	dfu->inited = 0;
+
+	return 0;
+}
diff --git a/include/dfu.h b/include/dfu.h
index 9c6b364..86b7d66 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -56,6 +56,15 @@ struct mmc_internal_data {
 	unsigned int part;
 };
 
+struct nand_internal_data {
+	/* RAW programming */
+	u64 start;
+	u64 size;
+
+	unsigned int dev;
+	unsigned int part;
+};
+
 static inline unsigned int get_mmc_blk_size(int dev)
 {
 	return find_mmc_device(dev)->read_bl_len;
@@ -75,6 +84,7 @@ struct dfu_entity {
 
 	union {
 		struct mmc_internal_data mmc;
+		struct nand_internal_data nand;
 	} data;
 
 	int (*read_medium)(struct dfu_entity *dfu,
@@ -95,6 +105,8 @@ struct dfu_entity {
 	long r_left;
 	long b_left;
 
+	u32 bad_skip;	/* for nand use */
+
 	unsigned int inited : 1;
 };
 
@@ -119,4 +131,15 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s)
 	return -1;
 }
 #endif
+
+#ifdef CONFIG_DFU_NAND
+extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *s);
+#else
+static inline int dfu_fill_entity_nand(struct dfu_entity *dfu, char *s)
+{
+	puts("NAND support not available!\n");
+	return -1;
+}
+#endif
+
 #endif /* __DFU_ENTITY_H_ */
-- 
1.7.9.5

[U-Boot] [PATCH v2 3/4] am335x_evm: Add CONFIG_CMD_MTDPARTS and relevant defaults

[Tom Rini]

Signed-off-by: Tom Rini <trini@ti.com>
---
 include/configs/am335x_evm.h |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
index 59647d1..61b861d 100644
--- a/include/configs/am335x_evm.h
+++ b/include/configs/am335x_evm.h
@@ -60,6 +60,8 @@
 	"fdtfile=\0" \
 	"console=ttyO0,115200n8\0" \
 	"optargs=\0" \
+	"mtdids=" MTDIDS_DEFAULT "\0" \
+	"mtdparts=" MTDPARTS_DEFAULT "\0" \
 	"mmcdev=0\0" \
 	"mmcroot=/dev/mmcblk0p2 ro\0" \
 	"mmcrootfstype=ext4 rootwait\0" \
@@ -341,6 +343,13 @@
 /* NAND support */
 #ifdef CONFIG_NAND
 #define CONFIG_CMD_NAND
+#define CONFIG_CMD_MTDPARTS
+#define MTDIDS_DEFAULT			"nand0=omap2-nand.0"
+#define MTDPARTS_DEFAULT		"mtdparts=omap2-nand.0:128k(SPL)," \
+					"128k(SPL.backup1)," \
+					"128k(SPL.backup2)," \
+					"128k(SPL.backup3),1920k(u-boot)," \
+					"128k(u-boot-env),5m(kernel),-(rootfs)"
 #define CONFIG_NAND_OMAP_GPMC
 #define GPMC_NAND_ECC_LP_x16_LAYOUT	1
 #define CONFIG_SYS_NAND_BASE		(0x08000000)	/* physical address */
-- 
1.7.9.5

[U-Boot] [PATCH v2 4/4] am335x_evm: Enable DFU for NAND and MMC, provide example alt_info for both

[Tom Rini]

From: Pantelis Antoniou <panto@antoniou-consulting.com>

- Add CONFIG_DFU_NAND
- Set dfu_alt_info_nand and dfu_alt_info_mmc to show a working example
  for both.
- Increase CONFIG_SYS_MAXARGS due to hush parsing bugs that would
  otherwise disallow 'setenv dfu_alt_info ${dfu_alt_info_nand}'.
- Enable CONFIG_FAT_WRITE to allow updating on MMC

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
 include/configs/am335x_evm.h |   38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
index 61b861d..230e609 100644
--- a/include/configs/am335x_evm.h
+++ b/include/configs/am335x_evm.h
@@ -62,6 +62,8 @@
 	"optargs=\0" \
 	"mtdids=" MTDIDS_DEFAULT "\0" \
 	"mtdparts=" MTDPARTS_DEFAULT "\0" \
+	"dfu_alt_info_mmc=" DFU_ALT_INFO_MMC "\0" \
+	"dfu_alt_info_nand=" DFU_ALT_INFO_NAND "\0" \
 	"mmcdev=0\0" \
 	"mmcroot=/dev/mmcblk0p2 ro\0" \
 	"mmcrootfstype=ext4 rootwait\0" \
@@ -118,8 +120,8 @@
 
 #define CONFIG_CMD_ECHO
 
-/* max number of command args */
-#define CONFIG_SYS_MAXARGS		16
+/* We set the max number of command args high to avoid HUSH bugs. */
+#define CONFIG_SYS_MAXARGS		64
 
 /* Console I/O Buffer Size */
 #define CONFIG_SYS_CBSIZE		512
@@ -148,6 +150,7 @@
 #define CONFIG_CMD_MMC
 #define CONFIG_DOS_PARTITION
 #define CONFIG_CMD_FAT
+#define CONFIG_FAT_WRITE
 #define CONFIG_CMD_EXT2
 
 #define CONFIG_SPI
@@ -158,6 +161,36 @@
 #define CONFIG_CMD_SF
 #define CONFIG_SF_DEFAULT_SPEED		(24000000)
 
+/* USB Composite download gadget - g_dnl */
+#define CONFIG_USB_GADGET
+#define CONFIG_USBDOWNLOAD_GADGET
+
+/* USB TI's IDs */
+#define CONFIG_USBD_HS
+#define CONFIG_G_DNL_VENDOR_NUM 0x0403
+#define CONFIG_G_DNL_PRODUCT_NUM 0xBD00
+#define CONFIG_G_DNL_MANUFACTURER "Texas Instruments"
+
+/* USB Device Firmware Update support */
+#define CONFIG_DFU_FUNCTION
+#define CONFIG_DFU_MMC
+#define CONFIG_DFU_NAND
+#define CONFIG_CMD_DFU
+#define DFU_ALT_INFO_MMC \
+	"boot part 0 1;" \
+	"rootfs part 0 2;" \
+	"MLO fat 0 1;" \
+	"u-boot.img fat 0 1;" \
+	"uEnv.txt fat 0 1"
+#define DFU_ALT_INFO_NAND \
+       "SPL part 0 1;" \
+       "SPL.backup1 part 0 2;" \
+       "SPL.backup2 part 0 3;" \
+       "SPL.backup3 part 0 4;" \
+       "u-boot part 0 5;" \
+       "kernel part 0 7;" \
+       "rootfs part 0 8"
+
  /* Physical Memory Map */
 #define CONFIG_NR_DRAM_BANKS		1		/*  1 bank of DRAM */
 #define PHYS_DRAM_1			0x80000000	/* DRAM Bank #1 */
@@ -302,6 +335,7 @@
 #define CONFIG_MUSB_GADGET
 #define CONFIG_MUSB_PIO_ONLY
 #define CONFIG_USB_GADGET_DUALSPEED
+#define CONFIG_USB_GADGET_VBUS_DRAW	2
 #define CONFIG_MUSB_HOST
 #define CONFIG_AM335X_USB0
 #define CONFIG_AM335X_USB0_MODE	MUSB_PERIPHERAL
-- 
1.7.9.5

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Scott Wood]

  On 02/26/2013 09:56:08 AM, Tom Rini wrote:
> We make these two functions take a size_t pointer to how much space
> was used on NAND to read or write the buffer (when reads/writes  
> happen)
> so that bad blocks can be accounted for.  We also make them take an
> loff_t limit on how much data can be read or written.  This means that
> we can now catch the case of when writing to a partition would exceed
> the partition size due to bad blocks.  To do this we also need to make
> check_skip_len care about total actual size used rather than  
> block_size
> chunks used.  All callers of nand_(read|write)_skip_bad are adjusted  
> to
> call these with the most sensible limits available.
> 
> The changes were started by Pantelis and finished by Tom.
> 
> Cc: Scott Wood <scottwood@freescale.com>
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>  common/cmd_nand.c            |   61  
> +++++++++++++++++++++--------------------
>  common/env_nand.c            |    5 ++--
>  drivers/mtd/nand/nand_util.c |   62  
> +++++++++++++++++++++++++++++++-----------
>  include/nand.h               |    4 +--
>  4 files changed, 82 insertions(+), 50 deletions(-)
> 
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> index 1568594..e091e02 100644
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -137,7 +137,8 @@ static inline int str2long(const char *p, ulong  
> *num)
>  	return *p != '\0' && *endptr == '\0';
>  }
> 
> -static int get_part(const char *partname, int *idx, loff_t *off,  
> loff_t *size)
> +static int get_part(const char *partname, int *idx, loff_t *off,  
> loff_t *size,
> +		loff_t *maxsize)
>  {
>  #ifdef CONFIG_CMD_MTDPARTS
>  	struct mtd_device *dev;
> @@ -160,6 +161,7 @@ static int get_part(const char *partname, int  
> *idx, loff_t *off, loff_t *size)
> 
>  	*off = part->offset;
>  	*size = part->size;
> +	*maxsize = part->offset + part->size;
>  	*idx = dev->id->num;

The name "maxsize" suggests that it's a size, not a position.

> 
>  	ret = set_dev(*idx);
> @@ -173,10 +175,11 @@ static int get_part(const char *partname, int  
> *idx, loff_t *off, loff_t *size)
>  #endif
>  }
> 
> -static int arg_off(const char *arg, int *idx, loff_t *off, loff_t  
> *maxsize)
> +static int arg_off(const char *arg, int *idx, loff_t *off, loff_t  
> *size,
> +		loff_t *maxsize)
>  {
>  	if (!str2off(arg, off))
> -		return get_part(arg, idx, off, maxsize);
> +		return get_part(arg, idx, off, size, maxsize);
> 
>  	if (*off >= nand_info[*idx].size) {
>  		puts("Offset exceeds device limit\n");

...and in the get_part case arg-off is still treating maxsize as a size.

> @@ -605,7 +601,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  	}
> 
>  	if (strncmp(cmd, "read", 4) == 0 || strncmp(cmd, "write", 5) ==  
> 0) {
> -		size_t rwsize;
> +		size_t rwsize, actual;
>  		ulong pagecount = 1;
>  		int read;
>  		int raw;
> @@ -625,7 +621,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  		if (s && !strcmp(s, ".raw")) {
>  			raw = 1;
> 
> -			if (arg_off(argv[3], &dev, &off, &size))
> +			if (arg_off(argv[3], &dev, &off, &size,  
> &maxsize))
>  				return 1;
> 
>  			if (argc > 4 && !str2long(argv[4], &pagecount))  
> {
> @@ -641,7 +637,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  			rwsize = pagecount * (nand->writesize +  
> nand->oobsize);
>  		} else {
>  			if (arg_off_size(argc - 3, argv + 3, &dev,
> -						&off, &size) != 0)
> +						&off, &size, &maxsize)  
> != 0)
>  				return 1;
> 
>  			rwsize = size;
> @@ -651,9 +647,11 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  		    !strcmp(s, ".e") || !strcmp(s, ".i")) {
>  			if (read)
>  				ret = nand_read_skip_bad(nand, off,  
> &rwsize,
> +							 &actual,  
> maxsize,
>  							 (u_char  
> *)addr);
>  			else
>  				ret = nand_write_skip_bad(nand, off,  
> &rwsize,
> +							  &actual,  
> maxsize,
>  							  (u_char  
> *)addr, 0);
>  #ifdef CONFIG_CMD_NAND_TRIMFFS
>  		} else if (!strcmp(s, ".trimffs")) {
> @@ -661,8 +659,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  				printf("Unknown nand command suffix  
> '%s'\n", s);
>  				return 1;
>  			}
> -			ret = nand_write_skip_bad(nand, off, &rwsize,
> -						(u_char *)addr,
> +			ret = nand_write_skip_bad(nand, off, &rwsize,  
> &actual,
> +						maxsize, (u_char *)addr,
>  						WITH_DROP_FFS);

Do we care about actual here?  Let the skip_bad functions accept NULL  
if the caller doesn't care.

> diff --git a/drivers/mtd/nand/nand_util.c  
> b/drivers/mtd/nand/nand_util.c
> index 2ba0c5e..5ed5b1d 100644
> --- a/drivers/mtd/nand/nand_util.c
> +++ b/drivers/mtd/nand/nand_util.c
> @@ -397,34 +397,38 @@ int nand_unlock(struct mtd_info *mtd, loff_t  
> start, size_t length,
>   * blocks fits into device
>   *
>   * @param nand NAND device
> - * @param offset offset in flash
> + * @param offsetp offset in flash (on exit offset where it's ending)
>   * @param length image length
>   * @return 0 if the image fits and there are no bad blocks
>   *         1 if the image fits, but there are bad blocks
>   *        -1 if the image does not fit
>   */
> -static int check_skip_len(nand_info_t *nand, loff_t offset, size_t  
> length)
> +static int check_skip_len(nand_info_t *nand, loff_t *offset, size_t  
> length)

Comment changed "offset" to "offsetp" but code did not.

Can we use a different parameter to return the end offset (or actual  
size)?  That way we don't need the tmp_offset stuff,
and there should be fewer changes to this function.

>  {
> -	size_t len_excl_bad = 0;
>  	int ret = 0;
> 
> -	while (len_excl_bad < length) {
> +	while (length > 0) {
>  		size_t block_len, block_off;
>  		loff_t block_start;
> 
> -		if (offset >= nand->size)
> +		if (*offset >= nand->size)
>  			return -1;
> 
> -		block_start = offset & ~(loff_t)(nand->erasesize - 1);
> -		block_off = offset & (nand->erasesize - 1);
> +		block_start = *offset & ~(loff_t)(nand->erasesize - 1);
> +		block_off = *offset & (nand->erasesize - 1);
>  		block_len = nand->erasesize - block_off;
> 
> -		if (!nand_block_isbad(nand, block_start))
> -			len_excl_bad += block_len;
> -		else
> +		if (!nand_block_isbad(nand, block_start)) {
> +			if (block_len > length) {
> +				/* Final chunk is smaller than block. */
> +				*offset += length;
> +				return ret;
> +			} else
> +				length -= block_len;
> +		} else
>  			ret = 1;

Traditionally U-Boot style has been to use braces on both sides of an  
if/else if one side needs them.

> -		offset += block_len;
> +		*offset += block_len;
>  	}
> 
>  	return ret;
> @@ -459,22 +463,26 @@ static size_t drop_ffs(const nand_info_t *nand,  
> const u_char *buf,
>   * Write image to NAND flash.
>   * Blocks that are marked bad are skipped and the is written to the  
> next
>   * block instead as long as the image is short enough to fit even  
> after
> - * skipping the bad blocks.
> + * skipping the bad blocks.  Note that the actual size needed may  
> exceed
> + * both the length and available NAND due to bad blocks.

If that happens, then the function returns failure.  Are the contents  
of "actual" well-defined when the function returns failure?

>   *
>   * @param nand  	NAND device
>   * @param offset	offset in flash
>   * @param length	buffer length
> + * @param actual	size required to write length worth of buffer
> + * @param lim		end location of where data in the  
> buffer may be written.
>   * @param buffer        buffer to read from
>   * @param flags		flags modifying the behaviour of the  
> write to NAND
>   * @return		0 in case of success
>   */

Please note which pointer parameters are in/out versus out-only.

>  int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t  
> *length,
> -			u_char *buffer, int flags)
> +			size_t *actual, loff_t lim, u_char *buffer, int  
> flags)
>  {
>  	int rval = 0, blocksize;
>  	size_t left_to_write = *length;
>  	u_char *p_buffer = buffer;
>  	int need_skip;
> +	loff_t tmp_offset;
> 
>  #ifdef CONFIG_CMD_NAND_YAFFS
>  	if (flags & WITH_YAFFS_OOB) {
> @@ -509,16 +517,25 @@ int nand_write_skip_bad(nand_info_t *nand,  
> loff_t offset, size_t *length,
>  	if ((offset & (nand->writesize - 1)) != 0) {
>  		printf("Attempt to write non page-aligned data\n");
>  		*length = 0;
> +		*actual = 0;
>  		return -EINVAL;
>  	}
> 
> -	need_skip = check_skip_len(nand, offset, *length);
> +	tmp_offset = offset;
> +	need_skip = check_skip_len(nand, &tmp_offset, *length);
> +	*actual = tmp_offset;

More size/offset mismatch with actual.  Docs above say it's a size.

-Scott

[U-Boot] [PATCH v2 3/4] am335x_evm: Add CONFIG_CMD_MTDPARTS and relevant defaults

[Peter Korsgaard]

>>>>> "Tom" == Tom Rini <trini@ti.com> writes:

 Tom> Signed-off-by: Tom Rini <trini@ti.com>
 Tom> ---
 Tom>  include/configs/am335x_evm.h |    9 +++++++++
 Tom>  1 file changed, 9 insertions(+)

 Tom> diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
 Tom> index 59647d1..61b861d 100644
 Tom> --- a/include/configs/am335x_evm.h
 Tom> +++ b/include/configs/am335x_evm.h
 Tom> @@ -60,6 +60,8 @@
 Tom>  	"fdtfile=\0" \
 Tom>  	"console=ttyO0,115200n8\0" \
 Tom>  	"optargs=\0" \
 Tom> +	"mtdids=" MTDIDS_DEFAULT "\0" \
 Tom> +	"mtdparts=" MTDPARTS_DEFAULT "\0" \
 Tom>  	"mmcdev=0\0" \
 Tom>  	"mmcroot=/dev/mmcblk0p2 ro\0" \
 Tom>  	"mmcrootfstype=ext4 rootwait\0" \
 Tom> @@ -341,6 +343,13 @@
 Tom>  /* NAND support */
 Tom>  #ifdef CONFIG_NAND
 Tom>  #define CONFIG_CMD_NAND
 Tom> +#define CONFIG_CMD_MTDPARTS
 Tom> +#define MTDIDS_DEFAULT			"nand0=omap2-nand.0"
 Tom> +#define MTDPARTS_DEFAULT		"mtdparts=omap2-nand.0:128k(SPL)," \
 Tom> +					"128k(SPL.backup1)," \
 Tom> +					"128k(SPL.backup2)," \
 Tom> +					"128k(SPL.backup3),1920k(u-boot)," \
 Tom> +					"128k(u-boot-env),5m(kernel),-(rootfs)"

Is there a particular reason why the u-boot partition is so big?

-- 
Bye, Peter Korsgaard

[U-Boot] [PATCH v2 3/4] am335x_evm: Add CONFIG_CMD_MTDPARTS and relevant defaults

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/27/2013 03:54 AM, Peter Korsgaard wrote:
>>>>>> "Tom" == Tom Rini <trini@ti.com> writes:
> 
> Tom> Signed-off-by: Tom Rini <trini@ti.com> Tom> --- Tom>
> include/configs/am335x_evm.h |    9 +++++++++ Tom>  1 file changed,
> 9 insertions(+)
> 
> Tom> diff --git a/include/configs/am335x_evm.h
> b/include/configs/am335x_evm.h Tom> index 59647d1..61b861d 100644 
> Tom> --- a/include/configs/am335x_evm.h Tom> +++
> b/include/configs/am335x_evm.h Tom> @@ -60,6 +60,8 @@ Tom>
> "fdtfile=\0" \ Tom>  	"console=ttyO0,115200n8\0" \ Tom>
> "optargs=\0" \ Tom> +	"mtdids=" MTDIDS_DEFAULT "\0" \ Tom> +
> "mtdparts=" MTDPARTS_DEFAULT "\0" \ Tom>  	"mmcdev=0\0" \ Tom>
> "mmcroot=/dev/mmcblk0p2 ro\0" \ Tom>  	"mmcrootfstype=ext4
> rootwait\0" \ Tom> @@ -341,6 +343,13 @@ Tom>  /* NAND support */ 
> Tom>  #ifdef CONFIG_NAND Tom>  #define CONFIG_CMD_NAND Tom>
> +#define CONFIG_CMD_MTDPARTS Tom> +#define MTDIDS_DEFAULT
> "nand0=omap2-nand.0" Tom> +#define MTDPARTS_DEFAULT
> "mtdparts=omap2-nand.0:128k(SPL)," \ Tom> +
> "128k(SPL.backup1)," \ Tom> +					"128k(SPL.backup2)," \ Tom> +
> "128k(SPL.backup3),1920k(u-boot)," \ Tom> +
> "128k(u-boot-env),5m(kernel),-(rootfs)"
> 
> Is there a particular reason why the u-boot partition is so big?

Convention (which would allow for redundant copies of U-Boot within
this area) I believe.  Same as some of the semi-related boards.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRLghGAAoJENk4IS6UOR1WU20QAJZdE9KI282/72AA6i47SDzG
G1TLymHDOddNBp2ylcjyxkdWNKRxk348Z/bGJ25VIv+jcbRv/SubTnyXsT1RkFX2
qxzJU5/hsMqEQ5++jVbAjph0zBCBFYR68XJ7r4vjLO0N9/eJKR/zNkxqMeQMMY2j
LVsdju2DfRYAWn9d8CuM2vRrf5iscK3PB6AEqO4SafMtHCxkzLScmAsvQMEUekkZ
sWkYRVnLEbSmdSnRdnHi0Yt0jiMSULVoGelMbyOA+I9FEsgQaaY0S4Sy7YOOR6ml
Ajkv8j7TZvHsOZRponsxv6dsJq9CZlnzlSLrhrXi1EjAKnSlUukF6D4mWdkMQnXX
MoX2o1ICLL5qti1fqlv6f+JWeT5a0PydWRHy9Y8+g3kZCRcXFLKwrdVb1F/x7AVt
5z1g6f9QEzw9CoJZDSRSLnc2+wdntsZs4KGGeoWFfqzQfej5eW11j43Is23zNkys
XB+6EILlotNttLoHJ2SzcHUG5dRKJ+I6sCuVDtWd90Bw7uFO1i9p+uwYcNhOiDXz
qbxu11ZSvH8A7/XD5vfdsDmStGl7+SMikO/rY0YHrS575AWcbOC4tcUaJtSHpyMD
nUDOsQ25G59pdA0+B2CoY22nbRM4tRpQA5S+jlSuAtYpPL5GcSifE6zXvO96zX9D
lUzhyJmHgxs8/caWWgFe
=NVWs
-----END PGP SIGNATURE-----

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/26/2013 09:08 PM, Scott Wood wrote:
> On 02/26/2013 09:56:08 AM, Tom Rini wrote:
>> We make these two functions take a size_t pointer to how much 
>> space was used on NAND to read or write the buffer (when 
>> reads/writes happen) so that bad blocks can be accounted for.
>> We also make them take an loff_t limit on how much data can be
>> read or written.  This means that we can now catch the case of
>> when writing to a partition would exceed the partition size due
>> to bad blocks.  To do this we also need to make check_skip_len
>> care about total actual size used rather than block_size chunks
>> used. All callers of nand_(read|write)_skip_bad are adjusted to
>> call these with the most sensible limits available.
>> 
>> The changes were started by Pantelis and finished by Tom.
>> 
>> Cc: Scott Wood <scottwood@freescale.com> Signed-off-by: Pantelis 
>> Antoniou <panto@antoniou-consulting.com> Signed-off-by: Tom Rini 
>> <trini@ti.com> --- common/cmd_nand.c            |   61 
>> +++++++++++++++++++++-------------------- common/env_nand.c |
>> 5 ++-- drivers/mtd/nand/nand_util.c |   62 
>> +++++++++++++++++++++++++++++++----------- include/nand.h |    4
>> +-- 4 files changed, 82 insertions(+), 50 deletions(-)
>> 
>> diff --git a/common/cmd_nand.c b/common/cmd_nand.c index 
>> 1568594..e091e02 100644 --- a/common/cmd_nand.c +++ 
>> b/common/cmd_nand.c @@ -137,7 +137,8 @@ static inline int 
>> str2long(const char *p, ulong *num) return *p != '\0' && *endptr 
>> == '\0'; }
>> 
>> -static int get_part(const char *partname, int *idx, loff_t *off,
>> loff_t *size) +static int get_part(const char *partname, int
>> *idx, loff_t *off, loff_t *size, +        loff_t *maxsize) { 
>> #ifdef CONFIG_CMD_MTDPARTS struct mtd_device *dev; @@ -160,6 
>> +161,7 @@ static int get_part(const char *partname, int *idx, 
>> loff_t *off, loff_t *size)
>> 
>> *off = part->offset; *size = part->size; +    *maxsize = 
>> part->offset + part->size; *idx = dev->id->num;
> 
> The name "maxsize" suggests that it's a size, not a position.

OK, I'll call it maxoff (because it's the max offset within the NAND
for a given partition, or end of the NAND).

>> ret = set_dev(*idx); @@ -173,10 +175,11 @@ static int 
>> get_part(const char *partname, int *idx, loff_t *off, loff_t 
>> *size) #endif }
>> 
>> -static int arg_off(const char *arg, int *idx, loff_t *off, 
>> loff_t *maxsize) +static int arg_off(const char *arg, int *idx, 
>> loff_t *off, loff_t *size, +        loff_t *maxsize) { if 
>> (!str2off(arg, off)) -        return get_part(arg, idx, off, 
>> maxsize); +        return get_part(arg, idx, off, size, 
>> maxsize);
>> 
>> if (*off >= nand_info[*idx].size) { puts("Offset exceeds device 
>> limit\n");
> 
> ...and in the get_part case arg-off is still treating maxsize as a 
> size.

OK, bug here then I missed.  Making sure both *size and *maxoff are
set when get_part doesn't set them.

[snip]
>> -            ret = nand_write_skip_bad(nand, off, &rwsize, - 
>> (u_char *)addr, +            ret = nand_write_skip_bad(nand,
>> off, &rwsize, &actual, +                        maxsize, (u_char 
>> *)addr, WITH_DROP_FFS);
> 
> Do we care about actual here?  Let the skip_bad functions accept 
> NULL if the caller doesn't care.

Will make it so.

>> diff --git a/drivers/mtd/nand/nand_util.c 
>> b/drivers/mtd/nand/nand_util.c index 2ba0c5e..5ed5b1d 100644 --- 
>> a/drivers/mtd/nand/nand_util.c +++ b/drivers/mtd/nand/nand_util.c
>> @@ -397,34 +397,38 @@ int nand_unlock(struct mtd_info *mtd,
>> loff_t start, size_t length, * blocks fits into device * * @param
>> nand NAND device - * @param offset offset in flash + * @param
>> offsetp offset in flash (on exit offset where it's ending) *
>> @param length image length * @return 0 if the image fits and
>> there are no bad blocks * 1 if the image fits, but there are bad
>> blocks *        -1 if the image does not fit */ -static int
>> check_skip_len(nand_info_t *nand, loff_t offset, size_t length)
>> +static int check_skip_len(nand_info_t *nand, loff_t *offset,
>> size_t length)
> 
> Comment changed "offset" to "offsetp" but code did not.

Oops.

> Can we use a different parameter to return the end offset (or 
> actual size)?  That way we don't need the tmp_offset stuff, and 
> there should be fewer changes to this function.

First, the big changes to the function are so that we track (and
report back) the correct amount of a partial block we would use for
the request.  I'll see if I can't do something like loff_t offset,
*something else.

[snip]
> Traditionally U-Boot style has been to use braces on both sides of 
> an if/else if one side needs them.

OK, fixing.

> 
>> -        offset += block_len; +        *offset += block_len; }
>> 
>> return ret; @@ -459,22 +463,26 @@ static size_t drop_ffs(const 
>> nand_info_t *nand, const u_char *buf, * Write image to NAND 
>> flash. * Blocks that are marked bad are skipped and the is 
>> written to the next * block instead as long as the image is
>> short enough to fit even after - * skipping the bad blocks. + * 
>> skipping the bad blocks.  Note that the actual size needed may 
>> exceed + * both the length and available NAND due to bad blocks.
> 
> If that happens, then the function returns failure.  Are the 
> contents of "actual" well-defined when the function returns 
> failure?

They are as well defined as what happens with length.  If we say we
can't write, we set both to 0 and return an error.  I'll take this as
a request to expand the comment and do so.

>> * * @param nand      NAND device * @param offset    offset in 
>> flash * @param length    buffer length + * @param actual    size 
>> required to write length worth of buffer + * @param lim end
>> location of where data in the buffer may be written. * @param 
>> buffer        buffer to read from * @param flags        flags 
>> modifying the behaviour of the write to NAND * @return        0 
>> in case of success */
> 
> Please note which pointer parameters are in/out versus out-only.

I think I follow, I'll re-word.

>> int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t
>>  *length, -            u_char *buffer, int flags) + size_t
>> *actual, loff_t lim, u_char *buffer, int flags) { int rval = 0,
>> blocksize; size_t left_to_write = *length; u_char *p_buffer =
>> buffer; int need_skip; +    loff_t tmp_offset;
>> 
>> #ifdef CONFIG_CMD_NAND_YAFFS if (flags & WITH_YAFFS_OOB) { @@ 
>> -509,16 +517,25 @@ int nand_write_skip_bad(nand_info_t *nand, 
>> loff_t offset, size_t *length, if ((offset & (nand->writesize - 
>> 1)) != 0) { printf("Attempt to write non page-aligned data\n"); 
>> *length = 0; +        *actual = 0; return -EINVAL; }
>> 
>> -    need_skip = check_skip_len(nand, offset, *length); + 
>> tmp_offset = offset; +    need_skip = check_skip_len(nand, 
>> &tmp_offset, *length); +    *actual = tmp_offset;
> 
> More size/offset mismatch with actual.  Docs above say it's a 
> size.

I guess I sacrificed continence for clarity here.  The "issue" is that
we walk blocks from *offset until we've fit in length.  Another way of
doing this and not muddying the type-waters is starting to stare me in
the face now, so I'll go see about re-working things.  Thanks!

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRLhYiAAoJENk4IS6UOR1WxMcP/jQKxJlm6/f24eF/RQ3Q2l/B
RfCu67cdIg3MKX/B9RHyXRIRRCsDwWq/PKFRWXdOMKeirhposKqfDxc8r/s/MpiP
GL+x4hiunH6q+Ct4ZkVg9tBY90+F+UQc7cHsxjAQmHK3gjR+0ODdmk7iHg0MzNEB
4MhASBYfSnfLY7yk0/+Lq4UktULzyalq1aR653DvFhX1Gn9bso4eUlXRjEen2+22
nEfIGHtKQV1CNK6rGWFgyNIjvjFZqtHtz/gjFDEqr/xPynCROXm+dF7LNnSlVF+4
7SzxRdEHqTsBj/1H6sEZsw3NzA56aFLlrsUUiv5cbeaHDsvOCDvqxoDfqXg9U1t3
+5Jw8ctdiDk1uh4ARFKow3DmGKW/7pTDmQNz/zLNUusXOY6KhsPEraDSZeS62hnM
RpM5fWnMivFLOGcif5ELN6MHPGntTJ+J39L8ABRNPouFV6BFHNqHqC4+7PrJ4BsG
ALRNdcrk1IpglehcxJTBd/GliKkT8GbMQv9chlZOAO+t/ND20SX1HTDElHH1reEf
cXYRjt/UUW43HQa5CNvM1pq4uSvBkWS/2aGR0Wzrs/ZoLcbssW3xIBdTcTgRWSMG
NbM69czKVRUS4gDt451akASONXbpnm91CKLsg31jYIGphpw4WBr+IT+VZGsl1pgC
Bz73jBNxHA4VYK3vJJ9W
=gAJe
-----END PGP SIGNATURE-----

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini]

On Tue, Feb 26, 2013 at 10:56:08AM -0500, Tom Rini wrote:

> We make these two functions take a size_t pointer to how much space
> was used on NAND to read or write the buffer (when reads/writes happen)
> so that bad blocks can be accounted for.  We also make them take an
> loff_t limit on how much data can be read or written.  This means that
> we can now catch the case of when writing to a partition would exceed
> the partition size due to bad blocks.  To do this we also need to make
[snip]
>  int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
> -		       u_char *buffer)
> +		       size_t *actual, loff_t lim, u_char *buffer)
[snip]
> +	if (*actual > lim) {
> +		puts("Size of read exceeds partition or device limit\n");
> +		*length = 0;
> +		return -EFBIG;
> +	}

The more I look at this and try testing things, I think I shouldn't be
introducing a change here.  Before you could do:
nand read ${address} partname-with-badblock

And it would suceed but bleed into the next partition if it wasn't the
last one.  So your production system could do "nand read ${address}
kernel" and be OK.  But with this change, it would fail because reading
the whole partition is now too large with a bad block (you would need
partition+(blocksize*number bad blocks).

So I'm going to put this back to a check simply against requested size
being greater than lim rather than required size greater than lim (since
required size exceeds device is still caught).

-- 
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/20130227/b62f0136/attachment.pgp>

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Scott Wood]

On 02/27/2013 10:49:55 AM, Tom Rini wrote:
> On Tue, Feb 26, 2013 at 10:56:08AM -0500, Tom Rini wrote:
> 
> > We make these two functions take a size_t pointer to how much space
> > was used on NAND to read or write the buffer (when reads/writes  
> happen)
> > so that bad blocks can be accounted for.  We also make them take an
> > loff_t limit on how much data can be read or written.  This means  
> that
> > we can now catch the case of when writing to a partition would  
> exceed
> > the partition size due to bad blocks.  To do this we also need to  
> make
> [snip]
> >  int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t  
> *length,
> > -		       u_char *buffer)
> > +		       size_t *actual, loff_t lim, u_char *buffer)
> [snip]
> > +	if (*actual > lim) {
> > +		puts("Size of read exceeds partition or device  
> limit\n");
> > +		*length = 0;
> > +		return -EFBIG;
> > +	}
> 
> The more I look at this and try testing things, I think I shouldn't be
> introducing a change here.  Before you could do:
> nand read ${address} partname-with-badblock
> 
> And it would suceed but bleed into the next partition if it wasn't the
> last one.  So your production system could do "nand read ${address}
> kernel" and be OK.  But with this change, it would fail because  
> reading
> the whole partition is now too large with a bad block (you would need
> partition+(blocksize*number bad blocks).

You wouldn't be quite so OK if it were a write instead.

> So I'm going to put this back to a check simply against requested size
> being greater than lim rather than required size greater than lim  
> (since
> required size exceeds device is still caught).

No, please retain the check.  The other issue is a separate (though  
related) bug, and there's a patch from Harvey Chapman to deal with it.

-Scott

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/27/2013 12:09 PM, Scott Wood wrote:
> On 02/27/2013 10:49:55 AM, Tom Rini wrote:
>> On Tue, Feb 26, 2013 at 10:56:08AM -0500, Tom Rini wrote:
>> 
>>> We make these two functions take a size_t pointer to how much
>>> space was used on NAND to read or write the buffer (when
>>> reads/writes happen) so that bad blocks can be accounted for.
>>> We also make them take an loff_t limit on how much data can be
>>> read or written.  This means that we can now catch the case of
>>> when writing to a partition would exceed the partition size due
>>> to bad blocks.  To do this we also need to make
>> [snip]
>>> int nand_read_skip_bad(nand_info_t *nand, loff_t offset,
>>> size_t
>> *length,
>>> -               u_char *buffer) +               size_t *actual,
>>> loff_t lim, u_char *buffer)
>> [snip]
>>> +    if (*actual > lim) { +        puts("Size of read exceeds
>>> partition or device limit\n"); +        *length = 0; +
>>> return -EFBIG; +    }
>> 
>> The more I look at this and try testing things, I think I
>> shouldn't be introducing a change here.  Before you could do: 
>> nand read ${address} partname-with-badblock
>> 
>> And it would suceed but bleed into the next partition if it
>> wasn't the last one.  So your production system could do "nand
>> read ${address} kernel" and be OK.  But with this change, it
>> would fail because reading the whole partition is now too large
>> with a bad block (you would need partition+(blocksize*number bad
>> blocks).
> 
> You wouldn't be quite so OK if it were a write instead.

Correct.  But the check is now inside of nand_(read|write)_skip_bad.
So the write case will fail (without trying to write), but the read
case should not.

>> So I'm going to put this back to a check simply against requested
>> size being greater than lim rather than required size greater
>> than lim (since required size exceeds device is still caught).
> 
> No, please retain the check.  The other issue is a separate
> (though related) bug, and there's a patch from Harvey Chapman to
> deal with it.

I could be missing something, but I'm not sure how to otherwise adjust
things here.  With all of the checking moved to nand_util.c and
check_skip_len not knowing if we're a read or write it can only say
"fits using X" or "exceeds device", then it's up to the caller to
decide the next action.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRLj+TAAoJENk4IS6UOR1WJAAP/RLxsGHN4HqoEC0zfe2eIDZe
ZJU+sevT/murom6fNOfKFuC8pwPavy64FGdXNspwdpJCRTcXIs503k4x4ZMy9USC
HszV9NKF42HyscQi3RmBqlIUb9WQ6UikMZEUpteO0ZGQajRKho3nL3M1qjQsASG9
745qrmJijQEjEu3MsnfEZYEzyliVCpHHiIylhBALizwpjEwzKwlebe/ZrmbTPYO6
QXyfHLd+/5iNUJEjXOaXP/z7gXU+85m2uwxRhyLB1PmULGkmh7A+cvByTPt8TcxK
HTWnL3at4S+OvWqlnYTdTYbsCBUQ3jp/wnLF39vq9E5VIPv7UshwPponeNbFEGzt
GOZP1fIGMWcSFJHd3usR7wkhA5tJZu9329Av283ckwIbSlwz/tOP/efwFF/Zsrc5
oLn4ezRrA+RSKCtCujYJPhnAyEJEkncvpA4Imx6R9v9LZlXu3z8w5AQKcuAlsrxm
FDcjtcAA6y804I2jGPvL9laoCy9li1SVMHbd71zVtRwRJBMYUMfw7aI6uUwBslLJ
4GlnVKKsfEAkEb7APZ5v5YJrwD+mgSJ4TR1fwtoirbQKpG3OrMnXNhzZSBD5UFv6
DUxhMRL+EOawFJIl5AUPHHB7LdvV4qtDwee1bPg31ZXetnimQGLHn8qzppioMIvC
suvFme3/03ZGTDFwUUjp
=My8j
-----END PGP SIGNATURE-----

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini]

On Wed, Feb 27, 2013 at 12:17:07PM -0500, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/27/2013 12:09 PM, Scott Wood wrote:
> > On 02/27/2013 10:49:55 AM, Tom Rini wrote:
> >> On Tue, Feb 26, 2013 at 10:56:08AM -0500, Tom Rini wrote:
> >> 
> >>> We make these two functions take a size_t pointer to how much
> >>> space was used on NAND to read or write the buffer (when
> >>> reads/writes happen) so that bad blocks can be accounted for.
> >>> We also make them take an loff_t limit on how much data can be
> >>> read or written.  This means that we can now catch the case of
> >>> when writing to a partition would exceed the partition size due
> >>> to bad blocks.  To do this we also need to make
> >> [snip]
> >>> int nand_read_skip_bad(nand_info_t *nand, loff_t offset,
> >>> size_t
> >> *length,
> >>> -               u_char *buffer) +               size_t *actual,
> >>> loff_t lim, u_char *buffer)
> >> [snip]
> >>> +    if (*actual > lim) { +        puts("Size of read exceeds
> >>> partition or device limit\n"); +        *length = 0; +
> >>> return -EFBIG; +    }
> >> 
> >> The more I look at this and try testing things, I think I
> >> shouldn't be introducing a change here.  Before you could do: 
> >> nand read ${address} partname-with-badblock
> >> 
> >> And it would suceed but bleed into the next partition if it
> >> wasn't the last one.  So your production system could do "nand
> >> read ${address} kernel" and be OK.  But with this change, it
> >> would fail because reading the whole partition is now too large
> >> with a bad block (you would need partition+(blocksize*number bad
> >> blocks).
> > 
> > You wouldn't be quite so OK if it were a write instead.
> 
> Correct.  But the check is now inside of nand_(read|write)_skip_bad.
> So the write case will fail (without trying to write), but the read
> case should not.
> 
> >> So I'm going to put this back to a check simply against requested
> >> size being greater than lim rather than required size greater
> >> than lim (since required size exceeds device is still caught).
> > 
> > No, please retain the check.  The other issue is a separate
> > (though related) bug, and there's a patch from Harvey Chapman to
> > deal with it.
> 
> I could be missing something, but I'm not sure how to otherwise adjust
> things here.  With all of the checking moved to nand_util.c and
> check_skip_len not knowing if we're a read or write it can only say
> "fits using X" or "exceeds device", then it's up to the caller to
> decide the next action.

OK, I see it now.  Yes, I think they should be able to live together
nicely.

-- 
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/20130227/90d6f19c/attachment.pgp>

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Scott Wood]

On 02/27/2013 08:20:19 AM, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/26/2013 09:08 PM, Scott Wood wrote:
> > The name "maxsize" suggests that it's a size, not a position.
> 
> OK, I'll call it maxoff (because it's the max offset within the NAND
> for a given partition, or end of the NAND).

Wouldn't it be less intrusive to just make it actually be the size  
instead of an offset?

> >> -        offset += block_len; +        *offset += block_len; }
> >>
> >> return ret; @@ -459,22 +463,26 @@ static size_t drop_ffs(const
> >> nand_info_t *nand, const u_char *buf, * Write image to NAND
> >> flash. * Blocks that are marked bad are skipped and the is
> >> written to the next * block instead as long as the image is
> >> short enough to fit even after - * skipping the bad blocks. + *
> >> skipping the bad blocks.  Note that the actual size needed may
> >> exceed + * both the length and available NAND due to bad blocks.
> >
> > If that happens, then the function returns failure.  Are the
> > contents of "actual" well-defined when the function returns
> > failure?
> 
> They are as well defined as what happens with length.  If we say we
> can't write, we set both to 0 and return an error.  I'll take this as
> a request to expand the comment and do so.

The comments could use expanding (it doesn't even explain what happens  
to length in the non-error case), but also it looks like there are some  
error paths where actual doesn't get cleared, in the  
CONFIG_CMD_NAND_YAFFS section.

I was also wondering about the case where check_skip_bad() says it  
doesn't fit.  It doesn't return the actual in that case -- it returns  
offset as of when it stopped.  So a caller of  
nand_read|write_skip_bad() would see the same "actual" as if it just  
barely fit.  I'm not sure that this function ever would return an  
actual size that exceeds the available NAND.

-Scott

[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/27/2013 05:04 PM, Scott Wood wrote:
> On 02/27/2013 08:20:19 AM, Tom Rini wrote:
[snip]
>>>> -        offset += block_len; +        *offset += block_len;
>>>> }
>>>> 
>>>> return ret; @@ -459,22 +463,26 @@ static size_t
>>>> drop_ffs(const nand_info_t *nand, const u_char *buf, * Write
>>>> image to NAND flash. * Blocks that are marked bad are skipped
>>>> and the is written to the next * block instead as long as the
>>>> image is short enough to fit even after - * skipping the bad
>>>> blocks. + * skipping the bad blocks.  Note that the actual
>>>> size needed may exceed + * both the length and available NAND
>>>> due to bad blocks.
>>> 
>>> If that happens, then the function returns failure.  Are the 
>>> contents of "actual" well-defined when the function returns 
>>> failure?
>> 
>> They are as well defined as what happens with length.  If we say
>> we can't write, we set both to 0 and return an error.  I'll take
>> this as a request to expand the comment and do so.
> 
> The comments could use expanding (it doesn't even explain what
> happens to length in the non-error case), but also it looks like
> there are some error paths where actual doesn't get cleared, in
> the CONFIG_CMD_NAND_YAFFS section.

I've dropped actual for everything except for DFU where it's really
needed.  And I've expanded the big comment block (not the @param)
lines with what happens to length and actual.

> I was also wondering about the case where check_skip_bad() says it 
> doesn't fit.  It doesn't return the actual in that case -- it
> returns offset as of when it stopped.  So a caller of
> nand_read|write_skip_bad() would see the same "actual" as if it
> just barely fit.  I'm not sure that this function ever would return
> an actual size that exceeds the available NAND.

No?  check_skip_bad only bails out totally on end of NAND (and doesn't
care about limit).  I've re-worked things so check_skip_bad treats
actual as a size not an offset, but yes, in the case of actual exceeds
NAND, actual is only as far as we calculated.  If we're going to start
fiddling around in here with these cases for non-human users (IOW, the
"You've exceeded the device size" print wouldn't be seen) then we need
to start using more than just -EINVAL so we can differentiate "No
space" and "Too Big".

At least with v3 (which I was about to send and saw your email) should
address everything.  I jsut want to re-read your comments above with
the code and a fresh mind in the morning before re-posting.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRLpiJAAoJENk4IS6UOR1Wj4MP/08nqOakLldtjLUWBhFhTQA5
8kjaDZj0pP/RFkVUgprDuOM+JTWSY08KVVQ25pooXUN7C5OxivLgZPPcsExKeXWd
TiO8ADzVqns24Cg/RmS16ZZzpNiwjhzR/oWoaI+Zi6t1S4pSx7UBMQVQhH81sXOn
VQzWzGithY74bsFzUwQQkIIeWL+WqgCZuySop09JIB1mXcleiV4FB9/5aXmWEzOg
YhtE10cR6RCHVPBJR50qMbelvtS+h2DzFSrvz66YlicBWXdoD8gkLJ2WdM0zojuP
cetdpFSsbgadpjp8MO4SQLpim7ytdTm3IWpqGUFFTFLoaEiAxcqts82I+NcPIxF7
c9q3iS2P5h+NT9ZUqW+j+BcEz8fvJv1lWwB6t3CTsLE4EynK9iZaCNfAa8eZ3457
a3G4eVmCCFH5ZaLgmnDQR1ol6Pc2iuoZH6XX6I5JWMdSRRs5arHMaF8LCydSuPcE
gII59PK3O+RbpJYGE8nvFwByamGPtaNhTzBqBSkdGp3+lKhkEWM31dvUR0s6GGRS
5rTdEIXkHqLpu0+4YMDVFp2PAYuITnkuZCynOWyjJSojufEYTcwJiX4GRFfL+o+x
SCMsWdzqAnzrVQAP9Re+MDU+6qiPw4GRe36SOu5uq/QNwyAyFM1xC6dXogcfHYSH
rgmSTadLscWZBaR3guc4
=hPgQ
-----END PGP SIGNATURE-----