Patch: xhci controller driver improvements

Hello,

Post by Rajasekhar Pulluru
Hi,
The attached patch addresses following points.
- With some usb devices, often some endpoints gets stalled. Resetting the
endpoint helps recover usb transactions.
- When a usb device is removed while data is being copied to/from, all the
queued transfer requests needs to be aborted gracefully so that transfer
status is reported up to the application.
- Some boards require an additional interrupt acknowledgement specific to
the architecture. Added a function pointer that needs to be set during that
board arch-specific initialization. If this is not required, function
pointer needs to be set to NULL.
- Added debug functions to dump xhci registers that could help us in case
of any issue.
Note: I have manually patched the code changes from my older
version(verified) of code to the latest as of today in MAIN branch. And the
code is not compiled either, sorry about that. If anyone is interested to
patch this and verify, it would be really helpful.

I reformatted whitespace and made several changes:

- avoid lock error when calling xhci_do_command from
xhci_stop_endpoint
- assume usb_uncallout(a,b,c) is callout_stop(&a)
- comment out checking xfer->device->bus->intr_context
- declare gsc used in debug code (would be set in ddb?)

It should build at least, but still experimental.

--
t-hash

matthew green

2014-08-10 03:31:31 UTC

+#include <uvm/uvm.h> /* for vtophys on arm */

should probably be uvm_extern.h.

.mrg.

Takahiro HAYASHI

2014-08-10 04:46:33 UTC

Post by matthew green

+#include <uvm/uvm.h> /* for vtophys on arm */

should probably be uvm_extern.h.

I see, thank you.

--
t-hash

Nick Hudson

2014-08-10 08:06:33 UTC

Post by Takahiro HAYASHI
Hello,

Post by Rajasekhar Pulluru
Hi,
The attached patch addresses following points.
- With some usb devices, often some endpoints gets stalled. Resetting the
endpoint helps recover usb transactions.
- When a usb device is removed while data is being copied to/from, all the
queued transfer requests needs to be aborted gracefully so that transfer
status is reported up to the application.
- Some boards require an additional interrupt acknowledgement
specific to
the architecture. Added a function pointer that needs to be set during that
board arch-specific initialization. If this is not required, function
pointer needs to be set to NULL.
- Added debug functions to dump xhci registers that could help us in case
of any issue.
Note: I have manually patched the code changes from my older
version(verified) of code to the latest as of today in MAIN branch. And the
code is not compiled either, sorry about that. If anyone is
interested to
patch this and verify, it would be really helpful.

- avoid lock error when calling xhci_do_command from
xhci_stop_endpoint
- assume usb_uncallout(a,b,c) is callout_stop(&a)
- comment out checking xfer->device->bus->intr_context
- declare gsc used in debug code (would be set in ddb?)
It should build at least, but still experimental.

Some comments...

Post by Takahiro HAYASHI
--- src/sys/dev/usb/xhci.c.orig 2014-08-05 22:24:27.000000000 +0900
+++ src/sys/dev/usb/xhci.c 2014-08-07 17:35:16.000000000 +0900
@@ -55,6 +55,8 @@ __KERNEL_RCSID(0, "$NetBSD: xhci.c,v 1.2
#include <dev/usb/xhcivar.h>
#include <dev/usb/usbroothub_subr.h>
+#include <uvm/uvm.h> /* for vtophys on arm */
+

Shouldn't be needed at all - the hexdump use is questionable.

Post by Takahiro HAYASHI
@@ -1127,6 +1165,81 @@ xhci_open(usbd_pipe_handle pipe)
}
static void
+xhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)

This function needs usbmp-ifiction, i.e. updating for -current by
removing spl, tsleep, wakeup, etc.

Post by Takahiro HAYASHI
@@ -2889,6 +3028,7 @@ xhci_timeout(void *addr)
struct xhci_softc * const sc = xfer->pipe->device->bus->hci_private;
if (sc->sc_dying) {
+ xhci_abort_xfer(xfer, USBD_TIMEOUT);
return;
}

This looks very strange.

The driver makes far too much use of device_printf and all USB should
move to KERNHIST.

Nick

Takahiro HAYASHI

2014-08-10 11:54:21 UTC

Hi,

Post by Nick Hudson
Some comments...

--- src/sys/dev/usb/xhci.c.orig 2014-08-05 22:24:27.000000000 +0900
+++ src/sys/dev/usb/xhci.c 2014-08-07 17:35:16.000000000 +0900
@@ -55,6 +55,8 @@ __KERNEL_RCSID(0, "$NetBSD: xhci.c,v 1.2
#include <dev/usb/xhcivar.h>
#include <dev/usb/usbroothub_subr.h>
+#include <uvm/uvm.h> /* for vtophys on arm */
+

Shouldn't be needed at all - the hexdump use is questionable.

The code of hexdump is wrapperd with #if 0, but I added
this just for someone who wants enable this and dump
TRBs on arm e.g. mirabox.

@@ -1127,6 +1165,81 @@ xhci_open(usbd_pipe_handle pipe)
}
static void
+xhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)

This function needs usbmp-ifiction, i.e. updating for -current by removing spl, tsleep, wakeup, etc.

Hmm, I need to learn about them.
I guess this patch is based on netbsd-5 so it uses spl family.

@@ -2889,6 +3028,7 @@ xhci_timeout(void *addr)
struct xhci_softc * const sc = xfer->pipe->device->bus->hci_private;
if (sc->sc_dying) {
+ xhci_abort_xfer(xfer, USBD_TIMEOUT);
return;
}

This looks very strange.

Does it need sc_lock and unlock?
or it's strange that xhci_abort_xfer is called
from callout(softclock) interrupt context?

Post by Nick Hudson
The driver makes far too much use of device_printf and all USB should move to KERNHIST.

I didn't know about KERNHIST, thanks for notifying.
I've replaced device_printf with DPRINTF or DPRINTFN in
my local tree.

--
t-hash

Nick Hudson

2014-08-11 10:40:14 UTC

Post by Takahiro HAYASHI
Hi,

Hi,

Post by Nick Hudson
Some comments...

Post by Takahiro HAYASHI
@@ -2889,6 +3028,7 @@ xhci_timeout(void *addr)
struct xhci_softc * const sc =
xfer->pipe->device->bus->hci_private;
if (sc->sc_dying) {
+ xhci_abort_xfer(xfer, USBD_TIMEOUT);
return;
}

This looks very strange.

Does it need sc_lock and unlock?
or it's strange that xhci_abort_xfer is called
from callout(softclock) interrupt context?

Calling xhci_abort_xfer while dying.

Post by Nick Hudson
The driver makes far too much use of device_printf and all USB should move to KERNHIST.

I didn't know about KERNHIST, thanks for notifying.
I've replaced device_printf with DPRINTF or DPRINTFN in
my local tree.

Please send diff :)

Nick

Takahiro HAYASHI

2014-08-11 22:45:33 UTC

This looks very strange.

Does it need sc_lock and unlock?
or it's strange that xhci_abort_xfer is called
from callout(softclock) interrupt context?

Calling xhci_abort_xfer while dying.

well, should it be like this?

--- xhci.c.orig 2014-08-11 20:14:13.000000000 +0900
+++ xhci.c 2014-08-12 06:41:55.000000000 +0900
@@ -2896,6 +3492,11 @@ xhci_timeout(void *addr)
struct xhci_softc * const sc = xfer->pipe->device->bus->hci_private;

if (sc->sc_dying) {
+ mutex_enter(&sc->sc_lock);
+ xfer->status = USBD_TIMEOUT;
+ callout_stop(&xfer->timeout_handle);
+ usb_transfer_complete(xfer);
+ mutex_exit(&sc->sc_lock);
return;
}

This resembles {u,o,e}hci.c do in dying path.

Post by Nick Hudson
The driver makes far too much use of device_printf and all USB should move to KERNHIST.

I didn't know about KERNHIST, thanks for notifying.
I've replaced device_printf with DPRINTF or DPRINTFN in
my local tree.

Please send diff :)

Wait for a while plz.

--
t-hash

Takahiro HAYASHI

2014-08-12 09:42:04 UTC

Post by Nick Hudson
Please send diff :)

Wait for a while plz.

lessprf.diff
replaces most of device_printf.
I define new macros DPRINTD and DPRINTDF. former prints
args with device_xname(sc->sc_dev), latter prints args
with device name and function name.

