Re: [PATCH v2 2/2] arm64: dts: qcom: Add Xiaomi Poco M3

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: citrus: wifi node bypasses wcn3950-pmu regulators
- [Low] arm64: dts: qcom: citrus: mismatched unit-addresses on reserved-memory nodes
--

commit f6a18aab14c146c73ad430a3e102de48b29909dc
Author: Amir Hanip <killdeath104@gmail.com>

arm64: dts: qcom: Add Xiaomi Poco M3

This commit adds the initial device tree for the Xiaomi Poco M3 (codename
citrus) based on the Qualcomm Snapdragon 662 (SM6115) SoC, enabling UFS,
microSD, USB host, WiFi, Bluetooth, and remoteprocs.

> diff --git a/arch/arm64/boot/dts/qcom/sm6115-xiaomi-citrus.dts b/arch/arm64/boot/dts/qcom/sm6115-xiaomi-citrus.dts
> new file mode 100644
> index 0000000000000..330f626ae7b1f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm6115-xiaomi-citrus.dts

[ ... ]

> +	wcn3950-pmu {
> +		compatible = "qcom,wcn3950-pmu";
> +
> +		pinctrl-0 = <&sw_ctrl_default>;
> +		pinctrl-names = "default";
> +
> +		vddio-supply = <&pm6125_l9>;
> +		vddxo-supply = <&pm6125_l16>;
> +		vddrf-supply = <&pm6125_l17>;
> +		vddch0-supply = <&pm6125_l23>;
> +
> +		swctrl-gpios = <&tlmm 87 GPIO_ACTIVE_HIGH>;
> +
> +		regulators {
> +			vreg_pmu_io: ldo0 {
> +				regulator-name = "vreg_pmu_io";
> +			};
> +
> +			vreg_pmu_xo: ldo1 {
> +				regulator-name = "vreg_pmu_xo";
> +			};
> +
> +			vreg_pmu_rf: ldo2 {
> +				regulator-name = "vreg_pmu_rf";
> +			};
> +
> +			vreg_pmu_ch0: ldo3 {
> +				regulator-name = "vreg_pmu_ch0";
> +			};
> +
> +			vreg_pmu_ch1: ldo4 {
> +				regulator-name = "vreg_pmu_ch1";
> +			};
> +		};
> +	};
> +};

[ ... ]

> +&pil_adsp_mem {
> +	reg = <0x0 0x53800000 0x0 0x2200000>;
> +};
> +
> +&pil_cdsp_mem {
> +	reg = <0x0 0x51a00000 0x0 0xa00000>;
> +};
> +
> +&pil_gpu_mem {
> +	reg = <0x0 0x55a15000 0x0 0x2000>;
> +};
> +
> +&pil_ipa_fw_mem {
> +	reg = <0x0 0x55a00000 0x0 0x10000>;
> +};
> +
> +&pil_ipa_gsi_mem {
> +	reg = <0x0 0x55a10000 0x0 0x5000>;
> +};

[Severity: Low]
In arch/arm64/boot/dts/qcom/sm6115.dtsi, pil_ipa_fw_mem, pil_ipa_gsi_mem,
and pil_gpu_mem are declared under /reserved-memory with unit-addresses matching
their original base addresses:

arch/arm64/boot/dts/qcom/sm6115.dtsi:
		pil_ipa_fw_mem: memory@56100000 {
			reg = <0x0 0x56100000 0x0 0x10000>;
			no-map;
		};

		pil_ipa_gsi_mem: memory@56110000 {
			reg = <0x0 0x56110000 0x0 0x5000>;
			no-map;
		};

		pil_gpu_mem: memory@56115000 {
			reg = <0x0 0x56115000 0x0 0x2000>;
			no-map;
		};

Overriding their reg properties in-place leaves the compiled DTB with
memory@56100000, memory@56110000, and memory@56115000 nodes whose unit-addresses
no longer match their reg base addresses (0x55a00000, 0x55a10000, and
0x55a15000), which causes dtbs_check warnings.

Should these nodes be removed with /delete-node/ and recreated under
/reserved-memory with matching unit-addresses (memory@55a00000, memory@55a10000,
and memory@55a15000)?

[ ... ]

> +&uart3 {
> +	pinctrl-0 = <&uart3_default>;
> +	pinctrl-1 = <&uart3_sleep>;
> +	pinctrl-names = "default", "sleep";
> +
> +	status = "okay";
> +
> +	bluetooth {
> +		compatible = "qcom,wcn3950-bt";
> +		vddio-supply = <&vreg_pmu_io>;
> +		vddxo-supply = <&vreg_pmu_xo>;
> +		vddrf-supply = <&vreg_pmu_rf>;
> +		vddch0-supply = <&vreg_pmu_ch0>;
> +	};
> +};

[ ... ]

> +&wifi {
> +	vdd-0.8-cx-mx-supply = <&pm6125_l4>;
> +	vdd-1.8-xo-supply = <&pm6125_l16>;
> +	vdd-1.3-rfa-supply = <&pm6125_l17>;
> +	vdd-3.3-ch0-supply = <&pm6125_l23>;
> +	vdd-3.3-ch1-supply = <&pm6125_l24>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Should the wifi node reference the wcn3950-pmu regulators (&vreg_pmu_xo,
&vreg_pmu_rf, and &vreg_pmu_ch0) instead of pointing directly to the PM6125
RPM regulators?

When drivers/power/sequencing/pwrseq-qcom-wcn.c binds to wcn3950-pmu,
pwrseq_qcom_wcn3990_match() matches the WiFi device by checking whether
vdd-1.8-xo-supply resolves to a regulator subnode under wcn3950-pmu:

drivers/power/sequencing/pwrseq-qcom-wcn.c:pwrseq_qcom_wcn3990_match() {
	/* BT device */
	ret = pwrseq_qcom_wcn_match_regulator(pwrseq, dev, "vddio-supply");
	if (ret == PWRSEQ_MATCH_OK)
		return ret;

	/* WiFi device match */
	return pwrseq_qcom_wcn_match_regulator(pwrseq, dev, "vdd-1.8-xo-supply");
}

Because wifi points vdd-1.8-xo-supply directly to &pm6125_l16 rather than
&vreg_pmu_xo, pwrseq_qcom_wcn3990_match() returns PWRSEQ_NO_MATCH, causing
devm_pwrseq_get(&pdev->dev, "wlan") in ath10k_snoc_probe() to return
-EPROBE_DEFER and fall back to legacy regulator management with
ar_snoc->pwrseq = NULL:

drivers/net/wireless/ath/ath10k/snoc.c:ath10k_snoc_probe() {
	ar_snoc->pwrseq = devm_pwrseq_get(&pdev->dev, "wlan");
	if (IS_ERR(ar_snoc->pwrseq)) {
		ret = PTR_ERR(ar_snoc->pwrseq);
		ar_snoc->pwrseq = NULL;
		if (ret != -EPROBE_DEFER)
			goto err_free_irq;

		ar_snoc->num_vregs = ARRAY_SIZE(ath10k_regulators);
	} else {
		ar_snoc->num_vregs = 1;
	}
}

As a result, ath10k bypasses the wcn3950-pmu power sequencer and never enables
or votes on vddio-supply (&pm6125_l9), which is only referenced by wcn3950-pmu
and is not in ath10k_regulators[]. Will this leave vddio unpowered when WiFi is
used while Bluetooth is off, or cause pwrseq_disable() to turn off &pm6125_l9
when Bluetooth is disabled while WiFi is active?

Also, if ch1 is used on this board, should wcn3950-pmu define vddch1-supply and
wifi reference &vreg_pmu_ch1, or if WCN3950 only uses ch0, should
vdd-3.3-ch1-supply on wifi and vreg_pmu_ch1 on wcn3950-pmu be dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027205146.16384-1-killdeath104@gmail.com?part=2