Re: [PATCH v4 08/10] arm64: dts: qcom: Add initial support for MSM8917

[Stephan Gerhold <stephan.gerhold@xxxxxxxxxx>]

On Sat, Nov 09, 2024 at 01:08:10PM +0100, Barnabás Czémán wrote:
> From: Otto Pflüger <otto.pflueger@xxxxxxxxx>
> 
> Add initial support for MSM8917 SoC.
> 
> Signed-off-by: Otto Pflüger <otto.pflueger@xxxxxxxxx>
> [reword commit, rebase, fix schema errors]
> Signed-off-by: Barnabás Czémán <barnabas.czeman@xxxxxxxxxxxxxx>
> ---
>  arch/arm64/boot/dts/qcom/msm8917.dtsi | 1997 +++++++++++++++++++++++++++++++++
>  1 file changed, 1997 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8917.dtsi b/arch/arm64/boot/dts/qcom/msm8917.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..f866843772684c695069debfc764f7a0a58843bb
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8917.dtsi
> @@ -0,0 +1,1997 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <dt-bindings/clock/qcom,gcc-msm8917.h>
> +#include <dt-bindings/clock/qcom,rpmcc.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
> +#include <dt-bindings/soc/qcom,apr.h>
> +#include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
> +#include <dt-bindings/thermal/thermal.h>
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		mmc0 = &sdhc_1; /* SDC1 eMMC slot */
> +		mmc1 = &sdhc_2; /* SDC2 SD card slot */
> +	};

I think we put aliases in each board separately nowadays, see e.g.
commit 154f23a8d70c ("arm64: dts: qcom: msm8916: Move aliases to
boards").

> [...]
> +		domain-idle-states {
> +			cluster_sleep_0: cluster-sleep-0 {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x41000023>;
> +				entry-latency-us = <700>;
> +				exit-latency-us = <650>;
> +				min-residency-us = <1972>;
> +			};
> +
> +			cluster_sleep_1: cluster-sleep-1 {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x41000043>;
> +				entry-latency-us = <240>;
> +				exit-latency-us = <280>;
> +				min-residency-us = <806>;
> +			};

This is strange, the deeper sleep state has lower timings than the
previous one?

> +
> +			cluster_sleep_2: cluster-sleep-2 {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x41000053>;
> +				entry-latency-us = <700>;
> +				exit-latency-us = <1000>;
> +				min-residency-us = <6500>;
> +			};
> +		};
> [...]
> +
> +	rpm: remoteproc {
> +		compatible = "qcom,msm8917-rpm-proc", "qcom,rpm-proc";
> +
> +		smd-edge {
> +			interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
> +			qcom,ipc = <&apcs 8 0>;

Can you use mboxes here?

> +			qcom,smd-edge = <15>;
> +
> [...]
> +
> +		mpss_mem: mpss@86800000 {
> +			/*
> +			 * The memory region for the mpss firmware is generally
> +			 * relocatable and could be allocated dynamically.
> +			 * However, many firmware versions tend to fail when
> +			 * loaded to some special addresses, so it is hard to
> +			 * define reliable alloc-ranges.
> +			 *
> +			 * alignment = <0x0 0x400000>;
> +			 * alloc-ranges = <0x0 0x86800000 0x0 0x8000000>;
> +			 */

Not sure how many devices you have access to, but have you tried if this
is actually still the case? I'd have hoped they fixed those issues in
the firmware.

Thanks for using alloc-ranges instead of fixed addresses BTW :)

> +			reg = <0x0 0x86800000 0x0 0>; /* size is device-specific */
> +			no-map;
> +			status = "disabled";
> +		};
> +
> [...]
> +	smp2p-adsp {
> +		compatible = "qcom,smp2p";
> +		qcom,smem = <443>, <429>;
> +
> +		interrupts = <GIC_SPI 291 IRQ_TYPE_EDGE_RISING>;
> +
> +		mboxes = <&apcs 10>;
> +
> +		qcom,local-pid = <0>;
> +		qcom,remote-pid = <2>;
> +
> +		adsp_smp2p_out: master-kernel {
> +			qcom,entry-name = "master-kernel";
> +
> +			#qcom,smem-state-cells = <1>;
> +		};
> +
> +		adsp_smp2p_in: slave-kernel {
> +			qcom,entry-name = "slave-kernel";
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> +
> +	smp2p-modem {
> +		compatible = "qcom,smp2p";
> +		qcom,smem = <435>, <428>;
> +
> +		interrupts = <GIC_SPI 27 IRQ_TYPE_EDGE_RISING>;
> +
> +		qcom,ipc = <&apcs 8 14>;

You have mboxes for adsp above, what about modem and

> +
> +		qcom,local-pid = <0>;
> +		qcom,remote-pid = <1>;
> +
> +		modem_smp2p_out: master-kernel {
> +			qcom,entry-name = "master-kernel";
> +
> +			#qcom,smem-state-cells = <1>;
> +		};
> +
> +		modem_smp2p_in: slave-kernel {
> +			qcom,entry-name = "slave-kernel";
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> +
> +	smp2p-wcnss {
> +		compatible = "qcom,smp2p";
> +		qcom,smem = <451>, <431>;
> +
> +		interrupts = <GIC_SPI 143 IRQ_TYPE_EDGE_RISING>;
> +
> +		qcom,ipc = <&apcs 8 18>;

... wcnss?

> +
> +		qcom,local-pid = <0>;
> +		qcom,remote-pid = <4>;
> +
> +		wcnss_smp2p_out: master-kernel {
> +			qcom,entry-name = "master-kernel";
> +
> +			#qcom,smem-state-cells = <1>;
> +		};
> +
> +		wcnss_smp2p_in: slave-kernel {
> +			qcom,entry-name = "slave-kernel";
> +
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> +
> [...]
> +
> +		restart@4ab000 {
> +			compatible = "qcom,pshold";
> +			reg = <0x004ab000 0x4>;
> +		};

You have PSCI for shutting down, do you actually need this?

> [...]
> +		blsp_i2c4: i2c@78b8000 {
> +			compatible = "qcom,i2c-qup-v2.2.1";
> +			reg = <0x078b8000 0x500>;
> +			interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_BLSP1_QUP4_I2C_APPS_CLK>,
> +				 <&gcc GCC_BLSP1_AHB_CLK>;
> +			clock-names = "core", "iface";
> +			dmas = <&blsp1_dma 10>, <&blsp1_dma 11>;
> +			dma-names = "tx", "rx";
> +			pinctrl-0 = <&i2c4_default>;
> +			pinctrl-1 = <&i2c4_sleep>;
> +			pinctrl-names = "default", "sleep";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		blsp_i2c5: i2c@7af5000 {

Please use a full label here with the BLSP number and QUP instance
(&blspX_i2cY). This corresponds to the name of the clock, so this node
is actually

> +			compatible = "qcom,i2c-qup-v2.2.1";
> +			reg = <0x07af5000 0x600>;
> +			interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_BLSP2_QUP1_I2C_APPS_CLK>,

... &blsp2_i2c1.

I guess the current naming is taken from downstream, but I think they
just assigned consecutive numbers to all the QUP instances they needed.
This will cause headaches in the future if someone wants to add an
instance that wasn't used by QC in the reference designs.

> +				 <&gcc GCC_BLSP2_AHB_CLK>;
> +			clock-names = "core", "iface";
> +			dmas = <&blsp2_dma 4>, <&blsp2_dma 5>;
> +			dma-names = "tx", "rx";
> +			pinctrl-0 = <&i2c5_default>;
> +			pinctrl-1 = <&i2c5_sleep>;

&blsp2_i2c1_default/sleep for clarity

> +			pinctrl-names = "default", "sleep";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		blsp_spi6: spi@7af6000 {

&blsp2_spi2

> +			compatible = "qcom,spi-qup-v2.2.1";
> +			reg = <0x07af6000 0x600>;
> +			interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>,
> +				 <&gcc GCC_BLSP2_AHB_CLK>;
> +			clock-names = "core", "iface";
> +			dmas = <&blsp2_dma 6>, <&blsp2_dma 7>;
> +			dma-names = "tx", "rx";
> +			pinctrl-0 = <&spi6_default>;
> +			pinctrl-1 = <&spi6_sleep>;

&blsp2_spi2_default/sleep

> +			pinctrl-names = "default", "sleep";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
> [...]
> +		timer@b120000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			compatible = "arm,armv7-timer-mem";
> +			reg = <0x0b120000 0x1000>;
> +			clock-frequency = <19200000>;

Should be unneeded unless the firmware is broken. Can you try dropping
it and see if you get a warning in dmesg?

> [...]
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> +		clock-frequency = <19200000>;

Same here.

Thanks,
Stephan