[PATCH v2 0/7] Thunderbolt and DP altmode support for cros-ec-typec

[PATCH v2 0/7] Thunderbolt and DP altmode support for cros-ec-typec

[Abhishek Pandit-Subedi]

Hi Heikki, Tzung-Bi et al,

This patch series adds support for alternate mode entry for the
cros-ec-typec driver for Displayport and Thunderbolt.

Thunderbolt support is added by adapting an RFC Heikki had posted
previously:

https://lore.kernel.org/linux-usb/20191230152857.43917-1-heikki.krogerus@linux.intel.com/

A few comments on the series:

* The cros-ec interface will not accept any VDOs/VDMs so we simply
  ignore any configurations we are passed (i.e. DPConfigure). This means
  the sysfs control of DP lanes won't work.
* ChromeOS has two modes of operation for alt-modes: entirely EC driven
  or AP-driven from userspace (via the typec daemon). Thus, we don't
  expect the kernel alt-mode drivers to auto-enter modes in all cases.
  This series allows auto-enter for displayport but disables it for TBT
  for this reason.

This was tested with a ChromeOS Brya device using kernel 6.6 and built
with allmodconfig for linux-usb.

Thanks,
Abhishek

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
- Update altmode_match to ignore mode entirely
- Also apply the same behavior to typec_match
- Refactored displayport into cros_typec_altmode.c to extract common
  implementation between altmodes
- Refactored thunderbolt support into cros_typec_altmode.c
- Only disable auto-enter for Thunderbolt
- Update commit message to clearly indicate the need for userspace
  intervention to enter TBT mode

Abhishek Pandit-Subedi (6):
  usb: typec: Only use SVID for matching altmodes
  usb: typec: Auto enter control for alternate modes
  platform/chrome: cros_ec_typec: Update partner altmode active
  platform/chrome: cros_ec_typec: Displayport support
  platform/chrome: cros_ec_typec: Thunderbolt support
  platform/chrome: cros_ec_typec: Disable tbt auto_enter

Heikki Krogerus (1):
  usb: typec: Add driver for Thunderbolt 3 Alternate Mode

 Documentation/ABI/testing/sysfs-bus-typec    |   9 +
 MAINTAINERS                                  |   3 +
 drivers/platform/chrome/Makefile             |   3 +-
 drivers/platform/chrome/cros_ec_typec.c      |  56 ++-
 drivers/platform/chrome/cros_ec_typec.h      |   1 +
 drivers/platform/chrome/cros_typec_altmode.c | 362 +++++++++++++++++++
 drivers/platform/chrome/cros_typec_altmode.h |  48 +++
 drivers/usb/typec/altmodes/Kconfig           |   9 +
 drivers/usb/typec/altmodes/Makefile          |   2 +
 drivers/usb/typec/altmodes/displayport.c     |   6 +-
 drivers/usb/typec/altmodes/thunderbolt.c     | 309 ++++++++++++++++
 drivers/usb/typec/bus.c                      |   3 +-
 drivers/usb/typec/class.c                    |  33 +-
 include/linux/usb/typec.h                    |   2 +
 include/linux/usb/typec_altmode.h            |   2 +
 include/linux/usb/typec_tbt.h                |   3 +-
 16 files changed, 830 insertions(+), 21 deletions(-)
 create mode 100644 drivers/platform/chrome/cros_typec_altmode.c
 create mode 100644 drivers/platform/chrome/cros_typec_altmode.h
 create mode 100644 drivers/usb/typec/altmodes/thunderbolt.c

-- 
2.47.0.163.g1226f6d8fa-goog

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

[Abhishek Pandit-Subedi]

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-goog

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

[kernel test robot]

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

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

[Heikki Krogerus]

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.

> +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

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

[Abhishek Pandit-Subedi]

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

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

[Heikki Krogerus]

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

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

[Abhishek Pandit-Subedi]

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

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

[Heikki Krogerus]

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

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

[Abhishek Pandit-Subedi]

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

[PATCH v2 2/7] usb: typec: Only use SVID for matching altmodes

[Abhishek Pandit-Subedi]

Mode in struct typec_altmode is used to indicate the index of the
altmode on a port, partner or plug. When searching for altmodes, it
doesn't make sense to use the mode as a criteria since it could be any
value depending on the enumeration order of the driver.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
- Update altmode_match to ignore mode entirely
- Also apply the same behavior to typec_match

 drivers/usb/typec/bus.c   | 3 +--
 drivers/usb/typec/class.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
index aa879253d3b8..a5cb4bbb877d 100644
--- a/drivers/usb/typec/bus.c
+++ b/drivers/usb/typec/bus.c
@@ -454,8 +454,7 @@ static int typec_match(struct device *dev, const struct device_driver *driver)
 	const struct typec_device_id *id;
 
 	for (id = drv->id_table; id->svid; id++)
-		if (id->svid == altmode->svid &&
-		    (id->mode == TYPEC_ANY_MODE || id->mode == altmode->mode))
+		if (id->svid == altmode->svid)
 			return 1;
 	return 0;
 }
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index bd41abceb050..85494b9f7502 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -237,7 +237,7 @@ static int altmode_match(struct device *dev, void *data)
 	if (!is_typec_altmode(dev))
 		return 0;
 
-	return ((adev->svid == id->svid) && (adev->mode == id->mode));
+	return (adev->svid == id->svid);
 }
 
 static void typec_altmode_set_partner(struct altmode *altmode)
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v2 2/7] usb: typec: Only use SVID for matching altmodes

[Heikki Krogerus]

On Wed, Oct 30, 2024 at 02:28:33PM -0700, Abhishek Pandit-Subedi wrote:
> Mode in struct typec_altmode is used to indicate the index of the
> altmode on a port, partner or plug. When searching for altmodes, it
> doesn't make sense to use the mode as a criteria since it could be any
> value depending on the enumeration order of the driver.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v2:
> - Update altmode_match to ignore mode entirely
> - Also apply the same behavior to typec_match
> 
>  drivers/usb/typec/bus.c   | 3 +--
>  drivers/usb/typec/class.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
> index aa879253d3b8..a5cb4bbb877d 100644
> --- a/drivers/usb/typec/bus.c
> +++ b/drivers/usb/typec/bus.c
> @@ -454,8 +454,7 @@ static int typec_match(struct device *dev, const struct device_driver *driver)
>  	const struct typec_device_id *id;
>  
>  	for (id = drv->id_table; id->svid; id++)
> -		if (id->svid == altmode->svid &&
> -		    (id->mode == TYPEC_ANY_MODE || id->mode == altmode->mode))
> +		if (id->svid == altmode->svid)
>  			return 1;
>  	return 0;
>  }
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index bd41abceb050..85494b9f7502 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -237,7 +237,7 @@ static int altmode_match(struct device *dev, void *data)
>  	if (!is_typec_altmode(dev))
>  		return 0;
>  
> -	return ((adev->svid == id->svid) && (adev->mode == id->mode));
> +	return (adev->svid == id->svid);
>  }
>  
>  static void typec_altmode_set_partner(struct altmode *altmode)
> -- 
> 2.47.0.163.g1226f6d8fa-goog

-- 
heikki

[PATCH v2 3/7] usb: typec: Auto enter control for alternate modes

[Abhishek Pandit-Subedi]

Add controls for whether an alternate mode is automatically entered when
a partner connects. The auto_enter control is only available on ports
and applies immediately after a partner connects. The default behavior
is to enable auto enter and drivers must explicitly disable it.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

(no changes since v1)

 Documentation/ABI/testing/sysfs-bus-typec |  9 +++++++
 drivers/usb/typec/altmodes/displayport.c  |  6 +++--
 drivers/usb/typec/altmodes/thunderbolt.c  |  3 ++-
 drivers/usb/typec/class.c                 | 31 +++++++++++++++++++++++
 include/linux/usb/typec.h                 |  2 ++
 include/linux/usb/typec_altmode.h         |  2 ++
 6 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
index 205d9c91e2e1..f09d05727b82 100644
--- a/Documentation/ABI/testing/sysfs-bus-typec
+++ b/Documentation/ABI/testing/sysfs-bus-typec
@@ -12,6 +12,15 @@ Description:
 
 		Valid values are boolean.
 
+What:		/sys/bus/typec/devices/.../auto_enter
+Date:		September 2024
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Controls whether a mode will be automatically entered when a partner is
+		connected.
+
+		This field is only valid and displayed on a port. Valid values are boolean.
+
 What:		/sys/bus/typec/devices/.../description
 Date:		July 2018
 Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 2f03190a9873..62263f1d3a72 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -767,8 +767,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
 	if (plug)
 		typec_altmode_set_drvdata(plug, dp);
 
-	dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
-	schedule_work(&dp->work);
+	if (port->auto_enter) {
+		dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
+		schedule_work(&dp->work);
+	}
 
 	return 0;
 }
diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
index 8380b22d26a7..181892bf1225 100644
--- a/drivers/usb/typec/altmodes/thunderbolt.c
+++ b/drivers/usb/typec/altmodes/thunderbolt.c
@@ -212,6 +212,7 @@ static const struct typec_altmode_ops tbt_altmode_ops = {
 
 static int tbt_altmode_probe(struct typec_altmode *alt)
 {
+	const struct typec_altmode *port = typec_altmode_get_partner(alt);
 	struct tbt_altmode *tbt;
 
 	tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
@@ -226,7 +227,7 @@ static int tbt_altmode_probe(struct typec_altmode *alt)
 	typec_altmode_set_drvdata(alt, tbt);
 	typec_altmode_set_ops(alt, &tbt_altmode_ops);
 
-	if (tbt_ready(alt)) {
+	if (port->auto_enter && 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])
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 85494b9f7502..e74f835c6859 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -403,6 +403,31 @@ static ssize_t active_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(active);
 
+static ssize_t
+auto_enter_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct typec_altmode *alt = to_typec_altmode(dev);
+
+	return sprintf(buf, "%s\n", alt->auto_enter ? "yes" : "no");
+}
+
+static ssize_t auto_enter_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t size)
+{
+	struct typec_altmode *adev = to_typec_altmode(dev);
+	bool auto_enter;
+	int ret;
+
+	ret = kstrtobool(buf, &auto_enter);
+	if (ret)
+		return ret;
+
+	adev->auto_enter = auto_enter;
+
+	return size;
+}
+static DEVICE_ATTR_RW(auto_enter);
+
 static ssize_t
 supported_roles_show(struct device *dev, struct device_attribute *attr,
 		     char *buf)
@@ -446,6 +471,7 @@ static DEVICE_ATTR_RO(svid);
 
 static struct attribute *typec_altmode_attrs[] = {
 	&dev_attr_active.attr,
+	&dev_attr_auto_enter.attr,
 	&dev_attr_mode.attr,
 	&dev_attr_svid.attr,
 	&dev_attr_vdo.attr,
@@ -461,6 +487,10 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
 		if (!adev->ops || !adev->ops->activate)
 			return 0444;
 
+	if (attr == &dev_attr_auto_enter.attr)
+		if (!is_typec_port(adev->dev.parent))
+			return 0;
+
 	return attr->mode;
 }
 
