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

[Rob Herring]

On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > 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@xxxxxxxxxxxx> 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>;
> >                     };
> >                 };
> >             };
>
> 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