[v9,2/2] dt-bindings: msm/dp: Add bindings of MSM DisplayPort controller

Commit Message

Krishna Manikandan Dec. 11, 2020, 7:09 a.m. UTC

Changes in v10:
- Change title of this patch as it does not contain PLL bindings anymore
- Remove redundant properties
- Remove use of IRQ flag
- Fix ports property

Previous Change log:
https://lkml.kernel.org/lkml/[e-mail]/

Signed-off-by: Chandan Uddaraju <[e-mail]>
Signed-off-by: Vara Reddy <[e-mail]>
Signed-off-by: Tanmay Shah <[e-mail]>
---
 .../bindings/display/msm/dp-controller.yaml        | 120 +++++++++++++++++++++
 .../bindings/display/msm/dpu-sc7180.yaml           |  10 ++
 2 files changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/msm/dp-controller.yaml

Comments

Stephen Boyd Dec. 11, 2020, 10:36 p.m. UTC | #1

Quoting Krishna Manikandan (2020-12-10 23:09:45)
> Changes in v10:
> - Change title of this patch as it does not contain PLL bindings anymore
> - Remove redundant properties
> - Remove use of IRQ flag
> - Fix ports property
> 
> Previous Change log:
> https://lkml.kernel.org/lkml/[e-mail]/

Why can't that be put in here instead of making me click the link?

> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> new file mode 100644
> index 0000000..bc80632
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)

Why two spaces between 'only and 'OR'?

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MSM Display Port Controller.

Drop the full-stop?

> +
> +maintainers:
> +  - Chandan Uddaraju <[e-mail]>
> +  - Vara Reddy <[e-mail]>
> +  - Tanmay Shah <[e-mail]>
> +
> +description: |
> +  Device tree bindings for DisplayPort host controller for MSM targets
> +  that are compatible with VESA DisplayPort interface specification.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sc7180-dp
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: AHB clock to enable register access
> +      - description: Display Port AUX clock
> +      - description: Display Port Link clock
> +      - description: Link interface clock between DP and PHY
> +      - description: Display Port Pixel clock
> +
> +  clock-names:
> +    items:
> +      - const: core_iface
> +      - const: core_aux
> +      - const: ctrl_link
> +      - const: ctrl_link_iface
> +      - const: stream_pixel

There should be a dp-phy too, so 'phy' and 'phy-names' property. I see a
'power-domains' property downstream, so please add that too. And also
'#sound-dai-cells".

> +
> +  ports:
> +    type: object
> +    description: |
> +      A ports node with endpoint definitions as defined in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt.
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      port@0:
> +        type: object
> +        description: Input endpoint of the controller.
> +
> +      port@1:
> +        type: object
> +        description: Output endpoint of the controller.

This should have a required section too, and state that all the above
properties are required.

> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
> +
> +    mdss: mdss {

Can drop the label. Also this node name is bad! Should be
'subsystem@<reg>' I guess? And then the reg can just be the first
address in a reg property that is remapped for the children nodes.

> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      interrupt-controller;
> +
> +      msm_dp: displayport-controller@ae90000 {

Can drop the label.

> +        compatible = "qcom,sc7180-dp";
> +        reg = <0 0xae90000 0 0x1400>;
> +        interrupt-parent = <&mdss>;
> +        interrupts = <12>;
> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +                 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
> +                 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
> +                 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
> +        clock-names = "core_iface", "core_aux",
> +                      "ctrl_link",
> +                      "ctrl_link_iface", "stream_pixel";
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                dp_in: endpoint {

Can drop the label.

> +                    remote-endpoint = <&dpu_intf0_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                dp_out: endpoint {

Can drop the label and show some remote-endpoint connection?

> +                };
> +            };
> +        };
> +      };
> +    };
> +...

Tanmay Shah Dec. 17, 2020, 10:52 p.m. UTC | #2

On 2020-12-11 14:36, Stephen Boyd wrote:
> Quoting Krishna Manikandan (2020-12-10 23:09:45)
>> Changes in v10:
>> - Change title of this patch as it does not contain PLL bindings 
>> anymore
>> - Remove redundant properties
>> - Remove use of IRQ flag
>> - Fix ports property
>> 
>> Previous Change log:
>> https://lkml.kernel.org/lkml/[e-mail]/
> 
> Why can't that be put in here instead of making me click the link?
> 

Thanks Stephen for reviews.

I just wanted to use space efficiently. But if it helps in reviews,
I will bring all the previous change logs back.

>> diff --git
>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> new file mode 100644
>> index 0000000..bc80632
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -0,0 +1,120 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
> 
> Why two spaces between 'only and 'OR'?
> 
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/msm/dp-controller.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MSM Display Port Controller.
> 
> Drop the full-stop?
> 
>> +
>> +maintainers:
>> +  - Chandan Uddaraju <[e-mail]>
>> +  - Vara Reddy <[e-mail]>
>> +  - Tanmay Shah <[e-mail]>
>> +
>> +description: |
>> +  Device tree bindings for DisplayPort host controller for MSM 
>> targets
>> +  that are compatible with VESA DisplayPort interface specification.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,sc7180-dp
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: AHB clock to enable register access
>> +      - description: Display Port AUX clock
>> +      - description: Display Port Link clock
>> +      - description: Link interface clock between DP and PHY
>> +      - description: Display Port Pixel clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: core_iface
>> +      - const: core_aux
>> +      - const: ctrl_link
>> +      - const: ctrl_link_iface
>> +      - const: stream_pixel
> 
> There should be a dp-phy too, so 'phy' and 'phy-names' property. I see 
> a
> 'power-domains' property downstream, so please add that too. And also
> '#sound-dai-cells".
> 
>> +
>> +  ports:
>> +    type: object
>> +    description: |
>> +      A ports node with endpoint definitions as defined in
>> +      Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +      port@0:
>> +        type: object
>> +        description: Input endpoint of the controller.
>> +
>> +      port@1:
>> +        type: object
>> +        description: Output endpoint of the controller.
> 
> This should have a required section too, and state that all the above
> properties are required.
> 
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
>> +
>> +    mdss: mdss {
> 
> Can drop the label. Also this node name is bad! Should be
> 'subsystem@<reg>' I guess? And then the reg can just be the first
> address in a reg property that is remapped for the children nodes.
> 

I didn't get this part:
"And then the reg can just be the first address in a reg property that 
is remapped for children nodes."

Does this mean, I should add reg property for mdss node as well? Node 
mdss address is 0x0ae00000. So something like:

reg = <0 0x0ae00000 0 0x1000>;



>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +      interrupt-controller;
>> +
>> +      msm_dp: displayport-controller@ae90000 {
> 
> Can drop the label.
> 
>> +        compatible = "qcom,sc7180-dp";
>> +        reg = <0 0xae90000 0 0x1400>;
>> +        interrupt-parent = <&mdss>;
>> +        interrupts = <12>;
>> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
>> +        clock-names = "core_iface", "core_aux",
>> +                      "ctrl_link",
>> +                      "ctrl_link_iface", "stream_pixel";
>> +
>> +        ports {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            port@0 {
>> +                reg = <0>;
>> +                dp_in: endpoint {
> 
> Can drop the label.
> 
>> +                    remote-endpoint = <&dpu_intf0_out>;
>> +                };
>> +            };
>> +
>> +            port@1 {
>> +                reg = <1>;
>> +                dp_out: endpoint {
> 
> Can drop the label and show some remote-endpoint connection?
> 

I am not sure what should I include here as connection. IIRC, Robh 
suggested It should connect to the Type-C connector through some sort of 
mixing node. However, I see only typec connector node at: 
Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml. 
Should we use that directly or should we use ec node?

Can we do something like:

remote-endpoint = <&typec> or <&ec>

Rest of the comments looks good and I will address them in next patch.

Thanks.

>> +                };
>> +            };
>> +        };
>> +      };
>> +    };
>> +...

Stephen Boyd Dec. 18, 2020, 2:58 a.m. UTC | #3

Quoting Tanmay Shah (2020-12-17 14:52:54)
> On 2020-12-11 14:36, Stephen Boyd wrote:
> > Quoting Krishna Manikandan (2020-12-10 23:09:45)
> >> Changes in v10:
> >> - Change title of this patch as it does not contain PLL bindings 
> >> anymore
> >> - Remove redundant properties
> >> - Remove use of IRQ flag
> >> - Fix ports property
> >> 
> >> Previous Change log:
> >> https://lkml.kernel.org/lkml/[e-mail]/
> > 
> > Why can't that be put in here instead of making me click the link?
> > 
> 
> Thanks Stephen for reviews.
> 
> I just wanted to use space efficiently. But if it helps in reviews,
> I will bring all the previous change logs back.

Or it can be left under the cut (---). This is probably going to merge
through the binding tree so it doesn't follow drm practice of putting
version history in the commit text.

> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> >> new file mode 100644
> >> index 0000000..bc80632
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
[...]
> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +    #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
> >> +
> >> +    mdss: mdss {
> > 
> > Can drop the label. Also this node name is bad! Should be
> > 'subsystem@<reg>' I guess? And then the reg can just be the first
> > address in a reg property that is remapped for the children nodes.
> > 
> 
> I didn't get this part:
> "And then the reg can just be the first address in a reg property that 
> is remapped for children nodes."
> 
> Does this mean, I should add reg property for mdss node as well? Node 
> mdss address is 0x0ae00000. So something like:
> 
> reg = <0 0x0ae00000 0 0x1000>;
> 

Yes and then a ranges property so that child nodes can figure out their
reg property appropriately.

> 
> 
> >> +      #address-cells = <2>;
> >> +      #size-cells = <2>;
> >> +      interrupt-controller;
[...]
> >> +                    remote-endpoint = <&dpu_intf0_out>;
> >> +                };
> >> +            };
> >> +
> >> +            port@1 {
> >> +                reg = <1>;
> >> +                dp_out: endpoint {
> > 
> > Can drop the label and show some remote-endpoint connection?
> > 
> 
> I am not sure what should I include here as connection. IIRC, Robh 
> suggested It should connect to the Type-C connector through some sort of 
> mixing node. However, I see only typec connector node at: 
> Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml. 
> Should we use that directly or should we use ec node?
> 
> Can we do something like:
> 
> remote-endpoint = <&typec> or <&ec>

Sure. I suppose <&typec> would work.

Patch

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
new file mode 100644
index 0000000..bc80632
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -0,0 +1,120 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dp-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MSM Display Port Controller.
+
+maintainers:
+  - Chandan Uddaraju <[e-mail]>
+  - Vara Reddy <[e-mail]>
+  - Tanmay Shah <[e-mail]>
+
+description: |
+  Device tree bindings for DisplayPort host controller for MSM targets
+  that are compatible with VESA DisplayPort interface specification.
+
+properties:
+  compatible:
+    enum:
+      - qcom,sc7180-dp
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: AHB clock to enable register access
+      - description: Display Port AUX clock
+      - description: Display Port Link clock
+      - description: Link interface clock between DP and PHY
+      - description: Display Port Pixel clock
+
+  clock-names:
+    items:
+      - const: core_iface
+      - const: core_aux
+      - const: ctrl_link
+      - const: ctrl_link_iface
+      - const: stream_pixel
+
+  ports:
+    type: object
+    description: |
+      A ports node with endpoint definitions as defined in
+      Documentation/devicetree/bindings/media/video-interfaces.txt.
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+      port@0:
+        type: object
+        description: Input endpoint of the controller.
+
+      port@1:
+        type: object
+        description: Output endpoint of the controller.
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
+
+    mdss: mdss {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      interrupt-controller;
+
+      msm_dp: displayport-controller@ae90000 {
+        compatible = "qcom,sc7180-dp";
+        reg = <0 0xae90000 0 0x1400>;
+        interrupt-parent = <&mdss>;
+        interrupts = <12>;
+        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
+                 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
+                 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
+                 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
+                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
+        clock-names = "core_iface", "core_aux",
+                      "ctrl_link",
+                      "ctrl_link_iface", "stream_pixel";
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                dp_in: endpoint {
+                    remote-endpoint = <&dpu_intf0_out>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                dp_out: endpoint {
+                };
+            };
+        };
+      };
+    };
+...
diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml
index 8397ea8..9549d8f 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml
@@ -132,6 +132,9 @@  patternProperties:
           port@1:
             type: object
             description: DPU_INTF2 (DSI2)
+          port@2:
+            type: object
+            description: DPU_INTF0 (DP)
 
       assigned-clocks:
         maxItems: 4
@@ -228,6 +231,13 @@  examples:
                                                   remote-endpoint = <&dsi0_in>;
                                    };
                            };
+
+                            port@2 {
+                                    reg = <2>;
+                                    dpu_intf0_out: endpoint {
+                                                   remote-endpoint = <&dp_in>;
+                                    };
+                            };
                    };
          };
       };