RE: [PATCH 3/4] platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1 Support

[Patel, Utkarsh H]

Hi Prashant,

Thank you for the review and feedback. 

> -----Original Message-----
> From: Prashant Malani <pmalani@xxxxxxxxxxxx>
> Sent: Friday, August 18, 2023 10:16 AM
> To: Patel, Utkarsh H <utkarsh.h.patel@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> heikki.krogerus@xxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx;
> bleung@xxxxxxxxxxxx
> Subject: Re: [PATCH 3/4] platform/chrome: cros_ec_typec: Add Displayport
> Alternatemode 2.1 Support
> 
> Hi Utkarsh,
> 
> Thanks for the patch. Please include the chrome-platform mailing list to each
> patch in the series; at the very least, patches which touch
> drivers/platform/chrome should definitely have the mailing list (chrome-
> platform@xxxxxxxxxxxxxxx). Otherwise, we don't have enough context about
> what changes are being made across the series.

Ack. I will add this mailing list.

> >
> > +static int cros_typec_dp21_support(struct cros_typec_port *port,
> > +				   struct typec_displayport_data dp21_data,
> > +				   struct ec_response_usb_pd_control_v2
> *pd_ctrl) {
> > +	u32 cable_vdo = cros_typec_get_cable_vdo(port,
> USB_TYPEC_DP_SID);
> > +
> > +	if (cable_vdo & DP_CAP_DPAM_VERSION) {
> > +		dp21_data.conf |= cable_vdo;
> > +	} else {
> > +		/* Cable Speed */
> > +		dp21_data.conf |= pd_ctrl->cable_speed <<
> DP_CONF_SIGNALLING_SHIFT;
> > +
> > +		/* Cable Type */
> > +		if (pd_ctrl->cable_gen & USB_PD_CTRL_OPTICAL_CABLE)
> > +			dp21_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL
> << DP_CONF_CABLE_TYPE_SHIFT;
> > +		else if (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID)
> & TBT_CABLE_RETIMER)
> > +			dp21_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER
> << DP_CONF_CABLE_TYPE_SHIFT;
> > +		else if (pd_ctrl->cable_gen & USB_PD_CTRL_ACTIVE_CABLE)
> > +			dp21_data.conf |=
> DP_CONF_CABLE_TYPE_RE_DRIVER << DP_CONF_CABLE_TYPE_SHIFT;
> > +	}
> 
> I don't understand why the conf VDO is being recreated here. cable_vdo
> should already contain the necessary bits. Just use the cable_vdo that you get
> from cros_typec_get_cable_vdo(); it will have all the bits set correctly already
> (the EC should be doing that).
> 
> The "if" condition should also be unnecessary.
> 
> You are already doing something similar in the other patch for "active retimer
> cable support" [1]

"if" condition is necessary because there are cables (LRD Gen3) with DPSID but does not support DPAM 2.1 and in that case we need to check TBTSID of the cable and use the cable properties e.g. SPEED and Type. Since that information is already available in pd_ctrl, we are leveraging it.  I will remove the part where it's checking retimer cable as DP2.1 is anyway not supported.

> 
> > +
> > +	port->state.data = &dp21_data;
> > +
> > +	return typec_mux_set(port->mux, &port->state);
> 
> Note that now you have reversed the order in which the muxes are set (which
> leads to subtle timing issues with Burnside Bridge and other similar retimers).
> So please don't do this.

Are you suggesting to add same order here?
ret = cros_typec_retimer_set(port->retimer, port->state);
	if (!ret)
		ret = typec_mux_set(port->mux, &port->state);

> 
> > +}
> > +
> >  /* Spoof the VDOs that were likely communicated by the partner. */
> > static int cros_typec_enable_dp(struct cros_typec_data *typec,
> >  				int port_num,
> > @@ -524,6 +550,9 @@ 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));
> >
> > +	if (typec->typec_dp21_supported)
> > +		return cros_typec_dp21_support(port, dp_data, pd_ctrl);
> > +
> >  	ret = cros_typec_retimer_set(port->retimer, port->state);
> >  	if (!ret)
> >  		ret = typec_mux_set(port->mux, &port->state); @@ -1196,6
> +1225,7 @@
> > static int cros_typec_probe(struct platform_device *pdev)
> >
> >  	typec->typec_cmd_supported = cros_ec_check_features(ec_dev,
> EC_FEATURE_TYPEC_CMD);
> >  	typec->needs_mux_ack = cros_ec_check_features(ec_dev,
> > EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);
> > +	typec->typec_dp21_supported = cros_ec_check_features(ec_dev,
> > +EC_FEATURE_TYPEC_DP2_1);
> 
> This entire feature isn't necessary. Regardless of whether dp2.1 is supported
> or not, the port driver just needs to forward the cable_vdo it receives faithfully
> to the mux driver, which can deal with internal details (based on whether *it*
> supports DP 2.1 or not).

I am Okay with that.
We thought we can avoid unnecessary check for the cable_vdo for DP alternate mode on platform where DP2.1 is not supported. 

Sincerely,
Utkarsh Patel.