Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

Niklas Cassel
2025-06-12T13:19:28Z

On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > and instead enumerate the bus directly after receiving the Link Up IRQ.
> >
> > This means that there is no longer any delay between link up and the bus
> > getting enumerated.
>
> Minor quibble about "no longer any delay": dw_pcie_wait_for_link()
> doesn't contain any explicit delay *after* we notice the link is up,
> so we didn't guarantee sufficient delay even before ec9fd499b9c6.
>
> If the link came up before the first check, dw_pcie_wait_for_link()
> didn't delay at all. Otherwise, it delayed 90ms * N, and we have no
> idea when in the 90ms period the link came up, so the post link-up
> delay was effectively some random amount between 0 and 90ms.
>
> I would propose something like:
>
> PCI: dw-rockchip: Wait PCIE_T_RRS_READY_MS after link-up IRQ
>
> Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
> 100ms (PCIE_T_RRS_READY_MS) after Link training completes before
> sending a Configuration Request.
>
> Prior to ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since
> we can detect Link Up"), dw-rockchip used dw_pcie_wait_for_link(),
> which waited between 0 and 90ms after the link came up before we
> enumerate the bus, and this was apparently enough for most devices.
>
> After ec9fd499b9c6, rockchip_pcie_rc_sys_irq_thread() started
> enumeration immediately when handling the link-up IRQ, and devices
> (e.g., Laszlo Fiat's PLEXTOR PX-256M8PeGN NVMe SSD) may not be ready
> to handle config requests yet.
>
> Delay PCIE_T_RRS_READY_MS after the link-up IRQ before starting
> enumeration.

Ok, I will shamelessly use this text verbatim.

> I think the comment at the PCIE_T_RRS_READY_MS definition should be
> enough (although it might need to be updated to mention link-up).
> This delay is going to be a standard piece of every driver, so it
> won't require special notice.

Looking at pci.h, we already have a comment mentioning exactly this
(link-up):
https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63

I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.

Kind regards,
Niklas