Re: [PATCH v1 2/4] arm64: dts: Add msm8939 SoC

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 18 Apr 20:09 CDT 2022, Bryan O'Donoghue wrote:

> Add msm8939 a derivative SoC of msm8916. This SoC contains a number of key
> differences to msm8916.
> 

Thanks for posting this Bryan, happy to see this being moved upstream,
despite the PSCI shortcomings.

> - big.LITTLE Octa Core - quad 1.5GHz + quad 1.0GHz
> - DRAM 1x800 LPDDR3
> - Camera 4+4 lane CSI
> - Venus @ 1080p60 HEVC
> - DSI x 2
> - Adreno A405
> - WiFi wcn3660/wcn3680b 802.11ac
> 
> Co-developed-by: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Co-developed-by: Jun Nie <jun.nie@linaro.org>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> Co-developed-by: Benjamin Li <benl@squareup.com>
> Signed-off-by: Benjamin Li <benl@squareup.com>
> Co-developed-by: James Willcox <jwillcox@squareup.com>
> Signed-off-by: James Willcox <jwillcox@squareup.com>
> Co-developed-by: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> Co-developed-by: Joseph Gates <jgates@squareup.com>
> Signed-off-by: Joseph Gates <jgates@squareup.com>
> Co-developed-by: Max Chen <mchen@squareup.com>
> Signed-off-by: Max Chen <mchen@squareup.com>
> Co-developed-by: Zac Crosby <zac@squareup.com>
> Signed-off-by: Zac Crosby <zac@squareup.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8939.dtsi | 2017 +++++++++++++++++++++++++
>  1 file changed, 2017 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/msm8939.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8939.dtsi b/arch/arm64/boot/dts/qcom/msm8939.dtsi
> new file mode 100644
> index 000000000000..f1aa79b7d0e9
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8939.dtsi
> @@ -0,0 +1,2017 @@
> +// SPDX-License-Identifier: BSD-3-Clause

Nice

> +/*
> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2020-2022, Linaro Limited
> + *

The reminder of this comment denotes that the file is licensed under
GPL, which should be covered by the SPDX license, but is now in conflict
with it...

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <dt-bindings/interconnect/qcom,msm8939.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/qcom,gcc-msm8939.h>
> +#include <dt-bindings/reset/qcom,gcc-msm8939.h>
> +#include <dt-bindings/clock/qcom,rpmcc.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
> +#include <dt-bindings/thermal/thermal.h>
> +
> +/ {
> +	model = "Qualcomm Technologies, Inc. MSM8939";
> +	compatible = "qcom,msm8939";

It's expected that the board dts will specify both model and
compatible, so these should always be overridden and as such can be
omitted from the dtsi.

> +	qcom,msm-id = <239 0>, <239 0x30000>, <241 0x30000>, <263 0x30000>;
> +
> +	interrupt-parent = <&intc>;
> +
> +	#address-cells = <2>;
> +	#size-cells = <2>;

Why are these 2 when /soc uses 1?

> +
> +	memory {

Please do keep nodes sorted, primarily by address, then alphabetically.

> +		device_type = "memory";
> +		/* We expect the bootloader to fill in the reg */
> +		reg = <0 0 0 0>;
> +	};
[..]
> +	cci_opp_table: cci-opp-table {

I don't see a reference to this, please introduce it together with the
consumer node. And as with the &gpu_opp_table below.

> +		compatible = "operating-points-v2";
> +
> +		opp-200000000 {
> +			opp-hz = /bits/ 64 <200000000>;
> +		};
> +
> +		opp-297600000 {
> +			opp-hz = /bits/ 64 <297600000>;
> +		};
> +
> +		opp-400000000 {
> +			opp-hz = /bits/ 64 <400000000>;
> +		};
> +
> +		opp-595200000 {
> +			opp-hz = /bits/ 64 <595200000>;
> +		};
> +	};
> +
> +	gpu_opp_table: gpu-opp-table {

&gpu_opp_table is tied to the gpu node, so it can with favor be moved
into the gpu node itself - and can then be named simply "opp-table".

> +		compatible = "operating-points-v2";
> +
> +		opp-550000000 {
> +			opp-hz = /bits/ 64 <550000000>;
> +		};
> +
> +		opp-465000000 {
> +			opp-hz = /bits/ 64 <465000000>;
> +		};
> +
> +		opp-400000000 {
> +			opp-hz = /bits/ 64 <400000000>;
> +		};
> +
> +		opp-220000000 {
> +			opp-hz = /bits/ 64 <220000000>;
> +		};
> +
> +		opp-19200000 {
> +			opp-hz = /bits/ 64 <19200000>;
> +		};
> +	};
[..]
> +	clocks {
> +		xo_board: xo_board {

No underscores in node names. If you still rely on string based
parent_names in the clock drivers add a clock-output-names property to
make the name "xo_board".

But preferably you'd switch to use parent_data and fw_name based clock
parent lookup in the clock drivers.

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <19200000>;
> +		};
> +
> +		sleep_clk: sleep_clk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <32768>;
> +		};
> +	};
> +
> +	smem {

Per the binding you may now add the additional properties to &smem_mem
and avoid having this intermediate node connecting the various smem
pieces together.

> +		compatible = "qcom,smem";
> +
> +		memory-region = <&smem_mem>;
> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
> +
> +		hwlocks = <&tcsr_mutex 3>;
> +	};
> +
> +	firmware {
> +		scm: scm {
> +			compatible = "qcom,scm";
> +			clocks = <&gcc GCC_CRYPTO_CLK>,
> +				 <&gcc GCC_CRYPTO_AXI_CLK>,
> +				 <&gcc GCC_CRYPTO_AHB_CLK>;
> +			clock-names = "core", "bus", "iface";
> +			#reset-cells = <1>;
> +
> +			qcom,dload-mode = <&tcsr 0x6100>;
> +		};
> +	};
> +
> +	soc: soc {

This should be soc@0

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0 0xffffffff>;
> +		compatible = "simple-bus";
> +
> +		qfprom_cpr: qfprom_cpr@58000 {

0x58000 is the raw qfprom interface, while the CRC corrected values are
available at 0x5c000. As such, you should quite likely move the cell
definitions to &qfprom and drop this node.

No underscores in node names.

> +			compatible = "qcom,qfprom";
> +			reg = <0x00058000 0x1000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			cpr_efuse_init_voltage1: ivoltage1@dc {
> +				reg = <0xdc 0x4>;
> +				bits = <4 6>;
> +			};
> +			cpr_efuse_init_voltage2: ivoltage2@da {
> +				reg = <0xda 0x4>;
> +				bits = <2 6>;
> +			};
> +			cpr_efuse_init_voltage3: ivoltage3@d8 {
> +				reg = <0xd8 0x4>;
> +				bits = <0 6>;
> +			};
> +			cpr_efuse_quot1: quot1@dc {
> +				reg = <0xdd 0x8>;
> +				bits = <2 12>;
> +			};
> +			cpr_efuse_quot2: quot2@da {
> +				reg = <0xdb 0x8>;
> +				bits = <0x0 12>;
> +			};
> +			cpr_efuse_quot3: quot3@d8 {
> +				reg = <0xd8 0x8>;
> +				bits = <6 12>;
> +			};
> +			cpr_efuse_ring1: ring1@de {
> +				reg = <0xde 0x4>;
> +				bits = <6 3>;
> +			};
> +			cpr_efuse_ring2: ring2@de {
> +				reg = <0xde 0x4>;
> +				bits = <6 3>;
> +			};
> +			cpr_efuse_ring3: ring3@de {
> +				reg = <0xde 0x4>;
> +				bits = <6 3>;
> +			};
> +			cpr_efuse_revision: revision@4 {
> +				reg = <0x5 0x1>;
> +				bits = <5 1>;
> +			};
> +			cpr_efuse_revision_high: revision_high@4 {
> +				reg = <0x7 0x1>;
> +				bits = <0 1>;
> +			};
> +			cpr_efuse_pvs_version: pvs@4 {
> +				reg = <0x3 0x1>;
> +				bits = <5 1>;
> +			};
> +			cpr_efuse_pvs_version_high: pvs_high@4 {
> +				reg = <0x6 0x1>;
> +				bits = <2 2>;
> +			};
> +			cpr_efuse_speedbin: speedbin@c {
> +				reg = <0xc 0x1>;
> +				bits = <2 3>;
> +			};
> +
> +		};
> +
> +		acc0: clock-controller@b088000 {
> +			compatible = "qcom,kpss-acc-v2";
> +			reg = <0xb088000 0x1000>;

If you pad all addresses to 8 digits it's much faster so see if the
nodes are sorted.

And please sort the nodes by address.

> +		};
> +
[..]
> +		tcsr_mutex: hwlock {
> +			compatible = "qcom,tcsr-mutex";

Without a reg this should live outside /soc, but in order to avoid the
dummy tcsr-mutex node just pointing to a nameless syscon the binding has
been changed such that you should move the compatible and #hwlock-cells
to &tcsr_mutex_regs directly.

> +			syscon = <&tcsr_mutex_regs 0 0x1000>;
> +			#hwlock-cells = <1>;
> +		};
> +
> +		rpm_msg_ram: memory@60000 {
> +			compatible = "qcom,rpm-msg-ram";
> +			reg = <0x60000 0x8000>;
> +		};
[..]
> +
> +		lpass: lpass@7708000 {
> +			status = "disabled";

Please move status last in this node.

> +			compatible = "qcom,apq8016-lpass-cpu";
> +			clocks = <&gcc GCC_ULTAUDIO_AHBFABRIC_IXFABRIC_CLK>,
> +				 <&gcc GCC_ULTAUDIO_PCNOC_MPORT_CLK>,
> +				 <&gcc GCC_ULTAUDIO_PCNOC_SWAY_CLK>,
> +				 <&gcc GCC_ULTAUDIO_LPAIF_PRI_I2S_CLK>,
> +				 <&gcc GCC_ULTAUDIO_LPAIF_SEC_I2S_CLK>,
> +				 <&gcc GCC_ULTAUDIO_LPAIF_SEC_I2S_CLK>,
> +				 <&gcc GCC_ULTAUDIO_LPAIF_AUX_I2S_CLK>;
> +
> +			clock-names = "ahbix-clk",
> +					"pcnoc-mport-clk",
> +					"pcnoc-sway-clk",
> +					"mi2s-bit-clk0",
> +					"mi2s-bit-clk1",
> +					"mi2s-bit-clk2",
> +					"mi2s-bit-clk3";
> +			#sound-dai-cells = <1>;
> +
> +			interrupts = <0 160 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "lpass-irq-lpaif";
> +			reg = <0x07708000 0x10000>;
> +			reg-names = "lpass-lpaif";
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
[..]
> +		apps_iommu: iommu@1ef0000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#iommu-cells = <1>;

Please start with compatible, reg, clocks etc used by the node. Then
properties related to what's exposed, followed by the subnodes.

> +			compatible = "qcom,msm8916-iommu", "qcom,msm-iommu-v1";
> +			ranges = <0 0x1e20000 0x40000>;
> +			reg = <0x1ef0000 0x3000>;
> +			clocks = <&gcc GCC_SMMU_CFG_CLK>,
> +				 <&gcc GCC_APSS_TCU_CLK>;
> +			clock-names = "iface", "bus";
> +			qcom,iommu-secure-id = <17>;
> +
> +			/* mdp_0: */
> +			iommu-ctx@4000 {
> +				compatible = "qcom,msm-iommu-v1-ns";
> +				reg = <0x4000 0x1000>;
> +				interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +
> +			/* venus_ns: */
> +			iommu-ctx@5000 {
> +				compatible = "qcom,msm-iommu-v1-sec";
> +				reg = <0x5000 0x1000>;
> +				interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +		};
> +
> +		gpu_iommu: iommu@1f08000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#iommu-cells = <1>;
> +			compatible = "qcom,msm8916-iommu", "qcom,msm-iommu-v1";
> +			ranges = <0 0x1f08000 0x10000>;
> +			clocks = <&gcc GCC_SMMU_CFG_CLK>,
> +				 <&gcc GCC_GFX_TCU_CLK>,
> +				 <&gcc GCC_GFX_TBU_CLK>;
> +			clock-names = "iface", "bus", "tlb";
> +			qcom,iommu-secure-id = <18>;
> +
> +			/* gfx3d_user: */
> +			iommu-ctx@1000 {
> +				compatible = "qcom,msm-iommu-v1-ns";
> +				reg = <0x1000 0x1000>;
> +				interrupts = <GIC_SPI 241 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +
> +			/* gfx3d_priv: */
> +			iommu-ctx@2000 {
> +				compatible = "qcom,msm-iommu-v1-ns";
> +				reg = <0x2000 0x1000>;
> +				interrupts = <GIC_SPI 242 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +		};
> +
> +		gpu@1c00000 {
> +			compatible = "qcom,adreno-405.0", "qcom,adreno";
> +			reg = <0x01c00000 0x10000>;
> +			reg-names = "kgsl_3d0_reg_memory";
> +			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "kgsl_3d0_irq";
> +			clock-names =

Please line break after the first entry, rather than before. Then indent
to match that.

> +			    "core",
> +			    "iface",
> +			    "mem",
> +			    "mem_iface",
> +			    "alt_mem_iface",
> +			    "gfx3d",
> +			    "rbbmtimer";
> +			clocks =
> +			    <&gcc GCC_OXILI_GFX3D_CLK>,
> +			    <&gcc GCC_OXILI_AHB_CLK>,
> +			    <&gcc GCC_OXILI_GMEM_CLK>,
> +			    <&gcc GCC_BIMC_GFX_CLK>,
> +			    <&gcc GCC_BIMC_GPU_CLK>,
> +			    <&gcc GFX3D_CLK_SRC>,
> +			    <&gcc GCC_OXILI_TIMER_CLK>;
> +			power-domains = <&gcc OXILI_GDSC>;
> +			operating-points-v2 = <&gpu_opp_table>;
> +			iommus = <&gpu_iommu 1>, <&gpu_iommu 2>;
> +		};
> +
> +		mdss: mdss@1a00000 {
> +			compatible = "qcom,mdss";
> +			reg = <0x1a00000 0x1000>,
> +			      <0x1ac8000 0x3000>;
> +			reg-names = "mdss_phys", "vbif_phys";
> +
> +			power-domains = <&gcc MDSS_GDSC>;
> +
> +			clocks = <&gcc GCC_MDSS_AHB_CLK>,
> +				 <&gcc GCC_MDSS_AXI_CLK>,
> +				 <&gcc GCC_MDSS_VSYNC_CLK>;
> +			clock-names = "iface",
> +				      "bus",
> +				      "vsync";
> +
> +			interrupts = <0 72 IRQ_TYPE_LEVEL_HIGH>;

s/0/GIT_SPI/

> +
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			mdp: mdp@1a01000 {

If you label this mdss_mdp: instead the nodes would group nicely in the
board dts as references there should be sorted by label.

> +				compatible = "qcom,mdp5";
> +				reg = <0x1a01000 0x89000>;
> +				reg-names = "mdp_phys";
> +
> +				interrupt-parent = <&mdss>;
> +				interrupts = <0 0>;

&mdss is #interrupt-cells = <1>, so the second 0 should go.

> +
> +				clocks = <&gcc GCC_MDSS_AHB_CLK>,
> +					 <&gcc GCC_MDSS_AXI_CLK>,
> +					 <&gcc GCC_MDSS_MDP_CLK>,
> +					 <&gcc GCC_MDSS_VSYNC_CLK>,
> +					 <&gcc GCC_MDP_TBU_CLK>,
> +					 <&gcc GCC_MDP_RT_TBU_CLK>;
> +				clock-names = "iface",
> +					      "bus",
> +					      "core",
> +					      "vsync",
> +					      "tbu",
> +					      "tbu_rt";
> +
> +				iommus = <&apps_iommu 4>;
> +
> +				interconnects = <&snoc_mm MASTER_MDP_PORT0 &bimc SLAVE_EBI_CH0>,
> +						<&snoc_mm MASTER_MDP_PORT1 &bimc SLAVE_EBI_CH0>;
> +				interconnect-names = "mdp0-mem", "mdp1-mem";
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					port@0 {
> +						reg = <0>;
> +						mdp5_intf1_out: endpoint {
> +							remote-endpoint = <&dsi0_in>;
> +						};
> +					};
> +
> +					port@1 {
> +						reg = <1>;
> +						mdp5_intf2_out: endpoint {
> +							remote-endpoint = <&dsi1_in>;
> +						};
> +					};
> +				};
> +			};
> +
> +			dsi0: dsi@1a98000 {

As above, mdss_dsi0:

> +				compatible = "qcom,mdss-dsi-ctrl";
> +				reg = <0x1a98000 0x25c>;
> +				reg-names = "dsi_ctrl";
> +
> +				interrupt-parent = <&mdss>;
> +				interrupts = <4 0>;

Same as &mdp.

> +
> +				assigned-clocks = <&gcc BYTE0_CLK_SRC>,
> +						  <&gcc PCLK0_CLK_SRC>;
> +				assigned-clock-parents = <&dsi0_phy 0>,
> +							 <&dsi0_phy 1>;
> +
> +				clocks = <&gcc GCC_MDSS_MDP_CLK>,
> +					 <&gcc GCC_MDSS_AHB_CLK>,
> +					 <&gcc GCC_MDSS_AXI_CLK>,
> +					 <&gcc GCC_MDSS_BYTE0_CLK>,
> +					 <&gcc GCC_MDSS_PCLK0_CLK>,
> +					 <&gcc GCC_MDSS_ESC0_CLK>;
> +				clock-names = "mdp_core",
> +					      "iface",
> +					      "bus",
> +					      "byte",
> +					      "pixel",
> +					      "core";
> +				phys = <&dsi0_phy>;
> +				phy-names = "dsi-phy";
> +				status = "disabled";
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					port@0 {
> +						reg = <0>;
> +						dsi0_in: endpoint {
> +							remote-endpoint = <&mdp5_intf1_out>;
> +						};
> +					};
> +
> +					port@1 {
> +						reg = <1>;
> +						dsi0_out: endpoint {
> +						};
> +					};
> +				};
> +			};
> +
> +			dsi1: dsi@1aa0000 {

mdss_dsi1:

> +				compatible = "qcom,mdss-dsi-ctrl";
> +				reg = <0x1aa0000 0x25c>;
> +				reg-names = "dsi_ctrl";
> +
> +				interrupt-parent = <&mdss>;
> +				interrupts = <5 0>;
> +
> +				assigned-clocks = <&gcc BYTE1_CLK_SRC>,
> +						  <&gcc PCLK1_CLK_SRC>;
> +				assigned-clock-parents = <&dsi1_phy 0>,
> +							 <&dsi1_phy 1>;
> +
[..]
> +
> +		hexagon@4080000 {

This is disabled but doesn't have a label.

remoteproc_mss: remoteproc@4080000

seems reasonable.

> +			compatible = "qcom,q6v5-pil";
> +			reg = <0x04080000 0x100>,
> +			      <0x04020000 0x040>;
> +
> +			reg-names = "qdsp6", "rmb";
> +
[..]
> +
> +		pronto: wcnss@a204000 {

remoteproc_pronto: remoteproc@a204000

> +			compatible = "qcom,pronto-v2-pil", "qcom,pronto";
> +			reg = <0x0a204000 0x2000>, <0x0a202000 0x1000>, <0x0a21b000 0x3000>;
> +			reg-names = "ccu", "dxe", "pmu";
> +
> +			memory-region = <&wcnss_mem>;
[..]
> +
> +#include "msm8916-pins.dtsi"

I thought we got rid of the separate *-pins.dtsi file. I'm also worried
about sharing this between 8916 and 8939. Can you please inline this in
line with one of the newer platform.

Regards,
Bjorn