Re: [PATCH v3 2/2] Staging: octeon-usb: Changes macro CVMX_WAIT_FOR_FIELD32 to function call.

[Aaro Koskinen]

Hi,

On Sun, Jul 29, 2018 at 02:40:35PM +0300, Georgios Tsotsos wrote:
> +/**
> + * Loop through register until txfflsh or rxfflsh become zero.
> + *
> + * @usb:           USB block
> + * @address:       64bit address to read
> + * @timeout_usec:  Timeout
> + * @fflsh_type:    Indicates fflsh type, 0 for txfflsh, 1 for rxfflsh
> + *
> + */
> +static int cvmx_wait_for_field32(struct octeon_hcd *usb, u64 address,
> +				 u64 timeout_usec, int fflsh_type)

You should change the name of the function, as it's no longer generic
"wait for any field" to reach specified condition.

The "address" should be calculated from "usb" inside the function.

The timeout is always 100, you could just omit it.

Nobody is ever checking the result, so it's also redundant...

[...]

> +{
> +	int result;
> +	u64 done = cvmx_get_cycle() + timeout_usec *
> +		   (u64)octeon_get_clock_rate / 1000000;
> +
> +	union cvmx_usbcx_grstctl c;
> +
> +	while (1) {
> +		c.u32 = cvmx_usb_read_csr32(usb, address);
> +		if (fflsh_type == 0 && c.s.txfflsh == 0) {
> +			result = 0;
> +			break;
> +		} else if (fflsh_type == 1 && c.s.rxfflsh == 0) {
> +			result = 0;
> +			break;
> +		} else if (cvmx_get_cycle() > done) {
> +			result = -1;
> +			break;
> +		}
> +
> +		__delay(100);
> +	}
> +	return result;
> +}
> +
>  static void cvmx_fifo_setup(struct octeon_hcd *usb)
>  {
>  	union cvmx_usbcx_ghwcfg3 usbcx_ghwcfg3;
> @@ -635,11 +648,9 @@ static void cvmx_fifo_setup(struct octeon_hcd *usb)
>  	/* Flush all FIFOs */
>  	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, txfnum, 0x10);
>  	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, txfflsh, 1);
> -	CVMX_WAIT_FOR_FIELD32(address, cvmx_usbcx_grstctl,
> -			      c.s.txfflsh == 0, 100);
> +	cvmx_wait_for_field32(usb, address, 0, 100);

How did you test this patch? The parameters are in wrong order, you are
specifying 0 timeout.

>  	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, rxfflsh, 1);
> -	CVMX_WAIT_FOR_FIELD32(address, cvmx_usbcx_grstctl,
> -			      c.s.rxfflsh == 0, 100);
> +	cvmx_wait_for_field32(usb, address, 1, 100);

Also wrong order here.

A.