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

[Mike Looijmans <mike.looijmans@topic.nl>]

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