[PATCH v5 0/9] usb: typec: Introduce typec-switch binding

[PATCH v5 0/9] usb: typec: Introduce typec-switch binding

[Prashant Malani]

This series introduces a binding for Type-C data lane switches. These
control the routing and operating modes of USB Type-C data lanes based
on the PD messaging from the Type-C port driver regarding connected
peripherals.

The first 2 patches introduce the new "typec-switch" binding as
well as one user of it (the ANX7625 drm bridge).

Patches 3-5 add functionality to the anx7625 driver to
register the mode-switches, as well as program its crosspoint
switch depending on which Type-C port has a DisplayPort (DP) peripheral
connected to it.

Patch 6-9 add similar bindings update and Type-C switch support to the
it6505 driver.

v4:
https://lore.kernel.org/linux-usb/20220615172129.1314056-8-pmalani@chromium.org/

Changes in v5:
- Rebased on usb-next, so removed Patch v4 1/7 and Patch v4 2/7 from
  this version (v5) since they are already in usb-next.
- Added newer Reviewed-by tags.
- Added new patches (6-9) in this version for a 2nd example (it6505)
  of a binding of the user.

Patch submission suggestions:
Option 1:
- Bindings patches 1/9 and 2/9 can go through the USB repo (since they are
  already reviewed from v4 [1]).
- Bindings patch 6/9 can go through the USB repo, and the remaining patches
  (3-5,7-9) can go through the DRM repo.
  <or>
- Patches 3-9 can all go through the DRM repo.

Option 2:
- All patches (1-9) go through the USB repo.

(My apologies if I've made this confusing, and I appreciate any
suggestions for better submission strategy).

[1]: https://lore.kernel.org/linux-usb/YrMxFeMc0tk%2FK1qL@kroah.com/

Pin-Yen Lin (5):
  drm/bridge: anx7625: Add typec_mux_set callback function
  dt/bindings: drm/bridge: it6505: Add mode-switch support
  drm/bridge: it6505: Register number of Type C switches
  drm/bridge: it6505: Register Type-C mode switches
  drm/bridge: it6505: Add typec_mux_set callback function

Prashant Malani (4):
  dt-bindings: usb: Add Type-C switch binding
  dt-bindings: drm/bridge: anx7625: Add mode-switch support
  drm/bridge: anx7625: Register number of Type C switches
  drm/bridge: anx7625: Register Type-C mode switches

 .../display/bridge/analogix,anx7625.yaml      |  64 +++++++
 .../bindings/display/bridge/ite,it6505.yaml   |  97 +++++++++-
 .../devicetree/bindings/usb/typec-switch.yaml |  74 ++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.c     | 148 +++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h     |  20 ++
 drivers/gpu/drm/bridge/ite-it6505.c           | 171 +++++++++++++++++-
 6 files changed, 569 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml

-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

Introduce a binding which represents a component that can control the
routing of USB Type-C data lines as well as address data line
orientation (based on CC lines' orientation).

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v4:
- Added Reviewed-by tags.
- Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
  applied to usb-next)

Changes since v3:
- No changes.

Changes since v2:
- Added Reviewed-by and Tested-by tags.

Changes since v1:
- Removed "items" from compatible.
- Fixed indentation in example.

 .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml

diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
new file mode 100644
index 000000000000..78b0190c8543
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: USB Type-C Switch
+
+maintainers:
+  - Prashant Malani <pmalani@chromium.org>
+
+description:
+  A USB Type-C switch represents a component which routes USB Type-C data
+  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
+  Thunderbolt etc.) depending on which mode the Type-C port, port partner
+  and cable are operating in. It can also modify lane routing based on
+  the orientation of a connected Type-C peripheral.
+
+properties:
+  compatible:
+    const: typec-switch
+
+  mode-switch:
+    type: boolean
+    description: Specify that this switch can handle alternate mode switching.
+
+  orientation-switch:
+    type: boolean
+    description: Specify that this switch can handle orientation switching.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description: OF graph binding modelling data lines to the Type-C switch.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Link between the switch and a Type-C connector.
+
+    required:
+      - port@0
+
+required:
+  - compatible
+  - ports
+
+anyOf:
+  - required:
+      - mode-switch
+  - required:
+      - orientation-switch
+
+additionalProperties: true
+
+examples:
+  - |
+    drm-bridge {
+        usb-switch {
+            compatible = "typec-switch";
+            mode-switch;
+            orientation-switch;
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    anx_ep: endpoint {
+                        remote-endpoint = <&typec_controller>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.37.0.rc0.104.g0611611a94-goog

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Stephen Boyd]

Quoting Prashant Malani (2022-06-22 10:34:30)
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> new file mode 100644
> index 000000000000..78b0190c8543
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: USB Type-C Switch
> +
> +maintainers:
> +  - Prashant Malani <pmalani@chromium.org>
> +
> +description:
> +  A USB Type-C switch represents a component which routes USB Type-C data
> +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> +  and cable are operating in. It can also modify lane routing based on
> +  the orientation of a connected Type-C peripheral.
> +
> +properties:
> +  compatible:
> +    const: typec-switch
> +
> +  mode-switch:
> +    type: boolean
> +    description: Specify that this switch can handle alternate mode switching.
> +
> +  orientation-switch:
> +    type: boolean
> +    description: Specify that this switch can handle orientation switching.
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description: OF graph binding modelling data lines to the Type-C switch.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Link between the switch and a Type-C connector.

Is there an update to the usb-c-connector binding to accept this port
connection?

> +
> +    required:
> +      - port@0
> +
> +required:
> +  - compatible
> +  - ports
> +
> +anyOf:
> +  - required:
> +      - mode-switch
> +  - required:
> +      - orientation-switch
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    drm-bridge {
> +        usb-switch {
> +            compatible = "typec-switch";

I still don't understand the subnode design here. usb-switch as a
container node indicates to me that this is a bus, but in earlier rounds
of this series it was stated this isn't a bus. Why doesn't it work to
merge everything inside usb-switch directly into the drm-bridge node?

> +            mode-switch;
> +            orientation-switch;
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +                    anx_ep: endpoint {
> +                        remote-endpoint = <&typec_controller>;
> +                    };
> +                };
> +            };
> +        };

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

On Thu, Jun 23, 2022 at 11:30 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-22 10:34:30)
> > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > new file mode 100644
> > index 000000000000..78b0190c8543
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > @@ -0,0 +1,74 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: USB Type-C Switch
> > +
> > +maintainers:
> > +  - Prashant Malani <pmalani@chromium.org>
> > +
> > +description:
> > +  A USB Type-C switch represents a component which routes USB Type-C data
> > +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> > +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> > +  and cable are operating in. It can also modify lane routing based on
> > +  the orientation of a connected Type-C peripheral.
> > +
> > +properties:
> > +  compatible:
> > +    const: typec-switch
> > +
> > +  mode-switch:
> > +    type: boolean
> > +    description: Specify that this switch can handle alternate mode switching.
> > +
> > +  orientation-switch:
> > +    type: boolean
> > +    description: Specify that this switch can handle orientation switching.
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +    description: OF graph binding modelling data lines to the Type-C switch.
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: Link between the switch and a Type-C connector.
>
> Is there an update to the usb-c-connector binding to accept this port
> connection?

Not at this time. I don't think we should enforce that either.
(Type-C data-lines could theoretically be routed through intermediate
hardware like retimers/repeaters)

>
> > +
> > +    required:
> > +      - port@0
> > +
> > +required:
> > +  - compatible
> > +  - ports
> > +
> > +anyOf:
> > +  - required:
> > +      - mode-switch
> > +  - required:
> > +      - orientation-switch
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +    drm-bridge {
> > +        usb-switch {
> > +            compatible = "typec-switch";
>
> I still don't understand the subnode design here. usb-switch as a
> container node indicates to me that this is a bus, but in earlier rounds
> of this series it was stated this isn't a bus.

I am not aware of this as a requirement. Can you please point me to the
documentation that states this needs to be the case?

> Why doesn't it work to
> merge everything inside usb-switch directly into the drm-bridge node?

I attempted to explain the rationale in the previous version [1], but
using a dedicated sub-node means the driver doesn't haven't to
inspect individual ports to determine which of them need switches
registered for them. If it sees a `typec-switch`, it registers a
mode-switch and/or orientation-switch. IMO it simplifies the hardware
device binding too.

It also maps with the internal block diagram for these hardware
components (for ex. the anx7625 crosspoint switch is a separate
sub-block within anx7625).

[1] https://lore.kernel.org/linux-usb/CACeCKaeH6qTTdG_huC4yw0xxG8TYEOtfPW3tiVNwYs=P4QVPXg@mail.gmail.com/

>
> > +            mode-switch;
> > +            orientation-switch;
> > +            ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                port@0 {
> > +                    reg = <0>;
> > +                    anx_ep: endpoint {
> > +                        remote-endpoint = <&typec_controller>;
> > +                    };
> > +                };
> > +            };
> > +        };

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Stephen Boyd]

Quoting Prashant Malani (2022-06-23 12:08:21)
> On Thu, Jun 23, 2022 at 11:30 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-22 10:34:30)
> > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > new file mode 100644
> > > index 000000000000..78b0190c8543
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > @@ -0,0 +1,74 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
[...]
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: Link between the switch and a Type-C connector.
> >
> > Is there an update to the usb-c-connector binding to accept this port
> > connection?
>
> Not at this time. I don't think we should enforce that either.
> (Type-C data-lines could theoretically be routed through intermediate
> hardware like retimers/repeaters)

I'm mostly wondering if having such a connection to the usb-c-connector,
or even through some retimer/repeater, would be sufficient to detect how
many type-c ports are connected to the device. If the type-c pin
assignments only support two or four lanes for DP then it seems like we
should describe the two lanes or four lanes as one graph endpoint
"output" and then have some 'data-lanes' property in case the DP lanes
are flipped while being sent to the retimer or usb-c-connector. This
would of course depend on the capability of the device, i.e. if it can
remap DP lanes or only has 2 lanes of DP, etc.

> > > +  - |
> > > +    drm-bridge {
> > > +        usb-switch {
> > > +            compatible = "typec-switch";
> >
> > I still don't understand the subnode design here. usb-switch as a
> > container node indicates to me that this is a bus, but in earlier rounds
> > of this series it was stated this isn't a bus.
>
> I am not aware of this as a requirement. Can you please point me to the
> documentation that states this needs to be the case?

I'm not aware of any documentation for the dos and don'ts here. Are
there any examples in the bindings directory that split up a device into
subnodes that isn't in bindings/mfd? I just know from experience that
any time I try to make a child node of an existing node that I'm
supposed to be describing a bus, unless I'm adding some sort of
exception node like a graph binding or an opp table. Typically a node
corresponds 1:1 with a device in the kernel. I'll defer to Rob for any
citations.

>
> > Why doesn't it work to
> > merge everything inside usb-switch directly into the drm-bridge node?
>
> I attempted to explain the rationale in the previous version [1], but
> using a dedicated sub-node means the driver doesn't haven't to
> inspect individual ports to determine which of them need switches
> registered for them. If it sees a `typec-switch`, it registers a
> mode-switch and/or orientation-switch. IMO it simplifies the hardware
> device binding too.

How is that any harder than hard-coding that detail into the driver
about which port and endpoint is possibly connected to the
usb-c-connector (or retimer)? All of that logic could be behind some API
that registers a typec-switch based on a graph port number that's passed
in, ala drm_of_find_panel_or_bridge()'s design.

Coming from a DT writer's perspective, I just want to go through the
list of output pins in the datasheet and match them up to the ports
binding for this device. If it's a pure DP bridge, where USB hardware
isn't an input or an output like the ITE chip, then I don't want to have
to describe a port graph binding for the case when it's connected to a
dp-connector (see dp-connector.yaml) in the top-level node and then have
to make an entirely different subnode for the usb-c-connector case with
a whole other set of graph ports.

How would I even know which two differential pairs correspond to port0
or port1 in this binding in the ITE case? Ideally we make the graph
binding more strict for devices by enforcing that their graph ports
exist. Otherwise we're not fully describing the connections between
devices and our dtb checkers are going to let things through where the
driver most likely will fail because it can't figure out what to do,
e.g. display DP on 4 lanes or play some DP lane rerouting games to act
as a mux.

>
> It also maps with the internal block diagram for these hardware
> components (for ex. the anx7625 crosspoint switch is a separate
> sub-block within anx7625).

We don't make DT bindings for sub-components like this very often. It
would make more sense to me to have a subnode if a typec switch was some
sort of off the shelf hard macro that the hardware engineer placed down
inside the IC that they delivered. Then we could have a completely
generic driver that binds to the generic binding that knows how to drive
the hardware, because it's an unchangeable hard macro with a well
defined programming interface.

>
> [1] https://lore.kernel.org/linux-usb/CACeCKaeH6qTTdG_huC4yw0xxG8TYEOtfPW3tiVNwYs=P4QVPXg@mail.gmail.com/

I looked at the fsa4480 driver and the device has a publicly available
datasheet[2]. That device is designed for "audio accessory mode" but I
guess it's being used to simply mux SBU lines? There isn't an upstream
user of the binding so far, but it also doesn't look like a complete
binding. I'd expect to see DN_L/R as a graph output connected to the
usb-c-connector and probably have a usb2.0 input port and a 'sound-dai'
property to represent the input audio path.

Finally, simply connecting to the typec controller node isn't sufficient
because a typec controller can be controlling many usb-c-connectors so I
don't see how the graph binding would be able to figure out how many
usb-c-connectors are connected to a mux like device, unless we took the
approach of this patch. Is that why you're proposing this binding? To
avoid describing a graph binding in the usb-c-connector and effectively
"pushing" the port count up to the mux?

[2] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-23 12:08:21)
> > On Thu, Jun 23, 2022 at 11:30 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Prashant Malani (2022-06-22 10:34:30)
> > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > new file mode 100644
> > > > index 000000000000..78b0190c8543
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > @@ -0,0 +1,74 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> [...]
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > +        description: Link between the switch and a Type-C connector.
> > >
> > > Is there an update to the usb-c-connector binding to accept this port
> > > connection?
> >
> > Not at this time. I don't think we should enforce that either.
> > (Type-C data-lines could theoretically be routed through intermediate
> > hardware like retimers/repeaters)
>
> I'm mostly wondering if having such a connection to the usb-c-connector,
> or even through some retimer/repeater, would be sufficient to detect how
> many type-c ports are connected to the device. If the type-c pin
> assignments only support two or four lanes for DP then it seems like we
> should describe the two lanes or four lanes as one graph endpoint
> "output" and then have some 'data-lanes' property in case the DP lanes
> are flipped while being sent to the retimer or usb-c-connector. This
> would of course depend on the capability of the device, i.e. if it can
> remap DP lanes or only has 2 lanes of DP, etc.
>
> > > > +  - |
> > > > +    drm-bridge {
> > > > +        usb-switch {
> > > > +            compatible = "typec-switch";
> > >
> > > I still don't understand the subnode design here. usb-switch as a
> > > container node indicates to me that this is a bus, but in earlier rounds
> > > of this series it was stated this isn't a bus.
> >
> > I am not aware of this as a requirement. Can you please point me to the
> > documentation that states this needs to be the case?
>
> I'm not aware of any documentation for the dos and don'ts here. Are
> there any examples in the bindings directory that split up a device into
> subnodes that isn't in bindings/mfd?

usb-c-connector [3] and its users is an example.

>  I just know from experience that
> any time I try to make a child node of an existing node that I'm
> supposed to be describing a bus, unless I'm adding some sort of
> exception node like a graph binding or an opp table. Typically a node
> corresponds 1:1 with a device in the kernel. I'll defer to Rob for any
> citations.
>
> >
> > > Why doesn't it work to
> > > merge everything inside usb-switch directly into the drm-bridge node?
> >
> > I attempted to explain the rationale in the previous version [1], but
> > using a dedicated sub-node means the driver doesn't haven't to
> > inspect individual ports to determine which of them need switches
> > registered for them. If it sees a `typec-switch`, it registers a
> > mode-switch and/or orientation-switch. IMO it simplifies the hardware
> > device binding too.
>
> How is that any harder than hard-coding that detail into the driver
> about which port and endpoint is possibly connected to the
> usb-c-connector (or retimer)? All of that logic could be behind some API
> that registers a typec-switch based on a graph port number that's passed
> in, ala drm_of_find_panel_or_bridge()'s design.

If each driver has to do it (and the port specifics vary for each driver),
it becomes an avoidable overhead for each of them.
I prefer hard-coding such details if avoidable. I suppose both approaches
require modifications to the binding and the driver code.

>
> Coming from a DT writer's perspective, I just want to go through the
> list of output pins in the datasheet and match them up to the ports
> binding for this device. If it's a pure DP bridge, where USB hardware
> isn't an input or an output like the ITE chip, then I don't want to have
> to describe a port graph binding for the case when it's connected to a
> dp-connector (see dp-connector.yaml) in the top-level node and then have
> to make an entirely different subnode for the usb-c-connector case with
> a whole other set of graph ports.

This approach still allows for that, if the driver has any use for it
(AFAICT these drivers don't).
Iff that driver uses it, one can (optionally) route their output
(top-level) ports through the
"typec-switch" sub-node (and onwards as required).
If it's being used in a "pure-DP" configuration, the "typec-switch" just
goes away (the top level ports can be routed as desired by the driver).
(Again, I must reiterate that neither this driver or the anx driver
utilizes this)

>
> How would I even know which two differential pairs correspond to port0
> or port1 in this binding in the ITE case?

Why do we need to know that? It doesn't affect this or the other
driver or hardware's
functioning in a perceivable way.

> Ideally we make the graph
> binding more strict for devices by enforcing that their graph ports
> exist. Otherwise we're not fully describing the connections between
> devices and our dtb checkers are going to let things through where the
> driver most likely will fail because it can't figure out what to do,
> e.g. display DP on 4 lanes or play some DP lane rerouting games to act
> as a mux.

How is the current binding enforcing this? The typec-switch binding
as a first step ensures that the DT is connecting the hardware(anx,ite
etc) to something
that at least "claims" to be a Type-C switch.

>
> >
> > It also maps with the internal block diagram for these hardware
> > components (for ex. the anx7625 crosspoint switch is a separate
> > sub-block within anx7625).
>
> We don't make DT bindings for sub-components like this very often. It
> would make more sense to me to have a subnode if a typec switch was some
> sort of off the shelf hard macro that the hardware engineer placed down
> inside the IC that they delivered. Then we could have a completely
> generic driver that binds to the generic binding that knows how to drive
> the hardware, because it's an unchangeable hard macro with a well
> defined programming interface.
>
> >
> > [1] https://lore.kernel.org/linux-usb/CACeCKaeH6qTTdG_huC4yw0xxG8TYEOtfPW3tiVNwYs=P4QVPXg@mail.gmail.com/
>
> I looked at the fsa4480 driver and the device has a publicly available
> datasheet[2]. That device is designed for "audio accessory mode" but I
> guess it's being used to simply mux SBU lines? There isn't an upstream
> user of the binding so far, but it also doesn't look like a complete
> binding. I'd expect to see DN_L/R as a graph output connected to the
> usb-c-connector and probably have a usb2.0 input port and a 'sound-dai'
> property to represent the input audio path.
>
> Finally, simply connecting to the typec controller node isn't sufficient
> because a typec controller can be controlling many usb-c-connectors so I
> don't see how the graph binding would be able to figure out how many
> usb-c-connectors are connected to a mux like device, unless we took the
> approach of this patch.

It can follow the endpoint of the typec-switch port (the port parent
of the remote
end-point would be a 'usb-c-connector'). That is if the graph binding
(I'm assuming you mean the switch device here) wants to figure this
out in the first place.

> Is that why you're proposing this binding? To
> avoid describing a graph binding in the usb-c-connector and effectively
> "pushing" the port count up to the mux?

No, that is not the intention behind this series. The
'usb-c-connector' still needs the
graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)

>
> [2] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf

[3] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/connector/usb-connector.yaml#L23

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

On Thu, Jun 23, 2022 at 5:35 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-23 12:08:21)
> > > On Thu, Jun 23, 2022 at 11:30 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Prashant Malani (2022-06-22 10:34:30)
> > > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..78b0190c8543
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > @@ -0,0 +1,74 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > [...]
> > > > > +  ports:
> > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > > > +
> > > > > +    properties:
> > > > > +      port@0:
> > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > +        description: Link between the switch and a Type-C connector.
> > > >
> > > > Is there an update to the usb-c-connector binding to accept this port
> > > > connection?
> > >
> > > Not at this time. I don't think we should enforce that either.
> > > (Type-C data-lines could theoretically be routed through intermediate
> > > hardware like retimers/repeaters)
> >
> > I'm mostly wondering if having such a connection to the usb-c-connector,
> > or even through some retimer/repeater, would be sufficient to detect how
> > many type-c ports are connected to the device. If the type-c pin
> > assignments only support two or four lanes for DP then it seems like we
> > should describe the two lanes or four lanes as one graph endpoint
> > "output" and then have some 'data-lanes' property in case the DP lanes
> > are flipped while being sent to the retimer or usb-c-connector. This
> > would of course depend on the capability of the device, i.e. if it can
> > remap DP lanes or only has 2 lanes of DP, etc.
> >
> > > > > +  - |
> > > > > +    drm-bridge {
> > > > > +        usb-switch {
> > > > > +            compatible = "typec-switch";
> > > >
> > > > I still don't understand the subnode design here. usb-switch as a
> > > > container node indicates to me that this is a bus, but in earlier rounds
> > > > of this series it was stated this isn't a bus.
> > >
> > > I am not aware of this as a requirement. Can you please point me to the
> > > documentation that states this needs to be the case?
> >
> > I'm not aware of any documentation for the dos and don'ts here. Are
> > there any examples in the bindings directory that split up a device into
> > subnodes that isn't in bindings/mfd?
>
> usb-c-connector [3] and its users is an example.
>
> >  I just know from experience that
> > any time I try to make a child node of an existing node that I'm
> > supposed to be describing a bus, unless I'm adding some sort of
> > exception node like a graph binding or an opp table. Typically a node
> > corresponds 1:1 with a device in the kernel. I'll defer to Rob for any
> > citations.
> >
> > >
> > > > Why doesn't it work to
> > > > merge everything inside usb-switch directly into the drm-bridge node?
> > >
> > > I attempted to explain the rationale in the previous version [1], but
> > > using a dedicated sub-node means the driver doesn't haven't to
> > > inspect individual ports to determine which of them need switches
> > > registered for them. If it sees a `typec-switch`, it registers a
> > > mode-switch and/or orientation-switch. IMO it simplifies the hardware
> > > device binding too.
> >
> > How is that any harder than hard-coding that detail into the driver
> > about which port and endpoint is possibly connected to the
> > usb-c-connector (or retimer)? All of that logic could be behind some API
> > that registers a typec-switch based on a graph port number that's passed
> > in, ala drm_of_find_panel_or_bridge()'s design.
>
> If each driver has to do it (and the port specifics vary for each driver),
> it becomes an avoidable overhead for each of them.
> I prefer hard-coding such details if avoidable. I suppose both approaches
Sorry, I meant "I prefer not hard-coding such details..."

> require modifications to the binding and the driver code.
>
> >
> > Coming from a DT writer's perspective, I just want to go through the
> > list of output pins in the datasheet and match them up to the ports
> > binding for this device. If it's a pure DP bridge, where USB hardware
> > isn't an input or an output like the ITE chip, then I don't want to have
> > to describe a port graph binding for the case when it's connected to a
> > dp-connector (see dp-connector.yaml) in the top-level node and then have
> > to make an entirely different subnode for the usb-c-connector case with
> > a whole other set of graph ports.
>
> This approach still allows for that, if the driver has any use for it
> (AFAICT these drivers don't).
> Iff that driver uses it, one can (optionally) route their output
> (top-level) ports through the
> "typec-switch" sub-node (and onwards as required).
> If it's being used in a "pure-DP" configuration, the "typec-switch" just
> goes away (the top level ports can be routed as desired by the driver).
> (Again, I must reiterate that neither this driver or the anx driver
> utilizes this)
>
> >
> > How would I even know which two differential pairs correspond to port0
> > or port1 in this binding in the ITE case?
>
> Why do we need to know that? It doesn't affect this or the other
> driver or hardware's
> functioning in a perceivable way.
>
> > Ideally we make the graph
> > binding more strict for devices by enforcing that their graph ports
> > exist. Otherwise we're not fully describing the connections between
> > devices and our dtb checkers are going to let things through where the
> > driver most likely will fail because it can't figure out what to do,
> > e.g. display DP on 4 lanes or play some DP lane rerouting games to act
> > as a mux.
>
> How is the current binding enforcing this? The typec-switch binding
> as a first step ensures that the DT is connecting the hardware(anx,ite
> etc) to something
> that at least "claims" to be a Type-C switch.
>
> >
> > >
> > > It also maps with the internal block diagram for these hardware
> > > components (for ex. the anx7625 crosspoint switch is a separate
> > > sub-block within anx7625).
> >
> > We don't make DT bindings for sub-components like this very often. It
> > would make more sense to me to have a subnode if a typec switch was some
> > sort of off the shelf hard macro that the hardware engineer placed down
> > inside the IC that they delivered. Then we could have a completely
> > generic driver that binds to the generic binding that knows how to drive
> > the hardware, because it's an unchangeable hard macro with a well
> > defined programming interface.
> >
> > >
> > > [1] https://lore.kernel.org/linux-usb/CACeCKaeH6qTTdG_huC4yw0xxG8TYEOtfPW3tiVNwYs=P4QVPXg@mail.gmail.com/
> >
> > I looked at the fsa4480 driver and the device has a publicly available
> > datasheet[2]. That device is designed for "audio accessory mode" but I
> > guess it's being used to simply mux SBU lines? There isn't an upstream
> > user of the binding so far, but it also doesn't look like a complete
> > binding. I'd expect to see DN_L/R as a graph output connected to the
> > usb-c-connector and probably have a usb2.0 input port and a 'sound-dai'
> > property to represent the input audio path.
> >
> > Finally, simply connecting to the typec controller node isn't sufficient
> > because a typec controller can be controlling many usb-c-connectors so I
> > don't see how the graph binding would be able to figure out how many
> > usb-c-connectors are connected to a mux like device, unless we took the
> > approach of this patch.
>
> It can follow the endpoint of the typec-switch port (the port parent
> of the remote
> end-point would be a 'usb-c-connector'). That is if the graph binding
> (I'm assuming you mean the switch device here) wants to figure this
> out in the first place.
>
> > Is that why you're proposing this binding? To
> > avoid describing a graph binding in the usb-c-connector and effectively
> > "pushing" the port count up to the mux?
>
> No, that is not the intention behind this series. The
> 'usb-c-connector' still needs the
> graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
> muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)
>
> >
> > [2] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf
>
> [3] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/connector/usb-connector.yaml#L23

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Stephen Boyd]

Quoting Prashant Malani (2022-06-23 17:35:38)
> On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > I'm not aware of any documentation for the dos and don'ts here. Are
> > there any examples in the bindings directory that split up a device into
> > subnodes that isn't in bindings/mfd?
>
> usb-c-connector [3] and its users is an example.

What are the subnodes? The graph ports? That is not what I meant. I
meant splitting up a device functionality, like type-c and display
bridge, into subnodes. Composition of devices through DT bindings isn't
how it's done. Instead, we dump all the different functionality into the
same node. For example, look at the number of bindings that have both
#clock-cells and #reset-cells, when those are distinct frameworks in the
kernel and also different properties. We don't make subnodes to contain
the different functionality of a device.

And in this case I still don't see the point to making a subnode. The
API can simply setup a type-c switch based on a graph binding for the
toplevel node, e.g. the drm-bridge, and the driver can tell the API
which port+endpoint to use to search the graph for the usb-c-connector
to associate with the switch. We don't need to connect the graph within
the drm-bridge node to the graph within the typec-switch node to do
that. That's an internal detail of the drm-bridge that we don't expose
to DT, because the driver knows the detail. It also aligns the graph
binding for the top-level node with non-typec bindings, like drm, which
don't use a second level of graph binding to achieve essentially the
same thing when the output is connected to a DP connector.

> >
> > >
> > > > Why doesn't it work to
> > > > merge everything inside usb-switch directly into the drm-bridge node?
> > >
> > > I attempted to explain the rationale in the previous version [1], but
> > > using a dedicated sub-node means the driver doesn't haven't to
> > > inspect individual ports to determine which of them need switches
> > > registered for them. If it sees a `typec-switch`, it registers a
> > > mode-switch and/or orientation-switch. IMO it simplifies the hardware
> > > device binding too.
> >
> > How is that any harder than hard-coding that detail into the driver
> > about which port and endpoint is possibly connected to the
> > usb-c-connector (or retimer)? All of that logic could be behind some API
> > that registers a typec-switch based on a graph port number that's passed
> > in, ala drm_of_find_panel_or_bridge()'s design.
>
> If each driver has to do it (and the port specifics vary for each driver),
> it becomes an avoidable overhead for each of them.
> I prefer hard-coding such details if avoidable. I suppose both approaches
> require modifications to the binding and the driver code.

Ok, sounds like it is not any harder.

>
> >
> > Coming from a DT writer's perspective, I just want to go through the
> > list of output pins in the datasheet and match them up to the ports
> > binding for this device. If it's a pure DP bridge, where USB hardware
> > isn't an input or an output like the ITE chip, then I don't want to have
> > to describe a port graph binding for the case when it's connected to a
> > dp-connector (see dp-connector.yaml) in the top-level node and then have
> > to make an entirely different subnode for the usb-c-connector case with
> > a whole other set of graph ports.
>
> This approach still allows for that, if the driver has any use for it
> (AFAICT these drivers don't).
> Iff that driver uses it, one can (optionally) route their output
> (top-level) ports through the
> "typec-switch" sub-node (and onwards as required).
> If it's being used in a "pure-DP" configuration, the "typec-switch" just
> goes away (the top level ports can be routed as desired by the driver).
> (Again, I must reiterate that neither this driver or the anx driver
> utilizes this)
>
> >
> > How would I even know which two differential pairs correspond to port0
> > or port1 in this binding in the ITE case?
>
> Why do we need to know that? It doesn't affect this or the other
> driver or hardware's
> functioning in a perceivable way.

If the device registers allow control of the DP lane to physical pin
mapping, so that DP lane0 and DP lane1 can be swapped logically, then
we'll want to know which DP lanes we need to swap by writing some lane
remapping register in the device. Sometimes for routing purposes devices
support this lane remapping feature so the PCB can route the lines
directly to the connector instead of going in circles and destroying the
signal integrity.

>
> > Ideally we make the graph
> > binding more strict for devices by enforcing that their graph ports
> > exist. Otherwise we're not fully describing the connections between
> > devices and our dtb checkers are going to let things through where the
> > driver most likely will fail because it can't figure out what to do,
> > e.g. display DP on 4 lanes or play some DP lane rerouting games to act
> > as a mux.
>
> How is the current binding enforcing this? The typec-switch binding
> as a first step ensures that the DT is connecting the hardware(anx,ite
> etc) to something
> that at least "claims" to be a Type-C switch.

I'm simply saying that we can extend existing bindings like anx or ite
to have required properties for ports so that we know the driver will
find something on the other end of the graph. A binding that doesn't
have any ports will be invalid. I don't know if that's possible to do
in the schema.

>
> > Is that why you're proposing this binding? To
> > avoid describing a graph binding in the usb-c-connector and effectively
> > "pushing" the port count up to the mux?
>
> No, that is not the intention behind this series. The
> 'usb-c-connector' still needs the
> graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
> muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)

If the usb-c-connector still needs a graph binding to the typec-switch
then why isn't that part of this series?

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

On Thu, Jun 23, 2022 at 7:13 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-23 17:35:38)
> > On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > I'm not aware of any documentation for the dos and don'ts here. Are
> > > there any examples in the bindings directory that split up a device into
> > > subnodes that isn't in bindings/mfd?
> >
> > usb-c-connector [3] and its users is an example.
>
> What are the subnodes? The graph ports? That is not what I meant.

cros-ec-typec [4] uses subnodes of usb-c-connector. Chrome OS DTs
use the ports from the included usb-c-connector to switching hardware.

> I meant splitting up a device functionality, like type-c and display
> bridge, into subnodes. Composition of devices through DT bindings isn't
> how it's done. Instead, we dump all the different functionality into the
> same node. For example, look at the number of bindings that have both
> #clock-cells and #reset-cells, when those are distinct frameworks in the
> kernel and also different properties. We don't make subnodes to contain
> the different functionality of a device.
>
> And in this case I still don't see the point to making a subnode.

I've already provided my best effort at explaining the rationale.

> The
> API can simply setup a type-c switch based on a graph binding for the
> toplevel node, e.g. the drm-bridge, and the driver can tell the API
> which port+endpoint to use to search the graph for the usb-c-connector
> to associate with the switch.

OK, drm-bridge uses that approach. This is another approach. I didn't fully
understand why we *have* to follow what drm-bridge is doing.

> We don't need to connect the graph within
> the drm-bridge node to the graph within the typec-switch node to do
> that. That's an internal detail of the drm-bridge that we don't expose
> to DT, because the driver knows the detail.

I still don't understand why we can't do that. These devices have actual
hardware blocks that represent the Type-C switch functionality.

> It also aligns the graph
> binding for the top-level node with non-typec bindings, like drm, which
> don't use a second level of graph binding to achieve essentially the
> same thing when the output is connected to a DP connector.
>
> > >
> > > >
> > > > > Why doesn't it work to
> > > > > merge everything inside usb-switch directly into the drm-bridge node?
> > > >
> > > > I attempted to explain the rationale in the previous version [1], but
> > > > using a dedicated sub-node means the driver doesn't haven't to
> > > > inspect individual ports to determine which of them need switches
> > > > registered for them. If it sees a `typec-switch`, it registers a
> > > > mode-switch and/or orientation-switch. IMO it simplifies the hardware
> > > > device binding too.
> > >
> > > How is that any harder than hard-coding that detail into the driver
> > > about which port and endpoint is possibly connected to the
> > > usb-c-connector (or retimer)? All of that logic could be behind some API
> > > that registers a typec-switch based on a graph port number that's passed
> > > in, ala drm_of_find_panel_or_bridge()'s design.
> >
> > If each driver has to do it (and the port specifics vary for each driver),
> > it becomes an avoidable overhead for each of them.
> > I prefer hard-coding such details if avoidable. I suppose both approaches
> > require modifications to the binding and the driver code.
>
> Ok, sounds like it is not any harder.

I feel this approach is easier :)

>
> >
> > >
> > > Coming from a DT writer's perspective, I just want to go through the
> > > list of output pins in the datasheet and match them up to the ports
> > > binding for this device. If it's a pure DP bridge, where USB hardware
> > > isn't an input or an output like the ITE chip, then I don't want to have
> > > to describe a port graph binding for the case when it's connected to a
> > > dp-connector (see dp-connector.yaml) in the top-level node and then have
> > > to make an entirely different subnode for the usb-c-connector case with
> > > a whole other set of graph ports.
> >
> > This approach still allows for that, if the driver has any use for it
> > (AFAICT these drivers don't).
> > Iff that driver uses it, one can (optionally) route their output
> > (top-level) ports through the
> > "typec-switch" sub-node (and onwards as required).
> > If it's being used in a "pure-DP" configuration, the "typec-switch" just
> > goes away (the top level ports can be routed as desired by the driver).
> > (Again, I must reiterate that neither this driver or the anx driver
> > utilizes this)
> >
> > >
> > > How would I even know which two differential pairs correspond to port0
> > > or port1 in this binding in the ITE case?
> >
> > Why do we need to know that? It doesn't affect this or the other
> > driver or hardware's
> > functioning in a perceivable way.
>
> If the device registers allow control of the DP lane to physical pin
> mapping, so that DP lane0 and DP lane1 can be swapped logically, then
> we'll want to know which DP lanes we need to swap by writing some lane
> remapping register in the device. Sometimes for routing purposes devices
> support this lane remapping feature so the PCB can route the lines
> directly to the connector instead of going in circles and destroying the
> signal integrity.

Then add more end-points to port@1 (for each differential pair
you want to describe) of the usb-c-connector and route them
to the typec-switch accordingly.
FWIW I'm not aware of h/w *that supports DP alt mode* that uses the
functionality
you're referring to.

>
> >
> > > Ideally we make the graph
> > > binding more strict for devices by enforcing that their graph ports
> > > exist. Otherwise we're not fully describing the connections between
> > > devices and our dtb checkers are going to let things through where the
> > > driver most likely will fail because it can't figure out what to do,
> > > e.g. display DP on 4 lanes or play some DP lane rerouting games to act
> > > as a mux.
> >
> > How is the current binding enforcing this? The typec-switch binding
> > as a first step ensures that the DT is connecting the hardware(anx,ite
> > etc) to something
> > that at least "claims" to be a Type-C switch.
>
> I'm simply saying that we can extend existing bindings like anx or ite
> to have required properties for ports so that we know the driver will
> find something on the other end of the graph. A binding that doesn't
> have any ports will be invalid.

typec-switch requires a port.

I don't know if that's possible to do
> in the schema.
>
> >
> > > Is that why you're proposing this binding? To
> > > avoid describing a graph binding in the usb-c-connector and effectively
> > > "pushing" the port count up to the mux?
> >
> > No, that is not the intention behind this series. The
> > 'usb-c-connector' still needs the
> > graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
> > muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)
>
> If the usb-c-connector still needs a graph binding to the typec-switch
> then why isn't that part of this series?

That's not what I meant (what I meant earlier is the intention is not
what you stated).
I simply meant that the usb-c-connectors ports should be connected to
the typec-switch
ports. There isn't any binding update required for this.

[4] https://www.kernel.org/doc/Documentation/devicetree/bindings/chrome/google%2Ccros-ec-typec.yaml

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Stephen Boyd]

Quoting Prashant Malani (2022-06-23 19:48:04)
> On Thu, Jun 23, 2022 at 7:13 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-23 17:35:38)
> > > On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > I'm not aware of any documentation for the dos and don'ts here. Are
> > > > there any examples in the bindings directory that split up a device into
> > > > subnodes that isn't in bindings/mfd?
> > >
> > > usb-c-connector [3] and its users is an example.
> >
> > What are the subnodes? The graph ports? That is not what I meant.
>
> cros-ec-typec [4] uses subnodes of usb-c-connector. Chrome OS DTs
> use the ports from the included usb-c-connector to switching hardware.

Ok, got it. usb-c-connector nodes are children of the typec controller
(in this case cros-ec-typec) because otherwise we would need to make a
phandle link from the usb-c-connector node(s) under the root node / to
the typec controller. The phandle link may need to be done in both
directions, so it makes more sense to put the usb-c-connector nodes
underneath the typec controller to express the direct relationship
between the typec controller and the usb-c-connectors.

Furthermore, the usb-c-connector is not integrated as part of the EC in
the same package. There is a discrete part placed on the board that
corresponds to the usb-c-connector and that is separate from the EC. The
connectors are in essence only controllable through the EC because
that's the typec controller. It's similar to how we place i2c devices as
child nodes of the i2c controller.

>
> > I meant splitting up a device functionality, like type-c and display
> > bridge, into subnodes. Composition of devices through DT bindings isn't
> > how it's done. Instead, we dump all the different functionality into the
> > same node. For example, look at the number of bindings that have both
> > #clock-cells and #reset-cells, when those are distinct frameworks in the
> > kernel and also different properties. We don't make subnodes to contain
> > the different functionality of a device.
> >
> > And in this case I still don't see the point to making a subnode.
>
> I've already provided my best effort at explaining the rationale.
>
> > The
> > API can simply setup a type-c switch based on a graph binding for the
> > toplevel node, e.g. the drm-bridge, and the driver can tell the API
> > which port+endpoint to use to search the graph for the usb-c-connector
> > to associate with the switch.
>
> OK, drm-bridge uses that approach. This is another approach. I didn't fully
> understand why we *have* to follow what drm-bridge is doing.
>
> > We don't need to connect the graph within
> > the drm-bridge node to the graph within the typec-switch node to do
> > that. That's an internal detail of the drm-bridge that we don't expose
> > to DT, because the driver knows the detail.
>
> I still don't understand why we can't do that. These devices have actual
> hardware blocks that represent the Type-C switch functionality.
>

We don't break up device functionality for an IC into different subnodes
with different compatibles. Similarly, we don't describe internal
connection details of device nodes. The device driver that binds to the
compatible should know the details of the internal block diagram of the
part. The DT binding should describe the external connections of the
part and have properties that inform the driver about how the part was
integrated into the system (e.g. mode-switch). The unwritten DT mantra
is "less is more".

We could definitely make many subnodes and add properties for everything
inside an IC so that the DT describes the complete block diagram of the
part, but at that point the driver is a shell of its former self. The
driver will spend time parsing properties to learn details that are
entirely unchanging for the lifetime of the device (e.g. that the device
has typec switch capabilities); things that should be hard-coded in the
driver.

Of course, if the device is integrated into the system and doesn't need
to perform typec switching, then we want a property to tell the driver
that this device is integrated in a way that the typec switch is not
needed/used. Basically the driver should key that functionality off of
the presence of the 'mode-switch' or 'orientation-switch' property
instead of off the presence of a typec-switch subnode.

> > >
> > > >
> > > > How would I even know which two differential pairs correspond to port0
> > > > or port1 in this binding in the ITE case?
> > >
> > > Why do we need to know that? It doesn't affect this or the other
> > > driver or hardware's
> > > functioning in a perceivable way.
> >
> > If the device registers allow control of the DP lane to physical pin
> > mapping, so that DP lane0 and DP lane1 can be swapped logically, then
> > we'll want to know which DP lanes we need to swap by writing some lane
> > remapping register in the device. Sometimes for routing purposes devices
> > support this lane remapping feature so the PCB can route the lines
> > directly to the connector instead of going in circles and destroying the
> > signal integrity.
>
> Then add more end-points to port@1 (for each differential pair
> you want to describe) of the usb-c-connector and route them
> to the typec-switch accordingly.
> FWIW I'm not aware of h/w *that supports DP alt mode* that uses the
> functionality
> you're referring to.
>

The Qualcomm QMP usb+dp phy supports lane remapping.

> >
> > >
> > > > Is that why you're proposing this binding? To
> > > > avoid describing a graph binding in the usb-c-connector and effectively
> > > > "pushing" the port count up to the mux?
> > >
> > > No, that is not the intention behind this series. The
> > > 'usb-c-connector' still needs the
> > > graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
> > > muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)
> >
> > If the usb-c-connector still needs a graph binding to the typec-switch
> > then why isn't that part of this series?
>
> That's not what I meant (what I meant earlier is the intention is not
> what you stated).
> I simply meant that the usb-c-connectors ports should be connected to
> the typec-switch
> ports. There isn't any binding update required for this.
>

Ok. Got it.

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

On Fri, Jun 24, 2022 at 12:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-23 19:48:04)
> > On Thu, Jun 23, 2022 at 7:13 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Prashant Malani (2022-06-23 17:35:38)
> > > > On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > I'm not aware of any documentation for the dos and don'ts here. Are
> > > > > there any examples in the bindings directory that split up a device into
> > > > > subnodes that isn't in bindings/mfd?
> > > >
> > > > usb-c-connector [3] and its users is an example.
> > >
> > > What are the subnodes? The graph ports? That is not what I meant.
> >
> > cros-ec-typec [4] uses subnodes of usb-c-connector. Chrome OS DTs
> > use the ports from the included usb-c-connector to switching hardware.
>
> Ok, got it. usb-c-connector nodes are children of the typec controller
> (in this case cros-ec-typec) because otherwise we would need to make a
> phandle link from the usb-c-connector node(s) under the root node / to
> the typec controller. The phandle link may need to be done in both
> directions, so it makes more sense to put the usb-c-connector nodes
> underneath the typec controller to express the direct relationship
> between the typec controller and the usb-c-connectors.
>
> Furthermore, the usb-c-connector is not integrated as part of the EC in
> the same package. There is a discrete part placed on the board that
> corresponds to the usb-c-connector and that is separate from the EC. The
> connectors are in essence only controllable through the EC because
> that's the typec controller.

From the perspective of the AP, the `usb-c-connector` *is* an integrated part of
the Chrome EC; there is no alternative way to control it except
through the Chrome EC.
So the above example reinforces the usage model for typec-switch (which is
also an "integrated" component).

> It's similar to how we place i2c devices as
> child nodes of the i2c controller.
>
> >
> > > I meant splitting up a device functionality, like type-c and display
> > > bridge, into subnodes. Composition of devices through DT bindings isn't
> > > how it's done. Instead, we dump all the different functionality into the
> > > same node. For example, look at the number of bindings that have both
> > > #clock-cells and #reset-cells, when those are distinct frameworks in the
> > > kernel and also different properties. We don't make subnodes to contain
> > > the different functionality of a device.
> > >
> > > And in this case I still don't see the point to making a subnode.
> >
> > I've already provided my best effort at explaining the rationale.
> >
> > > The
> > > API can simply setup a type-c switch based on a graph binding for the
> > > toplevel node, e.g. the drm-bridge, and the driver can tell the API
> > > which port+endpoint to use to search the graph for the usb-c-connector
> > > to associate with the switch.
> >
> > OK, drm-bridge uses that approach. This is another approach. I didn't fully
> > understand why we *have* to follow what drm-bridge is doing.
> >
> > > We don't need to connect the graph within
> > > the drm-bridge node to the graph within the typec-switch node to do
> > > that. That's an internal detail of the drm-bridge that we don't expose
> > > to DT, because the driver knows the detail.
> >
> > I still don't understand why we can't do that. These devices have actual
> > hardware blocks that represent the Type-C switch functionality.
> >
>
> We don't break up device functionality for an IC into different subnodes
> with different compatibles. Similarly, we don't describe internal
> connection details of device nodes. The device driver that binds to the
> compatible should know the details of the internal block diagram of the
> part.

I don't completely agree with the above. There
is scope for middle-ground where some details can be codified into
DT bindings, and the driver can have the flexibility to be able to handle them.
But this now devolves into an ideological debate which I don't want
to get involved in, so I will restrict my responses on this subject.

> The DT binding should describe the external connections of the
> part and have properties that inform the driver about how the part was
> integrated into the system (e.g. mode-switch). The unwritten DT mantra
> is "less is more".
>
> We could definitely make many subnodes and add properties for everything
> inside an IC so that the DT describes the complete block diagram of the
> part, but at that point the driver is a shell of its former self.

That is a pathological/extreme argument which is not the case here,
we're just adding 1 sub-node because it's a sub-component that interfaces
with a kernel framework (Type-C class etc). The driver should be able to deal
with varying hardware configurations for the device and I don't believe that
makes it a "shell of its former self" any more than hard-coding port
details in the driver.

> The driver will spend time parsing properties to learn details that are

This parsing only occurs 1 once at probe, so I don't consider it much
of an overhead. The alternative suggested leads to the driver using time
looking up OF ports (with the port number). I fail to see how either is
noticeably more efficient than the other, especially on modern systems.

