usb: cdns3: improve the sg use case

Hi,

Quoted 4 lines · Peter Chen

drivers/usb/cdns3/gadget.c +0 -0

@@ -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);
 

drivers/usb/cdns3/gadget.c static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep, · line 1167

Quoted 6 lines

drivers/usb/cdns3/gadget.c +3 -1

+	if (sg_supported)
+		s = request->sg;
+
 	/* set incorrect Cycle Bit for first trb*/
 	control = priv_ep->pcs ? 0 : TRB_CYCLE;
-

unnecessary change

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

---
 drivers/usb/cdns3/gadget.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/usb/cdns3/gadget.c +0 -0

index 50282cab5fb6..f68c30b808dc 100644

Quoted 9 lines · 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 <[email protected]>

---
 drivers/usb/cdns3/gadget.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/usb/cdns3/gadget.c static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep, · line 1092

Quoted 10 lines

drivers/usb/cdns3/gadget.c +1 -1

index 50282cab5fb6..f68c30b808dc 100644
@@ -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

Hi,

Quoted 2 lines · Peter Chen

(quoted diff) +0 -0

@@ -1168,9 +1169,19 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 

drivers/usb/cdns3/gadget.c static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep, · line 1175

Quoted 6 lines

drivers/usb/cdns3/gadget.c +4 -0

 	/* 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?

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

---
 drivers/usb/cdns3/gadget.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/usb/cdns3/gadget.c +0 -0

index 25e3ff1cdf61..a308a694abc5 100644
@@ -1221,8 +1221,14 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 		else
 			priv_req->trb->control = cpu_to_le32(control);

Hi,

Quoted 16 lines · 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 <[email protected]>

---
 drivers/usb/cdns3/gadget.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/usb/cdns3/gadget.c +0 -0

index 25e3ff1cdf61..a308a694abc5 100644
@@ -1221,8 +1221,14 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 		else
 			priv_req->trb->control = cpu_to_le32(control);
 

drivers/usb/cdns3/gadget.c static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep, · line 1231

Quoted 9 lines

drivers/usb/cdns3/gadget.c +7 -1

-		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?

Quoted 16 lines

Quoted 16 lines

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

---
 drivers/usb/cdns3/gadget.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/usb/cdns3/gadget.c +0 -0

index 25e3ff1cdf61..a308a694abc5 100644
@@ -1221,8 +1221,14 @@ static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
 		else
 			priv_req->trb->control = cpu_to_le32(control);
 

Quoted 12 lines

drivers/usb/cdns3/gadget.c static int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep, · line 1231

Quoted 9 lines

drivers/usb/cdns3/gadget.c +7 -1

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

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

---
 drivers/usb/cdns3/gadget.c | 85 +++++++++++++++++++++++++-------------
 drivers/usb/cdns3/gadget.h |  9 ++++
 2 files changed, 65 insertions(+), 29 deletions(-)

drivers/usb/cdns3/gadget.c +8 -5

index a308a694abc5..6cb44c354f40 100644
@@ -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:

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

    ^^^^^^^^
    calculate

Quoted 3 lines

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

Quoted 14 lines

+ * 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

Quoted 8 lines

* 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

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.

Quoted 10 lines

Quoted 10 lines

+ * 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.

Quoted 8 lines

Quoted 8 lines

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

Quoted 7 lines

Quoted 6 lines

- *
* 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

Quoted 2 lines

not part of $subject

I will make another patch for comment improvement, thanks.

Add sg support for WA2 case.

Signed-off-by Peter Chen <[email protected]>

---
 drivers/usb/cdns3/gadget.c | 44 +++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

drivers/usb/cdns3/gadget.c +0 -0

index 6cb44c354f40..1fd36bc5c6db 100644
@@ -462,6 +462,36 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev,
 		(reg) |= EP_STS_EN_DESCMISEN; \
 	} } while (0)

Hi,

Quoted 1 line · Peter Chen

Add sg support for WA2 case.

what's WA2? Care to spell it out?

Quoted 13 lines

Signed-off-by Peter Chen <[email protected]>

---
 drivers/usb/cdns3/gadget.c | 44 +++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

drivers/usb/cdns3/gadget.c +0 -0

index 6cb44c354f40..1fd36bc5c6db 100644
@@ -462,6 +462,36 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev,
 		(reg) |= EP_STS_EN_DESCMISEN; \
 	} } while (0)

drivers/usb/cdns3/gadget.c static int cdns3_start_all_request(struct cdns3_device *priv_dev, · line 473

Quoted 10 lines

drivers/usb/cdns3/gadget.c +9 -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, ... ?

drivers/usb/cdns3/gadget.c static int cdns3_start_all_request(struct cdns3_device *priv_dev, · line 478

Quoted 5 lines

drivers/usb/cdns3/gadget.c +5 -0

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

                                           ^^^^^^^
                                           occur

ps: famous last words :-)

drivers/usb/cdns3/gadget.c static int cdns3_start_all_request(struct cdns3_device *priv_dev, · line 479

Quoted 1 line

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

+			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?

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

Signed-off-by Peter Chen <[email protected]>

---
 drivers/usb/cdns3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

index 1fd36bc5c6db..82dc362550bf 100644
@@ -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;
 

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Peter Chen <[email protected]>

---
 drivers/usb/cdns3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

index 1fd36bc5c6db..82dc362550bf 100644
@@ -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;
 

drivers/usb/cdns3/gadget.c static int cdns3_gadget_start(struct cdns3 *cdns) · line 3203

Quoted 6 lines

drivers/usb/cdns3/gadget.c +2 -0

@@ -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?

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

Hi,

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

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?

Quoted 9 lines · Felipe Balbi

Hi,

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

Quoted 1 line

can you give a little more detail here?

I will.

Quoted 1 line

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.

Quoted 3 lines

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.