Re: [PATCH] pcie-xilinx: Support reset GPIO and wait for link-up status

[Mike Looijmans]

On 24-03-2025 08:49, Manivannan Sadhasivam wrote:
> On Fri, Mar 14, 2025 at 03:59:02PM +0100, Mike Looijmans wrote:
> > Support providing the PERST reset signal through a devicetree binding.
> Where is the bindings change?

Will add in v2


> > Thus the system no longer relies on external components to perform the
> > bus reset.
> There is a similar series for the CPM controller:
> https://lore.kernel.org/linux-pci/20250318092648.2298280-1-sai.krishna.musham@xxxxxxx/
>
> Please take a look for reference.

Seems similar indeed. (You may have noticed we've actually been using 
this patch for a few years)


> > When the driver loads, the transceiver may still be in the state of
> > setting up a link. Wait for that to complete before continuing. This
> > fixes that the PCIe core does not work when loading the PL bitstream
> > from userspace. There's only milliseconds between the FPGA boot and the
> > core initializing in that case, and the link won't be up yet. The design
> > works when the FPGA was programmed in the bootloader, as that will give
> > the system hundreds of milliseconds to boot.
> >
> > As the PCIe spec mentions about 120 ms time to establish a link, we'll
> > allow up to 200ms before giving up.
> This should be a separate change/patch.

Indeed, this is a separate issue, I'll split it in two parts.


> > Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
> > ---
> >
> >   drivers/pci/controller/pcie-xilinx.c | 25 ++++++++++++++++++++++++-
> >   1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
> > index 0b534f73a942..cd09deba0ddc 100644
> > --- a/drivers/pci/controller/pcie-xilinx.c
> > +++ b/drivers/pci/controller/pcie-xilinx.c
> > @@ -15,8 +15,10 @@
> >   #include <linux/irqdomain.h>
> >   #include <linux/kernel.h>
> >   #include <linux/init.h>
> > +#include <linux/iopoll.h>
> >   #include <linux/msi.h>
> >   #include <linux/of_address.h>
> > +#include <linux/of_gpio.h>
> >   #include <linux/of_pci.h>
> >   #include <linux/of_platform.h>
> >   #include <linux/of_irq.h>
> > @@ -126,6 +128,14 @@ static inline bool xilinx_pcie_link_up(struct xilinx_pcie *pcie)
> >   		XILINX_PCIE_REG_PSCR_LNKUP) ? 1 : 0;
> >   }
> >   
> > +static int xilinx_pci_wait_link_up(struct xilinx_pcie *pcie)
> > +{
> > +	u32 val;
> > +
> > +	return readl_poll_timeout(pcie->reg_base + XILINX_PCIE_REG_PSCR, val,
> > +			(val & XILINX_PCIE_REG_PSCR_LNKUP), 2000, 200000);
> > +}
> > +
> >   /**
> >    * xilinx_pcie_clear_err_interrupts - Clear Error Interrupts
> >    * @pcie: PCIe port information
> > @@ -492,8 +502,21 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie *pcie)
> >   static void xilinx_pcie_init_port(struct xilinx_pcie *pcie)
> >   {
> >   	struct device *dev = pcie->dev;
> > +	struct gpio_desc *perst_gpio;
> > +
> > +	perst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(perst_gpio)) {
> > +		dev_err(dev, "gpio request failed: %d\n", PTR_ERR(perst_gpio));
> > +		perst_gpio = NULL;
> No. If the GPIO is defined in DT and there is a problem in acquiring it, you
> should return the error.

Agree. And also, this should be moved to probe() so it can handle 
deferred properly.


> > +	}
> > +
> > +	if (perst_gpio) {
> This check is not needed as gpiod_set_value_cansleep() can accept NULL.

The check is to prevent the extra "sleep" calls. If the reset was 
performed externally (some bootloader or POR logic) the sleep calls 
should be skipped.


> - Mani
Thanks for your feedback.

--
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@xxxxxxxx
W: www.topic.nl