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

[Niklas Cassel <cassel@kernel.org>]

On Tue, Jun 03, 2025 at 01:12:50PM -0500, Bjorn Helgaas wrote:
> 
> Hmmm, sorry, I misinterpreted both 1/4 and 2/4.  I read them as "add
> this delay so the PLEXTOR device works", but in fact, I think in both
> cases, the delay is actually to enforce the PCIe r6.0, sec 6.6.1,
> requirement for software to wait 100ms before issuing a config
> request, and the fact that it makes PLEXTOR work is a side effect of
> that.

Well, the Plextor NVMe drive used to work with previous kernels,
but regressed.

But yes, the delay was added to enforce "PCIe r6.0, sec 6.6.1"
requirement for software to wait 100ms, which once again makes
the Plextor NVMe drive work.


> 
> The beginning of that 100ms delay is "exit from Conventional Reset"
> (ports that support <= 5.0 GT/s) or "link training completes" (ports
> that support > 5.0 GT/s).
> 
> I think we lack that 100ms delay in dwc drivers in general.  The only
> generic dwc delay is in dw_pcie_host_init() via the LINK_WAIT_SLEEP_MS
> in dw_pcie_wait_for_link(), but that doesn't count because it's
> *before* the link comes up.  We have to wait 100ms *after* exiting
> Conventional Reset or completing link training.

In dw_pcie_wait_for_link(), in the first iteration of the loop, the link
will never be up (because the link was just started),
dw_pcie_wait_for_link() will then sleep for LINK_WAIT_SLEEP_MS (90 ms),
before trying again.

Most likely the link training took way less than 100 ms, so most of those
90 ms will probably be after link training has completed.

That is most likely why Plextor worked on older kernels (which does not
use the link up IRQ).

If we add a 100 ms sleep after wait_for_link(), then I suggest that we
also reduce LINK_WAIT_SLEEP_MS to something shorter.


> 
> We don't know when the exit from Conventional Reset was, but it was
> certainly before the link came up.  In the absence of a timestamp for
> exit from reset, starting the wait after link-up is probably the best
> we can do.  This could be either after dw_pcie_wait_for_link() finds
> the link up or when we handle the link-up interrupt.
> 
> Patches 1 and 2 would fix the link-up interrupt case.  I think we need
> another patch for the dwc core for dw_pcie_wait_for_link().

I agree, sounds like a plan.


> 
> I wish I'd had time to spend on this and include patches 1 and 2, but
> we're up against the merge window wire and I'll be out the end of this
> week, so I think they'll have to wait.  It seems like something we can
> still justify for v6.16 though.

I think it sounds good to target this as fixes for v6.16.

Do you plan to send out something after -rc1, or do you prefer me to do it?


Kind regards,
Niklas