[PATCH v4 0/5] Displayport Alternate Mode 2.1 Support

[PATCH v4 0/5] Displayport Alternate Mode 2.1 Support

[Utkarsh Patel]

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

Please add this series to usb-next. 

Changes in v4:
- Added [PATCH 3/5] back in this series with Reviewed-by tag from Heikki.

Changes in v3:
- Removed [PATCH 3/5] usb: pd: Add helper macro to get Type C cable speed
  instead added the helper function in [PATCH 3/4].
- In [PATCH 3/4] Removed use of variable cable_speed. 

Changes in v2:
- Added two new patches [PATCH 2/5] and [PATCH 3/5] in to this series.
- Removed [PATCH 2/4]  platform/chrome: cros_ec: Add Displayport
  Alternatemode 2.1 feature flag
- In [PATCH 4/5] Removed feature flag and separate function for DP2.1.
- In [PATCH 4/5] used cable vdos instead of host commands to get cable
  details.

Utkarsh Patel (5):
  usb: typec: Add Displayport Alternate Mode 2.1 Support
  usb: typec: Add Active or Passive cable defination to cable discover
    mode VDO
  usb: pd: Add helper macro to get Type C cable speed
  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  | 28 ++++++++++++++++++++++++
 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 ++--
 include/linux/usb/pd_vdo.h               |  1 +
 include/linux/usb/typec_dp.h             | 28 ++++++++++++++++++++----
 include/linux/usb/typec_tbt.h            |  1 +
 8 files changed, 85 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v4 1/5] 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>
---
Changes in v4:
- No change

Changes in v3:
- No change.

Changes in v2:
- No change.

 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 426c88a516e5..f503cb4cd721 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -86,8 +86,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 v4 2/5] usb: typec: Add Active or Passive cable defination to cable discover mode VDO

[Utkarsh Patel]

As per USB Type-C Connector specification v2.2 section F.2.6, BIT25
represents Active or Passive cable. Added BIT25 defination to the
Thunderbolt 3 cable discover mode VDO.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
---
Changes in v4:
- No change.

Changes in v3:
- Reviewed-by tag from Heikki is added.

Changes in v2:
- This change is being added as new patch in this series.

 include/linux/usb/typec_tbt.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
index 63dd44b72e0c..c7a2153bd6f5 100644
--- a/include/linux/usb/typec_tbt.h
+++ b/include/linux/usb/typec_tbt.h
@@ -46,6 +46,7 @@ struct typec_thunderbolt_data {
 #define TBT_CABLE_OPTICAL		BIT(21)
 #define TBT_CABLE_RETIMER		BIT(22)
 #define TBT_CABLE_LINK_TRAINING		BIT(23)
+#define TBT_CABLE_ACTIVE_PASSIVE	BIT(25)
 
 #define TBT_SET_CABLE_SPEED(_s_)	(((_s_) & GENMASK(2, 0)) << 16)
 #define TBT_SET_CABLE_ROUNDED(_g_)	(((_g_) & GENMASK(1, 0)) << 19)
-- 
2.25.1

[PATCH v4 3/5] usb: pd: Add helper macro to get Type C cable speed

[Utkarsh Patel]

Added a helper macro to get the Type C cable speed when provided the
cable VDO.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
---
Changes in v4:
- Added this patch back in this series with Reviewed-by tag from Heikki.

Changes in v2:
- This change is being added as new patch in this series.

 include/linux/usb/pd_vdo.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h
index b057250704e8..3a747938cdab 100644
--- a/include/linux/usb/pd_vdo.h
+++ b/include/linux/usb/pd_vdo.h
@@ -376,6 +376,7 @@
 	 | ((vbm) & 0x3) << 9 | (sbu) << 8 | (sbut) << 7 | ((cur) & 0x3) << 5	\
 	 | (vbt) << 4 | (sopp) << 3 | ((spd) & 0x7))
 
+#define VDO_TYPEC_CABLE_SPEED(vdo)	((vdo) & 0x7)
 #define VDO_TYPEC_CABLE_TYPE(vdo)	(((vdo) >> 18) & 0x3)
 
 /*
-- 
2.25.1

[PATCH v4 4/5] 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.

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(+)

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);
-- 
2.25.1

[PATCH v4 5/5] 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>
---
Changes in v4:
- No change.

Changes in v3:
- No change.

Changes in v2:
- No change.

 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 60ed1f809130..233958084b43 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -191,6 +191,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];
@@ -293,6 +299,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 v4 5/5] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1

[Sergei Shtylyov]

On 9/20/23 5:32 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>
> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> ---
> Changes in v4:
> - No change.
> 
> Changes in v3:
> - No change.
> 
> Changes in v2:
> - No change.
> 
>  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 60ed1f809130..233958084b43 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
[...]
> @@ -293,6 +299,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;

   Why not *switch* instead of string of *if*s?

> +	}
> +
>  	ret = pmc_usb_command(port, (void *)&req, sizeof(req));
>  	if (ret)
>  		return ret;

MBR, Sergey

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

[Andy Shevchenko]

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;

-- 
With Best Regards,
Andy Shevchenko

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

[Patel, Utkarsh H]

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.

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

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 5/5] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1

[Patel, Utkarsh H]

Hi Sergei,

Thank you for the review.

> -----Original Message-----
> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> Sent: Wednesday, September 20, 2023 2:10 AM
> To: Patel, Utkarsh H <utkarsh.h.patel@intel.com>; linux-
> kernel@vger.kernel.org; linux-usb@vger.kernel.org
> Cc: heikki.krogerus@linux.intel.com; pmalani@chromium.org; chrome-
> platform@lists.linux.dev; andriy.shevchenko@linux.intel.com;
> bleung@chromium.org
> Subject: Re: [PATCH v4 5/5] usb: typec: intel_pmc_mux: Configure Displayport
> Alternate mode 2.1
> 
> On 9/20/23 5:32 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>
> > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> > ---
> > Changes in v4:
> > - No change.
> >
> > Changes in v3:
> > - No change.
> >
> > Changes in v2:
> > - No change.
> >
> >  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 60ed1f809130..233958084b43 100644
> > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> [...]
> > @@ -293,6 +299,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;
> 
>    Why not *switch* instead of string of *if*s?
> 

Switch can be used here but if there is no added benefit, would stick to if/else. 

Sincerely,
Utkarsh Patel. 

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

[Patel, Utkarsh H]

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

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

[patchwork-bot+chrome-platform]

Hello:

This series was applied to chrome-platform/linux.git (for-kernelci)
by Greg Kroah-Hartman <gregkh@linuxfoundation.org>:

On Tue, 19 Sep 2023 19:32:38 -0700 you wrote:
> This series enabels cable identification flow required for Displayport
> Alternate Mode 2.1 support.
> 
> Please add this series to usb-next.
> 
> Changes in v4:
> - Added [PATCH 3/5] back in this series with Reviewed-by tag from Heikki.
> 
> [...]

Here is the summary with links:
  - [v4,1/5] usb: typec: Add Displayport Alternate Mode 2.1 Support
    https://git.kernel.org/chrome-platform/c/a17fae8fc38e
  - [v4,2/5] usb: typec: Add Active or Passive cable defination to cable discover mode VDO
    https://git.kernel.org/chrome-platform/c/c365b1e1f404
  - [v4,3/5] usb: pd: Add helper macro to get Type C cable speed
    https://git.kernel.org/chrome-platform/c/f9ee6043283a
  - [v4,4/5] platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1 Support
    https://git.kernel.org/chrome-platform/c/70ca6c7312c5
  - [v4,5/5] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1
    https://git.kernel.org/chrome-platform/c/6c29de68fb29

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[patchwork-bot+chrome-platform]

Hello:

This series was applied to chrome-platform/linux.git (for-next)
by Greg Kroah-Hartman <gregkh@linuxfoundation.org>:

On Tue, 19 Sep 2023 19:32:38 -0700 you wrote:
> This series enabels cable identification flow required for Displayport
> Alternate Mode 2.1 support.
> 
> Please add this series to usb-next.
> 
> Changes in v4:
> - Added [PATCH 3/5] back in this series with Reviewed-by tag from Heikki.
> 
> [...]

Here is the summary with links:
  - [v4,1/5] usb: typec: Add Displayport Alternate Mode 2.1 Support
    https://git.kernel.org/chrome-platform/c/a17fae8fc38e
  - [v4,2/5] usb: typec: Add Active or Passive cable defination to cable discover mode VDO
    https://git.kernel.org/chrome-platform/c/c365b1e1f404
  - [v4,3/5] usb: pd: Add helper macro to get Type C cable speed
    https://git.kernel.org/chrome-platform/c/f9ee6043283a
  - [v4,4/5] platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1 Support
    https://git.kernel.org/chrome-platform/c/70ca6c7312c5
  - [v4,5/5] usb: typec: intel_pmc_mux: Configure Displayport Alternate mode 2.1
    https://git.kernel.org/chrome-platform/c/6c29de68fb29

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html