[U-Boot] [PATCH 0/3] s3c-otg: fastboot: fixes and cleanup

[U-Boot] [PATCH 0/3] s3c-otg: fastboot: fixes and cleanup

[Roger Quadros]

Hi,

These are the patches discussed in the thread [1].
This should fix fastboot on boards using s3c-otg/dwc2 gadget controller.

[1] - https://www.mail-archive.com/u-boot at lists.denx.de/msg209740.html

NOTE: I haven't tested this extensively on TI platforms other than just
a fastboot download command.

--
cheers,
-roger

Roger Quadros (3):
  fastboot: Clean up bulk-out logic
  usb: s3c-otg: Fix short packet for request size > ep.maxpacket
  usb: s3c-otg: Fix remaining bytes in debug messages

 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c |  6 ++--
 drivers/usb/gadget/f_fastboot.c            | 50 ++++++++++++++----------------
 2 files changed, 26 insertions(+), 30 deletions(-)

-- 
2.5.0

[U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic

[Roger Quadros]

Just use ep->maxpacket to get the maxpacket size
and simplify the bulk-out maxpacket alignment.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Tested-by: Steve Rae <srae@broadcom.com>
---
 drivers/usb/gadget/f_fastboot.c | 50 +++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index 7961231..28b244a 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -39,6 +39,11 @@
 #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
 
 #define EP_BUFFER_SIZE			4096
+/*
+ * EP_BUFFER_SIZE must always be an integral multiple of maxpacket size
+ * (64 or 512 or 1024), else we break on certain controllers like DWC3
+ * that expect bulk OUT requests to be divisible by maxpacket size.
+ */
 
 struct f_fastboot {
 	struct usb_function usb_function;
@@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
 static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
-static bool is_high_speed;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
 	.bLength            = USB_DT_ENDPOINT_SIZE,
@@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function *f,
 	debug("%s: func: %s intf: %d alt: %d\n",
 	      __func__, f->name, interface, alt);
 
-	if (gadget->speed == USB_SPEED_HIGH)
-		is_high_speed = true;
-	else
-		is_high_speed = false;
-
 	d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
 	ret = usb_ep_enable(f_fb->out_ep, d);
 	if (ret) {
@@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req)
 	fastboot_tx_write_str(response);
 }
 
-static unsigned int rx_bytes_expected(unsigned int maxpacket)
+static unsigned int rx_bytes_expected(struct usb_ep *ep)
 {
 	int rx_remain = download_size - download_bytes;
-	int rem = 0;
-	if (rx_remain < 0)
+	unsigned int rem;
+	unsigned int maxpacket = ep->maxpacket;
+
+	if (rx_remain <= 0)
 		return 0;
-	if (rx_remain > EP_BUFFER_SIZE)
+	else if (rx_remain > EP_BUFFER_SIZE)
 		return EP_BUFFER_SIZE;
-	if (rx_remain < maxpacket) {
-		rx_remain = maxpacket;
-	} else if (rx_remain % maxpacket != 0) {
-		rem = rx_remain % maxpacket;
+
+	/*
+	 * Some controllers e.g. DWC3 don't like OUT transfers to be
+	 * not ending in maxpacket boundary. So just make them happy by
+	 * always requesting for integral multiple of maxpackets.
+	 * This shouldn't bother controllers that don't care about it.
+	 */
+	rem = rx_remain % maxpacket;
+	if (rem > 0)
 		rx_remain = rx_remain + (maxpacket - rem);
-	}
+
 	return rx_remain;
 }
 
@@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)
 	const unsigned char *buffer = req->buf;
 	unsigned int buffer_size = req->actual;
 	unsigned int pre_dot_num, now_dot_num;
-	unsigned int max;
 
 	if (req->status != 0) {
 		printf("Bad status: %d\n", req->status);
@@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)
 
 		printf("\ndownloading of %d bytes finished\n", download_bytes);
 	} else {
-		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
-				fs_ep_out.wMaxPacketSize;
-		req->length = rx_bytes_expected(max);
-		if (req->length < ep->maxpacket)
-			req->length = ep->maxpacket;
+		req->length = rx_bytes_expected(ep);
 	}
 
 	req->actual = 0;
@@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req)
 {
 	char *cmd = req->buf;
 	char response[FASTBOOT_RESPONSE_LEN];
-	unsigned int max;
 
 	strsep(&cmd, ":");
 	download_size = simple_strtoul(cmd, NULL, 16);
@@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req)
 	} else {
 		sprintf(response, "DATA%08x", download_size);
 		req->complete = rx_handler_dl_image;
-		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
-			fs_ep_out.wMaxPacketSize;
-		req->length = rx_bytes_expected(max);
-		if (req->length < ep->maxpacket)
-			req->length = ep->maxpacket;
+		req->length = rx_bytes_expected(ep);
 	}
 	fastboot_tx_write_str(response);
 }
-- 
2.5.0

[U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket

[Roger Quadros]

Request size can be greater than ep.packet and still end in a
short packet. We need to tackle this case as end of transfer
(if short_not_ok is not set) as indicated in USB 2.0 Specification [1],
else we get stuck up on certain protocols like fastboot.

[1] - USB2.0 Specification, Section 5.3.2 Pipes

Reported-by: Steve Rae <steve.rae@broadcom.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
Tested-by: Steve Rae <steve.rae@broadcom.com>
---
 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
index bce9c30..a31d875 100644
--- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
+++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
@@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8 ep_num)
 				ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
 
 	req->req.actual += min(xfer_size, req->req.length - req->req.actual);
