On Thu, Jun 16, 2022 at 12:57 PM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
>
> On Thu, Jun 16, 2022 at 12:34 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > On Thu, Jun 16, 2022 at 01:54:36AM -0700, Prashant Malani wrote:
> > > On Thu, Jun 16, 2022 at 12:42 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> > > >
> > > > Quoting Prashant Malani (2022-06-15 10:20:20)
> > > > >
> > > > > .../display/bridge/analogix,anx7625.yaml | 64 +++++++++++++++++++
> > > > > 1 file changed, 64 insertions(+)
> > > >
> > > > Can this file get a link to the product brief[1]? It helps to quickly
> > > > find the block diagram.
> > >
> > > Sure, but I don't really think that should be included in this patch
> > > (or series).
> > > I'd be happy to submit a separate patch once this series is resolved.
> > >
> > > >
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > > > index 35a48515836e..bc6f7644db31 100644
> > > > > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > > > @@ -105,6 +105,34 @@ properties:
> > > > > - port@0
> > > > > - port@1
> > > > >
> > > > > + switches:
> > > > > + type: object
> > > > > + description: Set of switches controlling DisplayPort traffic on
> > > > > + outgoing RX/TX lanes to Type C ports.
> > > > > + additionalProperties: false
> > > > > +
> > > > > + properties:
> > > > > + '#address-cells':
> > > > > + const: 1
> > > > > +
> > > > > + '#size-cells':
> > > > > + const: 0
> > > > > +
> > > > > + patternProperties:
> > > > > + '^switch@[01]$':
> > > > > + $ref: /schemas/usb/typec-switch.yaml#
> > > > > + unevaluatedProperties: false
> > > > > +
> > > > > + properties:
> > > > > + reg:
> > > > > + maxItems: 1
> > > > > +
> > > > > + required:
> > > > > + - reg
> > > > > +
> > > > > + required:
> > > > > + - switch@0
> > > > > +
> > > > > required:
> > > > > - compatible
> > > > > - reg
> > > > > @@ -167,5 +195,41 @@ examples:
> > > > > };
> > > > > };
> > > > > };
> > > > > + switches {
> > > >
> > > > Is "switches" a bus?
> > >
> > > No.
> > >
> > > >
> > > > > + #address-cells = <1>;
> > > > > + #size-cells = <0>;
> > > > > + switch@0 {
> > > > > + compatible = "typec-switch";
> > > >
> > > > Is this compatible matched against a driver that's populated on this
> > > > "switches" bus?
> > >
> > > No. Patch 6/7 has the implementation details on how the anx driver
> > > performs the enumeration of switches.
> > >
> > > >
> > > > > + reg = <0>;
> > > > > + mode-switch;
> > > > > +
> > > > > + ports {
> > > > > + #address-cells = <1>;
> > > > > + #size-cells = <0>;
> > > > > + port@0 {
> > > > > + reg = <0>;
> > > > > + anx_typec0: endpoint {
> > > > > + remote-endpoint = <&typec_port0>;
> > > > > + };
> > > > > + };
> > > > > + };
> > > >
> > > > I was expecting to see these simply be more ports in the existing graph
> > > > binding of this device, and then have the 'mode-switch' or
> > > > 'orientation-switch' properties be at the same level as the compatible
> > > > string "analogix,anx7625". Here's the reasoning, based on looking at the
> > > > product brief and the existing binding/implementation.
> > > >
> > > > Looking at the only existing implementation of this binding upstream in
> > > > mt8183-kukui-jacuzzi.dtsi it looks like one of these typec ports is
> > > > actually the same physically as the 'anx7625_out' endpoint (reg address
> > > > of 1) that is already defined in the binding. It seems that MIPI DSI/DPI
> > > > comes in and is output through 2 lanes, SSRX2 and SSTX2 according to the
> > > > product brief[1], and that is connected to some eDP panel
> > > > ("auo,b116xw03"). Presumably that is the same as anx_typec1 in this
> > > > patch? I suspect the USB3.1 input is not connected on this board, and
> > > > thus the crosspoint switch is never used, nor the SSRX1/SSTX1 pins.
> > > >
> > > > The existing binding defines the MIPI DSI/DPI input as port0 and two of
> > > > the four lanes of output that is probably by default connected to the
> > > > "DisplayPort Transmitter" as port1 because that's how the crosspoint
> > > > switch comes out of reset. That leaves the USB3.1 input possibly needing
> > > > a port in the ports binding, and the other two lanes of output needing a
> > > > port in the ports binding to describe their connection to the downstream
> > > > device. And finally information about if the crosspoint switch needs to
> > > > be registered with the typec framework to do typec things, which can be
> > > > achieved by the presence of the 'mode-switch' property.
> > > >
> > > > On a board like kukui-jacuzzi these new properties and ports wouldn't be
> > > > specified, because what is there is already sufficient. If this chip is
> > > > connected to a usb-c-connector then I'd expect to see a connection from
> > > > the output ports in the graph binding to the connector node's ports.
> > > > There aren't any ports in the usb-c-connector binding though from what I
> > > > see.
> > > >
> > > > I believe there's also one more use case here where USB3.1 or MIPI
> > > > DSI/DPI is connected on the input side and this device is used to steer
> > > > USB3.1 or DP through the crosspoint switch to either of the two output
> > > > pairs. This last scenario means that we have to describe both output
> > > > pairs, SSRX1/SSTX1 and SSRX2/SSTX2, as different ports in the binding so
> > > > they can be connected to different usb-c-connectors if the hardware
> > > > engineer wired the output pins that way.
> > > >
> > > > TL;DR: Can we add 'mode-switch' as an optional property and two more
> > > > ports at address 2 and 3 for the USB3.1 input and the SSRX1/SSTX1 pair
> > > > respectively to the existing graph part of this binding?
> > >
> > > Sorry, but I got lost midway through the preceding explanation.
> >
> > Made sense to me.
> >
> > > The binding
> > > can always add additional ports to each "switch" to accomplish the
> > > graph connections
> > > you are alluding to (if the driver needs/uses it, which I don't think
> > > this one does at present).
> >
> > Why is the switch special? If I just look at this from a block diagram
> > perspective, I just see a list of interfaces that need to be described
> > in the graph.
>
> Because it is specific to Type-C connectors. The anx7625.h does
> contain a cross-point
> switch which controls data lines coming from 1 (or more) Type-C
> connectors, so it seems reasonable
> to have a dedicated binding for such types of hardware sub-components,
> which helps define the graph connections
> in a more uniform manner. That's not to say:
> - this can only be used by this hardware. The typec-switch binding is
> generic enough to accommodate other hardware.
> - there is only 1 way to do this. The interfaces could be described
> using existing port OF graph bindings, but I don't
> see that as reason enough to not include a dedicated switch binding if
> it makes the overall binding more logically organized (IMO) and
> makes driver registration code mode clean.
>
> >
> > > Adding extra ports to existing ports gets tricky from a mode-switch
> > > enumeration perspective (which
> > > ports should have the modes switches, which shouldn't? Do you follow
> > > the remote end points for each port
> > > and see which one is a Type C connector?
> >
> > The driver knows which port is which because the binding has to define
> > it. So you have to check 2 of them (SSRX1/SSTX1 and SSRX2/SSTX2) to find
> > usb C connectors.
>
> Right, but with the switch binding you no longer need to check. If
> there is a typec-switch, you know
> it is coming from a Type-C connector, so you can just register the
> switches with the Type-C framework.
>
> >
> > > What if we add an
> > > intermediate switch device in the future?)
> > > Having a dedicated "switch" binding makes this consistent and easy
> > > (port0 will always have the end-point for the switch).
> > >
> > > While there may be more than 1 valid approach here, I believe the
> > > current one is appropriate.
> >
> > To put it simply, if you want to define a generic binding, I want to see
> > at least 2 users of it.
Pin-Yen and I will work on adding another user for the binding to v5 of
this patch series.
Best regards,
- Prashant
