* [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep
@ 2012-09-13 17:26 Tom Rini
2012-09-13 19:01 ` Kim Phillips
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Tom Rini @ 2012-09-13 17:26 UTC (permalink / raw)
To: u-boot
From: Pankaj Bharadiya <pankaj.bharadiya@ti.com>
The endpoint rx count register value will be zero if it is read before
receive packet ready bit (PERI_RXCSR:RXPKTRDY) is set.
Check for the receive packet ready bit (PERI_RXCSR:RXPKTRDY) before
reading endpoint rx count register. Proceed with rx count read and
FIFO read only if RXPKTRDY bit is set.
As this makes the function fit less-well within 80 columns, use __func__
in some debug statements rather than __PRETTY_FUNCTION__ as they are
identical for C programs.
Signed-off-by: Pankaj Bharadiya <pankaj.bharadiya@ti.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
drivers/usb/musb/musb_udc.c | 97 +++++++++++++++++++++++--------------------
1 file changed, 52 insertions(+), 45 deletions(-)
diff --git a/drivers/usb/musb/musb_udc.c b/drivers/usb/musb/musb_udc.c
index 09cdec3..7180de8 100644
--- a/drivers/usb/musb/musb_udc.c
+++ b/drivers/usb/musb/musb_udc.c
@@ -640,58 +640,65 @@ static void musb_peri_ep0(void)
static void musb_peri_rx_ep(unsigned int ep)
{
- u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
-
- if (peri_rxcount) {
- struct usb_endpoint_instance *endpoint;
- u32 length;
- u8 *data;
-
- endpoint = GET_ENDPOINT(udc_device, ep);
- if (endpoint && endpoint->rcv_urb) {
- struct urb *urb = endpoint->rcv_urb;
- unsigned int remaining_space = urb->buffer_length -
- urb->actual_length;
-
- if (remaining_space) {
- int urb_bad = 0; /* urb is good */
-
- if (peri_rxcount > remaining_space)
- length = remaining_space;
- else
- length = peri_rxcount;
-
- data = (u8 *) urb->buffer_data;
- data += urb->actual_length;
-
- /* The common musb fifo reader */
- read_fifo(ep, length, data);
-
- musb_peri_rx_ack(ep);
-
- /*
- * urb's actual_length is updated in
- * usbd_rcv_complete
- */
- usbd_rcv_complete(endpoint, length, urb_bad);
+ u8 peri_rxcsr = readw(&musbr->ep[ep].epN.rxcsr);
+ if ((peri_rxcsr & MUSB_RXCSR_RXPKTRDY)) {
+ u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
+ if (peri_rxcount) {
+ struct usb_endpoint_instance *endpoint;
+ u32 length;
+ u8 *data;
+ endpoint = GET_ENDPOINT(udc_device, ep);
+ if (endpoint && endpoint->rcv_urb) {
+ struct urb *urb = endpoint->rcv_urb;
+ unsigned int remaining_space =
+ urb->buffer_length -
+ urb->actual_length;
+
+ if (remaining_space) {
+ int urb_bad = 0; /* urb is good */
+
+ if (peri_rxcount > remaining_space)
+ length = remaining_space;
+ else
+ length = peri_rxcount;
+
+ data = (u8 *) urb->buffer_data;
+ data += urb->actual_length;
+
+ /* The common musb fifo reader */
+ read_fifo(ep, length, data);
+
+ musb_peri_rx_ack(ep);
+
+ /*
+ * urb's actual_length is updated in
+ * usbd_rcv_complete
+ */
+ usbd_rcv_complete(endpoint, length,
+ urb_bad);
+
+ } else {
+ if (debug_level > 0)
+ serial_printf("ERROR : %s %d"
+ " no space "
+ "in rcv "
+ "buffer\n",
+ __func__,
+ ep);
+ }
} else {
if (debug_level > 0)
- serial_printf("ERROR : %s %d no space "
- "in rcv buffer\n",
- __PRETTY_FUNCTION__, ep);
+ serial_printf("ERROR : %s %d problem "
+ "with endpoint\n",
+ __func__, ep);
+
}
} else {
if (debug_level > 0)
- serial_printf("ERROR : %s %d problem with "
- "endpoint\n",
- __PRETTY_FUNCTION__, ep);
+ serial_printf("ERROR : %s %d with nothing "
+ "to do\n", __func__, ep);
}
-
- } else {
- if (debug_level > 0)
- serial_printf("ERROR : %s %d with nothing to do\n",
- __PRETTY_FUNCTION__, ep);
}
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep
2012-09-13 17:26 [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep Tom Rini
@ 2012-09-13 19:01 ` Kim Phillips
2012-09-13 19:31 ` Tom Rini
2012-09-14 0:39 ` Marek Vasut
2012-09-14 0:43 ` Marek Vasut
2 siblings, 1 reply; 5+ messages in thread
From: Kim Phillips @ 2012-09-13 19:01 UTC (permalink / raw)
To: u-boot
On Thu, 13 Sep 2012 10:26:31 -0700
Tom Rini <trini@ti.com> wrote:
> + } else {
> + if (debug_level > 0)
> + serial_printf("ERROR : %s %d"
> + " no space "
> + "in rcv "
> + "buffer\n",
> + __func__,
> + ep);
> + }
that's pretty bad.
strings are allowed to exceed the 80 column limit, for grep purposes.
also, if the conditional logic is reversed, one can regain some of
that horizontal space, i.e.:
if (!(peri_rxcsr & MUSB_RXCSR_RXPKTRDY)) {
if (debug_level > 0)
serial_printf("ERROR : %s %d with nothing to do\n",
__PRETTY_FUNCTION__, ep);
return;
}
peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
if (!peri_rxcount)
if (debug_level > 0) {
serial_printf("ERROR : %s %d problem with endpoint\n",
__PRETTY_FUNCTION__, ep);
serial_printf("ERROR : %s %d with nothing to do\n",
__func__, ep);
return;
}
...and so on.
Kim
^ permalink raw reply [flat|nested] 5+ messages in thread* [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep
2012-09-13 19:01 ` Kim Phillips
@ 2012-09-13 19:31 ` Tom Rini
0 siblings, 0 replies; 5+ messages in thread
From: Tom Rini @ 2012-09-13 19:31 UTC (permalink / raw)
To: u-boot
On Thu, Sep 13, 2012 at 02:01:43PM -0500, Kim Phillips wrote:
> On Thu, 13 Sep 2012 10:26:31 -0700
> Tom Rini <trini@ti.com> wrote:
>
> > + } else {
> > + if (debug_level > 0)
> > + serial_printf("ERROR : %s %d"
> > + " no space "
> > + "in rcv "
> > + "buffer\n",
> > + __func__,
> > + ep);
> > + }
>
> that's pretty bad.
Yes, it is.
> strings are allowed to exceed the 80 column limit, for grep purposes.
Good point, just following existing style to the extreme in this case.
> also, if the conditional logic is reversed, one can regain some of
> that horizontal space, i.e.:
An execellent point. I'll post v2 which will debug print and return if
the bit is not set.
--
Tom
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep
2012-09-13 17:26 [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep Tom Rini
2012-09-13 19:01 ` Kim Phillips
@ 2012-09-14 0:39 ` Marek Vasut
2012-09-14 0:43 ` Marek Vasut
2 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2012-09-14 0:39 UTC (permalink / raw)
To: u-boot
Dear Tom Rini,
> From: Pankaj Bharadiya <pankaj.bharadiya@ti.com>
>
> The endpoint rx count register value will be zero if it is read before
> receive packet ready bit (PERI_RXCSR:RXPKTRDY) is set.
>
> Check for the receive packet ready bit (PERI_RXCSR:RXPKTRDY) before
> reading endpoint rx count register. Proceed with rx count read and
> FIFO read only if RXPKTRDY bit is set.
>
> As this makes the function fit less-well within 80 columns, use __func__
> in some debug statements rather than __PRETTY_FUNCTION__ as they are
> identical for C programs.
I'd say this is TI stuff, so let's push it through TI tree (both the musb
patches please).
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep
2012-09-13 17:26 [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep Tom Rini
2012-09-13 19:01 ` Kim Phillips
2012-09-14 0:39 ` Marek Vasut
@ 2012-09-14 0:43 ` Marek Vasut
2 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2012-09-14 0:43 UTC (permalink / raw)
To: u-boot
Dear Tom Rini,
> From: Pankaj Bharadiya <pankaj.bharadiya@ti.com>
>
> The endpoint rx count register value will be zero if it is read before
> receive packet ready bit (PERI_RXCSR:RXPKTRDY) is set.
>
> Check for the receive packet ready bit (PERI_RXCSR:RXPKTRDY) before
> reading endpoint rx count register. Proceed with rx count read and
> FIFO read only if RXPKTRDY bit is set.
>
> As this makes the function fit less-well within 80 columns, use __func__
> in some debug statements rather than __PRETTY_FUNCTION__ as they are
> identical for C programs.
>
> Signed-off-by: Pankaj Bharadiya <pankaj.bharadiya@ti.com>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
> drivers/usb/musb/musb_udc.c | 97
> +++++++++++++++++++++++-------------------- 1 file changed, 52
> insertions(+), 45 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_udc.c b/drivers/usb/musb/musb_udc.c
> index 09cdec3..7180de8 100644
> --- a/drivers/usb/musb/musb_udc.c
> +++ b/drivers/usb/musb/musb_udc.c
> @@ -640,58 +640,65 @@ static void musb_peri_ep0(void)
>
> static void musb_peri_rx_ep(unsigned int ep)
> {
> - u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
> -
> - if (peri_rxcount) {
> - struct usb_endpoint_instance *endpoint;
> - u32 length;
> - u8 *data;
> -
> - endpoint = GET_ENDPOINT(udc_device, ep);
> - if (endpoint && endpoint->rcv_urb) {
> - struct urb *urb = endpoint->rcv_urb;
> - unsigned int remaining_space = urb->buffer_length -
> - urb->actual_length;
> -
> - if (remaining_space) {
> - int urb_bad = 0; /* urb is good */
> -
> - if (peri_rxcount > remaining_space)
> - length = remaining_space;
> - else
> - length = peri_rxcount;
> -
> - data = (u8 *) urb->buffer_data;
> - data += urb->actual_length;
> -
> - /* The common musb fifo reader */
> - read_fifo(ep, length, data);
> -
> - musb_peri_rx_ack(ep);
> -
> - /*
> - * urb's actual_length is updated in
> - * usbd_rcv_complete
> - */
> - usbd_rcv_complete(endpoint, length, urb_bad);
> + u8 peri_rxcsr = readw(&musbr->ep[ep].epN.rxcsr);
> + if ((peri_rxcsr & MUSB_RXCSR_RXPKTRDY)) {
if (!cond)
return;
This will cut down one indent below.
> + u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
> + if (peri_rxcount) {
> + struct usb_endpoint_instance *endpoint;
> + u32 length;
> + u8 *data;
>
> + endpoint = GET_ENDPOINT(udc_device, ep);
> + if (endpoint && endpoint->rcv_urb) {
> + struct urb *urb = endpoint->rcv_urb;
> + unsigned int remaining_space =
> + urb->buffer_length -
> + urb->actual_length;
> +
> + if (remaining_space) {
> + int urb_bad = 0; /* urb is good */
> +
> + if (peri_rxcount > remaining_space)
> + length = remaining_space;
> + else
> + length = peri_rxcount;
> +
> + data = (u8 *) urb->buffer_data;
> + data += urb->actual_length;
> +
> + /* The common musb fifo reader */
> + read_fifo(ep, length, data);
> +
> + musb_peri_rx_ack(ep);
> +
> + /*
> + * urb's actual_length is updated in
> + * usbd_rcv_complete
> + */
> + usbd_rcv_complete(endpoint, length,
> + urb_bad);
> +
> + } else {
> + if (debug_level > 0)
> + serial_printf("ERROR : %s %d"
> + " no space "
> + "in rcv "
> + "buffer\n",
> + __func__,
> + ep);
So ... if (error) print error and die.
if (!remaining_space && debug_level)
print and die.
{ rest of the function, as above }
One more indent level down.
Apply common sense to the other else {} and it's work out.
> + }
> } else {
> if (debug_level > 0)
> - serial_printf("ERROR : %s %d no space "
> - "in rcv buffer\n",
> - __PRETTY_FUNCTION__, ep);
> + serial_printf("ERROR : %s %d problem "
use printf(), serial_printf is flat out forbidden and this is a NACK /!\
> + "with endpoint\n",
> + __func__, ep);
> +
> }
> } else {
> if (debug_level > 0)
> - serial_printf("ERROR : %s %d problem with "
> - "endpoint\n",
> - __PRETTY_FUNCTION__, ep);
> + serial_printf("ERROR : %s %d with nothing "
> + "to do\n", __func__, ep);
> }
> -
> - } else {
> - if (debug_level > 0)
> - serial_printf("ERROR : %s %d with nothing to do\n",
> - __PRETTY_FUNCTION__, ep);
> }
> }
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-14 0:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-13 17:26 [U-Boot] [PATCH] USB: musb_udc: Bugfix musb_peri_rx_ep Tom Rini
2012-09-13 19:01 ` Kim Phillips
2012-09-13 19:31 ` Tom Rini
2012-09-14 0:39 ` Marek Vasut
2012-09-14 0:43 ` Marek Vasut
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox