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

[barnabas.czeman@mainlining.org]

On 2025-02-12 14:07, Konrad Dybcio wrote:
> On 11.02.2025 11:37 PM, Barnabás Czémán wrote:
>> From: Dang Huynh <danct12@riseup.net>
>> 
>> Add initial support for MSM8937 SoC.
>> 
>> Signed-off-by: Dang Huynh <danct12@riseup.net>
>> Co-developed-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
> 
> So the computer tells me 8917 and 8937 are *very* similar. Have you
> tried assessing how making 8937.dtsi an overlay atop 8917.dtsi would
> work out?

They are similar but there are many small differences:
- have two dsi
- using adreno 505
- different iommu it uses arm,smmu for gpu and qcom,iommu for 
applications
- 8 cores
- camss will be a different a bit
- venus will be different a bit
- have more i2c and spi
- different mdp version

Maybe i can find more differences, originally it was based on 
msm8917.dtsi
but we have decided to keep it separate, also it have different license 
from 8917.
The plan is MSM8940 and SDM439 support will based on msm8937.dtsi in the 
future.

> 
> 
>>  arch/arm64/boot/dts/qcom/msm8937.dtsi | 2145 
>> +++++++++++++++++++++++++++++++++
>>  1 file changed, 2145 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/msm8937.dtsi 
>> b/arch/arm64/boot/dts/qcom/msm8937.dtsi
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..ef633c1694ad98165e58130cbeb186d2f0e2dcaa
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/msm8937.dtsi
>> @@ -0,0 +1,2145 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * Copyright (c) 2023, Dang Huynh <danct12@riseup.net>
>> + */
>> +
>> +#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/thermal/thermal.h>
>> +
>> +/ {
>> +	interrupt-parent = <&intc>;
>> +
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	clocks {
>> +		xo_board: xo-board {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		sleep_clk: sleep-clk {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +		};
>> +	};
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		cpu4: cpu@0 {
> 
> I'm pretty sure a CPU with a MPIDR of 0 should be called CPU0
> 
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x0>;
>> +			device_type = "cpu";
>> +			enable-method = "psci";
>> +			operating-points-v2 = <&cpu_opp_table_c0>;
>> +			next-level-cache = <&l2_0>;
>> +			#cooling-cells = <2>;
>> +			l2_0: l2-cache {
> 
> Please add a newline between the last property and the subnode
> 
>> +				compatible = "cache";
>> +				cache-level = <2>;
> 
> cache-size = <0x80000>;
> 
> [...]
> 
>> +		cpu0: cpu@100 {
>> +			compatible = "arm,cortex-a53";
>> +			reg = <0x100>;
>> +			device_type = "cpu";
>> +			next-level-cache = <&l2_1>;
>> +			enable-method = "psci";
>> +			operating-points-v2 = <&cpu_opp_table_c1>;
>> +			#cooling-cells = <2>;
>> +			power-domains = <&cpu_pd0>;
>> +			power-domain-names = "psci";
>> +			l2_1: l2-cache {
>> +				compatible = "cache";
>> +				cache-level = <2>;
> 
> cache-size = <0x100000>;
> 
> I'll do further review if you decide it makes sense to keep this
> separate from 8917
> 
> Konrad