linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
@ 2020-11-09 17:19 Vidya Sagar
  2020-11-09 17:19 ` [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code Vidya Sagar
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Vidya Sagar @ 2020-11-09 17:19 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh+dt, bhelgaas, thierry.reding, jonathanh,
	amanharitsh123, dinghao.liu, kw
  Cc: linux-pci, linux-tegra, linux-kernel, kthota, mmaddireddy,
	vidyas, sagar.tv

This series of patches do some enhancements and some bug fixes to the
Tegra194 PCIe platform driver like
- Fix Vendor-ID corruption
- Map DBI space correctly
- Update DWC IP version
- Continue with uninitialization sequence even if parts fail
- Check return value of tegra_pcie_init_controller()

V4:
* Added a new patch to address link-up issues with some of the cards

V3:
* Addressed Bjorn's review comments
* Split earlier patch-4 into two
  - Continue with the uninitialization sequence even if some parts fail
  - Check return value of tegra_pcie_init_controller() and exit accordingly

V2:
* Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE'

Vidya Sagar (6):
  PCI: tegra: Fix ASPM-L1SS advertisement disable code
  PCI: tegra: Map configuration space as nGnRnE
  PCI: tegra: Set DesignWare IP version
  PCI: tegra: Continue unconfig sequence even if parts fail
  PCI: tegra: Check return value of tegra_pcie_init_controller()
  PCI: tegra: Disable LTSSM during L2 entry

 drivers/pci/controller/dwc/pcie-tegra194.c | 78 +++++++++++-----------
 1 file changed, 39 insertions(+), 39 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code
  2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
@ 2020-11-09 17:19 ` Vidya Sagar
  2020-11-26 11:33   ` Thierry Reding
  2020-11-09 17:19 ` [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE Vidya Sagar
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-11-09 17:19 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh+dt, bhelgaas, thierry.reding, jonathanh,
	amanharitsh123, dinghao.liu, kw
  Cc: linux-pci, linux-tegra, linux-kernel, kthota, mmaddireddy,
	vidyas, sagar.tv

If the absence of CLKREQ# signal is indicated by the absence of
"supports-clkreq" in the device-tree node, current driver is disabling
the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1 Sub-States
offset is correctly initialized. Since default value of the ASPM-L1SS
offset is zero, this is causing the Vendor-ID wrongly programmed to 0x10d2
instead of Nvidia's 0x10de thereby the quirks applicable for Tegra194 are
not being applied. This patch fixes this issue by refactoring the
code that disables the ASPM-L1SS advertisement.

Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* None

V3:
* None

V2:
* None

 drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index aa511ec0d800..b172b1d49713 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -896,6 +896,12 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
 
 	init_host_aspm(pcie);
 
+	/* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+	if (!pcie->supports_clkreq) {
+		disable_aspm_l11(pcie);
+		disable_aspm_l12(pcie);
+	}
+
 	val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
 	val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
 	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
@@ -1400,12 +1406,6 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
 	pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci,
 						      PCI_CAP_ID_EXP);
 
-	/* Disable ASPM-L1SS advertisement as there is no CLKREQ routing */
-	if (!pcie->supports_clkreq) {
-		disable_aspm_l11(pcie);
-		disable_aspm_l12(pcie);
-	}
-
 	return ret;
 
 fail_phy:
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE
  2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
  2020-11-09 17:19 ` [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code Vidya Sagar
@ 2020-11-09 17:19 ` Vidya Sagar
  2020-11-26 11:33   ` Thierry Reding
  2020-11-09 17:19 ` [PATCH V4 3/6] PCI: tegra: Set DesignWare IP version Vidya Sagar
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-11-09 17:19 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh+dt, bhelgaas, thierry.reding, jonathanh,
	amanharitsh123, dinghao.liu, kw
  Cc: linux-pci, linux-tegra, linux-kernel, kthota, mmaddireddy,
	vidyas, sagar.tv

As specified in the comment for pci_remap_cfgspace() define in
arch/arm64/include/asm/io.h file, PCIe configuration space should be
mapped as nGnRnE. Hence changing to dev_pci_remap_cfgspace() from
devm_ioremap_resource() for mapping DBI space as that is nothing but
the root port's own configuration space.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* None

V3:
* None

V2:
* Changed 'Strongly Ordered' to 'nGnRnE'

 drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index b172b1d49713..7a0c64436861 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 	}
 	pcie->dbi_res = dbi_res;
 
-	pci->dbi_base = devm_ioremap_resource(dev, dbi_res);
+	pci->dbi_base = devm_pci_remap_cfgspace(dev,
+						dbi_res->start,
+						resource_size(dbi_res));
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH V4 3/6] PCI: tegra: Set DesignWare IP version
  2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
  2020-11-09 17:19 ` [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code Vidya Sagar
  2020-11-09 17:19 ` [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE Vidya Sagar
@ 2020-11-09 17:19 ` Vidya Sagar
  2020-11-26 11:34   ` Thierry Reding
  2020-11-09 17:19 ` [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail Vidya Sagar
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-11-09 17:19 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh+dt, bhelgaas, thierry.reding, jonathanh,
	amanharitsh123, dinghao.liu, kw
  Cc: linux-pci, linux-tegra, linux-kernel, kthota, mmaddireddy,
	vidyas, sagar.tv

Set the DesignWare IP version for Tegra194 to 0x490A. This would be used
by the DesigWare sub-system to do any version specific configuration
(Ex:- TD bit programming for ECRC).

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* None

V3:
* None

V2:
* None

 drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 7a0c64436861..253d91033bc3 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -2011,6 +2011,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 	pci->ops = &tegra_dw_pcie_ops;
 	pci->n_fts[0] = N_FTS_VAL;
 	pci->n_fts[1] = FTS_VAL;
+	pci->version = 0x490A;
 
 	pp = &pci->pp;
 	pcie->dev = &pdev->dev;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail
  2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
                   ` (2 preceding siblings ...)
  2020-11-09 17:19 ` [PATCH V4 3/6] PCI: tegra: Set DesignWare IP version Vidya Sagar
@ 2020-11-09 17:19 ` Vidya Sagar
  2020-11-26 11:34   ` Thierry Reding
  2020-11-30 12:10   ` Lorenzo Pieralisi
  2020-11-09 17:19 ` [PATCH V4 5/6] PCI: tegra: Check return value of tegra_pcie_init_controller() Vidya Sagar
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 21+ messages in thread
From: Vidya Sagar @ 2020-11-09 17:19 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh+dt, bhelgaas, thierry.reding, jonathanh,
	amanharitsh123, dinghao.liu, kw
  Cc: linux-pci, linux-tegra, linux-kernel, kthota, mmaddireddy,
	vidyas, sagar.tv

Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* None

V3:
* Modified subject as per Bjorn's suggestion
* Removed tegra_pcie_init_controller()'s error checking part and pushed
  a separate patch for it

V2:
* None

 drivers/pci/controller/dwc/pcie-tegra194.c | 39 +++++++++-------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 253d91033bc3..9be10c8953df 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
 	return ret;
 }
 
-static int __deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
 {
 	int ret;
 
 	ret = reset_control_assert(pcie->core_rst);
-	if (ret) {
-		dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
-			ret);
-		return ret;
-	}
+	if (ret)
+		dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n", ret);
 
 	tegra_pcie_disable_phy(pcie);
 
 	ret = reset_control_assert(pcie->core_apb_rst);
-	if (ret) {
+	if (ret)
 		dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
-		return ret;
-	}
 
 	clk_disable_unprepare(pcie->core_clk);
 
 	ret = regulator_disable(pcie->pex_ctl_supply);
-	if (ret) {
+	if (ret)
 		dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
-		return ret;
-	}
 
 	tegra_pcie_disable_slot_regulators(pcie);
 
 	ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
-	if (ret) {
+	if (ret)
 		dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
 			pcie->cid, ret);
-		return ret;
-	}
-
-	return ret;
 }
 
 static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
@@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
 	return 0;
 
 fail_host_init:
-	return __deinit_controller(pcie);
+	tegra_pcie_unconfig_controller(pcie);
+	return ret;
 }
 
 static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
@@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct tegra_pcie_dw *pcie)
 	appl_writel(pcie, data, APPL_PINMUX);
 }
 
-static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
 {
 	tegra_pcie_downstream_dev_to_D0(pcie);
 	dw_pcie_host_deinit(&pcie->pci.pp);
 	tegra_pcie_dw_pme_turnoff(pcie);
-
-	return __deinit_controller(pcie);
+	tegra_pcie_unconfig_controller(pcie);
 }
 
 static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
@@ -2238,8 +2227,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
 					       PORT_LOGIC_MSI_CTRL_INT_0_EN);
 	tegra_pcie_downstream_dev_to_D0(pcie);
 	tegra_pcie_dw_pme_turnoff(pcie);
+	tegra_pcie_unconfig_controller(pcie);
 
-	return __deinit_controller(pcie);
+	return 0;
 }
 
 static int tegra_pcie_dw_resume_noirq(struct device *dev)
@@ -2267,7 +2257,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
 	return 0;
 
 fail_host_init:
-	return __deinit_controller(pcie);
+	tegra_pcie_unconfig_controller(pcie);
+	return ret;
 }
 
 static int tegra_pcie_dw_resume_early(struct device *dev)
@@ -2305,7 +2296,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev)
 		disable_irq(pcie->pci.pp.msi_irq);
 
 	tegra_pcie_dw_pme_turnoff(pcie);
-	__deinit_controller(pcie);
+	tegra_pcie_unconfig_controller(pcie);
 }
 
 static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH V4 5/6] PCI: tegra: Check return value of tegra_pcie_init_controller()
  2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
                   ` (3 preceding siblings ...)
  2020-11-09 17:19 ` [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail Vidya Sagar
@ 2020-11-09 17:19 ` Vidya Sagar
  2020-11-26 11:34   ` Thierry Reding
  2020-11-09 17:19 ` [PATCH V4 6/6] PCI: tegra: Disable LTSSM during L2 entry Vidya Sagar
  2020-11-25 17:57 ` [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Thierry Reding
  6 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-11-09 17:19 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh+dt, bhelgaas, thierry.reding, jonathanh,
	amanharitsh123, dinghao.liu, kw
  Cc: linux-pci, linux-tegra, linux-kernel, kthota, mmaddireddy,
	vidyas, sagar.tv

The return value of tegra_pcie_init_controller() must be checked before
PCIe link up check and registering debugfs entries subsequently as it
doesn't make sense to do these when the controller initialization itself
has failed.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* None

V3:
* New patch in this series

 drivers/pci/controller/dwc/pcie-tegra194.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 9be10c8953df..8c08998b9ce1 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1579,7 +1579,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
 		goto fail_pm_get_sync;
 	}
 
-	tegra_pcie_init_controller(pcie);
+	ret = tegra_pcie_init_controller(pcie);
+	if (ret < 0) {
+		dev_err(dev, "Failed to initialize controller: %d\n", ret);
+		goto fail_pm_get_sync;
+	}
 
 	pcie->link_state = tegra_pcie_dw_link_up(&pcie->pci);
 	if (!pcie->link_state) {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH V4 6/6] PCI: tegra: Disable LTSSM during L2 entry
  2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
                   ` (4 preceding siblings ...)
  2020-11-09 17:19 ` [PATCH V4 5/6] PCI: tegra: Check return value of tegra_pcie_init_controller() Vidya Sagar
@ 2020-11-09 17:19 ` Vidya Sagar
  2020-11-26 11:34   ` Thierry Reding
  2020-11-25 17:57 ` [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Thierry Reding
  6 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-11-09 17:19 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh+dt, bhelgaas, thierry.reding, jonathanh,
	amanharitsh123, dinghao.liu, kw
  Cc: linux-pci, linux-tegra, linux-kernel, kthota, mmaddireddy,
	vidyas, sagar.tv

PCIe cards like Marvell SATA controller and some of the Samsung NVMe
drives don't support taking the link to L2 state. When the link doesn't
go to L2 state, Tegra194 requires the LTSSM to be disabled to allow PHY
to start the next link up process cleanly during suspend/resume sequence.
Failing to disable LTSSM results in the PCIe link not coming up in the
next resume cycle.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* New patch in this series

 drivers/pci/controller/dwc/pcie-tegra194.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 8c08998b9ce1..57ff0657bbe2 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1513,6 +1513,14 @@ static void tegra_pcie_dw_pme_turnoff(struct tegra_pcie_dw *pcie)
 		data &= ~APPL_PINMUX_PEX_RST;
 		appl_writel(pcie, data, APPL_PINMUX);
 
+		/*
+		 * Some cards do not go to detect state even after de-asserting
+		 * PERST#. So, de-assert LTSSM to bring link to detect state.
+		 */
+		data = readl(pcie->appl_base + APPL_CTRL);
+		data &= ~APPL_CTRL_LTSSM_EN;
+		writel(data, pcie->appl_base + APPL_CTRL);
+
 		err = readl_poll_timeout_atomic(pcie->appl_base + APPL_DEBUG,
 						data,
 						((data &
@@ -1520,14 +1528,8 @@ static void tegra_pcie_dw_pme_turnoff(struct tegra_pcie_dw *pcie)
 						APPL_DEBUG_LTSSM_STATE_SHIFT) ==
 						LTSSM_STATE_PRE_DETECT,
 						1, LTSSM_TIMEOUT);
-		if (err) {
+		if (err)
 			dev_info(pcie->dev, "Link didn't go to detect state\n");
-		} else {
-			/* Disable LTSSM after link is in detect state */
-			data = appl_readl(pcie, APPL_CTRL);
-			data &= ~APPL_CTRL_LTSSM_EN;
-			appl_writel(pcie, data, APPL_CTRL);
-		}
 	}
 	/*
 	 * DBI registers may not be accessible after this as PLL-E would be
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
  2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
                   ` (5 preceding siblings ...)
  2020-11-09 17:19 ` [PATCH V4 6/6] PCI: tegra: Disable LTSSM during L2 entry Vidya Sagar
@ 2020-11-25 17:57 ` Thierry Reding
  2020-11-25 19:51   ` Vidya Sagar
  6 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-11-25 17:57 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

On Mon, Nov 09, 2020 at 10:49:31PM +0530, Vidya Sagar wrote:
> This series of patches do some enhancements and some bug fixes to the
> Tegra194 PCIe platform driver like
> - Fix Vendor-ID corruption
> - Map DBI space correctly
> - Update DWC IP version
> - Continue with uninitialization sequence even if parts fail
> - Check return value of tegra_pcie_init_controller()
> 
> V4:
> * Added a new patch to address link-up issues with some of the cards
> 
> V3:
> * Addressed Bjorn's review comments
> * Split earlier patch-4 into two
>   - Continue with the uninitialization sequence even if some parts fail
>   - Check return value of tegra_pcie_init_controller() and exit accordingly
> 
> V2:
> * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE'
> 
> Vidya Sagar (6):
>   PCI: tegra: Fix ASPM-L1SS advertisement disable code
>   PCI: tegra: Map configuration space as nGnRnE
>   PCI: tegra: Set DesignWare IP version
>   PCI: tegra: Continue unconfig sequence even if parts fail
>   PCI: tegra: Check return value of tegra_pcie_init_controller()
>   PCI: tegra: Disable LTSSM during L2 entry
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 78 +++++++++++-----------
>  1 file changed, 39 insertions(+), 39 deletions(-)

I was going to test this series, but then I noticed that PCI is causing
a crash on linux-next (as of fairly recently). So I tried applying this
on top of v5.10-rc1, but that gives me the following:

	[    3.595161] ahci 0001:01:00.0: version 3.0
	[    3.595726] ahci 0001:01:00.0: SSS flag set, parallel bus scan disabled
	[    4.609923] ahci 0001:01:00.0: controller reset failed (0xffffffff)
	[    4.610343] ahci: probe of 0001:01:00.0 failed with error -5

So the device enumerates fine, but it's not able to reset the SATA
controller. That said, this seems to happen regardless of this patch
series, so plain v5.10-rc1 also shows the above.

Given the above, I think we should hold off on applying this series
until we've fixed PCI on linux-next and made sure that SATA also works
properly, otherwise we don't have a known good baseline to test this
against.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
  2020-11-25 17:57 ` [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Thierry Reding
@ 2020-11-25 19:51   ` Vidya Sagar
  2020-11-26 11:31     ` Thierry Reding
  0 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-11-25 19:51 UTC (permalink / raw)
  To: Thierry Reding
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, Jonathan Hunter,
	amanharitsh123, dinghao.liu, kw, linux-pci, linux-tegra,
	linux-kernel, Krishna Thota, Manikanta Maddireddy, sagar.tv



> -----Original Message-----
> From: Thierry Reding <thierry.reding@gmail.com>
> Sent: Wednesday, November 25, 2020 11:27 PM
> To: Vidya Sagar <vidyas@nvidia.com>
> Cc: lorenzo.pieralisi@arm.com; robh+dt@kernel.org; bhelgaas@google.com;
> Jonathan Hunter <jonathanh@nvidia.com>; amanharitsh123@gmail.com;
> dinghao.liu@zju.edu.cn; kw@linux.com; linux-pci@vger.kernel.org; linux-
> tegra@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Thota
> <kthota@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>;
> sagar.tv@gmail.com
> Subject: Re: [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
> 
> On Mon, Nov 09, 2020 at 10:49:31PM +0530, Vidya Sagar wrote:
> > This series of patches do some enhancements and some bug fixes to the
> > Tegra194 PCIe platform driver like
> > - Fix Vendor-ID corruption
> > - Map DBI space correctly
> > - Update DWC IP version
> > - Continue with uninitialization sequence even if parts fail
> > - Check return value of tegra_pcie_init_controller()
> >
> > V4:
> > * Added a new patch to address link-up issues with some of the cards
> >
> > V3:
> > * Addressed Bjorn's review comments
> > * Split earlier patch-4 into two
> >   - Continue with the uninitialization sequence even if some parts fail
> >   - Check return value of tegra_pcie_init_controller() and exit
> > accordingly
> >
> > V2:
> > * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE'
> >
> > Vidya Sagar (6):
> >   PCI: tegra: Fix ASPM-L1SS advertisement disable code
> >   PCI: tegra: Map configuration space as nGnRnE
> >   PCI: tegra: Set DesignWare IP version
> >   PCI: tegra: Continue unconfig sequence even if parts fail
> >   PCI: tegra: Check return value of tegra_pcie_init_controller()
> >   PCI: tegra: Disable LTSSM during L2 entry
> >
> >  drivers/pci/controller/dwc/pcie-tegra194.c | 78
> > +++++++++++-----------
> >  1 file changed, 39 insertions(+), 39 deletions(-)
> 
> I was going to test this series, but then I noticed that PCI is causing a crash on
> linux-next (as of fairly recently).
I root caused the crash issue to the following commit
a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource 
setup into common code")

I also pushed the following two patches to fix this issue for review

http://patchwork.ozlabs.org/project/linux-pci/patch/20201125192234.2270-1-vidyas@nvidia.com/
http://patchwork.ozlabs.org/project/linux-pci/patch/20201125192554.5401-1-vidyas@nvidia.com/


> So I tried applying this on top of v5.10-rc1, but
> that gives me the following:
> 
> 	[    3.595161] ahci 0001:01:00.0: version 3.0
> 	[    3.595726] ahci 0001:01:00.0: SSS flag set, parallel bus scan disabled
> 	[    4.609923] ahci 0001:01:00.0: controller reset failed (0xffffffff)
> 	[    4.610343] ahci: probe of 0001:01:00.0 failed with error -5
> 
> So the device enumerates fine, but it's not able to reset the SATA controller.
> That said, this seems to happen regardless of this patch series, so plain v5.10-rc1
> also shows the above.
This was also a known issue and we need the following commit to make 
things work (FWIW, it is already accepted)
9fff3256f93d PCI: dwc: Restore ATU memory resource setup to use last entry

Otherwise, v5.10-rc3 can be used which has working state of PCIe on 
Tegra194.

> 
> Given the above, I think we should hold off on applying this series until we've
> fixed PCI on linux-next and made sure that SATA also works properly, otherwise
> we don't have a known good baseline to test this against.
> 
> Thierry

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
  2020-11-25 19:51   ` Vidya Sagar
@ 2020-11-26 11:31     ` Thierry Reding
  0 siblings, 0 replies; 21+ messages in thread
From: Thierry Reding @ 2020-11-26 11:31 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, Jonathan Hunter,
	amanharitsh123, dinghao.liu, kw, linux-pci, linux-tegra,
	linux-kernel, Krishna Thota, Manikanta Maddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 3680 bytes --]

On Thu, Nov 26, 2020 at 01:21:44AM +0530, Vidya Sagar wrote:
> 
> 
> > -----Original Message-----
> > From: Thierry Reding <thierry.reding@gmail.com>
> > Sent: Wednesday, November 25, 2020 11:27 PM
> > To: Vidya Sagar <vidyas@nvidia.com>
> > Cc: lorenzo.pieralisi@arm.com; robh+dt@kernel.org; bhelgaas@google.com;
> > Jonathan Hunter <jonathanh@nvidia.com>; amanharitsh123@gmail.com;
> > dinghao.liu@zju.edu.cn; kw@linux.com; linux-pci@vger.kernel.org; linux-
> > tegra@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Thota
> > <kthota@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>;
> > sagar.tv@gmail.com
> > Subject: Re: [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
> > 
> > On Mon, Nov 09, 2020 at 10:49:31PM +0530, Vidya Sagar wrote:
> > > This series of patches do some enhancements and some bug fixes to the
> > > Tegra194 PCIe platform driver like
> > > - Fix Vendor-ID corruption
> > > - Map DBI space correctly
> > > - Update DWC IP version
> > > - Continue with uninitialization sequence even if parts fail
> > > - Check return value of tegra_pcie_init_controller()
> > >
> > > V4:
> > > * Added a new patch to address link-up issues with some of the cards
> > >
> > > V3:
> > > * Addressed Bjorn's review comments
> > > * Split earlier patch-4 into two
> > >   - Continue with the uninitialization sequence even if some parts fail
> > >   - Check return value of tegra_pcie_init_controller() and exit
> > > accordingly
> > >
> > > V2:
> > > * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE'
> > >
> > > Vidya Sagar (6):
> > >   PCI: tegra: Fix ASPM-L1SS advertisement disable code
> > >   PCI: tegra: Map configuration space as nGnRnE
> > >   PCI: tegra: Set DesignWare IP version
> > >   PCI: tegra: Continue unconfig sequence even if parts fail
> > >   PCI: tegra: Check return value of tegra_pcie_init_controller()
> > >   PCI: tegra: Disable LTSSM during L2 entry
> > >
> > >  drivers/pci/controller/dwc/pcie-tegra194.c | 78
> > > +++++++++++-----------
> > >  1 file changed, 39 insertions(+), 39 deletions(-)
> > 
> > I was going to test this series, but then I noticed that PCI is causing a crash on
> > linux-next (as of fairly recently).
> I root caused the crash issue to the following commit
> a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup
> into common code")
> 
> I also pushed the following two patches to fix this issue for review
> 
> http://patchwork.ozlabs.org/project/linux-pci/patch/20201125192234.2270-1-vidyas@nvidia.com/
> http://patchwork.ozlabs.org/project/linux-pci/patch/20201125192554.5401-1-vidyas@nvidia.com/

Great, those fix Tegra194 PCIe on next-20201126 for me!

> > So I tried applying this on top of v5.10-rc1, but
> > that gives me the following:
> > 
> > 	[    3.595161] ahci 0001:01:00.0: version 3.0
> > 	[    3.595726] ahci 0001:01:00.0: SSS flag set, parallel bus scan disabled
> > 	[    4.609923] ahci 0001:01:00.0: controller reset failed (0xffffffff)
> > 	[    4.610343] ahci: probe of 0001:01:00.0 failed with error -5
> > 
> > So the device enumerates fine, but it's not able to reset the SATA controller.
> > That said, this seems to happen regardless of this patch series, so plain v5.10-rc1
> > also shows the above.
> This was also a known issue and we need the following commit to make things
> work (FWIW, it is already accepted)
> 9fff3256f93d PCI: dwc: Restore ATU memory resource setup to use last entry
> 
> Otherwise, v5.10-rc3 can be used which has working state of PCIe on
> Tegra194.

Okay, I see that now. Thanks for following up on those.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code
  2020-11-09 17:19 ` [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code Vidya Sagar
@ 2020-11-26 11:33   ` Thierry Reding
  2020-12-03 12:36     ` Vidya Sagar
  0 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-11-26 11:33 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

On Mon, Nov 09, 2020 at 10:49:32PM +0530, Vidya Sagar wrote:
> If the absence of CLKREQ# signal is indicated by the absence of
> "supports-clkreq" in the device-tree node, current driver is disabling
> the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1 Sub-States
> offset is correctly initialized. Since default value of the ASPM-L1SS
> offset is zero, this is causing the Vendor-ID wrongly programmed to 0x10d2
> instead of Nvidia's 0x10de thereby the quirks applicable for Tegra194 are
> not being applied. This patch fixes this issue by refactoring the
> code that disables the ASPM-L1SS advertisement.
> 
> Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * None
> 
> V2:
> * None
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Looks like this no longer applies cleanly after that other fix that you
sent earlier. But looking more closely, that's because that other fix
already incorporates an equivalent change, so I think this can be
dropped from this series.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE
  2020-11-09 17:19 ` [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE Vidya Sagar
@ 2020-11-26 11:33   ` Thierry Reding
  2020-12-03 12:56     ` Vidya Sagar
  0 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-11-26 11:33 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

On Mon, Nov 09, 2020 at 10:49:33PM +0530, Vidya Sagar wrote:
> As specified in the comment for pci_remap_cfgspace() define in
> arch/arm64/include/asm/io.h file, PCIe configuration space should be
> mapped as nGnRnE. Hence changing to dev_pci_remap_cfgspace() from
> devm_ioremap_resource() for mapping DBI space as that is nothing but
> the root port's own configuration space.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * None
> 
> V2:
> * Changed 'Strongly Ordered' to 'nGnRnE'
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index b172b1d49713..7a0c64436861 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
>  	}
>  	pcie->dbi_res = dbi_res;
>  
> -	pci->dbi_base = devm_ioremap_resource(dev, dbi_res);
> +	pci->dbi_base = devm_pci_remap_cfgspace(dev,
> +						dbi_res->start,
> +						resource_size(dbi_res));
>  	if (IS_ERR(pci->dbi_base))
>  		return PTR_ERR(pci->dbi_base);
>  

Similarly to patch 1/6, this is no longer required because it's already
part of one of Rob's earlier patches, so this, too, can be dropped.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 3/6] PCI: tegra: Set DesignWare IP version
  2020-11-09 17:19 ` [PATCH V4 3/6] PCI: tegra: Set DesignWare IP version Vidya Sagar
@ 2020-11-26 11:34   ` Thierry Reding
  0 siblings, 0 replies; 21+ messages in thread
From: Thierry Reding @ 2020-11-26 11:34 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

On Mon, Nov 09, 2020 at 10:49:34PM +0530, Vidya Sagar wrote:
> Set the DesignWare IP version for Tegra194 to 0x490A. This would be used
> by the DesigWare sub-system to do any version specific configuration
> (Ex:- TD bit programming for ECRC).
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * None
> 
> V2:
> * None
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
>  1 file changed, 1 insertion(+)

Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail
  2020-11-09 17:19 ` [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail Vidya Sagar
@ 2020-11-26 11:34   ` Thierry Reding
  2020-11-30 12:10   ` Lorenzo Pieralisi
  1 sibling, 0 replies; 21+ messages in thread
From: Thierry Reding @ 2020-11-26 11:34 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> Currently the driver checks for error value of different APIs during the
> uninitialization sequence. It just returns from there if there is any error
> observed for one of those calls. Comparatively it is better to continue the
> uninitialization sequence irrespective of whether some of them are
> returning error. That way, it is more closer to complete uninitialization.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * Modified subject as per Bjorn's suggestion
> * Removed tegra_pcie_init_controller()'s error checking part and pushed
>   a separate patch for it
> 
> V2:
> * None
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 39 +++++++++-------------
>  1 file changed, 15 insertions(+), 24 deletions(-)

Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 5/6] PCI: tegra: Check return value of tegra_pcie_init_controller()
  2020-11-09 17:19 ` [PATCH V4 5/6] PCI: tegra: Check return value of tegra_pcie_init_controller() Vidya Sagar
@ 2020-11-26 11:34   ` Thierry Reding
  0 siblings, 0 replies; 21+ messages in thread
From: Thierry Reding @ 2020-11-26 11:34 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Mon, Nov 09, 2020 at 10:49:36PM +0530, Vidya Sagar wrote:
> The return value of tegra_pcie_init_controller() must be checked before
> PCIe link up check and registering debugfs entries subsequently as it
> doesn't make sense to do these when the controller initialization itself
> has failed.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * New patch in this series
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 6/6] PCI: tegra: Disable LTSSM during L2 entry
  2020-11-09 17:19 ` [PATCH V4 6/6] PCI: tegra: Disable LTSSM during L2 entry Vidya Sagar
@ 2020-11-26 11:34   ` Thierry Reding
  0 siblings, 0 replies; 21+ messages in thread
From: Thierry Reding @ 2020-11-26 11:34 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Mon, Nov 09, 2020 at 10:49:37PM +0530, Vidya Sagar wrote:
> PCIe cards like Marvell SATA controller and some of the Samsung NVMe
> drives don't support taking the link to L2 state. When the link doesn't
> go to L2 state, Tegra194 requires the LTSSM to be disabled to allow PHY
> to start the next link up process cleanly during suspend/resume sequence.
> Failing to disable LTSSM results in the PCIe link not coming up in the
> next resume cycle.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * New patch in this series
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail
  2020-11-09 17:19 ` [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail Vidya Sagar
  2020-11-26 11:34   ` Thierry Reding
@ 2020-11-30 12:10   ` Lorenzo Pieralisi
  2020-12-01 14:24     ` Thierry Reding
  1 sibling, 1 reply; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-11-30 12:10 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: robh+dt, bhelgaas, thierry.reding, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> Currently the driver checks for error value of different APIs during the
> uninitialization sequence. It just returns from there if there is any error
> observed for one of those calls. Comparatively it is better to continue the
> uninitialization sequence irrespective of whether some of them are
> returning error. That way, it is more closer to complete uninitialization.

Hi Vidya, Thierry,

I can apply this series (dropping patches as suggested by Thierry),
before though I wanted to ask you if this patch is really an
improvement, it is hard to understand why skipping some error
codes is OK for device correct operations to continue, maybe it
is worth describing why some of those failures aren't really
fatal.

Please let me know, thanks.

Lorenzo

> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * Modified subject as per Bjorn's suggestion
> * Removed tegra_pcie_init_controller()'s error checking part and pushed
>   a separate patch for it
> 
> V2:
> * None
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 39 +++++++++-------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 253d91033bc3..9be10c8953df 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>  	return ret;
>  }
>  
> -static int __deinit_controller(struct tegra_pcie_dw *pcie)
> +static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
>  {
>  	int ret;
>  
>  	ret = reset_control_assert(pcie->core_rst);
> -	if (ret) {
> -		dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
> -			ret);
> -		return ret;
> -	}
> +	if (ret)
> +		dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n", ret);
>  
>  	tegra_pcie_disable_phy(pcie);
>  
>  	ret = reset_control_assert(pcie->core_apb_rst);
> -	if (ret) {
> +	if (ret)
>  		dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
> -		return ret;
> -	}
>  
>  	clk_disable_unprepare(pcie->core_clk);
>  
>  	ret = regulator_disable(pcie->pex_ctl_supply);
> -	if (ret) {
> +	if (ret)
>  		dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
> -		return ret;
> -	}
>  
>  	tegra_pcie_disable_slot_regulators(pcie);
>  
>  	ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
> -	if (ret) {
> +	if (ret)
>  		dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
>  			pcie->cid, ret);
> -		return ret;
> -	}
> -
> -	return ret;
>  }
>  
>  static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
> @@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
>  	return 0;
>  
>  fail_host_init:
> -	return __deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
> +	return ret;
>  }
>  
>  static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
> @@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct tegra_pcie_dw *pcie)
>  	appl_writel(pcie, data, APPL_PINMUX);
>  }
>  
> -static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
> +static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
>  {
>  	tegra_pcie_downstream_dev_to_D0(pcie);
>  	dw_pcie_host_deinit(&pcie->pci.pp);
>  	tegra_pcie_dw_pme_turnoff(pcie);
> -
> -	return __deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
>  }
>  
>  static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
> @@ -2238,8 +2227,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
>  					       PORT_LOGIC_MSI_CTRL_INT_0_EN);
>  	tegra_pcie_downstream_dev_to_D0(pcie);
>  	tegra_pcie_dw_pme_turnoff(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
>  
> -	return __deinit_controller(pcie);
> +	return 0;
>  }
>  
>  static int tegra_pcie_dw_resume_noirq(struct device *dev)
> @@ -2267,7 +2257,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
>  	return 0;
>  
>  fail_host_init:
> -	return __deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
> +	return ret;
>  }
>  
>  static int tegra_pcie_dw_resume_early(struct device *dev)
> @@ -2305,7 +2296,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev)
>  		disable_irq(pcie->pci.pp.msi_irq);
>  
>  	tegra_pcie_dw_pme_turnoff(pcie);
> -	__deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
>  }
>  
>  static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail
  2020-11-30 12:10   ` Lorenzo Pieralisi
@ 2020-12-01 14:24     ` Thierry Reding
  2020-12-01 14:44       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-12-01 14:24 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Vidya Sagar, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

On Mon, Nov 30, 2020 at 12:10:07PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> > Currently the driver checks for error value of different APIs during the
> > uninitialization sequence. It just returns from there if there is any error
> > observed for one of those calls. Comparatively it is better to continue the
> > uninitialization sequence irrespective of whether some of them are
> > returning error. That way, it is more closer to complete uninitialization.
> 
> Hi Vidya, Thierry,
> 
> I can apply this series (dropping patches as suggested by Thierry),
> before though I wanted to ask you if this patch is really an
> improvement, it is hard to understand why skipping some error
> codes is OK for device correct operations to continue, maybe it
> is worth describing why some of those failures aren't really
> fatal.
> 
> Please let me know, thanks.

As explained in the commit message, the idea is to continue tearing
down even if things fail somewhere in the middle, because that ensures
that the hardware gets as close to an "uninitialized" state as possible.
If for example the first reset assert were to fail, then none of the
PHYs get disabled, the regulator stays on and the clocks stays on, all
of which can continue draining power after the controller has already
been torn down.

So yes, I think this is an improvement. It's unclear to me what you're
asking for, though. Would you rather have a comment somewhere near the
tegra_pcie_unconfig_controller() function that states the same thing as
the commit message?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail
  2020-12-01 14:24     ` Thierry Reding
@ 2020-12-01 14:44       ` Lorenzo Pieralisi
  0 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-12-01 14:44 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Vidya Sagar, robh+dt, bhelgaas, jonathanh, amanharitsh123,
	dinghao.liu, kw, linux-pci, linux-tegra, linux-kernel, kthota,
	mmaddireddy, sagar.tv

On Tue, Dec 01, 2020 at 03:24:24PM +0100, Thierry Reding wrote:
> On Mon, Nov 30, 2020 at 12:10:07PM +0000, Lorenzo Pieralisi wrote:
> > On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> > > Currently the driver checks for error value of different APIs during the
> > > uninitialization sequence. It just returns from there if there is any error
> > > observed for one of those calls. Comparatively it is better to continue the
> > > uninitialization sequence irrespective of whether some of them are
> > > returning error. That way, it is more closer to complete uninitialization.
> > 
> > Hi Vidya, Thierry,
> > 
> > I can apply this series (dropping patches as suggested by Thierry),
> > before though I wanted to ask you if this patch is really an
> > improvement, it is hard to understand why skipping some error
> > codes is OK for device correct operations to continue, maybe it
> > is worth describing why some of those failures aren't really
> > fatal.
> > 
> > Please let me know, thanks.
> 
> As explained in the commit message, the idea is to continue tearing
> down even if things fail somewhere in the middle, because that ensures
> that the hardware gets as close to an "uninitialized" state as possible.
> If for example the first reset assert were to fail, then none of the
> PHYs get disabled, the regulator stays on and the clocks stays on, all
> of which can continue draining power after the controller has already
> been torn down.

Understood. By reading the code it looked weird that eg a reset failure
was tolerable - I thought an error would be fatal (I don't know what are
the consequences for instance on a subsequent resume) but it looks like
it actually is not, that's the only point I raised.

> So yes, I think this is an improvement. It's unclear to me what you're
> asking for, though. Would you rather have a comment somewhere near the
> tegra_pcie_unconfig_controller() function that states the same thing as
> the commit message?

It could be useful but it is up to you, I will merge the series as-is
or I can add it myself, as you wish.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code
  2020-11-26 11:33   ` Thierry Reding
@ 2020-12-03 12:36     ` Vidya Sagar
  0 siblings, 0 replies; 21+ messages in thread
From: Vidya Sagar @ 2020-12-03 12:36 UTC (permalink / raw)
  To: Thierry Reding
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, Jonathan Hunter,
	amanharitsh123, dinghao.liu, kw, linux-pci, linux-tegra,
	linux-kernel, Krishna Thota, Manikanta Maddireddy, sagar.tv



> -----Original Message-----
> From: Thierry Reding <thierry.reding@gmail.com>
> Sent: Thursday, November 26, 2020 5:03 PM
> To: Vidya Sagar <vidyas@nvidia.com>
> Cc: lorenzo.pieralisi@arm.com; robh+dt@kernel.org; bhelgaas@google.com;
> Jonathan Hunter <jonathanh@nvidia.com>; amanharitsh123@gmail.com;
> dinghao.liu@zju.edu.cn; kw@linux.com; linux-pci@vger.kernel.org; linux-
> tegra@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Thota
> <kthota@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>;
> sagar.tv@gmail.com
> Subject: Re: [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable
> code
> 
> On Mon, Nov 09, 2020 at 10:49:32PM +0530, Vidya Sagar wrote:
> > If the absence of CLKREQ# signal is indicated by the absence of
> > "supports-clkreq" in the device-tree node, current driver is disabling
> > the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1
> > Sub-States offset is correctly initialized. Since default value of the
> > ASPM-L1SS offset is zero, this is causing the Vendor-ID wrongly
> > programmed to 0x10d2 instead of Nvidia's 0x10de thereby the quirks
> > applicable for Tegra194 are not being applied. This patch fixes this
> > issue by refactoring the code that disables the ASPM-L1SS advertisement.
> >
> > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > ---
> > V4:
> > * None
> >
> > V3:
> > * None
> >
> > V2:
> > * None
> >
> >  drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Looks like this no longer applies cleanly after that other fix that you sent earlier.
> But looking more closely, that's because that other fix already incorporates an
> equivalent change, so I think this can be dropped from this series.
Yes. This is no longer applies cleanly and I'll fix it in the next 
series, but, the current patch is still required.
The other change I pushed is taking care of getting a valid 'dbi' 
address before accessing the dbi region, but, this current change would 
make sure that 'pcie->cfg_link_cap_l1sub' would have a valid value 
before calling disable_aspm_l1/2() APIs.

Thanks,
Vidya Sagar
> 
> Thierry

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE
  2020-11-26 11:33   ` Thierry Reding
@ 2020-12-03 12:56     ` Vidya Sagar
  0 siblings, 0 replies; 21+ messages in thread
From: Vidya Sagar @ 2020-12-03 12:56 UTC (permalink / raw)
  To: Thierry Reding
  Cc: lorenzo.pieralisi, robh+dt, bhelgaas, Jonathan Hunter,
	amanharitsh123, dinghao.liu, kw, linux-pci, linux-tegra,
	linux-kernel, Krishna Thota, Manikanta Maddireddy, sagar.tv



> -----Original Message-----
> From: Thierry Reding <thierry.reding@gmail.com>
> Sent: Thursday, November 26, 2020 5:04 PM
> To: Vidya Sagar <vidyas@nvidia.com>
> Cc: lorenzo.pieralisi@arm.com; robh+dt@kernel.org; bhelgaas@google.com;
> Jonathan Hunter <jonathanh@nvidia.com>; amanharitsh123@gmail.com;
> dinghao.liu@zju.edu.cn; kw@linux.com; linux-pci@vger.kernel.org; linux-
> tegra@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Thota
> <kthota@nvidia.com>; Manikanta Maddireddy <mmaddireddy@nvidia.com>;
> sagar.tv@gmail.com
> Subject: Re: [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE
> 
> On Mon, Nov 09, 2020 at 10:49:33PM +0530, Vidya Sagar wrote:
> > As specified in the comment for pci_remap_cfgspace() define in
> > arch/arm64/include/asm/io.h file, PCIe configuration space should be
> > mapped as nGnRnE. Hence changing to dev_pci_remap_cfgspace() from
> > devm_ioremap_resource() for mapping DBI space as that is nothing but
> > the root port's own configuration space.
> >
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > ---
> > V4:
> > * None
> >
> > V3:
> > * None
> >
> > V2:
> > * Changed 'Strongly Ordered' to 'nGnRnE'
> >
> >  drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
> > b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index b172b1d49713..7a0c64436861 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct
> platform_device *pdev)
> >  	}
> >  	pcie->dbi_res = dbi_res;
> >
> > -	pci->dbi_base = devm_ioremap_resource(dev, dbi_res);
> > +	pci->dbi_base = devm_pci_remap_cfgspace(dev,
> > +						dbi_res->start,
> > +						resource_size(dbi_res));
> >  	if (IS_ERR(pci->dbi_base))
> >  		return PTR_ERR(pci->dbi_base);
> >
> 
> Similarly to patch 1/6, this is no longer required because it's already part of one
> of Rob's earlier patches, so this, too, can be dropped.
Yes. This patch is not required now. I'll drop it from the next patch 
series.

Thanks,
Vidya Sagar
> 
> Thierry

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-12-03 12:58 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 17:19 [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Vidya Sagar
2020-11-09 17:19 ` [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code Vidya Sagar
2020-11-26 11:33   ` Thierry Reding
2020-12-03 12:36     ` Vidya Sagar
2020-11-09 17:19 ` [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE Vidya Sagar
2020-11-26 11:33   ` Thierry Reding
2020-12-03 12:56     ` Vidya Sagar
2020-11-09 17:19 ` [PATCH V4 3/6] PCI: tegra: Set DesignWare IP version Vidya Sagar
2020-11-26 11:34   ` Thierry Reding
2020-11-09 17:19 ` [PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail Vidya Sagar
2020-11-26 11:34   ` Thierry Reding
2020-11-30 12:10   ` Lorenzo Pieralisi
2020-12-01 14:24     ` Thierry Reding
2020-12-01 14:44       ` Lorenzo Pieralisi
2020-11-09 17:19 ` [PATCH V4 5/6] PCI: tegra: Check return value of tegra_pcie_init_controller() Vidya Sagar
2020-11-26 11:34   ` Thierry Reding
2020-11-09 17:19 ` [PATCH V4 6/6] PCI: tegra: Disable LTSSM during L2 entry Vidya Sagar
2020-11-26 11:34   ` Thierry Reding
2020-11-25 17:57 ` [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver Thierry Reding
2020-11-25 19:51   ` Vidya Sagar
2020-11-26 11:31     ` Thierry Reding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).