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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] arm64: dts: qcom: msm8952: CPU cooling-maps bound to hot trips
- [High] arm64: dts: qcom: msm8952: MPM pin 53 collision between mdss and gpio62
- [High] arm64: dts: qcom: msm8952: missing bam_clk on cryptobam and usbbam
- [High] arm64: dts: qcom: msm8952: missing TBU clocks on mdss_mdp and gpu_iommu
--

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

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, QFPROM,
TSENS, TLMM, GCC, MDSS, IOMMUs, 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..ee932f6b2a9c8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8952.dtsi
> @@ -0,0 +1,2909 @@

[ ... ]

> +	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 mapping MPM pin 53 to GIC SPI 72 in qcom,mpm-pin-map (and routing
mdss through <&mpm 53 IRQ_TYPE_LEVEL_HIGH> below) collide with TLMM GPIO
62?

In drivers/pinctrl/qcom/pinctrl-msm8952.c, msm8952_mpm_map[] maps GPIO 62
to MPM wakeup pin 53:

	{ 45, 24 }, { 46, 25 }, { 48, 26 }, { 54, 14 }, { 58, 17 }, { 62, 53 },

When an interrupt is requested on a wakeup-capable TLMM GPIO via
msm_gpio_irq_domain_alloc() -> qcom_mpm_alloc(), drivers/irqchip/irq-qcom-mpm.c
checks whether the MPM pin is in qcom,mpm-pin-map:

	map = get_mpm_gic_map(priv, pin);
	if (map == NULL)
		return irq_domain_disconnect_hierarchy(domain->parent, virq);

Because pin 53 is present in qcom,mpm-pin-map as <53 72>, get_mpm_gic_map()
does not return NULL for GPIO 62, so qcom_mpm_alloc() fails to disconnect
the GIC hierarchy and instead binds GPIO 62 to GIC SPI 72, colliding with
mdss on both MPM pin 53 and GIC SPI 72.

Since MDSS is in the collapsible MDSS_GDSC power domain rather than an
always-on MPM wakeup source, should <53 72> be dropped from
qcom,mpm-pin-map and mdss use interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>
directly?

[ ... ]

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

[Severity: High]
Could omitting clocks = <&gcc GCC_CRYPTO_AHB_CLK> and
clock-names = "bam_clk" on cryptobam (and similarly omitting
<&gcc GCC_USB_HS_AHB_CLK> on usbbam: dma-controller@78c4000 below) lead to
unclocked MMIO accesses?

When qcom,controlled-remotely is set without bam_clk in the DT node,
bam_dma_probe() in drivers/dma/qcom/bam_dma.c sets bdev->bamclk to NULL via
devm_clk_get_optional(bdev->dev, "bam_clk").

For cryptobam, qce_crypto_probe() in drivers/crypto/qce/core.c requests the
DMA channels and then autosuspends via qce_runtime_suspend(), gating off
GCC_CRYPTO_AHB_CLK (or suspends immediately via the
PM_RUNTIME_ACQUIRE_AUTOSUSPEND guard if probe fails after
devm_qce_dma_request()). If the crypto device is later unbound or unwinds
from a probe failure, qce_dma_release() -> dma_release_channel() ->
bam_free_chan() resets the BAM channels and masks interrupts at 0x00704000
while GCC_CRYPTO_AHB_CLK is disabled.

Similarly, usbbam is enabled by default while usb@78db000 is disabled, so
bam_dma_remove() will access 0x078c4000 with GCC_USB_HS_AHB_CLK gated off
if usbbam is unbound.

Should cryptobam and usbbam specify their respective AHB clocks as bam_clk?

[ ... ]

> +		mdss: display-subsystem@1a00000 {
> +			compatible = "qcom,mdss";
> +			reg = <0x01a00000 0x1000>,
> +			      <0x01ac8000 0x3000>;
> +			reg-names = "mdss_phys",
> +				    "vbif_phys";
> +
> +			ranges;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			power-domains = <&gcc MDSS_GDSC>;
> +
> +			clocks = <&gcc GCC_MDSS_AHB_CLK>,
> +				 <&gcc GCC_MDSS_AXI_CLK>,
> +				 <&gcc GCC_MDSS_VSYNC_CLK>;
> +			clock-names = "iface",
> +				      "bus",
> +				      "vsync";
> +
> +			interrupts-extended = <&mpm 53 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +
> +			resets = <&gcc GCC_MDSS_BCR>;
> +
> +			status = "disabled";
> +
> +			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]
Are mdss_mdp and gpu_iommu (iommu@1f08000 below) missing their TBU clocks?

In drivers/clk/qcom/gcc-msm8952.c, gcc_mdp_tbu_clk (GCC_MDP_TBU_CLK) and
gcc_gfx_tbu_clk (GCC_GFX_TBU_CLK) are registered as non-critical votable
branch clocks:

	static struct clk_branch gcc_gfx_tbu_clk = {
		.halt_reg = 0x12010,
		.halt_check = BRANCH_HALT_VOTED,
		.clkr = {
			.enable_reg = 0x4500c,
			.enable_mask = BIT(3),
	...
	static struct clk_branch gcc_mdp_tbu_clk = {
		.halt_reg = 0x1201c,
		.halt_check = BRANCH_HALT_VOTED,
		.clkr = {
			.enable_reg = 0x4500c,
			.enable_mask = BIT(4),

Because neither clock is referenced in msm8952.dtsi, clk_disable_unused()
will clear their vote bits at late_initcall. When mdss_mdp enables its
clocks in mdp5_enable() or gpu_iommu enables its clocks in
qcom_iommu_resume(), the MDP and GPU TBUs will remain unclocked during
IOMMU translations.

Should mdss_mdp include <&gcc GCC_MDP_TBU_CLK> with clock-names "tbu", and
gpu_iommu include <&gcc GCC_GFX_TBU_CLK> with clock-names "tbu"?

[ ... ]

> +	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_alert1>;
> +					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: High]
In cpu4-thermal (as well as cpu5-thermal, cpu6-thermal, cpu7-thermal, and
cpu0-3-thermal below), map0 binds the CPU cooling devices to cpuX_alert1
(which has type = "hot") while leaving cpuX_alert0 (type = "passive")
unbound, and omits polling-delay-passive.

In the thermal framework, thermal governors ignore THERMAL_TRIP_HOT trips
when managing cooling devices, for example in
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);
	}

In addition, without polling-delay-passive in these zones,
tz->passive_delay_jiffies is 0, so monitor_thermal_zone() in
drivers/thermal/thermal_core.c will not schedule periodic polling while a
passive trip is active (and pid_controller() in
drivers/thermal/gov_power_allocator.c divides by
jiffies_to_msecs(tz->passive_delay_jiffies)).

Should map0 in cpu4-thermal, cpu5-thermal, cpu6-thermal, cpu7-thermal, and
cpu0-3-thermal reference the passive trip (cpuX_alert0) instead of
cpuX_alert1 (or change cpuX_alert1 to type = "passive"), and define
polling-delay-passive on those zones?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-msm8952-initial-support-v7-0-ad748ace375d@mainlining.org?part=9