Re: [PATCH v2 6/8] PCI: aardvark: Add 100 ms delay after link training

On 5/13/26 15:20, Pali Rohár wrote:
> On Wednesday 13 May 2026 15:00:04 Hans Zhang wrote:
>>
>>
>> On 5/13/26 05:25, Pali Rohár wrote:
>>> On Wednesday 06 May 2026 23:23:44 Hans Zhang wrote:
>>>> The Aardvark PCIe controller driver waits for the link to come up but
>>>> does not implement the mandatory 100 ms delay after link training
>>>> completes for speeds greater than 5.0 GT/s (PCIe r6.0 sec 6.6.1).
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Hans Zhang <[email protected]>
>>>> ---
>>>>    drivers/pci/controller/pci-aardvark.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
>>>> index e34bea1ff0ac..526351c21c49 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);
>>>>    	}
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>> Are you sure that this is correct to do? Have you checked the A3720
>>> Functional Specification which describes how to bring PCIe link up?
>>>
>>> A3720 PCIe controller is buggy and needs more timing hacks to make it
>>> behave. Playing with random sleeps can break its internal logic.
>>> I'm not sure if it could be safe without proper testing.
>>>
>>> And IIRC A3720 PCIe controller is just PCIe2.0 with 5 GT/s.
>>
>>
>> Hi Pali,
>>
>> 1. This driver does not support A3720.
>>
>> static const struct of_device_id advk_pcie_of_match_table[] = {
>> 	{ .compatible = "marvell,armada-3700-pcie", },
>> 	{},
>> };
>> MODULE_DEVICE_TABLE(of, advk_pcie_of_match_table);
>>
>> If you need support for A3720, please submit the corresponding patch so that
>> Bjorn and Mani can review it.
> 
> 3700 (or 37xx) is family and covers both a3710 and a3720. In most cases is the
> a3720 dominant and hence identifiers 3700 and 3720 are begin mixed.
> 
>>
>> 2. If A3720 only supports GEN2, you can configure "max-link-speed" to be 2
>> in the DT. This will not affect the functionality of this patch.
> 
> Whole A37xx supports only GEN2. And in DT files for 37xx should be
> already there max-link-speed.
> 
> Seems that in advk_pcie_of_match_table there is no GEN3 device
> specified.
> 

Hi Pali,

However, I saw many GEN3 assignments and conditions in the code.

ret = of_pci_get_max_link_speed(dev->of_node);
if (ret <= 0 || ret > 3)
	pcie->link_gen = 3;
else
	pcie->link_gen = ret;


static void advk_pcie_train_link(struct advk_pcie *pcie)
{
	struct device *dev = &pcie->pdev->dev;
	u32 reg;
	int ret;

	/*
	 * Setup PCIe rev / gen compliance based on device tree property
	 * 'max-link-speed' which also forces maximal link speed.
	 */
	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
	reg &= ~PCIE_GEN_SEL_MSK;
	if (pcie->link_gen == 3)
		reg |= SPEED_GEN_3;
	else if (pcie->link_gen == 2)
		reg |= SPEED_GEN_2;
	else
		reg |= SPEED_GEN_1;
	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);

	/*
	 * Set maximal link speed value also into PCIe Link Control 2 register.
	 * Armada 3700 Functional Specification says that default value is based
	 * on SPEED_GEN but tests showed that default value is always 8.0 GT/s.
	 */
	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL2);
	reg &= ~PCI_EXP_LNKCTL2_TLS;
	if (pcie->link_gen == 3)
		reg |= PCI_EXP_LNKCTL2_TLS_8_0GT;
	else if (pcie->link_gen == 2)
		reg |= PCI_EXP_LNKCTL2_TLS_5_0GT;
	else
		reg |= PCI_EXP_LNKCTL2_TLS_2_5GT;
	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL2);

....


If you are certain about the relevant information. Is it understandable 
that we need to delete the code related to GEN3?


Best regards,
Hans


>> 3. This patch is a common delay requirement stipulated by the PCIe
>> specification. If it is greater than GEN2, then msleep(100) will be added;
>> otherwise, there will be no such delay.
>>
>> 4. For instance, we often come across the situation where some common APIs
>> are modified, and in many cases, their functionality does not require the
>> actual development board for verification. I believe that many other
>> developers and maintainers have modified different parts of the code. For
>> example, the recent submission:
> 
> Switching one API to another is one thing. But changing code which looks
> to be critical, specially when it is known that hw has bugs, can cause
> breaking of existing boards.
> 
>> commit 750277048afe7ce8ebfc0b120de7dfbc745058a7
>> Author: Nam Cao <[email protected]>
>> Date:   Thu Jun 26 16:47:53 2025 +0200
>>
>>      PCI: aardvark: Switch to msi_create_parent_irq_domain()
>>
>>      Switch to msi_create_parent_irq_domain() from
>> pci_msi_create_irq_domain()
>>      which was using legacy MSI domain setup.
>>
>>
>> And many controller drivers have been modified.
>>
>>
>> Best regards,
>> Hans
>>
>>