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

Rob Herring
2023-01-31T19:44:12Z

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