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(-)
--
2.16.4
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(-)
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
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(-)
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
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(+)
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
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.
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 <[email protected]> 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
[email protected]
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos
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 <[email protected]>
> ---
> 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
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
<[email protected]> 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 <[email protected]>
> > ---
> > 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*
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
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(-)
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
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(-)
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
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
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(-)
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
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 <[email protected]>
> ---
> 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
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 <[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
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(-)
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
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(-)
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
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(-)
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
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
<[email protected]> 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 <[email protected]>
> > ---
> > 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
[email protected]
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos
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.
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.
Hi,
Indeed there was a mix-up with patches i will send correction asap.
On Sun, 29 Jul 2018 at 23:21, Aaro Koskinen <[email protected]> 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
[email protected]
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: 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
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
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 <[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(-)
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
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
<[email protected]> 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 <[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(-)
>
> 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
[email protected]
skype: tsotsos
------------------------------------
Georgios Tsotsos
*Greece - Evia - Chalkida*
tsotsos[at]linux.com
skype: tsotsos