[v3] usb: dwc2: gadget: Add scatter-gather mode

[v3] usb: dwc2: gadget: Add scatter-gather mode

[Andrzej Pietrasiewicz]

Hi Minas,


W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze:
> Hi,
> 
> SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on
> ep1in. It's first BULK IN transaction after GET MAXLUN.
>
Can you help me reproduce the problem? I have an Odroid U3.

> 
> Meantime it's passing smoke test with mass storage function connected to
> Linux host.

So the "real" use case with mass storage works, right?

Andrzej

[v3] usb: dwc2: gadget: Add scatter-gather mode

[Minas Harutyunyan]

Hi Andrzej,

On 3/15/2019 3:32 PM, Andrzej Pietrasiewicz wrote:
> Hi again,
> 
> sorry for long lines. Reformatted now.
> 
> Hi Minas,
> 
> 
> W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze:
>> Hi,
>>
>> SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on
>> ep1in. It's first BULK IN transaction after GET MAXLUN.
>>
> 
> Thank you for reporting.
> 
> I bisected and identified my own
> 
> commit 10209abe87f5ebfd482a00323f5236d6094d0865
> Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Date:   Mon Jan 21 14:44:47 2019 +0100
> 
>       usb: dwc2: gadget: Add scatter-gather mode
> 
> as the one introducing the regression.
> 
> At this moment I don't know how to fix it. I will lokk into it next week.
> 
> @colleagues from Samsung: can you have a look at the 4412 datasheet
> and verify that in this patch all bits are properly set for
> scatter-gather mode?
> 
> 
> Andrzej
> 
> PS. @Minas: Is there any reliable way to switch back and forth between
> running CV software and regular usage of USB? What an aggressive piece
> of software the USB 3 Gen X Command Verifier is! Its installer warns
> "not to run the application" in case you use devices connected to the chosen
> Host Controller because it will take over that HC. I thought this
> referred to
> the application being installed and that as long I didn't run the tests
> (i.e. the installed application) everything would be fine. How wrong I was!
> The warning should have clearly indicated that it refers to the
> installer itself!
> I was then unable to use keyboard/mouse connected to the docking station
> even if I uninstalled the CV software. I use Linux 99% of the time, but
> for the
> remaining 1% I want my Windows installation to be working, I don't know how
> to repair Windows. I somehow managed to recover but I am not going to
> install this on my laptop again. I managed to get some other laptop which
> is not used with a docking station, but still with this software it is
> best to have
> a dedicated machine only for that purpose. Maybe it is the Windows itself
> and the application is implemented the only possible way, I don't know.
> 
Yes, using personal laptop for USB CV testing is not good idea. We use 
lab machine for this kind of testing.

Thanks,
Minas

[v3] usb: dwc2: gadget: Add scatter-gather mode

[Andrzej Pietrasiewicz]

Hi again,

sorry for long lines. Reformatted now.

Hi Minas,


W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze:
> Hi,
>
> SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on
> ep1in. It's first BULK IN transaction after GET MAXLUN.
>

Thank you for reporting.

I bisected and identified my own

commit 10209abe87f5ebfd482a00323f5236d6094d0865
Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Date:   Mon Jan 21 14:44:47 2019 +0100

     usb: dwc2: gadget: Add scatter-gather mode

as the one introducing the regression.

At this moment I don't know how to fix it. I will lokk into it next week.

@colleagues from Samsung: can you have a look at the 4412 datasheet
and verify that in this patch all bits are properly set for 
scatter-gather mode?


Andrzej

PS. @Minas: Is there any reliable way to switch back and forth between
running CV software and regular usage of USB? What an aggressive piece
of software the USB 3 Gen X Command Verifier is! Its installer warns
"not to run the application" in case you use devices connected to the chosen
Host Controller because it will take over that HC. I thought this 
referred to
the application being installed and that as long I didn't run the tests
(i.e. the installed application) everything would be fine. How wrong I was!
The warning should have clearly indicated that it refers to the 
installer itself!
I was then unable to use keyboard/mouse connected to the docking station
even if I uninstalled the CV software. I use Linux 99% of the time, but 
for the
remaining 1% I want my Windows installation to be working, I don't know how
to repair Windows. I somehow managed to recover but I am not going to
install this on my laptop again. I managed to get some other laptop which
is not used with a docking station, but still with this software it is 
best to have
a dedicated machine only for that purpose. Maybe it is the Windows itself
and the application is implemented the only possible way, I don't know.

[v3] usb: dwc2: gadget: Add scatter-gather mode

[Andrzej Pietrasiewicz]

Hi Minas,

W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze:
> Hi,
> 
> SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on
> ep1in. It's first BULK IN transaction after GET MAXLUN.
> 

