[U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

[Marek Vasut]

On Wednesday, September 09, 2015 at 06:30:20 AM, Peng Fan wrote:
> Hi Marek,

Hi!

> On Wed, Sep 09, 2015 at 11:24:37AM +0800, Peng Fan wrote:
> >On Tue, Sep 08, 2015 at 07:17:22PM -0700, Stephen Warren wrote:
> >>On 09/03/2015 03:11 PM, Marek Vasut wrote:
> >>> On Tuesday, September 01, 2015 at 09:45:12 PM, Stephen Warren wrote:
> >>>> On 08/30/2015 12:26 AM, Peng Fan wrote:
> >>>>> Hi Stephen,
> >>> 
> >>> Hi,
> >>> 
> >>> sorry for the delayed reply, I had to dig into the code myself.
> >>> 
> >>>>> On Fri, Aug 28, 2015 at 08:05:36AM +0800, Peng Fan wrote:
> >>>>>> Hi Stephen,
> >>>>>> 
> >>>>>> On Thu, Aug 27, 2015 at 10:06:14AM -0600, Stephen Warren wrote:
> >>>>>>> On 08/27/2015 05:08 AM, Marek Vasut wrote:
> >>>>>>>> On Thursday, August 27, 2015 at 01:00:50 PM, Peng Fan wrote:
> >>>>>>>>> Implement endpoint dequeue callback function.
> >>>>>>>>> 
> >>>>>>>>> Without this function, uboot will hang when executing fastboot
> >>>>>>>>> comamnd. See following flow:
> >>>>>>>>> "fastboot_tx_write_str->fastboot_tx_write->usb_ep_dequeue->ep->op
> >>>>>>>>> s->d equeue " without implement ci_udc dequeue function,
> >>>>>>>>> ep->ops->dequeue is NULL, then uboot will hang.
> >>>>>>>>> 
> >>>>>>>>> Tested on mx6qsabresd board with fastboot enabled.
> >>>>>>>>> 
> >>>>>>>>> diff --git a/drivers/usb/gadget/ci_udc.c
> >>>>>>>>> b/drivers/usb/gadget/ci_udc.c
> >>>>>>>>> 
> >>>>>>>>> +static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request
> >>>>>>>>> *_req)
> >>>>>>>>> 
> >>>>>>>>> +	if (ci_req->req.status == -EINPROGRESS) {
> >>>>>>>>> +		ci_req->req.status = -ECONNRESET;
> >>>>>>>>> +		if (ci_req->req.complete)
> >>>>>>>>> +			ci_req->req.complete(_ep, _req);
> >>>>>>>>> +	}
> >>>>>>> 
> >>>>>>> Is there no need to reprogram the HW to abort the transfer?
> >>>>>> 
> >>>>>> I checked linux udc driver drivers/usb/gadget/udc/fsl_qe_udc.c
> >>>>>> qe_ep_dequeue->done->usb_gadget_giveback_request->"req->complete(ep,
> >>>>>> req)" I did not see code to reprogram the HW to abort the transfer.
> >>>>> 
> >>>>> Do you have further comments?
> >>>>> I checked other gadget drivers in drivers/usb/gadget/, I did not see
> >>>>> drivers that reprogram the HW to abort the transfer. For now, I do
> >>>>> not think out a scenario to reprogram the HW to abort the transfer
> >>>> 
> >>>> Marek, what are the semantics of this function? Is it supposed to
> >>>> simply update SW state to make U-Boot not care about the transaction
> >>> 
> >>> Yes, that's correct.
> >>> 
> >>>> or is it supposed to actually stop the HW performing the transaction
> >>>> on the USB bus?
> >>> 
> >>> No, it's not supposed to kill the transaction in hardware.
> >>
> >>OK, the patch seems fine then.
> >>
> >>>> If it's the former, then the patch is likely fine. If it's the latter,
> >>>> then I think the function does need actually need to do something to
> >>>> make the HW stop, or we can't implement this particular function.
> >>> 
> >>> Do we need this for the current release or is this for -next ?
> >>
> >>I assume that's a question for Peng?
> >
> >Since this is a bug fix, I think this patch should be in current release.
> >Without this patch, boards which want to enable fastboot using ci_udc
> >driver will hang.
> 
> If you have plan to pick the bug fix, please pick V2.
> https://patchwork.ozlabs.org/patch/511739/

Aye aye, sir! One applied patch, coming right up!