Re: [PATCH v4] PCI: dwc: Wait for link up only if link is started

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux