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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jun 12, 2025 at 06:30:37PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 12, 2025 at 07:21:08AM -0500, Bjorn Helgaas wrote:
> > On Thu, Jun 12, 2025 at 01:40:23PM +0200, Niklas Cassel wrote:
> > > On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > > > > 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.
> > > > 
> > > > > > 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.
> > > > 
> > > > I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> > > > exist.  It looks like they got merged at about the same time by
> > > > different people, so we didn't notice.
> > > 
> > > I came to the same conclusion, I will send a patch to remove
> > > PCIE_T_RRS_READY_MS and convert the only existing user to use
> > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS.
> > 
> > I think PCIE_T_RRS_READY_MS expresses the purpose of the wait more
> > specifically.  It's not that the device is completely ready after
> > 100ms; just that it should be able to respond with RRS if it needs
> > more time.
> 
> Yes, but none of the drivers are checking for the RRS status
> currently. So using PCIE_T_RRS_READY_MS gives a wrong impression
> that the driver is waiting for the RRS status from the device.

There's 100ms immediately after reset or link-up when we can't send
config requests because the device may not be able to respond at all.

After 100ms, the device should be able to respond to config requests
with SC, UR, RRS, or CA status (sec 2.2.9.1).  If it responds with
RRS, the access should be retried either by hardware or (if RRS SV is
enabled) by software.  This is the origin of "RRS_READY" -- the device
can at least do RRS.

"CONFIG_READY" would make sense except that it would be confused with
the spec's usage of "Configuration Ready" (unfortunately not formally
defined).  The PCIe r6.0, sec 6.22 implementation note says devices
may take up to 1 second to become Configuration Ready, and that when a
device is Configuration Ready, system software can proceed without
further delay to configure the device.

"PCIE_RESET_CONFIG_DEVICE_WAIT_MS" seems a little long to me (we might
not need "DEVICE"), but it does include "CONFIG" which is definitely
relevant.  "PCIE_RESET_CONFIG_WAIT_MS"?

Bjorn