[U-Boot] [PATCH 1/4] usb: ci_udc: detect queued requests on ep0

[U-Boot] [PATCH 1/4] usb: ci_udc: detect queued requests on ep0

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The flipping of ep0 between IN and OUT relies on ci_ep_queue() consuming
the current IN/OUT setting immediately. If this is deferred to a later
point when the req is pulled out of ci_req->queue, then the IN/OUT
setting may have been changed since the req was queued, and state will
get out of sync. This condition doesn't occur today, but could if bugs
were introduced later, and this error-check will save a lot of debugging
time.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/ci_udc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 9cd003636a44..a68a85f84e70 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -397,6 +397,21 @@ static int ci_ep_queue(struct usb_ep *ep,
 	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
 
+	if (!num && ci_ep->req_primed) {
+		/*
+		 * The flipping of ep0 between IN and OUT relies on
+		 * ci_ep_queue consuming the current IN/OUT setting
+		 * immediately. If this is deferred to a later point when the
+		 * req is pulled out of ci_req->queue, then the IN/OUT setting
+		 * may have been changed since the req was queued, and state
+		 * will get out of sync. This condition doesn't occur today,
+		 * but could if bugs were introduced later, and this error
+		 * check will save a lot of debugging time.
+		 */
+		printf("%s: ep0 transaction already in progress\n", __func__);
+		return -EPROTO;
+	}
+
 	ret = ci_bounce(ci_req, in);
 	if (ret)
 		return ret;
-- 
1.8.1.5

[U-Boot] [PATCH 2/4] usb: ci_udc: use a single descriptor for ep0

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

ci_udc currently points ep->desc at separate descriptors for IN and OUT.
These descriptors only differ in the ep address IN/OUT field. Modify the
code to use a single descriptor, and change that descriptor's ep address
to indicate IN/OUT as required. This removes some data duplication.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/ci_udc.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index a68a85f84e70..77f8c9ef7f0f 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -56,14 +56,7 @@ static const char *reqname(unsigned r)
 }
 #endif
 
-static struct usb_endpoint_descriptor ep0_out_desc = {
-	.bLength = sizeof(struct usb_endpoint_descriptor),
-	.bDescriptorType = USB_DT_ENDPOINT,
-	.bEndpointAddress = 0,
-	.bmAttributes =	USB_ENDPOINT_XFER_CONTROL,
-};
-
-static struct usb_endpoint_descriptor ep0_in_desc = {
+static struct usb_endpoint_descriptor ep0_desc = {
 	.bLength = sizeof(struct usb_endpoint_descriptor),
 	.bDescriptorType = USB_DT_ENDPOINT,
 	.bEndpointAddress = USB_DIR_IN,
@@ -435,7 +428,7 @@ static void handle_ep_complete(struct ci_ep *ep)
 	num = ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	in = (ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
 	if (num == 0)
-		ep->desc = &ep0_out_desc;
+		ep0_desc.bEndpointAddress = 0;
 	item = ci_get_qtd(num, in);
 	ci_invalidate_qtd(num);
 
@@ -460,7 +453,7 @@ static void handle_ep_complete(struct ci_ep *ep)
 	if (num == 0) {
 		ci_req->req.length = 0;
 		usb_ep_queue(&ep->ep, &ci_req->req, 0);
-		ep->desc = &ep0_in_desc;
+		ep0_desc.bEndpointAddress = USB_DIR_IN;
 	}
 }
 
@@ -771,7 +764,7 @@ static int ci_udc_probe(void)
 
 	/* Init EP 0 */
 	memcpy(&controller.ep[0].ep, &ci_ep_init[0], sizeof(*ci_ep_init));
-	controller.ep[0].desc = &ep0_in_desc;
+	controller.ep[0].desc = &ep0_desc;
 	INIT_LIST_HEAD(&controller.ep[0].queue);
 	controller.ep[0].req_primed = false;
 	controller.gadget.ep0 = &controller.ep[0].ep;
-- 
1.8.1.5

[U-Boot] [PATCH 3/4] usb: ci_udc: pre-allocate ep0 req

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Allocate ep0's USB request object when the UDC driver is probed. This
solves a couple of issues in the current code:

a) A request object always exists for ep0. Prior to this patch, if setup
transactions arrived in an unexpected order, handle_setup() would need
to reply to a setup transaction before any ep0 usb_req was created.

This issue was introduced in commit 2813006fecda "usb: ci_udc: allow
multiple buffer allocs per ep."

b) handle_ep_complete no longer /has/ to queue the ep0 request again
after every single request completion. This is currently required, since
handle_setup() assumes it can find some request object in ep0's request
queue. This patch doesn't actually stop handle_ep_complete() from always
requeueing the request, but the next patch will.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/ci_udc.c | 17 ++++++++++++++++-
 drivers/usb/gadget/ci_udc.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 77f8c9ef7f0f..03ad23164fe1 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -198,8 +198,14 @@ static void ci_invalidate_qtd(int ep_num)
 static struct usb_request *
 ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags)
 {
+	struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
+	int num;
 	struct ci_req *ci_req;
 
+	num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
+	if (num == 0 && controller.ep0_req)
+		return &controller.ep0_req->req;
+
 	ci_req = memalign(ARCH_DMA_MINALIGN, sizeof(*ci_req));
 	if (!ci_req)
 		return NULL;
@@ -207,6 +213,9 @@ ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags)
 	INIT_LIST_HEAD(&ci_req->queue);
 	ci_req->b_buf = 0;
 
