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

[barnabas.czeman@mainlining.org]

On 2024-11-11 11:44, Stephan Gerhold wrote:
> On Sat, Nov 09, 2024 at 01:08:10PM +0100, Barnabás Czémán wrote:
>> From: Otto Pflüger <otto.pflueger@abscue.de>
>> 
>> Add initial support for MSM8917 SoC.
>> 
>> Signed-off-by: Otto Pflüger <otto.pflueger@abscue.de>
>> [reword commit, rebase, fix schema errors]
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
>>  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.
This is come from msm8916.dtsi and it is not used yet because of not 
upstreamed modem patch,
maybe better to remove it, i guess.
> 
> 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