Re: [v1] arm64: dts: sc7180: add display dt nodes
From: Stephen Boyd
Date: Tue Jan 21 2020 - 18:16:23 EST
Quoting Harigovindan P (2020-01-21 07:52:08)
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> old mode 100644
> new mode 100755
> index 8011c5f..963f5c1
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -1151,6 +1151,131 @@
> };
> };
>
> + mdss: mdss@ae00000 {
Is there a better node name for this? display-subsystem perhaps?
> + compatible = "qcom,sc7180-mdss";
> + reg = <0 0x0ae00000 0 0x1000>;
> + reg-names = "mdss";
> +
> + power-domains = <&dispcc MDSS_GDSC>;
> +
> + clocks = <&gcc GCC_DISP_AHB_CLK>,
> + <&gcc GCC_DISP_HF_AXI_CLK>,
> + <&dispcc DISP_CC_MDSS_MDP_CLK>;
> + clock-names = "iface", "gcc_bus", "core";
> +
> + assigned-clocks = <&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_smmu 0x800 0x2>;
> +
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + mdss_mdp: mdp@ae01000 {
Is there a better node name for this? display-controller perhaps? Also,
first reg property is supposed to be the one after the @ sign. In this
case that would be ae00000.
> + compatible = "qcom,sc7180-dpu";
> + reg = <0 0x0ae00000 0 0x1000>,
> + <0 0x0ae01000 0 0x8f000>,
> + <0 0x0aeb0000 0 0x2008>,
> + <0 0x0af03000 0 0x16>;
> + reg-names = "mdss","mdp", "vbif", "disp_cc";
^
Nitpick: Add a space here after the comma.
> +
> + clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> + <&dispcc DISP_CC_MDSS_ROT_CLK>,
> + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>,
> + <&dispcc DISP_CC_MDSS_MDP_CLK>,
> + <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
> + clock-names = "iface", "rot", "lut", "core",
> + "vsync";
Nitpick: Tabbing seems weird here. The clocks property is aligned but
not the clock-names.
> + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> + <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
> + assigned-clock-rates = <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>;
> + };
> + };
> + };
> + };
> +
> + dsi0: qcom,mdss_dsi_ctrl0@ae94000 {
Is there a better node name for this? dsi-controller perhaps?
> + compatible = "qcom,mdss-dsi-ctrl";
> + reg = <0 0x0ae94000 0 0x400>;
> + reg-names = "dsi_ctrl";
> +
> + interrupt-parent = <&mdss>;
> + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
> +
> + clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>,
> + <&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>,
> + <&dispcc DISP_CC_MDSS_PCLK0_CLK>,
> + <&dispcc DISP_CC_MDSS_ESC0_CLK>,
> + <&dispcc DISP_CC_MDSS_AHB_CLK>,
> + <&gcc GCC_DISP_HF_AXI_CLK>;
> + clock-names = "byte",
> + "byte_intf",
> + "pixel",
> + "core",
> + "iface",
> + "bus";
Nitpick: Tabbing is all of here too.
> +
> + phys = <&dsi0_phy>;
> + phy-names = "dsi";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + dsi0_in: endpoint {
> + remote-endpoint = <&dpu_intf1_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + dsi0_out: endpoint {
> + };
> + };
> + };
> + };
> +
> + dsi0_phy: dsi-phy0@ae94400 {
Just call it 'dsi-phy' or 'phy' please. The address differentiates it and
the phandle can call it 0.
> + compatible = "qcom,dsi-phy-10nm";
> + reg = <0 0x0ae94400 0 0x200>,
> + <0 0x0ae94600 0 0x280>,
> + <0 0x0ae94a00 0 0x1e0>;
> + reg-names = "dsi_phy",
> + "dsi_phy_lane",
> + "dsi_pll";
> +
> + #clock-cells = <1>;
> + #phy-cells = <0>;
> +
> + clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>;
Do you need the XO or reference clk here too? So that the PLL can generate a clk with
the reference clk?
> + clock-names = "iface";
> +
Nitpick: Why the extra newline? Please remove.
> + };
> + };
> +
> pdc: interrupt-controller@b220000 {
> compatible = "qcom,sc7180-pdc", "qcom,pdc";
> reg = <0 0x0b220000 0 0x30000>;