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

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

[Sajid Dalvi]

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.

Call trace when start_link() is not defined:
dw_pcie_wait_for_link << spins in a loop for 1 second
dw_pcie_host_init

Signed-off-by: Sajid Dalvi <sdalvi@google.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9952057c8819..9709f69f173e 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		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);
+		/* Ignore errors, the link may come up later */
+		dw_pcie_wait_for_link(pci);
+	}
 
 	bridge->sysdata = pp;
 
-- 
2.39.2.722.g9855ee24e9-goog

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

[Han Jingoo]

On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
>
> Call trace when start_link() is not defined:
> dw_pcie_wait_for_link << spins in a loop for 1 second
> dw_pcie_host_init
>
> Signed-off-by: Sajid Dalvi <sdalvi@google.com>

(CC'ed Krzysztof Kozlowski)

Acked-by: Jingoo Han <jingoohan1@gmail.com>

It looks good to me. I also checked the previous thread.
I agree with Krzysztof's opinion that we should include
only hardware-related features into DT.
Thank you.

Best regards,
Jingoo Han

> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 9952057c8819..9709f69f173e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>                 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);
> +               /* Ignore errors, the link may come up later */
> +               dw_pcie_wait_for_link(pci);
> +       }
>
>         bridge->sysdata = pp;
>
> --
> 2.39.2.722.g9855ee24e9-goog
>

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

[Sajid Dalvi]

Thanks for your review Jingoo.
Sajid

On Tue, Feb 28, 2023 at 4:04 PM Han Jingoo <jingoohan1@gmail.com> wrote:
>
> On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
> >
> > Call trace when start_link() is not defined:
> > dw_pcie_wait_for_link << spins in a loop for 1 second
> > dw_pcie_host_init
> >
> > Signed-off-by: Sajid Dalvi <sdalvi@google.com>
>
> (CC'ed Krzysztof Kozlowski)
>
> Acked-by: Jingoo Han <jingoohan1@gmail.com>
>
> It looks good to me. I also checked the previous thread.
> I agree with Krzysztof's opinion that we should include
> only hardware-related features into DT.
> Thank you.
>
> Best regards,
> Jingoo Han
>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 9952057c8819..9709f69f173e 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >                 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);
> > +               /* Ignore errors, the link may come up later */
> > +               dw_pcie_wait_for_link(pci);
> > +       }
> >
> >         bridge->sysdata = pp;
> >
> > --
> > 2.39.2.722.g9855ee24e9-goog
> >

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

[Sajid Dalvi]

On Tue, Feb 28, 2023 at 10:36 PM Sajid Dalvi <sdalvi@google.com> wrote:
>
> Thanks for your review Jingoo.
> Sajid
>
> On Tue, Feb 28, 2023 at 4:04 PM Han Jingoo <jingoohan1@gmail.com> wrote:
> >
> > On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
> > >
> > > Call trace when start_link() is not defined:
> > > dw_pcie_wait_for_link << spins in a loop for 1 second
> > > dw_pcie_host_init
> > >
> > > Signed-off-by: Sajid Dalvi <sdalvi@google.com>
> >
> > (CC'ed Krzysztof Kozlowski)
> >
> > Acked-by: Jingoo Han <jingoohan1@gmail.com>
> >
> > It looks good to me. I also checked the previous thread.
> > I agree with Krzysztof's opinion that we should include
> > only hardware-related features into DT.
> > Thank you.
> >
> > Best regards,
> > Jingoo Han
> >
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 9952057c8819..9709f69f173e 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >                 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);
> > > +               /* Ignore errors, the link may come up later */
> > > +               dw_pcie_wait_for_link(pci);
> > > +       }
> > >
> > >         bridge->sysdata = pp;
> > >
> > > --
> > > 2.39.2.722.g9855ee24e9-goog
> > >

@bhelgaas Can this be picked up in your tree:
 https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/

Thanks
Sajid

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

[Lorenzo Pieralisi]

On Thu, Mar 16, 2023 at 06:05:02PM -0500, Sajid Dalvi wrote:
> On Tue, Feb 28, 2023 at 10:36 PM Sajid Dalvi <sdalvi@google.com> wrote:
> >
> > Thanks for your review Jingoo.
> > Sajid
> >
> > On Tue, Feb 28, 2023 at 4:04 PM Han Jingoo <jingoohan1@gmail.com> wrote:
> > >
> > > On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
> > > >
> > > > Call trace when start_link() is not defined:
> > > > dw_pcie_wait_for_link << spins in a loop for 1 second
> > > > dw_pcie_host_init
> > > >
> > > > Signed-off-by: Sajid Dalvi <sdalvi@google.com>
> > >
> > > (CC'ed Krzysztof Kozlowski)
> > >
> > > Acked-by: Jingoo Han <jingoohan1@gmail.com>
> > >
> > > It looks good to me. I also checked the previous thread.
> > > I agree with Krzysztof's opinion that we should include
> > > only hardware-related features into DT.
> > > Thank you.
> > >
> > > Best regards,
> > > Jingoo Han
> > >
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index 9952057c8819..9709f69f173e 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > >                 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);
> > > > +               /* Ignore errors, the link may come up later */
> > > > +               dw_pcie_wait_for_link(pci);
> > > > +       }
> > > >
> > > >         bridge->sysdata = pp;
> > > >
> > > > --
> > > > 2.39.2.722.g9855ee24e9-goog
> > > >
> 
> @bhelgaas Can this be picked up in your tree:
>  https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/

This patch seems fine to me. The question I have though is why the
*current* code is written the way it is. Perhaps it is just the way
it is, I wonder whether this change can trigger a regression though.

I guess the only way to know is merging this path and check the
fallout.

Lorenzo

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

[Bjorn Helgaas]

On Wed, Apr 05, 2023 at 03:24:36PM +0200, Lorenzo Pieralisi wrote:
> On Thu, Mar 16, 2023 at 06:05:02PM -0500, Sajid Dalvi wrote:
> > On Tue, Feb 28, 2023 at 10:36 PM Sajid Dalvi <sdalvi@google.com> wrote:
> > >
> > > Thanks for your review Jingoo.
> > > Sajid
> > >
> > > On Tue, Feb 28, 2023 at 4:04 PM Han Jingoo <jingoohan1@gmail.com> wrote:
> > > >
> > > > On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
> > > > >
> > > > > Call trace when start_link() is not defined:
> > > > > dw_pcie_wait_for_link << spins in a loop for 1 second
> > > > > dw_pcie_host_init
> > > > >
> > > > > Signed-off-by: Sajid Dalvi <sdalvi@google.com>
> > > >
> > > > (CC'ed Krzysztof Kozlowski)
> > > >
> > > > Acked-by: Jingoo Han <jingoohan1@gmail.com>
> > > >
> > > > It looks good to me. I also checked the previous thread.
> > > > I agree with Krzysztof's opinion that we should include
> > > > only hardware-related features into DT.
> > > > Thank you.
> > > >
> > > > Best regards,
> > > > Jingoo Han
> > > >
> > > > > ---
> > > > >  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > index 9952057c8819..9709f69f173e 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > > >                 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);
> > > > > +               /* Ignore errors, the link may come up later */
> > > > > +               dw_pcie_wait_for_link(pci);
> > > > > +       }
> > > > >
> > > > >         bridge->sysdata = pp;
> > > > >
> > > > > --
> > > > > 2.39.2.722.g9855ee24e9-goog
> > > > >
> > 
> > @bhelgaas Can this be picked up in your tree:
> >  https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/
> 
> This patch seems fine to me. The question I have though is why the
> *current* code is written the way it is. Perhaps it is just the way
> it is, I wonder whether this change can trigger a regression though.

The new code will look basically like this:

  if (!dw_pcie_link_up(pci)) {
    dw_pcie_start_link(pci);
    dw_pcie_wait_for_link(pci);
  }

If the link is already up by the time we get here, this change means
we won't get this message emitted by dw_pcie_wait_for_link():

  dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", ...)

I don't know how important that is, but I bet somebody cares about it.

From the commit log, I expected the patch to do something based on
whether ->start_link() was defined, but there really isn't a direct
connection, so maybe the log could be refined.

Bjorn

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

[William McVicker]

On 04/05/2023, Bjorn Helgaas wrote:
> On Wed, Apr 05, 2023 at 03:24:36PM +0200, Lorenzo Pieralisi wrote:
> > On Thu, Mar 16, 2023 at 06:05:02PM -0500, Sajid Dalvi wrote:
> > > On Tue, Feb 28, 2023 at 10:36 PM Sajid Dalvi <sdalvi@google.com> wrote:
> > > >
> > > > Thanks for your review Jingoo.
> > > > Sajid
> > > >
> > > > On Tue, Feb 28, 2023 at 4:04 PM Han Jingoo <jingoohan1@gmail.com> wrote:
> > > > >
> > > > > On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
> > > > > >
> > > > > > Call trace when start_link() is not defined:
> > > > > > dw_pcie_wait_for_link << spins in a loop for 1 second
> > > > > > dw_pcie_host_init
> > > > > >
> > > > > > Signed-off-by: Sajid Dalvi <sdalvi@google.com>
> > > > >
> > > > > (CC'ed Krzysztof Kozlowski)
> > > > >
> > > > > Acked-by: Jingoo Han <jingoohan1@gmail.com>
> > > > >
> > > > > It looks good to me. I also checked the previous thread.
> > > > > I agree with Krzysztof's opinion that we should include
> > > > > only hardware-related features into DT.
> > > > > Thank you.
> > > > >
> > > > > Best regards,
> > > > > Jingoo Han
> > > > >
> > > > > > ---
> > > > > >  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > index 9952057c8819..9709f69f173e 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > >                 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);
> > > > > > +               /* Ignore errors, the link may come up later */
> > > > > > +               dw_pcie_wait_for_link(pci);
> > > > > > +       }
> > > > > >
> > > > > >         bridge->sysdata = pp;
> > > > > >
> > > > > > --
> > > > > > 2.39.2.722.g9855ee24e9-goog
> > > > > >
> > > 
> > > @bhelgaas Can this be picked up in your tree:
> > >  https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/
> > 
> > This patch seems fine to me. The question I have though is why the
> > *current* code is written the way it is. Perhaps it is just the way
> > it is, I wonder whether this change can trigger a regression though.
> 
> The new code will look basically like this:
> 
>   if (!dw_pcie_link_up(pci)) {
>     dw_pcie_start_link(pci);
>     dw_pcie_wait_for_link(pci);
>   }
> 
> If the link is already up by the time we get here, this change means
> we won't get this message emitted by dw_pcie_wait_for_link():
> 
>   dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", ...)
> 
> I don't know how important that is, but I bet somebody cares about it.
> 
> From the commit log, I expected the patch to do something based on
> whether ->start_link() was defined, but there really isn't a direct
> connection, so maybe the log could be refined.
> 
> Bjorn
> 
> -- 
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> 

After taking a deeper dive into this patch, I found that [1] changes the
original intent which was to skip the call to dw_pcie_wait_for_link()
when pci->ops->start_link is NULL. I talked to Sajid offline and he
agreed we should put back the start_link NULL check. The updated patch
should look like this:

  if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
    ret = dw_pcie_start_link(pci);
    if (ret)
      goto err_free_msi;
    dw_pcie_wait_for_link(pci);
  }


...which will ensure that we don't call dw_pcie_wait_for_link() when
pci->ops->start_link is NULL.

With regards to the log, I think there are 2 ways to solve this:

1) We could also call dw_pcie_wait_for_link() in a new else if
   dw_pcie_link_up() returns 1.
2) We could add this to the top of dw_pcie_wait_for_link() and leave the
   code as is:

   if (!pci->ops || !pci->ops->start_link)
     return 0;

I kind of like (2) since that solves both Sajid's original issue and
will keep the original log.

[1] https://lore.kernel.org/all/20220624143428.8334-14-Sergey.Semin@baikalelectronics.ru/

Regards,
Will

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

[William McVicker]

On 04/05/2023, William McVicker wrote:
> On 04/05/2023, Bjorn Helgaas wrote:
> > On Wed, Apr 05, 2023 at 03:24:36PM +0200, Lorenzo Pieralisi wrote:
> > > On Thu, Mar 16, 2023 at 06:05:02PM -0500, Sajid Dalvi wrote:
> > > > On Tue, Feb 28, 2023 at 10:36 PM Sajid Dalvi <sdalvi@google.com> wrote:
> > > > >
> > > > > Thanks for your review Jingoo.
> > > > > Sajid
> > > > >
> > > > > On Tue, Feb 28, 2023 at 4:04 PM Han Jingoo <jingoohan1@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
> > > > > > >
> > > > > > > Call trace when start_link() is not defined:
> > > > > > > dw_pcie_wait_for_link << spins in a loop for 1 second
> > > > > > > dw_pcie_host_init
> > > > > > >
> > > > > > > Signed-off-by: Sajid Dalvi <sdalvi@google.com>
> > > > > >
> > > > > > (CC'ed Krzysztof Kozlowski)
> > > > > >
> > > > > > Acked-by: Jingoo Han <jingoohan1@gmail.com>
> > > > > >
> > > > > > It looks good to me. I also checked the previous thread.
> > > > > > I agree with Krzysztof's opinion that we should include
> > > > > > only hardware-related features into DT.
> > > > > > Thank you.
> > > > > >
> > > > > > Best regards,
> > > > > > Jingoo Han
> > > > > >
> > > > > > > ---
> > > > > > >  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
> > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > index 9952057c8819..9709f69f173e 100644
> > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > > >                 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);
> > > > > > > +               /* Ignore errors, the link may come up later */
> > > > > > > +               dw_pcie_wait_for_link(pci);
> > > > > > > +       }
> > > > > > >
> > > > > > >         bridge->sysdata = pp;
> > > > > > >
> > > > > > > --
> > > > > > > 2.39.2.722.g9855ee24e9-goog
> > > > > > >
> > > > 
> > > > @bhelgaas Can this be picked up in your tree:
> > > >  https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/
> > > 
> > > This patch seems fine to me. The question I have though is why the
> > > *current* code is written the way it is. Perhaps it is just the way
> > > it is, I wonder whether this change can trigger a regression though.
> > 
> > The new code will look basically like this:
> > 
> >   if (!dw_pcie_link_up(pci)) {
> >     dw_pcie_start_link(pci);
> >     dw_pcie_wait_for_link(pci);
> >   }
> > 
> > If the link is already up by the time we get here, this change means
> > we won't get this message emitted by dw_pcie_wait_for_link():
> > 
> >   dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", ...)
> > 
> > I don't know how important that is, but I bet somebody cares about it.
> > 
> > From the commit log, I expected the patch to do something based on
> > whether ->start_link() was defined, but there really isn't a direct
> > connection, so maybe the log could be refined.
> > 
> > Bjorn
> > 
> > -- 
> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > 
> 
> After taking a deeper dive into this patch, I found that [1] changes the
> original intent which was to skip the call to dw_pcie_wait_for_link()
> when pci->ops->start_link is NULL. I talked to Sajid offline and he
> agreed we should put back the start_link NULL check. The updated patch
> should look like this:
> 
>   if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
>     ret = dw_pcie_start_link(pci);
>     if (ret)
>       goto err_free_msi;
>     dw_pcie_wait_for_link(pci);
>   }
> 
> 
> ...which will ensure that we don't call dw_pcie_wait_for_link() when
> pci->ops->start_link is NULL.
> 
> With regards to the log, I think there are 2 ways to solve this:
> 
> 1) We could also call dw_pcie_wait_for_link() in a new else if
>    dw_pcie_link_up() returns 1.
> 2) We could add this to the top of dw_pcie_wait_for_link() and leave the
>    code as is:
> 
>    if (!pci->ops || !pci->ops->start_link)
>      return 0;
> 
> I kind of like (2) since that solves both Sajid's original issue and
> will keep the original log.
> 
> [1] https://lore.kernel.org/all/20220624143428.8334-14-Sergey.Semin@baikalelectronics.ru/
> 
> Regards,
> Will