@@ -564,6 +594,7 @@ typec_register_altmode(struct device *parent,
 	if (is_port) {
 		alt->attrs[3] = &dev_attr_supported_roles.attr;
 		alt->adev.active = true; /* Enabled by default */
+		alt->adev.auto_enter = !desc->no_auto_enter;
 	}
 
 	sprintf(alt->group_name, "mode%d", desc->mode);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index d616b8807000..5336b7c92ca4 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -139,6 +139,7 @@ int typec_cable_set_identity(struct typec_cable *cable);
  * @svid: Standard or Vendor ID
  * @mode: Index of the Mode
  * @vdo: VDO returned by Discover Modes USB PD command
+ * @no_auto_enter: Only for ports. Disables auto enter which is default behavior.
  * @roles: Only for ports. DRP if the mode is available in both roles
  *
  * Description of an Alternate Mode which a connector, cable plug or partner
@@ -148,6 +149,7 @@ struct typec_altmode_desc {
 	u16			svid;
 	u8			mode;
 	u32			vdo;
+	bool			no_auto_enter;
 	/* Only used with ports */
 	enum typec_port_data	roles;
 };
diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
index b3c0866ea70f..ab7c3ebe4926 100644
--- a/include/linux/usb/typec_altmode.h
+++ b/include/linux/usb/typec_altmode.h
@@ -18,6 +18,7 @@ struct typec_altmode_ops;
  * @mode: Index of the Mode
  * @vdo: VDO returned by Discover Modes USB PD command
  * @active: Tells has the mode been entered or not
+ * @auto_enter: Tells whether to auto-enter mode (only valid for port mode).
  * @desc: Optional human readable description of the mode
  * @ops: Operations vector from the driver
  * @cable_ops: Cable operations vector from the driver.
@@ -28,6 +29,7 @@ struct typec_altmode {
 	int				mode;
 	u32				vdo;
 	unsigned int			active:1;
+	unsigned int			auto_enter:1;
 
 	char				*desc;
 	const struct typec_altmode_ops	*ops;
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v2 3/7] usb: typec: Auto enter control for alternate modes

[Heikki Krogerus]

On Wed, Oct 30, 2024 at 02:28:34PM -0700, Abhishek Pandit-Subedi wrote:
> Add controls for whether an alternate mode is automatically entered when
> a partner connects. The auto_enter control is only available on ports
> and applies immediately after a partner connects. The default behavior
> is to enable auto enter and drivers must explicitly disable it.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  Documentation/ABI/testing/sysfs-bus-typec |  9 +++++++
>  drivers/usb/typec/altmodes/displayport.c  |  6 +++--
>  drivers/usb/typec/altmodes/thunderbolt.c  |  3 ++-
>  drivers/usb/typec/class.c                 | 31 +++++++++++++++++++++++
>  include/linux/usb/typec.h                 |  2 ++
>  include/linux/usb/typec_altmode.h         |  2 ++
>  6 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
> index 205d9c91e2e1..f09d05727b82 100644
> --- a/Documentation/ABI/testing/sysfs-bus-typec
> +++ b/Documentation/ABI/testing/sysfs-bus-typec
> @@ -12,6 +12,15 @@ Description:
>  
>  		Valid values are boolean.
>  
> +What:		/sys/bus/typec/devices/.../auto_enter
> +Date:		September 2024
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Controls whether a mode will be automatically entered when a partner is
> +		connected.
> +
> +		This field is only valid and displayed on a port. Valid values are boolean.

So, why can't this be controlled with the "active" property of the
port altmode instead? That's why it's there.

Sorry if I missed something in v1 related to this question.

thanks,

-- 
heikki

Re: [PATCH v2 3/7] usb: typec: Auto enter control for alternate modes

[Abhishek Pandit-Subedi]

On Thu, Oct 31, 2024 at 7:33 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Oct 30, 2024 at 02:28:34PM -0700, Abhishek Pandit-Subedi wrote:
> > Add controls for whether an alternate mode is automatically entered when
> > a partner connects. The auto_enter control is only available on ports
> > and applies immediately after a partner connects. The default behavior
> > is to enable auto enter and drivers must explicitly disable it.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  Documentation/ABI/testing/sysfs-bus-typec |  9 +++++++
> >  drivers/usb/typec/altmodes/displayport.c  |  6 +++--
> >  drivers/usb/typec/altmodes/thunderbolt.c  |  3 ++-
> >  drivers/usb/typec/class.c                 | 31 +++++++++++++++++++++++
> >  include/linux/usb/typec.h                 |  2 ++
> >  include/linux/usb/typec_altmode.h         |  2 ++
> >  6 files changed, 50 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
> > index 205d9c91e2e1..f09d05727b82 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-typec
> > +++ b/Documentation/ABI/testing/sysfs-bus-typec
> > @@ -12,6 +12,15 @@ Description:
> >
> >               Valid values are boolean.
> >
> > +What:                /sys/bus/typec/devices/.../auto_enter
> > +Date:                September 2024
> > +Contact:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:
> > +             Controls whether a mode will be automatically entered when a partner is
> > +             connected.
> > +
> > +             This field is only valid and displayed on a port. Valid values are boolean.
>
> So, why can't this be controlled with the "active" property of the
> port altmode instead? That's why it's there.
>
> Sorry if I missed something in v1 related to this question.

There was a bit of discussion around this in another patch in v1:
https://patchwork.kernel.org/project/chrome-platform/patch/20240925092505.8.Ic14738918e3d026fa2d85e95fb68f8e07a0828d0@changeid/
And this patch is probably a good place to continue that discussion.

With the way altmodes drivers currently work, they will auto-enter
when probed. So if you have a partner that supports both displayport
and thunderbolt, they will both attempt to auto-enter on probe. I
think I could use the `active` field instead so that the port altmode
blocks entry until userspace enables it -- this would avoid the need
to add one more sysfs ABI. I'll actually go ahead and do this for the
next patch series I send up.

However, the underlying problem I'm trying to solve still exists: how
do you choose a specific altmode to enter if there are multiple to
choose from? I tried to implement a method that first tries USB4 and
then Thunderbolt and then DP but I realized that the altmode drivers
don't necessarily bind immediately after a partner altmode is
registered so I can't just call `activate` (since no ops are attached
to the partner altmode yet). Do you have any thoughts about how to
handle multiple modes as well as how to handle fallback mode entry
(i.e. thunderbolt fails so you try DPAM next)?

>
> thanks,
>
> --
> heikki

Re: [PATCH v2 3/7] usb: typec: Auto enter control for alternate modes

[Heikki Krogerus]

On Thu, Oct 31, 2024 at 03:48:45PM -0700, Abhishek Pandit-Subedi wrote:
> On Thu, Oct 31, 2024 at 7:33 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Wed, Oct 30, 2024 at 02:28:34PM -0700, Abhishek Pandit-Subedi wrote:
> > > Add controls for whether an alternate mode is automatically entered when
> > > a partner connects. The auto_enter control is only available on ports
> > > and applies immediately after a partner connects. The default behavior
> > > is to enable auto enter and drivers must explicitly disable it.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  Documentation/ABI/testing/sysfs-bus-typec |  9 +++++++
> > >  drivers/usb/typec/altmodes/displayport.c  |  6 +++--
> > >  drivers/usb/typec/altmodes/thunderbolt.c  |  3 ++-
> > >  drivers/usb/typec/class.c                 | 31 +++++++++++++++++++++++
> > >  include/linux/usb/typec.h                 |  2 ++
> > >  include/linux/usb/typec_altmode.h         |  2 ++
> > >  6 files changed, 50 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
> > > index 205d9c91e2e1..f09d05727b82 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-typec
> > > +++ b/Documentation/ABI/testing/sysfs-bus-typec
> > > @@ -12,6 +12,15 @@ Description:
> > >
> > >               Valid values are boolean.
> > >
> > > +What:                /sys/bus/typec/devices/.../auto_enter
> > > +Date:                September 2024
> > > +Contact:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:
> > > +             Controls whether a mode will be automatically entered when a partner is
> > > +             connected.
> > > +
> > > +             This field is only valid and displayed on a port. Valid values are boolean.
> >
> > So, why can't this be controlled with the "active" property of the
> > port altmode instead? That's why it's there.
> >
> > Sorry if I missed something in v1 related to this question.
> 
> There was a bit of discussion around this in another patch in v1:
> https://patchwork.kernel.org/project/chrome-platform/patch/20240925092505.8.Ic14738918e3d026fa2d85e95fb68f8e07a0828d0@changeid/
> And this patch is probably a good place to continue that discussion.
> 
> With the way altmodes drivers currently work, they will auto-enter
> when probed. So if you have a partner that supports both displayport
> and thunderbolt, they will both attempt to auto-enter on probe. I
> think I could use the `active` field instead so that the port altmode
> blocks entry until userspace enables it -- this would avoid the need
> to add one more sysfs ABI. I'll actually go ahead and do this for the
> next patch series I send up.
> 
> However, the underlying problem I'm trying to solve still exists: how
> do you choose a specific altmode to enter if there are multiple to
> choose from? I tried to implement a method that first tries USB4 and
> then Thunderbolt and then DP but I realized that the altmode drivers
> don't necessarily bind immediately after a partner altmode is
> registered so I can't just call `activate` (since no ops are attached
> to the partner altmode yet). Do you have any thoughts about how to
> handle multiple modes as well as how to handle fallback mode entry
> (i.e. thunderbolt fails so you try DPAM next)?

If the user space needs to take over control of the entry order, then
can't it just de-activate all port alt modes by default, and then
activate the one that needs to enter? The port driver probable needs
to implent the "activate" callback for this.

The user space can see when the driver is bound to a device by
monitoring the uevents, no?

thanks,

-- 
heikki

Re: [PATCH v2 3/7] usb: typec: Auto enter control for alternate modes

[Abhishek Pandit-Subedi]

On Fri, Nov 1, 2024 at 6:59 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Thu, Oct 31, 2024 at 03:48:45PM -0700, Abhishek Pandit-Subedi wrote:
> > On Thu, Oct 31, 2024 at 7:33 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Wed, Oct 30, 2024 at 02:28:34PM -0700, Abhishek Pandit-Subedi wrote:
> > > > Add controls for whether an alternate mode is automatically entered when
> > > > a partner connects. The auto_enter control is only available on ports
> > > > and applies immediately after a partner connects. The default behavior
> > > > is to enable auto enter and drivers must explicitly disable it.
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  Documentation/ABI/testing/sysfs-bus-typec |  9 +++++++
> > > >  drivers/usb/typec/altmodes/displayport.c  |  6 +++--
> > > >  drivers/usb/typec/altmodes/thunderbolt.c  |  3 ++-
> > > >  drivers/usb/typec/class.c                 | 31 +++++++++++++++++++++++
> > > >  include/linux/usb/typec.h                 |  2 ++
> > > >  include/linux/usb/typec_altmode.h         |  2 ++
> > > >  6 files changed, 50 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
> > > > index 205d9c91e2e1..f09d05727b82 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-typec
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-typec
> > > > @@ -12,6 +12,15 @@ Description:
> > > >
> > > >               Valid values are boolean.
> > > >
> > > > +What:                /sys/bus/typec/devices/.../auto_enter
> > > > +Date:                September 2024
> > > > +Contact:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > +Description:
> > > > +             Controls whether a mode will be automatically entered when a partner is
> > > > +             connected.
> > > > +
> > > > +             This field is only valid and displayed on a port. Valid values are boolean.
> > >
> > > So, why can't this be controlled with the "active" property of the
> > > port altmode instead? That's why it's there.
> > >
> > > Sorry if I missed something in v1 related to this question.
> >
> > There was a bit of discussion around this in another patch in v1:
> > https://patchwork.kernel.org/project/chrome-platform/patch/20240925092505.8.Ic14738918e3d026fa2d85e95fb68f8e07a0828d0@changeid/
> > And this patch is probably a good place to continue that discussion.
> >
> > With the way altmodes drivers currently work, they will auto-enter
> > when probed. So if you have a partner that supports both displayport
> > and thunderbolt, they will both attempt to auto-enter on probe. I
> > think I could use the `active` field instead so that the port altmode
> > blocks entry until userspace enables it -- this would avoid the need
> > to add one more sysfs ABI. I'll actually go ahead and do this for the
> > next patch series I send up.
> >
> > However, the underlying problem I'm trying to solve still exists: how
> > do you choose a specific altmode to enter if there are multiple to
> > choose from? I tried to implement a method that first tries USB4 and
> > then Thunderbolt and then DP but I realized that the altmode drivers
> > don't necessarily bind immediately after a partner altmode is
> > registered so I can't just call `activate` (since no ops are attached
> > to the partner altmode yet). Do you have any thoughts about how to
> > handle multiple modes as well as how to handle fallback mode entry
> > (i.e. thunderbolt fails so you try DPAM next)?
>
> If the user space needs to take over control of the entry order, then
> can't it just de-activate all port alt modes by default, and then
> activate the one that needs to enter? The port driver probable needs
> to implent the "activate" callback for this.
>
> The user space can see when the driver is bound to a device by
> monitoring the uevents, no?

This requires userspace intervention to do the correct thing. Let's
take a real world example:

I have a TBT4 dock that supports DPAM (svid 0xff01), TBT (svid 0x8087)
and also USB4.

* When I plug in the dock, it enumerates and registers the partner
altmodes. The altmode bus matches typec_displayport and
typec_thunderbolt and loads the drivers for them. By default, both
drivers will try to activate their altmode on probe(). Having a
userspace daemon disable the altmode on the ports and enable them on
connection will probably work here.
* If I boot with the dock connected, the same thing happens but my
userspace daemon may not be running yet. What should the default
kernel behavior be to enter alt-modes then? When you throw USB4 into
the mix, this becomes another can of worms since you probably don't
want to downgrade from USB4 to DPAM.

On ChromeOS, prior to this patch series, our userspace daemon (typecd)
could handle all of this in userspace since it could just wait for
`num_alt_modes` to be filled on partner-attach before trying to enter
the right mode (via a side-band channel to the EC). After this change,
typecd will be in a similar bind -- it will have to wait until all
attached partner SVIDs have drivers attached (if available).

Underlying all this, I guess the real need here is for some sort of
signal that says: All partner modes are registered, any necessary
drivers for these modes are bound and you are ready to make a decision
on which mode to enter. Then, we could iteratively try to enter the
best mode (USB4 > TBT > DP) and report failure conditions on why it
couldn't be entered (i.e. Cable speed, partner problem / link
training, etc). This could be done in kernel or userspace depending on
the system.

>
> thanks,
>
> --
> heikki

Re: [PATCH v2 3/7] usb: typec: Auto enter control for alternate modes

[Heikki Krogerus]

