[PATCH v2 0/3] Staging: octeon-usb fixes for coding style, SPDX and readability.

[PATCH v2 0/3] Staging: octeon-usb fixes for coding style, SPDX and readability.

[Georgios Tsotsos]

Hello, 
Ok, this have gone too far, i sent these series so many times wrong...
I am sorry its my first patch (and series) and i keep mix-up patch generation
an sending to my self. Last time not everybody was cc.

Here are three patches which trying to resolve TODO's list requirements 
number 45 about octeon-usb. 
There are SPDX licence additions on code, makefile and header files.
Checkpatch warnings are resolved,also a notice about CVMX_WAIT_FOR_FIELD32 macro.
After Joe Perches's (joe@perches.com) suggestion i broke down the 
cvmx_usb_poll_channel function in order to improve readability and keeping it
under 80 columns.

Georgios Tsotsos (3):
  Staging: octeon-usb: Adding SPDX license identifier
  Staging: octeon-usb: Change coding style of CVMX_WAIT_FOR_FIELD32 marco.
  Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

 drivers/staging/octeon-usb/Makefile     |   1 +
 drivers/staging/octeon-usb/octeon-hcd.c | 128 +++++++++++++++++++-------------
 drivers/staging/octeon-usb/octeon-hcd.h |   1 +
 3 files changed, 80 insertions(+), 50 deletions(-)

-- 
2.16.4

[PATCH 1/3] Staging: octeon-usb: Adding SPDX license identifier

[Georgios Tsotsos]

Adding appropriate SPDX-License-Identifier (GPL-2) that were missing
from code, header and make files.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/Makefile     | 1 +
 drivers/staging/octeon-usb/octeon-hcd.c | 1 +
 drivers/staging/octeon-usb/octeon-hcd.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/staging/octeon-usb/Makefile b/drivers/staging/octeon-usb/Makefile
index 5588be395f2a..9873a0130ad5 100644
--- a/drivers/staging/octeon-usb/Makefile
+++ b/drivers/staging/octeon-usb/Makefile
@@ -1 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
 obj-${CONFIG_OCTEON_USB} := octeon-hcd.o
diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index cded30f145aa..cff5e790b196 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * This file is subject to the terms and conditions of the GNU General Public
  * License.  See the file "COPYING" in the main directory of this archive
diff --git a/drivers/staging/octeon-usb/octeon-hcd.h b/drivers/staging/octeon-usb/octeon-hcd.h
index 3353aefe662e..769c36cf6614 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.h
+++ b/drivers/staging/octeon-usb/octeon-hcd.h
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Octeon HCD hardware register definitions.
  *
-- 
2.16.4

[PATCH v2 1/3] Staging: octeon-usb: Adding SPDX license identifier

[Georgios Tsotsos]

Adding appropriate SPDX-License-Identifier (GPL-2) that were missing
from code, header and make files.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/Makefile     | 1 +
 drivers/staging/octeon-usb/octeon-hcd.c | 1 +
 drivers/staging/octeon-usb/octeon-hcd.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/staging/octeon-usb/Makefile b/drivers/staging/octeon-usb/Makefile
index 5588be395f2a..9873a0130ad5 100644
--- a/drivers/staging/octeon-usb/Makefile
+++ b/drivers/staging/octeon-usb/Makefile
@@ -1 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
 obj-${CONFIG_OCTEON_USB} := octeon-hcd.o
diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index cded30f145aa..cff5e790b196 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * This file is subject to the terms and conditions of the GNU General Public
  * License.  See the file "COPYING" in the main directory of this archive
diff --git a/drivers/staging/octeon-usb/octeon-hcd.h b/drivers/staging/octeon-usb/octeon-hcd.h
index 3353aefe662e..769c36cf6614 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.h
+++ b/drivers/staging/octeon-usb/octeon-hcd.h
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Octeon HCD hardware register definitions.
  *
-- 
2.16.4

[PATCH 2/3] Staging: octeon-usb: Change coding style of CVMX_WAIT_FOR_FIELD32 marco.

[Georgios Tsotsos]

Fixing coding style for CVMX_WAIT_FOR_FIELD32 was confusing. Also
encapsulates into parentheses timeout_usec.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 44 +++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index cff5e790b196..c8e0ebf1434f 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -378,27 +378,29 @@ struct octeon_hcd {
 };
 
 /* This macro spins on a register waiting for it to reach a condition. */
-#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	    \
-	({int result;							    \
-	do {								    \
-		u64 done = cvmx_get_cycle() + (u64)timeout_usec *	    \
-			   octeon_get_clock_rate() / 1000000;		    \
-		union _union c;						    \
-									    \
-		while (1) {						    \
-			c.u32 = cvmx_usb_read_csr32(usb, address);	    \
-									    \
-			if (cond) {					    \
-				result = 0;				    \
-				break;					    \
-			} else if (cvmx_get_cycle() > done) {		    \
-				result = -1;				    \
-				break;					    \
-			} else						    \
-				__delay(100);				    \
-		}							    \
-	} while (0);							    \
-	result; })
+#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	\
+({									\
+	int result;							\
+	do {								\
+		u64 done = cvmx_get_cycle() + (u64)(timeout_usec) *	\
+			   octeon_get_clock_rate() / 1000000;		\
+		union _union c;						\
+									\
+		while (1) {						\
+			c.u32 = cvmx_usb_read_csr32(usb, address);	\
+									\
+			if (cond) {					\
+				result = 0;				\
+				break;					\
+			} else if (cvmx_get_cycle() > done) {		\
+				result = -1;				\
+				break;					\
+			} else						\
+				__delay(100);				\
+		}							\
+	} while (0);							\
+	result;								\
+})
 
 /*
  * This macro logically sets a single field in a CSR. It does the sequence
-- 
2.16.4

[PATCH v2 2/3] Staging: octeon-usb: Change coding style of CVMX_WAIT_FOR_FIELD32 marco.

[Georgios Tsotsos]

Fixing coding style for CVMX_WAIT_FOR_FIELD32 was confusing. Also
encapsulates into parentheses timeout_usec.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 44 +++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index cff5e790b196..c8e0ebf1434f 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -378,27 +378,29 @@ struct octeon_hcd {
 };
 
 /* This macro spins on a register waiting for it to reach a condition. */
