All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] dfu: initial implementation
@ 2011-11-02 10:00 Andrzej Pietrasiewicz
  2011-11-02 14:29 ` Stefan Schmidt
                   ` (3 more replies)
  0 siblings, 4 replies; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-02 10:00 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Dear All,

This is Device Firmware Upgrade (DFU) implementation which supports data
upload and download function to devices which are equipped with a UDC.

Device Firmware Upgrade (DFU) is an extension to the USB specification.
As of the time of this writing it is documented at

http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf

The aim of DFU is to allow transferring data to/from selected areas of
interest within a compliant device. In the DFU nomenclature the host->device
direction is called download and the device->host direction is called
upload. The areas are defined by the compliant device. In the DFU
nomenclature the area of interest within the device is called an altsetting.
The device is connected to the host through USB. On the host side compliant
software must be used to initiate the data transfer. Example piece of such
software is dfu-util package from the OpenMoko project:

http://wiki.openmoko.org/wiki/Dfu-util.

Please refer to dfu-util documentation for details.
The below ASCII-art presents a connection between the host and the device.

      +-----------------+    <--- DFU --->    +-------------------------+
      |  e.g. dfu-util  |    <=== USB ===>    |          / altsetting 0 |
      |          host   +---------------------+   device - altsetting 1 |
      |   file /        |                     |          \     ...      |
      +-----------------+                     +-------------------------+

The DFU implementation on the device side remains in one of the two modes:
the Run-Time mode and the DFU mode. The USB descriptor sets exported by
the device depend on which mode is in force. While in DFU mode the device
exports the descriptors corresponding to each available altsetting. An
example dfu-util session when the device is in the DFU mode looks similar
to this:

# dfu-util -l
dfu-util - (C) 2007-2008 by OpenMoko Inc.
This program is Free Software and has ABSOLUTELY NO WARRANTY

Found DFU: [0x0525:0xffff] devnum=46, cfg=0, intf=0, alt=0, name="bootldr"
Found DFU: [0x0525:0xffff] devnum=46, cfg=0, intf=0, alt=1, name="kernel"

In the above example there are two altsettings, altsetting 0 with a
human-readable name "bootldr" and altsetting 1 with a human-readable name
"kernel"

To initiate data transfer the user at the host side must specify the
altsetting to/from which the data transfer is to be performed. In case of
the dfu-util it is specified with a "-a" option followed by either a number
or a human-readable name. The user also needs to specify the direction of
the data transfer and the file on the host from/to which data are to be
transfered. An example download command line looks similar to this:

# dfu-util -D vmlinux -a kernel

In the above command line the contents of the file vmlinux is to be
downloaded into the altsetting named kernel.

An example upload command line looks similar to this:

# dfu-util -U vmlinux -a kernel

In the above command line the contents of the altsetting named kernel is to
be uploaded into the file vmlinux.

This u-boot implementation introduces a parameter-less dfu command.
After the user finishes with dfu they can Ctrl-C to return to u-boot main
menu.

The implementation is split into two parts: the dfu gadget implementation
and a "flashing backend", the interface between the two being the
struct flash_entity. The "flashing backend"'s implementation is
board-specific and is implemented on the Goni target.

Patch summary:

Andrzej Pietrasiewicz (1):
  dfu: initial implementation

 board/samsung/goni/Makefile |    2 +
 board/samsung/goni/flash.c  |  634 +++++++++++++++++++++++++++++
 board/samsung/goni/flash.h  |   28 ++
 board/samsung/goni/goni.c   |   17 +
 common/Makefile             |    1 +
 common/cmd_dfu.c            |   50 +++
 drivers/usb/gadget/Makefile |    1 +
 drivers/usb/gadget/dfu.c    |  920 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/dfu.h    |  171 ++++++++
 include/configs/s5p_goni.h  |    6 +
 include/dfu.h               |   31 ++
 include/flash_entity.h      |   39 ++
 include/mbr.h               |   49 +++
 13 files changed, 1949 insertions(+), 0 deletions(-)
 create mode 100644 board/samsung/goni/flash.c
 create mode 100644 board/samsung/goni/flash.h
 create mode 100644 common/cmd_dfu.c
 create mode 100644 drivers/usb/gadget/dfu.c
 create mode 100644 drivers/usb/gadget/dfu.h
 create mode 100644 include/dfu.h
 create mode 100644 include/flash_entity.h
 create mode 100644 include/mbr.h

diff --git a/board/samsung/goni/Makefile b/board/samsung/goni/Makefile
index ecde7a7..c07b0a2 100644
--- a/board/samsung/goni/Makefile
+++ b/board/samsung/goni/Makefile
@@ -31,6 +31,8 @@ LIB	= $(obj)lib$(BOARD).o
 COBJS-y	:= goni.o onenand.o
 SOBJS	:= lowlevel_init.o
 
+COBJS-$(CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE) += flash.o
+
 SRCS    := $(SOBJS:.o=.S) $(COBJS-y:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS-y))
 SOBJS	:= $(addprefix $(obj),$(SOBJS))
diff --git a/board/samsung/goni/flash.c b/board/samsung/goni/flash.c
new file mode 100644
index 0000000..a6dd7d2
--- /dev/null
+++ b/board/samsung/goni/flash.c
@@ -0,0 +1,634 @@
+/*
+ * flash.c -- board flashing routines
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <mbr.h>
+#include <mmc.h>
+#include <fat.h>
+#include <flash_entity.h>
+#include <linux/types.h>
+#include <jffs2/load_kernel.h>
+
+#define MMC_BLOCK_SZ		(256 * 1024)
+#define MMC_FAT_BLOCK_SZ	(4 * 1024 * 1024)
+#define MMC_SECTOR_SZ		512
+#define MMC_UBOOT_OFFSET	128
+#define MMC_UBOOT_SZ		512
+#define PARAM_LEN		12
+#define SHORT_PART_NAME		15
+#define LONG_PART_NAME		20
+#define SIGNATURE		((unsigned short) 0xAA55)
+#define CALLOC_STRUCT(n, type)	(struct type *) calloc(n, sizeof(struct type))
+#define DEFAULT_MMC_PART_NAME	"mmc-default-part"
+
+/* partition IDs counted from 0, eg. mmc0-pri-1's ID is 0 */
+#define UIMAGE_PART_ID		1
+#define EXTENDED_PART_ID	3
+#define UMS_PART_ID		7
+#define UIMAGE_PART_NAME	"mmc0-pri-2"
+
+#define USE_MMC_UBOOT
+#define USE_MMC
+
+typedef int (*rw_op)(void *buf, unsigned int len, unsigned long from);
+typedef int (*erase_op)(unsigned int len, unsigned long from);
+
+struct flash_entity_ctx {
+	unsigned long		offset;	/* offset into the device */
+	unsigned long		length; /* size of the entity */
+	u8			*buf; /* buffer for one chunk */
+	unsigned long		buf_len; /* one chunk length */
+	unsigned int		buffered; /* # available bytes in buf */
+	unsigned int		num_done; /* total bytes handled */
+	rw_op			read; /* chunk read op for this medium */
+	rw_op			write; /* chunk write op for this medium */
+	erase_op		erase; /* erase op for this medium or NULL */
+	struct flash_entity	*this_entity; /* the containing entity */
+	void			*associated; /* related entity, if any */
+};
+
+struct mbr_part_data {
+	unsigned long		offset; /* #sectors from mmc begin */
+	unsigned long		length; /* #sectors in this partition*/
+	u8			primary; /* != 0 if primary, 0 if extended */
+};
+
+/*
+ * MMC u-boot partitions
+ */
+static struct mbr_part_data *uboot_pdata;
+
+static u8 uboot_part_num;
+static u8 used_uboot_parts;
+
+int use_uboot(struct mbr_part_data *pdata, u8 i)
+{
+	/*
+	 * Use i and pdata[i] members to decide if the partition is used
+	 */
+	return 1;
+}
+
+char *alloc_uboot_name(u8 i)
+{
+	char *name = calloc(SHORT_PART_NAME, 1);
+
+	if (name) {
+		sprintf(name, "mmc-u-boot");
+		return name;
+	}
+
+	return DEFAULT_MMC_PART_NAME;
+}
+
+/*
+ * MMC partitions and MMC operations
+ */
+struct mmc *mmc;
+
+static struct mbr_part_data *mmc_pdata;
+
+static u8 mmc_part_num;
+static u8 used_mmc_parts;
+
+static u8 mmc_buf[MMC_BLOCK_SZ];
+
+static int extended_lba;
+
+static int mmc_mbr_dev;
+
+static u8 pri_count;
+static u8 ext_count = 4;
+
+/*
+ * Define files available in the UIMAGE partition which has FAT on it.
+ * Only flat structure without subdirectories is supported.
+ */
+static char *uImage_part_files[] = {
+	"uImage",
+};
+#define UIMAGE_PART_NUM_FILES ARRAY_SIZE(uImage_part_files)
+
+static int read_ebr(struct mmc *mmc, struct mbr_partition *mp,
+		int ebr_next, struct mbr_part_data *pd, int parts)
+{
+	struct mbr *ebr;
+	struct mbr_partition *p;
+	char buf[512];
+	int ret, i;
+	int lba = 0;
+
+	if (ebr_next)
+		lba = extended_lba;
+
+	lba += mp->lba;
+	ret = mmc->block_dev.block_read(mmc_mbr_dev, lba, 1, buf);
+	if (ret != 1)
+		return 0;
+
+	ebr = (struct mbr *) buf;
+
+	if (ebr->signature != SIGNATURE) {
+		printf("Signature error 0x%x\n", ebr->signature);
+		return 0;
+	}
+
+	for (i = 0; i < 2; i++) {
+		p = (struct mbr_partition *) &ebr->parts[i];
+
+		if (i == 0) {
+			lba += p->lba;
+			if (p->partition_type == 0x83) {
+				if (pd) {
+					pd[parts].offset = lba;
+					pd[parts].length = p->nsectors;
+					pd[parts].primary = 0;
+				}
+				parts++;
+			}
+		}
+	}
+
+	if (p->lba && p->partition_type == 0x5)
+		parts = read_ebr(mmc, p, 1, pd, parts);
+
+	return parts;
+}
+
+static int read_mbr(struct mmc *mmc, struct mbr_part_data *pd)
+{
+	struct mbr_partition *mp;
+	struct mbr *mbr;
+	char buf[512];
+	int ret, i;
+	int parts = 0;
+
+	ret = mmc->block_dev.block_read(mmc_mbr_dev, 0, 1, buf);
+	if (ret != 1)
+		return 0;
+
+	mbr = (struct mbr *) buf;
+
+	if (mbr->signature != SIGNATURE) {
+		printf("Signature error 0x%x\n", mbr->signature);
+		return 0;
+	}
+
+	for (i = 0; i < 4; i++) {
+		mp = (struct mbr_partition *) &mbr->parts[i];
+
+		if (!mp->partition_type)
+			continue;
+
+		if (mp->partition_type == 0x83) {
+			if (pd) {
+				pd[parts].offset = mp->lba;
+				pd[parts].length = mp->nsectors;
+				pd[parts].primary = 1;
+			}
+			parts++;
+		}
+
+		if (mp->lba && mp->partition_type == 0x5) {
+			extended_lba = mp->lba;
+			parts = read_ebr(mmc, mp, 0, pd, parts);
+		}
+	}
+
+	return parts;
+}
+
+static int rw_mmc(void *buf, char *op, unsigned int len, unsigned long from)
+{
+	char ram_addr[PARAM_LEN];
+	char offset[PARAM_LEN];
+	char length[PARAM_LEN];
+	char *argv[] = {"mmc", op, ram_addr, offset, length};
+
+	sprintf(ram_addr, "0x%lx", (unsigned long)buf);
+	sprintf(offset, "0x%lx", from / MMC_SECTOR_SZ); /* guaranteed integer */
+	sprintf(length, "0x%x", (len + MMC_SECTOR_SZ - 1) / MMC_SECTOR_SZ);
+
+	return do_mmcops(NULL, 0, 6, argv);
+}
+
+static inline int read_mmc(void *buf, unsigned int len, unsigned long from)
+{
+	return rw_mmc(buf, "read", len, from);
+}
+
+static inline int write_mmc(void *buf, unsigned int len, unsigned long from)
+{
+	return rw_mmc(buf, "write", len, from);
+}
+
+/*
+ * Return number of flash entities per this partition
+ */
+u8 use_mmc(struct mbr_part_data *pdata, u8 i)
+{
+	/*
+	 * Use i and pdata[i] members to decide if the partition is used
+	 */
+	if (i == UIMAGE_PART_ID)
+		return UIMAGE_PART_NUM_FILES;
+	if (i == EXTENDED_PART_ID)
+		return 0; /* do not expose the extended partition as a whole */
+	if (i == UMS_PART_ID)
+		return 0; /* do not expose UMS; there is a separate command */
+	return 1;
+}
+
+char *alloc_mmc_name(struct mbr_part_data *pdata, u8 i, u8 l)
+{
+	char *name = calloc(PARAM_LEN, 1);
+
+	if (name) {
+		sprintf(name, "mmc0-");
+		if (pdata[i].primary)
+			sprintf(name + strlen(name), "pri-%d",
+				l ? pri_count : ++pri_count);
+		else
+			sprintf(name + strlen(name), "ext-%d",
+				l ? ext_count : ++ext_count);
+
+		return name;
+	}
+
+	return DEFAULT_MMC_PART_NAME;
+}
+
+/*
+ * FAT operations
+ */
+static u8 fat_buf[MMC_FAT_BLOCK_SZ];
+
+static char *fat_filename;
+static int fat_part_num;
+
+static int read_fat(void *buf, unsigned int len, unsigned long from)
+{
+	int ret;
+
+	ret = fat_register_device(&mmc->block_dev, fat_part_num);
+	if (ret < 0) {
+		printf("error : fat_register_device\n");
+		return 0;
+	}
+	printf("read up to %d B ", MMC_FAT_BLOCK_SZ);
+	return file_fat_read(fat_filename, buf, len);
+}
+
+static int write_fat(void *buf, unsigned int len, unsigned long from)
+{
+#ifdef CONFIG_FAT_WRITE
+	int ret;
+
+	ret = fat_register_device(&mmc->block_dev, fat_part_num);
+	if (ret < 0) {
+		printf("error : fat_register_divce\n");
+		return 0;
+	}
+
+	printf("write up to %d B ", MMC_FAT_BLOCK_SZ);
+	ret = file_fat_write(fat_filename, buf, len);
+
+	/* format and write again */
+	if (ret == 1) {
+		printf("formatting\n");
+		if (mkfs_vfat(&mmc->block_dev, fat_part_num)) {
+			printf("error formatting device\n");
+			return 0;
+		}
+		ret = file_fat_write(fat_filename, buf, len);
+	}
+
+	if (ret < 0) {
+		printf("error : writing %s\n", fat_filename);
+		return 0;
+	}
+#else
+	printf("error : FAT write not supported\n");
+	return 0;
+#endif
+	return len;
+}
+
+/*
+ * Entity-specific prepare and finish
+ */
+static void reset_ctx(struct flash_entity_ctx *ctx)
+{
+	ctx->buffered = 0;
+	ctx->num_done = 0;
+}
+
+static int generic_prepare(void *ctx, u8 mode)
+{
+	struct flash_entity_ctx *ct = ctx;
+
+	reset_ctx(ct);
+	memset(ct->buf, 0, ct->buf_len);
+	if (mode == FLASH_WRITE) {
+		if (ct->erase) {
+			printf("Erase entity: %s ", ct->this_entity->name);
+			ct->erase(ct->length, ct->offset);
+		}
+		printf("Write entity: %s ", ct->this_entity->name);
+	} else if (mode == FLASH_READ) {
+		printf("Read entity: %s ", ct->this_entity->name);
+	}
+	return 0;
+}
+
+static int generic_finish(void *ctx, u8 mode)
+{
+	struct flash_entity_ctx *ct = ctx;
+
+	if (mode == FLASH_WRITE && ct->buffered > 0)
+		ct->write(ct->buf, ct->buffered, ct->offset + ct->num_done);
+
+	return 0;
+}
+
+static int prepare_fat(void *ctx, u8 mode)
+{
+	struct flash_entity_ctx *ct = ctx;
+
+	fat_filename = ct->associated;
+	if (!strncmp(ct->this_entity->name, UIMAGE_PART_NAME,
+		     strlen(UIMAGE_PART_NAME)))
+		fat_part_num = UIMAGE_PART_ID + 1;
+	return generic_prepare(ctx, mode);
+}
+
+/*
+ * Transport layer to storage adaptation
+ */
+
+/*
+ * Adapt transport layer buffer size to storage chunk size
+ *
+ * return < n to indicate no more data to read
+ */
+static int read_block(void *ctx, unsigned int n, void *buf)
+{
+	struct flash_entity_ctx *ct = ctx;
+	unsigned int nread = 0;
+
+	if (n == 0)
+		return n;
+
+	while (nread < n) {
+		unsigned int copy;
+
+		if (ct->num_done >= ct->length)
+			break;
+		if (ct->buffered == 0) {
+			ct->read(ct->buf, ct->buf_len,
+				 ct->offset + ct->num_done);
+			ct->buffered = ct->buf_len;
+		}
+		copy = min(n - nread, ct->buffered);
+
+		memcpy(buf + nread, ct->buf + ct->buf_len - ct->buffered, copy);
+		nread += copy;
+		ct->buffered -= copy;
+		ct->num_done += copy;
+	}
+
+	return nread;
+}
+
+/*
+ * Adapt transport layer buffer size to storage chunk size
+ */
+static int write_block(void *ctx, unsigned int n, void *buf)
+{
+	struct flash_entity_ctx *ct = ctx;
+	unsigned int nwritten = 0;
+
+	if (n == 0)
+		return n;
+
+	while (nwritten < n) {
+		unsigned int copy;
+
+		if (ct->num_done >= ct->length)
+			break;
+		if (ct->buffered >= ct->buf_len) {
+			ct->write(ct->buf, ct->buf_len,
+				  ct->offset + ct->num_done);
+			ct->buffered = 0;
+			ct->num_done += ct->buf_len;
+			if (ct->num_done >= ct->length)
+				break;
+		}
+		copy = min(n - nwritten, ct->buf_len - ct->buffered);
+
+		memcpy(ct->buf + ct->buffered, buf + nwritten, copy);
+		nwritten += copy;
+		ct->buffered += copy;
+	}
+
+	return nwritten;
+}
+
+/*
+ * Flash entities definitions for this board
+ */
+static struct flash_entity *flash_ents;
+
+void customize_entities(struct flash_entity *fe, u8 n_ents)
+{
+	int i;
+	for (i = 0; i < n_ents; ++i) {
+		/* i counts all entities, not just mmc entities */
+		/* add similar "if" blocks for other customizable entities */
+		if (!strcmp(fe[i].name, UIMAGE_PART_NAME)) {
+			struct flash_entity_ctx *ctx;
+			char *name, file_number;
+
+			fe[i].prepare = prepare_fat;
+			ctx = fe[i].ctx;
+			ctx->length = min(ctx->length, MMC_FAT_BLOCK_SZ);
+			ctx->buf = fat_buf;
+			ctx->buf_len = ctx->length;
+			ctx->read = read_fat;
+			ctx->write = write_fat;
+			file_number = (char)ctx->associated;
+			/* by design file_number cannot exceed array size */
+			ctx->associated = uImage_part_files[file_number];
+
+			name = calloc(LONG_PART_NAME, 1);
+			if (name) {
+				sprintf(name, "%s-%s", fe[i].name,
+					(char *)ctx->associated);
+				free(fe[i].name);
+				fe[i].name = name;
+			}
+
+			continue;
+		}
+	}
+}
+
+static inline void generic_flash_entity(struct flash_entity *fe)
+{
+	fe->prepare = generic_prepare;
+	fe->finish = generic_finish;
+	fe->read_block = read_block;
+	fe->write_block = write_block;
+}
+
+static inline void generic_flash_entity_ctx(struct flash_entity_ctx *ctx,
+					    struct flash_entity *fe)
+{
+	ctx->buf = mmc_buf;
+	ctx->buf_len = MMC_BLOCK_SZ;
+	ctx->read = read_mmc;
+	ctx->write = write_mmc;
+	ctx->erase = NULL;
+	ctx->this_entity = fe;
+	fe->ctx = ctx;
+}
+
+void register_flash_areas(void)
+{
+	u8 i, j;
+
+#ifdef USE_MMC
+	mmc = find_mmc_device(mmc_mbr_dev);
+	if (mmc && !mmc_init(mmc)) {
+		mmc_part_num = read_mbr(mmc, NULL);
+		if (mmc_part_num) {
+			mmc_pdata = CALLOC_STRUCT(mmc_part_num,
+						  mbr_part_data);
+			if (mmc_pdata)
+				if (!read_mbr(mmc, mmc_pdata)) {
+					free(mmc_pdata);
+					mmc_pdata = NULL;
+				}
+		}
+	}
+	used_mmc_parts = 0;
+	for (i = 0; mmc_pdata && i < mmc_part_num; ++i)
+		used_mmc_parts += use_mmc(mmc_pdata, i);
+	pri_count = 0;
+	ext_count = 4;
+#endif
+
+#ifdef USE_MMC_UBOOT
+	if (mmc) {
+		uboot_part_num = 1;
+		if (uboot_part_num) {
+			uboot_pdata = CALLOC_STRUCT(uboot_part_num,
+						    mbr_part_data);
+			if (uboot_pdata)
+				for (i = 0; i < uboot_part_num; ++i) {
+					uboot_pdata[i].offset =
+						MMC_UBOOT_OFFSET;
+					uboot_pdata[i].length =
+						MMC_UBOOT_SZ;
+					uboot_pdata[i].primary = 0;
+				}
+		}
+	}
+	used_uboot_parts = 0;
+	for (i = 0; uboot_pdata && i < uboot_part_num; ++i)
+		used_uboot_parts += use_uboot(uboot_pdata, i);
+#endif
+
+	flash_ents = CALLOC_STRUCT(used_uboot_parts + used_mmc_parts,
+				   flash_entity);
+	if (!flash_ents)
+		goto partinfo_alloc_rollback;
+
+	j = 0;
+	for (i = 0; i < uboot_part_num; ++i)
+		if (use_uboot(uboot_pdata, i)) {
+			struct flash_entity *fe;
+			struct flash_entity_ctx *ctx;
+
+			fe = &flash_ents[j++];
+			fe->name = alloc_uboot_name(i);
+			generic_flash_entity(fe);
+
+			ctx = CALLOC_STRUCT(1, flash_entity_ctx);
+			if (!ctx)
+				goto flash_ents_alloc_rollback;
+			generic_flash_entity_ctx(ctx, fe);
+			ctx->offset = uboot_pdata[i].offset * MMC_SECTOR_SZ;
+			ctx->length = uboot_pdata[i].length * MMC_SECTOR_SZ;
+		}
+	uboot_part_num = used_uboot_parts;
+
+	for (i = 0; i < mmc_part_num; ++i) {
+		u8 k = use_mmc(mmc_pdata, i);
+		u8 l;
+		for (l = 0; l < k; ++l) {
+			struct flash_entity *fe;
+			struct flash_entity_ctx *ctx;
+
+			fe = &flash_ents[j++];
+			fe->name = alloc_mmc_name(mmc_pdata, i, l);
+			generic_flash_entity(fe);
+
+			ctx = CALLOC_STRUCT(1, flash_entity_ctx);
+			if (!ctx)
+				goto flash_ents_alloc_rollback;
+			generic_flash_entity_ctx(ctx, fe);
+			ctx->offset = mmc_pdata[i].offset * MMC_SECTOR_SZ;
+			ctx->length = mmc_pdata[i].length * MMC_SECTOR_SZ;
+			ctx->associated = (void *)l;
+		}
+	}
+	mmc_part_num = used_mmc_parts;
+	customize_entities(flash_ents, uboot_part_num + mmc_part_num);
+	register_flash_entities(flash_ents, uboot_part_num + mmc_part_num);
+
+	return;
+
+flash_ents_alloc_rollback:
+	while (j--) {
+		free(flash_ents[j].name);
+		free(flash_ents[j].ctx);
+	}
+	free(flash_ents);
+
+partinfo_alloc_rollback:
+	free(uboot_pdata);
+	free(mmc_pdata);
+}
+
+void unregister_flash_areas(void)
+{
+	int j = uboot_part_num + mmc_part_num;
+	while (j--) {
+		free(flash_ents[j].name);
+		free(flash_ents[j].ctx);
+	}
+	free(flash_ents);
+	free(uboot_pdata);
+	free(mmc_pdata);
+}
+
diff --git a/board/samsung/goni/flash.h b/board/samsung/goni/flash.h
new file mode 100644
index 0000000..ffe4d6d
--- /dev/null
+++ b/board/samsung/goni/flash.h
@@ -0,0 +1,28 @@
+/*
+ * flash.h -- board flashing routines interface
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef FLASH_GONI_H_
+#define FLASH_GONI_H_
+
+void register_flash_areas(void);
+void unregister_flash_areas(void);
+
+#endif
diff --git a/board/samsung/goni/goni.c b/board/samsung/goni/goni.c
index c4fc3e9..da9afd0 100644
--- a/board/samsung/goni/goni.c
+++ b/board/samsung/goni/goni.c
@@ -29,6 +29,9 @@
 #include <asm/arch/hs_otg.h>
 #include <asm/arch/cpu.h>
 #include <max8998_pmic.h>
+
+#include "flash.h"
+
 DECLARE_GLOBAL_DATA_PTR;
 
 static struct s5pc110_gpio *s5pc110_gpio;
@@ -144,4 +147,18 @@ struct s3c_plat_otg_data s5pc110_otg_data = {
 	.regs_phy = S5PC110_PHY_BASE,
 	.regs_otg = S5PC110_OTG_BASE,
 };
+
+#ifdef CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE
+void board_dfu_init(void)
+{
+	register_flash_areas();
+	s3c_udc_probe(&s5pc110_otg_data);
+}
+
+void board_dfu_cleanup(void)
+{
+	unregister_flash_areas();
+}
+#endif
+
 #endif
diff --git a/common/Makefile b/common/Makefile
index ae795e0..de926d9 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -162,6 +162,7 @@ COBJS-y += cmd_usb.o
 COBJS-y += usb.o
 COBJS-$(CONFIG_USB_STORAGE) += usb_storage.o
 endif
+COBJS-$(CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE) += cmd_dfu.o
 COBJS-$(CONFIG_CMD_XIMG) += cmd_ximg.o
 COBJS-$(CONFIG_YAFFS2) += cmd_yaffs2.o
 
diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
new file mode 100644
index 0000000..c85fbb1
--- /dev/null
+++ b/common/cmd_dfu.c
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2011 Samsung Electrnoics
+ * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <command.h>
+#include <dfu.h>
+#include <flash_entity.h>
+
+static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+	board_dfu_init();
+	dfu_init();
+	while (1) {
+		int irq_res;
+		/* Handle control-c and timeouts */
+		if (ctrlc()) {
+			printf("The remote end did not respond in time.\n");
+			goto fail;
+		}
+
+		irq_res = usb_gadget_handle_interrupts();
+	}
+fail:
+	dfu_cleanup();
+	board_dfu_cleanup();
+	return -1;
+}
+
+U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
+	"Use the DFU [Device Firmware Upgrade]",
+	"dfu - device firmware upgrade"
+);
+
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index cd22bbe..4b173e2 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -28,6 +28,7 @@ LIB	:= $(obj)libusb_gadget.o
 # new USB gadget layer dependencies
 ifdef CONFIG_USB_GADGET
 COBJS-y += epautoconf.o config.o usbstring.o
+COBJS-$(CONFIG_DFU_GADGET) += dfu.o
 COBJS-$(CONFIG_USB_GADGET_S3C_UDC_OTG) += s3c_udc_otg.o
 endif
 ifdef CONFIG_USB_ETHER
diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
new file mode 100644
index 0000000..535e194
--- /dev/null
+++ b/drivers/usb/gadget/dfu.c
@@ -0,0 +1,920 @@
+/*
+ * dfu.c -- Device Firmware Update gadget
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
+ *
+ * Based on gadget zero:
+ * Copyright (C) 2003-2007 David Brownell
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#define VERBOSE_DEBUG
+#define DEBUG
+
+/*
+#include <linux/kernel.h>
+#include <linux/utsname.h>
+#include <linux/device.h>
+*/
+
+#include <common.h>
+#include <asm-generic/errno.h>
+#include <linux/usb/ch9.h>
+#include <linux/usb/gadget.h>
+
+#include <flash_entity.h>
+
+#include "gadget_chips.h"
+/* #include "epautoconf.c" */
+/* #include "config.c" */
+/* #include "usbstring.c" */
+
+#include <malloc.h>
+#include "dfu.h"
+
+static struct flash_entity *flash_ents;
+static int num_flash_ents;
+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof device_desc,
+	.bDescriptorType =	USB_DT_DEVICE,
+	.bcdUSB =		__constant_cpu_to_le16(0x0100),
+	.bDeviceClass =		USB_CLASS_VENDOR_SPEC,
+	.idVendor =		__constant_cpu_to_le16(DRIVER_VENDOR_NUM),
+	.idProduct =		__constant_cpu_to_le16(DRIVER_PRODUCT_NUM),
+	.iManufacturer =	STRING_MANUFACTURER,
+	.iProduct =		STRING_PRODUCT,
+	.iSerialNumber =	STRING_SERIAL,
+	.bNumConfigurations =	1,
+};
+
+static struct usb_config_descriptor dfu_config = {
+	.bLength =		sizeof dfu_config,
+	.bDescriptorType =	USB_DT_CONFIG,
+	/* compute wTotalLength on the fly */
+	.bNumInterfaces =	1,
+	.bConfigurationValue =	DFU_CONFIG_VAL,
+	.iConfiguration =	STRING_DFU_NAME,
+	.bmAttributes =		USB_CONFIG_ATT_ONE | USB_CONFIG_ATT_SELFPOWER,
+	.bMaxPower =		1,	/* self-powered */
+};
+
+static struct usb_otg_descriptor otg_descriptor = {
+	.bLength =		sizeof otg_descriptor,
+	.bDescriptorType =	USB_DT_OTG,
+	.bmAttributes =		USB_OTG_SRP,
+};
+
+static const struct dfu_function_descriptor dfu_func = {
+	.bLength =		sizeof dfu_func,
+	.bDescriptorType =	DFU_DT_FUNC,
+	.bmAttributes =		DFU_BIT_WILL_DETACH |
+				DFU_BIT_MANIFESTATION_TOLERANT |
+				DFU_BIT_CAN_UPLOAD |
+				DFU_BIT_CAN_DNLOAD,
+	.wDetachTimeOut =	0,
+	.wTransferSize =	USB_BUFSIZ,
+	.bcdDFUVersion =	__constant_cpu_to_le16(0x0110),
+};
+
+static const struct usb_interface_descriptor dfu_intf_runtime = {
+	.bLength =		sizeof dfu_intf_runtime,
+	.bDescriptorType =	USB_DT_INTERFACE,
+	.bNumEndpoints =	0,
+	.bInterfaceClass =	USB_CLASS_APP_SPEC,
+	.bInterfaceSubClass =	1,
+	.bInterfaceProtocol =	1,
+	.iInterface =		STRING_DFU_NAME,
+};
+
+static const struct usb_descriptor_header *dfu_function_runtime[] = {
+	(struct usb_descriptor_header *) &otg_descriptor,
+	(struct usb_descriptor_header *) &dfu_func,
+	(struct usb_descriptor_header *) &dfu_intf_runtime,
+	NULL,
+};
+
+static struct usb_qualifier_descriptor dev_qualifier = {
+	.bLength =		sizeof dev_qualifier,
+	.bDescriptorType =	USB_DT_DEVICE_QUALIFIER,
+	.bcdUSB =		__constant_cpu_to_le16(0x0200),
+	.bDeviceClass =		USB_CLASS_VENDOR_SPEC,
+	.bNumConfigurations =	1,
+};
+
+static char manufacturer[50];
+static const char longname[] = "DFU Gadget";
+/* default serial number takes at least two packets */
+static const char serial[] = "0123456789.0123456789.012345678";
+static const char dfu_name[] = "Device Firmware Upgrade";
+static const char shortname[] = "dfu";
+
+/* static strings, in UTF-8 */
+static struct usb_string strings_runtime[] = {
+	{ STRING_MANUFACTURER, manufacturer, },
+	{ STRING_PRODUCT, longname, },
+	{ STRING_SERIAL, serial, },
+	{ STRING_DFU_NAME, dfu_name, },
+	{  }			/* end of list */
+};
+
+static struct usb_gadget_strings stringtab_runtime = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_runtime,
+};
+
+static struct usb_gadget_strings stringtab_dfu = {
+	.language	= 0x0409,	/* en-us */
+};
+
+static bool is_runtime(struct dfu_dev *dev)
+{
+	return dev->dfu_state == DFU_STATE_appIDLE ||
+		dev->dfu_state == DFU_STATE_appDETACH;
+}
+
+static int config_buf(struct usb_gadget *gadget,
+		u8 *buf, u8 type, unsigned index)
+{
+	int hs = 0;
+	struct dfu_dev *dev = get_gadget_data(gadget);
+	const struct usb_descriptor_header **function;
+	int len;
+
+	if (index > 0)
+		return -EINVAL;
+
+	if (gadget_is_dualspeed(gadget)) {
+		hs = (gadget->speed == USB_SPEED_HIGH);
+		if (type == USB_DT_OTHER_SPEED_CONFIG)
+			hs = !hs;
+	}
+	if (is_runtime(dev))
+		function = dfu_function_runtime;
+	else
+		function = (const struct usb_descriptor_header **)dev->function;
+
+	/* for now, don't advertise srp-only devices */
+	if (!gadget_is_otg(gadget))
+		function++;
+
+	len = usb_gadget_config_buf(&dfu_config,
+			buf, USB_BUFSIZ, function);
+	if (len < 0)
+		return len;
+	((struct usb_config_descriptor *) buf)->bDescriptorType = type;
+	return len;
+}
+
+static void dfu_reset_config(struct dfu_dev *dev)
+{
+	if (dev->config == 0)
+		return;
+
+	DBG(dev, "reset config\n");
+
+	dev->config = 0;
+}
+
+static int dfu_set_config(struct dfu_dev *dev, unsigned number)
+{
+	int result = 0;
+	struct usb_gadget *gadget = dev->gadget;
+	char *speed;
+
+	if (number == dev->config)
+		return 0;
+
+	dfu_reset_config(dev);
+
+	if (DFU_CONFIG_VAL != number) {
+		result = -EINVAL;
+		return result;
+	}
+
+	switch (gadget->speed) {
+	case USB_SPEED_LOW:
+		speed = "low";
+		break;
+	case USB_SPEED_FULL:
+		speed = "full";
+		break;
+	case USB_SPEED_HIGH:
+		speed = "high";
+		break;
+	default:
+		speed = "?";
+		break;
+	}
+
+	dev->config = number;
+	INFO(dev, "%s speed config #%d: %s\n", speed, number, dfu_name);
+	return result;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static void empty_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	/* intentionally empty */
+}
+
+static void dnload_request_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	struct dfu_dev *dev = req->context;
+	struct flash_entity *fe = &flash_ents[dev->altsetting];
+
+	if (dev->not_prepared) {
+		printf("DOWNLOAD %s\n", fe->name);
+		fe->prepare(fe->ctx, FLASH_WRITE);
+		dev->not_prepared = false;
+	}
+
+	if (req->length > 0)
+		fe->write_block(fe->ctx, req->length, req->buf);
+	else {
+		fe->finish(fe->ctx, FLASH_WRITE);
+		dev->not_prepared = true;
+	}
+}
+
+static void handle_getstatus(struct usb_request *req)
+{
+	struct dfu_status *dstat = (struct dfu_status *)req->buf;
+	struct dfu_dev *dev = req->context;
+
+	switch (dev->dfu_state) {
+	case DFU_STATE_dfuDNLOAD_SYNC:
+	case DFU_STATE_dfuDNBUSY:
+		dev->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
+		break;
+	case DFU_STATE_dfuMANIFEST_SYNC:
+		break;
+	default:
+		break;
+	}
+
+	/* send status response */
+	dstat->bStatus = dev->dfu_status;
+	/* FIXME: set dstat->bwPollTimeout */
+	dstat->bState = dev->dfu_state;
+	dstat->iString = 0;
+}
+
+static void handle_getstate(struct usb_request *req)
+{
+	struct dfu_dev *dev = req->context;
+
+	((u8 *)req->buf)[0] = dev->dfu_state & 0xff;
+	req->actual = sizeof(u8);
+}
+
+static int handle_upload(struct usb_request *req, u16 len)
+{
+	struct dfu_dev *dev = req->context;
+	struct flash_entity *fe = &flash_ents[dev->altsetting];
+	int n;
+
+	if (dev->not_prepared) {
+		printf("UPLOAD %s\n", fe->name);
+		fe->prepare(fe->ctx, FLASH_READ);
+		dev->not_prepared = false;
+	}
+	n = fe->read_block(fe->ctx, len, req->buf);
+
+	/* no more data to read from this entity */
+	if (n < len) {
+		fe->finish(fe->ctx, FLASH_READ);
+		dev->not_prepared = true;
+	}
+
+	return n;
+}
+
+static int handle_dnload(struct usb_gadget *gadget, u16 len)
+{
+	struct dfu_dev *dev = get_gadget_data(gadget);
+	struct usb_request *req = dev->req;
+
+	if (len == 0)
+		dev->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
+
+	req->complete = dnload_request_complete;
+
+	return len;
+}
+
+static int
+dfu_handle(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
+{
+	struct dfu_dev *dev = get_gadget_data(gadget);
+	struct usb_request *req = dev->req;
+	u16 len = le16_to_cpu(ctrl->wLength);
+	int rc = 0;
+
+	switch (dev->dfu_state) {
+	case DFU_STATE_appIDLE:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_GETSTATUS:
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		case USB_REQ_DFU_DETACH:
+			dev->dfu_state = DFU_STATE_appDETACH;
+			dev->dfu_state = DFU_STATE_dfuIDLE;
+			return RET_ZLP;
+		default:
+			return RET_STALL;
+		}
+		break;
+	case DFU_STATE_appDETACH:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_GETSTATUS:
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		default:
+			dev->dfu_state = DFU_STATE_appIDLE;
+			return RET_STALL;
+		}
+		/* FIXME: implement timer to return to appIDLE */
+		break;
+	case DFU_STATE_dfuIDLE:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_DNLOAD:
+			if (len == 0) {
+				dev->dfu_state = DFU_STATE_dfuERROR;
+				return RET_STALL;
+			}
+			dev->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
+			return handle_dnload(gadget, len);
+		case USB_REQ_DFU_UPLOAD:
+			dev->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
+			return handle_upload(req, len);
+		case USB_REQ_DFU_ABORT:
+			/* no zlp? */
+			return RET_ZLP;
+		case USB_REQ_DFU_GETSTATUS:
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		case USB_REQ_DFU_DETACH:
+			/* Proprietary extension: 'detach' from idle mode and
+			 * get back to runtime mode in case of USB Reset.  As
+			 * much as I dislike this, we just can't use every USB
+			 * bus reset to switch back to runtime mode, since at
+			 * least the Linux USB stack likes to send a number of
+			 * resets in a row :(
+			 */
+			dev->dfu_state = DFU_STATE_dfuMANIFEST_WAIT_RST;
+			dev->dfu_state = DFU_STATE_appIDLE;
+			break;
+		default:
+			dev->dfu_state = DFU_STATE_dfuERROR;
+			return RET_STALL;
+		}
+		break;
+	case DFU_STATE_dfuDNLOAD_SYNC:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_GETSTATUS:
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+			/* FIXME: state transition depending
+			 * on block completeness */
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		default:
+			dev->dfu_state = DFU_STATE_dfuERROR;
+			return RET_STALL;
+		}
+		break;
+	case DFU_STATE_dfuDNBUSY:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_GETSTATUS:
+			/* FIXME: only accept getstatus if bwPollTimeout
+			 * has elapsed */
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		default:
+			dev->dfu_state = DFU_STATE_dfuERROR;
+			return RET_STALL;
+		}
+		break;
+	case DFU_STATE_dfuDNLOAD_IDLE:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_DNLOAD:
+			dev->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
+			return handle_dnload(gadget, len);
+		case USB_REQ_DFU_ABORT:
+			dev->dfu_state = DFU_STATE_dfuIDLE;
+			return RET_ZLP;
+		case USB_REQ_DFU_GETSTATUS:
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		default:
+			dev->dfu_state = DFU_STATE_dfuERROR;
+			return RET_STALL;
+		}
+		break;
+	case DFU_STATE_dfuMANIFEST_SYNC:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_GETSTATUS:
+			/* We're MainfestationTolerant */
+			dev->dfu_state = DFU_STATE_dfuIDLE;
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		default:
+			dev->dfu_state = DFU_STATE_dfuERROR;
+			return RET_STALL;
+		}
+		break;
+	case DFU_STATE_dfuMANIFEST:
+		/* we should never go here */
+		dev->dfu_state = DFU_STATE_dfuERROR;
+		return RET_STALL;
+	case DFU_STATE_dfuMANIFEST_WAIT_RST:
+		/* we should never go here */
+		break;
+	case DFU_STATE_dfuUPLOAD_IDLE:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_UPLOAD:
+			/* state transition if less data then requested */
+			rc = handle_upload(req, len);
+			if (rc >= 0 && rc < len)
+				dev->dfu_state = DFU_STATE_dfuIDLE;
+			return rc;
+		case USB_REQ_DFU_ABORT:
+			dev->dfu_state = DFU_STATE_dfuIDLE;
+			/* no zlp? */
+			return RET_ZLP;
+		case USB_REQ_DFU_GETSTATUS:
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		default:
+			dev->dfu_state = DFU_STATE_dfuERROR;
+			return RET_STALL;
+		}
+		break;
+	case DFU_STATE_dfuERROR:
+		switch (ctrl->bRequest) {
+		case USB_REQ_DFU_GETSTATUS:
+			handle_getstatus(req);
+			return RET_STAT_LEN;
+		case USB_REQ_DFU_GETSTATE:
+			handle_getstate(req);
+			break;
+		case USB_REQ_DFU_CLRSTATUS:
+			dev->dfu_state = DFU_STATE_dfuIDLE;
+			dev->dfu_status = DFU_STATUS_OK;
+			/* no zlp? */
+			return RET_ZLP;
+		default:
+			dev->dfu_state = DFU_STATE_dfuERROR;
+			return RET_STALL;
+		}
+		break;
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+static int
+dfu_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
+{
+	struct dfu_dev *dev = get_gadget_data(gadget);
+	struct usb_request *req = dev->req;
+	int value = -EOPNOTSUPP;
+	u16 w_index = le16_to_cpu(ctrl->wIndex);
+	u16 w_value = le16_to_cpu(ctrl->wValue);
+	u16 w_length = le16_to_cpu(ctrl->wLength);
+
+	req->zero = 0;
+	req->complete = empty_complete;
+	if (!(ctrl->bRequestType & USB_TYPE_CLASS))
+		switch (ctrl->bRequest) {
+
+		case USB_REQ_GET_DESCRIPTOR:
+			if (ctrl->bRequestType != USB_DIR_IN)
+				goto unknown;
+			switch (w_value >> 8) {
+
+			case USB_DT_DEVICE:
+				value = min(w_length, (u16) sizeof device_desc);
+				memcpy(req->buf, &device_desc, value);
+				break;
+			case USB_DT_DEVICE_QUALIFIER:
+				if (!gadget_is_dualspeed(gadget))
+					break;
+				value = min(w_length,
+					    (u16) sizeof dev_qualifier);
+				memcpy(req->buf, &dev_qualifier, value);
+				break;
+
+			case USB_DT_OTHER_SPEED_CONFIG:
+				if (!gadget_is_dualspeed(gadget))
+					break;
+				/* FALLTHROUGH */
+			case USB_DT_CONFIG:
+				value = config_buf(gadget, req->buf,
+						w_value >> 8,
+						w_value & 0xff);
+				if (value >= 0)
+					value = min_t(w_length, (u16) value);
+				break;
+
+			case USB_DT_STRING:
+				/* wIndex == language code. */
+				value = usb_gadget_get_string(
+					is_runtime(dev) ? &stringtab_runtime :
+					&stringtab_dfu,
+					w_value & 0xff, req->buf);
+				if (value >= 0)
+					value = min_t(w_length, (u16) value);
+				break;
+			case DFU_DT_FUNC:
+				value = min(w_length, (u16) sizeof dfu_func);
+				memcpy(req->buf, &dfu_func, value);
+				break;
+			}
+			break;
+
+		case USB_REQ_SET_CONFIGURATION:
+			if (ctrl->bRequestType != 0)
+				goto unknown;
+			if (gadget->a_hnp_support)
+				DBG(dev, "HNP available\n");
+			else if (gadget->a_alt_hnp_support)
+				DBG(dev, "HNP needs a different root port\n");
+			else
+				VDBG(dev, "HNP inactive\n");
+			spin_lock(&dev->lock);
+			value = dfu_set_config(dev, w_value);
+			spin_unlock(&dev->lock);
+			break;
+		case USB_REQ_GET_CONFIGURATION:
+			if (ctrl->bRequestType != USB_DIR_IN)
+				goto unknown;
+			*(u8 *)req->buf = dev->config;
+			value = min(w_length, (u16) 1);
+			break;
+
+		/* until we add altsetting support, or other interfaces,
+		 * only 0/0 are possible.  pxa2xx only supports 0/0 (poorly)
+		 * and already killed pending endpoint I/O.
+		 */
+		case USB_REQ_SET_INTERFACE:
+			if (ctrl->bRequestType != USB_RECIP_INTERFACE)
+				goto unknown;
+			spin_lock(&dev->lock);
+			if (dev->config && w_index == 0) {
+				u8		config = dev->config;
+
+				/* resets interface configuration, forgets about
+				 * previous transaction state (queued bufs, etc)
+				 * and re-inits endpoint state (toggle etc)
+				 * no response queued, just zero
+				 * status == success.
+				 * if we had more than one interface we couldn't
+				 * use this "reset the config" shortcut.
+				 */
+				dfu_reset_config(dev);
+				dfu_set_config(dev, config);
+				dev->altsetting = w_value;
+				value = 0;
+			}
+			spin_unlock(&dev->lock);
+			break;
+		case USB_REQ_GET_INTERFACE:
+			if (ctrl->bRequestType !=
+			    (USB_DIR_IN|USB_RECIP_INTERFACE))
+				goto unknown;
+			if (!dev->config)
+				break;
+			if (w_index != 0) {
+				value = -EDOM;
+				break;
+			}
+			*(u8 *)req->buf = 0;
+			value = min(w_length, (u16) 1);
+			break;
+
+		default:
+unknown:
+			VDBG(dev,
+				"unknown control req%02x.%02x v%04x i%04x l%d\n",
+				ctrl->bRequestType, ctrl->bRequest,
+				w_value, w_index, w_length);
+		}
+	else
+		value = dfu_handle(gadget, ctrl);
+
+	/* respond with data transfer before status phase? */
+	if (value >= 0) {
+		req->length = value;
+		req->zero = value < w_length;
+		value = usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
+		if (value < 0) {
+			DBG(dev, "ep_queue --> %d\n", value);
+			req->status = 0;
+		}
+	}
+
+	/* device either stalls (value < 0) or reports success */
+	return value;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static void dfu_disconnect(struct usb_gadget *gadget)
+{
+	struct dfu_dev *dev = get_gadget_data(gadget);
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->lock, flags);
+	dfu_reset_config(dev);
+	spin_unlock_irqrestore(&dev->lock, flags);
+}
+
+static int dfu_prepare_function(struct dfu_dev *dev, int n)
+{
+	struct usb_interface_descriptor *d;
+	int i = 0;
+
+	dev->function = kzalloc((ALTSETTING_BASE + n + 1) *
+				sizeof(struct usb_descriptor_header *),
+				GFP_KERNEL);
+	if (!dev->function)
+		goto enomem;
+
+	dev->function[0] = (struct usb_descriptor_header *)&otg_descriptor;
+	dev->function[1] = (struct usb_descriptor_header *)&dfu_func;
+
+	for (i = 0; i < n; ++i) {
+		d = kzalloc(sizeof(*d), GFP_KERNEL);
+		if (!d)
+			goto enomem;
+
+		d->bLength =		sizeof(*d);
+		d->bDescriptorType =	USB_DT_INTERFACE;
+		d->bAlternateSetting =	i;
+		d->bNumEndpoints =	0;
+		d->bInterfaceClass =	USB_CLASS_APP_SPEC;
+		d->bInterfaceSubClass =	1;
+		d->bInterfaceProtocol =	2;
+		d->iInterface =		DFU_STR_BASE + i;
+
+		dev->function[ALTSETTING_BASE + i] =
+			(struct usb_descriptor_header *)d;
+	}
+	dev->function[ALTSETTING_BASE + i] = NULL;
+
+	return 1;
+
+enomem:
+	while (i) {
+		kfree(dev->function[--i + ALTSETTING_BASE]);
+		dev->function[i + ALTSETTING_BASE] = NULL;
+	}
+	kfree(dev->function);
+
+	return 0;
+}
+
+static int
+dfu_prepare_strings(struct dfu_dev *dev, struct flash_entity *f, int n)
+{
+	int i = 0;
+
+	dev->strings = kzalloc((STRING_ALTSETTING_BASE + n + 1) *
+				sizeof(struct usb_string),
+				GFP_KERNEL);
+	if (!dev->strings)
+		goto enomem;
+
+	dev->strings[0].id = STRING_MANUFACTURER;
+	dev->strings[0].s = manufacturer;
+	dev->strings[1].id = STRING_PRODUCT;
+	dev->strings[1].s = longname;
+	dev->strings[2].id = STRING_SERIAL;
+	dev->strings[2].s = serial;
+	dev->strings[3].id = STRING_DFU_NAME;
+	dev->strings[3].s = dfu_name;
+
+	for (i = 0; i < n; ++i) {
+		char *s;
+
+		dev->strings[STRING_ALTSETTING_BASE + i].id = DFU_STR_BASE + i;
+		s = kzalloc(strlen(f[i].name) + 1, GFP_KERNEL);
+		if (!s)
+			goto enomem;
+
+		strcpy(s, f[i].name);
+		dev->strings[STRING_ALTSETTING_BASE + i].s = s;
+	}
+	dev->strings[STRING_ALTSETTING_BASE + i].id = 0;
+	dev->strings[STRING_ALTSETTING_BASE + i].s = NULL;
+
+	return 1;
+
+enomem:
+	while (i) {
+		kfree((void *)dev->strings[--i + STRING_ALTSETTING_BASE].s);
+		dev->strings[i + STRING_ALTSETTING_BASE].s = NULL;
+	}
+	kfree(dev->strings);
+
+	return 0;
+}
+
+static void dfu_unbind(struct usb_gadget *gadget)
+{
+	struct dfu_dev *dev = get_gadget_data(gadget);
+	int i;
+
+	DBG(dev, "unbind\n");
+
+	if (dev->strings) {
+		i = num_flash_ents;
+		while (i) {
+			kfree((void *)
+			      dev->strings[--i + STRING_ALTSETTING_BASE].s);
+			dev->strings[i + STRING_ALTSETTING_BASE].s = NULL;
+		}
+		kfree(dev->strings);
+	}
+	if (dev->function) {
+		i = num_flash_ents;
+		while (i) {
+			kfree(dev->function[--i + ALTSETTING_BASE]);
+			dev->function[i + ALTSETTING_BASE] = NULL;
+		}
+		kfree(dev->function);
+	}
+	/* we've already been disconnected ... no i/o is active */
+	if (dev->req) {
+		dev->req->length = USB_BUFSIZ;
+		kfree(dev->req->buf);
+		usb_ep_free_request(gadget->ep0, dev->req);
+	}
+	kfree(dev);
+	set_gadget_data(gadget, NULL);
+}
+
+static int __init dfu_bind(struct usb_gadget *gadget)
+{
+	struct dfu_dev *dev;
+	int gcnum;
+
+	usb_ep_autoconfig_reset(gadget);
+
+	gcnum = usb_gadget_controller_number(gadget);
+	if (gcnum >= 0)
+		device_desc.bcdDevice = cpu_to_le16(0x0200 + gcnum);
+	else {
+		pr_warning("%s: controller '%s' not recognized\n",
+			shortname, gadget->name);
+		device_desc.bcdDevice = __constant_cpu_to_le16(0x9999);
+	}
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+	spin_lock_init(&dev->lock);
+	dev->gadget = gadget;
+	set_gadget_data(gadget, dev);
+
+	dev->req = usb_ep_alloc_request(gadget->ep0, GFP_KERNEL);
+	if (!dev->req)
+		goto enomem;
+	dev->req->buf = kmalloc(USB_BUFSIZ, GFP_KERNEL);
+	if (!dev->req->buf)
+		goto enomem;
+
+	dev->req->complete = empty_complete;
+
+	device_desc.bMaxPacketSize0 = gadget->ep0->maxpacket;
+
+	if (gadget_is_dualspeed(gadget))
+		dev_qualifier.bMaxPacketSize0 = device_desc.bMaxPacketSize0;
+
+	if (gadget_is_otg(gadget)) {
+		otg_descriptor.bmAttributes |= USB_OTG_HNP,
+		dfu_config.bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+	}
+
+	usb_gadget_set_selfpowered(gadget);
+
+	dev->dfu_state = DFU_STATE_appIDLE;
+	dev->dfu_status = DFU_STATUS_OK;
+	dev->not_prepared = true;
+
+	if (!dfu_prepare_function(dev, num_flash_ents))
+		goto enomem;
+
+	if (!dfu_prepare_strings(dev, flash_ents, num_flash_ents))
+		goto enomem;
+	stringtab_dfu.strings = dev->strings;
+
+	gadget->ep0->driver_data = dev;
+	dev->req->context = dev;
+
+	INFO(dev, "%s, version: " DRIVER_VERSION "\n", longname);
+
+	/* snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
+		init_utsname()->sysname, init_utsname()->release,
+		gadget->name); */
+
+	return 0;
+
+enomem:
+	dfu_unbind(gadget);
+	return -ENOMEM;
+}
+
+static void dfu_suspend(struct usb_gadget *gadget)
+{
+	if (gadget->speed == USB_SPEED_UNKNOWN)
+		return;
+
+	DBG(dev, "suspend\n");
+}
+
+static void dfu_resume(struct usb_gadget *gadget)
+{
+	DBG(dev, "resume\n");
+}
+
+static struct usb_gadget_driver dfu_driver = {
+#ifdef CONFIG_USB_GADGET_DUALSPEED
+	.speed		= USB_SPEED_HIGH,
+#else
+	.speed		= USB_SPEED_FULL,
+#endif
+	/*.function	= (char *) longname,*/
+	.bind		= dfu_bind,
+	.unbind		= __exit_p(dfu_unbind),
+
+	.setup		= dfu_setup,
+	.disconnect	= dfu_disconnect,
+
+	.suspend	= dfu_suspend,
+	.resume		= dfu_resume,
+
+	/*
+	.driver		= {
+		.name		= (char *) shortname,
+		.owner		= THIS_MODULE,
+	},*/
+};
+
+void register_flash_entities(struct flash_entity *flents, int n)
+{
+	flash_ents = flents;
+	num_flash_ents = n;
+}
+
+int __init dfu_init(void)
+{
+	return usb_gadget_register_driver(&dfu_driver);
+}
+module_init(dfu_init);
+
+void __exit dfu_cleanup(void)
+{
+	usb_gadget_unregister_driver(&dfu_driver);
+}
+module_exit(cleanup);
+
diff --git a/drivers/usb/gadget/dfu.h b/drivers/usb/gadget/dfu.h
new file mode 100644
index 0000000..5a6386e
--- /dev/null
+++ b/drivers/usb/gadget/dfu.h
@@ -0,0 +1,171 @@
+/*
+ * dfu.h -- Device Firmware Update gadget
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef DFU_H_
+#define DFU_H_
+
+#include <linux/compiler.h>
+
+/*
+ * Linux kernel compatibility layer
+ */
+#define GFP_ATOMIC				((gfp_t) 0)
+#define GFP_KERNEL				((gfp_t) 0)
+#define true					1
+#define false					0
+#define dev_dbg(...)				do {} while (0)
+#define dev_vdbg(...)				do {} while (0)
+#define dev_err(...)				do {} while (0)
+#define dev_warn(...)				do {} while (0)
+#define dev_info(...)				do {} while (0)
+#define pr_warning(...)				do {} while (0)
+#define spin_lock_init(lock)			do {} while (0)
+#define spin_lock(lock)				do {} while (0)
+#define spin_unlock(lock)			do {} while (0)
+#define spin_lock_irqsave(lock,flags)		do {flags = 1;} while (0)
+#define spin_unlock_irqrestore(lock,flags)	do {flags = 0;} while (0)
+#define kmalloc(x,y)				malloc(x)
+#define kfree(x)				free(x)
+#define kzalloc(size,flags)			calloc((size), 1)
+#define __init
+#define __exit
+#define __exit_p(x)				x
+#define module_init(...)
+#define module_exit(...)
+#define min_t					min
+#define spinlock_t				int
+#define bool					int
+/*
+ * end compatibility layer
+ */
+
+#define DBG(d, fmt, args...) \
+	dev_dbg(&(d)->gadget->dev , fmt , ## args)
+#define VDBG(d, fmt, args...) \
+	dev_vdbg(&(d)->gadget->dev , fmt , ## args)
+#define ERROR(d, fmt, args...) \
+	dev_err(&(d)->gadget->dev , fmt , ## args)
+#define WARN(d, fmt, args...) \
+	dev_warn(&(d)->gadget->dev , fmt , ## args)
+#define INFO(d, fmt, args...) \
+	dev_info(&(d)->gadget->dev , fmt , ## args)
+
+#define DRIVER_VERSION			"Marzanna"
+
+/* Thanks to NetChip Technologies for donating this product ID.  */
+#define DRIVER_VENDOR_NUM		0x0525		/* NetChip */
+#define DRIVER_PRODUCT_NUM		0xffff		/* DFU */
+
+#define STRING_MANUFACTURER		0
+#define STRING_PRODUCT			1
+#define STRING_SERIAL			2
+#define STRING_DFU_NAME			49
+#define DFU_STR_BASE			50
+
+#define	DFU_CONFIG_VAL			1
+#define DFU_DT_FUNC			0x21
+
+#define DFU_BIT_WILL_DETACH		(0x1 << 3)
+#define DFU_BIT_MANIFESTATION_TOLERANT	(0x1 << 2)
+#define DFU_BIT_CAN_UPLOAD		(0x1 << 1)
+#define DFU_BIT_CAN_DNLOAD		0x1
+
+/* big enough to hold our biggest descriptor */
+#define USB_BUFSIZ			4096
+
+#define USB_REQ_DFU_DETACH		0x00
+#define USB_REQ_DFU_DNLOAD		0x01
+#define USB_REQ_DFU_UPLOAD		0x02
+#define USB_REQ_DFU_GETSTATUS		0x03
+#define USB_REQ_DFU_CLRSTATUS		0x04
+#define USB_REQ_DFU_GETSTATE		0x05
+#define USB_REQ_DFU_ABORT		0x06
+
+#define DFU_STATUS_OK			0x00
+#define DFU_STATUS_errTARGET		0x01
+#define DFU_STATUS_errFILE		0x02
+#define DFU_STATUS_errWRITE		0x03
+#define DFU_STATUS_errERASE		0x04
+#define DFU_STATUS_errCHECK_ERASED	0x05
+#define DFU_STATUS_errPROG		0x06
+#define DFU_STATUS_errVERIFY		0x07
+#define DFU_STATUS_errADDRESS		0x08
+#define DFU_STATUS_errNOTDONE		0x09
+#define DFU_STATUS_errFIRMWARE		0x0a
+#define DFU_STATUS_errVENDOR		0x0b
+#define DFU_STATUS_errUSBR		0x0c
+#define DFU_STATUS_errPOR		0x0d
+#define DFU_STATUS_errUNKNOWN		0x0e
+#define DFU_STATUS_errSTALLEDPKT	0x0f
+
+#define RET_STALL			-1
+#define RET_ZLP				0
+#define RET_STAT_LEN			6
+
+#define ALTSETTING_BASE			2
+#define STRING_ALTSETTING_BASE		4
+
+enum dfu_state {
+	DFU_STATE_appIDLE		= 0,
+	DFU_STATE_appDETACH		= 1,
+	DFU_STATE_dfuIDLE		= 2,
+	DFU_STATE_dfuDNLOAD_SYNC	= 3,
+	DFU_STATE_dfuDNBUSY		= 4,
+	DFU_STATE_dfuDNLOAD_IDLE	= 5,
+	DFU_STATE_dfuMANIFEST_SYNC	= 6,
+	DFU_STATE_dfuMANIFEST		= 7,
+	DFU_STATE_dfuMANIFEST_WAIT_RST	= 8,
+	DFU_STATE_dfuUPLOAD_IDLE	= 9,
+	DFU_STATE_dfuERROR		= 10,
+};
+
+struct dfu_status {
+	__u8				bStatus;
+	__u8				bwPollTimeout[3];
+	__u8				bState;
+	__u8				iString;
+} __packed;
+
+struct dfu_function_descriptor {
+	__u8				bLength;
+	__u8				bDescriptorType;
+	__u8				bmAttributes;
+	__le16				wDetachTimeOut;
+	__le16				wTransferSize;
+	__le16				bcdDFUVersion;
+} __packed;
+
+struct dfu_dev {
+	spinlock_t			lock;
+	struct usb_gadget		*gadget;
+	struct usb_request		*req;	/* for control responses */
+
+	/* when configured, we have one config */
+	u8				config;
+	u8				altsetting;
+	enum dfu_state			dfu_state;
+	unsigned int			dfu_status;
+	struct usb_descriptor_header	**function;
+	struct usb_string		*strings;
+	bool				not_prepared;
+};
+
+#endif /* DFU_H_ */
diff --git a/include/configs/s5p_goni.h b/include/configs/s5p_goni.h
index df97802..186da76 100644
--- a/include/configs/s5p_goni.h
+++ b/include/configs/s5p_goni.h
@@ -86,6 +86,8 @@
 #define CONFIG_CMD_ONENAND
 #define CONFIG_CMD_MTDPARTS
 #define CONFIG_CMD_MMC
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE
 
 #define CONFIG_BOOTDELAY		1
 #define CONFIG_ZERO_BOOTDELAY_CHECK
@@ -241,4 +243,8 @@
 #define CONFIG_USB_GADGET_S3C_UDC_OTG	1
 #define CONFIG_USB_GADGET_DUALSPEED	1
 
+#if defined (CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE)
+#define CONFIG_DFU_GADGET
+#endif
+
 #endif	/* __CONFIG_H */
diff --git a/include/dfu.h b/include/dfu.h
new file mode 100644
index 0000000..977ca91
--- /dev/null
+++ b/include/dfu.h
@@ -0,0 +1,31 @@
+/*
+ * dfu.h - Device Firmware Upgrade
+ *
+ * copyright (c) 2011 samsung electronics
+ * author: andrzej pietrasiewicz <andrzej.p@samsung.com>
+ *
+ * this program is free software; you can redistribute it and/or modify
+ * it under the terms of the gnu general public license as published by
+ * the free software foundation; either version 2 of the license, or
+ * (at your option) any later version.
+ *
+ * this program is distributed in the hope that it will be useful,
+ * but without any warranty; without even the implied warranty of
+ * merchantability or fitness for a particular purpose.  see the
+ * gnu general public license for more details.
+ *
+ * you should have received a copy of the gnu general public license
+ * along with this program; if not, write to the free software
+ * foundation, inc., 59 temple place, suite 330, boston, ma  02111-1307  usa
+ */
+
+#ifndef __DFU_H__
+#define __DFU_H__
+
+extern void board_dfu_init(void);
+extern int dfu_init(void);
+extern int dfu_cleanup(void);
+extern int board_dfu_cleanup(void);
+extern int usb_gadget_handle_interrupts(void);
+
+#endif
diff --git a/include/flash_entity.h b/include/flash_entity.h
new file mode 100644
index 0000000..daa90ee
--- /dev/null
+++ b/include/flash_entity.h
@@ -0,0 +1,39 @@
+/*
+ * flash_entity.h - flashable area description
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef FLASH_ENTITY_H_
+#define FLASH_ENTITY_H_
+
+#define FLASH_READ			0
+#define FLASH_WRITE			1
+
+struct flash_entity {
+	char				*name;
+	void				*ctx;
+	int (*prepare)(void *ctx, u8 mode);
+	int (*read_block)(void *ctx, unsigned int n, void *buf);
+	int (*write_block)(void *ctx, unsigned int n, void *buf);
+	int (*finish)(void *ctx, u8 mode);
+};
+
+void register_flash_entities(struct flash_entity *flents, int n);
+
+#endif /* FLASH_ENTITY_H_ */
diff --git a/include/mbr.h b/include/mbr.h
new file mode 100644
index 0000000..9cbeb2e
--- /dev/null
+++ b/include/mbr.h
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2010 Samsung Electrnoics
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <linux/compiler.h>
+
+#define MBR_RESERVED_SIZE	0x8000
+#define MBR_START_OFS_BLK	(0x500000 / 512)
+
+struct mbr_partition {
+	char status;
+	char f_chs[3];
+	char partition_type;
+	char l_chs[3];
+	int lba;
+	int nsectors;
+} __packed;
+
+struct mbr {
+	char code_area[440];
+	char disk_signature[4];
+	char nulls[2];
+	struct mbr_partition parts[4];
+	unsigned short signature;
+};
+
+extern unsigned int mbr_offset[16];
+extern unsigned int mbr_parts;
+
+void set_mbr_dev(int dev);
+void set_mbr_info(char *ramaddr, unsigned int len, unsigned int reserved);
+void set_mbr_table(unsigned int start_addr, int parts,
+		unsigned int *blocks, unsigned int *part_offset);
+int get_mbr_table(unsigned int *part_offset);
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 10:00 [U-Boot] [PATCH] dfu: initial implementation Andrzej Pietrasiewicz
@ 2011-11-02 14:29 ` Stefan Schmidt
  2011-11-03  8:12   ` Andrzej Pietrasiewicz
  2011-11-02 15:16 ` Mike Frysinger
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-02 14:29 UTC (permalink / raw)
  To: u-boot

Hello.

This really comes as surprise to me as I'm working on exactly the same
at the moment. I just realised that I only mentioned it here inside the
fastboot patches thread.

I started from the old patches Harald Welte wrote for u-boot during
his work for OpenMoko. I worked with him during this time at OpenMoko
and I'm the current maintainer of dfu-util. (Just as background).

You seem to have started from scratch with this DFU implementation
instead of using the older but available patches. Was there a specific
reason for this?

I found it not that complicated to update them on a current u-boot and
adjust it accordingly. I have it working here with a beagleboard. Was
about to send them out next week but you have been a small tick faster
here. :)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 10:00 [U-Boot] [PATCH] dfu: initial implementation Andrzej Pietrasiewicz
  2011-11-02 14:29 ` Stefan Schmidt
@ 2011-11-02 15:16 ` Mike Frysinger
  2011-11-02 19:30   ` Stefan Schmidt
  2011-11-03  8:17   ` Andrzej Pietrasiewicz
  2011-11-02 20:07 ` Stefan Schmidt
  2011-11-03 18:13 ` Wolfgang Denk
  3 siblings, 2 replies; 26+ messages in thread
From: Mike Frysinger @ 2011-11-02 15:16 UTC (permalink / raw)
  To: u-boot

On Wednesday 02 November 2011 06:00:07 Andrzej Pietrasiewicz wrote:
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 
> Dear All,
> 
> This is Device Firmware Upgrade (DFU) implementation which supports data
> upload and download function to devices which are equipped with a UDC.

this information belongs in the changelog (above the "---" marker)

are you working with the elinux.org guys ?
	http://elinux.org/Merge_DFU_support_into_mainline_U-Boot

>  board/samsung/goni/Makefile |    2 +
>  board/samsung/goni/flash.c  |  634
>  board/samsung/goni/flash.h  |   28 ++
>  board/samsung/goni/goni.c   |   17 +
>  common/Makefile             |    1 +
>  common/cmd_dfu.c            |   50 +++
>  drivers/usb/gadget/Makefile |    1 +
>  drivers/usb/gadget/dfu.c    |  920
>  drivers/usb/gadget/dfu.h    |  171
>  include/configs/s5p_goni.h  |    6 +
>  include/dfu.h               |   31 ++
>  include/flash_entity.h      |   39 ++
>  include/mbr.h               |   49 +++

this should be split up into at least the dfu core and board-specific changes.  
although i'd wonder how much of the board/samsung/ stuff is really board 
specific and couldn't be generalized ...

> --- /dev/null
> +++ b/include/dfu.h
>
> +extern int usb_gadget_handle_interrupts(void);

this doesn't belong in the dfu header

> --- /dev/null
> +++ b/include/flash_entity.h
>
> +struct flash_entity {
> +	char				*name;

should probably be const

> --- /dev/null
> +++ b/include/mbr.h
>
> +/*
> + * Copyright (C) 2010 Samsung Electrnoics
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <linux/compiler.h>

missing ifdef protection against multiple inclusion

this also should be split out of the dfu core ... although it seems weird that 
u-boot doesn't already have mbr parsing code ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111102/11ca9ca8/attachment.pgp 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 15:16 ` Mike Frysinger
@ 2011-11-02 19:30   ` Stefan Schmidt
  2011-11-03  8:44     ` Andrzej Pietrasiewicz
  2011-11-03  8:17   ` Andrzej Pietrasiewicz
  1 sibling, 1 reply; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-02 19:30 UTC (permalink / raw)
  To: u-boot

Hello.

On Wed, 2011-11-02 at 11:16, Mike Frysinger wrote:
> On Wednesday 02 November 2011 06:00:07 Andrzej Pietrasiewicz wrote:
> 
> are you working with the elinux.org guys ?
> 	http://elinux.org/Merge_DFU_support_into_mainline_U-Boot

That would be me. As I stated in my other mail I was surprised seeing
this patch coming in. I did not communicate my work on this broadly
either. Wanted to show up with soem real code not only promises. Same
as Andrzej it seems. :)

A quick comparison shows me that there are areas that don't touch each
other at all (mmc and images on fat as backend vs. raw nand flash as
backend) and areas that are duplicated and needed to get sorted out.
The DFU protocol implementation in this case. I hope Andrzej is
willing to work toegtehr with me on a final implementation that
contains pieces of both versions. Mine is missing a code split between
DFU protocol and flashing backend for example but feature other parts
missing here.

> >  board/samsung/goni/Makefile |    2 +
> >  board/samsung/goni/flash.c  |  634
> >  board/samsung/goni/flash.h  |   28 ++
> >  board/samsung/goni/goni.c   |   17 +
> >  common/Makefile             |    1 +
> >  common/cmd_dfu.c            |   50 +++
> >  drivers/usb/gadget/Makefile |    1 +
> >  drivers/usb/gadget/dfu.c    |  920
> >  drivers/usb/gadget/dfu.h    |  171
> >  include/configs/s5p_goni.h  |    6 +
> >  include/dfu.h               |   31 ++
> >  include/flash_entity.h      |   39 ++
> >  include/mbr.h               |   49 +++
> 
> this should be split up into at least the dfu core and board-specific changes.  
> although i'd wonder how much of the board/samsung/ stuff is really board 
> specific and couldn't be generalized ...

Agreed. The eMMC flashing with files on FAT is nothing goni specific.
Others should be able to use this as well. I see three different parts
here that can be separated:

1) The DFU protocol implementation which lives in usb gadget with an
interface for flashing backends

2) The flashing backends (no idea where to put them best). At the
moment that would be the implementation here with eMMC and files on
FAT, mine with raw NAND devices and in the future maybe others. Each
flashing backend should be generic enough to allow different boards
using it.

3) Board specific information like partitions or hints for the
flashing backend. The information itself should be in the board file
here and only used by the flashing backends.

How does this sound to you? Andrzej?

regards
Stefan Schmidt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 250 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111102/4d516221/attachment.pgp 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 10:00 [U-Boot] [PATCH] dfu: initial implementation Andrzej Pietrasiewicz
  2011-11-02 14:29 ` Stefan Schmidt
  2011-11-02 15:16 ` Mike Frysinger
@ 2011-11-02 20:07 ` Stefan Schmidt
  2011-11-03 10:33   ` Andrzej Pietrasiewicz
                     ` (2 more replies)
  2011-11-03 18:13 ` Wolfgang Denk
  3 siblings, 3 replies; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-02 20:07 UTC (permalink / raw)
  To: u-boot

Hello.

@Remy: One question I have for you is if the DFU implementation should
be based on the re-written gadget layer from samsung or based on the
current one?

First, and only high level, review for the DFU part.

Against which u-boot tree/branch/version is this patch? I tried to
apply it against HEAD and it failed for me. Anything I miss?

On Wed, 2011-11-02 at 11:00, Andrzej Pietrasiewicz wrote:
> 
> The implementation is split into two parts: the dfu gadget implementation
> and a "flashing backend", the interface between the two being the
> struct flash_entity. The "flashing backend"'s implementation is
> board-specific and is implemented on the Goni target.

I replied to Mike's mail with my ideas on this. Split between dfu and
flashing backends is good and missng in my approach currently. I would
not put it into the board file though but make it generic for other
boards as well and only define the needed informations in the board
file. Please see my other mail for details. Comments appreciated!

> diff --git a/common/Makefile b/common/Makefile
> index ae795e0..de926d9 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -162,6 +162,7 @@ COBJS-y += cmd_usb.o
>  COBJS-y += usb.o
>  COBJS-$(CONFIG_USB_STORAGE) += usb_storage.o
>  endif
> +COBJS-$(CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE) += cmd_dfu.o

CONFIG_CMD_DFU instead? Looks a bit long to me.

>  COBJS-$(CONFIG_CMD_XIMG) += cmd_ximg.o
>  COBJS-$(CONFIG_YAFFS2) += cmd_yaffs2.o
>  
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
V> new file mode 100644
> index 0000000..c85fbb1
> --- /dev/null
> +++ b/common/cmd_dfu.c
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright (C) 2011 Samsung Electrnoics
> + * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <dfu.h>
> +#include <flash_entity.h>
> +
> +static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	board_dfu_init();
> +	dfu_init();
> +	while (1) {
> +		int irq_res;
> +		/* Handle control-c and timeouts */
> +		if (ctrlc()) {
> +			printf("The remote end did not respond in time.\n");
> +			goto fail;
> +		}
> +
> +		irq_res = usb_gadget_handle_interrupts();
> +	}
> +fail:
> +	dfu_cleanup();
> +	board_dfu_cleanup();
> +	return -1;
> +}
> +
> +U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
> +	"Use the DFU [Device Firmware Upgrade]",
> +	"dfu - device firmware upgrade"
> +);
> +

While I think a dfu command is usefull I don't like the need to
execute it before any DFU interaction can happen. That may be an
option during development but for field upgrades or receovery it is
not.

I already wrote a bit about my approach here. At OpenMoko we used a
button to enter u-boot during startup when pressed. This offered a
usbtty interface as usb gadget as well as the runtime descripto for
DFU. With dfu-util it was possible to iniate the DFU download or
upload procedure while being in the mode. Another option would be to
directly jump into DFU mode when a button is pressed.

> diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> new file mode 100644
> index 0000000..535e194
> --- /dev/null
> +++ b/drivers/usb/gadget/dfu.c
> @@ -0,0 +1,920 @@
> +/*
> + * dfu.c -- Device Firmware Update gadget
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> + *
> + * Based on gadget zero:
> + * Copyright (C) 2003-2007 David Brownell
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#define VERBOSE_DEBUG
> +#define DEBUG
> +
> +/*
> +#include <linux/kernel.h>
> +#include <linux/utsname.h>
> +#include <linux/device.h>
> +*/
> +
> +#include <common.h>
> +#include <asm-generic/errno.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +
> +#include <flash_entity.h>
> +
> +#include "gadget_chips.h"
> +/* #include "epautoconf.c" */
> +/* #include "config.c" */
> +/* #include "usbstring.c" */

Not needed, can be removed.

> +#include <malloc.h>
> +#include "dfu.h"
> +
> +static struct flash_entity *flash_ents;
> +static int num_flash_ents;
> +
> +static struct usb_device_descriptor device_desc = {
> +	.bLength =		sizeof device_desc,
> +	.bDescriptorType =	USB_DT_DEVICE,
> +	.bcdUSB =		__constant_cpu_to_le16(0x0100),
> +	.bDeviceClass =		USB_CLASS_VENDOR_SPEC,
> +	.idVendor =		__constant_cpu_to_le16(DRIVER_VENDOR_NUM),
> +	.idProduct =		__constant_cpu_to_le16(DRIVER_PRODUCT_NUM),
> +	.iManufacturer =	STRING_MANUFACTURER,
> +	.iProduct =		STRING_PRODUCT,
> +	.iSerialNumber =	STRING_SERIAL,
> +	.bNumConfigurations =	1,
> +};
> +
> +static struct usb_config_descriptor dfu_config = {
> +	.bLength =		sizeof dfu_config,
> +	.bDescriptorType =	USB_DT_CONFIG,
> +	/* compute wTotalLength on the fly */
> +	.bNumInterfaces =	1,
> +	.bConfigurationValue =	DFU_CONFIG_VAL,
> +	.iConfiguration =	STRING_DFU_NAME,
> +	.bmAttributes =		USB_CONFIG_ATT_ONE | USB_CONFIG_ATT_SELFPOWER,
> +	.bMaxPower =		1,	/* self-powered */
> +};
> +
> +static struct usb_otg_descriptor otg_descriptor = {
> +	.bLength =		sizeof otg_descriptor,
> +	.bDescriptorType =	USB_DT_OTG,
> +	.bmAttributes =		USB_OTG_SRP,
> +};
> +
> +static const struct dfu_function_descriptor dfu_func = {
> +	.bLength =		sizeof dfu_func,
> +	.bDescriptorType =	DFU_DT_FUNC,
> +	.bmAttributes =		DFU_BIT_WILL_DETACH |

Here are you setting the WILL_DETACH bit. Is the device really
detaching itself? Its not obvious to me from the code that it does.

> +				DFU_BIT_MANIFESTATION_TOLERANT |
> +				DFU_BIT_CAN_UPLOAD |
> +				DFU_BIT_CAN_DNLOAD,
> +	.wDetachTimeOut =	0,
> +	.wTransferSize =	USB_BUFSIZ,
> +	.bcdDFUVersion =	__constant_cpu_to_le16(0x0110),
> +};
> +
> +static const struct usb_interface_descriptor dfu_intf_runtime = {
> +	.bLength =		sizeof dfu_intf_runtime,
> +	.bDescriptorType =	USB_DT_INTERFACE,
> +	.bNumEndpoints =	0,
> +	.bInterfaceClass =	USB_CLASS_APP_SPEC,
> +	.bInterfaceSubClass =	1,
> +	.bInterfaceProtocol =	1,
> +	.iInterface =		STRING_DFU_NAME,
> +};
> +
> +static const struct usb_descriptor_header *dfu_function_runtime[] = {
> +	(struct usb_descriptor_header *) &otg_descriptor,
> +	(struct usb_descriptor_header *) &dfu_func,
> +	(struct usb_descriptor_header *) &dfu_intf_runtime,
> +	NULL,
> +};
> +
> +static struct usb_qualifier_descriptor dev_qualifier = {
> +	.bLength =		sizeof dev_qualifier,
> +	.bDescriptorType =	USB_DT_DEVICE_QUALIFIER,
> +	.bcdUSB =		__constant_cpu_to_le16(0x0200),
> +	.bDeviceClass =		USB_CLASS_VENDOR_SPEC,
> +	.bNumConfigurations =	1,
> +};
> +
> +static char manufacturer[50];
> +static const char longname[] = "DFU Gadget";
> +/* default serial number takes at least two packets */
> +static const char serial[] = "0123456789.0123456789.012345678";
> +static const char dfu_name[] = "Device Firmware Upgrade";
> +static const char shortname[] = "dfu";

This surely should be come from the board configuration?

> +
> +/* static strings, in UTF-8 */
> +static struct usb_string strings_runtime[] = {
> +	{ STRING_MANUFACTURER, manufacturer, },
> +	{ STRING_PRODUCT, longname, },
> +	{ STRING_SERIAL, serial, },
> +	{ STRING_DFU_NAME, dfu_name, },
> +	{  }			/* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_runtime = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_runtime,
> +};
> +
> +static struct usb_gadget_strings stringtab_dfu = {
> +	.language	= 0x0409,	/* en-us */
> +};
> +
> +static bool is_runtime(struct dfu_dev *dev)
> +{
> +	return dev->dfu_state == DFU_STATE_appIDLE ||
> +		dev->dfu_state == DFU_STATE_appDETACH;
> +}

Hmm, here you are checking if you are in run-time mode. In the
introduction you wrote that it is in DFU mode when the dfu command is
executed. When does it enter the run-time mode? After the first
flashing?

> +static int config_buf(struct usb_gadget *gadget,
> +		u8 *buf, u8 type, unsigned index)
> +{
> +	int hs = 0;
> +	struct dfu_dev *dev = get_gadget_data(gadget);
> +	const struct usb_descriptor_header **function;
> +	int len;
> +
> +	if (index > 0)
> +		return -EINVAL;
> +
> +	if (gadget_is_dualspeed(gadget)) {
> +		hs = (gadget->speed == USB_SPEED_HIGH);
> +		if (type == USB_DT_OTHER_SPEED_CONFIG)
> +			hs = !hs;
> +	}
> +	if (is_runtime(dev))
> +		function = dfu_function_runtime;
> +	else
> +		function = (const struct usb_descriptor_header **)dev->function;
> +
> +	/* for now, don't advertise srp-only devices */
> +	if (!gadget_is_otg(gadget))
> +		function++;
> +
> +	len = usb_gadget_config_buf(&dfu_config,
> +			buf, USB_BUFSIZ, function);
> +	if (len < 0)
> +		return len;
> +	((struct usb_config_descriptor *) buf)->bDescriptorType = type;
> +	return len;
> +}
> +
> +static void dfu_reset_config(struct dfu_dev *dev)
> +{
> +	if (dev->config == 0)
> +		return;
> +
> +	DBG(dev, "reset config\n");
> +
> +	dev->config = 0;
> +}
> +
> +static int dfu_set_config(struct dfu_dev *dev, unsigned number)
> +{
> +	int result = 0;
> +	struct usb_gadget *gadget = dev->gadget;
> +	char *speed;
> +
> +	if (number == dev->config)
> +		return 0;
> +
> +	dfu_reset_config(dev);
> +
> +	if (DFU_CONFIG_VAL != number) {
> +		result = -EINVAL;
> +		return result;
> +	}
> +
> +	switch (gadget->speed) {
> +	case USB_SPEED_LOW:
> +		speed = "low";
> +		break;
> +	case USB_SPEED_FULL:
> +		speed = "full";
> +		break;
> +	case USB_SPEED_HIGH:
> +		speed = "high";
> +		break;
> +	default:
> +		speed = "?";
> +		break;
> +	}
> +
> +	dev->config = number;
> +	INFO(dev, "%s speed config #%d: %s\n", speed, number, dfu_name);
> +	return result;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static void empty_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	/* intentionally empty */
> +}
> +
> +static void dnload_request_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct dfu_dev *dev = req->context;
> +	struct flash_entity *fe = &flash_ents[dev->altsetting];
> +
> +	if (dev->not_prepared) {
> +		printf("DOWNLOAD %s\n", fe->name);
> +		fe->prepare(fe->ctx, FLASH_WRITE);
> +		dev->not_prepared = false;
> +	}
> +
> +	if (req->length > 0)
> +		fe->write_block(fe->ctx, req->length, req->buf);
> +	else {
> +		fe->finish(fe->ctx, FLASH_WRITE);
> +		dev->not_prepared = true;
> +	}
> +}
> +
> +static void handle_getstatus(struct usb_request *req)
> +{
> +	struct dfu_status *dstat = (struct dfu_status *)req->buf;
> +	struct dfu_dev *dev = req->context;
> +
> +	switch (dev->dfu_state) {
> +	case DFU_STATE_dfuDNLOAD_SYNC:
> +	case DFU_STATE_dfuDNBUSY:
> +		dev->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
> +		break;
> +	case DFU_STATE_dfuMANIFEST_SYNC:
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* send status response */
> +	dstat->bStatus = dev->dfu_status;
> +	/* FIXME: set dstat->bwPollTimeout */

This really needs to be set. It was a nightmare with dfu-util not
having it set with the initial u-boot implementation. setting the
correct values here is not that easy though. It depends on the flash
layer and if can get the information about the maximal time a flash
write can take.

> +	dstat->bState = dev->dfu_state;
> +	dstat->iString = 0;
> +}
> +
> +static void handle_getstate(struct usb_request *req)
> +{
> +	struct dfu_dev *dev = req->context;
> +
> +	((u8 *)req->buf)[0] = dev->dfu_state & 0xff;
> +	req->actual = sizeof(u8);
> +}
> +
> +static int handle_upload(struct usb_request *req, u16 len)
> +{
> +	struct dfu_dev *dev = req->context;
> +	struct flash_entity *fe = &flash_ents[dev->altsetting];
> +	int n;
> +
> +	if (dev->not_prepared) {
> +		printf("UPLOAD %s\n", fe->name);
> +		fe->prepare(fe->ctx, FLASH_READ);
> +		dev->not_prepared = false;
> +	}
> +	n = fe->read_block(fe->ctx, len, req->buf);
> +
> +	/* no more data to read from this entity */
> +	if (n < len) {
> +		fe->finish(fe->ctx, FLASH_READ);
> +		dev->not_prepared = true;
> +	}
> +
> +	return n;
> +}
> +
> +static int handle_dnload(struct usb_gadget *gadget, u16 len)
> +{
> +	struct dfu_dev *dev = get_gadget_data(gadget);
> +	struct usb_request *req = dev->req;
> +
> +	if (len == 0)
> +		dev->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
> +
> +	req->complete = dnload_request_complete;
> +
> +	return len;
> +}
> +
> +static int
> +dfu_handle(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> +{
> +	struct dfu_dev *dev = get_gadget_data(gadget);
> +	struct usb_request *req = dev->req;
> +	u16 len = le16_to_cpu(ctrl->wLength);
> +	int rc = 0;
> +
> +	switch (dev->dfu_state) {
> +	case DFU_STATE_appIDLE:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_GETSTATUS:
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		case USB_REQ_DFU_DETACH:
> +			dev->dfu_state = DFU_STATE_appDETACH;
> +			dev->dfu_state = DFU_STATE_dfuIDLE;
> +			return RET_ZLP;
> +		default:
> +			return RET_STALL;
> +		}
> +		break;
> +	case DFU_STATE_appDETACH:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_GETSTATUS:
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		default:
> +			dev->dfu_state = DFU_STATE_appIDLE;
> +			return RET_STALL;
> +		}
> +		/* FIXME: implement timer to return to appIDLE */
> +		break;
> +	case DFU_STATE_dfuIDLE:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_DNLOAD:
> +			if (len == 0) {
> +				dev->dfu_state = DFU_STATE_dfuERROR;
> +				return RET_STALL;
> +			}
> +			dev->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
> +			return handle_dnload(gadget, len);
> +		case USB_REQ_DFU_UPLOAD:
> +			dev->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
> +			return handle_upload(req, len);
> +		case USB_REQ_DFU_ABORT:
> +			/* no zlp? */
> +			return RET_ZLP;
> +		case USB_REQ_DFU_GETSTATUS:
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		case USB_REQ_DFU_DETACH:
> +			/* Proprietary extension: 'detach' from idle mode and
> +			 * get back to runtime mode in case of USB Reset.  As
> +			 * much as I dislike this, we just can't use every USB
> +			 * bus reset to switch back to runtime mode, since at
> +			 * least the Linux USB stack likes to send a number of
> +			 * resets in a row :(
> +			 */

OK, this makes it clear that you took the DFU state machine from
Haralds implementation I'm also using. :)

It would be nice if the related copyright holder would be stated in
the header of the file.

> +			dev->dfu_state = DFU_STATE_dfuMANIFEST_WAIT_RST;
> +			dev->dfu_state = DFU_STATE_appIDLE;
> +			break;
> +		default:
> +			dev->dfu_state = DFU_STATE_dfuERROR;
> +			return RET_STALL;
> +		}
> +		break;
> +	case DFU_STATE_dfuDNLOAD_SYNC:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_GETSTATUS:
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +			/* FIXME: state transition depending
> +			 * on block completeness */
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		default:
> +			dev->dfu_state = DFU_STATE_dfuERROR;
> +			return RET_STALL;
> +		}
> +		break;
> +	case DFU_STATE_dfuDNBUSY:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_GETSTATUS:
> +			/* FIXME: only accept getstatus if bwPollTimeout
> +			 * has elapsed */
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		default:
> +			dev->dfu_state = DFU_STATE_dfuERROR;
> +			return RET_STALL;
> +		}
> +		break;
> +	case DFU_STATE_dfuDNLOAD_IDLE:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_DNLOAD:
> +			dev->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
> +			return handle_dnload(gadget, len);
> +		case USB_REQ_DFU_ABORT:
> +			dev->dfu_state = DFU_STATE_dfuIDLE;
> +			return RET_ZLP;
> +		case USB_REQ_DFU_GETSTATUS:
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		default:
> +			dev->dfu_state = DFU_STATE_dfuERROR;
> +			return RET_STALL;
> +		}
> +		break;
> +	case DFU_STATE_dfuMANIFEST_SYNC:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_GETSTATUS:
> +			/* We're MainfestationTolerant */
> +			dev->dfu_state = DFU_STATE_dfuIDLE;
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		default:
> +			dev->dfu_state = DFU_STATE_dfuERROR;
> +			return RET_STALL;
> +		}
> +		break;
> +	case DFU_STATE_dfuMANIFEST:
> +		/* we should never go here */
> +		dev->dfu_state = DFU_STATE_dfuERROR;
> +		return RET_STALL;
> +	case DFU_STATE_dfuMANIFEST_WAIT_RST:
> +		/* we should never go here */
> +		break;
> +	case DFU_STATE_dfuUPLOAD_IDLE:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_UPLOAD:
> +			/* state transition if less data then requested */
> +			rc = handle_upload(req, len);
> +			if (rc >= 0 && rc < len)
> +				dev->dfu_state = DFU_STATE_dfuIDLE;
> +			return rc;
> +		case USB_REQ_DFU_ABORT:
> +			dev->dfu_state = DFU_STATE_dfuIDLE;
> +			/* no zlp? */
> +			return RET_ZLP;
> +		case USB_REQ_DFU_GETSTATUS:
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		default:
> +			dev->dfu_state = DFU_STATE_dfuERROR;
> +			return RET_STALL;
> +		}
> +		break;
> +	case DFU_STATE_dfuERROR:
> +		switch (ctrl->bRequest) {
> +		case USB_REQ_DFU_GETSTATUS:
> +			handle_getstatus(req);
> +			return RET_STAT_LEN;
> +		case USB_REQ_DFU_GETSTATE:
> +			handle_getstate(req);
> +			break;
> +		case USB_REQ_DFU_CLRSTATUS:
> +			dev->dfu_state = DFU_STATE_dfuIDLE;
> +			dev->dfu_status = DFU_STATUS_OK;
> +			/* no zlp? */
> +			return RET_ZLP;
> +		default:
> +			dev->dfu_state = DFU_STATE_dfuERROR;
> +			return RET_STALL;
> +		}
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	return 0;
> +}

[snap]

> +int __init dfu_init(void)
> +{
> +	return usb_gadget_register_driver(&dfu_driver);
> +}

All this is absed on the re-written gadget layer? It loks as if all
kind of kernel stuff is brought over here. Is this gadget layer
already merged into u-boot mainline?

> diff --git a/drivers/usb/gadget/dfu.h b/drivers/usb/gadget/dfu.h
> new file mode 100644
> index 0000000..5a6386e
> --- /dev/null
> +++ b/drivers/usb/gadget/dfu.h
> @@ -0,0 +1,171 @@
> +/*
> + * dfu.h -- Device Firmware Update gadget
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef DFU_H_
> +#define DFU_H_
> +
> +#include <linux/compiler.h>
> +
> +/*
> + * Linux kernel compatibility layer
> + */
> +#define GFP_ATOMIC				((gfp_t) 0)
> +#define GFP_KERNEL				((gfp_t) 0)
> +#define true					1
> +#define false					0
> +#define dev_dbg(...)				do {} while (0)
> +#define dev_vdbg(...)				do {} while (0)
> +#define dev_err(...)				do {} while (0)
> +#define dev_warn(...)				do {} while (0)
> +#define dev_info(...)				do {} while (0)
> +#define pr_warning(...)				do {} while (0)
> +#define spin_lock_init(lock)			do {} while (0)
> +#define spin_lock(lock)				do {} while (0)
> +#define spin_unlock(lock)			do {} while (0)
> +#define spin_lock_irqsave(lock,flags)		do {flags = 1;} while (0)
> +#define spin_unlock_irqrestore(lock,flags)	do {flags = 0;} while (0)
> +#define kmalloc(x,y)				malloc(x)
> +#define kfree(x)				free(x)
> +#define kzalloc(size,flags)			calloc((size), 1)
> +#define __init
> +#define __exit
> +#define __exit_p(x)				x
> +#define module_init(...)
> +#define module_exit(...)
> +#define min_t					min
> +#define spinlock_t				int
> +#define bool					int
> +/*
> + * end compatibility layer
> + */

Urgs, is this all needed? Do we really want to have all this around.
This is not the kernel. U-Boots debug infrastrcuture should be used.

> +#define DBG(d, fmt, args...) \
> +	dev_dbg(&(d)->gadget->dev , fmt , ## args)
> +#define VDBG(d, fmt, args...) \
> +	dev_vdbg(&(d)->gadget->dev , fmt , ## args)
> +#define ERROR(d, fmt, args...) \
> +	dev_err(&(d)->gadget->dev , fmt , ## args)
> +#define WARN(d, fmt, args...) \
> +	dev_warn(&(d)->gadget->dev , fmt , ## args)
> +#define INFO(d, fmt, args...) \
> +	dev_info(&(d)->gadget->dev , fmt , ## args)
> +
> +#define DRIVER_VERSION			"Marzanna"

Maybe that helps you internal but it does not make much sense for
people outside Samsung. :)

It was good to see that the DFU state machine was taken from Haralds
patches. That should make it a lot easier to find a common ground
between the implementations. And I agree that splitting out the DFU
protocol from the flashing parts makes sense.

I would like to get some initila feedback from Remy if this should be
based on the new gadget stuff or not.

How would you like to proceed in getting our stuff merged and finally
integrated into mainline?

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 14:29 ` Stefan Schmidt
@ 2011-11-03  8:12   ` Andrzej Pietrasiewicz
  2011-11-03 14:01     ` Stefan Schmidt
  0 siblings, 1 reply; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-03  8:12 UTC (permalink / raw)
  To: u-boot

Hello Stefan,

On Wednesday, November 02, 2011 3:29 PM Stefan Shmidt wrote:

> 
> Have you fully implemented 1.1? With the detahc logic inside the
> device implementation?

As you noticed in another post, the state machine is reused.

> Just curious. What version of dfu-util your are using for your tests?
> I released 0.5 earlier today.

I used what was available in my desktop's Ubuntu 10.04 LTS, and
it happens to be version 0.1+svn :O It works, though.

> Hmm, that sounds like you only implemented the DFU mode but not the
> run-time mode yet?

This conclusion is not correct. After issuing the dfu command
my implementation begins in runtime mode. Only after using dfu-util
from the host is the mode changed to DFU mode.
Actually, as far as usage of dfu-util is concerned, I find it
inconvenient when the device is in runtime mode, because there
(at least with my version of dfu-util) is no way for the user
to guess what altsettings are actually available. I personally
do dfu-util -U <file> -a 0 or a similar command to kick the device
into DFU mode, then dfu-util -l to discover what altsettings there are.
So, it could be tempting to omit the runtime mode at all and start
in DFU mode, but, to be standard compliant, I start in runtime mode.
How do you discover with dfu-util what altsettings there are in the
device?

> While the original purpose
> of DFU was to make updating USB device easy enough for end-users as
> well.

True, that's what the standard says about the very purpose of DFU.
However, pressing keys to bring the device into a specific mode is
probably very much device-specific. It would be nice if we
are able to come up with a generic solution which only delegates
the necessary details to board-specific files.

> In the end I would like to see different options available:
> 
> 1) Entering DFU mode directly when a button is pressed on startup. An
> update mode if you want to call it like this.

Please see my comment above.

> 
> 2) Normal startup, but with DFU descripton to allow dfu-util detaching
> and entering the DFU mode for flashing.
> 
> 3) Starting dfu mode form the command line as you did implement it.
> 

I think 2 and 3 don't differ too much - it seems that enriching a
setup without DFU with additional USB descriptors will do the job.
But 2 and 3 probably should not be merged, though.

> > The implementation is split into two parts: the dfu gadget
> implementation
> > and a "flashing backend", the interface between the two being the
> > struct flash_entity. The "flashing backend"'s implementation is
> > board-specific and is implemented on the Goni target.
> 
> What is your reasoning behind this split? I have it working here with
> the normal nand_flashing routines taking care of bad blocks, etc.

The reason is to factor out board-specific stuff from the dfu gadget
implementation. However, in what I factored out probably there are
parts which are generic enough not to be board-specific. Since there
seems to be considerable interest in DFU implementation for u-boot,
this could be reworked.

> To make it clear, I'm happy to see somebody else working on this

"Whassssuuuup! --- Oh, man, we are not alone!" ;) I am happy to
hear from someone else who works on this, too.

Regards,

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 15:16 ` Mike Frysinger
  2011-11-02 19:30   ` Stefan Schmidt
@ 2011-11-03  8:17   ` Andrzej Pietrasiewicz
  1 sibling, 0 replies; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-03  8:17 UTC (permalink / raw)
  To: u-boot

Hello Mike,

Thank you for your review. Please see my comments inline.

On Wednesday, November 02, 2011 4:16 PM Mike Frysinger wrote:

> >
> > Dear All,
> >
> > This is Device Firmware Upgrade (DFU) implementation which supports
> > data upload and download function to devices which are equipped with
> a UDC.
> 
> this information belongs in the changelog (above the "---" marker)
> 
I generally agree to remarks related to coding style and implementation's
distribution into respective patches.

> are you working with the elinux.org guys ?
> 	http://elinux.org/Merge_DFU_support_into_mainline_U-Boot
>
That's not me.

> 
> this should be split up into at least the dfu core and board-specific
> changes.
> although i'd wonder how much of the board/samsung/ stuff is really
> board specific and couldn't be generalized ...

You are right, probably the "flashing backend" part contains
some code which can be generalized.

Regards,

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 19:30   ` Stefan Schmidt
@ 2011-11-03  8:44     ` Andrzej Pietrasiewicz
  2011-11-03 14:06       ` Stefan Schmidt
  0 siblings, 1 reply; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-03  8:44 UTC (permalink / raw)
  To: u-boot

Hello Stefan,

On Wednesday, November 02, 2011 8:30 PM Stefan Schmidt writes:

> 
> Agreed. The eMMC flashing with files on FAT is nothing goni specific.
> Others should be able to use this as well. I see three different parts
> here that can be separated:

I agree. Since there is interest in DFU implementation this can and should
be generalized.

> 
> 1) The DFU protocol implementation which lives in usb gadget with an
> interface for flashing backends
> 
> 2) The flashing backends (no idea where to put them best). At the
> moment that would be the implementation here with eMMC and files on FAT,
> mine with raw NAND devices and in the future maybe others. Each
> flashing backend should be generic enough to allow different boards
> using it.
> 
> 3) Board specific information like partitions or hints for the flashing
> backend. The information itself should be in the board file here and
> only used by the flashing backends.
> 
> How does this sound to you? Andrzej?

Sounds good to me. In my implementation the interface between dfu gadget
and flashing backend is around the struct flash_entity.
It contains a character string intended to provide a human-readable name,
a void * context which is not interpreted by the gadget code,
but is passed to the flashing backend and understood by it.
The struct flash_entity also contains prepare-finish methods
to be called before and after read/write operations, and the read_block-
write_block pair. What do you think?

As far as generalization is concerned, in my flashing backend
implementation I see these parts as candidates for generalization:

1) mbr/ebr reading
2) reading/writing mmc
3) read/write fat
4) generic prepare/finish; not sure if fat-specific prepare can be
generalized
5) read/write_block
6) some more work should be put to create an interface between the board
initialization routine, the flashing backend and the DFU gadget
implementation.
In my implementation the board initialization routine calls board-specific
register_flash_areas, which, in turn, calls DFU gadget's
register_flash_entities.
What's your opinion?

Thanks,

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 20:07 ` Stefan Schmidt
@ 2011-11-03 10:33   ` Andrzej Pietrasiewicz
  2011-11-03 14:24     ` Stefan Schmidt
  2011-11-03 12:47   ` Andrzej Pietrasiewicz
  2011-11-05 15:31   ` Wolfgang Denk
  2 siblings, 1 reply; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-03 10:33 UTC (permalink / raw)
  To: u-boot

Hello Stefan,

Thank you for your review. Please see comments inline.

On Wednesday, November 02, 2011 9:07 PM Stefan Schmidt wrote:

> 
> First, and only high level, review for the DFU part.
> 
> Against which u-boot tree/branch/version is this patch? I tried to
> apply it against HEAD and it failed for me. Anything I miss?
> 
Sorry about that. I forgot to mention the reference. It is
http://patchwork.ozlabs.org/patch/122080

> On Wed, 2011-11-02 at 11:00, Andrzej Pietrasiewicz wrote:
> >
> > The implementation is split into two parts: the dfu gadget
> implementation
> > and a "flashing backend", the interface between the two being the
> > struct flash_entity. The "flashing backend"'s implementation is
> > board-specific and is implemented on the Goni target.
> 
> I replied to Mike's mail with my ideas on this. Split between dfu and
> flashing backends is good and missng in my approach currently. I would
> not put it into the board file though but make it generic for other
> boards as well and only define the needed informations in the board
> file. Please see my other mail for details. Comments appreciated!

Comments sent in other mails;) I agree that since there is interest
in implementing DFU in u-boot the code which in fact is not board
specific can and should be generalized. We need to think of an
appropriate place in the tree to store it, though.

> 
> > diff --git a/common/Makefile b/common/Makefile
> > index ae795e0..de926d9 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -162,6 +162,7 @@ COBJS-y += cmd_usb.o
> >  COBJS-y += usb.o
> >  COBJS-$(CONFIG_USB_STORAGE) += usb_storage.o
> >  endif
> > +COBJS-$(CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE) += cmd_dfu.o
> 
> CONFIG_CMD_DFU instead? Looks a bit long to me.

ACK. No problem with changing that.

> 
> I already wrote a bit about my approach here. At OpenMoko we used a
> button to enter u-boot during startup when pressed. This offered a
> usbtty interface as usb gadget as well as the runtime descripto for
> DFU. With dfu-util it was possible to iniate the DFU download or
> upload procedure while being in the mode. Another option would be to
> directly jump into DFU mode when a button is pressed.

Please see my comment in the other mail. A generic way to handle
"a button pressed" seems necessary. Actually in my opinion just
providing the "dfu" command is enough. If, in some hardware,
initiating the runtime mode is desired by pressing some keys, then
the board initialization code should check for such an event and
if it happens then just programmatically run the dfu command.
What do you think?

> > +/* #include "usbstring.c" */
> 
> Not needed, can be removed.

I agree to coding style/reduntant code comments; will not
specifically comment on them unless I disagree.
 
> > +static const struct dfu_function_descriptor dfu_func = {
> > +	.bLength =		sizeof dfu_func,
> > +	.bDescriptorType =	DFU_DT_FUNC,
> > +	.bmAttributes =		DFU_BIT_WILL_DETACH |
> 
> Here are you setting the WILL_DETACH bit. Is the device really
> detaching itself? Its not obvious to me from the code that it does.

It seems it does not. The dfu-util session says "Resetting USB", so I
assume that resetting is initiated by the host, not by the device.
To be removed.

> > +static char manufacturer[50];
> > +static const char longname[] = "DFU Gadget";
> > +/* default serial number takes at least two packets */
> > +static const char serial[] = "0123456789.0123456789.012345678";
> > +static const char dfu_name[] = "Device Firmware Upgrade";
> > +static const char shortname[] = "dfu";
> 
> This surely should be come from the board configuration?

Yes and no - depends on whether we want to version the gadget itself
or the flashing backend, or both. As probably the last option
is best, the answer is probably "yes". We need some interface then.

> > +static bool is_runtime(struct dfu_dev *dev)
> > +{
> > +	return dev->dfu_state == DFU_STATE_appIDLE ||
> > +		dev->dfu_state == DFU_STATE_appDETACH;
> > +}
> 
> Hmm, here you are checking if you are in run-time mode. In the
> introduction you wrote that it is in DFU mode when the dfu command is
> executed. When does it enter the run-time mode? After the first
> flashing?

I think this is over-interpretation of what I wrote in introduction.
I gave an example dfu-util session when the device is already in DFU
mode. I didn't say that it only has DFU mode. It has both.

> > +	/* send status response */
> > +	dstat->bStatus = dev->dfu_status;
> > +	/* FIXME: set dstat->bwPollTimeout */
> 
> This really needs to be set. It was a nightmare with dfu-util not
> having it set with the initial u-boot implementation. setting the
> correct values here is not that easy though. It depends on the flash
> layer and if can get the information about the maximal time a flash
> write can take.

Not sure what to do at this moment, either.

> > +		case USB_REQ_DFU_DETACH:
> > +			/* Proprietary extension: 'detach' from idle mode
and
> > +			 * get back to runtime mode in case of USB Reset.
As
> > +			 * much as I dislike this, we just can't use every
> USB
> > +			 * bus reset to switch back to runtime mode, since
at
> > +			 * least the Linux USB stack likes to send a number
> of
> > +			 * resets in a row :(
> > +			 */
> 
> OK, this makes it clear that you took the DFU state machine from
> Haralds implementation I'm also using. :)
> 
> It would be nice if the related copyright holder would be stated in
> the header of the file.

You are absolutely right. My gadget implementation is also based on
Gadget Zero from Linux and I did mention that, but missed including
appropriate information regarding the DFU state machine.

> 
> > +int __init dfu_init(void)
> > +{
> > +	return usb_gadget_register_driver(&dfu_driver);
> > +}
> 
> All this is absed on the re-written gadget layer? It loks as if all
> kind of kernel stuff is brought over here. Is this gadget layer
> already merged into u-boot mainline?

I think that the gadget framework works well and can be successfully
reused in u-boot. What other interface would you see to talk to UDC?
Also, USB mass storage implementation can be easy done with usb gadget.

Actually, the original intention was to implement the dfu both in u-boot
and in kernel. I did post my kernel implementation to linux-usb and
did give example of being useful. Even though this kind of a driver
could seem controversial, there are other drivers based on similar
philosophy
which are in the mainline kernel. Anyway, Felipe Balbi, the usb maintainer
in
Linux, did not approve because he says this kind of things belongs
to userspace. Still, I see the gadget framework suitable
to do the job for us here.

> > +/*
> > + * end compatibility layer
> > + */
> > 
> Urgs, is this all needed? Do we really want to have all this around.
> This is not the kernel. U-Boots debug infrastrcuture should be used.

Please see the comment above. In such circumstances, we probably don't.

> > +#define DRIVER_VERSION			"Marzanna"
> 
> Maybe that helps you internal but it does not make much sense for
> people outside Samsung. :)
> 

The code I started to work on had an equally nice name here, which
was like "Victory Day" or something. "Marzanna" is one of female
names in Polish and it was Marzanna's name day when I started work
on dfu. Self explanatory ;)

> It was good to see that the DFU state machine was taken from Haralds
> patches. That should make it a lot easier to find a common ground
> between the implementations. And I agree that splitting out the DFU
> protocol from the flashing parts makes sense.
> 

Absolutely yes.

> How would you like to proceed in getting our stuff merged and finally
> integrated into mainline?
> 

You mentioned that your DFU implementation does things which mine doesn't.
Can you please tell something more? As far as I understand what you wrote,
there is something in your DFU implementation which does more, and also we
use different media as backends.

Thank you,

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 20:07 ` Stefan Schmidt
  2011-11-03 10:33   ` Andrzej Pietrasiewicz
@ 2011-11-03 12:47   ` Andrzej Pietrasiewicz
  2011-11-03 13:31     ` Stefan Schmidt
  2011-11-05 15:31   ` Wolfgang Denk
  2 siblings, 1 reply; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-03 12:47 UTC (permalink / raw)
  To: u-boot

