Re: [PATCH v4 15/18] dt-bindings: usb: Add ports to google,cros-ec-typec for DP altmode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 31, 2024 at 02:45:29PM -0700, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2024-10-31 11:42:36)
> > On Tue, Oct 29, 2024 at 01:15:51PM -0700, Stephen Boyd wrote:
> > > At this point we need to tell the DP bridge, like IT6505, that it's one
> > > or the other output endpoints that it should use, but we haven't
> > > directly connected the usb-c-connector to the output ports of IT6505
> > > because drm_of_find_panel_or_bridge() can't find the parent of the
> > > usb-c-connector if we connect the DP bridge to the usb-c-connector
> > > graphs. We'll need a way for the bridge to know which output port is
> > > connected to a usb-c-connector fwnode. Some sort of API like
> >
> > I think that the final bridge should be the IT6505. It can save you from
> > some troubles, like the inter-bridge lane negotiation. Please remember
> > that using lanes 2-3 as primary lanes doesn't seem to fall into the
> > standard DisplayPort usage. It is documented by USB-C and only because
> > of the orientation switching.
> 
> If the final bridge is IT6505 then drm_of_find_panel_or_bridge() isn't
> called and I think we're OK. But then we have to traverse the
> remote-endpoint of the usb-c-connector to IT6505 in displayport.c in the
> Corsola case while knowing to look at the parent of the usb-c-connector
> node and traversing the remote-endpoint to the QMP phy in the Trogdor
> case. The logic in dp_altmode_probe() is like
> 
>   if (port@1/endpoint@1 exists in usb-c-connector)
>     connector_fwnode = port@1/endpoint@1/remote-endpoint
>   else if (cros-ec-typec/port exists)
>     connector_fwnode = cros-ec-typec/port@0/endpoint/remote-endpoint
>   else
>     original stuff

I'd say, definitely ugh. Maybe we can add the swnode with the
"displayport" property pointing back to the DP bridge / endpoint.

But... read below.

