| Message ID | 20230412093425.3659088-1-ajayagarwal@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v4] PCI: dwc: Wait for link up only if link is started | expand |
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@google.com> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > --- > 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@google.com>
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? On Thu, Apr 20, 2023 at 12:59 AM William McVicker <willmcvicker@google.com> 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@google.com> > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > > --- > > 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@google.com>
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@google.com> 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@google.com> > > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > > > --- > > > 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@google.com>
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@kernel.org> 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@google.com> 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@google.com> > > > > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > > > > > --- > > > > > 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@google.com> > > Hi Lorenzo, Could you confirm that you'll be taking this patch for the v6.5 main PCI branch please? Thanks, Will
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@kernel.org> 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@google.com> 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@google.com> > > > > > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > > > > > > --- > > > > > > 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@google.com> > > > > > 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
On Wed, 12 Apr 2023 15:04:25 +0530, 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. > > [...] Applied to controller/dwc, thanks! [1/1] PCI: dwc: Wait for link up only if link is started https://git.kernel.org/pci/pci/c/da56a1bfbab5 Thanks, Lorenzo
Hi Ajay, On 12/04/2023 10:34, 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@google.com> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > --- > 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) > { After this change was merged we are seeing the following errors on some of our Tegra platforms ... tegra194-pcie 141a0000.pcie: Failed to add PCIe port: -110 tegra194-pcie 141a0000.pcie: Failed to initialize controller: -110 This is causing the probe of some of the PCIe controllers to fail. On some of our Tegra boards we do have some open PCIe slots and so the PCIe controllers are enabled, but there might not be a device connected. I am wondering if your change always assumes that there is a device connected? If that is the case, then this will not work for these boards. Note that previously we had changed a dev_err message to dev_info, because the link failing to come up was not actually an error for these boards. See ... commit 8405d8f0956d227c3355d9bdbabc23f79f721ce4 Author: Vidya Sagar <vidyas@nvidia.com> Date: Tue Sep 13 15:42:37 2022 +0530 PCI: dwc: Use dev_info for PCIe link down event logging Jon
Hi Jon, This issue has been reported on a QCOM platform as well. The patch will get reverted: https://lore.kernel.org/all/20230706082610.26584-1-johan+linaro@kernel.org/. Once it gets merged, I will upload a version of my patch where I would not fail the probe if the start_link() is defined and the link still does not come up. Johan, is your patch set to be picked in the next release? Regards Ajay
On Tue, Jul 25, 2023 at 11:10:26PM +0530, Ajay Agarwal wrote: > Hi Jon, > This issue has been reported on a QCOM platform as well. The patch will > get reverted: > https://lore.kernel.org/all/20230706082610.26584-1-johan+linaro@kernel.org/. > Once it gets merged, I will upload a version of my patch where I would > not fail the probe if the start_link() is defined and the link still > does not come up. > > Johan, is your patch set to be picked in the next release? I applied Johan's patch to for-linus, so it will appear in v6.5: https://lore.kernel.org/linux-pci/20230725200515.GA663333@bhelgaas/ Bjorn
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) {