diff mbox series

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

Message ID 20240901040658.157425-16-swboyd@chromium.org
State New
Headers show
Series platform/chrome: Add DT USB/DP muxing/topology support | expand

Commit Message

Stephen Boyd Sept. 1, 2024, 4:06 a.m. UTC
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 +-
 .../bindings/usb/google,cros-ec-typec.yaml    | 229 ++++++++++++++++++
 2 files changed, 233 insertions(+), 3 deletions(-)

Comments

Lee Jones Sept. 3, 2024, 3:35 p.m. UTC | #1
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(-)
Dmitry Baryshkov Sept. 20, 2024, 9:38 a.m. UTC | #2
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
>
Dmitry Baryshkov Oct. 25, 2024, 10:49 a.m. UTC | #3
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.
Stephen Boyd Oct. 29, 2024, 8:15 p.m. UTC | #4
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.
Stephen Boyd Oct. 31, 2024, 9:45 p.m. UTC | #5
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.
diff mbox series

Patch

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>;
+              };
+            };
           };
         };
       };