Hi Sam, Thanks for reviews. The changes were posted by Vara Reddy. Due to some configuration errors, changes were posted with my E-mail ID. Vara will be addressing comments, and we will take care of this error with next patchset. Thanks, Tanmay -----Original Message----- From: Sam Ravnborg <sam@xxxxxxxxxxxx> Sent: Tuesday, March 31, 2020 10:50 PM To: Tanmay Shah <tanmay@xxxxxxxxxxxxxx> Cc: freedreno@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx; swboyd@xxxxxxxxxxxx; abhinavk@xxxxxxxxxxxxxx; hoegsberg@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Vara Reddy <varar@xxxxxxxxxxxxxx>; aravindh@xxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; Chandan Uddaraju <chandanu@xxxxxxxxxxxxxx> Subject: 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@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > Signed-off-by: Vara Reddy <varar@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > + - Vara Reddy <varar@xxxxxxxxxxxxxx> > + > +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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel
