Re: [PATCH v2 1/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Fri, Nov 01, 2024 at 11:48:07AM -0700, Abhishek Pandit-Subedi wrote:
> On Fri, Nov 1, 2024 at 6:16 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Thu, Oct 31, 2024 at 04:02:22PM -0700, Abhishek Pandit-Subedi wrote:
> > > On Thu, Oct 31, 2024 at 7:11 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > Hi Abhishek,
> > > >
> > > > On Wed, Oct 30, 2024 at 02:28:32PM -0700, Abhishek Pandit-Subedi wrote:
> > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > >
> > > > > Thunderbolt 3 Alternate Mode entry flow is described in
> > > > > USB Type-C Specification Release 2.0.
> > > > >
> > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > ---
> > > > >
> > > > > Changes:
> > > > > * Delay cable + plug checks so that the module doesn't fail to probe
> > > > >   if cable + plug information isn't available by the time the partner
> > > > >   altmode is registered.
> > > > > * Remove unncessary brace after if (IS_ERR(plug))
> > > > >
> > > > > The rest of this patch should be the same as Heikki's original RFC.
> > > > >
> > > > >
> > > > > Changes in v2:
> > > > > - Use <linux/usb/typec_tbt.h> and add missing TBT_CABLE_ROUNDED
> > > > > - Pass struct typec_thunderbolt_data to typec_altmode_notify
> > > > > - Rename TYPEC_TBT_MODE to USB_TYPEC_TBT_MODE
> > > > > - Use USB_TYPEC_TBT_SID and USB_TYPEC_TBT_MODE for device id
> > > > > - Change module license to GPL due to checkpatch warning
> > > > >
> > > > >  drivers/platform/chrome/cros_ec_typec.c  |   2 +-
> > > > >  drivers/usb/typec/altmodes/Kconfig       |   9 +
> > > > >  drivers/usb/typec/altmodes/Makefile      |   2 +
> > > > >  drivers/usb/typec/altmodes/thunderbolt.c | 308 +++++++++++++++++++++++
> > > > >  include/linux/usb/typec_tbt.h            |   3 +-
> > > > >  5 files changed, 322 insertions(+), 2 deletions(-)
> > > > >  create mode 100644 drivers/usb/typec/altmodes/thunderbolt.c
> > > > >
> > > > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > > > > index c7781aea0b88..53d93baa36a8 100644
> > > > > --- a/drivers/platform/chrome/cros_ec_typec.c
> > > > > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > > > > @@ -499,7 +499,7 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
> > > > >       }
> > > > >
> > > > >       port->state.data = &data;
> > > > > -     port->state.mode = TYPEC_TBT_MODE;
> > > > > +     port->state.mode = USB_TYPEC_TBT_MODE;
> > > > >
> > > > >       return typec_mux_set(port->mux, &port->state);
> > > > >  }
> > > >
> > > > The definition should be changed in a separate patch.
> > >
> > > Ack -- will pull the rename out into its own patch.
> > >
> > > >
> > > > > +static const struct typec_device_id tbt_typec_id[] = {
> > > > > +     { USB_TYPEC_TBT_SID, USB_TYPEC_TBT_MODE },
> > > > > +     { }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(typec, tbt_typec_id);
> > > >
> > > > Now the mode would be the same thing as connector state, which is not
> > > > true. The connector state is supposed to reflect the pin assignment,
> > > > and the mode is the mode index used with the actual VDMs. For example,
> > > > DP alt mode has several different states, but only one mode.
> > > >
> > > > The TBT3 altmode driver will not work with this patch alone, it will
> > > > never bind to the partner TBT3 alt mode because the mode does not
> > > > match.
> > > >
> > > > Can you reorganise this series so that the patch 2/7 comes before this
> > > > one? Then I think you can just use the SVID unless I'm mistaken:
> > > >
> > > >         static const struct typec_device_id tbt_typec_id[] = {
> > > >                 { USB_TYPEC_TBT_SID },
> > > >                 { }
> > > >         };
> > > >         MODULE_DEVICE_TABLE(typec, tbt_typec_id);
> > > >
> > > > Alternatively, just leave it to TYPEC_ANY_MODE for now.
> > > >
> > >
> > > Sure, I'll re-order the patches and get rid of the mode. I'm actually
> > > a bit confused as to how mode is supposed to be used since typec_dp.h
> > > defines USB_TYPEC_DP_MODE=1, typec_tbt.h defines
> > > USB_TYPEC_TBT_MODE=TYPEC_STATE_MODAL and it looks like USB state also
> > > starts from TYPEC_STATE_MODAL and continues.
> > >
> > > Is this documented in the spec somewhere? How should this mode value
> > > be used and shared between USB and various alt-modes? At least the DP
> > > case seems clear because as you said it describes different pin
> > > assignments. However, the term "mode" seems to be overloaded since
> > > it's used in other areas.
> >
> > Well, this is confusing, I admit. One problem is that the term "mode"
> > really means different things depending on the spec. In DP alt mode
> > specification for example, "mode" basically means the same as pin
> > assignment, so not the object position like it does in USB PD and
> > Type-C specifications.
> >
> > But the alt modes are in any case meant to be differentiated from the
> > common USB and accessory modes simply by checking if there is struct
> > altmode or not.
> >
> > So the mux drivers for example can use the "alt" member in struct
> > typec_mux_state to check is the connector meant to enter alt mode, or
> > USB or accessory mode.
> >
> > I.e. if the "alt" member is there, then it's alt mode, and the "mode"
> > member value matches whatever is defined for that specific alt mode.
> >
> > If "alt" is NULL, then connector is in USB mode or accessory mode, and
> > the "mode" member is one of the common modes:
> >
> > enum {
> >         TYPEC_MODE_USB2 = TYPEC_STATE_MODAL,    /* USB 2.0 mode */
> >         TYPEC_MODE_USB3,                        /* USB 3.2 mode */
> >         TYPEC_MODE_USB4,                        /* USB4 mode */
> >         TYPEC_MODE_AUDIO,                       /* Audio Accessory */
> >         TYPEC_MODE_DEBUG,                       /* Debug Accessory */
> > }
> >
> > I hope this answers your question. Maybe this needs to be clarified in
> > this document:
> > https://docs.kernel.org/driver-api/usb/typec.html#multiplexer-demultiplexer-switches
> >
> > ..and the code obviously. Maybe the "mode" member struct
> > typec_mux_state should be renamed to "state"? Though, I'm not sure
> > that improves the situation.
> >
> 
> This does make things clearer -- thank you. Based on the various
> meanings of mode vs state, I think the following will make things
> clearer:
> 
> Let's change |mode| to |mode_index| in `struct typec_altmode_desc`.
> Looking at the Discover SVIDs and Discover Modes response in PD 3.2
> spec, the value we are passing here is actually the mode_index since
> that's what's necessary in the VDM to identify which mode we are
> trying to enter.

Yes, mode_index sounds better.

> |USB_TYPEC_DP_MODE| needs to change to |USB_TYPEC_DP_MODE_INDEX| in typec_dp.h
> |USB_TYPEC_TBT_MODE| should also be |USB_TYPEC_TBT_MODE_INDEX| with a
> value of 1 and we should define a new |TYPEC_TBT_STATE| as an enum
> with base value of TYPEC_STATE_MODAL.
> 
> Getting rid of the mode index for altmode matching makes sense for DP
> and TBT (since both have spec defined standard values) but for
> non-standard modes which might return >1 modes in Discover Modes the
> driver will match for all modes and not just the specific mode like it
> was prior to patch 2 in this series. Do we want to retain that and
> change the TBT driver to only match on mode_index = 1 instead. I have
> no examples of non-standard mode behavior to decide which is the
> better option here.

Let's drop it for now. We can always add it back.

thanks,

> > > > > +static struct typec_altmode_driver tbt_altmode_driver = {
> > > > > +     .id_table = tbt_typec_id,
> > > > > +     .probe = tbt_altmode_probe,
> > > > > +     .remove = tbt_altmode_remove,
> > > > > +     .driver = {
> > > > > +             .name = "typec-thunderbolt",
> > > > > +             .owner = THIS_MODULE,
> > > > > +     }
> > > > > +};
> > > > > +module_typec_altmode_driver(tbt_altmode_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
> > > > > +MODULE_LICENSE("GPL");
> > > > > +MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
> > > > > diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
> > > > > index fa97d7e00f5c..3ff82641f6a0 100644
> > > > > --- a/include/linux/usb/typec_tbt.h
> > > > > +++ b/include/linux/usb/typec_tbt.h
> > > > > @@ -10,7 +10,7 @@
> > > > >  #define USB_TYPEC_TBT_SID            USB_TYPEC_VENDOR_INTEL
> > > > >
> > > > >  /* Connector state for Thunderbolt3 */
> > > > > -#define TYPEC_TBT_MODE                       TYPEC_STATE_MODAL
> > > > > +#define USB_TYPEC_TBT_MODE           TYPEC_STATE_MODAL
> > > >
> > > > I think USB_TYPEC_STATE_TBT would be better. But please change this in
> > > > a separate patch in any case.
> > >
> > > Same question as above about mode vs state :)
> >
> > Well, I was thinking that maybe we should use the term "state" here
> > with the idea that "state" would be something purely kernel specific,
> > and "mode" would then be something defined in a specification... But
> > now I'm not so sure (I don't think it's always clear).
> >
> > Maybe USB_TYPEC_TBT_MODE after all. I'll leave the decision to you.
> >
> > cheers,
> >
> > --
> > heikki

-- 
heikki