Message-ID: <geygcp65goaajdy4lxwnux6qw6v2lbbhphk7yqnlsfrutpgszm@vnwqt5ifhmzo>
Date: Thu, 5 Feb 2026 10:41:59 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Frank Li <Frank.li@....com>
Cc: Sherry Sun <sherry.sun@....com>, hongxing.zhu@....com, 
	l.stach@...gutronix.de, bhelgaas@...gle.com, lpieralisi@...nel.org, 
	kwilczynski@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	s.hauer@...gutronix.de, festevam@...il.com, imx@...ts.linux.dev, kernel@...gutronix.de, 
	linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 02/10] PCI: imx6: Add support for parsing the reset
 property in new Root Port binding

On Wed, Feb 04, 2026 at 10:25:28AM -0500, Frank Li wrote:
> 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@....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.
> 

I don't think there is a need for a common API for cleanup. Moreover,
devm_add_action_or_reset() will only be executed when the 'struct device' gets
freed, which only happens during system shutdown. Until then, the cleanup
handler won't be invoked.

It is probably OK to free the resources like kzalloced memory with
devm_add_action_or_reset(), but for any other cases like performing resets or
other actions that unwind the prior probe path, explicit err labels should be
used.

Though in this driver, only ports are freed in the handler, someone may abuse it
in the future to perform other cleanups as well. This is why I always perfer
using explicit err labels.

Hope this clarifies!

- Mani

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

Powered by blists - more mailing lists