Re: [PATCH v4 5/7] PCI: aardvark: Add 100 ms delay after link training

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Failure to enforce the mandatory 100ms post-reset delay for Gen 1 and Gen 2 devices.
- [Medium] Commit message incorrectly claims that `link_gen` holds the "negotiated link speed".
--

commit 82256a299ecbac6749b1ecf25bbecc74415b47f2
Author: Hans Zhang <18255117159@163.com>

PCI: aardvark: Add 100 ms delay after link training

This commit introduces a mandatory 100ms delay after link training completes
for Aardvark PCIe controllers operating at speeds greater than 5.0 GT/s. It
utilizes pci_host_common_link_train_delay() along with the existing link_gen
field to insert this delay once the link is confirmed up.

> The driver already maintains a 'link_gen' field that holds the negotiated
> link speed. Use it together with pci_host_common_link_train_delay() to

This isn't a bug, but does link_gen actually hold the negotiated link speed?

Looking at advk_pcie_probe(), pcie->link_gen is populated using
of_pci_get_max_link_speed(), which reads the static max-link-speed
capability from the device tree rather than the dynamically negotiated speed.

While passing this static capability correctly implements the PCIe spec,
could the commit message be updated to accurately describe the variable's
contents?

> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index e34bea1ff0ac6..fd9c7d53e8a78 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c

[ ... ]

> @@ -350,8 +351,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
>  
>  	/* check if the link is up or not */
>  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> -		if (advk_pcie_link_up(pcie))
> +		if (advk_pcie_link_up(pcie)) {
> +			pci_host_common_link_train_delay(pcie->link_gen);
>  			return 0;
> +		}
>  
>  		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
>  	}

Does this code enforce the mandatory 100ms post-reset delay for Gen 1 and
Gen 2 devices?

While this issue wasn't introduced by this patch, the existing code
fundamentally violates the PCIe Base Specification. The spec requires a
minimum 100ms delay after Conventional Reset (PERST# deassertion) before
sending Configuration Requests for ports supporting <= 5.0 GT/s.

In advk_pcie_setup_hw(), after PERST# is deasserted, the code calls
advk_pcie_wait_for_link(). If a Gen 1 or Gen 2 device trains quickly, the
new pci_host_common_link_train_delay() adds no delay, and the function
returns immediately.

Consequently, the PCI core will proceed to pci_host_probe() and start
issuing Configuration Requests before the 100ms minimum window has elapsed.
Can this cause Gen 1 or Gen 2 devices to fail to enumerate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=5