[PATCH v2 1/3] pcie-xilinx: Wait for link-up status during initialization

[PATCH v2 1/3] pcie-xilinx: Wait for link-up status during initialization

[Mike Looijmans]

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
only worked when the FPGA was programmed in the bootloader, as that will
give the system hundreds of milliseconds to boot.

As the PCIe spec allows up to 100 ms time to establish a link, we'll
allow up to 200ms before giving up.

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

Changes in v2:
Split into "reset GPIO" and "wait for link" patches
Add timeout explanation

 drivers/pci/controller/pcie-xilinx.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index 0b534f73a942..2e59b91f43e0 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -15,6 +15,7 @@
 #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_pci.h>
@@ -126,6 +127,19 @@ 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;
+
+	/*
+	 * PCIe r6.0, sec 6.6.1 provides 100ms timeout. Since this is FPGA
+	 * fabric, we're more lenient and allow 200 ms for link training.
+	 */
+	return readl_poll_timeout(pcie->reg_base + XILINX_PCIE_REG_PSCR, val,
+			(val & XILINX_PCIE_REG_PSCR_LNKUP), 2 * USEC_PER_MSEC,
+			200 * USEC_PER_MSEC);
+}
+
 /**
  * xilinx_pcie_clear_err_interrupts - Clear Error Interrupts
  * @pcie: PCIe port information
@@ -493,7 +507,7 @@ static void xilinx_pcie_init_port(struct xilinx_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 
-	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

[PATCH v2 2/3] dt-bindings: PCI: xilinx-pcie: Add reset-gpios for PERST#

[Mike Looijmans]

Introduce optional `reset-gpios` property to enable GPIO-based control
of the PCIe root port PERST# signal, as described in pci.txt.

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

Changes in v2:
Add binding for reset-gpios

 .../devicetree/bindings/pci/xlnx,axi-pcie-host.yaml          | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
index fb87b960a250..2b0fabdd5e16 100644
--- a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
@@ -28,6 +28,9 @@ properties:
           ranges for the PCI memory regions (I/O space region is not
           supported by hardware)
 
+  reset-gpios:
+    maxItems: 1
+
   "#interrupt-cells":
     const: 1
 
@@ -63,6 +66,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/interrupt-controller/irq.h>
 
@@ -80,6 +84,7 @@ examples:
                         <0 0 0 3 &pcie_intc 3>,
                         <0 0 0 4 &pcie_intc 4>;
         ranges = <0x02000000 0 0x60000000 0x60000000 0 0x10000000>;
+        reset-gpios = <&gpio0 9 GPIO_ACTIVE_LOW>;
         pcie_intc: interrupt-controller {
             interrupt-controller;
             #address-cells = <0>;
-- 
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 v2 2/3] dt-bindings: PCI: xilinx-pcie: Add reset-gpios for PERST#

[Krzysztof Kozlowski]

On Tue, Mar 25, 2025 at 08:18:26AM +0100, Mike Looijmans wrote:
> Introduce optional `reset-gpios` property to enable GPIO-based control
> of the PCIe root port PERST# signal, as described in pci.txt.

Drop pci.txt, we don't use TXT bindings anymore.

> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
> Changes in v2:
> Add binding for reset-gpios

So what was in v1? Empty patch?


> 
>  .../devicetree/bindings/pci/xlnx,axi-pcie-host.yaml          | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
> index fb87b960a250..2b0fabdd5e16 100644
> --- a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
> @@ -28,6 +28,9 @@ properties:
>            ranges for the PCI memory regions (I/O space region is not
>            supported by hardware)
>  
> +  reset-gpios:
> +    maxItems: 1

Why do you need it? It's already there, in PCI schemas, isn't it?

Why is this patch needed?

Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: PCI: xilinx-pcie: Add reset-gpios for PERST#

[Mike Looijmans]

On 25-03-2025 09:17, Krzysztof Kozlowski wrote:
> On Tue, Mar 25, 2025 at 08:18:26AM +0100, Mike Looijmans wrote:
>> Introduce optional `reset-gpios` property to enable GPIO-based control
>> of the PCIe root port PERST# signal, as described in pci.txt.
> Drop pci.txt, we don't use TXT bindings anymore.
>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>
>> Changes in v2:
>> Add binding for reset-gpios
> So what was in v1? Empty patch?

Feedback on v1 was that I had to add bindings documentation...


>>   .../devicetree/bindings/pci/xlnx,axi-pcie-host.yaml          | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
>> index fb87b960a250..2b0fabdd5e16 100644
>> --- a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
>> @@ -28,6 +28,9 @@ properties:
>>             ranges for the PCI memory regions (I/O space region is not
>>             supported by hardware)
>>   
>> +  reset-gpios:
>> +    maxItems: 1
> Why do you need it? It's already there, in PCI schemas, isn't it?
>
> Why is this patch needed?

Apparently not needed then, sorry for the noise.



>
> Best regards,
> Krzysztof
>

-- 
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 v2 2/3] dt-bindings: PCI: xilinx-pcie: Add reset-gpios for PERST#

[Manivannan Sadhasivam]

On Wed, Mar 26, 2025 at 12:57:44PM +0100, Mike Looijmans wrote:
> On 25-03-2025 09:17, Krzysztof Kozlowski wrote:
> > On Tue, Mar 25, 2025 at 08:18:26AM +0100, Mike Looijmans wrote:
> > > Introduce optional `reset-gpios` property to enable GPIO-based control
> > > of the PCIe root port PERST# signal, as described in pci.txt.
> > Drop pci.txt, we don't use TXT bindings anymore.
> > 
> > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> > > ---
> > > 
> > > Changes in v2:
> > > Add binding for reset-gpios
> > So what was in v1? Empty patch?
> 
> Feedback on v1 was that I had to add bindings documentation...
> 
> 
> > >   .../devicetree/bindings/pci/xlnx,axi-pcie-host.yaml          | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
> > > index fb87b960a250..2b0fabdd5e16 100644
> > > --- a/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/xlnx,axi-pcie-host.yaml
> > > @@ -28,6 +28,9 @@ properties:
> > >             ranges for the PCI memory regions (I/O space region is not
> > >             supported by hardware)
> > > +  reset-gpios:
> > > +    maxItems: 1
> > Why do you need it? It's already there, in PCI schemas, isn't it?
> > 
> > Why is this patch needed?
> 
> Apparently not needed then, sorry for the noise.
> 

That's my bad. I missed that this property is defined in the common schema.
Another source of confusion if you keep schemas in two different places.

- Mani

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

[PATCH v2 3/3] pcie-xilinx: Support reset GPIO for PERST#

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

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

Changes in v2:
Split into "reset GPIO" and "wait for link" patches
Handle GPIO defer and/or errors

 drivers/pci/controller/pcie-xilinx.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index 2e59b91f43e0..e191ab95d669 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -18,6 +18,7 @@
 #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>
@@ -577,11 +578,17 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct xilinx_pcie *pcie;
 	struct pci_host_bridge *bridge;
+	struct gpio_desc *perst_gpio;
 	int err;
 
 	if (!dev->of_node)
 		return -ENODEV;
 
+	perst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(perst_gpio))
+		return dev_err_probe(dev, PTR_ERR(perst_gpio),
+				     "reset-gpios request failed\n");
+
 	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
 	if (!bridge)
 		return -ENODEV;
@@ -596,6 +603,13 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	if (perst_gpio) {
+		usleep_range(10, 20); /* Assert the reset for ~10 us */
+		gpiod_set_value_cansleep(perst_gpio, 0);
+		/* Initial delay to provide endpoint time to restart */
+		usleep_range(1000, 2000);
+	}
+
 	xilinx_pcie_init_port(pcie);
 
 	err = xilinx_pcie_init_irq_domain(pcie);
