[RFC v1 0/2] PCI: tegra: A couple of cleanups

[RFC v1 0/2] PCI: tegra: A couple of cleanups

[Anand Moon]

Hi All,

This small series provides two cleanup patches for the Tegra PCIe driver. 
The overall goal is to replace custom, open-coded logic with standard
kernel helper functions.

These changes improve the driver's readability and maintainability by 
everaging modern, well-tested APIs for clock management and register
polling.

Thanks
-Anand 

Anand Moon (2):
  PCI: tegra: Simplify clock handling by using clk_bulk*() functions
  PCI: tegra: Use readl_poll_timeout() for link status polling

 drivers/pci/controller/pci-tegra.c | 109 +++++++----------------------
 1 file changed, 27 insertions(+), 82 deletions(-)


base-commit: 5c3b3264e5858813632031ba58bcd6e1eeb3b214
-- 
2.50.1

[RFC v1 1/2] PCI: tegra: Simplify clock handling by using clk_bulk*() functions

[Anand Moon]

Currently, the driver acquires clocks and prepare/enable/disable/unprepare
the clocks individually thereby making the driver complex to read.

The driver can be simplified by using the clk_bulk*() APIs.

Use:
  - devm_clk_bulk_get_all() API to acquire all the clocks
  - clk_bulk_prepare_enable() to prepare/enable clocks
  - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk

As part of this cleanup, the legacy has_cml_clk flag and explicit handling
of individual clocks (pex, afi, pll_e, cml) are removed. Clock sequencing
is now implicitly determined by the order defined in the device tree,
eliminating hardcoded logic and improving maintainability.

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pci/controller/pci-tegra.c | 71 +++++-------------------------
 1 file changed, 12 insertions(+), 59 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 467ddc701adce..3841489198b64 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -297,7 +297,6 @@ struct tegra_pcie_soc {
 	bool has_pex_clkreq_en;
 	bool has_pex_bias_ctrl;
 	bool has_intr_prsnt_sense;
-	bool has_cml_clk;
 	bool has_gen2;
 	bool force_pca_enable;
 	bool program_uphy;
@@ -330,10 +329,8 @@ struct tegra_pcie {
 
 	struct resource cs;
 
-	struct clk *pex_clk;
-	struct clk *afi_clk;
-	struct clk *pll_e;
-	struct clk *cml_clk;
+	struct clk_bulk_data *clks;
+	int    num_clks;
 
 	struct reset_control *pex_rst;
 	struct reset_control *afi_rst;
@@ -1153,15 +1150,11 @@ static void tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	const struct tegra_pcie_soc *soc = pcie->soc;
 	int err;
 
 	reset_control_assert(pcie->afi_rst);
 
-	clk_disable_unprepare(pcie->pll_e);
-	if (soc->has_cml_clk)
-		clk_disable_unprepare(pcie->cml_clk);
-	clk_disable_unprepare(pcie->afi_clk);
+	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
 
 	if (!dev->pm_domain)
 		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
@@ -1174,7 +1167,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	const struct tegra_pcie_soc *soc = pcie->soc;
 	int err;
 
 	reset_control_assert(pcie->pcie_xrst);
@@ -1202,35 +1194,16 @@ static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 		}
 	}
 
-	err = clk_prepare_enable(pcie->afi_clk);
+	err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
 	if (err < 0) {
-		dev_err(dev, "failed to enable AFI clock: %d\n", err);
+		dev_err(dev, "filed to enable clocks: %d\n", err);
 		goto powergate;
 	}
 
-	if (soc->has_cml_clk) {
-		err = clk_prepare_enable(pcie->cml_clk);
-		if (err < 0) {
-			dev_err(dev, "failed to enable CML clock: %d\n", err);
-			goto disable_afi_clk;
-		}
-	}
-
-	err = clk_prepare_enable(pcie->pll_e);
-	if (err < 0) {
-		dev_err(dev, "failed to enable PLLE clock: %d\n", err);
-		goto disable_cml_clk;
-	}
-
 	reset_control_deassert(pcie->afi_rst);
 
 	return 0;
 
-disable_cml_clk:
-	if (soc->has_cml_clk)
-		clk_disable_unprepare(pcie->cml_clk);
-disable_afi_clk:
-	clk_disable_unprepare(pcie->afi_clk);
 powergate:
 	if (!dev->pm_domain)
 		tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
@@ -1254,25 +1227,11 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
 static int tegra_pcie_clocks_get(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	const struct tegra_pcie_soc *soc = pcie->soc;
-
-	pcie->pex_clk = devm_clk_get(dev, "pex");
-	if (IS_ERR(pcie->pex_clk))
-		return PTR_ERR(pcie->pex_clk);
 
-	pcie->afi_clk = devm_clk_get(dev, "afi");
-	if (IS_ERR(pcie->afi_clk))
-		return PTR_ERR(pcie->afi_clk);
-
-	pcie->pll_e = devm_clk_get(dev, "pll_e");
-	if (IS_ERR(pcie->pll_e))
-		return PTR_ERR(pcie->pll_e);
-
-	if (soc->has_cml_clk) {
-		pcie->cml_clk = devm_clk_get(dev, "cml");
-		if (IS_ERR(pcie->cml_clk))
-			return PTR_ERR(pcie->cml_clk);
-	}
+	pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks);
+	if (pcie->num_clks < 0)
+		return dev_err_probe(dev, pcie->num_clks,
+				     "failed to get clocks\n");
 
 	return 0;
 }
@@ -2345,7 +2304,6 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.has_pex_clkreq_en = false,
 	.has_pex_bias_ctrl = false,
 	.has_intr_prsnt_sense = false,
-	.has_cml_clk = false,
 	.has_gen2 = false,
 	.force_pca_enable = false,
 	.program_uphy = true,
@@ -2374,7 +2332,6 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
-	.has_cml_clk = true,
 	.has_gen2 = false,
 	.force_pca_enable = false,
 	.program_uphy = true,
@@ -2395,7 +2352,6 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
-	.has_cml_clk = true,
 	.has_gen2 = true,
 	.force_pca_enable = false,
 	.program_uphy = true,
@@ -2418,7 +2374,6 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
-	.has_cml_clk = true,
 	.has_gen2 = true,
 	.force_pca_enable = true,
 	.program_uphy = true,
@@ -2459,7 +2414,6 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
-	.has_cml_clk = false,
 	.has_gen2 = true,
 	.force_pca_enable = false,
 	.program_uphy = false,
@@ -2672,7 +2626,7 @@ static int tegra_pcie_pm_suspend(struct device *dev)
 	}
 
 	reset_control_assert(pcie->pex_rst);
-	clk_disable_unprepare(pcie->pex_clk);
+	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_disable_msi(pcie);
@@ -2706,9 +2660,9 @@ static int tegra_pcie_pm_resume(struct device *dev)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		tegra_pcie_enable_msi(pcie);
 
-	err = clk_prepare_enable(pcie->pex_clk);
+	err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
 	if (err) {
-		dev_err(dev, "failed to enable PEX clock: %d\n", err);
+		dev_err(dev, "failed to enable clock: %d\n", err);
 		goto pex_dpd_enable;
 	}
 
@@ -2729,7 +2683,6 @@ static int tegra_pcie_pm_resume(struct device *dev)
 
 disable_pex_clk:
 	reset_control_assert(pcie->pex_rst);
-	clk_disable_unprepare(pcie->pex_clk);
 pex_dpd_enable:
 	pinctrl_pm_select_idle_state(dev);
 poweroff:
-- 
2.50.1

Re: [RFC v1 1/2] PCI: tegra: Simplify clock handling by using clk_bulk*() functions

[Jon Hunter]

On 31/08/2025 20:00, Anand Moon wrote:
> Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> the clocks individually thereby making the driver complex to read.
> 
> The driver can be simplified by using the clk_bulk*() APIs.
> 
> Use:
>    - devm_clk_bulk_get_all() API to acquire all the clocks
>    - clk_bulk_prepare_enable() to prepare/enable clocks
>    - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
> 
> As part of this cleanup, the legacy has_cml_clk flag and explicit handling
> of individual clocks (pex, afi, pll_e, cml) are removed. Clock sequencing
> is now implicitly determined by the order defined in the device tree,
> eliminating hardcoded logic and improving maintainability.

