| Message ID | 20190201104758.1483-5-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Support for DP alt mode | expand |
Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a 1-based index, not a 32-bit mode VDO) can cause incorrect matches if the GET_ALTERNATE_MODES returns different ordering for recipient=connector and recipient=sop for a particular svid.
For example, UCSI command GET_ALTERNATE_MODES with recipient=connector returns
[0] SVID=0xff01, ModeVDO=0x00000405 (Mode = 1)
[1] SVID=0xff01, ModeVDO=0x00000805 (Mode = 2)
And UCSI command GET_ALTERNATE_MODES with recipient=sop returns
[0] SVID=0xff01, ModeVDO=0x00000c05 (Mode = 1)
Then when DP alternate mode with pin D is active, GET_CURRENT_CAM returns index 1 (connector alternate mode = 1, i.e. SVID=0xff01, Mode=2, ModeVDO=0x00000805).
But, the function will be unable to match it with partner_alt_mode corresponding to (SVID=0xff01,Mode=1).
Can you compare against 32-bit VDO instead of ->mode? Also, use '&' bitwise AND operator when masking the partner's mode VDO (0x0c05) against the connector's mode VDO (0x405 or 0x805) to determine it there is an alternate mode match.
> +void ucsi_altmode_update_active(struct ucsi_connector *con) {
> + struct typec_altmode *altmode;
> + struct ucsi_control ctrl;
> + int ret;
> + u8 cur;
> + int i;
> +
> + UCSI_CMD_GET_CURRENT_CAM(ctrl, con->num);
> + ret = ucsi_run_command(con->ucsi, &ctrl, &cur, sizeof(cur));
> + if (ret < 0) {
> + dev_err(con->ucsi->dev, "GET_CURRENT_CAM command failed\n");
> + return;
> + }
> +
> + altmode = typec_match_altmode(con->partner_altmode, UCSI_MAX_ALTMODES,
> + con->port_altmode[cur]->svid,
> + con->port_altmode[cur]->mode);
> +
> + for (i = 0; con->partner_altmode[i]; i++)
> + typec_altmode_update_active(con->partner_altmode[i],
> + con->partner_altmode[i] == altmode); }
> +
nvpublic
-----Original Message-----
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Sent: Friday, February 1, 2019 2:48 AM
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta <ajayg@nvidia.com>; Michael Hsu <mhsu@nvidia.com>
Cc: linux-usb@vger.kernel.org
Subject: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
With UCSI the alternate modes, just like everything else related to USB Type-C connectors, are handled in firmware.
The operating system can see the status and is allowed to request certain things, for example entering and exiting the modes, but the support for alternate modes is very limited in UCSI. The feature is also optional, which means that even when the platform supports alternate modes, the operating system may not be even made aware of them.
UCSI does not support direct VDM reading or writing.
Instead, alternate modes can be entered and exited using a single custom command which takes also an optional SVID specific configuration value as parameter. That means every supported alternate mode has to be handled separately in UCSI driver.
This commit does not include support for any specific alternate mode. The discovered alternate modes are now registered, but binding a driver to an alternate mode will not be possible until support for that alternate mode is added to the UCSI driver.
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
drivers/usb/typec/ucsi/trace.c | 12 ++ drivers/usb/typec/ucsi/trace.h | 26 +++ drivers/usb/typec/ucsi/ucsi.c | 351 +++++++++++++++++++++++++++------ drivers/usb/typec/ucsi/ucsi.h | 72 +++++++
4 files changed, 396 insertions(+), 65 deletions(-)
diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c index ffa3b4c3f338..1dabafb74320 100644
--- a/drivers/usb/typec/ucsi/trace.c
+++ b/drivers/usb/typec/ucsi/trace.c
@@ -60,3 +60,15 @@ const char *ucsi_cci_str(u32 cci)
return "";
}
+
+static const char * const ucsi_recipient_strs[] = {
+ [UCSI_RECIPIENT_CON] = "port",
+ [UCSI_RECIPIENT_SOP] = "partner",
+ [UCSI_RECIPIENT_SOP_P] = "plug (prime)",
+ [UCSI_RECIPIENT_SOP_PP] = "plug (double prime)",
+};
+
+const char *ucsi_recipient_str(u8 recipient) {
+ return ucsi_recipient_strs[recipient]; }
diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h index 5e2906df2db7..783ec9c72055 100644
--- a/drivers/usb/typec/ucsi/trace.h
+++ b/drivers/usb/typec/ucsi/trace.h
@@ -7,6 +7,7 @@
#define __UCSI_TRACE_H
#include <linux/tracepoint.h>
+#include <linux/usb/typec_altmode.h>
const char *ucsi_cmd_str(u64 raw_cmd);
const char *ucsi_ack_str(u8 ack);
@@ -134,6 +135,31 @@ DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port,
TP_ARGS(port, status)
);
+DECLARE_EVENT_CLASS(ucsi_log_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt),
+ TP_STRUCT__entry(
+ __field(u8, recipient)
+ __field(u16, svid)
+ __field(u8, mode)
+ __field(u32, vdo)
+ ),
+ TP_fast_assign(
+ __entry->recipient = recipient;
+ __entry->svid = alt->svid;
+ __entry->mode = alt->mode;
+ __entry->vdo = alt->vdo;
+ ),
+ TP_printk("%s alt mode: svid %04x, mode %d vdo %x",
+ ucsi_recipient_str(__entry->recipient), __entry->svid,
+ __entry->mode, __entry->vdo)
+);
+
+DEFINE_EVENT(ucsi_log_register_altmode, ucsi_register_altmode,
+ TP_PROTO(u8 recipient, struct typec_altmode *alt),
+ TP_ARGS(recipient, alt)
+);
+
#endif /* __UCSI_TRACE_H */
/* This part must be outside protection */ diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 8d0a6fe748bd..5190f8dd4548 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -12,7 +12,7 @@
#include <linux/module.h>
#include <linux/delay.h>
#include <linux/slab.h>
-#include <linux/usb/typec.h>
+#include <linux/usb/typec_altmode.h>
#include "ucsi.h"
#include "trace.h"
@@ -45,22 +45,6 @@ enum ucsi_status {
UCSI_ERROR,
};
-struct ucsi_connector {
- int num;
-
- struct ucsi *ucsi;
- struct work_struct work;
- struct completion complete;
-
- struct typec_port *port;
- struct typec_partner *partner;
-
- struct typec_capability typec_cap;
-
- struct ucsi_connector_status status;
- struct ucsi_connector_capability cap;
-};
-
struct ucsi {
struct device *dev;
struct ucsi_ppm *ppm;
@@ -238,8 +222,201 @@ static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
return ret;
}
+int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+ void *retval, size_t size)
+{
+ int ret;
+
+ mutex_lock(&ucsi->ppm_lock);
+ ret = ucsi_run_command(ucsi, ctrl, retval, size);
+ mutex_unlock(&ucsi->ppm_lock);
+
+ return ret;
+}
+
/* -------------------------------------------------------------------------- */
+void ucsi_altmode_update_active(struct ucsi_connector *con) {
+ struct typec_altmode *altmode;
+ struct ucsi_control ctrl;
+ int ret;
+ u8 cur;
+ int i;
+
+ UCSI_CMD_GET_CURRENT_CAM(ctrl, con->num);
+ ret = ucsi_run_command(con->ucsi, &ctrl, &cur, sizeof(cur));
+ if (ret < 0) {
+ dev_err(con->ucsi->dev, "GET_CURRENT_CAM command failed\n");
+ return;
+ }
+
+ altmode = typec_match_altmode(con->partner_altmode, UCSI_MAX_ALTMODES,
+ con->port_altmode[cur]->svid,
+ con->port_altmode[cur]->mode);
+
+ for (i = 0; con->partner_altmode[i]; i++)
+ typec_altmode_update_active(con->partner_altmode[i],
+ con->partner_altmode[i] == altmode); }
+
+static u8 ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid)
+{
+ u8 mode = 1;
+ int i;
+
+ for (i = 0; alt[i]; i++)
+ if (alt[i]->svid == svid)
+ mode++;
+
+ return mode;
+}
+
+static int ucsi_next_altmode(struct typec_altmode **alt) {
+ int i = 0;
+
+ for (i = 0; i < UCSI_MAX_ALTMODES; i++)
+ if (!alt[i])
+ return i;
+
+ return -ENOENT;
+}
+
+static int ucsi_register_altmode(struct ucsi_connector *con,
+ struct typec_altmode_desc *desc,
+ u8 recipient)
+{
+ struct typec_altmode *alt;
+ int ret;
+ int i;
+
+ switch (recipient) {
+ case UCSI_RECIPIENT_CON:
+ i = ucsi_next_altmode(con->port_altmode);
+ if (i < 0) {
+ ret = i;
+ goto err;
+ }
+
+ desc->mode = ucsi_altmode_next_mode(con->port_altmode,
+ desc->svid);
+
+ alt = typec_port_register_altmode(con->port, desc);
+ if (IS_ERR(alt)) {
+ ret = PTR_ERR(alt);
+ goto err;
+ }
+
+ con->port_altmode[i] = alt;
+ break;
+ case UCSI_RECIPIENT_SOP:
+ i = ucsi_next_altmode(con->partner_altmode);
+ if (i < 0) {
+ ret = i;
+ goto err;
+ }
+
+ desc->mode = ucsi_altmode_next_mode(con->partner_altmode,
+ desc->svid);
+
+ alt = typec_partner_register_altmode(con->partner, desc);
+ if (IS_ERR(alt)) {
+ ret = PTR_ERR(alt);
+ goto err;
+ }
+
+ con->partner_altmode[i] = alt;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ trace_ucsi_register_altmode(recipient, alt);
+
+ return 0;
+
+err:
+ dev_err(con->ucsi->dev, "failed to registers svid 0x%04x mode %d\n",
+ desc->svid, desc->mode);
+
+ return ret;
+}
+
+static int ucsi_register_altmodes(struct ucsi_connector *con, u8
+recipient) {
+ int max_altmodes = UCSI_MAX_ALTMODES;
+ struct typec_altmode_desc desc;
+ struct ucsi_altmode alt[2];
+ struct ucsi_control ctrl;
+ int num = 1;
+ int ret;
+ int len;
+ int j;
+ int i;
+
+ if (!(con->ucsi->cap.features & UCSI_CAP_ALT_MODE_DETAILS))
+ return 0;
+
+ if (recipient == UCSI_RECIPIENT_CON)
+ max_altmodes = con->ucsi->cap.num_alt_modes;
+
+ for (i = 0; i < max_altmodes;) {
+ memset(alt, 0, sizeof(alt));
+ UCSI_CMD_GET_ALTERNATE_MODES(ctrl, recipient, con->num, i, 1);
+ len = ucsi_run_command(con->ucsi, &ctrl, alt, sizeof(alt));
+ if (len <= 0)
+ return len;
+
+ /*
+ * This code is requesting one alt mode at a time, but some PPMs
+ * may still return two. If that happens both alt modes need be
+ * registered and the offset for the next alt mode has to be
+ * incremented.
+ */
+ num = len / sizeof(alt[0]);
+ i += num;
+
+ for (j = 0; j < num; j++) {
+ if (!alt[j].svid)
+ return 0;
+
+ memset(&desc, 0, sizeof(desc));
+ desc.vdo = alt[j].mid;
+ desc.svid = alt[j].svid;
+ desc.roles = TYPEC_PORT_DRD;
+
+ ret = ucsi_register_altmode(con, &desc, recipient);
+ if (ret)
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8
+recipient) {
+ struct typec_altmode **adev;
+ int i = 0;
+
+ switch (recipient) {
+ case UCSI_RECIPIENT_CON:
+ adev = con->port_altmode;
+ break;
+ case UCSI_RECIPIENT_SOP:
+ adev = con->partner_altmode;
+ break;
+ default:
+ return;
+ }
+
+ while (adev[i]) {
+ typec_unregister_altmode(adev[i]);
+ adev[i++] = NULL;
+ }
+}
+
static void ucsi_pwr_opmode_change(struct ucsi_connector *con) {
switch (con->status.pwr_op_mode) {
@@ -299,10 +476,43 @@ static void ucsi_unregister_partner(struct ucsi_connector *con)
if (!con->partner)
return;
+ ucsi_unregister_altmodes(con, UCSI_RECIPIENT_SOP);
typec_unregister_partner(con->partner);
con->partner = NULL;
}
+static void ucsi_partner_change(struct ucsi_connector *con) {
+ int ret;
+
+ if (!con->partner)
+ return;
+
+ switch (con->status.partner_type) {
+ case UCSI_CONSTAT_PARTNER_TYPE_UFP:
+ typec_set_data_role(con->port, TYPEC_HOST);
+ break;
+ case UCSI_CONSTAT_PARTNER_TYPE_DFP:
+ typec_set_data_role(con->port, TYPEC_DEVICE);
+ break;
+ default:
+ break;
+ }
+
+ /* Complete pending data role swap */
+ if (!completion_done(&con->complete))
+ complete(&con->complete);
+
+ /* Can't rely on Partner Flags field. Always checking the alt modes. */
+ ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
+ if (ret)
+ dev_err(con->ucsi->dev,
+ "con%d: failed to register partner alternate modes\n",
+ con->num);
+ else
+ ucsi_altmode_update_active(con);
+}
+
static void ucsi_connector_change(struct work_struct *work) {
struct ucsi_connector *con = container_of(work, struct ucsi_connector, @@ -311,10 +521,10 @@ static void ucsi_connector_change(struct work_struct *work)
struct ucsi_control ctrl;
int ret;
- mutex_lock(&ucsi->ppm_lock);
+ mutex_lock(&con->lock);
UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num);
- ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status));
+ ret = ucsi_send_command(ucsi, &ctrl, &con->status,
+sizeof(con->status));
if (ret < 0) {
dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
__func__, ret);
@@ -332,23 +542,6 @@ static void ucsi_connector_change(struct work_struct *work)
complete(&con->complete);
}
- if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) {
- switch (con->status.partner_type) {
- case UCSI_CONSTAT_PARTNER_TYPE_UFP:
- typec_set_data_role(con->port, TYPEC_HOST);
- break;
- case UCSI_CONSTAT_PARTNER_TYPE_DFP:
- typec_set_data_role(con->port, TYPEC_DEVICE);
- break;
- default:
- break;
- }
-
- /* Complete pending data role swap */
- if (!completion_done(&con->complete))
- complete(&con->complete);
- }
-
if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) {
typec_set_pwr_role(con->port, con->status.pwr_dir);
@@ -369,6 +562,19 @@ static void ucsi_connector_change(struct work_struct *work)
ucsi_unregister_partner(con);
}
+ if (con->status.change & UCSI_CONSTAT_CAM_CHANGE) {
+ /*
+ * We don't need to know the currently supported alt modes here.
+ * Running GET_CAM_SUPPORTED command just to make sure the PPM
+ * does not get stuck in case it assumes we do so.
+ */
+ UCSI_CMD_GET_CAM_SUPPORTED(ctrl, con->num);
+ ucsi_run_command(con->ucsi, &ctrl, NULL, 0);
+ }
+
+ if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
+ ucsi_partner_change(con);
+
ret = ucsi_ack(ucsi, UCSI_ACK_EVENT);
if (ret)
dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret); @@ -377,7 +583,7 @@ static void ucsi_connector_change(struct work_struct *work)
out_unlock:
clear_bit(EVENT_PENDING, &ucsi->flags);
- mutex_unlock(&ucsi->ppm_lock);
+ mutex_unlock(&con->lock);
}
/**
@@ -427,7 +633,7 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
UCSI_CMD_CONNECTOR_RESET(ctrl, con, hard);
- return ucsi_run_command(con->ucsi, &ctrl, NULL, 0);
+ return ucsi_send_command(con->ucsi, &ctrl, NULL, 0);
}
static int ucsi_reset_ppm(struct ucsi *ucsi) @@ -481,15 +687,17 @@ static int ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control *ctrl) {
int ret;
- ret = ucsi_run_command(con->ucsi, ctrl, NULL, 0);
+ ret = ucsi_send_command(con->ucsi, ctrl, NULL, 0);
if (ret == -ETIMEDOUT) {
struct ucsi_control c;
/* PPM most likely stopped responding. Resetting everything. */
+ mutex_lock(&con->ucsi->ppm_lock);
ucsi_reset_ppm(con->ucsi);
+ mutex_unlock(&con->ucsi->ppm_lock);
UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
- ucsi_run_command(con->ucsi, &c, NULL, 0);
+ ucsi_send_command(con->ucsi, &c, NULL, 0);
ucsi_reset_connector(con, true);
}
@@ -504,10 +712,12 @@ ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role)
struct ucsi_control ctrl;
int ret = 0;
- if (!con->partner)
- return -ENOTCONN;
+ mutex_lock(&con->lock);
- mutex_lock(&con->ucsi->ppm_lock);
+ if (!con->partner) {
+ ret = -ENOTCONN;
+ goto out_unlock;
+ }
if ((con->status.partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP &&
role == TYPEC_DEVICE) ||
@@ -520,18 +730,14 @@ ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role)
if (ret < 0)
goto out_unlock;
- mutex_unlock(&con->ucsi->ppm_lock);
-
if (!wait_for_completion_timeout(&con->complete,
msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS)))
- return -ETIMEDOUT;
-
- return 0;
+ ret = -ETIMEDOUT;
out_unlock:
- mutex_unlock(&con->ucsi->ppm_lock);
+ mutex_unlock(&con->lock);
- return ret;
+ return ret < 0 ? ret : 0;
}
static int
@@ -541,10 +747,12 @@ ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role)
struct ucsi_control ctrl;
int ret = 0;
- if (!con->partner)
- return -ENOTCONN;
+ mutex_lock(&con->lock);
- mutex_lock(&con->ucsi->ppm_lock);
+ if (!con->partner) {
+ ret = -ENOTCONN;
+ goto out_unlock;
+ }
if (con->status.pwr_dir == role)
goto out_unlock;
@@ -554,13 +762,11 @@ ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role)
if (ret < 0)
goto out_unlock;
- mutex_unlock(&con->ucsi->ppm_lock);
-
if (!wait_for_completion_timeout(&con->complete,
- msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS)))
- return -ETIMEDOUT;
-
- mutex_lock(&con->ucsi->ppm_lock);
+ msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) {
+ ret = -ETIMEDOUT;
+ goto out_unlock;
+ }
/* Something has gone wrong while swapping the role */
if (con->status.pwr_op_mode != UCSI_CONSTAT_PWR_OPMODE_PD) { @@ -569,7 +775,7 @@ ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role)
}
out_unlock:
- mutex_unlock(&con->ucsi->ppm_lock);
+ mutex_unlock(&con->lock);
return ret;
}
@@ -595,6 +801,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
INIT_WORK(&con->work, ucsi_connector_change);
init_completion(&con->complete);
+ mutex_init(&con->lock);
con->num = index + 1;
con->ucsi = ucsi;
@@ -636,6 +843,12 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
if (IS_ERR(con->port))
return PTR_ERR(con->port);
+ /* Alternate modes */
+ ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
+ if (ret)
+ dev_err(ucsi->dev, "con%d: failed to register alt modes\n",
+ con->num);
+
/* Get the status */
UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num);
ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status)); @@ -662,6 +875,16 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
if (con->status.connected)
ucsi_register_partner(con);
+ if (con->partner) {
+ ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
+ if (ret)
+ dev_err(ucsi->dev,
+ "con%d: failed to register alternate modes\n",
+ con->num);
+ else
+ ucsi_altmode_update_active(con);
+ }
+
trace_ucsi_register_port(con->num, &con->status);
return 0;
@@ -788,17 +1011,15 @@ void ucsi_unregister_ppm(struct ucsi *ucsi)
/* Make sure that we are not in the middle of driver initialization */
cancel_work_sync(&ucsi->work);
- mutex_lock(&ucsi->ppm_lock);
-
/* Disable everything except command complete notification */
UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_CMD_COMPLETE)
- ucsi_run_command(ucsi, &ctrl, NULL, 0);
-
- mutex_unlock(&ucsi->ppm_lock);
+ ucsi_send_command(ucsi, &ctrl, NULL, 0);
for (i = 0; i < ucsi->cap.num_connectors; i++) {
cancel_work_sync(&ucsi->connector[i].work);
ucsi_unregister_partner(&ucsi->connector[i]);
+ ucsi_unregister_altmodes(&ucsi->connector[i],
+ UCSI_RECIPIENT_CON);
typec_unregister_port(ucsi->connector[i].port);
}
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 53b80f40a908..c416bae4b5ca 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -6,6 +6,7 @@
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/types.h>
+#include <linux/usb/typec.h>
/* -------------------------------------------------------------------------- */
@@ -60,6 +61,20 @@ struct ucsi_uor_cmd {
u16:6; /* reserved */
} __packed;
+/* Get Alternate Modes Command structure */ struct ucsi_altmode_cmd {
+ u8 cmd;
+ u8 length;
+ u8 recipient;
+#define UCSI_RECIPIENT_CON 0
+#define UCSI_RECIPIENT_SOP 1
+#define UCSI_RECIPIENT_SOP_P 2
+#define UCSI_RECIPIENT_SOP_PP 3
+ u8 con_num;
+ u8 offset;
+ u8 num_altmodes;
+} __packed;
+
struct ucsi_control {
union {
u64 raw_cmd;
@@ -67,6 +82,7 @@ struct ucsi_control {
struct ucsi_uor_cmd uor;
struct ucsi_ack_cmd ack;
struct ucsi_con_rst con_rst;
+ struct ucsi_altmode_cmd alt;
};
};
@@ -112,6 +128,30 @@ struct ucsi_control {
(_ctrl_).cmd.data = _con_; \
}
+/* Helper for preparing ucsi_control for GET_ALTERNATE_MODES command.
+*/ #define UCSI_CMD_GET_ALTERNATE_MODES(_ctrl_, _r_, _con_num_, _o_, _num_)\
+{ \
+ __UCSI_CMD((_ctrl_), UCSI_GET_ALTERNATE_MODES) \
+ _ctrl_.alt.recipient = (_r_); \
+ _ctrl_.alt.con_num = (_con_num_); \
+ _ctrl_.alt.offset = (_o_); \
+ _ctrl_.alt.num_altmodes = (_num_) - 1; \
+}
+
+/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */
+#define UCSI_CMD_GET_CAM_SUPPORTED(_ctrl_, _con_) \
+{ \
+ __UCSI_CMD((_ctrl_), UCSI_GET_CAM_SUPPORTED) \
+ _ctrl_.cmd.data = (_con_); \
+}
+
+/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */
+#define UCSI_CMD_GET_CURRENT_CAM(_ctrl_, _con_) \
+{ \
+ __UCSI_CMD((_ctrl_), UCSI_GET_CURRENT_CAM) \
+ _ctrl_.cmd.data = (_con_); \
+}
+
/* Helper for preparing ucsi_control for GET_CONNECTOR_STATUS command. */
#define UCSI_CMD_GET_CONNECTOR_STATUS(_ctrl_, _con_) \
{ \
@@ -334,4 +374,36 @@ struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm); void ucsi_unregister_ppm(struct ucsi *ucsi); void ucsi_notify(struct ucsi *ucsi);
+/*
+-----------------------------------------------------------------------
+--- */
+
+struct ucsi;
+
+#define UCSI_MAX_SVID 5
+#define UCSI_MAX_ALTMODES (UCSI_MAX_SVID * 6)
+
+struct ucsi_connector {
+ int num;
+
+ struct ucsi *ucsi;
+ struct mutex lock; /* port lock */
+ struct work_struct work;
+ struct completion complete;
+
+ struct typec_port *port;
+ struct typec_partner *partner;
+
+ struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES];
+ struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES];
+
+ struct typec_capability typec_cap;
+
+ struct ucsi_connector_status status;
+ struct ucsi_connector_capability cap;
+};
+
+int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+ void *retval, size_t size);
+
+void ucsi_altmode_update_active(struct ucsi_connector *con);
+
#endif /* __DRIVER_USB_TYPEC_UCSI_H */
--
2.20.1
Hi, On Fri, Feb 01, 2019 at 10:02:19PM +0000, Michael Hsu wrote: > Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a 1-based > index, not a 32-bit mode VDO) can cause incorrect matches if the > GET_ALTERNATE_MODES returns different ordering for recipient=connector and > recipient=sop for a particular svid. > > For example, UCSI command GET_ALTERNATE_MODES with recipient=connector returns > [0] SVID=0xff01, ModeVDO=0x00000405 (Mode = 1) > [1] SVID=0xff01, ModeVDO=0x00000805 (Mode = 2) > And UCSI command GET_ALTERNATE_MODES with recipient=sop returns > [0] SVID=0xff01, ModeVDO=0x00000c05 (Mode = 1) > > Then when DP alternate mode with pin D is active, GET_CURRENT_CAM returns > index 1 (connector alternate mode = 1, i.e. SVID=0xff01, Mode=2, > ModeVDO=0x00000805). > But, the function will be unable to match it with partner_alt_mode > corresponding to (SVID=0xff01,Mode=1). > > Can you compare against 32-bit VDO instead of ->mode? Also, use '&' bitwise > AND operator when masking the partner's mode VDO (0x0c05) against the > connector's mode VDO (0x405 or 0x805) to determine it there is an alternate > mode match. No top-posting! From now on inline your comments in your replies. The "process" guide in kernel should provide more information for you: https://www.kernel.org/doc/html/latest/process/2.Process.html#mailing-lists Your PPM is reporting a separate mode for every pin-assignment it supports. It really should _not_ do that! You need to be able to get the capabilities for DP alt mode with GET_ALTERNATE_MODE command in the format that the DP alt mode spec defines, also with the connector. You are not getting them like that. Now for example in both modes your connector can only act as DisplayPort sink (i.e. a display) which I'm pretty sure is not the case. With the current version of the DP alt mode spec we do not ever need more than one mode for DP. That mode needs to show all the supported pin assignments the device, partner or connector, supports. That is exactly how all the other UCSI PPMs work currently. For example the PPM on Dell XPS 13 9380 that I use for testing gives only one mode for the connector with SVID ff01. The mode VDO (or MID in UCSI lingo) has the value 0x1c46: SVID=0xff01, VDO=0x1c46 From that you can clearly see that the connector can only act as the DisplayPort source, and that it support pin assignments C, D and E. So in practice you have a firmware bug. I understand why the PPM was made that way. That "hack" allows the PPM to workaround a limitation in UCSI where we in practice can't tell which configuration is in use at any given time. Unfortunately it can not be done like that. It leaves us with custom VDO values that we would have to be interpreted separately, that only your PPM supports. Please see if you can get the firmware (UCSI PPM) fixed. It really needs to expose only one mode for DisplayPort alt mode in the connector, and the VDO in it should be in the same form that it could be send to the partner, i.e. giving the actual DisplayPort capabilities for the connector. We should not do anything before you ask them to fix the PPM. Experience tells that if we workaround an issue in firmware, the firmware will never ever get fixed. And in this case the workaround may not be as simple as one would think. Even if we have to workaround this, it needs a separate patch with a good explanation. And it probable does not belong to this series. Thanks,
Hi Heikki > -----Original Message----- > From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org> On > Behalf Of Heikki Krogerus > Sent: Friday, February 1, 2019 2:48 AM > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > <ajayg@nvidia.com>; Michael Hsu <mhsu@nvidia.com> > Cc: linux-usb@vger.kernel.org > Subject: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes > > With UCSI the alternate modes, just like everything else related to USB Type-C > connectors, are handled in firmware. > The operating system can see the status and is allowed to request certain things, > for example entering and exiting the modes, but the support for alternate > modes is very limited in UCSI. The feature is also optional, which means that > even when the platform supports alternate modes, the operating system may > not be even made aware of them. > > UCSI does not support direct VDM reading or writing. > Instead, alternate modes can be entered and exited using a single custom > command which takes also an optional SVID specific configuration value as > parameter. That means every supported alternate mode has to be handled > separately in UCSI driver. > > This commit does not include support for any specific alternate mode. The > discovered alternate modes are now registered, but binding a driver to an > alternate mode will not be possible until support for that alternate mode is > added to the UCSI driver. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/trace.c | 12 ++ drivers/usb/typec/ucsi/trace.h | 26 +++ > drivers/usb/typec/ucsi/ucsi.c | 351 +++++++++++++++++++++++++++------ > drivers/usb/typec/ucsi/ucsi.h | 72 +++++++ > 4 files changed, 396 insertions(+), 65 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c index > ffa3b4c3f338..1dabafb74320 100644 > --- a/drivers/usb/typec/ucsi/trace.c > +++ b/drivers/usb/typec/ucsi/trace.c > @@ -60,3 +60,15 @@ const char *ucsi_cci_str(u32 cci) > > return ""; > } > + > +static const char * const ucsi_recipient_strs[] = { > + [UCSI_RECIPIENT_CON] = "port", > + [UCSI_RECIPIENT_SOP] = "partner", > + [UCSI_RECIPIENT_SOP_P] = "plug (prime)", > + [UCSI_RECIPIENT_SOP_PP] = "plug (double prime)", > +}; > + > +const char *ucsi_recipient_str(u8 recipient) { > + return ucsi_recipient_strs[recipient]; } > diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h index > 5e2906df2db7..783ec9c72055 100644 > --- a/drivers/usb/typec/ucsi/trace.h > +++ b/drivers/usb/typec/ucsi/trace.h > @@ -7,6 +7,7 @@ > #define __UCSI_TRACE_H > > #include <linux/tracepoint.h> > +#include <linux/usb/typec_altmode.h> > > const char *ucsi_cmd_str(u64 raw_cmd); > const char *ucsi_ack_str(u8 ack); > @@ -134,6 +135,31 @@ DEFINE_EVENT(ucsi_log_connector_status, > ucsi_register_port, > TP_ARGS(port, status) > ); > > +DECLARE_EVENT_CLASS(ucsi_log_register_altmode, > + TP_PROTO(u8 recipient, struct typec_altmode *alt), > + TP_ARGS(recipient, alt), > + TP_STRUCT__entry( > + __field(u8, recipient) > + __field(u16, svid) > + __field(u8, mode) > + __field(u32, vdo) > + ), > + TP_fast_assign( > + __entry->recipient = recipient; > + __entry->svid = alt->svid; > + __entry->mode = alt->mode; > + __entry->vdo = alt->vdo; > + ), > + TP_printk("%s alt mode: svid %04x, mode %d vdo %x", > + ucsi_recipient_str(__entry->recipient), __entry->svid, > + __entry->mode, __entry->vdo) > +); > + > +DEFINE_EVENT(ucsi_log_register_altmode, ucsi_register_altmode, > + TP_PROTO(u8 recipient, struct typec_altmode *alt), > + TP_ARGS(recipient, alt) > +); > + > #endif /* __UCSI_TRACE_H */ > > /* This part must be outside protection */ diff --git > a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index > 8d0a6fe748bd..5190f8dd4548 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -12,7 +12,7 @@ > #include <linux/module.h> > #include <linux/delay.h> > #include <linux/slab.h> > -#include <linux/usb/typec.h> > +#include <linux/usb/typec_altmode.h> > > #include "ucsi.h" > #include "trace.h" > @@ -45,22 +45,6 @@ enum ucsi_status { > UCSI_ERROR, > }; > > -struct ucsi_connector { > - int num; > - > - struct ucsi *ucsi; > - struct work_struct work; > - struct completion complete; > - > - struct typec_port *port; > - struct typec_partner *partner; > - > - struct typec_capability typec_cap; > - > - struct ucsi_connector_status status; > - struct ucsi_connector_capability cap; > -}; > - > struct ucsi { > struct device *dev; > struct ucsi_ppm *ppm; > @@ -238,8 +222,201 @@ static int ucsi_run_command(struct ucsi *ucsi, struct > ucsi_control *ctrl, > return ret; > } > > +int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl, > + void *retval, size_t size) > +{ > + int ret; > + > + mutex_lock(&ucsi->ppm_lock); > + ret = ucsi_run_command(ucsi, ctrl, retval, size); > + mutex_unlock(&ucsi->ppm_lock); > + > + return ret; > +} > + > /* -------------------------------------------------------------------------- */ > > +void ucsi_altmode_update_active(struct ucsi_connector *con) { > + struct typec_altmode *altmode; > + struct ucsi_control ctrl; > + int ret; > + u8 cur; > + int i; > + > + UCSI_CMD_GET_CURRENT_CAM(ctrl, con->num); > + ret = ucsi_run_command(con->ucsi, &ctrl, &cur, sizeof(cur)); > + if (ret < 0) { > + dev_err(con->ucsi->dev, "GET_CURRENT_CAM command > failed\n"); > + return; > + } > + > + altmode = typec_match_altmode(con->partner_altmode, > UCSI_MAX_ALTMODES, > + con->port_altmode[cur]->svid, > + con->port_altmode[cur]->mode); > + > + for (i = 0; con->partner_altmode[i]; i++) > + typec_altmode_update_active(con->partner_altmode[i], > + con->partner_altmode[i] == > altmode); } > + > +static u8 ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid) > +{ > + u8 mode = 1; > + int i; > + > + for (i = 0; alt[i]; i++) > + if (alt[i]->svid == svid) > + mode++; > + > + return mode; > +} > + > +static int ucsi_next_altmode(struct typec_altmode **alt) { > + int i = 0; > + > + for (i = 0; i < UCSI_MAX_ALTMODES; i++) > + if (!alt[i]) > + return i; > + > + return -ENOENT; > +} > + > +static int ucsi_register_altmode(struct ucsi_connector *con, > + struct typec_altmode_desc *desc, > + u8 recipient) > +{ > + struct typec_altmode *alt; > + int ret; > + int i; > + > + switch (recipient) { > + case UCSI_RECIPIENT_CON: > + i = ucsi_next_altmode(con->port_altmode); > + if (i < 0) { > + ret = i; > + goto err; > + } > + > + desc->mode = ucsi_altmode_next_mode(con->port_altmode, > + desc->svid); > + > + alt = typec_port_register_altmode(con->port, desc); > + if (IS_ERR(alt)) { > + ret = PTR_ERR(alt); > + goto err; > + } > + > + con->port_altmode[i] = alt; > + break; > + case UCSI_RECIPIENT_SOP: > + i = ucsi_next_altmode(con->partner_altmode); We are seeing duplicate partner altmode devices getting created when we set "active" file from 1->0->1 Please add a check here to see if altmode device already exists. [...] case UCSI_RECIPIENT_SOP: /* check to see if partner altmode already exists */ if (ucsi_altmode_found(con->partner_altmode, desc)) break; i = ucsi_next_altmode(con->partner_altmode); if (i < 0) { [...] static bool ucsi_altmode_found(struct typec_altmode **alt, struct typec_altmode_desc *desc) { int i; for (i = 0; i < UCSI_MAX_ALTMODES; i++) { if (!alt[i]) return false; if (alt[i]->svid == desc->svid && alt[i]->vdo == desc->vdo) return true; } return false; } thanks > nvpublic > + if (i < 0) { > + ret = i; > + goto err; > + } > + > + desc->mode = ucsi_altmode_next_mode(con- > >partner_altmode, > + desc->svid); > + > + alt = typec_partner_register_altmode(con->partner, desc); > + if (IS_ERR(alt)) { > + ret = PTR_ERR(alt); > + goto err; > + } > + > + con->partner_altmode[i] = alt; > + break; > + default: > + return -EINVAL; > + } > + > + trace_ucsi_register_altmode(recipient, alt); > + > + return 0; > + > +err: > + dev_err(con->ucsi->dev, "failed to registers svid 0x%04x mode %d\n", > + desc->svid, desc->mode); > + > + return ret; > +} > + > +static int ucsi_register_altmodes(struct ucsi_connector *con, u8 > +recipient) { > + int max_altmodes = UCSI_MAX_ALTMODES; > + struct typec_altmode_desc desc; > + struct ucsi_altmode alt[2]; > + struct ucsi_control ctrl; > + int num = 1; > + int ret; > + int len; > + int j; > + int i; > + > + if (!(con->ucsi->cap.features & UCSI_CAP_ALT_MODE_DETAILS)) > + return 0; > + > + if (recipient == UCSI_RECIPIENT_CON) > + max_altmodes = con->ucsi->cap.num_alt_modes; > + > + for (i = 0; i < max_altmodes;) { > + memset(alt, 0, sizeof(alt)); > + UCSI_CMD_GET_ALTERNATE_MODES(ctrl, recipient, con->num, > i, 1); > + len = ucsi_run_command(con->ucsi, &ctrl, alt, sizeof(alt)); > + if (len <= 0) > + return len; > + > + /* > + * This code is requesting one alt mode at a time, but some > PPMs > + * may still return two. If that happens both alt modes need be > + * registered and the offset for the next alt mode has to be > + * incremented. > + */ > + num = len / sizeof(alt[0]); > + i += num; > + > + for (j = 0; j < num; j++) { > + if (!alt[j].svid) > + return 0; > + > + memset(&desc, 0, sizeof(desc)); > + desc.vdo = alt[j].mid; > + desc.svid = alt[j].svid; > + desc.roles = TYPEC_PORT_DRD; > + > + ret = ucsi_register_altmode(con, &desc, recipient); > + if (ret) > + return ret; > + } > + } > + > + return 0; > +} > + > +static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 > +recipient) { > + struct typec_altmode **adev; > + int i = 0; > + > + switch (recipient) { > + case UCSI_RECIPIENT_CON: > + adev = con->port_altmode; > + break; > + case UCSI_RECIPIENT_SOP: > + adev = con->partner_altmode; > + break; > + default: > + return; > + } > + > + while (adev[i]) { > + typec_unregister_altmode(adev[i]); > + adev[i++] = NULL; > + } > +} > + > static void ucsi_pwr_opmode_change(struct ucsi_connector *con) { > switch (con->status.pwr_op_mode) { > @@ -299,10 +476,43 @@ static void ucsi_unregister_partner(struct > ucsi_connector *con) > if (!con->partner) > return; > > + ucsi_unregister_altmodes(con, UCSI_RECIPIENT_SOP); > typec_unregister_partner(con->partner); > con->partner = NULL; > } > > +static void ucsi_partner_change(struct ucsi_connector *con) { > + int ret; > + > + if (!con->partner) > + return; > + > + switch (con->status.partner_type) { > + case UCSI_CONSTAT_PARTNER_TYPE_UFP: > + typec_set_data_role(con->port, TYPEC_HOST); > + break; > + case UCSI_CONSTAT_PARTNER_TYPE_DFP: > + typec_set_data_role(con->port, TYPEC_DEVICE); > + break; > + default: > + break; > + } > + > + /* Complete pending data role swap */ > + if (!completion_done(&con->complete)) > + complete(&con->complete); > + > + /* Can't rely on Partner Flags field. Always checking the alt modes. */ > + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP); > + if (ret) > + dev_err(con->ucsi->dev, > + "con%d: failed to register partner alternate modes\n", > + con->num); > + else > + ucsi_altmode_update_active(con); > +} > + > static void ucsi_connector_change(struct work_struct *work) { > struct ucsi_connector *con = container_of(work, struct ucsi_connector, > @@ -311,10 +521,10 @@ static void ucsi_connector_change(struct work_struct > *work) > struct ucsi_control ctrl; > int ret; > > - mutex_lock(&ucsi->ppm_lock); > + mutex_lock(&con->lock); > > UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num); > - ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status)); > + ret = ucsi_send_command(ucsi, &ctrl, &con->status, > +sizeof(con->status)); > if (ret < 0) { > dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed > (%d)\n", > __func__, ret); > @@ -332,23 +542,6 @@ static void ucsi_connector_change(struct work_struct > *work) > complete(&con->complete); > } > > - if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) { > - switch (con->status.partner_type) { > - case UCSI_CONSTAT_PARTNER_TYPE_UFP: > - typec_set_data_role(con->port, TYPEC_HOST); > - break; > - case UCSI_CONSTAT_PARTNER_TYPE_DFP: > - typec_set_data_role(con->port, TYPEC_DEVICE); > - break; > - default: > - break; > - } > - > - /* Complete pending data role swap */ > - if (!completion_done(&con->complete)) > - complete(&con->complete); > - } > - > if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) { > typec_set_pwr_role(con->port, con->status.pwr_dir); > > @@ -369,6 +562,19 @@ static void ucsi_connector_change(struct work_struct > *work) > ucsi_unregister_partner(con); > } > > + if (con->status.change & UCSI_CONSTAT_CAM_CHANGE) { > + /* > + * We don't need to know the currently supported alt modes > here. > + * Running GET_CAM_SUPPORTED command just to make sure > the PPM > + * does not get stuck in case it assumes we do so. > + */ > + UCSI_CMD_GET_CAM_SUPPORTED(ctrl, con->num); > + ucsi_run_command(con->ucsi, &ctrl, NULL, 0); > + } > + > + if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) > + ucsi_partner_change(con); > + > ret = ucsi_ack(ucsi, UCSI_ACK_EVENT); > if (ret) > dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret); @@ - > 377,7 +583,7 @@ static void ucsi_connector_change(struct work_struct *work) > > out_unlock: > clear_bit(EVENT_PENDING, &ucsi->flags); > - mutex_unlock(&ucsi->ppm_lock); > + mutex_unlock(&con->lock); > } > > /** > @@ -427,7 +633,7 @@ static int ucsi_reset_connector(struct ucsi_connector > *con, bool hard) > > UCSI_CMD_CONNECTOR_RESET(ctrl, con, hard); > > - return ucsi_run_command(con->ucsi, &ctrl, NULL, 0); > + return ucsi_send_command(con->ucsi, &ctrl, NULL, 0); > } > > static int ucsi_reset_ppm(struct ucsi *ucsi) @@ -481,15 +687,17 @@ static int > ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control *ctrl) { > int ret; > > - ret = ucsi_run_command(con->ucsi, ctrl, NULL, 0); > + ret = ucsi_send_command(con->ucsi, ctrl, NULL, 0); > if (ret == -ETIMEDOUT) { > struct ucsi_control c; > > /* PPM most likely stopped responding. Resetting everything. */ > + mutex_lock(&con->ucsi->ppm_lock); > ucsi_reset_ppm(con->ucsi); > + mutex_unlock(&con->ucsi->ppm_lock); > > UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL); > - ucsi_run_command(con->ucsi, &c, NULL, 0); > + ucsi_send_command(con->ucsi, &c, NULL, 0); > > ucsi_reset_connector(con, true); > } > @@ -504,10 +712,12 @@ ucsi_dr_swap(const struct typec_capability *cap, > enum typec_data_role role) > struct ucsi_control ctrl; > int ret = 0; > > - if (!con->partner) > - return -ENOTCONN; > + mutex_lock(&con->lock); > > - mutex_lock(&con->ucsi->ppm_lock); > + if (!con->partner) { > + ret = -ENOTCONN; > + goto out_unlock; > + } > > if ((con->status.partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP > && > role == TYPEC_DEVICE) || > @@ -520,18 +730,14 @@ ucsi_dr_swap(const struct typec_capability *cap, > enum typec_data_role role) > if (ret < 0) > goto out_unlock; > > - mutex_unlock(&con->ucsi->ppm_lock); > - > if (!wait_for_completion_timeout(&con->complete, > > msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) > - return -ETIMEDOUT; > - > - return 0; > + ret = -ETIMEDOUT; > > out_unlock: > - mutex_unlock(&con->ucsi->ppm_lock); > + mutex_unlock(&con->lock); > > - return ret; > + return ret < 0 ? ret : 0; > } > > static int > @@ -541,10 +747,12 @@ ucsi_pr_swap(const struct typec_capability *cap, > enum typec_role role) > struct ucsi_control ctrl; > int ret = 0; > > - if (!con->partner) > - return -ENOTCONN; > + mutex_lock(&con->lock); > > - mutex_lock(&con->ucsi->ppm_lock); > + if (!con->partner) { > + ret = -ENOTCONN; > + goto out_unlock; > + } > > if (con->status.pwr_dir == role) > goto out_unlock; > @@ -554,13 +762,11 @@ ucsi_pr_swap(const struct typec_capability *cap, > enum typec_role role) > if (ret < 0) > goto out_unlock; > > - mutex_unlock(&con->ucsi->ppm_lock); > - > if (!wait_for_completion_timeout(&con->complete, > - > msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) > - return -ETIMEDOUT; > - > - mutex_lock(&con->ucsi->ppm_lock); > + msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) { > + ret = -ETIMEDOUT; > + goto out_unlock; > + } > > /* Something has gone wrong while swapping the role */ > if (con->status.pwr_op_mode != UCSI_CONSTAT_PWR_OPMODE_PD) { > @@ -569,7 +775,7 @@ ucsi_pr_swap(const struct typec_capability *cap, enum > typec_role role) > } > > out_unlock: > - mutex_unlock(&con->ucsi->ppm_lock); > + mutex_unlock(&con->lock); > > return ret; > } > @@ -595,6 +801,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) > > INIT_WORK(&con->work, ucsi_connector_change); > init_completion(&con->complete); > + mutex_init(&con->lock); > con->num = index + 1; > con->ucsi = ucsi; > > @@ -636,6 +843,12 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) > if (IS_ERR(con->port)) > return PTR_ERR(con->port); > > + /* Alternate modes */ > + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON); > + if (ret) > + dev_err(ucsi->dev, "con%d: failed to register alt modes\n", > + con->num); > + > /* Get the status */ > UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num); > ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status)); > @@ -662,6 +875,16 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) > if (con->status.connected) > ucsi_register_partner(con); > > + if (con->partner) { > + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP); > + if (ret) > + dev_err(ucsi->dev, > + "con%d: failed to register alternate modes\n", > + con->num); > + else > + ucsi_altmode_update_active(con); > + } > + > trace_ucsi_register_port(con->num, &con->status); > > return 0; > @@ -788,17 +1011,15 @@ void ucsi_unregister_ppm(struct ucsi *ucsi) > /* Make sure that we are not in the middle of driver initialization */ > cancel_work_sync(&ucsi->work); > > - mutex_lock(&ucsi->ppm_lock); > - > /* Disable everything except command complete notification */ > UCSI_CMD_SET_NTFY_ENABLE(ctrl, > UCSI_ENABLE_NTFY_CMD_COMPLETE) > - ucsi_run_command(ucsi, &ctrl, NULL, 0); > - > - mutex_unlock(&ucsi->ppm_lock); > + ucsi_send_command(ucsi, &ctrl, NULL, 0); > > for (i = 0; i < ucsi->cap.num_connectors; i++) { > cancel_work_sync(&ucsi->connector[i].work); > ucsi_unregister_partner(&ucsi->connector[i]); > + ucsi_unregister_altmodes(&ucsi->connector[i], > + UCSI_RECIPIENT_CON); > typec_unregister_port(ucsi->connector[i].port); > } > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index > 53b80f40a908..c416bae4b5ca 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -6,6 +6,7 @@ > #include <linux/bitops.h> > #include <linux/device.h> > #include <linux/types.h> > +#include <linux/usb/typec.h> > > /* -------------------------------------------------------------------------- */ > > @@ -60,6 +61,20 @@ struct ucsi_uor_cmd { > u16:6; /* reserved */ > } __packed; > > +/* Get Alternate Modes Command structure */ struct ucsi_altmode_cmd { > + u8 cmd; > + u8 length; > + u8 recipient; > +#define UCSI_RECIPIENT_CON 0 > +#define UCSI_RECIPIENT_SOP 1 > +#define UCSI_RECIPIENT_SOP_P 2 > +#define UCSI_RECIPIENT_SOP_PP 3 > + u8 con_num; > + u8 offset; > + u8 num_altmodes; > +} __packed; > + > struct ucsi_control { > union { > u64 raw_cmd; > @@ -67,6 +82,7 @@ struct ucsi_control { > struct ucsi_uor_cmd uor; > struct ucsi_ack_cmd ack; > struct ucsi_con_rst con_rst; > + struct ucsi_altmode_cmd alt; > }; > }; > > @@ -112,6 +128,30 @@ struct ucsi_control { > (_ctrl_).cmd.data = _con_; \ > } > > +/* Helper for preparing ucsi_control for GET_ALTERNATE_MODES command. > +*/ #define UCSI_CMD_GET_ALTERNATE_MODES(_ctrl_, _r_, _con_num_, _o_, > _num_)\ > +{ \ > + __UCSI_CMD((_ctrl_), UCSI_GET_ALTERNATE_MODES) > \ > + _ctrl_.alt.recipient = (_r_); \ > + _ctrl_.alt.con_num = (_con_num_); \ > + _ctrl_.alt.offset = (_o_); \ > + _ctrl_.alt.num_altmodes = (_num_) - 1; \ > +} > + > +/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */ > +#define UCSI_CMD_GET_CAM_SUPPORTED(_ctrl_, _con_) > \ > +{ \ > + __UCSI_CMD((_ctrl_), UCSI_GET_CAM_SUPPORTED) > \ > + _ctrl_.cmd.data = (_con_); \ > +} > + > +/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */ > +#define UCSI_CMD_GET_CURRENT_CAM(_ctrl_, _con_) \ > +{ \ > + __UCSI_CMD((_ctrl_), UCSI_GET_CURRENT_CAM) > \ > + _ctrl_.cmd.data = (_con_); \ > +} > + > /* Helper for preparing ucsi_control for GET_CONNECTOR_STATUS command. > */ > #define UCSI_CMD_GET_CONNECTOR_STATUS(_ctrl_, _con_) > \ > { \ > @@ -334,4 +374,36 @@ struct ucsi *ucsi_register_ppm(struct device *dev, > struct ucsi_ppm *ppm); void ucsi_unregister_ppm(struct ucsi *ucsi); void > ucsi_notify(struct ucsi *ucsi); > > +/* > +----------------------------------------------------------------------- > +--- */ > + > +struct ucsi; > + > +#define UCSI_MAX_SVID 5 > +#define UCSI_MAX_ALTMODES (UCSI_MAX_SVID * 6) > + > +struct ucsi_connector { > + int num; > + > + struct ucsi *ucsi; > + struct mutex lock; /* port lock */ > + struct work_struct work; > + struct completion complete; > + > + struct typec_port *port; > + struct typec_partner *partner; > + > + struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES]; > + struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES]; > + > + struct typec_capability typec_cap; > + > + struct ucsi_connector_status status; > + struct ucsi_connector_capability cap; > +}; > + > +int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl, > + void *retval, size_t size); > + > +void ucsi_altmode_update_active(struct ucsi_connector *con); > + > #endif /* __DRIVER_USB_TYPEC_UCSI_H */ > -- > 2.20.1
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Monday, February 4, 2019 4:10 AM > To: Michael Hsu <mhsu@nvidia.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > modes > > Hi, > > On Fri, Feb 01, 2019 at 10:02:19PM +0000, Michael Hsu wrote: > > Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a > > 1-based index, not a 32-bit mode VDO) can cause incorrect matches if > > the GET_ALTERNATE_MODES returns different ordering for > > recipient=connector and recipient=sop for a particular svid. > > > > For example, UCSI command GET_ALTERNATE_MODES with > recipient=connector returns > > [0] SVID=0xff01, ModeVDO=0x00000405 (Mode = 1) > > [1] SVID=0xff01, ModeVDO=0x00000805 (Mode = 2) And UCSI command > > GET_ALTERNATE_MODES with recipient=sop returns > > [0] SVID=0xff01, ModeVDO=0x00000c05 (Mode = 1) > > > > Then when DP alternate mode with pin D is active, GET_CURRENT_CAM > > returns index 1 (connector alternate mode = 1, i.e. SVID=0xff01, > > Mode=2, ModeVDO=0x00000805). > > But, the function will be unable to match it with partner_alt_mode > > corresponding to (SVID=0xff01,Mode=1). > > > > Can you compare against 32-bit VDO instead of ->mode? Also, use '&' > > bitwise AND operator when masking the partner's mode VDO (0x0c05) > > against the connector's mode VDO (0x405 or 0x805) to determine it > > there is an alternate mode match. > > No top-posting! From now on inline your comments in your replies. The > "process" guide in kernel should provide more information for you: > https://www.kernel.org/doc/html/latest/process/2.Process.html#mailing-lists > OK, got it... answering inline. > Your PPM is reporting a separate mode for every pin-assignment it supports. It > really should _not_ do that! You need to be able to get the capabilities for DP > alt mode with GET_ALTERNATE_MODE command in the format that the DP alt > mode spec defines, also with the connector. > You are not getting them like that. Now for example in both modes your > connector can only act as DisplayPort sink (i.e. a display) which I'm pretty sure > is not the case. > > With the current version of the DP alt mode spec we do not ever need more > than one mode for DP. That mode needs to show all the supported pin > assignments the device, partner or connector, supports. That is exactly how all > the other UCSI PPMs work currently. For example the PPM on Dell XPS 13 9380 > that I use for testing gives only one mode for the connector with SVID ff01. The > mode VDO (or MID in UCSI lingo) has the value 0x1c46: > > SVID=0xff01, VDO=0x1c46 > > From that you can clearly see that the connector can only act as the DisplayPort > source, and that it support pin assignments C, D and E. > > So in practice you have a firmware bug. I understand why the PPM was made > that way. That "hack" allows the PPM to workaround a limitation in UCSI where > we in practice can't tell which configuration is in use at any given time. > Unfortunately it can not be done like that. It leaves us with custom VDO values > that we would have to be interpreted separately, that only your PPM supports. > I still think it complies with the letter and spirit of the UCSI spec... There was nothing in the document to suggest that this method of implementing alt modes (i.e., assigning one connector alt mode index per DP pin configuration) is not allowed. Actually, there's also a (great) benefit with this method, the UCSI GET_SUPPORT_CAM can now be used to determine which DP pin assignments are allowed - dynamically. For example, sometimes due to another (non-DisplayPort) alt mode being active, the pins used for DP pin C might not be available, but DP pin D is still allowed. So, a call to UCSI GET_SUPPORTED_CAM would indicate that the pin C DP alt mode was not supported, but the pin D DP alt mode was currently allowed. The drawback which you mentioned "custom VDO values" is not accurate. The GET_ALTERNATE_MODES commands still returns DP-compliant mode VDOs - i.e., bits 15-8 still indicate the DP pin configurations allowed when entering that DP-compatible alternate mode. My original suggestion was to use a bitwise AND mask to make the correlation between the connector's alt mode and the partner's alt mode. This has the benefit of (1) working with PPMs which report a single connector alternate mode for all the DP pin assignments (2) working with PPMs which report separate connector alternate modes for each DP pin assignment If we use your original patch which does a simple mode index equality comparison (instead of a bitwise AND mask), it cannot handle case (2) above. > Please see if you can get the firmware (UCSI PPM) fixed. It really needs to > expose only one mode for DisplayPort alt mode in the connector, and the VDO > in it should be in the same form that it could be send to the partner, i.e. giving > the actual DisplayPort capabilities for the connector. > There is infrastructure build on top of this capability of being able to determine the DP pin assignment support using just the connector alt mode (CAM) index. So, removing the "distinct alt mode index" implementation is not feasible. But, as mentioned above, we believe a minor change (replacing == comparison of the mode index, with a "&" masking of the mode VDO) would support all cases. > We should not do anything before you ask them to fix the PPM. > Experience tells that if we workaround an issue in firmware, the firmware will > never ever get fixed. And in this case the workaround may not be as simple as > one would think. Even if we have to workaround this, it needs a separate patch > with a good explanation. And it probable does not belong to this series. Of the available choices, in terms of number of lines of C code to change + engineering resources, choice (b) is more straightforward... (a) Change the PPM firmware: do not report multiple connector alt modes per DisplayPort pin assignment - significant rewrite of firmware alt mode code - requires QA requalification of PPM with other display monitors - changing internal tools to avoid querying alt mode pin configurations (or possibly require non-UCSI interface to be created to replace removed feature) (b) Change Linux driver's alt mode comparison function Given that the proposed patch modification is a few lines of code and increases number of PPMs supported, can we re-consider it for this series of patches? > > > Thanks, > > -- > heikki Thanks, Michael nvpublic
Hi Michael, On Tue, Feb 05, 2019 at 02:20:34AM +0000, Michael Hsu wrote: > > Your PPM is reporting a separate mode for every pin-assignment it supports. It > > really should _not_ do that! You need to be able to get the capabilities for DP > > alt mode with GET_ALTERNATE_MODE command in the format that the DP alt > > mode spec defines, also with the connector. > > You are not getting them like that. Now for example in both modes your > > connector can only act as DisplayPort sink (i.e. a display) which I'm pretty sure > > is not the case. > > > > With the current version of the DP alt mode spec we do not ever need more > > than one mode for DP. That mode needs to show all the supported pin > > assignments the device, partner or connector, supports. That is exactly how all > > the other UCSI PPMs work currently. For example the PPM on Dell XPS 13 9380 > > that I use for testing gives only one mode for the connector with SVID ff01. The > > mode VDO (or MID in UCSI lingo) has the value 0x1c46: > > > > SVID=0xff01, VDO=0x1c46 > > > > From that you can clearly see that the connector can only act as the DisplayPort > > source, and that it support pin assignments C, D and E. > > > > So in practice you have a firmware bug. I understand why the PPM was made > > that way. That "hack" allows the PPM to workaround a limitation in UCSI where > > we in practice can't tell which configuration is in use at any given time. > > Unfortunately it can not be done like that. It leaves us with custom VDO values > > that we would have to be interpreted separately, that only your PPM supports. > > > > I still think it complies with the letter and spirit of the UCSI spec... > There was nothing in the document to suggest that this method of implementing > alt modes (i.e., assigning one connector alt mode index per DP pin > configuration) is not allowed. The spec does not say anything about how that MID field should be interpreted, and that is the problem here. The core problem here is the UCSI spec. It really needs a new version that considers alternate modes a bit better. > Actually, there's also a (great) benefit with this method, the UCSI > GET_SUPPORT_CAM can now be used to determine which DP pin assignments are > allowed - dynamically. For example, sometimes due to another > (non-DisplayPort) alt mode being active, the pins used for DP pin C might not > be available, but DP pin D is still allowed. So, a call to UCSI > GET_SUPPORTED_CAM would indicate that the pin C DP alt mode was not supported, > but the pin D DP alt mode was currently allowed. > > The drawback which you mentioned "custom VDO values" is not accurate. The > GET_ALTERNATE_MODES commands still returns DP-compliant mode VDOs - i.e., bits > 15-8 still indicate the DP pin configurations allowed when entering that > DP-compatible alternate mode. > > My original suggestion was to use a bitwise AND mask to make the correlation > between the connector's alt mode and the partner's alt mode. This has the > benefit of > (1) working with PPMs which report a single connector alternate mode for all > the DP pin assignments > (2) working with PPMs which report separate connector alternate modes for each > DP pin assignment > > If we use your original patch which does a simple mode index equality > comparison (instead of a bitwise AND mask), it cannot handle case (2) above. I don't think you can use the VDO value as a mask even with DP alt mode (let alone with certain other alt modes) because you need to be able to match bits 15:8 to bits 23:16 and vise versa in some cases. The matching needs to be alt mode specific in any case and I think that partially renders the "masks" useless. Regardless of that, you are missing the point here. The problem is that your PPM is exposing bit masks in those MID fields, while others are exposing the connector DP capabilities. There is a difference in the behaviour depending on PPM implementation. That's not a small problem. Perhaps equally importantly, you are using two different kinds of values in that same data structure depending on the request - with partners the data structure gives you the capabilities VDO, but with connectors you get something else - i.e. the returned data structure is in practice not always the same. You can't just change the data structure like that just when it's convenient for you. You only do something like that if all the alternative data structures are documented in some standard. That's not the case here. You have also made that hack just for the sake of one limitation in UCSI. By corrupting the data structure in connector case you will only know the pin assignment, but that you can in practice guess in case of DP, and we can even figure it out for certainty if needed by communicating with GPU drivers from the UCSI DP driver. Much bigger limitation in UCSI is that there is no way to see the Attention VDM payloads, let alone get an event when the partner send the Attention VDM. In case of DP, we can't see the DP Status VDO. On some platforms we could have really used the DP Status and get the Attention events because of HPD. > > Please see if you can get the firmware (UCSI PPM) fixed. It really needs to > > expose only one mode for DisplayPort alt mode in the connector, and the VDO > > in it should be in the same form that it could be send to the partner, i.e. giving > > the actual DisplayPort capabilities for the connector. > > > > There is infrastructure build on top of this capability of being able to > determine the DP pin assignment support using just the connector alt mode > (CAM) index. So, removing the "distinct alt mode index" implementation is not > feasible. > > But, as mentioned above, we believe a minor change (replacing == comparison of > the mode index, with a "&" masking of the mode VDO) would support all cases. > > > We should not do anything before you ask them to fix the PPM. > > Experience tells that if we workaround an issue in firmware, the firmware will > > never ever get fixed. And in this case the workaround may not be as simple as > > one would think. Even if we have to workaround this, it needs a separate patch > > with a good explanation. And it probable does not belong to this series. > > Of the available choices, in terms of number of lines of C code to change + > engineering resources, choice (b) is more straightforward... > (a) Change the PPM firmware: do not report multiple connector alt modes per > DisplayPort pin assignment > - significant rewrite of firmware alt mode code If that is the case, then the UCSI code has been mixed in with the state machine code, and there is no real structure in the firmware code. I don't believe that that is the case. I'm pretty sure that changing the values that the PPM exposes to the OPM in the UCSI data structures requires the minimum amount of effort. > - requires QA requalification of PPM with other display monitors Now why would that be required? The actual SVID specific communication with the partners has not changed. There should be absolutely no changes to the state machines, even the policies will stay the same. The OPM interface simply can't be critical. Let's not forget: "The PPM is expected to function without any OS interaction" > - changing internal tools to avoid querying alt mode pin configurations > (or possibly require non-UCSI interface to be created to replace removed > feature) That sounds to me like your tools are not using the ABI defined in Linux kernel: Documentation/ABI/testing/sysfs-driver-typec-displayport I think you need to update your tools regardless how this problem is solved. > (b) Change Linux driver's alt mode comparison function > > Given that the proposed patch modification is a few lines of code and > increases number of PPMs supported, can we re-consider it for this series of > patches? We have to make sure all the options have been considered before accepting a workaround like that. Once it's part of mainline, you may be off the hook, but me and the community are not. Even if there's no way to get rid of that mask hack, it has to be handled differently (in the UCSI DP driver) in any case. If we really have to accept it, then I do want a separate patch for it with a good explanation why it's needed in the commit message. Before we do anything, let's just look at our options: If you need certainty regarding the selected pin-assignment, then I would propose initially that we communicate with the GPU/DP driver from UCSI DP driver. Those drivers can tell us how many DP lanes are in use, and that should be enough to tell are we using C or D. The HPD problem I mentioned above was solved by supplying the operating system a custom event mimicking HPD IRQ. The generic UCSI code was not affected by that at all. How about if you do something similar? You would just need to be able to see the content of the Configuration VDO that the PPM used with the partner in your driver (ucsi_ccg,c). Is that an option? Note, we will in all these cases need to tweak the UCSI drivers a little, but that's not the issue. thanks,
Hi Ajay, On Tue, Feb 05, 2019 at 12:59:27AM +0000, Ajay Gupta wrote: > > +static int ucsi_register_altmode(struct ucsi_connector *con, > > + struct typec_altmode_desc *desc, > > + u8 recipient) > > +{ > > + struct typec_altmode *alt; > > + int ret; > > + int i; > > + > > + switch (recipient) { > > + case UCSI_RECIPIENT_CON: > > + i = ucsi_next_altmode(con->port_altmode); > > + if (i < 0) { > > + ret = i; > > + goto err; > > + } > > + > > + desc->mode = ucsi_altmode_next_mode(con->port_altmode, > > + desc->svid); > > + > > + alt = typec_port_register_altmode(con->port, desc); > > + if (IS_ERR(alt)) { > > + ret = PTR_ERR(alt); > > + goto err; > > + } > > + > > + con->port_altmode[i] = alt; > > + break; > > + case UCSI_RECIPIENT_SOP: > > + i = ucsi_next_altmode(con->partner_altmode); > We are seeing duplicate partner altmode devices getting created when we set > "active" file from 1->0->1 Please add a check here to see if altmode device > already exists. > > [...] > case UCSI_RECIPIENT_SOP: > /* check to see if partner altmode already exists */ > if (ucsi_altmode_found(con->partner_altmode, desc)) > break; > > i = ucsi_next_altmode(con->partner_altmode); > if (i < 0) { > [...] > > > static bool ucsi_altmode_found(struct typec_altmode **alt, > struct typec_altmode_desc *desc) > { > int i; > > for (i = 0; i < UCSI_MAX_ALTMODES; i++) { > if (!alt[i]) > return false; > if (alt[i]->svid == desc->svid && alt[i]->vdo == desc->vdo) > return true; > } > > return false; > } OK. I'll prepare new version later this week. thanks,
Hi Michael, On Fri, Feb 01, 2019 at 10:02:19PM +0000, Michael Hsu wrote: > Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a 1-based > index, not a 32-bit mode VDO) can cause incorrect matches if the > GET_ALTERNATE_MODES returns different ordering for recipient=connector and > recipient=sop for a particular svid. > > For example, UCSI command GET_ALTERNATE_MODES with recipient=connector returns > [0] SVID=0xff01, ModeVDO=0x00000405 (Mode = 1) > [1] SVID=0xff01, ModeVDO=0x00000805 (Mode = 2) > And UCSI command GET_ALTERNATE_MODES with recipient=sop returns > [0] SVID=0xff01, ModeVDO=0x00000c05 (Mode = 1) > > Then when DP alternate mode with pin D is active, GET_CURRENT_CAM returns > index 1 (connector alternate mode = 1, i.e. SVID=0xff01, Mode=2, > ModeVDO=0x00000805). > But, the function will be unable to match it with partner_alt_mode > corresponding to (SVID=0xff01,Mode=1). > > Can you compare against 32-bit VDO instead of ->mode? Also, use '&' bitwise > AND operator when masking the partner's mode VDO (0x0c05) against the > connector's mode VDO (0x405 or 0x805) to determine it there is an alternate > mode match. FYI, I'll improve the matching done ucsi_altmode_update_active(), it is true that we cannot rely on the the mode index, but note I'm not going to support those masks at this point. I'm still expecting that the firmware can be updated. The UCSI data structures really need to supply consistent information to the OS, regardless of the PPM implementation. I threw some ideas how we should be able to determine the DP pin assignment with more certainty in my previous mail, in case the currently executed "guessing" is not reliable enough for you guys. Let me know what you think. thanks,
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Tuesday, February 5, 2019 7:24 AM > To: Michael Hsu <mhsu@nvidia.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > modes > > Hi Michael, > > On Tue, Feb 05, 2019 at 02:20:34AM +0000, Michael Hsu wrote: > > > Your PPM is reporting a separate mode for every pin-assignment it > > > supports. It really should _not_ do that! You need to be able to get > > > the capabilities for DP alt mode with GET_ALTERNATE_MODE command in > > > the format that the DP alt mode spec defines, also with the connector. > > > You are not getting them like that. Now for example in both modes > > > your connector can only act as DisplayPort sink (i.e. a display) > > > which I'm pretty sure is not the case. > > > > > > With the current version of the DP alt mode spec we do not ever need > > > more than one mode for DP. That mode needs to show all the supported > > > pin assignments the device, partner or connector, supports. That is > > > exactly how all the other UCSI PPMs work currently. For example the > > > PPM on Dell XPS 13 9380 that I use for testing gives only one mode > > > for the connector with SVID ff01. The mode VDO (or MID in UCSI lingo) has > the value 0x1c46: > > > > > > SVID=0xff01, VDO=0x1c46 > > > > > > From that you can clearly see that the connector can only act as the > > > DisplayPort source, and that it support pin assignments C, D and E. > > > > > > So in practice you have a firmware bug. I understand why the PPM was > > > made that way. That "hack" allows the PPM to workaround a limitation > > > in UCSI where we in practice can't tell which configuration is in use at any > given time. > > > Unfortunately it can not be done like that. It leaves us with custom > > > VDO values that we would have to be interpreted separately, that only your > PPM supports. > > > > > > > I still think it complies with the letter and spirit of the UCSI spec... > > There was nothing in the document to suggest that this method of > > implementing alt modes (i.e., assigning one connector alt mode index > > per DP pin > > configuration) is not allowed. > > The spec does not say anything about how that MID field should be interpreted, > and that is the problem here. > > The core problem here is the UCSI spec. It really needs a new version that > considers alternate modes a bit better. > Agreed. UCSI spec does not specify how the connector alt mode table (returned by UCSI GET_ALTERNATE_MODES cmd with recipient==connector) can be matched with the partner alt mode table (returned by UCSI GET_ALTERNATE_MODES cmd with recipient==SOP). Your existing patch matches the SVID and then requires one-for-one ordering in case either alt mode table has multiple mode entries for a particular SVID. How about this solution... given that different ucsi ppms are allowed by ambiguous spec to have different interpretations of the connector alt mode tables' ModeID values, there should be an entry in struct ucsi_ppm { ... + int (*match_altmode_idx)(struct ucsi_ppm *, int partner_svid, int partner_mode_vdo); /* returns connector alt mode index */ }; This structure makes sense to have this function added, since in addition to the existing device-specific cmd() / sync() functions, this additional function of alt mode index matching also varies between devices. > > Actually, there's also a (great) benefit with this method, the UCSI > > GET_SUPPORT_CAM can now be used to determine which DP pin > assignments > > are allowed - dynamically. For example, sometimes due to another > > (non-DisplayPort) alt mode being active, the pins used for DP pin C > > might not be available, but DP pin D is still allowed. So, a call to > > UCSI GET_SUPPORTED_CAM would indicate that the pin C DP alt mode was > > not supported, but the pin D DP alt mode was currently allowed. > > > > The drawback which you mentioned "custom VDO values" is not accurate. > > The GET_ALTERNATE_MODES commands still returns DP-compliant mode > VDOs > > - i.e., bits > > 15-8 still indicate the DP pin configurations allowed when entering > > that DP-compatible alternate mode. > > > > My original suggestion was to use a bitwise AND mask to make the > > correlation between the connector's alt mode and the partner's alt > > mode. This has the benefit of > > (1) working with PPMs which report a single connector alternate mode > > for all the DP pin assignments > > (2) working with PPMs which report separate connector alternate modes > > for each DP pin assignment > > > > If we use your original patch which does a simple mode index equality > > comparison (instead of a bitwise AND mask), it cannot handle case (2) above. > > I don't think you can use the VDO value as a mask even with DP alt mode (let > alone with certain other alt modes) because you need to be able to match bits > 15:8 to bits 23:16 and vise versa in some cases. > The matching needs to be alt mode specific in any case and I think that partially > renders the "masks" useless. > > Regardless of that, you are missing the point here. The problem is that your > PPM is exposing bit masks in those MID fields, while others are exposing the > connector DP capabilities. There is a difference in the behaviour depending on > PPM implementation. That's not a small problem. > > Perhaps equally importantly, you are using two different kinds of values in that > same data structure depending on the request - with partners the data > structure gives you the capabilities VDO, but with connectors you get > something else - i.e. the returned data structure is in practice not always the > same. You can't just change the data structure like that just when it's > convenient for you. You only do something like that if all the alternative data > structures are documented in some standard. That's not the case here. > > You have also made that hack just for the sake of one limitation in UCSI. By > corrupting the data structure in connector case you will only know the pin > assignment, but that you can in practice guess in case of DP, and we can even > figure it out for certainty if needed by communicating with GPU drivers from > the UCSI DP driver. Much bigger limitation in UCSI is that there is no way to see > the Attention VDM payloads, let alone get an event when the partner send the > Attention VDM. In case of DP, we can't see the DP Status VDO. On some > platforms we could have really used the DP Status and get the Attention events > because of HPD. > > > > Please see if you can get the firmware (UCSI PPM) fixed. It really > > > needs to expose only one mode for DisplayPort alt mode in the > > > connector, and the VDO in it should be in the same form that it > > > could be send to the partner, i.e. giving the actual DisplayPort capabilities > for the connector. > > > > > > > There is infrastructure build on top of this capability of being able > > to determine the DP pin assignment support using just the connector > > alt mode > > (CAM) index. So, removing the "distinct alt mode index" > > implementation is not feasible. > > > > But, as mentioned above, we believe a minor change (replacing == > > comparison of the mode index, with a "&" masking of the mode VDO) would > support all cases. > > > > > We should not do anything before you ask them to fix the PPM. > > > Experience tells that if we workaround an issue in firmware, the > > > firmware will never ever get fixed. And in this case the workaround > > > may not be as simple as one would think. Even if we have to > > > workaround this, it needs a separate patch with a good explanation. And it > probable does not belong to this series. > > > > Of the available choices, in terms of number of lines of C code to > > change + engineering resources, choice (b) is more straightforward... > > (a) Change the PPM firmware: do not report multiple connector alt > > modes per DisplayPort pin assignment > > - significant rewrite of firmware alt mode code > > If that is the case, then the UCSI code has been mixed in with the state machine > code, and there is no real structure in the firmware code. I don't believe that > that is the case. I'm pretty sure that changing the values that the PPM exposes > to the OPM in the UCSI data structures requires the minimum amount of effort. > > > - requires QA requalification of PPM with other display monitors > > Now why would that be required? The actual SVID specific communication with > the partners has not changed. There should be absolutely no changes to the > state machines, even the policies will stay the same. > The OPM interface simply can't be critical. Let's not forget: > > "The PPM is expected to function without any OS interaction" > > > - changing internal tools to avoid querying alt mode pin configurations > > (or possibly require non-UCSI interface to be created to replace removed > > feature) > > That sounds to me like your tools are not using the ABI defined in Linux kernel: > > Documentation/ABI/testing/sysfs-driver-typec-displayport > Actually, our tools predated the adoption of this ABI. The previous sysfs for typec had generic (protocol independent) "active" nodes which directly matched the UCSI CAM index. > I think you need to update your tools regardless how this problem is solved. > > > (b) Change Linux driver's alt mode comparison function > > > > Given that the proposed patch modification is a few lines of code and > > increases number of PPMs supported, can we re-consider it for this > > series of patches? > > We have to make sure all the options have been considered before accepting a > workaround like that. Once it's part of mainline, you may be off the hook, but > me and the community are not. > > Even if there's no way to get rid of that mask hack, it has to be handled > differently (in the UCSI DP driver) in any case. If we really have to accept it, > then I do want a separate patch for it with a good explanation why it's needed > in the commit message. > > Before we do anything, let's just look at our options: > OK, as mentioned above, we don't need the mask hack if the ucsi_ppm is allowed to supply a device-specifc function for alt mode index matching between the connector alt mode table and the partner alt mode table. Another way to summarize the issue: - UCSI spec requires connector alt mode index to select /deselect alt mode - Linux displayport ABI exposes partner alt mode sysfs nodes and needs to translate partner alt mode index to UCSI's connector alt mode index > If you need certainty regarding the selected pin-assignment, then I would > propose initially that we communicate with the GPU/DP driver from UCSI DP > driver. Those drivers can tell us how many DP lanes are in use, and that should > be enough to tell are we using C or D. > For our device, we have certainty since the GET_CURRENT_CAM returns a different index depending on which DisplayPort pin assignment is active. But, that's internal to our device and we need not expect other devices to also reveal its current pin assignment. > The HPD problem I mentioned above was solved by supplying the operating > system a custom event mimicking HPD IRQ. The generic UCSI code was not > affected by that at all. How about if you do something similar? > This seems like a side channel communication, and we wanted to not create extra channels - which was why we arranged our UCSI connector alt mode table the way we did. > You would just need to be able to see the content of the Configuration VDO that > the PPM used with the partner in your driver (ucsi_ccg,c). > Is that an option? > > Note, we will in all these cases need to tweak the UCSI drivers a little, but that's > not the issue. > > > thanks, > > -- > heikki Thanks, Michael nvpublic
Hi Michael, On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote: > Your existing patch matches the SVID and then requires one-for-one ordering in > case either alt mode table has multiple mode entries for a particular SVID. And we'll fix that, but that does not solve the issue that I'm talking about. There are two separate issues here. That problem is a bug in the driver, but the major problem still is that you have separate alternate modes for the two DP pin assignments, and fixing the index handling does not help with that. According to the DP alt mode standard you do not have a separate mode for every supported DṔ pin assignment. When you change the DP alt mode pin assignment, the current mode is not exited and a new one entered. You stay in the DisplayPort mode, and simply re-configure using the DisplayPort Configure command. In your implementation however, the mode is existed, and a new one entered. So your implementation is not made according to the DisplayPort Alt Mode standard. I guess I have been able to explain just how big of a problem that is, and also how exactly we will have to handle it... Let's look at this from user space perspective. The user space should not need to be aware of the method used to interface with the USB Type-C connectors on the system. The kernel needs to hide that and supply unified interface to the user space which looks and behaves the same, _always_. In case of the DP alt mode, the user space should be allowed to expect the behaviour to follow the DP alt mode spec, so when the user space selects another pin assignment for the DP alt mode adapter, it should be a pass-fail operation without any side effects, just like explained in the VDM flows in the DP alt mode spec. But now with your PPM, there is a major side effect. Every time the user space selects a new DP pin assignment, the current mode is actually exited and a new mode entered. That is not standard behaviour. The user space will not just ignore the mode being exited. It has to react to it, most likely by assuming there was a fatal error somewhere. So in practice the user space is now forced to handle your connector as a special case. To protect the user space from that, and to keep the interface we provide to it consistent and predictable, we would have to handle your PPM completely separately. The user space will see only one connector DP alt mode no matter what. Even if you are unable change the PPM, the driver will still have to expose the two connector DP alternate modes as one connector alternate mode to the user space. That does not leave much use for the separated modes for the two pin assignments. The only thing they are providing in any case is the guarantee of the initial pin assignment, but as said, that we can guess in any case. Sorry for the long explanation. Br,
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Thursday, February 7, 2019 5:16 AM > To: Michael Hsu <mhsu@nvidia.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > modes > > Hi Michael, > > On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote: > > Your existing patch matches the SVID and then requires one-for-one > > ordering in case either alt mode table has multiple mode entries for a > particular SVID. > > And we'll fix that, but that does not solve the issue that I'm talking about. There > are two separate issues here. That problem is a bug in the driver, but the major > problem still is that you have separate alternate modes for the two DP pin > assignments, and fixing the index handling does not help with that. > > According to the DP alt mode standard you do not have a separate mode for > every supported DṔ pin assignment. When you change the DP alt mode pin > assignment, the current mode is not exited and a new one entered. > You stay in the DisplayPort mode, and simply re-configure using the DisplayPort > Configure command. In your implementation however, the mode is existed, > and a new one entered. So your implementation is not made according to the > DisplayPort Alt Mode standard. > > I guess I have been able to explain just how big of a problem that is, and also > how exactly we will have to handle it... Let's look at this from user space > perspective. > > The user space should not need to be aware of the method used to interface > with the USB Type-C connectors on the system. The kernel needs to hide that > and supply unified interface to the user space which looks and behaves the > same, _always_. > > In case of the DP alt mode, the user space should be allowed to expect the > behaviour to follow the DP alt mode spec, so when the user space selects > another pin assignment for the DP alt mode adapter, it should be a pass-fail > operation without any side effects, just like explained in the VDM flows in the > DP alt mode spec. > > But now with your PPM, there is a major side effect. Every time the user space > selects a new DP pin assignment, the current mode is actually exited and a new > mode entered. That is not standard behaviour. The user space will not just > ignore the mode being exited. Not exactly true... If you look on a USB PD analyzer at the type c connector's CC lines, you would *not* see and "Exit Mode" VDMs - just "DP Configure" VDMs. The "active" sysfs node with the value of "yes" would change paths (since another UCSI CAM index is active), but that's because there was a UCSI connector status change. > It has to react to it, most likely by assuming there was a fatal error somewhere. > So in practice the user space is now forced to handle your connector as a > special case. The expected user behavior is to read every sysfs node of the form... /sys/class/typec/.../svid /sys/class/typec/.../vdo And if it decides that's the alt mode it wants to activate, it writes 1 to /sys/class/typec/.../active And optionally, reads above sysfs file to confirm that it did indeed change to 'yes'. It would treat it as an failure to enter alt mode if it was read as still 'no'. The unexpected behavior which you are talking about is if the user uses the displayport pin assignment sysfs node to switch from pin C to pin D. Then, a previously active mode would turn to 'no' and another active sysfs node would change to 'yes'. But, user-mode application has to be able to expect this anyways. (This = some active sysfs node changing even though the application did not cause it.) For example, a TypeC-to-DP dongle is attached. This dongle is also a hub so a downstream USB superspeed device can be connected to it. When there is no downstream USB device attached, DisplayPort Pin C (4 lanes of video) are supported. But, user plugs in a USB device to the dongle - then the dongle will switch from pin C to pin D automatically to allow the USB superspeed lanes to be connected. In this case, I'd expect your displayport ABI to reflect the automatic pin assignment change from C to D caused not by any sysfs activity, but simply plugging in USB device to the typec alt mode adapter. In our particular case, in addition to the "pin" sysfs node changing from "[C] D E" to "C [D] E", the "active" sysfs node with the "yes" value would change from one path to another. So, applications must expect values of sysfs nodes under /sys/class/typec to change by itself. > > To protect the user space from that, and to keep the interface we provide to it > consistent and predictable, we would have to handle your PPM completely > separately. The user space will see only one connector DP alt mode no matter > what. Even if you are unable change the PPM, the driver will still have to > expose the two connector DP alternate modes as one connector alternate > mode to the user space. If this is acceptable to you, we can prepare a patch to translate the PPM's returned alt mode list (which contains different CAM index for each DP pin assignment) into a compressed table. We would also need to do the reverse transformation. Method (1). In ucsi_ppm structure... struct ucsi_ppm { + int (*canonize_alt_mode_table)(struct typec_altmode *altmodes); /* compresses multiple DP alt modes into single alt mode */ + int (*get_ppm_cam)(struct typec_altmode *altmodes, int cam, unsigned long am_specific); }; Method (2). Hook directly into ucsi_ccg.c's command / response handler. When there is a response from ppm to GET_ALTERNATE_MODES (recipient = connector), it substitutes the return value with the single SVID/ModeID entry. When it gets a UCSI SET_CURRENT_CAM command, it will translate the index value from the ucsi/displayport.c's index into the ppm's value. This isolates changes within ucsi_ccg.c and does not require addition of function callbacks to the ucsi_ppm structure. > > That does not leave much use for the separated modes for the two pin > assignments. The only thing they are providing in any case is the guarantee of > the initial pin assignment, but as said, that we can guess in any case. > > Sorry for the long explanation. > > > Br, > > -- > heikki Thanks, Michael nvpublic
Hi Michael, On Thu, Feb 07, 2019 at 10:01:15PM +0000, Michael Hsu wrote: > Hi Heikki, > > > -----Original Message----- > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Sent: Thursday, February 7, 2019 5:16 AM > > To: Michael Hsu <mhsu@nvidia.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > > modes > > > > Hi Michael, > > > > On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote: > > > Your existing patch matches the SVID and then requires one-for-one > > > ordering in case either alt mode table has multiple mode entries for a > > particular SVID. > > > > And we'll fix that, but that does not solve the issue that I'm talking about. There > > are two separate issues here. That problem is a bug in the driver, but the major > > problem still is that you have separate alternate modes for the two DP pin > > assignments, and fixing the index handling does not help with that. > > > > According to the DP alt mode standard you do not have a separate mode for > > every supported DṔ pin assignment. When you change the DP alt mode pin > > assignment, the current mode is not exited and a new one entered. > > You stay in the DisplayPort mode, and simply re-configure using the DisplayPort > > Configure command. In your implementation however, the mode is existed, > > and a new one entered. So your implementation is not made according to the > > DisplayPort Alt Mode standard. > > > > I guess I have been able to explain just how big of a problem that is, and also > > how exactly we will have to handle it... Let's look at this from user space > > perspective. > > > > The user space should not need to be aware of the method used to interface > > with the USB Type-C connectors on the system. The kernel needs to hide that > > and supply unified interface to the user space which looks and behaves the > > same, _always_. > > > > In case of the DP alt mode, the user space should be allowed to expect the > > behaviour to follow the DP alt mode spec, so when the user space selects > > another pin assignment for the DP alt mode adapter, it should be a pass-fail > > operation without any side effects, just like explained in the VDM flows in the > > DP alt mode spec. > > > > But now with your PPM, there is a major side effect. Every time the user space > > selects a new DP pin assignment, the current mode is actually exited and a new > > mode entered. That is not standard behaviour. The user space will not just > > ignore the mode being exited. > > Not exactly true... If you look on a USB PD analyzer at the type c connector's > CC lines, you would *not* see and "Exit Mode" VDMs - just "DP Configure" VDMs. So your PPM does not actually reflect the behaviour of the hardware and firmware. You are not actually changing the mode when you change the pin assignment, yet you still want the operating system to think it has to change the mode every time it changes the pin assignment. That means the two modes you have are for completely customised software (firmware) representation of the DisplayPort alt mode support that does not follow the standard, and as we can now see, reflect even the actual behaviour. If your connector could act as the sink, how many DP alt modes would return to the partner as a response to Discover Modes with DP SVID? You would return only one regardless of how many pin assignments you support, right? That's what you need to return to the OS as well, as source or sink. > The "active" sysfs node with the value of "yes" would change paths (since > another UCSI CAM index is active), but that's because there was a UCSI > connector status change. > > > It has to react to it, most likely by assuming there was a fatal error somewhere. > > So in practice the user space is now forced to handle your connector as a > > special case. > > The expected user behavior is to read every sysfs node of the form... > /sys/class/typec/.../svid > /sys/class/typec/.../vdo > And if it decides that's the alt mode it wants to activate, it writes 1 to > /sys/class/typec/.../active > And optionally, reads above sysfs file to confirm that it did indeed change to > 'yes'. > It would treat it as an failure to enter alt mode if it was read as still > 'no'. Do you understand that those steps you can take _only_ on your hardware in order to change the DP pin assignment? They work only with your PPM. The approach is completely custom. The other UCSI PPMs handle this part correctly. There is always only one DisplayPort alt mode for the connector, regardless of how many DP pin assignment the connector support. Your PPM is the only exception. > The unexpected behavior which you are talking about is if the user uses the > displayport pin assignment sysfs node to switch from pin C to pin D. Note that the sysfs attribute file for the DP pin assignment is the _standard_ interface. That sysfs file is the interface the user space will use for changing the DisplayPort pin assignment. > Then, a reviously active mode would turn to 'no' and another active > sysfs node would change to 'yes'. > > But, user-mode application has to be able to expect this anyways. (This = > some active sysfs node changing even though the application did not cause it.) > For example, a TypeC-to-DP dongle is attached. This dongle is also a hub so a > downstream USB superspeed device can be connected to it. When there is no > downstream USB device attached, DisplayPort Pin C (4 lanes of video) are > supported. But, user plugs in a USB device to the dongle - then the dongle > will switch from pin C to pin D automatically to allow the USB superspeed > lanes to be connected. In this case, I'd expect your displayport ABI to > reflect the automatic pin assignment change from C to D caused not by any > sysfs activity, but simply plugging in USB device to the typec alt mode > adapter. In our particular case, in addition to the "pin" sysfs node changing > from "[C] D E" to "C [D] E", the "active" sysfs node with the "yes" value > would change from one path to another. So, applications must expect values of > sysfs nodes under /sys/class/typec to change by itself. We are not talking about just some sysfs files here. Every alternate mode will have a kernel object representing them. When a change happens on those objects, for example if a mode is entered or exited (regardless of was it as a result of user action or not), or the DP pin assignment gets changed, the kernel generates events for those objects. Those events are the primary source of initial information in user space. The problem is that changing the DP pin assignment has different meaning than exiting and entering the modes. Again, changing the DP pin assignment does not cause the mode to be exited and another to be entered. That is how the standard defines it (and that is the reason why we have the separate sysfs attribute file for the DP pin assignment in the first place), and that's how the user space will expect the interface to behave. With your PPM however the pin assignment is completely coupled with the connector mode. If the pin assignment is changed, the mode gets changed, and if the mode is changed, the pin assignment gets changed. That is simply not the standard behaviour. > > To protect the user space from that, and to keep the interface we provide to it > > consistent and predictable, we would have to handle your PPM completely > > separately. The user space will see only one connector DP alt mode no matter > > what. Even if you are unable change the PPM, the driver will still have to > > expose the two connector DP alternate modes as one connector alternate > > mode to the user space. > > If this is acceptable to you, It really isn't, but it will be something that we have to do unless you guys can fix your firmware. This will be a large quirk that we would have to implement and then maintain, just to workaround yet another firmware issue. Kernel is already full of workarounds like that, so I'm begging you, please get the firmware fixed. Surely you now see that it does not exactly comply with the DP alt mode standard. Why would you need the two DP alt modes for the connector? Br,
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Friday, February 8, 2019 5:48 AM > To: Michael Hsu <mhsu@nvidia.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > modes > > Hi Michael, > > On Thu, Feb 07, 2019 at 10:01:15PM +0000, Michael Hsu wrote: > > Hi Heikki, > > > > > -----Original Message----- > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Sent: Thursday, February 7, 2019 5:16 AM > > > To: Michael Hsu <mhsu@nvidia.com> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > > > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for > > > alternate modes > > > > > > Hi Michael, > > > > > > On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote: > > > > Your existing patch matches the SVID and then requires one-for-one > > > > ordering in case either alt mode table has multiple mode entries > > > > for a > > > particular SVID. > > > > > > And we'll fix that, but that does not solve the issue that I'm > > > talking about. There are two separate issues here. That problem is a > > > bug in the driver, but the major problem still is that you have > > > separate alternate modes for the two DP pin assignments, and fixing the > index handling does not help with that. > > > > > > According to the DP alt mode standard you do not have a separate > > > mode for every supported DṔ pin assignment. When you change the DP > > > alt mode pin assignment, the current mode is not exited and a new one > entered. > > > You stay in the DisplayPort mode, and simply re-configure using the > > > DisplayPort Configure command. In your implementation however, the > > > mode is existed, and a new one entered. So your implementation is > > > not made according to the DisplayPort Alt Mode standard. > > > > > > I guess I have been able to explain just how big of a problem that > > > is, and also how exactly we will have to handle it... Let's look at > > > this from user space perspective. > > > > > > The user space should not need to be aware of the method used to > > > interface with the USB Type-C connectors on the system. The kernel > > > needs to hide that and supply unified interface to the user space > > > which looks and behaves the same, _always_. > > > > > > In case of the DP alt mode, the user space should be allowed to > > > expect the behaviour to follow the DP alt mode spec, so when the > > > user space selects another pin assignment for the DP alt mode > > > adapter, it should be a pass-fail operation without any side > > > effects, just like explained in the VDM flows in the DP alt mode spec. > > > > > > But now with your PPM, there is a major side effect. Every time the > > > user space selects a new DP pin assignment, the current mode is > > > actually exited and a new mode entered. That is not standard > > > behaviour. The user space will not just ignore the mode being exited. > > > > Not exactly true... If you look on a USB PD analyzer at the type c > > connector's CC lines, you would *not* see and "Exit Mode" VDMs - just "DP > Configure" VDMs. > > So your PPM does not actually reflect the behaviour of the hardware and > firmware. You are not actually changing the mode when you change the pin > assignment, yet you still want the operating system to think it has to change the > mode every time it changes the pin assignment. > > That means the two modes you have are for completely customised software > (firmware) representation of the DisplayPort alt mode support that does not > follow the standard, and as we can now see, reflect even the actual behaviour. > > If your connector could act as the sink, how many DP alt modes would return to > the partner as a response to Discover Modes with DP SVID? > You would return only one regardless of how many pin assignments you > support, right? That's what you need to return to the OS as well, as source or > sink. > > > The "active" sysfs node with the value of "yes" would change paths > > (since another UCSI CAM index is active), but that's because there was > > a UCSI connector status change. > > > > > It has to react to it, most likely by assuming there was a fatal error > somewhere. > > > So in practice the user space is now forced to handle your connector > > > as a special case. > > > > The expected user behavior is to read every sysfs node of the form... > > /sys/class/typec/.../svid > > /sys/class/typec/.../vdo > > And if it decides that's the alt mode it wants to activate, it writes 1 to > > /sys/class/typec/.../active > > And optionally, reads above sysfs file to confirm that it did indeed > > change to 'yes'. > > It would treat it as an failure to enter alt mode if it was read as > > still 'no'. > > Do you understand that those steps you can take _only_ on your hardware in > order to change the DP pin assignment? They work only with your PPM. The > approach is completely custom. > I disagree, most user-mode applications would read the contents of the 'svid' and 'vdo' sysfs nodes and then decide if they want to enter that alternate mode - by writing to the corresponding 'active' node. Question: If the application does not examine both the 'svid' and 'vdo' nodes, what other node do they read before deciding whether to enter that alternate mode or not? Question2: Why even report this 'svid' and 'vdo' values if it was not the intent of the type c driver design to allow applications to inspect them and act on them? > The other UCSI PPMs handle this part correctly. There is always only one > DisplayPort alt mode for the connector, regardless of how many DP pin > assignment the connector support. Your PPM is the only exception. > In the absence of any express or implied prohibition on this implementation (in ucsi, type c, or usb PD specifications), it would be better driver design to support this (be flexible in what you accept as input, be strict in what you output). > > The unexpected behavior which you are talking about is if the user > > uses the displayport pin assignment sysfs node to switch from pin C to pin D. > > Note that the sysfs attribute file for the DP pin assignment is the _standard_ > interface. That sysfs file is the interface the user space will use for changing the > DisplayPort pin assignment. > No problem... With the use of '&' masking between the UCSI connector alt mode vdo with the UCSI partner alt mode vdo, they DP sysfs pin assignment nodes would work. What is a problem is requiring one for one mode index matching between the UCSI connector alt mode table with the UCSI partner alt mode table. According to DP over Type C spec, "Future versions of this Standard may describe other modes associated with DP_SID. Such modes will be identified by having a non-zero value in bits 31:24 of the VDO. The DFP_U shall examine the list of modes returned until it finds 0s in bits 31:24 of the VDO and a non-zero value in bits 23:0 of the VDO (i.e., DisplayPort capabilities). The DFP_U and UFP_U shall use the corresponding offset (indexed from 1) as the Object Position in the Enter Mode, DisplayPort Configure, DisplayPort Status Update, Attention, and Exit Mode commands." Above requirement means that for SVID 0xff01, it is possible for multiple Mode VDOs to be associated with it. Hence, the latest patch's algorithm (of matching mode index will not work). To illustrate: Alt mode adapter responds to Discover Modes VDM (for SVID 0xff01) with following: (a) 0x00000c05 ==> this has svid 0xff01, partner mode index 1 (b) 0x01xxxxxxx --> non-zero value in bits 31 to 24 indicate future DP alt mode ==> this has svid 0xff01, partner mode index 2 But GPU PPM supports only (b) hence its UCSI GET_ALTERNATE_MODES (recipient = connector) returns only one connecter alt mode: (1) 0x01xxxxxx --> GPU supports only extended DP alt mode ==> this has svid 0xff01, connector mode index 1 The patch's algorithm of '==' comparison with mode index would yield the wrong correlation between connector and partner alt modes. Note how using '&' mask between the UCSI's connector mode VDO and UCSI's partner mode VDO would work in all cases. > > Then, a reviously active mode would turn to 'no' and another active > > sysfs node would change to 'yes'. > > > > But, user-mode application has to be able to expect this anyways. (This = > > some active sysfs node changing even though the application did not > > cause it.) For example, a TypeC-to-DP dongle is attached. This dongle > > is also a hub so a downstream USB superspeed device can be connected > > to it. When there is no downstream USB device attached, DisplayPort > > Pin C (4 lanes of video) are supported. But, user plugs in a USB > > device to the dongle - then the dongle will switch from pin C to pin D > > automatically to allow the USB superspeed lanes to be connected. In > > this case, I'd expect your displayport ABI to reflect the automatic > > pin assignment change from C to D caused not by any sysfs activity, > > but simply plugging in USB device to the typec alt mode adapter. In > > our particular case, in addition to the "pin" sysfs node changing from > > "[C] D E" to "C [D] E", the "active" sysfs node with the "yes" value > > would change from one path to another. So, applications must expect values > of sysfs nodes under /sys/class/typec to change by itself. > > We are not talking about just some sysfs files here. Every alternate mode will > have a kernel object representing them. When a change happens on those > objects, for example if a mode is entered or exited (regardless of was it as a > result of user action or not), or the DP pin assignment gets changed, the kernel > generates events for those objects. Those events are the primary source of > initial information in user space. > > The problem is that changing the DP pin assignment has different meaning > than exiting and entering the modes. Again, changing the DP pin assignment > does not cause the mode to be exited and another to be entered. That is how > the standard defines it (and that is the reason why we have the separate sysfs > attribute file for the DP pin assignment in the first place), and that's how the > user space will expect the interface to behave. > > With your PPM however the pin assignment is completely coupled with the > connector mode. If the pin assignment is changed, the mode gets changed, and > if the mode is changed, the pin assignment gets changed. > That is simply not the standard behaviour. > Keep in mind that the previous type c ABI did not have this restriction. This new set of patches for creation of the displayport ABI implicitly added this. However, one of my previous suggestions would maintain this invariance (i.e., changing pin assignments would not cause alt mode change). - basically, when PPM returns alt mode table with separate entries for each mode VDO, the ucsi_ccg.c driver would compress the alt mode table before presenting it to ucsi/displayport.c. So, if this is your only objection, we can accept this. Need to change struct ucsi_ppm to add function pointers to translate ppm's alt mode table into the format that ucsi/displayport.c required. > > > To protect the user space from that, and to keep the interface we > > > provide to it consistent and predictable, we would have to handle > > > your PPM completely separately. The user space will see only one > > > connector DP alt mode no matter what. Even if you are unable change > > > the PPM, the driver will still have to expose the two connector DP > > > alternate modes as one connector alternate mode to the user space. > > > > If this is acceptable to you, > > It really isn't, but it will be something that we have to do unless you guys can fix > your firmware. This will be a large quirk that we would have to implement and > then maintain, just to workaround yet another firmware issue. > > Kernel is already full of workarounds like that, so I'm begging you, please get > the firmware fixed. Surely you now see that it does not exactly comply with the > DP alt mode standard. This "workaround" was actually required by the following design "flaw" in the current patch: - you create partner sysfs nodes using UCSI GET_ALT_MODES (recipient == SOP) - UCSI SET_NEW_CAM requires *connector* alt mode index - problem: how to correlate *connector* alt mode index with *partner* alt mode index in the absence of any device-independent means to do this in the UCSI spec Here's a method of avoiding the above dilemma: - create partner sysfs alt mode nodes using UCSI GET_ALT_MODES (with recipient == connector) - also query UCSI GET_SUPPORTED_CAM - create partner sysfs alt modes only if GET_SUPPORT_CAM returns 1 bit for a particular connector alt mode index You might consider it wrong to populate partner sysfs nodes with connector alt modes, but it makes sense If you ask the question: why report partner alt modes which are not supported at the connector side? You'd never be able to enter that partner alt mode anyways, unless both the connector and partner mutually supported it. > > Why would you need the two DP alt modes for the connector? > As mentioned earlier, this was due to limitation of UCSI which did not allow querying of current DP pin assignment. We maintained compliance with UCSI, yet overcame its limitations using this connector alt mode table arrangement. If you hadn’t objected so strongly to this technique, we would've probably recommended this technique to all our technical partners. Maybe even asked Intel to bless this scheme when they publish the next UCSI spec update. > > Br, > > -- > heikki Thanks, Michael nvpublic
Hi, On Mon, Feb 11, 2019 at 11:43:17PM +0000, Michael Hsu wrote: > Hi Heikki, > > > -----Original Message----- > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Sent: Friday, February 8, 2019 5:48 AM > > To: Michael Hsu <mhsu@nvidia.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > > modes > > > > Hi Michael, > > > > On Thu, Feb 07, 2019 at 10:01:15PM +0000, Michael Hsu wrote: > > > Hi Heikki, > > > > > > > -----Original Message----- > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > Sent: Thursday, February 7, 2019 5:16 AM > > > > To: Michael Hsu <mhsu@nvidia.com> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > > > > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > > > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for > > > > alternate modes > > > > > > > > Hi Michael, > > > > > > > > On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote: > > > > > Your existing patch matches the SVID and then requires one-for-one > > > > > ordering in case either alt mode table has multiple mode entries > > > > > for a > > > > particular SVID. > > > > > > > > And we'll fix that, but that does not solve the issue that I'm > > > > talking about. There are two separate issues here. That problem is a > > > > bug in the driver, but the major problem still is that you have > > > > separate alternate modes for the two DP pin assignments, and fixing the > > index handling does not help with that. > > > > > > > > According to the DP alt mode standard you do not have a separate > > > > mode for every supported DṔ pin assignment. When you change the DP > > > > alt mode pin assignment, the current mode is not exited and a new one > > entered. > > > > You stay in the DisplayPort mode, and simply re-configure using the > > > > DisplayPort Configure command. In your implementation however, the > > > > mode is existed, and a new one entered. So your implementation is > > > > not made according to the DisplayPort Alt Mode standard. > > > > > > > > I guess I have been able to explain just how big of a problem that > > > > is, and also how exactly we will have to handle it... Let's look at > > > > this from user space perspective. > > > > > > > > The user space should not need to be aware of the method used to > > > > interface with the USB Type-C connectors on the system. The kernel > > > > needs to hide that and supply unified interface to the user space > > > > which looks and behaves the same, _always_. > > > > > > > > In case of the DP alt mode, the user space should be allowed to > > > > expect the behaviour to follow the DP alt mode spec, so when the > > > > user space selects another pin assignment for the DP alt mode > > > > adapter, it should be a pass-fail operation without any side > > > > effects, just like explained in the VDM flows in the DP alt mode spec. > > > > > > > > But now with your PPM, there is a major side effect. Every time the > > > > user space selects a new DP pin assignment, the current mode is > > > > actually exited and a new mode entered. That is not standard > > > > behaviour. The user space will not just ignore the mode being exited. > > > > > > Not exactly true... If you look on a USB PD analyzer at the type c > > > connector's CC lines, you would *not* see and "Exit Mode" VDMs - just "DP > > Configure" VDMs. > > > > So your PPM does not actually reflect the behaviour of the hardware and > > firmware. You are not actually changing the mode when you change the pin > > assignment, yet you still want the operating system to think it has to > > change the mode every time it changes the pin assignment. > > > > That means the two modes you have are for completely customised software > > (firmware) representation of the DisplayPort alt mode support that does not > > follow the standard, and as we can now see, reflect even the actual > > behaviour. > > > > If your connector could act as the sink, how many DP alt modes would return > > to the partner as a response to Discover Modes with DP SVID? You would > > return only one regardless of how many pin assignments you support, right? > > That's what you need to return to the OS as well, as source or sink. > > > > > The "active" sysfs node with the value of "yes" would change paths > > > (since another UCSI CAM index is active), but that's because there was > > > a UCSI connector status change. > > > > > > > It has to react to it, most likely by assuming there was a fatal error > > somewhere. > > > > So in practice the user space is now forced to handle your connector > > > > as a special case. > > > > > > The expected user behavior is to read every sysfs node of the form... > > > /sys/class/typec/.../svid > > > /sys/class/typec/.../vdo > > > And if it decides that's the alt mode it wants to activate, it writes 1 to > > > /sys/class/typec/.../active > > > And optionally, reads above sysfs file to confirm that it did indeed > > > change to 'yes'. > > > It would treat it as an failure to enter alt mode if it was read as > > > still 'no'. > > > > Do you understand that those steps you can take _only_ on your hardware in > > order to change the DP pin assignment? They work only with your PPM. The > > approach is completely custom. > > > > I disagree, most user-mode applications would read the contents of the 'svid' > and 'vdo' sysfs nodes and then decide if they want to enter that alternate > mode - by writing to the corresponding 'active' node. Now the user can read the vdo from DP alt mode, and always know that it contains the DisplayPort capabilities, just like it should. If we were to add support for your PPM as is, the user would loose guarantee for that. The vdo would then mean something completely different with your PPM in case of the connectors. The user would then have to be able to, firstly identify the hardware, and then of course know how that handle it. Basically the user space would have to deal with your hardware completely separately. And only with your hardware. <snip> > > The other UCSI PPMs handle this part correctly. There is always only one > > DisplayPort alt mode for the connector, regardless of how many DP pin > > assignment the connector support. Your PPM is the only exception. > > > > In the absence of any express or implied prohibition on this implementation > (in ucsi, type c, or usb PD specifications), it would be better driver design > to support this (be flexible in what you accept as input, be strict in what > you output). > > > > The unexpected behavior which you are talking about is if the user > > > uses the displayport pin assignment sysfs node to switch from pin C to pin > > > D. > > > > Note that the sysfs attribute file for the DP pin assignment is the > > _standard_ interface. That sysfs file is the interface the user space will > > use for changing the DisplayPort pin assignment. > > > > No problem... With the use of '&' masking between the UCSI connector alt mode > vdo with the UCSI partner alt mode vdo, they DP sysfs pin assignment nodes > would work. > > What is a problem is requiring one for one mode index matching between the > UCSI connector alt mode table with the UCSI partner alt mode table. > > According to DP over Type C spec, > "Future versions of this Standard may describe other modes associated with > DP_SID. Such modes will be identified by having a non-zero value in bits > 31:24 of the VDO. The DFP_U shall examine the list of modes returned until > it finds 0s in bits 31:24 of the VDO and a non-zero value in bits 23:0 of > the VDO (i.e., DisplayPort capabilities). The DFP_U and UFP_U shall use the > corresponding offset (indexed from 1) as the Object Position in the Enter > Mode, DisplayPort Configure, DisplayPort Status Update, Attention, and Exit > Mode commands." Yes, we may have multiple modes defined in future versions of the DP alt mode spec, but the connector, just like the partners attached to it, will still have only one mode per definition. Right now there is only one mode defined, but you still return two for the connector. > Above requirement means that for SVID 0xff01, it is possible for multiple Mode > VDOs to be associated with it. Hence, the latest patch's algorithm (of > matching mode index will not work). > > To illustrate: > Alt mode adapter responds to Discover Modes VDM (for SVID 0xff01) with > following: > (a) 0x00000c05 > ==> this has svid 0xff01, partner mode index 1 > (b) 0x01xxxxxxx --> non-zero value in bits 31 to 24 indicate future DP alt > mode > ==> this has svid 0xff01, partner mode index 2 > But GPU PPM supports only (b) hence its UCSI GET_ALTERNATE_MODES (recipient = > connector) returns only one connecter alt mode: > (1) 0x01xxxxxx --> GPU supports only extended DP alt mode > ==> this has svid 0xff01, connector mode index 1 > > The patch's algorithm of '==' comparison with mode index would yield the wrong > correlation between connector and partner alt modes. > > Note how using '&' mask between the UCSI's connector mode VDO and UCSI's > partner mode VDO would work in all cases. The current patch does have a bug, and it needs to be fixed, but that does not fix the issue you have with the two DP modes for the connector. We are talking about two separate issues here. > > > Then, a reviously active mode would turn to 'no' and another active > > > sysfs node would change to 'yes'. > > > > > > But, user-mode application has to be able to expect this anyways. (This = > > > some active sysfs node changing even though the application did not > > > cause it.) For example, a TypeC-to-DP dongle is attached. This dongle > > > is also a hub so a downstream USB superspeed device can be connected > > > to it. When there is no downstream USB device attached, DisplayPort > > > Pin C (4 lanes of video) are supported. But, user plugs in a USB > > > device to the dongle - then the dongle will switch from pin C to pin D > > > automatically to allow the USB superspeed lanes to be connected. In > > > this case, I'd expect your displayport ABI to reflect the automatic > > > pin assignment change from C to D caused not by any sysfs activity, > > > but simply plugging in USB device to the typec alt mode adapter. In > > > our particular case, in addition to the "pin" sysfs node changing from > > > "[C] D E" to "C [D] E", the "active" sysfs node with the "yes" value > > > would change from one path to another. So, applications must expect > > > values of sysfs nodes under /sys/class/typec to change by itself. > > > > We are not talking about just some sysfs files here. Every alternate mode > > will have a kernel object representing them. When a change happens on those > > objects, for example if a mode is entered or exited (regardless of was it as > > a result of user action or not), or the DP pin assignment gets changed, the > > kernel generates events for those objects. Those events are the primary > > source of initial information in user space. > > > > The problem is that changing the DP pin assignment has different meaning > > than exiting and entering the modes. Again, changing the DP pin assignment > > does not cause the mode to be exited and another to be entered. That is how > > the standard defines it (and that is the reason why we have the separate > > sysfs attribute file for the DP pin assignment in the first place), and > > that's how the user space will expect the interface to behave. > > > > With your PPM however the pin assignment is completely coupled with the > > connector mode. If the pin assignment is changed, the mode gets changed, and > > if the mode is changed, the pin assignment gets changed. > > That is simply not the standard behaviour. > > > > Keep in mind that the previous type c ABI did not have this restriction. > This new set of patches for creation of the displayport ABI implicitly added > this. > > However, one of my previous suggestions would maintain this invariance (i.e., > changing pin assignments would not cause alt mode change). > - basically, when PPM returns alt mode table with separate entries for each > mode VDO, the ucsi_ccg.c driver would compress the alt mode table before > presenting it to ucsi/displayport.c. > > So, if this is your only objection, we can accept this. Need to change struct > ucsi_ppm to add function pointers to translate ppm's alt mode table into the > format that ucsi/displayport.c required. Note that you can't just mix that into the ucsi_ccg.c. You are not the only one using the Cypress CCGx USB PD controllers. I think even we use them on some of our platforms, but on our platform the PD controller is usually attached to the embedded controller, so the operating systems continues to access the PPM using the ACPI mailbox. But something like that has to be done. I just realised that the hardware is already being sold to the customers, so I don't think we have a choice. The problem is that once the workaround is in, the firmware will never ever be fixed :-(. > > > > To protect the user space from that, and to keep the interface we > > > > provide to it consistent and predictable, we would have to handle > > > > your PPM completely separately. The user space will see only one > > > > connector DP alt mode no matter what. Even if you are unable change > > > > the PPM, the driver will still have to expose the two connector DP > > > > alternate modes as one connector alternate mode to the user space. > > > > > > If this is acceptable to you, > > > > It really isn't, but it will be something that we have to do unless you guys > > can fix your firmware. This will be a large quirk that we would have to > > implement and then maintain, just to workaround yet another firmware issue. > > > > Kernel is already full of workarounds like that, so I'm begging you, please > > get the firmware fixed. Surely you now see that it does not exactly comply > > with the DP alt mode standard. > > This "workaround" was actually required by the following design "flaw" in the > current patch: > - you create partner sysfs nodes using UCSI GET_ALT_MODES (recipient == SOP) > - UCSI SET_NEW_CAM requires *connector* alt mode index > - problem: how to correlate *connector* alt mode index with *partner* alt mode > index in the absence of any device-independent means to do this in the UCSI > spec I think for clarity's sake I better fix the mode index issue now and send second version of these patches so we can concentrate on the bigger problem, the two connector DP alt modes. > Here's a method of avoiding the above dilemma: > - create partner sysfs alt mode nodes using UCSI GET_ALT_MODES (with recipient > == connector) > - also query UCSI GET_SUPPORTED_CAM > - create partner sysfs alt modes only if GET_SUPPORT_CAM returns 1 bit for a > particular connector alt mode index No! We must not hide stuff like that from the user. Even if the connector does not support a partner mode, we still show it to the user space. This rule should apply to everything else. For example, even when the connector does not support Audio Accessory devices, a partner device should still pop up. Nothing is preventing the user from comparing the connector and partner and determine things from that on its own, for example if audio accessory or an alternate mode is supported or not, but it means the interface must follow the rules. No exceptions. As a shortcut with the alternate modes, the user can determine if a partner mode is supported by simply checking if it's linked to a connector mode (there is a symlink pointing to the connector mode), or alternatively by checking if the mode is bind to a driver. > You might consider it wrong to populate partner sysfs nodes with connector alt > modes, but it makes sense If you ask the question: why report partner alt > modes which are not supported at the connector side? You'd never be able to > enter that partner alt mode anyways, unless both the connector and partner > mutually supported it. We can not put user experience aside. The user space must know what are the capabilities of the platform as well as the capabilities of the devices attached to the platform, and what is going on in general. One of the big problems with USB Type-C is that the user has no idea what does his/her hardware support and not support. That we can not fix, but we must do everything to make sure that it will at least always be possible to supply information to the user. If a connector does not support Audio Accessory, Thunderbolt, or whatever, the user should always be informed by the operating system that you just plugged this or that device and, sorry, but it's not supported on this connector, whenever the user tries to attach one of those devices. Otherwise the user will simply assume that everything is working, the OS did not say anything, and then start scratching his/her head... Why isn't it working? The Billboard Device Class does not help here as it only tells the capabilities of the partner device. > > > > Why would you need the two DP alt modes for the connector? > > > > As mentioned earlier, this was due to limitation of UCSI which did not allow > querying of current DP pin assignment. We maintained compliance with UCSI, yet > overcame its limitations using this connector alt mode table arrangement. > > If you hadn’t objected so strongly to this technique, we would've probably > recommended this technique to all our technical partners. > > Maybe even asked Intel to bless this scheme when they publish the next UCSI > spec update. Unfortunately the other players clearly interpreted the situation differently. I think spec update is needed even regardless of this issue. I'll see if I can start a dialog first inside Intel regarding the limitations and lack of clear definition in some parts of the current UCSI spec. thanks,
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Tuesday, February 12, 2019 7:22 AM > To: Michael Hsu <mhsu@nvidia.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > modes > > Hi, > > On Mon, Feb 11, 2019 at 11:43:17PM +0000, Michael Hsu wrote: > > Hi Heikki, > > > > > -----Original Message----- > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Sent: Friday, February 8, 2019 5:48 AM > > > To: Michael Hsu <mhsu@nvidia.com> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > > > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for > > > alternate modes > > > > > > Hi Michael, > > > > > > On Thu, Feb 07, 2019 at 10:01:15PM +0000, Michael Hsu wrote: > > > > Hi Heikki, > > > > > > > > > -----Original Message----- > > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > Sent: Thursday, February 7, 2019 5:16 AM > > > > > To: Michael Hsu <mhsu@nvidia.com> > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ajay Gupta > > > > > <ajayg@nvidia.com>; linux-usb@vger.kernel.org > > > > > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support > > > > > for alternate modes > > > > > > > > > > Hi Michael, > > > > > > > > > > On Wed, Feb 06, 2019 at 10:36:22PM +0000, Michael Hsu wrote: > > > > > > Your existing patch matches the SVID and then requires > > > > > > one-for-one ordering in case either alt mode table has > > > > > > multiple mode entries for a > > > > > particular SVID. > > > > > > > > > > And we'll fix that, but that does not solve the issue that I'm > > > > > talking about. There are two separate issues here. That problem > > > > > is a bug in the driver, but the major problem still is that you > > > > > have separate alternate modes for the two DP pin assignments, > > > > > and fixing the > > > index handling does not help with that. > > > > > > > > > > According to the DP alt mode standard you do not have a separate > > > > > mode for every supported DṔ pin assignment. When you change the > > > > > DP alt mode pin assignment, the current mode is not exited and a > > > > > new one > > > entered. > > > > > You stay in the DisplayPort mode, and simply re-configure using > > > > > the DisplayPort Configure command. In your implementation > > > > > however, the mode is existed, and a new one entered. So your > > > > > implementation is not made according to the DisplayPort Alt Mode > standard. > > > > > > > > > > I guess I have been able to explain just how big of a problem > > > > > that is, and also how exactly we will have to handle it... Let's > > > > > look at this from user space perspective. > > > > > > > > > > The user space should not need to be aware of the method used to > > > > > interface with the USB Type-C connectors on the system. The > > > > > kernel needs to hide that and supply unified interface to the > > > > > user space which looks and behaves the same, _always_. > > > > > > > > > > In case of the DP alt mode, the user space should be allowed to > > > > > expect the behaviour to follow the DP alt mode spec, so when the > > > > > user space selects another pin assignment for the DP alt mode > > > > > adapter, it should be a pass-fail operation without any side > > > > > effects, just like explained in the VDM flows in the DP alt mode spec. > > > > > > > > > > But now with your PPM, there is a major side effect. Every time > > > > > the user space selects a new DP pin assignment, the current mode > > > > > is actually exited and a new mode entered. That is not standard > > > > > behaviour. The user space will not just ignore the mode being exited. > > > > > > > > Not exactly true... If you look on a USB PD analyzer at the type c > > > > connector's CC lines, you would *not* see and "Exit Mode" VDMs - > > > > just "DP > > > Configure" VDMs. > > > > > > So your PPM does not actually reflect the behaviour of the hardware > > > and firmware. You are not actually changing the mode when you change > > > the pin assignment, yet you still want the operating system to think > > > it has to change the mode every time it changes the pin assignment. > > > > > > That means the two modes you have are for completely customised > > > software > > > (firmware) representation of the DisplayPort alt mode support that > > > does not follow the standard, and as we can now see, reflect even > > > the actual behaviour. > > > > > > If your connector could act as the sink, how many DP alt modes would > > > return to the partner as a response to Discover Modes with DP SVID? > > > You would return only one regardless of how many pin assignments you > support, right? > > > That's what you need to return to the OS as well, as source or sink. > > > > > > > The "active" sysfs node with the value of "yes" would change paths > > > > (since another UCSI CAM index is active), but that's because there > > > > was a UCSI connector status change. > > > > > > > > > It has to react to it, most likely by assuming there was a fatal > > > > > error > > > somewhere. > > > > > So in practice the user space is now forced to handle your > > > > > connector as a special case. > > > > > > > > The expected user behavior is to read every sysfs node of the form... > > > > /sys/class/typec/.../svid > > > > /sys/class/typec/.../vdo > > > > And if it decides that's the alt mode it wants to activate, it writes 1 to > > > > /sys/class/typec/.../active > > > > And optionally, reads above sysfs file to confirm that it did > > > > indeed change to 'yes'. > > > > It would treat it as an failure to enter alt mode if it was read > > > > as still 'no'. > > > > > > Do you understand that those steps you can take _only_ on your > > > hardware in order to change the DP pin assignment? They work only > > > with your PPM. The approach is completely custom. > > > > > > > I disagree, most user-mode applications would read the contents of the 'svid' > > and 'vdo' sysfs nodes and then decide if they want to enter that > > alternate mode - by writing to the corresponding 'active' node. > > Now the user can read the vdo from DP alt mode, and always know that it > contains the DisplayPort capabilities, just like it should. If we were to add > support for your PPM as is, the user would loose guarantee for that. The vdo > would then mean something completely different with your PPM in case of the > connectors. > > The user would then have to be able to, firstly identify the hardware, and then > of course know how that handle it. > > Basically the user space would have to deal with your hardware completely > separately. And only with your hardware. > > <snip> > > > > The other UCSI PPMs handle this part correctly. There is always only > > > one DisplayPort alt mode for the connector, regardless of how many > > > DP pin assignment the connector support. Your PPM is the only exception. > > > > > > > In the absence of any express or implied prohibition on this > > implementation (in ucsi, type c, or usb PD specifications), it would > > be better driver design to support this (be flexible in what you > > accept as input, be strict in what you output). > > > > > > The unexpected behavior which you are talking about is if the user > > > > uses the displayport pin assignment sysfs node to switch from pin > > > > C to pin D. > > > > > > Note that the sysfs attribute file for the DP pin assignment is the > > > _standard_ interface. That sysfs file is the interface the user > > > space will use for changing the DisplayPort pin assignment. > > > > > > > No problem... With the use of '&' masking between the UCSI connector > > alt mode vdo with the UCSI partner alt mode vdo, they DP sysfs pin > > assignment nodes would work. > > > > What is a problem is requiring one for one mode index matching between > > the UCSI connector alt mode table with the UCSI partner alt mode table. > > > > According to DP over Type C spec, > > "Future versions of this Standard may describe other modes associated with > > DP_SID. Such modes will be identified by having a non-zero value in bits > > 31:24 of the VDO. The DFP_U shall examine the list of modes returned until > > it finds 0s in bits 31:24 of the VDO and a non-zero value in bits 23:0 of > > the VDO (i.e., DisplayPort capabilities). The DFP_U and UFP_U shall use the > > corresponding offset (indexed from 1) as the Object Position in the Enter > > Mode, DisplayPort Configure, DisplayPort Status Update, Attention, and Exit > > Mode commands." > > Yes, we may have multiple modes defined in future versions of the DP alt mode > spec, but the connector, just like the partners attached to it, will still have only > one mode per definition. > > Right now there is only one mode defined, but you still return two for the > connector. > > > Above requirement means that for SVID 0xff01, it is possible for > > multiple Mode VDOs to be associated with it. Hence, the latest patch's > > algorithm (of matching mode index will not work). > > > > To illustrate: > > Alt mode adapter responds to Discover Modes VDM (for SVID 0xff01) with > > following: > > (a) 0x00000c05 > > ==> this has svid 0xff01, partner mode index 1 > > (b) 0x01xxxxxxx --> non-zero value in bits 31 to 24 indicate future DP alt > > mode > > ==> this has svid 0xff01, partner mode index 2 But GPU PPM > > supports only (b) hence its UCSI GET_ALTERNATE_MODES (recipient = > > connector) returns only one connecter alt mode: > > (1) 0x01xxxxxx --> GPU supports only extended DP alt mode > > ==> this has svid 0xff01, connector mode index 1 > > > > The patch's algorithm of '==' comparison with mode index would yield > > the wrong correlation between connector and partner alt modes. > > > > Note how using '&' mask between the UCSI's connector mode VDO and > > UCSI's partner mode VDO would work in all cases. > > The current patch does have a bug, and it needs to be fixed, but that does not > fix the issue you have with the two DP modes for the connector. We are talking > about two separate issues here. > > > > > Then, a reviously active mode would turn to 'no' and another > > > > active sysfs node would change to 'yes'. > > > > > > > > But, user-mode application has to be able to expect this anyways. > > > > (This = some active sysfs node changing even though the > > > > application did not cause it.) For example, a TypeC-to-DP dongle > > > > is attached. This dongle is also a hub so a downstream USB > > > > superspeed device can be connected to it. When there is no > > > > downstream USB device attached, DisplayPort Pin C (4 lanes of > > > > video) are supported. But, user plugs in a USB device to the > > > > dongle - then the dongle will switch from pin C to pin D > > > > automatically to allow the USB superspeed lanes to be connected. > > > > In this case, I'd expect your displayport ABI to reflect the > > > > automatic pin assignment change from C to D caused not by any > > > > sysfs activity, but simply plugging in USB device to the typec alt > > > > mode adapter. In our particular case, in addition to the "pin" > > > > sysfs node changing from "[C] D E" to "C [D] E", the "active" > > > > sysfs node with the "yes" value would change from one path to another. > So, applications must expect values of sysfs nodes under /sys/class/typec to > change by itself. > > > > > > We are not talking about just some sysfs files here. Every alternate > > > mode will have a kernel object representing them. When a change > > > happens on those objects, for example if a mode is entered or exited > > > (regardless of was it as a result of user action or not), or the DP > > > pin assignment gets changed, the kernel generates events for those > > > objects. Those events are the primary source of initial information in user > space. > > > > > > The problem is that changing the DP pin assignment has different > > > meaning than exiting and entering the modes. Again, changing the DP > > > pin assignment does not cause the mode to be exited and another to > > > be entered. That is how the standard defines it (and that is the > > > reason why we have the separate sysfs attribute file for the DP pin > > > assignment in the first place), and that's how the user space will expect the > interface to behave. > > > > > > With your PPM however the pin assignment is completely coupled with > > > the connector mode. If the pin assignment is changed, the mode gets > > > changed, and if the mode is changed, the pin assignment gets changed. > > > That is simply not the standard behaviour. > > > > > > > Keep in mind that the previous type c ABI did not have this restriction. > > This new set of patches for creation of the displayport ABI implicitly > > added this. > > > > However, one of my previous suggestions would maintain this invariance > > (i.e., changing pin assignments would not cause alt mode change). > > - basically, when PPM returns alt mode table with separate entries for > > each mode VDO, the ucsi_ccg.c driver would compress the alt mode table > > before presenting it to ucsi/displayport.c. > > > > So, if this is your only objection, we can accept this. Need to > > change struct ucsi_ppm to add function pointers to translate ppm's alt > > mode table into the format that ucsi/displayport.c required. > > Note that you can't just mix that into the ucsi_ccg.c. You are not the only one > using the Cypress CCGx USB PD controllers. I think even we use them on some > of our platforms, but on our platform the PD controller is usually attached to > the embedded controller, so the operating systems continues to access the PPM > using the ACPI mailbox. > > But something like that has to be done. I just realised that the hardware is > already being sold to the customers, so I don't think we have a choice. The > problem is that once the workaround is in, the firmware will never ever be > fixed :-(. > We can't change our PPM firmware since we have too many devices out their already. But, we'll be submitting future patches on top of yours which address the objections you've mentioned already in this thread. > > > > > To protect the user space from that, and to keep the interface > > > > > we provide to it consistent and predictable, we would have to > > > > > handle your PPM completely separately. The user space will see > > > > > only one connector DP alt mode no matter what. Even if you are > > > > > unable change the PPM, the driver will still have to expose the > > > > > two connector DP alternate modes as one connector alternate mode to > the user space. > > > > > > > > If this is acceptable to you, > > > > > > It really isn't, but it will be something that we have to do unless > > > you guys can fix your firmware. This will be a large quirk that we > > > would have to implement and then maintain, just to workaround yet > another firmware issue. > > > > > > Kernel is already full of workarounds like that, so I'm begging you, > > > please get the firmware fixed. Surely you now see that it does not > > > exactly comply with the DP alt mode standard. > > > > This "workaround" was actually required by the following design "flaw" > > in the current patch: > > - you create partner sysfs nodes using UCSI GET_ALT_MODES (recipient > > == SOP) > > - UCSI SET_NEW_CAM requires *connector* alt mode index > > - problem: how to correlate *connector* alt mode index with *partner* > > alt mode index in the absence of any device-independent means to do > > this in the UCSI spec > > I think for clarity's sake I better fix the mode index issue now and send second > version of these patches so we can concentrate on the bigger problem, the two > connector DP alt modes. > Agreed. Please update your patch for the mode index issue (being able to reliably match partner mode index with the connector mode index returned by UCSI). > > Here's a method of avoiding the above dilemma: > > - create partner sysfs alt mode nodes using UCSI GET_ALT_MODES (with > > recipient == connector) > > - also query UCSI GET_SUPPORTED_CAM > > - create partner sysfs alt modes only if GET_SUPPORT_CAM returns 1 bit > > for a particular connector alt mode index > > No! We must not hide stuff like that from the user. Even if the connector does > not support a partner mode, we still show it to the user space. > OK, but this means the "active" sysfs node for a partner mode (which is not supported by GET_SUPPORTED_CAM) will always show "no". And writing "yes" to it will silently fail... Did this intentional? Having some partner sysfs "active" nodes which do nothing without any warning to user that they don't work? > This rule should apply to everything else. For example, even when the > connector does not support Audio Accessory devices, a partner device should > still pop up. > > Nothing is preventing the user from comparing the connector and partner and > determine things from that on its own, for example if audio accessory or an > alternate mode is supported or not, but it means the interface must follow the > rules. No exceptions. > > As a shortcut with the alternate modes, the user can determine if a partner > mode is supported by simply checking if it's linked to a connector mode (there > is a symlink pointing to the connector mode), or alternatively by checking if the > mode is bind to a driver. > OK, we can come up with some method of interfacing our PPM firmware's mode table with your patch's expectation (of only one alt mode index per DP alt mode). Let's treat that independently from your set of patches. > > You might consider it wrong to populate partner sysfs nodes with > > connector alt modes, but it makes sense If you ask the question: why > > report partner alt modes which are not supported at the connector > > side? You'd never be able to enter that partner alt mode anyways, > > unless both the connector and partner mutually supported it. > > We can not put user experience aside. The user space must know what are the > capabilities of the platform as well as the capabilities of the devices attached to > the platform, and what is going on in general. > > One of the big problems with USB Type-C is that the user has no idea what does > his/her hardware support and not support. That we can not fix, but we must do > everything to make sure that it will at least always be possible to supply > information to the user. > > If a connector does not support Audio Accessory, Thunderbolt, or whatever, the > user should always be informed by the operating system that you just plugged > this or that device and, sorry, but it's not supported on this connector, > whenever the user tries to attach one of those devices. > > Otherwise the user will simply assume that everything is working, the OS did > not say anything, and then start scratching his/her head... Why isn't it working? > > The Billboard Device Class does not help here as it only tells the capabilities of > the partner device. > > > > > > > Why would you need the two DP alt modes for the connector? > > > > > > > As mentioned earlier, this was due to limitation of UCSI which did not > > allow querying of current DP pin assignment. We maintained compliance > > with UCSI, yet overcame its limitations using this connector alt mode table > arrangement. > > > > If you hadn’t objected so strongly to this technique, we would've > > probably recommended this technique to all our technical partners. > > > > Maybe even asked Intel to bless this scheme when they publish the next > > UCSI spec update. > > Unfortunately the other players clearly interpreted the situation differently. > > I think spec update is needed even regardless of this issue. I'll see if I can start a > dialog first inside Intel regarding the limitations and lack of clear definition in > some parts of the current UCSI spec. > > > thanks, > > -- > heikki Thanks, Michael nvpublic
Hi, On Sat, Feb 16, 2019 at 01:36:05AM +0000, Michael Hsu wrote: > > I think for clarity's sake I better fix the mode index issue now and send second > > version of these patches so we can concentrate on the bigger problem, the two > > connector DP alt modes. > > > > Agreed. Please update your patch for the mode index issue (being able to > reliably match partner mode index with the connector mode index returned by > UCSI). I'll finish some other task before coming back to this. We shouldn't be in any hurry with this. These are not going to v5.1 in any case. > > > Here's a method of avoiding the above dilemma: > > > - create partner sysfs alt mode nodes using UCSI GET_ALT_MODES (with > > > recipient == connector) > > > - also query UCSI GET_SUPPORTED_CAM > > > - create partner sysfs alt modes only if GET_SUPPORT_CAM returns 1 bit > > > for a particular connector alt mode index > > > > No! We must not hide stuff like that from the user. Even if the connector does > > not support a partner mode, we still show it to the user space. > > > > OK, but this means the "active" sysfs node for a partner mode (which is not > supported by GET_SUPPORTED_CAM) will always show "no". And writing "yes" > to it will silently fail... They will not always say "no", but you can not change the value without an alternate mode driver. > Did this intentional? Having some partner sysfs "active" nodes which do > nothing without any warning to user that they don't work? The key here is the driver. The user will know that partner alternate mode can't be operated unless it has been bind to a driver. The ABI for the alternate modes is defined for the typec bus, not the class. So in real world the user space applications will not start doing anything with an alternate mode, other than possible notifying the user of its existence, until the user space sees kernel event indicating that the alternate mode has been bind to a driver. Br,
diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c index ffa3b4c3f338..1dabafb74320 100644 --- a/drivers/usb/typec/ucsi/trace.c +++ b/drivers/usb/typec/ucsi/trace.c @@ -60,3 +60,15 @@ const char *ucsi_cci_str(u32 cci) return ""; } + +static const char * const ucsi_recipient_strs[] = { + [UCSI_RECIPIENT_CON] = "port", + [UCSI_RECIPIENT_SOP] = "partner", + [UCSI_RECIPIENT_SOP_P] = "plug (prime)", + [UCSI_RECIPIENT_SOP_PP] = "plug (double prime)", +}; + +const char *ucsi_recipient_str(u8 recipient) +{ + return ucsi_recipient_strs[recipient]; +} diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h index 5e2906df2db7..783ec9c72055 100644 --- a/drivers/usb/typec/ucsi/trace.h +++ b/drivers/usb/typec/ucsi/trace.h @@ -7,6 +7,7 @@ #define __UCSI_TRACE_H #include <linux/tracepoint.h> +#include <linux/usb/typec_altmode.h> const char *ucsi_cmd_str(u64 raw_cmd); const char *ucsi_ack_str(u8 ack); @@ -134,6 +135,31 @@ DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port, TP_ARGS(port, status) ); +DECLARE_EVENT_CLASS(ucsi_log_register_altmode, + TP_PROTO(u8 recipient, struct typec_altmode *alt), + TP_ARGS(recipient, alt), + TP_STRUCT__entry( + __field(u8, recipient) + __field(u16, svid) + __field(u8, mode) + __field(u32, vdo) + ), + TP_fast_assign( + __entry->recipient = recipient; + __entry->svid = alt->svid; + __entry->mode = alt->mode; + __entry->vdo = alt->vdo; + ), + TP_printk("%s alt mode: svid %04x, mode %d vdo %x", + ucsi_recipient_str(__entry->recipient), __entry->svid, + __entry->mode, __entry->vdo) +); + +DEFINE_EVENT(ucsi_log_register_altmode, ucsi_register_altmode, + TP_PROTO(u8 recipient, struct typec_altmode *alt), + TP_ARGS(recipient, alt) +); + #endif /* __UCSI_TRACE_H */ /* This part must be outside protection */ diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 8d0a6fe748bd..5190f8dd4548 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -12,7 +12,7 @@ #include <linux/module.h> #include <linux/delay.h> #include <linux/slab.h> -#include <linux/usb/typec.h> +#include <linux/usb/typec_altmode.h> #include "ucsi.h" #include "trace.h" @@ -45,22 +45,6 @@ enum ucsi_status { UCSI_ERROR, }; -struct ucsi_connector { - int num; - - struct ucsi *ucsi; - struct work_struct work; - struct completion complete; - - struct typec_port *port; - struct typec_partner *partner; - - struct typec_capability typec_cap; - - struct ucsi_connector_status status; - struct ucsi_connector_capability cap; -}; - struct ucsi { struct device *dev; struct ucsi_ppm *ppm; @@ -238,8 +222,201 @@ static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl, return ret; } +int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl, + void *retval, size_t size) +{ + int ret; + + mutex_lock(&ucsi->ppm_lock); + ret = ucsi_run_command(ucsi, ctrl, retval, size); + mutex_unlock(&ucsi->ppm_lock); + + return ret; +} + /* -------------------------------------------------------------------------- */ +void ucsi_altmode_update_active(struct ucsi_connector *con) +{ + struct typec_altmode *altmode; + struct ucsi_control ctrl; + int ret; + u8 cur; + int i; + + UCSI_CMD_GET_CURRENT_CAM(ctrl, con->num); + ret = ucsi_run_command(con->ucsi, &ctrl, &cur, sizeof(cur)); + if (ret < 0) { + dev_err(con->ucsi->dev, "GET_CURRENT_CAM command failed\n"); + return; + } + + altmode = typec_match_altmode(con->partner_altmode, UCSI_MAX_ALTMODES, + con->port_altmode[cur]->svid, + con->port_altmode[cur]->mode); + + for (i = 0; con->partner_altmode[i]; i++) + typec_altmode_update_active(con->partner_altmode[i], + con->partner_altmode[i] == altmode); +} + +static u8 ucsi_altmode_next_mode(struct typec_altmode **alt, u16 svid) +{ + u8 mode = 1; + int i; + + for (i = 0; alt[i]; i++) + if (alt[i]->svid == svid) + mode++; + + return mode; +} + +static int ucsi_next_altmode(struct typec_altmode **alt) +{ + int i = 0; + + for (i = 0; i < UCSI_MAX_ALTMODES; i++) + if (!alt[i]) + return i; + + return -ENOENT; +} + +static int ucsi_register_altmode(struct ucsi_connector *con, + struct typec_altmode_desc *desc, + u8 recipient) +{ + struct typec_altmode *alt; + int ret; + int i; + + switch (recipient) { + case UCSI_RECIPIENT_CON: + i = ucsi_next_altmode(con->port_altmode); + if (i < 0) { + ret = i; + goto err; + } + + desc->mode = ucsi_altmode_next_mode(con->port_altmode, + desc->svid); + + alt = typec_port_register_altmode(con->port, desc); + if (IS_ERR(alt)) { + ret = PTR_ERR(alt); + goto err; + } + + con->port_altmode[i] = alt; + break; + case UCSI_RECIPIENT_SOP: + i = ucsi_next_altmode(con->partner_altmode); + if (i < 0) { + ret = i; + goto err; + } + + desc->mode = ucsi_altmode_next_mode(con->partner_altmode, + desc->svid); + + alt = typec_partner_register_altmode(con->partner, desc); + if (IS_ERR(alt)) { + ret = PTR_ERR(alt); + goto err; + } + + con->partner_altmode[i] = alt; + break; + default: + return -EINVAL; + } + + trace_ucsi_register_altmode(recipient, alt); + + return 0; + +err: + dev_err(con->ucsi->dev, "failed to registers svid 0x%04x mode %d\n", + desc->svid, desc->mode); + + return ret; +} + +static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient) +{ + int max_altmodes = UCSI_MAX_ALTMODES; + struct typec_altmode_desc desc; + struct ucsi_altmode alt[2]; + struct ucsi_control ctrl; + int num = 1; + int ret; + int len; + int j; + int i; + + if (!(con->ucsi->cap.features & UCSI_CAP_ALT_MODE_DETAILS)) + return 0; + + if (recipient == UCSI_RECIPIENT_CON) + max_altmodes = con->ucsi->cap.num_alt_modes; + + for (i = 0; i < max_altmodes;) { + memset(alt, 0, sizeof(alt)); + UCSI_CMD_GET_ALTERNATE_MODES(ctrl, recipient, con->num, i, 1); + len = ucsi_run_command(con->ucsi, &ctrl, alt, sizeof(alt)); + if (len <= 0) + return len; + + /* + * This code is requesting one alt mode at a time, but some PPMs + * may still return two. If that happens both alt modes need be + * registered and the offset for the next alt mode has to be + * incremented. + */ + num = len / sizeof(alt[0]); + i += num; + + for (j = 0; j < num; j++) { + if (!alt[j].svid) + return 0; + + memset(&desc, 0, sizeof(desc)); + desc.vdo = alt[j].mid; + desc.svid = alt[j].svid; + desc.roles = TYPEC_PORT_DRD; + + ret = ucsi_register_altmode(con, &desc, recipient); + if (ret) + return ret; + } + } + + return 0; +} + +static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) +{ + struct typec_altmode **adev; + int i = 0; + + switch (recipient) { + case UCSI_RECIPIENT_CON: + adev = con->port_altmode; + break; + case UCSI_RECIPIENT_SOP: + adev = con->partner_altmode; + break; + default: + return; + } + + while (adev[i]) { + typec_unregister_altmode(adev[i]); + adev[i++] = NULL; + } +} + static void ucsi_pwr_opmode_change(struct ucsi_connector *con) { switch (con->status.pwr_op_mode) { @@ -299,10 +476,43 @@ static void ucsi_unregister_partner(struct ucsi_connector *con) if (!con->partner) return; + ucsi_unregister_altmodes(con, UCSI_RECIPIENT_SOP); typec_unregister_partner(con->partner); con->partner = NULL; } +static void ucsi_partner_change(struct ucsi_connector *con) +{ + int ret; + + if (!con->partner) + return; + + switch (con->status.partner_type) { + case UCSI_CONSTAT_PARTNER_TYPE_UFP: + typec_set_data_role(con->port, TYPEC_HOST); + break; + case UCSI_CONSTAT_PARTNER_TYPE_DFP: + typec_set_data_role(con->port, TYPEC_DEVICE); + break; + default: + break; + } + + /* Complete pending data role swap */ + if (!completion_done(&con->complete)) + complete(&con->complete); + + /* Can't rely on Partner Flags field. Always checking the alt modes. */ + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP); + if (ret) + dev_err(con->ucsi->dev, + "con%d: failed to register partner alternate modes\n", + con->num); + else + ucsi_altmode_update_active(con); +} + static void ucsi_connector_change(struct work_struct *work) { struct ucsi_connector *con = container_of(work, struct ucsi_connector, @@ -311,10 +521,10 @@ static void ucsi_connector_change(struct work_struct *work) struct ucsi_control ctrl; int ret; - mutex_lock(&ucsi->ppm_lock); + mutex_lock(&con->lock); UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num); - ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status)); + ret = ucsi_send_command(ucsi, &ctrl, &con->status, sizeof(con->status)); if (ret < 0) { dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n", __func__, ret); @@ -332,23 +542,6 @@ static void ucsi_connector_change(struct work_struct *work) complete(&con->complete); } - if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) { - switch (con->status.partner_type) { - case UCSI_CONSTAT_PARTNER_TYPE_UFP: - typec_set_data_role(con->port, TYPEC_HOST); - break; - case UCSI_CONSTAT_PARTNER_TYPE_DFP: - typec_set_data_role(con->port, TYPEC_DEVICE); - break; - default: - break; - } - - /* Complete pending data role swap */ - if (!completion_done(&con->complete)) - complete(&con->complete); - } - if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) { typec_set_pwr_role(con->port, con->status.pwr_dir); @@ -369,6 +562,19 @@ static void ucsi_connector_change(struct work_struct *work) ucsi_unregister_partner(con); } + if (con->status.change & UCSI_CONSTAT_CAM_CHANGE) { + /* + * We don't need to know the currently supported alt modes here. + * Running GET_CAM_SUPPORTED command just to make sure the PPM + * does not get stuck in case it assumes we do so. + */ + UCSI_CMD_GET_CAM_SUPPORTED(ctrl, con->num); + ucsi_run_command(con->ucsi, &ctrl, NULL, 0); + } + + if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) + ucsi_partner_change(con); + ret = ucsi_ack(ucsi, UCSI_ACK_EVENT); if (ret) dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret); @@ -377,7 +583,7 @@ static void ucsi_connector_change(struct work_struct *work) out_unlock: clear_bit(EVENT_PENDING, &ucsi->flags); - mutex_unlock(&ucsi->ppm_lock); + mutex_unlock(&con->lock); } /** @@ -427,7 +633,7 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard) UCSI_CMD_CONNECTOR_RESET(ctrl, con, hard); - return ucsi_run_command(con->ucsi, &ctrl, NULL, 0); + return ucsi_send_command(con->ucsi, &ctrl, NULL, 0); } static int ucsi_reset_ppm(struct ucsi *ucsi) @@ -481,15 +687,17 @@ static int ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control *ctrl) { int ret; - ret = ucsi_run_command(con->ucsi, ctrl, NULL, 0); + ret = ucsi_send_command(con->ucsi, ctrl, NULL, 0); if (ret == -ETIMEDOUT) { struct ucsi_control c; /* PPM most likely stopped responding. Resetting everything. */ + mutex_lock(&con->ucsi->ppm_lock); ucsi_reset_ppm(con->ucsi); + mutex_unlock(&con->ucsi->ppm_lock); UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL); - ucsi_run_command(con->ucsi, &c, NULL, 0); + ucsi_send_command(con->ucsi, &c, NULL, 0); ucsi_reset_connector(con, true); } @@ -504,10 +712,12 @@ ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role) struct ucsi_control ctrl; int ret = 0; - if (!con->partner) - return -ENOTCONN; + mutex_lock(&con->lock); - mutex_lock(&con->ucsi->ppm_lock); + if (!con->partner) { + ret = -ENOTCONN; + goto out_unlock; + } if ((con->status.partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP && role == TYPEC_DEVICE) || @@ -520,18 +730,14 @@ ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role) if (ret < 0) goto out_unlock; - mutex_unlock(&con->ucsi->ppm_lock); - if (!wait_for_completion_timeout(&con->complete, msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) - return -ETIMEDOUT; - - return 0; + ret = -ETIMEDOUT; out_unlock: - mutex_unlock(&con->ucsi->ppm_lock); + mutex_unlock(&con->lock); - return ret; + return ret < 0 ? ret : 0; } static int @@ -541,10 +747,12 @@ ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role) struct ucsi_control ctrl; int ret = 0; - if (!con->partner) - return -ENOTCONN; + mutex_lock(&con->lock); - mutex_lock(&con->ucsi->ppm_lock); + if (!con->partner) { + ret = -ENOTCONN; + goto out_unlock; + } if (con->status.pwr_dir == role) goto out_unlock; @@ -554,13 +762,11 @@ ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role) if (ret < 0) goto out_unlock; - mutex_unlock(&con->ucsi->ppm_lock); - if (!wait_for_completion_timeout(&con->complete, - msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) - return -ETIMEDOUT; - - mutex_lock(&con->ucsi->ppm_lock); + msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) { + ret = -ETIMEDOUT; + goto out_unlock; + } /* Something has gone wrong while swapping the role */ if (con->status.pwr_op_mode != UCSI_CONSTAT_PWR_OPMODE_PD) { @@ -569,7 +775,7 @@ ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role) } out_unlock: - mutex_unlock(&con->ucsi->ppm_lock); + mutex_unlock(&con->lock); return ret; } @@ -595,6 +801,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) INIT_WORK(&con->work, ucsi_connector_change); init_completion(&con->complete); + mutex_init(&con->lock); con->num = index + 1; con->ucsi = ucsi; @@ -636,6 +843,12 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) if (IS_ERR(con->port)) return PTR_ERR(con->port); + /* Alternate modes */ + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON); + if (ret) + dev_err(ucsi->dev, "con%d: failed to register alt modes\n", + con->num); + /* Get the status */ UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num); ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status)); @@ -662,6 +875,16 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) if (con->status.connected) ucsi_register_partner(con); + if (con->partner) { + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP); + if (ret) + dev_err(ucsi->dev, + "con%d: failed to register alternate modes\n", + con->num); + else + ucsi_altmode_update_active(con); + } + trace_ucsi_register_port(con->num, &con->status); return 0; @@ -788,17 +1011,15 @@ void ucsi_unregister_ppm(struct ucsi *ucsi) /* Make sure that we are not in the middle of driver initialization */ cancel_work_sync(&ucsi->work); - mutex_lock(&ucsi->ppm_lock); - /* Disable everything except command complete notification */ UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_CMD_COMPLETE) - ucsi_run_command(ucsi, &ctrl, NULL, 0); - - mutex_unlock(&ucsi->ppm_lock); + ucsi_send_command(ucsi, &ctrl, NULL, 0); for (i = 0; i < ucsi->cap.num_connectors; i++) { cancel_work_sync(&ucsi->connector[i].work); ucsi_unregister_partner(&ucsi->connector[i]); + ucsi_unregister_altmodes(&ucsi->connector[i], + UCSI_RECIPIENT_CON); typec_unregister_port(ucsi->connector[i].port); } diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 53b80f40a908..c416bae4b5ca 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -6,6 +6,7 @@ #include <linux/bitops.h> #include <linux/device.h> #include <linux/types.h> +#include <linux/usb/typec.h> /* -------------------------------------------------------------------------- */ @@ -60,6 +61,20 @@ struct ucsi_uor_cmd { u16:6; /* reserved */ } __packed; +/* Get Alternate Modes Command structure */ +struct ucsi_altmode_cmd { + u8 cmd; + u8 length; + u8 recipient; +#define UCSI_RECIPIENT_CON 0 +#define UCSI_RECIPIENT_SOP 1 +#define UCSI_RECIPIENT_SOP_P 2 +#define UCSI_RECIPIENT_SOP_PP 3 + u8 con_num; + u8 offset; + u8 num_altmodes; +} __packed; + struct ucsi_control { union { u64 raw_cmd; @@ -67,6 +82,7 @@ struct ucsi_control { struct ucsi_uor_cmd uor; struct ucsi_ack_cmd ack; struct ucsi_con_rst con_rst; + struct ucsi_altmode_cmd alt; }; }; @@ -112,6 +128,30 @@ struct ucsi_control { (_ctrl_).cmd.data = _con_; \ } +/* Helper for preparing ucsi_control for GET_ALTERNATE_MODES command. */ +#define UCSI_CMD_GET_ALTERNATE_MODES(_ctrl_, _r_, _con_num_, _o_, _num_)\ +{ \ + __UCSI_CMD((_ctrl_), UCSI_GET_ALTERNATE_MODES) \ + _ctrl_.alt.recipient = (_r_); \ + _ctrl_.alt.con_num = (_con_num_); \ + _ctrl_.alt.offset = (_o_); \ + _ctrl_.alt.num_altmodes = (_num_) - 1; \ +} + +/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */ +#define UCSI_CMD_GET_CAM_SUPPORTED(_ctrl_, _con_) \ +{ \ + __UCSI_CMD((_ctrl_), UCSI_GET_CAM_SUPPORTED) \ + _ctrl_.cmd.data = (_con_); \ +} + +/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */ +#define UCSI_CMD_GET_CURRENT_CAM(_ctrl_, _con_) \ +{ \ + __UCSI_CMD((_ctrl_), UCSI_GET_CURRENT_CAM) \ + _ctrl_.cmd.data = (_con_); \ +} + /* Helper for preparing ucsi_control for GET_CONNECTOR_STATUS command. */ #define UCSI_CMD_GET_CONNECTOR_STATUS(_ctrl_, _con_) \ { \ @@ -334,4 +374,36 @@ struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm); void ucsi_unregister_ppm(struct ucsi *ucsi); void ucsi_notify(struct ucsi *ucsi); +/* -------------------------------------------------------------------------- */ + +struct ucsi; + +#define UCSI_MAX_SVID 5 +#define UCSI_MAX_ALTMODES (UCSI_MAX_SVID * 6) + +struct ucsi_connector { + int num; + + struct ucsi *ucsi; + struct mutex lock; /* port lock */ + struct work_struct work; + struct completion complete; + + struct typec_port *port; + struct typec_partner *partner; + + struct typec_altmode *port_altmode[UCSI_MAX_ALTMODES]; + struct typec_altmode *partner_altmode[UCSI_MAX_ALTMODES]; + + struct typec_capability typec_cap; + + struct ucsi_connector_status status; + struct ucsi_connector_capability cap; +}; + +int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl, + void *retval, size_t size); + +void ucsi_altmode_update_active(struct ucsi_connector *con); + #endif /* __DRIVER_USB_TYPEC_UCSI_H */
With UCSI the alternate modes, just like everything else related to USB Type-C connectors, are handled in firmware. The operating system can see the status and is allowed to request certain things, for example entering and exiting the modes, but the support for alternate modes is very limited in UCSI. The feature is also optional, which means that even when the platform supports alternate modes, the operating system may not be even made aware of them. UCSI does not support direct VDM reading or writing. Instead, alternate modes can be entered and exited using a single custom command which takes also an optional SVID specific configuration value as parameter. That means every supported alternate mode has to be handled separately in UCSI driver. This commit does not include support for any specific alternate mode. The discovered alternate modes are now registered, but binding a driver to an alternate mode will not be possible until support for that alternate mode is added to the UCSI driver. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/ucsi/trace.c | 12 ++ drivers/usb/typec/ucsi/trace.h | 26 +++ drivers/usb/typec/ucsi/ucsi.c | 351 +++++++++++++++++++++++++++------ drivers/usb/typec/ucsi/ucsi.h | 72 +++++++ 4 files changed, 396 insertions(+), 65 deletions(-)