[PATCH 0/4] PCI: tegra: Configuration space mapping cleanups and fixes

[PATCH 0/4] PCI: tegra: Configuration space mapping cleanups and fixes

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Hi Lorenzo,

This cleans up a few oddities that I found while reviewing and testing
the patch

	[PATCH V3 1/2] PCI: tegra: refactor config space mapping code

that Vidya Sagar sent out earlier. The first three patches are mostly
cleanup and admittedly somewhat bikeshedding in nature. They could've
been just review comments, but I thought I'd just submit them as a
series of patches since I had already typed them up anyway.

The last patch gets rid of an artificial restriction regarding the
mapping address and does a bit of simplification.

These are technically incremental on top of the original patch, but if
you prefer, feel free to squash them into that patch.

I've tested these on all of Tegra20, Tegra30, Tegra124, Tegra210 and
Tegra186.

Thanks,
Thierry

Thierry Reding (4):
  PCI: tegra: Clarify configuration space address computations
  PCI: tegra: Reorder parameters in offset computations
  PCI: tegra: Consolidate I/O register variables
  PCI: tegra: Remove artificial mapping restriction

 drivers/pci/host/pci-tegra.c | 71 ++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 38 deletions(-)

-- 
2.15.1

[PATCH 1/4] PCI: tegra: Clarify configuration space address computations

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Tegra uses a non-compatible variant of ECAM where the extended register
field is separate from the register field. Clarify that the register
offset also factors into the computation of the configuration space
addresses.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index f6d0430e6704..8a07c6f9e1b0 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -364,11 +364,13 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset)
  *
  * Mapping the whole extended configuration space would require 256 MiB of
  * virtual address space, only a small part of which will actually be used.
- * To work around this, a 4K of region is used to generate required
- * configuration transaction with relevant B:D:F values. This is achieved by
- * dynamically programming base address and size of AFI_AXI_BAR used for
- * end point config space mapping to make sure that the address (access to
- * which generates correct config transaction) falls in this 4K region
+ *
+ * To work around this, a 4 KiB region is used to generate the required
+ * configuration transaction with relevant B:D:F and register offset values.
+ * This is achieved by dynamically programming base address and size of
+ * AFI_AXI_BAR used for end point config space mapping to make sure that the
+ * address (access to which generates correct config transaction) falls in
+ * this 4 KiB region.
  */
 static unsigned long tegra_pcie_conf_offset(unsigned char b, unsigned int devfn,
 					    int where)
-- 
2.15.1

[PATCH 2/4] PCI: tegra: Reorder parameters in offset computations

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The current computation of the configuration space offset is slightly
difficult to read because the fields aren't naturally ordered. This is
no doubt done to put extended register and register fields together,
but that's confusing because they are separate in the address mapping
given in the comment above the computations.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 8a07c6f9e1b0..26b734c84850 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -372,11 +372,11 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset)
  * address (access to which generates correct config transaction) falls in
  * this 4 KiB region.
  */
-static unsigned long tegra_pcie_conf_offset(unsigned char b, unsigned int devfn,
-					    int where)
+static unsigned int tegra_pcie_conf_offset(u8 bus, unsigned int devfn,
+					   unsigned int where)
 {
-	return (b << 16) | (PCI_SLOT(devfn) << 11) | (PCI_FUNC(devfn) << 8) |
-	       (((where & 0xf00) >> 8) << 24) | (where & 0xff);
+	return ((where & 0xf00) << 16) | (bus << 16) | (PCI_SLOT(devfn) << 11) |
+	       (PCI_FUNC(devfn) << 8) | (where & 0xff);
 }
 
 static int tegra_pcie_add_bus(struct pci_bus *bus)
-- 
2.15.1

[PATCH 3/4] PCI: tegra: Consolidate I/O register variables

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Move variables that store I/O register region mappings together for
slightly better readability.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 26b734c84850..7f8a81e17db6 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -270,13 +270,12 @@ struct tegra_pcie {
 
 	void __iomem *pads;
 	void __iomem *afi;
+	void __iomem *cfg;
 	int irq;
 
 	struct list_head buses;
 	struct resource *cs;
 
-	void __iomem *cfg_va_base;
-
 	struct resource io;
 	struct resource pio;
 	struct resource mem;
@@ -434,7 +433,7 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 		}
 	} else {
 		offset = tegra_pcie_conf_offset(bus->number, devfn, where);
-		addr = pcie->cfg_va_base + (offset & (SZ_4K - 1));
+		addr = pcie->cfg + (offset & (SZ_4K - 1));
 		val = offset & ~(SZ_4K - 1);
 		afi_writel(pcie, pcie->cs->start - val, AFI_AXI_BAR0_START);
 		afi_writel(pcie, (val + SZ_4K) >> 12, AFI_AXI_BAR0_SZ);
@@ -1305,8 +1304,8 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 		goto poweroff;
 	}
 
-	pcie->cfg_va_base = devm_ioremap(dev, pcie->cs->start, SZ_4K);
-	if (!pcie->cfg_va_base) {
+	pcie->cfg = devm_ioremap(dev, pcie->cs.start, SZ_4K);
+	if (!pcie->cfg) {
 		dev_err(pcie->dev, "failed to ioremap config space\n");
 		err = -EADDRNOTAVAIL;
 		goto poweroff;
-- 
2.15.1

[PATCH 4/4] PCI: tegra: Remove artificial mapping restriction

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The current code restricts the location of the 4 KiB configuration space
mapping region. This is unnecessary if the AFI_FPCI_BAR0 register is
used to move the 4 KiB window into the FPCI address map. Doing this will
allow all generations of Tegra to be handled in a unified way.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 46 +++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 7f8a81e17db6..2e7bea127120 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -257,7 +257,6 @@ struct tegra_pcie_soc {
 	bool has_gen2;
 	bool force_pca_enable;
 	bool program_uphy;
-	bool use_4k_conf_space;
 };
 
 static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
@@ -274,7 +273,7 @@ struct tegra_pcie {
 	int irq;
 
 	struct list_head buses;
-	struct resource *cs;
+	struct resource cs;
 
 	struct resource io;
 	struct resource pio;
@@ -418,8 +417,6 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 	struct pci_host_bridge *host = pci_find_host_bridge(bus);
 	struct tegra_pcie *pcie = pci_host_bridge_priv(host);
 	void __iomem *addr = NULL;
-	u32 val = 0;
-	u32 offset = 0;
 
 	if (bus->number == 0) {
 		unsigned int slot = PCI_SLOT(devfn);
@@ -432,11 +429,17 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 			}
 		}
 	} else {
+		unsigned int offset;
+		u32 base;
+
 		offset = tegra_pcie_conf_offset(bus->number, devfn, where);
+
+		/* move 4 KiB window to offset within the FPCI region */
+		base = 0xfe100000 + ((offset & ~(SZ_4K - 1)) >> 8);
+		afi_writel(pcie, base, AFI_FPCI_BAR0);
+
+		/* move to correct offset within the 4 KiB page */
 		addr = pcie->cfg + (offset & (SZ_4K - 1));
-		val = offset & ~(SZ_4K - 1);
-		afi_writel(pcie, pcie->cs->start - val, AFI_AXI_BAR0_START);
-		afi_writel(pcie, (val + SZ_4K) >> 12, AFI_AXI_BAR0_SZ);
 	}
 
 	return addr;
@@ -689,8 +692,9 @@ static void tegra_pcie_setup_translations(struct tegra_pcie *pcie)
 	u32 fpci_bar, size, axi_address;
 
 	/* Bar 0: type 1 extended configuration space */
-	fpci_bar = 0xfe100000;
-	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR0);
+	size = resource_size(&pcie->cs);
+	afi_writel(pcie, pcie->cs.start, AFI_AXI_BAR0_START);
+	afi_writel(pcie, size >> 12, AFI_AXI_BAR0_SZ);
 
 	/* Bar 1: downstream IO bar */
 	fpci_bar = 0xfdfc0000;
@@ -1246,7 +1250,6 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct resource *pads, *afi, *res;
 	const struct tegra_pcie_soc *soc = pcie->soc;
-	u32 axi_addr = 0;
 	int err;
 
 	err = tegra_pcie_clocks_get(pcie);
@@ -1296,18 +1299,14 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 		goto poweroff;
 	}
 
-	axi_addr = pcie->soc->use_4k_conf_space ?
-		   res->start : res->end - SZ_4K + 1;
-	pcie->cs = devm_request_mem_region(dev, axi_addr, SZ_4K, res->name);
-	if (!pcie->cs) {
-		err = -EADDRNOTAVAIL;
-		goto poweroff;
-	}
+	pcie->cs = *res;
 
