| Message ID | 20201113202503.6559-7-utkarsh.h.patel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Thunderbolt3/USB4 cable rounded and active cable plug link training support | expand |
On 13/11/20 21:25, Utkarsh Patel wrote: > Configure Thunderbolt3/USB4 cable generation value by filing Thunderbolt 3 > cable discover mode VDO to support rounded and non-rounded Thunderbolt3/ > USB4 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> Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > -- > 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..b7416e82c3b3 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); > + /* > + * This driver does not have access to the identity information or > + * capabilities of the cable, so we don't know is it a real USB4 or > + * TBT3 cable. Therefore pretending that it's always TBT3 cable by > + * filling the TBT3 Cable VDO. > + */ > + 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; >
On Fri, Nov 13, 2020 at 12:25:01PM -0800, Utkarsh Patel wrote: > Configure Thunderbolt3/USB4 cable generation value by filing Thunderbolt 3 > cable discover mode VDO to support rounded and non-rounded Thunderbolt3/ > USB4 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> FWIW: Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > -- > 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..b7416e82c3b3 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); > + /* > + * This driver does not have access to the identity information or > + * capabilities of the cable, so we don't know is it a real USB4 or > + * TBT3 cable. Therefore pretending that it's always TBT3 cable by > + * filling the TBT3 Cable VDO. > + */ > + 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; > -- > 2.17.1
Hi Utkarsh, On Fri, Nov 13, 2020 at 12:25:01PM -0800, Utkarsh Patel wrote: > Configure Thunderbolt3/USB4 cable generation value by filing Thunderbolt 3 > cable discover mode VDO to support rounded and non-rounded Thunderbolt3/ > USB4 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 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..b7416e82c3b3 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); > + /* > + * This driver does not have access to the identity information or > + * capabilities of the cable, so we don't know is it a real USB4 or > + * TBT3 cable. Therefore pretending that it's always TBT3 cable by > + * filling the TBT3 Cable VDO. > + */ > + data.tbt_cable_vdo = TBT_MODE; Is it safe to be making this assumption unconditionally? It might work for Intel Mux agent but is it guaranteed to be safe for any other future mux implementation? In other words, what if a "true" USB4 cable is connected which doesn't have the Thunderbolt SVID alt mode? (Pre-fetching some alternatives in case the answer is no) You might want to check with the Cros EC team if you can repurpose a bit of the "reserved" field for specifying whether the cable is TBT or not. Either that or see if there is a way to determine from the pd_ctrl->cable_speed whether the cable is actually TBT or not. Failing all the above, perhaps you'll have to wait for the PD discovery stuff to make it's way through review and use that (note that there may be timing issues between the Mux update event and PD discovery complete event reaching the port driver). Best regards, -Prashant
On Tue, Nov 17, 2020 at 10:19 AM Prashant Malani <pmalani@chromium.org> wrote: > > Hi Utkarsh, > > On Fri, Nov 13, 2020 at 12:25:01PM -0800, Utkarsh Patel wrote: > > Configure Thunderbolt3/USB4 cable generation value by filing Thunderbolt 3 > > cable discover mode VDO to support rounded and non-rounded Thunderbolt3/ > > USB4 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 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..b7416e82c3b3 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); > > + /* > > + * This driver does not have access to the identity information or > > + * capabilities of the cable, so we don't know is it a real USB4 or > > + * TBT3 cable. Therefore pretending that it's always TBT3 cable by > > + * filling the TBT3 Cable VDO. > > + */ > > + data.tbt_cable_vdo = TBT_MODE; > > Is it safe to be making this assumption unconditionally? It might work for > Intel Mux agent but is it guaranteed to be safe for any other future > mux implementation? In other words, what if a "true" USB4 cable is > connected which doesn't have the Thunderbolt SVID alt mode? I dug into this a bit more and can maybe articulate my concern better: Is there a situation where both of the following are true ? : - Cable type = EUDO_CABLE_TYPE_OPTICAL or EUDO_CABLE_TYPE_RE_TIMER - No TBT_CABLE_LINK_TRAINING or TBT_CABLE_ROUNDED_SUPPORT defined (both these are 0). If both the above are true, then in Patch 7/8, wouldn't we never hit the else condition (labeled "Active USB cable") and therefore not set the mode_data correctly? > > (Pre-fetching some alternatives in case the answer is no) > > You might want to check with the Cros EC team if you can repurpose a bit of > the "reserved" field for specifying whether the cable is TBT or not. > > Either that or see if there is a way to determine from the pd_ctrl->cable_speed > whether the cable is actually TBT or not. It seems link cable_gen and USB_PD_CTRL_ACTIVE_LINK_UNIDIR are reasonable proxies for whether the cable has TBT support, so perhaps we should only set tbt_cable_vdo = TBT_MODE if either of those are non-zero? WDYT? Best regards, -Prashant
Hi Prashant, Thank you for the review and feedback. > On Tue, Nov 17, 2020 at 10:19 AM Prashant Malani <pmalani@chromium.org> > wrote: > > > > Hi Utkarsh, > > > > On Fri, Nov 13, 2020 at 12:25:01PM -0800, Utkarsh Patel wrote: > > > Configure Thunderbolt3/USB4 cable generation value by filing > > > Thunderbolt 3 cable discover mode VDO to support rounded and > > > non-rounded Thunderbolt3/ > > > USB4 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 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..b7416e82c3b3 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); > > > + /* > > > + * This driver does not have access to the identity information or > > > + * capabilities of the cable, so we don't know is it a real USB4 or > > > + * TBT3 cable. Therefore pretending that it's always TBT3 cable by > > > + * filling the TBT3 Cable VDO. > > > + */ > > > + data.tbt_cable_vdo = TBT_MODE; > > > > Is it safe to be making this assumption unconditionally? It might work > > for Intel Mux agent but is it guaranteed to be safe for any other > > future mux implementation? In other words, what if a "true" USB4 cable > > is connected which doesn't have the Thunderbolt SVID alt mode? > > I dug into this a bit more and can maybe articulate my concern better: > > Is there a situation where both of the following are true ? : > - Cable type = EUDO_CABLE_TYPE_OPTICAL or EUDO_CABLE_TYPE_RE_TIMER > - No TBT_CABLE_LINK_TRAINING or TBT_CABLE_ROUNDED_SUPPORT defined > (both > these are 0). No, not in the case of USB4. > > If both the above are true, then in Patch 7/8, wouldn't we never hit the else > condition (labeled "Active USB cable") and therefore not set the mode_data > correctly? > > > > > (Pre-fetching some alternatives in case the answer is no) > > > > You might want to check with the Cros EC team if you can repurpose a > > bit of the "reserved" field for specifying whether the cable is TBT or not. > > > > Either that or see if there is a way to determine from the > > pd_ctrl->cable_speed whether the cable is actually TBT or not. > > It seems link cable_gen and USB_PD_CTRL_ACTIVE_LINK_UNIDIR are > reasonable proxies for whether the cable has TBT support, so perhaps we > should only set tbt_cable_vdo = TBT_MODE if either of those are non-zero? > > WDYT? Since we do not have these information available with USB4 cables, we can use them to check for TBT support and then set tbt_cable_vdo. > > Best regards, > > -Prashant Sincerely, Utkarsh Patel.
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index 8111ed1fc574..b7416e82c3b3 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); + /* + * This driver does not have access to the identity information or + * capabilities of the cable, so we don't know is it a real USB4 or + * TBT3 cable. Therefore pretending that it's always TBT3 cable by + * filling the TBT3 Cable VDO. + */ + 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;
Configure Thunderbolt3/USB4 cable generation value by filing Thunderbolt 3 cable discover mode VDO to support rounded and non-rounded Thunderbolt3/ USB4 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 v2: - No change. -- --- drivers/platform/chrome/cros_ec_typec.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)