Message-ID: <5vqicxeaqdljsahpsddrfcwrkqdoszq3k5ziw4kurqfwwwbjje@ynyzteelkiw4>
Date: Sat, 6 Sep 2025 20:10:55 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: Bjorn Helgaas <helgaas@...nel.org>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kwilczynski@...nel.org>,
        Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Saravana Kannan <saravanak@...gle.com>, linux-pci@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org,
        Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
        Brian Norris <briannorris@...omium.org>, stable+noautosel@...nel.org
Subject: Re: [PATCH v2 1/5] PCI: qcom: Wait for PCIE_RESET_CONFIG_WAIT_MS
 after PERST# deassert

On Sat, Sep 06, 2025 at 04:13:08PM GMT, Manivannan Sadhasivam wrote:
> On Fri, Sep 05, 2025 at 05:44:30PM GMT, Bjorn Helgaas wrote:
> > On Wed, Sep 03, 2025 at 12:43:23PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > PCIe spec r6.0, sec 6.6.1 mandates waiting for 100ms before deasserting
> > > PERST# if the downstream port does not support Link speeds greater than
> > > 5.0 GT/s.
> > 
> > I guess you mean we need to wait 100ms *after* deasserting PERST#?
> > 
> 
> Right, that's a typo.
> 
> > I.e., this wait before sending config requests to a downstream device:
> > 
> >   ◦ With a Downstream Port that does not support Link speeds greater
> >     than 5.0 GT/s, software must wait a minimum of 100 ms following
> >     exit from a Conventional Reset before sending a Configuration
> >     Request to the device immediately below that Port.
> > 
> > > But in practice, this delay seem to be required irrespective of
> > > the supported link speed as it gives the endpoints enough time to
> > > initialize.
> > 
> > Saying "but in practice ... seems to be required" suggests that the
> > spec requirement isn't actually enough.  But the spec does say the
> > 100ms delay before config requests is required for all link speeds.
> > The difference is when we start that timer: for 5 GT/s or slower it
> > starts at exit from Conventional Reset; for faster than 5 GT/s it
> > starts when link training completes.
> > 
> > > Hence, add the delay by reusing the PCIE_RESET_CONFIG_WAIT_MS definition if
> > > the linkup_irq is not supported. If the linkup_irq is supported, the driver
> > > already waits for 100ms in the IRQ handler post link up.
> > 
> > I didn't look into this, but I wondered whether it's possible to miss
> > the interrupt, especially the first time during probe.
> > 
> 
> No, it is not. Since, the controller reinitializes itself during probe() and it
> starts LTSSM. So once link up happens, this IRQ will be triggered.
> 
> > > Also, remove the redundant comment for PCIE_T_PVPERL_MS. Finally, the
> > > PERST_DELAY_US sleep can be moved to PERST# assert where it should be.
> > 
> > Unless this PERST_DELAY_US move is logically part of the
> > PCIE_RESET_CONFIG_WAIT_MS change, putting it in a separate patch would
> > make *this* patch easier to read.
> > 
> > > Cc: stable+noautosel@...nel.org # non-trivial dependency
> > > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..bcd080315d70e64eafdefd852740fe07df3dbe75 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -302,20 +302,22 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> > >  	else
> > >  		list_for_each_entry(port, &pcie->ports, list)
> > >  			gpiod_set_value_cansleep(port->reset, val);
> > > -
> > > -	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> > >  }
> > >  
> > >  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > >  {
> > >  	qcom_perst_assert(pcie, true);
> > > +	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> > >  }
> > >  
> > >  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> > >  {
> > > -	/* Ensure that PERST has been asserted for at least 100 ms */
> > > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> > > +
> > >  	msleep(PCIE_T_PVPERL_MS);
> > >  	qcom_perst_assert(pcie, false);
> > > +	if (!pp->use_linkup_irq)
> > > +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> > 
> > I'm a little confused about why you test pp->use_linkup_irq here
> > instead of testing the max supported link speed.  And I'm not positive
> > that this is the right place, at least at this point in the series.
> > 
> 
> Because, pci->max_link_speed used to only cache the value of the
> 'max-link-speed' DT property. But I totally forgot that I changed that behavior
> to cache the max supported link speed of the Root Port with commit:
> 19a69cbd9d43 ("PCI: dwc: Always cache the maximum link speed value in
> dw_pcie::max_link_speed")
> 
> So yes, I should've been using 'pci->max_link_speed' here.
> 
> > At this point, qcom_ep_reset_deassert() is only used by
> > qcom_pcie_host_init(), so the flow is like this:
> > 
> >   qcom_pcie_probe
> >     irq = platform_get_irq_byname_optional(pdev, "global")
> >     if (irq > 0)
> >       pp->use_linkup_irq = true
> >     dw_pcie_host_init
> >       pp->ops->init
> >         qcom_pcie_host_init                         # .init
> >           qcom_ep_reset_deassert                    # <--
> >  +          if (!pp->use_linkup_irq)
> >  +            msleep(PCIE_RESET_CONFIG_WAIT_MS)     # 100ms
> >       if (!dw_pcie_link_up(pci))
> >         dw_pcie_start_link
> >       if (!pp->use_linkup_irq)
> >         dw_pcie_wait_for_link
> >           for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
> >             if (dw_pcie_link_up(pci))
> >               break
> >             msleep(PCIE_LINK_WAIT_SLEEP_MS)         # 90ms
> >           if (pci->max_link_speed > 2)              # > 5.0 GT/s
> >             msleep(PCIE_RESET_CONFIG_WAIT_MS)       # 100ms
> > 
> > For slow links (<= 5 GT/s), it's possible that the link comes up
> > before we even call dw_pcie_link_up() the first time, which would mean
> > we really don't wait at all.
> > 
> > My guess is that most links wouldn't come up that fast but *would*
> > come up within 90ms.  Even in that case, we wouldn't quite meet the
> > spec 100ms requirement.
> > 
> > I wonder if dw_pcie_wait_for_link() should look more like this:
> > 
> >   dw_pcie_wait_for_link
> >     if (pci->max_link_speed <= 2)                   # <= 5.0 GT/s
> >       msleep(PCIE_RESET_CONFIG_WAIT_MS)         dw_pcie_wait_for_link    # 100ms
> > 
> >     for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
> >       if (dw_pcie_link_up(pci))
> >         break;
> >       msleep(...)
> > 
> >     if (pci->max_link_speed > 2)                    # > 5.0 GT/s
> >       msleep(PCIE_RESET_CONFIG_WAIT_MS)             # 100ms
> > 
> > Then we'd definitely wait the required 100ms even for the slow links.
> > The retry loop could start with a much shorter interval and back off.
> > 
> 
> Your concerns are valid. I'll submit a separate series along with *this* patch
> to fix these. I don't think clubbing these with this series is a good idea.
> 

I think we missed one bigger issue:

As per r6.0, sec 6.6.1:

"Unless Readiness Notifications mechanisms are used, the Root Complex and/or
system software must allow at least 1.0 s following exit from a Conventional
Reset of a device, before determining that the device is broken if it fails to
return a Successful Completion status for a valid Configuration Request. This
period is independent of how quickly Link training completes."

So this clearly says that the software has to expect a successful completion
status for a valid configuration request sent to the device. But in
dw_pcie_link_up(), we are just waiting for the link training to be completed
by polling the controller specific 'Link Up' bit. And the spec also mentions
clearly that this delay is independent of how quickly the link training
completes.

So the 1.0 s delay doesn't seem to be applicable for link up. But in practice,
we do need to wait *some* time for the link to come up, but doesn't necessarily
1.0 s as mentioned in 6.6.1.

Also, if we go by the spec, we have to make sure that the controller drivers
poll for a successfull completion of a config request (like reading the VID)
of the device for 1.0 s. But how can we know where the device actually lives
in the downstream bus of the Root Port? I believe we cannot assume that the
device will always be in 0.0, or can we?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists