| Message ID | 20220615172129.1314056-1-pmalani@chromium.org |
|---|---|
| Headers | show |
| Series | usb: typec: Introduce typec-switch binding | expand |
Quoting Prashant Malani (2022-06-15 10:20:20) > > .../display/bridge/analogix,anx7625.yaml | 64 +++++++++++++++++++ > 1 file changed, 64 insertions(+) Can this file get a link to the product brief[1]? It helps to quickly find the block diagram. > > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > index 35a48515836e..bc6f7644db31 100644 > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > @@ -105,6 +105,34 @@ properties: > - port@0 > - port@1 > > + switches: > + type: object > + description: Set of switches controlling DisplayPort traffic on > + outgoing RX/TX lanes to Type C ports. > + additionalProperties: false > + > + properties: > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + patternProperties: > + '^switch@[01]$': > + $ref: /schemas/usb/typec-switch.yaml# > + unevaluatedProperties: false > + > + properties: > + reg: > + maxItems: 1 > + > + required: > + - reg > + > + required: > + - switch@0 > + > required: > - compatible > - reg > @@ -167,5 +195,41 @@ examples: > }; > }; > }; > + switches { Is "switches" a bus? > + #address-cells = <1>; > + #size-cells = <0>; > + switch@0 { > + compatible = "typec-switch"; Is this compatible matched against a driver that's populated on this "switches" bus? > + reg = <0>; > + mode-switch; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + anx_typec0: endpoint { > + remote-endpoint = <&typec_port0>; > + }; > + }; > + }; I was expecting to see these simply be more ports in the existing graph binding of this device, and then have the 'mode-switch' or 'orientation-switch' properties be at the same level as the compatible string "analogix,anx7625". Here's the reasoning, based on looking at the product brief and the existing binding/implementation. Looking at the only existing implementation of this binding upstream in mt8183-kukui-jacuzzi.dtsi it looks like one of these typec ports is actually the same physically as the 'anx7625_out' endpoint (reg address of 1) that is already defined in the binding. It seems that MIPI DSI/DPI comes in and is output through 2 lanes, SSRX2 and SSTX2 according to the product brief[1], and that is connected to some eDP panel ("auo,b116xw03"). Presumably that is the same as anx_typec1 in this patch? I suspect the USB3.1 input is not connected on this board, and thus the crosspoint switch is never used, nor the SSRX1/SSTX1 pins. The existing binding defines the MIPI DSI/DPI input as port0 and two of the four lanes of output that is probably by default connected to the "DisplayPort Transmitter" as port1 because that's how the crosspoint switch comes out of reset. That leaves the USB3.1 input possibly needing a port in the ports binding, and the other two lanes of output needing a port in the ports binding to describe their connection to the downstream device. And finally information about if the crosspoint switch needs to be registered with the typec framework to do typec things, which can be achieved by the presence of the 'mode-switch' property. On a board like kukui-jacuzzi these new properties and ports wouldn't be specified, because what is there is already sufficient. If this chip is connected to a usb-c-connector then I'd expect to see a connection from the output ports in the graph binding to the connector node's ports. There aren't any ports in the usb-c-connector binding though from what I see. I believe there's also one more use case here where USB3.1 or MIPI DSI/DPI is connected on the input side and this device is used to steer USB3.1 or DP through the crosspoint switch to either of the two output pairs. This last scenario means that we have to describe both output pairs, SSRX1/SSTX1 and SSRX2/SSTX2, as different ports in the binding so they can be connected to different usb-c-connectors if the hardware engineer wired the output pins that way. TL;DR: Can we add 'mode-switch' as an optional property and two more ports at address 2 and 3 for the USB3.1 input and the SSRX1/SSTX1 pair respectively to the existing graph part of this binding? > + }; > + 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>; > + }; > + }; > + }; > + }; > + }; > }; [1] https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf
Il 15/06/22 19:20, Prashant Malani ha scritto: > 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 patch introduces a change to the Type-C mux class mode-switch > matching code, while the second adds a config guard to a Type-C header. > The next couple of patches introduce the new "typec-switch" binding as > well as one user of it (the ANX7625 drm bridge). > > The remaining patches 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. > > v3: https://lore.kernel.org/linux-usb/20220614193558.1163205-1-pmalani@chromium.org/ > For the entire series: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Regards, Angelo
On Thu, Jun 16, 2022 at 01:54:36AM -0700, Prashant Malani wrote: > On Thu, Jun 16, 2022 at 12:42 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Prashant Malani (2022-06-15 10:20:20) > > > > > > .../display/bridge/analogix,anx7625.yaml | 64 +++++++++++++++++++ > > > 1 file changed, 64 insertions(+) > > > > Can this file get a link to the product brief[1]? It helps to quickly > > find the block diagram. > > Sure, but I don't really think that should be included in this patch > (or series). > I'd be happy to submit a separate patch once this series is resolved. > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > index 35a48515836e..bc6f7644db31 100644 > > > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > @@ -105,6 +105,34 @@ properties: > > > - port@0 > > > - port@1 > > > > > > + switches: > > > + type: object > > > + description: Set of switches controlling DisplayPort traffic on > > > + outgoing RX/TX lanes to Type C ports. > > > + additionalProperties: false > > > + > > > + properties: > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > + > > > + patternProperties: > > > + '^switch@[01]$': > > > + $ref: /schemas/usb/typec-switch.yaml# > > > + unevaluatedProperties: false > > > + > > > + properties: > > > + reg: > > > + maxItems: 1 > > > + > > > + required: > > > + - reg > > > + > > > + required: > > > + - switch@0 > > > + > > > required: > > > - compatible > > > - reg > > > @@ -167,5 +195,41 @@ examples: > > > }; > > > }; > > > }; > > > + switches { > > > > Is "switches" a bus? > > No. > > > > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + switch@0 { > > > + compatible = "typec-switch"; > > > > Is this compatible matched against a driver that's populated on this > > "switches" bus? > > No. Patch 6/7 has the implementation details on how the anx driver > performs the enumeration of switches. > > > > > > + reg = <0>; > > > + mode-switch; > > > + > > > + ports { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + port@0 { > > > + reg = <0>; > > > + anx_typec0: endpoint { > > > + remote-endpoint = <&typec_port0>; > > > + }; > > > + }; > > > + }; > > > > I was expecting to see these simply be more ports in the existing graph > > binding of this device, and then have the 'mode-switch' or > > 'orientation-switch' properties be at the same level as the compatible > > string "analogix,anx7625". Here's the reasoning, based on looking at the > > product brief and the existing binding/implementation. > > > > Looking at the only existing implementation of this binding upstream in > > mt8183-kukui-jacuzzi.dtsi it looks like one of these typec ports is > > actually the same physically as the 'anx7625_out' endpoint (reg address > > of 1) that is already defined in the binding. It seems that MIPI DSI/DPI > > comes in and is output through 2 lanes, SSRX2 and SSTX2 according to the > > product brief[1], and that is connected to some eDP panel > > ("auo,b116xw03"). Presumably that is the same as anx_typec1 in this > > patch? I suspect the USB3.1 input is not connected on this board, and > > thus the crosspoint switch is never used, nor the SSRX1/SSTX1 pins. > > > > The existing binding defines the MIPI DSI/DPI input as port0 and two of > > the four lanes of output that is probably by default connected to the > > "DisplayPort Transmitter" as port1 because that's how the crosspoint > > switch comes out of reset. That leaves the USB3.1 input possibly needing > > a port in the ports binding, and the other two lanes of output needing a > > port in the ports binding to describe their connection to the downstream > > device. And finally information about if the crosspoint switch needs to > > be registered with the typec framework to do typec things, which can be > > achieved by the presence of the 'mode-switch' property. > > > > On a board like kukui-jacuzzi these new properties and ports wouldn't be > > specified, because what is there is already sufficient. If this chip is > > connected to a usb-c-connector then I'd expect to see a connection from > > the output ports in the graph binding to the connector node's ports. > > There aren't any ports in the usb-c-connector binding though from what I > > see. > > > > I believe there's also one more use case here where USB3.1 or MIPI > > DSI/DPI is connected on the input side and this device is used to steer > > USB3.1 or DP through the crosspoint switch to either of the two output > > pairs. This last scenario means that we have to describe both output > > pairs, SSRX1/SSTX1 and SSRX2/SSTX2, as different ports in the binding so > > they can be connected to different usb-c-connectors if the hardware > > engineer wired the output pins that way. > > > > TL;DR: Can we add 'mode-switch' as an optional property and two more > > ports at address 2 and 3 for the USB3.1 input and the SSRX1/SSTX1 pair > > respectively to the existing graph part of this binding? > > Sorry, but I got lost midway through the preceding explanation. Made sense to me. > The binding > can always add additional ports to each "switch" to accomplish the > graph connections > you are alluding to (if the driver needs/uses it, which I don't think > this one does at present). Why is the switch special? If I just look at this from a block diagram perspective, I just see a list of interfaces that need to be described in the graph. > Adding extra ports to existing ports gets tricky from a mode-switch > enumeration perspective (which > ports should have the modes switches, which shouldn't? Do you follow > the remote end points for each port > and see which one is a Type C connector? The driver knows which port is which because the binding has to define it. So you have to check 2 of them (SSRX1/SSTX1 and SSRX2/SSTX2) to find usb C connectors. > What if we add an > intermediate switch device in the future?) > Having a dedicated "switch" binding makes this consistent and easy > (port0 will always have the end-point for the switch). > > While there may be more than 1 valid approach here, I believe the > current one is appropriate. To put it simply, if you want to define a generic binding, I want to see at least 2 users of it. What I really want to see is someone looking at all the Type-C related bindings and h/w possibilities, not just 1 problem or their own h/w. IOW, a Type-C binding czar. Rob
On Wed, Jun 15, 2022 at 11:13:33AM -0700, Prashant Malani wrote: > I should add: > > Series submission suggestions (of course, open to better suggestions too): > - Patches 1-3 can go through the USB repo. I will take patches 1 and 2 now. seems the others need reworks or acks from the DT people. thanks, greg k-h
On Thu, Jun 16, 2022 at 12:57 PM Prashant Malani <pmalani@chromium.org> wrote: > > On Thu, Jun 16, 2022 at 12:34 PM Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Jun 16, 2022 at 01:54:36AM -0700, Prashant Malani wrote: > > > On Thu, Jun 16, 2022 at 12:42 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > Quoting Prashant Malani (2022-06-15 10:20:20) > > > > > > > > > > .../display/bridge/analogix,anx7625.yaml | 64 +++++++++++++++++++ > > > > > 1 file changed, 64 insertions(+) > > > > > > > > Can this file get a link to the product brief[1]? It helps to quickly > > > > find the block diagram. > > > > > > Sure, but I don't really think that should be included in this patch > > > (or series). > > > I'd be happy to submit a separate patch once this series is resolved. > > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > > > index 35a48515836e..bc6f7644db31 100644 > > > > > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > > > @@ -105,6 +105,34 @@ properties: > > > > > - port@0 > > > > > - port@1 > > > > > > > > > > + switches: > > > > > + type: object > > > > > + description: Set of switches controlling DisplayPort traffic on > > > > > + outgoing RX/TX lanes to Type C ports. > > > > > + additionalProperties: false > > > > > + > > > > > + properties: > > > > > + '#address-cells': > > > > > + const: 1 > > > > > + > > > > > + '#size-cells': > > > > > + const: 0 > > > > > + > > > > > + patternProperties: > > > > > + '^switch@[01]$': > > > > > + $ref: /schemas/usb/typec-switch.yaml# > > > > > + unevaluatedProperties: false > > > > > + > > > > > + properties: > > > > > + reg: > > > > > + maxItems: 1 > > > > > + > > > > > + required: > > > > > + - reg > > > > > + > > > > > + required: > > > > > + - switch@0 > > > > > + > > > > > required: > > > > > - compatible > > > > > - reg > > > > > @@ -167,5 +195,41 @@ examples: > > > > > }; > > > > > }; > > > > > }; > > > > > + switches { > > > > > > > > Is "switches" a bus? > > > > > > No. > > > > > > > > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + switch@0 { > > > > > + compatible = "typec-switch"; > > > > > > > > Is this compatible matched against a driver that's populated on this > > > > "switches" bus? > > > > > > No. Patch 6/7 has the implementation details on how the anx driver > > > performs the enumeration of switches. > > > > > > > > > > > > + reg = <0>; > > > > > + mode-switch; > > > > > + > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + port@0 { > > > > > + reg = <0>; > > > > > + anx_typec0: endpoint { > > > > > + remote-endpoint = <&typec_port0>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > > > > I was expecting to see these simply be more ports in the existing graph > > > > binding of this device, and then have the 'mode-switch' or > > > > 'orientation-switch' properties be at the same level as the compatible > > > > string "analogix,anx7625". Here's the reasoning, based on looking at the > > > > product brief and the existing binding/implementation. > > > > > > > > Looking at the only existing implementation of this binding upstream in > > > > mt8183-kukui-jacuzzi.dtsi it looks like one of these typec ports is > > > > actually the same physically as the 'anx7625_out' endpoint (reg address > > > > of 1) that is already defined in the binding. It seems that MIPI DSI/DPI > > > > comes in and is output through 2 lanes, SSRX2 and SSTX2 according to the > > > > product brief[1], and that is connected to some eDP panel > > > > ("auo,b116xw03"). Presumably that is the same as anx_typec1 in this > > > > patch? I suspect the USB3.1 input is not connected on this board, and > > > > thus the crosspoint switch is never used, nor the SSRX1/SSTX1 pins. > > > > > > > > The existing binding defines the MIPI DSI/DPI input as port0 and two of > > > > the four lanes of output that is probably by default connected to the > > > > "DisplayPort Transmitter" as port1 because that's how the crosspoint > > > > switch comes out of reset. That leaves the USB3.1 input possibly needing > > > > a port in the ports binding, and the other two lanes of output needing a > > > > port in the ports binding to describe their connection to the downstream > > > > device. And finally information about if the crosspoint switch needs to > > > > be registered with the typec framework to do typec things, which can be > > > > achieved by the presence of the 'mode-switch' property. > > > > > > > > On a board like kukui-jacuzzi these new properties and ports wouldn't be > > > > specified, because what is there is already sufficient. If this chip is > > > > connected to a usb-c-connector then I'd expect to see a connection from > > > > the output ports in the graph binding to the connector node's ports. > > > > There aren't any ports in the usb-c-connector binding though from what I > > > > see. > > > > > > > > I believe there's also one more use case here where USB3.1 or MIPI > > > > DSI/DPI is connected on the input side and this device is used to steer > > > > USB3.1 or DP through the crosspoint switch to either of the two output > > > > pairs. This last scenario means that we have to describe both output > > > > pairs, SSRX1/SSTX1 and SSRX2/SSTX2, as different ports in the binding so > > > > they can be connected to different usb-c-connectors if the hardware > > > > engineer wired the output pins that way. > > > > > > > > TL;DR: Can we add 'mode-switch' as an optional property and two more > > > > ports at address 2 and 3 for the USB3.1 input and the SSRX1/SSTX1 pair > > > > respectively to the existing graph part of this binding? > > > > > > Sorry, but I got lost midway through the preceding explanation. > > > > Made sense to me. > > > > > The binding > > > can always add additional ports to each "switch" to accomplish the > > > graph connections > > > you are alluding to (if the driver needs/uses it, which I don't think > > > this one does at present). > > > > Why is the switch special? If I just look at this from a block diagram > > perspective, I just see a list of interfaces that need to be described > > in the graph. > > Because it is specific to Type-C connectors. The anx7625.h does > contain a cross-point > switch which controls data lines coming from 1 (or more) Type-C > connectors, so it seems reasonable > to have a dedicated binding for such types of hardware sub-components, > which helps define the graph connections > in a more uniform manner. That's not to say: > - this can only be used by this hardware. The typec-switch binding is > generic enough to accommodate other hardware. > - there is only 1 way to do this. The interfaces could be described > using existing port OF graph bindings, but I don't > see that as reason enough to not include a dedicated switch binding if > it makes the overall binding more logically organized (IMO) and > makes driver registration code mode clean. > > > > > > Adding extra ports to existing ports gets tricky from a mode-switch > > > enumeration perspective (which > > > ports should have the modes switches, which shouldn't? Do you follow > > > the remote end points for each port > > > and see which one is a Type C connector? > > > > The driver knows which port is which because the binding has to define > > it. So you have to check 2 of them (SSRX1/SSTX1 and SSRX2/SSTX2) to find > > usb C connectors. > > Right, but with the switch binding you no longer need to check. If > there is a typec-switch, you know > it is coming from a Type-C connector, so you can just register the > switches with the Type-C framework. > > > > > > What if we add an > > > intermediate switch device in the future?) > > > Having a dedicated "switch" binding makes this consistent and easy > > > (port0 will always have the end-point for the switch). > > > > > > While there may be more than 1 valid approach here, I believe the > > > current one is appropriate. > > > > To put it simply, if you want to define a generic binding, I want to see > > at least 2 users of it. Pin-Yen and I will work on adding another user for the binding to v5 of this patch series. Best regards, - Prashant
On 21/06/2022 15:17, Greg Kroah-Hartman wrote: > On Wed, Jun 15, 2022 at 11:13:33AM -0700, Prashant Malani wrote: >> I should add: >> >> Series submission suggestions (of course, open to better suggestions too): >> - Patches 1-3 can go through the USB repo. > > I will take patches 1 and 2 now. > > seems the others need reworks or acks from the DT people. I just gave for patch 3 and before for 4, so you can grab these as well. Thanks! Best regards, Krzysztof
On Wed, Jun 22, 2022 at 04:53:40PM +0200, Krzysztof Kozlowski wrote: > On 21/06/2022 15:17, Greg Kroah-Hartman wrote: > > On Wed, Jun 15, 2022 at 11:13:33AM -0700, Prashant Malani wrote: > >> I should add: > >> > >> Series submission suggestions (of course, open to better suggestions too): > >> - Patches 1-3 can go through the USB repo. > > > > I will take patches 1 and 2 now. > > > > seems the others need reworks or acks from the DT people. > > I just gave for patch 3 and before for 4, so you can grab these as well. They are gone from my queue, a resend with that ack would be good so that I can pick it up easier. thanks, gre gk-h
On Wed, Jun 22, 2022 at 8:11 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jun 22, 2022 at 04:53:40PM +0200, Krzysztof Kozlowski wrote: > > On 21/06/2022 15:17, Greg Kroah-Hartman wrote: > > > On Wed, Jun 15, 2022 at 11:13:33AM -0700, Prashant Malani wrote: > > >> I should add: > > >> > > >> Series submission suggestions (of course, open to better suggestions too): > > >> - Patches 1-3 can go through the USB repo. > > > > > > I will take patches 1 and 2 now. > > > > > > seems the others need reworks or acks from the DT people. > > > > I just gave for patch 3 and before for 4, so you can grab these as well. > > They are gone from my queue, a resend with that ack would be good so > that I can pick it up easier. Thanks Greg. I've sent out v5 [1] which has the Reviewed-by tags from Krzysztof. [1] https://lore.kernel.org/linux-usb/20220622173605.1168416-1-pmalani@chromium.org/ Best regards, -Prashant