[U-Boot] [RFC PATCH v1] dfu: introduce dfu_mtd support
Heiko Schocher
hs at denx.de
Tue Feb 2 11:36:54 CET 2016
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 at 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 at 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 at 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
More information about the U-Boot
mailing list