Re: [DPU PATCH v5 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon

Hi Tanmay

Reviewing the yaml bindings triggered a few comments. See below.

Sam

On Tue, Mar 31, 2020 at 05:30:27PM -0700, Tanmay Shah wrote:

From: Chandan Uddaraju chandanu@codeaurora.org

Add bindings for Snapdragon DisplayPort and display-port PLL 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.

Signed-off-by: Chandan Uddaraju chandanu@codeaurora.org Signed-off-by: Vara Reddy varar@codeaurora.org

As you handle the patch, thus the patch passed throgh you, you are supposed to sign-off the patch.

The changes to dpu.txt is not explained in the changelog.

---

.../devicetree/bindings/display/msm/dp-sc7180.yaml | 325 +++++++++++++++++++++ .../devicetree/bindings/display/msm/dpu.txt | 16 +- 2 files changed, 337 insertions(+), 4 deletions(-) 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..761a01d --- /dev/null +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml @@ -0,0 +1,325 @@ +# SPDX-License-Identifier: GPL-2.0-only

For new bindings please use: (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: Description of Qualcomm Display Port dt properties.

+maintainers:

• • Chandan Uddaraju chandanu@codeaurora.org
• • Vara Reddy varar@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:

• "msm_dp":

The quotes seems not necessary. This describes the name of the node. The typical way to identify a node is using a compatible.

So I think that the right solution here is to drop "msm_dp".

• type: object
• description: |

•  Node containing Display port register address bases, clocks, power supplies.
  

And start here.

• properties:

• compatible:
  

•   items:
  

•     - const: qcom,dp-display
  

• cell-index:
  

•   description: Specifies the controller instance.
  

• reg:
  

•   description: Physical base address and length of controller's registers.
  

This description is generic and can be omitted. But it would be good with a descrition of the individual registers like this:

reg: items: - description: AHB bla bla - description: aux bla bla

• reg-names:
  

•   description: |
  

•     Names for different register regions defined above. The required region
  

•     is mentioned below.
  

•   items:
  

•     - const: dp_ahb
  

•     - const: dp_aux
  

•     - const: dp_link
  

•     - const: dp_p0
  

•     - const: dp_phy
  

•     - const: dp_ln_tx0
  

•     - const: dp_ln_tx1
  

•     - const: afprom_physical
  

•     - const: dp_pll
  

•     - const: usb3_dp_com
  

•     - const: hdcp_physical
  

• interrupts:
  

•   description: The interrupt signal from the DP block.
  

• clocks:
  

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

items: - description: aux clock bla bla - description: ref clock bla bla

• 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_clk
  

•     - const: core_ref_clk_src
  

•     - const: core_usb_ref_clk
  

•     - const: core_usb_cfg_ahb_clk
  

•     - const: core_usb_pipe_clk
  

•     - const: ctrl_link_clk
  

•     - const: ctrl_link_iface_clk
  

•     - const: ctrl_pixel_clk
  

•     - const: crypto_clk
  

•     - const: pixel_clk_rcg
  

• pll-node:
  

•   description: phandle to DP PLL node.
  

Add type (phandle)

• vdda-1p2-supply:
  

•   description: phandle to vdda 1.2V regulator node.
  

• vdda-0p9-supply:
  

•   description: phandle to vdda 0.9V regulator node.
  

• aux-cfg0-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 0 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

Add type, goes for all *-settings

• aux-cfg1-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 1 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg2-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 2 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg3-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 3 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg4-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 4 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg5-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 5 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg6-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 6 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg7-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 7 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg8-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 8 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• aux-cfg9-settings:
  

•   description: |
  

•     Specifies the DP AUX configuration 9 settings.
  

•     The first entry in this array corresponds to the register offset
  

•     within DP AUX, while the remaining entries indicate the
  

•     programmable values.
  

• max-pclk-frequency-khz:
  

•   description: Maximum displayport pixel clock supported for the chipset.
  

• data-lanes:
  

•   description: Maximum number of lanes that can be used for Display port.
  

• usbplug-cc-gpio:
  

•   maxItems: 1
  

•   description: Specifies the usbplug orientation gpio.
  

Shall be named -gpios. Goes for all -gpio properties. maxItems: 1 is good. Keep it.

• aux-en-gpio:
  

•   maxItems: 1
  

•   description: Specifies the aux-channel enable gpio.
  

• aux-sel-gpio:
  

•   maxItems: 1
  

•   description: Specifies the sux-channel select gpio.
  

• 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.
  

• "dp_pll":

quotes should not be required here.

I looks like yo try to describe two differents nodes in the same file. Consider to split in two files.

Some of the comments from above applies here too.

• type: object
  

• description: Node contains properties of Display port pll and phy driver.
  

• properties:
  

•   compatible:
  

•     items:
  

•       - const: qcom,dp-pll-10nm
  

•   cell-index:
  

•     description: Specifies the controller instance.
  

•   reg:
  

•     description: Physical base address and length of DP phy and pll registers.
  

•   reg-names:
  

•     description: |
  

•       Names for different register regions defined above. The required region
  

•       is mentioned below.
  

•     items:
  

•       - const: pll_base
  

•       - const: phy_base
  

•       - const: ln_tx0_base
  

•       - const: ln_tx1_base
  

•       - const: gdsc_base
  

•   clocks:
  

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

•   clock-names:
  

•     description: |
  

•       Device clock names in the same order as mentioned in clocks property.
  

•       The required clocks are mentioned below.
  

•     items:
  

•       - const: iface
  

•       - const: ref
  

•       - const: cfg_ahb
  

•       - const: pipe
  

+examples:

4 spaces as indent - good. You have include files - good.

• • |
• #include <dt-bindings/interrupt-controller/arm-gic.h>
• #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
• #include <dt-bindings/clock/qcom,gcc-sdm845.h>
• #include <dt-bindings/clock/qcom,rpmh.h>
• #include <dt-bindings/gpio/gpio.h>
• msm_dp: displayport-controller@ae90000{

•            cell-index = <0>;
  

indent only four spaces

•    compatible = "qcom,dp-display";
  

•    reg =   <0 0xae90000 0 0x200>,
  

Only one space after '=' - rememebr to adjust indent in following lines.

•            <0 0xae90200 0 0x200>,
  

•            <0 0xae90400 0 0xc00>,
  

•            <0 0xae91000 0 0x400>,
  

•            <0 0x88eaa00 0 0x200>,
  

•            <0 0x88ea200 0 0x200>,
  

•            <0 0x88ea600 0 0x200>,
  

•            <0 0x780000 0 0x6228>,
  

•            <0 0x088ea000 0 0x40>,
  

•            <0 0x88e8000 0 0x20>,
  

•            <0 0x0aee1000 0 0x034>;
  

•    reg-names = "dp_ahb", "dp_aux", "dp_link",
  

•        "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
  

•        "qfprom_physical", "dp_pll",
  

•        "usb3_dp_com", "hdcp_physical";
  

Indent so names in following lines starts where names in previous lines starts. Like this: reg-names = "dp_ahb", "dp_aux", "dp_link", "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1", "qfprom_physical", "dp_pll", "usb3_dp_com", "hdcp_physical";

•    interrupt-parent = <&display_subsystem>;
  

•    interrupts = <12 0>;
  

•    clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
  

•        <&rpmhcc RPMH_CXO_CLK>,
  

Indent so '<' are aligned under each other. Like done above for reg =

•        <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
  

•        <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
  

•        <&gcc GCC_USB3_PRIM_PHY_PIPE_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_CRYPTO_CLK>,
  

•        <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
  

•    clock-names = "core_aux_clk", "core_ref_clk_src",
  

•        "core_usb_ref_clk", "core_usb_cfg_ahb_clk",
  

•        "core_usb_pipe_clk", "ctrl_link_clk",
  

•        "ctrl_link_iface_clk", "ctrl_pixel_clk",
  

•        "crypto_clk", "pixel_clk_rcg";
  

Fix indent

•    pll-node = <&dp_pll>;
  

•    vdda-1p2-supply = <&vreg_l3c_1p2>;
  

•    vdda-0p9-supply = <&vreg_l4a_0p8>;
  

•    aux-cfg0-settings = [20 00];
  

•    aux-cfg1-settings = [24 13 23 1d];
  

•    aux-cfg2-settings = [28 24];
  

•    aux-cfg3-settings = [2c 00];
  

•    aux-cfg4-settings = [30 0a];
  

•    aux-cfg5-settings = [34 26];
  

•    aux-cfg6-settings = [38 0a];
  

•    aux-cfg7-settings = [3c 03];
  

•    aux-cfg8-settings = [40 bb];
  

•    aux-cfg9-settings = [44 03];
  

•    max-pclk-frequency-khz = <67500>;
  

•    data-lanes = <2>;
  

•    aux-en-gpio = <&msmgpio 55 1>;
  

•    aux-sel-gpio = <&msmgpio 110 1>;
  

•    usbplug-cc-gpio = <&msmgpio 90 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 {
  

•            };
  

•        };
  

•    };
  

• };
• dp_pll: dp-pll@088ea000 {

•    compatible = "qcom,dp-pll-10nm";
  

•    label = "DP PLL";
  

•    cell-index = <0>;
  

•    #clock-cells = <1>;
  

•    reg = <0 0x088ea000 0 0x200>,
  

•          <0 0x088eaa00 0 0x200>,
  

•          <0 0x088ea200 0 0x200>,
  

•          <0 0x088ea600 0 0x200>,
  

•          <0 0x08803000 0 0x8>;
  

•    reg-names = "pll_base", "phy_base", "ln_tx0_base",
  

•        "ln_tx1_base", "gdsc_base";
  

•    clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
  

•         <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
  

•         <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
  

•         <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
  

•    clock-names = "iface_clk", "ref_clk",
  

•        "cfg_ahb_clk", "pipe_clk";
  

• };

diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt index 551ae26..7e99e45 100644 --- a/Documentation/devicetree/bindings/display/msm/dpu.txt +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt @@ -63,8 +63,9 @@ Required properties: Documentation/devicetree/bindings/graph.txt Documentation/devicetree/bindings/media/video-interfaces.txt

• Port 0 -> DPU_INTF1 (DSI1)
• Port 1 -> DPU_INTF2 (DSI2)

• Port 0 -> DPU_INTF0 (DP)
• Port 1 -> DPU_INTF1 (DSI1)
• Port 2 -> DPU_INTF2 (DSI2)

Optional properties:

• assigned-clocks: list of clock specifiers for clocks needing rate assignment

@@ -125,13 +126,20 @@ Example:

		port@0 {
			reg = <0>;

• 			dpu_intf1_out: endpoint {
  

• 				remote-endpoint = <&dsi0_in>;
  

• 			dpu_intf0_out: endpoint {
  

• 				remote-endpoint = <&dp_in>;
  		};
  	};
  
  	port@1 {
  		reg = <1>;
  

• 			dpu_intf1_out: endpoint {
  

• 				remote-endpoint = <&dsi0_in>;
  

• 			};
  

• 		};
  

• 		port@2 {
  

• 			reg = <2>;
  		dpu_intf2_out: endpoint {
  			remote-endpoint = <&dsi1_in>;
  		};
  

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