Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200422222242.241699-1-pmalani@chromium.org> <20200511192800.GA28762@bogus>
 <20200511204635.GC136540@google.com> <20200512134154.GC2085641@kuha.fi.intel.com>
 <CAL_JsqJ2pbh5BbjGd9eEiD6-sV94=omk6o+mLXjCYiVnUOtO=g@mail.gmail.com>
 <CACeCKadiiokPdPB2Q5WBQFrPuxjpm3TiDgaaerncVR_Z7Z0nvg@mail.gmail.com>
 <CAL_Jsq+MM3-ugLvSGc_wc6RvHVyxyDUD0DkvwQaQJMYCCFpfHg@mail.gmail.com>
 <20200609235740.GA154315@google.com> <20200610153356.GC3213128@kuha.fi.intel.com>
 <CAL_JsqKsObFhC+J6gK2EDXdpBLO6t+rswXDipnjt4uMr2Qx2zg@mail.gmail.com> <20200612124634.GD3213128@kuha.fi.intel.com>
In-Reply-To: <20200612124634.GD3213128@kuha.fi.intel.com>
From:   Rob Herring <robh@kernel.org>
Date:   Fri, 12 Jun 2020 08:20:28 -0600
Message-ID: <CAL_Jsq+ORkzHchpD0qsH97zDJzXGj3jWy8=orXSVhNQd4kr9Kg@mail.gmail.com>
Subject: Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Prashant Malani <pmalani@chromium.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Tim Wawrzynczak <twawrzynczak@chromium.org>,
        Benson Leung <bleung@chromium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Guenter Roeck <groeck@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 12, 2020 at 6:46 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> 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?

Because it is easily 3 separate h/w components (nodes) that have a
link to the connector.

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

The connector was designed pretty much before there was any TypeC
support in the kernel. Bindings shouldn't be designed around the
*current* needs of a particular OS.

The simplest case for the connector would be:

usb@1234 {
  compatible = "vendor,some-usb-2and3-with-typec-controller";
  ...
  connector {
    compatible = "usb-type-c-connector";
    /* No ports! */
  };

In this case, the h/w for
"vendor,some-usb-2and3-with-typec-controller" can handle everything
for the connector. Doesn't need anything for alt modes because either
it is not supported or there's only one possible source.

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

Either you hard-code things in DT with "generic", low-level binding or
you hard-code things in a driver. Or maybe in your case, things are
hard-coded in the EC? But most platforms don't have that.

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

I'm pretty sure we have cases where the alt mode is HDMI. Maybe
there's not yet been a case of multiple alt modes til now. So now the
binding needs to be extended.

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

Well, all bindings are a balancing act of being flexible enough vs.
high-level enough to be stable. What I need is something that's going
to work for everyone, not just CrOS. Adding a new property at time is
death by 1000 cuts and usually a sign of someone only fixing their own
immediate problem.

Rob