diff mbox series

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

Message ID 20230628173727.701140-2-utkarsh.h.patel@intel.com (mailing list archive)
State Superseded
Headers show
Series Add support to configure active retimer cable | expand

Commit Message

Patel, Utkarsh H June 28, 2023, 5:37 p.m. UTC 
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(-)

Comments

Prashant Malani June 29, 2023, 5:38 p.m. UTC | #1 
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
>
Patel, Utkarsh H June 30, 2023, 12:43 a.m. UTC | #2 
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.
diff mbox series

Patch

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