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

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

[Steve Rae]

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

This has been tested on ARMv7.
This is a WIP -- and I would appreciate some comments/advice...
Thanks, Steve


Steve Rae (3):
  usb/gadget: fastboot: add sparse image definitions
  usb/gadget: fastboot: add eMMC support for flash command
  usb/gadget: fastboot: add support for flash command

 README                          |  10 +++
 common/Makefile                 |   5 ++
 common/fb_mmc.c                 | 144 ++++++++++++++++++++++++++++++++++++++++
 doc/README.android-fastboot     |   5 +-
 drivers/usb/gadget/f_fastboot.c |  31 +++++++++
 include/fb_mmc.h                |   8 +++
 include/sparse_format.h         |  58 ++++++++++++++++
 7 files changed, 259 insertions(+), 2 deletions(-)
 create mode 100644 common/fb_mmc.c
 create mode 100644 include/fb_mmc.h
 create mode 100644 include/sparse_format.h

-- 
1.8.5

[U-Boot] [RFC PATCH 1/3] usb/gadget: fastboot: add sparse image definitions

[Steve Rae]

- to prepare for the support of fastboot sparse images

Signed-off-by: Steve Rae <srae@broadcom.com>
---
This file is ASIS from:
  https://raw.githubusercontent.com/AOSB/android_system_core/master/libsparse/sparse_format.h
  (commit 28fa5bc347390480fe190294c6c385b6a9f0d68b)
except for the __UBOOT__ conditional include.

 include/sparse_format.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 include/sparse_format.h

diff --git a/include/sparse_format.h b/include/sparse_format.h
new file mode 100644
index 0000000..21fbd05
--- /dev/null
+++ b/include/sparse_format.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2010 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef _LIBSPARSE_SPARSE_FORMAT_H_
+#define _LIBSPARSE_SPARSE_FORMAT_H_
+#define __UBOOT__
+#ifndef __UBOOT__
+#include "sparse_defs.h"
+#endif
+
+typedef struct sparse_header {
+  __le32	magic;		/* 0xed26ff3a */
+  __le16	major_version;	/* (0x1) - reject images with higher major versions */
+  __le16	minor_version;	/* (0x0) - allow images with higer minor versions */
+  __le16	file_hdr_sz;	/* 28 bytes for first revision of the file format */
+  __le16	chunk_hdr_sz;	/* 12 bytes for first revision of the file format */
+  __le32	blk_sz;		/* block size in bytes, must be a multiple of 4 (4096) */
+  __le32	total_blks;	/* total blocks in the non-sparse output image */
+  __le32	total_chunks;	/* total chunks in the sparse input image */
+  __le32	image_checksum; /* CRC32 checksum of the original data, counting "don't care" */
+				/* as 0. Standard 802.3 polynomial, use a Public Domain */
+				/* table implementation */
+} sparse_header_t;
+
+#define SPARSE_HEADER_MAGIC	0xed26ff3a
+
+#define CHUNK_TYPE_RAW		0xCAC1
+#define CHUNK_TYPE_FILL		0xCAC2
+#define CHUNK_TYPE_DONT_CARE	0xCAC3
+#define CHUNK_TYPE_CRC32    0xCAC4
+
+typedef struct chunk_header {
+  __le16	chunk_type;	/* 0xCAC1 -> raw; 0xCAC2 -> fill; 0xCAC3 -> don't care */
+  __le16	reserved1;
+  __le32	chunk_sz;	/* in blocks in output image */
+  __le32	total_sz;	/* in bytes of chunk input file including chunk header and data */
+} chunk_header_t;
+
+/* Following a Raw or Fill or CRC32 chunk is data.
+ *  For a Raw chunk, it's the data in chunk_sz * blk_sz.
+ *  For a Fill chunk, it's 4 bytes of the fill data.
+ *  For a CRC32 chunk, it's 4 bytes of CRC32
+ */
+
+#endif
-- 
1.8.5

[U-Boot] [RFC PATCH 2/3] usb/gadget: fastboot: add eMMC support for flash command

[Steve Rae]

- add support for 'fastboot flash' command for eMMC devices

Signed-off-by: Steve Rae <srae@broadcom.com>
---
I suspect that the "sparse image" handling (ie. the "while (remaining_chunks)" loop)
has been implemented elsewhere -- I need help finding the original code to determine
any licensing issues....
Thanks, Steve

 common/Makefile  |   5 ++
 common/fb_mmc.c  | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fb_mmc.h |   8 ++++
 3 files changed, 157 insertions(+)
 create mode 100644 common/fb_mmc.c
 create mode 100644 include/fb_mmc.h

diff --git a/common/Makefile b/common/Makefile
index f6cd980..c825c2c 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -264,4 +264,9 @@ obj-$(CONFIG_FIT_SIGNATURE) += image-sig.o
 obj-y += memsize.o
 obj-y += stdio.o
 
+# This option is not just y/n - it can have a numeric value
+ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV
+obj-y += fb_mmc.o
+endif
+
 CFLAGS_env_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)
diff --git a/common/fb_mmc.c b/common/fb_mmc.c
new file mode 100644
index 0000000..de6a4c7
--- /dev/null
+++ b/common/fb_mmc.c
@@ -0,0 +1,144 @@
+/*
+ * Copyright TODO
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <fb_mmc.h>
+#include <mmc.h>
+#include <sparse_format.h>
+
+void fb_mmc_flash_write(const char *cmd, void *download_buffer,
+			unsigned int download_bytes, char *response)
+{
+	int ret;
+	block_dev_desc_t *mmc_dev;
+	disk_partition_t info;
+	lbaint_t blk;
+	lbaint_t blkcnt;
+	ulong blks;
+	sparse_header_t *s_header = (sparse_header_t *)download_buffer;
+	chunk_header_t *c_header;
+	void *buffer;
+	uint32_t blk_sz;
+	uint32_t remaining_chunks;
+	uint32_t bytes_written = 0;
+
+	mmc_dev = mmc_get_dev(CONFIG_FASTBOOT_FLASH_MMC_DEV);
+	if (!mmc_dev || mmc_dev->type == DEV_TYPE_UNKNOWN) {
+		printf("%s: invalid mmc device\n", __func__);
+		strcpy(response, "FAILinvalid mmc device");
+		return;
+	}
+
+	ret = get_partition_info_efi_by_name(mmc_dev, cmd, &info);
+	if (ret) {
+		printf("%s: cannot find partition: '%s'\n", __func__, cmd);
+		strcpy(response, "FAILcannot find partition");
+		return;
+	}
+
+	if ((le32_to_cpu(s_header->magic) == SPARSE_HEADER_MAGIC) &&
+	    (le16_to_cpu(s_header->major_version) == 1)) {
+		/* sparse image */
+
+		blk_sz = le32_to_cpu(s_header->blk_sz);
+
+		/* verify s_header->blk_sz is exact multiple of info.blksz */
+		if (blk_sz != (blk_sz & ~(info.blksz - 1))) {
+			printf("%s: Sparse image block size issue [%u]\n",
+			       __func__, blk_sz);
+			strcpy(response, "FAILsparse image block size issue");
+			return;
+		}
+
+		if ((le32_to_cpu(s_header->total_blks) * blk_sz) >
+		    (info.size * info.blksz)) {
+			printf("%s: Sparse image is too large for the partition\n",
+			       __func__);
+			strcpy(response, "FAILsparse image is too large");
+			return;
+		}
+
+		printf("Flashing Sparse Image\n");
+
+		blk = info.start;
+		remaining_chunks = le32_to_cpu(s_header->total_chunks);
+		c_header = (chunk_header_t *)(download_buffer +
+		    le16_to_cpu(s_header->file_hdr_sz));
+		while (remaining_chunks) {
+			switch (le16_to_cpu(c_header->chunk_type)) {
+			case CHUNK_TYPE_RAW:
+				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;
+				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;
+			}
+
+			blk += (le32_to_cpu(c_header->chunk_sz) * blk_sz) /
+			    info.blksz;
+			c_header = (chunk_header_t *)((void *)c_header +
+			    le32_to_cpu(c_header->total_sz));
+			remaining_chunks--;
+		}
+
+		printf("........ wrote %u bytes to '%s'\n", bytes_written, cmd);
+	} else {
+		/* raw image */
+
+		/* 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);
+	}
+	strcpy(response, "OKAY");
+}
diff --git a/include/fb_mmc.h b/include/fb_mmc.h
new file mode 100644
index 0000000..1ad1d13
--- /dev/null
+++ b/include/fb_mmc.h
@@ -0,0 +1,8 @@
+/*
+ * Copyright 2014 Broadcom Corporation.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+void fb_mmc_flash_write(const char *cmd, void *download_buffer,
+			unsigned int download_bytes, char *response);
-- 
1.8.5

[U-Boot] [RFC PATCH 3/3] usb/gadget: fastboot: add support for flash command

[Steve Rae]

- implement 'fastboot flash' for eMMC devices

Signed-off-by: Steve Rae <srae@broadcom.com>
---

 README                          | 10 ++++++++++
 doc/README.android-fastboot     |  5 +++--
 drivers/usb/gadget/f_fastboot.c | 31 +++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/README b/README
index 7129df8..73071fc 100644
--- a/README
+++ b/README
@@ -1600,6 +1600,16 @@ The following options need to be configured:
 		downloads. This buffer should be as large as possible for a
 		platform. Define this to the size available RAM for fastboot.
 
+		CONFIG_FASTBOOT_FLASH
+		The fastboot protocol includes a "flash" command for writing
+		the downloaded image to a non-volatile storage device. Define
+		this to enable the "fastboot flash" command.
+
+		CONFIG_FASTBOOT_FLASH_MMC_DEV
+		The fastboot "flash" command requires addition information
+		regarding the non-volatile storage device. Define this to
+		the eMMC device that fastboot should use to store the image.
+
 - Journaling Flash filesystem support:
 		CONFIG_JFFS2_NAND, CONFIG_JFFS2_NAND_OFF, CONFIG_JFFS2_NAND_SIZE,
 		CONFIG_JFFS2_NAND_DEV
diff --git a/doc/README.android-fastboot b/doc/README.android-fastboot
index f1d128c..f00fd41 100644
--- a/doc/README.android-fastboot
+++ b/doc/README.android-fastboot
@@ -6,8 +6,9 @@ Overview
 The protocol that is used over USB is described in
 README.android-fastboot-protocol in same directory.
 
-The current implementation does not yet support the flash and erase
-commands.
+The current implementation does not yet support the erase
+command, and there is minimal support for the flash command;
+it only supports eMMC devices.
 
 Client installation
 ===================
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index 9dd85b6..89c2d3e 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -19,6 +19,9 @@
 #include <linux/compiler.h>
 #include <version.h>
 #include <g_dnl.h>
+#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV
+#include <fb_mmc.h>
+#endif
 
 #define FASTBOOT_VERSION		"0.4"
 
@@ -466,6 +469,28 @@ static void cb_boot(struct usb_ep *ep, struct usb_request *req)
 	fastboot_tx_write_str("OKAY");
 }
 
+#ifdef CONFIG_FASTBOOT_FLASH
+static void cb_flash(struct usb_ep *ep, struct usb_request *req)
+{
+	char *cmd = req->buf;
+	char response[RESPONSE_LEN];
+
+	strsep(&cmd, ":");
+	if (!cmd) {
+		printf("%s: missing partition name\n", __func__);
+		fastboot_tx_write_str("FAILmissing partition name");
+		return;
+	}
+
+	strcpy(response, "FAILno flash device defined");
+#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV
+	fb_mmc_flash_write(cmd, (void *)CONFIG_USB_FASTBOOT_BUF_ADDR,
+			   download_bytes, response);
+#endif
+	fastboot_tx_write_str(response);
+}
+#endif
+
 struct cmd_dispatch_info {
 	char *cmd;
 	void (*cb)(struct usb_ep *ep, struct usb_request *req);
@@ -485,6 +510,12 @@ static const struct cmd_dispatch_info cmd_dispatch_info[] = {
 		.cmd = "boot",
 		.cb = cb_boot,
 	},
+#ifdef CONFIG_FASTBOOT_FLASH
+	{
+		.cmd = "flash",
+		.cb = cb_flash,
+	},
+#endif
 };
 
 static void rx_handler_command(struct usb_ep *ep, struct usb_request *req)
-- 
1.8.5

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

[Lukasz Majewski]

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. 

> This has been tested on ARMv7.
> This is a WIP -- and I would appreciate some comments/advice...
> Thanks, Steve
> 
> 
> Steve Rae (3):
>   usb/gadget: fastboot: add sparse image definitions
>   usb/gadget: fastboot: add eMMC support for flash command
>   usb/gadget: fastboot: add support for flash command
> 
>  README                          |  10 +++
>  common/Makefile                 |   5 ++
>  common/fb_mmc.c                 | 144
> ++++++++++++++++++++++++++++++++++++++++
> doc/README.android-fastboot     |   5 +-
> drivers/usb/gadget/f_fastboot.c |  31 +++++++++
> include/fb_mmc.h                |   8 +++
> include/sparse_format.h         |  58 ++++++++++++++++ 7 files
> changed, 259 insertions(+), 2 deletions(-) create mode 100644
> common/fb_mmc.c create mode 100644 include/fb_mmc.h
>  create mode 100644 include/sparse_format.h
> 



-- 
Best regards,

Lukasz Majewski

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

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

[Marek Vasut]

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] [RFC PATCH 0/3] Implement "fastboot flash" for eMMC

[Steve Rae]

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

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; 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 case of 
the "mmc_block_op()" be acceptable?
- a method which uses "get_partition_info_efi_by_name()"
	-- no ideas yet...

If the consensus is to use this DFU backend, then I will continue is 
this direction.

Please advise,
Thanks, Steve

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

[Lukasz Majewski]

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

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

[Steve Rae]

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

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

[Rob Herring]

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?

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

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

[Pantelis Antoniou]

Hi Rob,

On Jun 25, 2014, at 4:59 PM, 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?
> 
> 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 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
>>> 
>>> 
>>> 
>> 

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

[Steve Rae]

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

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

[Rob Herring]

On Wed, Jun 25, 2014 at 7:16 PM, Steve Rae <srae@broadcom.com> wrote:
> 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 ????)

Yes, I believe it is dfu_write.

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

There are commands for SATA, SCSI (also SATA), eMMC, IDE, etc. They
are all pretty much the same set of sub-commands and duplicate the
same functionality. Those could all be combined to a single
implementation and/or command for block devices. That part is not DFU
related, but this problem then proliferates to other areas as it has
for DFU. The file drivers/dfu/dfu_mmc.c is mostly generic, but has
some eMMC dependencies with find_mmc_device and mmc_switch_part. So
read and write are already pretty much generic, but there's still some
work to do around device addressing/selection.

Rob

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

[Steve Rae]

On 14-06-26 06:20 AM, Rob Herring wrote:
> On Wed, Jun 25, 2014 at 7:16 PM, Steve Rae <srae@broadcom.com> wrote:
>> 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 ????)
>
> Yes, I believe it is dfu_write.
>
>>> 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...
>
> There are commands for SATA, SCSI (also SATA), eMMC, IDE, etc. They
> are all pretty much the same set of sub-commands and duplicate the
> same functionality. Those could all be combined to a single
> implementation and/or command for block devices. That part is not DFU
> related, but this problem then proliferates to other areas as it has
> for DFU. The file drivers/dfu/dfu_mmc.c is mostly generic, but has
> some eMMC dependencies with find_mmc_device and mmc_switch_part. So
> read and write are already pretty much generic, but there's still some
> work to do around device addressing/selection.
>
> Rob
>
While I agree in general that to make everything generic is ideal, IMO, 
I don't think that there is a design or a roadmap to get us there yet
I would suggest that any generic interface would also need to support:
- handling of multiple HW partitions (0=USER 1-BOOT1 2=BOOT2 etc.)
    >> which I already attempted to implement (and abandoned):
    http://lists.denx.de/pipermail/u-boot/2014-May/180468.html
- handling of partition names
    >> for EFI Partitions, this did get accepted:
    http://lists.denx.de/pipermail/u-boot/2014-May/180292.html
So now I would propose two phases:
(1) short term - get "fastboot flash" working (and "erase", and "oem 
format", etc.)
    >> I have code that works for eMMC device (and potentially for NAMD...)
(2) longer term - define the "generic block device" (probably enhance
"block_dev_desc_t" ?!?!?) and move the "short term solution" into this 
new design.

I will submit a "v2" to see if it will get accepted as part of the 
"short term solution".

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

[Lukasz Majewski]

Hi Rob,

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

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

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

[Lukasz Majewski]

Hi Rob,

> On Wed, Jun 25, 2014 at 7:16 PM, Steve Rae <srae@broadcom.com> wrote:
> > 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 ????)
> 
> Yes, I believe it is dfu_write.

dfu_write should be used.

> 
> >> 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...
> 
> There are commands for SATA, SCSI (also SATA), eMMC, IDE, etc. They
> are all pretty much the same set of sub-commands and duplicate the
> same functionality. Those could all be combined to a single
> implementation and/or command for block devices. That part is not DFU
> related, but this problem then proliferates to other areas as it has
> for DFU. The file drivers/dfu/dfu_mmc.c is mostly generic, but has
> some eMMC dependencies with find_mmc_device and mmc_switch_part. 

dfu_mmc.c is intended to handle eMMC/SD card writing. Please note that
there are other files - dfu_nand.c and dfu_raw.c, which are responsible
for accessing and storing data to other medium.

The "generic" file here is the dfu.c which tries to combine all
available media.

> So
> read and write are already pretty much generic, but there's still some
> work to do around device addressing/selection.
> 
> Rob


-- 
Best regards,

Lukasz Majewski

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

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

[Lukasz Majewski]

Hi Steve,

> 
> 
> On 14-06-26 06:20 AM, Rob Herring wrote:
> > On Wed, Jun 25, 2014 at 7:16 PM, Steve Rae <srae@broadcom.com>
> > wrote:
> >> 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 ????)
> >
> > Yes, I believe it is dfu_write.
> >
> >>> 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...
> >
> > There are commands for SATA, SCSI (also SATA), eMMC, IDE, etc. They
> > are all pretty much the same set of sub-commands and duplicate the
> > same functionality. Those could all be combined to a single
> > implementation and/or command for block devices. That part is not
> > DFU related, but this problem then proliferates to other areas as
> > it has for DFU. The file drivers/dfu/dfu_mmc.c is mostly generic,
> > but has some eMMC dependencies with find_mmc_device and
> > mmc_switch_part. So read and write are already pretty much generic,
> > but there's still some work to do around device
> > addressing/selection.
> >
> > Rob
> >
> While I agree in general that to make everything generic is ideal,
> IMO, I don't think that there is a design or a roadmap to get us
> there yet I would suggest that any generic interface would also need
> to support:
> - handling of multiple HW partitions (0=USER 1-BOOT1 2=BOOT2 etc.)
>     >> which I already attempted to implement (and abandoned):
>     http://lists.denx.de/pipermail/u-boot/2014-May/180468.html

As fair as I remember there are available methods to switch HW
partitions (like mmc_access_part()).

> - handling of partition names
>     >> for EFI Partitions, this did get accepted:
>     http://lists.denx.de/pipermail/u-boot/2014-May/180292.html
> So now I would propose two phases:
> (1) short term - get "fastboot flash" working (and "erase", and "oem 
> format", etc.)

Would the short term solution allow writing fastboot only to eMMC or
other media are going to be supported?

>     >> I have code that works for eMMC device (and potentially for
>     >> NAMD...)
> (2) longer term - define the "generic block device" (probably enhance
> "block_dev_desc_t" ?!?!?) and move the "short term solution" into
> this new design.
> 
> I will submit a "v2" to see if it will get accepted as part of the 
> "short term solution".

I will do my best to review your patches.

-- 
Best regards,

Lukasz Majewski

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