Hi Heiko, > Hello Lukasz, > > Am 28.01.2016 um 15:56 schrieb Lukasz Majewski: > > Hi Heiko, > > > >> change the get_medium_size() function from > >> - long (*get_medium_size)(struct dfu_entity *dfu); > >> + int (*get_medium_size)(struct dfu_entity *dfu, u64 *size); > >> > >> so it can return bigger medium sizes than 2GiB, and the return > >> value is seperate from the size. > >> > >> Signed-off-by: Heiko Schocher <h...@denx.de> > >> > >> --- > >> I just have a DDP nand with a size of 4GiB, and the > >> mtd partition layout is: > >> device nand2 <omap2-nand.0>, # parts = 9 > >> #: name size offset mask_flags > >> 0: spl 0x00080000 0x00000000 0 > >> 1: spl.backup1 0x00080000 0x00080000 0 > >> 2: spl.backup2 0x00080000 0x00100000 0 > >> 3: spl.backup3 0x00080000 0x00180000 0 > >> 4: u-boot 0x00780000 0x00200000 0 > >> 5: u-boot.env0 0x00200000 0x00980000 0 > >> 6: u-boot.env1 0x00200000 0x00b80000 0 > >> 7: mtdoops 0x00200000 0x00d80000 0 > >> 8: rootfs 0xff080000 0x00f80000 0 > >> > >> so the last partition is to big for returning the size in a long. > >> > >> drivers/dfu/dfu.c | 8 ++++---- > >> drivers/dfu/dfu_mmc.c | 8 +++++--- > >> drivers/dfu/dfu_nand.c | 5 +++-- > >> drivers/dfu/dfu_ram.c | 5 +++-- > >> drivers/dfu/dfu_sf.c | 5 +++-- > >> include/dfu.h | 4 ++-- > >> 6 files changed, 20 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c > >> index 8f5915e..daa2eb9 100644 > >> --- a/drivers/dfu/dfu.c > >> +++ b/drivers/dfu/dfu.c > >> @@ -335,11 +335,11 @@ int dfu_read(struct dfu_entity *dfu, void > >> *buf, int size, int blk_seq_num) if (dfu->i_buf_start == NULL) > >> return -ENOMEM; > >> > >> - dfu->r_left = dfu->get_medium_size(dfu); > >> - if (dfu->r_left < 0) > >> - return dfu->r_left; > >> + ret = dfu->get_medium_size(dfu, &dfu->r_left); > >> + if (ret < 0) > >> + return ret; > >> > >> - debug("%s: %s %ld [B]\n", __func__, dfu->name, > >> dfu->r_left); > >> + debug("%s: %s %lld [B]\n", __func__, dfu->name, > >> dfu->r_left); > >> dfu->i_blk_seq_num = 0; > >> dfu->crc = 0; > >> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c > >> index 395d472..5c1c1d1 100644 > >> --- a/drivers/dfu/dfu_mmc.c > >> +++ b/drivers/dfu/dfu_mmc.c > >> @@ -205,14 +205,15 @@ int dfu_flush_medium_mmc(struct dfu_entity > >> *dfu) return ret; > >> } > >> > >> -long dfu_get_medium_size_mmc(struct dfu_entity *dfu) > >> +int dfu_get_medium_size_mmc(struct dfu_entity *dfu, u64 *size) > > > > The idea to use the return value to get error code and separate > > pointer for passing the size is the way to go in my opinion. > > Thats, why I posted it as an RFC, I was unsure too... > > > The problem is in details. Please find my comments below. > > > >> { > >> int ret; > >> long len; > >> > >> switch (dfu->layout) { > >> case DFU_RAW_ADDR: > >> - return dfu->data.mmc.lba_size * > >> dfu->data.mmc.lba_blk_size; > >> + *size = dfu->data.mmc.lba_size * > >> dfu->data.mmc.lba_blk_size; > >> + return 0; > >> case DFU_FS_FAT: > >> case DFU_FS_EXT4: > >> dfu_file_buf_filled = -1; > >> @@ -221,7 +222,8 @@ long dfu_get_medium_size_mmc(struct dfu_entity > >> *dfu) return ret; > >> if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE) > >> return -1; > >> - return len; > >> + *size = len; > >> + return 0; > >> default: > >> printf("%s: Layout (%s) not (yet) supported!\n", > >> __func__, dfu_get_layout(dfu->layout)); > >> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c > >> index a975492..4612e09 100644 > >> --- a/drivers/dfu/dfu_nand.c > >> +++ b/drivers/dfu/dfu_nand.c > >> @@ -114,9 +114,10 @@ static int dfu_write_medium_nand(struct > >> dfu_entity *dfu, return ret; > >> } > >> > >> -long dfu_get_medium_size_nand(struct dfu_entity *dfu) > >> +int dfu_get_medium_size_nand(struct dfu_entity *dfu, u64 *size) > >> { > >> - return dfu->data.nand.size; > >> + *size = dfu->data.nand.size; > >> + return 0; > >> } > >> > >> static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 > >> offset, void *buf, diff --git a/drivers/dfu/dfu_ram.c > >> b/drivers/dfu/dfu_ram.c index e094a94..466759d 100644 > >> --- a/drivers/dfu/dfu_ram.c > >> +++ b/drivers/dfu/dfu_ram.c > >> @@ -41,9 +41,10 @@ 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) > >> +int dfu_get_medium_size_ram(struct dfu_entity *dfu, u64 *size) > >> { > >> - return dfu->data.ram.size; > >> + *size = dfu->data.ram.size; > >> + return 0; > >> } > >> > >> static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 > >> offset, diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c > >> index 9702eee..35c5fa1 100644 > >> --- a/drivers/dfu/dfu_sf.c > >> +++ b/drivers/dfu/dfu_sf.c > >> @@ -12,9 +12,10 @@ > >> #include <spi.h> > >> #include <spi_flash.h> > >> > >> -static long dfu_get_medium_size_sf(struct dfu_entity *dfu) > >> +int dfu_get_medium_size_sf(struct dfu_entity *dfu, u64 *size) > > > > Originally the dfu_get_medium_size returns int. It is not suitable > > for sizes > 2GiB. > > > > I don't like the u64 for *size, since we use that size to perform > > some arithmetic operation and comparison (>) in the dfu.c file. I > > would prefer to have long long here. > > Ok. > > >> { > >> - return dfu->data.sf.size; > >> + *size = dfu->data.sf.size; > >> + return 0; > >> } > >> > >> static int dfu_read_medium_sf(struct dfu_entity *dfu, u64 offset, > >> void *buf, diff --git a/include/dfu.h b/include/dfu.h > >> index 6118dc2..323b032 100644 > >> --- a/include/dfu.h > >> +++ b/include/dfu.h > >> @@ -110,7 +110,7 @@ struct dfu_entity { > >> struct sf_internal_data sf; > >> } data; > >> > >> - long (*get_medium_size)(struct dfu_entity *dfu); > >> + int (*get_medium_size)(struct dfu_entity *dfu, u64 *size); > >> > >> int (*read_medium)(struct dfu_entity *dfu, > >> u64 offset, void *buf, long *len); > >> @@ -132,7 +132,7 @@ struct dfu_entity { > >> u8 *i_buf; > >> u8 *i_buf_start; > >> u8 *i_buf_end; > >> - long r_left; > >> + u64 r_left; > > > > This patch changes r_left to be unsigned, but leaves b_left as > > signed (long). > > > > I think that both b_left and r_left should be promoted to long long. > > Ok, I change it to long long ... but, I have detected another problem: > > drivers/dfu/dfu_nand.c: > > static int nand_block_op(enum dfu_op op, struct dfu_entity *dfu, > u64 offset, void *buf, long *len) > { > loff_t start, lim; > size_t count, actual; > int ret; > nand_info_t *nand; > > /* if buf == NULL return total size of the area */ > if (buf == NULL) { > *len = dfu->data.nand.size; > return 0; > } > > returns also a long only. nand_block_op gets called in the end from > > static int dfu_write_medium_nand(struct dfu_entity *dfu, > u64 offset, void *buf, long *len) > > and > > static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset, > void *buf, long *len) > { > > So I have to change also this functions ... Why returns the read/write > operation the length of the complete area, if buf is NULL ? Is this > a DFU feature?
Git blame hints that this code is a reminiscent of first code, which added NAND support to dfu (SHA1: c6631764, year 2013). It seems like in the past the nand_block_op() has been called with buf = NULL parameter to read the NAND area size. However, Stephen Warren has added the get_medium_size_nand() function a year later (in 2014) to read the medium size. Hence it seems to me, that the buf == NULL code in nand_block_op could be safely removed. As I've check - in contemporary u-boot - there is no call to this function with buf NULL parameter. > > bye, > Heiko > > > >> long b_left; > >> > >> u32 bad_skip; /* for nand use */ > > > > > > > -- 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