RE: [PATCH V11 04/12] PCI: imx6: Add support for parsing the reset property in new Root Port binding

Sherry Sun
2026-04-08T08:34:18Z

> On Tue, Apr 07, 2026 at 06:41:46PM +0800, Sherry Sun wrote:
> > The current DT binding for pci-imx6 specifies the 'reset-gpios'
> > property in the host bridge node. However, the PERST# signal logically
> > belongs to individual Root Ports rather than the host bridge itself.
> > This becomes important when supporting PCIe KeyE connector and PCI
> > power control framework for pci-imx6 driver, which requires properties
> > to be specified in Root Port nodes.
> >
> > Add support for parsing 'reset-gpios' from Root Port child nodes using
> > the common helper pci_host_common_parse_ports(), and update the reset
> > GPIO handling to use the parsed port list from bridge->ports. To
> > maintain DT backwards compatibility, fallback to the legacy method of
> > parsing the host bridge node if the reset property is not present in
> > the Root Port node.
> >
> > Since now the reset GPIO is obtained with GPIOD_ASIS flag, it may be
> > in input mode, using gpiod_direction_output() instead of
> > gpiod_set_value_cansleep() to ensure the reset GPIO is properly
> > configured as output before setting its value.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 75
> > +++++++++++++++++++++------
> > 1 file changed, 60 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index d99da7e42590..dd8f9c0fcec4 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -34,6 +34,7 @@
> > #include <linux/pm_runtime.h>
> >
> > #include "../../pci.h"
> > +#include "../pci-host-common.h"
> > #include "pcie-designware.h"
> >
> > #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> > @@ -152,7 +153,6 @@ struct imx_lut_data {
> >
> > struct imx_pcie {
> > struct dw_pcie *pci;
> > - struct gpio_desc *reset_gpiod;
> > struct clk_bulk_data *clks;
> > int num_clks;
> > bool supports_clkreq;
> > @@ -1224,6 +1224,32 @@ static void imx_pcie_disable_device(struct
> pci_host_bridge *bridge,
> > imx_pcie_remove_lut(imx_pcie, pci_dev_id(pdev)); }
> >
> > +static int imx_pcie_parse_legacy_binding(struct imx_pcie *pcie) {
> > + struct device *dev = pcie->pci->dev;
> > + struct pci_host_bridge *bridge = pcie->pci->pp.bridge;
> > + struct pci_host_port *port;
> > + struct gpio_desc *reset;
> > +
> > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
> > + if (IS_ERR(reset))
> > + return PTR_ERR(reset);
> > +
> > + if (!reset)
> > + return 0;
> > +
> > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > + if (!port)
> > + return -ENOMEM;
> > +
> > + port->reset = reset;
> > + INIT_LIST_HEAD(&port->list);
> > + list_add_tail(&port->list, &bridge->ports);
> > +
> > + return devm_add_action_or_reset(dev,
> pci_host_common_delete_ports,
> > + &bridge->ports);
> > +}
> > +
> > static void imx_pcie_vpcie_aux_disable(void *data) {
> > struct regulator *vpcie_aux = data;
> > @@ -1233,13 +1259,22 @@ static void imx_pcie_vpcie_aux_disable(void
> > *data)
> >
> > static void imx_pcie_assert_perst(struct imx_pcie *imx_pcie, bool
> > assert) {
> > - if (assert) {
> > - gpiod_set_value_cansleep(imx_pcie->reset_gpiod, 1);
> > - } else {
> > - if (imx_pcie->reset_gpiod) {
> > - msleep(PCIE_T_PVPERL_MS);
> > - gpiod_set_value_cansleep(imx_pcie->reset_gpiod, 0);
> > - msleep(PCIE_RESET_CONFIG_WAIT_MS);
> > + struct dw_pcie *pci = imx_pcie->pci;
> > + struct pci_host_bridge *bridge = pci->pp.bridge;
> > + struct pci_host_port *port;
> > +
> > + if (!bridge)
> > + return;
> > +
> > + list_for_each_entry(port, &bridge->ports, list) {
> > + if (assert) {
> > + gpiod_direction_output(port->reset, 1);
> > + } else {
> > + if (port->reset) {
> > + msleep(PCIE_T_PVPERL_MS);
> > + gpiod_direction_output(port->reset, 0);
> > + msleep(PCIE_RESET_CONFIG_WAIT_MS);
> > + }
>
> Sashiko flagged this loop:
>
> ```
> Does this loop multiply the initialization delays?
> If a controller has multiple Root Ports, the msleep calls will run sequentially
> for each port, linearly increasing the delay. Could we optimize this by
> asserting all reset GPIOs, waiting the pre-delay once, de-asserting all GPIOs,
> and waiting the post-delay once for the entire bus?
> ```
>
> Maybe you should do:
>
> if (!list_empty(&bridge->ports) && !assert)
> msleep(PCIE_T_PVPERL_MS);
>
> list_for_each_entry(port, &bridge->ports, list) {
> ...
> gpiod_direction_output(port->reset, 0);
> ...
> }
>
> if (!list_empty(&bridge->ports) && !assert)
> msleep(PCIE_RESET_CONFIG_WAIT_MS);
>

Hi Mani, I think the code below looks clearer, is that ok for you?

if (assert) {
list_for_each_entry(port, &bridge->ports, list)
gpiod_direction_output(port->reset, 1);
} else {
if (list_empty(&bridge->ports))
return;

msleep(PCIE_T_PVPERL_MS);
list_for_each_entry(port, &bridge->ports, list)
gpiod_direction_output(port->reset, 0);
msleep(PCIE_RESET_CONFIG_WAIT_MS);
}

> And then this:
>
> ```
> Also, since this function is called from imx_pcie_resume_noirq, which
> executes with hardware interrupts disabled, does the use of msleep here
> trigger a 'sleeping while atomic' bug?
> ```
>
> This is a valid concern. You should use mdelay(). But I'd recommend switching
> to IRQ enabled callback, resume() instead. There is no complelling reason to
> use resume_noirq() in this driver and adding delays in noirq() callbacks is not
> recommended as it may increase the overall system resume time.
>
> I will submit a separate series to convert dw_pcie_resume_noirq() and its
> callers to IRQ enabled callbacks since this dw_pcie_resume_noirq() could
> potentially cause delay up to 1sec.

Yes, this is not a new bug introduced by this patch. I agree we should covert the
convert dw_pcie_resume_noirq() and the caller to IRQ enabled callbacks to fix
this in a separate patch series.
For now, should I leave it as is, or switch to mdelay in this patch?

>
> > }
> > }
> > }
> > @@ -1249,8 +1284,25 @@ static int imx_pcie_host_init(struct dw_pcie_rp
> *pp)
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > struct device *dev = pci->dev;
> > struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> > + struct pci_host_bridge *bridge = pp->bridge;
> > int ret;
> >
> > + if (bridge && list_empty(&bridge->ports)) {
> > + /* Parse Root Port nodes if present */
> > + ret = pci_host_common_parse_ports(dev, bridge);
> > + if (ret) {
> > + if (ret != -ENOENT) {
> > + dev_err(dev, "Failed to parse Root Port
> nodes: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + /* Fallback to legacy binding for DT backwards
> compatibility */
> > + ret = imx_pcie_parse_legacy_binding(imx_pcie);
>
> This is also flagged by Sashiko:
>
> ```
> Could this error handling corrupt the port state and trigger an invalid legacy
> fallback?
>
> If a device tree defines multiple Root Ports and one lacks the optional reset
> GPIO, pci_host_common_parse_ports returns -ENOENT. This causes the code
> to fall back to imx_pcie_parse_legacy_binding.
>
> Since the already-parsed child ports remain in bridge->ports without rollback,
> the legacy host bridge GPIO will be appended alongside them.
> Valid child nodes are skipped, and both child and legacy GPIOs will be toggled
> simultaneously.
> ```
>
> You should try to cleanup Root Port resources if
> pci_host_common_parse_ports() fails with -ENOENT.

Sure, I will call pci_host_common_delete_ports() to clean up any partially parsed
Root Port resources before falling back to legacy binding.

Best Regards
Sherry