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.