-#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	    \
-	({int result;							    \
-	do {								    \
-		u64 done = cvmx_get_cycle() + (u64)timeout_usec *	    \
-			   octeon_get_clock_rate() / 1000000;		    \
-		union _union c;						    \
-									    \
-		while (1) {						    \
-			c.u32 = cvmx_usb_read_csr32(usb, address);	    \
-									    \
-			if (cond) {					    \
-				result = 0;				    \
-				break;					    \
-			} else if (cvmx_get_cycle() > done) {		    \
-				result = -1;				    \
-				break;					    \
-			} else						    \
-				__delay(100);				    \
-		}							    \
-	} while (0);							    \
-	result; })
+#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	\
+({									\
+	int result;							\
+	do {								\
+		u64 done = cvmx_get_cycle() + (u64)(timeout_usec) *	\
+			   octeon_get_clock_rate() / 1000000;		\
+		union _union c;						\
+									\
+		while (1) {						\
+			c.u32 = cvmx_usb_read_csr32(usb, address);	\
+									\
+			if (cond) {					\
+				result = 0;				\
+				break;					\
+			} else if (cvmx_get_cycle() > done) {		\
+				result = -1;				\
+				break;					\
+			} else						\
+				__delay(100);				\
+		}							\
+	} while (0);							\
+	result;								\
+})
 
 /*
  * This macro logically sets a single field in a CSR. It does the sequence
-- 
2.16.4

Re: [PATCH v2 2/3] Staging: octeon-usb: Change coding style of CVMX_WAIT_FOR_FIELD32 marco.

[Greg Kroah-Hartman]

On Thu, Jul 26, 2018 at 06:41:52PM +0300, Georgios Tsotsos wrote:
> Fixing coding style for CVMX_WAIT_FOR_FIELD32 was confusing. Also
> encapsulates into parentheses timeout_usec.
> 
> Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
> ---
>  drivers/staging/octeon-usb/octeon-hcd.c | 44 +++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> index cff5e790b196..c8e0ebf1434f 100644
> --- a/drivers/staging/octeon-usb/octeon-hcd.c
> +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> @@ -378,27 +378,29 @@ struct octeon_hcd {
>  };
>  
>  /* This macro spins on a register waiting for it to reach a condition. */
> -#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	    \
> -	({int result;							    \
> -	do {								    \
> -		u64 done = cvmx_get_cycle() + (u64)timeout_usec *	    \
> -			   octeon_get_clock_rate() / 1000000;		    \
> -		union _union c;						    \
> -									    \
> -		while (1) {						    \
> -			c.u32 = cvmx_usb_read_csr32(usb, address);	    \
> -									    \
> -			if (cond) {					    \
> -				result = 0;				    \
> -				break;					    \
> -			} else if (cvmx_get_cycle() > done) {		    \
> -				result = -1;				    \
> -				break;					    \
> -			} else						    \
> -				__delay(100);				    \
> -		}							    \
> -	} while (0);							    \
> -	result; })
> +#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	\
> +({									\
> +	int result;							\
> +	do {								\
> +		u64 done = cvmx_get_cycle() + (u64)(timeout_usec) *	\
> +			   octeon_get_clock_rate() / 1000000;		\
> +		union _union c;						\
> +									\
> +		while (1) {						\
> +			c.u32 = cvmx_usb_read_csr32(usb, address);	\
> +									\
> +			if (cond) {					\
> +				result = 0;				\
> +				break;					\
> +			} else if (cvmx_get_cycle() > done) {		\
> +				result = -1;				\
> +				break;					\
> +			} else						\
> +				__delay(100);				\
> +		}							\
> +	} while (0);							\
> +	result;								\
> +})

It's still a mess, why not just make this a function call instead?

thanks,

greg k-h

Re: [PATCH v2 2/3] Staging: octeon-usb: Change coding style of CVMX_WAIT_FOR_FIELD32 marco.

[Georgios Tsotsos]

I will change it into function, as i checked so far i will need to
change USB_SET_FIELD32 for the same reason.

On Fri, 27 Jul 2018 at 18:15, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 26, 2018 at 06:41:52PM +0300, Georgios Tsotsos wrote:
> > Fixing coding style for CVMX_WAIT_FOR_FIELD32 was confusing. Also
> > encapsulates into parentheses timeout_usec.
> >
> > Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
> > ---
> >  drivers/staging/octeon-usb/octeon-hcd.c | 44 +++++++++++++++++----------------
> >  1 file changed, 23 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> > index cff5e790b196..c8e0ebf1434f 100644
> > --- a/drivers/staging/octeon-usb/octeon-hcd.c
> > +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> > @@ -378,27 +378,29 @@ struct octeon_hcd {
> >  };
> >
> >  /* This macro spins on a register waiting for it to reach a condition. */
> > -#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)       \
> > -     ({int result;                                                       \
> > -     do {                                                                \
> > -             u64 done = cvmx_get_cycle() + (u64)timeout_usec *           \
> > -                        octeon_get_clock_rate() / 1000000;               \
> > -             union _union c;                                             \
> > -                                                                         \
> > -             while (1) {                                                 \
> > -                     c.u32 = cvmx_usb_read_csr32(usb, address);          \
> > -                                                                         \
> > -                     if (cond) {                                         \
> > -                             result = 0;                                 \
> > -                             break;                                      \
> > -                     } else if (cvmx_get_cycle() > done) {               \
> > -                             result = -1;                                \
> > -                             break;                                      \
> > -                     } else                                              \
> > -                             __delay(100);                               \
> > -             }                                                           \
> > -     } while (0);                                                        \
> > -     result; })
> > +#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)   \
> > +({                                                                   \
> > +     int result;                                                     \
> > +     do {                                                            \
> > +             u64 done = cvmx_get_cycle() + (u64)(timeout_usec) *     \
> > +                        octeon_get_clock_rate() / 1000000;           \
> > +             union _union c;                                         \
> > +                                                                     \
> > +             while (1) {                                             \
> > +                     c.u32 = cvmx_usb_read_csr32(usb, address);      \
> > +                                                                     \
> > +                     if (cond) {                                     \
> > +                             result = 0;                             \
> > +                             break;                                  \
> > +                     } else if (cvmx_get_cycle() > done) {           \
> > +                             result = -1;                            \
> > +                             break;                                  \
> > +                     } else                                          \
> > +                             __delay(100);                           \
> > +             }                                                       \
> > +     } while (0);                                                    \
> > +     result;                                                         \
> > +})
>
> It's still a mess, why not just make this a function call instead?
>
> thanks,
>
> greg k-h



