Re: [PATCH v4 4/4] arm64: dts: qcom: Add IMDT QCS8550 SBC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Encoding downstream device resets and straps into the PCIe host controller's pinctrl to control boot sequencing.
- [Low] Dangling `enable-active-high` property in `regulator-gpio` without `enable-gpios`.
--

commit 69cdd29642da9fee5fcb6d62c3df7354abac11dc
Author: William Bright <william.bright@imd-tec.com>

arm64: dts: qcom: Add IMDT QCS8550 SBC

This commit adds device tree support for the IMDT QCS8550 SBC. It enables
various peripherals including regulators, UART, UFS, PCIe, USB, and Ethernet.

> diff --git a/arch/arm64/boot/dts/qcom/qcs8550-imdt-sbc.dts b/arch/arm64/boot/dts/qcom/qcs8550-imdt-sbc.dts
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs8550-imdt-sbc.dts

[ ... ]

> +	sdhci_2_vqmmc: regulator-sdhci-2-vqmmc {
> +		compatible = "regulator-gpio";
> +
> +		regulator-name = "sdhci_2_vqmmc";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-type = "voltage";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sd_vset_default>;
> +
> +		gpios = <&tlmm 4 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;

[Severity: Low]
Is this enable-active-high property needed here?

This regulator uses regulator-gpio and defines the gpios array for voltage
state selection, but it does not specify an enable-gpios property. Since the
property is dangling, will this trigger a dtbs_check validation warning?

> +
> +		gpios-states = <0>;
> +		states = <3300000 0>,
> +			 <1800000 1>;

[ ... ]

> +&pcie0 {
> +	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
> +	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
> +
> +	/*
> +	 * pcie0 hosts the M.2 Key-E slot. Apply the SDIO
> +	 * reset de-assert here so any module's chip enable is settled
> +	 * before pcie0 trains its link.
> +	 */
> +	pinctrl-0 = <&pcie0_default_state>, <&m2e_sdio_resetn_default>;

[Severity: Medium]
Is it correct to encode downstream device resets into the PCIe host
controller's pinctrl?

Tying board-level reset pins like the SDIO reset to the PCIe host
controller's pinctrl tightly couples peripheral initialization to the host
driver's state machine.

Should these downstream devices be modeled using PCI child nodes with their
own reset properties or standard power sequencing mechanisms instead, to
avoid unintended GPIO toggling during PCIe suspend and resume?

> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +};

[ ... ]

> +&pcie1 {
> +	wake-gpios = <&tlmm 99 GPIO_ACTIVE_HIGH>;
> +	perst-gpios = <&tlmm 97 GPIO_ACTIVE_LOW>;
> +
> +	/*
> +	 * pcie_switch_sel_default and gbe_reset_default are board-init
> +	 * lines that must be stable before pcie1 trains its link: the
> +	 * PCIe switch needs its mode-select strap settled, and the
> +	 * downstream LAN743x must be out of reset to enumerate.
> +	 * Applying them via pcie1's pinctrl-0 fires them during
> +	 * qcom-pcie probe, before bus enumeration.
> +	 */
> +	pinctrl-0 = <&pcie1_default_state>,
> +		    <&pcie_switch_sel_default>,
> +		    <&gbe_reset_default>;

[Severity: Medium]
Does assigning the PCIe switch strap and downstream GbE PHY reset to the pcie1
pinctrl violate device tree abstractions?

Similar to pcie0, this couples downstream device initialization directly to
the host controller. Modeling this hardware topology through proper PCI child
nodes might be safer than relying on host driver sequencing.

> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-imdt-qcs8550-sbc-rfc-v4-0-358e71d606bc@imd-tec.com?part=4