Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs

[Matthias Kaehlcke]

On Thu, Oct 01, 2020 at 09:21:53PM -0400, Alan Stern wrote:
> On Thu, Oct 01, 2020 at 02:54:12PM -0700, Matthias Kaehlcke wrote:
> > Hi,
> > 
> > thanks for providing more insights on the USB hardware!
> 
> Sure.
> 
> > On Wed, Sep 30, 2020 at 09:24:13PM -0400, Alan Stern wrote:
> > > A hub that attaches only to the USB-3 data wires in a cable is not USB
> > > compliant.  A USB-2 device plugged into such a hub would not work.
> > > 
> > > But ports can be wired up in weird ways.  For example, it is possible
> > > to have the USB-3 wires from a port going directly to the host
> > > controller, while the USB-2 wires from the same port go through a
> > > USB-2 hub which is then connected to a separate host controller.  (In
> > > fact, my office computer has just such an arrangement.)
> > 
> > It's not clear to me how this case would be addressed when (some of) the
> > handling is done in xhci-plat.c We have two host controllers now, which one
> > is supposed to be in charge? I guess the idea is to specify the hub only
> > for one of the controllers?
> 
> I don't grasp the point of this question.  It doesn't seem to be
> relevant to the case you're concerned about -- your board isn't going to
> wire up the special hub in this weird way, is it?

When doing upstream development I try to look beyond my specific use case
and aim for solutions that are generally useful.

I don't know how common a configuration like the one on your office computer
is. If it isn't a fringe case it seems like we should support it if feasible.

> > > > Yes, I've been saying for some time we need a pre-probe. Or we need a
> > > > forced probe where the subsystem walks the DT nodes for the bus and
> > > > probes the devices in DT (if they're in DT, we know they are present).
> > > > This was the discussion only a few weeks ago for MDIO (which I think
> > > > concluded with they already do the latter).
> > > 
> > > This is why I suggested putting the new code into the xhci-platform
> > > driver.  That is the right place for doing these "pre-probes" of DT
> > > nodes for hubs attached to the host controller.
> > 
> > Reminder that the driver is not exclusively about powering the hub, but
> > also about powering it off conditionally during system suspend, depending
> > on what devices are connected to either of the busses. Should this also
> > be done in the xhci-platform driver?
> 
> It certainly could be.  The platform-specific xhci suspend and resume
> routines could power the hub on and off as needed, along with powering
> the host controller.
> 
> > Since we are talking about "pre-probes" I imagine the idea is to have a
> > USB device driver that implements the power on/off sequence (in pre_probe()
> > and handles the suspend/resume case. I already went through a variant of
> > this with an earlier version of the onboard_hub_driver, where suspend/resume
> > case was handled by the USB hub device. One of the problems with this was
> > that power must only be turned off after both USB hub devices have been
> > suspended. Some instance needs to be aware that there are two USB devices
> > and make the decision whether to cut the power during system suspend
> > or not, which is one of the reasons I ended up with the platform
> > driver. It's not clear to me how this would be addressed by using
> > "pre-probes". Potentially some of the handling could be done by
> > xhci-platform, but would that be really better than a dedicated driver?
> 
> _All_ of the handling could be done by xhci-plat.  Since the xHCI
> controller is the parent of both the USB-2 and USB-3 incarnations of
> the special hub, it won't get suspended until they are both in
> suspend, and it will get resumed before either of them.  Similarly,
> the power to the special hub could be switched on as part of the host
> controller's probe routine and switched off during the host
> controller's remove routine.
> 
> Using xhci-plat in this way would be better than a dedicated driver in
> the sense that it wouldn't then be necessary to make up a fictitious
> platform device and somehow describe it in DT.
> 
> The disadvantage is that we would end up with a driver that's
> nominally meant to handle host controllers but now also manages (at
> least in part) hubs.  A not-so-clean separation of functions.  But
> that's not terribly different from the way your current patch works,
> right?

Yes, this muddling of the xhci-plat code with the handling of hubs was
one of my concerns, but who am I to argue if you as USB maintainer see
that preferable over a dedicated driver. I suppose you are taking into
account that there will be a need for code for different hub models that
has to live somewhere (could be a dedicated file or directory).

And even if it is not my specific use case it would be nice to support
hubs that are part of a hierarchy and not wired directly to the host
controller. We don't necessarily have to implement all support for this
initially, but should have it in mind at least for the bindings.

Also we would probably lose the ability to use a sysfs attribute to
configure whether the hub should be always powered during suspend or
not. This could be worked around with a DT property, however DT
maintainers tend to be reluctant about configuration entries that
don't translate directly to the hardware.

I think at this point I should say that I can't personally commit to
implement such a solution in a foreseeable future due to other
commitments involved in shipping products. But I also want to make it
clear that as a project Chrome OS is interested in landing this
functionality upstream. I might be able to carve out some time, but it's
not certain. If not we will come back to this eventually, be it myself
or someone else on behalf of Chrome OS.