Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] arm64: dts: qcom: msm8952: broken CPU thermal cooling and L2 critical trip - [High] arm64: dts: qcom: msm8952: MPM pin 53 mapping collides with GPIO 62 and MDSS - [High] arm64: dts: qcom: msm8952: missing bam_clk in crypto_dma and usb_dma nodes - [High] arm64: dts: qcom: msm8952: missing TBU clocks in mdss_mdp and gpu_iommu - [Medium] arm64: dts: qcom: msm8952: undersized rpm_msg_ram master-stats SRAM regions - [Medium] arm64: dts: qcom: msm8952: smsm hexagon@1 mixes Modem reg with ADSP IRQ --
commit 3b6974623411799bae0925eac2cbe5847d66336d Author: Muzaffer Kadir <[email protected]> arm64: dts: qcom: Add initial support for MSM8952 This commit adds the initial device tree source include file for the Qualcomm MSM8952 SoC, including support for CPUs, RPM, MPM, master-stats, SMP2P, SMSM, QFPROM, TSENS, TLMM, GCC, MDSS, GPU, IOMMU, SDHCI, BLSP, USB, and ADSP. > diff --git a/arch/arm64/boot/dts/qcom/msm8952.dtsi > b/arch/arm64/boot/dts/qcom/msm8952.dtsi > new file mode 100644 > index 0000000000000..a7180d694fc76 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/msm8952.dtsi > @@ -0,0 +1,2884 @@ [ ... ] > + rpm: remoteproc { > + compatible = "qcom,msm8952-rpm-proc", > + "qcom,rpm-proc"; > + > + mpm: interrupt-controller { > + compatible = "qcom,mpm"; > + > + interrupts = <GIC_SPI 171 IRQ_TYPE_EDGE_RISING>; > + > + mboxes = <&apcs1 1>; > + interrupt-controller; > + #interrupt-cells = <2>; > + #power-domain-cells = <0>; > + interrupt-parent = <&intc>; > + qcom,rpm-msg-ram = <&apss_mpm>; > + qcom,mpm-pin-count = <64>; > + qcom,mpm-pin-map = <2 184>, /* TSENS UPLOW */ > + <49 140>, /* USB ASYNC */ > + <53 72>, /* MDSS */ > + <58 134>, /* USB */ > + <62 190>; /* SPMI */ > + }; [Severity: High] Does including <53 72> in qcom,mpm-pin-map under mpm in arch/arm64/boot/dts/qcom/msm8952.dtsi cause an interrupt allocation conflict between TLMM GPIO 62 and mdss? In drivers/pinctrl/qcom/pinctrl-msm8952.c, msm8952_mpm_map maps GPIO 62 to MPM wakeup pin 53: drivers/pinctrl/qcom/pinctrl-msm8952.c: static const struct msm_gpio_wakeirq_map msm8952_mpm_map[] = { ... { 45, 24 }, { 46, 25 }, { 48, 26 }, { 54, 14 }, { 58, 17 }, { 62, 53 }, When a consumer requests an interrupt on GPIO 62, msm_gpio_irq_domain_alloc() -> qcom_mpm_alloc() looks up MPM pin 53 via get_mpm_gic_map(): drivers/irqchip/irq-qcom-mpm.c:qcom_mpm_alloc() { ... map = get_mpm_gic_map(priv, pin); if (map == NULL) return irq_domain_disconnect_hierarchy(domain->parent, virq); ... return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &fwspec); } For TLMM wakeup GPIOs, get_mpm_gic_map() needs to return NULL so that qcom_mpm_alloc() disconnects the GIC parent hierarchy, since GPIO interrupts are routed through the TLMM summary interrupt (GIC_SPI 208). With <53 72> present in qcom,mpm-pin-map while mdss connects directly to &intc on GIC_SPI 72, requesting an interrupt on GPIO 62 will instead attempt to allocate GIC_SPI 72 in the GIC domain as its parent interrupt, colliding with mdss. [ ... ] > + smsm { > + compatible = "qcom,smsm"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + mboxes = <0>, > + <&apcs1 13>, > + <&apcs1 9>, > + <&apcs1 19>; > + > + apps_smsm: apps@0 { > + reg = <0>; > + > + #qcom,smem-state-cells = <1>; > + }; > + > + hexagon_smsm: hexagon@1 { > + reg = <1>; > + interrupts = <GIC_SPI 290 IRQ_TYPE_EDGE_RISING>; > + > + interrupt-controller; > + #interrupt-cells = <2>; > + }; [Severity: Medium] In the smsm node in arch/arm64/boot/dts/qcom/msm8952.dtsi, does hexagon_smsm: hexagon@1 mix the Modem SMSM state entry index with the ADSP SMSM interrupt? In qcom_smsm_probe() in drivers/soc/qcom/smsm.c, each child node's reg property selects the SMSM state entry and subscription row: drivers/soc/qcom/smsm.c:qcom_smsm_probe() { ... ret = of_property_read_u32(node, "reg", &id); ... entry = &smsm->entries[id]; entry->smsm = smsm; entry->remote_state = states + id; } Here mboxes lists host 0 (APPS), host 1 (Modem/MPSS on <&apcs1 13>), host 2 (ADSP/LPASS on <&apcs1 9>), and host 3 (WCNSS on <&apcs1 19>). Entry 1 is Modem (whose SMSM interrupt is GIC_SPI 26), whereas GIC_SPI 290 is the ADSP SMSM interrupt corresponding to entry 2. Pairing reg = <1> with GIC_SPI 290 causes smsm_intr() to read Modem's state word (states + 1) when the ADSP SMSM interrupt fires, while leaving Modem's SMSM interrupt unhandled. [ ... ] > + rpm_msg_ram: sram@60000 { > + compatible = "qcom,rpm-msg-ram", > + "mmio-sram"; > + reg = <0x00060000 0x8000>; > + > + ranges = <0 0x00060000 0x8000>; > + #address-cells = <1>; > + #size-cells = <1>; > + > + apss_master_stats: sram@150 { > + reg = <0x150 0x14>; > + }; > + > + apss_mpm: sram@1d0 { > + reg = <0x1d0 0x64>; > + }; > + > + mpss_master_stats: sram@1150 { > + reg = <0x1150 0x14>; > + }; > + > + pronto_master_stats: sram@2550 { > + reg = <0x2550 0x14>; > + }; > + > + tz_master_stats: sram@3550 { > + reg = <0x3550 0x14>; > + }; > + > + lpass_master_stats: sram@4550 { > + reg = <0x4550 0x14>; > + }; > + }; [Severity: Medium] Should the reg sizes for apss_master_stats, mpss_master_stats, pronto_master_stats, tz_master_stats, and lpass_master_stats under rpm_msg_ram in arch/arm64/boot/dts/qcom/msm8952.dtsi be 0x50 (80 bytes) instead of 0x14 (20 bytes)? In drivers/soc/qcom/rpm_master_stats.c, master_stats_probe() maps each region using resource_size(&res), and master_stats_show() reads sizeof(struct rpm_master_stats), which is 80 (0x50) bytes (20 32-bit words): drivers/soc/qcom/rpm_master_stats.c:master_stats_show() { struct master_stats_data *data = s->private; struct rpm_master_stats stat; memcpy_fromio(&stat, data->base, sizeof(stat)); } With 0x14 specified in reg, master_stats_show() reads 60 bytes past the end of the 20-byte ioremapped resource, and sram_reserve_regions() in drivers/misc/sram.c leaves the remaining 60 bytes of each master-stats block unreserved in the allocatable mmio-sram pool. [ ... ] > + crypto_dma: dma-controller@704000 { > + compatible = "qcom,bam-v1.7.0"; > + reg = <0x00704000 0x20000>; > + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + num-channels = <16>; > + qcom,ee = <1>; > + qcom,num-ees = <4>; > + qcom,controlled-remotely; > + }; [Severity: High] What keeps GCC_CRYPTO_AHB_CLK enabled when bam_dma accesses registers in crypto_dma: dma-controller@704000 (and similarly GCC_USB_HS_AHB_CLK in usb_dma: dma-controller@78c4000) in arch/arm64/boot/dts/qcom/msm8952.dtsi? Because crypto_dma sets qcom,controlled-remotely and omits clocks = <&gcc GCC_CRYPTO_AHB_CLK> and clock-names = "bam_clk", bam_dma_probe() sets bdev->bamclk = NULL. When crypto: crypto@73a000 probes in qce_crypto_probe() (drivers/crypto/qce/core.c), it requests crypto_dma channels 2 and 3 via devm_qce_dma_request() and enables runtime PM autosuspend. Once qce_runtime_suspend() runs after the 100ms autosuspend delay (or on probe failure cleanup), it disables GCC_CRYPTO_AHB_CLK: drivers/crypto/qce/core.c:qce_runtime_suspend() { struct qce_device *qce = dev_get_drvdata(dev); int ret; clk_disable_unprepare(qce->core); clk_disable_unprepare(qce->iface); clk_disable_unprepare(qce->bus); } If the crypto device is later unbound, the qcrypto module is unloaded, or qce_crypto_probe() unwinds after a failure, qce_dma_release() -> dma_release_channel() -> bam_free_chan() -> bam_reset_channel() performs MMIO reads and writes to crypto_dma at 0x00704000 while GCC_CRYPTO_AHB_CLK is gated off, which triggers an unclocked bus access crash. Similarly, usb_dma: dma-controller@78c4000 is enabled without bam_clk while usb: usb@78db000 is disabled, so unbinding usb_dma or unloading bam_dma executes bam_dma_remove() MMIO writes at 0x078c4000 while GCC_USB_HS_AHB_CLK is off. [ ... ] > + mdss_mdp: display-controller@1a01000 { > + compatible = "qcom,msm8952-mdp5", > + "qcom,mdp5"; > + reg = <0x01a01000 0x89000>; > + reg-names = "mdp_phys"; > + > + interrupt-parent = <&mdss>; > + interrupts = <0>; > + > + clocks = <&gcc GCC_MDSS_AHB_CLK>, > + <&gcc GCC_MDSS_AXI_CLK>, > + <&gcc GCC_MDSS_MDP_CLK>, > + <&gcc GCC_MDSS_VSYNC_CLK>; > + clock-names = "iface", > + "bus", > + "core", > + "vsync"; > + > + operating-points-v2 = <&mdp_opp_table>; > + power-domains = <&gcc MDSS_GDSC>; > + > + iommus = <&apps_iommu 0x17>; [Severity: High] Does mdss_mdp: display-controller@1a01000 need <&gcc GCC_MDP_TBU_CLK> ("tbu") and gpu_iommu: iommu@1f08000 need <&gcc GCC_GFX_TBU_CLK> ("tbu") in arch/arm64/boot/dts/qcom/msm8952.dtsi? In drivers/clk/qcom/gcc-msm8952.c, gcc_mdp_tbu_clk and gcc_gfx_tbu_clk are registered as votable branch clocks without CLK_IS_CRITICAL or CLK_IGNORE_UNUSED, so clk_disable_unused() gates them off if unclaimed. For display, mdss_mdp uses iommus = <&apps_iommu 0x17>, and mdp5_dev_probe() in drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c looks up the "tbu" clock to enable in mdp5_enable(): drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:mdp5_dev_probe() { ... get_clk(pdev, &mdp5_kms->lut_clk, "lut", false); get_clk(pdev, &mdp5_kms->tbu_clk, "tbu", false); get_clk(pdev, &mdp5_kms->tbu_rt_clk, "tbu_rt", false); } For the GPU IOMMU, qcom_iommu_device_probe() in drivers/iommu/arm/arm-smmu/qcom_iommu.c requests the "tbu" clock and enables it in qcom_iommu_resume() so that TLB invalidation in qcom_iommu_tlb_sync() does not time out on Adreno 405 SoCs (as described in commit 5bc1cf1466f6 ("iommu/qcom: add optional 'tbu' clock for TLB invalidate")). Without these clocks in mdss_mdp and gpu_iommu, the MDP and GFX TBUs remain unclocked. [ ... ] > + l2-thermal { > + thermal-sensors = <&tsens 4>; > + > + trips { > + l2_alert0: trip-point0 { > + temperature = <50000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + l2_alert1: trip-point1 { > + temperature = <55000>; > + hysteresis = <2000>; > + type = "hot"; > + }; > + > + l2_crit: l2-crit { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "critical"; > + }; > + }; > + }; > + > + cpu4-thermal { > + thermal-sensors = <&tsens 5>; > + > + cooling-maps { > + map0 { > + trip = <&cpu4_alert1>; > + cooling-device = <&cpu4 > THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + > + trips { > + cpu4_alert0: trip-point0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + cpu4_alert1: trip-point1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "hot"; > + }; > + > + cpu4_crit: cpu-crit { > + temperature = <110000>; > + hysteresis = <2000>; > + type = "critical"; > + }; > + }; > + }; > + > + cpu5-thermal { > + thermal-sensors = <&tsens 6>; > + > + cooling-maps { > + map0 { > + trip = <&cpu5_alert1>; > + cooling-device = <&cpu5 > THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; [ ... ] > + cpu0-3-thermal { > + thermal-sensors = <&tsens 9>; > + > + cooling-maps { > + map0 { > + trip = <&cpu0_alert1>; > + cooling-device = <&cpu0 > THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu1 > THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu2 > THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > + <&cpu3 > THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > + }; > + }; > + > + trips { > + cpu0_alert0: trip-point0 { > + temperature = <75000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + cpu0_alert1: trip-point1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "hot"; > + }; [Severity: High] Will passive CPU frequency throttling ever activate in thermal-zones in arch/arm64/boot/dts/qcom/msm8952.dtsi with this configuration, and will l2-thermal shut the system down prematurely at 75C? There appear to be four related issues in thermal-zones: 1. In cpu4-thermal, cpu5-thermal, cpu6-thermal, cpu7-thermal, and cpu0-3-thermal, cooling-maps binds map0 to cpuX_alert1 (type = "hot") rather than cpuX_alert0 (type = "passive"). Thermal governors ignore THERMAL_TRIP_HOT trips when throttling cooling devices: drivers/thermal/gov_step_wise.c:step_wise_manage() { ... for_each_trip_desc(tz, td) { const struct thermal_trip *trip = &td->trip; if (trip->temperature == THERMAL_TEMP_INVALID || trip->type == THERMAL_TRIP_CRITICAL || trip->type == THERMAL_TRIP_HOT) continue; thermal_zone_trip_update(tz, td, td->threshold); } } Because no cooling-maps are attached to the passive trips, CPU frequency throttling will never engage. 2. In cpu5-thermal, cpu6-thermal, and cpu7-thermal, cooling-device only references <&cpu5 ...>, <&cpu6 ...>, and <&cpu7 ...> respectively. Because cpu4..cpu7 share &apcs1 and cpu_opp_table_c1 (opp-shared), of_cpufreq_cooling_register() in drivers/thermal/cpufreq_cooling.c registers a single cooling device for the cluster policy with cdev->np set to of_get_cpu_node(policy->cpu, NULL) (&cpu4). When thermal_of_get_cooling_spec() in drivers/thermal/thermal_of.c checks cooling_spec.np != cdev->np, cpu5-thermal, cpu6-thermal, and cpu7-thermal will fail to bind to the cluster cooling device unless <&cpu4 ...> (or all four cluster CPUs) is included. 3. None of the thermal zones set polling-delay-passive, which leaves tz->passive_delay_jiffies at 0 so monitor_thermal_zone() in drivers/thermal/thermal_core.c does not schedule periodic polling while a passive trip is crossed. 4. In l2-thermal, l2_crit has temperature = <75000> (75C), which is the same temperature where the CPU passive trips start, causing handle_critical_trips() to trigger an emergency system shutdown as soon as the L2 sensor reaches 75C. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261028-msm8952-initial-support-v6-0-e07ddd8bc...@mainlining.org?part=9
