Re: [PATCH v2 35/37] arm64: dts: qcom: sm8650: Move PCIe phy and GPIOs to root port node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Commit message inaccuracy: `phy-names` is deleted rather than moved.
- [Low] Duplicate `&pcie1_port0` node override blocks in `sm8650-ayaneo-pocket-s2.dts`.
--

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

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

This commit moves PCIe PHY references and perst/wake GPIO properties from
the RC controller node to the root port nodes across several sm8650 device
trees. It also renames perst-gpios to reset-gpios to match the bindings.

> The PCIe phy reference and the perst/wake GPIO properties are
> per root port and belong in the root port node (pcie@0), not in the
> RC controller node. Move phys and phy-names from the controller to
> the existing pcieport0 and pcie1_port0, allowing board-level

[Severity: Medium]
The commit message states that both phys and phy-names are moved to the port
nodes. However, looking at the diff for sm8650.dtsi below, the phy-names
property is deleted from the controller but never added to the port node.

Could the commit message be updated to reflect that phy-names is being dropped,
or should phy-names also be added to the port nodes?

> diff --git a/arch/arm64/boot/dts/qcom/sm8650-ayaneo-pocket-s2.dts b/arch/arm64/boot/dts/qcom/sm8650-ayaneo-pocket-s2.dts
> index 2123312d88f6b..74a286bf76960 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650-ayaneo-pocket-s2.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8650-ayaneo-pocket-s2.dts

[ ... ]

> @@ -1108,15 +1108,17 @@ &pcie0_phy {
>  };
>  
>  &pcie1 {
> -	wake-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> -	perst-gpios = <&tlmm 97 GPIO_ACTIVE_LOW>;
> -
>  	pinctrl-0 = <&pcie1_default_state>;
>  	pinctrl-names = "default";
>  
>  	status = "okay";
>  };
>  
> +&pcie1_port0 {
> +	reset-gpios = <&tlmm 97 GPIO_ACTIVE_LOW>;
> +	wake-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> +};
> +
>  &pcie1_port0 {
>  	/* Renesas μPD720201 PCIe USB3.0 HOST CONTROLLER */
>  	usb-controller@0 {

[Severity: Low]
This isn't a functional bug since the device tree compiler will merge these
blocks, but does this leave two adjacent &pcie1_port0 override blocks?

Would it be better to consolidate the reset-gpios and wake-gpios into the
existing &pcie1_port0 block right below this to keep the tree tidy?

> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 1604bc8cff373..fc6fc4d7e39d7 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi

[ ... ]

> @@ -3644,9 +3644,6 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  			num-lanes = <2>;
>  			bus-range = <0 0xff>;
>  
> -			phys = <&pcie0_phy>;
> -			phy-names = "pciephy";
> -
>  			#address-cells = <3>;
>  			#size-cells = <2>;
>  			ranges = <0x01000000 0 0x00000000 0 0x60200000 0 0x100000>,
> @@ -3716,6 +3713,8 @@ pcieport0: pcie@0 {
>  				#address-cells = <3>;
>  				#size-cells = <2>;
>  				ranges;
> +
> +				phys = <&pcie0_phy>;
>  			};
>  		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-wake-v2-0-2744251b1181@oss.qualcomm.com?part=35