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

[Yureka Lilian]

On 7/7/26 11:25, Marc Zyngier wrote:
> On Mon, 06 Jul 2026 23:38:27 +0100,
> Yureka Lilian <yureka@xxxxxxxxxxxxxx> 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.
> >
> > Previously, a custom solution for powering on the WiFi and SD card
> > reader was proposed[1], but we can now use the new pci-pwrctrl-generic
> > driver for this purpose.
> >
> > Link[1]: https://lore.kernel.org/lkml/20220502093832.32778-4-marcan@xxxxxxxxx/
> nit: this paragraph and the accompanying link don't belong in the
> commit message and should be moved below the --- mark or even better,
> to the cover letter.
ack, will leave it out of the commit message of the individual commit in v2
> > Signed-off-by: Yureka Lilian <yureka@xxxxxxxxxxxxxx>
> > ---
> >   drivers/pci/controller/Kconfig      |  1 +
> >   drivers/pci/controller/pcie-apple.c | 16 ++++++++++++++++
> >   2 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index 2247709ef6d6..af64630d28fa 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..db038a9d4831 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,21 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> >   	if (WARN_ON(!pcie))
> >   		return -ENOENT;
> >   
> > +	ret = pci_pwrctrl_create_devices(pcie->dev);
> > +	if (ret) {
> > +		dev_err(pcie->dev, "Failed to create pwrctrl devices: %pe\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = pci_pwrctrl_power_on_devices(pcie->dev);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER) {
> > +			dev_err(pcie->dev, "Failed to power on devices: %pe\n", ret);
> > +			pci_pwrctrl_destroy_devices(pcie->dev);
> > +		}
> > +		return ret;
> > +	}
> > +
> Why is this done globally while the whole driver works on a per-port
> basis, and that the proposed DT updates are also per port?

pci_pwrctrl_power_on_devices takes a struct device as parameter, but
pcie-apple does not allocate device structs for the individual ports.
This could be changed of course. But since pci_pwrctrl_* operate on
the subnodes recursively, it works just fine this way.
Additionally, we would like to be sure all the endpoints can be powered
before we start initializing the individual ports. Otherwise we could
end up in a situation where some ports are initialized but others are
not when we realize some driver needed to power on the endpoints for
port n is not yet bound. There is no appropriate pci_pwrctrl API for
checking for the availability without changing the state, so this is
an additional reason to do it early.

> Thanks,
>
> 	M.