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

[Dmitry Baryshkov]

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 at 1/endpoint at 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 at 1/endpoint at 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 at 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 at 1 {
> >                 port at 1 {
> >                         endpoint at 0 {
> >                                 remtoe = <&usb_hub_1>;
> >                         };
> >
> >                         endpoint at 1 {
> >                                 remote = <&dp_bridge_out_1>;
> >                         };
> >                 };
> >         };
> > };
> >
> > dp_bridge {
> >         ports {
> >                 port at 1 {
> >                         dp_bridge_out_0: endpoint at 0 {
> >                                 remote = <usb_c0_ss_dp>;
> >                                 data-lanes = <0 1>;
> >                         };
> >
> >                         dp_bridge_out_1: endpoint at 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.

-- 
With best wishes
Dmitry