[DPU PATCH v5 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon
tanmay at codeaurora.org
tanmay at codeaurora.org
Tue Apr 7 04:23:09 UTC 2020
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 at ravnborg.org>
Sent: Tuesday, March 31, 2020 10:50 PM
To: Tanmay Shah <tanmay at codeaurora.org>
Cc: freedreno at lists.freedesktop.org; linux-arm-msm at vger.kernel.org;
devicetree at vger.kernel.org; seanpaul at chromium.org; swboyd at chromium.org;
abhinavk at codeaurora.org; hoegsberg at google.com;
dri-devel at lists.freedesktop.org; Vara Reddy <varar at codeaurora.org>;
aravindh at codeaurora.org; linux-clk at vger.kernel.org; Chandan Uddaraju
<chandanu at codeaurora.org>
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 at 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 at codeaurora.org>
> Signed-off-by: Vara Reddy <varar at 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 at codeaurora.org>
> + - Vara Reddy <varar at 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 at 0, set to phandle of the connected panel/bridge's
> + input endpoint. For port at 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 at 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 at 0 {
> + reg = <0>;
> + dp_in: endpoint {
> + remote-endpoint = <&dpu_intf0_out>;
> + };
> + };
> +
> + port at 1 {
> + reg = <1>;
> + dp_out: endpoint {
> + };
> + };
> + };
> + };
> +
> + dp_pll: dp-pll at 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 at 0 {
> reg = <0>;
> - dpu_intf1_out: endpoint {
> - remote-endpoint =
<&dsi0_in>;
> + dpu_intf0_out: endpoint {
> + remote-endpoint = <&dp_in>;
> };
> };
>
> port at 1 {
> reg = <1>;
> + dpu_intf1_out: endpoint {
> + remote-endpoint =
<&dsi0_in>;
> + };
> + };
> +
> + port at 2 {
> + reg = <2>;
> dpu_intf2_out: endpoint {
> remote-endpoint =
<&dsi1_in>;
> };
> --
> 1.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list