| Message ID | 20220622173605.1168416-1-pmalani@chromium.org |
|---|---|
| Headers | show |
| Series | usb: typec: Introduce typec-switch binding | expand |
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>; > + }; > + }; > + }; > + };
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>; > > + }; > > + }; > > + }; > > + };
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
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
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
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.
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.
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?
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