Re: [PATCH 1/2] PCI: cadence: Ensure that cdns_pcie_host_wait_for_link() waits 100 ms after link up

[Hans Zhang <18255117159@163.com>]

On 5/5/26 00:22, Bjorn Helgaas wrote:
> On Mon, May 04, 2026 at 02:23:34PM +0800, Hans Zhang wrote:
>> On 5/4/26 13:08, Siddharth Vadapalli wrote:
>>> On 03/05/26 21:16, Hans Zhang wrote:
>>>> On 5/2/26 13:18, Siddharth Vadapalli wrote:
>>>>> On 01/05/26 21:05, Hans Zhang wrote:
>>>>>> As per PCIe r6.0, sec 6.6.1, a Downstream Port that supports
>>>>>> Link speeds
>>>>>> greater than 5.0 GT/s, software must wait a minimum of 100
>>>>>> ms after Link
>>>>>> training completes before sending a Configuration Request.
>>>>>>
>>>>>> Add a new 'max_link_speed' field in struct cdns_pcie to record the
>>>>>> maximum supported (or currently configured) link speed of
>>>>>> the controller.
>>>>>>
>>>>>> In cdns_pcie_host_wait_for_link(), after the link is reported as up,
>>>>>> insert a 100 ms delay if max_link_speed > 2 (i.e., > 5 GT/s). This
>>>>>> implements the required delay at the common Cadence host layer.
>>>>>>
>>>>>> Currently max_link_speed is zero-initialized, so the delay is not yet
>>>>>> active. Glue drivers must set max_link_speed appropriately to enable
>>>>>> the delay. This matches the approach taken for the Synopsys DWC
>>>>>> controller in commit 80dc18a0cba8d ("PCI: dwc: Ensure that
>>>>>> dw_pcie_wait_for_link() waits 100 ms after link up").
>>>>>>
>>>>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>>>>> ---
>>>>>>    .../pci/controller/cadence/pcie-cadence-host-common.c    |
>>>>>> 9 +++++ ++++
>>>>>>    drivers/pci/controller/cadence/pcie-cadence.h            | 2 ++
>>>>>>    2 files changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git
>>>>>> a/drivers/pci/controller/cadence/pcie-cadence-host- common.c
>>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>>>> index 2b0211870f02..d4ae762f423f 100644
>>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>>>> @@ -14,6 +14,7 @@
>>>>>>    #include "pcie-cadence.h"
>>>>>>    #include "pcie-cadence-host-common.h"
>>>>>> +#include "../../pci.h"
>>>>>>    #define LINK_RETRAIN_TIMEOUT HZ
>>>>>> @@ -55,6 +56,14 @@ int cdns_pcie_host_wait_for_link(struct
>>>>>> cdns_pcie *pcie,
>>>>>>        /* Check if the link is up or not */
>>>>>>        for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
>>>>>>            if (pcie_link_up(pcie)) {
>>>>>> +            /*
>>>>>> +             * As per PCIe r6.0, sec 6.6.1, a Downstream Port that
>>>>>> +             * supports Link speeds greater than 5.0 GT/s, software
>>>>>> +             * must wait a minimum of 100 ms after Link training
>>>>>> +             * completes before sending a Configuration Request.
>>>>>> +             */
>>>>>> +            if (pcie->max_link_speed > 2)
>>>>>> +                msleep(PCIE_RESET_CONFIG_WAIT_MS);
>>>>>
>>>>> I think the above could be moved to cdns_pcie_host_start_link()
>>>>> as follows:
>>>>>
>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-
>>>>> common.c b/
>>>>> drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>>> index 2b0211870f02..0f885dcbdb12 100644
>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>>> @@ -115,6 +115,15 @@ int cdns_pcie_host_start_link(struct
>>>>> cdns_pcie_rc *rc,
>>>>>        if (!ret && rc->quirk_retrain_flag)
>>>>>            ret = cdns_pcie_retrain(pcie, pcie_link_up);
>>>>>
>>>>> +    /*
>>>>> +     * As per PCIe r6.0, sec 6.6.1, a Downstream Port that
>>>>> +     * supports Link speeds greater than 5.0 GT/s, software
>>>>> +     * must wait a minimum of 100 ms after Link training
>>>>> +     * completes before sending a Configuration Request.
>>>>> +     */
>>>>> +    if (!ret && pcie->max_link_speed > 2)
>>>>> +        msleep(PCIE_RESET_CONFIG_WAIT_MS);
>>>>> +
>>>>>        return ret;
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
>>>>>
>>>>> This will avoid an additional and unnecessary delay when
>>>>> 'cdns_pcie_retrain()' retrains the link.
>>>>>
>>>>> Instead of checking for the link being up using
>>>>> "pcie_link_up(pcie)", checking for 'ret' being zero should also
>>>>> work (ret being zero indicates that the link is up).
>>>>>
>>>>> Since configuration space accesses will not be performed until
>>>>> cdns_pcie_host_start_link() completes executing, it should be
>>>>> safe to switch to the above implementation.
>>>>
>>>> Hi Siddharth,
>>>>
>>>> I think this is applicable to LGA IP as per the method you
>>>> mentioned. However, for HPA IP, additional repetitive code needs to
>>>> be added in the following code.
>>>
>>> Yes, additional code is required as you rightly pointed out, but the
>>> problem I was trying to address with your patch is the following:
>>>       cdns_pcie_host_start_link()
>>>         calls cdns_pcie_host_wait_for_link()
>>>           Link is Up and we wait for 100 ms here
>>>         calls cdns_pcie_retrain()
>>>             calls cdns_pcie_host_wait_for_link() a second time
>>>               Link is Up again after retraining and we wait and
>>>               we wait an additional 100 ms here.
>>>
>>> Instead, it will be sufficient if we could wait just once after
>>> cdns_pcie_retrain() returns.
>>
>> Hi Siddharth,
>>
>> Yes, I looked at the code and indeed it works this way.
>>
>> Because of the abundance of redundant comments. I'm wondering if it's
>> possible to encapsulate a helper function in the file
>> drivers/pci/controller/pci-host-common.c, so that controller drivers like
>> dwc and cadence can call this API. Or do you know where it would be
>> appropriate to place it?
>>
>> Hello, Bjorn and Mani, I wonder what your opinions are.
> 
> Make a proposal.  Sounds fine to remove redundant comments if they
> cause confusion.  Adding a helper to make things more consistent
> across drivers also sounds fine, but it would be better to have a
> straw-man proposal to respond to.

Hi Bjorn,

Thank you for your reply. I will then prepare the next version.

Best regards,
Hans

> 
> Bjorn