| Message ID | 1370337774-16768-1-git-send-email-hs@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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
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
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 :)
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
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.
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
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.
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
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
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(-)