> entirely unchanging for the lifetime of the device (e.g. that the device
> has typec switch capabilities); things that should be hard-coded in the
> driver.
>
> Of course, if the device is integrated into the system and doesn't need
> to perform typec switching, then we want a property to tell the driver
> that this device is integrated in a way that the typec switch is not
> needed/used. Basically the driver should key that functionality off of
> the presence of the 'mode-switch' or 'orientation-switch' property
> instead of off the presence of a typec-switch subnode.
>
> > > >
> > > > >
> > > > > How would I even know which two differential pairs correspond to port0
> > > > > or port1 in this binding in the ITE case?
> > > >
> > > > Why do we need to know that? It doesn't affect this or the other
> > > > driver or hardware's
> > > > functioning in a perceivable way.
> > >
> > > If the device registers allow control of the DP lane to physical pin
> > > mapping, so that DP lane0 and DP lane1 can be swapped logically, then
> > > we'll want to know which DP lanes we need to swap by writing some lane
> > > remapping register in the device. Sometimes for routing purposes devices
> > > support this lane remapping feature so the PCB can route the lines
> > > directly to the connector instead of going in circles and destroying the
> > > signal integrity.
> >
> > Then add more end-points to port@1 (for each differential pair
> > you want to describe) of the usb-c-connector and route them
> > to the typec-switch accordingly.
> > FWIW I'm not aware of h/w *that supports DP alt mode* that uses the
> > functionality
> > you're referring to.
> >
>
> The Qualcomm QMP usb+dp phy supports lane remapping.

Ok great. So we can follow the method described above for specifying these
differential pairs if required. That is not related to this patch
series (although it is compatible
with it).

>
> > >
> > > >
> > > > > Is that why you're proposing this binding? To
> > > > > avoid describing a graph binding in the usb-c-connector and effectively
> > > > > "pushing" the port count up to the mux?
> > > >
> > > > No, that is not the intention behind this series. The
> > > > 'usb-c-connector' still needs the
> > > > graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
> > > > muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)
> > >
> > > If the usb-c-connector still needs a graph binding to the typec-switch
> > > then why isn't that part of this series?
> >
> > That's not what I meant (what I meant earlier is the intention is not
> > what you stated).
> > I simply meant that the usb-c-connectors ports should be connected to
> > the typec-switch
> > ports. There isn't any binding update required for this.
> >
>
> Ok. Got it.

This really is a limited binding change that helps describe connections
between Type-C components, helps these components integrate with
the kernel Type-C framework, and consolidates the associated properties.
I believe it works for most current use cases in the upstream kernel.

I'm happy to discuss more theoretical use cases further, but
respectfully, I prefer to do
so off-list.

If the maintainer is OK with it (Krzysztof has reviewed it, but I
don't want to presume
what the protocol is for patches in this subsystem), and we've
provided 2 users as asked for
in v4 [5], then I request its consideration for submission.
If the maintainers have further concerns, we'd be happy to address them.

Best regards,

-Prashant

[5] https://lore.kernel.org/linux-usb/20220616193424.GA3844759-robh@kernel.org/

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Stephen Boyd]

Quoting Prashant Malani (2022-06-24 14:41:36)
> On Fri, Jun 24, 2022 at 12:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-23 19:48:04)
> > > On Thu, Jun 23, 2022 at 7:13 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Prashant Malani (2022-06-23 17:35:38)
> > > > > On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > I'm not aware of any documentation for the dos and don'ts here. Are
> > > > > > there any examples in the bindings directory that split up a device into
> > > > > > subnodes that isn't in bindings/mfd?
> > > > >
> > > > > usb-c-connector [3] and its users is an example.
> > > >
> > > > What are the subnodes? The graph ports? That is not what I meant.
> > >
> > > cros-ec-typec [4] uses subnodes of usb-c-connector. Chrome OS DTs
> > > use the ports from the included usb-c-connector to switching hardware.
> >
> > Ok, got it. usb-c-connector nodes are children of the typec controller
> > (in this case cros-ec-typec) because otherwise we would need to make a
> > phandle link from the usb-c-connector node(s) under the root node / to
> > the typec controller. The phandle link may need to be done in both
> > directions, so it makes more sense to put the usb-c-connector nodes
> > underneath the typec controller to express the direct relationship
> > between the typec controller and the usb-c-connectors.
> >
> > Furthermore, the usb-c-connector is not integrated as part of the EC in
> > the same package. There is a discrete part placed on the board that
> > corresponds to the usb-c-connector and that is separate from the EC. The
> > connectors are in essence only controllable through the EC because
> > that's the typec controller.
>
> From the perspective of the AP, the `usb-c-connector` *is* an integrated part of
> the Chrome EC; there is no alternative way to control it except
> through the Chrome EC.
> So the above example reinforces the usage model for typec-switch (which is
> also an "integrated" component).

No the usb-c-connector is not an integrated part of the EC. It is a
discrete part with a part number that gets placed on the PCB. From the
perspective of the AP it is controlled via the EC, but it is not
integrated into the same package that the EC is packaged in to be
soldered down to the PCB.

So the example of usb-c-connector as a subnode doesn't reinforce the
argument for a typec-switch binding. It highlights the difference that
I've been trying to explain. The difference between internal vs.
external components of a device. In the EC case the usb-c-connector is
an external component from the EC, hence the two nodes. In the anx or
ite case the typec switch is an internal component, hence the single
node for the anx or ite part.

>
> This really is a limited binding change that helps describe connections
> between Type-C components, helps these components integrate with
> the kernel Type-C framework, and consolidates the associated properties.
> I believe it works for most current use cases in the upstream kernel.
>
> I'm happy to discuss more theoretical use cases further, but
> respectfully, I prefer to do
> so off-list.

I'm happy to move the discussion to the anx or ite bindings if you'd
prefer to discuss more concrete bindings.

>
> If the maintainer is OK with it (Krzysztof has reviewed it, but I
> don't want to presume
> what the protocol is for patches in this subsystem), and we've
> provided 2 users as asked for
> in v4 [5], then I request its consideration for submission.
> If the maintainers have further concerns, we'd be happy to address them.
>

Rob?

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Krzysztof Kozlowski]

On 24/06/2022 23:41, Prashant Malani wrote:
> On Fri, Jun 24, 2022 at 12:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> Quoting Prashant Malani (2022-06-23 19:48:04)
>>> On Thu, Jun 23, 2022 at 7:13 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>>>
>>>> Quoting Prashant Malani (2022-06-23 17:35:38)
>>>>> On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>>>>>
>>>>>> I'm not aware of any documentation for the dos and don'ts here. Are
>>>>>> there any examples in the bindings directory that split up a device into
>>>>>> subnodes that isn't in bindings/mfd?
>>>>>
>>>>> usb-c-connector [3] and its users is an example.
>>>>
>>>> What are the subnodes? The graph ports? That is not what I meant.
>>>
>>> cros-ec-typec [4] uses subnodes of usb-c-connector. Chrome OS DTs
>>> use the ports from the included usb-c-connector to switching hardware.
>>
>> Ok, got it. usb-c-connector nodes are children of the typec controller
>> (in this case cros-ec-typec) because otherwise we would need to make a
>> phandle link from the usb-c-connector node(s) under the root node / to
>> the typec controller. The phandle link may need to be done in both
>> directions, so it makes more sense to put the usb-c-connector nodes
>> underneath the typec controller to express the direct relationship
>> between the typec controller and the usb-c-connectors.
>>
>> Furthermore, the usb-c-connector is not integrated as part of the EC in
>> the same package. There is a discrete part placed on the board that
>> corresponds to the usb-c-connector and that is separate from the EC. The
>> connectors are in essence only controllable through the EC because
>> that's the typec controller.
> 
> From the perspective of the AP, the `usb-c-connector` *is* an integrated part of
> the Chrome EC; there is no alternative way to control it except
> through the Chrome EC.
> So the above example reinforces the usage model for typec-switch (which is
> also an "integrated" component).
> 
>> It's similar to how we place i2c devices as
>> child nodes of the i2c controller.
>>
>>>
>>>> I meant splitting up a device functionality, like type-c and display
>>>> bridge, into subnodes. Composition of devices through DT bindings isn't
>>>> how it's done. Instead, we dump all the different functionality into the
>>>> same node. For example, look at the number of bindings that have both
>>>> #clock-cells and #reset-cells, when those are distinct frameworks in the
>>>> kernel and also different properties. We don't make subnodes to contain
>>>> the different functionality of a device.
>>>>
>>>> And in this case I still don't see the point to making a subnode.
>>>
>>> I've already provided my best effort at explaining the rationale.
>>>
>>>> The
>>>> API can simply setup a type-c switch based on a graph binding for the
>>>> toplevel node, e.g. the drm-bridge, and the driver can tell the API
>>>> which port+endpoint to use to search the graph for the usb-c-connector
>>>> to associate with the switch.
>>>
>>> OK, drm-bridge uses that approach. This is another approach. I didn't fully
>>> understand why we *have* to follow what drm-bridge is doing.
>>>
>>>> We don't need to connect the graph within
>>>> the drm-bridge node to the graph within the typec-switch node to do
>>>> that. That's an internal detail of the drm-bridge that we don't expose
>>>> to DT, because the driver knows the detail.
>>>
>>> I still don't understand why we can't do that. These devices have actual
>>> hardware blocks that represent the Type-C switch functionality.
>>>
>>
>> We don't break up device functionality for an IC into different subnodes
>> with different compatibles. Similarly, we don't describe internal
>> connection details of device nodes. The device driver that binds to the
>> compatible should know the details of the internal block diagram of the
>> part.
> 
> I don't completely agree with the above. There
> is scope for middle-ground where some details can be codified into
> DT bindings, and the driver can have the flexibility to be able to handle them.
> But this now devolves into an ideological debate which I don't want
> to get involved in, so I will restrict my responses on this subject.
> 
>> The DT binding should describe the external connections of the
>> part and have properties that inform the driver about how the part was
>> integrated into the system (e.g. mode-switch). The unwritten DT mantra
>> is "less is more".
>>
>> We could definitely make many subnodes and add properties for everything
>> inside an IC so that the DT describes the complete block diagram of the
>> part, but at that point the driver is a shell of its former self.
> 
> That is a pathological/extreme argument which is not the case here,
> we're just adding 1 sub-node because it's a sub-component that interfaces
> with a kernel framework (Type-C class etc). The driver should be able to deal
> with varying hardware configurations for the device and I don't believe that
> makes it a "shell of its former self" any more than hard-coding port
> details in the driver.
> 
>> The driver will spend time parsing properties to learn details that are
> 
> This parsing only occurs 1 once at probe, so I don't consider it much
> of an overhead. The alternative suggested leads to the driver using time
> looking up OF ports (with the port number). I fail to see how either is
> noticeably more efficient than the other, especially on modern systems.
> 
>> entirely unchanging for the lifetime of the device (e.g. that the device
>> has typec switch capabilities); things that should be hard-coded in the
>> driver.
>>
>> Of course, if the device is integrated into the system and doesn't need
>> to perform typec switching, then we want a property to tell the driver
>> that this device is integrated in a way that the typec switch is not
>> needed/used. Basically the driver should key that functionality off of
>> the presence of the 'mode-switch' or 'orientation-switch' property
>> instead of off the presence of a typec-switch subnode.
>>
>>>>>
>>>>>>
>>>>>> How would I even know which two differential pairs correspond to port0
>>>>>> or port1 in this binding in the ITE case?
>>>>>
>>>>> Why do we need to know that? It doesn't affect this or the other
>>>>> driver or hardware's
>>>>> functioning in a perceivable way.
>>>>
>>>> If the device registers allow control of the DP lane to physical pin
>>>> mapping, so that DP lane0 and DP lane1 can be swapped logically, then
>>>> we'll want to know which DP lanes we need to swap by writing some lane
>>>> remapping register in the device. Sometimes for routing purposes devices
>>>> support this lane remapping feature so the PCB can route the lines
>>>> directly to the connector instead of going in circles and destroying the
>>>> signal integrity.
>>>
>>> Then add more end-points to port@1 (for each differential pair
>>> you want to describe) of the usb-c-connector and route them
>>> to the typec-switch accordingly.
>>> FWIW I'm not aware of h/w *that supports DP alt mode* that uses the
>>> functionality
>>> you're referring to.
>>>
>>
>> The Qualcomm QMP usb+dp phy supports lane remapping.
> 
> Ok great. So we can follow the method described above for specifying these
> differential pairs if required. That is not related to this patch
> series (although it is compatible
> with it).
> 
>>
>>>>
>>>>>
>>>>>> Is that why you're proposing this binding? To
>>>>>> avoid describing a graph binding in the usb-c-connector and effectively
>>>>>> "pushing" the port count up to the mux?
>>>>>
>>>>> No, that is not the intention behind this series. The
>>>>> 'usb-c-connector' still needs the
>>>>> graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
>>>>> muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)
>>>>
>>>> If the usb-c-connector still needs a graph binding to the typec-switch
>>>> then why isn't that part of this series?
>>>
>>> That's not what I meant (what I meant earlier is the intention is not
>>> what you stated).
>>> I simply meant that the usb-c-connectors ports should be connected to
>>> the typec-switch
>>> ports. There isn't any binding update required for this.
>>>
>>
>> Ok. Got it.
> 
> This really is a limited binding change that helps describe connections
> between Type-C components, helps these components integrate with
> the kernel Type-C framework, and consolidates the associated properties.
> I believe it works for most current use cases in the upstream kernel.
> 
> I'm happy to discuss more theoretical use cases further, but
> respectfully, I prefer to do
> so off-list.
> 
> If the maintainer is OK with it (Krzysztof has reviewed it, but I
> don't want to presume
> what the protocol is for patches in this subsystem), and we've
> provided 2 users as asked for

Although I reviewed it, but Stephen has legitimate concerns and they
should be addressed.

I guess Rob's feedback would be valuable here as well.

I think it would help if you articulated the exact problem, because
there is a quite a discussion. Do I understand correctly that the
bindings mimic USB connector and this is not appropriate for this type
of a device?

Or maybe this should not be represented in DT at all?

> in v4 [5], then I request its consideration for submission.
> If the maintainers have further concerns, we'd be happy to address them.
> 
> Best regards,
> 
> -Prashant
> 
> [5] https://lore.kernel.org/linux-usb/20220616193424.GA3844759-robh@kernel.org/


Best regards,
Krzysztof

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Rob Herring]

