[U-Boot] dfu: make data buffer size configurable

[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher]

Dfu transfer uses a buffer before writing data to the
raw storage device. Make the size (in bytes) of this buffer
configurable.

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
Cc: Tom Rini <trini@ti.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Vasut <marex@denx.de>
---
 README            | 5 +++++
 drivers/dfu/dfu.c | 2 +-
 include/dfu.h     | 4 +++-
 3 Dateien ge?ndert, 9 Zeilen hinzugef?gt(+), 2 Zeilen entfernt(-)

diff --git a/README b/README
index b1b3e17..8550f34 100644
--- a/README
+++ b/README
@@ -1360,6 +1360,11 @@ The following options need to be configured:
 		CONFIG_DFU_NAND
 		This enables support for exposing NAND devices via DFU.
 
+		CONFIG_SYS_DFU_DATA_BUF_SIZE
+		Dfu transfer uses a buffer before writing data to the
+		raw storage device. Make the size (in bytes) of this buffer
+		configurable.
+
 		CONFIG_SYS_DFU_MAX_FILE_SIZE
 		When updating files rather than the raw storage device,
 		we use a static buffer to copy the file into and then write
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 6af6890..fe3a36e 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -42,7 +42,7 @@ static int dfu_find_alt_num(const char *s)
 }
 
 static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
-				     dfu_buf[DFU_DATA_BUF_SIZE];
+				     dfu_buf[CONFIG_SYS_DFU_DATA_BUF_SIZE];
 
 static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 {
diff --git a/include/dfu.h b/include/dfu.h
index a107f4b..124653c 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -68,7 +68,9 @@ static inline unsigned int get_mmc_blk_size(int dev)
 
 #define DFU_NAME_SIZE			32
 #define DFU_CMD_BUF_SIZE		128
-#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
+#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
+#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
+#endif
 #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
 #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
 #endif
-- 
1.7.11.7

[U-Boot] dfu: make data buffer size configurable

[Pantelis Antoniou]

Hi Heiko,

Just thinking out loud here. Can we have an extra option that
allocates the buffer dynamically based on an env variable?

Regards

-- Pantelis

On Jun 4, 2013, at 12:22 PM, Heiko Schocher wrote:

> Dfu transfer uses a buffer before writing data to the
> raw storage device. Make the size (in bytes) of this buffer
> configurable.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Tom Rini <trini@ti.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
> README            | 5 +++++
> drivers/dfu/dfu.c | 2 +-
> include/dfu.h     | 4 +++-
> 3 Dateien ge?ndert, 9 Zeilen hinzugef?gt(+), 2 Zeilen entfernt(-)
> 
> diff --git a/README b/README
> index b1b3e17..8550f34 100644
> --- a/README
> +++ b/README
> @@ -1360,6 +1360,11 @@ The following options need to be configured:
> 		CONFIG_DFU_NAND
> 		This enables support for exposing NAND devices via DFU.
> 
> +		CONFIG_SYS_DFU_DATA_BUF_SIZE
> +		Dfu transfer uses a buffer before writing data to the
> +		raw storage device. Make the size (in bytes) of this buffer
> +		configurable.
> +
> 		CONFIG_SYS_DFU_MAX_FILE_SIZE
> 		When updating files rather than the raw storage device,
> 		we use a static buffer to copy the file into and then write
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 6af6890..fe3a36e 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -42,7 +42,7 @@ static int dfu_find_alt_num(const char *s)
> }
> 
> static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
> -				     dfu_buf[DFU_DATA_BUF_SIZE];
> +				     dfu_buf[CONFIG_SYS_DFU_DATA_BUF_SIZE];
> 
> static int dfu_write_buffer_drain(struct dfu_entity *dfu)
> {
> diff --git a/include/dfu.h b/include/dfu.h
> index a107f4b..124653c 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -68,7 +68,9 @@ static inline unsigned int get_mmc_blk_size(int dev)
> 
> #define DFU_NAME_SIZE			32
> #define DFU_CMD_BUF_SIZE		128
> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> +#endif
> #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
> #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
> #endif
> -- 
> 1.7.11.7
> 

[U-Boot] dfu: make data buffer size configurable

[Lukasz Majewski]

Hi Heiko,

> Dfu transfer uses a buffer before writing data to the
> raw storage device. Make the size (in bytes) of this buffer
> configurable.
> 

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Tom Rini <trini@ti.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
>  README            | 5 +++++
>  drivers/dfu/dfu.c | 2 +-
>  include/dfu.h     | 4 +++-
>  3 Dateien ge?ndert, 9 Zeilen hinzugef?gt(+), 2 Zeilen entfernt(-)
> 
> diff --git a/README b/README
> index b1b3e17..8550f34 100644
> --- a/README
> +++ b/README
> @@ -1360,6 +1360,11 @@ The following options need to be configured:
>  		CONFIG_DFU_NAND
>  		This enables support for exposing NAND devices via
> DFU. 
> +		CONFIG_SYS_DFU_DATA_BUF_SIZE
> +		Dfu transfer uses a buffer before writing data to the
> +		raw storage device. Make the size (in bytes) of this
> buffer
> +		configurable.
> +
>  		CONFIG_SYS_DFU_MAX_FILE_SIZE
>  		When updating files rather than the raw storage
> device, we use a static buffer to copy the file into and then write
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 6af6890..fe3a36e 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -42,7 +42,7 @@ static int dfu_find_alt_num(const char *s)
>  }
>  
>  static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
> -				     dfu_buf[DFU_DATA_BUF_SIZE];
> +
> dfu_buf[CONFIG_SYS_DFU_DATA_BUF_SIZE]; 
>  static int dfu_write_buffer_drain(struct dfu_entity *dfu)
>  {
> diff --git a/include/dfu.h b/include/dfu.h
> index a107f4b..124653c 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -68,7 +68,9 @@ static inline unsigned int get_mmc_blk_size(int dev)
>  
>  #define DFU_NAME_SIZE			32
>  #define DFU_CMD_BUF_SIZE		128
> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8
> MiB */ +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE
> (1024*1024*8)	/* 8 MiB */ +#endif
>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4
> MiB */ #endif



-- 
Best regards,

Lukasz Majewski

Samsung R&D Poland Institute (SRPOL) | Linux Platform Group

[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher]

Hello Pantelis,

Am 04.06.2013 12:08, schrieb Pantelis Antoniou:
> Hi Heiko,
> 
> Just thinking out loud here. Can we have an extra option that
> allocates the buffer dynamically based on an env variable?

Hmm.. also a possibility... I have here no preferences ...

Name: "dfu_data_buf_size" if not defined, or env invalid, use
default CONFIG_SYS_DFU_DATA_BUF_SIZE size?

But this can be done in a second step, right?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] dfu: make data buffer size configurable