-	is_short = (xfer_size < ep->ep.maxpacket);
+	is_short = xfer_size % ep->ep.maxpacket;
 
 	debug_cond(DEBUG_OUT_EP != 0,
 		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
-- 
2.5.0

[U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages

[Roger Quadros]

Remaining bytes means bytes that are not yet transferred
and not the bytes that were transferred in the last transfer.

Reported-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
index a31d875..0594c3d 100644
--- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
+++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
@@ -235,7 +235,7 @@ static void complete_rx(struct dwc2_udc *dev, u8 ep_num)
 		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
 		   "is_short = %d, DOEPTSIZ = 0x%x, remained bytes = %d\n",
 		   __func__, ep_num, req->req.actual, req->req.length,
-		   is_short, ep_tsr, xfer_size);
+		   is_short, ep_tsr, req->req.length - req->req.actual);
 
 	if (is_short || req->req.actual == req->req.length) {
 		if (ep_num == EP0_CON && dev->ep0state == DATA_STATE_RECV) {
@@ -292,7 +292,7 @@ static void complete_tx(struct dwc2_udc *dev, u8 ep_num)
 		"%s: TX DMA done : ep = %d, tx bytes = %d/%d, "
 		"is_short = %d, DIEPTSIZ = 0x%x, remained bytes = %d\n",
 		__func__, ep_num, req->req.actual, req->req.length,
-		is_short, ep_tsr, xfer_size);
+		is_short, ep_tsr, req->req.length - req->req.actual);
 
 	if (ep_num == 0) {
 		if (dev->ep0state == DATA_STATE_XMIT) {
-- 
2.5.0

[U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic

[Lukasz Majewski]

Hi Roger,

> Just use ep->maxpacket to get the maxpacket size
> and simplify the bulk-out maxpacket alignment.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Tested-by: Steve Rae <srae@broadcom.com>
> ---
>  drivers/usb/gadget/f_fastboot.c | 50
> +++++++++++++++++++---------------------- 1 file changed, 23
> insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_fastboot.c
> b/drivers/usb/gadget/f_fastboot.c index 7961231..28b244a 100644
> --- a/drivers/usb/gadget/f_fastboot.c
> +++ b/drivers/usb/gadget/f_fastboot.c
> @@ -39,6 +39,11 @@
>  #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
>  
>  #define EP_BUFFER_SIZE			4096
> +/*
> + * EP_BUFFER_SIZE must always be an integral multiple of maxpacket
> size
> + * (64 or 512 or 1024), else we break on certain controllers like
> DWC3
> + * that expect bulk OUT requests to be divisible by maxpacket size.
> + */
>  
>  struct f_fastboot {
>  	struct usb_function usb_function;
> @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
>  static unsigned int fastboot_flash_session_id;
>  static unsigned int download_size;
>  static unsigned int download_bytes;
> -static bool is_high_speed;
>  
>  static struct usb_endpoint_descriptor fs_ep_in = {
>  	.bLength            = USB_DT_ENDPOINT_SIZE,
> @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function
> *f, debug("%s: func: %s intf: %d alt: %d\n",
>  	      __func__, f->name, interface, alt);
>  
> -	if (gadget->speed == USB_SPEED_HIGH)
> -		is_high_speed = true;
> -	else
> -		is_high_speed = false;
> -
>  	d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
>  	ret = usb_ep_enable(f_fb->out_ep, d);
>  	if (ret) {
> @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct
> usb_request *req) fastboot_tx_write_str(response);
>  }
>  
> -static unsigned int rx_bytes_expected(unsigned int maxpacket)
> +static unsigned int rx_bytes_expected(struct usb_ep *ep)
>  {
>  	int rx_remain = download_size - download_bytes;
> -	int rem = 0;
> -	if (rx_remain < 0)
> +	unsigned int rem;
> +	unsigned int maxpacket = ep->maxpacket;
> +
> +	if (rx_remain <= 0)
>  		return 0;
> -	if (rx_remain > EP_BUFFER_SIZE)
> +	else if (rx_remain > EP_BUFFER_SIZE)
>  		return EP_BUFFER_SIZE;
> -	if (rx_remain < maxpacket) {
> -		rx_remain = maxpacket;
> -	} else if (rx_remain % maxpacket != 0) {
> -		rem = rx_remain % maxpacket;
> +
> +	/*
> +	 * Some controllers e.g. DWC3 don't like OUT transfers to be
> +	 * not ending in maxpacket boundary. So just make them happy
> by
> +	 * always requesting for integral multiple of maxpackets.
> +	 * This shouldn't bother controllers that don't care about
> it.
> +	 */
> +	rem = rx_remain % maxpacket;
> +	if (rem > 0)
>  		rx_remain = rx_remain + (maxpacket - rem);
> -	}
> +
>  	return rx_remain;
>  }
>  
> @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep
> *ep, struct usb_request *req) const unsigned char *buffer = req->buf;
>  	unsigned int buffer_size = req->actual;
>  	unsigned int pre_dot_num, now_dot_num;
> -	unsigned int max;
>  
>  	if (req->status != 0) {
>  		printf("Bad status: %d\n", req->status);
> @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep
> *ep, struct usb_request *req) 
>  		printf("\ndownloading of %d bytes finished\n",
> download_bytes); } else {
> -		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> -				fs_ep_out.wMaxPacketSize;
> -		req->length = rx_bytes_expected(max);
> -		if (req->length < ep->maxpacket)
> -			req->length = ep->maxpacket;
> +		req->length = rx_bytes_expected(ep);
>  	}
>  
>  	req->actual = 0;
> @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct
> usb_request *req) {
>  	char *cmd = req->buf;
>  	char response[FASTBOOT_RESPONSE_LEN];
> -	unsigned int max;
>  
>  	strsep(&cmd, ":");
>  	download_size = simple_strtoul(cmd, NULL, 16);
> @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep,
> struct usb_request *req) } else {
>  		sprintf(response, "DATA%08x", download_size);
>  		req->complete = rx_handler_dl_image;
> -		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> -			fs_ep_out.wMaxPacketSize;
> -		req->length = rx_bytes_expected(max);
> -		if (req->length < ep->maxpacket)
> -			req->length = ep->maxpacket;
> +		req->length = rx_bytes_expected(ep);
>  	}
>  	fastboot_tx_write_str(response);
>  }

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket

[Lukasz Majewski]

Hi Roger,

> Request size can be greater than ep.packet and still end in a
> short packet. We need to tackle this case as end of transfer
> (if short_not_ok is not set) as indicated in USB 2.0 Specification
> [1], else we get stuck up on certain protocols like fastboot.
> 
> [1] - USB2.0 Specification, Section 5.3.2 Pipes
> 
> Reported-by: Steve Rae <steve.rae@broadcom.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Tested-by: Steve Rae <steve.rae@broadcom.com>
> ---
>  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..a31d875
> 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> @@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
> ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
>  
>  	req->req.actual += min(xfer_size, req->req.length -
> req->req.actual);
> -	is_short = (xfer_size < ep->ep.maxpacket);
> +	is_short = xfer_size % ep->ep.maxpacket;

is_short is a flag - so maybe it would be better to write something
like:

is_short = !!(xfer_size % ep->ep.maxpacket)) ?

I'm going to test those patches on my boards. I will share the results
ASAP.

>  
>  	debug_cond(DEBUG_OUT_EP != 0,
>  		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket

[Roger Quadros]

Hi Lukasz,

On 19/04/16 12:02, Lukasz Majewski wrote:
> Hi Roger,
> 
>> Request size can be greater than ep.packet and still end in a
>> short packet. We need to tackle this case as end of transfer
>> (if short_not_ok is not set) as indicated in USB 2.0 Specification
>> [1], else we get stuck up on certain protocols like fastboot.
>>
>> [1] - USB2.0 Specification, Section 5.3.2 Pipes
>>
>> Reported-by: Steve Rae <steve.rae@broadcom.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Tested-by: Steve Rae <steve.rae@broadcom.com>
>> ---
>>  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..a31d875
>> 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> @@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
>> ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
>>  
>>  	req->req.actual += min(xfer_size, req->req.length -
>> req->req.actual);
>> -	is_short = (xfer_size < ep->ep.maxpacket);
>> +	is_short = xfer_size % ep->ep.maxpacket;
> 
> is_short is a flag - so maybe it would be better to write something
> like:

but it is defined as u32 so I thought it might as well print the
short packet length instead of just 1/0.

> 
> is_short = !!(xfer_size % ep->ep.maxpacket)) ?
> 
> I'm going to test those patches on my boards. I will share the results
> ASAP.
> 
>>  
>>  	debug_cond(DEBUG_OUT_EP != 0,
>>  		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
> 
> 
> 

--
cheers,
-roger

[U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket

[Lukasz Majewski]

Hi Roger,

> Hi Lukasz,
> 
> On 19/04/16 12:02, Lukasz Majewski wrote:
> > Hi Roger,
> > 
> >> Request size can be greater than ep.packet and still end in a
> >> short packet. We need to tackle this case as end of transfer
> >> (if short_not_ok is not set) as indicated in USB 2.0 Specification
> >> [1], else we get stuck up on certain protocols like fastboot.
> >>
> >> [1] - USB2.0 Specification, Section 5.3.2 Pipes
> >>
> >> Reported-by: Steve Rae <steve.rae@broadcom.com>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> Tested-by: Steve Rae <steve.rae@broadcom.com>
> >> ---
> >>  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> >> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..a31d875
> >> 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> >> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> >> @@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev,
> >> u8 ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
> >>  
> >>  	req->req.actual += min(xfer_size, req->req.length -
> >> req->req.actual);
> >> -	is_short = (xfer_size < ep->ep.maxpacket);
> >> +	is_short = xfer_size % ep->ep.maxpacket;
> > 
> > is_short is a flag - so maybe it would be better to write something
> > like:
> 
> but it is defined as u32 so I thought it might as well print the
> short packet length instead of just 1/0.

I mean that it looks strange for me in debug message when one see:

"is_short: 8" (as it was shown at Steve's output).


> 
> > 
> > is_short = !!(xfer_size % ep->ep.maxpacket)) ?
> > 
> > I'm going to test those patches on my boards. I will share the
> > results ASAP.
> > 
> >>  
> >>  	debug_cond(DEBUG_OUT_EP != 0,
> >>  		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
> > 
> > 
> > 
> 
> --
> cheers,
> -roger
> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket

[Roger Quadros]

On 19/04/16 14:25, Lukasz Majewski wrote:
> Hi Roger,
> 
>> Hi Lukasz,
>>
>> On 19/04/16 12:02, Lukasz Majewski wrote:
>>> Hi Roger,
>>>
>>>> Request size can be greater than ep.packet and still end in a
>>>> short packet. We need to tackle this case as end of transfer
>>>> (if short_not_ok is not set) as indicated in USB 2.0 Specification
>>>> [1], else we get stuck up on certain protocols like fastboot.
>>>>
>>>> [1] - USB2.0 Specification, Section 5.3.2 Pipes
>>>>
>>>> Reported-by: Steve Rae <steve.rae@broadcom.com>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Tested-by: Steve Rae <steve.rae@broadcom.com>
>>>> ---
>>>>  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..a31d875
>>>> 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>>>> @@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev,
>>>> u8 ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
>>>>  
>>>>  	req->req.actual += min(xfer_size, req->req.length -
>>>> req->req.actual);
>>>> -	is_short = (xfer_size < ep->ep.maxpacket);
>>>> +	is_short = xfer_size % ep->ep.maxpacket;
>>>
>>> is_short is a flag - so maybe it would be better to write something
>>> like:
>>
>> but it is defined as u32 so I thought it might as well print the
>> short packet length instead of just 1/0.
> 
> I mean that it looks strange for me in debug message when one see:
> 
> "is_short: 8" (as it was shown at Steve's output).

Agreed. I'll convert it to boolean then.

--
cheers,
-roger

[U-Boot] [PATCH v2 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket

[Roger Quadros]

Request size can be greater than ep.packet and still end in a
short packet. We need to tackle this case as end of transfer
(if short_not_ok is not set) as indicated in USB 2.0 Specification [1],
else we get stuck up on certain protocols like fastboot.

[1] - USB2.0 Specification, Section 5.3.2 Pipes

Reported-by: Steve Rae <steve.rae@broadcom.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
Tested-by: Steve Rae <steve.rae@broadcom.com>
---
 drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
index bce9c30..9aa0f33 100644
--- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
+++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
@@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8 ep_num)
 				ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
 
 	req->req.actual += min(xfer_size, req->req.length - req->req.actual);
-	is_short = (xfer_size < ep->ep.maxpacket);
+	is_short = !!(xfer_size % ep->ep.maxpacket);
 
 	debug_cond(DEBUG_OUT_EP != 0,
 		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
-- 
2.5.0

[U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic

[Lukasz Majewski]

Hi Roger,

> Just use ep->maxpacket to get the maxpacket size
> and simplify the bulk-out maxpacket alignment.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Tested-by: Steve Rae <srae@broadcom.com>
> ---
>  drivers/usb/gadget/f_fastboot.c | 50
> +++++++++++++++++++---------------------- 1 file changed, 23
> insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_fastboot.c
> b/drivers/usb/gadget/f_fastboot.c index 7961231..28b244a 100644
> --- a/drivers/usb/gadget/f_fastboot.c
> +++ b/drivers/usb/gadget/f_fastboot.c
> @@ -39,6 +39,11 @@
>  #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
>  
>  #define EP_BUFFER_SIZE			4096
> +/*
> + * EP_BUFFER_SIZE must always be an integral multiple of maxpacket
> size
> + * (64 or 512 or 1024), else we break on certain controllers like
> DWC3
> + * that expect bulk OUT requests to be divisible by maxpacket size.
> + */
>  
>  struct f_fastboot {
>  	struct usb_function usb_function;
> @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
>  static unsigned int fastboot_flash_session_id;
>  static unsigned int download_size;
>  static unsigned int download_bytes;
> -static bool is_high_speed;
>  
>  static struct usb_endpoint_descriptor fs_ep_in = {
>  	.bLength            = USB_DT_ENDPOINT_SIZE,
> @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function
> *f, debug("%s: func: %s intf: %d alt: %d\n",
>  	      __func__, f->name, interface, alt);
>  
> -	if (gadget->speed == USB_SPEED_HIGH)
> -		is_high_speed = true;
> -	else
> -		is_high_speed = false;
> -
>  	d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
>  	ret = usb_ep_enable(f_fb->out_ep, d);
>  	if (ret) {
> @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct
> usb_request *req) fastboot_tx_write_str(response);
>  }
>  
> -static unsigned int rx_bytes_expected(unsigned int maxpacket)
> +static unsigned int rx_bytes_expected(struct usb_ep *ep)
>  {
>  	int rx_remain = download_size - download_bytes;
> -	int rem = 0;
> -	if (rx_remain < 0)
> +	unsigned int rem;
> +	unsigned int maxpacket = ep->maxpacket;
> +
> +	if (rx_remain <= 0)
>  		return 0;
> -	if (rx_remain > EP_BUFFER_SIZE)
> +	else if (rx_remain > EP_BUFFER_SIZE)
>  		return EP_BUFFER_SIZE;
> -	if (rx_remain < maxpacket) {
> -		rx_remain = maxpacket;
> -	} else if (rx_remain % maxpacket != 0) {
> -		rem = rx_remain % maxpacket;
> +
> +	/*
> +	 * Some controllers e.g. DWC3 don't like OUT transfers to be
> +	 * not ending in maxpacket boundary. So just make them happy
> by
> +	 * always requesting for integral multiple of maxpackets.
> +	 * This shouldn't bother controllers that don't care about
> it.
> +	 */
> +	rem = rx_remain % maxpacket;
> +	if (rem > 0)
>  		rx_remain = rx_remain + (maxpacket - rem);
> -	}
> +
>  	return rx_remain;
>  }
>  
> @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep
> *ep, struct usb_request *req) const unsigned char *buffer = req->buf;
>  	unsigned int buffer_size = req->actual;
>  	unsigned int pre_dot_num, now_dot_num;
> -	unsigned int max;
>  
>  	if (req->status != 0) {
>  		printf("Bad status: %d\n", req->status);
> @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep
> *ep, struct usb_request *req) 
>  		printf("\ndownloading of %d bytes finished\n",
> download_bytes); } else {
> -		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> -				fs_ep_out.wMaxPacketSize;
> -		req->length = rx_bytes_expected(max);
> -		if (req->length < ep->maxpacket)
> -			req->length = ep->maxpacket;
> +		req->length = rx_bytes_expected(ep);
>  	}
>  
>  	req->actual = 0;
> @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct
> usb_request *req) {
>  	char *cmd = req->buf;
>  	char response[FASTBOOT_RESPONSE_LEN];
> -	unsigned int max;
>  
>  	strsep(&cmd, ":");
>  	download_size = simple_strtoul(cmd, NULL, 16);
> @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep,
> struct usb_request *req) } else {
>  		sprintf(response, "DATA%08x", download_size);
>  		req->complete = rx_handler_dl_image;
> -		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> -			fs_ep_out.wMaxPacketSize;
> -		req->length = rx_bytes_expected(max);
> -		if (req->length < ep->maxpacket)
> -			req->length = ep->maxpacket;
> +		req->length = rx_bytes_expected(ep);
>  	}
>  	fastboot_tx_write_str(response);
>  }

I cannot apply this patch on temporary u-boot-usb/master branch
(SHA1: e6c0bc0643e5a4387fecbcf83080d0b796eb067c).

Could you apply your changes (from this patch) on top of newest
u-boot-usb repo?

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v2 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket

[Lukasz Majewski]

Hi Roger,

> Request size can be greater than ep.packet and still end in a
> short packet. We need to tackle this case as end of transfer
> (if short_not_ok is not set) as indicated in USB 2.0 Specification
> [1], else we get stuck up on certain protocols like fastboot.
> 
> [1] - USB2.0 Specification, Section 5.3.2 Pipes
> 
> Reported-by: Steve Rae <steve.rae@broadcom.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Tested-by: Steve Rae <steve.rae@broadcom.com>
> ---
>  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..9aa0f33
> 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> @@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
> ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
>  
>  	req->req.actual += min(xfer_size, req->req.length -
> req->req.actual);
> -	is_short = (xfer_size < ep->ep.maxpacket);
> +	is_short = !!(xfer_size % ep->ep.maxpacket);
>  
>  	debug_cond(DEBUG_OUT_EP != 0,
>  		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "

Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Test HW: Odroid U3 (Exynos 4412)

Test: ./test/py DFU & UMS

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages

[Lukasz Majewski]

Hi Roger,

> Remaining bytes means bytes that are not yet transferred
> and not the bytes that were transferred in the last transfer.
> 
> Reported-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index a31d875..0594c3d
> 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> @@ -235,7 +235,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
> ep_num) "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
>  		   "is_short = %d, DOEPTSIZ = 0x%x, remained bytes =
> %d\n", __func__, ep_num, req->req.actual, req->req.length,
> -		   is_short, ep_tsr, xfer_size);
> +		   is_short, ep_tsr, req->req.length -
> req->req.actual); 
>  	if (is_short || req->req.actual == req->req.length) {
>  		if (ep_num == EP0_CON && dev->ep0state ==
> DATA_STATE_RECV) { @@ -292,7 +292,7 @@ static void complete_tx(struct
> dwc2_udc *dev, u8 ep_num) "%s: TX DMA done : ep = %d, tx bytes =
> %d/%d, " "is_short = %d, DIEPTSIZ = 0x%x, remained bytes = %d\n",
>  		__func__, ep_num, req->req.actual, req->req.length,
> -		is_short, ep_tsr, xfer_size);
> +		is_short, ep_tsr, req->req.length - req->req.actual);
>  
>  	if (ep_num == 0) {
>  		if (dev->ep0state == DATA_STATE_XMIT) {

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages

[Steve Rae]

On Thu, Apr 21, 2016 at 3:05 AM, Lukasz Majewski <l.majewski@samsung.com>
wrote:

> Hi Roger,
>
> > Remaining bytes means bytes that are not yet transferred
> > and not the bytes that were transferred in the last transfer.
> >
> > Reported-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > ---
> >  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> > b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index a31d875..0594c3d
> > 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> > +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
> > @@ -235,7 +235,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
> > ep_num) "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
> >                  "is_short = %d, DOEPTSIZ = 0x%x, remained bytes =
> > %d\n", __func__, ep_num, req->req.actual, req->req.length,
> > -                is_short, ep_tsr, xfer_size);
> > +                is_short, ep_tsr, req->req.length -
> > req->req.actual);
> >       if (is_short || req->req.actual == req->req.length) {
> >               if (ep_num == EP0_CON && dev->ep0state ==
> > DATA_STATE_RECV) { @@ -292,7 +292,7 @@ static void complete_tx(struct
> > dwc2_udc *dev, u8 ep_num) "%s: TX DMA done : ep = %d, tx bytes =
> > %d/%d, " "is_short = %d, DIEPTSIZ = 0x%x, remained bytes = %d\n",
> >               __func__, ep_num, req->req.actual, req->req.length,
> > -             is_short, ep_tsr, xfer_size);
> > +             is_short, ep_tsr, req->req.length - req->req.actual);
> >
> >       if (ep_num == 0) {
> >               if (dev->ep0state == DATA_STATE_XMIT) {
>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>
> Tested-by: Steve Rae <srae@broadcom.com>
[Test HW: bcm28155_ap board]

Thanks, Steve


> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>

[U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic

[Steve Rae]

On Thu, Apr 21, 2016 at 3:03 AM, Lukasz Majewski <l.majewski@samsung.com>
wrote:

> Hi Roger,
>
> > Just use ep->maxpacket to get the maxpacket size
> > and simplify the bulk-out maxpacket alignment.
> >
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > Tested-by: Steve Rae <srae@broadcom.com>
> > ---
> >  drivers/usb/gadget/f_fastboot.c | 50
> > +++++++++++++++++++---------------------- 1 file changed, 23
> > insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/f_fastboot.c
> > b/drivers/usb/gadget/f_fastboot.c index 7961231..28b244a 100644
> > --- a/drivers/usb/gadget/f_fastboot.c
> > +++ b/drivers/usb/gadget/f_fastboot.c
> > @@ -39,6 +39,11 @@
> >  #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
> >
> >  #define EP_BUFFER_SIZE                       4096
> > +/*
> > + * EP_BUFFER_SIZE must always be an integral multiple of maxpacket
> > size
> > + * (64 or 512 or 1024), else we break on certain controllers like
> > DWC3
> > + * that expect bulk OUT requests to be divisible by maxpacket size.
> > + */
> >
> >  struct f_fastboot {
> >       struct usb_function usb_function;
> > @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
> >  static unsigned int fastboot_flash_session_id;
> >  static unsigned int download_size;
> >  static unsigned int download_bytes;
> > -static bool is_high_speed;
> >
> >  static struct usb_endpoint_descriptor fs_ep_in = {
> >       .bLength            = USB_DT_ENDPOINT_SIZE,
> > @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function
> > *f, debug("%s: func: %s intf: %d alt: %d\n",
> >             __func__, f->name, interface, alt);
> >
> > -     if (gadget->speed == USB_SPEED_HIGH)
> > -             is_high_speed = true;
> > -     else
> > -             is_high_speed = false;
> > -
> >       d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
> >       ret = usb_ep_enable(f_fb->out_ep, d);
> >       if (ret) {
> > @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct
> > usb_request *req) fastboot_tx_write_str(response);
> >  }
> >
> > -static unsigned int rx_bytes_expected(unsigned int maxpacket)
> > +static unsigned int rx_bytes_expected(struct usb_ep *ep)
> >  {
> >       int rx_remain = download_size - download_bytes;
> > -     int rem = 0;
> > -     if (rx_remain < 0)
> > +     unsigned int rem;
> > +     unsigned int maxpacket = ep->maxpacket;
> > +
> > +     if (rx_remain <= 0)
> >               return 0;
> > -     if (rx_remain > EP_BUFFER_SIZE)
> > +     else if (rx_remain > EP_BUFFER_SIZE)
> >               return EP_BUFFER_SIZE;
> > -     if (rx_remain < maxpacket) {
> > -             rx_remain = maxpacket;
> > -     } else if (rx_remain % maxpacket != 0) {
> > -             rem = rx_remain % maxpacket;
> > +
> > +     /*
> > +      * Some controllers e.g. DWC3 don't like OUT transfers to be
> > +      * not ending in maxpacket boundary. So just make them happy
> > by
> > +      * always requesting for integral multiple of maxpackets.
> > +      * This shouldn't bother controllers that don't care about
> > it.
> > +      */
> > +     rem = rx_remain % maxpacket;
> > +     if (rem > 0)
> >               rx_remain = rx_remain + (maxpacket - rem);
> > -     }
> > +
> >       return rx_remain;
> >  }
> >
> > @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep
> > *ep, struct usb_request *req) const unsigned char *buffer = req->buf;
> >       unsigned int buffer_size = req->actual;
> >       unsigned int pre_dot_num, now_dot_num;
> > -     unsigned int max;
> >
> >       if (req->status != 0) {
> >               printf("Bad status: %d\n", req->status);
> > @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep
> > *ep, struct usb_request *req)
> >               printf("\ndownloading of %d bytes finished\n",
> > download_bytes); } else {
> > -             max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> > -                             fs_ep_out.wMaxPacketSize;
> > -             req->length = rx_bytes_expected(max);
> > -             if (req->length < ep->maxpacket)
> > -                     req->length = ep->maxpacket;
> > +             req->length = rx_bytes_expected(ep);
> >       }
> >
> >       req->actual = 0;
> > @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct
> > usb_request *req) {
> >       char *cmd = req->buf;
> >       char response[FASTBOOT_RESPONSE_LEN];
> > -     unsigned int max;
> >
> >       strsep(&cmd, ":");
> >       download_size = simple_strtoul(cmd, NULL, 16);
> > @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep,
> > struct usb_request *req) } else {
> >               sprintf(response, "DATA%08x", download_size);
> >               req->complete = rx_handler_dl_image;
> > -             max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> > -                     fs_ep_out.wMaxPacketSize;
> > -             req->length = rx_bytes_expected(max);
> > -             if (req->length < ep->maxpacket)
> > -                     req->length = ep->maxpacket;
> > +             req->length = rx_bytes_expected(ep);
> >       }
> >       fastboot_tx_write_str(response);
> >  }
>
> I cannot apply this patch on temporary u-boot-usb/master branch
> (SHA1: e6c0bc0643e5a4387fecbcf83080d0b796eb067c).
>
> Could you apply your changes (from this patch) on top of newest
> u-boot-usb repo?
>
> Lukasz, (in case Roger doesn't reply soon....)
-- this needs to be applied on top of his earlier patches:
(1) http://patchwork.ozlabs.org/patch/609417/
        [U-Boot,1/2] fastboot: Fix wMaxPacketSize for High-Speed IN endpoint
(2) http://patchwork.ozlabs.org/patch/609896/
        [U-Boot,v2,2/2] fastboot: Enable the respective speed endpoints at
runtime

... then this one will apply (http://patchwork.ozlabs.org/patch/612006/)

Thanks, Steve

--
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>

[U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages

[Steve Rae]

On Thu, Apr 21, 2016 at 7:49 PM, Steve Rae <steve.rae@broadcom.com> wrote:

>
>
> On Thu, Apr 21, 2016 at 3:05 AM, Lukasz Majewski <l.majewski@samsung.com>
> wrote:
>
>> Hi Roger,
>>
>> > Remaining bytes means bytes that are not yet transferred
>> > and not the bytes that were transferred in the last transfer.
>> >
>> > Reported-by: Lukasz Majewski <l.majewski@samsung.com>
>> > Signed-off-by: Roger Quadros <rogerq@ti.com>
>> > ---
>> >  drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> > b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index a31d875..0594c3d
>> > 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> > +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
>> > @@ -235,7 +235,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
>> > ep_num) "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
>> >                  "is_short = %d, DOEPTSIZ = 0x%x, remained bytes =
>> > %d\n", __func__, ep_num, req->req.actual, req->req.length,
>> > -                is_short, ep_tsr, xfer_size);
>> > +                is_short, ep_tsr, req->req.length -
>> > req->req.actual);
>> >       if (is_short || req->req.actual == req->req.length) {
>> >               if (ep_num == EP0_CON && dev->ep0state ==
>> > DATA_STATE_RECV) { @@ -292,7 +292,7 @@ static void complete_tx(struct
>> > dwc2_udc *dev, u8 ep_num) "%s: TX DMA done : ep = %d, tx bytes =
>> > %d/%d, " "is_short = %d, DIEPTSIZ = 0x%x, remained bytes = %d\n",
>> >               __func__, ep_num, req->req.actual, req->req.length,
>> > -             is_short, ep_tsr, xfer_size);
>> > +             is_short, ep_tsr, req->req.length - req->req.actual);
>> >
>> >       if (ep_num == 0) {
>> >               if (dev->ep0state == DATA_STATE_XMIT) {
>>
>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>
>> Tested-by: Steve Rae <srae@broadcom.com>
> [Test HW: bcm28155_ap board]
>
> Thanks, Steve
>

retry because patchworks missed this....

Tested-by: Steve Rae <srae@broadcom.com>
[Test HW: bcm28155_ap board]



>
>
>> --
>> Best regards,
>>
>> Lukasz Majewski
>>
>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>>
>
>

[U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic

[Lukasz Majewski]

On Thu, 21 Apr 2016 19:59:21 -0700
Steve Rae <steve.rae@broadcom.com> wrote:

> On Thu, Apr 21, 2016 at 3:03 AM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> 
> > Hi Roger,
> >
> > > Just use ep->maxpacket to get the maxpacket size
> > > and simplify the bulk-out maxpacket alignment.
> > >
> > > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > > Tested-by: Steve Rae <srae@broadcom.com>
> > > ---
> > >  drivers/usb/gadget/f_fastboot.c | 50
> > > +++++++++++++++++++---------------------- 1 file changed, 23
> > > insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/f_fastboot.c
> > > b/drivers/usb/gadget/f_fastboot.c index 7961231..28b244a 100644
> > > --- a/drivers/usb/gadget/f_fastboot.c
> > > +++ b/drivers/usb/gadget/f_fastboot.c
> > > @@ -39,6 +39,11 @@
> > >  #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
> > >
> > >  #define EP_BUFFER_SIZE                       4096
> > > +/*
> > > + * EP_BUFFER_SIZE must always be an integral multiple of
> > > maxpacket size
> > > + * (64 or 512 or 1024), else we break on certain controllers like
> > > DWC3
> > > + * that expect bulk OUT requests to be divisible by maxpacket
> > > size.
> > > + */
> > >
> > >  struct f_fastboot {
> > >       struct usb_function usb_function;
> > > @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
> > >  static unsigned int fastboot_flash_session_id;
> > >  static unsigned int download_size;
> > >  static unsigned int download_bytes;
> > > -static bool is_high_speed;
> > >
> > >  static struct usb_endpoint_descriptor fs_ep_in = {
> > >       .bLength            = USB_DT_ENDPOINT_SIZE,
> > > @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct
> > > usb_function *f, debug("%s: func: %s intf: %d alt: %d\n",
> > >             __func__, f->name, interface, alt);
> > >
> > > -     if (gadget->speed == USB_SPEED_HIGH)
> > > -             is_high_speed = true;
> > > -     else
> > > -             is_high_speed = false;
> > > -
> > >       d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
> > >       ret = usb_ep_enable(f_fb->out_ep, d);
> > >       if (ret) {
> > > @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep,
> > > struct usb_request *req) fastboot_tx_write_str(response);
> > >  }
> > >
> > > -static unsigned int rx_bytes_expected(unsigned int maxpacket)
> > > +static unsigned int rx_bytes_expected(struct usb_ep *ep)
> > >  {
> > >       int rx_remain = download_size - download_bytes;
> > > -     int rem = 0;
> > > -     if (rx_remain < 0)
> > > +     unsigned int rem;
> > > +     unsigned int maxpacket = ep->maxpacket;
> > > +
> > > +     if (rx_remain <= 0)
> > >               return 0;
> > > -     if (rx_remain > EP_BUFFER_SIZE)
> > > +     else if (rx_remain > EP_BUFFER_SIZE)
> > >               return EP_BUFFER_SIZE;
> > > -     if (rx_remain < maxpacket) {
> > > -             rx_remain = maxpacket;
> > > -     } else if (rx_remain % maxpacket != 0) {
> > > -             rem = rx_remain % maxpacket;
> > > +
> > > +     /*
> > > +      * Some controllers e.g. DWC3 don't like OUT transfers to be
> > > +      * not ending in maxpacket boundary. So just make them happy
> > > by
> > > +      * always requesting for integral multiple of maxpackets.
> > > +      * This shouldn't bother controllers that don't care about
> > > it.
> > > +      */
> > > +     rem = rx_remain % maxpacket;
> > > +     if (rem > 0)
> > >               rx_remain = rx_remain + (maxpacket - rem);
> > > -     }
> > > +
> > >       return rx_remain;
> > >  }
> > >
> > > @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep
> > > *ep, struct usb_request *req) const unsigned char *buffer =
> > > req->buf; unsigned int buffer_size = req->actual;
> > >       unsigned int pre_dot_num, now_dot_num;
> > > -     unsigned int max;
> > >
> > >       if (req->status != 0) {
> > >               printf("Bad status: %d\n", req->status);
> > > @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep
> > > *ep, struct usb_request *req)
> > >               printf("\ndownloading of %d bytes finished\n",
> > > download_bytes); } else {
> > > -             max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> > > -                             fs_ep_out.wMaxPacketSize;
> > > -             req->length = rx_bytes_expected(max);
> > > -             if (req->length < ep->maxpacket)
> > > -                     req->length = ep->maxpacket;
> > > +             req->length = rx_bytes_expected(ep);
> > >       }
> > >
> > >       req->actual = 0;
> > > @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep,
> > > struct usb_request *req) {
> > >       char *cmd = req->buf;
> > >       char response[FASTBOOT_RESPONSE_LEN];
> > > -     unsigned int max;
> > >
> > >       strsep(&cmd, ":");
> > >       download_size = simple_strtoul(cmd, NULL, 16);
> > > @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep,
> > > struct usb_request *req) } else {
> > >               sprintf(response, "DATA%08x", download_size);
> > >               req->complete = rx_handler_dl_image;
> > > -             max = is_high_speed ? hs_ep_out.wMaxPacketSize :
> > > -                     fs_ep_out.wMaxPacketSize;
> > > -             req->length = rx_bytes_expected(max);
> > > -             if (req->length < ep->maxpacket)
> > > -                     req->length = ep->maxpacket;
> > > +             req->length = rx_bytes_expected(ep);
> > >       }
> > >       fastboot_tx_write_str(response);
> > >  }
> >
> > I cannot apply this patch on temporary u-boot-usb/master branch
> > (SHA1: e6c0bc0643e5a4387fecbcf83080d0b796eb067c).
> >
> > Could you apply your changes (from this patch) on top of newest
> > u-boot-usb repo?
> >
> > Lukasz, (in case Roger doesn't reply soon....)
> -- this needs to be applied on top of his earlier patches:
> (1) http://patchwork.ozlabs.org/patch/609417/
>         [U-Boot,1/2] fastboot: Fix wMaxPacketSize for High-Speed IN
> endpoint (2) http://patchwork.ozlabs.org/patch/609896/
>         [U-Boot,v2,2/2] fastboot: Enable the respective speed
> endpoints at runtime
> 
> ... then this one will apply
> (http://patchwork.ozlabs.org/patch/612006/)

Steve, thanks for tip. I was missing this information :-)

Best regards,
?ukasz

> 
> Thanks, Steve
> 
> --
> > Best regards,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> >
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160422/fc936c89/attachment.sig>