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

On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
[off-list ref] wrote:

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

quoted

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:

quoted

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

The pins that can be reassigned can in practice go anywhere. We can't
group them in any way. What do we do for example when the two sideband
pins go to different locations?

The sideband pins from the connector go to multiple places or the
sideband signal from a controller go to multiple connectors? Either
way, that's solved with multiple endpoints. In the former case, port@2
would have multiple endpoints with all the possible connections. The
general model of the graph is each port is a separate data channel and
multiple endpoints are either a mux or fanout depending on the data
direction.

It looks like the OF graph for the USB Type-C connectors expects the
pins to be grouped like that, which is too bad, because unfortunately
it will not work. It would require that we have a separate port for
every pin that can be reassigned on the connector, and let's not even
consider that.

I guess you are referring to the 4 SS signal pairs and that they could
be 2 USB links, 1 USB link and 1-2 Alt mode links, or 4 Alt mode
links. I think the grouping of SS signals is fine as I'd expect
there's a single entity deciding the routing. That would be 'mux' node
I think, but 'mux' is the wrong abstraction here. I guess we could
have 4 muxes (1 for each signal), but I'd hope we don't need that
level of detail in DT. I think we're in agreement on that.

We need higher level description of the connections for the USB Type-C
connectors. For example, a connector can be connected to this (or
these) DisplayPort(s), this USB 2.0 port, this USB 3.x port, this USB4
port, etc. And this is the mux that handles the pins on this
connector, and these are the retimers, etc. etc.

We also need a way to identify those connections, and relying on
something like fixed port node addresses, so indexes in practice,
feels really risky to me. A conflict may seem unlikely now, but we
seen those so many times in the past other things like GPIOs, IRQs,
etc. We really need to define string identifiers for these
connections.

I assume for IRQs you are referring to cases where we have a variable
number such as 'interrupts = <1 2 3>;' where 'interrupts = <1 3>'
doesn't work because we can't describe interrupt 2 is not present? The
graph binding doesn't suffer from that issue since we can easily omit
port or endpoint nodes.

Also, the numbering is specific to a compatible string. If we need
different numbering, then we can do a new compatible.

Rob