-	pcie->cfg = devm_ioremap(dev, pcie->cs.start, SZ_4K);
-	if (!pcie->cfg) {
-		dev_err(pcie->dev, "failed to ioremap config space\n");
-		err = -EADDRNOTAVAIL;
+	/* constrain configuration space to 4 KiB */
+	pcie->cs.end = pcie->cs.start + SZ_4K - 1;
+
+	pcie->cfg = devm_ioremap_resource(dev, &pcie->cs);
+	if (IS_ERR(pcie->cfg)) {
+		err = PTR_ERR(pcie->cfg);
 		goto poweroff;
 	}
 
@@ -2120,7 +2119,6 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.has_gen2 = false,
 	.force_pca_enable = false,
 	.program_uphy = true,
-	.use_4k_conf_space = true,
 };
 
 static const struct tegra_pcie_soc tegra30_pcie = {
@@ -2138,7 +2136,6 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.has_gen2 = false,
 	.force_pca_enable = false,
 	.program_uphy = true,
-	.use_4k_conf_space = false,
 };
 
 static const struct tegra_pcie_soc tegra124_pcie = {
@@ -2155,7 +2152,6 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.has_gen2 = true,
 	.force_pca_enable = false,
 	.program_uphy = true,
-	.use_4k_conf_space = false,
 };
 
 static const struct tegra_pcie_soc tegra210_pcie = {
@@ -2172,7 +2168,6 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.has_gen2 = true,
 	.force_pca_enable = true,
 	.program_uphy = true,
-	.use_4k_conf_space = false,
 };
 
 static const struct tegra_pcie_soc tegra186_pcie = {
@@ -2190,7 +2185,6 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.has_gen2 = true,
 	.force_pca_enable = false,
 	.program_uphy = false,
-	.use_4k_conf_space = true,
 };
 
 static const struct of_device_id tegra_pcie_of_match[] = {
-- 
2.15.1

Re: [PATCH 0/4] PCI: tegra: Configuration space mapping cleanups and fixes

[Lorenzo Pieralisi]

Hi Thierry,

On Thu, Dec 14, 2017 at 02:45:41PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Hi Lorenzo,
> 
> This cleans up a few oddities that I found while reviewing and testing
> the patch
> 
> 	[PATCH V3 1/2] PCI: tegra: refactor config space mapping code
> 
> that Vidya Sagar sent out earlier. The first three patches are mostly
> cleanup and admittedly somewhat bikeshedding in nature. They could've
> been just review comments, but I thought I'd just submit them as a
> series of patches since I had already typed them up anyway.
> 
> The last patch gets rid of an artificial restriction regarding the
> mapping address and does a bit of simplification.
> 
> These are technically incremental on top of the original patch, but if
> you prefer, feel free to squash them into that patch.

I took some time to have a look at all of them and actually I am happy
with the end result, except that I would prefer if you squash them all
in and rewrite the logs since I can easily miss something (eg I have no
insights into the Tegra config space FPCI windowing mechanism) - I will
merge the resulting patch(es).

I have a question: after merging both series, are

tegra_pcie_{add/remove}_bus()

(and struct tegra_pcie_bus)

still needed ? I do not think so.

> I've tested these on all of Tegra20, Tegra30, Tegra124, Tegra210 and
> Tegra186.

If the testing goes OK please send me unified series and will merge
that one.

This brings me to a question for you and Bjorn: how do you usually
handle DT updates ? I assume we send them via the PCI tree but I am
asking to prevent any issue upfront.

Thanks,
Lorenzo

Re: [PATCH 0/4] PCI: tegra: Configuration space mapping cleanups and fixes

[Thierry Reding]

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

On Thu, Dec 14, 2017 at 05:37:45PM +0000, Lorenzo Pieralisi wrote:
> Hi Thierry,
> 
> On Thu, Dec 14, 2017 at 02:45:41PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Hi Lorenzo,
> > 
> > This cleans up a few oddities that I found while reviewing and testing
> > the patch
> > 
> > 	[PATCH V3 1/2] PCI: tegra: refactor config space mapping code
> > 
> > that Vidya Sagar sent out earlier. The first three patches are mostly
> > cleanup and admittedly somewhat bikeshedding in nature. They could've
> > been just review comments, but I thought I'd just submit them as a
> > series of patches since I had already typed them up anyway.
> > 
> > The last patch gets rid of an artificial restriction regarding the
> > mapping address and does a bit of simplification.
> > 
> > These are technically incremental on top of the original patch, but if
> > you prefer, feel free to squash them into that patch.
> 
> I took some time to have a look at all of them and actually I am happy
> with the end result, except that I would prefer if you squash them all
> in and rewrite the logs since I can easily miss something (eg I have no
> insights into the Tegra config space FPCI windowing mechanism) - I will
> merge the resulting patch(es).
> 
> I have a question: after merging both series, are
> 
> tegra_pcie_{add/remove}_bus()
> 
> (and struct tegra_pcie_bus)
> 
> still needed ? I do not think so.

Yes, you're right, those were only to track the quilt mappings that we
used to do. I've removed them in the patch I just sent (v4).

> > I've tested these on all of Tegra20, Tegra30, Tegra124, Tegra210 and
> > Tegra186.
> 
> If the testing goes OK please send me unified series and will merge
> that one.
> 
> This brings me to a question for you and Bjorn: how do you usually
> handle DT updates ? I assume we send them via the PCI tree but I am
> asking to prevent any issue upfront.

DT updates usually go through the ARM SoC tree. It's usually okay to do
that because the DT stability requirement nicely decouples DT and driver
updates already.

In this particular case, the driver will be presented with a 256 MiB
window from existing DTs and simply use the first 4 KiB of that window
for the configuration space mapping. We can apply the DT patches at any
later time to reduce the region from 256 MiB to 4 KiB. I plan to do that
during for v4.17.

Thierry

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