[PATCH 0/8] usb: cdns3: improve the sg use case

[PATCH 0/8] usb: cdns3: improve the sg use case

[Peter Chen]

For sg use cases, there will be serveral TRBs in TD, and the short transfer
may occur during the TD, current code doesn't handle it well, improve it
through this series. Tested by Android MTP and ADB use case.

Peter Chen (8):
  usb: cdns3: gadget: using correct sg operations
  usb: cdns3: gadget: improve the dump TRB operation at
    cdns3_ep_run_transfer
  usb: cdns3: gadget: calculate TDL correctly
  usb: cdns3: gadget: add CHAIN and ISP bit for sg list use case
  usb: cdns3: gadget: handle sg list use case at completion correctly
  usb: cdns3: gadget: need to handle sg case for WA2 case
  usb: cdns3: gadget: sg_support is only for DEV_VER_V2 or above
  usb: cdns3: gadget: enlarge the TRB ring length

 drivers/usb/cdns3/gadget.c | 205 +++++++++++++++++++++++++------------
 drivers/usb/cdns3/gadget.h |  11 +-
 2 files changed, 151 insertions(+), 65 deletions(-)

-- 
2.17.1

[PATCH 1/8] usb: cdns3: gadget: using correct sg operations

[Peter Chen]

It needs to use request->num_mapped_sgs to indicate mapped sg number,
the request->num_sgs is the sg number before the mapping. These two
entries have different values for the platforms which iommu or
swiotlb is used. Besides, it needs to use correct sg APIs for
mapped sg list for TRB assignment.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 2ea4d30e1828..50282cab5fb6 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1098,11 +1098,13 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 	u32 control;
 	int pcs;
 	u16 total_tdl = 0;
+	struct scatterlist *s = NULL;
+	bool sg_supported = !!(request->num_mapped_sgs);
 
 	if (priv_ep->type == USB_ENDPOINT_XFER_ISOC)
 		num_trb = priv_ep->interval;
 	else
-		num_trb = request->num_sgs ? request->num_sgs : 1;
+		num_trb = sg_supported ? request->num_mapped_sgs : 1;
 
 	if (num_trb > priv_ep->free_trbs) {
 		priv_ep->flags |= EP_RING_FULL;
@@ -1162,22 +1164,24 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 	if (priv_dev->dev_ver <= DEV_VER_V2)
 		togle_pcs = cdns3_wa1_update_guard(priv_ep, trb);
 
+	if (sg_supported)
+		s = request->sg;
+
 	/* set incorrect Cycle Bit for first trb*/
 	control = priv_ep->pcs ? 0 : TRB_CYCLE;
-
 	do {
 		u32 length;
 		u16 td_size = 0;
 
 		/* fill TRB */
 		control |= TRB_TYPE(TRB_NORMAL);
-		trb->buffer = cpu_to_le32(TRB_BUFFER(request->num_sgs == 0
-				? trb_dma : request->sg[sg_iter].dma_address));
-
-		if (likely(!request->num_sgs))
+		if (sg_supported) {
+			trb->buffer = cpu_to_le32(TRB_BUFFER(sg_dma_address(s)));
+			length = sg_dma_len(s);
+		} else {
+			trb->buffer = cpu_to_le32(TRB_BUFFER(trb_dma));
 			length = request->length;
-		else
-			length = request->sg[sg_iter].length;
+		}
 
 		if (likely(priv_dev->dev_ver >= DEV_VER_V2))
 			td_size = DIV_ROUND_UP(length,
@@ -1215,6 +1219,9 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 		else
 			priv_req->trb->control = cpu_to_le32(control);
 
+		if (sg_supported)
+			s = sg_next(s);
+
 		control = 0;
 		++sg_iter;
 		priv_req->end_trb = priv_ep->enqueue;
-- 
2.17.1

[PATCH 2/8] usb: cdns3: gadget: improve the dump TRB operation at cdns3_ep_run_transfer

[Peter Chen]

It only dumps the first TRB per request, it is not useful if only dump
the first TRB when there are several TRBs per request. We improve it by
dumpping all TRBs per request in this commit.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 50282cab5fb6..f68c30b808dc 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1089,7 +1089,7 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 {
 	struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
 	struct cdns3_request *priv_req;
-	struct cdns3_trb *trb;
+	struct cdns3_trb *trb, *link_trb;
 	dma_addr_t trb_dma;
 	u32 togle_pcs = 1;
 	int sg_iter = 0;
@@ -1130,7 +1130,6 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 
 	/* prepare ring */
 	if ((priv_ep->enqueue + num_trb)  >= (priv_ep->num_trbs - 1)) {
-		struct cdns3_trb *link_trb;
 		int doorbell, dma_index;
 		u32 ch_bit = 0;
 
@@ -1265,7 +1264,22 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 	if (priv_dev->dev_ver <= DEV_VER_V2)
 		cdns3_wa1_tray_restore_cycle_bit(priv_dev, priv_ep);
 
-	trace_cdns3_prepare_trb(priv_ep, priv_req->trb);
+	if (num_trb > 1) {
+		int i = 0;
+
+		while (i < num_trb) {
+			trace_cdns3_prepare_trb(priv_ep, trb + i);
+			if (trb + i == link_trb) {
+				trb = priv_ep->trb_pool;
+				num_trb = num_trb - i;
+				i = 0;
+			} else {
+				i++;
+			}
+		}
+	} else {
+		trace_cdns3_prepare_trb(priv_ep, priv_req->trb);
+	}
 
 	/*
 	 * Memory barrier - Cycle Bit must be set before trb->length  and
-- 
2.17.1

[PATCH 3/8] usb: cdns3: gadget: calculate TDL correctly

[Peter Chen]

TDL is for TD length, so we need to calculate this value for
TD (request), but not for TRB, the TDL value is only needed
to stored at the first TRB in TD.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index f68c30b808dc..25e3ff1cdf61 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1100,6 +1100,7 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 	u16 total_tdl = 0;
 	struct scatterlist *s = NULL;
 	bool sg_supported = !!(request->num_mapped_sgs);
+	u16 td_size = 0;
 
 	if (priv_ep->type == USB_ENDPOINT_XFER_ISOC)
 		num_trb = priv_ep->interval;
@@ -1168,9 +1169,19 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 
 	/* set incorrect Cycle Bit for first trb*/
 	control = priv_ep->pcs ? 0 : TRB_CYCLE;
+	trb->length = 0;
+	if (priv_dev->dev_ver >= DEV_VER_V2) {
+		td_size = DIV_ROUND_UP(request->length,
+				       priv_ep->endpoint.maxpacket);
+
+		if (priv_dev->gadget.speed == USB_SPEED_SUPER)
+			trb->length = TRB_TDL_SS_SIZE(td_size);
+		else
+			control |= TRB_TDL_HS_SIZE(td_size);
+	}
+
 	do {
 		u32 length;
-		u16 td_size = 0;
 
 		/* fill TRB */
 		control |= TRB_TYPE(TRB_NORMAL);
@@ -1182,20 +1193,12 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 			length = request->length;
 		}
 
-		if (likely(priv_dev->dev_ver >= DEV_VER_V2))
-			td_size = DIV_ROUND_UP(length,
-					       priv_ep->endpoint.maxpacket);
-		else if (priv_ep->flags & EP_TDLCHK_EN)
+		if (priv_ep->flags & EP_TDLCHK_EN)
 			total_tdl += DIV_ROUND_UP(length,
 					       priv_ep->endpoint.maxpacket);
 
-		trb->length = cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size) |
+		trb->length |= cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size) |
 					TRB_LEN(length));
-		if (priv_dev->gadget.speed == USB_SPEED_SUPER)
-			trb->length |= cpu_to_le32(TRB_TDL_SS_SIZE(td_size));
-		else
-			control |= TRB_TDL_HS_SIZE(td_size);
-
 		pcs = priv_ep->pcs ? TRB_CYCLE : 0;
 
 		/*
@@ -1226,6 +1229,7 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 		priv_req->end_trb = priv_ep->enqueue;
 		cdns3_ep_inc_enq(priv_ep);
 		trb = priv_ep->trb_pool + priv_ep->enqueue;
+		trb->length = 0;
 	} while (sg_iter < num_trb);
 
 	trb = priv_req->trb;
-- 
2.17.1

[PATCH 4/8] usb: cdns3: gadget: add CHAIN and ISP bit for sg list use case

[Peter Chen]

For sg buffer list use case, we need to add ISP for each TRB, and
add CHAIN bit for each TRB except for the last TRB.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 25e3ff1cdf61..a308a694abc5 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1221,8 +1221,14 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 		else
 			priv_req->trb->control = cpu_to_le32(control);
 
-		if (sg_supported)
+		if (sg_supported) {
+			trb->control |= TRB_ISP;
+			/* Don't set chain bit for last TRB */
+			if (sg_iter < num_trb - 1)
+				trb->control |= TRB_CHAIN;
+
 			s = sg_next(s);
+		}
 
 		control = 0;
 		++sg_iter;
-- 
2.17.1

[PATCH 5/8] usb: cdns3: gadget: handle sg list use case at completion correctly

[Peter Chen]

- Judge each TRB has been handled at cdns3_trb_handled, since
the DMA pointer may be at the middle of the TD, we can't consider
this TD has finished at that time.
- Calcuate req->actual according to finished TRBs.
- Handle short transfer for sg list use case correctly. When the
short transfer occurs, we check OUT_SMM at TRB to see if it is
the last TRB.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 85 +++++++++++++++++++++++++-------------
 drivers/usb/cdns3/gadget.h |  9 ++++
 2 files changed, 65 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index a308a694abc5..6cb44c354f40 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -817,6 +817,8 @@ void cdns3_gadget_giveback(struct cdns3_endpoint *priv_ep,
 		       request->length);
 
 	priv_req->flags &= ~(REQUEST_PENDING | REQUEST_UNALIGNED);
+	/* All TRBs have finished, clear the counter */
+	priv_req->finished_trb = 0;
 	trace_cdns3_gadget_giveback(priv_req);
 
 	if (priv_dev->dev_ver < DEV_VER_V2) {
@@ -1241,6 +1243,7 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 	trb = priv_req->trb;
 
 	priv_req->flags |= REQUEST_PENDING;
+	priv_req->num_of_trb = num_trb;
 
 	if (sg_iter == 1)
 		trb->control |= cpu_to_le32(TRB_IOC | TRB_ISP);
@@ -1362,7 +1365,7 @@ void cdns3_set_hw_configuration(struct cdns3_device *priv_dev)
 }
 
 /**
- * cdns3_request_handled - check whether request has been handled by DMA
+ * cdns3_trb_handled - check whether trb has been handled by DMA
  *
  * @priv_ep: extended endpoint object.
  * @priv_req: request object for checking
@@ -1379,32 +1382,28 @@ void cdns3_set_hw_configuration(struct cdns3_device *priv_dev)
  * ET = priv_req->end_trb - index of last TRB in transfer ring
  * CI = current_index - index of processed TRB by DMA.
  *
- * As first step, function checks if cycle bit for priv_req->start_trb is
- * correct.
+ * As first step, we check if the TRB between the ST and ET.
+ * Then, we check if cycle bit for index priv_ep->dequeue
+ * is correct.
  *
  * some rules:
- * 1. priv_ep->dequeue never exceed current_index.
+ * 1. priv_ep->dequeue never equals to current_index.
  * 2  priv_ep->enqueue never exceed priv_ep->dequeue
  * 3. exception: priv_ep->enqueue == priv_ep->dequeue
  *    and priv_ep->free_trbs is zero.
  *    This case indicate that TR is full.
  *
- * Then We can split recognition into two parts:
+ * At below two cases, the request have been handled.
  * Case 1 - priv_ep->dequeue < current_index
  *      SR ... EQ ... DQ ... CI ... ER
  *      SR ... DQ ... CI ... EQ ... ER
  *
- *      Request has been handled by DMA if ST and ET is between DQ and CI.
- *
  * Case 2 - priv_ep->dequeue > current_index
- * This situation take place when CI go through the LINK TRB at the end of
+ * This situation takes place when CI go through the LINK TRB at the end of
  * transfer ring.
  *      SR ... CI ... EQ ... DQ ... ER
- *
- *      Request has been handled by DMA if ET is less then CI or
- *      ET is greater or equal DQ.
  */
-static bool cdns3_request_handled(struct cdns3_endpoint *priv_ep,
+static bool cdns3_trb_handled(struct cdns3_endpoint *priv_ep,
 				  struct cdns3_request *priv_req)
 {
 	struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
@@ -1416,7 +1415,25 @@ static bool cdns3_request_handled(struct cdns3_endpoint *priv_ep,
 	current_index = cdns3_get_dma_pos(priv_dev, priv_ep);
 	doorbell = !!(readl(&priv_dev->regs->ep_cmd) & EP_CMD_DRDY);
 
-	trb = &priv_ep->trb_pool[priv_req->start_trb];
+	/* current trb doesn't belong to this request */
+	if (priv_req->start_trb < priv_req->end_trb) {
+		if (priv_ep->dequeue > priv_req->end_trb)
+			goto finish;
+
+		if (priv_ep->dequeue < priv_req->start_trb)
+			goto finish;
+	}
+
+	if ((priv_req->start_trb > priv_req->end_trb) &&
+		(priv_ep->dequeue > priv_req->end_trb) &&
+		(priv_ep->dequeue < priv_req->start_trb))
+		goto finish;
+
+	if ((priv_req->start_trb == priv_req->end_trb) &&
+		(priv_ep->dequeue != priv_req->end_trb))
+		goto finish;
+
+	trb = &priv_ep->trb_pool[priv_ep->dequeue];
 
 	if ((le32_to_cpu(trb->control) & TRB_CYCLE) != priv_ep->ccs)
 		goto finish;
@@ -1438,12 +1455,8 @@ static bool cdns3_request_handled(struct cdns3_endpoint *priv_ep,
 		    !priv_ep->dequeue)
 			goto finish;
 
-		if (priv_req->end_trb >= priv_ep->dequeue &&
-		    priv_req->end_trb < current_index)
-			handled = 1;
+		handled = 1;
 	} else if (priv_ep->dequeue  > current_index) {
-		if (priv_req->end_trb  < current_index ||
-		    priv_req->end_trb >= priv_ep->dequeue)
 			handled = 1;
 	}
 
@@ -1459,6 +1472,8 @@ static void cdns3_transfer_completed(struct cdns3_device *priv_dev,
 	struct cdns3_request *priv_req;
 	struct usb_request *request;
 	struct cdns3_trb *trb;
+	bool request_handled = false;
+	bool transfer_end = false;
 
 	while (!list_empty(&priv_ep->pending_req_list)) {
 		request = cdns3_next_request(&priv_ep->pending_req_list);
@@ -1478,20 +1493,32 @@ static void cdns3_transfer_completed(struct cdns3_device *priv_dev,
 			 */
 			cdns3_select_ep(priv_dev, priv_ep->endpoint.address);
 
-			if (!cdns3_request_handled(priv_ep, priv_req))
-				goto prepare_next_td;
+			while (cdns3_trb_handled(priv_ep, priv_req)) {
+				priv_req->finished_trb++;
+				if (priv_req->finished_trb >= priv_req->num_of_trb)
+					request_handled = true;
 
-			trb = priv_ep->trb_pool + priv_ep->dequeue;
-			trace_cdns3_complete_trb(priv_ep, trb);
+				trb = priv_ep->trb_pool + priv_ep->dequeue;
+				trace_cdns3_complete_trb(priv_ep, trb);
 
-			if (trb != priv_req->trb)
-				dev_warn(priv_dev->dev,
-					 "request_trb=0x%p, queue_trb=0x%p\n",
-					 priv_req->trb, trb);
+				if (!transfer_end)
+					request->actual +=
+						TRB_LEN(le32_to_cpu(trb->length));
 
-			request->actual = TRB_LEN(le32_to_cpu(trb->length));
-			cdns3_move_deq_to_next_trb(priv_req);
-			cdns3_gadget_giveback(priv_ep, priv_req, 0);
+				if (priv_req->num_of_trb > 1 &&
+					le32_to_cpu(trb->control) & TRB_SMM)
+					transfer_end = true;
+
+				cdns3_ep_inc_deq(priv_ep);
+			}
+
+			if (request_handled) {
+				cdns3_gadget_giveback(priv_ep, priv_req, 0);
+				request_handled = false;
+				transfer_end = false;
+			} else {
+				goto prepare_next_td;
+			}
 
 			if (priv_ep->type != USB_ENDPOINT_XFER_ISOC &&
 			    TRBS_PER_SEGMENT == 2)
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
index 52765b098b9e..9f8bd452847e 100644
--- a/drivers/usb/cdns3/gadget.h
+++ b/drivers/usb/cdns3/gadget.h
@@ -1030,6 +1030,11 @@ struct cdns3_trb {
  * When set to '1', the device will toggle its interpretation of the Cycle bit
  */
 #define TRB_TOGGLE		BIT(1)
+/*
+ * The controller will set it if OUTSMM (OUT size mismatch) is detected,
+ * this bit is for normal TRB
+ */
+#define TRB_SMM			BIT(1)
 
 /*
  * Short Packet (SP). OUT EPs at DMULT=1 only. Indicates if the TRB was
@@ -1215,6 +1220,8 @@ struct cdns3_aligned_buf {
  *               this endpoint
  * @flags: flag specifying special usage of request
  * @list: used by internally allocated request to add to wa2_descmiss_req_list.
+ * @finished_trb: number of trb has already finished per request
+ * @num_of_trb: how many trbs in this request
  */
 struct cdns3_request {
 	struct usb_request		request;
@@ -1230,6 +1237,8 @@ struct cdns3_request {
 #define REQUEST_UNALIGNED		BIT(4)
 	u32				flags;
 	struct list_head		list;
+	int				finished_trb;
+	int				num_of_trb;
 };
 
 #define to_cdns3_request(r) (container_of(r, struct cdns3_request, request))
-- 
2.17.1

[PATCH 6/8] usb: cdns3: gadget: need to handle sg case for WA2 case

[Peter Chen]

Add sg support for WA2 case.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 44 +++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 6cb44c354f40..1fd36bc5c6db 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -462,6 +462,36 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev,
 		(reg) |= EP_STS_EN_DESCMISEN; \
 	} } while (0)
 
+static void __cdns3_descmiss_copy_data(struct usb_request *request,
+	struct usb_request *descmiss_req)
+{
+	int length = request->actual + descmiss_req->actual;
+	struct scatterlist *s = request->sg;
+
+	if (!s) {
+		if (length <= request->length) {
+			memcpy(&((u8 *)request->buf)[request->actual],
+			       descmiss_req->buf,
+			       descmiss_req->actual);
+			request->actual = length;
+		} else {
+			/* It should never occures */
+			request->status = -ENOMEM;
+		}
+	} else {
+		if (length <= sg_dma_len(s)) {
+			void *p = phys_to_virt(sg_dma_address(s));
+
+			memcpy(&((u8 *)p)[request->actual],
+				descmiss_req->buf,
+				descmiss_req->actual);
+			request->actual = length;
+		} else {
+			request->status = -ENOMEM;
+		}
+	}
+}
+
 /**
  * cdns3_wa2_descmiss_copy_data copy data from internal requests to
  * request queued by class driver.
@@ -488,21 +518,9 @@ static void cdns3_wa2_descmiss_copy_data(struct cdns3_endpoint *priv_ep,
 
 		chunk_end = descmiss_priv_req->flags & REQUEST_INTERNAL_CH;
 		length = request->actual + descmiss_req->actual;
-
 		request->status = descmiss_req->status;
-
-		if (length <= request->length) {
-			memcpy(&((u8 *)request->buf)[request->actual],
-			       descmiss_req->buf,
-			       descmiss_req->actual);
-			request->actual = length;
-		} else {
-			/* It should never occures */
-			request->status = -ENOMEM;
-		}
-
+		__cdns3_descmiss_copy_data(request, descmiss_req);
 		list_del_init(&descmiss_priv_req->list);
-
 		kfree(descmiss_req->buf);
 		cdns3_gadget_ep_free_request(&priv_ep->endpoint, descmiss_req);
 		--priv_ep->wa2_counter;
-- 
2.17.1

[PATCH 7/8] usb: cdns3: gadget: sg_support is only for DEV_VER_V2 or above

[Peter Chen]

The scatter buffer list support earlier than DEV_VER_V2 is not
good enough, software can't know well about short transfer for it.

Cc: Pawel Laszczak <pawell@cadence.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 1fd36bc5c6db..82dc362550bf 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -3161,7 +3161,6 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
 	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
 	priv_dev->gadget.ops = &cdns3_gadget_ops;
 	priv_dev->gadget.name = "usb-ss-gadget";
-	priv_dev->gadget.sg_supported = 1;
 	priv_dev->gadget.quirk_avoids_skb_reserve = 1;
 	priv_dev->gadget.irq = cdns->dev_irq;
 
@@ -3200,6 +3199,8 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
 		readl(&priv_dev->regs->usb_cap2));
 
 	priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
+	if (priv_dev->dev_ver >= DEV_VER_V2)
+		priv_dev->gadget.sg_supported = 1;
 
 	priv_dev->zlp_buf = kzalloc(CDNS3_EP_ZLP_BUF_SIZE, GFP_KERNEL);
 	if (!priv_dev->zlp_buf) {
-- 
2.17.1

[PATCH 8/8] usb: cdns3: gadget: enlarge the TRB ring length

[Peter Chen]

At Android ADB and MTP use case, it uses f_fs which supports scatter list,
it means one request may need several TRBs for it. Besides, TRB consumes
very fast compared to TRB has prepared for above use case, so we need to
enlarge the TRB ring length to avoid "no free TRB error".

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
index 9f8bd452847e..1ccecd237530 100644
--- a/drivers/usb/cdns3/gadget.h
+++ b/drivers/usb/cdns3/gadget.h
@@ -966,7 +966,7 @@ struct cdns3_usb_regs {
 /*
  * USBSS-DEV DMA interface.
  */
-#define TRBS_PER_SEGMENT	40
+#define TRBS_PER_SEGMENT	600
 
 #define ISO_MAX_INTERVAL	10
 
-- 
2.17.1

Re: [PATCH 2/8] usb: cdns3: gadget: improve the dump TRB operation at cdns3_ep_run_transfer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

Peter Chen <peter.chen@nxp.com> writes:

> It only dumps the first TRB per request, it is not useful if only dump
> the first TRB when there are several TRBs per request. We improve it by
> dumpping all TRBs per request in this commit.
>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/gadget.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 50282cab5fb6..f68c30b808dc 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -1089,7 +1089,7 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>  {
>  	struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
>  	struct cdns3_request *priv_req;
> -	struct cdns3_trb *trb;
> +	struct cdns3_trb *trb, *link_trb;

one declaration per line

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 1/8] usb: cdns3: gadget: using correct sg operations

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> @@ -1162,22 +1164,24 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>  	if (priv_dev->dev_ver <= DEV_VER_V2)
>  		togle_pcs = cdns3_wa1_update_guard(priv_ep, trb);
>  
> +	if (sg_supported)
> +		s = request->sg;
> +
>  	/* set incorrect Cycle Bit for first trb*/
>  	control = priv_ep->pcs ? 0 : TRB_CYCLE;
> -

unnecessary change

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 3/8] usb: cdns3: gadget: calculate TDL correctly

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> @@ -1168,9 +1169,19 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>  
>  	/* set incorrect Cycle Bit for first trb*/
>  	control = priv_ep->pcs ? 0 : TRB_CYCLE;
> +	trb->length = 0;
> +	if (priv_dev->dev_ver >= DEV_VER_V2) {
> +		td_size = DIV_ROUND_UP(request->length,
> +				       priv_ep->endpoint.maxpacket);

spaces for indentation?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 4/8] usb: cdns3: gadget: add CHAIN and ISP bit for sg list use case

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> For sg buffer list use case, we need to add ISP for each TRB, and
> add CHAIN bit for each TRB except for the last TRB.
>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/gadget.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 25e3ff1cdf61..a308a694abc5 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -1221,8 +1221,14 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>  		else
>  			priv_req->trb->control = cpu_to_le32(control);
>  
> -		if (sg_supported)
> +		if (sg_supported) {
> +			trb->control |= TRB_ISP;
> +			/* Don't set chain bit for last TRB */
> +			if (sg_iter < num_trb - 1)
> +				trb->control |= TRB_CHAIN;
> +
>  			s = sg_next(s);
> +		}

is this a bugfix?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 5/8] usb: cdns3: gadget: handle sg list use case at completion correctly

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

Peter Chen <peter.chen@nxp.com> writes:

> - Judge each TRB has been handled at cdns3_trb_handled, since
> the DMA pointer may be at the middle of the TD, we can't consider
> this TD has finished at that time.
> - Calcuate req->actual according to finished TRBs.
    ^^^^^^^^
    calculate

> - Handle short transfer for sg list use case correctly. When the
> short transfer occurs, we check OUT_SMM at TRB to see if it is
> the last TRB.

it looks like each of these should be split to its own patch.

> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/gadget.c | 85 +++++++++++++++++++++++++-------------
>  drivers/usb/cdns3/gadget.h |  9 ++++
>  2 files changed, 65 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index a308a694abc5..6cb44c354f40 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -817,6 +817,8 @@ void cdns3_gadget_giveback(struct cdns3_endpoint *priv_ep,
>  		       request->length);
>  
>  	priv_req->flags &= ~(REQUEST_PENDING | REQUEST_UNALIGNED);
> +	/* All TRBs have finished, clear the counter */
> +	priv_req->finished_trb = 0;
>  	trace_cdns3_gadget_giveback(priv_req);
>  
>  	if (priv_dev->dev_ver < DEV_VER_V2) {
> @@ -1241,6 +1243,7 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>  	trb = priv_req->trb;
>  
>  	priv_req->flags |= REQUEST_PENDING;
> +	priv_req->num_of_trb = num_trb;
>  
>  	if (sg_iter == 1)
>  		trb->control |= cpu_to_le32(TRB_IOC | TRB_ISP);
> @@ -1362,7 +1365,7 @@ void cdns3_set_hw_configuration(struct cdns3_device *priv_dev)
>  }
>  
>  /**
> - * cdns3_request_handled - check whether request has been handled by DMA
> + * cdns3_trb_handled - check whether trb has been handled by DMA
>   *
>   * @priv_ep: extended endpoint object.
>   * @priv_req: request object for checking
> @@ -1379,32 +1382,28 @@ void cdns3_set_hw_configuration(struct cdns3_device *priv_dev)
>   * ET = priv_req->end_trb - index of last TRB in transfer ring
>   * CI = current_index - index of processed TRB by DMA.
>   *
> - * As first step, function checks if cycle bit for priv_req->start_trb is
> - * correct.
> + * As first step, we check if the TRB between the ST and ET.
> + * Then, we check if cycle bit for index priv_ep->dequeue
> + * is correct.
>   *
>   * some rules:
> - * 1. priv_ep->dequeue never exceed current_index.
> + * 1. priv_ep->dequeue never equals to current_index.
>   * 2  priv_ep->enqueue never exceed priv_ep->dequeue
>   * 3. exception: priv_ep->enqueue == priv_ep->dequeue
>   *    and priv_ep->free_trbs is zero.
>   *    This case indicate that TR is full.
>   *
> - * Then We can split recognition into two parts:
> + * At below two cases, the request have been handled.
>   * Case 1 - priv_ep->dequeue < current_index
>   *      SR ... EQ ... DQ ... CI ... ER
>   *      SR ... DQ ... CI ... EQ ... ER
>   *
> - *      Request has been handled by DMA if ST and ET is between DQ and CI.
> - *
>   * Case 2 - priv_ep->dequeue > current_index
> - * This situation take place when CI go through the LINK TRB at the end of
> + * This situation takes place when CI go through the LINK TRB at the end of

not part of $subject

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 6/8] usb: cdns3: gadget: need to handle sg case for WA2 case

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> Add sg support for WA2 case.

what's WA2? Care to spell it out?

> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/gadget.c | 44 +++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 6cb44c354f40..1fd36bc5c6db 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -462,6 +462,36 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev,
>  		(reg) |= EP_STS_EN_DESCMISEN; \
>  	} } while (0)
>  
> +static void __cdns3_descmiss_copy_data(struct usb_request *request,
> +	struct usb_request *descmiss_req)
> +{
> +	int length = request->actual + descmiss_req->actual;
> +	struct scatterlist *s = request->sg;
> +
> +	if (!s) {
> +		if (length <= request->length) {
> +			memcpy(&((u8 *)request->buf)[request->actual],

			memcpy(request->buf + request->actual, ... ?

> +			       descmiss_req->buf,
> +			       descmiss_req->actual);
> +			request->actual = length;
> +		} else {
> +			/* It should never occures */
                                           ^^^^^^^
                                           occur

ps: famous last words :-)

> +			request->status = -ENOMEM;

this is not documented as a valid status for usb request
completion. Who's treating this -ENOMEM case? Which gadgets have you
tested with this change?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 7/8] usb: cdns3: gadget: sg_support is only for DEV_VER_V2 or above

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

Peter Chen <peter.chen@nxp.com> writes:

> The scatter buffer list support earlier than DEV_VER_V2 is not
> good enough, software can't know well about short transfer for it.
>
> Cc: Pawel Laszczak <pawell@cadence.com>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/gadget.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 1fd36bc5c6db..82dc362550bf 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -3161,7 +3161,6 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
>  	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>  	priv_dev->gadget.ops = &cdns3_gadget_ops;
>  	priv_dev->gadget.name = "usb-ss-gadget";
> -	priv_dev->gadget.sg_supported = 1;
>  	priv_dev->gadget.quirk_avoids_skb_reserve = 1;
>  	priv_dev->gadget.irq = cdns->dev_irq;
>  
> @@ -3200,6 +3199,8 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
>  		readl(&priv_dev->regs->usb_cap2));
>  
>  	priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
> +	if (priv_dev->dev_ver >= DEV_VER_V2)
> +		priv_dev->gadget.sg_supported = 1;

is this a bug fix?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 8/8] usb: cdns3: gadget: enlarge the TRB ring length

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> At Android ADB and MTP use case, it uses f_fs which supports scatter list,
> it means one request may need several TRBs for it. Besides, TRB consumes
> very fast compared to TRB has prepared for above use case, so we need to
> enlarge the TRB ring length to avoid "no free TRB error".

