Re: [PATCH V3 02/10] PCI: imx6: Add support for parsing the reset property in new Root Port binding

[Frank Li <Frank.li@nxp.com>]

On Wed, Feb 04, 2026 at 05:58:33PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Feb 03, 2026 at 12:43:17PM -0500, Frank Li wrote:
> > On Tue, Feb 03, 2026 at 06:38:18PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Feb 03, 2026 at 09:56:06AM +0800, Sherry Sun wrote:
> > > > DT binding allows specifying 'reset' property in both host bridge and
> > > > Root Port nodes, but specifying in the host bridge node is marked as
> > > > deprecated. So add support for parsing the new binding that uses
> > > > 'reset-gpios' property for PERST#.
> > > >
> > > > The initial idea is to add the PCIe M.2 KeyE connector support and PCI
> > > > power control framework to the pcie-imx6 driver. Since the new
> > > > M.2/pwrctrl model is implemented based on Root Ports and requires the
> > > > pwrctrl driver to bind to a Root Port device, we need to introduce a
> > > > Root Port child node on i.MX boards that provide an M.2 connector.
> > > >
> > > > To follow a more standardized DT structure, it also makes sense to move
> > > > the reset-gpios and wake-gpios properties into the Root Port node. These
> > > > signals logically belong to the Root Port rather than the host bridge,
> > > > and placing them there aligns with the new M.2/pwrctrl model.
> > > >
> > > > 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.
> > > >
> > > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pci-imx6.c | 123 +++++++++++++++++++++++---
> > > >  1 file changed, 109 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index a5b8d0b71677..e3ba68976bee 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -148,10 +148,15 @@ struct imx_lut_data {
> > > >  	u32 data2;
> > > >  };
> > > >
> > > > +struct imx_pcie_port {
> > > > +	struct list_head	list;
> > > > +	struct gpio_desc	*reset;
> > > > +};
> > > > +
> > >
> > > I'd love to abstract the Root Port properties in a generic struct so that we can
> > > introduce generic APIs to parse the ports. But I'm not asking you to implement
> > > it :)
> > >
> >
> > good idea, where is good place to put it?
> >
>
> drivers/pci/controller/pci-host-common.c
>
> > > >
> > > > @@ -1688,12 +1771,24 @@ static int imx_pcie_probe(struct platform_device *pdev)
> > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > >  	}
> > > >
> > > > -	/* Fetch GPIOs */
> > > > -	imx_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > > -	if (IS_ERR(imx_pcie->reset_gpiod))
> > > > -		return dev_err_probe(dev, PTR_ERR(imx_pcie->reset_gpiod),
> > > > -				     "unable to get reset gpio\n");
> > > > -	gpiod_set_consumer_name(imx_pcie->reset_gpiod, "PCIe reset");
> > > > +	ret = imx_pcie_parse_ports(imx_pcie);
> > > > +	if (ret) {
> > > > +		if (ret != -ENOENT)
> > > > +			return dev_err_probe(dev, ret, "Failed to parse Root Port: %d\n", ret);
> > > > +
> > > > +		/*
> > > > +		 * In the case of properties not populated in Root Port node,
> > > > +		 * fallback to the legacy method of parsing the Host Bridge
> > > > +		 * node. This is to maintain DT backwards compatibility.
> > > > +		 */
> > > > +		ret = imx_pcie_parse_legacy_binding(imx_pcie);
> > > > +		if (ret)
> > > > +			return dev_err_probe(dev, ret, "Unable to get reset gpio: %d\n", ret);
> > > > +	}
> > > > +
> > > > +	ret = devm_add_action_or_reset(dev, imx_pcie_delete_ports, imx_pcie);
> > > > +	if (ret)
> > > > +		return ret;
> > >
> > > I'd prefer to do it in err labels.
> >
> > we just removed err label and module remove.
> >
>
> This driver is always built-in. So the only way 'struct dev' will get freed is
> when probe fails. The usual pattern within the host controller drivers is to use
> err label to do the cleanup not devm_add_action_or_reset() (there are some
> exceptions though).
>
> It just helps us to keep uniformity across the drivers.

Thanks, but err label is quite easy to make mistake. More and more function
support devm_*. I think if well design common API at pci-host-common.c,
goto can be avoid.

Frank

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