What platforms have you tested this change on?

Thanks
Jon

-- 
nvpublic

Re: [RFC v1 1/2] PCI: tegra: Simplify clock handling by using clk_bulk*() functions

[Anand Moon]

Hi Jon,

On Wed, 17 Sept 2025 at 19:14, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 31/08/2025 20:00, Anand Moon wrote:
> > Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> > the clocks individually thereby making the driver complex to read.
> >
> > The driver can be simplified by using the clk_bulk*() APIs.
> >
> > Use:
> >    - devm_clk_bulk_get_all() API to acquire all the clocks
> >    - clk_bulk_prepare_enable() to prepare/enable clocks
> >    - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
> >
> > As part of this cleanup, the legacy has_cml_clk flag and explicit handling
> > of individual clocks (pex, afi, pll_e, cml) are removed. Clock sequencing
> > is now implicitly determined by the order defined in the device tree,
> > eliminating hardcoded logic and improving maintainability.
>
> What platforms have you tested this change on?
>
I'm using a Jetson Nano 4GB model as my test platform.
> Thanks
> Jon
Thanks
-Anand

Re: [RFC v1 1/2] PCI: tegra: Simplify clock handling by using clk_bulk*() functions

[Jon Hunter]

On 17/09/2025 19:26, Anand Moon wrote:
> Hi Jon,
> 
> On Wed, 17 Sept 2025 at 19:14, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 31/08/2025 20:00, Anand Moon wrote:
>>> Currently, the driver acquires clocks and prepare/enable/disable/unprepare
>>> the clocks individually thereby making the driver complex to read.
>>>
>>> The driver can be simplified by using the clk_bulk*() APIs.
>>>
>>> Use:
>>>     - devm_clk_bulk_get_all() API to acquire all the clocks
>>>     - clk_bulk_prepare_enable() to prepare/enable clocks
>>>     - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
>>>
>>> As part of this cleanup, the legacy has_cml_clk flag and explicit handling
>>> of individual clocks (pex, afi, pll_e, cml) are removed. Clock sequencing
>>> is now implicitly determined by the order defined in the device tree,
>>> eliminating hardcoded logic and improving maintainability.
>>
>> What platforms have you tested this change on?
>>
> I'm using a Jetson Nano 4GB model as my test platform.

Thanks. One concern I have about this is that right now the DT binding 
doc for this device is still in the legacy text format and not converted 
to yaml. Therefore, there is no way to validate the device-tree bindings 
for this driver. So by making this change we are susceptible to people 
getting the device-tree incorrect and there is no way to check. Right 
now the driver will fail is a given clock is missing but after this 
change we are completely reliant that the device-tree is correct but no 
way to validate.	

It would be great to convert the text binding doc to yaml as part of 
this series.

Also if you look at the dwmac-tegra.c driver this one still populates 
the clock names when using the bulk APIs so that we know that the clocks 
that we require are present.

Jon

[0] drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c

-- 
nvpublic

Re: [RFC v1 1/2] PCI: tegra: Simplify clock handling by using clk_bulk*() functions

[Anand Moon]

Hi Jon,

On Thu, 18 Sept 2025 at 14:47, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 17/09/2025 19:26, Anand Moon wrote:
> > Hi Jon,
> >
> > On Wed, 17 Sept 2025 at 19:14, Jon Hunter <jonathanh@nvidia.com> wrote:
> >>
> >>
> >> On 31/08/2025 20:00, Anand Moon wrote:
> >>> Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> >>> the clocks individually thereby making the driver complex to read.
> >>>
> >>> The driver can be simplified by using the clk_bulk*() APIs.
> >>>
> >>> Use:
> >>>     - devm_clk_bulk_get_all() API to acquire all the clocks
> >>>     - clk_bulk_prepare_enable() to prepare/enable clocks
> >>>     - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
> >>>
> >>> As part of this cleanup, the legacy has_cml_clk flag and explicit handling
> >>> of individual clocks (pex, afi, pll_e, cml) are removed. Clock sequencing
> >>> is now implicitly determined by the order defined in the device tree,
> >>> eliminating hardcoded logic and improving maintainability.
> >>
> >> What platforms have you tested this change on?
> >>
> > I'm using a Jetson Nano 4GB model as my test platform.
>
> Thanks. One concern I have about this is that right now the DT binding
> doc for this device is still in the legacy text format and not converted
> to yaml. Therefore, there is no way to validate the device-tree bindings
> for this driver. So by making this change we are susceptible to people
> getting the device-tree incorrect and there is no way to check. Right
> now the driver will fail is a given clock is missing but after this
> change we are completely reliant that the device-tree is correct but no
> way to validate.
>
> It would be great to convert the text binding doc to yaml as part of
> this series.
>
I will convert the legacy text binding to a YAML file as part of this series.

