Re: [PATCH v2 2/3] PCI: apple: Integrate pwrctrl API

[Janne Grunau]

On Wed, Nov 04, 2026 at 12:40:30PM +0100, Yureka Lilian wrote:
> Integrate the PCI pwrctrl framework into the Apple PCIe host driver to
> provide standardized power management for PCI devices.
> 
> Notably, this allows enabling powering on the WiFi, SD card reader on
> various Macs by means of the pwrctrl framework before probing the ports.
> 
> Signed-off-by: Yureka Lilian <yureka@xxxxxxxxxxxxxx>
> ---
>  drivers/pci/controller/Kconfig      |  1 +
>  drivers/pci/controller/pcie-apple.c | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index d246bbe37948..791892a6c0c6 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -46,6 +46,7 @@ config PCIE_APPLE
>  	depends on OF
>  	depends on PCI_MSI
>  	select PCI_HOST_COMMON
> +	select PCI_PWRCTRL_GENERIC
>  	select IRQ_MSI_LIB
>  	help
>  	  Say Y here if you want to enable PCIe controller support on Apple
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index c2cffc0659f4..2d32092e2122 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -30,6 +30,7 @@
>  #include <linux/msi.h>
>  #include <linux/of_irq.h>
>  #include <linux/pci-ecam.h>
> +#include <linux/pci-pwrctrl.h>
>  
>  #include "pci-host-common.h"
>  
> @@ -825,6 +826,28 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  	if (WARN_ON(!pcie))
>  		return -ENOENT;
>  
> +	// Ensure all ports can be powered on before attempting initialization
> +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		ret = pci_pwrctrl_create_device(of_port, pcie->dev);
> +		if (ret) {
> +			dev_err(pcie->dev, "Failed to create pwrctrl device: %pe\n", ERR_PTR(ret));

This should power down devices it already powered up and destroy the
created devices. As far as I can see -EINVAL is the only possible error
so the platform devices should be cleaned up

> +			return ret;
> +		}
> +
> +		ret = pci_pwrctrl_power_on_device(of_port);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER) {

-EPROBE_DEFER can be persistent, devices should be powered and
destroyed here as well.

> +				dev_err(pcie->dev,
> +					"Failed to power on device: %pe\n",
> +					ERR_PTR(ret));
> +				for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +					pci_pwrctrl_destroy_device(of_port);

this misses to power down already powered up devices.

One way to avoid the need to power down devices after
pci_pwrctrl_create_device() failures would be to split the loop into
two.

The power handling is now done on a per port basis as asked in v1 but it
is more or less a re-implementation of pci_pwrctrl_create_devices(),
pci_pwrctrl_destroy_devices() and pci_pwrctrl_power_on_devices().
For pcie-apple those are essentially loops over the the available ports
with error handling.

One annoying side effect of this change. I a single device fails to
power-on we will break ethernet on desktop Macs although it does not
need power control. So maybe ignoring errors except -EPROBE_DEFER would
be the better implementation. -EPROBE_DEFER itself is still a problem as
the power control for some ports are a optional dependency for other
ports.

Janne