[U-Boot] USB: musb_udc: Bugfix musb_peri_rx_ep

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(-)

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

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.

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

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