Hi Lukasz, > Hi Stephen, > > > On 05/22/2014 04:20 AM, Lukasz Majewski wrote: > > > Hi Stephen, > > > > > >> From: Stephen Warren <swar...@nvidia.com> > > >> > > >> DFU read support appears to rely upon dfu->read_medium() updating > > >> the passed-by-reference len parameter to indicate the remaining > > >> size available for reading. > > >> > > >> dfu_read_medium_mmc() never does this, and the implementation of > > >> dfu_read_medium_nand() will only work if called just once; it > > >> hard-codes the value to the total size of the NAND device > > >> irrespective of read offset. > > >> > > >> I believe that overloading dfu->read_medium() is confusing. As > > >> such, this patch introduces a new function dfu->get_medium_size() > > >> which can be used to explicitly find out the medium size, and > > >> nothing else. dfu_read() is modified to use this function to set > > >> the initial value for dfu->r_left, rather than attempting to use > > >> the side-effects of dfu->read_medium() for this purpose. > > >> > > >> Due to this change, dfu_read() must initially set dfu->b_left to > > >> 0, since no data has been read. > > >> > > >> dfu_read_buffer_fill() must also be modified not to adjust > > >> dfu->r_left when simply copying data from dfu->i_buf_start to the > > >> upload request buffer. r_left represents the amount of data left > > >> to be read from HW. That value is not affected by the memcpy(), > > >> but only by calls to dfu->read_medium(). > > >> > > >> After this change, I can read from either a 4MB or 1.5MB chunk > > >> of a 4MB eMMC boot partion with > > >> CONFIG_SYS_DFU_DATA_BUF_SIZE==1MB. Without this change, > > >> attempting to do that would result in DFU read returning no data > > >> at all due to r_left never being set. > > ... > > > I've tested it with trats2. It introduces regression with my tests > > > setup. > > > > > > I think that it is the highest time to share my test setup for > > > DFU. Proper patches would be prepared ASAP. > > > > Ah, your test scripts imply you're testing using files on a > > filesystem, whereas I've only tested with raw MMC partitions: > > > > setenv dfu_alt_info boot0 raw 0 0x2000 mmcpart 1\;boot1 raw 0 0x2000 > > mmcpart 2; dfu 0 mmc 0 > > > > (I test on alt info "boot1") > > > > Can you re-run your tests on a raw partition and validate they work > > OK for you? That would at least prove the concept of the patches. > > I'm not convinced tests on raw partitions would work with or > > without my patches. > > On the mmcparts I've got bootloders, so I would like to avoid erasing > them. > > I've tested if raw u-boot can be downloaded and uploaded via DFU. The > u-boot size is 1MiB precisely. > > Corresponding dfu_alt_info entry: > "u-boot raw 0x80 0x800;" \ > > SHA1 of the u-boot/master: > f23adc9f219977e603cf057a2704605349f02d36 > > 1. Download (raw): > > dfu-util -a0 -D u-boot-mmc.bin -> OK > I've double checked it with ums: > dd if=/dev/sdc of=u-boot-mmc.bin_ums skip=128 u-boot-mmc.bin_target > bs=512 count=2048 > > CRC match => downloading works. > > > 2. Upload (raw): > > dfu-util -a0 -U u-boot-mmc.bin_target > It exits immediately and I can only see the file size of 0. > > So obviously we have regression here. However, since I didn't covered > this case in my tests I don't know when it was broken. > > > BTW: I definitely have to extend the test script to handle this case.
I've tested this again with the newest u-boot-dfu branch and the above senario works. > > > > > I can see why my patches made files rather than raw partitions > > fail; I only wrote dfu_get_medium_size_mmc() to support raw > > partitions, so it needs extra code to work for files. Solving the > > same problem for files rather than raw partitions might be painful; > > I guess I'll see! > > The problem here is that raw files are far more larger than files to > be stored on the file system (as I've explained below). Moreover it > happens that raw files (like rootfs.img) are even bigger than > available RAM. > > > > > As an aside, mmc_file_op() doesn't seem to support files larger than > > the DFU buffer, size no offset information is ever passed to the > > fatload/fatwrite commands. Is that intentional? > > Unfortunately it is, since U-boot's implementation of FAT or Ext4 > doesn't support seek(). Due to that the whole file needs to fit into > the buffer before being written. > > > > > I'm also puzzled why the file IO code is part of dfu_mmc.c; commands > > like fat_load (or the internal U-Boot filesystem APIs) support more > > than just MMC. > > Initially, the idea was to separate IO to different mediums - that is > why we have dfu_mmc.c, dfu_nand.c, dfu_ram.c. > > Afterwards we had to adjust dfu_mmc to be able to perform ram IO and > other file systems. > > The DFU design was rapidly evolving when we started to use it. This > partially explains the current, rather ugly, state of the code. > > > Wouldn't it be better to have the following separate > > "backends" to DFU: > > > > * MMC (raw IO only) > > * NAND (raw IO only) > > * RAM (raw IO only) > > * Filesystem (anything that fs/fat/ext* load/write can support) > > > > That way, DFU could be applied to e.g. USB mass storage, IDE, > > SATA, ... devices too. > > I think that separating the IO even from dfu_mmc.c code would be a > good way to go. > > Could you however, present how would you like to fit it to the current > design? > -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
