Re: [PATCH v4 2/2] PCI: xilinx: Support reset GPIO for PERST#

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

On 10-06-2025 21:07, Bjorn Helgaas wrote:
> On Tue, Jun 10, 2025 at 04:39:04PM +0200, 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.
>> @@ -576,11 +577,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),
>> +				     "Failed to request reset GPIO\n");
>> +
>>   	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
>>   	if (!bridge)
>>   		return -ENODEV;
>> @@ -595,6 +602,13 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>>   		return err;
>>   	}
>>   
>> +	if (perst_gpio) {
>> +		msleep(PCIE_T_PVPERL_MS); /* Minimum assertion time */
>> +		gpiod_set_value_cansleep(perst_gpio, 0);
> Are we assured that PERST# was already asserted when we entered
> xilinx_pcie_probe()?

Yes, because of the GPIOD_OUT_HIGH a few lines up, the reset GPIO is asserted 
when we arrive here.


>> +		/* Initial delay to provide endpoint time to initialize */
>> +		msleep(PCIE_T_RRS_READY_MS);
> I don't think this is the right spot for PCIE_T_RRS_READY_MS, details
> in https://lore.kernel.org/r/20250610185734.GA819344@bhelgaas
>
> I guess the spec assumes that for ports that don't support speeds
> greater than 5.0 GT/s, 100ms is enough for the link to come up *and*
> the endpoint to initialize.  But since you're going to wait for the
> link to come up immediately *after* this PCIE_T_RRS_READY_MS sleep, I
> would think you could extend the timeout in xilinx_pci_wait_link_up()
> and then do the PCIE_T_RRS_READY_MS sleep.

That would change the behavior of the original driver though, which never did 
any sleep during init. But it appears to match the spec better. Note that the 
hardware is limited to 5GT/s.

M.