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: <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>
 <CACeCKadq6tuqzR_6DuiZeL+=aOMb05EWd4o0sNyGOcZJ=dYx8g@mail.gmail.com>
 <CAL_JsqJQb5P26JC-KqkeHoWxAb63N+_XRK==b-WWJ+pYpdHO8Q@mail.gmail.com> <CACeCKacUa1-ttBmKS_Q_xZCsArgGWkB4s9eG0c5Lc5RHa1W35Q@mail.gmail.com>
In-Reply-To: <CACeCKacUa1-ttBmKS_Q_xZCsArgGWkB4s9eG0c5Lc5RHa1W35Q@mail.gmail.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Mon, 29 Jun 2020 13:41:48 -0700
Message-ID: <CACeCKachd34UtiZXY3i8za_ZHG9xtHSiPX55=Ed78n=S15b3Hg@mail.gmail.com>
Subject: Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props
To:     Rob Herring <robh@kernel.org>
Cc:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        "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>,
        Rajmohan Mani <rajmohan.mani@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Rob,

Just following up on this. Would the below example align better with
OF graph requirements?

Example begins at <example_start>, but in summary:
- port@1 (Superspeed) of usb-c-connector will have 3 endpoints (0 =
goes to mode switch, 1 = goes to orientation switch, 2 = goes to data
role switch)
- port@2 (SBU) of usb-c-connector will have 2 endpoints (0 = goes to
mode switch, 1 = goes to orientation switch)
-These end points can go through arbitrarily long paths (including
retimers) as long as they end up at the following devices:
    a. device with compatible string "typec-mode-switch" for endpoint 0.
    b. device with compatible string "typec-orientation-switch" for endpoint 1.
    c. device with compatible string "typec-data-role-switch" for endpoint 2.
- Connector class framework will perform the traversal from
usb-c-connector port endpoints to the "*-switch" devices.

Best regards,

On Fri, Jun 12, 2020 at 10:34 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Rob,
>
> Thanks as always for your help in reviewing this proposal!
>
> Kindly see inline
>
> (Trimming text);
> On Thu, Jun 11, 2020 at 02:00:47PM -0600, Rob Herring wrote:
> > On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > >
> > > I think the updated example handles this grouping (port@1 going to a
> > > "SS mux") although as you said it should probably be a group of muxes,
> > > but I think the example illustrates the point. Is that assessment
> > > correct?
> >
> > Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".
>
> Ack.
>
> Let's go with "-switch" ? That's what the connector class uses and it
> conveys the meaning (unless that is a reserved keyword in DT).
>
> >
> > > Would this block the addition of the "*-switch" properties? IIUC the
> > > two are related but not dependent on each other.
> > >
> > > The *-switch properties are phandles which the Type C connector class
> > > framework expects (and uses to get handles to those switches).
> > > These would point to the "mux" or "group of mux" abstractions as noted earlier.
> >
> > You don't need them though. Walk the graph. You get the connector
> > port@1 remote endpoint and then get its parent.
> >
>
> I see; would it be something along the lines of this? (DT example
> follows; search for "example_end" to jump to bottom):
>
> <example_start>
>
> connector@0 {
>     compatible = "usb-c-connector";
>     reg = <0>;
>     power-role = "dual";
>     data-role = "dual";
>     try-power-role = "source";
>     ....
>     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>;
>             #address-cells = <1>;
>             #size-cells = <0>;
>
>             usb_con0_ss_mode: endpoint@0 {
>                 reg = <0>
>                 remote-endpoint = <&mode_switch_ss_in>;
>             };
>
>             usb_con0_ss_orientation: endpoint@1 {
>                         reg = <1>
>                         remote-endpoint = <&orientation_switch_ss_in>;
>             };
>
>             usb_con0_ss_data_role: endpoint@2 {
>                         reg = <2>
>                         remote-endpoint = <&data_role_switch_in>;
>             };
>         };
>
>         port@2 {
>             reg = <2>;
>             #address-cells = <1>;
>             #size-cells = <0>;
>             usb_con0_sbu_mode: endpoint@0 {
>                         reg = <0>
>                         remote-endpoint = <&mode_switch_sbu_in>;
>             };
>             usb_con0_sbu_orientation: endpoint@1 {
>                         reg = <1>
>                         remote-endpoint = <&orientation_switch_sbu_in>;
>             };
>         };
>     };
> };
>
> mode_switch {
>     compatible = "typec-mode-switch";
>     mux-controls = <&mode_mux_controller>;
>     mux-control-names = "mode";
>     #address-cells = <1>;
>     #size-cells = <0>;
>
>     port@0 {
>         reg = <0>;
>         mode_switch_ss_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_mode>
>         };
>     };
>
>     port@1 {
>         reg = <1>;
>         mode_switch_out_usb3: endpoint {
>             remote-endpoint = <&usb3_0_ep>
>         };
>     };
>
>     port@2 {
>         reg = <2>;
>         mode_switch_out_dp: endpoint {
>             remote-endpoint = <&dp0_out_ep>
>         };
>     };
>
>     port@3 {
>         reg = <3>;
>         mode_switch_sbu_in: endpoint {
>             remote-endpoint = <&usb_con0_sbu_mode>
>         };
>     };
>     // ... other ports similarly defined.
> };
>
> orientation_switch {
>     compatible = "typec-orientation-switch";
>     mux-controls = <&orientation_mux_controller>;
>     mux-control-names = "orientation";
>     #address-cells = <1>;
>     #size-cells = <0>;
>
>     port@0 {
>         reg = <0>;
>         orientation_switch_ss_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_orientation>
>         };
>     };
>
>     port@1
>         reg = <1>;
>         orientation_switch_sbu_in: endpoint {
>             remote-endpoint = <&usb_con0_sbu_orientation>
>         };
>     };
>     // ... other ports similarly defined.
> };
>
> data_role_switch {
>     compatible = "typec-data-role-switch";
>     mux-controls = <&data_role_switch_controller>;
>     mux-control-names = "data_role";
>
>     port {
>         data_role_switch_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_data_role>
>         };
>     };
> };
>
> <example_end>
>
> Would this be conformant to OF graph and usb-connector bindings
> requirements? We'll certainly send out a format PATCH/RFC series for
> this, but I was hoping to gauge whether we're thinking along the right lines.
>
> So, in effect this would mean:
> - New bindings(and compatible strings) to be added for:
>   typec-{orientation,data-role,mode}-switch.
> - Handling in Type C connector class to parse switches from OF graph.
> - Handling in Type C connector class for distinct switches for port@1
>   (SS lines) and port@2 (SBU lines).
>
> The only thing I'm confused about is how we can define these switch
> remote-endpoint bindings in usb-connector.yaml; the port can have an
> remote-endpoint, but can we specify what the parent of the remote-endpoint
> should have as a compatible string? Or do we not need to?
>
> Best regards,
>
> -Prashant
>