On Thu, Jun 12, 2025 at 10:24:42AM -0500, Bjorn Helgaas wrote:
> 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"?
>
Maybe yes.
> 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."
>
Yeah, it cannot be.
> 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.
I think we should just leave the RRS for good :) I was having a feeling that the
RRS define we have didn't fit the purpose of waiting for the device to be ready
post link up. Hence, I looked it up in the spec and spotted the above statement,
just to confuse you and myself.
- Mani
--
மணிவண்ணன் சதாசிவம்
