Re: patch: 3.0 hub support for xhci

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

In article <551D31C0.5020504%netbsd.org@localhost>, Nick Hudson wrote:
> On 03/24/15 13:30, Robert Sprowson wrote:
> > > 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: [...]
> >
> > 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?

It wasn't so much what I wanted, it was more a question to make sure the
work wasn't lost. My natural assumption is for it to be on a branch so that
it didn't risk anyone trying to use it in its current form (without USB3
hubs) on the trunk from ending up in a bad place.

> > 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 [...] 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
> >
> >   xhci_new_device_addr(usbd_device_handle dev)
> >
> > 	 xhci_new_device_post(usbd_device_handle dev) 
> >
> > 	 xhci_new_device_remove(usbd_device_handle dev)
>
> Wouldn't a a set_address method be much more simple?

With an XHCI controller it's not your job to set the address, the addresses
are dished out by the controller. To get to that point there are setup
phases where controller resources are initialised (new_device_pre) then ask
what address got assigned (new_device_addr) then get the default descriptor
and tell the controller what it was (new_device_post) and clean up if the
device gets unplugged (new_device_remove).

This is /sort of/ illustrated in figure 10 in 4.5.3 of the XHCI spec.

Basically, some bits of the (software) USB stack have been handed to the
XHCI controller hardware. But the XHCI controller is still pretty dumb, so
complicated operations like parsing descriptors are still done in software,
and the two need to be kept in sync. 

I believe the intent is so that one controller can service many virtualised
operating systems and that attached devices don't get confused when more
than 1 OS tries to claim a peripheral.

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

The simplest illustration here is if you repeatedly plug/unplug a device.
Pretty quickly you run out of slots in the XHCI controller because nothing's
currently freeing them up - the hardware is (very) out of sync with the
(software) USB stack,
Robert.