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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Wed, Jun 10, 2020 at 10:53:45AM -0600, Rob Herring wrote:
> On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> 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@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>;
> > >                     };
> > >                 };
> > >             };
> >
> > 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.

No, that's not what I'm trying to ask here. Bad example, sorry. I'm
trying to understand why is it necessary to slit the connector into
three separate interfaces? There does not seem to be anything in the
kernel that would benefit from that. Why isn't the connector described
as a single interface in devicetree?

My concern with the three separate interfaces is that they may force
us to know in kernel which of the three interfaces are association
with a mode, and actually not just the mode, but the possible the pin
configurations of the mode. That is something that we may end up
having to hard code into the drivers, even though it does not provide
any useful information to us, and that would not be OK.

Right now they also allow making assumptions regarding the alternate
modes since there are no "bindings" for those, for example, if these
OF graph ports have an endpoint to the DP, it means DP alt mode is
supported. But that is of course not true. DisplayPort is used also
with other alternate modes. We must never make any assumptions based
on those interfaces. So again, why do we have them?

Either I'm missing something, or the devicetree description of the
Type-C connectors really is way too complex, way too "low level",
causing us potential problems without providing anything that we could
actually ever use in the operating system.


thanks,

-- 
heikki