Re: [PATCH v2 6/8] 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] Bypass of the mandatory 100ms post-reset delay for Gen 1 / Gen 2 endpoints.
- [Medium] The commit message incorrectly claims that `link_gen` holds the negotiated link speed.
--

commit 9114690f6dce7f4f9d92fc0cec16e81c387aa8fb
Author: Hans Zhang <18255117159@163.com>
PCI: aardvark: Add 100 ms delay after link training

This commit adds a 100 ms delay after link training completes for speeds
greater than 5.0 GT/s. It utilizes the pcie_wait_after_link_train() helper
together with the driver's link_gen field to insert the delay once the link
is up.

> The driver already maintains a 'link_gen' field that holds the negotiated
> link speed. Use it together with pcie_wait_after_link_train() to insert
> the required delay immediately after confirming that the link is up.

Is this description of link_gen accurate?

Looking at the driver, pcie->link_gen appears to be populated at probe time
via of_pci_get_max_link_speed() and statically stores the port's maximum
configured link speed capability, rather than being dynamically updated with
the actual negotiated speed.

While passing the maximum capability to pcie_wait_after_link_train() is
correct per the PCIe specification, could the commit message be updated to
reflect the actual contents of the link_gen field?

> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index e34bea1ff0ac6..526351c21c49e 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -350,8 +350,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)) {
> +			pcie_wait_after_link_train(pcie->link_gen);
>  			return 0;
> +		}
>  
>  		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
>  	}

Does this sequence bypass the mandatory 100ms post-reset delay for Gen 1 and
Gen 2 endpoints?

The PCIe specification requires a minimum 100ms delay after a conventional
reset before sending configuration requests. This function is called by
advk_pcie_setup_hw() to fulfill that requirement, but it polls and returns
immediately upon link-up.

If a link trains quickly (e.g., in 10-20ms), the function exits early. Since
pcie_wait_after_link_train() only executes a delay if link_gen > 2:

drivers/pci/pci.h:pcie_wait_after_link_train() {
    if (max_link_speed > 2)
        msleep(PCIE_RESET_CONFIG_WAIT_MS);
}

Are Gen 1 and Gen 2 endpoints left exposed to premature configuration requests
since the remainder of the mandatory 100ms post-reset delay is skipped?

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