Hi Rob,

Thanks as always for your help in reviewing this proposal!

Kindly see inline

(Trimming text);
On Thu, Jun 11, 2020 at 02:00:47PM -0600, Rob Herring wrote:

On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani [off-list ref] wrote:

quoted

Hi Rob,

On Wed, Jun 10, 2020 at 9:53 AM Rob Herring [off-list ref] wrote:

quoted

quoted

On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:

I think the updated example handles this grouping (port@1 going to a
"SS mux") although as you said it should probably be a group of muxes,
but I think the example illustrates the point. Is that assessment
correct?

Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".

Ack.

Let's go with "-switch" ? That's what the connector class uses and it
conveys the meaning (unless that is a reserved keyword in DT).

quoted

Would this block the addition of the "*-switch" properties? IIUC the
two are related but not dependent on each other.

The *-switch properties are phandles which the Type C connector class
framework expects (and uses to get handles to those switches).
These would point to the "mux" or "group of mux" abstractions as noted earlier.

You don't need them though. Walk the graph. You get the connector
port@1 remote endpoint and then get its parent.

I see; would it be something along the lines of this? (DT example
follows; search for "example_end" to jump to bottom):

<example_start>

connector@0 {
    compatible = "usb-c-connector";
    reg = <0>;
    power-role = "dual";
    data-role = "dual";
    try-power-role = "source";
    ....
    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>;
            #address-cells = <1>;
            #size-cells = <0>;

            usb_con0_ss_mode: endpoint@0 {
                reg = <0>
                remote-endpoint = <&mode_switch_ss_in>;
            };

            usb_con0_ss_orientation: endpoint@1 {
                        reg = <1>
                        remote-endpoint = <&orientation_switch_ss_in>;
            };

            usb_con0_ss_data_role: endpoint@2 {
                        reg = <2>
                        remote-endpoint = <&data_role_switch_in>;
            };
        };

        port@2 {
            reg = <2>;
            #address-cells = <1>;
            #size-cells = <0>;
            usb_con0_sbu_mode: endpoint@0 {
                        reg = <0>
                        remote-endpoint = <&mode_switch_sbu_in>;
            };
            usb_con0_sbu_orientation: endpoint@1 {
                        reg = <1>
                        remote-endpoint = <&orientation_switch_sbu_in>;
            };
        };
    };
};

mode_switch {
    compatible = "typec-mode-switch";
    mux-controls = <&mode_mux_controller>;
    mux-control-names = "mode";
    #address-cells = <1>;
    #size-cells = <0>;

    port@0 {
        reg = <0>;
        mode_switch_ss_in: endpoint {
            remote-endpoint = <&usb_con0_ss_mode>
        };
    };

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

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

    port@3 {
        reg = <3>;
        mode_switch_sbu_in: endpoint {
            remote-endpoint = <&usb_con0_sbu_mode>
        };
    };
    // ... other ports similarly defined.
};

orientation_switch {
    compatible = "typec-orientation-switch";
    mux-controls = <&orientation_mux_controller>;
    mux-control-names = "orientation";
    #address-cells = <1>;
    #size-cells = <0>;

    port@0 {
        reg = <0>;
        orientation_switch_ss_in: endpoint {
            remote-endpoint = <&usb_con0_ss_orientation>
        };
    };

    port@1
        reg = <1>;
        orientation_switch_sbu_in: endpoint {
            remote-endpoint = <&usb_con0_sbu_orientation>
        };
    };
    // ... other ports similarly defined.
};

data_role_switch {
    compatible = "typec-data-role-switch";
    mux-controls = <&data_role_switch_controller>;
    mux-control-names = "data_role";

    port {
        data_role_switch_in: endpoint {
            remote-endpoint = <&usb_con0_ss_data_role>
        };
    };
};

<example_end>

Would this be conformant to OF graph and usb-connector bindings
requirements? We'll certainly send out a format PATCH/RFC series for
this, but I was hoping to gauge whether we're thinking along the right lines.

So, in effect this would mean:
- New bindings(and compatible strings) to be added for:
  typec-{orientation,data-role,mode}-switch.
- Handling in Type C connector class to parse switches from OF graph.
- Handling in Type C connector class for distinct switches for port@1
  (SS lines) and port@2 (SBU lines).

The only thing I'm confused about is how we can define these switch
remote-endpoint bindings in usb-connector.yaml; the port can have an
remote-endpoint, but can we specify what the parent of the remote-endpoint
should have as a compatible string? Or do we not need to?

Best regards,

-Prashant