[Pantelis Antoniou]

Heiko,

On Jun 4, 2013, at 1:31 PM, Heiko Schocher wrote:

> Hello Pantelis,
> 
> Am 04.06.2013 12:08, schrieb Pantelis Antoniou:
>> Hi Heiko,
>> 
>> Just thinking out loud here. Can we have an extra option that
>> allocates the buffer dynamically based on an env variable?
> 
> Hmm.. also a possibility... I have here no preferences ...
> 
> Name: "dfu_data_buf_size" if not defined, or env invalid, use
> default CONFIG_SYS_DFU_DATA_BUF_SIZE size?
> 
> But this can be done in a second step, right?
> 

Yes, only as a second step please.

> bye,
> Heiko
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Regards

-- Pantelis

[U-Boot] dfu: make data buffer size configurable

[Tom Rini]

On Tue, Jun 04, 2013 at 11:22:54AM +0200, Heiko Schocher wrote:

> Dfu transfer uses a buffer before writing data to the
> raw storage device. Make the size (in bytes) of this buffer
> configurable.

NAK.
> +		CONFIG_SYS_DFU_DATA_BUF_SIZE
> +		Dfu transfer uses a buffer before writing data to the
> +		raw storage device. Make the size (in bytes) of this buffer
> +		configurable.
> +
>  		CONFIG_SYS_DFU_MAX_FILE_SIZE
>  		When updating files rather than the raw storage device,
>  		we use a static buffer to copy the file into and then write

The point of the buffer being configurable is to allow for larger files,
right?  We need to fix CONFIG_SYS_DFU_MAX_FILE_SIZE so that..

> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> +#endif
>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
>  #endif

We use one variable for both spots.  Or is there some case I'm missing
where we need to buffer 8MiB at a time for raw writes?  In which case we
still need to make CONFIG_SYS_DFU_MAX_FILE_SIZE be used :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130604/9690fcdf/attachment.pgp>

[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher]

Hello Tom,

Am 04.06.2013 22:04, schrieb Tom Rini:
> On Tue, Jun 04, 2013 at 11:22:54AM +0200, Heiko Schocher wrote:
> 
>> Dfu transfer uses a buffer before writing data to the
>> raw storage device. Make the size (in bytes) of this buffer
>> configurable.
> 
> NAK.

:-(

>> +		CONFIG_SYS_DFU_DATA_BUF_SIZE
>> +		Dfu transfer uses a buffer before writing data to the
>> +		raw storage device. Make the size (in bytes) of this buffer
>> +		configurable.
>> +
>>  		CONFIG_SYS_DFU_MAX_FILE_SIZE
>>  		When updating files rather than the raw storage device,
>>  		we use a static buffer to copy the file into and then write
> 
> The point of the buffer being configurable is to allow for larger files,
> right?  We need to fix CONFIG_SYS_DFU_MAX_FILE_SIZE so that..

In current code CONFIG_SYS_DFU_MAX_FILE_SIZE is not used in dfu_nand.c,
as if buffer is full, it is immediately flushed to nand.
Also default value from CONFIG_SYS_DFU_MAX_FILE_SIZE is smaller (4MiB)
as default value of CONFIG_SYS_DFU_DATA_BUF_SIZE (8MiB) ...

I used on my upcoming board port a CONFIG_SYS_DFU_DATA_BUF_SIZE from
1MiB and that worked perfectly, when transferring a file > 200MB.
The default value from 8MiB sometimes caused an error on the host:

[]# date;dfu-util -a rootfs -D dxr2-org/dxr2.xx-release-image-UNKNOWN-dxr2.ubi;date
Di 28. Mai 14:20:44 CEST 2013
dfu-util 0.5
[...]
Copying data from PC to DFU device
Starting download: [#############################################dfu_download: libusb_control_transfer returned -7
Error during download

Why we have a buffersize from 8MiB for raw writes, but a max file size
from 4MiB only?


>> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
>> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>> +#endif
>>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
>>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
>>  #endif
> 
> We use one variable for both spots.  Or is there some case I'm missing
> where we need to buffer 8MiB at a time for raw writes?  In which case we
> still need to make CONFIG_SYS_DFU_MAX_FILE_SIZE be used :)

I do not really know, why we have 2 defines here!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] dfu: make data buffer size configurable

[Tom Rini]