I'll dump my local misc patches too.

usb3.diff
tries to make usb stack recognize super speed and
make usbdevs(8) print super speed.
This patch also adds XHCI_DEBUG flag to opt_usb.h.

lockmore.diff
adds lock with sc_intr_lock to xhci_intr and xhci_poll.

lsmps.diff
makes use 8 as wMaxPacketSize for LS when addressing device.
http://mail-index.netbsd.org/source-changes/2013/03/20/msg042367.html

Thanks,

--
t-hash

Nick Hudson

2014-08-12 14:07:01 UTC

Post by Nick Hudson
Please send diff :)

Wait for a while plz.

lessprf.diff
replaces most of device_printf.
I define new macros DPRINTD and DPRINTDF. former prints
args with device_xname(sc->sc_dev), latter prints args
with device name and function name.

Can you check that this compiles with all the combinations of USB_DEBUG,
XHCI_DEBUG and DIAGNOSTIC please?

Post by Takahiro HAYASHI
I'll dump my local misc patches too.
usb3.diff
tries to make usb stack recognize super speed and
make usbdevs(8) print super speed.
This patch also adds XHCI_DEBUG flag to opt_usb.h.

I committed most of this apart from the memset as I don't think it's
required.

Post by Takahiro HAYASHI
lockmore.diff
adds lock with sc_intr_lock to xhci_intr and xhci_poll.

committed

Post by Takahiro HAYASHI
lsmps.diff
makes use 8 as wMaxPacketSize for LS when addressing device.
http://mail-index.netbsd.org/source-changes/2013/03/20/msg042367.html

Committed.

Post by Takahiro HAYASHI
Thanks,

Nick

Nick Hudson

2014-08-12 14:27:32 UTC

I committed most of this apart from the memset as I don't think it's
required.

UPS_SUPER_SPEED should be done differently in the long run by adding
super speed hub support properly

Nick

Takahiro HAYASHI

2014-08-12 16:20:52 UTC

Post by Nick Hudson
Please send diff :)

Wait for a while plz.

lessprf.diff
replaces most of device_printf.
I define new macros DPRINTD and DPRINTDF. former prints
args with device_xname(sc->sc_dev), latter prints args
with device name and function name.

Can you check that this compiles with all the combinations of USB_DEBUG, XHCI_DEBUG and DIAGNOSTIC please?

Sorry, some declrations was reported unused.
New patch is attached.

I committed most of this apart from the memset as I don't think it's required.

Post by Takahiro HAYASHI
lockmore.diff
adds lock with sc_intr_lock to xhci_intr and xhci_poll.

committed

Post by Takahiro HAYASHI
lsmps.diff
makes use 8 as wMaxPacketSize for LS when addressing device.
http://mail-index.netbsd.org/source-changes/2013/03/20/msg042367.html

Committed.

Thank you for commit!

--
t-hash

Nick Hudson

2014-09-20 07:12:44 UTC

Post by Nick Hudson
Please send diff :)

Wait for a while plz.

lessprf.diff
replaces most of device_printf.
I define new macros DPRINTD and DPRINTDF. former prints
args with device_xname(sc->sc_dev), latter prints args
with device name and function name.

Can you check that this compiles with all the combinations of
USB_DEBUG, XHCI_DEBUG and DIAGNOSTIC please?

Sorry, some declrations was reported unused.
New patch is attached.

Hi,

Can you convert the DPRINTFs to USBHIST_LOG, please?

Thanks,
Nick

Takahiro HAYASHI

2014-09-24 14:45:20 UTC

hi,

(I haven't noticed this mail, sorry for delay.)

Post by Takahiro HAYASHI
lessprf.diff
replaces most of device_printf.
I define new macros DPRINTD and DPRINTDF. former prints
args with device_xname(sc->sc_dev), latter prints args
with device name and function name.

Can you check that this compiles with all the combinations of USB_DEBUG, XHCI_DEBUG and DIAGNOSTIC please?

Sorry, some declrations was reported unused.
New patch is attached.

Hi,
Can you convert the DPRINTFs to USBHIST_LOG, please?

Yes, I'll try to do.

Thanks,

--
t-hash

Takahiro HAYASHI

2014-09-25 15:58:49 UTC

Hi,