Thank you for reporting.

I bisected and identified my own

commit 10209abe87f5ebfd482a00323f5236d6094d0865
Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Date:   Mon Jan 21 14:44:47 2019 +0100

     usb: dwc2: gadget: Add scatter-gather mode

as the one introducing the regression.

At this moment I don't know how to fix it. I will lokk into it next week.

@colleagues from Samsung: can you have a look at the 4412 datasheet and 
verify that in this patch all bits are properly set for scatter-gather mode?

Andrzej

PS. @Minas: Is there any reliable way to switch back and forth between 
running CV software and regular usage of USB? What an aggressive piece 
of software the USB 3 Gen X Command Verifier is! Its installer warns 
"not to run the application" in case you use devices connected to the 
chosen Host Controller because it will take over that HC. I thought this 
referred to the application being installed and that as long I didn't 
run the tests (i.e. the installed application) everything would be fine. 
How wrong I was! The warning should have clearly indicated that it 
refers to the installer itself! I was then unable to use keyboard/mouse 
connected to the docking station even if I uninstalled the CV software. 
I use Linux 99% of the time, but for the remaining 1% I want my Windows 
installation to be working, I don't know how to repair Windows. I 
somehow managed to recover but I am not going to install this on my 
laptop again. I managed to get some other laptop which is not used with 
a docking station, but still with this software it is best to have a 
dedicated machine only for that purpose. Maybe it is the Windows itself 
and the application is implemented the only possible way, I don't know.

[v3] usb: dwc2: gadget: Add scatter-gather mode

[Minas Harutyunyan]

Hi Andrzej,

On 3/12/2019 3:39 PM, Andrzej Pietrasiewicz wrote:
> Hi Minas,
> 
> 
> W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze:
>> Hi,
>>
>> SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on
>> ep1in. It's first BULK IN transaction after GET MAXLUN.
>>
> Can you help me reproduce the problem? I have an Odroid U3.
1. Download from usb.org USB CV tool. Install on MS Windows.
2. Setup your Odroid U3 as mass storage and connect to MS Windows.
3. Run USB CV. Choose MSC tests to run.

I used "USB 3 Gen X Command Verifier".
> 
>>
>> Meantime it's passing smoke test with mass storage function connected to
>> Linux host.
> 
> So the "real" use case with mass storage works, right?
> 
Yes, it's works. Actually, USB CV use another OS and host driver which 
can be reason of fail, but mass storage should pass all CV MSC tests 
besides smoke tests with Linux.

> Andrzej
> 

Thanks,
Minas

[v3] usb: dwc2: gadget: Add scatter-gather mode

[Minas Harutyunyan]

Hi,

SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on 
ep1in. It's first BULK IN transaction after GET MAXLUN.

[319523.955339] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04088028 00080000 
(d88c3cc4) retry 8
[319523.955357] dwc2 dwc2.1.auto: dwc2_hsotg_irq: daint=00020000
[319523.955366] dwc2 dwc2.1.auto: dwc2_hsotg_epint: ep1(out) 
DxEPINT=0x00000001
[319523.955372] dwc2 dwc2.1.auto: dwc2_hsotg_epint: XferCompl: 
DxEPCTL=0x00098200, DXEPTSIZ=15c7d486
[319523.955377] dwc2 dwc2.1.auto: complete: ep 00000000b58e703a ep1out, 
req 00000000bde78704, 0 => 000000002620ceb8
[319523.955396] dwc2 dwc2.1.auto: ep1in: req 00000000acdadb34: 
36@00000000450496f9, noi=0, zero=0, snok=0
[319523.955403] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: 
DxEPCTL=0x004a8200, ep 1, dir in
[319523.955406] dwc2 dwc2.1.auto: ureq->length:36 ureq->actual:0
[319523.955408] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: 1@36/36, 
0x20080024 => 0x00000930
[319523.955410] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: 36cbc000 pad => 
0x00000934
[319523.955412] dwc2 dwc2.1.auto: ep0 state:0
[319523.955413] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: DxEPCTL=0x844a8200
[319523.955420] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: DXEPCTL=0x80488200
[319523.955425] dwc2 dwc2.1.auto: ep1in: req 00000000f74873bc: 
13@000000007a860fd2, noi=0, zero=0, snok=0
[319523.955446] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04048028 00040000 
(d88c3cc4) retry 8
[319523.955451] dwc2 dwc2.1.auto: dwc2_hsotg_irq: daint=00000002
[319523.955460] dwc2 dwc2.1.auto: dwc2_hsotg_epint: ep1(in) 
DxEPINT=0x00000200
[319523.955462] dwc2 dwc2.1.auto: dwc2_hsotg_epint: BNA interrupt
[319553.987219] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04008428 00000400 
(d88c3cc4) retry 8

Meantime it's passing smoke test with mass storage function connected to 
Linux host.

Thanks,
Minas

[v3] usb: dwc2: gadget: Add scatter-gather mode

[Marek Szyprowski]

From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

This patch adds support for transferring requests, which are
non-contiguous in physical memory, i.e. the data buffer is described by
a scatter-list. This allows transferring large requests without relying
on error-prone contiguous buffer allocations. This way of allocating
requests is already implemented in functionfs and TCM USB functions and
automatically used if UDC driver advertises scatter-gather suppport.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
[mszyprow: fixed null pointer issue, rewrote commit message]
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Andrzej no longer works at Samsung, so I decided to continue this work.

Here are some implementation details, which helps to understand the
proposed changes.

Non-isochronous transfers:
The code for filling a single DDMA entry is extracted from
dwc2_gadget_config_nonisoc_xfer_ddma() as
dwc2_gadget_fill_nonisoc_xfer_ddma_one() function. The original function
now either calls the extracted code (in case of physically contiguous
request buffer) or iterates over scatterlist elements, for each of them
calling the mentioned code.

Isochronous transfers:
If the usb request contains a scatterlist, the address from the first
element is used as dma address at dwc2_gadget_fill_isoc_desc() invocations.
Current code for isoc transfers does not allow more than 4096 bytes per
transfer, so it is assumed there is only one element in the scatterlist.
If there are more, a warning is issued. Please see the fragment under
the comment:

/* In DDMA mode for ISOC's don't queue request if length greater
 * than descriptor limits.
 */

in dwc2_hsotg_ep_queue() to see the limits for isoc transfers.
---
Changelog:
v2..v3:
	- fixed null pointer issue when processing ep0 zlp
	- renamed _dwc2_gadget_config_nonisoc_xfer_ddma() to
	  dwc2_gadget_fill_nonisoc_xfer_ddma_one()
	- rewrote commit message to explain the reason for
	  the proposed changes

v1..v2:
	- using_desc_dma() called to check if DDMA available
---
 drivers/usb/dwc2/gadget.c | 113 ++++++++++++++++++++++++++------------
 1 file changed, 77 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 357beb40b7a3..af419034e438 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -835,22 +835,13 @@ static u32 dwc2_gadget_get_desc_params(struct dwc2_hsotg_ep *hs_ep, u32 *mask)
 	return desc_size;
 }
 
-/*
- * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain.
- * @hs_ep: The endpoint
- * @dma_buff: DMA address to use
- * @len: Length of the transfer
- *
- * This function will iterate over descriptor chain and fill its entries
- * with corresponding information based on transfer data.
- */
-static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep,
+static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep,
+						 struct dwc2_dma_desc **desc,
 						 dma_addr_t dma_buff,
-						 unsigned int len)
+						 unsigned int len,
+						 bool true_last)
 {
-	struct dwc2_hsotg *hsotg = hs_ep->parent;
 	int dir_in = hs_ep->dir_in;
-	struct dwc2_dma_desc *desc = hs_ep->desc_list;
 	u32 mps = hs_ep->ep.maxpacket;
 	u32 maxsize = 0;
 	u32 offset = 0;
@@ -865,39 +856,77 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep,
 		hs_ep->desc_count = 1;
 
 	for (i = 0; i < hs_ep->desc_count; ++i) {
-		desc->status = 0;
-		desc->status |= (DEV_DMA_BUFF_STS_HBUSY
+		(*desc)->status = 0;
+		(*desc)->status |= (DEV_DMA_BUFF_STS_HBUSY
 				 << DEV_DMA_BUFF_STS_SHIFT);
 
 		if (len > maxsize) {
 			if (!hs_ep->index && !dir_in)
-				desc->status |= (DEV_DMA_L | DEV_DMA_IOC);
+				(*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC);
 
-			desc->status |= (maxsize <<
-						DEV_DMA_NBYTES_SHIFT & mask);
-			desc->buf = dma_buff + offset;
+			(*desc)->status |=
+				maxsize << DEV_DMA_NBYTES_SHIFT & mask;
+			(*desc)->buf = dma_buff + offset;
 
 			len -= maxsize;
 			offset += maxsize;
 		} else {
-			desc->status |= (DEV_DMA_L | DEV_DMA_IOC);
+			if (true_last)
+				(*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC);
 
 			if (dir_in)
-				desc->status |= (len % mps) ? DEV_DMA_SHORT :
-					((hs_ep->send_zlp) ? DEV_DMA_SHORT : 0);
-			if (len > maxsize)
-				dev_err(hsotg->dev, "wrong len %d\n", len);
+				(*desc)->status |= (len % mps) ? DEV_DMA_SHORT :
+					((hs_ep->send_zlp && true_last) ?
+					DEV_DMA_SHORT : 0);
 
-			desc->status |=
+			(*desc)->status |=
 				len << DEV_DMA_NBYTES_SHIFT & mask;
-			desc->buf = dma_buff + offset;
+			(*desc)->buf = dma_buff + offset;
 		}
 
-		desc->status &= ~DEV_DMA_BUFF_STS_MASK;
-		desc->status |= (DEV_DMA_BUFF_STS_HREADY
+		(*desc)->status &= ~DEV_DMA_BUFF_STS_MASK;
+		(*desc)->status |= (DEV_DMA_BUFF_STS_HREADY
 				 << DEV_DMA_BUFF_STS_SHIFT);
-		desc++;
+		(*desc)++;
+	}
+}
+
+/*
+ * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain.
+ * @hs_ep: The endpoint
+ * @ureq: Request to transfer
+ * @offset: offset in bytes
+ * @len: Length of the transfer
+ *
+ * This function will iterate over descriptor chain and fill its entries
+ * with corresponding information based on transfer data.
+ */
+static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep,
+						 struct usb_request *ureq,
+						 unsigned int offset,
+						 unsigned int len)
+{
+	struct dwc2_dma_desc *desc = hs_ep->desc_list;
+	struct scatterlist *sg;
+	int i;
+	u8 desc_count = 0;
+
+	/* non-DMA sg buffer */
+	if (!ureq->num_sgs) {
+		dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc,
+			ureq->dma + offset, len, true);
+		return;
 	}
+
+	/* DMA sg buffer */
+	for_each_sg(ureq->sg, sg, ureq->num_sgs, i) {
+		dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc,
+			sg_dma_address(sg) + sg->offset, sg_dma_len(sg),
+			sg_is_last(sg));
+		desc_count += hs_ep->desc_count;
+	}
+
+	hs_ep->desc_count = desc_count;
 }
 
 /*
@@ -1011,7 +1040,13 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep)
 
 	hs_ep->next_desc = 0;
 	list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) {
-		ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma,
+		dma_addr_t dma_addr = hs_req->req.dma;
+
+		if (hs_req->req.num_sgs) {
+			WARN_ON(hs_req->req.num_sgs > 1);
+			dma_addr = sg_dma_address(hs_req->req.sg);
+		}
+		ret = dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr,
 						 hs_req->req.length);
 		if (ret)
 			break;
@@ -1167,7 +1202,7 @@ static void dwc2_hsotg_start_req(struct dwc2_hsotg *hsotg,
 			offset = ureq->actual;
 
 		/* Fill DDMA chain entries */
-		dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq->dma + offset,
+		dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq, offset,
 						     length);
 
 		/* write descriptor chain address to control register */
@@ -1466,7 +1501,13 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req,
 	 */
 	if (using_desc_dma(hs) && hs_ep->isochronous) {
 		if (hs_ep->target_frame != TARGET_FRAME_INITIAL) {
-			dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma,
+			dma_addr_t dma_addr = hs_req->req.dma;
+
+			if (hs_req->req.num_sgs) {
+				WARN_ON(hs_req->req.num_sgs > 1);
+				dma_addr = sg_dma_address(hs_req->req.sg);
+			}
+			dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr,
 						   hs_req->req.length);
 		}
 		return 0;
@@ -2054,13 +2095,12 @@ static void dwc2_hsotg_program_zlp(struct dwc2_hsotg *hsotg,
 		dev_dbg(hsotg->dev, "Receiving zero-length packet on ep%d\n",
 			index);
 	if (using_desc_dma(hsotg)) {
-		/* Not specific buffer needed for ep0 ZLP */
-		dma_addr_t dma = hs_ep->desc_list_dma;
-
 		if (!index)
 			dwc2_gadget_set_ep0_desc_chain(hsotg, hs_ep);
 
-		dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, dma, 0);
+		/* Not specific buffer needed for ep0 ZLP */
+		dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &hs_ep->desc_list,
+			hs_ep->desc_list_dma, 0, true);
 	} else {
 		dwc2_writel(hsotg, DXEPTSIZ_MC(1) | DXEPTSIZ_PKTCNT(1) |
 			    DXEPTSIZ_XFERSIZE(0),
@@ -4471,6 +4511,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget,
 	hsotg->enabled = 0;
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
+	gadget->sg_supported = using_desc_dma(hsotg);
 	dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name);
 
 	return 0;