Hi Rob,

Thanks again for the comments and feedback. Kindly see responses inline:

(Trimming unrelated text from thread):

On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:

On Fri, May 29, 2020 at 5:30 PM Prashant Malani [off-list ref] wrote:

quoted

Nodes truncated and unrelated fields omitted in the interest of brevity:

// Chrome OS EC Type C Port Manager.
typec {
    compatible = "google,cros-ec-typec";
    #address-cells = <1>;
    #size-cells = <0>;

    connector@0 {
        compatible = "usb-c-connector";
        reg = <0>;
        power-role = "dual";
        data-role = "dual";
        try-power-role = "source";
        mode-switch = <&foo_mux>;
        // Other switches can point to the same mux.
        ....

The connector is supposed to have 'ports' for USB2, USB3, and Aux
unless the parent is the USB controller.

Understood; so, coupled with Heikki's explanation (see below for where
I've pasted it), would it be something like so? (adding inline to the connector
node definition):

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    usb_con_hs: endpoint {
                        remote-endpoint = <&foo_usb_hs_controller>;
                    };
                };

                port@1 {
                    reg = <1>;
                    usb_con0_ss: endpoint@0 {
                        remote-endpoint = <&mode_mux_in>;
                    };
                };

                port@2 {
                    reg = <2>;
                    usb_con_sbu: endpoint {
                        remote-endpoint = <&foo_dp_aux>;
                    };
                };
            };

quoted

    };
};

// Mux switch
// TODO: Can possibly embed this in the PHY controller node itself?
foo_mux {
    compatible = "vendor,typec-mux";
    mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;

    ports {
        #address-cells = <1>;
        #size-cells = <0>;
        port@0 {
            reg = <0>;
            mux_dp_in: endpoint {
                remote-endpoint = <&dp_phy_out>;
            };
        };

        port@1 {
            reg = <1>;
            mux_usb_in: endpoint1 {
                remote-endpoint = <&usb3_phy_out>;
            };
        };

This all goes away if you have ports in the connector node. More below.

I think I my earlier example may have been a bit incorrect. Per Heikki's
explanation of who the consumer of the mux-control bindings is, the
foo_mux definition would now be like so:

foo_mux {
    compatible = "typec-mux-consumer";
    // This can be expanded to add more mux-controls for orientation,
    // data role etc.
    mux-controls = <&mode_mux_controller>;
    mux-control-names = "mode";
    #address-cells = <1>;
    #size-cells = <0>;

    port@0 {
        reg = <0>;
        mode_mux_in: endpoint {
            remote-endpoint = <&usb_con0_ss>
        };
    };

    port@1 {
        reg = <1>;
        mode_mux_out_usb3: endpoint {
            remote-endpoint = <&usb3_0_ep>
        };
    };

    port@2 {
        reg = <2>;
        mode_mux_out_dp: endpoint {
            remote-endpoint = <&dp0_out_ep>
        };
    };
};

and we add the definition for the mux-controller, where the mux-gpio
control actually resides:

mode_mux_controller: mux-controller {
    compatible = "vendor,typec-mode-mux";
    mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
};

quoted

    };
};

// Type C PHY Controller.
tcphy0: phy@ff7c0000 {
    compatible = "rockchip,rk3399-typec-phy";
    reg = <0x0 0xff7c0000 0x0 0x40000>;
    ...
    tcphy0_dp: phy@dc00000 {
        compatible = "soc,dp-phy";
        reg = <0xdc00000 0x1000>;
        ports {
            port@0 {
                reg = <0>;
                dp_phy_out: endpoint {
                    remote-endpoint = <&mux_dp_in>;
                };
            };

This is wrong in that 'phys' are not part of the graph. It's the DP
and USB controllers that should be part of the graph. Any phys are
referenced with the phys binding and are not part of the graph.

Got it. So the new PHY definition would be just the same, but no
"ports".

quoted

        };
    };

    tcphy0_usb3: phy@db00000 {
        compatible = "soc,usb3-phy";
        reg = <0xdb00000 0x1000>;
        ports {
            port@0 {
                reg = <0>;
                usb3_phy_out: endpoint {
                    remote-endpoint = <&mux_usb3_in>;
                };
            };
        };
    };
};


// USB3 Host controller
usbdrd3_0: usb@fe800000 {
    compatible = "rockchip,rk3399-dwc3";
    #address-cells = <2>;
    #size-cells = <2>;
    clocks = ...;
    clock-names = ...;
    status = "disabled";

    usbdrd_dwc3_0: usb@fe800000 {
        compatible = "snps,dwc3";
        reg = <0x0 0xfe800000 0x0 0x100000>;
        interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH 0>;
        clocks = ...;
        clock-names = ...;
        dr_mode = "otg";
        phys = <&tcphy0_usb3>;
        phy-names = "usb3-phy";
        phy_type = "utmi_wide";
        power-domains = <&power RK3399_PD_USB3>;
        status = "disabled";

This means a port definition here:
            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    usb3_0_ep: endpoint {
                        remote-endpoint = <&mode_mux_out_usb3>;
                    };
                };
            };

quoted

    };
};

// DP controller
cdn_dp: dp@fec00000 {
    compatible = "rockchip,rk3399-cdn-dp";
    reg = <0x0 0xfec00000 0x0 0x100000>;
    interrupts = ...;
    clocks = ...;
    clock-names = ...;
    phys = <&tcphy0_dp>;
    ...
    ports {
        dp_in: port {
            #address-cells = <1>;
            #size-cells = <0>;

            dp_in_vopb: endpoint@0 {
                reg = <0>;
                remote-endpoint = <&vopb_out_dp>;
            };

            dp_in_vopl: endpoint@1 {
                reg = <1>;
                remote-endpoint = <&vopl_out_dp>;
            };
        };

This should have an output port and then that is connected to the USB
connector. Given that DP is muxed with the USB SS signals, port@1
(defined as USB SS) should then have 2 endpoints.

So, completing the example, another port here:

            dp_out: port@1 {
                reg = <1>;
                dp0_out_ep: endpoint {
                    remote-endpoint = <&mode_mux_out_dp>;
                };
            };

Then the only new thing here is how to represent the GPIO line
controlling the mux. Normally, I'd expect this to be in the parent of
the connector (the type-C controller), but since you have multiple
connectors, that doesn't work so well. So you can put 'mux-gpios' in
the port@1 node. (Or maybe it should be 'mux-controls' with a gpio mux
defined elsewhere).

I think the updated example better helps delineate what we're trying to
achieve. Per Heikki's earlier explanation (re-quoting here) :

"

quoted

quoted

quoted

The mode-switch here would actually represent the "consumer" part in
the mux-control bindings. So the mux-controls would describe the
relationship between the "mode-switch" and the mux controller(s),
while the mode-switch property describes the relationship between
something like USB Type-C Port Manager (or this cros_ec function) and
the "mux consumer".

"

So the device that "mode-switch" points to (in this case, foo_mux) is the "consumer" of
the mux-control direct consumer). Heikki, does this example better
demonstrate what the Type C connector class expects?

Also, I think it might be better to move these *-switch properties into the
usb-connector.yaml binding in the event that we decide to take this route,
since the "Chrome EC Type C port driver" isn't expected to use them directly.
Rob, kindly LMK if you'd prefer that, and I can make the change in the
next patch version.

I can re-write this example in a non-inline form if that would be easier
to interpret. Kindly LMK, and also LMK if this graph connection scheme
example is acceptable.

Thanks again, and best regards,

-Prashant

Rob