Skip to contentSkip to conversation

Staging: octeon-usb fixes for coding style, SPDX and readability.

PATCH In review Georgios Tsotsos 4 participants 25 messages Jul 26, 2018 – Jul 30, 2018 Staging

lore
Share & tools
lore

Georgios Tsotsos

author
#0 lore raw

v2 cover Staging: octeon-usb fixes for coding style, SPDX and readability.

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 ([email protected]) 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(-)
Signature · 1 line
2.16.4
author
#1 lore raw

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

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

Signed-off-by Georgios Tsotsos <[email protected]>
---
 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(+)
drivers/staging/octeon-usb/Makefile +1 -0
index 5588be395f2a..9873a0130ad5 100644
@@ -1 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
 obj-${CONFIG_OCTEON_USB} := octeon-hcd.o
drivers/staging/octeon-usb/octeon-hcd.c +1 -0
index cded30f145aa..cff5e790b196 100644
@@ -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
drivers/staging/octeon-usb/octeon-hcd.h +1 -0
index 3353aefe662e..769c36cf6614 100644
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Octeon HCD hardware register definitions.
  *
Signature · 1 line
2.16.4
author
#2 lore raw

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

In reply to Georgios Tsotsos

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

Signed-off-by Georgios Tsotsos <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 44 +++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +23 -21
index cff5e790b196..c8e0ebf1434f 100644
@@ -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
Signature · 1 line
2.16.4
maintainer
#3 lore raw

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

Quoted 50 lines · Georgios Tsotsos

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

Signed-off-by Georgios Tsotsos <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 44 +++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +13 -21
index cff5e790b196..c8e0ebf1434f 100644
@@ -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;				\

drivers/staging/octeon-usb/octeon-hcd.c struct octeon_hcd { · line 403

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +10 -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

author
#4 lore raw

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

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

Quoted 68 lines · Greg Kroah-Hartman
Quoted 50 lines · Georgios Tsotsos

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

Signed-off-by Georgios Tsotsos <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 44 +++++++++++++++++----------------
 1 file changed, 23 insertions(+), 21 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +13 -21
index cff5e790b196..c8e0ebf1434f 100644
@@ -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;                             \

drivers/staging/octeon-usb/octeon-hcd.c struct octeon_hcd { · line 403

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +10 -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

Signature · 6 lines
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
author
#5 lore raw

v3 cover Staging: octeon-usb: Changed CVMX_WAIT_FOR_FIELD32 macro

In reply to Greg Kroah-Hartman

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(-)
Signature · 1 line
2.16.4
author
#6 lore raw

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

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

Signed-off-by Georgios Tsotsos <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +10 -10
index cff5e790b196..4615133292b5 100644
@@ -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);
 }
 
 /**
Signature · 1 line
2.16.4
maintainer
#7 lore raw

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

Quoted 7 lines · Georgios Tsotsos

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

Signed-off-by Georgios Tsotsos <[email protected]>
---
 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

author
#8 lore raw

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

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

Signed-off-by Georgios Tsotsos <[email protected]>

---
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(-)
drivers/staging/octeon-usb/octeon-hcd.c +10 -10
index cff5e790b196..4615133292b5 100644
@@ -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);
 }
 
 /**
Signature · 1 line
2.16.4
author
#9 lore raw

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

Replacing CVMX_WAIT_FOR_FIELD32 macro with equivalent function.

Signed-off-by Georgios Tsotsos <[email protected]>

---
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(-)
drivers/staging/octeon-usb/octeon-hcd.c +38 -27
index 4615133292b5..8a7bdf1a9fe6 100644
@@ -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);
 }
 
 /**
Signature · 1 line
2.16.4
author
#10 lore raw

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

In reply to Georgios Tsotsos

Replacing CVMX_WAIT_FOR_FIELD32 macro with equivalent function.

Signed-off-by Georgios Tsotsos <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 65 +++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 27 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +38 -27
index 4615133292b5..8a7bdf1a9fe6 100644
@@ -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);
 }
 
 /**
Signature · 1 line
2.16.4
#11 lore raw

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

Hi,

drivers/staging/octeon-usb/octeon-hcd.c static inline int cvmx_usb_get_data_pid(struct cvmx_usb_pipe *pipe) · line 583

Quoted 11 lines · Georgios Tsotsos
drivers/staging/octeon-usb/octeon-hcd.c +11 -0
+/**
+ * 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...

[...]

Quoted 29 lines
drivers/staging/octeon-usb/octeon-hcd.c +25 -0
+{
+	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;

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_fifo_setup(struct octeon_hcd *usb) · line 651

Quoted 6 lines
drivers/staging/octeon-usb/octeon-hcd.c +1 -2
@@ -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.

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_fifo_setup(struct octeon_hcd *usb) · line 653

Quoted 4 lines
drivers/staging/octeon-usb/octeon-hcd.c +1 -2
 	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.

author
#12 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 83 +++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 29 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +54 -29
index c8e0ebf1434f..f9f429d385ce 100644
@@ -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 {
 		/*
Signature · 1 line
2.16.4
#13 lore raw

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

Quoted 2 lines · Georgios Tsotsos

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

[]

drivers/staging/octeon-usb/octeon-hcd.c

Quoted 1 line
drivers/staging/octeon-usb/octeon-hcd.c +0 -0

[]

Quoted 8 lines
drivers/staging/octeon-usb/octeon-hcd.c +4 -0
@@ -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

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2601

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +10 -0
+ * @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.

author
#14 lore raw

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

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

Quoted 33 lines · Joe Perches
Quoted 2 lines · Georgios Tsotsos

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

[]

drivers/staging/octeon-usb/octeon-hcd.c

Quoted 1 line
drivers/staging/octeon-usb/octeon-hcd.c +0 -0

[]

Quoted 8 lines
drivers/staging/octeon-usb/octeon-hcd.c +4 -0
@@ -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

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2601

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +10 -0
+ * @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.

Signature · 10 lines
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
[email protected]
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos
author
#15 lore raw

v3 cover Staging: octeon-usb: Breaks down cvmx_usb_poll_channel()

In reply to Joe Perches

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(-)
Signature · 1 line
2.16.4
author
#16 lore raw

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

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 <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 81 +++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 28 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +53 -28
index 8a7bdf1a9fe6..3f44ac260eff 100644
@@ -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 {
 		/*
Signature · 1 line
2.16.4
maintainer
#17 lore raw

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

Quoted 13 lines · 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 <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 81 +++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 28 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +0 -0
index 8a7bdf1a9fe6..3f44ac260eff 100644

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2596

Quoted 4 lines
drivers/staging/octeon-usb/octeon-hcd.c +1 -0
@@ -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.

Quoted 21 lines
drivers/staging/octeon-usb/octeon-hcd.c +20 -0
+ * 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 */

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2627

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +10 -0
+		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.)

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2636