-- 
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*

[PATCH v3 0/2] Staging: octeon-usb: Changed CVMX_WAIT_FOR_FIELD32 macro

[Georgios Tsotsos]

Replying to previous message, i changed CVMX_WAIT_FOR_FIELD32 macro to function
and altered a multiple calling of CVMX_USBCX_GRSTCTL call to single call and
stored to variable.

Georgios Tsotsos (2):
  Staging: octeon-usb: Change multiple calling of CVMX_USBCX_GRSTCTL
  Staging: octeon-usb: Changes macro CVMX_WAIT_FOR_FIELD32 to function
    call.

 drivers/staging/octeon-usb/octeon-hcd.c | 77 +++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 33 deletions(-)

-- 
2.16.4

[PATCH v3 1/2] Staging: octeon-usb: Change multiple calling of CVMX_USBCX_GRSTCTL

[Georgios Tsotsos]

Assign to variable the result of CVMX_USBCX_GRSTCTL instead of multiple
calling a macro.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index cff5e790b196..4615133292b5 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -598,6 +598,7 @@ static void cvmx_fifo_setup(struct octeon_hcd *usb)
 	union cvmx_usbcx_ghwcfg3 usbcx_ghwcfg3;
 	union cvmx_usbcx_gnptxfsiz npsiz;
 	union cvmx_usbcx_hptxfsiz psiz;
+	u64 address;
 
 	usbcx_ghwcfg3.u32 = cvmx_usb_read_csr32(usb,
 						CVMX_USBCX_GHWCFG3(usb->index));
@@ -629,17 +630,16 @@ static void cvmx_fifo_setup(struct octeon_hcd *usb)
 	psiz.s.ptxfstaddr = 3 * usbcx_ghwcfg3.s.dfifodepth / 4;
 	cvmx_usb_write_csr32(usb, CVMX_USBCX_HPTXFSIZ(usb->index), psiz.u32);
 
+	address = CVMX_USBCX_GRSTCTL(usb->index);
+
 	/* Flush all FIFOs */
-	USB_SET_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			cvmx_usbcx_grstctl, txfnum, 0x10);
-	USB_SET_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			cvmx_usbcx_grstctl, txfflsh, 1);
-	CVMX_WAIT_FOR_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			      cvmx_usbcx_grstctl, c.s.txfflsh == 0, 100);
-	USB_SET_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			cvmx_usbcx_grstctl, rxfflsh, 1);
-	CVMX_WAIT_FOR_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			      cvmx_usbcx_grstctl, c.s.rxfflsh == 0, 100);
+	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);
+	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, rxfflsh, 1);
+	CVMX_WAIT_FOR_FIELD32(address, cvmx_usbcx_grstctl,
+			      c.s.rxfflsh == 0, 100);
 }
 
 /**
-- 
2.16.4

Re: [PATCH v3 1/2] Staging: octeon-usb: Change multiple calling of CVMX_USBCX_GRSTCTL

[Greg Kroah-Hartman]

On Sun, Jul 29, 2018 at 02:40:34PM +0300, Georgios Tsotsos wrote:
> Assign to variable the result of CVMX_USBCX_GRSTCTL instead of multiple
> calling a macro.
> 
> Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
> ---
>  drivers/staging/octeon-usb/octeon-hcd.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

What changed from previous versions?  You should always list that below
the --- line so that we know what is going on...

v4?  :)

thanks,

greg k-h

[PATCH v4 1/2] Staging: octeon-usb: Change multiple calling of CVMX_USBCX_GRSTCTL

[Georgios Tsotsos]

Assign to variable the result of CVMX_USBCX_GRSTCTL instead of multiple
calling a macro.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
v2: It wasn't exist in this or earlier versions of patch series
v3: It seems a logical to avoid multiple calls of CVMX_USBCX_GRSTCTL that  will
also help cleaning up calls of CVMX_WAIT_FOR_FIELD32
v4: Added patch version text

 drivers/staging/octeon-usb/octeon-hcd.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index cff5e790b196..4615133292b5 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -598,6 +598,7 @@ static void cvmx_fifo_setup(struct octeon_hcd *usb)
 	union cvmx_usbcx_ghwcfg3 usbcx_ghwcfg3;
 	union cvmx_usbcx_gnptxfsiz npsiz;
 	union cvmx_usbcx_hptxfsiz psiz;
+	u64 address;
 
 	usbcx_ghwcfg3.u32 = cvmx_usb_read_csr32(usb,
 						CVMX_USBCX_GHWCFG3(usb->index));
@@ -629,17 +630,16 @@ static void cvmx_fifo_setup(struct octeon_hcd *usb)
 	psiz.s.ptxfstaddr = 3 * usbcx_ghwcfg3.s.dfifodepth / 4;
 	cvmx_usb_write_csr32(usb, CVMX_USBCX_HPTXFSIZ(usb->index), psiz.u32);
 
+	address = CVMX_USBCX_GRSTCTL(usb->index);
+
 	/* Flush all FIFOs */
-	USB_SET_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			cvmx_usbcx_grstctl, txfnum, 0x10);
-	USB_SET_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			cvmx_usbcx_grstctl, txfflsh, 1);
-	CVMX_WAIT_FOR_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			      cvmx_usbcx_grstctl, c.s.txfflsh == 0, 100);
-	USB_SET_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			cvmx_usbcx_grstctl, rxfflsh, 1);
-	CVMX_WAIT_FOR_FIELD32(CVMX_USBCX_GRSTCTL(usb->index),
-			      cvmx_usbcx_grstctl, c.s.rxfflsh == 0, 100);
+	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);
+	USB_SET_FIELD32(address, cvmx_usbcx_grstctl, rxfflsh, 1);
+	CVMX_WAIT_FOR_FIELD32(address, cvmx_usbcx_grstctl,
+			      c.s.rxfflsh == 0, 100);
 }
 
 /**
-- 
2.16.4

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

[Georgios Tsotsos]

Replacing CVMX_WAIT_FOR_FIELD32 macro with equivalent function.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 65 +++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index 4615133292b5..8a7bdf1a9fe6 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -377,29 +377,6 @@ struct octeon_hcd {
 	struct cvmx_usb_tx_fifo nonperiodic;
 };
 
-/* This macro spins on a register waiting for it to reach a condition. */
-#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	    \
-	({int result;							    \
-	do {								    \
-		u64 done = cvmx_get_cycle() + (u64)timeout_usec *	    \
-			   octeon_get_clock_rate() / 1000000;		    \
-		union _union c;						    \
-									    \
-		while (1) {						    \
-			c.u32 = cvmx_usb_read_csr32(usb, address);	    \
-									    \
-			if (cond) {					    \
-				result = 0;				    \
-				break;					    \
-			} else if (cvmx_get_cycle() > done) {		    \
-				result = -1;				    \
-				break;					    \
-			} else						    \
-				__delay(100);				    \
-		}							    \
-	} while (0);							    \
-	result; })
-
 /*
  * This macro logically sets a single field in a CSR. It does the sequence
  * read, modify, and write
@@ -593,6 +570,42 @@ static inline int cvmx_usb_get_data_pid(struct cvmx_usb_pipe *pipe)
 	return 0; /* Data0 */
 }
 
