diff mbox series

[v4,4/5] platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1 Support

Message ID 20230920023243.2494410-5-utkarsh.h.patel@intel.com (mailing list archive)
State Accepted
Commit 70ca6c7312c5ec5bd8a3656c9df8b2c90d04bdc5
Headers show
Series Displayport Alternate Mode 2.1 Support | expand

Commit Message

Patel, Utkarsh H Sept. 20, 2023, 2:32 a.m. UTC 
Displayport Alternatemode 2.1 requires cable capabilities such as cable
signalling, cable type, DPAM version which then will be used by mux
driver for displayport configuration. These capabilities can be derived
from the Cable VDO.

Acked-by: Prashant Malani <pmalani@chromium.org>
Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
---
Changes in v4:
- Removed helper macro changes from this patch as [PATCH 3/5] is added back.
- Added Acked-by tag from Prashant.

Changes in v3:
- Removed use of variable cable_seepd.
- Added helper macro of pd_vdo.h in this patch as cros_ec_typec is the user.

Changes in v2:
- Remvoed feature flag specifice to DP2.1.
- Remvoed seperate function for DP2.1.
- Removed use of EC host coammnd to get cable details.
- TBT cable VDO is used to get cable details.
- Using VDO_CABLE_SPEED macro to get passive cable speed. 

 drivers/platform/chrome/cros_ec_typec.c | 28 +++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Andy Shevchenko Sept. 20, 2023, 2:42 p.m. UTC | #1 
On Tue, Sep 19, 2023 at 07:32:42PM -0700, Utkarsh Patel wrote:
> Displayport Alternatemode 2.1 requires cable capabilities such as cable
> signalling, cable type, DPAM version which then will be used by mux
> driver for displayport configuration. These capabilities can be derived
> from the Cable VDO.

...

> +	/**

Are you sure?

> +	 * Get cable VDO for thunderbolt cables and cables with DPSID but does not
> +	 * support DPAM2.1.
> +	 */

...

> +	if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
> +		dp_data.conf |= cable_dp_vdo;
> +	} else if (cable_tbt_vdo) {
> +		dp_data.conf |=  TBT_CABLE_SPEED(cable_tbt_vdo) << DP_CONF_SIGNALLING_SHIFT;
> +
> +		/* Cable Type */
> +		if (cable_tbt_vdo & TBT_CABLE_OPTICAL)
> +			dp_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL << DP_CONF_CABLE_TYPE_SHIFT;
> +		else if (cable_tbt_vdo & TBT_CABLE_RETIMER)
> +			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER << DP_CONF_CABLE_TYPE_SHIFT;
> +		else if (cable_tbt_vdo & TBT_CABLE_ACTIVE_PASSIVE)
> +			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER << DP_CONF_CABLE_TYPE_SHIFT;
> +	} else if (PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_PCABLE) {
> +		dp_data.conf |= VDO_TYPEC_CABLE_SPEED(port->c_identity.vdo[0]) <<
> +				DP_CONF_SIGNALLING_SHIFT;
> +	}

You can also make it a bit more readable with (use better names
if you think it's needed)

	u32 signalling = 0;
	u32 cable_type = 0;

	...

	if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
		dp_data.conf |= cable_dp_vdo;
	} else if (cable_tbt_vdo) {
		signalling = TBT_CABLE_SPEED(cable_tbt_vdo);

		/* Cable Type */
		if (cable_tbt_vdo & TBT_CABLE_OPTICAL)
			cable_type = DP_CONF_CABLE_TYPE_OPTICAL;
		else if (cable_tbt_vdo & TBT_CABLE_RETIMER)
			cable_type = DP_CONF_CABLE_TYPE_RE_TIMER;
		else if (cable_tbt_vdo & TBT_CABLE_ACTIVE_PASSIVE)
			cable_type = DP_CONF_CABLE_TYPE_RE_DRIVER;
	} else if (PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_PCABLE) {
		signalling = VDO_TYPEC_CABLE_SPEED(port->c_identity.vdo[0]);
	}

	dp_data.conf |= signalling << DP_CONF_SIGNALLING_SHIFT;
	dp_data.conf |= cable_type << DP_CONF_CABLE_TYPE_SHIFT;
Patel, Utkarsh H Sept. 25, 2023, 3:54 p.m. UTC | #2 
Hi Andy,

Thank you for the review. 

> 
> ...
> 
> > +	/**
> 
> Are you sure?
> 
> > +	 * Get cable VDO for thunderbolt cables and cables with DPSID but
> does not
> > +	 * support DPAM2.1.
> > +	 */
> 
> ...

Yes, there are TBT3 cables which advertise DPSID but does not provide any DP capabilities in the DP discover mode VDO but does support UHBR.
In that case, need to use TBTSID and use capabilities from TBT discover mode VDO.

> 
> > +	if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
> > +		dp_data.conf |= cable_dp_vdo;
> > +	} else if (cable_tbt_vdo) {
> > +		dp_data.conf |=  TBT_CABLE_SPEED(cable_tbt_vdo) <<
> > +DP_CONF_SIGNALLING_SHIFT;
> > +
> > +		/* Cable Type */
> > +		if (cable_tbt_vdo & TBT_CABLE_OPTICAL)
> > +			dp_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL <<
> DP_CONF_CABLE_TYPE_SHIFT;
> > +		else if (cable_tbt_vdo & TBT_CABLE_RETIMER)
> > +			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER <<
> DP_CONF_CABLE_TYPE_SHIFT;
> > +		else if (cable_tbt_vdo & TBT_CABLE_ACTIVE_PASSIVE)
> > +			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER
> << DP_CONF_CABLE_TYPE_SHIFT;
> > +	} else if (PD_IDH_PTYPE(port->c_identity.id_header) ==
> IDH_PTYPE_PCABLE) {
> > +		dp_data.conf |= VDO_TYPEC_CABLE_SPEED(port-
> >c_identity.vdo[0]) <<
> > +				DP_CONF_SIGNALLING_SHIFT;
> > +	}
> 
> You can also make it a bit more readable with (use better names if you think it's
> needed)
> 
> 	u32 signalling = 0;
> 	u32 cable_type = 0;

In v2 version of this patch I had them but there was feedback to remove extra variables and use them inline. 


Sincerely,
Utkarsh Patel.
Andy Shevchenko Sept. 25, 2023, 7:27 p.m. UTC | #3 
On Mon, Sep 25, 2023 at 03:54:40PM +0000, Patel, Utkarsh H wrote:

...

> > > +	/**
> > 
> > Are you sure?
> > 
> > > +	 * Get cable VDO for thunderbolt cables and cables with DPSID but
> > does not
> > > +	 * support DPAM2.1.
> > > +	 */
> > 
> Yes, there are TBT3 cables which advertise DPSID but does not provide any DP
> capabilities in the DP discover mode VDO but does support UHBR.  In that
> case, need to use TBTSID and use capabilities from TBT discover mode VDO.

My comment was against the style of the comment, not about content.

...

> > You can also make it a bit more readable with (use better names if you think it's
> > needed)
> > 
> > 	u32 signalling = 0;
> > 	u32 cable_type = 0;
> 
> In v2 version of this patch I had them but there was feedback to remove extra
> variables and use them inline. 

OK!
Patel, Utkarsh H Sept. 28, 2023, 1:31 a.m. UTC | #4 
> 
> On Mon, Sep 25, 2023 at 03:54:40PM +0000, Patel, Utkarsh H wrote:
> 
> ...
> 
> > > > +	/**
> > >
> > > Are you sure?
> > >
> > > > +	 * Get cable VDO for thunderbolt cables and cables with DPSID
> > > > +but
> > > does not
> > > > +	 * support DPAM2.1.
> > > > +	 */
> > >
> > Yes, there are TBT3 cables which advertise DPSID but does not provide
> > any DP capabilities in the DP discover mode VDO but does support UHBR.
> > In that case, need to use TBTSID and use capabilities from TBT discover mode
> VDO.
> 
> My comment was against the style of the comment, not about content.
> 

Ahh, Okay.  Thank you for clarifying.

Sincerely,
Utkarsh Patel.
diff mbox series

Patch

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index d0b4d3fc40ed..cca913400b39 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -492,6 +492,8 @@  static int cros_typec_enable_dp(struct cros_typec_data *typec,
 {
 	struct cros_typec_port *port = typec->ports[port_num];
 	struct typec_displayport_data dp_data;
+	u32 cable_tbt_vdo;
+	u32 cable_dp_vdo;
 	int ret;
 
 	if (typec->pd_ctrl_ver < 2) {
@@ -524,6 +526,32 @@  static int cros_typec_enable_dp(struct cros_typec_data *typec,
 	port->state.data = &dp_data;
 	port->state.mode = TYPEC_MODAL_STATE(ffs(pd_ctrl->dp_mode));
 
+	/* Get cable VDO for cables with DPSID to check DPAM2.1 is supported */
+	cable_dp_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_DP_SID);
+
+	/**
+	 * Get cable VDO for thunderbolt cables and cables with DPSID but does not
+	 * support DPAM2.1.
+	 */
+	cable_tbt_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
+
+	if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
+		dp_data.conf |= cable_dp_vdo;
+	} else if (cable_tbt_vdo) {
+		dp_data.conf |=  TBT_CABLE_SPEED(cable_tbt_vdo) << DP_CONF_SIGNALLING_SHIFT;
+
+		/* Cable Type */
+		if (cable_tbt_vdo & TBT_CABLE_OPTICAL)
+			dp_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL << DP_CONF_CABLE_TYPE_SHIFT;
+		else if (cable_tbt_vdo & TBT_CABLE_RETIMER)
+			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER << DP_CONF_CABLE_TYPE_SHIFT;
+		else if (cable_tbt_vdo & TBT_CABLE_ACTIVE_PASSIVE)
+			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER << DP_CONF_CABLE_TYPE_SHIFT;
+	} else if (PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_PCABLE) {
+		dp_data.conf |= VDO_TYPEC_CABLE_SPEED(port->c_identity.vdo[0]) <<
+				DP_CONF_SIGNALLING_SHIFT;
+	}
+
 	ret = cros_typec_retimer_set(port->retimer, port->state);
 	if (!ret)
 		ret = typec_mux_set(port->mux, &port->state);