diff mbox series

[v3,2/6] dt-bindings: usb: Add Qualcomm PMIC type C controller dt-binding

Message ID 20200617180209.5636-3-wcheng@codeaurora.org (mailing list archive)
State Superseded, archived
Headers show
Series Introduce PMIC based USB type C detection | expand

Commit Message

Wesley Cheng June 17, 2020, 6:02 p.m. UTC 
Introduce the dt-binding for enabling USB type C orientation and role
detection using the PM8150B.  The driver will be responsible for receiving
the interrupt at a state change on the CC lines, reading the orientation/role,
and communicating this information to the remote clients, which can include
a role switch node and a type C switch.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 .../bindings/usb/qcom,pmic-typec.yaml         | 117 ++++++++++++++++++
 1 file changed, 117 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml

Comments

Rob Herring June 18, 2020, 6:33 p.m. UTC | #1 
On Wed, Jun 17, 2020 at 12:02 PM Wesley Cheng <wcheng@codeaurora.org> wrote:
>
> Introduce the dt-binding for enabling USB type C orientation and role
> detection using the PM8150B.  The driver will be responsible for receiving
> the interrupt at a state change on the CC lines, reading the orientation/role,
> and communicating this information to the remote clients, which can include
> a role switch node and a type C switch.
>
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> ---
>  .../bindings/usb/qcom,pmic-typec.yaml         | 117 ++++++++++++++++++
>  1 file changed, 117 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
> new file mode 100644
> index 000000000000..085b4547d75a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/usb/qcom,pmic-typec.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Qualcomm PMIC based USB type C Detection Driver
> +
> +maintainers:
> +  - Wesley Cheng <wcheng@codeaurora.org>
> +
> +description: |
> +  Qualcomm PMIC Type C Detect
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,pm8150b-usb-typec
> +
> +  reg:
> +    maxItems: 1
> +    description: Type C base address
> +
> +  interrupts:
> +    maxItems: 1
> +    description: CC change interrupt from PMIC
> +
> +  connector:
> +    description: Connector type for remote endpoints
> +    type: object

You are duplicating everything in usb-connector.yaml. You should have
a $ref to it.

> +
> +    properties:
> +      compatible:
> +        enum:
> +          - usb-c-connector
> +
> +      power-role:
> +       enum:
> +         - dual
> +         - source
> +         - sink
> +
> +      data-role:
> +        enum:
> +          - dual
> +          - host
> +          - device
> +
> +      port:
> +        description: Remote endpoint connections
> +        type: object
> +
> +        properties:
> +          endpoint@0:
> +            description: Connection to USB type C mux node

This is wrong. The connector binding says port 0 is the connection the
USB HS controller.

What's a type C mux node? Is there a binding for that? There's an
ongoing discussion with the CrOS folks on how to describe Alt mode
mux/switches.

> +            type: object
> +
> +            properties:
> +              remote-endpoint:
> +                maxItems: 1
> +                description: Node reference to the type C mux
> +
> +          endpoint@1:
> +            description: Connection to role switch node

Again, what's this?

> +            type: object
> +
> +            properties:
> +              remote-endpoint:
> +                maxItems: 1
> +                description: Node reference to the role switch node
> +
> +    required:
> +      - compatible
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - connector
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    pm8150b {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        qcom,typec@1500 {
> +            compatible = "qcom,pm8150b-usb-typec";
> +            reg = <0x1500>;
> +            interrupts =
> +                <0x2 0x15 0x5 IRQ_TYPE_EDGE_RISING>;
> +
> +            connector {
> +                compatible = "usb-c-connector";
> +                power-role = "dual";
> +                data-role = "dual";
> +                port {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                    usb3_data_ss: endpoint@0 {
> +                        reg = <0>;
> +                        remote-endpoint =
> +                                <&qmp_ss_mux>;
> +                    };
> +
> +                    usb3_role: endpoint@1 {
> +
> +                        reg = <1>;
> +                        remote-endpoint =
> +                                <&dwc3_drd_switch>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> +...
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Wesley Cheng June 18, 2020, 8:09 p.m. UTC | #2 
On 6/18/2020 11:33 AM, Rob Herring wrote:
> On Wed, Jun 17, 2020 at 12:02 PM Wesley Cheng <wcheng@codeaurora.org> wrote:
> 
> You are duplicating everything in usb-connector.yaml. You should have
> a $ref to it.
> 

Hi Rob,

Sure, I will add a reference to that doc.

> 
> This is wrong. The connector binding says port 0 is the connection the
> USB HS controller.
> 
> What's a type C mux node? Is there a binding for that? There's an
> ongoing discussion with the CrOS folks on how to describe Alt mode
> mux/switches.

I reviewed the connector binding previously, and couldn't seem to come
up with a model which fit a design where the type C controller (ie the
entity which does the CC orientation and role detection) does not have
the SS lane mux included.  The SS lane mux is the HW which handles the
selection of the SS lanes to utilize based on cable orientation.

I looked at the FUSB302 TCPM driver, which doesn't have an integrated SS
lane mux, and relies on an external FUSB340 switch to handle the
polarity, but seems that in the fusb302.c driver it doesn't implement
the polarity handler.  They might possibly have a HW output signal which
controls the mux directly, but I'm not sure on that.

Anyway, if someone wanted to take a SW approach and program an external
mux, this model doesn't seem to allow that.  This is somewhat unrelated
to the DP AUX mode switching, as that probably will only come into the
picture after the policy engine has detected there is a DP adapter
connected, whereas this is applicable to non DP/alt mode situations.

Thanks!

>> +            type: object
>> +
>> +            properties:
>> +              remote-endpoint:
>> +                maxItems: 1
>> +                description: Node reference to the type C mux
>> +
>> +          endpoint@1:
>> +            description: Connection to role switch node
> 
> Again, what's this?
>
Rob Herring June 18, 2020, 10:23 p.m. UTC | #3 
On Thu, Jun 18, 2020 at 2:09 PM Wesley Cheng <wcheng@codeaurora.org> wrote:
>
>
> On 6/18/2020 11:33 AM, Rob Herring wrote:
> > On Wed, Jun 17, 2020 at 12:02 PM Wesley Cheng <wcheng@codeaurora.org> wrote:
> >
> > You are duplicating everything in usb-connector.yaml. You should have
> > a $ref to it.
> >
>
> Hi Rob,
>
> Sure, I will add a reference to that doc.
>
> >
> > This is wrong. The connector binding says port 0 is the connection the
> > USB HS controller.
> >
> > What's a type C mux node? Is there a binding for that? There's an
> > ongoing discussion with the CrOS folks on how to describe Alt mode
> > mux/switches.
>
> I reviewed the connector binding previously, and couldn't seem to come
> up with a model which fit a design where the type C controller (ie the
> entity which does the CC orientation and role detection) does not have
> the SS lane mux included.  The SS lane mux is the HW which handles the
> selection of the SS lanes to utilize based on cable orientation.

The intent was the controller would be the parent node of the connector.

How the SS lane mux is represented is what needs to be figured out. I
don't know what that looks like, but it needs to be something that
works for multiple designs. Ideally, that's an extension of the
existing 'usb-c-connector' binding, but if there's good reasons to
redesign it that can happen.

Rob
Wesley Cheng June 22, 2020, 5:47 p.m. UTC | #4 
On 6/18/2020 3:23 PM, Rob Herring wrote:
> On Thu, Jun 18, 2020 at 2:09 PM Wesley Cheng <wcheng@codeaurora.org> wrote:
>>
>>
>> On 6/18/2020 11:33 AM, Rob Herring wrote:
>>> On Wed, Jun 17, 2020 at 12:02 PM Wesley Cheng <wcheng@codeaurora.org> wrote:
>>>
>>> You are duplicating everything in usb-connector.yaml. You should have
>>> a $ref to it.
>>>
>>
>> Hi Rob,
>>
>> Sure, I will add a reference to that doc.
>>
>>>
>>> This is wrong. The connector binding says port 0 is the connection the
>>> USB HS controller.
>>>
>>> What's a type C mux node? Is there a binding for that? There's an
>>> ongoing discussion with the CrOS folks on how to describe Alt mode
>>> mux/switches.
>>
>> I reviewed the connector binding previously, and couldn't seem to come
>> up with a model which fit a design where the type C controller (ie the
>> entity which does the CC orientation and role detection) does not have
>> the SS lane mux included.  The SS lane mux is the HW which handles the
>> selection of the SS lanes to utilize based on cable orientation.
> 
> The intent was the controller would be the parent node of the connector.
> 

Hi Rob,

Correct, I agree with that point, and in the changes uploaded, the QCOM
PMIC type C controller will be the parent node for the connector.

> How the SS lane mux is represented is what needs to be figured out. I
> don't know what that looks like, but it needs to be something that
> works for multiple designs. Ideally, that's an extension of the
> existing 'usb-c-connector' binding, but if there's good reasons to
> redesign it that can happen.
> 
> Rob
> 

We probably wouldn't need to redesign it, but maybe if we can remove the
connector port assignments requirement, it would allow for some
flexibility.  From my knowledge, I don't think any driver is actually
utilizing or checking the port number assignments, so there isn't a
limitation on what could be defined in there.

Here's a simplified diagram of the FUSB302 reference design from the
data sheet.  The I2C bus is just for CSR access to the FUSB302.

				   _______		 _______
                            ______|FUSB302|		|SOC	|
			   |	  |Type C |		|	|
			   |      |Cntrl  |__I2C_______	|	|
			   |	  |_______|		|	|
 ___                       |       			|	|
|   |______ CC1/2 _________|				|	|
|   |______ HS DP/DM __________________________________	|	|
|   |							|	|
				   ________		|	|
|   |______ SS RX/TX1 ____________|FUSB304 |__SS RX/TX_	|	|
|   |______ SS RX/TX2 ____________|USB Mux |		|_______|
|   |                             |________|
|   |
|___|


Otherwise, we can just simply add another port definition for external
SS lane muxes if possible.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
new file mode 100644
index 000000000000..085b4547d75a
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
@@ -0,0 +1,117 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/qcom,pmic-typec.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Qualcomm PMIC based USB type C Detection Driver
+
+maintainers:
+  - Wesley Cheng <wcheng@codeaurora.org>
+
+description: |
+  Qualcomm PMIC Type C Detect
+
+properties:
+  compatible:
+    enum:
+      - qcom,pm8150b-usb-typec
+
+  reg:
+    maxItems: 1
+    description: Type C base address
+
+  interrupts:
+    maxItems: 1
+    description: CC change interrupt from PMIC
+
+  connector:
+    description: Connector type for remote endpoints
+    type: object
+
+    properties:
+      compatible:
+        enum:
+          - usb-c-connector
+
+      power-role:
+       enum:
+         - dual
+         - source
+         - sink
+
+      data-role:
+        enum:
+          - dual
+          - host
+          - device
+
+      port:
+        description: Remote endpoint connections
+        type: object
+
+        properties:
+          endpoint@0:
+            description: Connection to USB type C mux node
+            type: object
+
+            properties:
+              remote-endpoint:
+                maxItems: 1
+                description: Node reference to the type C mux
+
+          endpoint@1:
+            description: Connection to role switch node
+            type: object
+
+            properties:
+              remote-endpoint:
+                maxItems: 1
+                description: Node reference to the role switch node
+
+    required:
+      - compatible
+
+required:
+  - compatible
+  - interrupts
+  - connector
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    pm8150b {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        qcom,typec@1500 {
+            compatible = "qcom,pm8150b-usb-typec";
+            reg = <0x1500>;
+            interrupts =
+                <0x2 0x15 0x5 IRQ_TYPE_EDGE_RISING>;
+
+            connector {
+                compatible = "usb-c-connector";
+                power-role = "dual";
+                data-role = "dual";
+                port {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+                    usb3_data_ss: endpoint@0 {
+                        reg = <0>;
+                        remote-endpoint =
+                                <&qmp_ss_mux>;
+                    };
+
+                    usb3_role: endpoint@1 {
+
+                        reg = <1>;
+                        remote-endpoint =
+                                <&dwc3_drd_switch>;
+                    };
+                };
+            };
+        };
+    };
+...