All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-09  3:38 ` Tanmay Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Tanmay Shah @ 2020-06-09  3:38 UTC (permalink / raw)
  To: devicetree
  Cc: sam, swboyd, seanpaul, freedreno, linux-arm-msm, chandanu,
	robdclark, abhinavk, nganji, dri-devel, linux-clk, Vara Reddy,
	Tanmay Shah

From: Chandan Uddaraju <chandanu@codeaurora.org>

Add bindings for Snapdragon DisplayPort controller driver.

Changes in V2:
Provide details about sel-gpio

Changes in V4:
Provide details about max dp lanes
Change the commit text

Changes in V5:
moved dp.txt to yaml file

Changes in v6:
- Squash all AUX LUT properties into one pattern Property
- Make aux-cfg[0-9]-settings properties optional
- Remove PLL/PHY bindings from DP controller dts
- Add DP clocks description
- Remove _clk suffix from clock names
- Rename pixel clock to stream_pixel
- Remove redundant bindings (GPIO, PHY, HDCP clock, etc..)
- Fix indentation
- Add Display Port as interface of DPU in DPU bindings
  and add port mapping accordingly.

Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
Signed-off-by: Vara Reddy <varar@codeaurora.org>
Signed-off-by: Tanmay Shah <tanmay@codeaurora.org>
---
 .../devicetree/bindings/display/msm/dp-sc7180.yaml | 142 +++++++++++++++++++++
 .../devicetree/bindings/display/msm/dpu.txt        |   8 ++
 2 files changed, 150 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
new file mode 100644
index 0000000..5fdb915
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display Port Controller.
+
+maintainers:
+  - Chandan Uddaraju <chandanu@codeaurora.org>
+  - Vara Reddy <varar@codeaurora.org>
+  - Tanmay Shah <tanmay@codeaurora.org>
+
+description: |
+  Device tree bindings for MSM Display Port which supports DP host controllers
+  that are compatible with VESA Display Port interface specification.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,dp-display
+
+  cell-index:
+    description: Specifies the controller instance.
+
+  reg:
+    items:
+      - description: DP controller registers
+
+  interrupts:
+    description: The interrupt signal from the DP block.
+
+  clocks:
+    description: List of clock specifiers for clocks needed by the device.
+    items:
+      - description: Display Port AUX clock
+      - description: Display Port Link clock
+      - description: Link interface clock between DP and PHY
+      - description: Display Port Pixel clock
+      - description: Root clock generator for pixel clock
+
+  clock-names:
+    description: |
+      Device clock names in the same order as mentioned in clocks property.
+      The required clocks are mentioned below.
+    items:
+      - const: core_aux
+      - const: ctrl_link
+      - const: ctrl_link_iface
+      - const: stream_pixel
+      - const: pixel_rcg
+  "#clock-cells":
+    const: 1
+
+  vdda-1p2-supply:
+    description: phandle to vdda 1.2V regulator node.
+
+  vdda-0p9-supply:
+    description: phandle to vdda 0.9V regulator node.
+
+  data-lanes = <0 1>:
+    type: object
+    description: Maximum number of lanes that can be used for Display port.
+
+  ports:
+    description: |
+       Contains display port controller endpoint subnode.
+       remote-endpoint: |
+         For port@0, set to phandle of the connected panel/bridge's
+         input endpoint. For port@1, set to the DPU interface output.
+         Documentation/devicetree/bindings/graph.txt and
+         Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+patternProperties:
+  "^aux-cfg([0-9])-settings$":
+    type: object
+    description: |
+      Specifies the DP AUX configuration [0-9] settings.
+      The first entry in this array corresponds to the register offset
+      within DP AUX, while the remaining entries indicate the
+      programmable values.
+
+required:
+  - compatible
+  - cell-index
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - vdda-1p2-supply
+  - vdda-0p9-supply
+  - data-lanes
+  - ports
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    msm_dp: displayport-controller@ae90000{
+        compatible = "qcom,dp-display";
+        cell-index = <0>;
+        reg = <0 0xae90000 0 0x1400>;
+        reg-names = "dp_controller";
+
+        interrupt-parent = <&display_subsystem>;
+        interrupts = <12 0>;
+
+        clocks = <&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>,
+                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
+        clock-names = "core_aux",
+                      "ctrl_link",
+                      "ctrl_link_iface", "stream_pixel",
+                      "pixel_rcg";
+        #clock-cells = <1>;
+
+        vdda-1p2-supply = <&vreg_l3c_1p2>;
+        vdda-0p9-supply = <&vreg_l4a_0p8>;
+
+        data-lanes = <0 1>;
+
+        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.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
index 551ae26..30c8ab4 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
@@ -65,6 +65,7 @@ Required properties:
 
 	Port 0 -> DPU_INTF1 (DSI1)
 	Port 1 -> DPU_INTF2 (DSI2)
+	Port 2 -> DPU_INTF0 (DP)
 
 Optional properties:
 - assigned-clocks: list of clock specifiers for clocks needing rate assignment
@@ -136,6 +137,13 @@ Example:
 						remote-endpoint = <&dsi1_in>;
 					};
 				};
+
+				port@2 {
+					reg = <2>;
+					dpu_intf0_out: endpoint {
+						remote-endpoint = <&dp_in>;
+					};
+				};
 			};
 		};
 	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-09  3:38 ` Tanmay Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Tanmay Shah @ 2020-06-09  3:38 UTC (permalink / raw)
  To: devicetree
  Cc: Tanmay Shah, sam, linux-arm-msm, abhinavk, swboyd, seanpaul,
	dri-devel, Vara Reddy, freedreno, linux-clk, chandanu

From: Chandan Uddaraju <chandanu@codeaurora.org>

Add bindings for Snapdragon DisplayPort controller driver.

Changes in V2:
Provide details about sel-gpio

Changes in V4:
Provide details about max dp lanes
Change the commit text

Changes in V5:
moved dp.txt to yaml file

Changes in v6:
- Squash all AUX LUT properties into one pattern Property
- Make aux-cfg[0-9]-settings properties optional
- Remove PLL/PHY bindings from DP controller dts
- Add DP clocks description
- Remove _clk suffix from clock names
- Rename pixel clock to stream_pixel
- Remove redundant bindings (GPIO, PHY, HDCP clock, etc..)
- Fix indentation
- Add Display Port as interface of DPU in DPU bindings
  and add port mapping accordingly.

Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
Signed-off-by: Vara Reddy <varar@codeaurora.org>
Signed-off-by: Tanmay Shah <tanmay@codeaurora.org>
---
 .../devicetree/bindings/display/msm/dp-sc7180.yaml | 142 +++++++++++++++++++++
 .../devicetree/bindings/display/msm/dpu.txt        |   8 ++
 2 files changed, 150 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
new file mode 100644
index 0000000..5fdb915
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display Port Controller.
+
+maintainers:
+  - Chandan Uddaraju <chandanu@codeaurora.org>
+  - Vara Reddy <varar@codeaurora.org>
+  - Tanmay Shah <tanmay@codeaurora.org>
+
+description: |
+  Device tree bindings for MSM Display Port which supports DP host controllers
+  that are compatible with VESA Display Port interface specification.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,dp-display
+
+  cell-index:
+    description: Specifies the controller instance.
+
+  reg:
+    items:
+      - description: DP controller registers
+
+  interrupts:
+    description: The interrupt signal from the DP block.
+
+  clocks:
+    description: List of clock specifiers for clocks needed by the device.
+    items:
+      - description: Display Port AUX clock
+      - description: Display Port Link clock
+      - description: Link interface clock between DP and PHY
+      - description: Display Port Pixel clock
+      - description: Root clock generator for pixel clock
+
+  clock-names:
+    description: |
+      Device clock names in the same order as mentioned in clocks property.
+      The required clocks are mentioned below.
+    items:
+      - const: core_aux
+      - const: ctrl_link
+      - const: ctrl_link_iface
+      - const: stream_pixel
+      - const: pixel_rcg
+  "#clock-cells":
+    const: 1
+
+  vdda-1p2-supply:
+    description: phandle to vdda 1.2V regulator node.
+
+  vdda-0p9-supply:
+    description: phandle to vdda 0.9V regulator node.
+
+  data-lanes = <0 1>:
+    type: object
+    description: Maximum number of lanes that can be used for Display port.
+
+  ports:
+    description: |
+       Contains display port controller endpoint subnode.
+       remote-endpoint: |
+         For port@0, set to phandle of the connected panel/bridge's
+         input endpoint. For port@1, set to the DPU interface output.
+         Documentation/devicetree/bindings/graph.txt and
+         Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+patternProperties:
+  "^aux-cfg([0-9])-settings$":
+    type: object
+    description: |
+      Specifies the DP AUX configuration [0-9] settings.
+      The first entry in this array corresponds to the register offset
+      within DP AUX, while the remaining entries indicate the
+      programmable values.
+
+required:
+  - compatible
+  - cell-index
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - vdda-1p2-supply
+  - vdda-0p9-supply
+  - data-lanes
+  - ports
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    msm_dp: displayport-controller@ae90000{
+        compatible = "qcom,dp-display";
+        cell-index = <0>;
+        reg = <0 0xae90000 0 0x1400>;
+        reg-names = "dp_controller";
+
+        interrupt-parent = <&display_subsystem>;
+        interrupts = <12 0>;
+
+        clocks = <&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>,
+                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
+        clock-names = "core_aux",
+                      "ctrl_link",
+                      "ctrl_link_iface", "stream_pixel",
+                      "pixel_rcg";
+        #clock-cells = <1>;
+
+        vdda-1p2-supply = <&vreg_l3c_1p2>;
+        vdda-0p9-supply = <&vreg_l4a_0p8>;
+
+        data-lanes = <0 1>;
+
+        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.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
index 551ae26..30c8ab4 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
@@ -65,6 +65,7 @@ Required properties:
 
 	Port 0 -> DPU_INTF1 (DSI1)
 	Port 1 -> DPU_INTF2 (DSI2)
+	Port 2 -> DPU_INTF0 (DP)
 
 Optional properties:
 - assigned-clocks: list of clock specifiers for clocks needing rate assignment
@@ -136,6 +137,13 @@ Example:
 						remote-endpoint = <&dsi1_in>;
 					};
 				};
+
+				port@2 {
+					reg = <2>;
+					dpu_intf0_out: endpoint {
+						remote-endpoint = <&dp_in>;
+					};
+				};
 			};
 		};
 	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-09  3:38 ` Tanmay Shah
@ 2020-06-10  2:15   ` Stephen Boyd
  -1 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2020-06-10  2:15 UTC (permalink / raw)
  To: Tanmay Shah, devicetree
  Cc: sam, seanpaul, freedreno, linux-arm-msm, chandanu, robdclark,
	abhinavk, nganji, dri-devel, linux-clk, Vara Reddy, Tanmay Shah

Quoting Tanmay Shah (2020-06-08 20:38:18)
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> new file mode 100644
> index 0000000..5fdb915
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

Typically the file name matches the compatible string. But the
compatible string is just qcom,dp-display. Maybe the compatible string
should be qcom,sc7180-dp? Notice that the SoC number comes first as is
preferred.


> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Display Port Controller.
> +
> +maintainers:
> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> +  - Vara Reddy <varar@codeaurora.org>
> +  - Tanmay Shah <tanmay@codeaurora.org>
> +
> +description: |
> +  Device tree bindings for MSM Display Port which supports DP host controllers
> +  that are compatible with VESA Display Port interface specification.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,dp-display
> +
> +  cell-index:
> +    description: Specifies the controller instance.
> +
> +  reg:
> +    items:
> +      - description: DP controller registers
> +
> +  interrupts:
> +    description: The interrupt signal from the DP block.
> +
> +  clocks:
> +    description: List of clock specifiers for clocks needed by the device.
> +    items:
> +      - description: Display Port AUX clock
> +      - description: Display Port Link clock
> +      - description: Link interface clock between DP and PHY
> +      - description: Display Port Pixel clock
> +      - description: Root clock generator for pixel clock
> +
> +  clock-names:
> +    description: |
> +      Device clock names in the same order as mentioned in clocks property.
> +      The required clocks are mentioned below.
> +    items:
> +      - const: core_aux
> +      - const: ctrl_link
> +      - const: ctrl_link_iface
> +      - const: stream_pixel
> +      - const: pixel_rcg

Why not just 'pixel'? And why is the root clk generator important? It
looks like this binding should be using the assigned clock parents
property instead so that it doesn't have to call clk_set_parent()
explicitly.

> +  "#clock-cells":
> +    const: 1
> +
> +  vdda-1p2-supply:
> +    description: phandle to vdda 1.2V regulator node.
> +
> +  vdda-0p9-supply:
> +    description: phandle to vdda 0.9V regulator node.
> +
> +  data-lanes = <0 1>:

Is this correct? We can have = <value> in the property name? Also feels
generic and possibly should come from the phy binding instead of from
the controller binding.

> +    type: object
> +    description: Maximum number of lanes that can be used for Display port.
> +
> +  ports:
> +    description: |
> +       Contains display port controller endpoint subnode.
> +       remote-endpoint: |
> +         For port@0, set to phandle of the connected panel/bridge's
> +         input endpoint. For port@1, set to the DPU interface output.
> +         Documentation/devicetree/bindings/graph.txt and
> +         Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +patternProperties:
> +  "^aux-cfg([0-9])-settings$":
> +    type: object
> +    description: |
> +      Specifies the DP AUX configuration [0-9] settings.
> +      The first entry in this array corresponds to the register offset
> +      within DP AUX, while the remaining entries indicate the
> +      programmable values.

I'd prefer this was removed from the binding and hardcoded in the driver
until we can understand what the values are. If they're not
understandable then they most likely don't change and should be done in
the driver.

> +
> +required:
> +  - compatible
> +  - cell-index
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - vdda-1p2-supply
> +  - vdda-0p9-supply
> +  - data-lanes
> +  - ports
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    msm_dp: displayport-controller@ae90000{
> +        compatible = "qcom,dp-display";
> +        cell-index = <0>;
> +        reg = <0 0xae90000 0 0x1400>;
> +        reg-names = "dp_controller";
> +
> +        interrupt-parent = <&display_subsystem>;
> +        interrupts = <12 0>;
> +
> +        clocks = <&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>,
> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +        clock-names = "core_aux",
> +                      "ctrl_link",
> +                      "ctrl_link_iface", "stream_pixel",
> +                      "pixel_rcg";
> +        #clock-cells = <1>;
> +
> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> +
> +        data-lanes = <0 1>;
> +
> +        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 {
> +                };
> +            };
> +        };
> +    };

I believe there should be a '...' here.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-10  2:15   ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2020-06-10  2:15 UTC (permalink / raw)
  To: Tanmay Shah, devicetree
  Cc: Tanmay Shah, sam, abhinavk, seanpaul, dri-devel, linux-arm-msm,
	Vara Reddy, freedreno, linux-clk, chandanu

Quoting Tanmay Shah (2020-06-08 20:38:18)
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> new file mode 100644
> index 0000000..5fdb915
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

Typically the file name matches the compatible string. But the
compatible string is just qcom,dp-display. Maybe the compatible string
should be qcom,sc7180-dp? Notice that the SoC number comes first as is
preferred.


> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Display Port Controller.
> +
> +maintainers:
> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> +  - Vara Reddy <varar@codeaurora.org>
> +  - Tanmay Shah <tanmay@codeaurora.org>
> +
> +description: |
> +  Device tree bindings for MSM Display Port which supports DP host controllers
> +  that are compatible with VESA Display Port interface specification.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,dp-display
> +
> +  cell-index:
> +    description: Specifies the controller instance.
> +
> +  reg:
> +    items:
> +      - description: DP controller registers
> +
> +  interrupts:
> +    description: The interrupt signal from the DP block.
> +
> +  clocks:
> +    description: List of clock specifiers for clocks needed by the device.
> +    items:
> +      - description: Display Port AUX clock
> +      - description: Display Port Link clock
> +      - description: Link interface clock between DP and PHY
> +      - description: Display Port Pixel clock
> +      - description: Root clock generator for pixel clock
> +
> +  clock-names:
> +    description: |
> +      Device clock names in the same order as mentioned in clocks property.
> +      The required clocks are mentioned below.
> +    items:
> +      - const: core_aux
> +      - const: ctrl_link
> +      - const: ctrl_link_iface
> +      - const: stream_pixel
> +      - const: pixel_rcg

Why not just 'pixel'? And why is the root clk generator important? It
looks like this binding should be using the assigned clock parents
property instead so that it doesn't have to call clk_set_parent()
explicitly.

> +  "#clock-cells":
> +    const: 1
> +
> +  vdda-1p2-supply:
> +    description: phandle to vdda 1.2V regulator node.
> +
> +  vdda-0p9-supply:
> +    description: phandle to vdda 0.9V regulator node.
> +
> +  data-lanes = <0 1>:

Is this correct? We can have = <value> in the property name? Also feels
generic and possibly should come from the phy binding instead of from
the controller binding.

> +    type: object
> +    description: Maximum number of lanes that can be used for Display port.
> +
> +  ports:
> +    description: |
> +       Contains display port controller endpoint subnode.
> +       remote-endpoint: |
> +         For port@0, set to phandle of the connected panel/bridge's
> +         input endpoint. For port@1, set to the DPU interface output.
> +         Documentation/devicetree/bindings/graph.txt and
> +         Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +patternProperties:
> +  "^aux-cfg([0-9])-settings$":
> +    type: object
> +    description: |
> +      Specifies the DP AUX configuration [0-9] settings.
> +      The first entry in this array corresponds to the register offset
> +      within DP AUX, while the remaining entries indicate the
> +      programmable values.

I'd prefer this was removed from the binding and hardcoded in the driver
until we can understand what the values are. If they're not
understandable then they most likely don't change and should be done in
the driver.

> +
> +required:
> +  - compatible
> +  - cell-index
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - vdda-1p2-supply
> +  - vdda-0p9-supply
> +  - data-lanes
> +  - ports
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    msm_dp: displayport-controller@ae90000{
> +        compatible = "qcom,dp-display";
> +        cell-index = <0>;
> +        reg = <0 0xae90000 0 0x1400>;
> +        reg-names = "dp_controller";
> +
> +        interrupt-parent = <&display_subsystem>;
> +        interrupts = <12 0>;
> +
> +        clocks = <&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>,
> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +        clock-names = "core_aux",
> +                      "ctrl_link",
> +                      "ctrl_link_iface", "stream_pixel",
> +                      "pixel_rcg";
> +        #clock-cells = <1>;
> +
> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> +
> +        data-lanes = <0 1>;
> +
> +        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 {
> +                };
> +            };
> +        };
> +    };

I believe there should be a '...' here.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-10  2:15   ` Stephen Boyd
@ 2020-06-11 20:07     ` tanmay
  -1 siblings, 0 replies; 19+ messages in thread
From: tanmay @ 2020-06-11 20:07 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: devicetree, sam, seanpaul, freedreno, linux-arm-msm, chandanu,
	robdclark, abhinavk, nganji, dri-devel, linux-clk, Vara Reddy

On 2020-06-09 19:15, Stephen Boyd wrote:
> Quoting Tanmay Shah (2020-06-08 20:38:18)
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml 
>> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> new file mode 100644
>> index 0000000..5fdb915
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> 
> Typically the file name matches the compatible string. But the
> compatible string is just qcom,dp-display. Maybe the compatible string
> should be qcom,sc7180-dp? Notice that the SoC number comes first as is
> preferred.
> 
These bindings will be similar for upcoming SOC as well.
So just for understanding, when we add new SOC do we create new file 
with same bidings
with SOC number in new file name?
Instead we can keep this file's name as qcom,dp-display.yaml (same as 
compatible const) and we can include SOC number in compatible enum ?
some examples:
https://patchwork.kernel.org/patch/11448357/
https://patchwork.kernel.org/patch/11164619/
> 
>> @@ -0,0 +1,142 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Display Port Controller.
>> +
>> +maintainers:
>> +  - Chandan Uddaraju <chandanu@codeaurora.org>
>> +  - Vara Reddy <varar@codeaurora.org>
>> +  - Tanmay Shah <tanmay@codeaurora.org>
>> +
>> +description: |
>> +  Device tree bindings for MSM Display Port which supports DP host 
>> controllers
>> +  that are compatible with VESA Display Port interface specification.
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: qcom,dp-display
>> +
>> +  cell-index:
>> +    description: Specifies the controller instance.
>> +
>> +  reg:
>> +    items:
>> +      - description: DP controller registers
>> +
>> +  interrupts:
>> +    description: The interrupt signal from the DP block.
>> +
>> +  clocks:
>> +    description: List of clock specifiers for clocks needed by the 
>> device.
>> +    items:
>> +      - description: Display Port AUX clock
>> +      - description: Display Port Link clock
>> +      - description: Link interface clock between DP and PHY
>> +      - description: Display Port Pixel clock
>> +      - description: Root clock generator for pixel clock
>> +
>> +  clock-names:
>> +    description: |
>> +      Device clock names in the same order as mentioned in clocks 
>> property.
>> +      The required clocks are mentioned below.
>> +    items:
>> +      - const: core_aux
>> +      - const: ctrl_link
>> +      - const: ctrl_link_iface
>> +      - const: stream_pixel
>> +      - const: pixel_rcg
> 
> Why not just 'pixel'? And why is the root clk generator important? It
> looks like this binding should be using the assigned clock parents
> property instead so that it doesn't have to call clk_set_parent()
> explicitly.
> 
Are we talking about renaming stream_pixel to pixel only?
We divide clocks in categories: core, control and stream clock.
Similar terminology will be used in subsequent driver patches as well.

We can remove pixel_rcg use assigned clock parents property and remove 
clk_set_parent
from driver.

>> +  "#clock-cells":
>> +    const: 1
>> +
>> +  vdda-1p2-supply:
>> +    description: phandle to vdda 1.2V regulator node.
>> +
>> +  vdda-0p9-supply:
>> +    description: phandle to vdda 0.9V regulator node.
>> +
>> +  data-lanes = <0 1>:
> 
> Is this correct? We can have = <value> in the property name? Also feels
> generic and possibly should come from the phy binding instead of from
> the controller binding.
> 
We are using this property in DP controller programming sequence such as 
link training.
So I think we can keep this here.
You are right about <value>. <0 1> part should be in example only. It 
was passing through dt_binding_check though.
Here it should be like:
data-lanes:
minItems:1
maxItems:4

>> +    type: object
>> +    description: Maximum number of lanes that can be used for Display 
>> port.
>> +
>> +  ports:
>> +    description: |
>> +       Contains display port controller endpoint subnode.
>> +       remote-endpoint: |
>> +         For port@0, set to phandle of the connected panel/bridge's
>> +         input endpoint. For port@1, set to the DPU interface output.
>> +         Documentation/devicetree/bindings/graph.txt and
>> +         
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +patternProperties:
>> +  "^aux-cfg([0-9])-settings$":
>> +    type: object
>> +    description: |
>> +      Specifies the DP AUX configuration [0-9] settings.
>> +      The first entry in this array corresponds to the register 
>> offset
>> +      within DP AUX, while the remaining entries indicate the
>> +      programmable values.
> 
> I'd prefer this was removed from the binding and hardcoded in the 
> driver
> until we can understand what the values are. If they're not
> understandable then they most likely don't change and should be done in
> the driver.
> 
Typically customers tune these values by working with vendor. So for 
different boards it can be different. Even though it is hard for 
customers to do this themselves, these are still board specific and 
belong to dts. As requested earlier, we have added default values 
already and made these properties optional but, we would like to keep it 
in bindings so we can have option to tune them as required.
>> +
>> +required:
>> +  - compatible
>> +  - cell-index
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - vdda-1p2-supply
>> +  - vdda-0p9-supply
>> +  - data-lanes
>> +  - ports
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>> +    msm_dp: displayport-controller@ae90000{
>> +        compatible = "qcom,dp-display";
>> +        cell-index = <0>;
>> +        reg = <0 0xae90000 0 0x1400>;
>> +        reg-names = "dp_controller";
>> +
>> +        interrupt-parent = <&display_subsystem>;
>> +        interrupts = <12 0>;
>> +
>> +        clocks = <&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>,
>> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> +        clock-names = "core_aux",
>> +                      "ctrl_link",
>> +                      "ctrl_link_iface", "stream_pixel",
>> +                      "pixel_rcg";
>> +        #clock-cells = <1>;
>> +
>> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
>> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
>> +
>> +        data-lanes = <0 1>;
>> +
>> +        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 {
>> +                };
>> +            };
>> +        };
>> +    };
> 
> I believe there should be a '...' here.
I think you mean signature is missing? If not could you please explain?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-11 20:07     ` tanmay
  0 siblings, 0 replies; 19+ messages in thread
From: tanmay @ 2020-06-11 20:07 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: devicetree, sam, abhinavk, seanpaul, dri-devel, linux-arm-msm,
	Vara Reddy, freedreno, linux-clk, chandanu

On 2020-06-09 19:15, Stephen Boyd wrote:
> Quoting Tanmay Shah (2020-06-08 20:38:18)
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml 
>> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> new file mode 100644
>> index 0000000..5fdb915
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> 
> Typically the file name matches the compatible string. But the
> compatible string is just qcom,dp-display. Maybe the compatible string
> should be qcom,sc7180-dp? Notice that the SoC number comes first as is
> preferred.
> 
These bindings will be similar for upcoming SOC as well.
So just for understanding, when we add new SOC do we create new file 
with same bidings
with SOC number in new file name?
Instead we can keep this file's name as qcom,dp-display.yaml (same as 
compatible const) and we can include SOC number in compatible enum ?
some examples:
https://patchwork.kernel.org/patch/11448357/
https://patchwork.kernel.org/patch/11164619/
> 
>> @@ -0,0 +1,142 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Display Port Controller.
>> +
>> +maintainers:
>> +  - Chandan Uddaraju <chandanu@codeaurora.org>
>> +  - Vara Reddy <varar@codeaurora.org>
>> +  - Tanmay Shah <tanmay@codeaurora.org>
>> +
>> +description: |
>> +  Device tree bindings for MSM Display Port which supports DP host 
>> controllers
>> +  that are compatible with VESA Display Port interface specification.
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: qcom,dp-display
>> +
>> +  cell-index:
>> +    description: Specifies the controller instance.
>> +
>> +  reg:
>> +    items:
>> +      - description: DP controller registers
>> +
>> +  interrupts:
>> +    description: The interrupt signal from the DP block.
>> +
>> +  clocks:
>> +    description: List of clock specifiers for clocks needed by the 
>> device.
>> +    items:
>> +      - description: Display Port AUX clock
>> +      - description: Display Port Link clock
>> +      - description: Link interface clock between DP and PHY
>> +      - description: Display Port Pixel clock
>> +      - description: Root clock generator for pixel clock
>> +
>> +  clock-names:
>> +    description: |
>> +      Device clock names in the same order as mentioned in clocks 
>> property.
>> +      The required clocks are mentioned below.
>> +    items:
>> +      - const: core_aux
>> +      - const: ctrl_link
>> +      - const: ctrl_link_iface
>> +      - const: stream_pixel
>> +      - const: pixel_rcg
> 
> Why not just 'pixel'? And why is the root clk generator important? It
> looks like this binding should be using the assigned clock parents
> property instead so that it doesn't have to call clk_set_parent()
> explicitly.
> 
Are we talking about renaming stream_pixel to pixel only?
We divide clocks in categories: core, control and stream clock.
Similar terminology will be used in subsequent driver patches as well.

We can remove pixel_rcg use assigned clock parents property and remove 
clk_set_parent
from driver.

>> +  "#clock-cells":
>> +    const: 1
>> +
>> +  vdda-1p2-supply:
>> +    description: phandle to vdda 1.2V regulator node.
>> +
>> +  vdda-0p9-supply:
>> +    description: phandle to vdda 0.9V regulator node.
>> +
>> +  data-lanes = <0 1>:
> 
> Is this correct? We can have = <value> in the property name? Also feels
> generic and possibly should come from the phy binding instead of from
> the controller binding.
> 
We are using this property in DP controller programming sequence such as 
link training.
So I think we can keep this here.
You are right about <value>. <0 1> part should be in example only. It 
was passing through dt_binding_check though.
Here it should be like:
data-lanes:
minItems:1
maxItems:4

>> +    type: object
>> +    description: Maximum number of lanes that can be used for Display 
>> port.
>> +
>> +  ports:
>> +    description: |
>> +       Contains display port controller endpoint subnode.
>> +       remote-endpoint: |
>> +         For port@0, set to phandle of the connected panel/bridge's
>> +         input endpoint. For port@1, set to the DPU interface output.
>> +         Documentation/devicetree/bindings/graph.txt and
>> +         
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +patternProperties:
>> +  "^aux-cfg([0-9])-settings$":
>> +    type: object
>> +    description: |
>> +      Specifies the DP AUX configuration [0-9] settings.
>> +      The first entry in this array corresponds to the register 
>> offset
>> +      within DP AUX, while the remaining entries indicate the
>> +      programmable values.
> 
> I'd prefer this was removed from the binding and hardcoded in the 
> driver
> until we can understand what the values are. If they're not
> understandable then they most likely don't change and should be done in
> the driver.
> 
Typically customers tune these values by working with vendor. So for 
different boards it can be different. Even though it is hard for 
customers to do this themselves, these are still board specific and 
belong to dts. As requested earlier, we have added default values 
already and made these properties optional but, we would like to keep it 
in bindings so we can have option to tune them as required.
>> +
>> +required:
>> +  - compatible
>> +  - cell-index
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - vdda-1p2-supply
>> +  - vdda-0p9-supply
>> +  - data-lanes
>> +  - ports
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>> +    msm_dp: displayport-controller@ae90000{
>> +        compatible = "qcom,dp-display";
>> +        cell-index = <0>;
>> +        reg = <0 0xae90000 0 0x1400>;
>> +        reg-names = "dp_controller";
>> +
>> +        interrupt-parent = <&display_subsystem>;
>> +        interrupts = <12 0>;
>> +
>> +        clocks = <&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>,
>> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> +        clock-names = "core_aux",
>> +                      "ctrl_link",
>> +                      "ctrl_link_iface", "stream_pixel",
>> +                      "pixel_rcg";
>> +        #clock-cells = <1>;
>> +
>> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
>> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
>> +
>> +        data-lanes = <0 1>;
>> +
>> +        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 {
>> +                };
>> +            };
>> +        };
>> +    };
> 
> I believe there should be a '...' here.
I think you mean signature is missing? If not could you please explain?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-11 20:07     ` tanmay
@ 2020-06-16 10:55       ` Stephen Boyd
  -1 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2020-06-16 10:55 UTC (permalink / raw)
  To: tanmay
  Cc: devicetree, sam, seanpaul, freedreno, linux-arm-msm, chandanu,
	robdclark, abhinavk, nganji, dri-devel, linux-clk, Vara Reddy

Quoting tanmay@codeaurora.org (2020-06-11 13:07:09)
> On 2020-06-09 19:15, Stephen Boyd wrote:
> > Quoting Tanmay Shah (2020-06-08 20:38:18)
> >> diff --git 
> >> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml 
> >> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> >> new file mode 100644
> >> index 0000000..5fdb915
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > 
> > Typically the file name matches the compatible string. But the
> > compatible string is just qcom,dp-display. Maybe the compatible string
> > should be qcom,sc7180-dp? Notice that the SoC number comes first as is
> > preferred.
> > 
> These bindings will be similar for upcoming SOC as well.
> So just for understanding, when we add new SOC do we create new file 
> with same bidings
> with SOC number in new file name?
> Instead we can keep this file's name as qcom,dp-display.yaml (same as 
> compatible const) and we can include SOC number in compatible enum ?
> some examples:
> https://patchwork.kernel.org/patch/11448357/
> https://patchwork.kernel.org/patch/11164619/

Yes that works too. It's really up to robh here.

> > 
> >> @@ -0,0 +1,142 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Qualcomm Display Port Controller.
> >> +
> >> +maintainers:
> >> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> >> +  - Vara Reddy <varar@codeaurora.org>
> >> +  - Tanmay Shah <tanmay@codeaurora.org>
> >> +
> >> +description: |
> >> +  Device tree bindings for MSM Display Port which supports DP host 
> >> controllers
> >> +  that are compatible with VESA Display Port interface specification.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: qcom,dp-display
> >> +
> >> +  cell-index:
> >> +    description: Specifies the controller instance.
> >> +
> >> +  reg:
> >> +    items:
> >> +      - description: DP controller registers
> >> +
> >> +  interrupts:
> >> +    description: The interrupt signal from the DP block.
> >> +
> >> +  clocks:
> >> +    description: List of clock specifiers for clocks needed by the 
> >> device.
> >> +    items:
> >> +      - description: Display Port AUX clock
> >> +      - description: Display Port Link clock
> >> +      - description: Link interface clock between DP and PHY
> >> +      - description: Display Port Pixel clock
> >> +      - description: Root clock generator for pixel clock
> >> +
> >> +  clock-names:
> >> +    description: |
> >> +      Device clock names in the same order as mentioned in clocks 
> >> property.
> >> +      The required clocks are mentioned below.
> >> +    items:
> >> +      - const: core_aux
> >> +      - const: ctrl_link
> >> +      - const: ctrl_link_iface
> >> +      - const: stream_pixel
> >> +      - const: pixel_rcg
> > 
> > Why not just 'pixel'? And why is the root clk generator important? It
> > looks like this binding should be using the assigned clock parents
> > property instead so that it doesn't have to call clk_set_parent()
> > explicitly.
> > 
> Are we talking about renaming stream_pixel to pixel only?
> We divide clocks in categories: core, control and stream clock.
> Similar terminology will be used in subsequent driver patches as well.
> 
> We can remove pixel_rcg use assigned clock parents property and remove 
> clk_set_parent
> from driver.

Cool. Using assigned clock parents is good.

> 
> >> +  "#clock-cells":
> >> +    const: 1
> >> +
> >> +  vdda-1p2-supply:
> >> +    description: phandle to vdda 1.2V regulator node.
> >> +
> >> +  vdda-0p9-supply:
> >> +    description: phandle to vdda 0.9V regulator node.
> >> +
> >> +  data-lanes = <0 1>:
> > 
> > Is this correct? We can have = <value> in the property name? Also feels
> > generic and possibly should come from the phy binding instead of from
> > the controller binding.
> > 
> We are using this property in DP controller programming sequence such as 
> link training.
> So I think we can keep this here.
> You are right about <value>. <0 1> part should be in example only. It 
> was passing through dt_binding_check though.
> Here it should be like:
> data-lanes:
> minItems:1
> maxItems:4

Ok.

> 
> >> +    type: object
> >> +    description: Maximum number of lanes that can be used for Display 
> >> port.
> >> +
> >> +  ports:
> >> +    description: |
> >> +       Contains display port controller endpoint subnode.
> >> +       remote-endpoint: |
> >> +         For port@0, set to phandle of the connected panel/bridge's
> >> +         input endpoint. For port@1, set to the DPU interface output.
> >> +         Documentation/devicetree/bindings/graph.txt and
> >> +         
> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> +
> >> +patternProperties:
> >> +  "^aux-cfg([0-9])-settings$":
> >> +    type: object
> >> +    description: |
> >> +      Specifies the DP AUX configuration [0-9] settings.
> >> +      The first entry in this array corresponds to the register 
> >> offset
> >> +      within DP AUX, while the remaining entries indicate the
> >> +      programmable values.
> > 
> > I'd prefer this was removed from the binding and hardcoded in the 
> > driver
> > until we can understand what the values are. If they're not
> > understandable then they most likely don't change and should be done in
> > the driver.
> > 
> Typically customers tune these values by working with vendor. So for 
> different boards it can be different. Even though it is hard for 
> customers to do this themselves, these are still board specific and 
> belong to dts. As requested earlier, we have added default values 
> already and made these properties optional but, we would like to keep it 
> in bindings so we can have option to tune them as required.

If they're in the binding then they should make sense instead of just
being random values. So please move the defaults to the driver and
have human understandable DT properties to tune these settings. This has
been done for the qcom USB phy already (see things like
qcom,hstx-trim-value for example).

> >> +
> >> +required:
> >> +  - compatible
> >> +  - cell-index
> >> +  - reg
> >> +  - interrupts
> >> +  - clocks
> >> +  - clock-names
> >> +  - vdda-1p2-supply
> >> +  - vdda-0p9-supply
> >> +  - data-lanes
> >> +  - ports
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> >> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> >> +    msm_dp: displayport-controller@ae90000{
> >> +        compatible = "qcom,dp-display";
> >> +        cell-index = <0>;
> >> +        reg = <0 0xae90000 0 0x1400>;
> >> +        reg-names = "dp_controller";
> >> +
> >> +        interrupt-parent = <&display_subsystem>;
> >> +        interrupts = <12 0>;
> >> +
> >> +        clocks = <&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>,
> >> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> >> +        clock-names = "core_aux",
> >> +                      "ctrl_link",
> >> +                      "ctrl_link_iface", "stream_pixel",
> >> +                      "pixel_rcg";
> >> +        #clock-cells = <1>;
> >> +
> >> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> >> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> >> +
> >> +        data-lanes = <0 1>;
> >> +
> >> +        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 {
> >> +                };
> >> +            };
> >> +        };
> >> +    };
> > 
> > I believe there should be a '...' here.
> I think you mean signature is missing? If not could you please explain?

No I mean there should be a triple dot at the end.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-16 10:55       ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2020-06-16 10:55 UTC (permalink / raw)
  To: tanmay
  Cc: devicetree, sam, abhinavk, seanpaul, dri-devel, linux-arm-msm,
	Vara Reddy, freedreno, linux-clk, chandanu

Quoting tanmay@codeaurora.org (2020-06-11 13:07:09)
> On 2020-06-09 19:15, Stephen Boyd wrote:
> > Quoting Tanmay Shah (2020-06-08 20:38:18)
> >> diff --git 
> >> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml 
> >> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> >> new file mode 100644
> >> index 0000000..5fdb915
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > 
> > Typically the file name matches the compatible string. But the
> > compatible string is just qcom,dp-display. Maybe the compatible string
> > should be qcom,sc7180-dp? Notice that the SoC number comes first as is
> > preferred.
> > 
> These bindings will be similar for upcoming SOC as well.
> So just for understanding, when we add new SOC do we create new file 
> with same bidings
> with SOC number in new file name?
> Instead we can keep this file's name as qcom,dp-display.yaml (same as 
> compatible const) and we can include SOC number in compatible enum ?
> some examples:
> https://patchwork.kernel.org/patch/11448357/
> https://patchwork.kernel.org/patch/11164619/

Yes that works too. It's really up to robh here.

> > 
> >> @@ -0,0 +1,142 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Qualcomm Display Port Controller.
> >> +
> >> +maintainers:
> >> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> >> +  - Vara Reddy <varar@codeaurora.org>
> >> +  - Tanmay Shah <tanmay@codeaurora.org>
> >> +
> >> +description: |
> >> +  Device tree bindings for MSM Display Port which supports DP host 
> >> controllers
> >> +  that are compatible with VESA Display Port interface specification.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: qcom,dp-display
> >> +
> >> +  cell-index:
> >> +    description: Specifies the controller instance.
> >> +
> >> +  reg:
> >> +    items:
> >> +      - description: DP controller registers
> >> +
> >> +  interrupts:
> >> +    description: The interrupt signal from the DP block.
> >> +
> >> +  clocks:
> >> +    description: List of clock specifiers for clocks needed by the 
> >> device.
> >> +    items:
> >> +      - description: Display Port AUX clock
> >> +      - description: Display Port Link clock
> >> +      - description: Link interface clock between DP and PHY
> >> +      - description: Display Port Pixel clock
> >> +      - description: Root clock generator for pixel clock
> >> +
> >> +  clock-names:
> >> +    description: |
> >> +      Device clock names in the same order as mentioned in clocks 
> >> property.
> >> +      The required clocks are mentioned below.
> >> +    items:
> >> +      - const: core_aux
> >> +      - const: ctrl_link
> >> +      - const: ctrl_link_iface
> >> +      - const: stream_pixel
> >> +      - const: pixel_rcg
> > 
> > Why not just 'pixel'? And why is the root clk generator important? It
> > looks like this binding should be using the assigned clock parents
> > property instead so that it doesn't have to call clk_set_parent()
> > explicitly.
> > 
> Are we talking about renaming stream_pixel to pixel only?
> We divide clocks in categories: core, control and stream clock.
> Similar terminology will be used in subsequent driver patches as well.
> 
> We can remove pixel_rcg use assigned clock parents property and remove 
> clk_set_parent
> from driver.

Cool. Using assigned clock parents is good.

> 
> >> +  "#clock-cells":
> >> +    const: 1
> >> +
> >> +  vdda-1p2-supply:
> >> +    description: phandle to vdda 1.2V regulator node.
> >> +
> >> +  vdda-0p9-supply:
> >> +    description: phandle to vdda 0.9V regulator node.
> >> +
> >> +  data-lanes = <0 1>:
> > 
> > Is this correct? We can have = <value> in the property name? Also feels
> > generic and possibly should come from the phy binding instead of from
> > the controller binding.
> > 
> We are using this property in DP controller programming sequence such as 
> link training.
> So I think we can keep this here.
> You are right about <value>. <0 1> part should be in example only. It 
> was passing through dt_binding_check though.
> Here it should be like:
> data-lanes:
> minItems:1
> maxItems:4

Ok.

> 
> >> +    type: object
> >> +    description: Maximum number of lanes that can be used for Display 
> >> port.
> >> +
> >> +  ports:
> >> +    description: |
> >> +       Contains display port controller endpoint subnode.
> >> +       remote-endpoint: |
> >> +         For port@0, set to phandle of the connected panel/bridge's
> >> +         input endpoint. For port@1, set to the DPU interface output.
> >> +         Documentation/devicetree/bindings/graph.txt and
> >> +         
> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> +
> >> +patternProperties:
> >> +  "^aux-cfg([0-9])-settings$":
> >> +    type: object
> >> +    description: |
> >> +      Specifies the DP AUX configuration [0-9] settings.
> >> +      The first entry in this array corresponds to the register 
> >> offset
> >> +      within DP AUX, while the remaining entries indicate the
> >> +      programmable values.
> > 
> > I'd prefer this was removed from the binding and hardcoded in the 
> > driver
> > until we can understand what the values are. If they're not
> > understandable then they most likely don't change and should be done in
> > the driver.
> > 
> Typically customers tune these values by working with vendor. So for 
> different boards it can be different. Even though it is hard for 
> customers to do this themselves, these are still board specific and 
> belong to dts. As requested earlier, we have added default values 
> already and made these properties optional but, we would like to keep it 
> in bindings so we can have option to tune them as required.

If they're in the binding then they should make sense instead of just
being random values. So please move the defaults to the driver and
have human understandable DT properties to tune these settings. This has
been done for the qcom USB phy already (see things like
qcom,hstx-trim-value for example).

> >> +
> >> +required:
> >> +  - compatible
> >> +  - cell-index
> >> +  - reg
> >> +  - interrupts
> >> +  - clocks
> >> +  - clock-names
> >> +  - vdda-1p2-supply
> >> +  - vdda-0p9-supply
> >> +  - data-lanes
> >> +  - ports
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> >> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> >> +    msm_dp: displayport-controller@ae90000{
> >> +        compatible = "qcom,dp-display";
> >> +        cell-index = <0>;
> >> +        reg = <0 0xae90000 0 0x1400>;
> >> +        reg-names = "dp_controller";
> >> +
> >> +        interrupt-parent = <&display_subsystem>;
> >> +        interrupts = <12 0>;
> >> +
> >> +        clocks = <&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>,
> >> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> >> +        clock-names = "core_aux",
> >> +                      "ctrl_link",
> >> +                      "ctrl_link_iface", "stream_pixel",
> >> +                      "pixel_rcg";
> >> +        #clock-cells = <1>;
> >> +
> >> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> >> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> >> +
> >> +        data-lanes = <0 1>;
> >> +
> >> +        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 {
> >> +                };
> >> +            };
> >> +        };
> >> +    };
> > 
> > I believe there should be a '...' here.
> I think you mean signature is missing? If not could you please explain?

No I mean there should be a triple dot at the end.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-16 10:55       ` Stephen Boyd
@ 2020-06-16 22:30         ` tanmay
  -1 siblings, 0 replies; 19+ messages in thread
From: tanmay @ 2020-06-16 22:30 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: devicetree, sam, seanpaul, freedreno, linux-arm-msm, chandanu,
	robdclark, abhinavk, nganji, dri-devel, linux-clk, Vara Reddy

Thanks Stephen for your answers.

On 2020-06-16 03:55, Stephen Boyd wrote:
> Quoting tanmay@codeaurora.org (2020-06-11 13:07:09)
>> On 2020-06-09 19:15, Stephen Boyd wrote:
>> > Quoting Tanmay Shah (2020-06-08 20:38:18)
>> >> diff --git
>> >> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> >> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> >> new file mode 100644
>> >> index 0000000..5fdb915
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> >
>> > Typically the file name matches the compatible string. But the
>> > compatible string is just qcom,dp-display. Maybe the compatible string
>> > should be qcom,sc7180-dp? Notice that the SoC number comes first as is
>> > preferred.
>> >
>> These bindings will be similar for upcoming SOC as well.
>> So just for understanding, when we add new SOC do we create new file
>> with same bidings
>> with SOC number in new file name?
>> Instead we can keep this file's name as qcom,dp-display.yaml (same as
>> compatible const) and we can include SOC number in compatible enum ?
>> some examples:
>> https://patchwork.kernel.org/patch/11448357/
>> https://patchwork.kernel.org/patch/11164619/
> 
> Yes that works too. It's really up to robh here.
> 
>> >
>> >> @@ -0,0 +1,142 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: Qualcomm Display Port Controller.
>> >> +
>> >> +maintainers:
>> >> +  - Chandan Uddaraju <chandanu@codeaurora.org>
>> >> +  - Vara Reddy <varar@codeaurora.org>
>> >> +  - Tanmay Shah <tanmay@codeaurora.org>
>> >> +
>> >> +description: |
>> >> +  Device tree bindings for MSM Display Port which supports DP host
>> >> controllers
>> >> +  that are compatible with VESA Display Port interface specification.
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    items:
>> >> +      - const: qcom,dp-display
>> >> +
>> >> +  cell-index:
>> >> +    description: Specifies the controller instance.
>> >> +
>> >> +  reg:
>> >> +    items:
>> >> +      - description: DP controller registers
>> >> +
>> >> +  interrupts:
>> >> +    description: The interrupt signal from the DP block.
>> >> +
>> >> +  clocks:
>> >> +    description: List of clock specifiers for clocks needed by the
>> >> device.
>> >> +    items:
>> >> +      - description: Display Port AUX clock
>> >> +      - description: Display Port Link clock
>> >> +      - description: Link interface clock between DP and PHY
>> >> +      - description: Display Port Pixel clock
>> >> +      - description: Root clock generator for pixel clock
>> >> +
>> >> +  clock-names:
>> >> +    description: |
>> >> +      Device clock names in the same order as mentioned in clocks
>> >> property.
>> >> +      The required clocks are mentioned below.
>> >> +    items:
>> >> +      - const: core_aux
>> >> +      - const: ctrl_link
>> >> +      - const: ctrl_link_iface
>> >> +      - const: stream_pixel
>> >> +      - const: pixel_rcg
>> >
>> > Why not just 'pixel'? And why is the root clk generator important? It
>> > looks like this binding should be using the assigned clock parents
>> > property instead so that it doesn't have to call clk_set_parent()
>> > explicitly.
>> >
>> Are we talking about renaming stream_pixel to pixel only?
>> We divide clocks in categories: core, control and stream clock.
>> Similar terminology will be used in subsequent driver patches as well.
>> 
>> We can remove pixel_rcg use assigned clock parents property and remove
>> clk_set_parent
>> from driver.
> 
> Cool. Using assigned clock parents is good.
> 
>> 
>> >> +  "#clock-cells":
>> >> +    const: 1
>> >> +
>> >> +  vdda-1p2-supply:
>> >> +    description: phandle to vdda 1.2V regulator node.
>> >> +
>> >> +  vdda-0p9-supply:
>> >> +    description: phandle to vdda 0.9V regulator node.
>> >> +
>> >> +  data-lanes = <0 1>:
>> >
>> > Is this correct? We can have = <value> in the property name? Also feels
>> > generic and possibly should come from the phy binding instead of from
>> > the controller binding.
>> >
>> We are using this property in DP controller programming sequence such 
>> as
>> link training.
>> So I think we can keep this here.
>> You are right about <value>. <0 1> part should be in example only. It
>> was passing through dt_binding_check though.
>> Here it should be like:
>> data-lanes:
>> minItems:1
>> maxItems:4
> 
> Ok.
> 
>> 
>> >> +    type: object
>> >> +    description: Maximum number of lanes that can be used for Display
>> >> port.
>> >> +
>> >> +  ports:
>> >> +    description: |
>> >> +       Contains display port controller endpoint subnode.
>> >> +       remote-endpoint: |
>> >> +         For port@0, set to phandle of the connected panel/bridge's
>> >> +         input endpoint. For port@1, set to the DPU interface output.
>> >> +         Documentation/devicetree/bindings/graph.txt and
>> >> +
>> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
>> >> +
>> >> +patternProperties:
>> >> +  "^aux-cfg([0-9])-settings$":
>> >> +    type: object
>> >> +    description: |
>> >> +      Specifies the DP AUX configuration [0-9] settings.
>> >> +      The first entry in this array corresponds to the register
>> >> offset
>> >> +      within DP AUX, while the remaining entries indicate the
>> >> +      programmable values.
>> >
>> > I'd prefer this was removed from the binding and hardcoded in the
>> > driver
>> > until we can understand what the values are. If they're not
>> > understandable then they most likely don't change and should be done in
>> > the driver.
>> >
>> Typically customers tune these values by working with vendor. So for
>> different boards it can be different. Even though it is hard for
>> customers to do this themselves, these are still board specific and
>> belong to dts. As requested earlier, we have added default values
>> already and made these properties optional but, we would like to keep 
>> it
>> in bindings so we can have option to tune them as required.
> 
> If they're in the binding then they should make sense instead of just
> being random values. So please move the defaults to the driver and
> have human understandable DT properties to tune these settings. This 
> has
> been done for the qcom USB phy already (see things like
> qcom,hstx-trim-value for example).
> 
Ok. For now I will move these values to driver and later we will add dt 
properties as required.

>> >> +
>> >> +required:
>> >> +  - compatible
>> >> +  - cell-index
>> >> +  - reg
>> >> +  - interrupts
>> >> +  - clocks
>> >> +  - clock-names
>> >> +  - vdda-1p2-supply
>> >> +  - vdda-0p9-supply
>> >> +  - data-lanes
>> >> +  - ports
>> >> +
>> >> +examples:
>> >> +  - |
>> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> >> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>> >> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>> >> +    msm_dp: displayport-controller@ae90000{
>> >> +        compatible = "qcom,dp-display";
>> >> +        cell-index = <0>;
>> >> +        reg = <0 0xae90000 0 0x1400>;
>> >> +        reg-names = "dp_controller";
>> >> +
>> >> +        interrupt-parent = <&display_subsystem>;
>> >> +        interrupts = <12 0>;
>> >> +
>> >> +        clocks = <&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>,
>> >> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> >> +        clock-names = "core_aux",
>> >> +                      "ctrl_link",
>> >> +                      "ctrl_link_iface", "stream_pixel",
>> >> +                      "pixel_rcg";
>> >> +        #clock-cells = <1>;
>> >> +
>> >> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
>> >> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
>> >> +
>> >> +        data-lanes = <0 1>;
>> >> +
>> >> +        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 {
>> >> +                };
>> >> +            };
>> >> +        };
>> >> +    };
>> >
>> > I believe there should be a '...' here.
>> I think you mean signature is missing? If not could you please 
>> explain?
> 
> No I mean there should be a triple dot at the end.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-16 22:30         ` tanmay
  0 siblings, 0 replies; 19+ messages in thread
From: tanmay @ 2020-06-16 22:30 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: devicetree, sam, abhinavk, seanpaul, dri-devel, linux-arm-msm,
	Vara Reddy, freedreno, linux-clk, chandanu

Thanks Stephen for your answers.

On 2020-06-16 03:55, Stephen Boyd wrote:
> Quoting tanmay@codeaurora.org (2020-06-11 13:07:09)
>> On 2020-06-09 19:15, Stephen Boyd wrote:
>> > Quoting Tanmay Shah (2020-06-08 20:38:18)
>> >> diff --git
>> >> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> >> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> >> new file mode 100644
>> >> index 0000000..5fdb915
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> >
>> > Typically the file name matches the compatible string. But the
>> > compatible string is just qcom,dp-display. Maybe the compatible string
>> > should be qcom,sc7180-dp? Notice that the SoC number comes first as is
>> > preferred.
>> >
>> These bindings will be similar for upcoming SOC as well.
>> So just for understanding, when we add new SOC do we create new file
>> with same bidings
>> with SOC number in new file name?
>> Instead we can keep this file's name as qcom,dp-display.yaml (same as
>> compatible const) and we can include SOC number in compatible enum ?
>> some examples:
>> https://patchwork.kernel.org/patch/11448357/
>> https://patchwork.kernel.org/patch/11164619/
> 
> Yes that works too. It's really up to robh here.
> 
>> >
>> >> @@ -0,0 +1,142 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: Qualcomm Display Port Controller.
>> >> +
>> >> +maintainers:
>> >> +  - Chandan Uddaraju <chandanu@codeaurora.org>
>> >> +  - Vara Reddy <varar@codeaurora.org>
>> >> +  - Tanmay Shah <tanmay@codeaurora.org>
>> >> +
>> >> +description: |
>> >> +  Device tree bindings for MSM Display Port which supports DP host
>> >> controllers
>> >> +  that are compatible with VESA Display Port interface specification.
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    items:
>> >> +      - const: qcom,dp-display
>> >> +
>> >> +  cell-index:
>> >> +    description: Specifies the controller instance.
>> >> +
>> >> +  reg:
>> >> +    items:
>> >> +      - description: DP controller registers
>> >> +
>> >> +  interrupts:
>> >> +    description: The interrupt signal from the DP block.
>> >> +
>> >> +  clocks:
>> >> +    description: List of clock specifiers for clocks needed by the
>> >> device.
>> >> +    items:
>> >> +      - description: Display Port AUX clock
>> >> +      - description: Display Port Link clock
>> >> +      - description: Link interface clock between DP and PHY
>> >> +      - description: Display Port Pixel clock
>> >> +      - description: Root clock generator for pixel clock
>> >> +
>> >> +  clock-names:
>> >> +    description: |
>> >> +      Device clock names in the same order as mentioned in clocks
>> >> property.
>> >> +      The required clocks are mentioned below.
>> >> +    items:
>> >> +      - const: core_aux
>> >> +      - const: ctrl_link
>> >> +      - const: ctrl_link_iface
>> >> +      - const: stream_pixel
>> >> +      - const: pixel_rcg
>> >
>> > Why not just 'pixel'? And why is the root clk generator important? It
>> > looks like this binding should be using the assigned clock parents
>> > property instead so that it doesn't have to call clk_set_parent()
>> > explicitly.
>> >
>> Are we talking about renaming stream_pixel to pixel only?
>> We divide clocks in categories: core, control and stream clock.
>> Similar terminology will be used in subsequent driver patches as well.
>> 
>> We can remove pixel_rcg use assigned clock parents property and remove
>> clk_set_parent
>> from driver.
> 
> Cool. Using assigned clock parents is good.
> 
>> 
>> >> +  "#clock-cells":
>> >> +    const: 1
>> >> +
>> >> +  vdda-1p2-supply:
>> >> +    description: phandle to vdda 1.2V regulator node.
>> >> +
>> >> +  vdda-0p9-supply:
>> >> +    description: phandle to vdda 0.9V regulator node.
>> >> +
>> >> +  data-lanes = <0 1>:
>> >
>> > Is this correct? We can have = <value> in the property name? Also feels
>> > generic and possibly should come from the phy binding instead of from
>> > the controller binding.
>> >
>> We are using this property in DP controller programming sequence such 
>> as
>> link training.
>> So I think we can keep this here.
>> You are right about <value>. <0 1> part should be in example only. It
>> was passing through dt_binding_check though.
>> Here it should be like:
>> data-lanes:
>> minItems:1
>> maxItems:4
> 
> Ok.
> 
>> 
>> >> +    type: object
>> >> +    description: Maximum number of lanes that can be used for Display
>> >> port.
>> >> +
>> >> +  ports:
>> >> +    description: |
>> >> +       Contains display port controller endpoint subnode.
>> >> +       remote-endpoint: |
>> >> +         For port@0, set to phandle of the connected panel/bridge's
>> >> +         input endpoint. For port@1, set to the DPU interface output.
>> >> +         Documentation/devicetree/bindings/graph.txt and
>> >> +
>> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
>> >> +
>> >> +patternProperties:
>> >> +  "^aux-cfg([0-9])-settings$":
>> >> +    type: object
>> >> +    description: |
>> >> +      Specifies the DP AUX configuration [0-9] settings.
>> >> +      The first entry in this array corresponds to the register
>> >> offset
>> >> +      within DP AUX, while the remaining entries indicate the
>> >> +      programmable values.
>> >
>> > I'd prefer this was removed from the binding and hardcoded in the
>> > driver
>> > until we can understand what the values are. If they're not
>> > understandable then they most likely don't change and should be done in
>> > the driver.
>> >
>> Typically customers tune these values by working with vendor. So for
>> different boards it can be different. Even though it is hard for
>> customers to do this themselves, these are still board specific and
>> belong to dts. As requested earlier, we have added default values
>> already and made these properties optional but, we would like to keep 
>> it
>> in bindings so we can have option to tune them as required.
> 
> If they're in the binding then they should make sense instead of just
> being random values. So please move the defaults to the driver and
> have human understandable DT properties to tune these settings. This 
> has
> been done for the qcom USB phy already (see things like
> qcom,hstx-trim-value for example).
> 
Ok. For now I will move these values to driver and later we will add dt 
properties as required.

>> >> +
>> >> +required:
>> >> +  - compatible
>> >> +  - cell-index
>> >> +  - reg
>> >> +  - interrupts
>> >> +  - clocks
>> >> +  - clock-names
>> >> +  - vdda-1p2-supply
>> >> +  - vdda-0p9-supply
>> >> +  - data-lanes
>> >> +  - ports
>> >> +
>> >> +examples:
>> >> +  - |
>> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> >> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>> >> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>> >> +    msm_dp: displayport-controller@ae90000{
>> >> +        compatible = "qcom,dp-display";
>> >> +        cell-index = <0>;
>> >> +        reg = <0 0xae90000 0 0x1400>;
>> >> +        reg-names = "dp_controller";
>> >> +
>> >> +        interrupt-parent = <&display_subsystem>;
>> >> +        interrupts = <12 0>;
>> >> +
>> >> +        clocks = <&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>,
>> >> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> >> +        clock-names = "core_aux",
>> >> +                      "ctrl_link",
>> >> +                      "ctrl_link_iface", "stream_pixel",
>> >> +                      "pixel_rcg";
>> >> +        #clock-cells = <1>;
>> >> +
>> >> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
>> >> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
>> >> +
>> >> +        data-lanes = <0 1>;
>> >> +
>> >> +        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 {
>> >> +                };
>> >> +            };
>> >> +        };
>> >> +    };
>> >
>> > I believe there should be a '...' here.
>> I think you mean signature is missing? If not could you please 
>> explain?
> 
> No I mean there should be a triple dot at the end.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-17 15:38       ` Rob Herring
  (?)
@ 2020-06-17 19:52       ` Stephen Boyd
  -1 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2020-06-17 19:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, aravindh, linux-arm-msm, Abhinav Kumar, linux-kernel,
	dri-devel, Sean Paul, Tanmay Shah, Vara Reddy, freedreno,
	Sam Ravnborg, Chandan Uddaraju

Quoting Rob Herring (2020-06-17 08:38:20)
> On Tue, Jun 16, 2020 at 5:15 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Tanmay Shah (2020-06-11 18:50:26)
> > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > > new file mode 100644
> > > index 000000000000..5fdb9153df00
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > > @@ -0,0 +1,142 @@
> > > +        data-lanes = <0 1>;
> > > +
> > > +        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 {
> >
> > Just curious what is eventually connected here? This is possibly a
> > question for Rob Herring, but I can't figure out how we're supposed to
> > connect this to the USB type-c connector that is receiving the DP
> > signal. Does the type-c connector binding support connecting to this end
> > of the graph? Or should this connect to the DP phy and then the phy
> > connects to the USB type-c connector node? Right now it is empty which
> > seems wrong.
> 
> It should connect to the Type-C connector perhaps thru some sort of
> switching/muxing node, but that's not really flushed out though. See
> 'dt-bindings: chrome: Add cros-ec-typec mux props' discussion with
> your CrOS colleagues.
> 

Ok. I see that we have an sbu endpoint for an aux channel but not a DP
endpoint in the type-c connector. I guess it's an alternate mode of the
connector too so maybe that complicates things. I'll talk to pmalani.
Thanks.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-16 11:15     ` Stephen Boyd
@ 2020-06-17 15:38       ` Rob Herring
  -1 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2020-06-17 15:38 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Tanmay Shah, Sam Ravnborg, devicetree, linux-kernel,
	linux-arm-msm, dri-devel, freedreno, Sean Paul, Rob Clark,
	aravindh, Abhinav Kumar, Chandan Uddaraju, Vara Reddy

On Tue, Jun 16, 2020 at 5:15 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Tanmay Shah (2020-06-11 18:50:26)
> > diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > new file mode 100644
> > index 000000000000..5fdb9153df00
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > @@ -0,0 +1,142 @@
> > +        data-lanes = <0 1>;
> > +
> > +        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 {
>
> Just curious what is eventually connected here? This is possibly a
> question for Rob Herring, but I can't figure out how we're supposed to
> connect this to the USB type-c connector that is receiving the DP
> signal. Does the type-c connector binding support connecting to this end
> of the graph? Or should this connect to the DP phy and then the phy
> connects to the USB type-c connector node? Right now it is empty which
> seems wrong.

It should connect to the Type-C connector perhaps thru some sort of
switching/muxing node, but that's not really flushed out though. See
'dt-bindings: chrome: Add cros-ec-typec mux props' discussion with
your CrOS colleagues.

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-17 15:38       ` Rob Herring
  0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2020-06-17 15:38 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: devicetree, aravindh, linux-arm-msm, Abhinav Kumar, linux-kernel,
	dri-devel, Sean Paul, Tanmay Shah, Vara Reddy, freedreno,
	Sam Ravnborg, Chandan Uddaraju

On Tue, Jun 16, 2020 at 5:15 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Tanmay Shah (2020-06-11 18:50:26)
> > diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > new file mode 100644
> > index 000000000000..5fdb9153df00
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> > @@ -0,0 +1,142 @@
> > +        data-lanes = <0 1>;
> > +
> > +        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 {
>
> Just curious what is eventually connected here? This is possibly a
> question for Rob Herring, but I can't figure out how we're supposed to
> connect this to the USB type-c connector that is receiving the DP
> signal. Does the type-c connector binding support connecting to this end
> of the graph? Or should this connect to the DP phy and then the phy
> connects to the USB type-c connector node? Right now it is empty which
> seems wrong.

It should connect to the Type-C connector perhaps thru some sort of
switching/muxing node, but that's not really flushed out though. See
'dt-bindings: chrome: Add cros-ec-typec mux props' discussion with
your CrOS colleagues.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-12  1:50   ` Tanmay Shah
@ 2020-06-16 11:15     ` Stephen Boyd
  -1 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2020-06-16 11:15 UTC (permalink / raw)
  To: Tanmay Shah, robh+dt, sam
  Cc: devicetree, linux-kernel, linux-arm-msm, dri-devel, freedreno,
	seanpaul, robdclark, aravindh, abhinavk, Chandan Uddaraju,
	Vara Reddy, Tanmay Shah

Quoting Tanmay Shah (2020-06-11 18:50:26)
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> new file mode 100644
> index 000000000000..5fdb9153df00
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> @@ -0,0 +1,142 @@
> +        data-lanes = <0 1>;
> +
> +        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 {

Just curious what is eventually connected here? This is possibly a
question for Rob Herring, but I can't figure out how we're supposed to
connect this to the USB type-c connector that is receiving the DP
signal. Does the type-c connector binding support connecting to this end
of the graph? Or should this connect to the DP phy and then the phy
connects to the USB type-c connector node? Right now it is empty which
seems wrong.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-16 11:15     ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2020-06-16 11:15 UTC (permalink / raw)
  To: Tanmay Shah, robh+dt, sam
  Cc: devicetree, Tanmay Shah, linux-arm-msm, linux-kernel, dri-devel,
	seanpaul, abhinavk, Vara Reddy, aravindh, freedreno,
	Chandan Uddaraju

Quoting Tanmay Shah (2020-06-11 18:50:26)
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> new file mode 100644
> index 000000000000..5fdb9153df00
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> @@ -0,0 +1,142 @@
> +        data-lanes = <0 1>;
> +
> +        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 {

Just curious what is eventually connected here? This is possibly a
question for Rob Herring, but I can't figure out how we're supposed to
connect this to the USB type-c connector that is receiving the DP
signal. Does the type-c connector binding support connecting to this end
of the graph? Or should this connect to the DP phy and then the phy
connects to the USB type-c connector node? Right now it is empty which
seems wrong.

> +                };
> +            };
> +        };
> +    };
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-12  1:50   ` Tanmay Shah
@ 2020-06-12 16:21     ` Rob Herring
  -1 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2020-06-12 16:21 UTC (permalink / raw)
  To: Tanmay Shah
  Cc: Sam Ravnborg, Stephen Boyd, devicetree, linux-kernel,
	linux-arm-msm, dri-devel, freedreno, Sean Paul, Rob Clark,
	aravindh, Abhinav Kumar, Chandan Uddaraju, Vara Reddy

On Thu, Jun 11, 2020 at 7:51 PM Tanmay Shah <tanmay@codeaurora.org> wrote:
>
> From: Chandan Uddaraju <chandanu@codeaurora.org>
>
> Add bindings for Snapdragon DisplayPort controller driver.
>
> Changes in V2:
> Provide details about sel-gpio
>
> Changes in V4:
> Provide details about max dp lanes
> Change the commit text
>
> Changes in V5:
> moved dp.txt to yaml file
>
> Changes in v6:
> - Squash all AUX LUT properties into one pattern Property
> - Make aux-cfg[0-9]-settings properties optional
> - Remove PLL/PHY bindings from DP controller dts
> - Add DP clocks description
> - Remove _clk suffix from clock names
> - Rename pixel clock to stream_pixel
> - Remove redundant bindings (GPIO, PHY, HDCP clock, etc..)
> - Fix indentation
> - Add Display Port as interface of DPU in DPU bindings
>   and add port mapping accordingly.
>
> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
> Signed-off-by: Vara Reddy <varar@codeaurora.org>
> Signed-off-by: Tanmay Shah <tanmay@codeaurora.org>
> ---
>  .../bindings/display/msm/dp-sc7180.yaml       | 142 ++++++++++++++++++
>  .../devicetree/bindings/display/msm/dpu.txt   |   8 +
>  2 files changed, 150 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

Is it too much to ask for Qualcomm to coordinate your work? I'm not
going to do that for you. This conflicts with "[v4] dt-bindings: msm:
disp: add yaml schemas for DPU and DSI bindings".

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

Extra space.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Display Port Controller.
> +
> +maintainers:
> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> +  - Vara Reddy <varar@codeaurora.org>
> +  - Tanmay Shah <tanmay@codeaurora.org>
> +
> +description: |
> +  Device tree bindings for MSM Display Port which supports DP host controllers
> +  that are compatible with VESA Display Port interface specification.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,dp-display

That's what the h/w reference manual calls this? It should be SoC specific.

> +
> +  cell-index:
> +    description: Specifies the controller instance.

Pretty sure I already said no to this.

> +
> +  reg:
> +    items:
> +      - description: DP controller registers

Just: 'maxItems: 1'

> +
> +  interrupts:
> +    description: The interrupt signal from the DP block.

How many?

The description is useless. That's every 'interrupts'.

> +
> +  clocks:
> +    description: List of clock specifiers for clocks needed by the device.

That's every 'clocks' property. Drop.

> +    items:
> +      - description: Display Port AUX clock
> +      - description: Display Port Link clock
> +      - description: Link interface clock between DP and PHY
> +      - description: Display Port Pixel clock
> +      - description: Root clock generator for pixel clock
> +
> +  clock-names:
> +    description: |
> +      Device clock names in the same order as mentioned in clocks property.
> +      The required clocks are mentioned below.

Drop.

> +    items:
> +      - const: core_aux
> +      - const: ctrl_link
> +      - const: ctrl_link_iface
> +      - const: stream_pixel
> +      - const: pixel_rcg

blank line

> +  "#clock-cells":
> +    const: 1
> +
> +  vdda-1p2-supply:
> +    description: phandle to vdda 1.2V regulator node.
> +
> +  vdda-0p9-supply:
> +    description: phandle to vdda 0.9V regulator node.
> +
> +  data-lanes = <0 1>:

Err, what?!

> +    type: object

This is a DT node?

> +    description: Maximum number of lanes that can be used for Display port.
> +
> +  ports:
> +    description: |
> +       Contains display port controller endpoint subnode.
> +       remote-endpoint: |
> +         For port@0, set to phandle of the connected panel/bridge's
> +         input endpoint. For port@1, set to the DPU interface output.

Look at other schemas and see how they are done.

> +         Documentation/devicetree/bindings/graph.txt and
> +         Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +patternProperties:
> +  "^aux-cfg([0-9])-settings$":
> +    type: object

This is a DT node?

> +    description: |
> +      Specifies the DP AUX configuration [0-9] settings.
> +      The first entry in this array corresponds to the register offset
> +      within DP AUX, while the remaining entries indicate the
> +      programmable values.
> +
> +required:
> +  - compatible
> +  - cell-index
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - vdda-1p2-supply
> +  - vdda-0p9-supply
> +  - data-lanes
> +  - ports

Add:
additionalProperties: false

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    msm_dp: displayport-controller@ae90000{
> +        compatible = "qcom,dp-display";
> +        cell-index = <0>;
> +        reg = <0 0xae90000 0 0x1400>;
> +        reg-names = "dp_controller";
> +
> +        interrupt-parent = <&display_subsystem>;
> +        interrupts = <12 0>;
> +
> +        clocks = <&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>,
> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +        clock-names = "core_aux",
> +                      "ctrl_link",
> +                      "ctrl_link_iface", "stream_pixel",
> +                      "pixel_rcg";
> +        #clock-cells = <1>;
> +
> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> +
> +        data-lanes = <0 1>;
> +
> +        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.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
> index 551ae26f60da..30c8ab479b02 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -65,6 +65,7 @@ Required properties:
>
>         Port 0 -> DPU_INTF1 (DSI1)
>         Port 1 -> DPU_INTF2 (DSI2)
> +       Port 2 -> DPU_INTF0 (DP)
>
>  Optional properties:
>  - assigned-clocks: list of clock specifiers for clocks needing rate assignment
> @@ -136,6 +137,13 @@ Example:
>                                                 remote-endpoint = <&dsi1_in>;
>                                         };
>                                 };
> +
> +                               port@2 {
> +                                       reg = <2>;
> +                                       dpu_intf0_out: endpoint {
> +                                               remote-endpoint = <&dp_in>;
> +                                       };
> +                               };
>                         };
>                 };
>         };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-12 16:21     ` Rob Herring
  0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2020-06-12 16:21 UTC (permalink / raw)
  To: Tanmay Shah
  Cc: devicetree, aravindh, linux-arm-msm, linux-kernel, dri-devel,
	Stephen Boyd, Sean Paul, Abhinav Kumar, Vara Reddy, freedreno,
	Sam Ravnborg, Chandan Uddaraju

On Thu, Jun 11, 2020 at 7:51 PM Tanmay Shah <tanmay@codeaurora.org> wrote:
>
> From: Chandan Uddaraju <chandanu@codeaurora.org>
>
> Add bindings for Snapdragon DisplayPort controller driver.
>
> Changes in V2:
> Provide details about sel-gpio
>
> Changes in V4:
> Provide details about max dp lanes
> Change the commit text
>
> Changes in V5:
> moved dp.txt to yaml file
>
> Changes in v6:
> - Squash all AUX LUT properties into one pattern Property
> - Make aux-cfg[0-9]-settings properties optional
> - Remove PLL/PHY bindings from DP controller dts
> - Add DP clocks description
> - Remove _clk suffix from clock names
> - Rename pixel clock to stream_pixel
> - Remove redundant bindings (GPIO, PHY, HDCP clock, etc..)
> - Fix indentation
> - Add Display Port as interface of DPU in DPU bindings
>   and add port mapping accordingly.
>
> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
> Signed-off-by: Vara Reddy <varar@codeaurora.org>
> Signed-off-by: Tanmay Shah <tanmay@codeaurora.org>
> ---
>  .../bindings/display/msm/dp-sc7180.yaml       | 142 ++++++++++++++++++
>  .../devicetree/bindings/display/msm/dpu.txt   |   8 +
>  2 files changed, 150 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

Is it too much to ask for Qualcomm to coordinate your work? I'm not
going to do that for you. This conflicts with "[v4] dt-bindings: msm:
disp: add yaml schemas for DPU and DSI bindings".

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

Extra space.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Display Port Controller.
> +
> +maintainers:
> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> +  - Vara Reddy <varar@codeaurora.org>
> +  - Tanmay Shah <tanmay@codeaurora.org>
> +
> +description: |
> +  Device tree bindings for MSM Display Port which supports DP host controllers
> +  that are compatible with VESA Display Port interface specification.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,dp-display

That's what the h/w reference manual calls this? It should be SoC specific.

> +
> +  cell-index:
> +    description: Specifies the controller instance.

Pretty sure I already said no to this.

> +
> +  reg:
> +    items:
> +      - description: DP controller registers

Just: 'maxItems: 1'

> +
> +  interrupts:
> +    description: The interrupt signal from the DP block.

How many?

The description is useless. That's every 'interrupts'.

> +
> +  clocks:
> +    description: List of clock specifiers for clocks needed by the device.

That's every 'clocks' property. Drop.

> +    items:
> +      - description: Display Port AUX clock
> +      - description: Display Port Link clock
> +      - description: Link interface clock between DP and PHY
> +      - description: Display Port Pixel clock
> +      - description: Root clock generator for pixel clock
> +
> +  clock-names:
> +    description: |
> +      Device clock names in the same order as mentioned in clocks property.
> +      The required clocks are mentioned below.

Drop.

> +    items:
> +      - const: core_aux
> +      - const: ctrl_link
> +      - const: ctrl_link_iface
> +      - const: stream_pixel
> +      - const: pixel_rcg

blank line

> +  "#clock-cells":
> +    const: 1
> +
> +  vdda-1p2-supply:
> +    description: phandle to vdda 1.2V regulator node.
> +
> +  vdda-0p9-supply:
> +    description: phandle to vdda 0.9V regulator node.
> +
> +  data-lanes = <0 1>:

Err, what?!

> +    type: object

This is a DT node?

> +    description: Maximum number of lanes that can be used for Display port.
> +
> +  ports:
> +    description: |
> +       Contains display port controller endpoint subnode.
> +       remote-endpoint: |
> +         For port@0, set to phandle of the connected panel/bridge's
> +         input endpoint. For port@1, set to the DPU interface output.

Look at other schemas and see how they are done.

> +         Documentation/devicetree/bindings/graph.txt and
> +         Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +patternProperties:
> +  "^aux-cfg([0-9])-settings$":
> +    type: object

This is a DT node?

> +    description: |
> +      Specifies the DP AUX configuration [0-9] settings.
> +      The first entry in this array corresponds to the register offset
> +      within DP AUX, while the remaining entries indicate the
> +      programmable values.
> +
> +required:
> +  - compatible
> +  - cell-index
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - vdda-1p2-supply
> +  - vdda-0p9-supply
> +  - data-lanes
> +  - ports

Add:
additionalProperties: false

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    msm_dp: displayport-controller@ae90000{
> +        compatible = "qcom,dp-display";
> +        cell-index = <0>;
> +        reg = <0 0xae90000 0 0x1400>;
> +        reg-names = "dp_controller";
> +
> +        interrupt-parent = <&display_subsystem>;
> +        interrupts = <12 0>;
> +
> +        clocks = <&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>,
> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +        clock-names = "core_aux",
> +                      "ctrl_link",
> +                      "ctrl_link_iface", "stream_pixel",
> +                      "pixel_rcg";
> +        #clock-cells = <1>;
> +
> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> +
> +        data-lanes = <0 1>;
> +
> +        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.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
> index 551ae26f60da..30c8ab479b02 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -65,6 +65,7 @@ Required properties:
>
>         Port 0 -> DPU_INTF1 (DSI1)
>         Port 1 -> DPU_INTF2 (DSI2)
> +       Port 2 -> DPU_INTF0 (DP)
>
>  Optional properties:
>  - assigned-clocks: list of clock specifiers for clocks needing rate assignment
> @@ -136,6 +137,13 @@ Example:
>                                                 remote-endpoint = <&dsi1_in>;
>                                         };
>                                 };
> +
> +                               port@2 {
> +                                       reg = <2>;
> +                                       dpu_intf0_out: endpoint {
> +                                               remote-endpoint = <&dp_in>;
> +                                       };
> +                               };
>                         };
>                 };
>         };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
  2020-06-12  1:50 [PATCH v6 0/5] Add support for DisplayPort driver on Tanmay Shah
@ 2020-06-12  1:50   ` Tanmay Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Tanmay Shah @ 2020-06-12  1:50 UTC (permalink / raw)
  To: sam, robh+dt, swboyd
  Cc: devicetree, linux-kernel, linux-arm-msm, dri-devel, freedreno,
	seanpaul, robdclark, aravindh, abhinavk, Chandan Uddaraju,
	Vara Reddy, Tanmay Shah

From: Chandan Uddaraju <chandanu@codeaurora.org>

Add bindings for Snapdragon DisplayPort controller driver.

Changes in V2:
Provide details about sel-gpio

Changes in V4:
Provide details about max dp lanes
Change the commit text

Changes in V5:
moved dp.txt to yaml file

Changes in v6:
- Squash all AUX LUT properties into one pattern Property
- Make aux-cfg[0-9]-settings properties optional
- Remove PLL/PHY bindings from DP controller dts
- Add DP clocks description
- Remove _clk suffix from clock names
- Rename pixel clock to stream_pixel
- Remove redundant bindings (GPIO, PHY, HDCP clock, etc..)
- Fix indentation
- Add Display Port as interface of DPU in DPU bindings
  and add port mapping accordingly.

Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
Signed-off-by: Vara Reddy <varar@codeaurora.org>
Signed-off-by: Tanmay Shah <tanmay@codeaurora.org>
---
 .../bindings/display/msm/dp-sc7180.yaml       | 142 ++++++++++++++++++
 .../devicetree/bindings/display/msm/dpu.txt   |   8 +
 2 files changed, 150 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
new file mode 100644
index 000000000000..5fdb9153df00
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display Port Controller.
+
+maintainers:
+  - Chandan Uddaraju <chandanu@codeaurora.org>
+  - Vara Reddy <varar@codeaurora.org>
+  - Tanmay Shah <tanmay@codeaurora.org>
+
+description: |
+  Device tree bindings for MSM Display Port which supports DP host controllers
+  that are compatible with VESA Display Port interface specification.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,dp-display
+
+  cell-index:
+    description: Specifies the controller instance.
+
+  reg:
+    items:
+      - description: DP controller registers
+
+  interrupts:
+    description: The interrupt signal from the DP block.
+
+  clocks:
+    description: List of clock specifiers for clocks needed by the device.
+    items:
+      - description: Display Port AUX clock
+      - description: Display Port Link clock
+      - description: Link interface clock between DP and PHY
+      - description: Display Port Pixel clock
+      - description: Root clock generator for pixel clock
+
+  clock-names:
+    description: |
+      Device clock names in the same order as mentioned in clocks property.
+      The required clocks are mentioned below.
+    items:
+      - const: core_aux
+      - const: ctrl_link
+      - const: ctrl_link_iface
+      - const: stream_pixel
+      - const: pixel_rcg
+  "#clock-cells":
+    const: 1
+
+  vdda-1p2-supply:
+    description: phandle to vdda 1.2V regulator node.
+
+  vdda-0p9-supply:
+    description: phandle to vdda 0.9V regulator node.
+
+  data-lanes = <0 1>:
+    type: object
+    description: Maximum number of lanes that can be used for Display port.
+
+  ports:
+    description: |
+       Contains display port controller endpoint subnode.
+       remote-endpoint: |
+         For port@0, set to phandle of the connected panel/bridge's
+         input endpoint. For port@1, set to the DPU interface output.
+         Documentation/devicetree/bindings/graph.txt and
+         Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+patternProperties:
+  "^aux-cfg([0-9])-settings$":
+    type: object
+    description: |
+      Specifies the DP AUX configuration [0-9] settings.
+      The first entry in this array corresponds to the register offset
+      within DP AUX, while the remaining entries indicate the
+      programmable values.
+
+required:
+  - compatible
+  - cell-index
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - vdda-1p2-supply
+  - vdda-0p9-supply
+  - data-lanes
+  - ports
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    msm_dp: displayport-controller@ae90000{
+        compatible = "qcom,dp-display";
+        cell-index = <0>;
+        reg = <0 0xae90000 0 0x1400>;
+        reg-names = "dp_controller";
+
+        interrupt-parent = <&display_subsystem>;
+        interrupts = <12 0>;
+
+        clocks = <&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>,
+                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
+        clock-names = "core_aux",
+                      "ctrl_link",
+                      "ctrl_link_iface", "stream_pixel",
+                      "pixel_rcg";
+        #clock-cells = <1>;
+
+        vdda-1p2-supply = <&vreg_l3c_1p2>;
+        vdda-0p9-supply = <&vreg_l4a_0p8>;
+
+        data-lanes = <0 1>;
+
+        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.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
index 551ae26f60da..30c8ab479b02 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
@@ -65,6 +65,7 @@ Required properties:
 
 	Port 0 -> DPU_INTF1 (DSI1)
 	Port 1 -> DPU_INTF2 (DSI2)
+	Port 2 -> DPU_INTF0 (DP)
 
 Optional properties:
 - assigned-clocks: list of clock specifiers for clocks needing rate assignment
@@ -136,6 +137,13 @@ Example:
 						remote-endpoint = <&dsi1_in>;
 					};
 				};
+
+				port@2 {
+					reg = <2>;
+					dpu_intf0_out: endpoint {
+						remote-endpoint = <&dp_in>;
+					};
+				};
 			};
 		};
 	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
@ 2020-06-12  1:50   ` Tanmay Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Tanmay Shah @ 2020-06-12  1:50 UTC (permalink / raw)
  To: sam, robh+dt, swboyd
  Cc: devicetree, Tanmay Shah, linux-arm-msm, linux-kernel, dri-devel,
	seanpaul, abhinavk, Vara Reddy, aravindh, freedreno,
	Chandan Uddaraju

From: Chandan Uddaraju <chandanu@codeaurora.org>

Add bindings for Snapdragon DisplayPort controller driver.

Changes in V2:
Provide details about sel-gpio

Changes in V4:
Provide details about max dp lanes
Change the commit text

Changes in V5:
moved dp.txt to yaml file

Changes in v6:
- Squash all AUX LUT properties into one pattern Property
- Make aux-cfg[0-9]-settings properties optional
- Remove PLL/PHY bindings from DP controller dts
- Add DP clocks description
- Remove _clk suffix from clock names
- Rename pixel clock to stream_pixel
- Remove redundant bindings (GPIO, PHY, HDCP clock, etc..)
- Fix indentation
- Add Display Port as interface of DPU in DPU bindings
  and add port mapping accordingly.

Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
Signed-off-by: Vara Reddy <varar@codeaurora.org>
Signed-off-by: Tanmay Shah <tanmay@codeaurora.org>
---
 .../bindings/display/msm/dp-sc7180.yaml       | 142 ++++++++++++++++++
 .../devicetree/bindings/display/msm/dpu.txt   |   8 +
 2 files changed, 150 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
new file mode 100644
index 000000000000..5fdb9153df00
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display Port Controller.
+
+maintainers:
+  - Chandan Uddaraju <chandanu@codeaurora.org>
+  - Vara Reddy <varar@codeaurora.org>
+  - Tanmay Shah <tanmay@codeaurora.org>
+
+description: |
+  Device tree bindings for MSM Display Port which supports DP host controllers
+  that are compatible with VESA Display Port interface specification.
+
+properties:
+  compatible:
+    items:
+      - const: qcom,dp-display
+
+  cell-index:
+    description: Specifies the controller instance.
+
+  reg:
+    items:
+      - description: DP controller registers
+
+  interrupts:
+    description: The interrupt signal from the DP block.
+
+  clocks:
+    description: List of clock specifiers for clocks needed by the device.
+    items:
+      - description: Display Port AUX clock
+      - description: Display Port Link clock
+      - description: Link interface clock between DP and PHY
+      - description: Display Port Pixel clock
+      - description: Root clock generator for pixel clock
+
+  clock-names:
+    description: |
+      Device clock names in the same order as mentioned in clocks property.
+      The required clocks are mentioned below.
+    items:
+      - const: core_aux
+      - const: ctrl_link
+      - const: ctrl_link_iface
+      - const: stream_pixel
+      - const: pixel_rcg
+  "#clock-cells":
+    const: 1
+
+  vdda-1p2-supply:
+    description: phandle to vdda 1.2V regulator node.
+
+  vdda-0p9-supply:
+    description: phandle to vdda 0.9V regulator node.
+
+  data-lanes = <0 1>:
+    type: object
+    description: Maximum number of lanes that can be used for Display port.
+
+  ports:
+    description: |
+       Contains display port controller endpoint subnode.
+       remote-endpoint: |
+         For port@0, set to phandle of the connected panel/bridge's
+         input endpoint. For port@1, set to the DPU interface output.
+         Documentation/devicetree/bindings/graph.txt and
+         Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+patternProperties:
+  "^aux-cfg([0-9])-settings$":
+    type: object
+    description: |
+      Specifies the DP AUX configuration [0-9] settings.
+      The first entry in this array corresponds to the register offset
+      within DP AUX, while the remaining entries indicate the
+      programmable values.
+
+required:
+  - compatible
+  - cell-index
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - vdda-1p2-supply
+  - vdda-0p9-supply
+  - data-lanes
+  - ports
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    msm_dp: displayport-controller@ae90000{
+        compatible = "qcom,dp-display";
+        cell-index = <0>;
+        reg = <0 0xae90000 0 0x1400>;
+        reg-names = "dp_controller";
+
+        interrupt-parent = <&display_subsystem>;
+        interrupts = <12 0>;
+
+        clocks = <&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>,
+                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
+        clock-names = "core_aux",
+                      "ctrl_link",
+                      "ctrl_link_iface", "stream_pixel",
+                      "pixel_rcg";
+        #clock-cells = <1>;
+
+        vdda-1p2-supply = <&vreg_l3c_1p2>;
+        vdda-0p9-supply = <&vreg_l4a_0p8>;
+
+        data-lanes = <0 1>;
+
+        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.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
index 551ae26f60da..30c8ab479b02 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
@@ -65,6 +65,7 @@ Required properties:
 
 	Port 0 -> DPU_INTF1 (DSI1)
 	Port 1 -> DPU_INTF2 (DSI2)
+	Port 2 -> DPU_INTF0 (DP)
 
 Optional properties:
 - assigned-clocks: list of clock specifiers for clocks needing rate assignment
@@ -136,6 +137,13 @@ Example:
 						remote-endpoint = <&dsi1_in>;
 					};
 				};
+
+				port@2 {
+					reg = <2>;
+					dpu_intf0_out: endpoint {
+						remote-endpoint = <&dp_in>;
+					};
+				};
 			};
 		};
 	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-06-18  7:18 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-09  3:38 [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon Tanmay Shah
2020-06-09  3:38 ` Tanmay Shah
2020-06-10  2:15 ` Stephen Boyd
2020-06-10  2:15   ` Stephen Boyd
2020-06-11 20:07   ` tanmay
2020-06-11 20:07     ` tanmay
2020-06-16 10:55     ` Stephen Boyd
2020-06-16 10:55       ` Stephen Boyd
2020-06-16 22:30       ` tanmay
2020-06-16 22:30         ` tanmay
2020-06-12  1:50 [PATCH v6 0/5] Add support for DisplayPort driver on Tanmay Shah
2020-06-12  1:50 ` [PATCH v6 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon Tanmay Shah
2020-06-12  1:50   ` Tanmay Shah
2020-06-12 16:21   ` Rob Herring
2020-06-12 16:21     ` Rob Herring
2020-06-16 11:15   ` Stephen Boyd
2020-06-16 11:15     ` Stephen Boyd
2020-06-17 15:38     ` Rob Herring
2020-06-17 15:38       ` Rob Herring
2020-06-17 19:52       ` Stephen Boyd

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.