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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: qcom: sm6115-xiaomi-citrus: wifi node bypasses wcn3950-pmu
- [Low] arm64: dts: qcom: sm6115-xiaomi-citrus: reserved-memory unit-address mismatch
- [Low] arm64: dts: qcom: sm6115-xiaomi-citrus: missing regulator-allow-set-load on pm6125_l22
--

commit 7e5e34e4180038cdeff80b6bdbee89dac2d1329f
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 (citrus),
a smartphone based on the Qualcomm Snapdragon 662 (SM6115) SoC.

> 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..00c56e0f4f91a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm6115-xiaomi-citrus.dts
> @@ -0,0 +1,516 @@

[ ... ]

> +	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_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, these reserved-memory nodes are
defined as:

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

Because overriding reg by label reference does not rename the nodes, does
this leave the unit-addresses (@56115000, @56100000, and @56110000) out of
sync with their new base addresses and trigger dtc / dtbs_check warnings?

Should these nodes be removed with /delete-node/ and recreated under
&reserved_memory with matching @55a00000, @55a10000, and @55a15000
unit-addresses and lowercase hex literals?

[ ... ]

> +		pm6125_l22: l22 {
> +			regulator-min-microvolt = <2952000>;
> +			regulator-max-microvolt = <3304000>;
> +		};

[Severity: Low]
Since pm6125_l22 is used as vmmc-supply for &sdhc_2, should this node also
include regulator-allow-set-load?

When sdhci_msm powers the SD card bus on or off via
sdhci_msm_handle_pwr_irq() -> sdhci_msm_set_vmmc() ->
msm_config_vmmc_regulator(), it calls regulator_set_load(mmc->supply.vmmc,
load). Without regulator-allow-set-load in the device tree node,
of_get_regulation_constraints() does not set REGULATOR_CHANGE_DRMS, causing
drms_uA_update() in drivers/regulator/core.c to skip calling
rpm_reg_set_load():

drivers/regulator/core.c:drms_uA_update() {
    ...
    if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS)) {
        rdev_dbg(rdev, "DRMS operation not allowed\n");
        return 0;
    }
    ...
}

[ ... ]

> +&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: High]
Should &wifi reference the wcn3950-pmu LDO regulators (&vreg_pmu_xo,
&vreg_pmu_rf, &vreg_pmu_ch0) instead of pointing directly to the PM6125
regulators?

In drivers/power/sequencing/pwrseq-qcom-wcn.c, pwrseq_qcom_wcn3990_match()
matches the WiFi device to wcn3950-pmu by checking whether
vdd-1.8-xo-supply resolves to a child regulator of the PMU node:

drivers/power/sequencing/pwrseq-qcom-wcn.c:pwrseq_qcom_wcn3990_match() {
    ...
    /* WiFi device match */
    return pwrseq_qcom_wcn_match_regulator(pwrseq, dev, "vdd-1.8-xo-supply");
}

Because vdd-1.8-xo-supply is set to <&pm6125_l16> instead of
<&vreg_pmu_xo>, pwrseq_qcom_wcn3990_match() returns PWRSEQ_NO_MATCH and
devm_pwrseq_get(&pdev->dev, "wlan") in ath10k_snoc_probe() returns
-EPROBE_DEFER:

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);
    }
    ...
}

As a result, ath10k_snoc_probe() falls back to direct regulator management
with ar_snoc->pwrseq = NULL, bypassing wcn3950-pmu power sequencing. Since
pm6125_l9 (the WCN3950 vddio-supply) is only referenced by wcn3950-pmu and
is not part of ath10k_regulators, won't pm6125_l9 remain unpowered or get
turned off whenever Bluetooth is disabled?

Also, wcn3950-pmu defines vreg_pmu_ch1: ldo4 without a corresponding
vddch1-supply, while &wifi connects vdd-3.3-ch1-supply directly to
&pm6125_l24. Should vreg_pmu_ch1 either be wired via wcn3950-pmu or
dropped if unused?

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