[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

This series adds NXP's Serial Download Protocol (SDP) support via
USB for SPL/U-Boot. It allows to download U-Boot via USB from a
(recovered) SPL using the same tools used to download SPL itself
(specifically imx_usb, but also sb_loader seems to work).

The idea has been brought up when the first targets started to make
use of SPL for DDR initialization, see:
https://lists.denx.de/pipermail/u-boot/2015-July/220330.html

The initial SDP implementation (patch 2) requires the payload to
have the imx specific headers (hence the move of the imx header
file in patch 1).

Patch 3 extends image header support beyond the SDP specification,
specifically implements also support for U-Boot headers. This
allows to use the same SPL/U-Boot binaries for recovery as used on
the regular boot device (SD/eMMC). For that to work also the host
side imx_usb tools needed an extension, currently available here:

https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_refactored

The full patchset allows to download SPL and U-Boot over USB to a
target in recovery mode using the same usb_imx utility:

The usb_imx utility also has a batch mode which allows to download
multiple artifacts with a single invocation. The details are
outlined in the imx_usb commit message:
https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60

In case this patchset gets accepted in U-Boot, I plan to push the
imx_usb changes upstream as well.


Stefan Agner (7):
  imx: move imximage header to common location
  usb: gadget: add SDP driver
  usb: gadget: sdp: extend images compatible for jumps
  cmd: add sdp command
  spl: add serial download protocol (SDP) support
  apalis/colibri_imx6: use independent USB PID for SPL
  apalis/colibri_imx6: enable SDP by default

 board/toradex/apalis_imx6/apalis_imx6.c   |  13 +
 board/toradex/colibri_imx6/colibri_imx6.c |  13 +
 cmd/Kconfig                               |   7 +
 cmd/Makefile                              |   1 +
 cmd/usb_gadget_sdp.c                      |  53 +++
 common/spl/Kconfig                        |   6 +
 common/spl/Makefile                       |   1 +
 common/spl/spl_sdp.c                      |  38 ++
 configs/apalis_imx6_defconfig             |   4 +
 configs/colibri_imx6_defconfig            |   4 +
 drivers/usb/gadget/Kconfig                |   7 +
 drivers/usb/gadget/Makefile               |   2 +
 drivers/usb/gadget/f_sdp.c                | 739 ++++++++++++++++++++++++++++++
 {tools => include}/imximage.h             |   0
 include/sdp.h                             |  16 +
 15 files changed, 904 insertions(+)
 create mode 100644 cmd/usb_gadget_sdp.c
 create mode 100644 common/spl/spl_sdp.c
 create mode 100644 drivers/usb/gadget/f_sdp.c
 rename {tools => include}/imximage.h (100%)
 create mode 100644 include/sdp.h

-- 
2.13.3

[U-Boot] [PATCH v1 1/7] imx: move imximage header to common location

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Move the imximage.h header file to a common location so we can make
use of it from U-Boot too.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 {tools => include}/imximage.h | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename {tools => include}/imximage.h (100%)

diff --git a/tools/imximage.h b/include/imximage.h
similarity index 100%
rename from tools/imximage.h
rename to include/imximage.h
-- 
2.13.3

[U-Boot] [PATCH v1 2/7] usb: gadget: add SDP driver

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Add SDP (Serial Downloader Protocol) implementation for U-Boot. The
protocol is used in NXP SoC's boot ROM and allows to download program
images. Beside that, it can also be used to read/write registers and
download complete Device Configuration Data (DCD) sets. This basic
implementation supports downloading images with the imx header format
and reading registers.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 drivers/usb/gadget/Kconfig  |   7 +
 drivers/usb/gadget/Makefile |   1 +
 drivers/usb/gadget/f_sdp.c  | 723 ++++++++++++++++++++++++++++++++++++++++++++
 include/sdp.h               |  16 +
 4 files changed, 747 insertions(+)
 create mode 100644 drivers/usb/gadget/f_sdp.c
 create mode 100644 include/sdp.h

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 261ed128ac..225b66bc95 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -103,6 +103,13 @@ config USB_GADGET_DOWNLOAD
 
 if USB_GADGET_DOWNLOAD
 
+config USB_FUNCTION_SDP
+	bool "Enable USB SDP (Serial Download Protocol)"
+	help
+	  Enable Serial Download Protocol (SDP) device support in U-Boot. This
+	  allows to download images into memory and execute (jump to) them
+	  using the same protocol as implemented by the i.MX family's boot ROM.
+
 config G_DNL_MANUFACTURER
 	string "Vendor name of USB device"
 
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 5e316a7cff..6a007d1bcb 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_USB_FUNCTION_THOR) += f_thor.o
 obj-$(CONFIG_USB_FUNCTION_DFU) += f_dfu.o
 obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
 obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
+obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
 endif
 endif
 ifdef CONFIG_USB_ETHER
diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
new file mode 100644
index 0000000000..eb89695aaf
--- /dev/null
+++ b/drivers/usb/gadget/f_sdp.c
@@ -0,0 +1,723 @@
+/*
+ * f_sdp.c -- USB HID Serial Download Protocol
+ *
+ * Copyright (C) 2016 Toradex
+ * Author: Stefan Agner <stefan.agner@toradex.com>
+ *
+ * This file implements the Serial Download Protocol (SDP) as specified in
+ * the i.MX 6 Reference Manual. The SDP is a USB HID based protocol and
+ * allows to download images directly to memory. The implementation
+ * works with the imx_loader (imx_usb) USB client software on host side.
+ *
+ * Not all commands are implemented, e.g. WRITE_REGISTER, DCD_WRITE and
+ * SKIP_DCD_HEADER are only stubs.
+ *
+ * Parts of the implementation are based on f_dfu and f_thor.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <errno.h>
+#include <common.h>
+#include <console.h>
+#include <malloc.h>
+
+#include <linux/usb/ch9.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/composite.h>
+
+#include <asm/io.h>
+#include <g_dnl.h>
+#include <sdp.h>
+#include <imximage.h>
+
+#define HID_REPORT_ID_MASK	0x000000ff
+
+/*
+ * HID class requests
+ */
+#define HID_REQ_GET_REPORT		0x01
+#define HID_REQ_GET_IDLE		0x02
+#define HID_REQ_GET_PROTOCOL		0x03
+#define HID_REQ_SET_REPORT		0x09
+#define HID_REQ_SET_IDLE		0x0A
+#define HID_REQ_SET_PROTOCOL		0x0B
+
+#define HID_USAGE_PAGE_LEN		76
+
+struct hid_report {
+	u8 usage_page[HID_USAGE_PAGE_LEN];
+} __packed;
+
+#define SDP_READ_REGISTER	0x0101
+#define SDP_WRITE_REGISTER	0x0202
+#define SDP_WRITE_FILE		0x0404
+#define SDP_ERROR_STATUS	0x0505
+#define SDP_DCD_WRITE		0x0a0a
+#define SDP_JUMP_ADDRESS	0x0b0b
+#define SDP_SKIP_DCD_HEADER	0x0c0c
+
+#define SDP_WRITE_FILE_COMPLETE		0x88888888
+#define SDP_WRITE_REGISTER_COMPLETE	0x128A8A12
+#define SDP_SKIP_DCD_HEADER_COMPLETE	0x900DD009
+#define SDP_ERROR_IMXHEADER		0x000a0533
+
+#define SDP_COMMAND_LEN		16
+
+struct sdp_command {
+	u16 cmd;
+	u32 addr;
+	u8 format;
+	u32 cnt;
+	u32 data;
+	u8 rsvd;
+} __packed;
+
+enum sdp_state {
+	SDP_STATE_IDLE,
+	SDP_STATE_RX_DCD_DATA,
+	SDP_STATE_RX_FILE_DATA,
+	SDP_STATE_TX_SEC_CONF,
+	SDP_STATE_TX_SEC_CONF_BUSY,
+	SDP_STATE_TX_REGISTER,
+	SDP_STATE_TX_REGISTER_BUSY,
+	SDP_STATE_TX_STATUS,
+	SDP_STATE_TX_STATUS_BUSY,
+	SDP_STATE_JUMP,
+};
+
+struct f_sdp {
+	struct usb_function		usb_function;
+
+	struct usb_descriptor_header	**function;
+
+	u8				altsetting;
+	enum sdp_state			state;
+	enum sdp_state			next_state;
+	u32				dnl_address;
+	u32				dnl_bytes_remaining;
+	u32				jmp_address;
+	bool				always_send_status;
+	u32				error_status;
+
+	/* EP0 request */
+	struct usb_request		*req;
+
+	/* EP1 IN */
+	struct usb_ep			*in_ep;
+	struct usb_request		*in_req;
+
+	bool				configuration_done;
+};
+
+static struct f_sdp *sdp_func;
+
+static inline struct f_sdp *func_to_sdp(struct usb_function *f)
+{
+	return container_of(f, struct f_sdp, usb_function);
+}
+
+static struct usb_interface_descriptor sdp_intf_runtime = {
+	.bLength =		sizeof(sdp_intf_runtime),
+	.bDescriptorType =	USB_DT_INTERFACE,
+	.bAlternateSetting =	0,
+	.bNumEndpoints =	1,
+	.bInterfaceClass =	USB_CLASS_HID,
+	.bInterfaceSubClass =	0,
+	.bInterfaceProtocol =	0,
+	/* .iInterface = DYNAMIC */
+};
+
+/* HID configuration */
+static struct usb_class_hid_descriptor sdp_hid_desc = {
+	.bLength =		sizeof(sdp_hid_desc),
+	.bDescriptorType =	USB_DT_CS_DEVICE,
+
+	.bcdCDC =		__constant_cpu_to_le16(0x0110),
+	.bCountryCode =		0,
+	.bNumDescriptors =	1,
+
+	.bDescriptorType0	= USB_DT_HID_REPORT,
+	.wDescriptorLength0	= HID_USAGE_PAGE_LEN,
+};
+
+static struct usb_endpoint_descriptor in_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT, /*USB_DT_CS_ENDPOINT*/
+
+	.bEndpointAddress =	1 | USB_DIR_IN,
+	.bmAttributes =	USB_ENDPOINT_XFER_INT,
+	.wMaxPacketSize =	64,
+	.bInterval =		1,
+};
+
+static struct usb_descriptor_header *sdp_runtime_descs[] = {
+	(struct usb_descriptor_header *)&sdp_intf_runtime,
+	(struct usb_descriptor_header *)&sdp_hid_desc,
+	(struct usb_descriptor_header *)&in_desc,
+	NULL,
+};
+
+/* This is synchronized with what the SoC implementation reports */
+static struct hid_report sdp_hid_report = {
+	.usage_page = {
+		0x06, 0x00, 0xff, /* Usage Page */
+		0x09, 0x01, /* Usage (Poiter?) */
+		0xa1, 0x01, /* Collection */
+
+		0x85, 0x01, /* Report ID */
+		0x19, 0x01, /* Usage Minimum */
+		0x29, 0x01, /* Usage Maximum */
+		0x15, 0x00, /* Local Minimum */
+		0x26, 0xFF, 0x00, /* Local Maximum? */
+		0x75, 0x08, /* Report Size */
+		0x95, 0x10, /* Report Count */
+		0x91, 0x02, /* Output Data */
+
+		0x85, 0x02, /* Report ID */
+		0x19, 0x01, /* Usage Minimum */
+		0x29, 0x01, /* Usage Maximum */
+		0x15, 0x00, /* Local Minimum */
+		0x26, 0xFF, 0x00, /* Local Maximum? */
+		0x75, 0x80, /* Report Size 128 */
+		0x95, 0x40, /* Report Count */
+		0x91, 0x02, /* Output Data */
+
+		0x85, 0x03, /* Report ID */
+		0x19, 0x01, /* Usage Minimum */
+		0x29, 0x01, /* Usage Maximum */
+		0x15, 0x00, /* Local Minimum */
+		0x26, 0xFF, 0x00, /* Local Maximum? */
+		0x75, 0x08, /* Report Size 8 */
+		0x95, 0x04, /* Report Count */
+		0x81, 0x02, /* Input Data */
+
+		0x85, 0x04, /* Report ID */
+		0x19, 0x01, /* Usage Minimum */
+		0x29, 0x01, /* Usage Maximum */
+		0x15, 0x00, /* Local Minimum */
+		0x26, 0xFF, 0x00, /* Local Maximum? */
+		0x75, 0x08, /* Report Size 8 */
+		0x95, 0x40, /* Report Count */
+		0x81, 0x02, /* Input Data */
+		0xc0
+	},
+};
+
+static const char sdp_name[] = "Serial Downloader Protocol";
+
+/*
+ * static strings, in UTF-8
+ */
+static struct usb_string strings_sdp_generic[] = {
+	[0].s = sdp_name,
+	{  }			/* end of list */
+};
+
+static struct usb_gadget_strings stringtab_sdp_generic = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_sdp_generic,
+};
+
+static struct usb_gadget_strings *sdp_generic_strings[] = {
+	&stringtab_sdp_generic,
+	NULL,
+};
+
+static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	struct f_sdp *sdp = req->context;
+	int status = req->status;
+	u8 *data = req->buf;
+	u8 report = data[0];
+
+	if (status != 0) {
+		error("Status: %d", status);
+		return;
+	}
+
+	if (report != 1) {
+		error("Unexpected report %d", report);
+		return;
+	}
+
+	struct sdp_command *cmd = req->buf + 1;
+
+	debug("%s: command: %04x, addr: %08x, cnt: %u\n",
+	      __func__, be16_to_cpu(cmd->cmd),
+	      be32_to_cpu(cmd->addr), be32_to_cpu(cmd->cnt));
+
+	switch (be16_to_cpu(cmd->cmd)) {
+	case SDP_READ_REGISTER:
+		sdp->always_send_status = false;
+		sdp->error_status = 0x0;
+
+		sdp->state = SDP_STATE_TX_SEC_CONF;
+		sdp->dnl_address = be32_to_cpu(cmd->addr);
+		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
+		sdp->next_state = SDP_STATE_TX_REGISTER;
+		printf("Reading %d registers at 0x%08x... ",
+		       sdp->dnl_bytes_remaining, sdp->dnl_address);
+		break;
+	case SDP_WRITE_FILE:
+		sdp->always_send_status = true;
+		sdp->error_status = SDP_WRITE_FILE_COMPLETE;
+
+		sdp->state = SDP_STATE_RX_FILE_DATA;
+		sdp->dnl_address = be32_to_cpu(cmd->addr);
+		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
+		sdp->next_state = SDP_STATE_IDLE;
+
+		printf("Downloading file of size %d to 0x%08x... ",
+		       sdp->dnl_bytes_remaining, sdp->dnl_address);
+
+		break;
+	case SDP_ERROR_STATUS:
+		sdp->always_send_status = true;
+		sdp->error_status = 0;
+
+		sdp->state = SDP_STATE_TX_SEC_CONF;
+		sdp->next_state = SDP_STATE_IDLE;
+		break;
+	case SDP_DCD_WRITE:
+		sdp->always_send_status = true;
+		sdp->error_status = SDP_WRITE_REGISTER_COMPLETE;
+
+		sdp->state = SDP_STATE_RX_DCD_DATA;
+		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
+		sdp->next_state = SDP_STATE_IDLE;
+		break;
+	case SDP_JUMP_ADDRESS:
+		sdp->always_send_status = false;
+		sdp->error_status = 0;
+
+		sdp->jmp_address = be32_to_cpu(cmd->addr);
+		sdp->state = SDP_STATE_TX_SEC_CONF;
+		sdp->next_state = SDP_STATE_JUMP;
+		break;
+	case SDP_SKIP_DCD_HEADER:
+		sdp->always_send_status = true;
+		sdp->error_status = SDP_SKIP_DCD_HEADER_COMPLETE;
+
+		/* Ignore command, DCD not supported anyway */
+		sdp->state = SDP_STATE_TX_SEC_CONF;
+		sdp->next_state = SDP_STATE_IDLE;
+		break;
+	default:
+		error("Unknown command: %08x\n", be16_to_cpu(cmd->cmd));
+	}
+}
+
+static void sdp_rx_data_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	struct f_sdp *sdp = req->context;
+	int status = req->status;
+	u8 *data = req->buf;
+	u8 report = data[0];
+	int datalen = req->length - 1;
+
+	if (status != 0) {
+		error("Status: %d", status);
+		return;
+	}
+
+	if (report != 2) {
+		error("Unexpected report %d", report);
+		return;
+	}
+
+	if (sdp->dnl_bytes_remaining < datalen) {
+		/*
+		 * Some USB stacks require to send a complete buffer as
+		 * specified in the HID descriptor. This leads to longer
+		 * transfers than the file length, no problem for us.
+		 */
+		sdp->dnl_bytes_remaining = 0;
+	} else {
+		sdp->dnl_bytes_remaining -= datalen;
+	}
+
+	if (sdp->state == SDP_STATE_RX_FILE_DATA) {
+		memcpy((void *)sdp->dnl_address, req->buf + 1, datalen);
+		sdp->dnl_address += datalen;
+	}
+
+	if (sdp->dnl_bytes_remaining)
+		return;
+
+	printf("done\n");
+
+	switch (sdp->state) {
+	case SDP_STATE_RX_FILE_DATA:
+		sdp->state = SDP_STATE_TX_SEC_CONF;
+		break;
+	case SDP_STATE_RX_DCD_DATA:
+		sdp->state = SDP_STATE_TX_SEC_CONF;
+		break;
+	default:
+		error("Invalid state: %d", sdp->state);
+	}
+}
+
+
+
+static void sdp_tx_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	struct f_sdp *sdp = req->context;
+	int status = req->status;
+
+	if (status != 0) {
+		error("Status: %d", status);
+		return;
+	}
+
+	switch (sdp->state) {
+	case SDP_STATE_TX_SEC_CONF_BUSY:
+		/* Not all commands require status report */
+		if (sdp->always_send_status || sdp->error_status)
+			sdp->state = SDP_STATE_TX_STATUS;
+		else
+			sdp->state = sdp->next_state;
+
+		break;
+	case SDP_STATE_TX_STATUS_BUSY:
+		sdp->state = sdp->next_state;
+		break;
+	case SDP_STATE_TX_REGISTER_BUSY:
+		if (sdp->dnl_bytes_remaining)
+			sdp->state = SDP_STATE_TX_REGISTER;
+		else
+			sdp->state = SDP_STATE_IDLE;
+		break;
+	default:
+		error("Wrong State: %d", sdp->state);
+		sdp->state = SDP_STATE_IDLE;
+		break;
+	}
+	debug("%s complete --> %d, %d/%d\n", ep->name,
+	      status, req->actual, req->length);
+}
+
+static int sdp_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
+{
+	struct usb_gadget *gadget = f->config->cdev->gadget;
+	struct usb_request *req = f->config->cdev->req;
+	struct f_sdp *sdp = f->config->cdev->req->context;
+	u16 len = le16_to_cpu(ctrl->wLength);
+	u16 w_value = le16_to_cpu(ctrl->wValue);
+	int value = 0;
+	u8 req_type = ctrl->bRequestType & USB_TYPE_MASK;
+
+	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
+	debug("req_type: 0x%x ctrl->bRequest: 0x%x sdp->state: %d\n",
+	      req_type, ctrl->bRequest, sdp->state);
+
+	if (req_type == USB_TYPE_STANDARD) {
+		if (ctrl->bRequest == USB_REQ_GET_DESCRIPTOR) {
+			/* Send HID report descriptor */
+			value = min(len, (u16) sizeof(sdp_hid_report));
+			memcpy(req->buf, &sdp_hid_report, value);
+			sdp->configuration_done = true;
+		}
+	}
+
+	if (req_type == USB_TYPE_CLASS) {
+		int report = w_value & HID_REPORT_ID_MASK;
+
+		/* HID (SDP) request */
+		switch (ctrl->bRequest) {
+		case HID_REQ_SET_REPORT:
+			switch (report) {
+			case 1:
+				value = SDP_COMMAND_LEN + 1;
+				req->complete = sdp_rx_command_complete;
+				break;
+			case 2:
+				value = len;
+				req->complete = sdp_rx_data_complete;
+				break;
+			}
+		}
+	}
+
+	if (value >= 0) {
+		req->length = value;
+		req->zero = value < len;
+		value = usb_ep_queue(gadget->ep0, req, 0);
+		if (value < 0) {
+			debug("ep_queue --> %d\n", value);
+			req->status = 0;
+		}
+	}
+
+	return value;
+}
+
+static int sdp_bind(struct usb_configuration *c, struct usb_function *f)
+{
+	struct usb_gadget *gadget = c->cdev->gadget;
+	struct usb_composite_dev *cdev = c->cdev;
+	struct f_sdp *sdp = func_to_sdp(f);
+	int rv = 0, id;
+
+	id = usb_interface_id(c, f);
+	if (id < 0)
+		return id;
+	sdp_intf_runtime.bInterfaceNumber = id;
+
+	struct usb_ep *ep;
+
+	/* allocate instance-specific endpoints */
+	ep = usb_ep_autoconfig(gadget, &in_desc);
+	if (!ep) {
+		rv = -ENODEV;
+		goto error;
+	}
+
+	sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
+
+	cdev->req->context = sdp;
+
+error:
+	return rv;
+}
+
+static void sdp_unbind(struct usb_configuration *c, struct usb_function *f)
+{
+	free(sdp_func);
+	sdp_func = NULL;
+}
+
+static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length)
+{
+	struct usb_request *req;
+
+	req = usb_ep_alloc_request(ep, 0);
+	if (!req)
+		return req;
+
+	req->length = length;
+	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, length);
+	if (!req->buf) {
+		usb_ep_free_request(ep, req);
+		req = NULL;
+	}
+
+	return req;
+}
+
+
+static struct usb_request *sdp_start_ep(struct usb_ep *ep)
+{
+	struct usb_request *req;
+
+	req = alloc_ep_req(ep, 64);
+	debug("%s: ep:%p req:%p\n", __func__, ep, req);
+
+	if (!req)
+		return NULL;
+
+	memset(req->buf, 0, req->length);
+	req->complete = sdp_tx_complete;
+
+	return req;
+}
+static int sdp_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
+{
+	struct f_sdp *sdp = func_to_sdp(f);
+	struct usb_composite_dev *cdev = f->config->cdev;
+	int result;
+
+	debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
+
+	result = usb_ep_enable(sdp->in_ep, &in_desc);
+	if (result)
+		return result;
+	sdp->in_req = sdp_start_ep(sdp->in_ep);
+	sdp->in_req->context = sdp;
+
+	sdp->in_ep->driver_data = cdev; /* claim */
+
+	sdp->altsetting = alt;
+	sdp->state = SDP_STATE_IDLE;
+
+	return 0;
+}
+
+static int sdp_get_alt(struct usb_function *f, unsigned intf)
+{
+	struct f_sdp *sdp = func_to_sdp(f);
+
+	return sdp->altsetting;
+}
+
+static void sdp_disable(struct usb_function *f)
+{
+	struct f_sdp *sdp = func_to_sdp(f);
+
+	usb_ep_disable(sdp->in_ep);
+
+	if (sdp->in_req) {
+		free(sdp->in_req);
+		sdp->in_req = NULL;
+	}
+}
+
+static int sdp_bind_config(struct usb_configuration *c)
+{
+	int status;
+
+	if (!sdp_func) {
+		sdp_func = memalign(CONFIG_SYS_CACHELINE_SIZE, sizeof(*sdp_func));
+		if (!sdp_func)
+			return -ENOMEM;
+	}
+
+	memset(sdp_func, 0, sizeof(*sdp_func));
+
+	sdp_func->usb_function.name = "sdp";
+	sdp_func->usb_function.hs_descriptors = sdp_runtime_descs;
+	sdp_func->usb_function.descriptors = sdp_runtime_descs;
+	sdp_func->usb_function.bind = sdp_bind;
+	sdp_func->usb_function.unbind = sdp_unbind;
+	sdp_func->usb_function.set_alt = sdp_set_alt;
+	sdp_func->usb_function.get_alt = sdp_get_alt;
+	sdp_func->usb_function.disable = sdp_disable;
+	sdp_func->usb_function.strings = sdp_generic_strings;
+	sdp_func->usb_function.setup = sdp_setup;
+
+	status = usb_add_function(c, &sdp_func->usb_function);
+
+	return status;
+}
+
+int sdp_init(void)
+{
+	printf("SDP: initialize...\n");
+	while (!sdp_func->configuration_done) {
+		if (ctrlc()) {
+			puts("\rCTRL+C - Operation aborted.\n");
+			return 0;
+		}
+		usb_gadget_handle_interrupts(0);
+	}
+
+	return 0;
+}
+
+static u32 sdp_jump_imxheader(void *address)
+{
+	flash_header_v2_t *headerv2 = address;
+	ulong (*entry)(void);
+
+	if (headerv2->header.tag != IVT_HEADER_TAG) {
+		printf("Header Tag is not a IMX image\n");
+		return SDP_ERROR_IMXHEADER;
+	}
+
+	printf("Jumping to 0x%08x\n", headerv2->entry);
+	entry = (void *)headerv2->entry;
+	entry();
+
+	/* The image probably never returns hence we wont reach that point */
+	return 0;
+}
+
+static void sdp_handle_in_ep(void)
+{
+	u8 *data = sdp_func->in_req->buf;
+	u32 status;
+	int datalen;
+
+	switch (sdp_func->state) {
+	case SDP_STATE_TX_SEC_CONF:
+		debug("Report 3: HAB security\n");
+		data[0] = 3;
+
+		data[1] = 0x56;
+		data[2] = 0x78;
+		data[3] = 0x78;
+		data[4] = 0x56;
+
+		sdp_func->in_req->length = 5;
+		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
+		sdp_func->state = SDP_STATE_TX_SEC_CONF_BUSY;
+		break;
+
+	case SDP_STATE_TX_STATUS:
+		debug("Report 4: Status\n");
+		data[0] = 4;
+
+		memcpy(&data[1], &sdp_func->error_status, 4);
+		sdp_func->in_req->length = 65;
+		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
+		sdp_func->state = SDP_STATE_TX_STATUS_BUSY;
+		break;
+	case SDP_STATE_TX_REGISTER:
+		debug("Report 4: Register Values\n");
+		data[0] = 4;
+
+		datalen = sdp_func->dnl_bytes_remaining;
+
+		if (datalen > 64)
+			datalen = 64;
+
+		memcpy(&data[1], (void *)sdp_func->dnl_address, datalen);
+		sdp_func->in_req->length = 65;
+
+		sdp_func->dnl_bytes_remaining -= datalen;
+		sdp_func->dnl_address += datalen;
+
+		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
+		sdp_func->state = SDP_STATE_TX_REGISTER_BUSY;
+		break;
+	case SDP_STATE_JUMP:
+		printf("Checking imxheader at 0x%08x\n", f_sdp->jmp_address);
+		status = sdp_jump_imxheader((void *)f_sdp->jmp_address);
+
+		sdp_func->next_state = SDP_STATE_IDLE;
+		sdp_func->error_status = status;
+
+		/* Only send Report 4 if there was an error */
+		if (status)
+			sdp_func->state = SDP_STATE_TX_STATUS;
+		else
+			sdp_func->state = SDP_STATE_IDLE;
+		break;
+	default:
+		break;
+	};
+}
+
+int sdp_handle(void)
+{
+	printf("SDP: handle requests...\n");
+	while (1) {
+		if (ctrlc()) {
+			puts("\rCTRL+C - Operation aborted.\n");
+			return 0;
+		}
+
+		usb_gadget_handle_interrupts(0);
+
+		sdp_handle_in_ep();
+	}
+}
+
+int sdp_add(struct usb_configuration *c)
+{
+	int id;
+
+	id = usb_string_id(c->cdev);
+	if (id < 0)
+		return id;
+	strings_sdp_generic[0].id = id;
+	sdp_intf_runtime.iInterface = id;
+
+	debug("%s: cdev: 0x%p gadget:0x%p gadget->ep0: 0x%p\n", __func__,
+	      c->cdev, c->cdev->gadget, c->cdev->gadget->ep0);
+
+	return sdp_bind_config(c);
+}
+
+DECLARE_GADGET_BIND_CALLBACK(usb_dnl_sdp, sdp_add);
diff --git a/include/sdp.h b/include/sdp.h
new file mode 100644
index 0000000000..03c4a23434
--- /dev/null
+++ b/include/sdp.h
@@ -0,0 +1,16 @@
+/*
+ * sdp.h - Serial Download Protocol
+ *
+ * Copyright (C) 2016 Toradex
+ * Author: Stefan Agner <stefan.agner@toradex.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __SDP_H_
+#define __SDP_H_
+
+int sdp_init(void);
+int sdp_handle(void);
+
+#endif /* __SDP_H_ */
-- 
2.13.3

[U-Boot] [PATCH v1 3/7] usb: gadget: sdp: extend images compatible for jumps

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Support U-Boot images in SPL so that u-boot.img files can be
directly downloaded and executed. Furthermore support U-Boot
scripts download and execution in full U-Boot so that custom
recovery actions can be downloaded from the host in a third
step.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 drivers/usb/gadget/f_sdp.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
index eb89695aaf..9a752843f0 100644
--- a/drivers/usb/gadget/f_sdp.c
+++ b/drivers/usb/gadget/f_sdp.c
@@ -29,6 +29,8 @@
 #include <asm/io.h>
 #include <g_dnl.h>
 #include <sdp.h>
+#include <spl.h>
+#include <image.h>
 #include <imximage.h>
 
 #define HID_REPORT_ID_MASK	0x000000ff
@@ -672,8 +674,22 @@ static void sdp_handle_in_ep(void)
 		sdp_func->state = SDP_STATE_TX_REGISTER_BUSY;
 		break;
 	case SDP_STATE_JUMP:
-		printf("Checking imxheader at 0x%08x\n", f_sdp->jmp_address);
-		status = sdp_jump_imxheader((void *)f_sdp->jmp_address);
+		printf("Jumping to header at 0x%08x\n", sdp_func->jmp_address);
+		status = sdp_jump_imxheader((void *)sdp_func->jmp_address);
+
+		/* If imx header fails, try some U-Boot specific headers */
+		if (status) {
+#ifdef CONFIG_SPL_BUILD
+			/* In SPL, allow jumps to U-Boot images */
+			struct spl_image_info spl_image = {};
+			spl_parse_image_header(&spl_image,
+				(struct image_header *)sdp_func->jmp_address);
+			jump_to_image_no_args(&spl_image);
+#else
+			/* In U-Boot, allow jumps to scripts */
+			source(sdp_func->jmp_address, "script at 1");
+#endif
+		}
 
 		sdp_func->next_state = SDP_STATE_IDLE;
 		sdp_func->error_status = status;
-- 
2.13.3

[U-Boot] [PATCH v1 4/7] cmd: add sdp command

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Add a new command to start USB Serial Download Protocol (SDP)
state machine.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 cmd/Kconfig          |  7 +++++++
 cmd/Makefile         |  1 +
 cmd/usb_gadget_sdp.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)
 create mode 100644 cmd/usb_gadget_sdp.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index f18efc1e88..87333b3a97 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -665,6 +665,13 @@ config CMD_DFU
 	  Enables the command "dfu" which is used to have U-Boot create a DFU
 	  class device via USB.
 
+config CMD_USB_SDP
+	bool "sdp"
+	select USB_FUNCTION_SDP
+	help
+	  Enables the command "sdp" which is used to have U-Boot emulating the
+	  Serial Download Protocol (SDP) via USB.
+
 config CMD_USB_MASS_STORAGE
 	bool "UMS usb mass storage"
 	help
diff --git a/cmd/Makefile b/cmd/Makefile
index bd231f24d8..e0b5940ba6 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_CMD_FASTBOOT) += fastboot.o
 obj-$(CONFIG_CMD_FS_UUID) += fs_uuid.o
 
 obj-$(CONFIG_CMD_USB_MASS_STORAGE) += usb_mass_storage.o
+obj-$(CONFIG_CMD_USB_SDP) += usb_gadget_sdp.o
 obj-$(CONFIG_CMD_THOR_DOWNLOAD) += thordown.o
 obj-$(CONFIG_CMD_XIMG) += ximg.o
 obj-$(CONFIG_YAFFS2) += yaffs2.o
