Hi Rob, On Jun 25, 2014, at 4:59 PM, Rob Herring wrote:
> 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. 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. > I completely agree. Started looking into this but there's lots of inertia :( We have device specific backends where a generic one should suffice... > Don't you also need the ability to partition a disk with fastboot? > > Rob > Regards -- Pantelis >> >> 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 >>> >>> >>> >> _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot