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

[Lukasz Majewski <l.majewski@samsung.com>]

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


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group