diff --git a/cmd/usb_gadget_sdp.c b/cmd/usb_gadget_sdp.c
new file mode 100644
index 0000000000..09ddb4f3aa
--- /dev/null
+++ b/cmd/usb_gadget_sdp.c
@@ -0,0 +1,53 @@
+/*
+ * cmd_sdp.c -- sdp command
+ *
+ * Copyright (C) 2016 Toradex
+ * Author: Stefan Agner <stefan.agner@toradex.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <g_dnl.h>
+#include <sdp.h>
+#include <usb.h>
+
+static int do_sdp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int ret = CMD_RET_SUCCESS;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	char *usb_controller = argv[1];
+	int controller_index = simple_strtoul(usb_controller, NULL, 0);
+	board_usb_init(controller_index, USB_INIT_DEVICE);
+
+	g_dnl_clear_detach();
+	g_dnl_register("usb_dnl_sdp");
+
+	ret = sdp_init();
+	if (ret) {
+		error("SDP init failed: %d", ret);
+		ret = CMD_RET_FAILURE;
+		goto exit;
+	}
+
+	ret = sdp_handle();
+	if (ret) {
+		error("SDP failed: %d", ret);
+		ret = CMD_RET_FAILURE;
+		goto exit;
+	}
+
+exit:
+	g_dnl_unregister();
+
+	return ret;
+}
+
+U_BOOT_CMD(sdp, 2, 1, do_sdp,
+	"Serial Downloader Protocol",
+	"<USB_controller>\n"
+	"  - serial downloader protocol via <USB_controller>\n"
+);
-- 
2.13.3

[U-Boot] [PATCH v1 5/7] spl: add serial download protocol (SDP) support

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Add USB serial download protocol support to SPL. If the SoC started
in recovery mode the SPL will immediately switch to SDP and wait for
further downloads/commands from the host side.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 common/spl/Kconfig          |  6 ++++++
 common/spl/Makefile         |  1 +
 common/spl/spl_sdp.c        | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/Makefile |  1 +
 4 files changed, 46 insertions(+)
 create mode 100644 common/spl/spl_sdp.c

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 4de81392b0..95378b98a0 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -646,6 +646,12 @@ config SPL_DFU_RAM
 
 endchoice
 
+config SPL_USB_SDP_SUPPORT
+	bool "Support SDP (Serial Download Protocol)"
+	help
+	  Enable Serial Download Protocol (SDP) device support in SPL. This
+	  allows to download images into memory and execute (jump to) them
+	  using the same protocol as implemented by the i.MX family's boot ROM.
 endif
 
 config SPL_WATCHDOG_SUPPORT
diff --git a/common/spl/Makefile b/common/spl/Makefile
index 47a64dd7d0..a979560acf 100644
--- a/common/spl/Makefile
+++ b/common/spl/Makefile
@@ -29,4 +29,5 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
 obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
 obj-$(CONFIG_SPL_SPI_LOAD) += spl_spi.o
 obj-$(CONFIG_SPL_RAM_SUPPORT) += spl_ram.o
+obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += spl_sdp.o
 endif
diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
new file mode 100644
index 0000000000..faa74b8bec
--- /dev/null
+++ b/common/spl/spl_sdp.c
@@ -0,0 +1,38 @@
+/*
+ * (C) Copyright 2016 Toradex
+ * Author: Stefan Agner <stefan.agner@toradex.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <spl.h>
+#include <usb.h>
+#include <g_dnl.h>
+#include <sdp.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static int spl_sdp_load_image(struct spl_image_info *spl_image,
+			      struct spl_boot_device *bootdev)
+{
+	int ret;
+
+	g_dnl_clear_detach();
+	g_dnl_register("usb_dnl_sdp");
+
+	ret = sdp_init();
+	if (ret) {
+		error("SDP init failed: %d", ret);
+		return -ENODEV;
+	}
+
+	ret = sdp_handle();
+	if (ret) {
+		error("SDP failed: %d", ret);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+SPL_LOAD_IMAGE_METHOD("USB SDP", 0, BOOT_DEVICE_UART, spl_sdp_load_image);
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 6a007d1bcb..7258099c1c 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_USB_ETHER) += epautoconf.o config.o usbstring.o
 ifdef CONFIG_SPL_BUILD
 obj-$(CONFIG_SPL_USB_GADGET_SUPPORT) += g_dnl.o
 obj-$(CONFIG_SPL_DFU_SUPPORT) += f_dfu.o
+obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += f_sdp.o
 endif
 
 # new USB gadget layer dependencies
-- 
2.13.3

[U-Boot] [PATCH v1 6/7] apalis/colibri_imx6: use independent USB PID for SPL

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Use a completely independent USB Product ID for SPL. This allows
to differentiate a SDP running in SPL and SDP running in a U-Boot
which could not read the config block successfully.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Acked-by: Max Krummenacher <max.krummenacher@toradex.com>
---

 board/toradex/apalis_imx6/apalis_imx6.c   | 13 +++++++++++++
 board/toradex/colibri_imx6/colibri_imx6.c | 13 +++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/board/toradex/apalis_imx6/apalis_imx6.c b/board/toradex/apalis_imx6/apalis_imx6.c
index 8e5613cb12..edaca5d346 100644
--- a/board/toradex/apalis_imx6/apalis_imx6.c
+++ b/board/toradex/apalis_imx6/apalis_imx6.c
@@ -28,6 +28,7 @@
 #include <dm/platform_data/serial_mxc.h>
 #include <dm/platdata.h>
 #include <fsl_esdhc.h>
+#include <g_dnl.h>
 #include <i2c.h>
 #include <imx_thermal.h>
 #include <linux/errno.h>
@@ -1233,6 +1234,18 @@ void reset_cpu(ulong addr)
 {
 }
 
+#ifdef CONFIG_SPL_USB_GADGET_SUPPORT
+int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
+{
+	unsigned short usb_pid;
+
+	usb_pid = TORADEX_USB_PRODUCT_NUM_OFFSET + 0xfff;
+	put_unaligned(usb_pid, &dev->idProduct);
+
+	return 0;
+}
+#endif
+
 #endif
 
 static struct mxc_serial_platdata mxc_serial_plat = {
diff --git a/board/toradex/colibri_imx6/colibri_imx6.c b/board/toradex/colibri_imx6/colibri_imx6.c
index cbf7aa952a..0cc958a0a8 100644
--- a/board/toradex/colibri_imx6/colibri_imx6.c
+++ b/board/toradex/colibri_imx6/colibri_imx6.c
@@ -28,6 +28,7 @@
 #include <dm/platform_data/serial_mxc.h>
 #include <dm/platdata.h>
 #include <fsl_esdhc.h>
+#include <g_dnl.h>
 #include <i2c.h>
 #include <imx_thermal.h>
 #include <linux/errno.h>
@@ -1118,6 +1119,18 @@ void reset_cpu(ulong addr)
 {
 }
 
+#ifdef CONFIG_SPL_USB_GADGET_SUPPORT
+int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
+{
+	unsigned short usb_pid;
+
+	usb_pid = TORADEX_USB_PRODUCT_NUM_OFFSET + 0xfff;
+	put_unaligned(usb_pid, &dev->idProduct);
+
+	return 0;
+}
+#endif
+
 #endif
 
 static struct mxc_serial_platdata mxc_serial_plat = {
-- 
2.13.3

[U-Boot] [PATCH v1 7/7] apalis/colibri_imx6: enable SDP by default

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Enable Serial Download Protocol (SDP) in SPL and U-Boot. This is
useful to make use of imx_usb to download the complete U-Boot
(u-boot.img) after SPL has been downloaded. The U-Boot command
sdp allows to enumerate as SDP capable device again, e.g. to
download a Linux kernel and/or U-Boot script.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Acked-by: Max Krummenacher <max.krummenacher@toradex.com>
---

 configs/apalis_imx6_defconfig  | 4 ++++
 configs/colibri_imx6_defconfig | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/configs/apalis_imx6_defconfig b/configs/apalis_imx6_defconfig
index 4d88e70200..2a271f002c 100644
--- a/configs/apalis_imx6_defconfig
+++ b/configs/apalis_imx6_defconfig
@@ -17,6 +17,9 @@ CONFIG_BOARD_EARLY_INIT_F=y
 CONFIG_SPL=y
 CONFIG_SPL_DMA_SUPPORT=y
 CONFIG_SPL_I2C_SUPPORT=y
+CONFIG_SPL_USB_HOST_SUPPORT=y
+CONFIG_SPL_USB_GADGET_SUPPORT=y
+CONFIG_SPL_USB_SDP_SUPPORT=y
 CONFIG_HUSH_PARSER=y
 CONFIG_SYS_PROMPT="Apalis iMX6 # "
 CONFIG_CMD_BOOTZ=y
@@ -31,6 +34,7 @@ CONFIG_CMD_MMC=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_DFU=y
+CONFIG_CMD_USB_SDP=y
 CONFIG_CMD_USB_MASS_STORAGE=y
 # CONFIG_CMD_FPGA is not set
 CONFIG_CMD_GPIO=y
diff --git a/configs/colibri_imx6_defconfig b/configs/colibri_imx6_defconfig
index a23975f858..d5d3e7a143 100644
--- a/configs/colibri_imx6_defconfig
+++ b/configs/colibri_imx6_defconfig
@@ -17,6 +17,9 @@ CONFIG_BOARD_EARLY_INIT_F=y
 CONFIG_SPL=y
 CONFIG_SPL_DMA_SUPPORT=y
 CONFIG_SPL_I2C_SUPPORT=y
+CONFIG_SPL_USB_HOST_SUPPORT=y
+CONFIG_SPL_USB_GADGET_SUPPORT=y
+CONFIG_SPL_USB_SDP_SUPPORT=y
 CONFIG_HUSH_PARSER=y
 CONFIG_SYS_PROMPT="Colibri iMX6 # "
 CONFIG_CMD_BOOTZ=y
@@ -31,6 +34,7 @@ CONFIG_CMD_MMC=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_DFU=y
+CONFIG_CMD_USB_SDP=y
 CONFIG_CMD_USB_MASS_STORAGE=y
 # CONFIG_CMD_FPGA is not set
 CONFIG_CMD_GPIO=y
-- 
2.13.3

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Eric Nelson]

Hi Stefan,

On 08/04/2017 04:38 PM, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> This series adds NXP's Serial Download Protocol (SDP) support via
> USB for SPL/U-Boot. It allows to download U-Boot via USB from a
> (recovered) SPL using the same tools used to download SPL itself
> (specifically imx_usb, but also sb_loader seems to work).
> 

Nice!

> The idea has been brought up when the first targets started to make
> use of SPL for DDR initialization, see:
> https://lists.denx.de/pipermail/u-boot/2015-July/220330.html
> 

There have been lots of discussions surrounding the use of SDP,
and this seems to be a nice alternative to using the i.MX "plugin"
mode that I explored a while back:

https://lists.denx.de/pipermail/u-boot/2017-July/thread.html#298266

> The initial SDP implementation (patch 2) requires the payload to
> have the imx specific headers (hence the move of the imx header
> file in patch 1).
> 
> Patch 3 extends image header support beyond the SDP specification,
> specifically implements also support for U-Boot headers. This
> allows to use the same SPL/U-Boot binaries for recovery as used on
> the regular boot device (SD/eMMC). For that to work also the host
> side imx_usb tools needed an extension, currently available here:
> 
> https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_refactored
> 
> The full patchset allows to download SPL and U-Boot over USB to a
> target in recovery mode using the same usb_imx utility:

imx_usb?

> The usb_imx utility also has a batch mode which allows to download
> multiple artifacts with a single invocation. The details are
> outlined in the imx_usb commit message:
> https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60
> 
> In case this patchset gets accepted in U-Boot, I plan to push the
> imx_usb changes upstream as well.
> 

Do you have numbers for how much code/data size this adds to SPL?

I believe some i.MX users have struggled to stay within the
size of internal RAM.

Regards,


Eric

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Stefan Agner]

Hi Eric,

On 2017-08-06 08:19, Eric Nelson wrote:
> Hi Stefan,
> 
> On 08/04/2017 04:38 PM, Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> This series adds NXP's Serial Download Protocol (SDP) support via
>> USB for SPL/U-Boot. It allows to download U-Boot via USB from a
>> (recovered) SPL using the same tools used to download SPL itself
>> (specifically imx_usb, but also sb_loader seems to work).
>>
> 
> Nice!
> 
>> The idea has been brought up when the first targets started to make
>> use of SPL for DDR initialization, see:
>> https://lists.denx.de/pipermail/u-boot/2015-July/220330.html
>>
> 
> There have been lots of discussions surrounding the use of SDP,
> and this seems to be a nice alternative to using the i.MX "plugin"
> mode that I explored a while back:
> 
> https://lists.denx.de/pipermail/u-boot/2017-July/thread.html#298266
> 

Hm, wasn't aware of that particular effort, thanks for pointing to it.
From a quick glance, it did not work out so far, is this correct?

I looked into plugin mode also a little bit, but I did not continue to
look into it after reading this sentence in the i.MX 6 RM:

8.7 Plugin Image
The boot ROM detects the image type using the plugin flag of the boot
data structure (see
Boot Data Structure). If the plugin flag is 1, then the ROM uses the
image as a plugin
function. The function must initialize the boot device and copy the
program image to the
final location. At the end the plugin function must return with the
program image
parameters. (See High level boot sequence for details about boot flow).


So if SPL should work as a plugin as NXP defines it, SPL is supposed to
load the image from somewhere. The boot ROM then only takes care about
image verification and further boot steps, but it's the plugins job to
get the image from somewhere and store it in RAM.

Afact this is not really helpful in our case. We would want the boot ROM
to go through the boot sequence (again).

Unless returning an error/invalid image causes the boot ROM to go
through boot sequence again?


The nice thing with our own SDP implementation is we can reuse it even
from within U-Boot again, e.g. to download a kernel/initramfs....

>> The initial SDP implementation (patch 2) requires the payload to
>> have the imx specific headers (hence the move of the imx header
>> file in patch 1).
>>
>> Patch 3 extends image header support beyond the SDP specification,
>> specifically implements also support for U-Boot headers. This
>> allows to use the same SPL/U-Boot binaries for recovery as used on
>> the regular boot device (SD/eMMC). For that to work also the host
>> side imx_usb tools needed an extension, currently available here:
>>
>> https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_refactored
>>
>> The full patchset allows to download SPL and U-Boot over USB to a
>> target in recovery mode using the same usb_imx utility:
> 
> imx_usb?
> 

Yeah right, sorry.

>> The usb_imx utility also has a batch mode which allows to download
>> multiple artifacts with a single invocation. The details are
>> outlined in the imx_usb commit message:
>> https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60
>>
>> In case this patchset gets accepted in U-Boot, I plan to push the
>> imx_usb changes upstream as well.
>>
> 
> Do you have numbers for how much code/data size this adds to SPL?
> 

Besides the protocol implementation also general USB (gadget) support is
required, hence it adds quite a bit.


Without USB Gadget/SDP support (Apalis iMX6 SPL):

$ arm-linux-gnueabihf-size spl/u-boot-spl
   text    data     bss     dec     hex filename
  24552    3808      92   28452    6f24 spl/u-boot-spl


With USB Gadget/SDP support:

$ arm-linux-gnueabihf-size spl/u-boot-spl
   text    data     bss     dec     hex filename
  42549    4679    1984   49212    c03c spl/u-boot-spl


> I believe some i.MX users have struggled to stay within the
> size of internal RAM.

Hm, I think the limit is somewhere around 64kiB? In our case we are
still well below...

--
Stefan

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Stefano Babic]

Hi Stefan,

On 07/08/2017 20:06, Stefan Agner wrote:
> Hi Eric,
> 
> On 2017-08-06 08:19, Eric Nelson wrote:
>> Hi Stefan,
>>
>> On 08/04/2017 04:38 PM, Stefan Agner wrote:
>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> This series adds NXP's Serial Download Protocol (SDP) support via
>>> USB for SPL/U-Boot. It allows to download U-Boot via USB from a
>>> (recovered) SPL using the same tools used to download SPL itself
>>> (specifically imx_usb, but also sb_loader seems to work).
>>>
>>
>> Nice!
>>
>>> The idea has been brought up when the first targets started to make
>>> use of SPL for DDR initialization, see:
>>> https://lists.denx.de/pipermail/u-boot/2015-July/220330.html
>>>
>>
>> There have been lots of discussions surrounding the use of SDP,
>> and this seems to be a nice alternative to using the i.MX "plugin"
>> mode that I explored a while back:
>>
>> https://lists.denx.de/pipermail/u-boot/2017-July/thread.html#298266
>>
> 
> Hm, wasn't aware of that particular effort, thanks for pointing to it.
> From a quick glance, it did not work out so far, is this correct?
> 
> I looked into plugin mode also a little bit, but I did not continue to
> look into it after reading this sentence in the i.MX 6 RM:
> 
> 8.7 Plugin Image
> The boot ROM detects the image type using the plugin flag of the boot
> data structure (see
> Boot Data Structure). If the plugin flag is 1, then the ROM uses the
> image as a plugin
> function. The function must initialize the boot device and copy the
> program image to the
> final location. At the end the plugin function must return with the
> program image
> parameters. (See High level boot sequence for details about boot flow).
> 
> 
> So if SPL should work as a plugin as NXP defines it, SPL is supposed to
> load the image from somewhere. The boot ROM then only takes care about
> image verification and further boot steps, but it's the plugins job to
> get the image from somewhere and store it in RAM.
> 

Right, and this is also the weak point. There are also some cases (at
least, in some projects of mine) where this conflicts with the setup of
the RAM controller. We need to set the RAM controller to load the code
or to fight with the limitation of the IRAM.

> Afact this is not really helpful in our case. We would want the boot ROM
> to go through the boot sequence (again).

Agree. This makes the whole boot process easier to understand - and not
to mention the fact that code of BootROM is not officially published,
and we cannot know what it exactly does.

> 
> Unless returning an error/invalid image causes the boot ROM to go
> through boot sequence again?
> 
> 
> The nice thing with our own SDP implementation is we can reuse it even
> from within U-Boot again, e.g. to download a kernel/initramfs....

Right - I think SDP is a nice solution, and thanks for your effort !

> 
>>> The initial SDP implementation (patch 2) requires the payload to
>>> have the imx specific headers (hence the move of the imx header
>>> file in patch 1).
>>>
>>> Patch 3 extends image header support beyond the SDP specification,
>>> specifically implements also support for U-Boot headers. This
>>> allows to use the same SPL/U-Boot binaries for recovery as used on
>>> the regular boot device (SD/eMMC). For that to work also the host
>>> side imx_usb tools needed an extension, currently available here:
>>>
>>> https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_refactored
>>>
>>> The full patchset allows to download SPL and U-Boot over USB to a
>>> target in recovery mode using the same usb_imx utility:
>>
>> imx_usb?
>>
> 
> Yeah right, sorry.

But what about to merge into the official imx_usb repo ?

> 
>>> The usb_imx utility also has a batch mode which allows to download
>>> multiple artifacts with a single invocation. The details are
>>> outlined in the imx_usb commit message:
>>> https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60
>>>
>>> In case this patchset gets accepted in U-Boot, I plan to push the
>>> imx_usb changes upstream as well.
>>>
>>
>> Do you have numbers for how much code/data size this adds to SPL?
>>
> 
> Besides the protocol implementation also general USB (gadget) support is
> required, hence it adds quite a bit.
> 
> 
> Without USB Gadget/SDP support (Apalis iMX6 SPL):
> 
> $ arm-linux-gnueabihf-size spl/u-boot-spl
>    text    data     bss     dec     hex filename
>   24552    3808      92   28452    6f24 spl/u-boot-spl
> 
> 
> With USB Gadget/SDP support:
> 
> $ arm-linux-gnueabihf-size spl/u-boot-spl
>    text    data     bss     dec     hex filename
>   42549    4679    1984   49212    c03c spl/u-boot-spl


mmmhhh...ok, I see.

> 
> 
>> I believe some i.MX users have struggled to stay within the
>> size of internal RAM.
> 
> Hm, I think the limit is somewhere around 64kiB? In our case we are
> still well below...

I will try to build. The issue is with MX6 Solo (and some lower level
MX6) because it has 64KB IRAM and due to BootROM Stack the maximum size
for SPL is lower (maybe 48K ?). As far as I see, apalis is MX6Q or MX6D,
in both cases no problem. If the size is too big for Solo, we will have
to limit SDP's usage to the SOCs with more IRAM (Dual / Quad/ QP..)

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v1 2/7] usb: gadget: add SDP driver

[Lothar Waßmann]

Hi,

On Fri,  4 Aug 2017 16:38:08 -0700 Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add SDP (Serial Downloader Protocol) implementation for U-Boot. The
> protocol is used in NXP SoC's boot ROM and allows to download program
> images. Beside that, it can also be used to read/write registers and
> download complete Device Configuration Data (DCD) sets. This basic
> implementation supports downloading images with the imx header format
> and reading registers.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  drivers/usb/gadget/Kconfig  |   7 +
>  drivers/usb/gadget/Makefile |   1 +
>  drivers/usb/gadget/f_sdp.c  | 723 ++++++++++++++++++++++++++++++++++++++++++++
>  include/sdp.h               |  16 +
>  4 files changed, 747 insertions(+)
>  create mode 100644 drivers/usb/gadget/f_sdp.c
>  create mode 100644 include/sdp.h
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 261ed128ac..225b66bc95 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -103,6 +103,13 @@ config USB_GADGET_DOWNLOAD
>  
>  if USB_GADGET_DOWNLOAD
>  
> +config USB_FUNCTION_SDP
> +	bool "Enable USB SDP (Serial Download Protocol)"
> +	help
> +	  Enable Serial Download Protocol (SDP) device support in U-Boot. This
> +	  allows to download images into memory and execute (jump to) them
> +	  using the same protocol as implemented by the i.MX family's boot ROM.
> +
>  config G_DNL_MANUFACTURER
>  	string "Vendor name of USB device"
>  
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 5e316a7cff..6a007d1bcb 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_USB_FUNCTION_THOR) += f_thor.o
>  obj-$(CONFIG_USB_FUNCTION_DFU) += f_dfu.o
>  obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
>  obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
> +obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
>  endif
>  endif
>  ifdef CONFIG_USB_ETHER
> diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> new file mode 100644
> index 0000000000..eb89695aaf
> --- /dev/null
> +++ b/drivers/usb/gadget/f_sdp.c
> @@ -0,0 +1,723 @@
> +/*
> + * f_sdp.c -- USB HID Serial Download Protocol
> + *
> + * Copyright (C) 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * This file implements the Serial Download Protocol (SDP) as specified in
> + * the i.MX 6 Reference Manual. The SDP is a USB HID based protocol and
> + * allows to download images directly to memory. The implementation
> + * works with the imx_loader (imx_usb) USB client software on host side.
> + *
> + * Not all commands are implemented, e.g. WRITE_REGISTER, DCD_WRITE and
> + * SKIP_DCD_HEADER are only stubs.
> + *
> + * Parts of the implementation are based on f_dfu and f_thor.
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <errno.h>
> +#include <common.h>
> +#include <console.h>
> +#include <malloc.h>
> +
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/composite.h>
> +
> +#include <asm/io.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +#include <imximage.h>
> +
> +#define HID_REPORT_ID_MASK	0x000000ff
> +
> +/*
> + * HID class requests
> + */
> +#define HID_REQ_GET_REPORT		0x01
> +#define HID_REQ_GET_IDLE		0x02
> +#define HID_REQ_GET_PROTOCOL		0x03
> +#define HID_REQ_SET_REPORT		0x09
> +#define HID_REQ_SET_IDLE		0x0A
> +#define HID_REQ_SET_PROTOCOL		0x0B
> +
> +#define HID_USAGE_PAGE_LEN		76
> +
> +struct hid_report {
> +	u8 usage_page[HID_USAGE_PAGE_LEN];
> +} __packed;
> +
> +#define SDP_READ_REGISTER	0x0101
> +#define SDP_WRITE_REGISTER	0x0202
> +#define SDP_WRITE_FILE		0x0404
> +#define SDP_ERROR_STATUS	0x0505
> +#define SDP_DCD_WRITE		0x0a0a
> +#define SDP_JUMP_ADDRESS	0x0b0b
> +#define SDP_SKIP_DCD_HEADER	0x0c0c
> +
> +#define SDP_WRITE_FILE_COMPLETE		0x88888888
> +#define SDP_WRITE_REGISTER_COMPLETE	0x128A8A12
> +#define SDP_SKIP_DCD_HEADER_COMPLETE	0x900DD009
> +#define SDP_ERROR_IMXHEADER		0x000a0533
> +
> +#define SDP_COMMAND_LEN		16
> +
> +struct sdp_command {
> +	u16 cmd;
> +	u32 addr;
> +	u8 format;
> +	u32 cnt;
> +	u32 data;
> +	u8 rsvd;
> +} __packed;
> +
> +enum sdp_state {
> +	SDP_STATE_IDLE,
> +	SDP_STATE_RX_DCD_DATA,
> +	SDP_STATE_RX_FILE_DATA,
> +	SDP_STATE_TX_SEC_CONF,
> +	SDP_STATE_TX_SEC_CONF_BUSY,
> +	SDP_STATE_TX_REGISTER,
> +	SDP_STATE_TX_REGISTER_BUSY,
> +	SDP_STATE_TX_STATUS,
> +	SDP_STATE_TX_STATUS_BUSY,
> +	SDP_STATE_JUMP,
> +};
> +
> +struct f_sdp {
> +	struct usb_function		usb_function;
> +
> +	struct usb_descriptor_header	**function;
> +
> +	u8				altsetting;
> +	enum sdp_state			state;
> +	enum sdp_state			next_state;
> +	u32				dnl_address;
> +	u32				dnl_bytes_remaining;
> +	u32				jmp_address;
> +	bool				always_send_status;
> +	u32				error_status;
> +
> +	/* EP0 request */
> +	struct usb_request		*req;
> +
> +	/* EP1 IN */
> +	struct usb_ep			*in_ep;
> +	struct usb_request		*in_req;
> +
> +	bool				configuration_done;
> +};
> +
> +static struct f_sdp *sdp_func;
> +
> +static inline struct f_sdp *func_to_sdp(struct usb_function *f)
> +{
> +	return container_of(f, struct f_sdp, usb_function);
> +}
> +
> +static struct usb_interface_descriptor sdp_intf_runtime = {
> +	.bLength =		sizeof(sdp_intf_runtime),
> +	.bDescriptorType =	USB_DT_INTERFACE,
> +	.bAlternateSetting =	0,
> +	.bNumEndpoints =	1,
> +	.bInterfaceClass =	USB_CLASS_HID,
> +	.bInterfaceSubClass =	0,
> +	.bInterfaceProtocol =	0,
> +	/* .iInterface = DYNAMIC */
> +};
> +
> +/* HID configuration */
> +static struct usb_class_hid_descriptor sdp_hid_desc = {
> +	.bLength =		sizeof(sdp_hid_desc),
> +	.bDescriptorType =	USB_DT_CS_DEVICE,
> +
> +	.bcdCDC =		__constant_cpu_to_le16(0x0110),
> +	.bCountryCode =		0,
> +	.bNumDescriptors =	1,
> +
> +	.bDescriptorType0	= USB_DT_HID_REPORT,
> +	.wDescriptorLength0	= HID_USAGE_PAGE_LEN,
> +};
> +
> +static struct usb_endpoint_descriptor in_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT, /*USB_DT_CS_ENDPOINT*/
> +
> +	.bEndpointAddress =	1 | USB_DIR_IN,
> +	.bmAttributes =	USB_ENDPOINT_XFER_INT,
> +	.wMaxPacketSize =	64,
> +	.bInterval =		1,
> +};
> +
> +static struct usb_descriptor_header *sdp_runtime_descs[] = {
> +	(struct usb_descriptor_header *)&sdp_intf_runtime,
> +	(struct usb_descriptor_header *)&sdp_hid_desc,
> +	(struct usb_descriptor_header *)&in_desc,
> +	NULL,
> +};
> +
> +/* This is synchronized with what the SoC implementation reports */
> +static struct hid_report sdp_hid_report = {
> +	.usage_page = {
> +		0x06, 0x00, 0xff, /* Usage Page */
> +		0x09, 0x01, /* Usage (Poiter?) */
> +		0xa1, 0x01, /* Collection */
> +
> +		0x85, 0x01, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size */
> +		0x95, 0x10, /* Report Count */
> +		0x91, 0x02, /* Output Data */
> +
> +		0x85, 0x02, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x80, /* Report Size 128 */
> +		0x95, 0x40, /* Report Count */
> +		0x91, 0x02, /* Output Data */
> +
> +		0x85, 0x03, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size 8 */
> +		0x95, 0x04, /* Report Count */
> +		0x81, 0x02, /* Input Data */
> +
> +		0x85, 0x04, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size 8 */
> +		0x95, 0x40, /* Report Count */
> +		0x81, 0x02, /* Input Data */
> +		0xc0
> +	},
> +};
> +
> +static const char sdp_name[] = "Serial Downloader Protocol";
> +
> +/*
> + * static strings, in UTF-8
> + */
> +static struct usb_string strings_sdp_generic[] = {
> +	[0].s = sdp_name,
> +	{  }			/* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_sdp_generic = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_sdp_generic,
> +};
> +
> +static struct usb_gadget_strings *sdp_generic_strings[] = {
> +	&stringtab_sdp_generic,
> +	NULL,
> +};
> +
> +static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +	u8 *data = req->buf;
> +	u8 report = data[0];
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	if (report != 1) {
> +		error("Unexpected report %d", report);
> +		return;
> +	}
> +
> +	struct sdp_command *cmd = req->buf + 1;
> +
> +	debug("%s: command: %04x, addr: %08x, cnt: %u\n",
> +	      __func__, be16_to_cpu(cmd->cmd),
> +	      be32_to_cpu(cmd->addr), be32_to_cpu(cmd->cnt));
> +
> +	switch (be16_to_cpu(cmd->cmd)) {
> +	case SDP_READ_REGISTER:
> +		sdp->always_send_status = false;
> +		sdp->error_status = 0x0;
> +
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_TX_REGISTER;
> +		printf("Reading %d registers at 0x%08x... ",
> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
> +		break;
> +	case SDP_WRITE_FILE:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_WRITE_FILE_COMPLETE;
> +
> +		sdp->state = SDP_STATE_RX_FILE_DATA;
> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_IDLE;
> +
> +		printf("Downloading file of size %d to 0x%08x... ",
> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
> +
> +		break;
> +	case SDP_ERROR_STATUS:
> +		sdp->always_send_status = true;
> +		sdp->error_status = 0;
> +
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	case SDP_DCD_WRITE:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_WRITE_REGISTER_COMPLETE;
> +
> +		sdp->state = SDP_STATE_RX_DCD_DATA;
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	case SDP_JUMP_ADDRESS:
> +		sdp->always_send_status = false;
> +		sdp->error_status = 0;
> +
> +		sdp->jmp_address = be32_to_cpu(cmd->addr);
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_JUMP;
> +		break;
> +	case SDP_SKIP_DCD_HEADER:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_SKIP_DCD_HEADER_COMPLETE;
> +
> +		/* Ignore command, DCD not supported anyway */
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		error("Unknown command: %08x\n", be16_to_cpu(cmd->cmd));
> +	}
> +}
> +
> +static void sdp_rx_data_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +	u8 *data = req->buf;
> +	u8 report = data[0];
> +	int datalen = req->length - 1;
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	if (report != 2) {
> +		error("Unexpected report %d", report);
> +		return;
> +	}
> +
> +	if (sdp->dnl_bytes_remaining < datalen) {
> +		/*
> +		 * Some USB stacks require to send a complete buffer as
> +		 * specified in the HID descriptor. This leads to longer
> +		 * transfers than the file length, no problem for us.
> +		 */
> +		sdp->dnl_bytes_remaining = 0;
> +	} else {
> +		sdp->dnl_bytes_remaining -= datalen;
> +	}
> +
> +	if (sdp->state == SDP_STATE_RX_FILE_DATA) {
> +		memcpy((void *)sdp->dnl_address, req->buf + 1, datalen);
> +		sdp->dnl_address += datalen;
> +	}
> +
> +	if (sdp->dnl_bytes_remaining)
> +		return;
> +
> +	printf("done\n");
> +
> +	switch (sdp->state) {
> +	case SDP_STATE_RX_FILE_DATA:
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		break;
> +	case SDP_STATE_RX_DCD_DATA:
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		break;
> +	default:
> +		error("Invalid state: %d", sdp->state);
> +	}
> +}
> +
> +
> +
> +static void sdp_tx_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	switch (sdp->state) {
> +	case SDP_STATE_TX_SEC_CONF_BUSY:
> +		/* Not all commands require status report */
> +		if (sdp->always_send_status || sdp->error_status)
> +			sdp->state = SDP_STATE_TX_STATUS;
> +		else
> +			sdp->state = sdp->next_state;
> +
> +		break;
> +	case SDP_STATE_TX_STATUS_BUSY:
> +		sdp->state = sdp->next_state;
> +		break;
> +	case SDP_STATE_TX_REGISTER_BUSY:
> +		if (sdp->dnl_bytes_remaining)
> +			sdp->state = SDP_STATE_TX_REGISTER;
> +		else
> +			sdp->state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		error("Wrong State: %d", sdp->state);
> +		sdp->state = SDP_STATE_IDLE;
> +		break;
> +	}
> +	debug("%s complete --> %d, %d/%d\n", ep->name,
> +	      status, req->actual, req->length);
> +}
> +
> +static int sdp_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
> +{
> +	struct usb_gadget *gadget = f->config->cdev->gadget;
> +	struct usb_request *req = f->config->cdev->req;
> +	struct f_sdp *sdp = f->config->cdev->req->context;
> +	u16 len = le16_to_cpu(ctrl->wLength);
> +	u16 w_value = le16_to_cpu(ctrl->wValue);
> +	int value = 0;
> +	u8 req_type = ctrl->bRequestType & USB_TYPE_MASK;
> +
> +	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
>
s/0x%x/0x%04x/g ?

> +	debug("req_type: 0x%x ctrl->bRequest: 0x%x sdp->state: %d\n",
s/0x%x/0x%02x/g ?

[...]
> +static u32 sdp_jump_imxheader(void *address)
> +{
> +	flash_header_v2_t *headerv2 = address;
> +	ulong (*entry)(void);
> +
> +	if (headerv2->header.tag != IVT_HEADER_TAG) {
> +		printf("Header Tag is not a IMX image\n");
<nit>
s/a IMX/an IMX/'

> +		return SDP_ERROR_IMXHEADER;
> +	}
> +
> +	printf("Jumping to 0x%08x\n", headerv2->entry);
> +	entry = (void *)headerv2->entry;
> +	entry();
> +
> +	/* The image probably never returns hence we wont reach that point */
s/wont/won't/
</nit>

[...]
> +int sdp_add(struct usb_configuration *c)
> +{
> +	int id;
> +
> +	id = usb_string_id(c->cdev);
> +	if (id < 0)
> +		return id;
> +	strings_sdp_generic[0].id = id;
> +	sdp_intf_runtime.iInterface = id;
> +
> +	debug("%s: cdev: 0x%p gadget:0x%p gadget->ep0: 0x%p\n", __func__,
>
The string printed with '%p' already contains a '0x' prefix.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[U-Boot] [PATCH v1 5/7] spl: add serial download protocol (SDP) support

[Lothar Waßmann]

Hi,

On Fri,  4 Aug 2017 16:38:11 -0700 Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add USB serial download protocol support to SPL. If the SoC started
> in recovery mode the SPL will immediately switch to SDP and wait for
> further downloads/commands from the host side.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  common/spl/Kconfig          |  6 ++++++
>  common/spl/Makefile         |  1 +
>  common/spl/spl_sdp.c        | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/Makefile |  1 +
>  4 files changed, 46 insertions(+)
>  create mode 100644 common/spl/spl_sdp.c
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 4de81392b0..95378b98a0 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -646,6 +646,12 @@ config SPL_DFU_RAM
>  
>  endchoice
>  
> +config SPL_USB_SDP_SUPPORT
> +	bool "Support SDP (Serial Download Protocol)"
> +	help
> +	  Enable Serial Download Protocol (SDP) device support in SPL. This
> +	  allows to download images into memory and execute (jump to) them
> +	  using the same protocol as implemented by the i.MX family's boot ROM.
>  endif
>  
>  config SPL_WATCHDOG_SUPPORT
> diff --git a/common/spl/Makefile b/common/spl/Makefile
> index 47a64dd7d0..a979560acf 100644
> --- a/common/spl/Makefile
> +++ b/common/spl/Makefile
> @@ -29,4 +29,5 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
>  obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
>  obj-$(CONFIG_SPL_SPI_LOAD) += spl_spi.o
>  obj-$(CONFIG_SPL_RAM_SUPPORT) += spl_ram.o
> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += spl_sdp.o
>  endif
> diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
> new file mode 100644
> index 0000000000..faa74b8bec
> --- /dev/null
> +++ b/common/spl/spl_sdp.c
> @@ -0,0 +1,38 @@
> +/*
> + * (C) Copyright 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <spl.h>
> +#include <usb.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static int spl_sdp_load_image(struct spl_image_info *spl_image,
> +			      struct spl_boot_device *bootdev)
> +{
> +	int ret;
> +
> +	g_dnl_clear_detach();
> +	g_dnl_register("usb_dnl_sdp");
> +
> +	ret = sdp_init();
> +	if (ret) {
> +		error("SDP init failed: %d", ret);
> +		return -ENODEV;
> +	}
> +
> +	ret = sdp_handle();
>
This function (and sdp_init() above) always return 0. Do you expect to
be changed in future? Otherwise the return type of those functions
should be changed to void.


Lothar Waßmann

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Marcel Ziswiler]

Hi Stefano

On Tue, 2017-08-08 at 11:15 +0200, Stefano Babic wrote:
> Hi Stefan,
> 
> On 07/08/2017 20:06, Stefan Agner wrote:
> > Hi Eric,
> > 
> > On 2017-08-06 08:19, Eric Nelson wrote:
> > > Hi Stefan,
> > > 
> > > On 08/04/2017 04:38 PM, Stefan Agner wrote:
> > > > From: Stefan Agner <stefan.agner@toradex.com>
> > > > 
> > > > This series adds NXP's Serial Download Protocol (SDP) support
> > > > via
> > > > USB for SPL/U-Boot. It allows to download U-Boot via USB from a
> > > > (recovered) SPL using the same tools used to download SPL
> > > > itself
> > > > (specifically imx_usb, but also sb_loader seems to work).
> > > > 
> > > 
> > > Nice!
> > > 
> > > > The idea has been brought up when the first targets started to
> > > > make
> > > > use of SPL for DDR initialization, see:
> > > > https://lists.denx.de/pipermail/u-boot/2015-July/220330.html
> > > > 
> > > 
> > > There have been lots of discussions surrounding the use of SDP,
> > > and this seems to be a nice alternative to using the i.MX
> > > "plugin"
> > > mode that I explored a while back:
> > > 
> > > https://lists.denx.de/pipermail/u-boot/2017-July/thread.html#2982
> > > 66
> > > 
> > 
> > Hm, wasn't aware of that particular effort, thanks for pointing to
> > it.
> > From a quick glance, it did not work out so far, is this correct?
> > 
> > I looked into plugin mode also a little bit, but I did not continue
> > to
> > look into it after reading this sentence in the i.MX 6 RM:
> > 
> > 8.7 Plugin Image
> > The boot ROM detects the image type using the plugin flag of the
> > boot
> > data structure (see
> > Boot Data Structure). If the plugin flag is 1, then the ROM uses
> > the
> > image as a plugin
> > function. The function must initialize the boot device and copy the
> > program image to the
> > final location. At the end the plugin function must return with the
> > program image
> > parameters. (See High level boot sequence for details about boot
> > flow).
> > 
> > 
> > So if SPL should work as a plugin as NXP defines it, SPL is
> > supposed to
> > load the image from somewhere. The boot ROM then only takes care
> > about
> > image verification and further boot steps, but it's the plugins job
> > to
> > get the image from somewhere and store it in RAM.
> > 
> 
> Right, and this is also the weak point. There are also some cases (at
> least, in some projects of mine) where this conflicts with the setup
> of
> the RAM controller. We need to set the RAM controller to load the
> code
> or to fight with the limitation of the IRAM.
> 
> > Afact this is not really helpful in our case. We would want the
> > boot ROM
> > to go through the boot sequence (again).
> 
> Agree. This makes the whole boot process easier to understand - and
> not
> to mention the fact that code of BootROM is not officially published,
> and we cannot know what it exactly does.
> 
> > 
> > Unless returning an error/invalid image causes the boot ROM to go
> > through boot sequence again?
> > 
> > 
> > The nice thing with our own SDP implementation is we can reuse it
> > even
> > from within U-Boot again, e.g. to download a kernel/initramfs....
> 
> Right - I think SDP is a nice solution, and thanks for your effort !
> 
> > 
> > > > The initial SDP implementation (patch 2) requires the payload
> > > > to
> > > > have the imx specific headers (hence the move of the imx header
> > > > file in patch 1).
> > > > 
> > > > Patch 3 extends image header support beyond the SDP
> > > > specification,
> > > > specifically implements also support for U-Boot headers. This
> > > > allows to use the same SPL/U-Boot binaries for recovery as used
> > > > on
> > > > the regular boot device (SD/eMMC). For that to work also the
> > > > host
> > > > side imx_usb tools needed an extension, currently available
> > > > here:
> > > > 
> > > > https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_r
> > > > efactored
> > > > 
> > > > The full patchset allows to download SPL and U-Boot over USB to
> > > > a
> > > > target in recovery mode using the same usb_imx utility:
> > > 
> > > imx_usb?
> > > 
> > 
> > Yeah right, sorry.
> 
> But what about to merge into the official imx_usb repo ?

Yes, of course as Stefan mentioned before that is planned once this
made it into U-Boot proper.

> > > > The usb_imx utility also has a batch mode which allows to
> > > > download
> > > > multiple artifacts with a single invocation. The details are
> > > > outlined in the imx_usb commit message:
> > > > https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d2
> > > > 2332d9558bed6d42db9f60
> > > > 
> > > > In case this patchset gets accepted in U-Boot, I plan to push
> > > > the
> > > > imx_usb changes upstream as well.
> > > > 
> > > 
> > > Do you have numbers for how much code/data size this adds to SPL?
> > > 
> > 
> > Besides the protocol implementation also general USB (gadget)
> > support is
> > required, hence it adds quite a bit.
> > 
> > 
> > Without USB Gadget/SDP support (Apalis iMX6 SPL):
> > 
> > $ arm-linux-gnueabihf-size spl/u-boot-spl
> >    text    data     bss     dec     hex filename
> >   24552    3808      92   28452    6f24 spl/u-boot-spl
> > 
> > 
> > With USB Gadget/SDP support:
> > 
> > $ arm-linux-gnueabihf-size spl/u-boot-spl
> >    text    data     bss     dec     hex filename
> >   42549    4679    1984   49212    c03c spl/u-boot-spl
> 
> 
> mmmhhh...ok, I see.
> 
> > 
> > 
> > > I believe some i.MX users have struggled to stay within the
> > > size of internal RAM.
> > 
> > Hm, I think the limit is somewhere around 64kiB? In our case we are
> > still well below...
> 
> I will try to build. The issue is with MX6 Solo (and some lower level
> MX6) because it has 64KB IRAM and due to BootROM Stack the maximum
> size
> for SPL is lower (maybe 48K ?). As far as I see, apalis is MX6Q or
> MX6D,

We of course also have our Colibri iMX6 with both Solo and DualLite
options:

https://www.toradex.com/computer-on-modules/colibri-arm-family/nxp-free
scale-imx6#module_features

> in both cases no problem. If the size is too big for Solo, we will
> have
> to limit SDP's usage to the SOCs with more IRAM (Dual / Quad/ QP..)

No, it really works fine for us across all i.MX 6 variants.

> Best regards,
> Stefano

Cheers

Marcel

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Stefan Agner]

On 2017-08-08 02:15, Stefano Babic wrote:
> Hi Stefan,
> 
> On 07/08/2017 20:06, Stefan Agner wrote:
>> Hi Eric,
>>
>> On 2017-08-06 08:19, Eric Nelson wrote:
>>> Hi Stefan,
>>>
>>> On 08/04/2017 04:38 PM, Stefan Agner wrote:
>>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>>
>>>> This series adds NXP's Serial Download Protocol (SDP) support via
>>>> USB for SPL/U-Boot. It allows to download U-Boot via USB from a
>>>> (recovered) SPL using the same tools used to download SPL itself
>>>> (specifically imx_usb, but also sb_loader seems to work).
>>>>
>>>
>>> Nice!
>>>
>>>> The idea has been brought up when the first targets started to make
>>>> use of SPL for DDR initialization, see:
>>>> https://lists.denx.de/pipermail/u-boot/2015-July/220330.html
>>>>
>>>
>>> There have been lots of discussions surrounding the use of SDP,
>>> and this seems to be a nice alternative to using the i.MX "plugin"
>>> mode that I explored a while back:
>>>
>>> https://lists.denx.de/pipermail/u-boot/2017-July/thread.html#298266
>>>
>>
>> Hm, wasn't aware of that particular effort, thanks for pointing to it.
>> From a quick glance, it did not work out so far, is this correct?
>>
>> I looked into plugin mode also a little bit, but I did not continue to
>> look into it after reading this sentence in the i.MX 6 RM:
>>
>> 8.7 Plugin Image
>> The boot ROM detects the image type using the plugin flag of the boot
>> data structure (see
>> Boot Data Structure). If the plugin flag is 1, then the ROM uses the
>> image as a plugin
>> function. The function must initialize the boot device and copy the
>> program image to the
>> final location. At the end the plugin function must return with the
>> program image
>> parameters. (See High level boot sequence for details about boot flow).
>>
>>
>> So if SPL should work as a plugin as NXP defines it, SPL is supposed to
>> load the image from somewhere. The boot ROM then only takes care about
>> image verification and further boot steps, but it's the plugins job to
>> get the image from somewhere and store it in RAM.
>>
> 
> Right, and this is also the weak point. There are also some cases (at
> least, in some projects of mine) where this conflicts with the setup of
> the RAM controller. We need to set the RAM controller to load the code
> or to fight with the limitation of the IRAM.
> 
>> Afact this is not really helpful in our case. We would want the boot ROM
>> to go through the boot sequence (again).
> 
> Agree. This makes the whole boot process easier to understand - and not
> to mention the fact that code of BootROM is not officially published,
> and we cannot know what it exactly does.
> 

Yeah, I really wish NXP would add some functionality and document the
boot ROM clearly.

One issue I have currently is when the SPL gets downloaded via USB
serial download when the SoC entered serial download due to unbootable
device, spl_boot_mode cannot detect that.

Downstream uses "circumstantial evidence" to get this info:
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/tree/arch/arm/include/asm/arch-mx7/imx-regs.h?h=imx_v2016.03_4.1.15_2.0.0_ga#n1204

And I just learned that i.MX 7 Boot ROM actually provides a structure
with boot info now, see get_boot_device in arch/arm/mach-imx/mx7/soc.c.

If anybody has a reliable/nicer mechanism than the is_boot_from_usb from
downstream for i.MX 6 that I am all ears.

>>
>> Unless returning an error/invalid image causes the boot ROM to go
>> through boot sequence again?
>>
>>
>> The nice thing with our own SDP implementation is we can reuse it even
>> from within U-Boot again, e.g. to download a kernel/initramfs....
> 
> Right - I think SDP is a nice solution, and thanks for your effort !
> 
>>
>>>> The initial SDP implementation (patch 2) requires the payload to
>>>> have the imx specific headers (hence the move of the imx header
>>>> file in patch 1).
>>>>
>>>> Patch 3 extends image header support beyond the SDP specification,
>>>> specifically implements also support for U-Boot headers. This
>>>> allows to use the same SPL/U-Boot binaries for recovery as used on
>>>> the regular boot device (SD/eMMC). For that to work also the host
>>>> side imx_usb tools needed an extension, currently available here:
>>>>
>>>> https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_refactored
>>>>
>>>> The full patchset allows to download SPL and U-Boot over USB to a
>>>> target in recovery mode using the same usb_imx utility:
>>>
>>> imx_usb?
>>>
>>
>> Yeah right, sorry.
> 
> But what about to merge into the official imx_usb repo ?
> 

FWIW, many preparation patches actually already made it upstream:
https://github.com/boundarydevices/imx_usb_loader/pull/48

The pending patches are the changes which only make sense in case
upstream U-Boot gains support for this (e.g. U-Boot header support for
imx_usb).

It probably makes sense for reviewers/testers to look at those patches
too.

Especially the batch mode:
https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60

I was going to create the pull request after the U-Boot part gets
merged, but if preferred, I can create a pull request immediately so
people can review it over at Github?

>>
>>>> The usb_imx utility also has a batch mode which allows to download
>>>> multiple artifacts with a single invocation. The details are
>>>> outlined in the imx_usb commit message:
>>>> https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60
>>>>
>>>> In case this patchset gets accepted in U-Boot, I plan to push the
>>>> imx_usb changes upstream as well.
>>>>
>>>
>>> Do you have numbers for how much code/data size this adds to SPL?
>>>
>>
>> Besides the protocol implementation also general USB (gadget) support is
>> required, hence it adds quite a bit.
>>
>>
>> Without USB Gadget/SDP support (Apalis iMX6 SPL):
>>
>> $ arm-linux-gnueabihf-size spl/u-boot-spl
>>    text    data     bss     dec     hex filename
>>   24552    3808      92   28452    6f24 spl/u-boot-spl
>>
>>
>> With USB Gadget/SDP support:
>>
>> $ arm-linux-gnueabihf-size spl/u-boot-spl
>>    text    data     bss     dec     hex filename
>>   42549    4679    1984   49212    c03c spl/u-boot-spl
> 
> 
> mmmhhh...ok, I see.
> 
>>
>>
>>> I believe some i.MX users have struggled to stay within the
>>> size of internal RAM.
>>
>> Hm, I think the limit is somewhere around 64kiB? In our case we are
>> still well below...
> 
> I will try to build. The issue is with MX6 Solo (and some lower level
> MX6) because it has 64KB IRAM and due to BootROM Stack the maximum size
> for SPL is lower (maybe 48K ?). As far as I see, apalis is MX6Q or MX6D,
> in both cases no problem. If the size is too big for Solo, we will have
> to limit SDP's usage to the SOCs with more IRAM (Dual / Quad/ QP..)

As Marcel mentioned, we successfully run it on Solo/DualLite too.

Boot ROM stack? Since it is not running as plugin, can we not just trash
that?

--
Stefan

[U-Boot] [PATCH v1 5/7] spl: add serial download protocol (SDP) support

[Stefan Agner]

On 2017-08-08 03:43, Lothar Waßmann wrote:
> Hi,
> 
> On Fri,  4 Aug 2017 16:38:11 -0700 Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Add USB serial download protocol support to SPL. If the SoC started
>> in recovery mode the SPL will immediately switch to SDP and wait for
>> further downloads/commands from the host side.
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> ---
>>
>>  common/spl/Kconfig          |  6 ++++++
>>  common/spl/Makefile         |  1 +
>>  common/spl/spl_sdp.c        | 38 ++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/gadget/Makefile |  1 +
>>  4 files changed, 46 insertions(+)
>>  create mode 100644 common/spl/spl_sdp.c
>>
>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>> index 4de81392b0..95378b98a0 100644
>> --- a/common/spl/Kconfig
>> +++ b/common/spl/Kconfig
>> @@ -646,6 +646,12 @@ config SPL_DFU_RAM
>>
>>  endchoice
>>
>> +config SPL_USB_SDP_SUPPORT
>> +	bool "Support SDP (Serial Download Protocol)"
>> +	help
>> +	  Enable Serial Download Protocol (SDP) device support in SPL. This
>> +	  allows to download images into memory and execute (jump to) them
>> +	  using the same protocol as implemented by the i.MX family's boot ROM.
>>  endif
>>
>>  config SPL_WATCHDOG_SUPPORT
>> diff --git a/common/spl/Makefile b/common/spl/Makefile
>> index 47a64dd7d0..a979560acf 100644
>> --- a/common/spl/Makefile
>> +++ b/common/spl/Makefile
>> @@ -29,4 +29,5 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
>>  obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
>>  obj-$(CONFIG_SPL_SPI_LOAD) += spl_spi.o
>>  obj-$(CONFIG_SPL_RAM_SUPPORT) += spl_ram.o
>> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += spl_sdp.o
>>  endif
>> diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
>> new file mode 100644
>> index 0000000000..faa74b8bec
>> --- /dev/null
>> +++ b/common/spl/spl_sdp.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * (C) Copyright 2016 Toradex
>> + * Author: Stefan Agner <stefan.agner@toradex.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <spl.h>
>> +#include <usb.h>
>> +#include <g_dnl.h>
>> +#include <sdp.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +static int spl_sdp_load_image(struct spl_image_info *spl_image,
>> +			      struct spl_boot_device *bootdev)
>> +{
>> +	int ret;
>> +
>> +	g_dnl_clear_detach();
>> +	g_dnl_register("usb_dnl_sdp");
>> +
>> +	ret = sdp_init();
>> +	if (ret) {
>> +		error("SDP init failed: %d", ret);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = sdp_handle();
>>
> This function (and sdp_init() above) always return 0. Do you expect to
> be changed in future? Otherwise the return type of those functions
> should be changed to void.
> 

I followed f_thor's approach here which use int in both functions, but
yeah, in the SDP case there is currently no error condition.

Not sure there will be one added in the future, I guess we could just
make it void for now.

--
Stefan

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Łukasz Majewski]

Hi Stefan,

> From: Stefan Agner <stefan.agner@toradex.com>
>
> This series adds NXP's Serial Download Protocol (SDP) support via
> USB for SPL/U-Boot. It allows to download U-Boot via USB from a
> (recovered) SPL using the same tools used to download SPL itself
> (specifically imx_usb, but also sb_loader seems to work).

If I might ask - could you prepare some ./doc/README.sdp entry for v2?

I mean some kind of howto for using this feature?

Nothing long - just the most important commands (imx_usb?) including the 
list of supported (and not) features.

>
> The idea has been brought up when the first targets started to make
> use of SPL for DDR initialization, see:
> https://lists.denx.de/pipermail/u-boot/2015-July/220330.html
>
> The initial SDP implementation (patch 2) requires the payload to
> have the imx specific headers (hence the move of the imx header
> file in patch 1).
>
> Patch 3 extends image header support beyond the SDP specification,
> specifically implements also support for U-Boot headers. This
> allows to use the same SPL/U-Boot binaries for recovery as used on
> the regular boot device (SD/eMMC). For that to work also the host
> side imx_usb tools needed an extension, currently available here:
>
> https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_refactored
>
> The full patchset allows to download SPL and U-Boot over USB to a
> target in recovery mode using the same usb_imx utility:
>
> The usb_imx utility also has a batch mode which allows to download
> multiple artifacts with a single invocation. The details are
> outlined in the imx_usb commit message:
> https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60
>
> In case this patchset gets accepted in U-Boot, I plan to push the
> imx_usb changes upstream as well.

Thanks for the patch set. It would definitely facilitate work with IMX SoCs.

>
>
> Stefan Agner (7):
>   imx: move imximage header to common location
>   usb: gadget: add SDP driver
>   usb: gadget: sdp: extend images compatible for jumps
>   cmd: add sdp command
>   spl: add serial download protocol (SDP) support
>   apalis/colibri_imx6: use independent USB PID for SPL
>   apalis/colibri_imx6: enable SDP by default
>
>  board/toradex/apalis_imx6/apalis_imx6.c   |  13 +
>  board/toradex/colibri_imx6/colibri_imx6.c |  13 +
>  cmd/Kconfig                               |   7 +
>  cmd/Makefile                              |   1 +
>  cmd/usb_gadget_sdp.c                      |  53 +++
>  common/spl/Kconfig                        |   6 +
>  common/spl/Makefile                       |   1 +
>  common/spl/spl_sdp.c                      |  38 ++
>  configs/apalis_imx6_defconfig             |   4 +
>  configs/colibri_imx6_defconfig            |   4 +
>  drivers/usb/gadget/Kconfig                |   7 +
>  drivers/usb/gadget/Makefile               |   2 +
>  drivers/usb/gadget/f_sdp.c                | 739 ++++++++++++++++++++++++++++++
>  {tools => include}/imximage.h             |   0
>  include/sdp.h                             |  16 +
>  15 files changed, 904 insertions(+)
>  create mode 100644 cmd/usb_gadget_sdp.c
>  create mode 100644 common/spl/spl_sdp.c
>  create mode 100644 drivers/usb/gadget/f_sdp.c
>  rename {tools => include}/imximage.h (100%)
>  create mode 100644 include/sdp.h
>


-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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

[U-Boot] [PATCH v1 1/7] imx: move imximage header to common location

[Łukasz Majewski]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 839 bytes --]

On 08/05/2017 01:38 AM, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> Move the imximage.h header file to a common location so we can make
> use of it from U-Boot too.
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
>
>  {tools => include}/imximage.h | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename {tools => include}/imximage.h (100%)
>
> diff --git a/tools/imximage.h b/include/imximage.h
> similarity index 100%
> rename from tools/imximage.h
> rename to include/imximage.h
>

Reviewed-by: Łukasz Majewski <lukma@denx.de>

-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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

[U-Boot] [PATCH v1 2/7] usb: gadget: add SDP driver

[Łukasz Majewski]

Hi Stefan,

> From: Stefan Agner <stefan.agner@toradex.com>
>
> Add SDP (Serial Downloader Protocol) implementation for U-Boot. The
> protocol is used in NXP SoC's boot ROM and allows to download program
> images. Beside that, it can also be used to read/write registers and
> download complete Device Configuration Data (DCD) sets. This basic
> implementation supports downloading images with the imx header format
> and reading registers.
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>

Just some minor comments (despite comments from Lothar).

> ---
>
>  drivers/usb/gadget/Kconfig  |   7 +
>  drivers/usb/gadget/Makefile |   1 +
>  drivers/usb/gadget/f_sdp.c  | 723 ++++++++++++++++++++++++++++++++++++++++++++
>  include/sdp.h               |  16 +
>  4 files changed, 747 insertions(+)
>  create mode 100644 drivers/usb/gadget/f_sdp.c
>  create mode 100644 include/sdp.h
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 261ed128ac..225b66bc95 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -103,6 +103,13 @@ config USB_GADGET_DOWNLOAD
>
>  if USB_GADGET_DOWNLOAD
>
> +config USB_FUNCTION_SDP
> +	bool "Enable USB SDP (Serial Download Protocol)"
> +	help
> +	  Enable Serial Download Protocol (SDP) device support in U-Boot. This
> +	  allows to download images into memory and execute (jump to) them
> +	  using the same protocol as implemented by the i.MX family's boot ROM.
> +
>  config G_DNL_MANUFACTURER
>  	string "Vendor name of USB device"
>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 5e316a7cff..6a007d1bcb 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_USB_FUNCTION_THOR) += f_thor.o
>  obj-$(CONFIG_USB_FUNCTION_DFU) += f_dfu.o
>  obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
>  obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
> +obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
>  endif
>  endif
>  ifdef CONFIG_USB_ETHER
> diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> new file mode 100644
> index 0000000000..eb89695aaf
> --- /dev/null
> +++ b/drivers/usb/gadget/f_sdp.c
> @@ -0,0 +1,723 @@
> +/*
> + * f_sdp.c -- USB HID Serial Download Protocol
> + *
> + * Copyright (C) 2016 Toradex
		    ^^^^ - minor -> If you are going to prepare v2, please update the 
date.

> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * This file implements the Serial Download Protocol (SDP) as specified in
> + * the i.MX 6 Reference Manual. The SDP is a USB HID based protocol and
> + * allows to download images directly to memory. The implementation
> + * works with the imx_loader (imx_usb) USB client software on host side.
> + *
> + * Not all commands are implemented, e.g. WRITE_REGISTER, DCD_WRITE and
> + * SKIP_DCD_HEADER are only stubs.
> + *
> + * Parts of the implementation are based on f_dfu and f_thor.
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <errno.h>
> +#include <common.h>
> +#include <console.h>
> +#include <malloc.h>
> +
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/composite.h>
> +
> +#include <asm/io.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +#include <imximage.h>
> +
> +#define HID_REPORT_ID_MASK	0x000000ff
> +
> +/*
> + * HID class requests
> + */
> +#define HID_REQ_GET_REPORT		0x01
> +#define HID_REQ_GET_IDLE		0x02
> +#define HID_REQ_GET_PROTOCOL		0x03
> +#define HID_REQ_SET_REPORT		0x09
> +#define HID_REQ_SET_IDLE		0x0A
> +#define HID_REQ_SET_PROTOCOL		0x0B
> +
> +#define HID_USAGE_PAGE_LEN		76
> +
> +struct hid_report {
> +	u8 usage_page[HID_USAGE_PAGE_LEN];
> +} __packed;
> +
> +#define SDP_READ_REGISTER	0x0101
> +#define SDP_WRITE_REGISTER	0x0202
> +#define SDP_WRITE_FILE		0x0404
> +#define SDP_ERROR_STATUS	0x0505
> +#define SDP_DCD_WRITE		0x0a0a
> +#define SDP_JUMP_ADDRESS	0x0b0b
> +#define SDP_SKIP_DCD_HEADER	0x0c0c
> +
> +#define SDP_WRITE_FILE_COMPLETE		0x88888888
> +#define SDP_WRITE_REGISTER_COMPLETE	0x128A8A12
> +#define SDP_SKIP_DCD_HEADER_COMPLETE	0x900DD009
> +#define SDP_ERROR_IMXHEADER		0x000a0533
> +
> +#define SDP_COMMAND_LEN		16
> +
> +struct sdp_command {
> +	u16 cmd;
> +	u32 addr;
> +	u8 format;
> +	u32 cnt;
> +	u32 data;
> +	u8 rsvd;
> +} __packed;
> +
> +enum sdp_state {
> +	SDP_STATE_IDLE,
> +	SDP_STATE_RX_DCD_DATA,
> +	SDP_STATE_RX_FILE_DATA,
> +	SDP_STATE_TX_SEC_CONF,
> +	SDP_STATE_TX_SEC_CONF_BUSY,
> +	SDP_STATE_TX_REGISTER,
> +	SDP_STATE_TX_REGISTER_BUSY,
> +	SDP_STATE_TX_STATUS,
> +	SDP_STATE_TX_STATUS_BUSY,
> +	SDP_STATE_JUMP,
> +};
> +
> +struct f_sdp {
> +	struct usb_function		usb_function;
> +
> +	struct usb_descriptor_header	**function;
> +
> +	u8				altsetting;
> +	enum sdp_state			state;
> +	enum sdp_state			next_state;
> +	u32				dnl_address;
> +	u32				dnl_bytes_remaining;
> +	u32				jmp_address;
> +	bool				always_send_status;
> +	u32				error_status;
> +
> +	/* EP0 request */
> +	struct usb_request		*req;
> +
> +	/* EP1 IN */
> +	struct usb_ep			*in_ep;
> +	struct usb_request		*in_req;
> +
> +	bool				configuration_done;
> +};
> +
> +static struct f_sdp *sdp_func;
> +
> +static inline struct f_sdp *func_to_sdp(struct usb_function *f)
> +{
> +	return container_of(f, struct f_sdp, usb_function);
> +}
> +
> +static struct usb_interface_descriptor sdp_intf_runtime = {
> +	.bLength =		sizeof(sdp_intf_runtime),
> +	.bDescriptorType =	USB_DT_INTERFACE,
> +	.bAlternateSetting =	0,
> +	.bNumEndpoints =	1,
> +	.bInterfaceClass =	USB_CLASS_HID,
> +	.bInterfaceSubClass =	0,
> +	.bInterfaceProtocol =	0,
> +	/* .iInterface = DYNAMIC */
> +};
> +
> +/* HID configuration */
> +static struct usb_class_hid_descriptor sdp_hid_desc = {
> +	.bLength =		sizeof(sdp_hid_desc),
> +	.bDescriptorType =	USB_DT_CS_DEVICE,
> +
> +	.bcdCDC =		__constant_cpu_to_le16(0x0110),
> +	.bCountryCode =		0,
> +	.bNumDescriptors =	1,
> +
> +	.bDescriptorType0	= USB_DT_HID_REPORT,
> +	.wDescriptorLength0	= HID_USAGE_PAGE_LEN,
> +};
> +
> +static struct usb_endpoint_descriptor in_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT, /*USB_DT_CS_ENDPOINT*/
> +
> +	.bEndpointAddress =	1 | USB_DIR_IN,
> +	.bmAttributes =	USB_ENDPOINT_XFER_INT,
> +	.wMaxPacketSize =	64,
> +	.bInterval =		1,
> +};
> +
> +static struct usb_descriptor_header *sdp_runtime_descs[] = {
> +	(struct usb_descriptor_header *)&sdp_intf_runtime,
> +	(struct usb_descriptor_header *)&sdp_hid_desc,
> +	(struct usb_descriptor_header *)&in_desc,
> +	NULL,
> +};
> +
> +/* This is synchronized with what the SoC implementation reports */
> +static struct hid_report sdp_hid_report = {
> +	.usage_page = {
> +		0x06, 0x00, 0xff, /* Usage Page */
> +		0x09, 0x01, /* Usage (Poiter?) */
				      ^^^^^^^ Pointer?

> +		0xa1, 0x01, /* Collection */
> +
> +		0x85, 0x01, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size */
> +		0x95, 0x10, /* Report Count */
> +		0x91, 0x02, /* Output Data */
> +
> +		0x85, 0x02, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x80, /* Report Size 128 */
> +		0x95, 0x40, /* Report Count */
> +		0x91, 0x02, /* Output Data */
> +
> +		0x85, 0x03, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size 8 */
> +		0x95, 0x04, /* Report Count */
> +		0x81, 0x02, /* Input Data */
> +
> +		0x85, 0x04, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size 8 */
> +		0x95, 0x40, /* Report Count */
> +		0x81, 0x02, /* Input Data */
> +		0xc0
> +	},
> +};
> +
> +static const char sdp_name[] = "Serial Downloader Protocol";
> +
> +/*
> + * static strings, in UTF-8
> + */
> +static struct usb_string strings_sdp_generic[] = {
> +	[0].s = sdp_name,
> +	{  }			/* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_sdp_generic = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_sdp_generic,
> +};
> +
> +static struct usb_gadget_strings *sdp_generic_strings[] = {
> +	&stringtab_sdp_generic,
> +	NULL,
> +};
> +
> +static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +	u8 *data = req->buf;
> +	u8 report = data[0];
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	if (report != 1) {
> +		error("Unexpected report %d", report);
> +		return;
> +	}
> +
> +	struct sdp_command *cmd = req->buf + 1;
> +
> +	debug("%s: command: %04x, addr: %08x, cnt: %u\n",
> +	      __func__, be16_to_cpu(cmd->cmd),
> +	      be32_to_cpu(cmd->addr), be32_to_cpu(cmd->cnt));
> +
> +	switch (be16_to_cpu(cmd->cmd)) {
> +	case SDP_READ_REGISTER:
> +		sdp->always_send_status = false;
> +		sdp->error_status = 0x0;
> +
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_TX_REGISTER;
> +		printf("Reading %d registers at 0x%08x... ",
> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
> +		break;
> +	case SDP_WRITE_FILE:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_WRITE_FILE_COMPLETE;
> +
> +		sdp->state = SDP_STATE_RX_FILE_DATA;
> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_IDLE;
> +
> +		printf("Downloading file of size %d to 0x%08x... ",
> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
> +
> +		break;
> +	case SDP_ERROR_STATUS:
> +		sdp->always_send_status = true;
> +		sdp->error_status = 0;
> +
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	case SDP_DCD_WRITE:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_WRITE_REGISTER_COMPLETE;
> +
> +		sdp->state = SDP_STATE_RX_DCD_DATA;
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	case SDP_JUMP_ADDRESS:
> +		sdp->always_send_status = false;
> +		sdp->error_status = 0;
> +
> +		sdp->jmp_address = be32_to_cpu(cmd->addr);
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_JUMP;
> +		break;
> +	case SDP_SKIP_DCD_HEADER:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_SKIP_DCD_HEADER_COMPLETE;
> +
> +		/* Ignore command, DCD not supported anyway */
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		error("Unknown command: %08x\n", be16_to_cpu(cmd->cmd));
> +	}
> +}
> +
> +static void sdp_rx_data_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +	u8 *data = req->buf;
> +	u8 report = data[0];
> +	int datalen = req->length - 1;
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	if (report != 2) {
> +		error("Unexpected report %d", report);
> +		return;
> +	}
> +
> +	if (sdp->dnl_bytes_remaining < datalen) {
> +		/*
> +		 * Some USB stacks require to send a complete buffer as
> +		 * specified in the HID descriptor. This leads to longer
> +		 * transfers than the file length, no problem for us.
> +		 */
> +		sdp->dnl_bytes_remaining = 0;
> +	} else {
> +		sdp->dnl_bytes_remaining -= datalen;
> +	}
> +
> +	if (sdp->state == SDP_STATE_RX_FILE_DATA) {
> +		memcpy((void *)sdp->dnl_address, req->buf + 1, datalen);
> +		sdp->dnl_address += datalen;
> +	}
> +
> +	if (sdp->dnl_bytes_remaining)
> +		return;
> +
> +	printf("done\n");
> +
> +	switch (sdp->state) {
> +	case SDP_STATE_RX_FILE_DATA:
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		break;
> +	case SDP_STATE_RX_DCD_DATA:
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		break;
> +	default:
> +		error("Invalid state: %d", sdp->state);
> +	}
> +}
> +
> +

Please remove blank lines.

> +
> +static void sdp_tx_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	switch (sdp->state) {
> +	case SDP_STATE_TX_SEC_CONF_BUSY:
> +		/* Not all commands require status report */
> +		if (sdp->always_send_status || sdp->error_status)
> +			sdp->state = SDP_STATE_TX_STATUS;
> +		else
> +			sdp->state = sdp->next_state;
> +
> +		break;
> +	case SDP_STATE_TX_STATUS_BUSY:
> +		sdp->state = sdp->next_state;
> +		break;
> +	case SDP_STATE_TX_REGISTER_BUSY:
> +		if (sdp->dnl_bytes_remaining)
> +			sdp->state = SDP_STATE_TX_REGISTER;
> +		else
> +			sdp->state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		error("Wrong State: %d", sdp->state);
> +		sdp->state = SDP_STATE_IDLE;
> +		break;
> +	}
> +	debug("%s complete --> %d, %d/%d\n", ep->name,
> +	      status, req->actual, req->length);
> +}
> +
> +static int sdp_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
> +{
> +	struct usb_gadget *gadget = f->config->cdev->gadget;
> +	struct usb_request *req = f->config->cdev->req;
> +	struct f_sdp *sdp = f->config->cdev->req->context;
> +	u16 len = le16_to_cpu(ctrl->wLength);
> +	u16 w_value = le16_to_cpu(ctrl->wValue);
> +	int value = 0;
> +	u8 req_type = ctrl->bRequestType & USB_TYPE_MASK;
> +
> +	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
> +	debug("req_type: 0x%x ctrl->bRequest: 0x%x sdp->state: %d\n",
> +	      req_type, ctrl->bRequest, sdp->state);
> +
> +	if (req_type == USB_TYPE_STANDARD) {
> +		if (ctrl->bRequest == USB_REQ_GET_DESCRIPTOR) {
> +			/* Send HID report descriptor */
> +			value = min(len, (u16) sizeof(sdp_hid_report));
> +			memcpy(req->buf, &sdp_hid_report, value);
> +			sdp->configuration_done = true;
> +		}
> +	}
> +
> +	if (req_type == USB_TYPE_CLASS) {
> +		int report = w_value & HID_REPORT_ID_MASK;
> +
> +		/* HID (SDP) request */
> +		switch (ctrl->bRequest) {
> +		case HID_REQ_SET_REPORT:
> +			switch (report) {
> +			case 1:
> +				value = SDP_COMMAND_LEN + 1;
> +				req->complete = sdp_rx_command_complete;
> +				break;
> +			case 2:
> +				value = len;
> +				req->complete = sdp_rx_data_complete;
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (value >= 0) {
> +		req->length = value;
> +		req->zero = value < len;
> +		value = usb_ep_queue(gadget->ep0, req, 0);
> +		if (value < 0) {
> +			debug("ep_queue --> %d\n", value);
> +			req->status = 0;
> +		}
> +	}
> +
> +	return value;
> +}
> +
> +static int sdp_bind(struct usb_configuration *c, struct usb_function *f)
> +{
> +	struct usb_gadget *gadget = c->cdev->gadget;
> +	struct usb_composite_dev *cdev = c->cdev;
> +	struct f_sdp *sdp = func_to_sdp(f);
> +	int rv = 0, id;
> +
> +	id = usb_interface_id(c, f);
> +	if (id < 0)
> +		return id;
> +	sdp_intf_runtime.bInterfaceNumber = id;
> +
> +	struct usb_ep *ep;
> +
> +	/* allocate instance-specific endpoints */
> +	ep = usb_ep_autoconfig(gadget, &in_desc);
> +	if (!ep) {
> +		rv = -ENODEV;
> +		goto error;
> +	}
> +
> +	sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
> +
> +	cdev->req->context = sdp;
> +
> +error:
> +	return rv;
> +}
> +
> +static void sdp_unbind(struct usb_configuration *c, struct usb_function *f)
> +{
> +	free(sdp_func);
> +	sdp_func = NULL;
> +}
> +
> +static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length)
> +{
> +	struct usb_request *req;
> +
> +	req = usb_ep_alloc_request(ep, 0);
> +	if (!req)
> +		return req;
> +
> +	req->length = length;
> +	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, length);
> +	if (!req->buf) {
> +		usb_ep_free_request(ep, req);
> +		req = NULL;
> +	}
> +
> +	return req;
> +}
> +
> +
> +static struct usb_request *sdp_start_ep(struct usb_ep *ep)
> +{
> +	struct usb_request *req;
> +
> +	req = alloc_ep_req(ep, 64);
> +	debug("%s: ep:%p req:%p\n", __func__, ep, req);
> +
> +	if (!req)
> +		return NULL;
> +
> +	memset(req->buf, 0, req->length);
> +	req->complete = sdp_tx_complete;
> +
> +	return req;
> +}
> +static int sdp_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
> +{
> +	struct f_sdp *sdp = func_to_sdp(f);
> +	struct usb_composite_dev *cdev = f->config->cdev;
> +	int result;
> +
> +	debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
> +
> +	result = usb_ep_enable(sdp->in_ep, &in_desc);
> +	if (result)
> +		return result;
> +	sdp->in_req = sdp_start_ep(sdp->in_ep);
> +	sdp->in_req->context = sdp;
> +
> +	sdp->in_ep->driver_data = cdev; /* claim */
> +
> +	sdp->altsetting = alt;
> +	sdp->state = SDP_STATE_IDLE;
> +
> +	return 0;
> +}
> +
> +static int sdp_get_alt(struct usb_function *f, unsigned intf)
> +{
> +	struct f_sdp *sdp = func_to_sdp(f);
> +
> +	return sdp->altsetting;
> +}
> +
> +static void sdp_disable(struct usb_function *f)
> +{
> +	struct f_sdp *sdp = func_to_sdp(f);
> +
> +	usb_ep_disable(sdp->in_ep);
> +
> +	if (sdp->in_req) {
> +		free(sdp->in_req);
> +		sdp->in_req = NULL;
> +	}
> +}
> +
> +static int sdp_bind_config(struct usb_configuration *c)
> +{
> +	int status;
> +
> +	if (!sdp_func) {
> +		sdp_func = memalign(CONFIG_SYS_CACHELINE_SIZE, sizeof(*sdp_func));
> +		if (!sdp_func)
> +			return -ENOMEM;
> +	}
> +
> +	memset(sdp_func, 0, sizeof(*sdp_func));
> +
> +	sdp_func->usb_function.name = "sdp";
> +	sdp_func->usb_function.hs_descriptors = sdp_runtime_descs;
> +	sdp_func->usb_function.descriptors = sdp_runtime_descs;
> +	sdp_func->usb_function.bind = sdp_bind;
> +	sdp_func->usb_function.unbind = sdp_unbind;
> +	sdp_func->usb_function.set_alt = sdp_set_alt;
> +	sdp_func->usb_function.get_alt = sdp_get_alt;
> +	sdp_func->usb_function.disable = sdp_disable;
> +	sdp_func->usb_function.strings = sdp_generic_strings;
> +	sdp_func->usb_function.setup = sdp_setup;
> +
> +	status = usb_add_function(c, &sdp_func->usb_function);
> +
> +	return status;
> +}
> +
> +int sdp_init(void)
> +{
> +	printf("SDP: initialize...\n");
> +	while (!sdp_func->configuration_done) {
> +		if (ctrlc()) {
> +			puts("\rCTRL+C - Operation aborted.\n");
> +			return 0;
> +		}
> +		usb_gadget_handle_interrupts(0);
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 sdp_jump_imxheader(void *address)
> +{
> +	flash_header_v2_t *headerv2 = address;
> +	ulong (*entry)(void);
> +
> +	if (headerv2->header.tag != IVT_HEADER_TAG) {
> +		printf("Header Tag is not a IMX image\n");
> +		return SDP_ERROR_IMXHEADER;
> +	}
> +
> +	printf("Jumping to 0x%08x\n", headerv2->entry);
> +	entry = (void *)headerv2->entry;
> +	entry();
> +
> +	/* The image probably never returns hence we wont reach that point */
> +	return 0;
> +}
> +
> +static void sdp_handle_in_ep(void)
> +{
> +	u8 *data = sdp_func->in_req->buf;
> +	u32 status;
> +	int datalen;
> +
> +	switch (sdp_func->state) {
> +	case SDP_STATE_TX_SEC_CONF:
> +		debug("Report 3: HAB security\n");
> +		data[0] = 3;
> +
> +		data[1] = 0x56;
> +		data[2] = 0x78;
> +		data[3] = 0x78;
> +		data[4] = 0x56;
> +
> +		sdp_func->in_req->length = 5;
> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
> +		sdp_func->state = SDP_STATE_TX_SEC_CONF_BUSY;
> +		break;
> +
> +	case SDP_STATE_TX_STATUS:
> +		debug("Report 4: Status\n");
> +		data[0] = 4;
> +
> +		memcpy(&data[1], &sdp_func->error_status, 4);
> +		sdp_func->in_req->length = 65;
> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
> +		sdp_func->state = SDP_STATE_TX_STATUS_BUSY;
> +		break;
> +	case SDP_STATE_TX_REGISTER:
> +		debug("Report 4: Register Values\n");
> +		data[0] = 4;
> +
> +		datalen = sdp_func->dnl_bytes_remaining;
> +
> +		if (datalen > 64)
> +			datalen = 64;
> +
> +		memcpy(&data[1], (void *)sdp_func->dnl_address, datalen);
> +		sdp_func->in_req->length = 65;
> +
> +		sdp_func->dnl_bytes_remaining -= datalen;
> +		sdp_func->dnl_address += datalen;
> +
> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
> +		sdp_func->state = SDP_STATE_TX_REGISTER_BUSY;
> +		break;
> +	case SDP_STATE_JUMP:
> +		printf("Checking imxheader at 0x%08x\n", f_sdp->jmp_address);
> +		status = sdp_jump_imxheader((void *)f_sdp->jmp_address);
> +
> +		sdp_func->next_state = SDP_STATE_IDLE;
> +		sdp_func->error_status = status;
> +
> +		/* Only send Report 4 if there was an error */
> +		if (status)
> +			sdp_func->state = SDP_STATE_TX_STATUS;
> +		else
> +			sdp_func->state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		break;
> +	};
> +}
> +
> +int sdp_handle(void)
> +{
> +	printf("SDP: handle requests...\n");
> +	while (1) {
> +		if (ctrlc()) {
> +			puts("\rCTRL+C - Operation aborted.\n");
> +			return 0;
> +		}
> +
> +		usb_gadget_handle_interrupts(0);
> +
> +		sdp_handle_in_ep();
> +	}
> +}
> +
> +int sdp_add(struct usb_configuration *c)
> +{
> +	int id;
> +
> +	id = usb_string_id(c->cdev);
> +	if (id < 0)
> +		return id;
> +	strings_sdp_generic[0].id = id;
> +	sdp_intf_runtime.iInterface = id;
> +
> +	debug("%s: cdev: 0x%p gadget:0x%p gadget->ep0: 0x%p\n", __func__,
> +	      c->cdev, c->cdev->gadget, c->cdev->gadget->ep0);
> +
> +	return sdp_bind_config(c);
> +}
> +
> +DECLARE_GADGET_BIND_CALLBACK(usb_dnl_sdp, sdp_add);
> diff --git a/include/sdp.h b/include/sdp.h
> new file mode 100644
> index 0000000000..03c4a23434
> --- /dev/null
> +++ b/include/sdp.h
> @@ -0,0 +1,16 @@
> +/*
> + * sdp.h - Serial Download Protocol
> + *
> + * Copyright (C) 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef __SDP_H_
> +#define __SDP_H_
> +
> +int sdp_init(void);
> +int sdp_handle(void);
> +
> +#endif /* __SDP_H_ */
>


-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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

[U-Boot] [PATCH v1 3/7] usb: gadget: sdp: extend images compatible for jumps

[Łukasz Majewski]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 2138 bytes --]

Hi Stefan,

> From: Stefan Agner <stefan.agner@toradex.com>
>
> Support U-Boot images in SPL so that u-boot.img files can be
> directly downloaded and executed. Furthermore support U-Boot
> scripts download and execution in full U-Boot so that custom
> recovery actions can be downloaded from the host in a third
> step.
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>

Reviewed-by: Łukasz Majewski <lukma@denx.de>

> ---
>
>  drivers/usb/gadget/f_sdp.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> index eb89695aaf..9a752843f0 100644
> --- a/drivers/usb/gadget/f_sdp.c
> +++ b/drivers/usb/gadget/f_sdp.c
> @@ -29,6 +29,8 @@
>  #include <asm/io.h>
>  #include <g_dnl.h>
>  #include <sdp.h>
> +#include <spl.h>
> +#include <image.h>
>  #include <imximage.h>
>
>  #define HID_REPORT_ID_MASK	0x000000ff
> @@ -672,8 +674,22 @@ static void sdp_handle_in_ep(void)
>  		sdp_func->state = SDP_STATE_TX_REGISTER_BUSY;
>  		break;
>  	case SDP_STATE_JUMP:
> -		printf("Checking imxheader at 0x%08x\n", f_sdp->jmp_address);
> -		status = sdp_jump_imxheader((void *)f_sdp->jmp_address);
> +		printf("Jumping to header at 0x%08x\n", sdp_func->jmp_address);
> +		status = sdp_jump_imxheader((void *)sdp_func->jmp_address);
> +
> +		/* If imx header fails, try some U-Boot specific headers */
> +		if (status) {
> +#ifdef CONFIG_SPL_BUILD
> +			/* In SPL, allow jumps to U-Boot images */
> +			struct spl_image_info spl_image = {};
> +			spl_parse_image_header(&spl_image,
> +				(struct image_header *)sdp_func->jmp_address);
> +			jump_to_image_no_args(&spl_image);
> +#else
> +			/* In U-Boot, allow jumps to scripts */
> +			source(sdp_func->jmp_address, "script at 1");
> +#endif
> +		}
>
>  		sdp_func->next_state = SDP_STATE_IDLE;
>  		sdp_func->error_status = status;
>


-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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

[U-Boot] [PATCH v1 4/7] cmd: add sdp command

[Łukasz Majewski]

On 08/05/2017 01:38 AM, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> Add a new command to start USB Serial Download Protocol (SDP)
> state machine.
>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
>
>  cmd/Kconfig          |  7 +++++++
>  cmd/Makefile         |  1 +
>  cmd/usb_gadget_sdp.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
>  create mode 100644 cmd/usb_gadget_sdp.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index f18efc1e88..87333b3a97 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -665,6 +665,13 @@ config CMD_DFU
>  	  Enables the command "dfu" which is used to have U-Boot create a DFU
>  	  class device via USB.
>
> +config CMD_USB_SDP
> +	bool "sdp"
> +	select USB_FUNCTION_SDP
> +	help
> +	  Enables the command "sdp" which is used to have U-Boot emulating the
> +	  Serial Download Protocol (SDP) via USB.
> +
>  config CMD_USB_MASS_STORAGE
>  	bool "UMS usb mass storage"
>  	help
> diff --git a/cmd/Makefile b/cmd/Makefile
> index bd231f24d8..e0b5940ba6 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -131,6 +131,7 @@ obj-$(CONFIG_CMD_FASTBOOT) += fastboot.o
>  obj-$(CONFIG_CMD_FS_UUID) += fs_uuid.o
>
>  obj-$(CONFIG_CMD_USB_MASS_STORAGE) += usb_mass_storage.o
> +obj-$(CONFIG_CMD_USB_SDP) += usb_gadget_sdp.o
>  obj-$(CONFIG_CMD_THOR_DOWNLOAD) += thordown.o
>  obj-$(CONFIG_CMD_XIMG) += ximg.o
>  obj-$(CONFIG_YAFFS2) += yaffs2.o
> diff --git a/cmd/usb_gadget_sdp.c b/cmd/usb_gadget_sdp.c
> new file mode 100644
> index 0000000000..09ddb4f3aa
> --- /dev/null
> +++ b/cmd/usb_gadget_sdp.c
> @@ -0,0 +1,53 @@
> +/*
> + * cmd_sdp.c -- sdp command
> + *
> + * Copyright (C) 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +#include <usb.h>
> +
> +static int do_sdp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	int ret = CMD_RET_SUCCESS;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	char *usb_controller = argv[1];
> +	int controller_index = simple_strtoul(usb_controller, NULL, 0);
> +	board_usb_init(controller_index, USB_INIT_DEVICE);
> +
> +	g_dnl_clear_detach();
> +	g_dnl_register("usb_dnl_sdp");
> +
> +	ret = sdp_init();
> +	if (ret) {
> +		error("SDP init failed: %d", ret);
> +		ret = CMD_RET_FAILURE;
> +		goto exit;
> +	}
> +
> +	ret = sdp_handle();
> +	if (ret) {
> +		error("SDP failed: %d", ret);
> +		ret = CMD_RET_FAILURE;
> +		goto exit;
> +	}
> +
> +exit:
> +	g_dnl_unregister();
> +
> +	return ret;
> +}
> +
> +U_BOOT_CMD(sdp, 2, 1, do_sdp,
> +	"Serial Downloader Protocol",
> +	"<USB_controller>\n"
> +	"  - serial downloader protocol via <USB_controller>\n"
> +);
>

Reviewed-by: Łukasz Majewski <lukma@denx.de>

-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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

[U-Boot] [PATCH v1 0/7] imx: add USB Serial Download Protocol (SDP) support

[Eric Nelson]

Hi Stefan,

On 08/07/2017 11:06 AM, Stefan Agner wrote:
> Hi Eric,
> 
> On 2017-08-06 08:19, Eric Nelson wrote:
>> Hi Stefan,
>>
>> On 08/04/2017 04:38 PM, Stefan Agner wrote:
>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> This series adds NXP's Serial Download Protocol (SDP) support via
>>> USB for SPL/U-Boot. It allows to download U-Boot via USB from a
>>> (recovered) SPL using the same tools used to download SPL itself
>>> (specifically imx_usb, but also sb_loader seems to work).
>>>
>>
>> Nice!
>>
>>> The idea has been brought up when the first targets started to make
>>> use of SPL for DDR initialization, see:
>>> https://lists.denx.de/pipermail/u-boot/2015-July/220330.html
>>>
>>
>> There have been lots of discussions surrounding the use of SDP,
>> and this seems to be a nice alternative to using the i.MX "plugin"
>> mode that I explored a while back:
>>
>> https://lists.denx.de/pipermail/u-boot/2017-July/thread.html#298266
>>
> 
> Hm, wasn't aware of that particular effort, thanks for pointing to it.
>>From a quick glance, it did not work out so far, is this correct?

That's right. I believe that the trouble is in the return-to-ROM
process. I hacked together a form of "setjmp/longjmp" to try and
get it to work, but wasn't successful and the documentation for
the entry points had me confused.

> 
> I looked into plugin mode also a little bit, but I did not continue to
> look into it after reading this sentence in the i.MX 6 RM:
> 
> 8.7 Plugin Image
> The boot ROM detects the image type using the plugin flag of the boot
> data structure (see
> Boot Data Structure). If the plugin flag is 1, then the ROM uses the
> image as a plugin
> function. The function must initialize the boot device and copy the
> program image to the
> final location. At the end the plugin function must return with the
> program image
> parameters. (See High level boot sequence for details about boot flow).
> 

Hmm. That doesn't seem to match the existing code in the NXP U-Boot.

> 
> So if SPL should work as a plugin as NXP defines it, SPL is supposed to
> load the image from somewhere. The boot ROM then only takes care about
> image verification and further boot steps, but it's the plugins job to
> get the image from somewhere and store it in RAM.
> 

I think the documentation is just misleading, as NXP is using SPL
to load the payload in stages. The ROM is loading the portion
that goes into RAM after executing the plugin to initialize the
DDR controller (and PMIC if needed).

> Afact this is not really helpful in our case. We would want the boot ROM
> to go through the boot sequence (again).
> 

Not the full boot sequence. We'd just want it to load the rest of the
image into external RAM.

> Unless returning an error/invalid image causes the boot ROM to go
> through boot sequence again?
> 
I'm not clear on how errors are handled.

> The nice thing with our own SDP implementation is we can reuse it even
> from within U-Boot again, e.g. to download a kernel/initramfs....
> 

There are lots of nice things about having SDP implemented in
open-source "C" code and I'm inclined to give up on worrying about
"plugin" mode, at least for now.

My primary rationale for trying to get plugins to work was to prevent
the need to have a "full" U-Boot for initial programming of eMMC.

There is one other use case for plugins though, and that's the
"resume from LPSR" on i.MX7. In this case, RAM already has a
running kernel loaded, but the LPDDR is in self-refresh and
something needs to detect that during boot and return after
restoring the DDR registers.

>>> The initial SDP implementation (patch 2) requires the payload to
>>> have the imx specific headers (hence the move of the imx header
>>> file in patch 1).
>>>
>>> Patch 3 extends image header support beyond the SDP specification,
>>> specifically implements also support for U-Boot headers. This
>>> allows to use the same SPL/U-Boot binaries for recovery as used on
>>> the regular boot device (SD/eMMC). For that to work also the host
>>> side imx_usb tools needed an extension, currently available here:
>>>
>>> https://github.com/toradex/imx_loader/tree/imx_usb_batch_mode_refactored
>>>
>>> The full patchset allows to download SPL and U-Boot over USB to a
>>> target in recovery mode using the same usb_imx utility:
>>
>> imx_usb?
>>
> 
> Yeah right, sorry.
> 
>>> The usb_imx utility also has a batch mode which allows to download
>>> multiple artifacts with a single invocation. The details are
>>> outlined in the imx_usb commit message:
>>> https://github.com/toradex/imx_loader/commit/5434415d921f1cc4d22332d9558bed6d42db9f60
>>>
>>> In case this patchset gets accepted in U-Boot, I plan to push the
>>> imx_usb changes upstream as well.
>>>
>>
>> Do you have numbers for how much code/data size this adds to SPL?
>>
> 
> Besides the protocol implementation also general USB (gadget) support is
> required, hence it adds quite a bit.
> 
> Without USB Gadget/SDP support (Apalis iMX6 SPL):
> 
> $ arm-linux-gnueabihf-size spl/u-boot-spl
>     text    data     bss     dec     hex filename
>    24552    3808      92   28452    6f24 spl/u-boot-spl
> 
> With USB Gadget/SDP support:
> 
> $ arm-linux-gnueabihf-size spl/u-boot-spl
>     text    data     bss     dec     hex filename
>    42549    4679    1984   49212    c03c spl/u-boot-spl
> 
>> I believe some i.MX users have struggled to stay within the
>> size of internal RAM.
> 
> Hm, I think the limit is somewhere around 64kiB? In our case we are
> still well below...
> 

I think these numbers will be a problem for some use cases.

Someone on the list (Tim Harvey perhaps?) has been struggling to keep a
unified U-Boot under the IRAM limit and would need to choose between
supporting lots of different hardware and boot options and using SPL
with SDP.

Regards,


Eric

[U-Boot] [PATCH v1 5/7] spl: add serial download protocol (SDP) support

[Stefan Agner]

Stefano,

One question below:

On 2017-08-04 16:38, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add USB serial download protocol support to SPL. If the SoC started
> in recovery mode the SPL will immediately switch to SDP and wait for
> further downloads/commands from the host side.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  common/spl/Kconfig          |  6 ++++++
>  common/spl/Makefile         |  1 +
>  common/spl/spl_sdp.c        | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/Makefile |  1 +
>  4 files changed, 46 insertions(+)
>  create mode 100644 common/spl/spl_sdp.c
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 4de81392b0..95378b98a0 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -646,6 +646,12 @@ config SPL_DFU_RAM
>  
>  endchoice
>  
> +config SPL_USB_SDP_SUPPORT
> +	bool "Support SDP (Serial Download Protocol)"
> +	help
> +	  Enable Serial Download Protocol (SDP) device support in SPL. This
> +	  allows to download images into memory and execute (jump to) them
> +	  using the same protocol as implemented by the i.MX family's boot ROM.
>  endif
>  
>  config SPL_WATCHDOG_SUPPORT
> diff --git a/common/spl/Makefile b/common/spl/Makefile
> index 47a64dd7d0..a979560acf 100644
> --- a/common/spl/Makefile
> +++ b/common/spl/Makefile
> @@ -29,4 +29,5 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
>  obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
>  obj-$(CONFIG_SPL_SPI_LOAD) += spl_spi.o
>  obj-$(CONFIG_SPL_RAM_SUPPORT) += spl_ram.o
> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += spl_sdp.o
>  endif
> diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
> new file mode 100644
> index 0000000000..faa74b8bec
> --- /dev/null
> +++ b/common/spl/spl_sdp.c
> @@ -0,0 +1,38 @@
> +/*
> + * (C) Copyright 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <spl.h>
> +#include <usb.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static int spl_sdp_load_image(struct spl_image_info *spl_image,
> +			      struct spl_boot_device *bootdev)
> +{
> +	int ret;
> +
> +	g_dnl_clear_detach();
> +	g_dnl_register("usb_dnl_sdp");
> +
> +	ret = sdp_init();
> +	if (ret) {
> +		error("SDP init failed: %d", ret);
> +		return -ENODEV;
> +	}
> +
> +	ret = sdp_handle();
> +	if (ret) {
> +		error("SDP failed: %d", ret);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +SPL_LOAD_IMAGE_METHOD("USB SDP", 0, BOOT_DEVICE_UART, spl_sdp_load_image);


We currently use BOOT_DEVICE_UART when serial downloader boot mode is
detected. This can be either USB or UART...

In fact, USB is probably much more often used since only 6UL/ULL have
UART serial downloader support afact...

There is BOOT_DEVICE_BOARD which is used by e.g. Sunxi in case the boot
ROM mechanism is used, what do you think, should be change that?

Ideally we should be able to discriminate between USB and UART. But I
don't think its possible... (the USBPH0_PWD method likely does not work
since even in the UART case the boot ROM will initialize the USB PHY
first, at least according to the flow diagram in ULL's Chapter 9...)

The i.MX 7 which has the new Boot Information block actually only
support USB serial downloader...

Thoughts?

--
Stefan


> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 6a007d1bcb..7258099c1c 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_USB_ETHER) += epautoconf.o config.o usbstring.o
>  ifdef CONFIG_SPL_BUILD
>  obj-$(CONFIG_SPL_USB_GADGET_SUPPORT) += g_dnl.o
>  obj-$(CONFIG_SPL_DFU_SUPPORT) += f_dfu.o
> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += f_sdp.o
>  endif
>  
>  # new USB gadget layer dependencies

[U-Boot] [PATCH v1 5/7] spl: add serial download protocol (SDP) support

[Stefano Babic]

Hi Stefan,

On 09/08/2017 02:59, Stefan Agner wrote:
> Stefano,
> 
> One question below:
> 
> On 2017-08-04 16:38, Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Add USB serial download protocol support to SPL. If the SoC started
>> in recovery mode the SPL will immediately switch to SDP and wait for
>> further downloads/commands from the host side.
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> ---
>>
>>  common/spl/Kconfig          |  6 ++++++
>>  common/spl/Makefile         |  1 +
>>  common/spl/spl_sdp.c        | 38 ++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/gadget/Makefile |  1 +
>>  4 files changed, 46 insertions(+)
>>  create mode 100644 common/spl/spl_sdp.c
>>
>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>> index 4de81392b0..95378b98a0 100644
>> --- a/common/spl/Kconfig
>> +++ b/common/spl/Kconfig
>> @@ -646,6 +646,12 @@ config SPL_DFU_RAM
>>  
>>  endchoice
>>  
>> +config SPL_USB_SDP_SUPPORT
>> +	bool "Support SDP (Serial Download Protocol)"
>> +	help
>> +	  Enable Serial Download Protocol (SDP) device support in SPL. This
>> +	  allows to download images into memory and execute (jump to) them
>> +	  using the same protocol as implemented by the i.MX family's boot ROM.
>>  endif
>>  
>>  config SPL_WATCHDOG_SUPPORT
>> diff --git a/common/spl/Makefile b/common/spl/Makefile
>> index 47a64dd7d0..a979560acf 100644
>> --- a/common/spl/Makefile
>> +++ b/common/spl/Makefile
>> @@ -29,4 +29,5 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
>>  obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
>>  obj-$(CONFIG_SPL_SPI_LOAD) += spl_spi.o
>>  obj-$(CONFIG_SPL_RAM_SUPPORT) += spl_ram.o
>> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += spl_sdp.o
>>  endif
>> diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
>> new file mode 100644
>> index 0000000000..faa74b8bec
>> --- /dev/null
>> +++ b/common/spl/spl_sdp.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * (C) Copyright 2016 Toradex
>> + * Author: Stefan Agner <stefan.agner@toradex.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <spl.h>
>> +#include <usb.h>
>> +#include <g_dnl.h>
>> +#include <sdp.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +static int spl_sdp_load_image(struct spl_image_info *spl_image,
>> +			      struct spl_boot_device *bootdev)
>> +{
>> +	int ret;
>> +
>> +	g_dnl_clear_detach();
>> +	g_dnl_register("usb_dnl_sdp");
>> +
>> +	ret = sdp_init();
>> +	if (ret) {
>> +		error("SDP init failed: %d", ret);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = sdp_handle();
>> +	if (ret) {
>> +		error("SDP failed: %d", ret);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +SPL_LOAD_IMAGE_METHOD("USB SDP", 0, BOOT_DEVICE_UART, spl_sdp_load_image);
> 
> 
> We currently use BOOT_DEVICE_UART when serial downloader boot mode is
> detected. This can be either USB or UART...
> 

Right

> In fact, USB is probably much more often used since only 6UL/ULL have
> UART serial downloader support afact...

The misunderstanding is originated from NXP's documentation. The
official name is "Serial Downloader" and this can be easy confused as
"UART" downloader. Even if documentation is correct, it can be bad
interpreted.

To be correct, we have a "serial protocol" over a device that can be
either USB or UART.

> 
> There is BOOT_DEVICE_BOARD which is used by e.g. Sunxi in case the boot
> ROM mechanism is used, what do you think, should be change that?

Good point. To be consistent (thanks for hint, I was not aware of this
in sunxi) we should change it, yes.

> 
> Ideally we should be able to discriminate between USB and UART. But I
> don't think its possible... (the USBPH0_PWD method likely does not work
> since even in the UART case the boot ROM will initialize the USB PHY
> first, at least according to the flow diagram in ULL's Chapter 9...)

I agree with you - we should get rid of undocumented feature inside ROM,
whose behaviour could be changed with SOC revision number.

The use case is fixed - if a board loads from USB, it will always load
from USB. A runtime detection is difficult and even overkilling for this
application.

> 
> The i.MX 7 which has the new Boot Information block actually only
> support USB serial downloader...
> 
> Thoughts?

As far as I can see and apart of names, we should reach to have this
mainlined as USB downloader. It covers quite all cases.

Best regards,
Stefano


> 
> --
> Stefan
> 
> 
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 6a007d1bcb..7258099c1c 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_USB_ETHER) += epautoconf.o config.o usbstring.o
>>  ifdef CONFIG_SPL_BUILD
>>  obj-$(CONFIG_SPL_USB_GADGET_SUPPORT) += g_dnl.o
>>  obj-$(CONFIG_SPL_DFU_SUPPORT) += f_dfu.o
>> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += f_sdp.o
>>  endif
>>  
>>  # new USB gadget layer dependencies


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v1 2/7] usb: gadget: add SDP driver

[Stefano Babic]

Hi Stefan,

On 05/08/2017 01:38, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add SDP (Serial Downloader Protocol) implementation for U-Boot. The
> protocol is used in NXP SoC's boot ROM and allows to download program
> images. Beside that, it can also be used to read/write registers and
> download complete Device Configuration Data (DCD) sets. This basic
> implementation supports downloading images with the imx header format
> and reading registers.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  drivers/usb/gadget/Kconfig  |   7 +
>  drivers/usb/gadget/Makefile |   1 +
>  drivers/usb/gadget/f_sdp.c  | 723 ++++++++++++++++++++++++++++++++++++++++++++
>  include/sdp.h               |  16 +
>  4 files changed, 747 insertions(+)
>  create mode 100644 drivers/usb/gadget/f_sdp.c
>  create mode 100644 include/sdp.h
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 261ed128ac..225b66bc95 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -103,6 +103,13 @@ config USB_GADGET_DOWNLOAD
>  
>  if USB_GADGET_DOWNLOAD
>  
> +config USB_FUNCTION_SDP
> +	bool "Enable USB SDP (Serial Download Protocol)"
> +	help
> +	  Enable Serial Download Protocol (SDP) device support in U-Boot. This
> +	  allows to download images into memory and execute (jump to) them
> +	  using the same protocol as implemented by the i.MX family's boot ROM.
> +
>  config G_DNL_MANUFACTURER
>  	string "Vendor name of USB device"
>  
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 5e316a7cff..6a007d1bcb 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_USB_FUNCTION_THOR) += f_thor.o
>  obj-$(CONFIG_USB_FUNCTION_DFU) += f_dfu.o
>  obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
>  obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
> +obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
>  endif
>  endif
>  ifdef CONFIG_USB_ETHER
> diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> new file mode 100644
> index 0000000000..eb89695aaf
> --- /dev/null
> +++ b/drivers/usb/gadget/f_sdp.c
> @@ -0,0 +1,723 @@
> +/*
> + * f_sdp.c -- USB HID Serial Download Protocol
> + *
> + * Copyright (C) 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * This file implements the Serial Download Protocol (SDP) as specified in
> + * the i.MX 6 Reference Manual. The SDP is a USB HID based protocol and
> + * allows to download images directly to memory. The implementation
> + * works with the imx_loader (imx_usb) USB client software on host side.
> + *
> + * Not all commands are implemented, e.g. WRITE_REGISTER, DCD_WRITE and
> + * SKIP_DCD_HEADER are only stubs.
> + *
> + * Parts of the implementation are based on f_dfu and f_thor.
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <errno.h>
> +#include <common.h>
> +#include <console.h>
> +#include <malloc.h>
> +
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/composite.h>
> +
> +#include <asm/io.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +#include <imximage.h>
> +
> +#define HID_REPORT_ID_MASK	0x000000ff
> +
> +/*
> + * HID class requests
> + */
> +#define HID_REQ_GET_REPORT		0x01
> +#define HID_REQ_GET_IDLE		0x02
> +#define HID_REQ_GET_PROTOCOL		0x03
> +#define HID_REQ_SET_REPORT		0x09
> +#define HID_REQ_SET_IDLE		0x0A
> +#define HID_REQ_SET_PROTOCOL		0x0B
> +
> +#define HID_USAGE_PAGE_LEN		76
> +
> +struct hid_report {
> +	u8 usage_page[HID_USAGE_PAGE_LEN];
> +} __packed;
> +
> +#define SDP_READ_REGISTER	0x0101
> +#define SDP_WRITE_REGISTER	0x0202
> +#define SDP_WRITE_FILE		0x0404
> +#define SDP_ERROR_STATUS	0x0505
> +#define SDP_DCD_WRITE		0x0a0a
> +#define SDP_JUMP_ADDRESS	0x0b0b
> +#define SDP_SKIP_DCD_HEADER	0x0c0c

It looks like that I am again out of sync with documentation. Where is
defined SDP_SKIP_DCD_HEADER ? It is undefined for MX6Q/D, Solo and DL.

> +
> +#define SDP_WRITE_FILE_COMPLETE		0x88888888
> +#define SDP_WRITE_REGISTER_COMPLETE	0x128A8A12
> +#define SDP_SKIP_DCD_HEADER_COMPLETE	0x900DD009
> +#define SDP_ERROR_IMXHEADER		0x000a0533
> +
> +#define SDP_COMMAND_LEN		16
> +
> +struct sdp_command {
> +	u16 cmd;
> +	u32 addr;
> +	u8 format;
> +	u32 cnt;
> +	u32 data;
> +	u8 rsvd;
> +} __packed;
> +
> +enum sdp_state {
> +	SDP_STATE_IDLE,
> +	SDP_STATE_RX_DCD_DATA,
> +	SDP_STATE_RX_FILE_DATA,
> +	SDP_STATE_TX_SEC_CONF,
> +	SDP_STATE_TX_SEC_CONF_BUSY,
> +	SDP_STATE_TX_REGISTER,
> +	SDP_STATE_TX_REGISTER_BUSY,
> +	SDP_STATE_TX_STATUS,
> +	SDP_STATE_TX_STATUS_BUSY,
> +	SDP_STATE_JUMP,
> +};
> +
> +struct f_sdp {
> +	struct usb_function		usb_function;
> +
> +	struct usb_descriptor_header	**function;
> +
> +	u8				altsetting;
> +	enum sdp_state			state;
> +	enum sdp_state			next_state;
> +	u32				dnl_address;
> +	u32				dnl_bytes_remaining;
> +	u32				jmp_address;
> +	bool				always_send_status;
> +	u32				error_status;
> +
> +	/* EP0 request */
> +	struct usb_request		*req;
> +
> +	/* EP1 IN */
> +	struct usb_ep			*in_ep;
> +	struct usb_request		*in_req;
> +
> +	bool				configuration_done;
> +};
> +
> +static struct f_sdp *sdp_func;
> +
> +static inline struct f_sdp *func_to_sdp(struct usb_function *f)
> +{
> +	return container_of(f, struct f_sdp, usb_function);
> +}
> +
> +static struct usb_interface_descriptor sdp_intf_runtime = {
> +	.bLength =		sizeof(sdp_intf_runtime),
> +	.bDescriptorType =	USB_DT_INTERFACE,
> +	.bAlternateSetting =	0,
> +	.bNumEndpoints =	1,
> +	.bInterfaceClass =	USB_CLASS_HID,
> +	.bInterfaceSubClass =	0,
> +	.bInterfaceProtocol =	0,
> +	/* .iInterface = DYNAMIC */
> +};
> +
> +/* HID configuration */
> +static struct usb_class_hid_descriptor sdp_hid_desc = {
> +	.bLength =		sizeof(sdp_hid_desc),
> +	.bDescriptorType =	USB_DT_CS_DEVICE,
> +
> +	.bcdCDC =		__constant_cpu_to_le16(0x0110),
> +	.bCountryCode =		0,
> +	.bNumDescriptors =	1,
> +
> +	.bDescriptorType0	= USB_DT_HID_REPORT,
> +	.wDescriptorLength0	= HID_USAGE_PAGE_LEN,
> +};
> +
> +static struct usb_endpoint_descriptor in_desc = {
> +	.bLength =		USB_DT_ENDPOINT_SIZE,
> +	.bDescriptorType =	USB_DT_ENDPOINT, /*USB_DT_CS_ENDPOINT*/
> +
> +	.bEndpointAddress =	1 | USB_DIR_IN,
> +	.bmAttributes =	USB_ENDPOINT_XFER_INT,
> +	.wMaxPacketSize =	64,
> +	.bInterval =		1,
> +};
> +
> +static struct usb_descriptor_header *sdp_runtime_descs[] = {
> +	(struct usb_descriptor_header *)&sdp_intf_runtime,
> +	(struct usb_descriptor_header *)&sdp_hid_desc,
> +	(struct usb_descriptor_header *)&in_desc,
> +	NULL,
> +};
> +
> +/* This is synchronized with what the SoC implementation reports */
> +static struct hid_report sdp_hid_report = {
> +	.usage_page = {
> +		0x06, 0x00, 0xff, /* Usage Page */
> +		0x09, 0x01, /* Usage (Poiter?) */
> +		0xa1, 0x01, /* Collection */
> +
> +		0x85, 0x01, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size */
> +		0x95, 0x10, /* Report Count */
> +		0x91, 0x02, /* Output Data */
> +
> +		0x85, 0x02, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x80, /* Report Size 128 */
> +		0x95, 0x40, /* Report Count */
> +		0x91, 0x02, /* Output Data */
> +
> +		0x85, 0x03, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size 8 */
> +		0x95, 0x04, /* Report Count */
> +		0x81, 0x02, /* Input Data */
> +
> +		0x85, 0x04, /* Report ID */
> +		0x19, 0x01, /* Usage Minimum */
> +		0x29, 0x01, /* Usage Maximum */
> +		0x15, 0x00, /* Local Minimum */
> +		0x26, 0xFF, 0x00, /* Local Maximum? */
> +		0x75, 0x08, /* Report Size 8 */
> +		0x95, 0x40, /* Report Count */
> +		0x81, 0x02, /* Input Data */
> +		0xc0
> +	},
> +};
> +
> +static const char sdp_name[] = "Serial Downloader Protocol";
> +
> +/*
> + * static strings, in UTF-8
> + */
> +static struct usb_string strings_sdp_generic[] = {
> +	[0].s = sdp_name,
> +	{  }			/* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_sdp_generic = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_sdp_generic,
> +};
> +
> +static struct usb_gadget_strings *sdp_generic_strings[] = {
> +	&stringtab_sdp_generic,
> +	NULL,
> +};
> +
> +static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +	u8 *data = req->buf;
> +	u8 report = data[0];
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	if (report != 1) {
> +		error("Unexpected report %d", report);
> +		return;
> +	}
> +
> +	struct sdp_command *cmd = req->buf + 1;
> +
> +	debug("%s: command: %04x, addr: %08x, cnt: %u\n",
> +	      __func__, be16_to_cpu(cmd->cmd),
> +	      be32_to_cpu(cmd->addr), be32_to_cpu(cmd->cnt));
> +
> +	switch (be16_to_cpu(cmd->cmd)) {
> +	case SDP_READ_REGISTER:
> +		sdp->always_send_status = false;
> +		sdp->error_status = 0x0;
> +
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_TX_REGISTER;
> +		printf("Reading %d registers at 0x%08x... ",
> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
> +		break;
> +	case SDP_WRITE_FILE:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_WRITE_FILE_COMPLETE;
> +
> +		sdp->state = SDP_STATE_RX_FILE_DATA;
> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_IDLE;
> +
> +		printf("Downloading file of size %d to 0x%08x... ",
> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
> +
> +		break;
> +	case SDP_ERROR_STATUS:
> +		sdp->always_send_status = true;
> +		sdp->error_status = 0;
> +
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	case SDP_DCD_WRITE:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_WRITE_REGISTER_COMPLETE;
> +
> +		sdp->state = SDP_STATE_RX_DCD_DATA;
> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;

It is fine, but I am just missing if this is a use case. DCD is
interpreted by boot ROM, and we are here already over in SPL.

> +	case SDP_JUMP_ADDRESS:
> +		sdp->always_send_status = false;
> +		sdp->error_status = 0;
> +
> +		sdp->jmp_address = be32_to_cpu(cmd->addr);
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_JUMP;
> +		break;
> +	case SDP_SKIP_DCD_HEADER:
> +		sdp->always_send_status = true;
> +		sdp->error_status = SDP_SKIP_DCD_HEADER_COMPLETE;
> +
> +		/* Ignore command, DCD not supported anyway */

Right - we load a file, we do not need a DCD.

> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		sdp->next_state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		error("Unknown command: %08x\n", be16_to_cpu(cmd->cmd));
> +	}
> +}
> +
> +static void sdp_rx_data_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +	u8 *data = req->buf;
> +	u8 report = data[0];
> +	int datalen = req->length - 1;
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	if (report != 2) {
> +		error("Unexpected report %d", report);
> +		return;
> +	}
> +
> +	if (sdp->dnl_bytes_remaining < datalen) {
> +		/*
> +		 * Some USB stacks require to send a complete buffer as
> +		 * specified in the HID descriptor. This leads to longer
> +		 * transfers than the file length, no problem for us.
> +		 */
> +		sdp->dnl_bytes_remaining = 0;
> +	} else {
> +		sdp->dnl_bytes_remaining -= datalen;
> +	}
> +
> +	if (sdp->state == SDP_STATE_RX_FILE_DATA) {
> +		memcpy((void *)sdp->dnl_address, req->buf + 1, datalen);
> +		sdp->dnl_address += datalen;
> +	}
> +
> +	if (sdp->dnl_bytes_remaining)
> +		return;
> +
> +	printf("done\n");
> +
> +	switch (sdp->state) {
> +	case SDP_STATE_RX_FILE_DATA:
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		break;
> +	case SDP_STATE_RX_DCD_DATA:
> +		sdp->state = SDP_STATE_TX_SEC_CONF;
> +		break;
> +	default:
> +		error("Invalid state: %d", sdp->state);
> +	}
> +}
> +
> +
> +
> +static void sdp_tx_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct f_sdp *sdp = req->context;
> +	int status = req->status;
> +
> +	if (status != 0) {
> +		error("Status: %d", status);
> +		return;
> +	}
> +
> +	switch (sdp->state) {
> +	case SDP_STATE_TX_SEC_CONF_BUSY:
> +		/* Not all commands require status report */
> +		if (sdp->always_send_status || sdp->error_status)
> +			sdp->state = SDP_STATE_TX_STATUS;
> +		else
> +			sdp->state = sdp->next_state;
> +
> +		break;
> +	case SDP_STATE_TX_STATUS_BUSY:
> +		sdp->state = sdp->next_state;
> +		break;
> +	case SDP_STATE_TX_REGISTER_BUSY:
> +		if (sdp->dnl_bytes_remaining)
> +			sdp->state = SDP_STATE_TX_REGISTER;
> +		else
> +			sdp->state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		error("Wrong State: %d", sdp->state);
> +		sdp->state = SDP_STATE_IDLE;
> +		break;
> +	}
> +	debug("%s complete --> %d, %d/%d\n", ep->name,
> +	      status, req->actual, req->length);
> +}
> +
> +static int sdp_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
> +{
> +	struct usb_gadget *gadget = f->config->cdev->gadget;
> +	struct usb_request *req = f->config->cdev->req;
> +	struct f_sdp *sdp = f->config->cdev->req->context;
> +	u16 len = le16_to_cpu(ctrl->wLength);
> +	u16 w_value = le16_to_cpu(ctrl->wValue);
> +	int value = 0;
> +	u8 req_type = ctrl->bRequestType & USB_TYPE_MASK;
> +
> +	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
> +	debug("req_type: 0x%x ctrl->bRequest: 0x%x sdp->state: %d\n",
> +	      req_type, ctrl->bRequest, sdp->state);
> +
> +	if (req_type == USB_TYPE_STANDARD) {
> +		if (ctrl->bRequest == USB_REQ_GET_DESCRIPTOR) {
> +			/* Send HID report descriptor */
> +			value = min(len, (u16) sizeof(sdp_hid_report));
> +			memcpy(req->buf, &sdp_hid_report, value);
> +			sdp->configuration_done = true;
> +		}
> +	}
> +
> +	if (req_type == USB_TYPE_CLASS) {
> +		int report = w_value & HID_REPORT_ID_MASK;
> +
> +		/* HID (SDP) request */
> +		switch (ctrl->bRequest) {
> +		case HID_REQ_SET_REPORT:
> +			switch (report) {
> +			case 1:
> +				value = SDP_COMMAND_LEN + 1;
> +				req->complete = sdp_rx_command_complete;
> +				break;
> +			case 2:
> +				value = len;
> +				req->complete = sdp_rx_data_complete;
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (value >= 0) {
> +		req->length = value;
> +		req->zero = value < len;
> +		value = usb_ep_queue(gadget->ep0, req, 0);
> +		if (value < 0) {
> +			debug("ep_queue --> %d\n", value);
> +			req->status = 0;
> +		}
> +	}
> +
> +	return value;
> +}
> +
> +static int sdp_bind(struct usb_configuration *c, struct usb_function *f)
> +{
> +	struct usb_gadget *gadget = c->cdev->gadget;
> +	struct usb_composite_dev *cdev = c->cdev;
> +	struct f_sdp *sdp = func_to_sdp(f);
> +	int rv = 0, id;
> +
> +	id = usb_interface_id(c, f);
> +	if (id < 0)
> +		return id;
> +	sdp_intf_runtime.bInterfaceNumber = id;
> +
> +	struct usb_ep *ep;
> +
> +	/* allocate instance-specific endpoints */
> +	ep = usb_ep_autoconfig(gadget, &in_desc);
> +	if (!ep) {
> +		rv = -ENODEV;
> +		goto error;
> +	}
> +
> +	sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
> +
> +	cdev->req->context = sdp;
> +
> +error:
> +	return rv;
> +}
> +
> +static void sdp_unbind(struct usb_configuration *c, struct usb_function *f)
> +{
> +	free(sdp_func);
> +	sdp_func = NULL;
> +}
> +
> +static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length)
> +{
> +	struct usb_request *req;
> +
> +	req = usb_ep_alloc_request(ep, 0);
> +	if (!req)
> +		return req;
> +
> +	req->length = length;
> +	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, length);
> +	if (!req->buf) {
> +		usb_ep_free_request(ep, req);
> +		req = NULL;
> +	}
> +
> +	return req;
> +}
> +
> +
> +static struct usb_request *sdp_start_ep(struct usb_ep *ep)
> +{
> +	struct usb_request *req;
> +
> +	req = alloc_ep_req(ep, 64);
> +	debug("%s: ep:%p req:%p\n", __func__, ep, req);
> +
> +	if (!req)
> +		return NULL;
> +
> +	memset(req->buf, 0, req->length);
> +	req->complete = sdp_tx_complete;
> +
> +	return req;
> +}
> +static int sdp_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
> +{
> +	struct f_sdp *sdp = func_to_sdp(f);
> +	struct usb_composite_dev *cdev = f->config->cdev;
> +	int result;
> +
> +	debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
> +
> +	result = usb_ep_enable(sdp->in_ep, &in_desc);
> +	if (result)
> +		return result;
> +	sdp->in_req = sdp_start_ep(sdp->in_ep);
> +	sdp->in_req->context = sdp;
> +
> +	sdp->in_ep->driver_data = cdev; /* claim */
> +
> +	sdp->altsetting = alt;
> +	sdp->state = SDP_STATE_IDLE;
> +
> +	return 0;
> +}
> +
> +static int sdp_get_alt(struct usb_function *f, unsigned intf)
> +{
> +	struct f_sdp *sdp = func_to_sdp(f);
> +
> +	return sdp->altsetting;
> +}
> +
> +static void sdp_disable(struct usb_function *f)
> +{
> +	struct f_sdp *sdp = func_to_sdp(f);
> +
> +	usb_ep_disable(sdp->in_ep);
> +
> +	if (sdp->in_req) {
> +		free(sdp->in_req);
> +		sdp->in_req = NULL;
> +	}
> +}
> +
> +static int sdp_bind_config(struct usb_configuration *c)
> +{
> +	int status;
> +
> +	if (!sdp_func) {
> +		sdp_func = memalign(CONFIG_SYS_CACHELINE_SIZE, sizeof(*sdp_func));
> +		if (!sdp_func)
> +			return -ENOMEM;
> +	}
> +
> +	memset(sdp_func, 0, sizeof(*sdp_func));
> +
> +	sdp_func->usb_function.name = "sdp";
> +	sdp_func->usb_function.hs_descriptors = sdp_runtime_descs;
> +	sdp_func->usb_function.descriptors = sdp_runtime_descs;
> +	sdp_func->usb_function.bind = sdp_bind;
> +	sdp_func->usb_function.unbind = sdp_unbind;
> +	sdp_func->usb_function.set_alt = sdp_set_alt;
> +	sdp_func->usb_function.get_alt = sdp_get_alt;
> +	sdp_func->usb_function.disable = sdp_disable;
> +	sdp_func->usb_function.strings = sdp_generic_strings;
> +	sdp_func->usb_function.setup = sdp_setup;
> +
> +	status = usb_add_function(c, &sdp_func->usb_function);
> +
> +	return status;
> +}
> +
> +int sdp_init(void)
> +{
> +	printf("SDP: initialize...\n");
> +	while (!sdp_func->configuration_done) {
> +		if (ctrlc()) {
> +			puts("\rCTRL+C - Operation aborted.\n");
> +			return 0;
> +		}
> +		usb_gadget_handle_interrupts(0);
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 sdp_jump_imxheader(void *address)
> +{
> +	flash_header_v2_t *headerv2 = address;
> +	ulong (*entry)(void);
> +
> +	if (headerv2->header.tag != IVT_HEADER_TAG) {
> +		printf("Header Tag is not a IMX image\n");
> +		return SDP_ERROR_IMXHEADER;
> +	}
> +
> +	printf("Jumping to 0x%08x\n", headerv2->entry);
> +	entry = (void *)headerv2->entry;
> +	entry();
> +
> +	/* The image probably never returns hence we wont reach that point */
> +	return 0;
> +}
> +
> +static void sdp_handle_in_ep(void)
> +{
> +	u8 *data = sdp_func->in_req->buf;
> +	u32 status;
> +	int datalen;
> +
> +	switch (sdp_func->state) {
> +	case SDP_STATE_TX_SEC_CONF:
> +		debug("Report 3: HAB security\n");
> +		data[0] = 3;
> +
> +		data[1] = 0x56;
> +		data[2] = 0x78;
> +		data[3] = 0x78;
> +		data[4] = 0x56;

I am quite lost here - can you explain what are these magic numbers, and
maybe add a comment for it (or self explaining defines) ?

> +
> +		sdp_func->in_req->length = 5;
> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
> +		sdp_func->state = SDP_STATE_TX_SEC_CONF_BUSY;
> +		break;
> +
> +	case SDP_STATE_TX_STATUS:
> +		debug("Report 4: Status\n");
> +		data[0] = 4;
> +
> +		memcpy(&data[1], &sdp_func->error_status, 4);
> +		sdp_func->in_req->length = 65;
> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
> +		sdp_func->state = SDP_STATE_TX_STATUS_BUSY;
> +		break;
> +	case SDP_STATE_TX_REGISTER:
> +		debug("Report 4: Register Values\n");
> +		data[0] = 4;
> +
> +		datalen = sdp_func->dnl_bytes_remaining;
> +
> +		if (datalen > 64)
> +			datalen = 64;
> +
> +		memcpy(&data[1], (void *)sdp_func->dnl_address, datalen);
> +		sdp_func->in_req->length = 65;
> +
> +		sdp_func->dnl_bytes_remaining -= datalen;
> +		sdp_func->dnl_address += datalen;
> +
> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
> +		sdp_func->state = SDP_STATE_TX_REGISTER_BUSY;
> +		break;
> +	case SDP_STATE_JUMP:
> +		printf("Checking imxheader at 0x%08x\n", f_sdp->jmp_address);
> +		status = sdp_jump_imxheader((void *)f_sdp->jmp_address);
> +
> +		sdp_func->next_state = SDP_STATE_IDLE;
> +		sdp_func->error_status = status;
> +
> +		/* Only send Report 4 if there was an error */
> +		if (status)
> +			sdp_func->state = SDP_STATE_TX_STATUS;
> +		else
> +			sdp_func->state = SDP_STATE_IDLE;
> +		break;
> +	default:
> +		break;
> +	};
> +}
> +
> +int sdp_handle(void)
> +{
> +	printf("SDP: handle requests...\n");
> +	while (1) {
> +		if (ctrlc()) {
> +			puts("\rCTRL+C - Operation aborted.\n");
> +			return 0;
> +		}
> +
> +		usb_gadget_handle_interrupts(0);
> +
> +		sdp_handle_in_ep();
> +	}
> +}
> +
> +int sdp_add(struct usb_configuration *c)
> +{
> +	int id;
> +
> +	id = usb_string_id(c->cdev);
> +	if (id < 0)
> +		return id;
> +	strings_sdp_generic[0].id = id;
> +	sdp_intf_runtime.iInterface = id;
> +
> +	debug("%s: cdev: 0x%p gadget:0x%p gadget->ep0: 0x%p\n", __func__,
> +	      c->cdev, c->cdev->gadget, c->cdev->gadget->ep0);
> +
> +	return sdp_bind_config(c);
> +}
> +
> +DECLARE_GADGET_BIND_CALLBACK(usb_dnl_sdp, sdp_add);
> diff --git a/include/sdp.h b/include/sdp.h
> new file mode 100644
> index 0000000000..03c4a23434
> --- /dev/null
> +++ b/include/sdp.h
> @@ -0,0 +1,16 @@
> +/*
> + * sdp.h - Serial Download Protocol
> + *
> + * Copyright (C) 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef __SDP_H_
> +#define __SDP_H_
> +
> +int sdp_init(void);
> +int sdp_handle(void);
> +
> +#endif /* __SDP_H_ */
> 

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v1 3/7] usb: gadget: sdp: extend images compatible for jumps

[Stefano Babic]

On 05/08/2017 01:38, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Support U-Boot images in SPL so that u-boot.img files can be
> directly downloaded and executed. Furthermore support U-Boot
> scripts download and execution in full U-Boot so that custom
> recovery actions can be downloaded from the host in a third
> step.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  drivers/usb/gadget/f_sdp.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> index eb89695aaf..9a752843f0 100644
> --- a/drivers/usb/gadget/f_sdp.c
> +++ b/drivers/usb/gadget/f_sdp.c
> @@ -29,6 +29,8 @@
>  #include <asm/io.h>
>  #include <g_dnl.h>
>  #include <sdp.h>
> +#include <spl.h>
> +#include <image.h>
>  #include <imximage.h>
>  
>  #define HID_REPORT_ID_MASK	0x000000ff
> @@ -672,8 +674,22 @@ static void sdp_handle_in_ep(void)
>  		sdp_func->state = SDP_STATE_TX_REGISTER_BUSY;
>  		break;
>  	case SDP_STATE_JUMP:
> -		printf("Checking imxheader at 0x%08x\n", f_sdp->jmp_address);
> -		status = sdp_jump_imxheader((void *)f_sdp->jmp_address);
> +		printf("Jumping to header at 0x%08x\n", sdp_func->jmp_address);
> +		status = sdp_jump_imxheader((void *)sdp_func->jmp_address);
> +
> +		/* If imx header fails, try some U-Boot specific headers */
> +		if (status) {
> +#ifdef CONFIG_SPL_BUILD
> +			/* In SPL, allow jumps to U-Boot images */
> +			struct spl_image_info spl_image = {};
> +			spl_parse_image_header(&spl_image,
> +				(struct image_header *)sdp_func->jmp_address);
> +			jump_to_image_no_args(&spl_image);
> +#else
> +			/* In U-Boot, allow jumps to scripts */
> +			source(sdp_func->jmp_address, "script at 1");
> +#endif
> +		}
>  
>  		sdp_func->next_state = SDP_STATE_IDLE;
>  		sdp_func->error_status = status;
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v1 4/7] cmd: add sdp command

[Stefano Babic]

Hi Stefan,

On 05/08/2017 01:38, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add a new command to start USB Serial Download Protocol (SDP)
> state machine.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  cmd/Kconfig          |  7 +++++++
>  cmd/Makefile         |  1 +
>  cmd/usb_gadget_sdp.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
>  create mode 100644 cmd/usb_gadget_sdp.c
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index f18efc1e88..87333b3a97 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -665,6 +665,13 @@ config CMD_DFU
>  	  Enables the command "dfu" which is used to have U-Boot create a DFU
>  	  class device via USB.
>  
> +config CMD_USB_SDP
> +	bool "sdp"
> +	select USB_FUNCTION_SDP
> +	help
> +	  Enables the command "sdp" which is used to have U-Boot emulating the
> +	  Serial Download Protocol (SDP) via USB.
> +
>  config CMD_USB_MASS_STORAGE
>  	bool "UMS usb mass storage"
>  	help
> diff --git a/cmd/Makefile b/cmd/Makefile
> index bd231f24d8..e0b5940ba6 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -131,6 +131,7 @@ obj-$(CONFIG_CMD_FASTBOOT) += fastboot.o
>  obj-$(CONFIG_CMD_FS_UUID) += fs_uuid.o
>  
>  obj-$(CONFIG_CMD_USB_MASS_STORAGE) += usb_mass_storage.o
> +obj-$(CONFIG_CMD_USB_SDP) += usb_gadget_sdp.o
>  obj-$(CONFIG_CMD_THOR_DOWNLOAD) += thordown.o
>  obj-$(CONFIG_CMD_XIMG) += ximg.o
>  obj-$(CONFIG_YAFFS2) += yaffs2.o
> diff --git a/cmd/usb_gadget_sdp.c b/cmd/usb_gadget_sdp.c
> new file mode 100644
> index 0000000000..09ddb4f3aa
> --- /dev/null
> +++ b/cmd/usb_gadget_sdp.c
> @@ -0,0 +1,53 @@
> +/*
> + * cmd_sdp.c -- sdp command
> + *
> + * Copyright (C) 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +#include <usb.h>
> +
> +static int do_sdp(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	int ret = CMD_RET_SUCCESS;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	char *usb_controller = argv[1];
> +	int controller_index = simple_strtoul(usb_controller, NULL, 0);
> +	board_usb_init(controller_index, USB_INIT_DEVICE);
> +
> +	g_dnl_clear_detach();
> +	g_dnl_register("usb_dnl_sdp");
> +
> +	ret = sdp_init();
> +	if (ret) {
> +		error("SDP init failed: %d", ret);
> +		ret = CMD_RET_FAILURE;
> +		goto exit;
> +	}
> +
> +	ret = sdp_handle();
> +	if (ret) {
> +		error("SDP failed: %d", ret);
> +		ret = CMD_RET_FAILURE;
> +		goto exit;
> +	}
> +
> +exit:
> +	g_dnl_unregister();
> +
> +	return ret;
> +}
> +
> +U_BOOT_CMD(sdp, 2, 1, do_sdp,
> +	"Serial Downloader Protocol",
> +	"<USB_controller>\n"
> +	"  - serial downloader protocol via <USB_controller>\n"
> +);
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v1 5/7] spl: add serial download protocol (SDP) support

[Stefano Babic]

Hi Stefan,

On 05/08/2017 01:38, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> Add USB serial download protocol support to SPL. If the SoC started
> in recovery mode the SPL will immediately switch to SDP and wait for
> further downloads/commands from the host side.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  common/spl/Kconfig          |  6 ++++++
>  common/spl/Makefile         |  1 +
>  common/spl/spl_sdp.c        | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/Makefile |  1 +
>  4 files changed, 46 insertions(+)
>  create mode 100644 common/spl/spl_sdp.c
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 4de81392b0..95378b98a0 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -646,6 +646,12 @@ config SPL_DFU_RAM
>  
>  endchoice
>  
> +config SPL_USB_SDP_SUPPORT
> +	bool "Support SDP (Serial Download Protocol)"
> +	help
> +	  Enable Serial Download Protocol (SDP) device support in SPL. This
> +	  allows to download images into memory and execute (jump to) them
> +	  using the same protocol as implemented by the i.MX family's boot ROM.
>  endif
>  
>  config SPL_WATCHDOG_SUPPORT
> diff --git a/common/spl/Makefile b/common/spl/Makefile
> index 47a64dd7d0..a979560acf 100644
> --- a/common/spl/Makefile
> +++ b/common/spl/Makefile
> @@ -29,4 +29,5 @@ obj-$(CONFIG_SPL_SATA_SUPPORT) += spl_sata.o
>  obj-$(CONFIG_SPL_DFU_SUPPORT) += spl_dfu.o
>  obj-$(CONFIG_SPL_SPI_LOAD) += spl_spi.o
>  obj-$(CONFIG_SPL_RAM_SUPPORT) += spl_ram.o
> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += spl_sdp.o
>  endif
> diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c
> new file mode 100644
> index 0000000000..faa74b8bec
> --- /dev/null
> +++ b/common/spl/spl_sdp.c
> @@ -0,0 +1,38 @@
> +/*
> + * (C) Copyright 2016 Toradex
> + * Author: Stefan Agner <stefan.agner@toradex.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <spl.h>
> +#include <usb.h>
> +#include <g_dnl.h>
> +#include <sdp.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static int spl_sdp_load_image(struct spl_image_info *spl_image,
> +			      struct spl_boot_device *bootdev)
> +{
> +	int ret;
> +
> +	g_dnl_clear_detach();
> +	g_dnl_register("usb_dnl_sdp");
> +
> +	ret = sdp_init();
> +	if (ret) {
> +		error("SDP init failed: %d", ret);
> +		return -ENODEV;
> +	}
> +
> +	ret = sdp_handle();
> +	if (ret) {
> +		error("SDP failed: %d", ret);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +SPL_LOAD_IMAGE_METHOD("USB SDP", 0, BOOT_DEVICE_UART, spl_sdp_load_image);
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 6a007d1bcb..7258099c1c 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_USB_ETHER) += epautoconf.o config.o usbstring.o
>  ifdef CONFIG_SPL_BUILD
>  obj-$(CONFIG_SPL_USB_GADGET_SUPPORT) += g_dnl.o
>  obj-$(CONFIG_SPL_DFU_SUPPORT) += f_dfu.o
> +obj-$(CONFIG_SPL_USB_SDP_SUPPORT) += f_sdp.o
>  endif
>  
>  # new USB gadget layer dependencies
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v1 2/7] usb: gadget: add SDP driver

[Stefan Agner]

On 2017-08-10 01:14, Stefano Babic wrote:
> Hi Stefan,
> 
> On 05/08/2017 01:38, Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> Add SDP (Serial Downloader Protocol) implementation for U-Boot. The
>> protocol is used in NXP SoC's boot ROM and allows to download program
>> images. Beside that, it can also be used to read/write registers and
>> download complete Device Configuration Data (DCD) sets. This basic
>> implementation supports downloading images with the imx header format
>> and reading registers.
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> ---
>>
>>  drivers/usb/gadget/Kconfig  |   7 +
>>  drivers/usb/gadget/Makefile |   1 +
>>  drivers/usb/gadget/f_sdp.c  | 723 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/sdp.h               |  16 +
>>  4 files changed, 747 insertions(+)
>>  create mode 100644 drivers/usb/gadget/f_sdp.c
>>  create mode 100644 include/sdp.h
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 261ed128ac..225b66bc95 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -103,6 +103,13 @@ config USB_GADGET_DOWNLOAD
>>
>>  if USB_GADGET_DOWNLOAD
>>
>> +config USB_FUNCTION_SDP
>> +	bool "Enable USB SDP (Serial Download Protocol)"
>> +	help
>> +	  Enable Serial Download Protocol (SDP) device support in U-Boot. This
>> +	  allows to download images into memory and execute (jump to) them
>> +	  using the same protocol as implemented by the i.MX family's boot ROM.
>> +
>>  config G_DNL_MANUFACTURER
>>  	string "Vendor name of USB device"
>>
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 5e316a7cff..6a007d1bcb 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_USB_FUNCTION_THOR) += f_thor.o
>>  obj-$(CONFIG_USB_FUNCTION_DFU) += f_dfu.o
>>  obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
>>  obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
>> +obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
>>  endif
>>  endif
>>  ifdef CONFIG_USB_ETHER
>> diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
>> new file mode 100644
>> index 0000000000..eb89695aaf
>> --- /dev/null
>> +++ b/drivers/usb/gadget/f_sdp.c
>> @@ -0,0 +1,723 @@
>> +/*
>> + * f_sdp.c -- USB HID Serial Download Protocol
>> + *
>> + * Copyright (C) 2016 Toradex
>> + * Author: Stefan Agner <stefan.agner@toradex.com>
>> + *
>> + * This file implements the Serial Download Protocol (SDP) as specified in
>> + * the i.MX 6 Reference Manual. The SDP is a USB HID based protocol and
>> + * allows to download images directly to memory. The implementation
>> + * works with the imx_loader (imx_usb) USB client software on host side.
>> + *
>> + * Not all commands are implemented, e.g. WRITE_REGISTER, DCD_WRITE and
>> + * SKIP_DCD_HEADER are only stubs.
>> + *
>> + * Parts of the implementation are based on f_dfu and f_thor.
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <errno.h>
>> +#include <common.h>
>> +#include <console.h>
>> +#include <malloc.h>
>> +
>> +#include <linux/usb/ch9.h>
>> +#include <linux/usb/gadget.h>
>> +#include <linux/usb/composite.h>
>> +
>> +#include <asm/io.h>
>> +#include <g_dnl.h>
>> +#include <sdp.h>
>> +#include <imximage.h>
>> +
>> +#define HID_REPORT_ID_MASK	0x000000ff
>> +
>> +/*
>> + * HID class requests
>> + */
>> +#define HID_REQ_GET_REPORT		0x01
>> +#define HID_REQ_GET_IDLE		0x02
>> +#define HID_REQ_GET_PROTOCOL		0x03
>> +#define HID_REQ_SET_REPORT		0x09
>> +#define HID_REQ_SET_IDLE		0x0A
>> +#define HID_REQ_SET_PROTOCOL		0x0B
>> +
>> +#define HID_USAGE_PAGE_LEN		76
>> +
>> +struct hid_report {
>> +	u8 usage_page[HID_USAGE_PAGE_LEN];
>> +} __packed;
>> +
>> +#define SDP_READ_REGISTER	0x0101
>> +#define SDP_WRITE_REGISTER	0x0202
>> +#define SDP_WRITE_FILE		0x0404
>> +#define SDP_ERROR_STATUS	0x0505
>> +#define SDP_DCD_WRITE		0x0a0a
>> +#define SDP_JUMP_ADDRESS	0x0b0b
>> +#define SDP_SKIP_DCD_HEADER	0x0c0c
> 
> It looks like that I am again out of sync with documentation. Where is
> defined SDP_SKIP_DCD_HEADER ? It is undefined for MX6Q/D, Solo and DL.
> 

This is only available in newer SoC's e.g. i.MX 7.

It allows to skip the DCD header in a downloaded image. Since the DCD
header is anyway ignored by this SDP implementation, the command is kind
of useless. I still think it is a good idea to have the command type
define for completeness... And I think also some SDP host side
implementation might issue the command...

>> +
>> +#define SDP_WRITE_FILE_COMPLETE		0x88888888
>> +#define SDP_WRITE_REGISTER_COMPLETE	0x128A8A12
>> +#define SDP_SKIP_DCD_HEADER_COMPLETE	0x900DD009
>> +#define SDP_ERROR_IMXHEADER		0x000a0533
>> +
>> +#define SDP_COMMAND_LEN		16
>> +
>> +struct sdp_command {
>> +	u16 cmd;
>> +	u32 addr;
>> +	u8 format;
>> +	u32 cnt;
>> +	u32 data;
>> +	u8 rsvd;
>> +} __packed;
>> +
>> +enum sdp_state {
>> +	SDP_STATE_IDLE,
>> +	SDP_STATE_RX_DCD_DATA,
>> +	SDP_STATE_RX_FILE_DATA,
>> +	SDP_STATE_TX_SEC_CONF,
>> +	SDP_STATE_TX_SEC_CONF_BUSY,
>> +	SDP_STATE_TX_REGISTER,
>> +	SDP_STATE_TX_REGISTER_BUSY,
>> +	SDP_STATE_TX_STATUS,
>> +	SDP_STATE_TX_STATUS_BUSY,
>> +	SDP_STATE_JUMP,
>> +};
>> +
>> +struct f_sdp {
>> +	struct usb_function		usb_function;
>> +
>> +	struct usb_descriptor_header	**function;
>> +
>> +	u8				altsetting;
>> +	enum sdp_state			state;
>> +	enum sdp_state			next_state;
>> +	u32				dnl_address;
>> +	u32				dnl_bytes_remaining;
>> +	u32				jmp_address;
>> +	bool				always_send_status;
>> +	u32				error_status;
>> +
>> +	/* EP0 request */
>> +	struct usb_request		*req;
>> +
>> +	/* EP1 IN */
>> +	struct usb_ep			*in_ep;
>> +	struct usb_request		*in_req;
>> +
>> +	bool				configuration_done;
>> +};
>> +
>> +static struct f_sdp *sdp_func;
>> +
>> +static inline struct f_sdp *func_to_sdp(struct usb_function *f)
>> +{
>> +	return container_of(f, struct f_sdp, usb_function);
>> +}
>> +
>> +static struct usb_interface_descriptor sdp_intf_runtime = {
>> +	.bLength =		sizeof(sdp_intf_runtime),
>> +	.bDescriptorType =	USB_DT_INTERFACE,
>> +	.bAlternateSetting =	0,
>> +	.bNumEndpoints =	1,
>> +	.bInterfaceClass =	USB_CLASS_HID,
>> +	.bInterfaceSubClass =	0,
>> +	.bInterfaceProtocol =	0,
>> +	/* .iInterface = DYNAMIC */
>> +};
>> +
>> +/* HID configuration */
>> +static struct usb_class_hid_descriptor sdp_hid_desc = {
>> +	.bLength =		sizeof(sdp_hid_desc),
>> +	.bDescriptorType =	USB_DT_CS_DEVICE,
>> +
>> +	.bcdCDC =		__constant_cpu_to_le16(0x0110),
>> +	.bCountryCode =		0,
>> +	.bNumDescriptors =	1,
>> +
>> +	.bDescriptorType0	= USB_DT_HID_REPORT,
>> +	.wDescriptorLength0	= HID_USAGE_PAGE_LEN,
>> +};
>> +
>> +static struct usb_endpoint_descriptor in_desc = {
>> +	.bLength =		USB_DT_ENDPOINT_SIZE,
>> +	.bDescriptorType =	USB_DT_ENDPOINT, /*USB_DT_CS_ENDPOINT*/
>> +
>> +	.bEndpointAddress =	1 | USB_DIR_IN,
>> +	.bmAttributes =	USB_ENDPOINT_XFER_INT,
>> +	.wMaxPacketSize =	64,
>> +	.bInterval =		1,
>> +};
>> +
>> +static struct usb_descriptor_header *sdp_runtime_descs[] = {
>> +	(struct usb_descriptor_header *)&sdp_intf_runtime,
>> +	(struct usb_descriptor_header *)&sdp_hid_desc,
>> +	(struct usb_descriptor_header *)&in_desc,
>> +	NULL,
>> +};
>> +
>> +/* This is synchronized with what the SoC implementation reports */
>> +static struct hid_report sdp_hid_report = {
>> +	.usage_page = {
>> +		0x06, 0x00, 0xff, /* Usage Page */
>> +		0x09, 0x01, /* Usage (Poiter?) */
>> +		0xa1, 0x01, /* Collection */
>> +
>> +		0x85, 0x01, /* Report ID */
>> +		0x19, 0x01, /* Usage Minimum */
>> +		0x29, 0x01, /* Usage Maximum */
>> +		0x15, 0x00, /* Local Minimum */
>> +		0x26, 0xFF, 0x00, /* Local Maximum? */
>> +		0x75, 0x08, /* Report Size */
>> +		0x95, 0x10, /* Report Count */
>> +		0x91, 0x02, /* Output Data */
>> +
>> +		0x85, 0x02, /* Report ID */
>> +		0x19, 0x01, /* Usage Minimum */
>> +		0x29, 0x01, /* Usage Maximum */
>> +		0x15, 0x00, /* Local Minimum */
>> +		0x26, 0xFF, 0x00, /* Local Maximum? */
>> +		0x75, 0x80, /* Report Size 128 */
>> +		0x95, 0x40, /* Report Count */
>> +		0x91, 0x02, /* Output Data */
>> +
>> +		0x85, 0x03, /* Report ID */
>> +		0x19, 0x01, /* Usage Minimum */
>> +		0x29, 0x01, /* Usage Maximum */
>> +		0x15, 0x00, /* Local Minimum */
>> +		0x26, 0xFF, 0x00, /* Local Maximum? */
>> +		0x75, 0x08, /* Report Size 8 */
>> +		0x95, 0x04, /* Report Count */
>> +		0x81, 0x02, /* Input Data */
>> +
>> +		0x85, 0x04, /* Report ID */
>> +		0x19, 0x01, /* Usage Minimum */
>> +		0x29, 0x01, /* Usage Maximum */
>> +		0x15, 0x00, /* Local Minimum */
>> +		0x26, 0xFF, 0x00, /* Local Maximum? */
>> +		0x75, 0x08, /* Report Size 8 */
>> +		0x95, 0x40, /* Report Count */
>> +		0x81, 0x02, /* Input Data */
>> +		0xc0
>> +	},
>> +};
>> +
>> +static const char sdp_name[] = "Serial Downloader Protocol";
>> +
>> +/*
>> + * static strings, in UTF-8
>> + */
>> +static struct usb_string strings_sdp_generic[] = {
>> +	[0].s = sdp_name,
>> +	{  }			/* end of list */
>> +};
>> +
>> +static struct usb_gadget_strings stringtab_sdp_generic = {
>> +	.language	= 0x0409,	/* en-us */
>> +	.strings	= strings_sdp_generic,
>> +};
>> +
>> +static struct usb_gadget_strings *sdp_generic_strings[] = {
>> +	&stringtab_sdp_generic,
>> +	NULL,
>> +};
>> +
>> +static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req)
>> +{
>> +	struct f_sdp *sdp = req->context;
>> +	int status = req->status;
>> +	u8 *data = req->buf;
>> +	u8 report = data[0];
>> +
>> +	if (status != 0) {
>> +		error("Status: %d", status);
>> +		return;
>> +	}
>> +
>> +	if (report != 1) {
>> +		error("Unexpected report %d", report);
>> +		return;
>> +	}
>> +
>> +	struct sdp_command *cmd = req->buf + 1;
>> +
>> +	debug("%s: command: %04x, addr: %08x, cnt: %u\n",
>> +	      __func__, be16_to_cpu(cmd->cmd),
>> +	      be32_to_cpu(cmd->addr), be32_to_cpu(cmd->cnt));
>> +
>> +	switch (be16_to_cpu(cmd->cmd)) {
>> +	case SDP_READ_REGISTER:
>> +		sdp->always_send_status = false;
>> +		sdp->error_status = 0x0;
>> +
>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
>> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
>> +		sdp->next_state = SDP_STATE_TX_REGISTER;
>> +		printf("Reading %d registers at 0x%08x... ",
>> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
>> +		break;
>> +	case SDP_WRITE_FILE:
>> +		sdp->always_send_status = true;
>> +		sdp->error_status = SDP_WRITE_FILE_COMPLETE;
>> +
>> +		sdp->state = SDP_STATE_RX_FILE_DATA;
>> +		sdp->dnl_address = be32_to_cpu(cmd->addr);
>> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
>> +		sdp->next_state = SDP_STATE_IDLE;
>> +
>> +		printf("Downloading file of size %d to 0x%08x... ",
>> +		       sdp->dnl_bytes_remaining, sdp->dnl_address);
>> +
>> +		break;
>> +	case SDP_ERROR_STATUS:
>> +		sdp->always_send_status = true;
>> +		sdp->error_status = 0;
>> +
>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>> +		sdp->next_state = SDP_STATE_IDLE;
>> +		break;
>> +	case SDP_DCD_WRITE:
>> +		sdp->always_send_status = true;
>> +		sdp->error_status = SDP_WRITE_REGISTER_COMPLETE;
>> +
>> +		sdp->state = SDP_STATE_RX_DCD_DATA;
>> +		sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt);
>> +		sdp->next_state = SDP_STATE_IDLE;
>> +		break;
> 
> It is fine, but I am just missing if this is a use case. DCD is
> interpreted by boot ROM, and we are here already over in SPL.
> 

I also don't have a use case currently, but it is rather cheap so why
don't?

Note that the SDP_READ_REGISTER command is actually used by the
sb_loader to do some verification whether the image got correctly
downloaded... But I don't think it required DCD_WRITE for something, but
I would have to retest.

>> +	case SDP_JUMP_ADDRESS:
>> +		sdp->always_send_status = false;
>> +		sdp->error_status = 0;
>> +
>> +		sdp->jmp_address = be32_to_cpu(cmd->addr);
>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>> +		sdp->next_state = SDP_STATE_JUMP;
>> +		break;
>> +	case SDP_SKIP_DCD_HEADER:
>> +		sdp->always_send_status = true;
>> +		sdp->error_status = SDP_SKIP_DCD_HEADER_COMPLETE;
>> +
>> +		/* Ignore command, DCD not supported anyway */
> 
> Right - we load a file, we do not need a DCD.
> 
>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>> +		sdp->next_state = SDP_STATE_IDLE;
>> +		break;
>> +	default:
>> +		error("Unknown command: %08x\n", be16_to_cpu(cmd->cmd));
>> +	}
>> +}
>> +
>> +static void sdp_rx_data_complete(struct usb_ep *ep, struct usb_request *req)
>> +{
>> +	struct f_sdp *sdp = req->context;
>> +	int status = req->status;
>> +	u8 *data = req->buf;
>> +	u8 report = data[0];
>> +	int datalen = req->length - 1;
>> +
>> +	if (status != 0) {
>> +		error("Status: %d", status);
>> +		return;
>> +	}
>> +
>> +	if (report != 2) {
>> +		error("Unexpected report %d", report);
>> +		return;
>> +	}
>> +
>> +	if (sdp->dnl_bytes_remaining < datalen) {
>> +		/*
>> +		 * Some USB stacks require to send a complete buffer as
>> +		 * specified in the HID descriptor. This leads to longer
>> +		 * transfers than the file length, no problem for us.
>> +		 */
>> +		sdp->dnl_bytes_remaining = 0;
>> +	} else {
>> +		sdp->dnl_bytes_remaining -= datalen;
>> +	}
>> +
>> +	if (sdp->state == SDP_STATE_RX_FILE_DATA) {
>> +		memcpy((void *)sdp->dnl_address, req->buf + 1, datalen);
>> +		sdp->dnl_address += datalen;
>> +	}
>> +
>> +	if (sdp->dnl_bytes_remaining)
>> +		return;
>> +
>> +	printf("done\n");
>> +
>> +	switch (sdp->state) {
>> +	case SDP_STATE_RX_FILE_DATA:
>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>> +		break;
>> +	case SDP_STATE_RX_DCD_DATA:
>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>> +		break;
>> +	default:
>> +		error("Invalid state: %d", sdp->state);
>> +	}
>> +}
>> +
>> +
>> +
>> +static void sdp_tx_complete(struct usb_ep *ep, struct usb_request *req)
>> +{
>> +	struct f_sdp *sdp = req->context;
>> +	int status = req->status;
>> +
>> +	if (status != 0) {
>> +		error("Status: %d", status);
>> +		return;
>> +	}
>> +
>> +	switch (sdp->state) {
>> +	case SDP_STATE_TX_SEC_CONF_BUSY:
>> +		/* Not all commands require status report */
>> +		if (sdp->always_send_status || sdp->error_status)
>> +			sdp->state = SDP_STATE_TX_STATUS;
>> +		else
>> +			sdp->state = sdp->next_state;
>> +
>> +		break;
>> +	case SDP_STATE_TX_STATUS_BUSY:
>> +		sdp->state = sdp->next_state;
>> +		break;
>> +	case SDP_STATE_TX_REGISTER_BUSY:
>> +		if (sdp->dnl_bytes_remaining)
>> +			sdp->state = SDP_STATE_TX_REGISTER;
>> +		else
>> +			sdp->state = SDP_STATE_IDLE;
>> +		break;
>> +	default:
>> +		error("Wrong State: %d", sdp->state);
>> +		sdp->state = SDP_STATE_IDLE;
>> +		break;
>> +	}
>> +	debug("%s complete --> %d, %d/%d\n", ep->name,
>> +	      status, req->actual, req->length);
>> +}
>> +
>> +static int sdp_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>> +{
>> +	struct usb_gadget *gadget = f->config->cdev->gadget;
>> +	struct usb_request *req = f->config->cdev->req;
>> +	struct f_sdp *sdp = f->config->cdev->req->context;
>> +	u16 len = le16_to_cpu(ctrl->wLength);
>> +	u16 w_value = le16_to_cpu(ctrl->wValue);
>> +	int value = 0;
>> +	u8 req_type = ctrl->bRequestType & USB_TYPE_MASK;
>> +
>> +	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
>> +	debug("req_type: 0x%x ctrl->bRequest: 0x%x sdp->state: %d\n",
>> +	      req_type, ctrl->bRequest, sdp->state);
>> +
>> +	if (req_type == USB_TYPE_STANDARD) {
>> +		if (ctrl->bRequest == USB_REQ_GET_DESCRIPTOR) {
>> +			/* Send HID report descriptor */
>> +			value = min(len, (u16) sizeof(sdp_hid_report));
>> +			memcpy(req->buf, &sdp_hid_report, value);
>> +			sdp->configuration_done = true;
>> +		}
>> +	}
>> +
>> +	if (req_type == USB_TYPE_CLASS) {
>> +		int report = w_value & HID_REPORT_ID_MASK;
>> +
>> +		/* HID (SDP) request */
>> +		switch (ctrl->bRequest) {
>> +		case HID_REQ_SET_REPORT:
>> +			switch (report) {
>> +			case 1:
>> +				value = SDP_COMMAND_LEN + 1;
>> +				req->complete = sdp_rx_command_complete;
>> +				break;
>> +			case 2:
>> +				value = len;
>> +				req->complete = sdp_rx_data_complete;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	if (value >= 0) {
>> +		req->length = value;
>> +		req->zero = value < len;
>> +		value = usb_ep_queue(gadget->ep0, req, 0);
>> +		if (value < 0) {
>> +			debug("ep_queue --> %d\n", value);
>> +			req->status = 0;
>> +		}
>> +	}
>> +
>> +	return value;
>> +}
>> +
>> +static int sdp_bind(struct usb_configuration *c, struct usb_function *f)
>> +{
>> +	struct usb_gadget *gadget = c->cdev->gadget;
>> +	struct usb_composite_dev *cdev = c->cdev;
>> +	struct f_sdp *sdp = func_to_sdp(f);
>> +	int rv = 0, id;
>> +
>> +	id = usb_interface_id(c, f);
>> +	if (id < 0)
>> +		return id;
>> +	sdp_intf_runtime.bInterfaceNumber = id;
>> +
>> +	struct usb_ep *ep;
>> +
>> +	/* allocate instance-specific endpoints */
>> +	ep = usb_ep_autoconfig(gadget, &in_desc);
>> +	if (!ep) {
>> +		rv = -ENODEV;
>> +		goto error;
>> +	}
>> +
>> +	sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
>> +
>> +	cdev->req->context = sdp;
>> +
>> +error:
>> +	return rv;
>> +}
>> +
>> +static void sdp_unbind(struct usb_configuration *c, struct usb_function *f)
>> +{
>> +	free(sdp_func);
>> +	sdp_func = NULL;
>> +}
>> +
>> +static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length)
>> +{
>> +	struct usb_request *req;
>> +
>> +	req = usb_ep_alloc_request(ep, 0);
>> +	if (!req)
>> +		return req;
>> +
>> +	req->length = length;
>> +	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, length);
>> +	if (!req->buf) {
>> +		usb_ep_free_request(ep, req);
>> +		req = NULL;
>> +	}
>> +
>> +	return req;
>> +}
>> +
>> +
>> +static struct usb_request *sdp_start_ep(struct usb_ep *ep)
>> +{
>> +	struct usb_request *req;
>> +
>> +	req = alloc_ep_req(ep, 64);
>> +	debug("%s: ep:%p req:%p\n", __func__, ep, req);
>> +
>> +	if (!req)
>> +		return NULL;
>> +
>> +	memset(req->buf, 0, req->length);
>> +	req->complete = sdp_tx_complete;
>> +
>> +	return req;
>> +}
>> +static int sdp_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>> +{
>> +	struct f_sdp *sdp = func_to_sdp(f);
>> +	struct usb_composite_dev *cdev = f->config->cdev;
>> +	int result;
>> +
>> +	debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
>> +
>> +	result = usb_ep_enable(sdp->in_ep, &in_desc);
>> +	if (result)
>> +		return result;
>> +	sdp->in_req = sdp_start_ep(sdp->in_ep);
>> +	sdp->in_req->context = sdp;
>> +
>> +	sdp->in_ep->driver_data = cdev; /* claim */
>> +
>> +	sdp->altsetting = alt;
>> +	sdp->state = SDP_STATE_IDLE;
>> +
>> +	return 0;
>> +}
>> +
>> +static int sdp_get_alt(struct usb_function *f, unsigned intf)
>> +{
>> +	struct f_sdp *sdp = func_to_sdp(f);
>> +
>> +	return sdp->altsetting;
>> +}
>> +
>> +static void sdp_disable(struct usb_function *f)
>> +{
>> +	struct f_sdp *sdp = func_to_sdp(f);
>> +
>> +	usb_ep_disable(sdp->in_ep);
>> +
>> +	if (sdp->in_req) {
>> +		free(sdp->in_req);
>> +		sdp->in_req = NULL;
>> +	}
>> +}
>> +
>> +static int sdp_bind_config(struct usb_configuration *c)
>> +{
>> +	int status;
>> +
>> +	if (!sdp_func) {
>> +		sdp_func = memalign(CONFIG_SYS_CACHELINE_SIZE, sizeof(*sdp_func));
>> +		if (!sdp_func)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	memset(sdp_func, 0, sizeof(*sdp_func));
>> +
>> +	sdp_func->usb_function.name = "sdp";
>> +	sdp_func->usb_function.hs_descriptors = sdp_runtime_descs;
>> +	sdp_func->usb_function.descriptors = sdp_runtime_descs;
>> +	sdp_func->usb_function.bind = sdp_bind;
>> +	sdp_func->usb_function.unbind = sdp_unbind;
>> +	sdp_func->usb_function.set_alt = sdp_set_alt;
>> +	sdp_func->usb_function.get_alt = sdp_get_alt;
>> +	sdp_func->usb_function.disable = sdp_disable;
>> +	sdp_func->usb_function.strings = sdp_generic_strings;
>> +	sdp_func->usb_function.setup = sdp_setup;
>> +
>> +	status = usb_add_function(c, &sdp_func->usb_function);
>> +
>> +	return status;
>> +}
>> +
>> +int sdp_init(void)
>> +{
>> +	printf("SDP: initialize...\n");
>> +	while (!sdp_func->configuration_done) {
>> +		if (ctrlc()) {
>> +			puts("\rCTRL+C - Operation aborted.\n");
>> +			return 0;
>> +		}
>> +		usb_gadget_handle_interrupts(0);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static u32 sdp_jump_imxheader(void *address)
>> +{
>> +	flash_header_v2_t *headerv2 = address;
>> +	ulong (*entry)(void);
>> +
>> +	if (headerv2->header.tag != IVT_HEADER_TAG) {
>> +		printf("Header Tag is not a IMX image\n");
>> +		return SDP_ERROR_IMXHEADER;
>> +	}
>> +
>> +	printf("Jumping to 0x%08x\n", headerv2->entry);
>> +	entry = (void *)headerv2->entry;
>> +	entry();
>> +
>> +	/* The image probably never returns hence we wont reach that point */
>> +	return 0;
>> +}
>> +
>> +static void sdp_handle_in_ep(void)
>> +{
>> +	u8 *data = sdp_func->in_req->buf;
>> +	u32 status;
>> +	int datalen;
>> +
>> +	switch (sdp_func->state) {
>> +	case SDP_STATE_TX_SEC_CONF:
>> +		debug("Report 3: HAB security\n");
>> +		data[0] = 3;
>> +
>> +		data[1] = 0x56;
>> +		data[2] = 0x78;
>> +		data[3] = 0x78;
>> +		data[4] = 0x56;
> 
> I am quite lost here - can you explain what are these magic numbers, and
> maybe add a comment for it (or self explaining defines) ?
> 

Yeah protocol specific magic number:
HAB security
configuration. Device
sends 0x12343412 in
closed mode and
0x56787856 in open
mode.


We always assume open. Not sure what kind of implication that can have,
I think imx_usb basically just prints out what the device says.

Will create proper defines.

--
Stefan

>> +
>> +		sdp_func->in_req->length = 5;
>> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
>> +		sdp_func->state = SDP_STATE_TX_SEC_CONF_BUSY;
>> +		break;
>> +
>> +	case SDP_STATE_TX_STATUS:
>> +		debug("Report 4: Status\n");
>> +		data[0] = 4;
>> +
>> +		memcpy(&data[1], &sdp_func->error_status, 4);
>> +		sdp_func->in_req->length = 65;
>> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
>> +		sdp_func->state = SDP_STATE_TX_STATUS_BUSY;
>> +		break;
>> +	case SDP_STATE_TX_REGISTER:
>> +		debug("Report 4: Register Values\n");
>> +		data[0] = 4;
>> +
>> +		datalen = sdp_func->dnl_bytes_remaining;
>> +
>> +		if (datalen > 64)
>> +			datalen = 64;
>> +
>> +		memcpy(&data[1], (void *)sdp_func->dnl_address, datalen);
>> +		sdp_func->in_req->length = 65;
>> +
>> +		sdp_func->dnl_bytes_remaining -= datalen;
>> +		sdp_func->dnl_address += datalen;
>> +
>> +		usb_ep_queue(sdp_func->in_ep, sdp_func->in_req, 0);
>> +		sdp_func->state = SDP_STATE_TX_REGISTER_BUSY;
>> +		break;
>> +	case SDP_STATE_JUMP:
>> +		printf("Checking imxheader at 0x%08x\n", f_sdp->jmp_address);
>> +		status = sdp_jump_imxheader((void *)f_sdp->jmp_address);
>> +
>> +		sdp_func->next_state = SDP_STATE_IDLE;
>> +		sdp_func->error_status = status;
>> +
>> +		/* Only send Report 4 if there was an error */
>> +		if (status)
>> +			sdp_func->state = SDP_STATE_TX_STATUS;
>> +		else
>> +			sdp_func->state = SDP_STATE_IDLE;
>> +		break;
>> +	default:
>> +		break;
>> +	};
>> +}
>> +
>> +int sdp_handle(void)
>> +{
>> +	printf("SDP: handle requests...\n");
>> +	while (1) {
>> +		if (ctrlc()) {
>> +			puts("\rCTRL+C - Operation aborted.\n");
>> +			return 0;
>> +		}
>> +
>> +		usb_gadget_handle_interrupts(0);
>> +
>> +		sdp_handle_in_ep();
>> +	}
>> +}
>> +
>> +int sdp_add(struct usb_configuration *c)
>> +{
>> +	int id;
>> +
>> +	id = usb_string_id(c->cdev);
>> +	if (id < 0)
>> +		return id;
>> +	strings_sdp_generic[0].id = id;
>> +	sdp_intf_runtime.iInterface = id;
>> +
>> +	debug("%s: cdev: 0x%p gadget:0x%p gadget->ep0: 0x%p\n", __func__,
>> +	      c->cdev, c->cdev->gadget, c->cdev->gadget->ep0);
>> +
>> +	return sdp_bind_config(c);
>> +}
>> +
>> +DECLARE_GADGET_BIND_CALLBACK(usb_dnl_sdp, sdp_add);
>> diff --git a/include/sdp.h b/include/sdp.h
>> new file mode 100644
>> index 0000000000..03c4a23434
>> --- /dev/null
>> +++ b/include/sdp.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * sdp.h - Serial Download Protocol
>> + *
>> + * Copyright (C) 2016 Toradex
>> + * Author: Stefan Agner <stefan.agner@toradex.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#ifndef __SDP_H_
>> +#define __SDP_H_
>> +
>> +int sdp_init(void);
>> +int sdp_handle(void);
>> +
>> +#endif /* __SDP_H_ */
>>
> 
> Best regards,
> Stefano

[U-Boot] [PATCH v1 2/7] usb: gadget: add SDP driver

[Stefano Babic]

Hi Stefan,

On 15/08/2017 23:54, Stefan Agner wrote:
>>
>> It looks like that I am again out of sync with documentation. Where is
>> defined SDP_SKIP_DCD_HEADER ? It is undefined for MX6Q/D, Solo and DL.
>>
> 
> This is only available in newer SoC's e.g. i.MX 7.

ok

> 
> It allows to skip the DCD header in a downloaded image. Since the DCD
> header is anyway ignored by this SDP implementation, the command is kind
> of useless. I still think it is a good idea to have the command type
> define for completeness... 

I agree with you - I just wanted to understand.

>And I think also some SDP host side
> implementation might issue the command...

That's ok, thanks for clarification.

>>
>> It is fine, but I am just missing if this is a use case. DCD is
>> interpreted by boot ROM, and we are here already over in SPL.
>>
> 
> I also don't have a use case currently, but it is rather cheap so why
> don't?

Yes, that's fine.

> 
> Note that the SDP_READ_REGISTER command is actually used by the
> sb_loader to do some verification whether the image got correctly
> downloaded... But I don't think it required DCD_WRITE for something, but
> I would have to retest.
> 
>>> +	case SDP_JUMP_ADDRESS:
>>> +		sdp->always_send_status = false;
>>> +		sdp->error_status = 0;
>>> +
>>> +		sdp->jmp_address = be32_to_cpu(cmd->addr);
>>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>>> +		sdp->next_state = SDP_STATE_JUMP;
>>> +		break;
>>> +	case SDP_SKIP_DCD_HEADER:
>>> +		sdp->always_send_status = true;
>>> +		sdp->error_status = SDP_SKIP_DCD_HEADER_COMPLETE;
>>> +
>>> +		/* Ignore command, DCD not supported anyway */
>>
>> Right - we load a file, we do not need a DCD.
>>
>>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>>> +		sdp->next_state = SDP_STATE_IDLE;
>>> +		break;
>>> +	default:
>>> +		error("Unknown command: %08x\n", be16_to_cpu(cmd->cmd));
>>> +	}
>>> +}
>>> +
>>> +static void sdp_rx_data_complete(struct usb_ep *ep, struct usb_request *req)
>>> +{
>>> +	struct f_sdp *sdp = req->context;
>>> +	int status = req->status;
>>> +	u8 *data = req->buf;
>>> +	u8 report = data[0];
>>> +	int datalen = req->length - 1;
>>> +
>>> +	if (status != 0) {
>>> +		error("Status: %d", status);
>>> +		return;
>>> +	}
>>> +
>>> +	if (report != 2) {
>>> +		error("Unexpected report %d", report);
>>> +		return;
>>> +	}
>>> +
>>> +	if (sdp->dnl_bytes_remaining < datalen) {
>>> +		/*
>>> +		 * Some USB stacks require to send a complete buffer as
>>> +		 * specified in the HID descriptor. This leads to longer
>>> +		 * transfers than the file length, no problem for us.
>>> +		 */
>>> +		sdp->dnl_bytes_remaining = 0;
>>> +	} else {
>>> +		sdp->dnl_bytes_remaining -= datalen;
>>> +	}
>>> +
>>> +	if (sdp->state == SDP_STATE_RX_FILE_DATA) {
>>> +		memcpy((void *)sdp->dnl_address, req->buf + 1, datalen);
>>> +		sdp->dnl_address += datalen;
>>> +	}
>>> +
>>> +	if (sdp->dnl_bytes_remaining)
>>> +		return;
>>> +
>>> +	printf("done\n");
>>> +
>>> +	switch (sdp->state) {
>>> +	case SDP_STATE_RX_FILE_DATA:
>>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>>> +		break;
>>> +	case SDP_STATE_RX_DCD_DATA:
>>> +		sdp->state = SDP_STATE_TX_SEC_CONF;
>>> +		break;
>>> +	default:
>>> +		error("Invalid state: %d", sdp->state);
>>> +	}
>>> +}
>>> +
>>> +
>>> +
>>> +static void sdp_tx_complete(struct usb_ep *ep, struct usb_request *req)
>>> +{
>>> +	struct f_sdp *sdp = req->context;
>>> +	int status = req->status;
>>> +
>>> +	if (status != 0) {
>>> +		error("Status: %d", status);
>>> +		return;
>>> +	}
>>> +
>>> +	switch (sdp->state) {
>>> +	case SDP_STATE_TX_SEC_CONF_BUSY:
>>> +		/* Not all commands require status report */
>>> +		if (sdp->always_send_status || sdp->error_status)
>>> +			sdp->state = SDP_STATE_TX_STATUS;
>>> +		else
>>> +			sdp->state = sdp->next_state;
>>> +
>>> +		break;
>>> +	case SDP_STATE_TX_STATUS_BUSY:
>>> +		sdp->state = sdp->next_state;
>>> +		break;
>>> +	case SDP_STATE_TX_REGISTER_BUSY:
>>> +		if (sdp->dnl_bytes_remaining)
>>> +			sdp->state = SDP_STATE_TX_REGISTER;
>>> +		else
>>> +			sdp->state = SDP_STATE_IDLE;
>>> +		break;
>>> +	default:
>>> +		error("Wrong State: %d", sdp->state);
>>> +		sdp->state = SDP_STATE_IDLE;
>>> +		break;
>>> +	}
>>> +	debug("%s complete --> %d, %d/%d\n", ep->name,
>>> +	      status, req->actual, req->length);
>>> +}
>>> +
>>> +static int sdp_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>>> +{
>>> +	struct usb_gadget *gadget = f->config->cdev->gadget;
>>> +	struct usb_request *req = f->config->cdev->req;
>>> +	struct f_sdp *sdp = f->config->cdev->req->context;
>>> +	u16 len = le16_to_cpu(ctrl->wLength);
>>> +	u16 w_value = le16_to_cpu(ctrl->wValue);
>>> +	int value = 0;
>>> +	u8 req_type = ctrl->bRequestType & USB_TYPE_MASK;
>>> +
>>> +	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
>>> +	debug("req_type: 0x%x ctrl->bRequest: 0x%x sdp->state: %d\n",
>>> +	      req_type, ctrl->bRequest, sdp->state);
>>> +
>>> +	if (req_type == USB_TYPE_STANDARD) {
>>> +		if (ctrl->bRequest == USB_REQ_GET_DESCRIPTOR) {
>>> +			/* Send HID report descriptor */
>>> +			value = min(len, (u16) sizeof(sdp_hid_report));
>>> +			memcpy(req->buf, &sdp_hid_report, value);
>>> +			sdp->configuration_done = true;
>>> +		}
>>> +	}
>>> +
>>> +	if (req_type == USB_TYPE_CLASS) {
>>> +		int report = w_value & HID_REPORT_ID_MASK;
>>> +
>>> +		/* HID (SDP) request */
>>> +		switch (ctrl->bRequest) {
>>> +		case HID_REQ_SET_REPORT:
>>> +			switch (report) {
>>> +			case 1:
>>> +				value = SDP_COMMAND_LEN + 1;
>>> +				req->complete = sdp_rx_command_complete;
>>> +				break;
>>> +			case 2:
>>> +				value = len;
>>> +				req->complete = sdp_rx_data_complete;
>>> +				break;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	if (value >= 0) {
>>> +		req->length = value;
>>> +		req->zero = value < len;
>>> +		value = usb_ep_queue(gadget->ep0, req, 0);
>>> +		if (value < 0) {
>>> +			debug("ep_queue --> %d\n", value);
>>> +			req->status = 0;
>>> +		}
>>> +	}
>>> +
>>> +	return value;
>>> +}
>>> +
>>> +static int sdp_bind(struct usb_configuration *c, struct usb_function *f)
>>> +{
>>> +	struct usb_gadget *gadget = c->cdev->gadget;
>>> +	struct usb_composite_dev *cdev = c->cdev;
>>> +	struct f_sdp *sdp = func_to_sdp(f);
>>> +	int rv = 0, id;
>>> +
>>> +	id = usb_interface_id(c, f);
>>> +	if (id < 0)
>>> +		return id;
>>> +	sdp_intf_runtime.bInterfaceNumber = id;
>>> +
>>> +	struct usb_ep *ep;
>>> +
>>> +	/* allocate instance-specific endpoints */
>>> +	ep = usb_ep_autoconfig(gadget, &in_desc);
>>> +	if (!ep) {
>>> +		rv = -ENODEV;
>>> +		goto error;
>>> +	}
>>> +
>>> +	sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
>>> +
>>> +	cdev->req->context = sdp;
>>> +
>>> +error:
>>> +	return rv;
>>> +}
>>> +
>>> +static void sdp_unbind(struct usb_configuration *c, struct usb_function *f)
>>> +{
>>> +	free(sdp_func);
>>> +	sdp_func = NULL;
>>> +}
>>> +
>>> +static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length)
>>> +{
>>> +	struct usb_request *req;
>>> +
>>> +	req = usb_ep_alloc_request(ep, 0);
>>> +	if (!req)
>>> +		return req;
>>> +
>>> +	req->length = length;
>>> +	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, length);
>>> +	if (!req->buf) {
>>> +		usb_ep_free_request(ep, req);
>>> +		req = NULL;
>>> +	}
>>> +
>>> +	return req;
>>> +}
>>> +
>>> +
>>> +static struct usb_request *sdp_start_ep(struct usb_ep *ep)
>>> +{
>>> +	struct usb_request *req;
>>> +
>>> +	req = alloc_ep_req(ep, 64);
>>> +	debug("%s: ep:%p req:%p\n", __func__, ep, req);
>>> +
>>> +	if (!req)
>>> +		return NULL;
>>> +
>>> +	memset(req->buf, 0, req->length);
>>> +	req->complete = sdp_tx_complete;
>>> +
>>> +	return req;
>>> +}
>>> +static int sdp_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>> +{
>>> +	struct f_sdp *sdp = func_to_sdp(f);
>>> +	struct usb_composite_dev *cdev = f->config->cdev;
>>> +	int result;
>>> +
>>> +	debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
>>> +
>>> +	result = usb_ep_enable(sdp->in_ep, &in_desc);
>>> +	if (result)
>>> +		return result;
>>> +	sdp->in_req = sdp_start_ep(sdp->in_ep);
>>> +	sdp->in_req->context = sdp;
>>> +
>>> +	sdp->in_ep->driver_data = cdev; /* claim */
>>> +
>>> +	sdp->altsetting = alt;
>>> +	sdp->state = SDP_STATE_IDLE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sdp_get_alt(struct usb_function *f, unsigned intf)
>>> +{
>>> +	struct f_sdp *sdp = func_to_sdp(f);
>>> +
>>> +	return sdp->altsetting;
>>> +}
>>> +
>>> +static void sdp_disable(struct usb_function *f)
>>> +{
>>> +	struct f_sdp *sdp = func_to_sdp(f);
>>> +
>>> +	usb_ep_disable(sdp->in_ep);
>>> +
>>> +	if (sdp->in_req) {
>>> +		free(sdp->in_req);
>>> +		sdp->in_req = NULL;
>>> +	}
>>> +}
>>> +
>>> +static int sdp_bind_config(struct usb_configuration *c)
>>> +{
>>> +	int status;
>>> +
>>> +	if (!sdp_func) {
>>> +		sdp_func = memalign(CONFIG_SYS_CACHELINE_SIZE, sizeof(*sdp_func));
>>> +		if (!sdp_func)
>>> +			return -ENOMEM;
>>> +	}
>>> +
>>> +	memset(sdp_func, 0, sizeof(*sdp_func));
>>> +
>>> +	sdp_func->usb_function.name = "sdp";
>>> +	sdp_func->usb_function.hs_descriptors = sdp_runtime_descs;
>>> +	sdp_func->usb_function.descriptors = sdp_runtime_descs;
>>> +	sdp_func->usb_function.bind = sdp_bind;
>>> +	sdp_func->usb_function.unbind = sdp_unbind;
>>> +	sdp_func->usb_function.set_alt = sdp_set_alt;
>>> +	sdp_func->usb_function.get_alt = sdp_get_alt;
>>> +	sdp_func->usb_function.disable = sdp_disable;
>>> +	sdp_func->usb_function.strings = sdp_generic_strings;
>>> +	sdp_func->usb_function.setup = sdp_setup;
>>> +
>>> +	status = usb_add_function(c, &sdp_func->usb_function);
>>> +
>>> +	return status;
>>> +}
>>> +
>>> +int sdp_init(void)
>>> +{
>>> +	printf("SDP: initialize...\n");
>>> +	while (!sdp_func->configuration_done) {
>>> +		if (ctrlc()) {
>>> +			puts("\rCTRL+C - Operation aborted.\n");
>>> +			return 0;
>>> +		}
>>> +		usb_gadget_handle_interrupts(0);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static u32 sdp_jump_imxheader(void *address)
>>> +{
>>> +	flash_header_v2_t *headerv2 = address;
>>> +	ulong (*entry)(void);
>>> +
>>> +	if (headerv2->header.tag != IVT_HEADER_TAG) {
>>> +		printf("Header Tag is not a IMX image\n");
>>> +		return SDP_ERROR_IMXHEADER;
>>> +	}
>>> +
>>> +	printf("Jumping to 0x%08x\n", headerv2->entry);
>>> +	entry = (void *)headerv2->entry;
>>> +	entry();
>>> +
>>> +	/* The image probably never returns hence we wont reach that point */
>>> +	return 0;
>>> +}
>>> +
>>> +static void sdp_handle_in_ep(void)
>>> +{
>>> +	u8 *data = sdp_func->in_req->buf;
>>> +	u32 status;
>>> +	int datalen;
>>> +
>>> +	switch (sdp_func->state) {
>>> +	case SDP_STATE_TX_SEC_CONF:
>>> +		debug("Report 3: HAB security\n");
>>> +		data[0] = 3;
>>> +
>>> +		data[1] = 0x56;
>>> +		data[2] = 0x78;
>>> +		data[3] = 0x78;
>>> +		data[4] = 0x56;
>>
>> I am quite lost here - can you explain what are these magic numbers, and
>> maybe add a comment for it (or self explaining defines) ?
>>
> 
> Yeah protocol specific magic number:
> HAB security
> configuration. Device
> sends 0x12343412 in
> closed mode and
> 0x56787856 in open
> mode.
> 
> 
> We always assume open. Not sure what kind of implication that can have,
> I think imx_usb basically just prints out what the device says.
> 
> Will create proper defines.

Thanks.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================