On Wed, Jun 22, 2022 at 05:34:30PM +0000, Prashant Malani wrote:
> Introduce a binding which represents a component that can control the
> routing of USB Type-C data lines as well as address data line
> orientation (based on CC lines' orientation).
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v4:
> - Added Reviewed-by tags.
> - Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
>   applied to usb-next)
> 
> Changes since v3:
> - No changes.
> 
> Changes since v2:
> - Added Reviewed-by and Tested-by tags.
> 
> Changes since v1:
> - Removed "items" from compatible.
> - Fixed indentation in example.
> 
>  .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> new file mode 100644
> index 000000000000..78b0190c8543
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: USB Type-C Switch
> +
> +maintainers:
> +  - Prashant Malani <pmalani@chromium.org>
> +
> +description:
> +  A USB Type-C switch represents a component which routes USB Type-C data
> +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> +  and cable are operating in. It can also modify lane routing based on
> +  the orientation of a connected Type-C peripheral.
> +
> +properties:
> +  compatible:
> +    const: typec-switch
> +
> +  mode-switch:
> +    type: boolean
> +    description: Specify that this switch can handle alternate mode switching.
> +
> +  orientation-switch:
> +    type: boolean
> +    description: Specify that this switch can handle orientation switching.
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description: OF graph binding modelling data lines to the Type-C switch.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Link between the switch and a Type-C connector.
> +
> +    required:
> +      - port@0
> +
> +required:
> +  - compatible
> +  - ports
> +
> +anyOf:
> +  - required:
> +      - mode-switch
> +  - required:
> +      - orientation-switch
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    drm-bridge {
> +        usb-switch {
> +            compatible = "typec-switch";

Unless this child is supposed to represent what the parent output is 
connected to, this is just wrong as, at least for the it6505 chip, it 
doesn't know anything about Type-C functionality. The bridge is 
just a protocol converter AFAICT. 

If the child node represents what the output is connected to (like a 
bus), then yes that is a pattern we have used. For example, a panel 
represented as child node of a display controller. However, that only 
works for simple cases, and is a pattern we have gotten away from in 
favor of using the graph binding.

I think Stephen and I are pretty much saying the same thing.

Rob

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

Hello Rob,

On Mon, Jun 27, 2022 at 2:04 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jun 22, 2022 at 05:34:30PM +0000, Prashant Malani wrote:
> > Introduce a binding which represents a component that can control the
> > routing of USB Type-C data lines as well as address data line
> > orientation (based on CC lines' orientation).
> >
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v4:
> > - Added Reviewed-by tags.
> > - Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
> >   applied to usb-next)
> >
> > Changes since v3:
> > - No changes.
> >
> > Changes since v2:
> > - Added Reviewed-by and Tested-by tags.
> >
> > Changes since v1:
> > - Removed "items" from compatible.
> > - Fixed indentation in example.
> >
> >  .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > new file mode 100644
> > index 000000000000..78b0190c8543
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > @@ -0,0 +1,74 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: USB Type-C Switch
> > +
> > +maintainers:
> > +  - Prashant Malani <pmalani@chromium.org>
> > +
> > +description:
> > +  A USB Type-C switch represents a component which routes USB Type-C data
> > +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> > +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> > +  and cable are operating in. It can also modify lane routing based on
> > +  the orientation of a connected Type-C peripheral.
> > +
> > +properties:
> > +  compatible:
> > +    const: typec-switch
> > +
> > +  mode-switch:
> > +    type: boolean
> > +    description: Specify that this switch can handle alternate mode switching.
> > +
> > +  orientation-switch:
> > +    type: boolean
> > +    description: Specify that this switch can handle orientation switching.
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +    description: OF graph binding modelling data lines to the Type-C switch.
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: Link between the switch and a Type-C connector.
> > +
> > +    required:
> > +      - port@0
> > +
> > +required:
> > +  - compatible
> > +  - ports
> > +
> > +anyOf:
> > +  - required:
> > +      - mode-switch
> > +  - required:
> > +      - orientation-switch
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +    drm-bridge {
> > +        usb-switch {
> > +            compatible = "typec-switch";
>
> Unless this child is supposed to represent what the parent output is
> connected to, this is just wrong as, at least for the it6505 chip, it
> doesn't know anything about Type-C functionality. The bridge is
> just a protocol converter AFAICT.

I'll let Pin-Yen comment on the specifics of the it6505 chip.

>
> If the child node represents what the output is connected to (like a
> bus), then yes that is a pattern we have used.

For the anx7625 case, the child node does represent what the output is connected
to (the usb-c-connector via the switch). Does that not qualify? Or do you mean
the child node should be a usb-c-connector itself?

> For example, a panel
> represented as child node of a display controller. However, that only
> works for simple cases, and is a pattern we have gotten away from in
> favor of using the graph binding.

The child node will still use a OF graph binding to connect to the
usb-c-connector.
Is that insufficient to consider a child node usage here?
By "using the graph binding", do you mean "only use the top-level ports" ?
I'm trying to clarify this, so that it will inform future versions and patches.

>
> I think Stephen and I are pretty much saying the same thing.
>
> Rob

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Rob Herring]

On Mon, Jun 27, 2022 at 02:43:39PM -0700, Prashant Malani wrote:
> Hello Rob,
> 
> On Mon, Jun 27, 2022 at 2:04 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Jun 22, 2022 at 05:34:30PM +0000, Prashant Malani wrote:
> > > Introduce a binding which represents a component that can control the
> > > routing of USB Type-C data lines as well as address data line
> > > orientation (based on CC lines' orientation).
> > >
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >
> > > Changes since v4:
> > > - Added Reviewed-by tags.
> > > - Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
> > >   applied to usb-next)
> > >
> > > Changes since v3:
> > > - No changes.
> > >
> > > Changes since v2:
> > > - Added Reviewed-by and Tested-by tags.
> > >
> > > Changes since v1:
> > > - Removed "items" from compatible.
> > > - Fixed indentation in example.
> > >
> > >  .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
> > >  1 file changed, 74 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > new file mode 100644
> > > index 000000000000..78b0190c8543
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > @@ -0,0 +1,74 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: USB Type-C Switch
> > > +
> > > +maintainers:
> > > +  - Prashant Malani <pmalani@chromium.org>
> > > +
> > > +description:
> > > +  A USB Type-C switch represents a component which routes USB Type-C data
> > > +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> > > +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> > > +  and cable are operating in. It can also modify lane routing based on
> > > +  the orientation of a connected Type-C peripheral.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: typec-switch
> > > +
> > > +  mode-switch:
> > > +    type: boolean
> > > +    description: Specify that this switch can handle alternate mode switching.
> > > +
> > > +  orientation-switch:
> > > +    type: boolean
> > > +    description: Specify that this switch can handle orientation switching.
> > > +
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: Link between the switch and a Type-C connector.
> > > +
> > > +    required:
> > > +      - port@0
> > > +
> > > +required:
> > > +  - compatible
> > > +  - ports
> > > +
> > > +anyOf:
> > > +  - required:
> > > +      - mode-switch
> > > +  - required:
> > > +      - orientation-switch
> > > +
> > > +additionalProperties: true
> > > +
> > > +examples:
> > > +  - |
> > > +    drm-bridge {
> > > +        usb-switch {
> > > +            compatible = "typec-switch";
> >
> > Unless this child is supposed to represent what the parent output is
> > connected to, this is just wrong as, at least for the it6505 chip, it
> > doesn't know anything about Type-C functionality. The bridge is
> > just a protocol converter AFAICT.
> 
> I'll let Pin-Yen comment on the specifics of the it6505 chip.

We're all waiting...

> > If the child node represents what the output is connected to (like a
> > bus), then yes that is a pattern we have used.
> 
> For the anx7625 case, the child node does represent what the output is connected
> to (the usb-c-connector via the switch). Does that not qualify? Or do you mean
> the child node should be a usb-c-connector itself?
> 
> > For example, a panel
> > represented as child node of a display controller. However, that only
> > works for simple cases, and is a pattern we have gotten away from in
> > favor of using the graph binding.
> 
> The child node will still use a OF graph binding to connect to the
> usb-c-connector.
> Is that insufficient to consider a child node usage here?
> By "using the graph binding", do you mean "only use the top-level ports" ?
> I'm trying to clarify this, so that it will inform future versions and patches.

What I want to see is block diagrams of possible h/w with different 
scenarios and then what the binding looks like in those cases. The 
switching/muxing could be in the SoC, a bridge chip, a Type C 
controller, a standalone mux chip, or ????. If you want a somewhat 
genericish binding, then you need to consider all of these.

I don't really have the b/w to work thru all this (and switch/mux is 
just one part of dealing with Type-C). This is just one of about a 
hundred patches I get to review a week.

Rob

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Pin-yen Lin]

On Wed, Jun 29, 2022 at 2:23 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jun 27, 2022 at 02:43:39PM -0700, Prashant Malani wrote:
> > Hello Rob,
> >
> > On Mon, Jun 27, 2022 at 2:04 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Jun 22, 2022 at 05:34:30PM +0000, Prashant Malani wrote:
> > > > Introduce a binding which represents a component that can control the
> > > > routing of USB Type-C data lines as well as address data line
> > > > orientation (based on CC lines' orientation).
> > > >
> > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > > ---
> > > >
> > > > Changes since v4:
> > > > - Added Reviewed-by tags.
> > > > - Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
> > > >   applied to usb-next)
> > > >
> > > > Changes since v3:
> > > > - No changes.
> > > >
> > > > Changes since v2:
> > > > - Added Reviewed-by and Tested-by tags.
> > > >
> > > > Changes since v1:
> > > > - Removed "items" from compatible.
> > > > - Fixed indentation in example.
> > > >
> > > >  .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
> > > >  1 file changed, 74 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > new file mode 100644
> > > > index 000000000000..78b0190c8543
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > @@ -0,0 +1,74 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: USB Type-C Switch
> > > > +
> > > > +maintainers:
> > > > +  - Prashant Malani <pmalani@chromium.org>
> > > > +
> > > > +description:
> > > > +  A USB Type-C switch represents a component which routes USB Type-C data
> > > > +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> > > > +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> > > > +  and cable are operating in. It can also modify lane routing based on
> > > > +  the orientation of a connected Type-C peripheral.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: typec-switch
> > > > +
> > > > +  mode-switch:
> > > > +    type: boolean
> > > > +    description: Specify that this switch can handle alternate mode switching.
> > > > +
> > > > +  orientation-switch:
> > > > +    type: boolean
> > > > +    description: Specify that this switch can handle orientation switching.
> > > > +
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > +        description: Link between the switch and a Type-C connector.
> > > > +
> > > > +    required:
> > > > +      - port@0
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - ports
> > > > +
> > > > +anyOf:
> > > > +  - required:
> > > > +      - mode-switch
> > > > +  - required:
> > > > +      - orientation-switch
> > > > +
> > > > +additionalProperties: true
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    drm-bridge {
> > > > +        usb-switch {
> > > > +            compatible = "typec-switch";
> > >
> > > Unless this child is supposed to represent what the parent output is
> > > connected to, this is just wrong as, at least for the it6505 chip, it
> > > doesn't know anything about Type-C functionality. The bridge is
> > > just a protocol converter AFAICT.
> >
> > I'll let Pin-Yen comment on the specifics of the it6505 chip.
>
> We're all waiting...

Yes it6505 is just a protocol converter. But in our use case, the output DP
lines are connected to the Type-C ports and the chip has to know which
port has DP Alt mode enabled. Does this justify a child node here?

Does it make more sense if we we eliminate the usb-switch node here
and list the ports in the top level?
>
> > > If the child node represents what the output is connected to (like a
> > > bus), then yes that is a pattern we have used.
> >
> > For the anx7625 case, the child node does represent what the output is connected
> > to (the usb-c-connector via the switch). Does that not qualify? Or do you mean
> > the child node should be a usb-c-connector itself?
> >
> > > For example, a panel
> > > represented as child node of a display controller. However, that only
> > > works for simple cases, and is a pattern we have gotten away from in
> > > favor of using the graph binding.
> >
> > The child node will still use a OF graph binding to connect to the
> > usb-c-connector.
> > Is that insufficient to consider a child node usage here?
> > By "using the graph binding", do you mean "only use the top-level ports" ?
> > I'm trying to clarify this, so that it will inform future versions and patches.
>
> What I want to see is block diagrams of possible h/w with different
> scenarios and then what the binding looks like in those cases. The
> switching/muxing could be in the SoC, a bridge chip, a Type C
> controller, a standalone mux chip, or ????. If you want a somewhat
> genericish binding, then you need to consider all of these.
>
> I don't really have the b/w to work thru all this (and switch/mux is
> just one part of dealing with Type-C). This is just one of about a
> hundred patches I get to review a week.
>
> Rob

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Pin-yen Lin]

On Wed, Jun 29, 2022 at 10:33 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> On Wed, Jun 29, 2022 at 2:23 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Jun 27, 2022 at 02:43:39PM -0700, Prashant Malani wrote:
> > > Hello Rob,
> > >
> > > On Mon, Jun 27, 2022 at 2:04 PM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Wed, Jun 22, 2022 at 05:34:30PM +0000, Prashant Malani wrote:
> > > > > Introduce a binding which represents a component that can control the
> > > > > routing of USB Type-C data lines as well as address data line
> > > > > orientation (based on CC lines' orientation).
> > > > >
> > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > > > ---
> > > > >
> > > > > Changes since v4:
> > > > > - Added Reviewed-by tags.
> > > > > - Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
> > > > >   applied to usb-next)
> > > > >
> > > > > Changes since v3:
> > > > > - No changes.
> > > > >
> > > > > Changes since v2:
> > > > > - Added Reviewed-by and Tested-by tags.
> > > > >
> > > > > Changes since v1:
> > > > > - Removed "items" from compatible.
> > > > > - Fixed indentation in example.
> > > > >
> > > > >  .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
> > > > >  1 file changed, 74 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..78b0190c8543
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > @@ -0,0 +1,74 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: USB Type-C Switch
> > > > > +
> > > > > +maintainers:
> > > > > +  - Prashant Malani <pmalani@chromium.org>
> > > > > +
> > > > > +description:
> > > > > +  A USB Type-C switch represents a component which routes USB Type-C data
> > > > > +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> > > > > +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> > > > > +  and cable are operating in. It can also modify lane routing based on
> > > > > +  the orientation of a connected Type-C peripheral.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: typec-switch
> > > > > +
> > > > > +  mode-switch:
> > > > > +    type: boolean
> > > > > +    description: Specify that this switch can handle alternate mode switching.
> > > > > +
> > > > > +  orientation-switch:
> > > > > +    type: boolean
> > > > > +    description: Specify that this switch can handle orientation switching.
> > > > > +
> > > > > +  ports:
> > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > > > +
> > > > > +    properties:
> > > > > +      port@0:
> > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > +        description: Link between the switch and a Type-C connector.
> > > > > +
> > > > > +    required:
> > > > > +      - port@0
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - ports
> > > > > +
> > > > > +anyOf:
> > > > > +  - required:
> > > > > +      - mode-switch
> > > > > +  - required:
> > > > > +      - orientation-switch
> > > > > +
> > > > > +additionalProperties: true
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    drm-bridge {
> > > > > +        usb-switch {
> > > > > +            compatible = "typec-switch";
> > > >
> > > > Unless this child is supposed to represent what the parent output is
> > > > connected to, this is just wrong as, at least for the it6505 chip, it
> > > > doesn't know anything about Type-C functionality. The bridge is
> > > > just a protocol converter AFAICT.
> > >
> > > I'll let Pin-Yen comment on the specifics of the it6505 chip.
> >
> > We're all waiting...
>
> Yes it6505 is just a protocol converter. But in our use case, the output DP
> lines are connected to the Type-C ports and the chip has to know which
> port has DP Alt mode enabled. Does this justify a child node here?
>
> Does it make more sense if we we eliminate the usb-switch node here
> and list the ports in the top level?
> >
> > > > If the child node represents what the output is connected to (like a
> > > > bus), then yes that is a pattern we have used.
> > >
> > > For the anx7625 case, the child node does represent what the output is connected
> > > to (the usb-c-connector via the switch). Does that not qualify? Or do you mean
> > > the child node should be a usb-c-connector itself?
> > >
> > > > For example, a panel
> > > > represented as child node of a display controller. However, that only
> > > > works for simple cases, and is a pattern we have gotten away from in
> > > > favor of using the graph binding.
> > >
> > > The child node will still use a OF graph binding to connect to the
> > > usb-c-connector.
> > > Is that insufficient to consider a child node usage here?
> > > By "using the graph binding", do you mean "only use the top-level ports" ?
> > > I'm trying to clarify this, so that it will inform future versions and patches.
> >
> > What I want to see is block diagrams of possible h/w with different
> > scenarios and then what the binding looks like in those cases. The
> > switching/muxing could be in the SoC, a bridge chip, a Type C
> > controller, a standalone mux chip, or ????. If you want a somewhat
> > genericish binding, then you need to consider all of these.

Then, is it suitable to put the switch binding into the drivers own bindings
(i.e., the bindings for it6505 and anx7625), and introduce some helper
functions to eliminate the duplication in the code?
Though we will have two similar bindings in two drivers with this approach.

> >
> > I don't really have the b/w to work thru all this (and switch/mux is
> > just one part of dealing with Type-C). This is just one of about a
> > hundred patches I get to review a week.
> >
> > Rob

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Rob Herring]

On Wed, Jun 29, 2022 at 9:01 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> On Wed, Jun 29, 2022 at 10:33 PM Pin-yen Lin <treapking@chromium.org> wrote:
> >
> > On Wed, Jun 29, 2022 at 2:23 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, Jun 27, 2022 at 02:43:39PM -0700, Prashant Malani wrote:
> > > > Hello Rob,
> > > >
> > > > On Mon, Jun 27, 2022 at 2:04 PM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Wed, Jun 22, 2022 at 05:34:30PM +0000, Prashant Malani wrote:
> > > > > > Introduce a binding which represents a component that can control the
> > > > > > routing of USB Type-C data lines as well as address data line
> > > > > > orientation (based on CC lines' orientation).
> > > > > >
> > > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > > > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > Changes since v4:
> > > > > > - Added Reviewed-by tags.
> > > > > > - Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
> > > > > >   applied to usb-next)
> > > > > >
> > > > > > Changes since v3:
> > > > > > - No changes.
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Added Reviewed-by and Tested-by tags.
> > > > > >
> > > > > > Changes since v1:
> > > > > > - Removed "items" from compatible.
> > > > > > - Fixed indentation in example.
> > > > > >
> > > > > >  .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
> > > > > >  1 file changed, 74 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..78b0190c8543
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > > @@ -0,0 +1,74 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: USB Type-C Switch
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Prashant Malani <pmalani@chromium.org>
> > > > > > +
> > > > > > +description:
> > > > > > +  A USB Type-C switch represents a component which routes USB Type-C data
> > > > > > +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> > > > > > +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> > > > > > +  and cable are operating in. It can also modify lane routing based on
> > > > > > +  the orientation of a connected Type-C peripheral.
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: typec-switch
> > > > > > +
> > > > > > +  mode-switch:
> > > > > > +    type: boolean
> > > > > > +    description: Specify that this switch can handle alternate mode switching.
> > > > > > +
> > > > > > +  orientation-switch:
> > > > > > +    type: boolean
> > > > > > +    description: Specify that this switch can handle orientation switching.
> > > > > > +
> > > > > > +  ports:
> > > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > > > > +
> > > > > > +    properties:
> > > > > > +      port@0:
> > > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > +        description: Link between the switch and a Type-C connector.
> > > > > > +
> > > > > > +    required:
> > > > > > +      - port@0
> > > > > > +
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - ports
> > > > > > +
> > > > > > +anyOf:
> > > > > > +  - required:
> > > > > > +      - mode-switch
> > > > > > +  - required:
> > > > > > +      - orientation-switch
> > > > > > +
> > > > > > +additionalProperties: true
> > > > > > +
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    drm-bridge {
> > > > > > +        usb-switch {
> > > > > > +            compatible = "typec-switch";
> > > > >
> > > > > Unless this child is supposed to represent what the parent output is
> > > > > connected to, this is just wrong as, at least for the it6505 chip, it
> > > > > doesn't know anything about Type-C functionality. The bridge is
> > > > > just a protocol converter AFAICT.
> > > >
> > > > I'll let Pin-Yen comment on the specifics of the it6505 chip.
> > >
> > > We're all waiting...
> >
> > Yes it6505 is just a protocol converter. But in our use case, the output DP
> > lines are connected to the Type-C ports and the chip has to know which
> > port has DP Alt mode enabled. Does this justify a child node here?
> >
> > Does it make more sense if we we eliminate the usb-switch node here
> > and list the ports in the top level?

In the it6505 node? No, the it6505 h/w knows nothing about Type-C
switching so neither should its binding.

What device controls the switching in this case? Again, block diagrams
please if you want advice on what the binding should look like.

> > > > > If the child node represents what the output is connected to (like a
> > > > > bus), then yes that is a pattern we have used.
> > > >
> > > > For the anx7625 case, the child node does represent what the output is connected
> > > > to (the usb-c-connector via the switch). Does that not qualify? Or do you mean
> > > > the child node should be a usb-c-connector itself?
> > > >
> > > > > For example, a panel
> > > > > represented as child node of a display controller. However, that only
> > > > > works for simple cases, and is a pattern we have gotten away from in
> > > > > favor of using the graph binding.
> > > >
> > > > The child node will still use a OF graph binding to connect to the
> > > > usb-c-connector.
> > > > Is that insufficient to consider a child node usage here?
> > > > By "using the graph binding", do you mean "only use the top-level ports" ?
> > > > I'm trying to clarify this, so that it will inform future versions and patches.
> > >
> > > What I want to see is block diagrams of possible h/w with different
> > > scenarios and then what the binding looks like in those cases. The
> > > switching/muxing could be in the SoC, a bridge chip, a Type C
> > > controller, a standalone mux chip, or ????. If you want a somewhat
> > > genericish binding, then you need to consider all of these.
>
> Then, is it suitable to put the switch binding into the drivers own bindings
> (i.e., the bindings for it6505 and anx7625), and introduce some helper
> functions to eliminate the duplication in the code?

Only for h/w devices that have switch h/w.

> Though we will have two similar bindings in two drivers with this approach.

While the anx7625 driver having Type-C awareness makes sense given it
has a switch and a type-C controller, that doesn't make sense for
it6505 (and every other bridge driver that's just providing DP
output).

Rob

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Stephen Boyd]

Quoting Rob Herring (2022-06-29 10:58:52)
> On Wed, Jun 29, 2022 at 9:01 AM Pin-yen Lin <treapking@chromium.org> wrote:
> > >
> > > Yes it6505 is just a protocol converter. But in our use case, the output DP
> > > lines are connected to the Type-C ports and the chip has to know which
> > > port has DP Alt mode enabled. Does this justify a child node here?
> > >
> > > Does it make more sense if we we eliminate the usb-switch node here
> > > and list the ports in the top level?
>
> In the it6505 node? No, the it6505 h/w knows nothing about Type-C
> switching so neither should its binding.
>
> What device controls the switching in this case? Again, block diagrams
> please if you want advice on what the binding should look like.

My understanding is there are 4 DP lanes on it6505 and two lanes are
connected to one usb-c-connector and the other two lanes are connected
to a different usb-c-connector. The IT6505 driver will send DP out on
the associated two DP lanes depending on which usb-c-connector has DP
pins assigned by the typec manager.

        					     +-------+
        					     |       |
          +--------+                            /----+ usb-c |
          | IT6505 |                           / /---+       |
          |        +------------ lane 0 ------/ /    |       |
          |        +------------ lane 1 -------/     +-------+
 DPI -----+        |
          |        |                                 +-------+
          |        |                                 |       |
          |        +------------ lane 2 -------------+ usb-c |
          |        +------------ lane 3 -------------+       |
          |        |                                 |       |
          +--------+                                 +-------+

The bridge is a mux that steers DP to one or the other usb-c-connector
based on what the typec manager decides.

I would expect this to be described with the existing port binding in
the it6505 node. The binding would need to be extended to describe the
output side.

        bridge@5c {
            compatible = "ite,it6505";
	    ...

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    it6505_in: endpoint {
                        remote-endpoint = <&dpi_out>;
                    };
                };

                port@1 {
                    #address-cells = <1>;
                    #size-cells = <0>;
                    reg = <1>;

                    it6505_out_lanes_01: endpoint@0 {
                        reg = <0>
                        data-lanes = <0 1>;
                        remote-endpoint = <&typec0>;
                    };

                    it6505_out_lanes_23: endpoint@1 {
                        reg = <1>
                        data-lanes = <2 3>;
                        remote-endpoint = <&typec1>;
                    };
                };
            };
        };

        usb-c-connector {
            compatible = "usb-c-connector";
            ....
            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@1 {
                    reg = <1>;
                    typec0: endpoint {
                        remote-endpoint = <&it6505_out_lanes_01>;
                    };
                };
            };
        };

Note: port@1 in usb-c-connector above is for superspeed lines, which
technically DP reuses. But we can also shove USB3 superspeed lines over
the other two superspeed pins in the usb-c-connector pinout. Probably
port@1 should have two endpoints so we can connect usb superspeed lines
there in addition to DP lines.

Another use case would be to have the IT6505 send 4 lanes of DP to a
dp-connector. Or send one lane of DP to 4 dp-connectors? Sounds awful but
possible if this bridge can drive one lane DP out on any DP output lane.

        bridge@5c {
            compatible = "ite,it6505";
	    ...

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    it6505_in: endpoint {
                        remote-endpoint = <&dpi_out>;
                    };
                };

                port@1 {
                    #address-cells = <1>;
                    #size-cells = <0>;
                    reg = <1>;

                    it6505_out_lane_0: endpoint@0 {
                        reg = <0>
                        data-lanes = <0>;
                        remote-endpoint = <&dp0>;
                    };

                    it6505_out_lane_1: endpoint@1 {
                        reg = <1>
                        data-lanes = <1>;
                        remote-endpoint = <&dp1>;
                    };

                    it6505_out_lane_2: endpoint@2 {
                        reg = <2>
                        data-lanes = <2>;
                        remote-endpoint = <&dp1>;
                    };

                    it6505_out_lane_3: endpoint@3 {
                        reg = <3>
                        data-lanes = <3>;
                        remote-endpoint = <&dp1>;
                    };
                };
            };
        };


Or we could send 4 lanes of DP to a typec redriver that's controlled by
firmware outside of the kernel where the redriver takes in 4 lanes DP
and USB3 and muxes USB or USB+DP or just DP.

        bridge@5c {
            compatible = "ite,it6505";
	    ...

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    it6505_in: endpoint {
                        remote-endpoint = <&dpi_out>;
                    };
                };

                port@1 {
                    #address-cells = <1>;
                    #size-cells = <0>;
                    reg = <1>;

                    it6505_out: endpoint@0 {
                        reg = <0>
                        data-lanes = <0 1 2 3>;
                        remote-endpoint = <&cros_ec_typec_dp_in>;
                    };
                };
            };
        };

	cros_ec@0 {
	    compatible = "google,cros-ec-spi";
	    ...

	    cros_ec_typec: typec {
	         compatible = "google,cros-ec-typec";
		 ports {
                     #address-cells = <1>;
                     #size-cells = <0>;

                     port@0 {
		         cros_ec_typec_dp_in: endpoint {
			     remote-endpoint = <&it6505_out>;
			 };
                     };
		 };
	    };
	};

In this case we would want to have cros_ec_typec describe some sort of
input graph binding to accept DP. I imagine the EC as a dp-bridge in
this scenario, where it takes in DP and muxes it along with USB onto a
usb-c-connector. If the EC is managing multiple usb-c-connectors then
the typec framework may need help determining which port DP is going to,
especially in the case that two DP bridges are used and they're both
sent to the EC so that the EC can mux them onto a usb-c-connector along
with USB.

(I can draw more block diagrams if you prefer)

>
> > > > > > If the child node represents what the output is connected to (like a
> > > > > > bus), then yes that is a pattern we have used.
> > > > >
> > > > > For the anx7625 case, the child node does represent what the output is connected
> > > > > to (the usb-c-connector via the switch). Does that not qualify? Or do you mean
> > > > > the child node should be a usb-c-connector itself?
> > > > >
> > > > > > For example, a panel
> > > > > > represented as child node of a display controller. However, that only
> > > > > > works for simple cases, and is a pattern we have gotten away from in
> > > > > > favor of using the graph binding.
> > > > >
> > > > > The child node will still use a OF graph binding to connect to the
> > > > > usb-c-connector.
> > > > > Is that insufficient to consider a child node usage here?
> > > > > By "using the graph binding", do you mean "only use the top-level ports" ?
> > > > > I'm trying to clarify this, so that it will inform future versions and patches.
> > > >
> > > > What I want to see is block diagrams of possible h/w with different
> > > > scenarios and then what the binding looks like in those cases. The
> > > > switching/muxing could be in the SoC, a bridge chip, a Type C
> > > > controller, a standalone mux chip, or ????. If you want a somewhat
> > > > genericish binding, then you need to consider all of these.
> >
> > Then, is it suitable to put the switch binding into the drivers own bindings
> > (i.e., the bindings for it6505 and anx7625), and introduce some helper
> > functions to eliminate the duplication in the code?
>
> Only for h/w devices that have switch h/w.
>
> > Though we will have two similar bindings in two drivers with this approach.
>
> While the anx7625 driver having Type-C awareness makes sense given it
> has a switch and a type-C controller, that doesn't make sense for
> it6505 (and every other bridge driver that's just providing DP
> output).
>

I don't see the benefit to making a genericish binding for typec
switches, even if the hardware has typec awareness like anx7625. It
looks like the graph binding can already handle what we need. By putting
it in the top-level ports node we have one way to describe the
input/output of the device instead of describing it in the top-level in
the display connector case and the child typec switch node in the usb c
connector case.

I think the difficulty comes from the combinatorial explosion of
possible configurations. As evidenced here, hardware engineers can take
a DP bridge and use it as a DP mux as long as the bridge has lane
control. Or they can take a device like anx7625 and ignore the USB
aspect and use the internal crosspoint switch as a DP mux. The anx7625
part could be a MIPI-to-DP display bridge plus mux that is connected to
two dp-connectors, in which case typec isn't even involved, but we could
mux between two dp connectors.

Also, the typec framework would like to simply walk the graph from the
usb-c-connector looking for nodes that have 'mode-switch' or
'orientation-switch' properties and treat those devices as the typec
switches for the connector. This means that we have to add these typec
properties like 'mode-switch' to something like the IT6505 bridge
binding, which is a little awkward. I wonder if those properties aren't
really required. Would it be sufficient if the framework could walk the
graph and look for registered typec switches in the kernel that have a
matching of_node?

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

On Wed, Jun 29, 2022 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > What device controls the switching in this case? Again, block diagrams
> > please if you want advice on what the binding should look like.
>
> My understanding is there are 4 DP lanes on it6505 and two lanes are
> connected to one usb-c-connector and the other two lanes are connected
> to a different usb-c-connector. The IT6505 driver will send DP out on
> the associated two DP lanes depending on which usb-c-connector has DP
> pins assigned by the typec manager.
>
>                                                      +-------+
>                                                      |       |
>           +--------+                            /----+ usb-c |
>           | IT6505 |                           / /---+       |
>           |        +------------ lane 0 ------/ /    |       |
>           |        +------------ lane 1 -------/     +-------+
>  DPI -----+        |
>           |        |                                 +-------+
>           |        |                                 |       |
>           |        +------------ lane 2 -------------+ usb-c |
>           |        +------------ lane 3 -------------+       |
>           |        |                                 |       |
>           +--------+                                 +-------+
>
> The bridge is a mux that steers DP to one or the other usb-c-connector
> based on what the typec manager decides.
>
> I would expect this to be described with the existing port binding in
> the it6505 node. The binding would need to be extended to describe the
> output side.
>
>         bridge@5c {
>             compatible = "ite,it6505";

We'll need a top level "mode-switch" property here.
>             ...
>
>             ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                     reg = <0>;
>                     it6505_in: endpoint {
>                         remote-endpoint = <&dpi_out>;
>                     };
>                 };
>
>                 port@1 {
>                     #address-cells = <1>;
>                     #size-cells = <0>;
>                     reg = <1>;
>
>                     it6505_out_lanes_01: endpoint@0 {
>                         reg = <0>
>                         data-lanes = <0 1>;
>                         remote-endpoint = <&typec0>;
>                     };
>
>                     it6505_out_lanes_23: endpoint@1 {
>                         reg = <1>
>                         data-lanes = <2 3>;
>                         remote-endpoint = <&typec1>;
>                     };
>                 };
>             };
>         };
>
>         usb-c-connector {
>             compatible = "usb-c-connector";
>             ....
>             ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@1 {
>                     reg = <1>;
>                     typec0: endpoint {
>                         remote-endpoint = <&it6505_out_lanes_01>;
>                     };
>                 };
>             };
>         };

We can adopt this binding, but from what I gathered in this thread, that
shouldn't be done, because IT6505 isn't meant to be aware of Type-C
connections at all.

>
> I don't see the benefit to making a genericish binding for typec
> switches, even if the hardware has typec awareness like anx7625. It
> looks like the graph binding can already handle what we need. By putting
> it in the top-level ports node we have one way to describe the
> input/output of the device instead of describing it in the top-level in
> the display connector case and the child typec switch node in the usb c
> connector case.

Ack, I'll drop the generic binding for future revisions.

>
> I think the difficulty comes from the combinatorial explosion of
> possible configurations. As evidenced here, hardware engineers can take
> a DP bridge and use it as a DP mux as long as the bridge has lane
> control. Or they can take a device like anx7625 and ignore the USB
> aspect and use the internal crosspoint switch as a DP mux. The anx7625
> part could be a MIPI-to-DP display bridge plus mux that is connected to
> two dp-connectors, in which case typec isn't even involved, but we could
> mux between two dp connectors.

Each containing a single DP lane, right?
I think that will not be a valid configuration, since there is only 1 HPD
pin (so it's assuming both DP lanes go to the same DP sink).

But yes, your larger point is valid: h/w engineers can repurpose these
bridges in ways the datasheet doesn't originally anticipate.

>
> Also, the typec framework would like to simply walk the graph from the
> usb-c-connector looking for nodes that have 'mode-switch' or
> 'orientation-switch' properties and treat those devices as the typec
> switches for the connector. This means that we have to add these typec
> properties like 'mode-switch' to something like the IT6505 bridge
> binding, which is a little awkward. I wonder if those properties aren't
> really required. Would it be sufficient if the framework could walk the
> graph and look for registered typec switches in the kernel that have a
> matching of_node?

My interpretation of the current mode-switch search code [1] is that
a top level property of "mode-switch" is required.

[1] https://elixir.bootlin.com/linux/v5.19-rc4/source/drivers/usb/typec/mux.c#L347

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Stephen Boyd]

Quoting Prashant Malani (2022-06-29 15:55:10)
> On Wed, Jun 29, 2022 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > My understanding is there are 4 DP lanes on it6505 and two lanes are
> > connected to one usb-c-connector and the other two lanes are connected
> > to a different usb-c-connector. The IT6505 driver will send DP out on
> > the associated two DP lanes depending on which usb-c-connector has DP
> > pins assigned by the typec manager.
[...]
>
> We can adopt this binding, but from what I gathered in this thread, that
> shouldn't be done, because IT6505 isn't meant to be aware of Type-C
> connections at all.

How will the driver know which usb-c-connector to route DP to without
making the binding aware of typec connections?

> >
> > I think the difficulty comes from the combinatorial explosion of
> > possible configurations. As evidenced here, hardware engineers can take
> > a DP bridge and use it as a DP mux as long as the bridge has lane
> > control. Or they can take a device like anx7625 and ignore the USB
> > aspect and use the internal crosspoint switch as a DP mux. The anx7625
> > part could be a MIPI-to-DP display bridge plus mux that is connected to
> > two dp-connectors, in which case typec isn't even involved, but we could
> > mux between two dp connectors.
>
> Each containing a single DP lane, right?

Yes.

> I think that will not be a valid configuration, since there is only 1 HPD
> pin (so it's assuming both DP lanes go to the same DP sink).

HPD can be signalled out of band, or not at all (no-hpd). I suspect it's
valid to ignore/disconnect the HPD pin here and start/stop DP when, for
example, the HPD pin toggles within a dp-connector. HPD could be
signaled directly to the kernel via an out of band gpio going from the
dp-connector to the SoC. In this case HPD for each dp-connector could be
a different gpio and the driver may be required to arbitrate between the
two dp-connectors with some 'first to signal wins' logic or something.

>
> But yes, your larger point is valid: h/w engineers can repurpose these
> bridges in ways the datasheet doesn't originally anticipate.

Yeah, I'm also trying to say that devices with typec logic may not be
used for typec purposes.

>
> >
> > Also, the typec framework would like to simply walk the graph from the
> > usb-c-connector looking for nodes that have 'mode-switch' or
> > 'orientation-switch' properties and treat those devices as the typec
> > switches for the connector. This means that we have to add these typec
> > properties like 'mode-switch' to something like the IT6505 bridge
> > binding, which is a little awkward. I wonder if those properties aren't
> > really required. Would it be sufficient if the framework could walk the
> > graph and look for registered typec switches in the kernel that have a
> > matching of_node?
>
> My interpretation of the current mode-switch search code [1] is that
> a top level property of "mode-switch" is required.

Yeah that's how it is right now, but does it have to stay that way?
Could the code search the graph and look for a matching node that's
registered with the typec framework? The goal is to avoid adding typec
properties like 'mode-switch' to bindings like bridge converters that
aren't expected to work with typec. Hopefully the binding can be written
with the output pins in mind and what modes on those pins the hardware
supports (e.g. two lanes on anx7625 can't be split apart whereas on
it6505 each pair can be used directly).

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

(CC+ Bjorn)

On Wed, Jun 29, 2022 at 4:55 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-29 15:55:10)
> > On Wed, Jun 29, 2022 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > My understanding is there are 4 DP lanes on it6505 and two lanes are
> > > connected to one usb-c-connector and the other two lanes are connected
> > > to a different usb-c-connector. The IT6505 driver will send DP out on
> > > the associated two DP lanes depending on which usb-c-connector has DP
> > > pins assigned by the typec manager.
> [...]
> >
> > We can adopt this binding, but from what I gathered in this thread, that
> > shouldn't be done, because IT6505 isn't meant to be aware of Type-C
> > connections at all.
>
> How will the driver know which usb-c-connector to route DP to without
> making the binding aware of typec connections?

I agree with you; I'm saying my interpretation of the comments of this
thread are that it's not the intended usage of the it6505 part, so the driver
shouldn't be updated to support that.

>
> HPD can be signalled out of band, or not at all (no-hpd). I suspect it's
> valid to ignore/disconnect the HPD pin here and start/stop DP when, for
> example, the HPD pin toggles within a dp-connector. HPD could be
> signaled directly to the kernel via an out of band gpio going from the
> dp-connector to the SoC. In this case HPD for each dp-connector could be
> a different gpio and the driver may be required to arbitrate between the
> two dp-connectors with some 'first to signal wins' logic or something.

Sure, it's possible. I just didn't see anything in the anx7625 datasheet
to suggest it supported 2x1-lane DP outputs.

For that matter I don't think even it6505 supports > 1 DP sink (based
on my reading of the datasheet), but I don't have too much experience
with these parts.


> > My interpretation of the current mode-switch search code [1] is that
> > a top level property of "mode-switch" is required.
>
> Yeah that's how it is right now, but does it have to stay that way?
> Could the code search the graph and look for a matching node that's
> registered with the typec framework?

I'll have to get back to you on that after reading the code a bit more.
Maybe Heikki or Bjorn have some comments about it.
The ACPI Type-C ports do require a device handle labelled "mode-switch"
which points to the switch device.

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Rob Herring]

On Thu, Jun 30, 2022 at 10:10:32AM -0700, Prashant Malani wrote:
> (CC+ Bjorn)
> 
> On Wed, Jun 29, 2022 at 4:55 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-29 15:55:10)
> > > On Wed, Jun 29, 2022 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > My understanding is there are 4 DP lanes on it6505 and two lanes are
> > > > connected to one usb-c-connector and the other two lanes are connected
> > > > to a different usb-c-connector. The IT6505 driver will send DP out on
> > > > the associated two DP lanes depending on which usb-c-connector has DP
> > > > pins assigned by the typec manager.
> > [...]
> > >
> > > We can adopt this binding, but from what I gathered in this thread, that
> > > shouldn't be done, because IT6505 isn't meant to be aware of Type-C
> > > connections at all.
> >
> > How will the driver know which usb-c-connector to route DP to without
> > making the binding aware of typec connections?
> 
> I agree with you; I'm saying my interpretation of the comments of this
> thread are that it's not the intended usage of the it6505 part, so the driver
> shouldn't be updated to support that.

That's not the right interpretation. There should not be some Type-C 
specific child mux/switch node because the device has no such h/w within 
it. Assuming all the possibilities Stephen outlined are valid, it's 
clear this lane selection has nothing to do with Type-C. It does have an 
output port for its DP output already and using that to describe the 
connection to DP connector(s) and/or Type-C connector(s) should be 
handled.

Whether the driver is type-C aware is a separate question from the 
binding. I would think the driver just needs to be told (or it can ask) 
which endpoint should be active and it just enables output on the
corresponding lanes for that endpoint. I'm not sure if all DP bridge 
chips have the same flexibility on their output lanes, but I would 
assume many do and we don't want to be duplicating the same code to 
handle that in every bridge driver.

Rob

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

On Tue, Jul 12, 2022 at 10:45 AM Rob Herring <robh@kernel.org> wrote:
>
> > I agree with you; I'm saying my interpretation of the comments of this
> > thread are that it's not the intended usage of the it6505 part, so the driver
> > shouldn't be updated to support that.
>
> That's not the right interpretation. There should not be some Type-C
> specific child mux/switch node because the device has no such h/w within
> it. Assuming all the possibilities Stephen outlined are valid, it's
> clear this lane selection has nothing to do with Type-C. It does have an
> output port for its DP output already and using that to describe the
> connection to DP connector(s) and/or Type-C connector(s) should be
> handled.

Got it. Thanks for the clarification.

>
> Whether the driver is type-C aware is a separate question from the
> binding. I would think the driver just needs to be told (or it can ask)
> which endpoint should be active and it just enables output on the
> corresponding lanes for that endpoint.

Is it acceptable to tag the end-points with a "mode-switch" /
"orientation-switch"
property? Something like the following:

```
        dp-bridge@5c {
            compatible = "ite,it6505";
            ...
            port {
                #adderss-cells = <1>;
                #size-cells = <0>;

                ite_typec0: endpoint@0 {
                    reg = <0>;
                    mode-switch;
                    remote-endpoint = <&typec_connector0>;
                };
                ite_typec1: endpoint@1 {
                    reg = <1>;
                    mode-switch;
                    remote-endpoint = <&typec_connector1>;
                };
            };
        };
```
Or should the DRM bridge device binding not have those properties in
the end-points either?
The reasons those are required are:
- The Type-C matching code looks for the "mode-switch" identifier in
the fwnode while performing the switch matching [1]
- While we can look up whether the remote-endpoint is a
"usb-c-connector" in the bridge driver the
"mode-switch"/"orientation-switch" property tells the bridge driver
whether to register just a mode-switch, an orientation switch or both.

Best regards,

- Prashant

[1] https://elixir.bootlin.com/linux/v5.19-rc6/source/drivers/usb/typec/mux.c#L347

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

Hi Rob,

On Jul 12 11:45, Rob Herring wrote:
> 
> That's not the right interpretation. There should not be some Type-C 
> specific child mux/switch node because the device has no such h/w within 
> it. Assuming all the possibilities Stephen outlined are valid, it's 
> clear this lane selection has nothing to do with Type-C. It does have an 
> output port for its DP output already and using that to describe the 
> connection to DP connector(s) and/or Type-C connector(s) should be 
> handled.
> Rob

Below I've listed the proposal binding (for the Type-C connector) along
with 2 sample hardware diagrams and corresponding DT.

The updated binding in usb-c-connector would be as follows:

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
index ae515651fc6b..a043b09cb8ec 100644
--- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
+++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
@@ -183,6 +183,30 @@ properties:
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
         description: Super Speed (SS), present in SS capable connectors.
+        properties:
+          '#address-cells':
+            const: 1
+
+          '#size-cells':
+            const: 0
+
+        patternProperties:
+          "^endpoint@[0-1]$":
+            $ref: /schemas/graph.yaml#/$defs/endpoint-base
+            description:
+              Endpoints for the two SS lanes. endpoint@0 refers to SSTRX1 (A2,A3,B10,B11)
+              and endpoint@1 refers to SSTRX2 (B2,B3,A10,A11).
+            additionalProperties: false
+
+              properties:
+                reg:
+                  maxItems: 1
+
+                remote-endpoint: true
+
+              required:
+                - reg
+                - remote-endpoint

       port@2:
         $ref: /schemas/graph.yaml#/properties/port

Here are 2 examples of how that would look on some existing hardware:

Example 1. 2 usb-c-connectors connecting to 1 drm bridge / DP switch:

Here is the diagram we are using on the MTK platform:

                 SOC
        +---------------------+                                              C0
        |                     |            +----------+       2 lane      +--------+
        |                     |            |          +---------/---------+ SSTRX1 |
        |                     |            |          |                   |        |
        |    MIPI DPI         |            |          |  2 lane           |        |
        |                     +------------+ ANX 7625 +---/-----+    +----+ SSTRX2 |
        |                     |            |          |         |    |    +--------+
        |                     |            +----------+         |    |
        +---------------------+                                 |    |
        |                     |            +----------+ 2 lane  |    |       C1
        |                     |            |          +----/----C----+    +--------+
        |    USB3 HC          |   2 lane   |          |         |         | SSTRX1 |
        |                     +-----/------+ USB3 HUB |         +---------+        |
        |  (host controller)  |            |          |       2 lane      |        |
        |                     |            |          +---------/---------+ SSTRX2 |
        +---------------------+            |          |                   |        |
                                           +----------+                   +--------+

Some platforms use it6505, so that can be swapped in for anx7625
without any change to the rest of the hardware diagram.

From the above, we can see that it is helpful to describe the
Type-C SS lines as 2 endpoints:
- 1 for SSTX1+SSRX1 (A2,A3 + B10,B11)
- 1 for SSTX2+SSRX2 (B2,B3 + A10, A11)

A device tree for this would look as follows:

// Type-C port driver
ec {
    ...
    cros_ec_typec {
        ...
        usb-c0 {
            compatible = "usb-c-connector";
            ports {
                hs : port@0 {
                    ...
                };
                ss: port@1 {
                    reg = <1>;
                    c0_sstrx1: endpoint@0 {
                        reg = <0>;
                        remote-endpoint = <&anx7625_out0>;
                    };
                    c0_sstrx2: endpoint@0 {
                        reg = <0>;
                        remote-endpoint = <&usb3hub_out0>;
                    };
                };
                sbu : port@2 {
                    ...
                };
            };
        };
        usb-c1 {
            compatible = "usb-c-connector";
            ports {
                hs : port@0 {
                    ...
                };
                ss: port@1 {
                    reg = <1>;
                    c1_sstrx1: endpoint@0 {
                        reg = <0>;
                        remote-endpoint = <&anx7625_out1>;
                    };
                    c1_sstrx2: endpoint@0 {
                        reg = <0>;
                        remote-endpoint = <&usb3hub_out1>;
                    };
                };
                sbu : port@2 {
                    ...
                };
            };
        };
    };
};

// DRM bridge / Type-C mode switch
anx_bridge: anx7625@58 {
    compatible = "analogix,anx7625";
    reg = <0x58>;
    ...
    // Input from DP controller
    port@0 {
        reg = <0>;
        ...
    };

    // Output to Type-C connector / DP panel
    port@1 {
        reg = <1>;

        anx7625_out0: endpoint@0 {
            reg = <0>;
            mode-switch;
            remote-endpoint = <&c0_sstrx1>;
        };
        anx7625_out1: endpoint@1 {
            reg = <1>;
            mode-switch;
            remote-endpoint = <&c1_sstrx1>;
        };
    };
};

// USB3 hub
usb3hub: foo_hub {
    ...
    ports@0 {
         // End point connected to USB3 host controller on SOC.
    };
    port@1 {
        reg = <1>;

        foo_hub_out0: endpoint@0 {
            reg = <0>;
            mode-switch; ---> See c.) later
            remote-endpoint = <&c0_sstrx2>;
        };
        foo_hub_out1: endpoint@1 {
            reg = <1>;
            mode-switch;
            remote-endpoint = <&c1_sstrx2>;
        };
    };
};

Notes:
- On the Chrome OS platform, the USB3 Hub is controlled by
the EC, so we don't really need to describe that connection,
but I've added a minimal one here just to show how the graph
connection would work if the HUB was controlled by the SoC.
- The above assumes that other hardware is controlling orientation.
We can add "orientation-switch" drivers along the graph path
if there is other hardware which controls orientation.

Example 2: 1 USB-C connector connected to 1 drm-bridge/ mode-switch

I've tried to use Bjorn's example [1], but I might have made
some mistakes since I don't have access to the schematic.


                  SoC
  +------------------------------------------+
  |                                          |
  |  +---------------+                       |
  |  |               |                       |
  |  |  DP ctrllr    |       +---------+     |                 C0
  |  |               +-------+         |     |   2 lane     +----------+
  |  +---------------+       |  QMP    +-----+-----/--------+ SSTRX1   |
  |                          |  PHY    |     |              |          |
  |  +-------------+  2 lane |         |     |   2 lane     |          |
  |  |             +----/----+         +-----+-----/--------+ SSTRX2   |
  |  |    dwc3     |         +---------+     |              |          |
  |  |             |                         |              |          |
  |  |             |         +---------+     |              |          |
  |  |             +---------+ HS PHY  |     |   HS lanes   |          |
  |  +-------------+         |         +-----+----/---------+ D +/-    |
  |                          |         |     |              +----------+
  |                          +---------+     |
  |                                          |
  +------------------------------------------+

The DT would look something like this (borrowing from Stephen's example [2]):

qmp {
    mode-switch; ----> See b.) later.
    orientation-switch;
    ports {
        qmp_usb_in: port@0 {
            reg = <0>;
            remote-endpoint = <&usb3_phy_out>;
        };
        qmp_dp_in: port@1 {
            reg = <1>;
            remote-endpoint = <&dp_phy_out>;
        };
        port@2 {
            reg = <2>;
            qmp_usb_dp_out0: endpoint@0 {
                reg = <0>;
                remote-endpoint = <&c0_sstrx1>;
            };
            qmp_usb_dp_out1: endpoint@1 {
                reg = <1>;
                remote-endpoint = <&c0_sstrx2>;
            };
        };
};

dp-phy {
    ports {
        dp_phy_out: port {
            remote-endpoint = <&qmp_dp_in>;
        };
    };
};

dwc3: usb-phy {
    ports {
        usb3_phy_out: port@0 {
            reg = <0>;
            remote-endpoint = <&qmp_usb_in>;
        };
    };
};

glink {
    c0: usb-c-connector {
        compatible = "usb-c-connector";
        ports {
            hs: port@0 {
                reg = <0>;
                endpoint@0 {
                    reg = <0>;
                    remote-endpoint = <&hs_phy_out>;
                };
            };

            ss: port@1 {
                reg = <1>;
                c0_sstrx1: endpoint@0 {
                    reg = <0>;
                    remote-endpoint = <&qmp_usb_dp_out0>;
                };
                c0_sstrx2: endpoint@1 {
                    reg = <1>;
                    remote-endpoint = <&qmp_usb_dp_out1>;
                };
            };
        };
    };
};

Notes:
a. This proposal doesn't deal with the DRM bridge HPD forwarding; I
believe that is covered by Stephen's example/proposal in [2], and
can be addressed separately. That said, this binding is compatible
with the proposal in [2], that is, make the "mode-switch" driver a
drm-bridge and forward the HPD info to the upstream DRM-bridge (DP controller).
The driver implementing "mode-switch" will be able to do that, since
it gets DP status/attention VDOS with HPD info from the Type-C port driver.
b. If both SSTRX pairs from a connector are routed to the same
hardware block (example 2) then the device would keep "mode-switch"
as a top level property (and the fwnode associated with "mode-switch"
is the drm-bridge device).
c. If SSTRX pairs from 2 connectors are routed to the same
hardware block (example 1), then each end-point which is connected to
the USB-C connector will have a "mode-switch" property in its end-point.
There will be 2 mode switches registered here, and the fwnode for each
"mode-switch" is the end-point node.

b.) and c.) can be handled by Type C mux registration and matching
code. We already have 3 mux devs for each mux [3].

For the single mode-switch case, mux_dev[1] will just refer to the top-level
mode-switch registered by the DRM bridge / switch driver (example 1).
For the 2 mode-switch case, typec_mux_dev[1] will have 2 child
typec_mux_dev's, each of which represents the mode-switches
registered by the DRM bridge / switch driver. Introducing this
indirection means the port driver / alternate mode driver don't
need to care about how the connectors are routed; the framework
will just call the mux_set() function on the mux_dev() or its
children if it has any.

The benefit of this approach is existing bindings (which just
assume 1 endpoint from usb-c-connector/port@1) should continue to
work without any changes.

Why don't we use data lanes for the usb-c-connector
endpoints? I guess we could, but I am not a fan of adding the
extra data-lane parsing logic to the Type-C framework (I
don't think drivers need that level of detail from the connector
binding). And even then, we will still need an extra end-point
if the lanes of the USB-C connector are routed to different hardware blocks.

The Type-C connector spec doesn't specify any alternate modes
with < 1 SSTRX pair, so the most we can ever have (short of a
major change to the spec) is 2 SSTRX end points for a
connector each being routed to different hardware blocks.
Codifying these as endpoint@0 and endpoint@1 in the usb-c-connector
binding seems to line up nicely with this detail of the spec.

Thanks,

-Prashant

[1] https://lore.kernel.org/linux-usb/Yv1y9Wjp16CstJvK@baldur/
[2] https://lore.kernel.org/linux-usb/CAE-0n52-QVeUVCB1qZzPbYyrb1drrbJf6H2DEEW9bOE6mh7egw@mail.gmail.com/
[3] https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/usb/typec/mux.c#L259

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

Hi folks,

On Fri, Sep 2, 2022 at 12:41 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Rob,
>
> On Jul 12 11:45, Rob Herring wrote:
> >
> > That's not the right interpretation. There should not be some Type-C
> > specific child mux/switch node because the device has no such h/w within
> > it. Assuming all the possibilities Stephen outlined are valid, it's
> > clear this lane selection has nothing to do with Type-C. It does have an
> > output port for its DP output already and using that to describe the
> > connection to DP connector(s) and/or Type-C connector(s) should be
> > handled.
> > Rob
>
> Below I've listed the proposal binding (for the Type-C connector) along
> with 2 sample hardware diagrams and corresponding DT.

Any thoughts about this proposal?

>
> The updated binding in usb-c-connector would be as follows:
>
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> index ae515651fc6b..a043b09cb8ec 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> @@ -183,6 +183,30 @@ properties:
>        port@1:
>          $ref: /schemas/graph.yaml#/properties/port
>          description: Super Speed (SS), present in SS capable connectors.
> +        properties:
> +          '#address-cells':
> +            const: 1
> +
> +          '#size-cells':
> +            const: 0
> +
> +        patternProperties:
> +          "^endpoint@[0-1]$":
> +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +            description:
> +              Endpoints for the two SS lanes. endpoint@0 refers to SSTRX1 (A2,A3,B10,B11)
> +              and endpoint@1 refers to SSTRX2 (B2,B3,A10,A11).
> +            additionalProperties: false
> +
> +              properties:
> +                reg:
> +                  maxItems: 1
> +
> +                remote-endpoint: true
> +
> +              required:
> +                - reg
> +                - remote-endpoint
>
>        port@2:
>          $ref: /schemas/graph.yaml#/properties/port
>
> Here are 2 examples of how that would look on some existing hardware:
>
> Example 1. 2 usb-c-connectors connecting to 1 drm bridge / DP switch:
>
> Here is the diagram we are using on the MTK platform:
>
>                  SOC
>         +---------------------+                                              C0
>         |                     |            +----------+       2 lane      +--------+
>         |                     |            |          +---------/---------+ SSTRX1 |
>         |                     |            |          |                   |        |
>         |    MIPI DPI         |            |          |  2 lane           |        |
>         |                     +------------+ ANX 7625 +---/-----+    +----+ SSTRX2 |
>         |                     |            |          |         |    |    +--------+
>         |                     |            +----------+         |    |
>         +---------------------+                                 |    |
>         |                     |            +----------+ 2 lane  |    |       C1
>         |                     |            |          +----/----C----+    +--------+
>         |    USB3 HC          |   2 lane   |          |         |         | SSTRX1 |
>         |                     +-----/------+ USB3 HUB |         +---------+        |
>         |  (host controller)  |            |          |       2 lane      |        |
>         |                     |            |          +---------/---------+ SSTRX2 |
>         +---------------------+            |          |                   |        |
>                                            +----------+                   +--------+
>
> Some platforms use it6505, so that can be swapped in for anx7625
> without any change to the rest of the hardware diagram.
>
> From the above, we can see that it is helpful to describe the
> Type-C SS lines as 2 endpoints:
> - 1 for SSTX1+SSRX1 (A2,A3 + B10,B11)
> - 1 for SSTX2+SSRX2 (B2,B3 + A10, A11)
>
> A device tree for this would look as follows:
>
> // Type-C port driver
> ec {
>     ...
>     cros_ec_typec {
>         ...
>         usb-c0 {
>             compatible = "usb-c-connector";
>             ports {
>                 hs : port@0 {
>                     ...
>                 };
>                 ss: port@1 {
>                     reg = <1>;
>                     c0_sstrx1: endpoint@0 {
>                         reg = <0>;
>                         remote-endpoint = <&anx7625_out0>;
>                     };
>                     c0_sstrx2: endpoint@0 {
>                         reg = <0>;
>                         remote-endpoint = <&usb3hub_out0>;
>                     };
>                 };
>                 sbu : port@2 {
>                     ...
>                 };
>             };
>         };
>         usb-c1 {
>             compatible = "usb-c-connector";
>             ports {
>                 hs : port@0 {
>                     ...
>                 };
>                 ss: port@1 {
>                     reg = <1>;
>                     c1_sstrx1: endpoint@0 {
>                         reg = <0>;
>                         remote-endpoint = <&anx7625_out1>;
>                     };
>                     c1_sstrx2: endpoint@0 {
>                         reg = <0>;
>                         remote-endpoint = <&usb3hub_out1>;
>                     };
>                 };
>                 sbu : port@2 {
>                     ...
>                 };
>             };
>         };
>     };
> };
>
> // DRM bridge / Type-C mode switch
> anx_bridge: anx7625@58 {
>     compatible = "analogix,anx7625";
>     reg = <0x58>;
>     ...
>     // Input from DP controller
>     port@0 {
>         reg = <0>;
>         ...
>     };
>
>     // Output to Type-C connector / DP panel
>     port@1 {
>         reg = <1>;
>
>         anx7625_out0: endpoint@0 {
>             reg = <0>;
>             mode-switch;
>             remote-endpoint = <&c0_sstrx1>;
>         };
>         anx7625_out1: endpoint@1 {
>             reg = <1>;
>             mode-switch;
>             remote-endpoint = <&c1_sstrx1>;
>         };
>     };
> };
>
> // USB3 hub
> usb3hub: foo_hub {
>     ...
>     ports@0 {
>          // End point connected to USB3 host controller on SOC.
>     };
>     port@1 {
>         reg = <1>;
>
>         foo_hub_out0: endpoint@0 {
>             reg = <0>;
>             mode-switch; ---> See c.) later
>             remote-endpoint = <&c0_sstrx2>;
>         };
>         foo_hub_out1: endpoint@1 {
>             reg = <1>;
>             mode-switch;
>             remote-endpoint = <&c1_sstrx2>;
>         };
>     };
> };
>
> Notes:
> - On the Chrome OS platform, the USB3 Hub is controlled by
> the EC, so we don't really need to describe that connection,
> but I've added a minimal one here just to show how the graph
> connection would work if the HUB was controlled by the SoC.
> - The above assumes that other hardware is controlling orientation.
> We can add "orientation-switch" drivers along the graph path
> if there is other hardware which controls orientation.
>
> Example 2: 1 USB-C connector connected to 1 drm-bridge/ mode-switch
>
> I've tried to use Bjorn's example [1], but I might have made
> some mistakes since I don't have access to the schematic.
>
>
>                   SoC
>   +------------------------------------------+
>   |                                          |
>   |  +---------------+                       |
>   |  |               |                       |
>   |  |  DP ctrllr    |       +---------+     |                 C0
>   |  |               +-------+         |     |   2 lane     +----------+
>   |  +---------------+       |  QMP    +-----+-----/--------+ SSTRX1   |
>   |                          |  PHY    |     |              |          |
>   |  +-------------+  2 lane |         |     |   2 lane     |          |
>   |  |             +----/----+         +-----+-----/--------+ SSTRX2   |
>   |  |    dwc3     |         +---------+     |              |          |
>   |  |             |                         |              |          |
>   |  |             |         +---------+     |              |          |
>   |  |             +---------+ HS PHY  |     |   HS lanes   |          |
>   |  +-------------+         |         +-----+----/---------+ D +/-    |
>   |                          |         |     |              +----------+
>   |                          +---------+     |
>   |                                          |
>   +------------------------------------------+
>
> The DT would look something like this (borrowing from Stephen's example [2]):
>
> qmp {
>     mode-switch; ----> See b.) later.
>     orientation-switch;
>     ports {
>         qmp_usb_in: port@0 {
>             reg = <0>;
>             remote-endpoint = <&usb3_phy_out>;
>         };
>         qmp_dp_in: port@1 {
>             reg = <1>;
>             remote-endpoint = <&dp_phy_out>;
>         };
>         port@2 {
>             reg = <2>;
>             qmp_usb_dp_out0: endpoint@0 {
>                 reg = <0>;
>                 remote-endpoint = <&c0_sstrx1>;
>             };
>             qmp_usb_dp_out1: endpoint@1 {
>                 reg = <1>;
>                 remote-endpoint = <&c0_sstrx2>;
>             };
>         };
> };
>
> dp-phy {
>     ports {
>         dp_phy_out: port {
>             remote-endpoint = <&qmp_dp_in>;
>         };
>     };
> };
>
> dwc3: usb-phy {
>     ports {
>         usb3_phy_out: port@0 {
>             reg = <0>;
>             remote-endpoint = <&qmp_usb_in>;
>         };
>     };
> };
>
> glink {
>     c0: usb-c-connector {
>         compatible = "usb-c-connector";
>         ports {
>             hs: port@0 {
>                 reg = <0>;
>                 endpoint@0 {
>                     reg = <0>;
>                     remote-endpoint = <&hs_phy_out>;
>                 };
>             };
>
>             ss: port@1 {
>                 reg = <1>;
>                 c0_sstrx1: endpoint@0 {
>                     reg = <0>;
>                     remote-endpoint = <&qmp_usb_dp_out0>;
>                 };
>                 c0_sstrx2: endpoint@1 {
>                     reg = <1>;
>                     remote-endpoint = <&qmp_usb_dp_out1>;
>                 };
>             };
>         };
>     };
> };
>
> Notes:
> a. This proposal doesn't deal with the DRM bridge HPD forwarding; I
> believe that is covered by Stephen's example/proposal in [2], and
> can be addressed separately. That said, this binding is compatible
> with the proposal in [2], that is, make the "mode-switch" driver a
> drm-bridge and forward the HPD info to the upstream DRM-bridge (DP controller).
> The driver implementing "mode-switch" will be able to do that, since
> it gets DP status/attention VDOS with HPD info from the Type-C port driver.
> b. If both SSTRX pairs from a connector are routed to the same
> hardware block (example 2) then the device would keep "mode-switch"
> as a top level property (and the fwnode associated with "mode-switch"
> is the drm-bridge device).
> c. If SSTRX pairs from 2 connectors are routed to the same
> hardware block (example 1), then each end-point which is connected to
> the USB-C connector will have a "mode-switch" property in its end-point.
> There will be 2 mode switches registered here, and the fwnode for each
> "mode-switch" is the end-point node.
>
> b.) and c.) can be handled by Type C mux registration and matching
> code. We already have 3 mux devs for each mux [3].
>
> For the single mode-switch case, mux_dev[1] will just refer to the top-level
> mode-switch registered by the DRM bridge / switch driver (example 1).
> For the 2 mode-switch case, typec_mux_dev[1] will have 2 child
> typec_mux_dev's, each of which represents the mode-switches
> registered by the DRM bridge / switch driver. Introducing this
> indirection means the port driver / alternate mode driver don't
> need to care about how the connectors are routed; the framework
> will just call the mux_set() function on the mux_dev() or its
> children if it has any.
>
> The benefit of this approach is existing bindings (which just
> assume 1 endpoint from usb-c-connector/port@1) should continue to
> work without any changes.
>
> Why don't we use data lanes for the usb-c-connector
> endpoints? I guess we could, but I am not a fan of adding the
> extra data-lane parsing logic to the Type-C framework (I
> don't think drivers need that level of detail from the connector
> binding). And even then, we will still need an extra end-point
> if the lanes of the USB-C connector are routed to different hardware blocks.
>
> The Type-C connector spec doesn't specify any alternate modes
> with < 1 SSTRX pair, so the most we can ever have (short of a
> major change to the spec) is 2 SSTRX end points for a
> connector each being routed to different hardware blocks.
> Codifying these as endpoint@0 and endpoint@1 in the usb-c-connector
> binding seems to line up nicely with this detail of the spec.
>
> Thanks,
>
> -Prashant
>
> [1] https://lore.kernel.org/linux-usb/Yv1y9Wjp16CstJvK@baldur/
> [2] https://lore.kernel.org/linux-usb/CAE-0n52-QVeUVCB1qZzPbYyrb1drrbJf6H2DEEW9bOE6mh7egw@mail.gmail.com/
> [3] https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/usb/typec/mux.c#L259

Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Pin-yen Lin]

Hi all,

Are there any thoughts or comments about this proposal?

On Sat, Sep 17, 2022 at 2:22 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi folks,
>
> On Fri, Sep 2, 2022 at 12:41 AM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Jul 12 11:45, Rob Herring wrote:
> > >
> > > That's not the right interpretation. There should not be some Type-C
> > > specific child mux/switch node because the device has no such h/w within
> > > it. Assuming all the possibilities Stephen outlined are valid, it's
> > > clear this lane selection has nothing to do with Type-C. It does have an
> > > output port for its DP output already and using that to describe the
> > > connection to DP connector(s) and/or Type-C connector(s) should be
> > > handled.
> > > Rob
> >
> > Below I've listed the proposal binding (for the Type-C connector) along
> > with 2 sample hardware diagrams and corresponding DT.
>
> Any thoughts about this proposal?
>
> >
> > The updated binding in usb-c-connector would be as follows:
> >
> > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > index ae515651fc6b..a043b09cb8ec 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > @@ -183,6 +183,30 @@ properties:
> >        port@1:
> >          $ref: /schemas/graph.yaml#/properties/port
> >          description: Super Speed (SS), present in SS capable connectors.
> > +        properties:
> > +          '#address-cells':
> > +            const: 1
> > +
> > +          '#size-cells':
> > +            const: 0
> > +
> > +        patternProperties:
> > +          "^endpoint@[0-1]$":
> > +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > +            description:
> > +              Endpoints for the two SS lanes. endpoint@0 refers to SSTRX1 (A2,A3,B10,B11)
> > +              and endpoint@1 refers to SSTRX2 (B2,B3,A10,A11).
> > +            additionalProperties: false
> > +
> > +              properties:
> > +                reg:
> > +                  maxItems: 1
> > +
> > +                remote-endpoint: true
> > +
> > +              required:
> > +                - reg
> > +                - remote-endpoint
> >
> >        port@2:
> >          $ref: /schemas/graph.yaml#/properties/port
> >
> > Here are 2 examples of how that would look on some existing hardware:
> >
> > Example 1. 2 usb-c-connectors connecting to 1 drm bridge / DP switch:
> >
> > Here is the diagram we are using on the MTK platform:
> >
> >                  SOC
> >         +---------------------+                                              C0
> >         |                     |            +----------+       2 lane      +--------+
> >         |                     |            |          +---------/---------+ SSTRX1 |
> >         |                     |            |          |                   |        |
> >         |    MIPI DPI         |            |          |  2 lane           |        |
> >         |                     +------------+ ANX 7625 +---/-----+    +----+ SSTRX2 |
> >         |                     |            |          |         |    |    +--------+
> >         |                     |            +----------+         |    |
> >         +---------------------+                                 |    |
> >         |                     |            +----------+ 2 lane  |    |       C1
> >         |                     |            |          +----/----C----+    +--------+
> >         |    USB3 HC          |   2 lane   |          |         |         | SSTRX1 |
> >         |                     +-----/------+ USB3 HUB |         +---------+        |
> >         |  (host controller)  |            |          |       2 lane      |        |
> >         |                     |            |          +---------/---------+ SSTRX2 |
> >         +---------------------+            |          |                   |        |
> >                                            +----------+                   +--------+
> >
> > Some platforms use it6505, so that can be swapped in for anx7625
> > without any change to the rest of the hardware diagram.
> >
> > From the above, we can see that it is helpful to describe the
> > Type-C SS lines as 2 endpoints:
> > - 1 for SSTX1+SSRX1 (A2,A3 + B10,B11)
> > - 1 for SSTX2+SSRX2 (B2,B3 + A10, A11)
> >
> > A device tree for this would look as follows:
> >
> > // Type-C port driver
> > ec {
> >     ...
> >     cros_ec_typec {
> >         ...
> >         usb-c0 {
> >             compatible = "usb-c-connector";
> >             ports {
> >                 hs : port@0 {
> >                     ...
> >                 };
> >                 ss: port@1 {
> >                     reg = <1>;
> >                     c0_sstrx1: endpoint@0 {
> >                         reg = <0>;
> >                         remote-endpoint = <&anx7625_out0>;
> >                     };
> >                     c0_sstrx2: endpoint@0 {
> >                         reg = <0>;
> >                         remote-endpoint = <&usb3hub_out0>;
> >                     };
> >                 };
> >                 sbu : port@2 {
> >                     ...
> >                 };
> >             };
> >         };
> >         usb-c1 {
> >             compatible = "usb-c-connector";
> >             ports {
> >                 hs : port@0 {
> >                     ...
> >                 };
> >                 ss: port@1 {
> >                     reg = <1>;
> >                     c1_sstrx1: endpoint@0 {
> >                         reg = <0>;
> >                         remote-endpoint = <&anx7625_out1>;
> >                     };
> >                     c1_sstrx2: endpoint@0 {
> >                         reg = <0>;
> >                         remote-endpoint = <&usb3hub_out1>;
> >                     };
> >                 };
> >                 sbu : port@2 {
> >                     ...
> >                 };
> >             };
> >         };
> >     };
> > };
> >
> > // DRM bridge / Type-C mode switch
> > anx_bridge: anx7625@58 {
> >     compatible = "analogix,anx7625";
> >     reg = <0x58>;
> >     ...
> >     // Input from DP controller
> >     port@0 {
> >         reg = <0>;
> >         ...
> >     };
> >
> >     // Output to Type-C connector / DP panel
> >     port@1 {
> >         reg = <1>;
> >
> >         anx7625_out0: endpoint@0 {
> >             reg = <0>;
> >             mode-switch;
> >             remote-endpoint = <&c0_sstrx1>;
> >         };
> >         anx7625_out1: endpoint@1 {
> >             reg = <1>;
> >             mode-switch;
> >             remote-endpoint = <&c1_sstrx1>;
> >         };
> >     };
> > };
> >
> > // USB3 hub
> > usb3hub: foo_hub {
> >     ...
> >     ports@0 {
> >          // End point connected to USB3 host controller on SOC.
> >     };
> >     port@1 {
> >         reg = <1>;
> >
> >         foo_hub_out0: endpoint@0 {
> >             reg = <0>;
> >             mode-switch; ---> See c.) later
> >             remote-endpoint = <&c0_sstrx2>;
> >         };
> >         foo_hub_out1: endpoint@1 {
> >             reg = <1>;
> >             mode-switch;
> >             remote-endpoint = <&c1_sstrx2>;
> >         };
> >     };
> > };
> >
> > Notes:
> > - On the Chrome OS platform, the USB3 Hub is controlled by
> > the EC, so we don't really need to describe that connection,
> > but I've added a minimal one here just to show how the graph
> > connection would work if the HUB was controlled by the SoC.
> > - The above assumes that other hardware is controlling orientation.
> > We can add "orientation-switch" drivers along the graph path
> > if there is other hardware which controls orientation.
> >
> > Example 2: 1 USB-C connector connected to 1 drm-bridge/ mode-switch
> >
> > I've tried to use Bjorn's example [1], but I might have made
> > some mistakes since I don't have access to the schematic.
> >
> >
> >                   SoC
> >   +------------------------------------------+
> >   |                                          |
> >   |  +---------------+                       |
> >   |  |               |                       |
> >   |  |  DP ctrllr    |       +---------+     |                 C0
> >   |  |               +-------+         |     |   2 lane     +----------+
> >   |  +---------------+       |  QMP    +-----+-----/--------+ SSTRX1   |
> >   |                          |  PHY    |     |              |          |
> >   |  +-------------+  2 lane |         |     |   2 lane     |          |
> >   |  |             +----/----+         +-----+-----/--------+ SSTRX2   |
> >   |  |    dwc3     |         +---------+     |              |          |
> >   |  |             |                         |              |          |
> >   |  |             |         +---------+     |              |          |
> >   |  |             +---------+ HS PHY  |     |   HS lanes   |          |
> >   |  +-------------+         |         +-----+----/---------+ D +/-    |
> >   |                          |         |     |              +----------+
> >   |                          +---------+     |
> >   |                                          |
> >   +------------------------------------------+
> >
> > The DT would look something like this (borrowing from Stephen's example [2]):
> >
> > qmp {
> >     mode-switch; ----> See b.) later.
> >     orientation-switch;
> >     ports {
> >         qmp_usb_in: port@0 {
> >             reg = <0>;
> >             remote-endpoint = <&usb3_phy_out>;
> >         };
> >         qmp_dp_in: port@1 {
> >             reg = <1>;
> >             remote-endpoint = <&dp_phy_out>;
> >         };
> >         port@2 {
> >             reg = <2>;
> >             qmp_usb_dp_out0: endpoint@0 {
> >                 reg = <0>;
> >                 remote-endpoint = <&c0_sstrx1>;
> >             };
> >             qmp_usb_dp_out1: endpoint@1 {
> >                 reg = <1>;
> >                 remote-endpoint = <&c0_sstrx2>;
> >             };
> >         };
> > };
> >
> > dp-phy {
> >     ports {
> >         dp_phy_out: port {
> >             remote-endpoint = <&qmp_dp_in>;
> >         };
> >     };
> > };
> >
> > dwc3: usb-phy {
> >     ports {
> >         usb3_phy_out: port@0 {
> >             reg = <0>;
> >             remote-endpoint = <&qmp_usb_in>;
> >         };
> >     };
> > };
> >
> > glink {
> >     c0: usb-c-connector {
> >         compatible = "usb-c-connector";
> >         ports {
> >             hs: port@0 {
> >                 reg = <0>;
> >                 endpoint@0 {
> >                     reg = <0>;
> >                     remote-endpoint = <&hs_phy_out>;
> >                 };
> >             };
> >
> >             ss: port@1 {
> >                 reg = <1>;
> >                 c0_sstrx1: endpoint@0 {
> >                     reg = <0>;
> >                     remote-endpoint = <&qmp_usb_dp_out0>;
> >                 };
> >                 c0_sstrx2: endpoint@1 {
> >                     reg = <1>;
> >                     remote-endpoint = <&qmp_usb_dp_out1>;
> >                 };
> >             };
> >         };
> >     };
> > };
> >
> > Notes:
> > a. This proposal doesn't deal with the DRM bridge HPD forwarding; I
> > believe that is covered by Stephen's example/proposal in [2], and
> > can be addressed separately. That said, this binding is compatible
> > with the proposal in [2], that is, make the "mode-switch" driver a
> > drm-bridge and forward the HPD info to the upstream DRM-bridge (DP controller).
> > The driver implementing "mode-switch" will be able to do that, since
> > it gets DP status/attention VDOS with HPD info from the Type-C port driver.
> > b. If both SSTRX pairs from a connector are routed to the same
> > hardware block (example 2) then the device would keep "mode-switch"
> > as a top level property (and the fwnode associated with "mode-switch"
> > is the drm-bridge device).
> > c. If SSTRX pairs from 2 connectors are routed to the same
> > hardware block (example 1), then each end-point which is connected to
> > the USB-C connector will have a "mode-switch" property in its end-point.
> > There will be 2 mode switches registered here, and the fwnode for each
> > "mode-switch" is the end-point node.
> >
> > b.) and c.) can be handled by Type C mux registration and matching
> > code. We already have 3 mux devs for each mux [3].
> >
> > For the single mode-switch case, mux_dev[1] will just refer to the top-level
> > mode-switch registered by the DRM bridge / switch driver (example 1).
> > For the 2 mode-switch case, typec_mux_dev[1] will have 2 child
> > typec_mux_dev's, each of which represents the mode-switches
> > registered by the DRM bridge / switch driver. Introducing this
> > indirection means the port driver / alternate mode driver don't
> > need to care about how the connectors are routed; the framework
> > will just call the mux_set() function on the mux_dev() or its
> > children if it has any.
> >
> > The benefit of this approach is existing bindings (which just
> > assume 1 endpoint from usb-c-connector/port@1) should continue to
> > work without any changes.
> >
> > Why don't we use data lanes for the usb-c-connector
> > endpoints? I guess we could, but I am not a fan of adding the
> > extra data-lane parsing logic to the Type-C framework (I
> > don't think drivers need that level of detail from the connector
> > binding). And even then, we will still need an extra end-point
> > if the lanes of the USB-C connector are routed to different hardware blocks.
> >
> > The Type-C connector spec doesn't specify any alternate modes
> > with < 1 SSTRX pair, so the most we can ever have (short of a
> > major change to the spec) is 2 SSTRX end points for a
> > connector each being routed to different hardware blocks.
> > Codifying these as endpoint@0 and endpoint@1 in the usb-c-connector
> > binding seems to line up nicely with this detail of the spec.
> >
> > Thanks,
> >
> > -Prashant
> >
> > [1] https://lore.kernel.org/linux-usb/Yv1y9Wjp16CstJvK@baldur/
> > [2] https://lore.kernel.org/linux-usb/CAE-0n52-QVeUVCB1qZzPbYyrb1drrbJf6H2DEEW9bOE6mh7egw@mail.gmail.com/
> > [3] https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/usb/typec/mux.c#L259

[PATCH v5 2/9] dt-bindings: drm/bridge: anx7625: Add mode-switch support

[Prashant Malani]

Analogix 7625 can be used in systems to switch USB Type-C DisplayPort
alternate mode lane traffic between 2 Type-C ports.

Update the binding to accommodate this usage by introducing a switch
property.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v4:
- Added Reviewed-by tags.
- Patch moved to 2/9 position (since Patch v4 1/7 and 2/7 were
  applied to usb-next).

Changes since v3:
- Fix unevaluatedProperties usage.
- Add additionalProperties to top level "switches" nodes.
- Make quotes consistent.
- Add '^..$' to regex.
(All suggested by Krzysztof Kozlowski)

Changes since v2:
- Added Reviewed-by and Tested-by tags.

Changes since v1:
- Introduced patternProperties for "switch" children (suggested by
  Krzysztof Kozlowski).
- Added unevaluatedProperties descriptor (suggested by Krzysztof
  Kozlowski).
- Added "address-cells" and "size-cells" properties to "switches".

 .../display/bridge/analogix,anx7625.yaml      | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)

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 {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                switch@0 {
+                    compatible = "typec-switch";
+                    reg = <0>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            anx_typec0: endpoint {
+                                remote-endpoint = <&typec_port0>;
+                            };
+                        };
+                    };
+                };
+                switch@1 {
+                    compatible = "typec-switch";
+                    reg = <1>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            anx_typec1: endpoint {
+                                remote-endpoint = <&typec_port1>;
+                            };
+                        };
+                    };
+                };
+            };
         };
     };
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v5 3/9] drm/bridge: anx7625: Register number of Type C switches

[Prashant Malani]

Parse the "switches" node, if available, and count and store the number
of Type-C switches within it. Since we currently don't do anything with
this info, no functional changes are expected from this change.

This patch sets a foundation for the actual registering of Type-C
switches with the Type-C connector class framework.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v4:
- Added Reviewed-by tags.
- Patch moved to 3/9 position (since Patch v4 1/7 and 2/7 were
  applied to usb-next).

Changes since v3:
- No changes.

Changes since v2:
- Move ret variable to Patch v3 6/7.
- Make error print a dev_dbg, since it is noisy.
- Added Reviewed-by and Tested-by tags.

Changes since v1:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 18 ++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 53a5da6c49dd..e3d4c2738b8c 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -2581,6 +2581,20 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
+{
+	struct device_node *of = of_get_child_by_name(device->of_node, "switches");
+
+	if (!of)
+		return -ENODEV;
+
+	ctx->num_typec_switches = of_get_child_count(of);
+	if (ctx->num_typec_switches <= 0)
+		return -ENODEV;
+
+	return 0;
+}
+
 static int anx7625_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -2686,6 +2700,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 	if (platform->pdata.intp_irq)
 		queue_work(platform->workqueue, &platform->work);
 
+	ret = anx7625_register_typec_switches(dev, platform);
+	if (ret)
+		dev_dbg(dev, "Didn't register Type C switches, err: %d\n", ret);
+
 	platform->bridge.funcs = &anx7625_bridge_funcs;
 	platform->bridge.of_node = client->dev.of_node;
 	if (!anx7625_of_panel_on_aux_bus(&client->dev))
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index e257a84db962..d5cbca708842 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -473,6 +473,7 @@ struct anx7625_data {
 	struct drm_connector *connector;
 	struct mipi_dsi_device *dsi;
 	struct drm_dp_aux aux;
+	int num_typec_switches;
 };
 
 #endif  /* __ANX7625_H__ */
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v5 4/9] drm/bridge: anx7625: Register Type-C mode switches

[Prashant Malani]

When the DT node has "switches" available, register a Type-C mode-switch
for each listed "switch". This allows the driver to receive state
information about what operating mode a Type-C port and its connected
peripherals are in, as well as status information (like VDOs) related to
that state.

The callback function is currently a stub, but subsequent patches will
implement the required functionality.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v4:
- Added Reviewed-by tags.
- Patch moved to 4/9 position (since Patch v4 1/7 and 2/7 were
  applied to usb-next).

Changes since v3:
- No changes.

Changes since v2:
- Updated dev_info() to dev_warn() print, but added a check to ensure it
  only triggers on non -ENODEV errors.
- Made conflict resolutions resulting from changes introduced in
  Patch v3 5/7 (add ret variable here instead of in Patch v3 5/7).
- Added Reviewed-by and Tested-by tags.

Changes since v1:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 82 +++++++++++++++++++++--
 drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
 2 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index e3d4c2738b8c..bd21f159b973 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/usb/typec_mux.h>
 #include <linux/workqueue.h>
 
 #include <linux/of_gpio.h>
@@ -2581,10 +2582,61 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
+				 struct typec_mux_state *state)
+{
+	return 0;
+}
+
+static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
+					struct anx7625_data *ctx)
+{
+	struct anx7625_port_data *port_data;
+	struct typec_mux_desc mux_desc = {};
+	char name[32];
+	u32 port_num;
+	int ret;
+
+	ret = of_property_read_u32(node, "reg", &port_num);
+	if (ret)
+		return ret;
+
+	if (port_num >= ctx->num_typec_switches) {
+		dev_err(dev, "Invalid port number specified: %d\n", port_num);
+		return -EINVAL;
+	}
+
+	port_data = &ctx->typec_ports[port_num];
+	port_data->ctx = ctx;
+	mux_desc.fwnode = &node->fwnode;
+	mux_desc.drvdata = port_data;
+	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
+	mux_desc.name = name;
+	mux_desc.set = anx7625_typec_mux_set;
+
+	port_data->typec_mux = typec_mux_register(dev, &mux_desc);
+	if (IS_ERR(port_data->typec_mux)) {
+		ret = PTR_ERR(port_data->typec_mux);
+		dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
+	}
+
+	return ret;
+}
+
+static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
+{
+	int i;
+
+	for (i = 0; i < ctx->num_typec_switches; i++)
+		typec_mux_unregister(ctx->typec_ports[i].typec_mux);
+}
+
 static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
 {
-	struct device_node *of = of_get_child_by_name(device->of_node, "switches");
+	struct device_node *of, *sw;
+	int ret = 0;
 
+	of = of_get_child_by_name(device->of_node, "switches");
 	if (!of)
 		return -ENODEV;
 
@@ -2592,7 +2644,27 @@ static int anx7625_register_typec_switches(struct device *device, struct anx7625
 	if (ctx->num_typec_switches <= 0)
 		return -ENODEV;
 
-	return 0;
+	ctx->typec_ports = devm_kzalloc(device,
+					ctx->num_typec_switches * sizeof(struct anx7625_port_data),
+					GFP_KERNEL);
+	if (!ctx->typec_ports)
+		return -ENOMEM;
+
+	/* Register switches for each connector. */
+	for_each_available_child_of_node(of, sw) {
+		if (!of_property_read_bool(sw, "mode-switch"))
+			continue;
+		ret = anx7625_register_mode_switch(device, sw, ctx);
+		if (ret) {
+			dev_err(device, "Failed to register mode switch: %d\n", ret);
+			break;
+		}
+	}
+
+	if (ret)
+		anx7625_unregister_typec_switches(ctx);
+
+	return ret;
 }
 
 static int anx7625_i2c_probe(struct i2c_client *client,
@@ -2701,8 +2773,8 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 		queue_work(platform->workqueue, &platform->work);
 
 	ret = anx7625_register_typec_switches(dev, platform);
-	if (ret)
-		dev_dbg(dev, "Didn't register Type C switches, err: %d\n", ret);
+	if (ret && ret != -ENODEV)
+		dev_warn(dev, "Didn't register Type C switches, err: %d\n", ret);
 
 	platform->bridge.funcs = &anx7625_bridge_funcs;
 	platform->bridge.of_node = client->dev.of_node;
@@ -2757,6 +2829,8 @@ static int anx7625_i2c_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&platform->bridge);
 
+	anx7625_unregister_typec_switches(platform);
+
 	if (platform->pdata.intp_irq)
 		destroy_workqueue(platform->workqueue);
 
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index d5cbca708842..76cfc64f7574 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -443,6 +443,11 @@ struct anx7625_i2c_client {
 	struct i2c_client *tcpc_client;
 };
 
+struct anx7625_port_data {
+	struct typec_mux_dev *typec_mux;
+	struct anx7625_data *ctx;
+};
+
 struct anx7625_data {
 	struct anx7625_platform_data pdata;
 	struct platform_device *audio_pdev;
@@ -474,6 +479,7 @@ struct anx7625_data {
 	struct mipi_dsi_device *dsi;
 	struct drm_dp_aux aux;
 	int num_typec_switches;
+	struct anx7625_port_data *typec_ports;
 };
 
 #endif  /* __ANX7625_H__ */
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

From: Pin-Yen Lin <treapking@chromium.org>

Add the callback function when the driver receives state
changes of the Type-C port. The callback function configures the
crosspoint switch of the anx7625 bridge chip, which can change the
output pins of the signals according to the port state.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v4:
- Patch moved to 5/9 position (since Patch v4 1/7 and 2/7 were
  applied to usb-next).

Changes since v3:
- Added Reviewed-by tag from Angelo.

Changes since v2:
- Moved num_typec_switches check to beginning of function
- Made dp_connected assignments fit on one line (and removed unnecessary
  parentheses)
- Added Reviewed-by and Tested-by tags.

Changes since v1:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 56 +++++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h | 13 ++++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index bd21f159b973..5992fc8beeeb 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/usb/typec_dp.h>
 #include <linux/usb/typec_mux.h>
 #include <linux/workqueue.h>
 
@@ -2582,9 +2583,64 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
+					  enum typec_orientation orientation)
+{
+	if (orientation == TYPEC_ORIENTATION_NORMAL) {
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
+				  SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
+				  SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
+	} else if (orientation == TYPEC_ORIENTATION_REVERSE) {
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
+				  SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
+				  SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
+	}
+}
+
+static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
+{
+	if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
+		/* Both ports available, do nothing to retain the current one. */
+		return;
+	else if (ctx->typec_ports[0].dp_connected)
+		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
+	else if (ctx->typec_ports[1].dp_connected)
+		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
+}
+
 static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
 				 struct typec_mux_state *state)
 {
+	struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
+	struct anx7625_data *ctx = data->ctx;
+	struct device *dev = &ctx->client->dev;
+	bool new_dp_connected, old_dp_connected;
+
+	if (ctx->num_typec_switches == 1)
+		return 0;
+
+	old_dp_connected = ctx->typec_ports[0].dp_connected || ctx->typec_ports[1].dp_connected;
+
+	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
+		ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
+
+	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
+			      state->alt->mode == USB_TYPEC_DP_MODE);
+
+	new_dp_connected = ctx->typec_ports[0].dp_connected || ctx->typec_ports[1].dp_connected;
+
+	/* dp on, power on first */
+	if (!old_dp_connected && new_dp_connected)
+		pm_runtime_get_sync(dev);
+
+	anx7625_typec_two_ports_update(ctx);
+
+	/* dp off, power off last */
+	if (old_dp_connected && !new_dp_connected)
+		pm_runtime_put_sync(dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index 76cfc64f7574..7d6c6fdf9a3a 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -55,6 +55,18 @@
 #define HPD_STATUS_CHANGE 0x80
 #define HPD_STATUS 0x80
 
+#define TCPC_SWITCH_0 0xB4
+#define SW_SEL1_DPTX0_RX2 BIT(0)
+#define SW_SEL1_DPTX0_RX1 BIT(1)
+#define SW_SEL1_SSRX_RX2 BIT(4)
+#define SW_SEL1_SSRX_RX1 BIT(5)
+
+#define TCPC_SWITCH_1 0xB5
+#define SW_SEL2_DPTX1_TX2 BIT(0)
+#define SW_SEL2_DPTX1_TX1 BIT(1)
+#define SW_SEL2_SSTX_TX2 BIT(4)
+#define SW_SEL2_SSTX_TX1 BIT(5)
+
 /******** END of I2C Address 0x58 ********/
 
 /***************************************************************/
@@ -444,6 +456,7 @@ struct anx7625_i2c_client {
 };
 
 struct anx7625_port_data {
+	bool dp_connected;
 	struct typec_mux_dev *typec_mux;
 	struct anx7625_data *ctx;
 };
-- 
2.37.0.rc0.104.g0611611a94-goog

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Stephen Boyd]

Quoting Prashant Malani (2022-06-22 10:34:34)
> From: Pin-Yen Lin <treapking@chromium.org>
>
> Add the callback function when the driver receives state
> changes of the Type-C port. The callback function configures the
> crosspoint switch of the anx7625 bridge chip, which can change the
> output pins of the signals according to the port state.

Can this be combined with the previous two patches? They really don't
stand alone because the previous two patches are adding stubs that are
filled out later.

> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index bd21f159b973..5992fc8beeeb 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -15,6 +15,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <linux/usb/typec_dp.h>
>  #include <linux/usb/typec_mux.h>
>  #include <linux/workqueue.h>
>
> @@ -2582,9 +2583,64 @@ static void anx7625_runtime_disable(void *data)
>         pm_runtime_disable(data);
>  }
>
> +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> +                                         enum typec_orientation orientation)
> +{
> +       if (orientation == TYPEC_ORIENTATION_NORMAL) {
> +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +                                 SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +                                 SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> +       } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +                                 SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +                                 SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> +       }
> +}
> +
> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> +{
> +       if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> +               /* Both ports available, do nothing to retain the current one. */
> +               return;
> +       else if (ctx->typec_ports[0].dp_connected)
> +               anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> +       else if (ctx->typec_ports[1].dp_connected)
> +               anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> +}
> +
>  static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>                                  struct typec_mux_state *state)
>  {
> +       struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> +       struct anx7625_data *ctx = data->ctx;
> +       struct device *dev = &ctx->client->dev;
> +       bool new_dp_connected, old_dp_connected;
> +
> +       if (ctx->num_typec_switches == 1)

How do we handle the case where the usb-c-connector is directly
connected to the RX1/TX1 and RX2/TX2 pins? This device would be an
orientation (normal/reverse) and mode switch (usb/dp) in that scenario,
but this code is written in a way that the orientation switch isn't
going to flip the crosspoint switch for the different pin assignments.

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-22 10:34:34)
> > From: Pin-Yen Lin <treapking@chromium.org>
> >
> > Add the callback function when the driver receives state
> > changes of the Type-C port. The callback function configures the
> > crosspoint switch of the anx7625 bridge chip, which can change the
> > output pins of the signals according to the port state.
>
> Can this be combined with the previous two patches? They really don't
> stand alone because the previous two patches are adding stubs that are
> filled out later.

I split it out for ease of reviewing, but sure, I will combine it if
there is a v6.

>
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index bd21f159b973..5992fc8beeeb 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <linux/usb/typec_dp.h>
> >  #include <linux/usb/typec_mux.h>
> >  #include <linux/workqueue.h>
> >
> > @@ -2582,9 +2583,64 @@ static void anx7625_runtime_disable(void *data)
> >         pm_runtime_disable(data);
> >  }
> >
> > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> > +                                         enum typec_orientation orientation)
> > +{
> > +       if (orientation == TYPEC_ORIENTATION_NORMAL) {
> > +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                                 SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> > +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                                 SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> > +       } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> > +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                                 SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> > +               anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                                 SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> > +       }
> > +}
> > +
> > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> > +{
> > +       if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> > +               /* Both ports available, do nothing to retain the current one. */
> > +               return;
> > +       else if (ctx->typec_ports[0].dp_connected)
> > +               anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> > +       else if (ctx->typec_ports[1].dp_connected)
> > +               anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> > +}
> > +
> >  static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> >                                  struct typec_mux_state *state)
> >  {
> > +       struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> > +       struct anx7625_data *ctx = data->ctx;
> > +       struct device *dev = &ctx->client->dev;
> > +       bool new_dp_connected, old_dp_connected;
> > +
> > +       if (ctx->num_typec_switches == 1)
>
> How do we handle the case where the usb-c-connector is directly
> connected to the RX1/TX1 and RX2/TX2 pins? This device would be an
> orientation (normal/reverse) and mode switch (usb/dp) in that scenario,
> but this code is written in a way that the orientation switch isn't
> going to flip the crosspoint switch for the different pin assignments.

If all 4 SS lanes are connected to 1 usb-c-connector; there would be
just 1 "typec-switch" node.
In that case, the DT would only specify it as an "orientation-switch"
and register
an orientation-switch with the Type-C framework. The orientation switch would
pretty much do what the mode-switch callback does here (configuring
the crosspoint
switch).
One could also register a "mode-switch" there but it wouldn't do
anything (all 4 lanes are already
connected so there is nothing to re-route in the crosspoint switch).
Hence the above "if" check.

Unfortunately, I don't have hardware which connects all 4 SS lanes
from 1 Type-C port
to the anx7625, so I didn't add the orientation switch handling to the
driver (since I have no way of verifying it).

Regarding DP alt-mode pin assignments : I think anx7625 will only support Pin D
(only 2 lane DP, no 4 lane DP).

BR,

-Prashant

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Stephen Boyd]

Quoting Prashant Malani (2022-06-28 12:48:11)
> On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-22 10:34:34)
> > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > index bd21f159b973..5992fc8beeeb 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
[..]
> > > +
> > > +       if (ctx->num_typec_switches == 1)
> >
> > How do we handle the case where the usb-c-connector is directly
> > connected to the RX1/TX1 and RX2/TX2 pins? This device would be an
> > orientation (normal/reverse) and mode switch (usb/dp) in that scenario,
> > but this code is written in a way that the orientation switch isn't
> > going to flip the crosspoint switch for the different pin assignments.
>
> If all 4 SS lanes are connected to 1 usb-c-connector; there would be
> just 1 "typec-switch" node.
> In that case, the DT would only specify it as an "orientation-switch"
> and register
> an orientation-switch with the Type-C framework. The orientation switch would
> pretty much do what the mode-switch callback does here (configuring
> the crosspoint
> switch).
> One could also register a "mode-switch" there but it wouldn't do
> anything (all 4 lanes are already
> connected so there is nothing to re-route in the crosspoint switch).
> Hence the above "if" check.

Would we still want to route the DP traffic out if the pin assignment
didn't have DP? Does the hardware support some mode where the DP traffic
is shutdown? Or maybe the HPD pin needs to be quieted unless DP is
assigned?

I suppose none of those things matter though as long as there is some
typec switch registered here so that the driver can be informed of the
pin assignment. Is it right that the "mode-switch" property is only
required in DT if this device is going to control the mode of the
connector, i.e. USB+DP, or just DP? Where this device can't do that
because it doesn't support only DP.

>
> Unfortunately, I don't have hardware which connects all 4 SS lanes
> from 1 Type-C port
> to the anx7625, so I didn't add the orientation switch handling to the
> driver (since I have no way of verifying it).

Alright. Maybe add a TODO then so it's more obvious that orientation
isn't handled.

>
> Regarding DP alt-mode pin assignments : I think anx7625 will only support Pin D
> (only 2 lane DP, no 4 lane DP).
>

Makes sense. Thanks!

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-28 12:48:11)
> > On Tue, Jun 28, 2022 at 12:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Prashant Malani (2022-06-22 10:34:34)
> > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > index bd21f159b973..5992fc8beeeb 100644
> > > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> [..]
> > > > +
> > > > +       if (ctx->num_typec_switches == 1)
> > >
> > > How do we handle the case where the usb-c-connector is directly
> > > connected to the RX1/TX1 and RX2/TX2 pins? This device would be an
> > > orientation (normal/reverse) and mode switch (usb/dp) in that scenario,
> > > but this code is written in a way that the orientation switch isn't
> > > going to flip the crosspoint switch for the different pin assignments.
> >
> > If all 4 SS lanes are connected to 1 usb-c-connector; there would be
> > just 1 "typec-switch" node.
> > In that case, the DT would only specify it as an "orientation-switch"
> > and register
> > an orientation-switch with the Type-C framework. The orientation switch would
> > pretty much do what the mode-switch callback does here (configuring
> > the crosspoint
> > switch).
> > One could also register a "mode-switch" there but it wouldn't do
> > anything (all 4 lanes are already
> > connected so there is nothing to re-route in the crosspoint switch).
> > Hence the above "if" check.
>
> Would we still want to route the DP traffic out if the pin assignment
> didn't have DP? Does the hardware support some mode where the DP traffic
> is shutdown? Or maybe the HPD pin needs to be quieted unless DP is
> assigned?

I reference this below, but in the 1 connector case, CC lines would also be
routed to the anx7625 from the usb-connector, so it will know when HPD
is asserted
or not.

>
> I suppose none of those things matter though as long as there is some
> typec switch registered here so that the driver can be informed of the
> pin assignment. Is it right that the "mode-switch" property is only
> required in DT if this device is going to control the mode of the
> connector, i.e. USB+DP, or just DP? Where this device can't do that
> because it doesn't support only DP.

If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
the USB+DP case), a mode-switch wouldn't be of much use, since one
would also route the CC lines to the built-in PD controller; so it will
already have knowledge of what mode the switch is in.

The mode-switch is likely only relevant for this hardware configuration(
it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
One only has 2 SS lanes each (from each usb-c-connector).

Since there is no CC-line, the anx7625 needs to know which one has DP
enabled on it.

>
> >
> > Unfortunately, I don't have hardware which connects all 4 SS lanes
> > from 1 Type-C port
> > to the anx7625, so I didn't add the orientation switch handling to the
> > driver (since I have no way of verifying it).
>
> Alright. Maybe add a TODO then so it's more obvious that orientation
> isn't handled.

Ack. Will add a comment in v6.

>
> >
> > Regarding DP alt-mode pin assignments : I think anx7625 will only support Pin D
> > (only 2 lane DP, no 4 lane DP).
> >
>
> Makes sense. Thanks!

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Stephen Boyd]

Quoting Prashant Malani (2022-06-28 13:56:22)
> On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > I suppose none of those things matter though as long as there is some
> > typec switch registered here so that the driver can be informed of the
> > pin assignment. Is it right that the "mode-switch" property is only
> > required in DT if this device is going to control the mode of the
> > connector, i.e. USB+DP, or just DP? Where this device can't do that
> > because it doesn't support only DP.
>
> If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
> the USB+DP case), a mode-switch wouldn't be of much use, since one
> would also route the CC lines to the built-in PD controller; so it will
> already have knowledge of what mode the switch is in.
>
> The mode-switch is likely only relevant for this hardware configuration(
> it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
> One only has 2 SS lanes each (from each usb-c-connector).
>
> Since there is no CC-line, the anx7625 needs to know which one has DP
> enabled on it.

Can the CC line be "captured" and not actually sent to the anx7625? I
imagine if that is possible, maybe the CC lines would go to some
micro-controller or something that did more typec management things and
then the anx7625 driver would need to do software control of the mode
and orientation control.

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

On Thu, Jun 30, 2022 at 4:21 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-28 13:56:22)
> > On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > I suppose none of those things matter though as long as there is some
> > > typec switch registered here so that the driver can be informed of the
> > > pin assignment. Is it right that the "mode-switch" property is only
> > > required in DT if this device is going to control the mode of the
> > > connector, i.e. USB+DP, or just DP? Where this device can't do that
> > > because it doesn't support only DP.
> >
> > If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
> > the USB+DP case), a mode-switch wouldn't be of much use, since one
> > would also route the CC lines to the built-in PD controller; so it will
> > already have knowledge of what mode the switch is in.
> >
> > The mode-switch is likely only relevant for this hardware configuration(
> > it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
> > One only has 2 SS lanes each (from each usb-c-connector).
> >
> > Since there is no CC-line, the anx7625 needs to know which one has DP
> > enabled on it.
>
> Can the CC line be "captured" and not actually sent to the anx7625?

That's what happens on Chrome OS. The cc line goes to the EC (and is "consumed"
by the TCPM (Type C Port Manager)) and signals are then sent to the AP
over the Host command interface to `cros-ec-typec`. The signals here being all
the PD messages communicated between the peripheral and the port.

> I imagine if that is possible, maybe the CC lines would go to some
> micro-controller or something that did more typec management things and
> then the anx7625 driver would need to do software control of the mode
> and orientation control.

I _guess_ that is possible (though it would seem odd to not use all the PD
control hardware in that configuration)? If an system implements it in
such a way
then:
1. mode-switch: Can be updated to do something when num_typec_switches == 1 (
in the mux_set function imp.l I haven't looked into what registers
need to be configured, since we
don't have this hardware implementation.
2. orientation-switch: This should be registered, and then flip the
lanes when the port
driver tells it the orientation is one way or another.

So, if someone uses it that way, I think the driver needs only minor
updates to support it.

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

On Thu, Jun 30, 2022 at 4:38 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> On Thu, Jun 30, 2022 at 4:21 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-28 13:56:22)
> > > On Tue, Jun 28, 2022 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > I suppose none of those things matter though as long as there is some
> > > > typec switch registered here so that the driver can be informed of the
> > > > pin assignment. Is it right that the "mode-switch" property is only
> > > > required in DT if this device is going to control the mode of the
> > > > connector, i.e. USB+DP, or just DP? Where this device can't do that
> > > > because it doesn't support only DP.
> > >
> > > If the anx7625 is used just to route all lanes from 1 usb-c-connector (i.e
> > > the USB+DP case), a mode-switch wouldn't be of much use, since one
> > > would also route the CC lines to the built-in PD controller; so it will
> > > already have knowledge of what mode the switch is in.
> > >
> > > The mode-switch is likely only relevant for this hardware configuration(
> > > it's "DP only" in the sense that the USB pins to the SoC never go anywhere).
> > > One only has 2 SS lanes each (from each usb-c-connector).
> > >
> > > Since there is no CC-line, the anx7625 needs to know which one has DP
> > > enabled on it.
> >
> > Can the CC line be "captured" and not actually sent to the anx7625?
>
> That's what happens on Chrome OS. The cc line goes to the EC (and is "consumed"
> by the TCPM (Type C Port Manager)) and signals are then sent to the AP
> over the Host command interface to `cros-ec-typec`. The signals here being all
> the PD messages communicated between the peripheral and the port.
>
> > I imagine if that is possible, maybe the CC lines would go to some
> > micro-controller or something that did more typec management things and
> > then the anx7625 driver would need to do software control of the mode
> > and orientation control.
>
> I _guess_ that is possible (though it would seem odd to not use all the PD
> control hardware in that configuration)? If an system implements it in
> such a way
> then:
> 1. mode-switch: Can be updated to do something when num_typec_switches == 1 (
> in the mux_set function imp.l I haven't looked into what registers
> need to be configured, since we
> don't have this hardware implementation.
> 2. orientation-switch: This should be registered, and then flip the
> lanes when the port
> driver tells it the orientation is one way or another.
>
> So, if someone uses it that way, I think the driver needs only minor
> updates to support it.

Stephen, any pending concerns?
If not,I will post a v6 series with the suggested changes:
- Drop typec-switch binding; instead add a new top-level port with
end-points for each Type-C connector's switch.
- Drop it6505 patches.
- Squash anx7625 driver patches into one patch.
- Add a comment mentioning that we aren't registering the orientation-switch.

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Stephen Boyd]

Quoting Prashant Malani (2022-07-06 11:26:19)
>
> Stephen, any pending concerns?

No more pending concerns.

> If not,I will post a v6 series with the suggested changes:
> - Drop typec-switch binding; instead add a new top-level port with
> end-points for each Type-C connector's switch.
> - Drop it6505 patches.
> - Squash anx7625 driver patches into one patch.
> - Add a comment mentioning that we aren't registering the orientation-switch.

Ok. I'll take a look on v6.

Re: [PATCH v5 5/9] drm/bridge: anx7625: Add typec_mux_set callback function

[Pin-yen Lin]

On Thu, Jul 7, 2022 at 8:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-07-06 11:26:19)
> >
> > Stephen, any pending concerns?
>
> No more pending concerns.
>
> > If not,I will post a v6 series with the suggested changes:
> > - Drop typec-switch binding; instead add a new top-level port with
> > end-points for each Type-C connector's switch.
> > - Drop it6505 patches.
> > - Squash anx7625 driver patches into one patch.
> > - Add a comment mentioning that we aren't registering the orientation-switch.

We've been working on these changes, and the new DT node looks like this:

```
    anx_bridge_dp: anx7625-dp@58 {
        [...]
        mode-switch;
        ports {
            [...]
            typec_switches: port@2 {
                #adderss-cells = <1>;
                #size-cells = <0>;
                reg = <2>;

                anx_typec0: endpoint@0 {
                    reg = <0>;
                    remote-endpoint = <&typec_port0>;
                };
                anx_typec1: endpoint@1 {
                    reg = <1>;
                    remote-endpoint = <&typec_port1>;
                };
            };
        };
```

However we found some issues with that approach:
1. The current typec mux API does not allow us to put muxes into
`ports` directly.
`fwnode_typec_mux_get` searches for the parent node behind the port(s)
nodes, so we cannot register the muxes with the port nodes unless we
change the interface.
2. We need a compatible string between the `endpoint` nodes and the
parent node (anx7625-dp@58).
This is because when the driver core builds the device links, they
only add links on nodes with a compatible string for `remote-endpoint`
properties[1].
Without a compatible string, the parent node of `typec_port0`
(cros-ec-typec in our case) has to be probed before anx7625, but this
leads to a deadlock because cros-ec-typec requires anx7625 to register
the typec_mux drivers first. I'm not sure if this is cros-ec-typec
specific, though.
*Any* compatible string fixes this issue, and it doesn't have to be
"typec-switch".

--

Alternatively, can we split the two muxes into two sub-nodes, like the
following snippet?

```
    anx_bridge_dp: anx7625-dp@58 {
        [...]
        mode-switch;

        anx_mux0 {
            compatible = "typec-switch";
            reg = <0>;

            port {
                anx_typec0: endpoint {
                    remote-endpoint = <&typec_port0>;
                };
            };
        };

        anx_mux1 {
            compatible = "typec-switch";
            reg = <1>;

            port {
                anx_typec1: endpoint {
                    remote-endpoint = <&typec_port1>;
                };
            };
        };
```

This eliminates the additional "switches" node in the devicetree. The
sub-nodes also describe our hardware design, which split the DP lanes
of anx7625 to two type-c ports.

[1]: The `node_not_dev` property searches for a node with a compatible
string: https://elixir.bootlin.com/linux/latest/source/drivers/of/property.c#L1390



>
> Ok. I'll take a look on v6.

[PATCH v5 6/9] dt/bindings: drm/bridge: it6505: Add mode-switch support

[Prashant Malani]

From: Pin-Yen Lin <treapking@chromium.org>

ITE IT6505 can be used in systems to switch USB Type-C DisplayPort
alternate mode lane traffic between 2 Type-C ports.

Update the binding to accommodate this usage by introducing a switch
property.

Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

v5 is the first version for this patch.

 .../bindings/display/bridge/ite,it6505.yaml   | 97 ++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
index 833d11b2303a..86bb6dc5ae6f 100644
--- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
@@ -56,13 +56,46 @@ properties:
     $ref: /schemas/graph.yaml#/properties/port
     description: A port node pointing to DPI host port node
 
+  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
   - ovdd-supply
   - pwr18-supply
   - interrupts
   - reset-gpios
-  - extcon
+
+oneOf:
+  - required:
+      - extcon
+  - required:
+      - switches
 
 additionalProperties: false
 
@@ -92,3 +125,65 @@ examples:
             };
         };
     };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c3 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        it6505dptx: it6505dptx@5c {
+            compatible = "ite,it6505";
+            interrupts = <8 IRQ_TYPE_LEVEL_LOW 8 0>;
+            reg = <0x5c>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&it6505_pins>;
+            ovdd-supply = <&mt6366_vsim2_reg>;
+            pwr18-supply = <&pp1800_dpbrdg_dx>;
+            reset-gpios = <&pio 177 0>;
+
+            port {
+                it6505_dp_in: endpoint {
+                    remote-endpoint = <&dpi_out>;
+                };
+            };
+
+            switches {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                switch@0 {
+                    compatible = "typec-switch";
+                    reg = <0>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            ite_typec0: endpoint {
+                                remote-endpoint = <&typec_port0>;
+                            };
+                        };
+                    };
+                };
+
+                switch@1 {
+                    compatible = "typec-switch";
+                    reg = <1>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            ite_typec1: endpoint {
+                                remote-endpoint = <&typec_port1>;
+                            };
+                        };
+                    };
+                };
+            };
+        };
+    };
-- 
2.37.0.rc0.104.g0611611a94-goog

Re: [PATCH v5 6/9] dt/bindings: drm/bridge: it6505: Add mode-switch support

[Stephen Boyd]

Quoting Prashant Malani (2022-06-22 10:34:35)
> From: Pin-Yen Lin <treapking@chromium.org>
>
> ITE IT6505 can be used in systems to switch USB Type-C DisplayPort
> alternate mode lane traffic between 2 Type-C ports.

How does it work? From what I can tell from the information I find when
googling this part[1] and looking at the existing binding doc is that
this device is a DPI to DP bridge, and it outputs DP (probably 4 lanes
of it?). Does the 2 type-c port design work by transmitting DP on two
lanes of DP for one type-c port and another two lanes of DP for the
other type-c port?

DP could be one lane, so if this device is able to output one lane on
any output differential pair then I suspect it could support 4 type-c
ports if the hardware engineer connected it that way. Can you confirm my
suspicion?

[1] https://www.ite.com.tw/en/product/view?mid=45

Re: [PATCH v5 6/9] dt/bindings: drm/bridge: it6505: Add mode-switch support

[Prashant Malani]

On Thu, Jun 23, 2022 at 11:24 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-22 10:34:35)
> > From: Pin-Yen Lin <treapking@chromium.org>
> >
> > ITE IT6505 can be used in systems to switch USB Type-C DisplayPort
> > alternate mode lane traffic between 2 Type-C ports.
>
> How does it work? From what I can tell from the information I find when
> googling this part[1] and looking at the existing binding doc is that
> this device is a DPI to DP bridge, and it outputs DP (probably 4 lanes
> of it?). Does the 2 type-c port design work by transmitting DP on two
> lanes of DP for one type-c port and another two lanes of DP for the
> other type-c port?
>
> DP could be one lane, so if this device is able to output one lane on
> any output differential pair then I suspect it could support 4 type-c
> ports if the hardware engineer connected it that way. Can you confirm my
> suspicion?

I will let Pin-Yen comment re: this hardware, but 1-lane DP is not a
supported Type-C Pin assignment
(as per VESA DP Alternate Mode Spec version 2.0 [2]), so the H/W
configuration you are suggesting shouldn't be possible.

>
> [1] https://www.ite.com.tw/en/product/view?mid=45

Re: [PATCH v5 6/9] dt/bindings: drm/bridge: it6505: Add mode-switch support

[Stephen Boyd]

Quoting Prashant Malani (2022-06-23 11:37:08)
> On Thu, Jun 23, 2022 at 11:24 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Prashant Malani (2022-06-22 10:34:35)
> > > From: Pin-Yen Lin <treapking@chromium.org>
> > >
> > > ITE IT6505 can be used in systems to switch USB Type-C DisplayPort
> > > alternate mode lane traffic between 2 Type-C ports.
> >
> > How does it work? From what I can tell from the information I find when
> > googling this part[1] and looking at the existing binding doc is that
> > this device is a DPI to DP bridge, and it outputs DP (probably 4 lanes
> > of it?). Does the 2 type-c port design work by transmitting DP on two
> > lanes of DP for one type-c port and another two lanes of DP for the
> > other type-c port?
> >
> > DP could be one lane, so if this device is able to output one lane on
> > any output differential pair then I suspect it could support 4 type-c
> > ports if the hardware engineer connected it that way. Can you confirm my
> > suspicion?
>
> I will let Pin-Yen comment re: this hardware, but 1-lane DP is not a
> supported Type-C Pin assignment
> (as per VESA DP Alternate Mode Spec version 2.0 [2]), so the H/W

Some missing link?

> configuration you are suggesting shouldn't be possible.
>

Alright, cool. But it is possible in the DP spec. So it seems like if
this is connected to 4 DP connectors it could be used to mux between DP
on 4 DP ports.

Re: [PATCH v5 6/9] dt/bindings: drm/bridge: it6505: Add mode-switch support

[Prashant Malani]

On Thu, Jun 23, 2022 at 12:08 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-23 11:37:08)
> > On Thu, Jun 23, 2022 at 11:24 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Prashant Malani (2022-06-22 10:34:35)
> > > > From: Pin-Yen Lin <treapking@chromium.org>
> > > >
> > > > ITE IT6505 can be used in systems to switch USB Type-C DisplayPort
> > > > alternate mode lane traffic between 2 Type-C ports.
> > >
> > > How does it work? From what I can tell from the information I find when
> > > googling this part[1] and looking at the existing binding doc is that
> > > this device is a DPI to DP bridge, and it outputs DP (probably 4 lanes
> > > of it?). Does the 2 type-c port design work by transmitting DP on two
> > > lanes of DP for one type-c port and another two lanes of DP for the
> > > other type-c port?
> > >
> > > DP could be one lane, so if this device is able to output one lane on
> > > any output differential pair then I suspect it could support 4 type-c
> > > ports if the hardware engineer connected it that way. Can you confirm my
> > > suspicion?
> >
> > I will let Pin-Yen comment re: this hardware, but 1-lane DP is not a
> > supported Type-C Pin assignment
> > (as per VESA DP Alternate Mode Spec version 2.0 [2]), so the H/W
>
> Some missing link?

My bad. I tried to find a publicly accessible link to the DP altmode
spec, but it
seems like one needs to be a VESA member to access it :/

>
> > configuration you are suggesting shouldn't be possible.
> >
>
> Alright, cool. But it is possible in the DP spec. So it seems like if
> this is connected to 4 DP connectors it could be used to mux between DP
> on 4 DP ports.

Ack. In that case, no "typec-switches" should be added to the DT.

[PATCH v5 7/9] drm/bridge: it6505: Register number of Type C switches

[Prashant Malani]

From: Pin-Yen Lin <treapking@chromium.org>

Parse the "switches" node, if available, and count and store the number
of Type-C switches within it. The extcon registration is still
supported, but we don't expect both extcon and typec-switch be
registered at the same time.

This patch sets a foundation for the actual registering of Type-C
switches with the Type-C connector class framework.

Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

v5 is the first version for this patch.

 drivers/gpu/drm/bridge/ite-it6505.c | 34 +++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 4b673c4792d7..b259f9f367f6 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -452,6 +452,7 @@ struct it6505 {
 	struct delayed_work delayed_audio;
 	struct it6505_audio_data audio;
 	struct dentry *debugfs;
+	int num_typec_switches;
 
 	/* it6505 driver hold option */
 	bool enable_drv_hold;
@@ -3229,13 +3230,28 @@ static void it6505_shutdown(struct i2c_client *client)
 		it6505_lane_off(it6505);
 }
 
+static int it6505_register_typec_switches(struct device *device, struct it6505 *it6505)
+{
+	struct device_node *of;
+
+	of = of_get_child_by_name(device->of_node, "switches");
+	if (!of)
+		return -ENODEV;
+
+	it6505->num_typec_switches = of_get_child_count(of);
+	if (it6505->num_typec_switches <= 0)
+		return -ENODEV;
+
+	return 0;
+}
+
 static int it6505_i2c_probe(struct i2c_client *client,
 			    const struct i2c_device_id *id)
 {
 	struct it6505 *it6505;
 	struct device *dev = &client->dev;
 	struct extcon_dev *extcon;
-	int err, intp_irq;
+	int err, intp_irq, ret;
 
 	it6505 = devm_kzalloc(&client->dev, sizeof(*it6505), GFP_KERNEL);
 	if (!it6505)
@@ -3255,11 +3271,21 @@ static int it6505_i2c_probe(struct i2c_client *client,
 	if (PTR_ERR(extcon) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 	if (IS_ERR(extcon)) {
-		dev_err(dev, "can not get extcon device!");
-		return PTR_ERR(extcon);
+		if (PTR_ERR(extcon) != -ENODEV)
+			dev_warn(dev, "Cannot get extcon device: %ld", PTR_ERR(extcon));
+		it6505->extcon = NULL;
+	} else {
+		it6505->extcon = extcon;
 	}
 
-	it6505->extcon = extcon;
+	ret = it6505_register_typec_switches(dev, it6505);
+	if (ret) {
+		dev_dbg(dev, "Didn't register Type C switches, err: %d", ret);
+		if (!it6505->extcon) {
+			dev_err(dev, "Both extcon and typec-switch are not registered.");
+			return -EINVAL;
+		}
+	}
 
 	it6505->regmap = devm_regmap_init_i2c(client, &it6505_regmap_config);
 	if (IS_ERR(it6505->regmap)) {
-- 
2.37.0.rc0.104.g0611611a94-goog

Re: [PATCH v5 7/9] drm/bridge: it6505: Register number of Type C switches

[Rob Herring]

On Wed, Jun 22, 2022 at 05:34:36PM +0000, Prashant Malani wrote:
> From: Pin-Yen Lin <treapking@chromium.org>
> 
> Parse the "switches" node, if available, and count and store the number
> of Type-C switches within it. The extcon registration is still
> supported, but we don't expect both extcon and typec-switch be
> registered at the same time.
> 
> This patch sets a foundation for the actual registering of Type-C
> switches with the Type-C connector class framework.
> 
> Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> v5 is the first version for this patch.
> 
>  drivers/gpu/drm/bridge/ite-it6505.c | 34 +++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 4b673c4792d7..b259f9f367f6 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -452,6 +452,7 @@ struct it6505 {
>  	struct delayed_work delayed_audio;
>  	struct it6505_audio_data audio;
>  	struct dentry *debugfs;
> +	int num_typec_switches;
>  
>  	/* it6505 driver hold option */
>  	bool enable_drv_hold;
> @@ -3229,13 +3230,28 @@ static void it6505_shutdown(struct i2c_client *client)
>  		it6505_lane_off(it6505);
>  }
>  
> +static int it6505_register_typec_switches(struct device *device, struct it6505 *it6505)
> +{
> +	struct device_node *of;
> +
> +	of = of_get_child_by_name(device->of_node, "switches");
> +	if (!of)
> +		return -ENODEV;
> +
> +	it6505->num_typec_switches = of_get_child_count(of);
> +	if (it6505->num_typec_switches <= 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}

Not that I expect this to remain, but you have the same function in 2 
files. That's a clear sign this belongs in a helper.

Rob

[PATCH v5 8/9] drm/bridge: it6505: Register Type-C mode switches

[Prashant Malani]

From: Pin-Yen Lin <treapking@chromium.org>

When the DT node has "switches" available, register a Type-C mode-switch
for each listed "switch". This allows the driver to receive state
information about what operating mode a Type-C port and its connected
peripherals are in, as well as status information (like VDOs) related to
that state.

The callback function is currently a stub, but subsequent patches will
implement the required functionality.

Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

v5 is the first version for this patch.

 drivers/gpu/drm/bridge/ite-it6505.c | 85 ++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index b259f9f367f6..cb1dd4cbd33b 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -17,6 +17,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/types.h>
+#include <linux/usb/typec_mux.h>
 #include <linux/wait.h>
 
 #include <crypto/hash.h>
@@ -402,6 +403,11 @@ struct debugfs_entries {
 	const struct file_operations *fops;
 };
 
+struct it6505_port_data {
+	struct typec_mux_dev *typec_mux;
+	struct it6505 *it6505;
+};
+
 struct it6505 {
 	struct drm_dp_aux aux;
 	struct drm_bridge bridge;
@@ -453,6 +459,7 @@ struct it6505 {
 	struct it6505_audio_data audio;
 	struct dentry *debugfs;
 	int num_typec_switches;
+	struct it6505_port_data *typec_ports;
 
 	/* it6505 driver hold option */
 	bool enable_drv_hold;
@@ -3230,9 +3237,59 @@ static void it6505_shutdown(struct i2c_client *client)
 		it6505_lane_off(it6505);
 }
 
+static int it6505_typec_mux_set(struct typec_mux_dev *mux,
+				struct typec_mux_state *state)
+{
+	return 0;
+}
+
+static int it6505_register_mode_switch(struct device *dev, struct device_node *node,
+					struct it6505 *it6505)
+{
+	struct it6505_port_data *port_data;
+	struct typec_mux_desc mux_desc = {};
+	char name[32];
+	u32 port_num;
+	int ret;
+
+	ret = of_property_read_u32(node, "reg", &port_num);
+	if (ret)
+		return ret;
+
+	if (port_num >= it6505->num_typec_switches) {
+		dev_err(dev, "Invalid port number specified: %d\n", port_num);
+		return -EINVAL;
+	}
+
+	port_data = &it6505->typec_ports[port_num];
+	port_data->it6505 = it6505;
+	mux_desc.fwnode = &node->fwnode;
+	mux_desc.drvdata = port_data;
+	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
+	mux_desc.name = name;
+	mux_desc.set = it6505_typec_mux_set;
+
+	port_data->typec_mux = typec_mux_register(dev, &mux_desc);
+	if (IS_ERR(port_data->typec_mux)) {
+		ret = PTR_ERR(port_data->typec_mux);
+		dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
+	}
+
+	return ret;
+}
+
+static void it6505_unregister_typec_switches(struct it6505 *it6505)
+{
+	int i;
+
+	for (i = 0; i < it6505->num_typec_switches; i++)
+		typec_mux_unregister(it6505->typec_ports[i].typec_mux);
+}
+
 static int it6505_register_typec_switches(struct device *device, struct it6505 *it6505)
 {
-	struct device_node *of;
+	struct device_node *of, *sw;
+	int ret = 0;
 
 	of = of_get_child_by_name(device->of_node, "switches");
 	if (!of)
@@ -3241,8 +3298,28 @@ static int it6505_register_typec_switches(struct device *device, struct it6505 *
 	it6505->num_typec_switches = of_get_child_count(of);
 	if (it6505->num_typec_switches <= 0)
 		return -ENODEV;
+	it6505->typec_ports = devm_kzalloc(device,
+					   it6505->num_typec_switches *
+					   sizeof(struct it6505_port_data),
+					   GFP_KERNEL);
+	if (!it6505->typec_ports)
+		return -ENOMEM;
 
-	return 0;
+	/* Register switches for each connector. */
+	for_each_available_child_of_node(of, sw) {
+		if (!of_property_read_bool(sw, "mode-switch"))
+			continue;
+		ret = it6505_register_mode_switch(device, sw, it6505);
+		if (ret) {
+			dev_err(device, "Failed to register mode switch: %d\n", ret);
+			break;
+		}
+	}
+
+	if (ret)
+		it6505_unregister_typec_switches(it6505);
+
+	return ret;
 }
 
 static int it6505_i2c_probe(struct i2c_client *client,
@@ -3280,7 +3357,8 @@ static int it6505_i2c_probe(struct i2c_client *client,
 
 	ret = it6505_register_typec_switches(dev, it6505);
 	if (ret) {
-		dev_dbg(dev, "Didn't register Type C switches, err: %d", ret);
+		if (ret != -ENODEV)
+			dev_warn(dev, "Didn't register Type C switches, err: %d", ret);
 		if (!it6505->extcon) {
 			dev_err(dev, "Both extcon and typec-switch are not registered.");
 			return -EINVAL;
@@ -3350,6 +3428,7 @@ static int it6505_i2c_remove(struct i2c_client *client)
 	drm_dp_aux_unregister(&it6505->aux);
 	it6505_debugfs_remove(it6505);
 	it6505_poweroff(it6505);
+	it6505_unregister_typec_switches(it6505);
 
 	return 0;
 }
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v5 9/9] drm/bridge: it6505: Add typec_mux_set callback function

[Prashant Malani]

From: Pin-Yen Lin <treapking@chromium.org>

Add the callback function when the driver receives state changes of the
Type-C ports. The callback function configures the lane_swap state and
ends up updating the lane swap registers of it6505 bridge chip.

Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

v5 is the first version for this patch.

 drivers/gpu/drm/bridge/ite-it6505.c | 58 +++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index cb1dd4cbd33b..87b9bd742b52 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -17,6 +17,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/types.h>
+#include <linux/usb/typec_dp.h>
 #include <linux/usb/typec_mux.h>
 #include <linux/wait.h>
 
@@ -404,6 +405,7 @@ struct debugfs_entries {
 };
 
 struct it6505_port_data {
+	bool dp_connected;
 	struct typec_mux_dev *typec_mux;
 	struct it6505 *it6505;
 };
@@ -3237,9 +3239,65 @@ static void it6505_shutdown(struct i2c_client *client)
 		it6505_lane_off(it6505);
 }
 
+static void it6505_typec_ports_update(struct it6505 *it6505)
+{
+	usleep_range(3000, 4000);
+
+	if (it6505->typec_ports[0].dp_connected && it6505->typec_ports[1].dp_connected)
+		/* Both ports available, do nothing to retain the current one. */
+		return;
+	else if (it6505->typec_ports[0].dp_connected)
+		it6505->lane_swap = false;
+	else if (it6505->typec_ports[1].dp_connected)
+		it6505->lane_swap = true;
+
+	usleep_range(3000, 4000);
+}
+
 static int it6505_typec_mux_set(struct typec_mux_dev *mux,
 				struct typec_mux_state *state)
 {
+	struct it6505_port_data *data = typec_mux_get_drvdata(mux);
+	struct it6505 *it6505 = data->it6505;
+	struct device *dev = &it6505->client->dev;
+	bool old_dp_connected, new_dp_connected;
+
+	if (it6505->num_typec_switches == 1)
+		return 0;
+
+	mutex_lock(&it6505->extcon_lock);
+
+	old_dp_connected = it6505->typec_ports[0].dp_connected ||
+			   it6505->typec_ports[1].dp_connected;
+
+	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
+		it6505->typec_ports[0].dp_connected, it6505->typec_ports[1].dp_connected);
+
+	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
+			      state->alt->mode == USB_TYPEC_DP_MODE);
+
+	new_dp_connected = it6505->typec_ports[0].dp_connected ||
+			   it6505->typec_ports[1].dp_connected;
+
+	if (it6505->enable_drv_hold) {
+		dev_dbg(dev, "enable driver hold");
+		goto unlock;
+	}
+
+	it6505_typec_ports_update(it6505);
+
+	if (!old_dp_connected && new_dp_connected)
+		pm_runtime_get_sync(dev);
+
+	if (old_dp_connected && !new_dp_connected) {
+		pm_runtime_put_sync(dev);
+		if (it6505->bridge.dev)
+			drm_helper_hpd_irq_event(it6505->bridge.dev);
+		memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
+	}
+
+unlock:
+	mutex_unlock(&it6505->extcon_lock);
 	return 0;
 }
 
-- 
2.37.0.rc0.104.g0611611a94-goog