[PATCH 0/4] Displayport Alternate Mode 2.1 Support

[PATCH 0/4] Displayport Alternate Mode 2.1 Support

[Utkarsh Patel]

This series enabels cable identification flow required for Displayport
Alternate Mode 2.1 support.

In this series [PATCH 3/4] uses cros_typec_get_cable_vdo API from,
https://patchwork.kernel.org/project/linux-usb/list/?series=766752

Utkarsh Patel (4):
  usb: typec: Add Displayport Alternate Mode 2.1 Support
  platform/chrome: cros_ec: Add Displayport Alternatemode 2.1 feature
    flag
  platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1
    Support
  usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1

 drivers/platform/chrome/cros_ec_typec.c       | 30 +++++++++++++++++++
 drivers/platform/chrome/cros_ec_typec.h       |  1 +
 drivers/usb/typec/altmodes/displayport.c      |  5 +++-
 drivers/usb/typec/mux/intel_pmc_mux.c         | 24 +++++++++++++++
 drivers/usb/typec/ucsi/displayport.c          |  2 +-
 drivers/usb/typec/ucsi/ucsi_ccg.c             |  4 +--
 .../linux/platform_data/cros_ec_commands.h    |  5 ++++
 include/linux/usb/typec_dp.h                  | 28 ++++++++++++++---
 8 files changed, 91 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/4] usb: typec: Add Displayport Alternate Mode 2.1 Support

[Utkarsh Patel]

Displayport Alternate mode 2.1 requires configuration for additional
cable details such as signalling for cable, UHBR13.5 Support, Cable type
and DPAM version.
These details can be used with mux drivers to configure SOP DP
configuration for Displayport Alternate mode 2.1.
This change also includes pertinent cable signalling support in displayport
alternate mode.

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/usb/typec/altmodes/displayport.c |  5 ++++-
 drivers/usb/typec/ucsi/displayport.c     |  2 +-
 drivers/usb/typec/ucsi/ucsi_ccg.c        |  4 ++--
 include/linux/usb/typec_dp.h             | 28 ++++++++++++++++++++----
 4 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 66de880b28d0..97edb9005529 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -85,8 +85,11 @@ static int dp_altmode_notify(struct dp_altmode *dp)
 
 static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
 {
-	u32 conf = DP_CONF_SIGNALING_DP; /* Only DP signaling supported */
 	u8 pin_assign = 0;
+	u32 conf;
+
+	/* DP Signalling */
+	conf = (dp->data.conf & DP_CONF_SIGNALLING_MASK) >> DP_CONF_SIGNALLING_SHIFT;
 
 	switch (con) {
 	case DP_STATUS_CON_DISABLED:
diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 73cd5bf35047..d9d3c91125ca 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -315,7 +315,7 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
 	struct ucsi_dp *dp;
 
 	/* We can't rely on the firmware with the capabilities. */
-	desc->vdo |= DP_CAP_DP_SIGNALING | DP_CAP_RECEPTACLE;
+	desc->vdo |= DP_CAP_DP_SIGNALLING(0) | DP_CAP_RECEPTACLE;
 
 	/* Claiming that we support all pin assignments */
 	desc->vdo |= all_assignments << 8;
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 607061a37eca..449c125f6f87 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -501,8 +501,8 @@ static void ucsi_ccg_nvidia_altmode(struct ucsi_ccg *uc,
 	case NVIDIA_FTB_DP_OFFSET:
 		if (alt[0].mid == USB_TYPEC_NVIDIA_VLINK_DBG_VDO)
 			alt[0].mid = USB_TYPEC_NVIDIA_VLINK_DP_VDO |
-				DP_CAP_DP_SIGNALING | DP_CAP_USB |
-				DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_E));
+				     DP_CAP_DP_SIGNALLING(0) | DP_CAP_USB |
+				     DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_E));
 		break;
 	case NVIDIA_FTB_DBG_OFFSET:
 		if (alt[0].mid == USB_TYPEC_NVIDIA_VLINK_DP_VDO)
diff --git a/include/linux/usb/typec_dp.h b/include/linux/usb/typec_dp.h
index 8d09c2f0a9b8..1f358098522d 100644
--- a/include/linux/usb/typec_dp.h
+++ b/include/linux/usb/typec_dp.h
@@ -67,8 +67,10 @@ enum {
 #define   DP_CAP_UFP_D			1
 #define   DP_CAP_DFP_D			2
 #define   DP_CAP_DFP_D_AND_UFP_D	3
-#define DP_CAP_DP_SIGNALING		BIT(2) /* Always set */
-#define DP_CAP_GEN2			BIT(3) /* Reserved after v1.0b */
+#define DP_CAP_DP_SIGNALLING(_cap_)	(((_cap_) & GENMASK(5, 2)) >> 2)
+#define   DP_CAP_SIGNALLING_HBR3	1
+#define   DP_CAP_SIGNALLING_UHBR10	2
+#define   DP_CAP_SIGNALLING_UHBR20	3
 #define DP_CAP_RECEPTACLE		BIT(6)
 #define DP_CAP_USB			BIT(7)
 #define DP_CAP_DFP_D_PIN_ASSIGN(_cap_)	(((_cap_) & GENMASK(15, 8)) >> 8)
@@ -78,6 +80,13 @@ enum {
 			DP_CAP_UFP_D_PIN_ASSIGN(_cap_) : DP_CAP_DFP_D_PIN_ASSIGN(_cap_))
 #define DP_CAP_PIN_ASSIGN_DFP_D(_cap_) ((_cap_ & DP_CAP_RECEPTACLE) ? \
 			DP_CAP_DFP_D_PIN_ASSIGN(_cap_) : DP_CAP_UFP_D_PIN_ASSIGN(_cap_))
+#define DP_CAP_UHBR_13_5_SUPPORT	BIT(26)
+#define DP_CAP_CABLE_TYPE(_cap_)	(((_cap_) & GENMASK(29, 28)) >> 28)
+#define   DP_CAP_CABLE_TYPE_PASSIVE	0
+#define   DP_CAP_CABLE_TYPE_RE_TIMER	1
+#define   DP_CAP_CABLE_TYPE_RE_DRIVER	2
+#define   DP_CAP_CABLE_TYPE_OPTICAL	3
+#define DP_CAP_DPAM_VERSION		BIT(30)
 
 /* DisplayPort Status Update VDO bits */
 #define DP_STATUS_CONNECTION(_status_)	((_status_) & 3)
@@ -97,13 +106,24 @@ enum {
 #define DP_CONF_CURRENTLY(_conf_)	((_conf_) & 3)
 #define DP_CONF_UFP_U_AS_DFP_D		BIT(0)
 #define DP_CONF_UFP_U_AS_UFP_D		BIT(1)
-#define DP_CONF_SIGNALING_DP		BIT(2)
-#define DP_CONF_SIGNALING_GEN_2		BIT(3) /* Reserved after v1.0b */
+#define DP_CONF_SIGNALLING_MASK		GENMASK(5, 2)
+#define DP_CONF_SIGNALLING_SHIFT	2
+#define   DP_CONF_SIGNALLING_HBR3	1
+#define   DP_CONF_SIGNALLING_UHBR10	2
+#define   DP_CONF_SIGNALLING_UHBR20	3
 #define DP_CONF_PIN_ASSIGNEMENT_SHIFT	8
 #define DP_CONF_PIN_ASSIGNEMENT_MASK	GENMASK(15, 8)
 
 /* Helper for setting/getting the pin assignment value to the configuration */
 #define DP_CONF_SET_PIN_ASSIGN(_a_)	((_a_) << 8)
 #define DP_CONF_GET_PIN_ASSIGN(_conf_)	(((_conf_) & GENMASK(15, 8)) >> 8)
+#define DP_CONF_UHBR13_5_SUPPORT	BIT(26)
+#define DP_CONF_CABLE_TYPE_MASK		GENMASK(29, 28)
+#define DP_CONF_CABLE_TYPE_SHIFT	28
+#define   DP_CONF_CABLE_TYPE_PASSIVE	0
+#define   DP_CONF_CABLE_TYPE_RE_TIMER	1
+#define   DP_CONF_CABLE_TYPE_RE_DRIVER	2
+#define   DP_CONF_CABLE_TYPE_OPTICAL	3
+#define DP_CONF_DPAM_VERSION		BIT(30)
 
 #endif /* __USB_TYPEC_DP_H */
-- 
2.25.1

[PATCH 2/4] platform/chrome: cros_ec: Add Displayport Alternatemode 2.1 feature flag

[Utkarsh Patel]

Update EC command header to include feature flag for Displayport
Alternatemode 2.1 support.

This change also brings in corresponding EC header updates from the EC code
base [1].
Link: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h [1]

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>
---
 include/linux/platform_data/cros_ec_commands.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ab721cf13a98..f2b095107555 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -1312,6 +1312,11 @@ enum ec_feature_code {
 	 * The EC supports the AP composing VDMs for us to send.
 	 */
 	EC_FEATURE_TYPEC_AP_VDM_SEND = 46,
+	/*
+	 * The EC supports DP2.1 capability.
+	 */
+	EC_FEATURE_TYPEC_DP2_1 = 50,
+
 };
 
 #define EC_FEATURE_MASK_0(event_code) BIT(event_code % 32)
-- 
2.25.1

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

[Utkarsh Patel]

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;
+	}
+
+	port->state.data = &dp21_data;
+
+	return 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);
 
 	ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
 			  &resp, sizeof(resp));
diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
index deda180a646f..a588b2780823 100644
--- a/drivers/platform/chrome/cros_ec_typec.h
+++ b/drivers/platform/chrome/cros_ec_typec.h
@@ -37,6 +37,7 @@ struct cros_typec_data {
 	struct cros_typec_port *ports[EC_USB_PD_MAX_PORTS];
 	struct notifier_block nb;
 	struct work_struct port_work;
+	bool typec_dp21_supported;
 	bool typec_cmd_supported;
 	bool needs_mux_ack;
 };
-- 
2.25.1

[PATCH 4/4] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1

[Utkarsh Patel]

Mux agent driver can configure cable details such as cable type and
cable speed received as a part of displayport configuration to support
Displayport Alternate mode 2.1.

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/usb/typec/mux/intel_pmc_mux.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index 888632847a74..218f6e25518d 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -180,6 +180,12 @@ static int hsl_orientation(struct pmc_usb_port *port)
 	return port->orientation - 1;
 }
 
+static bool is_pmc_mux_tbt(struct acpi_device *adev)
+{
+	return acpi_dev_hid_uid_match(adev, "INTC1072", NULL) ||
+	       acpi_dev_hid_uid_match(adev, "INTC1079", NULL);
+}
+
 static int pmc_usb_send_command(struct intel_scu_ipc_dev *ipc, u8 *msg, u32 len)
 {
 	u8 response[4];
@@ -282,6 +288,24 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
 	req.mode_data |= (state->mode - TYPEC_STATE_MODAL) <<
 			 PMC_USB_ALTMODE_DP_MODE_SHIFT;
 
+	if (!is_pmc_mux_tbt(port->pmc->iom_adev)) {
+		u8 cable_speed = (data->conf & DP_CONF_SIGNALLING_MASK) >>
+				  DP_CONF_SIGNALLING_SHIFT;
+
+		u8 cable_type = (data->conf & DP_CONF_CABLE_TYPE_MASK) >>
+				 DP_CONF_CABLE_TYPE_SHIFT;
+
+		req.mode_data |= PMC_USB_ALTMODE_CABLE_SPD(cable_speed);
+
+		if (cable_type == DP_CONF_CABLE_TYPE_OPTICAL)
+			req.mode_data |= PMC_USB_ALTMODE_CABLE_TYPE;
+		else if (cable_type == DP_CONF_CABLE_TYPE_RE_TIMER)
+			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE |
+					 PMC_USB_ALTMODE_RETIMER_CABLE;
+		else if (cable_type == DP_CONF_CABLE_TYPE_RE_DRIVER)
+			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+	}
+
 	ret = pmc_usb_command(port, (void *)&req, sizeof(req));
 	if (ret)
 		return ret;
-- 
2.25.1

Re: [PATCH 4/4] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1

[Sergey Shtylyov]

Hello!

On 8/12/23 12:07 AM, Utkarsh Patel wrote:

> Mux agent driver can configure cable details such as cable type and
> cable speed received as a part of displayport configuration to support
> Displayport Alternate mode 2.1.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

   Hm, I think the R-b tags should follow your signoff...

> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> ---
>  drivers/usb/typec/mux/intel_pmc_mux.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index 888632847a74..218f6e25518d 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
[...]
> @@ -282,6 +288,24 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
>  	req.mode_data |= (state->mode - TYPEC_STATE_MODAL) <<
>  			 PMC_USB_ALTMODE_DP_MODE_SHIFT;
>  
> +	if (!is_pmc_mux_tbt(port->pmc->iom_adev)) {
> +		u8 cable_speed = (data->conf & DP_CONF_SIGNALLING_MASK) >>
> +				  DP_CONF_SIGNALLING_SHIFT;
> +
> +		u8 cable_type = (data->conf & DP_CONF_CABLE_TYPE_MASK) >>
> +				 DP_CONF_CABLE_TYPE_SHIFT;
> +
> +		req.mode_data |= PMC_USB_ALTMODE_CABLE_SPD(cable_speed);
> +
> +		if (cable_type == DP_CONF_CABLE_TYPE_OPTICAL)
> +			req.mode_data |= PMC_USB_ALTMODE_CABLE_TYPE;
> +		else if (cable_type == DP_CONF_CABLE_TYPE_RE_TIMER)
> +			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE |
> +					 PMC_USB_ALTMODE_RETIMER_CABLE;
> +		else if (cable_type == DP_CONF_CABLE_TYPE_RE_DRIVER)
> +			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;

   The chain of the *if* statements above is asking to use *switch* instead...

[...]

MBR, Sergey

Re: [PATCH 4/4] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1

[Andy Shevchenko]

On Sat, Aug 12, 2023 at 12:47:43PM +0300, Sergey Shtylyov wrote:
> On 8/12/23 12:07 AM, Utkarsh Patel wrote:
> 
> > Mux agent driver can configure cable details such as cable type and
> > cable speed received as a part of displayport configuration to support
> > Displayport Alternate mode 2.1.
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
>    Hm, I think the R-b tags should follow your signoff...

They following historical order.
So, before this patch appears upstream, it had collected Rb tags.

> > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1

[Andy Shevchenko]

On Mon, Aug 14, 2023 at 05:48:52PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 12, 2023 at 12:47:43PM +0300, Sergey Shtylyov wrote:
> > On 8/12/23 12:07 AM, Utkarsh Patel wrote:
> > 
> > > Mux agent driver can configure cable details such as cable type and
> > > cable speed received as a part of displayport configuration to support
> > > Displayport Alternate mode 2.1.
> > > 
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> >    Hm, I think the R-b tags should follow your signoff...
> 
> They following historical order.
> So, before this patch appears upstream, it had collected Rb tags.

Note, Submitting Patches only defines the order rules to SoB tag, for Rb there
is no such rule:

 "Both Tested-by and Reviewed-by tags, once received on mailing list from
 tester or reviewer, should be added by author to the applicable patches when
 sending next versions."

> > > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Prashant Malani]

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

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@chromium.org>
> Sent: Friday, August 18, 2023 10:16 AM
> To: Patel, Utkarsh H <utkarsh.h.patel@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> heikki.krogerus@linux.intel.com; andriy.shevchenko@linux.intel.com;
> bleung@chromium.org
> 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@lists.linux.dev). 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.

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