[0] Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

> Also if you look at the dwmac-tegra.c driver this one still populates
> the clock names when using the bulk APIs so that we know that the clocks
> that we require are present.
>
Only the Tegra20 SoC has three clocks.
    compatible = "nvidia,tegra20-pcie";
    clocks = <&tegra_car TEGRA20_CLK_PEX>,
                         <&tegra_car TEGRA20_CLK_AFI>,
                         <&tegra_car TEGRA20_CLK_PLL_E>;
                clock-names = "pex", "afi", "pll_e";

Whereas all the rest of the SoCs have 4 clocks.

  compatible = "nvidia,tegra30-pcie";
  compatible = "nvidia,tegra124-pcie";
  compatible = "nvidia,tegra210-pcie";
  compatible = "nvidia,tegra186-pcie";

  clocks = <&tegra_car TEGRA30_CLK_PCIE>,
                         <&tegra_car TEGRA30_CLK_AFI>,
                         <&tegra_car TEGRA30_CLK_PLL_E>,
                         <&tegra_car TEGRA30_CLK_CML0>;
                clock-names = "pex", "afi", "pll_e", "cml";

As suggested, I need to create two clock arrays for the clocks of the SoC.

But the code will introduce more overhead:

bulk clks -> devm_kcalloc (for clocks) -> assign id to clocks ->
devm_clk_bulk_get -> clk_bulk_prepare_enable.

I believe the use of devm_clk_bulk_get_all() is a cleaner and more modern
approach for the following reasons:
It simplifies the code by removing the need for manual memory allocation
(devm_kcalloc) and populating an array of clock specifications.
It is more efficient, as all clocks are fetched in a single API call,
reducing overhead.

Please let me know if this plan addresses your concerns.

> Jon
>
> [0] drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
>
> --
> nvpublic
>
Thanks
-Anand

Re: [RFC v1 1/2] PCI: tegra: Simplify clock handling by using clk_bulk*() functions

[Jon Hunter]

On 18/09/2025 16:06, Anand Moon wrote:
> Hi Jon,
> 
> On Thu, 18 Sept 2025 at 14:47, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 17/09/2025 19:26, Anand Moon wrote:
>>> Hi Jon,
>>>
>>> On Wed, 17 Sept 2025 at 19:14, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>
>>>>
>>>> On 31/08/2025 20:00, Anand Moon wrote:
>>>>> Currently, the driver acquires clocks and prepare/enable/disable/unprepare
>>>>> the clocks individually thereby making the driver complex to read.
>>>>>
>>>>> The driver can be simplified by using the clk_bulk*() APIs.
>>>>>
>>>>> Use:
>>>>>      - devm_clk_bulk_get_all() API to acquire all the clocks
>>>>>      - clk_bulk_prepare_enable() to prepare/enable clocks
>>>>>      - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
>>>>>
>>>>> As part of this cleanup, the legacy has_cml_clk flag and explicit handling
>>>>> of individual clocks (pex, afi, pll_e, cml) are removed. Clock sequencing
>>>>> is now implicitly determined by the order defined in the device tree,
>>>>> eliminating hardcoded logic and improving maintainability.
>>>>
>>>> What platforms have you tested this change on?
>>>>
>>> I'm using a Jetson Nano 4GB model as my test platform.
>>
>> Thanks. One concern I have about this is that right now the DT binding
>> doc for this device is still in the legacy text format and not converted
>> to yaml. Therefore, there is no way to validate the device-tree bindings
>> for this driver. So by making this change we are susceptible to people
>> getting the device-tree incorrect and there is no way to check. Right
>> now the driver will fail is a given clock is missing but after this
>> change we are completely reliant that the device-tree is correct but no
>> way to validate.
>>
>> It would be great to convert the text binding doc to yaml as part of
>> this series.
>>
> I will convert the legacy text binding to a YAML file as part of this series.
> 
> [0] Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

