[PATCH 0/2] dt-bindings: usb: qcom,pmic-typec: OF graph corrections

[PATCH 0/2] dt-bindings: usb: qcom,pmic-typec: OF graph corrections

[Dmitry Baryshkov]

Drop the extra port definition: it is not used by the DT files and
there is no correponding physical signal.
Update examples to follow usb-c-connector schema wrt. ports definitions.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (2):
      dt-bindings: usb: qcom,pmic-typec: drop port description
      dt-bindings: usb: qcom,pmic-typec: update example to follow connector schema

 .../devicetree/bindings/usb/qcom,pmic-typec.yaml   | 39 ++++++++++++++--------
 1 file changed, 26 insertions(+), 13 deletions(-)
---
base-commit: 226d3c72fcde130a99d760895ebdd20e78e02cb5
change-id: 20240322-typec-fix-example-3d9b1eca853d

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Dmitry Baryshkov]

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(-)

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

-- 
2.39.2

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Bryan O'Donoghue]

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

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Dmitry Baryshkov]

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
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Bryan O'Donoghue]

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

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[neil.armstrong]

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
> 

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Bryan O'Donoghue]

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

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Rob Herring]

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

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Dmitry Baryshkov]

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.


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Bryan O'Donoghue]

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

Re: [PATCH 1/2] dt-bindings: usb: qcom,pmic-typec: drop port description

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

[PATCH 2/2] dt-bindings: usb: qcom,pmic-typec: update example to follow connector schema

[Dmitry Baryshkov]

Update Qualcomm PMIC Type-C examples to follow the USB-C connector
schema. The USB-C connector should have three ports (USB HS @0,
SSTX/RX @1 and SBU @2 lanes). Reorder ports accordingly and add SBU port
connected to the SBU mux (e.g. FSA4480).

Fixes: 00bb478b829e ("dt-bindings: usb: Add Qualcomm PMIC Type-C")
Reported-by: Luca Weiss <luca.weiss@fairphone.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../devicetree/bindings/usb/qcom,pmic-typec.yaml   | 34 +++++++++++++++++-----
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
index 63020a88a355..beecbda40e3c 100644
--- a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
@@ -187,15 +187,22 @@ examples:
 
                     port@0 {
                         reg = <0>;
-                        pmic_typec_mux_out: endpoint {
-                            remote-endpoint = <&usb_phy_typec_mux_in>;
+                        pmic_typec_hs_in: endpoint {
+                            remote-endpoint = <&usb_hs_out>;
                         };
                     };
 
                     port@1 {
                         reg = <1>;
-                        pmic_typec_role_switch_out: endpoint {
-                            remote-endpoint = <&usb_role_switch_in>;
+                        pmic_typec_ss_in: endpoint {
+                            remote-endpoint = <&usb_phy_typec_ss_out>;
+                        };
+                    };
+
+                    port@2 {
+                        reg = <2>;
+                        pmic_typec_sbu: endpoint {
+                            remote-endpoint = <&usb_mux_sbu>;
                         };
                     };
                 };
@@ -207,8 +214,8 @@ examples:
         dr_mode = "otg";
         usb-role-switch;
         port {
-            usb_role_switch_in: endpoint {
-                remote-endpoint = <&pmic_typec_role_switch_out>;
+            usb_hs_out: endpoint {
+                remote-endpoint = <&pmic_typec_hs_in>;
             };
         };
     };
@@ -216,8 +223,19 @@ examples:
     usb-phy {
         orientation-switch;
         port {
-            usb_phy_typec_mux_in: endpoint {
-                remote-endpoint = <&pmic_typec_mux_out>;
+            usb_phy_typec_ss_out: endpoint {
+                remote-endpoint = <&pmic_typec_ss_in>;
+            };
+        };
+    };
+
+    usb-mux {
+        orientation-switch;
+        mode-switch;
+
+        port {
+            usb_mux_sbu: endpoint {
+                remote-endpoint = <&pmic_typec_sbu>;
             };
         };
     };

-- 
2.39.2

Re: [PATCH 2/2] dt-bindings: usb: qcom,pmic-typec: update example to follow connector schema

[Konrad Dybcio]

On 22.03.2024 12:52, Dmitry Baryshkov wrote:
> Update Qualcomm PMIC Type-C examples to follow the USB-C connector
> schema. The USB-C connector should have three ports (USB HS @0,
> SSTX/RX @1 and SBU @2 lanes). Reorder ports accordingly and add SBU port
> connected to the SBU mux (e.g. FSA4480).
> 
> Fixes: 00bb478b829e ("dt-bindings: usb: Add Qualcomm PMIC Type-C")
> Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad