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