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);
}
diff --git a/drivers/usb/typec/altmodes/Kconfig b/drivers/usb/typec/altmodes/Kconfig
index 1a6b5e872b0d..7867fa7c405d 100644
--- a/drivers/usb/typec/altmodes/Kconfig
+++ b/drivers/usb/typec/altmodes/Kconfig
@@ -23,4 +23,13 @@ config TYPEC_NVIDIA_ALTMODE
To compile this driver as a module, choose M here: the
module will be called typec_nvidia.
+config TYPEC_TBT_ALTMODE
+ tristate "Thunderbolt3 Alternate Mode driver"
+ help
+ Select this option if you have Thunderbolt3 hardware on your
+ system.
+
+ To compile this driver as a module, choose M here: the
+ module will be called typec_thunderbolt.
+
endmenu
diff --git a/drivers/usb/typec/altmodes/Makefile b/drivers/usb/typec/altmodes/Makefile
index 45717548b396..508a68351bd2 100644
--- a/drivers/usb/typec/altmodes/Makefile
+++ b/drivers/usb/typec/altmodes/Makefile
@@ -4,3 +4,5 @@ obj-$(CONFIG_TYPEC_DP_ALTMODE) += typec_displayport.o
typec_displayport-y := displayport.o
obj-$(CONFIG_TYPEC_NVIDIA_ALTMODE) += typec_nvidia.o
typec_nvidia-y := nvidia.o
+obj-$(CONFIG_TYPEC_TBT_ALTMODE) += typec_thunderbolt.o
+typec_thunderbolt-y := thunderbolt.o
diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
new file mode 100644
index 000000000000..8380b22d26a7
--- /dev/null
+++ b/drivers/usb/typec/altmodes/thunderbolt.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * USB Typec-C Thuderbolt3 Alternate Mode driver
+ *
+ * Copyright (C) 2019 Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/usb/pd_vdo.h>
+#include <linux/usb/typec_altmode.h>
+#include <linux/usb/typec_tbt.h>
+
+enum tbt_state {
+ TBT_STATE_IDLE,
+ TBT_STATE_SOP_P_ENTER,
+ TBT_STATE_SOP_PP_ENTER,
+ TBT_STATE_ENTER,
+ TBT_STATE_EXIT,
+ TBT_STATE_SOP_PP_EXIT,
+ TBT_STATE_SOP_P_EXIT
+};
+
+struct tbt_altmode {
+ enum tbt_state state;
+ struct typec_cable *cable;
+ struct typec_altmode *alt;
+ struct typec_altmode *plug[2];
+ u32 enter_vdo;
+
+ struct work_struct work;
+ struct mutex lock; /* device lock */
+};
+
+static bool tbt_ready(struct typec_altmode *alt);
+
+static int tbt_enter_mode(struct tbt_altmode *tbt)
+{
+ struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
+ u32 vdo;
+
+ vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
+ vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
+ vdo |= TBT_MODE;
+
+ if (plug) {
+ if (typec_cable_is_active(tbt->cable))
+ vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
+
+ vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
+ vdo |= plug->vdo & TBT_CABLE_ROUNDED;
+ vdo |= plug->vdo & TBT_CABLE_OPTICAL;
+ vdo |= plug->vdo & TBT_CABLE_RETIMER;
+ vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
+ } else {
+ vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
+ }
+
+ tbt->enter_vdo = vdo;
+ return typec_altmode_enter(tbt->alt, &vdo);
+}
+
+static void tbt_altmode_work(struct work_struct *work)
+{
+ struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
+ int ret;
+
+ mutex_lock(&tbt->lock);
+
+ switch (tbt->state) {
+ case TBT_STATE_SOP_P_ENTER:
+ ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
+ if (ret)
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
+ "failed to enter mode (%d)\n", ret);
+ break;
+ case TBT_STATE_SOP_PP_ENTER:
+ ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
+ if (ret)
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
+ "failed to enter mode (%d)\n", ret);
+ break;
+ case TBT_STATE_ENTER:
+ ret = tbt_enter_mode(tbt);
+ if (ret)
+ dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
+ ret);
+ break;
+ case TBT_STATE_EXIT:
+ typec_altmode_exit(tbt->alt);
+ break;
+ case TBT_STATE_SOP_PP_EXIT:
+ typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
+ break;
+ case TBT_STATE_SOP_P_EXIT:
+ typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
+ break;
+ default:
+ break;
+ }
+
+ tbt->state = TBT_STATE_IDLE;
+
+ mutex_unlock(&tbt->lock);
+}
+
+static int tbt_altmode_vdm(struct typec_altmode *alt,
+ const u32 hdr, const u32 *vdo, int count)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ int cmd_type = PD_VDO_CMDT(hdr);
+ int cmd = PD_VDO_CMD(hdr);
+
+ mutex_lock(&tbt->lock);
+
+ if (tbt->state != TBT_STATE_IDLE) {
+ mutex_unlock(&tbt->lock);
+ return -EBUSY;
+ }
+
+ switch (cmd_type) {
+ case CMDT_RSP_ACK:
+ switch (cmd) {
+ case CMD_ENTER_MODE:
+ /*
+ * Following the order describeded in USB Type-C Spec
+ * R2.0 Section 6.7.3.
+ */
+ if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_ENTER;
+ else
+ tbt->state = TBT_STATE_ENTER;
+ } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
+ tbt->state = TBT_STATE_ENTER;
+ } else {
+ struct typec_thunderbolt_data data;
+
+ data.device_mode = tbt->alt->vdo;
+ data.cable_mode =
+ tbt->plug[TYPEC_PLUG_SOP_P] ?
+ tbt->plug[TYPEC_PLUG_SOP_P]
+ ->vdo :
+ 0;
+ data.enter_vdo = tbt->enter_vdo;
+
+ typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
+ }
+ break;
+ case CMD_EXIT_MODE:
+ if (alt == tbt->alt) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_EXIT;
+ else if (tbt->plug[TYPEC_PLUG_SOP_P])
+ tbt->state = TBT_STATE_SOP_P_EXIT;
+ } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
+ tbt->state = TBT_STATE_SOP_P_EXIT;
+ }
+ break;
+ }
+ break;
+ case CMDT_RSP_NAK:
+ switch (cmd) {
+ case CMD_ENTER_MODE:
+ dev_warn(&alt->dev, "Enter Mode refused\n");
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+
+ if (tbt->state != TBT_STATE_IDLE)
+ schedule_work(&tbt->work);
+
+ mutex_unlock(&tbt->lock);
+
+ return 0;
+}
+
+static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ int ret;
+
+ mutex_lock(&tbt->lock);
+
+ if (!tbt_ready(alt))
+ return -ENODEV;
+
+ /* Preventing the user space from entering/exiting the cable alt mode */
+ if (alt != tbt->alt)
+ ret = -EPERM;
+ else if (activate)
+ ret = tbt_enter_mode(tbt);
+ else
+ ret = typec_altmode_exit(alt);
+
+ mutex_unlock(&tbt->lock);
+
+ return ret;
+}
+
+static const struct typec_altmode_ops tbt_altmode_ops = {
+ .vdm = tbt_altmode_vdm,
+ .activate = tbt_altmode_activate
+};
+
+static int tbt_altmode_probe(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt;
+
+ tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
+ if (!tbt)
+ return -ENOMEM;
+
+ INIT_WORK(&tbt->work, tbt_altmode_work);
+ mutex_init(&tbt->lock);
+ tbt->alt = alt;
+
+ alt->desc = "Thunderbolt3";
+ typec_altmode_set_drvdata(alt, tbt);
+ typec_altmode_set_ops(alt, &tbt_altmode_ops);
+
+ if (tbt_ready(alt)) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_ENTER;
+ else if (tbt->plug[TYPEC_PLUG_SOP_P])
+ tbt->state = TBT_STATE_SOP_P_ENTER;
+ else
+ tbt->state = TBT_STATE_ENTER;
+ schedule_work(&tbt->work);
+ }
+
+ return 0;
+}
+
+static void tbt_altmode_remove(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+
+ for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
+ if (tbt->plug[i])
+ typec_altmode_put_plug(tbt->plug[i]);
+ }
+
+ if (tbt->cable)
+ typec_cable_put(tbt->cable);
+}
+
+static bool tbt_ready(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ struct typec_altmode *plug;
+
+ if (tbt->cable)
+ return true;
+
+ /* Thundebolt 3 requires a cable with eMarker */
+ tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
+ if (!tbt->cable)
+ return false;
+
+ /* We accept systems without SOP' or SOP''. This means the port altmode
+ * driver will be responsible for properly ordering entry/exit.
+ */
+ for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
+ plug = typec_altmode_get_plug(tbt->alt, i);
+ if (IS_ERR(plug))
+ continue;
+
+ if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
+ break;
+
+ plug->desc = "Thunderbolt3";
+ plug->ops = &tbt_altmode_ops;
+ typec_altmode_set_drvdata(plug, tbt);
+
+ tbt->plug[i] = plug;
+ }
+
+ return true;
+}
+
+static const struct typec_device_id tbt_typec_id[] = {
+ { USB_TYPEC_TBT_SID, USB_TYPEC_TBT_MODE },
+ { }
+};
+MODULE_DEVICE_TABLE(typec, tbt_typec_id);
+
+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
/**
* struct typec_thunderbolt_data - Thundebolt3 Alt Mode specific data
@@ -44,6 +44,7 @@ struct typec_thunderbolt_data {
#define TBT_GEN3_NON_ROUNDED 0
#define TBT_GEN3_GEN4_ROUNDED_NON_ROUNDED 1
+#define TBT_CABLE_ROUNDED BIT(19)
#define TBT_CABLE_OPTICAL BIT(21)
#define TBT_CABLE_RETIMER BIT(22)
#define TBT_CABLE_LINK_TRAINING BIT(23)
--
2.47.0.163.g1226f6d8fa-googHi 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.
> +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.
> +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.
thanks,
--
heikki
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.
> > +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 :)
>
> thanks,
>
> --
> heikki
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.
> > > +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
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.
|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.
> > > > +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
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
On Mon, Nov 4, 2024 at 6:32 AM Heikki Krogerus
<heikki.krogerus@linux.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@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.
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@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
Hi Abhishek,
kernel test robot noticed the following build warnings:
[auto build test WARNING on chrome-platform/for-next]
[also build test WARNING on chrome-platform/for-firmware-next usb/usb-testing usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.12-rc5 next-20241030]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Abhishek-Pandit-Subedi/usb-typec-Add-driver-for-Thunderbolt-3-Alternate-Mode/20241031-053304
base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link: https://lore.kernel.org/r/20241030142833.v2.1.I3080b036e8de0b9957c57c1c3059db7149c5e549%40changeid
patch subject: [PATCH v2 1/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241031/202410311250.ALKPLCqo-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241031/202410311250.ALKPLCqo-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410311250.ALKPLCqo-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/usb/typec/altmodes/thunderbolt.c:16: warning: cannot understand function prototype: 'enum tbt_state '
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [m]:
- RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
vim +16 drivers/usb/typec/altmodes/thunderbolt.c
15
> 16 enum tbt_state {
17 TBT_STATE_IDLE,
18 TBT_STATE_SOP_P_ENTER,
19 TBT_STATE_SOP_PP_ENTER,
20 TBT_STATE_ENTER,
21 TBT_STATE_EXIT,
22 TBT_STATE_SOP_PP_EXIT,
23 TBT_STATE_SOP_P_EXIT
24 };
25
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2024 Red Hat, Inc.