Re: [PATCH 1/4] dt-bindings: usb-connector: Add support for Type-C alternate-modes

[Prashant Malani <pmalani@chromium.org>]

Hi Rob,

Restarting this thread, since I think we can re-use Patch 1/4, and I
dind't want some of the context to be lost by starting a new thread.

On Wed, Jul 22, 2020 at 09:18:02AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/21/20 4:26 AM, Rob Herring wrote:
> > On Tue, Jul 14, 2020 at 01:36:14PM +0200, Hans de Goede wrote:
> > > This commit adds the minimum bindings required to allow describing which
> > > altmodes a port supports. Currently this is limited to just specifying:
> > > 
> > > 1. The svid, which is the id of the altmode, e.g. displayport altmode has
> > > a svid of 0xff01.
> > > 
> > > 2. The vdo, a 32 bit integer, typically used as a bitmask describing the
> > > capabilities of the altmode, the bits in the vdo are specified in the
> > > specification of the altmode, the dt-binding simply refers to the
> > > specification as that is the canonical source of the meaning of the bits.
> > 
> > What if this information should be derived from information already in
> > DT (or would be there if alt mode connections are described)?
> > 
> > > 
> > > Later on we may want to extend the binding with extra properties specific
> > > to some altmode, but for now this is sufficient to e.g. hook up
> > > displayport alternate-mode.
> > 
> > I don't think this is sufficient as it doesn't describe how alternate
> > modes are connected to various components. This has been discussed some
> > here[1] with the CrOS folks. Maybe this is orthogonal, IDK, but I really
> > need something that is somewhat complete and not sprinkle a few new
> > properties at a time.
> 
> Right, but that is an orthogonal problem, this is telling the Type-C
> controller which modes it is allowed to negotiate and which capabilties
> (altmode specific, stored in the vdo) it should advertise.
> 

I concur. There is value in listing the alternate modes supported by a
connector in the connector bindings. PD negotiation (which may include
alternate mode entry) is something which is handled
by the port driver / TCPM itself, so this sounds like a reasonable place
to explicitly describe this information rather than derive it from OF
graph.

While it is important to describe how the connector is routed through the
switches and eventually to the various controllers (DP, xHCI, USB4 etc.),
it doesn't sound like we should make the Type C port driver rely on those
graph connections to derive what alternate modes to support.

FWIW, I did provide a more fleshed out example of how the OF graph
connections from port to various PHYs would work a while back, but
didn't get much feedback on it [1]

> BTW note that making the binding look like this was proposed by Heikki,
> the Type-C subsys maintainer, I ended up implementing this because Heikki
> did no have the time for it.

If the binding itself looks fine, then I'd request we move forward with
including it in the usb-connector bindings rather than stalling on the
OF graph switch bindings.
Heikki had mentioned [2] that we can adjust the ACPI bindings to accommodate
device tree requirements, and it looks like the current implementation is already in
the mainline connector class code [3], just the binding is missing.

I would be happy to re-upload this patch, with follow on patches which:
- Add the altmodes node to a Chrome OS device tree file
- Update the cros-ec-typec port driver to call the function introduced in [3].

I've tested this locally and it works fine.

[1]:
https://lore.kernel.org/lkml/CACeCKacUa1-ttBmKS_Q_xZCsArgGWkB4s9eG0c5Lc5RHa1W35Q@mail.gmail.com/
[2]:
https://lore.kernel.org/linux-usb/Ya8vxq+%2FP%2FWG4kHo@kuha.fi.intel.com/
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b458a4c5d7302947556e12c83cfe4da769665d0

Would be good to hear your thoughts on the above.

Thanks,

-Prashant