[PATCH v2 0/2] Add support to configure active retimer cable

[PATCH v2 0/2] Add support to configure active retimer cable

[Utkarsh Patel]

This change adds support to configure retimer cable type

Changes in v2:
- Implemented use of cable discover mode vdo.
- Removed adittional changes to host command.

Utkarsh Patel (2):
  platform/chrome: cros_ec_typec: Configure Retimer cable type
  usb: typec: intel_pmc_mux: Configure Active and Retimer Cable type

 drivers/platform/chrome/cros_ec_typec.c | 33 ++++++++++++++++++++++++-
 drivers/usb/typec/mux/intel_pmc_mux.c   | 28 ++++++++++++++++++---
 2 files changed, 56 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type

[Utkarsh Patel]

Connector class driver only configure cable type active or passive.
With this change it will also configure if the cable type is retimer or
redriver if required by AP. This detail will be provided as a part of
cable discover mode VDO.

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

---
Changes in v2:
 - Implemented use of cable discover mode vdo.
 - Removed adittional changes to host command. 
---

---
 drivers/platform/chrome/cros_ec_typec.c | 33 ++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 25f9767c28e8..557f396d1c00 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -406,6 +406,25 @@ static int cros_typec_usb_safe_state(struct cros_typec_port *port)
 	return ret;
 }
 
+static int cros_typec_get_cable_vdo(struct cros_typec_data *typec, int port_num,
+				    uint16_t svid)
+{
+	struct cros_typec_port *port = typec->ports[port_num];
+	struct list_head *head = &port->plug_mode_list;
+	struct cros_typec_altmode_node *node;
+
+	list_for_each_entry(node, head, list) {
+		if (node->amode->svid == svid)
+			break;
+	}
+
+	if (node->amode->svid != svid)
+		return 0;
+
+	return node->amode->vdo;
+}
+
+
 /*
  * Spoof the VDOs that were likely communicated by the partner for TBT alt
  * mode.
@@ -416,6 +435,7 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
 {
 	struct cros_typec_port *port = typec->ports[port_num];
 	struct typec_thunderbolt_data data;
+	uint32_t cable_vdo;
 	int ret;
 
 	if (typec->pd_ctrl_ver < 2) {
@@ -442,6 +462,11 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
 
 	data.cable_mode |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);
 
+	cable_vdo = cros_typec_get_cable_vdo(typec, port_num, USB_TYPEC_TBT_SID);
+
+	if (cable_vdo & TBT_CABLE_RETIMER)
+		data.cable_mode |= TBT_CABLE_RETIMER;
+
 	/* Enter Mode VDO */
 	data.enter_vdo = TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
 
@@ -513,17 +538,23 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
 {
 	struct cros_typec_port *port = typec->ports[port_num];
 	struct enter_usb_data data;
+	uint32_t cable_vdo;
 
 	data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
 
+	cable_vdo = cros_typec_get_cable_vdo(typec, port_num, USB_TYPEC_TBT_SID);
+
 	/* Cable Speed */
 	data.eudo |= pd_ctrl->cable_speed << EUDO_CABLE_SPEED_SHIFT;
 
 	/* Cable Type */
 	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
 		data.eudo |= EUDO_CABLE_TYPE_OPTICAL << EUDO_CABLE_TYPE_SHIFT;
-	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
+	else if (cable_vdo & TBT_CABLE_RETIMER)
 		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;
+	else if (!(cable_vdo & TBT_CABLE_RETIMER) &&
+		 (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE))
+		data.eudo |= EUDO_CABLE_TYPE_RE_DRIVER << EUDO_CABLE_TYPE_SHIFT;
 
 	data.active_link_training = !!(pd_ctrl->control_flags &
 				       USB_PD_CTRL_ACTIVE_LINK_UNIDIR);
-- 
2.25.1

Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type

[Prashant Malani]

Hi Utkarsh,

Thanks for sending the patch.

On Jun 28 10:37, Utkarsh Patel wrote:
> Connector class driver only configure cable type active or passive.
> With this change it will also configure if the cable type is retimer or

nit: Please use imperative form ("Configure if the cable type is...")

> redriver if required by AP. This detail will be provided as a part of
> cable discover mode VDO.
> 
> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> 
> ---
> Changes in v2:
>  - Implemented use of cable discover mode vdo.
>  - Removed adittional changes to host command. 
> ---
> 
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 33 ++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 25f9767c28e8..557f396d1c00 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -406,6 +406,25 @@ static int cros_typec_usb_safe_state(struct cros_typec_port *port)
>  	return ret;
>  }
>  
> +static int cros_typec_get_cable_vdo(struct cros_typec_data *typec, int port_num,
> +				    uint16_t svid)

u16 type is used in the kernel.

Also, if this function returns a VDO, the return type should be u32, but... (see later)

> +{
> +	struct cros_typec_port *port = typec->ports[port_num];

Pass the struct cros_typec_port directly (and then drop the port_num argument).

> +	struct list_head *head = &port->plug_mode_list;
> +	struct cros_typec_altmode_node *node;
> +
> +	list_for_each_entry(node, head, list) {
> +		if (node->amode->svid == svid)
> +			break;

Return the vdo here directly; that way, if you reach past the list iteration,
we know for sure the SVID wasn't found and you can unconditionally return the error
case.

> +	}
> +
> +	if (node->amode->svid != svid)
> +		return 0;

I think it is more correct here to have an int return type (so the "not found" case can
return -1 or the right error code), and then have the cable VDO as a pointer argument.

> +
> +	return node->amode->vdo;
> +}
> +
> +
>  /*
>   * Spoof the VDOs that were likely communicated by the partner for TBT alt
>   * mode.
> @@ -416,6 +435,7 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
>  {
>  	struct cros_typec_port *port = typec->ports[port_num];
>  	struct typec_thunderbolt_data data;
> +	uint32_t cable_vdo;
u32.

>  	int ret;
>  
>  	if (typec->pd_ctrl_ver < 2) {
> @@ -442,6 +462,11 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
>  
>  	data.cable_mode |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);

Probably a separate patch, but can we get rid of this too, since the cable_vdo should
have this information?

>  
> +	cable_vdo = cros_typec_get_cable_vdo(typec, port_num, USB_TYPEC_TBT_SID);
> +
> +	if (cable_vdo & TBT_CABLE_RETIMER)
> +		data.cable_mode |= TBT_CABLE_RETIMER;

Why just not or the cable_vdo into existing cable_mode"? :

data.cable_mode |= cable_vdo;

> +
>  	/* Enter Mode VDO */
>  	data.enter_vdo = TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
>  
> @@ -513,17 +538,23 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
>  {
>  	struct cros_typec_port *port = typec->ports[port_num];
>  	struct enter_usb_data data;
> +	uint32_t cable_vdo;

u32

>  
>  	data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
>  
> +	cable_vdo = cros_typec_get_cable_vdo(typec, port_num, USB_TYPEC_TBT_SID);
> +
>  	/* Cable Speed */
>  	data.eudo |= pd_ctrl->cable_speed << EUDO_CABLE_SPEED_SHIFT;
>  
>  	/* Cable Type */
>  	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
>  		data.eudo |= EUDO_CABLE_TYPE_OPTICAL << EUDO_CABLE_TYPE_SHIFT;
> -	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
> +	else if (cable_vdo & TBT_CABLE_RETIMER)
>  		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;
> +	else if (!(cable_vdo & TBT_CABLE_RETIMER) &&

The !(cable_vdo & TBT_CABLE_RETIMER) check shouldn't be necessary; the
earlier "else if" already ensures that by the time we reach here, this
clause is satisfied.

> +		 (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE))
> +		data.eudo |= EUDO_CABLE_TYPE_RE_DRIVER << EUDO_CABLE_TYPE_SHIFT;
>  
>  	data.active_link_training = !!(pd_ctrl->control_flags &
>  				       USB_PD_CTRL_ACTIVE_LINK_UNIDIR);
> -- 
> 2.25.1
> 

RE: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type

[Patel, Utkarsh H]

Hi Prashant,

Thank you for the review and feedback.

> Hi Utkarsh,
> 
> Thanks for sending the patch.
> 
> On Jun 28 10:37, Utkarsh Patel wrote:
> > Connector class driver only configure cable type active or passive.
> > With this change it will also configure if the cable type is retimer
> > or
> 
> nit: Please use imperative form ("Configure if the cable type is...")

Ack.

> 
> > redriver if required by AP. This detail will be provided as a part of
> > cable discover mode VDO.
> >
> > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> >

> > +static int cros_typec_get_cable_vdo(struct cros_typec_data *typec, int
> port_num,
> > +				    uint16_t svid)
> 
> u16 type is used in the kernel.

Ack. 

> 
> Also, if this function returns a VDO, the return type should be u32, but... (see
> later)
> 
> > +{
> > +	struct cros_typec_port *port = typec->ports[port_num];
> 
> Pass the struct cros_typec_port directly (and then drop the port_num
> argument).

Ack.

> 
> > +	struct list_head *head = &port->plug_mode_list;
> > +	struct cros_typec_altmode_node *node;
> > +
> > +	list_for_each_entry(node, head, list) {
> > +		if (node->amode->svid == svid)
> > +			break;
> 
> Return the vdo here directly; that way, if you reach past the list iteration, we
> know for sure the SVID wasn't found and you can unconditionally return the
> error case.

Ack.

> 
> > +	}
> > +
> > +	if (node->amode->svid != svid)
> > +		return 0;
> 
> I think it is more correct here to have an int return type (so the "not found"
> case can return -1 or the right error code), and then have the cable VDO as a
> pointer argument.

Ack.

> > +	uint32_t cable_vdo;
> u32.

Ack.

> 
> >  	int ret;
> >
> >  	if (typec->pd_ctrl_ver < 2) {
> > @@ -442,6 +462,11 @@ static int cros_typec_enable_tbt(struct
> > cros_typec_data *typec,
> >
> >  	data.cable_mode |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);
> 
> Probably a separate patch, but can we get rid of this too, since the cable_vdo
> should have this information?

Yes, it will need separate patch as there are other capabilities in cable mode and all can be removed once this change goes through.

> 
> >
> > +	cable_vdo = cros_typec_get_cable_vdo(typec, port_num,
> > +USB_TYPEC_TBT_SID);
> > +
> > +	if (cable_vdo & TBT_CABLE_RETIMER)
> > +		data.cable_mode |= TBT_CABLE_RETIMER;
> 
> Why just not or the cable_vdo into existing cable_mode"? :
> 
> data.cable_mode |= cable_vdo;

Agree, with this all other configs for cable mode can be removed and mux driver will just use cable mode VDO as is.

> 
> > +
> >  	/* Enter Mode VDO */
> >  	data.enter_vdo = TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
> >
> > @@ -513,17 +538,23 @@ static int cros_typec_enable_usb4(struct
> > cros_typec_data *typec,  {
> >  	struct cros_typec_port *port = typec->ports[port_num];
> >  	struct enter_usb_data data;
> > +	uint32_t cable_vdo;
> 
> u32

Ack.

> 
> >
> >  	data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
> >
> > +	cable_vdo = cros_typec_get_cable_vdo(typec, port_num,
> > +USB_TYPEC_TBT_SID);
> > +
> >  	/* Cable Speed */
> >  	data.eudo |= pd_ctrl->cable_speed << EUDO_CABLE_SPEED_SHIFT;
> >
> >  	/* Cable Type */
> >  	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
> >  		data.eudo |= EUDO_CABLE_TYPE_OPTICAL <<
> EUDO_CABLE_TYPE_SHIFT;
> > -	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
> > +	else if (cable_vdo & TBT_CABLE_RETIMER)
> >  		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER <<
> EUDO_CABLE_TYPE_SHIFT;
> > +	else if (!(cable_vdo & TBT_CABLE_RETIMER) &&
> 
> The !(cable_vdo & TBT_CABLE_RETIMER) check shouldn't be necessary; the
> earlier "else if" already ensures that by the time we reach here, this clause is
> satisfied.

Ack.

Sincerely,
Utkarsh Patel.

[PATCH v2 2/2] usb: typec: intel_pmc_mux: Configure Active and Retimer Cable type

[Utkarsh Patel]

Cable type such as active and retimer received as a part of Thunderbolt3
or Thunderbolt4 cable discover mode VDO needs to be configured in the
thunderbolt alternate mode.

Configuring the register bits for this cable type is changed with Intel
Meteor Lake platform. BIT2 for Retimer/Redriver cable and BIT22 for
Active/Passive cable.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
---
Changes in v2:
 - No changes.
---
---
 drivers/usb/typec/mux/intel_pmc_mux.c | 28 +++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index e049eadb591e..ac088e2e49bc 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -57,7 +57,7 @@ enum {
 };
 
 /* Common Mode Data bits */
-#define PMC_USB_ALTMODE_ACTIVE_CABLE	BIT(2)
+#define PMC_USB_ALTMODE_RETIMER_CABLE	BIT(2)
 
 #define PMC_USB_ALTMODE_ORI_SHIFT	1
 #define PMC_USB_ALTMODE_UFP_SHIFT	3
@@ -69,6 +69,7 @@ enum {
 #define PMC_USB_ALTMODE_TBT_TYPE	BIT(17)
 #define PMC_USB_ALTMODE_CABLE_TYPE	BIT(18)
 #define PMC_USB_ALTMODE_ACTIVE_LINK	BIT(20)
+#define PMC_USB_ALTMODE_ACTIVE_CABLE	BIT(22)
 #define PMC_USB_ALTMODE_FORCE_LSR	BIT(23)
 #define PMC_USB_ALTMODE_CABLE_SPD(_s_)	(((_s_) & GENMASK(2, 0)) << 25)
 #define   PMC_USB_ALTMODE_CABLE_USB31	1
@@ -313,8 +314,18 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
 	if (data->cable_mode & TBT_CABLE_LINK_TRAINING)
 		req.mode_data |= PMC_USB_ALTMODE_ACTIVE_LINK;
 
-	if (data->enter_vdo & TBT_ENTER_MODE_ACTIVE_CABLE)
-		req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+	if (acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1072", NULL) ||
+	    acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1079", NULL)) {
+		if ((data->enter_vdo & TBT_ENTER_MODE_ACTIVE_CABLE) ||
+		    (data->cable_mode & TBT_CABLE_RETIMER))
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+	} else {
+		if (data->enter_vdo & TBT_ENTER_MODE_ACTIVE_CABLE)
+			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+
+		if (data->cable_mode & TBT_CABLE_RETIMER)
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+	}
 
 	req.mode_data |= PMC_USB_ALTMODE_CABLE_SPD(cable_speed);
 
@@ -353,8 +364,17 @@ pmc_usb_mux_usb4(struct pmc_usb_port *port, struct typec_mux_state *state)
 	case EUDO_CABLE_TYPE_OPTICAL:
 		req.mode_data |= PMC_USB_ALTMODE_CABLE_TYPE;
 		fallthrough;
+	case EUDO_CABLE_TYPE_RE_TIMER:
+		if (!acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1072", NULL) ||
+		    !acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1079", NULL))
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+		fallthrough;
 	default:
-		req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
+		if (acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1072", NULL) ||
+		    acpi_dev_hid_uid_match(port->pmc->iom_adev, "INTC1079", NULL))
+			req.mode_data |= PMC_USB_ALTMODE_RETIMER_CABLE;
+		else
+			req.mode_data |= PMC_USB_ALTMODE_ACTIVE_CABLE;
 
 		/* Configure data rate to rounded in the case of Active TBT3
 		 * and USB4 cables.
-- 
2.25.1