Re: [PATCH 05/34] arm64: dts: qcom: sm8150: Fix PCIe wake GPIO polarity

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The commit fixes wake-gpios for pcie0 but misses the pcie1 node, which has a typoed 'enable-gpio' instead of 'wake-gpios' and incorrect active-high polarity.
- [High] The patch updates the PCIe WAKE# signal to GPIO_ACTIVE_LOW but neglects to fix the similarly inverted PERST# GPIO polarity in both PCIe nodes, and misses updating the dt-bindings example.
--

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

arm64: dts: qcom: sm8150: Fix PCIe wake GPIO polarity

This commit corrects the WAKE# GPIO polarity for the sm8150 pcie0 node 
to GPIO_ACTIVE_LOW, complying with the active-low requirement in the 
PCIe Base Specification.

> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index 94a3b02cb2897..74df5cd86f04f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -1905,7 +1905,7 @@ pcie0: pcie@1c00000 {
>  			phy-names = "pciephy";
>  
>  			perst-gpios = <&tlmm 35 GPIO_ACTIVE_HIGH>;

[Severity: High]
This isn't a bug introduced by this patch, but since the WAKE# polarity is
being fixed to match the PCIe Base Specification, should the PERST# polarity
be updated as well?

The PERST# signal is also strictly defined as active-low by the PCIe spec.
Leaving perst-gpios configured as GPIO_ACTIVE_HIGH in both pcie0 and pcie1
causes the pcie-qcom driver to invert the hardware reset sequence. When the
driver logically asserts the reset using GPIOD_OUT_HIGH, an active-high
configuration causes the physical pin to drive high on assertion and low on
de-assertion. Does this hold the endpoint in physical reset when the driver
attempts to initialize it?

Also, could the dt-bindings example in
Documentation/devicetree/bindings/pci/qcom,pcie-sm8150.yaml be updated? It
currently retains the GPIO_ACTIVE_HIGH polarity for both WAKE# and PERST#,
which might teach the wrong polarity to users.

> -			wake-gpios = <&tlmm 37 GPIO_ACTIVE_HIGH>;
> +			wake-gpios = <&tlmm 37 GPIO_ACTIVE_LOW>;

[Severity: High]
This is a pre-existing issue, but was the WAKE# configuration for the pcie1
node accidentally overlooked?

Looking at the pcie1 node, the WAKE# GPIO is misspelled as enable-gpio and
retains the incorrect active-high polarity:

        perst-gpios = <&tlmm 102 GPIO_ACTIVE_HIGH>;
        enable-gpio = <&tlmm 104 GPIO_ACTIVE_HIGH>;

Because of this typo, the PCI framework completely ignores the WAKE# GPIO,
breaking PCIe wake and power management features for the secondary
controller. Would it make sense to correct enable-gpio to wake-gpios and
set it to GPIO_ACTIVE_LOW in this patch?

>  
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&pcie0_default_state>;

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