Below is what I'm thinking will do the job. I verified on a Pixel 6
(which doesn't have start_link() defined) that we don't have the 1
second wait from dw_pcie_wait_for_link() during probe.

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 8e33e6e59e68..1bf04324ad2d 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -648,13 +648,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 {
 	u32 offset, val;
 	int retries;
+	int link_up = dw_pcie_link_up(pci);
 
-	/* Check if the link is up or not */
-	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
-		if (dw_pcie_link_up(pci))
-			break;
+	if (!link_up && !(pci->ops && pci->ops->start_link))
+		return 0;
 
+	/* Check if the link is up or not */
+	for (retries = 0; !link_up && retries < LINK_WAIT_MAX_RETRIES; retries++) {
 		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
+
+		link_up = dw_pcie_link_up(pci);
 	}
 
 	if (retries >= LINK_WAIT_MAX_RETRIES) {

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

[Ajay Agarwal]

On Wed, Apr 05, 2023 at 12:23:47PM -0700, William McVicker wrote:
> On 04/05/2023, William McVicker wrote:
> > On 04/05/2023, Bjorn Helgaas wrote:
> > > On Wed, Apr 05, 2023 at 03:24:36PM +0200, Lorenzo Pieralisi wrote:
> > > > On Thu, Mar 16, 2023 at 06:05:02PM -0500, Sajid Dalvi wrote:
> > > > > On Tue, Feb 28, 2023 at 10:36 PM Sajid Dalvi <sdalvi@google.com> wrote:
> > > > > >
> > > > > > Thanks for your review Jingoo.
> > > > > > Sajid
> > > > > >
> > > > > > On Tue, Feb 28, 2023 at 4:04 PM Han Jingoo <jingoohan1@gmail.com> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 27, 2023, Sajid Dalvi <sdalvi@google.com> 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.
> > > > > > > >
> > > > > > > > Call trace when start_link() is not defined:
> > > > > > > > dw_pcie_wait_for_link << spins in a loop for 1 second
> > > > > > > > dw_pcie_host_init
> > > > > > > >
> > > > > > > > Signed-off-by: Sajid Dalvi <sdalvi@google.com>
> > > > > > >
> > > > > > > (CC'ed Krzysztof Kozlowski)
> > > > > > >
> > > > > > > Acked-by: Jingoo Han <jingoohan1@gmail.com>
> > > > > > >
> > > > > > > It looks good to me. I also checked the previous thread.
> > > > > > > I agree with Krzysztof's opinion that we should include
> > > > > > > only hardware-related features into DT.
> > > > > > > Thank you.
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Jingoo Han
> > > > > > >
> > > > > > > > ---
> > > > > > > >  drivers/pci/controller/dwc/pcie-designware-host.c | 6 +++---
> > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > index 9952057c8819..9709f69f173e 100644
> > > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > @@ -489,10 +489,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > > > >                 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);
> > > > > > > > +               /* Ignore errors, the link may come up later */
> > > > > > > > +               dw_pcie_wait_for_link(pci);
> > > > > > > > +       }
> > > > > > > >
> > > > > > > >         bridge->sysdata = pp;
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.39.2.722.g9855ee24e9-goog
> > > > > > > >
> > > > > 
> > > > > @bhelgaas Can this be picked up in your tree:
> > > > >  https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/
> > > > 
> > > > This patch seems fine to me. The question I have though is why the
> > > > *current* code is written the way it is. Perhaps it is just the way
> > > > it is, I wonder whether this change can trigger a regression though.
> > > 
> > > The new code will look basically like this:
> > > 
> > >   if (!dw_pcie_link_up(pci)) {
> > >     dw_pcie_start_link(pci);
> > >     dw_pcie_wait_for_link(pci);
> > >   }
> > > 
> > > If the link is already up by the time we get here, this change means
> > > we won't get this message emitted by dw_pcie_wait_for_link():
> > > 
> > >   dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", ...)
> > > 
> > > I don't know how important that is, but I bet somebody cares about it.
> > > 
> > > From the commit log, I expected the patch to do something based on
> > > whether ->start_link() was defined, but there really isn't a direct
> > > connection, so maybe the log could be refined.
> > > 
> > > Bjorn
> > > 
> > > -- 
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > > 
> > 
> > After taking a deeper dive into this patch, I found that [1] changes the
> > original intent which was to skip the call to dw_pcie_wait_for_link()
> > when pci->ops->start_link is NULL. I talked to Sajid offline and he
> > agreed we should put back the start_link NULL check. The updated patch
> > should look like this:
> > 
> >   if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
> >     ret = dw_pcie_start_link(pci);
> >     if (ret)
> >       goto err_free_msi;
> >     dw_pcie_wait_for_link(pci);
> >   }
> > 
> > 
> > ...which will ensure that we don't call dw_pcie_wait_for_link() when
> > pci->ops->start_link is NULL.
> > 
> > With regards to the log, I think there are 2 ways to solve this:
> > 
> > 1) We could also call dw_pcie_wait_for_link() in a new else if
> >    dw_pcie_link_up() returns 1.
> > 2) We could add this to the top of dw_pcie_wait_for_link() and leave the
> >    code as is:
> > 
> >    if (!pci->ops || !pci->ops->start_link)
> >      return 0;
> > 
> > I kind of like (2) since that solves both Sajid's original issue and
> > will keep the original log.
> > 
> > [1] https://lore.kernel.org/all/20220624143428.8334-14-Sergey.Semin@baikalelectronics.ru/
> > 
> > Regards,
> > Will
> 
> Below is what I'm thinking will do the job. I verified on a Pixel 6
> (which doesn't have start_link() defined) that we don't have the 1
> second wait from dw_pcie_wait_for_link() during probe.
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 8e33e6e59e68..1bf04324ad2d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -648,13 +648,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>  {
>  	u32 offset, val;
>  	int retries;
> +	int link_up = dw_pcie_link_up(pci);
>  
> -	/* Check if the link is up or not */
> -	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> -		if (dw_pcie_link_up(pci))
> -			break;
> +	if (!link_up && !(pci->ops && pci->ops->start_link))
> +		return 0;
There is a problem with this approach. A platform driver could enable
link training internally, i.e., it does not have the start_link() pointer
defined. Then it could call `dw_pcie_wait_for_link` to wait for the link
to come up. (See pcie-intel-gw.c for an example of such a platform).
Your logic will end up regressing this driver by exiting early.
>  
> +	/* Check if the link is up or not */
> +	for (retries = 0; !link_up && retries < LINK_WAIT_MAX_RETRIES; retries++) {
>  		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> +
> +		link_up = dw_pcie_link_up(pci);
>  	}
>  
>  	if (retries >= LINK_WAIT_MAX_RETRIES) {
> 
The problem of the log is still not solved for a platform which could
have the link up by default, i.e., it does not need to explicitly enable
link training.

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

[Ajay Agarwal]

Here is my attempt at a patch which can satisfy all the requirements
(Ideally, I did not want to use `pci->ops` in the host driver but I
could not figure out any other way):

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9952057c8819..39c7219ec7c9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -485,15 +485,18 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		goto err_remove_edma;
 
-	if (!dw_pcie_link_up(pci)) {
-		ret = dw_pcie_start_link(pci);
+	ret = dw_pcie_start_link(pci);
+	if (ret)
+		goto err_remove_edma;
+
+	if (dw_pcie_link_up(pci)) {
+		dw_pcie_print_link_status(pci);
+	} else if (pci->ops && pci->ops->start_link) {
+		ret = dw_pcie_wait_for_link(pci);
 		if (ret)
-			goto err_remove_edma;
+			goto err_stop_link;
 	}
 
-	/* Ignore errors, the link may come up later */
-	dw_pcie_wait_for_link(pci);
-
 	bridge->sysdata = pp;
 
 	ret = pci_host_probe(bridge);
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)
 {

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

[William McVicker]

On 04/06/2023, 'Ajay Agarwal' via kernel-team wrote:
> Here is my attempt at a patch which can satisfy all the requirements
> (Ideally, I did not want to use `pci->ops` in the host driver but I
> could not figure out any other way):
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 9952057c8819..39c7219ec7c9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -485,15 +485,18 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret)
>  		goto err_remove_edma;
>  
> -	if (!dw_pcie_link_up(pci)) {
> -		ret = dw_pcie_start_link(pci);
> +	ret = dw_pcie_start_link(pci);
> +	if (ret)
> +		goto err_remove_edma;
> +
> +	if (dw_pcie_link_up(pci)) {
> +		dw_pcie_print_link_status(pci);
> +	} else if (pci->ops && pci->ops->start_link) {
> +		ret = dw_pcie_wait_for_link(pci);
>  		if (ret)
> -			goto err_remove_edma;
> +			goto err_stop_link;
>  	}
>  
> -	/* Ignore errors, the link may come up later */
> -	dw_pcie_wait_for_link(pci);
> -
>  	bridge->sysdata = pp;
>  
>  	ret = pci_host_probe(bridge);
> 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)
>  {
> 
> -- 
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> 

Thanks Ajay for the follow-up patch! I've tested it out on a Pixel 6 and
it's working as intended for me. Probing the PCIe RC device now only
take 0.02s vs ~1.02s. If others don't object, please send it as a v3
patch.

Thanks,
Will