[U-Boot] [RFC PATCH 0/3] Implement "fastboot flash" for eMMC

[Steve Rae <srae@broadcom.com>]

Rob,

On 14-06-25 06:59 AM, Rob Herring wrote:
> On Mon, Jun 23, 2014 at 1:37 PM, Steve Rae <srae@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?

The concern is not performance related -- just the amount of (overhead) 
code required to implement the "DFU backend" versus calling 
mmc_dev->block_write()
    (maybe someone can tell me where to interface into DFU: is it at 
"dfu_write() or ????)

>
> 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.

As I understand it, the "block_write" callback function is in the 
"block_dev_desc_t". Isn't this the part of the "generic block device" 
interface? Please explain...

>
> Don't you also need the ability to partition a disk with fastboot?

yes: though "fastboot oem format" is outside of this RFC -- because I 
wanted to minimize this request to ensure that licensing wasn't going to 
kill it.


>
> 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 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
>>>
>>>
>>>
>>