Re: [PATCH 32/34] arm64: dts: qcom: msm8996: Move PCIe phy and GPIOs to root port node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Removing `phys`, `phy-names`, `perst-gpios`, and `wake-gpios` from the PCIe root complex (controller) node breaks device tree backwards compatibility with older kernels.
- [Medium] The commit message falsely claims to move `phy-names` to the root port nodes, but the property is completely omitted.

Pre-existing issues:
- [High] Missing `phy_exit()` cleanup on error paths leads to unbalanced PHY initialization.
--

commit eb9552cbeaad8284f5ba99a861cad696426a8167
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

arm64: dts: qcom: msm8996: Move PCIe phy and GPIOs to root port node

This commit updates the device tree bindings for the MSM8996 PCIe controllers
by moving the PHY references and PERST/WAKE GPIO properties from the Root
Complex nodes into the child port nodes. It also renames the perst-gpios
property to reset-gpios to match the binding used in the root port context.

> Move phys and phy-names from the controller to
> pcie0_port0, pcie1_port0, and pcie2_port0, adding labels to these

[Severity: Medium]
The commit message claims that phy-names is moved to the pcieX_port0 nodes,
but it appears to be completely omitted in the changes below. Is the commit
message inaccurate, or was the property accidentally dropped?

[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> @@ -458,23 +458,32 @@ hdmi_ddc_suspend: hdmi-ddc-suspend-state {
>  
>  &pcie0 {
>  	status = "okay";
> -	perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
>  	vddpe-3v3-supply = <&wlan_en>;
>  	vdda-supply = <&vreg_l28a_0p925>;
>  };

[Severity: Critical]
Does removing the legacy perst-gpios property from the pcie0 controller
override cause a device tree backwards compatibility regression?

Older kernels booting with this updated device tree might fail to initialize
the PCIe controller because they cannot find the reset GPIOs on the Root
Complex node.

[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -1924,9 +1924,6 @@ pcie0: pcie@600000 {
>  				      <0x0c100000 0x100000>;
>  				reg-names = "parf", "dbi", "elbi","config";
>  
> -				phys = <&pciephy_0>;
> -				phy-names = "pciephy";

[Severity: Critical]
Will removing these PHY bindings from the pcie0 controller node cause older
kernel versions to fail to initialize the PCIe controller, since they may not
scan the child port nodes for the PHY properties?

>  				#address-cells = <3>;
>  				#size-cells = <2>;
>  				ranges = <0x01000000 0x0 0x00000000 0x0c200000 0x0 0x100000>,
> @@ -1975,11 +1972,13 @@ pcie0: pcie@600000 {
>  						"bus_master",
>  						"bus_slave";
>  
> -				pcie@0 {
> +				pcie0_port0: pcie@0 {
>  					device_type = "pci";
>  					reg = <0x0 0x0 0x0 0x0 0x0>;
>  					bus-range = <0x01 0xff>;
>  
> +					phys = <&pciephy_0>;
> +

[Severity: Medium]
Should phy-names = "pciephy" be added here, as described in the commit
message?

[ ... ]

[Severity: High]
This is a pre-existing issue, but while reviewing the driver handling for
these ports, is there a missing phy_exit() cleanup on error paths in
drivers/pci/controller/dwc/pcie-qcom.c?

If probe defers due to GPIOs not being ready, the unbalanced PHY enable counts
will accumulate. In qcom_pcie_parse_port():

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_parse_port() {
	...
	ret = qcom_pcie_parse_perst(pcie, port, node);
	if (ret)
		return ret;
	...
}

This early return bypasses PHY cleanup since the port is not yet added to the
global list. Similarly, in qcom_pcie_parse_legacy_binding():

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_parse_legacy_binding() {
	...
	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
	if (!port)
		return -ENOMEM;
	...
}

Could these early returns cause resource leaks of the PHY state initialized
earlier?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-root_port-v1-0-90a252ddbbfa@oss.qualcomm.com?part=32