[U-Boot] dfu: make data buffer size configurable
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
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_SIZEDfu transfer uses a buffer before writing data to theraw storage device. Make the size (in bytes) of this bufferconfigurable.- 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
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
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
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
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
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
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
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.
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
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
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_SIZEDfu transfer uses a buffer before writing data to theraw storage device. Make the size (in bytes) of thisbuffer
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
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_SIZEDfu transfer uses a buffer before writing data to theraw storage device. Make the size (in bytes) of this bufferconfigurable.- 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 :)
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_SIZEDfu transfer uses a buffer before writing data to theraw storage device. Make the size (in bytes) of this bufferconfigurable.- 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
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_SIZEDfu transfer uses a buffer before writing data to theraw storage device. Make the size (in bytes) of this bufferconfigurable.- 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.
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_SIZEDfu transfer uses a buffer before writing data to theraw storage device. Make the size (in bytes) of this bufferconfigurable.- 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
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.
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
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
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
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
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
participants (6)
-
Heiko Schocher -
Lukasz Majewski -
Marek Vasut -
Pantelis Antoniou -
Tom Rini -
Wolfgang Denk