Re: [PATCH 2/2] arm64: dts: qcom: sm8150: Drop non-existing enable-gpio in PCI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] arm64: dts: qcom: sm8150: pcie1 enable-gpio dropped instead
  of wake-gpios
- [Low] arm64: dts: qcom: sm8150: missing Fixes trailer for commit
  a1c86c680533
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] arm64: dts: qcom: sm8150: incorrect PCIe PERST# GPIO polarity
  [1]
--

commit 665cb84da938cddddc3913668cfa84f9461af819
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

> arm64: dts: qcom: sm8150: Drop non-existing enable-gpio in PCI
>
> Qualcomm PCI bindings do not allow enable-gpio.  Linux drivers do not
> use it, either.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

[Severity: Low]
Should this include a Fixes: tag for the commit that introduced the invalid
enable-gpio property on pcie1:

  Fixes: a1c86c680533 ("arm64: dts: qcom: sm8150: Add PCIe nodes")

> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index 293fcbacff346..e62a3005134f9 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -2019,8 +2019,6 @@ pcie1: pcie@1c08000 {
>  
>  			power-domains = <&gcc PCIE_1_GDSC>;
>  
> -			enable-gpio = <&tlmm 104 GPIO_ACTIVE_HIGH>;
> -
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&pcie1_default_state>;
>  

[Severity: Medium]
Should gpio104 be moved into the pcie@0 root port child node as
wake-gpios = <&tlmm 104 GPIO_ACTIVE_LOW> instead of being dropped?

Commit a1c86c680533 ("arm64: dts: qcom: sm8150: Add PCIe nodes") added both
pcie0 and pcie1 with misnamed enable-gpio properties for their WAKE# pins,
and pcie1_default_state still configures gpio104 under wake-pins.  On pcie0,
commits 7c38989d0f7a ("arm64: dts: qcom: sm8150: correct PCIe wake-gpios"),
463d931d6a66 ("arm64: dts: qcom: sm8150: Fix PCIe WAKE# GPIO polarity"), and
9e753907e6ba ("arm64: dts: qcom: sm8150: Move PCIe phy and GPIOs to root port
node") converted and moved gpio37 into pcie0's pcie@0 child node, but missed
pcie1 because its property was still named enable-gpio.  Dropping gpio104
leaves pcie1 without wake-gpios and prevents pci_configure_of_wake_gpio()
from configuring PCIe WAKE# wakeup on pcie1.

[Severity: High]
This problem wasn't introduced by this patch, but the PCIe PERST# GPIOs in
pcie0 and pcie1 (reset-gpios = <&tlmm 35 GPIO_ACTIVE_HIGH> and
reset-gpios = <&tlmm 102 GPIO_ACTIVE_HIGH>) are configured with inverted
polarity [1].

PERST# is an active-low signal.  When qcom_ep_reset_deassert() sets the
logical GPIO value to 0 to release the endpoint from reset, GPIO_ACTIVE_HIGH
causes gpiolib to drive the physical pin low, asserting PERST# and holding
connected PCIe endpoints in reset.

[1] https://sashiko.dev/#/bug/linux-9970a06f-8202-465a-86f2-3c66f7fd39f4

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106173719.276668-3-krzysztof.kozlowski@oss.qualcomm.com?part=2