From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
PCIe spec r6.0, sec 6.6.1 mandates waiting for 100ms before deasserting
PERST# if the downstream port does not support Link speeds greater than
5.0 GT/s. But in practice, this delay seem to be required irrespective of
the supported link speed as it gives the endpoints enough time to
initialize.
Hence, add the delay by reusing the PCIE_RESET_CONFIG_WAIT_MS definition if
the linkup_irq is not supported. If the linkup_irq is supported, the driver
already waits for 100ms in the IRQ handler post link up.
Also, remove the redundant comment for PCIE_T_PVPERL_MS. Finally, the
PERST_DELAY_US sleep can be moved to PERST# assert where it should be.
Cc: stable+noautosel@kernel.org # non-trivial dependency
Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..bcd080315d70e64eafdefd852740fe07df3dbe75 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -302,20 +302,22 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
else
list_for_each_entry(port, &pcie->ports, list)
gpiod_set_value_cansleep(port->reset, val);
-
- usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
}
static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
{
qcom_perst_assert(pcie, true);
+ usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
}
static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
{
- /* Ensure that PERST has been asserted for at least 100 ms */
+ struct dw_pcie_rp *pp = &pcie->pci->pp;
+
msleep(PCIE_T_PVPERL_MS);
qcom_perst_assert(pcie, false);
+ if (!pp->use_linkup_irq)
+ msleep(PCIE_RESET_CONFIG_WAIT_MS);
}
static int qcom_pcie_start_link(struct dw_pcie *pci)
--
2.45.2On Wed, Sep 03, 2025 at 12:43:23PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> PCIe spec r6.0, sec 6.6.1 mandates waiting for 100ms before deasserting
> PERST# if the downstream port does not support Link speeds greater than
> 5.0 GT/s.
I guess you mean we need to wait 100ms *after* deasserting PERST#?
I.e., this wait before sending config requests to a downstream device:
◦ With a Downstream Port that does not support Link speeds greater
than 5.0 GT/s, software must wait a minimum of 100 ms following
exit from a Conventional Reset before sending a Configuration
Request to the device immediately below that Port.
> But in practice, this delay seem to be required irrespective of
> the supported link speed as it gives the endpoints enough time to
> initialize.
Saying "but in practice ... seems to be required" suggests that the
spec requirement isn't actually enough. But the spec does say the
100ms delay before config requests is required for all link speeds.
The difference is when we start that timer: for 5 GT/s or slower it
starts at exit from Conventional Reset; for faster than 5 GT/s it
starts when link training completes.
> Hence, add the delay by reusing the PCIE_RESET_CONFIG_WAIT_MS definition if
> the linkup_irq is not supported. If the linkup_irq is supported, the driver
> already waits for 100ms in the IRQ handler post link up.
I didn't look into this, but I wondered whether it's possible to miss
the interrupt, especially the first time during probe.
> Also, remove the redundant comment for PCIE_T_PVPERL_MS. Finally, the
> PERST_DELAY_US sleep can be moved to PERST# assert where it should be.
Unless this PERST_DELAY_US move is logically part of the
PCIE_RESET_CONFIG_WAIT_MS change, putting it in a separate patch would
make *this* patch easier to read.
> Cc: stable+noautosel@kernel.org # non-trivial dependency
> Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..bcd080315d70e64eafdefd852740fe07df3dbe75 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -302,20 +302,22 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> else
> list_for_each_entry(port, &pcie->ports, list)
> gpiod_set_value_cansleep(port->reset, val);
> -
> - usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
>
> static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> {
> qcom_perst_assert(pcie, true);
> + usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
>
> static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> {
> - /* Ensure that PERST has been asserted for at least 100 ms */
> + struct dw_pcie_rp *pp = &pcie->pci->pp;
> +
> msleep(PCIE_T_PVPERL_MS);
> qcom_perst_assert(pcie, false);
> + if (!pp->use_linkup_irq)
> + msleep(PCIE_RESET_CONFIG_WAIT_MS);
I'm a little confused about why you test pp->use_linkup_irq here
instead of testing the max supported link speed. And I'm not positive
that this is the right place, at least at this point in the series.
At this point, qcom_ep_reset_deassert() is only used by
qcom_pcie_host_init(), so the flow is like this:
qcom_pcie_probe
irq = platform_get_irq_byname_optional(pdev, "global")
if (irq > 0)
pp->use_linkup_irq = true
dw_pcie_host_init
pp->ops->init
qcom_pcie_host_init # .init
qcom_ep_reset_deassert # <--
+ if (!pp->use_linkup_irq)
+ msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
if (!dw_pcie_link_up(pci))
dw_pcie_start_link
if (!pp->use_linkup_irq)
dw_pcie_wait_for_link
for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
if (dw_pcie_link_up(pci))
break
msleep(PCIE_LINK_WAIT_SLEEP_MS) # 90ms
if (pci->max_link_speed > 2) # > 5.0 GT/s
msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
For slow links (<= 5 GT/s), it's possible that the link comes up
before we even call dw_pcie_link_up() the first time, which would mean
we really don't wait at all.
My guess is that most links wouldn't come up that fast but *would*
come up within 90ms. Even in that case, we wouldn't quite meet the
spec 100ms requirement.
I wonder if dw_pcie_wait_for_link() should look more like this:
dw_pcie_wait_for_link
if (pci->max_link_speed <= 2) # <= 5.0 GT/s
msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
if (dw_pcie_link_up(pci))
break;
msleep(...)
if (pci->max_link_speed > 2) # > 5.0 GT/s
msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
Then we'd definitely wait the required 100ms even for the slow links.
The retry loop could start with a much shorter interval and back off.
I wish the max_link_speed checks used some kind of #define to make
them readable.
Bjorn
On Fri, Sep 05, 2025 at 05:44:30PM GMT, Bjorn Helgaas wrote:
> On Wed, Sep 03, 2025 at 12:43:23PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > PCIe spec r6.0, sec 6.6.1 mandates waiting for 100ms before deasserting
> > PERST# if the downstream port does not support Link speeds greater than
> > 5.0 GT/s.
>
> I guess you mean we need to wait 100ms *after* deasserting PERST#?
>
Right, that's a typo.
> I.e., this wait before sending config requests to a downstream device:
>
> ◦ With a Downstream Port that does not support Link speeds greater
> than 5.0 GT/s, software must wait a minimum of 100 ms following
> exit from a Conventional Reset before sending a Configuration
> Request to the device immediately below that Port.
>
> > But in practice, this delay seem to be required irrespective of
> > the supported link speed as it gives the endpoints enough time to
> > initialize.
>
> Saying "but in practice ... seems to be required" suggests that the
> spec requirement isn't actually enough. But the spec does say the
> 100ms delay before config requests is required for all link speeds.
> The difference is when we start that timer: for 5 GT/s or slower it
> starts at exit from Conventional Reset; for faster than 5 GT/s it
> starts when link training completes.
>
> > Hence, add the delay by reusing the PCIE_RESET_CONFIG_WAIT_MS definition if
> > the linkup_irq is not supported. If the linkup_irq is supported, the driver
> > already waits for 100ms in the IRQ handler post link up.
>
> I didn't look into this, but I wondered whether it's possible to miss
> the interrupt, especially the first time during probe.
>
No, it is not. Since, the controller reinitializes itself during probe() and it
starts LTSSM. So once link up happens, this IRQ will be triggered.
> > Also, remove the redundant comment for PCIE_T_PVPERL_MS. Finally, the
> > PERST_DELAY_US sleep can be moved to PERST# assert where it should be.
>
> Unless this PERST_DELAY_US move is logically part of the
> PCIE_RESET_CONFIG_WAIT_MS change, putting it in a separate patch would
> make *this* patch easier to read.
>
> > Cc: stable+noautosel@kernel.org # non-trivial dependency
> > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> > drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..bcd080315d70e64eafdefd852740fe07df3dbe75 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -302,20 +302,22 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> > else
> > list_for_each_entry(port, &pcie->ports, list)
> > gpiod_set_value_cansleep(port->reset, val);
> > -
> > - usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> > }
> >
> > static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > {
> > qcom_perst_assert(pcie, true);
> > + usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> > }
> >
> > static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> > {
> > - /* Ensure that PERST has been asserted for at least 100 ms */
> > + struct dw_pcie_rp *pp = &pcie->pci->pp;
> > +
> > msleep(PCIE_T_PVPERL_MS);
> > qcom_perst_assert(pcie, false);
> > + if (!pp->use_linkup_irq)
> > + msleep(PCIE_RESET_CONFIG_WAIT_MS);
>
> I'm a little confused about why you test pp->use_linkup_irq here
> instead of testing the max supported link speed. And I'm not positive
> that this is the right place, at least at this point in the series.
>
Because, pci->max_link_speed used to only cache the value of the
'max-link-speed' DT property. But I totally forgot that I changed that behavior
to cache the max supported link speed of the Root Port with commit:
19a69cbd9d43 ("PCI: dwc: Always cache the maximum link speed value in
dw_pcie::max_link_speed")
So yes, I should've been using 'pci->max_link_speed' here.
> At this point, qcom_ep_reset_deassert() is only used by
> qcom_pcie_host_init(), so the flow is like this:
>
> qcom_pcie_probe
> irq = platform_get_irq_byname_optional(pdev, "global")
> if (irq > 0)
> pp->use_linkup_irq = true
> dw_pcie_host_init
> pp->ops->init
> qcom_pcie_host_init # .init
> qcom_ep_reset_deassert # <--
> + if (!pp->use_linkup_irq)
> + msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
> if (!dw_pcie_link_up(pci))
> dw_pcie_start_link
> if (!pp->use_linkup_irq)
> dw_pcie_wait_for_link
> for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
> if (dw_pcie_link_up(pci))
> break
> msleep(PCIE_LINK_WAIT_SLEEP_MS) # 90ms
> if (pci->max_link_speed > 2) # > 5.0 GT/s
> msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
>
> For slow links (<= 5 GT/s), it's possible that the link comes up
> before we even call dw_pcie_link_up() the first time, which would mean
> we really don't wait at all.
>
> My guess is that most links wouldn't come up that fast but *would*
> come up within 90ms. Even in that case, we wouldn't quite meet the
> spec 100ms requirement.
>
> I wonder if dw_pcie_wait_for_link() should look more like this:
>
> dw_pcie_wait_for_link
> if (pci->max_link_speed <= 2) # <= 5.0 GT/s
> msleep(PCIE_RESET_CONFIG_WAIT_MS) dw_pcie_wait_for_link # 100ms
>
> for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
> if (dw_pcie_link_up(pci))
> break;
> msleep(...)
>
> if (pci->max_link_speed > 2) # > 5.0 GT/s
> msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
>
> Then we'd definitely wait the required 100ms even for the slow links.
> The retry loop could start with a much shorter interval and back off.
>
Your concerns are valid. I'll submit a separate series along with *this* patch
to fix these. I don't think clubbing these with this series is a good idea.
> I wish the max_link_speed checks used some kind of #define to make
> them readable.
>
It is mostly because it used to hold the DT property value which starts from 1.
We could still convert it to 'enum pci_bus_speed', but that is for a separate
cleanup.
- Mani
--
மணிவண்ணன் சதாசிவம்
On Sat, Sep 06, 2025 at 04:13:08PM GMT, Manivannan Sadhasivam wrote:
> On Fri, Sep 05, 2025 at 05:44:30PM GMT, Bjorn Helgaas wrote:
> > On Wed, Sep 03, 2025 at 12:43:23PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > PCIe spec r6.0, sec 6.6.1 mandates waiting for 100ms before deasserting
> > > PERST# if the downstream port does not support Link speeds greater than
> > > 5.0 GT/s.
> >
> > I guess you mean we need to wait 100ms *after* deasserting PERST#?
> >
>
> Right, that's a typo.
>
> > I.e., this wait before sending config requests to a downstream device:
> >
> > ◦ With a Downstream Port that does not support Link speeds greater
> > than 5.0 GT/s, software must wait a minimum of 100 ms following
> > exit from a Conventional Reset before sending a Configuration
> > Request to the device immediately below that Port.
> >
> > > But in practice, this delay seem to be required irrespective of
> > > the supported link speed as it gives the endpoints enough time to
> > > initialize.
> >
> > Saying "but in practice ... seems to be required" suggests that the
> > spec requirement isn't actually enough. But the spec does say the
> > 100ms delay before config requests is required for all link speeds.
> > The difference is when we start that timer: for 5 GT/s or slower it
> > starts at exit from Conventional Reset; for faster than 5 GT/s it
> > starts when link training completes.
> >
> > > Hence, add the delay by reusing the PCIE_RESET_CONFIG_WAIT_MS definition if
> > > the linkup_irq is not supported. If the linkup_irq is supported, the driver
> > > already waits for 100ms in the IRQ handler post link up.
> >
> > I didn't look into this, but I wondered whether it's possible to miss
> > the interrupt, especially the first time during probe.
> >
>
> No, it is not. Since, the controller reinitializes itself during probe() and it
> starts LTSSM. So once link up happens, this IRQ will be triggered.
>
> > > Also, remove the redundant comment for PCIE_T_PVPERL_MS. Finally, the
> > > PERST_DELAY_US sleep can be moved to PERST# assert where it should be.
> >
> > Unless this PERST_DELAY_US move is logically part of the
> > PCIE_RESET_CONFIG_WAIT_MS change, putting it in a separate patch would
> > make *this* patch easier to read.
> >
> > > Cc: stable+noautosel@kernel.org # non-trivial dependency
> > > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > ---
> > > drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++---
> > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..bcd080315d70e64eafdefd852740fe07df3dbe75 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -302,20 +302,22 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> > > else
> > > list_for_each_entry(port, &pcie->ports, list)
> > > gpiod_set_value_cansleep(port->reset, val);
> > > -
> > > - usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> > > }
> > >
> > > static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > > {
> > > qcom_perst_assert(pcie, true);
> > > + usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> > > }
> > >
> > > static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> > > {
> > > - /* Ensure that PERST has been asserted for at least 100 ms */
> > > + struct dw_pcie_rp *pp = &pcie->pci->pp;
> > > +
> > > msleep(PCIE_T_PVPERL_MS);
> > > qcom_perst_assert(pcie, false);
> > > + if (!pp->use_linkup_irq)
> > > + msleep(PCIE_RESET_CONFIG_WAIT_MS);
> >
> > I'm a little confused about why you test pp->use_linkup_irq here
> > instead of testing the max supported link speed. And I'm not positive
> > that this is the right place, at least at this point in the series.
> >
>
> Because, pci->max_link_speed used to only cache the value of the
> 'max-link-speed' DT property. But I totally forgot that I changed that behavior
> to cache the max supported link speed of the Root Port with commit:
> 19a69cbd9d43 ("PCI: dwc: Always cache the maximum link speed value in
> dw_pcie::max_link_speed")
>
> So yes, I should've been using 'pci->max_link_speed' here.
>
> > At this point, qcom_ep_reset_deassert() is only used by
> > qcom_pcie_host_init(), so the flow is like this:
> >
> > qcom_pcie_probe
> > irq = platform_get_irq_byname_optional(pdev, "global")
> > if (irq > 0)
> > pp->use_linkup_irq = true
> > dw_pcie_host_init
> > pp->ops->init
> > qcom_pcie_host_init # .init
> > qcom_ep_reset_deassert # <--
> > + if (!pp->use_linkup_irq)
> > + msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
> > if (!dw_pcie_link_up(pci))
> > dw_pcie_start_link
> > if (!pp->use_linkup_irq)
> > dw_pcie_wait_for_link
> > for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
> > if (dw_pcie_link_up(pci))
> > break
> > msleep(PCIE_LINK_WAIT_SLEEP_MS) # 90ms
> > if (pci->max_link_speed > 2) # > 5.0 GT/s
> > msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
> >
> > For slow links (<= 5 GT/s), it's possible that the link comes up
> > before we even call dw_pcie_link_up() the first time, which would mean
> > we really don't wait at all.
> >
> > My guess is that most links wouldn't come up that fast but *would*
> > come up within 90ms. Even in that case, we wouldn't quite meet the
> > spec 100ms requirement.
> >
> > I wonder if dw_pcie_wait_for_link() should look more like this:
> >
> > dw_pcie_wait_for_link
> > if (pci->max_link_speed <= 2) # <= 5.0 GT/s
> > msleep(PCIE_RESET_CONFIG_WAIT_MS) dw_pcie_wait_for_link # 100ms
> >
> > for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
> > if (dw_pcie_link_up(pci))
> > break;
> > msleep(...)
> >
> > if (pci->max_link_speed > 2) # > 5.0 GT/s
> > msleep(PCIE_RESET_CONFIG_WAIT_MS) # 100ms
> >
> > Then we'd definitely wait the required 100ms even for the slow links.
> > The retry loop could start with a much shorter interval and back off.
> >
>
> Your concerns are valid. I'll submit a separate series along with *this* patch
> to fix these. I don't think clubbing these with this series is a good idea.
>
I think we missed one bigger issue:
As per r6.0, sec 6.6.1:
"Unless Readiness Notifications mechanisms are used, the Root Complex and/or
system software must allow at least 1.0 s following exit from a Conventional
Reset of a device, before determining that the device is broken if it fails to
return a Successful Completion status for a valid Configuration Request. This
period is independent of how quickly Link training completes."
So this clearly says that the software has to expect a successful completion
status for a valid configuration request sent to the device. But in
dw_pcie_link_up(), we are just waiting for the link training to be completed
by polling the controller specific 'Link Up' bit. And the spec also mentions
clearly that this delay is independent of how quickly the link training
completes.
So the 1.0 s delay doesn't seem to be applicable for link up. But in practice,
we do need to wait *some* time for the link to come up, but doesn't necessarily
1.0 s as mentioned in 6.6.1.
Also, if we go by the spec, we have to make sure that the controller drivers
poll for a successfull completion of a config request (like reading the VID)
of the device for 1.0 s. But how can we know where the device actually lives
in the downstream bus of the Root Port? I believe we cannot assume that the
device will always be in 0.0, or can we?
- Mani
--
மணிவண்ணன் சதாசிவம்
© 2016 - 2025 Red Hat, Inc.