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

On 5/14/26 02:54, Pali Rohár wrote:
> On Wednesday 13 May 2026 15:34:46 Hans Zhang wrote:
>> 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?
> 
> Ok. So some explanation. pci-aardvark.c is implementing driver for PCIe
> controller with codename aardvark. I have no idea from what this
> codename comes and what is represents. What we know that the driver was
> written for A37xx SoC platform according to A37xx functional specification.
> As it is common in SoC world, vendors just buy some IP and integrate it
> into SoC. In this case Marvell bought this PCIe controller IP and
> integrated it into the A37xx. In past I tried to investigate what it
> could be and IIRC my assumption was that it was PCIe IP from Denali.
> Denali was acquired by Cadence, and when I compared Cadence PCIe
> controller registers and PCIe controller registers in A37xx functional
> specification there were large overlap. For me it looked like new
> Cadence PCIe controller is an evolution (or new version) of what is in
> A37xx. So this was some confirmation of my theory. Linux kernel has
> separate driver for PCIe controller from Cadence and for refactoring
> there were ideas to merge these two drivers... But there were more
> important things, fix issues related to A37xx PCIe, lot of changes
> which address these issues were sent to the list but they were not
> taken. I do not think that it makes sense to do refactoring or doing any
> other changes before addressing any existing issues with these
> drivers (like PCIe card is not working correctly).
> 
> There are reported more HW erratas for this PCIe controller which needs
> to be addressed in the software (meaning in Linux kernel) to make PCIe
> card working properly. And there are more design HW decision which needs
> does not conform to the PCIe specification and those deviations needs to
> be "fixed" or "adjusted" in software (meaning in pci-aardvark.c driver)
> to make PCI/PCIe compatible drivers to work correctly.
> 
> Now about GEN3. From register allocation it looks like that PCIe IP
> supports GEN3. A37xx does not support it (or at least officially). This
> does not mean that there cannot be some SoC with this "aardvark" PCIe IP
> that is GEN3 capable. Just we see that such SoC is not supported by Linux.
> Also as the comment in above code says, by default the speed is reported
> as 8.0 GT/s, so changing it to 5.0 GT/s or 2.5 GT/s is needed as so code
> some parts of GEN3 code in the driver is needed.
> 
> Does it makes sense to remove it? Does it makes sense to spend time on
> such thing which does not address any existing issue? For me not.
> Because it does not fix any _real_ issue with existing PCIe cards. And
> for refactoring it is better to merge drivers as explained above and
> IIRC cadence driver has HW on which is GEN3 used.
> 
> Now about your change. If you are sure that pcie_wait_after_link_train()
> function is noop for pcie->link_gen == 2 || pcie->link_gen == 1 then go
> ahead, I have no objects. I have not looked deeply at the change. I just
> spotted some change which is touching timing critical code path which
> was problematic in the past and broke many wifi cards. So I'm really
> careful to prevent breaking Linux support again.
> 

Hi Pali,

This condition, pcie->link_gen == 2 || pcie->link_gen == 1, will have no 
effect.


Best regards,
Hans

> As maintainers decided to not take any new changes from me for this
> driver, I have no motivation to prepare any new changes. I will rather
> spend my free time on something which will make sense and not be wasting
> of my free time.
>