Re: [PATCH v3 1/7] PCI: Add pci_host_common_link_train_delay() helper

[Hans Zhang <18255117159@163.com>]

On 5/12/26 15:05, Claudiu Beznea wrote:
> Hi, Hans,
> 
> On 5/11/26 08:59, Hans Zhang wrote:
>> PCIe r6.0, sec 6.6.1 (Conventional Reset) requires that for a Downstream
>> Port supporting Link speeds greater than 5.0 GT/s, software must wait a
>> minimum of 100 ms after Link training completes before sending any
>> Configuration Request.
>>
>> Introduce a static inline helper pci_host_common_link_train_delay() that
>> checks the given max_link_speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, etc.) and
>> calls msleep(100) only when the speed is greater than 5.0 GT/s.
>>
>> This allows multiple host controller drivers to share the same mandatory
>> delay without duplicating the logic.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   drivers/pci/controller/pci-host-common.h | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/ 
>> controller/pci-host-common.h
>> index b5075d4bd7eb..d709f7e3e11a 100644
>> --- a/drivers/pci/controller/pci-host-common.h
>> +++ b/drivers/pci/controller/pci-host-common.h
>> @@ -10,6 +10,9 @@
>>   #ifndef _PCI_HOST_COMMON_H
>>   #define _PCI_HOST_COMMON_H
>> +#include <linux/delay.h>
>> +#include "../pci.h"
>> +
>>   struct pci_ecam_ops;
>>   int pci_host_common_probe(struct platform_device *pdev);
>> @@ -20,4 +23,18 @@ void pci_host_common_remove(struct platform_device 
>> *pdev);
>>   struct pci_config_window *pci_host_common_ecam_create(struct device 
>> *dev,
>>       struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
>> +
>> +/**
>> + * pci_host_common_link_train_delay - Wait 100 ms if link speed > 5 GT/s
>> + * @max_link_speed: the maximum link speed (2 = 5.0 GT/s, 3 = 8.0 GT/ 
>> s, ...)
>> + *
>> + * Must be called after Link training completes and before the first
>> + * Configuration Request is sent.
>> + */
>> +static inline void pci_host_common_link_train_delay(int max_link_speed)
>> +{
>> +    if (max_link_speed > 2)
>> +        msleep(PCIE_RESET_CONFIG_WAIT_MS);
> 
> In case of RZ/G3S driver the max_link_speed is populated based on "max- 
> link-speed" DT property (by calling of_pci_get_max_link_speed()). My 
> understanding from [1] (and the review of the initial RZ/G3S driver 
> support) is that this is not a mandatory property (note also the "Host 
> drivers *could* add this" from [1]). At least for the RZ/G3S driver, in 
> case the "max-link-speed" DT property is not present in DT but the 
> controller supports more than 5GT/s (that is possible as the driver 
> supports more controller variants), the max_link_speed argument will be 
> negative. In that case the msleep() will not be called. This looks like 
> an opposite of what the patch set is trying to achieve.

Hi Claudiu,

The situation you mentioned also exists in the dwc common driver. My 
understanding is that we are writing this driver at the normal rate 
which is greater than GEN2. For some exceptions, or when the support is 
greater than GEN2 but the actual operation is less than or equal to 
GEN2, this situation might be unavoidable. Furthermore, for RZ/G3S, the 
"max-link-speed" attribute can be added to the DT.


> 
> Also, if I'm not wrong, there is also the possibility of having the max- 
> link-speed > 2 but the downstream port to not support more than 5GT/s. 
> In that case the mspeep() would also be executed (but I think that 
> wouldn't be really an issue).


Before this patch, the RZ/G3S driver would always perform a msleep(100) 
regardless of whether it was greater than GEN2 or less than or equal to 
GEN2.

Best regards,
Hans

> 
> Thank you,
> Claudiu
> 
> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/ 
> schemas/pci/pci-bus-common.yaml#L117
> 
> Thank you,
> Claudiu