> If we have the crazy three usb-c-connector design it can still work
> because we'd have something like
> 
>   cros-ec-typec {
>     port {
>       dp_endpoint: endpoint {
>         remote-endpoint = <&dp_ml0_ml1>;
>       };
>     };
> 
>     usb-c-connector@0 {
>       port@1 {
>         endpoint {
>           remote-endpoint = <&hub_ss0>;
>        };
>        // Implicitly dp_ml0_ml1
>       };
>     };
>     usb-c-connector@1 {
>       port@1 {
>         endpoint@0 {
>           remote-endpoint = <&hub_ss1>;
>         };
>         endpoint@1 {
>           remote-endpoint = <&dp_ml2_ml3>;
>         };
>       };
>     };
>     usb-c-connector@2 {
>       port@1 {
>         endpoint {
>           remote-endpoint = <&hub_ss2>;
>         };
>        // Implicitly dp_ml0_ml1
>       };
>     };
>   };
> 
> (I like thinking about this 3 connector case because it combines both
> Trogdor and Corsola designs so I can talk about both cases at the same
> time)
> 
> I don't know what happens when we have 4 connectors though, with 2 going
> to one pair of lanes and 2 going to the other 2 lanes. Maybe it's better
> to always have a DP input port in cros-ec-typec to avoid this problem
> and map back to the endpoint explicitly.
> 
>   cros-ec-typec {
>     port {
>       dp_endpoint0: endpoint@0 {
>         remote-endpoint = <&dp_ml0_ml1>;
>       };
>       dp_endpoint1: endpoint@1 {
>         remote-endpoint = <&dp_ml2_ml3>;
>       };
>     };
> 
>     usb-c-connector@0 {
>       port@1 {
>         endpoint@0 {
>           remote-endpoint = <&hub_ss0>;
>        };
>        endpoint@1 {
>          remote-endpoint = <&dp_endpoint0>;
>        };
>       };
>     };
>     usb-c-connector@1 {
>       port@1 {
>         endpoint@0 {
>           remote-endpoint = <&hub_ss1>;
>         };
>         endpoint@1 {
>           remote-endpoint = <&dp_endpoint1>;
>         };
>       };
>     };
>     usb-c-connector@2 {
>       port@1 {
>         endpoint@0 {
>           remote-endpoint = <&hub_ss2>;
>         };
>         endpoint@1 {
>           remote-endpoint = <&dp_endpoint1>;
>         };
>       };
>     };
>   };
> 
> Or use a displayport property that goes to connector node itself so that
> we don't extend the graph binding of the usb-c-connector.
> 
>   cros-ec-typec {
>     usb-c-connector@0 {
>       altmodes {
>         displayport {
>           connector = <&dp_ml0_ml1>;

I think this has been frowned upon. Not exactly this, but adding the
displayport = <&foo>.

Thus it can only go to the swnode that is generated in software by the
cros-ec driver.

>         };
>       };
>       port@1 {
>         endpoint@0 {
>           remote-endpoint = <&hub_ss0>;
>        };
>       };
>     };
>     usb-c-connector@1 {
>       altmodes {
>         displayport {
>           connector = <&dp_ml2_ml3>;
>         };
>       };
>       port@1 {
>         endpoint {
>           remote-endpoint = <&hub_ss1>;
>         };
>       };
>     };
>     usb-c-connector@2 {
>       altmodes {
>         displayport {
>           connector = <&dp_ml2_ml3>;
>         };
>       };
>       port@1 {
>         endpoint {
>           remote-endpoint = <&hub_ss2>;
>         };
>       };
>     };
>   };
> 
>   it6505 {
>     ports {
>       port@1 {
>         dp_ml0_ml1: endpoint@0 {
>           remote-endpoint = <??>;
>         };
>         dp_ml2_ml3: endpoint@1 {
>           remote-endpoint = <??>;
>         };
>       };
>     };
>   };
> 
> The logic could look at a node like usb-c-connector@2, find
> altmodes/display node, and look for a 'connector' property that points
> at the endpoint of the last bridge. If we don't use the OF graph binding
> it makes it easier to point at the same endpoint in the QMP phy or the
> IT6505 graph from more than one usb-c-connector. This also makes it very
> clear that we intend to pass that fwnode as the 'connector_fwnode' to
> oob_hotplug_event().
> 
> If we want to actually populate the 'remote-endpoint' property of IT6505
> we will need to make a graph in cros-ec-typec.
> 
>   cros-ec-typec {
>     port {
>       dp_endpoint0: endpoint@0 {
>         remote-endpoint = <&dp_ml0_ml1>;
>       };
>       dp_endpoint1: endpoint@1 {
>         remote-endpoint = <&dp_ml2_ml3>;
>       };
>     };
>     usb-c-connector@0 {
>       altmodes {
>         displayport {
>           connector = <&dp_endpoint0>;
>         };
>       };
>       port@1 {
>         endpoint@0 {
>           remote-endpoint = <&hub_ss0>;
>        };
>       };
>     };
>     usb-c-connector@1 {
>       altmodes {
>         displayport {
>           connector = <&dp_endpoint1>;
>         };
>       };
>       port@1 {
>         endpoint {
>           remote-endpoint = <&hub_ss1>;
>         };
>       };
>     };
>     usb-c-connector@2 {
>       altmodes {
>         displayport {
>           connector = <&dp_endpoint1>;
>         };
>       };
>       port@1 {
>         endpoint {
>           remote-endpoint = <&hub_ss2>;
>         };
>       };
>     };
>   };
> 
>   it6505 {
>     ports {
>       port@1 {
>         dp_ml0_ml1: endpoint@0 {
>           remote-endpoint = <dp_endpoint0>;
>         };
>         dp_ml2_ml3: endpoint@1 {
>           remote-endpoint = <dp_endpoint1>;
>         };
>       };
>     };
>   };
> 
> and then the logic in displayport.c will have to check if the
> 'connector' property points at a graph endpoint, traverse that to the
> remote-endpoint, and consider that the connector_fwnode.
> 
> >
> > Maybe that's just it? Register DP_bridge (or QMP PHY) as
> > orientation-switch? Then you don't need any extra API for the lane
> > mapping? The cross-ec-typec can provide orientation information and the
> > USB-C-aware controller will follow the lane mapping.
> 
> I'm not really following but I don't think the DT binding discussed here
> prevents that.

I'm thinking about:

it6505 {
  orientation-switch;

  ports {
    port@1 {
      it6505_dp_out: remote-endpoint = <&cros_ec_dp>;
      data-lanes = <0 1>;
    };
  };
};

cros-ec {
  port {
    cross_ec_dp: remote-endpoint = <&it6505_dp_out>;
  };

  connector@0 {
    reg = <0>;
    cros,dp-orientation = "normal";

    ports {
      // all USB HS and SS ports as usual;
    };
  };

  connector@1 {
    reg = <1>;
    cros,dp-orientation = "reverse";

    ports {
      // all USB HS and SS ports as usual;
    };
  };

  connector@2 {
    reg = <2>;
    cros,dp-orientation = "reverse";

    ports {
      // all USB HS and SS ports as usual;
    };
  };

  connector@3 {
    reg = <3>;
    cros,dp-orientation = "normal";

    ports {
      // all USB HS and SS ports as usual;
    };
  };
};

The cros-ec registers single drm bridge which will generate HPD events
except on Trogdor, etc. At the same time, cros-ec requests the
typec_switch_get(). When the cros-ec detects that the connector@N it
being used for DP, it just generates corresponding typec_switch_set()
call, setting the orientation of the it6505 (or QMP PHY). The rest can
be handled either by EC's HPD code or by DP's HPD handler, the
orientation should already be a correct one.

So, yes. It requires adding the typec_switch_desc implementation _in_
the it6505 (or in any other component which handles the 0-1 or 2-3
selection). On the other hand as I wrote previously, the 0-1 / 2-3 is
the USB-C functionality, not the DP one.

[...]

> 
> >
> > > > > Corsola could work with this design, but we'll need to teach
> > > > > dp_altmode_probe() to look for the drm_bridge elsewhere besides as the
> > > > > parent of the usb-c-connector node. That implies using the 'displayport'
> > > > > property in the cros-ec-typec node or teaching dp_altmode_probe() to
> > > > > look for the port@1/endpoint@1 remote-endpoint handle in the
> > > > > usb-c-connector graph.
> > > > >
> > > > > Assuming the bindings you've presented here are fine and good and I got
> > > > > over the differences between Trogdor and Corsola, then I can make mostly
> > > > > everything work with the drm_connector_oob_hotplug_event() signature
> > > > > change from above and some tweaks to dp_altmode_probe() to look for
> > > > > port@1/endpoint@1 first because that's the "logical" DP input endpoint
> > > > > in the usb-c-connector binding's graph. Great! The final roadblock I'm
> > > > > at is that HPD doesn't work on Trogdor, so I can't signal HPD through
> > > > > the typec framework.
> > > >
> > > > Hmm, I thought that a normal DP's HPD GPIO works on the trogdor. Was I
> > > > misunderstanding it? But then we don't know, which USB-C connector
> > > > triggered the HPD...
> > >
> > > By HPD not working on Trogdor I mean that the EC doesn't tell the kernel
> > > about the state of HPD for a usb-c-connector in software. Instead, HPD
> > > is signaled directly to the DP controller in hardware via a GPIO. It is
> > > as you suspect, we don't know which USB-C connector has HPD unless we
> > > read the mux controlled by the EC and combine that with what the DP
> > > driver knows about the state of the HPD pin.
> >
> > I see. So the HPD event gets delivered to the DP controller, but we
> > really need some API to read the port? If it's not the
> > orientation-switch, of course.
> 
> Yes. This is needed to understand what USB type-c connector the DP
> signals should go to. In the case of Corsola/IT6505 it's needed to know
> which two lanes should be sent if both type-c connectors/ports are
> capable of DP altmode. On Corsola, the EC could tell the kernel that
> both ports are in DP altmode but the EC is also controlling the AUX
> channel mux that decides which usb-c-connector type-c port is actually
> displaying DP.

-- 
With best wishes
Dmitry




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux