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

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

[Utkarsh Patel]

This change adds support to configure retimer cable type

Changes in v4:
- Removed local variables and used inline assignemtns.
- Added details about return values in cros_typec_get_cable_vdo.

Changes in v3:
- Changed the return method in cros_typec_get_cable_vdo.
- Changed passed parameters in cros_typec_get_cable_vdo.
- Corrected definition for unsigned integers as kerenl standard.
- Assigning cable_vdo values directly in to cable_mode.
- Removed unncessary checks for 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 | 28 ++++++++++++++++++++++++-
 drivers/usb/typec/mux/intel_pmc_mux.c   | 28 +++++++++++++++++++++----
 2 files changed, 51 insertions(+), 5 deletions(-)

-- 
2.25.1

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

[Utkarsh Patel]

Connector class driver only configure cable type active or passive.
Configure if the cable type is retimer or redriver with this change.
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 v4:
- Removed local variables and used inline assignemtns.
- Added details about return values in cros_typec_get_cable_vdo.

Changes in v3:
- Changed the return method in cros_typec_get_cable_vdo.
- Changed passed parameters in cros_typec_get_cable_vdo.
- Corrected definition for unsigned integers as kerenl standard.
- Assigning cable_vdo values directly in to cable_mode.
- Removed unncessary checks for Retimer cable type.

Changes in v2:
- Implemented use of cable discover mode vdo.
- Removed adittional changes to host command.
---
---
 drivers/platform/chrome/cros_ec_typec.c | 28 ++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 25f9767c28e8..d0b4d3fc40ed 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -406,6 +406,27 @@ static int cros_typec_usb_safe_state(struct cros_typec_port *port)
 	return ret;
 }
 
+/**
+ * cros_typec_get_cable_vdo() - Get Cable VDO of the connected cable
+ * @port: Type-C port data
+ * @svid: Standard or Vendor ID to match
+ *
+ * Returns the Cable VDO if match is found and returns 0 if match is not found.
+ */
+static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16 svid)
+{
+	struct list_head *head = &port->plug_mode_list;
+	struct cros_typec_altmode_node *node;
+	u32 ret = 0;
+
+	list_for_each_entry(node, head, list) {
+		if (node->amode->svid == svid)
+			return node->amode->vdo;
+	}
+
+	return ret;
+}
+
 /*
  * Spoof the VDOs that were likely communicated by the partner for TBT alt
  * mode.
@@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
 
 	/* Cable Discover Mode VDO */
 	data.cable_mode = TBT_MODE;
+
+	data.cable_mode |= cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
+
 	data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
 
 	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
@@ -522,8 +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
 	/* 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 (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & TBT_CABLE_RETIMER)
 		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;
+	else if (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

[PATCH v4 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 v4:
 - No changes.

Changes in v3:
 - No changes.

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 5e8edf3881c0..888632847a74 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -59,7 +59,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
@@ -71,6 +71,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
@@ -319,8 +320,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);
 
@@ -359,8 +370,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

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

[Greg KH]

On Mon, Jul 17, 2023 at 07:47:03PM -0700, Utkarsh Patel wrote:
> 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 v4:
>  - No changes.
> 
> Changes in v3:
>  - No changes.
> 
> Changes in v2:
>  - No changes.
> ---
> ---
>  drivers/usb/typec/mux/intel_pmc_mux.c | 28 +++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)

Why the 2 --- lines?

And why are you not cc:ing all the proper people (i.e. the person that
can actually apply this...)?

confused,

greg k-h

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

[Patel, Utkarsh H]

Hi Greg,

Thank you for the review and feedback. 

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, July 18, 2023 6:29 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; pmalani@chromium.org;
> bleung@chromium.org
> Subject: Re: [PATCH v4 2/2] usb: typec: intel_pmc_mux: Configure Active and
> Retimer Cable type
> 
> On Mon, Jul 17, 2023 at 07:47:03PM -0700, Utkarsh Patel wrote:
> > 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 v4:
> >  - No changes.
> >
> > Changes in v3:
> >  - No changes.
> >
> > Changes in v2:
> >  - No changes.
> > ---
> > ---
> >  drivers/usb/typec/mux/intel_pmc_mux.c | 28
> > +++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> Why the 2 --- lines?

This is by mistake.

> 
> And why are you not cc:ing all the proper people (i.e. the person that can
> actually apply this...)?
> 
Ack.

I will correct both of these when I send next patch. 

Sincerely,
Utkarsh Patel. 

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

[Patel, Utkarsh H]

Hi Prashant,

Could you please help review v4? 


> -----Original Message-----
> From: Patel, Utkarsh H <utkarsh.h.patel@intel.com>
> Sent: Monday, July 17, 2023 7:47 PM
> To: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> heikki.krogerus@linux.intel.com; pmalani@chromium.org;
> bleung@chromium.org
> Cc: Patel, Utkarsh H <utkarsh.h.patel@intel.com>
> Subject: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure Retimer
> cable type
> 
> Connector class driver only configure cable type active or passive.
> Configure if the cable type is retimer or redriver with this change.
> 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 v4:
> - Removed local variables and used inline assignemtns.
> - Added details about return values in cros_typec_get_cable_vdo.

Sincerely,
Utkarsh Patel.
 

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

[Patel, Utkarsh H]

Hi Prashant and Benson,

Could you please help review?

> > -----Original Message-----
> > From: Patel, Utkarsh H <utkarsh.h.patel@intel.com>
> > Sent: Monday, July 17, 2023 7:47 PM
> > To: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> > heikki.krogerus@linux.intel.com; pmalani@chromium.org;
> > bleung@chromium.org
> > Cc: Patel, Utkarsh H <utkarsh.h.patel@intel.com>
> > Subject: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure
> > Retimer cable type
> >
> > Connector class driver only configure cable type active or passive.
> > Configure if the cable type is retimer or redriver with this change.
> > 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 v4:
> > - Removed local variables and used inline assignemtns.
> > - Added details about return values in cros_typec_get_cable_vdo.

Sincerely,
Utkarsh Patel.

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

[Prashant Malani]

Hi Utkarsh,

Sorry I was out on leave, so didnt get to this earlier.


On Jul 17 19:47, Utkarsh Patel wrote:
>  
> +/**
> + * cros_typec_get_cable_vdo() - Get Cable VDO of the connected cable
> + * @port: Type-C port data
> + * @svid: Standard or Vendor ID to match
> + *
> + * Returns the Cable VDO if match is found and returns 0 if match is not found.
> + */
> +static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16 svid)
return type should be u32.

> +{
> +	struct list_head *head = &port->plug_mode_list;
> +	struct cros_typec_altmode_node *node;
> +	u32 ret = 0;
> +
> +	list_for_each_entry(node, head, list) {
> +		if (node->amode->svid == svid)
> +			return node->amode->vdo;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * Spoof the VDOs that were likely communicated by the partner for TBT alt
>   * mode.
> @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
>  
>  	/* Cable Discover Mode VDO */
>  	data.cable_mode = TBT_MODE;
> +
> +	data.cable_mode |= cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
> +
>  	data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
>  
>  	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
> @@ -522,8 +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
>  	/* 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 (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) & TBT_CABLE_RETIMER)
>  		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;
We shouldn't need to call cros_typec_get_cable_vdo more than once. Either call it once earlier
when you are crafting the data.cable_mode member and then re-use that variable here. Or don't
call it there and just call it here.

> +	else if (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 v4 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type

[Patel, Utkarsh H]

Hi Prashant,

Thank you for the review. 

> -----Original Message-----
> From: Prashant Malani <pmalani@chromium.org>
> Sent: Friday, August 18, 2023 10:02 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; bleung@chromium.org
> Subject: Re: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure
> Retimer cable type
> 
> >  /*
> >   * Spoof the VDOs that were likely communicated by the partner for TBT alt
> >   * mode.
> > @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct
> > cros_typec_data *typec,
> >
> >  	/* Cable Discover Mode VDO */
> >  	data.cable_mode = TBT_MODE;
> > +
> > +	data.cable_mode |= cros_typec_get_cable_vdo(port,
> > +USB_TYPEC_TBT_SID);
> > +
> >  	data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
> >
> >  	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@ -
> 522,8
> > +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data
> *typec,
> >  	/* 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 (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) &
> > +TBT_CABLE_RETIMER)
> >  		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER <<
> EUDO_CABLE_TYPE_SHIFT;
> We shouldn't need to call cros_typec_get_cable_vdo more than once. Either
> call it once earlier when you are crafting the data.cable_mode member and
> then re-use that variable here. Or don't call it there and just call it here.

We are only calling it once depending upon which mode we enter TBT Alt or USB4.

Sincerely,
Utkarsh Patel.

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

[Prashant Malani]

On Mon, Aug 21, 2023 at 10:18 AM Patel, Utkarsh H
<utkarsh.h.patel@intel.com> wrote:
>
> Hi Prashant,
>
> Thank you for the review.
>
> > -----Original Message-----
> > From: Prashant Malani <pmalani@chromium.org>
> > Sent: Friday, August 18, 2023 10:02 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; bleung@chromium.org
> > Subject: Re: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure
> > Retimer cable type
> >
> > >  /*
> > >   * Spoof the VDOs that were likely communicated by the partner for TBT alt
> > >   * mode.
> > > @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct
> > > cros_typec_data *typec,
> > >
> > >     /* Cable Discover Mode VDO */
> > >     data.cable_mode = TBT_MODE;
> > > +
> > > +   data.cable_mode |= cros_typec_get_cable_vdo(port,
> > > +USB_TYPEC_TBT_SID);
> > > +

Here is the first call to cros_typec_get_cable_vdo(port, TBT)....

> > >     data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
> > >
> > >     if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@ -
> > 522,8
> > > +546,10 @@ static int cros_typec_enable_usb4(struct cros_typec_data
> > *typec,
> > >     /* 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 (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) &
> > > +TBT_CABLE_RETIMER)

And here is the 2nd.

> > >             data.eudo |= EUDO_CABLE_TYPE_RE_TIMER <<
> > EUDO_CABLE_TYPE_SHIFT;
> > We shouldn't need to call cros_typec_get_cable_vdo more than once. Either
> > call it once earlier when you are crafting the data.cable_mode member and
> > then re-use that variable here. Or don't call it there and just call it here.
>
> We are only calling it once depending upon which mode we enter TBT Alt or USB4.

There should only be 1 "call site" and that should be sufficient to
grab the VDO from the
framework for all circumstances. Whether the other invocation doesn't get called
under certain circumstances isn't as relevant.

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

[Patel, Utkarsh H]

Hi Prashant,

> -----Original Message-----
> From: Prashant Malani <pmalani@chromium.org>
> Sent: Monday, August 21, 2023 4:31 PM
> 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; bleung@chromium.org
> Subject: Re: [PATCH v4 1/2] platform/chrome: cros_ec_typec: Configure
> Retimer cable type
> 
> On Mon, Aug 21, 2023 at 10:18 AM Patel, Utkarsh H
> <utkarsh.h.patel@intel.com> wrote:
> > > >  /*
> > > >   * Spoof the VDOs that were likely communicated by the partner for TBT
> alt
> > > >   * mode.
> > > > @@ -432,6 +453,9 @@ static int cros_typec_enable_tbt(struct
> > > > cros_typec_data *typec,
> > > >
> > > >     /* Cable Discover Mode VDO */
> > > >     data.cable_mode = TBT_MODE;
> > > > +
> > > > +   data.cable_mode |= cros_typec_get_cable_vdo(port,
> > > > +USB_TYPEC_TBT_SID);
> > > > +
> 
> Here is the first call to cros_typec_get_cable_vdo(port, TBT)....
> 
> > > >     data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
> > > >
> > > >     if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@ -
> > > 522,8
> > > > +546,10 @@ static int cros_typec_enable_usb4(struct
> > > > +cros_typec_data
> > > *typec,
> > > >     /* 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 (cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID) &
> > > > +TBT_CABLE_RETIMER)
> 
> And here is the 2nd.
> 
> > > >             data.eudo |= EUDO_CABLE_TYPE_RE_TIMER <<
> > > EUDO_CABLE_TYPE_SHIFT;
> > > We shouldn't need to call cros_typec_get_cable_vdo more than once.
> > > Either call it once earlier when you are crafting the
> > > data.cable_mode member and then re-use that variable here. Or don't call
> it there and just call it here.
> >
> > We are only calling it once depending upon which mode we enter TBT Alt or
> USB4.
> 
> There should only be 1 "call site" and that should be sufficient to grab the VDO
> from the framework for all circumstances. Whether the other invocation
> doesn't get called under certain circumstances isn't as relevant.

Are you suggesting something like this?

static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,)...

	if (port->mux_flags & USB_PD_MUX_USB4_ENABLED ||
		port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED)
		cable_tbt_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);

Sincerely,
Utkarsh Patel.

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

[Prashant Malani]

Hi Utkarsh,

On Aug 22 21:21, Patel, Utkarsh H wrote:
> Hi Prashant,
> 
> > -----Original Message-----
> > From: Prashant Malani <pmalani@chromium.org>
> > Sent: Monday, August 21, 2023 4:31 PM
> > >
> > > We are only calling it once depending upon which mode we enter TBT Alt or
> > USB4.
> > 
> > There should only be 1 "call site" and that should be sufficient to grab the VDO
> > from the framework for all circumstances. Whether the other invocation
> > doesn't get called under certain circumstances isn't as relevant.
> 
> Are you suggesting something like this?
> 
> static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,)...
> 
> 	if (port->mux_flags & USB_PD_MUX_USB4_ENABLED ||
> 		port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED)
> 		cable_tbt_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);

My apologies, I misread the patch. I think this looks good.

Acked-by: Prashant Malani <pmalani@chromium.org>

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

[Patel, Utkarsh H]

Hi Greg,

> > > >
> > > > We are only calling it once depending upon which mode we enter TBT
> > > > Alt or
> > > USB4.
> > >
> > > There should only be 1 "call site" and that should be sufficient to
> > > grab the VDO from the framework for all circumstances. Whether the
> > > other invocation doesn't get called under certain circumstances isn't as
> relevant.
> >
> > Are you suggesting something like this?
> >
> > static int cros_typec_configure_mux(struct cros_typec_data *typec, int
> port_num,)...
> >
> > 	if (port->mux_flags & USB_PD_MUX_USB4_ENABLED ||
> > 		port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED)
> > 		cable_tbt_vdo = cros_typec_get_cable_vdo(port,
> USB_TYPEC_TBT_SID);
> 
> My apologies, I misread the patch. I think this looks good.
> 
> Acked-by: Prashant Malani <pmalani@chromium.org>

Could you please add this patch to usb-next.

Sincerely,
Utkarsh Patel. 

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

[Greg KH]

On Fri, Aug 25, 2023 at 09:04:29PM +0000, Patel, Utkarsh H wrote:
> Hi Greg,
> 
> > > > >
> > > > > We are only calling it once depending upon which mode we enter TBT
> > > > > Alt or
> > > > USB4.
> > > >
> > > > There should only be 1 "call site" and that should be sufficient to
> > > > grab the VDO from the framework for all circumstances. Whether the
> > > > other invocation doesn't get called under certain circumstances isn't as
> > relevant.
> > >
> > > Are you suggesting something like this?
> > >
> > > static int cros_typec_configure_mux(struct cros_typec_data *typec, int
> > port_num,)...
> > >
> > > 	if (port->mux_flags & USB_PD_MUX_USB4_ENABLED ||
> > > 		port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED)
> > > 		cable_tbt_vdo = cros_typec_get_cable_vdo(port,
> > USB_TYPEC_TBT_SID);
> > 
> > My apologies, I misread the patch. I think this looks good.
> > 
> > Acked-by: Prashant Malani <pmalani@chromium.org>
> 
> Could you please add this patch to usb-next.

Ugh, it's late in the cycle, but this has been around for a while, let
me go see...

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

[Greg KH]

On Sat, Aug 26, 2023 at 09:40:20AM +0200, Greg KH wrote:
> On Fri, Aug 25, 2023 at 09:04:29PM +0000, Patel, Utkarsh H wrote:
> > Hi Greg,
> > 
> > > > > >
> > > > > > We are only calling it once depending upon which mode we enter TBT
> > > > > > Alt or
> > > > > USB4.
> > > > >
> > > > > There should only be 1 "call site" and that should be sufficient to
> > > > > grab the VDO from the framework for all circumstances. Whether the
> > > > > other invocation doesn't get called under certain circumstances isn't as
> > > relevant.
> > > >
> > > > Are you suggesting something like this?
> > > >
> > > > static int cros_typec_configure_mux(struct cros_typec_data *typec, int
> > > port_num,)...
> > > >
> > > > 	if (port->mux_flags & USB_PD_MUX_USB4_ENABLED ||
> > > > 		port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED)
> > > > 		cable_tbt_vdo = cros_typec_get_cable_vdo(port,
> > > USB_TYPEC_TBT_SID);
> > > 
> > > My apologies, I misread the patch. I think this looks good.
> > > 
> > > Acked-by: Prashant Malani <pmalani@chromium.org>
> > 
> > Could you please add this patch to usb-next.
> 
> Ugh, it's late in the cycle, but this has been around for a while, let
> me go see...

Looks like patch 2/2 was already in my tree, so I've queued up this one
now as well.

thanks,

greg k-h