| Message ID | 20200507150900.12102-3-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | usb: typec: Intel PMC driver changes | expand |
Hi Heikki, Thanks for the patches. On Thu, May 07, 2020 at 06:08:58PM +0300, Heikki Krogerus wrote: > The SBU and HSL orientation may be fixed/static from the mux > PoW. Apparently the retimer may take care of the orientation > of these lines. Handling the static SBU (AUX) and HSL > orientation with device properties. > > If the SBU orientation is static, a device property > "sbu-orintation" can be used. When the property exists, the > driver always sets the SBU orientation according to the > property value, and when it's not set, the driver uses the > cable plug orientation with SBU. > > And with static HSL orientation, "hsl-orientation" device > property can be used in the same way. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/mux/intel_pmc_mux.c | 42 +++++++++++++++++++++++---- > 1 file changed, 36 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c > index f5c5e0aef66f..1aac218099f3 100644 > --- a/drivers/usb/typec/mux/intel_pmc_mux.c > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c > @@ -91,6 +91,9 @@ struct pmc_usb_port { > > u8 usb2_port; > u8 usb3_port; > + > + enum typec_orientation sbu_orientation; > + enum typec_orientation hsl_orientation; > }; > > struct pmc_usb { > @@ -99,6 +102,22 @@ struct pmc_usb { > struct pmc_usb_port *port; > }; > > +static int sbu_orientation(struct pmc_usb_port *port) > +{ > + if (port->sbu_orientation) > + return port->sbu_orientation - 1; > + > + return port->orientation - 1; > +} > + > +static int hsl_orientation(struct pmc_usb_port *port) > +{ > + if (port->hsl_orientation) > + return port->hsl_orientation - 1; > + > + return port->orientation - 1; > +} > + > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > { > u8 response[4]; > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > + > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; I'm curious to know what would happen when sbu-orientation == "normal". That means |port->sbu_orientation| == 1. It sounds like what should happen is the AUX_SHIFT orientation setting should follow what |port->orientation| is, but here it looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning it should be set to 1 ? Apologies if I misunderstood the code... Best regards, > + req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > req.mode_data |= (state->mode - TYPEC_STATE_MODAL) << > PMC_USB_ALTMODE_DP_MODE_SHIFT; > @@ -173,8 +193,9 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state) > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > + > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > + req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > if (TBT_ADAPTER(data->device_mode) == TBT_ADAPTER_TBT3) > req.mode_data |= PMC_USB_ALTMODE_TBT_TYPE; > @@ -211,8 +232,8 @@ static int pmc_usb_connect(struct pmc_usb_port *port) > msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT; > > msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT; > - msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_HSL_SHIFT; > - msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_AUX_SHIFT; > + msg[1] |= hsl_orientation(port) << PMC_USB_MSG_ORI_HSL_SHIFT; > + msg[1] |= sbu_orientation(port) << PMC_USB_MSG_ORI_AUX_SHIFT; > > return pmc_usb_command(port, msg, sizeof(msg)); > } > @@ -296,6 +317,7 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index, > struct usb_role_switch_desc desc = { }; > struct typec_switch_desc sw_desc = { }; > struct typec_mux_desc mux_desc = { }; > + const char *str; > int ret; > > ret = fwnode_property_read_u8(fwnode, "usb2-port", &port->usb2_port); > @@ -306,6 +328,14 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index, > if (ret) > return ret; > > + ret = fwnode_property_read_string(fwnode, "sbu-orientation", &str); > + if (!ret) > + port->sbu_orientation = typec_find_orientation(str); > + > + ret = fwnode_property_read_string(fwnode, "hsl-orientation", &str); > + if (!ret) > + port->hsl_orientation = typec_find_orientation(str); > + > port->num = index; > port->pmc = pmc; > > -- > 2.26.2 >
Hi Prashant, On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote: > > +static int sbu_orientation(struct pmc_usb_port *port) > > +{ > > + if (port->sbu_orientation) > > + return port->sbu_orientation - 1; > > + > > + return port->orientation - 1; > > +} > > + > > +static int hsl_orientation(struct pmc_usb_port *port) > > +{ > > + if (port->hsl_orientation) > > + return port->hsl_orientation - 1; > > + > > + return port->orientation - 1; > > +} > > + > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > > { > > u8 response[4]; > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > + > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > I'm curious to know what would happen when sbu-orientation == "normal". > That means |port->sbu_orientation| == 1. > > It sounds like what should happen is the AUX_SHIFT orientation > setting should follow what |port->orientation| is, but here it > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning > it should be set to 1 ? I'll double check this, and get back to you.. Thanks a lot for reviewing this. If you guys have time, then please check also that the documentation I'm proposing in patch 3/4 for this driver has everything explained clearly enough, and nothing is missing. Br,
On Fri, May 08, 2020 at 02:18:40PM +0300, Heikki Krogerus wrote: > Hi Prashant, > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote: > > > +static int sbu_orientation(struct pmc_usb_port *port) > > > +{ > > > + if (port->sbu_orientation) > > > + return port->sbu_orientation - 1; > > > + > > > + return port->orientation - 1; > > > +} > > > + > > > +static int hsl_orientation(struct pmc_usb_port *port) > > > +{ > > > + if (port->hsl_orientation) > > > + return port->hsl_orientation - 1; > > > + > > > + return port->orientation - 1; > > > +} > > > + > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > > > { > > > u8 response[4]; > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > > + > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > I'm curious to know what would happen when sbu-orientation == "normal". > > That means |port->sbu_orientation| == 1. > > > > It sounds like what should happen is the AUX_SHIFT orientation > > setting should follow what |port->orientation| is, but here it > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning > > it should be set to 1 ? > > I'll double check this, and get back to you.. > > Thanks a lot for reviewing this. If you guys have time, then please > check also that the documentation I'm proposing in patch 3/4 for this > driver has everything explained clearly enough, and nothing is missing. > Sure thing, we'll take a look. Best, -Prashant > Br, > > -- > heikki
On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote: > Hi Prashant, > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote: > > > +static int sbu_orientation(struct pmc_usb_port *port) > > > +{ > > > + if (port->sbu_orientation) > > > + return port->sbu_orientation - 1; > > > + > > > + return port->orientation - 1; > > > +} > > > + > > > +static int hsl_orientation(struct pmc_usb_port *port) > > > +{ > > > + if (port->hsl_orientation) > > > + return port->hsl_orientation - 1; > > > + > > > + return port->orientation - 1; > > > +} > > > + > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > > > { > > > u8 response[4]; > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > > + > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > I'm curious to know what would happen when sbu-orientation == "normal". > > That means |port->sbu_orientation| == 1. > > > > It sounds like what should happen is the AUX_SHIFT orientation > > setting should follow what |port->orientation| is, but here it > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning > > it should be set to 1 ? > > I'll double check this, and get back to you.. This is not exactly an answer to your question, but it seems that those bits are only valid if "Alternate-Direct" message is used. Currently the driver does not support that message. I think the correct thing to do now is to remove the two lines from the driver where those bits (ORI-HSL and ORI-Aux) are set. Let me know if that's OK, and I'll update the series. thanks,
Hi Heikki, Thanks a lot for looking into this. Kindly see my response inline: On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote: > On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote: > > Hi Prashant, > > > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote: > > > > +static int sbu_orientation(struct pmc_usb_port *port) > > > > +{ > > > > + if (port->sbu_orientation) > > > > + return port->sbu_orientation - 1; > > > > + > > > > + return port->orientation - 1; > > > > +} > > > > + > > > > +static int hsl_orientation(struct pmc_usb_port *port) > > > > +{ > > > > + if (port->hsl_orientation) > > > > + return port->hsl_orientation - 1; > > > > + > > > > + return port->orientation - 1; > > > > +} > > > > + > > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > > > > { > > > > u8 response[4]; > > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > > > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > > > + > > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > > > I'm curious to know what would happen when sbu-orientation == "normal". > > > That means |port->sbu_orientation| == 1. > > > > > > It sounds like what should happen is the AUX_SHIFT orientation > > > setting should follow what |port->orientation| is, but here it > > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, > > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning > > > it should be set to 1 ? > > > > I'll double check this, and get back to you.. > > This is not exactly an answer to your question, but it seems that > those bits are only valid if "Alternate-Direct" message is used. > Currently the driver does not support that message. Could you kindly provide some detail on when "Alternate-Direct" would be preferred to the current method? Also, is there anything on the PMC side which is preventing the use of "Alternate-Direct" messages? It seems like the state transition diagram there would be simpler, although I'm likely missing significant details here. > > I think the correct thing to do now is to remove the two lines from > the driver where those bits (ORI-HSL and ORI-Aux) are set. I see. How would orientation then be handled in a retimer configuration where AUX/SBU is flipped by the retimer itself? Best regards, -Prashant > > Let me know if that's OK, and I'll update the series. > > thanks, > > -- > heikki
Hi Prashant, On Mon, May 11, 2020 at 10:57:19AM -0700, Prashant Malani wrote: > Hi Heikki, > > Thanks a lot for looking into this. Kindly see my response inline: > > On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote: > > On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote: > > > Hi Prashant, > > > > > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote: > > > > > +static int sbu_orientation(struct pmc_usb_port *port) > > > > > +{ > > > > > + if (port->sbu_orientation) > > > > > + return port->sbu_orientation - 1; > > > > > + > > > > > + return port->orientation - 1; > > > > > +} > > > > > + > > > > > +static int hsl_orientation(struct pmc_usb_port *port) > > > > > +{ > > > > > + if (port->hsl_orientation) > > > > > + return port->hsl_orientation - 1; > > > > > + > > > > > + return port->orientation - 1; > > > > > +} > > > > > + > > > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > > > > > { > > > > > u8 response[4]; > > > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > > > > > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > > > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > > > > + > > > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > > > > > I'm curious to know what would happen when sbu-orientation == "normal". > > > > That means |port->sbu_orientation| == 1. > > > > > > > > It sounds like what should happen is the AUX_SHIFT orientation > > > > setting should follow what |port->orientation| is, but here it > > > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, > > > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning > > > > it should be set to 1 ? > > > > > > I'll double check this, and get back to you.. > > > > This is not exactly an answer to your question, but it seems that > > those bits are only valid if "Alternate-Direct" message is used. > > Currently the driver does not support that message. > Could you kindly provide some detail on when "Alternate-Direct" would be > preferred to the current method? Alternate Mode Direct request is supposed to be used if an alternate mode is entered directly from disconnected state. > Also, is there anything on the PMC side which is preventing the use of > "Alternate-Direct" messages? It seems like the state transition diagram > there would be simpler, although I'm likely missing significant details > here. So we actually should use the "direct" request if we are in disconnected state to enter alt modes if I understood correctly. But otherwise we should use the normal Alternate Mode request and not the Alternate Mode "direct" request. And I'm afraid I don't know why. > > I think the correct thing to do now is to remove the two lines from > > the driver where those bits (ORI-HSL and ORI-Aux) are set. > I see. How would orientation then be handled in a retimer configuration > where AUX/SBU is flipped by the retimer itself? Note that if we send a separate "connection" request first, then we already tell the HSL and SBU orientation as part of the payload of that request. That is why there is no need to tell about the HSL and SBU orientation with the normal Alternate Mode Request. So we have already handled the HSL and SBU orientation by the time this function is called. thanks,
Hi Heikki, On Tue, May 12, 2020 at 05:22:51PM +0300, Heikki Krogerus wrote: > Hi Prashant, > > On Mon, May 11, 2020 at 10:57:19AM -0700, Prashant Malani wrote: > > Hi Heikki, > > > > Thanks a lot for looking into this. Kindly see my response inline: > > > > On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote: > > > On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote: > > > > Hi Prashant, > > > > > > > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote: > > > > > > +static int sbu_orientation(struct pmc_usb_port *port) > > > > > > +{ > > > > > > + if (port->sbu_orientation) > > > > > > + return port->sbu_orientation - 1; > > > > > > + > > > > > > + return port->orientation - 1; > > > > > > +} > > > > > > + > > > > > > +static int hsl_orientation(struct pmc_usb_port *port) > > > > > > +{ > > > > > > + if (port->hsl_orientation) > > > > > > + return port->hsl_orientation - 1; > > > > > > + > > > > > > + return port->orientation - 1; > > > > > > +} > > > > > > + > > > > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > > > > > > { > > > > > > u8 response[4]; > > > > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > > > > > > > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > > > > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > > > > > + > > > > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > > > > > > > I'm curious to know what would happen when sbu-orientation == "normal". > > > > > That means |port->sbu_orientation| == 1. > > > > > > > > > > It sounds like what should happen is the AUX_SHIFT orientation > > > > > setting should follow what |port->orientation| is, but here it > > > > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, > > > > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning > > > > > it should be set to 1 ? > > > > > > > > I'll double check this, and get back to you.. > > > > > > This is not exactly an answer to your question, but it seems that > > > those bits are only valid if "Alternate-Direct" message is used. > > > Currently the driver does not support that message. > > Could you kindly provide some detail on when "Alternate-Direct" would be > > preferred to the current method? > > Alternate Mode Direct request is supposed to be used if an alternate > mode is entered directly from disconnected state. Ack. > > > Also, is there anything on the PMC side which is preventing the use of > > "Alternate-Direct" messages? It seems like the state transition diagram > > there would be simpler, although I'm likely missing significant details > > here. > > So we actually should use the "direct" request if we are in > disconnected state to enter alt modes if I understood correctly. But > otherwise we should use the normal Alternate Mode request and not the > Alternate Mode "direct" request. And I'm afraid I don't know why. SG. > > > > I think the correct thing to do now is to remove the two lines from > > > the driver where those bits (ORI-HSL and ORI-Aux) are set. > > I see. How would orientation then be handled in a retimer configuration > > where AUX/SBU is flipped by the retimer itself? > > Note that if we send a separate "connection" request first, then we > already tell the HSL and SBU orientation as part of the payload of > that request. That is why there is no need to tell about the HSL and > SBU orientation with the normal Alternate Mode Request. > > So we have already handled the HSL and SBU orientation by the time > this function is called. Thanks for the explanation. I assume the HSL and SBU bit setting lines will be removed from pmc_usb_mux_tbt() too? Best regards, > > > thanks, > > -- > heikki
Hi Heikki, On Tue, May 12, 2020 at 12:19 PM Prashant Malani <pmalani@chromium.org> wrote: > > Hi Heikki, > > On Tue, May 12, 2020 at 05:22:51PM +0300, Heikki Krogerus wrote: > > Hi Prashant, > > > > On Mon, May 11, 2020 at 10:57:19AM -0700, Prashant Malani wrote: > > > Hi Heikki, > > > > > > Thanks a lot for looking into this. Kindly see my response inline: > > > > > > On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote: > > > > On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote: > > > > > Hi Prashant, > > > > > > > > > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote: > > > > > > > +static int sbu_orientation(struct pmc_usb_port *port) > > > > > > > +{ > > > > > > > + if (port->sbu_orientation) > > > > > > > + return port->sbu_orientation - 1; > > > > > > > + > > > > > > > + return port->orientation - 1; > > > > > > > +} > > > > > > > + > > > > > > > +static int hsl_orientation(struct pmc_usb_port *port) > > > > > > > +{ > > > > > > > + if (port->hsl_orientation) > > > > > > > + return port->hsl_orientation - 1; > > > > > > > + > > > > > > > + return port->orientation - 1; > > > > > > > +} > > > > > > > + > > > > > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) > > > > > > > { > > > > > > > u8 response[4]; > > > > > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) > > > > > > > > > > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; > > > > > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; > > > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; > > > > > > > + > > > > > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; > > > > > > > > > > > > I'm curious to know what would happen when sbu-orientation == "normal". > > > > > > That means |port->sbu_orientation| == 1. > > > > > > > > > > > > It sounds like what should happen is the AUX_SHIFT orientation > > > > > > setting should follow what |port->orientation| is, but here it > > > > > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0, > > > > > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning > > > > > > it should be set to 1 ? > > > > > > > > > > I'll double check this, and get back to you.. > > > > > > > > This is not exactly an answer to your question, but it seems that > > > > those bits are only valid if "Alternate-Direct" message is used. > > > > Currently the driver does not support that message. > > > Could you kindly provide some detail on when "Alternate-Direct" would be > > > preferred to the current method? > > > > Alternate Mode Direct request is supposed to be used if an alternate > > mode is entered directly from disconnected state. > > Ack. > > > > > Also, is there anything on the PMC side which is preventing the use of > > > "Alternate-Direct" messages? It seems like the state transition diagram > > > there would be simpler, although I'm likely missing significant details > > > here. > > > > So we actually should use the "direct" request if we are in > > disconnected state to enter alt modes if I understood correctly. But > > otherwise we should use the normal Alternate Mode request and not the > > Alternate Mode "direct" request. And I'm afraid I don't know why. > > SG. > > > > > > I think the correct thing to do now is to remove the two lines from > > > > the driver where those bits (ORI-HSL and ORI-Aux) are set. > > > I see. How would orientation then be handled in a retimer configuration > > > where AUX/SBU is flipped by the retimer itself? > > > > Note that if we send a separate "connection" request first, then we > > already tell the HSL and SBU orientation as part of the payload of > > that request. That is why there is no need to tell about the HSL and > > SBU orientation with the normal Alternate Mode Request. > > > > So we have already handled the HSL and SBU orientation by the time > > this function is called. > > Thanks for the explanation. I assume the HSL and SBU bit setting lines > will be removed from pmc_usb_mux_tbt() too? > I just realized, the issue I initially pointed out would apply to the connect message too, i.e I'm not sure if "normal" orientation setting handles the case where port orientation is reversed correctly. Overall, I am not sure that re-using the typec_orientations[] string list is the best option for this use-case. we're looking for "normal" (i.e follows port->orientation) and "fixed" (i.e is always the same orientation, regardless of what port->orientation is), so it is perhaps better to just define a new array just for this driver. > Best regards, > > > > > > thanks, > > > > -- > > heikki
Hi Prashant, > I just realized, the issue I initially pointed out would apply to the > connect message too, i.e I'm not sure if "normal" orientation setting > handles the case where port orientation is reversed correctly. > Overall, I am not sure that re-using the typec_orientations[] string > list is the best option for this use-case. > we're looking for "normal" (i.e follows port->orientation) and "fixed" > (i.e is always the same orientation, regardless of what > port->orientation is), so it is perhaps better to just define a new > array just for this driver. Sorry, I got sidetracked with that Alternate-Direct Request stuff. Let's start over.. The property itself is the indicator that the orientation is fixed for those lines, not its value. If the property exists, we know the orientation is fixed, and if it doesn't exist, we know we need to use the cable plug orientation. So if we only want to use the property as a flag, then it does not need to have any value at all. It would be a boolean property. But we would then always leave the ORI-HSL field with value 0 when the orientation is fixed, and that would rule out the possibility of supporting a platform where we have to use a fixed value of 1 there (fixed-reversed). If we ever needed to support configuration like that, then we would need to add a new property. That scenario may not be relevant on this platform, and it may seem like an unlikely, or even impossible case now, but experience (and the north mux-agent documentation) tells me we should prepare also for that. That is why I use the typec_orientation strings as the values for these properties. Then the fixed-reversed orientation is also covered with the same properties if we ever need to support it. Maybe this code would be better, or more clear in the driver: /* * Returns the value for the HSL-ORI field. */ static int hsl_orientation(struct pmc_usb_port *port) { enum typec_orientation orientation; /* * Is the orientation fixed: * Yes, use the fixed orientation. * No, use cable plug orientation. */ if (port->hsl_orientation) orientation = hsl_orientation; else orientation = port->orientation; switch (orientation) { case TYPEC_ORIENTATION_NORMAL: return 0; case TYPEC_ORIENTATION_REVERSE: return 1; } return -EINVAL; } thanks,
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c index f5c5e0aef66f..1aac218099f3 100644 --- a/drivers/usb/typec/mux/intel_pmc_mux.c +++ b/drivers/usb/typec/mux/intel_pmc_mux.c @@ -91,6 +91,9 @@ struct pmc_usb_port { u8 usb2_port; u8 usb3_port; + + enum typec_orientation sbu_orientation; + enum typec_orientation hsl_orientation; }; struct pmc_usb { @@ -99,6 +102,22 @@ struct pmc_usb { struct pmc_usb_port *port; }; +static int sbu_orientation(struct pmc_usb_port *port) +{ + if (port->sbu_orientation) + return port->sbu_orientation - 1; + + return port->orientation - 1; +} + +static int hsl_orientation(struct pmc_usb_port *port) +{ + if (port->hsl_orientation) + return port->hsl_orientation - 1; + + return port->orientation - 1; +} + static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len) { u8 response[4]; @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state) req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; + + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; + req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; req.mode_data |= (state->mode - TYPEC_STATE_MODAL) << PMC_USB_ALTMODE_DP_MODE_SHIFT; @@ -173,8 +193,9 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state) req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT; req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT; - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; + + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT; + req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT; if (TBT_ADAPTER(data->device_mode) == TBT_ADAPTER_TBT3) req.mode_data |= PMC_USB_ALTMODE_TBT_TYPE; @@ -211,8 +232,8 @@ static int pmc_usb_connect(struct pmc_usb_port *port) msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT; msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT; - msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_HSL_SHIFT; - msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_AUX_SHIFT; + msg[1] |= hsl_orientation(port) << PMC_USB_MSG_ORI_HSL_SHIFT; + msg[1] |= sbu_orientation(port) << PMC_USB_MSG_ORI_AUX_SHIFT; return pmc_usb_command(port, msg, sizeof(msg)); } @@ -296,6 +317,7 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index, struct usb_role_switch_desc desc = { }; struct typec_switch_desc sw_desc = { }; struct typec_mux_desc mux_desc = { }; + const char *str; int ret; ret = fwnode_property_read_u8(fwnode, "usb2-port", &port->usb2_port); @@ -306,6 +328,14 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index, if (ret) return ret; + ret = fwnode_property_read_string(fwnode, "sbu-orientation", &str); + if (!ret) + port->sbu_orientation = typec_find_orientation(str); + + ret = fwnode_property_read_string(fwnode, "hsl-orientation", &str); + if (!ret) + port->hsl_orientation = typec_find_orientation(str); + port->num = index; port->pmc = pmc;
The SBU and HSL orientation may be fixed/static from the mux PoW. Apparently the retimer may take care of the orientation of these lines. Handling the static SBU (AUX) and HSL orientation with device properties. If the SBU orientation is static, a device property "sbu-orintation" can be used. When the property exists, the driver always sets the SBU orientation according to the property value, and when it's not set, the driver uses the cable plug orientation with SBU. And with static HSL orientation, "hsl-orientation" device property can be used in the same way. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/mux/intel_pmc_mux.c | 42 +++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 6 deletions(-)