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

[Bjorn Helgaas]

On Thu, Jun 12, 2025 at 08:33:54PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 12, 2025 at 09:44:47AM -0500, Bjorn Helgaas wrote:
> > 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.
> 
> Yeah, but the usage of 100ms is only valid if RRS SV is enabled by
> the software as per sec 6.6.1:
> 
> "It is strongly recommended that software use 100 ms wait periods
> only if software enables Configuration RRS Software Visibility".

I see that statement but don't understand it.  Do you think it's meant
as an exception to the first two paragraphs that say "software must
wait a minimum of 100 ms" following either "exit from Conventional
Reset" or "after Link training completes"?

I can't imagine that it means "if the Root Port doesn't support RRS SV
or software doesn't enable RRS SV, software needn't wait at all before
issuing config requests."

This whole thing is about whether the Endpoint is ready to respond.
A Root Port property (RRS SV support or enablement) doesn't tell us
anything about the Endpoint.