ARM: tegra: Add PCIe device tree support

This commit adds a platform device driver for the PCIe controller on Tegra SOCs. Current users of the old code (TrimSlice and Harmony) are converted and now initialize and register a corresponding platform device.

arch/arm/mach-tegra/devices.c +7 -0

 
+static struct resource tegra_pcie_resources[] = {
+	[0] = {
+		.start = TEGRA_PCIE_BASE,
+		.end = TEGRA_PCIE_BASE + TEGRA_PCIE_SIZE - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {

#define MEM_BASE_0              (TEGRA_PCIE_BASE + SZ_256M)
#define MEM_SIZE_0              SZ_128M
#define MEM_BASE_1              (MEM_BASE_0 + MEM_SIZE_0)
#define MEM_SIZE_1              SZ_128M
#define PREFETCH_MEM_BASE_0     (MEM_BASE_1 + MEM_SIZE_1)
#define PREFETCH_MEM_SIZE_0     SZ_128M
#define PREFETCH_MEM_BASE_1     (PREFETCH_MEM_BASE_0 + PREFETCH_MEM_SIZE_0)
#define PREFETCH_MEM_SIZE_1     SZ_128M

/*
* Tegra2 defines 1GB in the AXI address map for PCIe.
*
* That address space is split into different regions, with sizes and
* offsets as follows:
*
* 0x80000000 - 0x80003fff - PCI controller registers

* 0x80004000 - 0x80103fff - PCI configuration space
* 0x80104000 - 0x80203fff - PCI extended configuration space
* 0x80203fff - 0x803fffff - unused
* 0x80400000 - 0x8040ffff - downstream IO
* 0x80410000 - 0x8fffffff - unused
* 0x90000000 - 0x9fffffff - non-prefetchable memory
* 0xa0000000 - 0xbfffffff - prefetchable memory
*/

Quoted 14 lines · Thierry Reding

This commit adds a platform device driver for the PCIe controller on Tegra SOCs. Current users of the old code (TrimSlice and Harmony) are converted and now initialize and register a corresponding platform device.

arch/arm/mach-tegra/devices.c +7 -0

 
+static struct resource tegra_pcie_resources[] = {
+	[0] = {
+		.start = TEGRA_PCIE_BASE,
+		.end = TEGRA_PCIE_BASE + TEGRA_PCIE_SIZE - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {

arch/arm/mach-tegra/devices.c struct platform_device tegra_nand_device = { · line 755

Quoted 10 lines

arch/arm/mach-tegra/devices.c +10 -0

+		.start = TEGRA_PCIE_MMIO_BASE,
+		.end = TEGRA_PCIE_MMIO_BASE + TEGRA_PCIE_MMIO_SIZE - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[2] = {
+		.start = INT_PCIE_INTR,
+		.end = INT_PCIE_INTR,
+		.flags = IORESOURCE_IRQ,
+	},
+};

I'm not sure those resources either cover all the necessary regions, nor are as fine-grained as they should be ...

In particular, in pcie.c, I see separate afi_writel() and pads_writel() implying those are separate regions.

Also, I see the following hard-code specific addresses, and are still used by the driver after conversion:

Quoted 8 lines

#define MEM_BASE_0              (TEGRA_PCIE_BASE + SZ_256M)
#define MEM_SIZE_0              SZ_128M
#define MEM_BASE_1              (MEM_BASE_0 + MEM_SIZE_0)
#define MEM_SIZE_1              SZ_128M
#define PREFETCH_MEM_BASE_0     (MEM_BASE_1 + MEM_SIZE_1)
#define PREFETCH_MEM_SIZE_0     SZ_128M
#define PREFETCH_MEM_BASE_1     (PREFETCH_MEM_BASE_0 + PREFETCH_MEM_SIZE_0)
#define PREFETCH_MEM_SIZE_1     SZ_128M

Also, there's a comment describing the register layout in terms of a number of separate regions:

Quoted 7 lines

/*
* Tegra2 defines 1GB in the AXI address map for PCIe.
*
* That address space is split into different regions, with sizes and
* offsets as follows:
*
* 0x80000000 - 0x80003fff - PCI controller registers

Quoted 8 lines

* 0x80004000 - 0x80103fff - PCI configuration space
* 0x80104000 - 0x80203fff - PCI extended configuration space
* 0x80203fff - 0x803fffff - unused
* 0x80400000 - 0x8040ffff - downstream IO
* 0x80410000 - 0x8fffffff - unused
* 0x90000000 - 0x9fffffff - non-prefetchable memory
* 0xa0000000 - 0xbfffffff - prefetchable memory
*/

(the latter 2 regions at least being also split in half for each of the 2 host ports)

Shouldn't each of these regions be a separate entry in the platform device resources.

This perhaps isn't that relevant for Tegra20 alone, but Tegra30 supports 3 host ports instead of 2 (hence I presume alters the internal layout of the 1G chunk of physical memory space allocated to PCIe), and moves the PCIe area from 2G..3G to 0G..1G (so invalidates the hard-coded *_MEM_BASE_* above).

That said, I'm not sure whether Tegra20's and Tegra30's PCIe controllers are similar enough to make a shared driver worthwhile/possible. (Although our downstream Android driver appears to handle both with a very small number of ifdefs).

If they are similar, then I think my comments above should be addressed. If they are not similar, then I think you can just have a single 1G memory region in the resources, and split it up internally, rather than needing separate resources for different parts of the address space.

While we can easily fix this kind of driver internals so this doesn't seem like a big deal, this kind of change would impact the device tree binding, so it seems that we need to sort it out before DT conversion.

AFAICT the even partitioning of the non-prefetchable and prefetchable memory regions is arbitrary and it could potentially be useful to make it configurable via the DT.

Quoted 3 lines · Thierry Reding

AFAICT the even partitioning of the non-prefetchable and prefetchable memory regions is arbitrary and it could potentially be useful to make it configurable via the DT.

So it turns out that this isn't true. But apart from the comments in the driver I couldn't find any reference to how the prefetch and non-prefetch ranges are partitioned. Judging by the code for Tegra2 this is evenly partitioned among ports 0 and 1 but it would be interesting to know this is done on Tegra3. Is there any way you can find out?

It'd also be nice if some better documentation for the PCIe controller could be made available. The Tegra2 TRM doesn't contain any of the AFI or PADS registers and while the Tegra3 TRM documents more registers, their offsets are completely missing.

/*
* Tegra2 defines 1GB in the AXI address map for PCIe.
*
* That address space is split into different regions, with sizes and
* offsets as follows:
*
* 0x80000000 - 0x80003fff - PCI controller registers

* 0x80004000 - 0x80103fff - PCI configuration space
* 0x80104000 - 0x80203fff - PCI extended configuration space
* 0x80203fff - 0x803fffff - unused
* 0x80400000 - 0x8040ffff - downstream IO
* 0x80410000 - 0x8fffffff - unused
* 0x90000000 - 0x9fffffff - non-prefetchable memory
* 0xa0000000 - 0xbfffffff - prefetchable memory
*/

/*
* AXI address map for the PCIe aperture , defines 1GB in the AXI
* address map for PCIe.
*

 *  That address space is split into different regions, with sizes and
 *  offsets as follows. Exepct for the Register space, SW is free to slice the
 *  regions as it chooces.
 *
 *  The split below seems to work fine for now.
 *
 *  0x0000_0000 to 0x00ff_ffff - Register space          16MB.
 *  0x0100_0000 to 0x01ff_ffff - Config space            16MB.
 *  0x0200_0000 to 0x02ff_ffff - Extended config space   16MB.
 *  0x0300_0000 to 0x03ff_ffff - Downstream IO space
 *   ... Will be filled with other BARS like MSI/upstream IO etc.
 *  0x1000_0000 to 0x1fff_ffff - non-prefetchable memory aperture
 *  0x2000_0000 to 0x3fff_ffff - Prefetchable memory aperture
 *
 *  Config and Extended config sizes are choosen to support
 *  maximum of 256 devices,
 *  which is good enough for all the current use cases.

*
*/

Quoted 12 lines · Thierry Reding

Quoted 3 lines · Thierry Reding

AFAICT the even partitioning of the non-prefetchable and prefetchable memory regions is arbitrary and it could potentially be useful to make it configurable via the DT.

So it turns out that this isn't true. But apart from the comments in the driver I couldn't find any reference to how the prefetch and non-prefetch ranges are partitioned. Judging by the code for Tegra2 this is evenly partitioned among ports 0 and 1 but it would be interesting to know this is done on Tegra3. Is there any way you can find out?

Quoted 4 lines

It'd also be nice if some better documentation for the PCIe controller could be made available. The Tegra2 TRM doesn't contain any of the AFI or PADS registers and while the Tegra3 TRM documents more registers, their offsets are completely missing.

I've filed a bug to request better documentation.

Looking at our downstream driver, the quoted comment below implies that the address space layout is pretty arbitrary and programmable at least for Tegra30. The code in function tegra_pcie_setup_translations() appears to support this (and is executed for both Tegra20 and Tegra30, even though the comment for Tegra20 doesn't mention any programmability).

Tegra20:

Quoted 7 lines

/*
* Tegra2 defines 1GB in the AXI address map for PCIe.
*
* That address space is split into different regions, with sizes and
* offsets as follows:
*
* 0x80000000 - 0x80003fff - PCI controller registers

Quoted 8 lines

* 0x80004000 - 0x80103fff - PCI configuration space
* 0x80104000 - 0x80203fff - PCI extended configuration space
* 0x80203fff - 0x803fffff - unused
* 0x80400000 - 0x8040ffff - downstream IO
* 0x80410000 - 0x8fffffff - unused
* 0x90000000 - 0x9fffffff - non-prefetchable memory
* 0xa0000000 - 0xbfffffff - prefetchable memory
*/

(I guess by "is split", it means the driver sets it up that way, not that the HW is fixed that way)

Tegra30:

Quoted 21 lines

/*
* AXI address map for the PCIe aperture , defines 1GB in the AXI
* address map for PCIe.
*

 *  That address space is split into different regions, with sizes and
 *  offsets as follows. Exepct for the Register space, SW is free to slice the
 *  regions as it chooces.
 *
 *  The split below seems to work fine for now.
 *
 *  0x0000_0000 to 0x00ff_ffff - Register space          16MB.
 *  0x0100_0000 to 0x01ff_ffff - Config space            16MB.
 *  0x0200_0000 to 0x02ff_ffff - Extended config space   16MB.
 *  0x0300_0000 to 0x03ff_ffff - Downstream IO space
 *   ... Will be filled with other BARS like MSI/upstream IO etc.
 *  0x1000_0000 to 0x1fff_ffff - non-prefetchable memory aperture
 *  0x2000_0000 to 0x3fff_ffff - Prefetchable memory aperture
 *
 *  Config and Extended config sizes are choosen to support
 *  maximum of 256 devices,
 *  which is good enough for all the current use cases.

Quoted 2 lines

*
*/

From
http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=blob;f=arch/arm/mach-tegra/pcie.c;h=b98d4892b5ee46120a9777a969e820a0b5cbb68a;hb=android-tegra-nv-3.1

This commit adds a platform device driver for the PCIe controller on Tegra SOCs. Current users of the old code (TrimSlice and Harmony) are converted and now initialize and register a corresponding platform device.

arch/arm/mach-tegra/pcie.c +0 -1

-static int tegra_pcie_clocks_get(void)

arch/arm/mach-tegra/pcie.c +2 -3

-	tegra_pcie.regs = ioremap_nocache(TEGRA_PCIE_BASE, PCIE_IOMAP_SZ);
-	if (tegra_pcie.regs == NULL) {
-		pr_err("PCIE: Failed to map PCI/AFI registers\n");
+	regs = request_mem_region(regs->start, resource_size(regs), "PCI/AFI");
+	if (regs == NULL) {

Quoted 10 lines · Thierry Reding

This commit adds a platform device driver for the PCIe controller on Tegra SOCs. Current users of the old code (TrimSlice and Harmony) are converted and now initialize and register a corresponding platform device.

arch/arm/mach-tegra/pcie.c +1 -1

-static int tegra_pcie_clocks_get(void)
+static int tegra_pcie_clocks_get(struct tegra_pcie_info *pcie)
 {
 	int err;

arch/arm/mach-tegra/pcie.c static int tegra_pcie_enable_controller(void) · line 712

Quoted 6 lines

arch/arm/mach-tegra/pcie.c +3 -3

-	tegra_pcie.pex_clk = clk_get(NULL, "pex");
-	if (IS_ERR(tegra_pcie.pex_clk))
-		return PTR_ERR(tegra_pcie.pex_clk);
+	pcie->pex_clk = clk_get(NULL, "pex");
+	if (IS_ERR(pcie->pex_clk))
+		return PTR_ERR(pcie->pex_clk);

While we're changing this, can we convert this to devm_clk_get(), and ...

Quoted 10 lines

arch/arm/mach-tegra/pcie.c +7 -3

-	tegra_pcie.regs = ioremap_nocache(TEGRA_PCIE_BASE, PCIE_IOMAP_SZ);
-	if (tegra_pcie.regs == NULL) {
-		pr_err("PCIE: Failed to map PCI/AFI registers\n");
+	regs = request_mem_region(regs->start, resource_size(regs), "PCI/AFI");
+	if (regs == NULL) {
+		dev_err(&pdev->dev, "failed to request PCI/AFI region: %d\n", err);
+		goto err_req_reg;
+	}
+
+	pcie->regs = ioremap_nocache(regs->start, resource_size(regs));

arch/arm/mach-tegra/pcie.c static int tegra_pcie_enable_controller(void) · line 790

Quoted 5 lines

arch/arm/mach-tegra/pcie.c +2 -0

+	if (pcie->regs == NULL) {
+		dev_err(&pdev->dev, "failed to map PCI/AFI registers\n");
 		err = -ENOMEM;
 		goto err_map_reg;
 	}

... that to devm_request_and_ioremap(). That would allow a bunch of the cleanup code to be deleted rather than just modified.

This commit adds support for message signaled interrupts to the Tegra PCIe controller. Based on code by Krishna Kishore <[email protected]>.

arch/arm/mach-tegra/pcie.c +1 -0

 
+static irqreturn_t tegra_pcie_msi_irq(int irq, void *data)

+static int tegra_pcie_disable_msi(struct platform_device *pdev)

Quoted 6 lines · Thierry Reding

This commit adds support for message signaled interrupts to the Tegra PCIe controller. Based on code by Krishna Kishore <[email protected]>.

arch/arm/mach-tegra/pcie.c +1 -0

 
+static irqreturn_t tegra_pcie_msi_irq(int irq, void *data)

...

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1018

Quoted 4 lines

arch/arm/mach-tegra/pcie.c +4 -0

+			irq = irq_find_mapping(pcie->msi->domain, index);
+			if (irq) {
+				if (test_bit(index, pcie->msi->used))
+					generic_handle_irq(irq);

This invokes the handler first ...

...

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1032

Quoted 4 lines

arch/arm/mach-tegra/pcie.c +4 -0

+			/* clear the interrupt */
+			afi_writel(pcie, 1 << offset, AFI_MSI_VEC0 + i * 4);
+			/* see if there's any more pending in this vector */
+			reg = afi_readl(pcie, AFI_MSI_VEC0 + i * 4);

... then clears the interrupt status in the PCIe controller. Won't that lose interrupts if one is raised between when the handler clears the root-cause, and when this code clears the received interrupt status?

Quoted 1 line

+static int tegra_pcie_disable_msi(struct platform_device *pdev)

Should this free pcie->msi->pages?

Why allocate pcie->msi separately; why not include the fields directly into struct tegra_pcie_info *pcie?

Quoted 4 lines · Stephen Warren

Quoted 3 lines · Thierry Reding

This commit adds support for message signaled interrupts to the Tegra PCIe controller. Based on code by Krishna Kishore <[email protected]>.

Quoted 2 lines

Why allocate pcie->msi separately; why not include the fields directly into struct tegra_pcie_info *pcie?

Second, and more importantly, this will keep the size of struct tegra_pcie_info smaller if PCI_MSI is not selected because there is just one unused pointer instead of five unused fields.

Quoted 5 lines · Thierry Reding

Quoted 4 lines · Stephen Warren

Quoted 3 lines · Thierry Reding

This commit adds support for message signaled interrupts to the Tegra PCIe controller. Based on code by Krishna Kishore <[email protected]>.

...

Quoted 2 lines

Quoted 2 lines

Why allocate pcie->msi separately; why not include the fields directly into struct tegra_pcie_info *pcie?

...

Quoted 3 lines

Second, and more importantly, this will keep the size of struct tegra_pcie_info smaller if PCI_MSI is not selected because there is just one unused pointer instead of five unused fields.

Well, you can always ifdef out the structure fields too, right?

Quoted 1 line · Stephen Warren

On 06/11/2012 09:05 AM, Thierry Reding wrote:

Quoted 3 lines

Quoted 1 line

+static int tegra_pcie_disable_msi(struct platform_device *pdev)

Should this free pcie->msi->pages?

Yes it should. I actually mention making that change in the changelog but in fact didn't.

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/11/2012 09:05 AM, Thierry Reding wrote:

[...]

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

+static int tegra_pcie_disable_msi(struct platform_device *pdev)

Should this free pcie->msi->pages?

Yes it should. I actually mention making that change in the changelog but in fact didn't.

This is really moot because the driver cannot be built as a module currently because arch_setup_msi_irq() and arch_teardown_msi_irq() need to be built-in so tegra_pcie_disable_msi() will be called only just before the machine is shut down.

Perhaps this is something that should be addressed? Or is it just not worth the effort?

Quoted 17 lines · Thierry Reding

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/11/2012 09:05 AM, Thierry Reding wrote:

[...]

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

+static int tegra_pcie_disable_msi(struct platform_device *pdev)

Should this free pcie->msi->pages?

Yes it should. I actually mention making that change in the changelog but in fact didn't.

This is really moot because the driver cannot be built as a module currently because arch_setup_msi_irq() and arch_teardown_msi_irq() need to be built-in so tegra_pcie_disable_msi() will be called only just before the machine is shut down.

Quoted 2 lines

Perhaps this is something that should be addressed? Or is it just not worth the effort?

Well, if the driver provides a shutdown/removal path at all, it seems like it should be complete. I assume it's trivial to free those pages?

This commit adds support for instantiating the Tegra PCIe controller from a device tree.

+++ b/Documentation/devicetree/bindings/pci/tegra-pcie.txt

+Required properties:

+- compatible: "nvidia,tegra20-pcie"
+- reg: physical base address and length of the controller's registers

+Optional properties:

+- pex-clk-supply: supply voltage for internal reference clock
+- vdd-supply: power supply for controller (1.05V)

arch/arm/boot/dts/tegra20.dtsi +1 -0

 
+	pci {

Quoted 4 lines · Thierry Reding

This commit adds support for instantiating the Tegra PCIe controller from a device tree.

+++ b/Documentation/devicetree/bindings/pci/tegra-pcie.txt

Can we please name this nvidia,tegra20-pcie.txt to match the naming of all the other Tegra bindings.

Quoted 3 lines

+Required properties:

+- compatible: "nvidia,tegra20-pcie"
+- reg: physical base address and length of the controller's registers

Since there's more than one range now, that should specify how many entries are required and what they represent.

Quoted 3 lines

+Optional properties:

+- pex-clk-supply: supply voltage for internal reference clock
+- vdd-supply: power supply for controller (1.05V)

Those shouldn't be optional. If the board has no regulator, the board's .dts should provide a fixed always-on regulator that those properties can refer to, so that the driver can always get() those regulators.

Quoted 3 lines

arch/arm/boot/dts/tegra20.dtsi +1 -0

 
+	pci {

...

arch/arm/boot/dts/tegra20.dtsi line 208

Quoted 1 line

arch/arm/boot/dts/tegra20.dtsi +1 -0

+		status = "disable";

That should be "disabled"; sorry for providing a bad example.

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1270

Quoted 5 lines

arch/arm/mach-tegra/pcie.c +2 -0

 
+static struct tegra_pcie_pdata *tegra_pcie_parse_dt(struct platform_device *pdev)
 
+	if (of_find_property(node, "vdd-supply", NULL)) {

As mentioned above, that if statement should be removed, since the regulators shouldn't be optional.

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1271

Quoted 1 line

arch/arm/mach-tegra/pcie.c +1 -0

+		pcie->vdd_supply = regulator_get(&pdev->dev, "vdd");

Those could be devm_regulator_get(). Then tegra_pcie_remove() wouldn't have to put() them.

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1283

Quoted 2 lines

arch/arm/mach-tegra/pcie.c +2 -0

+	for (i = 0; i < TEGRA_PCIE_MAX_PORTS; i++)
+		pdata->enable_ports[i] = true;

Shouldn't the DT indicate which ports are used? I assume there's some reason that the existing driver allows that to be configured, rather than always enabling all ports. At least, enumeration time wasted on non-existent ports springs to mind, and perhaps attempting to enable port 1 when port 0 is x4 and using all the lanes would cause errors in port 0?

Quoted 8 lines · Stephen Warren

Quoted 4 lines · Thierry Reding

This commit adds support for instantiating the Tegra PCIe controller from a device tree.

+++ b/Documentation/devicetree/bindings/pci/tegra-pcie.txt

Can we please name this nvidia,tegra20-pcie.txt to match the naming of all the other Tegra bindings.

Yes, will do.

Quoted 6 lines

Quoted 3 lines

+Required properties: +- compatible: "nvidia,tegra20-pcie" +-
reg: physical base address and length of the controller's registers

Since there's more than one range now, that should specify how many entries are required and what they represent.

Okay.

Quoted 8 lines

Quoted 3 lines

+Optional properties: +- pex-clk-supply: supply voltage for internal reference clock +- vdd-supply: power supply for controller (1.05V)

Those shouldn't be optional. If the board has no regulator, the board's .dts should provide a fixed always-on regulator that those properties can refer to, so that the driver can always get() those regulators.

That'll add more dummy regulators and I don't think sprinkling them across the DTS is going to work very well. Maybe collecting them under a top-level "regulators" node is a good option. If you have a better alternative, I'm all open for it.

Quoted 8 lines

Quoted 4 lines

arch/arm/boot/dts/tegra20.dtsi +1 -0

 
+	pci {

...

arch/arm/boot/dts/tegra20.dtsi line 208

Quoted 1 line

arch/arm/boot/dts/tegra20.dtsi +1 -0

+		status = "disable";

That should be "disabled"; sorry for providing a bad example.

Yes.

Quoted 10 lines

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1270

Quoted 7 lines

arch/arm/mach-tegra/pcie.c +1 -0

 
+static struct tegra_pcie_pdata *tegra_pcie_parse_dt(struct

platform_device *pdev)

arch/arm/mach-tegra/pcie.c +1 -0

+	if (of_find_property(node, "vdd-supply", NULL)) {

As mentioned above, that if statement should be removed, since the regulators shouldn't be optional.

Okay.

Quoted 4 lines

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1271

Quoted 1 line

arch/arm/mach-tegra/pcie.c +1 -0

+		pcie->vdd_supply = regulator_get(&pdev->dev, "vdd");

Those could be devm_regulator_get(). Then tegra_pcie_remove() wouldn't have to put() them.

Okay.

Quoted 9 lines

Quoted 2 lines

arch/arm/mach-tegra/pcie.c +1 -0

+	for (i = 0; i < TEGRA_PCIE_MAX_PORTS; i++) +

pdata->enable_ports[i] = true;

Shouldn't the DT indicate which ports are used? I assume there's some reason that the existing driver allows that to be configured, rather than always enabling all ports. At least, enumeration time wasted on non-existent ports springs to mind, and perhaps attempting to enable port 1 when port 0 is x4 and using all the lanes would cause errors in port 0?

Yes, that's been on my mind as well. I'm not sure about the best binding for this. Perhaps something like:

pci { enable-ports = <0 1 2>; };

Would do?

Quoted 79 lines · Thierry Reding

Quoted 8 lines · Stephen Warren

Quoted 4 lines · Thierry Reding

This commit adds support for instantiating the Tegra PCIe controller from a device tree.

+++ b/Documentation/devicetree/bindings/pci/tegra-pcie.txt

Can we please name this nvidia,tegra20-pcie.txt to match the naming of all the other Tegra bindings.

Yes, will do.

Quoted 6 lines

Quoted 3 lines

+Required properties: +- compatible: "nvidia,tegra20-pcie" +-
reg: physical base address and length of the controller's registers

Since there's more than one range now, that should specify how many entries are required and what they represent.

Okay.

Quoted 8 lines

Quoted 3 lines

+Optional properties: +- pex-clk-supply: supply voltage for internal reference clock +- vdd-supply: power supply for controller (1.05V)

Those shouldn't be optional. If the board has no regulator, the board's .dts should provide a fixed always-on regulator that those properties can refer to, so that the driver can always get() those regulators.

That'll add more dummy regulators and I don't think sprinkling them across the DTS is going to work very well. Maybe collecting them under a top-level "regulators" node is a good option. If you have a better alternative, I'm all open for it.

Quoted 8 lines

Quoted 4 lines

arch/arm/boot/dts/tegra20.dtsi +1 -0

 
+	pci {

...

arch/arm/boot/dts/tegra20.dtsi line 208

Quoted 1 line

arch/arm/boot/dts/tegra20.dtsi +1 -0

+		status = "disable";

That should be "disabled"; sorry for providing a bad example.

Yes.

Quoted 10 lines

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1270

Quoted 7 lines

arch/arm/mach-tegra/pcie.c +1 -0

 
+static struct tegra_pcie_pdata *tegra_pcie_parse_dt(struct

platform_device *pdev)

arch/arm/mach-tegra/pcie.c +1 -0

+	if (of_find_property(node, "vdd-supply", NULL)) {

As mentioned above, that if statement should be removed, since the regulators shouldn't be optional.

Okay.

Quoted 4 lines

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1271

Quoted 1 line

arch/arm/mach-tegra/pcie.c +1 -0

+		pcie->vdd_supply = regulator_get(&pdev->dev, "vdd");

Those could be devm_regulator_get(). Then tegra_pcie_remove() wouldn't have to put() them.

Okay.

Quoted 9 lines

Quoted 2 lines

arch/arm/mach-tegra/pcie.c +1 -0

+	for (i = 0; i < TEGRA_PCIE_MAX_PORTS; i++) +

pdata->enable_ports[i] = true;

Shouldn't the DT indicate which ports are used? I assume there's some reason that the existing driver allows that to be configured, rather than always enabling all ports. At least, enumeration time wasted on non-existent ports springs to mind, and perhaps attempting to enable port 1 when port 0 is x4 and using all the lanes would cause errors in port 0?

Yes, that's been on my mind as well. I'm not sure about the best binding for this. Perhaps something like:

Quoted 3 lines

pci { enable-ports = <0 1 2>; };

Would do?

That seems reasonable, although since the property is presumably something specific to the Tegra PCIe binding, not generic, I think it should be nvidia,enable-ports.

Quoted 80 lines · Stephen Warren

Quoted 79 lines · Thierry Reding

Quoted 8 lines · Stephen Warren

Quoted 4 lines · Thierry Reding

This commit adds support for instantiating the Tegra PCIe controller from a device tree.

+++ b/Documentation/devicetree/bindings/pci/tegra-pcie.txt

Can we please name this nvidia,tegra20-pcie.txt to match the naming of all the other Tegra bindings.

Yes, will do.

Quoted 6 lines

Quoted 3 lines

+Required properties: +- compatible: "nvidia,tegra20-pcie" +-
reg: physical base address and length of the controller's registers

Since there's more than one range now, that should specify how many entries are required and what they represent.

Okay.

Quoted 8 lines

Quoted 3 lines

+Optional properties: +- pex-clk-supply: supply voltage for internal reference clock +- vdd-supply: power supply for controller (1.05V)

Those shouldn't be optional. If the board has no regulator, the board's .dts should provide a fixed always-on regulator that those properties can refer to, so that the driver can always get() those regulators.

That'll add more dummy regulators and I don't think sprinkling them across the DTS is going to work very well. Maybe collecting them under a top-level "regulators" node is a good option. If you have a better alternative, I'm all open for it.

Quoted 8 lines

Quoted 4 lines

arch/arm/boot/dts/tegra20.dtsi +1 -0

 
+	pci {

...

arch/arm/boot/dts/tegra20.dtsi line 208

Quoted 1 line

arch/arm/boot/dts/tegra20.dtsi +1 -0

+		status = "disable";

That should be "disabled"; sorry for providing a bad example.

Yes.

Quoted 10 lines

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1270

Quoted 7 lines

arch/arm/mach-tegra/pcie.c +1 -0

 
+static struct tegra_pcie_pdata *tegra_pcie_parse_dt(struct

platform_device *pdev)

arch/arm/mach-tegra/pcie.c +1 -0

+	if (of_find_property(node, "vdd-supply", NULL)) {

As mentioned above, that if statement should be removed, since the regulators shouldn't be optional.

Okay.

Quoted 4 lines

arch/arm/mach-tegra/pcie.c static int tegra_pcie_disable_msi(struct platform_device *pdev) · line 1271

Quoted 1 line

arch/arm/mach-tegra/pcie.c +1 -0

+		pcie->vdd_supply = regulator_get(&pdev->dev, "vdd");

Those could be devm_regulator_get(). Then tegra_pcie_remove() wouldn't have to put() them.

Okay.

Quoted 9 lines

Quoted 2 lines

arch/arm/mach-tegra/pcie.c +1 -0

+	for (i = 0; i<  TEGRA_PCIE_MAX_PORTS; i++) +

pdata->enable_ports[i] = true;

Shouldn't the DT indicate which ports are used? I assume there's some reason that the existing driver allows that to be configured, rather than always enabling all ports. At least, enumeration time wasted on non-existent ports springs to mind, and perhaps attempting to enable port 1 when port 0 is x4 and using all the lanes would cause errors in port 0?

Yes, that's been on my mind as well. I'm not sure about the best binding for this. Perhaps something like:

Quoted 7 lines

Quoted 3 lines

pci { enable-ports =<0 1 2>; };

Would do?

That seems reasonable, although since the property is presumably something specific to the Tegra PCIe binding, not generic, I think it should be nvidia,enable-ports.

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

Thierry

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

On 6/12/2012 7:20 AM, Thierry Reding wrote:

Quoted 41 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts = <0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <1>;

		port at 80000000 {
			reg = <0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg = <0x80001000 0x00001000>;
			status = "disabled";
		};
	};

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

Quoted 1 line

What do you think of this?

The problem is that children of a PCI-ish bus have specific expectations about the parent address format - the standard 3-address-cell PCI addressing. So making a PCI bus node - even if it is PCIe - with 1 address cell is a problem.

Quoted 1 line · Mitch Bradley

On 6/12/2012 7:20 AM, Thierry Reding wrote:

...

Quoted 41 lines

Quoted 41 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

Quoted 6 lines

Quoted 1 line

What do you think of this?

The problem is that children of a PCI-ish bus have specific expectations about the parent address format - the standard 3-address-cell PCI addressing. So making a PCI bus node - even if it is PCIe - with 1 address cell is a problem.

Couldn't you put the #address-cells=<3> underneath each port node, and put any PCIe devices there rather than under the main PCIe controller node? I'm not sure how the interrupt mapping table etc. would work in that case, but that seems to solve the addressing issue.

Quoted 51 lines · Stephen Warren

Quoted 1 line · Mitch Bradley

On 6/12/2012 7:20 AM, Thierry Reding wrote:

...

Quoted 41 lines

Quoted 41 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

Quoted 6 lines

Quoted 1 line

What do you think of this?

The problem is that children of a PCI-ish bus have specific expectations about the parent address format - the standard 3-address-cell PCI addressing. So making a PCI bus node - even if it is PCIe - with 1 address cell is a problem.

Quoted 4 lines

Couldn't you put the #address-cells=<3> underneath each port node, and put any PCIe devices there rather than under the main PCIe controller node? I'm not sure how the interrupt mapping table etc. would work in that case, but that seems to solve the addressing issue.

Yes, that would work just fine, and in fact would be preferable, if a "root port" is essentially an independent PCI bus. The parent of those root ports should not be named "pci", though, as it is not in itself a PCI bus. It could be called "pcis".

Quoted 57 lines · Mitch Bradley

Quoted 55 lines · Stephen Warren

Quoted 1 line · Mitch Bradley

On 6/12/2012 7:20 AM, Thierry Reding wrote:

...

Quoted 41 lines

Quoted 41 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

Quoted 6 lines

Quoted 1 line

What do you think of this?

The problem is that children of a PCI-ish bus have specific expectations about the parent address format - the standard 3-address-cell PCI addressing. So making a PCI bus node - even if it is PCIe - with 1 address cell is a problem.

Couldn't you put the #address-cells=<3> underneath each port node, and
put any PCIe devices there rather than under the main PCIe controller node? I'm not sure how the interrupt mapping table etc. would work in that case, but that seems to solve the addressing issue.

Quoted 4 lines

Yes, that would work just fine, and in fact would be preferable, if a "root port" is essentially an independent PCI bus. The parent of those root ports should not be named "pci", though, as it is not in itself a PCI bus. It could be called "pcis".

Yes, on Tegra each of the ports provides a separate PCI bus. How about naming the parent "pcie-controller"?

IMO that reflects the nature of the device
better, whereas "pcis" would seem more like a "virtual" node for collecting similar children.

Thierry

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts = <0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <1>;

		port at 80000000 {
			reg = <0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg = <0x80001000 0x00001000>;
			status = "disabled";
		};
	};

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

        int rp_offset = 0;
        int ctrl_offset = AFI_PEX0_CTRL;

        for (port = 0; port < MAX_PCIE_SUPPORTED_PORTS; port++) {
                ctrl_offset += (port * 8);
                rp_offset = (rp_offset + 0x1000) * port;
                if (tegra_pcie.plat_data->port_status[port])
                        tegra_pcie_add_port(port, rp_offset, ctrl_offset);
        }

On 06/12/2012 11:20 AM, Thierry Reding wrote:
...

Quoted 36 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

Quoted 6 lines

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

As a general concept, this kind of design seems OK to me.

The "port" child nodes I think should be named "pci at ..." given Mitch's comments, I think.

The port nodes probably need two entries in reg, given the following in our downstream driver:

Quoted 2 lines

         int rp_offset = 0;
         int ctrl_offset = AFI_PEX0_CTRL;

...

Quoted 6 lines

         for (port = 0; port<  MAX_PCIE_SUPPORTED_PORTS; port++) {
                 ctrl_offset += (port * 8);
                 rp_offset = (rp_offset + 0x1000) * port;
                 if (tegra_pcie.plat_data->port_status[port])
                         tegra_pcie_add_port(port, rp_offset, ctrl_offset);
         }

(which actually looks likely to be horribly buggy for port>1 and only accidentally correct for port==1, but anyway...)

But instead, I'd be tempted to make the top-level node say:

	#address-cells =<1>;
	#size-cells =<0>;

... so that the child nodes' reg is just the port ID. The parent node can calculate the addresses/offsets of the per-port registers within the PCIe controller's register space based on the ID using code roughly like what I quoted above:

	pci at 0 {
		reg =<0>;
		status = "disabled";
	};

	pci at 1 {
		reg =<0>;
		status = "disabled";
	};

That would save having to put 2 entries in the reg, and perhaps remove the need for any ranges property.

I think you also need a property to specify the exact port layout; the Tegra20 controller supports:

1 x4 port
2 x2 ports (you can choose to use only 1 of these I assume)

So just because only 1 of the ports is enabled, doesn't imply it's x4; it could still be x2.

Tegra30 has more options.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Quoted 76 lines · Stephen Warren

On 06/12/2012 11:20 AM, Thierry Reding wrote:
...

Quoted 36 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

Quoted 6 lines

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

As a general concept, this kind of design seems OK to me.

The "port" child nodes I think should be named "pci at ..." given Mitch's comments, I think.

The port nodes probably need two entries in reg, given the following in our downstream driver:

Quoted 2 lines

        int rp_offset = 0;
        int ctrl_offset = AFI_PEX0_CTRL;

...

Quoted 6 lines

        for (port = 0; port<  MAX_PCIE_SUPPORTED_PORTS; port++) {
                ctrl_offset += (port * 8);
                rp_offset = (rp_offset + 0x1000) * port;
                if (tegra_pcie.plat_data->port_status[port])
                        tegra_pcie_add_port(port, rp_offset, ctrl_offset);
        }

(which actually looks likely to be horribly buggy for port>1 and only accidentally correct for port==1, but anyway...)

But instead, I'd be tempted to make the top-level node say:

	#address-cells =<1>;
	#size-cells =<0>;

... so that the child nodes' reg is just the port ID. The parent node can calculate the addresses/offsets of the per-port registers within the PCIe controller's register space based on the ID using code roughly like what I quoted above:

Quoted 9 lines

	pci at 0 {
		reg =<0>;
		status = "disabled";
	};

	pci at 1 {
		reg =<0>;
		status = "disabled";
	};

reg = <1> ?

Quoted 2 lines

Quoted 2 lines

That would save having to put 2 entries in the reg, and perhaps remove the need for any ranges property.

ISTM that having two reg entries, specifying the rp and ctrl registers, is preferable to having code to calculate the addresses. That makes the code simpler and the device tree more directly descriptive of the hardware layout. The less "magic" (in this case, the register address calculation), the better.

Quoted 90 lines · Mitch Bradley

Quoted 76 lines · Stephen Warren

On 06/12/2012 11:20 AM, Thierry Reding wrote:
...

Quoted 36 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

Quoted 6 lines

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

As a general concept, this kind of design seems OK to me.

The "port" child nodes I think should be named "pci at ..." given Mitch's comments, I think.

The port nodes probably need two entries in reg, given the following in our downstream driver:

Quoted 2 lines

         int rp_offset = 0;
         int ctrl_offset = AFI_PEX0_CTRL;

...

Quoted 6 lines

         for (port = 0; port<   MAX_PCIE_SUPPORTED_PORTS; port++) {
                 ctrl_offset += (port * 8);
                 rp_offset = (rp_offset + 0x1000) * port;
                 if (tegra_pcie.plat_data->port_status[port])
                         tegra_pcie_add_port(port, rp_offset, ctrl_offset);
         }

(which actually looks likely to be horribly buggy for port>1 and only accidentally correct for port==1, but anyway...)

But instead, I'd be tempted to make the top-level node say:

	#address-cells =<1>;
	#size-cells =<0>;

... so that the child nodes' reg is just the port ID. The parent node can calculate the addresses/offsets of the per-port registers within the PCIe controller's register space based on the ID using code roughly like what I quoted above:

Quoted 9 lines

	pci at 0 {
		reg =<0>;
		status = "disabled";
	};

	pci at 1 {
		reg =<0>;
		status = "disabled";
	};

reg =<1> ?

Quoted 2 lines

Quoted 8 lines

Quoted 2 lines

That would save having to put 2 entries in the reg, and perhaps remove the need for any ranges property.

ISTM that having two reg entries, specifying the rp and ctrl registers, is preferable to having code to calculate the addresses. That makes the code simpler and the device tree more directly descriptive of the hardware layout. The less "magic" (in this case, the register address calculation), the better.

The problem with this approach is that since the control registers are within the AFI register range, both the reg and ranges properties of the parent would have to include these holes. Furthermore it means that the controller driver would have to remap the AFI registers in chunks, for the sole reason of splitting out the controle registers.

Would it be acceptable to make an exception in this case and use the port's second reg entry as an offset into the AFI register range instead?

Thierry

Quoted 100 lines · Thierry Reding

Quoted 90 lines · Mitch Bradley

Quoted 76 lines · Stephen Warren

On 06/12/2012 11:20 AM, Thierry Reding wrote:
...

Quoted 36 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

Quoted 6 lines

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

As a general concept, this kind of design seems OK to me.

The "port" child nodes I think should be named "pci at ..." given Mitch's comments, I think.

The port nodes probably need two entries in reg, given the following in our downstream driver:

Quoted 2 lines

        int rp_offset = 0;
        int ctrl_offset = AFI_PEX0_CTRL;

...

Quoted 6 lines

        for (port = 0; port<   MAX_PCIE_SUPPORTED_PORTS; port++) {
                ctrl_offset += (port * 8);
                rp_offset = (rp_offset + 0x1000) * port;
                if (tegra_pcie.plat_data->port_status[port])
                        tegra_pcie_add_port(port, rp_offset, ctrl_offset);
        }

(which actually looks likely to be horribly buggy for port>1 and only accidentally correct for port==1, but anyway...)

But instead, I'd be tempted to make the top-level node say:

	#address-cells =<1>;
	#size-cells =<0>;

... so that the child nodes' reg is just the port ID. The parent node can calculate the addresses/offsets of the per-port registers within the PCIe controller's register space based on the ID using code roughly like what I quoted above:

Quoted 9 lines

	pci at 0 {
		reg =<0>;
		status = "disabled";
	};

	pci at 1 {
		reg =<0>;
		status = "disabled";
	};

reg =<1> ?

Quoted 2 lines

Quoted 8 lines

Quoted 2 lines

That would save having to put 2 entries in the reg, and perhaps remove the need for any ranges property.

ISTM that having two reg entries, specifying the rp and ctrl registers, is preferable to having code to calculate the addresses. That makes the code simpler and the device tree more directly descriptive of the hardware layout. The less "magic" (in this case, the register address calculation), the better.

Quoted 8 lines

The problem with this approach is that since the control registers are within the AFI register range, both the reg and ranges properties of the parent would have to include these holes. Furthermore it means that the controller driver would have to remap the AFI registers in chunks, for the sole reason of splitting out the controle registers.

Would it be acceptable to make an exception in this case and use the port's second reg entry as an offset into the AFI register range instead?

How about this:

    pcie-controller {
        compatible = "nvidia,tegra20-pcie";
        reg =<0x80003000 0x00000800   /* PADS registers */
              0x80003800 0x00000200>   /* AFI registers */
        interrupts =<0 98 0x04   /* controller interrupt */
                     0 99 0x04>; /* MSI interrupt */
        status = "disabled";

        ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
              0x80004000 0x80004000 0x00100000   /* configuration space */
              0x80104000 0x80104000 0x00100000   /* extended
configuration space */
              0x80400000 0x80400000 0x00010000   /* downstream I/O */
              0x90000000 0x90000000 0x10000000   /* non-prefetchable
memory */
              0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

        #address-cells =<1>;
        #size-cells =<1>;

        pci at 80000000 {
            reg =<0x80000000 0x00001000>;
            ctrl-offset =<0x0>;
            status = "disabled";
            #address-cells = <3>;  /* Standard for PCI */
            #size-cells =<2>;      /* Standard for PCI */
            ranges =</* Map from standard PCI child address spaces
to linear spaces above */>;
        };

        pci at 80001000 {
            reg =<0x80001000 0x00001000>;
            ctrl-offset =<0x8>;
            status = "disabled";
            #address-cells = <3>;  /* Standard for PCI */
            #size-cells =<2>;      /* Standard for PCI */
            ranges =</* Map from standard PCI child address spaces
to linear spaces above */>;
        };
    };

Using ctrl-offset instead of reg avoids any violation of the usual reg semantics.

Note that the pci subnodes need the full complement of PCI bus node properties.

Quoted 3 lines

Thierry

Quoted 112 lines · Mitch Bradley

Quoted 100 lines · Thierry Reding

Quoted 90 lines · Mitch Bradley

Quoted 76 lines · Stephen Warren

On 06/12/2012 11:20 AM, Thierry Reding wrote:
...

Quoted 36 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts =<0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		port at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";
		};
	};

Quoted 6 lines

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

As a general concept, this kind of design seems OK to me.

The "port" child nodes I think should be named "pci at ..." given Mitch's comments, I think.

The port nodes probably need two entries in reg, given the following in our downstream driver:

Quoted 2 lines

         int rp_offset = 0;
         int ctrl_offset = AFI_PEX0_CTRL;

...

Quoted 6 lines

         for (port = 0; port<    MAX_PCIE_SUPPORTED_PORTS; port++) {
                 ctrl_offset += (port * 8);
                 rp_offset = (rp_offset + 0x1000) * port;
                 if (tegra_pcie.plat_data->port_status[port])
                         tegra_pcie_add_port(port, rp_offset, ctrl_offset);
         }

(which actually looks likely to be horribly buggy for port>1 and only accidentally correct for port==1, but anyway...)

But instead, I'd be tempted to make the top-level node say:

	#address-cells =<1>;
	#size-cells =<0>;

... so that the child nodes' reg is just the port ID. The parent node can calculate the addresses/offsets of the per-port registers within the PCIe controller's register space based on the ID using code roughly like what I quoted above:

Quoted 9 lines

	pci at 0 {
		reg =<0>;
		status = "disabled";
	};

	pci at 1 {
		reg =<0>;
		status = "disabled";
	};

reg =<1> ?

Quoted 2 lines

Quoted 8 lines

Quoted 2 lines

That would save having to put 2 entries in the reg, and perhaps remove the need for any ranges property.

ISTM that having two reg entries, specifying the rp and ctrl registers, is preferable to having code to calculate the addresses. That makes the code simpler and the device tree more directly descriptive of the hardware layout. The less "magic" (in this case, the register address calculation), the better.

Quoted 8 lines

The problem with this approach is that since the control registers are within the AFI register range, both the reg and ranges properties of the parent would have to include these holes. Furthermore it means that the controller driver would have to remap the AFI registers in chunks, for the sole reason of splitting out the controle registers.

Would it be acceptable to make an exception in this case and use the port's second reg entry as an offset into the AFI register range instead?

How about this:

Quoted 16 lines

     pcie-controller {
         compatible = "nvidia,tegra20-pcie";
         reg =<0x80003000 0x00000800   /* PADS registers */
               0x80003800 0x00000200>    /* AFI registers */
         interrupts =<0 98 0x04   /* controller interrupt */
                      0 99 0x04>; /* MSI interrupt */
         status = "disabled";

         ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
               0x80004000 0x80004000 0x00100000   /* configuration space */
               0x80104000 0x80104000 0x00100000   /* extended
configuration space */
               0x80400000 0x80400000 0x00010000   /* downstream I/O */
               0x90000000 0x90000000 0x10000000   /* non-prefetchable
memory */
               0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

I think the configuration spaces and downstream I/O ranges need to be in the pcie-controller's reg property because they are remapped and used by the controller driver, not by the individual ports.

That's probably not really necessary but rather a result of how the driver was written. Perhaps the driver should handle them differently instead, listing the regions in the ranges property of the parent and listing the corresponding partitions in the ranges properties of the pci child nodes.

Like in the following, where the ranges property of the ports partition the ranges passed from the parent evenly:

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* AFI registers */
		interrupts =<0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80100000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		pci at 80000000 {
			reg =<0x80000000 0x00001000>;

			status = "disabled";

			#address-cells =<3>;
			#size-cells =<2>;

			ranges =<0x80400000 0x80400000 0x00008000   /* I/O */
				  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
				  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset =<0x0>;
			nvidia,num-lanes =<2>;
		};

		pci at 80001000 {
			reg =<0x80001000 0x00001000>;
			status = "disabled";

			#address-cells =<3>;
			#size-cells =<2>;

			ranges =<0x80408000 0x80408000 0x00008000   /* I/O */
				  0x98000000 0x98000000 0x08000000   /* non-prefetchable memory */
				  0xa8000000 0xa8000000 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset =<0x8>;
			nvidia,num-lanes =<2>;
		};
	};

I've also added the new num-lanes property that describes the physical connections of the port. Also the ctrl-offset and num-lanes property are pretty specific to the Tegra PCIe controller, so I've prefixed them with "nvidia,".

Quoted 25 lines

         #address-cells =<1>;
         #size-cells =<1>;

         pci at 80000000 {
             reg =<0x80000000 0x00001000>;
             ctrl-offset =<0x0>;
             status = "disabled";
             #address-cells =<3>;  /* Standard for PCI */
             #size-cells =<2>;      /* Standard for PCI */
             ranges =</* Map from standard PCI child address spaces
to linear spaces above */>;
         };

         pci at 80001000 {
             reg =<0x80001000 0x00001000>;
             ctrl-offset =<0x8>;
             status = "disabled";
             #address-cells =<3>;  /* Standard for PCI */
             #size-cells =<2>;      /* Standard for PCI */
             ranges =</* Map from standard PCI child address spaces
to linear spaces above */>;
         };
     };

Quoted 2 lines

Using ctrl-offset instead of reg avoids any violation of the usual reg semantics.

Yes, that sounds better.

Quoted 8 lines

Note that the pci subnodes need the full complement of PCI bus node properties.

Quoted 3 lines

Thierry

Quoted 32 lines · Thierry Reding

I think the configuration spaces and downstream I/O ranges need to be in the pcie-controller's reg property because they are remapped and used by the controller driver, not by the individual ports.

That's probably not really necessary but rather a result of how the driver was written. Perhaps the driver should handle them differently instead, listing the regions in the ranges property of the parent and listing the corresponding partitions in the ranges properties of the pci child nodes.

Like in the following, where the ranges property of the ports partition the ranges passed from the parent evenly:

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* AFI registers */
		interrupts =<0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80100000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		pci at 80000000 {
			reg =<0x80000000 0x00001000>;

Quoted 8 lines

			status = "disabled";

			#address-cells =<3>;
			#size-cells =<2>;

			ranges =<0x80400000 0x80400000 0x00008000   /* I/O */
				  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
				  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

You are on the right track here, but the format of the child-address portion of the above ranges property is incorrect. Since the child address space is the PCI address space, the child-address portion needs to be 3 cells. It's not a linear address but rather a triple. The first cell identifies the address type (config, I/O, memory..) and the second and third cells are offsets within that subspace. The second and third cells will typically be 0. The PCI binding has details.

Quoted 42 lines · Mitch Bradley

Quoted 40 lines · Thierry Reding

I think the configuration spaces and downstream I/O ranges need to be in the pcie-controller's reg property because they are remapped and used by the controller driver, not by the individual ports.

That's probably not really necessary but rather a result of how the driver was written. Perhaps the driver should handle them differently instead, listing the regions in the ranges property of the parent and listing the corresponding partitions in the ranges properties of the pci child nodes.

Like in the following, where the ranges property of the ports partition the ranges passed from the parent evenly:

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* AFI registers */
		interrupts =<0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80100000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		pci at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";

			#address-cells =<3>;
			#size-cells =<2>;

			ranges =<0x80400000 0x80400000 0x00008000   /* I/O */
				  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
				  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

Quoted 8 lines

You are on the right track here, but the format of the child-address portion of the above ranges property is incorrect. Since the child address space is the PCI address space, the child-address portion needs to be 3 cells. It's not a linear address but rather a triple. The first cell identifies the address type (config, I/O, memory..) and the second and third cells are offsets within that subspace. The second and third cells will typically be 0. The PCI binding has details.

Quoted 1 line

Okay, I'll need to read up some more.

Thanks,
Thierry

Quoted 43 lines · Thierry Reding

Quoted 42 lines · Mitch Bradley

Quoted 40 lines · Thierry Reding

I think the configuration spaces and downstream I/O ranges need to be in the pcie-controller's reg property because they are remapped and used by the controller driver, not by the individual ports.

That's probably not really necessary but rather a result of how the driver was written. Perhaps the driver should handle them differently instead, listing the regions in the ranges property of the parent and listing the corresponding partitions in the ranges properties of the pci child nodes.

Like in the following, where the ranges property of the ports partition the ranges passed from the parent evenly:

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* AFI registers */
		interrupts =<0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80100000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		pci at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";

			#address-cells =<3>;
			#size-cells =<2>;

			ranges =<0x80400000 0x80400000 0x00008000   /* I/O */
				  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
				  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

Quoted 8 lines

Quoted 8 lines

You are on the right track here, but the format of the child-address portion of the above ranges property is incorrect. Since the child address space is the PCI address space, the child-address portion needs to be 3 cells. It's not a linear address but rather a triple. The first cell identifies the address type (config, I/O, memory..) and the second and third cells are offsets within that subspace. The second and third cells will typically be 0. The PCI binding has details.

Also, the size field in ranges is specified according to the child address space, so there must be 2 size cells in the ranges at this level. Each ranges entry at this level is:

<child_address_space, child_address_high, child_address_low, parent_address, child_size_high, child_size_low>

The above should be:

			ranges =<0x81000000 0 0  0x80400000  0 0x00008000   /* I/O */
				 0x82000000 0 0  0x90000000  0 0x08000000   /* non-prefetchable memory */
				 0xc2000000 0 0  0xa0000000  0 0x08000000>; /* prefetchable memory */

Quoted 43 lines · Thierry Reding

Quoted 42 lines · Mitch Bradley

Quoted 40 lines · Thierry Reding

I think the configuration spaces and downstream I/O ranges need to be in the pcie-controller's reg property because they are remapped and used by the controller driver, not by the individual ports.

That's probably not really necessary but rather a result of how the driver was written. Perhaps the driver should handle them differently instead, listing the regions in the ranges property of the parent and listing the corresponding partitions in the ranges properties of the pci child nodes.

Like in the following, where the ranges property of the ports partition the ranges passed from the parent evenly:

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg =<0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* AFI registers */
		interrupts =<0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges =<0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80100000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells =<1>;
		#size-cells =<1>;

		pci at 80000000 {
			reg =<0x80000000 0x00001000>;
			status = "disabled";

			#address-cells =<3>;
			#size-cells =<2>;

			ranges =<0x80400000 0x80400000 0x00008000   /* I/O */
				  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
				  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

Quoted 8 lines

Quoted 8 lines

You are on the right track here, but the format of the child-address portion of the above ranges property is incorrect. Since the child address space is the PCI address space, the child-address portion needs to be 3 cells. It's not a linear address but rather a triple. The first cell identifies the address type (config, I/O, memory..) and the second and third cells are offsets within that subspace. The second and third cells will typically be 0. The PCI binding has details.

Also, the size field in ranges is specified according to the child address space, so there must be 2 size cells in the ranges at this level. Each ranges entry at this level is:

<child_address_space, child_address_high, child_address_low, parent_address, child_size_high, child_size_low>

The above should be:

			ranges =<0x81000000 0 0  0x80400000  0 0x00008000   /* I/O */
				 0x82000000 0 0  0x90000000  0 0x08000000   /* non-prefetchable memory */
				 0xc2000000 0 0  0xa0000000  0 0x08000000>; /* prefetchable memory */

Okay, so the new pcie-controller node looks like this:

pcie-controller { compatible = "nvidia,tegra20-pcie",
"simple-bus"; reg = <0x80003000 0x00000800 /* PADS registers */
0x80003800 0x00000200 /* AFI registers */ 0x80004000 0x00100000
/* configuration space */ 0x80104000 0x00100000>; /* extended
configuration space */ interrupts = <0 98 0x04 /* controller
interrupt */ 0 99 0x04>; /* MSI interrupt */ status = "disabled";

ranges = <0x80000000 0x80000000 0x00002000 /* 2 root ports */
0x80400000 0x80400000 0x00010000 /* downstream I/O */ 0x90000000
0x90000000 0x10000000 /* non-prefetchable memory */ 0xa0000000
0xa0000000 0x10000000>; /* prefetchable memory */

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

compatible = "nvidia,tegra20-pcie-port"; reg = <0x80000000 0x00001000>; status = "disabled";

#address-cells = <3>; #size-cells = <2>;

ranges = <0x81000000 0 0 0x80400000 0 0x00008000 /* I/O */
0x82000000 0 0 0x90000000 0 0x08000000 /* non-prefetchable memory
*/ 0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

nvidia,ctrl-offset = <0x110>; nvidia,num-lanes = <2>; };

pci at 80001000 { compatible = "nvidia,tegra20-pcie-port"; reg = <0x80001000 0x00001000>; status = "disabled";

#address-cells = <3>; #size-cells = <2>;

ranges = <0x81000000 0 0 0x80408000 0 0x00008000 /* I/O */
0x82000000 0 0 0x98000000 0 0x08000000 /* non-prefetchable memory
*/ 0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

nvidia,ctrl-offset = <0x118>; nvidia,num-lanes = <2>; }; };

While looking into some more code, trying to figure out how to hook this all up with the device tree I ran into a problem. I need to actually create a 'struct device' for each of the ports, so I added the "simple-bus" to the pcie-controller's "compatible" property. Furthermore, each PCI root port now becomes a platform_device, which are supported by a new tegra-pcie-port driver. I'm not sure if "port" is very common in PCI speek, so something like tegra-pcie-bridge (compatible = "nvidia,tegra20-pcie-bridge") may be more appropriate?

On 06/14/2012 03:19 AM, Thierry Reding wrote:
...

Quoted 9 lines

Okay, so the new pcie-controller node looks like this:

pcie-controller { compatible = "nvidia,tegra20-pcie",
"simple-bus"; reg = <0x80003000 0x00000800 /* PADS registers */
0x80003800 0x00000200 /* AFI registers */ 0x80004000 0x00100000
/* configuration space */ 0x80104000 0x00100000>; /* extended
configuration space */ interrupts = <0 98 0x04 /* controller
interrupt */ 0 99 0x04>; /* MSI interrupt */ status = "disabled";

Quoted 4 lines

ranges = <0x80000000 0x80000000 0x00002000 /* 2 root ports */
0x80400000 0x80400000 0x00010000 /* downstream I/O */ 0x90000000
0x90000000 0x10000000 /* non-prefetchable memory */ 0xa0000000
0xa0000000 0x10000000>; /* prefetchable memory */

Do we actually need to specifically enumerate all the ranges; would just "ranges;" be simpler?

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 5 lines

compatible = "nvidia,tegra20-pcie-port"; reg = <0x80000000 0x00001000>; status = "disabled";

#address-cells = <3>; #size-cells = <2>;

Quoted 4 lines

ranges = <0x81000000 0 0 0x80400000 0 0x00008000 /* I/O */
0x82000000 0 0 0x90000000 0 0x08000000 /* non-prefetchable memory
*/ 0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

The values here appear identical for both ports. Surely they should describe just the parts of the overall address space that have been assigned/delegated to the individual port/bridge?

Note that initial indications are that the overall controller receives transactions for those address ranges, and standard PCIe bridge registers are used to steer chunks of these address ranges into the individual downstream ports/busses. Hence, standard PCIe documentation should indicate how to do this.

Quoted 14 lines

nvidia,ctrl-offset = <0x110>; nvidia,num-lanes = <2>; };

pci at 80001000 { compatible = "nvidia,tegra20-pcie-port"; reg = <0x80001000 0x00001000>; status = "disabled";

#address-cells = <3>; #size-cells = <2>;

ranges = <0x81000000 0 0 0x80408000 0 0x00008000 /* I/O */
0x82000000 0 0 0x98000000 0 0x08000000 /* non-prefetchable memory
*/ 0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

nvidia,ctrl-offset = <0x118>; nvidia,num-lanes = <2>; }; };

Quoted 9 lines

While looking into some more code, trying to figure out how to hook this all up with the device tree I ran into a problem. I need to actually create a 'struct device' for each of the ports, so I added the "simple-bus" to the pcie-controller's "compatible" property. Furthermore, each PCI root port now becomes a platform_device, which are supported by a new tegra-pcie-port driver. I'm not sure if "port" is very common in PCI speek, so something like tegra-pcie-bridge (compatible = "nvidia,tegra20-pcie-bridge") may be more appropriate?

What is it that drives the need for each port to be a 'struct device'? The current driver supports 2 host ports, yet there's only a single struct device for it. Does the DT code assume a 1:1 mapping between struct device and DT node that represents the child bus? If so, perhaps it'd be better to rework that code to accept a DT node as a parameter and call it multiple times, rather than accept a struct device as a parameter and hence need multiple devices?

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 10 lines

Quoted 9 lines

compatible = "nvidia,tegra20-pcie-port"; reg = <0x80000000 0x00001000>; status = "disabled";

#address-cells = <3>; #size-cells = <2>;

ranges = <0x81000000 0 0 0x80400000 0 0x00008000 /* I/O */
0x82000000 0 0 0x90000000 0 0x08000000 /* non-prefetchable
memory */ 0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

Quoted 3 lines

The values here appear identical for both ports. Surely they should describe just the parts of the overall address space that have been assigned/delegated to the individual port/bridge?

They're not identical. Port 0 gets the first half and port 1 gets the second half of the ranges specified in the parent.

Quoted 10 lines

Quoted 9 lines

While looking into some more code, trying to figure out how to hook this all up with the device tree I ran into a problem. I need to actually create a 'struct device' for each of the ports, so I added the "simple-bus" to the pcie-controller's "compatible" property. Furthermore, each PCI root port now becomes a platform_device, which are supported by a new tegra-pcie-port driver. I'm not sure if "port" is very common in PCI speek, so something like tegra-pcie-bridge (compatible = "nvidia,tegra20-pcie-bridge") may be more appropriate?

Quoted 8 lines

What is it that drives the need for each port to be a 'struct device'? The current driver supports 2 host ports, yet there's only a single struct device for it. Does the DT code assume a 1:1 mapping between struct device and DT node that represents the child bus? If so, perhaps it'd be better to rework that code to accept a DT node as a parameter and call it multiple times, rather than accept a struct device as a parameter and hence need multiple devices?

It's not so much the DT code, but rather the PCI core and ultimately the device model that requires it. Each port is basically a PCI host bridge that provides a root PCI bus and the device model is used to represent the hierachy of the busses. Providing just the DT node isn't going to be enough.

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

compatible = "nvidia,tegra20-pcie-port"; reg = <0x80000000 0x00001000>; status = "disabled";

#address-cells = <3>; #size-cells = <2>;

ranges = <0x81000000 0 0 0x80400000 0 0x00008000 /* I/O */
0x82000000 0 0 0x90000000 0 0x08000000 /* non-prefetchable
memory */ 0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

Quoted 6 lines

Quoted 3 lines

The values here appear identical for both ports. Surely they should describe just the parts of the overall address space that have been assigned/delegated to the individual port/bridge?

They're not identical. Port 0 gets the first half and port 1 gets the second half of the ranges specified in the parent.

Oh right, I missed some 8s and 0s that looked the same!

Quoted 19 lines

Quoted 18 lines

Quoted 9 lines

While looking into some more code, trying to figure out how to hook this all up with the device tree I ran into a problem. I need to actually create a 'struct device' for each of the ports, so I added the "simple-bus" to the pcie-controller's "compatible" property. Furthermore, each PCI root port now becomes a platform_device, which are supported by a new tegra-pcie-port driver. I'm not sure if "port" is very common in PCI speek, so something like tegra-pcie-bridge (compatible = "nvidia,tegra20-pcie-bridge") may be more appropriate?

What is it that drives the need for each port to be a 'struct device'? The current driver supports 2 host ports, yet there's only a single struct device for it. Does the DT code assume a 1:1 mapping between struct device and DT node that represents the child bus? If so, perhaps it'd be better to rework that code to accept a DT node as a parameter and call it multiple times, rather than accept a struct device as a parameter and hence need multiple devices?

Quoted 5 lines

It's not so much the DT code, but rather the PCI core and ultimately the device model that requires it. Each port is basically a PCI host bridge that provides a root PCI bus and the device model is used to represent the hierachy of the busses. Providing just the DT node isn't going to be enough.

But the existing driver works without /any/ devices, let alone one per port.

Quoted 35 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

This really depends on the device vs. no device decision below. If we can make it work without needing an extra device for it, then using the index is certainly better. However, if we instantiate devices from the DT, then we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the port at address 0x80000000 to 2?).

Quoted 38 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

compatible = "nvidia,tegra20-pcie-port"; reg = <0x80000000 0x00001000>; status = "disabled";

#address-cells = <3>; #size-cells = <2>;

ranges = <0x81000000 0 0 0x80400000 0 0x00008000 /* I/O */
0x82000000 0 0 0x90000000 0 0x08000000 /* non-prefetchable
memory */ 0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

Quoted 6 lines

Quoted 3 lines

The values here appear identical for both ports. Surely they should describe just the parts of the overall address space that have been assigned/delegated to the individual port/bridge?

They're not identical. Port 0 gets the first half and port 1 gets the second half of the ranges specified in the parent.

Oh right, I missed some 8s and 0s that looked the same!

Quoted 19 lines

Quoted 18 lines

Quoted 9 lines

While looking into some more code, trying to figure out how to hook this all up with the device tree I ran into a problem. I need to actually create a 'struct device' for each of the ports, so I added the "simple-bus" to the pcie-controller's "compatible" property. Furthermore, each PCI root port now becomes a platform_device, which are supported by a new tegra-pcie-port driver. I'm not sure if "port" is very common in PCI speek, so something like tegra-pcie-bridge (compatible = "nvidia,tegra20-pcie-bridge") may be more appropriate?

What is it that drives the need for each port to be a 'struct device'? The current driver supports 2 host ports, yet there's only a single struct device for it. Does the DT code assume a 1:1 mapping between struct device and DT node that represents the child bus? If so, perhaps it'd be better to rework that code to accept a DT node as a parameter and call it multiple times, rather than accept a struct device as a parameter and hence need multiple devices?

Quoted 8 lines

Quoted 5 lines

It's not so much the DT code, but rather the PCI core and ultimately the device model that requires it. Each port is basically a PCI host bridge that provides a root PCI bus and the device model is used to represent the hierachy of the busses. Providing just the DT node isn't going to be enough.

But the existing driver works without /any/ devices, let alone one per port.

That doesn't necessarily mean it is correct. The representation of the device tree (as in the Linux kernel device tree) isn't quite accurate because you're missing the link of the host bridge to the parent PCIe controller. It also means that the representation of the kernel device tree doesn't match the DT representation.

Additionally, if you look at how PCI busses and devices are matched to their respective DT nodes, the code in drivers/pci/of.c provides a default implementation of pcibios_get_phb_of_node(), which matches the struct pci_bus up with the device_node of the parent device.

If we keep the current implementation that passes NULL as parent to the pci_scan_root_bus() function, then we'll have to provide a custom implementation of pcibios_get_phb_of_node() which has to go through similar hoops as the x86 version (see arch/x86/kernel/devicetree.c). That of course will not contribute to improving the current state of fragmentation in the PCI subsystem.

Quoted 1 line · Thierry Reding

On Thu, Jun 14, 2012 at 01:50:56PM -0600, Stephen Warren wrote:

Quoted 11 lines

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

This really depends on the device vs. no device decision below. If we can make it work without needing an extra device for it, then using the index is certainly better. However, if we instantiate devices from the DT, then we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the port at address 0x80000000 to 2?).

An additional problem with this is that we'd have to add the following to the pcie-controller node:

	#address-cells = <1>;
	#size-cells = <0>;

This will conflict with the "ranges" property, because suddenly we can no longer map the regions properly. Maybe Mitch can comment on whether this is possible or not?

To make it clearer what I'm talking about, here's the DT snippet again (with the compatible property removed from the pci@ nodes because they are no longer probed by a driver, the "simple-bus" removed from the pcie-controller node's compatible property removed and its #address- and #size-cells properties adjusted as described above).

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <0>;

		pci at 0 {
			reg = <2>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80400000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x90000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};

		pci at 1 {
			reg = <1>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80408000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x98000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};
	};

AIUI none of the ranges properties are valid anymore, because the bus represented by pcie-controller no longer reflects the truth, namely that it translates the CPU address space to the PCI address space.

Quoted 41 lines · Thierry Reding

Quoted 35 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

Quoted 6 lines

This really depends on the device vs. no device decision below. If we can make it work without needing an extra device for it, then using the index is certainly better. However, if we instantiate devices from the DT, then we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the port at address 0x80000000 to 2?).

An additional problem with this is that we'd have to add the following to the pcie-controller node:

	#address-cells = <1>;
	#size-cells = <0>;

This will conflict with the "ranges" property, because suddenly we can no longer map the regions properly. Maybe Mitch can comment on whether this is possible or not?

To make it clearer what I'm talking about, here's the DT snippet again (with the compatible property removed from the pci@ nodes because they are no longer probed by a driver, the "simple-bus" removed from the pcie-controller node's compatible property removed and its #address- and #size-cells properties adjusted as described above).

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <0>;

		pci at 0 {
			reg = <2>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80400000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x90000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};

		pci at 1 {
			reg = <1>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80408000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x98000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};
	};

AIUI none of the ranges properties are valid anymore, because the bus represented by pcie-controller no longer reflects the truth, namely that it translates the CPU address space to the PCI address space.

Quoted 119 lines · Thierry Reding

Quoted 41 lines · Thierry Reding

Quoted 35 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

Quoted 10 lines

This really depends on the device vs. no device decision below. If we can
make it work without needing an extra device for it, then using the index
is certainly better. However, if we instantiate devices from the DT, then
we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the
port at address 0x80000000 to 2?).

An additional problem with this is that we'd have to add the following to the pcie-controller node:

    #address-cells = <1>;
    #size-cells = <0>;

This will conflict with the "ranges" property, because suddenly we can no longer map the regions properly. Maybe Mitch can comment on whether this is possible or not?

To make it clearer what I'm talking about, here's the DT snippet again (with the compatible property removed from the pci@ nodes because they are no longer probed by a driver, the "simple-bus" removed from the pcie-controller node's compatible property removed and its #address- and #size-cells properties adjusted as described above).

    pcie-controller {
        compatible = "nvidia,tegra20-pcie";
        reg = <0x80003000 0x00000800   /* PADS registers */
               0x80003800 0x00000200   /* AFI registers */
               0x80004000 0x00100000   /* configuration space */
               0x80104000 0x00100000>; /* extended configuration space */
        interrupts = <0 98 0x04   /* controller interrupt */
                  0 99 0x04>; /* MSI interrupt */
        status = "disabled";

        ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
              0x80400000 0x80400000 0x00010000   /* downstream I/O */
              0x90000000 0x90000000 0x10000000   /* non-prefetchable
memory */
              0xa0000000 0xa0000000 0x10000000>; /* prefetchable

memory */

        #address-cells = <1>;
        #size-cells = <0>;

        pci at 0 {
            reg = <2>;
            status = "disabled";

            #address-cells = <3>;
            #size-cells = <2>;

            ranges = <0x81000000 0 0 0x80400000 0 0x00008000   /* I/O */
                  0x82000000 0 0 0x90000000 0 0x08000000   /*
non-prefetchable memory */
                  0xc2000000 0 0 0xa0000000 0 0x08000000>; /*

prefetchable memory */

            nvidia,ctrl-offset = <0x110>;
            nvidia,num-lanes = <2>;
        };

        pci at 1 {
            reg = <1>;
            status = "disabled";

            #address-cells = <3>;
            #size-cells = <2>;

            ranges = <0x81000000 0 0 0x80408000 0 0x00008000   /* I/O */
                  0x82000000 0 0 0x98000000 0 0x08000000   /*
non-prefetchable memory */
                  0xc2000000 0 0 0xa8000000 0 0x08000000>; /*

prefetchable memory */

Quoted 9 lines

            nvidia,ctrl-offset = <0x118>;
            nvidia,num-lanes = <2>;
        };
    };

AIUI none of the ranges properties are valid anymore, because the bus represented by pcie-controller no longer reflects the truth, namely that it translates the CPU address space to the PCI address space.

I think you can use a small-integer port number as an address by defining the intermediate address space properly, and using appropriate ranges above and below. Here's a swag at how that would look.

I present three versions, using different choices for the intermediate address space encoding. The intermediate address space may seem somewhat artificial, in that it decouples the "linear pass through of ranges" between the lower PCI address space and the upper CPU address space. But in another sense, it accurately reflects that fact that the bus bridge "slices and dices" that linear address space into non-contiguous pieces.

Note that I'm also fixing a problem that I neglected to mention earlier
- namely the fact that config space is part of the child PCI address space so it must be passed through.

Version A - 3 address cells:  In this version, the intermediate address
space has 3 cells:  port#, address type, offset.  Address type is

0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

On 06/19/2012 03:31 PM, Mitch Bradley wrote:
...

The third cell "offset" is necessary so that the size field has a number space that can include it.

Can you expand on that sentence a bit more; I don't quite understand that aspect. Thanks.

Quoted 7 lines · Stephen Warren

On 06/19/2012 03:31 PM, Mitch Bradley wrote:
...

The third cell "offset" is necessary so that the size field has a number space that can include it.

Quoted 2 lines

Can you expand on that sentence a bit more; I don't quite understand that aspect. Thanks.

The meaning of "size"(in the context of "reg" which is phys,size) is "the device occupies a
contiguous sequence of addresses beginning at phys and continuing to phys+size-1". The
implicit addition occurs on the last #size-cells cells of the phys.

Version A - 3 address cells:  In this version, the intermediate address 
space has 3 cells:  port#, address type, offset.  Address type is

0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

Version B - 2 address cells:  In this first version, the intermediate
address space has 2 cells:  port#, offset.  The address type (I/O, mem,
etc) is the high digit of in the offset:
    Offset 0....... : Root port
    Offset 1....... : config
    Offset 2....... : extended config
    Offset 3....... : I/O
    Offset 4....... : non-prefetchable memory
    Offset 5....... : prefetchable memory.

Version C - 2 address cells, 0 size cells (!) : In this version we hide the size component in the intermediate space. I don't know if that will actually work, but my guess is that it probably would. The intermediate address space is like version A with the omission of the offset field. The intermediate address just specifies the port number and the address type.

Quoted 11 lines · Mitch Bradley

Version A - 3 address cells:  In this version, the intermediate address
space has 3 cells:  port#, address type, offset.  Address type is

0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

I agree with Stephen that this is a clever way to encode all the address spaces, very nice!

Quoted 9 lines

Version B - 2 address cells:  In this first version, the intermediate
address space has 2 cells:  port#, offset.  The address type (I/O, mem,
etc) is the high digit of in the offset:
     Offset 0....... : Root port
     Offset 1....... : config
     Offset 2....... : extended config
     Offset 3....... : I/O
     Offset 4....... : non-prefetchable memory
     Offset 5....... : prefetchable memory.

This is similar to how the PCI binding works internally, but I find that a bit confusing as well, so given those two choices, I'd prefer the first one.

Quoted 6 lines

Version C - 2 address cells, 0 size cells (!) : In this version we hide the size component in the intermediate space. I don't know if that will actually work, but my guess is that it probably would. The intermediate address space is like version A with the omission of the offset field. The intermediate address just specifies the port number and the address type.

My guess is that it doesn't work with the current Linux code that transforms the addresses. I've just recently seen the code complain about any platform device whose parent has #size-cells=0.

Arnd

Quoted 114 lines · Thierry Reding

Quoted 41 lines · Thierry Reding

Quoted 35 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

Quoted 6 lines

This really depends on the device vs. no device decision below. If we can make it work without needing an extra device for it, then using the index is certainly better. However, if we instantiate devices from the DT, then we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the port at address 0x80000000 to 2?).

An additional problem with this is that we'd have to add the following to the pcie-controller node:

	#address-cells = <1>;
	#size-cells = <0>;

This will conflict with the "ranges" property, because suddenly we can no longer map the regions properly. Maybe Mitch can comment on whether this is possible or not?

To make it clearer what I'm talking about, here's the DT snippet again (with the compatible property removed from the pci@ nodes because they are no longer probed by a driver, the "simple-bus" removed from the pcie-controller node's compatible property removed and its #address- and #size-cells properties adjusted as described above).

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <0>;

		pci at 0 {
			reg = <2>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80400000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x90000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};

		pci at 1 {
			reg = <1>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80408000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x98000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};
	};

Quoted 4 lines

AIUI none of the ranges properties are valid anymore, because the bus represented by pcie-controller no longer reflects the truth, namely that it translates the CPU address space to the PCI address space.

I think you can use a small-integer port number as an address by defining the intermediate address space properly, and using appropriate ranges above and below. Here's a swag at how that would look.

I present three versions, using different choices for the intermediate address space encoding. The intermediate address space may seem somewhat artificial, in that it decouples the "linear pass through of ranges" between the lower PCI address space and the upper CPU address space. But in another sense, it accurately reflects that fact that the bus bridge "slices and dices" that linear address space into non-contiguous pieces.

Note that I'm also fixing a problem that I neglected to mention earlier - namely the fact that config space is part of the child PCI address space so it must be passed through.

Version A - 3 address cells:  In this version, the intermediate
address space has 3 cells:  port#, address type, offset.  Address

type is
0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0 0 0  0x80000000 0x00001000   /* Root port 0 */
			  0 1 0  0x80004000 0x00080000   /* Port 0 config space */
			  0 2 0  0x80104000 0x00080000   /* Port 0 ext config space *
			  0 3 0  0x80400000 0x00008000   /* Port 0 downstream I/O */
			  0 4 0  0x90000000 0x08000000   /* Port 0 non-prefetchable memory */
			  0 5 0  0xa0000000 0x08000000   /* Port 0 prefetchable memory */

			  1 0 0  0x80001000 0x00001000   /* Root port 1 */
			  1 1 0  0x80004000 0x00080000   /* Port 1 config space */
			  1 2 0  0x80184000 0x00080000   /* Port 1 ext config space */
			  1 3 0  0x80408000 0x00010000   /* Port 1 downstream I/O */
			  1 4 0  0x98000000 0x08000000   /* Port 1 non-prefetchable memory */
			  1 5 0  0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

		#address-cells = <3>;
		#size-cells = <1>;

		pci at 0 {
			reg = <0 0 0 0x1000>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x80000000 0 0  0 1 0  0 0x00080000   /* config */
				  0x90000000 0 0  0 2 0  0 0x00080000   /* extended config */
				  0x81000000 0 0  0 3 0  0 0x00008000   /* I/O */
				  0x82000000 0 0  0 4 0  0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0  0 5 0  0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};


		pci at 1 {
			reg = <1 0 0 0x1000>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x80000000 0 0  1 1 0  0 0x00080000   /* config */

				  0x90000000 0 0  1 2 0  0 0x00080000   /* extended config */
				  0x81000000 0 0  1 3 0  0 0x00008000   /* I/O */
				  0x82000000 0 0  1 4 0  0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0  1 5 0  0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};

Quoted 69 lines · Mitch Bradley

Version A - 3 address cells: ?In this version, the intermediate address space has 3 cells: ?port#, address type, offset. ?Address type is
? 0 : root port
? 1 : config space
? 2 : extended config space
? 3 : I/O
? 4 : non-prefetchable memory
? 5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

? ? ? pcie-controller {
? ? ? ? ? ? ? compatible = "nvidia,tegra20-pcie";
? ? ? ? ? ? ? reg = <0x80003000 0x00000800 ? /* PADS registers */ ? ? ? ? ? ? ? ? ? ? ?0x80003800 0x00000200>; /* extended configuration space */ ? ? ? ? ? ? ? interrupts = <0 98 0x04 ? /* controller interrupt */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0 99 0x04>; /* MSI interrupt */
? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ranges = <0 0 0 ?0x80000000 0x00001000 ? /* Root port 0 */
? ? ? ? ? ? ? ? ? ? ? ? 0 1 0 ?0x80004000 0x00080000 ? /* Port 0 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 0 2 0 ?0x80104000 0x00080000 ? /* Port 0 ext config space * ? ? ? ? ? ? ? ? ? ? ? ? 0 3 0 ?0x80400000 0x00008000 ? /* Port 0 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 0 4 0 ?0x90000000 0x08000000 ? /* Port 0 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 0 5 0 ?0xa0000000 0x08000000 ? /* Port 0 prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? ? 1 0 0 ?0x80001000 0x00001000 ? /* Root port 1 */
? ? ? ? ? ? ? ? ? ? ? ? 1 1 0 ?0x80004000 0x00080000 ? /* Port 1 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 2 0 ?0x80184000 0x00080000 ? /* Port 1 ext config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 3 0 ?0x80408000 0x00010000 ? /* Port 1 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 1 4 0 ?0x98000000 0x08000000 ? /* Port 1 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 1 5 0 ?0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? #size-cells = <1>;

? ? ? ? ? ? ? pci at 0 {
? ? ? ? ? ? ? ? ? ? ? reg = <0 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?0 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?0 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?0 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?0 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?0 5 0 ?0 0x08000000>; /* prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x110>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

? ? ? ? ? ? ? pci at 1 {
? ? ? ? ? ? ? ? ? ? ? reg = <1 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?1 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?1 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?1 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?1 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?1 5 0 ?0 0x08000000>; /* prefetchable memory */

Quoted 3 lines

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x118>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

Quoted 71 lines · Thierry Reding

Quoted 69 lines · Mitch Bradley

Version A - 3 address cells: ?In this version, the intermediate address space has 3 cells: ?port#, address type, offset. ?Address type is
? 0 : root port
? 1 : config space
? 2 : extended config space
? 3 : I/O
? 4 : non-prefetchable memory
? 5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

? ? ? pcie-controller {
? ? ? ? ? ? ? compatible = "nvidia,tegra20-pcie";
? ? ? ? ? ? ? reg = <0x80003000 0x00000800 ? /* PADS registers */ ? ? ? ? ? ? ? ? ? ? ?0x80003800 0x00000200>; /* extended configuration space */ ? ? ? ? ? ? ? interrupts = <0 98 0x04 ? /* controller interrupt */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0 99 0x04>; /* MSI interrupt */
? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ranges = <0 0 0 ?0x80000000 0x00001000 ? /* Root port 0 */
? ? ? ? ? ? ? ? ? ? ? ? 0 1 0 ?0x80004000 0x00080000 ? /* Port 0 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 0 2 0 ?0x80104000 0x00080000 ? /* Port 0 ext config space * ? ? ? ? ? ? ? ? ? ? ? ? 0 3 0 ?0x80400000 0x00008000 ? /* Port 0 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 0 4 0 ?0x90000000 0x08000000 ? /* Port 0 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 0 5 0 ?0xa0000000 0x08000000 ? /* Port 0 prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? ? 1 0 0 ?0x80001000 0x00001000 ? /* Root port 1 */
? ? ? ? ? ? ? ? ? ? ? ? 1 1 0 ?0x80004000 0x00080000 ? /* Port 1 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 2 0 ?0x80184000 0x00080000 ? /* Port 1 ext config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 3 0 ?0x80408000 0x00010000 ? /* Port 1 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 1 4 0 ?0x98000000 0x08000000 ? /* Port 1 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 1 5 0 ?0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? #size-cells = <1>;

? ? ? ? ? ? ? pci at 0 {
? ? ? ? ? ? ? ? ? ? ? reg = <0 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?0 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?0 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?0 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?0 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?0 5 0 ?0 0x08000000>; /* prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x110>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

? ? ? ? ? ? ? pci at 1 {
? ? ? ? ? ? ? ? ? ? ? reg = <1 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?1 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?1 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?1 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?1 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?1 5 0 ?0 0x08000000>; /* prefetchable memory */

Quoted 3 lines

Quoted 3 lines

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x118>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

I'm not familiar with device tree, so pardon me if these are stupid questions. These seem to be describing PCI host bridges.

I assume some of these ranges describe MMIO, prefetchable MMIO, and I/O port apertures that the bridge forwards to the PCI bus.

Is there provision for any address offset applied by the bridge when it forwards downstream? (Maybe these bridges don't apply any offset?)
"0xa0000000 0x08000000" appears for both ports 0 and 1 prefetchable
memory; I don't know if that's an error, an indication that each bridge applies a different offset, or that both bridges forward the same aperture (I hope not the latter, because Linux can't really deal with that).

Is the bus number aperture included somewhere? How do we know what bus numbers are available for allocation under each bridge?

I see mention of config space. Is some of that referring to the ECAM as in 7.2.2 of the PCIe spec v3.0 (what we refer to as MMCONFIG on x86)?

Quoted 73 lines · Bjorn Helgaas

Quoted 71 lines · Thierry Reding

Quoted 69 lines · Mitch Bradley

Version A - 3 address cells: ?In this version, the intermediate address space has 3 cells: ?port#, address type, offset. ?Address type is
? 0 : root port
? 1 : config space
? 2 : extended config space
? 3 : I/O
? 4 : non-prefetchable memory
? 5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

? ? ? pcie-controller {
? ? ? ? ? ? ? compatible = "nvidia,tegra20-pcie";
? ? ? ? ? ? ? reg = <0x80003000 0x00000800 ? /* PADS registers */ ? ? ? ? ? ? ? ? ? ? ?0x80003800 0x00000200>; /* extended configuration space */ ? ? ? ? ? ? ? interrupts = <0 98 0x04 ? /* controller interrupt */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0 99 0x04>; /* MSI interrupt */
? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ranges = <0 0 0 ?0x80000000 0x00001000 ? /* Root port 0 */
? ? ? ? ? ? ? ? ? ? ? ? 0 1 0 ?0x80004000 0x00080000 ? /* Port 0 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 0 2 0 ?0x80104000 0x00080000 ? /* Port 0 ext config space * ? ? ? ? ? ? ? ? ? ? ? ? 0 3 0 ?0x80400000 0x00008000 ? /* Port 0 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 0 4 0 ?0x90000000 0x08000000 ? /* Port 0 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 0 5 0 ?0xa0000000 0x08000000 ? /* Port 0 prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? ? 1 0 0 ?0x80001000 0x00001000 ? /* Root port 1 */
? ? ? ? ? ? ? ? ? ? ? ? 1 1 0 ?0x80004000 0x00080000 ? /* Port 1 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 2 0 ?0x80184000 0x00080000 ? /* Port 1 ext config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 3 0 ?0x80408000 0x00010000 ? /* Port 1 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 1 4 0 ?0x98000000 0x08000000 ? /* Port 1 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 1 5 0 ?0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? #size-cells = <1>;

? ? ? ? ? ? ? pci at 0 {
? ? ? ? ? ? ? ? ? ? ? reg = <0 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?0 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?0 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?0 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?0 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?0 5 0 ?0 0x08000000>; /* prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x110>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

? ? ? ? ? ? ? pci at 1 {
? ? ? ? ? ? ? ? ? ? ? reg = <1 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?1 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?1 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?1 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?1 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?1 5 0 ?0 0x08000000>; /* prefetchable memory */

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x118>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

I'm not familiar with device tree, so pardon me if these are stupid questions. ?These seem to be describing PCI host bridges.

Yes, correct.

Quoted 2 lines

I assume some of these ranges describe MMIO, prefetchable MMIO, and I/O port apertures that the bridge forwards to the PCI bus.

Yes.

Quoted 7 lines

Is there provision for any address offset applied by the bridge when it forwards downstream? ?(Maybe these bridges don't apply any offset?) ?"0xa0000000 0x08000000" appears for both ports 0 and 1 prefetchable memory; I don't know if that's an error, an indication that each bridge applies a different offset, or that both bridges forward the same aperture (I hope not the latter, because Linux can't really deal with that).

That seems to be a typo. It should have been 0xa8000000 in the second case. The PCIe controller has registers that are programmed with the aperture for the configuration and extended configuration spaces, as well as the downstream I/O, prefetchable and non-prefetchable memory regions.

The configuration spaces aren't actually forwarded by the bridges, but are handled only by the controller.

The other apertures are programmed
into the bridges using standard PCI registers.

Quoted 2 lines

Is the bus number aperture included somewhere? ?How do we know what bus numbers are available for allocation under each bridge?

Not yet. I don't think DT imposes a bus number allocation on PCI bridges. However the matching of DT nodes to PCI bridges is done based on the bus number. For that you provide a bus-ranges property which defines the bus aperture of the given PCI bridge. The DT matching code compares the first cell of this property with the primary bus number of the bridge.

This is in fact somewhat counter-productive because it requires you to hard-code the PCI hierarchy within the DT and you loose a lot of the probing functionality. This is not much of an issue for typical PCI endpoints (like network cards) but becomes a problem if you have a PCI endpoint that implements an I2C bus and you want to match I2C slaves on that bus to device nodes so that the kernel can parse and instantiate them properly. I guess in the latter cases hard-coding the PCI tree in DT is not actually a drawback, though.

Quoted 3 lines

I see mention of config space. ?Is some of that referring to the ECAM as in 7.2.2 of the PCIe spec v3.0 (what we refer to as MMCONFIG on x86)?

I only have access to the PCIe 2.0 specification, but it seems to contain the same 7.2.2 section. In fact it looks as though this particular controller indeed implements something similar to ECAM. The address mapping is a little different, though:

? ? ? ?A[(16 + n - 1):16]: bus number
? ? ? ?A[15:11]: device number
? ? ? ?A[10:8]: function number
? ? ? ?A[7:2]: register number
? ? ? ?A[1:0]: unused

That information comes directly from the driver and I have no access to the corresponding documentation. It seems like the extended configuration space is accessed using the same mapping but starting at a different base address.

But now that you mention it, maybe the driver code is buggy here, and the controller indeed implements ECAM. Furthermore it also seems like the current code doesn't work for the extended configuration space because while the correct offset into the extended configuration space is chosen, the access to registers is done using the same mapping as above, so instead of the 12 (10) bits required for the register number only 8 (6) are in fact used.

I wonder whether anyone's actually tested this.

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

Quoted 96 lines · Thierry Reding

Quoted 73 lines · Bjorn Helgaas

Quoted 71 lines · Thierry Reding

Quoted 69 lines · Mitch Bradley

Version A - 3 address cells: ?In this version, the intermediate address space has 3 cells: ?port#, address type, offset. ?Address type is
? 0 : root port
? 1 : config space
? 2 : extended config space
? 3 : I/O
? 4 : non-prefetchable memory
? 5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

? ? ? pcie-controller {
? ? ? ? ? ? ? compatible = "nvidia,tegra20-pcie";
? ? ? ? ? ? ? reg = <0x80003000 0x00000800 ? /* PADS registers */ ? ? ? ? ? ? ? ? ? ? ?0x80003800 0x00000200>; /* extended configuration space */ ? ? ? ? ? ? ? interrupts = <0 98 0x04 ? /* controller interrupt */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0 99 0x04>; /* MSI interrupt */
? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ranges = <0 0 0 ?0x80000000 0x00001000 ? /* Root port 0 */
? ? ? ? ? ? ? ? ? ? ? ? 0 1 0 ?0x80004000 0x00080000 ? /* Port 0 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 0 2 0 ?0x80104000 0x00080000 ? /* Port 0 ext config space * ? ? ? ? ? ? ? ? ? ? ? ? 0 3 0 ?0x80400000 0x00008000 ? /* Port 0 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 0 4 0 ?0x90000000 0x08000000 ? /* Port 0 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 0 5 0 ?0xa0000000 0x08000000 ? /* Port 0 prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? ? 1 0 0 ?0x80001000 0x00001000 ? /* Root port 1 */
? ? ? ? ? ? ? ? ? ? ? ? 1 1 0 ?0x80004000 0x00080000 ? /* Port 1 config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 2 0 ?0x80184000 0x00080000 ? /* Port 1 ext config space */ ? ? ? ? ? ? ? ? ? ? ? ? 1 3 0 ?0x80408000 0x00010000 ? /* Port 1 downstream I/O */ ? ? ? ? ? ? ? ? ? ? ? ? 1 4 0 ?0x98000000 0x08000000 ? /* Port 1 non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? 1 5 0 ?0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? #size-cells = <1>;

? ? ? ? ? ? ? pci at 0 {
? ? ? ? ? ? ? ? ? ? ? reg = <0 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?0 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?0 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?0 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?0 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?0 5 0 ?0 0x08000000>; /* prefetchable memory */

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x110>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

? ? ? ? ? ? ? pci at 1 {
? ? ? ? ? ? ? ? ? ? ? reg = <1 0 0 0x1000>;
? ? ? ? ? ? ? ? ? ? ? status = "disabled";

? ? ? ? ? ? ? ? ? ? ? #address-cells = <3>;
? ? ? ? ? ? ? ? ? ? ? #size-cells = <2>;

? ? ? ? ? ? ? ? ? ? ? ranges = <0x80000000 0 0 ?1 1 0 ?0 0x00080000 ? /* config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x90000000 0 0 ?1 2 0 ?0 0x00080000 ? /* extended config */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x81000000 0 0 ?1 3 0 ?0 0x00008000 ? /* I/O */
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x82000000 0 0 ?1 4 0 ?0 0x08000000 ? /* non-prefetchable memory */ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0xc2000000 0 0 ?1 5 0 ?0 0x08000000>; /* prefetchable memory */

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

? ? ? ? ? ? ? ? ? ? ? nvidia,ctrl-offset = <0x118>; ? ? ? ? ? ? ? ? ? ? ? nvidia,num-lanes = <2>; ? ? ? ? ? ? ? };

I'm not familiar with device tree, so pardon me if these are stupid questions. ?These seem to be describing PCI host bridges.

Yes, correct.

Quoted 2 lines

I assume some of these ranges describe MMIO, prefetchable MMIO, and I/O port apertures that the bridge forwards to the PCI bus.

Yes.

Quoted 7 lines

Is there provision for any address offset applied by the bridge when it forwards downstream? ?(Maybe these bridges don't apply any offset?) ?"0xa0000000 0x08000000" appears for both ports 0 and 1 prefetchable memory; I don't know if that's an error, an indication that each bridge applies a different offset, or that both bridges forward the same aperture (I hope not the latter, because Linux can't really deal with that).

Quoted 8 lines

That seems to be a typo. It should have been 0xa8000000 in the second case. The PCIe controller has registers that are programmed with the aperture for the configuration and extended configuration spaces, as well as the downstream I/O, prefetchable and non-prefetchable memory regions.

The configuration spaces aren't actually forwarded by the bridges, but are handled only by the controller.

Right. The host bridge (controller) would convert memory accesses on the upstream side into PCI config accesses on its downstream PCI interface.

Quoted 2 lines

The other apertures are programmed
into the bridges using standard PCI registers.

If you're talking about programming host bridge apertures, there are no "standard PCI registers" to do that. The programming model of the host bridge itself is not part of the PCI spec. PCI-to-PCI bridge apertures *are* specified by the PCI Bridge spec, so those are standard.

Quoted 9 lines

Quoted 2 lines

Is the bus number aperture included somewhere? ?How do we know what bus numbers are available for allocation under each bridge?

Not yet. I don't think DT imposes a bus number allocation on PCI bridges. However the matching of DT nodes to PCI bridges is done based on the bus number. For that you provide a bus-ranges property which defines the bus aperture of the given PCI bridge. The DT matching code compares the first cell of this property with the primary bus number of the bridge.

I don't fully understand this, but I can tell you that things don't work very well if we don't know the aperture. We can make assumptions, like the root bus is 00, and then enumerate everything reachable from there. But then all we know is the largest bus number actually reachable from bus 00, which is usually smaller then the end of the aperture. We don't know how many unused bus numbers there are in the aperture, so we can't safely allocate any for hot-added devices.

Quoted 37 lines

This is in fact somewhat counter-productive because it requires you to hard-code the PCI hierarchy within the DT and you loose a lot of the probing functionality. This is not much of an issue for typical PCI endpoints (like network cards) but becomes a problem if you have a PCI endpoint that implements an I2C bus and you want to match I2C slaves on that bus to device nodes so that the kernel can parse and instantiate them properly. I guess in the latter cases hard-coding the PCI tree in DT is not actually a drawback, though.

Quoted 3 lines

I see mention of config space. ?Is some of that referring to the ECAM as in 7.2.2 of the PCIe spec v3.0 (what we refer to as MMCONFIG on x86)?

I only have access to the PCIe 2.0 specification, but it seems to contain the same 7.2.2 section. In fact it looks as though this particular controller indeed implements something similar to ECAM. The address mapping is a little different, though:

? ? ? ?A[(16 + n - 1):16]: bus number
? ? ? ?A[15:11]: device number
? ? ? ?A[10:8]: function number
? ? ? ?A[7:2]: register number
? ? ? ?A[1:0]: unused

That information comes directly from the driver and I have no access to the corresponding documentation. It seems like the extended configuration space is accessed using the same mapping but starting at a different base address.

But now that you mention it, maybe the driver code is buggy here, and the controller indeed implements ECAM. Furthermore it also seems like the current code doesn't work for the extended configuration space because while the correct offset into the extended configuration space is chosen, the access to registers is done using the same mapping as above, so instead of the 12 (10) bits required for the register number only 8 (6) are in fact used.

Quoted 5 lines

I wonder whether anyone's actually tested this.

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

It'd be a real shame if they made up a proprietary scheme when ECAM seems to be required by the spec. I'm interested because the current MMCONFIG code seems unnecessarily x86-specific, and I expect every arch with PCIe would want to use ECAM, so maybe there's some chance for a generic implementation.

Quoted 12 lines · Bjorn Helgaas

Quoted 9 lines · Thierry Reding

Quoted 2 lines

Is the bus number aperture included somewhere? How do we know what bus numbers are available for allocation under each bridge?

Not yet. I don't think DT imposes a bus number allocation on PCI bridges. However the matching of DT nodes to PCI bridges is done based on the bus number. For that you provide a bus-ranges property which defines the bus aperture of the given PCI bridge. The DT matching code compares the first cell of this property with the primary bus number of the bridge.

Quoted 8 lines

I don't fully understand this, but I can tell you that things don't work very well if we don't know the aperture. We can make assumptions, like the root bus is 00, and then enumerate everything reachable from there. But then all we know is the largest bus number actually reachable from bus 00, which is usually smaller then the end of the aperture. We don't know how many unused bus numbers there are in the aperture, so we can't safely allocate any for hot-added devices.

I think DT support for PCI is lacking in a lot of areas. PowerPC seems to be the only architecture actively setting up busses according to the bus-range property specified in the DT. All other architectures seem to not pre-allocate bus apertures and go with the default instead. From what you say that means hot-plugging is out. Although I don't think the Tegra PCIe controller even supports hot-plugging.

Quoted 13 lines · Thierry Reding

Quoted 12 lines · Bjorn Helgaas

Quoted 9 lines · Thierry Reding

Quoted 2 lines

Is the bus number aperture included somewhere? ?How do we know what bus numbers are available for allocation under each bridge?

Not yet. I don't think DT imposes a bus number allocation on PCI bridges. However the matching of DT nodes to PCI bridges is done based on the bus number. For that you provide a bus-ranges property which defines the bus aperture of the given PCI bridge. The DT matching code compares the first cell of this property with the primary bus number of the bridge.

Quoted 8 lines

Quoted 8 lines

I don't fully understand this, but I can tell you that things don't work very well if we don't know the aperture. ?We can make assumptions, like the root bus is 00, and then enumerate everything reachable from there. ?But then all we know is the largest bus number actually reachable from bus 00, which is usually smaller then the end of the aperture. ?We don't know how many unused bus numbers there are in the aperture, so we can't safely allocate any for hot-added devices.

I believe the assumption today is that all bus numbers are ok on each root port, which is usually the case. There was something about a Power Mac that faked a single PCI bus number space across both an AGP and a PCIe domain in pmac_pci_fixup_u4_of_node(), but I think that is the exception.

Is there any requirement to use a bus aperture on non-PC hardware?

Quoted 6 lines

I think DT support for PCI is lacking in a lot of areas. PowerPC seems to be the only architecture actively setting up busses according to the bus-range property specified in the DT. All other architectures seem to not pre-allocate bus apertures and go with the default instead. From what you say that means hot-plugging is out. Although I don't think the Tegra PCIe controller even supports hot-plugging.

If the default is allowing any bus numbers, and that matches the hardware capabilities, I think hot-plugging would work.

? ? ? ?Arnd

The requirement (if there is one) isn't anything related to PC-ness. I just don't understand how things can actually work if two host bridges both claim the same bus number. If we do a config read to that bus, both bridges should claim it and turn it into config cycles on their respective root buses, and we should get two responses. I would expect the second response to cause an "unexpected response" machine check or similar.

Quoted 7 lines · Bjorn Helgaas

The requirement (if there is one) isn't anything related to PC-ness. I just don't understand how things can actually work if two host bridges both claim the same bus number. ?If we do a config read to that bus, both bridges should claim it and turn it into config cycles on their respective root buses, and we should get two responses. ?I would expect the second response to cause an "unexpected response" machine check or similar.

But each PCI domain has its own config space, so if you do a config read for one bus, it's always relative to the domain.

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · Bjorn Helgaas

The requirement (if there is one) isn't anything related to PC-ness. I just don't understand how things can actually work if two host bridges both claim the same bus number. If we do a config read to that bus, both bridges should claim it and turn it into config cycles on their respective root buses, and we should get two responses. I would expect the second response to cause an "unexpected response" machine check or similar.

Quoted 2 lines

But each PCI domain has its own config space, so if you do a config read for one bus, it's always relative to the domain.

Oh, I see! I totally missed the fact that each host bridge was in its own domain. If each has its own domain, then the bus number aperture can certainly be [bus 00-ff] and there's no problem.

The part that is specific to PCs is that they don't have PCI domains normally.

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

On 06/22/2012 05:00 AM, Thierry Reding wrote:
...

Quoted 3 lines

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

Sure. I have added this request to the bug I filed requesting more complete PCIe host documentation.

Quoted 6 lines · Stephen Warren

On 06/22/2012 05:00 AM, Thierry Reding wrote:
...

Quoted 3 lines

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

Quoted 2 lines

Sure. I have added this request to the bug I filed requesting more complete PCIe host documentation.

I've received unofficial confirmation that we do indeed implement a non-standard/non-ECAM mapping, and what's in our driver matches our HW.

Quoted 6 lines · Stephen Warren

On 06/22/2012 05:00 AM, Thierry Reding wrote:
...

Quoted 3 lines

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

Quoted 2 lines

Sure. I have added this request to the bug I filed requesting more complete PCIe host documentation.

I've received unofficial confirmation that we do indeed implement a non-standard/non-ECAM mapping, and what's in our driver matches our HW.

Quoted 9 lines · Stephen Warren

Quoted 7 lines · Stephen Warren

On 06/22/2012 05:00 AM, Thierry Reding wrote: ...

Quoted 3 lines

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

Sure. I have added this request to the bug I filed requesting more complete PCIe host documentation.

Quoted 3 lines

I've received unofficial confirmation that we do indeed implement a non-standard/non-ECAM mapping, and what's in our driver matches our HW.

What I don't quite see yet is how the extended configuration space is supposed to work with the current driver. The PCIE_CONF_* macros don't provide for registers >= 256. Passing a value higher than that will mess with the device function field.

Quoted 10 lines · Thierry Reding

Quoted 9 lines · Stephen Warren

Quoted 7 lines · Stephen Warren

On 06/22/2012 05:00 AM, Thierry Reding wrote: ...

Quoted 3 lines

Stephen: can you try to find out whether the Tegra PCIe controller indeed implements ECAM, or if this scheme is actually just a proprietary variant?

Sure. I have added this request to the bug I filed requesting more complete PCIe host documentation.

Quoted 8 lines

Quoted 3 lines

I've received unofficial confirmation that we do indeed implement a non-standard/non-ECAM mapping, and what's in our driver matches our HW.

What I don't quite see yet is how the extended configuration space is supposed to work with the current driver. The PCIE_CONF_* macros don't provide for registers >= 256. Passing a value higher than that will mess with the device function field.

The current downstream driver is incorrect for that case. We should be updating it (at least, I filed a bug to do this).

Here's how the HW works (I believe this information should be in some future version of the TRM):

There are 3 address spaces:

* The CPU bus.

* An internal (to the PCIe controller) 40-bit address space. Apparently the layout is HyperTransport-based. Whether HT defines this, or whether our HW engineers are referring to our particular implementation of HT, I'm not sure.

* The PCIe external bus.

Accesses from the CPU to the PCIe controller's 1GB aperture are mapped into the 40-bit bus using the BAR configurations in the PCIe controller; I believe this is what tegra_pcie_setup_translations() is configuring in our downstream driver.

Accesses are then mapped from this 40-bit bus to the external 32-bit bus, I believe using a hard-coded mapping, which I believe may be inherited from (our implementation of?) HyperTransport

For config and extended config accesses the mapping from the internal to external bus is as follows:

In the case of PCICFG space,
addr[39:28]=12'hFDF,
addr[27:24]=4'hE means type 0 and addr[27:24]=4'hF means type 1, addr[23:16]=bus number
addr[15:11]=device number
addr[10:8]=function number
addr[7:0]=register number

In the case of EXTCFG space,
addr[39:32]=8'hFE,
addr[31:28]=4'h0 means type 0 and addr[31:28]=4'h1 means type 1, addr[27:24]=upper register number (i.e. register number[11:8]) addr[23:16]=bus number
addr[15:11]=device number
addr[10:8]=function number
addr[7:0]=register number (i.e. register number[7:0])

(in actual fact, the HW matches the top 16 or 12 bits to determine config/ext-config and transaction type, so the top two fields in the lists above should really be considered merged)

I hope this helps!

Everybody seems to be happy with this approach, so I'll give it a shot. There is one thing I'm still unsure about, though. What if somebody uses the above scheme and maps the registers to the wrong port. The same goes for the nvidia,ctrl-offset property. It needs to match the register offset because they are directly related. I suppose we could leave that property away and look up the register via the port index (which, as Stephen already said, we'll have to do in other places anyway, unless we list all bit positions in the DT).

Can we safely ignore such issues and assume the device tree to always be right? Should we just not care if somebody uses it wrongly?

Quoted 133 lines · Mitch Bradley

Quoted 114 lines · Thierry Reding

Quoted 41 lines · Thierry Reding

Quoted 35 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

Quoted 6 lines

This really depends on the device vs. no device decision below. If we can make it work without needing an extra device for it, then using the index is certainly better. However, if we instantiate devices from the DT, then we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the port at address 0x80000000 to 2?).

An additional problem with this is that we'd have to add the following to the pcie-controller node:

	#address-cells = <1>;
	#size-cells = <0>;

This will conflict with the "ranges" property, because suddenly we can no longer map the regions properly. Maybe Mitch can comment on whether this is possible or not?

To make it clearer what I'm talking about, here's the DT snippet again (with the compatible property removed from the pci@ nodes because they are no longer probed by a driver, the "simple-bus" removed from the pcie-controller node's compatible property removed and its #address- and #size-cells properties adjusted as described above).

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <0>;

		pci at 0 {
			reg = <2>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80400000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x90000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};

		pci at 1 {
			reg = <1>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80408000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x98000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};
	};

Quoted 4 lines

AIUI none of the ranges properties are valid anymore, because the bus represented by pcie-controller no longer reflects the truth, namely that it translates the CPU address space to the PCI address space.

I think you can use a small-integer port number as an address by defining the intermediate address space properly, and using appropriate ranges above and below. Here's a swag at how that would look.

I present three versions, using different choices for the intermediate address space encoding. The intermediate address space may seem somewhat artificial, in that it decouples the "linear pass through of ranges" between the lower PCI address space and the upper CPU address space. But in another sense, it accurately reflects that fact that the bus bridge "slices and dices" that linear address space into non-contiguous pieces.

Quoted 3 lines

Note that I'm also fixing a problem that I neglected to mention earlier - namely the fact that config space is part of the child PCI address space so it must be passed through.

This doesn't quite match how the Tegra PCIe controller works. Basically, every access to a device's (extended) configuration space goes through a single region of memory-mapped registers, independent of which port is the parent of the device.

Is it okay to pass both configuration spaces via the ranges property but still list them in the reg property of the controller?

Quoted 65 lines

Version A - 3 address cells:  In this version, the intermediate
address space has 3 cells:  port#, address type, offset.  Address

type is
0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0 0 0  0x80000000 0x00001000   /* Root port 0 */
			  0 1 0  0x80004000 0x00080000   /* Port 0 config space */
			  0 2 0  0x80104000 0x00080000   /* Port 0 ext config space *
			  0 3 0  0x80400000 0x00008000   /* Port 0 downstream I/O */
			  0 4 0  0x90000000 0x08000000   /* Port 0 non-prefetchable memory */
			  0 5 0  0xa0000000 0x08000000   /* Port 0 prefetchable memory */

			  1 0 0  0x80001000 0x00001000   /* Root port 1 */
			  1 1 0  0x80004000 0x00080000   /* Port 1 config space */
			  1 2 0  0x80184000 0x00080000   /* Port 1 ext config space */
			  1 3 0  0x80408000 0x00010000   /* Port 1 downstream I/O */
			  1 4 0  0x98000000 0x08000000   /* Port 1 non-prefetchable memory */
			  1 5 0  0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

		#address-cells = <3>;
		#size-cells = <1>;

		pci at 0 {
			reg = <0 0 0 0x1000>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x80000000 0 0  0 1 0  0 0x00080000   /* config */
				  0x90000000 0 0  0 2 0  0 0x00080000   /* extended config */
				  0x81000000 0 0  0 3 0  0 0x00008000   /* I/O */
				  0x82000000 0 0  0 4 0  0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0  0 5 0  0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};


		pci at 1 {
			reg = <1 0 0 0x1000>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x80000000 0 0  1 1 0  0 0x00080000   /* config */

Quoted 8 lines

				  0x90000000 0 0  1 2 0  0 0x00080000   /* extended config */
				  0x81000000 0 0  1 3 0  0 0x00008000   /* I/O */
				  0x82000000 0 0  1 4 0  0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0  1 5 0  0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};

Everybody seems to be happy with this approach, so I'll give it a shot. There is one thing I'm still unsure about, though. What if somebody uses the above scheme and maps the registers to the wrong port. The same goes for the nvidia,ctrl-offset property. It needs to match the register offset because they are directly related. I suppose we could leave that property away and look up the register via the port index (which, as Stephen already said, we'll have to do in other places anyway, unless we list all bit positions in the DT).

Can we safely ignore such issues and assume the device tree to always be right? Should we just not care if somebody uses it wrongly?

Thierry

Quoted 114 lines · Thierry Reding

Quoted 41 lines · Thierry Reding

Quoted 35 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

Quoted 6 lines

This really depends on the device vs. no device decision below. If we can make it work without needing an extra device for it, then using the index is certainly better. However, if we instantiate devices from the DT, then we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the port at address 0x80000000 to 2?).

An additional problem with this is that we'd have to add the following to the pcie-controller node:

	#address-cells = <1>;
	#size-cells = <0>;

This will conflict with the "ranges" property, because suddenly we can no longer map the regions properly. Maybe Mitch can comment on whether this is possible or not?

To make it clearer what I'm talking about, here's the DT snippet again (with the compatible property removed from the pci@ nodes because they are no longer probed by a driver, the "simple-bus" removed from the pcie-controller node's compatible property removed and its #address- and #size-cells properties adjusted as described above).

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <0>;

		pci at 0 {
			reg = <2>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80400000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x90000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};

		pci at 1 {
			reg = <1>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80408000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x98000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};
	};

Quoted 4 lines

AIUI none of the ranges properties are valid anymore, because the bus represented by pcie-controller no longer reflects the truth, namely that it translates the CPU address space to the PCI address space.

I think you can use a small-integer port number as an address by defining the intermediate address space properly, and using appropriate ranges above and below. Here's a swag at how that would look.

I present three versions, using different choices for the intermediate address space encoding. The intermediate address space may seem somewhat artificial, in that it decouples the "linear pass through of ranges" between the lower PCI address space and the upper CPU address space. But in another sense, it accurately reflects that fact that the bus bridge "slices and dices" that linear address space into non-contiguous pieces.

Note that I'm also fixing a problem that I neglected to mention earlier - namely the fact that config space is part of the child PCI address space so it must be passed through.

Version A - 3 address cells:  In this version, the intermediate
address space has 3 cells:  port#, address type, offset.  Address

type is
0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0 0 0  0x80000000 0x00001000   /* Root port 0 */
			  0 1 0  0x80004000 0x00080000   /* Port 0 config space */
			  0 2 0  0x80104000 0x00080000   /* Port 0 ext config space *
			  0 3 0  0x80400000 0x00008000   /* Port 0 downstream I/O */
			  0 4 0  0x90000000 0x08000000   /* Port 0 non-prefetchable memory */
			  0 5 0  0xa0000000 0x08000000   /* Port 0 prefetchable memory */

			  1 0 0  0x80001000 0x00001000   /* Root port 1 */
			  1 1 0  0x80004000 0x00080000   /* Port 1 config space */
			  1 2 0  0x80184000 0x00080000   /* Port 1 ext config space */
			  1 3 0  0x80408000 0x00010000   /* Port 1 downstream I/O */
			  1 4 0  0x98000000 0x08000000   /* Port 1 non-prefetchable memory */
			  1 5 0  0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

		#address-cells = <3>;
		#size-cells = <1>;

		pci at 0 {
			reg = <0 0 0 0x1000>;

		};

		pci at 1 {
			reg = <1 0 0 0x1000>;

		};

Quoted 177 lines · Mitch Bradley

Quoted 114 lines · Thierry Reding

Quoted 41 lines · Thierry Reding

Quoted 35 lines · Stephen Warren

Quoted 2 lines · Thierry Reding

Quoted 1 line · Stephen Warren

On 06/14/2012 03:19 AM, Thierry Reding wrote:

...

Quoted 24 lines

Quoted 18 lines

Quoted 3 lines

#address-cells = <1>; #size-cells = <1>;

pci at 80000000 {

I'm still not convinced that using the address of the port's registers is the correct way to represent each port. The port index seems much more useful.

The main reason here is that there are a lot of registers that contain fields for each port - far more than the combination of this node's reg and ctrl-offset (which I assume is an address offset for just one example of this issue) properties can describe. The bit position and bit stride of these fields isn't necessarily the same in each register. Do we want a property like ctrl-offset for every single type of field in every single shared register that describes the location of the relevant data, or just a single "port ID" bit that can be applied to anything?

Quoted 5 lines

(Perhaps this isn't so obvious looking at the TRM since it doesn't document all registers, and I'm also looking at the Tegra30 documentation too, which might be more exposed to this - I haven't correlated all the documentation sources to be sure though)

Quoted 6 lines

I agree that maybe adding properties for each bit position or register offset may not work out too well. But I think it still makes sense to use the base address of the port's registers (see below). We could of course add some code to determine the index from the base address at initialization time and reuse the index where appropriate.

Quoted 4 lines

To me, working back from address to ID then using the ID to calculate some other addresses seems far more icky than just calculating all the addresses based off of one ID. But, I suppose this doesn't make a huge practical difference.

Quoted 6 lines

This really depends on the device vs. no device decision below. If we can make it work without needing an extra device for it, then using the index is certainly better. However, if we instantiate devices from the DT, then we have the address anyway and adding the index as a property would be redundant and error prone (what happens if somebody sets the index of the port at address 0x80000000 to 2?).

An additional problem with this is that we'd have to add the following to the pcie-controller node:

	#address-cells = <1>;
	#size-cells = <0>;

This will conflict with the "ranges" property, because suddenly we can no longer map the regions properly. Maybe Mitch can comment on whether this is possible or not?

To make it clearer what I'm talking about, here's the DT snippet again (with the compatible property removed from the pci@ nodes because they are no longer probed by a driver, the "simple-bus" removed from the pcie-controller node's compatible property removed and its #address- and #size-cells properties adjusted as described above).

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <0>;

		pci at 0 {
			reg = <2>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80400000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x90000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa0000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x110>;
			nvidia,num-lanes = <2>;
		};

		pci at 1 {
			reg = <1>;
			status = "disabled";

			#address-cells = <3>;
			#size-cells = <2>;

			ranges = <0x81000000 0 0 0x80408000 0 0x00008000   /* I/O */
				  0x82000000 0 0 0x98000000 0 0x08000000   /* non-prefetchable memory */
				  0xc2000000 0 0 0xa8000000 0 0x08000000>; /* prefetchable memory */

			nvidia,ctrl-offset = <0x118>;
			nvidia,num-lanes = <2>;
		};
	};

Quoted 4 lines

AIUI none of the ranges properties are valid anymore, because the bus represented by pcie-controller no longer reflects the truth, namely that it translates the CPU address space to the PCI address space.

I think you can use a small-integer port number as an address by defining the intermediate address space properly, and using appropriate ranges above and below. Here's a swag at how that would look.

I present three versions, using different choices for the intermediate address space encoding. The intermediate address space may seem somewhat artificial, in that it decouples the "linear pass through of ranges" between the lower PCI address space and the upper CPU address space. But in another sense, it accurately reflects that fact that the bus bridge "slices and dices" that linear address space into non-contiguous pieces.

Note that I'm also fixing a problem that I neglected to mention earlier - namely the fact that config space is part of the child PCI address space so it must be passed through.

Version A - 3 address cells:  In this version, the intermediate
address space has 3 cells:  port#, address type, offset.  Address

type is
0 : root port
1 : config space
2 : extended config space
3 : I/O
4 : non-prefetchable memory
5 : prefetchable memory.

The third cell "offset" is necessary so that the size field has a number space that can include it.

	pcie-controller {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200>; /* extended configuration space */
		interrupts = <0 98 0x04   /* controller interrupt */
			      0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0 0 0  0x80000000 0x00001000   /* Root port 0 */
			  0 1 0  0x80004000 0x00080000   /* Port 0 config space */
			  0 2 0  0x80104000 0x00080000   /* Port 0 ext config space *
			  0 3 0  0x80400000 0x00008000   /* Port 0 downstream I/O */
			  0 4 0  0x90000000 0x08000000   /* Port 0 non-prefetchable memory */
			  0 5 0  0xa0000000 0x08000000   /* Port 0 prefetchable memory */

			  1 0 0  0x80001000 0x00001000   /* Root port 1 */
			  1 1 0  0x80004000 0x00080000   /* Port 1 config space */
			  1 2 0  0x80184000 0x00080000   /* Port 1 ext config space */
			  1 3 0  0x80408000 0x00010000   /* Port 1 downstream I/O */
			  1 4 0  0x98000000 0x08000000   /* Port 1 non-prefetchable memory */
			  1 5 0  0xa0000000 0x08000000>; /* Port 1 prefetchable memory */

		#address-cells = <3>;
		#size-cells = <1>;

		pci at 0 {
			reg = <0 0 0 0x1000>;

[...]

Quoted 2 lines

		};

Quoted 2 lines

		pci at 1 {
			reg = <1 0 0 0x1000>;

[...]

Quoted 1 line

		};

It seems like this isn't working properly. For some reason both the reg property of pci at 0 and pci at 1 are translated to the same parent address 0x80000000. I'll have to investigate where exactly this goes wrong.

Quoted 3 lines

It seems like this isn't working properly. For some reason both the reg property of pci at 0 and pci at 1 are translated to the same parent address 0x80000000. I'll have to investigate where exactly this goes wrong.

So it turns out that while of_read_number() can actually read any number of cells, only the two least significant are kept because it returns a u64. Since of_bus_default_map() uses of_read_number() the addresses <0 0 0> and <1 0 0> are in fact the same. I'm not sure how best to solve this. I'm not aware of a 128 bit integer type in the kernel, so I guess the better alternative would be to fix of_bus_default_map() to cope with #address-cells > 2 properly.

Quoted 11 lines · Thierry Reding

Quoted 3 lines

It seems like this isn't working properly. For some reason both the reg property of pci at 0 and pci at 1 are translated to the same parent address 0x80000000. I'll have to investigate where exactly this goes wrong.

So it turns out that while of_read_number() can actually read any number of cells, only the two least significant are kept because it returns a u64. Since of_bus_default_map() uses of_read_number() the addresses <0 0 0> and <1 0 0> are in fact the same. I'm not sure how best to solve this. I'm not aware of a 128 bit integer type in the kernel, so I guess the better alternative would be to fix of_bus_default_map() to cope with #address-cells > 2 properly.

of_translate_address should get it right. Which codes uses of_read_number()? Can it be converted to use of_translate_address()?

Actually this is from of_translate_address(). The calling sequence looks like this:

        of_address_to_resource()
          __of_address_to_resource()
            of_translate_address()
              __of_translate_address()
                of_translate_one()
                  of_bus_default_map()
                    of_read_number()

                pci at 80000000 {
                        reg = <0x80000000 0x00001000>;
                        status = "disabled";

                        #address-cells = <3>;
                        #size-cells = <2>;

                        ranges = <0x80400000 0x80400000 0x00008000   /* I/O */
                                  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
                                  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

                        nvidia,ctrl-offset = <0x0>;
                        nvidia,num-lanes = <2>;
                };

Quoted 11 lines · Thierry Reding

                pci at 80000000 {
                        reg = <0x80000000 0x00001000>;
                        status = "disabled";

                        #address-cells = <3>;
                        #size-cells = <2>;

                        ranges = <0x80400000 0x80400000 0x00008000   /* I/O */
                                  0x90000000 0x90000000 0x08000000   /* non-prefetchable memory */
                                  0xa0000000 0xa0000000 0x08000000>; /* prefetchable memory */

Quoted 4 lines

                        nvidia,ctrl-offset = <0x0>;
                        nvidia,num-lanes = <2>;
                };

I believe you will need an "interrupt-map" property here, to map the host interrupts to the INTA-INTD lines of the attached devices.

I'm not sure whether we want to have a device_type="pciex" property here. powerpc and sparc seem to use that information, to distinguish a pcie bus from pci or cardbus.

Quoted 3 lines

I believe you will need an "interrupt-map" property here, to map the host interrupts to the INTA-INTD lines of the attached devices.

Legacy interrupts are something I cannot test at all because I have no hardware that supports them.

Quoted 3 lines

I'm not sure whether we want to have a device_type="pciex" property here. powerpc and sparc seem to use that information, to distinguish a pcie bus from pci or cardbus.

That'd be rather useless information given that the Tegra is unlikely to support either PCI or CardBus at some point.

Quoted 6 lines · Thierry Reding

Quoted 3 lines

I believe you will need an "interrupt-map" property here, to map the host interrupts to the INTA-INTD lines of the attached devices.

Legacy interrupts are something I cannot test at all because I have no hardware that supports them.

Hmm, I thought all PCIe hardware has to support them when you do not enable MSI. What hardware do you have then?

Quoted 6 lines

Quoted 3 lines

I'm not sure whether we want to have a device_type="pciex" property here. powerpc and sparc seem to use that information, to distinguish a pcie bus from pci or cardbus.

That'd be rather useless information given that the Tegra is unlikely to support either PCI or CardBus at some point.

But the generic code does not know that.

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · Thierry Reding

Quoted 3 lines

I believe you will need an "interrupt-map" property here, to map the host interrupts to the INTA-INTD lines of the attached devices.

Quoted 5 lines

Quoted 2 lines

Legacy interrupts are something I cannot test at all because I have no hardware that supports them.

Hmm, I thought all PCIe hardware has to support them when you do not enable MSI. What hardware do you have then?

The TEC (Tamonten Evaluation Carrier) has an FPGA which is connected to one of the Tegra PCIe ports and it only supports MSIs.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

I'm not sure whether we want to have a device_type="pciex" property here. powerpc and sparc seem to use that information, to distinguish a pcie bus from pci or cardbus.

That'd be rather useless information given that the Tegra is unlikely to support either PCI or CardBus at some point.

But the generic code does not know that.

True. Still there's not much generic code on ARM, so maybe it'd be better to add it along with the corresponding code?

Quoted 12 lines · Thierry Reding

Quoted 10 lines · Arnd Bergmann

Quoted 6 lines · Thierry Reding

Quoted 3 lines

I believe you will need an "interrupt-map" property here, to map the host interrupts to the INTA-INTD lines of the attached devices.

Legacy interrupts are something I cannot test at all because I have no hardware that supports them.

Hmm, I thought all PCIe hardware has to support them when you do not enable MSI. What hardware do you have then?

Quoted 2 lines

The TEC (Tamonten Evaluation Carrier) has an FPGA which is connected to one of the Tegra PCIe ports and it only supports MSIs.

Ah, I see. FPGA based devices often have incomplete PCI support so that explains why you can't test it. The board also appears to have a PCIe mini port though, so anything that you could plug in there should also support LSI interrupts.

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

I'm not sure whether we want to have a device_type="pciex" property here. powerpc and sparc seem to use that information, to distinguish a pcie bus from pci or cardbus.

That'd be rather useless information given that the Tegra is unlikely to support either PCI or CardBus at some point.

But the generic code does not know that.

Quoted 2 lines

True. Still there's not much generic code on ARM, so maybe it'd be better to add it along with the corresponding code?

I hope we can make the PCI host probing architecture independent one day, instead of each architecture implementing their own. I'd say better put that property in there so we don't have to change it in case the future generic code will need it. It is part of the binding at
http://www.openfirmware.org/1275/bindings/pci/pci-express.txt after all.

Note that we should generally not make /code/ be written with the anticipation of a future extension, but anything that concerns interfaces to code outside of the kernel (firmware or user space) is best written in a conservative way to allow later changes.

On 06/12/2012 11:20 AM, Thierry Reding wrote:
...

Quoted 36 lines

I came up with the following alternative:

	pci {
		compatible = "nvidia,tegra20-pcie";
		reg = <0x80003000 0x00000800   /* PADS registers */
		       0x80003800 0x00000200   /* AFI registers */
		       0x80004000 0x00100000   /* configuration space */
		       0x80104000 0x00100000   /* extended configuration space */
		       0x80400000 0x00010000   /* downstream I/O */
		       0x90000000 0x10000000   /* non-prefetchable memory */
		       0xa0000000 0x10000000>; /* prefetchable memory */
		interrupts = <0 98 0x04   /* controller interrupt */
		              0 99 0x04>; /* MSI interrupt */
		status = "disabled";

		ranges = <0x80000000 0x80000000 0x00002000   /* 2 root ports */
			  0x80004000 0x80004000 0x00100000   /* configuration space */
			  0x80104000 0x80104000 0x00100000   /* extended configuration space */
			  0x80400000 0x80400000 0x00010000   /* downstream I/O */
			  0x90000000 0x90000000 0x10000000   /* non-prefetchable memory */
			  0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */

		#address-cells = <1>;
		#size-cells = <1>;

		port at 80000000 {
			reg = <0x80000000 0x00001000>;
			status = "disabled";
		};

		port at 80001000 {
			reg = <0x80001000 0x00001000>;
			status = "disabled";
		};
	};

Quoted 6 lines

The "ranges" property can probably be cleaned up a bit, but the most interesting part is the port@ children, which can simply be enabled in board DTS files by setting the status property to "okay". I find that somewhat more intuitive to the variant with an "enable-ports" property.

What do you think of this?

As a general concept, this kind of design seems OK to me.

The "port" child nodes I think should be named "pci at ..." given Mitch's comments, I think.

The port nodes probably need two entries in reg, given the following in our downstream driver:

Quoted 2 lines

        int rp_offset = 0;
        int ctrl_offset = AFI_PEX0_CTRL;

...

Quoted 6 lines

        for (port = 0; port < MAX_PCIE_SUPPORTED_PORTS; port++) {
                ctrl_offset += (port * 8);
                rp_offset = (rp_offset + 0x1000) * port;
                if (tegra_pcie.plat_data->port_status[port])
                        tegra_pcie_add_port(port, rp_offset, ctrl_offset);
        }

(which actually looks likely to be horribly buggy for port>1 and only accidentally correct for port==1, but anyway...)

But instead, I'd be tempted to make the top-level node say:

	#address-cells = <1>;
	#size-cells = <0>;

... so that the child nodes' reg is just the port ID. The parent node can calculate the addresses/offsets of the per-port registers within the PCIe controller's register space based on the ID using code roughly like what I quoted above:

	pci at 0 {
		reg = <0>;
		status = "disabled";
	};

	pci at 1 {
		reg = <0>;
		status = "disabled";
	};

That would save having to put 2 entries in the reg, and perhaps remove the need for any ranges property.

I think you also need a property to specify the exact port layout; the Tegra20 controller supports:

1 x4 port
2 x2 ports (you can choose to use only 1 of these I assume)

So just because only 1 of the ports is enabled, doesn't imply it's x4; it could still be x2.

Tegra30 has more options.

* Stephen Warren wrote:

Quoted 10 lines

I think you also need a property to specify the exact port layout; the Tegra20 controller supports:

1 x4 port 2 x2 ports (you can choose to use only 1 of these I assume)

So just because only 1 of the ports is enabled, doesn't imply it's x4; it could still be x2.

Tegra30 has more options.

Both the upstream and downstream drivers currently hard-code this to dual and 411 (I assume that means 1x4, 2x1?) configurations for Tegra20 and Tegra30 respectively. Unfortunately the register AFI_PCIE_CONFIG isn't documented on Tegra20 at all and for Tegra30 lists only the valid configurations (see 28.4.1.5 PCIe CONFIG) but not the corresponding encodings.