+/**
+ * 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)
+{
+	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);
 	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);
 }
 
 /**
-- 
2.16.4

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.

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

[Georgios Tsotsos]

Replacing CVMX_WAIT_FOR_FIELD32 macro with equivalent function.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
v2: Changed CVMX_WAIT_FOR_FIELD32 syntax to avoid checkpatch check notice and
tried to make the macro more readable.
v3: Changed CVMX_WAIT_FOR_FIELD32 macro to function according as refereed in 
commit message and suggested by Greg Kroah-Hartman
v4: Added patch version text

 drivers/staging/octeon-usb/octeon-hcd.c | 65 +++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index 4615133292b5..8a7bdf1a9fe6 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -377,29 +377,6 @@ struct octeon_hcd {
 	struct cvmx_usb_tx_fifo nonperiodic;
 };
 
-/* This macro spins on a register waiting for it to reach a condition. */
-#define CVMX_WAIT_FOR_FIELD32(address, _union, cond, timeout_usec)	    \
-	({int result;							    \
-	do {								    \
-		u64 done = cvmx_get_cycle() + (u64)timeout_usec *	    \
-			   octeon_get_clock_rate() / 1000000;		    \
-		union _union c;						    \
-									    \
-		while (1) {						    \
-			c.u32 = cvmx_usb_read_csr32(usb, address);	    \
-									    \
-			if (cond) {					    \
-				result = 0;				    \
-				break;					    \
-			} else if (cvmx_get_cycle() > done) {		    \
-				result = -1;				    \
-				break;					    \
-			} else						    \
-				__delay(100);				    \
-		}							    \
-	} while (0);							    \
-	result; })
-
 /*
  * This macro logically sets a single field in a CSR. It does the sequence
  * read, modify, and write
@@ -593,6 +570,42 @@ static inline int cvmx_usb_get_data_pid(struct cvmx_usb_pipe *pipe)
 	return 0; /* Data0 */
 }
 
+/**
+ * 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)
+{
+	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);
 	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);
 }
 
 /**
-- 
2.16.4

[PATCH 3/3] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

[Georgios Tsotsos]

In order to make this function more clear a new function created that controls
channels halt on no DMA mode.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 83 +++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index c8e0ebf1434f..f9f429d385ce 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2585,6 +2585,52 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
 	}
 }
 
+/**
+ * Handles channels halt in non DMA mode
+ * @usbc_hcchar: Host Channel-n Characteristics Register (HCCHAR)
+ * @usbc_hcint:  Host Channel-n Interrupt Register
+ * @usb:         USB device
+ * @channel:     Channel to poll
+ *
+ * In non DMA mode the channels don't halt themselves. We need
+ * to manually disable channels that are left running
+ *
+ * Returns: -1 on halt
+ */
+static int cvmx_usb_dma_halt(union cvmx_usbcx_hccharx usbc_hcchar,
+			     union cvmx_usbcx_hcintx usbc_hcint,
+			     struct octeon_hcd *usb,
+			     int channel)
+{
+	struct usb_hcd *hcd = octeon_to_hcd(usb);
+	struct device *dev = hcd->self.controller;
+
+	if (usbc_hcchar.s.chena) {
+		union cvmx_usbcx_hcintmskx hcintmsk;
+		/* Disable all interrupts except CHHLTD */
+		hcintmsk.u32 = 0;
+		hcintmsk.s.chhltdmsk = 1;
+		cvmx_usb_write_csr32(usb,
+				     CVMX_USBCX_HCINTMSKX(channel, usb->index),
+				     hcintmsk.u32);
+		usbc_hcchar.s.chdis = 1;
+		cvmx_usb_write_csr32(usb,
+				     CVMX_USBCX_HCCHARX(channel, usb->index),
+				     usbc_hcchar.u32);
+		return -1;
+	} else if (usbc_hcint.s.xfercompl) {
+		/*
+		 * Successful IN/OUT with transfer complete.
+		 * Channel halt isn't needed.
+		 */
+	} else {
+		dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
+			usb->index, channel);
+		return -1;
+	}
+
+	return 0;
+}
 /**
  * Poll a channel for status
  *
@@ -2595,8 +2641,6 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
  */
 static int cvmx_usb_poll_channel(struct octeon_hcd *usb, int channel)
 {
-	struct usb_hcd *hcd = octeon_to_hcd(usb);
-	struct device *dev = hcd->self.controller;
 	union cvmx_usbcx_hcintx usbc_hcint;
 	union cvmx_usbcx_hctsizx usbc_hctsiz;
 	union cvmx_usbcx_hccharx usbc_hcchar;
@@ -2627,35 +2671,16 @@ static int cvmx_usb_poll_channel(struct octeon_hcd *usb, int channel)
 					     usbc_hcchar.u32);
 			return 0;
 		}
-
-		/*
-		 * In non DMA mode the channels don't halt themselves. We need
-		 * to manually disable channels that are left running
-		 */
+		/* In case of non DMA mode handle halt */
 		if (!usbc_hcint.s.chhltd) {
-			if (usbc_hcchar.s.chena) {
-				union cvmx_usbcx_hcintmskx hcintmsk;
-				/* Disable all interrupts except CHHLTD */
-				hcintmsk.u32 = 0;
-				hcintmsk.s.chhltdmsk = 1;
-				cvmx_usb_write_csr32(usb,
-						     CVMX_USBCX_HCINTMSKX(channel, usb->index),
-						     hcintmsk.u32);
-				usbc_hcchar.s.chdis = 1;
-				cvmx_usb_write_csr32(usb,
-						     CVMX_USBCX_HCCHARX(channel, usb->index),
-						     usbc_hcchar.u32);
-				return 0;
-			} else if (usbc_hcint.s.xfercompl) {
-				/*
-				 * Successful IN/OUT with transfer complete.
-				 * Channel halt isn't needed.
-				 */
-			} else {
-				dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
-					usb->index, channel);
+			int dma_halt_status = 0;
+
+			dma_halt_status = cvmx_usb_dma_halt(usbc_hcchar,
+							    usbc_hcint,
+							    usb, channel);
+
+			if (dma_halt_status < 0)
 				return 0;
-			}
 		}
 	} else {
 		/*
-- 
2.16.4

[PATCH v2 3/3] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

[Georgios Tsotsos]

In order to make this function more clear a new function created that controls
channels halt on no DMA mode.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 83 +++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index c8e0ebf1434f..f9f429d385ce 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2585,6 +2585,52 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
 	}
 }
 
+/**
+ * Handles channels halt in non DMA mode
+ * @usbc_hcchar: Host Channel-n Characteristics Register (HCCHAR)
+ * @usbc_hcint:  Host Channel-n Interrupt Register
+ * @usb:         USB device
+ * @channel:     Channel to poll
+ *
+ * In non DMA mode the channels don't halt themselves. We need
+ * to manually disable channels that are left running
+ *
+ * Returns: -1 on halt
+ */
+static int cvmx_usb_dma_halt(union cvmx_usbcx_hccharx usbc_hcchar,
+			     union cvmx_usbcx_hcintx usbc_hcint,
+			     struct octeon_hcd *usb,
+			     int channel)
+{
+	struct usb_hcd *hcd = octeon_to_hcd(usb);
+	struct device *dev = hcd->self.controller;
+
+	if (usbc_hcchar.s.chena) {
+		union cvmx_usbcx_hcintmskx hcintmsk;
+		/* Disable all interrupts except CHHLTD */
+		hcintmsk.u32 = 0;
+		hcintmsk.s.chhltdmsk = 1;
+		cvmx_usb_write_csr32(usb,
+				     CVMX_USBCX_HCINTMSKX(channel, usb->index),
+				     hcintmsk.u32);
+		usbc_hcchar.s.chdis = 1;
+		cvmx_usb_write_csr32(usb,
+				     CVMX_USBCX_HCCHARX(channel, usb->index),
+				     usbc_hcchar.u32);
+		return -1;
+	} else if (usbc_hcint.s.xfercompl) {
+		/*
+		 * Successful IN/OUT with transfer complete.
+		 * Channel halt isn't needed.
+		 */
+	} else {
+		dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
+			usb->index, channel);
+		return -1;
+	}
+
+	return 0;
+}
 /**
  * Poll a channel for status
  *
@@ -2595,8 +2641,6 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
  */
 static int cvmx_usb_poll_channel(struct octeon_hcd *usb, int channel)
 {
-	struct usb_hcd *hcd = octeon_to_hcd(usb);
-	struct device *dev = hcd->self.controller;
 	union cvmx_usbcx_hcintx usbc_hcint;
 	union cvmx_usbcx_hctsizx usbc_hctsiz;
 	union cvmx_usbcx_hccharx usbc_hcchar;
@@ -2627,35 +2671,16 @@ static int cvmx_usb_poll_channel(struct octeon_hcd *usb, int channel)
 					     usbc_hcchar.u32);
 			return 0;
 		}
-
-		/*
-		 * In non DMA mode the channels don't halt themselves. We need
-		 * to manually disable channels that are left running
-		 */
+		/* In case of non DMA mode handle halt */
 		if (!usbc_hcint.s.chhltd) {
-			if (usbc_hcchar.s.chena) {
-				union cvmx_usbcx_hcintmskx hcintmsk;
-				/* Disable all interrupts except CHHLTD */
-				hcintmsk.u32 = 0;
-				hcintmsk.s.chhltdmsk = 1;
-				cvmx_usb_write_csr32(usb,
-						     CVMX_USBCX_HCINTMSKX(channel, usb->index),
-						     hcintmsk.u32);
-				usbc_hcchar.s.chdis = 1;
-				cvmx_usb_write_csr32(usb,
-						     CVMX_USBCX_HCCHARX(channel, usb->index),
-						     usbc_hcchar.u32);
-				return 0;
-			} else if (usbc_hcint.s.xfercompl) {
-				/*
-				 * Successful IN/OUT with transfer complete.
-				 * Channel halt isn't needed.
-				 */
-			} else {
-				dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
-					usb->index, channel);
+			int dma_halt_status = 0;
+
+			dma_halt_status = cvmx_usb_dma_halt(usbc_hcchar,
+							    usbc_hcint,
+							    usb, channel);
+
+			if (dma_halt_status < 0)
 				return 0;
-			}
 		}
 	} else {
 		/*
-- 
2.16.4

Re: [PATCH v2 3/3] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

[Joe Perches]

On Thu, 2018-07-26 at 18:41 +0300, Georgios Tsotsos wrote:
> In order to make this function more clear a new function created that controls
> channels halt on no DMA mode.
[]
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
[]
> @@ -2585,6 +2585,52 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
>  	}
>  }
>  
> +/**
> + * Handles channels halt in non DMA mode
> + * @usbc_hcchar: Host Channel-n Characteristics Register (HCCHAR)
> + * @usbc_hcint:  Host Channel-n Interrupt Register
> + * @usb:         USB device
> + * @channel:     Channel to poll
> + *
> + * In non DMA mode the channels don't halt themselves. We need
> + * to manually disable channels that are left running
> + *
> + * Returns: -1 on halt
> + */
> +static int cvmx_usb_dma_halt(union cvmx_usbcx_hccharx usbc_hcchar,
> +			     union cvmx_usbcx_hcintx usbc_hcint,

It looks very suspect to pass unions on the stack.

Are you sure these aren't used after this function
is called?

Likely these should be pointers to unions.

Re: [PATCH v2 3/3] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

[Georgios Tsotsos]

Indeed i should probably either use pointer
or pass the values, i will do some more testing
and update this.
Thanks

On Thu, 26 Jul 2018 at 19:31, Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2018-07-26 at 18:41 +0300, Georgios Tsotsos wrote:
> > In order to make this function more clear a new function created that controls
> > channels halt on no DMA mode.
> []
> > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> []
> > @@ -2585,6 +2585,52 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
> >       }
> >  }
> >
> > +/**
> > + * Handles channels halt in non DMA mode
> > + * @usbc_hcchar: Host Channel-n Characteristics Register (HCCHAR)
> > + * @usbc_hcint:  Host Channel-n Interrupt Register
> > + * @usb:         USB device
> > + * @channel:     Channel to poll
> > + *
> > + * In non DMA mode the channels don't halt themselves. We need
> > + * to manually disable channels that are left running
> > + *
> > + * Returns: -1 on halt
> > + */
> > +static int cvmx_usb_dma_halt(union cvmx_usbcx_hccharx usbc_hcchar,
> > +                          union cvmx_usbcx_hcintx usbc_hcint,
>
> It looks very suspect to pass unions on the stack.
>
> Are you sure these aren't used after this function
> is called?
>
> Likely these should be pointers to unions.
>


-- 
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
tsotsos@linux.com
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos

[PATCH v3 0/1] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel()

[Georgios Tsotsos]

Changed the function in order to use only values instead passing unions
i think its better than passing pointers there.

Georgios Tsotsos (1):
  Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

 drivers/staging/octeon-usb/octeon-hcd.c | 81 +++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 28 deletions(-)

-- 
2.16.4

[PATCH v3 1/1] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

[Georgios Tsotsos]

In order to make this function more clear a new function created that controls
channels halt on no DMA mode.

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 81 +++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index 8a7bdf1a9fe6..3f44ac260eff 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2593,7 +2593,51 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
 		cvmx_usb_complete(usb, pipe, transaction, CVMX_USB_STATUS_OK);
 	}
 }
