Re: [PATCH 2/4] usb: typec: mux: intel_pmc_mux: Support for static SBU/HSL orientation

[Heikki Krogerus]

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,

-- 
heikki