usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

PATCH Superseded Peng Fan 4 participants 12 messages Aug 27, 2015 – Sep 9, 2015 USB

lore

Share & tools

lore

Peng Fan

author

2015-08-27 11:00 UTC #0 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

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 <[email protected]>

Cc "?ukasz Majewski" <[email protected]>

Cc Marek Vasut <[email protected]>

---
 drivers/usb/gadget/ci_udc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

drivers/usb/gadget/ci_udc.c +28 -0

index 3e8eb87..f9374b3 100644
@@ -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)
 {

Signature · 1 line

1.8.4

Marek Vasut

2015-08-27 11:08 UTC #1 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Quoted 53 lines · Peng Fan

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 <[email protected]>

Cc "?ukasz Majewski" <[email protected]>

Cc Marek Vasut <[email protected]>

---
 drivers/usb/gadget/ci_udc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

drivers/usb/gadget/ci_udc.c +1 -0

index 3e8eb87..f9374b3 100644
@@ -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 = {

drivers/usb/gadget/ci_udc.c +13 -0

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

drivers/usb/gadget/ci_udc.c static void ci_ep_submit_next_request(struct ci_ep · line 551

Quoted 10 lines

drivers/usb/gadget/ci_udc.c +10 -0

+
+	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 <[email protected]>

Stephen Warren

2015-08-27 16:06 UTC #2 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Quoted 13 lines · Marek Vasut

Quoted 11 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +0 -0

Quoted 7 lines

drivers/usb/gadget/ci_udc.c static void ci_ep_submit_next_request(struct ci_ep *ci_ep) · line 549
Quoted 7 lines
drivers/usb/gadget/ci_udc.c +6 -0
+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?

Peng Fan

2015-08-28 00:05 UTC #3 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Hi Stephen,

Quoted 16 lines · Stephen Warren

Quoted 14 lines · Marek Vasut
Quoted 13 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +1 -0
 
+static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)

Quoted 7 lines

Quoted 5 lines
drivers/usb/gadget/ci_udc.c static void ci_ep_submit_next_request(struct ci_ep *ci_ep) · line 549
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+	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.

Peng Fan

2015-08-30 07:26 UTC #4 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Hi Stephen,

Quoted 26 lines · Peng Fan

Hi Stephen,
Quoted 16 lines · Stephen Warren
Quoted 14 lines · Marek Vasut
Quoted 13 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +1 -0
 
+static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
Quoted 7 lines
Quoted 5 lines
drivers/usb/gadget/ci_udc.c static void ci_ep_submit_next_request(struct ci_ep *ci_ep) · line 549
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+	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?

Quoted 3 lines

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.

Quoted 4 lines

Regards,
Peng.

Stephen Warren

2015-09-01 19:45 UTC #5 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Quoted 28 lines · Peng Fan

Hi Stephen,
Quoted 26 lines · Peng Fan
Hi Stephen,
Quoted 23 lines · Stephen Warren
Quoted 13 lines · Marek Vasut
Quoted 11 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +0 -0
Quoted 7 lines
drivers/usb/gadget/ci_udc.c static void ci_ep_submit_next_request(struct ci_ep *ci_ep) · line 549
Quoted 7 lines
drivers/usb/gadget/ci_udc.c +6 -0
+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?

Quoted 8 lines

Quoted 3 lines
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.

Marek Vasut

2015-09-03 22:11 UTC #6 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Quoted 2 lines · Stephen Warren

Quoted 1 line · Peng Fan
Hi Stephen,

Hi,

sorry for the delayed reply, I had to dig into the code myself.

Quoted 34 lines

Quoted 34 lines · Peng Fan
Quoted 32 lines
Hi Stephen,
Quoted 18 lines · Stephen Warren
Quoted 17 lines · Marek Vasut
Quoted 16 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +1 -0
 
+static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request
*_req)
Quoted 7 lines
Quoted 5 lines
drivers/usb/gadget/ci_udc.c static void ci_ep_submit_next_request(struct ci_ep *ci_ep) · line 549
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+	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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 2 lines

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.

Quoted 3 lines

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

Stephen Warren

2015-09-09 02:17 UTC #7 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Quoted 50 lines · Marek Vasut

Quoted 2 lines · Stephen Warren
Quoted 1 line · Peng Fan
Hi Stephen,
Hi,
sorry for the delayed reply, I had to dig into the code myself.
Quoted 34 lines
Quoted 34 lines · Peng Fan
Quoted 32 lines
Hi Stephen,
Quoted 18 lines · Stephen Warren
Quoted 17 lines · Marek Vasut
Quoted 16 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +1 -0
 
+static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request
*_req)
Quoted 7 lines
Quoted 5 lines
drivers/usb/gadget/ci_udc.c
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+	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.
Quoted 7 lines
Quoted 4 lines
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

Quoted 6 lines

Yes, that's correct.
Quoted 2 lines
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.

Quoted 5 lines

Quoted 3 lines
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?

Peng Fan

2015-09-09 03:24 UTC #8 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Quoted 60 lines · Stephen Warren

Quoted 50 lines · Marek Vasut
Quoted 2 lines · Stephen Warren
Quoted 1 line · Peng Fan
Hi Stephen,
Hi,
sorry for the delayed reply, I had to dig into the code myself.
Quoted 34 lines
Quoted 34 lines · Peng Fan
Quoted 32 lines
Hi Stephen,
Quoted 18 lines · Stephen Warren
Quoted 17 lines · Marek Vasut
Quoted 16 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +1 -0
 
+static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request
*_req)
Quoted 7 lines
Quoted 5 lines
drivers/usb/gadget/ci_udc.c
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+	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.
Quoted 7 lines
Quoted 4 lines
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
Quoted 6 lines
Yes, that's correct.
Quoted 2 lines
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.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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.

Quoted 1 line

Peng Fan

2015-09-09 04:30 UTC #9 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Hi Marek,

Quoted 65 lines · Peng Fan

Quoted 64 lines · Stephen Warren
Quoted 50 lines · Marek Vasut
Quoted 2 lines · Stephen Warren
Quoted 1 line · Peng Fan
Hi Stephen,
Hi,
sorry for the delayed reply, I had to dig into the code myself.
Quoted 34 lines
Quoted 34 lines · Peng Fan
Quoted 32 lines
Hi Stephen,
Quoted 18 lines · Stephen Warren
Quoted 17 lines · Marek Vasut
Quoted 16 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +1 -0
 
+static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request
*_req)
Quoted 7 lines
Quoted 5 lines
drivers/usb/gadget/ci_udc.c
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+	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.
Quoted 7 lines
Quoted 4 lines
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
Quoted 6 lines
Yes, that's correct.
Quoted 2 lines
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.
Quoted 4 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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.

Quoted 6 lines

Regards,
Peng.
Quoted 1 line

Marek Vasut

2015-09-09 23:20 UTC #10 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

Quoted 1 line · Peng Fan

Hi Marek,

Hi!

Quoted 68 lines · Peng Fan

Quoted 67 lines · Stephen Warren
Quoted 66 lines · Marek Vasut
Quoted 50 lines · Stephen Warren
Quoted 2 lines · Peng Fan
Quoted 1 line
Hi Stephen,
Hi,
sorry for the delayed reply, I had to dig into the code myself.
Quoted 34 lines
Quoted 34 lines · Peng Fan
Quoted 32 lines
Hi Stephen,
Quoted 18 lines · Stephen Warren
Quoted 17 lines · Marek Vasut
Quoted 16 lines · Peng Fan
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.
drivers/usb/gadget/ci_udc.c +1 -0
 
+static int ci_ep_dequeue(struct usb_ep *_ep, struct usb_request
*_req)
Quoted 7 lines
Quoted 5 lines
drivers/usb/gadget/ci_udc.c
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+	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.
Quoted 7 lines
Quoted 4 lines
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
Quoted 6 lines
Yes, that's correct.
Quoted 2 lines
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.
Quoted 5 lines
Quoted 3 lines
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 ?

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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!

Peng Fan

2015-08-28 00:02 UTC #11 lore raw

PATCH 1/1 usb: gadget: ci_udc: implement usb_ep_ops dequeue callback

In reply to Marek Vasut

Hi Marek,

Quoted 59 lines · Marek Vasut

Quoted 58 lines · Peng Fan

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 <[email protected]>

Cc "?ukasz Majewski" <[email protected]>

Cc Marek Vasut <[email protected]>

---
 drivers/usb/gadget/ci_udc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

drivers/usb/gadget/ci_udc.c +1 -0

index 3e8eb87..f9374b3 100644
@@ -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 = {

drivers/usb/gadget/ci_udc.c +18 -0

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

Quoted 8 lines

drivers/usb/gadget/ci_udc.c static void ci_ep_submit_next_request(struct ci_ep *ci_ep) · line 551
Quoted 5 lines
drivers/usb/gadget/ci_udc.c +5 -0
+		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.

Quoted 5 lines

Looks good otherwise:
Reviewed-by Marek Vasut <[email protected]>

Thanks,
Peng.