+	if (num == 0)
+		controller.ep0_req = ci_req;
+
 	return &ci_req->req;
 }
 
@@ -471,7 +480,7 @@ static void handle_setup(void)
 	int num, in, _num, _in, i;
 	char *buf;
 
-	ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue);
+	ci_req = controller.ep0_req;
 	req = &ci_req->req;
 	head = ci_get_qh(0, 0);	/* EP0 OUT */
 
@@ -780,6 +789,12 @@ static int ci_udc_probe(void)
 			      &controller.gadget.ep_list);
 	}
 
+	ci_ep_alloc_request(&controller.ep[0].ep, 0);
+	if (!controller.ep0_req) {
+		free(controller.epts);
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h
index 23cff56d7ec9..7d76af84f037 100644
--- a/drivers/usb/gadget/ci_udc.h
+++ b/drivers/usb/gadget/ci_udc.h
@@ -97,6 +97,7 @@ struct ci_ep {
 
 struct ci_drv {
 	struct usb_gadget		gadget;
+	struct ci_req			*ep0_req;
 	struct usb_gadget_driver	*driver;
 	struct ehci_ctrl		*ctrl;
 	struct ept_queue_head		*epts;
-- 
1.8.1.5

[U-Boot] [PATCH 4/4] usb: ci_udc: complete ep0 direction handling

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

handle_setup() currently assumes that the response to a Setup transaction
will be an OUT transaction, and any subsequent packet (if any) will be an
IN transaction. This appears to be valid in many cases; both USB
enumeration and Mass Storage work OK with this restriction. However, DFU
uses ep0 to transfer data in both directions. This renders the assumption
invalid; when sending data from device to host, the Data Stage is an IN
transaction, and the Status Stage is an OUT transaction. Enhance
handle_setup() to deduce the correct direction for the USB transactions
based on Setup transaction data.

ep0's request object only needs to be automatically re-queued when the
Data Stage completes, in order to implement the Status Stage. Once the
Status Stage transaction is complete, there is no need to re-queue the
USB request, so don't do that.

Don't sent USB request completion callbacks for Status Stage transactions.
These were queued by ci_udc itself, and only serve to confuse the USB
function code. For example, f_dfu attempts to interpret the 0-length data
buffers for Status Stage transactions as DFU packets. These buffers
contain stale data from the previous transaction. This causes f_dfu to
complain about a sequence number mismatch.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/ci_udc.c | 48 ++++++++++++++++++++++++++++++++++++++-------
 drivers/usb/gadget/ci_udc.h |  1 +
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 03ad23164fe1..6dc20c6c954c 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -428,6 +428,17 @@ static int ci_ep_queue(struct usb_ep *ep,
 	return 0;
 }
 
+static void flip_ep0_direction(void)
+{
+	if (ep0_desc.bEndpointAddress == USB_DIR_IN) {
+		DBG("%s: Flipping ep0 ot OUT\n", __func__);
+		ep0_desc.bEndpointAddress = 0;
+	} else {
+		DBG("%s: Flipping ep0 ot IN\n", __func__);
+		ep0_desc.bEndpointAddress = USB_DIR_IN;
+	}
+}
+
 static void handle_ep_complete(struct ci_ep *ep)
 {
 	struct ept_queue_item *item;
@@ -436,8 +447,6 @@ static void handle_ep_complete(struct ci_ep *ep)
 
 	num = ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	in = (ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
-	if (num == 0)
-		ep0_desc.bEndpointAddress = 0;
 	item = ci_get_qtd(num, in);
 	ci_invalidate_qtd(num);
 
@@ -458,11 +467,18 @@ static void handle_ep_complete(struct ci_ep *ep)
 
 	DBG("ept%d %s req %p, complete %x\n",
 	    num, in ? "in" : "out", ci_req, len);
-	ci_req->req.complete(&ep->ep, &ci_req->req);
-	if (num == 0) {
+	if (num != 0 || controller.ep0_data_phase)
+		ci_req->req.complete(&ep->ep, &ci_req->req);
+	if (num == 0 && controller.ep0_data_phase) {
+		/*
+		 * Data Stage is complete, so flip ep0 dir for Status Stage,
+		 * which always transfers a packet in the opposite direction.
+		 */
+		DBG("%s: flip ep0 dir for Status Stage\n", __func__);
+		flip_ep0_direction();
+		controller.ep0_data_phase = false;
 		ci_req->req.length = 0;
 		usb_ep_queue(&ep->ep, &ci_req->req, 0);
-		ep0_desc.bEndpointAddress = USB_DIR_IN;
 	}
 }
 
@@ -491,8 +507,26 @@ static void handle_setup(void)
 #else
 	writel(EPT_RX(0), &udc->epstat);
 #endif
-	DBG("handle setup %s, %x, %x index %x value %x\n", reqname(r.bRequest),
-	    r.bRequestType, r.bRequest, r.wIndex, r.wValue);
+	DBG("handle setup %s, %x, %x index %x value %x length %x\n",
+	    reqname(r.bRequest), r.bRequestType, r.bRequest, r.wIndex,
+	    r.wValue, r.wLength);
+
+	/* Set EP0 dir for Data Stage based on Setup Stage data */
+	if (r.bRequestType & USB_DIR_IN) {
+		DBG("%s: Set ep0 to IN for Data Stage\n", __func__);
+		ep0_desc.bEndpointAddress = USB_DIR_IN;
+	} else {
+		DBG("%s: Set ep0 to OUT for Data Stage\n", __func__);
+		ep0_desc.bEndpointAddress = 0;
+	}
+	if (r.wLength) {
+		controller.ep0_data_phase = true;
+	} else {
+		/* 0 length -> no Data Stage. Flip dir for Status Stage */
+		DBG("%s: 0 length: flip ep0 dir for Status Stage\n", __func__);
+		flip_ep0_direction();
+		controller.ep0_data_phase = false;
+	}
 
 	list_del_init(&ci_req->queue);
 	ci_ep->req_primed = false;
diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h
index 7d76af84f037..c2144021e675 100644
--- a/drivers/usb/gadget/ci_udc.h
+++ b/drivers/usb/gadget/ci_udc.h
@@ -98,6 +98,7 @@ struct ci_ep {
 struct ci_drv {
 	struct usb_gadget		gadget;
 	struct ci_req			*ep0_req;
+	bool				ep0_data_phase;
 	struct usb_gadget_driver	*driver;
 	struct ehci_ctrl		*ctrl;
 	struct ept_queue_head		*epts;
-- 
1.8.1.5

[U-Boot] [PATCH 4/4] usb: ci_udc: complete ep0 direction handling

[Marek Vasut]

On Thursday, May 29, 2014 at 10:53:03 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> handle_setup() currently assumes that the response to a Setup transaction
> will be an OUT transaction, and any subsequent packet (if any) will be an
> IN transaction. This appears to be valid in many cases; both USB
> enumeration and Mass Storage work OK with this restriction. However, DFU
> uses ep0 to transfer data in both directions. This renders the assumption
> invalid; when sending data from device to host, the Data Stage is an IN
> transaction, and the Status Stage is an OUT transaction. Enhance
> handle_setup() to deduce the correct direction for the USB transactions
> based on Setup transaction data.
> 
> ep0's request object only needs to be automatically re-queued when the
> Data Stage completes, in order to implement the Status Stage. Once the
> Status Stage transaction is complete, there is no need to re-queue the
> USB request, so don't do that.
> 
> Don't sent USB request completion callbacks for Status Stage transactions.
> These were queued by ci_udc itself, and only serve to confuse the USB
> function code. For example, f_dfu attempts to interpret the 0-length data
> buffers for Status Stage transactions as DFU packets. These buffers
> contain stale data from the previous transaction. This causes f_dfu to
> complain about a sequence number mismatch.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/usb/gadget/ci_udc.c | 48
> ++++++++++++++++++++++++++++++++++++++------- drivers/usb/gadget/ci_udc.h
> |  1 +
>  2 files changed, 42 insertions(+), 7 deletions(-)

Applied all, thanks.

btw. we should weed out those DBG() macros and replace them with regular debug() 
or debug_cond().

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] usb: ci_udc: complete ep0 direction handling

[Stephen Warren]

On 06/01/2014 11:22 AM, Marek Vasut wrote:
> On Thursday, May 29, 2014 at 10:53:03 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> handle_setup() currently assumes that the response to a Setup transaction
>> will be an OUT transaction, and any subsequent packet (if any) will be an
>> IN transaction. This appears to be valid in many cases; both USB
...
> Applied all, thanks.

Thanks. Are these headed for v2014.07? I don't see these in the main
U-Boot repo yet.

[U-Boot] [PATCH 4/4] usb: ci_udc: complete ep0 direction handling

[Marek Vasut]

On Tuesday, June 10, 2014 at 05:58:25 PM, Stephen Warren wrote:
> On 06/01/2014 11:22 AM, Marek Vasut wrote:
> > On Thursday, May 29, 2014 at 10:53:03 PM, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >> 
> >> handle_setup() currently assumes that the response to a Setup
> >> transaction will be an OUT transaction, and any subsequent packet (if
> >> any) will be an IN transaction. This appears to be valid in many cases;
> >> both USB
> 
> ...
> 
> > Applied all, thanks.
> 
> Thanks. Are these headed for v2014.07? I don't see these in the main
> U-Boot repo yet.

Most likely, I am waiting for Tom to pick my PR up.

Best regards,
Marek Vasut