Re: [PATCH v8 09/11] arm64: dts: qcom: Add initial support for MSM8952

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: qcom: msm8952: missing bam_clk on cryptobam and usbbam
- [Medium] arm64: dts: qcom: msm8952: missing polling-delay-passive on CPU thermal zones
--

commit ea3b840e26e24e5872598db9df083561eb669ac2
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

arm64: dts: qcom: Add initial support for MSM8952

This commit adds the initial device tree source file for the Qualcomm
MSM8952 SoC, including CPU clusters, clocks, pinctrl, storage, display,
remoteproc, thermal sensors, USB, and BLSP peripherals.

> diff --git a/arch/arm64/boot/dts/qcom/msm8952.dtsi b/arch/arm64/boot/dts/qcom/msm8952.dtsi
> new file mode 100644
> index 0000000000000..3739d1c25c035
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8952.dtsi
> @@ -0,0 +1,2912 @@

[ ... ]

> +		cryptobam: 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;
> +		};

[ ... ]

> +		usbbam: dma-controller@78c4000 {
> +			compatible = "qcom,bam-v1.7.0";
> +			reg = <0x078c4000 0x17000>;
> +			interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>;
> +			#dma-cells = <1>;
> +			num-channels = <4>;
> +			qcom,num-ees = <4>;
> +			qcom,ee = <0>;
> +			qcom,controlled-remotely;
> +		};

[Severity: High]
As reported by Sashiko on v7 [1], both cryptobam and usbbam are enabled by
default with qcom,controlled-remotely, but omit clocks and
clock-names = "bam_clk". Can this cause unclocked register accesses when
freeing DMA channels or unbinding the BAM driver?

For cryptobam, qce_crypto_probe() in drivers/crypto/qce/core.c enables
runtime PM and calls devm_qce_dma_request(). Once the autosuspend delay
elapses (or if probe fails after requesting DMA), qce_runtime_suspend()
gates off the crypto clocks. When the qce driver is later unbound or
unwinds, devres runs:

qce_dma_release()
  dma_release_channel()
    bam_free_chan()
      pm_runtime_get_sync(bdev->dev);
      ...
      bam_reset_channel(bchan);

Because cryptobam has no bam_clk, bdev->bamclk is null in bam_dma_probe(),
so pm_runtime_get_sync() does not enable the crypto interface clock before
bam_reset_channel() writes to the BAM registers at 0x00704000.

Similarly, usbbam is enabled by default while usb@78db000 has
status = "disabled", so unbinding usbbam runs bam_dma_remove() and writes to
0x078c4000 while the USB interface clock is gated off.

Should cryptobam and usbbam provide clocks and clock-names = "bam_clk"
(similar to blsp1_dma and blsp2_dma), or should usbbam also be marked
disabled until enabled alongside usb?

[ ... ]

> +	thermal-zones {
> +		qdsp-thermal {
> +			thermal-sensors = <&tsens 2>;
> +
> +			trips {
> +				qdsp_alert0: trip-point0 {
> +					temperature = <75000>;
> +					hysteresis = <2000>;
> +					type = "hot";
> +				};
> +			};
> +		};
> +
> +		cpu4-thermal {
> +			thermal-sensors = <&tsens 5>;
> +
> +			cooling-maps {
> +				map0 {
> +					trip = <&cpu4_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 {
> +				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";
> +				};
> +			};
> +		};

[Severity: Medium]
As reported by Sashiko on v7 [2], cpu4-thermal, cpu5-thermal, cpu6-thermal,
cpu7-thermal, and cpu0-3-thermal each define a single passive trip point at
75000 mC mapped to the cluster CPUs, but omit polling-delay-passive. Without
polling-delay-passive, will passive CPU frequency throttling stall after a
single cooling step?

In drivers/thermal/thermal_of.c:thermal_of_monitor_init(), omitting
polling-delay-passive leaves tz->passive_delay_jiffies set to 0:

	ret = of_property_read_u32(np, "polling-delay-passive", pdelay);
	if (ret == -EINVAL) {
		*pdelay = 0;

When a CPU thermal zone crosses 75000 mC, tsens_irq_thread() triggers
__thermal_zone_device_update(), which reprograms the hardware interrupt
window to [72999 mC, 85000 mC]. Because tz->passive_delay_jiffies is 0,
monitor_thermal_zone() in drivers/thermal/thermal_core.c never schedules
polling while the passive trip is active:

	if (tz->passive > 0 && tz->passive_delay_jiffies)
		thermal_zone_device_set_polling(tz, tz->passive_delay_jiffies);

Under the step_wise governor, get_target_state() only advances the cooling
state by cur_state + 1 on that initial trip crossing and never re-evaluates
while the temperature remains between 73 C and 85 C, and under the
power_allocator governor, pid_controller() divides by
jiffies_to_msecs(tz->passive_delay_jiffies) which is 0.

Should these passive CPU thermal zones specify polling-delay-passive (such
as <250>, matching msm8916.dtsi, msm8939.dtsi, and msm8953.dtsi)?

[1] https://lore.kernel.org/r/20261030-msm8952-initial-support-v7-9-ad748ace375d@mainlining.org
[2] https://lore.kernel.org/r/20261030-msm8952-initial-support-v7-9-ad748ace375d@mainlining.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-msm8952-initial-support-v8-0-311ae8e8f14b@mainlining.org?part=9