[2/4] usb: typec: mux: intel_pmc_mux: Support for static SBU/HSL orientation
diff mbox series

Message ID 20200507150900.12102-3-heikki.krogerus@linux.intel.com
State New
Headers show
Series
  • usb: typec: Intel PMC driver changes
Related show

Commit Message

Heikki Krogerus May 7, 2020, 3:08 p.m. UTC
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(-)

Comments

Prashant Malani May 7, 2020, 10:40 p.m. UTC | #1
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
>
Heikki Krogerus May 8, 2020, 11:18 a.m. UTC | #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,
Prashant Malani May 8, 2020, 11:36 a.m. UTC | #3
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
Heikki Krogerus May 11, 2020, 1:32 p.m. UTC | #4
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,
Prashant Malani May 11, 2020, 5:57 p.m. UTC | #5
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
Heikki Krogerus May 12, 2020, 2:22 p.m. UTC | #6
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,
Prashant Malani May 12, 2020, 7:19 p.m. UTC | #7
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
Prashant Malani May 14, 2020, 8:51 p.m. UTC | #8
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
Heikki Krogerus May 15, 2020, 12:05 p.m. UTC | #9
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,

Patch
diff mbox series

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;