[U-Boot] [PATCH v4 0/4] Make some changes to SDP

Changes in v4: - Add SDP_USB_REQUEST_BUFLEN instead of magic number (65) in patch3/4. - Remove the unnecessary blank lines in patch3/4.
Changes in v3: - Add reviewed-by tag for patch2/4 and patch4/4 since this two have been reviewed in v1.
Changes in v2: - Update the commit log in patch 1/4.
This patchset adds: 1. Add usb_gadget_initialize() and usb_gadget_release() to initialize and release UDC during sdp download. 2. Add high speed endpoint descriptor for sdp. 3. Add a macro definition--CONFIG_SDP_LOADADDR as default sdp load address while SDP_WRITE and SDP_JUMP command addr is zero.
The current patches have been validated on both i.MX8 and i.MX8M platform.
Sherry Sun (4): imx: spl: Change USB boot device type for imx8/8m SDP: use CONFIG_SDP_LOADADDR as default load address SDP: fix wrong usb request size and add high speed endpoint descriptor SDP: Call usb_gadget_initialize and usb_gadget_release to support UDC
arch/arm/mach-imx/spl.c | 2 +- common/spl/spl_sdp.c | 4 ++++ drivers/usb/gadget/Kconfig | 4 ++++ drivers/usb/gadget/f_sdp.c | 43 +++++++++++++++++++++++++++++++++----- 4 files changed, 47 insertions(+), 6 deletions(-)

The SPL SDP is configured as BOOT_DEVICE_BOARD, but for i.MX8 and i.MX8M, the boot_device_spl is still set to BOOT_DEVICE_USB, which may cause SDP can't work, in order to fix this issue, when booting from USB in spl, we change its type to BOOT_DEVICE_BOARD.
Signed-off-by: Sherry Sun sherry.sun@nxp.com Signed-off-by: Ye Li ye.li@nxp.com Reviewed-by: Lukasz Majewski lukma@denx.de --- arch/arm/mach-imx/spl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index 1f230aca33..a74d222dd6 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -158,7 +158,7 @@ u32 spl_boot_device(void) case SPI_NOR_BOOT: return BOOT_DEVICE_SPI; case USB_BOOT: - return BOOT_DEVICE_USB; + return BOOT_DEVICE_BOARD; default: return BOOT_DEVICE_NONE; }

If SDP_WRITE and SDP_JUMP command addr is zero, use CONFIG_SDP_LOADADDR as default address.
Signed-off-by: Sherry Sun sherry.sun@nxp.com Signed-off-by: Frank Li Frank.Li@nxp.com Reviewed-by: Lukasz Majewski lukma@denx.de --- drivers/usb/gadget/Kconfig | 4 ++++ drivers/usb/gadget/f_sdp.c | 6 ++++-- 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 26b4d12a09..172a82195b 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -115,6 +115,10 @@ config USB_GADGET_VBUS_DRAW This value will be used except for system-specific gadget drivers that have more specific information.
+config SDP_LOADADDR + hex "Default load address at SDP_WRITE and SDP_JUMP" + default 0 + # Selected by UDC drivers that support high-speed operation. config USB_GADGET_DUALSPEED bool diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c index bcd1c5d47c..841814bc07 100644 --- a/drivers/usb/gadget/f_sdp.c +++ b/drivers/usb/gadget/f_sdp.c @@ -276,7 +276,8 @@ static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req) sdp->error_status = SDP_WRITE_FILE_COMPLETE;
sdp->state = SDP_STATE_RX_FILE_DATA; - sdp->dnl_address = be32_to_cpu(cmd->addr); + sdp->dnl_address = cmd->addr ? be32_to_cpu(cmd->addr) : + CONFIG_SDP_LOADADDR; sdp->dnl_bytes_remaining = be32_to_cpu(cmd->cnt); sdp->dnl_bytes = sdp->dnl_bytes_remaining; sdp->next_state = SDP_STATE_IDLE; @@ -304,7 +305,8 @@ static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req) sdp->always_send_status = false; sdp->error_status = 0;
- sdp->jmp_address = be32_to_cpu(cmd->addr); + sdp->jmp_address = cmd->addr ? be32_to_cpu(cmd->addr) : + CONFIG_SDP_LOADADDR; sdp->state = SDP_STATE_TX_SEC_CONF; sdp->next_state = SDP_STATE_JUMP; break;

Because the buffer length of sdp usb request is 65, we have to allocate 65 bytes not 64 bytes. Otherwise there is potential buffer overflow.
So the wMaxPacketSize of fullspeed can't meet the needs. Add HS endpoint descriptor for SDP. Then we can use high speed endpoint, and the SDP device can send packet with 512 byte size.
Signed-off-by: Sherry Sun sherry.sun@nxp.com Signed-off-by: Ye Li ye.li@nxp.com --- drivers/usb/gadget/f_sdp.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c index 841814bc07..e7daa1a4f5 100644 --- a/drivers/usb/gadget/f_sdp.c +++ b/drivers/usb/gadget/f_sdp.c @@ -70,6 +70,11 @@ struct hid_report {
#define SDP_COMMAND_LEN 16
+/* The first data is for report id, the next 64 bytes are the maximum amount + * of data we need to send to usb host. So a total of 65 bytes are needed. + */ +#define SDP_USB_REQUEST_BUFLEN 65 + struct sdp_command { u16 cmd; u32 addr; @@ -158,6 +163,16 @@ static struct usb_endpoint_descriptor in_desc = { .bInterval = 1, };
+static struct usb_endpoint_descriptor in_hs_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 = 512, + .bInterval = 1, +}; + static struct usb_descriptor_header *sdp_runtime_descs[] = { (struct usb_descriptor_header *)&sdp_intf_runtime, (struct usb_descriptor_header *)&sdp_hid_desc, @@ -165,6 +180,13 @@ static struct usb_descriptor_header *sdp_runtime_descs[] = { NULL, };
+static struct usb_descriptor_header *sdp_runtime_hs_descs[] = { + (struct usb_descriptor_header *)&sdp_intf_runtime, + (struct usb_descriptor_header *)&sdp_hid_desc, + (struct usb_descriptor_header *)&in_hs_desc, + NULL, +}; + /* This is synchronized with what the SoC implementation reports */ static struct hid_report sdp_hid_report = { .usage_page = { @@ -490,6 +512,11 @@ static int sdp_bind(struct usb_configuration *c, struct usb_function *f) goto error; }
+ if (gadget_is_dualspeed(gadget)) { + /* Assume endpoint addresses are the same for both speeds */ + in_hs_desc.bEndpointAddress = in_desc.bEndpointAddress; + } + sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
cdev->req->context = sdp; @@ -527,7 +554,7 @@ static struct usb_request *sdp_start_ep(struct usb_ep *ep) { struct usb_request *req;
- req = alloc_ep_req(ep, 64); + req = alloc_ep_req(ep, SDP_USB_REQUEST_BUFLEN); debug("%s: ep:%p req:%p\n", __func__, ep, req);
if (!req) @@ -542,11 +569,15 @@ 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; + struct usb_gadget *gadget = cdev->gadget; int result;
debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
- result = usb_ep_enable(sdp->in_ep, &in_desc); + if (gadget_is_dualspeed(gadget) && gadget->speed == USB_SPEED_HIGH) + result = usb_ep_enable(sdp->in_ep, &in_hs_desc); + else + result = usb_ep_enable(sdp->in_ep, &in_desc); if (result) return result; sdp->in_req = sdp_start_ep(sdp->in_ep); @@ -592,7 +623,7 @@ static int sdp_bind_config(struct usb_configuration *c) 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.hs_descriptors = sdp_runtime_hs_descs; sdp_func->usb_function.descriptors = sdp_runtime_descs; sdp_func->usb_function.bind = sdp_bind; sdp_func->usb_function.unbind = sdp_unbind;

Hi Lukasz,
Ping. Any comments on V4?
Best regards Sherry Sun
Because the buffer length of sdp usb request is 65, we have to allocate 65 bytes not 64 bytes. Otherwise there is potential buffer overflow.
So the wMaxPacketSize of fullspeed can't meet the needs. Add HS endpoint descriptor for SDP. Then we can use high speed endpoint, and the SDP device can send packet with 512 byte size.
Signed-off-by: Sherry Sun sherry.sun@nxp.com Signed-off-by: Ye Li ye.li@nxp.com
drivers/usb/gadget/f_sdp.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c index 841814bc07..e7daa1a4f5 100644 --- a/drivers/usb/gadget/f_sdp.c +++ b/drivers/usb/gadget/f_sdp.c @@ -70,6 +70,11 @@ struct hid_report {
#define SDP_COMMAND_LEN 16
+/* The first data is for report id, the next 64 bytes are the maximum +amount
- of data we need to send to usb host. So a total of 65 bytes are needed.
- */
+#define SDP_USB_REQUEST_BUFLEN 65
struct sdp_command { u16 cmd; u32 addr; @@ -158,6 +163,16 @@ static struct usb_endpoint_descriptor in_desc = { .bInterval = 1, };
+static struct usb_endpoint_descriptor in_hs_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 = 512,
- .bInterval = 1,
+};
static struct usb_descriptor_header *sdp_runtime_descs[] = { (struct usb_descriptor_header *)&sdp_intf_runtime, (struct usb_descriptor_header *)&sdp_hid_desc, @@ -165,6 +180,13 @@ static struct usb_descriptor_header *sdp_runtime_descs[] = { NULL, };
+static struct usb_descriptor_header *sdp_runtime_hs_descs[] = {
- (struct usb_descriptor_header *)&sdp_intf_runtime,
- (struct usb_descriptor_header *)&sdp_hid_desc,
- (struct usb_descriptor_header *)&in_hs_desc,
- NULL,
+};
/* This is synchronized with what the SoC implementation reports */ static struct hid_report sdp_hid_report = { .usage_page = { @@ -490,6 +512,11 @@ static int sdp_bind(struct usb_configuration *c, struct usb_function *f) goto error; }
- if (gadget_is_dualspeed(gadget)) {
/* Assume endpoint addresses are the same for both speeds
*/
in_hs_desc.bEndpointAddress = in_desc.bEndpointAddress;
}
sdp->in_ep = ep; /* Store IN EP for enabling @ setup */
cdev->req->context = sdp;
@@ -527,7 +554,7 @@ static struct usb_request *sdp_start_ep(struct usb_ep *ep) { struct usb_request *req;
- req = alloc_ep_req(ep, 64);
req = alloc_ep_req(ep, SDP_USB_REQUEST_BUFLEN); debug("%s: ep:%p req:%p\n", __func__, ep, req);
if (!req)
@@ -542,11 +569,15 @@ 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;
struct usb_gadget *gadget = cdev->gadget; int result;
debug("%s: intf: %d alt: %d\n", __func__, intf, alt);
- result = usb_ep_enable(sdp->in_ep, &in_desc);
- if (gadget_is_dualspeed(gadget) && gadget->speed ==
USB_SPEED_HIGH)
result = usb_ep_enable(sdp->in_ep, &in_hs_desc);
- else
if (result) return result; sdp->in_req = sdp_start_ep(sdp->in_ep); @@ -592,7 +623,7 @@result = usb_ep_enable(sdp->in_ep, &in_desc);
static int sdp_bind_config(struct usb_configuration *c) 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.hs_descriptors = sdp_runtime_hs_descs; sdp_func->usb_function.descriptors = sdp_runtime_descs; sdp_func->usb_function.bind = sdp_bind; sdp_func->usb_function.unbind = sdp_unbind;
-- 2.17.1

Need initialize UDC before run sdp download and release it at the end of sdp.
Signed-off-by: Sherry Sun sherry.sun@nxp.com Signed-off-by: Frank Li Frank.Li@nxp.com Reviewed-by: Lukasz Majewski lukma@denx.de --- common/spl/spl_sdp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/common/spl/spl_sdp.c b/common/spl/spl_sdp.c index 806bf1327e..7b0a213d4c 100644 --- a/common/spl/spl_sdp.c +++ b/common/spl/spl_sdp.c @@ -16,6 +16,8 @@ static int spl_sdp_load_image(struct spl_image_info *spl_image, int ret; const int controller_index = 0;
+ usb_gadget_initialize(controller_index); + g_dnl_clear_detach(); ret = g_dnl_register("usb_dnl_sdp"); if (ret) { @@ -37,6 +39,8 @@ static int spl_sdp_load_image(struct spl_image_info *spl_image, ret = spl_sdp_handle(controller_index, spl_image); debug("SDP ended\n");
+ usb_gadget_release(controller_index); + return ret; } SPL_LOAD_IMAGE_METHOD("USB SDP", 0, BOOT_DEVICE_BOARD, spl_sdp_load_image);
participants (1)
-
Sherry Sun