[Prashant Malani]

Hi Utkarsh,

On Mon, Aug 21, 2023 at 10:34 AM Patel, Utkarsh H
<utkarsh.h.patel@intel.com> wrote:
>
> Hi Prashant,
>
> Thank you for the review and feedback.
>
> > -----Original Message-----
> > From: Prashant Malani <pmalani@chromium.org>
>>
> > 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.

Ok, then grab the two VDOs distinctly (cable_dp_vdo and cable_tbt_vdo).
Also, the explanation you gave above seems like a good candidate for a comment
above the "if" block.

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

As mentioned in earlier patches related to this, we want to avoid
using EC-specific data structures
as much as possible moving forward, especially if the information can
be gleaned from the
available DiscSVID VDOs. So, please use the required cable VDO
(whether it is DP or TBT SID).

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

Specifically, breaking out a separate function for DP 2.1 and then
embedding that
in the overall enable_dp() function makes things unnecessarily complex.

Just craft the VDOs onew time, within enable_dp(), and then use the existing
locations where cros_retimer_set and typec_mux_set() are called.

This will become more clear once you rebase this commit on top of the changes
in [1]

Effectively cros_typec_enable_dp() should handle all DP cases, without needing
a special function broken out for DP 2.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.

The optimization is miniscule enough to not be worth it the added code
verbosity.