On Fri, Nov 01, 2024 at 09:53:14AM -0700, Abhishek Pandit-Subedi wrote:
> On Fri, Nov 1, 2024 at 6:59 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Thu, Oct 31, 2024 at 03:48:45PM -0700, Abhishek Pandit-Subedi wrote:
> > > On Thu, Oct 31, 2024 at 7:33 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Wed, Oct 30, 2024 at 02:28:34PM -0700, Abhishek Pandit-Subedi wrote:
> > > > > Add controls for whether an alternate mode is automatically entered when
> > > > > a partner connects. The auto_enter control is only available on ports
> > > > > and applies immediately after a partner connects. The default behavior
> > > > > is to enable auto enter and drivers must explicitly disable it.
> > > > >
> > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  Documentation/ABI/testing/sysfs-bus-typec |  9 +++++++
> > > > >  drivers/usb/typec/altmodes/displayport.c  |  6 +++--
> > > > >  drivers/usb/typec/altmodes/thunderbolt.c  |  3 ++-
> > > > >  drivers/usb/typec/class.c                 | 31 +++++++++++++++++++++++
> > > > >  include/linux/usb/typec.h                 |  2 ++
> > > > >  include/linux/usb/typec_altmode.h         |  2 ++
> > > > >  6 files changed, 50 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
> > > > > index 205d9c91e2e1..f09d05727b82 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-typec
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-typec
> > > > > @@ -12,6 +12,15 @@ Description:
> > > > >
> > > > >               Valid values are boolean.
> > > > >
> > > > > +What:                /sys/bus/typec/devices/.../auto_enter
> > > > > +Date:                September 2024
> > > > > +Contact:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > +Description:
> > > > > +             Controls whether a mode will be automatically entered when a partner is
> > > > > +             connected.
> > > > > +
> > > > > +             This field is only valid and displayed on a port. Valid values are boolean.
> > > >
> > > > So, why can't this be controlled with the "active" property of the
> > > > port altmode instead? That's why it's there.
> > > >
> > > > Sorry if I missed something in v1 related to this question.
> > >
> > > There was a bit of discussion around this in another patch in v1:
> > > https://patchwork.kernel.org/project/chrome-platform/patch/20240925092505.8.Ic14738918e3d026fa2d85e95fb68f8e07a0828d0@changeid/
> > > And this patch is probably a good place to continue that discussion.
> > >
> > > With the way altmodes drivers currently work, they will auto-enter
> > > when probed. So if you have a partner that supports both displayport
> > > and thunderbolt, they will both attempt to auto-enter on probe. I
> > > think I could use the `active` field instead so that the port altmode
> > > blocks entry until userspace enables it -- this would avoid the need
> > > to add one more sysfs ABI. I'll actually go ahead and do this for the
> > > next patch series I send up.
> > >
> > > However, the underlying problem I'm trying to solve still exists: how
> > > do you choose a specific altmode to enter if there are multiple to
> > > choose from? I tried to implement a method that first tries USB4 and
> > > then Thunderbolt and then DP but I realized that the altmode drivers
> > > don't necessarily bind immediately after a partner altmode is
> > > registered so I can't just call `activate` (since no ops are attached
> > > to the partner altmode yet). Do you have any thoughts about how to
> > > handle multiple modes as well as how to handle fallback mode entry
> > > (i.e. thunderbolt fails so you try DPAM next)?
> >
> > If the user space needs to take over control of the entry order, then
> > can't it just de-activate all port alt modes by default, and then
> > activate the one that needs to enter? The port driver probable needs
> > to implent the "activate" callback for this.
> >
> > The user space can see when the driver is bound to a device by
> > monitoring the uevents, no?
> 
> This requires userspace intervention to do the correct thing. Let's
> take a real world example:
> 
> I have a TBT4 dock that supports DPAM (svid 0xff01), TBT (svid 0x8087)
> and also USB4.
> 
> * When I plug in the dock, it enumerates and registers the partner
> altmodes. The altmode bus matches typec_displayport and
> typec_thunderbolt and loads the drivers for them. By default, both
> drivers will try to activate their altmode on probe(). Having a
> userspace daemon disable the altmode on the ports and enable them on
> connection will probably work here.
> * If I boot with the dock connected, the same thing happens but my
> userspace daemon may not be running yet. What should the default
> kernel behavior be to enter alt-modes then? When you throw USB4 into
> the mix, this becomes another can of worms since you probably don't
> want to downgrade from USB4 to DPAM.

If you have already entered USB4, then all alt modes need fail to
enter with -EBUSY, just like when another alt mode was already
entered successfully. Right now the port driver is responsible of
checking USB4 status, but we can easily add a check for the usb_mode
of the partner to the typec_altmode_enter().

The default entry order will in practice be the order in which the
modes are discovered (so USB4 will always be first), but the port
drivers can of course influence this by registering the modes in a
specific order - first-come, first-served. But that is only useful if
the port driver knows the priorities of the modes.

You can leave the decision to the user space for example by adding
that "no_auto_enter", that's not a problem, but it still does not
require a new sysfs attribute file. You just use that flag to set the
default value for the "active" attribute.

> On ChromeOS, prior to this patch series, our userspace daemon (typecd)
> could handle all of this in userspace since it could just wait for
> `num_alt_modes` to be filled on partner-attach before trying to enter
> the right mode (via a side-band channel to the EC). After this change,
> typecd will be in a similar bind -- it will have to wait until all
> attached partner SVIDs have drivers attached (if available).
> 
> Underlying all this, I guess the real need here is for some sort of
> signal that says: All partner modes are registered, any necessary
> drivers for these modes are bound and you are ready to make a decision
> on which mode to enter. Then, we could iteratively try to enter the
> best mode (USB4 > TBT > DP) and report failure conditions on why it
> couldn't be entered (i.e. Cable speed, partner problem / link
> training, etc). This could be done in kernel or userspace depending on
> the system.

In user space you use the "num_alt_modes" to see how many alt modes
there are, and then simply wait for all of them (or just the ones that
you are interested in) get registered and bind to a driver. After that
you can enter which ever you want. I don't think you need anything
else there.

If you still need some way to tell the port drivers how to prioritise
the modes without user space, then I think you need some way for the
firmware to be able to deliver that information to the driver.

thanks,

-- 
heikki

[PATCH v2 4/7] platform/chrome: cros_ec_typec: Update partner altmode active

[Abhishek Pandit-Subedi]

Mux configuration is often the final piece of mode entry and can be used
to determine whether a partner altmode is active. When mux configuration
is done, use the active port altmode's SVID to set the partner active
field for all partner alt modes.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

(no changes since v1)

 drivers/platform/chrome/cros_ec_typec.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 53d93baa36a8..0c8db11bd8a4 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -618,6 +618,7 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
 	};
 	struct ec_params_usb_pd_mux_ack mux_ack;
 	enum typec_orientation orientation;
+	struct cros_typec_altmode_node *node, *n;
 	int ret;
 
 	ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_MUX_INFO,
@@ -676,6 +677,16 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
 			port->mux_flags);
 	}
 
+	/* Iterate all partner alt-modes and set the active alternate mode. */
+	list_for_each_entry_safe(node, n, &port->partner_mode_list, list) {
+		if (port->state.alt != NULL &&
+		    node->amode->svid == port->state.alt->svid) {
+			typec_altmode_update_active(node->amode, true);
+		} else {
+			typec_altmode_update_active(node->amode, false);
+		}
+	}
+
 mux_ack:
 	if (!typec->needs_mux_ack)
 		return ret;
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 5/7] platform/chrome: cros_ec_typec: Displayport support

[Abhishek Pandit-Subedi]

Add support for entering and exiting displayport alt-mode on systems
using AP driven alt-mode.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
- Refactored displayport into cros_typec_altmode.c to extract common
  implementation between altmodes

 MAINTAINERS                                  |   3 +
 drivers/platform/chrome/Makefile             |   3 +-
 drivers/platform/chrome/cros_ec_typec.c      |  13 +-
 drivers/platform/chrome/cros_ec_typec.h      |   1 +
 drivers/platform/chrome/cros_typec_altmode.c | 277 +++++++++++++++++++
 drivers/platform/chrome/cros_typec_altmode.h |  34 +++
 6 files changed, 329 insertions(+), 2 deletions(-)
 create mode 100644 drivers/platform/chrome/cros_typec_altmode.c
 create mode 100644 drivers/platform/chrome/cros_typec_altmode.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e9659a5a7fb3..de99bcbda7d4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5369,9 +5369,12 @@ F:	include/linux/platform_data/cros_usbpd_notify.h
 
 CHROMEOS EC USB TYPE-C DRIVER
 M:	Prashant Malani <pmalani@chromium.org>
+M:	Benson Leung <bleung@chromium.org>
+M:	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
 L:	chrome-platform@lists.linux.dev
 S:	Maintained
 F:	drivers/platform/chrome/cros_ec_typec.*
+F:	drivers/platform/chrome/cros_typec_altmode.*
 F:	drivers/platform/chrome/cros_typec_switch.c
 F:	drivers/platform/chrome/cros_typec_vdm.*
 
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..8b007404c024 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -17,8 +17,9 @@ obj-$(CONFIG_CROS_EC_RPMSG)		+= cros_ec_rpmsg.o
 obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
 obj-$(CONFIG_CROS_EC_UART)		+= cros_ec_uart.o
 cros_ec_lpcs-objs			:= cros_ec_lpc.o cros_ec_lpc_mec.o
-cros-ec-typec-objs			:= cros_ec_typec.o cros_typec_vdm.o
+cros-ec-typec-objs			:= cros_ec_typec.o cros_typec_vdm.o cros_typec_altmode.o
 obj-$(CONFIG_CROS_EC_TYPEC)		+= cros-ec-typec.o
+
 obj-$(CONFIG_CROS_EC_LPC)		+= cros_ec_lpcs.o
 obj-$(CONFIG_CROS_EC_PROTO)		+= cros_ec_proto.o cros_ec_trace.o
 obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)	+= cros_kbd_led_backlight.o
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 0c8db11bd8a4..7997e7136c4c 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -18,6 +18,7 @@
 
 #include "cros_ec_typec.h"
 #include "cros_typec_vdm.h"
+#include "cros_typec_altmode.h"
 
 #define DRV_NAME "cros-ec-typec"
 
@@ -293,12 +294,16 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
 	desc.svid = USB_TYPEC_DP_SID;
 	desc.mode = USB_TYPEC_DP_MODE;
 	desc.vdo = DP_PORT_VDO;
-	amode = typec_port_register_altmode(port->port, &desc);
+	amode = cros_typec_register_displayport(port, &desc,
+						typec->ap_driven_altmode);
 	if (IS_ERR(amode))
 		return PTR_ERR(amode);
 	port->port_altmode[CROS_EC_ALTMODE_DP] = amode;
+
+#if !IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
 	typec_altmode_set_drvdata(amode, port);
 	amode->ops = &port_amode_ops;
+#endif
 
 	/*
 	 * Register TBT compatibility alt mode. The EC will not enter the mode
@@ -575,6 +580,10 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec,
 	if (!ret)
 		ret = typec_mux_set(port->mux, &port->state);
 
+	if (!ret)
+		cros_typec_displayport_status_update(port->state.alt,
+						     port->state.data);
+
 	return ret;
 }
 
@@ -1254,6 +1263,8 @@ static int cros_typec_probe(struct platform_device *pdev)
 
 	typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD);
 	typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);
+	typec->ap_driven_altmode = cros_ec_check_features(
+		ec_dev, EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY);
 
 	ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
 			  &resp, sizeof(resp));
diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
index deda180a646f..9fd5342bb0ad 100644
--- a/drivers/platform/chrome/cros_ec_typec.h
+++ b/drivers/platform/chrome/cros_ec_typec.h
@@ -39,6 +39,7 @@ struct cros_typec_data {
 	struct work_struct port_work;
 	bool typec_cmd_supported;
 	bool needs_mux_ack;
+	bool ap_driven_altmode;
 };
 
 /* Per port data. */
diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
new file mode 100644
index 000000000000..af2f077674f1
--- /dev/null
+++ b/drivers/platform/chrome/cros_typec_altmode.c
@@ -0,0 +1,277 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Alt-mode implementation on ChromeOS EC.
+ *
+ * Copyright 2024 Google LLC
+ * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
+ */
+#include "cros_ec_typec.h"
+
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/pd_vdo.h>
+
+#include "cros_typec_altmode.h"
+
+struct cros_typec_dp_data {
+	struct typec_displayport_data data;
+
+	bool configured;
+	bool pending_status_update;
+};
+
+struct cros_typec_altmode_data {
+	struct work_struct work;
+	struct cros_typec_port *port;
+	struct typec_altmode *alt;
+	bool ap_mode_entry;
+
+	u32 header;
+	u32 *vdo_data;
+	u8 vdo_size;
+
+	u16 sid;
+	u8 mode;
+
+	union am_specific {
+		struct cros_typec_dp_data dp;
+	} am_data;
+};
+
+static void cros_typec_altmode_work(struct work_struct *work)
+{
+	struct cros_typec_altmode_data *data =
+		container_of(work, struct cros_typec_altmode_data, work);
+
+	if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
+			      data->vdo_size))
+		dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
+
+	data->header = 0;
+	data->vdo_data = NULL;
+	data->vdo_size = 0;
+}
+
+static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
+{
+	struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
+	struct ec_params_typec_control req = {
+		.port = data->port->port_num,
+		.command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
+	};
+	int svdm_version;
+	int ret;
+
+	if (!data->ap_mode_entry) {
+		const struct typec_altmode *partner =
+			typec_altmode_get_partner(alt);
+		dev_warn(&partner->dev,
+			 "EC does not support ap driven mode entry\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (data->sid == USB_TYPEC_DP_SID)
+		req.mode_to_enter = CROS_EC_ALTMODE_DP;
+	else
+		return -EOPNOTSUPP;
+
+	ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
+			  &req, sizeof(req), NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	svdm_version = typec_altmode_get_svdm_version(alt);
+	if (svdm_version < 0)
+		return svdm_version;
+
+	data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
+	data->header |= VDO_OPOS(data->mode);
+	data->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+	data->vdo_data = NULL;
+	data->vdo_size = 1;
+
+	schedule_work(&data->work);
+
+	return ret;
+}
+
+static int cros_typec_altmode_exit(struct typec_altmode *alt)
+{
+	struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
+	struct ec_params_typec_control req = {
+		.port = data->port->port_num,
+		.command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
+	};
+	int svdm_version;
+	int ret;
+
+	if (!data->ap_mode_entry) {
+		const struct typec_altmode *partner =
+			typec_altmode_get_partner(alt);
+		dev_warn(&partner->dev,
+			 "EC does not support ap driven mode entry\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
+			  &req, sizeof(req), NULL, 0);
+
+	if (ret < 0)
+		return ret;
+
+	svdm_version = typec_altmode_get_svdm_version(alt);
+	if (svdm_version < 0)
+		return svdm_version;
+
+	data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
+	data->header |= VDO_OPOS(data->mode);
+	data->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+	data->vdo_data = NULL;
+	data->vdo_size = 1;
+
+	schedule_work(&data->work);
+
+	return ret;
+}
+
+static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
+				      const u32 *data, int count)
+{
+	struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
+
+	int cmd_type = PD_VDO_CMDT(header);
+	int cmd = PD_VDO_CMD(header);
+	int svdm_version;
+
+	if (!adata->ap_mode_entry) {
+		const struct typec_altmode *partner =
+			typec_altmode_get_partner(alt);
+		dev_warn(&partner->dev,
+			 "EC does not support ap driven mode entry\n");
+		return -EOPNOTSUPP;
+	}
+
+	svdm_version = typec_altmode_get_svdm_version(alt);
+	if (svdm_version < 0)
+		return svdm_version;
+
+	switch (cmd_type) {
+	case CMDT_INIT:
+		if (PD_VDO_SVDM_VER(header) < svdm_version) {
+			typec_partner_set_svdm_version(adata->port->partner,
+						       PD_VDO_SVDM_VER(header));
+			svdm_version = PD_VDO_SVDM_VER(header);
+		}
+
+		adata->header = VDO(adata->sid, 1, svdm_version, cmd);
+		adata->header |= VDO_OPOS(adata->mode);
+
+		/*
+		 * DP_CMD_CONFIGURE: We can't actually do anything with the
+		 * provided VDO yet so just send back an ACK.
+		 *
+		 * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
+		 * DPStatus Acks.
+		 */
+		switch (cmd) {
+		case DP_CMD_CONFIGURE:
+			adata->am_data.dp.data.conf = *data;
+			adata->header |= VDO_CMDT(CMDT_RSP_ACK);
+			adata->am_data.dp.configured = true;
+			schedule_work(&adata->work);
+			break;
+		case DP_CMD_STATUS_UPDATE:
+			adata->am_data.dp.pending_status_update = true;
+			break;
+		default:
+			adata->header |= VDO_CMDT(CMDT_RSP_ACK);
+			schedule_work(&adata->work);
+			break;
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
+				      const u32 *data, int count)
+{
+	struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
+
+	if (adata->sid == USB_TYPEC_DP_SID)
+		return cros_typec_displayport_vdm(alt, header, data, count);
+
+	return -EINVAL;
+}
+
+static const struct typec_altmode_ops cros_typec_altmode_ops = {
+	.enter = cros_typec_altmode_enter,
+	.exit = cros_typec_altmode_exit,
+	.vdm = cros_typec_altmode_vdm,
+};
+
+#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
+int cros_typec_displayport_status_update(struct typec_altmode *altmode,
+					 struct typec_displayport_data *data)
+{
+	struct cros_typec_altmode_data *adata =
+		typec_altmode_get_drvdata(altmode);
+
+	if (!adata->am_data.dp.pending_status_update) {
+		dev_dbg(&altmode->dev,
+			"Got DPStatus without a pending request");
+		return 0;
+	}
+
+	if (adata->am_data.dp.configured && adata->am_data.dp.data.conf != data->conf)
+		dev_dbg(&altmode->dev,
+			"DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
+			adata->am_data.dp.data.conf, data->conf);
+
+	adata->am_data.dp.data = *data;
+	adata->am_data.dp.pending_status_update = false;
+	adata->header |= VDO_CMDT(CMDT_RSP_ACK);
+	adata->vdo_data = &adata->am_data.dp.data.status;
+	adata->vdo_size = 2;
+
+	schedule_work(&adata->work);
+	return 0;
+}
+
+struct typec_altmode *
+cros_typec_register_displayport(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc,
+				bool ap_mode_entry)
+{
+	struct typec_altmode *alt;
+	struct cros_typec_altmode_data *data;
+
+	alt = typec_port_register_altmode(port->port, desc);
+	if (IS_ERR(alt))
+		return alt;
+
+	data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		typec_unregister_altmode(alt);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	INIT_WORK(&data->work, cros_typec_altmode_work);
+	data->alt = alt;
+	data->port = port;
+	data->ap_mode_entry = ap_mode_entry;
+	data->sid = USB_TYPEC_DP_SID;
+	data->mode = USB_TYPEC_DP_MODE;
+
+	data->am_data.dp.configured = false;
+	typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
+	typec_altmode_set_drvdata(alt, data);
+
+	return alt;
+}
+#endif
diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
new file mode 100644
index 000000000000..c6f8fb02c99c
--- /dev/null
+++ b/drivers/platform/chrome/cros_typec_altmode.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __CROS_TYPEC_ALTMODE_H__
+#define __CROS_TYPEC_ALTMODE_H__
+
+struct cros_typec_port;
+struct typec_altmode;
+struct typec_altmode_desc;
+struct typec_displayport_data;
+
+#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
+struct typec_altmode *
+cros_typec_register_displayport(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc,
+				bool ap_mode_entry);
+
+int cros_typec_displayport_status_update(struct typec_altmode *altmode,
+					 struct typec_displayport_data *data);
+#else
+static inline struct typec_altmode *
+cros_typec_register_displayport(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc,
+				bool ap_mode_entry)
+{
+	return typec_port_register_altmode(port->port, desc);
+}
+
+static inline int cros_typec_displayport_status_update(struct typec_altmode *altmode,
+					 struct typec_displayport_data *data)
+{
+	return 0;
+}
+#endif
+#endif /* __CROS_TYPEC_ALTMODE_H__ */
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v2 5/7] platform/chrome: cros_ec_typec: Displayport support

[kernel test robot]

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-20241031]
[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.5.I142fc0c09df58689b98f0cebf1c5e48b9d4fa800%40changeid
patch subject: [PATCH v2 5/7] platform/chrome: cros_ec_typec: Displayport support
config: arm-defconfig (https://download.01.org/0day-ci/archive/20241101/202411010134.yJIy0Z3P-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411010134.yJIy0Z3P-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/202411010134.yJIy0Z3P-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/platform/chrome/cros_typec_altmode.c:40:13: warning: unused function 'cros_typec_altmode_work' [-Wunused-function]
   static void cros_typec_altmode_work(struct work_struct *work)
               ^
>> drivers/platform/chrome/cros_typec_altmode.c:212:39: warning: unused variable 'cros_typec_altmode_ops' [-Wunused-const-variable]
   static const struct typec_altmode_ops cros_typec_altmode_ops = {
                                         ^
   2 warnings generated.


vim +/cros_typec_altmode_ops +212 drivers/platform/chrome/cros_typec_altmode.c

    39	
  > 40	static void cros_typec_altmode_work(struct work_struct *work)
    41	{
    42		struct cros_typec_altmode_data *data =
    43			container_of(work, struct cros_typec_altmode_data, work);
    44	
    45		if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
    46				      data->vdo_size))
    47			dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
    48	
    49		data->header = 0;
    50		data->vdo_data = NULL;
    51		data->vdo_size = 0;
    52	}
    53	
    54	static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
    55	{
    56		struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
    57		struct ec_params_typec_control req = {
    58			.port = data->port->port_num,
    59			.command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
    60		};
    61		int svdm_version;
    62		int ret;
    63	
    64		if (!data->ap_mode_entry) {
    65			const struct typec_altmode *partner =
    66				typec_altmode_get_partner(alt);
    67			dev_warn(&partner->dev,
    68				 "EC does not support ap driven mode entry\n");
    69			return -EOPNOTSUPP;
    70		}
    71	
    72		if (data->sid == USB_TYPEC_DP_SID)
    73			req.mode_to_enter = CROS_EC_ALTMODE_DP;
    74		else
    75			return -EOPNOTSUPP;
    76	
    77		ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
    78				  &req, sizeof(req), NULL, 0);
    79		if (ret < 0)
    80			return ret;
    81	
    82		svdm_version = typec_altmode_get_svdm_version(alt);
    83		if (svdm_version < 0)
    84			return svdm_version;
    85	
    86		data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
    87		data->header |= VDO_OPOS(data->mode);
    88		data->header |= VDO_CMDT(CMDT_RSP_ACK);
    89	
    90		data->vdo_data = NULL;
    91		data->vdo_size = 1;
    92	
    93		schedule_work(&data->work);
    94	
    95		return ret;
    96	}
    97	
    98	static int cros_typec_altmode_exit(struct typec_altmode *alt)
    99	{
   100		struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
   101		struct ec_params_typec_control req = {
   102			.port = data->port->port_num,
   103			.command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
   104		};
   105		int svdm_version;
   106		int ret;
   107	
   108		if (!data->ap_mode_entry) {
   109			const struct typec_altmode *partner =
   110				typec_altmode_get_partner(alt);
   111			dev_warn(&partner->dev,
   112				 "EC does not support ap driven mode entry\n");
   113			return -EOPNOTSUPP;
   114		}
   115	
   116		ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
   117				  &req, sizeof(req), NULL, 0);
   118	
   119		if (ret < 0)
   120			return ret;
   121	
   122		svdm_version = typec_altmode_get_svdm_version(alt);
   123		if (svdm_version < 0)
   124			return svdm_version;
   125	
   126		data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
   127		data->header |= VDO_OPOS(data->mode);
   128		data->header |= VDO_CMDT(CMDT_RSP_ACK);
   129	
   130		data->vdo_data = NULL;
   131		data->vdo_size = 1;
   132	
   133		schedule_work(&data->work);
   134	
   135		return ret;
   136	}
   137	
   138	static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
   139					      const u32 *data, int count)
   140	{
   141		struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
   142	
   143		int cmd_type = PD_VDO_CMDT(header);
   144		int cmd = PD_VDO_CMD(header);
   145		int svdm_version;
   146	
   147		if (!adata->ap_mode_entry) {
   148			const struct typec_altmode *partner =
   149				typec_altmode_get_partner(alt);
   150			dev_warn(&partner->dev,
   151				 "EC does not support ap driven mode entry\n");
   152			return -EOPNOTSUPP;
   153		}
   154	
   155		svdm_version = typec_altmode_get_svdm_version(alt);
   156		if (svdm_version < 0)
   157			return svdm_version;
   158	
   159		switch (cmd_type) {
   160		case CMDT_INIT:
   161			if (PD_VDO_SVDM_VER(header) < svdm_version) {
   162				typec_partner_set_svdm_version(adata->port->partner,
   163							       PD_VDO_SVDM_VER(header));
   164				svdm_version = PD_VDO_SVDM_VER(header);
   165			}
   166	
   167			adata->header = VDO(adata->sid, 1, svdm_version, cmd);
   168			adata->header |= VDO_OPOS(adata->mode);
   169	
   170			/*
   171			 * DP_CMD_CONFIGURE: We can't actually do anything with the
   172			 * provided VDO yet so just send back an ACK.
   173			 *
   174			 * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
   175			 * DPStatus Acks.
   176			 */
   177			switch (cmd) {
   178			case DP_CMD_CONFIGURE:
   179				adata->am_data.dp.data.conf = *data;
   180				adata->header |= VDO_CMDT(CMDT_RSP_ACK);
   181				adata->am_data.dp.configured = true;
   182				schedule_work(&adata->work);
   183				break;
   184			case DP_CMD_STATUS_UPDATE:
   185				adata->am_data.dp.pending_status_update = true;
   186				break;
   187			default:
   188				adata->header |= VDO_CMDT(CMDT_RSP_ACK);
   189				schedule_work(&adata->work);
   190				break;
   191			}
   192	
   193			break;
   194		default:
   195			break;
   196		}
   197	
   198		return 0;
   199	}
   200	
   201	static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
   202					      const u32 *data, int count)
   203	{
   204		struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
   205	
   206		if (adata->sid == USB_TYPEC_DP_SID)
   207			return cros_typec_displayport_vdm(alt, header, data, count);
   208	
   209		return -EINVAL;
   210	}
   211	
 > 212	static const struct typec_altmode_ops cros_typec_altmode_ops = {
   213		.enter = cros_typec_altmode_enter,
   214		.exit = cros_typec_altmode_exit,
   215		.vdm = cros_typec_altmode_vdm,
   216	};
   217	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 5/7] platform/chrome: cros_ec_typec: Displayport support

[Dmitry Baryshkov]

On Wed, Oct 30, 2024 at 02:28:36PM -0700, Abhishek Pandit-Subedi wrote:
> Add support for entering and exiting displayport alt-mode on systems
> using AP driven alt-mode.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v2:
> - Refactored displayport into cros_typec_altmode.c to extract common
>   implementation between altmodes

Thanks!

> 
>  MAINTAINERS                                  |   3 +
>  drivers/platform/chrome/Makefile             |   3 +-
>  drivers/platform/chrome/cros_ec_typec.c      |  13 +-
>  drivers/platform/chrome/cros_ec_typec.h      |   1 +
>  drivers/platform/chrome/cros_typec_altmode.c | 277 +++++++++++++++++++
>  drivers/platform/chrome/cros_typec_altmode.h |  34 +++
>  6 files changed, 329 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/platform/chrome/cros_typec_altmode.c
>  create mode 100644 drivers/platform/chrome/cros_typec_altmode.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9659a5a7fb3..de99bcbda7d4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5369,9 +5369,12 @@ F:	include/linux/platform_data/cros_usbpd_notify.h
>  
>  CHROMEOS EC USB TYPE-C DRIVER
>  M:	Prashant Malani <pmalani@chromium.org>
> +M:	Benson Leung <bleung@chromium.org>
> +M:	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>  L:	chrome-platform@lists.linux.dev
>  S:	Maintained
>  F:	drivers/platform/chrome/cros_ec_typec.*
> +F:	drivers/platform/chrome/cros_typec_altmode.*
>  F:	drivers/platform/chrome/cros_typec_switch.c
>  F:	drivers/platform/chrome/cros_typec_vdm.*
>  
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 2dcc6ccc2302..8b007404c024 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -17,8 +17,9 @@ obj-$(CONFIG_CROS_EC_RPMSG)		+= cros_ec_rpmsg.o
>  obj-$(CONFIG_CROS_EC_SPI)		+= cros_ec_spi.o
>  obj-$(CONFIG_CROS_EC_UART)		+= cros_ec_uart.o
>  cros_ec_lpcs-objs			:= cros_ec_lpc.o cros_ec_lpc_mec.o
> -cros-ec-typec-objs			:= cros_ec_typec.o cros_typec_vdm.o
> +cros-ec-typec-objs			:= cros_ec_typec.o cros_typec_vdm.o cros_typec_altmode.o
>  obj-$(CONFIG_CROS_EC_TYPEC)		+= cros-ec-typec.o
> +
>  obj-$(CONFIG_CROS_EC_LPC)		+= cros_ec_lpcs.o
>  obj-$(CONFIG_CROS_EC_PROTO)		+= cros_ec_proto.o cros_ec_trace.o
>  obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)	+= cros_kbd_led_backlight.o
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 0c8db11bd8a4..7997e7136c4c 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -18,6 +18,7 @@
>  
>  #include "cros_ec_typec.h"
>  #include "cros_typec_vdm.h"
> +#include "cros_typec_altmode.h"
>  
>  #define DRV_NAME "cros-ec-typec"
>  
> @@ -293,12 +294,16 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
>  	desc.svid = USB_TYPEC_DP_SID;
>  	desc.mode = USB_TYPEC_DP_MODE;
>  	desc.vdo = DP_PORT_VDO;
> -	amode = typec_port_register_altmode(port->port, &desc);
> +	amode = cros_typec_register_displayport(port, &desc,
> +						typec->ap_driven_altmode);
>  	if (IS_ERR(amode))
>  		return PTR_ERR(amode);
>  	port->port_altmode[CROS_EC_ALTMODE_DP] = amode;
> +
> +#if !IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
>  	typec_altmode_set_drvdata(amode, port);
>  	amode->ops = &port_amode_ops;
> +#endif
>  
>  	/*
>  	 * Register TBT compatibility alt mode. The EC will not enter the mode
> @@ -575,6 +580,10 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec,
>  	if (!ret)
>  		ret = typec_mux_set(port->mux, &port->state);
>  
> +	if (!ret)
> +		cros_typec_displayport_status_update(port->state.alt,
> +						     port->state.data);
> +
>  	return ret;
>  }
>  
> @@ -1254,6 +1263,8 @@ static int cros_typec_probe(struct platform_device *pdev)
>  
>  	typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD);
>  	typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);
> +	typec->ap_driven_altmode = cros_ec_check_features(
> +		ec_dev, EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY);
>  
>  	ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
>  			  &resp, sizeof(resp));
> diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
> index deda180a646f..9fd5342bb0ad 100644
> --- a/drivers/platform/chrome/cros_ec_typec.h
> +++ b/drivers/platform/chrome/cros_ec_typec.h
> @@ -39,6 +39,7 @@ struct cros_typec_data {
>  	struct work_struct port_work;
>  	bool typec_cmd_supported;
>  	bool needs_mux_ack;
> +	bool ap_driven_altmode;
>  };
>  
>  /* Per port data. */
> diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> new file mode 100644
> index 000000000000..af2f077674f1
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Alt-mode implementation on ChromeOS EC.
> + *
> + * Copyright 2024 Google LLC
> + * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> + */
> +#include "cros_ec_typec.h"
> +
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/pd_vdo.h>
> +
> +#include "cros_typec_altmode.h"
> +
> +struct cros_typec_dp_data {
> +	struct typec_displayport_data data;
> +
> +	bool configured;
> +	bool pending_status_update;
> +};
> +
> +struct cros_typec_altmode_data {
> +	struct work_struct work;
> +	struct cros_typec_port *port;
> +	struct typec_altmode *alt;
> +	bool ap_mode_entry;
> +
> +	u32 header;
> +	u32 *vdo_data;
> +	u8 vdo_size;
> +
> +	u16 sid;
> +	u8 mode;
> +
> +	union am_specific {
> +		struct cros_typec_dp_data dp;
> +	} am_data;

Can this be done other way around?

struct cros_typec_dp_altmode_data {
  struct cros_typec_altmode_data base;
  struct cros_typec_dp_data dp;
};

> +};
> +
> +static void cros_typec_altmode_work(struct work_struct *work)
> +{
> +	struct cros_typec_altmode_data *data =
> +		container_of(work, struct cros_typec_altmode_data, work);
> +
> +	if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
> +			      data->vdo_size))
> +		dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
> +
> +	data->header = 0;
> +	data->vdo_data = NULL;
> +	data->vdo_size = 0;
> +}
> +
> +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
> +{
> +	struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> +	struct ec_params_typec_control req = {
> +		.port = data->port->port_num,
> +		.command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
> +	};
> +	int svdm_version;
> +	int ret;
> +
> +	if (!data->ap_mode_entry) {
> +		const struct typec_altmode *partner =
> +			typec_altmode_get_partner(alt);
> +		dev_warn(&partner->dev,
> +			 "EC does not support ap driven mode entry\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (data->sid == USB_TYPEC_DP_SID)
> +		req.mode_to_enter = CROS_EC_ALTMODE_DP;
> +	else
> +		return -EOPNOTSUPP;
> +
> +	ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> +			  &req, sizeof(req), NULL, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	svdm_version = typec_altmode_get_svdm_version(alt);
> +	if (svdm_version < 0)
> +		return svdm_version;
> +
> +	data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
> +	data->header |= VDO_OPOS(data->mode);
> +	data->header |= VDO_CMDT(CMDT_RSP_ACK);
> +
> +	data->vdo_data = NULL;
> +	data->vdo_size = 1;
> +
> +	schedule_work(&data->work);
> +
> +	return ret;
> +}
> +
> +static int cros_typec_altmode_exit(struct typec_altmode *alt)
> +{
> +	struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> +	struct ec_params_typec_control req = {
> +		.port = data->port->port_num,
> +		.command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
> +	};
> +	int svdm_version;
> +	int ret;
> +
> +	if (!data->ap_mode_entry) {
> +		const struct typec_altmode *partner =
> +			typec_altmode_get_partner(alt);
> +		dev_warn(&partner->dev,
> +			 "EC does not support ap driven mode entry\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> +			  &req, sizeof(req), NULL, 0);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	svdm_version = typec_altmode_get_svdm_version(alt);
> +	if (svdm_version < 0)
> +		return svdm_version;
> +
> +	data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
> +	data->header |= VDO_OPOS(data->mode);
> +	data->header |= VDO_CMDT(CMDT_RSP_ACK);
> +
> +	data->vdo_data = NULL;
> +	data->vdo_size = 1;
> +
> +	schedule_work(&data->work);
> +
> +	return ret;
> +}
> +
> +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> +				      const u32 *data, int count)
> +{
> +	struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> +
> +	int cmd_type = PD_VDO_CMDT(header);
> +	int cmd = PD_VDO_CMD(header);
> +	int svdm_version;
> +
> +	if (!adata->ap_mode_entry) {
> +		const struct typec_altmode *partner =
> +			typec_altmode_get_partner(alt);
> +		dev_warn(&partner->dev,
> +			 "EC does not support ap driven mode entry\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	svdm_version = typec_altmode_get_svdm_version(alt);
> +	if (svdm_version < 0)
> +		return svdm_version;
> +
> +	switch (cmd_type) {
> +	case CMDT_INIT:
> +		if (PD_VDO_SVDM_VER(header) < svdm_version) {
> +			typec_partner_set_svdm_version(adata->port->partner,
> +						       PD_VDO_SVDM_VER(header));
> +			svdm_version = PD_VDO_SVDM_VER(header);
> +		}
> +
> +		adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> +		adata->header |= VDO_OPOS(adata->mode);
> +
> +		/*
> +		 * DP_CMD_CONFIGURE: We can't actually do anything with the
> +		 * provided VDO yet so just send back an ACK.
> +		 *
> +		 * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
> +		 * DPStatus Acks.
> +		 */
> +		switch (cmd) {
> +		case DP_CMD_CONFIGURE:
> +			adata->am_data.dp.data.conf = *data;
> +			adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> +			adata->am_data.dp.configured = true;
> +			schedule_work(&adata->work);
> +			break;
> +		case DP_CMD_STATUS_UPDATE:
> +			adata->am_data.dp.pending_status_update = true;
> +			break;
> +		default:
> +			adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> +			schedule_work(&adata->work);
> +			break;
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> +				      const u32 *data, int count)
> +{
> +	struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> +
> +	if (adata->sid == USB_TYPEC_DP_SID)
> +		return cros_typec_displayport_vdm(alt, header, data, count);
> +
> +	return -EINVAL;
> +}
> +
> +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> +	.enter = cros_typec_altmode_enter,
> +	.exit = cros_typec_altmode_exit,
> +	.vdm = cros_typec_altmode_vdm,
> +};
> +
> +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> +					 struct typec_displayport_data *data)
> +{
> +	struct cros_typec_altmode_data *adata =
> +		typec_altmode_get_drvdata(altmode);
> +
> +	if (!adata->am_data.dp.pending_status_update) {
> +		dev_dbg(&altmode->dev,
> +			"Got DPStatus without a pending request");
> +		return 0;
> +	}
> +
> +	if (adata->am_data.dp.configured && adata->am_data.dp.data.conf != data->conf)
> +		dev_dbg(&altmode->dev,
> +			"DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> +			adata->am_data.dp.data.conf, data->conf);
> +
> +	adata->am_data.dp.data = *data;
> +	adata->am_data.dp.pending_status_update = false;
> +	adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> +	adata->vdo_data = &adata->am_data.dp.data.status;
> +	adata->vdo_size = 2;
> +
> +	schedule_work(&adata->work);
> +	return 0;
> +}
> +
> +struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc,
> +				bool ap_mode_entry)
> +{
> +	struct typec_altmode *alt;
> +	struct cros_typec_altmode_data *data;
> +
> +	alt = typec_port_register_altmode(port->port, desc);
> +	if (IS_ERR(alt))
> +		return alt;
> +
> +	data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data) {
> +		typec_unregister_altmode(alt);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	INIT_WORK(&data->work, cros_typec_altmode_work);
> +	data->alt = alt;
> +	data->port = port;
> +	data->ap_mode_entry = ap_mode_entry;
> +	data->sid = USB_TYPEC_DP_SID;
> +	data->mode = USB_TYPEC_DP_MODE;
> +
> +	data->am_data.dp.configured = false;
> +	typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> +	typec_altmode_set_drvdata(alt, data);
> +
> +	return alt;
> +}
> +#endif
> diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
> new file mode 100644
> index 000000000000..c6f8fb02c99c
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __CROS_TYPEC_ALTMODE_H__
> +#define __CROS_TYPEC_ALTMODE_H__
> +
> +struct cros_typec_port;
> +struct typec_altmode;
> +struct typec_altmode_desc;
> +struct typec_displayport_data;
> +
> +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> +struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc,
> +				bool ap_mode_entry);
> +
> +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> +					 struct typec_displayport_data *data);
> +#else
> +static inline struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc,
> +				bool ap_mode_entry)
> +{
> +	return typec_port_register_altmode(port->port, desc);
> +}
> +
> +static inline int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> +					 struct typec_displayport_data *data)
> +{
> +	return 0;
> +}
> +#endif
> +#endif /* __CROS_TYPEC_ALTMODE_H__ */
> -- 
> 2.47.0.163.g1226f6d8fa-goog
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 5/7] platform/chrome: cros_ec_typec: Displayport support

