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

[Matthias Kaehlcke]

On Wed, Sep 30, 2020 at 02:19:32PM -0500, Rob Herring wrote:
> On Wed, Sep 30, 2020 at 1:00 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> > > >
> > > > We already have hubs in DT. See [1][2][3][4]. What's new here?
> >
> > After I sent my response I kept thinking about this and I realized
> > that I have prior art I can point out too!  :-)  Check out
> > "smsc,usb3503a".  That is describing a USB hub too and, at least on
> > "exynos5250-spring.dts" is is a top level node.  Since "smsc,usb3503a"
> > can be optionally connected to an i2c bus too, it could be listed
> > under an i2c controller as well (I believe it wasn't hooked up to i2c
> > on spring).
> >
> > Interestingly enough, the USB Hub that Matthias is trying to add
> > support for can _also_ be hooked up to i2c.  We don't actually have
> > i2c hooked up on our board, but conceivably it could be.  Presumably,
> > if i2c was hooked up, we would have no other choice but to represent
> > this chip as several device tree nodes: at least one under the i2c
> > controller and one (or two) under the USB controller.  Just because
> > (on this board) i2c isn't hooked up doesn't change the fact that there
> > is some extra control logic that could be represented in its own
> > device tree node.  To me, this seems to give extra evidence that the
> > correct way to model this device in device tree is with several nodes.
> >
> > I'll point out that on "exynos5250-spring.dts" we didn't have to solve
> > the problem that Matthias is trying to solve here because we never
> > actually supported waking up from USB devices there.  Thus the
> > regulator for the hub on spring can be unconditionally powered off in
> > suspend.  On newer boards we'd like to support waking up from USB
> > devices but also to save power if no wakeup devices are plugged into
> > USB.  In order to achieve this we need some type of link from the
> > top-level hub device to the actual USB devices that were enumerated.
> 
> Yes, in a prior version I mentioned we already had 2 ways to describe
> hubs. I view this as a 3rd way.

The description of the onboard hub driver is essentially the same as
that for the 'smsc,usb3503a'. Ths driver doesn't require the USB device
nodes, but they could as well exist, they are only omitted most of the
time because USB does discovery, some DT files include these implicit
nodes though.

It would be possible to rewrite the onboard_usb_hub driver in a way that
it wouldn't require phandles of the 'usb_hub' (or whatever) node, and
instead provide the 'usb_hub' with phandles of the USB devices. The
hub would be specified exactly once:

{
	usb_hub: usb-hub {
		compatible = "realtek,rts5411", "onboard-usb-hub";
		usbdevs = <&usb_1_udev1>, <&usb_1_udev2>;
		vdd-supply = <&pp3300_hub>;
        };

	&usb_1_dwc3 {
		usb_1_udev1: usb@1 {
        		reg = <1>;
		};

		usb_1_udev2: usb@2 {
			reg = <2>;
	        };
	};
}

The only difference with the 'smsc,usb3503a' would be that the nodes of
the (existing!) USB devices would be specified (without any custom
properties).

I'm not convinced that 'pre-probes' can solve the entire problem on the
driver side and keep thinking that there needs to be a single non-USB
instance that controls the power state, particularly for the
suspend/resume case. I will provide some more details in another reply
to this thread.