Skip to contentSkip to conversation

dfu: introduce dfu_mtd support

RFC PATCH New Heiko Schocher 2 participants 3 messages Feb 1, 2016 – Feb 2, 2016

lore
Share & tools
lore

Heiko Schocher

author
#0 lore raw

RFC PATCH 1/1 dfu: introduce dfu_mtd support

With the new dfu_mtd layer, now dfu supports reading/writing to mtd partitions, found on mtd devices. With this approach it is also possible to read/write to concatenated mtd devices.

Signed-off-by Heiko Schocher <[email protected]>

---
This patch is based on patch:
dfu: allow get_medium_size() to return bigger medium sizes than 2GiB

Tested this driver on etamin module on the dxr2 board with a DDP nand with a size of 4GiB (2 CS -> 2 nand devices, concatenated with mtdconcat to a new mtd device)

 drivers/dfu/Makefile  |   1 +
 drivers/dfu/dfu.c     |   3 +
 drivers/dfu/dfu_mtd.c | 274 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/dfu.h         |  23 +++++
 4 files changed, 301 insertions(+)
 create mode 100644 drivers/dfu/dfu_mtd.c
drivers/dfu/Makefile +1 -0
index 61f2b71..c769d8c 100644
@@ -7,6 +7,7 @@
 
 obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
 obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
+obj-$(CONFIG_DFU_MTD) += dfu_mtd.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
 obj-$(CONFIG_DFU_RAM) += dfu_ram.o
 obj-$(CONFIG_DFU_SF) += dfu_sf.o
drivers/dfu/dfu.c +3 -0
index 873dad5..bce619c 100644
@@ -406,6 +406,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, devstr, s))
 			return -1;
+	} else if (strcmp(interface, "mtd") == 0) {
+		if (dfu_fill_entity_mtd(dfu, devstr, s))
+			return -1;
 	} else if (strcmp(interface, "nand") == 0) {
 		if (dfu_fill_entity_nand(dfu, devstr, s))
 			return -1;
drivers/dfu/dfu_mtd.c new file +274 -0

274 lines not on this page · raw message

include/dfu.h +23 -0
index c327bb5..a0b111c 100644
@@ -23,6 +23,7 @@ enum dfu_device_type {
 	DFU_DEV_NAND,
 	DFU_DEV_RAM,
 	DFU_DEV_SF,
+	DFU_DEV_MTD,
 };
 
 enum dfu_layout {

@@ -67,6 +68,16 @@ struct nand_internal_data {
 	unsigned int ubi;
 };
 
+struct mtd_internal_data {
+	struct mtd_info *mtd;
+	struct part_info *part;
+
+	size_t	len;
+	/* for MTD/ubi use */
+	unsigned int ubi;
+	loff_t start_erase;
+};
+
 struct ram_internal_data {
 	void		*start;
 	unsigned int	size;

@@ -108,6 +119,7 @@ struct dfu_entity {
 		struct nand_internal_data nand;
 		struct ram_internal_data ram;
 		struct sf_internal_data sf;
+		struct mtd_internal_data mtd;
 	} data;
 
 	int (*get_medium_size)(struct dfu_entity *dfu, long long *size);

@@ -189,6 +201,17 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr,
 }
 #endif
 
+#ifdef CONFIG_DFU_MTD
+extern int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char *s);
+#else
+static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,
+				       char *s)
+{
+	puts("MTD support not available!\n");
+	return -1;
+}
+#endif
+
 #ifdef CONFIG_DFU_NAND
 extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char *devstr, char *s);
 #else
Signature · 1 line
2.5.0
#1 lore raw

RFC PATCH 1/1 dfu: introduce dfu_mtd support

Hi Heiko,

Please find below comments.

Quoted 54 lines

With the new dfu_mtd layer, now dfu supports reading/writing to mtd partitions, found on mtd devices. With this approach it is also possible to read/write to concatenated mtd devices.

Signed-off-by Heiko Schocher <[email protected]>

---
This patch is based on patch:
dfu: allow get_medium_size() to return bigger medium sizes than 2GiB

Tested this driver on etamin module on the dxr2 board with a DDP nand with a size of 4GiB (2 CS -> 2 nand devices, concatenated with mtdconcat to a new mtd device)

 drivers/dfu/Makefile  |   1 +
 drivers/dfu/dfu.c     |   3 +
 drivers/dfu/dfu_mtd.c | 274
++++++++++++++++++++++++++++++++++++++++++++++++++
include/dfu.h         |  23 +++++ 4 files changed, 301 insertions(+)
 create mode 100644 drivers/dfu/dfu_mtd.c
drivers/dfu/Makefile +1 -0
index 61f2b71..c769d8c 100644
@@ -7,6 +7,7 @@
 
 obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
 obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
+obj-$(CONFIG_DFU_MTD) += dfu_mtd.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
 obj-$(CONFIG_DFU_RAM) += dfu_ram.o
 obj-$(CONFIG_DFU_SF) += dfu_sf.o
drivers/dfu/dfu.c +3 -0
index 873dad5..bce619c 100644
@@ -406,6 +406,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, devstr, s))
 			return -1;
+	} else if (strcmp(interface, "mtd") == 0) {
+		if (dfu_fill_entity_mtd(dfu, devstr, s))
+			return -1;
 	} else if (strcmp(interface, "nand") == 0) {
 		if (dfu_fill_entity_nand(dfu, devstr, s))
 			return -1;
drivers/dfu/dfu_mtd.c new file +0 -0
new file mode 100644
index 0000000..de24f94

drivers/dfu/dfu_mtd.c line 2

Quoted 2 lines
drivers/dfu/dfu_mtd.c new file +2 -0
@@ -0,0 +1,274 @@
+/*
+ * dfu_mtd.c -- DFU for MTD routines.

MTD devices?

drivers/dfu/dfu_mtd.c line 7

Quoted 5 lines
drivers/dfu/dfu_mtd.c new file +5 -0
+ *
+ * Copyright (C) 2016 DENX Software Engineering GmbH <[email protected]>
+ *
+ * based on:
+ * Copyright (C) 2012-2013 Texas Instruments, Inc.

Based on:

Quoted 21 lines
drivers/dfu/dfu_mtd.c new file +21 -0
+ *
+ * Based on dfu_mmc.c which is:
+ * Copyright (C) 2012 Samsung Electronics
+ * author: Lukasz Majewski <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <errno.h>
+#include <div64.h>
+#include <dfu.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <jffs2/load_kernel.h>
+
+static int mtd_block_op(enum dfu_op op, struct dfu_entity *dfu,
+			u64 offset, void *buf, long long *len)
+{
+	loff_t start;

drivers/dfu/dfu_mtd.c line 38

Quoted 10 lines
drivers/dfu/dfu_mtd.c new file +10 -0
+	size_t retlen;
+	int length = *len;
+	int ret = 0;
+	struct mtd_info *mtd = dfu->data.mtd.mtd;
+	int bsize = mtd->erasesize;
+	int loops = length / bsize;
+	int rest = length % bsize;
+	char *curbuf;
+	int i = 0;
+	struct erase_info ei;

Try to clean it up to avoid camel case definitions. (If in doubt please refer to automatic definitions at dfu.c). Please check this globally.

drivers/dfu/dfu_mtd.c line 44

Quoted 6 lines
drivers/dfu/dfu_mtd.c new file +6 -0
+
+	/* if buf == NULL return total size of the area */
+	if (buf == NULL) {
+		*len = dfu->data.mtd.part->size;
+		return 0;
+	}

Do we need this if (buf == NULL) {
}
construct to get the size of the area?

A few lines down you have defined the dfu_get_medium_size_mtd() function.
I think that we can remove the above code.

Quoted 154 lines
drivers/dfu/dfu_mtd.c new file +3 -0
+
+	start = dfu->data.mtd.part->offset + dfu->bad_skip + offset;
+	if (start > dfu->data.mtd.part->offset +

dfu->data.mtd.part->size)

drivers/dfu/dfu_mtd.c new file +33 -0
+		return -EIO;
+
+	if (offset % bsize)
+		return -EFAULT;
+
+	if (rest)
+		loops++;
+
+	curbuf = buf;
+	while (i < loops) {
+		ret = mtd_block_isbad(mtd, start);
+		if (ret == -EINVAL)
+			return ret;
+
+		if (ret) {
+			/* we have a bad block */
+			start += bsize;
+			dfu->bad_skip += bsize;
+			continue;
+		}
+
+		if (op == DFU_OP_READ) {
+			ret = mtd_read(mtd, start, bsize, &retlen,
+			       (u_char *)curbuf);
+		} else {
+			memset(&ei, 0, sizeof(struct erase_info));
+			ei.mtd = mtd;
+			ei.addr = start;
+			ei.len = bsize;
+			ret = mtd_erase(mtd, &ei);
+			if (ret != 0) {
+				if (ret == -EIO) {
+					ret = mtd_block_isbad(mtd,

start);

drivers/dfu/dfu_mtd.c new file +5 -0
+					if (ret == -EINVAL)
+						return ret;
+
+					if (ret) {
+						/* This is now a bad

block */

drivers/dfu/dfu_mtd.c new file +2 -0
+						start += bsize;
+						dfu->bad_skip +=

bsize;

drivers/dfu/dfu_mtd.c new file +10 -0
+						continue;
+					}
+					return -EIO;
+				} else {
+					/* else we have an error */
+					return ret;
+				}
+			}
+
+			/* now we are sure, we can write to the

block */

drivers/dfu/dfu_mtd.c new file +15 -0
+			ret = mtd_write(mtd, start, bsize, &retlen,
+					(const u_char *)curbuf);
+			if (ret < 0)
+				return ret;
+
+			if (retlen != bsize)
+				return -EIO;
+		}
+		curbuf += bsize;
+		start += bsize;
+		i++;
+	}
+	if (ret != 0) {
+		printf("%s: mtd %s call failed at %llx!\n",
+		       __func__, op == DFU_OP_READ ? "read" :

"write",

drivers/dfu/dfu_mtd.c new file +31 -0
+		       start);
+		return ret;
+	}
+
+	dfu->data.mtd.start_erase = start;
+	return ret;
+}
+
+static inline int mtd_block_write(struct dfu_entity *dfu,
+		u64 offset, void *buf, long long *len)
+{
+	return mtd_block_op(DFU_OP_WRITE, dfu, offset, buf, len);
+}
+
+static inline int mtd_block_read(struct dfu_entity *dfu,
+		u64 offset, void *buf, long long *len)
+{
+	return mtd_block_op(DFU_OP_READ, dfu, offset, buf, len);
+}
+
+static int dfu_write_medium_mtd(struct dfu_entity *dfu,
+		u64 offset, void *buf, long long *len)
+{
+	int ret = -1;
+
+	switch (dfu->layout) {
+	case DFU_RAW_ADDR:
+		ret = mtd_block_write(dfu, offset, buf, len);
+		break;
+	default:
+		printf("%s: Layout (%s) not (yet) supported!\n",

__func__,

drivers/dfu/dfu_mtd.c new file +7 -0
+		       dfu_get_layout(dfu->layout));
+	}
+
+	return ret;
+}
+
+static int dfu_get_medium_size_mtd(struct dfu_entity *dfu, long long

*size) +{

drivers/dfu/dfu_mtd.c new file +8 -0
+	if (!size)
+		return -EFAULT;
+
+	*size = dfu->data.mtd.part->size;
+	return 0;
+}
+
+static int dfu_read_medium_mtd(struct dfu_entity *dfu, u64 offset,

void *buf,

drivers/dfu/dfu_mtd.c new file +10 -0
+		long long *len)
+{
+	int ret = -1;
+
+	switch (dfu->layout) {
+	case DFU_RAW_ADDR:
+		ret = mtd_block_read(dfu, offset, buf, len);
+		break;
+	default:
+		printf("%s: Layout (%s) not (yet) supported!\n",

__func__,

drivers/dfu/dfu_mtd.c new file +20 -0
+		       dfu_get_layout(dfu->layout));
+	}
+
+	return ret;
+}
+
+static int dfu_flush_medium_mtd(struct dfu_entity *dfu)
+{
+	int ret = 0;
+
+	/* in case of ubi partition, erase rest of the partition */
+	if (dfu->data.mtd.ubi) {
+		int ret;
+		struct erase_info ei;
+		int bsize = dfu->data.mtd.mtd->erasesize;
+		int loops;
+		int i = 0;
+		int len;
+
+		memset(&ei, 0, sizeof(struct erase_info));

drivers/dfu/dfu_mtd.c line 199

Quoted 10 lines
drivers/dfu/dfu_mtd.c new file +10 -0
+		ei.mtd = dfu->data.mtd.mtd;
+		ei.addr = dfu->data.mtd.start_erase;
+		ei.len = bsize;
+		len = dfu->data.mtd.part->size -
+		      (ei.addr - dfu->data.mtd.part->offset);
+		loops = len / bsize;
+
+		while (i < loops) {
+			ret = mtd_erase(dfu->data.mtd.mtd, &ei);
+			if (ret != 0)

if (ret) would be enough

drivers/dfu/dfu_mtd.c line 200

Quoted 1 line
drivers/dfu/dfu_mtd.c new file +1 -0
+				printf("Failure erase: %d\n", ret);

				printf("%s: Failure erase: %d\n",
				__func__,
				ret) or error().
				Please check
				this globally.
Quoted 32 lines
drivers/dfu/dfu_mtd.c new file +21 -0
+			i++;
+			ei.addr += bsize;
+		}
+	}
+
+	return ret;
+}
+
+static unsigned int dfu_polltimeout_mtd(struct dfu_entity *dfu)
+{
+	/*
+	 * Currently, Poll Timeout != 0 is only needed on MTD
+	 * ubi partition, as the not used sectors need an erase
+	 */
+	if (dfu->data.mtd.ubi)
+		return DFU_MANIFEST_POLL_TIMEOUT;
+
+	return DFU_DEFAULT_POLL_TIMEOUT;
+}
+
+int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char

*s) +{

drivers/dfu/dfu_mtd.c new file +10 -0
+	char *st;
+	struct mtd_device *dev;
+	struct part_info *part;
+
+	dfu->data.mtd.ubi = 0;
+	dfu->dev_type = DFU_DEV_MTD;
+	st = strsep(&s, " ");
+	if ((!strcmp(st, "mtddev")) || (!strcmp(st, "mtddevubi"))) {
+		char mtd_dev[16];
+		u8 pnum;

drivers/dfu/dfu_mtd.c line 242

Quoted 10 lines
drivers/dfu/dfu_mtd.c new file +10 -0
+
+		if (!strcmp(st, "mtddevubi"))
+			dfu->data.mtd.ubi = 1;
+		dfu->layout = DFU_RAW_ADDR;
+		/*
+		 * Search the mtd device number where this partition
+		 * is located
+		 */
+		if (mtdparts_init() != 0) {
+			printf("Error initializing mtdparts!\n");

Please make this printf more verbose (as pointed above) or simply use error()

For reference, please look into dfu_fill_entity_mmc() at dfu_mmc.c

Quoted 7 lines
drivers/dfu/dfu_mtd.c new file +4 -0
+			return -EINVAL;
+		}
+
+		if (find_dev_and_part(dfu->name, &dev, &pnum,

&part)) {

drivers/dfu/dfu_mtd.c new file +1 -0
+			printf("Partition %s not found!\n",

dfu->name);

The same as above. Please check globally.

Quoted 42 lines
drivers/dfu/dfu_mtd.c new file +8 -0
+			return -ENODEV;
+		}
+		sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(dev->id->type),
+			dev->id->num);
+		dfu->data.mtd.mtd = get_mtd_device_nm(mtd_dev);
+		dfu->data.mtd.part = part;
+		if (IS_ERR(dfu->data.mtd.mtd)) {
+			printf("Partition %s not found on device

%s!\n",

drivers/dfu/dfu_mtd.c new file +5 -0
+			       dfu->name, mtd_dev);
+			return -ENODEV;
+		}
+	} else {
+		printf("%s: Memory layout (%s) not supported!\n",

__func__, st);

drivers/dfu/dfu_mtd.c new file +14 -0
+		return -ENXIO;
+	}
+
+	dfu->get_medium_size = dfu_get_medium_size_mtd;
+	dfu->read_medium = dfu_read_medium_mtd;
+	dfu->write_medium = dfu_write_medium_mtd;
+	dfu->flush_medium = dfu_flush_medium_mtd;
+	dfu->poll_timeout = dfu_polltimeout_mtd;
+
+	/* initial state */
+	dfu->inited = 0;
+
+	return 0;
+}
include/dfu.h +1 -0
index c327bb5..a0b111c 100644
@@ -23,6 +23,7 @@ enum dfu_device_type {
 	DFU_DEV_NAND,
 	DFU_DEV_RAM,
 	DFU_DEV_SF,
+	DFU_DEV_MTD,
 };
 
 enum dfu_layout {

include/dfu.h struct nand_internal_data { · line 75

Quoted 8 lines
include/dfu.h +5 -0
@@ -67,6 +68,16 @@ struct nand_internal_data {
 	unsigned int ubi;
 };
 
+struct mtd_internal_data {
+	struct mtd_info *mtd;
+	struct part_info *part;
+
+	size_t	len;

It seems that size_t is defined at posix_types.h file as unsigned int. This means that the MTD partition (entity) cannot be larger than 4 GiB. Is this assumption correct? Shouldn't we be prepared for larger ones?

Quoted 35 lines
include/dfu.h +6 -0
+	/* for MTD/ubi use */
+	unsigned int ubi;
+	loff_t start_erase;
+};
+
 struct ram_internal_data {
 	void		*start;
 	unsigned int	size;

@@ -108,6 +119,7 @@ struct dfu_entity {
 		struct nand_internal_data nand;
 		struct ram_internal_data ram;
 		struct sf_internal_data sf;
+		struct mtd_internal_data mtd;
 	} data;
 
 	int (*get_medium_size)(struct dfu_entity *dfu, long long

*size); @@ -189,6 +201,17 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, }
#endif

include/dfu.h +2 -0
+#ifdef CONFIG_DFU_MTD
+extern int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,

char *s); +#else
+static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,

include/dfu.h +7 -0
+				       char *s)
+{
+	puts("MTD support not available!\n");
+	return -1;
+}
+#endif
+
 #ifdef CONFIG_DFU_NAND
 extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char

*devstr, char *s); #else

Signature · 5 lines
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
author
#2 lore raw

RFC PATCH 1/1 dfu: introduce dfu_mtd support

Hello Lukasz,

Am 02.02.2016 um 11:06 schrieb Lukasz Majewski:

Quoted 54 lines

Hi Heiko,

Please find below comments.

Quoted 50 lines

With the new dfu_mtd layer, now dfu supports reading/writing to mtd partitions, found on mtd devices. With this approach it is also possible to read/write to concatenated mtd devices.

Signed-off-by Heiko Schocher <[email protected]>

---
This patch is based on patch:
dfu: allow get_medium_size() to return bigger medium sizes than 2GiB

Tested this driver on etamin module on the dxr2 board with a DDP nand with a size of 4GiB (2 CS -> 2 nand devices, concatenated with mtdconcat to a new mtd device)

  drivers/dfu/Makefile  |   1 +
  drivers/dfu/dfu.c     |   3 +
  drivers/dfu/dfu_mtd.c | 274

++++++++++++++++++++++++++++++++++++++++++++++++++
include/dfu.h | 23 +++++ 4 files changed, 301 insertions(+)
create mode 100644 drivers/dfu/dfu_mtd.c

drivers/dfu/Makefile +1 -0
index 61f2b71..c769d8c 100644
@@ -7,6 +7,7 @@
 
  obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
  obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
+obj-$(CONFIG_DFU_MTD) += dfu_mtd.o
  obj-$(CONFIG_DFU_NAND) += dfu_nand.o
  obj-$(CONFIG_DFU_RAM) += dfu_ram.o
  obj-$(CONFIG_DFU_SF) += dfu_sf.o
drivers/dfu/dfu.c +3 -0
index 873dad5..bce619c 100644
@@ -406,6 +406,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, devstr, s))
  			return -1;
+	} else if (strcmp(interface, "mtd") == 0) {
+		if (dfu_fill_entity_mtd(dfu, devstr, s))
+			return -1;
  	} else if (strcmp(interface, "nand") == 0) {
  		if (dfu_fill_entity_nand(dfu, devstr, s))
  			return -1;
drivers/dfu/dfu_mtd.c new file +0 -0
new file mode 100644
Quoted 8 lines

drivers/dfu/dfu_mtd.c line 2

Quoted 6 lines

index 0000000..de24f94

drivers/dfu/dfu_mtd.c new file +2 -0
@@ -0,0 +1,274 @@
+/*
+ * dfu_mtd.c -- DFU for MTD routines.

MTD devices?

Fixed.

Quoted 7 lines

drivers/dfu/dfu_mtd.c line 7

Quoted 5 lines
drivers/dfu/dfu_mtd.c new file +5 -0
+ *
+ * Copyright (C) 2016 DENX Software Engineering GmbH <[email protected]>
+ *
+ * based on:
+ * Copyright (C) 2012-2013 Texas Instruments, Inc.

Based on:

Fixed.

Quoted 26 lines
Quoted 26 lines
drivers/dfu/dfu_mtd.c new file +26 -0
+ *
+ * Based on dfu_mmc.c which is:
+ * Copyright (C) 2012 Samsung Electronics
+ * author: Lukasz Majewski <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <errno.h>
+#include <div64.h>
+#include <dfu.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <jffs2/load_kernel.h>
+
+static int mtd_block_op(enum dfu_op op, struct dfu_entity *dfu,
+			u64 offset, void *buf, long long *len)
+{
+	loff_t start;
+	size_t retlen;
+	int length = *len;
+	int ret = 0;
+	struct mtd_info *mtd = dfu->data.mtd.mtd;
+	int bsize = mtd->erasesize;
Quoted 8 lines

drivers/dfu/dfu_mtd.c line 38

Quoted 5 lines
drivers/dfu/dfu_mtd.c new file +5 -0
+	int loops = length / bsize;
+	int rest = length % bsize;
+	char *curbuf;
+	int i = 0;
+	struct erase_info ei;

Try to clean it up to avoid camel case definitions. (If in doubt please refer to automatic definitions at dfu.c). Please check this globally.

Do I used CamelCase here?
Maybe I do not understand you here ...

Quoted 11 lines

drivers/dfu/dfu_mtd.c line 44

Quoted 6 lines
drivers/dfu/dfu_mtd.c new file +6 -0
+
+	/* if buf == NULL return total size of the area */
+	if (buf == NULL) {
+		*len = dfu->data.mtd.part->size;
+		return 0;
+	}

Do we need this if (buf == NULL) {
}
construct to get the size of the area?

Quoted 2 lines

A few lines down you have defined the dfu_get_medium_size_mtd() function.

i> I think that we can remove the above code.

Yes, removed.

Quoted 167 lines
Quoted 154 lines
drivers/dfu/dfu_mtd.c new file +3 -0
+
+	start = dfu->data.mtd.part->offset + dfu->bad_skip + offset;
+	if (start > dfu->data.mtd.part->offset +

dfu->data.mtd.part->size)

drivers/dfu/dfu_mtd.c new file +33 -0
+		return -EIO;
+
+	if (offset % bsize)
+		return -EFAULT;
+
+	if (rest)
+		loops++;
+
+	curbuf = buf;
+	while (i < loops) {
+		ret = mtd_block_isbad(mtd, start);
+		if (ret == -EINVAL)
+			return ret;
+
+		if (ret) {
+			/* we have a bad block */
+			start += bsize;
+			dfu->bad_skip += bsize;
+			continue;
+		}
+
+		if (op == DFU_OP_READ) {
+			ret = mtd_read(mtd, start, bsize, &retlen,
+			       (u_char *)curbuf);
+		} else {
+			memset(&ei, 0, sizeof(struct erase_info));
+			ei.mtd = mtd;
+			ei.addr = start;
+			ei.len = bsize;
+			ret = mtd_erase(mtd, &ei);
+			if (ret != 0) {
+				if (ret == -EIO) {
+					ret = mtd_block_isbad(mtd,

start);

drivers/dfu/dfu_mtd.c new file +5 -0
+					if (ret == -EINVAL)
+						return ret;
+
+					if (ret) {
+						/* This is now a bad

block */

drivers/dfu/dfu_mtd.c new file +2 -0
+						start += bsize;
+						dfu->bad_skip +=

bsize;

drivers/dfu/dfu_mtd.c new file +10 -0
+						continue;
+					}
+					return -EIO;
+				} else {
+					/* else we have an error */
+					return ret;
+				}
+			}
+
+			/* now we are sure, we can write to the

block */

drivers/dfu/dfu_mtd.c new file +15 -0
+			ret = mtd_write(mtd, start, bsize, &retlen,
+					(const u_char *)curbuf);
+			if (ret < 0)
+				return ret;
+
+			if (retlen != bsize)
+				return -EIO;
+		}
+		curbuf += bsize;
+		start += bsize;
+		i++;
+	}
+	if (ret != 0) {
+		printf("%s: mtd %s call failed at %llx!\n",
+		       __func__, op == DFU_OP_READ ? "read" :

"write",

drivers/dfu/dfu_mtd.c new file +31 -0
+		       start);
+		return ret;
+	}
+
+	dfu->data.mtd.start_erase = start;
+	return ret;
+}
+
+static inline int mtd_block_write(struct dfu_entity *dfu,
+		u64 offset, void *buf, long long *len)
+{
+	return mtd_block_op(DFU_OP_WRITE, dfu, offset, buf, len);
+}
+
+static inline int mtd_block_read(struct dfu_entity *dfu,
+		u64 offset, void *buf, long long *len)
+{
+	return mtd_block_op(DFU_OP_READ, dfu, offset, buf, len);
+}
+
+static int dfu_write_medium_mtd(struct dfu_entity *dfu,
+		u64 offset, void *buf, long long *len)
+{
+	int ret = -1;
+
+	switch (dfu->layout) {
+	case DFU_RAW_ADDR:
+		ret = mtd_block_write(dfu, offset, buf, len);
+		break;
+	default:
+		printf("%s: Layout (%s) not (yet) supported!\n",

__func__,

drivers/dfu/dfu_mtd.c new file +7 -0
+		       dfu_get_layout(dfu->layout));
+	}
+
+	return ret;
+}
+
+static int dfu_get_medium_size_mtd(struct dfu_entity *dfu, long long

*size) +{

drivers/dfu/dfu_mtd.c new file +8 -0
+	if (!size)
+		return -EFAULT;
+
+	*size = dfu->data.mtd.part->size;
+	return 0;
+}
+
+static int dfu_read_medium_mtd(struct dfu_entity *dfu, u64 offset,

void *buf,

drivers/dfu/dfu_mtd.c new file +10 -0
+		long long *len)
+{
+	int ret = -1;
+
+	switch (dfu->layout) {
+	case DFU_RAW_ADDR:
+		ret = mtd_block_read(dfu, offset, buf, len);
+		break;
+	default:
+		printf("%s: Layout (%s) not (yet) supported!\n",

__func__,

drivers/dfu/dfu_mtd.c new file +20 -0
+		       dfu_get_layout(dfu->layout));
+	}
+
+	return ret;
+}
+
+static int dfu_flush_medium_mtd(struct dfu_entity *dfu)
+{
+	int ret = 0;
+
+	/* in case of ubi partition, erase rest of the partition */
+	if (dfu->data.mtd.ubi) {
+		int ret;
+		struct erase_info ei;
+		int bsize = dfu->data.mtd.mtd->erasesize;
+		int loops;
+		int i = 0;
+		int len;
+
+		memset(&ei, 0, sizeof(struct erase_info));

drivers/dfu/dfu_mtd.c line 199

Quoted 10 lines
drivers/dfu/dfu_mtd.c new file +10 -0
+		ei.mtd = dfu->data.mtd.mtd;
+		ei.addr = dfu->data.mtd.start_erase;
+		ei.len = bsize;
+		len = dfu->data.mtd.part->size -
+		      (ei.addr - dfu->data.mtd.part->offset);
+		loops = len / bsize;
+
+		while (i < loops) {
+			ret = mtd_erase(dfu->data.mtd.mtd, &ei);
+			if (ret != 0)

if (ret) would be enough

Quoted 7 lines

drivers/dfu/dfu_mtd.c line 200

Quoted 1 line
drivers/dfu/dfu_mtd.c new file +1 -0
+				printf("Failure erase: %d\n", ret);

				printf("%s: Failure erase: %d\n",
				__func__,
				ret) or error().
				Please check
				this globally.

Changed to error() (And all similiar)

Quoted 104 lines
Quoted 32 lines
drivers/dfu/dfu_mtd.c new file +21 -0
+			i++;
+			ei.addr += bsize;
+		}
+	}
+
+	return ret;
+}
+
+static unsigned int dfu_polltimeout_mtd(struct dfu_entity *dfu)
+{
+	/*
+	 * Currently, Poll Timeout != 0 is only needed on MTD
+	 * ubi partition, as the not used sectors need an erase
+	 */
+	if (dfu->data.mtd.ubi)
+		return DFU_MANIFEST_POLL_TIMEOUT;
+
+	return DFU_DEFAULT_POLL_TIMEOUT;
+}
+
+int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr, char

*s) +{

drivers/dfu/dfu_mtd.c new file +10 -0
+	char *st;
+	struct mtd_device *dev;
+	struct part_info *part;
+
+	dfu->data.mtd.ubi = 0;
+	dfu->dev_type = DFU_DEV_MTD;
+	st = strsep(&s, " ");
+	if ((!strcmp(st, "mtddev")) || (!strcmp(st, "mtddevubi"))) {
+		char mtd_dev[16];
+		u8 pnum;

drivers/dfu/dfu_mtd.c line 242

Quoted 10 lines
drivers/dfu/dfu_mtd.c new file +10 -0
+
+		if (!strcmp(st, "mtddevubi"))
+			dfu->data.mtd.ubi = 1;
+		dfu->layout = DFU_RAW_ADDR;
+		/*
+		 * Search the mtd device number where this partition
+		 * is located
+		 */
+		if (mtdparts_init() != 0) {
+			printf("Error initializing mtdparts!\n");

Please make this printf more verbose (as pointed above) or simply use error()

For reference, please look into dfu_fill_entity_mmc() at dfu_mmc.c

Quoted 7 lines
drivers/dfu/dfu_mtd.c new file +4 -0
+			return -EINVAL;
+		}
+
+		if (find_dev_and_part(dfu->name, &dev, &pnum,

&part)) {

drivers/dfu/dfu_mtd.c new file +1 -0
+			printf("Partition %s not found!\n",

dfu->name);

The same as above. Please check globally.

Quoted 45 lines
drivers/dfu/dfu_mtd.c new file +8 -0
+			return -ENODEV;
+		}
+		sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(dev->id->type),
+			dev->id->num);
+		dfu->data.mtd.mtd = get_mtd_device_nm(mtd_dev);
+		dfu->data.mtd.part = part;
+		if (IS_ERR(dfu->data.mtd.mtd)) {
+			printf("Partition %s not found on device

%s!\n",

drivers/dfu/dfu_mtd.c new file +5 -0
+			       dfu->name, mtd_dev);
+			return -ENODEV;
+		}
+	} else {
+		printf("%s: Memory layout (%s) not supported!\n",

__func__, st);

drivers/dfu/dfu_mtd.c new file +14 -0
+		return -ENXIO;
+	}
+
+	dfu->get_medium_size = dfu_get_medium_size_mtd;
+	dfu->read_medium = dfu_read_medium_mtd;
+	dfu->write_medium = dfu_write_medium_mtd;
+	dfu->flush_medium = dfu_flush_medium_mtd;
+	dfu->poll_timeout = dfu_polltimeout_mtd;
+
+	/* initial state */
+	dfu->inited = 0;
+
+	return 0;
+}
include/dfu.h +1 -0
index c327bb5..a0b111c 100644
@@ -23,6 +23,7 @@ enum dfu_device_type {
  	DFU_DEV_NAND,
  	DFU_DEV_RAM,
  	DFU_DEV_SF,
+	DFU_DEV_MTD,
  };
 
  enum dfu_layout {

@@ -67,6 +68,16 @@ struct nand_internal_data {
  	unsigned int ubi;
  };
Quoted 10 lines

include/dfu.h struct nand_internal_data { · line 75

Quoted 5 lines
include/dfu.h +5 -0
+struct mtd_internal_data {
+	struct mtd_info *mtd;
+	struct part_info *part;
+
+	size_t	len;

It seems that size_t is defined at posix_types.h file as unsigned int. This means that the MTD partition (entity) cannot be larger than 4 GiB. Is this assumption correct? Shouldn't we be prepared for larger ones?

Yes, correct. Good catch! This var is not needed longer, as I use from "struct part_info" the "u64 size", so deleted this "size_t len;"

Quoted 20 lines
Quoted 20 lines
include/dfu.h +6 -0
+	/* for MTD/ubi use */
+	unsigned int ubi;
+	loff_t start_erase;
+};
+
  struct ram_internal_data {
  	void		*start;
  	unsigned int	size;

@@ -108,6 +119,7 @@ struct dfu_entity {
  		struct nand_internal_data nand;
  		struct ram_internal_data ram;
  		struct sf_internal_data sf;
+		struct mtd_internal_data mtd;
  	} data;
 
  	int (*get_medium_size)(struct dfu_entity *dfu, long long

*size); @@ -189,6 +201,17 @@ static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, }
#endif

Quoted 15 lines
Quoted 14 lines
include/dfu.h +2 -0
+#ifdef CONFIG_DFU_MTD
+extern int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,

char *s); +#else
+static inline int dfu_fill_entity_mtd(struct dfu_entity *dfu, char *devstr,

include/dfu.h +7 -0
+				       char *s)
+{
+	puts("MTD support not available!\n");
+	return -1;
+}
+#endif
+
  #ifdef CONFIG_DFU_NAND
  extern int dfu_fill_entity_nand(struct dfu_entity *dfu, char
Quoted 1 line

*devstr, char *s); #else

Thanks for the review.

bye,
Heiko

Signature · 2 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany