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

Sherry Sun
2026-04-09T02:40:41Z

> Subject: Re: [PATCH V11 04/12] PCI: imx6: Add support for parsing the reset
> property in new Root Port binding
>
> On Wed, Apr 08, 2026 at 08:34:03AM +0000, Sherry Sun wrote:
> > > 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;
> >
>
> This check should be moved out of the if() condition. Other than this, the
> change looks good.

Ok, will do.

>
> > 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?
> >
>
> Just use mdelay() in your patch for now.

Ok, thanks!

Best Regards
Sherry