RE: [PATCH V2 1/8] PCI: imx6: Add skip_pwrctrl_off flag support

[Sherry Sun]

> Subject: Re: [PATCH V2 1/8] PCI: imx6: Add skip_pwrctrl_off flag support
> 
> On Wed, Jun 24, 2026 at 07:09:26AM +0000, Sherry Sun wrote:
> > > Subject: Re: [PATCH V2 1/8] PCI: imx6: Add skip_pwrctrl_off flag
> > > support
> > >
> > > On Tue, Jun 23, 2026 at 11:07:28AM +0800, Sherry Sun (OSS) wrote:
> > > > From: Sherry Sun <sherry.sun@xxxxxxx>
> > > >
> > > > Use dw_pcie_rp::skip_pwrctrl_off to avoid powering off devices
> > > > during suspend to preserve wakeup capability of the devices and
> > > > also not to power on the devices in the init path.
> > > > This allows controller power-off to be skipped when some devices(e.g.
> > > > M.2 cards key E without auxiliary power) required to support PCIe
> > > > L2 link state and wake-up mechanisms.
> > > >
> > > > Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx>
> > > > ---
> > > >  drivers/pci/controller/dwc/pci-imx6.c | 36
> > > > +++++++++++++++++----------
> > > >  1 file changed, 23 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 0fa716d1ed75..ff5a9565dbbf 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -1382,16 +1382,20 @@ static int imx_pcie_host_init(struct
> > > > dw_pcie_rp
> > > *pp)
> > > >  		}
> > > >  	}
> > > >
> > > > -	ret = pci_pwrctrl_create_devices(dev);
> > > > -	if (ret) {
> > > > -		dev_err(dev, "failed to create pwrctrl devices\n");
> > > > -		goto err_reg_disable;
> > > > +	if (!pci->suspended) {
> > > > +		ret = pci_pwrctrl_create_devices(dev);
> > >
> > > Is possible move pci_pwrctrl_create_devices() of
> > > pci_pwrctrl_create_devices
> > >
> > > and call it direct at probe() function, like other regulator_get function.
> > >
> >
> > Hi Frank,
> > That makes sense. However, if we move pci_pwrctrl_create_devices () to
> > probe(), we may need to add the following goto err_pwrctrl_destroy
> > path in imx_pcie_probe() to properly handle errors from
> > pci_pwrctrl_power_on_devices(), is that acceptable?
> 
> Can you add a API devm_pci_pwrctrl_create_devices() ?
> 

Hi Frank, we cannot unconditionally destroy the pwrctrl devices
when probing fails by using devm API.
Since we need to check the return value of
pci_pwrctrl_power_on_devices() for example EPROBE_DEFER to decide
whether to destroy the pwrctrl devices to avoid the deferred probe loop.

You can find more related discussion here.
https://lore.kernel.org/all/tutxwjciedqoje5wxvtin4h637auni5zzpvb7rtfg4uticxoux@yfl6xg7oht7t/

Best Regards
Sherry
> 
> >
> > @@ -1960,11 +1949,15 @@ static int imx_pcie_probe(struct
> platform_device *pdev)
> >         if (ret)
> >                 return ret;
> >
> > +       ret = pci_pwrctrl_create_devices(dev);
> > +       if (ret)
> > +               return dev_err_probe(dev, ret, "failed to create
> > + pwrctrl devices\n");
> > +
> >         pci->use_parent_dt_ranges = true;
> >         if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
> >                 ret = imx_add_pcie_ep(imx_pcie, pdev);
> >                 if (ret < 0)
> > -                       return ret;
> > +                       goto err_pwrctrl_destroy;
> >
> >                 /*
> >                  * FIXME: Only single Device (EPF) is supported due to
> > the @@ -1979,7 +1972,7 @@ static int imx_pcie_probe(struct
> platform_device *pdev)
> >                 pci->pp.use_atu_msg = true;
> >                 ret = dw_pcie_host_init(&pci->pp);
> >                 if (ret < 0)
> > -                       return ret;
> > +                       goto err_pwrctrl_destroy;
> >
> >                 if (pci_msi_enabled()) {
> >                         u8 offset = dw_pcie_find_capability(pci,
> > PCI_CAP_ID_MSI); @@ -1991,6 +1984,11 @@ static int
> imx_pcie_probe(struct platform_device *pdev)
> >         }
> >
> >         return 0;
> > +
> > +err_pwrctrl_destroy:
> > +       if (ret != -EPROBE_DEFER)
> > +               pci_pwrctrl_destroy_devices(dev);
> > +       return ret;
> >  }
> >
> > Best Regards
> > Sherry
> >
> > >
> > > > +		if (ret) {
> > > > +			dev_err(dev, "failed to create pwrctrl devices\n");
> > > > +			goto err_reg_disable;
> > > > +		}
> > > >  	}
> > > >
> > > > -	ret = pci_pwrctrl_power_on_devices(dev);
> > > > -	if (ret) {
> > > > -		dev_err(dev, "failed to power on pwrctrl devices\n");
> > > > -		goto err_pwrctrl_destroy;
> > > > +	if (!pp->skip_pwrctrl_off) {
> > > > +		ret = pci_pwrctrl_power_on_devices(dev);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "failed to power on pwrctrl devices\n");
> > > > +			goto err_pwrctrl_destroy;
> > > > +		}
> > > >  	}
> > > >
> > > >  	ret = imx_pcie_clk_enable(imx_pcie); @@ -1460,9 +1464,10 @@
> > > static
> > > > int imx_pcie_host_init(struct dw_pcie_rp *pp)
> > > >  err_clk_disable:
> > > >  	imx_pcie_clk_disable(imx_pcie);
> > > >  err_pwrctrl_power_off:
> > > > -	pci_pwrctrl_power_off_devices(dev);
> > > > +	if (!pp->skip_pwrctrl_off)
> > > > +		pci_pwrctrl_power_off_devices(dev);
> > > >  err_pwrctrl_destroy:
> > > > -	if (ret != -EPROBE_DEFER)
> > > > +	if (ret != -EPROBE_DEFER && !pci->suspended)
> > > >  		pci_pwrctrl_destroy_devices(dev);
> > > >  err_reg_disable:
> > > >  	if (imx_pcie->vpcie)
> > > > @@ -1482,7 +1487,8 @@ static void imx_pcie_host_exit(struct
> > > > dw_pcie_rp
> > > *pp)
> > > >  	}
> > > >  	imx_pcie_clk_disable(imx_pcie);
> > > >
> > > > -	pci_pwrctrl_power_off_devices(pci->dev);
> > > > +	if (!pci->pp.skip_pwrctrl_off)
> > > > +		pci_pwrctrl_power_off_devices(pci->dev);
> > > >  	if (imx_pcie->vpcie)
> > > >  		regulator_disable(imx_pcie->vpcie);
> > > >  }
> > > > @@ -1990,12 +1996,16 @@ static int imx_pcie_probe(struct
> > > > platform_device *pdev)  static void imx_pcie_shutdown(struct
> > > > platform_device *pdev)  {
> > > >  	struct imx_pcie *imx_pcie = platform_get_drvdata(pdev);
> > > > +	struct dw_pcie *pci = imx_pcie->pci;
> > > > +	struct dw_pcie_rp *pp = &pci->pp;
> > > >
> > > >  	/* bring down link, so bootloader gets clean state in case of reboot */
> > > >  	imx_pcie_assert_core_reset(imx_pcie);
> > > >  	imx_pcie_assert_perst(imx_pcie, true);
> > > > -	pci_pwrctrl_power_off_devices(&pdev->dev);
> > > > -	pci_pwrctrl_destroy_devices(&pdev->dev);
> > > > +	if (!pp->skip_pwrctrl_off)
> > > > +		pci_pwrctrl_power_off_devices(&pdev->dev);
> > > > +	if (!pci->suspended)
> > > > +		pci_pwrctrl_destroy_devices(&pdev->dev);
> > > >  }
> > > >
> > > >  static const struct imx_pcie_drvdata drvdata[] = {
> > > > --
> > > > 2.50.1
> > > >
> > > >