On Thursday, November 03, 2011 11:33 AM Andrzej Pietrasiewicz wrote:

> >
> Sorry about that. I forgot to mention the reference. It is
> http://patchwork.ozlabs.org/patch/122080
> 
Sorry again. I meant http://patchwork.ozlabs.org/patch/122079

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-03 12:47   ` Andrzej Pietrasiewicz
@ 2011-11-03 13:31     ` Stefan Schmidt
  2011-11-03 14:11       ` Andrzej Pietrasiewicz
  0 siblings, 1 reply; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-03 13:31 UTC (permalink / raw)
  To: u-boot

Hello.

On Thu, 2011-11-03 at 13:47, Andrzej Pietrasiewicz wrote:
> On Thursday, November 03, 2011 11:33 AM Andrzej Pietrasiewicz wrote:
> 
> > Sorry about that. I forgot to mention the reference. It is
> > http://patchwork.ozlabs.org/patch/122080
> > 
> Sorry again. I meant http://patchwork.ozlabs.org/patch/122079

Hmm, applied this one but the DFU patch stills fails to apply in
goni.c and goni.h. Seems there is something else applied that is not
in mainline yet.

I can't test the goni part anyway as I don't have the hardware so I
will split it out of the patch and test the dfu command and its
implementation.

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-03  8:12   ` Andrzej Pietrasiewicz
@ 2011-11-03 14:01     ` Stefan Schmidt
  2011-11-04 13:14       ` Andrzej Pietrasiewicz
  0 siblings, 1 reply; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-03 14:01 UTC (permalink / raw)
  To: u-boot

Hello.

On Thu, 2011-11-03 at 09:12, Andrzej Pietrasiewicz wrote:
> On Wednesday, November 02, 2011 3:29 PM Stefan Shmidt wrote:
> > 
> > Have you fully implemented 1.1? With the detahc logic inside the
> > device implementation?
> 
> As you noticed in another post, the state machine is reused.

Yes, sorry about this. I was fouled into thinking you did not as it
was splitted out and no mentioning of Harald copyright. Having the
same state machine is a good base for further cooperation on this. :)

> > Just curious. What version of dfu-util your are using for your tests?
> > I released 0.5 earlier today.
> 
> I used what was available in my desktop's Ubuntu 10.04 LTS, and
> it happens to be version 0.1+svn :O It works, though.

Ah, latest Ubuntu has 0.3 and hopefully they will update as well.
Already pinged the maintainer.

> > Hmm, that sounds like you only implemented the DFU mode but not the
> > run-time mode yet?
> 
> This conclusion is not correct.

Indeed. I stand corrected.

> After issuing the dfu command
> my implementation begins in runtime mode. Only after using dfu-util
> from the host is the mode changed to DFU mode.
> Actually, as far as usage of dfu-util is concerned, I find it
> inconvenient when the device is in runtime mode, because there
> (at least with my version of dfu-util) is no way for the user
> to guess what altsettings are actually available.

Correct. Sadly this is due to the standard only allowing the
functional descriptor in runtime mode. On the other hand the exposed
alt settings in run-time mode would clutter the USB device settings.

> I personally
> do dfu-util -U <file> -a 0 or a similar command to kick the device
> into DFU mode, then dfu-util -l to discover what altsettings there are.

I did run exactly in the same problem. :) I added a detach command to
dfu-util to switch between the different modes. From run-time to dfu
and from dfu to run-time. I wanted to get the release out before
bringing it in. Pushed it to the master branch of dfu-util now. Feel
free to test and let me know if it works for you.

> So, it could be tempting to omit the runtime mode at all and start
> in DFU mode, but, to be standard compliant, I start in runtime mode.
> How do you discover with dfu-util what altsettings there are in the
> device?

See above. Not available in run-time mode. :/

> > While the original purpose
> > of DFU was to make updating USB device easy enough for end-users as
> > well.
> 
> True, that's what the standard says about the very purpose of DFU.
> However, pressing keys to bring the device into a specific mode is
> probably very much device-specific. It would be nice if we
> are able to come up with a generic solution which only delegates
> the necessary details to board-specific files.

Agreed. This should not be handled in the dfu implementation at all.
Your split between dfu and flashing backend is a good step in the
right direction here. Invoking the dfu mode through a command is also
a nice addition.

> > > The implementation is split into two parts: the dfu gadget
> > implementation
> > > and a "flashing backend", the interface between the two being the
> > > struct flash_entity. The "flashing backend"'s implementation is
> > > board-specific and is implemented on the Goni target.
> > 
> > What is your reasoning behind this split? I have it working here with
> > the normal nand_flashing routines taking care of bad blocks, etc.
> 
> The reason is to factor out board-specific stuff from the dfu gadget
> implementation. However, in what I factored out probably there are
> parts which are generic enough not to be board-specific. Since there
> seems to be considerable interest in DFU implementation for u-boot,
> this could be reworked.

After thinking more about it I agree with you here. The split makes
sense and it will allow for more flexibility when adding more flashing
backend to the implementation.

> > To make it clear, I'm happy to see somebody else working on this
> 
> "Whassssuuuup! --- Oh, man, we are not alone!" ;) I am happy to
> hear from someone else who works on this, too.

Good to know. :)

Brainstorming about a way going forward. Please comment if with your
view on this.

o I will send out my not ready for mainline patch to give you and
  others an impression how it is tackled in my patch.

o I like your split between dfu and flashing and also the addition of
  the dfu command. Could you split this part from the rest and send it
  as self conatining patch without other dependencies?

o And then one patch with the mmc/fat flashing backend in moved out of
  the board file (no idea what the best place is, maybe just another
  file for now) and a last patch with all the board specific changes?

o I will then rebase my code on yours and prepare patches against the
  dfu implementations as needed and also flashing backend for NAND.

o After this we need to review was is missing and bring it over.

Not all details covered in the plan but a start. Comments?

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-03  8:44     ` Andrzej Pietrasiewicz
@ 2011-11-03 14:06       ` Stefan Schmidt
  0 siblings, 0 replies; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-03 14:06 UTC (permalink / raw)
  To: u-boot

Hello.

On Thu, 2011-11-03 at 09:44, Andrzej Pietrasiewicz wrote:
> On Wednesday, November 02, 2011 8:30 PM Stefan Schmidt writes:
> > 
> > Agreed. The eMMC flashing with files on FAT is nothing goni specific.
> > Others should be able to use this as well. I see three different parts
> > here that can be separated:
> 
> I agree. Since there is interest in DFU implementation this can and should
> be generalized.

Great!

> > 1) The DFU protocol implementation which lives in usb gadget with an
> > interface for flashing backends
> > 
> > 2) The flashing backends (no idea where to put them best). At the
> > moment that would be the implementation here with eMMC and files on FAT,
> > mine with raw NAND devices and in the future maybe others. Each
> > flashing backend should be generic enough to allow different boards
> > using it.
> > 
> > 3) Board specific information like partitions or hints for the flashing
> > backend. The information itself should be in the board file here and
> > only used by the flashing backends.
> > 
> > How does this sound to you? Andrzej?
> 
> Sounds good to me. In my implementation the interface between dfu gadget
> and flashing backend is around the struct flash_entity.
> It contains a character string intended to provide a human-readable name,
> a void * context which is not interpreted by the gadget code,
> but is passed to the flashing backend and understood by it.
> The struct flash_entity also contains prepare-finish methods
> to be called before and after read/write operations, and the read_block-
> write_block pair. What do you think?

I would need to use it for my NAND backend before I really can comment
on it. I can only tell if I'm happy with and interface if I actually
used it. :)

Will do this when you send it a separate patch with only the DFU
implementation with the flash entity as interface. See my roadmap
proposal in the other mail.

> As far as generalization is concerned, in my flashing backend
> implementation I see these parts as candidates for generalization:
> 
> 1) mbr/ebr reading
> 2) reading/writing mmc
> 3) read/write fat

Agreed. That can be used by all kind of devices.

> 4) generic prepare/finish; not sure if fat-specific prepare can be
> generalized

Would be good to get soem comments on the fs custodians around here.

> 5) read/write_block

Agreed.

> 6) some more work should be put to create an interface between the board
> initialization routine, the flashing backend and the DFU gadget
> implementation.
> In my implementation the board initialization routine calls board-specific
> register_flash_areas, which, in turn, calls DFU gadget's
> register_flash_entities.
> What's your opinion?

The interface between the configuration in the board file and the
actual flashing backend is something I haven't wraped my mind around
yet. Need to think about it.

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-03 13:31     ` Stefan Schmidt
@ 2011-11-03 14:11       ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-03 14:11 UTC (permalink / raw)
  To: u-boot

Hello Stefan,

On Thursday, November 03, 2011 2:32 PM Stefan Schmidt wrote:

> >
> > > Sorry about that. I forgot to mention the reference. It is
> > > http://patchwork.ozlabs.org/patch/122080
> > >
> > Sorry again. I meant http://patchwork.ozlabs.org/patch/11122079
> 
> Hmm, applied this one but the DFU patch stills fails to apply in
> goni.c and goni.h. Seems there is something else applied that is not
> in mainline yet.

Ok, here is the full story: this http://patchwork.ozlabs.org/patch/11122079
is the second patch in a series, the first being
http://patchwork.ozlabs.org/patch/122080 and you actually need both.

I pulled the latest git.denx.de tree and had to also apply a patch which
Defines CONFIG_SYS_CACHELINE_SIZE for Goni target. It can be found
here: http://article.gmane.org/gmane.comp.boot-loaders.u-boot/112755/match=
I know this is confusing. Sorry about that.

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-03 10:33   ` Andrzej Pietrasiewicz
@ 2011-11-03 14:24     ` Stefan Schmidt
  0 siblings, 0 replies; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-03 14:24 UTC (permalink / raw)
  To: u-boot

Hello.

On Thu, 2011-11-03 at 11:33, Andrzej Pietrasiewicz wrote:
> On Wednesday, November 02, 2011 9:07 PM Stefan Schmidt wrote:
> > On Wed, 2011-11-02 at 11:00, Andrzej Pietrasiewicz wrote:
> > 
> > > diff --git a/common/Makefile b/common/Makefile
> > > index ae795e0..de926d9 100644
> > > --- a/common/Makefile
> > > +++ b/common/Makefile
> > > @@ -162,6 +162,7 @@ COBJS-y += cmd_usb.o
> > >  COBJS-y += usb.o
> > >  COBJS-$(CONFIG_USB_STORAGE) += usb_storage.o
> > >  endif
> > > +COBJS-$(CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE) += cmd_dfu.o
> > 
> > CONFIG_CMD_DFU instead? Looks a bit long to me.
> 
> ACK. No problem with changing that.

OK

> > I already wrote a bit about my approach here. At OpenMoko we used a
> > button to enter u-boot during startup when pressed. This offered a
> > usbtty interface as usb gadget as well as the runtime descripto for
> > DFU. With dfu-util it was possible to iniate the DFU download or
> > upload procedure while being in the mode. Another option would be to
> > directly jump into DFU mode when a button is pressed.
> 
> Please see my comment in the other mail. A generic way to handle
> "a button pressed" seems necessary. Actually in my opinion just
> providing the "dfu" command is enough. If, in some hardware,
> initiating the runtime mode is desired by pressing some keys, then
> the board initialization code should check for such an event and
> if it happens then just programmatically run the dfu command.
> What do you think?

Lets go with your dfu implementation and the dfu command. I will
prepare patches against is if I see something missing from my patch.

> > > +/* #include "usbstring.c" */
> > 
> > Not needed, can be removed.
> 
> I agree to coding style/reduntant code comments; will not
> specifically comment on them unless I disagree.

Thanks.

> > > +static const struct dfu_function_descriptor dfu_func = {
> > > +	.bLength =		sizeof dfu_func,
> > > +	.bDescriptorType =	DFU_DT_FUNC,
> > > +	.bmAttributes =		DFU_BIT_WILL_DETACH |
> > 
> > Here are you setting the WILL_DETACH bit. Is the device really
> > detaching itself? Its not obvious to me from the code that it does.
> 
> It seems it does not. The dfu-util session says "Resetting USB", so I
> assume that resetting is initiated by the host, not by the device.
> To be removed.

So far I also put this aside. Its only a 1.1 feature which can be
added with another patch when the basic stuff is working and merged.

> > > +static char manufacturer[50];
> > > +static const char longname[] = "DFU Gadget";
> > > +/* default serial number takes at least two packets */
> > > +static const char serial[] = "0123456789.0123456789.012345678";
> > > +static const char dfu_name[] = "Device Firmware Upgrade";
> > > +static const char shortname[] = "dfu";
> > 
> > This surely should be come from the board configuration?
> 
> Yes and no - depends on whether we want to version the gadget itself
> or the flashing backend, or both. As probably the last option
> is best, the answer is probably "yes". We need some interface then.

As I already stated this part is not worked out in my head yet. Feel
free to propose something or we will fix it while working on it.

> > > +static bool is_runtime(struct dfu_dev *dev)
> > > +{
> > > +	return dev->dfu_state == DFU_STATE_appIDLE ||
> > > +		dev->dfu_state == DFU_STATE_appDETACH;
> > > +}
> > 
> > Hmm, here you are checking if you are in run-time mode. In the
> > introduction you wrote that it is in DFU mode when the dfu command is
> > executed. When does it enter the run-time mode? After the first
> > flashing?
> 
> I think this is over-interpretation of what I wrote in introduction.
> I gave an example dfu-util session when the device is already in DFU
> mode. I didn't say that it only has DFU mode. It has both.

Yeah, sorry for that. I indeed misread this part of your
implementation.

> > > +	/* send status response */
> > > +	dstat->bStatus = dev->dfu_status;
> > > +	/* FIXME: set dstat->bwPollTimeout */
> > 
> > This really needs to be set. It was a nightmare with dfu-util not
> > having it set with the initial u-boot implementation. setting the
> > correct values here is not that easy though. It depends on the flash
> > layer and if can get the information about the maximal time a flash
> > write can take.
> 
> Not sure what to do at this moment, either.

If we don't have a back channel from the actual flashing subsystem we
need to oriantate the values on the hardware spec on own measurements
I fear. Soemthing that need to be set in the board file then.

> > > +		case USB_REQ_DFU_DETACH:
> > > +			/* Proprietary extension: 'detach' from idle mode
> and
> > > +			 * get back to runtime mode in case of USB Reset.
> As
> > > +			 * much as I dislike this, we just can't use every
> > USB
> > > +			 * bus reset to switch back to runtime mode, since
> at
> > > +			 * least the Linux USB stack likes to send a number
> > of
> > > +			 * resets in a row :(
> > > +			 */
> > 
> > OK, this makes it clear that you took the DFU state machine from
> > Haralds implementation I'm also using. :)
> > 
> > It would be nice if the related copyright holder would be stated in
> > the header of the file.
> 
> You are absolutely right. My gadget implementation is also based on
> Gadget Zero from Linux and I did mention that, but missed including
> appropriate information regarding the DFU state machine.

OK. Would be good to add this for the next round.

> > > +int __init dfu_init(void)
> > > +{
> > > +	return usb_gadget_register_driver(&dfu_driver);
> > > +}
> > 
> > All this is absed on the re-written gadget layer? It loks as if all
> > kind of kernel stuff is brought over here. Is this gadget layer
> > already merged into u-boot mainline?
> 
> I think that the gadget framework works well and can be successfully
> reused in u-boot. What other interface would you see to talk to UDC?
> Also, USB mass storage implementation can be easy done with usb gadget.

I don't wanted to argue against a good gadget framework. I was just
wondering if it already is in mainline and if not what is blocking it.

My patch has some additions to usbtty for the runtime descriptor  to
show up and all other parts are handled over ep0. The gadget framework
would make this easier and better abstracted to work toegther with
different usb gadget drivers. The only fear I have is to depend on
soemthing not yet in u-boot mainline will make it harder to get the
dfu implementation merged.

> > > +#define DRIVER_VERSION			"Marzanna"
> > 
> > Maybe that helps you internal but it does not make much sense for
> > people outside Samsung. :)
> > 
> 
> The code I started to work on had an equally nice name here, which
> was like "Victory Day" or something. "Marzanna" is one of female
> names in Polish and it was Marzanna's name day when I started work
> on dfu. Self explanatory ;)

Hehe :)

> > How would you like to proceed in getting our stuff merged and finally
> > integrated into mainline?
> 
> You mentioned that your DFU implementation does things which mine doesn't.
> Can you please tell something more? As far as I understand what you wrote,
> there is something in your DFU implementation which does more, and also we
> use different media as backends.

One thin was the runtime and dfu mode thing. I was wrong there as
yours also do both. For the flashing backend I have the alt settings
dynamically generated from available mtdparts and use the new nand
write functions skipping over bad blocks. For the core DFU protocol
I'm not sure about the differences yet. But as I said, prepare I stand
alone patch of yours and I will add send patches to add missing stuff
of the DFU implementation, if any.

My patch should be on the list later today or tomorrow to let you see
what I have.

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 10:00 [U-Boot] [PATCH] dfu: initial implementation Andrzej Pietrasiewicz
                   ` (2 preceding siblings ...)
  2011-11-02 20:07 ` Stefan Schmidt
@ 2011-11-03 18:13 ` Wolfgang Denk
  2011-11-04  6:52   ` Andrzej Pietrasiewicz
  3 siblings, 1 reply; 26+ messages in thread
From: Wolfgang Denk @ 2011-11-03 18:13 UTC (permalink / raw)
  To: u-boot

Dear Andrzej Pietrasiewicz,

In message <1320228007-8947-1-git-send-email-andrzej.p@samsung.com> you wrote:
>
> Device Firmware Upgrade (DFU) is an extension to the USB specification.
> As of the time of this writing it is documented at
> 
> http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
...
> Please refer to dfu-util documentation for details.
> The below ASCII-art presents a connection between the host and the device.
> 
>       +-----------------+    <--- DFU --->    +-------------------------+
>       |  e.g. dfu-util  |    <=== USB ===>    |          / altsetting 0 |
>       |          host   +---------------------+   device - altsetting 1 |
>       |   file /        |                     |          \     ...      |
>       +-----------------+                     +-------------------------+


Sorry for asking some probably really dumb questions...


If I understand correctly, there is no fundamental reason why the DFU
protocol would only be possible to implement over a USB link.  Or am I
missing something here?

Eventually it should be possible to run this protocol over Ethernet or
even over a serial line?

If my assumption is correct, then what would it take to split off
protocol part and make it independent of the actual driver interface?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Today is the yesterday you worried about tomorrow.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-03 18:13 ` Wolfgang Denk
@ 2011-11-04  6:52   ` Andrzej Pietrasiewicz
  2011-11-05 15:33     ` Wolfgang Denk
  0 siblings, 1 reply; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-04  6:52 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

Please see my comments inline.

On Thursday, November 03, 2011 7:14 PM Wolfgang Denk wrote:

> >
> > http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
> ...

DFU is part of USB; an extension to be precise, but an extension bound
so tightly to the design and philosophy of USB that it is rather
inconceivable to separate the two.

> 
> If I understand correctly, there is no fundamental reason why the DFU
> protocol would only be possible to implement over a USB link.  Or am I
> missing something here?
> 
> Eventually it should be possible to run this protocol over Ethernet or
> even over a serial line?

Of course there is no such a reason, provided we lay USB over Ethernet
or serial line first ;)
Seriously speaking, in view of ties between DFU and USB
IMHO it is impossible, or, at least, highly impractical.

> 
> If my assumption is correct, then what would it take to split off
> protocol part and make it independent of the actual driver interface?
> 

I guess that in the situation given it would be of little use.

Regards,

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-03 14:01     ` Stefan Schmidt
@ 2011-11-04 13:14       ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-04 13:14 UTC (permalink / raw)
  To: u-boot

Hello,

On Thursday, November 03, 2011 3:01 PM Stefan Schmidt wrote:

> 
> o I will send out my not ready for mainline patch to give you and
>   others an impression how it is tackled in my patch.
> 
> o I like your split between dfu and flashing and also the addition of
>   the dfu command. Could you split this part from the rest and send it
>   as self conatining patch without other dependencies?
> 
> o And then one patch with the mmc/fat flashing backend in moved out of
>   the board file (no idea what the best place is, maybe just another
>   file for now) and a last patch with all the board specific changes?


I assume splitting into 4 parts: (1) dfu gadget, (2) board specific code,
(3) flashing backend generic code, (4) dfu command.

As I today discovered, after latest merges in u-boot master the mmc
subsystem in Goni does not work properly - the detected capacity is 0,
and the number of available partitions is 0.
This took me some time to discover. I hope the responsible person will
fix this soon. So, as far as the splitting of my dfu implementation
into separate patches is concerned, I think I will come up with it
early next week.

> 
> o I will then rebase my code on yours and prepare patches against the
>   dfu implementations as needed and also flashing backend for NAND.
> 
> o After this we need to review was is missing and bring it over.
> 
> Not all details covered in the plan but a start. Comments?

In the beginning I intend to split the code into respective parts,
to give you something to work with. Then I intend to clean the code up,
taking into account your and Mike's review.

Regards,

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-02 20:07 ` Stefan Schmidt
  2011-11-03 10:33   ` Andrzej Pietrasiewicz
  2011-11-03 12:47   ` Andrzej Pietrasiewicz
@ 2011-11-05 15:31   ` Wolfgang Denk
  2011-11-06 16:36     ` Stefan Schmidt
  2 siblings, 1 reply; 26+ messages in thread
From: Wolfgang Denk @ 2011-11-05 15:31 UTC (permalink / raw)
  To: u-boot

Dear Stefan Schmidt,

In message <20111102200717.GP17069@excalibur.local> you wrote:
>
> While I think a dfu command is usefull I don't like the need to
> execute it before any DFU interaction can happen. That may be an
> option during development but for field upgrades or receovery it is
> not.

Yes, a command is the natural way in U-Boot to start any activities.

> I already wrote a bit about my approach here. At OpenMoko we used a
> button to enter u-boot during startup when pressed. This offered a

This is just another way to start a command.

> usbtty interface as usb gadget as well as the runtime descripto for
> DFU. With dfu-util it was possible to iniate the DFU download or
> upload procedure while being in the mode. Another option would be to
> directly jump into DFU mode when a button is pressed.

NAK.  Let's stick to standard interfaces.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Each honest calling, each walk of life, has its own  elite,  its  own
aristocracy based on excellence of performance. - James Bryant Conant

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-04  6:52   ` Andrzej Pietrasiewicz
@ 2011-11-05 15:33     ` Wolfgang Denk
  2011-11-06 17:02       ` Stefan Schmidt
  2011-11-07 12:12       ` Andrzej Pietrasiewicz
  0 siblings, 2 replies; 26+ messages in thread
From: Wolfgang Denk @ 2011-11-05 15:33 UTC (permalink / raw)
  To: u-boot

Dear Andrzej Pietrasiewicz,

In message <000601cc9abe$4f544bd0$edfce370$%p@samsung.com> you wrote:
> 
> > > http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
> 
> DFU is part of USB; an extension to be precise, but an extension bound
> so tightly to the design and philosophy of USB that it is rather
> inconceivable to separate the two.

Could you please be so kind and explain which exact issues you see for
such a separation?

> > Eventually it should be possible to run this protocol over Ethernet or
> > even over a serial line?
> 
> Of course there is no such a reason, provided we lay USB over Ethernet
> or serial line first ;)

This is of course not intended.  I was thinking about a plain standard
UDP based link.

> Seriously speaking, in view of ties between DFU and USB
> IMHO it is impossible, or, at least, highly impractical.

Can you please support this statement with a few facts?

> > If my assumption is correct, then what would it take to split off
> > protocol part and make it independent of the actual driver interface?
> 
> I guess that in the situation given it would be of little use.

What do you think would be of little use?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
This restaurant was advertising breakfast  any  time.  So  I  ordered
french toast in the renaissance.            - Steven Wright, comedian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-05 15:31   ` Wolfgang Denk
@ 2011-11-06 16:36     ` Stefan Schmidt
  2011-11-06 19:11       ` Wolfgang Denk
  0 siblings, 1 reply; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-06 16:36 UTC (permalink / raw)
  To: u-boot

Hello.

On Sat, 2011-11-05 at 16:31, Wolfgang Denk wrote:
> In message <20111102200717.GP17069@excalibur.local> you wrote:
> >
> > While I think a dfu command is usefull I don't like the need to
> > execute it before any DFU interaction can happen. That may be an
> > option during development but for field upgrades or receovery it is
> > not.
> 
> Yes, a command is the natural way in U-Boot to start any activities.
> 
> > I already wrote a bit about my approach here. At OpenMoko we used a
> > button to enter u-boot during startup when pressed. This offered a
> 
> This is just another way to start a command.

So bounding a key press to the command would be the way to achieve
what some people want. Fine with me.

> > usbtty interface as usb gadget as well as the runtime descripto for
> > DFU. With dfu-util it was possible to iniate the DFU download or
> > upload procedure while being in the mode. Another option would be to
> > directly jump into DFU mode when a button is pressed.
> 
> NAK.  Let's stick to standard interfaces.

What do you mean here? You don't want the DFU descripto available with
USBTTY or you don't want to start DFU directly in DFU mode?

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-05 15:33     ` Wolfgang Denk
@ 2011-11-06 17:02       ` Stefan Schmidt
  2011-11-06 19:46         ` Wolfgang Denk
  2011-11-07 12:12       ` Andrzej Pietrasiewicz
  1 sibling, 1 reply; 26+ messages in thread
From: Stefan Schmidt @ 2011-11-06 17:02 UTC (permalink / raw)
  To: u-boot

Hello.

On Sat, 2011-11-05 at 16:33, Wolfgang Denk wrote:
> In message <000601cc9abe$4f544bd0$edfce370$%p@samsung.com> you wrote:
> > 
> > > > http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
> > 
> > DFU is part of USB; an extension to be precise, but an extension bound
> > so tightly to the design and philosophy of USB that it is rather
> > inconceivable to separate the two.
> 
> Could you please be so kind and explain which exact issues you see for
> such a separation?

As Andrzej pointed out the DFU spec is written by the USB forum and
one can see that there target are USB devices. Let me bring in some
facts from the spec.

One part that is not covered here is the DFU suffix mandatory for all
firmware files. It gets stripped from the flashing tool before sending
it to the device code and thus it was not covered here so far. This
suffix holds, among others, fields with the PID of VID of the USB
device. Only allowing to be flashed if the device has the correct IDs.
How would assign the USB IDs to a network based approach for example?

The core part, I understand, is the DFU state machine you are referring
to. This sate machine relies on the USB descriptors in run-time or DFU
mode. It is hardcoded wot USB descriptors using bDeviceClass,
bDeviceSubClass, idVendor and idProduct to name a few. (See p. 14, 15
of the 1.1 spec).

In the reconfiguration phase the spec mandates the host to issue a
DFU_DETACH request on the USB EP0 and then issuing an USB reset. (p.
17)

All bound to the USB spec and hardware for using the spec in a
standard compliant way.

> > > Eventually it should be possible to run this protocol over Ethernet or
> > > even over a serial line?
> > 
> > Of course there is no such a reason, provided we lay USB over Ethernet
> > or serial line first ;)
> 
> This is of course not intended.  I was thinking about a plain standard
> UDP based link.

As always in computers one could try to come up with solutions to
replace the USB hardcoded parts with something else for a UDP based
solution. Its just now longer compliant with the DFU spec. All DFU
flashing utils out there are only working over USB. All the windows
flash utils as well as dfu-programmer, a small dfu util from the bluez
project and also dfu-util. The last ones are using libusb for this.

The main point is that if you replace the USB related parts with
something else not much of DFU stays anymore. It would be a different
flashing procedure. And for systems with a network interface available
there are way better ways to flash it then DFU. Even TFP would be more
convenient for them. You are way more flexible with Ethernet here.

DFU was designed for really small devices with minimal software
originally. Best examples are Bluetooth and WiFi USB dongles which can
be flashed this way if they. Its just that it becomes more popular as
many consumer electronics device are coming with a USB port but no
Ethernet these days.

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-06 16:36     ` Stefan Schmidt
@ 2011-11-06 19:11       ` Wolfgang Denk
  0 siblings, 0 replies; 26+ messages in thread
From: Wolfgang Denk @ 2011-11-06 19:11 UTC (permalink / raw)
  To: u-boot

Dear Stefan Schmidt,

In message <20111106163605.GA20104@excalibur.local> you wrote:
> 
> > > usbtty interface as usb gadget as well as the runtime descripto for
> > > DFU. With dfu-util it was possible to iniate the DFU download or
> > > upload procedure while being in the mode. Another option would be to
> > > directly jump into DFU mode when a button is pressed.
> > 
> > NAK.  Let's stick to standard interfaces.
> 
> What do you mean here? You don't want the DFU descripto available with
> USBTTY or you don't want to start DFU directly in DFU mode?

I don't want random transfers of control between unrelated
sub-systems.  If you want to run a command, then run a command.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the beginning, there was nothing, which exploded.
                                - Terry Pratchett, _Lords and Ladies_

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-06 17:02       ` Stefan Schmidt
@ 2011-11-06 19:46         ` Wolfgang Denk
  0 siblings, 0 replies; 26+ messages in thread
From: Wolfgang Denk @ 2011-11-06 19:46 UTC (permalink / raw)
  To: u-boot

Dear Stefan Schmidt,

In message <20111106170219.GB20104@excalibur.local> you wrote:
> 
> > Could you please be so kind and explain which exact issues you see for
> > such a separation?
> 
> As Andrzej pointed out the DFU spec is written by the USB forum and
> one can see that there target are USB devices. Let me bring in some
> facts from the spec.
...
> All bound to the USB spec and hardware for using the spec in a
> standard compliant way.

Yes, indeed.

But I feel you stick way too close to this spec, and are not trying to
abstract awway the low level details a bit.

Yes, this spec was written by the USB forum, and they did not even
attempt to look over the rim of their plates.  But does that mean we
all have to do the same?  I'm not willing to accept such a narrow
view.

Let's - just as a gedankenexperiment - assume our task was to
implement a solution that provides similar capabilities as DFU, but
must work over serial line, Ethernet, Bluetooth, Infrared, whatever.
Assume we thing DFU could be used as base, but we need to find a way
to move the USB related things into the USB layer, while things like
the state machine, the requests, device status, state transitions,
etc. are generic enough.

Yes, the DFU File Suffix contains fields that map nicely to USB
devices - but what prevents us to provide similar data on non-USB
media, too?  We just need to make sure that th code which handles this
allows to plug in handlers for other transfer media.


> As always in computers one could try to come up with solutions to
> replace the USB hardcoded parts with something else for a UDP based
> solution. Its just now longer compliant with the DFU spec. All DFU

I'm not sure what exactly you want to tell me here.

My idea is that of an implementation that is split into a generic and
a device specific part.  If implemented corectly, the combination of
the generic part with the USB specific part would be strictly
conformant to the DFU spec.

Of course, the combination of for example the DFU generic part with an
Ethernet network interface would not be conformant to the DFU spec,
but it would be something that works in a very similar way.  And that
would most probably be better than inventing yet another download
method.

> flashing utils out there are only working over USB. All the windows
> flash utils as well as dfu-programmer, a small dfu util from the bluez
> project and also dfu-util. The last ones are using libusb for this.

That's the status quo.  Agreed.  Bit this can be changed, extended.
Or not?

> The main point is that if you replace the USB related parts with
> something else not much of DFU stays anymore. It would be a different

I disagree here.  DFU is different in a number of aspects, and you
know this very well.

> flashing procedure. And for systems with a network interface available
> there are way better ways to flash it then DFU. Even TFP would be more
> convenient for them. You are way more flexible with Ethernet here.

I'm not sure what your exact argument is here. I'm not the one asking
for DFU support. I would be happy to use TFTP over Ethenret over USB.
We're actually doing this in some products.

> DFU was designed for really small devices with minimal software
> originally. Best examples are Bluetooth and WiFi USB dongles which can
> be flashed this way if they. Its just that it becomes more popular as
> many consumer electronics device are coming with a USB port but no
> Ethernet these days.

Agreed. And if it's becoming more popular, it might even be a good
idea to think about a less restricted implementation, which opens new
oportunites.


Yes, I think we should strive for such a separation of USB transport
layer and protocol implementation / state machine / command interface.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
While money can't buy happiness, it certainly lets  you  choose  your
own form of misery.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-05 15:33     ` Wolfgang Denk
  2011-11-06 17:02       ` Stefan Schmidt
@ 2011-11-07 12:12       ` Andrzej Pietrasiewicz
  2011-11-08 16:22         ` Detlev Zundel
  1 sibling, 1 reply; 26+ messages in thread
From: Andrzej Pietrasiewicz @ 2011-11-07 12:12 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

Please see my comments inline.

> >
> > DFU is part of USB; an extension to be precise, but an extension
> bound
> > so tightly to the design and philosophy of USB that it is rather
> > inconceivable to separate the two.
> 
> Could you please be so kind and explain which exact issues you see for
> such a separation?

First of all, I think that it is more proper to speak of communications
part and state machine part. They are bound together by the standard.
For example, in the dfuIDLE state, the receipt of DFU_DNLOAD request
with wLength == 0 triggers a "device stalls the control pipe" action.
This kind of stuff is very USB specific - I am not sure what would
"the control pipe" be for serial/Ethernet, nor what "stall the
control pipe" would mean for either of them.
Another example, in the dfuMANIFEST state, when the status poll
timeout occurs and bitManifestationTolerant == 1 then "device can
still communicate via the USB".

The communications part is of course the very USB itself, so the only
thing which could be extracted is the state machine part. But, then,
in order not to be bound to USB but reusable, it should be abstract.
Being abstract, it would not be DFU anymore, but some
"Generic Update Protocol" that you have on your mind,
but we don't know of. Is there any standard for such a protocol?
 
> 
> > Seriously speaking, in view of ties between DFU and USB
> > IMHO it is impossible, or, at least, highly impractical.
> 
> Can you please support this statement with a few facts?

Please see my comment above.
 
> > > If my assumption is correct, then what would it take to split off
> > > protocol part and make it independent of the actual driver
> interface?
> >
> > I guess that in the situation given it would be of little use.
> 
> What do you think would be of little use?
> 

As far as I understand you correctly, you would like the state
machine code extracted from the initial DFU implementation posted
on this mailing list. And then you would like the DFU implemented
in terms of this abstract state machine. This means actually
more code, probably not a desired thing. The generic state machine
would also require some accompanying generic data structures,
and this would also mean code to translate back and forth between
USB/DFU (DFU _is_ USB) and state machine's data structures.
All in all, this means adding more complexity and code. This looks
like a premature optimization (which is widely known to be the root
of all evil), while at the moment, there are no use cases
to justify it. Should the use cases appear, the code can be reworked
based on the needs of both USB/DFU and new state machine users.

Regards,

Andrzej

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [U-Boot] [PATCH] dfu: initial implementation
  2011-11-07 12:12       ` Andrzej Pietrasiewicz
@ 2011-11-08 16:22         ` Detlev Zundel
  0 siblings, 0 replies; 26+ messages in thread
From: Detlev Zundel @ 2011-11-08 16:22 UTC (permalink / raw)
  To: u-boot

Hi Andrzej,


[...]

>> > > If my assumption is correct, then what would it take to split off
>> > > protocol part and make it independent of the actual driver
>> interface?
>> >
>> > I guess that in the situation given it would be of little use.
>> 
>> What do you think would be of little use?
>> 
>
> As far as I understand you correctly, you would like the state
> machine code extracted from the initial DFU implementation posted
> on this mailing list. And then you would like the DFU implemented
> in terms of this abstract state machine. This means actually
> more code, probably not a desired thing. The generic state machine
> would also require some accompanying generic data structures,
> and this would also mean code to translate back and forth between
> USB/DFU (DFU _is_ USB) and state machine's data structures.
> All in all, this means adding more complexity and code. This looks
> like a premature optimization (which is widely known to be the root
> of all evil), while at the moment, there are no use cases
> to justify it. Should the use cases appear, the code can be reworked
> based on the needs of both USB/DFU and new state machine users.

Just to add my personal opinion here - I am occassionally lobbying for a
long time on this mailing list that mainline U-Boot gains DFU support,
so please let's get this working with the existing tools and in the
existing contexts, i.e. USB.  Personally I consider it to be completely
out of scope to dis-entangle DFU from its specification and abstract it
into something which it _could_ be, but really _isn't_.

When the implementation for USB is here, we all have much better grounds
to discuss possible generalizations (although I doubt that there are
many).

Cheers
  Detlev

-- 
Perfecting oneself is as much unlearning as it is learning. 
                                        -- Edsger Dijkstra 
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2011-11-08 16:22 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-02 10:00 [U-Boot] [PATCH] dfu: initial implementation Andrzej Pietrasiewicz
2011-11-02 14:29 ` Stefan Schmidt
2011-11-03  8:12   ` Andrzej Pietrasiewicz
2011-11-03 14:01     ` Stefan Schmidt
2011-11-04 13:14       ` Andrzej Pietrasiewicz
2011-11-02 15:16 ` Mike Frysinger
2011-11-02 19:30   ` Stefan Schmidt
2011-11-03  8:44     ` Andrzej Pietrasiewicz
2011-11-03 14:06       ` Stefan Schmidt
2011-11-03  8:17   ` Andrzej Pietrasiewicz
2011-11-02 20:07 ` Stefan Schmidt
2011-11-03 10:33   ` Andrzej Pietrasiewicz
2011-11-03 14:24     ` Stefan Schmidt
2011-11-03 12:47   ` Andrzej Pietrasiewicz
2011-11-03 13:31     ` Stefan Schmidt
2011-11-03 14:11       ` Andrzej Pietrasiewicz
2011-11-05 15:31   ` Wolfgang Denk
2011-11-06 16:36     ` Stefan Schmidt
2011-11-06 19:11       ` Wolfgang Denk
2011-11-03 18:13 ` Wolfgang Denk
2011-11-04  6:52   ` Andrzej Pietrasiewicz
2011-11-05 15:33     ` Wolfgang Denk
2011-11-06 17:02       ` Stefan Schmidt
2011-11-06 19:46         ` Wolfgang Denk
2011-11-07 12:12       ` Andrzej Pietrasiewicz
2011-11-08 16:22         ` Detlev Zundel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.