diff mbox series

[v3,2/4] platform/chrome: cros_ec_typec: Use Thunderbolt 3 cable discover mode VDO in USB4 mode

Message ID 20201119063211.2264-3-utkarsh.h.patel@intel.com
State New
Headers show
Series None | expand

Commit Message

Patel, Utkarsh H Nov. 19, 2020, 6:32 a.m. UTC
Configure Thunderbolt 3 cable generation value by filling Thunderbolt 3
cable discover mode VDO to support rounded Thunderbolt 3 cables.
While we are here use Thunderbolt 3 cable discover mode VDO to fill active
cable plug link training value.

Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>

--
Changes in v3:
- Added a check for cable's TBT support before filling TBT3 discover mode
  VDO.

Changes in v2:
- No change.
--
---
 drivers/platform/chrome/cros_ec_typec.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Patel, Utkarsh H Nov. 20, 2020, 2:32 a.m. UTC | #1
Hi Prashant,

> -----Original Message-----

> From: Prashant Malani <pmalani@chromium.org>

> Sent: Thursday, November 19, 2020 12:09 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; enric.balletbo@collabora.com; Mani,

> Rajmohan <rajmohan.mani@intel.com>; Shaikh, Azhar

> <azhar.shaikh@intel.com>

> Subject: Re: [PATCH v3 2/4] platform/chrome: cros_ec_typec: Use Thunderbolt

> 3 cable discover mode VDO in USB4 mode

> 

> Hi Utkarsh,

> 

> On Wed, Nov 18, 2020 at 10:32:09PM -0800, Utkarsh Patel wrote:

> > Configure Thunderbolt 3 cable generation value by filling Thunderbolt

> > 3 cable discover mode VDO to support rounded Thunderbolt 3 cables.

> > While we are here use Thunderbolt 3 cable discover mode VDO to fill

> > active cable plug link training value.

> >

> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

> >

> > --

> > Changes in v3:

> > - Added a check for cable's TBT support before filling TBT3 discover mode

> >   VDO.

> >

> > Changes in v2:

> > - No change.

> > --

> > ---

> >  drivers/platform/chrome/cros_ec_typec.c | 14 ++++++++++++--

> >  1 file changed, 12 insertions(+), 2 deletions(-)

> >

> > diff --git a/drivers/platform/chrome/cros_ec_typec.c

> > b/drivers/platform/chrome/cros_ec_typec.c

> > index 8111ed1fc574..68b17ee1d1ae 100644

> > --- a/drivers/platform/chrome/cros_ec_typec.c

> > +++ b/drivers/platform/chrome/cros_ec_typec.c

> > @@ -514,8 +514,18 @@ static int cros_typec_enable_usb4(struct

> cros_typec_data *typec,

> >  	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)

> >  		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER <<

> EUDO_CABLE_TYPE_SHIFT;

> >

> > -	data.active_link_training = !!(pd_ctrl->control_flags &

> > -				       USB_PD_CTRL_ACTIVE_LINK_UNIDIR);

> > +	/*

> > +	 * Filling TBT3 Cable VDO when TBT3 cable is being used to establish

> > +	 * USB4 connection.

> > +	 */

> > +	if (pd_ctrl->cable_gen) {

> > +		data.tbt_cable_vdo = TBT_MODE;

> > +

> > +		if (pd_ctrl->control_flags &

> USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> > +			data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> > +

> > +		data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl-

> >cable_gen);

> > +	}

> 

> I think the following would decouple Rounded Support and Active Cable Link

> Training?:


Any reason you would want to decouple them?
As for the TBT3 active cable to create USB4 connection, it needs have rounded data rates. 

> 

> 	struct typec_thunderbolt_data data = {}; ...

> 	if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> 		data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> 

> 	data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl-

> >cable_gen);

> 

> 	if (data.tbt_cable_vdo)

> 		data.tbt_cable_vdo |= TBT_MODE;

> 

> 

> Best regards,

> 

> -Prashant


Sincerely,
Utkarsh Patel.
Prashant Malani Nov. 20, 2020, 3:13 a.m. UTC | #2
Hi Utkarsh,

On Thu, Nov 19, 2020 at 6:32 PM Patel, Utkarsh H
<utkarsh.h.patel@intel.com> wrote:
>

> Hi Prashant,

>

> > -----Original Message-----

> > From: Prashant Malani <pmalani@chromium.org>

> > Sent: Thursday, November 19, 2020 12:09 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; enric.balletbo@collabora.com; Mani,

> > Rajmohan <rajmohan.mani@intel.com>; Shaikh, Azhar

> > <azhar.shaikh@intel.com>

> > Subject: Re: [PATCH v3 2/4] platform/chrome: cros_ec_typec: Use Thunderbolt

> > 3 cable discover mode VDO in USB4 mode

> >

> > Hi Utkarsh,

> >

> > On Wed, Nov 18, 2020 at 10:32:09PM -0800, Utkarsh Patel wrote:

> > > Configure Thunderbolt 3 cable generation value by filling Thunderbolt

> > > 3 cable discover mode VDO to support rounded Thunderbolt 3 cables.

> > > While we are here use Thunderbolt 3 cable discover mode VDO to fill

> > > active cable plug link training value.

> > >

> > > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

> > >

> > > --

> > > Changes in v3:

> > > - Added a check for cable's TBT support before filling TBT3 discover mode

> > >   VDO.

> > >

> > > Changes in v2:

> > > - No change.

> > > --

> > > ---

> > >  drivers/platform/chrome/cros_ec_typec.c | 14 ++++++++++++--

> > >  1 file changed, 12 insertions(+), 2 deletions(-)

> > >

> > > diff --git a/drivers/platform/chrome/cros_ec_typec.c

> > > b/drivers/platform/chrome/cros_ec_typec.c

> > > index 8111ed1fc574..68b17ee1d1ae 100644

> > > --- a/drivers/platform/chrome/cros_ec_typec.c

> > > +++ b/drivers/platform/chrome/cros_ec_typec.c

> > > @@ -514,8 +514,18 @@ static int cros_typec_enable_usb4(struct

> > cros_typec_data *typec,

> > >     else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)

> > >             data.eudo |= EUDO_CABLE_TYPE_RE_TIMER <<

> > EUDO_CABLE_TYPE_SHIFT;

> > >

> > > -   data.active_link_training = !!(pd_ctrl->control_flags &

> > > -                                  USB_PD_CTRL_ACTIVE_LINK_UNIDIR);

> > > +   /*

> > > +    * Filling TBT3 Cable VDO when TBT3 cable is being used to establish

> > > +    * USB4 connection.

> > > +    */

> > > +   if (pd_ctrl->cable_gen) {

> > > +           data.tbt_cable_vdo = TBT_MODE;

> > > +

> > > +           if (pd_ctrl->control_flags &

> > USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> > > +                   data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> > > +

> > > +           data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl-

> > >cable_gen);

> > > +   }

> >

> > I think the following would decouple Rounded Support and Active Cable Link

> > Training?:

>

> Any reason you would want to decouple them?


Is there anything in the spec that says Active Cable Link Training
needs Rounded Cable support (or vice versa)?
If yes, could you kindly point me to the relevant portion in the spec
that states this?

If no, then the two should be set independently based on the response
from the Chrome EC.

FWIW, Table F-11 ( TBT3 Cable Discover Mode VDO Responses) from the
USB Type-C Cable & Connector Spec (Rel 2.0) suggests
the two are independent bits although I don't have access to the TBT3
spec to confirm.

BR,

-Prashant
Heikki Krogerus Nov. 20, 2020, 11:22 a.m. UTC | #3
On Thu, Nov 19, 2020 at 12:09:06AM -0800, Prashant Malani wrote:
> Hi Utkarsh,

> 

> On Wed, Nov 18, 2020 at 10:32:09PM -0800, Utkarsh Patel wrote:

> > Configure Thunderbolt 3 cable generation value by filling Thunderbolt 3

> > cable discover mode VDO to support rounded Thunderbolt 3 cables.

> > While we are here use Thunderbolt 3 cable discover mode VDO to fill active

> > cable plug link training value.

> > 

> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

> > 

> > --

> > Changes in v3:

> > - Added a check for cable's TBT support before filling TBT3 discover mode

> >   VDO.

> > 

> > Changes in v2:

> > - No change.

> > --

> > ---

> >  drivers/platform/chrome/cros_ec_typec.c | 14 ++++++++++++--

> >  1 file changed, 12 insertions(+), 2 deletions(-)

> > 

> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c

> > index 8111ed1fc574..68b17ee1d1ae 100644

> > --- a/drivers/platform/chrome/cros_ec_typec.c

> > +++ b/drivers/platform/chrome/cros_ec_typec.c

> > @@ -514,8 +514,18 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,

> >  	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)

> >  		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;

> >  

> > -	data.active_link_training = !!(pd_ctrl->control_flags &

> > -				       USB_PD_CTRL_ACTIVE_LINK_UNIDIR);

> > +	/*

> > +	 * Filling TBT3 Cable VDO when TBT3 cable is being used to establish

> > +	 * USB4 connection.

> > +	 */

> > +	if (pd_ctrl->cable_gen) {

> > +		data.tbt_cable_vdo = TBT_MODE;

> > +

> > +		if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> > +			data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> > +

> > +		data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);

> > +	}

> 

> I think the following would decouple Rounded Support and Active Cable Link

> Training?:

> 

> 	struct typec_thunderbolt_data data = {};

> ...

> 	if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> 		data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> 

> 	data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);


I agree with this. We should not modify the data that we actually
have access to.

> 	if (data.tbt_cable_vdo)

> 		data.tbt_cable_vdo |= TBT_MODE;


That is wrong. If the LSRX communication is bi-directional, and/or the
data rates are non-rounded, the cable has to be TBT3 cable. So I think
what you would want is:

	if (!data.tbt_cable_vdo)
 		data.tbt_cable_vdo = TBT_MODE;

But of course we can not do that either, because we have to set the
TBT_MODE bit if there is any other data in tbt_cable_vdo (USB Type-C
spec does not define any other valid values except 0x0001 = TBT_MODE
for that field). Otherwise the mux driver should consider the data
corrupted. So we would have to do this:

        if (pd_ctrl->cable_gen &&
            pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)
                data.tbt_cable_vdo = 0; /* We assume USB4 cable */
        else
 		data.tbt_cable_vdo |= TBT_MODE; /* It is for sure TBT3 cable */

But I would not do that. TBT3 cable can also support unidirectional
SBU communication and rounded data rates.

IMO safer bet for now would be to just claim that the cable is always
TBT3 cable until we have access to information that can really tell us
is the cable USB4 or TBT3.

But it's up to you guys.


Br,

-- 
heikki
Prashant Malani Nov. 20, 2020, 12:07 p.m. UTC | #4
On Fri, Nov 20, 2020 at 01:22:18PM +0200, Heikki Krogerus wrote:
> On Thu, Nov 19, 2020 at 12:09:06AM -0800, Prashant Malani wrote:

> > Hi Utkarsh,

> > 

> > On Wed, Nov 18, 2020 at 10:32:09PM -0800, Utkarsh Patel wrote:

> > > Configure Thunderbolt 3 cable generation value by filling Thunderbolt 3

> > > cable discover mode VDO to support rounded Thunderbolt 3 cables.

> > > While we are here use Thunderbolt 3 cable discover mode VDO to fill active

> > > cable plug link training value.

> > > 

> > > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

> > > 

> > > --

> > > Changes in v3:

> > > - Added a check for cable's TBT support before filling TBT3 discover mode

> > >   VDO.

> > > 

> > > Changes in v2:

> > > - No change.

> > > --

> > > ---

> > >  drivers/platform/chrome/cros_ec_typec.c | 14 ++++++++++++--

> > >  1 file changed, 12 insertions(+), 2 deletions(-)

> > > 

> > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c

> > > index 8111ed1fc574..68b17ee1d1ae 100644

> > > --- a/drivers/platform/chrome/cros_ec_typec.c

> > > +++ b/drivers/platform/chrome/cros_ec_typec.c

> > > @@ -514,8 +514,18 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,

> > >  	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)

> > >  		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;

> > >  

> > > -	data.active_link_training = !!(pd_ctrl->control_flags &

> > > -				       USB_PD_CTRL_ACTIVE_LINK_UNIDIR);

> > > +	/*

> > > +	 * Filling TBT3 Cable VDO when TBT3 cable is being used to establish

> > > +	 * USB4 connection.

> > > +	 */

> > > +	if (pd_ctrl->cable_gen) {

> > > +		data.tbt_cable_vdo = TBT_MODE;

> > > +

> > > +		if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> > > +			data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> > > +

> > > +		data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);

> > > +	}

> > 

> > I think the following would decouple Rounded Support and Active Cable Link

> > Training?:

> > 

> > 	struct typec_thunderbolt_data data = {};

> > ...

> > 	if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> > 		data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> > 

> > 	data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);

> 

> I agree with this. We should not modify the data that we actually

> have access to.

> 

> > 	if (data.tbt_cable_vdo)

> > 		data.tbt_cable_vdo |= TBT_MODE;

> 

> That is wrong. If the LSRX communication is bi-directional, and/or the

> data rates are non-rounded, the cable has to be TBT3 cable. So I think

> what you would want is:


Thanks for pointing this out, I didn't consider this case.

> 

> 	if (!data.tbt_cable_vdo)

>  		data.tbt_cable_vdo = TBT_MODE;

> 

> But of course we can not do that either, because we have to set the

> TBT_MODE bit if there is any other data in tbt_cable_vdo (USB Type-C

> spec does not define any other valid values except 0x0001 = TBT_MODE

> for that field). Otherwise the mux driver should consider the data

> corrupted. So we would have to do this:

> 

>         if (pd_ctrl->cable_gen &&

>             pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

>                 data.tbt_cable_vdo = 0; /* We assume USB4 cable */

>         else

>  		data.tbt_cable_vdo |= TBT_MODE; /* It is for sure TBT3 cable */

> 

> But I would not do that. TBT3 cable can also support unidirectional

> SBU communication and rounded data rates.

> 

> IMO safer bet for now would be to just claim that the cable is always

> TBT3 cable until we have access to information that can really tell us

> is the cable USB4 or TBT3.


Which brings us back to v1 of the patch :S

That still leaves my underlying concern that we'll be telling the Mux
implementation that a TBT3 cable is connected when in fact it's a USB4
active cable.

How about we don't set the TBT_MODE bit at all ? IMO it's equally bad as setting it
always, but with the additional advantage:

- USB4 active cable case : you are covered (since if we unilaterally set
TBT_MODE then the Active USB4 cable case never gets executed in
pmc_usb_mux_usb4() in drivers/usb/typec/mux/intel_pmc_mux.c Patch 3/4)

- Bidirectional LSRX non-rounded TBT: Still supported since
  the code path in the Intel Mux agent is the same.

I understand neither of the options are ideal, but WDYT?

BR,

-Prashant
Prashant Malani Nov. 20, 2020, 1:36 p.m. UTC | #5
On Fri, Nov 20, 2020 at 4:07 AM Prashant Malani <pmalani@chromium.org> wrote:
>

> On Fri, Nov 20, 2020 at 01:22:18PM +0200, Heikki Krogerus wrote:

> > On Thu, Nov 19, 2020 at 12:09:06AM -0800, Prashant Malani wrote:

> > > Hi Utkarsh,

> > >

> > > On Wed, Nov 18, 2020 at 10:32:09PM -0800, Utkarsh Patel wrote:

> > > > Configure Thunderbolt 3 cable generation value by filling Thunderbolt 3

> > > > cable discover mode VDO to support rounded Thunderbolt 3 cables.

> > > > While we are here use Thunderbolt 3 cable discover mode VDO to fill active

> > > > cable plug link training value.

> > > >

> > > > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

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

> > > >

> > > > --

> > > > Changes in v3:

> > > > - Added a check for cable's TBT support before filling TBT3 discover mode

> > > >   VDO.

> > > >

> > > > Changes in v2:

> > > > - No change.

> > > > --

> > > > ---

> > > >  drivers/platform/chrome/cros_ec_typec.c | 14 ++++++++++++--

> > > >  1 file changed, 12 insertions(+), 2 deletions(-)

> > > >

> > > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c

> > > > index 8111ed1fc574..68b17ee1d1ae 100644

> > > > --- a/drivers/platform/chrome/cros_ec_typec.c

> > > > +++ b/drivers/platform/chrome/cros_ec_typec.c

> > > > @@ -514,8 +514,18 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,

> > > >   else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)

> > > >           data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;

> > > >

> > > > - data.active_link_training = !!(pd_ctrl->control_flags &

> > > > -                                USB_PD_CTRL_ACTIVE_LINK_UNIDIR);

> > > > + /*

> > > > +  * Filling TBT3 Cable VDO when TBT3 cable is being used to establish

> > > > +  * USB4 connection.

> > > > +  */

> > > > + if (pd_ctrl->cable_gen) {

> > > > +         data.tbt_cable_vdo = TBT_MODE;

> > > > +

> > > > +         if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> > > > +                 data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> > > > +

> > > > +         data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);

> > > > + }

> > >

> > > I think the following would decouple Rounded Support and Active Cable Link

> > > Training?:

> > >

> > >     struct typec_thunderbolt_data data = {};

> > > ...

> > >     if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> > >             data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;

> > >

> > >     data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);

> >

> > I agree with this. We should not modify the data that we actually

> > have access to.

> >

> > >     if (data.tbt_cable_vdo)

> > >             data.tbt_cable_vdo |= TBT_MODE;

> >

> > That is wrong. If the LSRX communication is bi-directional, and/or the

> > data rates are non-rounded, the cable has to be TBT3 cable. So I think

> > what you would want is:

>

> Thanks for pointing this out, I didn't consider this case.

>

> >

> >       if (!data.tbt_cable_vdo)

> >               data.tbt_cable_vdo = TBT_MODE;

> >

> > But of course we can not do that either, because we have to set the

> > TBT_MODE bit if there is any other data in tbt_cable_vdo (USB Type-C

> > spec does not define any other valid values except 0x0001 = TBT_MODE

> > for that field). Otherwise the mux driver should consider the data

> > corrupted. So we would have to do this:

> >

> >         if (pd_ctrl->cable_gen &&

> >             pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)

> >                 data.tbt_cable_vdo = 0; /* We assume USB4 cable */

> >         else

> >               data.tbt_cable_vdo |= TBT_MODE; /* It is for sure TBT3 cable */

> >

> > But I would not do that. TBT3 cable can also support unidirectional

> > SBU communication and rounded data rates.

> >

> > IMO safer bet for now would be to just claim that the cable is always

> > TBT3 cable until we have access to information that can really tell us

> > is the cable USB4 or TBT3.

>

> Which brings us back to v1 of the patch :S

>

> That still leaves my underlying concern that we'll be telling the Mux

> implementation that a TBT3 cable is connected when in fact it's a USB4

> active cable.

>

> How about we don't set the TBT_MODE bit at all ? IMO it's equally bad as setting it

> always, but with the additional advantage:

>

> - USB4 active cable case : you are covered (since if we unilaterally set

> TBT_MODE then the Active USB4 cable case never gets executed in

> pmc_usb_mux_usb4() in drivers/usb/typec/mux/intel_pmc_mux.c Patch 3/4)

>

> - Bidirectional LSRX non-rounded TBT: Still supported since

>   the code path in the Intel Mux agent is the same.


Thinking about this a bit more, never setting TBT_MODE would mean that
bidrectional LSRX non-rounded TBT would be treated the same as USB4
active cable in the mux agent
code, which is also incorrect (I think)

I need to think about this a bit more, but currently I don't have any
solutions to offer.
Heikki Krogerus Nov. 20, 2020, 1:41 p.m. UTC | #6
Hi Prashant,

On Fri, Nov 20, 2020 at 04:07:00AM -0800, Prashant Malani wrote:
> On Fri, Nov 20, 2020 at 01:22:18PM +0200, Heikki Krogerus wrote:

> > IMO safer bet for now would be to just claim that the cable is always

> > TBT3 cable until we have access to information that can really tell us

> > is the cable USB4 or TBT3.

> 

> Which brings us back to v1 of the patch :S

> 

> That still leaves my underlying concern that we'll be telling the Mux

> implementation that a TBT3 cable is connected when in fact it's a USB4

> active cable.


No matter what we do with these values, it does not change the fact
that we do not know is the cable USB4 or TBT3 at the moment. What ever
we do, we are guessing things.

Can you please explain your concern again? What can happen if we use
USB4 cable but claim that it is TBT3 to the mux?

> How about we don't set the TBT_MODE bit at all ?


Let's not come up with values that are not defined in the spec for
this field (or any other). Even though it is unlikely, we can not rule
out the possibility that one day value of 0x0000 is defined for that
field.

I'm strongly against this kind of "modifications". It would mean that
the member tbt_cable_vdo does not actually contain TBT3 Cable VDO, but
instead it has to be considered as driver or subsystem specific data.

> IMO it's equally bad as setting it always, but with the additional

> advantage:

> 

> - USB4 active cable case : you are covered (since if we unilaterally set

> TBT_MODE then the Active USB4 cable case never gets executed in

> pmc_usb_mux_usb4() in drivers/usb/typec/mux/intel_pmc_mux.c Patch 3/4)

> 

> - Bidirectional LSRX non-rounded TBT: Still supported since

>   the code path in the Intel Mux agent is the same.


This case will not work. The tbt_cable_vdo member is now 0, which to
the mux driver means that the cable is USB4 cable (and that is what it
really must mean to the mux driver - any mux driver). The mux driver
will therefore enable unidirectional SBU communication even though it
is actually bidirectional, and also rounded data rates even though the
cable supports only non-rounded data rates.

> I understand neither of the options are ideal, but WDYT?


I may be missing something, but I'm still going to say that let's just
claim that the cable is always TBT3 for now in this driver. That is
the only safe bet that we have.


thanks,

-- 
heikki
diff mbox series

Patch

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 8111ed1fc574..68b17ee1d1ae 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -514,8 +514,18 @@  static int cros_typec_enable_usb4(struct cros_typec_data *typec,
 	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
 		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;
 
-	data.active_link_training = !!(pd_ctrl->control_flags &
-				       USB_PD_CTRL_ACTIVE_LINK_UNIDIR);
+	/*
+	 * Filling TBT3 Cable VDO when TBT3 cable is being used to establish
+	 * USB4 connection.
+	 */
+	if (pd_ctrl->cable_gen) {
+		data.tbt_cable_vdo = TBT_MODE;
+
+		if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_LINK_UNIDIR)
+			data.tbt_cable_vdo |= TBT_CABLE_LINK_TRAINING;
+
+		data.tbt_cable_vdo |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);
+	}
 
 	port->state.alt = NULL;
 	port->state.data = &data;