On Fri, May 12, 2023 at 10:00:30AM -0700, William McVicker wrote:
> On 05/08/2023, Ajay Agarwal wrote:
> > Sure, understood. Thanks Bjorn for the explanation.
> > If v6.4-rc1 is tagged by now, please consider applying this patch to the
> > main branch.
> >
> > -Ajay
> >
> > On Wed, Apr 26, 2023 at 12:37 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> >
> > > On Tue, Apr 25, 2023 at 11:44:59PM +0530, Ajay Agarwal wrote:
> > > > Thanks for the review and testing the patch William!
> > > >
> > > > Hi Lorenzo
> > > > Can you please include this patch in the next release if it looks good?
> > >
> > > Just to set expectations, the v6.4 merge window is open now, so we'll
> > > be asking Linus to pull everything that has already been merged.
> > > v6.4-rc1 should be tagged May 7, and then we'll start applying patches
> > > (like this one) that are destined for v6.5.
> > >
> > > Bjorn
> > >
> > > > On Thu, Apr 20, 2023 at 12:59 AM William McVicker
> > > > <willmcvicker@xxxxxxxxxx> wrote:
> > > > >
> > > > > On 04/12/2023, Ajay Agarwal wrote:
> > > > > > In dw_pcie_host_init() regardless of whether the link has been
> > > > > > started or not, the code waits for the link to come up. Even in
> > > > > > cases where start_link() is not defined the code ends up spinning
> > > > > > in a loop for 1 second. Since in some systems dw_pcie_host_init()
> > > > > > gets called during probe, this one second loop for each pcie
> > > > > > interface instance ends up extending the boot time.
> > > > > >
> > > > > > Wait for the link up in only if the start_link() is defined.
> > > > > >
> > > > > > Signed-off-by: Sajid Dalvi <sdalvi@xxxxxxxxxx>
> > > > > > Signed-off-by: Ajay Agarwal <ajayagarwal@xxxxxxxxxx>
> > > > > > ---
> > > > > > Changelog since v3:
> > > > > > - Run dw_pcie_start_link() only if link is not already up
> > > > > >
> > > > > > Changelog since v2:
> > > > > > - Wait for the link up if start_link() is really defined.
> > > > > > - Print the link status if the link is up on init.
> > > > > >
> > > > > > .../pci/controller/dwc/pcie-designware-host.c | 13 ++++++++----
> > > > > > drivers/pci/controller/dwc/pcie-designware.c | 20
> > > ++++++++++++-------
> > > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 +
> > > > > > 3 files changed, 23 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > index 9952057c8819..cf61733bf78d 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > @@ -485,14 +485,19 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > > if (ret)
> > > > > > goto err_remove_edma;
> > > > > >
> > > > > > - if (!dw_pcie_link_up(pci)) {
> > > > > > + if (dw_pcie_link_up(pci)) {
> > > > > > + dw_pcie_print_link_status(pci);
> > > > > > + } else {
> > > > > > ret = dw_pcie_start_link(pci);
> > > > > > if (ret)
> > > > > > goto err_remove_edma;
> > > > > > - }
> > > > > >
> > > > > > - /* Ignore errors, the link may come up later */
> > > > > > - dw_pcie_wait_for_link(pci);
> > > > > > + if (pci->ops && pci->ops->start_link) {
> > > > > > + ret = dw_pcie_wait_for_link(pci);
> > > > > > + if (ret)
> > > > > > + goto err_stop_link;
> > > > > > + }
> > > > > > + }
> > > > > >
> > > > > > bridge->sysdata = pp;
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > > b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > index 53a16b8b6ac2..03748a8dffd3 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > @@ -644,9 +644,20 @@ void dw_pcie_disable_atu(struct dw_pcie *pci,
> > > u32 dir, int index)
> > > > > > dw_pcie_writel_atu(pci, dir, index, PCIE_ATU_REGION_CTRL2, 0);
> > > > > > }
> > > > > >
> > > > > > -int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > > > > > +void dw_pcie_print_link_status(struct dw_pcie *pci)
> > > > > > {
> > > > > > u32 offset, val;
> > > > > > +
> > > > > > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > > > > + val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> > > > > > +
> > > > > > + dev_info(pci->dev, "PCIe Gen.%u x%u link up\n",
> > > > > > + FIELD_GET(PCI_EXP_LNKSTA_CLS, val),
> > > > > > + FIELD_GET(PCI_EXP_LNKSTA_NLW, val));
> > > > > > +}
> > > > > > +
> > > > > > +int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > > > > > +{
> > > > > > int retries;
> > > > > >
> > > > > > /* Check if the link is up or not */
> > > > > > @@ -662,12 +673,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > > > > > return -ETIMEDOUT;
> > > > > > }
> > > > > >
> > > > > > - offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > > > > - val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> > > > > > -
> > > > > > - dev_info(pci->dev, "PCIe Gen.%u x%u link up\n",
> > > > > > - FIELD_GET(PCI_EXP_LNKSTA_CLS, val),
> > > > > > - FIELD_GET(PCI_EXP_LNKSTA_NLW, val));
> > > > > > + dw_pcie_print_link_status(pci);
> > > > > >
> > > > > > return 0;
> > > > > > }
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > > b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > index 79713ce075cc..615660640801 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > @@ -429,6 +429,7 @@ void dw_pcie_setup(struct dw_pcie *pci);
> > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci);
> > > > > > int dw_pcie_edma_detect(struct dw_pcie *pci);
> > > > > > void dw_pcie_edma_remove(struct dw_pcie *pci);
> > > > > > +void dw_pcie_print_link_status(struct dw_pcie *pci);
> > > > > >
> > > > > > static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg,
> > > u32 val)
> > > > > > {
> > > > > > --
> > > > > > 2.40.0.577.gac1e443424-goog
> > > > > >
> > > > >
> > > > > Thanks for sending this patch Ajay! I tested this on my Pixel 6 with
> > > 6.3-rc1.
> > > > > I verified the PCIe RC probes without the 1s delay in
> > > dw_pcie_wait_for_link().
> > > > > Feel free to include
> > > > >
> > > > > Tested-by: Will McVicker <willmcvicker@xxxxxxxxxx>
> > >
>
> Hi Lorenzo,
>
> Could you confirm that you'll be taking this patch for the v6.5 main PCI branch
> please?
>
> Thanks,
> Will
Hi Lorenzo,
Can you please provide your comment here?
Thanks
Ajay
