From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Mon, 23 Jun 2014 11:37:24 -0700 Subject: [U-Boot] [RFC PATCH 0/3] Implement "fastboot flash" for eMMC In-Reply-To: <20140623145858.5bef59fe@amdc2363> References: <1403211128-19631-1-git-send-email-srae@broadcom.com> <20140620081842.50471ae4@amdc2363> <201406200832.19841.marex@denx.de> <53A4ADE7.2080306@broadcom.com> <20140623145858.5bef59fe@amdc2363> Message-ID: <53A873E4.7040008@broadcom.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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.... 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 at 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 > >