Hi Mika,
I need some help because I have a hard time applying sec 6.6.1.
On Tue, Apr 04, 2023 at 08:27:14AM +0300, Mika Westerberg wrote:
> In order speed up reset and resume time of devices behind slow links,
> decrease the wait time to 1s. This should give enough time for them to
> respond.
Is there some spec language behind this? In sec 6.6.1, I see that all
devices "must be able to receive a Configuration Request and return a
Successful Completion".
A preceding rule says devices with slow links must enter LTSSM Detect
within 20ms, but I don't see a direct connection from that to a
shorter wait time.
> While doing this, instead of looking at the speed we check if
> the port supports active link reporting.
Why check dev->link_active_reporting (i.e., PCI_EXP_LNKCAP_DLLLARC)
instead of the link speed described by the spec?
DLLLARC is required for fast links, but it's not prohibited for slower
links and it's *required* for hotplug ports with slow links, so
dev->link_active_reporting is not completely determined by link speed.
IIUC, the current code basically has these cases:
1) All devices on secondary bus have zero D3cold delay:
return immediately; no delay at all
2) Non-PCIe bridge:
sleep 1000ms
sleep 100ms (typical, depends on downstream devices)
3) Speed <= 5 GT/s:
sleep 100ms (typical)
sleep up to 59.9s (typical) waiting for valid config read
4) Speed > 5 GT/s (DLLLARC required):
sleep 20ms
sleep up to 1000ms waiting for DLLLA
sleep 100ms (typical)
sleep up to 59.9s (typical) waiting for valid config read
This patch changes cases 3) and 4) to:
3) DLLLARC not supported:
sleep 100ms (typical)
sleep up to 1.0s (typical) waiting for valid config read
4) DLLLARC supported:
no change in wait times, ~60s total
And testing dev->link_active_reporting instead of speed means slow
hotplug ports (and possibly other slow ports that implement DLLLARC)
that previously were in case 3) will now be in case 4).
> If it does we can wait longer
> but if it does not we wait for the 1s prescribed in the PCIe spec.
>
> Since pci_bridge_wait_for_secondary_bus() handles all the delays
> internally now move the wait constants from drivers/pci/pci.h into
> drivers/pci/pci.c.
>
> Cc: Lukas Wunner <lukas@xxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
> drivers/pci/pci-driver.c | 3 +--
> drivers/pci/pci.c | 42 ++++++++++++++++++++++++++--------------
> drivers/pci/pci.h | 16 +--------------
> drivers/pci/pcie/dpc.c | 3 +--
> 4 files changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 6b5b2a818e65..1a5ee65edb10 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -572,8 +572,7 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
>
> static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev)
> {
> - pci_bridge_wait_for_secondary_bus(pci_dev, "resume",
> - PCIE_RESET_READY_POLL_MS);
> + pci_bridge_wait_for_secondary_bus(pci_dev, "resume");
> /*
> * When powering on a bridge from D3cold, the whole hierarchy may be
> * powered on into D0uninitialized state, resume them to give them a
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7a67611dc5f4..5302d900dbe7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -64,6 +64,19 @@ struct pci_pme_device {
>
> #define PME_TIMEOUT 1000 /* How long between PME checks */
>
> +/*
> + * Following exit from Conventional Reset, devices must be ready within 1 sec
> + * (PCIe r6.0 sec 6.6.1). A D3cold to D0 transition implies a Conventional
> + * Reset (PCIe r6.0 sec 5.8).
> + */
> +#define PCI_RESET_WAIT 1000 /* msec */
> +/*
> + * Devices may extend the 1 sec period through Request Retry Status completions
> + * (PCIe r6.0 sec 2.3.1). The spec does not provide an upper limit, but 60 sec
> + * ought to be enough for any device to become responsive.
> + */
> +#define PCIE_RESET_READY_POLL_MS 60000 /* msec */
> +
> static void pci_dev_d3_sleep(struct pci_dev *dev)
> {
> unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay);
> @@ -4939,7 +4952,6 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
> * pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible
> * @dev: PCI bridge
> * @reset_type: reset type in human-readable form
> - * @timeout: maximum time to wait for devices on secondary bus (milliseconds)
> *
> * Handle necessary delays before access to the devices on the secondary
> * side of the bridge are permitted after D3cold to D0 transition
> @@ -4952,8 +4964,7 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
> * Return 0 on success or -ENOTTY if the first device on the secondary bus
> * failed to become accessible.
> */
> -int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
> - int timeout)
> +int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> {
> struct pci_dev *child;
> int delay;
> @@ -5018,20 +5029,22 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
> if (!pcie_downstream_port(dev))
> return 0;
>
> - if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> + if (!dev->link_active_reporting) {
> pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
> msleep(delay);
> - } else {
> - pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
> - delay);
> - if (!pcie_wait_for_link_delay(dev, true, delay)) {
> - /* Did not train, no need to wait any further */
> - pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
> - return -ENOTTY;
> - }
> +
> + return pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay);
> + }
> +
> + pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
> + delay);
> + if (!pcie_wait_for_link_delay(dev, true, delay)) {
> + /* Did not train, no need to wait any further */
> + pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
> + return -ENOTTY;
> }
>
> - return pci_dev_wait(child, reset_type, timeout - delay);
> + return pci_dev_wait(child, reset_type, PCIE_RESET_READY_POLL_MS - delay);
> }
>
> void pci_reset_secondary_bus(struct pci_dev *dev)
> @@ -5068,8 +5081,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
> {
> pcibios_reset_secondary_bus(dev);
>
> - return pci_bridge_wait_for_secondary_bus(dev, "bus reset",
> - PCIE_RESET_READY_POLL_MS);
> + return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> }
> EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index d2c08670a20e..f2d3aeab91f4 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -64,19 +64,6 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
> #define PCI_PM_D3HOT_WAIT 10 /* msec */
> #define PCI_PM_D3COLD_WAIT 100 /* msec */
>
> -/*
> - * Following exit from Conventional Reset, devices must be ready within 1 sec
> - * (PCIe r6.0 sec 6.6.1). A D3cold to D0 transition implies a Conventional
> - * Reset (PCIe r6.0 sec 5.8).
> - */
> -#define PCI_RESET_WAIT 1000 /* msec */
> -/*
> - * Devices may extend the 1 sec period through Request Retry Status completions
> - * (PCIe r6.0 sec 2.3.1). The spec does not provide an upper limit, but 60 sec
> - * ought to be enough for any device to become responsive.
> - */
> -#define PCIE_RESET_READY_POLL_MS 60000 /* msec */
> -
> void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
> void pci_refresh_power_state(struct pci_dev *dev);
> int pci_power_up(struct pci_dev *dev);
> @@ -100,8 +87,7 @@ void pci_msix_init(struct pci_dev *dev);
> bool pci_bridge_d3_possible(struct pci_dev *dev);
> void pci_bridge_d3_update(struct pci_dev *dev);
> void pci_bridge_reconfigure_ltr(struct pci_dev *dev);
> -int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
> - int timeout);
> +int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
>
> static inline void pci_wakeup_event(struct pci_dev *dev)
> {
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index a5d7c69b764e..3ceed8e3de41 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -170,8 +170,7 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
> PCI_EXP_DPC_STATUS_TRIGGER);
>
> - if (pci_bridge_wait_for_secondary_bus(pdev, "DPC",
> - PCIE_RESET_READY_POLL_MS)) {
> + if (pci_bridge_wait_for_secondary_bus(pdev, "DPC")) {
> clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
> ret = PCI_ERS_RESULT_DISCONNECT;
> } else {
> --
> 2.39.2
>
