Message-ID: <CANFp7mVexEta5fecZ5LaM_Pr6xMigTcuDySz1+65BL_-cY2SOA@mail.gmail.com>
Date: Thu, 7 Nov 2024 11:21:54 -0800
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: tzungbi@...nel.org, linux-usb@...r.kernel.org, 
	chrome-platform@...ts.linux.dev, dmitry.baryshkov@...aro.org, 
	jthies@...gle.com, akuchynski@...gle.com, pmalani@...omium.org, 
	Benson Leung <bleung@...omium.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Guenter Roeck <groeck@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode

On Mon, Nov 4, 2024 at 6:32 AM Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
>
> 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@...ux.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@...ux.intel.com> wrote:
> > > > >
> > > > > Hi Abhishek,
> > > > >
> > > > > On Wed, Oct 30, 2024 at 02:28:32PM -0700, Abhishek Pandit-Subedi wrote:
> > > > > > From: Heikki Krogerus <heikki.krogerus@...ux.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@...ux.intel.com>
> > > > > > Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.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.

Opting not to do this rename for this series -- I tried and it was
touching a lot of drivers so I want to do it in a separate series.

TBT already defined the mode index as |TBT_MODE| in <typec_tbt.h> so I
opted to use that for now.

>
> > |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@...ux.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

Powered by blists - more mailing lists