On Wed, Jun 05, 2013 at 06:53:53AM +0200, Heiko Schocher wrote:
> Hello Tom,
> 
> Am 04.06.2013 22:04, schrieb Tom Rini:
> > On Tue, Jun 04, 2013 at 11:22:54AM +0200, Heiko Schocher wrote:
> > 
> >> Dfu transfer uses a buffer before writing data to the
> >> raw storage device. Make the size (in bytes) of this buffer
> >> configurable.
> > 
> > NAK.
> 
> :-(
> 
> >> +		CONFIG_SYS_DFU_DATA_BUF_SIZE
> >> +		Dfu transfer uses a buffer before writing data to the
> >> +		raw storage device. Make the size (in bytes) of this buffer
> >> +		configurable.
> >> +
> >>  		CONFIG_SYS_DFU_MAX_FILE_SIZE
> >>  		When updating files rather than the raw storage device,
> >>  		we use a static buffer to copy the file into and then write
> > 
> > The point of the buffer being configurable is to allow for larger files,
> > right?  We need to fix CONFIG_SYS_DFU_MAX_FILE_SIZE so that..
> 
> In current code CONFIG_SYS_DFU_MAX_FILE_SIZE is not used in dfu_nand.c,

Nor anywhere else.  As I said in the DFU + UBI thread, there's a bug
here :)

> as if buffer is full, it is immediately flushed to nand.
> Also default value from CONFIG_SYS_DFU_MAX_FILE_SIZE is smaller (4MiB)
> as default value of CONFIG_SYS_DFU_DATA_BUF_SIZE (8MiB) ...

Right, and the commit that did it was about increasing the size of the
kernel that could be sent over.

> I used on my upcoming board port a CONFIG_SYS_DFU_DATA_BUF_SIZE from
> 1MiB and that worked perfectly, when transferring a file > 200MB.
> The default value from 8MiB sometimes caused an error on the host:
> 
> []# date;dfu-util -a rootfs -D dxr2-org/dxr2.xx-release-image-UNKNOWN-dxr2.ubi;date
> Di 28. Mai 14:20:44 CEST 2013
> dfu-util 0.5
> [...]
> Copying data from PC to DFU device
> Starting download: [#############################################dfu_download: libusb_control_transfer returned -7
> Error during download
> 
> Why we have a buffersize from 8MiB for raw writes, but a max file size
> from 4MiB only?

Then we need to poke around the code here a bit more and see what's
going on, and fix things so that we can both do larger (say, 8MiB)
filesystem transfers and not have dfu-util get mad sometimes.

> >> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> >> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
> >> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> >> +#endif
> >>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
> >>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
> >>  #endif
> > 
> > We use one variable for both spots.  Or is there some case I'm missing
> > where we need to buffer 8MiB at a time for raw writes?  In which case we
> > still need to make CONFIG_SYS_DFU_MAX_FILE_SIZE be used :)
> 
> I do not really know, why we have 2 defines here!

File size vs buffer size?  I'm not quite certain it was the right way to
go either.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130605/bc2e7e74/attachment.pgp>

[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher]

Hello Tom,

Am 05.06.2013 14:43, schrieb Tom Rini:
> On Wed, Jun 05, 2013 at 06:53:53AM +0200, Heiko Schocher wrote:
>> Hello Tom,
>>
>> Am 04.06.2013 22:04, schrieb Tom Rini:
>>> On Tue, Jun 04, 2013 at 11:22:54AM +0200, Heiko Schocher wrote:
[...]
>>>> +		CONFIG_SYS_DFU_DATA_BUF_SIZE
>>>> +		Dfu transfer uses a buffer before writing data to the
>>>> +		raw storage device. Make the size (in bytes) of this buffer
>>>> +		configurable.
>>>> +
>>>>  		CONFIG_SYS_DFU_MAX_FILE_SIZE
>>>>  		When updating files rather than the raw storage device,
>>>>  		we use a static buffer to copy the file into and then write
>>>
>>> The point of the buffer being configurable is to allow for larger files,
>>> right?  We need to fix CONFIG_SYS_DFU_MAX_FILE_SIZE so that..
>>
>> In current code CONFIG_SYS_DFU_MAX_FILE_SIZE is not used in dfu_nand.c,
> 
> Nor anywhere else.  As I said in the DFU + UBI thread, there's a bug
> here :)

CONFIG_SYS_DFU_MAX_FILE_SIZE is used in ./drivers/dfu/dfu_mmc.c ...

>> as if buffer is full, it is immediately flushed to nand.
>> Also default value from CONFIG_SYS_DFU_MAX_FILE_SIZE is smaller (4MiB)
>> as default value of CONFIG_SYS_DFU_DATA_BUF_SIZE (8MiB) ...
> 
> Right, and the commit that did it was about increasing the size of the
> kernel that could be sent over.

Hmm.. the CONFIG_SYS_DFU_DATA_BUF_SIZE limits not the size of
a file that could be loaded into a partition. It specifies
only the size of one chunk, that get burned into the raw nand ...

And this should be a configurable size ...

>> I used on my upcoming board port a CONFIG_SYS_DFU_DATA_BUF_SIZE from
>> 1MiB and that worked perfectly, when transferring a file > 200MB.
>> The default value from 8MiB sometimes caused an error on the host:
>>
>> []# date;dfu-util -a rootfs -D dxr2-org/dxr2.xx-release-image-UNKNOWN-dxr2.ubi;date
>> Di 28. Mai 14:20:44 CEST 2013
>> dfu-util 0.5
>> [...]
>> Copying data from PC to DFU device
>> Starting download: [#############################################dfu_download: libusb_control_transfer returned -7
>> Error during download
>>
>> Why we have a buffersize from 8MiB for raw writes, but a max file size
>> from 4MiB only?
> 
> Then we need to poke around the code here a bit more and see what's
> going on, and fix things so that we can both do larger (say, 8MiB)
> filesystem transfers and not have dfu-util get mad sometimes.

Timeout in libusb_control_transfer while the target writes
the 8MiB into the nand ... ?

I try to find out something ...

>>>> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>>>> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
>>>> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>>>> +#endif
>>>>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
>>>>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
>>>>  #endif
>>>
>>> We use one variable for both spots.  Or is there some case I'm missing
>>> where we need to buffer 8MiB at a time for raw writes?  In which case we
>>> still need to make CONFIG_SYS_DFU_MAX_FILE_SIZE be used :)
>>
>> I do not really know, why we have 2 defines here!
> 
> File size vs buffer size?  I'm not quite certain it was the right way to
> go either.

Yeah, but why is the file size < buffer size as default?

In dfu_mmc:
If raw partition, if dfu_buf (size of CONFIG_SYS_DFU_DATA_BUF_SIZE)
full -> write it to the mmc. Same for nand.

If FAT or EXT4 partition (mmc only), write the dfu_buffer through
mmc_file_buffer() to dfu_file_buf[CONFIG_SYS_DFU_MAX_FILE_SIZE] ...
this seems buggy to me, but maybe I oversee something, I could not
try it ... and if the hole file is transfered, the dfu_file_buf
gets flushed to the partition ...

The CONFIG_SYS_DFU_MAX_FILE_SIZE is only needed as we can only
write a complete image to FAT, EXT4 (also UBI) partitions, I think.

So we have in the dfu subsystem following problems:

a) we have no common API to add image types. Currently
   all dfu_{device_type} has to program it.
b) we have no possibility to write image types (FAT, EXT4 or
   UBI) in chunks -> CONFIG_SYS_DFU_MAX_FILE_SIZE introduced
c) CONFIG_SYS_DFU_DATA_BUF_SIZE > CONFIG_SYS_DFU_MAX_FILE_SIZE
   which is in my eyes buggy ...
d) sporadic problems with CONFIG_SYS_DFU_DATA_BUF_SIZE = 8MiB
   Currently i get always an error ... try to find out why ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] dfu: make data buffer size configurable

[Tom Rini]

On Wed, Jun 05, 2013 at 04:04:46PM +0200, Heiko Schocher wrote:

[snip]
> >> In current code CONFIG_SYS_DFU_MAX_FILE_SIZE is not used in dfu_nand.c,
> > 
> > Nor anywhere else.  As I said in the DFU + UBI thread, there's a bug
> > here :)
> 
> CONFIG_SYS_DFU_MAX_FILE_SIZE is used in ./drivers/dfu/dfu_mmc.c ...

Ah yes, right.

> >> as if buffer is full, it is immediately flushed to nand.
> >> Also default value from CONFIG_SYS_DFU_MAX_FILE_SIZE is smaller (4MiB)
> >> as default value of CONFIG_SYS_DFU_DATA_BUF_SIZE (8MiB) ...
> > 
> > Right, and the commit that did it was about increasing the size of the
> > kernel that could be sent over.
> 
> Hmm.. the CONFIG_SYS_DFU_DATA_BUF_SIZE limits not the size of
> a file that could be loaded into a partition. It specifies
> only the size of one chunk, that get burned into the raw nand ...
> 
> And this should be a configurable size ...

Or a saner, smaller size?  I think we've got a few things being done in
a confusing and/or incorrect manner, see below.

> >> I used on my upcoming board port a CONFIG_SYS_DFU_DATA_BUF_SIZE from
> >> 1MiB and that worked perfectly, when transferring a file > 200MB.
> >> The default value from 8MiB sometimes caused an error on the host:
> >>
> >> []# date;dfu-util -a rootfs -D dxr2-org/dxr2.xx-release-image-UNKNOWN-dxr2.ubi;date
> >> Di 28. Mai 14:20:44 CEST 2013
> >> dfu-util 0.5
> >> [...]
> >> Copying data from PC to DFU device
> >> Starting download: [#############################################dfu_download: libusb_control_transfer returned -7
> >> Error during download
> >>
> >> Why we have a buffersize from 8MiB for raw writes, but a max file size
> >> from 4MiB only?
> > 
> > Then we need to poke around the code here a bit more and see what's
> > going on, and fix things so that we can both do larger (say, 8MiB)
> > filesystem transfers and not have dfu-util get mad sometimes.
> 
> Timeout in libusb_control_transfer while the target writes
> the 8MiB into the nand ... ?
> 
> I try to find out something ...

Well, it ought to be fine, but we're pushing some boundaries here, and
I don't know if we have a good reason for it.  We aren't changing the
size of the chunks being passed along.

> >>>> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> >>>> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
> >>>> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
> >>>> +#endif
> >>>>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
> >>>>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
> >>>>  #endif
> >>>
> >>> We use one variable for both spots.  Or is there some case I'm missing
> >>> where we need to buffer 8MiB at a time for raw writes?  In which case we
> >>> still need to make CONFIG_SYS_DFU_MAX_FILE_SIZE be used :)
> >>
> >> I do not really know, why we have 2 defines here!
> > 
> > File size vs buffer size?  I'm not quite certain it was the right way to
> > go either.
> 
> Yeah, but why is the file size < buffer size as default?

A bug, I'm pretty sure.  The change that made buffer > file was with the
comment to allow for bigger files.  I think it might not have been
completely tested :)

> In dfu_mmc:
> If raw partition, if dfu_buf (size of CONFIG_SYS_DFU_DATA_BUF_SIZE)
> full -> write it to the mmc. Same for nand.

Right.  Since we want to buffer up some amount of data, flush, repeat
until done.

> If FAT or EXT4 partition (mmc only), write the dfu_buffer through
> mmc_file_buffer() to dfu_file_buf[CONFIG_SYS_DFU_MAX_FILE_SIZE] ...
> this seems buggy to me, but maybe I oversee something, I could not
> try it ... and if the hole file is transfered, the dfu_file_buf
> gets flushed to the partition ...

The need here is that since we can't append to files, transfer the whole
file, then write.  We will not be told the filesize ahead of time, so we
have to transfer up to the buffer and if we would exceed, error out at
that point, otherwise continue.  Now I'm pretty sure, but not 100% sure
that there's a reason we can't use dfu_buf in both places.  That needs
re-checking.

> The CONFIG_SYS_DFU_MAX_FILE_SIZE is only needed as we can only
> write a complete image to FAT, EXT4 (also UBI) partitions, I think.

It'll be needed for any file write, rather than block write.  The
question is, can it ever be valid for MAX_FILE_SIZE to be smaller than
BUF_SIZE ?  Maybe.

> So we have in the dfu subsystem following problems:
> 
> a) we have no common API to add image types. Currently
>    all dfu_{device_type} has to program it.

We also have no common image types.

> b) we have no possibility to write image types (FAT, EXT4 or
>    UBI) in chunks -> CONFIG_SYS_DFU_MAX_FILE_SIZE introduced

Correct.

> c) CONFIG_SYS_DFU_DATA_BUF_SIZE > CONFIG_SYS_DFU_MAX_FILE_SIZE
>    which is in my eyes buggy ...

Or at least very odd, given the current defaults for both.

> d) sporadic problems with CONFIG_SYS_DFU_DATA_BUF_SIZE = 8MiB
>    Currently i get always an error ... try to find out why ...

Maybe we don't need to go so large for the buffer.

If you've got a beaglebone (and I know there's one in denx :)) or am335x
gp evm, you can test filesystem writes on SD cards with DFU.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130606/9af3e6b5/attachment.pgp>

[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher]

Hello Tom,

Am 06.06.2013 17:55, schrieb Tom Rini:
> On Wed, Jun 05, 2013 at 04:04:46PM +0200, Heiko Schocher wrote:
> 
> [snip]
>>>> In current code CONFIG_SYS_DFU_MAX_FILE_SIZE is not used in dfu_nand.c,
>>>
>>> Nor anywhere else.  As I said in the DFU + UBI thread, there's a bug
>>> here :)
>>
>> CONFIG_SYS_DFU_MAX_FILE_SIZE is used in ./drivers/dfu/dfu_mmc.c ...
> 
> Ah yes, right.
> 
>>>> as if buffer is full, it is immediately flushed to nand.
>>>> Also default value from CONFIG_SYS_DFU_MAX_FILE_SIZE is smaller (4MiB)
>>>> as default value of CONFIG_SYS_DFU_DATA_BUF_SIZE (8MiB) ...
>>>
>>> Right, and the commit that did it was about increasing the size of the
>>> kernel that could be sent over.
>>
>> Hmm.. the CONFIG_SYS_DFU_DATA_BUF_SIZE limits not the size of
>> a file that could be loaded into a partition. It specifies
>> only the size of one chunk, that get burned into the raw nand ...
>>
>> And this should be a configurable size ...
> 
> Or a saner, smaller size?  I think we've got a few things being done in
> a confusing and/or incorrect manner, see below.

Yes, smaller size is maybe the way to go ... see below

>>>> I used on my upcoming board port a CONFIG_SYS_DFU_DATA_BUF_SIZE from
>>>> 1MiB and that worked perfectly, when transferring a file > 200MB.
>>>> The default value from 8MiB sometimes caused an error on the host:
>>>>
>>>> []# date;dfu-util -a rootfs -D dxr2-org/dxr2.xx-release-image-UNKNOWN-dxr2.ubi;date
>>>> Di 28. Mai 14:20:44 CEST 2013
>>>> dfu-util 0.5
>>>> [...]
>>>> Copying data from PC to DFU device
>>>> Starting download: [#############################################dfu_download: libusb_control_transfer returned -7
>>>> Error during download
>>>>
>>>> Why we have a buffersize from 8MiB for raw writes, but a max file size
>>>> from 4MiB only?
>>>
>>> Then we need to poke around the code here a bit more and see what's
>>> going on, and fix things so that we can both do larger (say, 8MiB)
>>> filesystem transfers and not have dfu-util get mad sometimes.
>>
>> Timeout in libusb_control_transfer while the target writes
>> the 8MiB into the nand ... ?
>>
>> I try to find out something ...
> 
> Well, it ought to be fine, but we're pushing some boundaries here, and
> I don't know if we have a good reason for it.  We aren't changing the
> size of the chunks being passed along.

My suspicion that the problem is a timeout was the right idea!

I tried following patch in the current dfu-util sources:
(git clone git://gitorious.org/dfu-util/dfu-util.git
 current head: f66634406e9397e67c34033c3c0c26dde486528c)

diff --git a/src/main.c b/src/main.c
index 2aea134..12f6f1d 100644
--- a/src/main.c
+++ b/src/main.c
@@ -290,7 +290,8 @@ int main(int argc, char **argv)
 		exit(0);
 	}

-	dfu_init(5000);
+	printf("Using 20 sec timeout\n");
+	dfu_init(20000);

 	num_devs = count_dfu_devices(ctx, dif);
 	if (num_devs == 0) {

and I see no longer the above error! So I see two solutions
for my problem:

- make DFU_DATA_BUF_SIZE in U-Boot smaller or configurable
- make the timeout in dfu-util bigger or configurable

>>>>>> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>>>>>> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
>>>>>> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>>>>>> +#endif
>>>>>>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
>>>>>>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
>>>>>>  #endif
>>>>>
>>>>> We use one variable for both spots.  Or is there some case I'm missing
>>>>> where we need to buffer 8MiB at a time for raw writes?  In which case we
>>>>> still need to make CONFIG_SYS_DFU_MAX_FILE_SIZE be used :)
>>>>
>>>> I do not really know, why we have 2 defines here!
>>>
>>> File size vs buffer size?  I'm not quite certain it was the right way to
>>> go either.
>>
>> Yeah, but why is the file size < buffer size as default?
> 
> A bug, I'm pretty sure.  The change that made buffer > file was with the
> comment to allow for bigger files.  I think it might not have been
> completely tested :)

