From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukasz Majewski Date: Tue, 17 Jun 2014 10:59:27 +0200 Subject: [U-Boot] [PATCH V2 2/2] dfu: fix some issues with reads/uploads In-Reply-To: <1402512447-31897-2-git-send-email-swarren@wwwdotorg.org> References: <1402512447-31897-1-git-send-email-swarren@wwwdotorg.org> <1402512447-31897-2-git-send-email-swarren@wwwdotorg.org> Message-ID: <20140617105927.0fe3ea85@amdc2363> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Stephen, > From: Stephen Warren > > DFU read support appears to rely upon dfu->read_medium() updating the > passed-by-reference len parameter to indicate the remaining size > available for reading. > > dfu_read_medium_mmc() never does this, and the implementation of > dfu_read_medium_nand() will only work if called just once; it > hard-codes the value to the total size of the NAND device > irrespective of read offset. > > I believe that overloading dfu->read_medium() is confusing. As such, > this patch introduces a new function dfu->get_medium_size() which can > be used to explicitly find out the medium size, and nothing else. > dfu_read() is modified to use this function to set the initial value > for dfu->r_left, rather than attempting to use the side-effects of > dfu->read_medium() for this purpose. > > Due to this change, dfu_read() must initially set dfu->b_left to 0, > since no data has been read. > > dfu_read_buffer_fill() must also be modified not to adjust dfu->r_left > when simply copying data from dfu->i_buf_start to the upload request > buffer. r_left represents the amount of data left to be read from HW. > That value is not affected by the memcpy(), but only by calls to > dfu->read_medium(). > > After this change, I can read from either a 4MB or 1.5MB chunk of a > 4MB eMMC boot partion with CONFIG_SYS_DFU_DATA_BUF_SIZE==1MB. Without > this change, attempting to do that would result in DFU read returning > no data at all due to r_left never being set. > > Signed-off-by: Stephen Warren Stephen thanks for the patch. Acked-by: Lukasz Majewski Test HW: Trats2 (Exynos4412): Tested-by: Lukasz Majewski I will pull this patch to -dfu tree when I receive Tom's and Marek's responde to the first patch in this series. > --- > v2: > * Fix dfu_get_medium_size_mmc() to handle DFU_FS_FAT/EXT5 layouts too, > by calling the recently introduced "size" command. > --- > drivers/dfu/dfu.c | 20 ++++++++++++----- > drivers/dfu/dfu_mmc.c | 59 > ++++++++++++++++++++++++++++++++++++++++++-------- > drivers/dfu/dfu_nand.c | 7 +++++- drivers/dfu/dfu_ram.c | 11 > +++++----- include/dfu.h | 3 +++ > 5 files changed, 79 insertions(+), 21 deletions(-) > > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c > index dc09ff6466e6..dab5e7048ed5 100644 > --- a/drivers/dfu/dfu.c > +++ b/drivers/dfu/dfu.c > @@ -267,7 +267,6 @@ static int dfu_read_buffer_fill(struct dfu_entity > *dfu, void *buf, int size) > dfu->i_buf += chunk; > dfu->b_left -= chunk; > - dfu->r_left -= chunk; > size -= chunk; > buf += chunk; > readn += chunk; > @@ -313,10 +312,19 @@ int dfu_read(struct dfu_entity *dfu, void *buf, > int size, int blk_seq_num) if (dfu->i_buf_start == NULL) > return -ENOMEM; > > - ret = dfu->read_medium(dfu, 0, dfu->i_buf_start, > &dfu->r_left); > - if (ret != 0) { > - debug("%s: failed to get r_left\n", > __func__); > - return ret; > + dfu->r_left = dfu->get_medium_size(dfu); > + if (dfu->r_left < 0) > + return dfu->r_left; > + switch (dfu->layout) { > + case DFU_RAW_ADDR: > + case DFU_RAM_ADDR: > + break; > + default: > + if (dfu->r_left >= dfu_buf_size) { > + printf("%s: File too big for > buffer\n", > + __func__); > + return -EOVERFLOW; > + } > } > > debug("%s: %s %ld [B]\n", __func__, dfu->name, > dfu->r_left); @@ -326,7 +334,7 @@ int dfu_read(struct dfu_entity > *dfu, void *buf, int size, int blk_seq_num) dfu->offset = 0; > dfu->i_buf_end = dfu_get_buf() + dfu_buf_size; > dfu->i_buf = dfu->i_buf_start; > - dfu->b_left = min(dfu_buf_size, dfu->r_left); > + dfu->b_left = 0; > > dfu->bad_skip = 0; > > diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c > index 63cc876612c9..322bd8c5d2de 100644 > --- a/drivers/dfu/dfu_mmc.c > +++ b/drivers/dfu/dfu_mmc.c > @@ -12,6 +12,8 @@ > #include > #include > #include > +#include > +#include > #include > > static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE) > @@ -113,22 +115,17 @@ static int mmc_file_buffer(struct dfu_entity > *dfu, void *buf, long *len) static int mmc_file_op(enum dfu_op op, > struct dfu_entity *dfu, void *buf, long *len) > { > + const char *fsname, *opname; > char cmd_buf[DFU_CMD_BUF_SIZE]; > char *str_env; > int ret; > > switch (dfu->layout) { > case DFU_FS_FAT: > - sprintf(cmd_buf, "fat%s mmc %d:%d 0x%x %s", > - op == DFU_OP_READ ? "load" : "write", > - dfu->data.mmc.dev, dfu->data.mmc.part, > - (unsigned int) buf, dfu->name); > + fsname = "fat"; > break; > case DFU_FS_EXT4: > - sprintf(cmd_buf, "ext4%s mmc %d:%d 0x%x /%s", > - op == DFU_OP_READ ? "load" : "write", > - dfu->data.mmc.dev, dfu->data.mmc.part, > - (unsigned int) buf, dfu->name); > + fsname = "ext4"; > break; > default: > printf("%s: Layout (%s) not (yet) supported!\n", > __func__, @@ -136,6 +133,28 @@ static int mmc_file_op(enum dfu_op op, > struct dfu_entity *dfu, return -1; > } > > + switch (op) { > + case DFU_OP_READ: > + opname = "load"; > + break; > + case DFU_OP_WRITE: > + opname = "write"; > + break; > + case DFU_OP_SIZE: > + opname = "size"; > + break; > + default: > + return -1; > + } > + > + sprintf(cmd_buf, "%s%s mmc %d:%d", fsname, opname, > + dfu->data.mmc.dev, dfu->data.mmc.part); > + > + if (op != DFU_OP_SIZE) > + sprintf(cmd_buf + strlen(cmd_buf), " 0x%x", > (unsigned int)buf); + > + sprintf(cmd_buf + strlen(cmd_buf), " %s", dfu->name); > + > if (op == DFU_OP_WRITE) > sprintf(cmd_buf + strlen(cmd_buf), " %lx", *len); > > @@ -147,7 +166,7 @@ static int mmc_file_op(enum dfu_op op, struct > dfu_entity *dfu, return ret; > } > > - if (dfu->layout != DFU_RAW_ADDR && op == DFU_OP_READ) { > + if (op != DFU_OP_WRITE) { > str_env = getenv("filesize"); > if (str_env == NULL) { > puts("dfu: Wrong file size!\n"); > @@ -196,6 +215,27 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu) > return ret; > } > > +long dfu_get_medium_size_mmc(struct dfu_entity *dfu) > +{ > + int ret; > + long len; > + > + switch (dfu->layout) { > + case DFU_RAW_ADDR: > + return dfu->data.mmc.lba_size * > dfu->data.mmc.lba_blk_size; > + case DFU_FS_FAT: > + case DFU_FS_EXT4: > + ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, &len); > + if (ret < 0) > + return ret; > + return len; > + default: > + printf("%s: Layout (%s) not (yet) supported!\n", > __func__, > + dfu_get_layout(dfu->layout)); > + return -1; > + } > +} > + > int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void > *buf, long *len) > { > @@ -316,6 +356,7 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, > char *s) } > > dfu->dev_type = DFU_DEV_MMC; > + dfu->get_medium_size = dfu_get_medium_size_mmc; > dfu->read_medium = dfu_read_medium_mmc; > dfu->write_medium = dfu_write_medium_mmc; > dfu->flush_medium = dfu_flush_medium_mmc; > diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c > index ccdbef6b75f2..e1c9a8849246 100644 > --- a/drivers/dfu/dfu_nand.c > +++ b/drivers/dfu/dfu_nand.c > @@ -114,6 +114,11 @@ static int dfu_write_medium_nand(struct > dfu_entity *dfu, return ret; > } > > +long dfu_get_medium_size_nand(struct dfu_entity *dfu) > +{ > + return dfu->data.nand.size; > +} > + > static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, > void *buf, long *len) > { > @@ -121,7 +126,6 @@ static int dfu_read_medium_nand(struct dfu_entity > *dfu, u64 offset, void *buf, > switch (dfu->layout) { > case DFU_RAW_ADDR: > - *len = dfu->data.nand.size; > ret = nand_block_read(dfu, offset, buf, len); > break; > default: > @@ -220,6 +224,7 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu, > char *s) return -1; > } > > + dfu->get_medium_size = dfu_get_medium_size_nand; > dfu->read_medium = dfu_read_medium_nand; > dfu->write_medium = dfu_write_medium_nand; > dfu->flush_medium = dfu_flush_medium_nand; > diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c > index 335a8e1f2491..b6c6e60c443c 100644 > --- a/drivers/dfu/dfu_ram.c > +++ b/drivers/dfu/dfu_ram.c > @@ -41,14 +41,14 @@ static int dfu_write_medium_ram(struct dfu_entity > *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, > offset, buf, len); } > > +long dfu_get_medium_size_ram(struct dfu_entity *dfu) > +{ > + return dfu->data.ram.size; > +} > + > static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset, > void *buf, long *len) > { > - if (!*len) { > - *len = dfu->data.ram.size; > - return 0; > - } > - > return dfu_transfer_medium_ram(DFU_OP_READ, dfu, offset, > buf, len); } > > @@ -69,6 +69,7 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu, > char *s) dfu->data.ram.size = simple_strtoul(s, &s, 16); > > dfu->write_medium = dfu_write_medium_ram; > + dfu->get_medium_size = dfu_get_medium_size_ram; > dfu->read_medium = dfu_read_medium_ram; > > dfu->inited = 0; > diff --git a/include/dfu.h b/include/dfu.h > index 26ffbc8e81d2..df720310f2cc 100644 > --- a/include/dfu.h > +++ b/include/dfu.h > @@ -35,6 +35,7 @@ enum dfu_layout { > enum dfu_op { > DFU_OP_READ = 1, > DFU_OP_WRITE, > + DFU_OP_SIZE, > }; > > struct mmc_internal_data { > @@ -96,6 +97,8 @@ struct dfu_entity { > struct ram_internal_data ram; > } data; > > + long (*get_medium_size)(struct dfu_entity *dfu); > + > int (*read_medium)(struct dfu_entity *dfu, > u64 offset, void *buf, long *len); > -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group