Re: [PATCH v2 7/7] arm64: dts: qcom: lemans: Enable lemans-sa8255p-ride board support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dts: qcom: missing status = "okay" for pcie0_ep and pcie1_ep
- [Medium] dts: qcom: incorrect polarity for PCIe EP wake-gpios
--

commit 3fa46fe28ef2e9d52dd0b40b57be7d7192538595
Author: Nikunj Kela <quic_nkela@quicinc.com>

arm64: dts: qcom: lemans: Enable lemans-sa8255p-ride board support

Add initial device tree support for the Lemans SA8255P Ride board. Enable
UFS storage, UART10 as the serial console, PCIe EP interfaces, thermal
zones for PMM8654AU sensors, and SCMI protocol nodes.

> diff --git a/arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts b/arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts
> new file mode 100644
> index 0000000000000..4273f47c3dffc
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts

[ ... ]

> +&pcie0_ep {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_ep_clkreq_default &pcie0_ep_perst_default
> +		    &pcie0_ep_wake_default>;
> +	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;

[Severity: Medium]
Since the commit message states this patch enables the PCIe EP interfaces,
should pcie0_ep include a status = "okay"; property?

Without explicitly overriding the status, the node will likely remain
disabled as typically defined in the base device tree, preventing the kernel
from probing the controller.

> +	wake-gpios = <&tlmm 0 GPIO_ACTIVE_HIGH>;

[Severity: Medium]
Is the polarity for the wake-gpios correct here?

The PCIe WAKE# signal is architecturally active-low and open-drain. By
configuring it as GPIO_ACTIVE_HIGH, the physical pad will be driven low
when the driver intends to initialize or deassert it, and high when
asserting it.

Could this invert the wake signal and break PCIe endpoint power management
wakeups?

> +};
> +
> +&pcie1_ep {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie1_ep_clkreq_default &pcie1_ep_perst_default
> +		    &pcie1_ep_wake_default>;
> +	reset-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>;

[Severity: Medium]
Similarly, should pcie1_ep also include a status = "okay"; property to
ensure it is enabled?

> +	wake-gpios = <&tlmm 5 GPIO_ACTIVE_HIGH>;

[Severity: Medium]
Does this wake-gpios configuration also suffer from the same active-low
signal inversion issue as pcie0_ep?

> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015023711.721837-1-shengchao.guo@oss.qualcomm.com?part=7