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@xxxxxxxxxx # non-trivial dependency
> > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> > 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
--
மணிவண்ணன் சதாசிவம்