-- 
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 v2 3/3] pcie-xilinx: Support reset GPIO for PERST#

[Manivannan Sadhasivam]

On Tue, Mar 25, 2025 at 08:18:27AM +0100, Mike Looijmans wrote:
> Support providing the PERST# reset signal through a devicetree binding.
> Thus the system no longer relies on external components to perform the
> bus reset.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
> Changes in v2:
> Split into "reset GPIO" and "wait for link" patches
> Handle GPIO defer and/or errors
> 
>  drivers/pci/controller/pcie-xilinx.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
> index 2e59b91f43e0..e191ab95d669 100644
> --- a/drivers/pci/controller/pcie-xilinx.c
> +++ b/drivers/pci/controller/pcie-xilinx.c
> @@ -18,6 +18,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/msi.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>

Correct header is:

#include <linux/gpio/consumer.h>

>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
>  #include <linux/of_irq.h>
> @@ -577,11 +578,17 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct xilinx_pcie *pcie;
>  	struct pci_host_bridge *bridge;
> +	struct gpio_desc *perst_gpio;
>  	int err;
>  
>  	if (!dev->of_node)
>  		return -ENODEV;
>  
> +	perst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(perst_gpio))
> +		return dev_err_probe(dev, PTR_ERR(perst_gpio),
> +				     "reset-gpios request failed\n");

'Failed to request reset GPIO'

> +
>  	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
>  	if (!bridge)
>  		return -ENODEV;
> @@ -596,6 +603,13 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> +	if (perst_gpio) {
> +		usleep_range(10, 20); /* Assert the reset for ~10 us */

Delay should be PCIE_T_PVPERL_MS.

> +		gpiod_set_value_cansleep(perst_gpio, 0);
> +		/* Initial delay to provide endpoint time to restart */

s/restart/initialize

- Mani

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