Re: [PATCH v4 4/6] arm64: dts: qcom: Add initial support for MSM8937

[barnabas . czeman]

On 2025-04-14 22:55, Konrad Dybcio wrote:
> On 3/15/25 3:57 PM, Barnabás Czémán wrote:
> > From: Dang Huynh <danct12@xxxxxxxxxx>
> >
> > Add initial support for MSM8937 SoC.
> >
> > Signed-off-by: Dang Huynh <danct12@xxxxxxxxxx>
> > Co-developed-by: Barnabás Czémán <barnabas.czeman@xxxxxxxxxxxxxx>
> > Signed-off-by: Barnabás Czémán <barnabas.czeman@xxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +			power-domains = <&cpu_pd0>;
> > +			power-domain-names = "psci";
>
> So CPU4-7 get "nicer" idle, but 0-3 don't?
I am not sure, i will check.
> [...]
>
> > +		cpu-map {
> > +			/* The MSM8937 has 2 cluster A53 setup. */
>
> This comment seems superfluous
>
> [...]
>
> > +	timer {
>
> 'p' < 't', please sort top-level nodes alphabetically
>
> [...]
>
> > +				wcss-wlan2-pins {
> > +					pins = "gpio76";
> > +					function = "wcss_wlan2";
> > +					drive-strength = <6>;
>
> please unify this order (drive-strength before bias)
>
> > +					bias-pull-up;
> > +
> > +				};
>
> Extra newline
>
> [...]
>
> > +		gpu: gpu@1c00000 {
> > +			compatible = "qcom,adreno-505.0", "qcom,adreno";
> > +			reg = <0x1c00000 0x40000>;
> > +			reg-names = "kgsl_3d0_reg_memory";
> > +			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "kgsl_3d0_irq";
> > +			#cooling-cells = <2>;
> > +			clocks = <&gcc GCC_OXILI_GFX3D_CLK>,
> > +				<&gcc GCC_OXILI_AHB_CLK>,
> > +				<&gcc GCC_BIMC_GFX_CLK>,
> > +				<&gcc GCC_BIMC_GPU_CLK>,
> > +				<&gcc GCC_OXILI_TIMER_CLK>,
> > +				<&gcc GCC_OXILI_AON_CLK>;
>
> Please align the <s
>
> > +			clock-names = "core",
> > +				      "iface",
> > +				      "mem_iface",
> > +				      "alt_mem_iface",
> > +				      "rbbmtimer",
> > +				      "alwayson";
> > +			operating-points-v2 = <&gpu_opp_table>;
> > +			power-domains = <&gcc OXILI_GX_GDSC>;
> > +
> > +			iommus = <&adreno_smmu 0>;
> > +
> > +			status = "disabled";
> > +
> > +			gpu_opp_table: opp-table {
> > +				compatible = "operating-points-v2";
> > +
> > +				opp-19200000 {
> > +					opp-hz = /bits/ 64 <19200000>;
> > +					opp-supported-hw = <0xFF>;
>
> 0xff is overly broad, please document the existing known speed bins
>
> [...]
>
> > +		adreno_smmu: iommu@1c40000 {
> > +			compatible = "qcom,msm8996-smmu-v2",
> > +				     "qcom,adreno-smmu",
> > +				     "qcom,smmu-v2";
> > +			reg = <0x1c40000 0x10000>;
>
> Does it work as-is, without iommu changes?
Yes
> [...]
>
> > +	thermal_zones: thermal-zones {
> > +		aoss-thermal {
> > +			polling-delay-passive = <250>;
>
> There are no passive trip points> +
Should i remove polling-delay-passive?
> > +			thermal-sensors = <&tsens 0>;
> > +
> > +			trips {
> > +				aoss_alert0: trip-point0 {
> > +					temperature = <85000>;
> > +					hysteresis = <2000>;
> > +					type = "hot";
> > +				};
>
> Please convert these to 'critical' instead
>
> [...]
>
> > +		cpuss1-thermal {
> > +			polling-delay-passive = <250>;
>
> You can drop polling-delay-passive under CPU tzones, as threshold
> crossing is interrupt-driven
Should I remove polling-delay-passive then?
> > +
> > +			thermal-sensors = <&tsens 4>;
> > +
> > +			cooling-maps {
> > +				map0 {
> > +					trip = <&cpuss1_alert0>;
> > +					cooling-device = <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +							 <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +							 <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +							 <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +				};
> > +			};
> > +
> > +			trips {
> > +				cpuss1_alert0: trip-point0 {
> > +					temperature = <75000>;
> > +					hysteresis = <2000>;
> > +					type = "passive";
> > +				};
> > +
> > +				cpuss1_alert1: trip-point1 {
> > +					temperature = <85000>;
> > +					hysteresis = <2000>;
> > +					type = "hot";
> > +				};
>
> On newer platforms we rely on LMH to shut down the device if it
> were to reach the junction temperature, but let's leave them here
> as probably no one remembers for sure how reliable that is on these
> older platforms and you're most likely not willing to test that
>
> Konrad