PCIe support for the Armada 370 and Armada XP SoCs

	address-map {
		/* this device translates 64 bit MMIO bus addresses into 32 bit CPU addresses */
		compatible = "marvell,armada-addr-decoding-controller";
		reg = <0xd0020000 0x258>;
		#addres-cells = <2>;
		#address-cells = <1>;

		/* each remapped window has one entry here */
		ranges = <0xa 0 0xc0000000 0x10000>,     /* map window a to 0xc0000000 */
		         <0xb 0 0xc1000000 0x08000000>,  /* map window b to 0xc1000000 */
			 <...>; /* more windows */

		pciex {
			#addres-cells = <3>;
			#size-cells = <2>;
			ranges = <0x81000000 0 0 0xa 0 0 0x00010000,  /* I/O is window a */
				  0x82000000 0 0 0xb 0 0 0x08000000>; /* non-prefetchable memory */
			...
		};

		something-else {
			...
			reg = <0xc 0 0x10000>; /* window c */
		};
	};
};

Unfortunately, this means the windows are statically defined in the DT, which is simply not possible for PCIe, as we have already explained several times in this thread.

Any solution where the PCIe windows are statically described is simply not acceptable.

We have 10 PCIe interfaces, each requiring up to two windows, and we have on the system a total of 20 windows. Doing static assignments of windows is simply not an option.

Of course, you'll tell me that it's up to each board .dts to have a number of windows that matches the number of actually existing PCIe interface. But it means that each and every developer adding the support for a new board must understand this complex problem, which is something we do not want. We have a solution that makes all of this PCIe window assignment dynamic, so it surprises me that we have to continue to explain why a static solution is not appropriate.

This would mean that rather than putting the mapped physical address (0xc0000000, 0xc1000000, ...) in here, you would actually have 64-bit address as the destination as well, in whatever format the address map hardware uses, I assume using a numbered 32 bit address space for each object that can be remapped.

This would also let you do the PCI memory address assignment for each port separately, starting at bus address 0, followed by finding a location in the CPU address space and passing the start as the sys->mem_offset argument to pci_add_resource_offset.

arch/arm/boot/dts/armada-xp-mv78230.dtsi line 115

Quoted 11 lines

arch/arm/boot/dts/armada-xp-mv78230.dtsi +11 -0

+                       pcie at 0,0 {
+                               device_type = "pciex";
+                               reg = <0x0800 0 0xd0040000 0 0x2000>;
+                               #address-cells = <3>;
+                               #size-cells = <2>;
+                               marvell,pcie-port = <0>;
+                               marvell,pcie-lane = <0>;
+                               interrupts = <1>;
+                               clocks = <&gateclk 5>;
+                               status = "disabled";
+                       };

I think you are missing a "ranges" property here, at least an empty one, which is required by the standard but not currently enforced in the code.

Also, what should 'reg' be so that the PCI core binds the OF nodes properly? The standard says reg should have the configuration space address of the bridge, and I noticed Thierry was using something that almost looked like a config space address in his driver..

But that seems overly tricky.. When using the link stanzas, shouldn't this scheme be more like this:

// The PCI-E host bridge

pex at 0 {
   device_type = "pciex"; // <<-- Important!!

   ranges = <
      // Driver internal control registers in MMIO space
      0x82000000 0x10000000 0xd0040000  0xd0040000  0x0 0x8000000

      // CPU 0xe0000000 -> e8000000 bridged to PCI MMIO
      0x82000000 0x00000000 0xe0000000  0xe0000000  0x0 0x8000000
      // CPU IO bus 0x0000000 -> 0xa0000 bridged to PCI IO
      0x81000000 0x00000000 0x00000000  0x00000000  0x0 0xa0000
      // CPU 0xe8000000 -> f0000000 bridged to PCI MMIO, prefetchable
      0xC2000000 0x00000000 0xe8000000  0xe8000000  0x0 0x8000000
      >;

   // PCI-PCI bridge to Physical link 0
   pcie at 0,0 {
     device_type = "pciex";
     // The configuration space address of the PCI-PCI bridge required by OF
     reg = <0x80 0 0  0 0>; // Bus 0, Dev 0x10, Fn 0

     /* The 'bar' on the PCI-PCI bridge, maps to internal control
        registers, required by the driver. */
     assigned-addresses = <0x82000000 0x10000000 0xd0040000  0 0x2000>;
     // ..

}

On Wed, Feb 06, 2013 at 06:05:02PM -0700, Jason Gunthorpe wrote:
[...]

Quoted 11 lines

Also, what should 'reg' be so that the PCI core binds the OF nodes properly? The standard says reg should have the configuration space address of the bridge, and I noticed Thierry was using something that almost looked like a config space address in his driver..

But that seems overly tricky.. When using the link stanzas, shouldn't this scheme be more like this:

// The PCI-E host bridge

pex at 0 {
   device_type = "pciex"; // <<-- Important!!

That might actually work but is somewhat dangerous. I originally tried to trick the OF code into parsing the ranges properly by setting this to "pci". However that breaks horribly because of_bus_pci_match() in drivers/of/address.c will cause the parent bus of the pex at 0 controller to be PCI, which will cause #address-cells == 3 and #size-cells == 2, and thus messing up the address translation because you actually have #address-cells == 1 and #size-cells == 1.

Quoted 3 lines

   ranges = <
      // Driver internal control registers in MMIO space
      0x82000000 0x10000000 0xd0040000  0xd0040000  0x0 0x8000000

I'm not sure I understand what you're doing here. Where does the 0x10000000 in cell 2 come from?

I also just noticed that I used 0x00000800 in the first cell, maybe that should be 0x02000800, though I think that didn't quite work for some reason. I'll need to check that. The odd part about this is that the

address is in fact not within PCI MMIO space at all, so I'm not sure this is even a correct way to represent it. However I find it quite intuitive to do so and it is really the only way to make the translation from the PCI-PCI bridge's reg property work while at the same time having a proper PCI address for the port.

Quoted 5 lines

   // PCI-PCI bridge to Physical link 0
   pcie at 0,0 {
     device_type = "pciex";
     // The configuration space address of the PCI-PCI bridge required by OF
     reg = <0x80 0 0  0 0>; // Bus 0, Dev 0x10, Fn 0

I think the first cell should be 0x800.

Quoted 5 lines

     /* The 'bar' on the PCI-PCI bridge, maps to internal control
        registers, required by the driver. */
     assigned-addresses = <0x82000000 0x10000000 0xd0040000  0 0x2000>;
     // ..

}

The PCI DT binding says that each entry in the assigned-addresses property is to correspond to one of the PCI device's base address registers. So unless this is actually the value that ends up being written to one of the BARs I don't think this is correct.

Quoted 5 lines · Arnd Bergmann

This would mean that rather than putting the mapped physical address (0xc0000000, 0xc1000000, ...) in here, you would actually have 64-bit address as the destination as well, in whatever format the address map hardware uses, I assume using a numbered 32 bit address space for each object that can be remapped.

Do you mean like:

mbus_matrix {
   /* At this level addresses are
       <32 bit MBUS target ID> <32 bit target address offset>
      target_id is an internal MBUS specification
     */
   #address-cells = <2>;
   #size-cells = <1>;

   pex0 {
       ranges = <
      // CPU window bridged to PCI MMIO
      0x82000000 0x00000000 0x00000000  0x<target_id> 0x0 0x0 0x8000000
      // CPU IO window bridged to PCI IO
      0x81000000 0x00000000 0x00000000  0x<target_id> 0x0  0x0 0xa0000
      ...>;
   }

   nand {
       reg = <0x<target_id> 0 0x200>;
   };

   internal_regs {
       ranges = <0 0x<target_id> 0 0x10000>;

       timer {
           reg = <0x20300 0x20>;
      };
   };
}

?

I think the big issue would go back to how to pool all the link decoders together in a way that fits OF and Linux's PCI core will understand?

How does of_translate_address work in a world like this?

Quoted 5 lines

This would also let you do the PCI memory address assignment for each port separately, starting at bus address 0, followed by finding a location in the CPU address space and passing the start as the sys->mem_offset argument to pci_add_resource_offset.

That goes back to the original problem - the goal is to have only one pci_sys_data, not one for every link.

The host driver would have to request a large region of physical address space and still dole it out on a link by link basis. Not sure how to model that in DT??

In any event, changing how all the dynamic windows are configured in DT is a big job (there was another thread about this) it seems orthogonal to the PCI host driver..

Quoted 12 lines

Quoted 11 lines

arch/arm/boot/dts/armada-xp-mv78230.dtsi +11 -0

+                       pcie at 0,0 {
+                               device_type = "pciex";
+                               reg = <0x0800 0 0xd0040000 0 0x2000>;
+                               #address-cells = <3>;
+                               #size-cells = <2>;
+                               marvell,pcie-port = <0>;
+                               marvell,pcie-lane = <0>;
+                               interrupts = <1>;
+                               clocks = <&gateclk 5>;
+                               status = "disabled";
+                       };

Quoted 3 lines

I think you are missing a "ranges" property here, at least an empty one, which is required by the standard but not currently enforced in the code.

Maybe.. according to the standard the ranges in this stanza should reflect the bridge configuration, but that isn't known when the DT is written. An empty ranges means identity and that isn't really right either.

Also, what should 'reg' be so that the PCI core binds the OF nodes properly? The standard says reg should have the configuration space address of the bridge, and I noticed Thierry was using something that almost looked like a config space address in his driver..

But that seems overly tricky.. When using the link stanzas, shouldn't this scheme be more like this:

// The PCI-E host bridge

pex at 0 {
   device_type = "pciex"; // <<-- Important!!

   ranges = <
      // Driver internal control registers in MMIO space
      0x82000000 0x10000000 0xd0040000  0xd0040000  0x0 0x8000000

      // CPU 0xe0000000 -> e8000000 bridged to PCI MMIO
      0x82000000 0x00000000 0xe0000000  0xe0000000  0x0 0x8000000
      // CPU IO bus 0x0000000 -> 0xa0000 bridged to PCI IO
      0x81000000 0x00000000 0x00000000  0x00000000  0x0 0xa0000
      // CPU 0xe8000000 -> f0000000 bridged to PCI MMIO, prefetchable
      0xC2000000 0x00000000 0xe8000000  0xe8000000  0x0 0x8000000
      >;

   //...

   // PCI-PCI bridge to Physical link 0
   pcie at 0,0 {
     device_type = "pciex";
     // The configuration space address of the PCI-PCI bridge required by OF
     reg = <0x80 0 0  0 0>; // Bus 0, Dev 0x10, Fn 0

     /* The 'bar' on the PCI-PCI bridge, maps to internal control
        registers, required by the driver. */
     assigned-addresses = <0x82000000 0x10000000 0xd0040000  0 0x2000>;
     // ..

}
}

Thierry, what do you think?

I struggled with this particular area of the OF system recently and Grant Likely was very helpful - the above is based on that discussion..

======

Thomas, here is one possible alternate idea, not sure on the merit, just including it because it is close to what I've got in my DT right now..:

// The entire multi-lane controller and PCI root bus

pex at 0 {
   device_type = "pciex";

   /* This section configures the Linux PCI host driver. Each line is a
      physical PCI-Link. (Erorrs included :) */
   compatible = "marvell,armada-370-xp-pcie";
   regs = <0xd0040000 0x00002000   // port 0.0
           0xd0042000 0x00002000   // port 2.0
           0xd0044000 0x00002000   // port 0.1
           0xd0048000 0x00002000   // port 0.2
           0xd004C000 0x00002000>; // port 0.3
   io-cpu-window = <0xc0000000 0xa0000>;
   interrupts = <58 59 60 61 99>;
   clocks = <&gateclk 5
             &gateclk 6
             &gateclk 7
             &gateclk 8
             &gateclk 26>
   marvell,port-lane <0 0
                      0 1
                      0 2
                      0 3
                      2 0>;

   /* Below here configures the aggregate PCI bus the PEX0 stanza
      describes */

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

   // CPU resources allocated to this PCI host bridge
   bus-range = <0x00 0xff>;
   ranges = <
      // CPU 0xe0000000 -> e8000000 bridged to PCI MMIO
      0x82000000 0x00000000 0xe0000000  0xe0000000  0x0 0x8000000
      // CPU IO bus 0x0000000 -> 0xa0000 bridged to PCI IO
      0x81000000 0x00000000 0x00000000  0x00000000  0x0 0xa0000
      // CPU 0xe8000000 -> f0000000 bridged to PCI MMIO, prefetchable
      0xC2000000 0x00000000 0xe8000000  0xe8000000  0x0 0x8000000
      >;

   /* Any PCI devices associated with this bus go here, relative to
      the above ranges. As example: */
   link at 0 {
   	device_type = "pciex";
        // Root port at bus 0, device 0x10, function 0
	reg = <0x00000080 0 0  0 0>;

       ep {
   	 device_type = "pciex";
         // End port on bus 1, device 0, function 0
	 reg = <0x00000100 0 0  0 0>;
       } 
   }
}

Thoughts:
- 'regs' in the main stanza, keep to CPU addresses instead of confusing translated fake ranges address

- each regs line is matches to an interrupt, clock and port-lane line to describe a link. The above describes 5 links.

- The CPU physical address window to use for the IO space is set via io-cpu-window, not much choice here, the PCI format ranges must be 0 based.

- It is not necessary to have per-root port stanzas at all with the above.
- There is only one PCI bus stanza, the top level.

Yes, but the mbus-matrix node in the example would need a ranges property to map the addresses according to how the windows are set up.

Quoted 4 lines

Maybe.. according to the standard the ranges in this stanza should reflect the bridge configuration, but that isn't known when the DT is written. An empty ranges means identity and that isn't really right either.

Ok, I thought it was an identity mapping here.

Quoted 4 lines

Also, what should 'reg' be so that the PCI core binds the OF nodes properly? The standard says reg should have the configuration space address of the bridge, and I noticed Thierry was using something that almost looked like a config space address in his driver..

Well, that assumes that a bridge is addressed using configuration space, which IIRC is normally the case but not here.

I never really understood the 'assigned-addresses' property, but it looks sensible.

Review

Quoted 9 lines · Arnd Bergmann

Quoted 3 lines

- The CPU physical address window to use for the IO space is set via io-cpu-window, not much choice here, the PCI format ranges must be 0 based.

I don't think I understand this part. Why can't you put this into ranges as before?

(quoted diff) +1 -1

-       0x81000000 0x00000000 0x00000000  0x00000000  0x0 0xa0000
+	0x81000000 0x00000000 0x00000000  0xc0000000  0x0 0xa0000

The OF PCI core translates 0x81000000 IO space addresess into a 'struct resource' tagged with IORESOURCE_IO.

But 0xc0000000 is not an IORESOURCE_IO address, it is an IORESOURCE_MEM address..

So, I think with the current OF code this has to be 0, otherwise your IORESOURCE_IO's end up starting at 0xc000000 - but maybe there is some trickyness to work with in here? (Although none of this matters when Linux does resource assignment, the OF translation code is never enganged)

But I agree, 0xc0000000 seems much better...

To think about it from a different angle, what would you put in the 4th dword on x86? How do you describe the IO numberspace in DT on x86?