Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Rob Herring <robh@kernel.org>]

On Mon, Jan 30, 2023 at 01:42:14PM -0800, Bjorn Andersson wrote:
> On Mon, Jan 30, 2023 at 10:48:13AM -0600, Rob Herring wrote:
> > On Wed, Jan 25, 2023 at 03:40:13PM -0800, Bjorn Andersson wrote:
> > > On Tue, Jan 24, 2023 at 08:31:13PM -0600, Rob Herring wrote:
> > > > On Tue, Jan 24, 2023 at 11:04 AM Bjorn Andersson
> > > > <quic_bjorande@quicinc.com> wrote:
> > > > >
> > > > > On Tue, Jan 24, 2023 at 10:00:12AM -0600, Rob Herring wrote:
> > > > > > On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
> > > > > > <quic_bjorande@quicinc.com> wrote:
> [..]
> > > > > Are you saying that the connector should link with the mux and then the
> > > > > source of the signal should be daisy chained? Or that we should just
> > > > > link both of them as two separate endpoints from the connector?
> > > > 
> > > > The former. The data path of the signal in h/w should match in the DT
> > > > graph. The caveat being we don't normally show PHYs in that mix
> > > > because they are somewhat transparent. That's maybe becoming less true
> > > > with routing or muxing included in PHYs.
> > > > 
> > > 
> > > We have discussed and prototyped this a few times now in the Qualcomm
> > > community, and I am not a fan of having to add forwarding-logic to each
> > > implementation of a Type-C mux/switch, which in some configuration might
> > > have an entity behind it that needs the notifications.
> > 
> > I don't know if we can really escape that.
> > 
> 
> Okay, we'll have to figure out how to implement that when such need come...
> 
> > 
> > > But I don't think there's a concern for this binding (in my
> > > implementation), there's currently no mode/orientation switching
> > > happening beyond this entity.
> > > 
> > > 
> > > 
> > > That said, if we're to represent each signal path to the connector,
> > > I would like to bring up this problem again:
> > > https://lore.kernel.org/all/20220520164810.141400-1-bjorn.andersson@linaro.org/
> > > 
> > > port@0 represent the HS signals going to the connector, port@1 the SS
> > > signals going to the connector, port@2 the SBU signals going to the
> > > connector.
> > > 
> > > But I need some representation of the HPD (hot plug detection) "signal"
> > > (there is no actual signal path in hardware, this is a virtual
> > > notification) going _from_ the connector to the DisplayPort controller.
> > 
> > I would assume whatever entity is deciding to enable the DP signals on 
> > the connector would be what generates the HPD notification.
> 
> The HPD notification comes from the display/connector, and is
> conceptually equivalent to hpd-gpios in e.g. the dp-connector binding.

Except that it is software based rather than a h/w signal (ignoring the 
s/w generating a h/w HPD signal for you).

> 
> > I think you want to describe the DP signal connections and muxing, but
> > HPD itself wouldn't be in the DT.
> > 
> 
> Okay, so you're saying that the display driver needs to traverse the
> graph representing the display-signal path, in hope to find someone
> generating a HPD notification?

After further discussion, I think it is still the immediate neighbor, it 
is just that the immediate neighbor is some other component, not the 
type-c connector. 

> > > We discussed this (perhaps in person, as there's no trace on lkml?) and
> > > you suggested that I use a second endpoint under port@1, instead of
> > > introducing a fourth port.
> > 
> > Multiple endpoints on a port are typically a mux or fanout depending on 
> > the data direction. But the muxing is not really in the connector, so 
> > that should probably be represented somewhere else. I think a new port 
> > suffers from the same issue. Maybe that's close enough? Depends if there 
> > are cases of more alt modes or more complicated muxing/switching.
> > 
> > > I'm fine either way, but I don't think it would be convenient nor
> > > representable to daisy chain this behind any of the existing endpoints;
> > > none of the other endpoints should deal with the HPD signal and the
> > > direct of_graph-link between the usb-c-connector and the DP controller
> > > mimics that of e.g. dp-connector very nicely, both in description and
> > > implementation.
> > 
> > That would be nice, but the reality is there's a lot more between DP and 
> > the connector with USB-C and the graph should reflect that.
> 
> Not when it comes to delivering the HPD notification, afaict.
> 
> The TCPM will configure muxes & switches to ensure that the USB
> connector is wired up according to what has been decided over USB PD.
> 
> The HPD signal is orthogonal to that configuration, and should not be
> picked up by any of the other components.

Agreed as HPD is not a h/w signal routed between components.


> > I guess the 
> > problem there is being able to walk the graph. Suppose we have:
> > 
> > DP out port -> altmode mux in port -> altmode mux out port -> type-c 
> > port 1
> > 
> > The issue walking the graph here would be generic code doesn't know 
> > altmode mux port numbering as that's not a generic thing (could be 
> > perhaps?). Maybe you can walk from each end and see if you end up in 
> > the same device.
> > 
> > Of course, I haven't even considered the split cases where it's not 
> > just either USB3 OR DP, but combinations. 
> > 
> 
> The implementation that toggles between the DP-only and USB/DP-combo is
> not stable, so we currently only support USB/DP-combo upstream.

Okay, but I don't care about that from a binding standpoint. All 
possibilities need to be considered whether your h/w can support it or 
not.
 
