Re: [PATCH v5 4/4] arm64: dts: qcom: shikra-iqs: add M.2 Wi-Fi and Bluetooth overlay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] arm64: dts: qcom: wake-gpios placed on &pcie host bridge
  instead of &pcie_port0
- [Medium] arm64: dts: qcom: duplicate m2_keye_3p3 supply on &pcie
  bypasses M.2 pwrseq
- [Medium] arm64: dts: qcom: shikra: pcie interrupt-map missing 4th
  GICv3 cell
- [Low] arm64: dts: qcom: shikra-iqs: swapped pcie_phy supplies in M.2
  overlay
- [Low] arm64: dts: qcom: shikra-iqs: redundant iommu-map override in
  M.2 overlay
--

commit 1c809b4232eacddb83a64fcaf6b111c590153326
Author: Linghui Wu <linghui.wu@oss.qualcomm.com>

arm64: dts: qcom: shikra-iqs: add M.2 Wi-Fi and Bluetooth overlay

This commit adds a Device Tree overlay for M.2 Key-E Wi-Fi and Bluetooth
modules on the Qualcomm Shikra IQS EVK and disables the onboard WCN3988
Wi-Fi and Bluetooth nodes when the overlay is applied.

Link: https://lore.kernel.org/r/20261102045815.2938940-1-linghui.wu@oss.qualcomm.com

> diff --git a/arch/arm64/boot/dts/qcom/shikra-iqs-evk-m2-wifi-bt.dtso b/arch/arm64/boot/dts/qcom/shikra-iqs-evk-m2-wifi-bt.dtso
> new file mode 100644
> index 0000000000000..ef0f197a53f85
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/shikra-iqs-evk-m2-wifi-bt.dtso
> @@ -0,0 +1,122 @@

[ ... ]

> +	wifi-bt-connector {
> +		compatible = "pcie-m2-e-connector";
> +
> +		vpcie3v3-supply = <&m2_keye_3p3>;
> +		w-disable1-gpios = <&tlmm 61 GPIO_ACTIVE_LOW>;
> +		w-disable2-gpios = <&tlmm 60 GPIO_ACTIVE_LOW>;

[ ... ]

> +&pcie {
> +	iommu-map = <0x0 &apps_smmu 0x0800 0x0 0x1>,
> +		    <0x100 &apps_smmu 0x0801 0x0 0x1>;

[Severity: Low]
Should this 5-cell iommu-map live in shikra.dtsi instead of being
overridden in this overlay?

In shikra.dtsi (added in commit "arm64: dts: qcom: shikra: add PCIe PHY
and controller nodes"), apps_smmu has #iommu-cells = <2>, but the base
pcie node defines 4-cell iommu-map entries (<0x0 &apps_smmu 0x800 0x1>,
<0x100 &apps_smmu 0x801 0x1>), which triggers the of_check_bad_map()
warning in of_map_id().  Since this overlay maps the exact same BDFs and
SIDs and only supplies the missing second IOMMU specifier cell (0x0),
fixing iommu-map in shikra.dtsi allows dropping this override here.

> +	pinctrl-0 = <&pcie_default_state>;
> +	pinctrl-names = "default";
> +	vddpe-3v3-supply = <&m2_keye_3p3>;

[Severity: Medium]
Does setting vddpe-3v3-supply = <&m2_keye_3p3> on &pcie conflict with
pwrseq-pcie-m2 managing the same rail through wifi-bt-connector's
vpcie3v3-supply?

In qcom_pcie_host_init(), pcie->cfg->ops->init() (qcom_pcie_init_2_7_0())
enables vddpe-3v3-supply before pci_pwrctrl_create_devices() and
pci_pwrctrl_power_on_devices() run:

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_host_init() {
	ret = pcie->cfg->ops->init(pcie);
	if (ret)
		return ret;

	ret = qcom_pcie_phy_power_on(pcie);
	if (ret)
		goto err_deinit;

	if (!pci->suspended) {
		ret = pci_pwrctrl_create_devices(pci->dev);
		if (ret)
			goto err_disable_phy;
	}

	if (!pp->skip_pwrctrl_off) {
		ret = pci_pwrctrl_power_on_devices(pci->dev);
		if (ret)
			goto err_pwrctrl_destroy;
	}
}

This turns on m2_keye_3p3 before the M.2 power sequencer runs, glitches
the 3.3 V rail when qcom_pcie_host_init() unwinds through err_deinit on
an initial -EPROBE_DEFER from pci_pwrctrl_power_on_devices(), and holds an
enable vote on &pcie that prevents pwrseq-pcie-m2 from gating the rail.

> +	wake-gpios = <&tlmm 119 GPIO_ACTIVE_LOW>;

[Severity: Medium]
Should wake-gpios be placed on &pcie_port0 rather than on the &pcie host
bridge node?

Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml marks
wake-gpios on the host bridge node as deprecated in favor of the PCIe root
port node.  At runtime, pcie-qcom.c does not parse wake-gpios, and
pci_configure_of_wake_gpio() in drivers/pci/of.c only looks up wake-gpios
on the OF node attached to a struct pci_dev (such as &pcie_port0), so
PCIe WAKE# is never configured when the property is on &pcie.

> +
> +	status = "okay";
> +};

[Severity: Medium]
When this overlay enables &pcie, does legacy PCI INTx parsing fail due to
the 5-cell interrupt-map entries inherited from shikra.dtsi?

In shikra.dtsi, &intc declares #interrupt-cells = <4> and
#address-cells = <2>, so of_irq_parse_imap_parent() consumes 6 cells after
&intc in each interrupt-map entry.  However, the pcie node in shikra.dtsi
(added in commit "arm64: dts: qcom: shikra: add PCIe PHY and controller
nodes") only provides 5 cells after &intc:

arch/arm64/boot/dts/qcom/shikra.dtsi:
			interrupt-map = <0 0 0 1 &intc 0 0 0 499 IRQ_TYPE_LEVEL_HIGH>,
					<0 0 0 2 &intc 0 0 0 500 IRQ_TYPE_LEVEL_HIGH>,
					<0 0 0 3 &intc 0 0 0 501 IRQ_TYPE_LEVEL_HIGH>,
					<0 0 0 4 &intc 0 0 0 502 IRQ_TYPE_LEVEL_HIGH>;

Because the trailing 4th GICv3 cell (0) is missing from each entry,
of_irq_parse_imap_parent() consumes the first cell of the next entry as
the 4th interrupt cell and misaligns all subsequent entries, causing
INTB, INTC and INTD resolution to fail with -EINVAL.

> +
> +&pcie_phy {
> +	vdda-phy-supply = <&pm8150_l12>;
> +	vdda-pll-supply = <&pm8150_l9>;
> +
> +	status = "okay";
> +};

[Severity: Low]
Are vdda-phy-supply and vdda-pll-supply swapped here?

In shikra-iqs-som.dtsi, pm8150_l9 is the 0.88 V rail (875000..1000000 uV)
and pm8150_l12 is the 1.8 V rail (1650000..1950000 uV).  On QMP PHYs (such
as &usb_qmpphy in shikra-iqs-evk.dts and &pcie_phy in qcs615-ride.dts and
talos-evk-som.dtsi), vdda-phy-supply takes the ~0.88 V rail (&pm8150_l9)
and vdda-pll-supply takes the 1.8 V rail (&pm8150_l12).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112083355.3884737-1-linghui.wu@oss.qualcomm.com?part=4