Hi Heikki > On Mon, Aug 05, 2019 at 11:24:13AM -0700, Ajay Gupta wrote: > > From: Ajay Gupta <ajayg@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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? We can do in ucsi_ccg.c but the idea is to do it for all ucsi controllers. > > + } > > + > > + 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? Yes, we can do that. > > + 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? Since this is for UCSI_SET_NEW_CAM command so there will be atleast one con->partner_altmode[i] with svid matching with new_port->svid and therefore alt will always be non-NULL. I can still add a check after above loop if needed. > > > + 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? There is no assumption for always DP but it will be always DP due to below check at top. + if (cam == UCSI_MULTI_LINKED_INDEX) { UCSI_MULTI_LINKED_INDEX is set only after getting a duplicate DP altmode. I can rename the macro to reflect DP only (if needed). The code is trying to find best connector alt mode (CAM) index to set from among the multiple DP alt mode. It priorities pin E to D to C. For example, NVIDIA port reports below altmodes: svid vdo cam linked_idx ----- ----- ------ ----------------- 0x955 0x1 0 0 0xff01 0x405 (pin C) 1 1 0xff01 0x805 (pin D) 2 1 0x955 0x3 3 2 Then ucsi_altmode_update_active() will squash duplicate altmode 0xff01 into one and create altmode table as listed below. svid vdo cam linked_idx ----- ----- ------ ------------ 0x955 0x1 0 0 0xff01 0xC05 (pin C, D) 1 UCSI_MULTI_LINKED_INDEX 0x955 0x3 2 3 Now when UCSI_SET_NEW_CAM is initiated for 0xff01 and 0xC05 then we need to talk to controller in terms of original altmode table where we have option to either select pin C or pin D. The logic will select pin D here. I will add some comments to this code to make it clear. > > + 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. Ok, will do. > > + cmd &= ~(0xff << 24); > > I'm not sure I understand why couldn't the cmd be 0 before this point? We are trying to fill in the updated CAM based on original altmode table in the command without touching any other bits. > Actually, do you even need that variable for anything? We don't really need cmd variable. I added it to make it simpler. > > + cmd |= (cam << 24); > > + ctrl->raw_cmd = cmd; > > Couldn't you just update ctrl->raw_cmd directly? Yes, will do. > > > +} > > + > > 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? Yes, we can do that way too. I didn't want to add too much logic into ucsi_register_altmodes() for readability. > > /* 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. We need new structure for below reasons: a) linked_idx: To find CAM index from new squashed table to original altmode table and vice versa for non-duplicate altmodes b) active_idx: To store currently active CAM index in original altmode table for duplicate altmodes. For example, whether pin C or pin D is active with DP altmode svid of 0xff01 c) checked: This is helpful during squash logic. Once we find the duplicate DP altmode then we mark that as checked and considered and move on to next altmode in original table in for loop. > > Why don't you just have a pointer to struct ucsi_altmode in that structure? We can do that. > I'm not sure you really need those linked_idx, active_idx and checked members. We need them to handle UCSI_SET_NEW_CAM and UCSI_GET_CURRENT_CAM commands. We need a way to find right indexes from original table to new squashed table and vice versa. > > + > > 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; > }; > > > }; We want to use data structure which is easy to track and store CAM indexes from from original table to new squashed table and vice versa. Having two arrays of structures was easy to map between original and new tables with indexes. I will think more on this to further simplify it. > 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. We need to get all the connector altmodes and then go through it to squash duplicate DP altmodes. This can be done only in ucsi.c where it sends UCSI_CMD_GET_ALTERNATE_MODES command. Currently ucsi.c sends UCSI_CMD_GET_ALTERNATE_MODES command requesting one altmode at a time and then calls ucsi_register_altmode() to register that single altmode. Some PPM may send two altmode and we register both. Even if we change ucsi.c to first collect all altmodes and then register, then also ucsi_ccg.c will not be able to send squashed altmodes since it doesn't know if the connector has a duplicate altmode until it receives all of them. > 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(). See above. We will need to collect all altmodes inside ucsi.c and update it there. Separate handling of commands can be done inside ucsi_ccg.c but I thought it is better to enable this feature for all ucsi controllers and not only ccg. Thanks >nvpublic > > > thanks, > > -- > heikki
