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
