Re: [PATCH v2 19/21] dt-bindings: msm/disp: Add bindings for Snapdragon 845 DPU

[Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On Thu, Jul 12, 2018 at 05:08:37PM -0400, Sean Paul wrote:
> From: Jeykumar Sankaran <jsanka@codeaurora.org>
> 
> Adds bindings for Snapdragon 845 display processing unit
> 
> Changes in v2:
>  - Use SoC specific compatibles for mdss and dpu
>  - Use assigned-clocks to set initial clock frequency
> 
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> Signed-off-by: Rajesh Yadav <ryadav@codeaurora.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  .../devicetree/bindings/display/msm/dpu.txt   | 136 ++++++++++++++++++
>  1 file changed, 136 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/msm/dpu.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
> new file mode 100644
> index 000000000000..a998028896ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -0,0 +1,136 @@
> +Qualcomm Technologies, Inc. DPU KMS
> +
> +Description:
> +
> +Device tree bindings for MSM Mobile Display Subsytem(MDSS) that encapsulates
> +sub-blocks like DPU display controller, DSI and DP interfaces etc.
> +The DPU display controller is found in SDM845 SoC.
> +
> +MDSS:
> +Required properties:
> +- compatible: "qcom,sdm845-mdss"
> +- reg: physical base address and length of contoller's registers.
> +- reg-names: register region names. The following region is required:
> +  * "mdss_phys"

phys as in physical address? If so, that's always the case. *-names is 
pointless when there is only one anyways.

> +- power-domains: a power domain consumer specifier according to
> +  Documentation/devicetree/bindings/power/power_domain.txt
> +- clocks: list of phandles for clock device nodes needed by the device.
> +- clock-names: device clock names, must be in same order as clocks property.
> +  The following clocks are required:
> +  * "iface"
> +  * "bus"
> +  * "core"
> +- interrupts: interrupt signal from MDSS.
> +- interrupt-controller: identifies the node as an interrupt controller.
> +- #interrupt-cells: specifies the number of cells needed to encode an interrupt
> +  source, should be 1.
> +- iommus: phandle of iommu device node.
> +- #address-cells: number of address cells for the MDSS children. Should be 1.
> +- #size-cells: Should be 1.
> +- ranges: parent bus address space is the same as the child bus address space.
> +
> +Optional properties:
> +- assigned-clocks: list of phandles for clock device nodes needing rate

It's a list of clock specifiers (phandle and id), not device nodes.

> +		   assignment
> +- assigned-clock-rates: list of clock frequencies sorted in the same order as
> +  the assigned-clocks property.
> +
> +MDP:
> +Required properties:
> +- compatible: "qcom,sdm845-dpu"
> +- reg: physical base address and length of controller's registers.
> +- reg-names : register region names. The following region is required:
> +  * "mdp_phys"
> +  * "vbif_phys"

Same comment on "_phys" here.

> +- clocks: list of phandles for clock device nodes needed by the device.
> +- clock-names: device clock names, must be in same order as clocks property.
> +  The following clocks are required.
> +  * "bus"
> +  * "iface"
> +  * "core"
> +  * "vsync"
> +- interrupt-parent: phandle to MDSS block.

Actually, you don't need this if a parent node contains 
"interrupt-controller" property.

> +- interrupts: interrupt line from DPU to MDSS.
> +- ports: contains the list of output ports from DPU device. These ports connect
> +  to interfaces that are external to the DPU hardware, such as DSI, DP etc.
> +
> +  Each output port contains an endpoint that describes how it is connected to an
> +  external interface. These are described by the standard properties documented
> +  here:
> +	Documentation/devicetree/bindings/graph.txt
> +	Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +	Port 0 -> DPU_INTF1 (DSI1)
> +	Port 1 -> DPU_INTF2 (DSI2)
> +
> +Optional properties:
> +- assigned-clocks: list of phandles for clock device nodes needing rate
> +		   assignment
> +- assigned-clock-rates: list of clock frequencies sorted in the same order as
> +  the assigned-clocks property.
> +
> +Example:
> +
> +	mdss: mdss@ae00000 {
> +		compatible = "qcom,sdm845-mdss";
> +		reg = <0xae00000 0x1000>;
> +		reg-names = "mdss_phys";
> +
> +		power-domains = <&clock_dispcc 0>;
> +
> +		clocks = <&gcc GCC_DISP_AHB_CLK>, <&gcc GCC_DISP_AXI_CLK>,
> +			 <&clock_dispcc DISP_CC_MDSS_MDP_CLK>;
> +		clock-names = "iface", "bus", "core";
> +
> +		assigned-clocks = <&clock_dispcc DISP_CC_MDSS_MDP_CLK>;
> +		assigned-clock-rates = <300000000>;
> +
> +		interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		iommus = <&apps_iommu 0>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;

It's preferred to put actual values in and limit the range of child 
nodes. Is there anything in 0xae90000-0xaeaffff?

> +
> +		mdss_mdp: mdp@ae01000 {
> +			compatible = "qcom,sdm845-dpu";
> +			reg = <0x0ae01000 0x8f000>,
> +			      <0x0aeb0000 0x2008>;
> +			reg-names = "mdp_phys", "vbif_phys";
> +
> +			clocks = <&clock_dispcc DISP_CC_MDSS_AHB_CLK>,
> +				 <&clock_dispcc DISP_CC_MDSS_AXI_CLK>,
> +				 <&clock_dispcc DISP_CC_MDSS_MDP_CLK>,
> +				 <&clock_dispcc DISP_CC_MDSS_VSYNC_CLK>;
> +			clock-names = "iface", "bus", "core", "vsync";
> +
> +			assigned-clocks = <&clock_dispcc DISP_CC_MDSS_MDP_CLK>,
> +					  <&clock_dispcc DISP_CC_MDSS_VSYNC_CLK>;
> +			assigned-clock-rates = <0 0 300000000 19200000>;
> +
> +			interrupt-parent = <&mdss>;
> +			interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +					dpu_intf1_out: endpoint {
> +						remote-endpoint = <&dsi0_in>;
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +					dpu_intf2_out: endpoint {
> +						remote-endpoint = <&dsi1_in>;
> +					};
> +				};
> +			};
> +		};
> +	};
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno