Hi Stephen,

On Thu, Feb 11, 2021 at 11:14:39AM -0800, Stephen Boyd wrote:

Quoting Matthias Kaehlcke (2021-02-10 14:20:18)

quoted

On Wed, Feb 10, 2021 at 10:06:45PM +0100, Krzysztof Kozlowski wrote:

quoted

This looks hackish... what if later we have something else than hub?
Another if()?

What if hub could be connected to something else than XHCI controller?

In earlier versions this was standalone driver, which was more flexible and
didn't require cooperation from the XHCI driver:

https://lore.kernel.org/patchwork/patch/1313001/

Rob Herring raised objections about the DT bindings, since the USB hub would be
represented twice in the DT, once in the USB hierachry (with an explicit node or
implicitly) plus a node for the platform device for the new driver:

https://lore.kernel.org/patchwork/patch/1305395/
https://lore.kernel.org/patchwork/patch/1313000/

Alan Stern suggested to create the platform device in the XHCI platform driver:

https://lore.kernel.org/patchwork/patch/1313000/#1510227

I wasn't super happy about involving xhci-plat, but at least the code is minimal
and all the device specific stuff is handled by the onboard_usb_hub driver.

If you have better suggestions that might satisfy all parties please let us
know :)

Is it possible to use the graph binding to connect the USB controller on
the SoC to the port on the hub? Then the hub would be a standalone node
at the root of DT connected to the USB controller (or phy) and xhci code
could probe the firmware to see if there's a graph connection downstream
that is a powered hub like this. I didn't see this idea mentioned in the
previous discussions, but maybe I missed it.

Thanks for bringing this up. I'm not sure I completely understand your
suggestion, but in general it seems a direction that could be worth
exploring.

I think something like the following should work even without requiring
cooperation from the XHCI code:

onboard-usb-hub {
    compatible = “realtek,rts5411”, “onboard_usb_hub”;
    #address-cells = <1>;
    #size-cells = <0>;

    vdd-supply = <&pp3300_hub>;

    port@0 {
        reg = <0>;

        rts5411_3_0: endpoint {
            // should not be needed
            remote-endpoint = <&usb_1_dwc3_port1>;
        };
    };

    port@1 {
        reg = <1>;

        rts5411_2_0: endpoint {
            // should not be needed
            remote-endpoint = <&usb_1_dwc3_port2>;
        };
    };
};

&usb_1_dwc3 {
    dr_mode = "host";
    #address-cells = <1>;
    #size-cells = <0>;

    port@1 {
        reg = <1>;

        usb_1_dwc3_port1: endpoint {
            remote-endpoint = <&rts5411_3_0>;
        };
    };

    port@2 {
        reg = <2>;

        usb_1_dwc3_port2: endpoint {
            remote-endpoint = <&rts5411_2_0>;
        };
    };
};

That looks like an actual description of the hardware, without multiple DT
nodes for the hub.

The USB part of the onboard_hub driver could determine the platform device
from the remote endpoint and register the USB device with it.