diff mbox series

[v3,1/4] usb: typec: Use Thunderbolt 3 cable discover mode VDO in Enter_USB message

Message ID 20201119063211.2264-2-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

Commit Message

Patel, Utkarsh H Nov. 19, 2020, 6:32 a.m. UTC
When Thunderbolt 3 cable is being used to create USB4 connection, use
Thunderbolt 3 discover mode VDO to fill details such as active cable plug
link training and cable rounded support.
With USB4 cables, these VDO members need not be filled.

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

--
Changes in v3:
- Changed the commit mesage to reflect why TBT3 VDO is being used.
- Added more details in the header file about the usage of TBT3 VDO.

Changes in v2:
- No change.
--
---
 include/linux/usb/typec.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Heikki Krogerus Nov. 20, 2020, 8:05 a.m. UTC | #1
On Wed, Nov 18, 2020 at 10:32:08PM -0800, Utkarsh Patel wrote:
> When Thunderbolt 3 cable is being used to create USB4 connection, use
> Thunderbolt 3 discover mode VDO to fill details such as active cable plug
> link training and cable rounded support.
> With USB4 cables, these VDO members need not be filled.
> 
> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> 
> --
> Changes in v3:
> - Changed the commit mesage to reflect why TBT3 VDO is being used.
> - Added more details in the header file about the usage of TBT3 VDO.
> 
> Changes in v2:
> - No change.
> --
> ---
>  include/linux/usb/typec.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 6be558045942..25731ed863fa 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -75,6 +75,10 @@ enum typec_orientation {
>  /*
>   * struct enter_usb_data - Enter_USB Message details
>   * @eudo: Enter_USB Data Object
> + * @tbt_cable_vdo: TBT3 Cable Discover Mode Response

This is fine..

> + * @tbt_cable_vdo needs to be filled with details of active cable plug link
> + * training and cable rounded support when thunderbolt 3 cable is being used to
> + * create USB4 connection. Do not fill this in case of USB4 cable.

But this is not. The description of the member tells what the member
contains, but it does not make sense to explain also how to use the
member in the same place. Instead you should explain how to use the
member in the description of the structure. So..

>   * @active_link_training: Active Cable Plug Link Training
>   *
>   * @active_link_training is a flag that should be set with uni-directional SBRX

Put it here. That will make this much more readable.


thanks,
Prashant Malani Nov. 20, 2020, 8:36 a.m. UTC | #2
On Fri, Nov 20, 2020 at 10:05:14AM +0200, Heikki Krogerus wrote:
> On Wed, Nov 18, 2020 at 10:32:08PM -0800, Utkarsh Patel wrote:
> > When Thunderbolt 3 cable is being used to create USB4 connection, use
> > Thunderbolt 3 discover mode VDO to fill details such as active cable plug
> > link training and cable rounded support.
> > With USB4 cables, these VDO members need not be filled.
> > 
> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> > 
> > --
> > Changes in v3:
> > - Changed the commit mesage to reflect why TBT3 VDO is being used.
> > - Added more details in the header file about the usage of TBT3 VDO.
> > 
> > Changes in v2:
> > - No change.
> > --
> > ---
> >  include/linux/usb/typec.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > index 6be558045942..25731ed863fa 100644
> > --- a/include/linux/usb/typec.h
> > +++ b/include/linux/usb/typec.h
> > @@ -75,6 +75,10 @@ enum typec_orientation {
> >  /*
> >   * struct enter_usb_data - Enter_USB Message details
> >   * @eudo: Enter_USB Data Object
> > + * @tbt_cable_vdo: TBT3 Cable Discover Mode Response
> 
> This is fine..
> 
> > + * @tbt_cable_vdo needs to be filled with details of active cable plug link
> > + * training and cable rounded support when thunderbolt 3 cable is being used to
> > + * create USB4 connection. Do not fill this in case of USB4 cable.
> 
> But this is not. The description of the member tells what the member
> contains, but it does not make sense to explain also how to use the
> member in the same place.

Slightly tangential question here:

Is there a need to mention "active cable plug link training" and "cable
rounded support" at all? Wouldn't it be sufficient to omit those in the
description (in case some mux implementation wants to use the other fields
of the VDO) ?

> Instead you should explain how to use the
> member in the description of the structure. So..
> 
> >   * @active_link_training: Active Cable Plug Link Training
> >   *
> >   * @active_link_training is a flag that should be set with uni-directional SBRX
> 
> Put it here. That will make this much more readable.
> 
> 
> thanks,
> 
> -- 
> heikki
Heikki Krogerus Nov. 20, 2020, 8:52 a.m. UTC | #3
On Fri, Nov 20, 2020 at 12:36:25AM -0800, Prashant Malani wrote:
> On Fri, Nov 20, 2020 at 10:05:14AM +0200, Heikki Krogerus wrote:
> > On Wed, Nov 18, 2020 at 10:32:08PM -0800, Utkarsh Patel wrote:
> > > When Thunderbolt 3 cable is being used to create USB4 connection, use
> > > Thunderbolt 3 discover mode VDO to fill details such as active cable plug
> > > link training and cable rounded support.
> > > With USB4 cables, these VDO members need not be filled.
> > > 
> > > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> > > 
> > > --
> > > Changes in v3:
> > > - Changed the commit mesage to reflect why TBT3 VDO is being used.
> > > - Added more details in the header file about the usage of TBT3 VDO.
> > > 
> > > Changes in v2:
> > > - No change.
> > > --
> > > ---
> > >  include/linux/usb/typec.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > > index 6be558045942..25731ed863fa 100644
> > > --- a/include/linux/usb/typec.h
> > > +++ b/include/linux/usb/typec.h
> > > @@ -75,6 +75,10 @@ enum typec_orientation {
> > >  /*
> > >   * struct enter_usb_data - Enter_USB Message details
> > >   * @eudo: Enter_USB Data Object
> > > + * @tbt_cable_vdo: TBT3 Cable Discover Mode Response
> > 
> > This is fine..
> > 
> > > + * @tbt_cable_vdo needs to be filled with details of active cable plug link
> > > + * training and cable rounded support when thunderbolt 3 cable is being used to
> > > + * create USB4 connection. Do not fill this in case of USB4 cable.
> > 
> > But this is not. The description of the member tells what the member
> > contains, but it does not make sense to explain also how to use the
> > member in the same place.
> 
> Slightly tangential question here:
> 
> Is there a need to mention "active cable plug link training" and "cable
> rounded support" at all? Wouldn't it be sufficient to omit those in the
> description (in case some mux implementation wants to use the other fields
> of the VDO) ?

No, I don't think so. I think it would be enough to just mention that
we need the TBT3 Cable VDO only when the USB4 connection is created
using TBT3 cables. And that's it.


Br,
Patel, Utkarsh H Nov. 20, 2020, 5:04 p.m. UTC | #4
Hi Heikki and Prashant,

Thank you for the feedback. 

> > > >  include/linux/usb/typec.h | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > > > index 6be558045942..25731ed863fa 100644
> > > > --- a/include/linux/usb/typec.h
> > > > +++ b/include/linux/usb/typec.h
> > > > @@ -75,6 +75,10 @@ enum typec_orientation {
> > > >  /*
> > > >   * struct enter_usb_data - Enter_USB Message details
> > > >   * @eudo: Enter_USB Data Object
> > > > + * @tbt_cable_vdo: TBT3 Cable Discover Mode Response
> > >
> > > This is fine..
> > >
> > > > + * @tbt_cable_vdo needs to be filled with details of active cable
> > > > + plug link
> > > > + * training and cable rounded support when thunderbolt 3 cable is
> > > > + being used to
> > > > + * create USB4 connection. Do not fill this in case of USB4 cable.
> > >
> > > But this is not. The description of the member tells what the member
> > > contains, but it does not make sense to explain also how to use the
> > > member in the same place.
> >
> > Slightly tangential question here:
> >
> > Is there a need to mention "active cable plug link training" and
> > "cable rounded support" at all? Wouldn't it be sufficient to omit
> > those in the description (in case some mux implementation wants to use
> > the other fields of the VDO) ?
> 
> No, I don't think so. I think it would be enough to just mention that we need
> the TBT3 Cable VDO only when the USB4 connection is created using TBT3
> cables. And that's it.

Ack.

> 
> 
> Br,
> 
> --
> Heikki

Sincerely,
Utkarsh Patel.
diff mbox series

Patch

diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 6be558045942..25731ed863fa 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -75,6 +75,10 @@  enum typec_orientation {
 /*
  * struct enter_usb_data - Enter_USB Message details
  * @eudo: Enter_USB Data Object
+ * @tbt_cable_vdo: TBT3 Cable Discover Mode Response
+ * @tbt_cable_vdo needs to be filled with details of active cable plug link
+ * training and cable rounded support when thunderbolt 3 cable is being used to
+ * create USB4 connection. Do not fill this in case of USB4 cable.
  * @active_link_training: Active Cable Plug Link Training
  *
  * @active_link_training is a flag that should be set with uni-directional SBRX
@@ -83,6 +87,7 @@  enum typec_orientation {
  */
 struct enter_usb_data {
 	u32			eudo;
+	u32			tbt_cable_vdo;
 	unsigned char		active_link_training:1;
 };