* [U-Boot] [PATCH 0/3] s3c-otg: fastboot: fixes and cleanup
@ 2016-04-19 7:16 Roger Quadros
2016-04-19 7:16 ` [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic Roger Quadros
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Roger Quadros @ 2016-04-19 7:16 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic
2016-04-19 7:16 [U-Boot] [PATCH 0/3] s3c-otg: fastboot: fixes and cleanup Roger Quadros
@ 2016-04-19 7:16 ` Roger Quadros
2016-04-19 9:00 ` Lukasz Majewski
2016-04-21 10:03 ` Lukasz Majewski
2016-04-19 7:17 ` [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket Roger Quadros
2016-04-19 7:17 ` [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages Roger Quadros
2 siblings, 2 replies; 17+ messages in thread
From: Roger Quadros @ 2016-04-19 7:16 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket
2016-04-19 7:16 [U-Boot] [PATCH 0/3] s3c-otg: fastboot: fixes and cleanup Roger Quadros
2016-04-19 7:16 ` [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic Roger Quadros
@ 2016-04-19 7:17 ` Roger Quadros
2016-04-19 9:02 ` Lukasz Majewski
2016-04-19 12:20 ` [U-Boot] [PATCH v2 " Roger Quadros
2016-04-19 7:17 ` [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages Roger Quadros
2 siblings, 2 replies; 17+ messages in thread
From: Roger Quadros @ 2016-04-19 7:17 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages
2016-04-19 7:16 [U-Boot] [PATCH 0/3] s3c-otg: fastboot: fixes and cleanup Roger Quadros
2016-04-19 7:16 ` [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic Roger Quadros
2016-04-19 7:17 ` [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket Roger Quadros
@ 2016-04-19 7:17 ` Roger Quadros
2016-04-21 10:05 ` Lukasz Majewski
2 siblings, 1 reply; 17+ messages in thread
From: Roger Quadros @ 2016-04-19 7:17 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic
2016-04-19 7:16 ` [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic Roger Quadros
@ 2016-04-19 9:00 ` Lukasz Majewski
2016-04-21 10:03 ` Lukasz Majewski
1 sibling, 0 replies; 17+ messages in thread
From: Lukasz Majewski @ 2016-04-19 9:00 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket
2016-04-19 7:17 ` [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket Roger Quadros
@ 2016-04-19 9:02 ` Lukasz Majewski
2016-04-19 10:21 ` Roger Quadros
2016-04-19 12:20 ` [U-Boot] [PATCH v2 " Roger Quadros
1 sibling, 1 reply; 17+ messages in thread
From: Lukasz Majewski @ 2016-04-19 9:02 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket
2016-04-19 9:02 ` Lukasz Majewski
@ 2016-04-19 10:21 ` Roger Quadros
2016-04-19 11:25 ` Lukasz Majewski
0 siblings, 1 reply; 17+ messages in thread
From: Roger Quadros @ 2016-04-19 10:21 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket
2016-04-19 10:21 ` Roger Quadros
@ 2016-04-19 11:25 ` Lukasz Majewski
2016-04-19 11:52 ` Roger Quadros
0 siblings, 1 reply; 17+ messages in thread
From: Lukasz Majewski @ 2016-04-19 11:25 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket
2016-04-19 11:25 ` Lukasz Majewski
@ 2016-04-19 11:52 ` Roger Quadros
0 siblings, 0 replies; 17+ messages in thread
From: Roger Quadros @ 2016-04-19 11:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH v2 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket
2016-04-19 7:17 ` [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket Roger Quadros
2016-04-19 9:02 ` Lukasz Majewski
@ 2016-04-19 12:20 ` Roger Quadros
2016-04-21 10:04 ` Lukasz Majewski
1 sibling, 1 reply; 17+ messages in thread
From: Roger Quadros @ 2016-04-19 12:20 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic
2016-04-19 7:16 ` [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic Roger Quadros
2016-04-19 9:00 ` Lukasz Majewski
@ 2016-04-21 10:03 ` Lukasz Majewski
2016-04-22 2:59 ` Steve Rae
1 sibling, 1 reply; 17+ messages in thread
From: Lukasz Majewski @ 2016-04-21 10:03 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH v2 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket
2016-04-19 12:20 ` [U-Boot] [PATCH v2 " Roger Quadros
@ 2016-04-21 10:04 ` Lukasz Majewski
0 siblings, 0 replies; 17+ messages in thread
From: Lukasz Majewski @ 2016-04-21 10:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages
2016-04-19 7:17 ` [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages Roger Quadros
@ 2016-04-21 10:05 ` Lukasz Majewski
2016-04-22 2:49 ` Steve Rae
0 siblings, 1 reply; 17+ messages in thread
From: Lukasz Majewski @ 2016-04-21 10:05 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages
2016-04-21 10:05 ` Lukasz Majewski
@ 2016-04-22 2:49 ` Steve Rae
2016-04-22 3:03 ` Steve Rae
0 siblings, 1 reply; 17+ messages in thread
From: Steve Rae @ 2016-04-22 2:49 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic
2016-04-21 10:03 ` Lukasz Majewski
@ 2016-04-22 2:59 ` Steve Rae
2016-04-22 3:52 ` Lukasz Majewski
0 siblings, 1 reply; 17+ messages in thread
From: Steve Rae @ 2016-04-22 2:59 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages
2016-04-22 2:49 ` Steve Rae
@ 2016-04-22 3:03 ` Steve Rae
0 siblings, 0 replies; 17+ messages in thread
From: Steve Rae @ 2016-04-22 3:03 UTC (permalink / raw)
To: u-boot
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
>>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic
2016-04-22 2:59 ` Steve Rae
@ 2016-04-22 3:52 ` Lukasz Majewski
0 siblings, 0 replies; 17+ messages in thread
From: Lukasz Majewski @ 2016-04-22 3:52 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-04-22 3:52 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-19 7:16 [U-Boot] [PATCH 0/3] s3c-otg: fastboot: fixes and cleanup Roger Quadros
2016-04-19 7:16 ` [U-Boot] [PATCH 1/3] fastboot: Clean up bulk-out logic Roger Quadros
2016-04-19 9:00 ` Lukasz Majewski
2016-04-21 10:03 ` Lukasz Majewski
2016-04-22 2:59 ` Steve Rae
2016-04-22 3:52 ` Lukasz Majewski
2016-04-19 7:17 ` [U-Boot] [PATCH 2/3] usb: s3c-otg: Fix short packet for request size > ep.maxpacket Roger Quadros
2016-04-19 9:02 ` Lukasz Majewski
2016-04-19 10:21 ` Roger Quadros
2016-04-19 11:25 ` Lukasz Majewski
2016-04-19 11:52 ` Roger Quadros
2016-04-19 12:20 ` [U-Boot] [PATCH v2 " Roger Quadros
2016-04-21 10:04 ` Lukasz Majewski
2016-04-19 7:17 ` [U-Boot] [PATCH 3/3] usb: s3c-otg: Fix remaining bytes in debug messages Roger Quadros
2016-04-21 10:05 ` Lukasz Majewski
2016-04-22 2:49 ` Steve Rae
2016-04-22 3:03 ` Steve Rae
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.