can you give a little more detail here? How many sg entries do you get
with ADB? What's the size of each TRB? How many memory does 600 TRBs
actually amount to? How many segments are you using per endpoint?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 1/8] usb: cdns3: gadget: using correct sg operations

[Peter Chen]

On 20-09-08 09:19:52, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> writes:
> > @@ -1162,22 +1164,24 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
> >  	if (priv_dev->dev_ver <= DEV_VER_V2)
> >  		togle_pcs = cdns3_wa1_update_guard(priv_ep, trb);
> >  
> > +	if (sg_supported)
> > +		s = request->sg;
> > +
> >  	/* set incorrect Cycle Bit for first trb*/
> >  	control = priv_ep->pcs ? 0 : TRB_CYCLE;
> > -
> 
> unnecessary change
> 

Will change, thanks.

-- 

Thanks,
Peter Chen

Re: [PATCH 2/8] usb: cdns3: gadget: improve the dump TRB operation at cdns3_ep_run_transfer

[Peter Chen]

On 20-09-08 09:19:11, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
> 
> > It only dumps the first TRB per request, it is not useful if only dump
> > the first TRB when there are several TRBs per request. We improve it by
> > dumpping all TRBs per request in this commit.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/gadget.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > index 50282cab5fb6..f68c30b808dc 100644
> > --- a/drivers/usb/cdns3/gadget.c
> > +++ b/drivers/usb/cdns3/gadget.c
> > @@ -1089,7 +1089,7 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
> >  {
> >  	struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
> >  	struct cdns3_request *priv_req;
> > -	struct cdns3_trb *trb;
> > +	struct cdns3_trb *trb, *link_trb;
> 
> one declaration per line
> 

Will change, thanks.

-- 

Thanks,
Peter Chen

Re: [PATCH 3/8] usb: cdns3: gadget: calculate TDL correctly

[Peter Chen]

On 20-09-08 09:21:42, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> writes:
> > @@ -1168,9 +1169,19 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
> >  
> >  	/* set incorrect Cycle Bit for first trb*/
> >  	control = priv_ep->pcs ? 0 : TRB_CYCLE;
> > +	trb->length = 0;
> > +	if (priv_dev->dev_ver >= DEV_VER_V2) {
> > +		td_size = DIV_ROUND_UP(request->length,
> > +				       priv_ep->endpoint.maxpacket);
> 
> spaces for indentation?
> 

Will delete the spaces, thanks.

-- 

Thanks,
Peter Chen

Re: [PATCH 4/8] usb: cdns3: gadget: add CHAIN and ISP bit for sg list use case

[Peter Chen]

On 20-09-08 09:22:44, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> writes:
> > For sg buffer list use case, we need to add ISP for each TRB, and
> > add CHAIN bit for each TRB except for the last TRB.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/gadget.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > index 25e3ff1cdf61..a308a694abc5 100644
> > --- a/drivers/usb/cdns3/gadget.c
> > +++ b/drivers/usb/cdns3/gadget.c
> > @@ -1221,8 +1221,14 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
> >  		else
> >  			priv_req->trb->control = cpu_to_le32(control);
> >  
> > -		if (sg_supported)
> > +		if (sg_supported) {
> > +			trb->control |= TRB_ISP;
> > +			/* Don't set chain bit for last TRB */
> > +			if (sg_iter < num_trb - 1)
> > +				trb->control |= TRB_CHAIN;
> > +
> >  			s = sg_next(s);
> > +		}
> 
> is this a bugfix?
> 

The support for sg list is not good at current code, it needs all
changes in this patch series to let it work well, so it is better
let the whole things in this series as improvement.

-- 

Thanks,
Peter Chen

Re: [PATCH 4/8] usb: cdns3: gadget: add CHAIN and ISP bit for sg list use case

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
>> > For sg buffer list use case, we need to add ISP for each TRB, and
>> > add CHAIN bit for each TRB except for the last TRB.
>> >
>> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
>> > ---
>> >  drivers/usb/cdns3/gadget.c | 8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> > index 25e3ff1cdf61..a308a694abc5 100644
>> > --- a/drivers/usb/cdns3/gadget.c
>> > +++ b/drivers/usb/cdns3/gadget.c
>> > @@ -1221,8 +1221,14 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>> >  		else
>> >  			priv_req->trb->control = cpu_to_le32(control);
>> >  
>> > -		if (sg_supported)
>> > +		if (sg_supported) {
>> > +			trb->control |= TRB_ISP;
>> > +			/* Don't set chain bit for last TRB */
>> > +			if (sg_iter < num_trb - 1)
>> > +				trb->control |= TRB_CHAIN;
>> > +
>> >  			s = sg_next(s);
>> > +		}
>> 
>> is this a bugfix?
>> 
>
> The support for sg list is not good at current code, it needs all
> changes in this patch series to let it work well, so it is better
> let the whole things in this series as improvement.

Cool, thanks for clarifying :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 5/8] usb: cdns3: gadget: handle sg list use case at completion correctly

[Peter Chen]

On 20-09-08 09:25:41, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
> 
> > - Judge each TRB has been handled at cdns3_trb_handled, since
> > the DMA pointer may be at the middle of the TD, we can't consider
> > this TD has finished at that time.
> > - Calcuate req->actual according to finished TRBs.
>     ^^^^^^^^
>     calculate
> 
> > - Handle short transfer for sg list use case correctly. When the
> > short transfer occurs, we check OUT_SMM at TRB to see if it is
> > the last TRB.
> 
> it looks like each of these should be split to its own patch.

When I debug sg use case, it indeed took several patches for all
functions work, and some patches improved the old patches since
some short transfers use case did not be considered well.

Using this patch, it could let the completion work for both normal
transfer and short transfer. So I prefer keeping one patch.

> > + * Then, we check if cycle bit for index priv_ep->dequeue
> > + * is correct.
> >   *
> >   * some rules:
> > - * 1. priv_ep->dequeue never exceed current_index.
> > + * 1. priv_ep->dequeue never equals to current_index.
> >   * 2  priv_ep->enqueue never exceed priv_ep->dequeue
> >   * 3. exception: priv_ep->enqueue == priv_ep->dequeue
> >   *    and priv_ep->free_trbs is zero.
> >   *    This case indicate that TR is full.
> >   *
> > - * Then We can split recognition into two parts:
> > + * At below two cases, the request have been handled.
> >   * Case 1 - priv_ep->dequeue < current_index
> >   *      SR ... EQ ... DQ ... CI ... ER
> >   *      SR ... DQ ... CI ... EQ ... ER
> >   *
> > - *      Request has been handled by DMA if ST and ET is between DQ and CI.
> > - *
> >   * Case 2 - priv_ep->dequeue > current_index
> > - * This situation take place when CI go through the LINK TRB at the end of
> > + * This situation takes place when CI go through the LINK TRB at the end of
> 
> not part of $subject
> 

I will make another patch for comment improvement, thanks.

-- 

Thanks,
Peter Chen

Re: [PATCH 6/8] usb: cdns3: gadget: need to handle sg case for WA2 case

[Peter Chen]

On 20-09-08 09:29:55, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> writes:
> > Add sg support for WA2 case.
> 
> what's WA2? Care to spell it out?

It is the workaround 2, there is a description for it at the beginning
of this file. If you search the file, you may find there are some
functions has "wa2" prefix. I will improve the commit log for it.

> 
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/gadget.c | 44 +++++++++++++++++++++++++++-----------
> >  1 file changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > index 6cb44c354f40..1fd36bc5c6db 100644
> > --- a/drivers/usb/cdns3/gadget.c
> > +++ b/drivers/usb/cdns3/gadget.c
> > @@ -462,6 +462,36 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev,
> >  		(reg) |= EP_STS_EN_DESCMISEN; \
> >  	} } while (0)
> >  
> > +static void __cdns3_descmiss_copy_data(struct usb_request *request,
> > +	struct usb_request *descmiss_req)
> > +{
> > +	int length = request->actual + descmiss_req->actual;
> > +	struct scatterlist *s = request->sg;
> > +
> > +	if (!s) {
> > +		if (length <= request->length) {
> > +			memcpy(&((u8 *)request->buf)[request->actual],
> 
> 			memcpy(request->buf + request->actual, ... ?
> 
> > +			       descmiss_req->buf,
> > +			       descmiss_req->actual);
> > +			request->actual = length;
> > +		} else {
> > +			/* It should never occures */
>                                            ^^^^^^^
>                                            occur

Will fix.

> 
> ps: famous last words :-)
> 
> > +			request->status = -ENOMEM;
> 
> this is not documented as a valid status for usb request
> completion. Who's treating this -ENOMEM case? Which gadgets have you
> tested with this change?
> 

I just add sg use case for this WA2, do not touch the current design.
The legacy chip has issue that it gets OUT data at FIFO and send
ACK to host even the endpoints are not enabled, it assumes the length
of this data is not longer than the data length the gadget request
later. We use acm + ncm to reproduce it, the pre-load data at FIFO is
several bytes, no more than 1024 bytes.

I have not found the valid status described at gadget.h, it only
describes the usage for "-ESHUTDOWN".

-- 

Thanks,
Peter Chen

Re: [PATCH 7/8] usb: cdns3: gadget: sg_support is only for DEV_VER_V2 or above

[Peter Chen]

On 20-09-08 09:30:59, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
> 
> > The scatter buffer list support earlier than DEV_VER_V2 is not
> > good enough, software can't know well about short transfer for it.
> >
> > Cc: Pawel Laszczak <pawell@cadence.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/gadget.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > index 1fd36bc5c6db..82dc362550bf 100644
> > --- a/drivers/usb/cdns3/gadget.c
> > +++ b/drivers/usb/cdns3/gadget.c
> > @@ -3161,7 +3161,6 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
> >  	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
> >  	priv_dev->gadget.ops = &cdns3_gadget_ops;
> >  	priv_dev->gadget.name = "usb-ss-gadget";
> > -	priv_dev->gadget.sg_supported = 1;
> >  	priv_dev->gadget.quirk_avoids_skb_reserve = 1;
> >  	priv_dev->gadget.irq = cdns->dev_irq;
> >  
> > @@ -3200,6 +3199,8 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
> >  		readl(&priv_dev->regs->usb_cap2));
> >  
> >  	priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
> > +	if (priv_dev->dev_ver >= DEV_VER_V2)
> > +		priv_dev->gadget.sg_supported = 1;
> 
> is this a bug fix?
> 

Like answered at Patch 4, it is better to keep the whole patch series as
the improvement for sg use case.

-- 

Thanks,
Peter Chen

Re: [PATCH 8/8] usb: cdns3: gadget: enlarge the TRB ring length

[Peter Chen]

On 20-09-08 09:32:32, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> writes:
> > At Android ADB and MTP use case, it uses f_fs which supports scatter list,
> > it means one request may need several TRBs for it. Besides, TRB consumes
> > very fast compared to TRB has prepared for above use case, so we need to
> > enlarge the TRB ring length to avoid "no free TRB error".
> 
> can you give a little more detail here?

I will.

> How many sg entries do you get with ADB? What's the size of each TRB?

I remembered it is about 120 requests for ADB and MTP use case, 16KB for
each sg entry, so four TRBs (4KB/TRB) per sg entry at worst case.

> How many memory does 600 TRBs
> actually amount to? How many segments are you using per endpoint?
> 

Each TRB consumes 3 * 32 bits =  12 bytes, 600 TRB consumes 7200 bytes.
One segment for each endpoint, one segment includes 600 TRBs.

-- 

Thanks,
Peter Chen

Re: [PATCH 8/8] usb: cdns3: gadget: enlarge the TRB ring length

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> On 20-09-08 09:32:32, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Peter Chen <peter.chen@nxp.com> writes:
>> > At Android ADB and MTP use case, it uses f_fs which supports scatter list,
>> > it means one request may need several TRBs for it. Besides, TRB consumes
>> > very fast compared to TRB has prepared for above use case, so we need to
>> > enlarge the TRB ring length to avoid "no free TRB error".
>> 
>> can you give a little more detail here?
>
> I will.
>
>> How many sg entries do you get with ADB? What's the size of each TRB?
>
> I remembered it is about 120 requests for ADB and MTP use case, 16KB for
> each sg entry, so four TRBs (4KB/TRB) per sg entry at worst case.

I wonder why this isn't a problem for other UDCs, though. I haven't had
a similar report on dwc3, where we use 256 (255 + 1 link) TRBs for each
endpoint and we never use more than one segment. What you describe here
would amount of 480 TRBs worst case.

Anyway, this is hypothetical, something to keep an eye for dwc3 users
too, I guess.

>> How many memory does 600 TRBs
>> actually amount to? How many segments are you using per endpoint?
>> 
>
> Each TRB consumes 3 * 32 bits =  12 bytes, 600 TRB consumes 7200 bytes.
> One segment for each endpoint, one segment includes 600 TRBs.

thanks for the info

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

RE: [PATCH 5/8] usb: cdns3: gadget: handle sg list use case at completion correctly

[Peter Chen]

 
> 
> When I debug sg use case, it indeed took several patches for all functions work,
> and some patches improved the old patches since some short transfers use
> case did not be considered well.
> 
> Using this patch, it could let the completion work for both normal transfer and
> short transfer. So I prefer keeping one patch.
> 
> > > + * Then, we check if cycle bit for index priv_ep->dequeue
> > > + * is correct.
> > >   *
> > >   * some rules:
> > > - * 1. priv_ep->dequeue never exceed current_index.
> > > + * 1. priv_ep->dequeue never equals to current_index.
> > >   * 2  priv_ep->enqueue never exceed priv_ep->dequeue
> > >   * 3. exception: priv_ep->enqueue == priv_ep->dequeue
> > >   *    and priv_ep->free_trbs is zero.
> > >   *    This case indicate that TR is full.
> > >   *
> > > - * Then We can split recognition into two parts:
> > > + * At below two cases, the request have been handled.
> > >   * Case 1 - priv_ep->dequeue < current_index
> > >   *      SR ... EQ ... DQ ... CI ... ER
> > >   *      SR ... DQ ... CI ... EQ ... ER
> > >   *
> > > - *      Request has been handled by DMA if ST and ET is between DQ
> and CI.
> > > - *
> > >   * Case 2 - priv_ep->dequeue > current_index
> > > - * This situation take place when CI go through the LINK TRB at the
> > > end of
> > > + * This situation takes place when CI go through the LINK TRB at
> > > + the end of
> >
> > not part of $subject
> >
> 
> I will make another patch for comment improvement, thanks.
> 
 
I find I change the function from handle request to handle TRB, so the comments for this function
needs to be updated accordingly, it needs to be at the same patch.

Peter