[PATCH 0/2] Manage the PCIe supplies with pwrctrl

[PATCH 0/2] Manage the PCIe supplies with pwrctrl

[Shawn Lin]

The Rockchip PCIe host controller describes a single vpcie3v3 supply on
the controller node to power whatever is plugged into the slot. That is
enough for a board that only ever has one device there, but it cannot
express per-device supplies, and it does not fit boards where the slot
power is owned by a separate power sequencing or switch device.

This series switches the pcie-dw-rockchip driver to the generic PCI
pwrctrl framework, which lets the supplies of the devices below a host
controller be described in their own DT nodes and be enabled before the
PHY is initialized.

Patch 1 makes the driver create and power on the pwrctrl devices below
the controller ahead of the PHY init. The controller level vpcie3v3 supply
is kept for backward compatibility, so existing boards keep working unchanged.

Patch 2 moves the supply of the RK3588 EVB1 slot into a child node that
describes the slot as a PCI bridge.



Shawn Lin (2):
  PCI: dw-rockchip: Power on devices below the RC using pwrctrl
  arm64: dts: rockchip: rk3588-evb1: describe the PCIe3x4 slot supply

 .../boot/dts/rockchip/rk3588-evb1-v10.dts     | 14 ++++++++-
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 30 ++++++++++++++++---
 2 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] PCI: dw-rockchip: Power on devices below the RC using pwrctrl

[Shawn Lin]

Devices below the Rockchip PCIe host controller may need resources
(regulators, clocks) enabled before they can be detected on the bus. So
far the driver only enabled a single vpcie3v3 supply described by the
controller node itself, which cannot express per-device or per-slot
supplies.

Use the PCI pwrctrl framework: before initializing the PHY, create the
pwrctrl devices for the DT nodes below the controller and power them on.
The supplies of the slot must be up before PHY init, so this has to run
ahead of rockchip_pcie_phy_init().

The controller node supply is kept for backward compatibility: boards
that still describe vpcie3v3 on the controller keep working, and boards
that move the supply into a child node get it enabled by pwrctrl.

Only the RC mode is affected, the endpoint does not own the supplies of
the devices below it.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 30 ++++++++++++++++---
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index bd86e32cc2b1..952b4c6cc4bf 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -19,6 +19,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/phy/phy.h>
+#include <linux/pci-pwrctrl.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -841,16 +842,32 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Power on the devices described below this controller before the PHY
+	 * is initialized: the slot supplies must be up before PHY init.
+	 */
+	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");
+		}
+	}
+
 	/* DON'T MOVE ME: must be enable before PHY init */
 	ret = devm_regulator_get_enable_optional(dev, "vpcie3v3");
 	if (ret < 0 && ret != -ENODEV)
-		return dev_err_probe(dev, ret,
-				     "failed to enable vpcie3v3 regulator\n");
+		goto err_pwrctrl_off;
 
 	ret = rockchip_pcie_phy_init(rockchip);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "failed to initialize the phy\n");
+		goto err_pwrctrl_off;
 
 	ret = reset_control_deassert(rockchip->rst);
 	if (ret)
@@ -883,6 +900,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
 deinit_phy:
 	rockchip_pcie_phy_deinit(rockchip);
+err_pwrctrl_off:
+	if (data->mode == DW_PCIE_RC_TYPE) {
+		pci_pwrctrl_power_off_devices(dev);
+		pci_pwrctrl_destroy_devices(dev);
+	}
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH 1/2] PCI: dw-rockchip: Power on devices below the RC using pwrctrl

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: dw-rockchip: pwrctrl devices destroyed on -EPROBE_DEFER in probe
- [Medium] PCI: dw-rockchip: link failure when CONFIG_PCI_PWRCTRL=m
--

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

PCI: dw-rockchip: Power on devices below the RC using pwrctrl

This commit updates rockchip_pcie_probe() to create and power on PCI
pwrctrl devices for DT child nodes below the Rockchip PCIe host controller
before initializing the PHY when operating in RC mode.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index bd86e32cc2b15..952b4c6cc4bf2 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -19,6 +19,7 @@
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/phy/phy.h>
> +#include <linux/pci-pwrctrl.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> @@ -841,16 +842,32 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Power on the devices described below this controller before the PHY
> +	 * is initialized: the slot supplies must be up before PHY init.
> +	 */
> +	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");

[Severity: Medium]
Can this cause a vmlinux link failure when CONFIG_PCIE_ROCKCHIP_DW=y and
CONFIG_PCI_PWRCTRL=m?

In drivers/pci/controller/dwc/Kconfig, PCIE_ROCKCHIP_DW,
PCIE_ROCKCHIP_DW_HOST, and PCIE_ROCKCHIP_DW_EP are bool options:

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

In include/linux/pci-pwrctrl.h, the declarations for
pci_pwrctrl_create_devices(), pci_pwrctrl_destroy_devices(),
pci_pwrctrl_power_on_devices(), and pci_pwrctrl_power_off_devices() are
guarded by #if IS_ENABLED(CONFIG_PCI_PWRCTRL):

#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);

Because data->mode == DW_PCIE_RC_TYPE in rockchip_pcie_probe() is a runtime
check, built-in pcie-dw-rockchip.o emits references to the pci_pwrctrl_*
symbols whenever PCIE_ROCKCHIP_DW_HOST=y or PCIE_ROCKCHIP_DW_EP=y is
enabled, failing to link when pci-pwrctrl-core is built as a module (and
compiling to no-op stubs if CONFIG_PCI_PWRCTRL is disabled).

Should PCIE_ROCKCHIP_DW (or PCIE_ROCKCHIP_DW_HOST and PCIE_ROCKCHIP_DW_EP)
select PCI_PWRCTRL_GENERIC in drivers/pci/controller/dwc/Kconfig, similar
to PCI_IMX6_HOST, PCI_IMX6_EP, and PCIE_QCOM?

> +
> +		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");
> +		}
> +	}
> +
>  	/* DON'T MOVE ME: must be enable before PHY init */
>  	ret = devm_regulator_get_enable_optional(dev, "vpcie3v3");
>  	if (ret < 0 && ret != -ENODEV)
> -		return dev_err_probe(dev, ret,
> -				     "failed to enable vpcie3v3 regulator\n");
> +		goto err_pwrctrl_off;
>  
>  	ret = rockchip_pcie_phy_init(rockchip);
>  	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "failed to initialize the phy\n");
> +		goto err_pwrctrl_off;
>  
>  	ret = reset_control_deassert(rockchip->rst);
>  	if (ret)
> @@ -883,6 +900,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>  deinit_phy:
>  	rockchip_pcie_phy_deinit(rockchip);
> +err_pwrctrl_off:
> +	if (data->mode == DW_PCIE_RC_TYPE) {
> +		pci_pwrctrl_power_off_devices(dev);
> +		pci_pwrctrl_destroy_devices(dev);
> +	}

[Severity: High]
Does calling pci_pwrctrl_destroy_devices(dev) unconditionally in
rockchip_pcie_probe() when ret == -EPROBE_DEFER prevent deferred child
pwrctrl devices from binding and re-triggering probe?

When pci_pwrctrl_power_on_devices(dev) is called in rockchip_pcie_probe(),
drivers/pci/pwrctrl/core.c:pci_pwrctrl_power_on_device() checks whether
each created child pwrctrl platform device has bound to its driver:

	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;
		}
	}

If a child pwrctrl device has not bound yet (for example because its
supplier deferred probe, its driver probes asynchronously, or its module is
not yet loaded), pci_pwrctrl_power_on_devices(dev) returns -EPROBE_DEFER
(or a subsequent step in rockchip_pcie_probe() such as
devm_regulator_get_enable_optional() or rockchip_pcie_phy_init() may return
-EPROBE_DEFER and jump to err_pwrctrl_off).

Both the error path of pci_pwrctrl_power_on_devices(dev) and the
err_pwrctrl_off cleanup block call pci_pwrctrl_destroy_devices(dev)
unconditionally, which unregisters the child pwrctrl platform devices via
of_device_unregister() and removes them from deferred_probe_pending_list so
they can never bind and re-trigger rockchip_pcie_probe().

Should pci_pwrctrl_destroy_devices(dev) only be called when
ret != -EPROBE_DEFER (as done in 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=1

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

[Shawn Lin]

Move the vpcie3v3 supply of the PCIe3x4 controller into a child node
describing the slot, so that the PCI pwrctrl framework manages it.

The child node is modelled as a PCI bridge, which is what dtc expects
for a node below a PCI host bridge, and carries the regulator that
powers the devices in the slot.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

 arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
index 8969b56f3063..80eb36e783e0 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>;
+	};
 };
 
 &pd_gpu {
-- 
2.25.1

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

[sashiko-bot]

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