> Again, the TCPM will handle the muxing and orientation switching in the
> PHY and sbu-mux, then once a datapath capable of delivering DP-altmode
> data is established, it might send HPD notifications - to the display
> driver.
> 
> > 
> > What I'd really like to see for all this USB-C stuff is block diagrams 
> > of the h/w components and then what the corresponding DT looks like. 
> > Trying to extend things one piece at a time is hard to review and not 
> > likely going to end with a flexible design.
> > 
> 
> This is the design we have in a range of different boards:

*This* is what I need for every Type-C binding.

> 
>                      +------------- - -
>  USB connector       | SoC
>  +-+                 |   +--------+    +-------+
>  | |                 |   |        |    |       |
>  |*|<------- HS -----|-->| HS phy |<-->| (EUD) |<--+
>  | |                 |   |        |    |       |   |   +--------+
>  | |                 |   +--------+    +-------+   +-->|        |
>  | |                 |                                 |  dwc3  |
>  | |                 |   +--------+        /---------->|        |
>  | |   +----------+  |   |        |<------/            +--------+
>  |*|<--|(redriver)|<-|-->| SS phy |
>  | |   +----------+  |   |        |<-\   +------------+
>  | |                 |   +--------+   \->|            |
>  | |                 |                   |     DP     |
>  | |     +-----+     |                   | controller |
>  |*|<--->| SBU |<----|------------------>|            |
>  | |     | mux |     |                   |            |
>  | |     +----+      |                   +------------+
>  +-+                 |
>                      +------------- - -

Where's the TCPM?


> The EUD and redriver are only found/used in some designs.  My proposed
> representation of this (without those) is:

I'd assume a redriver is mostly transparent to s/w?


> 
> /soc {
>     usb-controller {
>         dwc3 {
>             port {
>                 dwc0-out: endpoint {
>                     remote-endpoint = <&connector0_hs>;
>             };
>         };
>     };
> 
>     ss_phy: phy {
>         port {
>             ss_phy_out: endpoint {
>                 remote-endpoint = <&connector0_ss>;
>             };
>         };
>     };
> 
>     display-subsystem {
>         displayport-controller {
>             phys = <&ss_phy>;
>             ports {
>                 port@1 {
>                     reg = <1>;
>                     dp0_out: endpoint {
>                         remote-endpoint = <&connector0_hpd>;
>                     };
>                 };
>             };
>         };
>     };
> };
> 
> usb0-sbu-mux {
>     compatible = "gpio-sbu-mux";
> 
>     port {
>         sbu0_out: endpoint {
>             remote-endpoint = <&connector_sbu>;
>         };
>     };
> };
> 
> tcpm {
>     connector@0 {
>         compatible = "usb-c-connector";
>         reg = <0>;
>         ports {
>             port@0 {
>                 reg = <0>;
>                 connector0_hs: endpoint {
>                     remote-endpoint = <&dwc0_out>;
>                 };
>             };
> 
>             port@1 {
>                 reg = <1>;
>                 connector0_ss: endpoint@0 {
>                     remote-endpoint = <&ss_phy_out>;
>                 };
>                 connector0_hpd: endpoint@1 {
>                     remote-endpoint = <&dp0_out>;
>                 };

This just looks wrong to me because one connection is the output of the 
phy's mux and one is the input. The USB SS connection is implicit, but I 
think it should be explicit from dwc3 to ss_phy. It would need an output 
port and 2 input ports. I want to say we already have bindings doing 
this.

>             };
> 
>             port@2 {
>                 reg = <2>;
>                     connector_sbu: endpoint {
>                         remote-endpoint = <&sbu0_out>;
>                 };
>             };
>         };
>     };
> };
> 
> The EUD needs to be able to override the role-switch state, so the design that
> was accepted was to implement the role-switch forwarding logic in the driver
> and daisy chain the of-graph.

Given EUD is a Qualcomm only thing, I'm not all that worried about it.

> 
> No redriver has made it to LKML, but the this is where the daisy chain vs
> reference all instances from port@1 comes in.
> 
> The Type-C port manager (tcpm) might be handling multiple usb-c-connectors, in
> which case the reg of the connector identifies which instance is being
> described.
> 
> 
> So I think that all these pieces fits your model, except the port@1/endpoint@1
> and the fact that displayport-controller has a of_graph.
> 
> 
> In particular we have a number of these:
> 
> /dp-connector {
>     compatible = "dp-connector";
> 
>     port {
>         connector: endpoint {
>             remote-endpoint = <&dp_out>;
>         };
>     };
> };
> 
> /soc {
>     display-subsystem {
>         displayport-controller {
>             phys = <&some_dp_phy>;
>             ports {
>                 port@1 {
>                     reg = <1>;
>                     dp_out: endpoint {
>                         remote-endpoint = <&connector>;
>                     };
>                 };
>             };
>         };
>     };
> };
> 
> As you said previously, it doesn't make sense to represent the phy in this
> graph. We just define the output of the controller as port@1 and link that to
> the connector.

What I said (or meant) was we don't represent phys which are just 
providing the electrical interface. Your 'phy' is also a mux/switch, so 
it does make sense to represent it in the graph.

> 
> So what is the output of the dp controller in the USB case - if we're not
> representing that as the HPD link directly between the connector and the
> controller?

The answer lies in your block diagram... 

The question I think is whether we could standardize the mux/switch 
graph ports. Say 'port@0' is the output to type-C connector port@1, 
and port@[1-9] are altmode connections to USB/DP/TB. If we did that, 
then generic code can walk the graph from a controller to the connector. 
We only need to know that port@0 goes to the connector. However, that 
assumes there is only 1 entity in the middle and I don't know if that 
holds true.

Rob