Quoted 9 lines
drivers/staging/octeon-usb/octeon-hcd.c +9 -0
+	} 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

author
#18 lore raw

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

Replaced -1 with defined error code EINVAL

Signed-off-by Georgios Tsotsos <[email protected]>

---
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(-)
drivers/staging/octeon-usb/octeon-hcd.c +4 -3
index 3f44ac260eff..edf87d1b3609 100644
@@ -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;
Signature · 1 line
2.16.4
#19 lore raw

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

Hi,

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2594

Quoted 10 lines · Georgios Tsotsos
drivers/staging/octeon-usb/octeon-hcd.c +1 -0
index 3f44ac260eff..edf87d1b3609 100644
@@ -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...

Quoted 7 lines
drivers/staging/octeon-usb/octeon-hcd.c +0 -1
 	}
 }

@@ -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;

drivers/staging/octeon-usb/octeon-hcd.c static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl, · line 2636

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +2 -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?

Quoted 7 lines
drivers/staging/octeon-usb/octeon-hcd.c +1 -1
-		return -1;
+		return -EINVAL;
 	}
 
 	return 0;
Signature · 1 line
2.16.4

A.

author
#20 lore raw

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

Hi,

Indeed there was a mix-up with patches i will send correction asap.

Quoted 46 lines · Aaro Koskinen

Hi,

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2594

Quoted 10 lines · Georgios Tsotsos
drivers/staging/octeon-usb/octeon-hcd.c +1 -0
index 3f44ac260eff..edf87d1b3609 100644
@@ -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...

Quoted 7 lines
drivers/staging/octeon-usb/octeon-hcd.c +0 -1
      }
 }

@@ -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;

drivers/staging/octeon-usb/octeon-hcd.c static int cvmx_usb_dma_halt(u32 hcchar_chena, u32 hcint_xfercompl, · line 2636

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +2 -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?

Quoted 7 lines
drivers/staging/octeon-usb/octeon-hcd.c +1 -2
-             return -1;
+             return -EINVAL;
      }
 
      return 0;
--

2.16.4

A.

Signature · 10 lines
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
[email protected]
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos
author
#21 lore raw

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

In reply to Aaro Koskinen

Replaced -1 with defined error code EINVAL

Signed-off-by Georgios Tsotsos <[email protected]>

---
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(-)
drivers/staging/octeon-usb/octeon-hcd.c +2 -2
index c9fbff93bed4..dd73fd48e12e 100644
@@ -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;
 	}
Signature · 1 line
2.16.4
maintainer
#22 lore raw

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

Quoted 10 lines · Georgios Tsotsos

Replaced -1 with defined error code EINVAL

Signed-off-by Georgios Tsotsos <[email protected]>

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

Quoted 2 lines
 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

author
#23 lore raw

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

Yes reseeding in a better way seems logical, i even got lost with the patch series. I will return with more consistent series.

Quoted 25 lines · Greg Kroah-Hartman
Quoted 10 lines · Georgios Tsotsos

Replaced -1 with defined error code EINVAL

Signed-off-by Georgios Tsotsos <[email protected]>

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

Quoted 2 lines
 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

Signature · 10 lines
Best regards!
Georgios Tsotsos
Greece-Evia-Chalkida
[email protected]
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos
author
#24 lore raw

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

In reply to Greg Kroah-Hartman

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

Quoted 70 lines · Greg Kroah-Hartman
Quoted 13 lines · 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 <[email protected]>
---
 drivers/staging/octeon-usb/octeon-hcd.c | 81 +++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 28 deletions(-)
drivers/staging/octeon-usb/octeon-hcd.c +0 -0
index 8a7bdf1a9fe6..3f44ac260eff 100644

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2596

Quoted 4 lines
drivers/staging/octeon-usb/octeon-hcd.c +1 -0
@@ -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.

Quoted 21 lines
drivers/staging/octeon-usb/octeon-hcd.c +20 -0
+ * 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 */

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2627

Quoted 10 lines
drivers/staging/octeon-usb/octeon-hcd.c +10 -0
+             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.)

drivers/staging/octeon-usb/octeon-hcd.c static void cvmx_usb_transfer_isoc(struct octeon_hcd *usb, · line 2636

Quoted 9 lines
drivers/staging/octeon-usb/octeon-hcd.c +9 -0
+     } 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;
Quoted 5 lines

Same here.

thanks,

greg k-h

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