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

[Claudiu Beznea <claudiu.beznea@kernel.org>]

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.

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).

Thank you,
Claudiu

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

Thank you,
Claudiu