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

[Prashant Malani <pmalani@chromium.org>]

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@lists.linux.dev). Otherwise,
we don't have enough context about what changes are being made across the series.

On Aug 11 14:07, 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 data as well as from
> the existing EC PD host command interface.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 30 +++++++++++++++++++++++++
>  drivers/platform/chrome/cros_ec_typec.h |  1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index d0b4d3fc40ed..eb4a1cb584a2 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -485,6 +485,32 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
>  	return typec_mux_set(port->mux, &port->state);
>  }
>  
> +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]

> +
> +	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.

> +}
> +
>  /* 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).

Thanks,

-Prashant

[1] https://lore.kernel.org/linux-usb/20230718024703.1013367-1-utkarsh.h.patel@intel.com/T/#m950b24e7874d34f11081f252ba3ef4e752628529