Patchwork [U-Boot,V4,10/17] usb: gadget: mv_udc: optimize bounce

login
register
mail settings
Submitter Troy Kisky
Date Sept. 20, 2013, 3:29 a.m.
Message ID <1379647780-2623-11-git-send-email-troy.kisky@boundarydevices.com>
Download mbox | patch
Permalink /patch/276188/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Troy Kisky - Sept. 20, 2013, 3:29 a.m.
Only perform one copy, either in the bounce
routine for IN transfers, or the debounce
rtn for OUT transfer.

On out transfers, only copy the number
of bytes received from the bounce buffer

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

---
v4: no change
---
 drivers/usb/gadget/mv_udc.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)
Marek Vasut - Sept. 20, 2013, 10:58 a.m.
Dear Troy Kisky,

> Only perform one copy, either in the bounce
> routine for IN transfers, or the debounce
> rtn for OUT transfer.
> 
> On out transfers, only copy the number
> of bytes received from the bounce buffer
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> ---
> v4: no change

Just a question here. Are you sure we never Send AND Reserve the data in one 
turn? Because that would need two copyings.

Best regards,
Marek Vasut
Troy Kisky - Sept. 20, 2013, 6:58 p.m.
On 9/20/2013 3:58 AM, Marek Vasut wrote:
> Dear Troy Kisky,
>
>> Only perform one copy, either in the bounce
>> routine for IN transfers, or the debounce
>> rtn for OUT transfer.
>>
>> On out transfers, only copy the number
>> of bytes received from the bounce buffer
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>
>> ---
>> v4: no change
> Just a question here. Are you sure we never Send AND Reserve the data in one
> turn? Because that would need two copyings.

  ???   s/Reserve/Receive/

As far as I'm aware, a single buffer is only ever used to capture or 
provide data not both.
But, if 2 transfers were queued, an OUT and then an IN using the same 
buffer, if it worked before
this patch, it should work after as well.
>
> Best regards,
> Marek Vasut
>
Marek Vasut - Sept. 23, 2013, 12:02 a.m.
Dear Troy Kisky,

> On 9/20/2013 3:58 AM, Marek Vasut wrote:
> > Dear Troy Kisky,
> > 
> >> Only perform one copy, either in the bounce
> >> routine for IN transfers, or the debounce
> >> rtn for OUT transfer.
> >> 
> >> On out transfers, only copy the number
> >> of bytes received from the bounce buffer
> >> 
> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >> 
> >> ---
> >> v4: no change
> > 
> > Just a question here. Are you sure we never Send AND Reserve the data in
> > one turn? Because that would need two copyings.
> 
>   ???   s/Reserve/Receive/
> 
> As far as I'm aware, a single buffer is only ever used to capture or
> provide data not both.
> But, if 2 transfers were queued, an OUT and then an IN using the same
> buffer, if it worked before
> this patch, it should work after as well.

How come? Before, it was doing flush before and inval after the transfer, right 
?

btw what does this part of the patch do/mean ? Why is it there?

@@ -387,10 +383,9 @@ static void handle_ep_complete(struct mv_ep *ep)
                       num, in ? "in" : "out", item->info, item->page0);
 
        len = (item->info >> 16) & 0x7fff;
-
-       mv_debounce(ep);
-
        ep->req.length -= len;
+       mv_debounce(ep, in);
+
        DBG("ept%d %s complete %x\n",
                        num, in ? "in" : "out", len);
        ep->req.complete(&ep->ep, &ep->req);

Best regards,
Marek Vasut
Troy Kisky - Sept. 23, 2013, 7:12 p.m.
On 9/22/2013 5:02 PM, Marek Vasut wrote:
> Dear Troy Kisky,
>
>> On 9/20/2013 3:58 AM, Marek Vasut wrote:
>>> Dear Troy Kisky,
>>>
>>>> Only perform one copy, either in the bounce
>>>> routine for IN transfers, or the debounce
>>>> rtn for OUT transfer.
>>>>
>>>> On out transfers, only copy the number
>>>> of bytes received from the bounce buffer
>>>>
>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>>
>>>> ---
>>>> v4: no change
>>> Just a question here. Are you sure we never Send AND Reserve the data in
>>> one turn? Because that would need two copyings.
>>    ???   s/Reserve/Receive/
>>
>> As far as I'm aware, a single buffer is only ever used to capture or
>> provide data not both.
>> But, if 2 transfers were queued, an OUT and then an IN using the same
Actually, I should have said "an IN (tx to the host controller) and then 
an OUT(rx from the host controller)"
>> buffer, if it worked before
>> this patch, it should work after as well.
> How come? Before, it was doing flush before and inval after the transfer, right
> ?

The 1st "IN" transfer (tx to the host), will [copy]/flush on mv_bounce 
and [free]/nothing on mv_debounce.
The 2nd "OUT" transfer (rx from the host) will flush on mv_bounce and 
invalidate/[copy/free] on mv_debounce.

>
> btw what does this part of the patch do/mean ? Why is it there?
>
> @@ -387,10 +383,9 @@ static void handle_ep_complete(struct mv_ep *ep)
>                         num, in ? "in" : "out", item->info, item->page0);
>   
>          len = (item->info >> 16) & 0x7fff;
> -
> -       mv_debounce(ep);
> -
>          ep->req.length -= len;
> +       mv_debounce(ep, in);
> +
>
That implements the "On out transfers, only copy the number of bytes 
received from the bounce buffer"
portion of the commit message.


Thanks
Troy
Marek Vasut - Sept. 23, 2013, 9:23 p.m.
Dear Troy Kisky,

> On 9/22/2013 5:02 PM, Marek Vasut wrote:
> > Dear Troy Kisky,
> > 
> >> On 9/20/2013 3:58 AM, Marek Vasut wrote:
> >>> Dear Troy Kisky,
> >>> 
> >>>> Only perform one copy, either in the bounce
> >>>> routine for IN transfers, or the debounce
> >>>> rtn for OUT transfer.
> >>>> 
> >>>> On out transfers, only copy the number
> >>>> of bytes received from the bounce buffer
> >>>> 
> >>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >>>> 
> >>>> ---
> >>>> v4: no change
> >>> 
> >>> Just a question here. Are you sure we never Send AND Reserve the data
> >>> in one turn? Because that would need two copyings.
> >>> 
> >>    ???   s/Reserve/Receive/
> >> 
> >> As far as I'm aware, a single buffer is only ever used to capture or
> >> provide data not both.
> >> But, if 2 transfers were queued, an OUT and then an IN using the same
> 
> Actually, I should have said "an IN (tx to the host controller) and then
> an OUT(rx from the host controller)"
> 
> >> buffer, if it worked before
> >> this patch, it should work after as well.
> > 
> > How come? Before, it was doing flush before and inval after the transfer,
> > right ?
> 
> The 1st "IN" transfer (tx to the host), will [copy]/flush on mv_bounce
> and [free]/nothing on mv_debounce.
> The 2nd "OUT" transfer (rx from the host) will flush on mv_bounce and
> invalidate/[copy/free] on mv_debounce.
> 
> > btw what does this part of the patch do/mean ? Why is it there?
> > 
> > @@ -387,10 +383,9 @@ static void handle_ep_complete(struct mv_ep *ep)
> > 
> >                         num, in ? "in" : "out", item->info, item->page0);
> >          
> >          len = (item->info >> 16) & 0x7fff;
> > 
> > -
> > -       mv_debounce(ep);
> > -
> > 
> >          ep->req.length -= len;
> > 
> > +       mv_debounce(ep, in);
> > +
> 
> That implements the "On out transfers, only copy the number of bytes
> received from the bounce buffer"
> portion of the commit message.

OK, thanks.

Best regards,
Marek Vasut

Patch

diff --git a/drivers/usb/gadget/mv_udc.c b/drivers/usb/gadget/mv_udc.c
index c2df749..97bab93 100644
--- a/drivers/usb/gadget/mv_udc.c
+++ b/drivers/usb/gadget/mv_udc.c
@@ -264,7 +264,7 @@  static int mv_ep_disable(struct usb_ep *ep)
 	return 0;
 }
 
-static int mv_bounce(struct mv_ep *ep)
+static int mv_bounce(struct mv_ep *ep, int in)
 {
 	uint32_t addr = (uint32_t)ep->req.buf;
 	uint32_t ba;
@@ -293,8 +293,8 @@  align:
 		if (!ep->b_buf)
 			return -ENOMEM;
 	}
-
-	memcpy(ep->b_buf, ep->req.buf, ep->req.length);
+	if (in)
+		memcpy(ep->b_buf, ep->req.buf, ep->req.length);
 
 flush:
 	ba = (uint32_t)ep->b_buf;
@@ -303,29 +303,25 @@  flush:
 	return 0;
 }
 
-static void mv_debounce(struct mv_ep *ep)
+static void mv_debounce(struct mv_ep *ep, int in)
 {
 	uint32_t addr = (uint32_t)ep->req.buf;
 	uint32_t ba = (uint32_t)ep->b_buf;
 
+	if (in) {
+		if (addr == ba)
+			return;		/* not a bounce */
+		goto free;
+	}
 	invalidate_dcache_range(ba, ba + ep->b_len);
 
-	/* Input buffer address is not aligned. */
-	if (addr & (ARCH_DMA_MINALIGN - 1))
-		goto copy;
-
-	/* Input buffer length is not aligned. */
-	if (ep->req.length & (ARCH_DMA_MINALIGN - 1))
-		goto copy;
-
-	/* The buffer is well aligned, only invalidate cache. */
-	return;
+	if (addr == ba)
+		return;		/* not a bounce */
 
-copy:
 	memcpy(ep->req.buf, ep->b_buf, ep->req.length);
-
+free:
 	/* Large payloads use allocated buffer, free it. */
-	if (ep->req.length > 64)
+	if (ep->b_buf != ep->b_fast)
 		free(ep->b_buf);
 }
 
@@ -343,7 +339,7 @@  static int mv_ep_queue(struct usb_ep *ep,
 	head = mv_get_qh(num, in);
 	len = req->length;
 
-	ret = mv_bounce(mv_ep);
+	ret = mv_bounce(mv_ep, in);
 	if (ret)
 		return ret;
 
@@ -387,10 +383,9 @@  static void handle_ep_complete(struct mv_ep *ep)
 		       num, in ? "in" : "out", item->info, item->page0);
 
 	len = (item->info >> 16) & 0x7fff;
-
-	mv_debounce(ep);
-
 	ep->req.length -= len;
+	mv_debounce(ep, in);
+
 	DBG("ept%d %s complete %x\n",
 			num, in ? "in" : "out", len);
 	ep->req.complete(&ep->ep, &ep->req);