| Message ID | 20240322-typec-fix-example-v1-1-6b01c347419e@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: usb: qcom,pmic-typec: OF graph corrections | expand |
On 22/03/2024 11:52, Dmitry Baryshkov wrote: > The PMIC Type-C controller doesn't have separate role-switching signal. > Instead it has an HS signal connection between embedded USB-C connector > node and the HS port of the USB controller. I take your point on port as a signal but the way type-c determines data-role is via the DR_Swap message. https://www.embedded.com/usb-type-c-and-power-delivery-101-power-delivery-protocol/ We receive an IRQ which is a packet containing DR_Swap - TCPM consumes that data and does a data-role switch. The port then establishes the link between typec-port and redriver or PHY. So, I think HS should be dropped from the commit logs and names in both series. BTW for the GLINK devices I think the adsp firmware just notifies the APSS of the data-role switch so, these types of devices probably should have an epdoint with "usb_role_switch" in the name. --- bod
On Fri, 22 Mar 2024 at 14:35, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 22/03/2024 11:52, Dmitry Baryshkov wrote: > > The PMIC Type-C controller doesn't have separate role-switching signal. > > Instead it has an HS signal connection between embedded USB-C connector > > node and the HS port of the USB controller. > > I take your point on port as a signal but the way type-c determines > data-role is via the DR_Swap message. > > https://www.embedded.com/usb-type-c-and-power-delivery-101-power-delivery-protocol/ > > We receive an IRQ which is a packet containing DR_Swap - TCPM consumes > that data and does a data-role switch. > > The port then establishes the link between typec-port and redriver or PHY. > > So, I think HS should be dropped from the commit logs and names in both > series. Then the actual usage doesn't match the schema. usb-c-connector clearly defines HS, SS and SBU ports The snps,dwc3.yaml describes ports as ones handling usb-role-switch, but then clearly writes that port@0 is HS and port@1 is SS. As such, I think, the correct name for the ports is to have _hs_ in the name We have pmic-typec/port, separate graph port for role-switching (supported by TCPM code), but we didn't use it at all on our platforms (nor do we need it, as we use the HS port). > > BTW for the GLINK devices I think the adsp firmware just notifies the > APSS of the data-role switch so, these types of devices probably should > have an epdoint with "usb_role_switch" in the name. > > --- > bod >
On 22/03/2024 13:28, Dmitry Baryshkov wrote: > Then the actual usage doesn't match the schema. usb-c-connector > clearly defines HS, SS and SBU ports Its a bit restrictive IMO, data-role and power-role switching is not limited to HS and in fact can be done with a GPIO for example. /Looks in Documentation/devicetree/bindings/connector/usb-connector.yaml Yeah I mean this just doesn't cover all use-cases .. ports: $ref: /schemas/graph.yaml#/properties/ports description: OF graph bindings modeling any data bus to the connector unless the bus is between parent node and the connector. Since a single connector can have multiple data buses every bus has an assigned OF graph port number as described below. properties: port@0: $ref: /schemas/graph.yaml#/properties/port description: High Speed (HS), present in all connectors. port@1: $ref: /schemas/graph.yaml#/properties/port description: Super Speed (SS), present in SS capable connectors. port@2: $ref: /schemas/graph.yaml#/properties/port description: Sideband Use (SBU), present in USB-C. This describes the alternate mode connection of which SBU is a part. TBH I think we should drop this HS, SS stuff from the connector definition - there's nothing to say in a h/w definition anywhere HS must be a port or indeed SS - not all hardware knows or cares about different HS/SS signalling. Documentation bit-rot --- bod
On 22/03/2024 15:52, Bryan O'Donoghue wrote: > On 22/03/2024 13:28, Dmitry Baryshkov wrote: >> Then the actual usage doesn't match the schema. usb-c-connector >> clearly defines HS, SS and SBU ports > > Its a bit restrictive IMO, data-role and power-role switching is not limited to HS and in fact can be done with a GPIO for example. > > /Looks in Documentation/devicetree/bindings/connector/usb-connector.yaml > > Yeah I mean this just doesn't cover all use-cases .. > > ports: > $ref: /schemas/graph.yaml#/properties/ports > description: OF graph bindings modeling any data bus to the connector > unless the bus is between parent node and the connector. Since a single > connector can have multiple data buses every bus has an assigned OF graph > port number as described below. > > properties: > port@0: > $ref: /schemas/graph.yaml#/properties/port > description: High Speed (HS), present in all connectors. > > port@1: > $ref: /schemas/graph.yaml#/properties/port > description: Super Speed (SS), present in SS capable connectors. > > port@2: > $ref: /schemas/graph.yaml#/properties/port > description: Sideband Use (SBU), present in USB-C. This describes the > alternate mode connection of which SBU is a part. > > TBH I think we should drop this HS, SS stuff from the connector definition - there's nothing to say in a h/w definition anywhere HS must be a port or indeed SS - not all hardware knows or cares about different HS/SS signalling. It matches the USB-C connector electrical characteristics, which by spec has, at least: - High-Speed USB Line - up to 4 differential high-speed lanes that can be switched to DP, USB2 or PCIe - SideBand line (SBU) And those 3 components can be handled by 3 different HW in the SoC, so each one has a dedicated port. Remember DT describes the HW, not the SW implementation. Neil > > Documentation bit-rot > > --- > bod >
On 22/03/2024 15:09, neil.armstrong@linaro.org wrote: >> TBH I think we should drop this HS, SS stuff from the connector >> definition - there's nothing to say in a h/w definition anywhere HS >> must be a port or indeed SS - not all hardware knows or cares about >> different HS/SS signalling. > > It matches the USB-C connector electrical characteristics, which by spec > has, at least: > - High-Speed USB Line > - up to 4 differential high-speed lanes that can be switched to DP, USB2 > or PCIe > - SideBand line (SBU) > > And those 3 components can be handled by 3 different HW in the SoC, so > each one has a dedicated port. > > Remember DT describes the HW, not the SW implementation. > > Neil Yes, you're right about that. I suppose 1. Orientation switches should be defined as coming from a port on the connector associated with the CC pins. port@3: orientation-switch port name goes here 2. Data-role switches... Again the CC pins https://community.silabs.com/s/article/what-s-the-role-of-cc-pin-in-type-c-solution?language=en_US Maybe the right-thing-to-do is to add another port for the CC pins - which would still describe the hardware characteristics but would _accurately_ name the thing which does the data-role/orientation switching CC1/CC2 Then we would not be abusing HS/SS/SBU for the port names - we'd be extending the connector definition but also naming the ports/endpoints appropriately associated with the data over the hw --- bod
On Fri, 22 Mar 2024 at 17:23, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 22/03/2024 15:09, neil.armstrong@linaro.org wrote: > >> TBH I think we should drop this HS, SS stuff from the connector > >> definition - there's nothing to say in a h/w definition anywhere HS > >> must be a port or indeed SS - not all hardware knows or cares about > >> different HS/SS signalling. > > > > It matches the USB-C connector electrical characteristics, which by spec > > has, at least: > > - High-Speed USB Line > > - up to 4 differential high-speed lanes that can be switched to DP, USB2 > > or PCIe > > - SideBand line (SBU) > > > > And those 3 components can be handled by 3 different HW in the SoC, so > > each one has a dedicated port. > > > > Remember DT describes the HW, not the SW implementation. > > > > Neil > > Yes, you're right about that. > > I suppose > > 1. Orientation switches should be defined as coming from a port on the > connector associated with the CC pins. > port@3: > orientation-switch port name goes here > > 2. Data-role switches... > Again the CC pins > > https://community.silabs.com/s/article/what-s-the-role-of-cc-pin-in-type-c-solution?language=en_US > > Maybe the right-thing-to-do is to add another port for the CC pins - > which would still describe the hardware characteristics but would > _accurately_ name the thing which does the data-role/orientation switching It's true that we don't describe CC lines. However In most of the cases CC lines are handled by the Type-C port manager directly. So there will be a connection from usb-c-connector to the pmic-typec itself (which looks pretty redundant). The TCPM then sends these events to the interested parties. SS and SBU chains really care only about orientation (to be able to remux SS lanes and SBU polarity). USB data role is only relevant for the USB controller itself. So either we should add special role-switching link from the TCPM to USB-controller, or just reuse the HS link. > > CC1/CC2 > > Then we would not be abusing HS/SS/SBU for the port names - we'd be > extending the connector definition but also naming the ports/endpoints > appropriately associated with the data over the hw
On Fri, Mar 22, 2024 at 05:49:00PM +0200, Dmitry Baryshkov wrote: > On Fri, 22 Mar 2024 at 17:23, Bryan O'Donoghue > <bryan.odonoghue@linaro.org> wrote: > > > > On 22/03/2024 15:09, neil.armstrong@linaro.org wrote: > > >> TBH I think we should drop this HS, SS stuff from the connector > > >> definition - there's nothing to say in a h/w definition anywhere HS > > >> must be a port or indeed SS - not all hardware knows or cares about > > >> different HS/SS signalling. > > > > > > It matches the USB-C connector electrical characteristics, which by spec > > > has, at least: > > > - High-Speed USB Line > > > - up to 4 differential high-speed lanes that can be switched to DP, USB2 > > > or PCIe > > > - SideBand line (SBU) > > > > > > And those 3 components can be handled by 3 different HW in the SoC, so > > > each one has a dedicated port. > > > > > > Remember DT describes the HW, not the SW implementation. > > > > > > Neil > > > > Yes, you're right about that. > > > > I suppose > > > > 1. Orientation switches should be defined as coming from a port on the > > connector associated with the CC pins. > > port@3: > > orientation-switch port name goes here > > > > 2. Data-role switches... > > Again the CC pins > > > > https://community.silabs.com/s/article/what-s-the-role-of-cc-pin-in-type-c-solution?language=en_US > > > > Maybe the right-thing-to-do is to add another port for the CC pins - > > which would still describe the hardware characteristics but would > > _accurately_ name the thing which does the data-role/orientation switching > > It's true that we don't describe CC lines. However In most of the > cases CC lines are handled by the Type-C port manager directly. So > there will be a connection from usb-c-connector to the pmic-typec > itself (which looks pretty redundant). The thought at the time this was designed was that would be the parent node of the connector. That's perhaps too simple. Rob
On Fri, 22 Mar 2024 at 20:36, Rob Herring <robh@kernel.org> wrote: > > On Fri, Mar 22, 2024 at 05:49:00PM +0200, Dmitry Baryshkov wrote: > > On Fri, 22 Mar 2024 at 17:23, Bryan O'Donoghue > > <bryan.odonoghue@linaro.org> wrote: > > > > > > On 22/03/2024 15:09, neil.armstrong@linaro.org wrote: > > > >> TBH I think we should drop this HS, SS stuff from the connector > > > >> definition - there's nothing to say in a h/w definition anywhere HS > > > >> must be a port or indeed SS - not all hardware knows or cares about > > > >> different HS/SS signalling. > > > > > > > > It matches the USB-C connector electrical characteristics, which by spec > > > > has, at least: > > > > - High-Speed USB Line > > > > - up to 4 differential high-speed lanes that can be switched to DP, USB2 > > > > or PCIe > > > > - SideBand line (SBU) > > > > > > > > And those 3 components can be handled by 3 different HW in the SoC, so > > > > each one has a dedicated port. > > > > > > > > Remember DT describes the HW, not the SW implementation. > > > > > > > > Neil > > > > > > Yes, you're right about that. > > > > > > I suppose > > > > > > 1. Orientation switches should be defined as coming from a port on the > > > connector associated with the CC pins. > > > port@3: > > > orientation-switch port name goes here > > > > > > 2. Data-role switches... > > > Again the CC pins > > > > > > https://community.silabs.com/s/article/what-s-the-role-of-cc-pin-in-type-c-solution?language=en_US > > > > > > Maybe the right-thing-to-do is to add another port for the CC pins - > > > which would still describe the hardware characteristics but would > > > _accurately_ name the thing which does the data-role/orientation switching > > > > It's true that we don't describe CC lines. However In most of the > > cases CC lines are handled by the Type-C port manager directly. So > > there will be a connection from usb-c-connector to the pmic-typec > > itself (which looks pretty redundant). > > The thought at the time this was designed was that would be the parent > node of the connector. That's perhaps too simple. Yep. In both our cases the TCPM is a parent of the usb-c-connector.
On 22/03/2024 15:49, Dmitry Baryshkov wrote: > It's true that we don't describe CC lines. However In most of the > cases CC lines are handled by the Type-C port manager directly. So > there will be a connection from usb-c-connector to the pmic-typec > itself (which looks pretty redundant). I think it more logical to associate the role-switch event with the CC lines which actually handle the messaging than the HS PHY which does not to be honest. If we predicate a name change on fixing the namespace then we should fix the namespace instead of reuse existing for expediency. $0.02 --- bod
On Fri, 22 Mar 2024 at 22:44, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 22/03/2024 15:49, Dmitry Baryshkov wrote: > > It's true that we don't describe CC lines. However In most of the > > cases CC lines are handled by the Type-C port manager directly. So > > there will be a connection from usb-c-connector to the pmic-typec > > itself (which looks pretty redundant). > > I think it more logical to associate the role-switch event with the CC > lines which actually handle the messaging than the HS PHY which does not > to be honest. > > If we predicate a name change on fixing the namespace then we should fix > the namespace instead of reuse existing for expediency. It's not an HS PHY. It is an HS host = DWC3. Anyway, CC lines do not go to the DWC3. They go directly to the PMIC.
diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml index d9694570c419..63020a88a355 100644 --- a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml @@ -84,11 +84,6 @@ properties: vdd-pdphy-supply: description: VDD regulator supply to the PDPHY. - port: - $ref: /schemas/graph.yaml#/properties/port - description: - Contains a port which produces data-role switching messages. - required: - compatible - reg
The PMIC Type-C controller doesn't have separate role-switching signal. Instead it has an HS signal connection between embedded USB-C connector node and the HS port of the USB controller. Fixes: 00bb478b829e ("dt-bindings: usb: Add Qualcomm PMIC Type-C") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml | 5 ----- 1 file changed, 5 deletions(-)