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

barnabas.czeman
2025-04-17T06:20:38Z

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@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>
>> ---
>
> [...]
>
>> + power-domains = <&cpu_pd0>;
>> + power-domain-names = "psci";
>
> So CPU4-7 get "nicer" idle, but 0-3 don't?
>
> [...]
>
>> + 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
There are no speedbins for 8937 gpu. 8940 have but 8940 is not scope of
this series.
>
> [...]
>
>> + 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?
>
> [...]
>
>> + thermal_zones: thermal-zones {
>> + aoss-thermal {
>> + polling-delay-passive = <250>;
>
> There are no passive trip points> +
>> + 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
>
>> +
>> + 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