Re: patch: 3.0 hub support for xhci

[Nick Hudson <skrll%netbsd.org@localhost>]

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