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 <h...@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?
> + *
> + * Copyright (C) 2016 DENX Software Engineering GmbH <h...@denx.de>
> + *
> + * based on:
> + * Copyright (C) 2012-2013 Texas Instruments, Inc.
Based on:
> + *
> + * Based on dfu_mmc.c which is:
> + * Copyright (C) 2012 Samsung Electronics
> + * author: Lukasz Majewski <l.majew...@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.
> +
> + /* 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.
> +
> + 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.
> + 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?
> + /* 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
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
