* [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec
@ 2024-12-06 23:38 Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
` (6 more replies)
0 siblings, 7 replies; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Benson Leung, Greg Kroah-Hartman,
Guenter Roeck, Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
linux-kbuild, linux-kernel
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 v4:
- Large refactor to use cable_altmodes
- Fixed ordering of cable mode enter/exit (SOP', SOP", then port for
enter; reverse order for exit)
- Other small fixes from v3 feedback
- memset struct typec_altmode_desc
- Add CONFIG_CROS_EC_TYPEC_ALTMODES for Makefile use
- Move ap_driven_altmode check to common vdm function
- Add locking to protect shared data
- Update enter/exit error messages
- Update Makefile + Kconfig to use CONFIG_CROS_EC_TYPEC_ALTMODES
- Add locking in vdm function
Changes in v3:
- Removed mode from altmode device ids
- Updated modalias for typecd bus to remove mode
- Re-ordered to start of series
- Revert rename of TYPEC_TBT_MODE
- Remove mode from typec_device_id
- Refactored typec_altmode_dp_data per review request
- Removed unused vdm operations during altmode registration
- Fix usage of TBT sid and mode.
- Removed unused vdm operations during altmode registration
- Set port.inactive = true instead of auto-enter.
Changes in v2:
- Update altmode_match to ignore mode entirely
- Also apply the same behavior to typec_match
- 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
- 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
Changes in v1:
- 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))
Abhishek Pandit-Subedi (6):
usb: typec: Only use SVID for matching altmodes
usb: typec: Print err when displayport fails to enter
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 on port
Heikki Krogerus (1):
usb: typec: Add driver for Thunderbolt 3 Alternate Mode
MAINTAINERS | 3 +
drivers/platform/chrome/Kconfig | 7 +
drivers/platform/chrome/Makefile | 4 +
drivers/platform/chrome/cros_ec_typec.c | 48 ++-
drivers/platform/chrome/cros_ec_typec.h | 1 +
drivers/platform/chrome/cros_typec_altmode.c | 369 ++++++++++++++++++
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 | 5 +-
drivers/usb/typec/altmodes/nvidia.c | 2 +-
drivers/usb/typec/altmodes/thunderbolt.c | 387 +++++++++++++++++++
drivers/usb/typec/bus.c | 6 +-
drivers/usb/typec/class.c | 9 +-
include/linux/usb/typec.h | 2 +
include/linux/usb/typec_tbt.h | 1 +
scripts/mod/devicetable-offsets.c | 1 -
scripts/mod/file2alias.c | 4 +-
18 files changed, 879 insertions(+), 29 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.338.g60cca15819-goog
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 1/7] usb: typec: Only use SVID for matching altmodes
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
@ 2024-12-06 23:38 ` Abhishek Pandit-Subedi
2024-12-11 9:40 ` Heikki Krogerus
2024-12-06 23:38 ` [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Greg Kroah-Hartman, Masahiro Yamada,
Nathan Chancellor, Nicolas Schier, linux-kbuild, linux-kernel
Mode in struct typec_altmode is used to indicate the index of the
altmode on a port, partner or plug. It is used in enter mode VDMs but
doesn't make much sense for matching against altmode drivers or for
matching partner to port altmodes.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
(no changes since v3)
Changes in v3:
- Removed mode from altmode device ids
- Updated modalias for typecd bus to remove mode
- Re-ordered to start of series
Changes in v2:
- Update altmode_match to ignore mode entirely
- Also apply the same behavior to typec_match
drivers/usb/typec/altmodes/displayport.c | 2 +-
drivers/usb/typec/altmodes/nvidia.c | 2 +-
drivers/usb/typec/bus.c | 6 ++----
drivers/usb/typec/class.c | 4 ++--
scripts/mod/devicetable-offsets.c | 1 -
scripts/mod/file2alias.c | 4 +---
6 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 2f03190a9873..3245e03d59e6 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -791,7 +791,7 @@ void dp_altmode_remove(struct typec_altmode *alt)
EXPORT_SYMBOL_GPL(dp_altmode_remove);
static const struct typec_device_id dp_typec_id[] = {
- { USB_TYPEC_DP_SID, USB_TYPEC_DP_MODE },
+ { USB_TYPEC_DP_SID },
{ },
};
MODULE_DEVICE_TABLE(typec, dp_typec_id);
diff --git a/drivers/usb/typec/altmodes/nvidia.c b/drivers/usb/typec/altmodes/nvidia.c
index fe70b36f078f..2b77d931e494 100644
--- a/drivers/usb/typec/altmodes/nvidia.c
+++ b/drivers/usb/typec/altmodes/nvidia.c
@@ -24,7 +24,7 @@ static void nvidia_altmode_remove(struct typec_altmode *alt)
}
static const struct typec_device_id nvidia_typec_id[] = {
- { USB_TYPEC_NVIDIA_VLINK_SID, TYPEC_ANY_MODE },
+ { USB_TYPEC_NVIDIA_VLINK_SID },
{ },
};
MODULE_DEVICE_TABLE(typec, nvidia_typec_id);
diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
index aa879253d3b8..ae90688d23e4 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;
}
@@ -470,8 +469,7 @@ static int typec_uevent(const struct device *dev, struct kobj_uevent_env *env)
if (add_uevent_var(env, "MODE=%u", altmode->mode))
return -ENOMEM;
- return add_uevent_var(env, "MODALIAS=typec:id%04Xm%02X",
- altmode->svid, altmode->mode);
+ return add_uevent_var(env, "MODALIAS=typec:id%04X", altmode->svid);
}
static int typec_altmode_create_links(struct altmode *alt)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 4b3047e055a3..febe453b96be 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -237,13 +237,13 @@ 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)
{
struct typec_altmode *adev = &altmode->adev;
- struct typec_device_id id = { adev->svid, adev->mode, };
+ struct typec_device_id id = { adev->svid };
struct typec_port *port = typec_altmode2port(adev);
struct altmode *partner;
struct device *dev;
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 9c7b404defbd..d3d00e85edf7 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -237,7 +237,6 @@ int main(void)
DEVID(typec_device_id);
DEVID_FIELD(typec_device_id, svid);
- DEVID_FIELD(typec_device_id, mode);
DEVID(tee_client_device_id);
DEVID_FIELD(tee_client_device_id, uuid);
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index c4cc11aa558f..218ccb7150bf 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1343,14 +1343,12 @@ static int do_tbsvc_entry(const char *filename, void *symval, char *alias)
return 1;
}
-/* Looks like: typec:idNmN */
+/* Looks like: typec:idN */
static int do_typec_entry(const char *filename, void *symval, char *alias)
{
DEF_FIELD(symval, typec_device_id, svid);
- DEF_FIELD(symval, typec_device_id, mode);
sprintf(alias, "typec:id%04X", svid);
- ADD(alias, "m", mode != TYPEC_ANY_MODE, mode);
return 1;
}
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
@ 2024-12-06 23:38 ` Abhishek Pandit-Subedi
2024-12-09 4:28 ` kernel test robot
2024-12-11 0:21 ` Stephen Boyd
2024-12-06 23:38 ` [PATCH v4 3/7] usb: typec: Print err when displayport fails to enter Abhishek Pandit-Subedi
` (4 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Greg Kroah-Hartman, linux-kernel
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 in v4:
- Large refactor to use cable_altmodes
- Fixed ordering of cable mode enter/exit (SOP', SOP", then port for
enter; reverse order for exit)
- Other small fixes from v3 feedback
Changes in v3:
- Revert rename of TYPEC_TBT_MODE
- Remove mode from typec_device_id
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
Changes in v1:
- 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))
drivers/usb/typec/altmodes/Kconfig | 9 +
drivers/usb/typec/altmodes/Makefile | 2 +
drivers/usb/typec/altmodes/thunderbolt.c | 387 +++++++++++++++++++++++
drivers/usb/typec/class.c | 5 +-
include/linux/usb/typec.h | 2 +
include/linux/usb/typec_tbt.h | 1 +
6 files changed, 404 insertions(+), 2 deletions(-)
create mode 100644 drivers/usb/typec/altmodes/thunderbolt.c
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..14e89e9a7691
--- /dev/null
+++ b/drivers/usb/typec/altmodes/thunderbolt.c
@@ -0,0 +1,387 @@
+// 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_cable_altmode_enter(tbt->alt, TYPEC_PLUG_SOP_P, NULL);
+ if (ret) {
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
+ "failed to enter mode (%d)\n", ret);
+ goto disable_plugs;
+ }
+ break;
+ case TBT_STATE_SOP_PP_ENTER:
+ ret = typec_cable_altmode_enter(tbt->alt, TYPEC_PLUG_SOP_PP, NULL);
+ if (ret) {
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
+ "failed to enter mode (%d)\n", ret);
+ goto disable_plugs;
+ }
+ 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_cable_altmode_exit(tbt->alt, TYPEC_PLUG_SOP_PP);
+ break;
+ case TBT_STATE_SOP_P_EXIT:
+ typec_cable_altmode_exit(tbt->alt, TYPEC_PLUG_SOP_P);
+ break;
+ default:
+ break;
+ }
+
+ tbt->state = TBT_STATE_IDLE;
+
+ mutex_unlock(&tbt->lock);
+ return;
+
+disable_plugs:
+ for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
+ if (tbt->plug[i])
+ typec_altmode_put_plug(tbt->plug[i]);
+
+ tbt->plug[i] = NULL;
+ }
+
+ tbt->state = TBT_STATE_ENTER;
+ schedule_work(&tbt->work);
+ mutex_unlock(&tbt->lock);
+}
+
+/* MUST HOLD tbt->lock.
+ *
+ * If SOP' is available, enter that first (which will trigger a VDM response
+ * that will enter SOP" if available and then the port). If entering SOP' fails,
+ * stop attempting to enter either cable altmode (probably not supported) and
+ * directly enter the port altmode.
+ */
+static int tbt_enter_modes_ordered(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ int ret = 0;
+
+ if (!tbt_ready(tbt->alt))
+ return -ENODEV;
+
+ if (tbt->plug[TYPEC_PLUG_SOP_P]) {
+ ret = typec_cable_altmode_enter(alt, TYPEC_PLUG_SOP_P, NULL);
+ if (ret < 0) {
+ for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
+ if (tbt->plug[i])
+ typec_altmode_put_plug(tbt->plug[i]);
+
+ tbt->plug[i] = NULL;
+ }
+ } else {
+ return ret;
+ }
+ }
+
+ return tbt_enter_mode(tbt);
+}
+
+static int tbt_cable_altmode_vdm(struct typec_altmode *alt,
+ enum typec_plug_index sop, 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 described in USB Type-C Spec
+ * R2.0 Section 6.7.3: SOP', SOP", then port.
+ */
+ if (sop == 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 (sop == TYPEC_PLUG_SOP_PP)
+ tbt->state = TBT_STATE_ENTER;
+
+ break;
+ case CMD_EXIT_MODE:
+ /* Exit in opposite order: Port, SOP", then SOP'. */
+ if (sop == TYPEC_PLUG_SOP_PP)
+ tbt->state = TBT_STATE_SOP_P_EXIT;
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+
+ if (tbt->state != TBT_STATE_IDLE)
+ schedule_work(&tbt->work);
+
+
+ mutex_unlock(&tbt->lock);
+ return 0;
+}
+
+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);
+ struct typec_thunderbolt_data data;
+ 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:
+ /* Port altmode is last to enter and first to exit. */
+ switch (cmd) {
+ case CMD_ENTER_MODE:
+ memset(&data, 0, sizeof(data));
+
+ data.device_mode = tbt->alt->vdo;
+ data.enter_vdo = tbt->enter_vdo;
+ if (tbt->plug[TYPEC_PLUG_SOP_P])
+ data.cable_mode = tbt->plug[TYPEC_PLUG_SOP_P]->vdo;
+
+ typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
+ break;
+ case CMD_EXIT_MODE:
+ 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;
+ 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 (activate)
+ ret = tbt_enter_modes_ordered(alt);
+ 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 const struct typec_cable_ops tbt_cable_ops = {
+ .vdm = tbt_cable_altmode_vdm,
+};
+
+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_P])
+ tbt->state = TBT_STATE_SOP_P_ENTER;
+ else if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_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;
+
+ /* Thunderbolt 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_TBT_SID)
+ break;
+
+ plug->desc = "Thunderbolt3";
+ plug->cable_ops = &tbt_cable_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 },
+ { }
+};
+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",
+ }
+};
+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/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index febe453b96be..b5e67a57762c 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -458,7 +458,8 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
struct typec_altmode *adev = to_typec_altmode(kobj_to_dev(kobj));
if (attr == &dev_attr_active.attr)
- if (!adev->ops || !adev->ops->activate)
+ if (!is_typec_port(adev->dev.parent) &&
+ (!adev->ops || !adev->ops->activate))
return 0444;
return attr->mode;
@@ -563,7 +564,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.active = !desc->inactive; /* Enabled by default */
}
sprintf(alt->group_name, "mode%d", desc->mode);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index d616b8807000..252af3f77039 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -140,6 +140,7 @@ int typec_cable_set_identity(struct typec_cable *cable);
* @mode: Index of the Mode
* @vdo: VDO returned by Discover Modes USB PD command
* @roles: Only for ports. DRP if the mode is available in both roles
+ * @inactive: Only for ports. Make this port inactive (default is active).
*
* Description of an Alternate Mode which a connector, cable plug or partner
* supports.
@@ -150,6 +151,7 @@ struct typec_altmode_desc {
u32 vdo;
/* Only used with ports */
enum typec_port_data roles;
+ bool inactive;
};
void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
index fa97d7e00f5c..55dcea12082c 100644
--- a/include/linux/usb/typec_tbt.h
+++ b/include/linux/usb/typec_tbt.h
@@ -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.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 3/7] usb: typec: Print err when displayport fails to enter
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
@ 2024-12-06 23:38 ` Abhishek Pandit-Subedi
2024-12-11 10:09 ` Heikki Krogerus
2024-12-06 23:38 ` [PATCH v4 4/7] platform/chrome: cros_ec_typec: Update partner altmode active Abhishek Pandit-Subedi
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Greg Kroah-Hartman, linux-kernel
Print the error reason for typec_altmode_enter so users can understand
why displayport failed to enter.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
(no changes since v1)
drivers/usb/typec/altmodes/displayport.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 3245e03d59e6..e292780ec309 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -252,7 +252,8 @@ static void dp_altmode_work(struct work_struct *work)
case DP_STATE_ENTER:
ret = typec_altmode_enter(dp->alt, NULL);
if (ret && ret != -EBUSY)
- dev_err(&dp->alt->dev, "failed to enter mode\n");
+ dev_err(&dp->alt->dev, "failed to enter mode: %d\n",
+ ret);
break;
case DP_STATE_ENTER_PRIME:
ret = typec_cable_altmode_enter(dp->alt, TYPEC_PLUG_SOP_P, NULL);
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 4/7] platform/chrome: cros_ec_typec: Update partner altmode active
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (2 preceding siblings ...)
2024-12-06 23:38 ` [PATCH v4 3/7] usb: typec: Print err when displayport fails to enter Abhishek Pandit-Subedi
@ 2024-12-06 23:38 ` Abhishek Pandit-Subedi
2024-12-10 23:32 ` Stephen Boyd
2024-12-06 23:38 ` [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Benson Leung, Guenter Roeck, linux-kernel
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 c7781aea0b88..e3eabe5e42ac 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.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (3 preceding siblings ...)
2024-12-06 23:38 ` [PATCH v4 4/7] platform/chrome: cros_ec_typec: Update partner altmode active Abhishek Pandit-Subedi
@ 2024-12-06 23:38 ` Abhishek Pandit-Subedi
2024-12-11 0:08 ` Stephen Boyd
2024-12-11 21:58 ` Stephen Boyd
2024-12-06 23:38 ` [PATCH v4 6/7] platform/chrome: cros_ec_typec: Thunderbolt support Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 7/7] platform/chrome: cros_ec_typec: Disable tbt on port Abhishek Pandit-Subedi
6 siblings, 2 replies; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Benson Leung, Guenter Roeck, linux-kernel
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 v4:
- memset struct typec_altmode_desc
- Add CONFIG_CROS_EC_TYPEC_ALTMODES for Makefile use
- Move ap_driven_altmode check to common vdm function
- Add locking to protect shared data
- Update enter/exit error messages
Changes in v3:
- Refactored typec_altmode_dp_data per review request
- Removed unused vdm operations during altmode registration
Changes in v2:
- Refactored displayport into cros_typec_altmode.c to extract common
implementation between altmodes
MAINTAINERS | 3 +
drivers/platform/chrome/Kconfig | 6 +
drivers/platform/chrome/Makefile | 4 +
drivers/platform/chrome/cros_ec_typec.c | 13 +-
drivers/platform/chrome/cros_ec_typec.h | 1 +
drivers/platform/chrome/cros_typec_altmode.c | 281 +++++++++++++++++++
drivers/platform/chrome/cros_typec_altmode.h | 34 +++
7 files changed, 339 insertions(+), 3 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 cd6aa609deba..5f9d8b8f1cb3 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/Kconfig b/drivers/platform/chrome/Kconfig
index 7dbeb786352a..984f843ea7a2 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -226,12 +226,18 @@ config CROS_EC_SYSFS
To compile this driver as a module, choose M here: the
module will be called cros_ec_sysfs.
+config CROS_EC_TYPEC_ALTMODES
+ bool
+ help
+ Selectable symbol to enable altmodes.
+
config CROS_EC_TYPEC
tristate "ChromeOS EC Type-C Connector Control"
depends on MFD_CROS_EC_DEV && TYPEC
depends on CROS_USBPD_NOTIFY
depends on USB_ROLE_SWITCH
default MFD_CROS_EC_DEV
+ select CROS_EC_TYPEC_ALTMODES if TYPEC_DP_ALTMODE
help
If you say Y here, you get support for accessing Type C connector
information from the Chrome OS EC.
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..aec2d043a0fe 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -18,7 +18,11 @@ 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
+ifneq ($(CONFIG_CROS_EC_TYPEC_ALTMODES),)
+ cros-ec-typec-objs += cros_typec_altmode.o
+endif
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 e3eabe5e42ac..0f3bc335f583 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"
@@ -290,15 +291,15 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
struct typec_altmode *amode;
/* All PD capable CrOS devices are assumed to support DP altmode. */
+ memset(&desc, 0, sizeof(desc));
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;
- typec_altmode_set_drvdata(amode, port);
- amode->ops = &port_amode_ops;
/*
* Register TBT compatibility alt mode. The EC will not enter the mode
@@ -575,6 +576,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 +1259,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..bb7c7ad2ff6e
--- /dev/null
+++ b/drivers/platform/chrome/cros_typec_altmode.c
@@ -0,0 +1,281 @@
+// 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_altmode_data {
+ struct work_struct work;
+ struct cros_typec_port *port;
+ struct typec_altmode *alt;
+ bool ap_mode_entry;
+
+ struct mutex lock;
+ u32 header;
+ u32 *vdo_data;
+ u8 vdo_size;
+
+ u16 sid;
+ u8 mode;
+};
+
+struct cros_typec_dp_data {
+ struct cros_typec_altmode_data adata;
+ struct typec_displayport_data data;
+ bool configured;
+ bool pending_status_update;
+};
+
+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);
+
+ mutex_lock(&data->lock);
+
+ 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;
+
+ mutex_unlock(&data->lock);
+}
+
+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) {
+ dev_warn(&alt->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;
+
+ mutex_lock(&data->lock);
+
+ 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);
+
+ mutex_unlock(&data->lock);
+ 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) {
+ dev_warn(&alt->dev,
+ "EC does not support AP driven mode exit\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;
+
+ mutex_lock(&data->lock);
+
+ 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);
+
+ mutex_unlock(&data->lock);
+ return ret;
+}
+
+static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
+ const u32 *data, int count)
+{
+ struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt);
+ struct cros_typec_altmode_data *adata = &dp_data->adata;
+
+
+ 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;
+
+ mutex_lock(&adata->lock);
+
+ 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:
+ dp_data->data.conf = *data;
+ adata->header |= VDO_CMDT(CMDT_RSP_ACK);
+ dp_data->configured = true;
+ schedule_work(&adata->work);
+ break;
+ case DP_CMD_STATUS_UPDATE:
+ dp_data->pending_status_update = true;
+ break;
+ default:
+ adata->header |= VDO_CMDT(CMDT_RSP_ACK);
+ schedule_work(&adata->work);
+ break;
+ }
+
+ break;
+ default:
+ break;
+ }
+
+ mutex_unlock(&adata->lock);
+ 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->ap_mode_entry)
+ return -EOPNOTSUPP;
+
+ 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_dp_data *dp_data =
+ typec_altmode_get_drvdata(altmode);
+ struct cros_typec_altmode_data *adata = &dp_data->adata;
+
+ if (!dp_data->pending_status_update) {
+ dev_dbg(&altmode->dev,
+ "Got DPStatus without a pending request");
+ return 0;
+ }
+
+ if (dp_data->configured && dp_data->data.conf != data->conf)
+ dev_dbg(&altmode->dev,
+ "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
+ dp_data->data.conf, data->conf);
+
+ mutex_lock(&adata->lock);
+
+ dp_data->data = *data;
+ dp_data->pending_status_update = false;
+ adata->header |= VDO_CMDT(CMDT_RSP_ACK);
+ adata->vdo_data = &dp_data->data.status;
+ adata->vdo_size = 2;
+ schedule_work(&adata->work);
+
+ mutex_unlock(&adata->lock);
+
+ 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);
+ mutex_init(&data->lock);
+ data->alt = alt;
+ data->port = port;
+ data->ap_mode_entry = ap_mode_entry;
+ data->sid = desc->svid;
+ data->mode = desc->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
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.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (4 preceding siblings ...)
2024-12-06 23:38 ` [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
@ 2024-12-06 23:38 ` Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 7/7] platform/chrome: cros_ec_typec: Disable tbt on port Abhishek Pandit-Subedi
6 siblings, 0 replies; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Benson Leung, Guenter Roeck, linux-kernel
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 v4:
- Update Makefile + Kconfig to use CONFIG_CROS_EC_TYPEC_ALTMODES
- Add locking in vdm function
Changes in v3:
- Fix usage of TBT sid and mode.
- Removed unused vdm operations during altmode registration
Changes in v2:
- Refactored thunderbolt support into cros_typec_altmode.c
drivers/platform/chrome/Kconfig | 1 +
drivers/platform/chrome/cros_ec_typec.c | 23 ++---
drivers/platform/chrome/cros_typec_altmode.c | 88 ++++++++++++++++++++
drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
4 files changed, 115 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 984f843ea7a2..3eef4b5c2f60 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -238,6 +238,7 @@ config CROS_EC_TYPEC
depends on USB_ROLE_SWITCH
default MFD_CROS_EC_DEV
select CROS_EC_TYPEC_ALTMODES if TYPEC_DP_ALTMODE
+ select CROS_EC_TYPEC_ALTMODES if TYPEC_TBT_ALTMODE
help
If you say Y here, you get support for accessing Type C connector
information from the Chrome OS EC.
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 0f3bc335f583..0b87be52e1db 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -303,18 +303,19 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
/*
* 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 = TBT_MODE;
+ amode = cros_typec_register_thunderbolt(port, &desc);
+ if (IS_ERR(amode))
+ return PTR_ERR(amode);
+ port->port_altmode[CROS_EC_ALTMODE_TBT] = amode;
+ }
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 bb7c7ad2ff6e..4db655465a86 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"
@@ -70,6 +71,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;
@@ -194,6 +197,56 @@ 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;
+
+ mutex_lock(&adata->lock);
+
+ 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);
+
+ 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;
+ }
+
+ mutex_unlock(&adata->lock);
+ return 0;
+}
+
+
static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
const u32 *data, int count)
{
@@ -205,6 +258,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;
}
@@ -279,3 +335,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 = desc->svid;
+ data->mode = desc->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..810b553ddcd8 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
+static inline 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.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 7/7] platform/chrome: cros_ec_typec: Disable tbt on port
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (5 preceding siblings ...)
2024-12-06 23:38 ` [PATCH v4 6/7] platform/chrome: cros_ec_typec: Thunderbolt support Abhishek Pandit-Subedi
@ 2024-12-06 23:38 ` Abhishek Pandit-Subedi
6 siblings, 0 replies; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 23:38 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Benson Leung, Guenter Roeck, linux-kernel
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 port and 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>
---
(no changes since v3)
Changes in v3:
- Set port.inactive = true instead of auto-enter.
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 | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 0b87be52e1db..743cb5a9fe51 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -311,6 +311,7 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
memset(&desc, 0, sizeof(desc));
desc.svid = USB_TYPEC_TBT_SID;
desc.mode = TBT_MODE;
+ desc.inactive = true;
amode = cros_typec_register_thunderbolt(port, &desc);
if (IS_ERR(amode))
return PTR_ERR(amode);
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-12-06 23:38 ` [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
@ 2024-12-09 4:28 ` kernel test robot
2024-12-11 0:21 ` Stephen Boyd
1 sibling, 0 replies; 21+ messages in thread
From: kernel test robot @ 2024-12-09 4:28 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, heikki.krogerus, tzungbi, linux-usb,
chrome-platform
Cc: oe-kbuild-all, akuchynski, pmalani, jthies, dmitry.baryshkov,
badhri, rdbabiera, Abhishek Pandit-Subedi, Greg Kroah-Hartman,
linux-kernel
Hi Abhishek,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.12]
[cannot apply to chrome-platform/for-next chrome-platform/for-firmware-next usb/usb-testing usb/usb-next usb/usb-linus masahiroy-kbuild/for-next masahiroy-kbuild/fixes linus/master v6.13-rc1 next-20241206]
[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-Only-use-SVID-for-matching-altmodes/20241207-074104
base: v6.12
patch link: https://lore.kernel.org/r/20241206153813.v4.2.I3080b036e8de0b9957c57c1c3059db7149c5e549%40changeid
patch subject: [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20241208/202412080010.bWqlreGq-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241208/202412080010.bWqlreGq-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/202412080010.bWqlreGq-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/usb/typec/altmodes/thunderbolt.c:16: warning: cannot understand function prototype: 'enum tbt_state '
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 4/7] platform/chrome: cros_ec_typec: Update partner altmode active
2024-12-06 23:38 ` [PATCH v4 4/7] platform/chrome: cros_ec_typec: Update partner altmode active Abhishek Pandit-Subedi
@ 2024-12-10 23:32 ` Stephen Boyd
2024-12-13 18:01 ` Abhishek Pandit-Subedi
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2024-12-10 23:32 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, chrome-platform, heikki.krogerus,
linux-usb, tzungbi
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Benson Leung, Guenter Roeck, linux-kernel
Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:15)
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index c7781aea0b88..e3eabe5e42ac 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -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) {
This can just be list_for_each_entry() because the list isn't changing
during iteration.
> + 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);
> + }
It could also be shorter:
list_for_each_entry(node, &port->partner_mode_list, list) {
typec_altmode_update_active(node->amode,
port->state.alt && node->amode->svid == port->state.alt->svid);
}
As far as I can tell, cros_typec_configure_mux() is called when the HPD
state changes. We'll iterate through here unnecessarily in that case.
Can that be avoided somehow?
> +
> mux_ack:
> if (!typec->needs_mux_ack)
> return ret;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-12-06 23:38 ` [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
@ 2024-12-11 0:08 ` Stephen Boyd
2024-12-13 18:29 ` Abhishek Pandit-Subedi
2024-12-11 21:58 ` Stephen Boyd
1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2024-12-11 0:08 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, chrome-platform, heikki.krogerus,
linux-usb, tzungbi
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Benson Leung, Guenter Roeck, linux-kernel
Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index e3eabe5e42ac..0f3bc335f583 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"
>
> @@ -290,15 +291,15 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> struct typec_altmode *amode;
>
> /* All PD capable CrOS devices are assumed to support DP altmode. */
> + memset(&desc, 0, sizeof(desc));
> 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;
> - typec_altmode_set_drvdata(amode, port);
> - amode->ops = &port_amode_ops;
>
> /*
> * Register TBT compatibility alt mode. The EC will not enter the mode
> @@ -575,6 +576,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,
Should we forward the return value from here?
> + port->state.data);
> +
> return ret;
> }
>
> @@ -1254,6 +1259,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;
Do we need to stash this? Can we cros_ec_check_features() in
cros_typec_init_ports() and pass the bool to
cros_typec_register_port_altmodes() instead to save a struct member?
> };
>
> /* 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..bb7c7ad2ff6e
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -0,0 +1,281 @@
> +// 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>
Please include workqueue.h, mutex.h, etc. for things used in this file.
> +
> +#include "cros_typec_altmode.h"
> +
> +struct cros_typec_altmode_data {
> + struct work_struct work;
> + struct cros_typec_port *port;
> + struct typec_altmode *alt;
> + bool ap_mode_entry;
The UCSI driver (drivers/usb/typec/ucsi/displayport.c) calls this
'override', can it be named the same thing? I also see that the UCSI
driver has two bools, 'override' and 'initialized', which seems to be to
support a DP_CMD_CONFIGURE that will respond with an ACK and then the
next DP_CMD_CONFIGURE will call ucsi_altmode_update_active() to set the
altmode as active. Maybe the same method can be followed here so that on
older chromebooks where EC is in control of mode entry we can emulate
entering the mode?
> +
> + struct mutex lock;
> + u32 header;
> + u32 *vdo_data;
> + u8 vdo_size;
> +
> + u16 sid;
> + u8 mode;
> +};
> +
> +struct cros_typec_dp_data {
> + struct cros_typec_altmode_data adata;
> + struct typec_displayport_data data;
> + bool configured;
> + bool pending_status_update;
> +};
> +
> +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);
> +
> + mutex_lock(&data->lock);
> +
> + 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);
These printks need newlines.
dev_err(&data->alt->dev, "VDM 0x%x failed\n", data->header);
> +
> + data->header = 0;
> + data->vdo_data = NULL;
> + data->vdo_size = 0;
> +
> + mutex_unlock(&data->lock);
> +}
> +
> +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) {
> + dev_warn(&alt->dev,
> + "EC does not support AP driven mode entry\n");
Like this one.
> + 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,
Do we plan to delete drivers/platform/chrome/cros_typec_vdm.c file at
some point? I ask because 'port_amode_ops' becomes unused after this
series.
> + &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;
> +
> + mutex_lock(&data->lock);
> +
> + 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);
> +
> + mutex_unlock(&data->lock);
> + 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) {
> + dev_warn(&alt->dev,
> + "EC does not support AP driven mode exit\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;
> +
> + mutex_lock(&data->lock);
> +
> + 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);
> +
> + mutex_unlock(&data->lock);
> + return ret;
> +}
> +
> +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> + const u32 *data, int count)
> +{
> + struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt);
> + struct cros_typec_altmode_data *adata = &dp_data->adata;
> +
> +
> + 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;
> +
> + mutex_lock(&adata->lock);
> +
> + 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:
> + dp_data->data.conf = *data;
> + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> + dp_data->configured = true;
> + schedule_work(&adata->work);
> + break;
> + case DP_CMD_STATUS_UPDATE:
> + dp_data->pending_status_update = true;
> + break;
> + default:
> + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> + schedule_work(&adata->work);
> + break;
> + }
> +
> + break;
> + default:
> + break;
> + }
> +
> + mutex_unlock(&adata->lock);
> + 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->ap_mode_entry)
> + return -EOPNOTSUPP;
> +
> + 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_dp_data *dp_data =
> + typec_altmode_get_drvdata(altmode);
> + struct cros_typec_altmode_data *adata = &dp_data->adata;
> +
> + if (!dp_data->pending_status_update) {
> + dev_dbg(&altmode->dev,
> + "Got DPStatus without a pending request");
> + return 0;
> + }
> +
> + if (dp_data->configured && dp_data->data.conf != data->conf)
> + dev_dbg(&altmode->dev,
> + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> + dp_data->data.conf, data->conf);
> +
> + mutex_lock(&adata->lock);
> +
> + dp_data->data = *data;
> + dp_data->pending_status_update = false;
> + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> + adata->vdo_data = &dp_data->data.status;
> + adata->vdo_size = 2;
> + schedule_work(&adata->work);
> +
> + mutex_unlock(&adata->lock);
> +
> + 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;
Can you name it 'adata' consistently? That makes it easy to search for
'adata' in this file and know it's always talking about a struct
cros_typec_altmode_data type of 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);
> + mutex_init(&data->lock);
> + data->alt = alt;
> + data->port = port;
> + data->ap_mode_entry = ap_mode_entry;
> + data->sid = desc->svid;
> + data->mode = desc->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
> 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__
#include <linux/kconfig.h> for IS_ENABLED()
#include <linux/usb/typec.h> for typec_port_register_altmode()
> +
> +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);
> +}
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-12-06 23:38 ` [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
2024-12-09 4:28 ` kernel test robot
@ 2024-12-11 0:21 ` Stephen Boyd
2024-12-13 17:55 ` Abhishek Pandit-Subedi
1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2024-12-11 0:21 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, chrome-platform, heikki.krogerus,
linux-usb, tzungbi
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Greg Kroah-Hartman, linux-kernel
Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:13)
> diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
> new file mode 100644
> index 000000000000..14e89e9a7691
> --- /dev/null
> +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> @@ -0,0 +1,387 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
Remove extra *, this isn't kerneldoc.
> + * USB Typec-C Thuderbolt3 Alternate Mode driver
> + *
> + * Copyright (C) 2019 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + */
> +
> +#include <linux/delay.h>
Is this include used?
> +#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>
Please include workqueue.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 */
What does it protect? The whole struct tbt_altmode?
> +};
[...]
> +
> +/* MUST HOLD tbt->lock.
Use lockdep_assert_held(tbt->lock) and remove the comment?
> + *
> + * If SOP' is available, enter that first (which will trigger a VDM response
> + * that will enter SOP" if available and then the port). If entering SOP' fails,
> + * stop attempting to enter either cable altmode (probably not supported) and
> + * directly enter the port altmode.
> + */
> +static int tbt_enter_modes_ordered(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int ret = 0;
> +
> + if (!tbt_ready(tbt->alt))
> + return -ENODEV;
> +
> + if (tbt->plug[TYPEC_PLUG_SOP_P]) {
> + ret = typec_cable_altmode_enter(alt, TYPEC_PLUG_SOP_P, NULL);
> + if (ret < 0) {
> + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> + if (tbt->plug[i])
> + typec_altmode_put_plug(tbt->plug[i]);
> +
> + tbt->plug[i] = NULL;
> + }
> + } else {
> + return ret;
> + }
> + }
> +
> + return tbt_enter_mode(tbt);
> +}
> +
> +static int tbt_cable_altmode_vdm(struct typec_altmode *alt,
> + enum typec_plug_index sop, const u32 hdr,
> + const u32 *vdo, int count)
> +{
[...]
> + case CMD_EXIT_MODE:
> + /* Exit in opposite order: Port, SOP", then SOP'. */
> + if (sop == TYPEC_PLUG_SOP_PP)
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + if (tbt->state != TBT_STATE_IDLE)
> + schedule_work(&tbt->work);
> +
> +
Nitpick: Why two newlines?
> + mutex_unlock(&tbt->lock);
> + return 0;
> +}
> +
[...]
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index febe453b96be..b5e67a57762c 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -458,7 +458,8 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
> struct typec_altmode *adev = to_typec_altmode(kobj_to_dev(kobj));
>
> if (attr == &dev_attr_active.attr)
> - if (!adev->ops || !adev->ops->activate)
> + if (!is_typec_port(adev->dev.parent) &&
> + (!adev->ops || !adev->ops->activate))
> return 0444;
>
> return attr->mode;
> @@ -563,7 +564,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.active = !desc->inactive; /* Enabled by default */
> }
>
> sprintf(alt->group_name, "mode%d", desc->mode);
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index d616b8807000..252af3f77039 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -140,6 +140,7 @@ int typec_cable_set_identity(struct typec_cable *cable);
> * @mode: Index of the Mode
> * @vdo: VDO returned by Discover Modes USB PD command
> * @roles: Only for ports. DRP if the mode is available in both roles
> + * @inactive: Only for ports. Make this port inactive (default is active).
> *
> * Description of an Alternate Mode which a connector, cable plug or partner
> * supports.
> @@ -150,6 +151,7 @@ struct typec_altmode_desc {
> u32 vdo;
> /* Only used with ports */
> enum typec_port_data roles;
> + bool inactive;
> };
>
> void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
These two files look like they can be a different patch? Or the commit
text can describe these changes.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/7] usb: typec: Only use SVID for matching altmodes
2024-12-06 23:38 ` [PATCH v4 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
@ 2024-12-11 9:40 ` Heikki Krogerus
0 siblings, 0 replies; 21+ messages in thread
From: Heikki Krogerus @ 2024-12-11 9:40 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: tzungbi, linux-usb, chrome-platform, akuchynski, pmalani, jthies,
dmitry.baryshkov, badhri, rdbabiera, Greg Kroah-Hartman,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier, linux-kbuild,
linux-kernel
On Fri, Dec 06, 2024 at 03:38:12PM -0800, Abhishek Pandit-Subedi wrote:
> Mode in struct typec_altmode is used to indicate the index of the
> altmode on a port, partner or plug. It is used in enter mode VDMs but
> doesn't make much sense for matching against altmode drivers or for
> matching partner to port altmodes.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Removed mode from altmode device ids
> - Updated modalias for typecd bus to remove mode
> - Re-ordered to start of series
>
> Changes in v2:
> - Update altmode_match to ignore mode entirely
> - Also apply the same behavior to typec_match
>
> drivers/usb/typec/altmodes/displayport.c | 2 +-
> drivers/usb/typec/altmodes/nvidia.c | 2 +-
> drivers/usb/typec/bus.c | 6 ++----
> drivers/usb/typec/class.c | 4 ++--
> scripts/mod/devicetable-offsets.c | 1 -
> scripts/mod/file2alias.c | 4 +---
> 6 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 2f03190a9873..3245e03d59e6 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -791,7 +791,7 @@ void dp_altmode_remove(struct typec_altmode *alt)
> EXPORT_SYMBOL_GPL(dp_altmode_remove);
>
> static const struct typec_device_id dp_typec_id[] = {
> - { USB_TYPEC_DP_SID, USB_TYPEC_DP_MODE },
> + { USB_TYPEC_DP_SID },
> { },
> };
> MODULE_DEVICE_TABLE(typec, dp_typec_id);
> diff --git a/drivers/usb/typec/altmodes/nvidia.c b/drivers/usb/typec/altmodes/nvidia.c
> index fe70b36f078f..2b77d931e494 100644
> --- a/drivers/usb/typec/altmodes/nvidia.c
> +++ b/drivers/usb/typec/altmodes/nvidia.c
> @@ -24,7 +24,7 @@ static void nvidia_altmode_remove(struct typec_altmode *alt)
> }
>
> static const struct typec_device_id nvidia_typec_id[] = {
> - { USB_TYPEC_NVIDIA_VLINK_SID, TYPEC_ANY_MODE },
> + { USB_TYPEC_NVIDIA_VLINK_SID },
> { },
> };
> MODULE_DEVICE_TABLE(typec, nvidia_typec_id);
> diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
> index aa879253d3b8..ae90688d23e4 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;
> }
> @@ -470,8 +469,7 @@ static int typec_uevent(const struct device *dev, struct kobj_uevent_env *env)
> if (add_uevent_var(env, "MODE=%u", altmode->mode))
> return -ENOMEM;
>
> - return add_uevent_var(env, "MODALIAS=typec:id%04Xm%02X",
> - altmode->svid, altmode->mode);
> + return add_uevent_var(env, "MODALIAS=typec:id%04X", altmode->svid);
> }
>
> static int typec_altmode_create_links(struct altmode *alt)
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 4b3047e055a3..febe453b96be 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -237,13 +237,13 @@ 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)
> {
> struct typec_altmode *adev = &altmode->adev;
> - struct typec_device_id id = { adev->svid, adev->mode, };
> + struct typec_device_id id = { adev->svid };
> struct typec_port *port = typec_altmode2port(adev);
> struct altmode *partner;
> struct device *dev;
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index 9c7b404defbd..d3d00e85edf7 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -237,7 +237,6 @@ int main(void)
>
> DEVID(typec_device_id);
> DEVID_FIELD(typec_device_id, svid);
> - DEVID_FIELD(typec_device_id, mode);
>
> DEVID(tee_client_device_id);
> DEVID_FIELD(tee_client_device_id, uuid);
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index c4cc11aa558f..218ccb7150bf 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1343,14 +1343,12 @@ static int do_tbsvc_entry(const char *filename, void *symval, char *alias)
> return 1;
> }
>
> -/* Looks like: typec:idNmN */
> +/* Looks like: typec:idN */
> static int do_typec_entry(const char *filename, void *symval, char *alias)
> {
> DEF_FIELD(symval, typec_device_id, svid);
> - DEF_FIELD(symval, typec_device_id, mode);
>
> sprintf(alias, "typec:id%04X", svid);
> - ADD(alias, "m", mode != TYPEC_ANY_MODE, mode);
>
> return 1;
> }
> --
> 2.47.0.338.g60cca15819-goog
--
heikki
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/7] usb: typec: Print err when displayport fails to enter
2024-12-06 23:38 ` [PATCH v4 3/7] usb: typec: Print err when displayport fails to enter Abhishek Pandit-Subedi
@ 2024-12-11 10:09 ` Heikki Krogerus
0 siblings, 0 replies; 21+ messages in thread
From: Heikki Krogerus @ 2024-12-11 10:09 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: tzungbi, linux-usb, chrome-platform, akuchynski, pmalani, jthies,
dmitry.baryshkov, badhri, rdbabiera, Greg Kroah-Hartman,
linux-kernel
On Fri, Dec 06, 2024 at 03:38:14PM -0800, Abhishek Pandit-Subedi wrote:
> Print the error reason for typec_altmode_enter so users can understand
> why displayport failed to enter.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> (no changes since v1)
>
> drivers/usb/typec/altmodes/displayport.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 3245e03d59e6..e292780ec309 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -252,7 +252,8 @@ static void dp_altmode_work(struct work_struct *work)
> case DP_STATE_ENTER:
> ret = typec_altmode_enter(dp->alt, NULL);
> if (ret && ret != -EBUSY)
> - dev_err(&dp->alt->dev, "failed to enter mode\n");
> + dev_err(&dp->alt->dev, "failed to enter mode: %d\n",
> + ret);
One line is enough.
> break;
> case DP_STATE_ENTER_PRIME:
> ret = typec_cable_altmode_enter(dp->alt, TYPEC_PLUG_SOP_P, NULL);
thanks,
--
heikki
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-12-06 23:38 ` [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
2024-12-11 0:08 ` Stephen Boyd
@ 2024-12-11 21:58 ` Stephen Boyd
2024-12-13 18:33 ` Abhishek Pandit-Subedi
1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2024-12-11 21:58 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, chrome-platform, heikki.krogerus,
linux-usb, tzungbi
Cc: akuchynski, pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Abhishek Pandit-Subedi, Benson Leung, Guenter Roeck, linux-kernel
Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> new file mode 100644
> index 000000000000..bb7c7ad2ff6e
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -0,0 +1,281 @@
[...]
> +
> +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_dp_data *dp_data =
> + typec_altmode_get_drvdata(altmode);
How does this work? I see that the type of the drvdata is struct
cros_typec_altmode_data per the allocation in
cros_typec_register_displayport(), but here we're treating it as the
type struct cros_typec_dp_data, which has a struct
cros_typec_altmode_data as the first member. The allocation is too small
from what I can tell. The same problem looks to be there in
cros_typec_displayport_vdm().
> + struct cros_typec_altmode_data *adata = &dp_data->adata;
> +
> + if (!dp_data->pending_status_update) {
> + dev_dbg(&altmode->dev,
> + "Got DPStatus without a pending request");
> + return 0;
> + }
> +
> + if (dp_data->configured && dp_data->data.conf != data->conf)
> + dev_dbg(&altmode->dev,
> + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> + dp_data->data.conf, data->conf);
> +
> + mutex_lock(&adata->lock);
> +
> + dp_data->data = *data;
> + dp_data->pending_status_update = false;
> + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> + adata->vdo_data = &dp_data->data.status;
> + adata->vdo_size = 2;
> + schedule_work(&adata->work);
> +
> + mutex_unlock(&adata->lock);
> +
> + 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);
> + mutex_init(&data->lock);
> + data->alt = alt;
> + data->port = port;
> + data->ap_mode_entry = ap_mode_entry;
> + data->sid = desc->svid;
> + data->mode = desc->mode;
> +
> + typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> + typec_altmode_set_drvdata(alt, data);
'data' is of type struct cros_typec_altmode_data here
> +
> + return alt;
> +}
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-12-11 0:21 ` Stephen Boyd
@ 2024-12-13 17:55 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-13 17:55 UTC (permalink / raw)
To: Stephen Boyd
Cc: chrome-platform, heikki.krogerus, linux-usb, tzungbi, akuchynski,
pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Greg Kroah-Hartman, linux-kernel
On Tue, Dec 10, 2024 at 4:21 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:13)
> > diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
> > new file mode 100644
> > index 000000000000..14e89e9a7691
> > --- /dev/null
> > +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> > @@ -0,0 +1,387 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
>
> Remove extra *, this isn't kerneldoc.
Done
>
> > + * USB Typec-C Thuderbolt3 Alternate Mode driver
> > + *
> > + * Copyright (C) 2019 Intel Corporation
> > + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > + */
> > +
> > +#include <linux/delay.h>
>
> Is this include used?
Compiles without it so I'm guessing no.
>
> > +#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>
>
> Please include workqueue.h
Done
>
> > +
> > +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 */
>
> What does it protect? The whole struct tbt_altmode?
This looks like it's protecting control flow (enter/exit/vdm can all
be triggered on probe, via .activate or potentially autonomously via
port driver triggering the alt-mode).
>
> > +};
> [...]
> > +
> > +/* MUST HOLD tbt->lock.
>
> Use lockdep_assert_held(tbt->lock) and remove the comment?
Done.
>
> > + *
> > + * If SOP' is available, enter that first (which will trigger a VDM response
> > + * that will enter SOP" if available and then the port). If entering SOP' fails,
> > + * stop attempting to enter either cable altmode (probably not supported) and
> > + * directly enter the port altmode.
> > + */
> > +static int tbt_enter_modes_ordered(struct typec_altmode *alt)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > + int ret = 0;
> > +
> > + if (!tbt_ready(tbt->alt))
> > + return -ENODEV;
> > +
> > + if (tbt->plug[TYPEC_PLUG_SOP_P]) {
> > + ret = typec_cable_altmode_enter(alt, TYPEC_PLUG_SOP_P, NULL);
> > + if (ret < 0) {
> > + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> > + if (tbt->plug[i])
> > + typec_altmode_put_plug(tbt->plug[i]);
> > +
> > + tbt->plug[i] = NULL;
> > + }
> > + } else {
> > + return ret;
> > + }
> > + }
> > +
> > + return tbt_enter_mode(tbt);
> > +}
> > +
> > +static int tbt_cable_altmode_vdm(struct typec_altmode *alt,
> > + enum typec_plug_index sop, const u32 hdr,
> > + const u32 *vdo, int count)
> > +{
> [...]
> > + case CMD_EXIT_MODE:
> > + /* Exit in opposite order: Port, SOP", then SOP'. */
> > + if (sop == TYPEC_PLUG_SOP_PP)
> > + tbt->state = TBT_STATE_SOP_P_EXIT;
> > + break;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + if (tbt->state != TBT_STATE_IDLE)
> > + schedule_work(&tbt->work);
> > +
> > +
>
> Nitpick: Why two newlines?
Clang format missed it :(
>
> > + mutex_unlock(&tbt->lock);
> > + return 0;
> > +}
> > +
> [...]
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index febe453b96be..b5e67a57762c 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -458,7 +458,8 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
> > struct typec_altmode *adev = to_typec_altmode(kobj_to_dev(kobj));
> >
> > if (attr == &dev_attr_active.attr)
> > - if (!adev->ops || !adev->ops->activate)
> > + if (!is_typec_port(adev->dev.parent) &&
> > + (!adev->ops || !adev->ops->activate))
> > return 0444;
> >
> > return attr->mode;
> > @@ -563,7 +564,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.active = !desc->inactive; /* Enabled by default */
> > }
> >
> > sprintf(alt->group_name, "mode%d", desc->mode);
> > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > index d616b8807000..252af3f77039 100644
> > --- a/include/linux/usb/typec.h
> > +++ b/include/linux/usb/typec.h
> > @@ -140,6 +140,7 @@ int typec_cable_set_identity(struct typec_cable *cable);
> > * @mode: Index of the Mode
> > * @vdo: VDO returned by Discover Modes USB PD command
> > * @roles: Only for ports. DRP if the mode is available in both roles
> > + * @inactive: Only for ports. Make this port inactive (default is active).
> > *
> > * Description of an Alternate Mode which a connector, cable plug or partner
> > * supports.
> > @@ -150,6 +151,7 @@ struct typec_altmode_desc {
> > u32 vdo;
> > /* Only used with ports */
> > enum typec_port_data roles;
> > + bool inactive;
> > };
> >
> > void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
>
> These two files look like they can be a different patch? Or the commit
> text can describe these changes.
I think earlier in the series, they were its own patch -- got merged
down into this over several refactors. I'll pull it out into its own
patch.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 4/7] platform/chrome: cros_ec_typec: Update partner altmode active
2024-12-10 23:32 ` Stephen Boyd
@ 2024-12-13 18:01 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-13 18:01 UTC (permalink / raw)
To: Stephen Boyd
Cc: chrome-platform, heikki.krogerus, linux-usb, tzungbi, akuchynski,
pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Benson Leung, Guenter Roeck, linux-kernel
On Tue, Dec 10, 2024 at 3:32 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:15)
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index c7781aea0b88..e3eabe5e42ac 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -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) {
>
> This can just be list_for_each_entry() because the list isn't changing
> during iteration.
Done
>
> > + 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);
> > + }
>
> It could also be shorter:
>
> list_for_each_entry(node, &port->partner_mode_list, list) {
> typec_altmode_update_active(node->amode,
> port->state.alt && node->amode->svid == port->state.alt->svid);
> }
Done
>
> As far as I can tell, cros_typec_configure_mux() is called when the HPD
> state changes. We'll iterate through here unnecessarily in that case.
> Can that be avoided somehow?
Writing the same value to `typec_altmode_update_active` is a no-op.
I'd prefer to leave this code as-is since it's quite simple (having to
determine HPD vs non-HPD, whether DP is currently active, etc. is
going to be more work than it saves from not calling this loop).
>
> > +
> > mux_ack:
> > if (!typec->needs_mux_ack)
> > return ret;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-12-11 0:08 ` Stephen Boyd
@ 2024-12-13 18:29 ` Abhishek Pandit-Subedi
2024-12-16 19:46 ` Stephen Boyd
0 siblings, 1 reply; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-13 18:29 UTC (permalink / raw)
To: Stephen Boyd
Cc: chrome-platform, heikki.krogerus, linux-usb, tzungbi, akuchynski,
pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Benson Leung, Guenter Roeck, linux-kernel
On Tue, Dec 10, 2024 at 4:08 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index e3eabe5e42ac..0f3bc335f583 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"
> >
> > @@ -290,15 +291,15 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> > struct typec_altmode *amode;
> >
> > /* All PD capable CrOS devices are assumed to support DP altmode. */
> > + memset(&desc, 0, sizeof(desc));
> > 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;
> > - typec_altmode_set_drvdata(amode, port);
> > - amode->ops = &port_amode_ops;
> >
> > /*
> > * Register TBT compatibility alt mode. The EC will not enter the mode
> > @@ -575,6 +576,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,
>
> Should we forward the return value from here?
Done
>
> > + port->state.data);
> > +
> > return ret;
> > }
> >
> > @@ -1254,6 +1259,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;
>
> Do we need to stash this? Can we cros_ec_check_features() in
> cros_typec_init_ports() and pass the bool to
> cros_typec_register_port_altmodes() instead to save a struct member?
We don't need to stash this but it keeps the feature checks
consistently in `cros_typec_probe`.
>
> > };
> >
> > /* 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..bb7c7ad2ff6e
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > @@ -0,0 +1,281 @@
> > +// 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>
>
> Please include workqueue.h, mutex.h, etc. for things used in this file.
Done. Btw, is there a script that does this for you in the kernel like
include-what-you-use does for userspace?
>
> > +
> > +#include "cros_typec_altmode.h"
> > +
> > +struct cros_typec_altmode_data {
> > + struct work_struct work;
> > + struct cros_typec_port *port;
> > + struct typec_altmode *alt;
> > + bool ap_mode_entry;
>
> The UCSI driver (drivers/usb/typec/ucsi/displayport.c) calls this
> 'override', can it be named the same thing? I also see that the UCSI
> driver has two bools, 'override' and 'initialized', which seems to be to
> support a DP_CMD_CONFIGURE that will respond with an ACK and then the
> next DP_CMD_CONFIGURE will call ucsi_altmode_update_active() to set the
> altmode as active. Maybe the same method can be followed here so that on
> older chromebooks where EC is in control of mode entry we can emulate
> entering the mode?
The reason it's called `override` in UCSI is because the feature is
called "alternate mode override supported". When this optional bit is
set, the UCSI method "SET_NEW_CAM" can be used to change what
alternate mode is active. However, it behaves differently from cros_ec
because even when override is set, the PD controller can/will still
autonomously enter a mode on connection. Whereas on cros_ec_typec, if
you set "ap-driven-mode", the EC will not enter any modes until the AP
tells it to.
Also, the reason the UCSI driver does the DP_CMD_CONFIGURE dance is
because the UCSI command, SET_NEW_CAM, requires the DP configuration
VDO as a parameter. Since UCSI doesn't define a VDM mechanism, the
UCSI driver fakes the ".entry" call and then uses the first
DP_CONFIGURE to do the actual entry. This also doesn't match the
cros_ec driver (either ap-driven or not) because the interface does
not allow setting the DP VDO at all. DP_CONFIGURE will be generated
and consumed entirely within the EC and all we can really use is the
mux update to generate a status update for the DP state machine.
Right now, EC driven Chromebooks will simply report active/inactive
for DP without reporting any configuration/status info. If you want to
handle DP_CONFIGURE and DP_STATUS from the altmode driver, you'll need
to fake the interactions in the port driver in a subsequent CL.
>
> > +
> > + struct mutex lock;
> > + u32 header;
> > + u32 *vdo_data;
> > + u8 vdo_size;
> > +
> > + u16 sid;
> > + u8 mode;
> > +};
> > +
> > +struct cros_typec_dp_data {
> > + struct cros_typec_altmode_data adata;
> > + struct typec_displayport_data data;
> > + bool configured;
> > + bool pending_status_update;
> > +};
> > +
> > +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);
> > +
> > + mutex_lock(&data->lock);
> > +
> > + 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);
>
> These printks need newlines.
Done.
>
> dev_err(&data->alt->dev, "VDM 0x%x failed\n", data->header);
>
> > +
> > + data->header = 0;
> > + data->vdo_data = NULL;
> > + data->vdo_size = 0;
> > +
> > + mutex_unlock(&data->lock);
> > +}
> > +
> > +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) {
> > + dev_warn(&alt->dev,
> > + "EC does not support AP driven mode entry\n");
>
> Like this one.
>
> > + 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,
>
> Do we plan to delete drivers/platform/chrome/cros_typec_vdm.c file at
> some point? I ask because 'port_amode_ops' becomes unused after this
> series.
Yes - I don't think we ever launched with the VDM request/response
feature enabled. I was going to do it as a follow-up to this CL to
handle attention.
>
> > + &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;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + 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);
> > +
> > + mutex_unlock(&data->lock);
> > + 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) {
> > + dev_warn(&alt->dev,
> > + "EC does not support AP driven mode exit\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;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + 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);
> > +
> > + mutex_unlock(&data->lock);
> > + return ret;
> > +}
> > +
> > +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> > + const u32 *data, int count)
> > +{
> > + struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt);
> > + struct cros_typec_altmode_data *adata = &dp_data->adata;
> > +
> > +
> > + 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;
> > +
> > + mutex_lock(&adata->lock);
> > +
> > + 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:
> > + dp_data->data.conf = *data;
> > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > + dp_data->configured = true;
> > + schedule_work(&adata->work);
> > + break;
> > + case DP_CMD_STATUS_UPDATE:
> > + dp_data->pending_status_update = true;
> > + break;
> > + default:
> > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > + schedule_work(&adata->work);
> > + break;
> > + }
> > +
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + mutex_unlock(&adata->lock);
> > + 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->ap_mode_entry)
> > + return -EOPNOTSUPP;
> > +
> > + 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_dp_data *dp_data =
> > + typec_altmode_get_drvdata(altmode);
> > + struct cros_typec_altmode_data *adata = &dp_data->adata;
> > +
> > + if (!dp_data->pending_status_update) {
> > + dev_dbg(&altmode->dev,
> > + "Got DPStatus without a pending request");
> > + return 0;
> > + }
> > +
> > + if (dp_data->configured && dp_data->data.conf != data->conf)
> > + dev_dbg(&altmode->dev,
> > + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> > + dp_data->data.conf, data->conf);
> > +
> > + mutex_lock(&adata->lock);
> > +
> > + dp_data->data = *data;
> > + dp_data->pending_status_update = false;
> > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > + adata->vdo_data = &dp_data->data.status;
> > + adata->vdo_size = 2;
> > + schedule_work(&adata->work);
> > +
> > + mutex_unlock(&adata->lock);
> > +
> > + 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;
>
> Can you name it 'adata' consistently? That makes it easy to search for
> 'adata' in this file and know it's always talking about a struct
> cros_typec_altmode_data type of data.
Done
>
> > +
> > + 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);
> > + mutex_init(&data->lock);
> > + data->alt = alt;
> > + data->port = port;
> > + data->ap_mode_entry = ap_mode_entry;
> > + data->sid = desc->svid;
> > + data->mode = desc->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
> > 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__
>
> #include <linux/kconfig.h> for IS_ENABLED()
> #include <linux/usb/typec.h> for typec_port_register_altmode()
Done
>
> > +
> > +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);
> > +}
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-12-11 21:58 ` Stephen Boyd
@ 2024-12-13 18:33 ` Abhishek Pandit-Subedi
2024-12-16 19:49 ` Stephen Boyd
0 siblings, 1 reply; 21+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-13 18:33 UTC (permalink / raw)
To: Stephen Boyd
Cc: chrome-platform, heikki.krogerus, linux-usb, tzungbi, akuchynski,
pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Benson Leung, Guenter Roeck, linux-kernel
On Wed, Dec 11, 2024 at 1:58 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> > new file mode 100644
> > index 000000000000..bb7c7ad2ff6e
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > @@ -0,0 +1,281 @@
> [...]
> > +
> > +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_dp_data *dp_data =
> > + typec_altmode_get_drvdata(altmode);
>
> How does this work? I see that the type of the drvdata is struct
> cros_typec_altmode_data per the allocation in
> cros_typec_register_displayport(), but here we're treating it as the
> type struct cros_typec_dp_data, which has a struct
> cros_typec_altmode_data as the first member. The allocation is too small
> from what I can tell. The same problem looks to be there in
> cros_typec_displayport_vdm().
>
> > + struct cros_typec_altmode_data *adata = &dp_data->adata;
> > +
> > + if (!dp_data->pending_status_update) {
> > + dev_dbg(&altmode->dev,
> > + "Got DPStatus without a pending request");
> > + return 0;
> > + }
> > +
> > + if (dp_data->configured && dp_data->data.conf != data->conf)
> > + dev_dbg(&altmode->dev,
> > + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> > + dp_data->data.conf, data->conf);
> > +
> > + mutex_lock(&adata->lock);
> > +
> > + dp_data->data = *data;
> > + dp_data->pending_status_update = false;
> > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > + adata->vdo_data = &dp_data->data.status;
> > + adata->vdo_size = 2;
> > + schedule_work(&adata->work);
> > +
> > + mutex_unlock(&adata->lock);
> > +
> > + 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);
> > + mutex_init(&data->lock);
> > + data->alt = alt;
> > + data->port = port;
> > + data->ap_mode_entry = ap_mode_entry;
> > + data->sid = desc->svid;
> > + data->mode = desc->mode;
> > +
> > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> > + typec_altmode_set_drvdata(alt, data);
>
> 'data' is of type struct cros_typec_altmode_data here
This should have been allocated as cros_typec_dp_data. Missed during a
previous refactor that changed the type from a union to this format.
>
> > +
> > + return alt;
> > +}
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-12-13 18:29 ` Abhishek Pandit-Subedi
@ 2024-12-16 19:46 ` Stephen Boyd
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2024-12-16 19:46 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: chrome-platform, heikki.krogerus, linux-usb, tzungbi, akuchynski,
pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Benson Leung, Guenter Roeck, linux-kernel
Quoting Abhishek Pandit-Subedi (2024-12-13 10:29:51)
> On Tue, Dec 10, 2024 at 4:08 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> > > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> > > new file mode 100644
> > > index 000000000000..bb7c7ad2ff6e
> > > --- /dev/null
> > > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > > @@ -0,0 +1,281 @@
> > > +// 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>
> >
> > Please include workqueue.h, mutex.h, etc. for things used in this file.
> Done. Btw, is there a script that does this for you in the kernel like
> include-what-you-use does for userspace?
I'm not aware of one.
>
> >
> > > +
> > > +#include "cros_typec_altmode.h"
> > > +
> > > +struct cros_typec_altmode_data {
> > > + struct work_struct work;
> > > + struct cros_typec_port *port;
> > > + struct typec_altmode *alt;
> > > + bool ap_mode_entry;
> >
> > The UCSI driver (drivers/usb/typec/ucsi/displayport.c) calls this
> > 'override', can it be named the same thing? I also see that the UCSI
> > driver has two bools, 'override' and 'initialized', which seems to be to
> > support a DP_CMD_CONFIGURE that will respond with an ACK and then the
> > next DP_CMD_CONFIGURE will call ucsi_altmode_update_active() to set the
> > altmode as active. Maybe the same method can be followed here so that on
> > older chromebooks where EC is in control of mode entry we can emulate
> > entering the mode?
>
> The reason it's called `override` in UCSI is because the feature is
> called "alternate mode override supported". When this optional bit is
> set, the UCSI method "SET_NEW_CAM" can be used to change what
> alternate mode is active. However, it behaves differently from cros_ec
> because even when override is set, the PD controller can/will still
> autonomously enter a mode on connection. Whereas on cros_ec_typec, if
> you set "ap-driven-mode", the EC will not enter any modes until the AP
> tells it to.
Ok, got it.
>
> Also, the reason the UCSI driver does the DP_CMD_CONFIGURE dance is
> because the UCSI command, SET_NEW_CAM, requires the DP configuration
> VDO as a parameter. Since UCSI doesn't define a VDM mechanism, the
> UCSI driver fakes the ".entry" call and then uses the first
> DP_CONFIGURE to do the actual entry. This also doesn't match the
> cros_ec driver (either ap-driven or not) because the interface does
> not allow setting the DP VDO at all. DP_CONFIGURE will be generated
> and consumed entirely within the EC and all we can really use is the
> mux update to generate a status update for the DP state machine.
Is the 'initialized' dance the fake ".entry"? I want the not ap-driven
(ec-driven?) mode to work with this series, and specifically not print
an error message. I'm guessing that to do that we should fake ".entry"
when the EC is in control of the mode entry. Or is there some way to
jump the state machine forward for the port altmode so that it is
already entered? I think to get the device to show the pin configuration
and mode like "source" vs. "sink" or "usb" we need to spoof multiple
VDMs.
>
> Right now, EC driven Chromebooks will simply report active/inactive
> for DP without reporting any configuration/status info. If you want to
> handle DP_CONFIGURE and DP_STATUS from the altmode driver, you'll need
> to fake the interactions in the port driver in a subsequent CL.
>
Ok. I'd very much like to do that because I need to make the displayport
partner altmode driver work to the point that it calls
drm_connector_oob_hotplug_event(). Do we need to spoof a
DP_CMD_STATUS_UPDATE on mode entry when the EC indicates that DP mode is
entered on a port?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-12-13 18:33 ` Abhishek Pandit-Subedi
@ 2024-12-16 19:49 ` Stephen Boyd
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2024-12-16 19:49 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: chrome-platform, heikki.krogerus, linux-usb, tzungbi, akuchynski,
pmalani, jthies, dmitry.baryshkov, badhri, rdbabiera,
Benson Leung, Guenter Roeck, linux-kernel
Quoting Abhishek Pandit-Subedi (2024-12-13 10:33:19)
> On Wed, Dec 11, 2024 at 1:58 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> > > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> > > new file mode 100644
> > > index 000000000000..bb7c7ad2ff6e
> > > --- /dev/null
> > > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > > @@ -0,0 +1,281 @@
[...]
> > > +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);
> > > + mutex_init(&data->lock);
> > > + data->alt = alt;
> > > + data->port = port;
> > > + data->ap_mode_entry = ap_mode_entry;
> > > + data->sid = desc->svid;
> > > + data->mode = desc->mode;
> > > +
> > > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> > > + typec_altmode_set_drvdata(alt, data);
> >
> > 'data' is of type struct cros_typec_altmode_data here
> This should have been allocated as cros_typec_dp_data. Missed during a
> previous refactor that changed the type from a union to this format.
It would be good to have the cros_typec_altmode_data member be somewhere
besides the first member of cros_typec_dp_data so that this fails faster
when stashing the pointer into the drvdata and treating it as the wrong
type on the other side.
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-12-16 19:49 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-06 23:38 [PATCH v4 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
2024-12-11 9:40 ` Heikki Krogerus
2024-12-06 23:38 ` [PATCH v4 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
2024-12-09 4:28 ` kernel test robot
2024-12-11 0:21 ` Stephen Boyd
2024-12-13 17:55 ` Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 3/7] usb: typec: Print err when displayport fails to enter Abhishek Pandit-Subedi
2024-12-11 10:09 ` Heikki Krogerus
2024-12-06 23:38 ` [PATCH v4 4/7] platform/chrome: cros_ec_typec: Update partner altmode active Abhishek Pandit-Subedi
2024-12-10 23:32 ` Stephen Boyd
2024-12-13 18:01 ` Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
2024-12-11 0:08 ` Stephen Boyd
2024-12-13 18:29 ` Abhishek Pandit-Subedi
2024-12-16 19:46 ` Stephen Boyd
2024-12-11 21:58 ` Stephen Boyd
2024-12-13 18:33 ` Abhishek Pandit-Subedi
2024-12-16 19:49 ` Stephen Boyd
2024-12-06 23:38 ` [PATCH v4 6/7] platform/chrome: cros_ec_typec: Thunderbolt support Abhishek Pandit-Subedi
2024-12-06 23:38 ` [PATCH v4 7/7] platform/chrome: cros_ec_typec: Disable tbt on port Abhishek Pandit-Subedi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).