tegra194 PCI endpoint fixes

The pci_epc_raise_irq() supplies a MSI or MSI-X interrupt number in range (1-N), see kdoc for pci_epc_raise_irq().

Thus, for MSI pci_epc_raise_irq() will supply interrupt number 1-32.

Convert the interrupt number to an MSI vector.

With this, the PCI endpoint kselftest test case MSI_TEST passes.

Also, set msi_capable to true, as the driver obviously supports MSI. This helps pci_endpoint_test to use the optimal IRQ type when using PCITEST_IRQ_TYPE_AUTO.

Cc [email protected]

Fixes c57247f940e8 ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194")

Signed-off-by Niklas Cassel <[email protected]>

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

drivers/pci/controller/dwc/pcie-tegra194.c +2 -2

index 7c295ec6f0f16..63d310e5335f4 100644
@@ -1969,10 +1969,10 @@ static int tegra_pcie_ep_raise_intx_irq(struct tegra_pcie_dw *pcie, u16 irq)
 
 static int tegra_pcie_ep_raise_msi_irq(struct tegra_pcie_dw *pcie, u16 irq)
 {
-	if (unlikely(irq > 31))
+	if (unlikely(irq > 32))
 		return -EINVAL;
 
-	appl_writel(pcie, BIT(irq), APPL_MSI_CTRL_1);
+	appl_writel(pcie, BIT(irq - 1), APPL_MSI_CTRL_1);
 
 	return 0;
 }

Quoted 39 lines · Niklas Cassel

The pci_epc_raise_irq() supplies a MSI or MSI-X interrupt number in range (1-N), see kdoc for pci_epc_raise_irq().

Thus, for MSI pci_epc_raise_irq() will supply interrupt number 1-32.

Convert the interrupt number to an MSI vector.

With this, the PCI endpoint kselftest test case MSI_TEST passes.

Also, set msi_capable to true, as the driver obviously supports MSI. This helps pci_endpoint_test to use the optimal IRQ type when using PCITEST_IRQ_TYPE_AUTO.

Cc [email protected]

Fixes c57247f940e8 ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194")

Signed-off-by Niklas Cassel <[email protected]>

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

drivers/pci/controller/dwc/pcie-tegra194.c +2 -2

index 7c295ec6f0f16..63d310e5335f4 100644
@@ -1969,10 +1969,10 @@ static int tegra_pcie_ep_raise_intx_irq(struct tegra_pcie_dw *pcie, u16 irq)
 
 static int tegra_pcie_ep_raise_msi_irq(struct tegra_pcie_dw *pcie, u16 irq)
 {
-	if (unlikely(irq > 31))
+	if (unlikely(irq > 32))
 		return -EINVAL;
 
-	appl_writel(pcie, BIT(irq), APPL_MSI_CTRL_1);
+	appl_writel(pcie, BIT(irq - 1), APPL_MSI_CTRL_1);
 
 	return 0;
 }

@@ -2012,6 +2012,7 @@ static int tegra_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 

drivers/pci/controller/dwc/pcie-tegra194.c static int tegra_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no, · line 2015

Quoted 3 lines

drivers/pci/controller/dwc/pcie-tegra194.c +1 -0

 static const struct pci_epc_features tegra_pcie_epc_features = {
 	.linkup_notifier = true,
+	.msi_capable = true,

This change is unrelated to the above tegra_pcie_ep_raise_msi_irq() fix. So this change should be in a separate patch.

Quoted 9 lines

drivers/pci/controller/dwc/pcie-tegra194.c static int tegra_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no, · line 2015

Quoted 5 lines

drivers/pci/controller/dwc/pcie-tegra194.c +1 -0

@@ -2012,6 +2012,7 @@ static int tegra_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 
 static const struct pci_epc_features tegra_pcie_epc_features = {
 	.linkup_notifier = true,
+	.msi_capable = true,

This change is unrelated to the above tegra_pcie_ep_raise_msi_irq() fix. So this change should be in a separate patch.

I did the split and applied the series, thanks!

Tegra already defines all BARs expect for BAR0 as BAR_RESERVED. This is sufficient for pci-epf-test to not allocate backing memory and to not call set_bar() for those BARs. However, marking a BAR as BAR_RESERVED does not mean that the BAR get disabled.

The host side driver, pci_endpoint_test, simply does an ioremap for all enabled BARs, and will run tests against all enabled BARs. (I.e. it will run tests also against the BARs marked as BAR_RESERVED.)

After running the BARs tests (which will write to all enabled BARs), the inbound address translation is broken.
This is because the tegra controller exposes the ATU Port Logic Structure in BAR4. So when BAR4 is written, the inbound address translation settings get overwritten.

To avoid this, implement the dw_pcie_ep_ops .init() callback and start off by disabling all BARs (pci-epf-test will later enable/configure BARs that are not defined as BAR_RESERVED).

This matches the behavior of other PCIe endpoint drivers:
dra7xx, imx6, layerscape-ep, artpec6, dw-rockchip, qcom-ep, rcar-gen4, and uniphier-ep.

With this, the PCI endpoint kselftest test case CONSECUTIVE_BAR_TEST (which was specifically made to detect address translation issues) passes.

Cc [email protected]

Fixes c57247f940e8 ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194")

Signed-off-by Niklas Cassel <[email protected]>

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

drivers/pci/controller/dwc/pcie-tegra194.c +10 -0

index 63d310e5335f4..7eb48cc13648e 100644
@@ -1955,6 +1955,15 @@ static irqreturn_t tegra_pcie_ep_pex_rst_irq(int irq, void *arg)
  	return IRQ_HANDLED;
  }
 
+static void tegra_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+};
+
  static int tegra_pcie_ep_raise_intx_irq(struct tegra_pcie_dw *pcie, u16 irq)
  {
  	/* Tegra194 supports only INTA */

@@ -2030,6 +2039,7 @@ tegra_pcie_ep_get_features(struct dw_pcie_ep *ep)
  }
 
  static const struct dw_pcie_ep_ops pcie_ep_ops = {
+	.init = tegra_pcie_ep_init,
  	.raise_irq = tegra_pcie_ep_raise_irq,
  	.get_features = tegra_pcie_ep_get_features,
  };

Hi Niklas,

Tegra PCIe exposes only DMA register over BAR4, not iATU. So, the issue described in this commit message is not applicable. This patch is disabling BAR2 and BAR4, after enumeration I see only BAR0. I think we should revert this patch.
Please share your inputs on this.

Quoted 7 lines · Manikanta Maddireddy

Hi Niklas,

Tegra PCIe exposes only DMA register over BAR4, not iATU. So, the issue described in this commit message is not applicable. This patch is disabling BAR2 and BAR4, after enumeration I see only BAR0. I think we should revert this patch.
Please share your inputs on this.

If you look at this commit, it only disables all BARs by default, which brings the tegra driver in-line with all other DWC based endpoint drivers: dra7xx, imx6, layerscape-ep, artpec6, dw-rockchip, qcom-ep, rcar-gen4, and uniphier-ep.

A PCI endpoint function (EPF) driver will still be able to enable a BAR that was disabled in .init().
However, an EPF driver will not be able to use/enable a reserved BAR.

Look at e.g. the code in pci-epf-test. It will not allocate backing memory for a BAR that is reserved, so having a BAR enabled that we have not allocated backing memory for is wrong.

Commit c57247f940e8 ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194") is the commit that marked all BARs other than BAR0 as reserved, so if you want to test BARs other than BAR0, talk to the author of that commit.

If you revert this patch, tools/testing/selftests/pci_endpoint/pci_endpoint_test will once again fail the consecutive BAR test, so I think it would be wrong to revert this patch.

If it is ATU registers or eDMA registers exposed in BAR4 does not really matter. The end result is that you overwrite eDMA registers that you should not be overwriting when you run the BAR tests. (So BAR4 should absolutely be marked as reserved).

I don't recall, but if you overwrite the eDMA registers, then in addition to the consecutive BAR test failing, most likely the DMA test cases will also fail.

Have you tried running tools/testing/selftests/pci_endpoint/pci_endpoint_test ?

Kind regards,
Niklas

Quoted 4 lines · Manikanta Maddireddy

Quoted 3 lines

Hi Niklas,

Tegra PCIe exposes only DMA register over BAR4, not iATU.

Hi Niklas,

In Tegra234 PCIe, BAR1 is MSI-X table and BAR2 is DMA registers backed by PCIe HW RAM and registers.

EPF driver shouldn't allocate memory for
these two BARs. This is the reason for marking them as reserved in Tegra PCIe driver. DMA registers are exposed over BAR2 to allow PCI client driver in host to transfer data from host to endpoint using endpoint remote DMA read functionality. BAR test fails on this because not all register bits are writable. Consider NVMe example which has RO capability bits at the start of the BAR, it is not correct to add BAR test on these bits.

I think following fixes are required to address this issue,

1. BAR test in pci_endpoint_test should skip MSI-X table.
2. BAR test in pci_endpoint_test should provide option to skip this test on known reserved BARs, maybe we can use pci_endpoint_test_data for this.

3. EPC driver should provide BAR_DISABLED enum to disable unused BARs.

4. Tegra PCIe driver should disable only BAR_DISABLED bars and leave BAR_RESERVED untouched.
5. Return NO_BAR for both BAR_DISABLED and BAR_RESERVED in pci_epc_get_next_free_bar()

BAR_RESERVED already means disabled, it just assumes that an EPC driver disables all BARs by default, which is the case for:
pci-dra7xx.c, pci-imx6.c, pci-layerscape-ep.c, pcie-artpec6.c, pcie-designware-plat.c, pcie-dw-rockchip.c, pcie-qcom-ep.c, pcie-rcar-gen4.c, pcie-stm32-ep.c, pcie-uniphier-ep.c.
(All drivers which disables all BARs by default in the init() callback using dw_pcie_ep_reset_bar(). pci-epf-test will later enable all BARs that are not marked as BAR_RESERVED.)

That leaves: pcie-keembay.c, pci-keystone.c, pcie-tegra194.c (before my patch).

For pcie-keembay.c, this is not a problem, because BAR0, BAR2, BAR4 are marked as only_64bit, so pci-epf-test configure these BARs as 64-bit BARs, and thus BAR1, BAR3, and BAR5 will get disabled implicitly.

For pci-keystone.c, this is the only driver that is a bit weird, it marks BAR0 and BAR1 as reserved, but does not disable them in the init() callback. It seems force set BAR0 as a 32-bit BAR in the init() callback.

Thus, for all drivers except for pci-keystone.c, BAR_RESERVED does mean BAR_DISABLED. Feel free to send a patch that renames BAR_RESERVED to BAR_DISABLED.

If you send such a patch, perhaps you also want to modify the PCI endpoint core to call reset_bar() for all BARs marked as BAR_RESERVED/BAR_DISABLED, instead of each EPC driver doing so in the init() callback. I think the main reason why this is not done already is that thare is no reset_bar() op in struct pci_epc_ops epc_ops, there is only clear_bar() which clears an BAR enabled by an EPF driver. (So you would most likely also need to add a .disable_bar() op in struct pci_epc_ops epc_ops.)

Quoted 23 lines

BAR_RESERVED already means disabled, it just assumes that an EPC driver disables all BARs by default, which is the case for:
pci-dra7xx.c, pci-imx6.c, pci-layerscape-ep.c, pcie-artpec6.c, pcie-designware-plat.c, pcie-dw-rockchip.c, pcie-qcom-ep.c, pcie-rcar-gen4.c, pcie-stm32-ep.c, pcie-uniphier-ep.c.
(All drivers which disables all BARs by default in the init() callback using dw_pcie_ep_reset_bar(). pci-epf-test will later enable all BARs that are not marked as BAR_RESERVED.)

That leaves: pcie-keembay.c, pci-keystone.c, pcie-tegra194.c (before my patch).

For pcie-keembay.c, this is not a problem, because BAR0, BAR2, BAR4 are marked as only_64bit, so pci-epf-test configure these BARs as 64-bit BARs, and thus BAR1, BAR3, and BAR5 will get disabled implicitly.

For pci-keystone.c, this is the only driver that is a bit weird, it marks BAR0 and BAR1 as reserved, but does not disable them in the init() callback. It seems force set BAR0 as a 32-bit BAR in the init() callback.

Thus, for all drivers except for pci-keystone.c, BAR_RESERVED does mean BAR_DISABLED. Feel free to send a patch that renames BAR_RESERVED to BAR_DISABLED.

Quoted 7 lines

If you send such a patch, perhaps you also want to modify the PCI endpoint core to call reset_bar() for all BARs marked as BAR_RESERVED/BAR_DISABLED, instead of each EPC driver doing so in the init() callback. I think the main reason why this is not done already is that thare is no reset_bar() op in struct pci_epc_ops epc_ops, there is only clear_bar() which clears an BAR enabled by an EPF driver. (So you would most likely also need to add a .disable_bar() op in struct pci_epc_ops epc_ops.)

Aksh (on To:),

since you have a @ti.com email, perhaps you can explain how pci-keystone.c can pass all the pci-epf-test test cases, considering that this is the only driver that has BARs (BAR0 and BAR1) marked as BAR_RESERVED but do not also disable the BARs (using dw_pcie_ep_reset_bar()) in the init() callback.

Or, perhaps the simple answer is that pci-keystone.c does not pass all pci-epf-test test cases?

Kind regards,
Niklas

Quoted 7 lines

since you have a @ti.com email, perhaps you can explain how pci-keystone.c can pass all the pci-epf-test test cases, considering that this is the only driver that has BARs (BAR0 and BAR1) marked as BAR_RESERVED but do not also disable the BARs (using dw_pcie_ep_reset_bar()) in the init() callback.

Or, perhaps the simple answer is that pci-keystone.c does not pass all pci-epf-test test cases?

Hi Niklas,

I just joined the organization and have no context on why the pci-keystone.c have BAR0 and BAR1 as reserved, without disabling the bars using dw_pcie_ep_reset_bar() in the .init() callback. Because the AM65 do not use any BARs for any purpose like Tegra194 does (ATU registers or eDMA registers exposed in BAR4 for example), there would be no issue if the BAR0 and BAR1 are overwritten.

This was introduced in the driver the time the EP support was added to the driver by Kishon in commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x Platforms"), where no context is provided in the comments or commit message why the BAR0/1 are marked as reserved in the features. Perhaps Kishon can provide a better insight over this.

Quoted 17 lines · Aksh Garg

Quoted 7 lines

since you have a @ti.com email, perhaps you can explain how pci-keystone.c can pass all the pci-epf-test test cases, considering that this is the only driver that has BARs (BAR0 and BAR1) marked as BAR_RESERVED but do not also disable the BARs (using dw_pcie_ep_reset_bar()) in the init() callback.

Or, perhaps the simple answer is that pci-keystone.c does not pass all pci-epf-test test cases?

Hi Niklas,

I just joined the organization and have no context on why the pci-keystone.c have BAR0 and BAR1 as reserved, without disabling the bars using dw_pcie_ep_reset_bar() in the .init() callback. Because the AM65 do not use any BARs for any purpose like Tegra194 does (ATU registers or eDMA registers exposed in BAR4 for example), there would be no issue if the BAR0 and BAR1 are overwritten.

Quoted 5 lines

This was introduced in the driver the time the EP support was added to the driver by Kishon in commit 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x Platforms"), where no context is provided in the comments or commit message why the BAR0/1 are marked as reserved in the features. Perhaps Kishon can provide a better insight over this.

It is extra confusing, since the older driver from TI:
pci-dra7xx.c does have the dw_pcie_ep_reset_bar() calls in init() (git blame shows added by Kishon), so it is a bit surprising that the newer driver (pci-keystone.c) does not.

(And like I explained, currently all DWC drivers except keystone and pcie-keembay.c do have the dw_pcie_ep_reset_bar() calls in init().)

Kind regards,
Niklas

Quoted 8 lines

For pci-keystone.c, this is the only driver that is a bit weird, it marks BAR0 and BAR1 as reserved, but does not disable them in the init() callback. It seems force set BAR0 as a 32-bit BAR in the init() callback.

Thus, for all drivers except for pci-keystone.c, BAR_RESERVED does mean BAR_DISABLED. Feel free to send a patch that renames BAR_RESERVED to BAR_DISABLED.

Quoted 8 lines

If you send such a patch, perhaps you also want to modify the PCI endpoint core to call reset_bar() for all BARs marked as BAR_RESERVED/BAR_DISABLED, instead of each EPC driver doing so in the init() callback. I think the main reason why this is not done already is that thare is no reset_bar() op in struct pci_epc_ops epc_ops, there is only clear_bar() which clears an BAR enabled by an EPF driver. (So you would most likely also need to add a .disable_bar() op in struct pci_epc_ops epc_ops.)

pci-epc.h defined

 * @BAR_RESERVED: The BAR should not be touched by an EPF driver.

I believe you are interpreting this as unused BAR?

In Tegra PCIe, BAR2 and BAR4 are backed by PCIe HW memory which

shouldn't be touched by EPF, but should be kept enabled.

This support is not available. I suggested to add BAR_DISABLED

for unused and use BAR_RESERVED for bars like above.

Tegra already defines all BARs expect for BAR0 as BAR_RESERVED. This is sufficient for pci-epf-test to not allocate backing memory and to not call set_bar() for those BARs. However, marking a BAR as BAR_RESERVED does not mean that the BAR get disabled.

The host side driver, pci_endpoint_test, simply does an ioremap for all enabled BARs, and will run tests against all enabled BARs. (I.e. it will run tests also against the BARs marked as BAR_RESERVED.)

After running the BARs tests (which will write to all enabled BARs), the inbound address translation is broken.
This is because the tegra controller exposes the ATU Port Logic Structure in BAR4. So when BAR4 is written, the inbound address translation settings get overwritten.

To avoid this, implement the dw_pcie_ep_ops .init() callback and start off by disabling all BARs (pci-epf-test will later enable/configure BARs that are not defined as BAR_RESERVED).

This matches the behavior of other PCIe endpoint drivers:
dra7xx, imx6, layerscape-ep, artpec6, dw-rockchip, qcom-ep, rcar-gen4, and uniphier-ep.

With this, the PCI endpoint kselftest test case CONSECUTIVE_BAR_TEST (which was specifically made to detect address translation issues) passes.

Cc [email protected]

Fixes c57247f940e8 ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194")

Signed-off-by Niklas Cassel <[email protected]>

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

drivers/pci/controller/dwc/pcie-tegra194.c +10 -0

index 63d310e5335f4..7eb48cc13648e 100644
@@ -1955,6 +1955,15 @@ static irqreturn_t tegra_pcie_ep_pex_rst_irq(int irq, void *arg)
  	return IRQ_HANDLED;
  }
 
+static void tegra_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+};
+
  static int tegra_pcie_ep_raise_intx_irq(struct tegra_pcie_dw *pcie, u16 irq)
  {
  	/* Tegra194 supports only INTA */

@@ -2030,6 +2039,7 @@ tegra_pcie_ep_get_features(struct dw_pcie_ep *ep)
  }
 
  static const struct dw_pcie_ep_ops pcie_ep_ops = {
+	.init = tegra_pcie_ep_init,
  	.raise_irq = tegra_pcie_ep_raise_irq,
  	.get_features = tegra_pcie_ep_get_features,
  };

Shin'ichiro and I have recently been trying to get the Jetson to run the PCI endpoint kselftests on a tegra based board. These patches ensure that the BAR test cases and MSI test cases pass. The MSI-X test cases still fail.

Dependency
==========
"PCI: tegra194: Fix broken tegra_pcie_ep_raise_msi_irq()" requires commit 27fce9e8c6f0 ("PCI: endpoint: Drop superfluous pci_epc_features initialization") which is currently queued on branch pci/endpoint.

[...]