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

[Manivannan Sadhasivam]

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@xxxxxxx>
> ---
>  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);

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.

>  		}
>  	}
>  }
> @@ -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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்