[Abhishek Pandit-Subedi]

On Thu, Oct 31, 2024 at 11:54 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Oct 30, 2024 at 02:28:36PM -0700, Abhishek Pandit-Subedi wrote:
> > Add support for entering and exiting displayport alt-mode on systems
> > using AP driven alt-mode.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Refactored displayport into cros_typec_altmode.c to extract common
> >   implementation between altmodes
>
> Thanks!
>
> >
> >  MAINTAINERS                                  |   3 +
> >  drivers/platform/chrome/Makefile             |   3 +-
> >  drivers/platform/chrome/cros_ec_typec.c      |  13 +-
> >  drivers/platform/chrome/cros_ec_typec.h      |   1 +
> >  drivers/platform/chrome/cros_typec_altmode.c | 277 +++++++++++++++++++
> >  drivers/platform/chrome/cros_typec_altmode.h |  34 +++
> >  6 files changed, 329 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/platform/chrome/cros_typec_altmode.c
> >  create mode 100644 drivers/platform/chrome/cros_typec_altmode.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e9659a5a7fb3..de99bcbda7d4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5369,9 +5369,12 @@ F:     include/linux/platform_data/cros_usbpd_notify.h
> >
> >  CHROMEOS EC USB TYPE-C DRIVER
> >  M:   Prashant Malani <pmalani@chromium.org>
> > +M:   Benson Leung <bleung@chromium.org>
> > +M:   Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >  L:   chrome-platform@lists.linux.dev
> >  S:   Maintained
> >  F:   drivers/platform/chrome/cros_ec_typec.*
> > +F:   drivers/platform/chrome/cros_typec_altmode.*
> >  F:   drivers/platform/chrome/cros_typec_switch.c
> >  F:   drivers/platform/chrome/cros_typec_vdm.*
> >
> > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > index 2dcc6ccc2302..8b007404c024 100644
> > --- a/drivers/platform/chrome/Makefile
> > +++ b/drivers/platform/chrome/Makefile
> > @@ -17,8 +17,9 @@ obj-$(CONFIG_CROS_EC_RPMSG)         += cros_ec_rpmsg.o
> >  obj-$(CONFIG_CROS_EC_SPI)            += cros_ec_spi.o
> >  obj-$(CONFIG_CROS_EC_UART)           += cros_ec_uart.o
> >  cros_ec_lpcs-objs                    := cros_ec_lpc.o cros_ec_lpc_mec.o
> > -cros-ec-typec-objs                   := cros_ec_typec.o cros_typec_vdm.o
> > +cros-ec-typec-objs                   := cros_ec_typec.o cros_typec_vdm.o cros_typec_altmode.o
> >  obj-$(CONFIG_CROS_EC_TYPEC)          += cros-ec-typec.o
> > +
> >  obj-$(CONFIG_CROS_EC_LPC)            += cros_ec_lpcs.o
> >  obj-$(CONFIG_CROS_EC_PROTO)          += cros_ec_proto.o cros_ec_trace.o
> >  obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 0c8db11bd8a4..7997e7136c4c 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -18,6 +18,7 @@
> >
> >  #include "cros_ec_typec.h"
> >  #include "cros_typec_vdm.h"
> > +#include "cros_typec_altmode.h"
> >
> >  #define DRV_NAME "cros-ec-typec"
> >
> > @@ -293,12 +294,16 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> >       desc.svid = USB_TYPEC_DP_SID;
> >       desc.mode = USB_TYPEC_DP_MODE;
> >       desc.vdo = DP_PORT_VDO;
> > -     amode = typec_port_register_altmode(port->port, &desc);
> > +     amode = cros_typec_register_displayport(port, &desc,
> > +                                             typec->ap_driven_altmode);
> >       if (IS_ERR(amode))
> >               return PTR_ERR(amode);
> >       port->port_altmode[CROS_EC_ALTMODE_DP] = amode;
> > +
> > +#if !IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> >       typec_altmode_set_drvdata(amode, port);
> >       amode->ops = &port_amode_ops;
> > +#endif
> >
> >       /*
> >        * Register TBT compatibility alt mode. The EC will not enter the mode
> > @@ -575,6 +580,10 @@ static int cros_typec_enable_dp(struct cros_typec_data *typec,
> >       if (!ret)
> >               ret = typec_mux_set(port->mux, &port->state);
> >
> > +     if (!ret)
> > +             cros_typec_displayport_status_update(port->state.alt,
> > +                                                  port->state.data);
> > +
> >       return ret;
> >  }
> >
> > @@ -1254,6 +1263,8 @@ static int cros_typec_probe(struct platform_device *pdev)
> >
> >       typec->typec_cmd_supported = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_CMD);
> >       typec->needs_mux_ack = cros_ec_check_features(ec_dev, EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK);
> > +     typec->ap_driven_altmode = cros_ec_check_features(
> > +             ec_dev, EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY);
> >
> >       ret = cros_ec_cmd(typec->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> >                         &resp, sizeof(resp));
> > diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
> > index deda180a646f..9fd5342bb0ad 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.h
> > +++ b/drivers/platform/chrome/cros_ec_typec.h
> > @@ -39,6 +39,7 @@ struct cros_typec_data {
> >       struct work_struct port_work;
> >       bool typec_cmd_supported;
> >       bool needs_mux_ack;
> > +     bool ap_driven_altmode;
> >  };
> >
> >  /* Per port data. */
> > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> > new file mode 100644
> > index 000000000000..af2f077674f1
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > @@ -0,0 +1,277 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Alt-mode implementation on ChromeOS EC.
> > + *
> > + * Copyright 2024 Google LLC
> > + * Author: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > + */
> > +#include "cros_ec_typec.h"
> > +
> > +#include <linux/usb/typec_dp.h>
> > +#include <linux/usb/pd_vdo.h>
> > +
> > +#include "cros_typec_altmode.h"
> > +
> > +struct cros_typec_dp_data {
> > +     struct typec_displayport_data data;
> > +
> > +     bool configured;
> > +     bool pending_status_update;
> > +};
> > +
> > +struct cros_typec_altmode_data {
> > +     struct work_struct work;
> > +     struct cros_typec_port *port;
> > +     struct typec_altmode *alt;
> > +     bool ap_mode_entry;
> > +
> > +     u32 header;
> > +     u32 *vdo_data;
> > +     u8 vdo_size;
> > +
> > +     u16 sid;
> > +     u8 mode;
> > +
> > +     union am_specific {
> > +             struct cros_typec_dp_data dp;
> > +     } am_data;
>
> Can this be done other way around?
>
> struct cros_typec_dp_altmode_data {
>   struct cros_typec_altmode_data base;
>   struct cros_typec_dp_data dp;
> };

Ack -- expect in the next patch series.

>
> > +};
> > +
> > +static void cros_typec_altmode_work(struct work_struct *work)
> > +{
> > +     struct cros_typec_altmode_data *data =
> > +             container_of(work, struct cros_typec_altmode_data, work);
> > +
> > +     if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
> > +                           data->vdo_size))
> > +             dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
> > +
> > +     data->header = 0;
> > +     data->vdo_data = NULL;
> > +     data->vdo_size = 0;
> > +}
> > +
> > +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
> > +{
> > +     struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> > +     struct ec_params_typec_control req = {
> > +             .port = data->port->port_num,
> > +             .command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
> > +     };
> > +     int svdm_version;
> > +     int ret;
> > +
> > +     if (!data->ap_mode_entry) {
> > +             const struct typec_altmode *partner =
> > +                     typec_altmode_get_partner(alt);
> > +             dev_warn(&partner->dev,
> > +                      "EC does not support ap driven mode entry\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     if (data->sid == USB_TYPEC_DP_SID)
> > +             req.mode_to_enter = CROS_EC_ALTMODE_DP;
> > +     else
> > +             return -EOPNOTSUPP;
> > +
> > +     ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> > +                       &req, sizeof(req), NULL, 0);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     svdm_version = typec_altmode_get_svdm_version(alt);
> > +     if (svdm_version < 0)
> > +             return svdm_version;
> > +
> > +     data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
> > +     data->header |= VDO_OPOS(data->mode);
> > +     data->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +
> > +     data->vdo_data = NULL;
> > +     data->vdo_size = 1;
> > +
> > +     schedule_work(&data->work);
> > +
> > +     return ret;
> > +}
> > +
> > +static int cros_typec_altmode_exit(struct typec_altmode *alt)
> > +{
> > +     struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> > +     struct ec_params_typec_control req = {
> > +             .port = data->port->port_num,
> > +             .command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
> > +     };
> > +     int svdm_version;
> > +     int ret;
> > +
> > +     if (!data->ap_mode_entry) {
> > +             const struct typec_altmode *partner =
> > +                     typec_altmode_get_partner(alt);
> > +             dev_warn(&partner->dev,
> > +                      "EC does not support ap driven mode entry\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> > +                       &req, sizeof(req), NULL, 0);
> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     svdm_version = typec_altmode_get_svdm_version(alt);
> > +     if (svdm_version < 0)
> > +             return svdm_version;
> > +
> > +     data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
> > +     data->header |= VDO_OPOS(data->mode);
> > +     data->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +
> > +     data->vdo_data = NULL;
> > +     data->vdo_size = 1;
> > +
> > +     schedule_work(&data->work);
> > +
> > +     return ret;
> > +}
> > +
> > +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> > +                                   const u32 *data, int count)
> > +{
> > +     struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> > +
> > +     int cmd_type = PD_VDO_CMDT(header);
> > +     int cmd = PD_VDO_CMD(header);
> > +     int svdm_version;
> > +
> > +     if (!adata->ap_mode_entry) {
> > +             const struct typec_altmode *partner =
> > +                     typec_altmode_get_partner(alt);
> > +             dev_warn(&partner->dev,
> > +                      "EC does not support ap driven mode entry\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     svdm_version = typec_altmode_get_svdm_version(alt);
> > +     if (svdm_version < 0)
> > +             return svdm_version;
> > +
> > +     switch (cmd_type) {
> > +     case CMDT_INIT:
> > +             if (PD_VDO_SVDM_VER(header) < svdm_version) {
> > +                     typec_partner_set_svdm_version(adata->port->partner,
> > +                                                    PD_VDO_SVDM_VER(header));
> > +                     svdm_version = PD_VDO_SVDM_VER(header);
> > +             }
> > +
> > +             adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> > +             adata->header |= VDO_OPOS(adata->mode);
> > +
> > +             /*
> > +              * DP_CMD_CONFIGURE: We can't actually do anything with the
> > +              * provided VDO yet so just send back an ACK.
> > +              *
> > +              * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
> > +              * DPStatus Acks.
> > +              */
> > +             switch (cmd) {
> > +             case DP_CMD_CONFIGURE:
> > +                     adata->am_data.dp.data.conf = *data;
> > +                     adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +                     adata->am_data.dp.configured = true;
> > +                     schedule_work(&adata->work);
> > +                     break;
> > +             case DP_CMD_STATUS_UPDATE:
> > +                     adata->am_data.dp.pending_status_update = true;
> > +                     break;
> > +             default:
> > +                     adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +                     schedule_work(&adata->work);
> > +                     break;
> > +             }
> > +
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> > +                                   const u32 *data, int count)
> > +{
> > +     struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> > +
> > +     if (adata->sid == USB_TYPEC_DP_SID)
> > +             return cros_typec_displayport_vdm(alt, header, data, count);
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> > +     .enter = cros_typec_altmode_enter,
> > +     .exit = cros_typec_altmode_exit,
> > +     .vdm = cros_typec_altmode_vdm,
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> > +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> > +                                      struct typec_displayport_data *data)
> > +{
> > +     struct cros_typec_altmode_data *adata =
> > +             typec_altmode_get_drvdata(altmode);
> > +
> > +     if (!adata->am_data.dp.pending_status_update) {
> > +             dev_dbg(&altmode->dev,
> > +                     "Got DPStatus without a pending request");
> > +             return 0;
> > +     }
> > +
> > +     if (adata->am_data.dp.configured && adata->am_data.dp.data.conf != data->conf)
> > +             dev_dbg(&altmode->dev,
> > +                     "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> > +                     adata->am_data.dp.data.conf, data->conf);
> > +
> > +     adata->am_data.dp.data = *data;
> > +     adata->am_data.dp.pending_status_update = false;
> > +     adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +     adata->vdo_data = &adata->am_data.dp.data.status;
> > +     adata->vdo_size = 2;
> > +
> > +     schedule_work(&adata->work);
> > +     return 0;
> > +}
> > +
> > +struct typec_altmode *
> > +cros_typec_register_displayport(struct cros_typec_port *port,
> > +                             struct typec_altmode_desc *desc,
> > +                             bool ap_mode_entry)
> > +{
> > +     struct typec_altmode *alt;
> > +     struct cros_typec_altmode_data *data;
> > +
> > +     alt = typec_port_register_altmode(port->port, desc);
> > +     if (IS_ERR(alt))
> > +             return alt;
> > +
> > +     data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data) {
> > +             typec_unregister_altmode(alt);
> > +             return ERR_PTR(-ENOMEM);
> > +     }
> > +
> > +     INIT_WORK(&data->work, cros_typec_altmode_work);
> > +     data->alt = alt;
> > +     data->port = port;
> > +     data->ap_mode_entry = ap_mode_entry;
> > +     data->sid = USB_TYPEC_DP_SID;
> > +     data->mode = USB_TYPEC_DP_MODE;
> > +
> > +     data->am_data.dp.configured = false;
> > +     typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> > +     typec_altmode_set_drvdata(alt, data);
> > +
> > +     return alt;
> > +}
> > +#endif
> > diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
> > new file mode 100644
> > index 000000000000..c6f8fb02c99c
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_typec_altmode.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef __CROS_TYPEC_ALTMODE_H__
> > +#define __CROS_TYPEC_ALTMODE_H__
> > +
> > +struct cros_typec_port;
> > +struct typec_altmode;
> > +struct typec_altmode_desc;
> > +struct typec_displayport_data;
> > +
> > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> > +struct typec_altmode *
> > +cros_typec_register_displayport(struct cros_typec_port *port,
> > +                             struct typec_altmode_desc *desc,
> > +                             bool ap_mode_entry);
> > +
> > +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> > +                                      struct typec_displayport_data *data);
> > +#else
> > +static inline struct typec_altmode *
> > +cros_typec_register_displayport(struct cros_typec_port *port,
> > +                             struct typec_altmode_desc *desc,
> > +                             bool ap_mode_entry)
> > +{
> > +     return typec_port_register_altmode(port->port, desc);
> > +}
> > +
> > +static inline int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> > +                                      struct typec_displayport_data *data)
> > +{
> > +     return 0;
> > +}
> > +#endif
> > +#endif /* __CROS_TYPEC_ALTMODE_H__ */
> > --
> > 2.47.0.163.g1226f6d8fa-goog
> >
>
> --
> With best wishes
> Dmitry

Re: [PATCH v2 5/7] platform/chrome: cros_ec_typec: Displayport support

[kernel test robot]

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-20241031]
[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.5.I142fc0c09df58689b98f0cebf1c5e48b9d4fa800%40changeid
patch subject: [PATCH v2 5/7] platform/chrome: cros_ec_typec: Displayport support
config: arm-multi_v7_defconfig (https://download.01.org/0day-ci/archive/20241101/202411010346.AqbfMqLc-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411010346.AqbfMqLc-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/202411010346.AqbfMqLc-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/platform/chrome/cros_typec_altmode.c:212:39: warning: 'cros_typec_altmode_ops' defined but not used [-Wunused-const-variable=]
     212 | static const struct typec_altmode_ops cros_typec_altmode_ops = {
         |                                       ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/platform/chrome/cros_typec_altmode.c:40:13: warning: 'cros_typec_altmode_work' defined but not used [-Wunused-function]
      40 | static void cros_typec_altmode_work(struct work_struct *work)
         |             ^~~~~~~~~~~~~~~~~~~~~~~


vim +/cros_typec_altmode_ops +212 drivers/platform/chrome/cros_typec_altmode.c

    39	
  > 40	static void cros_typec_altmode_work(struct work_struct *work)
    41	{
    42		struct cros_typec_altmode_data *data =
    43			container_of(work, struct cros_typec_altmode_data, work);
    44	
    45		if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
    46				      data->vdo_size))
    47			dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
    48	
    49		data->header = 0;
    50		data->vdo_data = NULL;
    51		data->vdo_size = 0;
    52	}
    53	
    54	static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
    55	{
    56		struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
    57		struct ec_params_typec_control req = {
    58			.port = data->port->port_num,
    59			.command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
    60		};
    61		int svdm_version;
    62		int ret;
    63	
    64		if (!data->ap_mode_entry) {
    65			const struct typec_altmode *partner =
    66				typec_altmode_get_partner(alt);
    67			dev_warn(&partner->dev,
    68				 "EC does not support ap driven mode entry\n");
    69			return -EOPNOTSUPP;
    70		}
    71	
    72		if (data->sid == USB_TYPEC_DP_SID)
    73			req.mode_to_enter = CROS_EC_ALTMODE_DP;
    74		else
    75			return -EOPNOTSUPP;
    76	
    77		ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
    78				  &req, sizeof(req), NULL, 0);
    79		if (ret < 0)
    80			return ret;
    81	
    82		svdm_version = typec_altmode_get_svdm_version(alt);
    83		if (svdm_version < 0)
    84			return svdm_version;
    85	
    86		data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
    87		data->header |= VDO_OPOS(data->mode);
    88		data->header |= VDO_CMDT(CMDT_RSP_ACK);
    89	
    90		data->vdo_data = NULL;
    91		data->vdo_size = 1;
    92	
    93		schedule_work(&data->work);
    94	
    95		return ret;
    96	}
    97	
    98	static int cros_typec_altmode_exit(struct typec_altmode *alt)
    99	{
   100		struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
   101		struct ec_params_typec_control req = {
   102			.port = data->port->port_num,
   103			.command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
   104		};
   105		int svdm_version;
   106		int ret;
   107	
   108		if (!data->ap_mode_entry) {
   109			const struct typec_altmode *partner =
   110				typec_altmode_get_partner(alt);
   111			dev_warn(&partner->dev,
   112				 "EC does not support ap driven mode entry\n");
   113			return -EOPNOTSUPP;
   114		}
   115	
   116		ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
   117				  &req, sizeof(req), NULL, 0);
   118	
   119		if (ret < 0)
   120			return ret;
   121	
   122		svdm_version = typec_altmode_get_svdm_version(alt);
   123		if (svdm_version < 0)
   124			return svdm_version;
   125	
   126		data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
   127		data->header |= VDO_OPOS(data->mode);
   128		data->header |= VDO_CMDT(CMDT_RSP_ACK);
   129	
   130		data->vdo_data = NULL;
   131		data->vdo_size = 1;
   132	
   133		schedule_work(&data->work);
   134	
   135		return ret;
   136	}
   137	
   138	static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
   139					      const u32 *data, int count)
   140	{
   141		struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
   142	
   143		int cmd_type = PD_VDO_CMDT(header);
   144		int cmd = PD_VDO_CMD(header);
   145		int svdm_version;
   146	
   147		if (!adata->ap_mode_entry) {
   148			const struct typec_altmode *partner =
   149				typec_altmode_get_partner(alt);
   150			dev_warn(&partner->dev,
   151				 "EC does not support ap driven mode entry\n");
   152			return -EOPNOTSUPP;
   153		}
   154	
   155		svdm_version = typec_altmode_get_svdm_version(alt);
   156		if (svdm_version < 0)
   157			return svdm_version;
   158	
   159		switch (cmd_type) {
   160		case CMDT_INIT:
   161			if (PD_VDO_SVDM_VER(header) < svdm_version) {
   162				typec_partner_set_svdm_version(adata->port->partner,
   163							       PD_VDO_SVDM_VER(header));
   164				svdm_version = PD_VDO_SVDM_VER(header);
   165			}
   166	
   167			adata->header = VDO(adata->sid, 1, svdm_version, cmd);
   168			adata->header |= VDO_OPOS(adata->mode);
   169	
   170			/*
   171			 * DP_CMD_CONFIGURE: We can't actually do anything with the
   172			 * provided VDO yet so just send back an ACK.
   173			 *
   174			 * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
   175			 * DPStatus Acks.
   176			 */
   177			switch (cmd) {
   178			case DP_CMD_CONFIGURE:
   179				adata->am_data.dp.data.conf = *data;
   180				adata->header |= VDO_CMDT(CMDT_RSP_ACK);
   181				adata->am_data.dp.configured = true;
   182				schedule_work(&adata->work);
   183				break;
   184			case DP_CMD_STATUS_UPDATE:
   185				adata->am_data.dp.pending_status_update = true;
   186				break;
   187			default:
   188				adata->header |= VDO_CMDT(CMDT_RSP_ACK);
   189				schedule_work(&adata->work);
   190				break;
   191			}
   192	
   193			break;
   194		default:
   195			break;
   196		}
   197	
   198		return 0;
   199	}
   200	
   201	static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
   202					      const u32 *data, int count)
   203	{
   204		struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
   205	
   206		if (adata->sid == USB_TYPEC_DP_SID)
   207			return cros_typec_displayport_vdm(alt, header, data, count);
   208	
   209		return -EINVAL;
   210	}
   211	
 > 212	static const struct typec_altmode_ops cros_typec_altmode_ops = {
   213		.enter = cros_typec_altmode_enter,
   214		.exit = cros_typec_altmode_exit,
   215		.vdm = cros_typec_altmode_vdm,
   216	};
   217	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 6/7] platform/chrome: cros_ec_typec: Thunderbolt support

[Abhishek Pandit-Subedi]

Add support for entering and exiting Thunderbolt alt-mode using AP
driven alt-mode.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
- Refactored thunderbolt support into cros_typec_altmode.c

 drivers/platform/chrome/cros_ec_typec.c      | 29 ++++---
 drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
 drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
 3 files changed, 116 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 7997e7136c4c..3e043b1c1cc8 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -304,21 +304,26 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
 	typec_altmode_set_drvdata(amode, port);
 	amode->ops = &port_amode_ops;
 #endif
-
 	/*
 	 * Register TBT compatibility alt mode. The EC will not enter the mode
-	 * if it doesn't support it, so it's safe to register it unconditionally
-	 * here for now.
+	 * if it doesn't support it and it will not enter automatically by
+	 * design so we can use the |ap_driven_altmode| feature to check if we
+	 * should register it.
 	 */
-	memset(&desc, 0, sizeof(desc));
-	desc.svid = USB_TYPEC_TBT_SID;
-	desc.mode = TYPEC_ANY_MODE;
-	amode = typec_port_register_altmode(port->port, &desc);
-	if (IS_ERR(amode))
-		return PTR_ERR(amode);
-	port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
-	typec_altmode_set_drvdata(amode, port);
-	amode->ops = &port_amode_ops;
+	if (typec->ap_driven_altmode) {
+		memset(&desc, 0, sizeof(desc));
+		desc.svid = USB_TYPEC_TBT_SID;
+		desc.mode = TYPEC_ANY_MODE;
+		amode = cros_typec_register_thunderbolt(port, &desc);
+		if (IS_ERR(amode))
+			return PTR_ERR(amode);
+		port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
+
+#if !IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
+		typec_altmode_set_drvdata(amode, port);
+		amode->ops = &port_amode_ops;
+#endif
+	}
 
 	port->state.alt = NULL;
 	port->state.mode = TYPEC_STATE_USB;
diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
index af2f077674f1..6cb1e1320d6c 100644
--- a/drivers/platform/chrome/cros_typec_altmode.c
+++ b/drivers/platform/chrome/cros_typec_altmode.c
@@ -8,6 +8,7 @@
 #include "cros_ec_typec.h"
 
 #include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_tbt.h>
 #include <linux/usb/pd_vdo.h>
 
 #include "cros_typec_altmode.h"
@@ -71,6 +72,8 @@ static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
 
 	if (data->sid == USB_TYPEC_DP_SID)
 		req.mode_to_enter = CROS_EC_ALTMODE_DP;
+	else if (data->sid == USB_TYPEC_TBT_SID)
+		req.mode_to_enter = CROS_EC_ALTMODE_TBT;
 	else
 		return -EOPNOTSUPP;
 
@@ -198,6 +201,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
 	return 0;
 }
 
+static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
+				      const u32 *data, int count)
+{
+	struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
+
+	int cmd_type = PD_VDO_CMDT(header);
+	int cmd = PD_VDO_CMD(header);
+	int svdm_version;
+
+	svdm_version = typec_altmode_get_svdm_version(alt);
+	if (svdm_version < 0)
+		return svdm_version;
+
+	switch (cmd_type) {
+	case CMDT_INIT:
+		if (PD_VDO_SVDM_VER(header) < svdm_version) {
+			typec_partner_set_svdm_version(adata->port->partner,
+						       PD_VDO_SVDM_VER(header));
+			svdm_version = PD_VDO_SVDM_VER(header);
+		}
+
+		adata->header = VDO(USB_TYPEC_TBT_SID, 1, svdm_version, cmd);
+		adata->header |= VDO_OPOS(USB_TYPEC_TBT_MODE);
+
+		switch (cmd) {
+		case CMD_ENTER_MODE:
+			/* Don't respond to the enter mode vdm because it
+			 * triggers mux configuration. This is handled directly
+			 * by the cros_ec_typec driver so the Thunderbolt driver
+			 * doesn't need to be involved.
+			 */
+			break;
+		default:
+			adata->header |= VDO_CMDT(CMDT_RSP_ACK);
+			schedule_work(&adata->work);
+			break;
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+
 static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
 				      const u32 *data, int count)
 {
@@ -206,6 +256,9 @@ static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
 	if (adata->sid == USB_TYPEC_DP_SID)
 		return cros_typec_displayport_vdm(alt, header, data, count);
 
+	if (adata->sid == USB_TYPEC_TBT_SID)
+		return cros_typec_thunderbolt_vdm(alt, header, data, count);
+
 	return -EINVAL;
 }
 
@@ -275,3 +328,35 @@ cros_typec_register_displayport(struct cros_typec_port *port,
 	return alt;
 }
 #endif
+
+#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
+struct typec_altmode *
+cros_typec_register_thunderbolt(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc)
+{
+	struct typec_altmode *alt;
+	struct cros_typec_altmode_data *data;
+
+	alt = typec_port_register_altmode(port->port, desc);
+	if (IS_ERR(alt))
+		return alt;
+
+	data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		typec_unregister_altmode(alt);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	INIT_WORK(&data->work, cros_typec_altmode_work);
+	data->alt = alt;
+	data->port = port;
+	data->ap_mode_entry = true;
+	data->sid = USB_TYPEC_TBT_SID;
+	data->mode = USB_TYPEC_TBT_MODE;
+
+	typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
+	typec_altmode_set_drvdata(alt, data);
+
+	return alt;
+}
+#endif
diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
index c6f8fb02c99c..c71568314e3f 100644
--- a/drivers/platform/chrome/cros_typec_altmode.h
+++ b/drivers/platform/chrome/cros_typec_altmode.h
@@ -31,4 +31,18 @@ static inline int cros_typec_displayport_status_update(struct typec_altmode *alt
 	return 0;
 }
 #endif
+
+#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
+struct typec_altmode *
+cros_typec_register_thunderbolt(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc);
+#else
+struct typec_altmode *
+cros_typec_register_thunderbolt(struct cros_typec_port *port,
+				struct typec_altmode_desc *desc)
+{
+	return typec_port_register_altmode(port->port, desc);
+}
+#endif
+
 #endif /* __CROS_TYPEC_ALTMODE_H__ */
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v2 6/7] platform/chrome: cros_ec_typec: Thunderbolt support

[kernel test robot]

Hi Abhishek,

kernel test robot noticed the following build errors:

[auto build test ERROR on chrome-platform/for-next]
[also build test ERROR on chrome-platform/for-firmware-next usb/usb-testing usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v6.12-rc5 next-20241031]
[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.6.Ic61ced3cdfb5d6776435356061f12307da719829%40changeid
patch subject: [PATCH v2 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20241101/202411010039.QHl0lhBw-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411010039.QHl0lhBw-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/202411010039.QHl0lhBw-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from drivers/platform/chrome/cros_ec_typec.c:21:
>> drivers/platform/chrome/cros_typec_altmode.h:41:1: warning: no previous prototype for 'cros_typec_register_thunderbolt' [-Wmissing-prototypes]
      41 | cros_typec_register_thunderbolt(struct cros_typec_port *port,
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   aarch64-linux-ld: drivers/platform/chrome/cros_typec_altmode.o: in function `cros_typec_register_thunderbolt':
>> drivers/platform/chrome/cros_typec_altmode.h:43: multiple definition of `cros_typec_register_thunderbolt'; drivers/platform/chrome/cros_ec_typec.o:drivers/platform/chrome/cros_typec_altmode.h:43: first defined here


vim +43 drivers/platform/chrome/cros_typec_altmode.h

    34	
    35	#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
    36	struct typec_altmode *
    37	cros_typec_register_thunderbolt(struct cros_typec_port *port,
    38					struct typec_altmode_desc *desc);
    39	#else
    40	struct typec_altmode *
  > 41	cros_typec_register_thunderbolt(struct cros_typec_port *port,
    42					struct typec_altmode_desc *desc)
  > 43	{
    44		return typec_port_register_altmode(port->port, desc);
    45	}
    46	#endif
    47	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 6/7] platform/chrome: cros_ec_typec: Thunderbolt support

[Dmitry Baryshkov]

On Wed, Oct 30, 2024 at 02:28:37PM -0700, Abhishek Pandit-Subedi wrote:
> Add support for entering and exiting Thunderbolt alt-mode using AP
> driven alt-mode.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v2:
> - Refactored thunderbolt support into cros_typec_altmode.c
> 
>  drivers/platform/chrome/cros_ec_typec.c      | 29 ++++---
>  drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
>  drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
>  3 files changed, 116 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 7997e7136c4c..3e043b1c1cc8 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -304,21 +304,26 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
>  	typec_altmode_set_drvdata(amode, port);
>  	amode->ops = &port_amode_ops;
>  #endif
> -
>  	/*
>  	 * Register TBT compatibility alt mode. The EC will not enter the mode
> -	 * if it doesn't support it, so it's safe to register it unconditionally
> -	 * here for now.
> +	 * if it doesn't support it and it will not enter automatically by
> +	 * design so we can use the |ap_driven_altmode| feature to check if we
> +	 * should register it.
>  	 */
> -	memset(&desc, 0, sizeof(desc));
> -	desc.svid = USB_TYPEC_TBT_SID;
> -	desc.mode = TYPEC_ANY_MODE;
> -	amode = typec_port_register_altmode(port->port, &desc);
> -	if (IS_ERR(amode))
> -		return PTR_ERR(amode);
> -	port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
> -	typec_altmode_set_drvdata(amode, port);
> -	amode->ops = &port_amode_ops;
> +	if (typec->ap_driven_altmode) {
> +		memset(&desc, 0, sizeof(desc));
> +		desc.svid = USB_TYPEC_TBT_SID;
> +		desc.mode = TYPEC_ANY_MODE;
> +		amode = cros_typec_register_thunderbolt(port, &desc);
> +		if (IS_ERR(amode))
> +			return PTR_ERR(amode);
> +		port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
> +
> +#if !IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
> +		typec_altmode_set_drvdata(amode, port);
> +		amode->ops = &port_amode_ops;
> +#endif

Why? Usually having the code block under an #if is a frowned upon
practice.

> +	}
>  
>  	port->state.alt = NULL;
>  	port->state.mode = TYPEC_STATE_USB;

[...]

> diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
> index c6f8fb02c99c..c71568314e3f 100644
> --- a/drivers/platform/chrome/cros_typec_altmode.h
> +++ b/drivers/platform/chrome/cros_typec_altmode.h
> @@ -31,4 +31,18 @@ static inline int cros_typec_displayport_status_update(struct typec_altmode *alt
>  	return 0;
>  }
>  #endif
> +
> +#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
> +struct typec_altmode *
> +cros_typec_register_thunderbolt(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc);
> +#else
> +struct typec_altmode *

static inline struct ...
LGTM otherwise

> +cros_typec_register_thunderbolt(struct cros_typec_port *port,
> +				struct typec_altmode_desc *desc)
> +{
> +	return typec_port_register_altmode(port->port, desc);
> +}
> +#endif
> +
>  #endif /* __CROS_TYPEC_ALTMODE_H__ */
> -- 
> 2.47.0.163.g1226f6d8fa-goog
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 6/7] platform/chrome: cros_ec_typec: Thunderbolt support

[Abhishek Pandit-Subedi]

On Thu, Oct 31, 2024 at 11:51 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Oct 30, 2024 at 02:28:37PM -0700, Abhishek Pandit-Subedi wrote:
> > Add support for entering and exiting Thunderbolt alt-mode using AP
> > driven alt-mode.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Refactored thunderbolt support into cros_typec_altmode.c
> >
> >  drivers/platform/chrome/cros_ec_typec.c      | 29 ++++---
> >  drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
> >  drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
> >  3 files changed, 116 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 7997e7136c4c..3e043b1c1cc8 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -304,21 +304,26 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> >       typec_altmode_set_drvdata(amode, port);
> >       amode->ops = &port_amode_ops;
> >  #endif
> > -
> >       /*
> >        * Register TBT compatibility alt mode. The EC will not enter the mode
> > -      * if it doesn't support it, so it's safe to register it unconditionally
> > -      * here for now.
> > +      * if it doesn't support it and it will not enter automatically by
> > +      * design so we can use the |ap_driven_altmode| feature to check if we
> > +      * should register it.
> >        */
> > -     memset(&desc, 0, sizeof(desc));
> > -     desc.svid = USB_TYPEC_TBT_SID;
> > -     desc.mode = TYPEC_ANY_MODE;
> > -     amode = typec_port_register_altmode(port->port, &desc);
> > -     if (IS_ERR(amode))
> > -             return PTR_ERR(amode);
> > -     port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
> > -     typec_altmode_set_drvdata(amode, port);
> > -     amode->ops = &port_amode_ops;
> > +     if (typec->ap_driven_altmode) {
> > +             memset(&desc, 0, sizeof(desc));
> > +             desc.svid = USB_TYPEC_TBT_SID;
> > +             desc.mode = TYPEC_ANY_MODE;
> > +             amode = cros_typec_register_thunderbolt(port, &desc);
> > +             if (IS_ERR(amode))
> > +                     return PTR_ERR(amode);
> > +             port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
> > +
> > +#if !IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
> > +             typec_altmode_set_drvdata(amode, port);
> > +             amode->ops = &port_amode_ops;
> > +#endif
>
> Why? Usually having the code block under an #if is a frowned upon
> practice.

There are some CrosEC implementations that provide full VDM access for
mode entry and this code was previously added to support that. I'm
looking into whether this was fully deployed -- if not, I will remove
this #if block entirely in my next patch series.

Will do the same for displayport above.

>
> > +     }
> >
> >       port->state.alt = NULL;
> >       port->state.mode = TYPEC_STATE_USB;
>
> [...]
>
> > diff --git a/drivers/platform/chrome/cros_typec_altmode.h b/drivers/platform/chrome/cros_typec_altmode.h
> > index c6f8fb02c99c..c71568314e3f 100644
> > --- a/drivers/platform/chrome/cros_typec_altmode.h
> > +++ b/drivers/platform/chrome/cros_typec_altmode.h
> > @@ -31,4 +31,18 @@ static inline int cros_typec_displayport_status_update(struct typec_altmode *alt
> >       return 0;
> >  }
> >  #endif
> > +
> > +#if IS_ENABLED(CONFIG_TYPEC_TBT_ALTMODE)
> > +struct typec_altmode *
> > +cros_typec_register_thunderbolt(struct cros_typec_port *port,
> > +                             struct typec_altmode_desc *desc);
> > +#else
> > +struct typec_altmode *
>
> static inline struct ...
> LGTM otherwise

I ran allmodconfig and x86_64_defconfig but forgot to run allmodconfig
- these features :( -- argh... I'll add this to my testing steps.

>
> > +cros_typec_register_thunderbolt(struct cros_typec_port *port,
> > +                             struct typec_altmode_desc *desc)
> > +{
> > +     return typec_port_register_altmode(port->port, desc);
> > +}
> > +#endif
> > +
> >  #endif /* __CROS_TYPEC_ALTMODE_H__ */
> > --
> > 2.47.0.163.g1226f6d8fa-goog
> >
>
> --
> With best wishes
> Dmitry

[PATCH v2 7/7] platform/chrome: cros_ec_typec: Disable tbt auto_enter

[Abhishek Pandit-Subedi]

Altmodes with cros_ec are either automatically entered by the EC or
entered by the AP if TBT or USB4 are supported on the system. Due to the
security risk of PCIe tunneling, TBT modes should not be auto entered by
the kernel at this time and will require user intervention.

With this change, a userspace program will need to explicitly activate
the thunderbolt mode on the partner in order to enter the mode and the
thunderbolt driver will not automatically enter when a partner is
connected.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
- Only disable auto-enter for Thunderbolt
- Update commit message to clearly indicate the need for userspace
  intervention to enter TBT mode

 drivers/platform/chrome/cros_ec_typec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 3e043b1c1cc8..aadd2704e445 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -313,7 +313,8 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
 	if (typec->ap_driven_altmode) {
 		memset(&desc, 0, sizeof(desc));
 		desc.svid = USB_TYPEC_TBT_SID;
-		desc.mode = TYPEC_ANY_MODE;
+		desc.mode = USB_TYPEC_TBT_MODE;
+		desc.no_auto_enter = true;
 		amode = cros_typec_register_thunderbolt(port, &desc);
 		if (IS_ERR(amode))
 			return PTR_ERR(amode);
-- 
2.47.0.163.g1226f6d8fa-goog