| Message ID | 20240901040658.157425-16-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | platform/chrome: Add DT USB/DP muxing/topology support | expand |
On Sat, 31 Aug 2024, Stephen Boyd wrote: > Add a DT graph binding to google,cros-ec-typec so that it can combine > DisplayPort (DP) and USB SuperSpeed (SS) data into a USB type-c endpoint > that is connected to the usb-c-connector node's SS endpoint. This also > allows us to connect the DP and USB nodes in the graph to the USB type-c > connectors, providing the full picture of the USB type-c data flows in > the system. > > Allow there to be multiple typec nodes underneath the EC node so that > one DT graph exists per DP bridge. The EC is actually controlling TCPCs > and redrivers that combine the DP and USB signals together so this more > accurately reflects the hardware design without introducing yet another > DT node underneath the EC for USB type-c. > > If the type-c ports are being shared between a single DP controller then > the ports need to know about each other and determine a policy to drive > DP to one type-c port. If the type-c ports each have their own dedicated > DP controller then they're able to operate independently and enter/exit > DP altmode independently as well. We can't connect the DP controller's > endpoint to one usb-c-connector port@1 endpoint and the USB controller's > endpoint to another usb-c-connector port@1 endpoint either because the > DP muxing case would have DP connected to two usb-c-connector endpoints > which the graph binding doesn't support. > > Therefore, one typec node is required per the capabilities of the type-c > port(s) being managed. This also lets us indicate which type-c ports the > DP controller is wired to. For example, if DP was connected to ports 0 > and 2, while port 1 was connected to another DP controller we wouldn't > be able to implement that without having some other DT property to > indicate which output ports are connected to the DP endpoint. > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > Cc: Conor Dooley <conor+dt@kernel.org> > Acked-by: Lee Jones <lee@kernel.org> > Cc: Benson Leung <bleung@chromium.org> > Cc: Guenter Roeck <groeck@chromium.org> > Cc: Prashant Malani <pmalani@chromium.org> > Cc: Tzung-Bi Shih <tzungbi@kernel.org> > Cc: <devicetree@vger.kernel.org> > Cc: <chrome-platform@lists.linux.dev> > Cc: Pin-yen Lin <treapking@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > .../bindings/mfd/google,cros-ec.yaml | 7 +- Acked-by: Lee Jones <lee@kernel.org> > .../bindings/usb/google,cros-ec-typec.yaml | 229 ++++++++++++++++++ > 2 files changed, 233 insertions(+), 3 deletions(-)
On Sat, Aug 31, 2024 at 09:06:53PM GMT, Stephen Boyd wrote: > Add a DT graph binding to google,cros-ec-typec so that it can combine > DisplayPort (DP) and USB SuperSpeed (SS) data into a USB type-c endpoint > that is connected to the usb-c-connector node's SS endpoint. This also > allows us to connect the DP and USB nodes in the graph to the USB type-c > connectors, providing the full picture of the USB type-c data flows in > the system. > > Allow there to be multiple typec nodes underneath the EC node so that > one DT graph exists per DP bridge. The EC is actually controlling TCPCs > and redrivers that combine the DP and USB signals together so this more > accurately reflects the hardware design without introducing yet another > DT node underneath the EC for USB type-c. > > If the type-c ports are being shared between a single DP controller then > the ports need to know about each other and determine a policy to drive > DP to one type-c port. If the type-c ports each have their own dedicated > DP controller then they're able to operate independently and enter/exit > DP altmode independently as well. We can't connect the DP controller's > endpoint to one usb-c-connector port@1 endpoint and the USB controller's > endpoint to another usb-c-connector port@1 endpoint either because the > DP muxing case would have DP connected to two usb-c-connector endpoints > which the graph binding doesn't support. > > Therefore, one typec node is required per the capabilities of the type-c > port(s) being managed. This also lets us indicate which type-c ports the > DP controller is wired to. For example, if DP was connected to ports 0 > and 2, while port 1 was connected to another DP controller we wouldn't > be able to implement that without having some other DT property to > indicate which output ports are connected to the DP endpoint. Based on our disccusions at LPC, here are several DT examples that seem sensible to implement this case and several related cases from other ChromeBooks. typec { compatible = "google,cros-ec-typec"; port { typec_dp_in: endpoint { remote-endpoint = <&usb_1_qmp_phy_out_dp>; }; }; usb_c0: connector@0 { compatible = "usb-c-connector"; reg = <0>; ports { port@0 { reg = <0>; usb_c0_hs_in: endpoint { remote-endpoint = <&usb_hub_dfp1_hs>; }; }; port@1 { reg = <1>; usb_c0_ss_in: endpoint { remote-endpoint = <&usb_hub_dfp1_ss>; }; }; }; }; usb_c1: connector@1 { compatible = "usb-c-connector"; reg = <1>; ports { port@0 { reg = <0>; usb_c1_hs_in: endpoint { remote-endpoint = <&usb_hub_dfp2_hs>; }; }; port@1 { reg = <1>; usb_c1_ss_in: endpoint { remote-endpoint = <&usb_hub_dfp2_ss>; }; }; }; }; }; &usb_1_qmpphy { ports { port@0 { endpoint@0 { data-lanes = <0 1>; // this might go to USB-3 hub }; usb_1_qmp_phy_out_dp: endpoint@1 { remote-endpoint = <&typec_dp_in>; data-lanes = <2 3>; }; } }; }; ------- typec { connector@0 { port@1 { endpoint@0 { remtoe = <&usb_hub_0>; }; endpoint@1 { remote = <&dp_bridge_out_0>; }; }; }; connector@1 { port@1 { endpoint@0 { remtoe = <&usb_hub_1>; }; endpoint@1 { remote = <&dp_bridge_out_1>; }; }; }; }; dp_bridge { ports { port@1 { dp_bridge_out_0: endpoint@0 { remote = <usb_c0_ss_dp>; data-lanes = <0 1>; }; dp_bridge_out_1: endpoint@1 { remote = <usb_c1_ss_dp>; data-lanes = <2 3>; }; }; }; }; ------- This one is really tough example, we didn't reach a conclusion here. If the EC doesn't handle lane remapping, dp_bridge has to get orientation-switch and mode-switch properties (as in the end it is the dp_bridge that handles reshuffling of the lanes for the Type-C). Per the DisplayPort standard the lanes are fixed (e.g. DPCD 101h explicitly names lane 0, lanes 0-1, lanes 0-1-2-3). typec { connector@0 { port@1 { endpoint@0 { remtoe = <&usb_hub_0>; }; endpoint@1 { remote = <&dp_bridge_out_0>; }; }; }; }; dp_bridge { orientation-switch; mode-switch; ports { port@1 { dp_bridge_out_0: endpoint { remote = <usb_c0_ss_dp>; data-lanes = <0 1 2 3>; }; }; }; }; ------- > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > Cc: Conor Dooley <conor+dt@kernel.org> > Acked-by: Lee Jones <lee@kernel.org> > Cc: Benson Leung <bleung@chromium.org> > Cc: Guenter Roeck <groeck@chromium.org> > Cc: Prashant Malani <pmalani@chromium.org> > Cc: Tzung-Bi Shih <tzungbi@kernel.org> > Cc: <devicetree@vger.kernel.org> > Cc: <chrome-platform@lists.linux.dev> > Cc: Pin-yen Lin <treapking@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > .../bindings/mfd/google,cros-ec.yaml | 7 +- > .../bindings/usb/google,cros-ec-typec.yaml | 229 ++++++++++++++++++ > 2 files changed, 233 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > index c991626dc22b..bbe28047d0c0 100644 > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > @@ -98,9 +98,6 @@ properties: > > gpio-controller: true > > - typec: > - $ref: /schemas/usb/google,cros-ec-typec.yaml# > - > ec-pwm: > $ref: /schemas/pwm/google,cros-ec-pwm.yaml# > deprecated: true > @@ -166,6 +163,10 @@ patternProperties: > type: object > $ref: /schemas/extcon/extcon-usbc-cros-ec.yaml# > > + "^typec(-[0-9])*$": > + type: object > + $ref: /schemas/usb/google,cros-ec-typec.yaml# > + > required: > - compatible > > diff --git a/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml > index 365523a63179..235b86da3cdd 100644 > --- a/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml > +++ b/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml > @@ -26,6 +26,106 @@ properties: > '#size-cells': > const: 0 > > + mux-gpios: > + description: GPIOs indicating which way the DP mux is steered > + maxItems: 1 > + > + no-hpd: > + description: Indicates this endpoint doesn't signal HPD for DisplayPort > + type: boolean > + > + mode-switch: > + $ref: usb-switch.yaml#properties/mode-switch > + > + orientation-switch: > + $ref: usb-switch.yaml#properties/orientation-switch > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: Output ports for combined DP and USB SS data > + patternProperties: > + "^endpoint@([0-8])$": > + $ref: usb-switch.yaml#/$defs/usbc-out-endpoint > + unevaluatedProperties: false > + > + anyOf: > + - required: > + - endpoint@0 > + - required: > + - endpoint@1 > + - required: > + - endpoint@2 > + - required: > + - endpoint@3 > + - required: > + - endpoint@4 > + - required: > + - endpoint@5 > + - required: > + - endpoint@6 > + - required: > + - endpoint@7 > + - required: > + - endpoint@8 > + > + port@1: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: > + Input port to receive USB SuperSpeed (SS) data > + patternProperties: > + "^endpoint@([0-8])$": > + $ref: usb-switch.yaml#/$defs/usbc-in-endpoint > + unevaluatedProperties: false > + > + anyOf: > + - required: > + - endpoint@0 > + - required: > + - endpoint@1 > + - required: > + - endpoint@2 > + - required: > + - endpoint@3 > + - required: > + - endpoint@4 > + - required: > + - endpoint@5 > + - required: > + - endpoint@6 > + - required: > + - endpoint@7 > + - required: > + - endpoint@8 > + > + port@2: > + $ref: /schemas/graph.yaml#/$defs/port-base > + description: > + Input port to receive DisplayPort (DP) data > + unevaluatedProperties: false > + > + properties: > + endpoint: > + $ref: usb-switch.yaml#/$defs/dp-endpoint > + unevaluatedProperties: false > + > + required: > + - endpoint > + > + required: > + - port@0 > + > + anyOf: > + - required: > + - port@1 > + - required: > + - port@2 > + > patternProperties: > '^connector@[0-9a-f]+$': > $ref: /schemas/connector/usb-connector.yaml# > @@ -35,6 +135,40 @@ patternProperties: > required: > - compatible > > +allOf: > + - if: > + required: > + - no-hpd > + then: > + properties: > + ports: > + required: > + - port@2 > + - if: > + required: > + - mux-gpios > + then: > + properties: > + ports: > + required: > + - port@2 > + - if: > + required: > + - orientation-switch > + then: > + properties: > + ports: > + required: > + - port@2 > + - if: > + required: > + - mode-switch > + then: > + properties: > + ports: > + required: > + - port@2 > + > additionalProperties: false > > examples: > @@ -50,6 +184,8 @@ examples: > > typec { > compatible = "google,cros-ec-typec"; > + orientation-switch; > + mode-switch; > > #address-cells = <1>; > #size-cells = <0>; > @@ -60,6 +196,99 @@ examples: > power-role = "dual"; > data-role = "dual"; > try-power-role = "source"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + usb_c0_hs: endpoint { > + remote-endpoint = <&usb_hub_dfp3_hs>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + usb_c0_ss: endpoint { > + remote-endpoint = <&cros_typec_c0_ss>; > + }; > + }; > + }; > + }; > + > + connector@1 { > + compatible = "usb-c-connector"; > + reg = <1>; > + power-role = "dual"; > + data-role = "dual"; > + try-power-role = "source"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + usb_c1_hs: endpoint { > + remote-endpoint = <&usb_hub_dfp2_hs>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + usb_c1_ss: endpoint { > + remote-endpoint = <&cros_typec_c1_ss>; > + }; > + }; > + }; > + }; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + cros_typec_c0_ss: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&usb_c0_ss>; > + data-lanes = <0 1 2 3>; > + }; > + > + cros_typec_c1_ss: endpoint@1 { > + reg = <1>; > + remote-endpoint = <&usb_c1_ss>; > + data-lanes = <2 3 0 1>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + usb_in_0: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&usb_ss_0_out>; > + }; > + > + usb_in_1: endpoint@1 { > + reg = <1>; > + remote-endpoint = <&usb_ss_1_out>; > + }; > + }; > + > + port@2 { > + reg = <2>; > + dp_in: endpoint { > + remote-endpoint = <&dp_phy>; > + data-lanes = <0 1>; > + }; > + }; > }; > }; > }; > -- > https://chromeos.dev >
Quoting Dmitry Baryshkov (2024-09-20 02:38:53) > On Sat, Aug 31, 2024 at 09:06:53PM GMT, Stephen Boyd wrote: > > Based on our disccusions at LPC, here are several DT examples that seem > sensible to implement this case and several related cases from other > ChromeBooks. (Apologies for getting back to this late. I've been brewing on this topic for a month; only appropriate for Halloween.) This is the Trogdor case, one DP controller with 2 lanes DP coming out that is steered with an analog mux controlled by the EC: > > typec { > compatible = "google,cros-ec-typec"; > > port { > typec_dp_in: endpoint { > remote-endpoint = <&usb_1_qmp_phy_out_dp>; > }; > }; > > usb_c0: connector@0 { > compatible = "usb-c-connector"; > reg = <0>; > > ports { > port@0 { > reg = <0>; > usb_c0_hs_in: endpoint { > remote-endpoint = <&usb_hub_dfp1_hs>; > }; > }; > > port@1 { > reg = <1>; > usb_c0_ss_in: endpoint { > remote-endpoint = <&usb_hub_dfp1_ss>; > }; > }; > }; > }; > > usb_c1: connector@1 { > compatible = "usb-c-connector"; > reg = <1>; > > ports { > port@0 { > reg = <0>; > usb_c1_hs_in: endpoint { > remote-endpoint = <&usb_hub_dfp2_hs>; > }; > }; > > port@1 { > reg = <1>; > usb_c1_ss_in: endpoint { > remote-endpoint = <&usb_hub_dfp2_ss>; > }; > }; > }; > }; > }; > > &usb_1_qmpphy { > ports { > port@0 { > endpoint@0 { > data-lanes = <0 1>; > // this might go to USB-3 hub > }; > > usb_1_qmp_phy_out_dp: endpoint@1 { > remote-endpoint = <&typec_dp_in>; > data-lanes = <2 3>; > }; > } > }; > }; > > ------- This is one Corsola case, one DP controller (IT6505) that's an external bridge that has 4 lanes DP but only 2 lanes of DP are usable at a time because 2 physical lanes are wired to one usb-c-connector while the other 2 physical lanes are wired to the other usb-c-connector. I say "one Corsola case" because there's also a DP bridge (ANX7625) on some Corsola variants that only has two DP lanes with a crosspoint switch that can send those DP lanes out of one of two pairs of lanes. The other two lanes are for USB3 output data if the part is connected to USB3 on the input side. I suspect this ANX7625 case can be described in the same way as below with two output endpoints and data-lanes describing which lanes are used for either DP endpoint. > > typec { > connector@0 { > port@1 { > endpoint@0 { > remtoe = <&usb_hub_0>; > }; > > endpoint@1 { > remote = <&dp_bridge_out_0>; > }; (TL;DR: I think I have a plan with the last paragraph in this section, so I'll hack on it for a bit to see what happens.) I'm not thrilled about having two endpoints in the SuperSpeed port@1 to hold both signals in the Corsola case but not the Trogdor case. The problem is that there's only one DP endpoint on Trogdor and we can't have two remote-endpoint properties in there pointing to either usb-c-connector. But then on Corsola we have two DP endpoints and they both can't go to one DP input node in the typec node's graph. To harmonize this the typec graph can have one DP input endpoint on Trogdor while the typec graph can have two DP input endpoints on Corsola. In both cases, the typec graph would have two USB input endpoints, and then we can connect output endpoints to each usb-c-connector node. It would be similar to my existing binding in this series, except now the typec DP port can have multiple input endpoints. I understand from our discussions at LPC that I should use drm_connector_oob_hotplug_event() from the displayport altmode driver (drivers/usb/typec/altmodes/displayport.c) to tell the DP bridge like ANX7625 or IT6505 which output endpoint should be displaying DP. I think dp_altmode_probe() looks at the usb-c-connector node's parent, e.g. typec in the examples above, for the drm_bridge. That means we'll need to register a drm_bridge in the cros-ec-typec compatible node? Or we need to use the 'displayport' property in cros-ec-typec to point at the drm_bridge node? Either way the problem seems to be that I need to associate one drm_bridge with two displayport altmode drivers and pass some fwnode handle to drm_connector_oob_hotplug_event() in a way that we can map that back to the right output endpoint in the DP bridge graph. That seems to imply that we need to pass the fwnode for the usb-c-connector in addition to the fwnode for the drm_bridge, so that the drm_bridge code can look at its DT graph and find the remote node connected. Basically something like this: void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode, struct fwnode_handle *usb_connector_fwnode, enum drm_connector_status status) (We might as well also pass the number of lanes here) 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. This series fixes that problem by "capturing" HPD state from the upstream drm_bridge, e.g. msm_dp, by hooking the struct drm_bridge_funcs::hpd_notify() path and injecting HPD into the typec messages received from the EC. That's a workaround to make the typec framework see HPD state changes that are otherwise invisible to the kernel. Newer firmwares actually tell us the state of HPD properly, but even then we have to read a gpio mux controlled by the EC to figure out which usb-c-connector is actually muxing DP when HPD changes on either typec_port. Having a drm_bridge in cros-ec-typec helped here because we could hook this path and signal HPD if we knew the firmware was fixed. If we don't have the drm_bridge anymore, I'm lost how to do this. Maybe the right answer here is to introduce a drm_connector_dp_typec structure that is created by the TCPM (cros-ec-typec) that sets a new DRM_BRIDGE_OP_DP_TYPEC bridge op flag? And then teach drm_bridge_connector about this new flag, similar to the HDMI one. The drm_bridge could implement some function that maps the typec_port (usb-c-connector) to the upstream drm_bridge (ANX7625) graph port, possibly all in drm_bridge_connector_oob_hotplug_event() so that nothing else really changes. It could also let us keep hooking the hpd_notify() path for the workaround needed on Trogdor. And finally it may let us harmonize the two DT bindings so that we only have one port@1/endpoint node in the usb-c-connector. > }; > }; > > connector@1 { > port@1 { > endpoint@0 { > remtoe = <&usb_hub_1>; > }; > > endpoint@1 { > remote = <&dp_bridge_out_1>; > }; > }; > }; > }; > > dp_bridge { > ports { > port@1 { > dp_bridge_out_0: endpoint@0 { > remote = <usb_c0_ss_dp>; > data-lanes = <0 1>; > }; > > dp_bridge_out_1: endpoint@1 { > remote = <usb_c1_ss_dp>; > data-lanes = <2 3>; > }; > }; > }; > }; > > ------- > > This one is really tough example, we didn't reach a conclusion here. > If the EC doesn't handle lane remapping, dp_bridge has to get > orientation-switch and mode-switch properties (as in the end it is the > dp_bridge that handles reshuffling of the lanes for the Type-C). Per the > DisplayPort standard the lanes are fixed (e.g. DPCD 101h explicitly > names lane 0, lanes 0-1, lanes 0-1-2-3). Are those logical or physical lanes? I think we'll punt on this one anyway though. We don't have any plans to do this orientation control mechanism so far. Previous attempts failed and we put an extra orientation switch control on the board to do the orientation flipping.
On Tue, Oct 22, 2024 at 06:15:47PM -0700, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2024-09-20 02:38:53) > > On Sat, Aug 31, 2024 at 09:06:53PM GMT, Stephen Boyd wrote: > > Either way the problem seems to be that I need to associate one > drm_bridge with two displayport altmode drivers and pass some fwnode > handle to drm_connector_oob_hotplug_event() in a way that we can map > that back to the right output endpoint in the DP bridge graph. That > seems to imply that we need to pass the fwnode for the usb-c-connector > in addition to the fwnode for the drm_bridge, so that the drm_bridge > code can look at its DT graph and find the remote node connected. > Basically something like this: > > void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode, > struct fwnode_handle > *usb_connector_fwnode, > enum drm_connector_status status) > > (We might as well also pass the number of lanes here) I think this is a bit of an overkill. The drm_connector_oob_hotplug_event() is fine as it is, it gets "fwnode_handle to report the event on". What needs to be changed (in my humble opinion) is the drm_connector_find_by_fwnode() function (or likely a new function is to be added): if it can not find drm_connector for the passed fwnode, it should look it up on the parent, then on parent's parent, etc, until we actually find the drm_connector (good) or we reach the root (sad). And finally after getting the drm_connector, the oob_hotplug_event() callback should also receive the fwnode argument. This way the connector (or the bridge) can identify the fwnode (aka usb-c-connector in our case) that caused the event. WDYT? > 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... > This series fixes that problem by "capturing" HPD state from the > upstream drm_bridge, e.g. msm_dp, by hooking the struct > drm_bridge_funcs::hpd_notify() path and injecting HPD into the typec > messages received from the EC. That's a workaround to make the typec > framework see HPD state changes that are otherwise invisible to the > kernel. Newer firmwares actually tell us the state of HPD properly, but > even then we have to read a gpio mux controlled by the EC to figure out > which usb-c-connector is actually muxing DP when HPD changes on either > typec_port. Having a drm_bridge in cros-ec-typec helped here because we > could hook this path and signal HPD if we knew the firmware was fixed. > If we don't have the drm_bridge anymore, I'm lost how to do this. It's probably okay to add one, but let me think if we can work without it. Can we make EC driver listen for that single HPD GPIO (by making it an actual GPIO rather than "dp_hot") and then react to it? > > Maybe the right answer here is to introduce a drm_connector_dp_typec > structure that is created by the TCPM (cros-ec-typec) that sets a new > DRM_BRIDGE_OP_DP_TYPEC bridge op flag? And then teach > drm_bridge_connector about this new flag, similar to the HDMI one. The > drm_bridge could implement some function that maps the typec_port > (usb-c-connector) to the upstream drm_bridge (ANX7625) graph port, > possibly all in drm_bridge_connector_oob_hotplug_event() so that nothing > else really changes. It could also let us keep hooking the hpd_notify() > path for the workaround needed on Trogdor. And finally it may let us > harmonize the two DT bindings so that we only have one port@1/endpoint > node in the usb-c-connector. I think we have lightly discussed adding drm_connector_displayport, so that part is okay. But my gut feeling is that there should be no _typec part in thart picture. The DRM framework shouldn't need to know all the Type-C details. > > > > }; > > }; > > > > connector@1 { > > port@1 { > > endpoint@0 { > > remtoe = <&usb_hub_1>; > > }; > > > > endpoint@1 { > > remote = <&dp_bridge_out_1>; > > }; > > }; > > }; > > }; > > > > dp_bridge { > > ports { > > port@1 { > > dp_bridge_out_0: endpoint@0 { > > remote = <usb_c0_ss_dp>; > > data-lanes = <0 1>; > > }; > > > > dp_bridge_out_1: endpoint@1 { > > remote = <usb_c1_ss_dp>; > > data-lanes = <2 3>; > > }; > > }; > > }; > > }; > > > > ------- > > > > This one is really tough example, we didn't reach a conclusion here. > > If the EC doesn't handle lane remapping, dp_bridge has to get > > orientation-switch and mode-switch properties (as in the end it is the > > dp_bridge that handles reshuffling of the lanes for the Type-C). Per the > > DisplayPort standard the lanes are fixed (e.g. DPCD 101h explicitly > > names lane 0, lanes 0-1, lanes 0-1-2-3). > > Are those logical or physical lanes? Physical lanes as far as I understand. > > I think we'll punt on this one anyway though. We don't have any plans to > do this orientation control mechanism so far. Previous attempts failed > and we put an extra orientation switch control on the board to do the > orientation flipping. Okay, it's definitely easier this way.
Quoting Dmitry Baryshkov (2024-10-25 03:49:36) > On Tue, Oct 22, 2024 at 06:15:47PM -0700, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2024-09-20 02:38:53) > > > On Sat, Aug 31, 2024 at 09:06:53PM GMT, Stephen Boyd wrote: > > > > > Either way the problem seems to be that I need to associate one > > drm_bridge with two displayport altmode drivers and pass some fwnode > > handle to drm_connector_oob_hotplug_event() in a way that we can map > > that back to the right output endpoint in the DP bridge graph. That > > seems to imply that we need to pass the fwnode for the usb-c-connector > > in addition to the fwnode for the drm_bridge, so that the drm_bridge > > code can look at its DT graph and find the remote node connected. > > Basically something like this: > > > > void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode, > > struct fwnode_handle > > *usb_connector_fwnode, > > enum drm_connector_status status) > > > > (We might as well also pass the number of lanes here) > > I think this is a bit of an overkill. > > The drm_connector_oob_hotplug_event() is fine as it is, it gets > "fwnode_handle to report the event on". Ok. I understand that drm_*() shouldn't know about USB or type-c in general. > > What needs to be changed (in my humble opinion) is the > drm_connector_find_by_fwnode() function (or likely a new function is to > be added): if it can not find drm_connector for the passed fwnode, it > should look it up on the parent, then on parent's parent, etc, until we > actually find the drm_connector (good) or we reach the root (sad). > > And finally after getting the drm_connector, the oob_hotplug_event() > callback should also receive the fwnode argument. This way the connector > (or the bridge) can identify the fwnode (aka usb-c-connector in our > case) that caused the event. > > WDYT? Ok I think I'm following along. The dp->connector_fwnode in displayport.c will always be the usb-c-connector node in your example? And that will search for the connector or bridge associated with that usb-c-connector node. Then when it comes time to call drm_connector_oob_hotplug_event() it will take the usb-c-connector fwnode as 'connector_fwnode' and in that function we'll make drm_connector_find_by_fwnode() implement the parent walk? The cros-ec-typec compatible node will register a drm_bridge in all cases, and that is the parent of the usb-c-connector node, so the walk will end there. Then you want to pass the usb-c-connector fwnode to connector->funcs->oob_hotplug_event()? So drm_bridge_connector_oob_hotplug_event() changes to also get the usb-c-connector fwnode. This plan should work. 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 fwnode_graph_get_endpoint_connected_to_fwnode(bridge_fwnode, usb_c_fwnode) that takes the bridge fwnode and traverses the graph to find the endpoint in that's connected to 'usb_c_fwnode'. That traversal API will need help from the intermediate node, cros-ec-typec, so maybe it is better as a drm_bridge API that uses some new drm_bridge_funcs callback. This way IT6505 can ask the bridge chain which output DP endpoint is actually associated with the connector fwnode it gets from the oob_hotplug_event() callback. Here's the two DT snippets that I've ended up with: typec { compatible = "google,cros-ec-typec"; ports { port@0 { reg = <0>; typec_dp_in: endpoint { remote-endpoint = <&usb_1_qmp_phy_out_dp>; }; }; port@1 { reg = <1>; typec_usb0_in: endpoint@0 { reg = <0>; remote-endpoint = <&usb_hub_dfp1_ss>; }; typec_usb1_in: endpoint@1 { reg = <1>; remote-endpoint = <&usb_hub_dfp2_ss>; }; } // This port is not really needed because we know the // mapping from input ports to usb-c-connectors port@2 { reg = <2>; typec0_out: endpoint@0 { reg = <0>; remote-endpoint = <&usb_c0_ss_in>; }; typec1_out: endpoint@1 { reg = <1>; remote-endpoint = <&usb_c1_ss_in>; }; } }; usb_c0: connector@0 { compatible = "usb-c-connector"; reg = <0>; ports { port@0 { reg = <0>; usb_c0_hs_in: endpoint { remote-endpoint = <&usb_hub_dfp1_hs>; }; }; port@1 { reg = <1>; usb_c0_ss_in: endpoint { remote-endpoint = <&typec0_out>; }; }; }; }; usb_c1: connector@1 { compatible = "usb-c-connector"; reg = <1>; ports { port@0 { reg = <0>; usb_c1_hs_in: endpoint { remote-endpoint = <&usb_hub_dfp2_hs>; }; }; port@1 { reg = <1>; usb_c1_ss_in: endpoint { remote-endpoint = <&typec1_out>; }; }; }; }; }; &usb_1_qmpphy { ports { port@0 { endpoint@0 { data-lanes = <0 1>; // this might go to USB-3 hub }; usb_1_qmp_phy_out_dp: endpoint@1 { remote-endpoint = <&typec_dp_in>; data-lanes = <2 3>; }; } }; }; ------- typec { ports { port@0 { reg = <0>; typec_dp0_in: endpoint@0 { reg = <0>; remote-endpoint = <&dp_bridge_out_0>; }; typec_dp1_in: endpoint@1 { reg = <1>; remote-endpoint = <&dp_bridge_out_1>; }; }; port@1 { reg = <1>; typec_usb0_in: endpoint@0 { reg = <0>; remote-endpoint = <&usb_hub_0_ss>; }; typec_usb1_in: endpoint@1 { reg = <1>; remote-endpoint = <&usb_hub_1_ss>; }; } }; connector@0 { port@1 { endpoint@0 { remote-endpoint = <&usb_hub_0_hs>; }; }; }; connector@1 { port@1 { endpoint@0 { remote-endpoint = <&usb_hub_1_hs>; }; }; }; }; dp_bridge { ports { port@1 { dp_bridge_out_0: endpoint@0 { remote-endpoint = <&typec_dp0_in>; data-lanes = <0 1>; }; dp_bridge_out_1: endpoint@1 { remote-endpoint = <&typec_dp1_in>; data-lanes = <2 3>; }; }; }; }; ------- I wonder about a case where we may take two lanes and connect them to a usb-c-connector and then take the other two lanes and send them through a mux to two more usb-c-connectors. I think we'll need another property in that case that indicates which input DP endpoints correspond to the usb-c-connector nodes. typec { ports { port@0 { reg = <0>; typec_dp0_in: endpoint@0 { reg = <0>; remote-endpoint = <&dp_bridge_out_0>; }; typec_dp1_in: endpoint@1 { reg = <1>; remote-endpoint = <&dp_bridge_out_1>; }; }; port@1 { reg = <1>; typec_usb0_in: endpoint@0 { reg = <0>; remote-endpoint = <&usb_hub_0_ss>; }; typec_usb1_in: endpoint@1 { reg = <1>; remote-endpoint = <&usb_hub_1_ss>; }; typec_usb2_in: endpoint@2 { reg = <2>; remote-endpoint = <&usb_hub_2_ss>; }; } }; dp-2-usb-mapping = <0 0>, <1 1>, <1 2>; }; This property would indicate dp endpoint 0 goes to usb-c-connector 0 while dp endpoint 1 goes to usb-c-connector 1 and 2. I don't have this hardware but I could see how someone might do this by adding another mux that the EC controls. I don't want to design a binding and have to rework it in the future to handle this new case. I hope adding a new property, or getting more information from the EC firmware, will be sufficient to describe the linkage between the DP endpoint and the connectors managed by the cros-ec-typec device. > > > 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. > > > This series fixes that problem by "capturing" HPD state from the > > upstream drm_bridge, e.g. msm_dp, by hooking the struct > > drm_bridge_funcs::hpd_notify() path and injecting HPD into the typec > > messages received from the EC. That's a workaround to make the typec > > framework see HPD state changes that are otherwise invisible to the > > kernel. Newer firmwares actually tell us the state of HPD properly, but > > even then we have to read a gpio mux controlled by the EC to figure out > > which usb-c-connector is actually muxing DP when HPD changes on either > > typec_port. Having a drm_bridge in cros-ec-typec helped here because we > > could hook this path and signal HPD if we knew the firmware was fixed. > > If we don't have the drm_bridge anymore, I'm lost how to do this. > > It's probably okay to add one, but let me think if we can work without > it. Can we make EC driver listen for that single HPD GPIO (by making it > an actual GPIO rather than "dp_hot") and then react to it? I don't know how we handle the attention message, HPD_IRQ, because that's a short pulse that the kernel may miss when this is a GPIO that has to be triggered when both falling and rising. When the pin goes directly to the DP controller this is fine because the hardware can detect that. Similarly, when the EC sends the message about an HPD_IRQ we can replay that into the type-c framework and signal attention through drm_connector_oob_hotplug_event()/hpd_notify() paths. > > > > > Maybe the right answer here is to introduce a drm_connector_dp_typec > > structure that is created by the TCPM (cros-ec-typec) that sets a new > > DRM_BRIDGE_OP_DP_TYPEC bridge op flag? And then teach > > drm_bridge_connector about this new flag, similar to the HDMI one. The > > drm_bridge could implement some function that maps the typec_port > > (usb-c-connector) to the upstream drm_bridge (ANX7625) graph port, > > possibly all in drm_bridge_connector_oob_hotplug_event() so that nothing > > else really changes. It could also let us keep hooking the hpd_notify() > > path for the workaround needed on Trogdor. And finally it may let us > > harmonize the two DT bindings so that we only have one port@1/endpoint > > node in the usb-c-connector. > > I think we have lightly discussed adding drm_connector_displayport, so > that part is okay. But my gut feeling is that there should be no _typec > part in thart picture. The DRM framework shouldn't need to know all the > Type-C details. > Alright, got it.
On Tue, Oct 29, 2024 at 01:15:51PM -0700, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2024-10-25 03:49:36) > > On Tue, Oct 22, 2024 at 06:15:47PM -0700, Stephen Boyd wrote: > > > Quoting Dmitry Baryshkov (2024-09-20 02:38:53) > > > > On Sat, Aug 31, 2024 at 09:06:53PM GMT, Stephen Boyd wrote: > > > > > > > > Either way the problem seems to be that I need to associate one > > > drm_bridge with two displayport altmode drivers and pass some fwnode > > > handle to drm_connector_oob_hotplug_event() in a way that we can map > > > that back to the right output endpoint in the DP bridge graph. That > > > seems to imply that we need to pass the fwnode for the usb-c-connector > > > in addition to the fwnode for the drm_bridge, so that the drm_bridge > > > code can look at its DT graph and find the remote node connected. > > > Basically something like this: > > > > > > void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode, > > > struct fwnode_handle > > > *usb_connector_fwnode, > > > enum drm_connector_status status) > > > > > > (We might as well also pass the number of lanes here) > > > > I think this is a bit of an overkill. > > > > The drm_connector_oob_hotplug_event() is fine as it is, it gets > > "fwnode_handle to report the event on". > > Ok. I understand that drm_*() shouldn't know about USB or type-c in > general. > > > > > What needs to be changed (in my humble opinion) is the > > drm_connector_find_by_fwnode() function (or likely a new function is to > > be added): if it can not find drm_connector for the passed fwnode, it > > should look it up on the parent, then on parent's parent, etc, until we > > actually find the drm_connector (good) or we reach the root (sad). > > > > And finally after getting the drm_connector, the oob_hotplug_event() > > callback should also receive the fwnode argument. This way the connector > > (or the bridge) can identify the fwnode (aka usb-c-connector in our > > case) that caused the event. > > > > WDYT? > > Ok I think I'm following along. The dp->connector_fwnode in > displayport.c will always be the usb-c-connector node in your example? > And that will search for the connector or bridge associated with that > usb-c-connector node. Then when it comes time to call > drm_connector_oob_hotplug_event() it will take the usb-c-connector > fwnode as 'connector_fwnode' and in that function we'll make > drm_connector_find_by_fwnode() implement the parent walk? The > cros-ec-typec compatible node will register a drm_bridge in all cases, > and that is the parent of the usb-c-connector node, so the walk will end > there. > > Then you want to pass the usb-c-connector fwnode to > connector->funcs->oob_hotplug_event()? So > drm_bridge_connector_oob_hotplug_event() changes to also get the > usb-c-connector fwnode. This plan should work. Yes, that was my proposal. This way we know the origin device (bridge / connector) and let it take care of any details on its own. > 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. 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. > > fwnode_graph_get_endpoint_connected_to_fwnode(bridge_fwnode, usb_c_fwnode) > > that takes the bridge fwnode and traverses the graph to find the > endpoint in that's connected to 'usb_c_fwnode'. That traversal API will > need help from the intermediate node, cros-ec-typec, so maybe it is > better as a drm_bridge API that uses some new drm_bridge_funcs callback. > This way IT6505 can ask the bridge chain which output DP endpoint is > actually associated with the connector fwnode it gets from the > oob_hotplug_event() callback. > > Here's the two DT snippets that I've ended up with: > > typec { > compatible = "google,cros-ec-typec"; > > ports { > port@0 { > reg = <0>; > typec_dp_in: endpoint { > remote-endpoint = <&usb_1_qmp_phy_out_dp>; > }; > }; > > port@1 { > reg = <1>; > typec_usb0_in: endpoint@0 { > reg = <0>; > remote-endpoint = <&usb_hub_dfp1_ss>; > }; > typec_usb1_in: endpoint@1 { > reg = <1>; > remote-endpoint = <&usb_hub_dfp2_ss>; > }; > } > > // This port is not really needed because we know the > // mapping from input ports to usb-c-connectors > port@2 { > reg = <2>; > typec0_out: endpoint@0 { > reg = <0>; > remote-endpoint = <&usb_c0_ss_in>; > }; > typec1_out: endpoint@1 { > reg = <1>; > remote-endpoint = <&usb_c1_ss_in>; > }; > } Why do we need these two ports? Can't &usb_hub_dfpN_ss be connected directly to the usb_cN_ss_in? I understand that you probably want to express the internal structure of the lane switching, but I think that's a bit of the overkill. Leaving this to the other commenters / DT maintainers. > }; > > usb_c0: connector@0 { > compatible = "usb-c-connector"; > reg = <0>; > > ports { > port@0 { > reg = <0>; > usb_c0_hs_in: endpoint { > remote-endpoint = <&usb_hub_dfp1_hs>; > }; > }; > > port@1 { > reg = <1>; > usb_c0_ss_in: endpoint { > remote-endpoint = <&typec0_out>; > }; > }; > }; > }; > > usb_c1: connector@1 { > compatible = "usb-c-connector"; > reg = <1>; > > ports { > port@0 { > reg = <0>; > usb_c1_hs_in: endpoint { > remote-endpoint = <&usb_hub_dfp2_hs>; > }; > }; > > port@1 { > reg = <1>; > usb_c1_ss_in: endpoint { > remote-endpoint = <&typec1_out>; > }; > }; > }; > }; > }; > > &usb_1_qmpphy { > ports { > port@0 { > endpoint@0 { > data-lanes = <0 1>; > // this might go to USB-3 hub > }; > > usb_1_qmp_phy_out_dp: endpoint@1 { > remote-endpoint = <&typec_dp_in>; > data-lanes = <2 3>; > }; > } > }; > }; > > ------- > > typec { > ports { > port@0 { > reg = <0>; > typec_dp0_in: endpoint@0 { > reg = <0>; > remote-endpoint = <&dp_bridge_out_0>; > }; > typec_dp1_in: endpoint@1 { > reg = <1>; > remote-endpoint = <&dp_bridge_out_1>; > }; > }; > > port@1 { > reg = <1>; > typec_usb0_in: endpoint@0 { > reg = <0>; > remote-endpoint = <&usb_hub_0_ss>; > }; > typec_usb1_in: endpoint@1 { > reg = <1>; > remote-endpoint = <&usb_hub_1_ss>; > }; > } > }; > > connector@0 { > port@1 { > endpoint@0 { > remote-endpoint = <&usb_hub_0_hs>; port@1 is for SS lanes, so something is wrong here. > }; > }; > }; > > connector@1 { > port@1 { > endpoint@0 { > remote-endpoint = <&usb_hub_1_hs>; > }; > }; > }; > }; > > dp_bridge { > ports { > port@1 { > dp_bridge_out_0: endpoint@0 { > remote-endpoint = <&typec_dp0_in>; > data-lanes = <0 1>; > }; > > dp_bridge_out_1: endpoint@1 { > remote-endpoint = <&typec_dp1_in>; > data-lanes = <2 3>; > }; > }; > }; > }; > > ------- > > I wonder about a case where we may take two lanes and connect them to a > usb-c-connector and then take the other two lanes and send them through > a mux to two more usb-c-connectors. I think we'll need another property > in that case that indicates which input DP endpoints correspond to the > usb-c-connector nodes. > > typec { > ports { > port@0 { > reg = <0>; > typec_dp0_in: endpoint@0 { > reg = <0>; > remote-endpoint = <&dp_bridge_out_0>; > }; > typec_dp1_in: endpoint@1 { > reg = <1>; > remote-endpoint = <&dp_bridge_out_1>; > }; > }; > > port@1 { > reg = <1>; > typec_usb0_in: endpoint@0 { > reg = <0>; > remote-endpoint = <&usb_hub_0_ss>; > }; > typec_usb1_in: endpoint@1 { > reg = <1>; > remote-endpoint = <&usb_hub_1_ss>; > }; > typec_usb2_in: endpoint@2 { > reg = <2>; > remote-endpoint = <&usb_hub_2_ss>; > }; > } > }; > > dp-2-usb-mapping = <0 0>, <1 1>, <1 2>; dp-to-typec-mapping? > }; > > This property would indicate dp endpoint 0 goes to usb-c-connector 0 > while dp endpoint 1 goes to usb-c-connector 1 and 2. I don't have this > hardware but I could see how someone might do this by adding another mux > that the EC controls. I don't want to design a binding and have to > rework it in the future to handle this new case. I hope adding a new > property, or getting more information from the EC firmware, will be > sufficient to describe the linkage between the DP endpoint and the > connectors managed by the cros-ec-typec device. Does it change anything from the DP point of view? It is still either lanes 0-1 or lanes 2-3? I'd really like to inject the hotplug OOB event to the dp_bridge letting it get one of the endpoints as a HPD even source. > > > 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. > > > > > > This series fixes that problem by "capturing" HPD state from the > > > upstream drm_bridge, e.g. msm_dp, by hooking the struct > > > drm_bridge_funcs::hpd_notify() path and injecting HPD into the typec > > > messages received from the EC. That's a workaround to make the typec > > > framework see HPD state changes that are otherwise invisible to the > > > kernel. Newer firmwares actually tell us the state of HPD properly, but > > > even then we have to read a gpio mux controlled by the EC to figure out > > > which usb-c-connector is actually muxing DP when HPD changes on either > > > typec_port. Having a drm_bridge in cros-ec-typec helped here because we > > > could hook this path and signal HPD if we knew the firmware was fixed. > > > If we don't have the drm_bridge anymore, I'm lost how to do this. > > > > It's probably okay to add one, but let me think if we can work without > > it. Can we make EC driver listen for that single HPD GPIO (by making it > > an actual GPIO rather than "dp_hot") and then react to it? > > I don't know how we handle the attention message, HPD_IRQ, because > that's a short pulse that the kernel may miss when this is a GPIO that > has to be triggered when both falling and rising. When the pin goes > directly to the DP controller this is fine because the hardware can > detect that. Similarly, when the EC sends the message about an HPD_IRQ > we can replay that into the type-c framework and signal attention > through drm_connector_oob_hotplug_event()/hpd_notify() paths. Ack. > > > > > > > > > Maybe the right answer here is to introduce a drm_connector_dp_typec > > > structure that is created by the TCPM (cros-ec-typec) that sets a new > > > DRM_BRIDGE_OP_DP_TYPEC bridge op flag? And then teach > > > drm_bridge_connector about this new flag, similar to the HDMI one. The > > > drm_bridge could implement some function that maps the typec_port > > > (usb-c-connector) to the upstream drm_bridge (ANX7625) graph port, > > > possibly all in drm_bridge_connector_oob_hotplug_event() so that nothing > > > else really changes. It could also let us keep hooking the hpd_notify() > > > path for the workaround needed on Trogdor. And finally it may let us > > > harmonize the two DT bindings so that we only have one port@1/endpoint > > > node in the usb-c-connector. > > > > I think we have lightly discussed adding drm_connector_displayport, so > > that part is okay. But my gut feeling is that there should be no _typec > > part in thart picture. The DRM framework shouldn't need to know all the > > Type-C details. > > > > Alright, got it.
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 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>; }; }; 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. > > > > > fwnode_graph_get_endpoint_connected_to_fwnode(bridge_fwnode, usb_c_fwnode) > > > > that takes the bridge fwnode and traverses the graph to find the > > endpoint in that's connected to 'usb_c_fwnode'. That traversal API will > > need help from the intermediate node, cros-ec-typec, so maybe it is > > better as a drm_bridge API that uses some new drm_bridge_funcs callback. > > This way IT6505 can ask the bridge chain which output DP endpoint is > > actually associated with the connector fwnode it gets from the > > oob_hotplug_event() callback. > > > > Here's the two DT snippets that I've ended up with: > > > > typec { > > compatible = "google,cros-ec-typec"; > > > > ports { > > port@0 { > > reg = <0>; > > typec_dp_in: endpoint { > > remote-endpoint = <&usb_1_qmp_phy_out_dp>; > > }; > > }; > > > > port@1 { > > reg = <1>; > > typec_usb0_in: endpoint@0 { > > reg = <0>; > > remote-endpoint = <&usb_hub_dfp1_ss>; > > }; > > typec_usb1_in: endpoint@1 { > > reg = <1>; > > remote-endpoint = <&usb_hub_dfp2_ss>; > > }; > > } > > > > // This port is not really needed because we know the > > // mapping from input ports to usb-c-connectors > > port@2 { > > reg = <2>; > > typec0_out: endpoint@0 { > > reg = <0>; > > remote-endpoint = <&usb_c0_ss_in>; > > }; > > typec1_out: endpoint@1 { > > reg = <1>; > > remote-endpoint = <&usb_c1_ss_in>; > > }; > > } > > Why do we need these two ports? Can't &usb_hub_dfpN_ss be connected > directly to the usb_cN_ss_in? I understand that you probably want to > express the internal structure of the lane switching, but I think that's > a bit of the overkill. Leaving this to the other commenters / DT > maintainers. We don't need port@2 because we know that DP goes there. > > > }; > > > > usb_c0: connector@0 { > > compatible = "usb-c-connector"; > > reg = <0>; > > > > ports { > > port@0 { > > reg = <0>; > > usb_c0_hs_in: endpoint { > > remote-endpoint = <&usb_hub_dfp1_hs>; > > }; > > }; > > > > port@1 { > > reg = <1>; > > usb_c0_ss_in: endpoint { > > remote-endpoint = <&typec0_out>; > > }; > > }; > > }; > > }; > > > > usb_c1: connector@1 { > > compatible = "usb-c-connector"; > > reg = <1>; > > > > ports { > > port@0 { > > reg = <0>; > > usb_c1_hs_in: endpoint { > > remote-endpoint = <&usb_hub_dfp2_hs>; > > }; > > }; > > > > port@1 { > > reg = <1>; > > usb_c1_ss_in: endpoint { > > remote-endpoint = <&typec1_out>; > > }; > > }; > > }; > > }; > > }; > > > > &usb_1_qmpphy { > > ports { > > port@0 { > > endpoint@0 { > > data-lanes = <0 1>; > > // this might go to USB-3 hub > > }; > > > > usb_1_qmp_phy_out_dp: endpoint@1 { > > remote-endpoint = <&typec_dp_in>; > > data-lanes = <2 3>; > > }; > > } > > }; > > }; > > > > ------- > > > > typec { > > ports { > > port@0 { > > reg = <0>; > > typec_dp0_in: endpoint@0 { > > reg = <0>; > > remote-endpoint = <&dp_bridge_out_0>; > > }; > > typec_dp1_in: endpoint@1 { > > reg = <1>; > > remote-endpoint = <&dp_bridge_out_1>; > > }; > > }; > > > > port@1 { > > reg = <1>; > > typec_usb0_in: endpoint@0 { > > reg = <0>; > > remote-endpoint = <&usb_hub_0_ss>; > > }; > > typec_usb1_in: endpoint@1 { > > reg = <1>; > > remote-endpoint = <&usb_hub_1_ss>; > > }; > > } > > }; > > > > connector@0 { > > port@1 { > > endpoint@0 { > > remote-endpoint = <&usb_hub_0_hs>; > > port@1 is for SS lanes, so something is wrong here. I meant port@0 > > > }; > > }; > > }; > > > > connector@1 { > > port@1 { > > endpoint@0 { > > remote-endpoint = <&usb_hub_1_hs>; > > }; > > }; > > }; > > }; > > > > dp_bridge { > > ports { > > port@1 { > > dp_bridge_out_0: endpoint@0 { > > remote-endpoint = <&typec_dp0_in>; > > data-lanes = <0 1>; > > }; > > > > dp_bridge_out_1: endpoint@1 { > > remote-endpoint = <&typec_dp1_in>; > > data-lanes = <2 3>; > > }; > > }; > > }; > > }; > > > > ------- > > > > I wonder about a case where we may take two lanes and connect them to a > > usb-c-connector and then take the other two lanes and send them through > > a mux to two more usb-c-connectors. I think we'll need another property > > in that case that indicates which input DP endpoints correspond to the > > usb-c-connector nodes. > > > > typec { > > ports { > > port@0 { > > reg = <0>; > > typec_dp0_in: endpoint@0 { > > reg = <0>; > > remote-endpoint = <&dp_bridge_out_0>; > > }; > > typec_dp1_in: endpoint@1 { > > reg = <1>; > > remote-endpoint = <&dp_bridge_out_1>; > > }; > > }; > > > > port@1 { > > reg = <1>; > > typec_usb0_in: endpoint@0 { > > reg = <0>; > > remote-endpoint = <&usb_hub_0_ss>; > > }; > > typec_usb1_in: endpoint@1 { > > reg = <1>; > > remote-endpoint = <&usb_hub_1_ss>; > > }; > > typec_usb2_in: endpoint@2 { > > reg = <2>; > > remote-endpoint = <&usb_hub_2_ss>; > > }; > > } > > }; > > > > dp-2-usb-mapping = <0 0>, <1 1>, <1 2>; > > dp-to-typec-mapping? Sure > > > }; > > > > This property would indicate dp endpoint 0 goes to usb-c-connector 0 > > while dp endpoint 1 goes to usb-c-connector 1 and 2. I don't have this > > hardware but I could see how someone might do this by adding another mux > > that the EC controls. I don't want to design a binding and have to > > rework it in the future to handle this new case. I hope adding a new > > property, or getting more information from the EC firmware, will be > > sufficient to describe the linkage between the DP endpoint and the > > connectors managed by the cros-ec-typec device. > > Does it change anything from the DP point of view? It is still either > lanes 0-1 or lanes 2-3? I'd really like to inject the hotplug OOB event > to the dp_bridge letting it get one of the endpoints as a HPD even > source. Nothing changes from the DP point of view. I understand that you want the bridge to see one of its endpoints as the 'connector_fwnode' passed to oob_hotplug_event(). > > > > > 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.
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.
Quoting Dmitry Baryshkov (2024-10-31 15:54:49) > 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: > > > > 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>. Do you have a pointer to that discussion? I'd like to understand the reasoning. > > Thus it can only go to the swnode that is generated in software by the > cros-ec driver. I recall swnode as a way to sidestep figuring out the DT bindings for usb typec. Where is this swnode being made? Somewhere inside the typec framework? > > > }; > > }; > > port@1 { > > endpoint@0 { > > remote-endpoint = <&hub_ss0>; > > }; > > }; > > }; > > usb-c-connector@1 { > > altmodes { > > displayport { > > connector = <&dp_ml2_ml3>; > > }; > > }; > > port@1 { > > endpoint { [....] > > > > > > > > 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. > I don't think we should be adding typec code to pure display hardware drivers like IT6505. To keep the driver focused on display stuff I proposed implementing runtime lane assignment for drm_bridge chains because DP has lanes. My understanding is that not all display technologies have lanes, so implementing generic lane assignment functionality is overkill/incorrect. DP has physical lanes in hardware though, and those physical lanes are assigned to certain pins in the type-c DP altmode spec, so it's not overkill to think about lanes when the bridge is a DP bridge wired up to a type-c connector. Long story short, I don't see how we can avoid _any_ lane assignment logic in drm_bridge. The logic shouldn't walk the entire bridge chain, but it should at least act on the bridge that is a DP bridge. I think you're saying pretty much the same thing here, but you want the lane remapping to be done via the typec layer whereas I want it to be done in the drm_bridge layer. To me it looks out of place to add a typec_switch_desc inside each DP drm_bridge because we duplicate the logic about USB type-c DP altmode lane assignment to each DP bridge. A DP bridge should just think about DP and not know or care about USB type-c. This is what's leading me to think we need some sort of lane assignment capability at the DP connector. How that assignment flows from the DP connector created in drm_bridge_connector.c to the hardware is where it is less clear to me. Should that be implemented as a typec_switch_desc, essentially out of band with drm_bridge, or as some drm_bridge_funcs function similar to struct drm_bridge_funcs::hdmi_*()? If you look at IT6505 in it6505_get_extcon_property() it actually wants to pull the orientation of the type-c port with extcon_get_property(EXTCON_DISP_DP, EXTCON_PROP_USB_TYPEC_POLARITY). Maybe pushing the orientation to the DP bridge is backwards and we should be exposing this as some sort of connector API that the drm_bridge can query whenever it wants. What about ANX7625 where two DP lanes go to a cross-point switch before leaving the chip on one of two pairs of lanes? This hardware is a DP bridge smashed together with an orientation switch (typec_switch_desc) so that you can simply wire the output pins up to a USB type-c connector and support 2 lanes DP altmode. Qualcomm's QMP phy is quite similar. Presumably we'd want the ANX driver to implement both a drm_bridge and a typec_switch_desc if it was directly connected to a usb-c-connector node. It's also interesting to think of the DT binding here, likely we would have one output port in the ANX node's graph that represents the combined DP and USB data that's connected to the SuperSpeed endpoint in the usb-c-connector. In the case where two lanes are wired to one USB type-c connector and the other two lanes are wired to a different USB type-c connector it would be odd to keep the typec_switch_desc and figure out a way to mangle the lanes we want for a USB type-c connector by setting the orientation of the typec_switch_desc. The chip isn't really acting as a typec orientation control here because it isn't combining USB data and DP data for a single USB type-c port. In fact, the type-c port has an orientation and we actively don't want to tell the ANX7625 driver about that port orientation because the orientation control is implemented between the ANX part and the type-c connector by some redriver controlled by the EC. To satisfy all these cases it almost feels like we need to make the DP connector have an "orientation", per your earlier DT snippet it would be "reversed" or "normal", even though in hardware a DP connector has no such concept because it can only be plugged in one way. All cases look to be covered if we say that the drm_connector can have an orientation, "normal" or "reversed", and we allow the bridge drivers to query that whenever they want with some bridge/connector API. The typical case will be that the orientation is normal, but we can make drm_connector_oob_hotplug_event() change that to "reversed" when the port is different. This leaves us with the binding you propose above, and then some sort of property that indicates the orientation of the DP connector. Instead of being vendor specific I wonder if we can simply have a property like "dp-reverse-orientation" in the connector node that the displayport.c driver can look for to set the connector orientation to the reverse one when DP altmode is entered on the port. This is what I have: it6505 { 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>; ports { // all USB HS and SS ports as usual; }; }; connector@1 { reg = <1>; dp-reverse-orientation; ports { // all USB HS and SS ports as usual; }; }; or ANX, swap out for it6505 node: anx7625 { ports { port@1 { anx7625_dp_out: remote-endpoint = <&cros_ec_dp>; data-lanes = <0 1>; }; }; }; and then a drm_bridge is created in cros-ec to terminate the bridge chain. The displayport altmode driver will find the drm_bridge and the drm_connector from the cros-ec node. When DP altmode is entered the displayport altmode driver will set the drm_connector orientation based on the presence of the dp-reverse-orientation property. We'll be able to hook the hpd_notify() path in cros-ec by adding code to the drm_bridge made there to do the HPD workaround. I'm not sure we need to use an auxiliary device in this case, because it's a one-off solution for cros-ec. And we don't even need to signal HPD from the cros-ec drm_bridge because the oob_hotplug event will do it for us. If anything, we need that displayport.c code to skip sending the hotplug event when "no-hpd" is present in the cros-ec node. Note, this works for any number of usb-c-connector nodes. And finally, DP bridges like IT6505 don't need to implement a typec_switch_desc, they can simply support flipping the orientation by querying the drm_connector for the bridge chain when they see fit. ANX7625 can support that as well when it doesn't see the 'orientation-switch' property. Did I miss anything? I suspect a drm_connector having an orientation is the most controversial part of this proposal.
On Thu, Nov 07, 2024 at 04:28:24PM -0800, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2024-10-31 15:54:49) > > 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: > > > > > > 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>. > > Do you have a pointer to that discussion? I'd like to understand the > reasoning. No, unfortunately I couldn't find it. > > > > > > Thus it can only go to the swnode that is generated in software by the > > cros-ec driver. > > I recall swnode as a way to sidestep figuring out the DT bindings for > usb typec. Where is this swnode being made? Somewhere inside the typec > framework? In 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 { > [....] > > > > > > > > > > > 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. > > > > I don't think we should be adding typec code to pure display hardware > drivers like IT6505. To keep the driver focused on display stuff I > proposed implementing runtime lane assignment for drm_bridge chains > because DP has lanes. My understanding is that not all display > technologies have lanes, so implementing generic lane assignment > functionality is overkill/incorrect. DP has physical lanes in hardware > though, and those physical lanes are assigned to certain pins in the > type-c DP altmode spec, so it's not overkill to think about lanes when > the bridge is a DP bridge wired up to a type-c connector. DisplayPort has fixed lanes assignment in the standard. So any driver that reassigns / reallocates DisplayPort lanes dynamically implements Type-C functionality. > Long story short, I don't see how we can avoid _any_ lane assignment > logic in drm_bridge. The logic shouldn't walk the entire bridge chain, > but it should at least act on the bridge that is a DP bridge. I think > you're saying pretty much the same thing here, but you want the lane > remapping to be done via the typec layer whereas I want it to be done in > the drm_bridge layer. To me it looks out of place to add a > typec_switch_desc inside each DP drm_bridge because we duplicate the > logic about USB type-c DP altmode lane assignment to each DP bridge. A > DP bridge should just think about DP and not know or care about USB > type-c. > > This is what's leading me to think we need some sort of lane assignment > capability at the DP connector. How that assignment flows from the DP > connector created in drm_bridge_connector.c to the hardware is where it > is less clear to me. Should that be implemented as a typec_switch_desc, > essentially out of band with drm_bridge, or as some drm_bridge_funcs > function similar to struct drm_bridge_funcs::hdmi_*()? If you look at > IT6505 in it6505_get_extcon_property() it actually wants to pull the > orientation of the type-c port with extcon_get_property(EXTCON_DISP_DP, > EXTCON_PROP_USB_TYPEC_POLARITY). Maybe pushing the orientation to the DP > bridge is backwards and we should be exposing this as some sort of > connector API that the drm_bridge can query whenever it wants. And it6505_get_extcon_property() / EXTCON_PROP_USB_TYPEC_POLARITY is a Type-C code, isn't it? > What about ANX7625 where two DP lanes go to a cross-point switch before > leaving the chip on one of two pairs of lanes? This hardware is a DP > bridge smashed together with an orientation switch (typec_switch_desc) > so that you can simply wire the output pins up to a USB type-c connector > and support 2 lanes DP altmode. Qualcomm's QMP phy is quite similar. > Presumably we'd want the ANX driver to implement both a drm_bridge and a > typec_switch_desc if it was directly connected to a usb-c-connector > node. It's also interesting to think of the DT binding here, likely we > would have one output port in the ANX node's graph that represents the > combined DP and USB data that's connected to the SuperSpeed endpoint in > the usb-c-connector. > > In the case where two lanes are wired to one USB type-c connector and > the other two lanes are wired to a different USB type-c connector it > would be odd to keep the typec_switch_desc and figure out a way to > mangle the lanes we want for a USB type-c connector by setting the > orientation of the typec_switch_desc. The chip isn't really acting as a > typec orientation control here because it isn't combining USB data and > DP data for a single USB type-c port. In fact, the type-c port has an > orientation and we actively don't want to tell the ANX7625 driver about > that port orientation because the orientation control is implemented > between the ANX part and the type-c connector by some redriver > controlled by the EC. > > To satisfy all these cases it almost feels like we need to make the DP > connector have an "orientation", per your earlier DT snippet it would be > "reversed" or "normal", even though in hardware a DP connector has no > such concept because it can only be plugged in one way. All cases look > to be covered if we say that the drm_connector can have an orientation, > "normal" or "reversed", and we allow the bridge drivers to query that > whenever they want with some bridge/connector API. The typical case will > be that the orientation is normal, but we can make > drm_connector_oob_hotplug_event() change that to "reversed" when the > port is different. The DP connector doesn't have the orientation, as you pointed out. Only Type-C does. > > This leaves us with the binding you propose above, and then some sort of > property that indicates the orientation of the DP connector. Instead of > being vendor specific I wonder if we can simply have a property like > "dp-reverse-orientation" in the connector node that the displayport.c > driver can look for to set the connector orientation to the reverse one > when DP altmode is entered on the port. > > This is what I have: > > it6505 { > 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>; > > ports { > // all USB HS and SS ports as usual; > }; > }; > > connector@1 { > reg = <1>; > dp-reverse-orientation; > > ports { > // all USB HS and SS ports as usual; > }; > }; > > or ANX, swap out for it6505 node: > > anx7625 { > ports { > port@1 { > anx7625_dp_out: remote-endpoint = <&cros_ec_dp>; > data-lanes = <0 1>; > }; > }; > }; > > and then a drm_bridge is created in cros-ec to terminate the bridge > chain. The displayport altmode driver will find the drm_bridge and the > drm_connector from the cros-ec node. When DP altmode is entered the > displayport altmode driver will set the drm_connector orientation based > on the presence of the dp-reverse-orientation property. We'll be able to > hook the hpd_notify() path in cros-ec by adding code to the drm_bridge > made there to do the HPD workaround. I'm not sure we need to use an > auxiliary device in this case, because it's a one-off solution for > cros-ec. And we don't even need to signal HPD from the cros-ec > drm_bridge because the oob_hotplug event will do it for us. If anything, > we need that displayport.c code to skip sending the hotplug event when > "no-hpd" is present in the cros-ec node. Note, this works for any number > of usb-c-connector nodes. And finally, DP bridges like IT6505 don't need > to implement a typec_switch_desc, they can simply support flipping the > orientation by querying the drm_connector for the bridge chain when they > see fit. ANX7625 can support that as well when it doesn't see the > 'orientation-switch' property. > > Did I miss anything? I suspect a drm_connector having an orientation is > the most controversial part of this proposal. Yes... I understand that having orientation-switch handling in the DRM driver sounds strange, but this is what we do in the QMP PHY driver. It makes the code easier, as it keeps lane remapping local to the place where it belongs - to the Type-C handlers.
Quoting Dmitry Baryshkov (2024-11-08 23:05:18) > On Thu, Nov 07, 2024 at 04:28:24PM -0800, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2024-10-31 15:54:49) > > > 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: > > Long story short, I don't see how we can avoid _any_ lane assignment > > logic in drm_bridge. The logic shouldn't walk the entire bridge chain, > > but it should at least act on the bridge that is a DP bridge. I think > > you're saying pretty much the same thing here, but you want the lane > > remapping to be done via the typec layer whereas I want it to be done in > > the drm_bridge layer. To me it looks out of place to add a > > typec_switch_desc inside each DP drm_bridge because we duplicate the > > logic about USB type-c DP altmode lane assignment to each DP bridge. A > > DP bridge should just think about DP and not know or care about USB > > type-c. > > > > This is what's leading me to think we need some sort of lane assignment > > capability at the DP connector. How that assignment flows from the DP > > connector created in drm_bridge_connector.c to the hardware is where it > > is less clear to me. Should that be implemented as a typec_switch_desc, > > essentially out of band with drm_bridge, or as some drm_bridge_funcs > > function similar to struct drm_bridge_funcs::hdmi_*()? If you look at > > IT6505 in it6505_get_extcon_property() it actually wants to pull the > > orientation of the type-c port with extcon_get_property(EXTCON_DISP_DP, > > EXTCON_PROP_USB_TYPEC_POLARITY). Maybe pushing the orientation to the DP > > bridge is backwards and we should be exposing this as some sort of > > connector API that the drm_bridge can query whenever it wants. > > And it6505_get_extcon_property() / EXTCON_PROP_USB_TYPEC_POLARITY is a > Type-C code, isn't it? > Sort of? It's combining DP and USB_TYPEC enums there so it's not very clear if it's one or the other instead of just both. > > and then a drm_bridge is created in cros-ec to terminate the bridge > > chain. The displayport altmode driver will find the drm_bridge and the > > drm_connector from the cros-ec node. When DP altmode is entered the > > displayport altmode driver will set the drm_connector orientation based > > on the presence of the dp-reverse-orientation property. We'll be able to > > hook the hpd_notify() path in cros-ec by adding code to the drm_bridge > > made there to do the HPD workaround. I'm not sure we need to use an > > auxiliary device in this case, because it's a one-off solution for > > cros-ec. And we don't even need to signal HPD from the cros-ec > > drm_bridge because the oob_hotplug event will do it for us. If anything, > > we need that displayport.c code to skip sending the hotplug event when > > "no-hpd" is present in the cros-ec node. Note, this works for any number > > of usb-c-connector nodes. And finally, DP bridges like IT6505 don't need > > to implement a typec_switch_desc, they can simply support flipping the > > orientation by querying the drm_connector for the bridge chain when they > > see fit. ANX7625 can support that as well when it doesn't see the > > 'orientation-switch' property. > > > > Did I miss anything? I suspect a drm_connector having an orientation is > > the most controversial part of this proposal. > > Yes... I understand that having orientation-switch handling in the DRM > driver sounds strange, but this is what we do in the QMP PHY driver. It > makes the code easier, as it keeps lane remapping local to the place > where it belongs - to the Type-C handlers. > The QMP PHY is a type-c PHY, similar to ANX7625. It sits on the output of the DP and USB PHYs and handles the type-c orientation and lane merging for different USB type-c alternate modes. It's not a great example of a plain DP bridge because it combines USB and USB type-c features. Either way, doing this through Type-C handlers is weird because the port orientation in the Type-C framework is for the connector and there is an orientation control hardware that handles the orientation already. For example, with the IT6505 part on Corsola, the orientation is controlled by a redriver part that the EC controls. It takes the DP and USB signals and routes them to the correct pins on the usb-c-connector depending on the cable orientation. The input side pinout is basically 2 or 4 lanes DP and 2 lanes USB and the output side pinout is the USB type-c pinout SSTXRX1 and SSTXRX2. This redriver is equivalent to the QMP PHY type-c part. Maybe to bring this example closer to QMP we can imagine if the QMP PHY was split into two pairs of lanes, and the USB functionality wasn't used. The orientation control for a usb-c-connector would be on a redriver that takes 2 DP lanes from the QMP PHY as input. Saying that this QMP PHY is the "orientation-switch" with that property in DT is confusing, because it isn't controlling the orientation of the type-c port. The orientation is handled by the redriver. That redriver may even be controlled by the kernel as an orientation-switch. I understand that the QMP PHY driver has implemented the lane control for orientation with a typec_switch_desc, but the QMP PHY is a plain DP PHY in this scenario. How would the type-c handlers work here? We couldn't call them through the type-c framework as far as I can tell. This is why I'm thinking the end of the bridge chain needs to have some sort of orientation. If we had that then the place where the chain ends and becomes muxed onto the usb-c-connector, i.e. the redriver, would be where the DP bridge is told that it needs to flip the lanes. In the cases I have, the redriver is the EC, and so we've combined them all together in one node, cros-ec-typec. In the QMP PHY case the redriver is the QMP PHY type-c part that sits on the DP and USB PHYs and sends their signals out of the SoC. Maybe the DT property in the ANX7625 or IT6505 node should be something like "dp-orientation-switch" and then we have the type-c framework find this property? Then we would need to add support for that property in IT6505 using a typec_switch_desc, which is weird. I guess it all feels like a hack because it's not always the case that the DP PHY is glued to a USB type-c PHY.
On Mon, Nov 11, 2024 at 06:16:27PM -0800, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2024-11-08 23:05:18) > > On Thu, Nov 07, 2024 at 04:28:24PM -0800, Stephen Boyd wrote: > > > Quoting Dmitry Baryshkov (2024-10-31 15:54:49) > > > > 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: > > > Long story short, I don't see how we can avoid _any_ lane assignment > > > logic in drm_bridge. The logic shouldn't walk the entire bridge chain, > > > but it should at least act on the bridge that is a DP bridge. I think > > > you're saying pretty much the same thing here, but you want the lane > > > remapping to be done via the typec layer whereas I want it to be done in > > > the drm_bridge layer. To me it looks out of place to add a > > > typec_switch_desc inside each DP drm_bridge because we duplicate the > > > logic about USB type-c DP altmode lane assignment to each DP bridge. A > > > DP bridge should just think about DP and not know or care about USB > > > type-c. > > > > > > This is what's leading me to think we need some sort of lane assignment > > > capability at the DP connector. How that assignment flows from the DP > > > connector created in drm_bridge_connector.c to the hardware is where it > > > is less clear to me. Should that be implemented as a typec_switch_desc, > > > essentially out of band with drm_bridge, or as some drm_bridge_funcs > > > function similar to struct drm_bridge_funcs::hdmi_*()? If you look at > > > IT6505 in it6505_get_extcon_property() it actually wants to pull the > > > orientation of the type-c port with extcon_get_property(EXTCON_DISP_DP, > > > EXTCON_PROP_USB_TYPEC_POLARITY). Maybe pushing the orientation to the DP > > > bridge is backwards and we should be exposing this as some sort of > > > connector API that the drm_bridge can query whenever it wants. > > > > And it6505_get_extcon_property() / EXTCON_PROP_USB_TYPEC_POLARITY is a > > Type-C code, isn't it? > > > > Sort of? It's combining DP and USB_TYPEC enums there so it's not very > clear if it's one or the other instead of just both. But EXTCON_PROP_USB_TYPEC_POLARITY is just a Type-C, nothing about DP in it. > > > > and then a drm_bridge is created in cros-ec to terminate the bridge > > > chain. The displayport altmode driver will find the drm_bridge and the > > > drm_connector from the cros-ec node. When DP altmode is entered the > > > displayport altmode driver will set the drm_connector orientation based > > > on the presence of the dp-reverse-orientation property. We'll be able to > > > hook the hpd_notify() path in cros-ec by adding code to the drm_bridge > > > made there to do the HPD workaround. I'm not sure we need to use an > > > auxiliary device in this case, because it's a one-off solution for > > > cros-ec. And we don't even need to signal HPD from the cros-ec > > > drm_bridge because the oob_hotplug event will do it for us. If anything, > > > we need that displayport.c code to skip sending the hotplug event when > > > "no-hpd" is present in the cros-ec node. Note, this works for any number > > > of usb-c-connector nodes. And finally, DP bridges like IT6505 don't need > > > to implement a typec_switch_desc, they can simply support flipping the > > > orientation by querying the drm_connector for the bridge chain when they > > > see fit. ANX7625 can support that as well when it doesn't see the > > > 'orientation-switch' property. > > > > > > Did I miss anything? I suspect a drm_connector having an orientation is > > > the most controversial part of this proposal. > > > > Yes... I understand that having orientation-switch handling in the DRM > > driver sounds strange, but this is what we do in the QMP PHY driver. It > > makes the code easier, as it keeps lane remapping local to the place > > where it belongs - to the Type-C handlers. > > > > The QMP PHY is a type-c PHY, similar to ANX7625. It sits on the output > of the DP and USB PHYs and handles the type-c orientation and lane > merging for different USB type-c alternate modes. It's not a great > example of a plain DP bridge because it combines USB and USB type-c > features. > > Either way, doing this through Type-C handlers is weird because the port > orientation in the Type-C framework is for the connector and there is an > orientation control hardware that handles the orientation already. For > example, with the IT6505 part on Corsola, the orientation is controlled > by a redriver part that the EC controls. It takes the DP and USB signals > and routes them to the correct pins on the usb-c-connector depending on > the cable orientation. The input side pinout is basically 2 or 4 lanes > DP and 2 lanes USB and the output side pinout is the USB type-c pinout > SSTXRX1 and SSTXRX2. > > This redriver is equivalent to the QMP PHY type-c part. Maybe to bring > this example closer to QMP we can imagine if the QMP PHY was split into > two pairs of lanes, and the USB functionality wasn't used. The > orientation control for a usb-c-connector would be on a redriver that > takes 2 DP lanes from the QMP PHY as input. Saying that this QMP PHY is > the "orientation-switch" with that property in DT is confusing, because > it isn't controlling the orientation of the type-c port. The orientation > is handled by the redriver. That redriver may even be controlled by the > kernel as an orientation-switch. This is clear. > > I understand that the QMP PHY driver has implemented the lane control > for orientation with a typec_switch_desc, but the QMP PHY is a plain DP > PHY in this scenario. How would the type-c handlers work here? We > couldn't call them through the type-c framework as far as I can tell. If QMP PHY is a plain DP PHY, it usually has no support for lane remapping (e.g. phy-qcom-edp doesn't). Let me reiterate, please: lane management is outside of the DisplayPort spec, at least as far as I can understand it. All lane remapping (especially a dynamic one) is a pure vendor extension to the standard. I'm trying to find a way to support Corsola and Trogdor without adding "this is done specially for Google" kind of API. Usually that doesn't fly in the long term. I understand that using Type-C API for the DRM bridge sounds strange. But even the mentioned bridge uses Type-C API. It asks for the Type-C polarity, not the DP polarity. > This is why I'm thinking the end of the bridge chain needs to have some > sort of orientation. If we had that then the place where the chain ends > and becomes muxed onto the usb-c-connector, i.e. the redriver, would be > where the DP bridge is told that it needs to flip the lanes. In the > cases I have, the redriver is the EC, and so we've combined them all > together in one node, cros-ec-typec. In the QMP PHY case the redriver is > the QMP PHY type-c part that sits on the DP and USB PHYs and sends their > signals out of the SoC. > > Maybe the DT property in the ANX7625 or IT6505 node should be something > like "dp-orientation-switch" and then we have the type-c framework find > this property? Then we would need to add support for that property in > IT6505 using a typec_switch_desc, which is weird. I guess it all feels > like a hack because it's not always the case that the DP PHY is glued to > a USB type-c PHY. I think just "orientation-switch" is enough. In the end it's not a "typec-orientation-switch".
Quoting Dmitry Baryshkov (2024-11-15 09:17:15) > On Mon, Nov 11, 2024 at 06:16:27PM -0800, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2024-11-08 23:05:18) > > > On Thu, Nov 07, 2024 at 04:28:24PM -0800, Stephen Boyd wrote: > > > > Quoting Dmitry Baryshkov (2024-10-31 15:54:49) > > > > > 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: > > > > Long story short, I don't see how we can avoid _any_ lane assignment > > > > logic in drm_bridge. The logic shouldn't walk the entire bridge chain, > > > > but it should at least act on the bridge that is a DP bridge. I think > > > > you're saying pretty much the same thing here, but you want the lane > > > > remapping to be done via the typec layer whereas I want it to be done in > > > > the drm_bridge layer. To me it looks out of place to add a > > > > typec_switch_desc inside each DP drm_bridge because we duplicate the > > > > logic about USB type-c DP altmode lane assignment to each DP bridge. A > > > > DP bridge should just think about DP and not know or care about USB > > > > type-c. > > > > > > > > This is what's leading me to think we need some sort of lane assignment > > > > capability at the DP connector. How that assignment flows from the DP > > > > connector created in drm_bridge_connector.c to the hardware is where it > > > > is less clear to me. Should that be implemented as a typec_switch_desc, > > > > essentially out of band with drm_bridge, or as some drm_bridge_funcs > > > > function similar to struct drm_bridge_funcs::hdmi_*()? If you look at > > > > IT6505 in it6505_get_extcon_property() it actually wants to pull the > > > > orientation of the type-c port with extcon_get_property(EXTCON_DISP_DP, > > > > EXTCON_PROP_USB_TYPEC_POLARITY). Maybe pushing the orientation to the DP > > > > bridge is backwards and we should be exposing this as some sort of > > > > connector API that the drm_bridge can query whenever it wants. > > > > > > And it6505_get_extcon_property() / EXTCON_PROP_USB_TYPEC_POLARITY is a > > > Type-C code, isn't it? > > > > > > > Sort of? It's combining DP and USB_TYPEC enums there so it's not very > > clear if it's one or the other instead of just both. > > But EXTCON_PROP_USB_TYPEC_POLARITY is just a Type-C, nothing about DP in it. It's extcon_get_property(it6505->extcon, EXTCON_DISP_DP, EXTCON_PROP_USB_TYPEC_POLARITY, ...) which has EXTCON_DISP_DP in there, so there's something about DP there. That's all I'm saying. > > > > I understand that the QMP PHY driver has implemented the lane control > > for orientation with a typec_switch_desc, but the QMP PHY is a plain DP > > PHY in this scenario. How would the type-c handlers work here? We > > couldn't call them through the type-c framework as far as I can tell. > > If QMP PHY is a plain DP PHY, it usually has no support for lane remapping > (e.g. phy-qcom-edp doesn't). > > Let me reiterate, please: lane management is outside of the DisplayPort > spec, at least as far as I can understand it. All lane remapping > (especially a dynamic one) is a pure vendor extension to the standard. > I'm trying to find a way to support Corsola and Trogdor without adding > "this is done specially for Google" kind of API. Usually that doesn't > fly in the long term. Got it. > > I understand that using Type-C API for the DRM bridge sounds strange. > But even the mentioned bridge uses Type-C API. It asks for the Type-C > polarity, not the DP polarity. > I understand that lane assignment isn't part of the DisplayPort spec, while it is part of the USB Type-C DisplayPort Altmode spec. I'm not entirely convinced that lane assignment is _only_ part of the altmode spec and should be implemented with a typec switch though, because I imagine some hardware design could be created that has two DisplayPort connectors, just like these two USB-C connectors, and some sort of HPD redriver logic similar to the EC that decides which DP port "wins" and should have DP sent to it. Or perhaps 2 lanes DP to a DP connector and 2 lanes DP sent to a DP to HDMI bridge (shudder). In either case, USB type-c isn't involved. It sounds like we're debating how to handle lane assignment in the kernel. Either way, the code is going to be implemented in the bridge driver because it's the one that has to change what physical lane a logical lane is assigned to. The question is if it should be some sort of bridge_funcs callback, or should bridge drivers hook into the typec framework to expose an orientation switch, or something else? I'm thinking we should introduce some sort of bridge_funcs callback that can be called from the DP altmode driver, either parallel to the drm_connector_oob_hotplug_event() function or from it directly. If we can pass the fwnode for the usb-c-connector to the oob_hotplug_event callback, maybe that's all we need to figure out which lanes go where. And then in the 2 DP connector muxing world we can call drm_connector_oob_hotplug_event() with one or the other DP connector node, which will likely be children nodes of the "HPD redriver" device.
diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml index c991626dc22b..bbe28047d0c0 100644 --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml @@ -98,9 +98,6 @@ properties: gpio-controller: true - typec: - $ref: /schemas/usb/google,cros-ec-typec.yaml# - ec-pwm: $ref: /schemas/pwm/google,cros-ec-pwm.yaml# deprecated: true @@ -166,6 +163,10 @@ patternProperties: type: object $ref: /schemas/extcon/extcon-usbc-cros-ec.yaml# + "^typec(-[0-9])*$": + type: object + $ref: /schemas/usb/google,cros-ec-typec.yaml# + required: - compatible diff --git a/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml index 365523a63179..235b86da3cdd 100644 --- a/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml +++ b/Documentation/devicetree/bindings/usb/google,cros-ec-typec.yaml @@ -26,6 +26,106 @@ properties: '#size-cells': const: 0 + mux-gpios: + description: GPIOs indicating which way the DP mux is steered + maxItems: 1 + + no-hpd: + description: Indicates this endpoint doesn't signal HPD for DisplayPort + type: boolean + + mode-switch: + $ref: usb-switch.yaml#properties/mode-switch + + orientation-switch: + $ref: usb-switch.yaml#properties/orientation-switch + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: Output ports for combined DP and USB SS data + patternProperties: + "^endpoint@([0-8])$": + $ref: usb-switch.yaml#/$defs/usbc-out-endpoint + unevaluatedProperties: false + + anyOf: + - required: + - endpoint@0 + - required: + - endpoint@1 + - required: + - endpoint@2 + - required: + - endpoint@3 + - required: + - endpoint@4 + - required: + - endpoint@5 + - required: + - endpoint@6 + - required: + - endpoint@7 + - required: + - endpoint@8 + + port@1: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port to receive USB SuperSpeed (SS) data + patternProperties: + "^endpoint@([0-8])$": + $ref: usb-switch.yaml#/$defs/usbc-in-endpoint + unevaluatedProperties: false + + anyOf: + - required: + - endpoint@0 + - required: + - endpoint@1 + - required: + - endpoint@2 + - required: + - endpoint@3 + - required: + - endpoint@4 + - required: + - endpoint@5 + - required: + - endpoint@6 + - required: + - endpoint@7 + - required: + - endpoint@8 + + port@2: + $ref: /schemas/graph.yaml#/$defs/port-base + description: + Input port to receive DisplayPort (DP) data + unevaluatedProperties: false + + properties: + endpoint: + $ref: usb-switch.yaml#/$defs/dp-endpoint + unevaluatedProperties: false + + required: + - endpoint + + required: + - port@0 + + anyOf: + - required: + - port@1 + - required: + - port@2 + patternProperties: '^connector@[0-9a-f]+$': $ref: /schemas/connector/usb-connector.yaml# @@ -35,6 +135,40 @@ patternProperties: required: - compatible +allOf: + - if: + required: + - no-hpd + then: + properties: + ports: + required: + - port@2 + - if: + required: + - mux-gpios + then: + properties: + ports: + required: + - port@2 + - if: + required: + - orientation-switch + then: + properties: + ports: + required: + - port@2 + - if: + required: + - mode-switch + then: + properties: + ports: + required: + - port@2 + additionalProperties: false examples: @@ -50,6 +184,8 @@ examples: typec { compatible = "google,cros-ec-typec"; + orientation-switch; + mode-switch; #address-cells = <1>; #size-cells = <0>; @@ -60,6 +196,99 @@ examples: power-role = "dual"; data-role = "dual"; try-power-role = "source"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + usb_c0_hs: endpoint { + remote-endpoint = <&usb_hub_dfp3_hs>; + }; + }; + + port@1 { + reg = <1>; + usb_c0_ss: endpoint { + remote-endpoint = <&cros_typec_c0_ss>; + }; + }; + }; + }; + + connector@1 { + compatible = "usb-c-connector"; + reg = <1>; + power-role = "dual"; + data-role = "dual"; + try-power-role = "source"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + usb_c1_hs: endpoint { + remote-endpoint = <&usb_hub_dfp2_hs>; + }; + }; + + port@1 { + reg = <1>; + usb_c1_ss: endpoint { + remote-endpoint = <&cros_typec_c1_ss>; + }; + }; + }; + }; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + cros_typec_c0_ss: endpoint@0 { + reg = <0>; + remote-endpoint = <&usb_c0_ss>; + data-lanes = <0 1 2 3>; + }; + + cros_typec_c1_ss: endpoint@1 { + reg = <1>; + remote-endpoint = <&usb_c1_ss>; + data-lanes = <2 3 0 1>; + }; + }; + + port@1 { + reg = <1>; + #address-cells = <1>; + #size-cells = <0>; + + usb_in_0: endpoint@0 { + reg = <0>; + remote-endpoint = <&usb_ss_0_out>; + }; + + usb_in_1: endpoint@1 { + reg = <1>; + remote-endpoint = <&usb_ss_1_out>; + }; + }; + + port@2 { + reg = <2>; + dp_in: endpoint { + remote-endpoint = <&dp_phy>; + data-lanes = <0 1>; + }; + }; }; }; };