Re: [PATCH 0/4] Displayport Alternate Mode 2.1 Support

[Greg KH]

On Fri, Aug 11, 2023 at 02:07:31PM -0700, Utkarsh Patel wrote:
> This series enabels cable identification flow required for Displayport
> Alternate Mode 2.1 support.
> 
> In this series [PATCH 3/4] uses cros_typec_get_cable_vdo API from,
> https://patchwork.kernel.org/project/linux-usb/list/?series=766752
> 
> Utkarsh Patel (4):
>   usb: typec: Add Displayport Alternate Mode 2.1 Support
>   platform/chrome: cros_ec: Add Displayport Alternatemode 2.1 feature
>     flag
>   platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1
>     Support
>   usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1
> 
>  drivers/platform/chrome/cros_ec_typec.c       | 30 +++++++++++++++++++
>  drivers/platform/chrome/cros_ec_typec.h       |  1 +
>  drivers/usb/typec/altmodes/displayport.c      |  5 +++-
>  drivers/usb/typec/mux/intel_pmc_mux.c         | 24 +++++++++++++++
>  drivers/usb/typec/ucsi/displayport.c          |  2 +-
>  drivers/usb/typec/ucsi/ucsi_ccg.c             |  4 +--
>  .../linux/platform_data/cros_ec_commands.h    |  5 ++++
>  include/linux/usb/typec_dp.h                  | 28 ++++++++++++++---
>  8 files changed, 91 insertions(+), 8 deletions(-)
> 
> -- 
> 2.25.1
> 

What tree are you expecting to see this patch series go through?  Please
be explicit when you resend the new version so we know who is
responsible to take it (or not.)

thanks,

greg k-h

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

[Patel, Utkarsh H]

Hi Prashant,

> > > 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.
> 
> Ok, then grab the two VDOs distinctly (cable_dp_vdo and cable_tbt_vdo).
> Also, the explanation you gave above seems like a good candidate for a
> comment above the "if" block.
> 
> > 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.
> 
> As mentioned in earlier patches related to this, we want to avoid using EC-
> specific data structures as much as possible moving forward, especially if the
> information can be gleaned from the available DiscSVID VDOs. So, please use
> the required cable VDO (whether it is DP or TBT SID).

Ack.

> 
> >
> > >
> > > > +
> > > > +   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?
> 
> Specifically, breaking out a separate function for DP 2.1 and then embedding
> that in the overall enable_dp() function makes things unnecessarily complex.
> 
> Just craft the VDOs onew time, within enable_dp(), and then use the existing
> locations where cros_retimer_set and typec_mux_set() are called.
> 
> This will become more clear once you rebase this commit on top of the
> changes in [1]
> 
> Effectively cros_typec_enable_dp() should handle all DP cases, without
> needing a special function broken out for DP 2.1

Ack.

> 
> > > 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.
> The optimization is miniscule enough to not be worth it the added code
> verbosity.

Ack. I will remove it.

Sincerely,
Utkarsh Patel.