Message ID | 1401396783-21873-4-git-send-email-swarren@wwwdotorg.org |
---|---|
State | Accepted |
Delegated to: | Marek Vasut |
Headers | show |
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
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.
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
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;