Re: [PATCH 2/2] arm64: dts: rockchip: rk3588-evb1: describe the PCIe3x4 slot supply

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: dw-rockchip: missing select PCI_PWRCTRL_GENERIC breaks link and power
- [High] PCI: dw-rockchip: rockchip_pcie_probe() destroys pwrctrl devices on deferral
--

commit 78bab75f21b5749b90228d2891ea140c86ee2884
Author: Shawn Lin <shawn.lin@rock-chips.com>

arm64: dts: rockchip: rk3588-evb1: describe the PCIe3x4 slot supply

This commit moves the vpcie3v3-supply regulator of the PCIe3x4 controller on
rk3588-evb1-v10 into a child pcie@0 bridge node so that it is managed by the
PCI pwrctrl framework.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
> index 8969b56f3063e..80eb36e783e0c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
> @@ -569,8 +569,20 @@ &pcie3x4 {
>  	pinctrl-0 = <&pcie3_reset>, <&pcie30x4m1_clkreqn>;
>  	supports-clkreq;
>  	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> -	vpcie3v3-supply = <&vcc3v3_pcie30>;
>  	status = "okay";
> +
> +	pcie@0 {
> +		device_type = "pci";
> +		compatible = "pciclass,0604";
> +		reg = <0x000000 0x0 0x0 0x0 0x0>;
> +		bus-range = <0x00 0x0f>;
> +
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		vpcie3v3-supply = <&vcc3v3_pcie30>;
> +	};

[Severity: High]
With vpcie3v3-supply moved from &pcie3x4 into the pcie@0 child node (matched
by compatible = "pciclass,0604" in pci-pwrctrl-slot under
CONFIG_PCI_PWRCTRL_GENERIC), and commit f0cd23e8e469 ("PCI: dw-rockchip:
Power on devices below the RC using pwrctrl") adding direct calls to the
pci_pwrctrl_* helpers in rockchip_pcie_probe(), should PCIE_ROCKCHIP_DW_HOST
select PCI_PWRCTRL_GENERIC in drivers/pci/controller/dwc/Kconfig?

Currently in drivers/pci/controller/dwc/Kconfig, PCIE_ROCKCHIP_DW and
PCIE_ROCKCHIP_DW_HOST are bool symbols built into vmlinux without selecting
PCI_PWRCTRL_GENERIC:

drivers/pci/controller/dwc/Kconfig:
	config PCIE_ROCKCHIP_DW
		bool

	config PCIE_ROCKCHIP_DW_HOST
		bool "Rockchip DesignWare PCIe controller (host mode)"
		depends on PCI_MSI
		depends on ARCH_ROCKCHIP || COMPILE_TEST
		depends on OF
		select PCIE_DW_HOST
		select PCIE_ROCKCHIP_DW
		select PCI_HOST_COMMON

Meanwhile, include/linux/pci-pwrctrl.h exposes the non-stub declarations
whenever CONFIG_PCI_PWRCTRL is enabled as either built-in or a module:

include/linux/pci-pwrctrl.h:
	#if IS_ENABLED(CONFIG_PCI_PWRCTRL)
	int pci_pwrctrl_create_devices(struct device *parent);
	void pci_pwrctrl_destroy_devices(struct device *parent);
	int pci_pwrctrl_power_on_devices(struct device *parent);
	void pci_pwrctrl_power_off_devices(struct device *parent);
	#else

If CONFIG_PCIE_ROCKCHIP_DW_HOST=y and CONFIG_PCI_PWRCTRL=m (for example when
CONFIG_PCI_PWRCTRL_GENERIC=m as in arch/arm64/configs/defconfig and no other
built-in driver selects PCI_PWRCTRL_GENERIC=y), linking vmlinux fails with
undefined references from rockchip_pcie_probe() to
pci_pwrctrl_create_devices(), pci_pwrctrl_power_on_devices(),
pci_pwrctrl_power_off_devices(), and pci_pwrctrl_destroy_devices().

Additionally, if CONFIG_PCI_PWRCTRL_GENERIC=n, either CONFIG_PCI_PWRCTRL=n
turns those calls into no-op stubs while &pcie3x4 no longer has
vpcie3v3-supply on the controller node (leaving vcc3v3_pcie30 disabled when
rockchip_pcie_phy_init() runs), or CONFIG_PCI_PWRCTRL=y via another pwrctrl
driver causes pci_pwrctrl_power_on_devices() to return -EPROBE_DEFER
permanently because no driver binds to "pciclass,0604".

[Severity: High]
What happens on rk3588-evb1-v10 when pci_pwrctrl_power_on_devices() or a
subsequent step in rockchip_pcie_probe() returns -EPROBE_DEFER for this
pcie@0 child node?

In drivers/pci/pwrctrl/core.c, pci_pwrctrl_power_on_devices() calls
pci_pwrctrl_power_on_device(), which returns -EPROBE_DEFER whenever the newly
created pwrctrl platform device for pcie@0 has not yet bound to its driver
(for example if slot_pwrctrl_probe() defers on vcc3v3_pcie30 or &gpio3, or if
pci-pwrctrl-slot is a module not yet loaded):

drivers/pci/pwrctrl/core.c:pci_pwrctrl_power_on_device()
	scoped_guard(device, &pdev->dev) {
		if (device_is_bound(&pdev->dev)) {
			ret = __pci_pwrctrl_power_on_device(&pdev->dev);
		} else {
			/* FIXME: Use blocking wait instead of probe deferral */
			dev_dbg(&pdev->dev, "driver is not bound\n");
			ret = -EPROBE_DEFER;
		}
	}

However, in drivers/pci/controller/dwc/pcie-dw-rockchip.c,
rockchip_pcie_probe() unconditionally destroys the pwrctrl child devices both
when pci_pwrctrl_power_on_devices() fails and at the err_pwrctrl_off cleanup
label:

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_probe()
	if (data->mode == DW_PCIE_RC_TYPE) {
		ret = pci_pwrctrl_create_devices(dev);
		if (ret)
			return dev_err_probe(dev, ret,
					     "failed to create pwrctrl devices\n");

		ret = pci_pwrctrl_power_on_devices(dev);
		if (ret) {
			pci_pwrctrl_destroy_devices(dev);
			return dev_err_probe(dev, ret,
					     "failed to power on pwrctrl devices\n");
		}
	}
	...
err_pwrctrl_off:
	if (data->mode == DW_PCIE_RC_TYPE) {
		pci_pwrctrl_power_off_devices(dev);
		pci_pwrctrl_destroy_devices(dev);
	}

Calling pci_pwrctrl_destroy_devices(dev) on -EPROBE_DEFER unregisters the
pcie@0 platform device via of_device_unregister(), removing it from sysfs and
deferred_probe_pending_list so it cannot bind to slot_pwrctrl_driver and
trigger a re-probe of &pcie3x4.

Should rockchip_pcie_probe() avoid calling pci_pwrctrl_destroy_devices(dev)
when ret == -EPROBE_DEFER (matching pcie-qcom.c, pci-imx6.c, and
pcie-mediatek-gen3.c)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027075520.3591049-1-shawn.lin@rock-chips.com?part=2