[v3,1/4] usb: dwc3: gadget: Refactor preparing TRBs

Commit Message

Thinh Nguyen Sept. 3, 2020, 2:06 a.m. UTC

There are a lot of common codes for preparing SG and linear TRBs. Let's
refactor them for easier read. No functional change here.

Signed-off-by: Thinh Nguyen <[e-mail]>
---
Changes in v3:
- None
Changes in v2:
- Remove unused variables rem and maxp

 drivers/usb/dwc3/gadget.c | 178 ++++++++++++++------------------------
 1 file changed, 67 insertions(+), 111 deletions(-)

Comments

Felipe Balbi Sept. 7, 2020, 6:24 a.m. UTC | #1

Thinh Nguyen <[e-mail]> writes:

> There are a lot of common codes for preparing SG and linear TRBs. Let's
> refactor them for easier read. No functional change here.
>
> Signed-off-by: Thinh Nguyen <[e-mail]>

email here doesn't match author's ;-)

> @@ -1091,6 +1091,65 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>  			stream_id, short_not_ok, no_interrupt, is_last);
>  }
>  
> +/**
> + * dwc3_prepare_last_sg - prepare TRBs for the last SG entry
> + * @dep: The endpoint that the request belongs to
> + * @req: The request to prepare
> + * @entry_length: The last SG entry size
> + * @node: Indicates whether this is not the first entry (for isoc only)
> + */
> +static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
> +				 struct dwc3_request *req,
> +				 unsigned int entry_length,
> +				 unsigned int node)

I think some of these can be combined into a single line. Also, why so
far to the right on the line breaks? Could you keep the existing style?

> +{
> +	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
> +	unsigned int rem = req->request.length % maxp;
> +	unsigned int num_extra_trbs = 0;
> +	unsigned int i;
> +	bool do_zlp = false;
> +
> +	if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> +	    req->request.zero && req->request.length && !rem) {

spaces for indentation?

Thinh Nguyen Sept. 8, 2020, 6:17 p.m. UTC | #2

Felipe Balbi wrote:
> Thinh Nguyen <[e-mail]> writes:
>
>> There are a lot of common codes for preparing SG and linear TRBs. Let's
>> refactor them for easier read. No functional change here.
>>
>> Signed-off-by: Thinh Nguyen <[e-mail]>
> email here doesn't match author's ;-)

Whenever I send email outside of our organization, the "From" header
will be modified to an alias email address. I'm working with our IT to
see if we can do something about it. This may take some time. I hope
that this doesn't cause too much issue accepting my patches in the
meanwhile.

>
>> @@ -1091,6 +1091,65 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>>  			stream_id, short_not_ok, no_interrupt, is_last);
>>  }
>>  
>> +/**
>> + * dwc3_prepare_last_sg - prepare TRBs for the last SG entry
>> + * @dep: The endpoint that the request belongs to
>> + * @req: The request to prepare
>> + * @entry_length: The last SG entry size
>> + * @node: Indicates whether this is not the first entry (for isoc only)
>> + */
>> +static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
>> +				 struct dwc3_request *req,
>> +				 unsigned int entry_length,
>> +				 unsigned int node)
> I think some of these can be combined into a single line. Also, why so
> far to the right on the line breaks? Could you keep the existing style?


It's because of open parenthesis descendant alignment style. I'll change
to not do that.

>> +{
>> +	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
>> +	unsigned int rem = req->request.length % maxp;
>> +	unsigned int num_extra_trbs = 0;
>> +	unsigned int i;
>> +	bool do_zlp = false;
>> +
>> +	if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
>> +	    req->request.zero && req->request.length && !rem) {
> spaces for indentation?
>

Same comment as above.

Thanks,
Thinh

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 13a187efb5b9..09810ca537bc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1091,6 +1091,65 @@  static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			stream_id, short_not_ok, no_interrupt, is_last);
 }
 
+/**
+ * dwc3_prepare_last_sg - prepare TRBs for the last SG entry
+ * @dep: The endpoint that the request belongs to
+ * @req: The request to prepare
+ * @entry_length: The last SG entry size
+ * @node: Indicates whether this is not the first entry (for isoc only)
+ */
+static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
+				 struct dwc3_request *req,
+				 unsigned int entry_length,
+				 unsigned int node)
+{
+	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
+	unsigned int rem = req->request.length % maxp;
+	unsigned int num_extra_trbs = 0;
+	unsigned int i;
+	bool do_zlp = false;
+
+	if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+	    req->request.zero && req->request.length && !rem) {
+		num_extra_trbs++;
+		do_zlp = true;
+	}
+
+	if (!req->direction && (!req->request.length || rem || do_zlp))
+		num_extra_trbs++;
+
+	if (num_extra_trbs > 0)
+		req->needs_extra_trb = true;
+
+	/* Prepare a normal TRB */
+	dwc3_prepare_one_trb(dep, req, entry_length, req->needs_extra_trb, node);
+
+	/* Prepare extra TRBs for ZLP and MPS OUT transfer alignment */
+	for (i = 0; i < num_extra_trbs; i++) {
+		struct dwc3 *dwc = dep->dwc;
+		struct dwc3_trb	*trb;
+		unsigned int extra_trb_length;
+		bool chain = true;
+
+		if (do_zlp && !i)
+			extra_trb_length = 0;
+		else
+			extra_trb_length = maxp - rem;
+
+		if (i == num_extra_trbs - 1)
+			chain = false;
+
+		trb = &dep->trb_pool[dep->trb_enqueue];
+		req->num_trbs++;
+		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
+				       extra_trb_length, chain, 1,
+				       req->request.stream_id,
+				       req->request.short_not_ok,
+				       req->request.no_interrupt,
+				       req->request.is_last);
+	}
+}
+
 static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
@@ -1109,10 +1168,8 @@  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		length -= sg_dma_len(s);
 
 	for_each_sg(sg, s, remaining, i) {
-		unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
-		unsigned int rem = length % maxp;
 		unsigned int trb_length;
-		unsigned int chain = true;
+		bool last_sg = false;
 
 		trb_length = min_t(unsigned int, length, sg_dma_len(s));
 
@@ -1126,60 +1183,12 @@  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		 * mapped sg.
 		 */
 		if ((i == remaining - 1) || !length)
-			chain = false;
+			last_sg = true;
 
-		if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {
-			struct dwc3	*dwc = dep->dwc;
-			struct dwc3_trb	*trb;
-
-			req->needs_extra_trb = true;
-
-			/* prepare normal TRB */
-			dwc3_prepare_one_trb(dep, req, trb_length, true, i);
-
-			/* Now prepare one extra TRB to align transfer size */
-			trb = &dep->trb_pool[dep->trb_enqueue];
-			req->num_trbs++;
-			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
-					maxp - rem, false, 1,
-					req->request.stream_id,
-					req->request.short_not_ok,
-					req->request.no_interrupt,
-					req->request.is_last);
-		} else if (req->request.zero && req->request.length &&
-			   !usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
-			   !rem && !chain) {
-			struct dwc3	*dwc = dep->dwc;
-			struct dwc3_trb	*trb;
-
-			req->needs_extra_trb = true;
-
-			/* Prepare normal TRB */
-			dwc3_prepare_one_trb(dep, req, trb_length, true, i);
-
-			/* Prepare one extra TRB to handle ZLP */
-			trb = &dep->trb_pool[dep->trb_enqueue];
-			req->num_trbs++;
-			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
-					       !req->direction, 1,
-					       req->request.stream_id,
-					       req->request.short_not_ok,
-					       req->request.no_interrupt,
-					       req->request.is_last);
-
-			/* Prepare one more TRB to handle MPS alignment */
-			if (!req->direction) {
-				trb = &dep->trb_pool[dep->trb_enqueue];
-				req->num_trbs++;
-				__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp,
-						       false, 1, req->request.stream_id,
-						       req->request.short_not_ok,
-						       req->request.no_interrupt,
-						       req->request.is_last);
-			}
-		} else {
-			dwc3_prepare_one_trb(dep, req, trb_length, chain, i);
-		}
+		if (last_sg)
+			dwc3_prepare_last_sg(dep, req, trb_length, i);
+		else
+			dwc3_prepare_one_trb(dep, req, trb_length, 1, i);
 
 		/*
 		 * There can be a situation where all sgs in sglist are not
@@ -1188,7 +1197,7 @@  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		 * we have free trbs we can continue queuing from where we
 		 * previously stopped
 		 */
-		if (chain)
+		if (!last_sg)
 			req->start_sg = sg_next(s);
 
 		req->num_queued_sgs++;
@@ -1211,60 +1220,7 @@  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
-	unsigned int length = req->request.length;
-	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
-	unsigned int rem = length % maxp;
-
-	if ((!length || rem) && usb_endpoint_dir_out(dep->endpoint.desc)) {
-		struct dwc3	*dwc = dep->dwc;
-		struct dwc3_trb	*trb;
-
-		req->needs_extra_trb = true;
-
-		/* prepare normal TRB */
-		dwc3_prepare_one_trb(dep, req, length, true, 0);
-
-		/* Now prepare one extra TRB to align transfer size */
-		trb = &dep->trb_pool[dep->trb_enqueue];
-		req->num_trbs++;
-		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
-				false, 1, req->request.stream_id,
-				req->request.short_not_ok,
-				req->request.no_interrupt,
-				req->request.is_last);
-	} else if (req->request.zero && req->request.length &&
-		   !usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
-		   (IS_ALIGNED(req->request.length, maxp))) {
-		struct dwc3	*dwc = dep->dwc;
-		struct dwc3_trb	*trb;
-
-		req->needs_extra_trb = true;
-
-		/* prepare normal TRB */
-		dwc3_prepare_one_trb(dep, req, length, true, 0);
-
-		/* Prepare one extra TRB to handle ZLP */
-		trb = &dep->trb_pool[dep->trb_enqueue];
-		req->num_trbs++;
-		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
-				!req->direction, 1, req->request.stream_id,
-				req->request.short_not_ok,
-				req->request.no_interrupt,
-				req->request.is_last);
-
-		/* Prepare one more TRB to handle MPS alignment for OUT */
-		if (!req->direction) {
-			trb = &dep->trb_pool[dep->trb_enqueue];
-			req->num_trbs++;
-			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp,
-					       false, 1, req->request.stream_id,
-					       req->request.short_not_ok,
-					       req->request.no_interrupt,
-					       req->request.is_last);
-		}
-	} else {
-		dwc3_prepare_one_trb(dep, req, length, false, 0);
-	}
+	dwc3_prepare_last_sg(dep, req, req->request.length, 0);
 }
 
 /*