Thanks!

>> Also if you look at the dwmac-tegra.c driver this one still populates
>> the clock names when using the bulk APIs so that we know that the clocks
>> that we require are present.
>>
> Only the Tegra20 SoC has three clocks.
>      compatible = "nvidia,tegra20-pcie";
>      clocks = <&tegra_car TEGRA20_CLK_PEX>,
>                           <&tegra_car TEGRA20_CLK_AFI>,
>                           <&tegra_car TEGRA20_CLK_PLL_E>;
>                  clock-names = "pex", "afi", "pll_e";
> 
> Whereas all the rest of the SoCs have 4 clocks.
> 
>    compatible = "nvidia,tegra30-pcie";
>    compatible = "nvidia,tegra124-pcie";
>    compatible = "nvidia,tegra210-pcie";
>    compatible = "nvidia,tegra186-pcie";
> 
>    clocks = <&tegra_car TEGRA30_CLK_PCIE>,
>                           <&tegra_car TEGRA30_CLK_AFI>,
>                           <&tegra_car TEGRA30_CLK_PLL_E>,
>                           <&tegra_car TEGRA30_CLK_CML0>;
>                  clock-names = "pex", "afi", "pll_e", "cml";
> 
> As suggested, I need to create two clock arrays for the clocks of the SoC.
> 
> But the code will introduce more overhead:
> 
> bulk clks -> devm_kcalloc (for clocks) -> assign id to clocks ->
> devm_clk_bulk_get -> clk_bulk_prepare_enable.
> 
> I believe the use of devm_clk_bulk_get_all() is a cleaner and more modern
> approach for the following reasons:
> It simplifies the code by removing the need for manual memory allocation
> (devm_kcalloc) and populating an array of clock specifications.
> It is more efficient, as all clocks are fetched in a single API call,
> reducing overhead.

Yes that's correct and I don't think it is that much overhead. The clk 
names array can just be part of the 'tegra_pcie_soc' structure.

Jon

-- 
nvpublic

[RFC v1 2/2] PCI: tegra: Use readl_poll_timeout() for link status polling

[Anand Moon]

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.1

Re: [RFC v1 2/2] PCI: tegra: Use readl_poll_timeout() for link status polling

[Mikko Perttunen]

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.

>  #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;
>  }
> 

Re: [RFC v1 2/2] PCI: tegra: Use readl_poll_timeout() for link status polling

[Anand Moon]

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

Re: [RFC v1 2/2] PCI: tegra: Use readl_poll_timeout() for link status polling

[Mikko Perttunen]

On Wednesday, September 17, 2025 4:45 PM Anand Moon wrote:
> 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?

I think the code is fine as is. Before, the shortest the timeout could be was 200ms, i.e. there should be no situation where we need a timeout longer than that, or otherwise that would fail randomly depending on the sleep duration. So I think the 200ms is correct here and the only change necessary is the removal of the second iopoll.h

Cheers,
Mikko

> 
> Thank
> -Anand

Re: [RFC v1 2/2] PCI: tegra: Use readl_poll_timeout() for link status polling

[Anand Moon]

Hi Mikko,

On Thu, 18 Sept 2025 at 06:56, Mikko Perttunen <mperttunen@nvidia.com> wrote:
>
> On Wednesday, September 17, 2025 4:45 PM Anand Moon wrote:
> > 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?
>
> I think the code is fine as is. Before, the shortest the timeout could be was 200ms, i.e. there should be no situation where we need a timeout longer than that, or otherwise that would fail randomly depending on the sleep duration. So I think the 200ms is correct here and the only change necessary is the removal of the second iopoll.h
>
Thanks for your input. I'll remove the header in the next version.
> Cheers,
> Mikko
>
Thanks
-Anand