Replace the manual `do-while` polling loops with the readl_poll_timeout()
helper when checking the link DL_UP and DL_LINK_ACTIVE status bits
during link bring-up. This simplifies the code by removing the open-coded
timeout logic in favor of the standard, more robust iopoll framework.
The change improves readability and reduces code duplication.
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
drivers/pci/controller/pci-tegra.c | 38 ++++++++++++------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 3841489198b64..8e850f7c84e40 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -24,6 +24,7 @@
#include <linux/irqchip/chained_irq.h>
#include <linux/irqchip/irq-msi-lib.h>
#include <linux/irqdomain.h>
+#include <linux/iopoll.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -2157,37 +2158,28 @@ static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
value |= RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT;
writel(value, port->base + RP_PRIV_MISC);
- do {
- unsigned int timeout = TEGRA_PCIE_LINKUP_TIMEOUT;
-
- do {
- value = readl(port->base + RP_VEND_XP);
-
- if (value & RP_VEND_XP_DL_UP)
- break;
-
- usleep_range(1000, 2000);
- } while (--timeout);
+ while (retries--) {
+ int err;
- if (!timeout) {
+ err = readl_poll_timeout(port->base + RP_VEND_XP, value,
+ value & RP_VEND_XP_DL_UP,
+ 1000,
+ TEGRA_PCIE_LINKUP_TIMEOUT * 1000);
+ if (err) {
dev_dbg(dev, "link %u down, retrying\n", port->index);
goto retry;
}
- timeout = TEGRA_PCIE_LINKUP_TIMEOUT;
-
- do {
- value = readl(port->base + RP_LINK_CONTROL_STATUS);
-
- if (value & RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE)
- return true;
-
- usleep_range(1000, 2000);
- } while (--timeout);
+ err = readl_poll_timeout(port->base + RP_LINK_CONTROL_STATUS,
+ value,
+ value & RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE,
+ 1000, TEGRA_PCIE_LINKUP_TIMEOUT * 1000);
+ if (!err)
+ return true;
retry:
tegra_pcie_port_reset(port);
- } while (--retries);
+ }
return false;
}
--
2.50.1On Monday, September 1, 2025 4:00 AM Anand Moon wrote:
> Replace the manual `do-while` polling loops with the readl_poll_timeout()
> helper when checking the link DL_UP and DL_LINK_ACTIVE status bits
> during link bring-up. This simplifies the code by removing the open-coded
> timeout logic in favor of the standard, more robust iopoll framework.
> The change improves readability and reduces code duplication.
>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> drivers/pci/controller/pci-tegra.c | 38 ++++++++++++------------------
> 1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 3841489198b64..8e850f7c84e40 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -24,6 +24,7 @@
> #include <linux/irqchip/chained_irq.h>
> #include <linux/irqchip/irq-msi-lib.h>
> #include <linux/irqdomain.h>
> +#include <linux/iopoll.h>
There is already an iopoll.h include in this file, so this adds a duplicate.
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/module.h>
> @@ -2157,37 +2158,28 @@ static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
> value |= RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT;
> writel(value, port->base + RP_PRIV_MISC);
>
> - do {
> - unsigned int timeout = TEGRA_PCIE_LINKUP_TIMEOUT;
> -
> - do {
> - value = readl(port->base + RP_VEND_XP);
> -
> - if (value & RP_VEND_XP_DL_UP)
> - break;
> -
> - usleep_range(1000, 2000);
> - } while (--timeout);
> + while (retries--) {
> + int err;
>
> - if (!timeout) {
> + err = readl_poll_timeout(port->base + RP_VEND_XP, value,
> + value & RP_VEND_XP_DL_UP,
> + 1000,
> + TEGRA_PCIE_LINKUP_TIMEOUT * 1000);
The logic change here looks OK to me. This makes the timeout 200ms (TEGRA_PCIE_LINKUP_TIMEOUT is 200). Previously, the code looped 200 times with a 1 to 2ms sleep on each iteration. So the timeout could have been longer than 200ms previously, but not in a way that could be relied on.
> + if (err) {
> dev_dbg(dev, "link %u down, retrying\n", port->index);
> goto retry;
> }
>
> - timeout = TEGRA_PCIE_LINKUP_TIMEOUT;
> -
> - do {
> - value = readl(port->base + RP_LINK_CONTROL_STATUS);
> -
> - if (value & RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE)
> - return true;
> -
> - usleep_range(1000, 2000);
> - } while (--timeout);
> + err = readl_poll_timeout(port->base + RP_LINK_CONTROL_STATUS,
> + value,
> + value & RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE,
> + 1000, TEGRA_PCIE_LINKUP_TIMEOUT * 1000);
> + if (!err)
> + return true;
>
> retry:
> tegra_pcie_port_reset(port);
> - } while (--retries);
> + }
>
> return false;
> }
>
Hi Mikko,
Thanks for your review comments.
On Wed, 17 Sept 2025 at 08:51, Mikko Perttunen <mperttunen@nvidia.com> wrote:
>
> On Monday, September 1, 2025 4:00 AM Anand Moon wrote:
> > Replace the manual `do-while` polling loops with the readl_poll_timeout()
> > helper when checking the link DL_UP and DL_LINK_ACTIVE status bits
> > during link bring-up. This simplifies the code by removing the open-coded
> > timeout logic in favor of the standard, more robust iopoll framework.
> > The change improves readability and reduces code duplication.
> >
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > drivers/pci/controller/pci-tegra.c | 38 ++++++++++++------------------
> > 1 file changed, 15 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> > index 3841489198b64..8e850f7c84e40 100644
> > --- a/drivers/pci/controller/pci-tegra.c
> > +++ b/drivers/pci/controller/pci-tegra.c
> > @@ -24,6 +24,7 @@
> > #include <linux/irqchip/chained_irq.h>
> > #include <linux/irqchip/irq-msi-lib.h>
> > #include <linux/irqdomain.h>
> > +#include <linux/iopoll.h>
>
> There is already an iopoll.h include in this file, so this adds a duplicate.
>
Opps, I missed this in rebasing my code.
> > #include <linux/kernel.h>
> > #include <linux/init.h>
> > #include <linux/module.h>
> > @@ -2157,37 +2158,28 @@ static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
> > value |= RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT;
> > writel(value, port->base + RP_PRIV_MISC);
> >
> > - do {
> > - unsigned int timeout = TEGRA_PCIE_LINKUP_TIMEOUT;
> > -
> > - do {
> > - value = readl(port->base + RP_VEND_XP);
> > -
> > - if (value & RP_VEND_XP_DL_UP)
> > - break;
> > -
> > - usleep_range(1000, 2000);
> > - } while (--timeout);
> > + while (retries--) {
> > + int err;
> >
> > - if (!timeout) {
> > + err = readl_poll_timeout(port->base + RP_VEND_XP, value,
> > + value & RP_VEND_XP_DL_UP,
> > + 1000,
> > + TEGRA_PCIE_LINKUP_TIMEOUT * 1000);
>
> The logic change here looks OK to me. This makes the timeout 200ms (TEGRA_PCIE_LINKUP_TIMEOUT is 200). Previously, the code looped 200 times with a 1 to 2ms sleep on each iteration. So the timeout could have been longer than 200ms previously, but not in a way that could be relied on.
You're right; the original usleep_range(1000, 2000) had a variable sleep time.
To replicate the worst-case behavior of the old loop, the
readl_poll_timeout should
use a delay_us of 1000 and a timeout_us that matches the original
maximum duration.
Since the previous code looped 200 times with a maximum 2ms sleep,
the correct timeout is 400ms, so update (TEGRA_PCIE_LINKUP_TIMEOUT * 2000).
or increase TEGRA_PCIE_LINKUP_TIMEOUT to 400.
Are these changes ok with you?
Thank
-Anand
© 2016 - 2025 Red Hat, Inc.