[4/5] usb: typec: ucsi: Preliminary support for alternate modes

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi,

On Fri, Feb 01, 2019 at 10:02:19PM +0000, Michael Hsu wrote:
> Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a 1-based
> index, not a 32-bit mode VDO) can cause incorrect matches if the
> GET_ALTERNATE_MODES returns different ordering for recipient=connector and
> recipient=sop for a particular svid.
> 
> For example, UCSI command GET_ALTERNATE_MODES with recipient=connector returns
>     [0] SVID=0xff01, ModeVDO=0x00000405 (Mode = 1)
>     [1] SVID=0xff01, ModeVDO=0x00000805 (Mode = 2)
> And UCSI command GET_ALTERNATE_MODES with recipient=sop returns
>     [0] SVID=0xff01, ModeVDO=0x00000c05 (Mode = 1)
> 
> Then when DP alternate mode with pin D is active, GET_CURRENT_CAM returns
> index 1 (connector alternate mode = 1, i.e. SVID=0xff01, Mode=2,
> ModeVDO=0x00000805).
> But, the function will be unable to match it with partner_alt_mode
> corresponding to (SVID=0xff01,Mode=1).
> 
> Can you compare against 32-bit VDO instead of ->mode?  Also, use '&' bitwise
> AND operator when masking the partner's mode VDO (0x0c05) against the
> connector's mode VDO (0x405 or 0x805) to determine it there is an alternate
> mode match.

No top-posting! From now on inline your comments in your replies. The
"process" guide in kernel should provide more information for you:
https://www.kernel.org/doc/html/latest/process/2.Process.html#mailing-lists

Your PPM is reporting a separate mode for every pin-assignment it
supports. It really should _not_ do that! You need to be able to get
the capabilities for DP alt mode with GET_ALTERNATE_MODE command in
the format that the DP alt mode spec defines, also with the connector.
You are not getting them like that. Now for example in both modes your
connector can only act as DisplayPort sink (i.e. a display) which I'm
pretty sure is not the case.

With the current version of the DP alt mode spec we do not ever need
more than one mode for DP. That mode needs to show all the supported
pin assignments the device, partner or connector, supports. That is
exactly how all the other UCSI PPMs work currently. For example the
PPM on Dell XPS 13 9380 that I use for testing gives only one mode for
the connector with SVID ff01. The mode VDO (or MID in UCSI lingo) has
the value 0x1c46:

        SVID=0xff01, VDO=0x1c46

From that you can clearly see that the connector can only act as the
DisplayPort source, and that it support pin assignments C, D and E.

So in practice you have a firmware bug. I understand why the PPM was
made that way. That "hack" allows the PPM to workaround a limitation
in UCSI where we in practice can't tell which configuration is in use
at any given time. Unfortunately it can not be done like that. It
leaves us with custom VDO values that we would have to be interpreted
separately, that only your PPM supports.

Please see if you can get the firmware (UCSI PPM) fixed. It really
needs to expose only one mode for DisplayPort alt mode in the
connector, and the VDO in it should be in the same form that it could
be send to the partner, i.e. giving the actual DisplayPort
capabilities for the connector.

We should not do anything before you ask them to fix the PPM.
Experience tells that if we workaround an issue in firmware, the
firmware will never ever get fixed. And in this case the workaround
may not be as simple as one would think. Even if we have to workaround
this, it needs a separate patch with a good explanation. And it
probable does not belong to this series.


Thanks,