All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v3 0/4] Make some changes to SDP
@ 2019-08-22  1:46 Sherry Sun
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 1/4] imx: spl: Change USB boot device type for imx8/8m Sherry Sun
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Sherry Sun @ 2019-08-22  1:46 UTC (permalink / raw)
  To: u-boot

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 | 39 +++++++++++++++++++++++++++++++++-----
 4 files changed, 43 insertions(+), 6 deletions(-)

-- 
2.17.1

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

* [U-Boot] [PATCH v3 1/4] imx: spl: Change USB boot device type for imx8/8m
  2019-08-22  1:46 [U-Boot] [PATCH v3 0/4] Make some changes to SDP Sherry Sun
@ 2019-08-22  1:46 ` Sherry Sun
  2019-08-26  7:58   ` Lukasz Majewski
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 2/4] SDP: use CONFIG_SDP_LOADADDR as default load address Sherry Sun
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Sherry Sun @ 2019-08-22  1:46 UTC (permalink / raw)
  To: u-boot

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>
---
 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;
 	}
-- 
2.17.1

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

* [U-Boot] [PATCH v3 2/4] SDP: use CONFIG_SDP_LOADADDR as default load address
  2019-08-22  1:46 [U-Boot] [PATCH v3 0/4] Make some changes to SDP Sherry Sun
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 1/4] imx: spl: Change USB boot device type for imx8/8m Sherry Sun
@ 2019-08-22  1:46 ` Sherry Sun
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor Sherry Sun
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 4/4] SDP: Call usb_gadget_initialize and usb_gadget_release to support UDC Sherry Sun
  3 siblings, 0 replies; 8+ messages in thread
From: Sherry Sun @ 2019-08-22  1:46 UTC (permalink / raw)
  To: u-boot

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

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

* [U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor
  2019-08-22  1:46 [U-Boot] [PATCH v3 0/4] Make some changes to SDP Sherry Sun
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 1/4] imx: spl: Change USB boot device type for imx8/8m Sherry Sun
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 2/4] SDP: use CONFIG_SDP_LOADADDR as default load address Sherry Sun
@ 2019-08-22  1:46 ` Sherry Sun
  2019-08-26  8:01   ` Lukasz Majewski
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 4/4] SDP: Call usb_gadget_initialize and usb_gadget_release to support UDC Sherry Sun
  3 siblings, 1 reply; 8+ messages in thread
From: Sherry Sun @ 2019-08-22  1:46 UTC (permalink / raw)
  To: u-boot

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 | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
index 841814bc07..8aaed92e9b 100644
--- a/drivers/usb/gadget/f_sdp.c
+++ b/drivers/usb/gadget/f_sdp.c
@@ -158,6 +158,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 +175,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 +507,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 +549,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, 65);
 	debug("%s: ep:%p req:%p\n", __func__, ep, req);
 
 	if (!req)
@@ -542,11 +564,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 +618,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;
@@ -725,6 +751,7 @@ static void sdp_handle_in_ep(struct spl_image_info *spl_image)
 			/* In SPL, allow jumps to U-Boot images */
 			struct spl_image_info spl_image = {};
 			spl_parse_image_header(&spl_image, header);
+
 			jump_to_image_no_args(&spl_image);
 #else
 			/* In U-Boot, allow jumps to scripts */
-- 
2.17.1

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

* [U-Boot] [PATCH v3 4/4] SDP: Call usb_gadget_initialize and usb_gadget_release to support UDC
  2019-08-22  1:46 [U-Boot] [PATCH v3 0/4] Make some changes to SDP Sherry Sun
                   ` (2 preceding siblings ...)
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor Sherry Sun
@ 2019-08-22  1:46 ` Sherry Sun
  3 siblings, 0 replies; 8+ messages in thread
From: Sherry Sun @ 2019-08-22  1:46 UTC (permalink / raw)
  To: u-boot

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);
-- 
2.17.1

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

* [U-Boot] [PATCH v3 1/4] imx: spl: Change USB boot device type for imx8/8m
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 1/4] imx: spl: Change USB boot device type for imx8/8m Sherry Sun
@ 2019-08-26  7:58   ` Lukasz Majewski
  0 siblings, 0 replies; 8+ messages in thread
From: Lukasz Majewski @ 2019-08-26  7:58 UTC (permalink / raw)
  To: u-boot

Hi Sherry,

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

Reviewed-by: Lukasz 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-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190826/01baef7d/attachment.sig>

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

* [U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor
  2019-08-22  1:46 ` [U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor Sherry Sun
@ 2019-08-26  8:01   ` Lukasz Majewski
  2019-08-26  8:52     ` Sherry Sun
  0 siblings, 1 reply; 8+ messages in thread
From: Lukasz Majewski @ 2019-08-26  8:01 UTC (permalink / raw)
  To: u-boot

On Thu, 22 Aug 2019 01:46:20 +0000
Sherry Sun <sherry.sun@nxp.com> wrote:

> 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 | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> index 841814bc07..8aaed92e9b 100644
> --- a/drivers/usb/gadget/f_sdp.c
> +++ b/drivers/usb/gadget/f_sdp.c
> @@ -158,6 +158,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 +175,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 +507,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 +549,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, 65);

Maybe it would be good to have the #define for this magic number (65)?

If normally we have 64, then it is pretty clear. With 65 we may need
#define with some extra comment in the code.

>  	debug("%s: ep:%p req:%p\n", __func__, ep, req);
>  
>  	if (!req)
> @@ -542,11 +564,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 +618,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;
> @@ -725,6 +751,7 @@ static void sdp_handle_in_ep(struct
> spl_image_info *spl_image) /* In SPL, allow jumps to U-Boot images */
>  			struct spl_image_info spl_image = {};
>  			spl_parse_image_header(&spl_image, header);
> +

Minor:

Please remove this white space.

>  			jump_to_image_no_args(&spl_image);
>  #else
>  			/* In U-Boot, allow jumps to scripts */



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-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190826/e3787b22/attachment.sig>

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

* [U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor
  2019-08-26  8:01   ` Lukasz Majewski
@ 2019-08-26  8:52     ` Sherry Sun
  0 siblings, 0 replies; 8+ messages in thread
From: Sherry Sun @ 2019-08-26  8:52 UTC (permalink / raw)
  To: u-boot

Hi Lukasz,

> 
> On Thu, 22 Aug 2019 01:46:20 +0000
> Sherry Sun <sherry.sun@nxp.com> wrote:
> 
> > 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 | 33 ++++++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c
> > index 841814bc07..8aaed92e9b 100644
> > --- a/drivers/usb/gadget/f_sdp.c
> > +++ b/drivers/usb/gadget/f_sdp.c
> > @@ -158,6 +158,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 +175,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 +507,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 +549,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, 65);
> 
> Maybe it would be good to have the #define for this magic number (65)?
> 
> If normally we have 64, then it is pretty clear. With 65 we may need #define
> with some extra comment in the code.

Okay, I will add this.

> 
> >  	debug("%s: ep:%p req:%p\n", __func__, ep, req);
> >
> >  	if (!req)
> > @@ -542,11 +564,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 +618,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; @@ -725,6 +751,7
> @@
> > static void sdp_handle_in_ep(struct spl_image_info *spl_image) /* In
> > SPL, allow jumps to U-Boot images */
> >  			struct spl_image_info spl_image = {};
> >  			spl_parse_image_header(&spl_image, header);
> > +
> 
> Minor:
> 
> Please remove this white space.

Thanks for your reminder, I will do it.

Best regards
Sherry sun

> 
> >  			jump_to_image_no_args(&spl_image);
> >  #else
> >  			/* In U-Boot, allow jumps to scripts */
> 
> 
> 
> 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-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de

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

end of thread, other threads:[~2019-08-26  8:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-22  1:46 [U-Boot] [PATCH v3 0/4] Make some changes to SDP Sherry Sun
2019-08-22  1:46 ` [U-Boot] [PATCH v3 1/4] imx: spl: Change USB boot device type for imx8/8m Sherry Sun
2019-08-26  7:58   ` Lukasz Majewski
2019-08-22  1:46 ` [U-Boot] [PATCH v3 2/4] SDP: use CONFIG_SDP_LOADADDR as default load address Sherry Sun
2019-08-22  1:46 ` [U-Boot] [PATCH v3 3/4] SDP: fix wrong usb request size and add high speed endpoint descriptor Sherry Sun
2019-08-26  8:01   ` Lukasz Majewski
2019-08-26  8:52     ` Sherry Sun
2019-08-22  1:46 ` [U-Boot] [PATCH v3 4/4] SDP: Call usb_gadget_initialize and usb_gadget_release to support UDC Sherry Sun

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