* [PATCH v3 1/7] usb: typec: Only use SVID for matching altmodes
2024-11-07 19:29 [PATCH v3 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
@ 2024-11-07 19:29 ` Abhishek Pandit-Subedi
2024-11-08 11:41 ` Heikki Krogerus
2024-11-07 19:29 ` [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
` (5 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-07 19:29 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: jthies, akuchynski, pmalani, dmitry.baryshkov,
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>
---
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.277.g8800431eea-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 1/7] usb: typec: Only use SVID for matching altmodes
2024-11-07 19:29 ` [PATCH v3 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
@ 2024-11-08 11:41 ` Heikki Krogerus
0 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2024-11-08 11:41 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: tzungbi, linux-usb, chrome-platform, jthies, akuchynski, pmalani,
dmitry.baryshkov, Greg Kroah-Hartman, Masahiro Yamada,
Nathan Chancellor, Nicolas Schier, linux-kbuild, linux-kernel
On Thu, Nov 07, 2024 at 11:29:54AM -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>
> ---
>
> 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.277.g8800431eea-goog
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-11-07 19:29 [PATCH v3 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
2024-11-07 19:29 ` [PATCH v3 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
@ 2024-11-07 19:29 ` Abhishek Pandit-Subedi
2024-11-09 6:21 ` Dmitry Baryshkov
2024-11-22 18:50 ` Benson Leung
2024-11-07 19:29 ` [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe Abhishek Pandit-Subedi
` (4 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-07 19:29 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: jthies, akuchynski, pmalani, dmitry.baryshkov,
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:
* Delay cable + plug checks so that the module doesn't fail to probe
if cable + plug information isn't available by the time the partner
altmode is registered.
* Remove unncessary brace after if (IS_ERR(plug))
The rest of this patch should be the same as Heikki's original RFC.
Changes in 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
drivers/usb/typec/altmodes/Kconfig | 9 +
drivers/usb/typec/altmodes/Makefile | 2 +
drivers/usb/typec/altmodes/thunderbolt.c | 308 +++++++++++++++++++++++
include/linux/usb/typec_tbt.h | 1 +
4 files changed, 320 insertions(+)
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..a945b9d35a1d
--- /dev/null
+++ b/drivers/usb/typec/altmodes/thunderbolt.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * USB Typec-C Thuderbolt3 Alternate Mode driver
+ *
+ * Copyright (C) 2019 Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/usb/pd_vdo.h>
+#include <linux/usb/typec_altmode.h>
+#include <linux/usb/typec_tbt.h>
+
+enum tbt_state {
+ TBT_STATE_IDLE,
+ TBT_STATE_SOP_P_ENTER,
+ TBT_STATE_SOP_PP_ENTER,
+ TBT_STATE_ENTER,
+ TBT_STATE_EXIT,
+ TBT_STATE_SOP_PP_EXIT,
+ TBT_STATE_SOP_P_EXIT
+};
+
+struct tbt_altmode {
+ enum tbt_state state;
+ struct typec_cable *cable;
+ struct typec_altmode *alt;
+ struct typec_altmode *plug[2];
+ u32 enter_vdo;
+
+ struct work_struct work;
+ struct mutex lock; /* device lock */
+};
+
+static bool tbt_ready(struct typec_altmode *alt);
+
+static int tbt_enter_mode(struct tbt_altmode *tbt)
+{
+ struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
+ u32 vdo;
+
+ vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
+ vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
+ vdo |= TBT_MODE;
+
+ if (plug) {
+ if (typec_cable_is_active(tbt->cable))
+ vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
+
+ vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
+ vdo |= plug->vdo & TBT_CABLE_ROUNDED;
+ vdo |= plug->vdo & TBT_CABLE_OPTICAL;
+ vdo |= plug->vdo & TBT_CABLE_RETIMER;
+ vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
+ } else {
+ vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
+ }
+
+ tbt->enter_vdo = vdo;
+ return typec_altmode_enter(tbt->alt, &vdo);
+}
+
+static void tbt_altmode_work(struct work_struct *work)
+{
+ struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
+ int ret;
+
+ mutex_lock(&tbt->lock);
+
+ switch (tbt->state) {
+ case TBT_STATE_SOP_P_ENTER:
+ ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
+ if (ret)
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
+ "failed to enter mode (%d)\n", ret);
+ break;
+ case TBT_STATE_SOP_PP_ENTER:
+ ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
+ if (ret)
+ dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
+ "failed to enter mode (%d)\n", ret);
+ break;
+ case TBT_STATE_ENTER:
+ ret = tbt_enter_mode(tbt);
+ if (ret)
+ dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
+ ret);
+ break;
+ case TBT_STATE_EXIT:
+ typec_altmode_exit(tbt->alt);
+ break;
+ case TBT_STATE_SOP_PP_EXIT:
+ typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
+ break;
+ case TBT_STATE_SOP_P_EXIT:
+ typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
+ break;
+ default:
+ break;
+ }
+
+ tbt->state = TBT_STATE_IDLE;
+
+ mutex_unlock(&tbt->lock);
+}
+
+static int tbt_altmode_vdm(struct typec_altmode *alt,
+ const u32 hdr, const u32 *vdo, int count)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ int cmd_type = PD_VDO_CMDT(hdr);
+ int cmd = PD_VDO_CMD(hdr);
+
+ mutex_lock(&tbt->lock);
+
+ if (tbt->state != TBT_STATE_IDLE) {
+ mutex_unlock(&tbt->lock);
+ return -EBUSY;
+ }
+
+ switch (cmd_type) {
+ case CMDT_RSP_ACK:
+ switch (cmd) {
+ case CMD_ENTER_MODE:
+ /*
+ * Following the order describeded in USB Type-C Spec
+ * R2.0 Section 6.7.3.
+ */
+ if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_ENTER;
+ else
+ tbt->state = TBT_STATE_ENTER;
+ } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
+ tbt->state = TBT_STATE_ENTER;
+ } else {
+ struct typec_thunderbolt_data data;
+
+ data.device_mode = tbt->alt->vdo;
+ data.cable_mode =
+ tbt->plug[TYPEC_PLUG_SOP_P] ?
+ tbt->plug[TYPEC_PLUG_SOP_P]
+ ->vdo :
+ 0;
+ data.enter_vdo = tbt->enter_vdo;
+
+ typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
+ }
+ break;
+ case CMD_EXIT_MODE:
+ if (alt == tbt->alt) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_EXIT;
+ else if (tbt->plug[TYPEC_PLUG_SOP_P])
+ tbt->state = TBT_STATE_SOP_P_EXIT;
+ } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
+ tbt->state = TBT_STATE_SOP_P_EXIT;
+ }
+ break;
+ }
+ break;
+ case CMDT_RSP_NAK:
+ switch (cmd) {
+ case CMD_ENTER_MODE:
+ dev_warn(&alt->dev, "Enter Mode refused\n");
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+
+ if (tbt->state != TBT_STATE_IDLE)
+ schedule_work(&tbt->work);
+
+ mutex_unlock(&tbt->lock);
+
+ return 0;
+}
+
+static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ int ret;
+
+ mutex_lock(&tbt->lock);
+
+ if (!tbt_ready(alt))
+ return -ENODEV;
+
+ /* Preventing the user space from entering/exiting the cable alt mode */
+ if (alt != tbt->alt)
+ ret = -EPERM;
+ else if (activate)
+ ret = tbt_enter_mode(tbt);
+ else
+ ret = typec_altmode_exit(alt);
+
+ mutex_unlock(&tbt->lock);
+
+ return ret;
+}
+
+static const struct typec_altmode_ops tbt_altmode_ops = {
+ .vdm = tbt_altmode_vdm,
+ .activate = tbt_altmode_activate
+};
+
+static int tbt_altmode_probe(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt;
+
+ tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
+ if (!tbt)
+ return -ENOMEM;
+
+ INIT_WORK(&tbt->work, tbt_altmode_work);
+ mutex_init(&tbt->lock);
+ tbt->alt = alt;
+
+ alt->desc = "Thunderbolt3";
+ typec_altmode_set_drvdata(alt, tbt);
+ typec_altmode_set_ops(alt, &tbt_altmode_ops);
+
+ if (tbt_ready(alt)) {
+ if (tbt->plug[TYPEC_PLUG_SOP_PP])
+ tbt->state = TBT_STATE_SOP_PP_ENTER;
+ else if (tbt->plug[TYPEC_PLUG_SOP_P])
+ tbt->state = TBT_STATE_SOP_P_ENTER;
+ else
+ tbt->state = TBT_STATE_ENTER;
+ schedule_work(&tbt->work);
+ }
+
+ return 0;
+}
+
+static void tbt_altmode_remove(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+
+ for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
+ if (tbt->plug[i])
+ typec_altmode_put_plug(tbt->plug[i]);
+ }
+
+ if (tbt->cable)
+ typec_cable_put(tbt->cable);
+}
+
+static bool tbt_ready(struct typec_altmode *alt)
+{
+ struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
+ struct typec_altmode *plug;
+
+ if (tbt->cable)
+ return true;
+
+ /* Thundebolt 3 requires a cable with eMarker */
+ tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
+ if (!tbt->cable)
+ return false;
+
+ /* We accept systems without SOP' or SOP''. This means the port altmode
+ * driver will be responsible for properly ordering entry/exit.
+ */
+ for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
+ plug = typec_altmode_get_plug(tbt->alt, i);
+ if (IS_ERR(plug))
+ continue;
+
+ if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
+ break;
+
+ plug->desc = "Thunderbolt3";
+ plug->ops = &tbt_altmode_ops;
+ typec_altmode_set_drvdata(plug, tbt);
+
+ tbt->plug[i] = plug;
+ }
+
+ return true;
+}
+
+static const struct typec_device_id tbt_typec_id[] = {
+ { USB_TYPEC_TBT_SID },
+ { }
+};
+MODULE_DEVICE_TABLE(typec, tbt_typec_id);
+
+static struct typec_altmode_driver tbt_altmode_driver = {
+ .id_table = tbt_typec_id,
+ .probe = tbt_altmode_probe,
+ .remove = tbt_altmode_remove,
+ .driver = {
+ .name = "typec-thunderbolt",
+ .owner = THIS_MODULE,
+ }
+};
+module_typec_altmode_driver(tbt_altmode_driver);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
index fa97d7e00f5c..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.277.g8800431eea-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-11-07 19:29 ` [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
@ 2024-11-09 6:21 ` Dmitry Baryshkov
2024-11-14 3:51 ` Abhishek Pandit-Subedi
2024-11-22 18:50 ` Benson Leung
1 sibling, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2024-11-09 6:21 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Greg Kroah-Hartman, linux-kernel
On Thu, Nov 07, 2024 at 11:29:55AM -0800, Abhishek Pandit-Subedi wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
> Thunderbolt 3 Alternate Mode entry flow is described in
> USB Type-C Specification Release 2.0.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes:
> * Delay cable + plug checks so that the module doesn't fail to probe
> if cable + plug information isn't available by the time the partner
> altmode is registered.
> * Remove unncessary brace after if (IS_ERR(plug))
>
> The rest of this patch should be the same as Heikki's original RFC.
>
>
> Changes in 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
>
> drivers/usb/typec/altmodes/Kconfig | 9 +
> drivers/usb/typec/altmodes/Makefile | 2 +
> drivers/usb/typec/altmodes/thunderbolt.c | 308 +++++++++++++++++++++++
> include/linux/usb/typec_tbt.h | 1 +
> 4 files changed, 320 insertions(+)
> 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..a945b9d35a1d
> --- /dev/null
> +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * USB Typec-C Thuderbolt3 Alternate Mode driver
> + *
> + * Copyright (C) 2019 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/usb/pd_vdo.h>
> +#include <linux/usb/typec_altmode.h>
> +#include <linux/usb/typec_tbt.h>
> +
> +enum tbt_state {
> + TBT_STATE_IDLE,
> + TBT_STATE_SOP_P_ENTER,
> + TBT_STATE_SOP_PP_ENTER,
> + TBT_STATE_ENTER,
> + TBT_STATE_EXIT,
> + TBT_STATE_SOP_PP_EXIT,
> + TBT_STATE_SOP_P_EXIT
> +};
> +
> +struct tbt_altmode {
> + enum tbt_state state;
> + struct typec_cable *cable;
> + struct typec_altmode *alt;
> + struct typec_altmode *plug[2];
> + u32 enter_vdo;
> +
> + struct work_struct work;
> + struct mutex lock; /* device lock */
> +};
> +
> +static bool tbt_ready(struct typec_altmode *alt);
> +
> +static int tbt_enter_mode(struct tbt_altmode *tbt)
> +{
> + struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
> + u32 vdo;
> +
> + vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
> + vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
> + vdo |= TBT_MODE;
> +
> + if (plug) {
> + if (typec_cable_is_active(tbt->cable))
> + vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
> +
> + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
> + vdo |= plug->vdo & TBT_CABLE_ROUNDED;
> + vdo |= plug->vdo & TBT_CABLE_OPTICAL;
> + vdo |= plug->vdo & TBT_CABLE_RETIMER;
> + vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
> + } else {
> + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
> + }
> +
> + tbt->enter_vdo = vdo;
> + return typec_altmode_enter(tbt->alt, &vdo);
> +}
> +
> +static void tbt_altmode_work(struct work_struct *work)
> +{
> + struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
> + int ret;
> +
> + mutex_lock(&tbt->lock);
> +
> + switch (tbt->state) {
> + case TBT_STATE_SOP_P_ENTER:
> + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
typec_cable_altmode_enter() ?
> + if (ret)
> + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
> + "failed to enter mode (%d)\n", ret);
> + break;
> + case TBT_STATE_SOP_PP_ENTER:
> + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
typec_cable_altmode_enter() ?
> + if (ret)
> + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
> + "failed to enter mode (%d)\n", ret);
> + break;
> + case TBT_STATE_ENTER:
> + ret = tbt_enter_mode(tbt);
> + if (ret)
> + dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
> + ret);
> + break;
> + case TBT_STATE_EXIT:
> + typec_altmode_exit(tbt->alt);
> + break;
> + case TBT_STATE_SOP_PP_EXIT:
> + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
> + break;
> + case TBT_STATE_SOP_P_EXIT:
> + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
> + break;
> + default:
> + break;
> + }
> +
> + tbt->state = TBT_STATE_IDLE;
> +
> + mutex_unlock(&tbt->lock);
> +}
> +
> +static int tbt_altmode_vdm(struct typec_altmode *alt,
> + const u32 hdr, const u32 *vdo, int count)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int cmd_type = PD_VDO_CMDT(hdr);
> + int cmd = PD_VDO_CMD(hdr);
> +
> + mutex_lock(&tbt->lock);
> +
> + if (tbt->state != TBT_STATE_IDLE) {
> + mutex_unlock(&tbt->lock);
> + return -EBUSY;
> + }
> +
> + switch (cmd_type) {
> + case CMDT_RSP_ACK:
> + switch (cmd) {
> + case CMD_ENTER_MODE:
> + /*
> + * Following the order describeded in USB Type-C Spec
> + * R2.0 Section 6.7.3.
> + */
> + if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_ENTER;
> + else
> + tbt->state = TBT_STATE_ENTER;
> + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> + tbt->state = TBT_STATE_ENTER;
> + } else {
> + struct typec_thunderbolt_data data;
> +
> + data.device_mode = tbt->alt->vdo;
> + data.cable_mode =
> + tbt->plug[TYPEC_PLUG_SOP_P] ?
> + tbt->plug[TYPEC_PLUG_SOP_P]
> + ->vdo :
I'd say, please move the -> vdo to the previous line, otherwise it's a
bit unreadable.
> + 0;
> + data.enter_vdo = tbt->enter_vdo;
> +
> + typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
> + }
> + break;
> + case CMD_EXIT_MODE:
> + if (alt == tbt->alt) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_EXIT;
> + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + }
> + break;
> + }
> + break;
> + case CMDT_RSP_NAK:
> + switch (cmd) {
> + case CMD_ENTER_MODE:
> + dev_warn(&alt->dev, "Enter Mode refused\n");
> + break;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + if (tbt->state != TBT_STATE_IDLE)
> + schedule_work(&tbt->work);
> +
> + mutex_unlock(&tbt->lock);
> +
> + return 0;
> +}
> +
> +static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int ret;
> +
> + mutex_lock(&tbt->lock);
> +
> + if (!tbt_ready(alt))
> + return -ENODEV;
> +
> + /* Preventing the user space from entering/exiting the cable alt mode */
> + if (alt != tbt->alt)
> + ret = -EPERM;
> + else if (activate)
> + ret = tbt_enter_mode(tbt);
> + else
> + ret = typec_altmode_exit(alt);
> +
> + mutex_unlock(&tbt->lock);
> +
> + return ret;
> +}
> +
> +static const struct typec_altmode_ops tbt_altmode_ops = {
> + .vdm = tbt_altmode_vdm,
> + .activate = tbt_altmode_activate
> +};
> +
> +static int tbt_altmode_probe(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt;
> +
> + tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
> + if (!tbt)
> + return -ENOMEM;
> +
> + INIT_WORK(&tbt->work, tbt_altmode_work);
> + mutex_init(&tbt->lock);
> + tbt->alt = alt;
> +
> + alt->desc = "Thunderbolt3";
> + typec_altmode_set_drvdata(alt, tbt);
> + typec_altmode_set_ops(alt, &tbt_altmode_ops);
> +
> + if (tbt_ready(alt)) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_ENTER;
> + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> + tbt->state = TBT_STATE_SOP_P_ENTER;
> + else
> + tbt->state = TBT_STATE_ENTER;
> + schedule_work(&tbt->work);
> + }
> +
> + return 0;
> +}
> +
> +static void tbt_altmode_remove(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> +
> + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> + if (tbt->plug[i])
> + typec_altmode_put_plug(tbt->plug[i]);
> + }
> +
> + if (tbt->cable)
> + typec_cable_put(tbt->cable);
> +}
> +
> +static bool tbt_ready(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + struct typec_altmode *plug;
> +
> + if (tbt->cable)
> + return true;
> +
> + /* Thundebolt 3 requires a cable with eMarker */
> + tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
> + if (!tbt->cable)
> + return false;
> +
> + /* We accept systems without SOP' or SOP''. This means the port altmode
> + * driver will be responsible for properly ordering entry/exit.
> + */
> + for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
> + plug = typec_altmode_get_plug(tbt->alt, i);
> + if (IS_ERR(plug))
> + continue;
> +
> + if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
!= USB_TYPEC_TBT_SID
> + break;
> +
> + plug->desc = "Thunderbolt3";
> + plug->ops = &tbt_altmode_ops;
Should this be plug->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",
> + .owner = THIS_MODULE,
Should not be necessary, it is set by __typec_altmode_register_driver()
> + }
> +};
> +module_typec_altmode_driver(tbt_altmode_driver);
> +
> +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
> diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
> index fa97d7e00f5c..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.277.g8800431eea-goog
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-11-09 6:21 ` Dmitry Baryshkov
@ 2024-11-14 3:51 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-14 3:51 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Greg Kroah-Hartman, linux-kernel
On Fri, Nov 8, 2024 at 10:21 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, Nov 07, 2024 at 11:29:55AM -0800, Abhishek Pandit-Subedi wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > Thunderbolt 3 Alternate Mode entry flow is described in
> > USB Type-C Specification Release 2.0.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes:
> > * Delay cable + plug checks so that the module doesn't fail to probe
> > if cable + plug information isn't available by the time the partner
> > altmode is registered.
> > * Remove unncessary brace after if (IS_ERR(plug))
> >
> > The rest of this patch should be the same as Heikki's original RFC.
> >
> >
> > Changes in 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
> >
> > drivers/usb/typec/altmodes/Kconfig | 9 +
> > drivers/usb/typec/altmodes/Makefile | 2 +
> > drivers/usb/typec/altmodes/thunderbolt.c | 308 +++++++++++++++++++++++
> > include/linux/usb/typec_tbt.h | 1 +
> > 4 files changed, 320 insertions(+)
> > 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..a945b9d35a1d
> > --- /dev/null
> > +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> > @@ -0,0 +1,308 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * USB Typec-C Thuderbolt3 Alternate Mode driver
> > + *
> > + * Copyright (C) 2019 Intel Corporation
> > + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/mutex.h>
> > +#include <linux/module.h>
> > +#include <linux/usb/pd_vdo.h>
> > +#include <linux/usb/typec_altmode.h>
> > +#include <linux/usb/typec_tbt.h>
> > +
> > +enum tbt_state {
> > + TBT_STATE_IDLE,
> > + TBT_STATE_SOP_P_ENTER,
> > + TBT_STATE_SOP_PP_ENTER,
> > + TBT_STATE_ENTER,
> > + TBT_STATE_EXIT,
> > + TBT_STATE_SOP_PP_EXIT,
> > + TBT_STATE_SOP_P_EXIT
> > +};
> > +
> > +struct tbt_altmode {
> > + enum tbt_state state;
> > + struct typec_cable *cable;
> > + struct typec_altmode *alt;
> > + struct typec_altmode *plug[2];
> > + u32 enter_vdo;
> > +
> > + struct work_struct work;
> > + struct mutex lock; /* device lock */
> > +};
> > +
> > +static bool tbt_ready(struct typec_altmode *alt);
> > +
> > +static int tbt_enter_mode(struct tbt_altmode *tbt)
> > +{
> > + struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
> > + u32 vdo;
> > +
> > + vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
> > + vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
> > + vdo |= TBT_MODE;
> > +
> > + if (plug) {
> > + if (typec_cable_is_active(tbt->cable))
> > + vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
> > +
> > + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
> > + vdo |= plug->vdo & TBT_CABLE_ROUNDED;
> > + vdo |= plug->vdo & TBT_CABLE_OPTICAL;
> > + vdo |= plug->vdo & TBT_CABLE_RETIMER;
> > + vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
> > + } else {
> > + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
> > + }
> > +
> > + tbt->enter_vdo = vdo;
> > + return typec_altmode_enter(tbt->alt, &vdo);
> > +}
> > +
> > +static void tbt_altmode_work(struct work_struct *work)
> > +{
> > + struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
> > + int ret;
> > +
> > + mutex_lock(&tbt->lock);
> > +
> > + switch (tbt->state) {
> > + case TBT_STATE_SOP_P_ENTER:
> > + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
>
> typec_cable_altmode_enter() ?
>
> > + if (ret)
> > + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
> > + "failed to enter mode (%d)\n", ret);
> > + break;
> > + case TBT_STATE_SOP_PP_ENTER:
> > + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
>
> typec_cable_altmode_enter() ?
>
> > + if (ret)
> > + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
> > + "failed to enter mode (%d)\n", ret);
> > + break;
> > + case TBT_STATE_ENTER:
> > + ret = tbt_enter_mode(tbt);
> > + if (ret)
> > + dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
> > + ret);
> > + break;
> > + case TBT_STATE_EXIT:
> > + typec_altmode_exit(tbt->alt);
> > + break;
> > + case TBT_STATE_SOP_PP_EXIT:
> > + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
> > + break;
> > + case TBT_STATE_SOP_P_EXIT:
> > + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + tbt->state = TBT_STATE_IDLE;
> > +
> > + mutex_unlock(&tbt->lock);
> > +}
> > +
> > +static int tbt_altmode_vdm(struct typec_altmode *alt,
> > + const u32 hdr, const u32 *vdo, int count)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > + int cmd_type = PD_VDO_CMDT(hdr);
> > + int cmd = PD_VDO_CMD(hdr);
> > +
> > + mutex_lock(&tbt->lock);
> > +
> > + if (tbt->state != TBT_STATE_IDLE) {
> > + mutex_unlock(&tbt->lock);
> > + return -EBUSY;
> > + }
> > +
> > + switch (cmd_type) {
> > + case CMDT_RSP_ACK:
> > + switch (cmd) {
> > + case CMD_ENTER_MODE:
> > + /*
> > + * Following the order describeded in USB Type-C Spec
> > + * R2.0 Section 6.7.3.
> > + */
> > + if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
> > + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> > + tbt->state = TBT_STATE_SOP_PP_ENTER;
> > + else
> > + tbt->state = TBT_STATE_ENTER;
> > + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> > + tbt->state = TBT_STATE_ENTER;
> > + } else {
> > + struct typec_thunderbolt_data data;
> > +
> > + data.device_mode = tbt->alt->vdo;
> > + data.cable_mode =
> > + tbt->plug[TYPEC_PLUG_SOP_P] ?
> > + tbt->plug[TYPEC_PLUG_SOP_P]
> > + ->vdo :
>
> I'd say, please move the -> vdo to the previous line, otherwise it's a
> bit unreadable.
>
> > + 0;
> > + data.enter_vdo = tbt->enter_vdo;
> > +
> > + typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
> > + }
> > + break;
> > + case CMD_EXIT_MODE:
> > + if (alt == tbt->alt) {
> > + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> > + tbt->state = TBT_STATE_SOP_PP_EXIT;
> > + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> > + tbt->state = TBT_STATE_SOP_P_EXIT;
> > + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> > + tbt->state = TBT_STATE_SOP_P_EXIT;
> > + }
> > + break;
> > + }
> > + break;
> > + case CMDT_RSP_NAK:
> > + switch (cmd) {
> > + case CMD_ENTER_MODE:
> > + dev_warn(&alt->dev, "Enter Mode refused\n");
> > + break;
> > + default:
> > + break;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + if (tbt->state != TBT_STATE_IDLE)
> > + schedule_work(&tbt->work);
> > +
> > + mutex_unlock(&tbt->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > + int ret;
> > +
> > + mutex_lock(&tbt->lock);
> > +
> > + if (!tbt_ready(alt))
> > + return -ENODEV;
> > +
> > + /* Preventing the user space from entering/exiting the cable alt mode */
> > + if (alt != tbt->alt)
> > + ret = -EPERM;
> > + else if (activate)
> > + ret = tbt_enter_mode(tbt);
> > + else
> > + ret = typec_altmode_exit(alt);
> > +
> > + mutex_unlock(&tbt->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct typec_altmode_ops tbt_altmode_ops = {
> > + .vdm = tbt_altmode_vdm,
> > + .activate = tbt_altmode_activate
> > +};
> > +
> > +static int tbt_altmode_probe(struct typec_altmode *alt)
> > +{
> > + struct tbt_altmode *tbt;
> > +
> > + tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
> > + if (!tbt)
> > + return -ENOMEM;
> > +
> > + INIT_WORK(&tbt->work, tbt_altmode_work);
> > + mutex_init(&tbt->lock);
> > + tbt->alt = alt;
> > +
> > + alt->desc = "Thunderbolt3";
> > + typec_altmode_set_drvdata(alt, tbt);
> > + typec_altmode_set_ops(alt, &tbt_altmode_ops);
> > +
> > + if (tbt_ready(alt)) {
> > + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> > + tbt->state = TBT_STATE_SOP_PP_ENTER;
> > + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> > + tbt->state = TBT_STATE_SOP_P_ENTER;
> > + else
> > + tbt->state = TBT_STATE_ENTER;
> > + schedule_work(&tbt->work);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void tbt_altmode_remove(struct typec_altmode *alt)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > +
> > + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> > + if (tbt->plug[i])
> > + typec_altmode_put_plug(tbt->plug[i]);
> > + }
> > +
> > + if (tbt->cable)
> > + typec_cable_put(tbt->cable);
> > +}
> > +
> > +static bool tbt_ready(struct typec_altmode *alt)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > + struct typec_altmode *plug;
> > +
> > + if (tbt->cable)
> > + return true;
> > +
> > + /* Thundebolt 3 requires a cable with eMarker */
> > + tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
> > + if (!tbt->cable)
> > + return false;
> > +
> > + /* We accept systems without SOP' or SOP''. This means the port altmode
> > + * driver will be responsible for properly ordering entry/exit.
> > + */
> > + for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
> > + plug = typec_altmode_get_plug(tbt->alt, i);
> > + if (IS_ERR(plug))
> > + continue;
> > +
> > + if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
>
> != USB_TYPEC_TBT_SID
>
> > + break;
> > +
> > + plug->desc = "Thunderbolt3";
> > + plug->ops = &tbt_altmode_ops;
>
> Should this be plug->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",
> > + .owner = THIS_MODULE,
>
> Should not be necessary, it is set by __typec_altmode_register_driver()
>
> > + }
> > +};
> > +module_typec_altmode_driver(tbt_altmode_driver);
> > +
> > +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
> > diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
> > index fa97d7e00f5c..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.277.g8800431eea-goog
> >
>
> --
> With best wishes
> Dmitry
Will address all comments in the next series.
The cable alt-modes are new to me. Looking at the implementation, one
part confuses me -- it looks like cable alt-modes expect the partner
to already be active when entering
(https://github.com/torvalds/linux/blob/master/drivers/usb/typec/bus.c#L272).
I'll reach out to the author of cable altmodes to see how to integrate
this correctly with the current series.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-11-07 19:29 ` [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
2024-11-09 6:21 ` Dmitry Baryshkov
@ 2024-11-22 18:50 ` Benson Leung
2024-12-06 22:27 ` Abhishek Pandit-Subedi
1 sibling, 1 reply; 24+ messages in thread
From: Benson Leung @ 2024-11-22 18:50 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, dmitry.baryshkov, Greg Kroah-Hartman,
linux-kernel, danielgeorgem
[-- Attachment #1: Type: text/plain, Size: 12138 bytes --]
Hi Abhishek,
On Thu, Nov 07, 2024 at 11:29:55AM -0800, Abhishek Pandit-Subedi wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
> Thunderbolt 3 Alternate Mode entry flow is described in
> USB Type-C Specification Release 2.0.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes:
> * Delay cable + plug checks so that the module doesn't fail to probe
> if cable + plug information isn't available by the time the partner
> altmode is registered.
> * Remove unncessary brace after if (IS_ERR(plug))
>
> The rest of this patch should be the same as Heikki's original RFC.
>
>
> Changes in 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
>
> drivers/usb/typec/altmodes/Kconfig | 9 +
> drivers/usb/typec/altmodes/Makefile | 2 +
> drivers/usb/typec/altmodes/thunderbolt.c | 308 +++++++++++++++++++++++
> include/linux/usb/typec_tbt.h | 1 +
> 4 files changed, 320 insertions(+)
> 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..a945b9d35a1d
> --- /dev/null
> +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * USB Typec-C Thuderbolt3 Alternate Mode driver
> + *
> + * Copyright (C) 2019 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/usb/pd_vdo.h>
> +#include <linux/usb/typec_altmode.h>
> +#include <linux/usb/typec_tbt.h>
> +
> +enum tbt_state {
> + TBT_STATE_IDLE,
> + TBT_STATE_SOP_P_ENTER,
> + TBT_STATE_SOP_PP_ENTER,
> + TBT_STATE_ENTER,
> + TBT_STATE_EXIT,
> + TBT_STATE_SOP_PP_EXIT,
> + TBT_STATE_SOP_P_EXIT
> +};
> +
> +struct tbt_altmode {
> + enum tbt_state state;
> + struct typec_cable *cable;
> + struct typec_altmode *alt;
> + struct typec_altmode *plug[2];
> + u32 enter_vdo;
> +
> + struct work_struct work;
> + struct mutex lock; /* device lock */
> +};
> +
> +static bool tbt_ready(struct typec_altmode *alt);
> +
> +static int tbt_enter_mode(struct tbt_altmode *tbt)
> +{
> + struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
> + u32 vdo;
> +
> + vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
> + vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
> + vdo |= TBT_MODE;
> +
> + if (plug) {
> + if (typec_cable_is_active(tbt->cable))
> + vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
> +
> + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
> + vdo |= plug->vdo & TBT_CABLE_ROUNDED;
> + vdo |= plug->vdo & TBT_CABLE_OPTICAL;
> + vdo |= plug->vdo & TBT_CABLE_RETIMER;
> + vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
> + } else {
> + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
> + }
> +
> + tbt->enter_vdo = vdo;
> + return typec_altmode_enter(tbt->alt, &vdo);
> +}
> +
> +static void tbt_altmode_work(struct work_struct *work)
> +{
> + struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
> + int ret;
> +
> + mutex_lock(&tbt->lock);
> +
> + switch (tbt->state) {
> + case TBT_STATE_SOP_P_ENTER:
> + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
> + if (ret)
> + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
> + "failed to enter mode (%d)\n", ret);
> + break;
> + case TBT_STATE_SOP_PP_ENTER:
> + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
> + if (ret)
> + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
> + "failed to enter mode (%d)\n", ret);
> + break;
> + case TBT_STATE_ENTER:
> + ret = tbt_enter_mode(tbt);
> + if (ret)
> + dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
> + ret);
> + break;
> + case TBT_STATE_EXIT:
> + typec_altmode_exit(tbt->alt);
> + break;
> + case TBT_STATE_SOP_PP_EXIT:
> + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
> + break;
> + case TBT_STATE_SOP_P_EXIT:
> + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
> + break;
> + default:
> + break;
> + }
> +
> + tbt->state = TBT_STATE_IDLE;
> +
> + mutex_unlock(&tbt->lock);
> +}
> +
> +static int tbt_altmode_vdm(struct typec_altmode *alt,
> + const u32 hdr, const u32 *vdo, int count)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int cmd_type = PD_VDO_CMDT(hdr);
> + int cmd = PD_VDO_CMD(hdr);
> +
> + mutex_lock(&tbt->lock);
> +
> + if (tbt->state != TBT_STATE_IDLE) {
> + mutex_unlock(&tbt->lock);
> + return -EBUSY;
> + }
> +
> + switch (cmd_type) {
> + case CMDT_RSP_ACK:
> + switch (cmd) {
> + case CMD_ENTER_MODE:
> + /*
> + * Following the order describeded in USB Type-C Spec
> + * R2.0 Section 6.7.3.
> + */
> + if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_ENTER;
> + else
> + tbt->state = TBT_STATE_ENTER;
> + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> + tbt->state = TBT_STATE_ENTER;
> + } else {
> + struct typec_thunderbolt_data data;
> +
> + data.device_mode = tbt->alt->vdo;
> + data.cable_mode =
> + tbt->plug[TYPEC_PLUG_SOP_P] ?
> + tbt->plug[TYPEC_PLUG_SOP_P]
> + ->vdo :
> + 0;
> + data.enter_vdo = tbt->enter_vdo;
> +
> + typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
> + }
> + break;
> + case CMD_EXIT_MODE:
> + if (alt == tbt->alt) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_EXIT;
> + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> + tbt->state = TBT_STATE_SOP_P_EXIT;
> + }
> + break;
> + }
> + break;
> + case CMDT_RSP_NAK:
> + switch (cmd) {
> + case CMD_ENTER_MODE:
> + dev_warn(&alt->dev, "Enter Mode refused\n");
> + break;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + if (tbt->state != TBT_STATE_IDLE)
> + schedule_work(&tbt->work);
> +
> + mutex_unlock(&tbt->lock);
> +
> + return 0;
> +}
> +
> +static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + int ret;
> +
> + mutex_lock(&tbt->lock);
> +
> + if (!tbt_ready(alt))
> + return -ENODEV;
You need to mutex_unlock(&tbt->lock); before the return.
Credit to danielgeorgem@google.com for his catching this in his code review.
> +
> + /* Preventing the user space from entering/exiting the cable alt mode */
> + if (alt != tbt->alt)
> + ret = -EPERM;
> + else if (activate)
> + ret = tbt_enter_mode(tbt);
> + else
> + ret = typec_altmode_exit(alt);
> +
> + mutex_unlock(&tbt->lock);
> +
> + return ret;
> +}
> +
> +static const struct typec_altmode_ops tbt_altmode_ops = {
> + .vdm = tbt_altmode_vdm,
> + .activate = tbt_altmode_activate
> +};
> +
> +static int tbt_altmode_probe(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt;
> +
> + tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
> + if (!tbt)
> + return -ENOMEM;
> +
> + INIT_WORK(&tbt->work, tbt_altmode_work);
> + mutex_init(&tbt->lock);
> + tbt->alt = alt;
> +
> + alt->desc = "Thunderbolt3";
> + typec_altmode_set_drvdata(alt, tbt);
> + typec_altmode_set_ops(alt, &tbt_altmode_ops);
> +
> + if (tbt_ready(alt)) {
> + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> + tbt->state = TBT_STATE_SOP_PP_ENTER;
> + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> + tbt->state = TBT_STATE_SOP_P_ENTER;
> + else
> + tbt->state = TBT_STATE_ENTER;
> + schedule_work(&tbt->work);
> + }
> +
> + return 0;
> +}
> +
> +static void tbt_altmode_remove(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> +
> + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> + if (tbt->plug[i])
> + typec_altmode_put_plug(tbt->plug[i]);
> + }
> +
> + if (tbt->cable)
> + typec_cable_put(tbt->cable);
> +}
> +
> +static bool tbt_ready(struct typec_altmode *alt)
> +{
> + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> + struct typec_altmode *plug;
> +
> + if (tbt->cable)
> + return true;
> +
> + /* Thundebolt 3 requires a cable with eMarker */
> + tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
> + if (!tbt->cable)
> + return false;
> +
> + /* We accept systems without SOP' or SOP''. This means the port altmode
> + * driver will be responsible for properly ordering entry/exit.
> + */
> + for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
> + plug = typec_altmode_get_plug(tbt->alt, i);
> + if (IS_ERR(plug))
> + continue;
> +
> + if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
> + break;
> +
> + plug->desc = "Thunderbolt3";
> + plug->ops = &tbt_altmode_ops;
> + typec_altmode_set_drvdata(plug, tbt);
> +
> + tbt->plug[i] = plug;
> + }
> +
> + return true;
> +}
> +
> +static const struct typec_device_id tbt_typec_id[] = {
> + { USB_TYPEC_TBT_SID },
> + { }
> +};
> +MODULE_DEVICE_TABLE(typec, tbt_typec_id);
> +
> +static struct typec_altmode_driver tbt_altmode_driver = {
> + .id_table = tbt_typec_id,
> + .probe = tbt_altmode_probe,
> + .remove = tbt_altmode_remove,
> + .driver = {
> + .name = "typec-thunderbolt",
> + .owner = THIS_MODULE,
> + }
> +};
> +module_typec_altmode_driver(tbt_altmode_driver);
> +
> +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
> diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
> index fa97d7e00f5c..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.277.g8800431eea-goog
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode
2024-11-22 18:50 ` Benson Leung
@ 2024-12-06 22:27 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 22:27 UTC (permalink / raw)
To: Benson Leung
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, dmitry.baryshkov, Greg Kroah-Hartman,
linux-kernel, danielgeorgem
On Fri, Nov 22, 2024 at 10:51 AM Benson Leung <bleung@google.com> wrote:
>
> Hi Abhishek,
>
>
> On Thu, Nov 07, 2024 at 11:29:55AM -0800, Abhishek Pandit-Subedi wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > Thunderbolt 3 Alternate Mode entry flow is described in
> > USB Type-C Specification Release 2.0.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes:
> > * Delay cable + plug checks so that the module doesn't fail to probe
> > if cable + plug information isn't available by the time the partner
> > altmode is registered.
> > * Remove unncessary brace after if (IS_ERR(plug))
> >
> > The rest of this patch should be the same as Heikki's original RFC.
> >
> >
> > Changes in 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
> >
> > drivers/usb/typec/altmodes/Kconfig | 9 +
> > drivers/usb/typec/altmodes/Makefile | 2 +
> > drivers/usb/typec/altmodes/thunderbolt.c | 308 +++++++++++++++++++++++
> > include/linux/usb/typec_tbt.h | 1 +
> > 4 files changed, 320 insertions(+)
> > 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..a945b9d35a1d
> > --- /dev/null
> > +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> > @@ -0,0 +1,308 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * USB Typec-C Thuderbolt3 Alternate Mode driver
> > + *
> > + * Copyright (C) 2019 Intel Corporation
> > + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/mutex.h>
> > +#include <linux/module.h>
> > +#include <linux/usb/pd_vdo.h>
> > +#include <linux/usb/typec_altmode.h>
> > +#include <linux/usb/typec_tbt.h>
> > +
> > +enum tbt_state {
> > + TBT_STATE_IDLE,
> > + TBT_STATE_SOP_P_ENTER,
> > + TBT_STATE_SOP_PP_ENTER,
> > + TBT_STATE_ENTER,
> > + TBT_STATE_EXIT,
> > + TBT_STATE_SOP_PP_EXIT,
> > + TBT_STATE_SOP_P_EXIT
> > +};
> > +
> > +struct tbt_altmode {
> > + enum tbt_state state;
> > + struct typec_cable *cable;
> > + struct typec_altmode *alt;
> > + struct typec_altmode *plug[2];
> > + u32 enter_vdo;
> > +
> > + struct work_struct work;
> > + struct mutex lock; /* device lock */
> > +};
> > +
> > +static bool tbt_ready(struct typec_altmode *alt);
> > +
> > +static int tbt_enter_mode(struct tbt_altmode *tbt)
> > +{
> > + struct typec_altmode *plug = tbt->plug[TYPEC_PLUG_SOP_P];
> > + u32 vdo;
> > +
> > + vdo = tbt->alt->vdo & (TBT_VENDOR_SPECIFIC_B0 | TBT_VENDOR_SPECIFIC_B1);
> > + vdo |= tbt->alt->vdo & TBT_INTEL_SPECIFIC_B0;
> > + vdo |= TBT_MODE;
> > +
> > + if (plug) {
> > + if (typec_cable_is_active(tbt->cable))
> > + vdo |= TBT_ENTER_MODE_ACTIVE_CABLE;
> > +
> > + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_SPEED(plug->vdo));
> > + vdo |= plug->vdo & TBT_CABLE_ROUNDED;
> > + vdo |= plug->vdo & TBT_CABLE_OPTICAL;
> > + vdo |= plug->vdo & TBT_CABLE_RETIMER;
> > + vdo |= plug->vdo & TBT_CABLE_LINK_TRAINING;
> > + } else {
> > + vdo |= TBT_ENTER_MODE_CABLE_SPEED(TBT_CABLE_USB3_PASSIVE);
> > + }
> > +
> > + tbt->enter_vdo = vdo;
> > + return typec_altmode_enter(tbt->alt, &vdo);
> > +}
> > +
> > +static void tbt_altmode_work(struct work_struct *work)
> > +{
> > + struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
> > + int ret;
> > +
> > + mutex_lock(&tbt->lock);
> > +
> > + switch (tbt->state) {
> > + case TBT_STATE_SOP_P_ENTER:
> > + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_P], NULL);
> > + if (ret)
> > + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
> > + "failed to enter mode (%d)\n", ret);
> > + break;
> > + case TBT_STATE_SOP_PP_ENTER:
> > + ret = typec_altmode_enter(tbt->plug[TYPEC_PLUG_SOP_PP], NULL);
> > + if (ret)
> > + dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
> > + "failed to enter mode (%d)\n", ret);
> > + break;
> > + case TBT_STATE_ENTER:
> > + ret = tbt_enter_mode(tbt);
> > + if (ret)
> > + dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
> > + ret);
> > + break;
> > + case TBT_STATE_EXIT:
> > + typec_altmode_exit(tbt->alt);
> > + break;
> > + case TBT_STATE_SOP_PP_EXIT:
> > + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_PP]);
> > + break;
> > + case TBT_STATE_SOP_P_EXIT:
> > + typec_altmode_exit(tbt->plug[TYPEC_PLUG_SOP_P]);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + tbt->state = TBT_STATE_IDLE;
> > +
> > + mutex_unlock(&tbt->lock);
> > +}
> > +
> > +static int tbt_altmode_vdm(struct typec_altmode *alt,
> > + const u32 hdr, const u32 *vdo, int count)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > + int cmd_type = PD_VDO_CMDT(hdr);
> > + int cmd = PD_VDO_CMD(hdr);
> > +
> > + mutex_lock(&tbt->lock);
> > +
> > + if (tbt->state != TBT_STATE_IDLE) {
> > + mutex_unlock(&tbt->lock);
> > + return -EBUSY;
> > + }
> > +
> > + switch (cmd_type) {
> > + case CMDT_RSP_ACK:
> > + switch (cmd) {
> > + case CMD_ENTER_MODE:
> > + /*
> > + * Following the order describeded in USB Type-C Spec
> > + * R2.0 Section 6.7.3.
> > + */
> > + if (alt == tbt->plug[TYPEC_PLUG_SOP_P]) {
> > + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> > + tbt->state = TBT_STATE_SOP_PP_ENTER;
> > + else
> > + tbt->state = TBT_STATE_ENTER;
> > + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> > + tbt->state = TBT_STATE_ENTER;
> > + } else {
> > + struct typec_thunderbolt_data data;
> > +
> > + data.device_mode = tbt->alt->vdo;
> > + data.cable_mode =
> > + tbt->plug[TYPEC_PLUG_SOP_P] ?
> > + tbt->plug[TYPEC_PLUG_SOP_P]
> > + ->vdo :
> > + 0;
> > + data.enter_vdo = tbt->enter_vdo;
> > +
> > + typec_altmode_notify(alt, TYPEC_STATE_MODAL, &data);
> > + }
> > + break;
> > + case CMD_EXIT_MODE:
> > + if (alt == tbt->alt) {
> > + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> > + tbt->state = TBT_STATE_SOP_PP_EXIT;
> > + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> > + tbt->state = TBT_STATE_SOP_P_EXIT;
> > + } else if (alt == tbt->plug[TYPEC_PLUG_SOP_PP]) {
> > + tbt->state = TBT_STATE_SOP_P_EXIT;
> > + }
> > + break;
> > + }
> > + break;
> > + case CMDT_RSP_NAK:
> > + switch (cmd) {
> > + case CMD_ENTER_MODE:
> > + dev_warn(&alt->dev, "Enter Mode refused\n");
> > + break;
> > + default:
> > + break;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + if (tbt->state != TBT_STATE_IDLE)
> > + schedule_work(&tbt->work);
> > +
> > + mutex_unlock(&tbt->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int tbt_altmode_activate(struct typec_altmode *alt, int activate)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > + int ret;
> > +
> > + mutex_lock(&tbt->lock);
> > +
> > + if (!tbt_ready(alt))
> > + return -ENODEV;
>
>
> You need to mutex_unlock(&tbt->lock); before the return.
>
> Credit to danielgeorgem@google.com for his catching this in his code review.
Good catch! Next update will have this.
Sorry for late replies -- it took me a while to get back to this +
refactor to use the new cable ops apis. In the process, I also
discovered some mistakes in how we were entering TBT from .activate so
I opted to follow a similar pattern to what RD has done for DP with
cables.
v4 will come soon.
>
> > +
> > + /* Preventing the user space from entering/exiting the cable alt mode */
> > + if (alt != tbt->alt)
> > + ret = -EPERM;
> > + else if (activate)
> > + ret = tbt_enter_mode(tbt);
> > + else
> > + ret = typec_altmode_exit(alt);
> > +
> > + mutex_unlock(&tbt->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct typec_altmode_ops tbt_altmode_ops = {
> > + .vdm = tbt_altmode_vdm,
> > + .activate = tbt_altmode_activate
> > +};
> > +
> > +static int tbt_altmode_probe(struct typec_altmode *alt)
> > +{
> > + struct tbt_altmode *tbt;
> > +
> > + tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
> > + if (!tbt)
> > + return -ENOMEM;
> > +
> > + INIT_WORK(&tbt->work, tbt_altmode_work);
> > + mutex_init(&tbt->lock);
> > + tbt->alt = alt;
> > +
> > + alt->desc = "Thunderbolt3";
> > + typec_altmode_set_drvdata(alt, tbt);
> > + typec_altmode_set_ops(alt, &tbt_altmode_ops);
> > +
> > + if (tbt_ready(alt)) {
> > + if (tbt->plug[TYPEC_PLUG_SOP_PP])
> > + tbt->state = TBT_STATE_SOP_PP_ENTER;
> > + else if (tbt->plug[TYPEC_PLUG_SOP_P])
> > + tbt->state = TBT_STATE_SOP_P_ENTER;
> > + else
> > + tbt->state = TBT_STATE_ENTER;
> > + schedule_work(&tbt->work);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void tbt_altmode_remove(struct typec_altmode *alt)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > +
> > + for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> > + if (tbt->plug[i])
> > + typec_altmode_put_plug(tbt->plug[i]);
> > + }
> > +
> > + if (tbt->cable)
> > + typec_cable_put(tbt->cable);
> > +}
> > +
> > +static bool tbt_ready(struct typec_altmode *alt)
> > +{
> > + struct tbt_altmode *tbt = typec_altmode_get_drvdata(alt);
> > + struct typec_altmode *plug;
> > +
> > + if (tbt->cable)
> > + return true;
> > +
> > + /* Thundebolt 3 requires a cable with eMarker */
> > + tbt->cable = typec_cable_get(typec_altmode2port(tbt->alt));
> > + if (!tbt->cable)
> > + return false;
> > +
> > + /* We accept systems without SOP' or SOP''. This means the port altmode
> > + * driver will be responsible for properly ordering entry/exit.
> > + */
> > + for (int i = 0; i < TYPEC_PLUG_SOP_PP + 1; i++) {
> > + plug = typec_altmode_get_plug(tbt->alt, i);
> > + if (IS_ERR(plug))
> > + continue;
> > +
> > + if (!plug || plug->svid != USB_TYPEC_VENDOR_INTEL)
> > + break;
> > +
> > + plug->desc = "Thunderbolt3";
> > + plug->ops = &tbt_altmode_ops;
> > + typec_altmode_set_drvdata(plug, tbt);
> > +
> > + tbt->plug[i] = plug;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static const struct typec_device_id tbt_typec_id[] = {
> > + { USB_TYPEC_TBT_SID },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(typec, tbt_typec_id);
> > +
> > +static struct typec_altmode_driver tbt_altmode_driver = {
> > + .id_table = tbt_typec_id,
> > + .probe = tbt_altmode_probe,
> > + .remove = tbt_altmode_remove,
> > + .driver = {
> > + .name = "typec-thunderbolt",
> > + .owner = THIS_MODULE,
> > + }
> > +};
> > +module_typec_altmode_driver(tbt_altmode_driver);
> > +
> > +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Thunderbolt3 USB Type-C Alternate Mode");
> > diff --git a/include/linux/usb/typec_tbt.h b/include/linux/usb/typec_tbt.h
> > index fa97d7e00f5c..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.277.g8800431eea-goog
> >
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe
2024-11-07 19:29 [PATCH v3 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
2024-11-07 19:29 ` [PATCH v3 1/7] usb: typec: Only use SVID for matching altmodes Abhishek Pandit-Subedi
2024-11-07 19:29 ` [PATCH v3 2/7] usb: typec: Add driver for Thunderbolt 3 Alternate Mode Abhishek Pandit-Subedi
@ 2024-11-07 19:29 ` Abhishek Pandit-Subedi
2024-11-08 12:21 ` Heikki Krogerus
2024-11-08 13:17 ` Greg Kroah-Hartman
2024-11-07 19:29 ` [PATCH v3 4/7] platform/chrome: cros_ec_typec: Update partner altmode active Abhishek Pandit-Subedi
` (3 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-07 19:29 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: jthies, akuchynski, pmalani, dmitry.baryshkov,
Abhishek Pandit-Subedi, Greg Kroah-Hartman, linux-kernel
Enforce the same requirement as when we attempt to activate altmode via
sysfs (do not enter partner mode if port mode is not active). In order
to set a default value for this field, also introduce the inactive field
in struct typec_altmode_desc.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in v3:
- Use port.active instead of introducing auto-enter field
- Introduce inactive field to typec_altmode_desc to set default value
for active.
- Always make port 'active' field writable
drivers/usb/typec/altmodes/displayport.c | 7 +++++--
drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
drivers/usb/typec/class.c | 5 +++--
include/linux/usb/typec.h | 2 ++
4 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 3245e03d59e6..f4116e96f6a1 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
if (plug)
typec_altmode_set_drvdata(plug, dp);
- dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
- schedule_work(&dp->work);
+ /* Only attempt to enter altmode if port is active. */
+ if (port->active) {
+ dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
+ schedule_work(&dp->work);
+ }
return 0;
}
diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
index a945b9d35a1d..45567abc3bb8 100644
--- a/drivers/usb/typec/altmodes/thunderbolt.c
+++ b/drivers/usb/typec/altmodes/thunderbolt.c
@@ -212,6 +212,7 @@ static const struct typec_altmode_ops tbt_altmode_ops = {
static int tbt_altmode_probe(struct typec_altmode *alt)
{
+ const struct typec_altmode *port = typec_altmode_get_partner(alt);
struct tbt_altmode *tbt;
tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
@@ -226,7 +227,10 @@ static int tbt_altmode_probe(struct typec_altmode *alt)
typec_altmode_set_drvdata(alt, tbt);
typec_altmode_set_ops(alt, &tbt_altmode_ops);
- if (tbt_ready(alt)) {
+ /* Only attempt to enter altmode if port is active and cable/plug
+ * information is ready.
+ */
+ if (port->active && tbt_ready(alt)) {
if (tbt->plug[TYPEC_PLUG_SOP_PP])
tbt->state = TBT_STATE_SOP_PP_ENTER;
else if (tbt->plug[TYPEC_PLUG_SOP_P])
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 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..56c01771c190 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 : 1;
};
void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
--
2.47.0.277.g8800431eea-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe
2024-11-07 19:29 ` [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe Abhishek Pandit-Subedi
@ 2024-11-08 12:21 ` Heikki Krogerus
2024-11-08 13:17 ` Greg Kroah-Hartman
1 sibling, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2024-11-08 12:21 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: tzungbi, linux-usb, chrome-platform, jthies, akuchynski, pmalani,
dmitry.baryshkov, Greg Kroah-Hartman, linux-kernel
On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote:
> Enforce the same requirement as when we attempt to activate altmode via
> sysfs (do not enter partner mode if port mode is not active). In order
> to set a default value for this field, also introduce the inactive field
> in struct typec_altmode_desc.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>
> Changes in v3:
> - Use port.active instead of introducing auto-enter field
> - Introduce inactive field to typec_altmode_desc to set default value
> for active.
> - Always make port 'active' field writable
>
> drivers/usb/typec/altmodes/displayport.c | 7 +++++--
> drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
> drivers/usb/typec/class.c | 5 +++--
> include/linux/usb/typec.h | 2 ++
> 4 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 3245e03d59e6..f4116e96f6a1 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
> if (plug)
> typec_altmode_set_drvdata(plug, dp);
>
> - dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> - schedule_work(&dp->work);
> + /* Only attempt to enter altmode if port is active. */
> + if (port->active) {
> + dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> + schedule_work(&dp->work);
> + }
>
> return 0;
> }
> diff --git a/drivers/usb/typec/altmodes/thunderbolt.c b/drivers/usb/typec/altmodes/thunderbolt.c
> index a945b9d35a1d..45567abc3bb8 100644
> --- a/drivers/usb/typec/altmodes/thunderbolt.c
> +++ b/drivers/usb/typec/altmodes/thunderbolt.c
> @@ -212,6 +212,7 @@ static const struct typec_altmode_ops tbt_altmode_ops = {
>
> static int tbt_altmode_probe(struct typec_altmode *alt)
> {
> + const struct typec_altmode *port = typec_altmode_get_partner(alt);
> struct tbt_altmode *tbt;
>
> tbt = devm_kzalloc(&alt->dev, sizeof(*tbt), GFP_KERNEL);
> @@ -226,7 +227,10 @@ static int tbt_altmode_probe(struct typec_altmode *alt)
> typec_altmode_set_drvdata(alt, tbt);
> typec_altmode_set_ops(alt, &tbt_altmode_ops);
>
> - if (tbt_ready(alt)) {
> + /* Only attempt to enter altmode if port is active and cable/plug
> + * information is ready.
> + */
> + if (port->active && tbt_ready(alt)) {
> if (tbt->plug[TYPEC_PLUG_SOP_PP])
> tbt->state = TBT_STATE_SOP_PP_ENTER;
> else if (tbt->plug[TYPEC_PLUG_SOP_P])
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 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..56c01771c190 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 : 1;
> };
>
> void typec_partner_set_pd_revision(struct typec_partner *partner, u16 pd_revision);
> --
> 2.47.0.277.g8800431eea-goog
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe
2024-11-07 19:29 ` [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe Abhishek Pandit-Subedi
2024-11-08 12:21 ` Heikki Krogerus
@ 2024-11-08 13:17 ` Greg Kroah-Hartman
2024-11-08 18:28 ` Abhishek Pandit-Subedi
1 sibling, 1 reply; 24+ messages in thread
From: Greg Kroah-Hartman @ 2024-11-08 13:17 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, dmitry.baryshkov, linux-kernel
On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote:
> Enforce the same requirement as when we attempt to activate altmode via
> sysfs (do not enter partner mode if port mode is not active). In order
> to set a default value for this field, also introduce the inactive field
> in struct typec_altmode_desc.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in v3:
> - Use port.active instead of introducing auto-enter field
> - Introduce inactive field to typec_altmode_desc to set default value
> for active.
> - Always make port 'active' field writable
>
> drivers/usb/typec/altmodes/displayport.c | 7 +++++--
> drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
> drivers/usb/typec/class.c | 5 +++--
> include/linux/usb/typec.h | 2 ++
> 4 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 3245e03d59e6..f4116e96f6a1 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
> if (plug)
> typec_altmode_set_drvdata(plug, dp);
>
> - dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> - schedule_work(&dp->work);
> + /* Only attempt to enter altmode if port is active. */
> + if (port->active) {
> + dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> + schedule_work(&dp->work);
> + }
What prevents active from changing right after you checked it?
> @@ -150,6 +151,7 @@ struct typec_altmode_desc {
> u32 vdo;
> /* Only used with ports */
> enum typec_port_data roles;
> + bool inactive : 1;
A boolean bitfield? That's not needed, please just do this properly.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe
2024-11-08 13:17 ` Greg Kroah-Hartman
@ 2024-11-08 18:28 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-08 18:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, dmitry.baryshkov, linux-kernel
On Fri, Nov 8, 2024 at 5:17 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 07, 2024 at 11:29:56AM -0800, Abhishek Pandit-Subedi wrote:
> > Enforce the same requirement as when we attempt to activate altmode via
> > sysfs (do not enter partner mode if port mode is not active). In order
> > to set a default value for this field, also introduce the inactive field
> > in struct typec_altmode_desc.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Use port.active instead of introducing auto-enter field
> > - Introduce inactive field to typec_altmode_desc to set default value
> > for active.
> > - Always make port 'active' field writable
> >
> > drivers/usb/typec/altmodes/displayport.c | 7 +++++--
> > drivers/usb/typec/altmodes/thunderbolt.c | 6 +++++-
> > drivers/usb/typec/class.c | 5 +++--
> > include/linux/usb/typec.h | 2 ++
> > 4 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> > index 3245e03d59e6..f4116e96f6a1 100644
> > --- a/drivers/usb/typec/altmodes/displayport.c
> > +++ b/drivers/usb/typec/altmodes/displayport.c
> > @@ -767,8 +767,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
> > if (plug)
> > typec_altmode_set_drvdata(plug, dp);
> >
> > - dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> > - schedule_work(&dp->work);
> > + /* Only attempt to enter altmode if port is active. */
> > + if (port->active) {
> > + dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
> > + schedule_work(&dp->work);
> > + }
>
> What prevents active from changing right after you checked it?
There's another check in `typec_altmode_enter` for the port itself:
https://github.com/torvalds/linux/blob/master/drivers/usb/typec/bus.c#L138
If I leave this out, it will just fail in `dp_altmode_work` when it
calls `typec_altmode_enter` and will leave a dev_err("failed to enter
mode"). This might be more user friendly since it's visible to the
user that the partner supports the mode but the port blocked entry.
I'll update the message on mode entry to also print the return value
(-EPERM) in the next patch.
>
> > @@ -150,6 +151,7 @@ struct typec_altmode_desc {
> > u32 vdo;
> > /* Only used with ports */
> > enum typec_port_data roles;
> > + bool inactive : 1;
>
> A boolean bitfield? That's not needed, please just do this properly.
Ack - will remove the bitfield. `struct typec_altmode` also does this
-- I'll remove that in a cleanup patch too.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 4/7] platform/chrome: cros_ec_typec: Update partner altmode active
2024-11-07 19:29 [PATCH v3 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (2 preceding siblings ...)
2024-11-07 19:29 ` [PATCH v3 3/7] usb: typec: Check port is active before enter mode on probe Abhishek Pandit-Subedi
@ 2024-11-07 19:29 ` Abhishek Pandit-Subedi
2024-11-07 19:29 ` [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
` (2 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-07 19:29 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: jthies, akuchynski, pmalani, dmitry.baryshkov,
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.277.g8800431eea-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-11-07 19:29 [PATCH v3 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (3 preceding siblings ...)
2024-11-07 19:29 ` [PATCH v3 4/7] platform/chrome: cros_ec_typec: Update partner altmode active Abhishek Pandit-Subedi
@ 2024-11-07 19:29 ` Abhishek Pandit-Subedi
2024-11-09 6:38 ` Dmitry Baryshkov
2024-11-12 0:16 ` Benson Leung
2024-11-07 19:29 ` [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support Abhishek Pandit-Subedi
2024-11-07 19:30 ` [PATCH v3 7/7] platform/chrome: cros_ec_typec: Disable tbt on port Abhishek Pandit-Subedi
6 siblings, 2 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-07 19:29 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: jthies, akuchynski, pmalani, dmitry.baryshkov,
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 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/Makefile | 4 +
drivers/platform/chrome/cros_ec_typec.c | 12 +-
drivers/platform/chrome/cros_ec_typec.h | 1 +
drivers/platform/chrome/cros_typec_altmode.c | 275 +++++++++++++++++++
drivers/platform/chrome/cros_typec_altmode.h | 34 +++
6 files changed, 326 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/Makefile b/drivers/platform/chrome/Makefile
index 2dcc6ccc2302..2f90d4db8099 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_TYPEC_DP_ALTMODE),)
+ 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..3a6f5f2717b9 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -18,6 +18,7 @@
#include "cros_ec_typec.h"
#include "cros_typec_vdm.h"
+#include "cros_typec_altmode.h"
#define DRV_NAME "cros-ec-typec"
@@ -293,12 +294,11 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
desc.svid = USB_TYPEC_DP_SID;
desc.mode = USB_TYPEC_DP_MODE;
desc.vdo = DP_PORT_VDO;
- amode = typec_port_register_altmode(port->port, &desc);
+ amode = cros_typec_register_displayport(port, &desc,
+ typec->ap_driven_altmode);
if (IS_ERR(amode))
return PTR_ERR(amode);
port->port_altmode[CROS_EC_ALTMODE_DP] = amode;
- 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 +575,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 +1258,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..3598b8a6ceee
--- /dev/null
+++ b/drivers/platform/chrome/cros_typec_altmode.c
@@ -0,0 +1,275 @@
+// 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;
+
+ 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);
+
+ if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
+ data->vdo_size))
+ dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
+
+ data->header = 0;
+ data->vdo_data = NULL;
+ data->vdo_size = 0;
+}
+
+static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
+{
+ struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
+ struct ec_params_typec_control req = {
+ .port = data->port->port_num,
+ .command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
+ };
+ int svdm_version;
+ int ret;
+
+ if (!data->ap_mode_entry) {
+ const struct typec_altmode *partner =
+ typec_altmode_get_partner(alt);
+ dev_warn(&partner->dev,
+ "EC does not support ap driven mode entry\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (data->sid == USB_TYPEC_DP_SID)
+ req.mode_to_enter = CROS_EC_ALTMODE_DP;
+ else
+ return -EOPNOTSUPP;
+
+ ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
+ &req, sizeof(req), NULL, 0);
+ if (ret < 0)
+ return ret;
+
+ svdm_version = typec_altmode_get_svdm_version(alt);
+ if (svdm_version < 0)
+ return svdm_version;
+
+ data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
+ data->header |= VDO_OPOS(data->mode);
+ data->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+ data->vdo_data = NULL;
+ data->vdo_size = 1;
+
+ schedule_work(&data->work);
+
+ return ret;
+}
+
+static int cros_typec_altmode_exit(struct typec_altmode *alt)
+{
+ struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
+ struct ec_params_typec_control req = {
+ .port = data->port->port_num,
+ .command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
+ };
+ int svdm_version;
+ int ret;
+
+ if (!data->ap_mode_entry) {
+ const struct typec_altmode *partner =
+ typec_altmode_get_partner(alt);
+ dev_warn(&partner->dev,
+ "EC does not support ap driven mode entry\n");
+ return -EOPNOTSUPP;
+ }
+
+ ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
+ &req, sizeof(req), NULL, 0);
+
+ if (ret < 0)
+ return ret;
+
+ svdm_version = typec_altmode_get_svdm_version(alt);
+ if (svdm_version < 0)
+ return svdm_version;
+
+ data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
+ data->header |= VDO_OPOS(data->mode);
+ data->header |= VDO_CMDT(CMDT_RSP_ACK);
+
+ data->vdo_data = NULL;
+ data->vdo_size = 1;
+
+ schedule_work(&data->work);
+
+ return ret;
+}
+
+static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
+ const u32 *data, int count)
+{
+ struct cros_typec_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;
+
+ if (!adata->ap_mode_entry) {
+ const struct typec_altmode *partner =
+ typec_altmode_get_partner(alt);
+ dev_warn(&partner->dev,
+ "EC does not support ap driven mode entry\n");
+ return -EOPNOTSUPP;
+ }
+
+ svdm_version = typec_altmode_get_svdm_version(alt);
+ if (svdm_version < 0)
+ return svdm_version;
+
+ switch (cmd_type) {
+ case CMDT_INIT:
+ if (PD_VDO_SVDM_VER(header) < svdm_version) {
+ typec_partner_set_svdm_version(adata->port->partner,
+ PD_VDO_SVDM_VER(header));
+ svdm_version = PD_VDO_SVDM_VER(header);
+ }
+
+ adata->header = VDO(adata->sid, 1, svdm_version, cmd);
+ adata->header |= VDO_OPOS(adata->mode);
+
+ /*
+ * DP_CMD_CONFIGURE: We can't actually do anything with the
+ * provided VDO yet so just send back an ACK.
+ *
+ * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
+ * DPStatus Acks.
+ */
+ switch (cmd) {
+ case DP_CMD_CONFIGURE:
+ 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;
+ }
+
+ return 0;
+}
+
+static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
+ const u32 *data, int count)
+{
+ struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
+
+ if (adata->sid == USB_TYPEC_DP_SID)
+ return cros_typec_displayport_vdm(alt, header, data, count);
+
+ return -EINVAL;
+}
+
+static const struct typec_altmode_ops cros_typec_altmode_ops = {
+ .enter = cros_typec_altmode_enter,
+ .exit = cros_typec_altmode_exit,
+ .vdm = cros_typec_altmode_vdm,
+};
+
+#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
+int cros_typec_displayport_status_update(struct typec_altmode *altmode,
+ struct typec_displayport_data *data)
+{
+ struct cros_typec_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);
+
+ 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);
+ return 0;
+}
+
+struct typec_altmode *
+cros_typec_register_displayport(struct cros_typec_port *port,
+ struct typec_altmode_desc *desc,
+ bool ap_mode_entry)
+{
+ struct typec_altmode *alt;
+ struct cros_typec_altmode_data *data;
+
+ alt = typec_port_register_altmode(port->port, desc);
+ if (IS_ERR(alt))
+ return alt;
+
+ data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ typec_unregister_altmode(alt);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ INIT_WORK(&data->work, cros_typec_altmode_work);
+ data->alt = alt;
+ data->port = port;
+ data->ap_mode_entry = ap_mode_entry;
+ data->sid = 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.277.g8800431eea-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-11-07 19:29 ` [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
@ 2024-11-09 6:38 ` Dmitry Baryshkov
2024-11-14 4:13 ` Abhishek Pandit-Subedi
2024-11-12 0:16 ` Benson Leung
1 sibling, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2024-11-09 6:38 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Benson Leung, Guenter Roeck, linux-kernel
On Thu, Nov 07, 2024 at 11:29:58AM -0800, Abhishek Pandit-Subedi wrote:
> Add support for entering and exiting displayport alt-mode on systems
> using AP driven alt-mode.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in 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/Makefile | 4 +
> drivers/platform/chrome/cros_ec_typec.c | 12 +-
> drivers/platform/chrome/cros_ec_typec.h | 1 +
> drivers/platform/chrome/cros_typec_altmode.c | 275 +++++++++++++++++++
> drivers/platform/chrome/cros_typec_altmode.h | 34 +++
> 6 files changed, 326 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/Makefile b/drivers/platform/chrome/Makefile
> index 2dcc6ccc2302..2f90d4db8099 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_TYPEC_DP_ALTMODE),)
> + 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..3a6f5f2717b9 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -18,6 +18,7 @@
>
> #include "cros_ec_typec.h"
> #include "cros_typec_vdm.h"
> +#include "cros_typec_altmode.h"
>
> #define DRV_NAME "cros-ec-typec"
>
> @@ -293,12 +294,11 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> desc.svid = USB_TYPEC_DP_SID;
> desc.mode = USB_TYPEC_DP_MODE;
> desc.vdo = DP_PORT_VDO;
> - amode = typec_port_register_altmode(port->port, &desc);
> + amode = cros_typec_register_displayport(port, &desc,
> + typec->ap_driven_altmode);
> if (IS_ERR(amode))
> return PTR_ERR(amode);
> port->port_altmode[CROS_EC_ALTMODE_DP] = amode;
> - 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 +575,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 +1258,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..3598b8a6ceee
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -0,0 +1,275 @@
> +// 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;
> +
> + 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);
> +
> + 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;
What protects data->header / vdo_data / vdo_size from concurrent
modification?
> +}
> +
> +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
> +{
> + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> + struct ec_params_typec_control req = {
> + .port = data->port->port_num,
> + .command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
> + };
> + int svdm_version;
> + int ret;
> +
> + if (!data->ap_mode_entry) {
> + const struct typec_altmode *partner =
> + typec_altmode_get_partner(alt);
> + dev_warn(&partner->dev,
> + "EC does not support ap driven mode entry\n");
Nit: AP, not ap
> + return -EOPNOTSUPP;
> + }
> +
> + if (data->sid == USB_TYPEC_DP_SID)
> + req.mode_to_enter = CROS_EC_ALTMODE_DP;
> + else
> + return -EOPNOTSUPP;
> +
> + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> + &req, sizeof(req), NULL, 0);
> + if (ret < 0)
> + return ret;
> +
> + svdm_version = typec_altmode_get_svdm_version(alt);
> + if (svdm_version < 0)
> + return svdm_version;
> +
> + data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
> + data->header |= VDO_OPOS(data->mode);
> + data->header |= VDO_CMDT(CMDT_RSP_ACK);
> +
> + data->vdo_data = NULL;
> + data->vdo_size = 1;
> +
> + schedule_work(&data->work);
> +
> + return ret;
> +}
> +
> +static int cros_typec_altmode_exit(struct typec_altmode *alt)
> +{
> + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> + struct ec_params_typec_control req = {
> + .port = data->port->port_num,
> + .command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
> + };
> + int svdm_version;
> + int ret;
> +
> + if (!data->ap_mode_entry) {
> + const struct typec_altmode *partner =
> + typec_altmode_get_partner(alt);
> + dev_warn(&partner->dev,
> + "EC does not support ap driven mode entry\n");
> + return -EOPNOTSUPP;
> + }
> +
> + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> + &req, sizeof(req), NULL, 0);
> +
> + if (ret < 0)
> + return ret;
> +
> + svdm_version = typec_altmode_get_svdm_version(alt);
> + if (svdm_version < 0)
> + return svdm_version;
> +
> + data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
> + data->header |= VDO_OPOS(data->mode);
> + data->header |= VDO_CMDT(CMDT_RSP_ACK);
> +
> + data->vdo_data = NULL;
> + data->vdo_size = 1;
> +
> + schedule_work(&data->work);
> +
> + return ret;
> +}
> +
> +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> + const u32 *data, int count)
> +{
> + struct cros_typec_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;
> +
> + if (!adata->ap_mode_entry) {
> + const struct typec_altmode *partner =
> + typec_altmode_get_partner(alt);
> + dev_warn(&partner->dev,
> + "EC does not support ap driven mode entry\n");
> + return -EOPNOTSUPP;
> + }
Move the ckeck to cros_typec_altmode_vdm() ?
But this makes me wonder, should the driver use different ops in such a
case? Can't we just use a stubbed version of ops if
cros_typec_register_displayport() gets ap_mode_entry = false ?
> +
> + svdm_version = typec_altmode_get_svdm_version(alt);
> + if (svdm_version < 0)
> + return svdm_version;
> +
> + switch (cmd_type) {
> + case CMDT_INIT:
> + if (PD_VDO_SVDM_VER(header) < svdm_version) {
> + typec_partner_set_svdm_version(adata->port->partner,
> + PD_VDO_SVDM_VER(header));
> + svdm_version = PD_VDO_SVDM_VER(header);
> + }
> +
> + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> + adata->header |= VDO_OPOS(adata->mode);
> +
> + /*
> + * DP_CMD_CONFIGURE: We can't actually do anything with the
> + * provided VDO yet so just send back an ACK.
> + *
> + * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
> + * DPStatus Acks.
> + */
> + switch (cmd) {
> + case DP_CMD_CONFIGURE:
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> + const u32 *data, int count)
> +{
> + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> +
> + if (adata->sid == USB_TYPEC_DP_SID)
> + return cros_typec_displayport_vdm(alt, header, data, count);
> +
> + return -EINVAL;
> +}
> +
> +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> + .enter = cros_typec_altmode_enter,
> + .exit = cros_typec_altmode_exit,
> + .vdm = cros_typec_altmode_vdm,
> +};
> +
> +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> + struct typec_displayport_data *data)
> +{
> + struct cros_typec_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);
> +
> + 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);
> + return 0;
> +}
> +
> +struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> + struct typec_altmode_desc *desc,
> + bool ap_mode_entry)
> +{
> + struct typec_altmode *alt;
> + struct cros_typec_altmode_data *data;
> +
> + alt = typec_port_register_altmode(port->port, desc);
> + if (IS_ERR(alt))
> + return alt;
> +
> + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + typec_unregister_altmode(alt);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + INIT_WORK(&data->work, cros_typec_altmode_work);
> + data->alt = alt;
> + data->port = port;
> + data->ap_mode_entry = ap_mode_entry;
> + data->sid = 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.277.g8800431eea-goog
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-11-09 6:38 ` Dmitry Baryshkov
@ 2024-11-14 4:13 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-14 4:13 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Benson Leung, Guenter Roeck, linux-kernel
On Fri, Nov 8, 2024 at 10:38 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, Nov 07, 2024 at 11:29:58AM -0800, Abhishek Pandit-Subedi wrote:
> > Add support for entering and exiting displayport alt-mode on systems
> > using AP driven alt-mode.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in 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/Makefile | 4 +
> > drivers/platform/chrome/cros_ec_typec.c | 12 +-
> > drivers/platform/chrome/cros_ec_typec.h | 1 +
> > drivers/platform/chrome/cros_typec_altmode.c | 275 +++++++++++++++++++
> > drivers/platform/chrome/cros_typec_altmode.h | 34 +++
> > 6 files changed, 326 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/Makefile b/drivers/platform/chrome/Makefile
> > index 2dcc6ccc2302..2f90d4db8099 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_TYPEC_DP_ALTMODE),)
> > + 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..3a6f5f2717b9 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -18,6 +18,7 @@
> >
> > #include "cros_ec_typec.h"
> > #include "cros_typec_vdm.h"
> > +#include "cros_typec_altmode.h"
> >
> > #define DRV_NAME "cros-ec-typec"
> >
> > @@ -293,12 +294,11 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
> > desc.svid = USB_TYPEC_DP_SID;
> > desc.mode = USB_TYPEC_DP_MODE;
> > desc.vdo = DP_PORT_VDO;
> > - amode = typec_port_register_altmode(port->port, &desc);
> > + amode = cros_typec_register_displayport(port, &desc,
> > + typec->ap_driven_altmode);
> > if (IS_ERR(amode))
> > return PTR_ERR(amode);
> > port->port_altmode[CROS_EC_ALTMODE_DP] = amode;
> > - 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 +575,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 +1258,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..3598b8a6ceee
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > @@ -0,0 +1,275 @@
> > +// 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;
> > +
> > + 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);
> > +
> > + 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;
>
> What protects data->header / vdo_data / vdo_size from concurrent
> modification?
Good catch! This needs a mutex -- will add for the next series.
>
> > +}
> > +
> > +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
> > +{
> > + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> > + struct ec_params_typec_control req = {
> > + .port = data->port->port_num,
> > + .command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
> > + };
> > + int svdm_version;
> > + int ret;
> > +
> > + if (!data->ap_mode_entry) {
> > + const struct typec_altmode *partner =
> > + typec_altmode_get_partner(alt);
> > + dev_warn(&partner->dev,
> > + "EC does not support ap driven mode entry\n");
>
> Nit: AP, not ap
Ack for next series.
>
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (data->sid == USB_TYPEC_DP_SID)
> > + req.mode_to_enter = CROS_EC_ALTMODE_DP;
> > + else
> > + return -EOPNOTSUPP;
> > +
> > + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> > + &req, sizeof(req), NULL, 0);
> > + if (ret < 0)
> > + return ret;
> > +
> > + svdm_version = typec_altmode_get_svdm_version(alt);
> > + if (svdm_version < 0)
> > + return svdm_version;
> > +
> > + data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
> > + data->header |= VDO_OPOS(data->mode);
> > + data->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +
> > + data->vdo_data = NULL;
> > + data->vdo_size = 1;
> > +
> > + schedule_work(&data->work);
> > +
> > + return ret;
> > +}
> > +
> > +static int cros_typec_altmode_exit(struct typec_altmode *alt)
> > +{
> > + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> > + struct ec_params_typec_control req = {
> > + .port = data->port->port_num,
> > + .command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
> > + };
> > + int svdm_version;
> > + int ret;
> > +
> > + if (!data->ap_mode_entry) {
> > + const struct typec_altmode *partner =
> > + typec_altmode_get_partner(alt);
> > + dev_warn(&partner->dev,
> > + "EC does not support ap driven mode entry\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> > + &req, sizeof(req), NULL, 0);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + svdm_version = typec_altmode_get_svdm_version(alt);
> > + if (svdm_version < 0)
> > + return svdm_version;
> > +
> > + data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
> > + data->header |= VDO_OPOS(data->mode);
> > + data->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +
> > + data->vdo_data = NULL;
> > + data->vdo_size = 1;
> > +
> > + schedule_work(&data->work);
> > +
> > + return ret;
> > +}
> > +
> > +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> > + const u32 *data, int count)
> > +{
> > + struct cros_typec_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;
> > +
> > + if (!adata->ap_mode_entry) {
> > + const struct typec_altmode *partner =
> > + typec_altmode_get_partner(alt);
> > + dev_warn(&partner->dev,
> > + "EC does not support ap driven mode entry\n");
> > + return -EOPNOTSUPP;
> > + }
>
> Move the ckeck to cros_typec_altmode_vdm() ?
>
> But this makes me wonder, should the driver use different ops in such a
> case? Can't we just use a stubbed version of ops if
> cros_typec_register_displayport() gets ap_mode_entry = false ?
>
I could leave out `.vdm` in typec_altmode_ops and I'd get the same
result (-ENOTSUPP) but without the warning message. That'll remove the
need to store the bool entirely. Expect it next series -- thanks!
> > +
> > + svdm_version = typec_altmode_get_svdm_version(alt);
> > + if (svdm_version < 0)
> > + return svdm_version;
> > +
> > + switch (cmd_type) {
> > + case CMDT_INIT:
> > + if (PD_VDO_SVDM_VER(header) < svdm_version) {
> > + typec_partner_set_svdm_version(adata->port->partner,
> > + PD_VDO_SVDM_VER(header));
> > + svdm_version = PD_VDO_SVDM_VER(header);
> > + }
> > +
> > + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> > + adata->header |= VDO_OPOS(adata->mode);
> > +
> > + /*
> > + * DP_CMD_CONFIGURE: We can't actually do anything with the
> > + * provided VDO yet so just send back an ACK.
> > + *
> > + * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
> > + * DPStatus Acks.
> > + */
> > + switch (cmd) {
> > + case DP_CMD_CONFIGURE:
> > + 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;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> > + const u32 *data, int count)
> > +{
> > + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> > +
> > + if (adata->sid == USB_TYPEC_DP_SID)
> > + return cros_typec_displayport_vdm(alt, header, data, count);
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> > + .enter = cros_typec_altmode_enter,
> > + .exit = cros_typec_altmode_exit,
> > + .vdm = cros_typec_altmode_vdm,
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> > +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> > + struct typec_displayport_data *data)
> > +{
> > + struct cros_typec_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);
> > +
> > + 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);
> > + return 0;
> > +}
> > +
> > +struct typec_altmode *
> > +cros_typec_register_displayport(struct cros_typec_port *port,
> > + struct typec_altmode_desc *desc,
> > + bool ap_mode_entry)
> > +{
> > + struct typec_altmode *alt;
> > + struct cros_typec_altmode_data *data;
> > +
> > + alt = typec_port_register_altmode(port->port, desc);
> > + if (IS_ERR(alt))
> > + return alt;
> > +
> > + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
> > + if (!data) {
> > + typec_unregister_altmode(alt);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + INIT_WORK(&data->work, cros_typec_altmode_work);
> > + data->alt = alt;
> > + data->port = port;
> > + data->ap_mode_entry = ap_mode_entry;
> > + data->sid = 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.277.g8800431eea-goog
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support
2024-11-07 19:29 ` [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
2024-11-09 6:38 ` Dmitry Baryshkov
@ 2024-11-12 0:16 ` Benson Leung
1 sibling, 0 replies; 24+ messages in thread
From: Benson Leung @ 2024-11-12 0:16 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, dmitry.baryshkov, Benson Leung,
Guenter Roeck, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 14586 bytes --]
Hi Abhishek,
On Thu, Nov 07, 2024 at 11:29:58AM -0800, Abhishek Pandit-Subedi wrote:
> Add support for entering and exiting displayport alt-mode on systems
> using AP driven alt-mode.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in 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/Makefile | 4 +
> drivers/platform/chrome/cros_ec_typec.c | 12 +-
> drivers/platform/chrome/cros_ec_typec.h | 1 +
> drivers/platform/chrome/cros_typec_altmode.c | 275 +++++++++++++++++++
> drivers/platform/chrome/cros_typec_altmode.h | 34 +++
> 6 files changed, 326 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/Makefile b/drivers/platform/chrome/Makefile
> index 2dcc6ccc2302..2f90d4db8099 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_TYPEC_DP_ALTMODE),)
> + 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..3a6f5f2717b9 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -18,6 +18,7 @@
>
> #include "cros_ec_typec.h"
> #include "cros_typec_vdm.h"
> +#include "cros_typec_altmode.h"
>
> #define DRV_NAME "cros-ec-typec"
>
> @@ -293,12 +294,11 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
As we debugged late last week, this desc here needs to be initialized ahead
of the register, as it had some junk from the stack that was causing the
"active" property to be sometimes set to no.
memset(&desc, 0, sizeof(desc));
This worked for me when testing your series.
> 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 +575,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 +1258,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..3598b8a6ceee
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -0,0 +1,275 @@
> +// 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;
> +
> + 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);
> +
> + if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
> + data->vdo_size))
> + dev_err(&data->alt->dev, "VDM 0x%x failed", data->header);
> +
> + data->header = 0;
> + data->vdo_data = NULL;
> + data->vdo_size = 0;
> +}
> +
> +static int cros_typec_altmode_enter(struct typec_altmode *alt, u32 *vdo)
> +{
> + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> + struct ec_params_typec_control req = {
> + .port = data->port->port_num,
> + .command = TYPEC_CONTROL_COMMAND_ENTER_MODE,
> + };
> + int svdm_version;
> + int ret;
> +
> + if (!data->ap_mode_entry) {
> + const struct typec_altmode *partner =
> + typec_altmode_get_partner(alt);
> + dev_warn(&partner->dev,
> + "EC does not support ap driven mode entry\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (data->sid == USB_TYPEC_DP_SID)
> + req.mode_to_enter = CROS_EC_ALTMODE_DP;
> + else
> + return -EOPNOTSUPP;
> +
> + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> + &req, sizeof(req), NULL, 0);
> + if (ret < 0)
> + return ret;
> +
> + svdm_version = typec_altmode_get_svdm_version(alt);
> + if (svdm_version < 0)
> + return svdm_version;
> +
> + data->header = VDO(data->sid, 1, svdm_version, CMD_ENTER_MODE);
> + data->header |= VDO_OPOS(data->mode);
> + data->header |= VDO_CMDT(CMDT_RSP_ACK);
> +
> + data->vdo_data = NULL;
> + data->vdo_size = 1;
> +
> + schedule_work(&data->work);
> +
> + return ret;
> +}
> +
> +static int cros_typec_altmode_exit(struct typec_altmode *alt)
> +{
> + struct cros_typec_altmode_data *data = typec_altmode_get_drvdata(alt);
> + struct ec_params_typec_control req = {
> + .port = data->port->port_num,
> + .command = TYPEC_CONTROL_COMMAND_EXIT_MODES,
> + };
> + int svdm_version;
> + int ret;
> +
> + if (!data->ap_mode_entry) {
> + const struct typec_altmode *partner =
> + typec_altmode_get_partner(alt);
> + dev_warn(&partner->dev,
> + "EC does not support ap driven mode entry\n");
> + return -EOPNOTSUPP;
> + }
> +
> + ret = cros_ec_cmd(data->port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> + &req, sizeof(req), NULL, 0);
> +
> + if (ret < 0)
> + return ret;
> +
> + svdm_version = typec_altmode_get_svdm_version(alt);
> + if (svdm_version < 0)
> + return svdm_version;
> +
> + data->header = VDO(data->sid, 1, svdm_version, CMD_EXIT_MODE);
> + data->header |= VDO_OPOS(data->mode);
> + data->header |= VDO_CMDT(CMDT_RSP_ACK);
> +
> + data->vdo_data = NULL;
> + data->vdo_size = 1;
> +
> + schedule_work(&data->work);
> +
> + return ret;
> +}
> +
> +static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> + const u32 *data, int count)
> +{
> + struct cros_typec_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;
> +
> + if (!adata->ap_mode_entry) {
> + const struct typec_altmode *partner =
> + typec_altmode_get_partner(alt);
> + dev_warn(&partner->dev,
> + "EC does not support ap driven mode entry\n");
> + return -EOPNOTSUPP;
> + }
> +
> + svdm_version = typec_altmode_get_svdm_version(alt);
> + if (svdm_version < 0)
> + return svdm_version;
> +
> + switch (cmd_type) {
> + case CMDT_INIT:
> + if (PD_VDO_SVDM_VER(header) < svdm_version) {
> + typec_partner_set_svdm_version(adata->port->partner,
> + PD_VDO_SVDM_VER(header));
> + svdm_version = PD_VDO_SVDM_VER(header);
> + }
> +
> + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> + adata->header |= VDO_OPOS(adata->mode);
> +
> + /*
> + * DP_CMD_CONFIGURE: We can't actually do anything with the
> + * provided VDO yet so just send back an ACK.
> + *
> + * DP_CMD_STATUS_UPDATE: We wait for Mux changes to send
> + * DPStatus Acks.
> + */
> + switch (cmd) {
> + case DP_CMD_CONFIGURE:
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> + const u32 *data, int count)
> +{
> + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> +
> + if (adata->sid == USB_TYPEC_DP_SID)
> + return cros_typec_displayport_vdm(alt, header, data, count);
> +
> + return -EINVAL;
> +}
> +
> +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> + .enter = cros_typec_altmode_enter,
> + .exit = cros_typec_altmode_exit,
> + .vdm = cros_typec_altmode_vdm,
> +};
> +
> +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> + struct typec_displayport_data *data)
> +{
> + struct cros_typec_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);
> +
> + 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);
> + return 0;
> +}
> +
> +struct typec_altmode *
> +cros_typec_register_displayport(struct cros_typec_port *port,
> + struct typec_altmode_desc *desc,
> + bool ap_mode_entry)
> +{
> + struct typec_altmode *alt;
> + struct cros_typec_altmode_data *data;
> +
> + alt = typec_port_register_altmode(port->port, desc);
> + if (IS_ERR(alt))
> + return alt;
> +
> + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + typec_unregister_altmode(alt);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + INIT_WORK(&data->work, cros_typec_altmode_work);
> + data->alt = alt;
> + data->port = port;
> + data->ap_mode_entry = ap_mode_entry;
> + data->sid = 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.277.g8800431eea-goog
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
2024-11-07 19:29 [PATCH v3 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (4 preceding siblings ...)
2024-11-07 19:29 ` [PATCH v3 5/7] platform/chrome: cros_ec_typec: Displayport support Abhishek Pandit-Subedi
@ 2024-11-07 19:29 ` Abhishek Pandit-Subedi
2024-11-09 6:41 ` Dmitry Baryshkov
2024-11-07 19:30 ` [PATCH v3 7/7] platform/chrome: cros_ec_typec: Disable tbt on port Abhishek Pandit-Subedi
6 siblings, 1 reply; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-07 19:29 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: jthies, akuchynski, pmalani, dmitry.baryshkov,
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 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/Makefile | 3 +
drivers/platform/chrome/cros_ec_typec.c | 23 +++---
drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
4 files changed, 114 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2f90d4db8099..b9b1281de063 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -21,6 +21,9 @@ cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
cros-ec-typec-objs += cros_typec_altmode.o
endif
+ifneq ($(CONFIG_TYPEC_TBT_ALTMODE),)
+ 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
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 3a6f5f2717b9..558b618df63c 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -302,18 +302,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 3598b8a6ceee..9cf2cef6c277 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"
@@ -67,6 +68,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;
@@ -196,6 +199,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
return 0;
}
+static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
+ const u32 *data, int count)
+{
+ struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
+
+ int cmd_type = PD_VDO_CMDT(header);
+ int cmd = PD_VDO_CMD(header);
+ int svdm_version;
+
+ svdm_version = typec_altmode_get_svdm_version(alt);
+ if (svdm_version < 0)
+ return svdm_version;
+
+ switch (cmd_type) {
+ case CMDT_INIT:
+ if (PD_VDO_SVDM_VER(header) < svdm_version) {
+ typec_partner_set_svdm_version(adata->port->partner,
+ PD_VDO_SVDM_VER(header));
+ svdm_version = PD_VDO_SVDM_VER(header);
+ }
+
+ adata->header = VDO(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;
+ }
+
+ return 0;
+}
+
+
static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
const u32 *data, int count)
{
@@ -204,6 +254,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;
}
@@ -273,3 +326,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.277.g8800431eea-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
2024-11-07 19:29 ` [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support Abhishek Pandit-Subedi
@ 2024-11-09 6:41 ` Dmitry Baryshkov
2024-11-14 4:01 ` Abhishek Pandit-Subedi
0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2024-11-09 6:41 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Benson Leung, Guenter Roeck, linux-kernel
On Thu, Nov 07, 2024 at 11:29:59AM -0800, Abhishek Pandit-Subedi wrote:
> Add support for entering and exiting Thunderbolt alt-mode using AP
> driven alt-mode.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in 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/Makefile | 3 +
> drivers/platform/chrome/cros_ec_typec.c | 23 +++---
> drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
> drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
> 4 files changed, 114 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 2f90d4db8099..b9b1281de063 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -21,6 +21,9 @@ cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
> ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
> cros-ec-typec-objs += cros_typec_altmode.o
> endif
> +ifneq ($(CONFIG_TYPEC_TBT_ALTMODE),)
> + cros-ec-typec-objs += cros_typec_altmode.o
> +endif
Doesn't this also result in the object file being included twice and
thus in a duplicate symbols declaration?
> obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.o
>
> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 3a6f5f2717b9..558b618df63c 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -302,18 +302,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 3598b8a6ceee..9cf2cef6c277 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"
> @@ -67,6 +68,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;
>
> @@ -196,6 +199,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> return 0;
> }
>
> +static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
> + const u32 *data, int count)
> +{
> + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> +
> + int cmd_type = PD_VDO_CMDT(header);
> + int cmd = PD_VDO_CMD(header);
> + int svdm_version;
I suppose that with the current approach this misses the ap_mode_entry
check. If it gets moved to cros_typec_altmode_vdm(), then it should be
okay.
> +
> + svdm_version = typec_altmode_get_svdm_version(alt);
> + if (svdm_version < 0)
> + return svdm_version;
> +
> + switch (cmd_type) {
> + case CMDT_INIT:
> + if (PD_VDO_SVDM_VER(header) < svdm_version) {
> + typec_partner_set_svdm_version(adata->port->partner,
> + PD_VDO_SVDM_VER(header));
> + svdm_version = PD_VDO_SVDM_VER(header);
> + }
> +
> + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> + adata->header |= VDO_OPOS(adata->mode);
> +
> + switch (cmd) {
> + case CMD_ENTER_MODE:
> + /* Don't respond to the enter mode vdm because it
> + * triggers mux configuration. This is handled directly
> + * by the cros_ec_typec driver so the Thunderbolt driver
> + * doesn't need to be involved.
> + */
> + break;
> + default:
> + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> + schedule_work(&adata->work);
> + break;
> + }
> +
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +
> static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> const u32 *data, int count)
> {
> @@ -204,6 +254,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;
> }
>
> @@ -273,3 +326,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.277.g8800431eea-goog
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
2024-11-09 6:41 ` Dmitry Baryshkov
@ 2024-11-14 4:01 ` Abhishek Pandit-Subedi
2024-11-14 10:55 ` Dmitry Baryshkov
0 siblings, 1 reply; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-14 4:01 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Benson Leung, Guenter Roeck, linux-kernel
On Fri, Nov 8, 2024 at 10:41 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, Nov 07, 2024 at 11:29:59AM -0800, Abhishek Pandit-Subedi wrote:
> > Add support for entering and exiting Thunderbolt alt-mode using AP
> > driven alt-mode.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in 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/Makefile | 3 +
> > drivers/platform/chrome/cros_ec_typec.c | 23 +++---
> > drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
> > drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
> > 4 files changed, 114 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > index 2f90d4db8099..b9b1281de063 100644
> > --- a/drivers/platform/chrome/Makefile
> > +++ b/drivers/platform/chrome/Makefile
> > @@ -21,6 +21,9 @@ cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
> > ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
> > cros-ec-typec-objs += cros_typec_altmode.o
> > endif
> > +ifneq ($(CONFIG_TYPEC_TBT_ALTMODE),)
> > + cros-ec-typec-objs += cros_typec_altmode.o
> > +endif
>
> Doesn't this also result in the object file being included twice and
> thus in a duplicate symbols declaration?
I was trying to figure out how to add this file if either of those
config options existed in
https://docs.kernel.org/kbuild/makefiles.html#built-in-object-goals-obj-y
and it says, "Duplicates in the lists are allowed: the first instance
will be linked into built-in.a and succeeding instances will be
ignored."
Is there a preferred way of doing the following in the Makefile:
if (defined(CONFIG_TYPEC_TBT_ALTMODE) ||
defined(CONFIG_TYPEC_DP_ALTMODE)) {...}
I briefly considered the following and dropped it because it is
terrible readability-wise:
ifneq ($(CONFIG_TYPEC_TBT_ALTMODE)$(CONFIG_TYPEC_DP_ALTMODE),)
>
> > obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.o
> >
> > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 3a6f5f2717b9..558b618df63c 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -302,18 +302,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 3598b8a6ceee..9cf2cef6c277 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"
> > @@ -67,6 +68,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;
> >
> > @@ -196,6 +199,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> > return 0;
> > }
> >
> > +static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
> > + const u32 *data, int count)
> > +{
> > + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> > +
> > + int cmd_type = PD_VDO_CMDT(header);
> > + int cmd = PD_VDO_CMD(header);
> > + int svdm_version;
>
> I suppose that with the current approach this misses the ap_mode_entry
> check. If it gets moved to cros_typec_altmode_vdm(), then it should be
> okay.
We don't register the thunderbolt port driver if ap_mode_entry is
false so it's an unnecessary check.
>
> > +
> > + svdm_version = typec_altmode_get_svdm_version(alt);
> > + if (svdm_version < 0)
> > + return svdm_version;
> > +
> > + switch (cmd_type) {
> > + case CMDT_INIT:
> > + if (PD_VDO_SVDM_VER(header) < svdm_version) {
> > + typec_partner_set_svdm_version(adata->port->partner,
> > + PD_VDO_SVDM_VER(header));
> > + svdm_version = PD_VDO_SVDM_VER(header);
> > + }
> > +
> > + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> > + adata->header |= VDO_OPOS(adata->mode);
> > +
> > + switch (cmd) {
> > + case CMD_ENTER_MODE:
> > + /* Don't respond to the enter mode vdm because it
> > + * triggers mux configuration. This is handled directly
> > + * by the cros_ec_typec driver so the Thunderbolt driver
> > + * doesn't need to be involved.
> > + */
> > + break;
> > + default:
> > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > + schedule_work(&adata->work);
> > + break;
> > + }
> > +
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> > const u32 *data, int count)
> > {
> > @@ -204,6 +254,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;
> > }
> >
> > @@ -273,3 +326,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.277.g8800431eea-goog
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
2024-11-14 4:01 ` Abhishek Pandit-Subedi
@ 2024-11-14 10:55 ` Dmitry Baryshkov
2024-12-06 22:23 ` Abhishek Pandit-Subedi
0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Baryshkov @ 2024-11-14 10:55 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Benson Leung, Guenter Roeck, linux-kernel
On Wed, Nov 13, 2024 at 08:01:57PM -0800, Abhishek Pandit-Subedi wrote:
> On Fri, Nov 8, 2024 at 10:41 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Thu, Nov 07, 2024 at 11:29:59AM -0800, Abhishek Pandit-Subedi wrote:
> > > Add support for entering and exiting Thunderbolt alt-mode using AP
> > > driven alt-mode.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > >
> > > Changes in 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/Makefile | 3 +
> > > drivers/platform/chrome/cros_ec_typec.c | 23 +++---
> > > drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
> > > drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
> > > 4 files changed, 114 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > > index 2f90d4db8099..b9b1281de063 100644
> > > --- a/drivers/platform/chrome/Makefile
> > > +++ b/drivers/platform/chrome/Makefile
> > > @@ -21,6 +21,9 @@ cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
> > > ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
> > > cros-ec-typec-objs += cros_typec_altmode.o
> > > endif
> > > +ifneq ($(CONFIG_TYPEC_TBT_ALTMODE),)
> > > + cros-ec-typec-objs += cros_typec_altmode.o
> > > +endif
> >
> > Doesn't this also result in the object file being included twice and
> > thus in a duplicate symbols declaration?
>
> I was trying to figure out how to add this file if either of those
> config options existed in
> https://docs.kernel.org/kbuild/makefiles.html#built-in-object-goals-obj-y
> and it says, "Duplicates in the lists are allowed: the first instance
> will be linked into built-in.a and succeeding instances will be
> ignored."
>
> Is there a preferred way of doing the following in the Makefile:
> if (defined(CONFIG_TYPEC_TBT_ALTMODE) ||
> defined(CONFIG_TYPEC_DP_ALTMODE)) {...}
>
> I briefly considered the following and dropped it because it is
> terrible readability-wise:
> ifneq ($(CONFIG_TYPEC_TBT_ALTMODE)$(CONFIG_TYPEC_DP_ALTMODE),)
The usual way would to define new Kconfig symbol:
config CROS_EC_TYPEC_ALTMODES
bool # Note, no description here, don't show in menuconfig
help
Selectable symbol to enable altmodes
config CROS_EC_TYPEC
...
select CROS_EC_TYPEC_ALTMODES if CONFIG_TYPEC_DP_ALTMODE
select CROS_EC_TYPEC_ALTMODES if CONFIG_TYPEC_TBT_ALTMODE
...
----
cros-ec-typec-$(CONFIG_CROS_EC_TYPEC_ALTMODES) += cros_typec_altmode.o
>
> >
> > > obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.o
> > >
> > > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > > index 3a6f5f2717b9..558b618df63c 100644
> > > --- a/drivers/platform/chrome/cros_ec_typec.c
> > > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > > @@ -302,18 +302,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 3598b8a6ceee..9cf2cef6c277 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"
> > > @@ -67,6 +68,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;
> > >
> > > @@ -196,6 +199,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> > > return 0;
> > > }
> > >
> > > +static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
> > > + const u32 *data, int count)
> > > +{
> > > + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> > > +
> > > + int cmd_type = PD_VDO_CMDT(header);
> > > + int cmd = PD_VDO_CMD(header);
> > > + int svdm_version;
> >
> > I suppose that with the current approach this misses the ap_mode_entry
> > check. If it gets moved to cros_typec_altmode_vdm(), then it should be
> > okay.
>
> We don't register the thunderbolt port driver if ap_mode_entry is
> false so it's an unnecessary check.
Why don't you register it? It would allow userspace to understand, what
is happening, e.g. that the Type-C has switched to the TBT mode.
>
> >
> > > +
> > > + svdm_version = typec_altmode_get_svdm_version(alt);
> > > + if (svdm_version < 0)
> > > + return svdm_version;
> > > +
> > > + switch (cmd_type) {
> > > + case CMDT_INIT:
> > > + if (PD_VDO_SVDM_VER(header) < svdm_version) {
> > > + typec_partner_set_svdm_version(adata->port->partner,
> > > + PD_VDO_SVDM_VER(header));
> > > + svdm_version = PD_VDO_SVDM_VER(header);
> > > + }
> > > +
> > > + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> > > + adata->header |= VDO_OPOS(adata->mode);
> > > +
> > > + switch (cmd) {
> > > + case CMD_ENTER_MODE:
> > > + /* Don't respond to the enter mode vdm because it
> > > + * triggers mux configuration. This is handled directly
> > > + * by the cros_ec_typec driver so the Thunderbolt driver
> > > + * doesn't need to be involved.
> > > + */
> > > + break;
> > > + default:
> > > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > > + schedule_work(&adata->work);
> > > + break;
> > > + }
> > > +
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +
> > > static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> > > const u32 *data, int count)
> > > {
> > > @@ -204,6 +254,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;
> > > }
> > >
> > > @@ -273,3 +326,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.277.g8800431eea-goog
> > >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
2024-11-14 10:55 ` Dmitry Baryshkov
@ 2024-12-06 22:23 ` Abhishek Pandit-Subedi
2024-12-06 23:30 ` Dmitry Baryshkov
0 siblings, 1 reply; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-12-06 22:23 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Benson Leung, Guenter Roeck, linux-kernel
On Thu, Nov 14, 2024 at 2:55 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Nov 13, 2024 at 08:01:57PM -0800, Abhishek Pandit-Subedi wrote:
> > On Fri, Nov 8, 2024 at 10:41 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Thu, Nov 07, 2024 at 11:29:59AM -0800, Abhishek Pandit-Subedi wrote:
> > > > Add support for entering and exiting Thunderbolt alt-mode using AP
> > > > driven alt-mode.
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > ---
> > > >
> > > > Changes in 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/Makefile | 3 +
> > > > drivers/platform/chrome/cros_ec_typec.c | 23 +++---
> > > > drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
> > > > drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
> > > > 4 files changed, 114 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > > > index 2f90d4db8099..b9b1281de063 100644
> > > > --- a/drivers/platform/chrome/Makefile
> > > > +++ b/drivers/platform/chrome/Makefile
> > > > @@ -21,6 +21,9 @@ cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
> > > > ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
> > > > cros-ec-typec-objs += cros_typec_altmode.o
> > > > endif
> > > > +ifneq ($(CONFIG_TYPEC_TBT_ALTMODE),)
> > > > + cros-ec-typec-objs += cros_typec_altmode.o
> > > > +endif
> > >
> > > Doesn't this also result in the object file being included twice and
> > > thus in a duplicate symbols declaration?
> >
> > I was trying to figure out how to add this file if either of those
> > config options existed in
> > https://docs.kernel.org/kbuild/makefiles.html#built-in-object-goals-obj-y
> > and it says, "Duplicates in the lists are allowed: the first instance
> > will be linked into built-in.a and succeeding instances will be
> > ignored."
> >
> > Is there a preferred way of doing the following in the Makefile:
> > if (defined(CONFIG_TYPEC_TBT_ALTMODE) ||
> > defined(CONFIG_TYPEC_DP_ALTMODE)) {...}
> >
> > I briefly considered the following and dropped it because it is
> > terrible readability-wise:
> > ifneq ($(CONFIG_TYPEC_TBT_ALTMODE)$(CONFIG_TYPEC_DP_ALTMODE),)
>
> The usual way would to define new Kconfig symbol:
>
> config CROS_EC_TYPEC_ALTMODES
> bool # Note, no description here, don't show in menuconfig
> help
> Selectable symbol to enable altmodes
>
> config CROS_EC_TYPEC
> ...
> select CROS_EC_TYPEC_ALTMODES if CONFIG_TYPEC_DP_ALTMODE
> select CROS_EC_TYPEC_ALTMODES if CONFIG_TYPEC_TBT_ALTMODE
> ...
>
> ----
>
> cros-ec-typec-$(CONFIG_CROS_EC_TYPEC_ALTMODES) += cros_typec_altmode.o
>
> >
> > >
> > > > obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.o
> > > >
> > > > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> > > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > > > index 3a6f5f2717b9..558b618df63c 100644
> > > > --- a/drivers/platform/chrome/cros_ec_typec.c
> > > > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > > > @@ -302,18 +302,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 3598b8a6ceee..9cf2cef6c277 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"
> > > > @@ -67,6 +68,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;
> > > >
> > > > @@ -196,6 +199,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
> > > > return 0;
> > > > }
> > > >
> > > > +static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
> > > > + const u32 *data, int count)
> > > > +{
> > > > + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
> > > > +
> > > > + int cmd_type = PD_VDO_CMDT(header);
> > > > + int cmd = PD_VDO_CMD(header);
> > > > + int svdm_version;
> > >
> > > I suppose that with the current approach this misses the ap_mode_entry
> > > check. If it gets moved to cros_typec_altmode_vdm(), then it should be
> > > okay.
> >
> > We don't register the thunderbolt port driver if ap_mode_entry is
> > false so it's an unnecessary check.
>
> Why don't you register it? It would allow userspace to understand, what
> is happening, e.g. that the Type-C has switched to the TBT mode.
Cros-EC does not support automatic entry of Thunderbolt/USB4 (i.e. all
firmware that supports TBT/USB4 MUST set ap_mode_entry).
>
> >
> > >
> > > > +
> > > > + svdm_version = typec_altmode_get_svdm_version(alt);
> > > > + if (svdm_version < 0)
> > > > + return svdm_version;
> > > > +
> > > > + switch (cmd_type) {
> > > > + case CMDT_INIT:
> > > > + if (PD_VDO_SVDM_VER(header) < svdm_version) {
> > > > + typec_partner_set_svdm_version(adata->port->partner,
> > > > + PD_VDO_SVDM_VER(header));
> > > > + svdm_version = PD_VDO_SVDM_VER(header);
> > > > + }
> > > > +
> > > > + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
> > > > + adata->header |= VDO_OPOS(adata->mode);
> > > > +
> > > > + switch (cmd) {
> > > > + case CMD_ENTER_MODE:
> > > > + /* Don't respond to the enter mode vdm because it
> > > > + * triggers mux configuration. This is handled directly
> > > > + * by the cros_ec_typec driver so the Thunderbolt driver
> > > > + * doesn't need to be involved.
> > > > + */
> > > > + break;
> > > > + default:
> > > > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > > > + schedule_work(&adata->work);
> > > > + break;
> > > > + }
> > > > +
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +
> > > > static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
> > > > const u32 *data, int count)
> > > > {
> > > > @@ -204,6 +254,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;
> > > > }
> > > >
> > > > @@ -273,3 +326,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.277.g8800431eea-goog
> > > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support
2024-12-06 22:23 ` Abhishek Pandit-Subedi
@ 2024-12-06 23:30 ` Dmitry Baryshkov
0 siblings, 0 replies; 24+ messages in thread
From: Dmitry Baryshkov @ 2024-12-06 23:30 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: heikki.krogerus, tzungbi, linux-usb, chrome-platform, jthies,
akuchynski, pmalani, Benson Leung, Guenter Roeck, linux-kernel
On 7 December 2024 00:23:42 EET, Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote:
>On Thu, Nov 14, 2024 at 2:55 AM Dmitry Baryshkov
><dmitry.baryshkov@linaro.org> wrote:
>>
>> On Wed, Nov 13, 2024 at 08:01:57PM -0800, Abhishek Pandit-Subedi wrote:
>> > On Fri, Nov 8, 2024 at 10:41 PM Dmitry Baryshkov
>> > <dmitry.baryshkov@linaro.org> wrote:
>> > >
>> > > On Thu, Nov 07, 2024 at 11:29:59AM -0800, Abhishek Pandit-Subedi wrote:
>> > > > Add support for entering and exiting Thunderbolt alt-mode using AP
>> > > > driven alt-mode.
>> > > >
>> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> > > > ---
>> > > >
>> > > > Changes in 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/Makefile | 3 +
>> > > > drivers/platform/chrome/cros_ec_typec.c | 23 +++---
>> > > > drivers/platform/chrome/cros_typec_altmode.c | 85 ++++++++++++++++++++
>> > > > drivers/platform/chrome/cros_typec_altmode.h | 14 ++++
>> > > > 4 files changed, 114 insertions(+), 11 deletions(-)
>> > > >
>> > > > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
>> > > > index 2f90d4db8099..b9b1281de063 100644
>> > > > --- a/drivers/platform/chrome/Makefile
>> > > > +++ b/drivers/platform/chrome/Makefile
>> > > > @@ -21,6 +21,9 @@ cros-ec-typec-objs := cros_ec_typec.o cros_typec_vdm.o
>> > > > ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
>> > > > cros-ec-typec-objs += cros_typec_altmode.o
>> > > > endif
>> > > > +ifneq ($(CONFIG_TYPEC_TBT_ALTMODE),)
>> > > > + cros-ec-typec-objs += cros_typec_altmode.o
>> > > > +endif
>> > >
>> > > Doesn't this also result in the object file being included twice and
>> > > thus in a duplicate symbols declaration?
>> >
>> > I was trying to figure out how to add this file if either of those
>> > config options existed in
>> > https://docs.kernel.org/kbuild/makefiles.html#built-in-object-goals-obj-y
>> > and it says, "Duplicates in the lists are allowed: the first instance
>> > will be linked into built-in.a and succeeding instances will be
>> > ignored."
>> >
>> > Is there a preferred way of doing the following in the Makefile:
>> > if (defined(CONFIG_TYPEC_TBT_ALTMODE) ||
>> > defined(CONFIG_TYPEC_DP_ALTMODE)) {...}
>> >
>> > I briefly considered the following and dropped it because it is
>> > terrible readability-wise:
>> > ifneq ($(CONFIG_TYPEC_TBT_ALTMODE)$(CONFIG_TYPEC_DP_ALTMODE),)
>>
>> The usual way would to define new Kconfig symbol:
>>
>> config CROS_EC_TYPEC_ALTMODES
>> bool # Note, no description here, don't show in menuconfig
>> help
>> Selectable symbol to enable altmodes
>>
>> config CROS_EC_TYPEC
>> ...
>> select CROS_EC_TYPEC_ALTMODES if CONFIG_TYPEC_DP_ALTMODE
>> select CROS_EC_TYPEC_ALTMODES if CONFIG_TYPEC_TBT_ALTMODE
>> ...
>>
>> ----
>>
>> cros-ec-typec-$(CONFIG_CROS_EC_TYPEC_ALTMODES) += cros_typec_altmode.o
>>
>> >
>> > >
>> > > > obj-$(CONFIG_CROS_EC_TYPEC) += cros-ec-typec.o
>> > > >
>> > > > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
>> > > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>> > > > index 3a6f5f2717b9..558b618df63c 100644
>> > > > --- a/drivers/platform/chrome/cros_ec_typec.c
>> > > > +++ b/drivers/platform/chrome/cros_ec_typec.c
>> > > > @@ -302,18 +302,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 3598b8a6ceee..9cf2cef6c277 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"
>> > > > @@ -67,6 +68,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;
>> > > >
>> > > > @@ -196,6 +199,53 @@ static int cros_typec_displayport_vdm(struct typec_altmode *alt, u32 header,
>> > > > return 0;
>> > > > }
>> > > >
>> > > > +static int cros_typec_thunderbolt_vdm(struct typec_altmode *alt, u32 header,
>> > > > + const u32 *data, int count)
>> > > > +{
>> > > > + struct cros_typec_altmode_data *adata = typec_altmode_get_drvdata(alt);
>> > > > +
>> > > > + int cmd_type = PD_VDO_CMDT(header);
>> > > > + int cmd = PD_VDO_CMD(header);
>> > > > + int svdm_version;
>> > >
>> > > I suppose that with the current approach this misses the ap_mode_entry
>> > > check. If it gets moved to cros_typec_altmode_vdm(), then it should be
>> > > okay.
>> >
>> > We don't register the thunderbolt port driver if ap_mode_entry is
>> > false so it's an unnecessary check.
>>
>> Why don't you register it? It would allow userspace to understand, what
>> is happening, e.g. that the Type-C has switched to the TBT mode.
>
>Cros-EC does not support automatic entry of Thunderbolt/USB4 (i.e. all
>firmware that supports TBT/USB4 MUST set ap_mode_entry).
Then this definitely looks like something that should also be handled by boltctl. Please consider patching it.
>
>>
>> >
>> > >
>> > > > +
>> > > > + svdm_version = typec_altmode_get_svdm_version(alt);
>> > > > + if (svdm_version < 0)
>> > > > + return svdm_version;
>> > > > +
>> > > > + switch (cmd_type) {
>> > > > + case CMDT_INIT:
>> > > > + if (PD_VDO_SVDM_VER(header) < svdm_version) {
>> > > > + typec_partner_set_svdm_version(adata->port->partner,
>> > > > + PD_VDO_SVDM_VER(header));
>> > > > + svdm_version = PD_VDO_SVDM_VER(header);
>> > > > + }
>> > > > +
>> > > > + adata->header = VDO(adata->sid, 1, svdm_version, cmd);
>> > > > + adata->header |= VDO_OPOS(adata->mode);
>> > > > +
>> > > > + switch (cmd) {
>> > > > + case CMD_ENTER_MODE:
>> > > > + /* Don't respond to the enter mode vdm because it
>> > > > + * triggers mux configuration. This is handled directly
>> > > > + * by the cros_ec_typec driver so the Thunderbolt driver
>> > > > + * doesn't need to be involved.
>> > > > + */
>> > > > + break;
>> > > > + default:
>> > > > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
>> > > > + schedule_work(&adata->work);
>> > > > + break;
>> > > > + }
>> > > > +
>> > > > + break;
>> > > > + default:
>> > > > + break;
>> > > > + }
>> > > > +
>> > > > + return 0;
>> > > > +}
>> > > > +
>> > > > +
>> > > > static int cros_typec_altmode_vdm(struct typec_altmode *alt, u32 header,
>> > > > const u32 *data, int count)
>> > > > {
>> > > > @@ -204,6 +254,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;
>> > > > }
>> > > >
>> > > > @@ -273,3 +326,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.277.g8800431eea-goog
>> > > >
>> > >
>> > > --
>> > > With best wishes
>> > > Dmitry
>>
>> --
>> With best wishes
>> Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 7/7] platform/chrome: cros_ec_typec: Disable tbt on port
2024-11-07 19:29 [PATCH v3 0/7] Thunderbolt and DP altmode support for cros-ec-typec Abhishek Pandit-Subedi
` (5 preceding siblings ...)
2024-11-07 19:29 ` [PATCH v3 6/7] platform/chrome: cros_ec_typec: Thunderbolt support Abhishek Pandit-Subedi
@ 2024-11-07 19:30 ` Abhishek Pandit-Subedi
6 siblings, 0 replies; 24+ messages in thread
From: Abhishek Pandit-Subedi @ 2024-11-07 19:30 UTC (permalink / raw)
To: heikki.krogerus, tzungbi, linux-usb, chrome-platform
Cc: jthies, akuchynski, pmalani, dmitry.baryshkov,
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>
---
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 558b618df63c..b01efe82fb1e 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -310,6 +310,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.277.g8800431eea-goog
^ permalink raw reply related [flat|nested] 24+ messages in thread