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".
So I guess we should only have 3 delays in the drivers:
1. 100ms after PERST# deassert for ports supporting link speed < 5.0 GT/s (I
believe once the gpiod_ call returns, it could be considered as the exit from
the conventional reset).
2. 1s to poll for the link up after PERST# deassert.
3. 100ms after link up (either by polling or interrupt) for ports supporting
link speed > 5.0 GT/s.
> "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"?
>
Shorter the better :) Looks good to me.
- Mani
--
மணிவண்ணன் சதாசிவம்
