Re: [U-Boot] [RFC PATCH v1] dfu: introduce dfu_mtd support

Hello Lukasz,

Am 02.02.2016 um 11:06 schrieb Lukasz Majewski:

Hi Heiko,

Please find below comments.

...

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 hs@denx.de

---

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

diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile index 61f2b71..c769d8c 100644 --- a/drivers/dfu/Makefile +++ b/drivers/dfu/Makefile @@ -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 diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 873dad5..bce619c 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -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;

diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c new file mode 100644 index 0000000..de24f94 --- /dev/null +++ b/drivers/dfu/dfu_mtd.c @@ -0,0 +1,274 @@ +/*

• • dfu_mtd.c -- DFU for MTD routines.

	MTD devices?

Fixed.

...

• • Copyright (C) 2016 DENX Software Engineering GmbH hs@denx.de
• • based on:
• • Copyright (C) 2012-2013 Texas Instruments, Inc.

Based on:

Fixed.

...

• • Based on dfu_mmc.c which is:
• • Copyright (C) 2012 Samsung Electronics
• • author: Lukasz Majewski l.majewski@samsung.com
• • 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;
• 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 ...

...

• /* 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> I think that we can remove the above code.

Yes, removed.

...

• start = dfu->data.mtd.part->offset + dfu->bad_skip + offset;
• if (start > dfu->data.mtd.part->offset +

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

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

• 			if (ret == -EINVAL)
  

• 				return ret;
  

• 			if (ret) {
  

• 				/* This is now a bad
  

block */

• 				start += bsize;
  

• 				dfu->bad_skip +=
  

bsize;

• 				continue;
  

• 			}
  

• 			return -EIO;
  

• 		} else {
  

• 			/* else we have an error */
  

• 			return ret;
  

• 		}
  

• 	}
  

• 	/* now we are sure, we can write to the
  

block */

• 	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",

•        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__,

•        dfu_get_layout(dfu->layout));
  

• }
• return ret;

+}

+static int dfu_get_medium_size_mtd(struct dfu_entity *dfu, long long *size) +{

• 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,

• 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__,

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

• 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

...

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

...

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

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

• 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

...

• 	return -EINVAL;
  

• }
  

• if (find_dev_and_part(dfu->name, &dev, &pnum,
  

&part)) {

• 	printf("Partition %s not found!\n",
  

dfu->name);

The same as above. Please check globally.

...

• 	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",

• 	       dfu->name, mtd_dev);
  

• 	return -ENODEV;
  

• }
  

• } else {

• printf("%s: Memory layout (%s) not supported!\n",
  

__func__, st);

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

+} diff --git a/include/dfu.h b/include/dfu.h index c327bb5..a0b111c 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -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;

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

...

• /* 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

Thanks for the review.

bye, Heiko