+/**
+ * Handles channels halt in non DMA mode
+ * @hcchar_chena:
+ * @hcint_xfercompl:
+ * @usb:             USB device
+ * @channel:         Channel to poll
+ *
+ * In non DMA mode the channels don't halt themselves. We need
+ * to manually disable channels that are left running
+ *
+ * Returns: -1 on halt
+ */
+static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl,
+			     struct octeon_hcd *usb, int channel)
+{
+	struct usb_hcd *hcd = octeon_to_hcd(usb);
+	struct device *dev = hcd->self.controller;
 
+	if (hcchar_chena) {
+		union cvmx_usbcx_hcintmskx hcintmsk;
+		union cvmx_usbcx_hccharx usbc_hcchar;
+		/* Disable all interrupts except CHHLTD */
+		hcintmsk.u32 = 0;
+		hcintmsk.s.chhltdmsk = 1;
+		cvmx_usb_write_csr32(usb,
+				     CVMX_USBCX_HCINTMSKX(channel, usb->index),
+				     hcintmsk.u32);
+		usbc_hcchar.s.chdis = 1;
+		cvmx_usb_write_csr32(usb,
+				     CVMX_USBCX_HCCHARX(channel, usb->index),
+				     usbc_hcchar.u32);
+		return -1;
+	} else if (hcint_xfercompl) {
+		/*
+		 * Successful IN/OUT with transfer complete.
+		 * Channel halt isn't needed.
+		 */
+	} else {
+		dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
+			usb->index, channel);
+		return -1;
+	}
+
+	return 0;
+}
 /**
  * Poll a channel for status
  *
@@ -2604,8 +2648,6 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
  */
 static int cvmx_usb_poll_channel(struct octeon_hcd *usb, int channel)
 {
-	struct usb_hcd *hcd = octeon_to_hcd(usb);
-	struct device *dev = hcd->self.controller;
 	union cvmx_usbcx_hcintx usbc_hcint;
 	union cvmx_usbcx_hctsizx usbc_hctsiz;
 	union cvmx_usbcx_hccharx usbc_hcchar;
@@ -2637,34 +2679,17 @@ static int cvmx_usb_poll_channel(struct octeon_hcd *usb, int channel)
 			return 0;
 		}
 
-		/*
-		 * In non DMA mode the channels don't halt themselves. We need
-		 * to manually disable channels that are left running
-		 */
+		/* In case of non DMA mode handle halt */
 		if (!usbc_hcint.s.chhltd) {
-			if (usbc_hcchar.s.chena) {
-				union cvmx_usbcx_hcintmskx hcintmsk;
-				/* Disable all interrupts except CHHLTD */
-				hcintmsk.u32 = 0;
-				hcintmsk.s.chhltdmsk = 1;
-				cvmx_usb_write_csr32(usb,
-						     CVMX_USBCX_HCINTMSKX(channel, usb->index),
-						     hcintmsk.u32);
-				usbc_hcchar.s.chdis = 1;
-				cvmx_usb_write_csr32(usb,
-						     CVMX_USBCX_HCCHARX(channel, usb->index),
-						     usbc_hcchar.u32);
-				return 0;
-			} else if (usbc_hcint.s.xfercompl) {
-				/*
-				 * Successful IN/OUT with transfer complete.
-				 * Channel halt isn't needed.
-				 */
-			} else {
-				dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
-					usb->index, channel);
+			int dma_halt_status = 0;
+			u32 xfercompl = usbc_hcint.s.xfercompl;
+
+			dma_halt_status = cvmx_usb_dma_halt(usbc_hcchar.s.chena,
+							    xfercompl,
+							    usb, channel);
+
+			if (dma_halt_status < 0)
 				return 0;
-			}
 		}
 	} else {
 		/*
-- 
2.16.4

Re: [PATCH v3 1/1] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

[Greg Kroah-Hartman]

On Sun, Jul 29, 2018 at 02:41:53PM +0300, Georgios Tsotsos wrote:
> In order to make this function more clear a new function created that controls
> channels halt on no DMA mode.
> 
> Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
> ---
>  drivers/staging/octeon-usb/octeon-hcd.c | 81 +++++++++++++++++++++------------
>  1 file changed, 53 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> index 8a7bdf1a9fe6..3f44ac260eff 100644
> --- a/drivers/staging/octeon-usb/octeon-hcd.c
> +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> @@ -2593,7 +2593,51 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
>  		cvmx_usb_complete(usb, pipe, transaction, CVMX_USB_STATUS_OK);
>  	}
>  }
> +/**

Blank line between functions please.

Also, as this is not a global function, no need for kerneldoc
formatting, but you did it already, so no big deal.

> + * Handles channels halt in non DMA mode
> + * @hcchar_chena:
> + * @hcint_xfercompl:
> + * @usb:             USB device
> + * @channel:         Channel to poll
> + *
> + * In non DMA mode the channels don't halt themselves. We need
> + * to manually disable channels that are left running
> + *
> + * Returns: -1 on halt
> + */
> +static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl,
> +			     struct octeon_hcd *usb, int channel)
> +{
> +	struct usb_hcd *hcd = octeon_to_hcd(usb);
> +	struct device *dev = hcd->self.controller;
>  
> +	if (hcchar_chena) {
> +		union cvmx_usbcx_hcintmskx hcintmsk;
> +		union cvmx_usbcx_hccharx usbc_hcchar;
> +		/* Disable all interrupts except CHHLTD */
> +		hcintmsk.u32 = 0;
> +		hcintmsk.s.chhltdmsk = 1;
> +		cvmx_usb_write_csr32(usb,
> +				     CVMX_USBCX_HCINTMSKX(channel, usb->index),
> +				     hcintmsk.u32);
> +		usbc_hcchar.s.chdis = 1;
> +		cvmx_usb_write_csr32(usb,
> +				     CVMX_USBCX_HCCHARX(channel, usb->index),
> +				     usbc_hcchar.u32);
> +		return -1;

Do not make up error values, return -EINVAL or something like that (what
ever the real error here is.)

> +	} else if (hcint_xfercompl) {
> +		/*
> +		 * Successful IN/OUT with transfer complete.
> +		 * Channel halt isn't needed.
> +		 */
> +	} else {
> +		dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
> +			usb->index, channel);
> +		return -1;

Same here.

thanks,

greg k-h

[PATCH v4 1/1] Staging: octeon-usb: Using defined error codes and applying coding style

[Georgios Tsotsos]

Replaced -1 with defined error code EINVAL

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
v2: Apply coding style to function cvmx_usb_poll_channel
v3: Break down function cvmx_usb_poll_channel
v4: Return defined error code and applying coding style for function calls
 drivers/staging/octeon-usb/octeon-hcd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index 3f44ac260eff..edf87d1b3609 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2590,6 +2590,7 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
 		}
 	} else {
 		pipe->next_tx_frame += pipe->interval;
+
 		cvmx_usb_complete(usb, pipe, transaction, CVMX_USB_STATUS_OK);
 	}
 }
@@ -2624,16 +2625,16 @@ static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl,
 		cvmx_usb_write_csr32(usb,
 				     CVMX_USBCX_HCCHARX(channel, usb->index),
 				     usbc_hcchar.u32);
-		return -1;
+		return -EINVAL;
 	} else if (hcint_xfercompl) {
 		/*
 		 * Successful IN/OUT with transfer complete.
 		 * Channel halt isn't needed.
 		 */
 	} else {
-		dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
+		dev_	err(dev, "USB%d: Channel %d interrupt without halt\n",
 			usb->index, channel);
-		return -1;
+		return -EINVAL;
 	}
 
 	return 0;
-- 
2.16.4

Re: [PATCH v4 1/1] Staging: octeon-usb: Using defined error codes and applying coding style

[Aaro Koskinen]

Hi,

On Sun, Jul 29, 2018 at 05:33:38PM +0300, Georgios Tsotsos wrote:
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> index 3f44ac260eff..edf87d1b3609 100644
> --- a/drivers/staging/octeon-usb/octeon-hcd.c
> +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> @@ -2590,6 +2590,7 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
>  		}
>  	} else {
>  		pipe->next_tx_frame += pipe->interval;
> +
>  		cvmx_usb_complete(usb, pipe, transaction, CVMX_USB_STATUS_OK);

Unrelated whitespace change...

>  	}
>  }
> @@ -2624,16 +2625,16 @@ static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl,
>  		cvmx_usb_write_csr32(usb,
>  				     CVMX_USBCX_HCCHARX(channel, usb->index),
>  				     usbc_hcchar.u32);
> -		return -1;
> +		return -EINVAL;
>  	} else if (hcint_xfercompl) {
>  		/*
>  		 * Successful IN/OUT with transfer complete.
>  		 * Channel halt isn't needed.
>  		 */
>  	} else {
> -		dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
> +		dev_	err(dev, "USB%d: Channel %d interrupt without halt\n",
>  			usb->index, channel);

...also here. Does this even compile?

> -		return -1;
> +		return -EINVAL;
>  	}
>  
>  	return 0;
> -- 
> 2.16.4

A.

Re: [PATCH v4 1/1] Staging: octeon-usb: Using defined error codes and applying coding style

[Georgios Tsotsos]

Hi,

Indeed there was a mix-up with patches i will send correction asap.
On Sun, 29 Jul 2018 at 23:21, Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
>
> Hi,
>
> On Sun, Jul 29, 2018 at 05:33:38PM +0300, Georgios Tsotsos wrote:
> > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> > index 3f44ac260eff..edf87d1b3609 100644
> > --- a/drivers/staging/octeon-usb/octeon-hcd.c
> > +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> > @@ -2590,6 +2590,7 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
> >               }
> >       } else {
> >               pipe->next_tx_frame += pipe->interval;
> > +
> >               cvmx_usb_complete(usb, pipe, transaction, CVMX_USB_STATUS_OK);
>
> Unrelated whitespace change...
>
> >       }
> >  }
> > @@ -2624,16 +2625,16 @@ static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl,
> >               cvmx_usb_write_csr32(usb,
> >                                    CVMX_USBCX_HCCHARX(channel, usb->index),
> >                                    usbc_hcchar.u32);
> > -             return -1;
> > +             return -EINVAL;
> >       } else if (hcint_xfercompl) {
> >               /*
> >                * Successful IN/OUT with transfer complete.
> >                * Channel halt isn't needed.
> >                */
> >       } else {
> > -             dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
> > +             dev_    err(dev, "USB%d: Channel %d interrupt without halt\n",
> >                       usb->index, channel);
>
> ...also here. Does this even compile?
>
> > -             return -1;
> > +             return -EINVAL;
> >       }
> >
> >       return 0;
> > --
> > 2.16.4
>
> A.



-- 
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
tsotsos@linux.com
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos

[PATCH v5] Staging: octeon-usb: Using defined error codes and applying coding style.

[Georgios Tsotsos]

Replaced -1 with defined error code EINVAL

Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
---
v2: Apply coding style to function cvmx_usb_poll_channel
v3: Break down function cvmx_usb_poll_channel
v4: Return defined error code and applying coding style for function calls
v5: Fixing wrong patch applied before with typo on dev_err and applying coding
style as suggested on v3

 drivers/staging/octeon-usb/octeon-hcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
index c9fbff93bed4..dd73fd48e12e 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2587,10 +2587,10 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
 		}
 	} else {
 		pipe->next_tx_frame += pipe->interval;
-
 		cvmx_usb_complete(usb, pipe, transaction, CVMX_USB_STATUS_OK);
 	}
 }
+
 /**
  * Handles channels halt in non DMA mode
  * @hcchar_chena:
@@ -2629,7 +2629,7 @@ static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl,
 		 * Channel halt isn't needed.
 		 */
 	} else {
-		dev_	err(dev, "USB%d: Channel %d interrupt without halt\n",
+		dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
 			usb->index, channel);
 		return -EINVAL;
 	}
-- 
2.16.4

Re: [PATCH v5] Staging: octeon-usb: Using defined error codes and applying coding style.

[Greg Kroah-Hartman]

On Mon, Jul 30, 2018 at 01:29:36AM +0300, Georgios Tsotsos wrote:
> Replaced -1 with defined error code EINVAL
> 
> Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
> ---
> v2: Apply coding style to function cvmx_usb_poll_channel
> v3: Break down function cvmx_usb_poll_channel
> v4: Return defined error code and applying coding style for function calls
> v5: Fixing wrong patch applied before with typo on dev_err and applying coding
> style as suggested on v3
> 
>  drivers/staging/octeon-usb/octeon-hcd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Ugh, I have no idea anymore what patches to apply from you for this
driver, sorry, there are too many flying around.

Take a day off, relax, and then resend all of your pending patches as
one series, properly numbered, and we can go from there.  I've dropped
all of your patches from my queue for now.

thanks,

greg k-h

Re: [PATCH v5] Staging: octeon-usb: Using defined error codes and applying coding style.

[Georgios Tsotsos]

Yes reseeding in a better way seems logical, i even got lost
with the patch series. I will return with more consistent
series.
On Mon, 30 Jul 2018 at 11:51, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 30, 2018 at 01:29:36AM +0300, Georgios Tsotsos wrote:
> > Replaced -1 with defined error code EINVAL
> >
> > Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
> > ---
> > v2: Apply coding style to function cvmx_usb_poll_channel
> > v3: Break down function cvmx_usb_poll_channel
> > v4: Return defined error code and applying coding style for function calls
> > v5: Fixing wrong patch applied before with typo on dev_err and applying coding
> > style as suggested on v3
> >
> >  drivers/staging/octeon-usb/octeon-hcd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Ugh, I have no idea anymore what patches to apply from you for this
> driver, sorry, there are too many flying around.
>
> Take a day off, relax, and then resend all of your pending patches as
> one series, properly numbered, and we can go from there.  I've dropped
> all of your patches from my queue for now.
>
> thanks,
>
> greg k-h



-- 
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
tsotsos@linux.com
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos

Re: [PATCH v3 1/1] Staging: octeon-usb: Breaks down cvmx_usb_poll_channel().

[Georgios Tsotsos]

Hello,
Regarding your latest comment, i have notice many functions in this module
using kerneldoc and not been global, also there are various erroneous
situations that functions return not defined error codes.
I will try to fix them all and a new patch series for those two issues after
this one is ok. (I hope with less versions :))
On Sun, 29 Jul 2018 at 15:43, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jul 29, 2018 at 02:41:53PM +0300, Georgios Tsotsos wrote:
> > In order to make this function more clear a new function created that controls
> > channels halt on no DMA mode.
> >
> > Signed-off-by: Georgios Tsotsos <tsotsos@gmail.com>
> > ---
> >  drivers/staging/octeon-usb/octeon-hcd.c | 81 +++++++++++++++++++++------------
> >  1 file changed, 53 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> > index 8a7bdf1a9fe6..3f44ac260eff 100644
> > --- a/drivers/staging/octeon-usb/octeon-hcd.c
> > +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> > @@ -2593,7 +2593,51 @@ static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb,
> >               cvmx_usb_complete(usb, pipe, transaction, CVMX_USB_STATUS_OK);
> >       }
> >  }
> > +/**
>
> Blank line between functions please.
>
> Also, as this is not a global function, no need for kerneldoc
> formatting, but you did it already, so no big deal.
>
> > + * Handles channels halt in non DMA mode
> > + * @hcchar_chena:
> > + * @hcint_xfercompl:
> > + * @usb:             USB device
> > + * @channel:         Channel to poll
> > + *
> > + * In non DMA mode the channels don't halt themselves. We need
> > + * to manually disable channels that are left running
> > + *
> > + * Returns: -1 on halt
> > + */
> > +static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl,
> > +                          struct octeon_hcd *usb, int channel)
> > +{
> > +     struct usb_hcd *hcd = octeon_to_hcd(usb);
> > +     struct device *dev = hcd->self.controller;
> >
> > +     if (hcchar_chena) {
> > +             union cvmx_usbcx_hcintmskx hcintmsk;
> > +             union cvmx_usbcx_hccharx usbc_hcchar;
> > +             /* Disable all interrupts except CHHLTD */
> > +             hcintmsk.u32 = 0;
> > +             hcintmsk.s.chhltdmsk = 1;
> > +             cvmx_usb_write_csr32(usb,
> > +                                  CVMX_USBCX_HCINTMSKX(channel, usb->index),
> > +                                  hcintmsk.u32);
> > +             usbc_hcchar.s.chdis = 1;
> > +             cvmx_usb_write_csr32(usb,
> > +                                  CVMX_USBCX_HCCHARX(channel, usb->index),
> > +                                  usbc_hcchar.u32);
> > +             return -1;
>
> Do not make up error values, return -EINVAL or something like that (what
> ever the real error here is.)
>
> > +     } else if (hcint_xfercompl) {
> > +             /*
> > +              * Successful IN/OUT with transfer complete.
> > +              * Channel halt isn't needed.
> > +              */
> > +     } else {
> > +             dev_err(dev, "USB%d: Channel %d interrupt without halt\n",
> > +                     usb->index, channel);
> > +             return -1;
>
> Same here.
>
> thanks,
>
> greg k-h



--
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
tsotsos@linux.com
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos