Re: patch: 3.0 hub support for xhci

[Robert Sprowson <webpages%sprow.co.uk@localhost>]

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
b) some experimental USB 3 work that might break it for some people, this
   should probably be on a private branch
c) some port specific tweaks for Intel chipsets, which should probably be
   compile time bracketed or taken out with quirks on other ports
d) new helper functions in the main USB stack 

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.

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,
Robert.

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