Hi Rob, > On Mon, Jun 23, 2014 at 1:37 PM, Steve Rae <s...@broadcom.com> wrote: > > Rob & Sebastian > > > > I would appreciate your comments on this issue; I suspect that you > > had some ideas regarding the implementation of the fastboot "flash" > > and "erase" commands.... > > I agree with Lukasz's and Marek's comments unless there are good > reasons not to use it which can't be fixed. Curiously, USB mass > storage does not use the DFU backend, but I don't know why.
USB mass storage is from its very beginning tied with eMMC/SD card. DFU/thor are a different in a way, that they allow storing data to other media like raw memory, eMMC, NAND, etc. The dfu backend tries to handle writing to many media and also file systems. > Perhaps > there are incompatibilities or converting it is on the todo list. Are > your performance concerns measurable or it's just the fact you are > adding another layer? > > I'd really like to see the eMMC backend be a generic block device > backend. There's no good reason for it to be eMMC/SD specific. > > Don't you also need the ability to partition a disk with fastboot? > > Rob > > > > > Thanks in advance, Steve > > > > > > On 14-06-23 05:58 AM, Lukasz Majewski wrote: > >> > >> Hi Steve, > >> > >>> > >>> > >>> On 14-06-19 11:32 PM, Marek Vasut wrote: > >>>> > >>>> On Friday, June 20, 2014 at 08:18:42 AM, Lukasz Majewski wrote: > >>>>> > >>>>> Hi Steve, > >>>>> > >>>>>> This series implements the "fastboot flash" command for eMMC > >>>>>> devices. It supports both raw and sparse images. > >>>>>> > >>>>>> NOTES: > >>>>>> - the support for the "fastboot flash" command is enabled with > >>>>>> CONFIG_FASTBOOT_FLASH > >>>>>> - the support for eMMC is enabled with > >>>>>> CONFIG_FASTBOOT_FLASH_MMC_DEV > >>>>>> - (future) the support for NAND would be enabled with > >>>>>> CONFIG_FASTBOOT_FLASH_NAND(???) > >>>>>> - thus the proposal is to place the code in common/fb_mmc.c and > >>>>>> (future) common/fb_nand.c(???), however, this may not be the > >>>>>> appropriate location.... > >>>>> > >>>>> > >>>>> Would you consider another approach for providing flashing > >>>>> backend for fastboot? > >>>>> > >>>>> I'd like to propose reusing of the dfu flashing code for this > >>>>> purpose. Such approach has been used successfully with USB > >>>>> "thor" downloading function. > >>>>> > >>>>> Since the "fastboot" is using gadget infrastructure (thanks to > >>>>> the effort of Rob Herring) I think that it would be feasible to > >>>>> reuse the same approach as "thor" does. In this way the low > >>>>> level code would be kept in one place and we could refine and > >>>>> test it more thoroughly. > >>>> > >>>> > >>>> I'm all for this approach as well if possible. > >>>> > >>>> Best regards, > >>>> Marek Vasut > >>>> _______________________________________________ > >>>> U-Boot mailing list > >>>> U-Boot@lists.denx.de > >>>> http://lists.denx.de/mailman/listinfo/u-boot > >>>> > >>> > >>> I have briefly investigated this suggestion.... > >>> And have 'hacked' some code as follows: > >>> > >>> --- common/fb_mmc.c_000 2014-06-20 14:13:43.271158073 -0700 > >>> +++ common/fb_mmc.c_001 2014-06-20 14:17:48.688072764 -0700 > >>> while (remaining_chunks) { > >>> switch > >>> (le16_to_cpu(c_header->chunk_type)) { case CHUNK_TYPE_RAW: > >>> +#if 0 > >>> blkcnt = > >>> (le32_to_cpu(c_header->chunk_sz) > >>> * blk_sz) / info.blksz; > >>> buffer = > >>> (void *)c_header + > >>> le16_to_cpu(s_header->chunk_hdr_sz); > >>> > >>> blks = > >>> mmc_dev->block_write(mmc_dev->dev, blk, blkcnt, buffer); > >>> if (blks != blkcnt) { > >>> printf("Write failed > >>> %lu\n", blks); strcpy(response, > >>> "FAILmmc write > >>> failure"); return; > >>> } > >>> > >>> bytes_written += blkcnt * > >>> info.blksz; +#else > >>> + buffer = > >>> + (void *)c_header + > >>> + > >>> le16_to_cpu(s_header->chunk_hdr_sz); + > >>> + len = > >>> le32_to_cpu(c_header->chunk_sz) * blk_sz; > >>> + ret_dfu = > >>> dfu_write_medium_mmc(dfu, offset, > >>> + > >>> buffer, &len); > >>> + if (ret_dfu) { > >>> + printf("Write failed > >>> %lu\n", len); > >>> + strcpy(response, > >>> + "FAILmmc write > >>> failure"); > >>> + return; > >>> + } > >>> + > >>> + > >>> + bytes_written += len; > >>> +#endif > >>> break; > >>> > >>> case CHUNK_TYPE_FILL: > >>> case CHUNK_TYPE_DONT_CARE: > >>> case CHUNK_TYPE_CRC32: > >>> /* do nothing */ > >>> break; > >>> > >>> default: > >>> /* error */ > >>> printf("Unknown chunk type\n"); > >>> strcpy(response, > >>> "FAILunknown chunk type in > >>> sparse image"); return; > >>> } > >>> > >>> +#if 0 > >>> blk += (le32_to_cpu(c_header->chunk_sz) * > >>> blk_sz) / info.blksz; > >>> +#else > >>> + offset += le32_to_cpu(c_header->chunk_sz) > >>> * blk_sz; +#endif > >>> c_header = (chunk_header_t *)((void > >>> *)c_header + le32_to_cpu(c_header->total_sz)); > >>> remaining_chunks--; > >>> } > >>> > >>> > >>> --- common/fb_mmc.c_000 2014-06-20 14:13:43.271158073 -0700 > >>> +++ common/fb_mmc.c_001 2014-06-20 14:17:48.688072764 -0700 > >>> /* raw image */ > >>> > >>> +#if 0 > >>> /* determine number of blocks to write */ > >>> blkcnt = > >>> ((download_bytes + (info.blksz - 1)) & > >>> ~(info.blksz - 1)); blkcnt = blkcnt / info.blksz; > >>> > >>> if (blkcnt > info.size) { > >>> printf("%s: too large for partition: > >>> '%s'\n", __func__, cmd); > >>> strcpy(response, "FAILtoo large for > >>> partition"); return; > >>> } > >>> > >>> printf("Flashing Raw Image\n"); > >>> > >>> blks = mmc_dev->block_write(mmc_dev->dev, > >>> info.start, blkcnt, download_buffer); > >>> if (blks != blkcnt) { > >>> printf("%s: failed writing to mmc device > >>> %d\n", __func__, mmc_dev->dev); > >>> strcpy(response, "FAILfailed writing to > >>> mmc device"); return; > >>> } > >>> > >>> printf("........ wrote %lu bytes to '%s'\n", > >>> blkcnt * info.blksz, cmd); > >>> +#else > >>> + printf("Flashing Raw Image\n"); > >>> + > >>> + ret_dfu = dfu_write_medium_mmc(dfu, offset, > >>> download_buffer, &len); > >>> + if (ret_dfu) { > >>> + printf("%s: failed writing to mmc device > >>> %d\n", > >>> + __func__, mmc_dev->dev); > >>> + strcpy(response, "FAILfailed writing to > >>> mmc device"); > >>> + return; > >>> + } > >>> + > >>> + printf("........ wrote %lu bytes to '%s'\n", len, > >>> cmd); +#endif > >>> } > >>> > >>> NOTE: > >>> - I know that I cannot call "dfu_write_medium_mmc()" directly -- > >>> but I just wanted to test this functionality > >> > >> > >> Indeed, it looks like an early proof-of-concept code :-). > >> > >>> > >>> My initial reaction is that using the DFU backend to effectively > >>> call the mmc block_write() function seems to cause an unnecessary > >>> amount of overhead; > >> > >> > >> It also allows to access/write data to other media - like NAND > >> memory. > >> > >>> and the only thing that it really provides is a proven > >>> method of calculating the "number of blocks to write"... > >>> > >>> I would be more interested in this backend if it would provide: > >>> - handling of the "sparse image format" > >>> -- would a CONFIG option to include this in the > >>> DFU_OP_WRITE > >> > >> > >> You are welcome to prepare patch which adds such functionality. > >> Moreover, in the u-boot-dfu repository (master branch) you can find > >> initial version of the regression tests for DFU. > >> Extending the current one, or adding your own would be awesome :-) > >> > >> > >>> case of the "mmc_block_op()" be acceptable? > >>> - a method which uses "get_partition_info_efi_by_name()" > >>> -- no ideas yet... > >>> > >> > >> I'm looking forward for RFC. > >> > >>> If the consensus is to use this DFU backend, then I will continue > >>> is this direction. > >> > >> > >> That would be great. > >> > >>> > >>> Please advise, > >>> Thanks, Steve > >> > >> > >> > > -- 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
