Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani <pmalani@chromium.org>]

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 <pmalani@chromium.org> wrote:
> >
> > 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>;
                    };
                };
            };

> 
> >     };
> > };
> >
> > // 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>;
};

> 
> >     };
> > };
> >
> > // 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".

> 
> >         };
> >     };
> >
> >     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>;
                    };
                };
            };

> >     };
> > };
> >
> > // 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) :

"
> > > > 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