Re: [PATCH v3 6/6] 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] arm64: dts: qcom: lemans-sa8255p-ride: pcie0_ep and pcie1_ep left disabled
- [Medium] arm64: dts: qcom: lemans-sa8255p-ride: wrong PCIe EP wake-gpios polarity
--

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

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

This commit adds initial device tree support for the Lemans SA8255P Ride
board, configuring UFS storage, UART consoles, PCIe endpoint interfaces,
PMM8654AU thermal zones, 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..6f19064997a4b
> --- /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>;
> +	wake-gpios = <&tlmm 0 GPIO_ACTIVE_HIGH>;
> +};
> +
> +&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>;
> +	wake-gpios = <&tlmm 5 GPIO_ACTIVE_HIGH>;
> +};

[Severity: Medium]
As reported by Sashiko on v2 [1], should pcie0_ep and pcie1_ep also set
status = "okay" here?

In arch/arm64/boot/dts/qcom/lemans.dtsi, pcie0, pcie0_ep, pcie1, and pcie1_ep
all default to status = "disabled". In
arch/arm64/boot/dts/qcom/lemans-gearvm.dtsi, pcie0_ep and pcie1_ep are
configured with the sa8255p compatible and power domains:

&pcie0_ep {
	compatible = "qcom,sa8255p-pcie-ep";
	power-domains = <&scmi5_pd 1>;
};

&pcie1_ep {
	compatible = "qcom,sa8255p-pcie-ep";
	power-domains = <&scmi6_pd 1>;
};

This board file enables scmi5 and scmi6 and configures the pinctrl and GPIOs
for pcie0_ep and pcie1_ep, and the commit message states that it enables the
PCIe EP interfaces. Although an earlier discussion on v2 noted that PCIe0 RC
is enabled so PCIe EP cannot be enabled simultaneously, lemans-sa8255p-ride.dts
only includes lemans-gearvm.dtsi (not lemans-ride-common.dtsi), so neither
pcie0 nor pcie1 is enabled in this device tree. Without status = "okay",
won't both pcie0_ep and pcie1_ep remain disabled at runtime?

[Severity: Medium]
As reported by Sashiko on v2 [2], is GPIO_ACTIVE_HIGH the intended polarity
for wake-gpios in pcie0_ep and pcie1_ep?

When qcom_pcie_ep_probe() in drivers/pci/controller/dwc/pcie-qcom-ep.c probes
a qcom,sa8255p-pcie-ep node, qcom_pcie_ep_get_resources() requests the wake
GPIO with GPIOD_OUT_LOW, and qcom_pcie_perst_deassert() toggles the logical
state to pulse the active-low wake signal to the root complex:

	gpiod_set_value_cansleep(pcie_ep->wake, 1);
	usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);
	gpiod_set_value_cansleep(pcie_ep->wake, 0);

Both Documentation/devicetree/bindings/pci/qcom,sa8255p-pcie-ep.yaml and
arch/arm64/boot/dts/qcom/lemans-ride-common.dtsi specify GPIO_ACTIVE_LOW for
wake-gpios on these same tlmm pins (&tlmm 0 and &tlmm 5). With
GPIO_ACTIVE_HIGH, won't the physical wake line be held low when idle and
driven high during the wakeup pulse?

[1] https://lore.kernel.org/r/sashiko-outbox-172176@kernel.org
[2] https://lore.kernel.org/r/sashiko-outbox-172176@kernel.org

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