From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 2 Feb 2016 11:36:54 +0100 Subject: [U-Boot] [RFC PATCH v1] dfu: introduce dfu_mtd support In-Reply-To: <20160202110629.14237ee2@amdc2363> References: <1454318108-5033-1-git-send-email-hs@denx.de> <20160202110629.14237ee2@amdc2363> Message-ID: <56B086C6.8040909@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 >> >> --- >> 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 >> + * >> + * 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 >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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