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

[Heiko Schocher <hs@denx.de>]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany