Re: [PATCH v4 0/8] PCI/pwrctrl: Major rework to integrate pwrctrl devices with controller drivers

[Sean Anderson <sean.anderson@seco.com>]

On 1/5/26 08:55, Manivannan Sadhasivam via B4 Relay wrote:
> Hi,

I asked substantially similar questions on v2, but since I never got a
response I want to reiterate them on v4 to make sure they don't get
lost.

> This series provides a major rework for the PCI power control (pwrctrl)
> framework to enable the pwrctrl devices to be controlled by the PCI controller
> drivers.
> 
> Problem Statement
> =================
> 
> Currently, the pwrctrl framework faces two major issues:
> 
> 1. Missing PERST# integration
> 2. Inability to properly handle bus extenders such as PCIe switch devices
> 
> First issue arises from the disconnect between the PCI controller drivers and
> pwrctrl framework. At present, the pwrctrl framework just operates on its own
> with the help of the PCI core. The pwrctrl devices are created by the PCI core
> during initial bus scan and the pwrctrl drivers once bind, just power on the
> PCI devices during their probe(). This design conflicts with the PCI Express
> Card Electromechanical Specification requirements for PERST# timing. The reason
> is, PERST# signals are mostly handled by the controller drivers and often
> deasserted even before the pwrctrl drivers probe. According to the spec, PERST#
> should be deasserted only after power and reference clock to the device are
> stable, within predefined timing parameters.
> 
> The second issue stems from the PCI bus scan completing before pwrctrl drivers
> probe. This poses a significant problem for PCI bus extenders like switches
> because the PCI core allocates upstream bridge resources during the initial
> scan. If the upstream bridge is not hotplug capable, resources are allocated
> only for the number of downstream buses detected at scan time, which might be
> just one if the switch was not powered and enumerated at that time. Later, when
> the pwrctrl driver powers on and enumerates the switch, enumeration fails due to
> insufficient upstream bridge resources.

OK, so to clarify the problem is an architecture like

    RP
    |-- Bridge 1 (automatic)
    |   |-- Device 1
    |   `-- Bridge 2 (needs pwrseq)
    |       `-- Device 2
    `-- Bridge 3 (automatic)
        `-- Device 3

where Bridge 2 has a devicetree node with a pwrseq binding? So we do the
initial scan and allocate resources for bridge/devices 1 and 3 with the
resources for bridge 3 immediately above those for bridge 1. Then when
bridge 2 shows up we can't resize bridge 1's windows since bridge 3's
windows are in the way?

But is it even valid to have a pwrseq node on bridge 2 without one on
bridge 1? If bridge 1 is automatically controlled, then I would expect
bridge 2 to be as well. E.g. I would expect bridge 2's reset sequence to
be controlled by the secondary bus reset bit in bridge 1's bridge
control register.

And a very similar architecture like

    RP
    |-- Bridge 4 (pwrseq)
    |   |-- Device 4
    `-- Bridge 5 (automatic)
        `-- Device 5

has no problems since the resources for bridge 4 can be allocated above
those for bridge 5 whenever it shows up.

These problems seem very similar to what hotplug bridges have to handle
(except that we (usually) only need to do one hotplug per boot). So
maybe we should set is_hotplug_bridge on bridges with a pwrseq node.
That way they'll get resources distributed for when the downstream port
shows up. As an optimization, we could then release those resources once
the downstream port is scanned.

> Proposal
> ========
> 
> This series addresses both issues by introducing new individual APIs for pwrctrl
> device creation, destruction, power on, and power off operations. Controller
> drivers are expected to invoke these APIs during their probe(), remove(),
> suspend(), and resume() operations.

(just for the record)

I think the existing design is quite elegant, since the operations
associated with the bridge correspond directly to device lifecycle
operations. It also avoids problems related to the root port trying to
look up its own child (possibly missing a driver) during probe.

> This integration allows better coordination
> between controller drivers and the pwrctrl framework, enabling enhanced features
> such as D3Cold support.


I think this should be handled by the power sequencing driver,
especially as there are timing requirements for the other resources
referenced to PERST? If we are going to touch each driver, it would
be much better to consolidate things by removing the ad-hoc PERST
support.

Different drivers control PERST in various ways, but I think this can
be abstracted behind a GPIO controller (if necessary for e.g. MMIO-based
control). If there's no reset-gpios property in the pwrseq node then we
could automatically look up the GPIO on the root port.

> The original design aimed to avoid modifying controller drivers for pwrctrl
> integration. However, this approach lacked scalability because different
> controllers have varying requirements for when devices should be powered on. For
> example, controller drivers require devices to be powered on early for
> successful PHY initialization.

Can you elaborate on this? Previously you said

| Some platforms do LTSSM during phy_init(), so they will fail if the
| device is not powered ON at that time.

What do you mean by "do LTSSM during phy_init()"? Do you have a specific
driver in mind?

I would expect that the LTSSM would remain in the Detect state until the
pwrseq driver is being probed.

> By using these explicit APIs, controller drivers gain fine grained control over
> their associated pwrctrl devices.
> 
> This series modified the pcie-qcom driver (only consumer of pwrctrl framework)
> to adopt to these APIs and also removed the old pwrctrl code from PCI core. This
> could be used as a reference to add pwrctrl support for other controller drivers
> also.
> 
> For example, to control the 3.3v supply to the PCI slot where the NVMe device is
> connected, below modifications are required:
> 
> Devicetree
> ----------
> 
> 	// In SoC dtsi:
> 
> 	pci@1bf8000 { // controller node
> 		...
> 		pcie1_port0: pcie@0 { // PCI Root Port node
> 			compatible = "pciclass,0604"; // required for pwrctrl
> 							 driver bind
> 			...
> 		};
> 	};
> 
> 	// In board dts:
> 
> 	&pcie1_port0 {
> 		reset-gpios = <&tlmm 152 GPIO_ACTIVE_LOW>; // optional
> 		vpcie3v3-supply = <&vreg_nvme>; // NVMe power supply
> 	};
> 
> Controller driver
> -----------------
> 
> 	// Select PCI_PWRCTRL_SLOT in controller Kconfig
> 
> 	probe() {
> 		...
> 		// Initialize controller resources
> 		pci_pwrctrl_create_devices(&pdev->dev);
> 		pci_pwrctrl_power_on_devices(&pdev->dev);
> 		// Deassert PERST# (optional)
> 		...
> 		pci_host_probe(); // Allocate host bridge and start bus scan
> 	}
> 
> 	suspend {
> 		// PME_Turn_Off broadcast
> 		// Assert PERST# (optional)
> 		pci_pwrctrl_power_off_devices(&pdev->dev);
> 		...
> 	}
> 
> 	resume {
> 		...
> 		pci_pwrctrl_power_on_devices(&pdev->dev);
> 		// Deassert PERST# (optional)
> 	}
> 
> I will add a documentation for the pwrctrl framework in the coming days to make
> it easier to use.
> 
> Testing
> =======
> 
> This series is tested on the Lenovo Thinkpad T14s laptop based on Qcom X1E
> chipset and RB3Gen2 development board with TC9563 switch based on Qcom QCS6490
> chipset.
> 
> **NOTE**: With this series, the controller driver may undergo multiple probe
> deferral if the pwrctrl driver was not available during the controller driver
> probe. This is pretty much required to avoid the resource allocation issue. I
> plan to replace probe deferral with blocking wait in the coming days.

You can only do a blocking wait after deferring at least once, since the
root port may be probed synchronously during boot. I really think this
is rather messy and something we should avoid architecturally while we
have the chance.

--Sean

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> Changes in v4:
> - Used platform_device_put()
> - Changed the return value of power_off() callback to 'int'
> - Splitted patch 6 into two and reworded the commit message
> - Collected tags
> - Link to v3: https://lore.kernel.org/r/20251229-pci-pwrctrl-rework-v3-0-c7d5918cd0db@oss.qualcomm.com
> 
> Changes in v3:
> - Integrated TC9563 change
> - Reworked the power_on API to properly power off the devices in error path
> - Fixed the error path in pcie-qcom.c to not destroy pwrctrl devices during
>   probe deferral
> - Rebased on top of pci/controller/dwc-qcom branch and dropped the PERST# patch
> - Added a patch for TC9563 to fix the refcount dropping for i2c adapter
> - Added patches to drop the assert_perst callback and rename the PERST# helpers in
>   pcie-qcom.c
> - Link to v2: https://lore.kernel.org/r/20251216-pci-pwrctrl-rework-v2-0-745a563b9be6@oss.qualcomm.com
> 
> Changes in v2:
> - Exported of_pci_supply_present() API
> - Demoted the -EPROBE_DEFER log to dev_dbg()
> - Collected tags and rebased on top of v6.19-rc1
> - Link to v1: https://lore.kernel.org/r/20251124-pci-pwrctrl-rework-v1-0-78a72627683d@oss.qualcomm.com
> 
> ---
> Krishna Chaitanya Chundru (1):
>       PCI/pwrctrl: Add APIs for explicitly creating and destroying pwrctrl devices
> 
> Manivannan Sadhasivam (7):
>       PCI/pwrctrl: tc9563: Use put_device() instead of i2c_put_adapter()
>       PCI/pwrctrl: Add 'struct pci_pwrctrl::power_{on/off}' callbacks
>       PCI/pwrctrl: Add APIs to power on/off the pwrctrl devices
>       PCI/pwrctrl: Switch to the new pwrctrl APIs
>       PCI: qcom: Drop the assert_perst() callbacks
>       PCI: Drop the assert_perst() callback
>       PCI: qcom: Rename PERST# assert/deassert helpers for uniformity
> 
>  drivers/pci/bus.c                                 |  19 --
>  drivers/pci/controller/dwc/pcie-designware-host.c |   9 -
>  drivers/pci/controller/dwc/pcie-designware.h      |   9 -
>  drivers/pci/controller/dwc/pcie-qcom.c            |  54 +++--
>  drivers/pci/of.c                                  |   1 +
>  drivers/pci/probe.c                               |  59 -----
>  drivers/pci/pwrctrl/core.c                        | 260 ++++++++++++++++++++--
>  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c          |  30 ++-
>  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c          |  48 ++--
>  drivers/pci/pwrctrl/slot.c                        |  48 ++--
>  drivers/pci/remove.c                              |  20 --
>  include/linux/pci-pwrctrl.h                       |  16 +-
>  include/linux/pci.h                               |   1 -
>  13 files changed, 367 insertions(+), 207 deletions(-)
> ---
> base-commit: 3e7f562e20ee87a25e104ef4fce557d39d62fa85
> change-id: 20251124-pci-pwrctrl-rework-c91a6e16c2f6
> prerequisite-message-id: 20251126081718.8239-1-mani@kernel.org
> prerequisite-patch-id: db9ff6c713e2303c397e645935280fd0d277793a
> prerequisite-patch-id: b5351b0a41f618435f973ea2c3275e51d46f01c5
> 
> Best regards,