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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@xxxxxxxxx>
> > >
> > > --
> > > 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
