* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback @ 2015-08-27 11:00 Peng Fan 2015-08-27 11:08 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Peng Fan @ 2015-08-27 11:00 UTC (permalink / raw) To: u-boot 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->ops->dequeue" without implement ci_udc dequeue function, ep->ops->dequeue is NULL, then uboot will hang. Tested on mx6qsabresd board with fastboot enabled. Signed-off-by: Peng Fan <Peng.Fan@freescale.com> Cc: "?ukasz Majewski" <l.majewski@samsung.com> Cc: Marek Vasut <marex@denx.de> --- drivers/usb/gadget/ci_udc.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 3e8eb87..f9374b3 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -87,6 +87,7 @@ static int ci_ep_enable(struct usb_ep *ep, static int ci_ep_disable(struct usb_ep *ep); static int ci_ep_queue(struct usb_ep *ep, struct usb_request *req, gfp_t gfp_flags); +static int ci_ep_dequeue(struct usb_ep *ep, struct usb_request *req); static struct usb_request * ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags); static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *_req); @@ -99,6 +100,7 @@ static struct usb_ep_ops ci_ep_ops = { .enable = ci_ep_enable, .disable = ci_ep_disable, .queue = ci_ep_queue, + .dequeue = ci_ep_dequeue, .alloc_request = ci_ep_alloc_request, .free_request = ci_ep_free_request, }; @@ -525,6 +527,32 @@ static void ci_ep_submit_next_request(struct ci_ep *ci_ep) writel(bit, &udc->epprime); } +static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) +{ + struct ci_ep *ci_ep = container_of(_ep, struct ci_ep, ep); + struct ci_req *ci_req; + + list_for_each_entry(ci_req, &ci_ep->queue, queue) { + if (&ci_req->req == _req) + break; + } + + if (&ci_req->req != _req) + return -EINVAL; + + list_del_init(&ci_req->queue); + + if (ci_req->req.status == -EINPROGRESS) { + ci_req->req.status = -ECONNRESET; + if (ci_req->req.complete) + ci_req->req.complete(_ep, _req); + } + + debug("callback completed\n"); + + return 0; +} + static int ci_ep_queue(struct usb_ep *ep, struct usb_request *req, gfp_t gfp_flags) { -- 1.8.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-08-27 11:00 [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback Peng Fan @ 2015-08-27 11:08 ` Marek Vasut 2015-08-27 16:06 ` Stephen Warren 2015-08-28 0:02 ` Peng Fan 0 siblings, 2 replies; 12+ messages in thread From: Marek Vasut @ 2015-08-27 11:08 UTC (permalink / raw) To: u-boot 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->ops->dequeue > " without implement ci_udc dequeue function, ep->ops->dequeue is NULL, then > uboot will hang. > > Tested on mx6qsabresd board with fastboot enabled. > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > Cc: "?ukasz Majewski" <l.majewski@samsung.com> > Cc: Marek Vasut <marex@denx.de> > --- > drivers/usb/gadget/ci_udc.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c > index 3e8eb87..f9374b3 100644 > --- a/drivers/usb/gadget/ci_udc.c > +++ b/drivers/usb/gadget/ci_udc.c > @@ -87,6 +87,7 @@ static int ci_ep_enable(struct usb_ep *ep, > static int ci_ep_disable(struct usb_ep *ep); > static int ci_ep_queue(struct usb_ep *ep, > struct usb_request *req, gfp_t gfp_flags); > +static int ci_ep_dequeue(struct usb_ep *ep, struct usb_request *req); > static struct usb_request * > ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags); > static void ci_ep_free_request(struct usb_ep *ep, struct usb_request > *_req); @@ -99,6 +100,7 @@ static struct usb_ep_ops ci_ep_ops = { > .enable = ci_ep_enable, > .disable = ci_ep_disable, > .queue = ci_ep_queue, > + .dequeue = ci_ep_dequeue, > .alloc_request = ci_ep_alloc_request, > .free_request = ci_ep_free_request, > }; > @@ -525,6 +527,32 @@ static void ci_ep_submit_next_request(struct ci_ep > *ci_ep) writel(bit, &udc->epprime); > } > > +static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) > +{ > + struct ci_ep *ci_ep = container_of(_ep, struct ci_ep, ep); > + struct ci_req *ci_req; > + > + list_for_each_entry(ci_req, &ci_ep->queue, queue) { > + if (&ci_req->req == _req) > + break; > + } > + > + if (&ci_req->req != _req) > + return -EINVAL; > + > + list_del_init(&ci_req->queue); > + > + if (ci_req->req.status == -EINPROGRESS) { > + ci_req->req.status = -ECONNRESET; > + if (ci_req->req.complete) > + ci_req->req.complete(_ep, _req); > + } > + > + debug("callback completed\n"); Please fix this useless debug() statement, either remove it, or at least make it print __func__ so one can identify where this came from. Looks good otherwise: Reviewed-by: Marek Vasut <marex@denx.de> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-08-27 11:08 ` Marek Vasut @ 2015-08-27 16:06 ` Stephen Warren 2015-08-28 0:05 ` Peng Fan 2015-08-28 0:02 ` Peng Fan 1 sibling, 1 reply; 12+ messages in thread From: Stephen Warren @ 2015-08-27 16:06 UTC (permalink / raw) To: u-boot 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->ops->dequeue >> " 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-08-27 16:06 ` Stephen Warren @ 2015-08-28 0:05 ` Peng Fan 2015-08-30 7:26 ` Peng Fan 0 siblings, 1 reply; 12+ messages in thread From: Peng Fan @ 2015-08-28 0:05 UTC (permalink / raw) To: u-boot 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->ops->dequeue >>>" 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. Regards, Peng. -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-08-28 0:05 ` Peng Fan @ 2015-08-30 7:26 ` Peng Fan 2015-09-01 19:45 ` Stephen Warren 0 siblings, 1 reply; 12+ messages in thread From: Peng Fan @ 2015-08-30 7:26 UTC (permalink / raw) To: u-boot Hi Stephen, 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->ops->dequeue >>>>" 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 Regards, Peng. > >Regards, >Peng. >-- -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-08-30 7:26 ` Peng Fan @ 2015-09-01 19:45 ` Stephen Warren 2015-09-03 22:11 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Stephen Warren @ 2015-09-01 19:45 UTC (permalink / raw) To: u-boot On 08/30/2015 12:26 AM, Peng Fan wrote: > Hi Stephen, > 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->ops->dequeue >>>>> " 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, or is it supposed to actually stop the HW performing the transaction on the USB bus? 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-09-01 19:45 ` Stephen Warren @ 2015-09-03 22:11 ` Marek Vasut 2015-09-09 2:17 ` Stephen Warren 0 siblings, 1 reply; 12+ messages in thread From: Marek Vasut @ 2015-09-03 22:11 UTC (permalink / raw) To: u-boot 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->ops->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. > 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 ? Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-09-03 22:11 ` Marek Vasut @ 2015-09-09 2:17 ` Stephen Warren 2015-09-09 3:24 ` Peng Fan 0 siblings, 1 reply; 12+ messages in thread From: Stephen Warren @ 2015-09-09 2:17 UTC (permalink / raw) To: u-boot 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->ops->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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-09-09 2:17 ` Stephen Warren @ 2015-09-09 3:24 ` Peng Fan 2015-09-09 4:30 ` Peng Fan 0 siblings, 1 reply; 12+ messages in thread From: Peng Fan @ 2015-09-09 3:24 UTC (permalink / raw) To: u-boot 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->ops->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. Regards, Peng. > -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-09-09 3:24 ` Peng Fan @ 2015-09-09 4:30 ` Peng Fan 2015-09-09 23:20 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Peng Fan @ 2015-09-09 4:30 UTC (permalink / raw) To: u-boot Hi Marek, 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->ops->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/ Thanks, Peng. > >Regards, >Peng. >> > >-- -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-09-09 4:30 ` Peng Fan @ 2015-09-09 23:20 ` Marek Vasut 0 siblings, 0 replies; 12+ messages in thread From: Marek Vasut @ 2015-09-09 23:20 UTC (permalink / raw) To: u-boot 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! ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback 2015-08-27 11:08 ` Marek Vasut 2015-08-27 16:06 ` Stephen Warren @ 2015-08-28 0:02 ` Peng Fan 1 sibling, 0 replies; 12+ messages in thread From: Peng Fan @ 2015-08-28 0:02 UTC (permalink / raw) To: u-boot Hi Marek, On Thu, Aug 27, 2015 at 01:08:46PM +0200, 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->ops->dequeue >> " without implement ci_udc dequeue function, ep->ops->dequeue is NULL, then >> uboot will hang. >> >> Tested on mx6qsabresd board with fastboot enabled. >> >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> >> Cc: "?ukasz Majewski" <l.majewski@samsung.com> >> Cc: Marek Vasut <marex@denx.de> >> --- >> drivers/usb/gadget/ci_udc.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c >> index 3e8eb87..f9374b3 100644 >> --- a/drivers/usb/gadget/ci_udc.c >> +++ b/drivers/usb/gadget/ci_udc.c >> @@ -87,6 +87,7 @@ static int ci_ep_enable(struct usb_ep *ep, >> static int ci_ep_disable(struct usb_ep *ep); >> static int ci_ep_queue(struct usb_ep *ep, >> struct usb_request *req, gfp_t gfp_flags); >> +static int ci_ep_dequeue(struct usb_ep *ep, struct usb_request *req); >> static struct usb_request * >> ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags); >> static void ci_ep_free_request(struct usb_ep *ep, struct usb_request >> *_req); @@ -99,6 +100,7 @@ static struct usb_ep_ops ci_ep_ops = { >> .enable = ci_ep_enable, >> .disable = ci_ep_disable, >> .queue = ci_ep_queue, >> + .dequeue = ci_ep_dequeue, >> .alloc_request = ci_ep_alloc_request, >> .free_request = ci_ep_free_request, >> }; >> @@ -525,6 +527,32 @@ static void ci_ep_submit_next_request(struct ci_ep >> *ci_ep) writel(bit, &udc->epprime); >> } >> >> +static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) >> +{ >> + struct ci_ep *ci_ep = container_of(_ep, struct ci_ep, ep); >> + struct ci_req *ci_req; >> + >> + list_for_each_entry(ci_req, &ci_ep->queue, queue) { >> + if (&ci_req->req == _req) >> + break; >> + } >> + >> + if (&ci_req->req != _req) >> + return -EINVAL; >> + >> + list_del_init(&ci_req->queue); >> + >> + if (ci_req->req.status == -EINPROGRESS) { >> + ci_req->req.status = -ECONNRESET; >> + if (ci_req->req.complete) >> + ci_req->req.complete(_ep, _req); >> + } >> + >> + debug("callback completed\n"); > >Please fix this useless debug() statement, either remove it, or at least >make it print __func__ so one can identify where this came from. Will remove it and send out v2 with your Reviewed-by Tag. > > >Looks good otherwise: > >Reviewed-by: Marek Vasut <marex@denx.de> Thanks, Peng. -- ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-09-09 23:20 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-27 11:00 [U-Boot] [PATCH] usb: gadget: ci_udc: implement usb_ep_ops dequeue callback Peng Fan 2015-08-27 11:08 ` Marek Vasut 2015-08-27 16:06 ` Stephen Warren 2015-08-28 0:05 ` Peng Fan 2015-08-30 7:26 ` Peng Fan 2015-09-01 19:45 ` Stephen Warren 2015-09-03 22:11 ` Marek Vasut 2015-09-09 2:17 ` Stephen Warren 2015-09-09 3:24 ` Peng Fan 2015-09-09 4:30 ` Peng Fan 2015-09-09 23:20 ` Marek Vasut 2015-08-28 0:02 ` Peng Fan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox