Skip to contentSkip to conversation

usb: dwc3: gadget: Improve END_TRANSFER handling

PATCH In review Thinh Nguyen 2 participants 7 messages Dec 19, 2019 – Jan 15, 2020

Subsystems (2)
lore
Share & tools
lore

Thinh Nguyen

author maintainer
#0 lore raw

v2 cover usb: dwc3: gadget: Improve END_TRANSFER handling

This patch series improves the handling of the END_TRANSFER command:

1) Don't issue multiple END_TRANSFER commands while the previous hasn't completed
2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
3) Remove arbitrary 100us delay

Changes in v2:
- Remove unused variable

Thinh Nguyen (3):
usb: dwc3: gadget: Check END_TRANSFER completion
usb: dwc3: gadget: Delay starting transfer
usb: dwc3: gadget: Remove END_TRANSFER delay

 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/ep0.c    |  4 +++-
 drivers/usb/dwc3/gadget.c | 35 +++++++++++++++++++++--------------
 3 files changed, 26 insertions(+), 15 deletions(-)
Signature · 1 line
2.11.0
author maintainer
#1 lore raw

PATCH v2 1/3 usb: dwc3: gadget: Check END_TRANSFER completion

While the END_TRANSFER command is sent but not completed, any request dequeue during this time will cause the driver to issue the END_TRANSFER command. The driver needs to submit the command only once to stop the controller from processing further. The controller may take more time to process the same command multiple times unnecessarily. Let's add a flag DWC3_EP_END_TRANSFER_PENDING to check for this condition.

Fixes 3aec99154db3 ("usb: dwc3: gadget: remove DWC3_EP_END_TRANSFER_PENDING")

---
Changes in v2:
- None

 drivers/usb/dwc3/core.h   | 1 +
 drivers/usb/dwc3/ep0.c    | 4 +++-
 drivers/usb/dwc3/gadget.c | 6 +++++-
 3 files changed, 9 insertions(+), 2 deletions(-)
drivers/usb/dwc3/core.h +1 -0
index 1c8b349379af..da0af11fbc1a 100644
@@ -688,6 +688,7 @@ struct dwc3_ep {
 #define DWC3_EP_STALL		BIT(1)
 #define DWC3_EP_WEDGE		BIT(2)
 #define DWC3_EP_TRANSFER_STARTED BIT(3)
+#define DWC3_EP_END_TRANSFER_PENDING BIT(4)
 #define DWC3_EP_PENDING_REQUEST	BIT(5)
 
 	/* This last one is specific to EP0 */
drivers/usb/dwc3/ep0.c +3 -1
index fd1b100d2927..6dee4dabc0a4 100644
@@ -1136,8 +1136,10 @@ void dwc3_ep0_interrupt(struct dwc3 *dwc,
 	case DWC3_DEPEVT_EPCMDCMPLT:
 		cmd = DEPEVT_PARAMETER_CMD(event->parameters);
 
-		if (cmd == DWC3_DEPCMD_ENDTRANSFER)
+		if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
+			dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 			dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+		}
 		break;
 	}
 }
drivers/usb/dwc3/gadget.c +5 -1
index b3f8514d1f27..218022c261bc 100644
@@ -2621,6 +2621,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 		cmd = DEPEVT_PARAMETER_CMD(event->parameters);
 
 		if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
+			dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 			dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 			dwc3_gadget_ep_cleanup_cancelled_requests(dep);
 		}
@@ -2679,7 +2680,8 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	u32 cmd;
 	int ret;
 
-	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
+	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
+	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
 		return;
 
 	/*
@@ -2724,6 +2726,8 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 
 	if (!interrupt)
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+	else
+		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
 
 	if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
 		udelay(100);
Signature · 1 line
2.11.0
author maintainer
#2 lore raw

PATCH v2 2/3 usb: dwc3: gadget: Delay starting transfer

In reply to Thinh Nguyen

If the END_TRANSFER command hasn't completed yet, then don't send the START_TRANSFER command. The controller may not be able to start if that's the case. Some controller revisions depend on this. See commit 76a638f8ac0d ("usb: dwc3: gadget: wait for End Transfer to complete"). Let's only send START_TRANSFER command after the END_TRANSFER command had completed.

Fixes 3aec99154db3 ("usb: dwc3: gadget: remove DWC3_EP_END_TRANSFER_PENDING")

---
Changes in v2:
- None

 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/gadget.c | 11 +++++++++++
 2 files changed, 12 insertions(+)
drivers/usb/dwc3/core.h +1 -0
index da0af11fbc1a..77c4a9abe365 100644
@@ -690,6 +690,7 @@ struct dwc3_ep {
 #define DWC3_EP_TRANSFER_STARTED BIT(3)
 #define DWC3_EP_END_TRANSFER_PENDING BIT(4)
 #define DWC3_EP_PENDING_REQUEST	BIT(5)
+#define DWC3_EP_DELAY_START	BIT(6)
 
 	/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN		BIT(31)
drivers/usb/dwc3/gadget.c +11 -0
index 218022c261bc..61b7bef98cf9 100644
@@ -1450,6 +1450,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 	list_add_tail(&req->list, &dep->pending_list);
 	req->status = DWC3_REQUEST_STATUS_QUEUED;
 
+	/* Start the transfer only after the END_TRANSFER is completed */
+	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
+		dep->flags |= DWC3_EP_DELAY_START;
+		return 0;
+	}
+
 	/*
 	 * NOTICE: Isochronous endpoints should NEVER be prestarted. We must
 	 * wait for a XferNotReady event so we will know what's the current
@@ -2624,6 +2630,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 			dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 			dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 			dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+			if ((dep->flags & DWC3_EP_DELAY_START) &&
+			    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
+				__dwc3_gadget_kick_transfer(dep);
+
+			dep->flags &= ~DWC3_EP_DELAY_START;
 		}
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
Signature · 1 line
2.11.0
author maintainer
#3 lore raw

PATCH v2 3/3 usb: dwc3: gadget: Remove END_TRANSFER delay

In reply to Thinh Nguyen

We had a 100us delay to synchronize the END_TRANSFER command completion before giving back requests to the function drivers. Now, the controller driver can handle cancelled TRBs with the requests' cancelled_list and it can also wait until the END_TRANSFER completion before starting new transfers. Synchronization can simply base on the controller's command completion interrupt. The 100us delay is no longer needed. Remove this arbitrary delay.

---
Changes in v2:
- Remove unused variable

 drivers/usb/dwc3/gadget.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)
drivers/usb/dwc3/gadget.c +5 -13
index 61b7bef98cf9..05dbeae1308a 100644
@@ -2686,7 +2686,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	bool interrupt)
 {
-	struct dwc3 *dwc = dep->dwc;
 	struct dwc3_gadget_ep_cmd_params params;
 	u32 cmd;
 	int ret;
@@ -2702,16 +2701,13 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * much trouble synchronizing between us and gadget driver.
 	 *
 	 * We have discussed this with the IP Provider and it was
-	 * suggested to giveback all requests here, but give HW some
-	 * extra time to synchronize with the interconnect. We're using
-	 * an arbitrary 100us delay for that.
+	 * suggested to giveback all requests here.
 	 *
 	 * Note also that a similar handling was tested by Synopsys
 	 * (thanks a lot Paul) and nothing bad has come out of it.
-	 * In short, what we're doing is:
-	 *
-	 * - Issue EndTransfer WITH CMDIOC bit set
-	 * - Wait 100us
+	 * In short, what we're doing is issuing EndTransfer with
+	 * CMDIOC bit set and delay kicking transfer until the
+	 * EndTransfer command had completed.
 	 *
 	 * As of IP version 3.10a of the DWC_usb3 IP, the controller
 	 * supports a mode to work around the above limitation. The
@@ -2720,8 +2716,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * by writing GUCTL2[14]. This polling is already done in the
 	 * dwc3_send_gadget_ep_cmd() function so if the mode is
 	 * enabled, the EndTransfer command will have completed upon
-	 * returning from this function and we don't need to delay for
-	 * 100us.
+	 * returning from this function.
 	 *
 	 * This mode is NOT available on the DWC_usb31 IP.
 	 */
@@ -2739,9 +2734,6 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 	else
 		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
-
-	if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
-		udelay(100);
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
Signature · 1 line
2.11.0
#4 lore raw
In reply to Thinh Nguyen

Hi,

Quoted 9 lines · Thinh Nguyen

This patch series improves the handling of the END_TRANSFER command:

1) Don't issue multiple END_TRANSFER commands while the previous hasn't completed
2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
3) Remove arbitrary 100us delay

Changes in v2:
- Remove unused variable

Tried applying these, but it failed on patch 1.

Signature · 1 line
balbi
author maintainer
#5 lore raw

Hi Felipe,

Quoted 5 lines · Felipe Balbi

Hi,

Quoted 2 lines · Thinh Nguyen

This patch series improves the handling of the END_TRANSFER command:

Quoted 9 lines
Quoted 7 lines

1) Don't issue multiple END_TRANSFER commands while the previous hasn't completed
2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
3) Remove arbitrary 100us delay

Changes in v2:
- Remove unused variable

Tried applying these, but it failed on patch 1.

Your next branch doesn't have these prerequisite patches from mainline:
d3abda5a98a1 ("usb: dwc3: gadget: Clear started flag for non-IOC") 2d7b78f59e02 ("usb: dwc3: ep0: Clear started flag on completion")

Will you rebase on v5.5 rc-x? Let me know how we can proceed to resolve the conflict.

Also, have you a chance to take a look at these patches:
https://patchwork.kernel.org/project/linux-usb/list/?series=214669
* usb: dwc3: gadget: Properly set maxpacket limit
* usb: dwc3: Fix GTXFIFOSIZ.TXFDEP macro name

Thanks!
Thinh

#6 lore raw

Hi,

Quoted 17 lines · Thinh Nguyen
Quoted 12 lines
Quoted 9 lines · Thinh Nguyen

This patch series improves the handling of the END_TRANSFER command:

1) Don't issue multiple END_TRANSFER commands while the previous hasn't completed
2) Don't issue START_TRANSFER command while END_TRANSFER hasn't completed
3) Remove arbitrary 100us delay

Changes in v2:
- Remove unused variable

Tried applying these, but it failed on patch 1.

Your next branch doesn't have these prerequisite patches from mainline:
d3abda5a98a1 ("usb: dwc3: gadget: Clear started flag for non-IOC") 2d7b78f59e02 ("usb: dwc3: ep0: Clear started flag on completion")

Quoted 7 lines

Will you rebase on v5.5 rc-x? Let me know how we can proceed to resolve the conflict.

Also, have you a chance to take a look at these patches:
https://patchwork.kernel.org/project/linux-usb/list/?series=214669
* usb: dwc3: gadget: Properly set maxpacket limit
* usb: dwc3: Fix GTXFIFOSIZ.TXFDEP macro name

indeed, now they've applied cleanly. Thanks

Signature · 1 line
balbi