Message-ID: <hqmx7jtkvrwvb27n56hw7rpefhp37lhr3a5fawz7gsl76uuj5s@h7m6wpdhibkk>
Date: Sat, 9 Nov 2024 09:05:18 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Stephen Boyd <swboyd@...omium.org>
Cc: chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	patches@...ts.linux.dev, devicetree@...r.kernel.org, 
	Douglas Anderson <dianders@...omium.org>, Pin-yen Lin <treapking@...omium.org>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Benson Leung <bleung@...omium.org>, 
	Conor Dooley <conor+dt@...nel.org>, Daniel Vetter <daniel@...ll.ch>, 
	David Airlie <airlied@...il.com>, dri-devel@...ts.freedesktop.org, 
	Guenter Roeck <groeck@...omium.org>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	Jonas Karlman <jonas@...boo.se>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Lee Jones <lee@...nel.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Prashant Malani <pmalani@...omium.org>, 
	Robert Foss <rfoss@...nel.org>, Rob Herring <robh+dt@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Alexandre Belloni <alexandre.belloni@...tlin.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Daniel Scally <djrscally@...il.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>, Ivan Orlov <ivan.orlov0322@...il.com>, 
	linux-acpi@...r.kernel.org, linux-usb@...r.kernel.org, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Vinod Koul <vkoul@...nel.org>, Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v4 15/18] dt-bindings: usb: Add ports to
 google,cros-ec-typec for DP altmode

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.


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists