Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] usb: typec: ucsi: add support for separate DP altmode devices
@ 2019-08-05 18:24 Ajay Gupta
  2019-08-12 14:42 ` Heikki Krogerus
  2019-08-15 15:36 ` Heikki Krogerus
  0 siblings, 2 replies; 3+ messages in thread
From: Ajay Gupta @ 2019-08-05 18:24 UTC (permalink / raw)
  To: heikki.krogerus; +Cc: linux-usb, Ajay Gupta

From: Ajay Gupta <ajayg@nvidia.com>

CCGx controller used on NVIDIA GPU card has two separate display
altmode for two DP pin assignments. UCSI specification doesn't
prohibits using separate display altmode.

Current UCSI Type-C framework expects only one display altmode for
all DP pin assignment. This patch squashes two separate display
altmode into single altmode to support controllers with separate
display altmode. We first read all the alternate modes of connector
and then run through it to know if there are separate display
altmodes. If so, it prepares a new port altmode set after squashing
two or more separate altmodes into one.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
Original discussion on this issue is at [1]

Change from v1->v2
	- Fix ucsi->ppm NULL check in ucsi_sync based on
	comment from an automated email from someone (I lost the email).

1. https://marc.info/?l=linux-usb&m=154905866830998&w=2

 drivers/usb/typec/ucsi/ucsi.c | 212 ++++++++++++++++++++++++++++++++--
 drivers/usb/typec/ucsi/ucsi.h |  12 ++
 2 files changed, 217 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ba288b964dc8..68ea66fcaa0e 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -39,11 +39,37 @@
  */
 #define UCSI_SWAP_TIMEOUT_MS	5000
 
+static void ucsi_update_get_current_cam_cmd(struct ucsi_connector *con,
+					    struct ucsi_data *data)
+{
+	u8 cam, new_cam;
+
+	if (data->cci.data_length == 0x1) {
+		cam = data->message_in[0];
+		new_cam = con->port_alt[cam].linked_idx;
+		data->message_in[0] = new_cam;
+		con->new_port_alt[new_cam].active_idx = cam;
+	}
+}
+
 static inline int ucsi_sync(struct ucsi *ucsi)
 {
-	if (ucsi->ppm && ucsi->ppm->sync)
-		return ucsi->ppm->sync(ucsi->ppm);
-	return 0;
+	struct ucsi_connector *con = ucsi->connector;
+	struct ucsi_data *data;
+	int ret = 0;
+
+	if (ucsi->ppm && ucsi->ppm->sync) {
+		ret = ucsi->ppm->sync(ucsi->ppm);
+		if (ret)
+			return ret;
+
+		data = ucsi->ppm->data;
+		if (data->ctrl.alt.cmd == UCSI_GET_CURRENT_CAM &&
+		    con->has_multiple_dp)
+			ucsi_update_get_current_cam_cmd(con, data);
+	}
+
+	return ret;
 }
 
 static int ucsi_command(struct ucsi *ucsi, struct ucsi_control *ctrl)
@@ -101,14 +127,65 @@ static int ucsi_ack(struct ucsi *ucsi, u8 ack)
 	return ret;
 }
 
+static void ucsi_update_set_new_cam_cmd(struct ucsi_connector *con,
+					struct ucsi_control *ctrl)
+{
+	struct new_ucsi_altmode *new_port, *port;
+	struct typec_altmode *alt = NULL;
+	u64 cmd;
+	u8 new_cam, cam, pin;
+	bool enter_new_mode;
+	int i, j, k = 0xff;
+
+	cmd = ctrl->raw_cmd;
+	new_cam = (cmd >> 24) & 0xff;
+	new_port = &con->new_port_alt[new_cam];
+	cam = new_port->linked_idx;
+	enter_new_mode = (cmd >> 23) & 1;
+
+	if (cam == UCSI_MULTI_LINKED_INDEX) {
+		if (enter_new_mode) {
+			port = con->port_alt;
+			for (i = 0; con->partner_altmode[i]; i++) {
+				alt = con->partner_altmode[i];
+				if (alt->svid == new_port->svid)
+					break;
+			}
+			for (j = 0; port[j].svid; j++) {
+				pin = DP_CONF_GET_PIN_ASSIGN(port[j].mid);
+				if (alt && port[j].svid == alt->svid &&
+				    (pin & DP_CONF_GET_PIN_ASSIGN(alt->vdo))) {
+					/* prioritize pin E->D->C */
+					if (k == 0xff || (k != 0xff && pin >
+					    DP_CONF_GET_PIN_ASSIGN(port[k].mid))
+					    ) {
+						k = j;
+					}
+				}
+			}
+			cam = k;
+			new_port->active_idx = cam;
+		} else {
+			cam = new_port->active_idx;
+		}
+	}
+	cmd &= ~(0xff << 24);
+	cmd |= (cam << 24);
+	ctrl->raw_cmd = cmd;
+}
+
 static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 			    void *data, size_t size)
 {
 	struct ucsi_control _ctrl;
+	struct ucsi_connector *con = ucsi->connector;
 	u8 data_length;
 	u16 error;
 	int ret;
 
+	if (ctrl->alt.cmd == UCSI_SET_NEW_CAM && con->has_multiple_dp)
+		ucsi_update_set_new_cam_cmd(con, ctrl);
+
 	ret = ucsi_command(ucsi, ctrl);
 	if (ret)
 		goto err;
@@ -364,10 +441,24 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 
 	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;
+
+		if (recipient == UCSI_RECIPIENT_CON) {
+			if (con->has_multiple_dp) {
+				alt[0].svid = con->new_port_alt[i].svid;
+				alt[0].mid = con->new_port_alt[i].mid;
+			} else {
+				alt[0].svid = con->port_alt[i].svid;
+				alt[0].mid = con->port_alt[i].mid;
+			}
+			len = sizeof(alt[0]);
+		} else {
+			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
@@ -521,6 +612,103 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 		ucsi_altmode_update_active(con);
 }
 
+static void ucsi_update_con_altmodes(struct ucsi_connector *con)
+{
+	int max_altmodes = con->ucsi->cap.num_alt_modes;
+	struct new_ucsi_altmode *alt, *new_alt;
+	int i, j, k = 0;
+	bool found = false;
+
+	alt = con->port_alt;
+	new_alt = con->new_port_alt;
+	memset(con->new_port_alt, 0, sizeof(con->new_port_alt));
+
+	for (i = 0; i < max_altmodes; i++) {
+		if (!alt[i].svid)
+			break;
+
+		/* already checked and considered */
+		if (alt[i].checked)
+			continue;
+
+		if (!DP_CONF_GET_PIN_ASSIGN(alt[i].mid)) {
+			/* Found Non DP altmode */
+			new_alt[k].svid = alt[i].svid;
+			new_alt[k].mid |= alt[i].mid;
+			new_alt[k].linked_idx = i;
+			alt[i].linked_idx = k;
+			k++;
+			continue;
+		}
+
+		for (j = i + 1; j < max_altmodes; j++) {
+			if (alt[i].svid != alt[j].svid ||
+			    !DP_CONF_GET_PIN_ASSIGN(alt[j].mid)) {
+				continue;
+			} else {
+				/* Found duplicate DP mode */
+				new_alt[k].svid = alt[i].svid;
+				new_alt[k].mid |= alt[i].mid | alt[j].mid;
+				new_alt[k].linked_idx = UCSI_MULTI_LINKED_INDEX;
+				alt[i].linked_idx = k;
+				alt[j].linked_idx = k;
+				alt[j].checked = true;
+				found = true;
+			}
+		}
+		if (found) {
+			con->has_multiple_dp = true;
+		} else {
+			/* Didn't find any duplicate DP altmode */
+			new_alt[k].svid = alt[i].svid;
+			new_alt[k].mid |= alt[i].mid;
+			new_alt[k].linked_idx = i;
+			alt[i].linked_idx = k;
+		}
+		k++;
+	}
+}
+
+static int ucsi_get_altmodes(struct ucsi_connector *con)
+{
+	int max_altmodes = con->ucsi->cap.num_alt_modes;
+	struct ucsi_altmode alt[2];
+	struct ucsi_control ctrl;
+	int num = 1;
+	int len;
+	int j;
+	int i;
+	int k = 0;
+
+	memset(con->port_alt, 0, sizeof(con->port_alt));
+
+	for (i = 0; i < max_altmodes;) {
+		memset(alt, 0, sizeof(alt));
+		UCSI_CMD_GET_ALTERNATE_MODES(ctrl, UCSI_RECIPIENT_CON,
+					     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.
+		 */
+		num = len / sizeof(alt[0]);
+		i += num;
+
+		for (j = 0; j < num; j++) {
+			if (!alt[j].svid)
+				return 0;
+
+			con->port_alt[k].mid = alt[j].mid;
+			con->port_alt[k].svid = alt[j].svid;
+			k++;
+		}
+	}
+	return 0;
+}
+
 static void ucsi_connector_change(struct work_struct *work)
 {
 	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
@@ -851,6 +1039,16 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	if (IS_ERR(con->port))
 		return PTR_ERR(con->port);
 
+	/* Get Alternate modes */
+	ret = ucsi_get_altmodes(con);
+	if (ret) {
+		dev_err(ucsi->dev,
+			"%s: con%d failed (%d)to get port alternate modes\n",
+			__func__, con->num, ret);
+		return 0;
+	}
+	ucsi_update_con_altmodes(con);
+
 	/* Alternate modes */
 	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
 	if (ret)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index de87d0b8319d..7bbdf83c8d4a 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -274,6 +274,15 @@ struct ucsi_connector_capability {
 	u8:6; /* reserved */
 } __packed;
 
+struct new_ucsi_altmode {
+	u16 svid;
+	u32 mid;
+	u8 linked_idx;
+	u8 active_idx;
+#define UCSI_MULTI_LINKED_INDEX	(0xff)
+	bool checked;
+} __packed;
+
 struct ucsi_altmode {
 	u16 svid;
 	u32 mid;
@@ -408,6 +417,7 @@ struct ucsi {
 
 struct ucsi_connector {
 	int num;
+	bool has_multiple_dp;
 
 	struct ucsi *ucsi;
 	struct mutex lock; /* port lock */
@@ -424,6 +434,8 @@ struct ucsi_connector {
 
 	struct ucsi_connector_status status;
 	struct ucsi_connector_capability cap;
+	struct new_ucsi_altmode port_alt[UCSI_MAX_ALTMODES];
+	struct new_ucsi_altmode new_port_alt[UCSI_MAX_ALTMODES];
 };
 
 int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] usb: typec: ucsi: add support for separate DP altmode devices
  2019-08-05 18:24 [PATCH v2] usb: typec: ucsi: add support for separate DP altmode devices Ajay Gupta
@ 2019-08-12 14:42 ` Heikki Krogerus
  2019-08-15 15:36 ` Heikki Krogerus
  1 sibling, 0 replies; 3+ messages in thread
From: Heikki Krogerus @ 2019-08-12 14:42 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb, Ajay Gupta

Hi Ajay,

On Mon, Aug 05, 2019 at 11:24:13AM -0700, Ajay Gupta wrote:
> From: Ajay Gupta <ajayg@nvidia.com>
> 
> CCGx controller used on NVIDIA GPU card has two separate display
> altmode for two DP pin assignments. UCSI specification doesn't
> prohibits using separate display altmode.
> 
> Current UCSI Type-C framework expects only one display altmode for
> all DP pin assignment. This patch squashes two separate display
> altmode into single altmode to support controllers with separate
> display altmode. We first read all the alternate modes of connector
> and then run through it to know if there are separate display
> altmodes. If so, it prepares a new port altmode set after squashing
> two or more separate altmodes into one.

I finally have time to go over this patch. Squashing those two
connector modes into one makes sense to me, but I'm not completely
happy about the quality of the code (although the entire driver has
always been a bit messy). The implementation also feels somehow too
integrated into the core UCSI driver. Give me a few days to think
about improvements. If I don't come up with anything, we'll go forward
with this as is.

> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> ---
> Original discussion on this issue is at [1]
> 
> Change from v1->v2
> 	- Fix ucsi->ppm NULL check in ucsi_sync based on
> 	comment from an automated email from someone (I lost the email).
> 
> 1. https://marc.info/?l=linux-usb&m=154905866830998&w=2
> 
>  drivers/usb/typec/ucsi/ucsi.c | 212 ++++++++++++++++++++++++++++++++--
>  drivers/usb/typec/ucsi/ucsi.h |  12 ++
>  2 files changed, 217 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index ba288b964dc8..68ea66fcaa0e 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -39,11 +39,37 @@
>   */
>  #define UCSI_SWAP_TIMEOUT_MS	5000
>  
> +static void ucsi_update_get_current_cam_cmd(struct ucsi_connector *con,
> +					    struct ucsi_data *data)
> +{
> +	u8 cam, new_cam;
> +
> +	if (data->cci.data_length == 0x1) {
> +		cam = data->message_in[0];
> +		new_cam = con->port_alt[cam].linked_idx;
> +		data->message_in[0] = new_cam;
> +		con->new_port_alt[new_cam].active_idx = cam;
> +	}
> +}
> +
>  static inline int ucsi_sync(struct ucsi *ucsi)
>  {
> -	if (ucsi->ppm && ucsi->ppm->sync)
> -		return ucsi->ppm->sync(ucsi->ppm);
> -	return 0;
> +	struct ucsi_connector *con = ucsi->connector;
> +	struct ucsi_data *data;
> +	int ret = 0;
> +
> +	if (ucsi->ppm && ucsi->ppm->sync) {
> +		ret = ucsi->ppm->sync(ucsi->ppm);
> +		if (ret)
> +			return ret;
> +
> +		data = ucsi->ppm->data;
> +		if (data->ctrl.alt.cmd == UCSI_GET_CURRENT_CAM &&
> +		    con->has_multiple_dp)
> +			ucsi_update_get_current_cam_cmd(con, data);
> +	}
> +
> +	return ret;
>  }
>  
>  static int ucsi_command(struct ucsi *ucsi, struct ucsi_control *ctrl)
> @@ -101,14 +127,65 @@ static int ucsi_ack(struct ucsi *ucsi, u8 ack)
>  	return ret;
>  }
>  
> +static void ucsi_update_set_new_cam_cmd(struct ucsi_connector *con,
> +					struct ucsi_control *ctrl)
> +{
> +	struct new_ucsi_altmode *new_port, *port;
> +	struct typec_altmode *alt = NULL;
> +	u64 cmd;
> +	u8 new_cam, cam, pin;
> +	bool enter_new_mode;
> +	int i, j, k = 0xff;
> +
> +	cmd = ctrl->raw_cmd;
> +	new_cam = (cmd >> 24) & 0xff;
> +	new_port = &con->new_port_alt[new_cam];
> +	cam = new_port->linked_idx;
> +	enter_new_mode = (cmd >> 23) & 1;
> +
> +	if (cam == UCSI_MULTI_LINKED_INDEX) {
> +		if (enter_new_mode) {
> +			port = con->port_alt;
> +			for (i = 0; con->partner_altmode[i]; i++) {
> +				alt = con->partner_altmode[i];
> +				if (alt->svid == new_port->svid)
> +					break;
> +			}
> +			for (j = 0; port[j].svid; j++) {
> +				pin = DP_CONF_GET_PIN_ASSIGN(port[j].mid);
> +				if (alt && port[j].svid == alt->svid &&
> +				    (pin & DP_CONF_GET_PIN_ASSIGN(alt->vdo))) {
> +					/* prioritize pin E->D->C */
> +					if (k == 0xff || (k != 0xff && pin >
> +					    DP_CONF_GET_PIN_ASSIGN(port[k].mid))
> +					    ) {
> +						k = j;
> +					}
> +				}
> +			}
> +			cam = k;
> +			new_port->active_idx = cam;
> +		} else {
> +			cam = new_port->active_idx;
> +		}
> +	}
> +	cmd &= ~(0xff << 24);
> +	cmd |= (cam << 24);
> +	ctrl->raw_cmd = cmd;
> +}
> +
>  static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
>  			    void *data, size_t size)
>  {
>  	struct ucsi_control _ctrl;
> +	struct ucsi_connector *con = ucsi->connector;
>  	u8 data_length;
>  	u16 error;
>  	int ret;
>  
> +	if (ctrl->alt.cmd == UCSI_SET_NEW_CAM && con->has_multiple_dp)
> +		ucsi_update_set_new_cam_cmd(con, ctrl);
> +
>  	ret = ucsi_command(ucsi, ctrl);
>  	if (ret)
>  		goto err;
> @@ -364,10 +441,24 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
>  
>  	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;
> +
> +		if (recipient == UCSI_RECIPIENT_CON) {
> +			if (con->has_multiple_dp) {
> +				alt[0].svid = con->new_port_alt[i].svid;
> +				alt[0].mid = con->new_port_alt[i].mid;
> +			} else {
> +				alt[0].svid = con->port_alt[i].svid;
> +				alt[0].mid = con->port_alt[i].mid;
> +			}
> +			len = sizeof(alt[0]);
> +		} else {
> +			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
> @@ -521,6 +612,103 @@ static void ucsi_partner_change(struct ucsi_connector *con)
>  		ucsi_altmode_update_active(con);
>  }
>  
> +static void ucsi_update_con_altmodes(struct ucsi_connector *con)
> +{
> +	int max_altmodes = con->ucsi->cap.num_alt_modes;
> +	struct new_ucsi_altmode *alt, *new_alt;
> +	int i, j, k = 0;
> +	bool found = false;
> +
> +	alt = con->port_alt;
> +	new_alt = con->new_port_alt;
> +	memset(con->new_port_alt, 0, sizeof(con->new_port_alt));
> +
> +	for (i = 0; i < max_altmodes; i++) {
> +		if (!alt[i].svid)
> +			break;
> +
> +		/* already checked and considered */
> +		if (alt[i].checked)
> +			continue;
> +
> +		if (!DP_CONF_GET_PIN_ASSIGN(alt[i].mid)) {
> +			/* Found Non DP altmode */
> +			new_alt[k].svid = alt[i].svid;
> +			new_alt[k].mid |= alt[i].mid;
> +			new_alt[k].linked_idx = i;
> +			alt[i].linked_idx = k;
> +			k++;
> +			continue;
> +		}
> +
> +		for (j = i + 1; j < max_altmodes; j++) {
> +			if (alt[i].svid != alt[j].svid ||
> +			    !DP_CONF_GET_PIN_ASSIGN(alt[j].mid)) {
> +				continue;
> +			} else {
> +				/* Found duplicate DP mode */
> +				new_alt[k].svid = alt[i].svid;
> +				new_alt[k].mid |= alt[i].mid | alt[j].mid;
> +				new_alt[k].linked_idx = UCSI_MULTI_LINKED_INDEX;
> +				alt[i].linked_idx = k;
> +				alt[j].linked_idx = k;
> +				alt[j].checked = true;
> +				found = true;
> +			}
> +		}
> +		if (found) {
> +			con->has_multiple_dp = true;
> +		} else {
> +			/* Didn't find any duplicate DP altmode */
> +			new_alt[k].svid = alt[i].svid;
> +			new_alt[k].mid |= alt[i].mid;
> +			new_alt[k].linked_idx = i;
> +			alt[i].linked_idx = k;
> +		}
> +		k++;
> +	}
> +}
> +
> +static int ucsi_get_altmodes(struct ucsi_connector *con)
> +{
> +	int max_altmodes = con->ucsi->cap.num_alt_modes;
> +	struct ucsi_altmode alt[2];
> +	struct ucsi_control ctrl;
> +	int num = 1;
> +	int len;
> +	int j;
> +	int i;
> +	int k = 0;
> +
> +	memset(con->port_alt, 0, sizeof(con->port_alt));
> +
> +	for (i = 0; i < max_altmodes;) {
> +		memset(alt, 0, sizeof(alt));
> +		UCSI_CMD_GET_ALTERNATE_MODES(ctrl, UCSI_RECIPIENT_CON,
> +					     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.
> +		 */
> +		num = len / sizeof(alt[0]);
> +		i += num;
> +
> +		for (j = 0; j < num; j++) {
> +			if (!alt[j].svid)
> +				return 0;
> +
> +			con->port_alt[k].mid = alt[j].mid;
> +			con->port_alt[k].svid = alt[j].svid;
> +			k++;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static void ucsi_connector_change(struct work_struct *work)
>  {
>  	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
> @@ -851,6 +1039,16 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	if (IS_ERR(con->port))
>  		return PTR_ERR(con->port);
>  
> +	/* Get Alternate modes */
> +	ret = ucsi_get_altmodes(con);
> +	if (ret) {
> +		dev_err(ucsi->dev,
> +			"%s: con%d failed (%d)to get port alternate modes\n",
> +			__func__, con->num, ret);
> +		return 0;
> +	}
> +	ucsi_update_con_altmodes(con);
> +
>  	/* Alternate modes */
>  	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
>  	if (ret)
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index de87d0b8319d..7bbdf83c8d4a 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -274,6 +274,15 @@ struct ucsi_connector_capability {
>  	u8:6; /* reserved */
>  } __packed;
>  
> +struct new_ucsi_altmode {
> +	u16 svid;
> +	u32 mid;
> +	u8 linked_idx;
> +	u8 active_idx;
> +#define UCSI_MULTI_LINKED_INDEX	(0xff)
> +	bool checked;
> +} __packed;
> +
>  struct ucsi_altmode {
>  	u16 svid;
>  	u32 mid;
> @@ -408,6 +417,7 @@ struct ucsi {
>  
>  struct ucsi_connector {
>  	int num;
> +	bool has_multiple_dp;
>  
>  	struct ucsi *ucsi;
>  	struct mutex lock; /* port lock */
> @@ -424,6 +434,8 @@ struct ucsi_connector {
>  
>  	struct ucsi_connector_status status;
>  	struct ucsi_connector_capability cap;
> +	struct new_ucsi_altmode port_alt[UCSI_MAX_ALTMODES];
> +	struct new_ucsi_altmode new_port_alt[UCSI_MAX_ALTMODES];
>  };
>  
>  int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
> -- 
> 2.17.1

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] usb: typec: ucsi: add support for separate DP altmode devices
  2019-08-05 18:24 [PATCH v2] usb: typec: ucsi: add support for separate DP altmode devices Ajay Gupta
  2019-08-12 14:42 ` Heikki Krogerus
@ 2019-08-15 15:36 ` Heikki Krogerus
  1 sibling, 0 replies; 3+ messages in thread
From: Heikki Krogerus @ 2019-08-15 15:36 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb, Ajay Gupta

Hi Ajay,

I'm really sorry for being so late with the review.

On Mon, Aug 05, 2019 at 11:24:13AM -0700, Ajay Gupta wrote:
> From: Ajay Gupta <ajayg@nvidia.com>
> 
> CCGx controller used on NVIDIA GPU card has two separate display
> altmode for two DP pin assignments. UCSI specification doesn't
> prohibits using separate display altmode.
> 
> Current UCSI Type-C framework expects only one display altmode for
> all DP pin assignment. This patch squashes two separate display
> altmode into single altmode to support controllers with separate
> display altmode. We first read all the alternate modes of connector
> and then run through it to know if there are separate display
> altmodes. If so, it prepares a new port altmode set after squashing
> two or more separate altmodes into one.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> ---
> Original discussion on this issue is at [1]
> 
> Change from v1->v2
> 	- Fix ucsi->ppm NULL check in ucsi_sync based on
> 	comment from an automated email from someone (I lost the email).
> 
> 1. https://marc.info/?l=linux-usb&m=154905866830998&w=2
> 
>  drivers/usb/typec/ucsi/ucsi.c | 212 ++++++++++++++++++++++++++++++++--
>  drivers/usb/typec/ucsi/ucsi.h |  12 ++
>  2 files changed, 217 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index ba288b964dc8..68ea66fcaa0e 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -39,11 +39,37 @@
>   */
>  #define UCSI_SWAP_TIMEOUT_MS	5000
>  
> +static void ucsi_update_get_current_cam_cmd(struct ucsi_connector *con,
> +					    struct ucsi_data *data)
> +{
> +	u8 cam, new_cam;
> +
> +	if (data->cci.data_length == 0x1) {
> +		cam = data->message_in[0];
> +		new_cam = con->port_alt[cam].linked_idx;
> +		data->message_in[0] = new_cam;
> +		con->new_port_alt[new_cam].active_idx = cam;
> +	}
> +}
> +
>  static inline int ucsi_sync(struct ucsi *ucsi)
>  {
> -	if (ucsi->ppm && ucsi->ppm->sync)
> -		return ucsi->ppm->sync(ucsi->ppm);
> -	return 0;
> +	struct ucsi_connector *con = ucsi->connector;
> +	struct ucsi_data *data;
> +	int ret = 0;
> +
> +	if (ucsi->ppm && ucsi->ppm->sync) {
> +		ret = ucsi->ppm->sync(ucsi->ppm);
> +		if (ret)
> +			return ret;
> +
> +		data = ucsi->ppm->data;
> +		if (data->ctrl.alt.cmd == UCSI_GET_CURRENT_CAM &&
> +		    con->has_multiple_dp)
> +			ucsi_update_get_current_cam_cmd(con, data);

Since you are capturing commands, then couldn't this be initially
handled in ucsi_ccg.c?

> +	}
> +
> +	return ret;
>  }
>  
>  static int ucsi_command(struct ucsi *ucsi, struct ucsi_control *ctrl)
> @@ -101,14 +127,65 @@ static int ucsi_ack(struct ucsi *ucsi, u8 ack)
>  	return ret;
>  }
>  
> +static void ucsi_update_set_new_cam_cmd(struct ucsi_connector *con,
> +					struct ucsi_control *ctrl)
> +{
> +	struct new_ucsi_altmode *new_port, *port;
> +	struct typec_altmode *alt = NULL;
> +	u64 cmd;
> +	u8 new_cam, cam, pin;
> +	bool enter_new_mode;
> +	int i, j, k = 0xff;
> +
> +	cmd = ctrl->raw_cmd;
> +	new_cam = (cmd >> 24) & 0xff;
> +	new_port = &con->new_port_alt[new_cam];
> +	cam = new_port->linked_idx;
> +	enter_new_mode = (cmd >> 23) & 1;
> +
> +	if (cam == UCSI_MULTI_LINKED_INDEX) {
> +		if (enter_new_mode) {
> +			port = con->port_alt;

You could have set that earlier, no?

> +			for (i = 0; con->partner_altmode[i]; i++) {
> +				alt = con->partner_altmode[i];
> +				if (alt->svid == new_port->svid)
> +					break;
> +			}

Why do you enter the next loop even if !alt?

> +			for (j = 0; port[j].svid; j++) {
> +				pin = DP_CONF_GET_PIN_ASSIGN(port[j].mid);
> +				if (alt && port[j].svid == alt->svid &&
> +				    (pin & DP_CONF_GET_PIN_ASSIGN(alt->vdo))) {
> +					/* prioritize pin E->D->C */
> +					if (k == 0xff || (k != 0xff && pin >
> +					    DP_CONF_GET_PIN_ASSIGN(port[k].mid))
> +					    ) {
> +						k = j;
> +					}
> +				}
> +			}

That is really difficult to read, let alone understand. You are making
assumption here that the alt mode is always DP alt?

> +			cam = k;
> +			new_port->active_idx = cam;
> +		} else {
> +			cam = new_port->active_idx;
> +		}
> +	}

You have a lot of nested stuff here. Please see if you can improve
the above code.

> +	cmd &= ~(0xff << 24);

I'm not sure I understand why couldn't the cmd be 0 before this point?
Actually, do you even need that variable for anything?

> +	cmd |= (cam << 24);
> +	ctrl->raw_cmd = cmd;

Couldn't you just update ctrl->raw_cmd directly?

> +}
> +
>  static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
>  			    void *data, size_t size)
>  {
>  	struct ucsi_control _ctrl;
> +	struct ucsi_connector *con = ucsi->connector;
>  	u8 data_length;
>  	u16 error;
>  	int ret;
>  
> +	if (ctrl->alt.cmd == UCSI_SET_NEW_CAM && con->has_multiple_dp)
> +		ucsi_update_set_new_cam_cmd(con, ctrl);
> +
>  	ret = ucsi_command(ucsi, ctrl);
>  	if (ret)
>  		goto err;
> @@ -364,10 +441,24 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
>  
>  	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;
> +
> +		if (recipient == UCSI_RECIPIENT_CON) {
> +			if (con->has_multiple_dp) {
> +				alt[0].svid = con->new_port_alt[i].svid;
> +				alt[0].mid = con->new_port_alt[i].mid;
> +			} else {
> +				alt[0].svid = con->port_alt[i].svid;
> +				alt[0].mid = con->port_alt[i].mid;
> +			}
> +			len = sizeof(alt[0]);
> +		} else {
> +			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
> @@ -521,6 +612,103 @@ static void ucsi_partner_change(struct ucsi_connector *con)
>  		ucsi_altmode_update_active(con);
>  }
>  
> +static void ucsi_update_con_altmodes(struct ucsi_connector *con)
> +{
> +	int max_altmodes = con->ucsi->cap.num_alt_modes;
> +	struct new_ucsi_altmode *alt, *new_alt;
> +	int i, j, k = 0;
> +	bool found = false;
> +
> +	alt = con->port_alt;
> +	new_alt = con->new_port_alt;
> +	memset(con->new_port_alt, 0, sizeof(con->new_port_alt));
> +
> +	for (i = 0; i < max_altmodes; i++) {
> +		if (!alt[i].svid)
> +			break;
> +
> +		/* already checked and considered */
> +		if (alt[i].checked)
> +			continue;
> +
> +		if (!DP_CONF_GET_PIN_ASSIGN(alt[i].mid)) {
> +			/* Found Non DP altmode */
> +			new_alt[k].svid = alt[i].svid;
> +			new_alt[k].mid |= alt[i].mid;
> +			new_alt[k].linked_idx = i;
> +			alt[i].linked_idx = k;
> +			k++;
> +			continue;
> +		}
> +
> +		for (j = i + 1; j < max_altmodes; j++) {
> +			if (alt[i].svid != alt[j].svid ||
> +			    !DP_CONF_GET_PIN_ASSIGN(alt[j].mid)) {
> +				continue;
> +			} else {
> +				/* Found duplicate DP mode */
> +				new_alt[k].svid = alt[i].svid;
> +				new_alt[k].mid |= alt[i].mid | alt[j].mid;
> +				new_alt[k].linked_idx = UCSI_MULTI_LINKED_INDEX;
> +				alt[i].linked_idx = k;
> +				alt[j].linked_idx = k;
> +				alt[j].checked = true;
> +				found = true;
> +			}
> +		}
> +		if (found) {
> +			con->has_multiple_dp = true;
> +		} else {
> +			/* Didn't find any duplicate DP altmode */
> +			new_alt[k].svid = alt[i].svid;
> +			new_alt[k].mid |= alt[i].mid;
> +			new_alt[k].linked_idx = i;
> +			alt[i].linked_idx = k;
> +		}
> +		k++;
> +	}
> +}
> +
> +static int ucsi_get_altmodes(struct ucsi_connector *con)
> +{
> +	int max_altmodes = con->ucsi->cap.num_alt_modes;
> +	struct ucsi_altmode alt[2];
> +	struct ucsi_control ctrl;
> +	int num = 1;
> +	int len;
> +	int j;
> +	int i;
> +	int k = 0;
> +
> +	memset(con->port_alt, 0, sizeof(con->port_alt));
> +
> +	for (i = 0; i < max_altmodes;) {
> +		memset(alt, 0, sizeof(alt));
> +		UCSI_CMD_GET_ALTERNATE_MODES(ctrl, UCSI_RECIPIENT_CON,
> +					     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.
> +		 */
> +		num = len / sizeof(alt[0]);
> +		i += num;
> +
> +		for (j = 0; j < num; j++) {
> +			if (!alt[j].svid)
> +				return 0;
> +
> +			con->port_alt[k].mid = alt[j].mid;
> +			con->port_alt[k].svid = alt[j].svid;
> +			k++;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static void ucsi_connector_change(struct work_struct *work)
>  {
>  	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
> @@ -851,6 +1039,16 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	if (IS_ERR(con->port))
>  		return PTR_ERR(con->port);
>  
> +	/* Get Alternate modes */
> +	ret = ucsi_get_altmodes(con);
> +	if (ret) {
> +		dev_err(ucsi->dev,
> +			"%s: con%d failed (%d)to get port alternate modes\n",
> +			__func__, con->num, ret);
> +		return 0;
> +	}
> +	ucsi_update_con_altmodes(con);

Instead of first collecting all the connector alt modes, and then
processing them, why not just process every connector alt mode right
after getting it?

>  	/* Alternate modes */
>  	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
>  	if (ret)
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index de87d0b8319d..7bbdf83c8d4a 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -274,6 +274,15 @@ struct ucsi_connector_capability {
>  	u8:6; /* reserved */
>  } __packed;
>  
> +struct new_ucsi_altmode {
> +	u16 svid;
> +	u32 mid;
> +	u8 linked_idx;
> +	u8 active_idx;
> +#define UCSI_MULTI_LINKED_INDEX	(0xff)
> +	bool checked;
> +} __packed;

The name "new_ucsi_altmode" is a not clear to me.

Why don't you just have a pointer to struct ucsi_altmode in that
structure? I'm not sure you really need those linked_idx, active_idx
and checked members.

> +
>  struct ucsi_altmode {
>  	u16 svid;
>  	u32 mid;
> @@ -408,6 +417,7 @@ struct ucsi {
>  
>  struct ucsi_connector {
>  	int num;
> +	bool has_multiple_dp;
>  
>  	struct ucsi *ucsi;
>  	struct mutex lock; /* port lock */
> @@ -424,6 +434,8 @@ struct ucsi_connector {
>  
>  	struct ucsi_connector_status status;
>  	struct ucsi_connector_capability cap;
> +	struct new_ucsi_altmode port_alt[UCSI_MAX_ALTMODES];
> +	struct new_ucsi_altmode new_port_alt[UCSI_MAX_ALTMODES];

I'm pretty sure you don't need two of those. You should be able to
handle this with a pointer to the correct port_altmode member inside
your structure. You can also add a member to your structure that is
pointer to another structure of the same type:

        struct my_ucsi_altmode {
                u16 svid;
                u32 mid;
                struct ucsi_altmode *port_altmode;
                struct my_ucsi_altmode *companion[UCSI_MAX_ALTMODES];
        };

But I pretty sure pointer to the correct port_altmode is enough:

        struct my_ucsi_altmode {
                u16 svid;
                u32 mid;
                struct ucsi_altmode *port_altmode;
        };

>  };

I don't think there is anything preventing all this from being done in
ucsi_ccg.c initially. I don't think we need to touch ucsi.h nor ucsi.c
at all at this point.

So just collect the connector alternate modes for example into struct
ucsi_ccg instead of struct ucsi_connector, and then process the
commands that need to be handled separately in ucsi_ccg_cmd().


thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-05 18:24 [PATCH v2] usb: typec: ucsi: add support for separate DP altmode devices Ajay Gupta
2019-08-12 14:42 ` Heikki Krogerus
2019-08-15 15:36 ` Heikki Krogerus

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox