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

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

[Mike Looijmans]

Support providing the PERST reset signal through a devicetree binding.
Thus the system no longer relies on external components to perform the
bus reset.

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.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

 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;
+	}
+
+	if (perst_gpio) {
+		usleep_range(10, 20); /* Assert the reset for ~10 us */
+		gpiod_set_value_cansleep(perst_gpio, 0);
+		usleep_range(1000, 2000);
+	}
 
-	if (xilinx_pcie_link_up(pcie))
+	if (!xilinx_pci_wait_link_up(pcie))
 		dev_info(dev, "PCIe Link is UP\n");
 	else
 		dev_info(dev, "PCIe Link is DOWN\n");
-- 
2.43.0


Met vriendelijke groet / kind regards,

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@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

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

[Manivannan Sadhasivam]

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?

> 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@amd.com/

Please take a look for reference.

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

> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
>  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.

> +	}
> +
> +	if (perst_gpio) {

This check is not needed as gpiod_set_value_cansleep() can accept NULL.

- Mani

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

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@amd.com/
>
> 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@topic.nl>
>> ---
>>
>>   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@topic.nl
W: www.topic.nl

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

[Bjorn Helgaas]

On Mon, Mar 24, 2025 at 04:46:55PM +0100, Mike Looijmans wrote:
> 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.

Use "PERST#" to include the polarity and match the spec usage.

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

And please include a specific spec reference, e.g., "PCIe r6.0, sec
x.y.z".