On Wednesday 06 February 2013, Thomas Petazzoni wrote:

Dear Arnd Bergmann,

On Wed, 6 Feb 2013 22:41:14 +0000, Arnd Bergmann wrote:

quoted

I've been thinking some more about this, and I wonder if it would
make more sense to describe the address remapping correctly as
a node on top of the pcie-controller node.

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.

Hum, good you give a skeleton example, because I'm not sure to
understand your suggestion.

I mean (roughly, since I don't know how that hardware defines it)

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

	memory at 0 {
		/* this node can not get remapped */
		reg = <0x0 0x40000000>;
	};

	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 */
		};
	};
};

quoted

quoted

+                       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.

Is it really wise to have DT properties that are not used by anything,
and therefore have a very high chance of either being incorrect, or
becoming incorrect?

In this case, definitely. It's mandated by an IEEE standard and the only reason
why we let some mistakes slip here is that some ancient PowerMac systems
get it wrong in their firmware. There are lots of cases where the difference
between an empty "ranges" property and an absent one is very significant
and I would really prefer to change the kernel to be standard compliant
here.

	Arnd