Re: [PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Hans de Goede]

Hi,

On 10-09-17 23:36, Peter Rosin wrote:
> On 2017-09-08 19:07, Hans de Goede wrote:
> > Hi,
> >
> > On 08-09-17 17:47, Peter Rosin wrote:
> > > On 2017-09-05 18:42, Hans de Goede wrote:
> > > > Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
> > > > USB device/host, resp. Type-C polarity/role/altmode mux drivers and
> > > > consumers to ensure that they agree on the meaning of the
> > > > mux_control_select() state argument.
> > > >
> > > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > > ---
>
> *snip*
>
> > > > +/*
> > > > + * Mux state values for Type-C polarity/role/altmode muxes.
> > > > + *
> > > > + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
> > > > + * inverted-polarity (Type-C plugged in upside down) can happen with any
> > > > + * other mux-state.
> > > > + */
> > > > +#define MUX_TYPEC_POLARITY_INV		BIT(0)   /* Polarity inverted bit */
> > > > +#define MUX_TYPEC_DEVICE		(0 << 1) /* USB device mode */
> > > > +#define MUX_TYPEC_HOST			(1 << 1) /* USB host mode */
> > > > +#define MUX_TYPEC_HOST_AND_DP_SRC	(2 << 1) /* USB host + 2 lanes DP src */
> > > > +#define MUX_TYPEC_DP_SRC		(3 << 1) /* 4 lanes Display Port src */
> > > > +#define MUX_TYPEC_STATES		(4 << 1)
> > >
> > > But USB Type-C muxes need not support just these states If I read it right?
> > > USB Type-C seems to be usable for a variety of protocols and the above list
> > > seems pretty much like a special case for this mux (and perhaps a set of
> > > other similar muxes). But when someone with a USB Type-C mux for different
> > > protocols shows up, that person will probably be frustrated by these
> > > defines, no? Or is there something I don't see that limits USB-C to DP?
> >
> > In general almost all hardware is limited to the above (+ analog audio over
> > the 2 Sideband use pins, but I expect that to have a separate mux).
> >
> > You're right, theoretically there might be other cases, e.g. there is a spec
> > for HDMI over Type-C (wishful thinking from the HDMI group, no one uses this),
> > but:
> >
> > 1) I expect most muxes to implement the above set, that is what all
> > hardware out there supports (well that or less).
> >
> > 2) We can always add extra defines here, that means that a Type-C mux may
> > not implement all states and return -EINVAL when asked for something it
> > does not implement, which I understand is a bit weird from a mux subsys
> > pov. But that can be the case anyways because even though the mux supports
> > these options, the board it is used on does no necessarily have to support
> > these options, e.g. there may be only 2 lanes of DP hooked up to the mux
> > (or no DP at all, but then I would them to expect a different mux).
> >
> > So the Type-C Port Manager already needs to be passed some platform
> > data describing which features the board has and keep that in mind
> > when negotiation with the dongle attached to the Type-C port, so if
> > we do get boards which do HDMI and no DP, then the TCPM would simply
> > never use the MUX_TYPEC_HOST_AND_DP_SRC and MUX_TYPEC_DP_SRC states.
>
> Ok, I googled "usb type c mux" and came up with HD3SS460 from Texas as
> the first hit.
>
> http://www.ti.com/lit/ds/symlink/hd3ss460.pdf
>
> That one has three control pins, but two of them (AMSEL and EN) are
> tri-state. So 18 states in theory. However, if EN is low everything is
> HighZ, so that collapses 6 states into 1, and 2 other states are reserved.
> Still 11 states, which is two more than what you have implemented for
> PI3USB30532. If we ignore polarity switching, it's only a one state diff.
> However, when I try to make sense of the states for the HD3SS460, I don't
> see anything that selects USB device or host. And I don't really see why
> a Type C mux has to know that; in my head the mux should just route the
> signals. And then when I look in your PI3USB30532 driver I don't seen any
> such difference either. Along the same lines, the Type C mux does not
> know/care if DP is source or sink. Or?
>
> How about:
>
> #define MUX_TYPEC_POLARITY_INV		BIT(0)   /* Polarity inverted bit */
> #define MUX_TYPEC_USB			(0 << 1) /* USB only mode */
> #define MUX_TYPEC_USB_AND_DP		(1 << 1) /* USB host + 2 lanes DP */
> #define MUX_TYPEC_DP			(2 << 1) /* 4 lanes Display Port */
> #define MUX_TYPEC_STATES		(3 << 1)

Sure that works for me, I will switch over to this for v3 of the patch-set.

One note though, compared to my list, this changes DEVICE / HOST to just a single
_USB entry. That works fine for my purpose, but typically USB host and device
controllers are 2 separate blocks with a mux in between them. Now most
current hardware have that mux in the SoC and then an external mux to
mux the USB3 lines and optionally also DP lines to the Type-C connector
and the above table does is correct (as the Type-C mux only has 1 USB
state not separate host / device states as my proposal was). But my
reason for having separate DEVICE / HOST states (and treating those
identical in the pi3usb30532 driver) was to future proof things a bit.

> I'm not sure what 2 states the HS3SS460 have in addition to the above, but
> the way I read the spec those to are variations on the MUX_TYPEC_USB_AND_DP
> state, but routing the DP signals to alternate pins. Which suggests that
> more documentation is needed to describe exactly what is meant when someone
> selects MUX_TYPEC_USB_AND_DP?

The DP over Type-C spec unfortunately is not open, but the slva844a.pdf
TI appnote has a table listing the possible pin permutations which can
be used with DP over Type-C (Table 1. page 5) which always has all the
superspeed USB pins in the same place.

Regards,

Hans