Maybe. I don't know.

>> In dfu_mmc:
>> If raw partition, if dfu_buf (size of CONFIG_SYS_DFU_DATA_BUF_SIZE)
>> full -> write it to the mmc. Same for nand.
> 
> Right.  Since we want to buffer up some amount of data, flush, repeat
> until done.

Yep.

>> If FAT or EXT4 partition (mmc only), write the dfu_buffer through
>> mmc_file_buffer() to dfu_file_buf[CONFIG_SYS_DFU_MAX_FILE_SIZE] ...
>> this seems buggy to me, but maybe I oversee something, I could not
>> try it ... and if the hole file is transfered, the dfu_file_buf
>> gets flushed to the partition ...
> 
> The need here is that since we can't append to files, transfer the whole
> file, then write.  We will not be told the filesize ahead of time, so we
> have to transfer up to the buffer and if we would exceed, error out at
> that point, otherwise continue.  Now I'm pretty sure, but not 100% sure
> that there's a reason we can't use dfu_buf in both places.  That needs
> re-checking.

Ok.

>> The CONFIG_SYS_DFU_MAX_FILE_SIZE is only needed as we can only
>> write a complete image to FAT, EXT4 (also UBI) partitions, I think.
> 
> It'll be needed for any file write, rather than block write.  The
> question is, can it ever be valid for MAX_FILE_SIZE to be smaller than
> BUF_SIZE ?  Maybe.

I would say no ...

>> So we have in the dfu subsystem following problems:
>>
>> a) we have no common API to add image types. Currently
>>    all dfu_{device_type} has to program it.
> 
> We also have no common image types.

Then we should introduce it ... oh, maybe we have them:
drivers/dfu/dfu_mmc.c in dfu_write_medium_mmc()i

switch(dfu->layout)
DFU_RAW_ADDR:
DFU_FS_FAT:
DFU_FS_EXT4:

?
>> b) we have no possibility to write image types (FAT, EXT4 or
>>    UBI) in chunks -> CONFIG_SYS_DFU_MAX_FILE_SIZE introduced
> 
> Correct.
> 
>> c) CONFIG_SYS_DFU_DATA_BUF_SIZE > CONFIG_SYS_DFU_MAX_FILE_SIZE
>>    which is in my eyes buggy ...
> 
> Or at least very odd, given the current defaults for both.
> 
>> d) sporadic problems with CONFIG_SYS_DFU_DATA_BUF_SIZE = 8MiB
>>    Currently i get always an error ... try to find out why ...
> 
> Maybe we don't need to go so large for the buffer.

Yep, with 1 or 2 MiB I see no problems with current dfu-util.

> If you've got a beaglebone (and I know there's one in denx :)) or am335x
> gp evm, you can test filesystem writes on SD cards with DFU.

Ah! Ok.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] dfu: make data buffer size configurable

[Wolfgang Denk]

Dear Heiko,

In message <51B17815.8060904@denx.de> you wrote:
> 
> and I see no longer the above error! So I see two solutions
> for my problem:
> 
> - make DFU_DATA_BUF_SIZE in U-Boot smaller or configurable
> - make the timeout in dfu-util bigger or configurable

I think we have just learned that the one-size-fits-all approach does
not work for everybody.  Actually both parameters should be
configurable.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Artificial Intelligence is no match for natural stupidity.

[U-Boot] dfu: make data buffer size configurable

[Marek Vasut]

Dear Pantelis Antoniou,

> Heiko,
> 
> On Jun 4, 2013, at 1:31 PM, Heiko Schocher wrote:
> > Hello Pantelis,
> > 
> > Am 04.06.2013 12:08, schrieb Pantelis Antoniou:
> >> Hi Heiko,
> >> 
> >> Just thinking out loud here. Can we have an extra option that
> >> allocates the buffer dynamically based on an env variable?
> > 
> > Hmm.. also a possibility... I have here no preferences ...
> > 
> > Name: "dfu_data_buf_size" if not defined, or env invalid, use
> > default CONFIG_SYS_DFU_DATA_BUF_SIZE size?
> > 
> > But this can be done in a second step, right?
> 
> Yes, only as a second step please.

Why not do it like that right away?

btw. sorry, I'm playing dead beetle until next week :-(

Best regards,
Marek Vasut

[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher]

Hello Marek,

Am 09.06.2013 22:01, schrieb Marek Vasut:
> Dear Pantelis Antoniou,
> 
>> Heiko,
>>
>> On Jun 4, 2013, at 1:31 PM, Heiko Schocher wrote:
>>> Hello Pantelis,
>>>
>>> Am 04.06.2013 12:08, schrieb Pantelis Antoniou:
>>>> Hi Heiko,
>>>>
>>>> Just thinking out loud here. Can we have an extra option that
>>>> allocates the buffer dynamically based on an env variable?
>>>
>>> Hmm.. also a possibility... I have here no preferences ...
>>>
>>> Name: "dfu_data_buf_size" if not defined, or env invalid, use
>>> default CONFIG_SYS_DFU_DATA_BUF_SIZE size?
>>>
>>> But this can be done in a second step, right?
>>
>> Yes, only as a second step please.
> 
> Why not do it like that right away?

Ok, I can change this. Envvar name "dfu_data_buf_size" is ok?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] dfu: make data buffer size configurable

[Lukasz Majewski]

Hi Heiko,

> Hello Marek,
> 
> Am 09.06.2013 22:01, schrieb Marek Vasut:
> > Dear Pantelis Antoniou,
> > 
> >> Heiko,
> >>
> >> On Jun 4, 2013, at 1:31 PM, Heiko Schocher wrote:
> >>> Hello Pantelis,
> >>>
> >>> Am 04.06.2013 12:08, schrieb Pantelis Antoniou:
> >>>> Hi Heiko,
> >>>>
> >>>> Just thinking out loud here. Can we have an extra option that
> >>>> allocates the buffer dynamically based on an env variable?
> >>>
> >>> Hmm.. also a possibility... I have here no preferences ...
> >>>
> >>> Name: "dfu_data_buf_size" if not defined, or env invalid, use
> >>> default CONFIG_SYS_DFU_DATA_BUF_SIZE size?
> >>>
> >>> But this can be done in a second step, right?
> >>
> >> Yes, only as a second step please.
> > 
> > Why not do it like that right away?
> 
> Ok, I can change this. Envvar name "dfu_data_buf_size" is ok?

For me it's fine.

> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] dfu: make data buffer size configurable

[Wolfgang Denk]

Dear Heiko Schocher,

In message <51B555D7.5010001@denx.de> you wrote:
> 
> Ok, I can change this. Envvar name "dfu_data_buf_size" is ok?

Such long names are a paint to type. As we can't buffer anything else
but data, we should be able to omit this, i. e. what about

	dfu_bufsiz

["bufsiz" as used for example as BUFSIZ in C89 stdio.h]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Fascinating is a word I use for the unexpected.
	-- Spock, "The Squire of Gothos", stardate 2124.5

[U-Boot] dfu: make data buffer size configurable

[Tom Rini]

On Mon, Jun 10, 2013 at 09:05:48AM +0200, Wolfgang Denk wrote:
> Dear Heiko Schocher,
> 
> In message <51B555D7.5010001@denx.de> you wrote:
> > 
> > Ok, I can change this. Envvar name "dfu_data_buf_size" is ok?
> 
> Such long names are a paint to type. As we can't buffer anything else
> but data, we should be able to omit this, i. e. what about
> 
> 	dfu_bufsiz
> 
> ["bufsiz" as used for example as BUFSIZ in C89 stdio.h]

Works for me.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130610/5a58b873/attachment.pgp>

[U-Boot] [PATCH v2] dfu: make data buffer size configurable

[Heiko Schocher]

Dfu transfer uses a buffer before writing data to the
raw storage device. Make the size (in bytes) of this buffer
configurable through environment variable "dfu_bufsiz".
Defaut value is configurable through CONFIG_SYS_DFU_DATA_BUF_SIZE

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
Cc: Tom Rini <trini@ti.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Wolfgang Denk <wd@denx.de>

---
- changes for v2:
  make the dfu_buf size configurable through the environment
  variable "dfu_bufsiz".

 README            |  6 ++++++
 drivers/dfu/dfu.c | 49 +++++++++++++++++++++++++++++++++++++++++--------
 include/dfu.h     |  4 +++-
 3 Dateien ge?ndert, 50 Zeilen hinzugef?gt(+), 9 Zeilen entfernt(-)

diff --git a/README b/README
index 33bda8c..40f4353 100644
--- a/README
+++ b/README
@@ -1376,6 +1376,12 @@ The following options need to be configured:
 		CONFIG_DFU_NAND
 		This enables support for exposing NAND devices via DFU.
 
+		CONFIG_SYS_DFU_DATA_BUF_SIZE
+		Dfu transfer uses a buffer before writing data to the
+		raw storage device. Make the size (in bytes) of this buffer
+		configurable. The size of this buffer is also configurable
+		through the "dfu_bufsiz" environment variable.
+
 		CONFIG_SYS_DFU_MAX_FILE_SIZE
 		When updating files rather than the raw storage device,
 		we use a static buffer to copy the file into and then write
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 6af6890..e429d74 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -20,6 +20,7 @@
  */
 
 #include <common.h>
+#include <errno.h>
 #include <malloc.h>
 #include <mmc.h>
 #include <fat.h>
@@ -41,8 +42,34 @@ static int dfu_find_alt_num(const char *s)
 	return ++i;
 }
 
