tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: patch: 3.0 hub support for xhci



On 03/24/15 13:30, Robert Sprowson wrote:
Hi,

This patch tries to support USB 3.0 for xhci.

xhci is still at best experimental.
Don't use it on production servers even though it looks work well.

Fixes: [...]
Known bugs: [...]
Misc bugs: [...]
I've spent a few hours looking at this patch, and there's plenty of meat in
there to chew on. However I think in its current form it's undigestable as
in fact it comprises
a) some good fixes that should go straight into CVS

I've picked out a few of these :)

b) some experimental USB 3 work that might break it for some people, this
    should probably be on a private branch

I'm happy to do this on my nick-nhusb branch, but maybe this isn't what you want.

c) some port specific tweaks for Intel chipsets, which should probably be
    compile time bracketed or taken out with quirks on other ports
Again, nick-nhusb?
d) new helper functions in the main USB stack
Most are in I think.

I guess that's why it's lingering, because at ~2000 lines it's probably too
big to see which falls into which category.

Taking a step back though, I'd tend to agree that XHCI is currently
experimental grade.

Looking at other open source stacks I see that for each TRB type there's a
call out from the common USB stack code to the equivalent hardware
operation, whereas in the current NetBSD implementation almost all of
xhci_new_device() is a straight copy and paste of the similarly named
function in usbd_subr.c which in turn has required making some internal
structures/functions of the stack externally visible which isn't great from
an stack layering point of view.

I think the single callout added in 1.193 of usb_subr.c

   if (bus->methods->new_device != NULL)
     return (bus->methods->new_device)(parent, bus, depth, speed, port, up);

is much too coarse granularity, and likely to lead to them getting out of
sync - I notice, for example, that the XHCI driver has lost the
USB_SET_ADDRESS_SETTLE delay code which it ought to have (XHCI spec 4.6.5).

I propose having more methods and getting rid of the new_device one. In a
prototype version here I think the functions needed are

   xhci_new_device_pre(usbd_device_handle dev, int port)
     This is called when something is first plugged in, and allocated a slot
     from the root hub, and initialises it.
     This slots into usb_subr.c just after establishing the default pipe, or
     does nothing if the controller in question doesn't register that method.

   xhci_new_device_addr(usbd_device_handle dev)
     This allocates a new context, and returns the device address to be
     used by the rest of the USB stack.
     This is used in place of usbd_getnewaddr() in usb_subr.c.

	 xhci_new_device_post(usbd_device_handle dev)
     This does any fixups once addressed, for example telling the XHCI
     controller what the MPS is.
     This is called just before the address settle delay once everything is
     known from the initial device descriptor read.

	 xhci_new_device_remove(usbd_device_handle dev)
     This is called whenever anything is unplugged, to free up the hardware
     allocations.
     This slots into usbd_remove_device() and also at the end of
     usb_disconnect_port() in case the whole lot gets dropped.

Wouldn't a a set_address method be much more simple?


This approach also fixes a number of leaks (both heap and hardware
assignments) for free since all the well exercised code paths in the main
USB stack are now being used, and the XHCI hardware kept in step with its
view of the tree.

Does this sound sensible? It'd be good to keep XHCI moving forward,

I'd like to move xhci forward as well.

Robert.

PS: Sorry if the mail header references got lost, this thread was scraped
off the web archives.




Nick


Home | Main Index | Thread Index | Old Index