-static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
-				     dfu_buf[DFU_DATA_BUF_SIZE];
+static unsigned char *dfu_buf;
+static unsigned long dfu_buf_size = CONFIG_SYS_DFU_DATA_BUF_SIZE;
+
+static unsigned char *dfu_free_buf(void)
+{
+	free(dfu_buf);
+	dfu_buf = NULL;
+	return dfu_buf;
+}
+
+static unsigned char *dfu_get_buf(void)
+{
+	char *s;
+
+	if (dfu_buf != NULL)
+		return dfu_buf;
+
+	s = getenv("dfu_bufsiz");
+	dfu_buf_size = s ? (unsigned long)simple_strtol(s, NULL, 16) :
+			CONFIG_SYS_DFU_DATA_BUF_SIZE;
+
+	dfu_buf = memalign(CONFIG_SYS_CACHELINE_SIZE, dfu_buf_size);
+	if (dfu_buf == NULL)
+		printf("%s: Could not memalign 0x%lx bytes\n",
+		       __func__, dfu_buf_size);
+
+	return dfu_buf;
+}
 
 static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 {
@@ -87,8 +114,10 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 		dfu->offset = 0;
 		dfu->bad_skip = 0;
 		dfu->i_blk_seq_num = 0;
-		dfu->i_buf_start = dfu_buf;
-		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf_start = dfu_get_buf();
+		if (dfu->i_buf_start == NULL)
+			return -ENOMEM;
+		dfu->i_buf_end = dfu_get_buf() + dfu_buf_size;
 		dfu->i_buf = dfu->i_buf_start;
 
 		dfu->inited = 1;
@@ -148,11 +177,12 @@ int dfu_write(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 		printf("\nDFU complete CRC32: 0x%08x\n", dfu->crc);
 
 		/* clear everything */
+		dfu_free_buf();
 		dfu->crc = 0;
 		dfu->offset = 0;
 		dfu->i_blk_seq_num = 0;
 		dfu->i_buf_start = dfu_buf;
-		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf_end = dfu_buf;
 		dfu->i_buf = dfu->i_buf_start;
 
 		dfu->inited = 0;
@@ -229,8 +259,10 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 		dfu->i_blk_seq_num = 0;
 		dfu->crc = 0;
 		dfu->offset = 0;
-		dfu->i_buf_start = dfu_buf;
-		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf_start = dfu_get_buf();
+		if (dfu->i_buf_start == NULL)
+			return -ENOMEM;
+		dfu->i_buf_end = dfu_get_buf() + dfu_buf_size;
 		dfu->i_buf = dfu->i_buf_start;
 		dfu->b_left = 0;
 
@@ -257,11 +289,12 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name, dfu->crc);
 		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
 
+		dfu_free_buf();
 		dfu->i_blk_seq_num = 0;
 		dfu->crc = 0;
 		dfu->offset = 0;
 		dfu->i_buf_start = dfu_buf;
-		dfu->i_buf_end = dfu_buf + sizeof(dfu_buf);
+		dfu->i_buf_end = dfu_buf;
 		dfu->i_buf = dfu->i_buf_start;
 		dfu->b_left = 0;
 
diff --git a/include/dfu.h b/include/dfu.h
index a107f4b..124653c 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -68,7 +68,9 @@ static inline unsigned int get_mmc_blk_size(int dev)
 
 #define DFU_NAME_SIZE			32
 #define DFU_CMD_BUF_SIZE		128
-#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
+#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
+#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
+#endif
 #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
 #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
 #endif
-- 
1.7.11.7

[U-Boot] dfu: make data buffer size configurable

[Marek Vasut]

Dear Tom Rini,

> On Mon, Jun 10, 2013 at 09:05:48AM +0200, Wolfgang Denk wrote:
> > Dear Heiko Schocher,
> > 
> > In message <51B555D7.5010001@denx.de> you wrote:
> > > Ok, I can change this. Envvar name "dfu_data_buf_size" is ok?
> > 
> > Such long names are a paint to type. As we can't buffer anything else
> > but data, we should be able to omit this, i. e. what about
> > 
> > 	dfu_bufsiz
> > 
> > ["bufsiz" as used for example as BUFSIZ in C89 stdio.h]
> 
> Works for me.

WFM, but I need to read the discussion around here. I just stopped playing dead 
beetle. Actually, I'm trying to find out why such a variable is needed at all. 
Can the buffer not be allocated dynamically (and thus dependant only on malloc 
area size)?

Please ignore this if it's been answered in one of the emails I didn't read yet.

Best regards,
Marek Vasut

[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher]

Hello Marek,

Am 12.06.2013 10:36, schrieb Marek Vasut:
> Dear Tom Rini,
> 
>> On Mon, Jun 10, 2013 at 09:05:48AM +0200, Wolfgang Denk wrote:
>>> Dear Heiko Schocher,
>>>
>>> In message <51B555D7.5010001@denx.de> you wrote:
>>>> Ok, I can change this. Envvar name "dfu_data_buf_size" is ok?
>>>
>>> Such long names are a paint to type. As we can't buffer anything else
>>> but data, we should be able to omit this, i. e. what about
>>>
>>> 	dfu_bufsiz
>>>
>>> ["bufsiz" as used for example as BUFSIZ in C89 stdio.h]
>>
>> Works for me.
> 
> WFM, but I need to read the discussion around here. I just stopped playing dead 
> beetle. Actually, I'm trying to find out why such a variable is needed at all. 
> Can the buffer not be allocated dynamically (and thus dependant only on malloc 
> area size)?
> 
> Please ignore this if it's been answered in one of the emails I didn't read yet.

posted a v2 for this here:

http://patchwork.ozlabs.org/patch/250665/

which allocates the buffer in the malloc area, dependend on
the environemnt variable "dfu_bufsiz".

A reason why we should have at last a config option see here:
http://lists.denx.de/pipermail/u-boot/2013-June/155924.html

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v2] dfu: make data buffer size configurable

[Tom Rini]

On Wed, Jun 12, 2013 at 06:05:51AM +0200, Heiko Schocher wrote:

> Dfu transfer uses a buffer before writing data to the
> raw storage device. Make the size (in bytes) of this buffer
> configurable through environment variable "dfu_bufsiz".
> Defaut value is configurable through CONFIG_SYS_DFU_DATA_BUF_SIZE
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Tom Rini <trini@ti.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>

Acked-by: Tom Rini <trini@ti.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130612/900ccb73/attachment.pgp>

[U-Boot] [PATCH v2] dfu: make data buffer size configurable

[Marek Vasut]

Dear Heiko Schocher,

> Dfu transfer uses a buffer before writing data to the
> raw storage device. Make the size (in bytes) of this buffer
> configurable through environment variable "dfu_bufsiz".
> Defaut value is configurable through CONFIG_SYS_DFU_DATA_BUF_SIZE
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Tom Rini <trini@ti.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>

Applied, thanks!

Best regards,
Marek Vasut